timothyandClaude Opus 5
183df2bb3d
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>
30 lines
1.4 KiB
Python
30 lines
1.4 KiB
Python
"""Keep the test suite out of the PRODUCTION hook-fire log (ersatztv#776).
|
|
|
|
Four suites here drive real hooks as subprocesses — `test_merge_consent_exemption.py`,
|
|
`test_merge_consent_base_change.py`, `test_prepush_rebase_check_tag_exemption.py`,
|
|
`test_pr_changed_files.py` — and they predate the instrumentation. Once every hook records its own
|
|
execution, running `pytest scripts/tests` wrote its synthetic invocations into
|
|
`$HOME/.cache/ersatztv/hook-fire/`: 96 `pretooluse-merge-consent` fires including two `deny`s, and
|
|
12 `blocked` decisions from `prepush-rebase-check`, none of them a real session.
|
|
|
|
That is not untidiness, it is the defect the whole change exists to remove. `scripts/hook-fire-log.sh
|
|
report` is meant to answer "what did the harness actually do", and a log carrying test artifacts
|
|
answers a different question while looking identical — inference reintroduced one layer up. The
|
|
record has no field distinguishing a test invocation from a real one, and adding one would only move
|
|
the problem, so the fix is that tests never write to the real log at all.
|
|
|
|
Autouse and session-independent: a new suite that drives a hook is isolated by existing, without
|
|
having to know this file is here.
|
|
"""
|
|
|
|
from __future__ import annotations
|
|
|
|
import pytest
|
|
|
|
|
|
@pytest.fixture(autouse=True)
|
|
def isolate_hook_fire_log(tmp_path_factory, monkeypatch):
|
|
monkeypatch.setenv(
|
|
"ETV_HOOK_FIRE_LOG_DIR", str(tmp_path_factory.mktemp("hook-fire-log"))
|
|
)
|