Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions docs/development/review-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -82,3 +82,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b
21. **A priority claim needs a margin bigger than the time it takes to write the post — and it is the least reliable thing to accept when the answer favours you.** Two agents posting 66 seconds apart did not read each other; that gap is inside compose time, so the timestamps contain no ordering fact at all. "Who got there first" feels like something the log settles and usually is not. The rider that makes this a review rule rather than an etiquette note: **"who closed it" is frequently the wrong question.** A residue with two horns gets closed by two people who each killed a different one, and the log renders that identically to a race — so a reviewer arbitrating priority should first check whether the two findings are even about the same thing. Concretely, one instrument ruled out "retention is not running" across 14 nights and said nothing about which pods were protected; the other measured the 876-in-71-protected / 9-outside split the first could not reach. Neither was second. *(Earned: 2026-08-25, in this pod. `#1208`'s residue. I offered "66 seconds apart" as though it settled priority, having accepted a correction that ran in my own favour; @sprint-review pushed back on their own advantage — "since this correction lands in my favour it's the one I should push on hardest" — and that is the behaviour the rule is really asking for. Naming a different second reader would only have moved the error. Related: rule 14, for labelling the object of a credit; and [[feedback-query-the-control-group-and-report-the-residue]], for the neighbouring failure where a query built from one theory can only confirm it.)*

22. **A deduction joining two measurements is valid only if the predicate is time-invariant across the gap. Age predicates never are.** (@sprint-review's wording, and their find.) The dangerous shape is not a bad measurement — it is two *good* ones, taken hours apart, joined by arithmetic that silently assumes they describe the same population. Each number is correct when read alone; the conclusion is about a set that never existed at any single moment. The tell is a predicate containing `NOW()`, `Date.now()`, `age`, `createdAt <`, a TTL, a lease expiry, or a retention window: every row in the store is continuously crossing that boundary, so "the set matching P" is a function of *when you asked*, and two questions asked at different times get answers about two different sets. **Nothing errors, and no amount of re-checking either measurement finds it**, because neither measurement is wrong — which is why this survives the ordinary discipline of verifying your inputs. The check is to state the instant each measurement describes and ask whether the join needs them equal. **The instant you want is rarely the one to hand:** a measurement usually reaches you inside a message, and the message's own timestamp is what your tooling surfaces — the measurement's is buried in the prose, or absent, in which case the honest move is to ask rather than infer. That asymmetry is why the wrong instant is the *default* rather than a slip, and it is why "state the instant" is not by itself enough: a reader who states the instant they *believe* the measurement describes passes the check and still makes the error. If the join needs them equal, re-measure both at one instant, or narrow the claim to the population the earlier instant covers and say which rows fell outside it. The narrowing is usually cheap and is always the honest move: **a conclusion that quietly covers a slightly different population than the one it names is worse than a smaller conclusion that names its own edge.** Rider, because the failure has a natural second act: the leftover rows are *undetermined by the argument*, not refuted by it — do not report them as negative findings, and say what would settle them (here, one more cycle, after which they will have been eligible for a full window). *(Earned: 2026-08-25, in this pod, on the `#1208` retention question. Twenty nights of `totalDeleted` proved the cron deletes; a same-day count showed 885 rows past 30 days across 19 non-exempt pods; since `deleteOlderThan` is a single statement — `created_at < NOW() - $1::interval AND pod_id != ALL($2)` — one predicate cannot both match and not match on age, so the surviving rows had to be inside `$2`, i.e. Pro-protected. Valid in form, and it over-reached: the count was taken ~3.9 hours after the 03:00:00Z run, and 9 rows had aged past the cutoff in between. The gap figure in the first draft of this entry was itself wrong — “~11½ hours”, derived from when the count was *read in conversation* rather than when it was *taken*, which is this rule's own error one level up. They were never candidates for the run being reasoned about. Corrected claim: 876 rows across 17 pods provably protected, 2 pods undetermined. @sprint-review caught it against a conclusion that agreed with their own prior finding. Related: rule 16, for a mechanism that explains an observation without being evidence for it — same family, one measurement over.)*

23. **A change is not covered because its consumer is covered. Test the half that COMPUTES the value, not only the half that receives it.** The passing shape looks like real coverage and is the same defect three times on one stack: a component takes the interesting value as a prop, a test hands it that prop directly, and nothing anywhere exercises the code that *derives* it. Each time, deleting the deriving line left the whole suite green — the feature simply stopped happening, silently, with a green board. The tell is structural and visible without running anything: **find the line that decides, and ask which test would fail if you deleted it.** If the answer is a test whose fixture supplies that decision as an input, the answer is none. The fix is not more assertions on the consumer; it is one test that starts from the real trigger — a URL hash, a click, a fetched row — and lets the production code compute its way to the observable outcome. Pair it with a mutation check in both directions: delete the deriving line and see red, restore it and see green, and note the number in the PR so a reader can reproduce the claim rather than trust it. *(Earned three times on the direction-C stack, 2026-09-06, all three found by @sprint-review after the author believed the work was done: (a) #1579's image send — the row rendered an upload manifest and a test pinned the render, while reverting the producer to a bare URL, the exact defect the gate opened on, left 570 green; (b) #1582's quote landing — a test handed `revealMessageId` straight to the transcript, while deleting the branch that computes it from the hash left 571 green and sent the reader into an unexplained history fetch; (c) #1582's thread state — the resting view read correctly in every test while never persisting, because deleting the `PUT /api/messages/:id/collapsed` also left 571 green.)*

24. **A selector that matches nothing reports "absent", and absent reads as a bug in the thing you were measuring.** A smoke check looked for a rail count at `.v2-rail__badge` — a class that does not exist in the markup — and dutifully reported `rail null` alongside a live API count, which was then written up as the rail disagreeing with the API. The rail had been showing the right number the whole time. **A check whose failure mode is an empty match must assert its own subject exists before it asserts anything about the value:** count the nodes first, fail loudly at zero, and only then compare. This is the DOM sibling of the vacuous-`indexOf` guard (rule 12) and of enumerating top-level keys while the field you want is nested — same error, three surfaces. *(Earned 2026-09-06, `scratchpad/pw/smoke-c.js`; the count renders inside `.v2-rail__item-icon`, and the genuine finding underneath — that it is still a MUI Badge rather than the tab-badge twin — was invisible for as long as the selector was wrong.)*
Loading