Round 2, from two independent cold reviews. The headline defect was found by
the test the reviews prompted rather than by the reviews themselves:
exec 0<"$sin" 2>/dev/null
reads as "suppress errors from this redirection". It is not. `exec` with
redirections and no command applies them to the shell PERMANENTLY, so this sent
each hook's ENTIRE stderr to /dev/null. Every husky guard's user-facing output
is stderr — the H6 "push to main BLOCKED" message, the BOM guard's remediation
text, the missing-trailer message — so the guards went on blocking correctly
while telling the human nothing about why. Invisible to the original A/B, which
compared only stdout and exit status.
The finding that exposed it was ranked Low ("the A/B ignores stderr").
Also fixed, each with an executed proof:
- The fd restore sat inside the same conditional as the replay, and the replay
read the PATH, so an unlinked-but-open scratch file meant a swallowed `deny`
at exit 0. Restore is now unconditional and the replay reads through a held
fd, which survives unlink.
- A SIGTERMed hook lost output it had already printed (52 bytes -> 0). Bash runs
no EXIT trap for an untrapped fatal signal; INT/TERM/HUP are trapped now.
- The classifier misreported an uppercase decision as prose, read the bare word
additionalContext in free text as a decision, and let a non-zero exit ERASE a
decision the hook really printed.
- The session id reached the log FILENAME unscrubbed. The first fix reused the
record scrubber, which deliberately keeps `/` and `.`, so `../../escaped` went
through untouched: passing a value through a scrub is not the same as passing
it through the right scrub.
- Preamble line 1 could abort a `set -e` hook, turning a fail-open guard
fail-closed; and a missing sink printed `command not found` to stderr.
The big one is test quality. `test_instrumentation_changes_NOTHING_the_harness_
can_see` was 160/165 empty-vs-empty and 165/165 exit-0-vs-exit-0, so deleting
the whole replay left it green for four hooks, two of which issue `deny`. Three
places claimed the matrix reached every deciding branch, including the decision
record. It now constructs the missing cases — a foreign worktree marker, a BOM
`.cs` in an *ersatztv*-named repo, stubbed memory_pressure, a real bare remote
for the three pre-push guards, a stub Gitea for prepush-donewhen — and
test_the_AB_is_not_VACUOUS_for_any_hook fails unless each hook has a case where
the control side actually says something.
Running pytest also wrote 115 synthetic fires into the production log, so the
report described the test suite while looking like a record of real sessions.
scripts/tests/conftest.py isolates every test; verified live, a full suite run
now leaves the real log untouched.
Refs: #776
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
timothyandClaude Opus 5
183df2bb3d