From 6f50b418652ba44bb73afccdb69980fe0c59ddaa Mon Sep 17 00:00:00 2001 From: Lahfir Date: Thu, 4 Jun 2026 10:47:15 -0700 Subject: [PATCH] fix: make explicit snapshots session-independent --- CONCEPTS.md | 8 +- README.md | 8 +- crates/core/src/commands/helpers.rs | 5 +- crates/core/src/commands/helpers_tests.rs | 41 ++++++ .../core/src/commands/wait_element_tests.rs | 38 +++++- crates/core/src/error.rs | 4 +- crates/core/src/refs_store.rs | 99 ++++++++++++-- crates/core/src/refs_store_tests.rs | 91 +++++++++++++ crates/core/src/snapshot_ref_tests.rs | 32 +++++ crates/macos/src/tree/resolve_roots.rs | 126 +++++++++++++----- crates/macos/src/tree/resolve_tests.rs | 26 ++-- skills/agent-desktop/SKILL.md | 4 +- .../references/commands-interaction.md | 2 +- .../references/commands-system.md | 2 +- src/cli/help_after.txt | 9 +- src/cli/help_before.txt | 2 +- src/cli/mod.rs | 2 +- src/cli_args/actions.rs | 12 +- src/cli_args/mod.rs | 6 +- src/cli_args/system.rs | 2 +- tests/conformance/README.md | 5 +- 21 files changed, 427 insertions(+), 97 deletions(-) diff --git a/CONCEPTS.md b/CONCEPTS.md index e6cfe7a..afe0d0b 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -11,7 +11,7 @@ A structured representation of an application's user interface exposed by the op 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. +A compact identifier for one persisted snapshot. Explicit snapshot IDs are direct handles; callers do not need the original session when they pass the ID. ### 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. @@ -40,9 +40,9 @@ Strict ref resolution rejects missing, stale, and ambiguous matches instead of g ## Coordination ### Session -A namespace for snapshots, ref maps, and the latest-snapshot pointer shared by one agent or a coordinated group of agents. +A coordination key for one agent or a coordinated group of agents that share a latest-snapshot pointer. -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. +Use sessions when callers intentionally omit `--snapshot` and want a shared latest observation. Explicit snapshot IDs remain the deterministic path for pinned actions and can be resolved without also passing the session. ## Action Reliability @@ -76,4 +76,4 @@ The requirement that language bindings using refs follow the same strict resolut ## 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. +A session owns one latest-snapshot pointer. A snapshot persists a ref map and can be selected directly by snapshot ID. 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/README.md b/README.md index 3d078db..8ba5f54 100644 --- a/README.md +++ b/README.md @@ -158,7 +158,7 @@ Agent loop: snapshot → decide → act → snapshot → decide → act → ... ### Shared sessions for multi-agent workflows -Use the same `--session ` when multiple agents coordinate on one desktop task. A session is the ref storage namespace, not a security boundary; each snapshot in that session gets its own `snapshot_id`, and the session also keeps a latest-snapshot pointer. Pass `--snapshot ` when an agent must act on a specific observation, or omit it when the agent should use that session's latest snapshot. +Use the same `--session ` when multiple agents coordinate on one desktop task. A session owns a latest-snapshot pointer, not a security boundary. Each snapshot gets its own `snapshot_id`; pass `--snapshot ` when an agent must act on a specific observation. Explicit snapshot IDs can be used without repeating `--session`; keep `--session` when you omit `--snapshot` and want that session's latest snapshot. ```mermaid flowchart LR @@ -167,12 +167,14 @@ flowchart LR A --> C["Agent A: click @e4 --snapshot s1"] B --> D["Agent B: wait --element @e9 --predicate actionable"] S --> E["latest_snapshot_id points at newest snapshot"] + C --> F["Explicit snapshot id works outside session too"] ``` ```bash agent-desktop --session release-fix snapshot --app Xcode -i --compact agent-desktop --session release-fix wait --element @e9 --predicate actionable --timeout 5000 agent-desktop --session release-fix click @e9 +agent-desktop click @e9 --snapshot s2 ``` ## Commands @@ -379,7 +381,7 @@ Errors include machine-readable codes and recovery hints: ## Ref System -`snapshot` assigns refs to interactive elements in depth-first order: `@e1`, `@e2`, `@e3`, etc. Refs are scoped to a compact `snapshot_id` such as `s8f3k2p9`. Commands can omit `--snapshot` to use the latest snapshot pointer, but passing the ID is more deterministic in multi-step flows. +`snapshot` assigns refs to interactive elements in depth-first order: `@e1`, `@e2`, `@e3`, etc. Refs are scoped to a compact `snapshot_id` such as `s8f3k2p9`. Commands can omit `--snapshot` to use the active session's latest snapshot pointer, but passing the ID is more deterministic in multi-step flows and does not require also passing `--session`. Interactive roles that receive refs: `button`, `textfield`, `checkbox`, `link`, `menuitem`, `tab`, `slider`, `combobox`, `treeitem`, `cell`, `radiobutton`, `incrementor`, `menubutton`, `switch`, `colorwell`, `dockitem`. @@ -387,7 +389,7 @@ Static elements (labels, groups, containers) appear in the tree for context but Reliability contract: -- `--session ` scopes snapshots, refs, and the latest snapshot pointer to one caller or agent team. +- `--session ` scopes the latest snapshot pointer to one caller or agent team; explicit `--snapshot ` resolves the saved snapshot directly. - Ref actions re-identify targets at action time: a moved unique target can proceed, while missing or changed identity returns `STALE_REF`. - Multiple plausible targets return `AMBIGUOUS_TARGET` instead of choosing arbitrarily. - Actions run an actionability preflight before dispatch: visibility, stability, enabled state, supported action, policy, and editability. diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 3285b82..990eb74 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -50,17 +50,16 @@ pub(crate) fn resolve_ref_with_context<'a>( "ref.resolve.start", || json!({ "ref": ref_id, "snapshot_id": snapshot_id }), )?; - let refmap = store.load(snapshot_id).map_err(|e| { + let refmap = store.load(snapshot_id).inspect_err(|e| { tracing::debug!("refmap load failed: {e}"); let _ = context.trace_lazy("ref.resolve.error", || { json!({ "ref": ref_id, "snapshot_id": snapshot_id, - "code": "STALE_REF", + "code": e.code(), "message": e.to_string() }) }); - AppError::stale_ref(ref_id) })?; let entry = match refmap.get(ref_id) { Some(entry) => entry.clone(), diff --git a/crates/core/src/commands/helpers_tests.rs b/crates/core/src/commands/helpers_tests.rs index 81adf8c..4333096 100644 --- a/crates/core/src/commands/helpers_tests.rs +++ b/crates/core/src/commands/helpers_tests.rs @@ -152,6 +152,47 @@ fn resolved_element_releases_handle_once_on_drop() { assert_eq!(adapter.releases.load(Ordering::SeqCst), 1); } +#[test] +fn explicit_session_snapshot_resolves_without_session_context() { + let _guard = HomeGuard::new(); + let mut refmap = RefMap::new(); + refmap.allocate(entry()); + let snapshot_id = RefStore::for_session(Some("agent-a")) + .unwrap() + .save_new_snapshot(&refmap) + .unwrap(); + let adapter = ReleaseCountingAdapter { + releases: AtomicU32::new(0), + }; + + let (_entry, resolved) = resolve_ref_with_context( + "@e1", + Some(&snapshot_id), + &adapter, + &CommandContext::default(), + ) + .unwrap(); + let _handle = resolved.handle(); + + assert_eq!(adapter.releases.load(Ordering::SeqCst), 0); +} + +#[test] +fn missing_snapshot_keeps_snapshot_not_found_error() { + let _guard = HomeGuard::new(); + let adapter = ReleaseCountingAdapter { + releases: AtomicU32::new(0), + }; + + let err = match resolve_ref("@e1", Some("smissing"), &adapter) { + Ok(_) => panic!("expected missing snapshot to fail"), + Err(err) => err, + }; + + assert_eq!(err.code(), "SNAPSHOT_NOT_FOUND"); + assert!(err.suggestion().unwrap().contains("snapshot_id")); +} + #[test] fn execute_ref_action_preserves_action_and_policy() { let _guard = HomeGuard::new(); diff --git a/crates/core/src/commands/wait_element_tests.rs b/crates/core/src/commands/wait_element_tests.rs index 4683c26..a399873 100644 --- a/crates/core/src/commands/wait_element_tests.rs +++ b/crates/core/src/commands/wait_element_tests.rs @@ -118,6 +118,14 @@ fn snapshot_with_disabled_ref() -> String { } fn save_ref(states: Vec) -> String { + save_ref_in_store(RefStore::new().unwrap(), states) +} + +fn save_ref_in_session(session_id: &str, states: Vec) -> String { + save_ref_in_store(RefStore::for_session(Some(session_id)).unwrap(), states) +} + +fn save_ref_in_store(store: RefStore, states: Vec) -> String { let mut refmap = RefMap::new(); refmap.allocate(RefEntry { pid: 1, @@ -137,7 +145,7 @@ fn save_ref(states: Vec) -> String { path_is_absolute: false, path: smallvec::SmallVec::new(), }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() + store.save_new_snapshot(&refmap).unwrap() } #[test] @@ -187,6 +195,34 @@ fn element_wait_enabled_predicate_uses_live_state() { assert_eq!(value["observed"]["enabled"], true); } +#[test] +fn element_wait_explicit_session_snapshot_without_session_context() { + let _guard = HomeGuard::new(); + let snapshot_id = save_ref_in_session("agent-a", Vec::new()); + let adapter = PredicateAdapter { + state: Some(ElementState { + role: "button".into(), + states: vec![], + value: None, + }), + value: None, + bounds: None, + }; + + let value = wait_for_element_test( + "@e1".into(), + Some(snapshot_id), + wait_predicate::ElementPredicate::Exists, + 50, + &adapter, + &crate::context::CommandContext::default(), + ) + .unwrap(); + + assert_eq!(value["found"], true); + assert_eq!(value["predicate"], "exists"); +} + #[test] fn element_wait_value_predicate_matches_live_value_without_leaking_it() { let _guard = HomeGuard::new(); diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index 99d24c4..6e53ef1 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -93,7 +93,7 @@ impl AdapterError { format!("{ref_id} not found in current RefMap"), ) .with_suggestion( - "Run 'snapshot' (or 'snapshot --skeleton') to refresh, then retry with updated ref", + "Use the snapshot_id returned with this ref, or run 'snapshot' / 'snapshot --skeleton' again for fresh refs", ) } @@ -150,7 +150,7 @@ impl AdapterError { ErrorCode::SnapshotNotFound, format!("Snapshot '{snapshot_id}' not found"), ) - .with_suggestion("Run 'snapshot' again and retry with the returned snapshot_id") + .with_suggestion("Run 'snapshot' again and retry with the returned snapshot_id; if you omitted --snapshot, use the same --session as the snapshot") } pub fn policy_denied(message: impl Into) -> Self { diff --git a/crates/core/src/refs_store.rs b/crates/core/src/refs_store.rs index 63bdb39..23ee0bb 100644 --- a/crates/core/src/refs_store.rs +++ b/crates/core/src/refs_store.rs @@ -4,8 +4,8 @@ use crate::{ refs::{RefMap, home_dir, new_snapshot_id, validate_snapshot_id, write_private_file}, refs_lock::RefStoreLock, }; -use std::io::Read; -use std::path::PathBuf; +use std::io::{ErrorKind, Read}; +use std::path::{Path, PathBuf}; const LATEST_SNAPSHOT_FILE: &str = "latest_snapshot_id"; const MAX_SAVED_SNAPSHOTS: usize = 512; @@ -64,7 +64,18 @@ impl RefStore { snapshot_id: &str, refmap: &RefMap, ) -> Result<(), AppError> { - self.with_write_lock(|| self.save_snapshot_unlocked(snapshot_id, refmap)) + validate_snapshot_id(snapshot_id)?; + let base_dir = if self.snapshot_path(snapshot_id).is_file() { + self.base_dir.clone() + } else { + self.discover_snapshot_base(snapshot_id)? + .unwrap_or_else(|| self.base_dir.clone()) + }; + let store = Self { + base_dir, + allow_legacy_migration: false, + }; + store.with_write_lock(|| store.save_snapshot_unlocked(snapshot_id, refmap)) } pub fn load(&self, snapshot_id: Option<&str>) -> Result { @@ -78,7 +89,7 @@ impl RefStore { if let Ok(id) = std::fs::read_to_string(self.latest_path()) { let id = id.trim(); if !id.is_empty() { - return self.load_snapshot(id); + return self.load_snapshot_from_base(&self.base_dir, id); } } if let Some(refmap) = self.migrate_legacy_latest()? { @@ -91,9 +102,38 @@ impl RefStore { pub fn load_snapshot(&self, snapshot_id: &str) -> Result { validate_snapshot_id(snapshot_id)?; - let path = self.snapshot_path(snapshot_id); - let mut file = std::fs::File::open(&path) - .map_err(|_| AppError::Adapter(AdapterError::snapshot_not_found(snapshot_id)))?; + if let Some(refmap) = self.read_snapshot_if_present(&self.base_dir, snapshot_id)? { + return Ok(refmap); + } + match self.discover_snapshot_base(snapshot_id)? { + Some(base_dir) => self.load_snapshot_from_base(&base_dir, snapshot_id), + None => Err(AppError::Adapter(AdapterError::snapshot_not_found( + snapshot_id, + ))), + } + } + + fn load_snapshot_from_base( + &self, + base_dir: &Path, + snapshot_id: &str, + ) -> Result { + validate_snapshot_id(snapshot_id)?; + self.read_snapshot_if_present(base_dir, snapshot_id)? + .ok_or_else(|| AppError::Adapter(AdapterError::snapshot_not_found(snapshot_id))) + } + + fn read_snapshot_if_present( + &self, + base_dir: &Path, + snapshot_id: &str, + ) -> Result, AppError> { + let path = Self::snapshot_path_for_base(base_dir, snapshot_id); + let mut file = match std::fs::File::open(&path) { + Ok(file) => file, + Err(err) if err.kind() == ErrorKind::NotFound => return Ok(None), + Err(err) => return Err(err.into()), + }; let metadata = file.metadata()?; if metadata.len() > crate::refs::MAX_REFMAP_BYTES { return Err(AppError::Internal( @@ -107,7 +147,7 @@ impl RefStore { "RefMap file exceeds 1MB size limit".into(), )); } - Ok(serde_json::from_str(&json)?) + Ok(Some(serde_json::from_str(&json)?)) } pub fn set_latest(&self, snapshot_id: &str) -> Result<(), AppError> { @@ -138,7 +178,11 @@ impl RefStore { } fn snapshot_path(&self, snapshot_id: &str) -> PathBuf { - self.base_dir + Self::snapshot_path_for_base(&self.base_dir, snapshot_id) + } + + fn snapshot_path_for_base(base_dir: &Path, snapshot_id: &str) -> PathBuf { + base_dir .join("snapshots") .join(snapshot_id) .join("refmap.json") @@ -188,6 +232,41 @@ impl RefStore { self.base_dir.join("refstore.lock") } + fn discover_snapshot_base(&self, snapshot_id: &str) -> Result, AppError> { + let home = + home_dir().ok_or_else(|| AppError::Internal("HOME directory not found".into()))?; + let agent_dir = home.join(".agent-desktop"); + let mut matches = Vec::new(); + if agent_dir != self.base_dir + && Self::snapshot_path_for_base(&agent_dir, snapshot_id).is_file() + { + matches.push(agent_dir.clone()); + } + let sessions_dir = agent_dir.join("sessions"); + let Ok(entries) = std::fs::read_dir(sessions_dir) else { + return Ok(matches.into_iter().next()); + }; + for entry in entries.flatten() { + let path = entry.path(); + if path == self.base_dir { + continue; + } + let Ok(file_type) = entry.file_type() else { + continue; + }; + if file_type.is_dir() && Self::snapshot_path_for_base(&path, snapshot_id).is_file() { + matches.push(path); + } + } + if matches.len() > 1 { + return Err(AppError::invalid_input_with_suggestion( + format!("Snapshot '{snapshot_id}' exists in more than one session"), + "Pass the matching --session for this rare snapshot id collision.", + )); + } + Ok(matches.into_iter().next()) + } + fn with_write_lock(&self, f: impl FnOnce() -> Result) -> Result { let _lock = RefStoreLock::acquire(&self.lock_path())?; f() @@ -201,7 +280,7 @@ impl RefStore { if let Ok(id) = std::fs::read_to_string(self.latest_path()) { let id = id.trim(); if !id.is_empty() { - return self.load_snapshot(id).map(Some); + return self.load_snapshot_from_base(&self.base_dir, id).map(Some); } } let refmap = match RefMap::load() { diff --git a/crates/core/src/refs_store_tests.rs b/crates/core/src/refs_store_tests.rs index e4a2907..84f0733 100644 --- a/crates/core/src/refs_store_tests.rs +++ b/crates/core/src/refs_store_tests.rs @@ -88,6 +88,97 @@ fn sessions_are_isolated_from_default_store() { ); } +#[test] +fn explicit_snapshot_id_loads_across_session_namespaces() { + 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 snapshot_id = session_a.save_new_snapshot(&map_with("Session A")).unwrap(); + + assert_eq!( + default_store + .load(Some(&snapshot_id)) + .unwrap() + .get("@e1") + .unwrap() + .name + .as_deref(), + Some("Session A") + ); + assert_eq!( + session_b + .load(Some(&snapshot_id)) + .unwrap() + .get("@e1") + .unwrap() + .name + .as_deref(), + Some("Session A") + ); + assert!(default_store.load(None).is_err()); + assert!(session_b.load(None).is_err()); +} + +#[test] +fn save_existing_snapshot_updates_discovered_owner_without_promoting_latest() { + let _guard = HomeGuard::new(); + let default_store = RefStore::new().unwrap(); + let session_a = RefStore::for_session(Some("agent-a")).unwrap(); + + let snapshot_id = session_a.save_new_snapshot(&map_with("Session A")).unwrap(); + default_store + .save_existing_snapshot(&snapshot_id, &map_with("Updated")) + .unwrap(); + + assert_eq!( + session_a + .load(Some(&snapshot_id)) + .unwrap() + .get("@e1") + .unwrap() + .name + .as_deref(), + Some("Updated") + ); + assert!(default_store.latest_snapshot_id().is_none()); + assert_eq!( + session_a.latest_snapshot_id().as_deref(), + Some(snapshot_id.as_str()) + ); +} + +#[test] +fn duplicate_explicit_snapshot_id_requires_session() { + 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(); + + session_a + .save_snapshot("sdup1", &map_with("Session A")) + .unwrap(); + session_b + .save_snapshot("sdup1", &map_with("Session B")) + .unwrap(); + + let err = default_store.load(Some("sdup1")).unwrap_err(); + + assert_eq!(err.code(), "INVALID_ARGS"); + assert!(err.suggestion().unwrap().contains("--session")); + assert_eq!( + session_a + .load(Some("sdup1")) + .unwrap() + .get("@e1") + .unwrap() + .name + .as_deref(), + Some("Session A") + ); +} + #[test] fn save_existing_snapshot_does_not_promote_latest_pointer() { let _guard = HomeGuard::new(); diff --git a/crates/core/src/snapshot_ref_tests.rs b/crates/core/src/snapshot_ref_tests.rs index 67c4796..b0b267e 100644 --- a/crates/core/src/snapshot_ref_tests.rs +++ b/crates/core/src/snapshot_ref_tests.rs @@ -98,6 +98,13 @@ fn save_latest(refmap: RefMap) -> String { .expect("snapshot refmap should save") } +fn save_session(session_id: &str, refmap: RefMap) -> String { + RefStore::for_session(Some(session_id)) + .unwrap() + .save_new_snapshot(&refmap) + .expect("session snapshot refmap should save") +} + fn load_latest() -> RefMap { RefStore::new() .unwrap() @@ -105,6 +112,13 @@ fn load_latest() -> RefMap { .expect("latest snapshot should load") } +fn load_session_snapshot(session_id: &str, snapshot_id: &str) -> RefMap { + RefStore::for_session(Some(session_id)) + .unwrap() + .load(Some(snapshot_id)) + .expect("session snapshot should load") +} + fn seed_skeleton_refmap() -> RefMap { let mut map = RefMap::new(); let anchor = ref_entry_from_node( @@ -176,6 +190,24 @@ fn test_run_from_ref_returns_subtree_and_persists_refs() { assert_eq!(adapter.release_calls.load(Ordering::SeqCst), 1); } +#[test] +fn test_run_from_ref_explicit_session_snapshot_without_session_context() { + let _guard = HomeGuard::new(); + let snapshot_id = save_session("agent-a", seed_skeleton_refmap()); + + let adapter = StubAdapter::new(named("button", "Save")); + let result = run_from_ref(&adapter, &drill_opts(), "@e1", Some(&snapshot_id)) + .expect("explicit snapshot should drill without repeating --session"); + + assert_eq!(result.snapshot_id.as_deref(), Some(snapshot_id.as_str())); + let on_disk = load_session_snapshot("agent-a", &snapshot_id); + assert!(on_disk.get("@e1").is_some(), "skeleton anchor preserved"); + let drill_ref = result.tree.ref_id.as_deref().expect("drill ref"); + let drill_entry = on_disk.get(drill_ref).expect("drill ref persisted"); + assert_eq!(drill_entry.name.as_deref(), Some("Save")); + assert!(RefStore::new().unwrap().latest_snapshot_id().is_none()); +} + #[test] fn test_run_from_ref_releases_handle_when_subtree_read_fails() { let _guard = HomeGuard::new(); diff --git a/crates/macos/src/tree/resolve_roots.rs b/crates/macos/src/tree/resolve_roots.rs index f26fd6d..1c361b0 100644 --- a/crates/macos/src/tree/resolve_roots.rs +++ b/crates/macos/src/tree/resolve_roots.rs @@ -80,19 +80,25 @@ fn source_window_scoped_roots( entry: &RefEntry, deadline: Instant, ) -> Result { - if let Some(window) = exact_source_window_number_root(entry, deadline)? { + let Some(windows) = windows_for_pid(entry.pid, deadline)? else { + return Ok(CandidateRoots { + roots: Vec::new(), + scope_verified: false, + }); + }; + if let Some(window) = window_by_number(&windows, source_window_number(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, - }); + if single_window_fallback_allowed(entry) { + if let Some(window) = fallback_source_window_root(&windows, entry, deadline) { + return Ok(CandidateRoots { + roots: vec![window], + scope_verified: false, + }); + } } Ok(CandidateRoots { roots: Vec::new(), @@ -126,18 +132,14 @@ fn exact_source_window_number_root( entry: &RefEntry, deadline: Instant, ) -> Result, AdapterError> { - let Some(source_window_number) = source_window_number(entry) else { + let Some(windows) = windows_for_pid(entry.pid, deadline)? else { return Ok(None); }; - let root = element_for_pid(entry.pid); - prepare_for_read(&root, deadline)?; - let Some(windows) = copy_ax_array(&root, "AXWindows") else { - return Ok(None); - }; - Ok(windows.into_iter().find(|win| { - prepare_for_read(win, deadline).is_ok() - && copy_i64_attr(win, "AXWindowNumber") == Some(source_window_number) - })) + Ok(window_by_number( + &windows, + source_window_number(entry), + deadline, + )) } #[cfg(target_os = "macos")] @@ -145,26 +147,80 @@ fn exact_source_window_root( entry: &RefEntry, deadline: Instant, ) -> Result, AdapterError> { - let root = element_for_pid(entry.pid); - prepare_for_read(&root, deadline)?; - let Some(windows) = copy_ax_array(&root, "AXWindows") else { + let Some(windows) = windows_for_pid(entry.pid, deadline)? else { return Ok(None); }; - if let Some(source_window_number) = source_window_number(entry) { - if let Some(window) = windows.iter().find(|win| { + if let Some(window) = window_by_number(&windows, source_window_number(entry), deadline) { + return Ok(Some(window)); + } + Ok(window_by_title( + &windows, + entry.source_window_title.as_deref(), + deadline, + )) +} + +#[cfg(target_os = "macos")] +fn windows_for_pid(pid: i32, deadline: Instant) -> Result>, AdapterError> { + let root = element_for_pid(pid); + prepare_for_read(&root, deadline)?; + Ok(copy_ax_array(&root, "AXWindows")) +} + +#[cfg(target_os = "macos")] +fn window_by_number( + windows: &[AXElement], + source_window_number: Option, + deadline: Instant, +) -> Option { + let source_window_number = source_window_number?; + 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())); - } + }) + .cloned() +} + +#[cfg(target_os = "macos")] +fn window_by_title( + windows: &[AXElement], + source_window_title: Option<&str>, + deadline: Instant, +) -> Option { + let source_window_title = source_window_title?; + windows + .iter() + .find(|win| { + prepare_for_read(win, deadline).is_ok() + && copy_string_attr(win, "AXTitle").as_deref() == Some(source_window_title) + }) + .cloned() +} + +#[cfg(target_os = "macos")] +fn fallback_source_window_root( + windows: &[AXElement], + entry: &RefEntry, + deadline: Instant, +) -> Option { + if let Some(window) = window_by_title(windows, entry.source_window_title.as_deref(), deadline) { + return Some(window); } - let Some(source_window_title) = entry.source_window_title.as_deref() else { - return Ok(None); - }; - Ok(windows.into_iter().find(|win| { + if !single_window_fallback_allowed(entry) { + return None; + } + let mut candidates = windows.iter().filter(|win| { prepare_for_read(win, deadline).is_ok() - && copy_string_attr(win, "AXTitle").as_deref() == Some(source_window_title) - })) + && copy_string_attr(win, "AXRole").as_deref() == Some("AXWindow") + }); + let first = candidates.next()?; + if candidates.next().is_none() { + Some(first.clone()) + } else { + None + } } #[cfg(target_os = "macos")] @@ -173,10 +229,8 @@ pub(super) fn source_window_scope_required(entry: &RefEntry) -> bool { } #[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() +pub(super) fn single_window_fallback_allowed(entry: &RefEntry) -> bool { + source_window_scope_required(entry) && entry.bounds_hash.is_some() } #[cfg(target_os = "macos")] diff --git a/crates/macos/src/tree/resolve_tests.rs b/crates/macos/src/tree/resolve_tests.rs index d249328..a0162a3 100644 --- a/crates/macos/src/tree/resolve_tests.rs +++ b/crates/macos/src/tree/resolve_tests.rs @@ -1,5 +1,5 @@ use super::*; -use crate::tree::resolve_roots::{source_window_number, source_window_title_fallback_allowed}; +use crate::tree::resolve_roots::{single_window_fallback_allowed, source_window_number}; use agent_desktop_core::adapter::SnapshotSurface; fn entry( @@ -225,28 +225,22 @@ fn non_window_identity_candidate_without_bounds_fails_closed() { } #[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( +fn single_window_fallback_requires_bounds_hash_not_title() { + assert!(single_window_fallback_allowed(&entry( Some(42), Some("w-10"), None, None ))); + assert!(!single_window_fallback_allowed(&entry( + None, + Some("w-10"), + Some("Documents"), + 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)); + assert!(!single_window_fallback_allowed(&menu_entry)); } #[test] diff --git a/skills/agent-desktop/SKILL.md b/skills/agent-desktop/SKILL.md index 490ebf6..005ec68 100644 --- a/skills/agent-desktop/SKILL.md +++ b/skills/agent-desktop/SKILL.md @@ -87,13 +87,13 @@ Use **progressive skeleton traversal** as the default approach. It reduces token - In skeleton mode, named/described containers at truncation boundary also get refs (drill-down targets with empty `available_actions`) - Static text, groups, containers remain in tree for context but have no ref - Refs are deterministic within a snapshot but NOT stable across snapshots if UI changed -- Every snapshot returns `snapshot_id`; ref-consuming commands accept `--snapshot ` +- Every snapshot returns `snapshot_id`; ref-consuming commands accept `--snapshot `, and explicit snapshot IDs do not require also passing `--session` - `last_refmap.json` is only a latest-snapshot inspection artifact. The command path uses snapshot-scoped storage. - After any action that changes UI, re-drill the affected region or re-snapshot - **Scoped invalidation:** re-drilling `--root @e3` only replaces refs from @e3's previous drill — refs from other regions and the skeleton itself are preserved - **Strict resolution:** stale refs return `STALE_REF`; duplicate plausible targets return `AMBIGUOUS_TARGET` instead of choosing arbitrarily. - **Actionability:** ref actions check live visibility, stability, enabled state, supported action, policy, and editability before dispatch. -- **Sessions:** use `--session ` for concurrent or multi-agent runs; batch entries may override with `"session": "id"`. +- **Sessions:** use `--session ` for concurrent or multi-agent runs that share a latest snapshot pointer; batch entries may override with `"session": "id"`. - **Trace:** use `--trace ` for JSONL diagnostics outside stdout; `--trace-strict` fails on trace setup and pre-action writes. Post-action success traces are best-effort because the desktop mutation already happened. ## JSON Output Contract diff --git a/skills/agent-desktop/references/commands-interaction.md b/skills/agent-desktop/references/commands-interaction.md index 3bc268f..fd9f1cc 100644 --- a/skills/agent-desktop/references/commands-interaction.md +++ b/skills/agent-desktop/references/commands-interaction.md @@ -4,7 +4,7 @@ Commands for modifying UI state — clicking, typing, selecting, scrolling, and Ref-based actions are headless by default. They try semantic accessibility operations and do not silently steal focus, move the cursor, synthesize keyboard input, or use the pasteboard. Physical/headed interaction is reserved for explicit `focus`, `press`, `hover`, `drag`, and `mouse-*` commands or an explicit FFI policy. The `type` command has an explicit focus-fallback tier for callers that opt into focus changes while still forbidding cursor movement; the default CLI path remains AX-value-first and headless. -All ref-based interaction commands accept `--snapshot `. Omit it for the latest saved snapshot, or pass the `snapshot_id` returned by `snapshot` to keep scripts pinned to the exact ref map they observed. +All ref-based interaction commands accept `--snapshot `. Omit it for the active session's latest saved snapshot, or pass the `snapshot_id` returned by `snapshot` to keep scripts pinned to the exact ref map they observed. Explicit snapshot IDs do not require also passing `--session`. ## Click Actions diff --git a/skills/agent-desktop/references/commands-system.md b/skills/agent-desktop/references/commands-system.md index 72dad75..c5b6c93 100644 --- a/skills/agent-desktop/references/commands-system.md +++ b/skills/agent-desktop/references/commands-system.md @@ -173,7 +173,7 @@ agent-desktop wait --element @e5 --predicate actionable --timeout 5000 agent-desktop wait --element @e5 --predicate value --value "Done" --timeout 5000 ``` Blocks until the element ref appears in the accessibility tree. Useful after triggering UI changes. -When `--snapshot` is omitted, the command polls the caller's latest session refmap and refreshes it on the built-in debounce. When `--snapshot` is passed, it stays pinned to that refmap. Element resolution is capped by the remaining `--timeout`, and timeout errors include the last observed predicate/actionability state. +When `--snapshot` is omitted, the command polls the caller's latest session refmap and refreshes it on the built-in debounce. When `--snapshot` is passed, it resolves that pinned refmap directly. Element resolution is capped by the remaining `--timeout`, and timeout errors include the last observed predicate/actionability state. ### wait (window) ```bash diff --git a/src/cli/help_after.txt b/src/cli/help_after.txt index 6348d7e..9c4ea4e 100644 --- a/src/cli/help_after.txt +++ b/src/cli/help_after.txt @@ -85,10 +85,11 @@ BATCH REF IDs snapshot assigns @e1, @e2, ... to interactive elements in depth-first order. Use a ref wherever appears. Refs are snapshot-scoped. Pass - --snapshot to pin a command to a known snapshot; when omitted, - ref commands use the latest saved snapshot. Run snapshot again after UI changes. - Use --session as a shared namespace for concurrent agents; each session - stores multiple snapshot IDs plus its own latest-snapshot pointer. + --snapshot to pin a command to a known snapshot; explicit + snapshot IDs do not require --session. When --snapshot is omitted, ref + commands use the active session's latest saved snapshot. Run snapshot again + after UI changes. Use --session as a shared latest-snapshot namespace + for concurrent agents. Ref actions use strict resolution: stale targets return STALE_REF; duplicate plausible targets return AMBIGUOUS_TARGET instead of choosing arbitrarily. Ref actions run actionability checks before dispatch. Use --trace to diff --git a/src/cli/help_before.txt b/src/cli/help_before.txt index 1ad247b..80d868d 100644 --- a/src/cli/help_before.txt +++ b/src/cli/help_before.txt @@ -2,7 +2,7 @@ For AI agents — read this first: agent-desktop skills get desktop --full Bundled skill docs travel with the binary, so they always match this - exact version. They cover the snapshot/ref loop, session-scoped refs, + exact version. They cover the snapshot-scoped ref loop, shared sessions, actionability, trace JSONL, error codes with recovery hints, and copy-paste patterns for forms, menus, dialogs, and async waits. Specialized skills go deeper on macOS internals (TCC, AX API, surfaces, diff --git a/src/cli/mod.rs b/src/cli/mod.rs index 98f67d2..54d2640 100644 --- a/src/cli/mod.rs +++ b/src/cli/mod.rs @@ -42,7 +42,7 @@ pub(crate) struct Cli { #[arg( long, global = true, - help = "Scope snapshots, refs, and latest pointer to a session id" + help = "Select the latest-snapshot namespace; explicit --snapshot IDs do not require it" )] pub session: Option, diff --git a/src/cli_args/actions.rs b/src/cli_args/actions.rs index c2f38b3..4cbb98e 100644 --- a/src/cli_args/actions.rs +++ b/src/cli_args/actions.rs @@ -25,7 +25,7 @@ pub(crate) struct TypeArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg(value_name = "TEXT", allow_hyphen_values = true, help = "Text to type")] @@ -40,7 +40,7 @@ pub(crate) struct SetValueArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg( @@ -59,7 +59,7 @@ pub(crate) struct SelectArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg(value_name = "VALUE", help = "Option to select")] @@ -74,7 +74,7 @@ pub(crate) struct ScrollArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg( @@ -119,7 +119,7 @@ pub(crate) struct HoverArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg(long, help = "Absolute coordinates as x,y")] @@ -142,7 +142,7 @@ pub(crate) struct DragCliArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg(long, help = "Drag duration in milliseconds")] diff --git a/src/cli_args/mod.rs b/src/cli_args/mod.rs index 578364c..5b8b971 100644 --- a/src/cli_args/mod.rs +++ b/src/cli_args/mod.rs @@ -170,7 +170,7 @@ pub(crate) struct GetArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg( @@ -190,7 +190,7 @@ pub(crate) struct IsArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] pub snapshot: Option, #[arg( @@ -210,7 +210,7 @@ pub(crate) struct RefArgs { #[arg( long = "snapshot", value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot; omit to use latest" + help = "Snapshot ID returned by snapshot; omit to use active session latest" )] #[serde(rename = "snapshot", alias = "snapshot_id")] pub snapshot_id: Option, diff --git a/src/cli_args/system.rs b/src/cli_args/system.rs index 33c57fb..68efa23 100644 --- a/src/cli_args/system.rs +++ b/src/cli_args/system.rs @@ -145,7 +145,7 @@ pub(crate) struct WaitPredicateArgs { #[arg( long, value_name = "SNAPSHOT_ID", - help = "Snapshot ID returned by snapshot for --element waits; omit to use latest" + help = "Snapshot ID for --element waits; omit to use active session latest" )] pub snapshot: Option, #[arg( diff --git a/tests/conformance/README.md b/tests/conformance/README.md index eb84ed3..70d60e5 100644 --- a/tests/conformance/README.md +++ b/tests/conformance/README.md @@ -14,12 +14,13 @@ adapter-provided refs. | Area | Required behavior | |------|-------------------| -| Snapshot refs | Refs are depth-first, snapshot-scoped, and persisted in the caller session | +| Snapshot refs | Refs are depth-first, snapshot-scoped, and explicit snapshot IDs resolve directly | | Strict resolve | A ref resolves only when identity still matches; stale refs return `STALE_REF` | | Ambiguity | Multiple plausible matches return `AMBIGUOUS_TARGET`, never an arbitrary click | | Actionability | Ref actions check live visibility, stability, enabled state, supported action, policy, and editability before dispatch | | Wait recovery | `wait --element` can poll the latest session refmap when no snapshot is pinned, honors the caller timeout while resolving, and reports the last observed predicate state | -| Session scope | `--session ` and batch item `session` never read or write another session's refmap | +| Session latest scope | Commands that omit `--snapshot` read and write only the active session's latest refmap | +| Explicit snapshot scope | Passing `--snapshot ` resolves that pinned snapshot even when the caller omits the original session | | Trace | `--trace ` writes JSONL diagnostics outside stdout and is best-effort unless strict | | FFI parity | FFI ref actions use strict resolve and actionability checks before adapter dispatch |