[kyle-sexton]

Closes #6709

Summary

check-bash-file-changes exited quietly whenever it left a change unexamined, so nobody reading its output could tell "checked and clean" from "not checked". It now reports every skip or truncation it can detect as "N changed files not examined: ".

Fix

• check() returns {findings, notes}. A note is added for:
  • files past the first 20 in path order (the issue's a01..a20 plus zz.sh repro)
  • files past the 40 s time budget
  • a git status that failed or listed more than 10000 paths, at the check or at the snapshot
  • a new symbolic link, which is still never followed
  • no snapshot recorded before the command, when the cwd is inside a repository
  • no bash available to run the guards
  • a run_in_background command, whose later writes are not checked
• When the snapshot's git status fails, it now writes {root, failed: true} so the check can say why it examined nothing.
• stamp() records symbolic links ([size, mtime, isLink] from lstat). A pre-existing link that the command did not touch stays quiet.
• Output: notes alone go out as hookSpecificOutput.additionalContext and never block. Next to a finding, they are appended to the existing block reason. A run with no findings and no notes prints nothing, as before.
• Not detectable, so not reported: a file the command wrote and then deleted. Binary and oversized (over 1 MiB) files are still skipped silently, as before. The header comment and README row now list these as residuals.
• Changelog fragment .changes/guardrails/ (patch).

Verification

• check-bash-file-changes.test.sh gained a MUST REPORT section (R1-R5). Against the unmodified hook it had 8 failures, all in the new cases, for example FAIL: file cap: count and reason: '1 changed file not examined: ...' not in output: with empty output. After the fix: PASS=48 FAIL=0.
• Old cases 9b (new symlink) and 11 (no snapshot) asserted silence. They moved to R4 and R5 as must-report, and R4 still asserts that the link's target is never quoted.
• New stay-quiet cases: exactly 20 changed files (at the cap), and a pre-existing symlink plus a deleted tracked file. Existing stay-quiet cases 5e-13 are unchanged and pass.
• Every guardrails suite passes (scripts/affected-tests.sh --run over plugins/guardrails/**/*.test.sh).
• shellcheck, shfmt, markdownlint-cli2 and typos are clean on the changed files. check-changelog-fragments.sh --check passes.

Related

• Refs feat(guardrails): check repository files a shell command changed #6690 (originating issue), fix(guardrails): block-hook-bypass is evaded by a script-file interpreter; gate the outcome, not the command shape #6674 (the hook's introduction).
• fix(guardrails): narrow block-hook-bypass to a single plain shell write #6708 (held draft, same plugin): independent of this change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EFqTQRnCGywRxhv4nw4o4V

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-10-10T15:33:19.466150Z	b68fc8d	Draft marked ready

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[claude]

Claude finished @kyle-sexton's task in 6m 44s —— View job

---

Review in progress

• Gather context (PR diff, changed files)
• Understand request (invoke /review:code-review CI lane)
• Review check-bash-file-changes.mjs changes (traced check()/snapshot()/report() logic against each new test case)
• Review check-bash-file-changes.test.sh changes
• Review README/changelog fragment for consistency with the code
• Adversarial validation of candidate finding (in progress, independent verifier agent)
• Post findings / summary

[claude]

Claude finished @kyle-sexton's task in 2m 15s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Read changed files for security-relevant surface
• Check instruction-surface deletions against protected classes
• Hunt for logic/authz/injection/secret-handling flaws
• Adversarially verify any candidate findings
• Post final review

No security issues found.

Reviewed the full diff (origin/main...HEAD) of plugins/guardrails/hooks/check-bash-file-changes.mjs, its test file, the README row, and the changelog fragment. This change only adds reporting of skip/truncation conditions (file cap, time budget, oversized git status, new symlinks, missing snapshot, no bash, run_in_background); it does not alter which writes are examined or which findings block.

Checked specifically for regressions a reviewer would expect here:

• Symlink handling still never follows the link. stamp() now records symlinks via lstat (previously it returned null for them), but check() filters them out (files = all.filter((f) => !f.link)) before any file is passed to readContent/addedLines, so a symlink's target content is still never read or quoted back. Test R4 explicitly asserts the outside-file content (jdoe) is absent from the notes.
• No new bypass from the before.failed/no-snapshot/git status failure paths. Each of these still results in notes.push(...) followed by return result with an empty findings array — functionally equivalent to the old silent return [], just with an explanation attached. No path lets a change skip examination and also incorrectly report a clean result as blocking-affecting.
• run_in_background note is additive, not a shortcut. Setting tool_input.run_in_background: true only adds an informational note; it does not skip or short-circuit the rest of check(), so it can't be used to suppress a real finding.
• Blocking semantics preserved for real findings. report(): the new findings.length === 0 branch only changes behavior for the previously-impossible "notes but no findings" case; whenever findings.length > 0 on a normal PostToolUse, it still returns decision: "block" exactly as before.
• No information disclosure in the new notes. notExamined() messages are built from a fixed template plus integer counts only — no file paths or content are interpolated, so they can't leak repository contents through additionalContext.
• Instruction-surface lens: this diff strengthens an existing guardrail's observability (silent skip → reported skip) rather than deleting, narrowing, or softening any guardrail, block list, or allowlist, so no protected-class check applies here.

[github-actions]

claude-security-review has reviewed this pull request through 69a6231; a later push is reviewed from there.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b68fc8d3b6

ℹ️ 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".

[github-actions]

claude-review has reviewed this pull request through 69a6231; a later push is reviewed from there.

[claude]

Claude finished @kyle-sexton's task in 1m 6s —— View job

---

Security review complete

• Gather context (incremental diff since b68fc8d3b6, read via .claude-lane/incremental.diff)
• Read the 4 changed files for security-relevant surface
• Check instruction-surface deletions against protected classes (none apply — this is an additive fix)
• Hunt for logic/authz/injection/secret-handling flaws
• Adversarially verify the candidate change
• Post final review

No security issues found in the changes made since commit b68fc8d3b6 (already reviewed). The prior review stands for everything else in this PR.

This increment fixes exactly the gap Codex flagged and @kyle-sexton confirmed as valid: commits past the last 20 a shell command makes were silently dropped before committedFiles() ever saw them, so a flagged file touched only by an early commit in a 21+-commit run went unexamined with no note.

Reviewed check():

• The fix only adds a notes.push(...) branch when commits.length > MAX_FILES; the commits array that feeds committedFiles() is computed identically to before (commitsMade(log, before.log) when the log grew), and committedFiles() still internally slices to commits.slice(-MAX_FILES) as it always did. So this is purely an added disclosure of a pre-existing truncation, not a change to which commits get examined — no new bypass, and no narrowing of what was already checked.
• The note text interpolates only two integers (commits.length, dropped), both derived from Array.length arithmetic — no file path, commit SHA, or file content reaches the message, so there's no injection or information-disclosure surface in the new additionalContext/block-reason string.
• check-bash-file-changes.test.sh's new R1c case and the README/changelog wording update are test/doc only, no behavior change there.

No instruction-surface deletions, no authz/trust-boundary changes, no secret handling touched in this increment.

[claude]

Claude finished @kyle-sexton's task in 3m 7s —— View job

---

Review complete

• Gather context (incremental diff, changed files)
• Understand request (invoke /review:code-review for the incremental scope)
• Review changes since b68fc8d3b6 in the 4 listed files
• Adversarial validation of any candidate findings
• Post findings / summary

No blocking findings.

This increment (b68fc8d3b6..69a6231) fixes exactly the truncation gap chatgpt-codex-connector flagged and kyle-sexton confirmed: when a command makes more than 20 commits, committedFiles() silently kept only the last 20 (commits.slice(-MAX_FILES)), so a file touched only by an earlier commit was never examined and never reported.

Traced the fix in check-bash-file-changes.mjs#L400-L411:

• commits is now computed before truncation, and a note is pushed when commits.length > MAX_FILES, with dropped = commits.length - MAX_FILES — this matches exactly what committedFiles()'s internal commits.slice(-MAX_FILES) discards (same constant, same direction: oldest commits dropped, "earlier" wording is accurate since commitsMade() returns reflog entries oldest-first).
• The note is purely additive additionalContext; it doesn't change which files get examined or alter blocking semantics (confirmed committedFiles(before.root, commits) still receives the unsliced array and slices internally exactly as before — functional detection is byte-for-byte unchanged, only the new note is added).
• New test R1c (check-bash-file-changes.test.sh:250-258) reproduces the exact repro from the Codex finding (flagged file in commit 1 of 21) and asserts the note text; grammar ("commit is" vs "commits are") is correct for the dropped === 1 case it exercises.
• README and changelog fragment wording updated consistently with the new behavior.

One non-blocking observation, not worth a fix on its own: unlike the file-cap path, which has a stay-quiet boundary test at exactly 20 changed files (check-bash-file-changes.test.sh:298-302), there's no symmetric stay-quiet case asserting silence at exactly 20 commits. The > MAX_FILES comparison is correct by inspection (20 is not > 20), so this is just a coverage gap, not a suspected bug.

I was not able to execute the test suite or node directly in this lane (no bash-execution permission for the hook's test script), so this review is based on static tracing of the diff against commitsMade()/committedFiles()/changedFiles() and the existing test harness conventions, plus the PR description's reported PASS=48 FAIL=0 run.