diff --git a/docs/README.md b/docs/README.md index 6411227fc..9070b833c 100644 --- a/docs/README.md +++ b/docs/README.md @@ -24,7 +24,8 @@ doc below, or that changes which sections a task signal points to.** | Concurrency / optimistic-locking work | `docs/api-conventions.md` §7a/b/c + `docs/decisions/optimistic-concurrency.md` | | Auth / security-surface work | `docs/decisions/api-auth-security.md` | | CI / release pipeline work | `docs/ci-cd.md` + `docs/decisions/release-ci-governance.md` | -| Proposing a new guard / CI check / regression test convention | `docs/defect-shapes-773.md` §4 (detector menu + the classes where no detector is plausible) | +| Proposing a new guard / CI check / regression test convention | `docs/defect-shapes-773.md` §4 (detector menu + the classes where no detector is plausible), then the two rules every guard must satisfy: `docs/decisions/records/testing/guard-derives-population-from-source.md` and `…/guard-ships-with-mutation-proof.md` | +| Adding / changing / deleting a guard file | `docs/guard-inventory.md` — every guard's row is machine-checked by `scripts/tests/test_guard_inventory.py`, so a new guard must acquire a row before the suite goes green | | Live local run / Playwright-MCP verification | `docs/e2e-local.md` + `scripts/e2e-local.sh` | | Adding/changing a UI-E2E browser flow | `docs/e2e-local.md` → "UI-E2E harness" + `scripts/e2e-ui.sh` | | What does a test suite cover | `docs/testing.md` | @@ -105,6 +106,13 @@ bounds, what's mined per issue): `docs/handoffs/chicorytv-issue-queue.md` → "K and an audit of which configured hooks/MCP servers/LSPs are actually invoked. Read it before proposing a new guard or CI check — §4 is the detector menu, and it argues against enumerating cases one incident at a time. +- **`docs/guard-inventory.md`** — every executable guard file, what it blocks, whether it is a + `GUARD` or `TOOLING`, and whether it ships a mutation proof (`MUTATION` / `BEHAVIOUR-ONLY` / + `NONE`) with a `file::function` ref. The population is derived from the filesystem and the + workflow/hook call sites and compared for set equality by `scripts/tests/test_guard_inventory.py`, + so a new guard cannot ship unclassified and a renamed test cannot leave a row claiming coverage it + has lost. Guards implemented inline in workflow YAML are deliberately outside that population — + the doc states the limit rather than implying coverage. - **`docs/tracker-retrofit-triage-237.md`** — audit trail for the #524 triage of ersatztv#237's 111 comments (method, per-comment classification, totals). Evidence for the `docs.tracker-comment-retrofit` decision; read it only when triaging another over-cap tracker. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index 16d02559d..40d690926 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -193,6 +193,8 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera | `testing.e2e-cleanup-scope-by-pid` | An E2E harness or agent may only kill processes whose PIDs it captured at launch — capture the PID; whoever owns the lifecycle releases it from a `trap ... EXIT INT TERM`. Never `pkill -f "dotnet ErsatzTV.dll"` (or any pattern that can match a process this run did not start). A foreign listener is reported, not reaped. | 2026-07-25 | [link](records/testing/e2e-cleanup-scope-by-pid.md) | | `testing.e2e-local-fresh-config-dir` | Always point `scripts/e2e-local.sh` at a fresh config dir — leftover channels/schedules/DB rows bleed state between runs and corrupt assertions. (The *readiness-probe hang* this record was originally written about was fixed in #533; the fresh-dir rule stands on state-bleed grounds alone.) | 2026-07-21 | [link](records/testing/e2e-local-fresh-config-dir.md) | | `testing.enumerating-guard-identity-not-position` | A guard that cross-checks a hand-reviewed registry against call sites discovered across the whole repo must key each entry on properties INTRINSIC to the site — file, kind, and the value source text — and never on its absolute line or column. A registry keyed on position is a function of every other file in the repo, so a branch that never touches the guard can invalidate it; and because each PR is green against its own base, that failure is structurally invisible pre-merge and lands on `main` after review and after the merge gate. Dropping the position keeps every mutation the guard exists for — a NEW site, a REMOVED site and a CHANGED value each still fail, since each changes the identity multiset — and costs exactly ONE case, which must be stated rather than implied: a SAME-IDENTITY SUBSTITUTION within one file (delete a registered site, add a different unreviewed one with the same kind and value token, net-zero count) now passes. A REPORTED failure still prints the discovered line:column, because identity and diagnostics need not share a format. Comparison stays a MULTISET count rather than set membership, so two sites in one file sharing an identity must be discovered exactly that many times and a third occurrence still fails. A SCANNER test that asserts real AST positions against FIXED inline fixtures is the opposite case and keeps its line/column identity — it has no churn, because its input does not move. | 2026-07-27 | [link](records/testing/enumerating-guard-identity-not-position.md) | +| `testing.guard-derives-population-from-source` | A guard that asserts a COMPLETENESS property enumerates its population from a machine-readable authoritative source — the enum, the generated OpenAPI document, the parsed workflow YAML, the provider list — and asserts SET EQUALITY in BOTH directions against it. It may not narrow that population with a filter, a `Where`, a `grep` or an early `continue` before the assertion, because a filter cannot see the member that is MISSING: the member whose absence is the defect is precisely the one the predicate excludes. A hand-written literal list of members is the same defect in slower motion — a filter frozen at authoring time, correct on the day it was written and unable to report the day it stopped being. Two boundaries bound the rule rather than weaken it. FIRST, filtering to select the SUBJECT of a PER-MEMBER property is legitimate and is not this defect: the excluded members satisfy the property vacuously, so the filtered walk and the whole walk assert the same thing (`ToolCatalogTests.Every_Query_Parameter_Should_Be_A_Declared_Property` filters to tools that declare query parameters, and a tool declaring none has nothing to check). The defect is filtering the population before a COMPLETENESS claim, which is what makes an absent member unrepresentable (#757 filtered on `QueryParameters is {Count: > 0}` and so could not see a tool that should have declared one and did not). SECOND, a population of VALUES always has an external authoritative source and this rule applies directly; a population of SITES IN CODE has no such list, needs find-all-references tooling, and is tracked separately in #777 — do not stretch a set-equality assertion over it. Distinguish the guard SCOPE (which subsystems it covers — a reviewed policy choice, legitimately hand-written) from the guard POPULATION (the members inside that scope — always derived). When the scope itself MIRRORS an authoritative source, the mirror needs its own equality check or a dated staleness marker, or the guard is complete within a scope that has silently gone stale. The canonical worked example in this repo is `ToolCatalogTests.Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`; the canonical residual gap is `MARKED_JOBS` in `scripts/tests/test_ci_dropped_step_guard.py`. | 2026-08-13 | [link](records/testing/guard-derives-population-from-source.md) | +| `testing.guard-ships-with-mutation-proof` | A guard is not considered tested because a test involving it passes. It ships with a MUTATION PROOF: remove or disarm THAT GUARD'S CLAUSE ALONE, and a NAMED test must go red. ONE NAMED EXCEPTION, with its limits, because the rule degenerates without it: where the guard IS a test (a checker enforcing a repo invariant, with no separate script behind it), disarming it makes it ABSENT rather than red, so the proof is the contrapositive — INTRODUCE THE DEFECT THE GUARD EXISTS TO CATCH into an isolated copy of the guarded artifact, and the named test must go red. That is a mutation of the guarded SYSTEM rather than of the assertion, and it is admissible ONLY for checker-guards and ONLY when the mutation was executed and witnessed. It is NOT a licence to grade an ordinary script-guard MUTATION for having a bad-input test: feeding a script an input its clause rejects is BEHAVIOUR-ONLY, which is what three rows were regraded for. A file-level grade under this exception covers the clause its cited case actually mutates, not every assertion that later lands in the same file; clause-level grading is tracked in #790. Three things this excludes, each of which has already shipped here as a green suite over a dead check. FIRST, a behavioural test — one that feeds the guard a good input and a bad input and checks it passes and fails — proves the guard REACTS, never that it is LOAD-BEARING; #685 had two guards on one condition where deleting either left the whole suite green while every behavioural test passed. SECOND, mutating the WHOLE FILE does not count (#510): a whole-file revert cannot show that a test reaches a particular clause, so the mutation must target the clause. THIRD, the guard being WIRED is not the guard RUNNING — #631's suite was invoked by no CI job, #751's step was dropped by the runner and the job reported success in 6s against a normal 14-17s, and #719's new logic was never connected to stdin. Every guard also carries an ANTI-VACUITY assertion, because the characteristic failure of a completeness check is reporting that it proved everything while its population was empty. Mechanical enforcement is possible for the BOOKKEEPING and not for the JUDGEMENT, and the split is the decision: `docs/guard-inventory.md` lists every guard file with its Kind, its Proof class (`MUTATION`/`BEHAVIOUR-ONLY`/`NONE`) and a `file::function` ref, and `scripts/tests/test_guard_inventory.py` derives the guard population from the filesystem and the call sites, asserts SET EQUALITY against the rows, and resolves every claimed ref to a real `def`. So a new guard cannot ship unclassified and a renamed test cannot leave a row silently claiming coverage. What stays with review, and is stated rather than papered over: nothing checks that a row claiming `MUTATION` is telling the truth. | 2026-08-13 | [link](records/testing/guard-ships-with-mutation-proof.md) | | `testing.live-e2e-prepush-timing` | Run live-E2E via `scripts/e2e-local.sh` before pushing a write-path or UI change, and exercise download endpoints with curl, never a browser tab. | 2026-07-21 | [link](records/testing/live-e2e-prepush-timing.md) | | `testing.playwright-mcp-download-and-recovery` | In Playwright-MCP E2E, fetch file-download endpoints with curl — never a browser tab or `window.open` — and if browser tools stall repeatedly, `pkill -f ms-playwright-mcp` and drive a fresh session. | 2026-07-21 | [link](records/testing/playwright-mcp-download-and-recovery.md) | | `testing.scripted-playout-golden-deferred` | The `PlayoutBuildGoldenTests` in-memory golden net covers Sequential (YAML) as of #381. Scripted's *end-to-end pipeline* is excluded — `ScriptedPlayoutBuilder` runs a user-authored external program that drives the engine over HTTP loopback, which the in-memory harness can't pin — so that full-pipeline (integration) harness is deferred to #563. But the scheduling *behavior* those scripts drive lives entirely in the in-process `SchedulingEngine` (the `ScriptedScheduleController` is a 1:1 pass-through to it), which IS directly unit/golden-testable; the earlier "Scripted is un-golden-able by construction" framing overstated the constraint by conflating transport with engine. #395 extracts that shared switch to `ContentEnumeratorBuilder` and adds a direct regression net (`ContentEnumeratorBuilderTests`) over it. | 2026-07-22 | [link](records/testing/scripted-playout-golden-deferred.md) | diff --git a/docs/decisions/records/testing/guard-derives-population-from-source.md b/docs/decisions/records/testing/guard-derives-population-from-source.md new file mode 100644 index 000000000..9f7f3968c --- /dev/null +++ b/docs/decisions/records/testing/guard-derives-population-from-source.md @@ -0,0 +1,85 @@ +--- +key: testing.guard-derives-population-from-source +title: '2026-08-13 — a guard derives its population from the authoritative source and asserts set equality, never a filter and never a sample (#774)' +status: active +since: '2026-08-13' +supersedes: none +superseded-by: none +rule: 'A guard that asserts a COMPLETENESS property enumerates its population from a machine-readable authoritative source — the enum, the generated OpenAPI document, the parsed workflow YAML, the provider list — and asserts SET EQUALITY in BOTH directions against it. It may not narrow that population with a filter, a `Where`, a `grep` or an early `continue` before the assertion, because a filter cannot see the member that is MISSING: the member whose absence is the defect is precisely the one the predicate excludes. A hand-written literal list of members is the same defect in slower motion — a filter frozen at authoring time, correct on the day it was written and unable to report the day it stopped being. Two boundaries bound the rule rather than weaken it. FIRST, filtering to select the SUBJECT of a PER-MEMBER property is legitimate and is not this defect: the excluded members satisfy the property vacuously, so the filtered walk and the whole walk assert the same thing (`ToolCatalogTests.Every_Query_Parameter_Should_Be_A_Declared_Property` filters to tools that declare query parameters, and a tool declaring none has nothing to check). The defect is filtering the population before a COMPLETENESS claim, which is what makes an absent member unrepresentable (#757 filtered on `QueryParameters is {Count: > 0}` and so could not see a tool that should have declared one and did not). SECOND, a population of VALUES always has an external authoritative source and this rule applies directly; a population of SITES IN CODE has no such list, needs find-all-references tooling, and is tracked separately in #777 — do not stretch a set-equality assertion over it. Distinguish the guard SCOPE (which subsystems it covers — a reviewed policy choice, legitimately hand-written) from the guard POPULATION (the members inside that scope — always derived). When the scope itself MIRRORS an authoritative source, the mirror needs its own equality check or a dated staleness marker, or the guard is complete within a scope that has silently gone stale. The canonical worked example in this repo is `ToolCatalogTests.Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`; the canonical residual gap is `MARKED_JOBS` in `scripts/tests/test_ci_dropped_step_guard.py`.' +signals: 'guard derives population · set equality both directions · never filter never sample · a filter cannot see the missing member · hardcoded list is a frozen filter · missing vs phantom · unreachable vs phantom query parameter · anti-vacuity count guard · accumulate drift do not fail fast · scope versus population · dated mirror of branch protection · `status_check_contexts` · `MARKED_JOBS` · values versus sites-in-code · paths: `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `scripts/tests/test_ci_dropped_step_guard.py`, `web/src/api/pageSizeCallSites.guard.test.ts` · issues: #774, #773, #757, #671, #650, #644, #633, #616, #503, #403' +mechanics: 'Both directions are named separately in the failure message — `missing`/`unreachable` (in the source, absent from the guarded set) and `phantom` (in the guarded set, absent from the source) — because the two are different defects and a single "sets differ" line invites fixing one and re-running.' +--- + +Across the 95 closed issues carrying a `## Closing record`, the largest family is one error wearing +four costumes: **reasoning about a representative instead of the population** (#773 §3 Family A — 27 +of the 69 process-failure records, 39%). This repo has already invented the cure four times, under +four names, and never written it down once. That is Family A operating on our own process. + +**The three targets it hits, because a rule aimed at only one of them misses.** The *fix* can sample +(#671 covered 4 of 10 media types; #403 was silent at 5 of 6 dispatch sites). The *verification* can +sample (#616 and #757 both filtered on the property whose absence WAS the defect; #633's +`ShouldContain("capped at 100")` passed against the text `"capped at 1000"`). And the *guard* can +sample (#644 grepped for an inflated `pageSize`, so #650's at-cap truncations were structurally +invisible to it). Same mistake, three altitudes. + +**Why a filter is not merely weaker but structurally blind.** A predicate over the population +partitions it into examined and excluded. A completeness claim asks whether some member is absent +altogether. The absent member is in neither partition — it is not a row the filter rejected, it is a +row that was never produced — so no strengthening of the predicate can reach it. This is why the +fix is never "tighten the filter"; it is to obtain the population from somewhere that knows the +whole of it and compare. + +**The canonical shape**, `ToolCatalogTests.Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`, +does five things and each earns its place. It reads `accepted` from the generated OpenAPI document +rather than from a list in the test. It walks `ToolCatalog.All` with no filter. It computes BOTH +`accepted.Except(declared)` and `declared.Except(accepted)`, because an unreachable parameter and a +phantom one are opposite defects. It carries an anti-vacuity floor, so a walk that reached nothing +cannot report success. And it accumulates the whole drift set instead of throwing on the first +mismatch — failing fast hands back one member at a time and invites fixing them one at a time, +which is how the #754 twin stayed hidden. + +**The boundary that keeps this rule honest.** `Every_Query_Parameter_Should_Be_A_Declared_Property` +sits eight lines away and DOES filter, correctly. Its property is per-member — each declared query +parameter must be a declared schema property — and a tool declaring none satisfies it vacuously, so +the filtered walk and the full walk assert the same thing. Read the two together before applying +this record: the trigger is not the token `.Where(`, it is a completeness claim standing behind one. +A rule stated as "no filters in guards" would flag the correct one and, once waved through twice, +stop being read at all. + +**Scope is not population, and this repo's best guard shows why the distinction matters.** +`scripts/tests/test_ci_dropped_step_guard.py` is the model for deriving a population: `_marked(job)` +reads the marked steps out of the parsed workflow, and +`test_the_guard_expects_EXACTLY_the_set_of_marked_keys_in_the_right_bucket` asserts set equality +against the guard's own argv. Inside its scope it is exemplary. Its SCOPE, though — +`MARKED_JOBS = ("test", "migrations")` — is a hand-written mirror of the required contexts on +`main`, annotated `Read live on 2026-08-10`. Those contexts are machine-readable +(`GET /repos/{owner}/{repo}/branch_protections` → `status_check_contexts`), and nothing in this repo +compares the two: `grep -rn 'status_check_contexts\|branch_protection'` over `*.py`/`*.sh`/`*.yml` +returns nothing. Verified 2026-08-13 — the live contexts still match the comment, so this is a +latent gap, not a live defect. But add a fourth required context and it acquires no dropped-step +guard, silently, and the file that models the rule is the one that cannot report it. A dated comment +is a claim about the past; it is not a check. + +**A weak detector is the symptom-keyed mistake, demonstrated rather than argued.** #774 wrote this +rule and then, in the same change, built a guard that violated it: a test asserting set equality +between the H10 verdict vocabulary's two shell copies, by extracting each with a regex. The +authoritative source it should have derived from does not exist — that is the entire problem, and +detector C's answer is to delete one copy, not to compare two. Six cold-review rounds each produced +another shell construction that either escaped the extractor (an unquoted value, a glob arm, an +indented reassignment, a comment-first arm) or made it red on a correct tree (a `) state=` in +a heredoc, a column-zero `esac` in a string truncating the scope). Every round's fix was locally +correct; the sequence never converged, because a regex over shell source is not a shell parser and +cannot be made into one. + +It was withdrawn, not patched a seventh time. **The residue is that the duplication is now +unmitigated** (#788 removes the second copy, which is the fix that was available all along), and the +lesson is the one this record already stated on theory: when the authoritative source is missing, +the answer is to create one, never to approximate it with a predicate over text. A guard whose +accompanying prose a reviewer can falsify each round is worse than no guard, because — by this +record's own argument — a guard described as sound stops being re-examined. + +**What this record does not cover.** A population of *sites in code* — the places that dispatch on a +value — has no external enumerator. #403 is that case: `PlaybackOrder`'s values are enumerable, but +the defect was 5 of 6 dispatch sites, and nothing lists dispatch sites. That residue needs +find-all-references tooling and is tracked in #777, where the relevant language server is currently +broken. Do not stretch set equality over it and call the class closed. diff --git a/docs/decisions/records/testing/guard-ships-with-mutation-proof.md b/docs/decisions/records/testing/guard-ships-with-mutation-proof.md new file mode 100644 index 000000000..c83a95ef7 --- /dev/null +++ b/docs/decisions/records/testing/guard-ships-with-mutation-proof.md @@ -0,0 +1,89 @@ +--- +key: testing.guard-ships-with-mutation-proof +title: '2026-08-13 — a guard ships with a proof it can go red: disarm that clause alone and a named test must fail (#775)' +status: active +since: '2026-08-13' +supersedes: none +superseded-by: none +rule: 'A guard is not considered tested because a test involving it passes. It ships with a MUTATION PROOF: remove or disarm THAT GUARD''S CLAUSE ALONE, and a NAMED test must go red. ONE NAMED EXCEPTION, with its limits, because the rule degenerates without it: where the guard IS a test (a checker enforcing a repo invariant, with no separate script behind it), disarming it makes it ABSENT rather than red, so the proof is the contrapositive — INTRODUCE THE DEFECT THE GUARD EXISTS TO CATCH into an isolated copy of the guarded artifact, and the named test must go red. That is a mutation of the guarded SYSTEM rather than of the assertion, and it is admissible ONLY for checker-guards and ONLY when the mutation was executed and witnessed. It is NOT a licence to grade an ordinary script-guard MUTATION for having a bad-input test: feeding a script an input its clause rejects is BEHAVIOUR-ONLY, which is what three rows were regraded for. A file-level grade under this exception covers the clause its cited case actually mutates, not every assertion that later lands in the same file; clause-level grading is tracked in #790. Three things this excludes, each of which has already shipped here as a green suite over a dead check. FIRST, a behavioural test — one that feeds the guard a good input and a bad input and checks it passes and fails — proves the guard REACTS, never that it is LOAD-BEARING; #685 had two guards on one condition where deleting either left the whole suite green while every behavioural test passed. SECOND, mutating the WHOLE FILE does not count (#510): a whole-file revert cannot show that a test reaches a particular clause, so the mutation must target the clause. THIRD, the guard being WIRED is not the guard RUNNING — #631''s suite was invoked by no CI job, #751''s step was dropped by the runner and the job reported success in 6s against a normal 14-17s, and #719''s new logic was never connected to stdin. Every guard also carries an ANTI-VACUITY assertion, because the characteristic failure of a completeness check is reporting that it proved everything while its population was empty. Mechanical enforcement is possible for the BOOKKEEPING and not for the JUDGEMENT, and the split is the decision: `docs/guard-inventory.md` lists every guard file with its Kind, its Proof class (`MUTATION`/`BEHAVIOUR-ONLY`/`NONE`) and a `file::function` ref, and `scripts/tests/test_guard_inventory.py` derives the guard population from the filesystem and the call sites, asserts SET EQUALITY against the rows, and resolves every claimed ref to a real `def`. So a new guard cannot ship unclassified and a renamed test cannot leave a row silently claiming coverage. What stays with review, and is stated rather than papered over: nothing checks that a row claiming `MUTATION` is telling the truth.' +signals: 'mutation proof · delete the guard alone see red · disarm the clause not the file · behaviour-only is not a proof · anti-vacuity assertion · guard wired is not guard running · a green job with no step output · `docs/guard-inventory.md` · set equality against the row set · proof ref resolves to a real def · 21 of 32 guards unproven · PROOF kind stops the regress · hook wiring is not hook existence · paths: `scripts/tests/test_guard_inventory.py`, `scripts/tests/test_ci_dropped_step_guard.py`, `scripts/ci-prove-ban-detects.sh` · issues: #775, #773, #751, #756, #719, #685, #631, #621, #510, #445' +mechanics: 'Proof classes are a closed vocabulary enforced by the inventory test; a `TOOLING` row may not claim a proof. `scripts/ci-prove-ban-detects.sh` is the one guard that runs its own mutation at CI time rather than in pytest, because the thing it proves — that the ban test is not deselected — is disarmable from inside pytest configuration.' +--- + +**Vacuous verification is the single most common recorded failure shape in this corpus — 17 of 69 +records (25%), of which about half are not sampling errors at all: the check was simply dead** +(#773 §3 Family B). The rule already exists in prose, in #685's record and in project memory. It is +enforced nowhere, which is why it keeps being relearned. This is a hooks problem wearing a docs +problem's clothes. + +**The six ways a check has already been dead here**, because the list is more convincing than the +principle. `scripts/tests/` was invoked by no CI job (#631) — *a test suite that runs nowhere is not +a safety net, it is a document*. Deleting the single line calling `record_wing_faults()` from +`main()` left the suite green while a real record vanished (#621). A `${{ }}` inside a shell +**comment** made the runner drop the step, and the job reported success in 6s where the working +version takes 14–17 (#751). New pre-push logic was never wired to receive stdin (#719). `if ! cmd; +then status=$?` reads bash's logical negation, so a failing spec run exited 0 (#445). And two guards +on one condition masked each other, so deleting either left the suite green and the boundary test +pinned nothing (#685). + +Note what those have in common: in every case a human had read the guard and believed it worked. The +guard was not subtly wrong, it was *absent*, and absence is invisible from the outside because a +dead check and a satisfied check produce the identical green. + +**Why behavioural coverage is not the same claim.** A test that drives the guard with a good input +and a bad input establishes that the guard's logic responds to its argument. It says nothing about +whether that logic is *connected* — to the runner, to the caller, to the exit code anyone reads. +#719 and #631 would both have passed such a test on the day they shipped dead. This is why +`docs/guard-inventory.md` grades `BEHAVIOUR-ONLY` separately from `MUTATION` rather than counting +them together: they answer different questions, and adding them up is how 19 unproven guards get +reported as covered. + +**The model, and what makes it the model.** +`test_ci_dropped_step_guard.py::test_dropping_ANY_single_step_FAILS_the_guard` removes each marked +step's marker *one at a time* and asserts the guard rejects the run — every step, explicitly not a +sample, on the argument that the interesting drop is whichever step someone actually edits. Around +it sit a positive control (the guard passes when every step marked itself), a negative control (a +fired skip gate still requires the always-steps, so the gate is not a blanket off-switch), and an +anti-vacuity check that `assert` with an empty expectation set REFUSES instead of printing *All 0 +expected step(s) executed* and exiting 0. Four kinds, none substituting for another. + +**What was mechanised here, and the line it stops at.** The bookkeeping half is fully mechanical and +now enforced: the inventory's population is derived, compared for set equality in both directions, +and every `Proof ref` is resolved to a real `def`. A new guard cannot be added without being +classified; a renamed test cannot leave a row claiming a proof that evaporated. That closes the two +ways this decays silently. The judgement half — *is this row's `MUTATION` claim true?* — is not +mechanical and was not faked. A generic mutation runner for shell hooks was considered and rejected: +it would have to know which clause of a 90-line hook is the guard, and a runner that guesses would +manufacture exactly the confident-but-empty coverage this record exists to prevent. + +**What "disarm the clause" means when the guard IS a test, because the rule degenerates otherwise.** +For a guard implemented as a shell script with a separate test, disarming is literal: delete the +clause, watch the named test redden. For a guard implemented AS a test — `test_guard_inventory.py`, +`test_ci_image_pin_population.py` — deleting the guard does not make anything red, it makes the test +*absent*. The equivalent proof is the contrapositive: **introduce the defect the guard exists to +catch, and the named test must go red.** This is the named exception in the `rule` above, not a +second standard sitting beside it — an earlier draft stated it only here, and cold review correctly +objected that a record carrying two incompatible criteria cannot adjudicate a future grade: one +reviewer would reject the self-referencing rows on the frontmatter, another accept them on the body. +It is a real mutation of the guarded system, not of the test, and it is what was executed here — a deleted inventory row, a new unclassified hook, a stale +proof ref, a `SHIP*` case arm the extractor could not parse, and each of four ways a container job +can lose its pin, applied to every registered job rather than a sample. Cold review pushed back on +calling these mutation proofs; the pushback is right that they mutate INPUT rather than the +assertion, and wrong that this makes them weaker — for a checker, the input IS the system under +guard. Recorded because the distinction will come up again. + +Where that review was straightforwardly right: neutering `pin_population_faults` wholesale (20 of 25 +red) is coarser than disarming one clause at a time, and coarse enough that a single surviving +clause would not be noticed. Read it as a floor, not as per-clause coverage. + +**Applied to itself, which is the only honest test of a rule like this — and it failed twice before +it passed.** The population guard's first draft compared two derived sets that shrank together, so +it was blind to a removed `container:` block: the exact defect `testing.guard-derives-population- +from-source` exists to prevent, in the guard enforcing it. Running the mutations caught that. Then +the inventory's own completeness check shipped with a path pattern that excluded `scripts/tests/`, +so the three guard files added in that very commit had no rows and the check stayed green — caught +not by running anything, but by a cross-family reviewer. And the summary counts were a hand-kept +mirror of the table that was wrong on arrival, which both reviewers found independently. Three +self-inflicted instances of two rules, in the change introducing them. That is the argument for +mechanising the bookkeeping rather than trusting care, made by the authors of the rules failing to +follow them while writing them down. diff --git a/docs/guard-inventory.md b/docs/guard-inventory.md new file mode 100644 index 000000000..64482c0bf --- /dev/null +++ b/docs/guard-inventory.md @@ -0,0 +1,191 @@ +# Guard inventory (ersatztv#774 / #775) + +Every executable guard **file** in this repo, what it blocks, and whether it ships a proof it can go +red. `scripts/tests/test_guard_inventory.py` derives the population from the filesystem and the +workflow/hook call sites and asserts **set equality** against the `Guard` column, so a new guard +cannot be added without acquiring a row here, and a row cannot name a proof that does not exist. + +**Read `docs/decisions/records/testing/guard-derives-population-from-source.md` and +`…/guard-ships-with-mutation-proof.md` before editing a guard or adding a row.** + +## Columns + +- **Kind** — `GUARD` (it can block a commit, a push, a tool call, a merge or a CI job), `TOOLING` + (it does work but asserts nothing; listed so its absence from the guard set is a recorded + decision, not an oversight), or `PROOF` (a `scripts/tests/` file whose job is to prove another + guard). `PROOF` exists to stop a regress: once test files entered the population, every mutation + proof became a row wanting a proof of its own. A `scripts/tests/` file that enforces a repo + invariant with no separate guard behind it is a `GUARD`, graded normally, and may cite a mutation + case in its own file. +- **Proof** — `MUTATION`: a clause-level mutation **was executed** and this named test **was + witnessed red**. The test either performs the disarm itself (a monkeypatch, a deselection, a + removed marker) or a recorded incident shows it going red when the clause went away. + `BEHAVIOUR-ONLY`: a real test drives the guard through its real entry point with good and bad + input, but no one has demonstrated that removing the clause reddens it. `NONE`. +- **Proof ref** — `file.py::function`, verified to exist by the inventory test. + +The distinction between `MUTATION` and `BEHAVIOUR-ONLY` is the whole point of #775 and is not a +grading curve. A behavioural test proves the guard reacts to its input; only a mutation proves the +guard is *load-bearing*. #685 shipped two guards on one condition where deleting either left the +suite green, and every behavioural test passed throughout. + +**The definition above is the second one.** The first read "a named test disarms this guard's clause +and asserts red", and three rows were graded `MUTATION` against it that do not disarm anything — +they feed the real script an input only that clause rejects (empty stdin, a short page, a full first +page). Cold review caught it, and the objection is right twice over: those tests are the same +species as ones graded `BEHAVIOUR-ONLY` eight rows away, so the column was being applied as a curve +on the very day it was introduced. They are regraded. The surviving criterion is *witnessed*, not +*plausible* — "removing this clause would surely redden that test" is an argument, and this table +exists because arguments of that shape have been wrong here six times. + +## Scope limit, stated rather than implied + +This inventory covers guard **files**, discovered by globbing `.claude/hooks/*.sh`, `.husky/*` and +`scripts/tests/test_*.py`, plus every `scripts/…` path referenced by a workflow or a hook. Six +classes are outside that population. They are listed because the first version of this section named +only the first one, and cold review found that the very guards this inventory shipped with were +sitting in the gap: + +1. **Guards inline in workflow YAML** — most importantly `pr-checks.yml:ci-image-pin`. "Which jobs + are guards" needs a judgement call per job the filesystem cannot supply. Two were audited under + #774 and one fixed; extending the population is tracked in #786. +2. **C# and TypeScript guards** — `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs` and + `web/src/api/pageSizeCallSites.guard.test.ts` are both structural guards and neither has a row. +3. **Mentions counted as call sites.** The `scripts/…` scrape matches any occurrence, including + inside a comment or an `::error::` string. `scripts/update-openapi.sh` is named in a + `pr-checks.yml` error message, so removing the step that runs it would leave its row intact. +4. **Nested and non-lowercase paths** beyond `scripts/tests/` — a guard under + `scripts/scripted-schedules/`, or with an uppercase name, is invisible to the scrape. +5. **`.yaml` workflows and non-`.sh` hooks** — the globs are `*.yml` and `*.sh` only. +6. **Transitive calls** — a script invoked only by another script, rather than by a workflow or + hook, is not discovered. + +Hook **wiring** is checked (`test_every_hook_file_is_actually_WIRED` reads `.claude/settings.json` +and the husky hooks with full-line comments stripped), so a hook file whose registration is deleted +fails rather than keeping a row that reads as coverage. Its limit, stated because the check reads +stronger than it is: **it is a substring test for the basename, not a parse of the invocation.** +`: # .claude/hooks/decisions-guard.sh disabled` still reads as wired, and conversely a hook invoked +through a wrapper or a constructed path reads as unwired. It catches deletion, which is the common +case; it does not catch deliberate disablement. The check does not extend to the `scripts/` half at +all. + +## Inventory + +| Guard | Blocks | Kind | Proof | Proof ref | +| --- | --- | --- | --- | --- | +| `.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/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` | +| `.claude/hooks/pretooluse-agent-model.sh` | an Agent dispatch naming no model (asks) | GUARD | NONE | — | +| `.claude/hooks/pretooluse-agent-ram.sh` | an Agent dispatch under 10% free RAM | GUARD | NONE | — | +| `.claude/hooks/pretooluse-bash-guard.sh` | a Bash call setting `ETV_UPDATE_GOLDENS` | GUARD | NONE | — | +| `.claude/hooks/pretooluse-bom-guard.sh` | a commit/push carrying a BOM in a touched `.cs` | GUARD | NONE | — | +| `.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 | — | +| `.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 | — | +| `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` | +| `scripts/ci-detect-already-validated.sh` | nothing directly (feeds the skip gate) | GUARD | NONE | — | +| `scripts/ci-detect-docs-only.sh` | nothing directly (feeds the skip gate) | GUARD | NONE | — | +| `scripts/ci-peak-anon.sh` | nothing (samples container memory) | TOOLING | NONE | — | +| `scripts/ci-prove-ban-detects.sh` | the release path, if the delimiter ban is disarmed | GUARD | NONE | — | +| `scripts/ci-step-ran.sh` | the two required contexts, on a dropped step | GUARD | MUTATION | `test_ci_dropped_step_guard.py::test_dropping_ANY_single_step_FAILS_the_guard` | +| `scripts/decisions_validate.py` | the `decisions-guard` job, on a lifecycle fault | GUARD | MUTATION | `test_decisions_validate.py::test_main_actually_CALLS_the_wing_scan` | +| `scripts/e2e-functional.sh` | the Functional E2E job, on a failed HTTP contract assertion | GUARD | NONE | — | +| `scripts/e2e-local.sh` | nothing (boots a local instance) | TOOLING | NONE | — | +| `scripts/e2e-ui.sh` | nothing (drives the Playwright flows) | TOOLING | NONE | — | +| `scripts/jq-preflight.sh` | the `script-tests` job, on a jq version change | GUARD | BEHAVIOUR-ONLY | `test_jq_preflight.py::test_below_the_floor_is_LOUD` | +| `scripts/post-review-verdict.sh` | nothing (writes the verdict status) | GUARD | BEHAVIOUR-ONLY | `test_post_review_verdict.py::test_never_retargets_the_verdict_at_the_new_head` | +| `scripts/pr-changed-files.sh` | the verdict exemption, on an incomplete enumeration | GUARD | BEHAVIOUR-ONLY | `test_pr_changed_files.py::test_a_SHORT_page_does_not_end_the_enumeration` | +| `scripts/update-openapi.sh` | nothing (regenerates the spec) | TOOLING | NONE | — | +| `scripts/tests/test_build_catalog.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` | +| `scripts/tests/test_ci_release_path_scan_job.py` | the `script-tests` job, on a weakened release-path scan job | GUARD | NONE | — | +| `scripts/tests/test_decisions_lib.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_decisions_validate.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_guard_inventory.py` | the `script-tests` job, on an unclassified guard or a stale proof ref | GUARD | MUTATION | `test_guard_inventory.py::test_the_inventory_covers_exactly_the_guards_that_exist` | +| `scripts/tests/test_jq_preflight.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_merge_consent_base_change.py` | the `script-tests` job | PROOF | NONE | — | +| `scripts/tests/test_merge_consent_exemption.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_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 | — | + +## What the numbers say + +31 guards, 4 tooling scripts, 12 proof files. **4 guards carry a mutation proof; 6 are +behaviour-only; 21 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 +independently. + +**One guard was WITHDRAWN rather than shipped**, and it is worth a line here because the inventory +is where a future session will look for it. A `test_review_verdict_vocabulary_parity.py` asserted +set equality between the H10 verdict vocabulary's write side and read side by extracting both from +shell source with regexes. Six cold-review rounds each found another shell construction that either +escaped it or made it red on a correct tree. It was deleted rather than patched a seventh time: the +`testing.guard-derives-population-from-source` record says a weak detector is itself the +symptom-keyed mistake, and this was the empirical demonstration. The underlying duplication is real +and is now UNMITIGATED — tracked in #788, which fixes it by removing the second copy. + +**What that file asserted, and where each part went** — enumerated because the first attempt at this +withdrawal silently dropped one and claimed otherwise, which is +`process.enumerate-workaround-behaviors-before-deleting` failing on its own removal: + +| Invariant it asserted | Where it is now | +|---|---| +| Bidirectional write/read vocabulary parity | **LOST.** The reason for the withdrawal; #788 | +| No READ-side word is both positive and negative | **LOST** as a universal property; #788 | +| No WRITE-side word is both success and failure | **LOST** as a universal property; #788 | +| Strict extractor consumes every case arm | Retired with the extractor; #788 | +| `POS_RE`/`NEG_RE` each assigned exactly once | Retired with the extractor; #788 | +| Both extractors find non-empty sets (anti-vacuity) | Retired with the extractor; #788 | +| A wrong write source makes the extractors disagree | Retired with the extractor; #788 | + +Two guards were ADDED in its place. They are **not** replacements for rows 2 and 3 and the table +above deliberately does not list them as such — an earlier draft did, and cold review was right that +relabelling a lost invariant as a narrower surviving one is the exact failure this table exists to +prevent, committed by the table: + +| Added | What it pins | +|---|---| +| `test_check_review_verdict.py::test_each_verdict_word_retains_its_established_polarity` | the five established tokens still READ with their established polarity | +| `test_post_review_verdict.py::test_each_verdict_word_posts_its_established_polarity` | the five established tokens still POST with their established polarity | + +**Polarity is strictly weaker than disjointness, and the gap is demonstrable.** Put `MERGEABLE` in +BOTH `case` arms of `post-review-verdict.sh`: the success arm wins, every polarity assertion stays +green, and the withdrawn disjointness test failed. What the two added guards do catch is the +dangerous direction — a token that a reviewer means as `BLOCKED` silently reading or posting as +approval, which writes a green `review-verdict/h10`. A token added to one script and not the other +is untested by either, which is the whole of #788. + +The 21 include every `PreToolUse` hook except merge-consent — the same population #776 found we +cannot even observe firing. A guard that is neither proven nor observable is an assertion about the +past. + +The gaps are not uniform in cost, and the ranking that matters is *what a silent failure would let +through*, not test count: + +1. `pretooluse-bom-guard.sh` — guards a defect that has recurred three times (#311, #402, #405). +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. + +Filling these 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. diff --git a/scripts/post-review-verdict.sh b/scripts/post-review-verdict.sh index 649623279..1a1da93f3 100755 --- a/scripts/post-review-verdict.sh +++ b/scripts/post-review-verdict.sh @@ -51,10 +51,21 @@ note="$*" printf '%s' "$pr" | grep -qE '^[0-9]+$' || die "pull-request number must be numeric, got '$pr'" -# Normalize to upper-case and classify. The positive set MUST stay in sync with the hook's -# condition-(c) regex in .claude/hooks/pretooluse-merge-consent.sh (mergeable|approved|lgtm) — -# a word this script treats as positive but the hook does not would let the server-side status go -# green while the hook still denies, which reads as an unexplained deny. +# Normalize to upper-case and classify. The positive set MUST stay in sync with the READ side's +# POS_RE/NEG_RE in scripts/check-review-verdict.sh (mergeable|approved|lgtm) — a word this script +# treats as positive but the classifier does not would let the server-side status go green while +# the merge-consent hook still denies, which reads as an unexplained deny. +# +# That sync is NOT asserted anywhere, and you should not assume otherwise. ersatztv#774 tried: a +# test extracted both vocabularies from their own shell source and compared them. Six cold-review +# rounds each found another shell construction that either escaped the extractor or made it fail on +# a correct tree, so it was withdrawn rather than patched a seventh time. Writing a shell parser as +# a regex does not converge. +# +# The fix is to stop having two copies — one declarative vocabulary both scripts read — tracked in +# ersatztv#788. Until that lands, THIS COMMENT IS THE ONLY THING holding the two lists together, and +# comments drift: this one already had, naming .claude/hooks/pretooluse-merge-consent.sh as the home +# of the regex. The hook carries no copy; it delegates to check-review-verdict.sh. verdict=$(printf '%s' "$verdict_raw" | tr '[:lower:]' '[:upper:]') case "$verdict" in MERGEABLE|APPROVED|LGTM) state="success" ;; diff --git a/scripts/tests/test_check_review_verdict.py b/scripts/tests/test_check_review_verdict.py index 5670c947e..2c7c6f4ed 100644 --- a/scripts/tests/test_check_review_verdict.py +++ b/scripts/tests/test_check_review_verdict.py @@ -419,3 +419,63 @@ def test_non_hex_head_is_an_input_error(): text=True, ) assert p.returncode == 2 + + +# --- read-side POLARITY regression, rescued from the withdrawn parity test (ersatztv#774) ------ + + +# The vocabulary as BOTH scripts spell it. A literal list, and per +# `testing.guard-derives-population-from-source` that is legitimate HERE and would not be for a +# completeness claim: the property below is PER-MEMBER — "each of these words classifies as exactly +# one thing" — so a word missing from this list is simply an untested word, not a defect the list +# conceals. It is emphatically NOT a claim that these are the only words the scripts accept. Proving +# THAT needs one shared vocabulary both scripts read, which is ersatztv#788. +POSITIVE_WORDS = ["MERGEABLE", "APPROVED", "LGTM"] +NEGATIVE_WORDS = ["BLOCKED", "NOT-MERGEABLE"] + + +@pytest.mark.parametrize( + ("word", "expected"), + [(w, "positive") for w in POSITIVE_WORDS] + [(w, "negative") for w in NEGATIVE_WORDS], +) +def test_each_verdict_word_retains_its_established_polarity(word, expected): + """The five established tokens still classify the way reviewers rely on. READ side only. + + NAMED FOR WHAT IT IS. An earlier version called itself a disjointness test and its docstring + said "no word may be in both vocabularies". Cold review was right that this is an overclaim: + pinning the observable classification of five hardcoded tokens cannot establish a universal + property over every token the scripts accept, and for THAT property an omitted token is not a + vacuous pass — it is precisely the untested member. This is a polarity regression, and the + honest scope is the five words listed. Universal disjointness needs one shared vocabulary both + scripts read (ersatztv#788). + + WHY IT SURVIVED ITS PARENT. That test asserted set equality between the write side's `case` arms + and the read side's `POS_RE`/`NEG_RE` by parsing shell with regexes, and six review rounds each + found another construction it mis-read; it was deleted rather than patched a seventh time. But + it carried a SECOND, separable invariant that had nothing to do with parsing, and deleting the + file silently took that with it — caught by cold review of the deletion, which is exactly the + "enumerate what a workaround provided before removing it" rule + (`process.enumerate-workaround-behaviors-before-deleting`). + + WHY THIS VERSION IS SOUND WHERE ITS PARENT WAS NOT. It EXECUTES the real classifier instead of + reading its source, so no shell construction can fool it. `check-review-verdict.sh` sets + `is_pos` and `is_neg` from two INDEPENDENT `grep -iqE` calls, so a word in both patterns sets + both flags and precedence decides. + + WHICH DIRECTION THIS CATCHES, MEASURED RATHER THAN REASONED — the first draft of this docstring + asserted it and was checked afterwards, which is the wrong order. `check-review-verdict.sh:212` + reads `if [ "$is_pos" = 1 ]; then head_pos=1; else head_neg=1; fi`, so `is_pos` wins per line + and an overlapping word resolves POSITIVE. Both mutations were run against this test: + + * `blocked` added to `POS_RE` -> `BLOCKED` classifies `positive` -> RED. That is the dangerous + direction — a verdict meant to block reporting as approval — and it is caught. + * `mergeable` added to `NEG_RE` -> still `positive`, stays green. NOT a gap: because `is_pos` + wins, that edit has no observable effect at all. `NEG_RE` is shadowed by `POS_RE` for any + overlapping word, so there is no behaviour there to catch. + """ + got, rc = classify([verdict(word, HEAD)]) + assert (got, rc) == (expected, 0), ( + f"'{word}' classified as {got!r} (rc={rc}), expected {expected!r}. If it now matches BOTH " + "POS_RE and NEG_RE, the classifier's two independent greps both fire and precedence picks " + "the verdict instead of the reviewer — a word meant to block could report positive." + ) diff --git a/scripts/tests/test_ci_image_pin_population.py b/scripts/tests/test_ci_image_pin_population.py new file mode 100644 index 000000000..58e20e7aa --- /dev/null +++ b/scripts/tests/test_ci_image_pin_population.py @@ -0,0 +1,333 @@ +"""The CI-image pin guard must see a container job that carries NO pin (ersatztv#774). + +WHAT THIS IS PROTECTING. `pr-checks.yml`'s `ci-image-pin` job states the invariant in its own error +text — "Every container: job must pin ersatztv-ci:<7-char-sha>" — and then does not check it. What +it checks is: + + mapfile -t pins < <(grep -oE 'ersatztv-ci:[0-9a-f]+' .gitea/workflows/docker-build.yml \ + | cut -d: -f2 | sort -u) + [ "${#pins[@]}" -eq 1 ] + +`sort -u` collapses to DISTINCT VALUES, so the count is a property of the pins that are PRESENT. A +job that carries a `container:` block with no `ersatztv-ci:` pin — or no `container:` block at all — +contributes nothing to grep's output, so it cannot move that count. Delete the `container:` block +from `test` and four pins remain: still one distinct value, still green, and a REQUIRED context now +runs on the bare runner instead of the toolchain image. That is ersatztv#774's Family A exactly: a +guard that cannot see the member that is MISSING, because its population is the set of matches +rather than the set of jobs. + +THE SPLIT WITH THE SHELL GUARD IS DELIBERATE, and is not two copies of one rule (which would be +#773's Family C). Two different assertions over the same subject: + + * `ci-image-pin` (shell, pr-checks.yml) owns the questions that need GIT HISTORY — does the pin + resolve to a commit, is it exactly 7 chars, is it the last commit to touch `docker/ci`. A + pytest cannot answer those without a full clone. + * this file owns the question that needs the PARSED YAML — is the set of jobs declaring a + `container:` exactly the set of jobs pinning the image. A shell grep structurally cannot answer + that, which is why it was never asked. + +Neither restates the other, and each says so above the code. +""" + +from __future__ import annotations + +import copy +import re +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +WORKFLOWS_DIR = REPO_ROOT / ".gitea" / "workflows" +WORKFLOW = WORKFLOWS_DIR / "docker-build.yml" + +# The image repository, without the tag. Matched as a whole path rather than by the bare +# `ersatztv-ci` token so a job pointing at a LOOK-ALIKE registry (a personal fork, a typo'd host) +# is a fault rather than a silent pass — the shell guard's `grep -oE 'ersatztv-ci:[0-9a-f]+'` reads +# the tail of any string and would accept `evil.example/ersatztv-ci:32747a0`. +IMAGE_REPO = "192.168.1.95:3000/timothy/ersatztv-ci" +_PIN = re.compile(rf"^{re.escape(IMAGE_REPO)}:([0-9a-f]+)$") + +_DOC = yaml.safe_load(WORKFLOW.read_text()) + +# THE HAND-REVIEWED REGISTRY of jobs that must run inside the CI toolchain image, cross-checked in +# BOTH directions against what the workflow actually declares. This is deliberately a literal, and +# the reason is the one case set equality between two DERIVED sets cannot cover. +# +# The first version of this file compared `container_jobs(doc)` against `pinned_jobs(doc)` and +# nothing else. That is blind to the mutation that matters most: delete a `container:` block and the +# job leaves BOTH sets together, so the comparison stays balanced and reports green — which is the +# very fail-open this file was written to close, reproduced one level up. A guard whose expected set +# shrinks in step with the thing it is guarding is not a guard. +# +# So the population needs one anchor that does NOT move when the workflow does, and a reviewed list +# is the only available one: nothing else in the repo records which jobs are supposed to need the +# toolchain. This is the same shape as `web/src/api/pageSizeCallSites.guard.test.ts` — discovery on +# one side, a reviewed registry on the other, compared both ways — and it is the SCOPE half of +# `testing.guard-derives-population-from-source`, not an exception to it. Editing this set is a +# reviewable act; a job silently losing its container block is not. +TOOLCHAIN_JOBS = frozenset({"test", "migrations", "functional-e2e", "api-docs", "format"}) + +# `scan` and `build` deliberately run on the bare runner: `scan` is `runs-on: small` and needs only +# python, and `build` drives docker/buildx on the host. Listed here so their ABSENCE above reads as +# a decision rather than an oversight. +BARE_RUNNER_JOBS = frozenset({"scan", "build"}) + + +def _jobs(doc) -> dict: + return doc["jobs"] + + +def container_jobs(doc) -> set[str]: + """Every job declaring a `container:`. THE AUTHORITATIVE POPULATION. + + Derived from the parsed workflow, which is the only thing that knows the whole of it. A literal + list here would reintroduce the defect one file over — correct on the day it was written and + unable to report the day a sixth job appeared. + """ + return {name for name, job in _jobs(doc).items() if isinstance(job, dict) and "container" in job} + + +def pinned_jobs(doc) -> dict[str, str]: + """job -> pinned tag, for every job whose container image is the CI toolchain image.""" + out = {} + for name, job in _jobs(doc).items(): + if not isinstance(job, dict): + continue + image = str((job.get("container") or {}).get("image", "")) + m = _PIN.match(image) + if m: + out[name] = m.group(1) + return out + + +def pin_population_faults(doc) -> list[str]: + """Set equality in BOTH directions, plus tag agreement. Accumulated, never fail-fast. + + Both directions are reported separately because they are different defects. UNPINNED (a + container job the guard cannot see) is the fail-open this file exists for. PINNED-BUT-NOT-A- + CONTAINER-JOB cannot arise from `pinned_jobs` as written, but is computed anyway so that a + future change to either helper cannot quietly make the comparison one-sided. + """ + declared = container_jobs(doc) + pinned = pinned_jobs(doc) + + faults = [] + # AGAINST THE REGISTRY FIRST. This is the direction the two derived sets cannot cover: a job + # that loses its `container:` block leaves `declared` and `pinned` together, so their equality + # survives untouched while the job quietly moves to the bare runner. + for name in sorted(TOOLCHAIN_JOBS - set(pinned)): + faults.append( + f"job '{name}' is registered as needing the CI toolchain image but does not pin it " + "(its container: block is missing or points elsewhere) — it is running on the bare " + "runner" + ) + for name in sorted(set(pinned) - TOOLCHAIN_JOBS): + faults.append(f"job '{name}' pins the toolchain image but is not in TOOLCHAIN_JOBS") + + for name in sorted(declared - set(pinned)): + image = str((_jobs(doc)[name].get("container") or {}).get("image", "")) + faults.append( + f"job '{name}' declares a container: but its image is {image!r}, not " + f"{IMAGE_REPO}:. ci-image-pin's grep cannot see this job at all, so the pin it " + "reports as current says nothing about what this job actually runs in." + ) + for name in sorted(set(pinned) - declared): + faults.append(f"job '{name}' pins the image without declaring a container: block") + + tags = set(pinned.values()) + if len(tags) > 1: + faults.append( + f"jobs pin DIFFERENT tags: {sorted((n, t) for n, t in pinned.items())}. All container " + "jobs must run the same toolchain image." + ) + return faults + + +# ------------------------------------------------------------------------------------------------ +# THE LIVE ASSERTION +# ------------------------------------------------------------------------------------------------ + + +def test_every_container_job_pins_the_CI_toolchain_image(): + faults = pin_population_faults(_DOC) + assert not faults, ( + "docker-build.yml has a container: job the CI-image pin guard cannot see:\n " + + "\n ".join(faults) + + "\n\n`ci-image-pin` counts DISTINCT pin strings, so a job with no pin contributes nothing " + "to that count and passes silently while running on the bare runner. See ersatztv#774." + ) + + +def test_the_registry_and_the_workflow_agree_on_which_jobs_use_the_toolchain(): + """BOTH directions against the reviewed registry — the anchor that does not move. + + Left-to-right catches a job silently LOSING its container block (the mutation set equality + between two derived sets cannot see, because both sides shrink together). Right-to-left catches + a NEW container job nobody registered. Neither direction is optional and the messages differ, + because the two are opposite mistakes. + """ + declared = container_jobs(_DOC) + assert TOOLCHAIN_JOBS - declared == frozenset(), ( + f"these jobs are registered as needing the CI toolchain image but no longer declare a " + f"container: block — {sorted(TOOLCHAIN_JOBS - declared)}. They are now running on the bare " + "runner. If that is deliberate, move them to BARE_RUNNER_JOBS in this file and say why in " + "the PR." + ) + assert declared - TOOLCHAIN_JOBS == frozenset(), ( + f"these jobs declare a container: but are not in TOOLCHAIN_JOBS — " + f"{sorted(declared - TOOLCHAIN_JOBS)}. Add them to the registry so the pin guard covers " + "them, or they will run on an image nothing checks." + ) + + +def test_the_registry_partitions_every_job_in_the_workflow(): + """No job may be in neither list. ANTI-VACUITY with teeth, and the reason it is here: + + a floor like `len(container_jobs) >= 3` would be satisfied by a broken parse that happened to + find four jobs, and would say nothing about a NEW job appearing in a third state nobody + considered. Partitioning the whole `jobs:` map means every job is a decision someone recorded. + """ + all_jobs = set(_jobs(_DOC)) + unclassified = all_jobs - TOOLCHAIN_JOBS - BARE_RUNNER_JOBS + assert not unclassified, ( + f"docker-build.yml has job(s) {sorted(unclassified)} that are in neither TOOLCHAIN_JOBS nor " + "BARE_RUNNER_JOBS. Every job must be one or the other, so that 'runs on the bare runner' is " + "always a recorded decision rather than an omission." + ) + assert not (TOOLCHAIN_JOBS & BARE_RUNNER_JOBS), "a job cannot be in both lists" + assert all_jobs == TOOLCHAIN_JOBS | BARE_RUNNER_JOBS, ( + f"the registry names jobs that do not exist: " + f"{sorted((TOOLCHAIN_JOBS | BARE_RUNNER_JOBS) - all_jobs)}" + ) + + +# ------------------------------------------------------------------------------------------------ +# MUTATION PROOFS — disarm the invariant one way at a time, each must be DETECTED (ersatztv#775) +# ------------------------------------------------------------------------------------------------ + + +def _mutants(): + """(id, mutated doc) for each single-job way the invariant can be broken. + + Every container job in turn, not a sample: the interesting drop is whichever job someone + actually edits, and proving detection on only the first would prove the case least likely to + happen (ersatztv#773 §3 Family A, applied to this file's own tests). + """ + for job in sorted(container_jobs(_DOC)): + dropped = copy.deepcopy(_DOC) + del dropped["jobs"][job]["container"] + yield f"{job}-container-removed", dropped + + unpinned = copy.deepcopy(_DOC) + unpinned["jobs"][job]["container"]["image"] = "mcr.microsoft.com/dotnet/sdk:10.0" + yield f"{job}-image-swapped", unpinned + + lookalike = copy.deepcopy(_DOC) + lookalike["jobs"][job]["container"]["image"] = "evil.example/timothy/ersatztv-ci:32747a0" + yield f"{job}-lookalike-registry", lookalike + + skewed = copy.deepcopy(_DOC) + skewed["jobs"][job]["container"]["image"] = f"{IMAGE_REPO}:deadbee" + yield f"{job}-tag-skewed", skewed + + +_MUTANTS = list(_mutants()) + + +@pytest.mark.parametrize("doc", [m for _, m in _MUTANTS], ids=[i for i, _ in _MUTANTS]) +def test_a_single_job_losing_its_pin_is_DETECTED(doc): + """The proof this guard can go red. Without it, `pin_population_faults` returning a constant + empty list would satisfy the live assertion above and prove nothing — which is how #621 and + #685 both shipped.""" + assert pin_population_faults(doc), ( + "the population check accepted a workflow in which a container job no longer runs the " + "pinned toolchain image" + ) + + +def test_the_mutation_set_is_not_empty(): + """The positive control for the parametrisation itself. + + If `container_jobs` ever returned an empty set, `_mutants()` would yield nothing, pytest would + collect zero cases from the decorator above, and the file would report all-green having proved + nothing. That is the vacuous-by-sampling shape this whole issue is about, and it is reachable + here through a single broken helper. + """ + expected = 4 * len(TOOLCHAIN_JOBS) + assert len(_MUTANTS) == expected, ( + f"expected 4 mutations per registered job ({expected}), got {len(_MUTANTS)}. A floor rather " + "than an equality here would let a `container_jobs()` that degraded to 3 of 5 jobs pass " + "while silently testing less — the message would still claim 4 per job." + ) + + +def test_docker_build_is_the_ONLY_workflow_pinning_the_toolchain_image(): + """This file reads ONE workflow, which is itself a scope mirror needing its own check. + + `WORKFLOW` hardcodes `docker-build.yml`, and the implicit claim — that no other workflow uses + the toolchain image — mirrors a machine-readable source (the glob of `.gitea/workflows/*.yml`) + that nothing consulted. `renovate.yml` already declares a `container:` with a different image, + so the shape is live. A future workflow adopting `ersatztv-ci:` would acquire no pin-population + guard, no single-tag check and no partition, silently, while `pin_population_faults`'s own error + text claims "All container jobs must run the same toolchain image". + + Found by cold review, which correctly noted this file criticises `MARKED_JOBS` for exactly this + and then shipped the same shape without even the dated comment `MARKED_JOBS` carries. + + Checked by PARSING each workflow's `container.image`, not by grepping the file. A text search + reports `ci-image.yml`, which names the image because it BUILDS and PUSHES it — a producer, not + a consumer. Grepping would have made this test permanently red on a correct tree, which is the + fastest route to a correct guard being deleted. + """ + others = [] + for p in sorted(WORKFLOWS_DIR.glob("*.yml")): + if p.name == WORKFLOW.name: + continue + doc = yaml.safe_load(p.read_text()) or {} + for name, job in (doc.get("jobs") or {}).items(): + if not isinstance(job, dict): + continue + image = str((job.get("container") or {}).get("image", "")) + # Keyed on the IMAGE REPOSITORY, not on `_PIN`'s literal-tag match. A job written as + # `image: :${{ matrix.tag }}` runs on the toolchain image but fails `_PIN`, so + # keying on the pin would have let a templated tag slip the whole check — found by cold + # review, which constructed exactly that. The tag being an expression is itself a fault + # (nothing could then verify WHICH image ran), so this reports the job either way. + if image.startswith(f"{IMAGE_REPO}:"): + others.append(f"{p.name}:{name}") + assert not others, ( + f"{sorted(others)} run container jobs on the CI toolchain image, but this file only checks " + f"{WORKFLOW.name}, so they have no pin-population guard at all. Extend the check to cover " + "them rather than leaving the coverage implied." + ) + + +def test_the_shell_guards_grep_sees_the_same_tags_the_jobs_run(): + """Ties the two halves together, so they cannot drift into disagreeing about the subject. + + `ci-image-pin` reads the file with a grep for `ersatztv-ci:`. This compares what that grep + sees against what the parsed jobs actually run. + + DISTINCT VALUES rather than a count, deliberately. The counts legitimately differ: the file's + header comment at docker-build.yml:32 documents the pin in prose, so the shell guard's grep + reads SIX strings where the YAML has five pinned jobs. Asserting on the count would either fail + today or have to hardcode "+1 for the comment", which breaks the moment a second comment + mentions the pin. + + What actually has to hold for the shell guard's verdict to be sound is that its `sort -u` set + equals the set of tags the jobs really run. Comparing the distinct sets says exactly that — and + as a free side effect it makes the header comment SELF-CHECKING: bump the five image lines and + forget the comment, and the sets diverge here with a message naming both, instead of the shell + guard reporting "pins MORE THAN ONE ersatztv-ci tag" and pointing at prose. + """ + text = WORKFLOW.read_text() + grepped = {m for m in re.findall(r"ersatztv-ci:([0-9a-f]+)", text)} + parsed = set(pinned_jobs(_DOC).values()) + assert grepped == parsed, ( + f"ci-image-pin's grep sees the distinct tags {sorted(grepped)} but the parsed container " + f"jobs run {sorted(parsed)}. A tag mentioned in the file but not run by any job (a stale " + "header comment) makes the shell guard's 'MORE THAN ONE pin' check fire on prose; a tag " + "run but not greppable means the shell guard is not checking that job at all." + ) diff --git a/scripts/tests/test_guard_inventory.py b/scripts/tests/test_guard_inventory.py new file mode 100644 index 000000000..34282ee8a --- /dev/null +++ b/scripts/tests/test_guard_inventory.py @@ -0,0 +1,300 @@ +"""docs/guard-inventory.md covers exactly the guards that exist (ersatztv#774, #775). + +THIS FILE IS THE ANSWER TO BOTH ISSUES' "is a mechanical check possible?" QUESTION, and the shape of +the answer matters more than the code. + +What is NOT possible, and must not be attempted: a lint that flags filter-shaped guards by matching +`.filter(` / `.Where(` / `grep` inside guard code. #774 asks for this explicitly and the honest +answer is no. The token is not the defect — `ToolCatalogTests.Every_Query_Parameter_Should_Be_A_ +Declared_Property` filters correctly eight lines from a completeness assertion that must not. A +matcher would flag both, and a matcher that is wrong half the time is waved through until it is +never read, which is the symptom-keyed-guard mistake this repo has now paid for at #644 and #650. +Worse, it is a string predicate over source, and this repo's own record is that a string predicate +takes three or more rounds to get right (#629, #633, #698). Building it would be #774 violating +#774. + +What IS possible, and is what this file does: you cannot mechanically detect that a guard reasons +about a sample, but you CAN mechanically guarantee that every guard has been *classified by someone* +and that its claimed proof exists. That converts both rules from "remember to do this" into "the +suite goes red until you have". Specifically: + + * the guard population is DERIVED — globbed from `.claude/hooks/` and `.husky/`, plus every + `scripts/*.sh|py` referenced by a workflow or a hook — and compared for SET EQUALITY against the + inventory's rows, in both directions; + * every row's `Proof ref` is resolved to a real file and a real `def` in it; + * every row's `Kind` and `Proof` come from a closed vocabulary, so a typo cannot invent a state. + +The residue this leaves, named rather than papered over: nothing here checks that a row's +`MUTATION` claim is TRUE. A test named in the table might merely exercise the guard. That judgement +stays with review, and the table is the thing review reads. +""" + +from __future__ import annotations + +import re +from collections import Counter +from pathlib import Path + +REPO_ROOT = Path(__file__).resolve().parents[2] +INVENTORY = REPO_ROOT / "docs" / "guard-inventory.md" +HOOKS_DIR = REPO_ROOT / ".claude" / "hooks" +HUSKY_DIR = REPO_ROOT / ".husky" +WORKFLOWS_DIR = REPO_ROOT / ".gitea" / "workflows" + +KINDS = {"GUARD", "TOOLING", "PROOF"} +PROOFS = {"MUTATION", "BEHAVIOUR-ONLY", "NONE"} + +# `scripts/x.sh` AND `scripts/tests/x.py`. The first version omitted the `/`, and the consequence was +# not theoretical: the three guard files this inventory shipped with were themselves outside the +# population it checked for completeness, so they acquired no rows and the guard stayed green. Cold +# review found it. A completeness guard blind to its own author's new guards is the defect this +# whole change is about, so the miss is recorded here rather than quietly corrected. +_SCRIPT_REF = re.compile(r"scripts/(?:[a-z0-9_.-]+/)?[a-z0-9_.-]+\.(?:sh|py)") +_ROW = re.compile(r"^\|\s*`([^`]+)`\s*\|([^|]*)\|\s*([A-Z-]+)\s*\|\s*([A-Z-]+)\s*\|([^|]*)\|\s*$", re.M) + +# The prose summary, parsed so it cannot drift from the table it summarises. It already had: +# shipped as "28 guards, 4 tooling … 6 … 3 … 19" against a table holding 27/5/6/3/18, because it was +# a hand-maintained mirror with no equality check — #773's Family C inside the deliverable arguing +# against it. Both reviewers caught it independently. +_SUMMARY = re.compile( + r"(\d+)\s+guards?,\s+(\d+)\s+tooling\s+scripts?,\s+(\d+)\s+proof\s+files?\.\s+" + r"\*\*(\d+)\s+guards?\s+carry\s+a\s+mutation\s+proof;\s+(\d+)\s+(?:are|is)\s+behaviour-only;\s+" + r"(\d+)\s+have\s+none\.\*\*" +) + + +def derived_guard_files() -> set[str]: + """THE AUTHORITATIVE POPULATION, from the filesystem and the call sites — never a list. + + Three sources, unioned. The hook and husky directories are globbed whole, so a new hook is in + the population the moment it exists. The scripts half is discovered by scanning what the + workflows and hooks actually INVOKE, rather than globbing `scripts/` — a script nothing calls is + not a guard, and globbing would drag in every helper and make the inventory a chore that gets + rubber-stamped. + """ + found = { + str(p.relative_to(REPO_ROOT)) for p in HOOKS_DIR.glob("*.sh") + } | { + str(p.relative_to(REPO_ROOT)) for p in HUSKY_DIR.iterdir() if p.is_file() + } | { + # `pr-checks.yml` runs `pytest scripts/tests` as a directory, so every file in it is + # invoked and none is individually named anywhere. Globbing is the only derivation that + # matches how they actually run. + str(p.relative_to(REPO_ROOT)) for p in (REPO_ROOT / "scripts" / "tests").glob("test_*.py") + } + + callers = list(WORKFLOWS_DIR.glob("*.yml")) + list(HOOKS_DIR.glob("*.sh")) + callers += [p for p in HUSKY_DIR.iterdir() if p.is_file()] + for caller in callers: + for ref in _SCRIPT_REF.findall(caller.read_text()): + if (REPO_ROOT / ref).exists(): + found.add(ref) + return found + + +def wired_hook_files() -> set[str]: + """Hooks reachable from `.claude/settings.json` or a husky hook — WIRING, not existence. + + Directory membership is not execution. A hook whose settings.json registration is deleted keeps + its file, keeps its inventory row, and stops running — and the table would go on describing a + working guard. That is #631 and #719's shape ("wired is not running") one level down, and it was + the derivation's blind spot until cold review named it. + + COMMENT LINES ARE STRIPPED from the husky hooks first, and that is not a refinement — the first + version of this function counted a mention anywhere, and `.husky/pre-commit:7` reads + + # CI where a base ref exists). Fail-open shim — see .claude/hooks/decisions-guard.sh. + + one line above the real invocation. Delete line 8 and keep line 7 and the hook would still have + read as wired, which is the exact substitution of mention for invocation this function exists to + stop, reproduced inside the fix for it. `.claude/settings.json` needs no stripping: JSON has no + comments, so every occurrence there is in a real command string. + """ + text = (REPO_ROOT / ".claude" / "settings.json").read_text() + for husky in HUSKY_DIR.iterdir(): + if husky.is_file(): + text += "\n".join( + line for line in husky.read_text().splitlines() + if not line.lstrip().startswith("#") + ) + return { + str(p.relative_to(REPO_ROOT)) for p in HOOKS_DIR.glob("*.sh") if p.name in text + } + + +def inventory_rows() -> list[tuple[str, str, str, str]]: + """(guard, kind, proof, proof_ref) for each table row.""" + rows = [] + for guard, _blocks, kind, proof, ref in _ROW.findall(INVENTORY.read_text()): + rows.append((guard.strip(), kind.strip(), proof.strip(), ref.strip())) + return rows + + +# ------------------------------------------------------------------------------------------------ +# ANTI-VACUITY FIRST — a row regex that stopped matching would make every assertion below compare +# empty sets and report a fully-covered inventory. That is the failure this file exists to prevent, +# so it is checked before anything depends on it. +# ------------------------------------------------------------------------------------------------ + + +def test_the_table_actually_parsed(): + rows = inventory_rows() + assert len(rows) >= 25, ( + f"only parsed {len(rows)} rows out of {INVENTORY.name} — the row pattern has stopped " + "matching the table's markdown, so the coverage assertions below are vacuous." + ) + assert len(derived_guard_files()) >= 25, "the guard discovery walk found almost nothing" + + +def test_no_duplicate_rows(): + """Two rows for one guard would let one satisfy the set comparison while the other says + anything at all — including a fabricated proof.""" + guards = [g for g, _, _, _ in inventory_rows()] + dupes = sorted({g for g in guards if guards.count(g) > 1}) + assert not dupes, f"{INVENTORY.name} has duplicate rows for {dupes}" + + +# ------------------------------------------------------------------------------------------------ +# SET EQUALITY, BOTH DIRECTIONS +# ------------------------------------------------------------------------------------------------ + + +def test_the_inventory_covers_exactly_the_guards_that_exist(): + listed = {g for g, _, _, _ in inventory_rows()} + found = derived_guard_files() + + missing = sorted(found - listed) + assert not missing, ( + f"these guard files exist but have no row in {INVENTORY.name}: {missing}. Every guard must " + "be classified — add a row giving what it blocks, whether it is a GUARD or TOOLING, and " + "whether it ships a mutation proof. An unclassified guard is one nobody has decided is " + "load-bearing, which is how #631's suite ran nowhere for months." + ) + phantom = sorted(listed - found) + assert not phantom, ( + f"{INVENTORY.name} lists {phantom}, which no longer exist or are no longer invoked by any " + "workflow or hook. A row for a guard that does not run reads as coverage and is not." + ) + + +# ------------------------------------------------------------------------------------------------ +# THE CLAIMS IN EACH ROW RESOLVE +# ------------------------------------------------------------------------------------------------ + + +def test_every_row_uses_the_closed_vocabulary(): + for guard, kind, proof, _ref in inventory_rows(): + assert kind in KINDS, f"{guard}: Kind {kind!r} is not one of {sorted(KINDS)}" + assert proof in PROOFS, f"{guard}: Proof {proof!r} is not one of {sorted(PROOFS)}" + + +def test_every_claimed_proof_names_a_test_that_exists(): + """The half that makes the table load-bearing rather than decorative. + + A row claiming MUTATION with a `Proof ref` that no longer resolves is worse than a row claiming + NONE: it tells the next reader this guard is covered. Renaming a test then silently converts a + proven guard into an unproven one that still reads as proven, and nothing else in the repo + would notice. + """ + for guard, _kind, proof, ref in inventory_rows(): + if proof == "NONE": + assert ref in ("—", "-", ""), f"{guard}: Proof is NONE but a ref is given ({ref!r})" + continue + + assert "::" in ref, f"{guard}: Proof is {proof} but the ref {ref!r} is not file::function" + filename, func = ref.strip("`").split("::", 1) + path = REPO_ROOT / "scripts" / "tests" / filename + assert path.exists(), f"{guard}: proof ref names {filename}, which does not exist" + assert re.search(rf"^def {re.escape(func)}\(", path.read_text(), re.M), ( + f"{guard}: {filename} has no `def {func}(`. The proof ref is stale — either the test " + "was renamed (update the row) or it was deleted (this guard is now unproven, and the " + "row must say NONE)." + ) + + +def test_every_hook_file_is_actually_WIRED(): + """A hook file nothing registers is dead code holding an inventory row that reads as coverage.""" + on_disk = {str(p.relative_to(REPO_ROOT)) for p in HOOKS_DIR.glob("*.sh")} + unwired = sorted(on_disk - wired_hook_files()) + assert not unwired, ( + f"these hook files exist and have inventory rows but are referenced by neither " + f".claude/settings.json nor any .husky/ hook: {unwired}. They do not run. Either wire them " + "or delete them — a row for a hook that never fires is the coverage claim #631 paid for." + ) + + +def test_proof_rows_do_not_themselves_claim_a_proof(): + """`PROOF` exists to stop an infinite regress, and the regress is not hypothetical. + + Once `scripts/tests/*.py` entered the population, every mutation proof became a row needing a + proof of its own, and so on. `PROOF` marks a file whose job IS to prove another guard; grading + it would ask what proves the prover, forever. Files under `scripts/tests/` that enforce a repo + invariant with no separate guard file behind them are `GUARD`, not `PROOF`, and are graded + normally — that is the honest place to draw the line. + """ + for guard, kind, proof, ref in inventory_rows(): + if kind == "PROOF": + assert proof == "NONE", f"{guard} is PROOF but claims Proof {proof}" + assert guard.startswith("scripts/tests/"), ( + f"{guard} is marked PROOF but does not live in scripts/tests/" + ) + assert ref in ("—", "-", ""), f"{guard}: PROOF rows carry no proof ref" + + +def test_every_proof_ref_points_at_a_row_marked_PROOF(): + """Ties the two halves of the table together. A guard citing a test that the table does not + classify as a PROOF means the population and the refs disagree about what that file is.""" + rows = inventory_rows() + proof_files = {g for g, k, _, _ in rows if k == "PROOF"} + for guard, _kind, proof, ref in rows: + if proof == "NONE": + continue + filename = ref.strip("`").split("::", 1)[0] + path = f"scripts/tests/{filename}" + # A test-file GUARD may cite ITSELF: its mutation cases live in the same file, because the + # thing it guards is a repo invariant rather than another script. Splitting those into a + # separate file to satisfy the table would be bookkeeping driving the code. + if path == guard: + continue + assert path in proof_files, ( + f"{guard} cites {filename} as its proof, but that file is not a PROOF row in this " + "table. The citation and the classification must agree." + ) + + +def test_the_summary_counts_match_the_table(): + """The prose is DERIVED-checked, not hand-maintained. + + It shipped wrong the first time — "28 guards, 4 tooling … 19 have none" against a table holding + 27/5/…/18 — because it was a mirror with no equality check. Both cold reviewers found it + independently, which is the clearest possible evidence that a summary nobody checks is a summary + nobody can trust. + """ + rows = inventory_rows() + kinds = Counter(k for _, k, _, _ in rows) + grades = Counter(p for _, k, p, _ in rows if k == "GUARD") + + m = _SUMMARY.search(INVENTORY.read_text()) + assert m, ( + "could not find the summary sentence in the expected shape. It must read exactly like: " + "`N guards, N tooling scripts, N proof files. **N guards carry a mutation proof; N are " + "behaviour-only; N have none.**` — if you reword it, update `_SUMMARY` in the same commit, " + "because an unparsed summary is an unchecked one." + ) + claimed = tuple(int(g) for g in m.groups()) + actual = ( + kinds["GUARD"], kinds["TOOLING"], kinds["PROOF"], + grades["MUTATION"], grades["BEHAVIOUR-ONLY"], grades["NONE"], + ) + assert claimed == actual, ( + f"the summary claims (guards, tooling, proofs, mutation, behaviour-only, none) = {claimed} " + f"but the table holds {actual}." + ) + + +def test_tooling_rows_never_claim_a_proof(): + """A TOOLING row asserting nothing cannot have a proof that it can go red, and letting one + carry a ref would quietly inflate the coverage count at the bottom of the inventory.""" + for guard, kind, proof, _ref in inventory_rows(): + if kind == "TOOLING": + assert proof == "NONE", f"{guard} is TOOLING but claims Proof {proof}" diff --git a/scripts/tests/test_post_review_verdict.py b/scripts/tests/test_post_review_verdict.py index f573d82d5..dbf27844a 100644 --- a/scripts/tests/test_post_review_verdict.py +++ b/scripts/tests/test_post_review_verdict.py @@ -330,3 +330,47 @@ def test_a_failed_HEAD_RECHECK_writes_no_status(gitea): assert result.returncode != 0 assert gitea.statuses() == [], ( "a status was written even though the head/base re-read failed — nothing was confirmed") + + +# --- write-side polarity, the half the #774 rescue initially missed ------------------------------ + + +# Kept deliberately in step with POSITIVE_WORDS/NEGATIVE_WORDS in test_check_review_verdict.py. +# The two lists are NOT compared to each other, and this file does not claim they agree — proving +# that needs one shared vocabulary both scripts read, which is ersatztv#788. What each side proves +# is its OWN polarity: that these established tokens still map the way reviewers rely on. +WRITE_POSITIVE = ["MERGEABLE", "APPROVED", "LGTM"] +WRITE_NEGATIVE = ["BLOCKED", "NOT-MERGEABLE"] + + +@pytest.mark.parametrize( + ("word", "expected"), + [(w, "success") for w in WRITE_POSITIVE] + [(w, "failure") for w in WRITE_NEGATIVE], +) +def test_each_verdict_word_posts_its_established_polarity(word, expected, gitea): + """The WRITE side of the polarity pair, added after cold review found only the read side. + + ersatztv#774 withdrew a test that compared the two verdict vocabularies by parsing shell, and + rescued its disjointness half into test_check_review_verdict.py. Review of that rescue found it + covered only `check-review-verdict.sh`: an in-memory mutation adding `BLOCKED` to the SUCCESS + arm here produced a write-side overlap the deleted test caught and the rescue did not, because + the rescue never executes this script. That was a real, undisclosed loss and this closes it. + + `case` takes the FIRST matching arm, so a token listed in both arms is not ambiguous — it + resolves to whichever comes first, exactly as `is_pos` wins on the read side. The consequence is + the same and it is the one that matters: a word a reviewer means as BLOCKED silently posting + `success` writes a green `review-verdict/h10`, which is the required context branch protection + honours. That flips this assertion. + + What this does NOT prove, stated because the parent test was withdrawn for overclaiming: it is + a polarity regression over five established tokens, not a universal disjointness property and + not parity with the read side. A token added to only one script is untested here, not caught. + """ + result = gitea.run("42", word) + assert result.returncode == 0, result.stderr + payload = gitea.statuses()[0]["payload"] + assert payload["state"] == expected, ( + f"'{word}' posted state {payload['state']!r}, expected {expected!r}. `case` takes the first " + "matching arm, so a token that has appeared in the other arm resolves there silently — and " + "a BLOCKED verdict posting `success` writes a green required context." + )