diff --git a/CONCEPTS.md b/CONCEPTS.md index 1b5f889..f2de5c0 100644 --- a/CONCEPTS.md +++ b/CONCEPTS.md @@ -44,6 +44,11 @@ A coordination key for one agent or a coordinated group of agents that share a l Use sessions when callers intentionally omit `--snapshot` and want a shared latest observation. Explicit snapshot IDs remain the deterministic path for pinned actions and can be resolved without also passing the session. +### Protected Process +A session-critical operating-system process that agent-desktop refuses to close on every surface, because terminating it would break the user's desktop session. + +The refusal is enforced where the close happens, so CLI, FFI, and any future consumer behave identically. Matching is exact — a process name or a bundle-identifier component, never a substring — so lookalike applications that merely contain a protected name stay closable. + ## Action Reliability ### Actionability @@ -62,6 +67,11 @@ A ref-based action that uses semantic accessibility operations without implicit Headless ref actions may still fail when the native accessibility API cannot perform the requested semantic operation; they fail closed with `POLICY_DENIED` rather than silently substituting physical input. The broader **headed** policy must be selected explicitly with `--headed`. +### Action Chain +The ordered ladder of strategies a ref action walks to perform one intent — semantic accessibility actions first, then settable attributes, then policy-gated physical input — with each step verified against the element's observed state before it counts as success. + +The chain pins one execution deadline at its start (distinct from the Resolver Deadline, which budgets re-identification) and every step observes it. Expiry while a step may have partially mutated the element surfaces as a structured timeout carrying the observed state, never as a plain step failure — the caller must be able to tell "nothing happened" from "something may have happened". + ### Wait Predicate The condition a wait command polls for before returning, such as element actionability, text presence, window appearance, menu state, or notification arrival. @@ -71,9 +81,11 @@ The remaining time budget carried through strict ref resolution so every native ### Coordinate Fallback An explicit opt-in path that uses screen coordinates or physical input when semantic accessibility operations cannot perform the requested action. +Physical input lands on the topmost window at the target point, so the fallback first ensures the target element's own window is frontmost — the app being frontmost is not sufficient when the element lives in a background window of that app. + ### FFI Ref-Action Parity The requirement that language bindings using refs follow the same strict resolution, actionability, and interaction-policy semantics as CLI ref commands. ## Relationships -A session owns one latest-snapshot pointer. A snapshot persists a ref map and can be selected directly by snapshot ID. A ref resolves through strict ref resolution into live native evidence, then actionability decides whether a headless ref action can safely dispatch. FFI ref-action parity keeps that same relationship true for language bindings. +A session owns one latest-snapshot pointer. A snapshot persists a ref map and can be selected directly by snapshot ID. A ref resolves through strict ref resolution into live native evidence, then actionability decides whether a headless ref action can safely dispatch, and the action chain executes that dispatch under its own deadline with the interaction policy gating its physical steps. FFI ref-action parity keeps that same relationship true for language bindings. diff --git a/docs/solutions/best-practices/abort-state-guidance-multi-step-physical-input.md b/docs/solutions/best-practices/abort-state-guidance-multi-step-physical-input.md new file mode 100644 index 0000000..928c44b --- /dev/null +++ b/docs/solutions/best-practices/abort-state-guidance-multi-step-physical-input.md @@ -0,0 +1,124 @@ +--- +title: Abort-state guidance on multi-step physical input errors +date: 2026-06-10 +category: best-practices +module: crates/macos +problem_type: best_practice +component: macos-adapter +severity: high +applies_when: + - Implementing a multi-step physical input sequence (drag, gesture, press-hold) using CGEvent synthesis + - An early return or error exit can leave cursor or button state at an intermediate position + - A Drop guard is introduced to cancel a partially committed OS operation + - An error from a multi-phase input helper needs recovery guidance attached + - The caller must distinguish "no drop committed" from "drop committed at the wrong target" +tags: + - drag + - cgevent + - abort-state + - error-recovery + - mouse-input + - headless-headed + - macos + - physical-fallback +--- + +# Abort-state guidance on multi-step physical input errors + +## Context + +A synthetic drag is a multi-step OS mutation: `LeftMouseDown` at the origin, interpolated `LeftMouseDragged` events, a dwell over the destination, then a final `LeftMouseUp`. Any step after the mouse-down can fail, and the original `MouseUpGuard` in `crates/macos/src/input/mouse.rs` had two bugs at once. The happy path disarmed the guard *before* posting the final `LeftMouseUp`, so a failure on that last post left the button logically held down system-wide — the exact defect the guard existed to prevent. And the guard's corrective release on `Drop` fired at the *destination*: CGEvents resolve at the coordinates embedded in the event, so a "corrective" up at the destination is indistinguishable from a successful drop. The file landed in the target folder while the command reported failure — the agent's perception (error) and the system's state (dropped) irreconcilably diverged. + +## Guidance + +The pattern has three coordinated legs. + +**Leg 1 — the guard owns the release, and disarms only after the post succeeds.** + +```rust +// crates/macos/src/input/mouse.rs +impl MouseUpGuard { + fn release_at(&mut self, point: CGPoint) -> Result<(), AdapterError> { + post_event(CGEventType::LeftMouseUp, point, CGMouseButton::Left)?; + self.armed = false; + Ok(()) + } +} +``` + +The happy path calls `release.release_at(to)` as its final statement. If that post fails, `armed` stays `true` and `Drop` still runs the abort path. + +**Leg 2 — abort at the origin, never the unreached destination.** + +```rust +impl Drop for MouseUpGuard { + fn drop(&mut self) { + if self.armed { + let _ = post_event(CGEventType::LeftMouseDragged, self.origin, CGMouseButton::Left); + let _ = post_event(CGEventType::LeftMouseUp, self.origin, CGMouseButton::Left); + } + } +} +``` + +`origin` is captured at `LeftMouseDown`. Releasing where the gesture picked up is a self-drop — a no-op to most drop targets — which gives the abort genuine cancel semantics. There is no "current cursor position" escape hatch: the coordinate in the event is where the OS resolves the release. + +**Leg 3 — every error from the sequence carries the end state.** + +```rust +pub fn synthesize_drag(params: DragParams) -> Result<(), AdapterError> { + drag_sequence(params).map_err(|err| { + if err.suggestion.is_some() { + return err; + } + err.with_suggestion( + "The drag was aborted: the button was released back at the origin (best-effort) and no drop was committed at the destination. The cursor ends at the origin. Re-check the source state before retrying.", + ) + }) +} +``` + +One `map_err` at the public boundary; inner errors that already carry a tailored suggestion pass through unchanged. The guard's doc comment also states the best-effort limits honestly: the corrective posts can themselves fail (typically the same systemic CGEventSource failure that aborted the drag), and a drop target sitting under the origin still sees a self-drop. + +## Why This Matters + +Without leg 1, a failure on the final post leaves the button logically held: the OS treats every subsequent cursor movement as a drag and every click target as a drop target, corrupting unrelated interactions until something releases the button. + +Without leg 2, an aborted drag silently commits as a real drop. This is the worst failure class for an agent caller: the error says "retry", but the filesystem or UI already changed — retrying duplicates the operation. + +Without leg 3, an agent holding `ACTION_FAILED`/`INTERNAL` cannot tell whether the world changed. With the suggestion, it knows: button released at origin, no drop committed, cursor at origin — and whether a re-snapshot is needed before retry. + +## When to Apply + +- A command posts a sequence of OS-level events where a later step can fail after an earlier irreversible step succeeded +- The abort path could itself mutate state (a corrective event at the wrong coordinates commits, not cancels) +- Errors must communicate post-failure system state, not just the failure cause, for callers to retry safely + +Specifically necessary for anything backed by `CGEventPost`-style APIs that resolve coordinates at event-creation time. + +## Examples + +Before (both bugs, as originally shipped): + +```rust +// happy path disarmed BEFORE the final fallible post +release.armed = false; +post_event(CGEventType::LeftMouseUp, to, CGMouseButton::Left) + +// and Drop released at the DESTINATION, committing the aborted drop +impl Drop for MouseUpGuard { + fn drop(&mut self) { + if self.armed { + let _ = post_event(CGEventType::LeftMouseUp, self.to, CGMouseButton::Left); + } + } +} +``` + +The fixed version is the Guidance section above. Verified by the E2E drag scenario observing the canvas effect, not the command's `ok:true`. + +## Related + +- `best-practices/macos-gesture-headless-capability-2026-06-10.md` — drag/hover/mouse-* are always physical and policy-gated; establishes when this multi-step path runs at all +- `best-practices/playwright-grade-desktop-reliability-2026-06-02.md` — the upstream principle that fallbacks must be explicit and failures honest; abort-state cleanup is that principle applied to mid-sequence failure +- `best-practices/preserve-command-policy-semantics-during-refactor-2026-05-12.md` — when the physical path is even entered (policy ownership) — context for why aborts are headed-mode territory diff --git a/docs/solutions/best-practices/exhaustiveness-guards-over-catch-alls-in-policy-mirrors.md b/docs/solutions/best-practices/exhaustiveness-guards-over-catch-alls-in-policy-mirrors.md new file mode 100644 index 0000000..3fdc7a5 --- /dev/null +++ b/docs/solutions/best-practices/exhaustiveness-guards-over-catch-alls-in-policy-mirrors.md @@ -0,0 +1,122 @@ +--- +title: Use named arms and exhaustiveness guard tests instead of catch-alls in policy and dispatch mirrors +date: 2026-06-10 +category: best-practices +module: crates/core, src +problem_type: best_practice +component: command-policy +severity: high +applies_when: + - A helper mirrors the per-case behavior of real command dispatch (a preflight reproducing each command's policy) + - A new command or action name is added and a parallel mapping must stay in sync + - A match over named semantic cases is tempted to use a catch-all arm + - A registry or test list must provably cover everything the codebase actually contains +tags: + - exhaustiveness + - command-policy + - dispatch + - wait-predicate + - regression-prevention + - contract-tests + - actionability + - rust-patterns +--- + +# Use named arms and exhaustiveness guard tests instead of catch-alls in policy and dispatch mirrors + +## Context + +`wait --predicate actionable --action ` answers "would this action succeed" by running the same actionability preflight the real command runs — which means it must mirror each command's interaction policy exactly. The first implementation centralized that mirror behind a catch-all: + +```rust +fn actionability_request(action: Action) -> ActionRequest { + match action { + Action::TypeText(_) => ActionRequest::focus_fallback(action), + _ => ActionRequest::headless(action), + } +} +``` + +Correct for the four actions that existed — and a trap for the fifth. Any future action would silently inherit headless policy, right or wrong, with no compiler complaint and no failing test. In a headless-first codebase where policy must flow explicitly from each command's declared base, a mirror that *infers* policy is exactly the drift the project's own learnings warn against. Code review flagged the catch-all as a silent-drift risk (auto memory [claude]: the repo treats policy inference as a standing hazard). + +## Guidance + +Three legs: explicit arms, a machine-derived universe guard, and per-case value pins. + +**Leg 1 — one arm per name; the only "default" is an error.** + +```rust +// crates/core/src/commands/wait_predicate.rs +/// Maps each `--action` name to the exact request its real command would +/// run with — policy included — so the preflight answers "would this action +/// succeed". Every name is an explicit arm: a catch-all here would let a +/// new action silently inherit the wrong policy. +fn parse_actionability_action(action: Option<&str>) -> Result { + match action.unwrap_or("click") { + "click" => Ok(ActionRequest::headless(Action::Click)), + "type" => Ok(ActionRequest::focus_fallback(Action::TypeText(String::new()))), + "set-value" => Ok(ActionRequest::headless(Action::SetValue(String::new()))), + "clear" => Ok(ActionRequest::headless(Action::Clear)), + other => Err(AppError::invalid_input_with_suggestion( + format!("Unknown actionability action '{other}'"), + "Use one of: click, type, set-value, clear.", + )), + } +} +``` + +The `other` arm rejects unrecognized input — it is input validation, not a semantic catch-all. Adding an action *requires* adding an arm; forgetting produces a user-visible error at this match, never a silent wrong-policy preflight. + +**Leg 2 — a guard test that derives the case universe mechanically.** + +```rust +// crates/core/src/commands/ref_policy_tests.rs +const POLICY_TESTED_COMMANDS: &[&str] = &[ + "check", "clear", "click", "collapse", "double_click", "expand", + "focus", "right_click", "scroll", "scroll_to", "select", "set_value", + "toggle", "triple_click", "type_text", "uncheck", +]; + +#[test] +fn all_context_request_callers_are_policy_tested() { + // scans crates/core/src/commands/*.rs (excluding *_tests) for files + // containing `context.request(` and fails, naming each stem, when one + // is absent from POLICY_TESTED_COMMANDS +} +``` + +The universe is not hand-maintained: the test scans the filesystem for the call-site signature every ref-action command shares. A new command file that calls `context.request(` without a registered policy assertion fails CI with a message naming the stem and the required follow-up. + +**Leg 3 — per-case value pins.** + +Each listed case is pinned to its exact mirrored value, never covered by an "everything else" loop — `type` is asserted `focus_fallback` specifically, each remaining command asserted headless by name. The same shape guards CLI registration in `src/cli/contract_tests.rs`: `NON_COMMAND_MODULES` plus a filesystem scan asserts every command module is either a registered CLI subcommand or an explicitly declared helper. + +## Why This Matters + +Mirrors rot silently. When real dispatch gains a case but the mirror does not, nothing fails — the mirror's catch-all *answers confidently and wrongly*. Here that means a false "actionable: true" for an action whose real command would run a different policy: the agent's wait reports ready, the action then fails or behaves differently. The compiler cannot help across a string-keyed boundary, so the guard test substitutes for exhaustiveness checking by deriving the universe from the same source of truth the dispatcher uses (the files that exist, the call signature they share). + +## When to Apply + +- A function maps symbolic names to typed behavior where cases must genuinely differ (policy, routing, config) +- A test list or registry mirrors real per-case code and grows as the system grows +- The case universe is file-shaped or string-keyed, so `match` exhaustiveness cannot be compiler-enforced — derive it mechanically instead +- When the universe IS a closed enum, prefer matching on the enum directly and let the compiler enforce exhaustiveness; the guard-test pattern is for universes the compiler cannot see + +## Examples + +Before — the policy mirror with a catch-all (silently wrong for the next action): + +```rust +match action { + Action::TypeText(_) => ActionRequest::focus_fallback(action), + _ => ActionRequest::headless(action), +} +``` + +After — the Guidance section above; the guard is `all_context_request_callers_are_policy_tested`, the value pin is `actionable_parse_mirrors_each_real_command_policy`. + +## Related + +- `best-practices/preserve-command-policy-semantics-during-refactor-2026-05-12.md` — the same risk class (per-case policy flattened by a structural abstraction) from the shared-helper angle; complementary defenses +- `best-practices/playwright-grade-desktop-reliability-2026-06-02.md` — the dispatch-correctness and test-ownership contract this pattern enforces at the match level +- `best-practices/macos-gesture-headless-capability-2026-06-10.md` — the per-gesture policy table whose explicitness named arms preserve diff --git a/docs/solutions/best-practices/ffi-repr-c-struct-size-pinning.md b/docs/solutions/best-practices/ffi-repr-c-struct-size-pinning.md new file mode 100644 index 0000000..f0e27b8 --- /dev/null +++ b/docs/solutions/best-practices/ffi-repr-c-struct-size-pinning.md @@ -0,0 +1,119 @@ +--- +title: Pin repr(C) struct sizes at every layer of the FFI boundary +date: 2026-06-10 +category: best-practices +module: crates/ffi +problem_type: best_practice +component: ffi +severity: high +applies_when: + - Adding or changing a public repr(C) struct in crates/ffi + - A nested repr(C) struct is embedded by value inside another (size change propagates silently) + - Writing or updating the committed C ABI header (include/agent_desktop.h) + - Adding a new FFI integration test in crates/ffi/tests/ + - A C, Swift, Python, Go, or Node consumer allocates the struct on the stack or in a fixed buffer +tags: + - ffi + - abi + - repr-c + - struct-layout + - static-assert + - compile-time-pin + - c-header + - memory-safety +--- + +# Pin repr(C) struct sizes at every layer of the FFI boundary + +## Context + +`AdDragParams` grew from 40 to 48 bytes when `drop_delay_ms: u64` was added. Because `AdDragParams` is embedded **by value** inside `AdAction`, `AdAction` silently grew 88 → 96 bytes with no guard of any kind — the growth was declared nowhere near the `AdAction` definition. A C caller holding the old layout would under-allocate; the Rust side then reads 8 bytes past the caller's buffer, and that stack garbage becomes a live `drop_delay_ms` value. The growth was caught only because a reviewer noticed it manually — it passed CI because there were no pins. + +## Guidance + +Three synchronized layers guard every `repr(C)` struct that crosses the ABI. Each layer catches drift at a different consumer point. + +**Layer 1 — Rust: published const + compile-time assert + extern size fn.** + +```rust +// crates/ffi/src/types/action.rs +pub const AD_ACTION_SIZE: usize = 96; + +const _: () = assert!(std::mem::size_of::() == AD_ACTION_SIZE); + +#[unsafe(no_mangle)] +pub extern "C" fn ad_action_size() -> usize { + std::mem::size_of::() +} +``` + +The anonymous const assert fails the Rust build the moment the layout drifts. The extern function lets any binding language query the true size at startup and compare it against its own layout computation. + +**Layer 2 — C header: macro + C11-gated `_Static_assert` + layout history.** + +```c +/* crates/ffi/include/agent_desktop.h */ +#define AD_ACTION_SIZE (sizeof(AdAction)) +#if defined(__STDC_VERSION__) && __STDC_VERSION__ >= 201112L +_Static_assert(sizeof(AdAction) == 96, "AdAction ABI size changed"); +#endif +``` + +C11 consumers fail at their own compile time when the header and the pinned literal diverge; pre-C11 consumers verify at runtime by comparing their own layout against `ad_action_size()` (the macro is a sizing shorthand, not a pin — `sizeof` always tracks the current struct). A layout-history comment in the header records past size changes (40→48, the AdAction propagation, renames) so fresh reviewers and upgrading callers stop re-discovering adjudicated breaks. + +**Layer 3 — integration test: size, alignment, offset ordering, zeroed-read, const-vs-extern agreement.** + +```rust +// crates/ffi/tests/c_abi_layout.rs +assert_eq!(agent_desktop_ffi::types::action::AD_ACTION_SIZE, 96); +assert_eq!(unsafe { common::ad_action_size() }, AD_ACTION_SIZE); +assert_eq!(size_of::(), 96); +assert_eq!(align_of::(), align_of::()); + +let offsets = [ + offset_of!(AdAction, kind), + offset_of!(AdAction, text), + offset_of!(AdAction, scroll), + offset_of!(AdAction, key), + offset_of!(AdAction, drag), +]; +assert!(offsets.windows(2).all(|pair| pair[0] < pair[1])); + +let copied = unsafe { + let action = MaybeUninit::::zeroed().assume_init(); + std::ptr::read(&action as *const AdAction) +}; +assert_eq!(copied.drag.drop_delay_ms, 0); +``` + +The zeroed-read assertion doubles as a sentinel check: every field must read as a safe default from zero-initialized memory, because the header tells callers to zero-initialize. + +## Why This Matters + +Embedded-by-value fields create a *transitive* size dependency: growing the inner struct grows every outer struct that embeds it, with no declaration at the outer definition. Without pins, that propagation is invisible until a caller under-allocates — undefined behavior in the best case, stack garbage promoted to live field values in the worst. The motivating incident proved this is not theoretical: the field addition passed CI cleanly. + +Three layers because each guards a different party: the Rust assert guards this repo's own builds, the `_Static_assert` guards C consumers compiling against the committed header, and the integration test guards the cross-language agreement (const, extern fn, and real layout all matching). + +## When to Apply + +- Every `#[repr(C)]` struct passed by pointer or embedded by value across the FFI boundary +- Double-apply to the **outer** struct whenever a pinned struct is embedded by value in another +- The extern size fn is mandatory when consumers include runtime-layout languages (Python ctypes, Go cgo, Swift unsafe pointers) + +## Examples + +Adding a field to a pinned struct forces this sequence, and any step done wrong fails loudly: + +1. Add the field to the Rust struct → the `const _` assert fails +2. Update the Rust const to the new size → build green +3. Update the header `_Static_assert` literal and the layout-history comment +4. Update the integration test size assertion and extend the zeroed-read check to the new field +5. The header-compile test and `c_abi_layout` test confirm both sides agree + +The process is self-documenting: the failing assert names the struct and the expectation at every step. + +## Related + +- `best-practices/keep-ffi-action-policy-aligned-with-cli-2026-05-12.md` — the behavioral-parity companion; this doc is the structural-parity half of the same FFI review discipline +- `best-practices/playwright-grade-desktop-reliability-2026-06-02.md` — "FFI and CLI divergence makes language bindings less reliable" is the motivation the pins serve +- `best-practices/deterministic-build-artifact-marker-2026-04-16.md` — records that the header is a hand-committed ABI contract, the discipline these pins extend