diff --git a/crates/core/src/actionability/live.rs b/crates/core/src/actionability/live.rs index 0d5b8fa2..fc46afed 100644 --- a/crates/core/src/actionability/live.rs +++ b/crates/core/src/actionability/live.rs @@ -10,6 +10,18 @@ use crate::{ refs::RefEntry, }; +/// Groups the read-only coordinates of a live element check: which element, +/// which handle to read it through, which adapter to read with, and the +/// deadline that bounds the read. Kept separate from the `request` (what +/// action to check against) and `stability` (how settled the element must +/// be) parameters, which vary independently per call. +pub(crate) struct LiveCheckTarget<'a> { + pub(crate) entry: &'a RefEntry, + pub(crate) handle: &'a NativeHandle, + pub(crate) adapter: &'a dyn PlatformAdapter, + pub(crate) deadline: crate::Deadline, +} + #[cfg(test)] pub(crate) fn check_live( entry: &RefEntry, @@ -19,30 +31,34 @@ pub(crate) fn check_live( ) -> Result { let deadline = crate::Deadline::standard()?; check_live_with_stability( - entry, - handle, - adapter, + &LiveCheckTarget { + entry, + handle, + adapter, + deadline, + }, request, StabilityExpectation::permissive(entry.geometry.bounds_hash), - deadline, ) } pub(crate) fn check_live_with_stability( - entry: &RefEntry, - handle: &NativeHandle, - adapter: &dyn PlatformAdapter, + target: &LiveCheckTarget<'_>, request: &ActionRequest, stability: StabilityExpectation, - deadline: crate::Deadline, ) -> Result { - let evidence = observe(entry, adapter.get_live_element(handle, deadline))?; + let evidence = observe( + target.entry, + target + .adapter + .get_live_element(target.handle, target.deadline), + )?; check_with_stability( stability, &evidence, request, - Some((handle, adapter)), - deadline, + Some((target.handle, target.adapter)), + target.deadline, ) } diff --git a/crates/core/src/actionability/mod.rs b/crates/core/src/actionability/mod.rs index 8ad915a6..c0d9f664 100644 --- a/crates/core/src/actionability/mod.rs +++ b/crates/core/src/actionability/mod.rs @@ -24,7 +24,7 @@ pub(crate) use gates::bounds_are_visible; pub(crate) use gates::states_are_enabled; #[cfg(test)] pub(crate) use live::check_live; -pub(crate) use live::check_live_with_stability; +pub(crate) use live::{LiveCheckTarget, check_live_with_stability}; pub(crate) use pointer_delivery::PointerDelivery; pub(crate) use receives_events::require_receives_events; pub(crate) use requirements::requires_stability; diff --git a/crates/core/src/commands/clipboard_get.rs b/crates/core/src/commands/clipboard_get.rs index d643a231..1712f5c6 100644 --- a/crates/core/src/commands/clipboard_get.rs +++ b/crates/core/src/commands/clipboard_get.rs @@ -3,12 +3,16 @@ use crate::{ adapter::PlatformAdapter, context::CommandContext, refs::{write_private_file, write_user_file}, + refs_store::{ + STALE_TMP_MAX_AGE, + prune::{is_orphaned_tmp_file, remove_stale_files_in_dir}, + }, session, }; use serde_json::{Value, json}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::sync::atomic::{AtomicU64, Ordering}; -use std::time::{SystemTime, UNIX_EPOCH}; +use std::time::{Duration, SystemTime, UNIX_EPOCH}; pub struct ClipboardGetArgs { pub format: Option, @@ -17,6 +21,8 @@ pub struct ClipboardGetArgs { static IMAGE_TEMP_COUNTER: AtomicU64 = AtomicU64::new(0); +const CLIPBOARD_IMAGE_MAX_AGE: Duration = Duration::from_secs(60 * 60); + pub fn execute( args: ClipboardGetArgs, adapter: &dyn PlatformAdapter, @@ -62,7 +68,11 @@ fn write_image( fn default_clipboard_image_path(context: &CommandContext) -> Result { let dir = match context.session_id() { Some(id) => session::session_dir(id)?.join("clipboard"), - None => session::agent_desktop_dir()?.join("tmp"), + None => { + let dir = session::agent_desktop_dir()?.join("tmp"); + prune_sessionless_clipboard_tmp_dir(&dir); + dir + } }; let nanos = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -75,6 +85,17 @@ fn default_clipboard_image_path(context: &CommandContext) -> Result bool { + path.file_name() + .and_then(|name| name.to_str()) + .is_some_and(|name| name.starts_with("clipboard-") && name.ends_with(".png")) +} + #[cfg(test)] #[path = "clipboard_get_tests.rs"] mod tests; diff --git a/crates/core/src/commands/clipboard_get_tests.rs b/crates/core/src/commands/clipboard_get_tests.rs index aa6dabff..a8237490 100644 --- a/crates/core/src/commands/clipboard_get_tests.rs +++ b/crates/core/src/commands/clipboard_get_tests.rs @@ -158,6 +158,198 @@ fn image_variant_with_explicit_out_writes_that_path() { let _ = std::fs::remove_dir_all(&dir); } +fn write_with_age(path: &std::path::Path, contents: &[u8], age: Duration) { + std::fs::write(path, contents).unwrap(); + let file = std::fs::OpenOptions::new().write(true).open(path).unwrap(); + file.set_modified(SystemTime::now() - age).unwrap(); +} + +#[test] +fn clipboard_image_predicate_matches_only_the_generated_shape() { + assert!(is_clipboard_image_file(Path::new( + "clipboard-123-456-0.png" + ))); + assert!(!is_clipboard_image_file(Path::new( + "clipboard-123-456-0.png.tmp" + ))); + assert!(!is_clipboard_image_file(Path::new("other.png"))); + assert!(!is_clipboard_image_file(Path::new("clipboard-123.txt"))); +} + +#[test] +fn sessionless_prune_removes_stale_matches_keeps_fresh_and_unrelated() { + let _home = HomeGuard::new(); + let dir = session::agent_desktop_dir().unwrap().join("tmp"); + std::fs::create_dir_all(&dir).unwrap(); + + let stale_png = dir.join("clipboard-1-1-0.png"); + let stale_tmp = dir.join("orphan-write.tmp"); + let fresh_png = dir.join("clipboard-2-2-0.png"); + let unrelated = dir.join("notes.txt"); + write_with_age( + &stale_png, + b"old", + CLIPBOARD_IMAGE_MAX_AGE + Duration::from_secs(1), + ); + write_with_age( + &stale_tmp, + b"old", + STALE_TMP_MAX_AGE + Duration::from_secs(1), + ); + write_with_age(&fresh_png, b"new", Duration::from_secs(1)); + write_with_age( + &unrelated, + b"keep me", + CLIPBOARD_IMAGE_MAX_AGE + Duration::from_secs(1), + ); + + prune_sessionless_clipboard_tmp_dir(&dir); + + assert!( + !stale_png.exists(), + "clipboard png older than its TTL must be pruned" + ); + assert!( + !stale_tmp.exists(), + "orphaned tmp file older than its TTL must be pruned" + ); + assert!( + fresh_png.exists(), + "clipboard png within its TTL must survive the sweep" + ); + assert!( + unrelated.exists(), + "a file outside the clipboard-*.png / *.tmp shapes must never be touched" + ); +} + +#[test] +fn sweep_never_removes_a_directory_even_if_it_matches_the_shape() { + let _home = HomeGuard::new(); + let dir = session::agent_desktop_dir().unwrap().join("tmp"); + let fake_dir = dir.join("clipboard-0-0-0.png"); + std::fs::create_dir_all(&fake_dir).unwrap(); + + remove_stale_files_in_dir(&dir, Duration::ZERO, is_clipboard_image_file); + + assert!( + fake_dir.is_dir(), + "the sweep must never remove a directory, even one matching the file shape" + ); +} + +#[test] +fn consecutive_sessionless_image_captures_keep_the_prior_capture() { + let _home = HomeGuard::new(); + let first_double = LocalDouble::returning(Ok(Some(ClipboardContent::Image(ImageBuffer { + data: vec![1], + format: crate::ImageFormat::Png, + width: 1, + height: 1, + scale_factor: 1.0, + })))); + let first = execute( + ClipboardGetArgs { + format: Some(ClipboardFormat::Image), + out: None, + }, + &first_double, + &no_session_context(), + ) + .unwrap(); + let first_path = PathBuf::from(first["path"].as_str().unwrap()); + + let second_double = LocalDouble::returning(Ok(Some(ClipboardContent::Image(ImageBuffer { + data: vec![2], + format: crate::ImageFormat::Png, + width: 1, + height: 1, + scale_factor: 1.0, + })))); + let _ = execute( + ClipboardGetArgs { + format: Some(ClipboardFormat::Image), + out: None, + }, + &second_double, + &no_session_context(), + ) + .unwrap(); + + assert!( + first_path.exists(), + "a clipboard image captured moments ago must not be pruned by the very next capture" + ); +} + +#[test] +fn image_variant_without_out_prunes_stale_sessionless_artifacts() { + let _home = HomeGuard::new(); + let dir = session::agent_desktop_dir().unwrap().join("tmp"); + std::fs::create_dir_all(&dir).unwrap(); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + for path in [dir.parent().unwrap(), dir.as_path()] { + std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o700)).unwrap(); + } + } + + let stale_png = dir.join("clipboard-9-9-0.png"); + let stale_tmp = dir.join("orphan.tmp"); + let unrelated = dir.join("keep.txt"); + write_with_age( + &stale_png, + b"old capture", + CLIPBOARD_IMAGE_MAX_AGE + Duration::from_secs(1), + ); + write_with_age( + &stale_tmp, + b"old", + STALE_TMP_MAX_AGE + Duration::from_secs(1), + ); + write_with_age( + &unrelated, + b"keep", + CLIPBOARD_IMAGE_MAX_AGE + Duration::from_secs(1), + ); + + let double = LocalDouble::returning(Ok(Some(ClipboardContent::Image(ImageBuffer { + data: vec![7, 7, 7], + format: crate::ImageFormat::Png, + width: 1, + height: 1, + scale_factor: 1.0, + })))); + let out = execute( + ClipboardGetArgs { + format: Some(ClipboardFormat::Image), + out: None, + }, + &double, + &no_session_context(), + ) + .unwrap(); + let new_path = PathBuf::from(out["path"].as_str().unwrap()); + + assert!( + new_path.exists(), + "the image just written by this call must survive its own write" + ); + assert!( + !stale_png.exists(), + "a stale clipboard png must be pruned by the next capture" + ); + assert!( + !stale_tmp.exists(), + "a stale orphaned tmp file must be pruned by the next capture" + ); + assert!( + unrelated.exists(), + "a non-matching file must never be touched" + ); +} + #[cfg(unix)] #[test] fn image_variant_without_out_writes_private_0600_file_under_session_dir() { diff --git a/crates/core/src/commands/drag.rs b/crates/core/src/commands/drag.rs index 11b09b91..9b3eb463 100644 --- a/crates/core/src/commands/drag.rs +++ b/crates/core/src/commands/drag.rs @@ -5,7 +5,7 @@ use crate::{ helpers::{apply_post_action_wait, validate_post_action_wait}, point_resolve::{PointResolveArgs, require_cursor_policy}, pointer_action::{ - ensure_point_deadline, focus_point_under_lease, point_deadline, + PointResolveAttempt, ensure_point_deadline, focus_point_under_lease, point_deadline, resolve_point_under_lease, retry_leased_point_phase, wait_for_point_with_deadline, }, }, @@ -13,11 +13,14 @@ use crate::{ }; use serde_json::{Value, json}; +pub struct DragEndpoint { + pub ref_id: Option, + pub xy: Option<(f64, f64)>, +} + pub struct DragArgs { - pub from_ref: Option, - pub from_xy: Option<(f64, f64)>, - pub to_ref: Option, - pub to_xy: Option<(f64, f64)>, + pub from: DragEndpoint, + pub to: DragEndpoint, pub snapshot_id: Option, pub duration_ms: Option, pub drop_delay_ms: Option, @@ -33,15 +36,15 @@ pub fn execute( validate_post_action_wait(context)?; let deadline = point_deadline(args.timeout_ms)?; let from_args = PointResolveArgs { - ref_id: args.from_ref.as_deref(), - xy: args.from_xy, + ref_id: args.from.ref_id.as_deref(), + xy: args.from.xy, snapshot_id: args.snapshot_id.as_deref(), missing_input_message: "Provide --from or --from-xy x,y", headed_requirement: crate::HeadedRequirement::FocusedWindowAndCursor, }; let to_args = PointResolveArgs { - ref_id: args.to_ref.as_deref(), - xy: args.to_xy, + ref_id: args.to.ref_id.as_deref(), + xy: args.to.xy, snapshot_id: args.snapshot_id.as_deref(), missing_input_message: "Provide --to or --to-xy x,y", headed_requirement: crate::HeadedRequirement::None, @@ -55,32 +58,44 @@ pub fn execute( let lease = adapter.acquire_interaction_lease(deadline)?; let focused = focus_point_under_lease(from_args, &lease, adapter, context)?; let from = resolve_point_under_lease( - (from_args, None), - !auto_wait, + PointResolveAttempt { + args: from_args, + stability: None, + allow_scroll: !auto_wait, + }, deadline, &lease, adapter, context, )?; let to = resolve_point_under_lease( - (to_args, None), - !auto_wait, + PointResolveAttempt { + args: to_args, + stability: None, + allow_scroll: !auto_wait, + }, deadline, &lease, adapter, context, )?; let mut from = resolve_point_under_lease( - (from_args, Some(from.bounds_hash)), - false, + PointResolveAttempt { + args: from_args, + stability: Some(from.bounds_hash), + allow_scroll: false, + }, deadline, &lease, adapter, context, )?; let to = resolve_point_under_lease( - (to_args, Some(to.bounds_hash)), - false, + PointResolveAttempt { + args: to_args, + stability: Some(to.bounds_hash), + allow_scroll: false, + }, deadline, &lease, adapter, @@ -116,13 +131,8 @@ pub fn execute( if from.focused { response["focused"] = json!(true); } - apply_post_action_wait( - response, - from.source_entry.as_ref(), - adapter, - context, - &lease, - ) + drop(lease); + apply_post_action_wait(response, from.source_entry.as_ref(), adapter, context) } #[cfg(test)] diff --git a/crates/core/src/commands/drag_retry_tests.rs b/crates/core/src/commands/drag_retry_tests.rs index c0f410db..05283b1d 100644 --- a/crates/core/src/commands/drag_retry_tests.rs +++ b/crates/core/src/commands/drag_retry_tests.rs @@ -100,10 +100,14 @@ fn transient_stale_ref_retries_then_succeeds_when_timeout_wired() { let value = execute( DragArgs { - from_ref: Some("@e1".into()), - from_xy: None, - to_ref: Some("@e2".into()), - to_xy: None, + from: DragEndpoint { + ref_id: Some("@e1".into()), + xy: None, + }, + to: DragEndpoint { + ref_id: Some("@e2".into()), + xy: None, + }, snapshot_id: Some(snapshot_id), duration_ms: None, drop_delay_ms: None, @@ -329,10 +333,14 @@ fn timeout_none_is_single_shot() { let error = execute( DragArgs { - from_ref: Some("@e1".into()), - from_xy: None, - to_ref: Some("@e2".into()), - to_xy: None, + from: DragEndpoint { + ref_id: Some("@e1".into()), + xy: None, + }, + to: DragEndpoint { + ref_id: Some("@e2".into()), + xy: None, + }, snapshot_id: Some(snapshot_id), duration_ms: None, drop_delay_ms: None, diff --git a/crates/core/src/commands/drag_tests.rs b/crates/core/src/commands/drag_tests.rs index d5d3e0b9..eb21690f 100644 --- a/crates/core/src/commands/drag_tests.rs +++ b/crates/core/src/commands/drag_tests.rs @@ -110,10 +110,14 @@ impl SystemOps for DragCaptureAdapter { fn xy_args(drop_delay_ms: Option) -> DragArgs { DragArgs { - from_ref: None, - from_xy: Some((1.0, 2.0)), - to_ref: None, - to_xy: Some((3.0, 4.0)), + from: DragEndpoint { + ref_id: None, + xy: Some((1.0, 2.0)), + }, + to: DragEndpoint { + ref_id: None, + xy: Some((3.0, 4.0)), + }, snapshot_id: None, duration_ms: None, drop_delay_ms, @@ -202,10 +206,14 @@ fn cross_app_snapshot() -> String { fn cross_app_args(snapshot_id: String) -> DragArgs { DragArgs { - from_ref: Some("@e1".into()), - from_xy: None, - to_ref: Some("@e2".into()), - to_xy: None, + from: DragEndpoint { + ref_id: Some("@e1".into()), + xy: None, + }, + to: DragEndpoint { + ref_id: Some("@e2".into()), + xy: None, + }, snapshot_id: Some(snapshot_id), duration_ms: None, drop_delay_ms: None, diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 3a658161..04e77a62 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -122,11 +122,22 @@ pub(crate) fn execute_ref_action_with_context( let value = serde_json::to_value(result).map_err(|error| { post_delivery_error(AppError::Json(error), json!({ "action": "delivered" })) })?; - let mut outcome = apply_post_action_wait(value, Some(&entry), adapter, context, &lease); let lease_hold_ms = u64::try_from(lease_started.elapsed().as_millis()).unwrap_or(u64::MAX); - update_lease_hold_ms(&mut outcome, lease_hold_ms); drop(lease); - crate::ref_action::finish_artifacts(context, adapter, &entry, &args.ref_id, &pre, deadline); + let mut outcome = apply_post_action_wait(value, Some(&entry), adapter, context); + update_lease_hold_ms(&mut outcome, lease_hold_ms); + crate::ref_action::finish_artifacts( + crate::ref_action_context::RefActionContext::new( + RefActionWaitContext { + adapter, + entry: &entry, + ref_id: &args.ref_id, + context, + }, + deadline, + ), + &pre, + ); outcome } @@ -148,7 +159,6 @@ pub(crate) fn apply_post_action_wait( entry: Option<&RefEntry>, adapter: &dyn PlatformAdapter, context: &CommandContext, - _lease: &crate::InteractionLease, ) -> Result { let Some(wait) = context.wait_selector() else { return Ok(result); diff --git a/crates/core/src/commands/helpers_ref_action_wait_result_tests.rs b/crates/core/src/commands/helpers_ref_action_wait_result_tests.rs index 5df6dc91..5e876c52 100644 --- a/crates/core/src/commands/helpers_ref_action_wait_result_tests.rs +++ b/crates/core/src/commands/helpers_ref_action_wait_result_tests.rs @@ -6,11 +6,7 @@ fn post_action_wait_without_flag_returns_action_only() { let mut refmap = RefMap::new(); refmap.allocate(entry()); let snapshot_id = RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap(); - let adapter = ScopedWaitAdapter { - request: Mutex::new(None), - polled_app: Mutex::new(None), - lease_held: Arc::new(AtomicBool::new(false)), - }; + let adapter = ScopedWaitAdapter::new(); let args = RefArgs { ref_id: "@e1".into(), snapshot_id: Some(snapshot_id), @@ -37,11 +33,7 @@ fn post_action_wait_timeout_embeds_action_result_in_details() { entry.source.source_app = Some("TargetApp".into()); refmap.allocate(entry); let snapshot_id = RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap(); - let adapter = ScopedWaitAdapter { - request: Mutex::new(None), - polled_app: Mutex::new(None), - lease_held: Arc::new(AtomicBool::new(false)), - }; + let adapter = ScopedWaitAdapter::new(); let context = CommandContext::default().with_wait_selector(Some(WaitSelector { query_raw: ":never-appears".into(), gone: false, diff --git a/crates/core/src/commands/helpers_ref_action_wait_tests.rs b/crates/core/src/commands/helpers_ref_action_wait_tests.rs index 1aa6ca6d..3a66b0b0 100644 --- a/crates/core/src/commands/helpers_ref_action_wait_tests.rs +++ b/crates/core/src/commands/helpers_ref_action_wait_tests.rs @@ -9,7 +9,7 @@ use crate::{AccessibilityNode, WindowInfo}; use crate::{action::Action, action_result::ActionResult}; use std::sync::{ Arc, Mutex, - atomic::{AtomicBool, Ordering}, + atomic::{AtomicBool, AtomicU32, Ordering}, }; struct LeaseGuard(Arc); @@ -24,6 +24,18 @@ struct ScopedWaitAdapter { request: Mutex>, polled_app: Mutex>, lease_held: Arc, + lease_free_polls: AtomicU32, +} + +impl ScopedWaitAdapter { + fn new() -> Self { + Self { + request: Mutex::new(None), + polled_app: Mutex::new(None), + lease_held: Arc::new(AtomicBool::new(false)), + lease_free_polls: AtomicU32::new(0), + } + } } impl ObservationOps for ScopedWaitAdapter { @@ -32,11 +44,12 @@ impl ObservationOps for ScopedWaitAdapter { root: crate::live_locator::ObservationRoot<'_>, _request: &crate::live_locator::ObservationRequest, ) -> Result { - if !self.lease_held.load(Ordering::SeqCst) { + if self.lease_held.load(Ordering::SeqCst) { return Err(AdapterError::internal( - "post-action wait escaped the interaction lease", + "post-action wait must poll with the interaction lease released", )); } + self.lease_free_polls.fetch_add(1, Ordering::SeqCst); crate::adapter::observed_tree( &root, AccessibilityNode { @@ -110,6 +123,7 @@ impl ActionOps for ScopedWaitAdapter { request: ActionRequest, _lease: &crate::InteractionLease, ) -> Result { + assert!(self.lease_held.load(Ordering::SeqCst)); *self.request.lock().unwrap() = Some(request); Ok(ActionResult::delivered_unverified("ok")) } @@ -137,11 +151,7 @@ fn post_action_wait_scopes_to_source_app_and_merges_action_result() { entry.source.source_app = Some("TargetApp".into()); refmap.allocate(entry); let snapshot_id = RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap(); - let adapter = ScopedWaitAdapter { - request: Mutex::new(None), - polled_app: Mutex::new(None), - lease_held: Arc::new(AtomicBool::new(false)), - }; + let adapter = ScopedWaitAdapter::new(); let context = CommandContext::default().with_wait_selector(Some(WaitSelector { query_raw: ":saved!".into(), gone: false, @@ -166,6 +176,7 @@ fn post_action_wait_scopes_to_source_app_and_merges_action_result() { Some("TargetApp") ); assert!(!adapter.lease_held.load(Ordering::SeqCst)); + assert!(adapter.lease_free_polls.load(Ordering::SeqCst) >= 1); assert_eq!(value["after_action"]["action"], "ok"); assert_eq!(value["matched_selector"], ":saved!"); } diff --git a/crates/core/src/commands/hover.rs b/crates/core/src/commands/hover.rs index ae6251be..cc94d600 100644 --- a/crates/core/src/commands/hover.rs +++ b/crates/core/src/commands/hover.rs @@ -5,7 +5,7 @@ use crate::{ helpers::{apply_post_action_wait, validate_post_action_wait}, point_resolve::{PointResolveArgs, require_cursor_policy}, pointer_action::{ - ensure_point_deadline, focus_point_under_lease, point_deadline, + PointResolveAttempt, ensure_point_deadline, focus_point_under_lease, point_deadline, resolve_point_under_lease, retry_leased_point_phase, wait_for_point_with_deadline, }, }, @@ -50,16 +50,22 @@ pub fn execute( let lease = adapter.acquire_interaction_lease(deadline)?; let focused = focus_point_under_lease(point_args, &lease, adapter, context)?; let first = resolve_point_under_lease( - (point_args, None), - !auto_wait, + PointResolveAttempt { + args: point_args, + stability: None, + allow_scroll: !auto_wait, + }, deadline, &lease, adapter, context, )?; let mut resolved = resolve_point_under_lease( - (point_args, Some(first.bounds_hash)), - false, + PointResolveAttempt { + args: point_args, + stability: Some(first.bounds_hash), + allow_scroll: false, + }, deadline, &lease, adapter, @@ -88,13 +94,8 @@ pub fn execute( if resolved.focused { response["focused"] = json!(true); } - apply_post_action_wait( - response, - resolved.source_entry.as_ref(), - adapter, - context, - &lease, - ) + drop(lease); + apply_post_action_wait(response, resolved.source_entry.as_ref(), adapter, context) } #[cfg(test)] diff --git a/crates/core/src/commands/mod.rs b/crates/core/src/commands/mod.rs index f30163c2..ea5435c9 100644 --- a/crates/core/src/commands/mod.rs +++ b/crates/core/src/commands/mod.rs @@ -75,6 +75,7 @@ pub(crate) mod wait_event_input; pub(crate) mod wait_mode; pub(crate) mod wait_predicate; pub mod wait_selector; +pub mod wait_surface; pub(crate) mod wait_text_match; pub(crate) mod wait_timeout; mod window_target; diff --git a/crates/core/src/commands/pointer_action.rs b/crates/core/src/commands/pointer_action.rs index 7caaf1ab..2ae8b47b 100644 --- a/crates/core/src/commands/pointer_action.rs +++ b/crates/core/src/commands/pointer_action.rs @@ -8,16 +8,33 @@ use crate::{ }; use serde_json::{Value, json}; -fn resolve_point_from_entry( - target: (&str, &RefEntry), +struct EntryPointResolve<'a> { + ref_id: &'a str, + entry: &'a RefEntry, stability: Option>, - deadline: crate::Deadline, - lease: Option<&crate::InteractionLease>, + lease: Option<&'a crate::InteractionLease>, verify_receives_events: bool, +} + +pub(crate) struct PointResolveAttempt<'a> { + pub args: crate::commands::point_resolve::PointResolveArgs<'a>, + pub stability: Option>, + pub allow_scroll: bool, +} + +fn resolve_point_from_entry( + request: EntryPointResolve<'_>, + deadline: crate::Deadline, adapter: &dyn PlatformAdapter, context: &CommandContext, ) -> Result { - let (ref_id, entry) = target; + let EntryPointResolve { + ref_id, + entry, + stability, + lease, + verify_receives_events, + } = request; let mut handle = resolve_handle_within_deadline(adapter, entry, deadline).inspect_err(|err| { let _ = context.trace_lazy("ref.resolve.error", || { @@ -150,11 +167,14 @@ pub(crate) fn wait_for_point_with_deadline<'a>( return Err(point_actionability_timeout(last_report)); } match resolve_point_from_entry( - (ref_id, &entry), - stability, + EntryPointResolve { + ref_id, + entry: &entry, + stability, + lease: None, + verify_receives_events: false, + }, deadline, - None, - false, adapter, context, ) { @@ -208,11 +228,7 @@ pub(crate) fn focus_point_under_lease( } pub(crate) fn resolve_point_under_lease<'a>( - target: ( - crate::commands::point_resolve::PointResolveArgs<'a>, - Option>, - ), - allow_scroll: bool, + attempt: PointResolveAttempt<'a>, deadline: crate::Deadline, lease: &crate::InteractionLease, adapter: &dyn PlatformAdapter, @@ -220,17 +236,24 @@ pub(crate) fn resolve_point_under_lease<'a>( ) -> Result { use crate::commands::point_resolve::resolve_point_from_ref_or_xy_with_context; - let (args, stability) = target; + let PointResolveAttempt { + args, + stability, + allow_scroll, + } = attempt; let Some(ref_id) = args.ref_id else { return resolve_point_from_ref_or_xy_with_context(args, adapter, context, deadline, lease); }; let entry = load_ref_entry(ref_id, args.snapshot_id, context)?; resolve_point_from_entry( - (ref_id, &entry), - stability, + EntryPointResolve { + ref_id, + entry: &entry, + stability, + lease: allow_scroll.then_some(lease), + verify_receives_events: true, + }, deadline, - allow_scroll.then_some(lease), - true, adapter, context, ) diff --git a/crates/core/src/commands/pointer_single_shot_tests.rs b/crates/core/src/commands/pointer_single_shot_tests.rs index 1d6d086e..df04e271 100644 --- a/crates/core/src/commands/pointer_single_shot_tests.rs +++ b/crates/core/src/commands/pointer_single_shot_tests.rs @@ -206,10 +206,14 @@ fn drag_none_scrolls_once_then_dispatches_revalidated_endpoints_once() { let adapter = SingleShotScrollAdapter::new(); let value = drag::execute( drag::DragArgs { - from_ref: Some("@e1".into()), - from_xy: None, - to_ref: Some("@e2".into()), - to_xy: None, + from: drag::DragEndpoint { + ref_id: Some("@e1".into()), + xy: None, + }, + to: drag::DragEndpoint { + ref_id: Some("@e2".into()), + xy: None, + }, snapshot_id: Some(snapshot(2)), duration_ms: None, drop_delay_ms: None, diff --git a/crates/core/src/commands/wait.rs b/crates/core/src/commands/wait.rs index ed48199e..0cbd45b0 100644 --- a/crates/core/src/commands/wait.rs +++ b/crates/core/src/commands/wait.rs @@ -5,6 +5,7 @@ use crate::{ helpers::resolve_app, wait_element::{ElementWaitInput, wait_for_element}, wait_mode::WaitMode, + wait_surface::SurfaceWait, wait_text_match, wait_timeout, }, context::CommandContext, @@ -33,9 +34,7 @@ pub struct WaitModeArgs { pub element: Option, pub window: Option, pub text: Option, - pub menu: bool, - pub menu_closed: bool, - pub notification: bool, + pub surface: Option, pub event: Option, pub window_id: Option, } diff --git a/crates/core/src/commands/wait_mode.rs b/crates/core/src/commands/wait_mode.rs index d9322392..8c7cd48a 100644 --- a/crates/core/src/commands/wait_mode.rs +++ b/crates/core/src/commands/wait_mode.rs @@ -1,6 +1,6 @@ use crate::{ AppError, - commands::{wait::WaitArgs, wait_predicate}, + commands::{wait::WaitArgs, wait_predicate, wait_surface::SurfaceWait}, refs::validate_ref_id, }; @@ -39,17 +39,26 @@ impl WaitMode { if let Some(ms) = args.mode.ms { return Ok(Self::Sleep(ms)); } - if args.mode.menu || args.mode.menu_closed { - return Ok(Self::Menu { - app: args.app, - open: args.mode.menu, - }); - } - if args.mode.notification { - return Ok(Self::Notification { - app: args.app, - text: args.mode.text, - }); + match args.mode.surface { + Some(SurfaceWait::Menu) => { + return Ok(Self::Menu { + app: args.app, + open: true, + }); + } + Some(SurfaceWait::MenuClosed) => { + return Ok(Self::Menu { + app: args.app, + open: false, + }); + } + Some(SurfaceWait::Notification) => { + return Ok(Self::Notification { + app: args.app, + text: args.mode.text, + }); + } + None => {} } if let Some(event) = args.mode.event { return Ok(Self::Event { @@ -103,7 +112,8 @@ pub(crate) fn validate_wait_mode(args: &WaitArgs) -> Result<(), AppError> { "Use --element --predicate value --value .", )); } - if args.predicate.count.is_some() && (args.mode.text.is_none() || args.mode.notification) { + let waits_for_notification = matches!(args.mode.surface, Some(SurfaceWait::Notification)); + if args.predicate.count.is_some() && (args.mode.text.is_none() || waits_for_notification) { return Err(AppError::invalid_input_with_suggestion( "--count is only valid for --text waits", "Use --text --count without --notification, or remove --count.", @@ -114,10 +124,8 @@ pub(crate) fn validate_wait_mode(args: &WaitArgs) -> Result<(), AppError> { args.mode.ms.is_some(), args.mode.element.is_some(), args.mode.window.is_some() && args.mode.event.is_none(), - args.mode.text.is_some() && !args.mode.notification, - args.mode.menu, - args.mode.menu_closed, - args.mode.notification, + args.mode.text.is_some() && !waits_for_notification, + args.mode.surface.is_some(), args.mode.event.is_some(), ] .into_iter() @@ -129,10 +137,14 @@ pub(crate) fn validate_wait_mode(args: &WaitArgs) -> Result<(), AppError> { if selected == 0 { return Err(missing_wait_mode()); } - Err(AppError::invalid_input_with_suggestion( + Err(ambiguous_wait_mode()) +} + +pub(crate) fn ambiguous_wait_mode() -> AppError { + AppError::invalid_input_with_suggestion( "wait accepts exactly one mode", "Use one of: ms, --element, --window, --text, --menu, --menu-closed, --notification, or --event.", - )) + ) } fn validate_event_filters(args: &WaitArgs) -> Result<(), AppError> { diff --git a/crates/core/src/commands/wait_mode_tests.rs b/crates/core/src/commands/wait_mode_tests.rs index 35073e6d..f5165894 100644 --- a/crates/core/src/commands/wait_mode_tests.rs +++ b/crates/core/src/commands/wait_mode_tests.rs @@ -22,9 +22,7 @@ fn mode() -> WaitModeArgs { element: None, window: None, text: None, - menu: false, - menu_closed: false, - notification: false, + surface: None, event: None, window_id: None, } @@ -83,6 +81,48 @@ fn from_args_threads_window_title_into_event_mode() { } } +#[test] +fn from_args_maps_surface_variants_to_menu_open_state() { + let open = WaitMode::from_args(args(WaitModeArgs { + surface: Some(SurfaceWait::Menu), + ..mode() + })) + .unwrap(); + assert!(matches!(open, WaitMode::Menu { open: true, .. })); + + let closed = WaitMode::from_args(args(WaitModeArgs { + surface: Some(SurfaceWait::MenuClosed), + ..mode() + })) + .unwrap(); + assert!(matches!(closed, WaitMode::Menu { open: false, .. })); +} + +#[test] +fn from_args_threads_text_filter_into_notification_mode() { + let parsed = WaitMode::from_args(args(WaitModeArgs { + surface: Some(SurfaceWait::Notification), + text: Some("done".into()), + ..mode() + })) + .unwrap(); + match parsed { + WaitMode::Notification { text, .. } => assert_eq!(text.as_deref(), Some("done")), + _ => panic!("expected WaitMode::Notification, got a different mode"), + } +} + +#[test] +fn surface_and_element_together_remain_ambiguous() { + let result = validate_wait_mode(&args(WaitModeArgs { + surface: Some(SurfaceWait::Menu), + element: Some("@e1".into()), + ..mode() + })); + + assert_eq!(result.unwrap_err().code(), "INVALID_ARGS"); +} + #[test] fn app_event_rejects_window_filter_immediately() { let result = validate_wait_mode(&args(WaitModeArgs { diff --git a/crates/core/src/commands/wait_predicate.rs b/crates/core/src/commands/wait_predicate.rs index b1900d4b..9567b330 100644 --- a/crates/core/src/commands/wait_predicate.rs +++ b/crates/core/src/commands/wait_predicate.rs @@ -174,7 +174,14 @@ fn actionable( stability: crate::actionability::StabilityExpectation, ) -> Result { match crate::actionability::check_live_with_stability( - entry, handle, adapter, request, stability, deadline, + &crate::actionability::LiveCheckTarget { + entry, + handle, + adapter, + deadline, + }, + request, + stability, ) { Ok(report) => Ok(json!(report)), Err(err) if err.code == ErrorCode::ActionFailed => match err.details { diff --git a/crates/core/src/commands/wait_scenario_tests.rs b/crates/core/src/commands/wait_scenario_tests.rs index 60568c1b..066c0b45 100644 --- a/crates/core/src/commands/wait_scenario_tests.rs +++ b/crates/core/src/commands/wait_scenario_tests.rs @@ -139,7 +139,7 @@ fn menu_closed_wait_requests_closed_state_and_reports_found() { let value = execute( WaitArgs { mode: WaitModeArgs { - menu_closed: true, + surface: Some(SurfaceWait::MenuClosed), ..wait_args().mode }, app: Some("MenuApp".into()), diff --git a/crates/core/src/commands/wait_surface.rs b/crates/core/src/commands/wait_surface.rs new file mode 100644 index 00000000..3d5032e6 --- /dev/null +++ b/crates/core/src/commands/wait_surface.rs @@ -0,0 +1,31 @@ +use crate::AppError; + +/// Which surface-lifecycle condition a `wait` targets: `--menu`, +/// `--menu-closed`, or `--notification`. One variant per flag makes the three +/// modes structurally mutually exclusive. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum SurfaceWait { + Menu, + MenuClosed, + Notification, +} + +impl SurfaceWait { + pub fn from_flags( + menu: bool, + menu_closed: bool, + notification: bool, + ) -> Result, AppError> { + match (menu, menu_closed, notification) { + (false, false, false) => Ok(None), + (true, false, false) => Ok(Some(Self::Menu)), + (false, true, false) => Ok(Some(Self::MenuClosed)), + (false, false, true) => Ok(Some(Self::Notification)), + _ => Err(crate::commands::wait_mode::ambiguous_wait_mode()), + } + } +} + +#[cfg(test)] +#[path = "wait_surface_tests.rs"] +mod tests; diff --git a/crates/core/src/commands/wait_surface_tests.rs b/crates/core/src/commands/wait_surface_tests.rs new file mode 100644 index 00000000..20426e55 --- /dev/null +++ b/crates/core/src/commands/wait_surface_tests.rs @@ -0,0 +1,38 @@ +use super::SurfaceWait; + +#[test] +fn no_flags_selects_no_surface_wait() { + assert_eq!(SurfaceWait::from_flags(false, false, false).unwrap(), None); +} + +#[test] +fn each_flag_maps_to_its_variant() { + assert_eq!( + SurfaceWait::from_flags(true, false, false).unwrap(), + Some(SurfaceWait::Menu) + ); + assert_eq!( + SurfaceWait::from_flags(false, true, false).unwrap(), + Some(SurfaceWait::MenuClosed) + ); + assert_eq!( + SurfaceWait::from_flags(false, false, true).unwrap(), + Some(SurfaceWait::Notification) + ); +} + +#[test] +fn conflicting_flags_report_exactly_one_mode_error() { + let err = SurfaceWait::from_flags(true, false, true).unwrap_err(); + + assert_eq!(err.code(), "INVALID_ARGS"); + assert_eq!(err.to_string(), "wait accepts exactly one mode"); + assert!(err.suggestion().is_some()); +} + +#[test] +fn menu_and_menu_closed_together_are_rejected() { + let err = SurfaceWait::from_flags(true, true, false).unwrap_err(); + + assert_eq!(err.code(), "INVALID_ARGS"); +} diff --git a/crates/core/src/commands/wait_test_support.rs b/crates/core/src/commands/wait_test_support.rs index 8716659a..be3886ff 100644 --- a/crates/core/src/commands/wait_test_support.rs +++ b/crates/core/src/commands/wait_test_support.rs @@ -22,9 +22,7 @@ pub(super) fn wait_args() -> WaitArgs { element: None, window: None, text: None, - menu: false, - menu_closed: false, - notification: false, + surface: None, event: None, window_id: None, }, diff --git a/crates/core/src/commands/wait_tests.rs b/crates/core/src/commands/wait_tests.rs index 1617a200..e2357e05 100644 --- a/crates/core/src/commands/wait_tests.rs +++ b/crates/core/src/commands/wait_tests.rs @@ -157,7 +157,7 @@ fn notification(index: usize, title: &str) -> NotificationInfo { fn notification_wait_args(timeout_ms: u64) -> WaitArgs { WaitArgs { mode: WaitModeArgs { - notification: true, + surface: Some(SurfaceWait::Notification), ..wait_args().mode }, timeout_ms, @@ -188,7 +188,7 @@ fn notification_wait_propagates_adapter_error() { let err = execute( WaitArgs { mode: WaitModeArgs { - notification: true, + surface: Some(SurfaceWait::Notification), ..wait_args().mode }, ..wait_args() @@ -372,7 +372,7 @@ fn notification_wait_allows_text_filter() { let result = validate_wait_mode(&WaitArgs { mode: WaitModeArgs { text: Some("done".into()), - notification: true, + surface: Some(SurfaceWait::Notification), ..wait_args().mode }, ..wait_args() diff --git a/crates/core/src/live_locator/resolve.rs b/crates/core/src/live_locator/resolve.rs index e3c16e62..814a0b30 100644 --- a/crates/core/src/live_locator/resolve.rs +++ b/crates/core/src/live_locator/resolve.rs @@ -28,14 +28,7 @@ pub fn resolve_query( return Err(transient_incomplete_timeout(deadline, &aggregate, last_incomplete).into()); } let attempt_request = LocatorResolveRequest { ..*request }; - match resolve_query_attempt( - adapter, - query, - root, - &attempt_request, - deadline, - &mut aggregate, - ) { + match resolve_query_attempt(adapter, query, root, &attempt_request, &mut aggregate) { Ok(mut resolution) => { resolution.stats.reads.counts.observation_attempts = resolution.stats.reads.counts.observation_attempts.max(1); @@ -176,11 +169,10 @@ fn resolve_query_attempt( query: &LocatorQuery, root: ObservationRoot<'_>, request: &LocatorResolveRequest, - deadline: crate::Deadline, aggregate: &mut LocatorStats, ) -> Result { let observation_request = - ObservationRequest::locator_for_root(query, request, root, deadline).validate()?; + ObservationRequest::locator_for_root(query, request, root, request.deadline).validate()?; let tree = crate::renderer_accessibility::observe_tree(adapter, root, &observation_request)?; let mut tree = tree; tree.stats.reads.counts.observation_attempts = diff --git a/crates/core/src/private_file.rs b/crates/core/src/private_file.rs index cbbea51c..3b053892 100644 --- a/crates/core/src/private_file.rs +++ b/crates/core/src/private_file.rs @@ -74,6 +74,7 @@ pub(crate) fn write_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<()> { bytes, crate::private_file_parent::ensure_private, sync_directory, + validate_private_destination, ) } @@ -83,6 +84,7 @@ pub(crate) fn write_user_atomic(path: &Path, bytes: &[u8]) -> std::io::Result<() bytes, crate::private_file_parent::ensure_user, sync_user_directory, + validate_user_destination, ) } @@ -91,16 +93,13 @@ fn write_atomic_with( bytes: &[u8], ensure_parent: fn(&Path) -> std::io::Result<()>, sync_parent: fn(&Path) -> std::io::Result<()>, + validate_destination: fn(&Path) -> std::io::Result<()>, ) -> std::io::Result<()> { let parent = path .parent() .ok_or_else(|| invalid_input("private file path has no parent"))?; ensure_parent(parent)?; - match open_private_read(path) { - Ok(_) => {} - Err(error) if error.kind() == std::io::ErrorKind::NotFound => {} - Err(error) => return Err(error), - } + validate_destination(path)?; let (temporary, mut file) = create_temporary(path)?; let result = (|| { file.write_all(bytes)?; @@ -118,6 +117,28 @@ fn write_atomic_with( result } +fn validate_private_destination(path: &Path) -> std::io::Result<()> { + match open_private_read(path) { + Ok(_) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + } +} + +fn validate_user_destination(path: &Path) -> std::io::Result<()> { + match std::fs::symlink_metadata(path) { + Ok(metadata) if metadata.file_type().is_symlink() => { + Err(invalid_input("user output path is a symlink")) + } + Ok(metadata) if !metadata.is_file() => { + Err(invalid_input("user output path is not a regular file")) + } + Ok(_) => Ok(()), + Err(error) if error.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(error) => Err(error), + } +} + pub(crate) fn validate_private_regular(file: &File) -> std::io::Result { let metadata = validate_regular(file)?; #[cfg(unix)] diff --git a/crates/core/src/private_file_tests.rs b/crates/core/src/private_file_tests.rs index ea6a12e4..5791267f 100644 --- a/crates/core/src/private_file_tests.rs +++ b/crates/core/src/private_file_tests.rs @@ -107,6 +107,69 @@ fn private_write_rejects_an_intermediate_directory_symlink() { std::fs::remove_dir_all(directory).unwrap(); } +#[test] +fn user_write_overwrites_an_existing_group_readable_file() { + let directory = directory("user-overwrite"); + std::fs::set_permissions(&directory, std::fs::Permissions::from_mode(0o755)).unwrap(); + let path = directory.join("out.png"); + std::fs::write(&path, b"old").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + write_user_atomic(&path, b"new").unwrap(); + + assert_eq!(std::fs::read(&path).unwrap(), b"new"); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn user_write_refuses_symlink_and_directory_destinations() { + let directory = directory("user-refuse"); + let target = directory.join("target"); + std::fs::write(&target, b"kept").unwrap(); + let symlink = directory.join("symlink"); + std::os::unix::fs::symlink(&target, &symlink).unwrap(); + let subdirectory = directory.join("subdirectory"); + std::fs::create_dir(&subdirectory).unwrap(); + + assert_eq!( + write_user_atomic(&symlink, b"new").unwrap_err().kind(), + std::io::ErrorKind::InvalidData + ); + assert_eq!( + write_user_atomic(&subdirectory, b"new").unwrap_err().kind(), + std::io::ErrorKind::InvalidData + ); + assert_eq!(std::fs::read(&target).unwrap(), b"kept"); + + let error = crate::refs::write_user_file(&symlink, b"new").unwrap_err(); + assert_eq!(error.code(), "INVALID_ARGS"); + assert!( + error + .suggestion() + .unwrap() + .contains(&symlink.display().to_string()) + ); + std::fs::remove_dir_all(directory).unwrap(); +} + +#[test] +fn private_write_still_rejects_a_group_readable_destination() { + let directory = directory("private-loose-destination"); + let path = directory.join("data"); + std::fs::write(&path, b"old").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + + let error = write_atomic(&path, b"new").unwrap_err(); + + assert_eq!(error.kind(), std::io::ErrorKind::PermissionDenied); + assert_eq!(std::fs::read(&path).unwrap(), b"old"); + std::fs::remove_dir_all(directory).unwrap(); +} + #[test] fn user_write_allows_the_system_temporary_directory() { let path = Path::new("/tmp").join(format!( diff --git a/crates/core/src/ref_action.rs b/crates/core/src/ref_action.rs index 272931e8..2e99b457 100644 --- a/crates/core/src/ref_action.rs +++ b/crates/core/src/ref_action.rs @@ -150,22 +150,20 @@ pub(crate) fn capture_pre_artifact( } pub(crate) fn finish_artifacts( - context: &CommandContext, - adapter: &dyn PlatformAdapter, - entry: &RefEntry, - ref_id: &str, + target: RefActionContext<'_>, pre: &crate::trace_artifacts::ArtifactOutcome, - deadline: crate::Deadline, ) { let post = crate::trace_artifacts::capture_action_screenshot( - context, - adapter, - entry, + target.context, + target.adapter, + target.entry, "post", - trace_capture_deadline(deadline), + trace_capture_deadline(target.deadline), ); - if let Err(error) = crate::trace_artifacts::emit_action_artifacts(context, ref_id, pre, &post) { - tracing::warn!(error = %error, ref_id, "action artifact emission failed"); + if let Err(error) = + crate::trace_artifacts::emit_action_artifacts(target.context, target.ref_id, pre, &post) + { + tracing::warn!(error = %error, ref_id = target.ref_id, "action artifact emission failed"); } } @@ -257,13 +255,8 @@ pub(crate) fn execute_resolved( let stability = actionability::StabilityExpectation::permissive(target.entry.geometry.bounds_hash); preflight_resolved(&target, &request, stability)?; - let context = target.context; - let adapter = target.adapter; - let entry = target.entry; - let ref_id = target.ref_id; - let deadline = target.deadline; let result = dispatch_resolved(target.target, request, lease); - finish_artifacts(context, adapter, entry, ref_id, &pre, deadline); + finish_artifacts(target.target, &pre); result } @@ -277,12 +270,14 @@ fn check_actionability_with_trace( || json!({ "ref": target.ref_id, "action": request.action.name() }), )?; let report = actionability::check_live_with_stability( - target.entry, - target.handle, - target.adapter, + &actionability::LiveCheckTarget { + entry: target.entry, + handle: target.handle, + adapter: target.adapter, + deadline: target.deadline, + }, request, stability, - target.deadline, ) .inspect_err(|err| { let _ = target.context.trace_lazy("actionability.check.error", || { diff --git a/crates/core/src/ref_action_wait.rs b/crates/core/src/ref_action_wait.rs index e1ae08bd..4cbf0d12 100644 --- a/crates/core/src/ref_action_wait.rs +++ b/crates/core/src/ref_action_wait.rs @@ -23,14 +23,7 @@ pub(crate) fn execute_with_auto_wait( let (result, lease, pre, deadline, _lease_started) = execute_with_auto_wait_and_lease(context, request, dispatch)?; drop(lease); - crate::ref_action::finish_artifacts( - context.context, - context.adapter, - context.entry, - context.ref_id, - &pre, - deadline, - ); + crate::ref_action::finish_artifacts(RefActionContext::new(context, deadline), &pre); Ok(result) } @@ -108,14 +101,7 @@ fn execute_with_deadline( Ok(result) => result, Err(error) => { drop(lease); - crate::ref_action::finish_artifacts( - context.context, - context.adapter, - context.entry, - context.ref_id, - &pre, - deadline, - ); + crate::ref_action::finish_artifacts(RefActionContext::new(context, deadline), &pre); return Err(error); } }; diff --git a/crates/core/src/refs.rs b/crates/core/src/refs.rs index f7372326..29e7e103 100644 --- a/crates/core/src/refs.rs +++ b/crates/core/src/refs.rs @@ -206,7 +206,19 @@ pub(crate) fn write_private_file(path: &Path, bytes: &[u8]) -> Result<(), AppErr } pub(crate) fn write_user_file(path: &Path, bytes: &[u8]) -> Result<(), AppError> { - crate::private_file::write_user_atomic(path, bytes).map_err(AppError::from) + crate::private_file::write_user_atomic(path, bytes).map_err(|error| { + if error.kind() == std::io::ErrorKind::InvalidData { + AppError::invalid_input_with_suggestion( + format!("Cannot write output file: {error}"), + format!( + "Pass an output path that is not '{}' or remove the conflicting entry there", + path.display() + ), + ) + } else { + AppError::from(error) + } + }) } pub(crate) fn is_symlink(path: &Path) -> bool { diff --git a/crates/core/src/refs_store.rs b/crates/core/src/refs_store.rs index 5904ef6c..bec98f29 100644 --- a/crates/core/src/refs_store.rs +++ b/crates/core/src/refs_store.rs @@ -9,7 +9,7 @@ use std::path::{Path, PathBuf}; const LATEST_SNAPSHOT_FILE: &str = "latest_snapshot_id"; const MAX_SAVED_SNAPSHOTS: usize = 512; -const STALE_TMP_MAX_AGE: std::time::Duration = std::time::Duration::from_secs(60); +pub(crate) const STALE_TMP_MAX_AGE: std::time::Duration = std::time::Duration::from_secs(60); #[derive(Debug, Clone)] pub struct RefStore { @@ -60,22 +60,6 @@ impl RefStore { self.with_write_lock(|| self.save_snapshot_unlocked(snapshot_id, refmap)) } - pub fn save_existing_snapshot( - &self, - snapshot_id: &str, - refmap: &RefMap, - ) -> Result<(), AppError> { - validate_snapshot_id(snapshot_id)?; - self.with_write_lock(|| { - if !self.snapshot_path(snapshot_id).is_file() { - return Err(AppError::Adapter(AdapterError::snapshot_not_found( - snapshot_id, - ))); - } - self.save_snapshot_unlocked(snapshot_id, refmap) - }) - } - pub(crate) fn update_existing_snapshot( &self, snapshot_id: &str, @@ -243,11 +227,12 @@ impl RefStore { } /// Pruning logic is a sibling `#[path]` module rather than a separate crate -/// module so it can access `base_dir`/`snapshots_dir` directly. Exposing them -/// as `pub(crate)` would widen the visibility surface to every module in the -/// crate; the path declaration keeps them private to this module tree. +/// module so it can access `base_dir`/`snapshots_dir` directly, without +/// widening those fields' visibility beyond this module tree. The module +/// itself is `pub(crate)` so its standalone, non-`RefStore` age-based-prune +/// helper is reachable from other commands that need the same TTL sweep. #[path = "refs_store_prune.rs"] -mod prune; +pub(crate) mod prune; #[cfg(test)] #[path = "refs_store_tests.rs"] diff --git a/crates/core/src/refs_store_prune.rs b/crates/core/src/refs_store_prune.rs index 21de478d..01545686 100644 --- a/crates/core/src/refs_store_prune.rs +++ b/crates/core/src/refs_store_prune.rs @@ -7,40 +7,15 @@ impl RefStore { /// the temp write and the atomic rename. Runs under the store write lock; /// the age threshold keeps any in-flight write from another process safe. pub(crate) fn remove_tmp_files_older_than(&self, max_age: std::time::Duration) { - self.remove_tmp_files_in_dir(&self.base_dir, max_age); + remove_stale_files_in_dir(&self.base_dir, max_age, is_orphaned_tmp_file); let snapshots_dir = self.snapshots_dir(); - self.remove_tmp_files_in_dir(&snapshots_dir, max_age); + remove_stale_files_in_dir(&snapshots_dir, max_age, is_orphaned_tmp_file); let Ok(entries) = std::fs::read_dir(snapshots_dir) else { return; }; for entry in entries.flatten() { if entry.file_type().is_ok_and(|kind| kind.is_dir()) { - self.remove_tmp_files_in_dir(&entry.path(), max_age); - } - } - } - - fn remove_tmp_files_in_dir(&self, dir: &std::path::Path, max_age: std::time::Duration) { - let Ok(entries) = std::fs::read_dir(dir) else { - return; - }; - for entry in entries.flatten() { - let path = entry.path(); - if path.extension().is_none_or(|ext| ext != "tmp") { - continue; - } - let is_plain_file = entry.file_type().is_ok_and(|kind| kind.is_file()); - if !is_plain_file { - continue; - } - let stale = entry - .metadata() - .ok() - .and_then(|metadata| metadata.modified().ok()) - .and_then(|modified| modified.elapsed().ok()) - .is_some_and(|age| age >= max_age); - if stale { - let _ = std::fs::remove_file(&path); + remove_stale_files_in_dir(&entry.path(), max_age, is_orphaned_tmp_file); } } } @@ -86,3 +61,41 @@ impl RefStore { Ok(()) } } + +pub(crate) fn is_orphaned_tmp_file(path: &std::path::Path) -> bool { + path.extension().is_some_and(|ext| ext == "tmp") +} + +/// Removes plain files directly under `dir` that satisfy `matches` and whose +/// mtime is at least `max_age` old. Never descends into subdirectories and +/// never removes a directory itself. Shared by refstore `*.tmp` cleanup and +/// the sessionless clipboard image sweep so both age-based prunes stay on one +/// implementation. +pub(crate) fn remove_stale_files_in_dir( + dir: &std::path::Path, + max_age: std::time::Duration, + matches: impl Fn(&std::path::Path) -> bool, +) { + let Ok(entries) = std::fs::read_dir(dir) else { + return; + }; + for entry in entries.flatten() { + let path = entry.path(); + if !matches(&path) { + continue; + } + let is_plain_file = entry.file_type().is_ok_and(|kind| kind.is_file()); + if !is_plain_file { + continue; + } + let stale = entry + .metadata() + .ok() + .and_then(|metadata| metadata.modified().ok()) + .and_then(|modified| modified.elapsed().ok()) + .is_some_and(|age| age >= max_age); + if stale { + let _ = std::fs::remove_file(&path); + } + } +} diff --git a/crates/core/src/refs_store_pruning_tests.rs b/crates/core/src/refs_store_pruning_tests.rs index a70cd3b6..e04c0a80 100644 --- a/crates/core/src/refs_store_pruning_tests.rs +++ b/crates/core/src/refs_store_pruning_tests.rs @@ -30,14 +30,14 @@ fn save_new_snapshot_prunes_old_snapshots_without_removing_latest() { } #[test] -fn save_existing_refuses_to_recreate_a_missing_snapshot() { +fn update_existing_refuses_to_recreate_a_missing_snapshot() { let _guard = HomeGuard::new(); let store = RefStore::new().unwrap(); let snapshot_id = store.save_new_snapshot(&map_with("Original")).unwrap(); std::fs::remove_dir_all(store.snapshots_dir().join(&snapshot_id)).unwrap(); let err = store - .save_existing_snapshot(&snapshot_id, &map_with("Recreated")) + .update_existing_snapshot(&snapshot_id, "@e1", &entry("Original"), |_| Ok(())) .unwrap_err(); assert_eq!(err.code(), "SNAPSHOT_NOT_FOUND"); diff --git a/crates/core/src/refs_store_tests.rs b/crates/core/src/refs_store_tests.rs index c4feb2a4..1ad04353 100644 --- a/crates/core/src/refs_store_tests.rs +++ b/crates/core/src/refs_store_tests.rs @@ -149,14 +149,14 @@ fn explicit_snapshot_id_remains_in_its_session_namespace() { } #[test] -fn save_existing_snapshot_cannot_cross_session_namespaces() { +fn update_existing_snapshot_cannot_cross_session_namespaces() { 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(); let err = default_store - .save_existing_snapshot(&snapshot_id, &map_with("Updated")) + .update_existing_snapshot(&snapshot_id, "@e1", &entry("Session A"), |_| Ok(())) .unwrap_err(); assert_eq!(err.code(), "SNAPSHOT_NOT_FOUND"); @@ -266,16 +266,19 @@ fn read_latest_rejects_symlinked_pointer() { } #[test] -fn save_existing_snapshot_does_not_promote_latest_pointer() { +fn update_existing_snapshot_does_not_promote_latest_pointer() { let _guard = HomeGuard::new(); let store = RefStore::new().unwrap(); - let mut first = map_with("First"); - let first_id = store.save_new_snapshot(&first).unwrap(); + let first_id = store.save_new_snapshot(&map_with("First")).unwrap(); let second_id = store.save_new_snapshot(&map_with("Second")).unwrap(); - first.allocate(entry("First Child")); - store.save_existing_snapshot(&first_id, &first).unwrap(); + store + .update_existing_snapshot(&first_id, "@e1", &entry("First"), |map| { + map.allocate(entry("First Child")); + Ok(()) + }) + .unwrap(); assert_eq!( store.latest_snapshot_id().unwrap().as_deref(), diff --git a/crates/ffi/include/agent_desktop.h b/crates/ffi/include/agent_desktop.h index 4cd522ae..39b4e422 100644 --- a/crates/ffi/include/agent_desktop.h +++ b/crates/ffi/include/agent_desktop.h @@ -813,7 +813,14 @@ typedef struct AdWaitScope { } AdWaitScope; /** - * Arguments for `ad_wait`, mirroring `core::commands::wait::WaitArgs`. + * Arguments for `ad_wait`, mirroring `core::commands::wait::WaitArgs` for + * the pause/element/text/surface wait modes and predicates. + * + * The core event-wait mode (`--event` / `--window-id`) is intentionally not + * exposed over FFI in this release; `wait_args_from_ffi` always forwards + * `event: None` and `window_id: None` to core. `mode.window` here is a + * title-appearance wait (poll until a window with the given title exists), + * which is a distinct semantic from the deferred event-wait mode. * * Mode, predicate, and scope fields are grouped into named PODs. Optional * numbers use `AdOptional*`; optional strings are nullable pointers. @@ -1060,9 +1067,9 @@ AdResult ad_init(uint32_t expected_major); * `action` must be a non-null pointer to a valid `AdAction`. * `out` must be a non-null pointer to an `AdActionResult` to write the result into. * - * This legacy entrypoint cannot express process-generation or typed native-id - * evidence and therefore fails closed. Use - * ad_execute_ref_action_exact_with_policy. + * Handles come from exact resolvers and already carry process-generation + * evidence, so this executes under the same policy as + * `ad_execute_action_with_policy`. */ AdResult ad_execute_action(const struct AdAdapter *adapter, const struct AdNativeHandle *handle, diff --git a/crates/ffi/src/actions/execute.rs b/crates/ffi/src/actions/execute.rs index 705e6585..2730e899 100644 --- a/crates/ffi/src/actions/execute.rs +++ b/crates/ffi/src/actions/execute.rs @@ -23,9 +23,9 @@ use agent_desktop_core::{Action, ActionRequest}; /// `action` must be a non-null pointer to a valid `AdAction`. /// `out` must be a non-null pointer to an `AdActionResult` to write the result into. /// -/// This legacy entrypoint cannot express process-generation or typed native-id -/// evidence and therefore fails closed. Use -/// ad_execute_ref_action_exact_with_policy. +/// Handles come from exact resolvers and already carry process-generation +/// evidence, so this executes under the same policy as +/// `ad_execute_action_with_policy`. #[unsafe(no_mangle)] pub unsafe extern "C" fn ad_execute_action( adapter: *const AdAdapter, diff --git a/crates/ffi/src/commands/wait.rs b/crates/ffi/src/commands/wait.rs index c4fc6613..33ccf5bd 100644 --- a/crates/ffi/src/commands/wait.rs +++ b/crates/ffi/src/commands/wait.rs @@ -8,6 +8,7 @@ use crate::pointer_guard::guard_non_null; use crate::types::wait_args::AdWaitArgs; use agent_desktop_core::AdapterError; use agent_desktop_core::commands::wait::{WaitArgs, WaitModeArgs, WaitPredicateArgs}; +use agent_desktop_core::commands::wait_surface::SurfaceWait; use std::ffi::c_char; use std::ptr; @@ -52,7 +53,8 @@ pub unsafe extern "C" fn ad_wait( trap_panic(|| { guard_non_null!(adapter, c"adapter is null"); - let wait_args = match wait_args_from_ffi(unsafe { &*args }) { + let ffi_args = unsafe { &*args }; + let mut wait_args = match wait_args_from_ffi(ffi_args) { Ok(args) => args, Err(err) => { error::set_last_error(&err); @@ -72,11 +74,15 @@ pub unsafe extern "C" fn ad_wait( let scope = crate::commands::command_scope!(ctx, "wait"); - let result = agent_desktop_core::commands::wait::execute( - wait_args, - adapter_ref.inner.as_ref(), - &ctx, - ); + let result = SurfaceWait::from_flags( + ffi_args.mode.surfaces.menu, + ffi_args.mode.surfaces.menu_closed, + ffi_args.mode.surfaces.notification, + ) + .and_then(|surface| { + wait_args.mode.surface = surface; + agent_desktop_core::commands::wait::execute(wait_args, adapter_ref.inner.as_ref(), &ctx) + }); crate::commands::complete_scope!(scope, &result); unsafe { write_command_envelope("wait", result, out) } @@ -90,9 +96,7 @@ fn wait_args_from_ffi(args: &AdWaitArgs) -> Result { element: optional_adapter_string(args.mode.element, "mode.element")?, window: optional_adapter_string(args.mode.window, "mode.window")?, text: optional_adapter_string(args.mode.text, "mode.text")?, - menu: args.mode.surfaces.menu, - menu_closed: args.mode.surfaces.menu_closed, - notification: args.mode.surfaces.notification, + surface: None, event: None, window_id: None, }, diff --git a/crates/ffi/src/types/wait_args.rs b/crates/ffi/src/types/wait_args.rs index 622c0769..fbc58689 100644 --- a/crates/ffi/src/types/wait_args.rs +++ b/crates/ffi/src/types/wait_args.rs @@ -1,6 +1,13 @@ use crate::types::{AdWaitMode, AdWaitPredicate, AdWaitScope}; -/// Arguments for `ad_wait`, mirroring `core::commands::wait::WaitArgs`. +/// Arguments for `ad_wait`, mirroring `core::commands::wait::WaitArgs` for +/// the pause/element/text/surface wait modes and predicates. +/// +/// The core event-wait mode (`--event` / `--window-id`) is intentionally not +/// exposed over FFI in this release; `wait_args_from_ffi` always forwards +/// `event: None` and `window_id: None` to core. `mode.window` here is a +/// title-appearance wait (poll until a window with the given title exists), +/// which is a distinct semantic from the deferred event-wait mode. /// /// Mode, predicate, and scope fields are grouped into named PODs. Optional /// numbers use `AdOptional*`; optional strings are nullable pointers. diff --git a/crates/macos/src/actions/chain.rs b/crates/macos/src/actions/chain.rs index 5ba2dee2..4c0df0fb 100644 --- a/crates/macos/src/actions/chain.rs +++ b/crates/macos/src/actions/chain.rs @@ -52,11 +52,27 @@ mod imp { tracing::debug!("chain: all {total} steps exhausted"); Err( AdapterError::new(ErrorCode::ActionFailed, "All chain steps exhausted") - .with_disposition(agent_desktop_core::DeliverySemantics::not_delivered()) + .with_disposition(exhaustion_disposition(&steps)) .with_suggestion(def.suggestion), ) } + pub(crate) fn exhaustion_disposition( + steps: &[ActionStep], + ) -> agent_desktop_core::DeliverySemantics { + let delivered = steps.iter().any(|step| { + matches!( + step.outcome, + agent_desktop_core::ActionStepOutcome::Succeeded + ) + }); + if delivered { + agent_desktop_core::DeliverySemantics::delivered_unverified() + } else { + agent_desktop_core::DeliverySemantics::not_delivered() + } + } + pub(crate) fn step_mechanism(step: &ChainStep) -> StepMechanism { match step { ChainStep::CGClick { .. } | ChainStep::FocusThenClearByKeyboard => { @@ -115,7 +131,9 @@ mod imp { } #[cfg(all(test, target_os = "macos"))] -pub(crate) use imp::{build_step, record_step_outcome, step_allowed, step_mechanism}; +pub(crate) use imp::{ + build_step, exhaustion_disposition, record_step_outcome, step_allowed, step_mechanism, +}; #[cfg(test)] #[path = "chain_tests.rs"] diff --git a/crates/macos/src/actions/chain_disclosure_steps.rs b/crates/macos/src/actions/chain_disclosure_steps.rs index 2228cd46..d8347ddd 100644 --- a/crates/macos/src/actions/chain_disclosure_steps.rs +++ b/crates/macos/src/actions/chain_disclosure_steps.rs @@ -32,10 +32,9 @@ mod imp { let action = if expanded { "AXExpand" } else { "AXCollapse" }; prepare(element, deadline)?; if crate::actions::ax_helpers::try_ax_action_or_err(element, action, deadline)? { - if verify_disclosure(element, expanded, deadline).map_err(after_delivery)? - == DeliveryOutcome::DeliveredVerified - { - return Ok(DeliveryOutcome::DeliveredVerified); + let outcome = verify_disclosure(element, expanded, deadline).map_err(after_delivery)?; + if let Some(delivered) = stop_if_delivered(outcome) { + return Ok(delivered); } } prepare(element, deadline)?; @@ -47,26 +46,30 @@ mod imp { expanded, deadline, )? { - if verify_disclosure(element, expanded, deadline).map_err(after_delivery)? - == DeliveryOutcome::DeliveredVerified - { - return Ok(DeliveryOutcome::DeliveredVerified); + let outcome = + verify_disclosure(element, expanded, deadline).map_err(after_delivery)?; + if let Some(delivered) = stop_if_delivered(outcome) { + return Ok(delivered); } } } if allow_toggle && disclosed_state(element, deadline)? == Some(!expanded) { prepare(element, deadline)?; if crate::actions::ax_helpers::try_ax_action_or_err(element, "AXPress", deadline)? { - if verify_disclosure(element, expanded, deadline).map_err(after_delivery)? - == DeliveryOutcome::DeliveredVerified - { - return Ok(DeliveryOutcome::DeliveredVerified); + let outcome = + verify_disclosure(element, expanded, deadline).map_err(after_delivery)?; + if let Some(delivered) = stop_if_delivered(outcome) { + return Ok(delivered); } } } Ok(DeliveryOutcome::NotDelivered) } + fn stop_if_delivered(outcome: DeliveryOutcome) -> Option { + outcome.was_delivered().then_some(outcome) + } + fn disclosure_plan(current: Option, desired: bool) -> (bool, bool) { (current == Some(desired), current == Some(!desired)) } @@ -132,7 +135,7 @@ mod imp { #[cfg(test)] mod tests { - use super::{DeliveryOutcome, disclosure_plan, unobserved_delivery}; + use super::{DeliveryOutcome, disclosure_plan, stop_if_delivered, unobserved_delivery}; #[test] fn disclosure_plan_never_blindly_toggles_unknown_state() { @@ -154,6 +157,25 @@ mod imp { unobserved_delivery(None, true).expect("unreadable state after delivery"), DeliveryOutcome::DeliveredUnverified ); + assert_eq!( + stop_if_delivered( + unobserved_delivery(None, true).expect("unreadable state after delivery") + ), + Some(DeliveryOutcome::DeliveredUnverified) + ); + } + + #[test] + fn delivered_outcomes_halt_further_mutation_strategies() { + assert_eq!( + stop_if_delivered(DeliveryOutcome::DeliveredUnverified), + Some(DeliveryOutcome::DeliveredUnverified) + ); + assert_eq!( + stop_if_delivered(DeliveryOutcome::DeliveredVerified), + Some(DeliveryOutcome::DeliveredVerified) + ); + assert_eq!(stop_if_delivered(DeliveryOutcome::NotDelivered), None); } } } diff --git a/crates/macos/src/actions/chain_tests.rs b/crates/macos/src/actions/chain_tests.rs index 0cb0ec69..705f66d3 100644 --- a/crates/macos/src/actions/chain_tests.rs +++ b/crates/macos/src/actions/chain_tests.rs @@ -1,4 +1,7 @@ -use super::{ChainStep, build_step, record_step_outcome, step_allowed, step_mechanism}; +use super::{ + ChainStep, build_step, exhaustion_disposition, record_step_outcome, step_allowed, + step_mechanism, +}; use crate::actions::chain_delivery::DeliveryOutcome; use agent_desktop_core::MouseButton; use agent_desktop_core::step_mechanism::StepMechanism; @@ -196,3 +199,45 @@ fn non_idempotent_chain_stops_after_unverified_delivery() { false, )); } + +#[test] +fn exhaustion_after_unverified_delivery_reports_delivered_unverified() { + let mut steps = Vec::new(); + assert!(!record_step_outcome( + &mut steps, + &ChainStep::FocusThenClearByKeyboard, + DeliveryOutcome::DeliveredUnverified, + true, + )); + assert!(!record_step_outcome( + &mut steps, + &ChainStep::SetDynamic { attr: "AXValue" }, + DeliveryOutcome::NotDelivered, + true, + )); + + assert_eq!( + exhaustion_disposition(&steps), + agent_desktop_core::DeliverySemantics::delivered_unverified() + ); +} + +#[test] +fn exhaustion_without_any_delivery_reports_not_delivered() { + let mut steps = Vec::new(); + assert!(!record_step_outcome( + &mut steps, + &ChainStep::Action("AXPress"), + DeliveryOutcome::NotDelivered, + false, + )); + + assert_eq!( + exhaustion_disposition(&steps), + agent_desktop_core::DeliverySemantics::not_delivered() + ); + assert_eq!( + exhaustion_disposition(&[]), + agent_desktop_core::DeliverySemantics::not_delivered() + ); +} diff --git a/crates/macos/src/input/mouse.rs b/crates/macos/src/input/mouse.rs index f4a21981..7efe5f96 100644 --- a/crates/macos/src/input/mouse.rs +++ b/crates/macos/src/input/mouse.rs @@ -62,11 +62,13 @@ mod imp { MouseEventKind::Click { count } => { agent_desktop_core::validate_mouse_click_count(count)?; synthesize_click( - point, - cg_button, - &event.button, - count, - flags, + ClickSpec { + point, + cg_button, + button: &event.button, + count, + flags, + }, deadline, verify_target, ) @@ -125,30 +127,34 @@ mod imp { Ok(rounded as i32) } - fn synthesize_click( + struct ClickSpec<'a> { point: CGPoint, cg_button: CGMouseButton, - button: &MouseButton, + button: &'a MouseButton, count: u32, flags: CGEventFlags, + } + + fn synthesize_click( + spec: ClickSpec, deadline: Deadline, verify_target: &mut dyn FnMut() -> Result<(), AdapterError>, ) -> Result<(), AdapterError> { - let down_ty = down_type(button); - let up_ty = up_type(button); + let down_ty = down_type(spec.button); + let up_ty = up_type(spec.button); let mut delivery = crate::actions::DeliveryTracker::default(); crate::input::mouse_move::post_move_events( - point, - cg_button, - flags, + spec.point, + spec.cg_button, + spec.flags, deadline, &mut delivery, )?; - for i in 1..=count { + for i in 1..=spec.count { ensure_budget(deadline, delivery)?; - let down = create_event(down_ty, point, cg_button, flags) + let down = create_event(down_ty, spec.point, spec.cg_button, spec.flags) .map_err(|error| delivery.annotate(error))?; - let up = create_event(up_ty, point, cg_button, flags) + let up = create_event(up_ty, spec.point, spec.cg_button, spec.flags) .map_err(|error| delivery.annotate(error))?; set_click_count(&down, i as i64); set_click_count(&up, i as i64); @@ -167,7 +173,7 @@ mod imp { .map_err(|error| delivery.annotate(error))?; up.post(CGEventTapLocation::HID); ensure_budget(deadline, delivery)?; - if i < count { + if i < spec.count { sleep_bounded(deadline, std::time::Duration::from_millis(30), delivery)?; } } @@ -222,15 +228,14 @@ mod imp { source: &CGEventSource, event: (CGEventType, CGPoint, CGMouseButton, CGEventFlags), deadline: Deadline, - delivery: crate::actions::DeliveryTracker, + delivery: &mut crate::actions::DeliveryTracker, ) -> Result<(), AdapterError> { - ensure_budget(deadline, delivery)?; + ensure_budget(deadline, *delivery)?; let ev = create_event_with_source(source, event.0, event.1, event.2, event.3) .map_err(|error| delivery.annotate(error))?; ev.post(CGEventTapLocation::HID); - let mut delivery = delivery; delivery.mark_delivered(); - ensure_budget(deadline, delivery) + ensure_budget(deadline, *delivery) } fn to_cg_button(button: &MouseButton) -> CGMouseButton { diff --git a/crates/macos/src/input/mouse_drag.rs b/crates/macos/src/input/mouse_drag.rs index b737c1b8..8c7d2930 100644 --- a/crates/macos/src/input/mouse_drag.rs +++ b/crates/macos/src/input/mouse_drag.rs @@ -73,7 +73,7 @@ fn drag_sequence(params: DragParams, deadline: Deadline) -> Result<(), AdapterEr CGEventFlags::empty(), ), deadline, - release.delivery(), + release.delivery_mut(), )?; crate::input::mouse::sleep_bounded(deadline, step_delay, release.delivery())?; } @@ -83,7 +83,7 @@ fn drag_sequence(params: DragParams, deadline: Deadline) -> Result<(), AdapterEr to, params.drop_delay_ms.unwrap_or(DEFAULT_DROP_DELAY_MS), deadline, - release.delivery(), + release.delivery_mut(), )?; release.release_at_destination(deadline) })(); @@ -142,6 +142,10 @@ impl DragReleaseGuard { self.delivery.delivery() } + fn delivery_mut(&mut self) -> &mut crate::actions::DeliveryTracker { + self.delivery.delivery_mut() + } + fn release_at_destination(&mut self, deadline: Deadline) -> Result<(), AdapterError> { crate::input::mouse::ensure_budget(deadline, self.delivery())?; let event = self.destination_up.take().ok_or_else(|| { @@ -171,7 +175,7 @@ fn dwell_over_destination( destination: CGPoint, delay_ms: u64, deadline: Deadline, - delivery: crate::actions::DeliveryTracker, + delivery: &mut crate::actions::DeliveryTracker, ) -> Result<(), AdapterError> { if delay_ms == 0 { return Ok(()); @@ -190,7 +194,7 @@ fn dwell_over_destination( delivery, )?; let tick_ms = remaining_ms.min(DWELL_TICK_MS); - crate::input::mouse::sleep_bounded(deadline, Duration::from_millis(tick_ms), delivery)?; + crate::input::mouse::sleep_bounded(deadline, Duration::from_millis(tick_ms), *delivery)?; remaining_ms -= tick_ms; } Ok(()) diff --git a/crates/macos/src/input/mouse_drag_state.rs b/crates/macos/src/input/mouse_drag_state.rs index c56f8dff..000f165c 100644 --- a/crates/macos/src/input/mouse_drag_state.rs +++ b/crates/macos/src/input/mouse_drag_state.rs @@ -27,6 +27,10 @@ impl DragDeliveryState { self.delivery } + pub(crate) fn delivery_mut(&mut self) -> &mut crate::actions::DeliveryTracker { + &mut self.delivery + } + pub(crate) fn enrich_error(&self, mut error: AdapterError) -> AdapterError { error = self.delivery.annotate(error); if self.delivery.delivered_units() == 0 { @@ -75,6 +79,9 @@ mod tests { let mut state = DragDeliveryState::default(); state.arm(); state.mark_down_posted(); + for _ in 0..3 { + state.delivery_mut().mark_delivered(); + } let error = state.enrich_error(AdapterError::timeout("deadline")); assert_eq!( error.disposition, @@ -82,7 +89,7 @@ mod tests { ); let details = error.details.unwrap(); - assert_eq!(details["delivered_events"], 1); + assert_eq!(details["delivered_events"], 4); assert_eq!(details["emergency_release_posted"], true); } } diff --git a/crates/macos/src/notifications/nc_session.rs b/crates/macos/src/notifications/nc_session.rs index 0ab0f7f7..f3c52a44 100644 --- a/crates/macos/src/notifications/nc_session.rs +++ b/crates/macos/src/notifications/nc_session.rs @@ -31,6 +31,19 @@ pub(crate) struct NcSession { cleanup_on_drop: bool, } +struct NcSessionOps +where + Open: FnMut(Deadline) -> Result<(), AdapterError>, + WaitUntilReady: FnMut(Deadline) -> Result, + Close: FnMut(Deadline) -> Result<(), AdapterError>, + Reactivate: FnMut(&ProcessIdentity, Deadline) -> Result<(), AdapterError>, +{ + open: Open, + wait_until_ready: WaitUntilReady, + close: Close, + reactivate: Reactivate, +} + impl NcSession { pub(crate) fn open( policy: InteractionPolicy, @@ -54,35 +67,40 @@ impl NcSession { Self::open_with( previous_app, deadline, - open_nc, - wait_for_nc_ready, - close_nc, - reactivate_app, + NcSessionOps { + open: open_nc, + wait_until_ready: wait_for_nc_ready, + close: close_nc, + reactivate: reactivate_app, + }, ) } - fn open_with( + fn open_with( previous_app: Option, deadline: Deadline, - mut open: impl FnMut(Deadline) -> Result<(), AdapterError>, - mut wait_until_ready: impl FnMut(Deadline) -> Result, - close: impl FnMut(Deadline) -> Result<(), AdapterError>, - reactivate: impl FnMut(&ProcessIdentity, Deadline) -> Result<(), AdapterError>, - ) -> Result { + mut ops: NcSessionOps, + ) -> Result + where + Open: FnMut(Deadline) -> Result<(), AdapterError>, + WaitUntilReady: FnMut(Deadline) -> Result, + Close: FnMut(Deadline) -> Result<(), AdapterError>, + Reactivate: FnMut(&ProcessIdentity, Deadline) -> Result<(), AdapterError>, + { let mut session = Self { pid: 0, close_pending: true, previous_app, cleanup_on_drop: true, }; - let result = open(deadline).and_then(|()| wait_until_ready(deadline)); + let result = (ops.open)(deadline).and_then(|()| (ops.wait_until_ready)(deadline)); match result { Ok(pid) => { session.pid = pid; Ok(session) } Err(error) => { - let cleanup = session.cleanup_with(close, reactivate); + let cleanup = session.cleanup_with(ops.close, ops.reactivate); merge_session_result(Err(error), cleanup) } } diff --git a/crates/macos/src/notifications/nc_session_tests.rs b/crates/macos/src/notifications/nc_session_tests.rs index a3944de9..7cd804fb 100644 --- a/crates/macos/src/notifications/nc_session_tests.rs +++ b/crates/macos/src/notifications/nc_session_tests.rs @@ -1,4 +1,6 @@ -use super::{NcSession, closed_center_policy_error, merge_session_result, nc_pid_from_output}; +use super::{ + NcSession, NcSessionOps, closed_center_policy_error, merge_session_result, nc_pid_from_output, +}; use agent_desktop_core::{AdapterError, ErrorCode, ProcessIdentity}; #[test] @@ -152,18 +154,20 @@ fn partial_open_failure_closes_center_and_restores_previous_app() { let result = NcSession::open_with( Some(previous.clone()), agent_desktop_core::Deadline::after(0).unwrap(), - |_| Ok(()), - |_| Err(AdapterError::timeout("readiness failed")), - |deadline| { - assert!(!deadline.is_expired()); - close_attempts += 1; - Ok(()) - }, - |app, deadline| { - assert_eq!(app, &previous); - assert!(!deadline.is_expired()); - restore_attempts += 1; - Ok(()) + NcSessionOps { + open: |_| Ok(()), + wait_until_ready: |_| Err(AdapterError::timeout("readiness failed")), + close: |deadline| { + assert!(!deadline.is_expired()); + close_attempts += 1; + Ok(()) + }, + reactivate: |app, deadline| { + assert_eq!(app, &previous); + assert!(!deadline.is_expired()); + restore_attempts += 1; + Ok(()) + }, }, ); let error = match result { diff --git a/crates/macos/src/system/window_resolve.rs b/crates/macos/src/system/window_resolve.rs index 40bc4453..127653c7 100644 --- a/crates/macos/src/system/window_resolve.rs +++ b/crates/macos/src/system/window_resolve.rs @@ -3,6 +3,14 @@ use agent_desktop_core::{AdapterError, ErrorCode, WindowInfo}; use crate::system::cg_window::WindowRecord; use crate::tree::{AXElement, attributes::set_messaging_timeout, element_for_pid}; +pub(crate) struct WindowIdentityEvidence<'a> { + pub pid: i32, + pub app: Option<&'a str>, + pub process_instance: Option<&'a str>, + pub title: Option<&'a str>, + pub bounds_hash: Option, +} + pub(crate) fn window_element_for_info( win: &WindowInfo, deadline: agent_desktop_core::Deadline, @@ -55,24 +63,20 @@ fn locate_verified_record_until( pub(crate) fn verify_window_identity_until( id: &str, - pid: i32, - app: Option<&str>, - process_instance: Option<&str>, - title: Option<&str>, - bounds_hash: Option, + evidence: WindowIdentityEvidence<'_>, deadline: std::time::Instant, ) -> Result<(), AdapterError> { let window_number = parse_window_number(id).ok_or_else(|| invalid_window_id(id))?; let record = crate::system::cg_window_exact::exact_window_record_until(window_number, deadline)? .ok_or_else(|| window_not_found(id))?; - if !window_record_matches_source(&record, pid, app, process_instance, title, bounds_hash) { + if !window_record_matches_source(&record, &evidence) { return Err(window_identity_mismatch(id)); } - let Some(process_instance) = process_instance else { + let Some(process_instance) = evidence.process_instance else { return Err(window_identity_mismatch(id)); }; - if !crate::system::process_identity::matches_instance(pid, process_instance)? { + if !crate::system::process_identity::matches_instance(evidence.pid, process_instance)? { return Err(window_identity_mismatch(id)); } Ok(()) @@ -80,20 +84,18 @@ pub(crate) fn verify_window_identity_until( fn window_record_matches_source( record: &WindowRecord, - pid: i32, - app: Option<&str>, - process_instance: Option<&str>, - title: Option<&str>, - bounds_hash: Option, + evidence: &WindowIdentityEvidence<'_>, ) -> bool { - if record.pid != pid - || app.is_some_and(|app| !app.is_empty() && record.app_name != app) - || process_instance.is_none() - || record.process_instance.as_deref() != process_instance + if record.pid != evidence.pid + || evidence + .app + .is_some_and(|app| !app.is_empty() && record.app_name != app) + || evidence.process_instance.is_none() + || record.process_instance.as_deref() != evidence.process_instance { return false; } - let bounds_changed = bounds_hash.is_some_and(|expected| { + let bounds_changed = evidence.bounds_hash.is_some_and(|expected| { record .bounds .bounds_hash() @@ -101,17 +103,17 @@ fn window_record_matches_source( }); if bounds_changed { tracing::debug!( - expected_bounds_hash = ?bounds_hash, + expected_bounds_hash = ?evidence.bounds_hash, actual_bounds_hash = ?record.bounds.bounds_hash(), "window moved or resized while immutable source identity remained valid" ); } - let title_changed = title.is_some_and(|title| { + let title_changed = evidence.title.is_some_and(|title| { !title.is_empty() && record.title.as_deref().unwrap_or(record.app_name.as_str()) != title }); if title_changed { tracing::debug!( - expected_title = ?title, + expected_title = ?evidence.title, actual_title = ?record.title, "window title changed while immutable source identity remained valid" ); diff --git a/crates/macos/src/system/window_resolve_tests.rs b/crates/macos/src/system/window_resolve_tests.rs index db3b473c..6f205c6c 100644 --- a/crates/macos/src/system/window_resolve_tests.rs +++ b/crates/macos/src/system/window_resolve_tests.rs @@ -107,11 +107,13 @@ fn source_identity_survives_window_move_and_resize() { assert!(window_record_matches_source( &live, - pid, - Some("TextEdit"), - Some(process_instance.as_str()), - Some("Untitled"), - original_hash, + &WindowIdentityEvidence { + pid, + app: Some("TextEdit"), + process_instance: Some(process_instance.as_str()), + title: Some("Untitled"), + bounds_hash: original_hash, + }, )); } @@ -122,19 +124,23 @@ fn source_identity_still_rejects_pid_and_application_mismatch() { assert!(!window_record_matches_source( &live, - pid + 1, - Some("TextEdit"), - Some(instance(pid).as_str()), - Some("Untitled"), - None, + &WindowIdentityEvidence { + pid: pid + 1, + app: Some("TextEdit"), + process_instance: Some(instance(pid).as_str()), + title: Some("Untitled"), + bounds_hash: None, + }, )); assert!(!window_record_matches_source( &live, - pid, - Some("DifferentApp"), - Some(instance(pid).as_str()), - Some("Untitled"), - None, + &WindowIdentityEvidence { + pid, + app: Some("DifferentApp"), + process_instance: Some(instance(pid).as_str()), + title: Some("Untitled"), + bounds_hash: None, + }, )); } @@ -145,19 +151,23 @@ fn source_identity_rejects_missing_or_changed_process_generation() { assert!(!window_record_matches_source( &live, - pid, - Some("TextEdit"), - None, - Some("Untitled"), - None, + &WindowIdentityEvidence { + pid, + app: Some("TextEdit"), + process_instance: None, + title: Some("Untitled"), + bounds_hash: None, + }, )); assert!(!window_record_matches_source( &live, - pid, - Some("TextEdit"), - Some("different-generation"), - Some("Untitled"), - None, + &WindowIdentityEvidence { + pid, + app: Some("TextEdit"), + process_instance: Some("different-generation"), + title: Some("Untitled"), + bounds_hash: None, + }, )); } diff --git a/crates/macos/src/tree/child_labels.rs b/crates/macos/src/tree/child_labels.rs index 8e2566f4..59822855 100644 --- a/crates/macos/src/tree/child_labels.rs +++ b/crates/macos/src/tree/child_labels.rs @@ -2,19 +2,23 @@ use crate::tree::AXElement; pub(crate) const MAX_LABEL_ELEMENTS: usize = 5; +pub(crate) struct NameEvidenceSinks<'a> { + pub(crate) stats: &'a mut agent_desktop_core::LocatorStats, + pub(crate) usage: &'a mut crate::tree::observation_usage::ObservationUsage, +} + pub(crate) fn complete_name_evidence_with_deadline( attrs: &crate::tree::NodeAttrs, role: &str, children: &[AXElement], deadline: std::time::Instant, - stats: &mut agent_desktop_core::LocatorStats, - usage: &mut crate::tree::observation_usage::ObservationUsage, + mut sinks: NameEvidenceSinks<'_>, ) -> Result<(agent_desktop_core::NameEvidence, bool), agent_desktop_core::AdapterError> { let mut evidence = attrs.name_evidence.clone(); if !should_read_child_label(role, &evidence) { return Ok((evidence, true)); } - let (label, complete) = label_from_children(children, deadline, stats, usage)?; + let (label, complete) = label_from_children(children, deadline, &mut sinks)?; evidence.child_label = label; Ok((evidence, complete)) } @@ -43,27 +47,25 @@ fn has_name_without_child_content(evidence: &agent_desktop_core::NameEvidence) - fn label_from_children( children: &[AXElement], deadline: std::time::Instant, - stats: &mut agent_desktop_core::LocatorStats, - usage: &mut crate::tree::observation_usage::ObservationUsage, + sinks: &mut NameEvidenceSinks<'_>, ) -> Result<(Option, bool), agent_desktop_core::AdapterError> { let mut labels = Vec::new(); - note_label_limit(children.len(), stats); + note_label_limit(children.len(), sinks.stats); let mut complete = children.len() <= MAX_LABEL_ELEMENTS; for child in children.iter().take(MAX_LABEL_ELEMENTS) { - let (role, role_complete) = timed_string(child, "AXRole", deadline, stats, usage)?; + let (role, role_complete) = timed_string(child, "AXRole", deadline, sinks)?; complete &= role_complete; match role.as_deref() { Some("AXStaticText") => { let (subrole, subrole_complete) = - timed_string(child, "AXSubrole", deadline, stats, usage)?; + timed_string(child, "AXSubrole", deadline, sinks)?; complete &= subrole_complete; if subrole.as_deref() != Some("AXSecureTextField") { - complete &= push_static_text(&mut labels, child, deadline, stats, usage)?; + complete &= push_static_text(&mut labels, child, deadline, sinks)?; } } Some("AXCell") | Some("AXGroup") => { - let (title, title_complete) = - timed_string(child, "AXTitle", deadline, stats, usage)?; + let (title, title_complete) = timed_string(child, "AXTitle", deadline, sinks)?; complete &= title_complete; if let Some(title) = title { labels.push(title); @@ -74,19 +76,21 @@ fn label_from_children( MAX_LABEL_ELEMENTS, deadline, ); - record_child_read(&grandchildren, stats)?; - complete &= grandchildren.complete && !grandchildren.truncated(); + record_child_read(&grandchildren, sinks.stats)?; + complete &= grandchildren.complete + && !grandchildren.truncated() + && !grandchildren.status.invalid_element; for grandchild in grandchildren.elements { let (role, role_complete) = - timed_string(&grandchild, "AXRole", deadline, stats, usage)?; + timed_string(&grandchild, "AXRole", deadline, sinks)?; complete &= role_complete; if role.as_deref() == Some("AXStaticText") { let (subrole, subrole_complete) = - timed_string(&grandchild, "AXSubrole", deadline, stats, usage)?; + timed_string(&grandchild, "AXSubrole", deadline, sinks)?; complete &= subrole_complete; if subrole.as_deref() != Some("AXSecureTextField") { complete &= - push_static_text(&mut labels, &grandchild, deadline, stats, usage)?; + push_static_text(&mut labels, &grandchild, deadline, sinks)?; } } } @@ -94,7 +98,7 @@ fn label_from_children( _ => {} } } - let (label, join_complete) = join_unique_labels(labels, usage); + let (label, join_complete) = join_unique_labels(labels, sinks.usage); Ok((label, complete && join_complete)) } @@ -102,8 +106,7 @@ fn label_from_children( fn label_from_children( _children: &[AXElement], _deadline: std::time::Instant, - _stats: &mut agent_desktop_core::LocatorStats, - _usage: &mut crate::tree::observation_usage::ObservationUsage, + _sinks: &mut NameEvidenceSinks<'_>, ) -> Result<(Option, bool), agent_desktop_core::AdapterError> { Ok((None, true)) } @@ -113,16 +116,19 @@ fn timed_string( element: &AXElement, attribute: &str, deadline: std::time::Instant, - stats: &mut agent_desktop_core::LocatorStats, - usage: &mut crate::tree::observation_usage::ObservationUsage, + sinks: &mut NameEvidenceSinks<'_>, ) -> Result<(Option, bool), agent_desktop_core::AdapterError> { - crate::tree::locator_deadline::prepare(element, deadline)?; - stats.semantic_reads.child_label_reads += 1; - let value = crate::tree::attributes::copy_string_attr_bounded_result( - element, attribute, deadline, usage, - ) - .map_err(|error| crate::tree::query::read_error::semantic_read(error, "child_label.text"))?; - Ok(complete_text(value, stats)) + sinks.stats.semantic_reads.child_label_reads += 1; + let read = crate::tree::attributes::copy_string_attr_bounded_result( + element, + attribute, + deadline, + sinks.usage, + ); + match read { + Ok(value) => Ok(complete_text(value, sinks.stats)), + Err(error) => degrade_transient_read(error, "child_label.text", sinks.stats), + } } #[cfg(target_os = "macos")] @@ -130,18 +136,21 @@ fn push_static_text( labels: &mut Vec, element: &AXElement, deadline: std::time::Instant, - stats: &mut agent_desktop_core::LocatorStats, - usage: &mut crate::tree::observation_usage::ObservationUsage, + sinks: &mut NameEvidenceSinks<'_>, ) -> Result { - crate::tree::locator_deadline::prepare(element, deadline)?; - stats.semantic_reads.child_label_reads += 1; - let value = crate::tree::attributes::copy_value_typed_bounded_result(element, deadline, usage) - .map_err(|error| { - crate::tree::query::read_error::semantic_read(error, "child_label.value") - })?; - let (mut text, mut complete) = complete_text(value, stats); + sinks.stats.semantic_reads.child_label_reads += 1; + let read = + crate::tree::attributes::copy_value_typed_bounded_result(element, deadline, sinks.usage); + let value = match read { + Ok(value) => value, + Err(error) => { + return degrade_transient_read(error, "child_label.value", sinks.stats) + .map(|(_, complete)| complete); + } + }; + let (mut text, mut complete) = complete_text(value, sinks.stats); if text.is_none() && complete { - let (title, title_complete) = timed_string(element, "AXTitle", deadline, stats, usage)?; + let (title, title_complete) = timed_string(element, "AXTitle", deadline, sinks)?; text = title; complete &= title_complete; } @@ -151,6 +160,24 @@ fn push_static_text( Ok(complete) } +#[cfg(target_os = "macos")] +fn degrade_transient_read( + error: i32, + phase: &str, + stats: &mut agent_desktop_core::LocatorStats, +) -> Result<(Option, bool), agent_desktop_core::AdapterError> { + if error == accessibility_sys::kAXErrorAPIDisabled { + return Err(crate::tree::query::read_error::semantic_read(error, phase)); + } + stats.reads.health.cannot_complete += + u64::from(error == accessibility_sys::kAXErrorCannotComplete); + stats.reads.health.native_read_failures += u64::from( + error != accessibility_sys::kAXErrorCannotComplete + && error != accessibility_sys::kAXErrorInvalidUIElement, + ); + Ok((None, false)) +} + fn complete_text( value: Option, stats: &mut agent_desktop_core::LocatorStats, @@ -183,12 +210,6 @@ fn record_child_read( "child_label.children", )); } - if read.status.invalid_element { - return Err(crate::tree::query::read_error::semantic_read( - accessibility_sys::kAXErrorInvalidUIElement, - "child_label.children", - )); - } Ok(()) } @@ -234,95 +255,5 @@ fn join_unique_labels( } #[cfg(test)] -mod tests { - use super::*; - use agent_desktop_core::ObservationBudget; - - fn usage(max_field_bytes: usize) -> crate::tree::observation_usage::ObservationUsage { - crate::tree::observation_usage::ObservationUsage::new(ObservationBudget { - max_field_bytes, - max_text_bytes: max_field_bytes, - ..ObservationBudget::default() - }) - } - - #[test] - fn child_label_cap_is_reported_as_incomplete_traversal() { - let mut stats = agent_desktop_core::LocatorStats::default(); - - note_label_limit(MAX_LABEL_ELEMENTS + 1, &mut stats); - - assert_eq!(stats.traversal.limits.child_label_hits, 1); - } - - #[test] - fn labels_are_normalized_deduplicated_and_joined_in_document_order() { - let mut usage = usage(256); - let labels = vec![ - " Save\n".to_string(), - "Save".to_string(), - " Draft title ".to_string(), - ]; - - assert_eq!( - join_unique_labels(labels, &mut usage), - (Some("Save Draft title".into()), true) - ); - } - - #[test] - fn label_budget_truncation_is_explicit_and_utf8_safe() { - let mut usage = usage(4); - - let (label, complete) = join_unique_labels(["a🙂z".into()], &mut usage); - - assert_eq!(label.as_deref(), Some("a")); - assert!(!complete); - } - - #[test] - fn description_only_name_skips_child_content_fallback() { - let evidence = agent_desktop_core::NameEvidence { - description: Some("scroll-area".into()), - ..Default::default() - }; - - assert!(!should_read_child_label("button", &evidence)); - } - - #[test] - fn unnamed_elements_still_use_bounded_child_content_fallback() { - assert!(should_read_child_label( - "button", - &agent_desktop_core::NameEvidence::default() - )); - } - - #[test] - fn container_roles_never_derive_names_from_children() { - for role in [ - "scrollarea", - "group", - "window", - "list", - "table", - "outline", - "toolbar", - ] { - assert!( - !should_read_child_label(role, &agent_desktop_core::NameEvidence::default()), - "container role {role} must not name itself from descendants" - ); - } - } - - #[test] - fn direct_names_skip_child_label_reads_for_every_role() { - let evidence = agent_desktop_core::NameEvidence { - explicit_label: Some("scroll-area".into()), - ..Default::default() - }; - - assert!(!should_read_child_label("button", &evidence)); - } -} +#[path = "child_labels_tests.rs"] +mod tests; diff --git a/crates/macos/src/tree/child_labels_tests.rs b/crates/macos/src/tree/child_labels_tests.rs new file mode 100644 index 00000000..2ef16fde --- /dev/null +++ b/crates/macos/src/tree/child_labels_tests.rs @@ -0,0 +1,159 @@ +use super::*; +use agent_desktop_core::ObservationBudget; + +fn usage(max_field_bytes: usize) -> crate::tree::observation_usage::ObservationUsage { + crate::tree::observation_usage::ObservationUsage::new(ObservationBudget { + max_field_bytes, + max_text_bytes: max_field_bytes, + ..ObservationBudget::default() + }) +} + +#[test] +fn child_label_cap_is_reported_as_incomplete_traversal() { + let mut stats = agent_desktop_core::LocatorStats::default(); + + note_label_limit(MAX_LABEL_ELEMENTS + 1, &mut stats); + + assert_eq!(stats.traversal.limits.child_label_hits, 1); +} + +#[test] +fn labels_are_normalized_deduplicated_and_joined_in_document_order() { + let mut usage = usage(256); + let labels = vec![ + " Save\n".to_string(), + "Save".to_string(), + " Draft title ".to_string(), + ]; + + assert_eq!( + join_unique_labels(labels, &mut usage), + (Some("Save Draft title".into()), true) + ); +} + +#[test] +fn label_budget_truncation_is_explicit_and_utf8_safe() { + let mut usage = usage(4); + + let (label, complete) = join_unique_labels(["a🙂z".into()], &mut usage); + + assert_eq!(label.as_deref(), Some("a")); + assert!(!complete); +} + +#[test] +fn description_only_name_skips_child_content_fallback() { + let evidence = agent_desktop_core::NameEvidence { + description: Some("scroll-area".into()), + ..Default::default() + }; + + assert!(!should_read_child_label("button", &evidence)); +} + +#[test] +fn unnamed_elements_still_use_bounded_child_content_fallback() { + assert!(should_read_child_label( + "button", + &agent_desktop_core::NameEvidence::default() + )); +} + +#[test] +fn container_roles_never_derive_names_from_children() { + for role in [ + "scrollarea", + "group", + "window", + "list", + "table", + "outline", + "toolbar", + ] { + assert!( + !should_read_child_label(role, &agent_desktop_core::NameEvidence::default()), + "container role {role} must not name itself from descendants" + ); + } +} + +#[test] +fn direct_names_skip_child_label_reads_for_every_role() { + let evidence = agent_desktop_core::NameEvidence { + explicit_label: Some("scroll-area".into()), + ..Default::default() + }; + + assert!(!should_read_child_label("button", &evidence)); +} + +#[cfg(target_os = "macos")] +#[test] +fn transient_child_label_error_degrades_name_to_unknown_and_incomplete() { + let mut stats = agent_desktop_core::LocatorStats::default(); + let mut usage = usage(256); + let children = [AXElement(std::ptr::null_mut())]; + let deadline = std::time::Instant::now() + std::time::Duration::from_millis(250); + + let (evidence, complete) = complete_name_evidence_with_deadline( + &crate::tree::NodeAttrs::default(), + "button", + &children, + deadline, + NameEvidenceSinks { + stats: &mut stats, + usage: &mut usage, + }, + ) + .expect("a transient child-label read error must not abort the traversal"); + + assert_eq!(evidence.child_label, None); + assert!(!complete); +} + +#[cfg(target_os = "macos")] +#[test] +fn api_disabled_child_label_reads_still_fail_closed() { + let mut stats = agent_desktop_core::LocatorStats::default(); + + let error = degrade_transient_read( + accessibility_sys::kAXErrorAPIDisabled, + "child_label.text", + &mut stats, + ) + .expect_err("API disablement must abort the observation"); + + assert_eq!(error.code, agent_desktop_core::ErrorCode::PermDenied); +} + +#[cfg(target_os = "macos")] +#[test] +fn transient_child_label_errors_are_recorded_in_read_health() { + let mut stats = agent_desktop_core::LocatorStats::default(); + + for error in [ + accessibility_sys::kAXErrorInvalidUIElement, + accessibility_sys::kAXErrorCannotComplete, + accessibility_sys::kAXErrorFailure, + ] { + assert_eq!( + degrade_transient_read(error, "child_label.text", &mut stats).unwrap(), + (None, false) + ); + } + + assert_eq!(stats.reads.health.cannot_complete, 1); + assert_eq!(stats.reads.health.native_read_failures, 1); +} + +#[cfg(target_os = "macos")] +#[test] +fn invalid_child_list_degrades_instead_of_aborting() { + let mut stats = agent_desktop_core::LocatorStats::default(); + let mut read = crate::tree::query::child_read::ChildRead::empty(true); + read.status.invalid_element = true; + + assert!(record_child_read(&read, &mut stats).is_ok()); +} diff --git a/crates/macos/src/tree/query/node_read.rs b/crates/macos/src/tree/query/node_read.rs index 78200313..4999dcc3 100644 --- a/crates/macos/src/tree/query/node_read.rs +++ b/crates/macos/src/tree/query/node_read.rs @@ -101,8 +101,10 @@ pub(crate) fn read_node( &role, &child_read.elements, deadline, - stats, - usage, + crate::tree::child_labels::NameEvidenceSinks { + stats: &mut *stats, + usage: &mut *usage, + }, )? } else { (attrs.name_evidence.clone(), true) diff --git a/crates/macos/src/tree/resolve_roots.rs b/crates/macos/src/tree/resolve_roots.rs index 03a323a8..6d5fe5fe 100644 --- a/crates/macos/src/tree/resolve_roots.rs +++ b/crates/macos/src/tree/resolve_roots.rs @@ -122,11 +122,13 @@ fn source_window_scoped_roots( } crate::system::window_resolve::verify_window_identity_until( id, - crate::system::process_identity::to_pid_t(entry.process.pid)?, - entry.source.source_app.as_deref(), - entry.process.process_instance.as_deref(), - entry.source.source_window_title.as_deref(), - entry.source.source_window_bounds_hash, + crate::system::window_resolve::WindowIdentityEvidence { + pid: crate::system::process_identity::to_pid_t(entry.process.pid)?, + app: entry.source.source_app.as_deref(), + process_instance: entry.process.process_instance.as_deref(), + title: entry.source.source_window_title.as_deref(), + bounds_hash: entry.source.source_window_bounds_hash, + }, deadline, )?; } diff --git a/src/cli/contract_tests.rs b/src/cli/contract_tests.rs index 0dd326d3..b0a6dc2e 100644 --- a/src/cli/contract_tests.rs +++ b/src/cli/contract_tests.rs @@ -21,6 +21,7 @@ const NON_COMMAND_MODULES: &[&str] = &[ "wait_latest_ref_cache", "wait_mode", "wait_predicate", + "wait_surface", "wait_test_support", "wait_text_match", "wait_timeout", diff --git a/src/dispatch/keyboard_mouse.rs b/src/dispatch/keyboard_mouse.rs index db8697ef..8918753c 100644 --- a/src/dispatch/keyboard_mouse.rs +++ b/src/dispatch/keyboard_mouse.rs @@ -81,10 +81,14 @@ pub(super) fn drag( ) -> Result { drag_command::execute( drag_command::DragArgs { - from_ref: args.target.from, - from_xy: parse_xy_opt(args.target.from_xy.as_deref())?, - to_ref: args.target.to, - to_xy: parse_xy_opt(args.target.to_xy.as_deref())?, + from: drag_command::DragEndpoint { + ref_id: args.target.from, + xy: parse_xy_opt(args.target.from_xy.as_deref())?, + }, + to: drag_command::DragEndpoint { + ref_id: args.target.to, + xy: parse_xy_opt(args.target.to_xy.as_deref())?, + }, snapshot_id: args.snapshot, duration_ms: args.duration, drop_delay_ms: args.drop_delay, diff --git a/src/dispatch/system.rs b/src/dispatch/system.rs index ccd1cd4f..d3d187d1 100644 --- a/src/dispatch/system.rs +++ b/src/dispatch/system.rs @@ -2,7 +2,7 @@ use agent_desktop_core::{ AppError, PermissionReport, PlatformAdapter, commands::{ permissions as permissions_command, skills as skills_command, status as status_command, - version as version_command, wait as wait_command, + version as version_command, wait as wait_command, wait_surface::SurfaceWait, }, context::CommandContext, }; @@ -29,9 +29,11 @@ pub(super) fn wait( element: args.mode.element, window: args.mode.window, text: args.mode.text, - menu: args.mode.menu, - menu_closed: args.mode.menu_closed, - notification: args.mode.notification, + surface: SurfaceWait::from_flags( + args.mode.menu, + args.mode.menu_closed, + args.mode.notification, + )?, event: args.event.event, window_id: args.event.window_id, }, diff --git a/tests/conformance/ref_action_contract.rs b/tests/conformance/ref_action_contract.rs index 13e94a4e..d1cf9c14 100644 --- a/tests/conformance/ref_action_contract.rs +++ b/tests/conformance/ref_action_contract.rs @@ -47,9 +47,7 @@ pub fn run_wait_element_command_with_predicate( element: Some("@e1".into()), window: None, text: None, - menu: false, - menu_closed: false, - notification: false, + surface: None, event: None, window_id: None, }, diff --git a/tests/e2e/test_electron_metrics.py b/tests/e2e/test_electron_metrics.py index 33560168..60d66f62 100644 --- a/tests/e2e/test_electron_metrics.py +++ b/tests/e2e/test_electron_metrics.py @@ -13,6 +13,7 @@ from electron_metrics import ( ensure_distinct_comparison, order_for, read_trace_events, + run_sample, verify_exact_namespace, ) from electron_metrics_report import build_report, paired_summary, summarize_run @@ -64,6 +65,46 @@ def command_result(data): ) +def timed_command_result(data, wall_ms=12.5, cpu_ms=3.4): + result = command_result(data) + result.wall_ms = wall_ms + result.cpu_ms = cpu_ms + return result + + +def failed_command_result( + termination_error=None, + timed_out=False, + output_limited=False, + stdout="", + stderr="", + returncode=1, + wall_ms=5.0, + cpu_ms=1.0, +): + return types.SimpleNamespace( + termination_error=termination_error, + timed_out=timed_out, + output_limited=output_limited, + stdout=stdout, + stderr=stderr, + returncode=returncode, + wall_ms=wall_ms, + cpu_ms=cpu_ms, + ) + + +def sample_runner(require_stats=False): + return types.SimpleNamespace( + binary="agent-desktop", + label="current", + environment={}, + trace_path="/tmp/agent-desktop-run-sample-test.jsonl", + trace_offset=0, + require_stats=require_stats, + ) + + class MetricsSchemaTests(unittest.TestCase): def test_reports_valid_per_command_metrics_and_reliability_rates(self): failed = sample(30, 5, correct=False) @@ -205,5 +246,87 @@ class MetricsIntegrityTests(unittest.TestCase): ensure_distinct_comparison(runners) +class RunSampleTests(unittest.TestCase): + def test_command_failure_sets_failure_kind_and_error_code(self): + result = failed_command_result(termination_error="killed by signal") + runner = sample_runner(require_stats=True) + + with patch("electron_metrics.run_bounded", return_value=result), \ + patch("electron_metrics.read_trace_events", return_value=([], 0)): + sample = run_sample(runner, arguments(), "w-1") + + self.assertFalse(sample["command_success"]) + self.assertEqual(sample["failure_kind"], "termination_failure") + self.assertEqual(sample["error_code"], "killed by signal") + self.assertFalse(sample["addressable"]) + self.assertFalse(sample["correct"]) + + def test_not_addressable_branch_when_match_is_missing(self): + result = timed_command_result({"snapshot_id": "s-1"}) + runner = sample_runner(require_stats=False) + + with patch("electron_metrics.run_bounded", return_value=result), \ + patch("electron_metrics.read_trace_events", return_value=([], 0)): + sample = run_sample(runner, arguments(), "w-1") + + self.assertTrue(sample["command_success"]) + self.assertFalse(sample["addressable"]) + self.assertEqual(sample["failure_kind"], "not_addressable") + self.assertFalse(sample["correct"]) + + def test_incomplete_traversal_marks_failure_even_when_reresolution_succeeds(self): + data = {"snapshot_id": "s-1", "match": {"ref_id": "@e1", "role": "button"}} + result = timed_command_result(data) + events = [{"event": "locator.resolve", "complete": False, "query_stats": {"nodes": 4}}] + runner = sample_runner(require_stats=True) + + with patch("electron_metrics.run_bounded", return_value=result), \ + patch("electron_metrics.read_trace_events", return_value=(events, 42)), \ + patch("electron_metrics.verify_exact_namespace", return_value=True): + sample = run_sample(runner, arguments(), "w-1") + + self.assertTrue(sample["addressable"]) + self.assertTrue(sample["exact_reresolution"]) + self.assertFalse(sample["correct"]) + self.assertEqual(sample["failure_kind"], "incomplete_traversal") + self.assertEqual(sample["stats"], {"nodes": 4}) + self.assertEqual(runner.trace_offset, 42) + + def test_reresolution_branch_marks_failure_when_namespace_check_fails(self): + data = {"snapshot_id": "s-1", "match": {"ref_id": "@e1", "role": "button"}} + result = timed_command_result(data) + runner = sample_runner(require_stats=False) + + with patch("electron_metrics.run_bounded", return_value=result), \ + patch("electron_metrics.read_trace_events", return_value=([], 0)), \ + patch("electron_metrics.verify_exact_namespace", return_value=False): + sample = run_sample(runner, arguments(), "w-1") + + self.assertTrue(sample["addressable"]) + self.assertFalse(sample["exact_reresolution"]) + self.assertFalse(sample["correct"]) + self.assertEqual(sample["failure_kind"], "reresolution") + + def test_success_path_reports_correct_result_and_strips_identity_fields(self): + data = {"snapshot_id": "s-1", "match": {"ref_id": "@e1", "role": "button"}} + result = timed_command_result(data) + events = [{"event": "locator.resolve", "complete": True, "query_stats": {"nodes": 6}}] + runner = sample_runner(require_stats=True) + + with patch("electron_metrics.run_bounded", return_value=result), \ + patch("electron_metrics.read_trace_events", return_value=(events, 7)), \ + patch("electron_metrics.verify_exact_namespace", return_value=True): + sample = run_sample(runner, arguments(), "w-1") + + self.assertTrue(sample["addressable"]) + self.assertTrue(sample["exact_reresolution"]) + self.assertTrue(sample["correct"]) + self.assertIsNone(sample["failure_kind"]) + self.assertEqual(sample["stats"], {"nodes": 6}) + self.assertNotIn("ref_id", sample) + self.assertNotIn("snapshot_id", sample) + self.assertNotIn("role", sample) + + if __name__ == "__main__": unittest.main()