From 629ab0b17385720b688fd73ffff9ca570af861aa Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:44:44 +0200 Subject: [PATCH 01/12] test(785): prove the worktree-ownership PAIR, including the cross-file seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pretooluse-worktree-guard.sh` and `posttooluse-worktree-marker.sh` are one mechanism in two files and neither had a test. #785 ranks them second because a regression in either half is invisible: both hooks are deliberately fail-open, so a marker hook that writes nothing and a guard that reads nothing both produce "the commit is allowed" — identical to a correct fail-open. Three clauses are disarmed and each was witnessed red against the REAL hooks in place, not only against the isolated copies the tests build: * the guard's marker read -> 5 failed, 5 passed * the guard's ownership comparison -> 2 failed, 8 passed (inverted, because disarming it the other way only makes the guard deny MORE and every deny assertion stays green) * the marker hook's write -> 6 failed, 4 passed The third is the one that could not exist while the halves were tested apart: the clause is in the marker hook, the assertion is about the guard's decision. The first attempt at that third demonstration used a `sed` that silently matched nothing and reported 10 passed. Re-run with an asserted replacement, hence the numbers above. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 9 +- .../tests/test_worktree_ownership_guard.py | 346 ++++++++++++++++++ 2 files changed, 351 insertions(+), 4 deletions(-) create mode 100644 scripts/tests/test_worktree_ownership_guard.py diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 02cdba063..2fc169293 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -75,7 +75,7 @@ all. | --- | --- | --- | --- | --- | | `.claude/hooks/decisions-guard.sh` | a commit | GUARD | NONE | — | | `.claude/hooks/design-sync-reminder.sh` | the first Stop after a UI change (one-shot, then allows) | GUARD | NONE | — | -| `.claude/hooks/posttooluse-worktree-marker.sh` | nothing (writes the marker the worktree guard reads) | GUARD | NONE | — | +| `.claude/hooks/posttooluse-worktree-marker.sh` | nothing (writes the marker the worktree guard reads) | GUARD | MUTATION | `test_worktree_ownership_guard.py::test_MUTATION_a_marker_hook_that_stops_WRITING_makes_the_guard_go_quiet` | | `.claude/hooks/prepush-clean-worktree-check.sh` | a push with uncommitted changes in the pushed set | GUARD | NONE | — | | `.claude/hooks/prepush-donewhen.sh` | a direct push to `main` with unticked Done-when boxes | GUARD | NONE | — | | `.claude/hooks/prepush-rebase-check.sh` | a push from a branch behind `origin/main` | GUARD | BEHAVIOUR-ONLY | `test_prepush_rebase_check_tag_exemption.py::test_zero_ref_lines_does_not_exempt` | @@ -85,7 +85,7 @@ all. | `.claude/hooks/pretooluse-bom-guard.sh` | a commit/push carrying a BOM in a touched `.cs` | GUARD | MUTATION | `test_bom_guard_detection.py::test_DISARMING_the_BOM_comparison_stops_detection` | | `.claude/hooks/pretooluse-merge-consent.sh` | a PR merge without derived consent | GUARD | BEHAVIOUR-ONLY | `test_merge_consent_exemption.py::test_protected_path_on_a_LATER_page_is_still_seen` | | `.claude/hooks/pretooluse-nav-guard.sh` | a browser navigate to a streaming URL | GUARD | NONE | — | -| `.claude/hooks/pretooluse-worktree-guard.sh` | a commit/merge in a foreign worktree | GUARD | NONE | — | +| `.claude/hooks/pretooluse-worktree-guard.sh` | a commit/merge in a foreign worktree | GUARD | MUTATION | `test_worktree_ownership_guard.py::test_MUTATION_disarming_the_guards_MARKER_READ_stops_the_deny` | | `.husky/commit-msg` | a commit with no `Co-Authored-By` trailer | GUARD | NONE | — | | `.husky/pre-commit` | a commit failing lint-staged, decisions, root-PNG or format | GUARD | NONE | — | | `.husky/pre-push` | a push failing any pre-push hook or the SPA gate | GUARD | NONE | — | @@ -127,11 +127,12 @@ all. | `scripts/tests/test_prepush_rebase_check_tag_exemption.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_prove_fix.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_remote_state_inventory.py` | the `script-tests` job, on an executable that talks to a remote service with no row in `docs/remote-state-inventory.md` | GUARD | MUTATION | `test_remote_state_inventory.py::test_MUTATION_PROOF_a_dropped_row_and_a_phantom_row_are_both_detected` | +| `scripts/tests/test_worktree_ownership_guard.py` | the `script-tests` job | PROOF | NONE | — | ## What the numbers say -34 guards, 5 tooling scripts, 15 proof files. **8 guards carry a mutation proof; 6 are -behaviour-only; 20 have none.** These figures are asserted against the table by +34 guards, 5 tooling scripts, 16 proof files. **10 guards carry a mutation proof; 6 are +behaviour-only; 18 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it, which is the duplication family this change argues against. Both cold reviewers found the error diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py new file mode 100644 index 000000000..a2c537c70 --- /dev/null +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -0,0 +1,346 @@ +"""The worktree-ownership mechanism is TWO files, and this drives both halves as one thing. + +`pretooluse-worktree-guard.sh` denies a `git commit`/`git merge` inside a worktree another session +created. It can only do that because `posttooluse-worktree-marker.sh` wrote the +`.claude-worktree-owner` marker at `git worktree add` time. Neither file had a test, and — the part +that makes this rank second in ersatztv#785 — **the halves had never been exercised together**, so a +regression in either one is invisible: the marker hook silently writing nothing and the guard hook +silently reading nothing produce the identical outcome, which is *the commit is allowed*, which is +also what a correct fail-open looks like. + +Both hooks are deliberately fail-open (`docs/decisions` — the main tree is never marked, and +pre-convention worktrees have no marker), and that is exactly why an absent mechanism is +indistinguishable from a working one from the outside. It is the shape +`testing.guard-ships-with-mutation-proof` was written for: "in every case a human had read the guard +and believed it worked. The guard was not subtly wrong, it was *absent*." + +So this file: + + * drives the REAL pair end to end over a REAL `git worktree add`, in the real payload shape — + marker hook first as the harness would fire it, then the guard hook; + * carries a negative control (a non-mutating git command) and a fail-open control (an unmarked + worktree), because a guard that denied everything would satisfy the deny assertions; + * and performs THREE clause-level mutations, one per clause the mechanism actually hangs on — + the guard's marker read, the guard's ownership comparison, and the *other file's* marker write. + The third is the one that could not exist while the halves were tested apart. +""" + +from __future__ import annotations + +import json +import os +import subprocess +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +GUARD = REPO_ROOT / ".claude" / "hooks" / "pretooluse-worktree-guard.sh" +MARKER_HOOK = REPO_ROOT / ".claude" / "hooks" / "posttooluse-worktree-marker.sh" +MARKER_NAME = ".claude-worktree-owner" + +SESSION_A = "session-aaaa-1111" +SESSION_B = "session-bbbb-2222" + +# The hooks resolve `scripts/hook-fire-log.sh` from CLAUDE_PROJECT_DIR, falling back to a path +# relative to their own location. A MUTATED copy lives in tmp_path, where that fallback finds +# nothing — so pin the project dir explicitly and both the real and the mutated hook load the same +# library. Otherwise the mutant differs from the subject in a second way and the comparison is not +# about the mutated clause any more. +_ENV = {**os.environ, "CLAUDE_PROJECT_DIR": str(REPO_ROOT)} + + +def _git(cwd: Path, *args: str) -> str: + p = 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", + }, + ) + return p.stdout + + +def _scratch_repo(tmp_path: Path) -> Path: + repo = tmp_path / "main-tree" + repo.mkdir() + _git(repo, "init", "-q", "-b", "main", ".") + (repo / "f.txt").write_text("one\n") + _git(repo, "add", "f.txt") + _git(repo, "commit", "-qm", "init") + return repo + + +def _run(hook: Path, payload: dict, cwd: Path) -> tuple[int, bytes]: + p = subprocess.run( + ["bash", str(hook)], + input=json.dumps(payload).encode(), + capture_output=True, + cwd=str(cwd), + env=_ENV, + timeout=60, + ) + return p.returncode, p.stdout + + +def _add_worktree(repo: Path, name: str, marker_hook: Path | None, session: str) -> Path: + """`git worktree add` exactly as a session does it, then fire the PostToolUse marker hook. + + The marker hook is driven with the payload the harness would hand it AFTER the command + succeeded, which is when PostToolUse fires — not a hand-planted marker file. A hand-planted + marker would make every deny below a test of the guard alone, and the untested seam is the + handoff between the two files. + """ + wt = repo.parent / name + _git(repo, "worktree", "add", "-q", str(wt)) + if marker_hook is not None: + rc, _ = _run( + marker_hook, + { + "session_id": session, + "hook_event_name": "PostToolUse", + "tool_name": "Bash", + "cwd": str(repo), + "tool_input": {"command": f"git worktree add {wt}"}, + }, + repo, + ) + assert rc == 0, "the marker hook must always exit 0" + return wt + + +def _commit_payload(session: str, cwd: Path, command: str = "git commit -m x") -> dict: + return { + "session_id": session, + "hook_event_name": "PreToolUse", + "tool_name": "Bash", + "cwd": str(cwd), + "tool_input": {"command": command}, + } + + +def _denied(out: bytes) -> bool: + return b'"permissionDecision": "deny"' in out or b'"permissionDecision":"deny"' in out + + +# ------------------------------------------------------------------------------------------------ +# ANTI-VACUITY — if the fixture never produces a marked worktree, every deny below is meaningless +# ------------------------------------------------------------------------------------------------ + + +def test_the_marker_hook_really_marks_the_worktree_it_was_told_about(tmp_path): + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + + marker = wt / MARKER_NAME + assert marker.is_file(), ( + f"no {MARKER_NAME} in {wt}. Without it the guard has nothing to read and every 'denied' " + "assertion in this file would be testing a mechanism that is not there" + ) + assert marker.read_text().strip() == SESSION_A, ( + f"the marker names {marker.read_text().strip()!r}, not the session that created the " + "worktree — ownership would be attributed to the wrong session" + ) + + +def test_the_marker_hook_ignores_a_command_that_is_not_a_worktree_add(tmp_path): + """The write side's own negative control: a hook that marked on any command would pass above.""" + repo = _scratch_repo(tmp_path) + wt = repo.parent / "wt-unrelated" + _git(repo, "worktree", "add", "-q", str(wt)) + rc, _ = _run( + MARKER_HOOK, + { + "session_id": SESSION_A, + "hook_event_name": "PostToolUse", + "tool_name": "Bash", + "cwd": str(repo), + "tool_input": {"command": f"ls {wt}"}, + }, + repo, + ) + assert rc == 0 + assert not (wt / MARKER_NAME).exists(), "the marker hook stamped a worktree it never created" + + +# ------------------------------------------------------------------------------------------------ +# THE PAIR DECIDES — both halves, in sequence, as the harness fires them +# ------------------------------------------------------------------------------------------------ + + +def test_a_commit_in_ANOTHER_sessions_worktree_is_DENIED(tmp_path): + """The #289 case the mechanism exists for, end to end across both files.""" + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + + rc, out = _run(GUARD, _commit_payload(SESSION_B, wt), wt) + assert rc == 0, "the hook communicates by printing, and must always exit 0" + assert _denied(out), f"a commit into session A's worktree was not denied from session B: {out!r}" + assert SESSION_A.encode() in out, ( + "the deny reason must name the owning session — without it the operator cannot tell whether " + f"the marker is stale or the worktree is genuinely foreign: {out!r}" + ) + + +def test_a_commit_in_MY_OWN_worktree_is_ALLOWED(tmp_path): + """Negative control. A guard that denied every marked worktree would pass the test above.""" + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + + rc, out = _run(GUARD, _commit_payload(SESSION_A, wt), wt) + assert rc == 0 + assert out == b"", f"the owning session was blocked from committing in its own worktree: {out!r}" + + +def test_an_UNMARKED_worktree_is_ALLOWED(tmp_path): + """The deliberate fail-open: pre-convention worktrees and the main tree carry no marker.""" + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-none", None, SESSION_A) + + rc, out = _run(GUARD, _commit_payload(SESSION_B, wt), wt) + assert rc == 0 + assert out == b"", f"an unmarked worktree was blocked, which breaks the main tree too: {out!r}" + + +def test_a_NON_MUTATING_git_command_in_a_foreign_worktree_is_ALLOWED(tmp_path): + """Only `commit`/`merge` are guarded; `git status` in a sibling worktree is normal work.""" + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + + rc, out = _run(GUARD, _commit_payload(SESSION_B, wt, "git status"), wt) + assert rc == 0 + assert out == b"", f"a read-only git command was denied: {out!r}" + + +def test_a_git_C_into_a_foreign_worktree_is_DENIED_from_the_main_tree(tmp_path): + """The redirection that makes the guard non-trivial. + + The session's cwd is its OWN tree — where committing is fine — and only the `-C` argument moves + the operation into the foreign worktree. A guard that looked at `cwd` alone would allow this, + and `git -C` is how the sibling-worktree commit actually gets typed. + """ + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + + rc, out = _run(GUARD, _commit_payload(SESSION_B, repo, f"git -C {wt} commit -m x"), repo) + assert rc == 0 + assert _denied(out), f"`git -C commit` was not denied: {out!r}" + + +# ------------------------------------------------------------------------------------------------ +# MUTATION PROOFS — three clauses, one per thing the mechanism hangs on +# +# Each mutant is an isolated copy with ONE clause disarmed, and each test asserts the UNMUTATED pair +# reaches the opposite decision on the same fixture FIRST. Without that positive control a mutation +# proof passes when the mechanism detects nothing at all, which is how the BOM guard sat fail-open +# for months while reading as covered. +# ------------------------------------------------------------------------------------------------ + + +def _mutate(src: Path, tmp_path: Path, old: str, new: str, why: str) -> Path: + assert old in src.read_text(), ( + f"the clause {old!r} has moved in {src.name}; RETARGET this mutation rather than loosening " + f"it — a mutation that silently stops mutating is the failure this file is about ({why})" + ) + dst = tmp_path / f"mutated-{src.name}" + dst.write_text(src.read_text().replace(old, new, 1)) + return dst + + +def test_MUTATION_disarming_the_guards_MARKER_READ_stops_the_deny(tmp_path): + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + payload = _commit_payload(SESSION_B, wt) + + rc_live, out_live = _run(GUARD, payload, wt) + assert rc_live == 0 and _denied(out_live), ( + "the UNMUTATED guard did not deny, so 'the mutant is silent' would prove nothing about the " + f"marker read: {out_live!r}" + ) + + mutant = _mutate( + GUARD, + tmp_path, + 'marker="$root/.claude-worktree-owner"', + 'marker="$root/.claude-worktree-owner-NOTHING-WRITES-THIS"', + "the guard's marker read", + ) + rc, out = _run(mutant, payload, wt) + assert rc == 0 + assert out == b"", ( + "the guard still denied with its marker read pointed at a file nothing writes, so the deny " + f"is not coming from ownership at all: {out!r}" + ) + + +def test_MUTATION_inverting_the_OWNERSHIP_COMPARISON_blocks_the_owner(tmp_path): + """The allow direction, which the deny mutation above cannot reach. + + Disarming the comparison the other way would only make the guard deny more, and every deny + assertion in this file would stay green. Inverting it is what shows the comparison — rather than + the mere presence of a marker — is what decides. + """ + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + own_payload = _commit_payload(SESSION_A, wt) + + rc_live, out_live = _run(GUARD, own_payload, wt) + assert rc_live == 0 and out_live == b"", ( + f"the UNMUTATED guard already blocked the owner, so the inversion below proves nothing: {out_live!r}" + ) + + mutant = _mutate( + GUARD, + tmp_path, + '[ "$owner" = "$me" ] && exit 0', + '[ "$owner" != "$me" ] && exit 0', + "the guard's ownership comparison", + ) + rc, out = _run(mutant, own_payload, wt) + assert rc == 0 + assert _denied(out), ( + "inverting the ownership comparison did not change the decision for the OWNING session, so " + f"the comparison is not what allows it through: {out!r}" + ) + + +def test_MUTATION_a_marker_hook_that_stops_WRITING_makes_the_guard_go_quiet(tmp_path): + """THE CROSS-FILE PROOF — the one that could not exist while the halves were tested apart. + + The clause disarmed here is in `posttooluse-worktree-marker.sh`; the assertion is about + `pretooluse-worktree-guard.sh`. A regression in the write half is otherwise completely silent: + the marker hook exits 0 either way, and the guard's fail-open turns a missing marker into an + allowed commit that looks exactly like a correctly allowed one. + """ + repo = _scratch_repo(tmp_path) + + wt_live = _add_worktree(repo, "wt-live", MARKER_HOOK, SESSION_A) + rc_live, out_live = _run(GUARD, _commit_payload(SESSION_B, wt_live), wt_live) + assert rc_live == 0 and _denied(out_live), ( + f"the UNMUTATED pair did not deny, so a silent mutant proves nothing about the marker write: {out_live!r}" + ) + + mutant_marker = _mutate( + MARKER_HOOK, + tmp_path, + 'printf \'%s\\n\' "$me" > "$abs/.claude-worktree-owner" 2>/dev/null || true', + "true", + "the marker hook's write", + ) + wt_dead = _add_worktree(repo, "wt-dead", mutant_marker, SESSION_A) + assert not (wt_dead / MARKER_NAME).exists(), ( + "the mutated marker hook wrote a marker anyway — the mutation did not disarm the write, so " + "the assertion below would be about nothing" + ) + + rc, out = _run(GUARD, _commit_payload(SESSION_B, wt_dead), wt_dead) + assert rc == 0 + assert out == b"", ( + "the guard denied a commit in a worktree that carries NO marker, which means the deny in " + f"the live case above is not evidence that the two halves are connected: {out!r}" + ) -- 2.47.3 From d05d6623d1cfb8ac0eff38f7cffd23dba3b38438 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:45:00 +0200 Subject: [PATCH 02/12] test(785): pin `.husky/pre-push`'s `unset GIT_DIR ...`, and measure why it matters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The one-line fix at pre-push:11 had nothing pinning it, and #785 ranks it third because the regression is a relocation rather than a deletion — a tidy-up that reads as a no-op. What the investigation established, since the comment above it asserts the mechanism without stating when it applies (git 2.55): * git exports GIT_DIR to pre-push when the push comes FROM A WORKTREE, and exports nothing from the main tree. `process.shared-tree-readonly` makes the worktree the mandated way to work here, so the exposed path is the normal one, not an edge case. * with GIT_DIR set and GIT_WORK_TREE unset, git stops discovering the repo and treats the CURRENT DIRECTORY as the work tree: `git rev-parse --show-toplevel` from web/ answers `/web`. * so `cd web && npm run check:api`'s `git diff --exit-code` compares against index paths that do not exist there and reports NO DIFF, exit 0. Generated-API drift ships and the gate prints success. The test drives the real pre-push file unedited, in a real worktree, with that exact environment; the hook calls are stubs (DERIVED from the file, so a new hook cannot leave the harness dying at a missing script and calling that a red) and `npm` is a stand-in whose `run check:api` performs the nested diff. Both mutations witnessed red against the real file: deletion -> 4 failed, 5 passed; relocation to the end -> 5 failed, 4 passed. The relocation mutation was wrong in the first draft. Appending the `unset` at the end also makes it the script's last statement, and a bare `unset` succeeds — so a `returncode == 0` assertion held for a reason unrelated to the nested git call. The stand-in npm now records the nested diff's OWN exit code and the mutations assert on that. Verified by making the mutation a no-op: the test fails, as it must. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 7 +- scripts/tests/test_prepush_unsets_git_env.py | 309 +++++++++++++++++++ 2 files changed, 313 insertions(+), 3 deletions(-) create mode 100644 scripts/tests/test_prepush_unsets_git_env.py diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 2fc169293..8e2c39999 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -88,7 +88,7 @@ all. | `.claude/hooks/pretooluse-worktree-guard.sh` | a commit/merge in a foreign worktree | GUARD | MUTATION | `test_worktree_ownership_guard.py::test_MUTATION_disarming_the_guards_MARKER_READ_stops_the_deny` | | `.husky/commit-msg` | a commit with no `Co-Authored-By` trailer | GUARD | NONE | — | | `.husky/pre-commit` | a commit failing lint-staged, decisions, root-PNG or format | GUARD | NONE | — | -| `.husky/pre-push` | a push failing any pre-push hook or the SPA gate | GUARD | NONE | — | +| `.husky/pre-push` | a push failing any pre-push hook or the SPA gate | GUARD | MUTATION | `test_prepush_unsets_git_env.py::test_MUTATION_DELETING_the_unset_lets_drift_through_silently` | | `scripts/build_decisions_catalog.py` | the `decisions-guard` job, on a stale catalog | GUARD | NONE | — | | `scripts/check-kickoff-guard.sh` | the `decisions-guard` job, on a revived #237 reference | GUARD | NONE | — | | `scripts/check-review-verdict.sh` | the merge-consent hook's verdict classification | GUARD | BEHAVIOUR-ONLY | `test_check_review_verdict.py::test_falseopen_token_must_be_a_whole_word` | @@ -123,6 +123,7 @@ all. | `scripts/tests/test_merge_consent_required_check.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_migration_equivalence.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_post_review_verdict.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_prepush_unsets_git_env.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_pr_changed_files.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_prepush_rebase_check_tag_exemption.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_prove_fix.py` | the `script-tests` job | PROOF | NONE | — | @@ -131,8 +132,8 @@ all. ## What the numbers say -34 guards, 5 tooling scripts, 16 proof files. **10 guards carry a mutation proof; 6 are -behaviour-only; 18 have none.** These figures are asserted against the table by +34 guards, 5 tooling scripts, 17 proof files. **11 guards carry a mutation proof; 6 are +behaviour-only; 17 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it, which is the duplication family this change argues against. Both cold reviewers found the error diff --git a/scripts/tests/test_prepush_unsets_git_env.py b/scripts/tests/test_prepush_unsets_git_env.py new file mode 100644 index 000000000..8cd4091bd --- /dev/null +++ b/scripts/tests/test_prepush_unsets_git_env.py @@ -0,0 +1,309 @@ +"""`.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** (git 2.55, macOS; the same holds on Linux): + + * 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]: + names = [] + for line in text.splitlines(): + marker = "./.claude/hooks/" + if marker in line: + rest = line.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" + ) -- 2.47.3 From f2240b0205b46e0d944d188dcb19904465285e7e Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:45:16 +0200 Subject: [PATCH 03/12] test(785): drive build_decisions_catalog's --check path, and its __main__ wiring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--check` is what the decisions-guard job runs; the existing tests call `render_catalog()` directly and never call `main()` at all. So neither the stale-detection comparison nor the `raise SystemExit(main())` wiring had any coverage. Two mutations, both witnessed red against the real script: * disarming `want.strip() != have.strip()` alone -> 3 failed, 3 passed * replacing `raise SystemExit(main())` with a bare `main()` -> 1 failed, 5 passed, and only the subprocess case reddens. That mutant prints `build_decisions_catalog: docs/decisions/README.md is stale` on stderr and exits 0. The workflow step reads nothing but the exit code, so CI would report green over a stale catalog — the #751/#719 shape the decision record names. The subprocess case runs the command DERIVED from pr-checks.yml rather than a restated copy of it, and runs it twice: once on an untouched copy of the corpus, which must exit 0. Without that half, any failure of the harness — an import error, a missing file in the copy — would also exit non-zero and read as the guard firing. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 7 +- .../tests/test_build_catalog_check_path.py | 247 ++++++++++++++++++ 2 files changed, 251 insertions(+), 3 deletions(-) create mode 100644 scripts/tests/test_build_catalog_check_path.py diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 8e2c39999..a7949fd32 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -89,7 +89,7 @@ all. | `.husky/commit-msg` | a commit with no `Co-Authored-By` trailer | GUARD | NONE | — | | `.husky/pre-commit` | a commit failing lint-staged, decisions, root-PNG or format | GUARD | NONE | — | | `.husky/pre-push` | a push failing any pre-push hook or the SPA gate | GUARD | MUTATION | `test_prepush_unsets_git_env.py::test_MUTATION_DELETING_the_unset_lets_drift_through_silently` | -| `scripts/build_decisions_catalog.py` | the `decisions-guard` job, on a stale catalog | GUARD | NONE | — | +| `scripts/build_decisions_catalog.py` | the `decisions-guard` job, on a stale catalog | GUARD | MUTATION | `test_build_catalog_check_path.py::test_MUTATION_disarming_the_stale_comparison_stops_detection` | | `scripts/check-kickoff-guard.sh` | the `decisions-guard` job, on a revived #237 reference | GUARD | NONE | — | | `scripts/check-review-verdict.sh` | the merge-consent hook's verdict classification | GUARD | BEHAVIOUR-ONLY | `test_check_review_verdict.py::test_falseopen_token_must_be_a_whole_word` | | `scripts/ci-detect-already-validated.sh` | nothing directly (feeds the skip gate) | GUARD | NONE | — | @@ -109,6 +109,7 @@ all. | `scripts/update-openapi.sh` | nothing (regenerates the spec) | TOOLING | NONE | — | | `scripts/tests/test_bom_guard_detection.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_build_catalog.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_build_catalog_check_path.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_check_review_verdict.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_ci_dropped_step_guard.py` | the `script-tests` job | PROOF | NONE | — | | `scripts/tests/test_ci_image_pin_population.py` | the `script-tests` job, when a container job loses its pin | GUARD | MUTATION | `test_ci_image_pin_population.py::test_a_single_job_losing_its_pin_is_DETECTED` | @@ -132,8 +133,8 @@ all. ## What the numbers say -34 guards, 5 tooling scripts, 17 proof files. **11 guards carry a mutation proof; 6 are -behaviour-only; 17 have none.** These figures are asserted against the table by +34 guards, 5 tooling scripts, 18 proof files. **12 guards carry a mutation proof; 6 are +behaviour-only; 16 have none.** These figures are asserted against the table by `test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it, which is the duplication family this change argues against. Both cold reviewers found the error diff --git a/scripts/tests/test_build_catalog_check_path.py b/scripts/tests/test_build_catalog_check_path.py new file mode 100644 index 000000000..9b6cbeb57 --- /dev/null +++ b/scripts/tests/test_build_catalog_check_path.py @@ -0,0 +1,247 @@ +"""`scripts/build_decisions_catalog.py`'s `--check` path — the one CI actually runs. + +`decisions-guard` (`.gitea/workflows/pr-checks.yml`, the "Active catalog in sync" step) runs +`python3 scripts/build_decisions_catalog.py --check`. `scripts/tests/test_build_catalog.py` covers +`render_catalog()` directly and never calls `main()` at all, so nothing there proves: + + * that `main()`'s stale-detection comparison (`want.strip() != have.strip()`) is load-bearing — + a version that always agreed would pass every existing test; + * that the `if __name__ == "__main__": raise SystemExit(main())` wiring actually turns a stale + catalog into a non-zero process exit code, which is the only thing CI's `run:` step reads. + +`testing.guard-ships-with-mutation-proof` (#775) is explicit that a guard is not tested because a +test *involving* it passes: it ships with a mutation proof — disarm the guard's own clause, alone, +and a named test must go red. This file is that proof for the catalog-guard, plus the subprocess +proof that the `__main__` wiring is connected (the #751/#719 shape the decision record names: a +green `main()` behind dead wiring). +""" + +from __future__ import annotations + +import importlib.util +import subprocess +import sys +from pathlib import Path + +import scripts.build_decisions_catalog as bc +import scripts.decisions_lib as dl + +REPO_ROOT = Path(__file__).resolve().parents[2] +CATALOG_SCRIPT = REPO_ROOT / "scripts" / "build_decisions_catalog.py" +DECISIONS_LIB = REPO_ROOT / "scripts" / "decisions_lib.py" +SCRIPTS_INIT = REPO_ROOT / "scripts" / "__init__.py" + + +def _current_catalog_text() -> str: + return bc.render_catalog(dl.all_active_records()) + + +# ------------------------------------------------------------------------------------------------ +# ANTI-VACUITY — if the real corpus is empty, every assertion below passes for nothing. +# ------------------------------------------------------------------------------------------------ + + +def test_the_real_corpus_is_non_empty_and_renders_a_real_catalog(): + records = dl.all_active_records() + active = [r for r in records if r.status == "active" and r.key] + assert active, "no active decision records were parsed — every test below would be vacuous" + + text = _current_catalog_text() + assert bc.BANNER in text, "render_catalog produced no banner — not a real catalog document" + assert f"`{active[0].key}`" in text, ( + "render_catalog produced no row for a known active record — not a real catalog document" + ) + + +# ------------------------------------------------------------------------------------------------ +# `main(["--check"])` — the comparison CI reads +# ------------------------------------------------------------------------------------------------ + + +def test_check_returns_0_when_OUTPUT_matches_render_catalog(tmp_path, monkeypatch): + fresh = _current_catalog_text() + output = tmp_path / "README.md" + output.write_text(fresh.rstrip("\n") + "\n", encoding="utf-8") + monkeypatch.setattr(bc, "OUTPUT", output) + + assert bc.main(["--check"]) == 0 + + +def test_check_returns_1_when_OUTPUT_is_stale(tmp_path, monkeypatch): + fresh = _current_catalog_text() + output = tmp_path / "README.md" + # Append a line: the on-disk file no longer matches what render_catalog would produce. + output.write_text(fresh.rstrip("\n") + "\nEXTRA STALE LINE\n", encoding="utf-8") + monkeypatch.setattr(bc, "OUTPUT", output) + + assert bc.main(["--check"]) == 1 + + +def test_generate_then_check_round_trips(tmp_path, monkeypatch): + """The no-argument path WRITES the catalog, and a subsequent --check must then pass. + + This pins the property CI depends on: generate and check agree. If they ever diverged, `main([])` + would produce a file that `main(["--check"])` immediately rejects — a self-contradiction that + would make the generator useless for fixing the exact problem `--check` reports. + """ + output = tmp_path / "README.md" + assert not output.exists() + monkeypatch.setattr(bc, "OUTPUT", output) + + assert bc.main([]) == 0 + assert output.exists(), "main([]) with no --check must write OUTPUT" + written = output.read_text(encoding="utf-8") + assert written.strip() == _current_catalog_text().strip() + + assert bc.main(["--check"]) == 0, "the file main([]) just wrote must satisfy main(['--check'])" + + +# ------------------------------------------------------------------------------------------------ +# THE `__main__` WIRING — proof CI's subprocess invocation actually surfaces staleness +# ------------------------------------------------------------------------------------------------ + + +def _seed_decisions_copy(root: Path) -> None: + """Copy only what build_decisions_catalog.py + decisions_lib.py need to resolve a real corpus.""" + (root / "scripts").mkdir(parents=True) + (root / "scripts" / "__init__.py").write_bytes(SCRIPTS_INIT.read_bytes()) + (root / "scripts" / "build_decisions_catalog.py").write_bytes(CATALOG_SCRIPT.read_bytes()) + (root / "scripts" / "decisions_lib.py").write_bytes(DECISIONS_LIB.read_bytes()) + + docs = root / "docs" + docs.mkdir() + (docs / "decisions.md").write_bytes((REPO_ROOT / "docs" / "decisions.md").read_bytes()) + + dst_decisions = docs / "decisions" + src_decisions = REPO_ROOT / "docs" / "decisions" + dst_decisions.mkdir() + for item in src_decisions.iterdir(): + if item.is_dir(): + _copy_tree(item, dst_decisions / item.name) + else: + (dst_decisions / item.name).write_bytes(item.read_bytes()) + + +def _copy_tree(src: Path, dst: Path) -> None: + dst.mkdir(parents=True, exist_ok=True) + for item in src.rglob("*"): + rel = item.relative_to(src) + target = dst / rel + if item.is_dir(): + target.mkdir(parents=True, exist_ok=True) + else: + target.parent.mkdir(parents=True, exist_ok=True) + target.write_bytes(item.read_bytes()) + + +WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "pr-checks.yml" + + +def _ci_check_command() -> list[str]: + """The command the `decisions-guard` job runs, DERIVED from the workflow, not restated here. + + `testing.guard-derives-population-from-source`: a hand-copied command is a second copy of the + workflow that drifts silently, and this test's entire value is that it runs what CI runs. If the + job is reworded, this fails loudly instead of proving the wiring of a command nobody invokes. + """ + for line in WORKFLOW.read_text().splitlines(): + if "build_decisions_catalog.py" in line and "--check" in line: + body = line.split("run:", 1)[1].strip() if "run:" in line else line.strip() + assert body.startswith("PYTHONPATH=."), ( + f"the decisions-guard invocation no longer starts with PYTHONPATH=.: {body!r}" + ) + return ["bash", "-c", body.replace("python3", sys.executable, 1)] + raise AssertionError( + "no `build_decisions_catalog.py --check` line found in pr-checks.yml — either the guard " + "stopped running in CI (which is the more serious finding) or this extractor has drifted" + ) + + +def test_CLI_subprocess_exits_nonzero_on_a_stale_catalog(tmp_path): + """The real CI invocation, as a subprocess, against a real corpus. + + This is the `__main__` → `SystemExit(main())` wiring proof: `main()` returning 1 is worthless if + the process still exits 0, and the workflow step reads nothing but the exit code. It is the + #751/#719 shape the decision record names — a green result behind wiring that is not connected. + + The FRESH half is not optional decoration. Without it this test passes whenever the subprocess + dies for any reason at all — an import error, a missing file in the copy, a syntax error — none + of which is the guard detecting anything. `arbitrary-sample-gives-false-negatives` in reverse: + a non-zero exit is only evidence when the same harness is shown to exit zero on a clean corpus. + """ + repo = tmp_path / "repo-copy" + repo.mkdir() + _seed_decisions_copy(repo) + cmd = _ci_check_command() + + fresh = subprocess.run(cmd, cwd=str(repo), capture_output=True, text=True, timeout=60) + assert fresh.returncode == 0, ( + "the copied corpus does not even pass --check when untouched, so a non-zero exit below " + f"would be the harness failing rather than the guard firing: {fresh.stdout!r} {fresh.stderr!r}" + ) + + readme = repo / "docs" / "decisions" / "README.md" + readme.write_text(readme.read_text(encoding="utf-8") + "\nSTALE INJECTED LINE\n", encoding="utf-8") + + stale = subprocess.run(cmd, cwd=str(repo), capture_output=True, text=True, timeout=60) + assert stale.returncode != 0, ( + "the CLI wiring did not surface a stale catalog as a non-zero exit — CI would report " + f"green over a stale README.md: stdout={stale.stdout!r} stderr={stale.stderr!r}" + ) + + +# ------------------------------------------------------------------------------------------------ +# THE MUTATION PROOF — disarm `main()`'s stale-detection clause alone, --check must stop detecting +# ------------------------------------------------------------------------------------------------ + + +def _load_mutated_module(tmp_path: Path, mutated_source: str): + mutated_path = tmp_path / "mutated_build_decisions_catalog.py" + mutated_path.write_text(mutated_source, encoding="utf-8") + spec = importlib.util.spec_from_file_location("mutated_build_decisions_catalog", mutated_path) + assert spec and spec.loader + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +def test_MUTATION_disarming_the_stale_comparison_stops_detection(tmp_path, monkeypatch): + """`testing.guard-ships-with-mutation-proof` (#775). + + `main()`'s ONLY stale-detection logic is the clause `want.strip() != have.strip()`. Disarm that + clause alone — replace it with the constant `False` in an isolated copy of the module, changing + nothing else — and `--check` must stop reporting staleness. If it still returns 1, the deny is + coming from somewhere other than the clause the guard is supposed to hang on, and every test + above proves nothing about it. + """ + text = CATALOG_SCRIPT.read_text(encoding="utf-8") + clause = "want.strip() != have.strip()" + assert clause in text, ( + "the stale-detection clause has moved or been reworded; RETARGET this mutation at its new " + "location rather than loosening the string match — a mutation that silently stops mutating " + "is the exact failure this file exists to catch" + ) + mutated_source = text.replace(clause, "False", 1) + assert mutated_source != text and clause not in mutated_source, ( + "the replacement did not change the source, so the mutant is the subject" + ) + + fresh = _current_catalog_text() + output = tmp_path / "README.md" + output.write_text(fresh.rstrip("\n") + "\nEXTRA STALE LINE\n", encoding="utf-8") + + # POSITIVE CONTROL FIRST. Without this, the mutation assertion below would pass just as well if + # `main(["--check"])` never detected anything at all on this input — "the mutant is silent" + # proves nothing unless the real guard is first shown to be loud on the exact same input. + monkeypatch.setattr(bc, "OUTPUT", output) + assert bc.main(["--check"]) == 1, ( + "the UNMUTATED guard did not detect the staleness on this input, so a silent mutant below " + "would prove nothing about the clause" + ) + + mutant = _load_mutated_module(tmp_path, mutated_source) + mutant.OUTPUT = output + assert mutant.main(["--check"]) == 0, ( + "disarming `want.strip() != have.strip()` alone did not stop --check from reporting " + "staleness, so that clause is not what the guard's exit code hangs on" + ) -- 2.47.3 From 3651b06c3c496c6afa55254771a023c8b96cb3dd Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:45:31 +0200 Subject: [PATCH 04/12] docs(785): record what each mutation established, and why every remaining row is NONE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ranked list at the bottom of the inventory named four gaps; all four now carry clause-level proofs. It keeps the ranking — that is the reusable part — and states what each mutation actually established, because two of the four turned out worse than the ranking predicted (the BOM guard was fail-open on every non-vim host; the catalog guard's wiring can drop a failure while still printing it). Two additions the table cannot express: * what a file-level MUTATION grade does NOT claim. Three of these four rows are multi-purpose files. Per `testing.guard-ships-with-mutation-proof` the grade covers the clause its cited case mutates, not every line in the file: `.husky/pre-push` runs four other things and its row asserts only that line 11 is load-bearing. Reading it as "pre-push is tested" is the relabelling error the withdrawn-guard table above already exists to prevent. * why each remaining guard is still NONE — the third `## Done-when` box. They split three ways, and only the first is a backlog: the PreToolUse advisory hooks (unproven, now observable via #776, which is the weaker claim); the two husky dispatchers, whose mutable clauses live in the guards they call and whose dispatch is already covered by `test_every_hook_file_is_actually_WIRED`; and six that each need their own harness rather than a proof written to clear a row. That list is prose and nothing checks it. Stated as such in the file rather than left to be discovered — a guard moving between groups will not redden anything. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 91 ++++++++++++++++++++++++++++++----------- 1 file changed, 68 insertions(+), 23 deletions(-) diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index a7949fd32..5a4edddbe 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -195,35 +195,80 @@ on the same reasoning as the vocabulary-parity withdrawal above. the BOM guard was regraded, and `test_the_summary_counts_match_the_table` cannot see it — the parser checks the formatted summary sentence and nothing else. A second hand-maintained copy of a number is the duplication family this file argues against, so the copy is removed rather than corrected: the -set is every `PreToolUse` hook except merge-consent and the BOM guard, and the count is in the -summary above. +set is every `PreToolUse` hook except merge-consent, the BOM guard and the worktree pair, and the +count is in the summary above. **They are now observable but still unproven, and the two words carry different weight.** Observable: every hook records its own execution through `scripts/hook-fire-log.sh`, so "did this hook fire, and what did it decide" is a measurement — run `scripts/hook-fire-log.sh report` (#776). Unproven: -nobody has demonstrated any of them is load-bearing, which is what #785 tracks. Observability tells -you a guard ran; only a mutation tells you it would have caught anything. The BOM guard is the case -that shows why the distinction matters — it was firing on every commit the whole time it was -fail-open. +nobody has demonstrated any of them is load-bearing — the first group in the standing list below. +Observability tells you a guard ran; only a mutation tells you it would have caught anything. The +BOM guard is the case that shows why the distinction matters — it was firing on every commit the +whole time it was fail-open. The gaps are not uniform in cost, and the ranking that matters is *what a silent failure would let -through*, not test count: +through*, not test count. **All four ranked entries now carry clause-level mutation proofs (#785); +they are kept here with what each mutation established, because the ranking is the reusable part and +because two of them turned out to be worse than the ranking predicted.** -1. ~~`pretooluse-bom-guard.sh`~~ — **now proven, and it was fail-open the whole time.** Ranked - first here because the defect it guards has recurred three times (#311, #402, #405); that - ranking turned out to be right for a worse reason than intended. It detected a BOM with - `xxd -p`, and `xxd` ships with vim and is **absent on the Linux CI runner**, so the comparison - never matched and every BOM was allowed in silence. `od` now, with a clause-level mutation proof. - The lesson for the rows below: an unproven guard is not merely untested, it is a guard whose - *current* behaviour nobody has established. -2. `pretooluse-worktree-guard.sh` + `posttooluse-worktree-marker.sh` — a two-file mechanism guarding - #289, where a regression in either half is invisible and the two halves have never been tested - together. -3. `.husky/pre-push:11`'s `unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE` — a one-line fix for a real - bug (a nested `git diff --exit-code` silently reporting no diff) that nothing pins. Reordering it - after the nested git calls reintroduces the bug silently. -4. `scripts/build_decisions_catalog.py` — its `--check` path is what CI runs and no test calls - `main()` at all; the tests exercise `render_catalog()` directly. +1. ~~`pretooluse-bom-guard.sh`~~ — **proven, and it was fail-open the whole time.** Ranked first + because the defect it guards has recurred three times (#311, #402, #405); that ranking turned out + to be right for a worse reason than intended. It detected a BOM with `xxd -p`, and `xxd` ships + with vim and is **absent on the Linux CI runner**, so the comparison never matched and every BOM + was allowed in silence. `od` now. The lesson for the rows below: an unproven guard is not merely + untested, it is a guard whose *current* behaviour nobody has established. +2. ~~`pretooluse-worktree-guard.sh` + `posttooluse-worktree-marker.sh`~~ — **proven as a pair.** + Three clauses were disarmed and witnessed red: the guard's marker read, the guard's ownership + comparison (inverted, because disarming it the other way only makes the guard deny more and every + deny assertion stays green), and — the one that could not exist while the halves were tested + apart — the *marker hook's write*, asserted against the *guard's* decision. Both hooks are + deliberately fail-open, so an absent mechanism and a working one produce the identical "commit + allowed". +3. ~~`.husky/pre-push:11`'s `unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE`~~ — **proven, and the case + is the normal one rather than an edge.** Git exports `GIT_DIR` to `pre-push` **when the push + comes from a worktree** and not from the main tree; `process.shared-tree-readonly` makes the + worktree the mandated way to work here, so every push takes the exposed path. With `GIT_DIR` set + and `GIT_WORK_TREE` unset git stops discovering the repo and treats the *current directory* as + the work tree, so `cd web && npm run check:api`'s `git diff --exit-code` compares against index + paths that do not exist and reports no diff. Both the deletion and the relocation are proved. +4. ~~`scripts/build_decisions_catalog.py`~~ — **proven, including the wiring.** The `--check` + comparison is mutated directly, and a separate subprocess case runs the command *derived from + `pr-checks.yml`* against a copied corpus. That second case is not redundant: replacing + `raise SystemExit(main())` with a bare `main()` leaves the script printing + `docs/decisions/README.md is stale` on stderr while exiting **0**, and the workflow step reads + nothing but the exit code. Only the subprocess case reddens — the #751/#719 shape. -Filling these is tracked rather than done in one pass, deliberately: a mutation proof written to +**What a file-level `MUTATION` grade does and does not claim, because three of these four rows are +multi-purpose files.** The grade covers *the clause the cited case actually mutates*, per +`testing.guard-ships-with-mutation-proof`; it is not a statement about every line in the file. +`.husky/pre-push` runs four other things, and its row asserts only that line 11 is load-bearing — +the three hooks it invokes carry their own rows and their own grades. Reading the row as "pre-push +is tested" is the same relabelling error the withdrawn-guard table above exists to prevent. +Clause-level grading is tracked in #790. + +**The remaining unproven guards, and why each is still `NONE` rather than merely unattended** — the +third `## Done-when` box of #785, answered here rather than left implicit. They fall into three +groups, and the split is the point: only the first is a backlog. + +- **The `PreToolUse` advisory hooks** (`decisions-guard.sh`, `design-sync-reminder.sh`, + `pretooluse-agent-model.sh`, `pretooluse-agent-ram.sh`, `pretooluse-bash-guard.sh`, + `pretooluse-nav-guard.sh`, `prepush-clean-worktree-check.sh`, `prepush-donewhen.sh`) — genuinely + unproven, and a real backlog. They are now *observable* (`scripts/hook-fire-log.sh report`, #776), + which is a weaker claim than proven and is stated as such below. +- **`.husky/commit-msg` and `.husky/pre-commit`** — thin dispatchers to `lint-staged`, the decisions + guard, the root-PNG check and the format gate, each of which is a guard in its own right. A + mutation proof of the dispatcher would pin the dispatch, which is what + `test_every_hook_file_is_actually_WIRED` already covers; the clauses worth mutating live in the + guards it calls. +- **`ci-detect-already-validated.sh`, `ci-detect-docs-only.sh`, `ci-prove-ban-detects.sh`, + `e2e-functional.sh`, `check-kickoff-guard.sh`, `test_ci_release_path_scan_job.py`** — not + unattended either, but each needs its own harness rather than a proof written to clear a row. + `ci-prove-ban-detects.sh` is the exception noted in `testing.guard-ships-with-mutation-proof`: it + runs its own mutation at CI time, because what it proves is disarmable from inside pytest. + +That list is prose and nothing checks it, which is the honest limit — a guard moving out of a group +will not redden anything. It is here so the next session inherits *why* a row is `NONE`, which is +the distinction #785 asked for; the machine-checked half remains the table. + +Filling the rest is tracked rather than done in one pass, deliberately: a mutation proof written to close a row is the kind of test that passes for the wrong reason. -- 2.47.3 From 9ebf8023f91b51f97bf24d68e190bdb348492504 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:49:07 +0200 Subject: [PATCH 05/12] docs(785): correct three false claims in the remaining-NONE groups MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the prose added in the previous commit found three statements that are not true of the files they describe: * `.husky/commit-msg` was called a thin dispatcher. It dispatches to nothing — it is one `grep -q '^Co-Authored-By:'` plus a `MERGE_HEAD` exemption, and nothing tests that the exemption fires only for merges. * `.husky/pre-commit` was called one too. It dispatches to `lint-staged` and `decisions-guard.sh`, but the root-level-`*.png` refusal and the `dotnet format whitespace --verify-no-changes` block are its own clauses. * eight hooks were grouped as "the `PreToolUse` advisory hooks". Only four are `PreToolUse`; two are pre-push, one pre-commit, one `Stop`. The first two mattered most: "it dispatches to guards" was being used to argue those rows need no proof of their own, which is the relabelling error this file's withdrawn-guard table already exists to prevent — committed by the paragraph explaining the grades. Both are now stated as backlogs. Membership was verified by deriving the GUARD+NONE set from the table: 16 rows, 8 + 2 + 6, matching the three groups exactly. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 5a4edddbe..78c7af0b4 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -248,18 +248,22 @@ Clause-level grading is tracked in #790. **The remaining unproven guards, and why each is still `NONE` rather than merely unattended** — the third `## Done-when` box of #785, answered here rather than left implicit. They fall into three -groups, and the split is the point: only the first is a backlog. +groups, and the split is the point: the first two are backlogs, the third is not. -- **The `PreToolUse` advisory hooks** (`decisions-guard.sh`, `design-sync-reminder.sh`, - `pretooluse-agent-model.sh`, `pretooluse-agent-ram.sh`, `pretooluse-bash-guard.sh`, - `pretooluse-nav-guard.sh`, `prepush-clean-worktree-check.sh`, `prepush-donewhen.sh`) — genuinely - unproven, and a real backlog. They are now *observable* (`scripts/hook-fire-log.sh report`, #776), - which is a weaker claim than proven and is stated as such below. -- **`.husky/commit-msg` and `.husky/pre-commit`** — thin dispatchers to `lint-staged`, the decisions - guard, the root-PNG check and the format gate, each of which is a guard in its own right. A - mutation proof of the dispatcher would pin the dispatch, which is what - `test_every_hook_file_is_actually_WIRED` already covers; the clauses worth mutating live in the - guards it calls. +- **The eight remaining hook guards** — genuinely unproven, and a real backlog. Four are + `PreToolUse` (`pretooluse-agent-model.sh`, `pretooluse-agent-ram.sh`, `pretooluse-bash-guard.sh`, + `pretooluse-nav-guard.sh`), two are pre-push (`prepush-clean-worktree-check.sh`, + `prepush-donewhen.sh`), one is pre-commit (`decisions-guard.sh`) and one is `Stop` + (`design-sync-reminder.sh`); they are grouped by their status, not by their event. They are now + *observable* (`scripts/hook-fire-log.sh report`, #776), which is a weaker claim than proven and is + stated as such above. +- **`.husky/commit-msg` and `.husky/pre-commit`** — unproven, and each carries its own clause, so + they are a backlog too rather than "covered by what they call". `pre-commit` dispatches to + `lint-staged` and `decisions-guard.sh`, but the root-level-`*.png` refusal and the + `dotnet format whitespace --verify-no-changes` block are its own. `commit-msg` is not a dispatcher + at all: it is one `grep -q '^Co-Authored-By:'` plus a `MERGE_HEAD` exemption, and nothing tests + that the exemption fires only for merges. Neither is covered by what it calls — the trap is to read + "it dispatches to guards" as "its own clauses are guarded". - **`ci-detect-already-validated.sh`, `ci-detect-docs-only.sh`, `ci-prove-ban-detects.sh`, `e2e-functional.sh`, `check-kickoff-guard.sh`, `test_ci_release_path_scan_job.py`** — not unattended either, but each needs its own harness rather than a proof written to clear a row. -- 2.47.3 From 77317e5c71656d4045d123e135d26d17778691fa Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:56:37 +0200 Subject: [PATCH 06/12] test(785): pin the pre-push premise to both platforms, measured not assumed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The docstring said "git 2.55, macOS; the same holds on Linux". The second half was an assertion, and this repo's canonical disaster (#775, the BOM guard) is precisely a guard that behaved differently on the platform nobody measured — macOS had `xxd`, the Linux runner did not, and it was fail-open there for months. So it was measured on the runner host (Linux, git 2.47.3) rather than reasoned about: * git exports GIT_DIR=/.git/worktrees/ to pre-push on a push from a worktree * `git rev-parse --show-toplevel` from web/ with that GIT_DIR answers `/web` * the nested `git diff --exit-code` reports exit 0 on a drifted file and all three new suites were run there: 25 passed. Disarming the `unset` on that host reddens 4 of the 9, the same four as on macOS. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- scripts/tests/test_prepush_unsets_git_env.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/scripts/tests/test_prepush_unsets_git_env.py b/scripts/tests/test_prepush_unsets_git_env.py index 8cd4091bd..6d9a73aaf 100644 --- a/scripts/tests/test_prepush_unsets_git_env.py +++ b/scripts/tests/test_prepush_unsets_git_env.py @@ -5,7 +5,11 @@ pinned it: reordering it after the nested git calls, or dropping it in a tidy-up 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** (git 2.55, macOS; the same holds on Linux): +**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 `/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 -- 2.47.3 From fe01982a970fd4c7e9974e405a05f16cc5f75004 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 19:59:29 +0200 Subject: [PATCH 07/12] test(785): close two findings from the independent cold review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both are the same species the milestone is about — a coverage claim resting on source-text presence rather than on the guard actually running or the clause actually being reached. MEDIUM, `test_build_catalog_check_path.py`. `_ci_check_command()` grepped the whole workflow for the command text. So it matched a step that had been COMMENTED OUT, and it was not scoped to the `decisions-guard` job at all — a match anywhere in the file would do. Commenting out a step is the ordinary way a check gets switched off during an incident and never switched back, and the subprocess proof would have stayed green over a guard CI no longer runs. `guard-ships-with-mutation-proof`: the guard being WIRED is not the guard RUNNING. Now the extractor finds the job by key, stops at the next top-level job, strips comment lines before matching, and asserts exactly one active step. `test_the_extractor_rejects_a_ COMMENTED_OUT_step` pins it. Witnessed against the real workflow: commenting the step out reddens 2 of 7, deleting it reddens the same 2. LOW, `test_worktree_ownership_guard.py`. The docstring claimed the three mutations covered every clause the pair hangs on. They did not: the detection clause is `(commit|merge)`, and every payload in the file used `commit`, so `merge` could be dropped and the file stayed green. That is the more damaging half to lose — the mechanism exists for the plumbing-merge path (`process.foreign-worktree-plumbing-merge`). Adds a merge deny case and a fourth mutation, narrow by construction: the mutant must stop denying a merge WHILE STILL denying a commit, so a mutation that merely broke the regex cannot pass for it. Witnessed: dropping `merge` from the real hook reddens exactly those 2. The docstring now also states which clauses are NOT mutated (the `git -C`/`cd` redirection extraction, the marker hook's argument parsing) rather than implying the file is exhaustive. The review's third finding — the inventory calling both husky files thin dispatchers — was raised against ad0bed458..7bd7b4c8 and had already been corrected in fdc91056f. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/test_build_catalog_check_path.py | 103 +++++++++++++++--- .../tests/test_worktree_ownership_guard.py | 62 ++++++++++- 2 files changed, 149 insertions(+), 16 deletions(-) diff --git a/scripts/tests/test_build_catalog_check_path.py b/scripts/tests/test_build_catalog_check_path.py index 9b6cbeb57..64ec9b557 100644 --- a/scripts/tests/test_build_catalog_check_path.py +++ b/scripts/tests/test_build_catalog_check_path.py @@ -137,23 +137,100 @@ def _copy_tree(src: Path, dst: Path) -> None: WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "pr-checks.yml" -def _ci_check_command() -> list[str]: +def _decisions_guard_run_lines(workflow: Path | None = None) -> list[str]: + """Every ACTIVE `run:` line inside the `decisions-guard` job, scoped to that job. + + Three things this deliberately does, each closing a way the extractor could report coverage + that is not there: + + * it finds the job by its key (`decisions-guard:`) and stops at the next top-level job, so a + matching command in some OTHER job cannot stand in for this one; + * it strips comment lines FIRST, so a step that has been commented out — the ordinary way a + CI check gets switched off during an incident and never switched back — does not read as + wired. `guard-ships-with-mutation-proof` is explicit that the guard being WIRED is not the + guard RUNNING, and a text search over YAML cannot tell those apart unless it excludes + comments; + * it returns the job's run lines rather than a single grepped match, so the caller asserts + the command it needs is among the commands that job actually executes. + """ + lines = (workflow or WORKFLOW).read_text().splitlines() + try: + start = next(i for i, ln in enumerate(lines) if ln.strip() == "decisions-guard:") + except StopIteration: + raise AssertionError( + "no `decisions-guard:` job in pr-checks.yml. Either the job was renamed or removed — " + "the second is the far more serious finding, and this test is where it surfaces" + ) from None + indent = len(lines[start]) - len(lines[start].lstrip()) + body = [] + for ln in lines[start + 1 :]: + stripped = ln.strip() + if stripped and not stripped.startswith("#"): + cur = len(ln) - len(ln.lstrip()) + if cur <= indent: + break + if stripped.startswith("#"): + continue + body.append(ln) + return [ln.split("run:", 1)[1].strip() for ln in body if "run:" in ln] + + +def _ci_check_command(workflow: Path | None = None) -> list[str]: """The command the `decisions-guard` job runs, DERIVED from the workflow, not restated here. `testing.guard-derives-population-from-source`: a hand-copied command is a second copy of the - workflow that drifts silently, and this test's entire value is that it runs what CI runs. If the - job is reworded, this fails loudly instead of proving the wiring of a command nobody invokes. + workflow that drifts silently, and this test's entire value is that it runs what CI runs. """ - for line in WORKFLOW.read_text().splitlines(): - if "build_decisions_catalog.py" in line and "--check" in line: - body = line.split("run:", 1)[1].strip() if "run:" in line else line.strip() - assert body.startswith("PYTHONPATH=."), ( - f"the decisions-guard invocation no longer starts with PYTHONPATH=.: {body!r}" - ) - return ["bash", "-c", body.replace("python3", sys.executable, 1)] - raise AssertionError( - "no `build_decisions_catalog.py --check` line found in pr-checks.yml — either the guard " - "stopped running in CI (which is the more serious finding) or this extractor has drifted" + runs = _decisions_guard_run_lines(workflow) + matches = [r for r in runs if "build_decisions_catalog.py" in r and "--check" in r] + assert matches, ( + "the `decisions-guard` job has no ACTIVE step running " + "`build_decisions_catalog.py --check`. The catalog guard has stopped running in CI — that " + f"is the finding, not this test's failure. Active run lines in that job: {runs}" + ) + assert len(matches) == 1, f"expected exactly one such step, found {matches}" + body = matches[0] + assert body.startswith("PYTHONPATH=."), ( + f"the decisions-guard invocation no longer starts with PYTHONPATH=.: {body!r}" + ) + return ["bash", "-c", body.replace("python3", sys.executable, 1)] + + +def test_the_extractor_rejects_a_COMMENTED_OUT_step(): + """A step commented out is a guard that has stopped running, and must not read as wired. + + Without this the subprocess proof below stays green for as long as the text survives anywhere in + the file — including inside the `#` that switched it off. That is the shape + `guard-ships-with-mutation-proof` names as "the guard being WIRED is not the guard RUNNING". + """ + active = _decisions_guard_run_lines() + assert any("build_decisions_catalog.py" in r for r in active), "precondition: it is wired today" + for r in active: + assert not r.lstrip().startswith("#"), f"a comment line survived the strip: {r!r}" + + raw = WORKFLOW.read_text() + commented = raw.replace( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + " # - name: Active catalog in sync\n" + " # run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + 1, + ) + assert commented != raw, "retarget this mutation; the step's text has changed" + + import tempfile + + with tempfile.TemporaryDirectory() as d: + alt = Path(d) / "pr-checks.yml" + alt.write_text(commented) + raised = False + try: + _ci_check_command(alt) + except AssertionError: + raised = True + assert raised, ( + "the extractor still found the command after the step was commented out, so the subprocess " + "proof would stay green over a guard CI no longer runs" ) diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py index a2c537c70..4f3ba8cc9 100644 --- a/scripts/tests/test_worktree_ownership_guard.py +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -20,9 +20,14 @@ So this file: marker hook first as the harness would fire it, then the guard hook; * carries a negative control (a non-mutating git command) and a fail-open control (an unmarked worktree), because a guard that denied everything would satisfy the deny assertions; - * and performs THREE clause-level mutations, one per clause the mechanism actually hangs on — - the guard's marker read, the guard's ownership comparison, and the *other file's* marker write. - The third is the one that could not exist while the halves were tested apart. + * and performs FOUR clause-level mutations: the guard's marker read, the guard's ownership + comparison, the command-detection alternation (`commit|merge`), and the *other file's* marker + write. The last is the one that could not exist while the halves were tested apart. + +These four are the clauses whose disarm this file detects. They are not every line in either hook — +the `git -C` / `cd` redirection extraction and the marker hook's argument parsing are exercised +behaviourally but not mutated, and the grade in `docs/guard-inventory.md` covers the clause its +cited case mutates, not the whole file. """ from __future__ import annotations @@ -217,6 +222,22 @@ def test_a_NON_MUTATING_git_command_in_a_foreign_worktree_is_ALLOWED(tmp_path): assert out == b"", f"a read-only git command was denied: {out!r}" +def test_a_MERGE_in_a_foreign_worktree_is_DENIED(tmp_path): + """The other half of the guarded alternation. + + Every other deny case here uses `git commit`, so `merge` could be dropped from the detection + regex and this file would stay green — the mechanism guards the plumbing-merge path + (`process.foreign-worktree-plumbing-merge`) specifically, which makes that the more damaging + half to lose. + """ + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + + rc, out = _run(GUARD, _commit_payload(SESSION_B, wt, "git merge --no-ff topic"), wt) + assert rc == 0 + assert _denied(out), f"a merge into session A's worktree was not denied: {out!r}" + + def test_a_git_C_into_a_foreign_worktree_is_DENIED_from_the_main_tree(tmp_path): """The redirection that makes the guard non-trivial. @@ -344,3 +365,38 @@ def test_MUTATION_a_marker_hook_that_stops_WRITING_makes_the_guard_go_quiet(tmp_ "the guard denied a commit in a worktree that carries NO marker, which means the deny in " f"the live case above is not evidence that the two halves are connected: {out!r}" ) + + +def test_MUTATION_dropping_MERGE_from_the_detection_clause_stops_denying_a_merge(tmp_path): + """The alternation is two guarded operations, and losing one of them is silent. + + This mutation is deliberately narrow: it must stop the guard denying a `merge` while leaving it + denying a `commit`. Asserting both is what distinguishes "the alternation is load-bearing" from + "the mutant broke the regex", which would redden everything and prove nothing about `merge`. + """ + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + merge_payload = _commit_payload(SESSION_B, wt, "git merge --no-ff topic") + commit_payload = _commit_payload(SESSION_B, wt) + + rc_live, out_live = _run(GUARD, merge_payload, wt) + assert rc_live == 0 and _denied(out_live), ( + f"the UNMUTATED guard did not deny a merge, so a silent mutant proves nothing: {out_live!r}" + ) + + mutant = _mutate( + GUARD, + tmp_path, + "(commit|merge)\\b", + "(commit)\\b", + "the command-detection alternation", + ) + rc, out = _run(mutant, merge_payload, wt) + assert rc == 0 + assert out == b"", f"dropping `merge` from the alternation did not stop the merge being denied: {out!r}" + + rc_c, out_c = _run(mutant, commit_payload, wt) + assert rc_c == 0 and _denied(out_c), ( + "the mutant stopped denying COMMITS too, so it broke detection wholesale rather than " + f"removing the merge alternative — this proves nothing about `merge`: {out_c!r}" + ) -- 2.47.3 From 4822003004d93da648701e399eb1387547805ef9 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 20:09:14 +0200 Subject: [PATCH 08/12] fix(785): stop the tests writing to the PRODUCTION hook-fire log, and correct a mislabelled count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent cold reviews, both against the real subjects rather than the diff. Five findings; the first is the serious one. HIGH — `test_worktree_ownership_guard.py` was defeating conftest's isolation. `_ENV` was a module-level `{**os.environ, ...}` snapshot, taken at IMPORT time, which is before the autouse `isolate_hook_fire_log` fixture monkeypatches `ETV_HOOK_FIRE_LOG_DIR`. So every hook subprocess logged to the real `$HOME/.cache/ersatztv/hook-fire/`. Measured: 1,488 records for the two synthetic session ids, and one further run added 28 more. That is #776's defect reintroduced inside the file meant to prove #776's hooks, and it corrupts the surface `docs/guard-inventory.md` cites as the observability claim for every guard still graded NONE. Nothing surfaced it because the fire-log library is fail-open by design — a logging failure must leave the hook behaving exactly as an uninstrumented one. The env is now built per call. Measured after: a full run of the file adds 0 records. The stray records were deleted; `s1`/`unknown-session` were left alone, not being ours. `test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir` pins it, witnessed red by reintroducing the snapshot. It is per-file, which is the same narrowness as the guard that should have caught this: `test_hook_fire_log.py::test_the_suite_does_not_write_to_the_ PRODUCTION_log` says conftest "must isolate every test, not just this file's" but checks only the running test and its own sandbox. Recorded in the inventory and tracked in #809, including why the obvious general check (diff the log dir per test) is racy against a real session's hooks firing concurrently. MEDIUM — a9433d5cb's commit message mislabels a measurement. It reports the ownership comparison mutation as "2 failed, 8 passed (inverted)". Reproduced both: INVERTING the clause reddens 8 of 12 at today's head; 2 failed / 8 passed is what DELETING it produces (2 of 10 at that commit's head). The number is real but belongs to the mutation the parenthetical says was rejected. The shipped test is unaffected — inversion genuinely reddens `test_MUTATION_inverting_the_OWNERSHIP_COMPARISON_blocks_the_owner`. Corrected here rather than by amending, since the history is the record. LOW — `docs/guard-inventory.md` described the unproven set as "every PreToolUse hook except ..." two paragraphs above a standing list of eight hooks spanning four events. Both sentences named the same set. `design-sync-reminder.sh` is registered on BOTH PreToolUse and Stop in `.claude/settings.json`, so any "every X hook" phrasing double-counts it; the set is now named by enumeration. LOW — a truncated sentence ("`test_hook_fire_log.py` asserts TWO clauses", no verb, no closing bold) orphaned the following paragraph's "That unproven set". Completed with both clauses and their proofs. NIT — `_hook_calls` matched comment lines and only the `./` invocation form. A commented ghost hook would have reddened a correct tree; a `bash .claude/hooks/x.sh` form would have gone unstubbed and exited 127, which two tests read as "drift caught". Both verified. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 27 +++++++-- scripts/tests/test_prepush_unsets_git_env.py | 23 ++++++-- .../tests/test_worktree_ownership_guard.py | 56 ++++++++++++++++--- 3 files changed, 90 insertions(+), 16 deletions(-) diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 78c7af0b4..4bf1f4004 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -189,14 +189,30 @@ lines flagged wrongly — and cold review then constructed more of both (`[[ "$x a backslash continuation, a `>` inside a quoted string). Deleted rather than patched a fifth time, on the same reasoning as the vocabulary-parity withdrawal above. -**`test_hook_fire_log.py` asserts TWO clauses +**`test_hook_fire_log.py` asserts TWO clauses**, and each has its own proof: the coverage clause +(every hook reports that it fired) via `test_a_hook_that_LOSES_its_instrumentation_is_DETECTED`, and +the placement clause (`etv_hook_fire_begin` must precede the stdin read) via +`test_begin_placed_AFTER_the_stdin_read_is_DETECTED`. Its row cites the first. + +**Its `test_the_suite_does_not_write_to_the_PRODUCTION_log` is narrower than its docstring**, which +says `conftest.py` "must isolate every test, not just this file's". What it checks is that the +fixture set `ETV_HOOK_FIRE_LOG_DIR` *for the test currently running* and that a hook it drives in +its own sandbox does not touch the real log. It cannot see another suite that snapshots +`os.environ` at IMPORT time — before the autouse fixture runs — and hands that stale mapping to its +subprocesses. That suite's hooks then write to `$HOME/.cache/ersatztv/hook-fire/` while every +assertion stays green, because the fire-log library is fail-open. #785 shipped exactly that defect +and measured 1,488 stray records before catching it; the pin is +`test_worktree_ownership_guard.py::test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir`, +which is per-file rather than general. Generalising it is #809. **That unproven set carries no number here on purpose.** It restated the count, drifted the moment the BOM guard was regraded, and `test_the_summary_counts_match_the_table` cannot see it — the parser checks the formatted summary sentence and nothing else. A second hand-maintained copy of a number is the duplication family this file argues against, so the copy is removed rather than corrected: the -set is every `PreToolUse` hook except merge-consent, the BOM guard and the worktree pair, and the -count is in the summary above. +set is the eight `.claude/hooks/` guards enumerated in the standing list below, and the count is in +the summary above. Naming them by event is what made the earlier wording wrong twice — they do not +share one event, and `design-sync-reminder.sh` is registered on **both** `PreToolUse` and `Stop` in +`.claude/settings.json`, so any "every X hook" phrasing double-counts it. **They are now observable but still unproven, and the two words carry different weight.** Observable: every hook records its own execution through `scripts/hook-fire-log.sh`, so "did this hook fire, and @@ -253,8 +269,9 @@ groups, and the split is the point: the first two are backlogs, the third is not - **The eight remaining hook guards** — genuinely unproven, and a real backlog. Four are `PreToolUse` (`pretooluse-agent-model.sh`, `pretooluse-agent-ram.sh`, `pretooluse-bash-guard.sh`, `pretooluse-nav-guard.sh`), two are pre-push (`prepush-clean-worktree-check.sh`, - `prepush-donewhen.sh`), one is pre-commit (`decisions-guard.sh`) and one is `Stop` - (`design-sync-reminder.sh`); they are grouped by their status, not by their event. They are now + `prepush-donewhen.sh`), one is pre-commit (`decisions-guard.sh`) and one is registered on both + `PreToolUse` and `Stop` (`design-sync-reminder.sh`); they are grouped by their status, not by + their event, because the events do not partition them. They are now *observable* (`scripts/hook-fire-log.sh report`, #776), which is a weaker claim than proven and is stated as such above. - **`.husky/commit-msg` and `.husky/pre-commit`** — unproven, and each carries its own clause, so diff --git a/scripts/tests/test_prepush_unsets_git_env.py b/scripts/tests/test_prepush_unsets_git_env.py index 6d9a73aaf..9eb9dd6aa 100644 --- a/scripts/tests/test_prepush_unsets_git_env.py +++ b/scripts/tests/test_prepush_unsets_git_env.py @@ -49,12 +49,27 @@ UNSET_CLAUSE = "unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE" # 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(): - marker = "./.claude/hooks/" - if marker in line: - rest = line.split(marker, 1)[1] - names.append(rest.split()[0].rstrip("|&;")) + stripped = line.strip() + if stripped.startswith("#"): + continue + marker = ".claude/hooks/" + if marker in stripped: + rest = stripped.split(marker, 1)[1] + names.append(rest.split()[0].rstrip("|&;\"'")) return names diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py index 4f3ba8cc9..454353d46 100644 --- a/scripts/tests/test_worktree_ownership_guard.py +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -45,12 +45,28 @@ MARKER_NAME = ".claude-worktree-owner" SESSION_A = "session-aaaa-1111" SESSION_B = "session-bbbb-2222" -# The hooks resolve `scripts/hook-fire-log.sh` from CLAUDE_PROJECT_DIR, falling back to a path -# relative to their own location. A MUTATED copy lives in tmp_path, where that fallback finds -# nothing — so pin the project dir explicitly and both the real and the mutated hook load the same -# library. Otherwise the mutant differs from the subject in a second way and the comparison is not -# about the mutated clause any more. -_ENV = {**os.environ, "CLAUDE_PROJECT_DIR": str(REPO_ROOT)} + +def _env() -> dict: + """The subprocess environment, built PER CALL — never snapshotted at import. + + Two things it must get right. + + `CLAUDE_PROJECT_DIR` is pinned because the hooks resolve `scripts/hook-fire-log.sh` from it, + falling back to a path relative to their own location; a MUTATED copy lives in tmp_path, where + that fallback finds nothing. Without the pin the mutant differs from the subject in a second way + and the comparison stops being about the mutated clause. + + And it is a FUNCTION rather than a module-level dict because `conftest.py`'s autouse + `isolate_hook_fire_log` fixture monkeypatches `ETV_HOOK_FIRE_LOG_DIR` into `os.environ` at test + setup — which happens AFTER this module is imported. A `{**os.environ}` snapshot taken at import + time captures the environment as it was before the fixture ran, so every hook subprocess writes + to the REAL `$HOME/.cache/ersatztv/hook-fire/` log instead of the fixture's tmp dir. That is not + untidiness: it is #776's defect reintroduced in the file that is meant to prove #776's hooks, + and it corrupts the `hook-fire-log.sh report` surface this repo cites as the observability claim + for every guard still graded NONE. Measured before the fix: 1,488 records for the two fake + session ids below in the production log. + """ + return {**os.environ, "CLAUDE_PROJECT_DIR": str(REPO_ROOT)} def _git(cwd: Path, *args: str) -> str: @@ -87,7 +103,7 @@ def _run(hook: Path, payload: dict, cwd: Path) -> tuple[int, bytes]: input=json.dumps(payload).encode(), capture_output=True, cwd=str(cwd), - env=_ENV, + env=_env(), timeout=60, ) return p.returncode, p.stdout @@ -138,6 +154,32 @@ def _denied(out: bytes) -> bool: # ------------------------------------------------------------------------------------------------ +def test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir(): + """The hooks these tests drive must log to the fixture's dir, never the production one. + + `conftest.py`'s autouse `isolate_hook_fire_log` monkeypatches `ETV_HOOK_FIRE_LOG_DIR` into + `os.environ` at test setup. Anything that snapshots `os.environ` at IMPORT time captures the + value from before the fixture ran and silently defeats it — the hooks then append to + `$HOME/.cache/ersatztv/hook-fire/`, which is #776's defect reintroduced inside the file that + proves #776's hooks, corrupting the one surface this repo cites as the observability claim for + every guard still graded NONE. + + It is invisible from the outside: the tests pass either way, because the fire-log library is + fail-open by design. So it needs its own assertion. + """ + env = _env() + isolated = os.environ.get("ETV_HOOK_FIRE_LOG_DIR") + assert isolated, "the autouse isolation fixture did not run; conftest.py is not being loaded" + assert env.get("ETV_HOOK_FIRE_LOG_DIR") == isolated, ( + "the subprocess environment does not carry the isolated log dir, so every hook driven by " + "this file is writing into the production hook-fire log. Build the env per call; do not " + f"snapshot os.environ at import time. env has {env.get('ETV_HOOK_FIRE_LOG_DIR')!r}" + ) + assert "hook-fire" not in str(Path.home() / ".cache") or isolated != str( + Path.home() / ".cache" / "ersatztv" / "hook-fire" + ), "the isolated dir IS the production dir" + + def test_the_marker_hook_really_marks_the_worktree_it_was_told_about(tmp_path): repo = _scratch_repo(tmp_path) wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) -- 2.47.3 From a01def3f10848aefb13bcb90c0f03734a3ee3c24 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 20:10:51 +0200 Subject: [PATCH 09/12] fix(785): the new leak guard's third assertion was a tautology MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir` shipped with assert "hook-fire" not in str(Path.home() / ".cache") or isolated != str(...) Evaluated: `str(Path.home() / ".cache")` is `/Users/timothy/.cache`, which never contains "hook-fire", so the left operand is always True and the `or` never reaches the comparison. The assertion could not fail. It was decoration inside a guard added specifically to stop vacuous checks — the recurring shape this milestone exists to close, committed by its own fix. The first two assertions in that test were doing the real work throughout, so nothing that was proved is now unproved. The third now compares the resolved paths, with the production path DERIVED the way `hook-fire-log.sh` derives it (`${HOME:-/tmp}/.cache/ersatztv/ hook-fire`) rather than restated as a literal that would drift from it. Verified reachable: with `isolated` set to the production path the comparison evaluates False, i.e. the assertion fires. Previously that same input passed. Found by re-reading the fix commit rather than by the reviews — which is the argument for re-reviewing the FIX and not only the original diff. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- scripts/tests/test_worktree_ownership_guard.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py index 454353d46..5ca91804a 100644 --- a/scripts/tests/test_worktree_ownership_guard.py +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -175,9 +175,13 @@ def test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir(): "this file is writing into the production hook-fire log. Build the env per call; do not " f"snapshot os.environ at import time. env has {env.get('ETV_HOOK_FIRE_LOG_DIR')!r}" ) - assert "hook-fire" not in str(Path.home() / ".cache") or isolated != str( - Path.home() / ".cache" / "ersatztv" / "hook-fire" - ), "the isolated dir IS the production dir" + # The path `hook-fire-log.sh` falls back to when ETV_HOOK_FIRE_LOG_DIR is unset, derived the + # same way it derives it rather than restated as a literal. + production = Path(os.environ.get("HOME", "/tmp")) / ".cache" / "ersatztv" / "hook-fire" # noqa: S108 — mirrors hook-fire-log.sh's own ${HOME:-/tmp} + assert Path(isolated).resolve() != production.resolve(), ( + f"the 'isolated' log dir IS the production one ({production}), so the fixture is isolating " + "nothing and this test would pass while the leak continued" + ) def test_the_marker_hook_really_marks_the_worktree_it_was_told_about(tmp_path): -- 2.47.3 From b2b651779a45ecc50058643f9be89de5b064ffe6 Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 20:36:09 +0200 Subject: [PATCH 10/12] =?UTF-8?q?fix(785):=20close=20the=20re-review's=20B?= =?UTF-8?q?LOCKER=20=E2=80=94=20the=20leak=20guard=20pinned=20the=20FIX,?= =?UTF-8?q?=20not=20the=20property?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- docs/guard-inventory.md | 19 +- .../tests/test_build_catalog_check_path.py | 239 +++++++++++++----- scripts/tests/test_prepush_unsets_git_env.py | 8 +- .../tests/test_worktree_ownership_guard.py | 35 ++- 4 files changed, 226 insertions(+), 75 deletions(-) diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 4bf1f4004..9b59f7112 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -200,10 +200,21 @@ fixture set `ETV_HOOK_FIRE_LOG_DIR` *for the test currently running* and that a its own sandbox does not touch the real log. It cannot see another suite that snapshots `os.environ` at IMPORT time — before the autouse fixture runs — and hands that stale mapping to its subprocesses. That suite's hooks then write to `$HOME/.cache/ersatztv/hook-fire/` while every -assertion stays green, because the fire-log library is fail-open. #785 shipped exactly that defect -and measured 1,488 stray records before catching it; the pin is -`test_worktree_ownership_guard.py::test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir`, -which is per-file rather than general. Generalising it is #809. +assertion stays green, because the fire-log library is fail-open. #785 shipped exactly that defect, +inside the file added to prove those very hooks. + +The reproduction is the part worth keeping: restore a module-level `{**os.environ}` snapshot — or +leave the helper correct and point a single `env=` argument at one — then run that file and count +records for its synthetic session ids. **58 per run**, measured identically on macOS/git 2.55 and +Linux/git 2.47.3. (The accumulated total observed before the fix ran to four figures across many +runs; that is an observation rather than a reproducible measurement, so check the per-run figure.) + +The pin is `test_worktree_ownership_guard.py::test_driving_a_hook_LANDS_its_records_in_the_ISOLATED_dir`, +which asserts the EFFECT — records land in the fixture's dir — rather than the shape of the fix. Its +predecessor asserted the helper's return value, and cold review showed that leaves the call site +unguarded: `_env()` correct, one `env=` reverted, all tests green, records still leaking. Pinning to +a hand-written revert rather than to the property is `verify-against-the-REAL-predecessor`. It is +still per-file rather than general; generalising it is #809. **That unproven set carries no number here on purpose.** It restated the count, drifted the moment the BOM guard was regraded, and `test_the_summary_counts_match_the_table` cannot see it — the parser diff --git a/scripts/tests/test_build_catalog_check_path.py b/scripts/tests/test_build_catalog_check_path.py index 64ec9b557..4745d4821 100644 --- a/scripts/tests/test_build_catalog_check_path.py +++ b/scripts/tests/test_build_catalog_check_path.py @@ -23,6 +23,9 @@ import subprocess import sys from pathlib import Path +import pytest +import yaml + import scripts.build_decisions_catalog as bc import scripts.decisions_lib as dl @@ -138,41 +141,63 @@ WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "pr-checks.yml" def _decisions_guard_run_lines(workflow: Path | None = None) -> list[str]: - """Every ACTIVE `run:` line inside the `decisions-guard` job, scoped to that job. + """Every command line the `decisions-guard` job would ACTUALLY execute. - Three things this deliberately does, each closing a way the extractor could report coverage - that is not there: + Parsed with `yaml.safe_load`, not scanned as text. The first version of this scanned lines for + the literal `run:`, and cold review broke it three ways in one pass: a `run: |` block scalar was + invisible (its content lines contain no `run:` — and the sibling "Validate decision lifecycle" + step in this very job uses that form); a job or step switched off with `if: false` or + `continue-on-error: true` still read as wired; and `run:` appearing inside block-scalar *text* + (`echo we no longer run: ...`) was extracted and executed as though it were the step's command. + Two of those are false GREENS. - * it finds the job by its key (`decisions-guard:`) and stops at the next top-level job, so a - matching command in some OTHER job cannot stand in for this one; - * it strips comment lines FIRST, so a step that has been commented out — the ordinary way a - CI check gets switched off during an incident and never switched back — does not read as - wired. `guard-ships-with-mutation-proof` is explicit that the guard being WIRED is not the - guard RUNNING, and a text search over YAML cannot tell those apart unless it excludes - comments; - * it returns the job's run lines rather than a single grepped match, so the caller asserts - the command it needs is among the commands that job actually executes. + `fixing-a-parser-bug-introduces-the-next-one` says a string predicate over structured text needs + several rounds and then fails in a way nobody anticipated. The structure is available — `pyyaml` + is installed by the `script-tests` job and four sibling suites already parse this workflow with + it — so this reads the structure instead of guessing at it. + + "Actually execute" is the operative phrase, and it is three separate conditions: the job exists, + the job is not disabled, and the step is neither disabled nor allowed to fail. A guard that is + present but switched off is the `guard-ships-with-mutation-proof` distinction between a guard + being WIRED and a guard RUNNING. """ - lines = (workflow or WORKFLOW).read_text().splitlines() - try: - start = next(i for i, ln in enumerate(lines) if ln.strip() == "decisions-guard:") - except StopIteration: - raise AssertionError( - "no `decisions-guard:` job in pr-checks.yml. Either the job was renamed or removed — " - "the second is the far more serious finding, and this test is where it surfaces" - ) from None - indent = len(lines[start]) - len(lines[start].lstrip()) - body = [] - for ln in lines[start + 1 :]: - stripped = ln.strip() - if stripped and not stripped.startswith("#"): - cur = len(ln) - len(ln.lstrip()) - if cur <= indent: - break - if stripped.startswith("#"): + doc = yaml.safe_load((workflow or WORKFLOW).read_text()) + jobs = (doc or {}).get("jobs") or {} + job = jobs.get("decisions-guard") + assert job is not None, ( + "no `decisions-guard` job in pr-checks.yml. Either it was renamed or it was removed — the " + f"second is the far more serious finding. Jobs present: {sorted(jobs)}" + ) + assert not _disabled(job), ( + "the `decisions-guard` job is disabled at the job level " + f"(if: {job.get('if')!r}, continue-on-error: {job.get('continue-on-error')!r}), so nothing " + "in it runs — including the catalog guard" + ) + + lines: list[str] = [] + for step in job.get("steps") or []: + if _disabled(step): continue - body.append(ln) - return [ln.split("run:", 1)[1].strip() for ln in body if "run:" in ln] + run = step.get("run") + if not run: + continue + lines.extend(ln.strip() for ln in str(run).splitlines() if ln.strip()) + return lines + + +def _disabled(node: dict) -> bool: + """A job or step that cannot fail the run: switched off, or allowed to fail. + + `if:` is normally an expression string that is truthy at parse time (`github.event_name == ...`) + and only a literal false/`"false"` means "never". `continue-on-error: true` is the subtler one — + the step still runs and still reports, but its failure does not fail the job, so it is not a + gate. + """ + cond = node.get("if", True) + if cond is False or (isinstance(cond, str) and cond.strip().lower() == "false"): + return True + coe = node.get("continue-on-error", False) + return coe is True or (isinstance(coe, str) and coe.strip().lower() == "true") def _ci_check_command(workflow: Path | None = None) -> list[str]: @@ -180,58 +205,138 @@ def _ci_check_command(workflow: Path | None = None) -> list[str]: `testing.guard-derives-population-from-source`: a hand-copied command is a second copy of the workflow that drifts silently, and this test's entire value is that it runs what CI runs. + + The match requires the line to BEGIN with `PYTHONPATH=.` rather than merely to contain the + script name, because a block scalar's text can mention the command without invoking it — an + `echo we no longer run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check` was + extracted and executed by the previous version, which is a guard proving a command nobody runs. """ runs = _decisions_guard_run_lines(workflow) - matches = [r for r in runs if "build_decisions_catalog.py" in r and "--check" in r] + matches = [r for r in runs if r.startswith("PYTHONPATH=.") and "build_decisions_catalog.py" in r and "--check" in r] assert matches, ( "the `decisions-guard` job has no ACTIVE step running " "`build_decisions_catalog.py --check`. The catalog guard has stopped running in CI — that " - f"is the finding, not this test's failure. Active run lines in that job: {runs}" + f"is the finding, not this test's failure. Active command lines in that job: {runs}" ) - assert len(matches) == 1, f"expected exactly one such step, found {matches}" - body = matches[0] - assert body.startswith("PYTHONPATH=."), ( - f"the decisions-guard invocation no longer starts with PYTHONPATH=.: {body!r}" - ) - return ["bash", "-c", body.replace("python3", sys.executable, 1)] + assert len(matches) == 1, f"expected exactly one such command, found {matches}" + return ["bash", "-c", matches[0].replace("python3", sys.executable, 1)] -def test_the_extractor_rejects_a_COMMENTED_OUT_step(): - """A step commented out is a guard that has stopped running, and must not read as wired. +@pytest.mark.parametrize( + "label,mutate", + [ + ( + "the step is COMMENTED OUT", + lambda s: s.replace( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + " # - name: Active catalog in sync\n" + " # run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + 1, + ), + ), + ( + "the step is DELETED", + lambda s: s.replace( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + "", + 1, + ), + ), + ( + "the step is switched off with `if: false`", + lambda s: s.replace( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + " - name: Active catalog in sync\n" + " if: false\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + 1, + ), + ), + ( + "the step is allowed to fail with `continue-on-error: true`", + lambda s: s.replace( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + " - name: Active catalog in sync\n" + " continue-on-error: true\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + 1, + ), + ), + ( + "the whole JOB is switched off with `if: false`", + lambda s: s.replace( + " decisions-guard:\n name: decisions lifecycle\n runs-on: small\n" + " if: github.event_name == 'pull_request'\n", + " decisions-guard:\n name: decisions lifecycle\n runs-on: small\n if: false\n", + 1, + ), + ), + ( + "the command survives only as TEXT inside another step's block scalar", + lambda s: s.replace( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + " - name: Note\n" + " run: |\n" + " echo we no longer run: PYTHONPATH=. python3 " + "scripts/build_decisions_catalog.py --check\n", + 1, + ), + ), + ], +) +def test_a_guard_that_stopped_RUNNING_is_DETECTED(tmp_path, label, mutate): + """Six ways the catalog guard can stop gating, each of which must be caught. - Without this the subprocess proof below stays green for as long as the text survives anywhere in - the file — including inside the `#` that switched it off. That is the shape - `guard-ships-with-mutation-proof` names as "the guard being WIRED is not the guard RUNNING". + Commenting out is one of them and was the only one the first version detected. `if: false`, + `continue-on-error: true` and a job-level `if: false` all leave the text in place, and a mention + inside another step's block scalar leaves it in place AND parseable — that one had the extractor + executing a command out of an `echo`. """ - active = _decisions_guard_run_lines() - assert any("build_decisions_catalog.py" in r for r in active), "precondition: it is wired today" - for r in active: - assert not r.lstrip().startswith("#"), f"a comment line survived the strip: {r!r}" - raw = WORKFLOW.read_text() - commented = raw.replace( + mutated = mutate(raw) + assert mutated != raw, f"the mutation for {label!r} matched nothing; RETARGET it" + + alt = tmp_path / "pr-checks.yml" + alt.write_text(mutated) + + try: + _ci_check_command(alt) + except AssertionError: + return + raise AssertionError( + f"the extractor still reported the catalog guard as running when {label}. The subprocess " + "proof below would stay green over a guard CI does not gate on." + ) + + +def test_the_extractor_accepts_the_step_in_BLOCK_SCALAR_form(tmp_path): + """The negative control for the six above: a legitimate reformatting must NOT be flagged. + + `run: |` is idiomatic in this very job (the sibling "Validate decision lifecycle" step uses it), + and the text-scanning version reported that form as "the guard has stopped running in CI" — a + false red whose message asserts a serious regression that has not happened. A detector that + cannot tell a reformat from a removal trains its readers to ignore it. + """ + raw = WORKFLOW.read_text() + mutated = raw.replace( " - name: Active catalog in sync\n" " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - " # - name: Active catalog in sync\n" - " # run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + " - name: Active catalog in sync\n" + " run: |\n" + " PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", 1, ) - assert commented != raw, "retarget this mutation; the step's text has changed" + assert mutated != raw, "retarget this reformatting; the step's text has changed" - import tempfile - - with tempfile.TemporaryDirectory() as d: - alt = Path(d) / "pr-checks.yml" - alt.write_text(commented) - raised = False - try: - _ci_check_command(alt) - except AssertionError: - raised = True - assert raised, ( - "the extractor still found the command after the step was commented out, so the subprocess " - "proof would stay green over a guard CI no longer runs" - ) + alt = tmp_path / "pr-checks.yml" + alt.write_text(mutated) + cmd = _ci_check_command(alt) + assert "build_decisions_catalog.py" in cmd[-1], cmd def test_CLI_subprocess_exits_nonzero_on_a_stale_catalog(tmp_path): diff --git a/scripts/tests/test_prepush_unsets_git_env.py b/scripts/tests/test_prepush_unsets_git_env.py index 9eb9dd6aa..c7173a87a 100644 --- a/scripts/tests/test_prepush_unsets_git_env.py +++ b/scripts/tests/test_prepush_unsets_git_env.py @@ -63,9 +63,11 @@ def _hook_calls(text: str) -> list[str]: """ names = [] for line in text.splitlines(): - stripped = line.strip() - if stripped.startswith("#"): - continue + # 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] diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py index 5ca91804a..61b6cf1c7 100644 --- a/scripts/tests/test_worktree_ownership_guard.py +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -184,6 +184,39 @@ def test_the_subprocess_env_CARRIES_the_isolated_hook_fire_log_dir(): ) +def test_driving_a_hook_LANDS_its_records_in_the_ISOLATED_dir(tmp_path): + """The invariant, asserted at the EFFECT rather than at the helper that is supposed to produce it. + + `test_the_subprocess_env_CARRIES_...` above checks `_env()`'s return value, and that is not the + same claim: `_env()` can be perfectly correct while a call site passes something else. Cold + review demonstrated exactly that — restore the module-level snapshot and change one `env=_env()` + back to `env=_ENV`, and all thirteen tests pass while 54 records leak into the real log. The + guard was pinned to the shape of the fix instead of to the property, which is + `verify-against-the-REAL-predecessor`: a hand-written revert is not the code a future tidy-up + produces. + + So this drives a real hook through the real `_run()` and asserts the records landed where the + fixture put them. It cannot pass while anything in this file logs to the production dir, + whatever the mechanism. + """ + isolated = Path(os.environ["ETV_HOOK_FIRE_LOG_DIR"]) + before = {p.name for p in isolated.glob("*.jsonl")} if isolated.exists() else set() + + repo = _scratch_repo(tmp_path) + wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) + rc, out = _run(GUARD, _commit_payload(SESSION_B, wt), wt) + + # Anti-vacuity: if the hook decided nothing, it may simply have had nothing to log. + assert rc == 0 and _denied(out), f"the hook reached no decision, so 'records landed' would prove nothing: {out!r}" + + after = {p.name for p in isolated.glob("*.jsonl")} if isolated.exists() else set() + assert after > before, ( + f"driving two hooks added no record to the isolated log dir {isolated}. Either the " + "instrumentation stopped firing, or these hooks are logging somewhere else — and the only " + "somewhere else is the production log this file must never touch" + ) + + def test_the_marker_hook_really_marks_the_worktree_it_was_told_about(tmp_path): repo = _scratch_repo(tmp_path) wt = _add_worktree(repo, "wt-a", MARKER_HOOK, SESSION_A) @@ -300,7 +333,7 @@ def test_a_git_C_into_a_foreign_worktree_is_DENIED_from_the_main_tree(tmp_path): # ------------------------------------------------------------------------------------------------ -# MUTATION PROOFS — three clauses, one per thing the mechanism hangs on +# MUTATION PROOFS — four clauses, one per thing the mechanism hangs on # # Each mutant is an isolated copy with ONE clause disarmed, and each test asserts the UNMUTATED pair # reaches the opposite decision on the same fixture FIRST. Without that positive control a mutation -- 2.47.3 From 52ed319c8a45072538c2a37d86e0be465a885fee Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 21:07:41 +0200 Subject: [PATCH 11/12] fix(785): withdraw the line-level workflow heuristic; land the doc fix I said I had landed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fourth cold review, scoped to the previous commit. Three findings, one of which is the worst kind. HIGH — a doc fix asserted as landed was not in the diff. `e4a14e282`'s message says the `test_hook_fire_log.py` "TWO clauses" correction was "verified against the files"; the file still said TWO. Cause: both doc edits were applied in one script that wrote once at the end, the second edit's `assert` fired, and `write_text` never ran — so the first edit was discarded silently while I reported it as done. Every edit here is applied, written and re-read individually. The correction itself was right and is now landed: that file proves THREE clauses — coverage, transparency, placement — and the row's Blocks column already claims two of them; the single Proof ref is a column limit, not a coverage statement. All four cited test names verified to exist. Also fixed a second instance of the same stale subject: the ranked entry still said "Three clauses were disarmed" for the worktree pair, and listed three, after the fourth was added two commits ago. MEDIUM — the line-level extractor was round three, and review found round four's defect on schedule: run: | cat <<'EOF' > /dev/null PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check EOF The matched line is heredoc DATA. The extractor reported the guard as running and the proof executed a command CI does not — the exact class the previous commit claimed to close, one level deeper. Deciding which lines of a shell script execute requires parsing shell, and this repo has lost that argument repeatedly. So the line-level heuristic is WITHDRAWN rather than patched a fourth time. The step's WHOLE `run` script is handed to bash, as the runner does. The heredoc then runs, writes to /dev/null, checks nothing and exits 0 — so the stale-catalog case fails to redden and the proof reports the defect instead of stepping around it. Ambiguity resolves by execution. That also removed a false RED the reviewer found and two I had not: a backslash continuation, a leading `set -euo pipefail`, and the command wrapped in an `if` block are all accepted now, and are pinned as parametrised negative controls. The disablement cases are eight, and assert on the whole guarantee (fresh passes AND stale fails) rather than on whether extraction raises, because the failure surfaces in different places. MEDIUM — `if: ${{ false }}` read as wired. `_falsey` compared literals only. The unwrap is a regex over the whole value, not `.strip("${{ }}")`, which strips a character SET and would turn `"false}"` into `"false"` while reading as though it removed a wrapper. MEDIUM — the leak guard's docstring claimed it "cannot pass while anything in this file logs to the production dir, whatever the mechanism". False, and review demonstrated it: add a second launcher beside `_run()` that passes a stale environment and point the mutation tests at it — 14 passed, 18 records leaked. I reproduced that before correcting. It guards the launch path it drives; every hook in the file goes through one helper today, which is what makes it sufficient there and not a general property. Both the docstring and the inventory now say so, and #809 carries the general form. Two figures in the previous message were also wrong. "13 tests pass and 58 records leak" conflates two runs — the pre-fix 13-test file leaks 54, and 58 is the 14-test file at head. And "four sibling suites already parse this workflow" is two: the other two parse docker-build.yml. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- docs/guard-inventory.md | 30 +- .../tests/test_build_catalog_check_path.py | 283 ++++++++++++------ .../tests/test_worktree_ownership_guard.py | 12 +- 3 files changed, 221 insertions(+), 104 deletions(-) diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md index 9b59f7112..908270137 100644 --- a/docs/guard-inventory.md +++ b/docs/guard-inventory.md @@ -189,10 +189,15 @@ lines flagged wrongly — and cold review then constructed more of both (`[[ "$x a backslash continuation, a `>` inside a quoted string). Deleted rather than patched a fifth time, on the same reasoning as the vocabulary-parity withdrawal above. -**`test_hook_fire_log.py` asserts TWO clauses**, and each has its own proof: the coverage clause -(every hook reports that it fired) via `test_a_hook_that_LOSES_its_instrumentation_is_DETECTED`, and -the placement clause (`etv_hook_fire_begin` must precede the stdin read) via -`test_begin_placed_AFTER_the_stdin_read_is_DETECTED`. Its row cites the first. +**`test_hook_fire_log.py` proves THREE clauses, and its row claims two of them.** Coverage (every +hook reports that it fired) via `test_a_hook_that_LOSES_its_instrumentation_is_DETECTED`; +transparency (the wrapper changes nothing the harness can see) via +`test_instrumentation_changes_NOTHING_the_harness_can_see` with its mutation proof +`test_DELETING_the_replay_makes_the_differential_go_RED`; and placement (`etv_hook_fire_begin` must +precede the stdin read) via `test_begin_placed_AFTER_the_stdin_read_is_DETECTED`. The row's `Blocks` +column covers the first two — "stops reporting that it fired, **or whose reporting changes what the +harness sees**" — and the `Proof ref` column holds one ref because the column holds one, not because +the second is unproven. Placement is proved and unclaimed. **Its `test_the_suite_does_not_write_to_the_PRODUCTION_log` is narrower than its docstring**, which says `conftest.py` "must isolate every test, not just this file's". What it checks is that the @@ -213,8 +218,13 @@ The pin is `test_worktree_ownership_guard.py::test_driving_a_hook_LANDS_its_reco which asserts the EFFECT — records land in the fixture's dir — rather than the shape of the fix. Its predecessor asserted the helper's return value, and cold review showed that leaves the call site unguarded: `_env()` correct, one `env=` reverted, all tests green, records still leaking. Pinning to -a hand-written revert rather than to the property is `verify-against-the-REAL-predecessor`. It is -still per-file rather than general; generalising it is #809. +a hand-written revert rather than to the property is `verify-against-the-REAL-predecessor`. + +It is still narrower than the property: it guards the launch path it drives. A second launcher in +the same file that passed a stale environment would leave it green — measured, 18 records — because +the hooks it drives would still log correctly. Every hook in that file goes through one helper +today, which is what makes it sufficient there. Generalising it is #809, and the reason that is hard +is that the obvious version races against a real session's hooks firing during the run. **That unproven set carries no number here on purpose.** It restated the count, drifted the moment the BOM guard was regraded, and `test_the_summary_counts_match_the_table` cannot see it — the parser @@ -245,10 +255,12 @@ because two of them turned out to be worse than the ranking predicted.** was allowed in silence. `od` now. The lesson for the rows below: an unproven guard is not merely untested, it is a guard whose *current* behaviour nobody has established. 2. ~~`pretooluse-worktree-guard.sh` + `posttooluse-worktree-marker.sh`~~ — **proven as a pair.** - Three clauses were disarmed and witnessed red: the guard's marker read, the guard's ownership + Four clauses were disarmed and witnessed red: the guard's marker read; the guard's ownership comparison (inverted, because disarming it the other way only makes the guard deny more and every - deny assertion stays green), and — the one that could not exist while the halves were tested - apart — the *marker hook's write*, asserted against the *guard's* decision. Both hooks are + deny assertion stays green); the `commit|merge` alternation, whose `merge` half every other case + in the file left untested while guarding the plumbing-merge path; and — the one that could not + exist while the halves were tested apart — the *marker hook's write*, asserted against the + *guard's* decision. Both hooks are deliberately fail-open, so an absent mechanism and a working one produce the identical "commit allowed". 3. ~~`.husky/pre-push:11`'s `unset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE`~~ — **proven, and the case diff --git a/scripts/tests/test_build_catalog_check_path.py b/scripts/tests/test_build_catalog_check_path.py index 4745d4821..2ed5cafd8 100644 --- a/scripts/tests/test_build_catalog_check_path.py +++ b/scripts/tests/test_build_catalog_check_path.py @@ -19,6 +19,7 @@ green `main()` behind dead wiring). from __future__ import annotations import importlib.util +import re import subprocess import sys from pathlib import Path @@ -140,26 +141,32 @@ def _copy_tree(src: Path, dst: Path) -> None: WORKFLOW = REPO_ROOT / ".gitea" / "workflows" / "pr-checks.yml" -def _decisions_guard_run_lines(workflow: Path | None = None) -> list[str]: - """Every command line the `decisions-guard` job would ACTUALLY execute. +def _active_runs(workflow: Path | None = None) -> list[str]: + """The `run` script of every step the `decisions-guard` job would ACTUALLY execute. - Parsed with `yaml.safe_load`, not scanned as text. The first version of this scanned lines for - the literal `run:`, and cold review broke it three ways in one pass: a `run: |` block scalar was - invisible (its content lines contain no `run:` — and the sibling "Validate decision lifecycle" - step in this very job uses that form); a job or step switched off with `if: false` or - `continue-on-error: true` still read as wired; and `run:` appearing inside block-scalar *text* - (`echo we no longer run: ...`) was extracted and executed as though it were the step's command. - Two of those are false GREENS. + Parsed with `yaml.safe_load`, and returned WHOLE — not split into lines. Both choices are + scar tissue. - `fixing-a-parser-bug-introduces-the-next-one` says a string predicate over structured text needs - several rounds and then fails in a way nobody anticipated. The structure is available — `pyyaml` - is installed by the `script-tests` job and four sibling suites already parse this workflow with - it — so this reads the structure instead of guessing at it. + Text-scanning for `run:` was round one, and cold review broke it three ways: a `run: |` block + scalar was invisible; a job or step switched off still read as wired; and `run:` inside + block-scalar *text* was extracted and executed. Round two parsed the YAML and matched a LINE + beginning with `PYTHONPATH=.` — and review broke that too, with a heredoc: - "Actually execute" is the operative phrase, and it is three separate conditions: the job exists, - the job is not disabled, and the step is neither disabled nor allowed to fail. A guard that is - present but switched off is the `guard-ships-with-mutation-proof` distinction between a guard - being WIRED and a guard RUNNING. + run: | + cat <<'EOF' > /dev/null + PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check + EOF + + The matched line is heredoc DATA. The extractor reported the guard as running, and the proof + executed a command CI does not. Deciding which lines of a shell script are executed requires + parsing shell, and `fixing-a-parser-bug-introduces-the-next-one` is explicit that this repo has + lost that argument repeatedly — a regex over shell is not a parser, and round four would find + round five. + + So the line-level heuristic is WITHDRAWN. The whole `run` script is handed to `bash`, exactly as + the runner does. The heredoc above then runs, writes to `/dev/null`, checks nothing, and exits + 0 — so the stale-catalog case fails to redden and the proof reports the defect instead of + stepping around it. No shell parsing, and the ambiguous cases resolve by execution. """ doc = yaml.safe_load((workflow or WORKFLOW).read_text()) jobs = (doc or {}).get("jobs") or {} @@ -173,53 +180,110 @@ def _decisions_guard_run_lines(workflow: Path | None = None) -> list[str]: f"(if: {job.get('if')!r}, continue-on-error: {job.get('continue-on-error')!r}), so nothing " "in it runs — including the catalog guard" ) + return [str(step["run"]) for step in (job.get("steps") or []) if step.get("run") and not _disabled(step)] - lines: list[str] = [] - for step in job.get("steps") or []: - if _disabled(step): - continue - run = step.get("run") - if not run: - continue - lines.extend(ln.strip() for ln in str(run).splitlines() if ln.strip()) - return lines + +def _unwrap(value: str) -> str: + """Strip an `${{ ... }}` expression wrapper, if present, and lowercase. + + Written as a regex over the WHOLE value rather than `.strip("${{ }}")`, which strips a character + SET — it would turn `"false}"` into `"false"` and reads as though it removed a wrapper it never + checked for. + """ + inner = value.strip() + m = re.fullmatch(r"\$\{\{(.*)\}\}", inner, flags=re.DOTALL) + if m: + inner = m.group(1) + return inner.strip().lower() + + +def _falsey(value) -> bool: + """A literal false, however this workflow dialect spells it. + + `if: false`, `if: "false"` and `if: ${{ false }}` all mean never. The middle and last are the + ones a text comparison misses; the last was a live false green — `${{ false }}` is the ordinary + spelling in Actions-flavoured YAML, and it read as wired. + + An expression that is merely falsy AT RUN TIME (`if: ${{ github.event_name == 'x' }}`) is not + decidable here and is deliberately not guessed at. + """ + if value is False: + return True + if not isinstance(value, str): + return False + return _unwrap(value) == "false" + + +def _truthy_literal(value) -> bool: + if value is True: + return True + if not isinstance(value, str): + return False + return _unwrap(value) == "true" def _disabled(node: dict) -> bool: """A job or step that cannot fail the run: switched off, or allowed to fail. - `if:` is normally an expression string that is truthy at parse time (`github.event_name == ...`) - and only a literal false/`"false"` means "never". `continue-on-error: true` is the subtler one — - the step still runs and still reports, but its failure does not fail the job, so it is not a - gate. + `continue-on-error: true` is the subtle one — the step still runs and still reports, but its + failure does not fail the job, so it is not a gate. """ - cond = node.get("if", True) - if cond is False or (isinstance(cond, str) and cond.strip().lower() == "false"): - return True - coe = node.get("continue-on-error", False) - return coe is True or (isinstance(coe, str) and coe.strip().lower() == "true") + return _falsey(node.get("if", True)) or _truthy_literal(node.get("continue-on-error", False)) def _ci_check_command(workflow: Path | None = None) -> list[str]: - """The command the `decisions-guard` job runs, DERIVED from the workflow, not restated here. + """The catalog step's script, DERIVED from the workflow and executed whole. `testing.guard-derives-population-from-source`: a hand-copied command is a second copy of the - workflow that drifts silently, and this test's entire value is that it runs what CI runs. - - The match requires the line to BEGIN with `PYTHONPATH=.` rather than merely to contain the - script name, because a block scalar's text can mention the command without invoking it — an - `echo we no longer run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check` was - extracted and executed by the previous version, which is a guard proving a command nobody runs. + workflow that drifts silently, and this test's whole value is that it runs what CI runs. """ - runs = _decisions_guard_run_lines(workflow) - matches = [r for r in runs if r.startswith("PYTHONPATH=.") and "build_decisions_catalog.py" in r and "--check" in r] + runs = _active_runs(workflow) + matches = [r for r in runs if "build_decisions_catalog.py" in r and "--check" in r] assert matches, ( - "the `decisions-guard` job has no ACTIVE step running " + "the `decisions-guard` job has no ACTIVE step mentioning " "`build_decisions_catalog.py --check`. The catalog guard has stopped running in CI — that " - f"is the finding, not this test's failure. Active command lines in that job: {runs}" + f"is the finding, not this test's failure. Active step scripts in that job: {runs}" ) - assert len(matches) == 1, f"expected exactly one such command, found {matches}" - return ["bash", "-c", matches[0].replace("python3", sys.executable, 1)] + assert len(matches) == 1, f"expected exactly one such step, found {matches}" + script = matches[0] + assert "${{" not in script, ( + "the catalog step's script interpolates an Actions expression, which cannot be expanded " + f"outside the runner — this proof would be executing something else: {script!r}" + ) + return ["bash", "-c", script.replace("python3", sys.executable)] + + +def _proof_holds(workflow: Path, repo: Path) -> bool: + """Does the whole guarantee hold — fresh corpus passes AND stale corpus fails? + + The disablement cases below assert on THIS rather than on whether extraction raises, because + the ways a guard can stop gating do not all surface at the same place. Deletion and disablement + surface as a failed extraction; a heredoc or an `echo` surfaces only when the script is run and + reports success over a stale catalog. One predicate covers both. + """ + try: + cmd = _ci_check_command(workflow) + except AssertionError: + return False + + readme = repo / "docs" / "decisions" / "README.md" + original = readme.read_text(encoding="utf-8") + try: + readme.write_text(original, encoding="utf-8") + fresh = subprocess.run(cmd, cwd=str(repo), capture_output=True, text=True, timeout=60) + if fresh.returncode != 0: + return False + readme.write_text(original + "\nSTALE INJECTED LINE\n", encoding="utf-8") + stale = subprocess.run(cmd, cwd=str(repo), capture_output=True, text=True, timeout=60) + return stale.returncode != 0 + finally: + readme.write_text(original, encoding="utf-8") + + +_STEP = ( + " - name: Active catalog in sync\n" + " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n" +) @pytest.mark.parametrize( @@ -228,29 +292,27 @@ def _ci_check_command(workflow: Path | None = None) -> list[str]: ( "the step is COMMENTED OUT", lambda s: s.replace( - " - name: Active catalog in sync\n" - " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + _STEP, " # - name: Active catalog in sync\n" " # run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", 1, ), ), + ("the step is DELETED", lambda s: s.replace(_STEP, "", 1)), ( - "the step is DELETED", + "the step is switched off with `if: false`", lambda s: s.replace( - " - name: Active catalog in sync\n" + _STEP, + " - name: Active catalog in sync\n if: false\n" " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - "", 1, ), ), ( - "the step is switched off with `if: false`", + "the step is switched off with `if: ${{ false }}`", lambda s: s.replace( - " - name: Active catalog in sync\n" - " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - " - name: Active catalog in sync\n" - " if: false\n" + _STEP, + " - name: Active catalog in sync\n if: ${{ false }}\n" " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", 1, ), @@ -258,10 +320,8 @@ def _ci_check_command(workflow: Path | None = None) -> list[str]: ( "the step is allowed to fail with `continue-on-error: true`", lambda s: s.replace( - " - name: Active catalog in sync\n" - " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - " - name: Active catalog in sync\n" - " continue-on-error: true\n" + _STEP, + " - name: Active catalog in sync\n continue-on-error: true\n" " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", 1, ), @@ -276,26 +336,34 @@ def _ci_check_command(workflow: Path | None = None) -> list[str]: ), ), ( - "the command survives only as TEXT inside another step's block scalar", + "the command survives only as TEXT in another step's `echo`", lambda s: s.replace( - " - name: Active catalog in sync\n" - " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - " - name: Note\n" - " run: |\n" + _STEP, + " - name: Note\n run: |\n" " echo we no longer run: PYTHONPATH=. python3 " "scripts/build_decisions_catalog.py --check\n", 1, ), ), + ( + "the command survives only as HEREDOC DATA", + lambda s: s.replace( + _STEP, + " - name: Note\n run: |\n" + " cat <<'EOF' > /dev/null\n" + " PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n" + " EOF\n", + 1, + ), + ), ], ) def test_a_guard_that_stopped_RUNNING_is_DETECTED(tmp_path, label, mutate): - """Six ways the catalog guard can stop gating, each of which must be caught. + """Eight ways the catalog guard can stop gating, each of which must be caught. - Commenting out is one of them and was the only one the first version detected. `if: false`, - `continue-on-error: true` and a job-level `if: false` all leave the text in place, and a mention - inside another step's block scalar leaves it in place AND parseable — that one had the extractor - executing a command out of an `echo`. + Commenting out is one of them and was the only one the first version detected. The last two + leave the command in the file, parseable and even matchable — as `echo` argument and as heredoc + data — which is why the proof executes the step's whole script instead of a line lifted out of it. """ raw = WORKFLOW.read_text() mutated = mutate(raw) @@ -303,40 +371,69 @@ def test_a_guard_that_stopped_RUNNING_is_DETECTED(tmp_path, label, mutate): alt = tmp_path / "pr-checks.yml" alt.write_text(mutated) + repo = tmp_path / "repo-copy" + repo.mkdir() + _seed_decisions_copy(repo) - try: - _ci_check_command(alt) - except AssertionError: - return - raise AssertionError( - f"the extractor still reported the catalog guard as running when {label}. The subprocess " - "proof below would stay green over a guard CI does not gate on." + assert not _proof_holds(alt, repo), ( + f"the catalog guard still reported as gating when {label}. CI would run nothing and this " + "file would report full coverage." ) -def test_the_extractor_accepts_the_step_in_BLOCK_SCALAR_form(tmp_path): - """The negative control for the six above: a legitimate reformatting must NOT be flagged. +@pytest.mark.parametrize( + "label,replacement", + [ + ( + "block scalar", + " - name: Active catalog in sync\n run: |\n" + " PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + ), + ( + "backslash continuation", + " - name: Active catalog in sync\n run: |\n" + " PYTHONPATH=. python3 \\\n" + " scripts/build_decisions_catalog.py --check\n", + ), + ( + "a leading `set -euo pipefail`", + " - name: Active catalog in sync\n run: |\n" + " set -euo pipefail\n" + " PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", + ), + ( + "wrapped in a shell block", + " - name: Active catalog in sync\n run: |\n" + " if true; then\n" + " PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n" + " fi\n", + ), + ], +) +def test_the_step_may_be_REFORMATTED_without_being_flagged(tmp_path, label, replacement): + """The negative controls for the eight above: a legitimate rewrite must NOT be flagged. `run: |` is idiomatic in this very job (the sibling "Validate decision lifecycle" step uses it), and the text-scanning version reported that form as "the guard has stopped running in CI" — a - false red whose message asserts a serious regression that has not happened. A detector that - cannot tell a reformat from a removal trains its readers to ignore it. + false red whose message asserts a regression that has not happened. A detector that cannot tell + a reformat from a removal trains its readers to ignore it, which is the whole subject of #806. + + A line-matching version had to be taught each of these shapes one at a time, and the + continuation case defeated two rounds of it. Executing the script gets all four for free: the + question "is the guard still gating" is answered by running it, not by recognising how it was + written. """ raw = WORKFLOW.read_text() - mutated = raw.replace( - " - name: Active catalog in sync\n" - " run: PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - " - name: Active catalog in sync\n" - " run: |\n" - " PYTHONPATH=. python3 scripts/build_decisions_catalog.py --check\n", - 1, - ) + mutated = raw.replace(_STEP, replacement, 1) assert mutated != raw, "retarget this reformatting; the step's text has changed" alt = tmp_path / "pr-checks.yml" alt.write_text(mutated) - cmd = _ci_check_command(alt) - assert "build_decisions_catalog.py" in cmd[-1], cmd + repo = tmp_path / "repo-copy" + repo.mkdir() + _seed_decisions_copy(repo) + + assert _proof_holds(alt, repo), f"a legitimate reformat ({label}) was reported as a removal" def test_CLI_subprocess_exits_nonzero_on_a_stale_catalog(tmp_path): diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py index 61b6cf1c7..60f6dab12 100644 --- a/scripts/tests/test_worktree_ownership_guard.py +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -196,8 +196,16 @@ def test_driving_a_hook_LANDS_its_records_in_the_ISOLATED_dir(tmp_path): produces. So this drives a real hook through the real `_run()` and asserts the records landed where the - fixture put them. It cannot pass while anything in this file logs to the production dir, - whatever the mechanism. + fixture put them. + + ITS SCOPE, stated because the first version of this docstring claimed more than it delivers: it + guards THE LAUNCH PATH IT DRIVES, not the file. Cold review demonstrated the gap — add a second + launcher alongside `_run()` that passes a stale snapshot and point the mutation tests at it, and + this test stays green while 18 records leak, because the hooks IT drives still log correctly. + Every hook in this file goes through `_run()` today, which is what makes the guard sufficient + HERE and not a general property. The general form is a suite-level check, tracked in #809; the + reason it is hard is that the obvious version — diff the production log around each test — races + against a real session's hooks firing concurrently. """ isolated = Path(os.environ["ETV_HOOK_FIRE_LOG_DIR"]) before = {p.name for p in isolated.glob("*.jsonl")} if isolated.exists() else set() -- 2.47.3 From 55a7df226e368f4d78c9e3015296aacc3767d8ea Mon Sep 17 00:00:00 2001 From: Timothy Date: Fri, 21 Aug 2026 21:24:35 +0200 Subject: [PATCH 12/12] test(785): pin the workflow's TRIGGERS, and sweep the retracted figure by subject MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fifth cold review returned MERGEABLE, having reproduced all five of the previous commit's measurements exactly and verified by execution that all twelve parametrised cases decide for their labelled reason rather than incidentally. Three residual items, none of which was coverage claimed and not delivered. The `on:` trigger was an uncovered channel. `_active_runs` started at `jobs:`, so a workflow whose triggers no longer include `pull_request` left a perfectly healthy `decisions-guard` job gating nothing while the proof stayed green. Nothing claimed to cover it, which is why it was LOW rather than a false coverage claim — but it is two lines and it is now the ninth disablement case. `on` parses to the BOOLEAN True in YAML 1.1, not the string `"on"`, so the key is looked up both ways. Verified which branch is live rather than assuming: pyyaml produces `True` here, so a lookup of `"on"` alone would have yielded an empty trigger set and fired the assertion on a correct workflow — a false red that the anti-vacuity check (the unmodified workflow must still hold) is what catches. `script.replace("python3", sys.executable)` was unbounded, having been `count=1` on a single extracted line before the withdrawal. It rewrote every occurrence including inside a path, so a step spelled `/usr/bin/python3` would have become nonsense and failed with a red blaming the catalog guard. Now substituted only where `python3` is a bare command word. And the retracted "1,488 records" figure survived in a second place — the leak guard's docstring — because the previous sweep went by file rather than by subject. Replaced with the reproduction and the figure that survives it, the same wording the inventory carries. `stale-comments-sweep-by-subject-not-verb`. Two channels remain uncovered and are deliberately not guessed at: a job `if:` whose expression is false only at run time, and `continue-on-error` as a non-literal expression. `_falsey`'s docstring already declines these as undecidable outside the runner, and no row, test name or docstring claims them. Refs: #785 Co-Authored-By: Claude Opus 5 (1M context) --- .../tests/test_build_catalog_check_path.py | 22 ++++++++++++++++++- .../tests/test_worktree_ownership_guard.py | 5 +++-- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/scripts/tests/test_build_catalog_check_path.py b/scripts/tests/test_build_catalog_check_path.py index 2ed5cafd8..8a0ae5fde 100644 --- a/scripts/tests/test_build_catalog_check_path.py +++ b/scripts/tests/test_build_catalog_check_path.py @@ -169,6 +169,18 @@ def _active_runs(workflow: Path | None = None) -> list[str]: stepping around it. No shell parsing, and the ambiguous cases resolve by execution. """ doc = yaml.safe_load((workflow or WORKFLOW).read_text()) + + # The workflow's TRIGGERS, before its jobs. A `decisions-guard` job that is perfectly healthy + # gates nothing if the workflow stopped running on pull requests, and starting at `jobs:` cannot + # see that. Note `on` parses to the boolean True in YAML 1.1 (the Norway problem's cousin), so + # the key is looked up both ways rather than assumed. + triggers = (doc or {}).get("on", (doc or {}).get(True)) or {} + names = set(triggers) if isinstance(triggers, dict) else {triggers} if isinstance(triggers, str) else set(triggers) + assert "pull_request" in names, ( + "pr-checks.yml no longer runs on `pull_request`, so NO gate in it — including the catalog " + f"guard — fires on a PR. Triggers found: {sorted(str(n) for n in names)}" + ) + jobs = (doc or {}).get("jobs") or {} job = jobs.get("decisions-guard") assert job is not None, ( @@ -250,7 +262,11 @@ def _ci_check_command(workflow: Path | None = None) -> list[str]: "the catalog step's script interpolates an Actions expression, which cannot be expanded " f"outside the runner — this proof would be executing something else: {script!r}" ) - return ["bash", "-c", script.replace("python3", sys.executable)] + # Substitute the interpreter only where `python3` is a bare command word. A plain + # `str.replace` rewrites EVERY occurrence, including inside a path — `/usr/bin/python3` would + # become `/usr/bin//bin/python3` and fail with ENOENT, a red blaming the catalog guard for + # something this line did. + return ["bash", "-c", re.sub(r"(? bool: @@ -345,6 +361,10 @@ _STEP = ( 1, ), ), + ( + "the WORKFLOW no longer runs on pull requests", + lambda s: s.replace("on:\n pull_request:", "on:\n workflow_dispatch:", 1), + ), ( "the command survives only as HEREDOC DATA", lambda s: s.replace( diff --git a/scripts/tests/test_worktree_ownership_guard.py b/scripts/tests/test_worktree_ownership_guard.py index 60f6dab12..9a461c352 100644 --- a/scripts/tests/test_worktree_ownership_guard.py +++ b/scripts/tests/test_worktree_ownership_guard.py @@ -63,8 +63,9 @@ def _env() -> dict: to the REAL `$HOME/.cache/ersatztv/hook-fire/` log instead of the fixture's tmp dir. That is not untidiness: it is #776's defect reintroduced in the file that is meant to prove #776's hooks, and it corrupts the `hook-fire-log.sh report` surface this repo cites as the observability claim - for every guard still graded NONE. Measured before the fix: 1,488 records for the two fake - session ids below in the production log. + for every guard still graded NONE. The reproduction, rather than a figure whose evidence has + since been deleted: reintroduce the snapshot and run this file, then count records for the two + synthetic session ids below — 58 per run, on macOS and Linux alike. """ return {**os.environ, "CLAUDE_PROJECT_DIR": str(REPO_ROOT)} -- 2.47.3