feat(desktop): park sensitive tool calls in waiting_approval and emit approval.requested (#20938, PR 1a)

[sauranshbhardwaj]

What changed and why

Kernel half of #20938 (PR 1a of the runtime work; PR 1b adds the relay and wire frames, PR 2 the Swift card). A sensitive on-device tool call (send_message, run_applescript, read_message_history, list_message_chats, list_mail_messages) without a covering scoped grant fails today with approval_required, and nothing in the app can grant one, so the tools shipped in 0.12.444 cannot run for a user. capture_screen has the opposite gap: its bundle (desktop.context.screenshot_image) is already classified sensitive and its description says it requires approval, but it was never in the broker's approval set, so today it takes and shares a full-screen screenshot with no approval at all; this PR adds it to the set with the stable resource screen. A capture is the whole display, including windows the person keeps out of capture elsewhere, so each screenshot is allowed once: its card offers no "Allow for this chat", no grant row covers a capture, and the kernel refuses to mint a grant the card did not offer. This PR gives the kernel the ability to park such a call instead: the capability broker prepares the ledger row and inserts one approval dispatch bound to that exact invocation (id, input hash, daemon boot epoch) in the same transaction, the run and attempt move to waiting_approval, and run.waiting_approval + approval.requested are appended together. resolve_desktop_dispatch gains the two user answers: resolution.decision: "allow" without a grant runs exactly this invocation once and mints nothing; allow with a grant whose runId is null mints a session-scoped grant through the existing insertGrant path, capped at 24 hours. Deny, cancellation, expiry, run termination and owner change fail the prepared invocation closed (failed, never outcome_unknown), each with its own approval.resolved, and the run resumes. A daemon restart turns the prepared invocation into failed, expires the dispatch and records approval.resolved; nothing is replayed.

The six gated tools are validated against their manifest schema at the kernel boundary (unknown keys such as a caller-supplied resource_ref, missing required fields and wrong types are refused as invalid_tool_input), and the resource a grant covers is derived only from to / script / chat_id / handle, so one approval can never cover a different recipient or script. An input the card cannot show in full (over the 4 KiB bound) is refused as input_too_large_to_approve with nothing prepared. allow_session is offered only when there is an exact resource to cover and says what it covers (covers). A dispatch created through create_desktop_dispatch has its invocation payload key stripped, so a model cannot forge a parked tool call. Every relay-callable tool is now classified on purpose (named in a policy set, or declared unbundled with a reason), never by the read-only default, and a test enforces it. A second test holds the approval set to every relay-callable tool in a sensitive bundle, with each deliberate exception named and justified (request_permission: the native macOS prompt plus current-turn consent; fill_cloud_connector_form: not one of this issue's device tools, and it only types into a connector form already open in the signed-in browser, though it takes an arbitrary server_url and can press submit). Screen-image tools that are not gated must stay off every chat relay: get_screenshot and its look_at_frame alias are local-agent-API only, and the realtime voice screenshot, which was advertised to pi-mono with no condition and so sat ungated in every typed chat, pill and workstream run, is now offered only to realtime voice runs through their surface projection. show_rewind_evidence is classed with the screen-history reads: the model gets back the stored frame's title, app and OCR excerpt, and the pixels go only to the person's own Chat turn. Grants for desktop.context.screenshot_image must name an exact resource. A new kernel operation, endChatApprovalScope, ends what a chat granted when the person clears it: the main chat keeps one kernel session across clears, so it closes every approval still parked in that session (cancelled, never run) and revokes every grant the person minted for it (revoked_at_ms, one grants.revoked audit event); PR 1b calls it from the app's journal clear. Authority is revalidated before an approval is admitted, that check runs before the resolving transaction so a revocation it triggers commits on its own, and a rolled-back decision restores the broker's memory. The model still cannot resolve its own dispatch: resolve_desktop_dispatch without trustedUserControl returns policy_denied.

Interim behavior: parking sits behind a kernel switch, AgentRuntimeKernelOptions.desktopToolApprovalsEnabled, default off and not reachable from the daemon in this PR (nothing in index.ts calls the new kernel method yet), so the approval flow itself stays off and a send_message call without a grant still fails immediately with approval_required. Four things do change on the existing path: for the gated tools, an input with unknown keys, a missing required field or a wrong type is now refused as invalid_tool_input, which takes precedence over approval_required; the grant resource for the two list tools is now messages:chats / mail:inbox instead of none, so a session grant can name them (a legacy * grant still matches); and capture_screen, which ran with no approval, now fails with approval_required like the others until a client renders the card (PR 1b and PR 2), so this PR must not ship in a release without them; and typed chat, pill and workstream runs no longer see the realtime voice screenshot tool (realtime voice runs still do). PR 1b wires the relay and a client-declared capability that turns the switch on; PR 2 flips the default and deletes the switch, so this is a temporary dual path, not a lasting one.

Product invariants affected

INV-AGENT-* (product/invariants/agent-control-plane.md), sections Authority ("Physical tool authority is run/attempt scoped and single use") and Lifecycle ("Exactly one non-terminal attempt per run has execution authority", "Silence, disappearance, and UI dismissal are not success"), plus Persistence (state transitions and their events commit in one withTransaction). The ledger transition stays prepared -> dispatched -> succeeded|failed; a parked invocation is prepared, so restart turns it into failed and never outcome_unknown, and holding the invocation object is not dispatch authority. Spec §8.3: approval requests are events and the dispatch row reuses desktop_dispatches; no approvals table. Spec §17: no automatic decision mints a grant; only a user answer with a finite expiry of at most 24 hours does. Guard tests updated: run-tool-capability.test.ts, control-tools.test.ts, sqlite-store.test.ts, desktop-tool-policy.test.ts.

INV-CHAT-1 (product/invariants/chat-continuity.md) is named because desktop/macos/agent/src/runtime/** matches its path glob. Unaffected: this change writes no conversation turns, adds no store outside omi-agentd.sqlite3, and leaves the kernel journal the single transcript owner.

How it was verified

From desktop/macos/agent at the branch tip (10ef09e, rebased on main d3208a0):

• npm ci && npm run build -> clean.
• npx vitest run -> 72 files, 946 tests passed (899 on main d3208a0 + 47 new), 0 failed. tool-surfaces-exhaustiveness.test.ts is in that run; this PR changes one manifest entry (the realtime voice screenshot loses its pi-mono advertisement), so GeneratedToolExecutors.swift (two digests) and tests/fixtures/tool-manifest.json were regenerated with scripts/generate-tool-surfaces.mjs and --check reports no drift.
• desktop/macos/scripts/agent-logic-harness.sh --node-only --skip-install -> "Harness passed." The full harness (Swift focused tests, 684 executed, 0 failures) was run on the combined change before the split; the only Swift change in this PR is the regenerated manifest digests, and xcrun swift build plus the realtime and generated-surface suites (487 tests) pass on the stack top.
• scripts/pr-preflight --base upstream/main --pr-body-file <this body> -> "PR preflight passed: 32 checks" (lane ci, base d3208a0, 16 files). With the machine default Python 3.10 the run stops at feature-flag-registry because backend/routers/conversations.py uses except* (3.11 syntax); the passing run used Python 3.11. scripts/failure-class prepare -> OK, no fix: commits.
• python3 .github/scripts/check_agent_doc_references.py -> OK; python3 .github/scripts/desktop-changelog.py validate -> OK.

Not verified: the real user-facing path. With the switch off, the park/resolve flow is exercised end to end only through the real kernel, broker and SQLite store in tests (a live acp-lane run with a deferred adapter, resolved through handleAgentControlToolCall), not through a model turn or the dev bridge. No packaged-app smoke was run, including realtime voice after the screenshot projection change.

Tests

Core path (control-tools.test.ts, "per-invocation device tool approvals"): with the switch off the kernel still rejects approval_required and writes nothing; with it on, a live acp-lane run parks in waiting_approval and records approval.requested in the UX-doc shape; allow once admits one dispatch, mints no grant, resumes the run and refuses a replay; allow for this chat mints a session-scoped grant that covers the same recipient and not another; deny fails the invocation and the run continues; a decision-less resolution is rejected; the model's own resolve_desktop_dispatch returns policy_denied with the dispatch still pending; a tampered payload admits nothing; expiry and run cancellation close the card with an audited approval.resolved; an owner change closes the card, records approval.resolved and frees the run; a resolution refused for stale authority (wrong owner) still cancels the dispatch durably; a grant over 24 hours is refused and the card stays answerable; ending a chat's approval scope closes its parked card and revokes only that session's user grants (not a policy grant, not another session's), refuses another owner, is idempotent with no second audit event, and the same send then asks again; a session grant for a parked capture_screen is refused because the card never offered one, nothing is minted, and the card is then allowed once; a decision whose transaction fails (store write made to throw once, for both resolve_desktop_dispatch and the system termination path) leaves the dispatch pending, the ledger row prepared and the broker still refusing dispatch, and the card then resolves normally; a model-created approval dispatch has its invocation key stripped and remains an ordinary dismissible item.

Error paths and restart (run-tool-capability.test.ts, "desktop tool approvals"): a caller-supplied resource_ref, a missing text and a wrong type are refused as invalid_tool_input with nothing prepared, and a grant for bob never covers mallory; too-large input -> input_too_large_to_approve; a hard policy deny never parks; owner mismatch and a superseded attempt at approve time revoke and close the card; owner revocation reports the closed approvals; restorePendingApproval round trip; the broker gate (holding the invocation object is not dispatch authority); binding mismatch; deny; a covering grant skipping the dispatch; stable list-tool resource refs; capture_screen parking under screen with Allow Once and Deny only, and a grant row for screen not covering the next capture; every relay-callable tool explicitly classified; the approval set matching every relay-callable sensitive-bundle tool except the named exceptions; get_screenshot and screenshot staying off the relays; show_rewind_evidence classed as stored-frame text; every gated tool being sensitive and relay-callable; the voice screenshot absent from pi-mono main chat, floating bar, floating pill, task chat and workstream runs and present in a realtime voice run; a terminal attempt cancelling the dispatch; a daemon restart (prepared -> failed with never_auto_retry, dispatch expired, one approval.resolved, idempotent second reconciliation). sqlite-store.test.ts covers reconciliation at the store contract. omi-tool-manifest.test.ts pins the voice screenshot to the realtime voice surface and off the pi-mono and stdio projections. desktop-tool-policy.test.ts covers the request builder (options, default deny, allow_session omitted without a resource, covers text, the plain screenshot card copy with Allow Once and Deny only, no grant covering a capture, an unscoped screen-image grant refused, preview allowlist, path redaction, 4 KiB bound, TTL).

UI evidence

UI-Evidence: none -- runtime-only change

Failure class (fixes)

Failure-Class: none

[Git-on-my-level]

Thanks @sauranshbhardwaj — this is carefully executed security work. Reviewing against the current head:

• run-tool-capability.ts: the evaluate/prepare split, per-invocation pendingApprovals, and readPendingApproval's full re-validation (inputHash, daemonBootEpoch, executionGeneration, run/attempt) close the replay/tamper window, and assertNotAwaitingApproval on both markInvocationDispatched and acquireExecutionLease means possession of the invocation object alone cannot dispatch a parked call. The caller-supplied resource_ref hole is genuinely fixed: toolResourceRef now derives the grant target only from to/script/chat_id/handle, and manifest-schema validation at the kernel boundary rejects unknown keys as invalid_tool_input, so one approval can never be widened to a different recipient or script.
• kernel-coordinator.ts: createDesktopDispatch strips the invocation binding key from model-created dispatches; resolveDesktopDispatch checks authority before the mutating transaction and restores the broker's parked state on rollback (restorePendingApproval); revoke paths write the cancelled dispatch row before the ledger row becomes failed — fail-closed ordering throughout. The 24h cap on session grants is enforced here as well.
• kernel-core.ts: derived approval.resolved events publish after the event that caused them, and recordCancelledToolApprovals resumes a run only when nothing is still parked — consistent with resumeRunAfterApproval in the coordinator.
• sqlite-store.ts: startup reconciliation expires invocation-bound approvals regardless of their TTL so nothing survives a daemon restart, with an idempotency test to match; types.ts carries the new expiredToolApprovalDispatchIds field.
• desktop-tool-policy.ts: deny as the default option, a 180s TTL documented against the pi-mono long-wait minus the Swift executor timeout, a 4 KiB preview bound, path redaction to basename, and allow_session offered only when an exact resource exists for the grant to cover.
• Tests are adversarial in the right places: forged dispatch payload, tampered inputHash, model self-approval via the control relay (policy_denied), owner-change revocation, transaction-rollback restore, and daemon-restart expiry in control-tools.test.ts / run-tool-capability.test.ts / sqlite-store.test.ts; desktop-tool-policy.test.ts pins the card copy and preview bounds. Changelog fragment present.

Two non-blocking notes: (1) after a user answer the run/attempt go straight back to running — I could not find an intermediate post-approval status in this diff, so whether that is the intended UX is a call for the surface owner; (2) everything sits behind desktopToolApprovalsEnabled (kernel-types.ts), default off until the Swift card ships — good interim containment, but it also means none of this is exercised in production until PR 2 lands.

Because this reshapes the desktop agent's permission architecture for sensitive device tools, it needs maintainer sign-off on the approval semantics before merge rather than an automated approval — leaving for human maintainer review.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[sauranshbhardwaj]

Thanks, appreciate the careful read against the head.

1. Post-approval status: intentional for this PR. After a user answer the run goes waiting_approval → running because the only thing that changed is that the kernel may now admit the one parked invocation; the surface shows the answer itself ("Allowed once" / "Allowed for this chat" / "Denied") on the card, so I didn't add a run-level state for it. If the surface owner wants an explicit approved state between the two, it's a small addition on top of resumeRunAfterApproval and I'm glad to do it.
2. Gate: yes, deliberately off until the card exists, so a beta from this PR alone changes nothing a user can see. PR 2 (the SwiftUI card) is built and in final testing — it flips the default and deletes the gate and the client_capabilities message. I'll open it as soon as its live checks are done.

[sauranshbhardwaj]

One update since your review, from live-testing the next phase (#21096) on a packaged build:

1. capture_screen was never in the enforced approval set, although Desktop: the new on-device tools (iMessage, Mail, AppleScript) can't run for users — add real per-action approvals #20938's acceptance criteria list it. In a typed chat the model took a full-screen capture with no card. It's now gated on all four PRs, and it's Allow Once only: a whole-screen capture can't be scoped to a resource, so the card offers no "Allow for This Chat". The coordinator also refuses to mint a grant for any parked call whose card never offered one.
2. A drift test, so this can't recur. Every tool the chat and pill relays can call must now be explicitly classified, either in a policy set or in a short unbundled list with a reason for each entry. Every tool that maps to a sensitive bundle must be in the approval set or in a named, justified exception. An unclassified new tool fails the test.
3. The voice screenshot tool is no longer advertised to pi-mono chat runs. It was offered there but only executes in realtime voice. Voice runs still get it through the realtime tool list.
4. Screen-image grants must name a resource, and show_rewind_evidence is classified with the screen-history reads, since it returns only the citation text to the model.

Bodies are updated with the new heads and test counts. The merge order is unchanged: #20973 → #20975 → #20997 → #20998.

[Git-on-my-level]

Reviewed the full diff of this head. This is careful, security-improving kernel work — detailed findings below. Thanks for the exceptionally thorough test suite.

What this gets right (verified against the diff):

• kernel-coordinator.ts — createDesktopDispatch strips the invocation payload key from model-created dispatches, so a model cannot forge a parked tool approval; resolveDesktopDispatch re-reads the pending row and validates authority before opening the resolution transaction, and restorePendingApproval undoes the broker's memory state if that transaction rolls back — no "decided in memory, pending in store" split.
• run-tool-capability.ts — the dispatch binding (invocationId, inputHash, daemonBootEpoch, executionGeneration) is re-validated against the ledger record on every approve/deny, with the boot epoch pinned to the live broker, so a restarted daemon can never replay a parked invocation. assertNotAwaitingApproval on both dispatch and lease acquisition, and revoke() cancels pending dispatch rows before failing their ledger rows.
• desktop-tool-policy.ts — capture_screen is ALLOW_ONCE_ONLY and hasAllowGrant returns false for allow-once tools, so no grant row can ever cover a future screenshot; approval previews are allow-listed per tool with file_path reduced to basename() and a 4 KiB bound, and inputs that don't fit the card are rejected (input_too_large_to_approve) rather than partially shown. Deny is the default option. Session grants are capped at 24h in the coordinator.
• omi-tool-manifest.ts + tests — removing screenshot from pi-mono chat adapters closes a real gap: typed chat previously got an advertised, ungated full-screen capture. Realtime voice keeps it via the realtime_voice surface projection.
• sqlite-store.ts — startup reconciliation expires invocation-bound approval dispatches regardless of TTL (the prepared invocation just failed), with audit events, matching the existing failed-prepared-invocation pass.
• Tests cover exactly the adversarial paths a reviewer would ask for: forged invocation payloads, tampered bindings, cross-restart replay, owner change mid-card, the model attempting to approve its own invocation, rollback restore, and the "no card renderer ⇒ keep today's immediate rejection" interim default.

Non-blocking observations for the maintainers:

1. terminateDesktopToolApproval and setDesktopToolApprovalsEnabled have no production callers in this PR — only tests. Expected for PR 1a, but the end-to-end flow is unverified until 1b (relay/wire) and 2 (Swift card) land. The interim gate defaulting off is the right call meanwhile.
2. The 180s DESKTOP_APPROVAL_TTL_MS constant is defined here; actual expiry enforcement will live with the relay caller in 1b. Worth confirming the pi-mono long-wait vs executor-timeout arithmetic holds once wired.
3. Only the neutral cubic reviewer ran on this head; the agent test suite hasn't run in CI on it.
4. run.waiting_approval / approval.requested / approval.resolved event shapes are asserted in tests against the UX-doc shape — good — but final sign-off on the card copy and permission UX belongs to a human.

No security concerns found in static review; the design is consistently fail-closed. Because this reshapes the device-tool permission surface (screen capture, messaging, AppleScript) and is the first PR of a stacked sequence, it needs maintainer sign-off before merge rather than automated action.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[Git-on-my-level]

Kernel half of the device tool approval flow (#20938, PR 1a). Reviewed the full diff statically; this is a well-built, security-tightening change, and it fixes two real gaps: capture_screen previously took and shared a full-screen screenshot with no approval at all, and the realtime screenshot tool was advertised to the pi-mono chat adapter. Since this PR defines the user-approval semantics for sensitive macOS device tools, it needs maintainer sign-off on the policy choices before merge, so I'm leaving detailed notes rather than a verdict.

What stands out, verified against the code:

• run-tool-capability.ts — the parking mechanism is fail-closed by construction. readPendingApproval re-validates invocationId, inputHash, daemonBootEpoch, executionGeneration, runId and attemptId against the live ledger row before anything is admitted (invocation_replayed otherwise); markInvocationDispatched and acquireExecutionLease both call assertNotAwaitingApproval; and deny/expiry/cancel/restart all terminalize the ledger row as failed, never outcome_unknown. toolResourceRef no longer honors a caller-supplied resource_ref, and gated tools are schema-validated at the kernel boundary (invalid_tool_input), so the model cannot influence which resource a grant covers.
• desktop-tool-policy.ts — approval card previews are field-allowlisted per tool (APPROVAL_PREVIEW_FIELDS), paths reduced to their final component, bounded at 4 KiB, and an input the card cannot show in full is refused outright (input_too_large_to_approve) instead of parking something the user cannot see: the user approves exactly what runs (inputHash-bound). No grant can ever cover a live screenshot (ALLOW_ONCE_ONLY_TOOLS in hasAllowGrant, plus the toolApprovalOffersSessionGrant gate at resolve time).
• kernel-coordinator.ts — createDesktopDispatch strips a model-supplied invocation payload key, so a model-created dispatch cannot impersonate a parked tool invocation (the forged-binding test covers this). Grant minting is capped at 24h and must match the dispatch's capability/operation/resourceRef exactly; endChatApprovalScope closes parked cards and revokes only that session's user-minted grants when a chat is cleared.
• kernel-core.ts + sqlite-store.ts — revocation-driven closures (owner change, run termination) are recorded as audited approval.resolved events published after their cause, and daemon-restart reconciliation expires invocation-bound approvals regardless of TTL so a post-restart answer finds nothing to admit.
• omi-tool-manifest.ts — removing screenshot from the pi-mono adapter (realtime voice surface projection only) closes the "ungated full-screen capture in every typed chat" hole. The new completeness tests ("classifies every relay-callable tool on purpose", "parks every relay-callable tool in a sensitive bundle, save the justified exceptions") prevent a future sensitive tool from shipping ungated by omission — good defensive structure.
• GeneratedToolExecutors.swift digest updates match the manifest change, and the changelog fragment's kind: "none" is consistent with the feature being inert until PR 1b lands (the desktopToolApprovalsEnabled gate in kernel-types.ts defaults off, so today's approval_required rejection is preserved while no client can render the card).

One question, not blocking: the 180s approval TTL is enforced by terminateDesktopToolApproval, which PR 1b's relay is expected to call. If the pre-existing generic pending-dispatch expiry sweep lapses a tool-approval dispatch first, the dispatch row expires but the ledger row stays prepared and the run stays waiting_approval until the attempt's tool-wait times out or restart reconciliation runs. Worth confirming in PR 1b that the terminate call wins that race — or teaching the sweep to fail the invocation closed.

Test coverage is unusually thorough (~1,600 new test lines across control-tools, run-tool-capability, desktop-tool-policy, sqlite-store and omi-tool-manifest), including the forged-binding, owner-change, tampered-payload, replay, rollback-restore and daemon-restart paths. Thanks for the careful work.

Because this PR sets the security semantics of user approval for sensitive device tools (grant scoping, TTL caps, the allow-once-only screenshot rule, the fail-closed lifecycle), it needs a maintainer's sign-off on those policy choices before merge despite the clean implementation.

---

by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with need human response.

[sauranshbhardwaj]

Checked this one. The race can't happen, because nothing expires pending dispatches while the daemon is running.

1. The generic expiry only runs at startup. It's the UPDATE … SET status = 'expired' inside reconcileStartup() (sqlite-store.ts:797-803), which runs only when the store opens. Earlier in that same transaction, every pending invocation-bound approval is already expired, its ledger row set to failed, and exactly one approval.resolved emitted (sqlite-store.ts:619-630, 754-795). So the generic UPDATE never sees one of them.
2. While the daemon runs, a lapsed expires_at_ms changes nothing on its own. The relay's timer fires at exactly dispatch.expiresAtMs (relay-tool-approvals.ts:143). terminateDesktopToolApproval checks only status = 'pending' (kernel-coordinator.ts:748-761), so it always closes the card. A late Allow between the TTL and the timer is refused by the store's expires_at_ms > now check and rolls back cleanly.

I confirmed both cases with a throwaway test (clock 10 minutes past the TTL with no timer fired, then the timer firing, then a restart 10 minutes after the TTL). Each card ended failed/expired with exactly one resolution. No code changed. If you'd like those two cases pinned as regression tests, I can add them to #20975.