test(785): mutation proofs for the three unproven ranked guards #810
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: timothy/ersatztv#810
Reference in New Issue
Block a user
fixes #785
docs/guard-inventory.mdranked four guards by what a silent failure would let through. The first(
pretooluse-bom-guard.sh) was proved earlier. This ships clause-level mutation proofs for the otherthree, and answers the inventory's third
## Done-whenbox — why every remaining row is stillNONE.What each proof established
2.
pretooluse-worktree-guard.sh+posttooluse-worktree-marker.sh— one mechanism in two files,neither tested, and never exercised together. Both 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. Four clauses disarmed and witnessed red against the real hooks: the marker read,
the ownership comparison (inverted — disarming it the other way only makes the guard deny more and
every deny assertion stays green), the
commit|mergealternation, and the other file's markerwrite asserted against the guard's decision. The last could not exist while the halves were tested
apart.
3.
.husky/pre-push:11'sunset GIT_DIR GIT_WORK_TREE GIT_INDEX_FILE— the guarded case is thenormal one, not an edge case, and that was measured rather than assumed. Git exports
GIT_DIRtopre-pushonly when the push comes from a worktree, whichprocess.shared-tree-readonlymakesthe mandated way to work here. With it set and
GIT_WORK_TREEunset, git treats the currentdirectory as the work tree —
rev-parse --show-toplevelfromweb/answers<wt>/web— socheck:api'sgit diff --exit-codecompares against index paths that do not exist and reports nodiff, exit 0. Generated-API drift ships and the gate prints success. Verified on macOS/git 2.55 and
Linux/git 2.47.3 (the runner host). Deletion and relocation are both proved.
4.
scripts/build_decisions_catalog.py --check— the path CI runs; the existing tests callrender_catalog()and nevermain(). Two mutations. The second is the one worth reading: replacingraise SystemExit(main())with a baremain()leaves the script printingdocs/decisions/README.md is staleon stderr while exiting 0, and the workflow step readsnothing but the exit code. Only the new subprocess case reddens — the #751/#719 shape.
Two defects found in this PR's own work
Recorded because they are the failure family the milestone is about, committed inside it.
{**os.environ, ...}snapshot is taken at import, beforeconftest.py's autouse fixturemonkeypatches
ETV_HOOK_FIRE_LOG_DIR— so every hook subprocess wrote to$HOME/.cache/ersatztv/hook-fire/. Measured: 1,488 records on macOS, 222 on the Linux host,every test green throughout, because the fire-log library is fail-open by design. That is #776's
defect reintroduced inside the file meant to prove #776's hooks, corrupting the surface this very
inventory cites as the observability claim for guards graded
NONE. Fixed, pinned, witnessed red,stray records removed from both hosts. The guard that should have caught it
(
test_the_suite_does_not_write_to_the_PRODUCTION_log) checks only the running test and its ownsandbox — recorded in the inventory and tracked in #809.
assert "hook-fire" not in str(Path.home()/".cache") or ...— the left operand is always true, so the comparison was never reached. A vacuous assertion inside
the guard added to prevent vacuous checks. Found by re-reading the fix commit, not by review.
Inventory
Rows regraded to
MUTATIONwith real refs; three newPROOFrows; counts now 34 guards / 5 tooling /18 proof, 12
MUTATION/ 6BEHAVIOUR-ONLY/ 16NONE(machine-checked against the table). Theranked list is kept — the ranking is the reusable part — and now states what each mutation
established. Added: what a file-level
MUTATIONgrade does not claim (it covers the clause itscited case mutates, not the whole file —
#790tracks clause-level), and the three groups the 16remaining
NONErows fall into, verified member-for-member against the derived set.Verification
scripts/tests: 758 passed, 2 skipped on the head; every commit green in isolation.reddens there too.
copies the tests build.
MUTATION/BEHAVIOUR-ONLYproof refs resolve to a realdef, checked independently oftest_guard_inventory.py..cstouched.Review
Three independent cold reviews in worktree isolation, two rounds. Findings closed: the log leak
(HIGH), a CI-command extractor that matched a commented-out workflow step (MEDIUM), a commit
message quoting the deletion count under the inversion label (MEDIUM, corrected in history), the
untested
mergehalf of the detection clause (LOW), two false claims and a truncated sentence in theinventory prose (LOW), and an over-narrow hook-call extractor (NIT).
Stated limitation: Codex failed twice to produce a cross-family review (it echoed the prompt and
exited, then got lost reading skill files), so all three reviews came from the same model family.
That is weaker than this repo's preference and is recorded rather than papered over.
Review history (five rounds, and what each found)
Recorded because the pattern is the point: every round found a defect introduced by the previous
round's fix, and every one of them was a green test over a dead check — the exact class #785 exists
to close, reproduced inside the work meant to close it.
mergehalf of the detection clause untested; two false claims in the inventory proseasserthad silently discardedThe turn came in round 4, by withdrawing the line-level workflow heuristic rather than patching it
a fourth time. Deciding which lines of a shell script execute needs a shell parser, and this repo has
lost that argument repeatedly (
fixing-a-parser-bug-introduces-the-next-one). The step's wholerunscript now goes to
bash, as the runner does, and ambiguity resolves by execution — which alsoremoved one false red the reviewer found and two I had not (backslash continuation,
set -euo pipefail, anifwrapper), all now pinned as negative controls.Nine ways the catalog guard can stop gating are detected — deleted, commented out,
if: false,if: ${{ false }},continue-on-error: true, job-levelif: false, surviving only as anechoargument, surviving only as heredoc data, and the workflow no longer triggering on
pull_request. Two channels are deliberately not guessed at and are stated as such: a jobif:false only at run time, and
continue-on-erroras a non-literal expression.Every figure in the durable artifacts now carries its reproduction. Four numbers in commit messages
were wrong along the way and are corrected in later messages rather than amended away.
Also filed: #809 — the production-log isolation guard is per-test but its docstring claims
per-suite; includes why the obvious general check is racy against a live session's hooks.
.husky/pre-push'sunset GIT_DIR ..., and measure why it matters d05d6623d1`--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) <noreply@anthropic.com>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) <noreply@anthropic.com>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) <noreply@anthropic.com>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) <noreply@anthropic.com>`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) <noreply@anthropic.com>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) <noreply@anthropic.com>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) <noreply@anthropic.com>Review-verdict: MERGEABLE @
55a7df2Five cold review rounds in worktree isolation (rounds 1-2 whole-branch, 3-5 scoped to each fix commit). Round 5 verdict MERGEABLE: reproduced all five quoted measurements exactly and verified by execution that all twelve parametrised cases decide for their labelled reason. Findings closed across rounds: production-log corruption (HIGH), a tautological assertion, a guard asserting on the helper not the effect, a commented-out-step false green, a heredoc false green, an untested merge clause, and five false or stale claims in prose. Residual, stated not hidden: two undecidable-outside-the-runner channels (runtime-false job if:, non-literal continue-on-error) that nothing claims to cover. LIMITATION: Codex failed twice to produce a cross-family review, so all rounds came from one model family - weaker than this repo's preference. Author-posted on the reviewers' behalf.