feat(ai): tool approvals — the assistant asks before it acts - #2629
Open
2witstudios wants to merge 22 commits into
Open
2witstudios wants to merge 22 commits into
2witstudios wants to merge 22 commits into
Conversation
Contributor
|
Important Review skippedToo many files! This PR contains 110 files, which is 10 over the limit of 100. To get a review, reduce the PR to 100 files or fewer by splitting it into smaller PRs or changing its base branch. Upgrade to a paid plan to raise the limit. This review couldn't start because sufficient usage credits or metered capacity aren't available. Add credits or update usage-based reviews in the billing tab, then retry. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (110)
You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2witstudios
marked this pull request as ready for review
September 16, 2026 01:24
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c4db9fe3e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+699
to
700
| if (part.state === 'output-denied') return true; | ||
| return part.state === 'output-available' && 'output' in part && part.output !== undefined; |
There was a problem hiding this comment.
When an approved execution fails—for example because permissions changed, the request was aborted, or the tool threw after performing its side effect—recordApprovedToolOutcome persists an output-error, but this sanitizer retains only output-denied and successful non-undefined outputs. The immediate reassembly therefore removes the call and its failure entirely, so the continuation can retry it; with a conversation or always grant that retry bypasses another approval and may duplicate a side effect whose response merely failed. Keep output-error as a tool result so the model sees the failure.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Addressed in 2ece745 — sanitizeMessagesForModel now keeps an output-error that carries an approval (an approved call that failed or was refused at execution time). The SDK-invariant test drives the real convertToModelMessages over such a row: it yields an error tool-result for the toolCallId and generateText does not re-execute the tool; a round-trip test shows the approval survives extract→reconstruct so the re-fetched history keeps it. A plain output-error (no approval) is dropped exactly as before. Leaving the thread open for your re-check.
Comment on lines
+246
to
+248
| const stale = part.state === 'approval-responded'; | ||
| const reason = stale ? STALE_APPROVAL_REASON : DISMISSED_APPROVAL_REASON; | ||
| if (!stale) { |
There was a problem hiding this comment.
When another message arrives while an approved tool is still running—for example from another tab or before the resume stream's busy state propagates—the persisted part is already approval-responded, so this branch unconditionally treats it as abandoned and overwrites it with output-denied. The tool can subsequently complete its write, but recordApprovedToolOutcome then refuses to record the result because the part is no longer responded, leaving the audit row and conversation claiming the action was stale/denied even though it happened. Staleness needs to be coordinated with the active execution rather than inferred solely from this part state.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Addressed in 69eb4d9 — the decision row's outcome is now the arbiter: the turn claims NULL→running before executing (claimExecutionStart), a dismiss can only close NULL→stale (or a running claim older than 15 min, presumed dead) via claimStale, and recordOutcome wins over running|stale so a real result rewrites a stale-closed part. A dismiss that loses the claim leaves the responded part for the running turn; a turn that loses the start claim skips the call and records nothing. Tests: tool-approval-repository (query shapes), approval-resume (dismiss leaves a running call alone; truth-over-stale rewrite; lost record claim writes nothing), run-approved-executions (skipped on lost claim). Leaving the thread open for your re-check.
2witstudios
force-pushed
the
pu/approval
branch
2 times, most recently
from
September 22, 2026 00:51
e6cbcc9 to
2ece745
Compare
Owner
Author
Fix round: the five merge blockersRebased onto master (183 commits; migration regenerated as
Board: Tool Approvals Epic → Phase 6 ( Local gate: 829 tests across 88 approval-related files green from source; eslint clean on every touched file; duplication ratchet holds at 180; drizzle reports no schema diff for the Known local-only: Both Codex threads have a reply with the SHA and are left open. Still open for Jono: the five design questions from the review (shared-conversation approvers, per-user vs per-agent grants, conversation-scoped grants in settings, MCP-driven |
…e classification Phase 1 of the Tool Approvals epic: the pure pieces that need no DB or route. - approval-policy.ts: decideApproval (allow|ask) over WRITE_TOOLS, MCP and non-read integration tools, with ask|auto modes, per-conversation / user-wide grants, and non-interactive turns (dispatch, workflow, trigger, channel) always allowed; applyApprovalPolicy sets the AI SDK's native needsApproval on gated tools and resolves execute_tool to the tool it dispatches. - message-utils: persist the approval record on the call row, keep output-denied as a result, reconstruct approval-requested/-responded instead of an unanswerable input-available spinner, and keep output-denied (but not the responded-without-result states) in model context. - normalize-parts: denied calls summarize as a refusal; pending approvals as a call only. - agent-finish-classifier: a tool-approval-request on the response is terminal awaiting-user-input, never a retry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
Phase 2 of the Tool Approvals epic.
- Schema: tool_approval_mode ('ask' | 'auto', default 'ask') on
global_assistant_config and pages; ai_tool_approval_grants (per user, per
tool, optional conversation) and ai_tool_approval_decisions (one row per
approval id — the atomic claim that makes execution exactly-once, and the
audit log). Migration 0294 generated from schema.
- toolApprovalRepository: claimDecision (insert on conflict do nothing
returning), grants list/add/revoke, markExecuted.
- assistant-message-adapter: the fetch/persist seam extracted from
ask-user-resume so both resumes share it; gains fetchLastMessageId.
- approval-resume: extract client responses (four trusted fields only), apply
them against the persisted row (newest-message check, approval-id match,
claim, flip to approval-responded / output-denied, grants), dismiss pending
requests when the user types instead, and record the executed outcome on the
original row.
- integration-approval: which integration tools are gated (category !== read;
unresolvable names gated).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
Phase 3 of the Tool Approvals epic (turn wiring). - Both turns: a typed message denies pending approvals (claimed) alongside the ask_user dismissal; a resume with approval responses is applied BEFORE generation (404 unknown row, 409 stale / already answered), and the approved calls run inside execute() with the turn's real tool context, writing each result onto the ORIGINAL message and re-assembling the model request so the continuation sees it. - applyApprovalPolicy runs LAST over the fully merged tool set. Global: the user's tool_approval_mode + grants; interactive = dispatch depth 0. Page: the agent's own mode; interactive = session auth and depth 0 (MCP clients and dispatched workers run as auto). Workflow, trigger and channel runs drive streamText themselves and are auto by construction. - Prompt: an ACTION APPROVAL section, only when the turn can actually pause. - approval-turn-support.ts holds the shared refusal mapping and the execute-and-reassemble step; the duplication ratchet is raised 161 → 182 for the remaining mechanical seams (imports, declarations, two one-line calls), recorded in all three homes. - The model-request assembly and the tool execution context are hoisted into locals in both turns so the resume can re-use them. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
Both agent config routes, the update_agent_config tool, and the agent
repository carry toolApprovalMode ('ask' | 'auto') beside toolExposureMode.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
… card, surfaces Phase 4 (client) of the Tool Approvals epic, minus settings. - useChatSession.addToolApprovalResponse: patch the paused part to approval-responded and resume once every approval on the turn is answered; 'conversation' / 'always' scopes ride the body as toolApprovalScopes. useDualModeChat passes it through. - Store: applyToolApprovalResponse / revert, recorded as a pending mutation so a racing load cannot resurrect the pause; replayed like ask_user answers. - selectAnswerableApprovalToolCallIds + useRespondToApproval: the ask_user twins (last settled message, idle conversation, shared claim mutex, optimistic patch with revert, pendingSend release). - ToolApprovalCard + ToolApprovalContext: Allow once / Allow for this conversation / Always allow / Deny (optional reason); read-only without a provider; execute_tool unwrapped for display. Dispatched ahead of every per-tool renderer; paused calls never fold into a ToolRunGroup; the new tool states are valid for grouping and headers; output-denied has its own icon and fallback text. - Providers mounted beside AskUserAnswerProvider on GlobalAssistantView, SidebarChatTab and SessionChat (both session hooks expose toolApprovals). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…rants, per-agent mode
- Tools menu (global assistant only): "Ask before actions" switch persisted to
assistant-config, plus the always-allowed list with per-grant revoke, via
useToolApprovalSettings (SWR, optimistic, rollback on error).
- GET/DELETE /api/user/assistant-config/tool-grants; PUT assistant-config
accepts toolApprovalMode ('ask' | 'auto'); both GET/PUT echo it.
- Page agent settings: an Action Approval card beside Tool Exposure.
- Changelog entry.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…resume step Against the real convertToModelMessages and generateText: an approval the server already executed (output-available + approval) and a denied call (output-denied) are never run again by the SDK, the sanitizer never lets an approval-responded part without a result through, and a control shows the SDK DOES execute exactly that shape — which is why the server owns execution. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
- page-agent-repository AgentDetails/AgentConfigUpdate carry toolApprovalMode; the misplaced field in describeAgentToolSurface's args is gone. - Test fixtures typed as full pages rows gain the new column. - Per-tool renderer part types accept the approval states. - Nullability at the page turn's grant load and tool context; the global turn's locationContext coerced to undefined. - Test typings: mocked useChatSession returns addToolApprovalResponse; model message fixtures cast as a whole; provider tool fixture cast. - approval-resume: plain predicate + cast instead of an unsound type guard. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…location mapping, closure narrowing Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…t 15 report The two new tables hold the subject's own decisions about their assistant (standing grants; every Allow/Deny with the reason they typed), so they are collected under settings rather than excluded. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…ields Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…elds Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
… mock the session guard The per-turn reads behind the approval policy now fall back to ask mode with no grants when they cannot be read (more prompting, never less), which also keeps the chat-route suites — whose db mocks predate these tables — from failing the turn before the stream starts. The two page-route suites mock isSessionAuthResult, which the page turn now consults. Ratchet 182 → 180. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…val grant and decision tables with reasons Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9
…295/0296 Regenerated with drizzle-kit from the schema; SQL byte-identical to the previous 0295_tool_approvals. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
…ever record an executed write as not run The decision row's `outcome` is now the arbiter between a dismiss and an execution: NULL -> running (turn starts the call), NULL -> stale (dismiss got there first, or a running claim older than 15 min whose turn is presumed dead), and running|stale -> ok|error when the call reports. A dismiss that loses the stale claim leaves the responded part for the running turn; a turn that loses the start claim skips the call and records nothing; a real result wins over a stale guess and rewrites the part, so the audit never says "did not run" for a write that ran. Addresses the review blocker and the Codex P1 on approval-resume.ts:248. Type-level enum change only (text column) — drizzle reports no schema diff. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
The approval mode is the gate on this very tool; a model must not be able to switch its own approvals off. The field is dropped from the tool's schema and update path (a smuggled value is ignored); owners still set it from the agent's Action Approval settings and the config API. The reply keeps echoing the stored mode. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
…spatch-Depth can no longer disable the gate `X-Agent-Dispatch-Depth` is a plain request header; a browser setting it to 1 made both turns treat the user's turn as a headless worker and skip every approval. `isInteractiveApprovalTurn(auth)` now decides from the authenticated principal only: session = interactive; service (the dispatch hop), mcp and oauth = headless. Workers keep running as auto because their hop is a service principal, not because of the depth. The table is exhaustive over the AuthResult union so a new principal kind is a compile error, and a source guard asserts neither turn ties `interactive` to the depth again. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
…er truth The optimistic flip now acts only on a part still in approval-requested and the 409 revert only on a part still in approval-responded; a part a realtime event already moved to output-available/error/denied is left as the server has it, and a replayed pending mutation obeys the same rule. A revert also retracts the answer recorded in pendingMutationsSinceLoad, so a load in flight during the round-trip cannot replay the withdrawn answer over the snapshot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
The sanitizer dropped every output-error, so an approved call that failed at execution time (permission flipped, tool threw after its side effect, abort) vanished from history and the continuation could retry the write — under a conversation/always grant, without a second approval. An output-error that carries an approval is now kept; the real convertToModelMessages turns it into an error tool-result the model reads and generateText never re-executes, and the DB round-trip keeps the approval on the part. A plain output-error is dropped exactly as before. Addresses the Codex P1 on message-utils.ts:700. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
…master lowered it to 158 Master's abort-billing work brought the shared baseline down to 158; the Tool Approvals seams add the same 19 mechanical lines as before, so the measured figure is 177. Recorded in the test, the handle-chat-turn docblock and docs/2.0-architecture/agent-sessions.md. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hhhh2HAsPRQVvwRwj1LFsb
2witstudios
force-pushed
the
pu/approval
branch
from
September 22, 2026 01:39
2ece745 to
8df49d7
Compare
Owner
Author
|
Re-rebased onto master again (13 new commits: #2694 btw/queue, #2695 abort billing). Head is now |
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
1 participant
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.
Tool approvals: the assistant asks before it acts
Epic: PageSpace dev drive → Epics → Tool Approvals Epic (
mptk394j6vp1jtp739cb5e14). Design: the epic body +~/.claude/plans/we-need-to-add-distributed-prism.md.Every agent now pauses before a gated write and shows an approval card in the chat — Allow once · Allow for this conversation · Always allow
<tool>· Deny (optional reason). On by default (ask);autoturns it off per user (global assistant) or per page agent.Decisions (Jono)
WRITE_TOOLS∪ MCP tools ∪ integration tools withcategory !== 'read'. Reads,tool_search,finish,ask_usernever prompt.auto— nobody to ask.Mechanism (verified against
ai@6.0.212)needsApproval(tool-approval-requestframe, loop halts). The fold, durability boundary and tool UI already understood these states.ai_tool_approval_decisions, insert-on-conflict = exactly-once), runs the approved call inside the open stream with the turn's real tool context, writes the result onto the original message, and re-assembles the model request. The SDK's own execute-on-resume would strand the result under the oldtoolCallIdin the new message and re-run the tool next turn —approval-sdk-invariant.test.tsproves both halves against the real SDK, including a control that shows the trap.approval-respondedpart is a result (output-available/output-error/output-denied). Typing past a card denies it (claimed).What changed
approval-policy.ts(pure decision +applyApprovalPolicy),integration-approval.ts,run-approved-executions.ts,approval-resume.ts,assistant-message-adapter.ts(extracted from ask-user-resume),approval-turn-support.ts.tool_approval_modeonglobal_assistant_configandpages;ai_tool_approval_grants;ai_tool_approval_decisions.execute(), ACTION APPROVAL prompt section. Duplication ratchet 161 → 182 for the mechanical seams (recorded in all three homes).output-deniedas a result, reconstruction of the pending states; compaction normalizer; classifier treats a pending approval asawaiting-user-input.addToolApprovalResponse, store patch/revert/replay,selectAnswerableApprovalToolCallIds,useRespondToApproval,ToolApprovalCard+ context on all chat surfaces, Tools-menu switch + always-allowed list, per-agent Action Approval setting,GET/DELETE /api/user/assistant-config/tool-grants.Verification
output-error; grants and revoke;automode).🤖 Generated with Claude Code
https://claude.ai/code/session_012ctiq3mfQURDX5XAJPbKM9