diff --git a/crates/core/src/action_result.rs b/crates/core/src/action_result.rs index 7d9c1c4..287db09 100644 --- a/crates/core/src/action_result.rs +++ b/crates/core/src/action_result.rs @@ -5,8 +5,6 @@ use serde::{Deserialize, Serialize}; pub struct ActionResult { pub action: String, #[serde(skip_serializing_if = "Option::is_none")] - pub ref_id: Option, - #[serde(skip_serializing_if = "Option::is_none")] pub post_state: Option, #[serde(skip_serializing_if = "Vec::is_empty", default)] pub steps: Vec, @@ -16,17 +14,11 @@ impl ActionResult { pub fn new(action: impl Into) -> Self { Self { action: action.into(), - ref_id: None, post_state: None, steps: Vec::new(), } } - pub fn with_ref(mut self, ref_id: impl Into) -> Self { - self.ref_id = Some(ref_id.into()); - self - } - pub fn with_state(mut self, state: ElementState) -> Self { self.post_state = Some(state); self diff --git a/crates/core/src/action_step.rs b/crates/core/src/action_step.rs index ac23686..7923e44 100644 --- a/crates/core/src/action_step.rs +++ b/crates/core/src/action_step.rs @@ -8,10 +8,6 @@ pub struct ActionStep { } impl ActionStep { - pub fn label(&self) -> &str { - &self.label - } - pub fn attempted(label: &'static str) -> Self { Self { label: label.to_string(), diff --git a/crates/core/src/capability.rs b/crates/core/src/capability.rs index 8515924..a1ad11e 100644 --- a/crates/core/src/capability.rs +++ b/crates/core/src/capability.rs @@ -45,7 +45,17 @@ pub fn for_action(action: &Action) -> &'static [&'static str] { } pub fn defaults_for_role(role: &str) -> Vec { - role_default_slice(role) + let capabilities: &[&str] = match role { + "button" | "link" | "menuitem" | "tab" | "radiobutton" => &[CLICK], + "textfield" | "incrementor" => &[CLICK, SET_VALUE, SET_FOCUS], + "checkbox" => &[CLICK, TOGGLE], + "combobox" => &[CLICK, SELECT], + "treeitem" => &[CLICK, EXPAND, COLLAPSE], + "slider" => &[SET_VALUE], + "cell" => &[CLICK], + _ => &[CLICK], + }; + capabilities .iter() .map(|capability| (*capability).to_string()) .collect() @@ -61,19 +71,6 @@ pub fn contains_any(actions: &[String], capabilities: &[&str]) -> bool { .any(|capability| contains(actions, capability)) } -fn role_default_slice(role: &str) -> &'static [&'static str] { - match role { - "button" | "link" | "menuitem" | "tab" | "radiobutton" => &[CLICK], - "textfield" | "incrementor" => &[CLICK, SET_VALUE, SET_FOCUS], - "checkbox" => &[CLICK, TOGGLE], - "combobox" => &[CLICK, SELECT], - "treeitem" => &[CLICK, EXPAND, COLLAPSE], - "slider" => &[SET_VALUE], - "cell" => &[CLICK], - _ => &[CLICK], - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/crates/core/src/context.rs b/crates/core/src/context.rs index c6f3756..e861f83 100644 --- a/crates/core/src/context.rs +++ b/crates/core/src/context.rs @@ -3,7 +3,7 @@ use crate::{ interaction_policy::InteractionPolicy, trace::TraceConfig, }; use serde_json::Value; -use std::path::{Path, PathBuf}; +use std::path::PathBuf; #[derive(Debug, Clone, Default)] pub struct CommandContext { @@ -87,10 +87,6 @@ impl CommandContext { 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> { @@ -279,18 +275,20 @@ mod tests { #[test] fn batch_item_inherits_or_overrides_session_without_trace_loss() { - let parent = CommandContext::new( - Some("parent".into()), - Some(std::env::temp_dir().join("agent-desktop-context-test.jsonl")), - false, - ) - .unwrap(); + let path = std::env::temp_dir().join("agent-desktop-context-test.jsonl"); + let _ = std::fs::remove_file(&path); + let parent = CommandContext::new(Some("parent".into()), Some(path.clone()), false).unwrap(); let inherited = parent.for_batch_item(None).unwrap(); let overridden = parent.for_batch_item(Some("child".into())).unwrap(); assert_eq!(inherited.session_id(), Some("parent")); assert_eq!(overridden.session_id(), Some("child")); - assert!(overridden.trace_path().is_some()); + overridden + .trace("batch.child", serde_json::json!({ "ok": true })) + .unwrap(); + let body = std::fs::read_to_string(&path).unwrap(); + assert!(body.contains("batch.child")); + let _ = std::fs::remove_file(path); } } diff --git a/crates/core/src/trace.rs b/crates/core/src/trace.rs index 12b977d..3810a9a 100644 --- a/crates/core/src/trace.rs +++ b/crates/core/src/trace.rs @@ -7,7 +7,6 @@ const MAX_TRACE_FILE_BYTES: u64 = 64 * 1024 * 1024; #[derive(Debug, Clone, Default)] pub struct TraceConfig { - path: Option, strict: bool, writer: Option>>, } @@ -32,11 +31,7 @@ impl TraceConfig { }, None => None, }; - Ok(Self { - path, - strict, - writer, - }) + Ok(Self { strict, writer }) } pub fn emit(&self, event: &str, fields: Value) -> Result<(), AppError> { @@ -60,10 +55,6 @@ 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 a5f18ef..4fc4dd4 100644 --- a/crates/ffi/src/actions/resolve.rs +++ b/crates/ffi/src/actions/resolve.rs @@ -61,12 +61,22 @@ pub(crate) unsafe fn core_ref_entry_from_ffi( let name = unsafe { optional_string(entry.name, "name") }?; let value = unsafe { optional_string(entry.value, "value") }?; let description = unsafe { optional_string(entry.description, "description") }?; - let states = unsafe { string_array(entry.states, entry.state_count, &STATES_LIMIT) }?; + let states = unsafe { + string_array( + entry.states, + entry.state_count, + "states", + "AD_MAX_REF_STATES", + crate::types::ref_entry::AD_MAX_REF_STATES, + ) + }?; let available_actions = unsafe { string_array( entry.available_actions, entry.available_action_count, - &ACTIONS_LIMIT, + "available_actions", + "AD_MAX_REF_ACTIONS", + crate::types::ref_entry::AD_MAX_REF_ACTIONS, ) }?; let bounds = if entry.has_bounds { @@ -121,51 +131,23 @@ unsafe fn optional_string( }) } -/// Per-field cap for a counted array crossing the C boundary. `constant` -/// names the header macro so the error message tells callers which -/// published limit they exceeded. -struct ArrayLimit { - field: &'static str, - constant: &'static str, - max: usize, -} - -const STATES_LIMIT: ArrayLimit = ArrayLimit { - field: "states", - constant: "AD_MAX_REF_STATES", - max: crate::types::ref_entry::AD_MAX_REF_STATES, -}; - -const ACTIONS_LIMIT: ArrayLimit = ArrayLimit { - field: "available_actions", - constant: "AD_MAX_REF_ACTIONS", - max: crate::types::ref_entry::AD_MAX_REF_ACTIONS, -}; - -const PATH_LIMIT: ArrayLimit = ArrayLimit { - field: "path", - constant: "AD_MAX_REF_PATH_DEPTH", - max: crate::types::ref_entry::AD_MAX_REF_PATH_DEPTH, -}; - fn check_array_len( len: usize, is_null: bool, - limit: &ArrayLimit, + field: &str, + constant: &str, + max: usize, ) -> Result<(), agent_desktop_core::error::AdapterError> { - if len > limit.max { + if len > max { return Err(agent_desktop_core::error::AdapterError::new( agent_desktop_core::error::ErrorCode::InvalidArgs, - format!( - "{} count {len} exceeds {} ({})", - limit.field, limit.constant, limit.max - ), + format!("{field} count {len} exceeds {constant} ({max})"), )); } if is_null { return Err(agent_desktop_core::error::AdapterError::new( agent_desktop_core::error::ErrorCode::InvalidArgs, - format!("{} count is nonzero but pointer is null", limit.field), + format!("{field} count is nonzero but pointer is null"), )); } Ok(()) @@ -174,18 +156,20 @@ fn check_array_len( unsafe fn string_array( ptr: *const *const std::os::raw::c_char, len: usize, - limit: &ArrayLimit, + field: &str, + constant: &str, + max: usize, ) -> Result, agent_desktop_core::error::AdapterError> { if len == 0 { return Ok(Vec::new()); } - check_array_len(len, ptr.is_null(), limit)?; + check_array_len(len, ptr.is_null(), field, constant, max)?; let items = unsafe { std::slice::from_raw_parts(ptr, len) }; items .iter() .enumerate() .map(|(index, item)| { - let element = format!("{}[{index}]", limit.field); + let element = format!("{field}[{index}]"); unsafe { optional_string(*item, &element) }?.ok_or_else(|| { agent_desktop_core::error::AdapterError::new( agent_desktop_core::error::ErrorCode::InvalidArgs, @@ -203,7 +187,13 @@ unsafe fn ref_path( if len == 0 { return Ok(smallvec::SmallVec::new()); } - check_array_len(len, ptr.is_null(), &PATH_LIMIT)?; + check_array_len( + len, + ptr.is_null(), + "path", + "AD_MAX_REF_PATH_DEPTH", + crate::types::ref_entry::AD_MAX_REF_PATH_DEPTH, + )?; let mut path = smallvec::SmallVec::new(); path.extend( unsafe { std::slice::from_raw_parts(ptr, len) } diff --git a/crates/ffi/src/actions/result.rs b/crates/ffi/src/actions/result.rs index 4fe6f9f..235f0be 100644 --- a/crates/ffi/src/actions/result.rs +++ b/crates/ffi/src/actions/result.rs @@ -7,7 +7,6 @@ const MAX_STATE_STRINGS_TO_FREE: usize = 1024; pub(crate) fn action_result_to_c(r: &CoreActionResult) -> AdActionResult { let action = string_to_c_lossy(&r.action); - let ref_id = opt_string_to_c(r.ref_id.as_deref()); let post_state = match &r.post_state { None => ptr::null_mut(), Some(state) => { @@ -36,7 +35,7 @@ pub(crate) fn action_result_to_c(r: &CoreActionResult) -> AdActionResult { }; AdActionResult { action, - ref_id, + ref_id: ptr::null(), post_state, } } @@ -93,7 +92,6 @@ mod tests { fn test_action_result_to_c_with_state() { let core_result = CoreActionResult { action: "click".to_owned(), - ref_id: Some("@e3".to_owned()), post_state: Some(ElementState { role: "button".to_owned(), states: vec!["focused".to_owned(), "enabled".to_owned()], @@ -104,7 +102,7 @@ mod tests { let c_result = action_result_to_c(&core_result); unsafe { assert_eq!(c_to_string(c_result.action).as_deref(), Some("click")); - assert_eq!(c_to_string(c_result.ref_id).as_deref(), Some("@e3")); + assert!(c_result.ref_id.is_null()); assert!(!c_result.post_state.is_null()); let state = &*c_result.post_state; assert_eq!(c_to_string(state.role).as_deref(), Some("button")); diff --git a/crates/ffi/src/error.rs b/crates/ffi/src/error.rs index 0723c38..242a46f 100644 --- a/crates/ffi/src/error.rs +++ b/crates/ffi/src/error.rs @@ -3,7 +3,6 @@ use std::cell::RefCell; use std::ffi::{CStr, CString, c_char}; const fn error_code_variant_count() -> usize { - let mut count = 0; let variants = [ ErrorCode::PermDenied, ErrorCode::ElementNotFound, @@ -21,12 +20,7 @@ const fn error_code_variant_count() -> usize { ErrorCode::PolicyDenied, ErrorCode::Internal, ]; - let mut i = 0; - while i < variants.len() { - count += 1; - i += 1; - } - count + variants.len() } const fn ad_result_error_variant_count() -> usize { @@ -47,13 +41,7 @@ const fn ad_result_error_variant_count() -> usize { AdResult::ErrSnapshotNotFound, AdResult::ErrPolicyDenied, ]; - let mut count = 0; - let mut i = 0; - while i < variants.len() { - count += 1; - i += 1; - } - count + variants.len() } const _: () = assert!( diff --git a/crates/macos/src/actions/chain.rs b/crates/macos/src/actions/chain.rs index b7dcf5e..07b72cc 100644 --- a/crates/macos/src/actions/chain.rs +++ b/crates/macos/src/actions/chain.rs @@ -27,7 +27,10 @@ mod imp { let deadline = ctx .deadline .unwrap_or_else(|| Instant::now() + chain_timeout()); - let ctx = ctx.with_deadline(deadline); + let ctx = ChainContext { + dynamic_value: ctx.dynamic_value, + deadline: Some(deadline), + }; let total = def.steps.len(); let mut steps = Vec::new(); diff --git a/crates/macos/src/actions/chain_context.rs b/crates/macos/src/actions/chain_context.rs index 695e637..733a41a 100644 --- a/crates/macos/src/actions/chain_context.rs +++ b/crates/macos/src/actions/chain_context.rs @@ -2,36 +2,3 @@ pub(crate) struct ChainContext<'a> { pub(crate) dynamic_value: Option<&'a str>, pub(crate) deadline: Option, } - -impl<'a> ChainContext<'a> { - /// Pins the chain's resolved deadline so every step — notably the - /// `IncrementToDynamic` loop — observes the same budget the chain - /// enforces between steps. Callers construct contexts with - /// `deadline: None`; the chain owns resolving that into an instant. - pub(crate) fn with_deadline(&self, deadline: std::time::Instant) -> ChainContext<'a> { - ChainContext { - dynamic_value: self.dynamic_value, - deadline: Some(deadline), - } - } -} - -#[cfg(test)] -mod tests { - use super::ChainContext; - use std::time::{Duration, Instant}; - - #[test] - fn with_deadline_pins_the_instant_and_keeps_the_dynamic_value() { - let base = ChainContext { - dynamic_value: Some("42"), - deadline: None, - }; - let deadline = Instant::now() + Duration::from_secs(1); - - let effective = base.with_deadline(deadline); - - assert_eq!(effective.dynamic_value, Some("42")); - assert_eq!(effective.deadline, Some(deadline)); - } -} diff --git a/crates/macos/src/actions/chain_disclosure_steps.rs b/crates/macos/src/actions/chain_disclosure_steps.rs index fb42892..82c1319 100644 --- a/crates/macos/src/actions/chain_disclosure_steps.rs +++ b/crates/macos/src/actions/chain_disclosure_steps.rs @@ -24,12 +24,6 @@ mod imp { press_toggle_disclosure(el, false, chain_deadline) } - enum Settle { - Confirmed, - Failed, - DeadlineExpired, - } - /// Tries the semantic action / settable attribute, then a press. Each is /// confirmed against the disclosed state; an action that succeeds at the AX /// layer but does not move the control is not counted. A settle wait that @@ -52,42 +46,25 @@ mod imp { }; if ax_helpers::has_ax_action(el, action) { let _ = ax_helpers::try_ax_action_retried_or_err(el, action)?; - if settled_or_deadline(el, want_expanded, chain_deadline)? { + if disclosure_settled(el, want_expanded, chain_deadline)? { return Ok(true); } } if ax_helpers::is_attr_settable(el, "AXExpanded") { let _ = ax_helpers::set_ax_bool_or_err(el, "AXExpanded", want_expanded)?; - if settled_or_deadline(el, want_expanded, chain_deadline)? { + if disclosure_settled(el, want_expanded, chain_deadline)? { return Ok(true); } } if ax_helpers::has_ax_action(el, "AXPress") && ax_helpers::try_ax_action_retried_or_err(el, "AXPress")? - && settled_or_deadline(el, want_expanded, chain_deadline)? + && disclosure_settled(el, want_expanded, chain_deadline)? { return Ok(true); } Ok(false) } - fn settled_or_deadline( - el: &AXElement, - want_expanded: bool, - chain_deadline: Option, - ) -> Result { - match disclosure_settled(el, want_expanded, chain_deadline) { - Settle::Confirmed => Ok(true), - Settle::Failed => Ok(false), - Settle::DeadlineExpired => { - Err(crate::actions::chain_verify::disclosure_deadline_error( - want_expanded, - disclosed_state(el), - )) - } - } - } - /// Polls for the disclosed state instead of a fixed settle sleep: fast UIs /// confirm on the first read, while animated disclosures get up to the /// settle budget. The budget is capped to the chain's remaining deadline; @@ -98,7 +75,7 @@ mod imp { el: &AXElement, want_expanded: bool, chain_deadline: Option, - ) -> Settle { + ) -> Result { use std::time::{Duration, Instant}; const POLL_INTERVAL: Duration = Duration::from_millis(20); @@ -109,14 +86,17 @@ mod imp { let truncated = deadline < budget_end; loop { if disclosed_state(el) == Some(want_expanded) { - return Settle::Confirmed; + return Ok(true); } let now = Instant::now(); if now >= deadline { return if truncated { - Settle::DeadlineExpired + Err(crate::actions::chain_verify::disclosure_deadline_error( + want_expanded, + disclosed_state(el), + )) } else { - Settle::Failed + Ok(false) }; } std::thread::sleep(POLL_INTERVAL.min(deadline - now)); diff --git a/crates/macos/src/system/force_close.rs b/crates/macos/src/system/force_close.rs index 82bf020..d378222 100644 --- a/crates/macos/src/system/force_close.rs +++ b/crates/macos/src/system/force_close.rs @@ -23,7 +23,7 @@ pub(crate) fn terminate_app(id: &str, pids: &[i32], timeout: Duration) -> Result fn signal_pids(id: &str, pids: &[i32], signal: Signal) -> Result<(), AdapterError> { for &pid in pids { - send_signal(pid, signal).map_err(|detail| { + signal_result(pid, signal).map(|_| ()).map_err(|detail| { AdapterError::new( ErrorCode::ActionFailed, format!("Failed to {} app '{id}' pid {pid}", signal.verb()), @@ -77,10 +77,6 @@ fn child_process_is_running(pid: i32) -> Option { } } -fn send_signal(pid: i32, signal: Signal) -> Result<(), String> { - signal_result(pid, signal).map(|_| ()) -} - fn signal_result(pid: i32, signal: Signal) -> Result { const POSIX_ESRCH: i32 = 3;