From cd3b0ed29d6cb57841908a63de3cf34b168518a5 Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 4 Jun 2026 08:52:25 -0700 Subject: [PATCH] fix: preserve safe window title fallback --- crates/macos/src/tree/resolve.rs | 30 ++++++------- crates/macos/src/tree/resolve_roots.rs | 59 ++++++++++++++++++++++---- crates/macos/src/tree/resolve_tests.rs | 51 ++++++++++++++++++++-- 3 files changed, 113 insertions(+), 27 deletions(-) diff --git a/crates/macos/src/tree/resolve.rs b/crates/macos/src/tree/resolve.rs index e73857d..cc1470a 100644 --- a/crates/macos/src/tree/resolve.rs +++ b/crates/macos/src/tree/resolve.rs @@ -1,5 +1,5 @@ use agent_desktop_core::{ - adapter::{NativeHandle, SnapshotSurface}, + adapter::NativeHandle, error::{AdapterError, ErrorCode}, refs::RefEntry, }; @@ -15,7 +15,9 @@ 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, source_window_number}; +use super::resolve_roots::{ + candidate_roots, path_candidate_roots, source_window_number, source_window_scope_required, +}; #[cfg(target_os = "macos")] pub fn resolve_element_impl(entry: &RefEntry) -> Result { @@ -32,7 +34,8 @@ pub fn resolve_element_with_timeout( for attempt in 0..attempts { if can_use_path_fast_path(entry) { let path_roots = path_candidate_roots(entry, deadline)?; - match find_entry_by_path(&path_roots, entry, deadline) { + let scope_verified = path_roots.scope_verified; + match find_entry_by_path(&path_roots.roots, entry, scope_verified, deadline) { Ok(handle) => { return Ok(handle); } @@ -53,7 +56,8 @@ pub fn resolve_element_with_timeout( continue; } let roots = candidate_roots(entry, deadline)?; - match find_entry_in_roots(&roots, entry, resolve_depth, deadline) { + let scope_verified = roots.scope_verified; + match find_entry_in_roots(&roots.roots, entry, resolve_depth, scope_verified, deadline) { Ok(handle) => { return Ok(handle); } @@ -95,7 +99,7 @@ fn requires_scoped_path_resolution(entry: &RefEntry) -> bool { (entry.root_ref.is_none() || entry.path_is_absolute) && entry.bounds_hash.is_none() && !entry.path.is_empty() - && source_window_number(entry).is_some() + && source_window_scope_required(entry) } #[cfg(target_os = "macos")] @@ -107,6 +111,7 @@ fn can_use_broad_search(entry: &RefEntry) -> bool { fn find_entry_by_path( roots: &[AXElement], entry: &RefEntry, + source_window_verified: bool, deadline: Instant, ) -> Result { if entry.path.is_empty() { @@ -129,7 +134,7 @@ fn find_entry_by_path( } } - classify_candidates(matches, entry) + classify_candidates(matches, entry, source_window_verified) } #[cfg(target_os = "macos")] @@ -157,6 +162,7 @@ fn find_entry_in_roots( roots: &[AXElement], entry: &RefEntry, resolve_depth: u8, + source_window_verified: bool, deadline: Instant, ) -> Result { let mut matches = Vec::new(); @@ -176,21 +182,20 @@ fn find_entry_in_roots( }; collect_elements_recursive(root, 0, &mut context)?; } - classify_candidates(matches, entry) + classify_candidates(matches, entry, source_window_verified) } #[cfg(target_os = "macos")] fn classify_candidates( mut matches: Vec, entry: &RefEntry, + source_window_verified: bool, ) -> Result { match matches.len() { 0 => Err(AdapterError::element_not_found("element")), 1 => { let candidate = matches.remove(0); - if source_window_scope_verifies_lone_match(entry) - || verified_bounds_match(&candidate, entry) - { + if source_window_verified || verified_bounds_match(&candidate, entry) { retained_handle(candidate) } else { Err(AdapterError::element_not_found("element")) @@ -200,11 +205,6 @@ fn classify_candidates( } } -#[cfg(target_os = "macos")] -fn source_window_scope_verifies_lone_match(entry: &RefEntry) -> bool { - matches!(entry.source_surface, SnapshotSurface::Window) && source_window_number(entry).is_some() -} - #[cfg(target_os = "macos")] fn verified_bounds_match(candidate: &AXElement, entry: &RefEntry) -> bool { entry.bounds_hash.is_some() && bounds_match(candidate, entry) diff --git a/crates/macos/src/tree/resolve_roots.rs b/crates/macos/src/tree/resolve_roots.rs index 312bd3c..f26fd6d 100644 --- a/crates/macos/src/tree/resolve_roots.rs +++ b/crates/macos/src/tree/resolve_roots.rs @@ -9,26 +9,34 @@ 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) struct CandidateRoots { + pub roots: Vec, + pub scope_verified: bool, +} + #[cfg(target_os = "macos")] pub(super) fn path_candidate_roots( entry: &RefEntry, deadline: Instant, -) -> Result, AdapterError> { +) -> Result { if entry.bounds_hash.is_some() { return candidate_roots(entry, deadline); } - Ok(scoped_surface_root(entry, deadline)?.into_iter().collect()) + let roots: Vec<_> = scoped_surface_root(entry, deadline)?.into_iter().collect(); + Ok(CandidateRoots { + scope_verified: source_window_scope_required(entry) && !roots.is_empty(), + roots, + }) } #[cfg(target_os = "macos")] pub(super) fn candidate_roots( entry: &RefEntry, deadline: Instant, -) -> Result, AdapterError> { +) -> Result { if source_window_scope_required(entry) { - return Ok(exact_source_window_number_root(entry, deadline)? - .into_iter() - .collect()); + return source_window_scoped_roots(entry, deadline); } let root = element_for_pid(entry.pid); @@ -61,7 +69,35 @@ pub(super) fn candidate_roots( if roots.is_empty() { roots.push(root); } - Ok(roots) + Ok(CandidateRoots { + roots, + scope_verified: false, + }) +} + +#[cfg(target_os = "macos")] +fn source_window_scoped_roots( + entry: &RefEntry, + deadline: Instant, +) -> Result { + if let Some(window) = exact_source_window_number_root(entry, deadline)? { + return Ok(CandidateRoots { + roots: vec![window], + scope_verified: true, + }); + } + if source_window_title_fallback_allowed(entry) { + return Ok(CandidateRoots { + roots: exact_source_window_root(entry, deadline)? + .into_iter() + .collect(), + scope_verified: false, + }); + } + Ok(CandidateRoots { + roots: Vec::new(), + scope_verified: false, + }) } #[cfg(target_os = "macos")] @@ -132,10 +168,17 @@ fn exact_source_window_root( } #[cfg(target_os = "macos")] -fn source_window_scope_required(entry: &RefEntry) -> bool { +pub(super) fn source_window_scope_required(entry: &RefEntry) -> bool { matches!(entry.source_surface, SnapshotSurface::Window) && source_window_number(entry).is_some() } +#[cfg(target_os = "macos")] +pub(super) fn source_window_title_fallback_allowed(entry: &RefEntry) -> bool { + source_window_scope_required(entry) + && entry.bounds_hash.is_some() + && entry.source_window_title.is_some() +} + #[cfg(target_os = "macos")] pub(super) fn source_window_number(entry: &RefEntry) -> Option { entry diff --git a/crates/macos/src/tree/resolve_tests.rs b/crates/macos/src/tree/resolve_tests.rs index 8cafd7f..d249328 100644 --- a/crates/macos/src/tree/resolve_tests.rs +++ b/crates/macos/src/tree/resolve_tests.rs @@ -1,5 +1,6 @@ use super::*; -use crate::tree::resolve_roots::source_window_number; +use crate::tree::resolve_roots::{source_window_number, source_window_title_fallback_allowed}; +use agent_desktop_core::adapter::SnapshotSurface; fn entry( bounds_hash: Option, @@ -20,7 +21,7 @@ fn entry( source_app: None, source_window_id: source_window_id.map(String::from), source_window_title: source_window_title.map(String::from), - source_surface: agent_desktop_core::adapter::SnapshotSurface::Window, + source_surface: SnapshotSurface::Window, root_ref: root_ref.map(String::from), path_is_absolute: false, path: smallvec::smallvec![0, 1], @@ -155,6 +156,7 @@ fn expired_deadline_fails_before_path_resolution_reads() { let err = match find_entry_by_path( &[], &entry(Some(42), Some("w-42"), Some("Documents"), None), + false, std::time::Instant::now(), ) { Ok(_) => panic!("expected timeout"), @@ -172,6 +174,7 @@ fn ambiguous_candidate_classification_reports_structured_details() { AXElement(std::ptr::null_mut()), ], &entry(None, Some("w-42"), Some("Documents"), None), + true, ) { Ok(_) => panic!("expected ambiguous target"), Err(err) => err, @@ -189,15 +192,17 @@ fn single_meaningful_identity_candidate_resolves_after_bounds_change() { let _handle = classify_candidates( vec![AXElement(std::ptr::null_mut())], &entry(None, Some("w-42"), Some("Documents"), None), + true, ) .expect("unique identity should resolve within the verified source window"); } #[test] -fn single_identity_candidate_without_window_or_bounds_fails_closed() { +fn cross_window_replacement_without_verified_source_window_fails_closed() { let err = match classify_candidates( vec![AXElement(std::ptr::null_mut())], - &entry(None, None, Some("Documents"), None), + &entry(None, Some("w-42"), Some("Documents"), None), + false, ) { Ok(_) => panic!("expected stale candidate to fail closed"), Err(err) => err, @@ -206,6 +211,44 @@ fn single_identity_candidate_without_window_or_bounds_fails_closed() { assert_eq!(err.code, ErrorCode::ElementNotFound); } +#[test] +fn non_window_identity_candidate_without_bounds_fails_closed() { + let mut menu_entry = entry(None, None, None, None); + menu_entry.source_surface = SnapshotSurface::Menu; + + let err = match classify_candidates(vec![AXElement(std::ptr::null_mut())], &menu_entry, false) { + Ok(_) => panic!("expected stale candidate to fail closed"), + Err(err) => err, + }; + + assert_eq!(err.code, ErrorCode::ElementNotFound); +} + +#[test] +fn source_window_title_fallback_requires_bounds_hash() { + assert!(source_window_title_fallback_allowed(&entry( + Some(42), + Some("w-10"), + Some("Documents"), + None + ))); + assert!(!source_window_title_fallback_allowed(&entry( + None, + Some("w-10"), + Some("Documents"), + None + ))); + assert!(!source_window_title_fallback_allowed(&entry( + Some(42), + Some("w-10"), + None, + None + ))); + let mut menu_entry = entry(Some(42), Some("w-10"), Some("Documents"), None); + menu_entry.source_surface = SnapshotSurface::Menu; + assert!(!source_window_title_fallback_allowed(&menu_entry)); +} + #[test] fn bounds_hash_keeps_collecting_to_disambiguate_identity_matches() { assert!(!should_stop_collecting(