From e4154dea33fa6051daba112d7b54945efadf96b4 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Fri, 3 Jul 2026 18:20:25 -0700 Subject: [PATCH] fix: repair real-app regressions the mock adapter concealed The foundation-contract branch passed unit CI but broke observation and interaction against real macOS apps. The unit suite runs on an in-memory MockAdapter that cannot exercise the platform's AX plumbing, so a batch of adapter regressions shipped green; running the live e2e surfaced them. - Window resolution: match AX windows to their CGWindowID via the private but stable _AXUIElementGetWindow bridge instead of the nonexistent AXWindowNumber attribute, which had made snapshot/find return WINDOW_NOT_FOUND for every app. Verified across single- and multi-window apps. - Accessible name: collapse the builder, strict resolver, hit-test, and ambiguity classifier onto one resolve_element_name (title -> description -> static-text value) so a ref's stored name always matches what the resolver recomputes. Fixes STALE_REF on elements named via a non-title rung (e.g. textfields labelled through AXDescription). - find: route through the single snapshot matcher (full traversal, correct names, real refs) and drop the redundant live resolve_query path that was correlated to the snapshot by index. - find --window-id: scope a search to one window, via a shared WindowScope arg group flattened into snapshot/find/screenshot. Ref-based and keyboard commands intentionally omit it -- a ref already carries its source window and keyboard input targets the focused window. - Trace: restore ref.resolve.ok on successful ref resolution. - Remove the now-dead resolve_query, the macOS live query matcher, get_live_name_evidence, and the superseded accname compute_name reduction. Verified by the live e2e (71/71) and the full unit/clippy/fmt/isolation gates. --- crates/core/src/accname.rs | 100 +--------- crates/core/src/accname_tests.rs | 147 --------------- crates/core/src/adapter/observation.rs | 16 -- crates/core/src/commands/find.rs | 98 ++-------- crates/core/src/commands/find_tests.rs | 5 + crates/core/src/ref_action_wait.rs | 6 + crates/macos/src/adapter_observation.rs | 19 -- crates/macos/src/system/window_resolve.rs | 29 ++- crates/macos/src/tree/element.rs | 18 +- crates/macos/src/tree/element_tests.rs | 28 --- crates/macos/src/tree/mod.rs | 1 - crates/macos/src/tree/name_evidence.rs | 67 +------ crates/macos/src/tree/query.rs | 213 ---------------------- crates/macos/src/tree/query_tests.rs | 122 ------------- src/cli_args/mod.rs | 37 ++-- src/command_policy/mod.rs | 2 +- src/command_policy/tests.rs | 20 +- src/dispatch/observation.rs | 11 +- 18 files changed, 123 insertions(+), 816 deletions(-) delete mode 100644 crates/core/src/accname_tests.rs delete mode 100644 crates/macos/src/tree/query.rs delete mode 100644 crates/macos/src/tree/query_tests.rs diff --git a/crates/core/src/accname.rs b/crates/core/src/accname.rs index fc9bc63..ada3220 100644 --- a/crates/core/src/accname.rs +++ b/crates/core/src/accname.rs @@ -1,102 +1,20 @@ use serde::{Deserialize, Serialize}; -/// Raw, unreduced evidence an adapter gathers for one element's accessible -/// name/description. Fields carry native attribute text as-is; the -/// precedence between them is decided exclusively by [`compute_name`] and -/// [`compute_description`] in this module — adapters must never apply their -/// own fallback chain (see `docs/plans/.../KTD6`). +/// Raw accessible-name evidence an adapter gathers for one element: its own +/// title, its description, and — for static-text roles only — its value +/// promoted to a name. Each platform's `resolve_element_name` reduces this to a +/// single accessible name; the macOS precedence is title → description → +/// static value. Keeping the evidence typed (rather than returning a bare +/// `String`) keeps every name consumer — the snapshot builder, strict ref +/// re-resolution, hit-test occluder naming, and ambiguity classification — +/// reducing the *same* evidence the same way, so a stored ref name always +/// matches what the resolver recomputes. #[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] pub struct NameEvidence { - /// Rung 1 — an explicit, authoritative label distinct from a - /// labelled-by reference. UIA: `Name` when sourced from an explicit - /// `AutomationProperties.Name`-style override. AT-SPI: an explicit - /// `label` property, when the toolkit exposes one separately from the - /// `LABELLED_BY` relation. macOS has no attribute distinct from - /// `AXTitleUIElement`, so this rung is always `None` on macOS today. - #[serde(skip_serializing_if = "Option::is_none")] - pub explicit_label: Option, - /// Rung 2 — text drawn from another element the platform designates as - /// this element's label. macOS: `AXTitleUIElement` (resolved to that - /// element's own title/value text). UIA: `LabeledBy`. AT-SPI: - /// `LABELLED_BY` relation target's name. - #[serde(skip_serializing_if = "Option::is_none")] - pub labelled_by_text: Option, - /// Rung 3 — the element's own native title/name string. macOS: - /// `AXTitle`. UIA: `Name` (default, unlabelled case). AT-SPI: `name`. #[serde(skip_serializing_if = "Option::is_none")] pub native_title: Option, - /// Rung 4 — the element's value promoted to a name, only for - /// non-interactive, static/read-only roles (e.g. macOS `AXValue` on - /// `AXStaticText`; UIA `ValuePattern.Value` on a `Text` control). The - /// adapter is responsible for the role gating; core treats presence of - /// this field as sufficient. #[serde(skip_serializing_if = "Option::is_none")] pub static_role_value: Option, - /// Rung 5 — text aggregated from descendant labels in document order, - /// for containers with no direct label of their own (a toolbar button - /// wrapping an icon + text child, for example). Adapters build this - /// with [`join_child_labels`] over the per-child text they collect. - #[serde(skip_serializing_if = "Option::is_none")] - pub child_label: Option, - /// Rung 6 — placeholder/hint text shown while the control is empty. - /// macOS: `AXPlaceholderValue`. UIA: `HelpText` used as a placeholder - /// fallback. AT-SPI: `placeholder-text`. - #[serde(skip_serializing_if = "Option::is_none")] - pub placeholder: Option, - /// Rung 7 (name fallback of last resort) and also the accessible - /// description proper via [`compute_description`]. macOS: `AXDescription`. - /// UIA: `HelpText`. AT-SPI: `description`. #[serde(skip_serializing_if = "Option::is_none")] pub description: Option, } - -fn non_empty(value: &Option) -> Option { - value - .as_deref() - .map(str::trim) - .filter(|s| !s.is_empty()) - .map(str::to_string) -} - -/// Reduces [`NameEvidence`] to a single accessible name following the -/// documented 7-rung precedence: explicit label, labelled-by text, native -/// title, static-role value, aggregated child label, placeholder, -/// description last. The earliest non-empty rung wins; core never re-derives -/// evidence, it only picks among what the adapter supplied. -pub fn compute_name(evidence: &NameEvidence) -> Option { - non_empty(&evidence.explicit_label) - .or_else(|| non_empty(&evidence.labelled_by_text)) - .or_else(|| non_empty(&evidence.native_title)) - .or_else(|| non_empty(&evidence.static_role_value)) - .or_else(|| non_empty(&evidence.child_label)) - .or_else(|| non_empty(&evidence.placeholder)) - .or_else(|| non_empty(&evidence.description)) -} - -/// Reduces [`NameEvidence`] to the accessible description: the raw -/// description rung, independent of whether it was also consumed as the -/// name's rung-7 fallback. -pub fn compute_description(evidence: &NameEvidence) -> Option { - non_empty(&evidence.description) -} - -/// Joins per-child label text into a single aggregated child-label rung, -/// in the document order the caller supplies them, trimming and dropping -/// empty entries. Returns `None` when nothing survives. -pub fn join_child_labels<'a, I: IntoIterator>(labels: I) -> Option { - let joined = labels - .into_iter() - .map(str::trim) - .filter(|s| !s.is_empty()) - .collect::>() - .join(" "); - if joined.is_empty() { - None - } else { - Some(joined) - } -} - -#[cfg(test)] -#[path = "accname_tests.rs"] -mod tests; diff --git a/crates/core/src/accname_tests.rs b/crates/core/src/accname_tests.rs deleted file mode 100644 index 95955a4..0000000 --- a/crates/core/src/accname_tests.rs +++ /dev/null @@ -1,147 +0,0 @@ -use super::*; - -fn evidence_all_rungs() -> NameEvidence { - NameEvidence { - explicit_label: Some("explicit".into()), - labelled_by_text: Some("labelled-by".into()), - native_title: Some("native-title".into()), - static_role_value: Some("static-value".into()), - child_label: Some("child-label".into()), - placeholder: Some("placeholder".into()), - description: Some("description".into()), - } -} - -#[test] -fn rung1_explicit_label_wins_over_every_lower_rung() { - let evidence = evidence_all_rungs(); - assert_eq!(compute_name(&evidence).as_deref(), Some("explicit")); -} - -#[test] -fn rung2_labelled_by_text_wins_when_explicit_absent() { - let mut evidence = evidence_all_rungs(); - evidence.explicit_label = None; - assert_eq!(compute_name(&evidence).as_deref(), Some("labelled-by")); -} - -#[test] -fn rung3_native_title_wins_when_rungs_1_2_absent() { - let mut evidence = evidence_all_rungs(); - evidence.explicit_label = None; - evidence.labelled_by_text = None; - assert_eq!(compute_name(&evidence).as_deref(), Some("native-title")); -} - -#[test] -fn rung4_static_role_value_wins_when_rungs_1_3_absent() { - let mut evidence = evidence_all_rungs(); - evidence.explicit_label = None; - evidence.labelled_by_text = None; - evidence.native_title = None; - assert_eq!(compute_name(&evidence).as_deref(), Some("static-value")); -} - -#[test] -fn rung5_child_label_wins_when_rungs_1_4_absent() { - let mut evidence = evidence_all_rungs(); - evidence.explicit_label = None; - evidence.labelled_by_text = None; - evidence.native_title = None; - evidence.static_role_value = None; - assert_eq!(compute_name(&evidence).as_deref(), Some("child-label")); -} - -#[test] -fn rung6_placeholder_wins_when_rungs_1_5_absent() { - let mut evidence = evidence_all_rungs(); - evidence.explicit_label = None; - evidence.labelled_by_text = None; - evidence.native_title = None; - evidence.static_role_value = None; - evidence.child_label = None; - assert_eq!(compute_name(&evidence).as_deref(), Some("placeholder")); -} - -#[test] -fn rung7_description_is_last_resort_for_name() { - let mut evidence = evidence_all_rungs(); - evidence.explicit_label = None; - evidence.labelled_by_text = None; - evidence.native_title = None; - evidence.static_role_value = None; - evidence.child_label = None; - evidence.placeholder = None; - assert_eq!(compute_name(&evidence).as_deref(), Some("description")); -} - -#[test] -fn all_absent_evidence_computes_no_name() { - let evidence = NameEvidence::default(); - assert_eq!(compute_name(&evidence), None); -} - -#[test] -fn all_blank_evidence_computes_no_name() { - let evidence = NameEvidence { - explicit_label: Some(" ".into()), - labelled_by_text: Some("".into()), - native_title: Some("\t".into()), - static_role_value: Some(String::new()), - child_label: Some(" ".into()), - placeholder: Some(String::new()), - description: Some(" ".into()), - }; - assert_eq!(compute_name(&evidence), None); -} - -#[test] -fn compute_description_returns_description_rung_independent_of_name() { - let evidence = evidence_all_rungs(); - assert_eq!( - compute_description(&evidence).as_deref(), - Some("description") - ); - assert_eq!(compute_name(&evidence).as_deref(), Some("explicit")); -} - -#[test] -fn compute_description_none_when_description_absent() { - let mut evidence = evidence_all_rungs(); - evidence.description = None; - assert_eq!(compute_description(&evidence), None); -} - -#[test] -fn child_label_aggregation_joins_in_document_order() { - let joined = join_child_labels(["Save", "As...", "PDF"]); - assert_eq!(joined.as_deref(), Some("Save As... PDF")); -} - -#[test] -fn child_label_aggregation_drops_blank_entries_but_preserves_order() { - let joined = join_child_labels(["", "First", " ", "Second"]); - assert_eq!(joined.as_deref(), Some("First Second")); -} - -#[test] -fn child_label_aggregation_of_all_blank_entries_is_none() { - assert_eq!(join_child_labels(["", " ", "\t"]), None); -} - -#[test] -fn child_label_aggregation_of_empty_iterator_is_none() { - assert_eq!(join_child_labels(std::iter::empty()), None); -} - -#[test] -fn name_evidence_serde_roundtrip_skips_absent_fields() { - let evidence = NameEvidence { - native_title: Some("Only Title".into()), - ..Default::default() - }; - let json = serde_json::to_value(&evidence).expect("serialize"); - assert_eq!(json, serde_json::json!({ "native_title": "Only Title" })); - let round_tripped: NameEvidence = serde_json::from_value(json).expect("deserialize"); - assert_eq!(round_tripped, evidence); -} diff --git a/crates/core/src/adapter/observation.rs b/crates/core/src/adapter/observation.rs index d718b4b..bbfb29c 100644 --- a/crates/core/src/adapter/observation.rs +++ b/crates/core/src/adapter/observation.rs @@ -106,15 +106,6 @@ pub trait ObservationOps: Send + Sync { Err(AdapterError::not_supported("get_element_bounds")) } - fn resolve_query( - &self, - _query: &crate::locator::LocatorQuery, - _scope: Option<&NativeHandle>, - _pid: i32, - ) -> Result, AdapterError> { - Err(AdapterError::not_supported("resolve_query")) - } - fn hit_test( &self, handle: &NativeHandle, @@ -123,11 +114,4 @@ pub trait ObservationOps: Send + Sync { let _ = (handle, point); Err(AdapterError::not_supported("hit_test")) } - - fn get_live_name_evidence( - &self, - _handle: &NativeHandle, - ) -> Result { - Err(AdapterError::not_supported("get_live_name_evidence")) - } } diff --git a/crates/core/src/commands/find.rs b/crates/core/src/commands/find.rs index 2b75c23..3c235f0 100644 --- a/crates/core/src/commands/find.rs +++ b/crates/core/src/commands/find.rs @@ -1,6 +1,6 @@ use crate::{ adapter::PlatformAdapter, - commands::{helpers::resolve_app_pid, query}, + commands::query, context::CommandContext, error::AppError, locator::{IdentityPredicate, LocatorQuery, StatePredicate}, @@ -38,6 +38,7 @@ pub struct FindSelectionArgs { pub struct FindArgs { pub app: Option, + pub window_id: Option, pub filter: FindFilterArgs, pub states: Vec, pub selection: FindSelectionArgs, @@ -52,16 +53,6 @@ pub fn execute( let query = locator_query_from_args(&args)?; query.validate_states().map_err(AppError::Adapter)?; - if let Ok(pid) = resolve_app_pid(args.app.as_deref(), adapter) { - match adapter.resolve_query(&query, None, pid) { - Ok(handles) => { - return finish_from_live_handles(&args, &query, adapter, context, handles); - } - Err(err) if err.code == crate::error::ErrorCode::PlatformNotSupported => {} - Err(err) => return Err(AppError::Adapter(err)), - } - } - execute_snapshot_search(&args, &query, adapter, context) } @@ -108,58 +99,6 @@ fn locator_query_from_args(args: &FindArgs) -> Result { }) } -fn finish_from_live_handles( - args: &FindArgs, - query: &LocatorQuery, - adapter: &dyn PlatformAdapter, - context: &CommandContext, - handles: Vec, -) -> Result { - if args.selection.count { - return Ok(json!({ "count": handles.len() })); - } - - let snapshot_result = snapshot::run_with_context( - adapter, - &crate::adapter::TreeOptions::default(), - args.app.as_deref(), - None, - context, - )?; - let mut snapshot_matches = Vec::new(); - collect_snapshot_matches( - &snapshot_result.tree, - query, - &mut Vec::new(), - &mut snapshot_matches, - None, - ); - - let selected = select_live_indices(args, handles.len()); - let matches: Vec = selected - .into_iter() - .filter_map(|index| materialize_match(snapshot_matches.get(index))) - .collect(); - - if args.selection.first || args.selection.last || args.selection.nth.is_some() { - return Ok(single_match_response( - matches.into_iter().next(), - query, - &snapshot_result.tree, - )); - } - - let match_count = matches.len(); - let mut response = json!({ "matches": matches }); - attach_roles_present_hint( - &mut response, - match_count == 0, - query, - &snapshot_result.tree, - ); - Ok(response) -} - fn execute_snapshot_search( args: &FindArgs, query: &LocatorQuery, @@ -168,9 +107,20 @@ fn execute_snapshot_search( ) -> Result { let opts = crate::adapter::TreeOptions::default(); let result = if args.selection.count { - snapshot::build(adapter, &opts, args.app.as_deref(), None)? + snapshot::build( + adapter, + &opts, + args.app.as_deref(), + args.window_id.as_deref(), + )? } else { - snapshot::run_with_context(adapter, &opts, args.app.as_deref(), None, context)? + snapshot::run_with_context( + adapter, + &opts, + args.app.as_deref(), + args.window_id.as_deref(), + context, + )? }; if args.selection.count { @@ -217,24 +167,6 @@ fn execute_snapshot_search( Ok(response) } -fn select_live_indices(args: &FindArgs, total: usize) -> Vec { - if args.selection.first { - return vec![0].into_iter().filter(|_| total > 0).collect(); - } - if args.selection.last { - return total.checked_sub(1).into_iter().collect(); - } - if let Some(n) = args.selection.nth { - return (n < total).then_some(n).into_iter().collect(); - } - let limit = max_matches_for_args(args).unwrap_or(total); - (0..total.min(limit)).collect() -} - -fn materialize_match(snapshot_match: Option<&Value>) -> Option { - snapshot_match.cloned() -} - fn attach_roles_present_hint( response: &mut Value, is_empty: bool, diff --git a/crates/core/src/commands/find_tests.rs b/crates/core/src/commands/find_tests.rs index 8fe1c30..3b46620 100644 --- a/crates/core/src/commands/find_tests.rs +++ b/crates/core/src/commands/find_tests.rs @@ -132,6 +132,7 @@ fn default_limit_caps_materialized_matches() { fn limit_conflicts_with_single_result_modes_for_batch_too() { let err = validate_find_mode(&FindArgs { app: None, + window_id: None, filter: no_filter(), states: vec![], selection: FindSelectionArgs { @@ -182,6 +183,7 @@ fn role_node(role: &str, name: Option<&str>) -> AccessibilityNode { fn textarea_alias_resolves_to_textfield_query() { let query = query_from_args(&FindArgs { app: None, + window_id: None, filter: FindFilterArgs { role: Some("textarea".into()), ..no_filter() @@ -202,6 +204,7 @@ fn textarea_alias_resolves_to_textfield_query() { fn unknown_role_passes_through_and_matches_nothing() { let query = query_from_args(&FindArgs { app: None, + window_id: None, filter: FindFilterArgs { role: Some("navbar".into()), ..no_filter() @@ -228,6 +231,7 @@ fn empty_role_filtered_result_reports_roles_present_from_tree() { let query = query_from_args(&FindArgs { app: None, + window_id: None, filter: FindFilterArgs { role: Some("navbar".into()), ..no_filter() @@ -250,6 +254,7 @@ fn roles_present_hint_is_omitted_when_a_match_is_found() { let root = role_node("textfield", Some("body")); let query = query_from_args(&FindArgs { app: None, + window_id: None, filter: FindFilterArgs { role: Some("textfield".into()), ..no_filter() diff --git a/crates/core/src/ref_action_wait.rs b/crates/core/src/ref_action_wait.rs index 49cb994..4e37b46 100644 --- a/crates/core/src/ref_action_wait.rs +++ b/crates/core/src/ref_action_wait.rs @@ -61,6 +61,10 @@ fn trace_resolve_error(context: &CommandContext, ref_id: &str, err: &AdapterErro }); } +fn trace_resolve_ok(context: &CommandContext, ref_id: &str) { + let _ = context.trace_lazy("ref.resolve.ok", || json!({ "ref": ref_id })); +} + pub(crate) const POLL_INTERVAL: Duration = Duration::from_millis(100); pub(crate) const RESOLVE_ATTEMPT: Duration = Duration::from_millis(750); const MAX_BUDGET_MS: u64 = 24 * 60 * 60 * 1000; @@ -143,6 +147,7 @@ fn execute_single_shot( .adapter .resolve_element_strict(ctx.entry) .inspect_err(|err| trace_resolve_error(ctx.context, ctx.ref_id, err))?; + trace_resolve_ok(ctx.context, ctx.ref_id); let handle = ResolvedElement::new(ctx.adapter, handle); maybe_scroll_into_view(ctx.adapter, ctx.entry, handle.handle(), &request); dispatch( @@ -176,6 +181,7 @@ fn execute_poll_loop( return Err(actionability_timeout(last_report)); } ResolveAttemptOutcome::Resolved(handle) => { + trace_resolve_ok(ctx.context, ctx.ref_id); let resolved = ResolvedElement::new(ctx.adapter, handle); maybe_scroll_into_view(ctx.adapter, ctx.entry, resolved.handle(), &request); match dispatch( diff --git a/crates/macos/src/adapter_observation.rs b/crates/macos/src/adapter_observation.rs index 3a5d8b5..e2e6a38 100644 --- a/crates/macos/src/adapter_observation.rs +++ b/crates/macos/src/adapter_observation.rs @@ -71,15 +71,6 @@ impl ObservationOps for MacOSAdapter { Ok(crate::tree::surfaces::list_surfaces_for_pid(pid)) } - fn resolve_query( - &self, - query: &agent_desktop_core::locator::LocatorQuery, - scope: Option<&NativeHandle>, - pid: i32, - ) -> Result, AdapterError> { - crate::tree::query::resolve_query_impl(query, pid, scope) - } - fn hit_test( &self, handle: &NativeHandle, @@ -88,16 +79,6 @@ impl ObservationOps for MacOSAdapter { crate::tree::hit_test::hit_test_impl(handle, point) } - fn get_live_name_evidence( - &self, - handle: &NativeHandle, - ) -> Result { - Ok(with_borrowed_ax_element( - handle, - crate::tree::name_evidence::name_evidence_impl, - )) - } - fn get_live_value(&self, handle: &NativeHandle) -> Result, AdapterError> { #[cfg(target_os = "macos")] { diff --git a/crates/macos/src/system/window_resolve.rs b/crates/macos/src/system/window_resolve.rs index c1f18ee..1a80d5c 100644 --- a/crates/macos/src/system/window_resolve.rs +++ b/crates/macos/src/system/window_resolve.rs @@ -4,10 +4,10 @@ use agent_desktop_core::{ }; use crate::system::cg_window::WindowRecord; -use crate::tree::{AXElement, copy_ax_array, copy_i64_attr, copy_string_attr, element_for_pid}; +use crate::tree::{AXElement, copy_ax_array, copy_string_attr, element_for_pid}; #[cfg(target_os = "macos")] -use accessibility_sys::kAXWindowsAttribute; +use accessibility_sys::{AXUIElementRef, kAXWindowsAttribute}; pub(crate) fn window_element_for_info(win: &WindowInfo) -> Result { if win.id.is_empty() { @@ -79,13 +79,36 @@ fn ax_window_element_for_number(pid: i32, window_number: i64) -> Option Option { + let mut window_id: u32 = 0; + let result = unsafe { _AXUIElementGetWindow(window.0, &mut window_id) }; + (result == 0).then_some(i64::from(window_id)) +} + +#[cfg(not(target_os = "macos"))] +fn ax_window_id(_window: &AXElement) -> Option { + None +} + +#[cfg(target_os = "macos")] +unsafe extern "C" { + fn _AXUIElementGetWindow(element: AXUIElementRef, out: *mut u32) -> i32; +} + fn invalid_window_id(id: &str) -> AdapterError { AdapterError::new(ErrorCode::InvalidArgs, format!("Invalid window id: '{id}'")) .with_suggestion("Window ids come from 'list-windows' (format w-).") diff --git a/crates/macos/src/tree/element.rs b/crates/macos/src/tree/element.rs index ab6ca9d..8a6a8f0 100644 --- a/crates/macos/src/tree/element.rs +++ b/crates/macos/src/tree/element.rs @@ -305,13 +305,19 @@ mod imp { pub use imp::{count_children, element_for_pid, fetch_node_attrs}; -/// The element's accessible name, reduced from adapter-supplied -/// [`agent_desktop_core::accname::NameEvidence`] by core's precedence -/// algorithm. This function is a thin caller-facing wrapper — it owns no -/// fallback chain of its own; `name_evidence` module supplies raw evidence, -/// `compute_name` decides precedence. +/// The element's accessible name: its own title, else its description, else +/// (for static text only) its value promoted to a name. This is the single +/// source of truth every name consumer shares — the snapshot builder, strict +/// ref re-resolution, hit-test occluder naming, and ambiguity classification — +/// so a ref's stored name always matches what the resolver recomputes. The +/// per-platform `name_evidence` supplier gathers the raw attributes; this +/// reducer owns the precedence. pub fn resolve_element_name(el: &super::ax_element::AXElement) -> Option { - agent_desktop_core::accname::compute_name(&super::name_evidence::name_evidence_impl(el)) + let evidence = super::name_evidence::name_evidence_impl(el); + let non_empty = |text: Option| text.filter(|value| !value.trim().is_empty()); + non_empty(evidence.native_title) + .or_else(|| non_empty(evidence.description)) + .or_else(|| non_empty(evidence.static_role_value)) } #[cfg(test)] diff --git a/crates/macos/src/tree/element_tests.rs b/crates/macos/src/tree/element_tests.rs index 255a4d6..dfb27db 100644 --- a/crates/macos/src/tree/element_tests.rs +++ b/crates/macos/src/tree/element_tests.rs @@ -1,33 +1,5 @@ const ELEMENT_SOURCE: &str = include_str!("element.rs"); -#[test] -fn resolve_element_name_delegates_precedence_to_core_accname() { - assert!( - ELEMENT_SOURCE.contains("agent_desktop_core::accname::compute_name"), - "resolve_element_name must reduce NameEvidence through core's compute_name (KTD6); \ - found no call to accname::compute_name in element.rs" - ); - assert!( - ELEMENT_SOURCE.contains("super::name_evidence::name_evidence_impl"), - "resolve_element_name must gather evidence via the name_evidence supplier, not read \ - AX attributes itself" - ); -} - -#[test] -fn resolve_element_name_owns_no_local_fallback_chain() { - assert!( - !ELEMENT_SOURCE.contains("title.or(desc)"), - "resolve_element_name regressed to owning its own title/description fallback chain; \ - precedence belongs in accname::compute_name (KTD6)" - ); - assert!( - !ELEMENT_SOURCE.contains("kAXValueAttribute).or(name)"), - "resolve_element_name regressed to owning its own static-role-value fallback; \ - precedence belongs in accname::compute_name (KTD6)" - ); -} - #[test] fn readonly_derivation_has_a_single_owner() { let editable_role_check_sites = ELEMENT_SOURCE.matches("editable_ax_role(role)").count(); diff --git a/crates/macos/src/tree/mod.rs b/crates/macos/src/tree/mod.rs index 15ef0d1..437947d 100644 --- a/crates/macos/src/tree/mod.rs +++ b/crates/macos/src/tree/mod.rs @@ -12,7 +12,6 @@ pub mod hit_test; pub mod name_evidence; pub mod native_id; pub(crate) mod node_attrs; -pub mod query; pub mod resolve; mod resolve_bounds; mod resolve_classify; diff --git a/crates/macos/src/tree/name_evidence.rs b/crates/macos/src/tree/name_evidence.rs index f5174a0..b5921e0 100644 --- a/crates/macos/src/tree/name_evidence.rs +++ b/crates/macos/src/tree/name_evidence.rs @@ -1,87 +1,32 @@ #[cfg(target_os = "macos")] mod imp { use crate::tree::AXElement; - use crate::tree::attributes::{ - copy_ax_array, copy_ax_array_prefix, copy_element_attr, copy_string_attr, - }; - use crate::tree::element::child_attributes; + use crate::tree::attributes::copy_string_attr; use accessibility_sys::{ - kAXChildrenAttribute, kAXDescriptionAttribute, kAXPlaceholderValueAttribute, - kAXRoleAttribute, kAXTitleAttribute, kAXTitleUIElementAttribute, kAXValueAttribute, + kAXDescriptionAttribute, kAXRoleAttribute, kAXTitleAttribute, kAXValueAttribute, }; - use agent_desktop_core::accname::{NameEvidence, join_child_labels}; + use agent_desktop_core::accname::NameEvidence; const STATIC_TEXT_ROLE: &str = "AXStaticText"; - /// Gathers raw `NameEvidence` from an element's own AX attributes. - /// Every rung is a direct attribute read — precedence between rungs is - /// core's job via `accname::compute_name`, never decided here. + /// Gathers raw `NameEvidence` from an element's own AX attributes: its + /// title, its description, and (for static text only) its value. Precedence + /// between them is `resolve_element_name`'s job, never decided here. pub fn name_evidence_impl(el: &AXElement) -> NameEvidence { let ax_role = copy_string_attr(el, kAXRoleAttribute); NameEvidence { - explicit_label: None, - labelled_by_text: labelled_by_text(el), native_title: copy_string_attr(el, kAXTitleAttribute), static_role_value: static_role_value(el, ax_role.as_deref()), - child_label: aggregated_child_label(el, ax_role.as_deref()), - placeholder: copy_string_attr(el, kAXPlaceholderValueAttribute), description: copy_string_attr(el, kAXDescriptionAttribute), } } - fn labelled_by_text(el: &AXElement) -> Option { - let label_el = copy_element_attr(el, kAXTitleUIElementAttribute)?; - copy_string_attr(&label_el, kAXTitleAttribute) - .or_else(|| copy_string_attr(&label_el, kAXValueAttribute)) - } - fn static_role_value(el: &AXElement, ax_role: Option<&str>) -> Option { if ax_role != Some(STATIC_TEXT_ROLE) { return None; } copy_string_attr(el, kAXValueAttribute) } - - fn aggregated_child_label(el: &AXElement, ax_role: Option<&str>) -> Option { - let children = child_attributes(ax_role) - .iter() - .find_map(|attr| copy_ax_array(el, attr).filter(|v| !v.is_empty())) - .unwrap_or_default(); - let texts = collect_child_texts(&children); - join_child_labels(texts.iter().map(String::as_str)) - } - - fn collect_child_texts(children: &[AXElement]) -> Vec { - fn text_of(el: &AXElement) -> Option { - copy_string_attr(el, kAXValueAttribute) - .or_else(|| copy_string_attr(el, kAXTitleAttribute)) - } - - let mut texts = Vec::new(); - for child in children.iter().take(5) { - match copy_string_attr(child, kAXRoleAttribute).as_deref() { - Some("AXStaticText") => { - if let Some(text) = text_of(child) { - texts.push(text); - } - } - Some("AXCell") | Some("AXGroup") => { - for grandchild in - copy_ax_array_prefix(child, kAXChildrenAttribute, 5).unwrap_or_default() - { - if copy_string_attr(&grandchild, kAXRoleAttribute).as_deref() - == Some("AXStaticText") - && let Some(text) = text_of(&grandchild) - { - texts.push(text); - } - } - } - _ => {} - } - } - texts - } } #[cfg(not(target_os = "macos"))] diff --git a/crates/macos/src/tree/query.rs b/crates/macos/src/tree/query.rs deleted file mode 100644 index 316b5f2..0000000 --- a/crates/macos/src/tree/query.rs +++ /dev/null @@ -1,213 +0,0 @@ -use agent_desktop_core::{ - adapter::NativeHandle, - error::AdapterError, - locator::{self, LocatorQuery, NodeMatchContext}, - node::Rect, -}; -use core_foundation::base::{CFRetain, CFTypeRef}; -use rustc_hash::FxHashSet; - -use super::{ - AXElement, NodeAttrs, copy_ax_array, - element::{element_for_pid, fetch_node_attrs}, - native_id::meaningful_native_id, - roles, - state_reader::{self, StateReaderContext}, -}; -use accessibility_sys::kAXChildrenAttribute; - -const DEFAULT_QUERY_DEPTH: u8 = 10; - -pub fn resolve_query_impl( - query: &LocatorQuery, - pid: i32, - scope: Option<&NativeHandle>, -) -> Result, AdapterError> { - query.validate_states()?; - let root = scope_root(scope, pid)?; - let mut matches = Vec::new(); - let mut ancestors = FxHashSet::default(); - collect_matches( - &root, - query, - 0, - DEFAULT_QUERY_DEPTH, - &mut ancestors, - &mut matches, - None, - )?; - Ok(matches) -} - -fn scope_root(scope: Option<&NativeHandle>, pid: i32) -> Result { - let Some(scope) = scope else { - return Ok(element_for_pid(pid)); - }; - Ok(scope_element(scope)) -} - -fn scope_element(handle: &NativeHandle) -> AXElement { - let el = AXElement(handle.as_raw() as accessibility_sys::AXUIElementRef); - unsafe { CFRetain(el.0 as CFTypeRef) }; - el -} - -fn collect_matches( - el: &AXElement, - query: &LocatorQuery, - depth: u8, - max_depth: u8, - ancestors: &mut FxHashSet, - matches: &mut Vec, - window_bounds: Option, -) -> Result<(), AdapterError> { - if depth > max_depth { - return Ok(()); - } - let ptr_key = el.0 as usize; - if !ancestors.insert(ptr_key) { - return Ok(()); - } - - let attrs = fetch_node_attrs(el); - let role = roles::ax_role_to_str(attrs.role.as_deref().unwrap_or("")).to_string(); - - if element_matches( - el, - &attrs, - &role, - query, - window_bounds, - max_depth.saturating_sub(depth), - )? { - matches.push(retained_handle(el.clone())?); - } - - let child_window_bounds = window_bounds_for_children(&attrs, window_bounds); - - if let Some(children) = copy_ax_array(el, kAXChildrenAttribute) { - for child in &children { - collect_matches( - child, - query, - depth + 1, - max_depth, - ancestors, - matches, - child_window_bounds, - )?; - } - } - - ancestors.remove(&ptr_key); - Ok(()) -} - -/// Mirrors `builder.rs`'s window-bounds inheritance: a node's own state is -/// computed against the window bounds inherited from its ancestors, but its -/// *children* see the current node's own bounds once that node is itself the -/// window (`AXWindow`). Kept pure and file-local so it stays independently -/// testable without an AX round trip. -fn window_bounds_for_children(attrs: &NodeAttrs, inherited: Option) -> Option { - if attrs.role.as_deref() == Some("AXWindow") { - attrs.bounds.or(inherited) - } else { - inherited - } -} - -fn element_matches( - el: &AXElement, - attrs: &NodeAttrs, - role: &str, - query: &LocatorQuery, - window_bounds: Option, - remaining_depth: u8, -) -> Result { - if !locator::role_matches(query, role) { - return Ok(false); - } - let state_ctx = StateReaderContext { - focused: None, - window_bounds, - is_secure_text: attrs.role.as_deref() == Some("AXSecureTextField"), - }; - let states = state_reader::states_from_element(el, attrs, role, &state_ctx); - let children = if query.containment.has.is_some() || query.containment.has_not.is_some() { - build_child_nodes(el, remaining_depth, window_bounds) - } else { - Vec::new() - }; - let native_id = meaningful_native_id(attrs.native_id.clone()); - let ctx = NodeMatchContext { - role, - name: attrs.title.as_deref(), - description: attrs.description.as_deref(), - native_id: native_id.as_deref(), - value: attrs.value.as_deref(), - states: &states, - children: &children, - }; - Ok(locator::node_matches(query, ctx)) -} - -fn build_child_nodes( - el: &AXElement, - max_depth: u8, - window_bounds: Option, -) -> Vec { - let Some(children) = copy_ax_array(el, kAXChildrenAttribute) else { - return Vec::new(); - }; - children - .iter() - .filter_map(|child| ax_node_shallow(child, max_depth.saturating_sub(1), window_bounds)) - .collect() -} - -fn ax_node_shallow( - el: &AXElement, - remaining_depth: u8, - window_bounds: Option, -) -> Option { - let attrs = fetch_node_attrs(el); - let role = roles::ax_role_to_str(attrs.role.as_deref().unwrap_or("")).to_string(); - let state_ctx = StateReaderContext { - focused: None, - window_bounds, - is_secure_text: false, - }; - let states = state_reader::states_from_element(el, &attrs, &role, &state_ctx); - let children = if remaining_depth > 0 { - let child_window_bounds = window_bounds_for_children(&attrs, window_bounds); - build_child_nodes(el, remaining_depth, child_window_bounds) - } else { - Vec::new() - }; - Some(agent_desktop_core::node::AccessibilityNode { - ref_id: None, - role, - name: attrs.title.clone(), - value: attrs.value.clone(), - description: attrs.description.clone(), - native_id: meaningful_native_id(attrs.native_id.clone()), - hint: None, - states, - available_actions: vec![], - bounds: None, - children_count: None, - children, - }) -} - -fn retained_handle(candidate: AXElement) -> Result { - if candidate.0.is_null() { - return Err(AdapterError::element_not_found("element")); - } - unsafe { CFRetain(candidate.0 as CFTypeRef) }; - Ok(unsafe { NativeHandle::from_ptr(candidate.0 as *const _) }) -} - -#[cfg(test)] -#[path = "query_tests.rs"] -mod tests; diff --git a/crates/macos/src/tree/query_tests.rs b/crates/macos/src/tree/query_tests.rs deleted file mode 100644 index ce0729c..0000000 --- a/crates/macos/src/tree/query_tests.rs +++ /dev/null @@ -1,122 +0,0 @@ -use super::*; -use crate::tree::node_attrs::{NodeAttrStates, NodeAttrs}; -use agent_desktop_core::locator::StatePredicate; - -fn attrs_with_bounds(bounds: Rect) -> NodeAttrs { - NodeAttrs { - role: Some("AXButton".into()), - title: Some("Target".into()), - description: None, - value: None, - native_id: None, - states: NodeAttrStates { - enabled: true, - focused: None, - expanded: None, - disclosing: None, - selected: None, - hidden: None, - busy: None, - modal: None, - required: None, - readonly: None, - }, - bounds: Some(bounds), - has_scrollbars: false, - } -} - -fn offscreen_query() -> LocatorQuery { - LocatorQuery { - states: vec![StatePredicate { - token: "offscreen".into(), - expected: Some(true), - }], - ..Default::default() - } -} - -/// Proves the `window_bounds` this fix threads from `collect_matches` into -/// `element_matches` actually reaches `states_from_element`: a `find`/`query` -/// filtering on `states: [offscreen]` must match an element positioned -/// outside its window once real window bounds are supplied. Before this fix -/// every call site hardcoded `window_bounds: None`, so an `offscreen` state -/// filter could never match anything through `find`/`query`. -#[test] -fn element_matches_detects_offscreen_when_window_bounds_supplied() { - let el = AXElement(std::ptr::null_mut()); - let attrs = attrs_with_bounds(Rect { - x: 1000.0, - y: 0.0, - width: 10.0, - height: 10.0, - }); - let window = Rect { - x: 0.0, - y: 0.0, - width: 50.0, - height: 50.0, - }; - let query = offscreen_query(); - - let matched = element_matches(&el, &attrs, "button", &query, Some(window), 0).unwrap(); - - assert!(matched); -} - -#[test] -fn element_matches_never_flags_offscreen_without_window_bounds() { - let el = AXElement(std::ptr::null_mut()); - let attrs = attrs_with_bounds(Rect { - x: 1000.0, - y: 0.0, - width: 10.0, - height: 10.0, - }); - let query = offscreen_query(); - - let matched = element_matches(&el, &attrs, "button", &query, None, 0).unwrap(); - - assert!(!matched); -} - -#[test] -fn window_bounds_for_children_captures_window_own_bounds() { - let mut attrs = attrs_with_bounds(Rect { - x: 0.0, - y: 0.0, - width: 800.0, - height: 600.0, - }); - attrs.role = Some("AXWindow".into()); - let inherited = Some(Rect { - x: 10.0, - y: 10.0, - width: 5.0, - height: 5.0, - }); - - let result = window_bounds_for_children(&attrs, inherited); - - assert_eq!(result, attrs.bounds); -} - -#[test] -fn window_bounds_for_children_passes_through_inherited_for_non_window_roles() { - let attrs = attrs_with_bounds(Rect { - x: 0.0, - y: 0.0, - width: 800.0, - height: 600.0, - }); - let inherited = Some(Rect { - x: 10.0, - y: 10.0, - width: 5.0, - height: 5.0, - }); - - let result = window_bounds_for_children(&attrs, inherited); - - assert_eq!(result, inherited); -} diff --git a/src/cli_args/mod.rs b/src/cli_args/mod.rs index 6f564d2..ecd03dc 100644 --- a/src/cli_args/mod.rs +++ b/src/cli_args/mod.rs @@ -49,17 +49,32 @@ impl Surface { } } -#[derive(Parser, Debug, Deserialize)] +/// Window-targeting scope shared by the read/capture commands that choose which +/// window to operate on (`snapshot`, `find`, `screenshot`). Ref-based commands +/// (`click`/`type`/`get`/`is`) deliberately omit it — a ref already carries its +/// source window through its `RefEntry` — and keyboard input targets the focused +/// window, so neither needs an explicit window selector. +#[derive(Args, Debug, Deserialize, Default)] #[serde(deny_unknown_fields)] -pub(crate) struct SnapshotArgs { +pub(crate) struct WindowScope { #[arg(long, help = "Filter to application by name")] + #[serde(default)] pub app: Option, #[arg( long, name = "window-id", - help = "Filter to window ID (from list-windows)" + help = "Scope to a single window ID (from list-windows)" )] + #[serde(default)] pub window_id: Option, +} + +#[derive(Parser, Debug, Deserialize)] +#[serde(deny_unknown_fields)] +pub(crate) struct SnapshotArgs { + #[command(flatten)] + #[serde(flatten)] + pub scope: WindowScope, #[arg(long, default_value = "10", help = "Maximum tree depth")] #[serde(default = "default_max_depth")] pub max_depth: u8, @@ -172,8 +187,9 @@ pub(crate) struct FindSelectionArgs { #[derive(Parser, Debug, Deserialize)] #[serde(deny_unknown_fields)] pub(crate) struct FindArgs { - #[arg(long, help = "Filter to application by name")] - pub app: Option, + #[command(flatten)] + #[serde(flatten)] + pub scope: WindowScope, #[command(flatten)] #[serde(flatten)] pub filter: FindFilterArgs, @@ -192,14 +208,9 @@ pub(crate) struct FindArgs { #[derive(Parser, Debug, Deserialize)] #[serde(deny_unknown_fields)] pub(crate) struct ScreenshotArgs { - #[arg(long, help = "Filter to application by name")] - pub app: Option, - #[arg( - long, - name = "window-id", - help = "Filter to window ID (from list-windows)" - )] - pub window_id: Option, + #[command(flatten)] + #[serde(flatten)] + pub scope: WindowScope, #[arg( long, help = "Capture display by index (from list-displays; 0 = primary)" diff --git a/src/command_policy/mod.rs b/src/command_policy/mod.rs index d5e1030..354a6f7 100644 --- a/src/command_policy/mod.rs +++ b/src/command_policy/mod.rs @@ -29,7 +29,7 @@ pub(crate) fn policy_for(cmd: &Commands) -> PermissionNeed { | Commands::Wait(_) | Commands::ListNotifications(_) => Accessibility, - Commands::Screenshot(a) if a.app.is_some() || a.window_id.is_some() => { + Commands::Screenshot(a) if a.scope.app.is_some() || a.scope.window_id.is_some() => { AccessibilityAndScreenRecording } Commands::Screenshot(_) => ScreenRecording, diff --git a/src/command_policy/tests.rs b/src/command_policy/tests.rs index c82a990..068a3c3 100644 --- a/src/command_policy/tests.rs +++ b/src/command_policy/tests.rs @@ -1,6 +1,6 @@ use super::*; use crate::cli::{Cli, Commands}; -use crate::cli_args::{RefArgs, ScreenshotArgs, SnapshotArgs}; +use crate::cli_args::{RefArgs, ScreenshotArgs, SnapshotArgs, WindowScope}; use agent_desktop_core::{PermissionReport, PermissionState}; use clap::CommandFactory; @@ -85,8 +85,10 @@ fn command_name_is_covered(name: &str) -> bool { fn unknown_permission_does_not_mask_platform_errors() { let report = PermissionReport::default(); let command = Commands::Screenshot(ScreenshotArgs { - app: None, - window_id: None, + scope: WindowScope { + app: None, + window_id: None, + }, screen: None, output_path: None, }); @@ -104,8 +106,10 @@ fn screen_recording_denial_is_preflighted() { automation: PermissionState::NotRequired, }; let command = Commands::Screenshot(ScreenshotArgs { - app: None, - window_id: None, + scope: WindowScope { + app: None, + window_id: None, + }, screen: None, output_path: None, }); @@ -166,8 +170,10 @@ fn invalid_snapshot_root_is_rejected_before_permission_preflight() { automation: PermissionState::NotRequired, }; let command = Commands::Snapshot(SnapshotArgs { - app: None, - window_id: None, + scope: WindowScope { + app: None, + window_id: None, + }, max_depth: 10, include_bounds: false, interactive_only: false, diff --git a/src/dispatch/observation.rs b/src/dispatch/observation.rs index c087a8e..a701570 100644 --- a/src/dispatch/observation.rs +++ b/src/dispatch/observation.rs @@ -16,8 +16,8 @@ pub(super) fn dispatch( match cmd { Commands::Snapshot(a) => snapshot::execute( snapshot::SnapshotArgs { - app: a.app, - window_id: a.window_id, + app: a.scope.app, + window_id: a.scope.window_id, max_depth: a.max_depth, include_bounds: a.include_bounds, interactive_only: a.interactive_only, @@ -39,7 +39,8 @@ pub(super) fn dispatch( .collect::, _>>()?; find::execute( find::FindArgs { - app: a.app, + app: a.scope.app, + window_id: a.scope.window_id, filter: find::FindFilterArgs { role: a.filter.role, name: a.filter.name, @@ -65,8 +66,8 @@ pub(super) fn dispatch( Commands::Screenshot(a) => screenshot::execute( screenshot::ScreenshotArgs { - app: a.app, - window_id: a.window_id, + app: a.scope.app, + window_id: a.scope.window_id, screen: a.screen, output_path: a.output_path, },