Files
ersatztv/scripts/tests/test_prepush_unsets_git_env.py
T
timothyandClaude Opus 5 b2b651779a fix(785): close the re-review's BLOCKER — the leak guard pinned the FIX, not the property
Third cold review, this one scoped to the previous two fix commits. Its central finding is
the one worth reading, because the guard it faults is the guard I added to stop exactly
this class of thing.

BLOCKER — `test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir` asserted on
`_env()`'s RETURN VALUE. `_env()` can be perfectly correct while a call site passes
something else, and that is the reintroduction a future tidy-up actually produces.
Demonstrated: leave `_env()` exactly as shipped, restore a module-level snapshot and point
one `env=` at it — 13 tests pass and 58 records leak. I reproduced it before acting.

The guard was pinned to the SHAPE OF THE FIX rather than to the property, which is
`verify-against-the-REAL-predecessor`: it was witnessed red against a hand-written revert,
so "pins it" and #809's "witnessed red by reintroducing the snapshot" were both overclaims.

`test_driving_a_hook_LANDS_its_records_in_the_ISOLATED_dir` asserts the EFFECT instead —
drive a real hook through the real `_run()`, records must land in the fixture's dir — with
an anti-vacuity check that the hook reached a decision first. Measured: green at head, RED
under the call-site reintroduction that the old guard passed.

MEDIUM x3 — the workflow extractor was a text scanner and review broke it three ways in
one pass. A `run: |` block scalar was invisible, so reformatting the step to the form its
own sibling in that job already uses produced a FALSE RED whose message asserts a CI
regression that has not happened. A job or step switched off with `if: false` or
`continue-on-error: true` still read as wired — 7 green with the guard not gating at all.
And `run:` inside block-scalar TEXT was extracted and executed, so an `echo we no longer
run: PYTHONPATH=. python3 ... --check` left the subprocess proof green while the real step
was deleted. Two of the three are false greens.

`fixing-a-parser-bug-introduces-the-next-one` says the second round of a string predicate
is not the last. The structure is available — pyyaml is installed by the `script-tests` job
and four sibling suites already parse this workflow with it — so the scanner is replaced by
`yaml.safe_load`, and "would actually execute" is now three explicit conditions: the job
exists, the job is not disabled, the step is neither disabled nor allowed to fail. The six
ways it can stop gating are a parametrised test, and the block-scalar REFORMAT is a
negative control, because a detector that cannot tell a reformat from a removal trains its
readers to ignore it.

LOW — `_hook_calls` skipped whole-line comments but not inline ones, and broadening the
marker in the previous commit is what made that reachable: a trailing
`# ... .claude/hooks/removed-helper.sh` reddened a correct `pre-push`. The fix for one
over-match introduced another. Verified both directions.

NIT — a section header still said "three clauses" after the fourth mutation was added.

Docs. Two accuracy fixes, both verified against the files rather than reasoned about:
`test_hook_fire_log.py` proves THREE clauses (coverage, transparency, placement), not two,
and its row's Blocks column already claims two of them — the single `Proof ref` is a column
limit, not a coverage statement. And the "1,488 stray records" figure was unreproducible in
a durable doc, the same commit having deleted the evidence. Replaced with the reproduction
and a figure that survives it: 58 records per run, measured identically on macOS/git 2.55
and Linux/git 2.47.3.

Also corrects a count in 88832047b's own message: it says the inversion reddens "8 of 12";
that file had 13 tests at that commit. Third mislabelled figure in this branch, all three
in messages about mislabelled figures.

Refs: #785
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-08-21 21:29:00 +02:00

331 lines
16 KiB
Python

"""`.husky/pre-push` line 11 — `unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE` — is load-bearing.
That one line is the fix for a real, silent defect, and ersatztv#785 ranks it third because nothing
pinned it: reordering it after the nested git calls, or dropping it in a tidy-up, reintroduces the
bug with no symptom at all. The failure is a check reporting SUCCESS, which is the family
`testing.guard-ships-with-mutation-proof` exists for.
**The mechanism, measured rather than asserted.** Measured on both platforms this suite runs on,
because the guard it replaces was fail-open for months on exactly the platform nobody measured:
macOS/git 2.55 (development) and Linux/git 2.47.3 (the `script-tests` runner host). Identical on
both — `GIT_DIR` exported, `show-toplevel` answering `<wt>/web`, the nested diff reporting exit 0.
The whole file was run green there, and the deletion mutation was witnessed red there:
* Git exports `GIT_DIR` to `pre-push` **when the push comes from a worktree** — e.g.
`GIT_DIR=/repo/.git/worktrees/wt`. From the main tree it exports nothing, which is why this
never bites in a plain checkout and why it bites here constantly: `process.shared-tree-readonly`
makes working in a worktree the mandated path, so the exported-`GIT_DIR` case is the NORMAL one.
* With `GIT_DIR` set and `GIT_WORK_TREE` unset, git stops discovering the repo and takes the
**current directory** as the work tree. `git rev-parse --show-toplevel` from `web/` answers
`/repo/wt/web`.
* So `pre-push`'s last line — `cd web && npm run check:api`, whose `check:api` ends in
`git diff --exit-code` — compares against index paths that do not exist under that root. It
reports **no diff and exits 0**. Generated-API drift ships, and the gate that exists to catch it
prints success.
The test drives the REAL `.husky/pre-push` file, unedited, in a real worktree, with the environment
git really exports. What is substituted is only what surrounds it: the three `.claude/hooks` calls
are stubs (they are separately guarded and are not the subject), and `npm` is a stub on PATH whose
`run check:api` performs the nested `git diff --exit-code` that the real one ends in. The subject —
the ordering of the `unset` against the nested git call — is untouched.
"""
from __future__ import annotations
import os
import shutil
import stat
import subprocess
from pathlib import Path
REPO_ROOT = Path(__file__).resolve().parents[2]
PRE_PUSH = REPO_ROOT / ".husky" / "pre-push"
UNSET_CLAUSE = "unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE"
# The hooks `pre-push` calls before the CI-parity block. Stubbed because each is guarded on its own
# terms and none of them is what this file is about — but DERIVED from the real file rather than
# hand-listed, so a hook added to `pre-push` tomorrow cannot leave this harness silently running a
# `pre-push` that dies at a missing script and calling that a red.
def _hook_calls(text: str) -> list[str]:
"""Every `.claude/hooks/` script `pre-push` actually invokes.
Two refinements over a plain substring scan, each closing a way the harness would misreport:
* **comment lines are skipped.** A comment naming a hook that no longer exists would otherwise
redden `test_the_stub_hooks_are_derived_from_the_real_file` for a file that is perfectly
correct.
* **the prefix is not assumed to be `./`.** A hook invoked as `bash .claude/hooks/x.sh` would
be missed, left unstubbed, and exit 127 — a NON-ZERO status that two tests here read as
"drift was caught". That is a false green in the direction that matters, so the match is on
the path segment rather than on `./`.
"""
names = []
for line in text.splitlines():
# INLINE comments too, not just whole-line ones. Broadening the marker from `./.claude/hooks/`
# to the path segment made a trailing `# ... .claude/hooks/removed-helper.sh` match, which
# would redden this file for a `pre-push` that is perfectly correct — a false red the
# narrower marker did not have. The fix for one over-match must not introduce another.
stripped = line.split("#", 1)[0].strip()
marker = ".claude/hooks/"
if marker in stripped:
rest = stripped.split(marker, 1)[1]
names.append(rest.split()[0].rstrip("|&;\"'"))
return names
def _git(cwd: Path, *args: str) -> str:
return subprocess.run(
["git", *args],
cwd=str(cwd),
check=True,
capture_output=True,
text=True,
env={
**os.environ,
"GIT_AUTHOR_NAME": "t",
"GIT_AUTHOR_EMAIL": "t@e",
"GIT_COMMITTER_NAME": "t",
"GIT_COMMITTER_EMAIL": "t@e",
},
).stdout.strip()
def _exe(path: Path, body: str) -> None:
path.write_text(body)
path.chmod(path.stat().st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
class Bench:
"""A real repo, a real worktree, the real `pre-push`, and the env git really exports."""
def __init__(self, tmp_path: Path, pre_push_text: str, *, drift: bool):
tmp_path.mkdir(parents=True, exist_ok=True)
self.root = tmp_path / "main-tree"
self.root.mkdir()
_git(self.root, "init", "-q", "-b", "main", ".")
(self.root / "seed").write_text("seed\n")
_git(self.root, "add", "seed")
_git(self.root, "commit", "-qm", "init")
self.wt = tmp_path / "wt"
_git(self.root, "worktree", "add", "-q", str(self.wt), "-b", "feature")
web = self.wt / "web"
web.mkdir()
(web / "gen.txt").write_text("generated\n")
_git(self.wt, "add", "web/gen.txt")
_git(self.wt, "commit", "-qm", "add generated file")
if drift:
# The condition `check:api` exists to catch: the committed generated artifact no longer
# matches what regeneration produces.
(web / "gen.txt").write_text("generated\nDRIFT\n")
(self.wt / ".husky").mkdir()
self.pre_push = self.wt / ".husky" / "pre-push"
_exe(self.pre_push, pre_push_text)
hooks = self.wt / ".claude" / "hooks"
hooks.mkdir(parents=True)
for name in _hook_calls(pre_push_text):
_exe(hooks / name, "#!/usr/bin/env bash\ncat >/dev/null\nexit 0\n")
# The npm stand-in. `run check:api` performs the nested `git diff --exit-code` the real
# script ends in; everything else is a no-op. It records the nested diff's OWN exit code,
# not merely that it ran. That distinction is load-bearing: one of the mutations below moves
# the `unset` to the end of the file, which also makes the script's terminal status 0 (a
# bare `unset` succeeds) — so asserting on pre-push's exit code alone would be satisfied for
# a reason that has nothing to do with the nested git call. The recorded diff verdict IS the
# defect; the script's exit code is downstream of it.
self.witness = tmp_path / "check-api-ran"
bindir = tmp_path / "bin"
bindir.mkdir()
_exe(
bindir / "npm",
"#!/usr/bin/env bash\n"
'if [ "$1" = "run" ] && [ "$2" = "check:api" ]; then\n'
" git diff --exit-code -- gen.txt >/dev/null\n"
" _rc=$?\n"
f' echo "$PWD $_rc" >> "{self.witness}"\n'
" exit $_rc\n"
"fi\n"
"exit 0\n",
)
self.bindir = bindir
# `GIT_DIR` exactly as git exports it for a push from this worktree, verified against a real
# push in the investigation that produced this file.
self.git_dir = _git(self.wt, "rev-parse", "--absolute-git-dir")
def run(self, *, export_git_dir: bool) -> subprocess.CompletedProcess:
env = {k: v for k, v in os.environ.items() if k not in ("GIT_DIR", "GIT_WORK_TREE", "GIT_INDEX_FILE")}
env["PATH"] = f"{self.bindir}:{env['PATH']}"
if export_git_dir:
env["GIT_DIR"] = self.git_dir
return subprocess.run(
["bash", str(self.pre_push), "origin", "file:///dev/null"],
input=b"refs/heads/feature abc refs/heads/feature def\n",
capture_output=True,
cwd=str(self.wt),
env=env,
timeout=120,
)
def check_api_ran(self) -> bool:
return self.witness.exists()
def nested_diff_rc(self) -> int:
"""The exit code the nested `git diff --exit-code` actually reported.
0 means it saw NO diff. With a drifted tree that answer is the bug.
"""
assert self.witness.exists(), "check:api never ran, so there is no nested diff verdict"
return int(self.witness.read_text().split()[-1])
# ------------------------------------------------------------------------------------------------
# ANTI-VACUITY — the clause exists, and the harness reaches the check that depends on it
# ------------------------------------------------------------------------------------------------
def test_the_unset_clause_is_still_in_pre_push():
text = PRE_PUSH.read_text()
assert UNSET_CLAUSE in text, (
f"`{UNSET_CLAUSE}` is gone from .husky/pre-push. If it was removed deliberately, this file "
"must be removed with it and docs/guard-inventory.md updated — do not delete this assertion "
"on its own, it is the only thing pinning the ordering"
)
lines = text.splitlines()
unset_at = next(i for i, ln in enumerate(lines) if UNSET_CLAUSE in ln)
nested_at = next(i for i, ln in enumerate(lines) if "npm run check:api" in ln)
assert unset_at < nested_at, (
"the unset now comes AFTER the nested git call it exists to protect — that ordering is the "
"regression, and it is silent"
)
def test_the_harness_actually_reaches_check_api(tmp_path):
bench = Bench(tmp_path, PRE_PUSH.read_text(), drift=True)
bench.run(export_git_dir=False)
assert bench.check_api_ran(), (
"the CI-parity block never ran, so every exit code below would be reporting on the hooks "
"before it rather than on the nested git call this file is about"
)
# ------------------------------------------------------------------------------------------------
# THE GUARD DECIDES
# ------------------------------------------------------------------------------------------------
def test_drift_is_CAUGHT_with_no_git_env_exported(tmp_path):
"""Positive control: the harness detects real drift when nothing is in the way."""
bench = Bench(tmp_path, PRE_PUSH.read_text(), drift=True)
p = bench.run(export_git_dir=False)
assert bench.nested_diff_rc() != 0, "the nested diff saw no drift even with a clean env"
assert p.returncode != 0, f"the harness did not detect drift even with a clean env: {p.stderr!r}"
def test_drift_is_CAUGHT_when_git_exports_GIT_DIR_from_a_worktree(tmp_path):
"""THE REAL CASE. Every push from a worktree — the mandated way to work here — runs this."""
bench = Bench(tmp_path, PRE_PUSH.read_text(), drift=True)
p = bench.run(export_git_dir=True)
assert bench.nested_diff_rc() != 0, (
"the nested `git diff --exit-code` reported NO DIFF on a drifted file. That is the silent "
"fail-open the `unset` exists to prevent, and it is invisible from the exit code alone"
)
assert p.returncode != 0, (
"pre-push reported success on a drifted generated file while git had exported GIT_DIR: "
f"{p.stdout!r} {p.stderr!r}"
)
def test_a_CLEAN_tree_is_allowed_through(tmp_path):
"""Negative control. A harness that always failed would pass both assertions above."""
bench = Bench(tmp_path, PRE_PUSH.read_text(), drift=False)
p = bench.run(export_git_dir=True)
assert p.returncode == 0, f"a clean tree was blocked: {p.stdout!r} {p.stderr!r}"
assert bench.nested_diff_rc() == 0, "the nested diff invented a diff on a clean tree"
# ------------------------------------------------------------------------------------------------
# MUTATION PROOFS — the two ways the clause stops protecting anything
# ------------------------------------------------------------------------------------------------
def _positive_control(tmp_path: Path, label: str) -> None:
bench = Bench(tmp_path / f"pc-{label}", PRE_PUSH.read_text(), drift=True)
p = bench.run(export_git_dir=True)
assert bench.nested_diff_rc() != 0 and p.returncode != 0, (
"the UNMUTATED pre-push did not catch the drift, so 'the mutant lets it through' proves "
f"nothing about the clause: {p.stdout!r} {p.stderr!r}"
)
def test_MUTATION_DELETING_the_unset_lets_drift_through_silently(tmp_path):
_positive_control(tmp_path, "delete")
text = PRE_PUSH.read_text()
assert UNSET_CLAUSE in text, "retarget this mutation; the clause has moved"
mutated = text.replace(UNSET_CLAUSE, "# clause removed by the mutation proof", 1)
bench = Bench(tmp_path / "mut", mutated, drift=True)
p = bench.run(export_git_dir=True)
assert bench.check_api_ran(), "the mutant died before check:api, so its exit code says nothing"
assert bench.nested_diff_rc() == 0, (
"removing the unset did NOT blind the nested diff, so the clause is not what protects it "
"and this whole file is pinning the wrong thing"
)
assert p.returncode == 0, f"the drift was still caught somehow: {p.stdout!r}"
def test_MUTATION_REORDERING_the_unset_after_the_nested_git_call_lets_drift_through(tmp_path):
"""The regression ersatztv#785 names by hand: not deletion, relocation.
Deleting a line is a conspicuous diff. Moving it — during a tidy-up, or when a new check is
appended above it — reads as a no-op and is not.
This asserts on the NESTED DIFF's verdict, not on pre-push's exit code. Relocating the `unset`
to the end of the file also makes it the script's last statement, and a bare `unset` succeeds —
so `returncode == 0` would hold here even if the nested git call had worked perfectly. That is a
test passing for the wrong reason, and it was written that way in this file's first draft.
"""
_positive_control(tmp_path, "reorder")
lines = PRE_PUSH.read_text().splitlines()
kept = [ln for ln in lines if UNSET_CLAUSE not in ln]
assert len(kept) == len(lines) - 1, "expected exactly one unset line to relocate"
mutated = "\n".join(kept + [UNSET_CLAUSE, ""])
bench = Bench(tmp_path / "mut", mutated, drift=True)
bench.run(export_git_dir=True)
assert bench.check_api_ran(), "the mutant died before check:api, so it reports on nothing"
assert bench.nested_diff_rc() == 0, (
"moving the unset below the nested git call did NOT blind it, so the ORDERING is not "
"load-bearing and the ordering assertion in the anti-vacuity test is decoration"
)
def test_the_stub_hooks_are_derived_from_the_real_file():
"""If `pre-push` gains a hook call, the harness must stub it rather than die at a missing file.
A `pre-push` that exits 127 at a missing script produces a non-zero exit — indistinguishable
from 'drift was caught' in two of the tests above.
"""
calls = _hook_calls(PRE_PUSH.read_text())
assert calls, "no ./.claude/hooks/ call found in pre-push; the extractor has stopped matching"
for name in calls:
assert (REPO_ROOT / ".claude" / "hooks" / name).is_file(), (
f"pre-push calls {name}, which does not exist in .claude/hooks/"
)
def test_shutil_which_npm_is_not_what_the_harness_used(tmp_path):
"""Anti-vacuity for the stand-in: a real `npm` on PATH would run the real scripts and pass."""
bench = Bench(tmp_path, PRE_PUSH.read_text(), drift=True)
resolved = shutil.which("npm", path=f"{bench.bindir}:{os.environ['PATH']}")
assert resolved == str(bench.bindir / "npm"), (
f"the harness would have used {resolved}, not its stand-in — the check it performs would "
"then be whatever the real package.json says, not the nested git call under test"
)