fix(agent): allow remaining actions for an approved task - #6113
Conversation
github-actions
Bot
added
needs runtime validation
github-actions
Bot
removed
needs runtime validation
|
Note: This PR is a bundled fix for current agent gate, separately it is pending removal and refactor, see #5818 and related |
github-actions
Bot
added
the
ready for review
StressTestor
left a comment
StressTestor
left a comment
There was a problem hiding this comment.
reviewed at fb3a401. the mechanism holds and i'd merge it, with two things below, one pre-existing and one that wants a maintainer call before it ships wider.
what i verified: approve_task is minted only by consume() on an owner+session-matched pending (tool_approvals.py:400-430, gated again at chat_routes.py:1146). it sets allow_remaining_actions, which becomes a per-run approval_gate_bypassed on the security context (agent_loop.py:3455-3467). nothing else reads or persists that flag, so a new user turn or a step-limit continue builds a fresh context and re-arms the gate. current tool policy, ownership, workspace confinement, and sandbox checks still run on every call (tool_execution.py:697-710). deny consumes and records resolution before returning with no model call (chat_routes.py:1177). 369 tests across the chat/agent/tool files plus the five focused suites the PR documents (test_tool_approval_task_scope, test_tool_approvals, test_external_context_tool_gate, test_tool_policy, test_chat_route_tool_policy) pass here; node --check, compileall, diff --check clean.
one thing to be aware of on "allow for this task": decision_for returns True before it inspects the tool (tool_capabilities.py:631-632), so untrusted content fetched after the click is ungated for the rest of that in-memory run. that's the deal the label makes and i'm fine with it. one line in the specs branch next to the gate contract would stop anyone reading "for this task" as "for this one sealed action".
continuation_query isn't as sealed as PR #6113's description implies. it's correctly absent from public_payload, but on the continuation turn message = pending_tool_approval.continuation_query (chat_routes.py:1184), which reaches the owner-gated stream_status query field (chat_routes.py:1594) and the chat.completed webhook's user_message (chat_helpers.py:1256). neither is behind the tool_approval_continuation gate. it's the owner's own query going to the owner's own webhook, so severity is low, but pre-PR that webhook fired the synthetic "Approved the exact..." string and now it re-emits the real query. worth making that a decision.
pre-existing, since this PR builds more on it
the resumed path appends the sealed result as assistant(content="") then user(result) (agent_loop.py:4738, via _append_tool_results(messages, "", ...)). Anthropic's API rejects a non-final assistant message with empty content, and i reproduced that payload shape locally. this isn't new in #6113: base 032967a already emits the same empty assistant turn on plain approve-once, so the PR inherits it and extends it to approve_task. the one-line fix is content=None instead of "" for the no-prose turn, the same thing the native-tool-call branch already does for Gemini/Ollama. openai-compat and local endpoints take the empty string, so this only bites a native-claude endpoint in agent mode. fine as a follow-up.
small
chatStream.js:18-38intercepts the synthetic approval click for 60s, but the retry loop that waits out an in-flight stream (chat.js:65-79) has no deadline. if streaming outlasts the window the click lands on the send button innewchatmode with an empty composer and opens a new chat (app.js:4034). callingchatForm.requestSubmit()from chat.js instead of.click()drops the interceptor and the race.- the PR body still says the synthetic approval line "stays in transcript history but is excluded from the resumed model prompt". at fb3a401 nothing is persisted (
chat_routes.py:1350,persist_user_message=not tool_approval_continuation) and the bubble is hidden. drop that sentence.
github-actions
Bot
added
needs runtime validation
|
The chat card now reuses the wire value `approve` to mean chat-session scope, and `consume()` returned `allow_remaining_actions=True` for it unconditionally. The skill-test approval route was never updated: it still sends `approve` meaning "once", and its button still reads "Allow once", but the grant it got back set `approval_gate_bypassed` for the rest of the resumed run. That surface wraps the skill body and every transcript byte as untrusted context, so it is the last place where one click should ungate everything that follows. Give `consume()` an explicit `allow_continuation` flag. Callers that own a resumable chat keep the scope the user picked; callers that do not — the skill tester, unattended audits — get SINGLE_ACTION and the gate re-arms behind the sealed action, which is what their label promises.
chatStream.js, compare/index.js and compare/stream.js all changed behaviour but kept their old `?v=`, while chat.js and chatRenderer.js were bumped. A returning browser therefore serves the new chat.js — which now deliberately leaves the composer empty and clicks the send button — next to the cached chatStream.js that has no interceptor. With an empty composer that button sits at `data-mode="newchat"`, so the click opens a new chat and the approval is dropped. Bump the three, and version compare/stream.js's chatRenderer import to match everyone else's so the ask_user keydown listener binds to one module instance instead of two.
With an approval card on screen and focus anywhere outside an input, a bare `1` fired `approve_task` — the widest of the three grants — with no modifier and no confirmation. That card is the one control whose entire purpose is deliberate consent after untrusted context influenced the run, and Deny sits at 3. Label the card with its kind and skip the shortcut for approvals. Ordinary ask_user questions keep 1-3.
…choice renderAskUserCard removes the card as soon as onSubmit accepts, but the resume loop gave up silently after 10s if the originating stream still owned the pane. The user saw the click land, the card vanish, and nothing happen, with no way to get it back. Re-render the card on that deadline and say why. The reroll case still returns without sending — that choice belongs to a stream that no longer exists.
`if decision != "deny"` is always true — the deny path returns a StreamingResponse a few lines above. It reads as if deny still falls through to the toggle restore.
There was a problem hiding this comment.
Pushed five commits on d98da547, one per issue so any can be reverted on its own.
8afb8bff: the skill-test route still sends approve meaning "once" and its button still reads Allow once, but now that approve means chat-session scope that grant set approval_gate_bypassed for the whole resumed run on the surface that wraps the skill body and transcript as untrusted context. consume() takes an explicit allow_continuation; callers without a resumable chat get a single-action grant and the gate re-arms behind the sealed action.
0f5c6f05: chatStream.js and the two compare modules changed but kept their old ?v=. A returning browser gets the new chat.js, which leaves the composer empty and clicks send, next to the cached chatStream.js that has no interceptor; with an empty composer that button sits at data-mode="newchat", so the click opens a new chat and the approval is dropped. Also versioned compare's chatRenderer import so the keydown listener binds to one instance.
8f26110c: with an approval card up and focus outside an input, a bare 1 fired approve_task; Deny is 3. The shortcut now skips approval cards, plain questions keep 1- 3.
abc674b3: a compare pane removed the card on click but returned silently after 10s if the origin stream still owned it, losing the choice. It re-renders the card instead. The reroll case still drops, which is right.
978a686e: if decision != "deny" is unreachable; deny returns above it.
Seven new tests. Full suite locally: 5763 passed, 4 failed, the four also fail on unmodified dev. Not verified: the real model path to a gated action after untrusted context, mobile, Docker.
One left open as your call: the chat-session grant is rebuilt from persisted transcript metadata, so it re-arms on every later turn in that chat, survives a restart, and has no revocation once the card is resolved. Is "forever in this thread" the intent, or should it expire?
Otherwise, looks good to me, approved! I wont merge since I'm the last one to have pushed on this PR.
StressTestor
left a comment
StressTestor
left a comment
There was a problem hiding this comment.
re-reviewed at 978a686, merging.
session scope is the part i wanted certainty on and it holds. the grant marker lives in server-owned message metadata the model never writes to, and AskUserTool builds its payload from an allowlist of question/options/multi, so a tool result can't smuggle in kind/resolved/session_id. probed it directly: a forged ask_user event grants nothing, the grant doesn't read across sessions, and approve_task and deny both confer nothing. 327 pass locally across the approval, gate, policy and compare suites.
two follow-ups, neither blocking. consume()'s allow_continuation defaults to True (tool_approvals.py:433), so a caller that never passes it gets continuation scope for free and skills_routes has to opt out by hand; the safer polarity is the other way round. and the interceptor race from my last pass is still live, chat.js:79 clicks while chatStream.js:38 stops intercepting at 60s, so a stream that outruns the window lands that click on a new chat.
…val JS index.html modulepreloaded chat.js under the pre-odysseus-dev#6113 buster while the script tag used the new one; and sw.js CACHE_NAME was never bumped, so clients kept serving the cached old HTML shell (stale-while-revalidate) and re-running old chat.js that sent approval clicks as literal text. Bump both so the approval card sends control fields instead.
odysseus-dev#6113 bumped app.js's internal imports of chat.js/chatRenderer.js to the new buster but left app.js's OWN reference in index.html at the old 20260815toolapproval4 buster. Browsers therefore served the cached pre-odysseus-dev#6113 app.js, whose imports pulled the old chat.js/chatRenderer.js — the old click handler sends approval as literal text instead of control fields, so 'Allow for this chat session' re-sealed the gate every turn. Bump app.js (modulepreload + script tag) and the SW CACHE_NAME so the fresh module graph is fetched.
…val JS index.html modulepreloaded chat.js under the pre-odysseus-dev#6113 buster while the script tag used the new one; and sw.js CACHE_NAME was never bumped, so clients kept serving the cached old HTML shell (stale-while-revalidate) and re-running old chat.js that sent approval clicks as literal text. Bump both so the approval card sends control fields instead.
odysseus-dev#6113 bumped app.js's internal imports of chat.js/chatRenderer.js to the new buster but left app.js's OWN reference in index.html at the old 20260815toolapproval4 buster. Browsers therefore served the cached pre-odysseus-dev#6113 app.js, whose imports pulled the old chat.js/chatRenderer.js — the old click handler sends approval as literal text instead of control fields, so 'Allow for this chat session' re-sealed the gate every turn. Bump app.js (modulepreload + script tag) and the SW CACHE_NAME so the fresh module graph is fetched.
odysseus-dev#6113 left app.js's own cache-buster stale (its internal imports were bumped but index.html still references app.js at 20260815toolapproval4). Our local bump of app.js + service-worker CACHE_NAME fixed the approval-click loop but broke upstream's regression test (test_frontend_tool_approval_uses_opaque_id_ and_fixed_decisions asserts the old buster appears exactly 2x). Revert to match upstream/dev; workaround is a hard refresh after deploy, and the real fix belongs upstream.
Rewrite the orientation doc to reflect the current Improvements branch state: frontend static assets and cache-buster behavior, tool approval gate history (upstream odysseus-dev#6113), and consolidated lessons learned from 2026-08-18/19. No code changes.
3 participants
Summary
Fix the repeated exact-approval loop from #6112 while keeping approval scope explicit and temporary. Approval continuation is treated as control-plane input rather than a new user turn: it reuses the interrupted request's bounded, digest-bound continuation query, memory/RAG/skills context, and server-selected tool candidates without persisting or rendering a synthetic approval message. The approval card now offers Allow for this task and Allow for this chat session alongside Deny.
The choices are:
Task scope does not carry into a new ordinary user turn. Chat-session scope is limited to the same server-owned chat session and does carry to later requests in that chat; a different chat or fork does not inherit it. Current disabled-tool rules, owner/admin checks, document freshness, workspace confinement, dispatcher policy, and any process sandbox remain authoritative.
Target branch
Linked Issue
Fixes #6112
Part of #5815
Related: #6083 / #6084, #6092 / #5819, and #6103 / #6104.
This is an independent current-dev bug fix prepared against 43682d4. It does not depend on #5818, #6084, #5819, or #6104. #6084 is an overlapping global-disable alternative; both should not be treated as cumulative product behavior without a maintainer decision.
Type of Change
Checklist
Behavior contract
How to Test
In a running app, use Agent mode with web plus memory/skills or file/shell tooling enabled, then trigger external context followed by a gated action. Runtime validation was not performed during preparation.
Choose Allow for this task. Confirm the sealed action executes, the original memory/RAG/skills context and selected tools remain available, later gated actions in the same continued task do not show another approval card, and a new ordinary message re-arms the gate.
Repeat in the same chat and choose Allow for this chat session. Confirm the sealed action executes and later otherwise-gated requests in that chat do not show another card at this gate, while a different chat or fork still asks.
Choose Deny and confirm the control resolves without executing the sealed action.
Disable Bash/web or use a non-admin account and confirm neither scope choice overrides those restrictions.
Visual / UI changes — REQUIRED if you touched anything that renders
This replaces the former one-shot choice with two explicit continuation-scope choices and adds numeric 1–3 shortcuts through the existing ask-user option handling. The existing card/options renderer is reused; no new CSS or component styling is introduced.
Screenshots / clips
The attached images document the approval-card layout. They are visual evidence only; end-to-end model/tool behavior and mobile-specific wrapping were not validated during preparation.
Preparation-environment validation