diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 30ca4b851..183058a46 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -53,4 +53,6 @@ 16. **A mechanism that explains the observation is not evidence for it — make it predict something, then check that.** When a state looks wrong, the tempting move is to find the machinery that would produce it. That machinery is often real, and every link in it independently true, and *none of that connects the chain to the thing you saw.* The tell is that the explanation was assembled after the observation and has not yet been asked to forbid anything. So before you commit a diagnosis or hand it to someone as actionable, name one consequence it *requires* — some other PR, row, or run that must look a particular way if you are right — and go look. If the chain is sound the check is cheap and you gain a second data point; if it is invented, this is the only step that will tell you. **The corollary is that a pipeline caught mid-flight is indistinguishable from a terminal state**, so a status read once is a snapshot, not a finding: re-read it after the in-flight work completes before you build anything on top of it. *(Earned: 2026-08-22, #1135. A docs-only PR read `MERGEABLE/UNSTABLE` with 10 checks against a sibling's 11; the missing one was `E2E Tests`, and `playwright.yml` really does filter on `frontend/** · backend/** · e2e/** · playwright.config.*`, which a `docs/**` diff really does fail to match, so the workflow really never dispatches. Every link true. The conclusion drawn — that such a PR can therefore never reach `CLEAN` and that a merge-only-when-`CLEAN` rule deadlocks on documentation — was false: `E2E Tests` is not required, and the `UNSTABLE` was one check still **pending**, not one missing. It cleared on its own. The invented mechanism made a checkable prediction — two sibling docs PRs must also never go `CLEAN` — and both settled `MERGEABLE/CLEAN` at 10 checks minutes later. That was one command, never run, because the chain already felt verified. It was committed to an audit entry and broadcast to the room as something to work around.)* -17. **A mutation test proves a term matters to the suite. It cannot tell you the suite's shape is real.** Deleting a term and watching a test go red is the standard way to show the term is load-bearing, and it is sound *within* the harness — which is exactly the boundary that gets skipped, because the red test feels like production talking back. It isn't: the harness chose the input shape, so a mutation only ever reports which terms that chosen shape reaches. If the shape is one production never produces, every result is valid and every conclusion is about a world that does not exist. **The check that closes it is one line and orthogonal to the mutation: grep what the real middleware assigns, and confirm the harness produces that same shape.** Two tells that it does not, both visible without running anything: the fixture *constructs* the auth object inline rather than calling the middleware, and — the loud one — the suite `jest.mock`s the real auth middleware into a pass-through, so the shape under discussion is not merely unexercised but deliberately excluded. Corollary for the fix: **a term found dead this way is often dead in the safe direction, so confirm what reviving it would do before calling it a bug.** *(Earned: 2026-08-22, the `tasksApi` identity terms. `resolveAgentInstanceId` reads `req.user.isBot`, while `agentRuntimeAuth` assigns only `req.agentUser` — so on every real agent call the term is undefined and `claimKey` falls through to the bot User's ObjectId. Dropping `req.user?.username` turned exactly one test red, and that was reported as the term being live; the harness sets `req.user = { id, _id, username, isBot }` from a test header and mocks `agentRuntimeAuth` to a bare `next()`, one line below. The reviewer had quoted that shim in their own review of the same file. Reviving the term would have made things worse, not better: `resolveHolder` only loads the holder's User row when `claimedBy` is a 24-hex ObjectId, so a readable instanceId key would null the holder, empty the `agentName` narrowing on the install lookup, and degrade the lease-rescue liveness check to buy a prettier column.)* +17. **A mutation test proves a term matters to the suite. It cannot tell you the suite's shape is real.** Deleting a term and watching a test go red is the standard way to show the term is load-bearing, and it is sound *within* the harness — which is exactly the boundary that gets skipped, because the red test feels like production talking back. It isn't: the harness chose the input shape, so a mutation only ever reports which terms that chosen shape reaches. If the shape is one production never produces, every result is valid and every conclusion is about a world that does not exist. **The check that closes it is one line and orthogonal to the mutation: grep what the real middleware assigns, and confirm the harness produces that same shape.** Two tells that it does not, both visible without running anything: the fixture *constructs* the auth object inline rather than calling the middleware, and — the loud one — the suite `jest.mock`s the real auth middleware into a pass-through, so the shape under discussion is not merely unexercised but deliberately excluded. Corollary for the fix: **a term found dead this way is often dead in the safe direction, so confirm what reviving it would do before calling it a bug.** And a rider on the closing check itself, because it has its own failure mode: **bind the projection to its own query, not to line proximity.** `grep '\.select(' ` near an assignment finds the wrong query whenever two run in the same block — a `.select('_id')` belonging to an interleaved `Pod.find` reads exactly like a projection on the `User.findOne` thirty-odd lines above it, and turns a live term into a confidently-reported dead one. Read the call the projection is chained to. *(Earned: 2026-08-22, the `tasksApi` identity terms. `resolveAgentInstanceId` reads `req.user.isBot`, while `agentRuntimeAuth` assigns only `req.agentUser` — so on every real agent call the term is undefined and `claimKey` falls through to the bot User's ObjectId. Dropping `req.user?.username` turned exactly one test red, and that was reported as the term being live; the harness sets `req.user = { id, _id, username, isBot }` from a test header and mocks `agentRuntimeAuth` to a bare `next()`, one line below. The reviewer had quoted that shim in their own review of the same file. *(Rider added 2026-08-23 by @sprint-review, from a near-miss running this very check against the fix that earned the rule: `agentRuntimeAuth.ts:98` is `.select('_id').lean()` on an interleaved DM-pod `Pod.find`, thirty-seven lines below the `User.findOne` — read as the User projection it would have condemned `req.agentUser?.username` as dead on arrival. Both `User.findOne` calls are in fact unprojected, so the term is live.)* Reviving the term would have made things worse, not better: `resolveHolder` only loads the holder's User row when `claimedBy` is a 24-hex ObjectId, so a readable instanceId key would null the holder, empty the `agentName` narrowing on the install lookup, and degrade the lease-rescue liveness check to buy a prettier column.)* + +18. **A source assertion is the right instrument when the claim is about absence — because absence of code cannot be demonstrated by execution.** The default suspicion of a test that greps the source instead of running it is correct and should stay: it pins text rather than behaviour, it goes red on a rename that changed nothing, and it is the lazy substitute for the behavioural test somebody did not want to write. But there is one class it is the *only* instrument for. A behavioural test can show that a branch produces the right answer; **it cannot show that a branch is gone.** Dead code is invisible to execution — it never runs, contributes nothing to any assertion, and sits there for the next reader to revive in good faith because it looks like a real path. So the question that separates the two cases is not "is this test grepping?" but **"is the property behavioural or structural?"** If the claim is *this input yields that output*, run it. If the claim is *this call site no longer exists*, *nothing outside this module reads that field*, or *no path constructs the deprecated shape*, then execution is silent on it by construction, and a source assertion is the instrument rather than the shortcut. Two riders. First, **say which it is in the test itself** — an unlabelled grep-test reads as the lazy kind to every future reviewer, and gets deleted by someone applying the default suspicion correctly. Second, **an absence assertion needs a positive control like any other negative**: a grep that matches nothing because the pattern is wrong is indistinguishable from one that matches nothing because the code is gone (see rule 12). There are two ways to match nothing, though, and they are closed by different things — so name which one you closed. **An empty haystack** is closed for free by how the file is loaded: a bare `readFileSync` throws on a bad path, so the source string can never quietly be `''` and every absence assertion in such a suite is already controlled against that half. **A wrong needle** is not, and no property of the loader touches it — a typo'd identifier or an over-anchored regex matches nothing against a file where the code is sitting in plain sight. That half needs the control: assert the same pattern-construction does match something you know is present in that file. The distinction is worth stating because the free half is the one people notice, and citing it reads as having controlled the assertion when the live risk is untouched. *(Rider sharpened 2026-08-25 by @sprint-review, against the exemplar this rule is built on: the `backfillPending` probe carries no positive control and is nonetheless sound, because `read()` is an unguarded `readFileSync`. Naming that mechanism is the point — the test was safe by construction and not by intent, and the behavioural test in the same PR got an explicit `CONTROL:` while the structural one, which is this rider's own subject, did not.)* *(Earned: 2026-08-23, #1149/#1162 — the `backfillPending` absence check was defended as "defensible as a barrier", which is a verdict about this one test rather than a rule anyone could reuse. The generalisation is what makes it decidable: it also explains why converting the ledger-read regex to an executing test was a strict gain in the same review — that property *was* behavioural, and the `catch` branch had no executing coverage at all.)*