diff --git a/crates/core/src/actionability/mod.rs b/crates/core/src/actionability/mod.rs index 8a07a1f..45c04b2 100644 --- a/crates/core/src/actionability/mod.rs +++ b/crates/core/src/actionability/mod.rs @@ -34,37 +34,35 @@ pub fn check_live( request: &ActionRequest, ) -> Result { let mut observed = entry.clone(); - let live = match adapter.get_live_element(handle) { - Ok(live) => live, + match adapter.get_live_element(handle) { + Ok(live) => { + if live_element_is_stale(&live) { + return Err(AdapterError::new( + ErrorCode::StaleRef, + "Resolved element no longer exposes live accessibility state", + ) + .with_suggestion( + "Re-run a snapshot to obtain fresh refs, then retry with the new ref (CLI: snapshot; FFI: ad_snapshot then ad_execute_by_ref with the returned snapshot_id)", + )); + } + if let Some(state) = live.state { + observed.role = state.role; + observed.states = state.states; + observed.value = state.value.or(observed.value); + } + observed.bounds = live.bounds; + if let Some(actions) = live.available_actions + && !actions.is_empty() + { + observed.available_actions = actions; + } + } Err(err) if matches!( err.code, ErrorCode::PlatformNotSupported | ErrorCode::ActionNotSupported - ) => - { - return check_with_stability(entry.bounds_hash, &observed, request, None); - } + ) => {} Err(err) => return Err(err), - }; - if live_element_is_stale(&live) { - return Err(AdapterError::new( - ErrorCode::StaleRef, - "Resolved element no longer exposes live accessibility state", - ) - .with_suggestion( - "Re-run a snapshot to obtain fresh refs, then retry with the new ref (CLI: snapshot; FFI: ad_snapshot then ad_execute_by_ref with the returned snapshot_id)", - )); - } - if let Some(state) = live.state { - observed.role = state.role; - observed.states = state.states; - observed.value = state.value.or(observed.value); - } - observed.bounds = live.bounds; - if let Some(actions) = live.available_actions - && !actions.is_empty() - { - observed.available_actions = actions; } check_with_stability( entry.bounds_hash, diff --git a/crates/core/src/commands/clipboard_set.rs b/crates/core/src/commands/clipboard_set.rs index fed1e81..f27f6cf 100644 --- a/crates/core/src/commands/clipboard_set.rs +++ b/crates/core/src/commands/clipboard_set.rs @@ -2,7 +2,7 @@ use crate::{ adapter::PlatformAdapter, clipboard_content::ClipboardContent, error::AppError, - image_buffer::{ImageBuffer, ImageFormat}, + image_buffer::{ImageBuffer, ImageFormat, parse_png_dimensions}, }; use serde_json::{Value, json}; use std::path::PathBuf; @@ -28,7 +28,9 @@ fn build_content(args: ClipboardSetArgs) -> Result { } if let Some(path) = args.image { let data = std::fs::read(&path)?; - let (width, height) = png_dimensions(&data); + let Some((width, height)) = parse_png_dimensions(&data) else { + return Err(AppError::invalid_input("--image file is not a valid PNG")); + }; return Ok(ClipboardContent::Image(ImageBuffer { data, format: ImageFormat::Png, @@ -60,15 +62,6 @@ fn validate_file_urls(urls: &[String]) -> Result, AppError> { Ok(urls.to_vec()) } -fn png_dimensions(data: &[u8]) -> (u32, u32) { - if data.len() < 24 { - return (0, 0); - } - let w = u32::from_be_bytes([data[16], data[17], data[18], data[19]]); - let h = u32::from_be_bytes([data[20], data[21], data[22], data[23]]); - (w, h) -} - #[cfg(test)] #[path = "clipboard_set_tests.rs"] mod tests; diff --git a/crates/core/src/commands/clipboard_set_tests.rs b/crates/core/src/commands/clipboard_set_tests.rs index 5745301..654a9ec 100644 --- a/crates/core/src/commands/clipboard_set_tests.rs +++ b/crates/core/src/commands/clipboard_set_tests.rs @@ -126,6 +126,8 @@ fn image_flag_reads_bytes_and_dimensions_from_file() { std::process::id() )); let mut bytes = vec![0u8; 24]; + bytes[0..8].copy_from_slice(&[0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]); + bytes[12..16].copy_from_slice(b"IHDR"); bytes[16..20].copy_from_slice(&11u32.to_be_bytes()); bytes[20..24].copy_from_slice(&8u32.to_be_bytes()); std::fs::write(&path, &bytes).unwrap(); diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index ed8657f..798138d 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -33,6 +33,24 @@ pub(crate) fn resolve_ref_with_context<'a>( snapshot_id: Option<&str>, adapter: &'a dyn PlatformAdapter, context: &CommandContext, +) -> Result<(RefEntry, ResolvedElement<'a>), AppError> { + resolve_ref_within_deadline(ref_id, snapshot_id, None, adapter, context) +} + +/// Single owner of ref resolution and its `ref.resolve.start/entry/ok/error` +/// tracing. With `deadline: None` the strict resolve is uncapped (the +/// `get`/`is`/single-shot pointer path); with `Some(deadline)` each resolve is +/// capped to the remaining budget via +/// [`crate::ref_action_wait::resolve_within_deadline`] so a ref-addressed +/// pointer wait (`hover`, `drag`) cannot spend its whole budget on one slow +/// attempt. Both modes emit identical telemetry, so budgeted and single-shot +/// resolution trace the same way. +fn resolve_ref_within_deadline<'a>( + ref_id: &str, + snapshot_id: Option<&str>, + deadline: Option, + adapter: &'a dyn PlatformAdapter, + context: &CommandContext, ) -> Result<(RefEntry, ResolvedElement<'a>), AppError> { validate_ref_id(ref_id)?; let store = RefStore::for_session(context.session_id())?; @@ -80,7 +98,7 @@ pub(crate) fn resolve_ref_with_context<'a>( "name": entry.name }) })?; - let handle = adapter.resolve_element_strict(&entry).inspect_err(|err| { + let handle = resolve_handle_within_deadline(adapter, &entry, deadline).inspect_err(|err| { let _ = context.trace_lazy("ref.resolve.error", || { json!({ "ref": ref_id, @@ -96,6 +114,26 @@ pub(crate) fn resolve_ref_with_context<'a>( Ok((entry, ResolvedElement::new(adapter, handle))) } +/// Performs the strict resolve for [`resolve_ref_within_deadline`], capping the +/// attempt to `deadline` when one is supplied and surfacing an exhausted budget +/// as a `TIMEOUT`, or resolving uncapped when it is not. +fn resolve_handle_within_deadline( + adapter: &dyn PlatformAdapter, + entry: &RefEntry, + deadline: Option, +) -> Result { + let Some(deadline) = deadline else { + return adapter.resolve_element_strict(entry); + }; + match crate::ref_action_wait::resolve_within_deadline(adapter, entry, deadline) { + crate::ref_action_wait::ResolveAttemptOutcome::Resolved(handle) => Ok(handle), + crate::ref_action_wait::ResolveAttemptOutcome::Failed(err) => Err(err), + crate::ref_action_wait::ResolveAttemptOutcome::DeadlinePassed => Err( + crate::error::AdapterError::timeout("Target did not resolve within the wait budget"), + ), + } +} + pub(crate) fn resolve_app_pid( app: Option<&str>, adapter: &dyn PlatformAdapter, @@ -254,13 +292,44 @@ pub(crate) fn resolve_window_for_app( window_lookup::find_window_for_pid(pid, adapter) } +/// Resolves the ref underlying a wait-budgeted point resolution through the +/// shared [`resolve_ref_within_deadline`] (so the capped pointer-wait path +/// emits the same `ref.resolve.*` telemetry as every other ref resolution and +/// each attempt is capped to what remains of `deadline`), then completes the +/// same bounds/actionability steps [`point_resolve::resolve_point_from_ref_or_xy_with_context`] +/// performs for its ref branch. +fn resolve_point_from_ref_capped( + ref_id: &str, + snapshot_id: Option<&str>, + deadline: std::time::Instant, + adapter: &dyn PlatformAdapter, + context: &CommandContext, +) -> Result { + let (entry, resolved) = + resolve_ref_within_deadline(ref_id, snapshot_id, Some(deadline), adapter, context)?; + let bounds = adapter + .get_element_bounds(resolved.handle())? + .ok_or_else(|| AppError::invalid_input(format!("Element {ref_id} has no bounds")))?; + let point = crate::action::Point { + x: bounds.x + bounds.width / 2.0, + y: bounds.y + bounds.height / 2.0, + }; + crate::actionability::require_receives_events(resolved.handle(), point.clone(), adapter)?; + Ok(crate::commands::point_resolve::ResolvedPoint { + point, + pid: Some(entry.pid), + }) +} + /// Resolves a ref-or-xy point the same way [`point_resolve`] does, but retries /// transient resolve failures (`STALE_REF`, `AMBIGUOUS_TARGET`, `TIMEOUT`) /// within `timeout_ms` — the auto-wait budget for ref-addressed pointer /// commands (`hover`, `drag`) that resolve coordinates rather than dispatching /// through [`execute_ref_action_with_context`]. Coordinate-only input (no /// `ref_id`) and `timeout_ms: None` (`--timeout-ms 0`) both make exactly one -/// attempt. +/// attempt. When a ref and a budget are both present, each retry's resolve is +/// capped to the remaining budget via [`resolve_point_from_ref_capped`] so a +/// single slow resolve cannot exhaust the whole wait window. pub(crate) fn resolve_point_with_wait<'a>( ref_id: Option<&'a str>, xy: Option<(f64, f64)>, @@ -286,12 +355,12 @@ pub(crate) fn resolve_point_with_wait<'a>( context, ) }; - let Some(budget_ms) = timeout_ms.filter(|_| ref_id.is_some()) else { + let (Some(budget_ms), Some(ref_id)) = (timeout_ms, ref_id) else { return attempt(); }; let deadline = std::time::Instant::now() + crate::ref_action_wait::budget_from_ms(budget_ms); loop { - match attempt() { + match resolve_point_from_ref_capped(ref_id, snapshot_id, deadline, adapter, context) { Ok(point) => return Ok(point), Err(err) => { let remaining = deadline.saturating_duration_since(std::time::Instant::now()); diff --git a/crates/core/src/image_buffer.rs b/crates/core/src/image_buffer.rs index 14b8ffc..a3e1e1b 100644 --- a/crates/core/src/image_buffer.rs +++ b/crates/core/src/image_buffer.rs @@ -21,3 +21,67 @@ impl ImageFormat { } } } + +const PNG_SIGNATURE: [u8; 8] = [0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]; +const PNG_IHDR_CHUNK_TYPE: &[u8; 4] = b"IHDR"; + +/// Reads width/height from a PNG's `IHDR` chunk, validating the 8-byte PNG +/// signature and the `IHDR` chunk type before trusting the byte offsets. +/// Returns `None` for anything that is not a well-formed PNG header, +/// including truncated buffers. +pub fn parse_png_dimensions(data: &[u8]) -> Option<(u32, u32)> { + if data.len() < 24 { + return None; + } + if data[0..8] != PNG_SIGNATURE { + return None; + } + if &data[12..16] != PNG_IHDR_CHUNK_TYPE { + return None; + } + let width = u32::from_be_bytes([data[16], data[17], data[18], data[19]]); + let height = u32::from_be_bytes([data[20], data[21], data[22], data[23]]); + Some((width, height)) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn valid_png_header(width: u32, height: u32) -> Vec { + let mut bytes = PNG_SIGNATURE.to_vec(); + bytes.extend_from_slice(&[0, 0, 0, 13]); + bytes.extend_from_slice(PNG_IHDR_CHUNK_TYPE); + bytes.extend_from_slice(&width.to_be_bytes()); + bytes.extend_from_slice(&height.to_be_bytes()); + bytes + } + + #[test] + fn parses_dimensions_from_valid_png_header() { + let bytes = valid_png_header(640, 480); + + assert_eq!(parse_png_dimensions(&bytes), Some((640, 480))); + } + + #[test] + fn rejects_undersized_buffer() { + assert_eq!(parse_png_dimensions(&[1, 2, 3]), None); + } + + #[test] + fn rejects_buffer_with_wrong_signature() { + let mut bytes = valid_png_header(640, 480); + bytes[0] = 0x00; + + assert_eq!(parse_png_dimensions(&bytes), None); + } + + #[test] + fn rejects_buffer_with_wrong_chunk_type() { + let mut bytes = valid_png_header(640, 480); + bytes[12..16].copy_from_slice(b"IDAT"); + + assert_eq!(parse_png_dimensions(&bytes), None); + } +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 4dae396..38510f2 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -37,7 +37,6 @@ pub mod refs_store; #[cfg(test)] mod refs_test_support; pub(crate) mod resolved_element; -pub mod role; pub mod roles; pub mod screenshot_target; pub(crate) mod search_text; @@ -74,6 +73,7 @@ pub use display_info::DisplayInfo; pub use element_state::ElementState; pub use error::{AdapterError, AppError, ErrorCode}; pub use hit_test::HitTestResult; +pub use image_buffer::parse_png_dimensions; pub use interaction_policy::InteractionPolicy; pub use node::{AccessibilityNode, AppInfo, Rect, WindowInfo}; pub use notification::{NotificationFilter, NotificationInfo}; diff --git a/crates/core/src/role.rs b/crates/core/src/role.rs deleted file mode 100644 index d26f3f9..0000000 --- a/crates/core/src/role.rs +++ /dev/null @@ -1,121 +0,0 @@ -#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -pub enum Role { - Button, - Cell, - Checkbox, - Colorwell, - Combobox, - Disclosure, - Dockitem, - Group, - Image, - Incrementor, - Link, - List, - Menubutton, - Menuitem, - Radiobutton, - Scrollarea, - Slider, - Statictext, - Switch, - Tab, - Table, - Textfield, - Treeitem, - Unknown, -} - -impl Role { - pub fn as_str(self) -> &'static str { - match self { - Self::Button => "button", - Self::Cell => "cell", - Self::Checkbox => "checkbox", - Self::Colorwell => "colorwell", - Self::Combobox => "combobox", - Self::Disclosure => "disclosure", - Self::Dockitem => "dockitem", - Self::Group => "group", - Self::Image => "image", - Self::Incrementor => "incrementor", - Self::Link => "link", - Self::List => "list", - Self::Menubutton => "menubutton", - Self::Menuitem => "menuitem", - Self::Radiobutton => "radiobutton", - Self::Scrollarea => "scrollarea", - Self::Slider => "slider", - Self::Statictext => "statictext", - Self::Switch => "switch", - Self::Tab => "tab", - Self::Table => "table", - Self::Textfield => "textfield", - Self::Treeitem => "treeitem", - Self::Unknown => "unknown", - } - } - - pub fn parse(role: &str) -> Self { - role.parse().unwrap_or(Self::Unknown) - } -} - -impl std::str::FromStr for Role { - type Err = std::convert::Infallible; - - fn from_str(role: &str) -> Result { - Ok(match role.trim().to_ascii_lowercase().as_str() { - "button" => Self::Button, - "cell" => Self::Cell, - "checkbox" => Self::Checkbox, - "colorwell" => Self::Colorwell, - "combobox" => Self::Combobox, - "disclosure" => Self::Disclosure, - "dockitem" => Self::Dockitem, - "group" => Self::Group, - "image" => Self::Image, - "incrementor" => Self::Incrementor, - "link" => Self::Link, - "list" => Self::List, - "menubutton" => Self::Menubutton, - "menuitem" => Self::Menuitem, - "radiobutton" => Self::Radiobutton, - "scrollarea" => Self::Scrollarea, - "slider" => Self::Slider, - "statictext" => Self::Statictext, - "switch" => Self::Switch, - "tab" => Self::Tab, - "table" => Self::Table, - "textfield" => Self::Textfield, - "treeitem" => Self::Treeitem, - _ => Self::Unknown, - }) - } -} - -impl Role { - pub fn is_interactive(self) -> bool { - crate::roles::INTERACTIVE_ROLES.contains(&self.as_str()) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn from_str_unknown_for_bogus_role() { - assert_eq!(Role::parse("bogus"), Role::Unknown); - } - - #[test] - fn is_interactive_matches_interactive_roles_list() { - for role in crate::roles::INTERACTIVE_ROLES { - assert!( - Role::parse(role).is_interactive(), - "{role} should be interactive" - ); - } - } -} diff --git a/crates/core/src/roles.rs b/crates/core/src/roles.rs index b0323ae..b4d8a15 100644 --- a/crates/core/src/roles.rs +++ b/crates/core/src/roles.rs @@ -1,5 +1,3 @@ -use crate::role::Role; - /// Interactive roles that receive refs during snapshot allocation. /// /// Each entry must be produced by at least one platform adapter's native-to-canonical @@ -41,7 +39,8 @@ pub fn normalize_role_query(role: &str) -> String { /// Returns true when `role` is in [`INTERACTIVE_ROLES`]. pub fn is_interactive_role(role: &str) -> bool { - Role::parse(role).is_interactive() + let normalized = role.trim().to_ascii_lowercase(); + INTERACTIVE_ROLES.contains(&normalized.as_str()) } /// Returns true for roles whose checked/unchecked state can be queried and set. @@ -64,6 +63,13 @@ pub fn is_mutable_value_role(role: &str) -> bool { mod tests { use super::*; + #[test] + fn is_interactive_role_matches_interactive_roles_list() { + for role in INTERACTIVE_ROLES { + assert!(is_interactive_role(role), "{role} should be interactive"); + } + } + #[test] fn interactive_roles_are_sorted_and_unique() { let mut sorted = INTERACTIVE_ROLES.to_vec(); diff --git a/crates/macos/src/input/clipboard_rich.rs b/crates/macos/src/input/clipboard_rich.rs index 70e2ae7..25e4003 100644 --- a/crates/macos/src/input/clipboard_rich.rs +++ b/crates/macos/src/input/clipboard_rich.rs @@ -1,3 +1,4 @@ +use agent_desktop_core::image_buffer::parse_png_dimensions; use core_foundation::base::TCFType; use core_foundation::string::CFString; use core_foundation::url::CFURL; @@ -17,16 +18,11 @@ unsafe extern "C" { static NSPasteboardTypeFileURL: Id; } -/// Reads the width/height stored in a PNG's `IHDR` chunk directly from the -/// byte buffer, mirroring `macos::system::screenshot::png_dimensions` — no -/// image-decoding dependency needed for two big-endian `u32` reads. +/// Reads the width/height stored in a PNG's `IHDR` chunk via the shared core +/// helper, defaulting to `(0, 0)` for read-path data that fails validation +/// rather than rejecting an already-received clipboard payload. pub(crate) fn png_dimensions(data: &[u8]) -> (u32, u32) { - if data.len() < 24 { - return (0, 0); - } - let w = u32::from_be_bytes([data[16], data[17], data[18], data[19]]); - let h = u32::from_be_bytes([data[20], data[21], data[22], data[23]]); - (w, h) + parse_png_dimensions(data).unwrap_or((0, 0)) } pub(crate) fn read_image(pb: Id) -> Option> { diff --git a/crates/macos/src/input/clipboard_rich_tests.rs b/crates/macos/src/input/clipboard_rich_tests.rs index 6f696b5..7b72133 100644 --- a/crates/macos/src/input/clipboard_rich_tests.rs +++ b/crates/macos/src/input/clipboard_rich_tests.rs @@ -3,6 +3,7 @@ use super::*; fn fake_png(width: u32, height: u32) -> Vec { let mut bytes = vec![0u8; 24]; bytes[0..8].copy_from_slice(&[0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]); + bytes[12..16].copy_from_slice(b"IHDR"); bytes[16..20].copy_from_slice(&width.to_be_bytes()); bytes[20..24].copy_from_slice(&height.to_be_bytes()); bytes diff --git a/crates/macos/src/input/clipboard_tests.rs b/crates/macos/src/input/clipboard_tests.rs index 51f58e7..488ce31 100644 --- a/crates/macos/src/input/clipboard_tests.rs +++ b/crates/macos/src/input/clipboard_tests.rs @@ -28,6 +28,7 @@ impl Drop for RestoreGuard { fn fake_png(width: u32, height: u32) -> Vec { let mut bytes = vec![0u8; 24]; bytes[0..8].copy_from_slice(&[0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A]); + bytes[12..16].copy_from_slice(b"IHDR"); bytes[16..20].copy_from_slice(&width.to_be_bytes()); bytes[20..24].copy_from_slice(&height.to_be_bytes()); bytes diff --git a/crates/macos/src/system/launch.rs b/crates/macos/src/system/launch.rs index f406929..5c490d8 100644 --- a/crates/macos/src/system/launch.rs +++ b/crates/macos/src/system/launch.rs @@ -17,6 +17,8 @@ pub fn launch_app_with_options_impl( const OPEN_TIMEOUT: Duration = Duration::from_secs(5); + validate_app_identifier(id)?; + let filter = WindowFilter { focused_only: false, app: Some(id.to_string()), @@ -92,13 +94,7 @@ pub fn launch_app_impl(id: &str, timeout_ms: u64) -> Result Result Result<(), AdapterError> { + if id.contains("..") || id.starts_with('/') { + return Err(AdapterError::new( + ErrorCode::InvalidArgs, + format!("Invalid app identifier: '{id}'"), + ) + .with_suggestion("Use an app name like 'Safari' or bundle ID like 'com.apple.Safari'.")); + } + Ok(()) +} + #[cfg(target_os = "macos")] fn open_app_args(id: &str) -> [&str; 3] { ["-g", "-a", id] diff --git a/crates/macos/src/system/screenshot.rs b/crates/macos/src/system/screenshot.rs index 7bbff8c..a8688c2 100644 --- a/crates/macos/src/system/screenshot.rs +++ b/crates/macos/src/system/screenshot.rs @@ -1,6 +1,7 @@ use agent_desktop_core::{ adapter::{ImageBuffer, ImageFormat}, error::{AdapterError, ErrorCode}, + image_buffer::parse_png_dimensions, }; #[cfg(target_os = "macos")] @@ -116,7 +117,7 @@ mod imp { fn read_png(path: &Path) -> Result { let data = std::fs::read(path) .map_err(|e| AdapterError::internal(format!("read screenshot: {e}")))?; - let (width, height) = png_dimensions(&data); + let (width, height) = parse_png_dimensions(&data).unwrap_or((0, 0)); Ok(ImageBuffer { data, format: ImageFormat::Png, @@ -152,15 +153,6 @@ mod imp { AdapterError::internal("screencapture exited with error").with_platform_detail(detail) } - fn png_dimensions(data: &[u8]) -> (u32, u32) { - if data.len() < 24 { - return (0, 0); - } - let w = u32::from_be_bytes([data[16], data[17], data[18], data[19]]); - let h = u32::from_be_bytes([data[20], data[21], data[22], data[23]]); - (w, h) - } - fn find_cg_window_id_for_pid(pid: i32) -> Option { let mut best_id: Option = None; let mut best_area: f64 = 0.0; diff --git a/crates/macos/src/system/signals.rs b/crates/macos/src/system/signals.rs index de1b049..c9dc300 100644 --- a/crates/macos/src/system/signals.rs +++ b/crates/macos/src/system/signals.rs @@ -29,7 +29,7 @@ pub fn capture_signal_baseline_impl(filter: &SignalFilter) -> Result Vec { - let all = crate::system::app_list::list_apps_impl().unwrap_or_default(); +fn matching_apps(filter: &SignalFilter) -> Result, AdapterError> { + let all = crate::system::app_list::list_apps_impl()?; if let Some(name) = &filter.app { - return all + return Ok(all .into_iter() .filter(|app| app.name.eq_ignore_ascii_case(name)) - .collect(); + .collect()); } if let Some(pid) = filter.pid { - return all.into_iter().filter(|app| app.pid == pid).collect(); + return Ok(all.into_iter().filter(|app| app.pid == pid).collect()); } - all + Ok(all) } /// Surfaces (sheets, alerts, popovers, open menus) are only inspected for diff --git a/crates/macos/src/system/signals_tests.rs b/crates/macos/src/system/signals_tests.rs index c6336e9..9c21a3a 100644 --- a/crates/macos/src/system/signals_tests.rs +++ b/crates/macos/src/system/signals_tests.rs @@ -19,7 +19,7 @@ fn matching_apps_filters_by_name_case_insensitively() { app: Some("Definitely-Not-A-Real-App-xyz123".into()), pid: None, }; - let apps = matching_apps(&filter); + let apps = matching_apps(&filter).expect("app enumeration must succeed on the test host"); assert!( apps.is_empty(), "an app name that matches no running process must yield no apps" diff --git a/crates/macos/src/system/window_resolve.rs b/crates/macos/src/system/window_resolve.rs index a17871f..bb823a4 100644 --- a/crates/macos/src/system/window_resolve.rs +++ b/crates/macos/src/system/window_resolve.rs @@ -73,6 +73,18 @@ fn window_info_from_record(win: &WindowInfo, record: &WindowRecord) -> WindowInf } } +/// Matches the CGWindow-verified `window_number` against the app's live +/// `AXWindow` elements. +/// +/// The CGWindow record was already verified (pid + title) by +/// `locate_verified_record`, so a total bridge miss means +/// `_AXUIElementGetWindow` transiently failed (busy/hung app, benign race), +/// not that the window is gone. In that case, fall back to the AX window +/// matching the verified title before reporting `WINDOW_NOT_FOUND` — but only +/// when the title identifies exactly one live `AXWindow`. If zero or two-plus +/// windows share that title, the correct target cannot be determined safely, +/// so this returns `None` and lets the caller surface `WINDOW_NOT_FOUND` +/// rather than risk acting on the wrong same-titled window. fn ax_window_element_for_number( pid: i32, window_number: i64, @@ -88,16 +100,16 @@ fn ax_window_element_for_number( return Some(window.clone()); } } - // The CGWindow record was already verified (pid + title) by - // locate_verified_record, so a total bridge miss means - // _AXUIElementGetWindow transiently failed (busy/hung app, benign race), - // not that the window is gone. Fall back to the AX window matching the - // verified title before reporting WINDOW_NOT_FOUND. let title = fallback_title?; - windows.into_iter().find(|window| { + let mut matches = windows.into_iter().filter(|window| { copy_string_attr(window, "AXRole").as_deref() == Some("AXWindow") && copy_string_attr(window, "AXTitle").as_deref() == Some(title) - }) + }); + let single_match = matches.next()?; + match matches.next() { + Some(_) => None, + None => Some(single_match), + } } /// Bridges an accessibility window element to its CoreGraphics window number diff --git a/src/cli_args/mod_tests.rs b/src/cli_args/mod_tests.rs index 1443f88..c12516f 100644 --- a/src/cli_args/mod_tests.rs +++ b/src/cli_args/mod_tests.rs @@ -1,4 +1,77 @@ use super::*; +use agent_desktop_core::adapter::SnapshotSurface; +use clap::ValueEnum; + +/// The exhaustive set of `SnapshotSurface` values the CLI's `Surface` enum is +/// meant to expose. `SnapshotSurface` is `#[non_exhaustive]`, so a new variant +/// added to it will not force `Surface::to_core`'s match arms (which match on +/// the local `Surface`, not on `SnapshotSurface`) to change — this list is the +/// independent tripwire that must be updated by hand alongside any new +/// `Surface`/`to_core` arm. +const EXPECTED_CORE_SURFACES: &[SnapshotSurface] = &[ + SnapshotSurface::Window, + SnapshotSurface::Focused, + SnapshotSurface::Menu, + SnapshotSurface::Menubar, + SnapshotSurface::Sheet, + SnapshotSurface::Popover, + SnapshotSurface::Alert, +]; + +/// Keeps the CLI `Surface` enum and the hand-maintained `EXPECTED_CORE_SURFACES` +/// tripwire in one-to-one correspondence: every `Surface` variant (enumerated +/// via its `ValueEnum` derive, so this side stays exhaustive automatically) maps +/// through `.to_core()` to a distinct `SnapshotSurface`, the tripwire lists each +/// of those exactly once, and the two sets match. An inconsistent hand-edit — a +/// `to_core` arm that collides with another, or the tripwire updated without a +/// matching `Surface`/`to_core` arm (or the reverse) — fails here. The one gap it +/// cannot close: because `SnapshotSurface` is `#[non_exhaustive]` with no +/// reflection, a new core variant added with no CLI-side edit keeps both sides +/// their current size and passes — the by-hand tripwire update is the only +/// checkpoint for that case (see `EXPECTED_CORE_SURFACES`). +#[test] +fn surface_to_core_maps_one_to_one_onto_expected_core_surfaces() { + let cli_variants = Surface::value_variants(); + assert_eq!( + cli_variants.len(), + EXPECTED_CORE_SURFACES.len(), + "Surface has {} variants but EXPECTED_CORE_SURFACES has {} — \ + update EXPECTED_CORE_SURFACES (and Surface::to_core) to match", + cli_variants.len(), + EXPECTED_CORE_SURFACES.len() + ); + + for (i, a) in EXPECTED_CORE_SURFACES.iter().enumerate() { + for (j, b) in EXPECTED_CORE_SURFACES.iter().enumerate() { + assert!( + i == j || a != b, + "EXPECTED_CORE_SURFACES lists SnapshotSurface::{a:?} more than once — \ + each expected core surface must appear exactly once" + ); + } + } + + let mapped: Vec = cli_variants.iter().map(Surface::to_core).collect(); + for (i, a) in mapped.iter().enumerate() { + for (j, b) in mapped.iter().enumerate() { + assert!( + i == j || a != b, + "Surface::{:?} and Surface::{:?} both map to the same \ + SnapshotSurface::{a:?} via to_core — mapping must be distinct", + cli_variants[i], + cli_variants[j] + ); + } + } + + for expected in EXPECTED_CORE_SURFACES { + assert!( + mapped.contains(expected), + "no Surface variant maps to SnapshotSurface::{expected:?} via to_core — \ + a Surface arm is missing for this core surface" + ); + } +} /// God-object regression: `FindArgs` used to carry 14 flat fields; the /// match-criteria and result-shaping groups now live in diff --git a/src/main.rs b/src/main.rs index f421094..4dbe5e7 100644 --- a/src/main.rs +++ b/src/main.rs @@ -23,6 +23,8 @@ use cli::{Cli, Commands}; use cli_args::skills::SkillsAction; use std::io::{BufWriter, Write}; +const EXIT_STDOUT_WRITE_FAILURE: i32 = 74; + const WAIT_SUPPORTED: &[&str] = &[ "snapshot", "click", @@ -55,10 +57,12 @@ fn main() { } let msg = e.to_string(); let first_line = msg.lines().next().unwrap_or("parse error"); - emit_response(&Response::err( + if let Err(write_err) = emit_response(&Response::err( "unknown", ErrorPayload::new("INVALID_ARGS", first_line), - )); + )) { + exit_after_emit_failure(write_err); + } std::process::exit(2); } }; @@ -165,20 +169,32 @@ fn run_with_adapter(cmd: Commands, cmd_name: &str, context: &CommandContext) { fn finish(cmd_name: &str, result: Result) { match result { Ok(data) => { - emit_response(&Response::ok(cmd_name, data)); + if let Err(write_err) = emit_response(&Response::ok(cmd_name, data)) { + exit_after_emit_failure(write_err); + } std::process::exit(0); } Err(e) => { - emit_response(&Response::err( + if let Err(write_err) = emit_response(&Response::err( cmd_name, agent_desktop_core::ErrorPayload::from_app_error(&e), - )); + )) { + exit_after_emit_failure(write_err); + } std::process::exit(1); } } } -fn emit_response(response: &Response) { +/// Reports a stdout write failure on stderr and terminates with a distinct +/// non-zero exit code, so a broken pipe or full disk is never mistaken for a +/// successful command outcome. +fn exit_after_emit_failure(write_err: std::io::Error) -> ! { + eprintln!("agent-desktop: failed to write response to stdout: {write_err}"); + std::process::exit(EXIT_STDOUT_WRITE_FAILURE); +} + +fn emit_response(response: &Response) -> std::io::Result<()> { match serde_json::to_value(response) { Ok(value) => emit_json(&value), Err(err) => emit_json(&serde_json::json!({ @@ -193,14 +209,12 @@ fn emit_response(response: &Response) { } } -fn emit_json(value: &serde_json::Value) { +fn emit_json(value: &serde_json::Value) -> std::io::Result<()> { let stdout = std::io::stdout(); let mut writer = BufWriter::new(stdout.lock()); - if serde_json::to_writer(&mut writer, value).is_err() { - return; - } - let _ = writer.write_all(b"\n"); - let _ = writer.flush(); + serde_json::to_writer(&mut writer, value).map_err(std::io::Error::other)?; + writer.write_all(b"\n")?; + writer.flush() } fn build_adapter() -> impl agent_desktop_core::adapter::PlatformAdapter {