diff --git a/crates/core/src/actionability.rs b/crates/core/src/actionability.rs index 3c99d448..f22b4cb8 100644 --- a/crates/core/src/actionability.rs +++ b/crates/core/src/actionability.rs @@ -2,6 +2,7 @@ use crate::{ action::{Action, ActionRequest}, adapter::{NativeHandle, PlatformAdapter}, error::{AdapterError, ErrorCode}, + node::Rect, refs::RefEntry, }; use serde::Serialize; @@ -84,19 +85,27 @@ fn visibility_check(entry: &RefEntry) -> ActionabilityCheck { let Some(bounds) = entry.bounds else { return unknown("visible", "bounds unavailable"); }; - if bounds.width <= 0.0 || bounds.height <= 0.0 { + if !bounds_are_visible(Some(bounds)) { return fail("visible", "bounds are zero-sized"); } pass("visible") } fn enabled_check(entry: &RefEntry) -> ActionabilityCheck { - if entry.states.iter().any(|state| state == "disabled") { + if !states_are_enabled(&entry.states) { return fail("enabled", "entry state contains disabled"); } pass("enabled") } +pub fn states_are_enabled(states: &[String]) -> bool { + !states.iter().any(|state| state == "disabled") +} + +pub fn bounds_are_visible(bounds: Option) -> bool { + bounds.is_some_and(|bounds| bounds.width > 0.0 && bounds.height > 0.0) +} + fn action_supported_check(entry: &RefEntry, request: &ActionRequest) -> ActionabilityCheck { if request.action.requires_cursor_policy() { return pass("supported_action"); diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 4f661329..11cac1be 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -49,36 +49,34 @@ pub(crate) fn resolve_ref_with_context<'a>( context: &CommandContext, ) -> Result<(RefEntry, ResolvedElement<'a>), AppError> { validate_ref_id(ref_id)?; - let store = RefStore::for_session(context.session_id.as_deref())?; - context.trace( + let store = RefStore::for_session(context.session_id())?; + context.trace_lazy( "ref.resolve.start", - json!({ "ref": ref_id, "snapshot_id": snapshot_id }), + || json!({ "ref": ref_id, "snapshot_id": snapshot_id }), )?; let refmap = store.load(snapshot_id).map_err(|e| { tracing::debug!("refmap load failed: {e}"); - let _ = context.trace( - "ref.resolve.error", + let _ = context.trace_lazy("ref.resolve.error", || { json!({ "ref": ref_id, "snapshot_id": snapshot_id, "code": "STALE_REF", "message": e.to_string() - }), - ); + }) + }); AppError::stale_ref(ref_id) })?; let entry = match refmap.get(ref_id) { Some(entry) => entry.clone(), None => { - context.trace( - "ref.resolve.error", + context.trace_lazy("ref.resolve.error", || { json!({ "ref": ref_id, "snapshot_id": snapshot_id, "code": "STALE_REF", "message": "ref not found in current RefMap" - }), - )?; + }) + })?; return Err(AppError::stale_ref(ref_id)); } }; @@ -89,28 +87,26 @@ pub(crate) fn resolve_ref_with_context<'a>( entry.role, entry.name.as_deref().unwrap_or("(none)") ); - context.trace( - "ref.resolve.entry", + context.trace_lazy("ref.resolve.entry", || { json!({ "ref": ref_id, "pid": entry.pid, "role": entry.role, "name": entry.name - }), - )?; + }) + })?; let handle = adapter.resolve_element_strict(&entry).inspect_err(|err| { - let _ = context.trace( - "ref.resolve.error", + let _ = context.trace_lazy("ref.resolve.error", || { json!({ "ref": ref_id, "snapshot_id": snapshot_id, "code": err.code.as_str(), "message": err.message.clone() - }), - ); + }) + }); })?; tracing::debug!("resolve: {} resolved successfully", ref_id); - context.trace("ref.resolve.ok", json!({ "ref": ref_id }))?; + context.trace_lazy("ref.resolve.ok", || json!({ "ref": ref_id }))?; Ok((entry, ResolvedElement::new(adapter, handle))) } @@ -180,8 +176,8 @@ pub(crate) fn execute_ref_action_result_with_context( &request, context, )?; - let result = crate::ref_action::execute_checked(adapter, handle.handle(), request)?; - context.trace("action.dispatch.ok", json!({ "ref": ref_id }))?; + let result = adapter.execute_action(handle.handle(), request)?; + context.trace_lazy("action.dispatch.ok", || json!({ "ref": ref_id }))?; Ok((entry, result)) } @@ -191,26 +187,25 @@ fn check_actionability_with_trace( request: &ActionRequest, context: &CommandContext, ) -> Result<(), AppError> { - context.trace( + context.trace_lazy( "actionability.check.start", - json!({ "ref": target.ref_id, "action": request.action.name() }), + || json!({ "ref": target.ref_id, "action": request.action.name() }), )?; crate::ref_action::check_resolved(adapter, target.entry, target.handle, request).inspect_err( |err| { - let _ = context.trace( - "actionability.check.error", + let _ = context.trace_lazy("actionability.check.error", || { json!({ "ref": target.ref_id, "action": request.action.name(), "code": err.code.as_str(), "message": err.message.clone() - }), - ); + }) + }); }, )?; - context.trace( + context.trace_lazy( "actionability.check.ok", - json!({ "ref": target.ref_id, "action": request.action.name() }), + || json!({ "ref": target.ref_id, "action": request.action.name() }), )?; Ok(()) } diff --git a/crates/core/src/commands/status.rs b/crates/core/src/commands/status.rs index 37e9a76b..40dd7e3e 100644 --- a/crates/core/src/commands/status.rs +++ b/crates/core/src/commands/status.rs @@ -30,7 +30,7 @@ pub fn execute_with_report_with_context( let permissions = permissions::execute_with_report(PermissionsArgs { request: false }, adapter, report)?; - let store = RefStore::for_session(context.session_id.as_deref()).ok(); + let store = RefStore::for_session(context.session_id()).ok(); let ref_count = store .as_ref() .and_then(|s| s.load_latest().ok()) diff --git a/crates/core/src/commands/wait.rs b/crates/core/src/commands/wait.rs index 6610712e..dcb73393 100644 --- a/crates/core/src/commands/wait.rs +++ b/crates/core/src/commands/wait.rs @@ -90,7 +90,7 @@ fn wait_for_element( ) -> Result { let start = Instant::now(); let timeout = Duration::from_millis(timeout_ms); - let store = RefStore::for_session(context.session_id.as_deref())?; + let store = RefStore::for_session(context.session_id())?; let fixed_refmap = match snapshot_id.as_deref() { Some(id) => Some(store.load_snapshot(id)?), None => None, @@ -231,7 +231,7 @@ fn wait_for_text( .map(|expected| matches.len() == expected) .unwrap_or_else(|| !matches.is_empty()); if matched { - let snapshot_id = RefStore::for_session(context.session_id.as_deref())? + let snapshot_id = RefStore::for_session(context.session_id())? .save_new_snapshot(&result.refmap)?; let elapsed = start.elapsed().as_millis(); let found = matches.first(); diff --git a/crates/core/src/commands/wait_element_tests.rs b/crates/core/src/commands/wait_element_tests.rs index af814d58..beff0934 100644 --- a/crates/core/src/commands/wait_element_tests.rs +++ b/crates/core/src/commands/wait_element_tests.rs @@ -22,16 +22,12 @@ struct PredicateAdapter { } impl PlatformAdapter for PredicateAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Ok(NativeHandle::null()) - } - fn resolve_element_strict_with_timeout( &self, - entry: &RefEntry, + _entry: &RefEntry, _timeout: std::time::Duration, ) -> Result { - self.resolve_element_strict(entry) + Ok(NativeHandle::null()) } fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { @@ -52,16 +48,12 @@ struct FlippingPredicateAdapter { } impl PlatformAdapter for FlippingPredicateAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Ok(NativeHandle::null()) - } - fn resolve_element_strict_with_timeout( &self, - entry: &RefEntry, + _entry: &RefEntry, _timeout: std::time::Duration, ) -> Result { - self.resolve_element_strict(entry) + Ok(NativeHandle::null()) } fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { @@ -79,16 +71,12 @@ struct LiveErrorPredicateAdapter { } impl PlatformAdapter for LiveErrorPredicateAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Ok(NativeHandle::null()) - } - fn resolve_element_strict_with_timeout( &self, - entry: &RefEntry, + _entry: &RefEntry, _timeout: std::time::Duration, ) -> Result { - self.resolve_element_strict(entry) + Ok(NativeHandle::null()) } fn get_live_state(&self, _handle: &NativeHandle) -> Result, AdapterError> { diff --git a/crates/core/src/commands/wait_predicate.rs b/crates/core/src/commands/wait_predicate.rs index 3e1f00f3..52b0304d 100644 --- a/crates/core/src/commands/wait_predicate.rs +++ b/crates/core/src/commands/wait_predicate.rs @@ -1,4 +1,5 @@ use crate::{ + actionability::{bounds_are_visible, states_are_enabled}, adapter::{NativeHandle, PlatformAdapter, optional_live_read}, error::{AdapterError, AppError, ErrorCode}, refs::RefEntry, @@ -97,8 +98,8 @@ fn enabled( adapter: &dyn PlatformAdapter, ) -> Result { let enabled = optional_live_read(adapter.get_live_state(handle))? - .map(|state| !state.states.iter().any(|item| item == "disabled")) - .unwrap_or_else(|| !entry.states.iter().any(|item| item == "disabled")); + .map(|state| states_are_enabled(&state.states)) + .unwrap_or_else(|| states_are_enabled(&entry.states)); Ok(json!({ "enabled": enabled })) } @@ -108,10 +109,7 @@ fn visible( adapter: &dyn PlatformAdapter, ) -> Result { let bounds = optional_live_read(adapter.get_element_bounds(handle))?.or(entry.bounds); - let visible = bounds - .map(|bounds| bounds.width > 0.0 && bounds.height > 0.0) - .unwrap_or(false); - Ok(json!({ "visible": visible })) + Ok(json!({ "visible": bounds_are_visible(bounds) })) } fn actionable( diff --git a/crates/core/src/commands/wait_resolution_tests.rs b/crates/core/src/commands/wait_resolution_tests.rs index 54204e5a..5da82598 100644 --- a/crates/core/src/commands/wait_resolution_tests.rs +++ b/crates/core/src/commands/wait_resolution_tests.rs @@ -12,16 +12,12 @@ use std::time::Duration; struct AmbiguousResolveAdapter; impl PlatformAdapter for AmbiguousResolveAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Err(AdapterError::ambiguous_target("2 candidates matched")) - } - fn resolve_element_strict_with_timeout( &self, - entry: &RefEntry, + _entry: &RefEntry, _timeout: Duration, ) -> Result { - self.resolve_element_strict(entry) + Err(AdapterError::ambiguous_target("2 candidates matched")) } } @@ -30,35 +26,27 @@ struct TransientResolveAdapter { } impl PlatformAdapter for TransientResolveAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + fn resolve_element_strict_with_timeout( + &self, + _entry: &RefEntry, + _timeout: Duration, + ) -> Result { if let Some(code) = self.errors.lock().unwrap().pop() { return Err(AdapterError::new(code, "transient resolution failure")); } Ok(NativeHandle::null()) } - - fn resolve_element_strict_with_timeout( - &self, - entry: &RefEntry, - _timeout: Duration, - ) -> Result { - self.resolve_element_strict(entry) - } } struct PermissionResolveAdapter; impl PlatformAdapter for PermissionResolveAdapter { - fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { - Err(AdapterError::permission_denied()) - } - fn resolve_element_strict_with_timeout( &self, - entry: &RefEntry, + _entry: &RefEntry, _timeout: Duration, ) -> Result { - self.resolve_element_strict(entry) + Err(AdapterError::permission_denied()) } } diff --git a/crates/core/src/context.rs b/crates/core/src/context.rs index 0a2983a9..8284f0dd 100644 --- a/crates/core/src/context.rs +++ b/crates/core/src/context.rs @@ -1,11 +1,11 @@ use crate::{error::AppError, trace::TraceConfig}; use serde_json::Value; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; #[derive(Debug, Clone, Default)] pub struct CommandContext { - pub session_id: Option, - pub trace: TraceConfig, + session_id: Option, + trace: TraceConfig, } impl CommandContext { @@ -37,6 +37,18 @@ impl CommandContext { pub fn trace(&self, event: &str, fields: Value) -> Result<(), AppError> { self.trace.emit(event, fields) } + + pub fn trace_lazy(&self, event: &str, fields: impl FnOnce() -> Value) -> Result<(), AppError> { + self.trace.emit_lazy(event, fields) + } + + pub fn session_id(&self) -> Option<&str> { + self.session_id.as_deref() + } + + pub fn trace_path(&self) -> Option<&Path> { + self.trace.path() + } } pub fn validate_session_id(id: &str) -> Result<(), AppError> { @@ -104,6 +116,21 @@ mod tests { assert!(CommandContext::new(None, Some(missing), true).is_err()); } + #[test] + fn trace_lazy_does_not_build_fields_when_trace_is_disabled() { + let context = CommandContext::default(); + let built = std::cell::Cell::new(false); + + context + .trace_lazy("event", || { + built.set(true); + serde_json::json!({}) + }) + .unwrap(); + + assert!(!built.get()); + } + #[cfg(unix)] #[test] fn trace_file_is_private_on_create() { @@ -202,8 +229,8 @@ mod tests { let inherited = parent.for_batch_item(None).unwrap(); let overridden = parent.for_batch_item(Some("child".into())).unwrap(); - assert_eq!(inherited.session_id.as_deref(), Some("parent")); - assert_eq!(overridden.session_id.as_deref(), Some("child")); - assert!(overridden.trace.path.is_some()); + assert_eq!(inherited.session_id(), Some("parent")); + assert_eq!(overridden.session_id(), Some("child")); + assert!(overridden.trace_path().is_some()); } } diff --git a/crates/core/src/output.rs b/crates/core/src/output.rs index 29faa30f..79218aa4 100644 --- a/crates/core/src/output.rs +++ b/crates/core/src/output.rs @@ -1,6 +1,8 @@ use serde::Serialize; use serde_json::Value; +use crate::error::AppError; + pub const ENVELOPE_VERSION: &str = "2.0"; /// Structured output envelope used by the Phase 3 MCP server transport layer. @@ -76,6 +78,18 @@ impl Response { } impl ErrorPayload { + pub fn from_app_error(err: &AppError) -> Self { + let mut payload = Self::new(err.code(), err.to_string()); + if let Some(suggestion) = err.suggestion() { + payload = payload.with_suggestion(suggestion); + } + if let AppError::Adapter(adapter_error) = err { + payload.platform_detail = adapter_error.platform_detail.clone(); + payload.details = adapter_error.details.clone(); + } + payload + } + pub fn new(code: impl Into, message: impl Into) -> Self { Self { code: code.into(), @@ -97,3 +111,28 @@ impl ErrorPayload { self } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::{AdapterError, ErrorCode}; + use serde_json::json; + + #[test] + fn app_error_payload_preserves_adapter_recovery_fields() { + let err = AppError::Adapter( + AdapterError::new(ErrorCode::ActionFailed, "not actionable") + .with_suggestion("wait and retry") + .with_platform_detail("AXPress failed") + .with_details(json!({ "check": "visible" })), + ); + + let payload = ErrorPayload::from_app_error(&err); + + assert_eq!(payload.code, "ACTION_FAILED"); + assert_eq!(payload.message, "not actionable"); + assert_eq!(payload.suggestion.as_deref(), Some("wait and retry")); + assert_eq!(payload.platform_detail.as_deref(), Some("AXPress failed")); + assert_eq!(payload.details, Some(json!({ "check": "visible" }))); + } +} diff --git a/crates/core/src/ref_action.rs b/crates/core/src/ref_action.rs index efe333de..69f5eec9 100644 --- a/crates/core/src/ref_action.rs +++ b/crates/core/src/ref_action.rs @@ -6,7 +6,7 @@ use crate::{ refs::RefEntry, }; -pub fn check_resolved( +pub(crate) fn check_resolved( adapter: &dyn PlatformAdapter, entry: &RefEntry, handle: &NativeHandle, @@ -15,31 +15,14 @@ pub fn check_resolved( actionability::check_live(entry, handle, adapter, request) } -pub fn execute_checked( - adapter: &dyn PlatformAdapter, - handle: &NativeHandle, - request: ActionRequest, -) -> Result { - adapter.execute_action(handle, request) -} - -pub fn execute_resolved( - adapter: &dyn PlatformAdapter, - entry: &RefEntry, - handle: &NativeHandle, - request: ActionRequest, -) -> Result { - check_resolved(adapter, entry, handle, &request)?; - execute_checked(adapter, handle, request) -} - pub fn execute_entry( adapter: &dyn PlatformAdapter, entry: &RefEntry, request: ActionRequest, ) -> Result { let handle = adapter.resolve_element_strict(entry)?; - let result = execute_resolved(adapter, entry, &handle, request); + let result = check_resolved(adapter, entry, &handle, &request) + .and_then(|_| adapter.execute_action(&handle, request)); let release = adapter.release_handle(&handle); match (result, release) { (Ok(result), Ok(())) => Ok(result), diff --git a/crates/core/src/snapshot.rs b/crates/core/src/snapshot.rs index 7e35bef8..e101994b 100644 --- a/crates/core/src/snapshot.rs +++ b/crates/core/src/snapshot.rs @@ -127,12 +127,12 @@ pub fn run_with_context( context: &CommandContext, ) -> Result { let mut result = build(adapter, opts, app_name, window_id)?; - let store = RefStore::for_session(context.session_id.as_deref())?; + let store = RefStore::for_session(context.session_id())?; let snapshot_id = store.save_new_snapshot(&result.refmap)?; result.snapshot_id = Some(snapshot_id); - context.trace( + context.trace_lazy( "snapshot.saved", - serde_json::json!({ "snapshot_id": result.snapshot_id, "ref_count": result.refmap.len() }), + || serde_json::json!({ "snapshot_id": result.snapshot_id, "ref_count": result.refmap.len() }), )?; Ok(result) } @@ -173,7 +173,7 @@ pub fn append_surface_refs_with_context( ..Default::default() }; let raw_tree = adapter.get_tree(&window, &opts)?; - let store = RefStore::for_session(context.session_id.as_deref())?; + let store = RefStore::for_session(context.session_id())?; let mut refmap = store.load_latest()?; let config = RefAllocConfig { include_bounds: false, diff --git a/crates/core/src/snapshot_ref.rs b/crates/core/src/snapshot_ref.rs index 1a68b2b1..40b5d5a3 100644 --- a/crates/core/src/snapshot_ref.rs +++ b/crates/core/src/snapshot_ref.rs @@ -32,7 +32,7 @@ pub fn run_from_ref_with_context( snapshot_id: Option<&str>, context: &CommandContext, ) -> Result { - let store = RefStore::for_session(context.session_id.as_deref())?; + let store = RefStore::for_session(context.session_id())?; let mut refmap = store.load(snapshot_id)?; let active_snapshot_id = snapshot_id .map(str::to_string) @@ -76,14 +76,13 @@ pub fn run_from_ref_with_context( } else { Some(store.save_new_snapshot(&refmap)?) }; - context.trace( - "snapshot.root.saved", + context.trace_lazy("snapshot.root.saved", || { serde_json::json!({ "root_ref": root_ref_id, "snapshot_id": saved_snapshot_id, "ref_count": refmap.len() - }), - )?; + }) + })?; let window = crate::window_lookup::find_window_for_pid(entry.pid, adapter).unwrap_or(WindowInfo { diff --git a/crates/core/src/trace.rs b/crates/core/src/trace.rs index 242b0ada..53006112 100644 --- a/crates/core/src/trace.rs +++ b/crates/core/src/trace.rs @@ -5,8 +5,8 @@ use std::sync::{Arc, Mutex}; #[derive(Debug, Clone, Default)] pub struct TraceConfig { - pub path: Option, - pub strict: bool, + path: Option, + strict: bool, writer: Option>>, } @@ -38,13 +38,17 @@ impl TraceConfig { } pub fn emit(&self, event: &str, fields: Value) -> Result<(), AppError> { + self.emit_lazy(event, || fields) + } + + pub fn emit_lazy(&self, event: &str, fields: impl FnOnce() -> Value) -> Result<(), AppError> { let Some(writer) = self.writer.as_ref() else { return Ok(()); }; match writer .lock() .map_err(|_| AppError::Internal("trace writer lock poisoned".into())) - .and_then(|mut file| write_event(&mut file, event, fields)) + .and_then(|mut file| write_event(&mut file, event, fields())) { Ok(()) => Ok(()), Err(err) if self.strict => Err(err), @@ -54,6 +58,10 @@ impl TraceConfig { } } } + + pub fn path(&self) -> Option<&Path> { + self.path.as_deref() + } } fn open_trace_file(path: &Path) -> Result { diff --git a/crates/ffi/src/actions/resolve.rs b/crates/ffi/src/actions/resolve.rs index d7b2bb09..b584710f 100644 --- a/crates/ffi/src/actions/resolve.rs +++ b/crates/ffi/src/actions/resolve.rs @@ -1,5 +1,6 @@ use crate::AdAdapter; use crate::convert::string::{c_to_string, try_c_to_string}; +use crate::convert::surface::snapshot_surface_from_c; use crate::error::{self, AdResult}; use crate::ffi_try::trap_panic; use crate::types::{AdNativeHandle, AdRefEntry}; @@ -83,7 +84,7 @@ pub(crate) unsafe fn core_ref_entry_from_ffi( } else { None }; - let source_surface = source_surface_from_c(entry.source_surface)?; + let source_surface = snapshot_surface_from_c(entry.source_surface, "source_surface")?; let path = unsafe { ref_path(entry.path, entry.path_count)? }; Ok(CoreRefEntry { @@ -171,24 +172,6 @@ unsafe fn ref_path( Ok(path) } -fn source_surface_from_c( - raw: i32, -) -> Result { - match raw { - 0 => Ok(agent_desktop_core::adapter::SnapshotSurface::Window), - 1 => Ok(agent_desktop_core::adapter::SnapshotSurface::Focused), - 2 => Ok(agent_desktop_core::adapter::SnapshotSurface::Menu), - 3 => Ok(agent_desktop_core::adapter::SnapshotSurface::Menubar), - 4 => Ok(agent_desktop_core::adapter::SnapshotSurface::Sheet), - 5 => Ok(agent_desktop_core::adapter::SnapshotSurface::Popover), - 6 => Ok(agent_desktop_core::adapter::SnapshotSurface::Alert), - _ => Err(agent_desktop_core::error::AdapterError::new( - agent_desktop_core::error::ErrorCode::InvalidArgs, - "invalid source_surface discriminant", - )), - } -} - #[cfg(test)] #[path = "resolve_tests.rs"] mod tests; diff --git a/crates/ffi/src/convert/surface.rs b/crates/ffi/src/convert/surface.rs index 6fe1855e..a017cc44 100644 --- a/crates/ffi/src/convert/surface.rs +++ b/crates/ffi/src/convert/surface.rs @@ -1,6 +1,10 @@ use crate::convert::string::{free_c_string, opt_string_to_c, string_to_c_lossy}; -use crate::types::AdSurfaceInfo; -use agent_desktop_core::node::SurfaceInfo; +use crate::types::{AdSnapshotSurface, AdSurfaceInfo}; +use agent_desktop_core::{ + adapter::SnapshotSurface, + error::{AdapterError, ErrorCode}, + node::SurfaceInfo, +}; use std::os::raw::c_char; use std::ptr; @@ -12,6 +16,32 @@ pub(crate) fn surface_info_to_c(s: &SurfaceInfo) -> AdSurfaceInfo { } } +pub(crate) fn snapshot_surface_to_core(surface: AdSnapshotSurface) -> SnapshotSurface { + match surface { + AdSnapshotSurface::Window => SnapshotSurface::Window, + AdSnapshotSurface::Focused => SnapshotSurface::Focused, + AdSnapshotSurface::Menu => SnapshotSurface::Menu, + AdSnapshotSurface::Menubar => SnapshotSurface::Menubar, + AdSnapshotSurface::Sheet => SnapshotSurface::Sheet, + AdSnapshotSurface::Popover => SnapshotSurface::Popover, + AdSnapshotSurface::Alert => SnapshotSurface::Alert, + } +} + +pub(crate) fn snapshot_surface_from_c( + raw: i32, + field: &str, +) -> Result { + AdSnapshotSurface::from_c(raw) + .map(snapshot_surface_to_core) + .ok_or_else(|| { + AdapterError::new( + ErrorCode::InvalidArgs, + format!("invalid {field} discriminant"), + ) + }) +} + pub(crate) unsafe fn free_surface_info_fields(s: &mut AdSurfaceInfo) { unsafe { free_c_string(s.kind as *mut c_char); @@ -40,4 +70,16 @@ mod tests { let mut c = c; unsafe { free_surface_info_fields(&mut c) }; } + + #[test] + fn snapshot_surface_from_c_uses_shared_enum_validation() { + assert_eq!( + snapshot_surface_from_c(5, "source_surface").unwrap(), + SnapshotSurface::Popover + ); + + let err = snapshot_surface_from_c(99, "source_surface").unwrap_err(); + assert_eq!(err.code, ErrorCode::InvalidArgs); + assert_eq!(err.message, "invalid source_surface discriminant"); + } } diff --git a/crates/ffi/src/tree/get.rs b/crates/ffi/src/tree/get.rs index 7d57ad7c..f1bbc8cd 100644 --- a/crates/ffi/src/tree/get.rs +++ b/crates/ffi/src/tree/get.rs @@ -1,23 +1,11 @@ use crate::AdAdapter; +use crate::convert::surface::snapshot_surface_from_c; use crate::error::{AdResult, set_last_error}; use crate::ffi_try::trap_panic; use crate::tree::flatten::flatten_tree; -use crate::types::{AdNodeTree, AdSnapshotSurface, AdTreeOptions, AdWindowInfo}; -use agent_desktop_core::adapter::SnapshotSurface; +use crate::types::{AdNodeTree, AdTreeOptions, AdWindowInfo}; use std::ptr; -fn core_surface(s: AdSnapshotSurface) -> SnapshotSurface { - match s { - AdSnapshotSurface::Window => SnapshotSurface::Window, - AdSnapshotSurface::Focused => SnapshotSurface::Focused, - AdSnapshotSurface::Menu => SnapshotSurface::Menu, - AdSnapshotSurface::Menubar => SnapshotSurface::Menubar, - AdSnapshotSurface::Sheet => SnapshotSurface::Sheet, - AdSnapshotSurface::Popover => SnapshotSurface::Popover, - AdSnapshotSurface::Alert => SnapshotSurface::Alert, - } -} - /// Snapshots `win`'s accessibility tree into the flat BFS layout /// described in the types module. The result is written into `*out` /// and must be freed with `ad_free_tree`. Direct children of any node @@ -87,13 +75,10 @@ pub unsafe extern "C" fn ad_get_tree( return crate::error::last_error_code(); } }; - let surface = match AdSnapshotSurface::from_c(opts_ref.surface) { - Some(s) => core_surface(s), - None => { - set_last_error(&agent_desktop_core::error::AdapterError::new( - agent_desktop_core::error::ErrorCode::InvalidArgs, - "invalid snapshot surface discriminant", - )); + let surface = match snapshot_surface_from_c(opts_ref.surface, "snapshot surface") { + Ok(surface) => surface, + Err(e) => { + set_last_error(&e); return AdResult::ErrInvalidArgs; } }; diff --git a/crates/macos/src/adapter.rs b/crates/macos/src/adapter.rs index 60afc7ce..b2e59fde 100644 --- a/crates/macos/src/adapter.rs +++ b/crates/macos/src/adapter.rs @@ -168,12 +168,10 @@ impl PlatformAdapter for MacOSAdapter { fn get_live_value(&self, handle: &NativeHandle) -> Result, AdapterError> { #[cfg(target_os = "macos")] { - use crate::tree::AXElement; - use std::mem::ManuallyDrop; - let el = ManuallyDrop::new(AXElement( - handle.as_raw() as accessibility_sys::AXUIElementRef - )); - Ok(crate::tree::copy_value_typed(&el)) + Ok(with_borrowed_ax_element( + handle, + crate::tree::copy_value_typed, + )) } #[cfg(not(target_os = "macos"))] Err(AdapterError::not_supported("get_live_value")) @@ -182,12 +180,10 @@ impl PlatformAdapter for MacOSAdapter { fn get_live_state(&self, handle: &NativeHandle) -> Result, AdapterError> { #[cfg(target_os = "macos")] { - use crate::tree::AXElement; - use std::mem::ManuallyDrop; - let el = ManuallyDrop::new(AXElement( - handle.as_raw() as accessibility_sys::AXUIElementRef - )); - Ok(Some(crate::actions::post_state::read_element_state(&el))) + Ok(Some(with_borrowed_ax_element( + handle, + crate::actions::post_state::read_element_state, + ))) } #[cfg(not(target_os = "macos"))] Err(AdapterError::not_supported("get_live_state")) @@ -196,16 +192,10 @@ impl PlatformAdapter for MacOSAdapter { fn get_live_actions(&self, handle: &NativeHandle) -> Result>, AdapterError> { #[cfg(target_os = "macos")] { - use crate::tree::AXElement; - use std::mem::ManuallyDrop; - let el = ManuallyDrop::new(AXElement( - handle.as_raw() as accessibility_sys::AXUIElementRef - )); - let state = crate::actions::post_state::read_element_state(&el); - Ok(Some(crate::tree::action_list::platform_available_actions( - &el, - &state.role, - ))) + Ok(Some(with_borrowed_ax_element(handle, |el| { + let state = crate::actions::post_state::read_element_state(el); + crate::tree::action_list::platform_available_actions(el, &state.role) + }))) } #[cfg(not(target_os = "macos"))] Err(AdapterError::not_supported("get_live_actions")) @@ -214,12 +204,10 @@ impl PlatformAdapter for MacOSAdapter { fn get_live_element(&self, handle: &NativeHandle) -> Result { #[cfg(target_os = "macos")] { - use crate::tree::AXElement; - use std::mem::ManuallyDrop; - let el = ManuallyDrop::new(AXElement( - handle.as_raw() as accessibility_sys::AXUIElementRef - )); - Ok(crate::actions::post_state::read_live_element(&el)) + Ok(with_borrowed_ax_element( + handle, + crate::actions::post_state::read_live_element, + )) } #[cfg(not(target_os = "macos"))] Err(AdapterError::not_supported("get_live_element")) @@ -228,12 +216,7 @@ impl PlatformAdapter for MacOSAdapter { fn get_element_bounds(&self, handle: &NativeHandle) -> Result, AdapterError> { #[cfg(target_os = "macos")] { - use crate::tree::AXElement; - use std::mem::ManuallyDrop; - let el = ManuallyDrop::new(AXElement( - handle.as_raw() as accessibility_sys::AXUIElementRef - )); - Ok(crate::tree::read_bounds(&el)) + Ok(with_borrowed_ax_element(handle, crate::tree::read_bounds)) } #[cfg(not(target_os = "macos"))] { @@ -302,29 +285,25 @@ impl PlatformAdapter for MacOSAdapter { handle: &NativeHandle, opts: &TreeOptions, ) -> Result { - use crate::tree::AXElement; - use std::mem::ManuallyDrop; - - let el = ManuallyDrop::new(AXElement( - handle.as_raw() as accessibility_sys::AXUIElementRef - )); - let mut ancestors = FxHashSet::default(); - let context = crate::tree::TreeBuildContext::empty(opts.include_bounds); - crate::tree::build_subtree( - &el, - 0, - 0, - opts.max_depth, - &mut ancestors, - opts.skeleton, - &context, - ) - .ok_or_else(|| { - AdapterError::new( - agent_desktop_core::error::ErrorCode::ElementNotFound, - "Element no longer exists in accessibility tree", + with_borrowed_ax_element(handle, |el| { + let mut ancestors = FxHashSet::default(); + let context = crate::tree::TreeBuildContext::empty(opts.include_bounds); + crate::tree::build_subtree( + el, + 0, + 0, + opts.max_depth, + &mut ancestors, + opts.skeleton, + &context, ) - .with_suggestion("Run 'snapshot' to refresh refs, then retry.") + .ok_or_else(|| { + AdapterError::new( + agent_desktop_core::error::ErrorCode::ElementNotFound, + "Element no longer exists in accessibility tree", + ) + .with_suggestion("Run 'snapshot' to refresh refs, then retry.") + }) }) } } @@ -333,11 +312,18 @@ fn execute_action_impl( handle: &NativeHandle, request: ActionRequest, ) -> Result { - use crate::tree::AXElement; + with_borrowed_ax_element(handle, |el| crate::actions::perform_action(el, &request)) +} + +#[cfg(target_os = "macos")] +fn with_borrowed_ax_element( + handle: &NativeHandle, + f: impl FnOnce(&crate::tree::AXElement) -> T, +) -> T { use std::mem::ManuallyDrop; - let el = ManuallyDrop::new(AXElement( + let el = ManuallyDrop::new(crate::tree::AXElement( handle.as_raw() as accessibility_sys::AXUIElementRef )); - crate::actions::perform_action(&el, &request) + f(&el) } diff --git a/crates/macos/src/tree/action_list.rs b/crates/macos/src/tree/action_list.rs index 13dd8265..a10ce385 100644 --- a/crates/macos/src/tree/action_list.rs +++ b/crates/macos/src/tree/action_list.rs @@ -1,7 +1,7 @@ use super::AXElement; use super::{ capabilities::{copy_action_names, is_attr_settable}, - copy_element_attr, + copy_first_element_attr, }; #[cfg(target_os = "macos")] @@ -68,8 +68,8 @@ fn has_scroll_mechanism(el: &AXElement, role: &str, has: &impl Fn(&str) -> bool) || has("AXScrollLeftByPage") || has("AXScrollRightByPage") || (role_may_own_scrollbars(role) - && (copy_element_attr(el, "AXVerticalScrollBar").is_some() - || copy_element_attr(el, "AXHorizontalScrollBar").is_some())) + && copy_first_element_attr(el, &["AXVerticalScrollBar", "AXHorizontalScrollBar"]) + .is_some()) } fn role_supports_scroll(role: &str) -> bool { diff --git a/crates/macos/src/tree/element.rs b/crates/macos/src/tree/element.rs index 1a09ab24..83720069 100644 --- a/crates/macos/src/tree/element.rs +++ b/crates/macos/src/tree/element.rs @@ -262,6 +262,35 @@ mod imp { ax_value::created_ax_element(value) } + pub fn copy_first_element_attr(el: &AXElement, attrs: &[&str]) -> Option { + if attrs.is_empty() { + return None; + } + let cf_names: Vec = attrs.iter().map(|attr| CFString::new(attr)).collect(); + let cf_refs: Vec<_> = cf_names.iter().map(|s| s.as_concrete_TypeRef()).collect(); + let names_arr = CFArray::from_copyable(&cf_refs); + let mut result_ref: CFTypeRef = std::ptr::null_mut(); + let err = unsafe { + AXUIElementCopyMultipleAttributeValues( + el.0, + names_arr.as_concrete_TypeRef(), + 0, + &mut result_ref as *mut _ as *mut _, + ) + }; + if !result_ref.is_null() { + let arr = created_cf_array(result_ref); + if err == kAXErrorSuccess + && let Some(arr) = arr + { + return arr + .into_iter() + .find_map(|item| ax_value::retained_ax_element(&item)); + } + } + attrs.iter().find_map(|attr| copy_element_attr(el, attr)) + } + pub fn count_children(element: &AXElement, ax_role: Option<&str>) -> u32 { unsafe { for attr_name in child_attributes(ax_role) { @@ -326,6 +355,10 @@ mod imp { None } + pub fn copy_first_element_attr(_el: &AXElement, _attrs: &[&str]) -> Option { + None + } + pub fn count_children(_element: &AXElement, _ax_role: Option<&str>) -> u32 { 0 } @@ -347,7 +380,7 @@ mod imp { } pub use imp::{ - copy_ax_array, copy_ax_array_prefix, copy_bool_attr, copy_element_attr, copy_i64_attr, - copy_string_attr, copy_value_typed, count_children, element_for_pid, fetch_node_attrs, - resolve_element_name, + copy_ax_array, copy_ax_array_prefix, copy_bool_attr, copy_element_attr, + copy_first_element_attr, copy_i64_attr, copy_string_attr, copy_value_typed, count_children, + element_for_pid, fetch_node_attrs, resolve_element_name, }; diff --git a/crates/macos/src/tree/mod.rs b/crates/macos/src/tree/mod.rs index 94039b4c..857f41c9 100644 --- a/crates/macos/src/tree/mod.rs +++ b/crates/macos/src/tree/mod.rs @@ -19,8 +19,8 @@ pub use build_context::TreeBuildContext; pub use builder::{build_subtree, window_element_for}; pub use capabilities::same_element; pub use element::{ - copy_ax_array, copy_bool_attr, copy_element_attr, copy_i64_attr, copy_string_attr, - copy_value_typed, element_for_pid, resolve_element_name, + copy_ax_array, copy_bool_attr, copy_element_attr, copy_first_element_attr, copy_i64_attr, + copy_string_attr, copy_value_typed, element_for_pid, resolve_element_name, }; pub use element_bounds::read_bounds; pub(crate) use node_attrs::NodeAttrs; diff --git a/crates/macos/src/tree/resolve.rs b/crates/macos/src/tree/resolve.rs index 7e827d65..d1834c40 100644 --- a/crates/macos/src/tree/resolve.rs +++ b/crates/macos/src/tree/resolve.rs @@ -43,7 +43,7 @@ pub fn resolve_element_with_timeout( Err(err) if is_retryable_resolution_error(&err) => {} Err(err) => return Err(err), } - if should_retry_scoped_path_resolution(entry) { + if requires_scoped_path_resolution(entry) { if attempt + 1 < attempts { sleep_before_retry(deadline); } @@ -114,11 +114,6 @@ fn requires_scoped_path_resolution(entry: &RefEntry) -> bool { && (entry.source_window_id.is_some() || entry.source_window_title.is_some()) } -#[cfg(target_os = "macos")] -fn should_retry_scoped_path_resolution(entry: &RefEntry) -> bool { - requires_scoped_path_resolution(entry) -} - #[cfg(target_os = "macos")] fn can_use_broad_search(entry: &RefEntry) -> bool { entry.bounds_hash.is_some() || has_meaningful_identity(entry) diff --git a/crates/macos/src/tree/resolve_tests.rs b/crates/macos/src/tree/resolve_tests.rs index f2ccd136..488bb538 100644 --- a/crates/macos/src/tree/resolve_tests.rs +++ b/crates/macos/src/tree/resolve_tests.rs @@ -92,9 +92,9 @@ fn no_bounds_source_window_refs_require_scoped_path_resolution() { fn scoped_path_retry_fails_closed_when_scope_is_unresolved() { let no_bounds_entry = entry(None, Some("w-10"), Some("Freeform"), None); - assert!(should_retry_scoped_path_resolution(&no_bounds_entry)); - assert!(should_retry_scoped_path_resolution(&description_entry())); - assert!(!should_retry_scoped_path_resolution(&entry( + assert!(requires_scoped_path_resolution(&no_bounds_entry)); + assert!(requires_scoped_path_resolution(&description_entry())); + assert!(!requires_scoped_path_resolution(&entry( Some(42), Some("w-10"), Some("Freeform"), @@ -107,7 +107,7 @@ fn scoped_path_retry_fails_closed_for_blank_identity_without_bounds() { let mut blank = entry(None, Some("w-10"), Some("Freeform"), None); blank.name = None; - assert!(should_retry_scoped_path_resolution(&blank)); + assert!(requires_scoped_path_resolution(&blank)); } #[test] diff --git a/src/batch.rs b/src/batch.rs index ac63766c..8231fb93 100644 --- a/src/batch.rs +++ b/src/batch.rs @@ -116,23 +116,12 @@ fn batch_entry(command: &str, result: Result) -> Value { json!({ "version": ENVELOPE_VERSION, "ok": true, "command": command, "data": data }) } Err(err) => { - json!({ "version": ENVELOPE_VERSION, "ok": false, "command": command, "error": error_payload(err) }) + let error = ErrorPayload::from_app_error(&err); + json!({ "version": ENVELOPE_VERSION, "ok": false, "command": command, "error": error }) } } } -fn error_payload(err: AppError) -> ErrorPayload { - let mut payload = ErrorPayload::new(err.code(), err.to_string()); - if let Some(suggestion) = err.suggestion() { - payload = payload.with_suggestion(suggestion); - } - if let AppError::Adapter(adapter_error) = err { - payload.platform_detail = adapter_error.platform_detail; - payload.details = adapter_error.details; - } - payload -} - fn decode(command: &str, args: Value) -> Result where T: DeserializeOwned, diff --git a/src/main.rs b/src/main.rs index 5f931d71..a46c5677 100644 --- a/src/main.rs +++ b/src/main.rs @@ -15,7 +15,6 @@ mod dispatch_parse; use agent_desktop_core::{ adapter::PlatformAdapter, context::CommandContext, - error::AppError, output::{ENVELOPE_VERSION, ErrorPayload, Response}, }; use clap::{CommandFactory, Parser}; @@ -106,15 +105,10 @@ fn finish(cmd_name: &str, result: Result { - let mut payload = ErrorPayload::new(e.code(), e.to_string()); - if let Some(s) = e.suggestion() { - payload = payload.with_suggestion(s); - } - if let AppError::Adapter(adapter_error) = &e { - payload.platform_detail = adapter_error.platform_detail.clone(); - payload.details = adapter_error.details.clone(); - } - emit_response(&Response::err(cmd_name, payload)); + emit_response(&Response::err( + cmd_name, + agent_desktop_core::ErrorPayload::from_app_error(&e), + )); std::process::exit(1); } }