Skip to content

feat(desktop): relay approval_requested to Swift and resume parked tool calls (#20938, PR 1b) - #20975

Open
sauranshbhardwaj wants to merge 5 commits into
BasedHardware:mainfrom
sauranshbhardwaj:feat/desktop-approval-dispatch-relay
Open

sauranshbhardwaj wants to merge 5 commits into
BasedHardware:mainfrom
sauranshbhardwaj:feat/desktop-approval-dispatch-relay

Conversation

@sauranshbhardwaj

@sauranshbhardwaj sauranshbhardwaj commented Oct 8, 2026 •

Copy link
Copy Markdown
Contributor

Depends on #20973 (PR 1a). This PR is stacked on feat/desktop-approval-dispatch, so its diff shows 1a's commits too until 1a merges; I will rebase onto main as soon as it does. The 1b-only change is the last three commits (14 files).

What changed and why

Relay half of #20938 (PR 1b, stacked on feat/desktop-approval-dispatch; PR 2 is the Swift card). PR 1a taught the kernel to park a sensitive device tool call behind an invocation-bound approval dispatch. This PR connects that to the stdio relay and to Swift. A new runtime/relay-tool-approvals.ts module, taking the kernel and the frame and result writers, holds the relay call, sends Swift an approval_requested frame (dispatch id, bound invocation, preview, options, expiry), and settles from the kernel's approval.resolved event: an allow decision hands the invocation to Swift through the ordinary authorized_tool_execution path exactly once; deny, expiry, cancellation, run termination and owner change return approval_denied to the model. Every resolution is mirrored to Swift as approval_resolved, so a card can close whatever ended it. Expiry fires from the runtime, a disconnected relay client cancels its own dispatch, and the Swift hand-off itself moves into the module so the whole relay side is covered by hermetic tests with injected timers. The six gated device tools (the five above plus capture_screen, which PR 1a added to the approval set) move to the long model-side timeout class so the pi-mono extension waits for the user instead of giving up at 30 s; the Swift surfaces were regenerated (manifest digests only).

Clearing a chat ends what it granted. The main chat keeps one kernel session across clears, so an "Allow for This Chat" grant would otherwise cover the new conversation with no card; the daemon's journal_clear_turns handler now calls PR 1a's endChatApprovalScope for that session before clearing the journal, which closes its parked approvals and revokes the grants the person minted for it. Only the app's own clear message reaches it: it is not a tool, so neither the model nor trusted direct control can call it (control-tools.test.ts), and runtime-stdio-contract.test.ts drives the real daemon through a clear and finds the grant revoked with one grants.revoked event.

Interim behavior, disclosed: parking is on only after a connected client declares desktop_tool_approval_cards through the new client_capabilities inbound message. Until PR 2 ships no client does, so a send_message or capture_screen call in a beta built from this PR fails immediately with approval_required (for send_message exactly as today); nothing hangs for 180 s and no "needs approval" state appears that nobody can act on. PR 2 sends the capability, then flips the default and deletes the message and the kernel switch; this is a temporary dual path. Realtime voice is not parked in this PR because its Swift external-tool wait is 180 s and would race the approval TTL.

Product invariants affected

INV-AGENT-* (product/invariants/agent-control-plane.md), section Authority ("Physical tool authority is run/attempt scoped and single use"): the relay never dispatches a parked invocation until the kernel's approval.resolved says allow, the broker still refuses markRunToolInvocationDispatched while an invocation is parked, and a Swift execution that never answers becomes outcome_unknown as before. Section Lifecycle: terminal run and attempt events fail parked calls like any other pending call. The runtime init handshake grows one advertised capability (desktop_tool_approval_requests); Swift requires a subset, so the current app is unaffected. Guard tests: relay-tool-approvals.test.ts (new), protocol-v2.test.ts.

INV-CHAT-1 (product/invariants/chat-continuity.md) is named because desktop/macos/agent/src/runtime/** matches its path glob. Unaffected: the parked relay entry is process-local, intentionally lost on restart, and writes no transcript state.

How it was verified

From desktop/macos/agent at the branch tip (7cc3022, stacked on PR 1a 10ef09e, main d3208a0):

  • npm ci && npm run build -> clean.
  • npx vitest run -> 73 files, 958 tests passed (946 at the PR 1a tip + 12 new), 0 failed.
  • node --experimental-strip-types scripts/generate-tool-surfaces.mjs -> regenerated GeneratedToolExecutors.swift (two manifest digests) and tests/fixtures/tool-manifest.json (six timeoutClass values); tool-surfaces-exhaustiveness.test.ts passes in the run above, so there is no generated drift.
  • desktop/macos/scripts/agent-logic-harness.sh --node-only --skip-install -> "Harness passed." The full harness (Swift focused lifecycle/state tests: 684 executed, 0 failures; pi-mono-extension package tests 143/143) was run on the combined change before the split; the only Swift change is the regenerated digests.
  • python3 .github/scripts/check_agent_doc_references.py -> OK; python3 .github/scripts/desktop-changelog.py validate -> OK.
  • scripts/pr-preflight --base feat/desktop-approval-dispatch --pr-body-file <this body> -> "PR preflight passed: 33 checks" (lane ci, base 10ef09e, 14 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.

Not verified: a real model turn through the stdio relay with the card answered via omi-ctl / the dev bridge and a message actually sent. The relay module is exercised with the real kernel, broker and SQLite store (a live acp-lane run with a deferred adapter) and fake frame/result writers; the thin index.ts glue that constructs the module is covered by typecheck and review only, and the client_capabilities handler in index.ts is covered by typecheck and the protocol-v2.test.ts shape test only, not by a runtime test that sends the message. No packaged-app smoke was run.

Tests

Core path (relay-tool-approvals.test.ts): park shows exactly one approval_requested; a kernel allow produces exactly one authorized_tool_execution with the bound invocation and input hash, registers the pending Swift call, and a second resolution changes nothing; deny answers the model with approval_denied and never reaches Swift; expiry (manual timer) fails the call closed as a system decision and a late answer finds nothing to admit; a disconnected relay client cancels only its own card and writes nowhere while another client's card stays answerable; a duplicate invocation id is refused while the first is parked or in flight; run cancellation fails the parked call and the card ends with one approval_resolved; an owner change ends the card with one frame and one failed result immediately, and the daemon's owner sweep afterwards finds nothing to fail twice; a Swift execution that never answers becomes outcome_unknown with swift_tool_timeout. protocol-v2.test.ts types approval_requested, approval_resolved and client_capabilities, and pins the advertised and declared capability names.

The manifest commit adds one test (run-tool-capability.test.ts) that holds every gated tool to the long class, so a newly gated tool cannot keep the 30 s wait; the behavioral property (the extension's 10 minute wait for long tools) already has coverage in pi-mono-extension/index.test.ts.

UI evidence

UI-Evidence: none -- runtime-only change

Failure class (fixes)

Failure-Class: none

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed the full diff of this stacked relay half (1b on top of #20973). This is a well-built, consistently fail-closed approval flow, and it closes some real holes — detail below, plus a few non-blocking questions.

What checked out on the authority paths (static review; I did not run code):

  • run-tool-capability.ts — the old toolResourceRef honored a model-supplied input.resource_ref, which would have let one approval mint a grant covering any resource the model cared to re-name. This PR removes that and derives the resource only from to / script / chat_id / handle (with stable refs messages:chats, mail:inbox for the list tools), and holds the five gated tools to their manifest schema at the kernel boundary (invalid_tool_input, input_too_large_to_approve). The "never lets the model name the resource a grant covers" test reproduces exactly the old smuggling pattern.
  • kernel-coordinator.ts — createDesktopDispatch strips an invocation payload key from model-created dispatches, so a forged dispatch can't impersonate a parked tool call; resolveDesktopDispatch requires a plain allow/deny for invocation-bound approvals, re-validates the binding (invocationId + inputHash + daemonBootEpoch + executionGeneration + run/attempt) before admitting anything, caps session grants at 24 h with exact resource match, and restores broker parking state when the surrounding transaction rolls back.
  • relay-tool-approvals.ts (new) — the relay call fails closed on expiry (180 s, correctly chosen to sit under the pi-mono long wait minus the Swift executor timeout), client disconnect, owner change, and terminal run/attempt events, and every card ends with exactly one approval.resolved event and approval_resolved frame. An allow hands the invocation to Swift through the ordinary authorized_tool_execution path exactly once.
  • sqlite-store.ts — startup reconciliation fails the prepared ledger row and expires the invocation-bound dispatch with an audit event, so a parked call never replays across a daemon restart and a late user answer finds nothing to admit (idempotent on re-open, tested).
  • desktop-tool-policy.ts — the preview allowlist (per-tool fields, file_path reduced to its basename, 4 KiB bound, truncate-then-refuse) and the covers copy for allow_session match the actual grant semantics; the schema-validated input means an unknown key can't ride into the card unexamined.
  • index.ts / protocol.ts — the interim client_capabilities gate means a build from this PR alone keeps today's immediate approval_required; nothing hangs for 180 s that nobody can answer. omi-tool-manifest.ts, the test fixture, and the regenerated digests in GeneratedToolExecutors.swift are consistent with the normal → long timeout flips (I could not independently re-derive the digests without running codegen).
  • device-tool-surface.md is accurate against the code, including the interim-gate disclosure.

Non-blocking questions:

  1. The interim gate (setDesktopToolApprovalsEnabled) is a single kernel-global flag set by whichever client last declared desktop_tool_approval_cards — it is not per-client and not reset when that client disconnects. A later card-incapable client would park into a 180 s fail-closed wait. Since PR 2 deletes the gate entirely, this is probably fine, but worth a conscious yes.
  2. No automated test workflow ran on this head (only an AI reviewer), despite ~1,700 lines of new hermetic, deterministic-timer tests across control-tools, run-tool-capability, relay-tool-approvals, desktop-tool-policy, protocol-v2, and sqlite-store. Please run the desktop agent suite before merge.
  3. .github/agent-docs/agent-grants-approval-ux.mdx flips its status from "Design-only … do not treat this document as implementation scope" to describing the parking flow and resolve_desktop_dispatch grant minting as live. That's accurate against this diff, but it only becomes true on main once feat(desktop): park sensitive tool calls in waiting_approval and emit approval.requested (#20938, PR 1a) #20973 and this PR both merge — agents working in the repo will read it as implemented behavior. Fine as long as the stack lands in order.

No blocking defect found, and relative to main this strengthens the permission surface rather than weakening it. That said, grant-minting semantics and the approval UX on a messaging/AppleScript surface are product and security judgment calls, so I'm leaving the merge decision to a human maintainer. Thanks @sauranshbhardwaj — the rollback-consistency and restart-reconciliation tests in particular are the kind of thing this surface deserves.


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 Git-on-my-level added needs-maintainer-review Needs a human maintainer to sign off before merge positive-signal Automation verified a genuine fix/quality contribution macOS labels Oct 8, 2026
@sauranshbhardwaj

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough read. On the three questions:

  1. Gate scope: yes, a conscious choice. It's a kernel-global flag because it exists only to keep a beta built from 1a/1b alone from parking calls nobody can answer; PR 2 (the SwiftUI card, built and under test now) deletes the gate and the client_capabilities message entirely. If you'd rather it reset on client disconnect even for the interim, that's a few lines and I'm happy to add it here.
  2. Agent suite on this head: cd desktop/macos/agent && npm run build && npx vitest run at 53701e6a5d → 73 files, 944 tests passed, 0 failed (899 on main + 45 new across 1a/1b); scripts/agent-logic-harness.sh --node-only passed; tool-surfaces-exhaustiveness 12/12. 1a at c283e92e8e: 935 passed.
  3. UX doc status: agreed; it's accurate only once feat(desktop): park sensitive tool calls in waiting_approval and emit approval.requested (#20938, PR 1a) #20973 and this land in order. I'll keep the stack ordered and rebase this onto main the moment 1a merges.

Happy to split the bridge actions + e2e flow into a follow-up if that makes review easier.

…ool policy

A dispatch_required decision for a sensitive on-device tool now has a policy-owned shape for what the user is asked: the capability, operation and resource the grant would be matched against, a bounded display preview of the input (attachment paths reduced to a file name, text and scripts capped at the 4 KiB card bound with a truncation flag the broker fails closed on), and the answers. Deny is the default; allow_once is always offered; allow_session is offered only when there is an exact resource for a grant to cover and carries plain words for what it would cover, so a script grant is shown as covering only that exact script. The 180s expiry matches the mobile device-tool transport and stays under the pi-mono long tool wait minus the Swift executor timeout, so a late answer cannot run an effect the model already reported as failed. Only this module decides the question and the options; adapters and the model never choose allow or deny.
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Re-reviewed the rebased head (d0956a2) — thanks for the clean rebase; the stack is intact (this branch's second-to-last commit is exactly #20973's current head), and thanks for the answers to the three questions. Quick close-out on each:

  1. Gate scope — agreed, acceptable as a consciously interim kernel-global flag given PR 2 deletes client_capabilities and the gate entirely. No change requested.
  2. Suite — appreciated. Since no CI workflow runs on this path, please keep running npm run build && npx vitest run on any further rebases before merge; the reported 944 passing on the head is a good state to hold.
  3. UX doc status — agreed, and worth restating for the maintainers: .github/agent-docs/agent-grants-approval-ux.mdx now describes the parking flow and resolve_desktop_dispatch grant minting as live. That is accurate against this diff, and it becomes true on main only once feat(desktop): park sensitive tool calls in waiting_approval and emit approval.requested (#20938, PR 1a) #20973 and this PR land in order — until then, coding/review agents reading the repo will see implemented-behavior wording slightly ahead of main. The doc's own claims check out against the code (the only live grant writer is resolve_desktop_dispatch, user-source, finite expiry, no policy/system writer), so this is a merge-order note, not an accuracy problem.

What I re-verified on the new head (static; I did not run code): the binding re-validation in readPendingApproval (invocationId + inputHash + daemonBootEpoch + executionGeneration + run/attempt against the live prepared ledger row), the stripping of the invocation payload key from model-created dispatches in createDesktopDispatch, the 24 h grant TTL cap and the allow_session-must-have-been-offered guard in resolveDesktopDispatch, capture_screen remaining grant-proof via ALLOW_ONCE_ONLY_TOOLS, the fail-closed paths in relay-tool-approvals.ts (180 s expiry, client disconnect, owner change, terminal run/attempt events, each card ending in exactly one approval.resolved), and the startup reconciliation in sqlite-store.ts that expires invocation-bound dispatches regardless of TTL. No blocking defect found; the screenshot de-advertisement from pi-mono is a genuine tightening.

The offer to split the bridge actions + e2e flow into a follow-up is noted but not needed — the PR is cohesive around one outcome and splitting would make the end-to-end approval flow harder to test.

Remaining before merge, for a human maintainer rather than this review: the allow_session grant semantics (per-tool covers wording, exact-resource matching, 24 h ceiling) and the approval UX on a messaging/AppleScript surface are product and security judgment calls that automation shouldn't sign off alone. Thanks again @sauranshbhardwaj — the rollback-restoration and restart-reconciliation tests are exactly what this surface needed.


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

… approval.requested

send_message, run_applescript, read_message_history, list_message_chats and list_mail_messages used to fail with approval_required whenever no scoped grant existed, and nothing in the app could create one. capture_screen did not fail at all: it took and shared a full-screen screenshot with no approval, although its own description promised one, so it now parks the same way under the stable resource "screen". A screenshot holds whatever is on screen, including windows the person keeps out of capture elsewhere, so each one is allowed once: its card offers no session grant, no grant row covers a capture, and the kernel refuses to mint a grant the card did not offer. The realtime voice screenshot tool is no longer advertised to pi-mono, where it sat ungated in every typed chat, pill and workstream run; realtime voice runs still get it through their surface projection. The capability broker now 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 kernel moves the run and attempt to waiting_approval and appends run.waiting_approval plus approval.requested, and the broker refuses to dispatch the invocation until the dispatch is resolved. resolve_desktop_dispatch gains the two user answers: decision allow without a grant admits 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 as failed, never outcome_unknown, each with its own approval.resolved, and the run resumes so the model receives approval_denied. 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 and revokes every grant the person minted for it, with one grants.revoked audit event. Startup reconciliation expires any pending bound approval and records approval.resolved, so a restart never replays a parked call.

Every relay-callable tool is now classified on purpose (named in a policy set or declared unbundled, never by the read-only default), and a test holds the approval set to every relay-callable tool in a sensitive bundle, with each deliberate exception named and justified, so a new tool cannot ship unclassified or ungated by omission. Grants for desktop.context.screenshot_image must name their exact resource. The gated tools are held to their manifest schema at the kernel boundary and the resource a grant covers is derived only from the fields the tool acts on, so a model-supplied resource_ref can no longer become the grant pattern. A dispatch created through create_desktop_dispatch has its invocation payload key stripped and can never pose as a parked call. Authority is revalidated before a resolution is admitted and that check runs before the resolving transaction, so a revocation it triggers commits on its own; a rolled-back decision restores the broker's memory. The model still cannot resolve its own dispatch: the relay path keeps returning policy_denied without trustedUserControl.

Parking sits behind a kernel switch, desktopToolApprovalsEnabled, that defaults to off and is not reachable from the daemon in this change, so runtime behavior is unchanged until a client that renders approval cards turns it on. The Swift card PR flips the default and deletes the switch.
…ol calls

The stdio relay no longer answers a parked device tool call immediately. A new relay-tool-approvals module, taking the kernel and the frame and result writers, sends Swift an approval_requested frame carrying the dispatch id, the bound invocation, the policy preview, the options and the expiry, keeps the relay call pending, and settles it from the kernel's approval.resolved event: an allow decision hands the invocation to Swift through the ordinary authorized_tool_execution path exactly once, every other outcome returns approval_denied to the model. Every resolution is mirrored to Swift as approval_resolved so a card can close whatever ended it, including owner changes. Expiry fires from the runtime, a disconnected relay client cancels its own dispatch, and terminal run events fail parked calls like any other pending tool call. The Swift hand-off itself moves into the same module so the whole relay side is covered by hermetic tests with injected timers.

Clearing a chat through journal_clear_turns now ends what that chat granted before the journal is cleared: the kernel closes the session's parked approvals and revokes the grants the person minted for it, because the main chat keeps one kernel session across clears and an "Allow for This Chat" grant would otherwise cover the new conversation. No tool reaches this; only the app's own clear message does.

Parking is on only after a client declares desktop_tool_approval_cards through the new client_capabilities message; until the Swift card ships no client does, and the relay keeps today's immediate approval_required. The runtime advertises desktop_tool_approval_requests so that client can require it first. Realtime voice keeps today's behavior because its Swift tool wait is 180s and would race the approval TTL.
The pi-mono extension waits 30s for a normal Swift tool result and gives up with a timeout error. A human answering an approval card needs longer than that, and a late approval would then run a send the model had already reported as failed. The six device tools that park behind an approval, capture_screen among them, now declare the long timeout class, which the extension maps to its 10 minute wait, and a test holds every gated tool to it; the approval TTL of 180s plus the 120s Swift executor timeout stays well inside it. Swift does not consume timeoutClass outside the generated surfaces, so the regenerated files only carry the new manifest digests.
The approval UX design doc keeps its design status and points at the device tool surface doc for the runtime half that now exists; the Swift card, the Claude Code lane and the voice surface stay deferred and section 24 still applies. The device tool surface doc gains the ask-and-answer table, the schema and resource rules that stop the model naming what a grant covers, the session-grant options and their 24 hour cap, the restart and expiry behavior, and the interim client-capability gate. The changelog fragment is kind none because nothing is user-visible until the Swift card lands.
@sauranshbhardwaj
sauranshbhardwaj force-pushed the feat/desktop-approval-dispatch-relay branch from d0956a2 to 7cc3022 Compare October 11, 2026 00:32
@sauranshbhardwaj

Copy link
Copy Markdown
Contributor Author

One behaviour change since your re-review, found in a live packaged test.

The problem: "Allow for This Chat" grants are keyed by the kernel session, and the main chat is one continuous session, so the grants outlived Clear Chat. A person who cleared the chat and started over was still covered by an earlier "for this chat" approval, and got no card.

The fix: clearing a chat now ends what that chat allowed. Every clear path already ends in one journal_clear_turns message, so the hook lives there:

  • In the daemon, journal_clear_turns now calls a new kernel endChatApprovalScope before it clears the journal. That closes any approval still parked in the session as cancelled; the call fails closed and the run resumes.
  • It also revokes the session's source = 'user' grants in one transaction, with one grants.revoked audit event. Policy grants and other sessions' grants are untouched, and a second call does nothing.
  • It isn't a tool: both names return unknown_control_tool, with and without trusted control, so neither the model nor direct control can trigger it.

Tests cover the kernel operation, a real-daemon journal clear (it fails if the hook is removed), and the Swift Clear Chat seam. Bodies on #20973, #20975 and #20997 are updated with the new heads and counts.

@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Thanks @sauranshbhardwaj — this is careful security work. I reviewed the full relay/kernel approval flow on this head (including the stacked 1a commits). What I verified:

  • run-tool-capability.ts: parked invocations fail closed on every termination path, and readPendingApproval re-validates inputHash, daemonBootEpoch against the live broker epoch (so a restart invalidates the binding), executionGeneration, and run/attempt identity — a tampered or stale dispatch admits nothing (the tampered-payload and replay tests pin this). Removing the model-controllable input.resource_ref and holding gated tools to their manifest schema (invalid_tool_input) closes a real grant-widening hole; the test that smuggles resource_ref now expects rejection.
  • kernel-coordinator.ts: createDesktopDispatch strips the invocation payload key so a model-created dispatch can never impersonate a parked tool approval; session grants are capped at 24h, must match the dispatch's capability/operation/resourceRef, and authority is asserted before the resolving transaction so a refused resolution cannot roll back its own revocation. resolve_desktop_dispatch from an adapter relay returns policy_denied — the model cannot approve itself.
  • relay-tool-approvals.ts: an allow dispatches to Swift exactly once; duplicate invocation ids are rejected while parked or in flight; a disconnected relay client cancels only its own card; expiry fails closed even if the kernel call errors.
  • sqlite-store.ts: startup reconciliation expires invocation-bound approval dispatches regardless of TTL and fails their prepared ledger rows — nothing replays across a daemon restart, and it's idempotent.
  • kernel-core.ts: derived approval.resolved events publish after the causing event both inside and outside transactions, so every card ends with exactly one resolution.
  • omi-tool-manifest.ts: dropping the pi-mono advertisement for the realtime screenshot tool (adapters: {}) and keeping it on the realtime_voice surface projection removes an ungated full-screen capture from typed chat, pill, and workstream runs — good tightening, and the manifest tests enforce it.
  • desktop/macos/docs/device-tool-surface.md reads accurate against the code (deny-default options, allow-session covers wording, chat-clear revocation, 180s TTL vs the 10-min long tool wait).

For the maintainer:

  • This is the authority surface for user-granted device permissions (messaging, AppleScript, Mail, live screen capture). The code reviews clean and fails closed everywhere I traced, but whether this policy shape is the right product behavior for Desktop: the new on-device tools (iMessage, Mail, AppleScript) can't run for users — add real per-action approvals #20938 — 180s approval TTL, 24h grant cap, allow-once-only screenshots, stable refs like mail:inbox — needs a human decision, so needs-maintainer-review stays and I've added security-review for the grant-minting surface.
  • .github/agent-docs/agent-grants-approval-ux.mdx changes status language from "design-only / grants dormant" to describing this dispatch flow as implemented. That file guides coding and review agents in this repo; the new text matches the code in this PR (single user-source grant writer via resolve_desktop_dispatch, everything else still design-only), so it reads accurate rather than aspirational — but it does change what agents treat as live behavior, so please confirm that framing, especially since the feature ships dark until the Swift card PR.
  • The interim client_capabilities gate means any connected relay client can enable parking by declaring desktop_tool_approval_cards. Direction is benign (parking is more restrictive than today's immediate approval_required), just noting the gate is client-trusted until the card PR replaces it.

No blocking changes from me; leaving the merge call to a maintainer.


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 Git-on-my-level added the security-review Touches auth, provider routing, secrets, or security-sensitive surfaces label Oct 11, 2026
@Git-on-my-level

Copy link
Copy Markdown
Collaborator

Reviewed the relay half of the device tool approval flow (#20938, PR 1b). This is careful security work — what I verified against the diff, plus a few questions.

What's solid:

  • runtime/relay-tool-approvals.ts — the relay holds no authority: dispatch row, ledger row, run status, and the approval.resolved event all stay in the kernel; the module only parks the call, mirrors frames, and settles. An allow hands the invocation to Swift exactly once (dispatchToSwift guards replay); deny, expiry, cancellation, owner change, terminal run, and a disconnected relay client all fail the invocation closed, each ending with exactly one approval.resolved.
  • runtime/run-tool-capability.ts — this closes a real hole: the old toolResourceRef honored a caller-supplied input.resource_ref, so one approval could cover any later recipient that repeated the same ref. The ref is now derived only from to / script / chat_id / handle, and gated tools are held to their manifest schema at the kernel boundary (unknown key, missing field, or wrong type → invalid_tool_input before anything is prepared). Also verified readPendingApproval re-checks invocationId, inputHash, daemonBootEpoch (bound value and current broker epoch), executionGeneration, run, and attempt against the live ledger — a tampered or stale dispatch admits nothing (tested).
  • runtime/kernel-coordinator.ts — createDesktopDispatch strips the invocation payload key, so a model-created dispatch can never impersonate a parked tool call (tested). resolveDesktopDispatch requires an explicit allow/deny for bound approvals, mints grants only when the card actually offered allow_session (toolApprovalOffersSessionGrant), matches capability/operation/resourceRef, requires finite future expiry, and caps session grants at 24h. endChatApprovalScope closes parked cards and revokes only that session's user grants before a chat clear, so a failed clear leaves fewer grants, not more (end-to-end stdio contract test covers it).
  • runtime/desktop-tool-policy.ts — capture_screen is allow-once-only and no grant row can ever cover it (ALLOW_ONCE_ONLY_TOOLS), even one naming screen; live screenshots ask each time.
  • runtime/omi-tool-manifest.ts — removing the realtime screenshot tool from the pi-mono advertisement closes an ungated full-screen capture path in typed chat; realtime voice still reaches it through the surface projection (covered by tests).
  • runtime/sqlite-store.ts — startup reconciliation expires invocation-bound approvals regardless of TTL and records an audit event, so a parked call never survives a daemon restart or replays after one; a late user answer finds nothing pending (tested, idempotent on second open).
  • index.ts — the interim gate (client_capabilities with desktop_tool_approval_cards, default off) keeps today's immediate approval_required until the Swift card ships; conservative and reversible.
  • Test coverage is unusually thorough: forged bindings, model self-approval returning policy_denied, tampered payloads, transaction-rollback consistency (restorePendingApproval), late answers after expiry, chat-clear grant revocation, and a test holding DESKTOP_APPROVAL_TOOLS to every relay-callable tool in a sensitive bundle with named, justified exceptions.

Questions / notes:

  1. show_rewind_evidence is classed as a screen-summary tool on the rationale that the model receives only the stored frame's title, app, and OCR excerpt while the pixels go to the user's chat turn. That classification is asserted by a test but the guarantee lives in the executor — if any path returns frame pixels to the model, it would bypass the screenshot gate. Worth a maintainer eyeball on the Swift/executor side.
  2. fill_cloud_connector_form stays ungated (documented exception: outside Desktop: the new on-device tools (iMessage, Mail, AppleScript) can't run for users — add real per-action approvals #20938's device tools, needs the connector form already open). It still takes an arbitrary server_url and can press submit — fine as a scoped decision, but it's the remaining sensitive relay tool without a card.
  3. The additions to LOCAL_READ_TOOLS (scan_files, search_chat_history, read_conversation_evidence, …) formalize the previous read-only fallback rather than changing behavior — the new classification test forces every relay-callable tool to be named explicitly. scan_files as non-sensitive local read is the one I'd double-check.
  4. Docs: .github/agent-docs/agent-grants-approval-ux.mdx now tells coding/review agents that grant writes are live via resolve_desktop_dispatch. That's accurate (the mint path, the source: "user"-only writer, and the no-policy/system-writer claim all check out), but "the desktop runtime now parks the on-device tools" reads as unconditional while parking is off by default until the Swift card PR — the linked device-tool-surface.md does state the interim gate. A qualifier in the status line would keep agents from assuming approvals already fire in shipped builds.
  5. Stacked on feat(desktop): park sensitive tool calls in waiting_approval and emit approval.requested (#20938, PR 1a) #20973 — the visible diff includes 1a's commits; I reviewed the combined behavior. Worth re-confirming the 1b-only commits after 1a lands.

The resource_ref fix, the binding checks, and the fail-closed coverage are merge-quality. Because this is the permission gate for messaging, AppleScript, mail, and screen capture — and it introduces the runtime's only grant-minting path — it needs a maintainer security sign-off before merge, and the two classification judgment calls above (show_rewind_evidence, fill_cloud_connector_form) deserve that eyeball.

AI-generated feedback posted on behalf of the maintainers.


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

Copy link
Copy Markdown
Contributor Author

Thanks for the careful read. Answers in the same order. Line numbers are at 7cc3022. The classifications themselves live in #20973; this PR leaves desktop-tool-policy.ts unchanged.

1. show_rewind_evidence: no path gives the model the frame's pixels.

  • The relay sends the call to executeShowRewindEvidence (desktop/macos/Desktop/Sources/Providers/ChatToolExecutor.swift:383-394). It runs only when the screenshot-sharing setting is on (:845-857) and only for a main-chat run with a live capability (:989-1001).
  • It loads the stored JPEG (:1031, :1034) and writes it to a 0600 file under the owner's ChatEvidence folder (:1074). It then attaches that file to the producing assistant turn as a resource (:1079-1096). The runtime checks that the call matches the producing run and appends the resource to that turn's journal entry (desktop/macos/agent/src/index.ts:3000-3035, desktop/macos/agent/src/runtime/conversation-journal.ts:1246). The person sees the image in their own chat turn.
  • The model gets back {"ok":true,"evidence_attached":true} plus one citation entry (ChatToolExecutor.swift:1104-1116). That entry holds the screenshot id, the window title or app name, the OCR text capped at 600 characters, the timestamp and the app name (desktop/macos/Desktop/Sources/Chat/ChatCitation.swift:65, :947-972). Its URL is nil (ChatCitation.swift:842), and the file path is never returned.
  • An image reaches a model request in one place only: the query's imageBase64 (desktop/macos/agent/src/runtime/jsonl-transport.ts:668-676, passed on at desktop/macos/agent/src/adapters/pi-mono.ts:902). Swift fills it once per user turn, from the person's own attachment, a notification screenshot or an explicit screen request (desktop/macos/Desktop/Sources/Providers/ChatProvider.swift:4648-4703). None of those reads the evidence resource. Nothing outside the executor references the ChatEvidence folder. In later history the resource is represented only by the text line Attachment: Rewind evidence (desktop/macos/agent/src/runtime/content-block-fallback.ts:53).

So the screen-summary class is accurate for what the model receives. The test at desktop/macos/agent/tests/run-tool-capability.test.ts:1461-1464 checks the classification.

2. fill_cloud_connector_form: fair point, and I'd tighten it rather than keep the exception as written.

The Claude path is bound to Omi's own connector page, but server_url isn't pinned to anything, and the ChatGPT path is looser than the exception comment at run-tool-capability.test.ts:1423-1427 suggests. I'm putting up a small separate PR, cut from main, that:

  • pins server_url (and the OAuth URLs) to Omi's own MCP endpoint, refusing anything else before any UI is touched;
  • requires the ChatGPT path to find ChatGPT's own connector page;
  • corrects the exception comment.

I'll link it here when it's up. If you'd rather park the tool behind the approval card instead, I can do that there. Its only legitimate input is Omi's own URL, so pinning seemed the simpler fix. Keeping it out of this PR avoids re-pushing the stack.

3. scan_files: it reads file metadata only, not file contents. local_read is the right class.

  • It ignores the paths argument (ChatToolExecutor.swift:2858). It always lists the same places: ~/Downloads, ~/Documents, ~/Desktop, ~/Developer, ~/Projects, /Applications and the two Apple Notes store folders (:2892-2921). Listing those folders can bring up macOS's own folder-access prompts (:2927).
  • For each file it reads only the name, size, creation and modification dates and the file type (desktop/macos/Desktop/Sources/FileIndexing/FileIndexerService.swift:203-206, :271-275; FileIndexScanPolicy.swift makeFileRecord). It opens no file. It writes those rows to the local indexed_files table.
  • Limits: folders at most 3 levels deep (FileIndexScanPolicy.swift:35, :107-109), a skip list of build and cache folders (:25-30), no hidden files, and no empty files or files over 500 MB.
  • What the model gets back (ChatToolExecutor.swift:3011-3130):
    • counts per file type (top 10);
    • up to 30 build-file names with their folder;
    • the 15 most recently modified file names, with type and date;
    • up to 30 installed app names;
    • up to 8 open task descriptions from the local task table.
  • It is advertised only on the onboarding stdio surface (omi-tool-manifest.ts:2064).

All of that is data execute_sql, already local_read, can return from the same local database (ChatToolExecutor.swift:1199-1258, no table allowlist). So desktop.context.local_read is consistent. Two small inaccuracies remain: the tool also writes the local file index and can raise the folder prompts, so readOnlyLocal is a little generous. I can note both in the manifest description in the same follow-up if you want.

4. The status line.

You're right that it reads as unconditional. Parking stays off until a client declares desktop_tool_approval_cards (desktop/macos/agent/src/index.ts:3941-3948). No client declares it until the Swift card in #20997 ships. The design doc already says this (desktop/macos/docs/device-tool-surface.md:131), but the status line in .github/agent-docs/agent-grants-approval-ux.mdx:3 doesn't. Changing it now would mean re-pushing the four PRs stacked above. So: if #20975 lands before #20997, I'll add the qualifier there. Once #20997 lands, the sentence is true as written. The qualifier, appended to that line:

Until the Swift card (#20997) ships, parking is off by default: it turns on only when a client declares desktop_tool_approval_cards, and otherwise these tools return approval_required immediately.

@sauranshbhardwaj

Copy link
Copy Markdown
Contributor Author

The fill_cloud_connector_form follow-up is up: #21330. It pins server_url and the OAuth URLs to Omi's own MCP endpoint (exact https match, refused before any UI step) and requires ChatGPT's own connector form in a supported browser. The corrected exception comment for run-tool-capability.test.ts is in its description, to apply here or on #20973 whichever lands first.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macOS needs-maintainer-review Needs a human maintainer to sign off before merge positive-signal Automation verified a genuine fix/quality contribution security-review Touches auth, provider routing, secrets, or security-sensitive surfaces

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants