Repository navigation
feat(agent): tell the model the app asks for device tool approval itself (#20938, PR 3) - #20998
Open
sauranshbhardwaj wants to merge 9 commits into
Open
sauranshbhardwaj wants to merge 9 commits into
sauranshbhardwaj wants to merge 9 commits into
Conversation
2 tasks done
Collaborator
|
Thanks @sauranshbhardwaj — this is a careful piece of work on a sensitive surface. I reviewed the kernel, relay, and card paths statically; summary below. No concrete defect found, but this needs maintainer sign-off before merge. What the design gets right (verified against the diff):
Why I'm not approving and why human review is needed:
Leaving for human maintainer review: the approval-flow security design and the stacked-merge sequencing both need a maintainer decision. Labeling by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
security-review
Contributor
Author
|
Thanks. On the three points:
|
2 tasks done
sauranshbhardwaj
force-pushed
the
feat/device-tool-descriptions
branch
from
October 10, 2026 07:25
8be5638 to
a6bb62d
Compare
Collaborator
|
Thanks @sauranshbhardwaj — this is careful work, and the PR-3-only delta (the model-facing tool guidance) is exactly the right scope for the last piece of the stack. What I verified against the diff:
One caveat: GitHub only shows the AI reviewer check on this PR, so the added Swift/vitest suites aren't independently green here — please make sure the stacked branch's local runs (and the e2e flow + UI evidence) stay reproducible when PR 2 merges underneath this. Leaving for human maintainer review: this is a security-sensitive device-tool authorization flow (approval parking, user-minted scoped grants through signed direct control), and the product call on the approval UX — Allow Once vs the 1-hour session grant, and which tools are covered — needs a maintainer sign-off before merge. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Git-on-my-level
added
the
positive-signal
Contributor
Author
|
Thanks. On keeping it reproducible: after each PR beneath this one merges, I'll rebase this branch and rerun the agent suite ( |
sauranshbhardwaj
force-pushed
the
feat/device-tool-descriptions
branch
2 times, most recently
from
October 11, 2026 00:38
0524b2d to
703fd95
Compare
Collaborator
|
Reviewed the PR-3 delta on top of the stacked sequence — this is a careful piece of work, and I want to name the parts that stood out before the maintainers' pass. What I verified against the diff:
The PR-3-specific change itself — rewriting the tool descriptions from "always requires an approval dispatch" to "call it directly; Omi asks the person in the app" (GeneratedToolCapabilities.swift, omi-tool-manifest.ts, fixtures/tool-manifest.json, with digests regenerated) — reads accurate to me because enforcement is unchanged and kernel-side; the model just stops pre-filing dispatches the app now supersedes. One note for maintainers: this rewrites the standing instructions every desktop model receives about sensitive tools, and the per-screenshot-always-once rule plus the interim Checks that ran are green (Desktop Swift Build & Tests, Agent Runtime, Mobile Release Eligibility). No blocking issues found; leaving the approval-flow product direction and the model-wording change for human maintainer review. AI-generated maintainer feedback on behalf of the Omi maintainers. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
Collaborator
|
@sauranshbhardwaj this conflicts with |
…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.
… 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.
Render the kernel's approval_requested frame as a card in the main chat, workstream and agent pill threads, matched to the asking session, with the exact target, the content preview, the expiry and the answers the runtime offered; answer through signed direct control (allow once, allow for this chat with a 1 h session grant, deny) and close the card on approval_resolved whoever ended it. Declare desktop_tool_approval_cards after the handshake so the relay parks a sensitive call instead of failing it, require desktop_tool_approval_requests from the runtime, and add tool_approval_snapshot and answer_tool_approval bridge actions so omi-ctl can drive the card without Accessibility permission.
… stall In the live run the chat's 90 s tool no-progress guard revoked the turn and interrupted the bridge while a send_message card was waiting for the person, so the kernel cancelled the run and the card read Cancelled, not run. Teach StallDetector a waitingOnUser state the turn's tick loop sets from the approval store: while a card for the session is pending nothing promotes to slow or stalled, no tool is ever overdue, the tool row reads Waiting for your approval, and when the card closes every clock restarts from that moment; the stop button still cancels. Give the hermes and openclaw ACP adapters the same rule, treating a run the kernel holds in waiting_approval as progress for their 150 s no-progress cancel.
Six in-process captures from the packaged named bundle: a parked send_message card, the same card after Deny, parked list_message_chats and list_mail_messages cards, and the Allowed once state after an answer, with evidence.json recording agent capture through the automation bridge.
The five sensitive device tools described themselves as always requiring an approval dispatch, so the in-app model answered a send request by calling create_desktop_dispatch instead of send_message; that dispatch has no card, nothing renders it, and it never expires. Now that the kernel parks a direct call behind the approval card, say so in the descriptions and capability guidance and tell the model to call the tool directly, including capture_screen, which is now gated the same way, and regenerate the Swift tool surfaces for the changed manifest digest.
sauranshbhardwaj
force-pushed
the
feat/device-tool-descriptions
branch
from
October 15, 2026 17:18
703fd95 to
a1575f0
Compare
Collaborator
|
Reviewed the device tool approval flow end to end - this is careful work, and the security shape holds up on inspection. What I verified against the diff:
One CI note: the Hygiene failure on this head is a runner network flake - desktop-agent-runtime-contract-conformance died on an npm ECONNRESET during dependency install, not a manifest mismatch (Agent Runtime passed on the same head). Please re-run that job; no code change needed for it. Since this rewrites the model-facing instructions for the messaging/mail/screenshot/AppleScript tools to "call directly, the app asks for approval", and it defines the grant semantics for those tools, it needs a human maintainer's sign-off on the approval UX and grant scope before merge (also given the stack on #20973/#20975/#20997). Leaving for maintainer review of the permission semantics - not urgent. This review was generated by AI on behalf of the maintainer team. by AI on behalf of David — if you need David’s attention urgently, please @Git-on-my-level and escalate with |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Invariants carried from PR 2 (its changes show in this diff until it merges; PR 3 itself touches only the tool manifest, its fixture, the regenerated surfaces and one manifest test): INV-AUTH-1 (
product/invariants/auth-session.md) and INV-CHAT-2 (product/invariants/chat-scroll-placement.md), named by PR 2 for the post-handshake card clearing and the card list inChatMessagesView.swift; see PR 2's body.Line-count exceptions carried from PR 2 (this branch is stacked on it, so its file growth shows in this diff until PR 2 merges; PR 3 itself grows no ratcheted file):
Line-Count-Exception: desktop/macos/Desktop/Sources/Chat/AgentRuntimeProcess.swift | 4373 -> 4384 | two kinds in the closed message switch, the post-handshake clear and conditional capability send, a two-line routing case, the owner-revoke case that clears cards, and one clear on daemon exit; the statics, wire message, routing body and clearing live in AgentRuntimeProcess+ToolApprovals.swift, and messageOwnerIsCurrentlyAuthorized drops private so that extension can call it
Line-Count-Exception: desktop/macos/Desktop/Sources/DesktopAutomationBridge.swift | 5172 -> 5173 | one registerToolApprovalActions() call in the builtin registration list; the actions live in Automation/DesktopAutomationToolApprovalActions.swift
Line-Count-Exception: desktop/macos/Desktop/Sources/FloatingControlBar/FloatingControlBarView.swift | 3018 -> 3020 | the approvalSurface argument at the one AIResponseView call site
Line-Count-Exception: desktop/macos/Desktop/Sources/MainWindow/Components/ChatBubble.swift | 2162 -> 2172 | the "Waiting for your approval" caption under the tool row and its accessibility value; the status itself and the icon live in ChatProvider's enum and ToolActivityTimelineLayout
Line-Count-Exception: desktop/macos/Desktop/Sources/Providers/ChatProvider.swift | 6824 -> 6834 | the waitingApproval case of ToolCallStatus with its doc line, its detector mapping, the turn's session id captured for the stall clocks, and the tick-loop lines that ask the approval store about that session; the lookup lives in ChatProvider+ToolApprovalWait.swift
What changed and why
Follow-up to #20938 PR 2, stacked on
feat/desktop-approval-card(fe0cbf0); merge right after it. The five sensitive device tools (send_message,run_applescript,read_message_history,list_message_chats,list_mail_messages) described themselves to the model as "always requires an approval dispatch", and their capability guidance repeated it. In PR 2's live run the in-app pi-mono model therefore answered "send an iMessage" by callingcreate_desktop_dispatchinstead ofsend_message: that model-authored dispatch has no invocation binding, no card renders it, and it never expires, so the person saw nothing and the message was never sent. Now that the kernel parks a direct call behind the approval card (PR 1a, 1b, 2), the two descriptions and four guidance lines inomi-tool-manifest.tssay that Omi asks the person in the app before the tool runs and tell the model to call the tool directly and not create a dispatch first.capture_screen, which PR 1a added to the approval set, gets the same wording in its description and its two guidance lines, replacing "requires explicit approval before image bytes are shared", plus a note that each screenshot is approved on its own, so the model should capture once and reuse the image. The read tools' descriptions were already plain and are unchanged. The manifest digest changes, soGeneratedToolExecutors.swift,GeneratedToolCapabilities.swiftand thetool-manifest.jsonfixture are regenerated withscripts/generate-tool-surfaces.mjs; no Swift logic changes.Product invariants affected
INV-AGENT-* (
product/invariants/agent-control-plane.md) is named becausedesktop/macos/agent/src/runtime/**matches its glob. Unaffected: only model-facing text and the digests derived from it change; policy, parking, grants and the ledger are untouched. INV-CHAT-1 (product/invariants/chat-continuity.md) is named for the same glob and is unaffected for the same reason.How it was verified
From
desktop/macosat 703fd95 (onfeat/desktop-approval-cardfe0cbf0):node --experimental-strip-types scripts/generate-tool-surfaces.mjswrote the two generated files and the fixture (manifest digest e2c2289938e8), and--checkthen reports all outputs match;npm run buildclean;npx vitest run-> 73 files, 959 tests passed, includingtool-surfaces-exhaustiveness.test.tsagainst the regenerated fixture;xcrun swift buildof the app with the regenerated surfaces clean andToolCallStatusTests31/31,AuthorizedToolExecutionTests15/15,ApprovalCardTests24/24;pi-mono-extensionnpm test143/143;desktop-changelog.py validateOK;scripts/pr-preflight --base feat/desktop-approval-card --pr-body-file <this body>-> "PR preflight passed: 31 checks" (base feat/desktop-approval-card, Python 3.11).Verified live with this branch on top of PR 2 (rebuilt bundle): a person asked, with no tool instructions, "Send an iMessage to +1 555 010 0002 saying: testing Omi approvals", and the in-app pi-mono model called
send_messagedirectly, raising the card (Allow Once ran it once; a second ask was denied); "List my recent Mail messages" likewise calledlist_mail_messagesdirectly. Before this change the same plain ask produced acreate_desktop_dispatchrow and the reply "I've requested approval" (PR 2's live run). One run each, so the model's compliance is observed, not guaranteed. Thecapture_screenwording was added after that run and was not exercised live.Tests
No new test:
omi-tool-manifest.test.tsnow expects thecapture_screenguidance to say Omi asks the person in the app (it pinned the old "requires explicit approval" wording), the exhaustiveness test already pins the generated surfaces to the manifest, and the wording has no code path of its own.UI evidence
UI-Evidence: none -- runtime-only change
Failure class (fixes)
Failure-Class: none