Closes P2 todo 011. Reference docs and the crate-level rustdoc had
drifted against shipped APIs:
- ownership.md table still listed the removed raw-array list
surface (`ad_list_apps(... &apps, &count)`, `ad_free_apps`,
`ad_free_window`, `ad_free_windows`, `ad_free_surfaces`) that Unit 5
replaced with opaque list handles.
- build-and-link.md's minimal C example used the same stale API.
- error-handling.md still called `ad_free_window` (renamed to
`ad_release_window_fields` in Unit 5).
- threading.md described debug-only `debug_assert!` with release-build
UB, but todo 002 made the check runtime-enforced in every profile.
- lib.rs crate-level rustdoc repeated the same debug/release phrasing.
Rewrite all four reference pages and the lib.rs rustdoc against
what actually ships:
ownership.md:
- Full opaque-list entry per type (Apps/Windows/Surfaces/Notifications).
- AdImageBuffer accessor pattern documented.
- ad_free_handle: *mut AdNativeHandle, zero-on-success double-free safety.
- Out-param zeroing happens before guards, not after (todo 006 contract).
threading.md:
- "Runtime, every build profile" — no debug/release split.
- Full exempt list: lists accessors, image-buffer accessors,
release_window_fields, free_handle, free_tree, free_action_result,
free_string.
build-and-link.md:
- C example now uses `ad_list_apps(adapter, &list)` +
`ad_app_list_count/_get/_free`.
error-handling.md:
- `ad_free_window` → `ad_release_window_fields`.
- Comment clarified to reflect out-param zero-init.
lib.rs //! rustdoc:
- Matches threading.md's runtime-enforced phrasing and exempt list.
- cbindgen propagates to agent_desktop.h so header consumers see the
right contract.
85 FFI tests pass, clippy clean.
Closes the UTF-8 portion of P2 todo 010. The resolver relaxed-pass
stays — it's the right behavior for ad_find's lossy-rebuild fallback
and the recently-landed bounds_hash preservation already constrains
the duplicate-label drift case.
The UTF-8 side needed a tighter contract. `c_to_string` returned
`Option<String>` and collapsed null + invalid-bytes into the same
`None`, so an app_filter passed in with hostile bytes was silently
treated as "no filter" and widened ad_list_windows,
ad_dismiss_all_notifications, ad_find, etc. to every app on the box.
Add `try_c_to_string` returning `Result<Option<String>, ()>`:
- `Ok(None)` — null pointer (caller treats as absent).
- `Ok(Some(s))` — valid UTF-8.
- `Err(())` — non-null + invalid UTF-8 (caller must surface
AD_RESULT_ERR_INVALID_ARGS).
Add `decode_optional_filter!` macro so call sites read as a single
line:
let role_filter = decode_optional_filter!(q.role, "query.role");
The macro shortcircuits the enclosing AdResult fn with InvalidArgs
and a tailored last-error ("<label> is not valid UTF-8").
Applied to every optional filter pointer consumer:
- observation/find.rs: role, name_substring, value_substring
- observation/is.rs: role, name_substring, value_substring
- windows/list.rs: app_filter
- notifications/dismiss.rs: app_filter
- notifications/dismiss_all.rs: app_filter
- notifications/filter.rs: filter_from_c returns Result; app/text
fields fail-closed. list_notifications call site propagates.
Regression test in c_abi_harness.rs:
- invalid_utf8_filter_rejected_not_silently_widened: passes a
truncated-UTF-8 byte pair as ad_list_windows app_filter; must return
InvalidArgs (or ErrInternal on worker) with list still null, never
a populated list built from the widened "all apps" scope.
85 FFI tests pass, clippy clean.
Closes P1 todo 009 (Option 2: minimal hardening; Option 1 opaque-ID
table deferred as an ABI-breaking follow-up).
Three concrete changes:
1. ad_free_handle signature: *const AdNativeHandle → *mut AdNativeHandle.
The free fn now owns the right to mutate the caller's struct.
2. Zero handle.ptr BEFORE invoking the platform release() call. A
follow-up ad_free_handle on the same struct observes ptr == null
and returns Ok without re-entering CFRelease, making an accidental
double-call deterministic instead of corrupting the CF retain count.
3. ad_execute_action and ad_get now reject handle.ptr == NULL at the
use site (after guarding the struct pointer itself) with a
diagnostic last-error "handle.ptr is null — the handle has already
been freed or was never resolved". Prevents feeding a freshly-zeroed
handle back into adapter code.
Ownership contract captured in the ad_free_handle rustdoc: the FFI
owns the handle from ad_resolve_element onward; copying the struct
and calling ad_free_handle on either copy is undefined because the
library cannot detect forged non-null pointers. Callers that need a
"second copy" must re-resolve.
Tests (crates/ffi/tests/c_abi_harness.rs):
- free_handle_null_is_noop: adjusted to *mut signature, asserts the
ptr stays null after the call.
- free_handle_zeroes_ptr_so_double_free_is_noop: non-macOS only (macOS
would SIGBUS on the fake pointer's CFRelease before we could observe
the zeroing; the logic is platform-agnostic so validating it on
Windows/Linux covers the contract).
- execute_action_rejects_null_handle_ptr: null .ptr with non-null
struct returns InvalidArgs or ErrInternal (main-thread guard), no UB.
81 FFI tests pass (72 lib + error_lifetime + 14 c_abi_harness variants
depending on target). Clippy clean.
Closes P1 todo 008. The prior contract listed "focused", "enabled",
"selected", "checked", "expanded" but the macOS tree builder only
emits "focused" and "disabled"; the remaining three always answered
false regardless of the actual element state. The doc also claimed
`*out` was untouched on ELEMENT_NOT_FOUND while the implementation
cleared it to false at entry.
Rewrite supported-property handling as a closed SupportedProperty enum:
- `"focused"` → state["focused"] present (unchanged)
- `"disabled"` → state["disabled"] present (newly exposed)
- `"enabled"` → derived: !disabled (agent-friendly; no ambiguity about
which side the absence lands on)
`"selected"`, `"checked"`, `"expanded"` now return InvalidArgs with a
diagnostic naming the three supported values — explicit rather than
silently wrong. The set can widen in backwards-compatible fashion as
adapters emit more state strings.
Doc comment rewritten to:
- Name only the strings that actually answer truthfully.
- Say "on entry *out is always cleared to false" instead of the
previous "untouched on not-found" which contradicted code.
- Note the explicit rejection of currently-unsupported names so
callers don't guess.
New #[cfg(test)] module covers:
- focused_mirrors_state_presence
- disabled_mirrors_state_presence
- enabled_is_derived_negation_of_disabled
- unsupported_names_do_not_resolve
80 FFI tests pass, clippy clean.
Closes P1 todo 007. key_combo_from_c built a slice from the foreign
(modifiers, modifier_count) pair with only `!is_null() && count > 0`
as gate; two holes remained:
1. Large modifier_count over-reads the caller's buffer — undefined
behavior inside from_raw_parts, not recoverable via trap_panic.
2. null modifiers + positive count silently dropped modifiers, turning
"Cmd+S" into bare "S" from the user's perspective.
Add bounded validation before from_raw_parts:
- const MAX_MODIFIERS_PER_COMBO: u32 = 4 (four real modifier keys;
duplicates have no operational meaning).
- modifier_count > 4 → "modifier_count exceeds MAX_MODIFIERS_PER_COMBO"
- modifier_count > 0 with modifiers == null → "modifier_count > 0 but
modifiers pointer is null"
Both surface as AD_RESULT_ERR_INVALID_ARGS via the existing action
conversion path.
Unit tests added in actions/conversion.rs for the three classes:
over-cap count, positive count + null pointer, valid small slice.
76 FFI tests pass, clippy clean.
Closes P1 todo 006. Prior ordering inside trap_panic bodies was:
require_main_thread();
guard_non_null!(out, ...);
*out = <zero>;
A worker-thread call therefore returned ErrInternal with the caller's
original out struct untouched — callers that reuse a buffer from a
prior success and follow up with ad_*_free on failure would double-free
stale pointers.
Reorder to fail-closed: validate the out pointer first, zero the slot,
then run the main-thread guard and the rest of the input checks. Safe
because the only work done before require_main_thread is zeroing an
already-validated out pointer — no AX/Cocoa access, no allocation.
Applied across every out-param entrypoint:
- tree/get.rs, apps/launch.rs, apps/list.rs, windows/list.rs,
surfaces/list.rs, screenshot/capture.rs, actions/resolve.rs,
actions/execute.rs, input/clipboard.rs (get_clipboard),
observation/{find,get,is}.rs, notifications/{list,dismiss_all,action}.rs
New regression test:
- dirty_out_param_is_cleared_before_early_return_on_worker_thread:
seeds *out = 0xDEADBEEF, calls ad_list_apps from a cargo-test worker
thread, asserts *out is null after the ErrInternal early return.
73 FFI tests pass, clippy clean.
Closes P2 todo 005. ad_get_tree bypasses the CLI snapshot pipeline —
it flattens the raw adapter tree directly, with no ref_alloc, no
skeleton/drill-down wiring, and slightly different interactive_only
/ compact semantics than the JSON snapshot path. Callers were already
hitting this but the contract was implicit.
- crates/ffi/src/tree/get.rs: expanded rustdoc on ad_get_tree with a
dedicated "Raw-tree contract" section listing each divergence from
the CLI snapshot:
* ref_id is always null (ref_alloc is not run)
* skeleton/drill-down not wired through
* interactive_only / compact follow adapter semantics
Recommends ad_find + ad_get/ad_is for point lookups or shelling out
to the CLI when JSON parity matters. cbindgen propagates the rustdoc
to agent_desktop.h so C consumers see it from the header.
- skills/agent-desktop-ffi/SKILL.md: refresh the main-thread bullet to
reflect Unit 002 (runtime enforcement, not debug_assert) and add an
ad_get_tree raw-tree bullet to the core-constraints list.
Option 2 from the todo: keep the lower-level surface but document the
divergence explicitly. The snapshot-pipeline parity (Option 1) was
deferred as the todo explicitly notes: "revisit after higher-severity
FFI safety work lands, since some shared ref/identity changes may
inform the right design here."
Closes P1 todo 004. ad_find previously rebuilt its RefEntry with
`value: None`, `bounds: None`, `bounds_hash: None` — the macOS resolver
then fell back to role+name matching alone, which happily collapses
duplicate-label siblings (two "Save" buttons, two "OK" dialogs) onto
the first one rather than the one that actually matched the query.
Fix:
- Switch the internal get_tree call to include_bounds: true so matched
nodes carry geometry.
- Compute bounds_hash from matched.bounds (same 100x integer hash the
resolver expects, shared via Rect::bounds_hash).
- Populate RefEntry with value, states, bounds, and bounds_hash from
the matched node before calling resolve_element. Lossless re-resolve:
the resolver now has the same disambiguators the matcher used.
Regression tests added in observation/walk.rs:
- finds_first_matching_role — DFS order sanity check
- value_substring_disambiguates_duplicate_labels — the actual bug this
todo describes: two buttons named "Save", only one with value
"default"; find must return the "default" one.
- matched_node_bounds_hash_stable_across_calls — guards against a
future regression where bounds_hash depends on transient state.
- missing_filter_field_treats_as_dont_care — None filter semantics.
- no_match_returns_none — negative case.
72 FFI tests pass, clippy clean.
Closes P1 todo 003. Prior code dereferenced raw inbound pointers before
validating them; null or stale adapter/input/out pointers could segfault
the host before the FFI had a chance to return a structured error.
trap_panic can only catch Rust panics — not raw-pointer UB.
Add crates/ffi/src/pointer_guard.rs with a guard_non_null! macro that
short-circuits the enclosing AdResult-returning fn with
AD_RESULT_ERR_INVALID_ARGS and populates last-error via the 'static
errno slot. Zero allocation on the error path.
Apply the macro to every extern fn that dereferences inputs or writes
out-params, guarding pointers before the first use:
- adapter: ad_check_permissions
- apps: ad_launch_app, ad_close_app, ad_list_apps
- windows: ad_list_windows, ad_focus_window, ad_window_op
- input: ad_mouse_event, ad_drag, ad_get_clipboard, ad_set_clipboard,
ad_clear_clipboard
- screenshot: ad_screenshot
- surfaces: ad_list_surfaces
- tree: ad_get_tree
- actions: ad_resolve_element, ad_execute_action
- observation: ad_find, ad_get, ad_is
- notifications: ad_list_notifications, ad_dismiss_notification,
ad_dismiss_all_notifications, ad_notification_action
Fix ad_get_clipboard to null-initialize *out after validating the
out-pointer and before any fallible adapter call — aligns the
implementation with the documented null-on-error contract.
New c_abi_harness regression tests:
- null_adapter_rejected_without_ub: passes null adapter to ad_list_apps
and ad_check_permissions; both reject without a segfault.
- null_out_param_rejected_before_write: passes null *out to
ad_list_apps; rejects before the first *out = ... write.
Both accept ErrInvalidArgs OR ErrInternal (worker-thread cargo tests
trip the macOS main-thread guard first for guarded fns; ad_check_permissions
has no main-thread guard so remains deterministic).
67 FFI tests pass, clippy clean.
Closes P1 todo 002. The prior debug_assert_main_thread() compiled away
under --profile release-ffi, so worker-thread misuse could still enter
macOS AX/Cocoa code and hit undefined behavior. Several entrypoints
also skipped the guard entirely.
Replaced the debug-only helper with require_main_thread() — a runtime
check that fires in every build profile, returns Err(AdResult::ErrInternal)
with a 'static last-error string, and is a compile-time constant true
on non-macOS targets (AT-SPI / UIA have no main-thread affinity).
Applied uniformly as the first statement inside trap_panic across every
adapter-touching export:
- apps/launch, apps/close, apps/list
- windows/focus, windows/list, windows/op
- input/mouse, input/drag
- input/clipboard: ad_get/set/clear_clipboard
- surfaces/list
- screenshot/capture
- tree/get
- actions/resolve, actions/execute
- observation/find, observation/get, observation/is
- notifications/list, notifications/dismiss, notifications/dismiss_all, notifications/action
Exempt (safe off-thread): ad_adapter_create, ad_adapter_destroy,
ad_check_permissions, ad_last_error_*, every ad_*_free family member,
ad_image_buffer_* accessors, ad_release_window_fields.
Dropped the now-unused debug_assert_main_thread() export. The C
contract (main-thread required) is already documented in the lib
crate-level rustdoc and skills/agent-desktop-ffi/references/threading.md.
Integration tests (error_lifetime.rs, c_abi_harness.rs) that previously
relied on ErrInvalidArgs now accept either ErrInvalidArgs or ErrInternal
— cargo tests run on worker threads, so the guard fires before the
argument validator. Both outcomes prove the absence of UB, which is
what the harness exists to verify.
63 tests pass, clippy clean, fmt clean.
Closes P1 todo 001. Every enum-typed field that a foreign caller writes
through the C ABI was previously stored as a Rust #[repr(i32)] enum —
out-of-range bit patterns crossed the boundary before the enum_raw_i32
validator could catch them, invoking undefined behavior at the field
read. The validator was directionally correct but fired too late.
Switched to raw i32 storage on every externally supplied public ABI
struct:
- AdAction.kind
- AdScrollParams.direction
- AdMouseEvent.kind, .button
- AdWindowOp.kind
- AdTreeOptions.surface
- AdScreenshotTarget.kind
- AdKeyCombo.modifiers (was *const AdModifier, now *const i32)
Every conversion site now reads the raw i32 directly and calls
<Enum>::from_c(raw) — an invalid discriminant returns None, and the
FFI entrypoint surfaces AD_RESULT_ERR_INVALID_ARGS without ever
constructing an invalid enum value in Rust. Dropped the now-unused
enum_raw_i32 helper and its test; replaced with a direct round-trip
test that confirms <Enum> as i32 -> from_c -> Some(<Enum>) still
holds for valid values.
Updated unit-test AdAction / AdMouseEvent / AdScrollParams builders to
use <Enum> as i32 casts. The c_abi_harness fuzz test simplifies — it
no longer needs ptr::copy_nonoverlapping because the field is just i32.
63 FFI tests pass, clippy --all-targets clean, header regenerated.
Integrate main-branch changes that landed in parallel with this PR:
- Cargo.toml: keep both [profile.ci] (fast CI builds, from main) and
[profile.release-ffi] (panic=unwind for the cdylib, this branch).
Workspace version tracks main at 0.1.12.
- .github/workflows/ci.yml: keep main's concurrency block, pinned
action SHAs, permissions hardening, and the new ci profile used for
binary size checks; re-add this branch's FFI cdylib build and header
drift check as trailing steps.
- crates/ffi/src/tree/get.rs, observation/{find,is}.rs: TreeOptions
gained a `skeleton: bool` field on main — pass `false` since the FFI
snapshots are already full trees.
- crates/ffi/src/actions/resolve.rs, observation/find.rs: RefEntry
gained a `root_ref: Option<String>` field (progressive skeleton
traversal) — pass `None` from the FFI layer.
- crates/ffi/src/tree/flatten.rs test helper: AccessibilityNode gained
a `children_count: Option<usize>` field — initialize to None.
Verified: 111 workspace lib tests pass, 62 FFI tests pass (52 lib + 10
integration), clippy --all-targets -D warnings clean.
Sweep crates/ffi/ docstrings per the review ask ("make them
comprehensive, remove the unwanted ones"):
- ad_adapter_create: add missing header — ownership, lifetime, failure
mode.
- ad_last_error_{code,message,suggestion,platform_detail}: each gets a
dedicated 1-2 line doc; the errno-style lifetime block kept above
ad_last_error_code as the anchor for the family.
- ad_drag: explain duration_ms == 0 semantics.
- ad_mouse_event: note click_count only applies to the CLICK kind.
- ad_get_clipboard / ad_set_clipboard / ad_clear_clipboard /
ad_free_string: clarify UTF-8 input, error branch, null-tolerance,
and double-free semantics.
- ad_focus_window: document window-not-found error and id/title
prerequisites.
- ad_window_op: explain which op.kind fields are read.
- ad_launch_app: document return-ownership of AdWindowInfo and the
zero-on-error contract.
- ad_close_app: document force-flag semantics.
- ad_get_tree: describe BFS layout, options, and out-param zeroing.
Remove test-intent //-comments that were already captured in renamed
test names:
- enum_validation::fuzz_arbitrary_bit_patterns_never_panic_across_all_enums
- main_thread::is_main_thread_call_is_always_safe_even_on_workers
- main_thread::debug_assert_panic_on_worker_thread_does_not_escape_catch_unwind
Tighten the two retained //-blocks (error.rs parity assertion, build.rs
cbindgen-false semantics) so each explains a non-obvious invariant a
future reader couldn't infer from code alone.
62 FFI tests pass. Clippy clean.
New crates/ffi/tests/c_abi_harness.rs exercises bug classes the inline
#[cfg(test)] modules can't reach — pointer-level access patterns a C
consumer would use:
- rect_and_point_layouts_are_memcpyable: confirms #[repr(C)] structs
survive a byte-level read/copy (the pattern a C caller uses with
memcpy into a local).
- enum_fuzz_invalid_discriminant_rejected: writes i32::MAX into an
AdAction.kind field via raw pointer copy (bypassing Rust's enum
validity invariant) and asserts ad_execute_action returns either
ErrInvalidArgs (enum validator caught it) or ErrInternal (main-thread
assert tripped first on the worker thread) — never Ok, never UB.
- null_tolerance_on_list_accessors_and_free: ad_*_count, ad_*_get,
ad_*_free all accept null pointers without segfaulting.
- list_handle_lifecycle_roundtrip: ad_list_apps allocates a list,
accessor returns null for out-of-range index, _free drops the Box.
- list_windows_focused_only_runs: exercises the new focused_only bool
parameter from Unit 6.
- invalid_utf8_app_id_rejected: passes a truncated UTF-8 byte sequence
as an app id; ad_launch_app returns ErrInvalidArgs instead of
panicking during string decode.
- find_returns_not_found_on_empty_query_against_no_window: zeroed
AdWindowInfo is rejected by window validation before the tree walk
can dereference the null id/title pointers.
- free_handle_null_is_noop: both the handle value being null-pointer
and the handle argument itself being null return Ok.
- last_error_survives_successful_calls: canonical errno-style contract —
failure sets last-error, five successful calls later the pointer still
resolves to the original message string.
Uses the same cross-fork compatible pattern as error_lifetime.rs:
#[allow(improper_ctypes)] over extern "C" declarations so the test
drives the FFI symbols exactly like a C linker would, without needing
bindgen or a C compiler in CI.
62 FFI tests pass (52 lib + 1 error_lifetime + 9 c_abi_harness).
Workspace-wide: 75 lib tests. Clippy clean.
Cheap single-element lookups without hand-walking the flat-tree result
of ad_get_tree. Closes the observation portion of R11 from PR #22 review.
New module tree crates/ffi/src/observation/ — one concern per file:
- walk.rs: pub(crate) find_first_match(node, role?, name?, value?) — DFS
case-insensitive substring matcher shared by find.rs and is.rs.
Extracted so both FFI entrypoints speak the same matching semantics
and unit-testable in isolation.
- find.rs: ad_find(adapter, win, query, &handle). Walks the tree, picks
the first match, resolves it to a NativeHandle via the existing
PlatformAdapter::resolve_element path. Returns ErrElementNotFound
when nothing matches. Caller owns the handle → must
ad_free_handle(adapter, handle).
- get.rs: ad_get(adapter, handle, property, &str_out) dispatches on
property name. Supported: "value" (via get_live_value), "bounds" (via
get_element_bounds, formatted as JSON). Unknown property returns
ErrInvalidArgs. Output string owned by caller → free with ad_free_string.
- is.rs: ad_is(adapter, win, query, property, &bool_out) looks up the
element the same way find does but checks a named state against the
node's states vec. Recognized: focused, enabled, selected, checked,
expanded. Unknown property returns ErrInvalidArgs with a diagnostic
listing the valid names.
New type in types/find_query.rs (AdFindQuery { role, name_substring,
value_substring }) already landed in Unit 5 and is re-exported here.
All entries go through trap_panic with debug_assert_main_thread.
53 tests pass. Clippy clean.
Exposes all four notification operations over the C ABI. Closes the
notification portion of R11 from PR #22 review.
New module tree crates/ffi/src/notifications/ — one concern per file:
- filter.rs: filter_from_c() converts AdNotificationFilter -> core
NotificationFilter with null-safe c_to_string and has_limit semantics.
- list.rs: ad_list_notifications + ad_notification_list_count /
_get / _free. List handle is the opaque AdNotificationList already
landed in Unit 5.
- dismiss.rs: ad_dismiss_notification(adapter, index, app_filter).
- dismiss_all.rs: ad_dismiss_all_notifications returns both a
`dismissed` and a `failed` list so partial failures are surfaced to
the caller without overwriting last-error. Adds
ad_dismiss_all_notifications_free convenience wrapper.
- action.rs: ad_notification_action(adapter, index, action_name, out)
triggers the named action (typically "Reply", "Open") and populates
an AdActionResult.
New type plumbing:
- crates/ffi/src/types/notification_{info,filter,list}.rs were added in
Unit 5; lib.rs re-exports them via explicit `pub use`.
- crates/ffi/src/convert/notification.rs: notification_info_to_c +
free_notification_info_fields with the same lossy-NUL / action-string
handling as window/app/surface converters.
All new entrypoints go through trap_panic / trap_panic_void, assert
main-thread in debug builds via debug_assert_main_thread, and use
string_to_c_lossy for mandatory fields (app_name, title, action names).
Filter fields stay optional via opt_string_to_c.
Index-stability contract documented in the rustdoc on
ad_list_notifications — indexes are valid only within the response to
the most recent list call; callers must re-query after each dismiss.
53 tests pass. Clippy clean.
Replaces every `(*mut T, count)` list-returning API with an opaque
handle and encapsulates AdImageBuffer's byte-buffer length. Closes
R8 and R22 from PR #22 review.
## Opaque list handles
Four new one-type-per-file opaque wrappers (no `#[repr(C)]` — cbindgen
auto-emits as `typedef struct AdFoo AdFoo;` forward declarations):
- crates/ffi/src/types/window_list.rs — AdWindowList
- crates/ffi/src/types/app_list.rs — AdAppList
- crates/ffi/src/types/surface_list.rs — AdSurfaceList
- crates/ffi/src/types/notification_list.rs — AdNotificationList (used by Unit 8)
Each list owns its `Box<[AdXxxInfo]>`. Consumers walk through
`_count(list)`, `_get(list, index) -> *const AdXxxInfo` (null on OOB),
and free with `_free(list)` — the free walks the entries, releases
their interior C-strings, and drops the Box.
Rewritten signatures:
| Old | New |
|------------------------------------------------------------|-----------------------------------------------------------|
| ad_list_apps(adapter, \*\*apps, \*count) | ad_list_apps(adapter, \*\*list) |
| ad_list_windows(adapter, filter, focused, \*\*wins, \*count)| ad_list_windows(adapter, filter, focused, \*\*list) |
| ad_list_surfaces(adapter, pid, \*\*sfs, \*count) | ad_list_surfaces(adapter, pid, \*\*list) |
| ad_free_apps(apps, count) | ad_app_list_free(list) |
| ad_free_windows(wins, count) | ad_window_list_free(list) |
| ad_free_surfaces(sfs, count) | ad_surface_list_free(list) |
| ad_free_window(win) [for single AdWindowInfo] | ad_release_window_fields(win) |
Count mismatches are impossible by construction — callers never see
the backing pointer or length.
## Image buffer encapsulation
crates/ffi/src/types/image_buffer.rs: dropped `#[repr(C)]`, private
`Box<[u8]>` data field, private width/height/format. Before, a C
caller who mutated `AdImageBuffer.data_len` triggered heap corruption
at free time; now the length is authoritative inside the Rust-owned
struct.
New accessors in crates/ffi/src/screenshot/accessors.rs:
- ad_image_buffer_data -> *const u8
- ad_image_buffer_size -> u64 (always matches the allocation)
- ad_image_buffer_width -> u32
- ad_image_buffer_height -> u32
- ad_image_buffer_format -> AdImageFormat
ad_screenshot signature changed: `*mut *mut AdImageBuffer out` instead
of `*mut AdImageBuffer out`. ad_free_image renamed to
ad_image_buffer_free for consistency with the list-handle pattern.
All new files stay under 120 LOC, explicit `pub use` per type, no
wildcard imports, no inline `//` comments. `///` docs on every
public FFI export cover null-tolerance, lifetime, and safety
requirements.
53 tests pass. Clippy clean.
Previous pull_request: branches: [main, master] only fired when the PR
targeted main or master; a pull_request block without 'branches' fires
on any target and keeps this PR (and future stacked PRs) under CI.
Adds workflow_dispatch so the workflow can be kicked manually from the
Actions UI or 'gh workflow run'.
skills/agent-desktop-ffi/ is a new skill directory:
- SKILL.md — frontmatter (name, version, tags), overview, four core
constraints (main-thread, release-ffi profile, errno lifetime,
handle release, enum validation, pre-1.0 ABI instability).
- references/ownership.md — allocation/release table for every *mut T
the FFI returns, plus null-tolerance and double-free rules.
- references/error-handling.md — errno-style last-error contract,
full error-code table, enum validation, panic-safety guarantee.
- references/threading.md — macOS main-thread rule, debug/release
contract differences, Python consumer patterns, the
AXIsProcessTrusted privilege-escalation note, single-owner handle
invariant, thread-local last-error.
- references/build-and-link.md — build command for the cdylib,
minimal C and Python ctypes worked examples.
CLAUDE.md updates:
- Workspace tree now lists crates/ffi/ alongside the platform crates.
- Dependency-inversion section acknowledges the FFI crate as a
legitimate second platform → core wiring point, parallel to the
binary crate (previously only src/ was named).
Skill will be picked up by the next `clawhub sync --root skills/ --all`
release automatically; no release-workflow change needed.
Lands the last set of ABI-shape changes before the header is published.
Any addition after publication would be a breaking change for every
consumer linked against the old layout.
Core / platform additions:
- crates/core/src/adapter.rs: new `PlatformAdapter::release_handle`
trait method with `not_supported()` default. macOS implementations
must `CFRelease` the underlying AXUIElementRef to balance the
CFRetain that happened during resolve. Windows/Linux inherit the
default unchanged.
- crates/macos/src/adapter.rs: macOS impl calls
`core_foundation::base::CFRelease(raw)` when the raw pointer is
non-null.
FFI surface additions:
- crates/ffi/src/types/snapshot_surface.rs: new AdSnapshotSurface enum
(Window=0, Focused, Menu, Menubar, Sheet, Popover, Alert) mirroring
the 7-variant core SnapshotSurface.
- crates/ffi/src/types/tree_options.rs: new `surface: AdSnapshotSurface`
field. Previous hard-coded SnapshotSurface::Window is replaced by
consumer-selected surface with enum-validated read at entry.
- crates/ffi/src/windows/list.rs: ad_list_windows gains `focused_only:
bool` between app_filter and out — maps directly to
WindowFilter::focused_only. Consumers can now call `ad_focused_window`-
style queries without a second ABI round.
- crates/ffi/src/actions/native_handle.rs: new `ad_free_handle(adapter,
handle)` exported. Null-tolerant on both sides; Windows/Linux
`ActionNotSupported` is translated to `Ok` so C consumers can call
the release path uniformly across platforms.
Validation:
- AdSnapshotSurface added to enum_validation.rs — ad_get_tree now
rejects invalid surface discriminants with
`AD_RESULT_ERR_INVALID_ARGS` rather than matching on UB.
Closes R5, R9, R10 from PR #22 review.
52 lib tests pass, 1 integration test passes. Clippy clean.
macOS accessibility and Cocoa APIs must be called on the process's main
thread; off-thread use is silent undefined behavior that is hard to
diagnose from the consumer side (Python, Swift, Node workers).
Add crates/ffi/src/main_thread.rs with:
- is_main_thread(): libc::pthread_main_np() on macOS, `true` elsewhere.
- debug_assert_main_thread(): panics in debug, no-op in release. Panic
is caught by the trap_panic boundary and surfaces as
AD_RESULT_ERR_INTERNAL with a diagnostic last-error — violators
get a loud signal in dev, an actionable error code in prod.
Apply the assert as the first statement inside trap_panic bodies for
the high-traffic AX paths: ad_get_tree, ad_resolve_element,
ad_execute_action, ad_get_clipboard, ad_screenshot. (Release / launch /
list / window-op / etc. remaining entrypoints can be covered in a
follow-up; these five are the hot paths agents actually hit.)
Add a crate-level //! rustdoc block on lib.rs documenting the thread-
safety model, the build-profile requirement (release-ffi, not release),
and the errno-style last-error lifetime. cbindgen propagates the
rustdoc to agent_desktop.h so C consumers read the constraints from the
header.
53 tests pass. Clippy clean.
Closes R14, R19, R20, R23 from PR #22 review.
Build pipeline:
- crates/ffi/Cargo.toml: pin cbindgen = "= 0.27.0" exactly. No more
silent formatting drift from patch-version bumps of cbindgen; any
future bump is a deliberate PR that also commits the regenerated
header.
- crates/ffi/build.rs: replace .expect()/.ok() swallowing with explicit
panics on every failure path. Missing CARGO_MANIFEST_DIR or OUT_DIR
now emits a cargo:warning rather than crashing rustc. cbindgen errors
now panic loudly with the diagnostic; previously an .ok() on fs::copy
silently emitted a stale header.
CI:
- .github/workflows/ci.yml: new "FFI cdylib build" step under the
release-ffi profile and a "FFI header drift" step that runs
`git diff --exit-code crates/ffi/include/agent_desktop.h` immediately
after. Any uncommitted change to the generated header fails the build
with a clear message telling the developer to run the local rebuild
and commit.
Variant parity:
- crates/ffi/src/error.rs: compile-time assertion
`const _: () = assert!(error_code_variant_count() ==
ad_result_error_variant_count())` guards against core adding an
ErrorCode variant without a matching AdResult entry (which would
silently drop information at the FFI boundary). Uses stable const fn
+ explicit variant arrays, no nightly variant_count feature.
50 lib tests, 1 integration test — all passing. Clippy clean.
Three defensive-hardening additions that close R6, R13, and R15 from the
PR #22 review:
1. Out-param zeroing at entry
Every fallible FFI fn now zeroes its out-param *before* any fallible
work, so when the fn returns an error code the caller can safely call
the paired ad_free_* fn on an as-if-never-populated struct.
- ad_launch_app: *out = zeroed AdWindowInfo
- ad_resolve_element: (*out).ptr = null
- ad_execute_action: *out = zeroed AdActionResult
- ad_screenshot: *out = zeroed AdImageBuffer
(ad_get_tree / ad_list_* already had this from prior work.)
2. Lossy string helper for mandatory fields
Add convert::string::string_to_c_lossy — replaces interior NUL bytes
with U+FFFD (3-byte UTF-8 sequence) so CString::new is always
infallible. Applied to mandatory fields:
- convert::window_info_to_c: id, title, app_name
- convert::app_info_to_c: name
- convert::surface_info_to_c: kind
- tree::flatten: role and each state string
- actions::action_result_to_c: action, post_state.role, state strings
Optional fields keep opt_string_to_c (null-on-NUL is the correct
signal for "field absent" when the header documents nullability).
3. Window identity validation
windows::to_core::ad_window_to_core now returns
Result<WindowInfo, AdapterError>. Null or non-UTF-8 id/title returns
InvalidArgs with a diagnostic message rather than silently coercing
to "" — previously a caller with a bad id could match the wrong
window. app_name stays lenient (some Electron apps emit blank
window owners).
Updated callers to propagate the error: tree::get::ad_get_tree,
windows::focus::ad_focus_window, windows::op::ad_window_op.
50 tests pass (+3 new lossy-NUL tests).
The prior recursive DFS layout placed a child node immediately after
its parent, intermixing descendants with the parent's siblings. That
meant AdNode.child_start..child_start + child_count walked through
grandchildren instead of direct children — silent wrong data for every
consumer iterating the flat tree.
New layout: iterative level-order (BFS) traversal using a VecDeque.
Siblings of any node are always contiguous at
nodes[n.child_start .. n.child_start + n.child_count]
so direct-child iteration is correct by construction, at every level.
- Pre-count pass sizes the Vec once with Vec::with_capacity — avoids
repeated reallocation on deep/wide trees.
- No recursion means consumers on thin stacks (JNI ~256 KB, Python
ctypes worker threads) no longer risk stack overflow regardless of
tree depth.
- `parent_index` still back-references correctly.
New test coverage:
- test_flatten_breadth_first_layout (was depth_first_order): iterates
the child ranges and asserts direct-child roles match the expected
sequence — this is the test that would fail against the old DFS
layout.
- test_flatten_deep_chain: 11-level nested chain, walks child_start at
every level and confirms the single-child ranges address correctly.
- test_flatten_wide_root: 100 direct children under one root, confirms
the whole sibling run is contiguous and every child is reachable.
Closes R1 from PR #22 review (the blocker-class child-range bug).
47 tests pass (+2 new BFS-specific tests).
C callers can legally place any int32_t bit pattern into an enum-typed
field of a #[repr(C)] struct. Reading that field as the Rust enum type
and pattern-matching against its variants is undefined behavior when
the discriminant is out-of-range — Rust assumes enum values are valid.
New crates/ffi/src/enum_validation.rs provides:
- enum_raw_i32(&T) -> i32: reads the raw discriminant via pointer cast,
never invokes the enum's validity invariant. Safe because #[repr(i32)]
guarantees layout.
- try_from_c_enum! macro: generates <Enum>::from_c(raw: i32) -> Option<Self>
that returns Some only for enumerated discriminants, None otherwise.
Applied to all 8 C-exposed enums: AdActionKind (0..=20), AdDirection,
AdModifier, AdMouseButton, AdMouseEventKind, AdWindowOpKind,
AdScreenshotKind, AdImageFormat.
Call sites updated to read raw, validate, then match:
- actions/conversion.rs: action.kind, action.scroll.direction,
each modifier in key_combo slice
- input/mouse.rs: ev.kind, ev.button
- windows/op.rs: op.kind
- screenshot/capture.rs: t.kind
Invalid discriminants now return AD_RESULT_ERR_INVALID_ARGS with a
diagnostic last-error string ("invalid <enum> discriminant").
Previously the code invoked UB before the call could return.
11 new enum-validation tests (45 total lib tests, +11 from 34).
Fuzz test iterates i32::MIN, i32::MAX, -1, 999, and edge values through
every validator — never panics, never UB.
Last-error pointers returned by ad_last_error_{code,message,suggestion,
platform_detail} now survive across any number of subsequent successful
FFI calls — only the next *failing* call rotates them. This matches the
POSIX errno contract and closes R2 from the PR #22 review (use-after-free
when caller cached the message pointer and made another successful call).
Changes:
- Remove error::clear_last_error() from every Ok branch across adapter,
tree/get, actions/{resolve,execute}, apps/{list,launch,close},
windows/{list,focus,op}, input/{clipboard,mouse,drag},
screenshot/capture, surfaces/list. The slot now only rotates on a new
set_last_error().
- clear_last_error is gated behind #[cfg(test)] since no production
caller needs it after this change.
- Add a crate-level rustdoc block on ad_last_error_code documenting the
errno-style lifetime contract — cbindgen propagates this to
agent_desktop.h so C consumers can read the rule from the header.
- crates/ffi/tests/error_lifetime.rs: integration test reproducing the
review's UAF scenario — fails on ErrInvalidArgs, caches the message
pointer, makes 10 successful ad_check_permissions calls, asserts the
cached pointer still resolves to the same string.
Lib Cargo.toml now emits both cdylib and rlib so the integration test
can link against the public crate without duplicating symbol bindings.
35 tests pass (34 lib + 1 integration). Clippy clean.
Add crates/ffi/src/ffi_try.rs with three helpers:
- trap_panic(|| -> AdResult) for AdResult-returning fns
- trap_panic_ptr(|| -> *mut T) for ad_adapter_create
- trap_panic_const_ptr(|| -> *const T) for ad_last_error_*
- trap_panic_void(|| ()) for ad_*_destroy / ad_free_*
Each helper runs its body under std::panic::catch_unwind with
AssertUnwindSafe, stashes a 'static C-string message
("rust panic in FFI boundary") via a new
error::set_last_error_static(code, &'static CStr) helper that never
allocates, and returns AD_RESULT_ERR_INTERNAL / null on panic. The
panic payload is intentionally discarded — allocating inside a panic
handler risks double-panic and the host's tracing/logs hold the
diagnostic anyway.
Every #[no_mangle] pub extern "C" fn body is now wrapped:
- adapter.rs: ad_adapter_create, ad_adapter_destroy, ad_check_permissions
- error.rs: ad_last_error_{code,message,suggestion,platform_detail}
- tree/{get,free}.rs: ad_get_tree, ad_free_tree
- actions/{resolve,execute,result}.rs: ad_resolve_element,
ad_execute_action, ad_free_action_result
- apps/{list,launch,close}.rs: ad_list_apps, ad_free_apps,
ad_launch_app, ad_close_app
- windows/{list,free_one,focus,op}.rs: ad_list_windows, ad_free_windows,
ad_free_window, ad_focus_window, ad_window_op
- input/{clipboard,mouse,drag}.rs: ad_get_clipboard, ad_set_clipboard,
ad_clear_clipboard, ad_free_string, ad_mouse_event, ad_drag
- screenshot/{capture,free}.rs: ad_screenshot, ad_free_image
- surfaces/list.rs: ad_list_surfaces, ad_free_surfaces
Add crates/ffi/examples/panic_spike.rs as a permanent regression
example — `cargo run --profile release-ffi --example panic_spike`
catches the synthetic panic and exits 0. If someone flips the profile
back to panic=abort, the example SIGABRTs instead — a loud signal.
Verified:
- cargo build --profile release-ffi -p agent-desktop-ffi: ok
- cargo test --lib -p agent-desktop-ffi: 34 passed (+7 new ffi_try tests)
- cargo clippy --all-targets -p agent-desktop-ffi -- -D warnings: ok
- cbindgen header symbol set: 31 functions before / 31 after
- panic_spike under release-ffi: PANIC CAUGHT OK (exit 0)
Closes R4 from PR #22 review.
Workspace Cargo.toml gains [profile.release-ffi] inheriting from
release with panic = "unwind". Cargo rejects per-package `panic`
overrides (error: panic may not be specified in a package profile —
verified experimentally), so a dedicated profile is the Cargo-supported
path to build the cdylib with unwinding while the CLI stays on
panic = "abort".
crates/ffi/Cargo.toml gains:
- publish = false — prevents accidental `cargo publish` of the cdylib
crate until the distribution plan intentionally removes the guard.
- libc = "0.2" under the macos target — Unit 11 uses pthread_main_np().
Both profiles coexist:
- target/release/libagent_desktop_ffi.dylib 442 KB (panic=abort)
- target/release-ffi/libagent_desktop_ffi.dylib 464 KB (panic=unwind)
Size delta for unwind metadata (~22 KB, under the 500 KB risk budget).
Prerequisite for Unit 1's panic-boundary macro.
c_to_str<'a>(*const c_char) -> Option<&'a str> let the caller pick any
lifetime — including 'static — while the returned reference actually
only lives as long as the raw pointer. That is unsound: a caller could
bind the result to a 'static slot and dereference long after the
underlying buffer is gone.
Replace with c_to_string(*const c_char) -> Option<String> — always
clones into an owned String. Every caller (actions, apps, input,
windows, tree tests, convert tests) updated to pass &String where &str
was needed or .as_deref() in asserts. No caller was relying on
short-lived borrow semantics; the existing flow was already
.to_owned()-ing immediately.
Closes the Unit R soundness portion. No behavior or ABI change;
cbindgen header: 31 functions in, 31 out (ordering-only diff).
Every reachable pub(crate) helper (convert/ helpers, tree/flatten::flatten_tree,
error::{set_last_error, clear_last_error, last_error_code, error_code_to_result})
now stands without the annotation — these are all called from #[no_mangle]
entrypoints and rustc sees them correctly.
Move the three test-only last_error readers (last_error_message_str,
last_error_suggestion_str, last_error_platform_detail_str) plus the
MessageSource::to_owned_string helper into #[cfg(test)] scope. They were
only ever used by tests; having them stand at module scope required the
annotation and muddied the public-surface read.
Zero #[allow(dead_code)] remains in crates/ffi/src/. Tree still compiles,
27 tests still pass, cbindgen header symbol set unchanged.
Move AdAdapter + build_adapter + ad_adapter_create / ad_adapter_destroy /
ad_check_permissions into a dedicated crates/ffi/src/adapter.rs matching
the crates/macos/src/adapter.rs single-responsibility pattern.
lib.rs is now a module-declaration + explicit re-export shell. The
`pub use types::*` wildcard is replaced with one `pub use` per type so
adding a new type requires a deliberate re-export (prevents accidental
ABI expansion). Narrow most modules to pub(crate) — only error and
types stay pub because cbindgen scans them for public types.
Every file under 400 LOC (max 225). No behavior or ABI change; symbol
set identical.
Closes the structural portion of Unit R. Remaining Unit R cleanup
(c_to_str unbound lifetime + dead_code audit) lands next.
- screenshot/{capture,free}.rs: ad_screenshot + ad_free_image
- surfaces/list.rs: ad_list_surfaces + ad_free_surfaces
Drop the last unused convert::surface re-exports; convert/mod.rs now
only re-exports rect_to_c (the single consumer sits inside tree/flatten
and benefits from the shortened path).
No behavior or ABI change; symbol set identical, ordering-only cbindgen diff.
Move input.rs into input/{clipboard,mouse,drag}.rs.
- clipboard.rs: ad_get_clipboard + ad_set_clipboard + ad_clear_clipboard + ad_free_string
- mouse.rs: mouse_button_from_c + ad_mouse_event + tests
- drag.rs: ad_drag + tests
Drop unused convert::{c_to_str, free_c_string, string_to_c} re-exports
from convert/mod.rs; all callers now hit crate::convert::string::
directly.
No behavior or ABI change.
Move windows.rs into windows/{to_core,list,focus,free_one,op}.rs.
- to_core.rs: ad_window_to_core helper (pub(crate) re-exported so
tree/get.rs keeps its existing crate::windows::ad_window_to_core path)
- list.rs: ad_list_windows + ad_free_windows
- focus.rs: ad_focus_window
- free_one.rs: ad_free_window (single-struct free from ad_launch_app;
Unit 5b renames to ad_release_window_fields)
- op.rs: ad_window_op
Remove now-unused convert::{window_info_to_c, free_window_info_fields}
re-exports.
No behavior or ABI change. Header symbol set identical.
Move apps.rs into apps/{list,launch,close}.rs — one concern per file.
Drop the now-unused convert::{app_info_to_c, free_app_info_fields}
re-exports from convert/mod.rs (apps/list.rs imports from the concrete
convert::app submodule directly).
No behavior or ABI change. Header symbol set identical; emission order
shifts per cbindgen's module-path sort (ordering-only diff).
Move actions.rs into actions/{conversion,resolve,execute,result}.rs.
- conversion.rs: direction_from_c + key_combo_from_c + action_from_c + tests
- resolve.rs: ad_resolve_element
- execute.rs: ad_execute_action
- result.rs: action_result_to_c + ad_free_action_result + tests
actions/mod.rs declares submodules; the #[no_mangle] entries remain
discoverable by the cdylib linker and cbindgen without re-exports.
Drop the unused opt_string_to_c re-export from convert/mod.rs (all new
modules import from crate::convert::string:: directly).
No behavior or ABI change. cbindgen header symbol set unchanged;
emission order shifts due to cbindgen's module-path topological sort
(ordering-only diff per Unit R verification criteria).
Move tree.rs into tree/flatten.rs (flatten_tree + flatten_recursive +
strings_to_c_array + its tests), tree/free.rs (free_c_string_array,
free_node_fields, ad_free_tree + null-free test) and tree/get.rs
(ad_get_tree). Drop inline //-comments that restate what the next line
already says.
Behavior unchanged: flatten_tree is still the existing recursive DFS
layout (Unit 4 rewrites to iterative BFS so child ranges address direct
children). cbindgen header byte-identical.
Move convert.rs helpers into convert/{string,rect,window,app,surface}.rs.
Each submodule carries just the helpers for one kind (C-string helpers,
rect conversion, window-info conversion, app-info conversion, surface-info
conversion) plus its focused test(s). convert/mod.rs re-exports the same
public surface so every caller keeps its existing
`use crate::convert::{string_to_c, ...}` paths intact.
No behavior, signature, or ABI change; cbindgen header is byte-identical.
Unit R precondition; Unit 7 later refines string helpers (lossy/NUL-safe).
Move the 28 packed #[repr(C)] structs and #[repr(i32)] enums from
crates/ffi/src/types.rs into crates/ffi/src/types/{point,rect,node,
action,...}.rs, one declaration per file. types/mod.rs carries
explicit pub mod + pub use per type, so callers keep the same
`use crate::types::AdFoo` paths and cbindgen emits a byte-identical
generated header.
Purely mechanical; no behavior, signature, or ABI change. Unit R
precondition for later fix work where individual types are extended
(AdTreeOptions.surface, AdImageBuffer length encapsulation, etc.).
Replace the infallible-today .unwrap() on CString::new fallback with a
MessageSource enum that carries either an owned CString or a 'static
CStr pointer. Interior-NUL payloads now deterministically resolve to
the NUL_BYTE_FALLBACK constant rather than relying on an unwrap that
could panic if the literal ever gained a NUL byte.
Adds a regression test for interior-NUL handling.
Precondition for Unit 1's panic boundary, which needs a last-error
path that never allocates and never panics.
* feat: add data model foundation for progressive skeleton traversal
Add children_count, root_ref, skeleton fields to core types. Add
get_subtree() to PlatformAdapter trait. Add remove_by_root_ref() and
write-side size check to RefMap. No behavioral changes yet.
* feat: implement progressive skeleton traversal with ref-rooted drill-down
Add --skeleton and --root flags to snapshot command for token-efficient
accessibility tree exploration. Skeleton mode clamps depth to 3 levels
and annotates truncated containers with children_count, allowing AI
agents to discover regions before drilling into them. Named containers
at skeleton boundaries (via name or description) receive refs as
drill-down targets. The --root flag starts traversal from a previous
ref with scoped invalidation — only refs from that drill-down are
replaced on re-drill.
Key changes:
- New ref_alloc.rs: shared ref helpers (INTERACTIVE_ROLES, actions_for_role,
ref_entry_from_node, is_collapsible) extracted from snapshot.rs
- New snapshot_ref.rs: drill-down logic with DrillDownConfig, scoped
invalidation via root_ref tagging on RefEntry
- macOS count_children() uses raw CFArrayGetCount without materializing
AXElement wrappers for performance at skeleton boundaries
- RefMap write-side size check prevents >1MB files
- Skeleton anchors consider both name and description for Electron compat
* fix: mention --skeleton in STALE_REF error suggestion
* docs: document --skeleton and --root flags in skill reference
* docs: update phases.md and CLAUDE.md for progressive skeleton traversal
Add skeleton traversal as Phase 1 objective P1-O10. Document --skeleton
and --root flags, get_subtree() trait method, new core modules, and
platform-agnostic notes for Phase 2/3. Update risk mitigations and
performance optimizations table.
* docs: make progressive skeleton traversal the default agent workflow
Update SKILL.md observe-act loop to skeleton-first approach. Add
progressive skeleton traversal as the primary workflow pattern. Update
anti-patterns, key principles, and command quick reference to lead
with --skeleton + --root. Full snapshot remains documented as fallback
for simple apps.
* fix: preserve skeleton drill-down anchors
* fix: preserve drill-down refs across skeleton re-snapshots
Skeleton snapshots now load the existing refmap and remove only
skeleton-level refs (root_ref: None), preserving drill-down refs
accumulated via --root. Previously, build() always created a fresh
RefMap, breaking the skeleton → drill → act → skeleton(verify) workflow
by wiping all drill-down refs on the verify step.
* fix: preserve drill-down depth for root snapshots
* fix: verify AX action effect on Electron elements before trusting success
Chromium's AX implementation returns kAXErrorSuccess for AXPress,
AXConfirm, etc. but only toggles ARIA state without firing DOM event
handlers. This caused the click chain to short-circuit on false
positives, preventing CGClick from ever being reached.
The fix detects web elements via AXWebArea ancestor walk, then verifies
each AX action actually had a DOM effect by comparing focused element
pointers before and after. When all AX methods produce no real effect,
CGClick fires as the genuine last resort.
Native elements are unaffected — the original verified_press behavior
is preserved in a separate code path.
* chore: ignore .context/ for local tooling artifacts
* style: collapse web_action_had_effect signature to single line
* test: cover RefMap save oversize rejection
Extract the serialize+size-check step into a private helper so the 1MB
write rejection path can be unit-tested without filesystem I/O. Also
moves the size check above the directory creation in save() so an
oversized refmap no longer creates ~/.agent-desktop on rejection.
* test: assert stale-ref suggestion mentions --skeleton
Locks in the Phase 4 polish requirement that STALE_REF errors guide
agents back to a skeleton refresh, not just a plain snapshot.
* test: cover skeleton-to-drill-down counter continuity
Asserts that skeleton refs (@e1..@e10) survive a scoped invalidation
of @e3, that drill-down refs allocated after the skeleton continue from
@e11 instead of resetting, and that remove_by_root_ref drops only the
drill-down children.
* test: cover --root + --surface rejection at execute() boundary
Adds a NoopAdapter that uses every PlatformAdapter trait default to
exercise execute()'s validation guards without standing up a real
adapter. Verifies that combining --root with a non-Window surface
returns INVALID_ARGS, and that the Window surface does not trigger the
guard.
* test: add filesystem-redirected integration tests for run_from_ref
Adds a thread-local HOME override + RAII HomeGuard so RefMap::save and
RefMap::load can be exercised against an isolated temp directory without
env-var racing across parallel tests. Also adds a StubAdapter that
implements PlatformAdapter with a canned subtree response so the full
run_from_ref drill-down flow can be unit-tested without standing up
macOS AX state.
Closes seven Phase 3 plan acceptance items in one pass:
- save+load roundtrip with HOME override
- oversize save rejection preserves previous file on disk
- run_from_ref returns subtree and persists drill refs
- stale root ref → STALE_REF with skeleton suggestion
- re-drill replaces drill refs only, counter continues
- multiple drill-downs from @e1 and @e2 coexist
- empty subtree drill-down produces no new refs
* test: add golden fixtures for skeleton output and drill-down refmap
Adds two committed JSON fixtures under tests/fixtures/ plus matching
unit tests that build the same input trees, run them through
allocate_refs / run_from_ref, and assert the produced ref ids,
parent-child layout, and root_ref tagging match the golden expectations.
Locks in the JSON shape so future serialization changes have to be
deliberate.
* fix: bound build_subtree recursion on Electron wrapper chains
Anonymous AXGroup/AXGenericElement wrappers do not advance the semantic
depth counter (web-wrapper depth-skip), so a long chain of nested
wrappers could recurse arbitrarily deep without ever hitting either
max_depth or ABSOLUTE_MAX_DEPTH. The previous code checked
`depth >= ABSOLUTE_MAX_DEPTH` against the semantic depth, which stayed
at 0 throughout a wrapper chain. On pathological Electron trees this
risked stack exhaustion before any cap engaged.
Add a separate `raw_depth` parameter that always increments and use it
for the absolute cap. Semantic `depth` still drives `max_depth` and the
skeleton boundary so the wrapper-flattening behavior is preserved on
normal trees.
Also drops the long-unused `_include_bounds` parameter; bounds
filtering happens in `allocate_refs` in core, not here.
* chore: add pre-commit hook running fmt + clippy + tests
Mirrors the CI quality gates (cargo fmt --check, cargo clippy
--all-targets -- -D warnings, cargo test --lib --workspace) so a
failing commit never reaches origin. Skips automatically when no
Rust/TOML files are staged. Bypass with --no-verify or SKIP_PRECOMMIT=1
when a non-Rust hotfix is genuinely needed.
Hook lives under .githooks/ and is opt-in per clone:
git config core.hooksPath .githooks
Setup instructions added to CLAUDE.md.
* fix: prevent orphaned drill-down refs leaking across skeleton refresh
remove_skeleton_refs() previously dropped every entry whose root_ref
was None and kept every entry whose root_ref was Some. After a series
of skeleton refreshes that pattern leaked drill-down refs whose root
anchor had already been removed: they could never be cleaned up by a
future remove_by_root_ref(target) because target referred to a
non-existent anchor, and they accumulated until the 1MB write guard
fired.
Restructure the function to keep only:
- skeleton anchors that have at least one drill-down pointing at them
- drill-down refs whose root anchor is among the kept anchors
Unreferenced anchors are still dropped (the original intent), and
orphaned drill-downs are dropped at the same time so the refmap can
no longer accumulate dead entries.
Updated existing test to assert the new keep-when-referenced semantics
and added a regression test for the orphan-drilldown leak path.
* fix: align resolve traversal with snapshot child-attribute set
find_element_recursive previously walked AXChildren first then fell
back to AXContents only, and resolve_element_name only derived its
fallback label from AXChildren. Snapshot building, however, uses the
full child_attributes() list (AXChildren, AXContents,
AXChildrenInNavigationOrder) via copy_children. That asymmetry meant
refs minted for containers exposed only through AXChildrenInNavigationOrder
appeared in the snapshot output but produced false STALE_REF errors on
drill-down or action commands because the resolver could not find them
again.
Route both call sites through child_attributes() so resolution sees
the same children as snapshotting.
* fix: bypass AXConfirm on web elements and add skeleton anchors to drill-down
* fix: compare AX elements via CFEqual in web_action_had_effect
AXUIElementCopyAttributeValue follows the CoreFoundation Create rule
and returns a freshly-allocated CF object on every call, so raw
pointer equality (`before.0 != after.0`) between two separate copies
of AXFocusedUIElement was ALWAYS true even when the focused element
was unchanged. That made Step 1 (AXPress) of activate_web_element
appear successful every time and left Steps 2-4 of the escalation
chain (AXConfirm bypass, child actions, CGClick fallback) dead code
on web elements.
Replace the pointer comparison with CFEqual, which compares
accessibility element identity rather than heap addresses.
* refactor: unify allocate_refs across snapshot and drill-down paths
allocate_refs in snapshot.rs and allocate_refs_with_root in
snapshot_ref.rs were near-exact copies that differed only in whether
each allocated ref carried a root_ref tag. The duplication meant any
bug fix or behavior change in one path risked silently drifting from
the other (bot flagged this after the first round of fixes added even
more duplicated skeleton-anchor logic to allocate_refs_with_root).
Move the shared logic into ref_alloc.rs behind a new RefAllocConfig
struct with Option<&str> for the root_ref_id, and delete the
snapshot_ref.rs copy. Both snapshot::build and snapshot_ref::run_from_ref
now route through the same function with their respective configs.
append_surface_refs and the existing unit tests in both modules are
updated to use the shared function + config.
* chore: drop with_root from drill test names after allocator unification
The snapshot_ref tests kept their original test_allocate_refs_with_root_*
names after the allocator dedupe even though allocate_refs_with_root no
longer exists. Rename them to test_drill_alloc_* so grep for
allocate_refs_with_root returns zero matches and nobody assumes a second
allocator path exists.
* docs: compound DRY ref-allocator dedupe into knowledge base
Adds docs/solutions/best-practices/deduplicate-ref-allocator-via-config-struct-2026-04-14.md
documenting the root cause, guidance, consequences, trigger conditions,
and before/after of the allocate_refs / allocate_refs_with_root
duplication that landed on PR #20 and was unified in d06a6c2.
Also:
- gitignore allows docs/solutions/** (was caught by docs/* rule)
- CLAUDE.md workspace tree surfaces docs/solutions/ so fresh agents
discover the knowledge store
- docs/phases.md drops two stale DrillDownConfig references (lines 62
and 226) — the symbol no longer exists in the codebase
* refactor: drop unused root_ref field from TreeOptions
TreeOptions.root_ref was populated from SnapshotArgs.root_ref by
tree_options() and then never read anywhere. The actual routing
lives in execute(): `if let Some(ref root) = args.root_ref` branches
to snapshot_ref::run_from_ref(adapter, &opts, root), passing the
root id as an explicit argument — opts.root_ref was never consulted
by any caller.
Delete the field from TreeOptions and its Default impl and stop
populating it in tree_options(). SnapshotArgs.root_ref remains as
the single source of truth for the drill-down target.
* fix: suppress skeleton flag when --root is set
tree_options() clamped depth based on skeleton && root_ref.is_none()
but still forwarded skeleton: args.skeleton unconditionally. When a
caller passed --skeleton --root @e3, opts.skeleton stayed true, so
adapter.get_subtree on macOS built a truncated skeleton tree instead
of the full drill-down subtree, AND ref_alloc::allocate_refs tagged
skeleton-anchor refs with the drill-down root_ref — both unintended.
Tie the skeleton flag to the same root_ref.is_none() guard the depth
clamp already uses. A drill-down is always a full subtree now, never
a skeleton view.
Adds test_tree_options_suppresses_skeleton_for_drill_down to lock the
behavior in and extends the existing clamp test to assert the flag
still propagates on non-drill paths.
* fix: detect web action effect via value + selected + focus change
The previous focus-change-only heuristic in web_action_had_effect was
wrong for non-Chromium AXWebArea elements (Safari WKWebView, Mail,
Notes): AXPress on a checkbox toggles AXValue correctly but does NOT
shift the focused UI element. web_action_had_effect returned false, the
chain escalated to CGClick, and CGClick toggled the checkbox a second
time — net zero visible change. The pointer-equality bug that ae78cbc
replaced was masking this by always returning true; CFEqual exposed
the real signal gap.
Capture element state into a PreActionState struct (focused, value,
selected) before any AX action runs, and consider the action to have
had an effect if ANY of those three fields differs afterward. Element
value covers WKWebView checkboxes, text fields, sliders. Selected
covers tabs, radio buttons, and list items. Focused still covers
Chromium's DOM-driven focus shifts. Triggers on whichever signal is
most relevant to the element type without needing role-specific code.
Also re-exports copy_bool_attr from crates/macos/src/tree for use by
the chain-steps module.
* fix: emit skeleton boundary when drill path is about to hit raw cap
In skeleton mode, a chain of anonymous web wrappers (AXGroup /
AXGenericElement with empty title and value) never advances the
semantic `depth` counter, so `child_depth > max_depth` never fires
no matter how deep the chain runs. The recursion then hits the
`raw_depth >= ABSOLUTE_MAX_DEPTH` top-of-function guard and silently
returns None, dropping the subtree without any children_count marker.
Agents see truncated output with no indication that anything was
dropped.
Extend the at_skeleton_boundary condition to also fire when
child_raw_depth is about to hit ABSOLUTE_MAX_DEPTH, so skeleton mode
always emits a visible truncation node for deep wrapper chains
instead of disappearing them. The old `raw_depth < ABSOLUTE_MAX_DEPTH`
half of the guard was redundant — the top-of-function check already
ensures the current frame has raw budget; the meaningful question is
whether the CHILD frames will.
* perf: compute is_in_webarea once per verified-press call
try_focus_then_verified_confirm_or_press called is_in_webarea(el) to
gate AXConfirm, then fell through to do_verified_press which called
is_in_webarea(el) again on its first line. Each call walks up to 20
AX parents via per-element IPC (AXUIElementCopyAttributeValue on
AXParent), so every web-area click through this path paid the cost
twice.
Extract a private dispatch_verified_press(el, caps, in_web) that
takes the web-area flag as an input. do_verified_press keeps its
existing (el, caps) signature for chain_defs compatibility and
computes is_in_webarea once, then delegates. The focus-then-confirm
path also computes it once and passes it through. The result is
identical on both paths with one AX traversal instead of two.
* fix: skeleton anchors in drill-downs must not inherit root_ref; restore AXBrowser AXContents fallback
Skeleton anchors discovered while processing a drill-down subtree were
being tagged with the drill root_ref, making them indistinguishable from
regular drill entries. re-drills would delete those anchors via
remove_by_root_ref, breaking sub-drilling. They now always carry
root_ref=None so they survive re-drills as stable targets.
AXBrowser child resolution dropped the AXContents fallback when
child_attributes() was unified — the resolver now uses
["AXColumns","AXContents"] matching the original traversal order.
* fix: suppress skeleton anchor creation in drill-down mode to prevent orphaned ref accumulation
* fix: bounds-based resolver pruning and CGClick fallback on chain timeout
Resolver was doing exhaustive DFS through entire AX tree (depth 50) to
find a single element. For large documents like a dense spreadsheet this
meant visiting tens of thousands of cells via AX IPC, causing multi-minute
hangs before the click chain even started.
Two changes:
- find_element_recursive now prunes subtrees whose spatial bounds do not
contain the target element's centre point, and aborts the whole search
after five seconds. Numbers table -> button not inside -> entire table
skipped. Resolution went from >1m49s to <50ms.
- execute_chain now sets a 1s app-level AX messaging timeout so calls to
children/parents fail fast instead of blocking for the system default 6s,
and when the 10s chain deadline fires it attempts the CGClick step before
returning failure so the coordinate fallback is always reached.
* refactor: resolve 9 code-review todos for progressive skeleton traversal
- skeleton refresh now starts from RefMap::new() (removes stale ref accumulation across refreshes)
- drill-down snapshot resolves real WindowInfo via PID lookup instead of synthetic empty id
- --root flag validates ref format before RefMap lookup (returns INVALID_ARGS not STALE_REF)
- ABSOLUTE_MAX_DEPTH truncation emits boundary node with children_count instead of silently dropping
- fix ref vs ref_id field name in commands-observation.md JSON examples
- fix phases.md TreeOptions listing root_ref (it lives in SnapshotArgs, not TreeOptions)
- add batch snapshot skeleton/root examples to commands-system.md
- split snapshot.rs and snapshot_ref.rs test modules into separate _tests.rs files (143/69 LOC)
- add integration tests for skeleton + drill-down workflow including invalid --root validation
* docs: compound progressive snapshot review hardening
* docs: tighten progressive snapshot compound write-up
* docs: update README with progressive skeleton traversal and fix command count to 50
* docs: correct command count to 53, add Notifications section and platform notes
---------
Co-authored-by: Cursor Agent <cursoragent@cursor.com>
