From d4874b620fe0db667f8e28f7dd02430f7daf4c98 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 18 Aug 2026 05:23:47 -0700 Subject: [PATCH 1/3] docs(review): a diff never shows the prose it just falsified Three times on 2026-08-18, in careful PRs by the author who knew the subject best, a change invalidated a comment it did not touch: #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. #997 the AX entry drafted the same hour, cataloguing this identical defect on two product surfaces. The third is the tell. A room that could name the failure in the product could not see it in its own diffs, because the author holds the pre-change model in their head as the thing being fixed. That makes it a reviewer's rule rather than an author's, which is why it lands here rather than in REVIEW.md. The check is mechanical and cheap: for each behaviour a PR alters, git grep the constant, env var, status value, threshold or tool name it touches, and read every prose hit. Two riders included because both changed what the fix should be -- the stale sentence is usually the stated justification for a design decision, so the next reader inherits a false model rather than a typo; and its conclusion is often still correct for a different surviving reason, so rewrite the reason rather than deleting the rule. Ran the rule against this diff: nothing states the checklist's length or enumerates its contents (ADR-019 cites rule 9 by number and is unaffected by an append), so no prose here goes stale. Rule 14 verified byte-identical. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 4d0de2ead..0a34e0160 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -48,3 +48,5 @@ 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. *(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.)* From ecf0f77133b635365cc30342917d55062420ff47 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 18 Aug 2026 05:53:57 -0700 Subject: [PATCH 2/3] =?UTF-8?q?docs(review):=20rule=2015=20=E2=80=94=20rea?= =?UTF-8?q?d=20the=20stale=20comment's=20siblings=20too?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Added by @pod-architect, from the case that earned the rule. Having found the onError block asserting a premise its own commit removed, they went back and read the collapse-block comment forty lines up — and it was clean: past tense throughout, describing behaviour the code now prevents rather than claiming it still happens. Two comments written minutes apart about the same change, one aged badly and one didn't. So finding the first says nothing about the second in either direction, and a reviewer who stops at the first hit closes the file with the other still wrong. The sweep is per-comment. 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 0a34e0160..fd8543988 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -49,4 +49,4 @@ 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. *(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.)* +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.)* From db34029ebd694ab52abf37a85237a964d12796c8 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 18 Aug 2026 06:22:52 -0700 Subject: [PATCH 3/3] =?UTF-8?q?docs(review):=20rule=2011=20=E2=80=94=20sca?= =?UTF-8?q?le=20re-verification=20to=20what=20the=20claim=20would=20stop?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rule 11 already says to verify against the current head. Tonight showed the cost is asymmetric in a way worth stating, because it changes what the rule asks for. Five crossings in thirty minutes on one PR. Three were findings that turned out already-known: someone re-read them and moved on, cost a paragraph each. Two were assertions that a blocker remained outstanding when the head under review had already fixed it -- and one of those was offered as the reason to hold a live-defect fix while the defect was still shipping. Same staleness, opposite cost. The difference is not accuracy, it is force: a stale observation is noise, a stale blocker stops work, and the author on the other side cannot tell which they are looking at. So the rule is not "re-verify more often" -- nobody sustains that across a long review. 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. 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 fd8543988..3c35a315e 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -41,7 +41,7 @@ 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.)*