diff --git a/crates/core/src/output.rs b/crates/core/src/output.rs index 86d28c63..2f225a95 100644 --- a/crates/core/src/output.rs +++ b/crates/core/src/output.rs @@ -59,6 +59,7 @@ fn retry_token_for_code(code: &ErrorCode) -> Option { Some("snapshot;execute_by_ref".to_owned()) } ErrorCode::PolicyDenied => Some("escalate_policy".to_owned()), + ErrorCode::AppUnresponsive => Some("wait;execute_by_ref".to_owned()), _ => None, } } @@ -100,170 +101,5 @@ impl ErrorPayload { } #[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("native press action 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("native press action failed") - ); - assert_eq!(payload.details, Some(json!({ "check": "visible" }))); - assert_eq!( - payload.retry_command, None, - "ACTION_FAILED must not carry a retry token" - ); - } - - #[test] - fn stale_ref_payload_carries_snapshot_retry_token() { - let err = AppError::stale_ref("@e5"); - let payload = ErrorPayload::from_app_error(&err); - assert_eq!(payload.code, "STALE_REF"); - assert_eq!( - payload.retry_command.as_deref(), - Some("snapshot;execute_by_ref"), - "STALE_REF must carry the canonical retry token" - ); - } - - #[test] - fn snapshot_not_found_payload_carries_snapshot_retry_token() { - let err = AppError::Adapter(AdapterError::snapshot_not_found("snap-abc")); - let payload = ErrorPayload::from_app_error(&err); - assert_eq!(payload.code, "SNAPSHOT_NOT_FOUND"); - assert_eq!( - payload.retry_command.as_deref(), - Some("snapshot;execute_by_ref"), - "SNAPSHOT_NOT_FOUND must carry the canonical retry token" - ); - } - - #[test] - fn policy_denied_payload_carries_escalate_policy_token() { - let err = AppError::Adapter(AdapterError::policy_denied("blocked by policy")); - let payload = ErrorPayload::from_app_error(&err); - assert_eq!(payload.code, "POLICY_DENIED"); - assert_eq!( - payload.retry_command.as_deref(), - Some("escalate_policy"), - "POLICY_DENIED must carry the escalate_policy token, not a snapshot token" - ); - } - - #[test] - fn retry_command_absent_for_non_retryable_errors() { - for err in [ - AppError::Adapter(AdapterError::new(ErrorCode::InvalidArgs, "bad input")), - AppError::Adapter(AdapterError::not_supported("method_x")), - AppError::Adapter(AdapterError::new(ErrorCode::ActionFailed, "failed")), - ] { - let payload = ErrorPayload::from_app_error(&err); - assert!( - payload.retry_command.is_none(), - "non-retryable error {} must not carry a retry token", - payload.code - ); - } - } - - #[test] - fn ok_response_json_shape_has_version_ok_command_data_and_no_error_field() { - let resp = Response::ok("snapshot", json!({"app": "Finder"})); - let map: serde_json::Map = - serde_json::from_value(serde_json::to_value(&resp).expect("serializable")) - .expect("map"); - - assert_eq!(map["version"].as_str(), Some("2.1"), "version must be 2.1"); - assert_eq!(map["ok"].as_bool(), Some(true), "ok must be true"); - assert_eq!( - map["command"].as_str(), - Some("snapshot"), - "command must match" - ); - assert!(map.contains_key("data"), "ok response must have data field"); - assert!( - !map.contains_key("error"), - "ok response must not serialize an error field (skip_serializing_if = is_none)" - ); - } - - #[test] - fn err_response_json_shape_has_version_ok_command_error_and_no_data_field() { - let payload = - ErrorPayload::new("STALE_REF", "ref @e1 is stale").with_suggestion("re-run snapshot"); - let resp = Response::err("click", payload); - let map: serde_json::Map = - serde_json::from_value(serde_json::to_value(&resp).expect("serializable")) - .expect("map"); - - assert_eq!(map["version"].as_str(), Some("2.1"), "version must be 2.1"); - assert_eq!(map["ok"].as_bool(), Some(false), "ok must be false"); - assert_eq!(map["command"].as_str(), Some("click"), "command must match"); - assert!( - !map.contains_key("data"), - "err response must not serialize a data field (skip_serializing_if = is_none)" - ); - assert!( - map.contains_key("error"), - "err response must have error field" - ); - assert_eq!( - map["error"]["code"].as_str(), - Some("STALE_REF"), - "error code must round-trip" - ); - assert_eq!( - map["error"]["message"].as_str(), - Some("ref @e1 is stale"), - "error message must round-trip" - ); - assert_eq!( - map["error"]["suggestion"].as_str(), - Some("re-run snapshot"), - "suggestion must be present when set" - ); - } - - #[test] - fn err_response_omits_optional_error_subfields_when_absent() { - let payload = ErrorPayload::new("INTERNAL", "something broke"); - let resp = Response::err("snapshot", payload); - let map: serde_json::Map = - serde_json::from_value(serde_json::to_value(&resp).expect("serializable")) - .expect("map"); - - let error = map["error"].as_object().expect("error must be an object"); - assert!( - !error.contains_key("suggestion"), - "absent suggestion must be omitted from JSON" - ); - assert!( - !error.contains_key("retry_command"), - "absent retry_command must be omitted from JSON" - ); - assert!( - !error.contains_key("platform_detail"), - "absent platform_detail must be omitted from JSON" - ); - assert!( - !error.contains_key("details"), - "absent details must be omitted from JSON" - ); - } -} +#[path = "output_tests.rs"] +mod tests; diff --git a/crates/core/src/output_tests.rs b/crates/core/src/output_tests.rs new file mode 100644 index 00000000..442f0c5b --- /dev/null +++ b/crates/core/src/output_tests.rs @@ -0,0 +1,174 @@ +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("native press action 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("native press action failed") + ); + assert_eq!(payload.details, Some(json!({ "check": "visible" }))); + assert_eq!( + payload.retry_command, None, + "ACTION_FAILED must not carry a retry token" + ); +} + +#[test] +fn stale_ref_payload_carries_snapshot_retry_token() { + let err = AppError::stale_ref("@e5"); + let payload = ErrorPayload::from_app_error(&err); + assert_eq!(payload.code, "STALE_REF"); + assert_eq!( + payload.retry_command.as_deref(), + Some("snapshot;execute_by_ref"), + "STALE_REF must carry the canonical retry token" + ); +} + +#[test] +fn snapshot_not_found_payload_carries_snapshot_retry_token() { + let err = AppError::Adapter(AdapterError::snapshot_not_found("snap-abc")); + let payload = ErrorPayload::from_app_error(&err); + assert_eq!(payload.code, "SNAPSHOT_NOT_FOUND"); + assert_eq!( + payload.retry_command.as_deref(), + Some("snapshot;execute_by_ref"), + "SNAPSHOT_NOT_FOUND must carry the canonical retry token" + ); +} + +#[test] +fn policy_denied_payload_carries_escalate_policy_token() { + let err = AppError::Adapter(AdapterError::policy_denied("blocked by policy")); + let payload = ErrorPayload::from_app_error(&err); + assert_eq!(payload.code, "POLICY_DENIED"); + assert_eq!( + payload.retry_command.as_deref(), + Some("escalate_policy"), + "POLICY_DENIED must carry the escalate_policy token, not a snapshot token" + ); +} + +#[test] +fn app_unresponsive_payload_carries_a_recovery_token() { + let err = AppError::Adapter(AdapterError::app_unresponsive("Finder")); + let payload = ErrorPayload::from_app_error(&err); + assert_eq!(payload.code, "APP_UNRESPONSIVE"); + assert_eq!( + payload.retry_command.as_deref(), + Some("wait;execute_by_ref"), + "APP_UNRESPONSIVE must carry a sensible recovery token, not silently omit one" + ); +} + +#[test] +fn retry_command_absent_for_non_retryable_errors() { + for err in [ + AppError::Adapter(AdapterError::new(ErrorCode::InvalidArgs, "bad input")), + AppError::Adapter(AdapterError::not_supported("method_x")), + AppError::Adapter(AdapterError::new(ErrorCode::ActionFailed, "failed")), + ] { + let payload = ErrorPayload::from_app_error(&err); + assert!( + payload.retry_command.is_none(), + "non-retryable error {} must not carry a retry token", + payload.code + ); + } +} + +#[test] +fn ok_response_json_shape_has_version_ok_command_data_and_no_error_field() { + let resp = Response::ok("snapshot", json!({"app": "Finder"})); + let map: serde_json::Map = + serde_json::from_value(serde_json::to_value(&resp).expect("serializable")).expect("map"); + + assert_eq!(map["version"].as_str(), Some("2.1"), "version must be 2.1"); + assert_eq!(map["ok"].as_bool(), Some(true), "ok must be true"); + assert_eq!( + map["command"].as_str(), + Some("snapshot"), + "command must match" + ); + assert!(map.contains_key("data"), "ok response must have data field"); + assert!( + !map.contains_key("error"), + "ok response must not serialize an error field (skip_serializing_if = is_none)" + ); +} + +#[test] +fn err_response_json_shape_has_version_ok_command_error_and_no_data_field() { + let payload = + ErrorPayload::new("STALE_REF", "ref @e1 is stale").with_suggestion("re-run snapshot"); + let resp = Response::err("click", payload); + let map: serde_json::Map = + serde_json::from_value(serde_json::to_value(&resp).expect("serializable")).expect("map"); + + assert_eq!(map["version"].as_str(), Some("2.1"), "version must be 2.1"); + assert_eq!(map["ok"].as_bool(), Some(false), "ok must be false"); + assert_eq!(map["command"].as_str(), Some("click"), "command must match"); + assert!( + !map.contains_key("data"), + "err response must not serialize a data field (skip_serializing_if = is_none)" + ); + assert!( + map.contains_key("error"), + "err response must have error field" + ); + assert_eq!( + map["error"]["code"].as_str(), + Some("STALE_REF"), + "error code must round-trip" + ); + assert_eq!( + map["error"]["message"].as_str(), + Some("ref @e1 is stale"), + "error message must round-trip" + ); + assert_eq!( + map["error"]["suggestion"].as_str(), + Some("re-run snapshot"), + "suggestion must be present when set" + ); +} + +#[test] +fn err_response_omits_optional_error_subfields_when_absent() { + let payload = ErrorPayload::new("INTERNAL", "something broke"); + let resp = Response::err("snapshot", payload); + let map: serde_json::Map = + serde_json::from_value(serde_json::to_value(&resp).expect("serializable")).expect("map"); + + let error = map["error"].as_object().expect("error must be an object"); + assert!( + !error.contains_key("suggestion"), + "absent suggestion must be omitted from JSON" + ); + assert!( + !error.contains_key("retry_command"), + "absent retry_command must be omitted from JSON" + ); + assert!( + !error.contains_key("platform_detail"), + "absent platform_detail must be omitted from JSON" + ); + assert!( + !error.contains_key("details"), + "absent details must be omitted from JSON" + ); +} diff --git a/crates/core/src/process_state.rs b/crates/core/src/process_state.rs index 996bcd0c..3d2d631c 100644 --- a/crates/core/src/process_state.rs +++ b/crates/core/src/process_state.rs @@ -1,9 +1,35 @@ use serde::{Deserialize, Serialize}; +/// Liveness/responsiveness classification for a target process, per KTD8. +/// +/// macOS can only emit `Running`, `Exited { code: None }`, and +/// `Unresponsive` — it has no way to read the exit code of a detached +/// process (apps launched via `open -g -a` are not children of this +/// process). `Crashed` stays in the contract for adapters with real crash +/// evidence (e.g. Windows `GetExitCodeProcess`). #[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] -#[serde(rename_all = "snake_case")] +#[serde(tag = "state", rename_all = "snake_case")] pub enum ProcessState { - Responsive, + Running, + Exited { code: Option }, + Crashed { signal_or_code: i32 }, Unresponsive, - Unknown, } + +impl ProcessState { + /// Compact lowercase tag for best-effort `details.process_state` + /// enrichment on terminal errors — a short label, not the full + /// serialized shape (which carries `code`/`signal_or_code` payloads). + pub fn label(&self) -> &'static str { + match self { + ProcessState::Running => "running", + ProcessState::Exited { .. } => "exited", + ProcessState::Crashed { .. } => "crashed", + ProcessState::Unresponsive => "unresponsive", + } + } +} + +#[cfg(test)] +#[path = "process_state_tests.rs"] +mod tests; diff --git a/crates/core/src/process_state_tests.rs b/crates/core/src/process_state_tests.rs new file mode 100644 index 00000000..706b2f40 --- /dev/null +++ b/crates/core/src/process_state_tests.rs @@ -0,0 +1,37 @@ +use super::*; + +#[test] +fn label_reports_short_lowercase_tag_per_variant() { + assert_eq!(ProcessState::Running.label(), "running"); + assert_eq!(ProcessState::Exited { code: None }.label(), "exited"); + assert_eq!(ProcessState::Exited { code: Some(1) }.label(), "exited"); + assert_eq!( + ProcessState::Crashed { signal_or_code: 11 }.label(), + "crashed" + ); + assert_eq!(ProcessState::Unresponsive.label(), "unresponsive"); +} + +#[test] +fn exited_serializes_with_optional_code_field() { + let value = serde_json::to_value(ProcessState::Exited { code: None }).expect("serializable"); + assert_eq!( + value, + serde_json::json!({ "state": "exited", "code": null }) + ); + + let value = serde_json::to_value(ProcessState::Exited { code: Some(9) }).expect("serializable"); + assert_eq!(value, serde_json::json!({ "state": "exited", "code": 9 })); +} + +#[test] +fn running_and_unresponsive_serialize_as_tag_only() { + assert_eq!( + serde_json::to_value(ProcessState::Running).expect("serializable"), + serde_json::json!({ "state": "running" }) + ); + assert_eq!( + serde_json::to_value(ProcessState::Unresponsive).expect("serializable"), + serde_json::json!({ "state": "unresponsive" }) + ); +} diff --git a/crates/core/src/ref_action_wait.rs b/crates/core/src/ref_action_wait.rs index a772cdde..4f85e2d9 100644 --- a/crates/core/src/ref_action_wait.rs +++ b/crates/core/src/ref_action_wait.rs @@ -10,20 +10,44 @@ use crate::{ use serde_json::{Value, json}; use std::time::{Duration, Instant}; -fn ensure_process_responsive( +/// Best-effort enrichment of a terminal (caller-visible) error with process +/// liveness context. Runs exactly once, on the error `execute_with_auto_wait` +/// is about to return — never on an internal auto-wait retry tick — and +/// never converts a success into a failure: it only ever transforms an +/// already-failed `Result`. Only `STALE_REF`/`APP_NOT_FOUND` are enriched +/// (the codes that plausibly indicate a dead or hung target); a probe error +/// (including `PLATFORM_NOT_SUPPORTED` on adapters without this capability) +/// leaves the original error untouched. +fn enrich_with_process_state( adapter: &dyn PlatformAdapter, entry: &RefEntry, -) -> Result<(), AdapterError> { - let state = match adapter.process_state(entry.pid) { - Ok(state) => state, - Err(err) if err.code == ErrorCode::PlatformNotSupported => return Ok(()), - Err(err) => return Err(err), + err: AdapterError, +) -> AdapterError { + if !matches!(err.code, ErrorCode::StaleRef | ErrorCode::AppNotFound) { + return err; + } + let Ok(state) = adapter.process_state(entry.pid) else { + return err; }; if state == crate::process_state::ProcessState::Unresponsive { let app = entry.source_app.as_deref().unwrap_or("target application"); - return Err(AdapterError::app_unresponsive(app)); + return AdapterError::app_unresponsive(app); } - Ok(()) + attach_process_state_detail(err, state) +} + +fn attach_process_state_detail( + err: AdapterError, + state: crate::process_state::ProcessState, +) -> AdapterError { + let mut details = err.details.clone().unwrap_or_else(|| json!({})); + match details.as_object_mut() { + Some(obj) => { + obj.insert("process_state".into(), json!(state.label())); + } + None => details = json!({ "process_state": state.label() }), + } + err.with_details(details) } fn trace_resolve_error(context: &CommandContext, ref_id: &str, err: &AdapterError) { @@ -56,9 +80,9 @@ pub(crate) fn execute_with_auto_wait( ActionRequest, ) -> Result, ) -> Result { - ensure_process_responsive(adapter, entry)?; let Some(budget_ms) = request.timeout_ms else { - return execute_single_shot(adapter, entry, ref_id, context, request, dispatch); + return execute_single_shot(adapter, entry, ref_id, context, request, dispatch) + .map_err(|err| enrich_with_process_state(adapter, entry, err)); }; execute_poll_loop( adapter, @@ -69,6 +93,7 @@ pub(crate) fn execute_with_auto_wait( budget_from_ms(budget_ms), dispatch, ) + .map_err(|err| enrich_with_process_state(adapter, entry, err)) } fn execute_single_shot( @@ -229,3 +254,7 @@ fn actionability_timeout(last_report: Option) -> AdapterError { #[cfg(test)] #[path = "ref_action_wait_tests.rs"] mod tests; + +#[cfg(test)] +#[path = "ref_action_wait_process_state_tests.rs"] +mod process_state_tests; diff --git a/crates/core/src/ref_action_wait_process_state_tests.rs b/crates/core/src/ref_action_wait_process_state_tests.rs new file mode 100644 index 00000000..096f9d5c --- /dev/null +++ b/crates/core/src/ref_action_wait_process_state_tests.rs @@ -0,0 +1,304 @@ +use super::*; +use crate::{ + action::Action, + adapter::{ActionOps, InputOps, NativeHandle, ObservationOps, SystemOps}, + capability, + error::{AdapterError, ErrorCode}, +}; +use std::sync::atomic::{AtomicU32, Ordering}; +use std::time::Duration; + +/// F23: `ensure_process_responsive` used to be an unconditional preflight +/// hard-gate that queried process liveness before every ref action, even +/// ones that would otherwise succeed, and turned a genuinely-Unresponsive +/// classification straight into the terminal error regardless of whether +/// the action itself failed. These tests cover the replacement contract: +/// terminal-only, best-effort enrichment that never touches a success. +fn entry() -> RefEntry { + RefEntry { + pid: 1, + role: "button".into(), + name: Some("Run".into()), + value: None, + description: None, + native_id: None, + states: vec![], + bounds: Some(crate::node::Rect { + x: 0.0, + y: 0.0, + width: 10.0, + height: 10.0, + }), + bounds_hash: Some(1), + available_actions: vec![capability::CLICK.into()], + source_app: None, + source_window_id: None, + source_window_title: None, + source_surface: crate::snapshot_surface::SnapshotSurface::Window, + root_ref: None, + path_is_absolute: false, + path: smallvec::SmallVec::new(), + } +} + +fn request_with_timeout(timeout_ms: u64) -> ActionRequest { + ActionRequest::headless(Action::Click).with_timeout_ms(Some(timeout_ms)) +} + +struct UnresponsiveProcessAdapter { + probe_calls: AtomicU32, +} + +impl ObservationOps for UnresponsiveProcessAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Err(AdapterError::stale_ref("@e1")) + } + + fn resolve_element_strict_with_timeout( + &self, + entry: &RefEntry, + _timeout: Duration, + ) -> Result { + self.resolve_element_strict(entry) + } +} + +impl ActionOps for UnresponsiveProcessAdapter { + fn execute_action( + &self, + _handle: &NativeHandle, + _request: ActionRequest, + ) -> Result { + Ok(crate::action_result::ActionResult::new("click")) + } +} + +impl InputOps for UnresponsiveProcessAdapter {} + +impl SystemOps for UnresponsiveProcessAdapter { + fn process_state(&self, _pid: i32) -> Result { + self.probe_calls.fetch_add(1, Ordering::SeqCst); + Ok(crate::process_state::ProcessState::Unresponsive) + } +} + +#[test] +fn terminal_stale_ref_against_unresponsive_process_surfaces_app_unresponsive() { + let adapter = UnresponsiveProcessAdapter { + probe_calls: AtomicU32::new(0), + }; + + let err = execute_with_auto_wait( + &adapter, + &entry(), + "@e1", + &CommandContext::default(), + ActionRequest::headless(Action::Click), + crate::ref_action::execute_resolved, + ) + .unwrap_err(); + + assert_eq!(err.code, ErrorCode::AppUnresponsive); + assert!( + err.suggestion.is_some(), + "APP_UNRESPONSIVE must carry a recovery suggestion" + ); + assert_eq!( + adapter.probe_calls.load(Ordering::SeqCst), + 1, + "the liveness probe must run exactly once when building the terminal error" + ); +} + +struct ExitedProcessAdapter; + +impl ObservationOps for ExitedProcessAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Err(AdapterError::stale_ref("@e1")) + } + + fn resolve_element_strict_with_timeout( + &self, + entry: &RefEntry, + _timeout: Duration, + ) -> Result { + self.resolve_element_strict(entry) + } +} + +impl ActionOps for ExitedProcessAdapter { + fn execute_action( + &self, + _handle: &NativeHandle, + _request: ActionRequest, + ) -> Result { + Ok(crate::action_result::ActionResult::new("click")) + } +} + +impl InputOps for ExitedProcessAdapter {} + +impl SystemOps for ExitedProcessAdapter { + fn process_state(&self, _pid: i32) -> Result { + Ok(crate::process_state::ProcessState::Exited { code: None }) + } +} + +#[test] +fn terminal_stale_ref_against_exited_process_carries_process_state_detail() { + let err = execute_with_auto_wait( + &ExitedProcessAdapter, + &entry(), + "@e1", + &CommandContext::default(), + ActionRequest::headless(Action::Click), + crate::ref_action::execute_resolved, + ) + .unwrap_err(); + + assert_eq!( + err.code, + ErrorCode::StaleRef, + "an Exited (not Unresponsive) classification must not replace the original error code" + ); + assert_eq!( + err.details.as_ref().and_then(|d| d.get("process_state")), + Some(&serde_json::json!("exited")), + "STALE_REF against a dead pid must carry details.process_state = \"exited\"" + ); +} + +struct SuccessWithUnresponsiveProbeAdapter { + probe_calls: AtomicU32, +} + +impl ObservationOps for SuccessWithUnresponsiveProbeAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn resolve_element_strict_with_timeout( + &self, + entry: &RefEntry, + _timeout: Duration, + ) -> Result { + self.resolve_element_strict(entry) + } +} + +impl ActionOps for SuccessWithUnresponsiveProbeAdapter { + fn execute_action( + &self, + _handle: &NativeHandle, + _request: ActionRequest, + ) -> Result { + Ok(crate::action_result::ActionResult::new("click")) + } +} + +impl InputOps for SuccessWithUnresponsiveProbeAdapter {} + +impl SystemOps for SuccessWithUnresponsiveProbeAdapter { + fn process_state(&self, _pid: i32) -> Result { + self.probe_calls.fetch_add(1, Ordering::SeqCst); + Ok(crate::process_state::ProcessState::Unresponsive) + } +} + +#[test] +fn enrichment_never_converts_a_successful_action_into_a_failure() { + let adapter = SuccessWithUnresponsiveProbeAdapter { + probe_calls: AtomicU32::new(0), + }; + + let result = execute_with_auto_wait( + &adapter, + &entry(), + "@e1", + &CommandContext::default(), + ActionRequest::headless(Action::Click), + crate::ref_action::execute_resolved, + ) + .unwrap(); + + assert_eq!(result.action, "click"); + assert_eq!( + adapter.probe_calls.load(Ordering::SeqCst), + 0, + "a successful action must never consult the liveness probe at all \ + (this used to be an unconditional preflight hard-gate)" + ); +} + +struct TicksThenAppNotFoundAdapter { + resolve_calls: AtomicU32, + probe_calls: AtomicU32, +} + +impl ObservationOps for TicksThenAppNotFoundAdapter { + fn resolve_element_strict(&self, _entry: &RefEntry) -> Result { + Ok(NativeHandle::null()) + } + + fn resolve_element_strict_with_timeout( + &self, + _entry: &RefEntry, + _timeout: Duration, + ) -> Result { + let attempt = self.resolve_calls.fetch_add(1, Ordering::SeqCst) + 1; + if attempt < 3 { + Err(AdapterError::new(ErrorCode::StaleRef, "not yet")) + } else { + Err(AdapterError::new(ErrorCode::AppNotFound, "app gone")) + } + } +} + +impl ActionOps for TicksThenAppNotFoundAdapter { + fn execute_action( + &self, + _handle: &NativeHandle, + _request: ActionRequest, + ) -> Result { + Ok(crate::action_result::ActionResult::new("click")) + } +} + +impl InputOps for TicksThenAppNotFoundAdapter {} + +impl SystemOps for TicksThenAppNotFoundAdapter { + fn process_state(&self, _pid: i32) -> Result { + self.probe_calls.fetch_add(1, Ordering::SeqCst); + Ok(crate::process_state::ProcessState::Running) + } +} + +#[test] +fn probe_call_count_is_independent_of_auto_wait_tick_count() { + let adapter = TicksThenAppNotFoundAdapter { + resolve_calls: AtomicU32::new(0), + probe_calls: AtomicU32::new(0), + }; + + let err = execute_with_auto_wait( + &adapter, + &entry(), + "@e1", + &CommandContext::default(), + request_with_timeout(5_000), + crate::ref_action::execute_resolved, + ) + .unwrap_err(); + + assert_eq!(err.code, ErrorCode::AppNotFound); + assert!( + adapter.resolve_calls.load(Ordering::SeqCst) >= 3, + "the poll loop must have ticked multiple times before the terminal error" + ); + assert_eq!( + adapter.probe_calls.load(Ordering::SeqCst), + 1, + "the liveness probe must run exactly once at the terminal boundary, \ + not once per auto-wait tick" + ); +} diff --git a/crates/macos/src/system/process_state.rs b/crates/macos/src/system/process_state.rs index 0cf03955..2e46a0be 100644 --- a/crates/macos/src/system/process_state.rs +++ b/crates/macos/src/system/process_state.rs @@ -1,26 +1,90 @@ use agent_desktop_core::error::AdapterError; use agent_desktop_core::process_state::ProcessState; +/// Result of one AX responsiveness read, decoupled from the raw AXError so +/// `classify` (below) is testable without a live accessibility tree. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum AxProbeResult { + Responsive, + CannotComplete, +} + +/// Pure classification: alive/dead + a probe closure in, `ProcessState` out. +/// Kept free of any platform API so the retry threshold (one transient +/// `CannotComplete` must not classify `Unresponsive`; two consecutive must) +/// is unit-testable on every host, not just macOS with a live AX tree. +pub(crate) fn classify(pid_alive: bool, mut probe: impl FnMut() -> AxProbeResult) -> ProcessState { + if !pid_alive { + return ProcessState::Exited { code: None }; + } + match probe() { + AxProbeResult::Responsive => ProcessState::Running, + AxProbeResult::CannotComplete => match probe() { + AxProbeResult::Responsive => ProcessState::Running, + AxProbeResult::CannotComplete => ProcessState::Unresponsive, + }, + } +} + #[cfg(target_os = "macos")] pub fn process_state_impl(pid: i32) -> Result { - use crate::tree::{copy_string_attr, element_for_pid}; - use accessibility_sys::kAXRoleAttribute; + use crate::tree::element_for_pid; + + Ok(classify(pid_is_alive(pid), || { + ax_probe(&element_for_pid(pid)) + })) +} + +/// `kill(pid, 0)`-style liveness check: signal 0 sends no actual signal, the +/// kernel only validates the target exists and is reachable. Mirrors the +/// convention already used by `system::force_close::signal_result`. +#[cfg(target_os = "macos")] +fn pid_is_alive(pid: i32) -> bool { + const POSIX_ESRCH: i32 = 3; if pid <= 0 { - return Ok(ProcessState::Unknown); + return false; } - let app = element_for_pid(pid); + unsafe extern "C" { + fn kill(pid: i32, sig: i32) -> i32; + } + if unsafe { kill(pid, 0) } == 0 { + return true; + } + std::io::Error::last_os_error().raw_os_error() != Some(POSIX_ESRCH) +} + +#[cfg(target_os = "macos")] +fn ax_probe(app: &crate::tree::AXElement) -> AxProbeResult { + use accessibility_sys::{ + AXUIElementCopyAttributeValue, kAXErrorCannotComplete, kAXErrorSuccess, kAXRoleAttribute, + }; + use core_foundation::{ + base::{CFType, CFTypeRef, TCFType}, + string::CFString, + }; + if app.0.is_null() { - return Ok(ProcessState::Unknown); + return AxProbeResult::Responsive; } - if copy_string_attr(&app, kAXRoleAttribute).is_some() { - Ok(ProcessState::Responsive) - } else { - Ok(ProcessState::Unresponsive) + let cf_attr = CFString::new(kAXRoleAttribute); + let mut value: CFTypeRef = std::ptr::null_mut(); + let err = + unsafe { AXUIElementCopyAttributeValue(app.0, cf_attr.as_concrete_TypeRef(), &mut value) }; + if err == kAXErrorCannotComplete { + return AxProbeResult::CannotComplete; } + if err == kAXErrorSuccess && !value.is_null() { + unsafe { CFType::wrap_under_create_rule(value) }; + } + AxProbeResult::Responsive } #[cfg(not(target_os = "macos"))] pub fn process_state_impl(_pid: i32) -> Result { Err(AdapterError::not_supported("process_state")) } + +#[cfg(test)] +#[path = "process_state_tests.rs"] +mod tests; diff --git a/crates/macos/src/system/process_state_tests.rs b/crates/macos/src/system/process_state_tests.rs new file mode 100644 index 00000000..f6ffb38f --- /dev/null +++ b/crates/macos/src/system/process_state_tests.rs @@ -0,0 +1,97 @@ +use super::*; +use agent_desktop_core::process_state::ProcessState; +use std::cell::Cell; + +#[test] +fn dead_pid_classifies_exited_without_consulting_probe() { + let probe_calls = Cell::new(0); + let state = classify(false, || { + probe_calls.set(probe_calls.get() + 1); + AxProbeResult::Responsive + }); + assert_eq!(state, ProcessState::Exited { code: None }); + assert_eq!( + probe_calls.get(), + 0, + "a dead pid must short-circuit before probing AX at all" + ); +} + +#[test] +fn single_transient_cannot_complete_does_not_classify_unresponsive() { + let calls = Cell::new(0); + let state = classify(true, || { + let n = calls.get() + 1; + calls.set(n); + if n == 1 { + AxProbeResult::CannotComplete + } else { + AxProbeResult::Responsive + } + }); + assert_eq!( + state, + ProcessState::Running, + "one transient AX blip on a healthy-but-busy app must not hard-fail as Unresponsive" + ); + assert_eq!(calls.get(), 2, "a transient failure retries exactly once"); +} + +#[test] +fn two_consecutive_cannot_complete_classifies_unresponsive() { + let calls = Cell::new(0); + let state = classify(true, || { + calls.set(calls.get() + 1); + AxProbeResult::CannotComplete + }); + assert_eq!(state, ProcessState::Unresponsive); + assert_eq!( + calls.get(), + 2, + "classification requires exactly a second consecutive CannotComplete, not more" + ); +} + +#[test] +fn immediately_responsive_pid_classifies_running_with_one_probe() { + let calls = Cell::new(0); + let state = classify(true, || { + calls.set(calls.get() + 1); + AxProbeResult::Responsive + }); + assert_eq!(state, ProcessState::Running); + assert_eq!(calls.get(), 1); +} + +#[cfg(target_os = "macos")] +#[test] +fn exited_child_process_is_classified_exited_with_no_code() { + let mut child = std::process::Command::new("/bin/echo") + .arg("hi") + .spawn() + .expect("spawn /bin/echo"); + let pid = child.id() as i32; + child.wait().expect("wait for exit"); + for _ in 0..50 { + if !pid_is_alive(pid) { + break; + } + std::thread::sleep(std::time::Duration::from_millis(20)); + } + + let state = process_state_impl(pid).expect("process_state_impl should not error"); + assert_eq!(state, ProcessState::Exited { code: None }); +} + +#[cfg(target_os = "macos")] +#[test] +fn nonpositive_pid_is_never_alive() { + assert!(!pid_is_alive(0)); + assert!(!pid_is_alive(-1)); +} + +#[cfg(target_os = "macos")] +#[test] +fn currently_running_pid_is_alive() { + assert!(pid_is_alive(std::process::id() as i32)); +}