From 771fb122e9463c7dce72f6281e6448ed6dcdaf5c Mon Sep 17 00:00:00 2001 From: Lahfir <70215676+lahfir@users.noreply.github.com> Date: Tue, 14 Jul 2026 17:26:36 -0700 Subject: [PATCH] refactor: simplify foundation contract implementation * fix: make npm staging cleanup recoverable * refactor: share transient resolution classification * refactor: centralize action result finalization * refactor: remove duplicate drag source resolution * refactor: avoid duplicate locator timeout setup * test: consolidate ref and locator fixtures * test: split bounded e2e process support * ci: extract smoke checks and remove destructive cleanup --- .github/workflows/ci.yml | 56 +---- crates/core/src/action_result.rs | 59 ++++- crates/core/src/action_result_tests.rs | 105 +++++++++ crates/core/src/adapter_error.rs | 46 ++++ crates/core/src/commands/drag.rs | 16 +- crates/core/src/commands/drag_tests.rs | 48 ++-- crates/core/src/commands/helpers.rs | 2 +- .../core/src/commands/helpers_test_support.rs | 13 ++ crates/core/src/commands/pointer_action.rs | 19 +- crates/core/src/commands/scroll_tests.rs | 52 +---- crates/core/src/commands/select_tests.rs | 52 +---- crates/core/src/commands/set_value_tests.rs | 52 +---- crates/core/src/commands/type_text_tests.rs | 52 +---- .../resolve_query_hydration_tests.rs | 2 +- .../src/live_locator/resolve_query_tests.rs | 18 +- .../selected_hydration_churn_tests.rs | 18 +- .../selected_hydration_contract_tests.rs | 18 +- .../selected_hydration_subtree_tests.rs | 18 +- .../live_locator/selected_hydration_tests.rs | 18 +- crates/core/src/live_locator/test_support.rs | 28 ++- crates/core/src/ref_action_poll.rs | 12 +- crates/macos/src/actions/dispatch.rs | 115 +--------- crates/macos/src/tree/query/traversal.rs | 1 - npm/scripts/postinstall.js | 18 +- scripts/ci-ffi-helper-discovery-smoke.sh | 18 ++ scripts/ci-npm-wrapper-smoke.sh | 44 ++++ tests/e2e/bounded_process.py | 208 +++++++++++++++++ tests/e2e/json_tool.py | 209 +----------------- tests/npm/postinstall.test.js | 86 ++++++- 29 files changed, 680 insertions(+), 723 deletions(-) create mode 100755 scripts/ci-ffi-helper-discovery-smoke.sh create mode 100755 scripts/ci-npm-wrapper-smoke.sh create mode 100644 tests/e2e/bounded_process.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 9489cf0..ab3af47 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -181,20 +181,7 @@ jobs: run: cargo build --locked --profile release-ffi -p agent-desktop-ffi - name: FFI dylib-adjacent helper discovery smoke - run: | - STAGE=$(mktemp -d /tmp/agent-desktop-ffi-helper.XXXXXX) - trap 'rm -rf "$STAGE"' EXIT - cp target/release-ffi/libagent_desktop_ffi.dylib "$STAGE/" - cp target/release/agent-desktop-macos-helper "$STAGE/" - chmod +x "$STAGE/agent-desktop-macos-helper" - DYLIB_SHA=$(shasum -a 256 "$STAGE/libagent_desktop_ffi.dylib" | awk '{print $1}') - HELPER_SHA=$(shasum -a 256 "$STAGE/agent-desktop-macos-helper" | awk '{print $1}') - AD_DYLIB_PATH="$STAGE/libagent_desktop_ffi.dylib" \ - AD_HEADER_PATH=crates/ffi/include/agent_desktop.h \ - AD_EXPECT_MACOS_HELPER=1 \ - python3 tests/ffi-python/smoke.py - test "$DYLIB_SHA" = "$(shasum -a 256 "$STAGE/libagent_desktop_ffi.dylib" | awk '{print $1}')" - test "$HELPER_SHA" = "$(shasum -a 256 "$STAGE/agent-desktop-macos-helper" | awk '{print $1}')" + run: scripts/ci-ffi-helper-discovery-smoke.sh - uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 with: @@ -206,46 +193,7 @@ jobs: node scripts/check-npm-package.js - name: NPM wrapper smoke - run: | - case "$(uname -s)-$(uname -m)" in - Darwin-arm64) NAME=agent-desktop-darwin-arm64 ;; - Darwin-x86_64) NAME=agent-desktop-darwin-x64 ;; - *) - echo "Unsupported smoke-test platform: $(uname -s)-$(uname -m)" - exit 1 - ;; - esac - HELPER_NAME=agent-desktop-macos-helper - trap 'rm -f "npm/bin/${NAME}" "npm/bin/${HELPER_NAME}"' EXIT - SOURCE=target/release/agent-desktop - HELPER_SOURCE=target/release/agent-desktop-macos-helper - SOURCE_SHA=$(shasum -a 256 "$SOURCE" | awk '{print $1}') - HELPER_SOURCE_SHA=$(shasum -a 256 "$HELPER_SOURCE" | awk '{print $1}') - cp "$SOURCE" "npm/bin/${NAME}" - cp "$HELPER_SOURCE" "npm/bin/${HELPER_NAME}" - chmod +x "npm/bin/${NAME}" - chmod +x "npm/bin/${HELPER_NAME}" - COPY_SHA=$(shasum -a 256 "npm/bin/${NAME}" | awk '{print $1}') - HELPER_COPY_SHA=$(shasum -a 256 "npm/bin/${HELPER_NAME}" | awk '{print $1}') - if [ "$SOURCE_SHA" != "$COPY_SHA" ] || [ "$HELPER_SOURCE_SHA" != "$HELPER_COPY_SHA" ]; then - echo "NPM smoke executable copy failed immutable identity verification" >&2 - exit 1 - fi - node npm/bin/agent-desktop.js version > /tmp/agent-desktop-version.json - node -e " - const out = require('fs').readFileSync('/tmp/agent-desktop-version.json', 'utf8'); - const json = JSON.parse(out); - if (json.ok !== true || !json.data || typeof json.data.version !== 'string') { - throw new Error('agent-desktop npm wrapper did not return version JSON'); - } - " - if [ "$SOURCE_SHA" != "$(shasum -a 256 "$SOURCE" | awk '{print $1}')" ] || \ - [ "$COPY_SHA" != "$(shasum -a 256 "npm/bin/${NAME}" | awk '{print $1}')" ] || \ - [ "$HELPER_SOURCE_SHA" != "$(shasum -a 256 "$HELPER_SOURCE" | awk '{print $1}')" ] || \ - [ "$HELPER_COPY_SHA" != "$(shasum -a 256 "npm/bin/${HELPER_NAME}" | awk '{print $1}')" ]; then - echo "NPM smoke executable changed while it was executed" >&2 - exit 1 - fi + run: scripts/ci-npm-wrapper-smoke.sh ffi-python-smoke: name: FFI Python Smoke diff --git a/crates/core/src/action_result.rs b/crates/core/src/action_result.rs index 15cc9fc..2372271 100644 --- a/crates/core/src/action_result.rs +++ b/crates/core/src/action_result.rs @@ -1,4 +1,7 @@ -use crate::{DeliverySemantics, action_step::ActionStep, element_state::ElementState}; +use crate::{ + Action, AdapterError, DeliverySemantics, ErrorCode, action_step::ActionStep, + action_step_outcome::ActionStepOutcome, element_state::ElementState, +}; use serde::{Deserialize, Deserializer, Serialize}; #[derive(Debug, Clone, Serialize, Deserialize)] @@ -18,6 +21,29 @@ pub struct ActionResult { } impl ActionResult { + pub fn from_execution( + action: &Action, + steps: Vec, + post_state: Option, + ) -> Result { + let label = action.name(); + let (delivered, verified) = delivery_summary(&steps); + if !delivered { + return Ok(Self::satisfied_without_delivery(label).with_steps(steps)); + } + if let Some(state) = post_state.as_ref() { + verify_post_state(action, state)?; + } + let mut result = Self::delivered_unverified(label).with_steps(steps); + if verified { + result = result.with_verified_delivery(); + } + if let Some(state) = post_state { + result = result.with_state(state); + } + Ok(result) + } + pub fn satisfied_without_delivery(action: impl Into) -> Self { Self { action: action.into(), @@ -65,6 +91,37 @@ impl ActionResult { } } +fn delivery_summary(steps: &[ActionStep]) -> (bool, bool) { + let mut delivered = false; + let mut verification = None; + for step in steps { + if matches!(step.outcome, ActionStepOutcome::Succeeded) { + delivered = true; + if let Some(verified) = step.verified() { + verification = Some(verification.unwrap_or(true) && verified); + } + } + } + (delivered, verification.unwrap_or(false)) +} + +fn verify_post_state(action: &Action, state: &ElementState) -> Result<(), AdapterError> { + if matches!(action, Action::Clear) + && state + .value + .as_deref() + .is_some_and(|value| !value.is_empty()) + { + return Err(AdapterError::new( + ErrorCode::ActionFailed, + "Clear reported success but element value is still non-empty", + ) + .with_suggestion("Retry 'clear', or use 'press cmd+a' then 'press delete'.") + .with_disposition(DeliverySemantics::delivered_unverified())); + } + Ok(()) +} + const fn default_action_disposition() -> DeliverySemantics { DeliverySemantics::delivered_unverified() } diff --git a/crates/core/src/action_result_tests.rs b/crates/core/src/action_result_tests.rs index 5d2bf47..272a767 100644 --- a/crates/core/src/action_result_tests.rs +++ b/crates/core/src/action_result_tests.rs @@ -1,5 +1,30 @@ use super::*; +fn state(value: Option<&str>) -> ElementState { + ElementState { + role: "textfield".into(), + states: vec![], + value: value.map(str::to_string), + enabled: None, + hidden: None, + offscreen: None, + } +} + +fn execution_result(verifications: &[Option]) -> ActionResult { + let steps = verifications + .iter() + .map(|verified| { + let step = ActionStep::succeeded("Step"); + match verified { + Some(value) => step.with_verified(*value), + None => step, + } + }) + .collect(); + ActionResult::from_execution(&Action::Click, steps, None).expect("execution result") +} + #[test] fn delivered_unverified_constructor_is_explicit() { assert_eq!( @@ -83,3 +108,83 @@ fn successful_action_deserializes_satisfied_without_delivery() { let result: ActionResult = serde_json::from_value(value).unwrap(); assert_eq!(result.disposition(), DeliverySemantics::not_delivered()); } + +#[test] +fn execution_without_succeeded_steps_is_satisfied_without_delivery() { + let steps = vec![ActionStep::skipped("AlreadyInState").with_verified(true)]; + let result = ActionResult::from_execution(&Action::Check, steps, Some(state(Some("1")))) + .expect("no-op result"); + + assert_eq!(result.disposition(), DeliverySemantics::not_delivered()); + assert_eq!(result.steps.len(), 1); + assert!(result.post_state.is_none()); +} + +#[test] +fn execution_derives_disposition_from_succeeded_step_evidence() { + for (verifications, expected) in [ + (vec![Some(true)], DeliverySemantics::delivered_verified()), + (vec![Some(false)], DeliverySemantics::delivered_unverified()), + ( + vec![Some(true), None], + DeliverySemantics::delivered_verified(), + ), + (vec![None], DeliverySemantics::delivered_unverified()), + ( + vec![Some(true), Some(false)], + DeliverySemantics::delivered_unverified(), + ), + ] { + assert_eq!(execution_result(&verifications).disposition(), expected); + } +} + +#[test] +fn execution_attaches_post_state_without_changing_serialization() { + let steps = vec![ActionStep::succeeded("AXValue").with_verified(false)]; + let post_state = state(Some("done")); + let actual = ActionResult::from_execution( + &Action::SetValue("done".into()), + steps.clone(), + Some(post_state.clone()), + ) + .expect("result"); + let expected = ActionResult::delivered_unverified("set-value") + .with_steps(steps) + .with_state(post_state); + + assert_eq!( + serde_json::to_value(actual).unwrap(), + serde_json::to_value(expected).unwrap() + ); +} + +#[test] +fn clear_postcondition_preserves_exact_error_contract() { + for post_state in [state(Some("")), state(None)] { + ActionResult::from_execution( + &Action::Clear, + vec![ActionStep::succeeded("AXValue").with_verified(true)], + Some(post_state), + ) + .expect("clear result"); + } + + let error = ActionResult::from_execution( + &Action::Clear, + vec![ActionStep::succeeded("AXValue").with_verified(true)], + Some(state(Some("still here"))), + ) + .expect_err("non-empty clear must fail"); + + assert_eq!(error.code, ErrorCode::ActionFailed); + assert_eq!( + error.message, + "Clear reported success but element value is still non-empty" + ); + assert_eq!( + error.suggestion.as_deref(), + Some("Retry 'clear', or use 'press cmd+a' then 'press delete'.") + ); + assert_eq!(error.disposition, DeliverySemantics::delivered_unverified()); +} diff --git a/crates/core/src/adapter_error.rs b/crates/core/src/adapter_error.rs index 8cebbe5..b21ce90 100644 --- a/crates/core/src/adapter_error.rs +++ b/crates/core/src/adapter_error.rs @@ -56,6 +56,16 @@ impl AdapterError { self.retryability == crate::retryability::Retryability::Retryable } + pub(crate) fn is_retryable_resolution_failure(&self) -> bool { + matches!( + self.code, + ErrorCode::StaleRef + | ErrorCode::AmbiguousTarget + | ErrorCode::Timeout + | ErrorCode::AppUnresponsive + ) && self.is_explicitly_retryable() + } + pub fn permits_retry_by_default(&self) -> bool { self.retryability != crate::retryability::Retryability::NonRetryable } @@ -221,4 +231,40 @@ mod tests { assert!(!error.is_explicitly_retryable()); assert!(!error.permits_retry_by_default()); } + + #[test] + fn retryable_resolution_failure_characterizes_every_error_code() { + let all = [ + ErrorCode::PermDenied, + ErrorCode::ElementNotFound, + ErrorCode::AppNotFound, + ErrorCode::ActionFailed, + ErrorCode::ActionNotSupported, + ErrorCode::StaleRef, + ErrorCode::AmbiguousTarget, + ErrorCode::WindowNotFound, + ErrorCode::PlatformNotSupported, + ErrorCode::Timeout, + ErrorCode::InvalidArgs, + ErrorCode::NotificationNotFound, + ErrorCode::SnapshotNotFound, + ErrorCode::PolicyDenied, + ErrorCode::AppUnresponsive, + ErrorCode::Internal, + ]; + let retryable = [ + ErrorCode::StaleRef, + ErrorCode::AmbiguousTarget, + ErrorCode::Timeout, + ErrorCode::AppUnresponsive, + ]; + + for code in all { + let expected = retryable.contains(&code); + let error = AdapterError::new(code.clone(), "failure") + .with_details(serde_json::json!({ "retryable": true })); + assert_eq!(error.is_retryable_resolution_failure(), expected); + assert!(!AdapterError::new(code, "failure").is_retryable_resolution_failure()); + } + } } diff --git a/crates/core/src/commands/drag.rs b/crates/core/src/commands/drag.rs index a1ef25c..ec5bcfb 100644 --- a/crates/core/src/commands/drag.rs +++ b/crates/core/src/commands/drag.rs @@ -44,17 +44,7 @@ pub fn execute( missing_input_message: "Provide --to or --to-xy x,y", headed_requirement: crate::HeadedRequirement::None, }; - let initial_from = resolve_point_with_deadline(from_args, deadline, &lease, adapter, context)?; - let from = resolve_point_with_deadline( - PointResolveArgs { - headed_requirement: crate::HeadedRequirement::None, - ..from_args - }, - deadline, - &lease, - adapter, - context, - )?; + let from = resolve_point_with_deadline(from_args, deadline, &lease, adapter, context)?; let to = resolve_point_with_deadline(to_args, deadline, &lease, adapter, context)?; ensure_point_deadline( deadline, @@ -80,12 +70,12 @@ pub fn execute( if let Some(drop_delay_ms) = args.drop_delay_ms { response["drop_delay_ms"] = json!(drop_delay_ms); } - if initial_from.focused { + if from.focused { response["focused"] = json!(true); } apply_post_action_wait( response, - initial_from.source_entry.as_ref(), + from.source_entry.as_ref(), adapter, context, &lease, diff --git a/crates/core/src/commands/drag_tests.rs b/crates/core/src/commands/drag_tests.rs index a63ef16..1b5ebab 100644 --- a/crates/core/src/commands/drag_tests.rs +++ b/crates/core/src/commands/drag_tests.rs @@ -17,6 +17,7 @@ struct DragCaptureAdapter { captured: Mutex>, focused_pids: Mutex>, resolve_calls: AtomicU32, + focused_bounds: Option, } impl DragCaptureAdapter { @@ -25,8 +26,14 @@ impl DragCaptureAdapter { captured: Mutex::new(None), focused_pids: Mutex::new(Vec::new()), resolve_calls: AtomicU32::new(0), + focused_bounds: None, } } + + fn with_focused_bounds(mut self, bounds: Rect) -> Self { + self.focused_bounds = Some(bounds); + self + } } impl ObservationOps for DragCaptureAdapter { @@ -44,6 +51,11 @@ impl ObservationOps for DragCaptureAdapter { _handle: &NativeHandle, _deadline: crate::Deadline, ) -> Result, AdapterError> { + if !self.focused_pids.lock().unwrap().is_empty() + && let Some(bounds) = self.focused_bounds + { + return Ok(Some(bounds)); + } Ok(Some(Rect { x: 10.0, y: 20.0, @@ -201,31 +213,6 @@ fn cross_app_args(snapshot_id: String) -> DragArgs { } } -#[test] -fn drag_resolves_ref_bounds_to_center_point() { - let _guard = HomeGuard::new(); - let store = RefStore::new().unwrap(); - let mut refmap = RefMap::new(); - refmap.allocate(ref_entry(1)); - let snapshot_id = store.save_new_snapshot(&refmap).unwrap(); - let adapter = DragCaptureAdapter::new(); - - let args = DragArgs { - from_ref: Some("@e1".into()), - from_xy: None, - to_ref: None, - to_xy: Some((100.0, 200.0)), - snapshot_id: Some(snapshot_id), - duration_ms: None, - drop_delay_ms: Some(300), - timeout_ms: None, - }; - execute(args, &adapter, &CommandContext::default().with_headed(true)).unwrap(); - - let captured = adapter.captured.lock().unwrap().clone().unwrap(); - assert_eq!((captured.from.x, captured.from.y), (30.0, 50.0)); -} - #[test] fn headless_ref_drag_is_policy_denied_before_cursor_move() { let _guard = HomeGuard::new(); @@ -248,7 +235,12 @@ fn headless_ref_drag_is_policy_denied_before_cursor_move() { fn headed_ref_drag_focuses_only_the_from_app_once() { let _guard = HomeGuard::new(); let snapshot_id = cross_app_snapshot(); - let adapter = DragCaptureAdapter::new(); + let adapter = DragCaptureAdapter::new().with_focused_bounds(Rect { + x: 100.0, + y: 200.0, + width: 40.0, + height: 60.0, + }); let value = execute( cross_app_args(snapshot_id), @@ -258,7 +250,9 @@ fn headed_ref_drag_focuses_only_the_from_app_once() { .unwrap(); assert_eq!(*adapter.focused_pids.lock().unwrap(), vec![1]); - assert_eq!(adapter.resolve_calls.load(Ordering::SeqCst), 6); + assert_eq!(adapter.resolve_calls.load(Ordering::SeqCst), 4); + let captured = adapter.captured.lock().unwrap().clone().unwrap(); + assert_eq!((captured.from.x, captured.from.y), (120.0, 230.0)); assert_eq!(value["focused"], true); } diff --git a/crates/core/src/commands/helpers.rs b/crates/core/src/commands/helpers.rs index 5bea025..3a65816 100644 --- a/crates/core/src/commands/helpers.rs +++ b/crates/core/src/commands/helpers.rs @@ -331,7 +331,7 @@ pub(crate) fn load_ref_entry( #[cfg(test)] #[path = "helpers_test_support.rs"] -mod test_support; +pub(super) mod test_support; #[cfg(test)] #[path = "helpers_tests.rs"] diff --git a/crates/core/src/commands/helpers_test_support.rs b/crates/core/src/commands/helpers_test_support.rs index 51d7c72..d8c4ba1 100644 --- a/crates/core/src/commands/helpers_test_support.rs +++ b/crates/core/src/commands/helpers_test_support.rs @@ -48,3 +48,16 @@ pub(super) fn text_entry() -> RefEntry { entry.capabilities.available_actions = vec!["SetValue".into()]; entry } + +pub(in crate::commands) fn save_one_ref_snapshot(role: &str, available_action: &str) -> String { + let mut entry = entry(); + entry.identity.role = role.into(); + entry.identity.name = Some("Target".into()); + entry.capabilities.available_actions = vec![available_action.into()]; + let mut refmap = crate::refs::RefMap::new(); + refmap.allocate(entry); + crate::refs_store::RefStore::new() + .unwrap() + .save_new_snapshot(&refmap) + .unwrap() +} diff --git a/crates/core/src/commands/pointer_action.rs b/crates/core/src/commands/pointer_action.rs index ee18f71..4785e30 100644 --- a/crates/core/src/commands/pointer_action.rs +++ b/crates/core/src/commands/pointer_action.rs @@ -223,24 +223,17 @@ fn point_observed_bounds_hash(err: &AppError) -> Option { } fn is_retryable_point_error(err: &AppError) -> bool { - match err.code() { - "STALE_REF" | "AMBIGUOUS_TARGET" | "TIMEOUT" | "APP_UNRESPONSIVE" => { - error_is_explicitly_retryable(err) + match err { + AppError::Adapter(error) if error.is_retryable_resolution_failure() => true, + AppError::Adapter(error) if error.code == crate::ErrorCode::ActionFailed => { + ["visible", "stable", "receives_events"] + .into_iter() + .any(|check| point_failed_check(err, check)) } - "ACTION_FAILED" => ["visible", "stable", "receives_events"] - .into_iter() - .any(|check| point_failed_check(err, check)), _ => false, } } -fn error_is_explicitly_retryable(err: &AppError) -> bool { - let AppError::Adapter(error) = err else { - return false; - }; - error.is_explicitly_retryable() -} - fn point_failed_check(err: &AppError, expected: &str) -> bool { let AppError::Adapter(error) = err else { return false; diff --git a/crates/core/src/commands/scroll_tests.rs b/crates/core/src/commands/scroll_tests.rs index 6b66e9e..616073c 100644 --- a/crates/core/src/commands/scroll_tests.rs +++ b/crates/core/src/commands/scroll_tests.rs @@ -1,13 +1,9 @@ use super::*; use crate::adapter::{ActionOps, InputOps, ObservationOps, SystemOps}; +use crate::commands::helpers::test_support::save_one_ref_snapshot; use crate::{ - AdapterError, Direction, - action_request::ActionRequest, - action_result::ActionResult, - adapter::NativeHandle, - commands::stale_retry_test_support::StaleRetryCounter, - refs::{RefEntry, RefMap}, - refs_store::RefStore, + AdapterError, Direction, action_request::ActionRequest, action_result::ActionResult, + adapter::NativeHandle, commands::stale_retry_test_support::StaleRetryCounter, refs::RefEntry, refs_test_support::HomeGuard, }; @@ -52,47 +48,7 @@ impl SystemOps for StaleThenOkAdapter { } fn snapshot_id() -> String { - let mut refmap = RefMap::new(); - let bounds = crate::Rect { - x: 1.0, - y: 1.0, - width: 20.0, - height: 20.0, - }; - refmap.allocate(RefEntry { - process: crate::RefProcess { - pid: crate::ProcessId::new(1), - process_instance: Some("test-instance".into()), - }, - identity: crate::RefEntryIdentity { - role: "scrollarea".into(), - name: Some("Target".into()), - value: None, - description: None, - native_id: None, - }, - geometry: crate::RefGeometry { - bounds: Some(bounds), - bounds_hash: bounds.bounds_hash(), - }, - capabilities: crate::RefCapabilities { - states: vec![], - available_actions: vec!["Scroll".into()], - }, - source: crate::RefSource { - source_app: None, - source_window_id: None, - source_window_title: None, - source_window_bounds_hash: None, - source_surface: crate::adapter::SnapshotSurface::Window, - }, - scope: crate::RefScope { - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }, - }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() + save_one_ref_snapshot("scrollarea", "Scroll") } /// Regression for the F2 fix: before it, `scroll::execute` always built its diff --git a/crates/core/src/commands/select_tests.rs b/crates/core/src/commands/select_tests.rs index 6b1133f..fc81ff5 100644 --- a/crates/core/src/commands/select_tests.rs +++ b/crates/core/src/commands/select_tests.rs @@ -1,13 +1,9 @@ use super::*; use crate::adapter::{ActionOps, InputOps, ObservationOps, SystemOps}; +use crate::commands::helpers::test_support::save_one_ref_snapshot; use crate::{ - AdapterError, - action_request::ActionRequest, - action_result::ActionResult, - adapter::NativeHandle, - commands::stale_retry_test_support::StaleRetryCounter, - refs::{RefEntry, RefMap}, - refs_store::RefStore, + AdapterError, action_request::ActionRequest, action_result::ActionResult, + adapter::NativeHandle, commands::stale_retry_test_support::StaleRetryCounter, refs::RefEntry, refs_test_support::HomeGuard, }; @@ -52,47 +48,7 @@ impl SystemOps for StaleThenOkAdapter { } fn snapshot_id() -> String { - let mut refmap = RefMap::new(); - let bounds = crate::Rect { - x: 1.0, - y: 1.0, - width: 20.0, - height: 20.0, - }; - refmap.allocate(RefEntry { - process: crate::RefProcess { - pid: crate::ProcessId::new(1), - process_instance: Some("test-instance".into()), - }, - identity: crate::RefEntryIdentity { - role: "combobox".into(), - name: Some("Target".into()), - value: None, - description: None, - native_id: None, - }, - geometry: crate::RefGeometry { - bounds: Some(bounds), - bounds_hash: bounds.bounds_hash(), - }, - capabilities: crate::RefCapabilities { - states: vec![], - available_actions: vec!["Select".into()], - }, - source: crate::RefSource { - source_app: None, - source_window_id: None, - source_window_title: None, - source_window_bounds_hash: None, - source_surface: crate::adapter::SnapshotSurface::Window, - }, - scope: crate::RefScope { - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }, - }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() + save_one_ref_snapshot("combobox", "Select") } /// Regression for the F2 fix: before it, `select::execute` always built its diff --git a/crates/core/src/commands/set_value_tests.rs b/crates/core/src/commands/set_value_tests.rs index dc9f16c..46075c3 100644 --- a/crates/core/src/commands/set_value_tests.rs +++ b/crates/core/src/commands/set_value_tests.rs @@ -1,13 +1,9 @@ use super::*; use crate::adapter::{ActionOps, InputOps, ObservationOps, SystemOps}; +use crate::commands::helpers::test_support::save_one_ref_snapshot; use crate::{ - AdapterError, - action_request::ActionRequest, - action_result::ActionResult, - adapter::NativeHandle, - commands::stale_retry_test_support::StaleRetryCounter, - refs::{RefEntry, RefMap}, - refs_store::RefStore, + AdapterError, action_request::ActionRequest, action_result::ActionResult, + adapter::NativeHandle, commands::stale_retry_test_support::StaleRetryCounter, refs::RefEntry, refs_test_support::HomeGuard, }; @@ -56,47 +52,7 @@ impl SystemOps for StaleThenOkAdapter { } fn snapshot_id() -> String { - let mut refmap = RefMap::new(); - let bounds = crate::Rect { - x: 1.0, - y: 1.0, - width: 20.0, - height: 20.0, - }; - refmap.allocate(RefEntry { - process: crate::RefProcess { - pid: crate::ProcessId::new(1), - process_instance: Some("test-instance".into()), - }, - identity: crate::RefEntryIdentity { - role: "textfield".into(), - name: Some("Target".into()), - value: None, - description: None, - native_id: None, - }, - geometry: crate::RefGeometry { - bounds: Some(bounds), - bounds_hash: bounds.bounds_hash(), - }, - capabilities: crate::RefCapabilities { - states: vec![], - available_actions: vec!["SetValue".into()], - }, - source: crate::RefSource { - source_app: None, - source_window_id: None, - source_window_title: None, - source_window_bounds_hash: None, - source_surface: crate::adapter::SnapshotSurface::Window, - }, - scope: crate::RefScope { - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }, - }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() + save_one_ref_snapshot("textfield", "SetValue") } /// Regression for the F2 fix: before it, `set_value::execute` always built its diff --git a/crates/core/src/commands/type_text_tests.rs b/crates/core/src/commands/type_text_tests.rs index 12990c1..2dd8751 100644 --- a/crates/core/src/commands/type_text_tests.rs +++ b/crates/core/src/commands/type_text_tests.rs @@ -1,13 +1,9 @@ use super::*; use crate::adapter::{ActionOps, InputOps, ObservationOps, SystemOps}; +use crate::commands::helpers::test_support::save_one_ref_snapshot; use crate::{ - AdapterError, - action_request::ActionRequest, - action_result::ActionResult, - adapter::NativeHandle, - commands::stale_retry_test_support::StaleRetryCounter, - refs::{RefEntry, RefMap}, - refs_store::RefStore, + AdapterError, action_request::ActionRequest, action_result::ActionResult, + adapter::NativeHandle, commands::stale_retry_test_support::StaleRetryCounter, refs::RefEntry, refs_test_support::HomeGuard, }; @@ -56,47 +52,7 @@ impl SystemOps for StaleThenOkAdapter { } fn snapshot_id() -> String { - let mut refmap = RefMap::new(); - let bounds = crate::Rect { - x: 1.0, - y: 1.0, - width: 20.0, - height: 20.0, - }; - refmap.allocate(RefEntry { - process: crate::RefProcess { - pid: crate::ProcessId::new(1), - process_instance: Some("test-instance".into()), - }, - identity: crate::RefEntryIdentity { - role: "textfield".into(), - name: Some("Target".into()), - value: None, - description: None, - native_id: None, - }, - geometry: crate::RefGeometry { - bounds: Some(bounds), - bounds_hash: bounds.bounds_hash(), - }, - capabilities: crate::RefCapabilities { - states: vec![], - available_actions: vec!["SetValue".into()], - }, - source: crate::RefSource { - source_app: None, - source_window_id: None, - source_window_title: None, - source_window_bounds_hash: None, - source_surface: crate::adapter::SnapshotSurface::Window, - }, - scope: crate::RefScope { - root_ref: None, - path_is_absolute: false, - path: smallvec::SmallVec::new(), - }, - }); - RefStore::new().unwrap().save_new_snapshot(&refmap).unwrap() + save_one_ref_snapshot("textfield", "SetValue") } /// Regression for the F2 fix: before it, `type_text::execute` always built its diff --git a/crates/core/src/live_locator/resolve_query_hydration_tests.rs b/crates/core/src/live_locator/resolve_query_hydration_tests.rs index 4cf40c9..f998280 100644 --- a/crates/core/src/live_locator/resolve_query_hydration_tests.rs +++ b/crates/core/src/live_locator/resolve_query_hydration_tests.rs @@ -1,4 +1,4 @@ -use super::resolve_query_tests::window; +use super::test_support::window; use super::{ LocatorMaterialization, LocatorResolveRequest, LocatorSelection, ObservationRequest, ObservationRoot, ObservedTree, resolve_query, diff --git a/crates/core/src/live_locator/resolve_query_tests.rs b/crates/core/src/live_locator/resolve_query_tests.rs index ff28a73..d200b96 100644 --- a/crates/core/src/live_locator/resolve_query_tests.rs +++ b/crates/core/src/live_locator/resolve_query_tests.rs @@ -1,9 +1,10 @@ +use super::test_support::window; use super::{ LocatorMaterialization, LocatorResolveRequest, LocatorSelection, LocatorStats, ObservationRequest, ObservationRoot, ObservedTree, resolve_query, }; use crate::{ - AdapterError, AppError, ErrorCode, WindowInfo, + AdapterError, AppError, ErrorCode, adapter::{ActionOps, InputOps, ObservationOps, SystemOps}, locator::{LocatorQuery, StatePredicate}, }; @@ -84,21 +85,6 @@ fn child_label_cap(_: usize) -> ObservedTree { observed_tree(true, stats) } -pub(super) fn window() -> WindowInfo { - WindowInfo { - id: "w-1".into(), - title: "Fixture".into(), - app: "FixtureApp".into(), - pid: crate::ProcessId::new(42), - process_instance: Some("test-instance".into()), - bounds: None, - state: crate::WindowState { - is_focused: true, - ..Default::default() - }, - } -} - #[test] fn resolve_query_validates_before_calling_the_adapter() { let adapter = CountingAdapter { diff --git a/crates/core/src/live_locator/selected_hydration_churn_tests.rs b/crates/core/src/live_locator/selected_hydration_churn_tests.rs index 8a0947a..7e0e977 100644 --- a/crates/core/src/live_locator/selected_hydration_churn_tests.rs +++ b/crates/core/src/live_locator/selected_hydration_churn_tests.rs @@ -1,10 +1,11 @@ +use super::test_support::window; use super::{ IdentifierEvidence, LocatorField, LocatorMaterialization, LocatorResolveRequest, LocatorSelection, ObservationRequest, ObservationRoot, ObservationSource, ObservedTree, resolve_query, }; use crate::{ - AdapterError, ElementIdentifier, IdentifierKind, LocatorQuery, NativeHandle, Rect, WindowInfo, + AdapterError, ElementIdentifier, IdentifierKind, LocatorQuery, NativeHandle, Rect, adapter::{ActionOps, InputOps, ObservationOps, SystemOps}, }; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -279,18 +280,3 @@ fn moved_bounds() -> Rect { ..button_bounds() } } - -fn window() -> WindowInfo { - WindowInfo { - id: "w-1".into(), - title: "Fixture".into(), - app: "FixtureApp".into(), - pid: crate::ProcessId::new(42), - process_instance: Some("test-instance".into()), - bounds: None, - state: crate::WindowState { - is_focused: true, - ..Default::default() - }, - } -} diff --git a/crates/core/src/live_locator/selected_hydration_contract_tests.rs b/crates/core/src/live_locator/selected_hydration_contract_tests.rs index b313f8b..9b5f78f 100644 --- a/crates/core/src/live_locator/selected_hydration_contract_tests.rs +++ b/crates/core/src/live_locator/selected_hydration_contract_tests.rs @@ -1,10 +1,11 @@ +use super::test_support::window; use super::{ IdentifierEvidence, LocatorField, LocatorMaterialization, LocatorResolveRequest, LocatorSelection, ObservationRequest, ObservationRoot, ObservationSource, ObservedTree, resolve_query, }; use crate::{ - AdapterError, AppError, LocatorQuery, NativeHandle, Rect, WindowInfo, + AdapterError, AppError, LocatorQuery, NativeHandle, Rect, adapter::{ActionOps, InputOps, ObservationOps, SystemOps}, }; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -253,18 +254,3 @@ fn bounds() -> Rect { height: 30.0, } } - -fn window() -> WindowInfo { - WindowInfo { - id: "w-1".into(), - title: "Fixture".into(), - app: "FixtureApp".into(), - pid: crate::ProcessId::new(42), - process_instance: Some("test-instance".into()), - bounds: None, - state: crate::WindowState { - is_focused: true, - ..Default::default() - }, - } -} diff --git a/crates/core/src/live_locator/selected_hydration_subtree_tests.rs b/crates/core/src/live_locator/selected_hydration_subtree_tests.rs index f2ec912..b5a63f0 100644 --- a/crates/core/src/live_locator/selected_hydration_subtree_tests.rs +++ b/crates/core/src/live_locator/selected_hydration_subtree_tests.rs @@ -1,10 +1,11 @@ +use super::test_support::window; use super::{ IdentifierEvidence, LocatorField, LocatorMaterialization, LocatorResolveRequest, LocatorSelection, ObservationRequest, ObservationRoot, ObservationSource, ObservedTree, resolve_query, }; use crate::{ - AdapterError, AppError, ContainmentPredicate, LocatorQuery, NativeHandle, Rect, WindowInfo, + AdapterError, AppError, ContainmentPredicate, LocatorQuery, NativeHandle, Rect, adapter::{ActionOps, InputOps, ObservationOps, SystemOps}, }; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -314,18 +315,3 @@ fn root_bounds() -> Rect { height: 200.0, } } - -fn window() -> WindowInfo { - WindowInfo { - id: "w-1".into(), - title: "Fixture".into(), - app: "FixtureApp".into(), - pid: crate::ProcessId::new(42), - process_instance: Some("test-instance".into()), - bounds: None, - state: crate::WindowState { - is_focused: true, - ..Default::default() - }, - } -} diff --git a/crates/core/src/live_locator/selected_hydration_tests.rs b/crates/core/src/live_locator/selected_hydration_tests.rs index 595ac7f..a12120b 100644 --- a/crates/core/src/live_locator/selected_hydration_tests.rs +++ b/crates/core/src/live_locator/selected_hydration_tests.rs @@ -1,10 +1,11 @@ +use super::test_support::window; use super::{ IdentifierEvidence, LocatorEvidence, LocatorField, LocatorMaterialization, LocatorRefEvidence, LocatorResolveRequest, LocatorSelection, LocatorStats, ObservationRequest, ObservationRoot, ObservationSource, ObservedTree, resolve_query, }; use crate::{ - AdapterError, ElementIdentifier, IdentifierKind, LocatorQuery, NativeHandle, Rect, WindowInfo, + AdapterError, ElementIdentifier, IdentifierKind, LocatorQuery, NativeHandle, Rect, adapter::{ActionOps, InputOps, ObservationOps, SystemOps}, }; use std::{ @@ -252,18 +253,3 @@ fn button_bounds() -> Rect { height: 30.0, } } - -fn window() -> WindowInfo { - WindowInfo { - id: "w-1".into(), - title: "Fixture".into(), - app: "FixtureApp".into(), - pid: crate::ProcessId::new(42), - process_instance: Some("test-instance".into()), - bounds: None, - state: crate::WindowState { - is_focused: true, - ..Default::default() - }, - } -} diff --git a/crates/core/src/live_locator/test_support.rs b/crates/core/src/live_locator/test_support.rs index 070b0fc..bc90ea5 100644 --- a/crates/core/src/live_locator/test_support.rs +++ b/crates/core/src/live_locator/test_support.rs @@ -4,6 +4,21 @@ use super::{ }; use crate::{WindowInfo, refs::RefPath}; +pub(super) fn window() -> WindowInfo { + WindowInfo { + id: "w-1".into(), + title: "Fixture".into(), + app: "FixtureApp".into(), + pid: crate::ProcessId::new(42), + process_instance: Some("test-instance".into()), + bounds: None, + state: crate::WindowState { + is_focused: true, + ..Default::default() + }, + } +} + pub(crate) fn evidence(role: &str, name: Option<&str>) -> LocatorEvidence { LocatorEvidence { role: LocatorField::Known(role.to_string()), @@ -46,18 +61,7 @@ pub(crate) fn tree( ObservedTree { nodes, roots, - source: ObservationSource::Window(WindowInfo { - id: "w-1".into(), - title: "Fixture".into(), - app: "FixtureApp".into(), - pid: crate::ProcessId::new(42), - process_instance: Some("test-instance".into()), - bounds: None, - state: crate::WindowState { - is_focused: true, - ..Default::default() - }, - }), + source: ObservationSource::Window(window()), stats: LocatorStats::default(), structurally_complete, } diff --git a/crates/core/src/ref_action_poll.rs b/crates/core/src/ref_action_poll.rs index ea0d7f6..3be977c 100644 --- a/crates/core/src/ref_action_poll.rs +++ b/crates/core/src/ref_action_poll.rs @@ -82,7 +82,7 @@ fn handle_resolve_failure( error: AdapterError, deadline: Deadline, ) -> Result<(), AdapterError> { - if is_permanent_error(&error.code) || !is_retryable_resolve_error(&error) { + if !error.is_retryable_resolution_failure() { trace_resolve_error(context.context, context.ref_id, &error); return Err(error); } @@ -125,16 +125,6 @@ fn is_permanent_actionability_error(code: &ErrorCode) -> bool { is_permanent_error(code) || matches!(code, ErrorCode::StaleRef | ErrorCode::AmbiguousTarget) } -fn is_retryable_resolve_error(error: &AdapterError) -> bool { - matches!( - error.code, - ErrorCode::StaleRef - | ErrorCode::AmbiguousTarget - | ErrorCode::Timeout - | ErrorCode::AppUnresponsive - ) && error.is_explicitly_retryable() -} - fn timeout(state: &RefActionPollState, deadline: Deadline) -> AdapterError { let mut details = json!({ "kind": "actionability_timeout", diff --git a/crates/macos/src/actions/dispatch.rs b/crates/macos/src/actions/dispatch.rs index 6c9dae6..58d835b 100644 --- a/crates/macos/src/actions/dispatch.rs +++ b/crates/macos/src/actions/dispatch.rs @@ -1,6 +1,6 @@ use agent_desktop_core::{ - Action, ActionResult, ActionStep, AdapterError, Deadline, ElementState, ErrorCode, - StepMechanism, action_request::ActionRequest, action_step_outcome::ActionStepOutcome, + Action, ActionResult, ActionStep, AdapterError, Deadline, ErrorCode, StepMechanism, + action_request::ActionRequest, action_step_outcome::ActionStepOutcome, }; #[cfg(target_os = "macos")] @@ -193,22 +193,13 @@ mod imp { } } - if !delivery_occurred(&steps) { - return Ok(ActionResult::satisfied_without_delivery(label).with_steps(steps)); - } - let verified = delivery_was_verified(&steps); - let mut result = ActionResult::delivered_unverified(label).with_steps(steps); - if verified { - result = result.with_verified_delivery(); - } - if !deadline.is_expired() - && let Some(state) = crate::actions::post_state::read_post_state(el, action, deadline) + let post_state = if delivery_occurred(&steps) && !deadline.is_expired() { + crate::actions::post_state::read_post_state(el, action, deadline) .map_err(after_delivery)? - { - verify_post_state(action, &state).map_err(after_delivery)?; - result = result.with_state(state); - } - Ok(result) + } else { + None + }; + ActionResult::from_execution(action, steps, post_state) } fn run_chain( @@ -230,13 +221,8 @@ mod imp { ) } - fn delivery_was_verified(steps: &[ActionStep]) -> bool { - let delivered = steps - .iter() - .filter(|step| matches!(step.outcome, ActionStepOutcome::Succeeded)) - .filter_map(ActionStep::verified) - .collect::>(); - !delivered.is_empty() && delivered.into_iter().all(|verified| verified) + fn after_delivery(error: AdapterError) -> AdapterError { + error.with_disposition(agent_desktop_core::DeliverySemantics::delivered_unverified()) } fn delivery_occurred(steps: &[ActionStep]) -> bool { @@ -244,87 +230,6 @@ mod imp { .iter() .any(|step| matches!(step.outcome, ActionStepOutcome::Succeeded)) } - - fn after_delivery(error: AdapterError) -> AdapterError { - error.with_disposition(agent_desktop_core::DeliverySemantics::delivered_unverified()) - } - - fn verify_post_state(action: &Action, state: &ElementState) -> Result<(), AdapterError> { - if matches!(action, Action::Clear) - && state - .value - .as_deref() - .is_some_and(|value| !value.is_empty()) - { - return Err(AdapterError::new( - ErrorCode::ActionFailed, - "Clear reported success but element value is still non-empty", - ) - .with_suggestion("Retry 'clear', or use 'press cmd+a' then 'press delete'.")); - } - Ok(()) - } - - #[cfg(test)] - mod tests { - use super::*; - use agent_desktop_core::element_state::ElementState; - - #[test] - fn clear_post_state_fails_when_value_remains() { - let err = verify_post_state( - &Action::Clear, - &ElementState { - role: "textfield".into(), - states: vec![], - value: Some("still here".into()), - enabled: None, - hidden: None, - offscreen: None, - }, - ) - .unwrap_err(); - - assert_eq!(err.code, ErrorCode::ActionFailed); - } - - #[test] - fn clear_post_state_accepts_empty_value() { - verify_post_state( - &Action::Clear, - &ElementState { - role: "textfield".into(), - states: vec![], - value: Some(String::new()), - enabled: None, - hidden: None, - offscreen: None, - }, - ) - .unwrap(); - } - - #[test] - fn result_delivery_is_derived_from_verified_steps() { - let verified = ActionStep::succeeded("AXValue") - .with_mechanism(StepMechanism::SemanticApi) - .with_verified(true); - let unverified = ActionStep::succeeded("AXPress") - .with_mechanism(StepMechanism::SemanticApi) - .with_verified(false); - - assert!(delivery_was_verified(&[verified])); - assert!(!delivery_was_verified(&[unverified])); - } - - #[test] - fn skipped_verified_step_does_not_claim_delivery() { - let skipped = ActionStep::skipped("AlreadyInState").with_verified(true); - - assert!(!delivery_occurred(std::slice::from_ref(&skipped))); - assert!(!delivery_was_verified(&[skipped])); - } - } } #[cfg(not(target_os = "macos"))] diff --git a/crates/macos/src/tree/query/traversal.rs b/crates/macos/src/tree/query/traversal.rs index 55aa3d4..b3132ad 100644 --- a/crates/macos/src/tree/query/traversal.rs +++ b/crates/macos/src/tree/query/traversal.rs @@ -84,7 +84,6 @@ impl LocatorTraversal { return Ok(None); } self.note_visit(logical_depth, raw_depth); - crate::tree::attributes::set_messaging_timeout(&element, self.deadline)?; let requirements = self.request.evidence_for_raw_depth(raw_depth); let boundary_elements = if raw_depth == 0 && self.request.hydrates_root_name_from_children() { diff --git a/npm/scripts/postinstall.js b/npm/scripts/postinstall.js index 21c73ba..dde791a 100644 --- a/npm/scripts/postinstall.js +++ b/npm/scripts/postinstall.js @@ -9,7 +9,6 @@ const { readFileSync, readdirSync, renameSync, - rmSync, symlinkSync, unlinkSync, writeFileSync, @@ -50,6 +49,18 @@ function log(msg) { process.stderr.write(`agent-desktop: ${msg}\n`); } +function trashRecoverably(path, trashCommand = 'trash') { + try { + execFileSync(trashCommand, [path], { stdio: 'pipe', timeout: 30000 }); + } catch (err) { + if (!existsSync(path)) return; + const reason = err.code === 'ENOENT' + ? `trash command is unavailable: ${trashCommand}` + : `trash exited with status ${err.status ?? 'unknown'}`; + log(`Could not move cleanup artifact to Trash; retained at ${path}: ${reason}`); + } +} + function getPlatformKey() { return `${platform()}-${arch()}`; } @@ -126,7 +137,7 @@ function validateArchive(tarballPath) { } } -function installArchive(tarballPath, binaryPath, helperPath) { +function installArchive(tarballPath, binaryPath, helperPath, trashCommand = 'trash') { validateArchive(tarballPath); const staging = mkdtempSync(join(binDir, '.extract-')); try { @@ -147,7 +158,7 @@ function installArchive(tarballPath, binaryPath, helperPath) { installExecutable(extractedHelper, helperPath); installExecutable(extractedBinary, binaryPath); } finally { - rmSync(staging, { recursive: true, force: true }); + trashRecoverably(staging, trashCommand); } } @@ -307,5 +318,6 @@ module.exports = { checksumFor, customHelperPath, installArchive, + trashRecoverably, validateArchive, }; diff --git a/scripts/ci-ffi-helper-discovery-smoke.sh b/scripts/ci-ffi-helper-discovery-smoke.sh new file mode 100755 index 0000000..0f502e7 --- /dev/null +++ b/scripts/ci-ffi-helper-discovery-smoke.sh @@ -0,0 +1,18 @@ +#!/usr/bin/env bash +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$ROOT" + +STAGE=$(mktemp -d /tmp/agent-desktop-ffi-helper.XXXXXX) +cp target/release-ffi/libagent_desktop_ffi.dylib "$STAGE/" +cp target/release/agent-desktop-macos-helper "$STAGE/" +chmod +x "$STAGE/agent-desktop-macos-helper" +DYLIB_SHA=$(shasum -a 256 "$STAGE/libagent_desktop_ffi.dylib" | awk '{print $1}') +HELPER_SHA=$(shasum -a 256 "$STAGE/agent-desktop-macos-helper" | awk '{print $1}') +AD_DYLIB_PATH="$STAGE/libagent_desktop_ffi.dylib" \ + AD_HEADER_PATH=crates/ffi/include/agent_desktop.h \ + AD_EXPECT_MACOS_HELPER=1 \ + python3 tests/ffi-python/smoke.py +test "$DYLIB_SHA" = "$(shasum -a 256 "$STAGE/libagent_desktop_ffi.dylib" | awk '{print $1}')" +test "$HELPER_SHA" = "$(shasum -a 256 "$STAGE/agent-desktop-macos-helper" | awk '{print $1}')" diff --git a/scripts/ci-npm-wrapper-smoke.sh b/scripts/ci-npm-wrapper-smoke.sh new file mode 100755 index 0000000..fb2521d --- /dev/null +++ b/scripts/ci-npm-wrapper-smoke.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +set -euo pipefail + +ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +cd "$ROOT" + +case "$(uname -s)-$(uname -m)" in + Darwin-arm64) NAME=agent-desktop-darwin-arm64 ;; + Darwin-x86_64) NAME=agent-desktop-darwin-x64 ;; + *) + echo "Unsupported smoke-test platform: $(uname -s)-$(uname -m)" + exit 1 + ;; +esac +HELPER_NAME=agent-desktop-macos-helper +SOURCE=target/release/agent-desktop +HELPER_SOURCE=target/release/agent-desktop-macos-helper +SOURCE_SHA=$(shasum -a 256 "$SOURCE" | awk '{print $1}') +HELPER_SOURCE_SHA=$(shasum -a 256 "$HELPER_SOURCE" | awk '{print $1}') +cp "$SOURCE" "npm/bin/${NAME}" +cp "$HELPER_SOURCE" "npm/bin/${HELPER_NAME}" +chmod +x "npm/bin/${NAME}" +chmod +x "npm/bin/${HELPER_NAME}" +COPY_SHA=$(shasum -a 256 "npm/bin/${NAME}" | awk '{print $1}') +HELPER_COPY_SHA=$(shasum -a 256 "npm/bin/${HELPER_NAME}" | awk '{print $1}') +if [ "$SOURCE_SHA" != "$COPY_SHA" ] || [ "$HELPER_SOURCE_SHA" != "$HELPER_COPY_SHA" ]; then + echo "NPM smoke executable copy failed immutable identity verification" >&2 + exit 1 +fi +node npm/bin/agent-desktop.js version > /tmp/agent-desktop-version.json +node -e " + const out = require('fs').readFileSync('/tmp/agent-desktop-version.json', 'utf8'); + const json = JSON.parse(out); + if (json.ok !== true || !json.data || typeof json.data.version !== 'string') { + throw new Error('agent-desktop npm wrapper did not return version JSON'); + } +" +if [ "$SOURCE_SHA" != "$(shasum -a 256 "$SOURCE" | awk '{print $1}')" ] || \ + [ "$COPY_SHA" != "$(shasum -a 256 "npm/bin/${NAME}" | awk '{print $1}')" ] || \ + [ "$HELPER_SOURCE_SHA" != "$(shasum -a 256 "$HELPER_SOURCE" | awk '{print $1}')" ] || \ + [ "$HELPER_COPY_SHA" != "$(shasum -a 256 "npm/bin/${HELPER_NAME}" | awk '{print $1}')" ]; then + echo "NPM smoke executable changed while it was executed" >&2 + exit 1 +fi diff --git a/tests/e2e/bounded_process.py b/tests/e2e/bounded_process.py new file mode 100644 index 0000000..2633b07 --- /dev/null +++ b/tests/e2e/bounded_process.py @@ -0,0 +1,208 @@ +import dataclasses +import os +import resource +import selectors +import signal +import subprocess +import time + + +DEFAULT_TIMEOUT_SECONDS = 20.0 +DEFAULT_MAX_CAPTURE_BYTES = 2 * 1024 * 1024 +INTERACTION_LEASE_FD_ENV = "AGENT_DESKTOP_INTERACTION_LEASE_FD" +INHERIT_INTERACTION_LEASE_ENV = "AGENT_DESKTOP_E2E_INHERIT_LEASE" + + +@dataclasses.dataclass +class BoundedResult: + args: list[str] + returncode: int + stdout: str + stderr: str + timed_out: bool + output_limited: bool + termination_error: str | None + wall_ms: float + cpu_ms: float + + +def _terminate_group(process): + if process.poll() is not None: + return None + try: + os.killpg(process.pid, signal.SIGTERM) + except ProcessLookupError: + return None + except OSError as error: + try: + process.terminate() + except OSError: + pass + return f"process-group SIGTERM failed: {error}" + try: + process.wait(timeout=0.25) + return None + except subprocess.TimeoutExpired: + pass + try: + os.killpg(process.pid, signal.SIGKILL) + except ProcessLookupError: + return None + except OSError as error: + try: + process.kill() + except OSError: + pass + return f"process-group SIGKILL failed: {error}" + return None + + +def _merge_error(current, additional): + if not additional: + return current + return f"{current}; {additional}" if current else additional + + +def _interaction_lease_fds(env): + environment = os.environ if env is None else env + raw_fd = environment.get(INTERACTION_LEASE_FD_ENV) + if raw_fd is None: + return () + try: + fd = int(raw_fd) + except ValueError as error: + raise ValueError("interaction lease FD must be a decimal integer") from error + if fd < 0: + raise ValueError("interaction lease FD must be nonnegative") + os.fstat(fd) + return (fd,) + + +def _close_streams(selector, process): + for stream in (process.stdout, process.stderr): + if stream is None: + continue + try: + selector.unregister(stream) + except (KeyError, ValueError): + pass + stream.close() + + +def run_bounded( + command, + timeout_seconds=None, + max_capture_bytes=None, + env=None, + inherit_interaction_lease=False, +): + timeout_seconds = float( + timeout_seconds + if timeout_seconds is not None + else os.environ.get("AGENT_DESKTOP_E2E_TIMEOUT_SECONDS", DEFAULT_TIMEOUT_SECONDS) + ) + max_capture_bytes = int( + max_capture_bytes + if max_capture_bytes is not None + else os.environ.get("AGENT_DESKTOP_E2E_MAX_CAPTURE_BYTES", DEFAULT_MAX_CAPTURE_BYTES) + ) + if timeout_seconds <= 0 or max_capture_bytes <= 0: + raise ValueError("timeout and capture limits must be positive") + + usage_before = resource.getrusage(resource.RUSAGE_CHILDREN) + started = time.perf_counter() + child_env = dict(os.environ if env is None else env) + pass_fds = _interaction_lease_fds(child_env) if inherit_interaction_lease else () + if not inherit_interaction_lease: + child_env.pop(INTERACTION_LEASE_FD_ENV, None) + child_env.pop(INHERIT_INTERACTION_LEASE_ENV, None) + process = subprocess.Popen( + command, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + start_new_session=True, + env=child_env, + pass_fds=pass_fds, + ) + selector = selectors.DefaultSelector() + selector.register(process.stdout, selectors.EVENT_READ, "stdout") + selector.register(process.stderr, selectors.EVENT_READ, "stderr") + chunks = {"stdout": [], "stderr": []} + captured = 0 + deadline = started + timeout_seconds + timed_out = False + output_limited = False + termination_error = None + + while selector.get_map(): + remaining = deadline - time.perf_counter() + if remaining <= 0: + timed_out = True + termination_error = _merge_error(termination_error, _terminate_group(process)) + _close_streams(selector, process) + break + events = selector.select(min(remaining, 0.1)) + if not events: + process.poll() + continue + for key, _ in events: + data = os.read(key.fileobj.fileno(), 65536) + if not data: + selector.unregister(key.fileobj) + key.fileobj.close() + continue + available = max_capture_bytes - captured + if len(data) > available: + if available > 0: + chunks[key.data].append(data[:available]) + captured += available + output_limited = True + termination_error = _merge_error(termination_error, _terminate_group(process)) + _close_streams(selector, process) + break + chunks[key.data].append(data) + captured += len(data) + if output_limited: + break + + selector.close() + if not timed_out and not output_limited: + remaining = max(0.0, deadline - time.perf_counter()) + try: + process.wait(timeout=remaining) + except subprocess.TimeoutExpired: + timed_out = True + termination_error = _merge_error(termination_error, _terminate_group(process)) + try: + process.wait(timeout=1) + except subprocess.TimeoutExpired: + termination_error = _merge_error(termination_error, _terminate_group(process)) + try: + process.wait(timeout=1) + except subprocess.TimeoutExpired: + termination_error = _merge_error( + termination_error, "child could not be reaped after termination" + ) + + usage_after = resource.getrusage(resource.RUSAGE_CHILDREN) + cpu_before = usage_before.ru_utime + usage_before.ru_stime + cpu_after = usage_after.ru_utime + usage_after.ru_stime + if timed_out: + returncode = 124 + elif output_limited: + returncode = 125 + elif process.returncode is None: + returncode = 126 + else: + returncode = int(process.returncode) + return BoundedResult( + args=list(command), + returncode=returncode, + stdout=b"".join(chunks["stdout"]).decode("utf-8", errors="replace"), + stderr=b"".join(chunks["stderr"]).decode("utf-8", errors="replace"), + timed_out=timed_out, + output_limited=output_limited, + termination_error=termination_error, + wall_ms=(time.perf_counter() - started) * 1000, + cpu_ms=max(0.0, (cpu_after - cpu_before) * 1000), + ) diff --git a/tests/e2e/json_tool.py b/tests/e2e/json_tool.py index ebb0648..1179f4d 100755 --- a/tests/e2e/json_tool.py +++ b/tests/e2e/json_tool.py @@ -1,217 +1,12 @@ #!/usr/bin/env python3 -import dataclasses import json import os -import resource -import selectors -import signal -import subprocess import sys -import time + +from bounded_process import INHERIT_INTERACTION_LEASE_ENV, run_bounded -DEFAULT_TIMEOUT_SECONDS = 20.0 -DEFAULT_MAX_CAPTURE_BYTES = 2 * 1024 * 1024 MAX_JSON_INPUT_BYTES = 4 * 1024 * 1024 -INTERACTION_LEASE_FD_ENV = "AGENT_DESKTOP_INTERACTION_LEASE_FD" -INHERIT_INTERACTION_LEASE_ENV = "AGENT_DESKTOP_E2E_INHERIT_LEASE" - - -@dataclasses.dataclass -class BoundedResult: - args: list[str] - returncode: int - stdout: str - stderr: str - timed_out: bool - output_limited: bool - termination_error: str | None - wall_ms: float - cpu_ms: float - - -def _terminate_group(process): - if process.poll() is not None: - return None - try: - os.killpg(process.pid, signal.SIGTERM) - except ProcessLookupError: - return None - except OSError as error: - try: - process.terminate() - except OSError: - pass - return f"process-group SIGTERM failed: {error}" - try: - process.wait(timeout=0.25) - return None - except subprocess.TimeoutExpired: - pass - try: - os.killpg(process.pid, signal.SIGKILL) - except ProcessLookupError: - return None - except OSError as error: - try: - process.kill() - except OSError: - pass - return f"process-group SIGKILL failed: {error}" - return None - - -def _merge_error(current, additional): - if not additional: - return current - return f"{current}; {additional}" if current else additional - - -def _interaction_lease_fds(env): - environment = os.environ if env is None else env - raw_fd = environment.get(INTERACTION_LEASE_FD_ENV) - if raw_fd is None: - return () - try: - fd = int(raw_fd) - except ValueError as error: - raise ValueError("interaction lease FD must be a decimal integer") from error - if fd < 0: - raise ValueError("interaction lease FD must be nonnegative") - os.fstat(fd) - return (fd,) - - -def _close_streams(selector, process): - for stream in (process.stdout, process.stderr): - if stream is None: - continue - try: - selector.unregister(stream) - except (KeyError, ValueError): - pass - stream.close() - - -def run_bounded( - command, - timeout_seconds=None, - max_capture_bytes=None, - env=None, - inherit_interaction_lease=False, -): - timeout_seconds = float( - timeout_seconds - if timeout_seconds is not None - else os.environ.get("AGENT_DESKTOP_E2E_TIMEOUT_SECONDS", DEFAULT_TIMEOUT_SECONDS) - ) - max_capture_bytes = int( - max_capture_bytes - if max_capture_bytes is not None - else os.environ.get("AGENT_DESKTOP_E2E_MAX_CAPTURE_BYTES", DEFAULT_MAX_CAPTURE_BYTES) - ) - if timeout_seconds <= 0 or max_capture_bytes <= 0: - raise ValueError("timeout and capture limits must be positive") - - usage_before = resource.getrusage(resource.RUSAGE_CHILDREN) - started = time.perf_counter() - child_env = dict(os.environ if env is None else env) - pass_fds = _interaction_lease_fds(child_env) if inherit_interaction_lease else () - if not inherit_interaction_lease: - child_env.pop(INTERACTION_LEASE_FD_ENV, None) - child_env.pop(INHERIT_INTERACTION_LEASE_ENV, None) - process = subprocess.Popen( - command, - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - start_new_session=True, - env=child_env, - pass_fds=pass_fds, - ) - selector = selectors.DefaultSelector() - selector.register(process.stdout, selectors.EVENT_READ, "stdout") - selector.register(process.stderr, selectors.EVENT_READ, "stderr") - chunks = {"stdout": [], "stderr": []} - captured = 0 - deadline = started + timeout_seconds - timed_out = False - output_limited = False - termination_error = None - - while selector.get_map(): - remaining = deadline - time.perf_counter() - if remaining <= 0: - timed_out = True - termination_error = _merge_error(termination_error, _terminate_group(process)) - _close_streams(selector, process) - break - events = selector.select(min(remaining, 0.1)) - if not events: - process.poll() - continue - for key, _ in events: - data = os.read(key.fileobj.fileno(), 65536) - if not data: - selector.unregister(key.fileobj) - key.fileobj.close() - continue - available = max_capture_bytes - captured - if len(data) > available: - if available > 0: - chunks[key.data].append(data[:available]) - captured += available - output_limited = True - termination_error = _merge_error(termination_error, _terminate_group(process)) - _close_streams(selector, process) - break - chunks[key.data].append(data) - captured += len(data) - if output_limited: - break - - selector.close() - if not timed_out and not output_limited: - remaining = max(0.0, deadline - time.perf_counter()) - try: - process.wait(timeout=remaining) - except subprocess.TimeoutExpired: - timed_out = True - termination_error = _merge_error(termination_error, _terminate_group(process)) - try: - process.wait(timeout=1) - except subprocess.TimeoutExpired: - termination_error = _merge_error(termination_error, _terminate_group(process)) - try: - process.wait(timeout=1) - except subprocess.TimeoutExpired: - termination_error = _merge_error( - termination_error, "child could not be reaped after termination" - ) - - usage_after = resource.getrusage(resource.RUSAGE_CHILDREN) - cpu_before = usage_before.ru_utime + usage_before.ru_stime - cpu_after = usage_after.ru_utime + usage_after.ru_stime - if timed_out: - returncode = 124 - elif output_limited: - returncode = 125 - elif process.returncode is None: - returncode = 126 - else: - returncode = int(process.returncode) - return BoundedResult( - args=list(command), - returncode=returncode, - stdout=b"".join(chunks["stdout"]).decode("utf-8", errors="replace"), - stderr=b"".join(chunks["stderr"]).decode("utf-8", errors="replace"), - timed_out=timed_out, - output_limited=output_limited, - termination_error=termination_error, - wall_ms=(time.perf_counter() - started) * 1000, - cpu_ms=max(0.0, (cpu_after - cpu_before) * 1000), - ) - - def read_json(): payload = sys.stdin.buffer.read(MAX_JSON_INPUT_BYTES + 1) if len(payload) > MAX_JSON_INPUT_BYTES: diff --git a/tests/npm/postinstall.test.js b/tests/npm/postinstall.test.js index 76f28db..b0db9e5 100644 --- a/tests/npm/postinstall.test.js +++ b/tests/npm/postinstall.test.js @@ -2,10 +2,10 @@ const assert = require('node:assert/strict'); const { execFileSync } = require('node:child_process'); const { chmodSync, + existsSync, mkdtempSync, mkdirSync, readFileSync, - rmSync, writeFileSync, } = require('node:fs'); const { tmpdir } = require('node:os'); @@ -19,7 +19,7 @@ const roots = []; afterEach(() => { delete process.env.AGENT_DESKTOP_MACOS_HELPER_PATH; for (const root of roots.splice(0)) { - rmSync(root, { recursive: true, force: true }); + postinstall.trashRecoverably(root); } }); @@ -43,6 +43,28 @@ function archive(entries) { return { root, tarball }; } +function executable(contents) { + const root = temporaryDirectory(); + const path = join(root, 'trash'); + writeFileSync(path, contents, { mode: 0o755 }); + return path; +} + +function captureWarnings(run) { + const warnings = []; + const write = process.stderr.write; + process.stderr.write = (chunk) => { + warnings.push(String(chunk)); + return true; + }; + try { + run(); + return warnings.join(''); + } finally { + process.stderr.write = write; + } +} + test('checksum lookup requires an exact archive name', () => { const hash = 'a'.repeat(64); assert.equal(postinstall.checksumFor(`${hash} release.tar.gz\n`, 'release.tar.gz'), hash); @@ -82,6 +104,66 @@ test('archive installation preserves the exact paired executables', () => { assert.equal(readFileSync(helper, 'utf8'), 'helper-build-v1'); }); +test('recoverable cleanup invokes trash and removes the original path', () => { + const target = temporaryDirectory(); + const recovered = `${target}.recovered`; + roots.push(recovered); + const fakeTrash = executable('#!/bin/sh\nmv "$1" "$1.recovered"\n'); + + postinstall.trashRecoverably(target, fakeTrash); + assert.equal(existsSync(target), false); + assert.equal(existsSync(recovered), true); +}); + +test('recoverable cleanup retains artifacts and warns when trash is unavailable or fails', () => { + const unavailable = temporaryDirectory(); + const failing = temporaryDirectory(); + const fakeTrash = executable('#!/bin/sh\nexit 9\n'); + + for (const [target, command, reason] of [ + [ + unavailable, + '/definitely-missing-agent-desktop-trash', + 'trash command is unavailable: /definitely-missing-agent-desktop-trash', + ], + [failing, fakeTrash, 'trash exited with status 9'], + ]) { + const warnings = captureWarnings(() => + postinstall.trashRecoverably(target, command), + ); + assert.equal(existsSync(target), true); + assert.ok(warnings.includes(`retained at ${target}:`)); + assert.ok(warnings.includes(reason)); + } +}); + +test('cleanup failure does not mask a successful archive install', () => { + const payload = archive({ + 'agent-desktop': 'cli-build-v2', + 'agent-desktop-macos-helper': 'helper-build-v2', + }); + const destination = temporaryDirectory(); + const binary = join(destination, 'agent-desktop-darwin-arm64'); + const helper = join(destination, 'agent-desktop-macos-helper'); + + const warnings = captureWarnings(() => + postinstall.installArchive( + payload.tarball, + binary, + helper, + '/definitely-missing-agent-desktop-trash', + ), + ); + + assert.equal(readFileSync(binary, 'utf8'), 'cli-build-v2'); + assert.equal(readFileSync(helper, 'utf8'), 'helper-build-v2'); + assert.match(warnings, /Could not move cleanup artifact to Trash; retained at .*\.extract-/); + const retained = warnings.match(/retained at (.*\.extract-[^:]+):/)?.[1]; + assert.ok(retained); + assert.equal(existsSync(retained), true); + roots.push(retained); +}); + test('custom helper override must be absolute', () => { process.env.AGENT_DESKTOP_MACOS_HELPER_PATH = 'relative-helper'; assert.throws(