fix(computer-use): close two approval bypasses and six correctness bugs

[bolichen97]

Problem

A max-effort code review of the merged PR #644 (computer use) surfaced 15 verified findings, and three mirrored reviewer passes over the fix diff surfaced 11 more. Two were reachable authorization bypasses that defeat the feature's entire security model:

• Any mutating tool call could skip interactive approval. hooks.py tested the computer-use read-only auto-approve before the _WRITE_TOOL_KINDS guard. tool_name is the display title, and select_tool_title (acp/_dispatch.py) prefers the LLM-authored description — on_tool_call's own docstring calls it untrusted for security decisions. So once the operator enabled computer use, an edit/execute/write/delete/move/create call that titled itself mcp__kirocrew-computer__computer_get_state was auto-approved. Verified for all six kinds.
• An app token could turn on the operator's desktop automation. PUT /api/computer-use/config had no caller check at all. request["user"] is truthy for an App-Kit-scoped token too, an app declaring permissions.api: ["/api/computer-use"] satisfies app_token_path_allowed (a bare prefix matches on the path boundary — verified), and enable_state.save_state deliberately bypasses is_sensitive_path so the operator's own panel can write a file the agent cannot. An agent able to author an app manifest could therefore read its own .app_secret, mint a token, and flip enabled: true.

Plus six correctness defects and a large body of documentation that outlived the governance model #644 removed.

Why it matters

The keystone computer_use.json is the whole security boundary for a feature that reads every window and synthesizes input into any application. The spec says so explicitly. Both bypasses route around it: one removes the human prompt that is the last thing standing between a prompt-injected agent and a click, the other lets the agent grant itself the capability. The correctness bugs are less severe but user-visible — one made a documented happy path (max_tree_nodes up to 5000, "raise it for dense apps") permanently unusable in a loop the model could not escape.

Fix (symptoms → root cause → change)

Approval bypass. Symptom: a write-kind call auto-approved. Root cause: a title-keyed branch sat above the semantic-kind guard, and the title is agent-controlled. Change: the mutating-kind test is hoisted above every title-keyed fast-path and returns allow (interactive approval). Equivalent for all prior cases — the old kind not in _WRITE_TOOL_KINDS and not kind was already redundant.

Keystone write. Symptom: an app token writes the enable. Root cause: no request["app"] == "" assertion, and the cookie check cannot separate an app from the operator. Change: 403 before the body is read, SEL-audited, matching handlers/kiro_prerequisite.py and messaging.py. The two machine routes already re-assert internal_auth, which an app token can never satisfy.

kCGEventSourceStatePrivate. Symptom: none visible. Root cause: the constant was 1; Apple's CGEventTypes.h declares it -1, and 1 is kCGEventSourceStateHIDSystemState — the shared table whose live modifier state produced the measured abc → ' I Abc' bug this source exists to avoid. Masked only because every path also calls CGEventSetFlags(event, 0). The old test asserted the constant against itself, so it could not catch a wrong value; it now asserts the literal.

Element indices above the config default. Symptom: computer_get_state(max_tree_nodes=2001) shows element 1400, the click is refused "no element at that index", and re-snapshotting reproduces it forever. Root cause: mutating tools take no budget arguments, so both the drift walk and the post-action refresh were built from the 1200 default. Change: Snapshot.walk_budget carries the budget the walk actually used. (Writing the third test exposed the refresh-walk half, which the review had not identified.)

Lossy ceiling round-trip. Symptom: no rects, no (editable), no focus line in any computer_get_state. Root cause: _element_payload/_element_from_payload enumerated 9 of ElementRec's 12 fields. Change: all 12, plus a _frame_from_payload that rejects partial/bool/NaN/inf rects rather than emitting a plausible wrong rectangle. editable is the load-bearing loss — it is the only signal separating a writable field from a read-only one.

post_text partial typing. Symptom: "typing failed" while the app holds half the string. Root cause: per-character encode inside the posting loop; a lone surrogate raises mid-way. Change: encode up front, refuse having typed nothing.

Overlay singleton race. Symptom: two fighting fake cursors and a leaked child process. Root cause: get_shared_overlay skipped its lock claiming callers live on the event loop; its only caller is sync and runs on the 8-worker subprocess_executor. Change: a threading.Lock, matching every sibling singleton.

Silent screenshot suppression. Symptom: screenshot: true on Chrome returns no image and no reason. Root cause: capture_macos refuses on a truncated walk (it cannot prove the window holds no password field) but _render_image_note special-cased only has_secure. Change: TRUNCATED_WINDOW_NOTE, naming the remedy — gated on walk_budget.want_image, because an unconditional note inverted the bug and announced a suppression on every mutating action's refresh (caught by a reviewer on the first version of this fix).

Security-posture blind spot. All ten computer-use schemas were missing from the report (63 → 73). The drift test written to catch that hardcoded the same two registries the implementation did; it now discovers them.

Doc drift. The deleted allow_pointer_move (including a help string served to the dashboard), a 409 and a read_only field neither of which exists in the module, a nonexistent approval-floor clamp, bundle_id/cu_action as governance matchers (naming either raises PlatformCompositionError and aborts governance boot), and operator advice to narrow a computer_use.apps scope that was removed. SKILL.md's only worked example called computer_press_key twice without element_index — a script the code refuses, shipped to every install.

Also deletes gate.targets_axis_is_governed: no caller, and a docstring asserting the inverse of an enforced control.

Tests

Every regression test was verified to fail on the pre-fix code, not just pass on the fixed code.

Test	Locks in
test_hooks.py::TestMutatingKindBeatsTheTitle	all 6 mutating kinds fall through to approval despite a CU title; genuine CU observations still auto-approve (6 fail pre-fix)
test_computer_use_api.py::TestAnAppTokenCannotWriteTheKeystone	app token → 403, keystone untouched, denial audited; dashboard user still works (3 fail pre-fix)
test_computer_use_ffi.py::test_the_private_event_source_constant_is_the_value_apple_declares	the literal -1, not the symbol
test_computer_use_ffi.py::test_post_text_types_NOTHING_when_a_character_cannot_be_encoded	zero CGEventPostToPid calls on an unencodable string
test_mcp_computer.py::TestTheDriftWalkHonoursTheSnapshotBudget	asserts the budget off the driver's own call journal — a behavioural assertion passed while the refresh walk was still wrong
test_computer_use_snapshot.py::TestTheCeilingRoundTripIsLossless	driven off dataclasses.fields, so a NEW field fails rather than being dropped
test_computer_use_snapshot.py::TestASuppressedScreenshotAlwaysSaysSoWhy	both directions (4 fail if the note is removed, 3 if the want_image guard is dropped), incl. one case through real dispatch_tool
test_computer_use_overlay.py::test_the_shared_overlay_is_one_instance_under_thread_contention	8 real threads → 1 instance
test_security_posture.py::test_every_mcp_schema_registry_is_covered_by_the_posture_view	registries discovered, not hardcoded
test_mcp_computer.py::test_no_worked_example_in_the_skill_omits_a_required_element_index	every literal call in the shipped skill is runnable

Secure-field floor re-verified after populating frame/traits/focused in the payload: a secure record still renders exactly 7 textfield <secure> — no title, value, traits, rect or focus marker.

Manual verification

• Apple's CGEventTypes.h read directly from the macOS SDK; both enum values probed live on darwin-arm64 (CGEventSourceGetSourceStateID returns an opaque id for -1, literally 1 for 1).
• The drift loop, the round-trip loss, the false truncation note and the overlay race were each reproduced end-to-end against the packaged fake backend before fixing.
• Keystone sensitive-path floor confirmed intact (is_sensitive_path plus the cat / > / tee shell forms).
• auto never resolves to click_method: "global" — 55 related tests pass.

Screenshots

N/A — no user-visible UI change. The only frontend edit is a corrected doc comment in api/client.ts (the ComputerUseConfigData interface body was already correct).

Gates

pytest 21625 passed · mypy 558 files clean (CI-parity venv, no faiss) · flake8 + isort --check-only clean · scrub-lint clean · tsc -b 0 · vitest 483 files / 5893 passed

[github-actions]

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of 34f4d7cd6ff066c8a2bbfb843402df67a8dae549 — updated in place on each push; does not block merge.

UX-Verdict: PASS

No UI change ships; every touched string improves the experience — the inescapable "no element at that index" loop and the silent screenshot suppression both now explain themselves and name the remedy.

The three model-facing surfaces this PR edits all pass the cold-read test: TRUNCATED_WINDOW_NOTE states what happened and the exact argument to raise; the typing refusal ("…cannot be typed (…); nothing was sent") guarantees the all-or-nothing contract it claims; the corrected SKILL.md worked example is now actually runnable, and its new truncation row tells the model when not to retry. The walk_budget fix removes a dead-end a user following the Settings panel's own "raise it for dense apps" advice would hit every time. The 403 body ("dashboard user required") only ever reaches machine callers. Cursor Motion help copy is clearer than what it replaced.

[UX-REVIEWED] 34f4d7c

[github-actions]

Arbiter — ✅ no blocking findings

Arbiter found no unresolved long-term items that require action before merging 34f4d7cd6ff066c8a2bbfb843402df67a8dae549.

Second-order review for 34f4d7cd6ff066c8a2bbfb843402df67a8dae549; this comment is updated in place on each push.

Review details

Both files read. The line-level reviewers (Opus 5, GPT 5.6) reported no findings, and the UX lane passed — so the only sub-threshold material to arbitrate is the design reviewer's two Watch items and one suggestion. I traced the design reviewer's dead-branch claim against the actual hooks.py diff before ruling.

The claim is textually correct: if kind in _READ_ONLY_TOOL_KINDS: return auto_approve() returns unconditionally before the computer-use branch guarded by the identical prefix condition, so _cu_read_only_auto_approve (class table + keystone check) can never fire, and the new test for it passes vacuously via the first branch. But tracing the decision table shows this changes no production security outcome relative to either the pre-diff code or the intended post-diff design: (a) pre-diff, kind in _READ_ONLY_TOOL_KINDS → auto_approve already existed unconditionally, so a forged kind: "read" skipped the prompt before this PR too — the diff strictly narrows the auto-approve surface (it closes the title-only and omitted-kind bypasses, which were the actual holes); (b) even if the CU branch fired as written, a call forging both the title and kind: "read" would pass both of its checks anyway, so the dead branch removes no defense against the one spoof that remains; (c) the only behavioral delta is a keystone-OFF auto-approval, which the in-band tools._dispatch gate refuses regardless — per this repo's own architecture, the hooks gate is fail-open and never the authoritative computer-use control. What the diff genuinely creates is dead code, a ~30-line comment and spec prose describing a control that cannot fire, and a vacuous test — exactly the "prose outlived the code" defect class, which is real, ironic in this PR, and trivially reversible in a follow-up commit. Not a one-way door (no contract, schema, or persisted data) and not a concrete harm this diff can trigger. The second Watch item (document which producer of kind is trusted) is a one-sentence spec addition for a pre-existing design-level tension; the reformatting note is style.

Arbiter-Verdict: PASS

No sub-threshold finding meets the long-term-impact bar.

Suggested follow-ups (open as issues — non-blocking)

• Dead computer-use auto-approve branch in hooks.py (design reviewer, Watch test: validate CI workflows on KiroCrew #1) — if kind in _READ_ONLY_TOOL_KINDS: return auto_approve() makes the subsequent kind in _READ_ONLY_TOOL_KINDS and _cu_read_only_auto_approve(tool_name) branch unreachable, so its keystone-enable check, its ~30-line comment, the matching prose in security.md, and test_a_genuine_cu_observation_still_auto_approves all describe a control that never fires. No production decision differs (the generic branch yields the same outcomes, and keystone-off calls are refused in-band at tools._dispatch), so this can safely wait — but either merge the CU predicate into the generic read-kind branch (e.g. exclude mcp__kirocrew-computer__* titles from it so CU tools reach only the stricter gate) or delete the dead branch and rewrite the comment/spec/test to state that CU reads ride the generic kind trust. Fix in src/kiro_crew/hooks.py, docs/system-specs/modules/security.md, and test/test_hooks.py in one commit.
• Pin the trust story for tool_kind (design reviewer, Watch refactor: rename project KiroClaw to KiroCrew #2) — the allow-list design trusts kind ∈ {read, fetch} while the same diff's comments call tool_kind "an arbitrary agent-influenced string"; this tension pre-exists the PR (the pre-diff gate trusted the same field the same way) and the PR only narrows it. Add one sentence to docs/system-specs/modules/security.md stating which producer of the ACP kind field is trusted (e.g. kiro-cli deriving it from code-owned MCP annotations) and why — or, if it cannot be pinned, note that the read-kind auto-approve is a convenience above the in-band gates, not a boundary.
• Keep mechanical reformatting out of security-sensitive diffs (design reviewer, Suggestion) — ~150 lines of pure black reformatting in validation.py dilute the reviewed surface of an approval-bypass fix. Process note for future PRs; nothing to change in the code.

[ARBITER-REVIEWED] 34f4d7c

False positive or not applicable? A repository writer can comment:
/ai-review override arbiter 34f4d7cd6ff066c8a2bbfb843402df67a8dae549: <one-sentence reason>

For a broader accepted-risk deferral, apply defer-longterm and explain why.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of 34f4d7cd6ff066c8a2bbfb843402df67a8dae549 — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

The generic kind=read branch returns before the computer-use branch, making the CU gate dead code and half-reopening the bypass this PR exists to close.

Watch

• In hooks.py, if kind in _READ_ONLY_TOOL_KINDS: return auto_approve() executes before if kind in _READ_ONLY_TOOL_KINDS and _cu_read_only_auto_approve(tool_name) — the second branch is unreachable (identical prefix condition, first returns unconditionally). Consequence chain: the diff's own premise is that tool_kind "is an arbitrary agent-influenced string", so a mutating computer_click that titles itself computer_get_state and claims kind: "read" auto-approves via the generic branch — never touching the code-owned class table or the keystone-enable check that _cu_read_only_auto_approve was written to enforce ("no auto-approval can exist while the feature is off"). The dead branch, its ~30-line comment, and test_a_genuine_cu_observation_still_auto_approves (which passes via the first branch even if the CU predicate is patched to False) all describe a control that cannot fire — the exact "prose outlived the code" failure half this diff eradicates. Either the generic read-kind branch must exclude mcp__kirocrew-computer__* titles so CU tools reach only the stricter gate, or the CU branch and its documentation should go and the comments state plainly that CU reads ride the generic kind trust.
• The whole allow-list design rests on tool_kind being trustworthy while the same comments call it agent-influenced. If kiro-cli assigns kind from code-owned MCP annotations that tension is fine, but it is asserted in neither code nor spec — worth one sentence in docs/system-specs/modules/security.md pinning which producer of kind is trusted and why.

Suggestions

• The ~150 lines of pure black-reformatting in validation.py dilute a security-sensitive diff; keep mechanical reformat out of approval-bypass fixes so the reviewed surface is the changed surface.

[DESIGN-REVIEWED] 34f4d7c

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 34f4d7cd6ff066c8a2bbfb843402df67a8dae549 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 34f4d7c

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 34f4d7cd6ff066c8a2bbfb843402df67a8dae549: <one-sentence reason>

[bolichen97]

Disposition for cd836ca9d27a9aa14d8015cee47ceac549af7fdb → now 4d01cb12

Finding	Disposition	Evidence
BLOCKING hooks.py:612 — unknown or omitted ACP kinds still bypass approval	fixed	Confirmed before fixing: with the CU predicate forced true, kind in {"other", "unknown", "switch_mode"} all returned auto_approve.

The denylist was the wrong shape and the finding is right about why: tool_kind is passed through verbatim from the ACP kind field (acp/_dispatch.py:717), so it is an arbitrary agent-influenced string and no enumeration of mutating kinds can be complete.

Inverted to an allow-list: only _READ_ONLY_TOOL_KINDS (read/fetch) auto-approves, every other non-empty kind returns allow (interactive approval), and a title-keyed branch — the computer-use one included — is reachable only when the kind is absent. Verified: read/fetch/absent → auto_approve; other/unknown/switch_mode/search/think/EDIT and all six known mutators → allow.

_WRITE_TOOL_KINDS is kept as documentation of the kinds observed to mutate and is no longer branched on. TestMutatingKindBeatsTheTitle now covers the unknown-kind cases behaviourally and asserts over the AST that on_tool_call references no mutating-kind denylist — reverting to the denylist form fails 7 of its cases.

Local gates on 4d01cb12: pytest 21634 passed · mypy 558 files clean · flake8 + isort clean.

[github-actions]

Opus 5 Review — ✅ no blocking findings

Reviewed 34f4d7cd6ff066c8a2bbfb843402df67a8dae549 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 34f4d7c

Verdict parsed from the review's SHA-scoped output markers for commit 34f4d7cd6ff066c8a2bbfb843402df67a8dae549.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 34f4d7cd6ff066c8a2bbfb843402df67a8dae549: <one-sentence reason>

[bolichen97]

Disposition for 4d01cb12c4951cf6aed4efee216ab82ce582c588 → now 34f4d7cd

Finding	Disposition	Evidence
BLOCKING hooks.py:617 — missing permission kinds still reach spoofable title auto-approval	fixed	Confirmed before fixing: with the CU predicate forced true, an absent kind on …__computer_get_state returned auto_approve.

Correct, and thanks — this is the same class as the last round one level deeper. Two agent-controlled inputs met in that branch and either alone was sufficient: the title (from select_tool_title, which prefers the LLM-authored description) and an omitted kind (indistinguishable from an honest omission). So a computer_click could forge an observation title, send no kind, and skip the prompt.

Fixed by requiring both to agree — the computer-use auto-approve is now reached only under the kind in _READ_ONLY_TOOL_KINDS branch, so an explicit read/fetch is mandatory. Verified: read/fetch → auto_approve; absent/other/unknown/edit/execute → allow (interactive approval).

One deliberate narrowing from your prescribed fix, stated so it can be challenged: I did not return allow for absent kinds in general. _is_read_only_tool — the pre-existing absent-kind fallback — rejects every mcp__kirocrew-computer__* title, so it cannot reach a computer-use auto-approve either way; blocking absent kinds outright would have removed "reads don't nag" for every ordinary tool (ls -la, plain read titles) with no security gain on this surface. That property is now asserted by test_the_generic_fallback_never_matches_a_computer_use_title, so if it ever stopped holding the guarantee could not weaken silently.

Also in this push: removed an orphaned # ── Governance ── header in handlers/computer_use.py that described widen-gating with no code beneath it (flagged by a local AUTOSDE reviewer — the same stale-prose class this PR exists to remove).

Local gates on 34f4d7cd: pytest 21635 passed · mypy 558 files clean · flake8 + isort clean. Opus 5 Review, Design Review, CodeQL, SAST, PR Hygiene, De-Amazon Scrub Lint and all completed test shards were green on 4d01cb12.