fix(tools): reuse a turn's just-granted approval for identical protected-write follow-ups - #109617
uttkarsh-26 wants to merge 1 commit into
Conversation
ebc070b to
96a3671
Compare
gaoanze888
left a comment
There was a problem hiding this comment.
The focused suite passes 63/63 at 96a3671de6b219deda0ce44d1b89e801763a3cce, and Ruff/py_compile/diff check are clean, but this changes the security meaning of Allow Once rather than merely deduplicating an identical request.
The cache key contains (session, turn, display, description). For this guard, display and description contain only the target name(s), not the proposed content, patch, tool-call identity, or a predeclared batch identity. Consequently, after the user approves one write to AGENTS.md, any later write to that path during the next 120 seconds of the same turn is silently authorized—even if its content is different, generated after the approval, or originates from a later model/tool round. The new happy-path test demonstrates this directly: approval for revision 1 also authorizes revision 2 and revision 3 without those operations having been presented when the user tapped Allow Once.
That is especially risky for an instruction-persistence boundary: a prompt-injected turn can front-load a benign write, obtain one-operation consent, then replace it with unrelated instructions under the cached target-only grant. A shared turn_id establishes execution lineage, not that multiple independently generated calls are one user-approved operation. The 120-second wall-clock limit does not restore that authorization binding.
Please keep Allow Once bound to exactly the request that was shown. If the product wants one approval for a serial batch, the executor needs to predeclare and bind the complete immutable batch (targets plus content/patch digests and a batch identity) before prompting, or the UI must explicitly ask for a broader scope such as “allow all protected writes to these targets for the rest of this turn.” A post-hoc target-only cache should not silently widen once. At minimum add a regression where a later same-turn write has different content/patch identity and prove it re-prompts; also cover a second tool round in the same turn, not only three direct helper calls.
96a3671 to
35f4012
Compare
…ted-write follow-ups
A gateway turn that emits a serial batch of protected-instruction writes (e.g.
three patch calls to AGENTS.md) posted one approval card per call: one tapped
"Allow Once" covered only the first, and the rest re-prompted and failed closed
as timed out ("Silence is not consent") even though the human had answered.
Record a granted gateway decision keyed by (session, turn, display, description,
operation digest) and short-circuit an identical follow-up in the same turn while
the grant is fresh (120s). The digest covers the exact operation the human was
shown — tool, target path(s), write content / patch old_string+new_string (or the
whole V4A patch) — so a later same-turn write of different content, or a
different patch, re-prompts rather than riding the first answer. Deny/timeout
never record, a missing turn or payload identity never reuses, a same-basename
target elsewhere asks again, and the interactive CLI keeps per-call prompts.
35f4012 to
aa3e9e8
Compare
|
Addressed in aa3e9e8 (branch rebased onto current main, ahead 1 / behind 0). The grant key now carries a digest of the exact operation the human approved — tool, target path(s), and the write content / patch Regressions covering your minimum, all in
The previous serial-batch test asserted exactly the bug you described — one card for three different contents — and now asserts one card for three identical ones. Receipts:
|
What does this PR do?
A gateway turn that emits a serial batch of protected-instruction writes (e.g. three
patchcalls toAGENTS.mdin one turn) asks the human once per call. Because the tool executor runs the batch serially, one tapped Allow Once covers only the first call: the remaining calls of the same batch re-prompt — and, with no further taps, end at the approval timeout, so the agent reportsBLOCKED ... Silence is not consent.for a batch the human just answered. The follow-up cards carry the identical<write to AGENTS.md>label, so nothing tells the user their earlier tap was seen.This PR makes a turn's identical follow-up reusable: on the gateway surface,
_request_protected_instruction_approvalrecords a freshly granted decision keyed by(session, turn, display, description)and short-circuits an identical request within the same turn while the grant is fresh (120s window). Boundaries are strictly fail-closed: deny/timeout never record, a missing turn identity never reuses, a different target/description re-asks, and the interactive CLI path is untouched (per-call prompts remain there).Related Issue
Fixes #109616
Type of Change
Changes Made
tools/file_tools_write_guards.py— gateway branch of_request_protected_instruction_approval: grant cache (_protected_grant_key/_protected_grant_is_fresh/_record_protected_grant, 120s window, pruned under a lock) + reuse short-circuit for an identical follow-up in the same turn.tools/approval_context.py— addget_current_turn_id()(the_approval_turn_idcontextvar already existed and is already bound around tool execution bymodel_tools.py; only the public getter was missing).tests/tools/test_file_write_safety.py— two invariant tests + grant-cache isolation fixture.How to Test
main:scripts/run_tests.sh tests/tools/test_file_write_safety.py -k serial_batch→ fails:serial batch re-prompted the human 3 timesfor one 3-write batch (deterministic repro of the reported symptom).test_batch_reuse_respects_turn_deny_and_target_boundarieslocks the fail-closed cases (new turn re-asks, deny is never reused, no turn identity never reuses).scripts/run_tests.sh tests/tools/test_file_write_safety.py— 63 passed, 0 failed.AGENTS.mdin one turn → one card; approve once; the batch completes with no extra cards.Checklist
Code
fix(scope):,feat(scope):, etc.)scripts/run_tests.sh: the new/updated file (63 passed) plustests/tools/test_approval*.py,tests/tools/test_request_tool_approval.py,tests/tools/test_binary_document_write_guard.py,tests/acp/test_permissions.py,tests/cli/test_cli_approval_ui.py(all green). Full suite left to CI.Documentation & Housekeeping
docs/, docstrings) — docstrings only; behavior notes live in_request_protected_instruction_approvaland the test class contractcli-config.yaml.exampleif I added/changed config keys — N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A