From ee07ee199766a5ed748e910bec20d0defc169e49 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 29 Sep 2026 21:35:13 -0700 Subject: [PATCH] docs(checklist): rule 46's remedy means investigate, not corrupted (TASK-214) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto main after #2036 landed rule 47 as cb531845, which took the press ahead of this PR and left it DIRTY. Rule 47 is preserved untouched; only rule 46's line moves, and no renumbering is needed — main runs 45, 46, 47. Collapses the branch's three commits into one, because each rewrote the same single line and the last one already contained all three: 1. The remedy is two steps, not one. As shipped, rule 46 said to treat a Tests-total mismatch as "a broken instrument rather than a result". Too strong: on #2038 at 7271de4c an arm renaming `@media (max-width: 680px)` to 681px moved the total 981 -> 978, and the cause was a GENUINE red — landingAnchorInsets.test.ts builds its fixture at module scope with a private helper that throws when the at-rule is absent, so the suite failed to collect and its three tests left the total. A module-scope throw fails to collect for the same reason a syntax error does, so the `Tests:` line cannot separate a corrupted tree from a real collection-time failure. Flag, then find which suites' counts moved, before reporting a green OR a defect. 2. Two things the summary hides, from sprint-impl's independent run at the same head: the SUITE total does not move (88 -> 88; only the Tests total drops), and the comparator is the TREE, not the branch name — main 03c17da6 read 980 where that head read 981. 3. The provenance names both rows. `(#2038 at 7271de4c, TASK-214)` read as if #2038 were TASK-214; it is TASK-213's PR, and TASK-213 appeared nowhere in the file. Guard in CI's mode against main: 47 rules, 1..47 ascending with no gap, 42 citations all resolve, no rule changed its number or its name. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 23c5fe4c8..8549edb3a 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -166,7 +166,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b ## Reading a mutation's result -46. **An arm's verdict is its test TOTAL compared against BASE, not the presence of the word `failed` — a suite that fails to COMPILE reports zero failures and simply stops contributing tests.** Jest counts a collection failure at the *suite* level only: `Test Suites:` names it, and `Tests:` is a sum over the suites that actually ran, so a corrupted tree prints a line with no failures on it and a total that is quietly smaller. The number stays plausible, which is what makes this worse than the empty case our existing discipline already covers — `Tests: 0 total` or a missing `Tests:` line announces itself, and `875 passed, 875 total` does not. Worse still, **the suite you are watching is usually not one of the casualties.** An invariant suite that reads the module as a *string* (`read('../landing/V2LandingPage.tsx')`) never imports it, so it collects, runs and passes over the corruption; the suites that die are the ones that `import` the module, and they are elsewhere in the tree. So the arm you are pointed at goes green while a tenth of the run vanishes. The defence costs one line: **have every arm print its own total and flag any total ≠ BASE's**, and treat a mismatch as a broken instrument rather than a result. Grepping `^Tests:` alone is not enough — that is the line engineered to look fine. The generalisation is worth holding onto beyond jest: **any per-test aggregate is blind to a suite that produced no tests**, which is also how a coverage threshold, a snapshot count or a "N tests added" check can be satisfied by a tree that does not build. *(Earned: 2026-09-29, TASK-209, during the #2022 gate, and it had already been live in two of that day's gate harnesses before anyone noticed. Reproduced at main `abe19fe0` with `npx jest` in `frontend/`: BASE is `Test Suites: 120 passed, 120 total` and `Tests: 1108 passed, 1108 total`; corrupting **one** line — line 48 of `V2LandingPage.tsx`, a broken JSX tag, which is what a mis-quoted mutation regex produces — gives `Test Suites: 5 failed, 115 passed, 120 total` beside `Tests: 1005 passed, 1005 total`. Zero failed on the line a harness reads, and **103 tests absent**. Name the invocation whenever you quote a pair like this, because the totals are a property of the filter: the same corruption under `npx jest src/v2` reads `5 failed, 83 passed, 88 total` and `875 passed, 875 total` — different totals, the **same 103**, because the absent count belongs to the corrupted module’s importers and not to the filter that happened to be running. The five are exactly the suites that import the module — `landingBarInset`, `landingInstallCopy`, `landingHeroContent`, `V2LandingPage.stats`, `V2Login` — each a `SyntaxError` at collection; `v2-layout-invariants`, the suite nearly every CSS and TSX arm is aimed at, is **not** among them, because it reads the file rather than importing it. The reviewer's own harness printed only the `Tests:` line, so the first run of this corruption was recorded as `870 passed, 870 total` and read as a clean green; the arm that produced it was reported as a real result for several minutes before the suite-level line was looked at. Its own entry rather than a rider on 45, by rule 41's test: 45 sends you to confirm the edit's SITE, and every one of those checks passes here — the site was right, printed, and inside the intended braces — while the run it produced was not a run.)* +46. **An arm's verdict is its test TOTAL compared against BASE, not the presence of the word `failed` — a suite that fails to COMPILE reports zero failures and simply stops contributing tests.** Jest counts a collection failure at the *suite* level only: `Test Suites:` names it, and `Tests:` is a sum over the suites that actually ran, so a corrupted tree prints a line with no failures on it and a total that is quietly smaller. The number stays plausible, which is what makes this worse than the empty case our existing discipline already covers — `Tests: 0 total` or a missing `Tests:` line announces itself, and `875 passed, 875 total` does not. Worse still, **the suite you are watching is usually not one of the casualties.** An invariant suite that reads the module as a *string* (`read('../landing/V2LandingPage.tsx')`) never imports it, so it collects, runs and passes over the corruption; the suites that die are the ones that `import` the module, and they are elsewhere in the tree. So the arm you are pointed at goes green while a tenth of the run vanishes. The defence costs one line: **have every arm print its own total and flag any total ≠ BASE's** — then, before reporting anything, **find out which suites' counts moved** (`jest --json`, compare each result's `assertionResults.length` against the base run). A mismatch is a signal to investigate, **not by itself evidence of a corrupted tree**: a suite that throws at MODULE scope — a top-level `const` calling a helper that raises when what it reads is absent — fails to collect for the same reason a syntax error does, so a *genuine* red surfaces as absent tests too, and the `Tests:` line cannot tell the two apart. Both shapes can occur in **one run**, so the question is never which of the two it was. Two things the summary will not show you. The **suite** total does not move — a suite that fails to collect is still counted, so `88 total` reads the same before and after and only the `Tests:` total drops; do not look for a shrinking suite count. And the comparator is the **tree** under test, not the branch name: the base figure has to come from the unmutated head you are mutating, because a head that adds a test reads one higher than `main` and every arm compared against `main` would then look like a missing test. Reporting either one as the other is the failure this rule exists to stop: call the arm broken and you dismiss a real finding; call it a result and you publish a green. Grepping `^Tests:` alone is not enough — that is the line engineered to look fine. The generalisation is worth holding onto beyond jest: **any per-test aggregate is blind to a suite that produced no tests**, which is also how a coverage threshold, a snapshot count or a "N tests added" check can be satisfied by a tree that does not build. *(Earned: 2026-09-29, TASK-209, during the #2022 gate, and it had already been live in two of that day's gate harnesses before anyone noticed. Reproduced at main `abe19fe0` with `npx jest` in `frontend/`: BASE is `Test Suites: 120 passed, 120 total` and `Tests: 1108 passed, 1108 total`; corrupting **one** line — line 48 of `V2LandingPage.tsx`, a broken JSX tag, which is what a mis-quoted mutation regex produces — gives `Test Suites: 5 failed, 115 passed, 120 total` beside `Tests: 1005 passed, 1005 total`. Zero failed on the line a harness reads, and **103 tests absent**. Name the invocation whenever you quote a pair like this, because the totals are a property of the filter: the same corruption under `npx jest src/v2` reads `5 failed, 83 passed, 88 total` and `875 passed, 875 total` — different totals, the **same 103**, because the absent count belongs to the corrupted module’s importers and not to the filter that happened to be running. The five are exactly the suites that import the module — `landingBarInset`, `landingInstallCopy`, `landingHeroContent`, `V2LandingPage.stats`, `V2Login` — each a `SyntaxError` at collection; `v2-layout-invariants`, the suite nearly every CSS and TSX arm is aimed at, is **not** among them, because it reads the file rather than importing it. The reviewer's own harness printed only the `Tests:` line, so the first run of this corruption was recorded as `870 passed, 870 total` and read as a clean green; the arm that produced it was reported as a real result for several minutes before the suite-level line was looked at. Its own entry rather than a rider on 45, by rule 41's test: 45 sends you to confirm the edit's SITE, and every one of those checks passes here — the site was right, printed, and inside the intended braces — while the run it produced was not a run. The investigate-before-reporting clause was earned separately, on the very next gate — while gating **#2038 at `7271de4c`, which is TASK-213’s PR**, and filed as TASK-214: an arm renaming `@media (max-width: 680px)` to `681px` moved the total 981 → 978, and the cause was not corruption — `landingAnchorInsets.test.ts` builds its fixture at module scope (`const PHONE = mediaBlock(BARE, '@media (max-width: 680px)')`) with a private helper that THROWS when the at-rule is absent, so that suite's three tests left the total while a second suite's ten assertions reddened normally. Both reds were real. The first draft of this rule said to treat the mismatch as a broken instrument, which would have dismissed one of them.)* ## Reaching a mutation's site