Skip to content
Merged
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: 3 additions & 1 deletion docs/development/review-checklist.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,12 @@

10. **Say what you did NOT verify.** An unqualified approval on something unchecked is worse than a partial one; a scoped confirmation ("verified module exports, did not read test diffs") tells the next reader exactly where to look. *(Earned: #772 — a design-conformance pass that didn't name its limits let a test-guard defect ride one round longer than it should have.)*

11. **Verify against the current head, not the diff you started reading.** State moves while reviews are written; a correct verdict about a superseded head is noise with confidence behind it. Name the commit you verified. *(Earned: crossed-in-flight incidents at least six times in the sprint that produced this doc — duplicate PR, stale verdicts, stale amendment lists, simultaneous mutual concessions — same failure shape each time; it is endemic to multi-agent rooms, and it is why ADR-017's envelope carries `basis`.)*
11. **Verify against the current head, not the diff you started reading.** State moves while reviews are written; a correct verdict about a superseded head is noise with confidence behind it. Name the commit you verified. **And scale the check to what the claim would stop.** A stale *finding* is noise — someone re-reads it and moves on. A stale *blocker* holds finished work open on a condition that no longer exists, and the author has no way to tell the difference from their side. So the rule is not "re-verify more often," which nobody sustains; it is **re-resolve the head before asserting anything that would hold a merge**, and accept that non-blocking observations will sometimes arrive already-answered. *(Sharpened 2026-08-18: five crossings in thirty minutes on one PR — three cost a paragraph each, two asserted an outstanding blocker that the head under review had already fixed. The two expensive ones were both verdicts about what remained; the cheap ones were observations. Same staleness, opposite cost.)* *(Earned: crossed-in-flight incidents at least six times in the sprint that produced this doc — duplicate PR, stale verdicts, stale amendment lists, simultaneous mutual concessions — same failure shape each time; it is endemic to multi-agent rooms, and it is why ADR-017's envelope carries `basis`.)*

12. **Verify the instrument before trusting a negative result — that it ran, AND what it covered.** A search that finds nothing, a grep with no hits, a log query returning empty: each is indistinguishable from a search that never ran. Confirm the tool works (a known-positive probe, a deliberate query that *should* match) before recording "not present" as a finding. Then confirm its **range**: a filter argument is not a coverage guarantee — `--since=48h` against a store holding 3 hours returns everything it has and reports nothing about the other 45. State the window you actually observed, not the one you asked for; **a negative over an unknown window is not a negative.** Asymmetric on purpose — positives announce themselves, negatives fail silently in the direction you already expected. *(Earned twice in one hour during the #781 disclosure hunt: a `timeout`-not-installed near-miss, then a "no exploitation in 48h" claim that was really 3 hours of retained ingress logs — corrected by its own author minutes after writing the first clause.)*

13. **For a security-adjacent change, ask "does this PR newly *expose* the surface?" — not only "does it *introduce* the defect?"** They are different questions and the first is the one that matters. A leak can be pre-existing in a route yet newly *reachable* because this PR ships the named tool, documented endpoint, or UI affordance that makes an agent or user actually hit it. "Pre-existing, not a blocker" is a verdict about introduction; reachability can still make the PR the thing that turns a latent defect into a live one. *(Earned: #781 — the `/api/agents/runtime/pods` private-pod + `latestSummary` disclosure was a latent route until #781 added `commonly_list_pods` to the standard MCP toolset, making it a named tool every agent sees; a review that weighted only "introduces?" approved it, and the verdict was superseded on the reachability question.)*

14. **When you credit a contribution, label the object so the label *discriminates* — or cite its id.** With two candidate objects in play, a superlative names neither: *"the real finding"*, *"the mechanism"*, *"the sharpest thing here"* read as precise and point at nothing checkable. A contrastive label does the work — *"the **mechanism** is yours; the **principle** is mine"* — because it distinguishes the candidates rather than ranking them, and any reader can then confirm it from the text alone. **An id (message id, commit SHA, `file:line`) is simply the description that always discriminates**, which is why it is the cheap default rather than a bureaucratic one. Asking the author what they meant does work, and it is the correct fallback — but it makes one person's recollection load-bearing for a fact the record was supposed to carry, which is the dependency this whole document exists to remove. Applies to blame as much as to credit: a self-accusation is a claim about the record and earns the same check, because agreeing with one is socially free and therefore under-audited. *(Earned: 2026-08-04, the attribution sequence around #802/#803. One sentence — "the gap underneath it is the real finding, and it's yours by refutation" — fused two distinct objects, a refutation and an articulation, under one superlative; two seats then credited each other for the same contribution, in writing, both in good faith, and the tie could only be broken by its author's memory of what they meant. A sibling message that labelled contrastively resolved from the text, by anyone, with no author present. Nine instances that day, most of them inside messages correcting the previous one.)*

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