Team-Commonly · lilyshen0722 · Aug 23, 2026 · Aug 23, 2026
diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md
@@ -52,3 +52,5 @@
 15. **Ask of every PR: which sentences in this repo were true this morning and are false now?** A diff shows changed *code*; it never shows the prose that described the old behaviour, so a comment three files away — or five lines away, inside the diff itself — keeps asserting a premise the change has just removed. The mechanical version is cheap and finds it: for each behaviour the PR alters, `git grep` the constant, env var, status value, threshold or tool name it touches, and read every prose hit. **The author is structurally the last person who will notice**, because they hold the pre-change model in their head as the thing being fixed — which is why this is a reviewer's rule and not an author's. Two riders. First, the stale sentence is usually *load-bearing*: it is the stated justification for a design decision ("WHY X IS NOT OPTIONAL"), so the next reader inherits a false model of the system rather than a cosmetic inaccuracy. Second, the conclusion of a stale comment is often still correct for a *different* surviving reason — so the fix is to rewrite the reason, not delete the rule; say which, or the next reader removes a constraint that still holds. Third, **when one stale rationale turns up, read its siblings in the same file before closing.** Comments written minutes apart about the same change age differently: in the #1002 case below, the block at the injection site asserted the removed behaviour in the present tense while the block forty lines up described it in the past tense and was correct. Finding the first is evidence about neither the second nor the file — the sweep is per-comment, not per-file-verdict. *(Earned three times in one session, 2026-08-18, all in careful PRs by the author who knew the subject best: #1001 removed the 30-minute pending delete while `models/OnboardingSilenceEpisode.ts:18–19` went on citing "pending rows are deleted at `AGENT_EVENT_STALE_PENDING_MINUTES` (default 30)" as the reason its event snapshot is NOT OPTIONAL; #1002 collapsed a double log emission while the comment it **added** in the same diff argued "each failure already prints twice" as the reason to stamp both sinks, a behaviour its own new test now forbids; and #997 — the AX entry being written concurrently — catalogues the identical defect on two product surfaces. The third instance is the tell: a room that can name this failure in the product could not see it in its own diffs.)*
 
 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.)*
Original file line number	Diff line number	Diff line change
Expand Up		@@ -52,3 +52,5 @@
		15. Ask of every PR: which sentences in this repo were true this morning and are false now? A diff shows changed code; it never shows the prose that described the old behaviour, so a comment three files away — or five lines away, inside the diff itself — keeps asserting a premise the change has just removed. The mechanical version is cheap and finds it: for each behaviour the PR alters, `git grep` the constant, env var, status value, threshold or tool name it touches, and read every prose hit. The author is structurally the last person who will notice, because they hold the pre-change model in their head as the thing being fixed — which is why this is a reviewer's rule and not an author's. Two riders. First, the stale sentence is usually load-bearing: it is the stated justification for a design decision ("WHY X IS NOT OPTIONAL"), so the next reader inherits a false model of the system rather than a cosmetic inaccuracy. Second, the conclusion of a stale comment is often still correct for a different surviving reason — so the fix is to rewrite the reason, not delete the rule; say which, or the next reader removes a constraint that still holds. Third, when one stale rationale turns up, read its siblings in the same file before closing. Comments written minutes apart about the same change age differently: in the #1002 case below, the block at the injection site asserted the removed behaviour in the present tense while the block forty lines up described it in the past tense and was correct. Finding the first is evidence about neither the second nor the file — the sweep is per-comment, not per-file-verdict. (Earned three times in one session, 2026-08-18, all in careful PRs by the author who knew the subject best: #1001 removed the 30-minute pending delete while `models/OnboardingSilenceEpisode.ts:18–19` went on citing "pending rows are deleted at `AGENT_EVENT_STALE_PENDING_MINUTES` (default 30)" as the reason its event snapshot is NOT OPTIONAL; #1002 collapsed a double log emission while the comment it added* in the same diff argued "each failure already prints twice" as the reason to stamp both sinks, a behaviour its own new test now forbids; and #997 — the AX entry being written concurrently — catalogues the identical defect on two product surfaces. The third instance is the tell: a room that can name this failure in the product could not see it in its own diffs.)*

		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.)