From 0bc04b9b82b44ebec971971d3d887008b7b74435 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Wed, 3 Jun 2026 18:25:11 -0700 Subject: [PATCH] fix: harden ref action reliability --- CLAUDE.md | 17 +- CONCEPTS.md | 71 ++++++ crates/core/src/action.rs | 194 --------------- crates/core/src/action_request.rs | 92 +++++++ crates/core/src/action_result.rs | 39 +++ crates/core/src/action_step.rs | 31 +++ crates/core/src/action_step_outcome.rs | 9 + crates/core/src/actionability.rs | 11 +- crates/core/src/actionability_tests.rs | 12 +- crates/core/src/adapter.rs | 9 +- crates/core/src/capability.rs | 164 +++++++++++++ crates/core/src/commands/check.rs | 3 +- crates/core/src/commands/clear.rs | 3 +- crates/core/src/commands/click.rs | 3 +- crates/core/src/commands/collapse.rs | 3 +- crates/core/src/commands/double_click.rs | 3 +- crates/core/src/commands/expand.rs | 3 +- crates/core/src/commands/focus.rs | 3 +- crates/core/src/commands/helpers.rs | 60 +++-- crates/core/src/commands/helpers_tests.rs | 5 +- crates/core/src/commands/is_check.rs | 19 +- crates/core/src/commands/press.rs | 3 +- crates/core/src/commands/ref_policy_tests.rs | 4 +- crates/core/src/commands/right_click.rs | 3 +- crates/core/src/commands/right_click_tests.rs | 2 +- crates/core/src/commands/scroll.rs | 3 +- crates/core/src/commands/scroll_to.rs | 3 +- crates/core/src/commands/select.rs | 6 +- crates/core/src/commands/set_value.rs | 6 +- crates/core/src/commands/toggle.rs | 3 +- crates/core/src/commands/triple_click.rs | 3 +- crates/core/src/commands/type_text.rs | 6 +- crates/core/src/commands/uncheck.rs | 3 +- crates/core/src/commands/wait.rs | 85 ++++--- .../core/src/commands/wait_element_tests.rs | 38 ++- crates/core/src/commands/wait_predicate.rs | 2 +- .../src/commands/wait_resolution_tests.rs | 32 ++- crates/core/src/commands/wait_timeout.rs | 54 +++++ crates/core/src/element_state.rs | 10 + crates/core/src/lib.rs | 15 +- crates/core/src/ref_action.rs | 7 +- crates/core/src/ref_alloc.rs | 15 +- crates/core/src/refs_store.rs | 10 + crates/core/src/refs_store_tests.rs | 132 ++++++++++ crates/core/src/refs_tests.rs | 116 +-------- crates/core/src/snapshot_ref_tests.rs | 4 +- crates/ffi/src/actions/execute.rs | 2 +- crates/ffi/src/actions/result.rs | 4 +- crates/macos/src/actions/chain.rs | 2 +- crates/macos/src/actions/chain_defs.rs | 2 +- crates/macos/src/actions/chain_steps.rs | 4 +- crates/macos/src/actions/dispatch.rs | 53 +--- crates/macos/src/actions/post_state.rs | 5 +- crates/macos/src/actions/scroll.rs | 7 +- crates/macos/src/actions/toggle_state.rs | 2 +- crates/macos/src/actions/type_text.rs | 4 +- crates/macos/src/adapter.rs | 7 +- crates/macos/src/notifications/actions.rs | 2 +- crates/macos/src/system/key_dispatch.rs | 5 +- crates/macos/src/tree/action_list.rs | 23 +- crates/macos/src/tree/attributes.rs | 228 ++++++++++++++++++ crates/macos/src/tree/builder.rs | 7 +- crates/macos/src/tree/element.rs | 199 +-------------- crates/macos/src/tree/mod.rs | 11 +- crates/macos/src/tree/resolve.rs | 80 +++--- crates/macos/src/tree/resolve_deadline.rs | 29 +++ crates/macos/src/tree/resolve_roots.rs | 89 ++++--- crates/macos/src/tree/resolve_tests.rs | 14 ++ crates/macos/src/tree/surfaces.rs | 5 +- ...ht-grade-desktop-reliability-2026-06-02.md | 200 +++++++++++++-- src/tests/conformance.rs | 5 +- 71 files changed, 1479 insertions(+), 829 deletions(-) create mode 100644 CONCEPTS.md create mode 100644 crates/core/src/action_request.rs create mode 100644 crates/core/src/action_result.rs create mode 100644 crates/core/src/action_step.rs create mode 100644 crates/core/src/action_step_outcome.rs create mode 100644 crates/core/src/capability.rs create mode 100644 crates/core/src/element_state.rs create mode 100644 crates/core/src/refs_store_tests.rs create mode 100644 crates/macos/src/tree/attributes.rs create mode 100644 crates/macos/src/tree/resolve_deadline.rs diff --git a/CLAUDE.md b/CLAUDE.md index 0367bfb..a8fdb02 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -63,6 +63,7 @@ agent-desktop is NOT an AI agent. It is a tool that AI agents invoke. It outputs ``` agent-desktop/ ├── Cargo.toml # workspace: members, shared deps +├── CONCEPTS.md # shared domain vocabulary for refs, snapshots, sessions, actionability, and related concepts ├── rust-toolchain.toml # pinned Rust version ├── clippy.toml # project-wide lint config ├── crates/ @@ -77,12 +78,12 @@ agent-desktop/ │ └── ffi/ # agent-desktop-ffi (cdylib + committed C ABI header) ├── src/ # agent-desktop binary (entry point) │ ├── main.rs # entry point, permission check, JSON envelope -│ ├── cli.rs # clap derive enum (Commands) -│ ├── cli_args.rs # all command argument structs -│ ├── batch.rs # batch JSON → typed Commands -│ ├── command_policy.rs # command permission/ref/side-effect policy -│ ├── dispatch.rs # command dispatcher + parse helpers -│ └── dispatch_notifications.rs +│ ├── batch/ # batch JSON → typed Commands +│ ├── cli/ # clap derive enum, help text, CLI contract tests +│ ├── cli_args/ # command argument structs by domain +│ ├── command_policy/ # permission/ref/side-effect policy +│ ├── dispatch/ # command dispatcher, parse helpers, notifications +│ └── tests/ # binary-level conformance tests ├── docs/ │ └── solutions/ # documented solutions to past problems (bugs, best practices, workflow patterns), organized by category with YAML frontmatter (module, tags, problem_type); relevant when implementing or debugging in documented areas └── tests/ @@ -150,7 +151,7 @@ pub fn dispatch( } ``` -Batch is not a second dispatcher. `src/batch.rs` deserializes JSON entries into the same typed `Commands` enum, runs the same `CommandPolicy` preflight, and calls the same `dispatch()` path as CLI. +Batch is not a second dispatcher. `src/batch/mod.rs` deserializes JSON entries into the same typed `Commands` enum, runs the same `CommandPolicy` preflight, and calls the same `dispatch()` path as CLI. ### Additive Phase Model @@ -259,7 +260,7 @@ crates/{macos,windows,linux}/src/ Adding a new command requires exactly these steps: 1. Create `crates/core/src/commands/{name}.rs` with an `execute()` function 2. Register it in `crates/core/src/commands/mod.rs` -3. Add the CLI subcommand variant to `src/cli.rs` (clap derive enum) +3. Add the CLI subcommand variant to `src/cli/mod.rs` and arguments under `src/cli_args/` 4. Add a match arm in `dispatch()` in the binary crate 5. If new `Action` variant needed, add to `crates/core/src/action.rs` 6. If new adapter method needed, add to `PlatformAdapter` trait with a default returning `Err(AdapterError::not_supported())` diff --git a/CONCEPTS.md b/CONCEPTS.md new file mode 100644 index 0000000..980ef2c --- /dev/null +++ b/CONCEPTS.md @@ -0,0 +1,71 @@ +# Concepts + +Shared domain vocabulary for this project -- entities, named processes, and status concepts with project-specific meaning. Seeded with core domain vocabulary, then accretes as ce-compound and ce-compound-refresh process learnings; direct edits are fine. Glossary only, not a spec or catch-all. + +## Desktop Observation + +### Accessibility Tree +A structured representation of an application's user interface exposed by the operating system accessibility APIs and used by agent-desktop as the source of truth for observation and semantic interaction. + +### Snapshot +An observation of an accessibility tree at a point in time, persisted with the element refs allocated from that observation. + +### Snapshot ID +A compact identifier for one persisted snapshot inside a session. + +### Surface +A scoped UI layer that can be observed separately from the whole window, such as an open menu, sheet, popover, alert, or focused area. + +### Drill-down +A snapshot operation that starts from an existing ref to observe that element's subtree instead of re-reading the entire window. + +## Refs And Identity + +### Ref +A short element identifier assigned by agent-desktop to an actionable or drillable node in a snapshot. + +Refs are deterministic inside one snapshot but are not stable across UI changes. Callers either pass the snapshot ID that produced the ref or intentionally use the session's latest snapshot pointer. + +### RefMap +The persisted mapping from refs to the identity evidence needed to re-identify elements later. + +### Stale Ref +A ref whose stored identity no longer matches a live element strongly enough to act safely. + +### Strict Ref Resolution +The fail-closed process of re-identifying a ref from stored identity evidence before a command acts on it. + +Strict ref resolution rejects missing, stale, and ambiguous matches instead of guessing. It is the boundary between an old observation and a live desktop mutation. + +## Coordination + +### Session +A namespace for snapshots, ref maps, and the latest-snapshot pointer shared by one agent or a coordinated group of agents. + +A session can contain many snapshots. The latest-snapshot pointer is a convenience for fluid workflows, not a replacement for explicit snapshot IDs when deterministic replay matters. + +## Action Reliability + +### Actionability +The pre-dispatch judgement that a resolved element is safe to act on, based on native evidence such as visibility, stability, enabled state, supported action, policy, and editability. + +### Interaction Policy +The side-effect contract attached to an action request, controlling whether the command may steal focus, move the cursor, or use physical input fallbacks. + +### Headless Ref Action +A ref-based action that uses semantic accessibility operations without implicit focus stealing, cursor movement, synthetic keyboard input, or pasteboard use. + +Headless ref actions may still fail when the native accessibility API cannot perform the requested semantic operation. A broader interaction policy must be explicit rather than silently substituting physical input. + +### Wait Predicate +The condition a wait command polls for before returning, such as element actionability, text presence, window appearance, menu state, or notification arrival. + +### Coordinate Fallback +An explicit opt-in path that uses screen coordinates or physical input when semantic accessibility operations cannot perform the requested action. + +### FFI Ref-Action Parity +The requirement that language bindings using refs follow the same strict resolution, actionability, and interaction-policy semantics as CLI ref commands. + +## Relationships + +A session contains many snapshots and owns one latest-snapshot pointer. A snapshot persists a ref map. A ref resolves through strict ref resolution into live native evidence, then actionability decides whether a headless ref action can safely dispatch. FFI ref-action parity keeps that same relationship true for language bindings. diff --git a/crates/core/src/action.rs b/crates/core/src/action.rs index d3e70de..3e76b44 100644 --- a/crates/core/src/action.rs +++ b/crates/core/src/action.rs @@ -53,28 +53,6 @@ impl Action { } } - pub fn semantic_capabilities(&self) -> &'static [&'static str] { - match self { - Self::Click | Self::DoubleClick | Self::TripleClick => &["Click"], - Self::RightClick => &["RightClick", "Click"], - Self::SetValue(_) | Self::Clear => &["SetValue"], - Self::SetFocus => &["SetFocus"], - Self::Expand => &["Expand"], - Self::Collapse => &["Collapse"], - Self::Select(_) => &["Select", "Click"], - Self::Toggle => &["Toggle", "Click"], - Self::Check | Self::Uncheck => &["Toggle", "Click"], - Self::Scroll(_, _) => &["Scroll", "ScrollTo"], - Self::ScrollTo => &["ScrollTo"], - Self::PressKey(_) => &["PressKey"], - Self::KeyDown(_) => &["KeyDown"], - Self::KeyUp(_) => &["KeyUp"], - Self::TypeText(_) => &["TypeText", "SetValue"], - Self::Hover => &["Hover"], - Self::Drag(_) => &["Drag"], - } - } - pub fn requires_cursor_policy(&self) -> bool { matches!(self, Self::Hover | Self::Drag(_)) } @@ -84,95 +62,6 @@ impl Action { } } -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ActionRequest { - pub action: Action, - pub policy: InteractionPolicy, -} - -#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] -pub struct InteractionPolicy { - pub allow_focus_steal: bool, - pub allow_cursor_move: bool, -} - -impl ActionRequest { - pub fn headless(action: Action) -> Self { - Self { - action, - policy: InteractionPolicy::headless(), - } - } - - pub fn focus_fallback(action: Action) -> Self { - Self { - action, - policy: InteractionPolicy::focus_fallback(), - } - } - - pub fn physical(action: Action) -> Self { - Self { - action, - policy: InteractionPolicy::physical(), - } - } -} - -impl InteractionPolicy { - pub fn headless() -> Self { - Self { - allow_focus_steal: false, - allow_cursor_move: false, - } - } - - pub fn focus_fallback() -> Self { - Self { - allow_focus_steal: true, - allow_cursor_move: false, - } - } - - pub fn physical() -> Self { - Self { - allow_focus_steal: true, - allow_cursor_move: true, - } - } -} - -impl Default for InteractionPolicy { - fn default() -> Self { - Self::headless() - } -} - -#[cfg(test)] -mod policy_tests { - use super::*; - - #[test] - fn default_policy_is_headless() { - let policy = InteractionPolicy::default(); - assert!(!policy.allow_focus_steal); - assert!(!policy.allow_cursor_move); - } - - #[test] - fn headless_request_blocks_physical_side_effects() { - let request = ActionRequest::headless(Action::Click); - assert_eq!(request.policy, InteractionPolicy::headless()); - } - - #[test] - fn focus_fallback_policy_never_moves_cursor() { - let request = ActionRequest::focus_fallback(Action::Scroll(Direction::Down, 1)); - assert!(request.policy.allow_focus_steal); - assert!(!request.policy.allow_cursor_move); - } -} - #[derive(Debug, Clone, Serialize, Deserialize)] pub enum Direction { Up, @@ -239,86 +128,3 @@ pub enum Modifier { Alt, Shift, } - -#[derive(Debug, Clone, Serialize, Deserialize)] -pub struct ActionResult { - pub action: String, - #[serde(skip_serializing_if = "Option::is_none")] - 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)] -pub struct ElementState { - pub role: String, - #[serde(skip_serializing_if = "Vec::is_empty", default)] - pub states: Vec, - #[serde(skip_serializing_if = "Option::is_none")] - 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(), - } - } - - pub fn with_ref(mut self, ref_id: impl Into) -> Self { - self.ref_id = Some(ref_id.into()); - self - } - - pub fn with_state(mut self, state: ElementState) -> Self { - 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/action_request.rs b/crates/core/src/action_request.rs new file mode 100644 index 0000000..c308f03 --- /dev/null +++ b/crates/core/src/action_request.rs @@ -0,0 +1,92 @@ +use crate::action::Action; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ActionRequest { + pub action: Action, + pub policy: InteractionPolicy, +} + +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +pub struct InteractionPolicy { + pub allow_focus_steal: bool, + pub allow_cursor_move: bool, +} + +impl ActionRequest { + pub fn headless(action: Action) -> Self { + Self { + action, + policy: InteractionPolicy::headless(), + } + } + + pub fn focus_fallback(action: Action) -> Self { + Self { + action, + policy: InteractionPolicy::focus_fallback(), + } + } + + pub fn physical(action: Action) -> Self { + Self { + action, + policy: InteractionPolicy::physical(), + } + } +} + +impl InteractionPolicy { + pub fn headless() -> Self { + Self { + allow_focus_steal: false, + allow_cursor_move: false, + } + } + + pub fn focus_fallback() -> Self { + Self { + allow_focus_steal: true, + allow_cursor_move: false, + } + } + + pub fn physical() -> Self { + Self { + allow_focus_steal: true, + allow_cursor_move: true, + } + } +} + +impl Default for InteractionPolicy { + fn default() -> Self { + Self::headless() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::action::{Action, Direction}; + + #[test] + fn default_policy_is_headless() { + let policy = InteractionPolicy::default(); + assert!(!policy.allow_focus_steal); + assert!(!policy.allow_cursor_move); + } + + #[test] + fn headless_request_blocks_physical_side_effects() { + let request = ActionRequest::headless(Action::Click); + assert_eq!(request.policy, InteractionPolicy::headless()); + } + + #[test] + fn focus_fallback_policy_never_moves_cursor() { + let request = ActionRequest::focus_fallback(Action::Scroll(Direction::Down, 1)); + assert!(request.policy.allow_focus_steal); + assert!(!request.policy.allow_cursor_move); + } +} diff --git a/crates/core/src/action_result.rs b/crates/core/src/action_result.rs new file mode 100644 index 0000000..7d9c1c4 --- /dev/null +++ b/crates/core/src/action_result.rs @@ -0,0 +1,39 @@ +use crate::{action_step::ActionStep, element_state::ElementState}; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ActionResult { + pub action: String, + #[serde(skip_serializing_if = "Option::is_none")] + 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, +} + +impl ActionResult { + pub fn new(action: impl Into) -> Self { + Self { + action: action.into(), + ref_id: None, + post_state: None, + steps: Vec::new(), + } + } + + pub fn with_ref(mut self, ref_id: impl Into) -> Self { + self.ref_id = Some(ref_id.into()); + self + } + + pub fn with_state(mut self, state: ElementState) -> Self { + 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/action_step.rs b/crates/core/src/action_step.rs new file mode 100644 index 0000000..4b323b2 --- /dev/null +++ b/crates/core/src/action_step.rs @@ -0,0 +1,31 @@ +use crate::action_step_outcome::ActionStepOutcome; +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ActionStep { + pub label: String, + pub outcome: ActionStepOutcome, +} + +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, + } + } +} diff --git a/crates/core/src/action_step_outcome.rs b/crates/core/src/action_step_outcome.rs new file mode 100644 index 0000000..fa63d84 --- /dev/null +++ b/crates/core/src/action_step_outcome.rs @@ -0,0 +1,9 @@ +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum ActionStepOutcome { + Attempted, + Skipped, + Succeeded, +} diff --git a/crates/core/src/actionability.rs b/crates/core/src/actionability.rs index b8a0a90..d53fb3f 100644 --- a/crates/core/src/actionability.rs +++ b/crates/core/src/actionability.rs @@ -1,5 +1,7 @@ +use crate::capability; use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::{NativeHandle, PlatformAdapter}, error::{AdapterError, ErrorCode}, node::Rect, @@ -149,7 +151,7 @@ fn action_supported_check(entry: &RefEntry, request: &ActionRequest) -> Actionab "semantic action unavailable but fallback policy allows attempt", ); } - let expected = request.action.semantic_capabilities().join(" or "); + let expected = capability::for_action(&request.action).join(" or "); fail("supported_action", format!("{expected} is not available")) } @@ -179,7 +181,7 @@ fn editable_check(entry: &RefEntry, action: &Action) -> ActionabilityCheck { if entry .available_actions .iter() - .any(|action| action == "SetValue") + .any(|action| action == capability::SET_VALUE) { return pass("editable"); } @@ -207,8 +209,7 @@ fn failed_check(report: &ActionabilityReport, name: &str) -> bool { } fn supported_by_available_actions(action: &Action, available_actions: &[String]) -> bool { - action - .semantic_capabilities() + capability::for_action(action) .iter() .any(|expected| available_actions.iter().any(|action| action == expected)) } diff --git a/crates/core/src/actionability_tests.rs b/crates/core/src/actionability_tests.rs index 21ccfb0..506ecc2 100644 --- a/crates/core/src/actionability_tests.rs +++ b/crates/core/src/actionability_tests.rs @@ -1,7 +1,9 @@ use super::*; use crate::{ - action::{Action, ActionRequest, Direction, ElementState}, + action::{Action, Direction}, + action_request::ActionRequest, adapter::{LiveElement, NativeHandle, PlatformAdapter, SnapshotSurface}, + element_state::ElementState, node::Rect, refs::RefEntry, }; @@ -158,6 +160,14 @@ fn cursor_movement_requires_physical_policy() { assert!(err.message.contains("policy")); } +#[test] +fn right_click_requires_right_click_capability_before_dispatch() { + let err = check(&entry(), &ActionRequest::headless(Action::RightClick)).unwrap_err(); + + assert_eq!(err.code, ErrorCode::ActionFailed); + assert!(err.message.contains("supported_action")); +} + #[test] fn command_aliases_match_platform_capabilities() { let click_entry = entry(); diff --git a/crates/core/src/adapter.rs b/crates/core/src/adapter.rs index a9fcfc6..4c16992 100644 --- a/crates/core/src/adapter.rs +++ b/crates/core/src/adapter.rs @@ -1,8 +1,9 @@ use crate::{ PermissionReport, PermissionState, - action::{ - ActionRequest, ActionResult, DragParams, ElementState, KeyCombo, MouseEvent, WindowOp, - }, + action::{DragParams, KeyCombo, MouseEvent, WindowOp}, + action_request::ActionRequest, + action_result::ActionResult, + element_state::ElementState, error::{AdapterError, ErrorCode}, node::{AccessibilityNode, AppInfo, Rect, SurfaceInfo, WindowInfo}, notification::{NotificationFilter, NotificationIdentity, NotificationInfo}, @@ -274,7 +275,7 @@ pub trait PlatformAdapter: Send + Sync { &self, _app_name: &str, _combo: &crate::action::KeyCombo, - ) -> Result { + ) -> Result { Err(AdapterError::not_supported("press_key_for_app")) } diff --git a/crates/core/src/capability.rs b/crates/core/src/capability.rs new file mode 100644 index 0000000..8ecc078 --- /dev/null +++ b/crates/core/src/capability.rs @@ -0,0 +1,164 @@ +use crate::action::Action; + +pub const CLICK: &str = "Click"; +pub const RIGHT_CLICK: &str = "RightClick"; +pub const SET_VALUE: &str = "SetValue"; +pub const SET_FOCUS: &str = "SetFocus"; +pub const EXPAND: &str = "Expand"; +pub const COLLAPSE: &str = "Collapse"; +pub const SELECT: &str = "Select"; +pub const TOGGLE: &str = "Toggle"; +pub const SCROLL: &str = "Scroll"; +pub const SCROLL_TO: &str = "ScrollTo"; +pub const PRESS_KEY: &str = "PressKey"; +pub const KEY_DOWN: &str = "KeyDown"; +pub const KEY_UP: &str = "KeyUp"; +pub const TYPE_TEXT: &str = "TypeText"; +pub const HOVER: &str = "Hover"; +pub const DRAG: &str = "Drag"; +pub const CHECK: &str = "Check"; +pub const UNCHECK: &str = "Uncheck"; + +pub const ALL: &[&str] = &[ + CLICK, + RIGHT_CLICK, + SET_VALUE, + SET_FOCUS, + EXPAND, + COLLAPSE, + SELECT, + TOGGLE, + SCROLL, + SCROLL_TO, + PRESS_KEY, + KEY_DOWN, + KEY_UP, + TYPE_TEXT, + HOVER, + DRAG, + CHECK, + UNCHECK, +]; + +pub const CHECKED_APPLICABILITY: &[&str] = &[TOGGLE, CHECK, UNCHECK]; +pub const EXPANDED_APPLICABILITY: &[&str] = &[EXPAND, COLLAPSE]; + +pub fn for_action(action: &Action) -> &'static [&'static str] { + match action { + Action::Click | Action::DoubleClick | Action::TripleClick => &[CLICK], + Action::RightClick => &[RIGHT_CLICK], + Action::SetValue(_) | Action::Clear => &[SET_VALUE], + Action::SetFocus => &[SET_FOCUS], + Action::Expand => &[EXPAND], + Action::Collapse => &[COLLAPSE], + Action::Select(_) => &[SELECT, CLICK], + Action::Toggle => &[TOGGLE, CLICK], + Action::Check | Action::Uncheck => &[TOGGLE, CLICK], + Action::Scroll(_, _) => &[SCROLL, SCROLL_TO], + Action::ScrollTo => &[SCROLL_TO], + Action::PressKey(_) => &[PRESS_KEY], + Action::KeyDown(_) => &[KEY_DOWN], + Action::KeyUp(_) => &[KEY_UP], + Action::TypeText(_) => &[TYPE_TEXT, SET_VALUE], + Action::Hover => &[HOVER], + Action::Drag(_) => &[DRAG], + } +} + +pub fn defaults_for_role(role: &str) -> Vec { + role_default_slice(role) + .iter() + .map(|capability| (*capability).to_string()) + .collect() +} + +pub fn contains(actions: &[String], capability: &str) -> bool { + actions.iter().any(|action| action == capability) +} + +pub fn contains_any(actions: &[String], capabilities: &[&str]) -> bool { + capabilities + .iter() + .any(|capability| contains(actions, capability)) +} + +fn role_default_slice(role: &str) -> &'static [&'static str] { + match role { + "button" | "link" | "menuitem" | "tab" | "radiobutton" => &[CLICK], + "textfield" | "incrementor" => &[CLICK, SET_VALUE, SET_FOCUS], + "checkbox" => &[CLICK, TOGGLE], + "combobox" => &[CLICK, SELECT], + "treeitem" => &[CLICK, EXPAND, COLLAPSE], + "slider" => &[SET_VALUE], + "cell" => &[CLICK], + _ => &[CLICK], + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::action::{Direction, KeyCombo}; + use std::collections::HashSet; + + const BEHAVIOR_FILES: &[(&str, &str)] = &[ + ("action.rs", include_str!("action.rs")), + ("actionability.rs", include_str!("actionability.rs")), + ("ref_alloc.rs", include_str!("ref_alloc.rs")), + ("commands/is_check.rs", include_str!("commands/is_check.rs")), + ]; + + #[test] + fn action_capabilities_are_declared_in_one_place() { + assert_eq!(for_action(&Action::Click), &[CLICK]); + assert_eq!(for_action(&Action::RightClick), &[RIGHT_CLICK]); + assert_eq!(for_action(&Action::SetValue("x".into())), &[SET_VALUE]); + assert_eq!(for_action(&Action::Clear), &[SET_VALUE]); + assert_eq!( + for_action(&Action::Scroll(Direction::Down, 1)), + &[SCROLL, SCROLL_TO] + ); + assert_eq!( + for_action(&Action::PressKey(KeyCombo { + key: "A".into(), + modifiers: vec![], + })), + &[PRESS_KEY] + ); + } + + #[test] + fn role_defaults_are_declared_in_one_place() { + assert_eq!(defaults_for_role("button"), strings(&[CLICK])); + assert_eq!( + defaults_for_role("textfield"), + strings(&[CLICK, SET_VALUE, SET_FOCUS]) + ); + assert_eq!( + defaults_for_role("treeitem"), + strings(&[CLICK, EXPAND, COLLAPSE]) + ); + } + + #[test] + fn capability_literals_are_unique_to_this_module() { + let mut seen = HashSet::new(); + for capability in ALL { + assert!( + seen.insert(*capability), + "duplicate capability: {capability}" + ); + let literal = format!("\"{capability}\""); + for (path, content) in BEHAVIOR_FILES { + assert!( + !content.contains(&literal), + "capability literal {literal} must be referenced through capability.rs in {path}" + ); + } + } + } + + fn strings(values: &[&str]) -> Vec { + values.iter().map(|value| (*value).to_string()).collect() + } +} diff --git a/crates/core/src/commands/check.rs b/crates/core/src/commands/check.rs index 1e13f41..582dafa 100644 --- a/crates/core/src/commands/check.rs +++ b/crates/core/src/commands/check.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/clear.rs b/crates/core/src/commands/clear.rs index e57cc92..f9cd6fe 100644 --- a/crates/core/src/commands/clear.rs +++ b/crates/core/src/commands/clear.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/click.rs b/crates/core/src/commands/click.rs index 562bb43..c8a6c21 100644 --- a/crates/core/src/commands/click.rs +++ b/crates/core/src/commands/click.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/collapse.rs b/crates/core/src/commands/collapse.rs index 7461d0b..5cda85a 100644 --- a/crates/core/src/commands/collapse.rs +++ b/crates/core/src/commands/collapse.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/double_click.rs b/crates/core/src/commands/double_click.rs index 6976652..f646bed 100644 --- a/crates/core/src/commands/double_click.rs +++ b/crates/core/src/commands/double_click.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/expand.rs b/crates/core/src/commands/expand.rs index 90f284b..4fd9c4f 100644 --- a/crates/core/src/commands/expand.rs +++ b/crates/core/src/commands/expand.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/focus.rs b/crates/core/src/commands/focus.rs index 6367020..1f306e5 100644 --- a/crates/core/src/commands/focus.rs +++ b/crates/core/src/commands/focus.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 29e4d14..b11b160 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -1,5 +1,7 @@ use crate::{ - action::{ActionRequest, ActionResult, Point, WindowOp}, + action::{Point, WindowOp}, + action_request::ActionRequest, + action_result::ActionResult, adapter::{NativeHandle, PlatformAdapter, WindowFilter}, context::CommandContext, error::AppError, @@ -160,7 +162,14 @@ 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(ActionabilityTraceInput { + ref_id, + entry: &entry, + handle: handle.handle(), + adapter, + request: &request, + context, + })?; context.trace_lazy( "action.dispatch.start", || json!({ "ref": ref_id, "action": request.action.name() }), @@ -174,31 +183,34 @@ pub(crate) fn execute_ref_action_result_with_context( Ok((entry, result)) } -fn check_actionability_with_trace( - ref_id: &str, - entry: &RefEntry, - handle: &NativeHandle, - adapter: &dyn PlatformAdapter, - request: &ActionRequest, - context: &CommandContext, -) -> Result<(), AppError> { - context.trace_lazy( +struct ActionabilityTraceInput<'a> { + ref_id: &'a str, + entry: &'a RefEntry, + handle: &'a NativeHandle, + adapter: &'a dyn PlatformAdapter, + request: &'a ActionRequest, + context: &'a CommandContext, +} + +fn check_actionability_with_trace(input: ActionabilityTraceInput<'_>) -> Result<(), AppError> { + input.context.trace_lazy( "actionability.check.start", - || json!({ "ref": ref_id, "action": request.action.name() }), + || json!({ "ref": input.ref_id, "action": input.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( + crate::actionability::check_live(input.entry, input.handle, input.adapter, input.request) + .inspect_err(|err| { + let _ = input.context.trace_lazy("actionability.check.error", || { + json!({ + "ref": input.ref_id, + "action": input.request.action.name(), + "code": err.code.as_str(), + "message": err.message.clone() + }) + }); + })?; + input.context.trace_lazy( "actionability.check.ok", - || json!({ "ref": ref_id, "action": request.action.name() }), + || json!({ "ref": input.ref_id, "action": input.request.action.name() }), )?; Ok(()) } diff --git a/crates/core/src/commands/helpers_tests.rs b/crates/core/src/commands/helpers_tests.rs index f38d6fc..f321de6 100644 --- a/crates/core/src/commands/helpers_tests.rs +++ b/crates/core/src/commands/helpers_tests.rs @@ -1,10 +1,13 @@ use super::*; -use crate::action::{Action, ActionResult, ActionStep, ElementState, InteractionPolicy}; use crate::adapter::NativeHandle; use crate::error::{AdapterError, ErrorCode}; use crate::node::AppInfo; use crate::refs::RefMap; use crate::refs_test_support::HomeGuard; +use crate::{ + action::Action, action_request::InteractionPolicy, action_result::ActionResult, + action_step::ActionStep, element_state::ElementState, +}; use std::sync::Mutex; use std::sync::atomic::{AtomicU32, Ordering}; diff --git a/crates/core/src/commands/is_check.rs b/crates/core/src/commands/is_check.rs index 5685bb3..3f97aa6 100644 --- a/crates/core/src/commands/is_check.rs +++ b/crates/core/src/commands/is_check.rs @@ -1,8 +1,8 @@ use crate::{ - action::ElementState, adapter::{PlatformAdapter, optional_live_read}, commands::helpers::resolve_ref_with_context, context::CommandContext, + element_state::ElementState, error::AppError, refs::RefEntry, }; @@ -78,23 +78,22 @@ fn is_applicable(property: &IsProperty, entry: &RefEntry, state: &ElementState) IsProperty::Checked => { crate::roles::is_toggleable_role(&entry.role) || has_state(state, "checked") - || has_available_action(entry, "Toggle") - || has_available_action(entry, "Check") - || has_available_action(entry, "Uncheck") + || crate::capability::contains_any( + &entry.available_actions, + crate::capability::CHECKED_APPLICABILITY, + ) } IsProperty::Expanded => { crate::roles::is_expandable_role(&entry.role) || has_state(state, "expanded") - || has_available_action(entry, "Expand") - || has_available_action(entry, "Collapse") + || crate::capability::contains_any( + &entry.available_actions, + crate::capability::EXPANDED_APPLICABILITY, + ) } } } -fn has_available_action(entry: &RefEntry, action: &str) -> bool { - entry.available_actions.iter().any(|a| a == action) -} - #[cfg(test)] #[path = "is_check_tests.rs"] mod tests; diff --git a/crates/core/src/commands/press.rs b/crates/core/src/commands/press.rs index 665e309..ceb27a0 100644 --- a/crates/core/src/commands/press.rs +++ b/crates/core/src/commands/press.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest, KeyCombo, Modifier}, + action::{Action, KeyCombo, Modifier}, + action_request::ActionRequest, adapter::PlatformAdapter, error::AppError, }; diff --git a/crates/core/src/commands/ref_policy_tests.rs b/crates/core/src/commands/ref_policy_tests.rs index 8536c89..5085d14 100644 --- a/crates/core/src/commands/ref_policy_tests.rs +++ b/crates/core/src/commands/ref_policy_tests.rs @@ -1,5 +1,7 @@ use crate::{ - action::{Action, ActionRequest, ActionResult, Direction, InteractionPolicy}, + action::{Action, Direction}, + action_request::{ActionRequest, InteractionPolicy}, + action_result::ActionResult, adapter::{NativeHandle, PlatformAdapter}, commands::{ check, clear, click, collapse, double_click, expand, focus, helpers::RefArgs, right_click, diff --git a/crates/core/src/commands/right_click.rs b/crates/core/src/commands/right_click.rs index 481a646..635132c 100644 --- a/crates/core/src/commands/right_click.rs +++ b/crates/core/src/commands/right_click.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::{PlatformAdapter, SnapshotSurface, TreeOptions}, commands::helpers::{RefArgs, execute_ref_action_result_with_context, find_window_for_pid}, context::CommandContext, diff --git a/crates/core/src/commands/right_click_tests.rs b/crates/core/src/commands/right_click_tests.rs index 4fc76b8..b5ff83d 100644 --- a/crates/core/src/commands/right_click_tests.rs +++ b/crates/core/src/commands/right_click_tests.rs @@ -1,6 +1,6 @@ use super::*; use crate::{ - action::ActionResult, + action_result::ActionResult, adapter::{NativeHandle, WindowFilter}, error::{AdapterError, ErrorCode}, node::WindowInfo, diff --git a/crates/core/src/commands/scroll.rs b/crates/core/src/commands/scroll.rs index 7a1cce8..5ec5762 100644 --- a/crates/core/src/commands/scroll.rs +++ b/crates/core/src/commands/scroll.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest, Direction}, + action::{Action, Direction}, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::execute_ref_action_result_with_context, context::CommandContext, diff --git a/crates/core/src/commands/scroll_to.rs b/crates/core/src/commands/scroll_to.rs index 108f32c..d629d8d 100644 --- a/crates/core/src/commands/scroll_to.rs +++ b/crates/core/src/commands/scroll_to.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/select.rs b/crates/core/src/commands/select.rs index 1d6afa9..b13e7cd 100644 --- a/crates/core/src/commands/select.rs +++ b/crates/core/src/commands/select.rs @@ -1,8 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, - adapter::PlatformAdapter, - commands::helpers::execute_ref_action_result_with_context, - context::CommandContext, + action::Action, action_request::ActionRequest, adapter::PlatformAdapter, + commands::helpers::execute_ref_action_result_with_context, context::CommandContext, error::AppError, }; use serde_json::Value; diff --git a/crates/core/src/commands/set_value.rs b/crates/core/src/commands/set_value.rs index 14c8cf5..aaae824 100644 --- a/crates/core/src/commands/set_value.rs +++ b/crates/core/src/commands/set_value.rs @@ -1,8 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, - adapter::PlatformAdapter, - commands::helpers::execute_ref_action_result_with_context, - context::CommandContext, + action::Action, action_request::ActionRequest, adapter::PlatformAdapter, + commands::helpers::execute_ref_action_result_with_context, context::CommandContext, error::AppError, }; use serde_json::Value; diff --git a/crates/core/src/commands/toggle.rs b/crates/core/src/commands/toggle.rs index 96e4860..d2e8e24 100644 --- a/crates/core/src/commands/toggle.rs +++ b/crates/core/src/commands/toggle.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/triple_click.rs b/crates/core/src/commands/triple_click.rs index c94a6aa..b55c27d 100644 --- a/crates/core/src/commands/triple_click.rs +++ b/crates/core/src/commands/triple_click.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/type_text.rs b/crates/core/src/commands/type_text.rs index 14304c5..18e1235 100644 --- a/crates/core/src/commands/type_text.rs +++ b/crates/core/src/commands/type_text.rs @@ -1,8 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, - adapter::PlatformAdapter, - commands::helpers::execute_ref_action_result_with_context, - context::CommandContext, + action::Action, action_request::ActionRequest, adapter::PlatformAdapter, + commands::helpers::execute_ref_action_result_with_context, context::CommandContext, error::AppError, }; use serde_json::Value; diff --git a/crates/core/src/commands/uncheck.rs b/crates/core/src/commands/uncheck.rs index 7324312..444f4a1 100644 --- a/crates/core/src/commands/uncheck.rs +++ b/crates/core/src/commands/uncheck.rs @@ -1,5 +1,6 @@ use crate::{ - action::{Action, ActionRequest}, + action::Action, + action_request::ActionRequest, adapter::PlatformAdapter, commands::helpers::{RefArgs, execute_ref_action_with_context}, context::CommandContext, diff --git a/crates/core/src/commands/wait.rs b/crates/core/src/commands/wait.rs index 9a92ff7..432bd61 100644 --- a/crates/core/src/commands/wait.rs +++ b/crates/core/src/commands/wait.rs @@ -66,11 +66,27 @@ pub fn execute_with_context( ref_id, snapshot_id, predicate, - } => wait_for_element(ref_id, snapshot_id, predicate, timeout_ms, adapter, context), + } => wait_for_element( + ElementWaitInput { + ref_id, + snapshot_id, + predicate, + timeout_ms, + }, + adapter, + context, + ), WaitMode::Window(title) => wait_for_window(title, timeout_ms, adapter), - WaitMode::Text { text, count, app } => { - wait_for_text(text, count, app, timeout_ms, adapter, context) - } + WaitMode::Text { text, count, app } => wait_for_text( + TextWaitInput { + text, + expected_count: count, + app, + timeout_ms, + }, + adapter, + context, + ), } } @@ -89,14 +105,24 @@ fn wait_for_menu( Ok(json!({ "found": true, "elapsed_ms": elapsed })) } -fn wait_for_element( +struct ElementWaitInput { ref_id: String, snapshot_id: Option, predicate: wait_predicate::ElementPredicate, timeout_ms: u64, +} + +fn wait_for_element( + input: ElementWaitInput, adapter: &dyn PlatformAdapter, context: &CommandContext, ) -> Result { + let ElementWaitInput { + ref_id, + snapshot_id, + predicate, + timeout_ms, + } = input; let start = Instant::now(); let timeout = Duration::from_millis(timeout_ms); let store = RefStore::for_session(context.session_id())?; @@ -203,17 +229,7 @@ fn wait_for_window( let remaining = timeout.saturating_sub(start.elapsed()); if remaining.is_zero() { - return Err(AppError::Adapter( - crate::error::AdapterError::timeout(format!( - "Window with title '{title}' not found within {timeout_ms}ms" - )) - .with_details(json!({ - "predicate": "window", - "title": title, - "timeout_ms": timeout_ms, - "last_error": last_error - })), - )); + return wait_timeout::window(&title, timeout_ms, last_error); } std::thread::sleep(remaining.min(Duration::from_millis(100))); @@ -230,14 +246,24 @@ fn is_retryable_wait_resolution_error(code: &ErrorCode) -> bool { ) } -fn wait_for_text( +struct TextWaitInput { text: String, expected_count: Option, app: Option, timeout_ms: u64, +} + +fn wait_for_text( + input: TextWaitInput, adapter: &dyn PlatformAdapter, context: &CommandContext, ) -> Result { + let TextWaitInput { + text, + expected_count, + app, + timeout_ms, + } = input; let start = Instant::now(); let timeout = Duration::from_millis(timeout_ms); let opts = crate::adapter::TreeOptions::default(); @@ -279,18 +305,7 @@ fn wait_for_text( let remaining = timeout.saturating_sub(start.elapsed()); if remaining.is_zero() { - return Err(AppError::Adapter( - crate::error::AdapterError::timeout(format!( - "Text '{text}' did not match within {timeout_ms}ms" - )) - .with_details(json!({ - "predicate": "text", - "text_chars": text.chars().count(), - "timeout_ms": timeout_ms, - "expected_count": expected_count, - "last_error": last_error - })), - )); + return wait_timeout::text(&text, timeout_ms, expected_count, last_error); } std::thread::sleep(remaining.min(interval)); @@ -328,17 +343,7 @@ fn wait_for_notification( loop { let remaining = timeout.saturating_sub(start.elapsed()); if remaining.is_zero() { - return Err(AppError::Adapter( - crate::error::AdapterError::timeout(format!( - "No new notification within {timeout_ms}ms" - )) - .with_details(json!({ - "predicate": "notification", - "timeout_ms": timeout_ms, - "app": app.clone(), - "text_chars": text.as_ref().map(|text| text.chars().count()) - })), - )); + return wait_timeout::notification(app.as_ref(), text.as_ref(), timeout_ms); } let current = adapter .list_notifications(&filter) diff --git a/crates/core/src/commands/wait_element_tests.rs b/crates/core/src/commands/wait_element_tests.rs index beff093..4683c26 100644 --- a/crates/core/src/commands/wait_element_tests.rs +++ b/crates/core/src/commands/wait_element_tests.rs @@ -1,7 +1,7 @@ use super::*; use crate::{ - action::ElementState, adapter::{NativeHandle, PlatformAdapter}, + element_state::ElementState, error::AdapterError, node::Rect, refs::{RefEntry, RefMap}, @@ -11,6 +11,26 @@ use crate::{ use std::sync::Mutex; use std::sync::atomic::{AtomicU32, Ordering}; +fn wait_for_element_test( + ref_id: String, + snapshot_id: Option, + predicate: wait_predicate::ElementPredicate, + timeout_ms: u64, + adapter: &dyn PlatformAdapter, + context: &crate::context::CommandContext, +) -> Result { + super::wait_for_element( + ElementWaitInput { + ref_id, + snapshot_id, + predicate, + timeout_ms, + }, + adapter, + context, + ) +} + struct NoopAdapter; impl PlatformAdapter for NoopAdapter {} @@ -125,7 +145,7 @@ fn snapshot_pinned_missing_ref_is_invalid_args() { let _guard = HomeGuard::new(); let snapshot_id = snapshot_with_one_ref(); - let err = wait_for_element( + let err = wait_for_element_test( "@e2".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, @@ -153,7 +173,7 @@ fn element_wait_enabled_predicate_uses_live_state() { bounds: None, }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Enabled, @@ -177,7 +197,7 @@ fn element_wait_value_predicate_matches_live_value_without_leaking_it() { bounds: None, }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Value("ready".into()), @@ -207,11 +227,11 @@ fn element_wait_timeout_reports_last_actionability_observation() { bounds: None, }; - let err = wait_for_element( + let err = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Actionable, - 1, + 50, &adapter, &crate::context::CommandContext::default(), ) @@ -242,7 +262,7 @@ fn element_wait_actionable_uses_live_state() { bounds: None, }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Actionable, @@ -264,7 +284,7 @@ fn element_wait_actionable_retries_until_live_state_converges() { states: Mutex::new(vec![vec![], vec!["disabled".into()]]), }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Actionable, @@ -286,7 +306,7 @@ fn element_wait_propagates_live_read_errors_after_releasing_handle() { releases: AtomicU32::new(0), }; - let err = wait_for_element( + let err = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Enabled, diff --git a/crates/core/src/commands/wait_predicate.rs b/crates/core/src/commands/wait_predicate.rs index 52b0304..919670b 100644 --- a/crates/core/src/commands/wait_predicate.rs +++ b/crates/core/src/commands/wait_predicate.rs @@ -117,7 +117,7 @@ fn actionable( handle: &NativeHandle, adapter: &dyn PlatformAdapter, ) -> Result { - let request = crate::action::ActionRequest::headless(crate::action::Action::Click); + let request = crate::action_request::ActionRequest::headless(crate::action::Action::Click); match crate::actionability::check_live(entry, handle, adapter, &request) { Ok(report) => Ok(json!(report)), Err(err) if err.code == ErrorCode::ActionFailed => { diff --git a/crates/core/src/commands/wait_resolution_tests.rs b/crates/core/src/commands/wait_resolution_tests.rs index 5da8259..b293c20 100644 --- a/crates/core/src/commands/wait_resolution_tests.rs +++ b/crates/core/src/commands/wait_resolution_tests.rs @@ -9,6 +9,26 @@ use crate::{ use std::sync::Mutex; use std::time::Duration; +fn wait_for_element_test( + ref_id: String, + snapshot_id: Option, + predicate: wait_predicate::ElementPredicate, + timeout_ms: u64, + adapter: &dyn PlatformAdapter, + context: &crate::context::CommandContext, +) -> Result { + super::wait_for_element( + ElementWaitInput { + ref_id, + snapshot_id, + predicate, + timeout_ms, + }, + adapter, + context, + ) +} + struct AmbiguousResolveAdapter; impl PlatformAdapter for AmbiguousResolveAdapter { @@ -104,7 +124,7 @@ fn element_wait_retries_transient_ambiguous_resolution() { errors: Mutex::new(vec![ErrorCode::AmbiguousTarget]), }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, @@ -126,7 +146,7 @@ fn element_wait_retries_transient_resolution_timeout() { errors: Mutex::new(vec![ErrorCode::Timeout]), }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, @@ -147,7 +167,7 @@ fn element_wait_passes_remaining_budget_to_resolver() { captured_ms: Mutex::new(vec![]), }; - let value = wait_for_element( + let value = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, @@ -168,7 +188,7 @@ fn element_wait_requires_timeout_aware_resolution() { let _guard = HomeGuard::new(); let snapshot_id = snapshot_with_one_ref(); - let err = wait_for_element( + let err = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, @@ -186,7 +206,7 @@ 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( + let err = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, @@ -213,7 +233,7 @@ fn element_wait_aborts_terminal_permission_error() { let _guard = HomeGuard::new(); let snapshot_id = snapshot_with_one_ref(); - let err = wait_for_element( + let err = wait_for_element_test( "@e1".into(), Some(snapshot_id), wait_predicate::ElementPredicate::Exists, diff --git a/crates/core/src/commands/wait_timeout.rs b/crates/core/src/commands/wait_timeout.rs index 2aa2cbd..20abc82 100644 --- a/crates/core/src/commands/wait_timeout.rs +++ b/crates/core/src/commands/wait_timeout.rs @@ -20,3 +20,57 @@ pub(crate) fn element( "last_observed": last_observed })))) } + +pub(crate) fn window( + title: &str, + timeout_ms: u64, + last_error: Option, +) -> Result { + Err(AppError::Adapter( + crate::error::AdapterError::timeout(format!( + "Window with title '{title}' not found within {timeout_ms}ms" + )) + .with_details(json!({ + "predicate": "window", + "title": title, + "timeout_ms": timeout_ms, + "last_error": last_error + })), + )) +} + +pub(crate) fn text( + text: &str, + timeout_ms: u64, + expected_count: Option, + last_error: Option, +) -> Result { + Err(AppError::Adapter( + crate::error::AdapterError::timeout(format!( + "Text '{text}' did not match within {timeout_ms}ms" + )) + .with_details(json!({ + "predicate": "text", + "text_chars": text.chars().count(), + "timeout_ms": timeout_ms, + "expected_count": expected_count, + "last_error": last_error + })), + )) +} + +pub(crate) fn notification( + app: Option<&String>, + text: Option<&String>, + timeout_ms: u64, +) -> Result { + Err(AppError::Adapter( + crate::error::AdapterError::timeout(format!("No new notification within {timeout_ms}ms")) + .with_details(json!({ + "predicate": "notification", + "timeout_ms": timeout_ms, + "app": app, + "text_chars": text.map(|text| text.chars().count()) + })), + )) +} diff --git a/crates/core/src/element_state.rs b/crates/core/src/element_state.rs new file mode 100644 index 0000000..64f57b5 --- /dev/null +++ b/crates/core/src/element_state.rs @@ -0,0 +1,10 @@ +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ElementState { + pub role: String, + #[serde(skip_serializing_if = "Vec::is_empty", default)] + pub states: Vec, + #[serde(skip_serializing_if = "Option::is_none")] + pub value: Option, +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index fcd891a..d39d10e 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -1,8 +1,14 @@ pub mod action; +pub mod action_request; +pub mod action_result; +pub mod action_step; +pub mod action_step_outcome; pub mod actionability; pub mod adapter; +pub mod capability; pub mod commands; pub mod context; +pub mod element_state; pub mod error; pub mod hints; pub mod node; @@ -26,14 +32,19 @@ pub mod trace; mod window_lookup; pub use action::{ - Action, ActionRequest, ActionResult, Direction, DragParams, ElementState, InteractionPolicy, - KeyCombo, Modifier, MouseButton, MouseEvent, MouseEventKind, Point, WindowOp, + Action, Direction, DragParams, KeyCombo, Modifier, MouseButton, MouseEvent, MouseEventKind, + Point, WindowOp, }; +pub use action_request::{ActionRequest, InteractionPolicy}; +pub use action_result::ActionResult; +pub use action_step::ActionStep; +pub use action_step_outcome::ActionStepOutcome; pub use adapter::{ ImageBuffer, ImageFormat, NativeHandle, PlatformAdapter, ScreenshotTarget, TreeOptions, WindowFilter, }; pub use context::CommandContext; +pub use element_state::ElementState; pub use error::{AdapterError, AppError, ErrorCode}; pub use node::{AccessibilityNode, AppInfo, Rect, WindowInfo}; pub use notification::{NotificationFilter, NotificationInfo}; diff --git a/crates/core/src/ref_action.rs b/crates/core/src/ref_action.rs index b12a6b4..9dba3f2 100644 --- a/crates/core/src/ref_action.rs +++ b/crates/core/src/ref_action.rs @@ -1,9 +1,6 @@ use crate::{ - action::{ActionRequest, ActionResult}, - actionability, - adapter::PlatformAdapter, - error::AdapterError, - refs::RefEntry, + action_request::ActionRequest, action_result::ActionResult, actionability, + adapter::PlatformAdapter, error::AdapterError, refs::RefEntry, }; pub fn execute_entry( diff --git a/crates/core/src/ref_alloc.rs b/crates/core/src/ref_alloc.rs index 5cb40f9..acbb97e 100644 --- a/crates/core/src/ref_alloc.rs +++ b/crates/core/src/ref_alloc.rs @@ -4,19 +4,6 @@ use crate::refs::{RefEntry, RefMap}; pub(crate) use crate::roles::INTERACTIVE_ROLES; -pub(crate) fn actions_for_role(role: &str) -> Vec { - match role { - "button" | "link" | "menuitem" | "tab" | "radiobutton" => vec!["Click".into()], - "textfield" | "incrementor" => vec!["Click".into(), "SetValue".into(), "SetFocus".into()], - "checkbox" => vec!["Click".into(), "Toggle".into()], - "combobox" => vec!["Click".into(), "Select".into()], - "treeitem" => vec!["Click".into(), "Expand".into(), "Collapse".into()], - "slider" => vec!["SetValue".into()], - "cell" => vec!["Click".into()], - _ => vec!["Click".into()], - } -} - pub(crate) fn ref_entry_from_node( node: &AccessibilityNode, pid: i32, @@ -36,7 +23,7 @@ pub(crate) fn ref_entry_from_node( bounds: node.bounds, bounds_hash: node.bounds.as_ref().map(|b| b.bounds_hash()), available_actions: if node.available_actions.is_empty() { - actions_for_role(&node.role) + crate::capability::defaults_for_role(&node.role) } else { node.available_actions.clone() }, diff --git a/crates/core/src/refs_store.rs b/crates/core/src/refs_store.rs index a122e1f..63bdb39 100644 --- a/crates/core/src/refs_store.rs +++ b/crates/core/src/refs_store.rs @@ -13,6 +13,7 @@ const MAX_SAVED_SNAPSHOTS: usize = 512; #[derive(Debug, Clone)] pub struct RefStore { base_dir: PathBuf, + allow_legacy_migration: bool, } impl RefStore { @@ -30,10 +31,12 @@ impl RefStore { .join(".agent-desktop") .join("sessions") .join(session_id), + allow_legacy_migration: false, }); } Ok(Self { base_dir: home.join(".agent-desktop"), + allow_legacy_migration: true, }) } @@ -191,6 +194,9 @@ impl RefStore { } fn migrate_legacy_latest(&self) -> Result, AppError> { + if !self.allow_legacy_migration { + return Ok(None); + } self.with_write_lock(|| { if let Ok(id) = std::fs::read_to_string(self.latest_path()) { let id = id.trim(); @@ -212,3 +218,7 @@ impl RefStore { }) } } + +#[cfg(test)] +#[path = "refs_store_tests.rs"] +mod tests; diff --git a/crates/core/src/refs_store_tests.rs b/crates/core/src/refs_store_tests.rs new file mode 100644 index 0000000..e4a2907 --- /dev/null +++ b/crates/core/src/refs_store_tests.rs @@ -0,0 +1,132 @@ +use super::*; +use crate::{ + adapter::SnapshotSurface, + refs::{RefEntry, RefMap}, + refs_test_support::HomeGuard, +}; + +fn entry(name: &str) -> RefEntry { + RefEntry { + pid: 7, + role: "button".into(), + name: Some(name.into()), + value: None, + description: None, + states: vec![], + bounds: None, + bounds_hash: Some(42), + available_actions: vec![crate::capability::CLICK.into()], + source_app: Some("TestApp".into()), + source_window_id: None, + source_window_title: Some("Test Window".into()), + source_surface: SnapshotSurface::Window, + root_ref: None, + path_is_absolute: false, + path: smallvec::SmallVec::new(), + } +} + +fn map_with(name: &str) -> RefMap { + let mut map = RefMap::new(); + map.allocate(entry(name)); + map +} + +#[test] +fn snapshot_roundtrip_updates_latest_pointer() { + let _guard = HomeGuard::new(); + let store = RefStore::new().unwrap(); + let map = map_with("Send"); + + let snapshot_id = store.save_new_snapshot(&map).unwrap(); + + assert_eq!( + store.latest_snapshot_id().as_deref(), + Some(snapshot_id.as_str()) + ); + assert_eq!(store.load(Some(&snapshot_id)).unwrap().len(), 1); + assert_eq!(store.load(None).unwrap().len(), 1); +} + +#[test] +fn sessions_are_isolated_from_default_store() { + let _guard = HomeGuard::new(); + let default_store = RefStore::new().unwrap(); + let session_a = RefStore::for_session(Some("agent-a")).unwrap(); + let session_b = RefStore::for_session(Some("agent-b")).unwrap(); + + let default_id = default_store + .save_new_snapshot(&map_with("Default")) + .unwrap(); + let session_id = session_a.save_new_snapshot(&map_with("Session A")).unwrap(); + + assert_eq!(default_store.load(None).unwrap().len(), 1); + assert_eq!( + default_store + .load(Some(&default_id)) + .unwrap() + .get("@e1") + .unwrap() + .name + .as_deref(), + Some("Default") + ); + assert_eq!( + session_a + .load(Some(&session_id)) + .unwrap() + .get("@e1") + .unwrap() + .name + .as_deref(), + Some("Session A") + ); + assert!(session_b.load(None).is_err()); + assert_ne!( + default_store.latest_snapshot_id(), + session_a.latest_snapshot_id() + ); +} + +#[test] +fn save_existing_snapshot_does_not_promote_latest_pointer() { + let _guard = HomeGuard::new(); + let store = RefStore::new().unwrap(); + + let mut first = map_with("First"); + let first_id = store.save_new_snapshot(&first).unwrap(); + let second_id = store.save_new_snapshot(&map_with("Second")).unwrap(); + + first.allocate(entry("First Child")); + store.save_existing_snapshot(&first_id, &first).unwrap(); + + assert_eq!( + store.latest_snapshot_id().as_deref(), + Some(second_id.as_str()) + ); + assert_eq!(store.load(Some(&first_id)).unwrap().len(), 2); +} + +#[test] +fn default_store_migrates_legacy_latest_refmap() { + let _guard = HomeGuard::new(); + map_with("Legacy").save().unwrap(); + + let store = RefStore::new().unwrap(); + let loaded = store.load_latest().unwrap(); + + assert_eq!(loaded.len(), 1); + assert!(store.latest_snapshot_id().is_some()); +} + +#[test] +fn session_store_does_not_migrate_global_legacy_refmap() { + let _guard = HomeGuard::new(); + map_with("Legacy").save().unwrap(); + + let store = RefStore::for_session(Some("fresh-agent")).unwrap(); + let err = store.load(None).unwrap_err(); + + assert_eq!(err.code(), "SNAPSHOT_NOT_FOUND"); + assert!(store.latest_snapshot_id().is_none()); +} diff --git a/crates/core/src/refs_tests.rs b/crates/core/src/refs_tests.rs index e620dab..ab8c456 100644 --- a/crates/core/src/refs_tests.rs +++ b/crates/core/src/refs_tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::{refs_store::RefStore, refs_test_support::HomeGuard}; +use crate::refs_test_support::HomeGuard; fn entry(role: &str, name: Option<&str>) -> RefEntry { RefEntry { @@ -208,106 +208,6 @@ fn test_save_load_roundtrip_with_home_override() { assert_eq!(entry.name.as_deref(), Some("Send")); } -#[test] -fn test_refstore_snapshot_roundtrip_and_latest_pointer() { - let _guard = HomeGuard::new(); - let store = RefStore::new().unwrap(); - let mut map = RefMap::new(); - map.allocate(RefEntry { - pid: 7, - role: "button".into(), - name: Some("Send".into()), - value: None, - description: None, - states: vec![], - bounds: None, - bounds_hash: Some(42), - available_actions: vec!["Click".into()], - source_app: Some("TestApp".into()), - source_window_id: None, - source_window_title: Some("Test Window".into()), - source_surface: crate::adapter::SnapshotSurface::Window, - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }); - - let snapshot_id = store.save_new_snapshot(&map).unwrap(); - - assert_eq!( - store.latest_snapshot_id().as_deref(), - Some(snapshot_id.as_str()) - ); - assert_eq!(store.load(Some(&snapshot_id)).unwrap().len(), 1); - assert_eq!(store.load(None).unwrap().len(), 1); -} - -#[test] -fn test_refstore_sessions_are_isolated_from_default_store() { - let _guard = HomeGuard::new(); - let default_store = RefStore::new().unwrap(); - let session_a = RefStore::for_session(Some("agent-a")).unwrap(); - let session_b = RefStore::for_session(Some("agent-b")).unwrap(); - - let mut default_map = RefMap::new(); - default_map.allocate(entry("button", Some("Default"))); - let default_id = default_store.save_new_snapshot(&default_map).unwrap(); - - let mut session_map = RefMap::new(); - session_map.allocate(entry("button", Some("Session A"))); - let session_id = session_a.save_new_snapshot(&session_map).unwrap(); - - assert_eq!(default_store.load(None).unwrap().len(), 1); - assert_eq!( - default_store - .load(Some(&default_id)) - .unwrap() - .get("@e1") - .unwrap() - .name - .as_deref(), - Some("Default") - ); - assert_eq!( - session_a - .load(Some(&session_id)) - .unwrap() - .get("@e1") - .unwrap() - .name - .as_deref(), - Some("Session A") - ); - assert!(session_b.load(None).is_err()); - assert_ne!( - default_store.latest_snapshot_id(), - session_a.latest_snapshot_id() - ); -} - -#[test] -fn test_save_existing_snapshot_does_not_promote_latest_pointer() { - let _guard = HomeGuard::new(); - let store = RefStore::new().unwrap(); - - let mut first = RefMap::new(); - first.allocate(entry("button", Some("First"))); - let first_id = store.save_new_snapshot(&first).unwrap(); - - let mut second = RefMap::new(); - second.allocate(entry("button", Some("Second"))); - let second_id = store.save_new_snapshot(&second).unwrap(); - - first.allocate(entry("button", Some("First Child"))); - store.save_existing_snapshot(&first_id, &first).unwrap(); - - assert_eq!( - store.latest_snapshot_id().as_deref(), - Some(second_id.as_str()) - ); - assert_eq!(store.load(Some(&first_id)).unwrap().len(), 2); -} - #[test] fn test_snapshot_ids_are_compact_and_valid() { let id = new_snapshot_id(); @@ -334,20 +234,6 @@ fn test_new_snapshot_id_passes_validation() { } } -#[test] -fn test_refstore_migrates_legacy_latest_refmap() { - let _guard = HomeGuard::new(); - let mut map = RefMap::new(); - map.allocate(entry("button", Some("Legacy"))); - map.save().unwrap(); - - let store = RefStore::new().unwrap(); - let loaded = store.load_latest().unwrap(); - - assert_eq!(loaded.len(), 1); - assert!(store.latest_snapshot_id().is_some()); -} - #[test] fn test_validate_snapshot_id_rejects_bad_values() { assert!(validate_snapshot_id("").is_err()); diff --git a/crates/core/src/snapshot_ref_tests.rs b/crates/core/src/snapshot_ref_tests.rs index 13ab122..67c4796 100644 --- a/crates/core/src/snapshot_ref_tests.rs +++ b/crates/core/src/snapshot_ref_tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::action::ActionRequest; +use crate::action_request::ActionRequest; use crate::adapter::{NativeHandle, PlatformAdapter}; use crate::error::AdapterError; use crate::node::AccessibilityNode; @@ -86,7 +86,7 @@ impl PlatformAdapter for StubAdapter { &self, _handle: &NativeHandle, _request: ActionRequest, - ) -> Result { + ) -> Result { Err(AdapterError::not_supported("execute_action")) } } diff --git a/crates/ffi/src/actions/execute.rs b/crates/ffi/src/actions/execute.rs index af8c667..5199bf6 100644 --- a/crates/ffi/src/actions/execute.rs +++ b/crates/ffi/src/actions/execute.rs @@ -4,7 +4,7 @@ use crate::actions::result::action_result_to_c; use crate::error::{self, AdResult}; use crate::ffi_try::trap_panic; use crate::types::{AdAction, AdActionResult, AdNativeHandle, AdPolicyKind, AdRefEntry}; -use agent_desktop_core::{action::ActionRequest, adapter::NativeHandle}; +use agent_desktop_core::{action_request::ActionRequest, adapter::NativeHandle}; /// # Safety /// diff --git a/crates/ffi/src/actions/result.rs b/crates/ffi/src/actions/result.rs index 9baeb4b..e406cf4 100644 --- a/crates/ffi/src/actions/result.rs +++ b/crates/ffi/src/actions/result.rs @@ -1,6 +1,6 @@ use crate::convert::string::{free_c_string, opt_string_to_c, string_to_c_lossy}; use crate::types::{AdActionResult, AdElementState}; -use agent_desktop_core::action::ActionResult as CoreActionResult; +use agent_desktop_core::action_result::ActionResult as CoreActionResult; use std::ptr; pub(crate) fn action_result_to_c(r: &CoreActionResult) -> AdActionResult { @@ -78,7 +78,7 @@ pub unsafe extern "C" fn ad_free_action_result(result: *mut AdActionResult) { mod tests { use super::*; use crate::convert::string::c_to_string; - use agent_desktop_core::action::ElementState; + use agent_desktop_core::element_state::ElementState; #[test] fn test_action_result_to_c_with_state() { diff --git a/crates/macos/src/actions/chain.rs b/crates/macos/src/actions/chain.rs index eb2259e..6fe5d94 100644 --- a/crates/macos/src/actions/chain.rs +++ b/crates/macos/src/actions/chain.rs @@ -1,5 +1,5 @@ -use agent_desktop_core::action::{ActionStep, InteractionPolicy}; use agent_desktop_core::error::{AdapterError, ErrorCode}; +use agent_desktop_core::{action_request::InteractionPolicy, action_step::ActionStep}; use crate::actions::discovery::ElementCaps; use crate::tree::AXElement; diff --git a/crates/macos/src/actions/chain_defs.rs b/crates/macos/src/actions/chain_defs.rs index 92f41de..33e5a9c 100644 --- a/crates/macos/src/actions/chain_defs.rs +++ b/crates/macos/src/actions/chain_defs.rs @@ -10,7 +10,7 @@ mod imp { discovery::ElementCaps, }; use crate::tree::AXElement; - use agent_desktop_core::action::{InteractionPolicy, MouseButton}; + use agent_desktop_core::{action::MouseButton, action_request::InteractionPolicy}; pub(crate) static CLICK_CHAIN: ChainDef = ChainDef { pre_scroll: true, diff --git a/crates/macos/src/actions/chain_steps.rs b/crates/macos/src/actions/chain_steps.rs index 2cdd98f..89e0130 100644 --- a/crates/macos/src/actions/chain_steps.rs +++ b/crates/macos/src/actions/chain_steps.rs @@ -36,7 +36,7 @@ mod imp { return ax_helpers::try_ax_action_retried_or_err(el, "AXPress"); } tracing::debug!("verified_press: native element in container, using AXPress"); - let selected_before = crate::tree::element::copy_bool_attr(el, "AXSelected"); + let selected_before = crate::tree::copy_bool_attr(el, "AXSelected"); if !ax_helpers::try_ax_action_retried_or_err(el, "AXPress")? { return Ok(false); } @@ -44,7 +44,7 @@ mod imp { return Ok(true); } std::thread::sleep(std::time::Duration::from_millis(50)); - let selected_after = crate::tree::element::copy_bool_attr(el, "AXSelected"); + let selected_after = crate::tree::copy_bool_attr(el, "AXSelected"); if selected_after == Some(true) { return Ok(true); } diff --git a/crates/macos/src/actions/dispatch.rs b/crates/macos/src/actions/dispatch.rs index 48eda05..f158918 100644 --- a/crates/macos/src/actions/dispatch.rs +++ b/crates/macos/src/actions/dispatch.rs @@ -1,8 +1,8 @@ use agent_desktop_core::{ - action::{ - Action, ActionRequest, ActionResult, InteractionPolicy, MouseButton, MouseEvent, - MouseEventKind, Point, - }, + action::{Action, MouseButton, MouseEvent, MouseEventKind, Point}, + action_request::{ActionRequest, InteractionPolicy}, + action_result::ActionResult, + element_state::ElementState, error::{AdapterError, ErrorCode}, }; @@ -63,7 +63,7 @@ mod imp { request: &ActionRequest, ) -> Result { let action = &request.action; - let label = action_label(action); + let label = action.name(); let mut steps = Vec::new(); tracing::debug!("action: perform {label}"); match action { @@ -109,7 +109,7 @@ mod imp { Action::SetValue(val) => { let caps = discovery::discover(el); let ctx = ChainContext { - dynamic_value: Some(val), + dynamic_value: Some(val.as_str()), deadline: None, }; steps.extend(execute_chain( @@ -137,7 +137,7 @@ mod imp { } Action::TypeText(text) => { - crate::actions::type_text::execute_type(el, text, request.policy)?; + crate::actions::type_text::execute_type(el, text.as_str(), request.policy)?; } Action::PressKey(combo) => { @@ -175,7 +175,7 @@ mod imp { } Action::Select(value) => { - crate::actions::extras::select_value(el, value)?; + crate::actions::extras::select_value(el, value.as_str())?; } Action::Scroll(direction, amount) => { @@ -237,7 +237,7 @@ mod imp { } _ => { - return Err(AdapterError::not_supported(&label)); + return Err(AdapterError::not_supported(label)); } } @@ -249,10 +249,7 @@ mod imp { Ok(result) } - fn verify_post_state( - action: &Action, - state: &agent_desktop_core::action::ElementState, - ) -> Result<(), AdapterError> { + fn verify_post_state(action: &Action, state: &ElementState) -> Result<(), AdapterError> { if matches!(action, Action::Clear) && state .value @@ -282,7 +279,7 @@ mod imp { #[cfg(test)] mod tests { use super::*; - use agent_desktop_core::action::ElementState; + use agent_desktop_core::element_state::ElementState; #[test] fn clear_post_state_fails_when_value_remains() { @@ -331,31 +328,3 @@ pub(crate) use imp::perform_action; #[cfg(target_os = "macos")] pub(crate) use imp::{ax_press_or_fail, click_via_bounds}; - -fn action_label(action: &Action) -> String { - match action { - Action::Click => "click", - Action::DoubleClick => "double_click", - Action::RightClick => "right_click", - Action::TripleClick => "triple_click", - Action::SetValue(_) => "set_value", - Action::SetFocus => "set_focus", - Action::Expand => "expand", - Action::Collapse => "collapse", - Action::Select(_) => "select", - Action::Toggle => "toggle", - Action::Check => "check", - Action::Uncheck => "uncheck", - Action::Scroll(_, _) => "scroll", - Action::ScrollTo => "scroll_to", - Action::PressKey(_) => "press_key", - Action::KeyDown(_) => "key_down", - Action::KeyUp(_) => "key_up", - Action::TypeText(_) => "type_text", - Action::Clear => "clear", - Action::Hover => "hover", - Action::Drag(_) => "drag", - _ => "unknown", - } - .to_string() -} diff --git a/crates/macos/src/actions/post_state.rs b/crates/macos/src/actions/post_state.rs index 5cb9c46..6b8f487 100644 --- a/crates/macos/src/actions/post_state.rs +++ b/crates/macos/src/actions/post_state.rs @@ -1,7 +1,4 @@ -use agent_desktop_core::{ - action::{Action, ElementState}, - adapter::LiveElement, -}; +use agent_desktop_core::{action::Action, adapter::LiveElement, element_state::ElementState}; #[cfg(target_os = "macos")] pub(crate) fn read_post_state( diff --git a/crates/macos/src/actions/scroll.rs b/crates/macos/src/actions/scroll.rs index 9d2934e..722f323 100644 --- a/crates/macos/src/actions/scroll.rs +++ b/crates/macos/src/actions/scroll.rs @@ -1,5 +1,8 @@ #[cfg(target_os = "macos")] -use agent_desktop_core::error::{AdapterError, ErrorCode}; +use agent_desktop_core::{ + action_request::InteractionPolicy, + error::{AdapterError, ErrorCode}, +}; #[cfg(target_os = "macos")] use crate::tree::AXElement; @@ -9,7 +12,7 @@ pub(crate) fn ax_scroll( el: &AXElement, direction: &agent_desktop_core::action::Direction, amount: u32, - policy: agent_desktop_core::action::InteractionPolicy, + policy: InteractionPolicy, ) -> Result<(), AdapterError> { use accessibility_sys::{ AXUIElementPerformAction, AXUIElementSetAttributeValue, kAXErrorSuccess, diff --git a/crates/macos/src/actions/toggle_state.rs b/crates/macos/src/actions/toggle_state.rs index 25cc346..46d4cec 100644 --- a/crates/macos/src/actions/toggle_state.rs +++ b/crates/macos/src/actions/toggle_state.rs @@ -1,5 +1,5 @@ use agent_desktop_core::{ - action::InteractionPolicy, + action_request::InteractionPolicy, error::{AdapterError, ErrorCode}, }; diff --git a/crates/macos/src/actions/type_text.rs b/crates/macos/src/actions/type_text.rs index ee08987..7a82685 100644 --- a/crates/macos/src/actions/type_text.rs +++ b/crates/macos/src/actions/type_text.rs @@ -1,6 +1,6 @@ #[cfg(target_os = "macos")] use agent_desktop_core::{ - action::InteractionPolicy, + action_request::InteractionPolicy, error::{AdapterError, ErrorCode}, }; @@ -83,7 +83,7 @@ fn type_via_ax_value(el: &AXElement, text: &str) -> Result<(), AdapterError> { pub(crate) fn execute_type( _el: &crate::tree::AXElement, _text: &str, - _policy: agent_desktop_core::action::InteractionPolicy, + _policy: agent_desktop_core::action_request::InteractionPolicy, ) -> Result<(), agent_desktop_core::error::AdapterError> { Err(agent_desktop_core::error::AdapterError::new( agent_desktop_core::error::ErrorCode::PlatformNotSupported, diff --git a/crates/macos/src/adapter.rs b/crates/macos/src/adapter.rs index b2e59fd..2d1e41c 100644 --- a/crates/macos/src/adapter.rs +++ b/crates/macos/src/adapter.rs @@ -1,10 +1,13 @@ use agent_desktop_core::{ PermissionReport, - action::{ActionRequest, ActionResult, DragParams, ElementState, MouseEvent, WindowOp}, + action::{DragParams, MouseEvent, WindowOp}, + action_request::ActionRequest, + action_result::ActionResult, adapter::{ ImageBuffer, LiveElement, NativeHandle, PlatformAdapter, ScreenshotTarget, SnapshotSurface, TreeOptions, WindowFilter, }, + element_state::ElementState, error::AdapterError, node::{AccessibilityNode, AppInfo, Rect, SurfaceInfo, WindowInfo}, notification::{NotificationFilter, NotificationIdentity, NotificationInfo}, @@ -144,7 +147,7 @@ impl PlatformAdapter for MacOSAdapter { &self, app_name: &str, combo: &agent_desktop_core::action::KeyCombo, - ) -> Result { + ) -> Result { crate::system::key_dispatch::press_for_app_impl(app_name, combo) } diff --git a/crates/macos/src/notifications/actions.rs b/crates/macos/src/notifications/actions.rs index 0589997..8203b6d 100644 --- a/crates/macos/src/notifications/actions.rs +++ b/crates/macos/src/notifications/actions.rs @@ -1,5 +1,5 @@ use agent_desktop_core::{ - action::ActionResult, + action_result::ActionResult, error::{AdapterError, ErrorCode}, notification::{NotificationFilter, NotificationIdentity, NotificationInfo}, }; diff --git a/crates/macos/src/system/key_dispatch.rs b/crates/macos/src/system/key_dispatch.rs index ff28348..05237d8 100644 --- a/crates/macos/src/system/key_dispatch.rs +++ b/crates/macos/src/system/key_dispatch.rs @@ -1,7 +1,4 @@ -use agent_desktop_core::{ - action::{ActionResult, KeyCombo}, - error::AdapterError, -}; +use agent_desktop_core::{action::KeyCombo, action_result::ActionResult, error::AdapterError}; #[cfg(target_os = "macos")] use agent_desktop_core::{action::Modifier, adapter::WindowFilter}; diff --git a/crates/macos/src/tree/action_list.rs b/crates/macos/src/tree/action_list.rs index a10ce38..952499b 100644 --- a/crates/macos/src/tree/action_list.rs +++ b/crates/macos/src/tree/action_list.rs @@ -3,6 +3,7 @@ use super::{ capabilities::{copy_action_names, is_attr_settable}, copy_first_element_attr, }; +use agent_desktop_core::capability; #[cfg(target_os = "macos")] use accessibility_sys::{kAXFocusedAttribute, kAXValueAttribute}; @@ -14,33 +15,33 @@ pub(crate) fn platform_available_actions(el: &AXElement, role: &str) -> Vec Option { + let cf_attr = CFString::new(attr); + let mut value: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) + }; + if err != kAXErrorSuccess || value.is_null() { + return None; + } + let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; + cf_type.downcast::().map(|s| s.to_string()) + } + + pub fn set_messaging_timeout(el: &AXElement, timeout: std::time::Duration) { + if el.0.is_null() { + return; + } + let seconds = timeout.as_secs_f32().clamp(0.001, 2.0); + unsafe { AXUIElementSetMessagingTimeout(el.0, seconds) }; + } + + pub fn copy_value_typed(el: &AXElement) -> Option { + let cf_attr = CFString::new(kAXValueAttribute); + let mut val_ref: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut val_ref) + }; + if err != kAXErrorSuccess || val_ref.is_null() { + return None; + } + let cf = unsafe { CFType::wrap_under_create_rule(val_ref) }; + if let Some(s) = cf.downcast::() { + return Some(s.to_string()); + } + if let Some(b) = cf.downcast::() { + return Some(bool::from(b).to_string()); + } + if let Some(n) = cf.downcast::() { + if let Some(i) = n.to_i64() { + return Some(i.to_string()); + } + if let Some(f) = n.to_f64() { + return Some(format!("{:.2}", f)); + } + } + None + } + + pub fn copy_bool_attr(el: &AXElement, attr: &str) -> Option { + let cf_attr = CFString::new(attr); + let mut value: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) + }; + if err != kAXErrorSuccess || value.is_null() { + return None; + } + let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; + cf_type.downcast::().map(|b| b.into()) + } + + pub fn copy_i64_attr(el: &AXElement, attr: &str) -> Option { + let cf_attr = CFString::new(attr); + let mut value: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) + }; + if err != kAXErrorSuccess || value.is_null() { + return None; + } + let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; + cf_type.downcast::().and_then(|n| n.to_i64()) + } + + pub fn copy_ax_array(el: &AXElement, attr: &str) -> Option> { + let cf_attr = CFString::new(attr); + let mut value: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) + }; + if err != kAXErrorSuccess || value.is_null() { + return None; + } + let arr = created_cf_array(value)?; + Some(ax_array_items(arr)) + } + + pub fn copy_ax_array_prefix( + el: &AXElement, + attr: &str, + max_values: usize, + ) -> Option> { + if max_values == 0 { + return Some(Vec::new()); + } + let cf_attr = CFString::new(attr); + let mut value: core_foundation_sys::array::CFArrayRef = std::ptr::null(); + let err = unsafe { + AXUIElementCopyAttributeValues( + el.0, + cf_attr.as_concrete_TypeRef(), + 0, + max_values as core_foundation_sys::base::CFIndex, + &mut value, + ) + }; + if err != kAXErrorSuccess || value.is_null() { + return None; + } + let arr = created_cf_array(value as CFTypeRef)?; + Some(ax_array_items(arr)) + } + + pub fn copy_element_attr(el: &AXElement, attr: &str) -> Option { + let cf_attr = CFString::new(attr); + let mut value: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) + }; + if err != kAXErrorSuccess || value.is_null() { + return None; + } + ax_value::created_ax_element(value) + } + + pub fn copy_first_element_attr(el: &AXElement, attrs: &[&str]) -> Option { + if attrs.is_empty() { + return None; + } + let cf_names: Vec = attrs.iter().map(|attr| CFString::new(attr)).collect(); + let cf_refs: Vec<_> = cf_names.iter().map(|s| s.as_concrete_TypeRef()).collect(); + let names_arr = CFArray::from_copyable(&cf_refs); + let mut result_ref: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyMultipleAttributeValues( + el.0, + names_arr.as_concrete_TypeRef(), + 0, + &mut result_ref as *mut _ as *mut _, + ) + }; + if !result_ref.is_null() { + let arr = created_cf_array(result_ref); + if err == kAXErrorSuccess + && let Some(arr) = arr + { + return arr + .into_iter() + .find_map(|item| ax_value::retained_ax_element(&item)); + } + } + attrs.iter().find_map(|attr| copy_element_attr(el, attr)) + } + + fn ax_array_items(arr: CFArray) -> Vec { + arr.into_iter() + .filter_map(|item| ax_value::retained_ax_element(&item)) + .collect() + } +} + +#[cfg(not(target_os = "macos"))] +mod imp { + use crate::tree::ax_element::AXElement; + + pub fn copy_ax_array(_el: &AXElement, _attr: &str) -> Option> { + None + } + + pub fn copy_ax_array_prefix( + _el: &AXElement, + _attr: &str, + _max_values: usize, + ) -> Option> { + None + } + + pub fn copy_string_attr(_el: &AXElement, _attr: &str) -> Option { + None + } + + pub fn copy_bool_attr(_el: &AXElement, _attr: &str) -> Option { + None + } + + pub fn copy_i64_attr(_el: &AXElement, _attr: &str) -> Option { + None + } + + pub fn copy_element_attr(_el: &AXElement, _attr: &str) -> Option { + None + } + + pub fn copy_first_element_attr(_el: &AXElement, _attrs: &[&str]) -> Option { + None + } + + pub fn copy_value_typed(_el: &AXElement) -> Option { + None + } + + pub fn set_messaging_timeout(_el: &AXElement, _timeout: std::time::Duration) {} +} + +pub(crate) use imp::{ + copy_ax_array, copy_ax_array_prefix, copy_bool_attr, copy_element_attr, + copy_first_element_attr, copy_i64_attr, copy_string_attr, copy_value_typed, + set_messaging_timeout, +}; diff --git a/crates/macos/src/tree/builder.rs b/crates/macos/src/tree/builder.rs index 5442826..017f464 100644 --- a/crates/macos/src/tree/builder.rs +++ b/crates/macos/src/tree/builder.rs @@ -1,13 +1,14 @@ +use agent_desktop_core::capability; use agent_desktop_core::node::AccessibilityNode; use rustc_hash::FxHashSet; use super::AXElement; use super::action_list::platform_available_actions; +use super::attributes::{copy_ax_array, copy_ax_array_prefix, copy_bool_attr, copy_string_attr}; use super::build_context::TreeBuildContext; use super::capabilities::same_element; use super::element::{ - ABSOLUTE_MAX_DEPTH, child_attributes, copy_ax_array, copy_ax_array_prefix, copy_bool_attr, - copy_string_attr, count_children, element_for_pid, fetch_node_attrs, + ABSOLUTE_MAX_DEPTH, child_attributes, count_children, element_for_pid, fetch_node_attrs, }; #[cfg(target_os = "macos")] @@ -136,7 +137,7 @@ pub fn build_subtree( let value = redact_secure_value(attrs.role.as_deref(), attrs.value); let is_promoted_item = promoted_label.is_some(); let available_actions = if is_promoted_item { - vec!["Click".into(), "RightClick".into()] + vec![capability::CLICK.into(), capability::RIGHT_CLICK.into()] } else { platform_available_actions(el, &role) }; diff --git a/crates/macos/src/tree/element.rs b/crates/macos/src/tree/element.rs index 112bd69..2aceab5 100644 --- a/crates/macos/src/tree/element.rs +++ b/crates/macos/src/tree/element.rs @@ -17,13 +17,12 @@ mod imp { cf_type::created_cf_array, tree::{ NodeAttrs, + attributes::{copy_ax_array, copy_bool_attr, copy_string_attr, copy_value_typed}, ax_element::AXElement, - ax_value, node_attrs::{parse_bool_attr, parse_enabled}, }, }; use accessibility_sys::{ - AXUIElementCopyAttributeValue, AXUIElementCopyAttributeValues, AXUIElementCopyMultipleAttributeValues, AXUIElementCreateApplication, AXUIElementGetAttributeValueCount, AXUIElementSetMessagingTimeout, kAXDescriptionAttribute, kAXEnabledAttribute, kAXErrorSuccess, kAXRoleAttribute, kAXTitleAttribute, @@ -31,7 +30,7 @@ mod imp { }; use core_foundation::{ array::CFArray, - base::{CFType, CFTypeRef, TCFType}, + base::{CFTypeRef, TCFType}, boolean::CFBoolean, number::CFNumber, string::CFString, @@ -159,152 +158,6 @@ mod imp { }) } - pub fn copy_string_attr(el: &AXElement, attr: &str) -> Option { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; - cf_type.downcast::().map(|s| s.to_string()) - } - - pub fn copy_value_typed(el: &AXElement) -> Option { - let cf_attr = CFString::new(kAXValueAttribute); - let mut val_ref: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut val_ref) - }; - if err != kAXErrorSuccess || val_ref.is_null() { - return None; - } - let cf = unsafe { CFType::wrap_under_create_rule(val_ref) }; - if let Some(s) = cf.downcast::() { - return Some(s.to_string()); - } - if let Some(b) = cf.downcast::() { - return Some(bool::from(b).to_string()); - } - if let Some(n) = cf.downcast::() { - if let Some(i) = n.to_i64() { - return Some(i.to_string()); - } - if let Some(f) = n.to_f64() { - return Some(format!("{:.2}", f)); - } - } - None - } - - pub fn copy_bool_attr(el: &AXElement, attr: &str) -> Option { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; - cf_type.downcast::().map(|b| b.into()) - } - - pub fn copy_i64_attr(el: &AXElement, attr: &str) -> Option { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - let cf_type = unsafe { CFType::wrap_under_create_rule(value) }; - cf_type.downcast::().and_then(|n| n.to_i64()) - } - - pub fn copy_ax_array(el: &AXElement, attr: &str) -> Option> { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - let arr = created_cf_array(value)?; - Some(ax_array_items(arr)) - } - - pub fn copy_ax_array_prefix( - el: &AXElement, - attr: &str, - max_values: usize, - ) -> Option> { - if max_values == 0 { - return Some(Vec::new()); - } - let cf_attr = CFString::new(attr); - let mut value: core_foundation_sys::array::CFArrayRef = std::ptr::null(); - let err = unsafe { - AXUIElementCopyAttributeValues( - el.0, - cf_attr.as_concrete_TypeRef(), - 0, - max_values as core_foundation_sys::base::CFIndex, - &mut value, - ) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - let arr = created_cf_array(value as CFTypeRef)?; - Some(ax_array_items(arr)) - } - - pub fn copy_element_attr(el: &AXElement, attr: &str) -> Option { - let cf_attr = CFString::new(attr); - let mut value: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyAttributeValue(el.0, cf_attr.as_concrete_TypeRef(), &mut value) - }; - if err != kAXErrorSuccess || value.is_null() { - return None; - } - ax_value::created_ax_element(value) - } - - pub fn copy_first_element_attr(el: &AXElement, attrs: &[&str]) -> Option { - if attrs.is_empty() { - return None; - } - let cf_names: Vec = attrs.iter().map(|attr| CFString::new(attr)).collect(); - let cf_refs: Vec<_> = cf_names.iter().map(|s| s.as_concrete_TypeRef()).collect(); - let names_arr = CFArray::from_copyable(&cf_refs); - let mut result_ref: CFTypeRef = std::ptr::null_mut(); - let err = unsafe { - AXUIElementCopyMultipleAttributeValues( - el.0, - names_arr.as_concrete_TypeRef(), - 0, - &mut result_ref as *mut _ as *mut _, - ) - }; - if !result_ref.is_null() { - let arr = created_cf_array(result_ref); - if err == kAXErrorSuccess - && let Some(arr) = arr - { - return arr - .into_iter() - .find_map(|item| ax_value::retained_ax_element(&item)); - } - } - attrs.iter().find_map(|attr| copy_element_attr(el, attr)) - } - pub fn count_children(element: &AXElement, ax_role: Option<&str>) -> u32 { unsafe { for attr_name in child_attributes(ax_role) { @@ -325,12 +178,6 @@ mod imp { 0 } } - - fn ax_array_items(arr: CFArray) -> Vec { - arr.into_iter() - .filter_map(|item| ax_value::retained_ax_element(&item)) - .collect() - } } #[cfg(not(target_os = "macos"))] @@ -341,38 +188,6 @@ mod imp { AXElement(std::ptr::null()) } - pub fn copy_ax_array(_el: &AXElement, _attr: &str) -> Option> { - None - } - - pub fn copy_ax_array_prefix( - _el: &AXElement, - _attr: &str, - _max_values: usize, - ) -> Option> { - None - } - - pub fn copy_string_attr(_el: &AXElement, _attr: &str) -> Option { - None - } - - pub fn copy_bool_attr(_el: &AXElement, _attr: &str) -> Option { - None - } - - pub fn copy_i64_attr(_el: &AXElement, _attr: &str) -> Option { - None - } - - pub fn copy_element_attr(_el: &AXElement, _attr: &str) -> Option { - None - } - - pub fn copy_first_element_attr(_el: &AXElement, _attrs: &[&str]) -> Option { - None - } - pub fn count_children(_element: &AXElement, _ax_role: Option<&str>) -> u32 { 0 } @@ -381,10 +196,6 @@ mod imp { None } - pub fn copy_value_typed(_el: &AXElement) -> Option { - None - } - pub fn fetch_node_attrs(_el: &AXElement) -> NodeAttrs { NodeAttrs { enabled: true, @@ -393,8 +204,4 @@ mod imp { } } -pub use imp::{ - copy_ax_array, copy_ax_array_prefix, copy_bool_attr, copy_element_attr, - copy_first_element_attr, copy_i64_attr, copy_string_attr, copy_value_typed, count_children, - element_for_pid, fetch_node_attrs, resolve_element_name, -}; +pub use imp::{count_children, element_for_pid, fetch_node_attrs, resolve_element_name}; diff --git a/crates/macos/src/tree/mod.rs b/crates/macos/src/tree/mod.rs index 5e82169..370aacf 100644 --- a/crates/macos/src/tree/mod.rs +++ b/crates/macos/src/tree/mod.rs @@ -1,4 +1,5 @@ pub mod action_list; +pub(crate) mod attributes; pub mod ax_element; pub(crate) mod ax_value; pub mod build_context; @@ -10,19 +11,21 @@ pub(crate) mod element_dedupe; pub(crate) mod node_attrs; pub mod resolve; mod resolve_bounds; +mod resolve_deadline; mod resolve_identity; mod resolve_roots; pub mod roles; pub mod surfaces; +pub(crate) use attributes::{ + copy_ax_array, copy_bool_attr, copy_element_attr, copy_first_element_attr, copy_i64_attr, + copy_string_attr, copy_value_typed, +}; pub use ax_element::AXElement; pub use build_context::TreeBuildContext; pub use builder::{build_subtree, window_element_for}; pub use capabilities::same_element; -pub use element::{ - copy_ax_array, copy_bool_attr, copy_element_attr, copy_first_element_attr, copy_i64_attr, - copy_string_attr, copy_value_typed, element_for_pid, resolve_element_name, -}; +pub use element::{element_for_pid, resolve_element_name}; pub use element_bounds::read_bounds; pub(crate) use node_attrs::NodeAttrs; pub use surfaces::{ diff --git a/crates/macos/src/tree/resolve.rs b/crates/macos/src/tree/resolve.rs index bf61cf5..643a75c 100644 --- a/crates/macos/src/tree/resolve.rs +++ b/crates/macos/src/tree/resolve.rs @@ -4,23 +4,28 @@ use agent_desktop_core::{ refs::RefEntry, }; use rustc_hash::FxHashSet; +use std::time::{Duration, Instant}; use super::AXElement; -use super::element::{child_attributes, copy_ax_array, copy_string_attr, resolve_element_name}; +use super::attributes::{copy_ax_array, copy_string_attr, set_messaging_timeout}; +use super::element::{child_attributes, resolve_element_name}; use super::element_dedupe::ElementDedupe; use super::resolve_bounds::{bounds_match, should_prune_by_bounds}; +use super::resolve_deadline::{ + ensure_before_deadline, remaining_before_deadline, sleep_before_retry, +}; use super::resolve_identity::{has_meaningful_identity, identity_matches}; use super::resolve_roots::{candidate_roots, path_candidate_roots}; #[cfg(target_os = "macos")] pub fn resolve_element_impl(entry: &RefEntry) -> Result { - resolve_element_with_timeout(entry, std::time::Duration::from_secs(5)) + resolve_element_with_timeout(entry, Duration::from_secs(5)) } #[cfg(target_os = "macos")] pub fn resolve_element_with_timeout( entry: &RefEntry, - timeout: std::time::Duration, + timeout: Duration, ) -> Result { tracing::debug!( "resolve: searching pid={} role={} name={:?} description={:?} bounds_hash={:?}", @@ -30,13 +35,12 @@ pub fn resolve_element_with_timeout( entry.description.as_deref().unwrap_or("(none)"), entry.bounds_hash ); - let resolve_depth: u8 = 50; - let deadline = std::time::Instant::now() + timeout; - let attempts = 4; + let (resolve_depth, attempts) = (50, 4); + let deadline = Instant::now() + timeout; for attempt in 0..attempts { if can_use_path_fast_path(entry) { - let path_roots = path_candidate_roots(entry); - match find_entry_by_path(&path_roots, entry) { + let path_roots = path_candidate_roots(entry, deadline)?; + match find_entry_by_path(&path_roots, entry, deadline) { Ok(handle) => { tracing::debug!("resolve: found path match"); return Ok(handle); @@ -57,7 +61,7 @@ pub fn resolve_element_with_timeout( } continue; } - let roots = candidate_roots(entry); + let roots = candidate_roots(entry, deadline)?; match find_entry_in_roots(&roots, entry, resolve_depth, deadline) { Ok(handle) => { tracing::debug!("resolve: found exact match"); @@ -90,14 +94,6 @@ fn is_retryable_resolution_error(err: &AdapterError) -> bool { err.code == ErrorCode::ElementNotFound } -#[cfg(target_os = "macos")] -fn sleep_before_retry(deadline: std::time::Instant) { - let remaining = deadline.saturating_duration_since(std::time::Instant::now()); - if !remaining.is_zero() { - std::thread::sleep(remaining.min(std::time::Duration::from_millis(75))); - } -} - #[cfg(target_os = "macos")] fn can_use_path_fast_path(entry: &RefEntry) -> bool { (entry.root_ref.is_none() || entry.path_is_absolute) @@ -121,18 +117,24 @@ fn can_use_broad_search(entry: &RefEntry) -> bool { } #[cfg(target_os = "macos")] -fn find_entry_by_path(roots: &[AXElement], entry: &RefEntry) -> Result { +fn find_entry_by_path( + roots: &[AXElement], + entry: &RefEntry, + deadline: Instant, +) -> Result { if entry.path.is_empty() { return Err(AdapterError::element_not_found("element")); } + ensure_before_deadline(deadline)?; let mut matches = Vec::new(); let mut dedupe = ElementDedupe; for root in roots { + ensure_before_deadline(deadline)?; if matches.len() > 1 { break; } - let Some(candidate) = element_at_path(root, &entry.path) else { + let Some(candidate) = element_at_path(root, &entry.path, deadline)? else { continue; }; if element_matches_entry(&candidate, entry) { @@ -144,14 +146,23 @@ fn find_entry_by_path(roots: &[AXElement], entry: &RefEntry) -> Result Option { +fn element_at_path( + root: &AXElement, + path: &[usize], + deadline: Instant, +) -> Result, AdapterError> { let mut current = root.clone(); for idx in path { + ensure_before_deadline(deadline)?; + set_messaging_timeout(¤t, remaining_before_deadline(deadline)?); let ax_role = copy_string_attr(¤t, accessibility_sys::kAXRoleAttribute); - let children = resolve_children(¤t, ax_role.as_deref()); - current = children.get(*idx)?.clone(); + let children = resolve_children(¤t, ax_role.as_deref(), deadline)?; + let Some(child) = children.get(*idx) else { + return Ok(None); + }; + current = child.clone(); } - Some(current) + Ok(Some(current)) } #[cfg(target_os = "macos")] @@ -159,7 +170,7 @@ fn find_entry_in_roots( roots: &[AXElement], entry: &RefEntry, resolve_depth: u8, - deadline: std::time::Instant, + deadline: Instant, ) -> Result { let mut matches = Vec::new(); let mut seen_matches = ElementDedupe; @@ -247,7 +258,7 @@ struct CollectContext<'a> { ancestors: &'a mut FxHashSet, seen_matches: &'a mut ElementDedupe, matches: &'a mut Vec, - deadline: std::time::Instant, + deadline: Instant, } #[cfg(target_os = "macos")] @@ -261,12 +272,7 @@ fn collect_elements_recursive( if context.matches.len() > 1 { return Ok(()); } - if std::time::Instant::now() > context.deadline { - return Err( - AdapterError::new(ErrorCode::Timeout, "Element resolution timed out") - .with_suggestion("Retry the command, or run 'snapshot' if the UI changed."), - ); - } + ensure_before_deadline(context.deadline)?; let ptr_key = el.0 as usize; if !context.ancestors.insert(ptr_key) { @@ -287,7 +293,7 @@ fn collect_elements_recursive( } if depth < context.max_depth && !should_prune_by_bounds(el, context.entry, depth) { - let children = resolve_children(el, ax_role.as_deref()); + let children = resolve_children(el, ax_role.as_deref(), context.deadline)?; for child in &children { collect_elements_recursive(child, depth + 1, context)?; } @@ -335,10 +341,16 @@ fn element_matches_path_entry_with_role( } #[cfg(target_os = "macos")] -fn resolve_children(el: &AXElement, ax_role: Option<&str>) -> Vec { +fn resolve_children( + el: &AXElement, + ax_role: Option<&str>, + deadline: Instant, +) -> Result, AdapterError> { let mut seen = FxHashSet::default(); let mut result = Vec::new(); for attr in child_attributes(ax_role) { + ensure_before_deadline(deadline)?; + set_messaging_timeout(el, remaining_before_deadline(deadline)?); if let Some(children) = copy_ax_array(el, attr) { for child in children { if seen.insert(child.0 as usize) { @@ -347,7 +359,7 @@ fn resolve_children(el: &AXElement, ax_role: Option<&str>) -> Vec { } } } - result + Ok(result) } #[cfg(test)] diff --git a/crates/macos/src/tree/resolve_deadline.rs b/crates/macos/src/tree/resolve_deadline.rs new file mode 100644 index 0000000..4895380 --- /dev/null +++ b/crates/macos/src/tree/resolve_deadline.rs @@ -0,0 +1,29 @@ +use agent_desktop_core::error::{AdapterError, ErrorCode}; +use std::time::{Duration, Instant}; + +#[cfg(target_os = "macos")] +pub(super) fn remaining_before_deadline(deadline: Instant) -> Result { + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return Err(timeout_error()); + } + Ok(remaining) +} + +#[cfg(target_os = "macos")] +pub(super) fn ensure_before_deadline(deadline: Instant) -> Result<(), AdapterError> { + remaining_before_deadline(deadline).map(|_| ()) +} + +#[cfg(target_os = "macos")] +pub(super) fn timeout_error() -> AdapterError { + AdapterError::new(ErrorCode::Timeout, "Element resolution timed out") + .with_suggestion("Retry the command, or run 'snapshot' if the UI changed.") +} + +#[cfg(target_os = "macos")] +pub(super) fn sleep_before_retry(deadline: Instant) { + if let Ok(remaining) = remaining_before_deadline(deadline) { + std::thread::sleep(remaining.min(Duration::from_millis(75))); + } +} diff --git a/crates/macos/src/tree/resolve_roots.rs b/crates/macos/src/tree/resolve_roots.rs index cea514c..ee556e5 100644 --- a/crates/macos/src/tree/resolve_roots.rs +++ b/crates/macos/src/tree/resolve_roots.rs @@ -1,81 +1,106 @@ -use agent_desktop_core::{adapter::SnapshotSurface, refs::RefEntry}; +use agent_desktop_core::{adapter::SnapshotSurface, error::AdapterError, refs::RefEntry}; +use std::time::Instant; use super::AXElement; -use super::builder::window_element_for; -use super::element::{ - copy_ax_array, copy_element_attr, copy_i64_attr, copy_string_attr, element_for_pid, +use super::attributes::{ + copy_ax_array, copy_element_attr, copy_i64_attr, copy_string_attr, set_messaging_timeout, }; +use super::element::element_for_pid; use super::element_dedupe::ElementDedupe; +use super::resolve_deadline::{ensure_before_deadline, remaining_before_deadline}; #[cfg(target_os = "macos")] -pub(super) fn path_candidate_roots(entry: &RefEntry) -> Vec { +pub(super) fn path_candidate_roots( + entry: &RefEntry, + deadline: Instant, +) -> Result, AdapterError> { if entry.bounds_hash.is_some() { - return candidate_roots(entry); + return candidate_roots(entry, deadline); } - scoped_surface_root(entry).into_iter().collect() + Ok(scoped_surface_root(entry, deadline)?.into_iter().collect()) } #[cfg(target_os = "macos")] -pub(super) fn candidate_roots(entry: &RefEntry) -> Vec { +pub(super) fn candidate_roots( + entry: &RefEntry, + deadline: Instant, +) -> Result, AdapterError> { let root = element_for_pid(entry.pid); + prepare_for_read(&root, deadline)?; let mut roots = Vec::new(); let mut dedupe = ElementDedupe; - if let Some(source_window_title) = entry.source_window_title.as_deref() { - dedupe.push( - &mut roots, - window_element_for(entry.pid, source_window_title), - ); + if let Some(window) = exact_source_window_root(entry, deadline)? { + dedupe.push(&mut roots, window); } + prepare_for_read(&root, deadline)?; if let Some(focused) = copy_element_attr(&root, "AXFocusedWindow") { dedupe.push(&mut roots, focused); } + prepare_for_read(&root, deadline)?; if let Some(main) = copy_element_attr(&root, "AXMainWindow") { dedupe.push(&mut roots, main); } + prepare_for_read(&root, deadline)?; for window in copy_ax_array(&root, "AXWindows").unwrap_or_default() { dedupe.push(&mut roots, window); } + ensure_before_deadline(deadline)?; if let Some(menubar) = crate::tree::menubar_for_pid(entry.pid) { dedupe.push(&mut roots, menubar); } + ensure_before_deadline(deadline)?; if let Some(menu) = crate::tree::menu_element_for_pid(entry.pid) { dedupe.push(&mut roots, menu); } if roots.is_empty() { roots.push(root); } - roots + Ok(roots) } #[cfg(target_os = "macos")] -fn scoped_surface_root(entry: &RefEntry) -> Option { - match entry.source_surface { - SnapshotSurface::Window => exact_source_window_root(entry), +fn scoped_surface_root( + entry: &RefEntry, + deadline: Instant, +) -> Result, AdapterError> { + ensure_before_deadline(deadline)?; + let root = match entry.source_surface { + SnapshotSurface::Window => exact_source_window_root(entry, deadline)?, SnapshotSurface::Focused => crate::tree::focused_surface_for_pid(entry.pid), SnapshotSurface::Menu => crate::tree::menu_element_for_pid(entry.pid), SnapshotSurface::Menubar => crate::tree::menubar_for_pid(entry.pid), SnapshotSurface::Sheet => crate::tree::sheet_for_pid(entry.pid), SnapshotSurface::Popover => crate::tree::popover_for_pid(entry.pid), SnapshotSurface::Alert => crate::tree::alert_for_pid(entry.pid), - } + }; + Ok(root) } #[cfg(target_os = "macos")] -fn exact_source_window_root(entry: &RefEntry) -> Option { +fn exact_source_window_root( + entry: &RefEntry, + deadline: Instant, +) -> Result, AdapterError> { let root = element_for_pid(entry.pid); - let windows = copy_ax_array(&root, "AXWindows")?; + prepare_for_read(&root, deadline)?; + let Some(windows) = copy_ax_array(&root, "AXWindows") else { + return Ok(None); + }; if let Some(source_window_number) = source_window_number(entry) { - if let Some(window) = windows - .iter() - .find(|win| copy_i64_attr(win, "AXWindowNumber") == Some(source_window_number)) - { - return Some(window.clone()); + if let Some(window) = windows.iter().find(|win| { + prepare_for_read(win, deadline).is_ok() + && copy_i64_attr(win, "AXWindowNumber") == Some(source_window_number) + }) { + return Ok(Some(window.clone())); } } - let source_window_title = entry.source_window_title.as_deref()?; - windows - .into_iter() - .find(|win| copy_string_attr(win, "AXTitle").as_deref() == Some(source_window_title)) + let Some(source_window_title) = entry.source_window_title.as_deref() else { + return Ok(None); + }; + Ok(windows.into_iter().find(|win| { + prepare_for_read(win, deadline).is_ok() + && copy_string_attr(win, "AXTitle").as_deref() == Some(source_window_title) + })) } #[cfg(target_os = "macos")] @@ -87,3 +112,9 @@ pub(super) fn source_window_number(entry: &RefEntry) -> Option { .parse() .ok() } + +#[cfg(target_os = "macos")] +fn prepare_for_read(element: &AXElement, deadline: Instant) -> Result<(), AdapterError> { + set_messaging_timeout(element, remaining_before_deadline(deadline)?); + Ok(()) +} diff --git a/crates/macos/src/tree/resolve_tests.rs b/crates/macos/src/tree/resolve_tests.rs index 488bb53..069bcba 100644 --- a/crates/macos/src/tree/resolve_tests.rs +++ b/crates/macos/src/tree/resolve_tests.rs @@ -150,6 +150,20 @@ fn only_element_not_found_is_retryable_resolution_error() { ))); } +#[test] +fn expired_deadline_fails_before_path_resolution_reads() { + let err = match find_entry_by_path( + &[], + &entry(Some(42), Some("w-42"), Some("Documents"), None), + std::time::Instant::now(), + ) { + Ok(_) => panic!("expected timeout"), + Err(err) => err, + }; + + assert_eq!(err.code, ErrorCode::Timeout); +} + #[test] fn ambiguous_candidate_classification_reports_structured_details() { let err = match classify_candidates( diff --git a/crates/macos/src/tree/surfaces.rs b/crates/macos/src/tree/surfaces.rs index 718eb85..7f85410 100644 --- a/crates/macos/src/tree/surfaces.rs +++ b/crates/macos/src/tree/surfaces.rs @@ -1,7 +1,6 @@ use super::AXElement; -use super::element::{ - copy_ax_array, copy_bool_attr, copy_element_attr, copy_string_attr, element_for_pid, -}; +use super::attributes::{copy_ax_array, copy_bool_attr, copy_element_attr, copy_string_attr}; +use super::element::element_for_pid; use agent_desktop_core::node::SurfaceInfo; #[cfg(target_os = "macos")] diff --git a/docs/solutions/best-practices/playwright-grade-desktop-reliability-2026-06-02.md b/docs/solutions/best-practices/playwright-grade-desktop-reliability-2026-06-02.md index 445a328..f243732 100644 --- a/docs/solutions/best-practices/playwright-grade-desktop-reliability-2026-06-02.md +++ b/docs/solutions/best-practices/playwright-grade-desktop-reliability-2026-06-02.md @@ -1,21 +1,26 @@ --- title: Playwright-grade desktop reliability contract date: 2026-06-02 +last_updated: 2026-06-04 category: best-practices -module: crates/core +module: crates/core, crates/macos, crates/ffi, src problem_type: best_practice -component: reliability +component: tooling severity: high applies_when: - - Ref resolution, action dispatch, wait semantics, session scope, or FFI action paths change - - A platform adapter is added or modified - - A command moves from direct adapter calls to shared helpers + - Ref resolution, action dispatch, wait semantics, session scope, trace output, or FFI ref-action paths change + - Platform adapters add live state, bounds, action, or resolution behavior + - CLI or FFI command paths are refactored around shared helpers + - Adapter conformance tests are added or changed tags: - reliability - - playwright - - refs + - ref-actions + - strict-resolution - actionability - - cross-platform + - wait + - sessions + - tracing + - ffi-parity --- # Playwright-grade desktop reliability contract @@ -28,19 +33,105 @@ fail with structured recovery when the target is stale or ambiguous. Desktop automation cannot copy browser semantics directly, but it can use the same engineering shape. -## Contract +The reliability enhancement moved agent-desktop toward that model without +turning the CLI into a browser tool. Core owns the contract. Platform adapters +provide native evidence. CLI and FFI callers share the same ref-action semantics +unless a lower-level API explicitly documents that it bypasses the strict ref +path. -Ref actions must pass through the shared reliability path: +This learning updates the original contract doc with the implementation details +that matter for future changes. + +## Guidance + +### Keep Hidden Identity Evidence + +Snapshot output may hide pixel bounds for compact, agent-friendly responses, but +the persisted ref identity still needs bounds evidence. The safe pattern is: + +1. Ask the adapter for bounds when building a ref-bearing snapshot. +2. Allocate refs and store `bounds_hash` from that full internal tree. +3. Strip visible `bounds` only from the returned output when the caller did not + request them. +4. Keep `bounds_hash` in the refmap so later actionability can detect stale refs. + +Do not let presentation options erase identity evidence. Compact output is a +serialization concern, not a weaker ref contract. + +### Centralize Strict Ref Actions + +Ref actions must pass through the same ladder: 1. Load the refmap from the caller's session. 2. Resolve the ref with strict platform identity checks. 3. Return `STALE_REF` when the old element no longer matches. 4. Return `AMBIGUOUS_TARGET` when multiple candidates match. 5. Run live actionability checks before adapter dispatch. -6. Keep waits bounded by the caller timeout, including platform resolution retries. -7. Emit trace events only to the requested JSONL trace path, never stdout. +6. Dispatch the adapter action chosen by the command's policy. +7. Release native handles through the adapter boundary. -## Cross-Platform Rule +The CLI path and FFI strict ref-action path should share this model. A lower +level native-handle FFI call may remain available, but it must be documented as +lower-level: it acts on a handle the caller already resolved and therefore does +not have ref identity evidence to re-check. + +Command-specific policy still belongs at the command edge. Centralization should +remove duplicated mechanics; it should not flatten `ActionRequest` choices or +post-condition verification. + +### Treat Actionability as a Core Contract + +Actionability is not an adapter-specific convenience. Core decides whether a +resolved ref is safe to act on using native evidence supplied by the adapter: + +- visibility from live bounds +- stability from live bounds hash versus snapshot bounds hash +- enabled state from live state +- supported action from live or snapshot actions +- interaction policy from the command request +- editability from role and supported actions + +Unavailable native evidence should be `unknown`, not a false failure, when the +platform cannot provide it. A non-empty live action list can narrow capabilities; +an empty transient live action list should not erase snapshot capabilities. + +### Keep Waits Bounded and Honest + +Wait commands must not hide permanent adapter failures behind timeout polling. +The reliable split is: + +- Retry transient resolution states such as stale, not found, ambiguous, or + timeout while the caller's timeout budget remains. +- Propagate permanent adapter errors immediately. +- Preserve the last observed retryable state in timeout details. +- For `wait --element` without `--snapshot`, refresh the latest-ref cache on a + bounded cadence; for a fixed `--snapshot`, treat missing refs as invalid input + instead of silently switching snapshots. + +This keeps `wait` useful for changing desktop state without making it a blanket +error suppressor. + +### Make Tracing Diagnostic, Not Behavioral + +Trace output belongs in the requested JSONL file and never in stdout. It must be +safe for agents to use with machine-readable command output: + +- create private trace files +- reject symlink trace paths on Unix +- redact sensitive text/value/name/message fields +- let `--trace-strict` fail on setup and pre-action trace writes +- keep post-action success trace writes best-effort after the desktop mutation + has already happened + +Reporting a successful desktop mutation as failed because the final trace write +failed is worse than losing that final diagnostic event. + +Do not describe the current trace as equivalent to Playwright Trace Viewer. It +is a JSONL diagnostic stream, not a bundled timeline artifact with before/after +snapshots, screenshots, source metadata, and an inspection UI. A Playwright-like +desktop trace should be introduced as a separate artifact format if needed. + +### Keep the Foundation Cross-Platform Core owns the contract; adapters own native evidence. Windows and Linux should not fork CLI semantics. UIA and AT-SPI implementations must map their native @@ -54,7 +145,45 @@ separate reads, but the CLI behavior must remain identical: empty transient action reads do not erase snapshot capabilities, while a non-empty live action set that lacks the required action can block dispatch. -## Review Rule +Do not add macOS-only assumptions to core to solve a macOS bug. If a behavior is +part of the CLI or FFI contract, core should express the contract and each +adapter should provide native evidence or return a structured unsupported error. + +### Triage Review Findings Against Project Constraints + +Not every simplification suggested by a reviewer is a good simplification. In +this repo: + +- One command per file is intentional; collapsing small command wrappers fights + the project structure. +- Keeping helpers extracted is correct when inlining would push a file toward + the 400 LOC limit or duplicate a shared contract. +- Centralizing strict ref-action behavior in core is better than moving it only + into FFI because CLI and FFI parity is the invariant. +- Windows/Linux portability means platform-neutral contracts in core, not + pretending current macOS AX evidence already exists on other platforms. + +False positives should be marked as such. Do not add code only to satisfy a +review comment that contradicts the architecture. + +## Why This Matters + +The user-visible failure modes are severe: + +- acting on a stale ref can mutate the wrong UI element +- hidden bounds in compact snapshots can accidentally weaken future stale-ref + detection +- wait loops that swallow permanent errors waste time and obscure permissions or + adapter failures +- FFI and CLI divergence makes language bindings less reliable than the command + line +- trace failures after mutation can make successful desktop actions look failed + +Playwright's reliability comes from a predictable action pipeline. agent-desktop +needs the same predictability across desktop platforms, even though native +accessibility APIs expose weaker and less uniform evidence than a browser DOM. + +## When to Apply Any change to ref resolution or action dispatch must include tests for: @@ -64,7 +193,50 @@ Any change to ref resolution or action dispatch must include tests for: - retrying waits that honor timeout and report last observed state - session isolation - FFI parity when the behavior is exposed through C ABI +- compact snapshot output preserving hidden identity evidence in the refmap +- trace strictness without post-mutation false failures If a platform needs a coordinate fallback, the fallback must be explicit and lower confidence. Do not silently replace a failed semantic action with a pixel click. + +## Examples + +Snapshot construction should request identity bounds internally, then hide only +presentation bounds when needed: + +```rust +let identity_opts = opts.with_ref_identity_bounds(); +let tree = adapter.get_tree(&window, &identity_opts)?; +let (tree, refmap) = allocate_refs(tree, opts)?; +let tree = strip_ref_bounds_when_hidden(tree, opts); +``` + +Ref action execution should keep the command-selected policy while centralizing +the strict ladder: + +```rust +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)?; +let result = adapter.execute_action(handle.handle(), request)?; +``` + +The platform adapter should expose a single live element read when possible: + +```rust +LiveElement { + state: live_state, + bounds: live_bounds, + available_actions: live_actions, +} +``` + +Windows UIA and Linux AT-SPI adapters can fill those fields differently, but the +core actionability decision must stay the same. + +## Related + +- [Keep FFI action policy aligned with CLI action policy](keep-ffi-action-policy-aligned-with-cli-2026-05-12.md) +- [Preserve command policy semantics during shared ref-action refactors](preserve-command-policy-semantics-during-refactor-2026-05-12.md) +- [Guard OS-reordered resources with an identity fingerprint, not a raw index](identity-fingerprint-against-os-reorder-2026-04-16.md) +- [Progressive snapshot contract fixes after review](../logic-errors/progressive-snapshot-review-contract-2026-04-16.md) diff --git a/src/tests/conformance.rs b/src/tests/conformance.rs index 7822df1..f431e5a 100644 --- a/src/tests/conformance.rs +++ b/src/tests/conformance.rs @@ -1,6 +1,9 @@ use agent_desktop_core::{ - action::{Action, ActionRequest, ActionResult, ElementState}, + action::Action, + action_request::ActionRequest, + action_result::ActionResult, adapter::{LiveElement, NativeHandle, PlatformAdapter, SnapshotSurface}, + element_state::ElementState, error::{AdapterError, ErrorCode}, node::Rect, refs::RefEntry,