From eeff52f62ab527d7416f24f325cdb17cbebfe654 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Tue, 2 Jun 2026 01:10:15 -0700 Subject: [PATCH] fix: harden reliability edge cases --- CLAUDE.md | 9 +- Cargo.lock | 1 + crates/core/src/action.rs | 7 +- crates/core/src/actionability.rs | 7 +- crates/core/src/actionability_tests.rs | 61 +++ crates/core/src/adapter.rs | 11 +- crates/core/src/commands/drag.rs | 54 +-- crates/core/src/commands/helpers.rs | 72 ++-- crates/core/src/commands/hover.rs | 12 +- crates/core/src/commands/wait.rs | 38 +- .../core/src/commands/wait_element_tests.rs | 364 ++++++++++++++++++ .../src/commands/wait_latest_ref_cache.rs | 4 + .../commands/wait_latest_ref_cache_tests.rs | 67 ++++ crates/core/src/commands/wait_predicate.rs | 10 +- crates/core/src/commands/wait_tests.rs | 318 +-------------- crates/core/src/context.rs | 54 +++ crates/core/src/trace.rs | 59 ++- crates/ffi/Cargo.toml | 1 + crates/ffi/include/agent_desktop.h | 7 + crates/ffi/src/error.rs | 23 ++ crates/ffi/src/error_tests.rs | 25 ++ crates/ffi/src/observation/find.rs | 2 +- crates/ffi/src/types/ref_entry.rs | 4 +- crates/ffi/tests/c_abi_layout.rs | 8 +- crates/ffi/tests/c_header_compile.rs | 2 + crates/ffi/tests/common/mod.rs | 1 + crates/ffi/tests/error_lifetime.rs | 4 + crates/macos/src/adapter.rs | 22 +- crates/macos/src/tree/action_list.rs | 35 +- crates/macos/src/tree/resolve_tests.rs | 20 + 30 files changed, 878 insertions(+), 424 deletions(-) create mode 100644 crates/core/src/commands/wait_element_tests.rs create mode 100644 crates/core/src/commands/wait_latest_ref_cache_tests.rs diff --git a/CLAUDE.md b/CLAUDE.md index 50971e5..0367bfb 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -184,8 +184,9 @@ Phases 2–4 add adapters, transports, and production readiness work. Nothing in ``` PERM_DENIED, ELEMENT_NOT_FOUND, APP_NOT_FOUND, ACTION_FAILED, -ACTION_NOT_SUPPORTED, STALE_REF, WINDOW_NOT_FOUND, -PLATFORM_NOT_SUPPORTED, TIMEOUT, INVALID_ARGS, INTERNAL +ACTION_NOT_SUPPORTED, STALE_REF, AMBIGUOUS_TARGET, WINDOW_NOT_FOUND, +PLATFORM_NOT_SUPPORTED, TIMEOUT, INVALID_ARGS, NOTIFICATION_NOT_FOUND, +SNAPSHOT_NOT_FOUND, POLICY_DENIED, INTERNAL ``` ### Exit Codes @@ -271,7 +272,7 @@ Every command produces a response envelope: ```json { - "version": "1.0", + "version": "2.0", "ok": true, "command": "snapshot", "data": { @@ -287,7 +288,7 @@ Error responses: ```json { - "version": "1.0", + "version": "2.0", "ok": false, "command": "click", "error": { diff --git a/Cargo.lock b/Cargo.lock index 62f11e6..94ea4ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -50,6 +50,7 @@ dependencies = [ "agent-desktop-macos", "agent-desktop-windows", "libc", + "serde_json", "smallvec", ] diff --git a/crates/core/src/action.rs b/crates/core/src/action.rs index 4b96c88..ceb0dd6 100644 --- a/crates/core/src/action.rs +++ b/crates/core/src/action.rs @@ -64,7 +64,8 @@ impl Action { Self::Select(_) => &["Select", "Click"], Self::Toggle => &["Toggle", "Click"], Self::Check | Self::Uncheck => &["Toggle", "Click"], - Self::Scroll(_, _) | Self::ScrollTo => &["ScrollTo"], + Self::Scroll(_, _) => &["Scroll", "ScrollTo"], + Self::ScrollTo => &["ScrollTo"], Self::PressKey(_) => &["PressKey"], Self::KeyDown(_) => &["KeyDown"], Self::KeyUp(_) => &["KeyUp"], @@ -78,10 +79,6 @@ impl Action { matches!(self, Self::Hover | Self::Drag(_)) } - pub fn requires_focus_policy(&self) -> bool { - matches!(self, Self::TypeText(_) | Self::PressKey(_)) - } - pub fn may_use_focus_fallback(&self) -> bool { matches!(self, Self::TypeText(_) | Self::PressKey(_)) } diff --git a/crates/core/src/actionability.rs b/crates/core/src/actionability.rs index 027aed9..e726de5 100644 --- a/crates/core/src/actionability.rs +++ b/crates/core/src/actionability.rs @@ -29,7 +29,7 @@ pub struct ActionabilityReport { pub checks: Vec, } -pub fn check( +pub(crate) fn check( entry: &RefEntry, request: &ActionRequest, ) -> Result { @@ -71,6 +71,9 @@ pub fn check_live( if let Some(bounds) = adapter.get_element_bounds(handle).ok().flatten() { observed.bounds = Some(bounds); } + if let Some(actions) = adapter.get_live_actions(handle).ok().flatten() { + observed.available_actions = actions; + } check(&observed, request) } @@ -115,7 +118,7 @@ fn policy_check(request: &ActionRequest) -> ActionabilityCheck { "action requires cursor movement but policy denies it", ); } - if request.action.requires_focus_policy() && !request.policy.allow_focus_steal { + if request.action.may_use_focus_fallback() && !request.policy.allow_focus_steal { return fail("policy", "action requires focus but policy denies it"); } pass("policy") diff --git a/crates/core/src/actionability_tests.rs b/crates/core/src/actionability_tests.rs index feef9f7..f7c1da7 100644 --- a/crates/core/src/actionability_tests.rs +++ b/crates/core/src/actionability_tests.rs @@ -9,6 +9,7 @@ use crate::{ struct LiveAdapter { state: Option, bounds: Option, + actions: Option>, } impl PlatformAdapter for LiveAdapter { @@ -19,6 +20,13 @@ impl PlatformAdapter for LiveAdapter { fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { Ok(self.bounds) } + + fn get_live_actions( + &self, + _handle: &NativeHandle, + ) -> Result>, AdapterError> { + Ok(self.actions.clone()) + } } fn entry() -> RefEntry { @@ -113,6 +121,16 @@ fn command_aliases_match_platform_capabilities() { assert!(check(&editable, &ActionRequest::headless(Action::Clear)).is_ok()); let mut scrollable = entry(); + scrollable.available_actions = vec!["Scroll".into()]; + assert!( + check( + &scrollable, + &ActionRequest::headless(Action::Scroll(Direction::Down, 1)) + ) + .is_ok() + ); + assert!(check(&scrollable, &ActionRequest::headless(Action::ScrollTo)).is_err()); + scrollable.available_actions = vec!["ScrollTo".into()]; assert!( check( @@ -135,6 +153,7 @@ fn live_actionability_overrides_stale_snapshot_state() { value: None, }), bounds: stale.bounds, + actions: Some(vec!["Click".into()]), }; let report = check_live( @@ -147,3 +166,45 @@ fn live_actionability_overrides_stale_snapshot_state() { assert!(report.actionable); } + +#[test] +fn live_actionability_uses_actions_gained_after_snapshot() { + let mut stale = entry(); + stale.available_actions = vec![]; + let adapter = LiveAdapter { + state: None, + bounds: stale.bounds, + actions: Some(vec!["Click".into()]), + }; + + let report = check_live( + &stale, + &NativeHandle::null(), + &adapter, + &ActionRequest::headless(Action::Click), + ) + .unwrap(); + + assert!(report.actionable); +} + +#[test] +fn live_actionability_fails_when_action_disappears_after_snapshot() { + let stale = entry(); + let adapter = LiveAdapter { + state: None, + bounds: stale.bounds, + actions: Some(vec![]), + }; + + let err = check_live( + &stale, + &NativeHandle::null(), + &adapter, + &ActionRequest::headless(Action::Click), + ) + .unwrap_err(); + + assert_eq!(err.code, ErrorCode::ActionFailed); + assert!(err.message.contains("supported_action")); +} diff --git a/crates/core/src/adapter.rs b/crates/core/src/adapter.rs index a361158..2b884af 100644 --- a/crates/core/src/adapter.rs +++ b/crates/core/src/adapter.rs @@ -143,10 +143,6 @@ pub trait PlatformAdapter: Send + Sync { Err(AdapterError::not_supported("execute_action")) } - fn resolve_element(&self, _entry: &RefEntry) -> Result { - Err(AdapterError::not_supported("resolve_element")) - } - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { Err(AdapterError::not_supported("resolve_element_strict")) } @@ -215,6 +211,13 @@ pub trait PlatformAdapter: Send + Sync { Err(AdapterError::not_supported("get_live_state")) } + fn get_live_actions( + &self, + _handle: &NativeHandle, + ) -> Result>, AdapterError> { + Err(AdapterError::not_supported("get_live_actions")) + } + fn press_key_for_app( &self, _app_name: &str, diff --git a/crates/core/src/commands/drag.rs b/crates/core/src/commands/drag.rs index 85eef8d..b82ea74 100644 --- a/crates/core/src/commands/drag.rs +++ b/crates/core/src/commands/drag.rs @@ -1,7 +1,7 @@ use crate::{ action::{DragParams, Point}, adapter::PlatformAdapter, - commands::helpers::resolve_point_from_ref_or_xy_with_context, + commands::helpers::{PointResolveArgs, resolve_point_from_ref_or_xy_with_context}, context::CommandContext, error::AppError, }; @@ -21,22 +21,8 @@ pub fn execute( adapter: &dyn PlatformAdapter, context: &CommandContext, ) -> Result { - let from = resolve_point( - &args.from_ref, - args.from_xy, - "from", - args.snapshot_id.as_deref(), - adapter, - context, - )?; - let to = resolve_point( - &args.to_ref, - args.to_xy, - "to", - args.snapshot_id.as_deref(), - adapter, - context, - )?; + let from = resolve_from_point(&args, adapter, context)?; + let to = resolve_to_point(&args, adapter, context)?; let params = DragParams { from: from.clone(), to: to.clone(), @@ -50,20 +36,36 @@ pub fn execute( })) } -fn resolve_point( - ref_id: &Option, - xy: Option<(f64, f64)>, - label: &str, - snapshot_id: Option<&str>, +fn resolve_from_point( + args: &DragArgs, adapter: &dyn PlatformAdapter, context: &CommandContext, ) -> Result { resolve_point_from_ref_or_xy_with_context( - ref_id.as_deref(), - xy, - snapshot_id, + PointResolveArgs { + ref_id: args.from_ref.as_deref(), + xy: args.from_xy, + snapshot_id: args.snapshot_id.as_deref(), + missing_input_message: "Provide --from or --from-xy x,y", + }, + adapter, + context, + ) +} + +fn resolve_to_point( + args: &DragArgs, + adapter: &dyn PlatformAdapter, + context: &CommandContext, +) -> Result { + resolve_point_from_ref_or_xy_with_context( + PointResolveArgs { + ref_id: args.to_ref.as_deref(), + xy: args.to_xy, + snapshot_id: args.snapshot_id.as_deref(), + missing_input_message: "Provide --to or --to-xy x,y", + }, adapter, - format!("Provide --{label} or --{label}-xy x,y"), context, ) } diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 5e2c72b..d46e9b7 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -20,6 +20,19 @@ pub struct RefArgs { pub snapshot_id: Option, } +pub(crate) struct PointResolveArgs<'a> { + pub ref_id: Option<&'a str>, + pub xy: Option<(f64, f64)>, + pub snapshot_id: Option<&'a str>, + pub missing_input_message: &'a str, +} + +struct ActionabilityTarget<'a> { + ref_id: &'a str, + entry: &'a RefEntry, + handle: &'a NativeHandle, +} + #[cfg(test)] pub(crate) fn resolve_ref<'a>( ref_id: &str, @@ -157,52 +170,59 @@ pub(crate) fn execute_ref_action_result_with_context( context: &CommandContext, ) -> Result<(RefEntry, ActionResult), AppError> { let (entry, handle) = resolve_ref_with_context(ref_id, snapshot_id, adapter, context)?; - check_actionability_with_trace(ref_id, &entry, handle.handle(), adapter, &request, context)?; + check_actionability_with_trace( + ActionabilityTarget { + ref_id, + entry: &entry, + handle: handle.handle(), + }, + adapter, + &request, + context, + )?; let result = adapter.execute_action(handle.handle(), request)?; context.trace("action.dispatch.ok", json!({ "ref": ref_id }))?; Ok((entry, result)) } -pub(crate) fn check_actionability_with_trace( - ref_id: &str, - entry: &RefEntry, - handle: &NativeHandle, +fn check_actionability_with_trace( + target: ActionabilityTarget<'_>, adapter: &dyn PlatformAdapter, request: &ActionRequest, context: &CommandContext, ) -> Result<(), AppError> { context.trace( "actionability.check.start", - json!({ "ref": ref_id, "action": request.action.name() }), + json!({ "ref": target.ref_id, "action": request.action.name() }), + )?; + crate::actionability::check_live(target.entry, target.handle, adapter, request).inspect_err( + |err| { + let _ = context.trace( + "actionability.check.error", + json!({ + "ref": target.ref_id, + "action": request.action.name(), + "code": err.code.as_str(), + "message": err.message.clone() + }), + ); + }, )?; - crate::actionability::check_live(entry, handle, adapter, request).inspect_err(|err| { - let _ = context.trace( - "actionability.check.error", - json!({ - "ref": ref_id, - "action": request.action.name(), - "code": err.code.as_str(), - "message": err.message.clone() - }), - ); - })?; context.trace( "actionability.check.ok", - json!({ "ref": ref_id, "action": request.action.name() }), + json!({ "ref": target.ref_id, "action": request.action.name() }), )?; Ok(()) } pub(crate) fn resolve_point_from_ref_or_xy_with_context( - ref_id: Option<&str>, - xy: Option<(f64, f64)>, - snapshot_id: Option<&str>, + args: PointResolveArgs<'_>, adapter: &dyn PlatformAdapter, - missing_input_message: impl Into, context: &CommandContext, ) -> Result { - if let Some(ref_id) = ref_id { - let (_entry, handle) = resolve_ref_with_context(ref_id, snapshot_id, adapter, context)?; + if let Some(ref_id) = args.ref_id { + let (_entry, handle) = + resolve_ref_with_context(ref_id, args.snapshot_id, adapter, context)?; let bounds = adapter .get_element_bounds(handle.handle())? .ok_or_else(|| AppError::invalid_input(format!("Element {ref_id} has no bounds")))?; @@ -211,10 +231,10 @@ pub(crate) fn resolve_point_from_ref_or_xy_with_context( y: bounds.y + bounds.height / 2.0, }); } - if let Some((x, y)) = xy { + if let Some((x, y)) = args.xy { return Ok(Point { x, y }); } - Err(AppError::invalid_input(missing_input_message.into())) + Err(AppError::invalid_input(args.missing_input_message)) } pub(crate) fn window_op_command( diff --git a/crates/core/src/commands/hover.rs b/crates/core/src/commands/hover.rs index efe27f2..b4a9687 100644 --- a/crates/core/src/commands/hover.rs +++ b/crates/core/src/commands/hover.rs @@ -1,7 +1,7 @@ use crate::{ action::{MouseButton, MouseEvent, MouseEventKind, Point}, adapter::PlatformAdapter, - commands::helpers::resolve_point_from_ref_or_xy_with_context, + commands::helpers::{PointResolveArgs, resolve_point_from_ref_or_xy_with_context}, context::CommandContext, error::AppError, }; @@ -37,11 +37,13 @@ fn resolve_hover_point( context: &CommandContext, ) -> Result { resolve_point_from_ref_or_xy_with_context( - args.ref_id.as_deref(), - args.xy, - args.snapshot_id.as_deref(), + PointResolveArgs { + ref_id: args.ref_id.as_deref(), + xy: args.xy, + snapshot_id: args.snapshot_id.as_deref(), + missing_input_message: "Provide a ref (@e1) or --xy x,y", + }, adapter, - "Provide a ref (@e1) or --xy x,y", context, ) } diff --git a/crates/core/src/commands/wait.rs b/crates/core/src/commands/wait.rs index 414de61..ca1d681 100644 --- a/crates/core/src/commands/wait.rs +++ b/crates/core/src/commands/wait.rs @@ -188,27 +188,17 @@ fn wait_for_element( })); } } - Err(err) if fixed_refmap.is_none() && err.code == ErrorCode::StaleRef => { + Err(err) if is_retryable_wait_resolution_error(&err.code) => { last_observed = json!({ "error": err.code.as_str(), "message": err.message }); - if let Some(cache) = latest_cache.as_mut() { - cache.refresh_if_due(); + if fixed_refmap.is_none() { + if let Some(cache) = latest_cache.as_mut() { + cache.refresh_if_due(); + } } } - Err(err) if err.code == ErrorCode::StaleRef => { - last_observed = json!({ - "error": err.code.as_str(), - "message": err.message - }); - } - Err(err) if err.code == ErrorCode::ElementNotFound => { - last_observed = json!({ - "error": err.code.as_str(), - "message": err.message - }); - } Err(err) => return Err(AppError::Adapter(err)), } } else if let Some(cache) = latest_cache.as_mut() { @@ -271,6 +261,16 @@ fn wait_for_window( } } +fn is_retryable_wait_resolution_error(code: &ErrorCode) -> bool { + matches!( + code, + ErrorCode::StaleRef + | ErrorCode::ElementNotFound + | ErrorCode::AmbiguousTarget + | ErrorCode::Timeout + ) +} + fn wait_for_text( text: String, expected_count: Option, @@ -316,7 +316,7 @@ fn wait_for_text( )) .with_details(json!({ "predicate": "text", - "text": text, + "text_chars": text.chars().count(), "timeout_ms": timeout_ms, "expected_count": expected_count })), @@ -357,7 +357,7 @@ fn wait_for_notification( "predicate": "notification", "timeout_ms": args.timeout_ms, "app": args.app.clone(), - "text": args.text.clone() + "text_chars": args.text.as_ref().map(|text| text.chars().count()) })), )); } @@ -384,3 +384,7 @@ fn wait_for_notification( #[cfg(test)] #[path = "wait_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "wait_element_tests.rs"] +mod element_tests; diff --git a/crates/core/src/commands/wait_element_tests.rs b/crates/core/src/commands/wait_element_tests.rs new file mode 100644 index 0000000..a70c3c5 --- /dev/null +++ b/crates/core/src/commands/wait_element_tests.rs @@ -0,0 +1,364 @@ +use super::*; +use crate::{ + action::ElementState, + adapter::{NativeHandle, PlatformAdapter}, + error::{AdapterError, ErrorCode}, + node::Rect, + refs::{RefEntry, RefMap}, + refs_store::RefStore, + refs_test_support::HomeGuard, +}; +use std::sync::Mutex; + +struct NoopAdapter; + +impl PlatformAdapter for NoopAdapter {} + +struct PredicateAdapter { + state: Option, + value: Option, + bounds: Option, +} + +impl PlatformAdapter for PredicateAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { + Ok(self.state.clone()) + } + + fn get_live_value(&self, _handle: &NativeHandle) -> Result, AdapterError> { + Ok(self.value.clone()) + } + + fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { + Ok(self.bounds) + } +} + +struct AmbiguousResolveAdapter; + +impl PlatformAdapter for AmbiguousResolveAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Err(AdapterError::ambiguous_target("2 candidates matched")) + } +} + +struct TransientResolveAdapter { + errors: Mutex>, +} + +impl PlatformAdapter for TransientResolveAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + if let Some(code) = self.errors.lock().unwrap().pop() { + return Err(AdapterError::new(code, "transient resolution failure")); + } + Ok(NativeHandle::null()) + } +} + +struct PermissionResolveAdapter; + +impl PlatformAdapter for PermissionResolveAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Err(AdapterError::permission_denied()) + } +} + +struct FlippingPredicateAdapter { + states: Mutex>>, +} + +impl PlatformAdapter for FlippingPredicateAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { + let states = self.states.lock().unwrap().pop().unwrap_or_default(); + Ok(Some(ElementState { + role: "button".into(), + states, + value: None, + })) + } +} + +fn snapshot_with_one_ref() -> String { + save_ref(Vec::new()) +} + +fn snapshot_with_disabled_ref() -> String { + save_ref(vec!["disabled".into()]) +} + +fn save_ref(states: Vec) -> String { + let mut refmap = RefMap::new(); + refmap.allocate(RefEntry { + pid: 1, + role: "button".into(), + name: Some("Run".into()), + value: None, + description: None, + states, + bounds: None, + bounds_hash: None, + available_actions: vec!["Click".into()], + source_app: None, + source_window_id: None, + source_window_title: None, + source_surface: crate::adapter::SnapshotSurface::Window, + root_ref: None, + path_is_absolute: false, + path: smallvec::SmallVec::new(), + }); + RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() +} + +#[test] +fn snapshot_pinned_missing_ref_is_invalid_args() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + + let err = wait_for_element( + "@e2".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Exists, + 1, + &NoopAdapter, + &crate::context::CommandContext::default(), + ) + .unwrap_err(); + + assert_eq!(err.code(), "INVALID_ARGS"); + assert!(err.suggestion().is_some()); +} + +#[test] +fn element_wait_enabled_predicate_uses_live_state() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + let adapter = PredicateAdapter { + state: Some(ElementState { + role: "button".into(), + states: vec![], + value: None, + }), + value: None, + bounds: None, + }; + + let value = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Enabled, + 1, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["predicate"], "enabled"); + assert_eq!(value["observed"]["enabled"], true); +} + +#[test] +fn element_wait_value_predicate_matches_live_value_without_leaking_it() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + let adapter = PredicateAdapter { + state: None, + value: Some("ready".into()), + bounds: None, + }; + + let value = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Value("ready".into()), + 1, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["predicate"], "value"); + assert_eq!(value["observed"]["matched"], true); + assert_eq!(value["observed"]["value_chars"], 5); + assert!(value["observed"].get("value").is_none()); +} + +#[test] +fn element_wait_timeout_reports_last_actionability_observation() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_disabled_ref(); + let adapter = PredicateAdapter { + state: Some(ElementState { + role: "button".into(), + states: vec!["disabled".into()], + value: None, + }), + value: None, + bounds: None, + }; + + let err = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Actionable, + 1, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap_err(); + + assert_eq!(err.code(), "TIMEOUT"); + match err { + AppError::Adapter(adapter_error) => { + let details = adapter_error.details.unwrap(); + assert_eq!(details["predicate"], "actionable"); + assert_eq!(details["last_observed"]["actionable"], false); + } + _ => panic!("expected adapter error"), + } +} + +#[test] +fn element_wait_actionable_uses_live_state() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_disabled_ref(); + let adapter = PredicateAdapter { + state: Some(ElementState { + role: "button".into(), + states: vec![], + value: None, + }), + value: None, + bounds: None, + }; + + let value = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Actionable, + 1, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["predicate"], "actionable"); + assert_eq!(value["observed"]["actionable"], true); +} + +#[test] +fn element_wait_actionable_retries_until_live_state_converges() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_disabled_ref(); + let adapter = FlippingPredicateAdapter { + states: Mutex::new(vec![vec![], vec!["disabled".into()]]), + }; + + let value = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Actionable, + 250, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["predicate"], "actionable"); + assert_eq!(value["observed"]["actionable"], true); +} + +#[test] +fn element_wait_retries_transient_ambiguous_resolution() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + let adapter = TransientResolveAdapter { + errors: Mutex::new(vec![ErrorCode::AmbiguousTarget]), + }; + + let value = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Exists, + 250, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["predicate"], "exists"); + assert_eq!(value["observed"]["exists"], true); +} + +#[test] +fn element_wait_retries_transient_resolution_timeout() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + let adapter = TransientResolveAdapter { + errors: Mutex::new(vec![ErrorCode::Timeout]), + }; + + let value = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Exists, + 250, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["observed"]["exists"], true); +} + +#[test] +fn element_wait_times_out_after_persistent_ambiguous_resolution() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + + let err = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Exists, + 1, + &AmbiguousResolveAdapter, + &crate::context::CommandContext::default(), + ) + .unwrap_err(); + + assert_eq!(err.code(), "TIMEOUT"); + match err { + AppError::Adapter(adapter_error) => { + assert_eq!( + adapter_error.details.unwrap()["last_observed"]["error"], + "AMBIGUOUS_TARGET" + ); + } + _ => panic!("expected adapter error"), + } +} + +#[test] +fn element_wait_aborts_terminal_permission_error() { + let _guard = HomeGuard::new(); + let snapshot_id = snapshot_with_one_ref(); + + let err = wait_for_element( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Exists, + 250, + &PermissionResolveAdapter, + &crate::context::CommandContext::default(), + ) + .unwrap_err(); + + assert_eq!(err.code(), "PERM_DENIED"); +} diff --git a/crates/core/src/commands/wait_latest_ref_cache.rs b/crates/core/src/commands/wait_latest_ref_cache.rs index 68b4856..6036c0a 100644 --- a/crates/core/src/commands/wait_latest_ref_cache.rs +++ b/crates/core/src/commands/wait_latest_ref_cache.rs @@ -47,3 +47,7 @@ impl<'a> LatestRefCache<'a> { } } } + +#[cfg(test)] +#[path = "wait_latest_ref_cache_tests.rs"] +mod tests; diff --git a/crates/core/src/commands/wait_latest_ref_cache_tests.rs b/crates/core/src/commands/wait_latest_ref_cache_tests.rs new file mode 100644 index 0000000..1051fe5 --- /dev/null +++ b/crates/core/src/commands/wait_latest_ref_cache_tests.rs @@ -0,0 +1,67 @@ +use super::*; +use crate::{ + adapter::SnapshotSurface, + refs::{RefEntry, RefMap}, + refs_test_support::HomeGuard, +}; + +fn save_ref(pid: i32, name: Option<&str>) -> String { + let mut refmap = RefMap::new(); + refmap.allocate(RefEntry { + pid, + role: "button".into(), + name: name.map(String::from), + value: None, + description: None, + states: vec![], + bounds: None, + bounds_hash: None, + available_actions: vec!["Click".into()], + source_app: None, + source_window_id: None, + source_window_title: None, + source_surface: SnapshotSurface::Window, + root_ref: None, + path_is_absolute: false, + path: smallvec::SmallVec::new(), + }); + RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() +} + +#[test] +fn latest_ref_cache_picks_up_newer_snapshot_after_refresh() { + let _guard = HomeGuard::new(); + let first_id = save_ref(1, Some("First")); + let store = RefStore::new().unwrap(); + + let mut cache = LatestRefCache::new(&store).unwrap(); + assert_eq!(cache.snapshot_id.as_deref(), Some(first_id.as_str())); + + let second_id = save_ref(99, Some("Second")); + assert_ne!(first_id, second_id); + + cache.last_refresh = std::time::Instant::now() - std::time::Duration::from_secs(2); + cache.refresh_if_due(); + + assert_eq!(cache.snapshot_id.as_deref(), Some(second_id.as_str())); + assert!(cache.entry("@e1").is_some()); +} + +#[test] +fn latest_ref_cache_debounces_consecutive_refreshes() { + let _guard = HomeGuard::new(); + let _first_id = save_ref(1, Some("First")); + let store = RefStore::new().unwrap(); + + let mut cache = LatestRefCache::new(&store).unwrap(); + let pinned_snapshot_id = cache.snapshot_id.clone(); + let pinned_refresh = cache.last_refresh; + + let _ = save_ref(2, None); + + cache.last_refresh = std::time::Instant::now(); + cache.refresh_if_due(); + + assert_eq!(cache.snapshot_id, pinned_snapshot_id); + assert_eq!(cache.last_refresh, pinned_refresh.max(cache.last_refresh)); +} diff --git a/crates/core/src/commands/wait_predicate.rs b/crates/core/src/commands/wait_predicate.rs index 4a10831..e5a4748 100644 --- a/crates/core/src/commands/wait_predicate.rs +++ b/crates/core/src/commands/wait_predicate.rs @@ -77,7 +77,7 @@ pub(crate) fn satisfied(predicate: &ElementPredicate, observed: &Value) -> bool ElementPredicate::Enabled => observed["enabled"].as_bool() == Some(true), ElementPredicate::Visible => observed["visible"].as_bool() == Some(true), ElementPredicate::Actionable => observed["actionable"].as_bool() == Some(true), - ElementPredicate::Value(expected) => observed["value"].as_str() == Some(expected.as_str()), + ElementPredicate::Value(_) => observed["matched"].as_bool() == Some(true), } } @@ -132,5 +132,11 @@ fn value( .ok() .flatten() .or(entry.value.clone()); - json!({ "value": observed, "expected": expected }) + let matched = observed.as_deref() == Some(expected); + json!({ + "matched": matched, + "value_present": observed.is_some(), + "value_chars": observed.as_ref().map(|value| value.chars().count()), + "expected_chars": expected.chars().count() + }) } diff --git a/crates/core/src/commands/wait_tests.rs b/crates/core/src/commands/wait_tests.rs index 7b94f41..3f745b9 100644 --- a/crates/core/src/commands/wait_tests.rs +++ b/crates/core/src/commands/wait_tests.rs @@ -1,43 +1,14 @@ use super::*; use crate::{ - action::ElementState, - adapter::{NativeHandle, PlatformAdapter}, + adapter::PlatformAdapter, error::{AdapterError, ErrorCode}, - node::Rect, notification::{NotificationFilter, NotificationInfo}, - refs::{RefEntry, RefMap}, - refs_store::RefStore, - refs_test_support::HomeGuard, }; struct NoopAdapter; impl PlatformAdapter for NoopAdapter {} -struct PredicateAdapter { - state: Option, - value: Option, - bounds: Option, -} - -impl PlatformAdapter for PredicateAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Ok(NativeHandle::null()) - } - - fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { - Ok(self.state.clone()) - } - - fn get_live_value(&self, _handle: &NativeHandle) -> Result, AdapterError> { - Ok(self.value.clone()) - } - - fn get_element_bounds(&self, _handle: &NativeHandle) -> Result, AdapterError> { - Ok(self.bounds) - } -} - struct NotificationErrorAdapter; impl PlatformAdapter for NotificationErrorAdapter { @@ -52,79 +23,6 @@ impl PlatformAdapter for NotificationErrorAdapter { } } -struct AmbiguousResolveAdapter; - -impl PlatformAdapter for AmbiguousResolveAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Err(AdapterError::ambiguous_target("2 candidates matched")) - } -} - -fn snapshot_with_one_ref() -> String { - let mut refmap = RefMap::new(); - refmap.allocate(RefEntry { - pid: 1, - role: "button".into(), - name: Some("Run".into()), - value: None, - description: None, - states: Vec::new(), - bounds: None, - bounds_hash: None, - available_actions: vec!["Click".into()], - source_app: None, - source_window_id: None, - source_window_title: None, - source_surface: crate::adapter::SnapshotSurface::Window, - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() -} - -fn snapshot_with_disabled_ref() -> String { - let mut refmap = RefMap::new(); - refmap.allocate(RefEntry { - pid: 1, - role: "button".into(), - name: Some("Run".into()), - value: None, - description: None, - states: vec!["disabled".into()], - bounds: None, - bounds_hash: None, - available_actions: vec!["Click".into()], - source_app: None, - source_window_id: None, - source_window_title: None, - source_surface: crate::adapter::SnapshotSurface::Window, - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() -} - -#[test] -fn snapshot_pinned_missing_ref_is_invalid_args() { - let _guard = HomeGuard::new(); - let snapshot_id = snapshot_with_one_ref(); - - let err = wait_for_element( - "@e2".into(), - Some(snapshot_id), - wait_predicate::ElementPredicate::Exists, - 1, - &NoopAdapter, - &crate::context::CommandContext::default(), - ) - .unwrap_err(); - - assert_eq!(err.code(), "INVALID_ARGS"); - assert!(err.suggestion().is_some()); -} - #[test] fn notification_wait_propagates_adapter_error() { let err = execute( @@ -197,141 +95,6 @@ fn notification_wait_allows_text_filter() { assert!(result.is_ok()); } -#[test] -fn element_wait_enabled_predicate_uses_live_state() { - let _guard = HomeGuard::new(); - let snapshot_id = snapshot_with_one_ref(); - let adapter = PredicateAdapter { - state: Some(ElementState { - role: "button".into(), - states: vec![], - value: None, - }), - value: None, - bounds: None, - }; - - let value = wait_for_element( - "@e1".into(), - Some(snapshot_id), - wait_predicate::ElementPredicate::Enabled, - 1, - &adapter, - &crate::context::CommandContext::default(), - ) - .unwrap(); - - assert_eq!(value["predicate"], "enabled"); - assert_eq!(value["observed"]["enabled"], true); -} - -#[test] -fn element_wait_value_predicate_matches_live_value() { - let _guard = HomeGuard::new(); - let snapshot_id = snapshot_with_one_ref(); - let adapter = PredicateAdapter { - state: None, - value: Some("ready".into()), - bounds: None, - }; - - let value = wait_for_element( - "@e1".into(), - Some(snapshot_id), - wait_predicate::ElementPredicate::Value("ready".into()), - 1, - &adapter, - &crate::context::CommandContext::default(), - ) - .unwrap(); - - assert_eq!(value["predicate"], "value"); - assert_eq!(value["observed"]["value"], "ready"); -} - -#[test] -fn element_wait_timeout_reports_last_actionability_observation() { - let _guard = HomeGuard::new(); - let snapshot_id = snapshot_with_disabled_ref(); - let adapter = PredicateAdapter { - state: Some(ElementState { - role: "button".into(), - states: vec!["disabled".into()], - value: None, - }), - value: None, - bounds: None, - }; - - let err = wait_for_element( - "@e1".into(), - Some(snapshot_id), - wait_predicate::ElementPredicate::Actionable, - 1, - &adapter, - &crate::context::CommandContext::default(), - ) - .unwrap_err(); - - assert_eq!(err.code(), "TIMEOUT"); - assert!(err.to_string().contains("\"actionable\":false")); - assert!(err.to_string().contains("entry state contains disabled")); - match err { - AppError::Adapter(adapter_error) => { - let details = adapter_error.details.unwrap(); - assert_eq!(details["predicate"], "actionable"); - assert_eq!(details["last_observed"]["actionable"], false); - } - _ => panic!("expected adapter error"), - } -} - -#[test] -fn element_wait_actionable_uses_live_state() { - let _guard = HomeGuard::new(); - let snapshot_id = snapshot_with_disabled_ref(); - let adapter = PredicateAdapter { - state: Some(ElementState { - role: "button".into(), - states: vec![], - value: None, - }), - value: None, - bounds: None, - }; - - let value = wait_for_element( - "@e1".into(), - Some(snapshot_id), - wait_predicate::ElementPredicate::Actionable, - 1, - &adapter, - &crate::context::CommandContext::default(), - ) - .unwrap(); - - assert_eq!(value["predicate"], "actionable"); - assert_eq!(value["observed"]["actionable"], true); -} - -#[test] -fn element_wait_propagates_ambiguous_resolution() { - let _guard = HomeGuard::new(); - let snapshot_id = snapshot_with_one_ref(); - - let err = wait_for_element( - "@e1".into(), - Some(snapshot_id), - wait_predicate::ElementPredicate::Exists, - 1, - &AmbiguousResolveAdapter, - &crate::context::CommandContext::default(), - ) - .unwrap_err(); - - assert_eq!(err.code(), "AMBIGUOUS_TARGET"); -} - #[test] fn predicate_requires_element_mode() { let err = validate_wait_mode(&WaitArgs { @@ -353,82 +116,3 @@ fn predicate_requires_element_mode() { assert_eq!(err.code(), "INVALID_ARGS"); } - -#[test] -fn latest_ref_cache_picks_up_newer_snapshot_after_refresh() { - let _guard = HomeGuard::new(); - let _ = snapshot_with_one_ref(); - let store = RefStore::new().unwrap(); - let first_id = store.latest_snapshot_id().unwrap(); - - let mut cache = LatestRefCache::new(&store).unwrap(); - assert_eq!(cache.snapshot_id.as_deref(), Some(first_id.as_str())); - - let mut second = RefMap::new(); - second.allocate(RefEntry { - pid: 99, - role: "button".into(), - name: Some("Second".into()), - value: None, - description: None, - states: vec![], - bounds: None, - bounds_hash: None, - available_actions: vec!["Click".into()], - source_app: None, - source_window_id: None, - source_window_title: None, - source_surface: crate::adapter::SnapshotSurface::Window, - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }); - let second_id = store.save_new_snapshot(&second).unwrap(); - assert_ne!(first_id, second_id); - - cache.last_refresh = std::time::Instant::now() - std::time::Duration::from_secs(2); - cache.refresh_if_due(); - - assert_eq!(cache.snapshot_id.as_deref(), Some(second_id.as_str())); - assert!(cache.entry("@e1").is_some()); -} - -#[test] -fn latest_ref_cache_debounces_consecutive_refreshes() { - let _guard = HomeGuard::new(); - let _ = snapshot_with_one_ref(); - let store = RefStore::new().unwrap(); - let first_id = store.latest_snapshot_id().unwrap(); - - let mut cache = LatestRefCache::new(&store).unwrap(); - let pinned_snapshot_id = cache.snapshot_id.clone(); - let pinned_refresh = cache.last_refresh; - - let mut other = RefMap::new(); - other.allocate(RefEntry { - pid: 1, - role: "button".into(), - name: None, - value: None, - description: None, - states: vec![], - bounds: None, - bounds_hash: None, - available_actions: vec!["Click".into()], - source_app: None, - source_window_id: None, - source_window_title: None, - source_surface: crate::adapter::SnapshotSurface::Window, - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }); - let _ = store.save_new_snapshot(&other).unwrap(); - - cache.last_refresh = std::time::Instant::now(); - cache.refresh_if_due(); - - assert_eq!(cache.snapshot_id, pinned_snapshot_id); - assert_eq!(cache.last_refresh, pinned_refresh.max(cache.last_refresh)); - let _ = first_id; -} diff --git a/crates/core/src/context.rs b/crates/core/src/context.rs index 48a9d63..73f14ec 100644 --- a/crates/core/src/context.rs +++ b/crates/core/src/context.rs @@ -124,6 +124,60 @@ mod tests { let _ = std::fs::remove_file(path); } + #[cfg(unix)] + #[test] + fn trace_rejects_loose_existing_file_permissions() { + use std::os::unix::fs::PermissionsExt; + + let path = std::env::temp_dir().join(format!( + "agent-desktop-loose-trace-{}.jsonl", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + std::fs::write(&path, "").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let err = CommandContext::new(None, Some(path.clone()), false).unwrap_err(); + + assert_eq!(err.code(), "INVALID_ARGS"); + let _ = std::fs::remove_file(path); + } + + #[test] + fn trace_redacts_sensitive_text_and_value_fields() { + let path = std::env::temp_dir().join(format!( + "agent-desktop-redacted-trace-{}.jsonl", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let context = CommandContext::new(None, Some(path.clone()), true).unwrap(); + context + .trace( + "event", + serde_json::json!({ + "text": "secret", + "value": "hidden", + "nested": { "expected": "token" } + }), + ) + .unwrap(); + + let body = std::fs::read_to_string(&path).unwrap(); + let event: serde_json::Value = serde_json::from_str(body.trim()).unwrap(); + assert_eq!(event["text"]["redacted"], true); + assert_eq!(event["text"]["chars"], 6); + assert_eq!(event["value"]["redacted"], true); + assert_eq!(event["nested"]["expected"]["redacted"], true); + assert!(!body.contains("secret")); + assert!(!body.contains("hidden")); + assert!(!body.contains("token")); + let _ = std::fs::remove_file(path); + } + #[test] fn trace_strict_requires_trace_path() { let err = CommandContext::new(None, None, true).unwrap_err(); diff --git a/crates/core/src/trace.rs b/crates/core/src/trace.rs index 53fb1f4..f8a3d3e 100644 --- a/crates/core/src/trace.rs +++ b/crates/core/src/trace.rs @@ -21,6 +21,7 @@ impl TraceConfig { let writer = match path.as_deref() { Some(path) => match open_trace_file(path) { Ok(file) => Some(Arc::new(Mutex::new(file))), + Err(err) if err.code() == "INVALID_ARGS" => return Err(err), Err(err) if strict => return Err(err), Err(err) => { tracing::warn!("trace open failed: {err}"); @@ -63,7 +64,9 @@ fn open_trace_file(path: &Path) -> Result { use std::os::unix::fs::OpenOptionsExt; options.mode(0o600); } - options.open(path).map_err(AppError::from) + let file = options.open(path).map_err(AppError::from)?; + reject_loose_trace_permissions(&file)?; + Ok(file) } fn write_event(file: &mut std::fs::File, event: &str, fields: Value) -> Result<(), AppError> { @@ -78,7 +81,7 @@ fn write_event(file: &mut std::fs::File, event: &str, fields: Value) -> Result<( .as_millis() ), ); - if let Value::Object(fields) = fields { + if let Value::Object(fields) = sanitize_trace_value(fields) { for (key, value) in fields { body.insert(key, value); } @@ -87,3 +90,55 @@ fn write_event(file: &mut std::fs::File, event: &str, fields: Value) -> Result<( use std::io::Write; file.write_all(b"\n").map_err(AppError::from) } + +#[cfg(unix)] +fn reject_loose_trace_permissions(file: &std::fs::File) -> Result<(), AppError> { + use std::os::unix::fs::PermissionsExt; + + let mode = file.metadata()?.permissions().mode() & 0o777; + if mode & 0o077 == 0 { + return Ok(()); + } + Err(AppError::invalid_input_with_suggestion( + "Trace file must not be readable or writable by group/other", + "Use a new --trace path or run chmod 600 on the existing trace file.", + )) +} + +#[cfg(not(unix))] +fn reject_loose_trace_permissions(_file: &std::fs::File) -> Result<(), AppError> { + Ok(()) +} + +fn sanitize_trace_value(value: Value) -> Value { + match value { + Value::Object(map) => Value::Object( + map.into_iter() + .map(|(key, value)| { + if is_sensitive_trace_key(&key) { + (key, redacted_value(value)) + } else { + (key, sanitize_trace_value(value)) + } + }) + .collect(), + ), + Value::Array(items) => Value::Array(items.into_iter().map(sanitize_trace_value).collect()), + other => other, + } +} + +fn is_sensitive_trace_key(key: &str) -> bool { + let key = key.to_ascii_lowercase(); + key.contains("text") || key.contains("value") || key == "expected" +} + +fn redacted_value(value: Value) -> Value { + match value { + Value::String(text) => json!({ "redacted": true, "chars": text.chars().count() }), + Value::Array(items) => json!({ "redacted": true, "items": items.len() }), + Value::Object(map) => json!({ "redacted": true, "keys": map.len() }), + Value::Null => Value::Null, + _ => json!({ "redacted": true }), + } +} diff --git a/crates/ffi/Cargo.toml b/crates/ffi/Cargo.toml index 397d6db..df24d46 100644 --- a/crates/ffi/Cargo.toml +++ b/crates/ffi/Cargo.toml @@ -10,6 +10,7 @@ crate-type = ["cdylib", "rlib"] [dependencies] smallvec.workspace = true +serde_json.workspace = true agent-desktop-core.workspace = true [target.'cfg(target_os = "macos")'.dependencies] diff --git a/crates/ffi/include/agent_desktop.h b/crates/ffi/include/agent_desktop.h index 1488f43..76f3167 100644 --- a/crates/ffi/include/agent_desktop.h +++ b/crates/ffi/include/agent_desktop.h @@ -614,6 +614,13 @@ const char *ad_last_error_suggestion(void); */ const char *ad_last_error_platform_detail(void); +/** + * Returns a borrowed JSON string with structured details for the last + * error, or null if the adapter didn't supply one. Same lifetime rules + * as `ad_last_error_message`. + */ +const char *ad_last_error_details(void); + /** * Reads the current clipboard text and writes an owned C string into * `*out`. The caller must free the returned pointer with diff --git a/crates/ffi/src/error.rs b/crates/ffi/src/error.rs index 902da0f..0723c38 100644 --- a/crates/ffi/src/error.rs +++ b/crates/ffi/src/error.rs @@ -125,6 +125,7 @@ struct StoredError { message: MessageSource, suggestion: Option, platform_detail: Option, + details: Option, } static NUL_BYTE_FALLBACK: &CStr = c"(message contained null byte)"; @@ -164,12 +165,18 @@ pub(crate) fn set_last_error(err: &AdapterError) { .platform_detail .as_deref() .and_then(|s| CString::new(s).ok()); + let details = err + .details + .as_ref() + .and_then(|details| serde_json::to_string(details).ok()) + .and_then(|details| CString::new(details).ok()); LAST_ERROR.with(|cell| { *cell.borrow_mut() = Some(StoredError { code, message, suggestion, platform_detail, + details, }); }); } @@ -190,6 +197,7 @@ pub(crate) fn set_last_error_static(code: AdResult, message: &'static CStr) { message: MessageSource::Static(message), suggestion: None, platform_detail: None, + details: None, }); }); } @@ -272,6 +280,21 @@ pub extern "C" fn ad_last_error_platform_detail() -> *const c_char { }) } +/// Returns a borrowed JSON string carrying structured details for the last +/// error, or null if the adapter didn't supply any. Same lifetime rules as +/// `ad_last_error_message`. +#[unsafe(no_mangle)] +pub extern "C" fn ad_last_error_details() -> *const c_char { + crate::ffi_try::trap_panic_const_ptr(|| { + LAST_ERROR.with(|cell| { + cell.borrow() + .as_ref() + .and_then(|e| e.details.as_ref().map(|s| s.as_ptr())) + .unwrap_or(std::ptr::null()) + }) + }) +} + #[cfg(test)] #[path = "error_tests.rs"] mod tests; diff --git a/crates/ffi/src/error_tests.rs b/crates/ffi/src/error_tests.rs index 58af4fa..0348257 100644 --- a/crates/ffi/src/error_tests.rs +++ b/crates/ffi/src/error_tests.rs @@ -24,6 +24,14 @@ fn last_error_platform_detail_str() -> Option { }) } +fn last_error_details_str() -> Option { + LAST_ERROR.with(|cell| { + cell.borrow() + .as_ref() + .and_then(|e| e.details.as_ref().map(|s| s.to_string_lossy().into_owned())) + }) +} + #[test] fn test_no_error_initially() { clear_last_error(); @@ -47,6 +55,23 @@ fn test_set_and_get_error() { assert_eq!(last_error_message_str().unwrap(), "element @e5 gone"); assert_eq!(last_error_suggestion_str().unwrap(), "run snapshot"); assert!(last_error_platform_detail_str().is_none()); + assert!(last_error_details_str().is_none()); +} + +#[test] +fn test_set_and_get_structured_details() { + let err = AdapterError::new(ErrorCode::AmbiguousTarget, "ambiguous").with_details( + serde_json::json!({ + "candidate_count": 2, + "role": "button" + }), + ); + set_last_error(&err); + + let details: serde_json::Value = + serde_json::from_str(&last_error_details_str().unwrap()).unwrap(); + assert_eq!(details["candidate_count"], 2); + assert_eq!(details["role"], "button"); } #[test] diff --git a/crates/ffi/src/observation/find.rs b/crates/ffi/src/observation/find.rs index 61c9e5c..401c4b5 100644 --- a/crates/ffi/src/observation/find.rs +++ b/crates/ffi/src/observation/find.rs @@ -109,7 +109,7 @@ pub unsafe extern "C" fn ad_find( path_is_absolute: false, path: smallvec::SmallVec::new(), }; - match adapter.inner.resolve_element(&ref_entry) { + match adapter.inner.resolve_element_strict(&ref_entry) { Ok(handle) => { let handle = ManuallyDrop::new(handle); (*out_handle).ptr = handle.as_raw(); diff --git a/crates/ffi/src/types/ref_entry.rs b/crates/ffi/src/types/ref_entry.rs index 03d446b..4706a15 100644 --- a/crates/ffi/src/types/ref_entry.rs +++ b/crates/ffi/src/types/ref_entry.rs @@ -26,9 +26,9 @@ pub struct AdRefEntry { pub path_count: usize, } -pub const AD_REF_ENTRY_SIZE: usize = std::mem::size_of::(); +pub const AD_REF_ENTRY_SIZE: usize = 192; #[unsafe(no_mangle)] pub extern "C" fn ad_ref_entry_size() -> usize { - AD_REF_ENTRY_SIZE + std::mem::size_of::() } diff --git a/crates/ffi/tests/c_abi_layout.rs b/crates/ffi/tests/c_abi_layout.rs index 581625a..11a0b4f 100644 --- a/crates/ffi/tests/c_abi_layout.rs +++ b/crates/ffi/tests/c_abi_layout.rs @@ -25,14 +25,12 @@ fn rect_and_point_layouts_are_memcpyable() { #[test] fn ref_entry_layout_is_guarded_for_c_consumers() { - assert_eq!( - agent_desktop_ffi::types::ref_entry::AD_REF_ENTRY_SIZE, - size_of::() - ); + assert_eq!(agent_desktop_ffi::types::ref_entry::AD_REF_ENTRY_SIZE, 192); assert_eq!( unsafe { common::ad_ref_entry_size() }, - size_of::() + agent_desktop_ffi::types::ref_entry::AD_REF_ENTRY_SIZE ); + assert_eq!(size_of::(), 192); assert_eq!(align_of::(), align_of::()); assert_eq!(offset_of!(AdRefEntry, pid), 0); diff --git a/crates/ffi/tests/c_header_compile.rs b/crates/ffi/tests/c_header_compile.rs index 3734ec9..4f7de8d 100644 --- a/crates/ffi/tests/c_header_compile.rs +++ b/crates/ffi/tests/c_header_compile.rs @@ -63,7 +63,9 @@ int main(void) { (void)AD_IMAGE_FORMAT_PNG; (void)AD_RESULT_OK; (void)ad_ref_entry_size; + (void)ad_last_error_details; _Static_assert(AD_REF_ENTRY_SIZE == sizeof(AdRefEntry), "AdRefEntry size macro drifted"); + _Static_assert(AD_REF_ENTRY_SIZE == 192, "AdRefEntry ABI size changed"); return 0; } "#; diff --git a/crates/ffi/tests/common/mod.rs b/crates/ffi/tests/common/mod.rs index e6e40fe..c536546 100644 --- a/crates/ffi/tests/common/mod.rs +++ b/crates/ffi/tests/common/mod.rs @@ -19,6 +19,7 @@ unsafe extern "C" { pub fn ad_last_error_code() -> AdResult; pub fn ad_last_error_message() -> *const c_char; + pub fn ad_last_error_details() -> *const c_char; pub fn ad_list_apps(adapter: *const AdAdapter, out: *mut *mut AdAppList) -> AdResult; pub fn ad_app_list_count(list: *const AdAppList) -> u32; diff --git a/crates/ffi/tests/error_lifetime.rs b/crates/ffi/tests/error_lifetime.rs index a03a7e9..a55308a 100644 --- a/crates/ffi/tests/error_lifetime.rs +++ b/crates/ffi/tests/error_lifetime.rs @@ -12,6 +12,7 @@ unsafe extern "C" { out: *mut agent_desktop_ffi::AdWindowInfo, ) -> AdResult; fn ad_last_error_message() -> *const std::os::raw::c_char; + fn ad_last_error_details() -> *const std::os::raw::c_char; fn ad_last_error_code() -> AdResult; } @@ -34,7 +35,9 @@ fn last_error_pointer_survives_across_successful_calls() { )); let first_msg_ptr = ad_last_error_message(); + let first_details_ptr = ad_last_error_details(); assert!(!first_msg_ptr.is_null()); + assert!(first_details_ptr.is_null()); let first_msg = CStr::from_ptr(first_msg_ptr).to_string_lossy().into_owned(); for _ in 0..10 { @@ -45,6 +48,7 @@ fn last_error_pointer_survives_across_successful_calls() { let later_msg_ptr = ad_last_error_message(); assert_eq!(first_msg_ptr, later_msg_ptr); + assert_eq!(first_details_ptr, ad_last_error_details()); let later_msg = CStr::from_ptr(later_msg_ptr).to_string_lossy().into_owned(); assert_eq!(first_msg, later_msg); assert_eq!(ad_last_error_code(), rc); diff --git a/crates/macos/src/adapter.rs b/crates/macos/src/adapter.rs index 6dba744..a837d96 100644 --- a/crates/macos/src/adapter.rs +++ b/crates/macos/src/adapter.rs @@ -81,10 +81,6 @@ impl PlatformAdapter for MacOSAdapter { execute_action_impl(handle, request) } - fn resolve_element(&self, entry: &RefEntry) -> Result { - self.resolve_element_strict(entry) - } - fn resolve_element_strict(&self, entry: &RefEntry) -> Result { crate::tree::resolve::resolve_element_impl(entry) } @@ -189,6 +185,24 @@ impl PlatformAdapter for MacOSAdapter { Err(AdapterError::not_supported("get_live_state")) } + fn get_live_actions(&self, handle: &NativeHandle) -> Result>, AdapterError> { + #[cfg(target_os = "macos")] + { + use crate::tree::AXElement; + use std::mem::ManuallyDrop; + let el = ManuallyDrop::new(AXElement( + handle.as_raw() as accessibility_sys::AXUIElementRef + )); + let state = crate::actions::post_state::read_element_state(&el); + Ok(Some(crate::tree::action_list::platform_available_actions( + &el, + &state.role, + ))) + } + #[cfg(not(target_os = "macos"))] + Err(AdapterError::not_supported("get_live_actions")) + } + fn get_element_bounds(&self, handle: &NativeHandle) -> Result, AdapterError> { #[cfg(target_os = "macos")] { diff --git a/crates/macos/src/tree/action_list.rs b/crates/macos/src/tree/action_list.rs index e67609c..9144d50 100644 --- a/crates/macos/src/tree/action_list.rs +++ b/crates/macos/src/tree/action_list.rs @@ -1,5 +1,8 @@ use super::AXElement; -use super::capabilities::{copy_action_names, is_attr_settable}; +use super::{ + capabilities::{copy_action_names, is_attr_settable}, + copy_element_attr, +}; #[cfg(target_os = "macos")] use accessibility_sys::{kAXFocusedAttribute, kAXValueAttribute}; @@ -23,8 +26,12 @@ pub(crate) fn platform_available_actions(el: &AXElement, role: &str) -> Vec bool { !matches!(role, "combobox" | "menubutton") } +fn has_scroll_mechanism(el: &AXElement, role: &str, has: &impl Fn(&str) -> bool) -> bool { + role_supports_scroll(role) + || has("AXScrollDownByPage") + || has("AXScrollUpByPage") + || has("AXScrollLeftByPage") + || has("AXScrollRightByPage") + || copy_element_attr(el, "AXVerticalScrollBar").is_some() + || copy_element_attr(el, "AXHorizontalScrollBar").is_some() +} + +fn role_supports_scroll(role: &str) -> bool { + matches!( + role, + "scrollarea" | "browser" | "table" | "outline" | "list" + ) +} + #[cfg(test)] mod tests { - use super::role_allows_context_menu_action; + use super::{role_allows_context_menu_action, role_supports_scroll}; #[test] fn menu_opening_controls_do_not_advertise_right_click() { @@ -65,4 +89,11 @@ mod tests { assert!(role_allows_context_menu_action("textfield")); assert!(role_allows_context_menu_action("button")); } + + #[test] + fn scroll_container_roles_advertise_scroll_without_scroll_to() { + assert!(role_supports_scroll("scrollarea")); + assert!(role_supports_scroll("browser")); + assert!(!role_supports_scroll("button")); + } } diff --git a/crates/macos/src/tree/resolve_tests.rs b/crates/macos/src/tree/resolve_tests.rs index 510e938..f2ccd13 100644 --- a/crates/macos/src/tree/resolve_tests.rs +++ b/crates/macos/src/tree/resolve_tests.rs @@ -150,6 +150,26 @@ fn only_element_not_found_is_retryable_resolution_error() { ))); } +#[test] +fn ambiguous_candidate_classification_reports_structured_details() { + let err = match classify_candidates( + vec![ + AXElement(std::ptr::null_mut()), + AXElement(std::ptr::null_mut()), + ], + &entry(None, Some("w-42"), Some("Documents"), None), + ) { + Ok(_) => panic!("expected ambiguous target"), + Err(err) => err, + }; + + assert_eq!(err.code, ErrorCode::AmbiguousTarget); + let details = err.details.unwrap(); + assert_eq!(details["candidate_count"], 2); + assert_eq!(details["role"], "cell"); + assert_eq!(details["source_window_id"], "w-42"); +} + fn description_entry() -> RefEntry { let mut entry = entry(None, Some("w-10"), Some("Freeform"), None); entry.role = "button".into();