From f92b6cc92eaa4fb8ce9ee9eed0deed7742e535d3 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Wed, 3 Jun 2026 17:37:45 -0700 Subject: [PATCH] fix: harden ref action reliability --- README.md | 6 +- crates/core/src/action.rs | 45 +++++++ crates/core/src/actionability.rs | 89 ++++++++++---- crates/core/src/actionability_tests.rs | 47 ++++++-- crates/core/src/adapter.rs | 8 ++ crates/core/src/commands/get.rs | 27 ++--- crates/core/src/commands/helpers.rs | 55 ++++----- crates/core/src/commands/helpers_tests.rs | 14 ++- crates/core/src/commands/is_check.rs | 1 - crates/core/src/commands/right_click.rs | 1 - crates/core/src/commands/snapshot.rs | 1 - crates/core/src/commands/status.rs | 2 - crates/core/src/commands/wait.rs | 103 +++++++++++----- crates/core/src/commands/wait_mode.rs | 46 ++++---- crates/core/src/commands/wait_tests.rs | 137 ++++++++++++++-------- crates/core/src/output.rs | 4 +- crates/core/src/ref_action.rs | 15 +-- crates/core/src/ref_alloc.rs | 1 - crates/core/src/ref_alloc_tests.rs | 14 ++- crates/core/src/snapshot.rs | 4 +- crates/core/src/snapshot_ref.rs | 2 +- crates/core/src/trace.rs | 28 +++++ crates/ffi/include/agent_desktop.h | 11 ++ crates/ffi/src/actions/result.rs | 1 + crates/macos/src/actions/chain.rs | 14 ++- crates/macos/src/actions/dispatch.rs | 55 +++++++-- crates/macos/src/actions/post_state.rs | 16 +-- crates/macos/src/tree/builder.rs | 6 +- crates/macos/src/tree/element.rs | 18 ++- crates/macos/src/tree/element_dedupe.rs | 25 ++-- crates/macos/src/tree/node_attrs.rs | 7 ++ crates/macos/src/tree/resolve.rs | 33 +++++- crates/macos/src/tree/resolve_roots.rs | 2 +- src/Cargo.toml | 4 + src/batch/tests.rs | 11 ++ src/cli/help_after.txt | 4 +- src/cli/mod.rs | 2 +- src/cli_args/system.rs | 68 +++++++---- src/command_policy/mod.rs | 2 +- src/dispatch/mod.rs | 26 ++-- src/dispatch/notifications.rs | 9 +- src/tests/conformance.rs | 114 ++++++++++++++++++ tests/conformance/README.md | 4 + 43 files changed, 771 insertions(+), 311 deletions(-) create mode 100644 src/tests/conformance.rs diff --git a/README.md b/README.md index 5e3668c..7e9ef5a 100644 --- a/README.md +++ b/README.md @@ -189,6 +189,8 @@ agent-desktop is @e7 --snapshot s8f3k2p9 --property checked # check boolean stat agent-desktop list-surfaces --app Notes # list menus, sheets, popovers, alerts ``` +`get` and `is` resolve the ref once, prefer live platform reads when available, and fall back only when that live read is unsupported by the adapter. + ### Interaction ```bash @@ -388,9 +390,9 @@ Reliability contract: - `--session ` scopes snapshots, refs, and the latest snapshot pointer to one caller or agent team. - Ref actions use strict re-identification and return `STALE_REF` instead of acting on a changed target. - Multiple plausible targets return `AMBIGUOUS_TARGET` instead of choosing arbitrarily. -- Actions run an actionability preflight before dispatch: visibility, enabled state, supported action, policy, and editability. +- Actions run an actionability preflight before dispatch: visibility, stability, enabled state, supported action, policy, and editability. - `wait --element @e3 --predicate actionable` polls until the target can be acted on. -- `--trace ` appends JSONL diagnostics outside stdout; add `--trace-strict` to fail if trace writing fails. +- `--trace ` appends JSONL diagnostics outside stdout; `--trace-strict` fails on trace setup and pre-action trace writes, while post-action success traces are best-effort after the desktop mutation has already happened. Stale ref recovery: diff --git a/crates/core/src/action.rs b/crates/core/src/action.rs index ceb0dd6..d3e70de 100644 --- a/crates/core/src/action.rs +++ b/crates/core/src/action.rs @@ -247,6 +247,8 @@ pub struct ActionResult { pub ref_id: Option, #[serde(skip_serializing_if = "Option::is_none")] pub post_state: Option, + #[serde(skip_serializing_if = "Vec::is_empty", default)] + pub steps: Vec, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -258,12 +260,50 @@ pub struct ElementState { pub value: Option, } +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ActionStep { + pub label: String, + pub outcome: ActionStepOutcome, +} + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ActionStepOutcome { + Attempted, + Skipped, + Succeeded, +} + +impl ActionStep { + pub fn attempted(label: impl Into) -> Self { + Self { + label: label.into(), + outcome: ActionStepOutcome::Attempted, + } + } + + pub fn skipped(label: impl Into) -> Self { + Self { + label: label.into(), + outcome: ActionStepOutcome::Skipped, + } + } + + pub fn succeeded(label: impl Into) -> Self { + Self { + label: label.into(), + outcome: ActionStepOutcome::Succeeded, + } + } +} + impl ActionResult { pub fn new(action: impl Into) -> Self { Self { action: action.into(), ref_id: None, post_state: None, + steps: Vec::new(), } } @@ -276,4 +316,9 @@ impl ActionResult { self.post_state = Some(state); self } + + pub fn with_steps(mut self, steps: Vec) -> Self { + self.steps = steps; + self + } } diff --git a/crates/core/src/actionability.rs b/crates/core/src/actionability.rs index f22b4cb..b8a0a90 100644 --- a/crates/core/src/actionability.rs +++ b/crates/core/src/actionability.rs @@ -30,31 +30,11 @@ pub struct ActionabilityReport { pub checks: Vec, } -pub(crate) fn check( +pub fn check( entry: &RefEntry, request: &ActionRequest, ) -> Result { - let checks = vec![ - visibility_check(entry), - enabled_check(entry), - action_supported_check(entry, request), - policy_check(request), - editable_check(entry, &request.action), - ]; - - let actionable = checks - .iter() - .all(|check| !matches!(check.status, ActionabilityStatus::Fail)); - let report = ActionabilityReport { actionable, checks }; - if report.actionable { - return Ok(report); - } - Err(AdapterError::new( - ErrorCode::ActionFailed, - format!("Target is not actionable: {}", failure_reasons(&report)), - ) - .with_details(json!(report)) - .with_suggestion("Wait for the target to become actionable, refresh the snapshot, or use an explicit physical/focus command if intended.")) + check_with_stability(entry.bounds_hash, entry, request) } pub fn check_live( @@ -70,15 +50,52 @@ pub fn check_live( observed.states = state.states; observed.value = state.value.or(observed.value); } - if let Some(bounds) = live.bounds { - observed.bounds = Some(bounds); - } + observed.bounds = live.bounds; if let Some(actions) = live.available_actions && !actions.is_empty() { observed.available_actions = actions; } - check(&observed, request) + check_with_stability(entry.bounds_hash, &observed, request) +} + +fn check_with_stability( + expected_bounds_hash: Option, + entry: &RefEntry, + request: &ActionRequest, +) -> Result { + let checks = vec![ + visibility_check(entry), + stability_check(expected_bounds_hash, entry.bounds), + enabled_check(entry), + action_supported_check(entry, request), + policy_check(request), + editable_check(entry, &request.action), + ]; + + let actionable = checks + .iter() + .all(|check| !matches!(check.status, ActionabilityStatus::Fail)); + let report = ActionabilityReport { actionable, checks }; + if report.actionable { + return Ok(report); + } + let code = if failed_check(&report, "stable") { + ErrorCode::StaleRef + } else { + ErrorCode::ActionFailed + }; + let suggestion = if code == ErrorCode::StaleRef { + "Run 'snapshot' to refresh, then retry with the updated ref." + } else { + "Wait for the target to become actionable, refresh the snapshot, or use an explicit physical/focus command if intended." + }; + Err(AdapterError::new( + code, + format!("Target is not actionable: {}", failure_reasons(&report)), + ) + .with_details(json!(report)) + .with_suggestion(suggestion)) } fn visibility_check(entry: &RefEntry) -> ActionabilityCheck { @@ -91,6 +108,19 @@ fn visibility_check(entry: &RefEntry) -> ActionabilityCheck { pass("visible") } +fn stability_check(expected_bounds_hash: Option, bounds: Option) -> ActionabilityCheck { + let Some(expected) = expected_bounds_hash else { + return unknown("stable", "snapshot bounds hash unavailable"); + }; + let Some(bounds) = bounds else { + return unknown("stable", "live bounds unavailable"); + }; + if bounds.bounds_hash() != expected { + return fail("stable", "bounds changed since snapshot"); + } + pass("stable") +} + fn enabled_check(entry: &RefEntry) -> ActionabilityCheck { if !states_are_enabled(&entry.states) { return fail("enabled", "entry state contains disabled"); @@ -169,6 +199,13 @@ fn failure_reasons(report: &ActionabilityReport) -> String { .join(", ") } +fn failed_check(report: &ActionabilityReport, name: &str) -> bool { + report + .checks + .iter() + .any(|check| check.name == name && matches!(check.status, ActionabilityStatus::Fail)) +} + fn supported_by_available_actions(action: &Action, available_actions: &[String]) -> bool { action .semantic_capabilities() diff --git a/crates/core/src/actionability_tests.rs b/crates/core/src/actionability_tests.rs index d808faa..21ccfb0 100644 --- a/crates/core/src/actionability_tests.rs +++ b/crates/core/src/actionability_tests.rs @@ -78,6 +78,12 @@ struct UnsupportedLiveAdapter; impl PlatformAdapter for UnsupportedLiveAdapter {} fn entry() -> RefEntry { + let bounds = Rect { + x: 1.0, + y: 1.0, + width: 20.0, + height: 20.0, + }; RefEntry { pid: 1, role: "button".into(), @@ -85,13 +91,8 @@ fn entry() -> RefEntry { value: None, description: None, states: vec![], - bounds: Some(Rect { - x: 1.0, - y: 1.0, - width: 20.0, - height: 20.0, - }), - bounds_hash: Some(1), + bounds: Some(bounds), + bounds_hash: Some(bounds.bounds_hash()), available_actions: vec!["Click".into()], source_app: None, source_window_id: None, @@ -125,12 +126,14 @@ fn disabled_entry_fails_before_action_dispatch() { #[test] fn zero_sized_bounds_fail_visibility() { let mut entry = entry(); - entry.bounds = Some(Rect { + let bounds = Rect { x: 1.0, y: 1.0, width: 0.0, height: 20.0, - }); + }; + entry.bounds = Some(bounds); + entry.bounds_hash = Some(bounds.bounds_hash()); let err = check(&entry, &ActionRequest::headless(Action::Click)).unwrap_err(); @@ -280,6 +283,32 @@ fn live_actionability_fails_when_action_disappears_after_snapshot() { assert!(err.message.contains("supported_action")); } +#[test] +fn live_actionability_fails_stale_when_bounds_changed_after_snapshot() { + let stale = entry(); + let adapter = LiveAdapter { + state: None, + bounds: Some(Rect { + x: 100.0, + y: 100.0, + width: 20.0, + height: 20.0, + }), + actions: Some(vec!["Click".into()]), + }; + + let err = check_live( + &stale, + &NativeHandle::null(), + &adapter, + &ActionRequest::headless(Action::Click), + ) + .unwrap_err(); + + assert_eq!(err.code, ErrorCode::StaleRef); + assert!(err.message.contains("stable")); +} + #[test] fn empty_live_actions_do_not_erase_snapshot_capabilities() { let stale = entry(); diff --git a/crates/core/src/adapter.rs b/crates/core/src/adapter.rs index 49ce159..a9fcfc6 100644 --- a/crates/core/src/adapter.rs +++ b/crates/core/src/adapter.rs @@ -34,6 +34,7 @@ impl SnapshotSurface { } } +#[derive(Clone, Copy)] pub struct TreeOptions { pub max_depth: u8, pub include_bounds: bool, @@ -56,6 +57,13 @@ impl Default for TreeOptions { } } +impl TreeOptions { + pub(crate) fn with_ref_identity_bounds(mut self) -> Self { + self.include_bounds = true; + self + } +} + #[derive(Debug, Clone, Default)] pub struct LiveElement { pub state: Option, diff --git a/crates/core/src/commands/get.rs b/crates/core/src/commands/get.rs index ef0f150..d5f02b1 100644 --- a/crates/core/src/commands/get.rs +++ b/crates/core/src/commands/get.rs @@ -29,24 +29,19 @@ pub fn execute( let (entry, handle) = resolve_ref_with_context(&args.ref_id, args.snapshot_id.as_deref(), adapter, context)?; - let value = match args.property { - GetProperty::Role => json!(entry.role), - GetProperty::Title => json!(entry.name), - GetProperty::Text | GetProperty::Value => { + let (prop_name, value) = match args.property { + GetProperty::Role => ("role", json!(entry.role)), + GetProperty::Title => ("title", json!(entry.name)), + GetProperty::Text => { let live = optional_live_read(adapter.get_live_value(handle.handle()))?; - json!(live.or(entry.value)) + ("text", json!(live.or(entry.value))) } - GetProperty::Bounds => json!(entry.bounds), - GetProperty::States => json!(entry.states), - }; - - let prop_name = match args.property { - GetProperty::Text => "text", - GetProperty::Value => "value", - GetProperty::Title => "title", - GetProperty::Bounds => "bounds", - GetProperty::Role => "role", - GetProperty::States => "states", + GetProperty::Value => { + let live = optional_live_read(adapter.get_live_value(handle.handle()))?; + ("value", json!(live.or(entry.value))) + } + GetProperty::Bounds => ("bounds", json!(entry.bounds)), + GetProperty::States => ("states", json!(entry.states)), }; Ok(json!({ "property": prop_name, "ref": args.ref_id, "value": value })) diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 11cac1b..29e4d14 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -27,12 +27,6 @@ pub(crate) struct PointResolveArgs<'a> { 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, @@ -166,46 +160,45 @@ 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( - ActionabilityTarget { - ref_id, - entry: &entry, - handle: handle.handle(), - }, - adapter, - &request, - context, + check_actionability_with_trace(ref_id, &entry, handle.handle(), adapter, &request, context)?; + context.trace_lazy( + "action.dispatch.start", + || json!({ "ref": ref_id, "action": request.action.name() }), )?; + let action_name = request.action.name(); let result = adapter.execute_action(handle.handle(), request)?; - context.trace_lazy("action.dispatch.ok", || json!({ "ref": ref_id }))?; + let _ = context.trace_lazy( + "action.dispatch.ok", + || json!({ "ref": ref_id, "action": action_name, "result": &result }), + ); Ok((entry, result)) } fn check_actionability_with_trace( - target: ActionabilityTarget<'_>, + ref_id: &str, + entry: &RefEntry, + handle: &NativeHandle, adapter: &dyn PlatformAdapter, request: &ActionRequest, context: &CommandContext, ) -> Result<(), AppError> { context.trace_lazy( "actionability.check.start", - || json!({ "ref": target.ref_id, "action": request.action.name() }), - )?; - crate::ref_action::check_resolved(adapter, target.entry, target.handle, request).inspect_err( - |err| { - let _ = context.trace_lazy("actionability.check.error", || { - json!({ - "ref": target.ref_id, - "action": request.action.name(), - "code": err.code.as_str(), - "message": err.message.clone() - }) - }); - }, + || json!({ "ref": ref_id, "action": request.action.name() }), )?; + crate::actionability::check_live(entry, handle, adapter, request).inspect_err(|err| { + let _ = context.trace_lazy("actionability.check.error", || { + json!({ + "ref": ref_id, + "action": request.action.name(), + "code": err.code.as_str(), + "message": err.message.clone() + }) + }); + })?; context.trace_lazy( "actionability.check.ok", - || json!({ "ref": target.ref_id, "action": request.action.name() }), + || json!({ "ref": ref_id, "action": request.action.name() }), )?; Ok(()) } diff --git a/crates/core/src/commands/helpers_tests.rs b/crates/core/src/commands/helpers_tests.rs index 7515065..f38d6fc 100644 --- a/crates/core/src/commands/helpers_tests.rs +++ b/crates/core/src/commands/helpers_tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::action::{Action, ActionResult, InteractionPolicy}; +use crate::action::{Action, ActionResult, ActionStep, ElementState, InteractionPolicy}; use crate::adapter::NativeHandle; use crate::error::{AdapterError, ErrorCode}; use crate::node::AppInfo; @@ -38,7 +38,13 @@ impl PlatformAdapter for RecordingAdapter { request: ActionRequest, ) -> Result { *self.request.lock().unwrap() = Some(request); - Ok(ActionResult::new("ok")) + Ok(ActionResult::new("ok") + .with_state(ElementState { + role: "textfield".into(), + states: vec!["focused".into()], + value: Some("updated".into()), + }) + .with_steps(vec![ActionStep::succeeded("AXPress")])) } } @@ -210,6 +216,10 @@ fn ref_action_trace_does_not_include_typed_text_payload() { let trace = std::fs::read_to_string(&trace_path).unwrap(); assert!(trace.contains("\"action\":\"type\"")); + assert!(trace.contains("\"event\":\"action.dispatch.start\"")); + assert!(trace.contains("\"event\":\"action.dispatch.ok\"")); + assert!(trace.contains("\"post_state\"")); + assert!(trace.contains("\"steps\"")); assert!(!trace.contains("super-secret")); let _ = std::fs::remove_file(trace_path); } diff --git a/crates/core/src/commands/is_check.rs b/crates/core/src/commands/is_check.rs index ceed412..5685bb3 100644 --- a/crates/core/src/commands/is_check.rs +++ b/crates/core/src/commands/is_check.rs @@ -23,7 +23,6 @@ pub enum IsProperty { } /// State is read live when the platform supports it, then falls back to snapshot state. -#[cfg(test)] pub fn execute(args: IsArgs, adapter: &dyn PlatformAdapter) -> Result { execute_with_context(args, adapter, &CommandContext::default()) } diff --git a/crates/core/src/commands/right_click.rs b/crates/core/src/commands/right_click.rs index ac0111d..481a646 100644 --- a/crates/core/src/commands/right_click.rs +++ b/crates/core/src/commands/right_click.rs @@ -9,7 +9,6 @@ use crate::{ }; use serde_json::{Value, json}; -#[cfg(test)] pub fn execute(args: RefArgs, adapter: &dyn PlatformAdapter) -> Result { execute_with_context(args, adapter, &CommandContext::default()) } diff --git a/crates/core/src/commands/snapshot.rs b/crates/core/src/commands/snapshot.rs index f14c11b..51a81f1 100644 --- a/crates/core/src/commands/snapshot.rs +++ b/crates/core/src/commands/snapshot.rs @@ -38,7 +38,6 @@ fn tree_options(args: &SnapshotArgs) -> crate::adapter::TreeOptions { } } -#[cfg(test)] pub fn execute(args: SnapshotArgs, adapter: &dyn PlatformAdapter) -> Result { execute_with_context(args, adapter, &CommandContext::default()) } diff --git a/crates/core/src/commands/status.rs b/crates/core/src/commands/status.rs index 40dd7e3..cf7df26 100644 --- a/crates/core/src/commands/status.rs +++ b/crates/core/src/commands/status.rs @@ -8,13 +8,11 @@ use crate::{ }; use serde_json::{Value, json}; -#[cfg(test)] pub fn execute(adapter: &dyn PlatformAdapter) -> Result { let report = adapter.permission_report(); execute_with_report(adapter, &report) } -#[cfg(test)] pub fn execute_with_report( adapter: &dyn PlatformAdapter, report: &PermissionReport, diff --git a/crates/core/src/commands/wait.rs b/crates/core/src/commands/wait.rs index dcb7339..9a92ff7 100644 --- a/crates/core/src/commands/wait.rs +++ b/crates/core/src/commands/wait.rs @@ -18,22 +18,31 @@ use crate::commands::wait_mode::validate_wait_mode; #[derive(Clone)] pub struct WaitArgs { + pub mode: WaitModeArgs, + pub predicate: WaitPredicateArgs, + pub timeout_ms: u64, + pub app: Option, +} + +#[derive(Clone)] +pub struct WaitModeArgs { pub ms: Option, pub element: Option, + pub window: Option, + pub text: Option, + pub menu: bool, + pub menu_closed: bool, + pub notification: bool, +} + +#[derive(Clone)] +pub struct WaitPredicateArgs { pub snapshot_id: Option, pub predicate: Option, pub value: Option, pub count: Option, - pub window: Option, - pub text: Option, - pub timeout_ms: u64, - pub menu: bool, - pub menu_closed: bool, - pub notification: bool, - pub app: Option, } -#[cfg(test)] pub fn execute(args: WaitArgs, adapter: &dyn PlatformAdapter) -> Result { execute_with_context(args, adapter, &CommandContext::default()) } @@ -173,13 +182,23 @@ fn wait_for_window( focused_only: false, app: None, }; + let mut last_error = None; loop { - if let Ok(windows) = adapter.list_windows(&filter) { - if let Some(win) = windows.into_iter().find(|w| w.title.contains(&title)) { - let elapsed = start.elapsed().as_millis(); - return Ok(json!({ "found": true, "window": win, "elapsed_ms": elapsed })); + match adapter.list_windows(&filter) { + Ok(windows) => { + if let Some(win) = windows.into_iter().find(|w| w.title.contains(&title)) { + let elapsed = start.elapsed().as_millis(); + return Ok(json!({ "found": true, "window": win, "elapsed_ms": elapsed })); + } } + Err(err) if is_retryable_wait_poll_error(&err.code) => { + last_error = Some(json!({ + "code": err.code.as_str(), + "message": err.message + })); + } + Err(err) => return Err(AppError::Adapter(err)), } let remaining = timeout.saturating_sub(start.elapsed()); @@ -191,7 +210,8 @@ fn wait_for_window( .with_details(json!({ "predicate": "window", "title": title, - "timeout_ms": timeout_ms + "timeout_ms": timeout_ms, + "last_error": last_error })), )); } @@ -223,28 +243,38 @@ fn wait_for_text( let opts = crate::adapter::TreeOptions::default(); let normalized_text = search_text::normalize(&text); let mut interval = Duration::from_millis(200); + let mut last_error = None; loop { - if let Ok(result) = snapshot::build(adapter, &opts, app.as_deref(), None) { - let matches = wait_text_match::find(&result.tree, &normalized_text, expected_count); - let matched = expected_count - .map(|expected| matches.len() == expected) - .unwrap_or_else(|| !matches.is_empty()); - if matched { - let snapshot_id = RefStore::for_session(context.session_id())? - .save_new_snapshot(&result.refmap)?; - let elapsed = start.elapsed().as_millis(); - let found = matches.first(); - return Ok(json!({ - "found": true, - "text": text, - "ref": found.and_then(|found| found.ref_id.clone()), - "role": found.map(|found| found.role.clone()), - "count": matches.len(), - "snapshot_id": snapshot_id, - "elapsed_ms": elapsed + match snapshot::build(adapter, &opts, app.as_deref(), None) { + Ok(result) => { + let matches = wait_text_match::find(&result.tree, &normalized_text, expected_count); + let matched = expected_count + .map(|expected| matches.len() == expected) + .unwrap_or_else(|| !matches.is_empty()); + if matched { + let snapshot_id = RefStore::for_session(context.session_id())? + .save_new_snapshot(&result.refmap)?; + let elapsed = start.elapsed().as_millis(); + let found = matches.first(); + return Ok(json!({ + "found": true, + "text": text, + "ref": found.and_then(|found| found.ref_id.clone()), + "role": found.map(|found| found.role.clone()), + "count": matches.len(), + "snapshot_id": snapshot_id, + "elapsed_ms": elapsed + })); + } + } + Err(err) if is_retryable_wait_app_error(&err) => { + last_error = Some(json!({ + "code": err.code(), + "message": err.to_string() })); } + Err(err) => return Err(err), } let remaining = timeout.saturating_sub(start.elapsed()); @@ -257,7 +287,8 @@ fn wait_for_text( "predicate": "text", "text_chars": text.chars().count(), "timeout_ms": timeout_ms, - "expected_count": expected_count + "expected_count": expected_count, + "last_error": last_error })), )); } @@ -267,6 +298,14 @@ fn wait_for_text( } } +fn is_retryable_wait_poll_error(code: &ErrorCode) -> bool { + matches!(code, ErrorCode::Timeout | ErrorCode::ElementNotFound) +} + +fn is_retryable_wait_app_error(err: &AppError) -> bool { + matches!(err.code(), "TIMEOUT" | "ELEMENT_NOT_FOUND") +} + fn wait_for_notification( app: Option, text: Option, diff --git a/crates/core/src/commands/wait_mode.rs b/crates/core/src/commands/wait_mode.rs index 41a5752..2fc182d 100644 --- a/crates/core/src/commands/wait_mode.rs +++ b/crates/core/src/commands/wait_mode.rs @@ -30,38 +30,40 @@ pub(crate) enum WaitMode { impl WaitMode { pub(crate) fn from_args(args: WaitArgs) -> Result { validate_wait_mode(&args)?; - if let Some(ms) = args.ms { + if let Some(ms) = args.mode.ms { return Ok(Self::Sleep(ms)); } - if args.menu || args.menu_closed { + if args.mode.menu || args.mode.menu_closed { return Ok(Self::Menu { app: args.app, - open: args.menu, + open: args.mode.menu, }); } - if args.notification { + if args.mode.notification { return Ok(Self::Notification { app: args.app, - text: args.text, + text: args.mode.text, }); } - if let Some(ref_id) = args.element { + if let Some(ref_id) = args.mode.element { validate_ref_id(&ref_id)?; - let predicate = - wait_predicate::ElementPredicate::parse(args.predicate.as_deref(), args.value)?; + let predicate = wait_predicate::ElementPredicate::parse( + args.predicate.predicate.as_deref(), + args.predicate.value, + )?; return Ok(Self::Element { ref_id, - snapshot_id: args.snapshot_id, + snapshot_id: args.predicate.snapshot_id, predicate, }); } - if let Some(title) = args.window { + if let Some(title) = args.mode.window { return Ok(Self::Window(title)); } - if let Some(text) = args.text { + if let Some(text) = args.mode.text { return Ok(Self::Text { text, - count: args.count, + count: args.predicate.count, app: args.app, }); } @@ -70,32 +72,32 @@ impl WaitMode { } pub(crate) fn validate_wait_mode(args: &WaitArgs) -> Result<(), AppError> { - if args.predicate.is_some() && args.element.is_none() { + if args.predicate.predicate.is_some() && args.mode.element.is_none() { return Err(AppError::invalid_input_with_suggestion( "--predicate requires --element", "Use --element with --predicate, or remove --predicate.", )); } - if args.value.is_some() && args.element.is_none() { + if args.predicate.value.is_some() && args.mode.element.is_none() { return Err(AppError::invalid_input_with_suggestion( "--value requires --element and --predicate value", "Use --element --predicate value --value .", )); } - if args.count.is_some() && (args.text.is_none() || args.notification) { + if args.predicate.count.is_some() && (args.mode.text.is_none() || args.mode.notification) { return Err(AppError::invalid_input_with_suggestion( "--count is only valid for --text waits", "Use --text --count without --notification, or remove --count.", )); } let selected = [ - args.ms.is_some(), - args.element.is_some(), - args.window.is_some(), - args.text.is_some() && !args.notification, - args.menu, - args.menu_closed, - args.notification, + args.mode.ms.is_some(), + args.mode.element.is_some(), + args.mode.window.is_some(), + args.mode.text.is_some() && !args.mode.notification, + args.mode.menu, + args.mode.menu_closed, + args.mode.notification, ] .into_iter() .filter(|selected| *selected) diff --git a/crates/core/src/commands/wait_tests.rs b/crates/core/src/commands/wait_tests.rs index 3f745b9..3b035d2 100644 --- a/crates/core/src/commands/wait_tests.rs +++ b/crates/core/src/commands/wait_tests.rs @@ -1,7 +1,8 @@ use super::*; use crate::{ - adapter::PlatformAdapter, + adapter::{PlatformAdapter, WindowFilter}, error::{AdapterError, ErrorCode}, + node::WindowInfo, notification::{NotificationFilter, NotificationInfo}, }; @@ -23,23 +24,45 @@ impl PlatformAdapter for NotificationErrorAdapter { } } -#[test] -fn notification_wait_propagates_adapter_error() { - let err = execute( - WaitArgs { +struct WindowErrorAdapter; + +impl PlatformAdapter for WindowErrorAdapter { + fn list_windows(&self, _filter: &WindowFilter) -> Result, AdapterError> { + Err(AdapterError::permission_denied()) + } +} + +fn wait_args() -> WaitArgs { + WaitArgs { + mode: WaitModeArgs { ms: None, element: None, + window: None, + text: None, + menu: false, + menu_closed: false, + notification: false, + }, + predicate: WaitPredicateArgs { snapshot_id: None, predicate: None, value: None, count: None, - window: None, - text: None, - timeout_ms: 1, - menu: false, - menu_closed: false, - notification: true, - app: None, + }, + timeout_ms: 1, + app: None, + } +} + +#[test] +fn notification_wait_propagates_adapter_error() { + let err = execute( + WaitArgs { + mode: WaitModeArgs { + notification: true, + ..wait_args().mode + }, + ..wait_args() }, &NotificationErrorAdapter, ) @@ -52,19 +75,12 @@ fn notification_wait_propagates_adapter_error() { fn rejects_multiple_wait_modes() { let err = execute( WaitArgs { - ms: Some(1), - element: Some("@e1".into()), - snapshot_id: None, - predicate: None, - value: None, - count: None, - window: None, - text: None, - timeout_ms: 1, - menu: false, - menu_closed: false, - notification: false, - app: None, + mode: WaitModeArgs { + ms: Some(1), + element: Some("@e1".into()), + ..wait_args().mode + }, + ..wait_args() }, &NoopAdapter, ) @@ -74,22 +90,49 @@ fn rejects_multiple_wait_modes() { assert!(err.suggestion().is_some()); } +#[test] +fn window_wait_propagates_permanent_adapter_error() { + let err = execute( + WaitArgs { + mode: WaitModeArgs { + window: Some("Document".into()), + ..wait_args().mode + }, + ..wait_args() + }, + &WindowErrorAdapter, + ) + .unwrap_err(); + + assert_eq!(err.code(), "PERM_DENIED"); +} + +#[test] +fn text_wait_propagates_permanent_snapshot_error() { + let err = execute( + WaitArgs { + mode: WaitModeArgs { + text: Some("hello".into()), + ..wait_args().mode + }, + ..wait_args() + }, + &WindowErrorAdapter, + ) + .unwrap_err(); + + assert_eq!(err.code(), "PERM_DENIED"); +} + #[test] fn notification_wait_allows_text_filter() { let result = validate_wait_mode(&WaitArgs { - ms: None, - element: None, - snapshot_id: None, - predicate: None, - value: None, - count: None, - window: None, - text: Some("done".into()), - timeout_ms: 1, - menu: false, - menu_closed: false, - notification: true, - app: None, + mode: WaitModeArgs { + text: Some("done".into()), + notification: true, + ..wait_args().mode + }, + ..wait_args() }); assert!(result.is_ok()); @@ -98,19 +141,11 @@ fn notification_wait_allows_text_filter() { #[test] fn predicate_requires_element_mode() { let err = validate_wait_mode(&WaitArgs { - ms: None, - element: None, - snapshot_id: None, - predicate: Some("enabled".into()), - value: None, - count: None, - window: None, - text: None, - timeout_ms: 1, - menu: false, - menu_closed: false, - notification: false, - app: None, + predicate: WaitPredicateArgs { + predicate: Some("enabled".into()), + ..wait_args().predicate + }, + ..wait_args() }) .unwrap_err(); diff --git a/crates/core/src/output.rs b/crates/core/src/output.rs index 79218aa..c25a61c 100644 --- a/crates/core/src/output.rs +++ b/crates/core/src/output.rs @@ -5,9 +5,7 @@ use crate::error::AppError; pub const ENVELOPE_VERSION: &str = "2.0"; -/// Structured output envelope used by the Phase 3 MCP server transport layer. -/// CLI commands currently build responses via inline `serde_json::json!` calls in `main.rs`; -/// this type provides the typed equivalent for programmatic consumers. +/// Structured output envelope used by the CLI and future programmatic transports. #[derive(Debug, Serialize)] pub struct Response { pub version: &'static str, diff --git a/crates/core/src/ref_action.rs b/crates/core/src/ref_action.rs index 69f5eec..b12a6b4 100644 --- a/crates/core/src/ref_action.rs +++ b/crates/core/src/ref_action.rs @@ -1,27 +1,18 @@ use crate::{ action::{ActionRequest, ActionResult}, - actionability::{self, ActionabilityReport}, - adapter::{NativeHandle, PlatformAdapter}, + actionability, + adapter::PlatformAdapter, error::AdapterError, refs::RefEntry, }; -pub(crate) fn check_resolved( - adapter: &dyn PlatformAdapter, - entry: &RefEntry, - handle: &NativeHandle, - request: &ActionRequest, -) -> Result { - actionability::check_live(entry, handle, adapter, request) -} - pub fn execute_entry( adapter: &dyn PlatformAdapter, entry: &RefEntry, request: ActionRequest, ) -> Result { let handle = adapter.resolve_element_strict(entry)?; - let result = check_resolved(adapter, entry, &handle, &request) + let result = actionability::check_live(entry, &handle, adapter, &request) .and_then(|_| adapter.execute_action(&handle, request)); let release = adapter.release_handle(&handle); match (result, release) { diff --git a/crates/core/src/ref_alloc.rs b/crates/core/src/ref_alloc.rs index 84f2f29..5cb40f9 100644 --- a/crates/core/src/ref_alloc.rs +++ b/crates/core/src/ref_alloc.rs @@ -206,7 +206,6 @@ fn allocate_refs_at_path( fn strip_ref_bounds_when_hidden(entry: &mut RefEntry, include_bounds: bool) { if !include_bounds { entry.bounds = None; - entry.bounds_hash = None; } } diff --git a/crates/core/src/ref_alloc_tests.rs b/crates/core/src/ref_alloc_tests.rs index c1e7b5a..e3ccb69 100644 --- a/crates/core/src/ref_alloc_tests.rs +++ b/crates/core/src/ref_alloc_tests.rs @@ -148,7 +148,7 @@ fn allocate_refs_records_structural_paths() { } #[test] -fn allocate_refs_hides_bounds_from_refmap_when_snapshot_hides_bounds() { +fn allocate_refs_keeps_bounds_hash_when_snapshot_hides_bounds() { let mut root = node("window", Some("w")); root.children = vec![node("button", Some("Open"))]; let mut refmap = RefMap::new(); @@ -171,12 +171,22 @@ fn allocate_refs_hides_bounds_from_refmap_when_snapshot_hides_bounds() { assert!(out.children[0].bounds.is_none()); assert!(entry.bounds.is_none()); - assert!(entry.bounds_hash.is_none()); + assert_eq!(entry.bounds_hash, Some(entry_hash())); assert_eq!(entry.path.as_slice(), [0]); assert_eq!(entry.source_window_id.as_deref(), Some("w-42")); assert_eq!(entry.source_window_title.as_deref(), Some("Documents")); } +fn entry_hash() -> u64 { + Rect { + x: 0.0, + y: 0.0, + width: 10.0, + height: 10.0, + } + .bounds_hash() +} + #[test] fn allocate_refs_keeps_bounds_in_refmap_when_snapshot_includes_bounds() { let mut root = node("window", Some("w")); diff --git a/crates/core/src/snapshot.rs b/crates/core/src/snapshot.rs index e101994..4efb547 100644 --- a/crates/core/src/snapshot.rs +++ b/crates/core/src/snapshot.rs @@ -76,7 +76,7 @@ pub fn build( })? }; - let raw_tree = adapter.get_tree(&window, opts)?; + let raw_tree = adapter.get_tree(&window, &opts.with_ref_identity_bounds())?; let mut refmap = RefMap::new(); let config = RefAllocConfig { @@ -172,7 +172,7 @@ pub fn append_surface_refs_with_context( interactive_only: true, ..Default::default() }; - let raw_tree = adapter.get_tree(&window, &opts)?; + let raw_tree = adapter.get_tree(&window, &opts.with_ref_identity_bounds())?; let store = RefStore::for_session(context.session_id())?; let mut refmap = store.load_latest()?; let config = RefAllocConfig { diff --git a/crates/core/src/snapshot_ref.rs b/crates/core/src/snapshot_ref.rs index 40b5d5a..71a0a4e 100644 --- a/crates/core/src/snapshot_ref.rs +++ b/crates/core/src/snapshot_ref.rs @@ -45,7 +45,7 @@ pub fn run_from_ref_with_context( let handle = ResolvedElement::new(adapter, adapter.resolve_element_strict(&entry)?); - let raw_tree = adapter.get_subtree(handle.handle(), opts)?; + let raw_tree = adapter.get_subtree(handle.handle(), &opts.with_ref_identity_bounds())?; refmap.remove_by_root_ref(root_ref_id); diff --git a/crates/core/src/trace.rs b/crates/core/src/trace.rs index 5300611..5ab2b85 100644 --- a/crates/core/src/trace.rs +++ b/crates/core/src/trace.rs @@ -71,6 +71,7 @@ fn open_trace_file(path: &Path) -> Result { { use std::os::unix::fs::OpenOptionsExt; options.mode(0o600); + options.custom_flags(libc::O_NOFOLLOW); } let file = options.open(path).map_err(AppError::from)?; reject_loose_trace_permissions(&file)?; @@ -153,3 +154,30 @@ fn redacted_value(value: Value) -> Value { _ => json!({ "redacted": true }), } } + +#[cfg(test)] +mod tests { + use super::*; + + #[cfg(unix)] + #[test] + fn trace_open_rejects_symlink_paths() { + let base = std::env::temp_dir().join(format!( + "agent-desktop-trace-symlink-{}", + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_nanos() + )); + let target = base.with_extension("target"); + let link = base.with_extension("link"); + std::fs::write(&target, b"existing").unwrap(); + std::os::unix::fs::symlink(&target, &link).unwrap(); + + let result = open_trace_file(&link); + + assert!(result.is_err()); + let _ = std::fs::remove_file(&link); + let _ = std::fs::remove_file(&target); + } +} diff --git a/crates/ffi/include/agent_desktop.h b/crates/ffi/include/agent_desktop.h index 76f3167..1070e4c 100644 --- a/crates/ffi/include/agent_desktop.h +++ b/crates/ffi/include/agent_desktop.h @@ -393,6 +393,10 @@ typedef struct AdWindowOp { } AdWindowOp; /** + * Low-level native-handle action. This does not perform strict ref + * re-identification or actionability preflight; callers that want CLI parity + * should use `ad_execute_ref_action_with_policy`. + * * # Safety * * `adapter` must be a non-null pointer returned by `ad_adapter_create`. @@ -406,6 +410,10 @@ AdResult ad_execute_action(const struct AdAdapter *adapter, struct AdActionResult *out); /** + * Low-level native-handle action with explicit interaction policy. This does + * not perform strict ref re-identification or actionability preflight; callers + * that want CLI parity should use `ad_execute_ref_action_with_policy`. + * * # Safety * * `adapter` must be a non-null pointer returned by `ad_adapter_create`. @@ -420,6 +428,9 @@ AdResult ad_execute_action_with_policy(const struct AdAdapter *adapter, struct AdActionResult *out); /** + * Strict ref action path matching CLI semantics: resolve the full ref identity, + * run actionability preflight, then dispatch using the requested policy. + * * # Safety * * `adapter` must be a non-null pointer returned by `ad_adapter_create`. diff --git a/crates/ffi/src/actions/result.rs b/crates/ffi/src/actions/result.rs index a0e5e8b..9baeb4b 100644 --- a/crates/ffi/src/actions/result.rs +++ b/crates/ffi/src/actions/result.rs @@ -90,6 +90,7 @@ mod tests { states: vec!["focused".to_owned(), "enabled".to_owned()], value: Some("OK".to_owned()), }), + steps: Vec::new(), }; let c_result = action_result_to_c(&core_result); unsafe { diff --git a/crates/macos/src/actions/chain.rs b/crates/macos/src/actions/chain.rs index 13bb467..eb2259e 100644 --- a/crates/macos/src/actions/chain.rs +++ b/crates/macos/src/actions/chain.rs @@ -1,4 +1,4 @@ -use agent_desktop_core::action::InteractionPolicy; +use agent_desktop_core::action::{ActionStep, InteractionPolicy}; use agent_desktop_core::error::{AdapterError, ErrorCode}; use crate::actions::discovery::ElementCaps; @@ -23,11 +23,12 @@ mod imp { def: &ChainDef, ctx: &ChainContext, policy: InteractionPolicy, - ) -> Result<(), AdapterError> { + ) -> Result, AdapterError> { let deadline = ctx .deadline .unwrap_or_else(|| Instant::now() + chain_timeout()); let total = def.steps.len(); + let mut steps = Vec::new(); if let Some(pid) = crate::system::app_ops::pid_from_element(el) { ax_helpers::set_messaging_timeout(&crate::tree::element_for_pid(pid), 1.0); @@ -37,6 +38,7 @@ mod imp { if def.pre_scroll { tracing::debug!("chain: pre-scroll AXScrollToVisible"); ax_helpers::ensure_visible(el); + steps.push(ActionStep::attempted("AXScrollToVisible")); } for (i, step) in def.steps.iter().enumerate() { @@ -47,10 +49,12 @@ mod imp { .iter() .find(|s| matches!(s, ChainStep::CGClick { .. })) { + let label = step_label(cg); if physical_click_permitted(policy) && execute_step(el, caps, cg, ctx, policy)? { tracing::debug!("chain: CGClick fallback succeeded"); - return Ok(()); + steps.push(ActionStep::succeeded(label)); + return Ok(steps); } } return Err( @@ -67,9 +71,11 @@ mod imp { let label = step_label(step); if execute_step(el, caps, step, ctx, policy)? { tracing::debug!("chain: [{}/{}] {} -> success", i + 1, total, label); - return Ok(()); + steps.push(ActionStep::succeeded(label)); + return Ok(steps); } tracing::debug!("chain: [{}/{}] {} -> skip", i + 1, total, label); + steps.push(ActionStep::skipped(label)); } tracing::debug!("chain: all {total} steps exhausted"); diff --git a/crates/macos/src/actions/dispatch.rs b/crates/macos/src/actions/dispatch.rs index 311a59f..48eda05 100644 --- a/crates/macos/src/actions/dispatch.rs +++ b/crates/macos/src/actions/dispatch.rs @@ -64,6 +64,7 @@ mod imp { ) -> Result { let action = &request.action; let label = action_label(action); + let mut steps = Vec::new(); tracing::debug!("action: perform {label}"); match action { Action::Click => { @@ -72,7 +73,13 @@ mod imp { dynamic_value: None, deadline: None, }; - execute_chain(el, &caps, &chain_defs::CLICK_CHAIN, &ctx, request.policy)?; + steps.extend(execute_chain( + el, + &caps, + &chain_defs::CLICK_CHAIN, + &ctx, + request.policy, + )?); } Action::DoubleClick => { @@ -86,13 +93,13 @@ mod imp { dynamic_value: None, deadline: None, }; - execute_chain( + steps.extend(execute_chain( el, &caps, &chain_defs::RIGHT_CLICK_CHAIN, &ctx, request.policy, - )?; + )?); } Action::Toggle => { @@ -105,13 +112,13 @@ mod imp { dynamic_value: Some(val), deadline: None, }; - execute_chain( + steps.extend(execute_chain( el, &caps, &chain_defs::SET_VALUE_CHAIN, &ctx, request.policy, - )?; + )?); } Action::SetFocus => { @@ -120,7 +127,13 @@ mod imp { dynamic_value: None, deadline: None, }; - execute_chain(el, &caps, &chain_defs::FOCUS_CHAIN, &ctx, request.policy)?; + steps.extend(execute_chain( + el, + &caps, + &chain_defs::FOCUS_CHAIN, + &ctx, + request.policy, + )?); } Action::TypeText(text) => { @@ -137,7 +150,13 @@ mod imp { dynamic_value: None, deadline: None, }; - execute_chain(el, &caps, &chain_defs::EXPAND_CHAIN, &ctx, request.policy)?; + steps.extend(execute_chain( + el, + &caps, + &chain_defs::EXPAND_CHAIN, + &ctx, + request.policy, + )?); } Action::Collapse => { @@ -146,7 +165,13 @@ mod imp { dynamic_value: None, deadline: None, }; - execute_chain(el, &caps, &chain_defs::COLLAPSE_CHAIN, &ctx, request.policy)?; + steps.extend(execute_chain( + el, + &caps, + &chain_defs::COLLAPSE_CHAIN, + &ctx, + request.policy, + )?); } Action::Select(value) => { @@ -176,13 +201,13 @@ mod imp { dynamic_value: None, deadline: None, }; - execute_chain( + steps.extend(execute_chain( el, &caps, &chain_defs::SCROLL_TO_CHAIN, &ctx, request.policy, - )?; + )?); } Action::Clear => { @@ -191,7 +216,13 @@ mod imp { dynamic_value: Some(""), deadline: None, }; - execute_chain(el, &caps, &chain_defs::CLEAR_CHAIN, &ctx, request.policy)?; + steps.extend(execute_chain( + el, + &caps, + &chain_defs::CLEAR_CHAIN, + &ctx, + request.policy, + )?); } Action::KeyDown(_) | Action::KeyUp(_) | Action::Hover | Action::Drag(_) => { @@ -210,7 +241,7 @@ mod imp { } } - let mut result = ActionResult::new(label); + let mut result = ActionResult::new(label).with_steps(steps); if let Some(state) = crate::actions::post_state::read_post_state(el, action) { verify_post_state(action, &state)?; result = result.with_state(state); diff --git a/crates/macos/src/actions/post_state.rs b/crates/macos/src/actions/post_state.rs index 66fc5d4..5cb9c46 100644 --- a/crates/macos/src/actions/post_state.rs +++ b/crates/macos/src/actions/post_state.rs @@ -24,13 +24,13 @@ pub(crate) fn read_post_state( pub(crate) fn read_element_state(el: &crate::tree::AXElement) -> ElementState { let attrs = crate::tree::element::fetch_node_attrs(el); let role = normalized_role(attrs.role.as_deref()); - element_state_from_attrs(el, attrs, role) + element_state_from_attrs(attrs, role) } pub(crate) fn read_live_element(el: &crate::tree::AXElement) -> LiveElement { let attrs = crate::tree::element::fetch_node_attrs(el); let role = normalized_role(attrs.role.as_deref()); - let state = element_state_from_attrs(el, attrs, role.clone()); + let state = element_state_from_attrs(attrs, role.clone()); LiveElement { state: Some(state), bounds: crate::tree::read_bounds(el), @@ -40,16 +40,10 @@ pub(crate) fn read_live_element(el: &crate::tree::AXElement) -> LiveElement { } } -fn element_state_from_attrs( - el: &crate::tree::AXElement, - attrs: crate::tree::NodeAttrs, - role: String, -) -> ElementState { +fn element_state_from_attrs(attrs: crate::tree::NodeAttrs, role: String) -> ElementState { let value = attrs.value; - let focused = crate::tree::element::copy_bool_attr(el, "AXFocused").unwrap_or(false); - let expanded = crate::tree::element::copy_bool_attr(el, "AXExpanded") - .or_else(|| crate::tree::element::copy_bool_attr(el, "AXDisclosing")) - .unwrap_or(false); + let focused = attrs.focused.unwrap_or(false); + let expanded = attrs.expanded.or(attrs.disclosing).unwrap_or(false); let mut states = Vec::new(); if focused { states.push("focused".into()); diff --git a/crates/macos/src/tree/builder.rs b/crates/macos/src/tree/builder.rs index 26bb5a0..5442826 100644 --- a/crates/macos/src/tree/builder.rs +++ b/crates/macos/src/tree/builder.rs @@ -168,7 +168,11 @@ pub fn build_subtree( if is_secure_text { states.push("secure".into()); } - if element_is_expanded(el) { + if attrs + .expanded + .or(attrs.disclosing) + .unwrap_or_else(|| element_is_expanded(el)) + { states.push("expanded".into()); } if super::roles::is_toggleable_role(&role) && value_is_checked(value.as_deref()) { diff --git a/crates/macos/src/tree/element.rs b/crates/macos/src/tree/element.rs index 8372006..112bd69 100644 --- a/crates/macos/src/tree/element.rs +++ b/crates/macos/src/tree/element.rs @@ -15,7 +15,12 @@ mod imp { use super::*; use crate::{ cf_type::created_cf_array, - tree::{NodeAttrs, ax_element::AXElement, ax_value, node_attrs::parse_enabled}, + tree::{ + NodeAttrs, + ax_element::AXElement, + ax_value, + node_attrs::{parse_bool_attr, parse_enabled}, + }, }; use accessibility_sys::{ AXUIElementCopyAttributeValue, AXUIElementCopyAttributeValues, @@ -47,6 +52,9 @@ mod imp { kAXDescriptionAttribute, kAXValueAttribute, kAXEnabledAttribute, + "AXFocused", + "AXExpanded", + "AXDisclosing", ]; let cf_names: Vec = attr_names.iter().map(|a| CFString::new(a)).collect(); let cf_refs: Vec<_> = cf_names.iter().map(|s| s.as_concrete_TypeRef()).collect(); @@ -91,7 +99,7 @@ mod imp { } None } - 4 => item + 4..=7 => item .downcast::() .map(|b| bool::from(b).to_string()), _ => None, @@ -106,6 +114,9 @@ mod imp { description: get(2), value: get(3), enabled: parse_enabled(get(4)), + focused: parse_bool_attr(get(5)), + expanded: parse_bool_attr(get(6)), + disclosing: parse_bool_attr(get(7)), } } @@ -121,6 +132,9 @@ mod imp { description: desc, value: val, enabled, + focused: copy_bool_attr(el, "AXFocused"), + expanded: copy_bool_attr(el, "AXExpanded"), + disclosing: copy_bool_attr(el, "AXDisclosing"), } } diff --git a/crates/macos/src/tree/element_dedupe.rs b/crates/macos/src/tree/element_dedupe.rs index 16b1cf4..85aa486 100644 --- a/crates/macos/src/tree/element_dedupe.rs +++ b/crates/macos/src/tree/element_dedupe.rs @@ -1,22 +1,13 @@ -use rustc_hash::FxHashSet; - use super::{AXElement, same_element}; #[derive(Default)] -pub(crate) struct ElementDedupe { - pointer_keys: FxHashSet, -} +pub(crate) struct ElementDedupe; impl ElementDedupe { pub(crate) fn push(&mut self, elements: &mut Vec, element: AXElement) -> bool { - let pointer_key = element.0 as usize; - if !self.pointer_keys.insert(pointer_key) { - return false; - } - if pointer_key != 0 - && elements - .iter() - .any(|existing| equivalent(existing, &element)) + if elements + .iter() + .any(|existing| equivalent(existing, &element)) { return false; } @@ -42,13 +33,13 @@ mod tests { use super::*; #[test] - fn pointer_duplicates_are_collapsed_without_semantic_lookup() { - let mut dedupe = ElementDedupe::default(); + fn null_elements_do_not_collapse_without_semantic_identity() { + let mut dedupe = ElementDedupe; let mut elements = Vec::new(); assert!(dedupe.push(&mut elements, null_element())); - assert!(!dedupe.push(&mut elements, null_element())); - assert_eq!(elements.len(), 1); + assert!(dedupe.push(&mut elements, null_element())); + assert_eq!(elements.len(), 2); } #[cfg(target_os = "macos")] diff --git a/crates/macos/src/tree/node_attrs.rs b/crates/macos/src/tree/node_attrs.rs index d946f3b..575dcb8 100644 --- a/crates/macos/src/tree/node_attrs.rs +++ b/crates/macos/src/tree/node_attrs.rs @@ -5,12 +5,19 @@ pub(crate) struct NodeAttrs { pub(crate) description: Option, pub(crate) value: Option, pub(crate) enabled: bool, + pub(crate) focused: Option, + pub(crate) expanded: Option, + pub(crate) disclosing: Option, } pub(crate) fn parse_enabled(enabled: Option) -> bool { enabled.map(|s| s == "true").unwrap_or(true) } +pub(crate) fn parse_bool_attr(value: Option) -> Option { + value.map(|s| s == "true") +} + #[cfg(test)] mod tests { use super::*; diff --git a/crates/macos/src/tree/resolve.rs b/crates/macos/src/tree/resolve.rs index eeebb00..bf61cf5 100644 --- a/crates/macos/src/tree/resolve.rs +++ b/crates/macos/src/tree/resolve.rs @@ -127,7 +127,7 @@ fn find_entry_by_path(roots: &[AXElement], entry: &RefEntry) -> Result 1 { break; @@ -162,7 +162,7 @@ fn find_entry_in_roots( deadline: std::time::Instant, ) -> Result { let mut matches = Vec::new(); - let mut seen_matches = ElementDedupe::default(); + let mut seen_matches = ElementDedupe; for root in roots { if matches.len() > 1 { break; @@ -208,11 +208,38 @@ fn classify_candidates( "description": entry.description, "source_app": entry.source_app, "source_window_id": entry.source_window_id, - "source_window_title": entry.source_window_title + "source_window_title": entry.source_window_title, + "candidates": candidate_summaries(&matches) }))), } } +#[cfg(target_os = "macos")] +fn candidate_summaries(matches: &[AXElement]) -> Vec { + matches + .iter() + .take(10) + .enumerate() + .map(|(index, element)| { + let ax_role = copy_string_attr(element, accessibility_sys::kAXRoleAttribute); + let role = crate::tree::roles::normalized_role_for_element(element, ax_role.as_deref()); + let name = crate::tree::roles::normalized_role_and_label(element, ax_role.as_deref()) + .1 + .or_else(|| resolve_element_name(element)); + let description = copy_string_attr(element, accessibility_sys::kAXDescriptionAttribute); + let bounds = crate::tree::read_bounds(element); + serde_json::json!({ + "index": index, + "role": role, + "name": name, + "description": description, + "bounds": bounds, + "bounds_hash": bounds.as_ref().map(|bounds| bounds.bounds_hash()) + }) + }) + .collect() +} + #[cfg(target_os = "macos")] struct CollectContext<'a> { entry: &'a RefEntry, diff --git a/crates/macos/src/tree/resolve_roots.rs b/crates/macos/src/tree/resolve_roots.rs index e808734..cea514c 100644 --- a/crates/macos/src/tree/resolve_roots.rs +++ b/crates/macos/src/tree/resolve_roots.rs @@ -19,7 +19,7 @@ pub(super) fn path_candidate_roots(entry: &RefEntry) -> Vec { pub(super) fn candidate_roots(entry: &RefEntry) -> Vec { let root = element_for_pid(entry.pid); let mut roots = Vec::new(); - let mut dedupe = ElementDedupe::default(); + let mut dedupe = ElementDedupe; if let Some(source_window_title) = entry.source_window_title.as_deref() { dedupe.push( &mut roots, diff --git a/src/Cargo.toml b/src/Cargo.toml index e561265..6487fe5 100644 --- a/src/Cargo.toml +++ b/src/Cargo.toml @@ -30,5 +30,9 @@ path = "main.rs" name = "snapshot_test" path = "tests/snapshot_test.rs" +[[test]] +name = "conformance" +path = "tests/conformance.rs" + [lints] workspace = true diff --git a/src/batch/tests.rs b/src/batch/tests.rs index f0868b9..348bc85 100644 --- a/src/batch/tests.rs +++ b/src/batch/tests.rs @@ -63,6 +63,17 @@ fn rejects_unknown_batch_args() { assert_eq!(err.code(), "INVALID_ARGS"); } +#[test] +fn rejects_unknown_wait_batch_args_after_flattening() { + let err = parse_command(item( + "wait", + serde_json::json!({ "ms": 1, "unexpected": true }), + )) + .expect_err("unknown wait field is rejected"); + + assert_eq!(err.code(), "INVALID_ARGS"); +} + #[test] fn stop_on_error_halts_after_first_failure() { let args = BatchArgs { diff --git a/src/cli/help_after.txt b/src/cli/help_after.txt index ac7640e..af131a4 100644 --- a/src/cli/help_after.txt +++ b/src/cli/help_after.txt @@ -92,8 +92,8 @@ REF IDs Ref actions use strict resolution: stale targets return STALE_REF; duplicate plausible targets return AMBIGUOUS_TARGET instead of choosing arbitrarily. Ref actions run actionability checks before dispatch. Use --trace to - write JSONL diagnostics outside stdout; add --trace-strict to fail on trace - write errors. + write JSONL diagnostics outside stdout; --trace-strict fails on trace setup + and pre-action write errors. KEY COMBOS Single keys: return, escape, tab, space, delete, up, down, left, right diff --git a/src/cli/mod.rs b/src/cli/mod.rs index d7effef..98f67d2 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -56,7 +56,7 @@ pub(crate) struct Cli { #[arg( long, global = true, - help = "Fail the command if writing --trace fails" + help = "Fail on trace setup/pre-action write errors" )] pub trace_strict: bool, diff --git a/src/cli_args/system.rs b/src/cli_args/system.rs index 4108cc3..33c57fb 100644 --- a/src/cli_args/system.rs +++ b/src/cli_args/system.rs @@ -1,4 +1,4 @@ -use clap::Parser; +use clap::{Args, Parser}; use serde::Deserialize; fn default_launch_timeout() -> u64 { @@ -97,10 +97,51 @@ pub(crate) struct ClipboardSetArgs { #[derive(Parser, Debug, Deserialize)] #[serde(deny_unknown_fields)] pub(crate) struct WaitArgs { + #[command(flatten)] + #[serde(flatten)] + pub mode: WaitModeArgs, + #[command(flatten)] + #[serde(flatten)] + pub predicate: WaitPredicateArgs, + #[arg( + long, + default_value = "30000", + help = "Timeout in milliseconds for element/window/text waits" + )] + #[serde(default = "default_wait_timeout")] + pub timeout: u64, + #[arg(long, help = "Scope element, window, or text wait to this application")] + pub app: Option, +} + +#[derive(Args, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub(crate) struct WaitModeArgs { #[arg(value_name = "MS", help = "Milliseconds to pause")] pub ms: Option, #[arg(long, help = "Block until this element ref appears in the tree")] pub element: Option, + #[arg(long, help = "Block until a window with this title appears")] + pub window: Option, + #[arg( + long, + help = "Block until text appears in the app's accessibility tree; with --notification, filter notification text" + )] + pub text: Option, + #[arg(long, help = "Block until a menu surface is open")] + #[serde(default)] + pub menu: bool, + #[arg(long, help = "Block until the menu surface is dismissed")] + #[serde(default)] + pub menu_closed: bool, + #[arg(long, help = "Block until a new notification arrives")] + #[serde(default)] + pub notification: bool, +} + +#[derive(Args, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub(crate) struct WaitPredicateArgs { #[arg( long, value_name = "SNAPSHOT_ID", @@ -125,31 +166,6 @@ pub(crate) struct WaitArgs { help = "Expected match count for --text waits" )] pub count: Option, - #[arg(long, help = "Block until a window with this title appears")] - pub window: Option, - #[arg( - long, - help = "Block until text appears in the app's accessibility tree; with --notification, filter notification text" - )] - pub text: Option, - #[arg( - long, - default_value = "30000", - help = "Timeout in milliseconds for element/window/text waits" - )] - #[serde(default = "default_wait_timeout")] - pub timeout: u64, - #[arg(long, help = "Block until a menu surface is open")] - #[serde(default)] - pub menu: bool, - #[arg(long, help = "Block until the menu surface is dismissed")] - #[serde(default)] - pub menu_closed: bool, - #[arg(long, help = "Block until a new notification arrives")] - #[serde(default)] - pub notification: bool, - #[arg(long, help = "Scope element, window, or text wait to this application")] - pub app: Option, } #[derive(Parser, Debug, Deserialize)] diff --git a/src/command_policy/mod.rs b/src/command_policy/mod.rs index 7e34f1f..1136fec 100644 --- a/src/command_policy/mod.rs +++ b/src/command_policy/mod.rs @@ -157,7 +157,7 @@ fn validate_args(cmd: &Commands) -> Result<(), AppError> { } } Commands::Wait(args) => { - if let Some(ref_id) = &args.element { + if let Some(ref_id) = &args.mode.element { validate_ref_id(ref_id)?; } } diff --git a/src/dispatch/mod.rs b/src/dispatch/mod.rs index 79922f6..2800677 100644 --- a/src/dispatch/mod.rs +++ b/src/dispatch/mod.rs @@ -304,18 +304,22 @@ pub(crate) fn dispatch( Commands::Wait(a) => wait::execute_with_context( wait::WaitArgs { - ms: a.ms, - element: a.element, - snapshot_id: a.snapshot, - predicate: a.predicate, - value: a.value, - count: a.count, - window: a.window, - text: a.text, + mode: wait::WaitModeArgs { + ms: a.mode.ms, + element: a.mode.element, + window: a.mode.window, + text: a.mode.text, + menu: a.mode.menu, + menu_closed: a.mode.menu_closed, + notification: a.mode.notification, + }, + predicate: wait::WaitPredicateArgs { + snapshot_id: a.predicate.snapshot, + predicate: a.predicate.predicate, + value: a.predicate.value, + count: a.predicate.count, + }, timeout_ms: a.timeout, - menu: a.menu, - menu_closed: a.menu_closed, - notification: a.notification, app: a.app, }, adapter, diff --git a/src/dispatch/notifications.rs b/src/dispatch/notifications.rs index 781c97e..0fe4253 100644 --- a/src/dispatch/notifications.rs +++ b/src/dispatch/notifications.rs @@ -3,7 +3,7 @@ use agent_desktop_core::{ commands::{ dismiss_all_notifications, dismiss_notification, list_notifications, notification_action, }, - error::AppError, + error::{AppError, ErrorCode}, }; use serde_json::Value; @@ -42,6 +42,11 @@ pub(crate) fn dispatch_notification( }, adapter, ), - _ => unreachable!(), + _ => Err(AppError::Adapter( + agent_desktop_core::error::AdapterError::new( + ErrorCode::InvalidArgs, + "dispatch_notification received a non-notification command", + ), + )), } } diff --git a/src/tests/conformance.rs b/src/tests/conformance.rs new file mode 100644 index 0000000..7822df1 --- /dev/null +++ b/src/tests/conformance.rs @@ -0,0 +1,114 @@ +use agent_desktop_core::{ + action::{Action, ActionRequest, ActionResult, ElementState}, + adapter::{LiveElement, NativeHandle, PlatformAdapter, SnapshotSurface}, + error::{AdapterError, ErrorCode}, + node::Rect, + refs::RefEntry, +}; +use std::sync::atomic::{AtomicU32, Ordering}; + +struct ContractAdapter { + live_bounds: Option, + dispatches: AtomicU32, +} + +impl PlatformAdapter for ContractAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn get_live_element(&self, _handle: &NativeHandle) -> Result { + Ok(LiveElement { + state: Some(ElementState { + role: "button".into(), + states: vec![], + value: None, + }), + bounds: self.live_bounds, + available_actions: Some(vec!["Click".into()]), + }) + } + + fn execute_action( + &self, + _handle: &NativeHandle, + _request: ActionRequest, + ) -> Result { + self.dispatches.fetch_add(1, Ordering::SeqCst); + Ok(ActionResult::new("click")) + } +} + +fn entry(bounds: Rect) -> RefEntry { + RefEntry { + pid: 1, + role: "button".into(), + name: Some("OK".into()), + value: None, + description: None, + states: vec![], + bounds: Some(bounds), + bounds_hash: Some(bounds.bounds_hash()), + 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: true, + path: Default::default(), + } +} + +#[test] +fn adapter_contract_blocks_stale_live_bounds_before_dispatch() { + let snapshot_bounds = Rect { + x: 1.0, + y: 1.0, + width: 20.0, + height: 20.0, + }; + let adapter = ContractAdapter { + live_bounds: Some(Rect { + x: 100.0, + y: 100.0, + width: 20.0, + height: 20.0, + }), + dispatches: AtomicU32::new(0), + }; + + let err = agent_desktop_core::ref_action::execute_entry( + &adapter, + &entry(snapshot_bounds), + ActionRequest::headless(Action::Click), + ) + .unwrap_err(); + + assert_eq!(err.code, ErrorCode::StaleRef); + assert_eq!(adapter.dispatches.load(Ordering::SeqCst), 0); +} + +#[test] +fn adapter_contract_dispatches_when_live_identity_is_stable() { + let bounds = Rect { + x: 1.0, + y: 1.0, + width: 20.0, + height: 20.0, + }; + let adapter = ContractAdapter { + live_bounds: Some(bounds), + dispatches: AtomicU32::new(0), + }; + + let result = agent_desktop_core::ref_action::execute_entry( + &adapter, + &entry(bounds), + ActionRequest::headless(Action::Click), + ) + .unwrap(); + + assert_eq!(result.action, "click"); + assert_eq!(adapter.dispatches.load(Ordering::SeqCst), 1); +} diff --git a/tests/conformance/README.md b/tests/conformance/README.md index 4fd1022..380b312 100644 --- a/tests/conformance/README.md +++ b/tests/conformance/README.md @@ -5,6 +5,10 @@ adapter is the first implementation, but the tests are written against `PlatformAdapter` semantics so Windows UIA and Linux AT-SPI can reuse the same expectations. +The executable smoke harness lives in `src/tests/conformance.rs`. It uses the +public `PlatformAdapter` contract to prove stale live identity blocks dispatch +and stable live identity permits dispatch. + ## Required Gates | Area | Required behavior |