Repository navigation
fix(shell): judge only added lines in the changed-script allow list check - #1071
tbhb-agent wants to merge 3 commits into
Conversation
…heck The changed-script check parsed every touched shell file and reported every unlisted command in it. Editing a long script that already ran awk or python3 failed on lines the change never touched, so no one could edit packages/agents/source/lib/lib.sh at all. The only other option was accepting the old findings in a baseline, which repository policy bans. With a base ref, the command now reads the added line ranges from a zero-context diff against the merge base and reports a finding only when its line is one of them. A new script with no history still counts in full, and the explicit path mode and the summary still scan whole files. A banned command on an added or edited line fails as before. Every line a change introduces gets the full check. Loosens the gate: findings on unchanged lines stop failing the changed mode. The summary command still counts them, which keeps the debt visible until the ports retire it. Assisted-by: claude:claude-sonnet-5-5 Signed-off-by: Tony Burns <tony@tonyburns.net>
tbhb-agent
added
change:fix
![tbhb-dev-reviews[bot]](https://avatars.githubusercontent.com/u/288164213?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed head: 2e4f4e2
Review: CHANGES REQUESTED
Loosens: yes — existing findings are excluded in changed mode; the bypasses below exceed merely ignoring unchanged commands.
Owner-tier: no changed path matches either pattern. No constitutional-path approval requirement is triggered; the gate relaxation still needs the owner's exact-head approval.
Changed paths (all M; owner-tier / constitutional):
- cmd/shell-allowlist/changed.go: no / no
- cmd/shell-allowlist/changed_test.go: no / no
- cmd/shell-allowlist/run.go: no / no
- cmd/shell-allowlist/summary.go: no / no
- docs/shell-allowlist.md: no / no
Policy patterns, quoted from .agents/agent-skills.conf:9 and .agents/agent-skills.conf:5:
constitutional_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/(rulesets/|CODEOWNERS$)|\.claude/settings\.json$|packages/agents/source/(lib|skills/merge-pr/(shared|claude|codex)/scripts)/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$|(packages/agents/[^/]+/\.apm/skills|\.(claude|agents)/skills)/[^/]+/scripts/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$)
owner_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/rulesets/|\.claude/settings\.json$|packages/agents/source/(lib/(lib|agent)\.sh$|skills/[^/]+/(shared|claude|codex)/scripts/guard[^/]*\.sh$|skills/merge-pr/(shared|claude|codex)/scripts/(merge-agent\.sh|merge-policy\.py)$)|(packages/agents/(claude|codex)/\.apm/skills|\.(claude|agents)/skills)/([^/]+/scripts/(guard[^/]*\.sh|(lib|agent)\.sh)$|(merge-pr|agent-merge-pr)/scripts/(merge-agent\.sh|merge-policy\.py)$))
AC assessment on this head:
| Issue / live AC | Met by this slice? | Evidence / allocation |
|---|---|---|
| #588 AC1 | No | cmd/shell-allowlist/changed.go:83 changes gate scope; complex shell replacement remains in later slices. |
| #588 AC2 | No, partial only | cmd/shell-allowlist/changed_test.go:78 tests filtering, not migration parity. Later slices. |
| #588 AC3 | No, not established | cmd/shell-allowlist/changed.go:83 is a prerequisite change; ordered migrations remain later work. |
| #588 AC4 | No, not established | packages/agents/source/lib/lib.sh:575 retains review authority logic; this slice supplies no migration proof. |
| #588 AC5 | No, not established | cmd/shell-allowlist/run.go:16 implements no protected-write/check-reader migration. Later slices own that proof. |
| #588 AC6 | No | cmd/shell-allowlist/summary.go:35 changes a call site, not canonical generation or deployment. Later slices. |
| #588 AC7 | No, not established | cmd/shell-allowlist/run.go:22 changes behavior; replacement-binary deployment remains unproved here. Later slices. |
| #588 AC8 | No, partial only | All checks passed; .agents/agent-skills.conf:4 still does not protect this Go enforcement code. Epic completion is later work. |
| #949 AC1 | No | packages/agents/source/lib/lib.sh:508 still discards stderr; :669 names no diagnostic file. Deferred to #949. |
| #949 AC2 | No | packages/agents/source/lib/lib.sh:628 still names no stderr file. Deferred to #949. |
| #949 AC3 | Yes, preserved | packages/agents/source/lib/lib.sh:652 and :659 retain readable-verdict output without a stderr filename. |
#949's latest Refinement verdict is READY; #588's comments contain status updates, without a Refinement verdict. Neither issue closes here. Related references are appropriate. Verification names tests but cites no AC IDs.
Findings:
- P1 — cmd/shell-allowlist/run.go:69: filtering only the command word's starting line misses a newly banned continued word. A two-line command with line 1 consisting of r plus a trailing backslash and line 2 containing m is allowed rm. Change only line 2 to uby: the command becomes banned ruby, but its finding still points to unchanged line 1 and is discarded. Judge edits intersecting the complete command word and add a regression.
- P1 — cmd/shell-allowlist/changed.go:97: diff output is not forced to raw text. A valid tracked script marked with a -diff attribute produces no hunks, so parseAdded returns an empty set and run.go:69 suppresses every finding, including an appended ruby command. Text conversion can likewise give unrelated line numbers. Force textual, non-text-converted input and refuse an unjudgeable changed script; add a regression.
- P2 — cmd/shell-allowlist/changed.go:83: neither referenced issue supplies an AC authorizing this gate-policy change (#588 excludes policy changes as a side effect; #949 concerns stderr diagnostics). Link a scoped issue/AC contract for the relaxation and cite its exact-head evidence in Verification.
Boundary audit: ordinary added/edited lines and zero-count deletion ranges are covered at cmd/shell-allowlist/changed_test.go:108, :114, :120 and :134. Untracked files scan in full (:126); a rename's destination-only path diff conservatively scans it as an addition. No baseline file is introduced. Moving/re-adding a command is judged only if Git emits that physical line as added; moving surrounding lines can leave an old banned command matched as context. Whitespace edits normally count as additions, subject to the command-word flaw above.
Suppression audit: no new loosening markers, no t.Skip, and no weakened existing assertions. The requested zero-nolint condition is unmet: existing directives remain at cmd/shell-allowlist/changed.go:71, cmd/shell-allowlist/changed_test.go:4 and cmd/shell-allowlist/run.go:55. No unnecessary guard, option, abstraction or layer was found.
SIZE: scc 4.1.0 over added and deleted fragments gives 136 code / 2 prose lines, below 800 / 2000; no vendor changes. D4: all five changed files passed the private-string scan. Coverage: COMPLETED SUCCESS. CodeQL: COMPLETED SUCCESS, including Go and Actions analysis. All 23 reported check contexts are COMPLETED SUCCESS. Mergeability: MERGEABLE, merge state BLOCKED, not draft.
Round: 1 of 3
The first version of the added-lines filter matched a finding by the line where its command word starts. A backslash can continue a word over two or more lines. An edit to a continuation line could turn an allowed command into a banned one. The finding still pointed at the unchanged first line, and the filter dropped it. Git also hides hunks for a file with a -diff attribute or a text conversion filter, and an empty hunk set made the filter drop every finding in that file. A finding now records the last line of its command word, and any added line inside that span counts. The check now diffs raw text, and a script that still diffs as binary fails the check instead of passing it unchecked. Assisted-by: claude:claude-sonnet-5-5 Signed-off-by: Tony Burns <tony@tonyburns.net>
There was a problem hiding this comment.
Reviewed head: 4ec38d0
Review: CHANGES REQUESTED
Loosens: yes — changed mode excludes unchanged command words, as #1073 permits. No new suppression markers.
Owner-tier: no changed path matches owner_paths or constitutional_paths. No constitutional-path approval is triggered. This gate relaxation still requires the owner's exact-head approval; none appears in the reviews.
Changed paths (git diff --name-status origin/main...HEAD; owner-tier / constitutional):
- M cmd/shell-allowlist/changed.go: no / no
- M cmd/shell-allowlist/changed_test.go: no / no
- M cmd/shell-allowlist/run.go: no / no
- M cmd/shell-allowlist/summary.go: no / no
- M docs/shell-allowlist.md: no / no
- M internal/shell/shell.go: no / no
- M internal/shell/shell_test.go: no / no
Policy patterns (.agents/agent-skills.conf:9 and :5):
constitutional_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/(rulesets/|CODEOWNERS$)|\.claude/settings\.json$|packages/agents/source/(lib|skills/merge-pr/(shared|claude|codex)/scripts)/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$|(packages/agents/[^/]+/\.apm/skills|\.(claude|agents)/skills)/[^/]+/scripts/(merge-policy\.py|merge-agent\.sh|signing\.sh|lib\.sh)$)
owner_paths = ^(\.agents/(agent-skills\.conf|review-instructions\.md)$|\.github/rulesets/|\.claude/settings\.json$|packages/agents/source/(lib/(lib|agent)\.sh$|skills/[^/]+/(shared|claude|codex)/scripts/guard[^/]*\.sh$|skills/merge-pr/(shared|claude|codex)/scripts/(merge-agent\.sh|merge-policy\.py)$)|(packages/agents/(claude|codex)/\.apm/skills|\.(claude|agents)/skills)/([^/]+/scripts/(guard[^/]*\.sh|(lib|agent)\.sh)$|(merge-pr|agent-merge-pr)/scripts/(merge-agent\.sh|merge-policy\.py)$))
| Live AC | Met on reviewed head? | Evidence / allocation |
|---|---|---|
| #1073 AC1 | Yes | cmd/shell-allowlist/changed.go:100 reads merge-base additions; cmd/shell-allowlist/run.go:69 filters findings; cmd/shell-allowlist/changed_test.go:105 covers unchanged, added and edited lines. |
| #1073 AC2 | Yes | cmd/shell-allowlist/changed.go:103 selects full-file judging for untracked files; cmd/shell-allowlist/changed_test.go:143 asserts it. |
| #1073 AC3 | Yes | internal/shell/shell.go:215 records the word's end; cmd/shell-allowlist/run.go:80 checks its entire line span; cmd/shell-allowlist/changed_test.go:129 is the continuation regression. |
| #1073 AC4 | Yes | cmd/shell-allowlist/changed.go:100 forces raw text without external diff or text conversion; cmd/shell-allowlist/run.go:61 fails on unparseable shell; cmd/shell-allowlist/changed_test.go:136 is the -diff regression. See the false-positive finding below. |
| #1073 AC5 | Yes | cmd/shell-allowlist/run.go:21 leaves explicit paths unfiltered; cmd/shell-allowlist/summary.go:35 passes nil; cmd/shell-allowlist/run_test.go:107 and cmd/shell-allowlist/summary_test.go:19 assert whole-file reports. |
| #1073 AC6 | Yes | docs/shell-allowlist.md:35 documents added-line scope and full judging of new scripts. |
| #588 AC1 | No | packages/agents/source/lib/lib.sh:504 retains complex shell; migrations belong to later slices. |
| #588 AC2 | No | cmd/shell-allowlist/changed_test.go:78 tests this prerequisite, not migration parity; later slices. |
| #588 AC3 | No | cmd/shell-allowlist/run.go:22 changes gate scope, without establishing the ordered migrations; later slices. |
| #588 AC4 | No | packages/agents/source/lib/lib.sh:652 retains authority/stamp behavior; migration proof belongs to later slices. |
| #588 AC5 | No | cmd/shell-allowlist/run.go:16 does not migrate protected writes or check readers; later slices. |
| #588 AC6 | No | cmd/shell-allowlist/summary.go:35 changes only a caller; generation/deployment migrations remain later work. |
| #588 AC7 | No | cmd/shell-allowlist/run.go:22 changes an existing binary; replacement-binary rollout remains later work. |
| #588 AC8 | No | Checks are green, but .agents/agent-skills.conf:4 does not protect this Go enforcement code; epic completion remains later work. |
| #949 AC1 | No | packages/agents/source/lib/lib.sh:508 discards stderr and :669 names no file; deferred to #949. |
| #949 AC2 | No | packages/agents/source/lib/lib.sh:628 names no stderr file; deferred to #949. |
| #949 AC3 | Yes, preserved | packages/agents/source/lib/lib.sh:652 and :659 retain readable-verdict output without a stderr filename. |
The description cites #1073 AC1–AC6 with verification evidence and closes only #1073. #588 and #949 are Related and remain open. #1073 has no comments or Refinement verdict; #949's latest Refinement verdict is READY; #588 has status comments without a Refinement verdict.
Round-1 resolution: both P1 bypasses are closed with regression assertions at cmd/shell-allowlist/changed_test.go:129 and :136. The scoped #1073 contract and per-AC description evidence resolve the previous P2.
Findings:
- P2 — cmd/shell-allowlist/changed.go:111: the binary guard searches the entire patch, including ordinary added/deleted source lines. Adding an allowed comment such as
# GIT binary patch, orprintf 'Binary files changed\n', makes a valid text script fail as binary. The same text in a deleted line also fails. This guard adds a false rejection beyond AC4. With --text already forcing textual output, remove the redundant guard, or recognize actual binary metadata outside hunk payloads. Add a regression for these valid text changes. - P2 — cmd/shell-allowlist/changed.go:72, cmd/shell-allowlist/changed_test.go:4, cmd/shell-allowlist/run.go:55: three pre-existing nolint directives remain in changed files, so the requested zero-nolint condition is still unmet. Resolve these sites to satisfy that condition; no new directive was introduced.
Boundary audit: zero-context new-side ranges include moved/re-added or whitespace-edited command lines when Git marks them added. A rename's destination-only diff conservatively judges the destination as a new file. Deleted files are excluded; zero-count deletion hunks add no judged lines (cmd/shell-allowlist/changed_test.go:123 and :153). Git may retain a relocated identical line as unchanged context, which AC1 intentionally excludes; this is not a new-command exemption. No baseline, allow-list entry or exemption file is added. No t.Skip or weakened assertion appears; EndLine assertions strengthen existing expectations. Apart from the binary guard finding, no unnecessary option, abstraction or layer was found.
SIZE: scc 4.1.0 on added plus deleted fragments against origin/main: 187 code / 2 prose lines, below 800 / 2000; 205 additions and 20 deletions physically; no vendor changes.
D4: all seven changed non-vendor files passed the private-string scan.
Checks: all 23 contexts COMPLETED SUCCESS. Coverage: COMPLETED SUCCESS. CodeQL: COMPLETED SUCCESS, including Go and Actions analysis.
Mergeability: UNKNOWN; merge state UNKNOWN; not draft (single status snapshot).
Round: 2 of 3
The changed-lines check searched the whole patch for the strings Git prints for binary files. An ordinary added or deleted line that holds the same text, such as a comment about a binary patch, made a valid script fail the check. The diff already runs with `--text`, so Git never emits binary markers for these paths. Remove the redundant search and its error value, and add a regression case for a script that contains both marker strings. Refs: #1073 Assisted-by: claude:claude-sonnet-5-5 Signed-off-by: Tony Burns <tony@tonyburns.net>
Summary
With
--changed BASE,shell-allowlistnow reports a finding only when its line is one a change adds relative to the merge base. A script with no history, such as an untracked file, counts in full. The explicit path mode and--summarystill scan whole files.Why
The changed-script gate parsed every touched shell file and reported every unlisted command in it. A long script that already ran
awkorpython3failed on any edit, even when the edit added only allowed commands.packages/agents/source/lib/lib.shhas many such findings. No change to it could pass, and the work that needs it (#949) stopped. Repository policy bans accepting the old findings in a baseline. Judging the lines a change adds keeps the gate strict on new code and stops it failing on code the change never touched.Verification
The commands below ran on the head of this pull request.
go test ./cmd/shell-allowlist -run TestChangedLinesOnly-diffattribute casego test ./cmd/shell-allowlist ./internal/shelldocs/shell-allowlist.mddescribes the scopemise run lintandmise run test-goThe
nolintdirectives incmd/shell-allowlist/changed.go,run.goandchanged_test.goalready exist onorigin/mainatchanged.go:69,run.go:48andchanged_test.go:4, and this change adds none.Doc impact:
docs/shell-allowlist.mddescribes the new--changedscope.Consumers receive the behavior with their next pinned
shell-allowlistbinary and don't change their configuration.Risk
Loosens: yes
The changed mode no longer fails on findings that sit on lines a change doesn't add. A line that moves or gets reformatted counts as added, and a banned command added anywhere in a changed script still fails.
--summarykeeps counting the old findings until the Go ports retire the scripts. A diff header the parser can't read, fails the check rather than shrinking the judged lines. An edit to any line of a backslash-continued command word counts as touching it. Reverting the change restores whole-file judging.Compatibility: none
Related
Closes #1073. Related to #588 and #949, which this change doesn't close.