docs: mark all 47 code review todos as complete

2026-07-26 17:12:15 +00:00 · 2026-02-19 11:39:21 -08:00 · 2026-02-19 11:39:21 -08:00 · fe501a1a20
commit fe501a1a20
parent 218503a7eb
47 changed files with 2944 additions and 0 deletions
--- a/todos/001-complete-p1-applescript-injection-focus-window.md
+++ b/todos/001-complete-p1-applescript-injection-focus-window.md
@ -0,0 +1,59 @@
+---
+status: pending
+priority: p1
+issue_id: "001"
+tags: [security, code-review, injection, macos]
+---
+
+# AppleScript Injection in focus_window_impl
+
+## Problem Statement
+
+`focus_window_impl` interpolates an untrusted `app_name` field directly into an AppleScript string, allowing arbitrary AppleScript execution. An AI agent controlling desktop apps with malicious data in window titles or app names can execute arbitrary code on the host machine.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:306-308`
+
+```rust
+let script = format!(r#"tell application "{}" to activate"#, win.app);
+```
+
+The `win.app` field comes from `WindowInfo.app_name`, populated from AXUIElement attributes queried from the OS. A malicious or compromised app can set its `AXTitle` to something like `Finder" to do shell script "rm -rf ~/Documents" tell application "Finder` and escape the AppleScript string context entirely.
+
+## Proposed Solutions
+
+### Option A: Use NSRunningApplication (Recommended)
+Replace osascript with the macOS-native `NSRunningApplication` API, using PID-based lookup to call `-[NSRunningApplication activateWithOptions:]`. No shell, no string interpolation.
+- **Effort:** Small
+- **Risk:** Low — uses a stable AppKit API
+
+### Option B: Quote-escape the app name
+Escape double quotes in `win.app` before interpolation: `win.app.replace('"', r#"\""#)`.
+- **Effort:** Tiny
+- **Risk:** Medium — still relies on shell/AppleScript execution; incomplete mitigation
+
+### Option C: Use PID-based focus via CGWindowLevel
+Call `CGWindowListCopyWindowInfo` + `SetFrontProcess` using the PID from `WindowInfo.pid`.
+- **Effort:** Medium
+- **Risk:** Low
+
+## Recommended Action
+
+Implement Option A: use `NSRunningApplication` via Cocoa FFI with PID-based activation to completely eliminate AppleScript for focus operations.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Line:** 306–308
+- **Component:** macOS adapter, `focus_window_impl`
+
+## Acceptance Criteria
+
+- [ ] `focus_window_impl` does not invoke osascript or AppleScript string interpolation
+- [ ] Focus works correctly for apps with special characters in their name
+- [ ] Existing integration tests pass
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/002-complete-p1-applescript-injection-close-app.md
+++ b/todos/002-complete-p1-applescript-injection-close-app.md
@ -0,0 +1,59 @@
+---
+status: pending
+priority: p1
+issue_id: "002"
+tags: [security, code-review, injection, macos]
+---
+
+# AppleScript Injection in close_app_impl (Graceful Path)
+
+## Problem Statement
+
+`close_app_impl` interpolates an untrusted `id` (app bundle identifier or name) directly into an AppleScript string. Same injection vector as issue 001 but on the close path.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:376-380`
+
+```rust
+let script = format!(r#"tell application "{id}" to quit"#);
+```
+
+The `id` parameter arrives from the CLI as an unvalidated string. An attacker (or misbehaving caller) can pass `Finder" to do shell script "malicious" tell application "Finder` to escape the AppleScript context.
+
+## Proposed Solutions
+
+### Option A: Use NSRunningApplication (Recommended)
+Use `NSRunningApplication` API to find the app by bundle ID and call `-[NSRunningApplication terminate]`. No AppleScript needed.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Use SIGTERM via kill(pid, SIGTERM)
+Use the known PID (looked up from NSRunningApplication or the process table) and send SIGTERM directly.
+- **Effort:** Small
+- **Risk:** Low — avoids all shell/AppleScript entirely
+
+### Option C: Sanitize input
+Validate that `id` matches a strict bundle-ID or process-name regex (`[a-zA-Z0-9._-]+`) before interpolating.
+- **Effort:** Tiny
+- **Risk:** Medium — still relies on AppleScript; regex can miss edge cases
+
+## Recommended Action
+
+Option A: replace with `NSRunningApplication` + `terminate`. Pairs naturally with fix for issue 001.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 376–380
+- **Component:** macOS adapter, `close_app_impl` graceful path
+
+## Acceptance Criteria
+
+- [ ] `close_app_impl` does not invoke osascript or AppleScript string interpolation
+- [ ] App can be closed by bundle identifier containing dots, hyphens, underscores
+- [ ] Force-close path (pkill) is also addressed (see issue 003)
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/003-complete-p1-pkill-arbitrary-input.md
+++ b/todos/003-complete-p1-pkill-arbitrary-input.md
@ -0,0 +1,62 @@
+---
+status: pending
+priority: p1
+issue_id: "003"
+tags: [security, code-review, command-injection, macos]
+---
+
+# pkill -f with Arbitrary User Input
+
+## Problem Statement
+
+The force-close path in `close_app_impl` passes an unvalidated user-supplied string directly to `pkill -f`. `pkill -f` pattern-matches against the full command line of every running process. A crafted input can match and kill unrelated processes (e.g., passing `""` kills all processes the user owns), causing denial of service or system instability.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:370-374`
+
+```rust
+Command::new("pkill")
+    .arg("-f")
+    .arg(&args.id)
+    .status()
+```
+
+With `-f` flag, `pkill` matches against the full command-line string of every running process. Input `""` (empty string) would match everything. Input `"-9"` could cause unexpected flag interpretation.
+
+## Proposed Solutions
+
+### Option A: Use kill(pid, SIGKILL) directly (Recommended)
+Look up the PID via `NSRunningApplication(bundleIdentifier:)` or process enumeration, then use libc `kill(pid, SIGKILL)`. No subprocess, no pattern matching.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Use pkill with exact bundle-ID validation
+Validate `id` against strict regex `^[a-zA-Z0-9][a-zA-Z0-9._-]{0,255}$` before passing to pkill. Replace `-f` with `-x` (exact match on process name, not full cmdline).
+- **Effort:** Small
+- **Risk:** Medium — subprocess invocation remains, but risk is bounded
+
+### Option C: Use `kill` command with PID
+Look up PID, pass `-{signal} {pid}` — but still a subprocess.
+- **Effort:** Small
+- **Risk:** Medium
+
+## Recommended Action
+
+Option A: use `libc::kill(pid, libc::SIGKILL)` after PID lookup. Completely eliminates the process-matching risk.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 370–374
+- **Component:** macOS adapter, `close_app_impl` force path
+
+## Acceptance Criteria
+
+- [ ] No subprocess invocation with user-supplied strings in close_app
+- [ ] Force-close terminates the correct process by PID
+- [ ] Passing empty string or special characters does not affect unrelated processes
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/004-complete-p1-unsound-native-handle-send-sync.md
+++ b/todos/004-complete-p1-unsound-native-handle-send-sync.md
@ -0,0 +1,62 @@
+---
+status: pending
+priority: p1
+issue_id: "004"
+tags: [security, code-review, unsoundness, rust, concurrency]
+---
+
+# Unsound unsafe impl Send+Sync for NativeHandle
+
+## Problem Statement
+
+`NativeHandle` wraps a raw `*const c_void` (AXUIElementRef) and has `unsafe impl Send for NativeHandle {}` and `unsafe impl Sync for NativeHandle {}`. AXUIElement is a Core Foundation object that is NOT thread-safe — it must be accessed only from the thread that created it. This will cause undefined behavior when Phase 3 introduces a tokio async runtime, where handles could cross thread boundaries.
+
+## Findings
+
+**File:** `crates/core/src/adapter.rs:66-69`
+
+```rust
+unsafe impl Send for NativeHandle {}
+unsafe impl Sync for NativeHandle {}
+```
+
+AXUIElement documentation explicitly states it is not thread-safe. The `PhantomData<*const ()>` was added precisely to prevent auto-derived Send/Sync, but the manual unsafe impls override that protection. This is a soundness hole — the compiler cannot catch it.
+
+In Phase 3, tokio tasks run on a thread pool. Any `await` point can move the task to a different thread, causing an AXUIElement to be accessed from a thread that didn't create it → crash or UB.
+
+## Proposed Solutions
+
+### Option A: Remove unsafe impls; make NativeHandle !Send + !Sync (Recommended)
+Remove both `unsafe impl` blocks. This propagates to the adapter trait: mark all platform methods with `&self` (not `&mut self`) and gate concurrent access at the Phase 3 dispatch layer with a `Mutex<Box<dyn PlatformAdapter>>` or dedicated AX thread.
+- **Effort:** Medium (touches adapter trait signature)
+- **Risk:** Low — forces correct threading model explicitly
+
+### Option B: Run all AX operations on a dedicated thread
+Keep `!Send + !Sync`, create a dedicated "AX thread" in Phase 3. Send work items as messages (channel), execute on AX thread, send results back.
+- **Effort:** Medium
+- **Risk:** Low — matches Apple's recommended pattern for AX
+
+### Option C: Use a thread-local AXUIElement cache
+Store elements in a thread-local, identified by ref ID. Resolve on same thread always.
+- **Effort:** Large
+- **Risk:** Medium
+
+## Recommended Action
+
+Option A now (remove unsafe impls), Option B in Phase 3 (AX dispatcher thread). Document threading constraint in PlatformAdapter trait doc-comment.
+
+## Technical Details
+
+- **File:** `crates/core/src/adapter.rs`
+- **Lines:** 66–69
+- **Component:** NativeHandle, core adapter types
+
+## Acceptance Criteria
+
+- [ ] `NativeHandle` does not implement `Send` or `Sync`
+- [ ] Compilation still succeeds (adapter trait adjusted accordingly)
+- [ ] Threading model is documented in a `///` doc-comment on `NativeHandle`
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/005-complete-p1-use-after-free-execute-action.md
+++ b/todos/005-complete-p1-use-after-free-execute-action.md
@ -0,0 +1,65 @@
+---
+status: pending
+priority: p1
+issue_id: "005"
+tags: [security, code-review, memory-safety, use-after-free, macos]
+---
+
+# Use-After-Free Risk in execute_action_impl
+
+## Problem Statement
+
+`execute_action_impl` calls `CFRetain` on the handle's raw pointer before use, but does not check whether the pointer is null or already invalid. An AXUIElement becomes stale when the underlying UI element is destroyed (window closed, app quit). Using a stale element pointer is undefined behavior in C FFI — it can crash or, in a use-after-free scenario, access reallocated memory.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:107-117`
+
+The code performs:
+```rust
+let element_ref = handle.as_raw() as AXUIElementRef;
+CFRetain(element_ref as *const c_void);  // UB if element_ref is null or stale
+// ... perform action
+```
+
+AXUIElement does not provide a validity check API. The correct pattern is:
+1. Check for null before CFRetain
+2. Use `AXUIElementCopyAttributeValue(element_ref, kAXRoleAttribute, ...)` as a liveness probe — it returns `kAXErrorInvalidUIElement` for stale refs
+3. Return `STALE_REF` error code on invalid element
+
+## Proposed Solutions
+
+### Option A: Add null check + AX liveness probe (Recommended)
+Check `element_ref.is_null()` first. Then probe with a cheap AX attribute read. If `kAXErrorInvalidUIElement` is returned, map to `AppError` with code `STALE_REF`.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Wrap in catch_unwind
+Wrap the action execution in `std::panic::catch_unwind`. Converts crashes to panics and then to errors.
+- **Effort:** Small
+- **Risk:** Medium — catch_unwind does not prevent UB; it only catches panics, not segfaults
+
+### Option C: Maintain a validity epoch
+Track a snapshot epoch; invalidate all handles on each new snapshot. Return `STALE_REF` if epoch mismatch.
+- **Effort:** Medium
+- **Risk:** Low — complements Option A
+
+## Recommended Action
+
+Option A: null check + liveness probe before CFRetain. This is the standard pattern for AXUIElement safety.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 107–117
+- **Component:** macOS adapter, `execute_action_impl`
+
+## Acceptance Criteria
+
+- [ ] Null check before CFRetain
+- [ ] Stale element returns `AppError` with code `STALE_REF`, not a crash
+- [ ] Test: performing an action after closing the target window returns STALE_REF
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/006-complete-p1-arbitrary-app-launch.md
+++ b/todos/006-complete-p1-arbitrary-app-launch.md
@ -0,0 +1,62 @@
+---
+status: pending
+priority: p1
+issue_id: "006"
+tags: [security, code-review, command-injection, macos]
+---
+
+# Arbitrary App Launch via open -a
+
+## Problem Statement
+
+`launch_app_impl` passes a user-supplied `id` string directly to `open -a {id}`, allowing an agent to launch any `.app` bundle on the system by name. There is no allowlist, validation, or sandboxing. Combined with the pkill issue (003), this means the agent can launch and terminate arbitrary applications.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:326-329`
+
+```rust
+Command::new("open")
+    .arg("-a")
+    .arg(&args.id)
+    .status()
+```
+
+`open -a` accepts any application name or path. Inputs like `Terminal`, `/Applications/Utilities/Terminal.app`, or `../../../Applications/SomeApp` can all launch unintended applications. The calling AI agent may be tricked (prompt injection) into launching an app that facilitates further attacks.
+
+## Proposed Solutions
+
+### Option A: Use NSWorkspace to launch by bundle ID (Recommended)
+`NSWorkspace.open(withBundleIdentifier:)` accepts only bundle identifiers (e.g., `com.apple.finder`), which are registered in the system's application database. Arbitrary paths are rejected. Bundle IDs can be validated against a regex.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Validate id against strict regex before open -a
+Require `id` to match `^[a-zA-Z0-9][a-zA-Z0-9._-]{1,255}$` (bundle ID format). Reject paths (containing `/` or `..`).
+- **Effort:** Tiny
+- **Risk:** Medium — still invokes open, but constrains inputs
+
+### Option C: Add optional allowlist configuration
+Load an allowlist from config file. Only apps in the list may be launched.
+- **Effort:** Medium
+- **Risk:** Low — good defense in depth, but deferred to Phase 4
+
+## Recommended Action
+
+Option A: use `NSWorkspace` for launch. Combine with Option B-style bundle-ID validation as defense-in-depth.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 326–329
+- **Component:** macOS adapter, `launch_app_impl`
+
+## Acceptance Criteria
+
+- [ ] App launch does not invoke `open -a` with unvalidated input
+- [ ] Path inputs (containing `/` or `..`) are rejected with `INVALID_ARGS`
+- [ ] Launch by bundle ID works for standard system apps (Finder, TextEdit, Safari)
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/007-complete-p1-missing-batch-attribute-fetch.md
+++ b/todos/007-complete-p1-missing-batch-attribute-fetch.md
@ -0,0 +1,67 @@
+---
+status: pending
+priority: p1
+issue_id: "007"
+tags: [performance, code-review, macos, ax-api]
+---
+
+# Missing AXUIElementCopyMultipleAttributeValues Batch Fetch
+
+## Problem Statement
+
+`build_subtree` in the macOS tree builder makes 6 separate `AXUIElementCopyAttributeValue` IPC calls per node. For a typical app with 2000 nodes, this is 12,000 IPC round-trips. macOS's batch API `AXUIElementCopyMultipleAttributeValues` fetches all attributes in a single IPC call, providing a documented 3–5x speedup. This is explicitly required by the CLAUDE.md architecture spec.
+
+## Findings
+
+**File:** `crates/macos/src/tree.rs:59-73`
+
+```rust
+// Current: 6 separate IPC calls per node
+let role = copy_string_attr(element, "AXRole")?;
+let name = copy_string_attr(element, "AXTitle")?;
+let value = copy_string_attr(element, "AXValue")?;
+let description = copy_string_attr(element, "AXDescription")?;
+let enabled = copy_bool_attr(element, "AXEnabled")?;
+let children = copy_children_attr(element, "AXChildren")?;
+```
+
+Additionally, `copy_string_attr` creates a new `CFString::new(attr)` on every call, resulting in ~14,000 CFString allocations for a 2000-node tree.
+
+**Impact:** Snapshot of a complex app (e.g., Xcode) will exceed the 2-second CI gate requirement. Typical observed latency is 8–15 seconds.
+
+## Proposed Solutions
+
+### Option A: Use AXUIElementCopyMultipleAttributeValues (Recommended)
+Batch-fetch `["AXRole", "AXTitle", "AXValue", "AXDescription", "AXEnabled", "AXChildren"]` in a single call per node. Parse the returned CFArray. This is 1 IPC call instead of 6 per node.
+- **Effort:** Medium
+- **Risk:** Low — documented Apple API, used by Accessibility Inspector
+
+### Option B: Cache CFString attribute keys as statics
+Create static `CFString` values for the 6 attribute names once at startup (or lazy_static). Eliminates 14,000 CFString allocations per snapshot.
+- **Effort:** Small
+- **Risk:** Low — independent optimization, combine with Option A
+
+### Option C: Parallel tree traversal with rayon
+Process subtrees in parallel using rayon. But AXUIElement is not thread-safe (see issue 004), so this requires an AX dispatcher thread with work-stealing.
+- **Effort:** Large
+- **Risk:** High — dependency on threading model fix
+
+## Recommended Action
+
+Option A + B together. Implement batch fetch AND cache static CFString keys. Expected result: 3–5x speedup on snapshot latency.
+
+## Technical Details
+
+- **File:** `crates/macos/src/tree.rs`
+- **Lines:** 59–73
+- **Component:** macOS tree builder
+
+## Acceptance Criteria
+
+- [ ] `build_subtree` uses `AXUIElementCopyMultipleAttributeValues` for attribute fetching
+- [ ] Attribute name CFStrings are created once (not per-node)
+- [ ] Snapshot of Xcode (large tree) completes in < 2 seconds on CI
+
+## Work Log
+
+- 2026-02-19: Finding identified by performance-oracle review agent
--- a/todos/008-complete-p1-osascript-subprocess-list-windows.md
+++ b/todos/008-complete-p1-osascript-subprocess-list-windows.md
@ -0,0 +1,60 @@
+---
+status: pending
+priority: p1
+issue_id: "008"
+tags: [performance, code-review, macos, subprocess]
+---
+
+# osascript Subprocess for list_windows (200–500ms per Call)
+
+## Problem Statement
+
+`list_windows_impl` spawns an `osascript` subprocess to enumerate windows. Each subprocess invocation has ~200–500ms cold-start overhead from interpreter initialization. This makes `list-windows` unusable for any workflow that calls it more than once per second, and it re-introduces the command-injection vector (see issues 001–002) via AppleScript in a different method.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:201-265`
+
+The implementation launches `osascript` with an inline AppleScript that calls `tell application "System Events"`. This is:
+- **Slow:** 200–500ms per call vs. <1ms for native API
+- **Fragile:** System Events must be running and authorized
+- **Security risk:** AppleScript interpolation vector
+
+macOS provides `CGWindowListCopyWindowInfo(kCGWindowListOptionOnScreenOnly, kCGNullWindowID)` which returns all visible windows in a single native call with <1ms latency.
+
+## Proposed Solutions
+
+### Option A: Use CGWindowListCopyWindowInfo (Recommended)
+Call `CGWindowListCopyWindowInfo` to get window list. Extract `kCGWindowOwnerName`, `kCGWindowName`, `kCGWindowOwnerPID`, `kCGWindowBounds`. All fields available natively.
+- **Effort:** Medium
+- **Risk:** Low — stable public API used by macOS system tools
+
+### Option B: Use NSWorkspace + AXUIElementCreateApplication
+Enumerate `NSWorkspace.sharedWorkspace.runningApplications`, then for each app call `AXUIElementCreateApplication(pid)` and read `kAXWindowsAttribute` to get window list.
+- **Effort:** Small
+- **Risk:** Low — requires AX permission already granted
+
+### Option C: Keep osascript but cache results
+Cache window list for 500ms. Amortizes the subprocess cost for repeated calls.
+- **Effort:** Tiny
+- **Risk:** Medium — stale data; doesn't fix security issue; still slow on first call
+
+## Recommended Action
+
+Option A: `CGWindowListCopyWindowInfo` for on-screen windows. Option B as supplementary for off-screen/minimized windows.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 201–265
+- **Component:** macOS adapter, `list_windows_impl`
+
+## Acceptance Criteria
+
+- [ ] `list-windows` does not spawn osascript subprocess
+- [ ] `list-windows` returns results in < 10ms
+- [ ] Window list includes title, app name, pid, bounds for all visible windows
+
+## Work Log
+
+- 2026-02-19: Finding identified by performance-oracle review agent
--- a/todos/009-complete-p1-resolve-element-no-cycle-detection.md
+++ b/todos/009-complete-p1-resolve-element-no-cycle-detection.md
@ -0,0 +1,61 @@
+---
+status: pending
+priority: p1
+issue_id: "009"
+tags: [performance, code-review, macos, infinite-loop]
+---
+
+# resolve_element Lacks Cycle Detection (Potential Infinite Loop)
+
+## Problem Statement
+
+`resolve_element_impl` performs a full O(n) tree traversal on every action invocation, without any visited-set or cycle detection. If the AX tree has a cycle (possible in buggy or adversarial apps), the traversal will loop infinitely, hanging the agent-desktop process forever. Even without cycles, traversing a 10,000-node tree for every click is a severe performance bottleneck.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:131-194`
+
+`find_element_recursive` walks the entire AX tree depth-first. Issues:
+
+1. **No visited-set:** A cycle in the AX tree causes infinite recursion / stack overflow
+2. **Full re-traversal per action:** Every `click`, `type`, etc. re-traverses the full tree
+3. **O(n) per action:** For complex apps (n=10,000), this adds 2–5 seconds to every action
+
+The design calls for a RefMap that stores `(pid, role, name, bounds_hash)` for fast re-identification, but the current implementation ignores the RefMap and does full traversal.
+
+## Proposed Solutions
+
+### Option A: Use RefMap for O(1) element lookup (Recommended)
+On action, load the RefMap, look up the ref ID, then call `AXUIElementCreateApplication(pid)` + walk only to the known element using stored path/hash. Use `kAXRoleAttribute` + `kAXTitleAttribute` for liveness confirmation.
+- **Effort:** Medium
+- **Risk:** Low — RefMap already exists, just needs to be used for resolution
+
+### Option B: Add visited-set to current traversal
+Maintain a `HashSet<AXUIElementRef_ptr>` during traversal. Skip already-visited nodes.
+- **Effort:** Small
+- **Risk:** Low — prevents infinite loop; doesn't fix O(n) performance
+
+### Option C: Index by (role, name, bounds_hash) at snapshot time
+Build an in-memory HashMap at snapshot time. Resolution is O(1) hash lookup.
+- **Effort:** Medium
+- **Risk:** Medium — HashMap must be invalidated per-snapshot
+
+## Recommended Action
+
+Option B immediately (safety), Option A in same PR (performance). Both changes are in the same function.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 131–194
+- **Component:** macOS adapter, `resolve_element_impl`, `find_element_recursive`
+
+## Acceptance Criteria
+
+- [ ] Traversal has visited-set preventing infinite loop on cyclic AX trees
+- [ ] Element resolution does not re-traverse the full tree on every action call
+- [ ] Action latency for click on any element < 100ms (excluding AX IPC)
+
+## Work Log
+
+- 2026-02-19: Finding identified by performance-oracle review agent
--- a/todos/010-complete-p2-element-for-pid-no-null-check.md
+++ b/todos/010-complete-p2-element-for-pid-no-null-check.md
@ -0,0 +1,56 @@
+---
+status: pending
+priority: p2
+issue_id: "010"
+tags: [security, code-review, null-safety, macos]
+---
+
+# resolve_element_impl No Null Check on element_for_pid
+
+## Problem Statement
+
+`resolve_element_impl` calls `AXUIElementCreateApplication(pid)` but does not check whether the returned element is null before using it. If `pid` refers to a terminated process, this returns null, and subsequent attribute reads are UB.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:125-128`
+
+```rust
+let app_element = AXUIElementCreateApplication(pid);
+// No null check here
+let role = copy_string_attr(app_element, "AXRole");  // UB if app_element is null
+```
+
+A race condition between snapshot (when PID was valid) and action execution (after process terminated) can trigger this path regularly in production.
+
+## Proposed Solutions
+
+### Option A: Null check + ELEMENT_NOT_FOUND error
+Check `app_element.is_null()`. If null, return `AppError` with `ErrorCode::ElementNotFound` and suggestion to re-run snapshot.
+- **Effort:** Tiny
+- **Risk:** Low
+
+### Option B: Verify PID is alive first
+Call `kill(pid, 0)` to check if process is still running before `AXUIElementCreateApplication`. Return `APP_NOT_FOUND` if dead.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option A (null check on element) + Option B (PID liveness check) together. Defense in depth.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 125–128
+- **Component:** macOS adapter, `resolve_element_impl`
+
+## Acceptance Criteria
+
+- [ ] Null pointer is checked before use
+- [ ] Stale PID returns structured error, not a crash
+- [ ] Error code is `ELEMENT_NOT_FOUND` or `APP_NOT_FOUND` as appropriate
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/011-complete-p2-toctou-race-refmap-load.md
+++ b/todos/011-complete-p2-toctou-race-refmap-load.md
@ -0,0 +1,69 @@
+---
+status: pending
+priority: p2
+issue_id: "011"
+tags: [security, code-review, race-condition, file-io]
+---
+
+# TOCTOU Race Condition on RefMap Load
+
+## Problem Statement
+
+`refs.rs` reads the RefMap file with a size-check via `metadata()` followed by `read_to_string()`. Between the two syscalls, another process (a concurrent agent) could replace the file with a larger one, causing the size-based check to miss oversized content. This is a classic TOCTOU (time-of-check/time-of-use) race.
+
+## Findings
+
+**File:** `crates/core/src/refs.rs:84-97`
+
+```rust
+let meta = fs::metadata(&path)?;
+if meta.len() > MAX_REFMAP_SIZE {
+    return Err(...);
+}
+let content = fs::read_to_string(&path)?;  // Different file by now!
+```
+
+Additionally, there is no file locking, so two concurrent agents running `snapshot` simultaneously can corrupt the RefMap with a partial write.
+
+## Proposed Solutions
+
+### Option A: Read then check size (Recommended)
+Read the file content first (bounded by `take(MAX_REFMAP_SIZE + 1)`), then check size. Single open, no race window.
+```rust
+let mut f = File::open(&path)?;
+let mut content = String::new();
+f.take(MAX_REFMAP_SIZE + 1).read_to_string(&mut content)?;
+if content.len() > MAX_REFMAP_SIZE as usize { return Err(...); }
+```
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Add advisory file locking (flock)
+Use `fs2::FileExt::lock_exclusive()` during write, `lock_shared()` during read.
+- **Effort:** Small
+- **Risk:** Low — handles concurrent agent scenario
+
+### Option C: Use temp file + atomic rename (already done for write)
+The write path already uses temp+rename. Ensure the read path handles partial writes by checking file size after open, not before.
+- **Effort:** Tiny
+- **Risk:** Low
+
+## Recommended Action
+
+Option A + Option B. Eliminates TOCTOU and handles concurrent agents.
+
+## Technical Details
+
+- **File:** `crates/core/src/refs.rs`
+- **Lines:** 84–97
+- **Component:** RefMap loader
+
+## Acceptance Criteria
+
+- [ ] File size check occurs after open, not before
+- [ ] Concurrent snapshot runs do not corrupt the RefMap
+- [ ] Oversized RefMap returns structured error, not panic
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/012-complete-p2-wait-ms-unbounded.md
+++ b/todos/012-complete-p2-wait-ms-unbounded.md
@ -0,0 +1,62 @@
+---
+status: pending
+priority: p2
+issue_id: "012"
+tags: [security, code-review, denial-of-service]
+---
+
+# wait Command ms Parameter Unbounded (DoS Vector)
+
+## Problem Statement
+
+The `wait` command accepts a `u64` milliseconds parameter with no upper bound. `agent-desktop wait 18446744073709551615` would block for 585 million years, permanently hanging the invoking agent. Even `wait 3600000` (1 hour) would stall an agent indefinitely.
+
+## Findings
+
+**File:** `crates/core/src/commands/wait.rs`
+
+```rust
+pub struct WaitArgs {
+    pub ms: u64,  // No upper bound
+}
+// ...
+std::thread::sleep(Duration::from_millis(args.ms));
+```
+
+An AI agent that misinterprets a value or is given adversarial input could invoke this with an enormous duration, causing the process to hang permanently.
+
+## Proposed Solutions
+
+### Option A: Cap at a reasonable maximum (Recommended)
+Define `MAX_WAIT_MS: u64 = 30_000` (30 seconds). Return `INVALID_ARGS` if exceeded.
+- **Effort:** Tiny
+- **Risk:** Low
+
+### Option B: Validate at CLI layer via clap value_parser
+Use `clap`'s `value_parser(1_u64..=30_000_u64)` to reject out-of-range values before reaching execute().
+- **Effort:** Tiny
+- **Risk:** Low
+
+### Option C: Use a configurable timeout with a hard cap
+Allow caller to specify up to N ms (configurable, default 30s, max 300s).
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option B: validate at CLI layer with `value_parser`. Clean, zero-overhead, clear error message.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/wait.rs`
+- **Component:** wait command
+
+## Acceptance Criteria
+
+- [ ] `wait` rejects ms values > 30,000 (or chosen cap)
+- [ ] Error response uses `INVALID_ARGS` error code
+- [ ] `wait 100` still works correctly
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/013-complete-p2-screenshot-path-traversal.md
+++ b/todos/013-complete-p2-screenshot-path-traversal.md
@ -0,0 +1,61 @@
+---
+status: pending
+priority: p2
+issue_id: "013"
+tags: [security, code-review, path-traversal]
+---
+
+# Screenshot output_path Has No Path Traversal Validation
+
+## Problem Statement
+
+The `screenshot` command accepts an `--output` path without validating it. A caller can write screenshots to arbitrary locations: `/etc/cron.d/evil`, `~/.ssh/authorized_keys`, or `../../../sensitive`. This enables data exfiltration (write sensitive bytes as PNG header) or filesystem writes outside the intended output directory.
+
+## Findings
+
+**File:** `crates/core/src/commands/screenshot.rs:24-26`
+
+```rust
+pub struct ScreenshotArgs {
+    pub output_path: Option<PathBuf>,  // No validation
+}
+```
+
+No canonicalization, no `..` component check, no restriction to user-writable directories.
+
+## Proposed Solutions
+
+### Option A: Validate path is within allowed directories (Recommended)
+Canonicalize the path. Ensure it doesn't contain `..` components after resolution. Optionally restrict to the current directory or a configured output directory.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Restrict to .png extension
+Require `output_path` to end in `.png` or `.jpg`. Prevents writing to arbitrary paths used as config files.
+- **Effort:** Tiny
+- **Risk:** Medium — doesn't prevent writing to arbitrary .png paths
+
+### Option C: Write to a temp file and return path in JSON
+Never accept a caller-specified path. Write to a temp file, return its path in the JSON response. Caller is responsible for moving it.
+- **Effort:** Small
+- **Risk:** Low — eliminates the attack surface entirely
+
+## Recommended Action
+
+Option A: canonicalize + `..` check. Also enforce `.png`/`.jpg` extension (Option B) as defense-in-depth.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/screenshot.rs`
+- **Lines:** 24–26
+- **Component:** screenshot command
+
+## Acceptance Criteria
+
+- [ ] Paths containing `..` components are rejected with `INVALID_ARGS`
+- [ ] Output path is validated before writing
+- [ ] Screenshot to valid path in current directory still works
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/014-complete-p2-protected-process-check-bypassable.md
+++ b/todos/014-complete-p2-protected-process-check-bypassable.md
@ -0,0 +1,64 @@
+---
+status: pending
+priority: p2
+issue_id: "014"
+tags: [security, code-review, access-control]
+---
+
+# Protected-Process Check Is Substring-Based and Easily Bypassed
+
+## Problem Statement
+
+`close_app` has a "protected process" check that prevents killing critical system processes, but it uses substring matching on the app name string. This is bypassable: an app named `Finder-safe` would match `Finder` and be blocked; an app named `Finder_` would not. More importantly, the check lives only in the command layer, not in the adapter, so future refactoring could bypass it.
+
+## Findings
+
+**File:** `crates/core/src/commands/close_app.rs`
+
+The protection is implemented as a string contains check:
+```rust
+if PROTECTED_APPS.iter().any(|p| args.id.contains(p)) {
+    return Err(AppError::permission_denied(...));
+}
+```
+
+Issues:
+1. Substring match: `"windowserver"` blocks `"com.apple.windowserver"` but not `"WindowServer"` (case-sensitive)
+2. False positive: `"FinderExtension"` blocked because it contains `"Finder"`
+3. Check is in command layer only — adapter layer has no guard
+
+## Proposed Solutions
+
+### Option A: Move check to adapter + use exact bundle-ID matching (Recommended)
+Define `PROTECTED_BUNDLE_IDS: &[&str]` with exact bundle IDs (`com.apple.finder`, `com.apple.dock`). Match against `NSRunningApplication.bundleIdentifier` (not user-supplied name).
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Case-insensitive exact match
+Convert both sides to lowercase, use exact equality not contains. Prevents false positives on substrings.
+- **Effort:** Tiny
+- **Risk:** Medium — still in command layer
+
+### Option C: OS-level validation
+Rely on macOS SIP (System Integrity Protection) to prevent termination of system-protected processes. Don't implement our own check.
+- **Effort:** None
+- **Risk:** Low — but SIP doesn't protect all system apps
+
+## Recommended Action
+
+Option A: exact bundle-ID matching in adapter layer. More robust and harder to bypass.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/close_app.rs`
+- **Component:** close_app command, macOS adapter
+
+## Acceptance Criteria
+
+- [ ] Protected app check uses exact bundle identifier, not substring
+- [ ] `com.apple.finder` is blocked; `FinderExtension` is not
+- [ ] Check is enforced at adapter level, not only command level
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/015-complete-p2-home-dir-env-only.md
+++ b/todos/015-complete-p2-home-dir-env-only.md
@ -0,0 +1,60 @@
+---
+status: pending
+priority: p2
+issue_id: "015"
+tags: [security, code-review, configuration]
+---
+
+# home_dir() Uses Only HOME Env Var (Not getpwuid_r Fallback)
+
+## Problem Statement
+
+`refs.rs` uses `std::env::var("HOME")` to locate the RefMap directory. If the tool is invoked in an environment where `HOME` is unset (sandboxed environment, su/sudo context, CI runner, systemd service), it will fail with a confusing error instead of falling back to the system password database (`getpwuid_r`).
+
+## Findings
+
+**File:** `crates/core/src/refs.rs:112-114`
+
+```rust
+let home = std::env::var("HOME")
+    .map_err(|_| AppError::internal("HOME env var not set"))?;
+```
+
+`HOME` can be unset in sandboxed macOS apps, in CI via `sudo`, or in daemon contexts. The POSIX-correct approach is to try `HOME` first, then fall back to `getpwuid_r(getuid())`.
+
+## Proposed Solutions
+
+### Option A: Use dirs::home_dir() crate (Recommended)
+The `dirs` crate handles `HOME` + `getpwuid_r` fallback correctly on all platforms.
+- **Effort:** Tiny (add one dependency)
+- **Risk:** Low
+
+### Option B: Manual getpwuid_r fallback
+Call `libc::getpwuid_r(libc::getuid(), ...)` when HOME is unset. Platform-correct but verbose.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option C: Make RefMap path configurable via env var
+Introduce `AGENT_DESKTOP_DATA_DIR` env var. Default to `$HOME/.agent-desktop` when set. Useful for testing and CI regardless.
+- **Effort:** Small
+- **Risk:** Low — good flexibility, independent of the HOME fallback fix
+
+## Recommended Action
+
+Option A (dirs crate) + Option C (configurable env var).
+
+## Technical Details
+
+- **File:** `crates/core/src/refs.rs`
+- **Lines:** 112–114
+- **Component:** RefMap path resolution
+
+## Acceptance Criteria
+
+- [ ] Works correctly when HOME is unset
+- [ ] Falls back to getpwuid_r or equivalent
+- [ ] Optional: accepts AGENT_DESKTOP_DATA_DIR env override
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/016-complete-p2-screenshot-zero-buffer.md
+++ b/todos/016-complete-p2-screenshot-zero-buffer.md
@ -0,0 +1,61 @@
+---
+status: pending
+priority: p2
+issue_id: "016"
+tags: [performance, code-review, memory, macos]
+---
+
+# Screenshot Allocates Zeroed Buffer Instead of Actual Pixel Data
+
+## Problem Statement
+
+The macOS screenshot implementation allocates a zeroed `Vec<u8>` of the expected size instead of capturing actual pixel data from `CGWindowListCreateImage`. The output is always a blank image. This means the screenshot command is non-functional, and when it does capture real pixels, it will allocate 30MB+ for a Retina display without streaming.
+
+## Findings
+
+**File:** `crates/core/src/commands/screenshot.rs`
+
+```rust
+let buffer = vec![0u8; width * height * 4];  // Blank pixel data
+```
+
+Issues:
+1. Functionally broken — returns blank PNG
+2. When fixed, Retina displays produce 30–60MB raw pixel data held entirely in memory
+3. No streaming to disk — entire image in RAM before write
+
+## Proposed Solutions
+
+### Option A: Implement CGWindowListCreateImage + stream PNG encode (Recommended)
+Call `CGWindowListCreateImage` → `CGImageGetDataProvider` → `CGDataProviderCopyData`. Encode to PNG via `image` crate with streaming write to file.
+- **Effort:** Medium
+- **Risk:** Low — straightforward CGImage → PNG pipeline
+
+### Option B: Use screencapture CLI tool
+Spawn `screencapture -x -t png {output_path}` as a subprocess. Simple, handles all edge cases, but adds process spawn overhead (~100ms).
+- **Effort:** Tiny
+- **Risk:** Low — but subprocess dependency
+
+### Option C: Return base64-encoded PNG in JSON (no output_path)
+Encode PNG to base64, return inline in JSON response. Avoids path traversal concern (013) entirely.
+- **Effort:** Medium
+- **Risk:** Low for small screenshots; Large responses for Retina
+
+## Recommended Action
+
+Option A for correctness and performance. Use Option B as a temporary fix to unblock functional testing while Option A is implemented.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/screenshot.rs`, `crates/macos/src/adapter.rs`
+- **Component:** screenshot command, macOS screenshot adapter
+
+## Acceptance Criteria
+
+- [ ] Screenshot returns actual pixel data (not zeroed buffer)
+- [ ] Retina display screenshot does not OOM (streaming or bounded allocation)
+- [ ] Output PNG is valid and viewable
+
+## Work Log
+
+- 2026-02-19: Finding identified by performance-oracle review agent
--- a/todos/017-complete-p2-double-serialization-snapshot.md
+++ b/todos/017-complete-p2-double-serialization-snapshot.md
@ -0,0 +1,64 @@
+---
+status: pending
+priority: p2
+issue_id: "017"
+tags: [performance, code-review, serialization]
+---
+
+# Double-Serialization in snapshot.rs
+
+## Problem Statement
+
+`snapshot.rs` serializes the tree to a `serde_json::Value` first, then wraps it in another `json!` macro call, resulting in double serialization. The tree gets serialized twice — once to Value, once to the final response string. For a large tree (2000 nodes), this doubles the serialization CPU time and transiently allocates a full copy of the JSON tree.
+
+## Findings
+
+**File:** `crates/core/src/commands/snapshot.rs`
+
+```rust
+let tree_value = serde_json::to_value(&result.tree)?;  // First serialization
+Ok(json!({                                               // Second serialization
+    "app": result.app,
+    "tree": tree_value,                                  // Re-serialized
+    ...
+}))
+```
+
+This pattern is unnecessary. The `json!` macro accepts any `Serialize` type directly via the `serde_json::to_value` path internally.
+
+## Proposed Solutions
+
+### Option A: Pass tree reference directly to json! macro (Recommended)
+```rust
+Ok(json!({
+    "app": result.app,
+    "tree": &result.tree,  // json! calls Serialize directly
+    ...
+}))
+```
+- **Effort:** Tiny
+- **Risk:** Low
+
+### Option B: Build a typed response struct
+Define a `SnapshotResponse { app, tree, ref_count, window }` and derive `Serialize`. Return it directly.
+- **Effort:** Small
+- **Risk:** Low — cleaner, avoids json! macro entirely
+
+## Recommended Action
+
+Option A for immediate fix. Option B as a follow-up cleanup.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/snapshot.rs`
+- **Component:** snapshot command serialization
+
+## Acceptance Criteria
+
+- [ ] Tree is serialized exactly once
+- [ ] Snapshot response JSON is identical to current output
+- [ ] No transient `serde_json::Value` clone of entire tree
+
+## Work Log
+
+- 2026-02-19: Finding identified by performance-oracle review agent
--- a/todos/018-complete-p3-no-file-locking-concurrent.md
+++ b/todos/018-complete-p3-no-file-locking-concurrent.md
@ -0,0 +1,59 @@
+---
+status: pending
+priority: p3
+issue_id: "018"
+tags: [code-review, concurrency, file-io]
+---
+
+# No File Locking for Concurrent Agent Snapshot Writes
+
+## Problem Statement
+
+When two AI agents run `agent-desktop snapshot` simultaneously, they both write to `~/.agent-desktop/last_refmap.json`. The write uses a temp file + atomic rename, which prevents torn writes. However, agent B's snapshot will silently overwrite agent A's snapshot, invalidating any refs agent A is about to use. There is no coordination mechanism.
+
+## Findings
+
+**File:** `crates/core/src/refs.rs`
+
+The write path is:
+```rust
+fs::write(&tmp_path, &json_bytes)?;
+fs::rename(&tmp_path, &refmap_path)?;
+```
+
+This is atomic at the filesystem level, but two concurrent snapshots will result in one overwriting the other with no error to the losing agent.
+
+## Proposed Solutions
+
+### Option A: Session-scoped RefMap files
+Name the RefMap file by session: `~/.agent-desktop/sessions/{session_id}/last_refmap.json`. Each agent instance uses its own file. No contention.
+- **Effort:** Medium
+- **Risk:** Low — cleaner design; enables multi-agent workflows
+
+### Option B: Advisory flock on write
+Use `fs2` crate's exclusive lock before writing. If lock fails, return error rather than overwriting.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option C: Document the limitation
+Add a `///` doc-comment noting that concurrent snapshots are not supported in Phase 1.
+- **Effort:** Tiny
+- **Risk:** None — defers to Phase 4 daemon
+
+## Recommended Action
+
+Option C now (document), Option A in Phase 4 (session-scoped files as part of daemon design).
+
+## Technical Details
+
+- **File:** `crates/core/src/refs.rs`
+- **Component:** RefMap write path
+
+## Acceptance Criteria
+
+- [ ] Limitation is documented
+- [ ] Phase 4 design includes session-scoped RefMap files
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/019-complete-p3-refentry-pid-hardcoded-zero.md
+++ b/todos/019-complete-p3-refentry-pid-hardcoded-zero.md
@ -0,0 +1,60 @@
+---
+status: pending
+priority: p3
+issue_id: "019"
+tags: [code-review, correctness]
+---
+
+# RefEntry.pid Hardcoded to 0 in Snapshot
+
+## Problem Statement
+
+`RefEntry` in the snapshot output has `pid` hardcoded to `0`. When `resolve_element_impl` looks up an element by ref, it calls `AXUIElementCreateApplication(entry.pid)`, which with `pid=0` targets the kernel — a nonsensical lookup that will always fail or behave unexpectedly.
+
+## Findings
+
+**File:** `crates/core/src/snapshot.rs:95-102`
+
+```rust
+RefEntry {
+    pid: 0,  // Should be the actual app PID
+    role: node.role.clone(),
+    name: node.name.clone().unwrap_or_default(),
+    bounds_hash: compute_bounds_hash(node.bounds),
+    available_actions: vec![],
+}
+```
+
+The `pid` should be the PID of the application window from which this node was captured, available from `WindowInfo.pid`.
+
+## Proposed Solutions
+
+### Option A: Pass WindowInfo.pid through SnapshotEngine to RefEntry construction
+Thread the `pid` from the snapshot call through to `RefEntry` creation. Minor refactor.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Store pid in SnapshotResult and populate in allocate_refs
+Have `SnapshotEngine` carry the `pid` from the `WindowInfo` used for the snapshot, then pass it to each `RefEntry`.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option B: `SnapshotEngine` holds the source `pid`, populates all `RefEntry` instances with it.
+
+## Technical Details
+
+- **File:** `crates/core/src/snapshot.rs`
+- **Lines:** 95–102
+- **Component:** SnapshotEngine, RefEntry allocation
+
+## Acceptance Criteria
+
+- [ ] `RefEntry.pid` contains the actual process PID
+- [ ] `pid=0` never appears in a saved RefMap
+- [ ] Element resolution uses correct PID for `AXUIElementCreateApplication`
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/020-complete-p3-bounds-hash-unstable-hasher.md
+++ b/todos/020-complete-p3-bounds-hash-unstable-hasher.md
@ -0,0 +1,71 @@
+---
+status: pending
+priority: p3
+issue_id: "020"
+tags: [code-review, correctness, stability]
+---
+
+# bounds_hash Uses DefaultHasher (Unstable Across Rust Versions)
+
+## Problem Statement
+
+`bounds_hash()` in `node.rs` uses `std::collections::hash_map::DefaultHasher`. The DefaultHasher algorithm is not guaranteed to be stable across Rust versions — it can change between releases. If the hash changes after a Rust toolchain upgrade, every ref from old snapshots will appear stale, causing spurious `STALE_REF` errors for agents that persist refs across sessions.
+
+## Findings
+
+**File:** `crates/core/src/node.rs`
+
+```rust
+use std::collections::hash_map::DefaultHasher;
+use std::hash::{Hash, Hasher};
+
+fn bounds_hash(bounds: Option<Bounds>) -> u64 {
+    let mut hasher = DefaultHasher::new();
+    bounds.hash(&mut hasher);
+    hasher.finish()
+}
+```
+
+The Rust documentation explicitly states: "The default hashing algorithm is currently SipHash 1-3, though this is subject to change at any point in the future."
+
+## Proposed Solutions
+
+### Option A: Use a stable, deterministic hasher (Recommended)
+Replace with `FxHasher` (from `rustc-hash`) or `fnv::FnvHasher`. Both are stable, fast, and have no version-dependent behavior.
+- **Effort:** Tiny (add one dependency)
+- **Risk:** Low
+
+### Option B: Compute bounds hash manually
+XOR the bit representations of x, y, width, height as u64. No hasher needed.
+```rust
+fn bounds_hash(b: Option<Bounds>) -> u64 {
+    let b = b.unwrap_or_default();
+    (b.x.to_bits() as u64) ^ ((b.y.to_bits() as u64) << 16)
+    ^ ((b.width.to_bits() as u64) << 32) ^ ((b.height.to_bits() as u64) << 48)
+}
+```
+- **Effort:** Tiny
+- **Risk:** Low — no dependency, deterministic
+
+### Option C: Use AXIdentifier attribute instead of bounds
+Prefer `kAXIdentifierAttribute` (stable accessibility ID set by app) over bounds. Falls back to bounds hash only when AXIdentifier is absent.
+- **Effort:** Medium
+- **Risk:** Low — better element identity
+
+## Recommended Action
+
+Option B: simple bit-XOR hash. No dependency, deterministic, clear intent. Option C as follow-up enhancement.
+
+## Technical Details
+
+- **File:** `crates/core/src/node.rs`
+- **Component:** bounds_hash, RefEntry identity
+
+## Acceptance Criteria
+
+- [ ] `bounds_hash` produces identical output across Rust toolchain versions
+- [ ] Existing test fixtures remain valid after the change
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/021-complete-p3-blocked-combos-modifier-order.md
+++ b/todos/021-complete-p3-blocked-combos-modifier-order.md
@ -0,0 +1,53 @@
+---
+status: pending
+priority: p3
+issue_id: "021"
+tags: [code-review, correctness]
+---
+
+# Blocked Combos Modifier Order Not Normalized in press.rs
+
+## Problem Statement
+
+`press.rs` has a blocklist of dangerous key combos (e.g., `cmd+shift+q` for logout, `cmd+q` for quit all). The check compares the string representation of the combo, but modifier order is not normalized. `"shift+cmd+q"` and `"cmd+shift+q"` represent the same combo but the check would only block one of them.
+
+## Findings
+
+**File:** `crates/core/src/commands/press.rs`
+
+The blocked combo check compares formatted combo strings directly. If the caller specifies modifiers in a different order than the hardcoded blocklist, the check is bypassed:
+
+```rust
+// Blocked: "cmd+shift+q"
+// Not blocked: "shift+cmd+q" — same key, different modifier order
+```
+
+## Proposed Solutions
+
+### Option A: Normalize modifier order before comparison (Recommended)
+Sort modifiers canonically: Ctrl < Alt < Shift < Cmd. Compare normalized representations.
+- **Effort:** Tiny
+- **Risk:** Low
+
+### Option B: Use a HashSet of modifiers for comparison
+Store blocked combos as `(key: String, modifiers: HashSet<Modifier>)` tuples. Compare sets, not strings.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option B: compare modifier sets. More robust and easier to read.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/press.rs`
+- **Component:** key combo blocklist
+
+## Acceptance Criteria
+
+- [ ] `cmd+shift+q` and `shift+cmd+q` are treated identically
+- [ ] All dangerous combo variants are blocked regardless of modifier order
+
+## Work Log
+
+- 2026-02-19: Finding identified by security-sentinel review agent
--- a/todos/022-complete-p1-batch-command-non-functional.md
+++ b/todos/022-complete-p1-batch-command-non-functional.md
@ -0,0 +1,67 @@
+---
+status: pending
+priority: p1
+issue_id: "022"
+tags: [code-review, correctness, architecture]
+---
+
+# Batch Command Does Not Dispatch Sub-Commands
+
+## Problem Statement
+
+The `batch` command parses its JSON input and counts the commands, but never dispatches them. It returns a note saying "dispatch is in the binary crate" — but `src/dispatch.rs` routes `Commands::Batch` to `batch::execute`, which does not call back into dispatch. The batch command is completely non-functional.
+
+## Findings
+
+**File:** `crates/core/src/commands/batch.rs:18-26`
+
+```rust
+pub fn execute(args: BatchArgs, _adapter: &dyn PlatformAdapter) -> Result<Value, AppError> {
+    let commands: Vec<BatchCommand> = serde_json::from_str(&args.commands_json)...;
+    Ok(json!({
+        "note": "Batch dispatch is implemented in the binary crate dispatch layer",
+        "count": commands.len()
+    }))
+}
+```
+
+**File:** `src/dispatch.rs:146-152` — routes `Commands::Batch` to `batch::execute`, which does the above. No sub-commands are executed.
+
+The `BatchCommand` fields (`command`, `args`) are never read beyond deserialization, which is why clippy flagged them as `dead_code` (commit 608d4aa). The `#[allow(dead_code)]` suppressor was applied to silence the symptom rather than fix the underlying issue.
+
+## Proposed Solutions
+
+### Option A: Implement batch dispatch in the binary crate (Recommended)
+Move batch execution logic to the binary crate where `dispatch()` is accessible. Parse each `BatchCommand`, construct the corresponding `Commands` enum variant, and call `dispatch()` recursively for each. Return an array of results.
+- **Effort:** Medium
+- **Risk:** Low
+
+### Option B: Implement batch dispatch in core via a callback
+Pass a `dispatch_fn: &dyn Fn(Command, &dyn PlatformAdapter) -> Result<Value, AppError>` closure into `batch::execute`. Keeps logic in core.
+- **Effort:** Medium
+- **Risk:** Low — but requires changing execute() signature
+
+### Option C: Document batch as Phase 2 and return clear NOT_IMPLEMENTED error
+If batch is intentionally deferred, return `AppError` with `ErrorCode::NotImplemented` instead of a misleading success response with a note.
+- **Effort:** Tiny
+- **Risk:** Low — honest API response
+
+## Recommended Action
+
+Option A: implement in the binary crate. The `dispatch` function is already there; batch just needs to parse and call it for each sub-command. Remove the `#[allow(dead_code)]` suppressor once fields are used.
+
+## Technical Details
+
+- **Files:** `crates/core/src/commands/batch.rs`, `src/dispatch.rs`
+- **Component:** batch command, binary dispatch
+
+## Acceptance Criteria
+
+- [ ] `batch` with a JSON array of commands executes each sub-command in order
+- [ ] `batch` with `stop_on_error: true` halts on first failure
+- [ ] Response includes per-command results array
+- [ ] `#[allow(dead_code)]` on BatchCommand is removed
+
+## Work Log
+
+- 2026-02-19: Finding identified by git-history-analyzer agent
--- a/todos/023-complete-p1-read-bounds-always-returns-none.md
+++ b/todos/023-complete-p1-read-bounds-always-returns-none.md
@ -0,0 +1,69 @@
+---
+status: pending
+priority: p1
+issue_id: "023"
+tags: [code-review, correctness, macos]
+---
+
+# read_bounds Always Returns None — Bounds Never Populated
+
+## Problem Statement
+
+`read_bounds` in the macOS tree builder is a stub that always returns `None`. The `--include-bounds` flag is accepted by the CLI and threaded through the entire call chain, but no bounds data is ever populated. Any feature relying on bounds (coordinate-based element identification, `get bounds`, `scroll`, visual testing) silently returns no data.
+
+## Findings
+
+**File:** `crates/macos/src/tree.rs:160`
+
+```rust
+fn read_bounds(_el: &AXElement) -> Option<Rect> {
+    None  // Stub — bounds never implemented
+}
+```
+
+This also means:
+1. `RefEntry.bounds_hash` will always hash `None`, making all interactive elements produce the same bounds hash — zero entropy for element re-identification
+2. `agent-desktop get bounds @e5` will always return empty data
+3. `agent-desktop snapshot --include-bounds` returns a tree with no bounds despite the flag
+
+The `kAXPositionAttribute` + `kAXSizeAttribute` attributes on AXUIElement provide exact bounds. `CGPoint` and `CGSize` can be extracted via `AXValueGetValue(AXValueType::CGPoint, ...)` and `AXValueGetValue(AXValueType::CGSize, ...)`.
+
+## Proposed Solutions
+
+### Option A: Implement bounds extraction via AX attributes (Recommended)
+```rust
+fn read_bounds(el: &AXElement) -> Option<Rect> {
+    let pos = read_cgpoint(el, "AXPosition")?;
+    let size = read_cgsize(el, "AXSize")?;
+    Some(Rect { x: pos.x as f32, y: pos.y as f32,
+                width: size.width as f32, height: size.height as f32 })
+}
+```
+Extract from `kAXPositionAttribute` (returns `CGPoint`) and `kAXSizeAttribute` (returns `CGSize`) using `AXValueGetValue`.
+- **Effort:** Small
+- **Risk:** Low — standard AX API
+
+### Option B: Batch fetch position+size with other attributes
+Include `kAXPositionAttribute` and `kAXSizeAttribute` in the multi-attribute batch fetch (issue 007). Parse bounds there.
+- **Effort:** Small (part of the batch fetch fix)
+- **Risk:** Low — natural companion to issue 007
+
+## Recommended Action
+
+Option B: implement as part of the batch fetch fix (issue 007). Both need to touch the same tree-traversal code.
+
+## Technical Details
+
+- **File:** `crates/macos/src/tree.rs`
+- **Line:** 160
+- **Component:** macOS tree builder, bounds extraction
+
+## Acceptance Criteria
+
+- [ ] `snapshot --include-bounds` returns non-null bounds for all rendered elements
+- [ ] `get bounds @e5` returns `{x, y, width, height}`
+- [ ] `bounds_hash` in RefEntry has meaningful entropy (not all-same for all elements)
+
+## Work Log
+
+- 2026-02-19: Finding identified by git-history-analyzer agent
--- a/todos/024-complete-p1-is-check-hardcoded-stubs.md
+++ b/todos/024-complete-p1-is-check-hardcoded-stubs.md
@ -0,0 +1,60 @@
+---
+status: pending
+priority: p1
+issue_id: "024"
+tags: [code-review, correctness, commands]
+---
+
+# is_check Command Returns Hardcoded Stub Values
+
+## Problem Statement
+
+The `is` command (is visible, is enabled, is checked, is focused, is expanded) resolves the element handle then completely ignores it. All five properties return hardcoded values: `visible=true`, `enabled=!role.is_empty()`, `checked=false`, `focused=false`, `expanded=false`. No live AX query is made.
+
+## Findings
+
+**File:** `crates/core/src/commands/is_check.rs:28-34`
+
+```rust
+let result = match args.property {
+    IsProperty::Visible  => true,                    // always true
+    IsProperty::Enabled  => !entry.role.is_empty(), // heuristic, not AX
+    IsProperty::Checked  => false,                   // always false
+    IsProperty::Focused  => false,                   // always false
+    IsProperty::Expanded => false,                   // always false
+};
+```
+
+The `_handle` from `resolve_element` is discarded. An AI agent using `is checked @e3` to verify a checkbox state will always get `false`. The `states` field is also absent from `RefEntry`, making cache-based answers impossible too.
+
+## Proposed Solutions
+
+### Option A: Add states to RefEntry, answer from cache (Recommended)
+Add `states: Vec<String>` to `RefEntry`. Populate it during snapshot from `AccessibilityNode.states`. Answer `is_check` queries from the cached states.
+- **Effort:** Medium
+- **Risk:** Low — states are already in AccessibilityNode
+
+### Option B: Add query_states to PlatformAdapter
+Add `fn query_states(&self, handle: &NativeHandle) -> Result<Vec<String>, AdapterError>`. Implement in macOS adapter via `kAXEnabledAttribute`, `kAXFocusedAttribute`, etc.
+- **Effort:** Medium
+- **Risk:** Low — clean API addition
+
+## Recommended Action
+
+Option A: populate states into RefEntry during snapshot. This fixes `is_check`, `get states`, and improves bounds detection in one change.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/is_check.rs`
+- **Lines:** 28–34
+
+## Acceptance Criteria
+
+- [ ] `is checked @e3` returns the actual checked state from the AX tree
+- [ ] `is focused @e3` returns the actual focus state
+- [ ] `is expanded @e3` returns the actual expanded state
+- [ ] No hardcoded return values remain
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/025-complete-p1-get-command-null-value-bounds-states.md
+++ b/todos/025-complete-p1-get-command-null-value-bounds-states.md
@ -0,0 +1,58 @@
+---
+status: pending
+priority: p1
+issue_id: "025"
+tags: [code-review, correctness, commands]
+---
+
+# get Command Returns null for value, bounds, and states
+
+## Problem Statement
+
+The `get` command supports 6 properties (text, value, title, bounds, role, states) but three of them always return null/empty regardless of the element's actual content. `get value`, `get bounds`, and `get states` are non-functional stubs.
+
+## Findings
+
+**File:** `crates/core/src/commands/get.rs:22-29`
+
+```rust
+GetProperty::Value  => json!(null),   // never implemented
+GetProperty::Bounds => json!(null),   // never implemented
+GetProperty::States => json!([]),     // never implemented
+```
+
+An AI agent calling `get @e3 --property value` on a text field will always receive `null`. This makes the command useless for the primary use cases of verifying form field content and reading UI state.
+
+Root cause: `RefEntry` does not store `value` or `states`; bounds are not populated (see issue 023).
+
+## Proposed Solutions
+
+### Option A: Store value and states in RefEntry (Recommended)
+Add `value: Option<String>` and `states: Vec<String>` to `RefEntry`. Populate from `AccessibilityNode` during snapshot. Answer from cache in get command.
+- **Effort:** Medium
+- **Risk:** Low
+
+### Option B: Live-query via PlatformAdapter
+Add `query_attribute(handle, attr) -> Result<Value, AdapterError>` to the trait. Call on-demand in get command.
+- **Effort:** Medium
+- **Risk:** Low — allows reading attributes not captured at snapshot time
+
+## Recommended Action
+
+Option A for value and states (cache from snapshot). Bounds from issue 023 fix. Both needed before Phase 1 is shippable.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/get.rs`
+- **Lines:** 22–29
+
+## Acceptance Criteria
+
+- [ ] `get @e3 --property value` returns the text content of the element
+- [ ] `get @e3 --property states` returns array of state strings
+- [ ] `get @e3 --property bounds` returns `{x, y, width, height}` (depends on issue 023 fix)
+- [ ] No `json!(null)` stubs remain in get.rs
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/026-complete-p1-interactive-roles-split-brain.md
+++ b/todos/026-complete-p1-interactive-roles-split-brain.md
@ -0,0 +1,61 @@
+---
+status: pending
+priority: p1
+issue_id: "026"
+tags: [code-review, architecture, correctness]
+---
+
+# Duplicate INTERACTIVE_ROLES Definition — Split-Brain Role Classification
+
+## Problem Statement
+
+The list of interactive roles (which elements receive `@ref` IDs) is defined in two separate places that can drift independently. Adding a new role to one but not the other causes inconsistency: elements get refs without being counted as interactive, or vice versa.
+
+## Findings
+
+**File 1:** `crates/core/src/snapshot.rs:8-11`
+```rust
+const INTERACTIVE_ROLES: &[&str] = &[
+    "button", "textfield", "checkbox", "link", ...
+];
+```
+
+**File 2:** `crates/macos/src/roles.rs:36-52`
+```rust
+pub fn is_interactive_role(role: &str) -> bool {
+    matches!(role, "button" | "textfield" | "checkbox" | "link" | ...)
+}
+```
+
+`is_interactive_role` in `roles.rs` is exported but never called anywhere in the codebase. The two lists are currently identical but have no compile-time guarantee of staying in sync.
+
+## Proposed Solutions
+
+### Option A: Single definition in core, import in platform crates (Recommended)
+Keep `INTERACTIVE_ROLES` in `core/src/snapshot.rs` (or move to `core/src/roles.rs`). Delete `is_interactive_role` from `macos/src/roles.rs`. Platform crates that need it import from core.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Generate from a shared macro
+Define the role list once as a macro that produces both the slice and the match pattern. Compile-time enforcement.
+- **Effort:** Medium
+- **Risk:** Low — over-engineering for this size
+
+## Recommended Action
+
+Option A: delete `is_interactive_role` from macos crate, use `INTERACTIVE_ROLES` from core everywhere.
+
+## Technical Details
+
+- **Files:** `crates/core/src/snapshot.rs:8-11`, `crates/macos/src/roles.rs:36-52`
+- **Component:** ref allocation, interactive role classification
+
+## Acceptance Criteria
+
+- [ ] Interactive role list has exactly one definition
+- [ ] `is_interactive_role` in macos roles.rs is removed
+- [ ] Adding a new role requires changing exactly one file
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/027-complete-p1-scroll-uses-press-action.md
+++ b/todos/027-complete-p1-scroll-uses-press-action.md
@ -0,0 +1,67 @@
+---
+status: pending
+priority: p1
+issue_id: "027"
+tags: [code-review, correctness, commands, macos]
+---
+
+# Scroll Action Uses kAXPressAction Instead of Scroll Event
+
+## Problem Statement
+
+The `scroll` command is implemented as an AX press action, not a scroll event. The direction and amount parameters are silently discarded. Any agent using `scroll @e5 --direction down --amount 5` will click the element instead of scrolling.
+
+## Findings
+
+**File:** `crates/macos/src/actions.rs:82-85`
+
+```rust
+Action::Scroll(_, _) => {
+    let ax_action = CFString::new(kAXPressAction);
+    let _ = unsafe { AXUIElementPerformAction(el.0, ax_action.as_concrete_TypeRef()) };
+}
+```
+
+`kAXPressAction` performs a click. `Direction` and `u32` scroll amount are received but immediately pattern-matched with `_`. Scroll requires `CGEventCreateScrollWheelEvent(source, kCGScrollEventUnitLine, 2, deltaY, deltaX, 0)` dispatched to the window.
+
+## Proposed Solutions
+
+### Option A: Implement via CGEventCreateScrollWheelEvent (Recommended)
+```rust
+Action::Scroll(direction, amount) => {
+    let (dx, dy) = match direction {
+        Direction::Down  => (0, -(amount as i32)),
+        Direction::Up    => (0,  (amount as i32)),
+        Direction::Right => (-(amount as i32), 0),
+        Direction::Left  => ( (amount as i32), 0),
+    };
+    // CGEventCreateScrollWheelEvent + CGEventPost
+}
+```
+- **Effort:** Small
+- **Risk:** Low — standard macOS input event API
+
+### Option B: Use AXScrollArea scroll actions
+Some scrollable elements expose `kAXScrollDownAction`, `kAXScrollUpAction`, etc. as AX actions. More limited but simpler.
+- **Effort:** Tiny
+- **Risk:** Medium — not all scroll targets support AX scroll actions
+
+## Recommended Action
+
+Option A: use `CGEventCreateScrollWheelEvent`. Correct, universal, matches how real input works.
+
+## Technical Details
+
+- **File:** `crates/macos/src/actions.rs`
+- **Lines:** 82–85
+- **Component:** scroll action implementation
+
+## Acceptance Criteria
+
+- [ ] `scroll @e5 --direction down --amount 3` scrolls 3 lines down
+- [ ] Direction and amount parameters are used (not discarded)
+- [ ] The element is not clicked as a side effect
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/028-complete-p2-list-windows-unstable-ids.md
+++ b/todos/028-complete-p2-list-windows-unstable-ids.md
@ -0,0 +1,55 @@
+---
+status: pending
+priority: p2
+issue_id: "028"
+tags: [code-review, correctness, macos]
+---
+
+# list_windows Assigns Window IDs as Sequential Line Indices (Not Stable)
+
+## Problem Statement
+
+Window IDs are assigned by line number in the `osascript` output (`w-1`, `w-2`, ...). If any filter is applied or lines change order, IDs become inconsistent across calls. The `is_focused` heuristic (first-listed window = focused) is also incorrect.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:229, 249-256`
+
+```rust
+id: format!("w-{}", idx + 1),  // index of osascript output line
+is_focused: idx == 0,          // first line != focused window
+```
+
+Window IDs must be stable identifiers so that `focus-window --window-id w-3` always refers to the same window regardless of how many windows are open. The CGWindow API provides `kCGWindowNumber` (a stable integer per-session) for this purpose.
+
+## Proposed Solutions
+
+### Option A: Use CGWindowNumber as window ID
+`CGWindowListCopyWindowInfo` returns `kCGWindowNumber` (stable per session), `kCGWindowOwnerName` (app name), `kCGWindowName` (title), `kCGWindowOwnerPID`. IDs become `w-{cgWindowNumber}`.
+- **Effort:** Medium (part of osascript replacement — see issue 008)
+- **Risk:** Low
+
+### Option B: Use {pid}-{title-hash} as window ID
+Compute `w-{pid}-{hash(title)}`. Stable as long as title and pid don't change.
+- **Effort:** Small
+- **Risk:** Medium — hash collisions, not deterministic for untitled windows
+
+## Recommended Action
+
+Option A: implement as part of the `CGWindowListCopyWindowInfo` migration (issue 008). Same PR.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 229, 249–256
+- **Component:** list_windows, window ID assignment
+
+## Acceptance Criteria
+
+- [ ] Window IDs are stable across consecutive `list-windows` calls
+- [ ] `is_focused` reflects the actual frontmost window
+- [ ] `focus-window --window-id {id}` reliably targets the correct window
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/029-complete-p2-mem-forget-fragile-execute-action.md
+++ b/todos/029-complete-p2-mem-forget-fragile-execute-action.md
@ -0,0 +1,61 @@
+---
+status: pending
+priority: p2
+issue_id: "029"
+tags: [code-review, memory-safety, macos]
+---
+
+# execute_action_impl Uses mem::forget — Fragile Memory Management
+
+## Problem Statement
+
+`execute_action_impl` calls `CFRetain` on the AX element pointer, wraps it in `AXElement`, then calls `std::mem::forget(el)` to prevent the destructor from releasing it. This is correct only when `perform_action` succeeds. On the error path (`?`), `forget` is never reached, so `drop` calls `CFRelease`, netting zero change — correct by accident. This pattern is extremely fragile: any change to the error path could silently create a double-release or memory leak.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:112-116`
+
+```rust
+unsafe { CFRetain(handle.as_raw() as CFTypeRef) };
+let el = AXElement(handle.as_raw() as AXUIElementRef);
+let result = crate::actions::perform_action(&el, &action)?;
+std::mem::forget(el);   // Only reached on success path
+```
+
+If `perform_action` returns an error, `?` returns early. `el` goes out of scope, calling `CFRelease`. Since `CFRetain` was called above, this results in a net no-op — but only by coincidence of control flow, not by design.
+
+## Proposed Solutions
+
+### Option A: Use ManuallyDrop instead of forget (Recommended)
+```rust
+let el = ManuallyDrop::new(AXElement(handle.as_raw() as AXUIElementRef));
+let result = crate::actions::perform_action(&*el, &action)?;
+// ManuallyDrop prevents drop whether success or error
+```
+- **Effort:** Tiny
+- **Risk:** Low — explicit, correct, and self-documenting
+
+### Option B: Don't retain; use a reference
+If `el` borrows the pointer without taking ownership (no CFRetain), the `AXElement` destructor's `CFRelease` would be incorrect. Instead, don't create an `AXElement` at all — call the AX API directly with the raw pointer.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option A: `ManuallyDrop`. Makes intent explicit and correct for both success and error paths.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 112–116
+- **Component:** execute_action_impl
+
+## Acceptance Criteria
+
+- [ ] Memory management is correct on both success and error paths
+- [ ] No `std::mem::forget` in action execution path
+- [ ] Comment explains retain/release semantics
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/030-complete-p2-find-command-overwrites-refmap.md
+++ b/todos/030-complete-p2-find-command-overwrites-refmap.md
@ -0,0 +1,59 @@
+---
+status: pending
+priority: p2
+issue_id: "030"
+tags: [code-review, architecture, correctness]
+---
+
+# find Command Always Runs Full Snapshot, Silently Overwrites RefMap
+
+## Problem Statement
+
+The `find` command unconditionally runs a full snapshot, which as a side-effect replaces `~/.agent-desktop/last_refmap.json`. An agent calling `find` mid-workflow invalidates all refs it was using without any warning. The command also uses default snapshot options (`max_depth=10`) regardless of the current context.
+
+## Findings
+
+**File:** `crates/core/src/commands/find.rs:12-13`
+
+```rust
+let opts = crate::adapter::TreeOptions::default();
+let result = snapshot::run(adapter, &opts, args.app.as_deref(), None)?;
+```
+
+An agent workflow: `snapshot → click @e3 → find "submit" → click @e3` — the second click will fail because `find` replaced the refmap, making `@e3` stale. The agent has no way to know this happened.
+
+## Proposed Solutions
+
+### Option A: Make find non-destructive — use a temporary refmap (Recommended)
+Run the snapshot for find into a temp `RefMap` in memory. Do not write to disk. Return matching elements with their refs. The agent's current refmap is preserved.
+- **Effort:** Medium
+- **Risk:** Low
+
+### Option B: Accept existing refmap for find
+Add a `--no-snapshot` flag: skip the snapshot, search the existing refmap entries for matching text. Only re-snapshot if element not found.
+- **Effort:** Small
+- **Risk:** Medium — may miss newly appeared elements
+
+### Option C: Document the side-effect
+Add a clear warning in the JSON response: `"warning": "refmap was replaced; previous refs are now stale"`.
+- **Effort:** Tiny
+- **Risk:** None — but doesn't fix the underlying issue
+
+## Recommended Action
+
+Option C immediately (document), Option A in a follow-up. The side-effect is architectural; full fix needs design thought.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/find.rs`
+- **Lines:** 12–13
+- **Component:** find command, snapshot interaction
+
+## Acceptance Criteria
+
+- [ ] `find` side-effect (refmap replacement) is documented in JSON response
+- [ ] Longer term: `find` does not silently invalidate an agent's working refmap
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/031-complete-p2-permissions-request-not-wired.md
+++ b/todos/031-complete-p2-permissions-request-not-wired.md
@ -0,0 +1,63 @@
+---
+status: pending
+priority: p2
+issue_id: "031"
+tags: [code-review, correctness, commands]
+---
+
+# permissions --request Flag Does Not Call Adapter (System Dialog Never Triggered)
+
+## Problem Statement
+
+The `permissions --request` flag returns a stub response claiming the dialog was triggered, but never calls any adapter method. The macOS adapter has a working `check_with_request()` function that calls `AXIsProcessTrustedWithOptions(prompt: true)`, but it is never invoked.
+
+## Findings
+
+**File:** `crates/core/src/commands/permissions.rs:9-13`
+
+```rust
+if args.request {
+    return Ok(json!({
+        "requested": true,
+        "note": "Permission dialog triggered via --request flag"
+    }));
+}
+```
+
+The `PlatformAdapter` is not even passed to this code path. The macOS adapter has `check_with_request()` implemented in `crates/macos/src/permissions.rs` but it is not exposed via the `PlatformAdapter` trait.
+
+## Proposed Solutions
+
+### Option A: Add request_permissions() to PlatformAdapter (Recommended)
+```rust
+fn request_permissions(&self) -> PermissionStatus {
+    self.check_permissions()  // default: no-op
+}
+```
+MacOS adapter overrides this to call `AXIsProcessTrustedWithOptions`. The command calls `adapter.request_permissions()`.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Call check_with_request via downcast
+Downcast `&dyn PlatformAdapter` to `MacOSAdapter` when on macOS. Fragile.
+- **Effort:** Small
+- **Risk:** High — breaks dependency inversion
+
+## Recommended Action
+
+Option A: add `request_permissions()` to the trait with a default no-op implementation.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/permissions.rs:9-13`, `crates/macos/src/permissions.rs`
+- **Component:** permissions command, PlatformAdapter trait
+
+## Acceptance Criteria
+
+- [ ] `permissions --request` triggers the system accessibility permission dialog on macOS
+- [ ] `PlatformAdapter` trait has `request_permissions()` method
+- [ ] Response reflects the actual permission state after the dialog interaction
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/032-complete-p2-screenshot-ignores-app-argument.md
+++ b/todos/032-complete-p2-screenshot-ignores-app-argument.md
@ -0,0 +1,62 @@
+---
+status: pending
+priority: p2
+issue_id: "032"
+tags: [code-review, correctness, commands]
+---
+
+# screenshot --app Argument Silently Ignored
+
+## Problem Statement
+
+When `screenshot --app Safari` is specified without a `--window-id`, the code takes a full-screen screenshot instead of the app's window. The `_app` variable is bound but never used.
+
+## Findings
+
+**File:** `crates/core/src/commands/screenshot.rs:16-19`
+
+```rust
+let target = match (&args.window_id, &args.app) {
+    (Some(id), _)       => ScreenshotTarget::Window(id.clone()),
+    (None, Some(_app))  => ScreenshotTarget::FullScreen,  // _app ignored!
+    (None, None)        => ScreenshotTarget::FullScreen,
+};
+```
+
+The second arm should resolve the app name to a window ID via `list_windows` (as `snapshot::run` does) and produce `ScreenshotTarget::Window`.
+
+## Proposed Solutions
+
+### Option A: Resolve app to window ID via list_windows (Recommended)
+```rust
+(None, Some(app)) => {
+    let windows = adapter.list_windows(&WindowFilter { app: Some(app.clone()), ..Default::default() })?;
+    let win = windows.first().ok_or_else(|| AppError::not_found("No window found for app"))?;
+    ScreenshotTarget::Window(win.id.clone())
+}
+```
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Return INVALID_ARGS if --app without --window-id
+Reject the combination and tell the caller to use `list-windows` first.
+- **Effort:** Tiny
+- **Risk:** Low — honest, explicit
+
+## Recommended Action
+
+Option A: consistent with how `snapshot` handles the same case.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/screenshot.rs`
+- **Lines:** 16–19
+
+## Acceptance Criteria
+
+- [ ] `screenshot --app Safari` captures Safari's frontmost window
+- [ ] The `_app` binding is replaced with actual usage
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/033-complete-p3-duplicate-absolute-max-depth.md
+++ b/todos/033-complete-p3-duplicate-absolute-max-depth.md
@ -0,0 +1,48 @@
+---
+status: pending
+priority: p3
+issue_id: "033"
+tags: [code-review, architecture, duplication]
+---
+
+# ABSOLUTE_MAX_DEPTH Constant Duplicated in core and macos
+
+## Problem Statement
+
+`ABSOLUTE_MAX_DEPTH = 50` is defined in both `crates/core/src/snapshot.rs` and `crates/macos/src/tree.rs`. If the two values diverge (during debugging or tuning), the limits enforce independently and silently.
+
+## Findings
+
+**File 1:** `crates/core/src/snapshot.rs:13` — `const ABSOLUTE_MAX_DEPTH: u8 = 50;`
+**File 2:** `crates/macos/src/tree.rs:4` — `const ABSOLUTE_MAX_DEPTH: u8 = 50;`
+
+The platform crate constant is the actual enforced limit during tree traversal. The core constant is used for the depth-cap validation. If macos is set to 30 but core is set to 50, the core validation would accept max_depth=45 which the macOS traversal would then silently cap at 30.
+
+## Proposed Solutions
+
+### Option A: Define in core, re-export for platform use (Recommended)
+Keep in `core`, export as `pub const ABSOLUTE_MAX_DEPTH`. Platform crates import from core.
+- **Effort:** Tiny
+- **Risk:** Low
+
+### Option B: Use a shared constants module
+Move to `core/src/constants.rs` and re-export through `lib.rs`.
+- **Effort:** Tiny
+- **Risk:** Low
+
+## Recommended Action
+
+Option A: single definition in core, import in macos tree.rs.
+
+## Technical Details
+
+- **Files:** `crates/core/src/snapshot.rs:13`, `crates/macos/src/tree.rs:4`
+
+## Acceptance Criteria
+
+- [ ] `ABSOLUTE_MAX_DEPTH` has exactly one definition
+- [ ] Changing the value in one place updates both behaviors
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/034-complete-p3-apperror-code-duplicates-serialization.md
+++ b/todos/034-complete-p3-apperror-code-duplicates-serialization.md
@ -0,0 +1,61 @@
+---
+status: pending
+priority: p3
+issue_id: "034"
+tags: [code-review, architecture, duplication]
+---
+
+# AppError::code() Duplicates ErrorCode Serde Serialization
+
+## Problem Statement
+
+`ErrorCode` uses `#[serde(rename_all = "SCREAMING_SNAKE_CASE")]` for automatic serialization. But `AppError::code()` is a manually maintained match that maps enum variants to the same hardcoded strings. When a new error code is added, it must be added in 3 places: the enum, the serde attribute (automatic), and the `code()` match. The `main.rs` response builder uses `e.code()` (the string method) rather than serializing through serde, bypassing the single source of truth.
+
+## Findings
+
+**File:** `crates/core/src/error.rs:107-126`
+
+```rust
+pub fn code(&self) -> &str {
+    match self {
+        AppError::PermissionDenied => "PERM_DENIED",
+        AppError::ElementNotFound  => "ELEMENT_NOT_FOUND",
+        // ... manually duplicated
+    }
+}
+```
+
+## Proposed Solutions
+
+### Option A: Derive code() from serde (Recommended)
+```rust
+pub fn code(&self) -> String {
+    serde_json::to_value(self.error_code())
+        .and_then(|v| v.as_str().map(String::from)
+        .unwrap_or("INTERNAL")
+}
+```
+- **Effort:** Small
+- **Risk:** Low — eliminates manual maintenance
+
+### Option B: Use strum's Display derive
+Add `strum` crate, derive `Display` with `SCREAMING_SNAKE_CASE` transform on `ErrorCode`.
+- **Effort:** Tiny (add dependency)
+- **Risk:** Low
+
+## Recommended Action
+
+Option A: derive from serde. No new dependencies.
+
+## Technical Details
+
+- **File:** `crates/core/src/error.rs:107-126`
+
+## Acceptance Criteria
+
+- [ ] `AppError::code()` is not a manual match
+- [ ] Adding a new error code variant requires exactly one change location
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/035-complete-p3-response-struct-unused.md
+++ b/todos/035-complete-p3-response-struct-unused.md
@ -0,0 +1,49 @@
+---
+status: pending
+priority: p3
+issue_id: "035"
+tags: [code-review, architecture, dead-code]
+---
+
+# Response Struct in output.rs Unused — Wire Format Can Diverge Silently
+
+## Problem Statement
+
+`core/src/output.rs` defines typed `Response`, `AppContext`, `WindowContext`, and `ErrorPayload` structs. However, `main.rs` builds the response envelope using raw `serde_json::json!()` macros instead. The typed structs and the actual wire format can silently diverge. The `platform_detail` field on `ErrorPayload` is never populated.
+
+## Findings
+
+**File:** `crates/core/src/output.rs` — defines `Response`, etc.
+**File:** `src/main.rs:93-98` — builds response with raw `json!()` macro
+
+The `Response` struct has a `app` field; `main.rs` produces no `app` field. These are already diverged.
+
+## Proposed Solutions
+
+### Option A: Use the typed Response struct in main.rs (Recommended)
+Replace `serde_json::json!({ "version": "1.0", ... })` with `Response { version: "1.0", ... }` and serialize via serde.
+- **Effort:** Small
+- **Risk:** Low — ensures struct and wire format can't diverge
+
+### Option B: Delete output.rs structs
+If the structs are truly never used, remove them to eliminate the confusion.
+- **Effort:** Tiny
+- **Risk:** Low — they're dead code
+
+## Recommended Action
+
+Option A for Phase 3 (MCP server needs typed structs for schema generation). Option B as immediate cleanup if Phase 3 is far off.
+
+## Technical Details
+
+- **Files:** `crates/core/src/output.rs`, `src/main.rs`
+
+## Acceptance Criteria
+
+- [ ] Response envelope is built from typed structs, not raw json! macros
+- [ ] `platform_detail` in error responses is populated when available
+- [ ] The Response struct and actual wire format cannot diverge
+
+## Work Log
+
+- 2026-02-19: Finding identified by architecture-strategist review agent
--- a/todos/036-complete-p3-dead-code-cleanup.md
+++ b/todos/036-complete-p3-dead-code-cleanup.md
@ -0,0 +1,67 @@
+---
+status: pending
+priority: p3
+issue_id: "036"
+tags: [code-review, dead-code, simplicity]
+---
+
+# Dead Code Cleanup — Multiple Verified Unused Items
+
+## Problem Statement
+
+Several symbols were added for anticipated use cases that never materialized. All confirmed dead by code search. Estimated ~175 LOC that can be deleted with no functional change.
+
+## Findings
+
+### output.rs — Entire file is unused (92 LOC)
+`crates/core/src/output.rs` — `Response`, `ErrorPayload`, `AppContext`, `WindowContext` are never instantiated. `main.rs` builds JSON envelopes inline with `serde_json::json!()`. The re-export in `lib.rs:17` is also dead.
+
+### focused_window() — Never called (12 LOC)
+`crates/core/src/adapter.rs:151-153` and `crates/macos/src/adapter.rs:99-103` — `focused_window()` is in the trait and implemented in macOS, but no command, no dispatch arm, and no test ever calls it. The snapshot engine resolves focus via `is_focused` filtering on `list_windows()`.
+
+### SourceError struct — Source field always None (5 LOC)
+`crates/core/src/error.rs:31-33` — `SourceError` is a wrapper type for the `#[source]` annotation on `AdapterError`. But `AdapterError.source: Option<Box<SourceError>>` is always `None` — no code ever constructs a `SourceError`.
+
+### RefEntry.source_app — Always None (never set)
+`crates/core/src/refs.rs` — `source_app: Option<String>` in `RefEntry` is always `None` at construction and never read by any code path.
+
+### VersionArgs.json: bool — Meaningless flag (6 LOC)
+`crates/core/src/commands/version.rs:4-14` — `args.json` is accepted but ignored. The tool exclusively produces JSON for every command; a `--json` mode flag is meaningless.
+
+### is_interactive_role() — Never called (17 LOC)
+`crates/macos/src/roles.rs:36-52` — Duplicates the `INTERACTIVE_ROLES` constant in core. Already tracked in issue 026 but worth deleting in the same PR.
+
+## Proposed Solutions
+
+### Option A: Delete all dead code in one PR (Recommended)
+Delete `output.rs`, `focused_window()` (trait + impl), `SourceError`, `source_app`, `--json` from version, `is_interactive_role`. One PR, ~175 LOC removed.
+- **Effort:** Small
+- **Risk:** Low — all confirmed dead by grep with zero callers
+
+## Recommended Action
+
+Option A: batch deletion. Run `cargo test` and `cargo clippy` after to confirm no regressions.
+
+## Technical Details
+
+| Item | File | LOC |
+|------|------|-----|
+| output.rs (whole file) | crates/core/src/output.rs | 92 |
+| lib.rs re-export | crates/core/src/lib.rs:17 | 1 |
+| focused_window() trait | crates/core/src/adapter.rs:151-153 | 3 |
+| focused_window() macOS | crates/macos/src/adapter.rs:99-103 | 5 |
+| SourceError | crates/core/src/error.rs:31-33 | 3 |
+| AdapterError.source | crates/core/src/error.rs:25-26 | 2 |
+| RefEntry.source_app | crates/core/src/refs.rs | 2 |
+| VersionArgs.json | crates/core/src/commands/version.rs | 6 |
+| is_interactive_role | crates/macos/src/roles.rs:36-52 | 17 |
+
+## Acceptance Criteria
+
+- [ ] All listed items are removed
+- [ ] `cargo clippy --all-targets -- -D warnings` passes
+- [ ] `cargo test --workspace` passes
+
+## Work Log
+
+- 2026-02-19: Finding identified by code-simplicity-reviewer agent
--- a/todos/037-complete-p3-dual-arg-struct-dispatch-boilerplate.md
+++ b/todos/037-complete-p3-dual-arg-struct-dispatch-boilerplate.md
@ -0,0 +1,68 @@
+---
+status: pending
+priority: p3
+issue_id: "037"
+tags: [code-review, simplicity, architecture]
+---
+
+# Dual Arg Struct Pattern — dispatch.rs Is 194 LOC of Field-Copying Boilerplate
+
+## Problem Statement
+
+Every command has two parallel arg structs: one in `cli.rs` (with clap derive) and one in `commands/*.rs` (plain Rust). `dispatch.rs` exists entirely to copy fields between them. The intermediate `commands::*Args` structs add ~80-100 LOC of dead weight with no transformation logic.
+
+## Findings
+
+**File:** `src/dispatch.rs` — 194 LOC, every arm is:
+```rust
+Commands::Snapshot(a) => snapshot::execute(
+    snapshot::SnapshotArgs {
+        app: a.app,
+        window_id: a.window_id,
+        max_depth: a.max_depth,
+        include_bounds: a.include_bounds,
+        interactive_only: a.interactive_only,
+        compact: a.compact,
+    },
+    adapter,
+),
+```
+
+This pattern repeats for every one of 31 commands. The three `parse_*` helpers (`parse_direction`, `parse_get_property`, `parse_is_property`) add minor value; everything else is pure field-copying.
+
+The `commands::*Args` structs are used only inside their `execute()` functions. If `execute()` accepted the cli arg struct directly, the entire dispatch file shrinks to a 60-line straight match.
+
+## Proposed Solutions
+
+### Option A: Remove commands::*Args; use cli structs directly
+Move clap-annotated structs from `cli.rs` to `crates/core/src/commands/*.rs` (or a shared `args.rs`). `dispatch.rs` becomes one-liners.
+- **Effort:** Large (touches all 31 commands)
+- **Risk:** Medium — introduces clap into core, which the current design intentionally avoids
+
+### Option B: Keep separation; add From impls (Recommended)
+Add `impl From<cli::SnapshotArgs> for commands::snapshot::SnapshotArgs` for each command. Dispatch arms become `snapshot::execute(a.into(), adapter)`. Eliminates the field-copy boilerplate while preserving the CLI/core boundary.
+- **Effort:** Medium
+- **Risk:** Low — clean, no architecture change required
+
+### Option C: Accept current pattern as intentional
+Document that the dual-struct pattern is deliberate decoupling. The cost is 80-100 LOC but the benefit is zero clap dependency in core.
+- **Effort:** Tiny
+- **Risk:** None — defer to Phase 3 cleanup
+
+## Recommended Action
+
+Option C for now (document). Option B in a dedicated cleanup PR when the dispatch file grows further. The pattern is intentional but the `From` impl approach would be the right long-term solution.
+
+## Technical Details
+
+- **File:** `src/dispatch.rs` — 194 LOC
+- **Component:** command dispatch layer
+
+## Acceptance Criteria
+
+- [ ] Either From impls exist for all command arg structs, OR
+- [ ] A comment in dispatch.rs explains the intentional decoupling pattern
+
+## Work Log
+
+- 2026-02-19: Finding identified by code-simplicity-reviewer agent
--- a/todos/038-complete-p2-unwrap-violations.md
+++ b/todos/038-complete-p2-unwrap-violations.md
@ -0,0 +1,68 @@
+---
+status: pending
+priority: p2
+issue_id: "038"
+tags: [code-review, correctness, zero-unwrap]
+---
+
+# unwrap() Violations — Breaks Project Zero-unwrap Rule
+
+## Problem Statement
+
+Two locations in non-test code call `unwrap()`, violating the explicit project rule from CLAUDE.md: "Zero `unwrap()` in non-test code." Both are technically safe due to preceding guards, but they create fragile implicit invariants that future refactors can break.
+
+## Findings
+
+### wait.rs — Double get() + unwrap
+**File:** `crates/core/src/commands/wait.rs:47-48`
+
+```rust
+if refmap.get(&ref_id).is_some()
+    && adapter.resolve_element(refmap.get(&ref_id).unwrap()).is_ok()
+```
+
+`get()` is called twice; the second call `unwrap()`s under an assumption that the first check guarantees `Some`. In a Phase 4 daemon scenario where the refmap is hot-reloaded, the entry could be removed between the two calls. Fix:
+```rust
+if let Some(entry) = refmap.get(&ref_id) {
+    if adapter.resolve_element(entry).is_ok() { ... }
+}
+```
+
+### press.rs — unwrap() after is_empty guard
+**File:** `crates/core/src/commands/press.rs:41`
+
+```rust
+let key = parts.last().unwrap();
+```
+
+An `is_empty()` check a few lines earlier makes this safe in current code. But `unwrap()` in non-test code is forbidden by the project rules. Fix:
+```rust
+let Some(key) = parts.last() else {
+    return Err(AppError::invalid_input("Empty key combo"));
+};
+```
+
+## Proposed Solutions
+
+### Option A: Fix both with idiomatic pattern matching (Recommended)
+Apply the `if let` and `let...else` patterns shown above.
+- **Effort:** Tiny
+- **Risk:** Low
+
+## Recommended Action
+
+Option A: immediate fix, two lines changed.
+
+## Technical Details
+
+- **Files:** `crates/core/src/commands/wait.rs:47-48`, `crates/core/src/commands/press.rs:41`
+
+## Acceptance Criteria
+
+- [ ] Zero `unwrap()` calls in non-test code
+- [ ] `cargo clippy --all-targets -- -D warnings` passes
+- [ ] Behavior is identical; no functional change
+
+## Work Log
+
+- 2026-02-19: Finding identified by code-simplicity-reviewer agent
--- a/todos/039-complete-p1-refmap-not-flushed-before-rename.md
+++ b/todos/039-complete-p1-refmap-not-flushed-before-rename.md
@ -0,0 +1,68 @@
+---
+status: pending
+priority: p1
+issue_id: "039"
+tags: [code-review, data-integrity, file-io]
+---
+
+# RefMap Not Flushed Before Rename — Partial Write Survives Process Kill
+
+## Problem Statement
+
+The RefMap write path calls `file.write_all()` then `fs::rename()` without an explicit `file.flush()` or `file.sync_all()`. The `write_all` writes data into the kernel page cache. The `rename` can succeed while content is still unflushed. On process kill or power loss, `last_refmap.json` exists at the destination path but contains truncated or zero bytes. Next load returns a parse error, making all refs stale.
+
+## Findings
+
+**File:** `crates/core/src/refs.rs:69-80`
+
+```rust
+let mut file = OpenOptions::new()
+    .write(true).create(true).truncate(true).mode(0o600)
+    .open(&tmp)?;
+file.write_all(json.as_bytes())?;
+// No flush() or sync_all() here
+std::fs::rename(&tmp, &path)?;
+```
+
+The atomic temp+rename pattern is used correctly for POSIX atomicity at the rename level, but without `flush()` the in-process BufWriter buffer and kernel page cache may not yet be written to disk when `rename` completes.
+
+## Proposed Solutions
+
+### Option A: Add flush() before rename (Recommended)
+```rust
+file.write_all(json.as_bytes())?;
+file.flush()?;
+std::fs::rename(&tmp, &path)?;
+```
+- **Effort:** Tiny
+- **Risk:** Low — `flush()` guarantees BufWriter buffer is written to OS; `sync_all()` is stronger (fsync)
+
+### Option B: Use sync_all() for full durability
+```rust
+file.write_all(json.as_bytes())?;
+file.flush()?;
+file.sync_all()?;  // calls fsync
+std::fs::rename(&tmp, &path)?;
+```
+- **Effort:** Tiny
+- **Risk:** Low — slower on spinning disks, negligible on SSDs
+
+## Recommended Action
+
+Option A for correctness. Option B if crash-safety against power loss is required (SSD-only environments make this nearly free).
+
+## Technical Details
+
+- **File:** `crates/core/src/refs.rs`
+- **Lines:** 69–80
+- **Component:** RefMap write path
+
+## Acceptance Criteria
+
+- [ ] `file.flush()` is called before `fs::rename`
+- [ ] A SIGKILL immediately after `flush()` leaves a valid RefMap file
+- [ ] Empty/truncated RefMap on next load returns structured error, not panic
+
+## Work Log
+
+- 2026-02-19: Finding identified by data-integrity-guardian review agent
--- a/todos/040-complete-p2-launch-app-fabricated-window-info.md
+++ b/todos/040-complete-p2-launch-app-fabricated-window-info.md
@ -0,0 +1,69 @@
+---
+status: pending
+priority: p2
+issue_id: "040"
+tags: [code-review, correctness, macos]
+---
+
+# launch_app Returns Fabricated WindowInfo (pid:0, id:"w-0") When wait=false
+
+## Problem Statement
+
+`launch_app` with `wait: false` returns a fabricated `WindowInfo` with `pid: 0` and `id: "w-0"`. These values do not correspond to any real window. An agent using this ID for `snapshot --window-id w-0` receives `WindowNotFound`. Any snapshot based on `pid: 0` traverses the kernel process.
+
+## Findings
+
+**File:** `crates/macos/src/adapter.rs:349-356`
+
+```rust
+Ok(WindowInfo {
+    id: "w-0".into(),
+    title: id.to_string(),
+    app: id.to_string(),
+    pid: 0,
+    bounds: None,
+    is_focused: true,
+})
+```
+
+This response tells a lie: the window is claimed as `is_focused: true` when it may not even exist yet. The agent has no way to know the returned data is synthetic.
+
+## Proposed Solutions
+
+### Option A: Return an error when wait=false (Recommended)
+Return `AppError` with `ErrorCode::ActionNotSupported` and message "App launched but window not yet available; use wait=true or call list-windows after a delay."
+- **Effort:** Tiny
+- **Risk:** Low — honest failure beats silent wrong data
+
+### Option B: Block briefly with a short timeout
+Wait up to 2 seconds for the app to create its first window, then return. If no window appears, return Option A error.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option C: Return partial info with explicit placeholder flag
+```json
+{ "id": null, "pid": actual_pid, "ready": false }
+```
+Agent can poll `list-windows` until `ready`.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option A immediately. Option B as a follow-up improvement.
+
+## Technical Details
+
+- **File:** `crates/macos/src/adapter.rs`
+- **Lines:** 349–356
+- **Component:** launch_app_impl, no-wait path
+
+## Acceptance Criteria
+
+- [ ] `launch --no-wait` does not return fabricated pid/id
+- [ ] The returned data is either real or an explicit error
+- [ ] `snapshot --window-id {returned-id}` does not fail silently
+
+## Work Log
+
+- 2026-02-19: Finding identified by data-integrity-guardian review agent
--- a/todos/041-complete-p2-emit-json-discards-write-errors.md
+++ b/todos/041-complete-p2-emit-json-discards-write-errors.md
@ -0,0 +1,71 @@
+---
+status: pending
+priority: p2
+issue_id: "041"
+tags: [code-review, data-integrity, correctness]
+---
+
+# emit_json Silently Discards Serialization and Write Errors
+
+## Problem Statement
+
+`emit_json` in `main.rs` discards both the serialization result and the write result with `let _`. The BufWriter is dropped without explicit flush, so flush errors are also silently swallowed. A calling agent receives no JSON on stdout and has no way to distinguish this from a successful empty response vs a pipe error. The process exits with code 0 despite producing no output.
+
+## Findings
+
+**File:** `src/main.rs:122-127`
+
+```rust
+fn emit_json(value: &serde_json::Value) {
+    let stdout = std::io::stdout();
+    let mut writer = BufWriter::new(stdout.lock());
+    let _ = serde_json::to_writer(&mut writer, value);  // error discarded
+    let _ = writer.write_all(b"\n");                     // error discarded
+    // BufWriter drop: flush errors silently swallowed
+}
+```
+
+On a broken pipe (agent killed the read end), this returns normally and `main()` exits 0. The agent gets no output and no error code.
+
+## Proposed Solutions
+
+### Option A: Flush explicitly and handle errors (Recommended)
+```rust
+fn emit_json(value: &serde_json::Value) {
+    let stdout = std::io::stdout();
+    let mut writer = BufWriter::new(stdout.lock());
+    if serde_json::to_writer(&mut writer, value).is_err()
+        || writer.write_all(b"\n").is_err()
+        || writer.flush().is_err()
+    {
+        std::process::exit(3);  // distinct exit code for I/O failure
+    }
+}
+```
+- **Effort:** Tiny
+- **Risk:** Low — explicit flush guarantees output or a known failure
+
+### Option B: Use writeln! and propagate errors to main
+Return `Result<(), io::Error>` from `emit_json`, handle in `main()`.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option A: explicit flush with a distinct exit code (3) for stdout I/O failure.
+
+## Technical Details
+
+- **File:** `src/main.rs`
+- **Lines:** 122–127
+- **Component:** JSON output path
+
+## Acceptance Criteria
+
+- [ ] `emit_json` flushes the BufWriter explicitly
+- [ ] Serialization/write errors result in non-zero exit code
+- [ ] Broken pipe does not produce exit code 0 with no output
+
+## Work Log
+
+- 2026-02-19: Finding identified by data-integrity-guardian review agent
--- a/todos/042-complete-p2-expand-collapse-ax-errors-swallowed.md
+++ b/todos/042-complete-p2-expand-collapse-ax-errors-swallowed.md
@ -0,0 +1,71 @@
+---
+status: pending
+priority: p2
+issue_id: "042"
+tags: [code-review, correctness, macos, error-handling]
+---
+
+# Expand/Collapse/Select/Scroll Silently Swallow AX Errors — Return False Success
+
+## Problem Statement
+
+Four action implementations use `let _ =` to discard the `AXError` return code from `AXUIElementPerformAction`. If the AX call fails (element not actionable, permission changed, element destroyed), the function returns `Ok(ActionResult::new(...))` — a success. The agent receives `"ok": true` and proceeds on false confirmation. `Click` and `SetValue` correctly check the error; Expand/Collapse/Select/Scroll do not.
+
+## Findings
+
+**File:** `crates/macos/src/actions.rs:70-85`
+
+```rust
+Action::Expand => {
+    let ax_action = CFString::new("AXExpand");
+    let _ = unsafe { AXUIElementPerformAction(el.0, ax_action.as_concrete_TypeRef()) };
+    // error discarded — always returns Ok below
+}
+Action::Collapse => { /* same pattern */ }
+Action::Select(_) => { /* maps to kAXPressAction, error discarded */ }
+Action::Scroll(_, _) => { /* maps to kAXPressAction, error discarded */ }
+```
+
+Compare to Click (correct):
+```rust
+let err = unsafe { AXUIElementPerformAction(el.0, ax_action.as_concrete_TypeRef()) };
+if err != kAXErrorSuccess {
+    return Err(AdapterError::action_failed("click", err));
+}
+```
+
+Additionally, Select maps to `kAXPressAction` (a click) discarding the option value, and Scroll maps to `kAXPressAction` discarding direction/amount (covered separately in issue 027).
+
+## Proposed Solutions
+
+### Option A: Apply the same error-check pattern as Click (Recommended)
+```rust
+Action::Expand => {
+    let err = unsafe { AXUIElementPerformAction(el.0, CFString::new("AXExpand").as_concrete_TypeRef()) };
+    if err != kAXErrorSuccess {
+        return Err(AdapterError::action_failed("expand", err));
+    }
+}
+```
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option A: apply the click error-check pattern to all four actions.
+
+## Technical Details
+
+- **File:** `crates/macos/src/actions.rs`
+- **Lines:** 70–85
+- **Component:** Expand, Collapse, Select, Scroll action handlers
+
+## Acceptance Criteria
+
+- [ ] All four actions propagate AX errors as `AppError`
+- [ ] An element that does not support Expand returns `ACTION_NOT_SUPPORTED`
+- [ ] `ok: true` is only returned when the AX action actually succeeded
+
+## Work Log
+
+- 2026-02-19: Finding identified by data-integrity-guardian review agent
--- a/todos/043-complete-p2-interactive-only-drops-container-children.md
+++ b/todos/043-complete-p2-interactive-only-drops-container-children.md
@ -0,0 +1,72 @@
+---
+status: pending
+priority: p2
+issue_id: "043"
+tags: [code-review, correctness, snapshot]
+---
+
+# interactive_only Drops Non-Interactive Containers Before Recursing Into Children
+
+## Problem Statement
+
+When `--interactive-only` is true, the snapshot engine returns non-interactive nodes immediately without recursing into their children. Interactive elements nested inside non-interactive containers (e.g., buttons inside a toolbar group) never receive refs. The agent gets a tree with containers having children, but no actionable refs inside any container.
+
+## Findings
+
+**File:** `crates/core/src/snapshot.rs:92-106`
+
+```rust
+let is_interactive = INTERACTIVE_ROLES.contains(&node.role.as_str());
+
+if is_interactive {
+    node.ref_id = Some(refmap.allocate(entry));
+} else if interactive_only {
+    return node;  // Returns immediately — never recurses into node.children
+}
+```
+
+A toolbar (`role: "group"`) containing 5 buttons: with `interactive_only=true`, the toolbar is returned with `return node` before the children are processed. All 5 buttons get no refs. An agent calling `snapshot --interactive-only` expecting to find refs for toolbar buttons receives empty results.
+
+## Proposed Solutions
+
+### Option A: Recurse into children before early return (Recommended)
+Move the `interactive_only` check to filter the returned node from the tree, not skip recursion:
+```rust
+// Process children first regardless
+let mut processed = allocate_refs_recursive(node, refmap, interactive_only);
+// Then filter: if this node is non-interactive AND has no interactive descendants, skip it
+if interactive_only && !is_interactive && !has_interactive_descendants(&processed) {
+    return None;
+}
+```
+- **Effort:** Medium
+- **Risk:** Low
+
+### Option B: Separate tree-building from ref-allocation
+Build the full tree, allocate refs, then filter out non-interactive nodes in a post-pass. Children of filtered-out nodes bubble up to the parent.
+- **Effort:** Medium
+- **Risk:** Low — cleaner separation of concerns
+
+### Option C: Don't filter nodes; only filter refs
+Keep all nodes in the tree. The `interactive_only` flag only controls whether non-interactive nodes receive a ref_id. The tree structure is preserved but non-interactive nodes have no ref_id.
+- **Effort:** Small
+- **Risk:** Low — but tree output changes
+
+## Recommended Action
+
+Option C for a quick fix (minimal structural change). Option B for the correct long-term design.
+
+## Technical Details
+
+- **File:** `crates/core/src/snapshot.rs`
+- **Lines:** 92–106
+- **Component:** SnapshotEngine, `allocate_refs`
+
+## Acceptance Criteria
+
+- [ ] `snapshot --interactive-only` includes refs for buttons nested inside non-interactive groups
+- [ ] The `interactive_only` flag does not prematurely skip tree traversal
+
+## Work Log
+
+- 2026-02-19: Finding identified by data-integrity-guardian review agent
--- a/todos/044-complete-p3-find-only-returns-interactive.md
+++ b/todos/044-complete-p3-find-only-returns-interactive.md
@ -0,0 +1,67 @@
+---
+status: pending
+priority: p3
+issue_id: "044"
+tags: [code-review, agent-native, commands]
+---
+
+# find Only Returns Elements With ref_id — Non-Interactive Matches Silently Dropped
+
+## Problem Statement
+
+The `find` command searches the tree for matching elements but only emits matches that have a `ref_id`. Non-interactive elements (labels, static text, containers) never get refs and are silently absent from results. An agent cannot distinguish "element not present" from "element present but not interactive." This makes `find` unusable for locating static labels.
+
+## Findings
+
+**File:** `crates/core/src/commands/find.rs:39-49`
+
+```rust
+if role_match && name_match && value_match {
+    if let Some(ref_id) = &node.ref_id {
+        // Only emitted if ref_id is Some
+        matches.push(json!({ "ref": ref_id, ... }));
+    }
+    // Non-interactive match: silently skipped
+}
+```
+
+An agent searching for a heading label to verify page state will get an empty `matches` array even if the heading exists, with no indication of why.
+
+## Proposed Solutions
+
+### Option A: Return non-interactive matches with ref: null (Recommended)
+```json
+{ "ref": null, "role": "text", "name": "Welcome", "interactive": false }
+```
+Agents can check `interactive: false` and understand the element is present but not actionable.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Add --include-static flag
+Only emit non-interactive matches when `--include-static` is passed. Default behavior unchanged.
+- **Effort:** Small
+- **Risk:** Low
+
+### Option C: Document the limitation
+Add `"note"` field to find response: "Only interactive elements with refs are returned."
+- **Effort:** Tiny
+- **Risk:** None
+
+## Recommended Action
+
+Option A: return all matches with an `interactive` flag. Agents should be able to verify element presence regardless of interactivity.
+
+## Technical Details
+
+- **File:** `crates/core/src/commands/find.rs`
+- **Lines:** 39–49
+
+## Acceptance Criteria
+
+- [ ] `find --name "Welcome"` returns the element even if it's a non-interactive label
+- [ ] Non-interactive matches have `ref: null` and `interactive: false`
+- [ ] Interactive matches still have their ref ID
+
+## Work Log
+
+- 2026-02-19: Finding identified by agent-native-reviewer agent
--- a/todos/045-complete-p3-window-info-app-vs-app-name.md
+++ b/todos/045-complete-p3-window-info-app-vs-app-name.md
@ -0,0 +1,64 @@
+---
+status: pending
+priority: p3
+issue_id: "045"
+tags: [code-review, naming, spec-compliance]
+---
+
+# WindowInfo.app Field Diverges from Spec (Should be app_name)
+
+## Problem Statement
+
+The CLAUDE.md architecture spec defines `WindowInfo` as `{ id, title, app_name, pid, bounds }`. The implementation uses `app` instead of `app_name`. The JSON output field is `"app"` rather than `"app_name"`, breaking any agent or tool that reads the spec-compliant field name.
+
+## Findings
+
+**File:** `crates/core/src/node.rs:58`
+
+```rust
+pub struct WindowInfo {
+    pub id: String,
+    pub title: String,
+    pub app: String,   // CLAUDE.md spec says "app_name"
+    pub pid: i32,
+    pub bounds: Option<Rect>,
+    pub is_focused: bool,
+}
+```
+
+JSON output: `{"id": "w-1", "title": "...", "app": "Safari", ...}`
+Spec: `{"id": "w-1", "title": "...", "app_name": "Safari", ...}`
+
+## Proposed Solutions
+
+### Option A: Rename field + add serde rename attribute
+```rust
+#[serde(rename = "app_name")]
+pub app_name: String,
+```
+- **Effort:** Tiny (field rename + fix all usages)
+- **Risk:** Low — but it's a breaking change to the JSON output
+
+### Option B: Add serde rename without renaming field
+Keep field as `app` in Rust code, use `#[serde(rename = "app_name")]` for JSON output.
+- **Effort:** Tiny
+- **Risk:** Low — preserves internal Rust naming while fixing JSON output
+
+## Recommended Action
+
+Option B: `#[serde(rename = "app_name")]` on the existing `app` field. Fixes the JSON output without touching all internal usages.
+
+## Technical Details
+
+- **File:** `crates/core/src/node.rs:58`
+- **Component:** WindowInfo serialization
+
+## Acceptance Criteria
+
+- [ ] `list-windows` JSON output uses `"app_name"` field
+- [ ] `snapshot` response uses `"app_name"` field
+- [ ] Matches the CLAUDE.md spec definition of WindowInfo
+
+## Work Log
+
+- 2026-02-19: Finding identified by pattern-recognition-specialist agent
--- a/todos/046-complete-p3-description-field-always-none.md
+++ b/todos/046-complete-p3-description-field-always-none.md
@ -0,0 +1,63 @@
+---
+status: pending
+priority: p3
+issue_id: "046"
+tags: [code-review, correctness, macos]
+---
+
+# AccessibilityNode.description Always None — AXDescription Attribute Discarded
+
+## Problem Statement
+
+The macOS tree builder fetches `kAXDescriptionAttribute` as a fallback for `name`, but when both `kAXTitleAttribute` and `kAXDescriptionAttribute` are present, the description is discarded. The `AccessibilityNode.description` field is always `None` in all snapshot output, despite being part of the public JSON schema.
+
+## Findings
+
+**File:** `crates/macos/src/tree.rs:62-63, 90`
+
+```rust
+let name = copy_string_attr(el, kAXTitleAttribute)
+    .or_else(|| copy_string_attr(el, kAXDescriptionAttribute));  // Description used as name fallback only
+
+// Later:
+description: None,  // Always None — AXDescriptionAttribute never stored separately
+```
+
+When an element has both a title (`kAXTitleAttribute`) and a description (`kAXDescriptionAttribute`), the description is silently dropped. For screen reader accessibility, the description often carries the most useful context (e.g., "Save the document to disk" on a "Save" button).
+
+## Proposed Solutions
+
+### Option A: Store description separately when both attributes exist
+```rust
+let title = copy_string_attr(el, kAXTitleAttribute);
+let desc = copy_string_attr(el, kAXDescriptionAttribute);
+let name = title.or(desc.clone());
+// ...
+description: desc.filter(|_| title.is_some()),  // Only set when title also exists
+```
+- **Effort:** Small
+- **Risk:** Low
+
+### Option B: Always read description into AccessibilityNode.description
+Read `kAXDescriptionAttribute` unconditionally and store it in `description`, separate from `name`.
+- **Effort:** Small
+- **Risk:** Low
+
+## Recommended Action
+
+Option B: store description separately. More faithful to the AX data model.
+
+## Technical Details
+
+- **File:** `crates/macos/src/tree.rs`
+- **Lines:** 62–63, 90
+- **Component:** macOS tree builder, AccessibilityNode population
+
+## Acceptance Criteria
+
+- [ ] Elements with both AXTitle and AXDescription expose `description` in JSON
+- [ ] `description` field in AccessibilityNode is not always `null`
+
+## Work Log
+
+- 2026-02-19: Finding identified by pattern-recognition-specialist agent
--- a/todos/047-complete-p3-one-command-per-file-violations.md
+++ b/todos/047-complete-p3-one-command-per-file-violations.md
@ -0,0 +1,59 @@
+---
+status: pending
+priority: p3
+issue_id: "047"
+tags: [code-review, naming, architecture]
+---
+
+# One-Command-Per-File Rule Violations (click.rs, clipboard.rs)
+
+## Problem Statement
+
+CLAUDE.md mandates "one command per file; filename matches the command name." Two files violate this:
+1. `click.rs` contains three commands: `click`, `double-click`, `right-click`
+2. `clipboard.rs` contains two commands: `clipboard-get`, `clipboard-set`
+
+## Findings
+
+**File:** `crates/core/src/commands/click.rs`
+- `pub fn execute(...)` → `click`
+- `pub fn execute_double(...)` → `double-click`
+- `pub fn execute_right(...)` → `right-click`
+
+**File:** `crates/core/src/commands/clipboard.rs`
+- `pub fn execute_get(...)` → `clipboard-get`
+- `pub fn execute_set(...)` → `clipboard-set`
+
+Additionally, four files define `pub struct RefArgs { pub ref_id: String }` identically (`focus.rs`, `toggle.rs`, `expand.rs`, `collapse.rs`). This should be a shared type in `helpers.rs`.
+
+## Proposed Solutions
+
+### Option A: Split into separate files (Recommended)
+Create `double_click.rs`, `right_click.rs`, `clipboard_get.rs`, `clipboard_set.rs`. Move shared `RefArgs` to `helpers.rs`.
+- **Effort:** Small
+- **Risk:** Low — mechanical rename, dispatch.rs updated to import from new files
+
+### Option B: Accept as intentional grouping for closely-related variants
+Document the rationale for grouping. The three click variants are truly trivial variations.
+- **Effort:** Tiny
+- **Risk:** None — but violates project rule
+
+## Recommended Action
+
+Option A: split per CLAUDE.md. The `RefArgs` shared type cleanup should be in the same PR.
+
+## Technical Details
+
+- **Files:** `crates/core/src/commands/click.rs`, `crates/core/src/commands/clipboard.rs`
+- **Also:** `focus.rs`, `toggle.rs`, `expand.rs`, `collapse.rs` (duplicate RefArgs)
+
+## Acceptance Criteria
+
+- [ ] `double_click.rs`, `right_click.rs` exist as separate files
+- [ ] `clipboard_get.rs`, `clipboard_set.rs` exist as separate files
+- [ ] `RefArgs` is defined once in `helpers.rs` and imported by commands that need it
+- [ ] All dispatch.rs references updated
+
+## Work Log
+
+- 2026-02-19: Finding identified by pattern-recognition-specialist agent