Scope a session patch grant to the file it approved - #6251
Conversation
…6247) `build_approval_grouping_key` is the scope of an "approve for the session" decision: two patches share that grant exactly when they share the key. For apply_patch the key was built by a second, weaker parser than the one the executor uses - it read paths only from `+++ b/` headers and the `replace`/`changes` array, never from the tool's own top-level `path` argument, and discarded the normalizer's error with `Err(_) => {}`. When it found nothing it returned the literal constant "no_files". Four supported shapes hit that constant: the documented `apply_patch{path, patch}` override (a bare hunk has no `+++` line, and apply_patch.rs itself tells the model "Ensure the patch includes ---/+++ headers or provide `path`"), the same shape through `File{action:"patch"}`, any `--no-prefix` diff (the executor's normalize_diff_path accepts `+++ src/x.rs` while the fingerprint demanded the literal `b/`), and any delete-only diff. So approving a card that read "patch .env.example" also approved every later path-less patch - to `.env`, to `.codewhale/settings.json` which decides what runs on the machine, to any absolute path outside the workspace, and in a non-git workspace to every file in the tree, since the #5185 carve-out is disabled there. No second card, no notice, no receipt. is_session_approved_for_tool is a bare set lookup on this key, and its own comment names this exact class: "approving one shell command used to auto-approve the entire shell tool for the session. The contains(tool_name) clause was the escalation (ops R2)." That escalation was closed for shell. It was open for patches, through a constant instead of a tool name. The fix is reuse, not new parsing: hash `preflight_apply_patch(input)`'s touched_files - already `pub`, already what core/engine.rs and auto_review.rs call for the permission path - which folds the path override, prefix-less headers and tab timestamps. Both empty arms now fail closed to a digest of the input rather than to a shared constant, so an unparseable patch is its own family and matches nothing but a byte-identical repeat. Three regression tests, each confirmed failing without the fix by reverting the resolver and keeping the tests: grouping_key_scopes_a_path_override_to_its_own_file FAILED -> ok grouping_key_reads_prefix_less_diff_headers FAILED -> ok grouping_key_fails_closed_on_an_unresolvable_patch FAILED -> ok tools::approval_cache + tools::apply_patch + approval 323 passed; 0 failed (--test-threads=1) cargo fmt --all -- --check clean cargo clippy --workspace --all-targets --all-features --locked (CI's allow list) clean One note for the reviewer: at --test-threads=2 this set reported task_manager::tests::pending_approval_suspends_idle_and_timeout_denial_settles_failed as failing. It passes isolated and passes at --test-threads=1; it is the same load-sensitive timeout class as the compatibility_stream tests, not this change. check-blocking-calls-budget.py still fails on terminal_input.rs - that is main's existing red, fixed in #6243, not introduced here. Closes #6247 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AJENKJ2smviQW4FVGzUTk9
|
Claude finished @Hmbown's task in 3m 27s —— View job Review: Scope a session patch grant to the file it approved (#6247)
SummaryThis is a solid, minimal fix and a good example of rung 2 of the ponytail ladder in action: it deletes a second, weaker patch parser ( Correctness
Provider/model/route safety — not applicable; no model/provider selection logic touched. Reuse/simplification — this is the standout part of the PR: it deletes a duplicate parser instead of adding a third one, matching the "an abstraction must delete caller code" principle from AGENTS.md. Tests — the three new tests are well-targeted and, per the PR description, were confirmed to fail against the pre-fix implementation (path-override scoping, prefix-less header parity with the executor, and closed-failure non-collision). I was not able to execute Nit (non-blocking): the module-doc table at the top of the file ( No correctness, safety, or security issues found. Good change. |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
Complete review coverage: 1/1 passes, 4 file patches, sha256:78aedcb62402a68cd5e569e3b69b65d0331aa35b83b51f4b05bda68fafbf4718.
Pass 1: The change replaces the ad-hoc +++ b/ line scanner that produced the apply_patch session-grant scope with preflight_apply_patch(input).touched_files (the same resolver the permission path uses), and makes both the error and empty arms fail closed to a per-input digest instead of the shared no_files constant. The core logic is sound and the three added tests are well targeted. The main gaps are regression tests for two of the four shapes the PR itself names as previously collapsing to no_files (delete-only diffs, File{action:"patch"}), and a couple of verification points around the removed sentinel and the fallback digest.
Findings
- [WARNING] Two of the four reported un-scoped shapes have no regression test (
crates/tui/src/tools/approval_cache.rs:410)
The PR description names four supported shapes that used to produce the sharedno_fileskey: (1)apply_patch{path, patch}, (2) the same viaFile{action:"patch", path, patch}, (3)--no-prefixdiffs, (4) delete-only diffs. The new tests cover (1) and (3) plus the fail-closed arm, but not (2) or (4). That matters because the fix is only as good aspreflight_apply_patch's coverage of those inputs: if the resolver reports an emptytouched_filesfor a delete-only diff (+++ /dev/null), such patches now land in theno_target_<hash>arm — safe (no escalation) but not scoped to the file as the changelog claims, and a later delete of the same file would still need a fresh card because the digest covers the whole input. Without a test the actual behavior for shape (4) is unverified in either direction, and shape (2) is asserted nowhere. A test tyingpreflight.touched_filesto the executor's target set for these two shapes would pin the invariant the whole change rests on. - [WARNING] The
no_filessentinel is gone; audit for other consumers (crates/tui/src/tools/approval_cache.rs:165)
hash_patch_pathspreviously returned the literal"no_files"for every unparseable, path-less or non-patch input, and that string was part of the grouping key surfaced to the rest of the app. It is now never produced. Anything that keyed off it — approvals UI copy, telemetry/counters, an integration test in another module that assertspatch:no_files, or docs — will silently change meaning or start failing. The PR only reports a run over thetools::approval_cache + tools::apply_patch + approvalfilters, so a workspace-wide grep/test forno_files(and forpatch:no_files) is worth doing before merge. - [INFO] Fail-closed guarantee depends on
hash_json_valuecovering the whole input (crates/tui/src/tools/approval_cache.rs:153)
Both new arms (unparsed_<digest>andno_target_<digest>) are only fail-closed ifhash_json_value(input)is a function of the entire JSON value. If it hashes a filtered subset (for example only a specific key, or a canonicalised projection), then two different unparseable patches that agree on that subset would still share one key — i.e. the original escalation would survive in a smaller form.hash_json_valueis defined outside this hunk, so please confirm it digests the full value; a short comment on the helper stating that requirement would keep future edits from weakening it. - [INFO] Intentional UX change: path-less/unresolvable patches lose their family grant (
crates/tui/src/tools/approval_cache.rs:162)
Previously any unparseable or path-less patch shared one session key, so one approval covered all of them (the bug). Now each distinct byte input gets its own key, so a session approval for such a patch is effectively one-shot until the exact input repeats. That is the right fail-closed behavior and it is documented in the rustdoc and both changelogs, but it is a user-visible increase in prompt frequency for the shapes that previously relied on the shared constant, so it is worth calling out in the PR body/release notes beyond the security framing.
Suggestions
crates/tui/src/tools/approval_cache.rs:410— Add regression tests for the two shapes the PR description names but the new tests do not exercise: a delete-only diff (--- a/foo.rs/+++ /dev/null, and the prefix-less variant) andFile{action:"patch", path, patch}. Assert what actually happens today (either that the key is scoped tofoo.rs, or that it falls into the per-inputno_target_digest and therefore differs between two different delete targets). Without this, the claim in the changelog that delete-only diffs used to collapse to one key is untested in both the old and the new implementation. No literal replacement is provided because the expected key depends on whetherpreflight_apply_patchrecords deleted paths, which is not visible in this diff.crates/tui/src/tools/approval_cache.rs:153— The fail-closed contract of both new arms rests onhash_json_valuedigesting the whole input value. Add a brief note at the helper's definition (or at the call sites here) stating that requirement, so a future change that narrows the digest cannot silently reintroduce a shared key for distinct unparseable patches. No literal replacement is offered because the helper is defined outside this hunk.
Assessment
Pass 1: The fix is correct and well-reasoned. Reusing preflight_apply_patch().touched_files removes the second parser and guarantees the fingerprint and the permission path agree on what a patch targets; the two fail-closed arms close the residual hole where an unresolvable patch collapsed to a shared constant. Worth noting the property that makes the fix robust: under-reporting by the resolver cannot escalate — a partial path set only narrows the grant, and an empty set now falls into the per-input digest — so the only escalation route was the shared constant, and it is gone. Remaining work is coverage and verification rather than correctness: test the delete-only and File{action:"patch"} shapes the PR itself cites, grep the workspace for dependents of the removed no_files sentinel, and confirm hash_json_value digests the full input. The two reds the author discloses (task_manager timeout flake at --test-threads=2, check-blocking-calls-budget.py on terminal_input.rs) are plausibly pre-existing and out of scope for this diff.
Advisory review by Codewhale (codewhale review --pr 6251 --post, head 04804a79e086714e70df1360f92091db65980362). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| "the shared constant must not survive anywhere: {rendered}" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
[WARNING] Two of the four reported un-scoped shapes have no regression test
The PR description names four supported shapes that used to produce the shared no_files key: (1) apply_patch{path, patch}, (2) the same via File{action:"patch", path, patch}, (3) --no-prefix diffs, (4) delete-only diffs. The new tests cover (1) and (3) plus the fail-closed arm, but not (2) or (4). That matters because the fix is only as good as preflight_apply_patch's coverage of those inputs: if the resolver reports an empty touched_files for a delete-only diff (+++ /dev/null), such patches now land in the no_target_<hash> arm — safe (no escalation) but not scoped to the file as the changelog claims, and a later delete of the same file would still need a fresh card because the digest covers the whole input. Without a test the actual behavior for shape (4) is unverified in either direction, and shape (2) is asserted nowhere. A test tying preflight.touched_files to the executor's target set for these two shapes would pin the invariant the whole change rests on.
| // The resolver parsed the input but found no target. Fail closed for | ||
| // the same reason as the error arm above: a shared key here is a | ||
| // shared grant. | ||
| return format!("no_target_{}", hash_json_value(input)); |
There was a problem hiding this comment.
[WARNING] The no_files sentinel is gone; audit for other consumers
hash_patch_paths previously returned the literal "no_files" for every unparseable, path-less or non-patch input, and that string was part of the grouping key surfaced to the rest of the app. It is now never produced. Anything that keyed off it — approvals UI copy, telemetry/counters, an integration test in another module that asserts patch:no_files, or docs — will silently change meaning or start failing. The PR only reports a run over the tools::approval_cache + tools::apply_patch + approval filters, so a workspace-wide grep/test for no_files (and for patch:no_files) is worth doing before merge.
|
|
||
| let mut paths: Vec<&str> = Vec::new(); | ||
| let Ok(preflight) = crate::tools::apply_patch::preflight_apply_patch(input) else { | ||
| return format!("unparsed_{}", hash_json_value(input)); |
There was a problem hiding this comment.
[INFO] Fail-closed guarantee depends on hash_json_value covering the whole input
Both new arms (unparsed_<digest> and no_target_<digest>) are only fail-closed if hash_json_value(input) is a function of the entire JSON value. If it hashes a filtered subset (for example only a specific key, or a canonicalised projection), then two different unparseable patches that agree on that subset would still share one key — i.e. the original escalation would survive in a smaller form. hash_json_value is defined outside this hunk, so please confirm it digests the full value; a short comment on the helper stating that requirement would keep future edits from weakening it.
|
|
||
| if paths.is_empty() { | ||
| return "no_files".to_string(); | ||
| // The resolver parsed the input but found no target. Fail closed for |
There was a problem hiding this comment.
[INFO] Intentional UX change: path-less/unresolvable patches lose their family grant
Previously any unparseable or path-less patch shared one session key, so one approval covered all of them (the bug). Now each distinct byte input gets its own key, so a session approval for such a patch is effectively one-shot until the exact input repeats. That is the right fail-closed behavior and it is documented in the rustdoc and both changelogs, but it is a user-visible increase in prompt frequency for the shapes that previously relied on the shared constant, so it is worth calling out in the PR body/release notes beyond the security framing.
| "the shared constant must not survive anywhere: {rendered}" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add regression tests for the two shapes the PR description names but the new tests do not exercise: a delete-only diff (--- a/foo.rs / +++ /dev/null, and the prefix-less variant) and File{action:"patch", path, patch}. Assert what actually happens today (either that the key is scoped to foo.rs, or that it falls into the per-input no_target_ digest and therefore differs between two different delete targets). Without this, the claim in the changelog that delete-only diffs used to collapse to one key is untested in both the old and the new implementation. No literal replacement is provided because the expected key depends on whether preflight_apply_patch records deleted paths, which is not visible in this diff.
| } | ||
| Err(_) => {} | ||
| } | ||
| let mut paths: Vec<&str> = preflight.touched_files.iter().map(String::as_str).collect(); |
There was a problem hiding this comment.
The fail-closed contract of both new arms rests on hash_json_value digesting the whole input value. Add a brief note at the helper's definition (or at the call sites here) stating that requirement, so a future change that narrows the digest cannot silently reintroduce a shared key for distinct unparseable patches. No literal replacement is offered because the helper is defined outside this hunk.
Closes #6247.
An "approve for the session" decision on
apply_patchwas not scoped to the file the user saw on the card. For a large class of supported inputs it was scoped to nothing at all.The mechanism
build_approval_grouping_keyis the scope of a session grant — two patches share the grant exactly when they share the key. Forapply_patchthat key came from a second, weaker parser than the one the executor uses:It never read the tool's own top-level
pathargument, accepted only the literal+++ b/prefix, and swallowed the normalizer's error — then collapsed to a shared constant.Four supported shapes produced
no_files:apply_patch{path, patch}override. A bare hunk has no+++line, andapply_patch.rsitself tells the model "Ensure the patch includes ---/+++ headers or providepath".File{action: "patch", path, patch}.--no-prefixdiff — the executor'snormalize_diff_pathaccepts+++ src/auth.rswhile the fingerprint demandedb/.What that meant
Approving a card reading "patch .env.example" also approved every later path-less patch: to
.env, to.codewhale/settings.json(hook and MCP configuration — what runs on the machine), to any absolute path outside the workspace, and in a non-git workspace to every file in the tree, since the #5185 carve-out is disabled there. No second card, no notice, no receipt.The gate is a bare set lookup, and its own comment names this exact class:
That escalation was closed for shell. It was open for patches — through a constant instead of a tool name.
The fix is reuse, not new parsing
Hash
preflight_apply_patch(input).touched_files— alreadypub, and already whatcore/engine.rs:7441andauto_review.rscall for the permission path. It folds thepathoverride, prefix-less headers and tab timestamps, so the fingerprint and the executor can no longer disagree about what a patch targets.Both empty arms now fail closed to a digest of the input rather than a shared constant, so an unparseable patch is its own family and matches nothing but a byte-identical repeat.
Net effect on the file: a second parser deleted, one import removed.
Evidence
Each new test was confirmed failing without the fix, by reverting the resolver body to the old implementation while keeping the tests:
The existing grouping tests only ever exercised the
replace/changesarray shape, which is why this went uncaught.Two things a reviewer should not have to rediscover
--test-threads=2this set reportedtask_manager::tests::pending_approval_suspends_idle_and_timeout_denial_settles_failedas failing. It passes isolated and at--test-threads=1. Same load-sensitive timeout class as thecompatibility_streamtests, not this change.check-blocking-calls-budget.pystill fails onterminal_input.rs. That is main's existing red, fixed in Budget the editor-handoff pause sleep, and note why main looked green without it #6243, not introduced here.🤖 Generated with Claude Code