Repository navigation
feat(desktop): relay approval_requested to Swift and resume parked tool calls (#20938, PR 1b) - #20975
sauranshbhardwaj wants to merge 5 commits into
Conversation
9571904 to
53701e6
Compare
|
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):
Non-blocking questions:
No blocking defect found, and relative to by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
|
Thanks for the thorough read. On the three questions:
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.
53701e6 to
d0956a2
Compare
|
Re-reviewed the rebased head (
What I re-verified on the new head (static; I did not run code): the binding re-validation in 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 by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
… 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.
d0956a2 to
7cc3022
Compare
|
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
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. |
|
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:
For the maintainer:
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 |
|
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:
Questions / notes:
The 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 |
|
Thanks for the careful read. Answers in the same order. Line numbers are at 7cc3022. The classifications themselves live in #20973; this PR leaves 1. show_rewind_evidence: no path gives the model the frame's pixels.
So the screen-summary class is accurate for what the model receives. The test at 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
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.
All of that is data 4. The status line. You're right that it reads as unconditional. Parking stays off until a client declares
|
|
The fill_cloud_connector_form follow-up is up: #21330. It pins |
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-boundapprovaldispatch. This PR connects that to the stdio relay and to Swift. A newruntime/relay-tool-approvals.tsmodule, taking the kernel and the frame and result writers, holds the relay call, sends Swift anapproval_requestedframe (dispatch id, bound invocation, preview, options, expiry), and settles from the kernel'sapproval.resolvedevent: an allow decision hands the invocation to Swift through the ordinaryauthorized_tool_executionpath exactly once; deny, expiry, cancellation, run termination and owner change returnapproval_deniedto the model. Every resolution is mirrored to Swift asapproval_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 pluscapture_screen, which PR 1a added to the approval set) move to thelongmodel-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_turnshandler now calls PR 1a'sendChatApprovalScopefor 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), andruntime-stdio-contract.test.tsdrives the real daemon through a clear and finds the grant revoked with onegrants.revokedevent.Interim behavior, disclosed: parking is on only after a connected client declares
desktop_tool_approval_cardsthrough the newclient_capabilitiesinbound message. Until PR 2 ships no client does, so asend_messageorcapture_screencall in a beta built from this PR fails immediately withapproval_required(forsend_messageexactly 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'sapproval.resolvedsays allow, the broker still refusesmarkRunToolInvocationDispatchedwhile an invocation is parked, and a Swift execution that never answers becomesoutcome_unknownas before. Section Lifecycle: terminal run and attempt events fail parked calls like any other pending call. The runtimeinithandshake 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 becausedesktop/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/agentat 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-> regeneratedGeneratedToolExecutors.swift(two manifest digests) andtests/fixtures/tool-manifest.json(sixtimeoutClassvalues);tool-surfaces-exhaustiveness.test.tspasses 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 atfeature-flag-registrybecausebackend/routers/conversations.pyusesexcept*(3.11 syntax); the passing run used Python 3.11.scripts/failure-class prepare-> OK, nofix: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 thinindex.tsglue that constructs the module is covered by typecheck and review only, and theclient_capabilitieshandler inindex.tsis covered by typecheck and theprotocol-v2.test.tsshape 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 oneapproval_requested; a kernel allow produces exactly oneauthorized_tool_executionwith the bound invocation and input hash, registers the pending Swift call, and a second resolution changes nothing; deny answers the model withapproval_deniedand 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 oneapproval_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 becomesoutcome_unknownwithswift_tool_timeout.protocol-v2.test.tstypesapproval_requested,approval_resolvedandclient_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 thelongclass, so a newly gated tool cannot keep the 30 s wait; the behavioral property (the extension's 10 minute wait forlongtools) already has coverage inpi-mono-extension/index.test.ts.UI evidence
UI-Evidence: none -- runtime-only change
Failure class (fixes)
Failure-Class: none