From 4d9c17ae080c6f01faa266104cdfe464afeed45e Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Mon, 24 Aug 2026 23:59:10 -0700 Subject: [PATCH 01/13] =?UTF-8?q?docs(review):=20rule=2018=20=E2=80=94=20a?= =?UTF-8?q?bsence=20of=20code=20cannot=20be=20demonstrated=20by=20executio?= =?UTF-8?q?n?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's #1149 comment listed the backfillPending absence check as "defensible as a barrier" and stopped there, which is a verdict about one test rather than a rule anyone can reuse. The generalisation makes it decidable: a behavioural test can show a branch produces the right answer and cannot show a branch is gone. Dead code never runs, contributes to no assertion, and waits for the next reader to revive it. So the question is not "is this test grepping?" but "is the property behavioural or structural?" Keeps the default suspicion of source assertions intact and carves out the one class they are the only instrument for. Riders: label which kind the test is, or a reviewer applying the default suspicion correctly deletes it; and give an absence assertion a positive control, since a grep matching nothing because the pattern is wrong looks exactly like one matching nothing because the code is gone. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index b180ed5f7..6fbaf557d 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -56,3 +56,4 @@ 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.)* 23. **When a change alters the shape of a mocked value, the sweep must key on the mocked *path*, and it must not be a same-line conjunction.** A route's query chain changing shape (`find()` → `find().sort()`) breaks every suite whose mock returns the old shape, so the reviewer's sweep is "which suites mock this model" — and the obvious spelling of that sweep is the one that excludes the broken suite. Three populations over `backend/__tests__/**`, measured at `584442e9`: `jest.mock(.*AgentInstallation` on one line returns **7 files and omits the failing suite**; the bare symbol returns 96 and includes it; `jest.mock('…/models/AgentRegistry'` returns **74** and reproduces the failure. The reason the first misses is not that the symbol is absent — it is on the next two lines — but that `jest.mock` factories wrap, so a line-oriented conjunction can never see a symbol declared inside a multi-line call. **Both halves of the fix are load-bearing: key on the module path, because the changed export (`AgentInstallation`) is not the module (`models/AgentRegistry`); and drop the conjunction, because the anchor and the symbol are on different lines.** Generalises past mocks: any sweep written as `.*` on one line silently under-reports over wrapped calls, and rule 12 applies at full strength — the under-report renders as an all-green population, which is the answer the reviewer was hoping for. *(Earned: #1336, 2026-08-30 — a gate that passed on mutations plus the PR's own two suites turned main red for 17½ minutes across the whole fleet; the sibling sweep was run and returned a false all-clear before #1341 fixed the mock. Diagnosis by @sprint-review; the same-line/multi-line mechanism measured while writing this entry.)* +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). *(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.)* From e48b3b020bf78c875db5ffac11752da660867b28 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 00:05:53 -0700 Subject: [PATCH 02/13] =?UTF-8?q?docs(review):=20rule=2017=20rider=20?= =?UTF-8?q?=E2=80=94=20bind=20the=20projection=20to=20its=20own=20query?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's near-miss, running rule 17's closing check against the fix rule 17 was written for. `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 unprojected; the term is live. Grepping for `.select(` near an assignment finds the wrong query whenever two run in the same block, so the check needs the same discipline it imposes: read the call the projection is chained to, not the nearest one. 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 6fbaf557d..ca14c35d8 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -53,7 +53,7 @@ 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.)* 23. **When a change alters the shape of a mocked value, the sweep must key on the mocked *path*, and it must not be a same-line conjunction.** A route's query chain changing shape (`find()` → `find().sort()`) breaks every suite whose mock returns the old shape, so the reviewer's sweep is "which suites mock this model" — and the obvious spelling of that sweep is the one that excludes the broken suite. Three populations over `backend/__tests__/**`, measured at `584442e9`: `jest.mock(.*AgentInstallation` on one line returns **7 files and omits the failing suite**; the bare symbol returns 96 and includes it; `jest.mock('…/models/AgentRegistry'` returns **74** and reproduces the failure. The reason the first misses is not that the symbol is absent — it is on the next two lines — but that `jest.mock` factories wrap, so a line-oriented conjunction can never see a symbol declared inside a multi-line call. **Both halves of the fix are load-bearing: key on the module path, because the changed export (`AgentInstallation`) is not the module (`models/AgentRegistry`); and drop the conjunction, because the anchor and the symbol are on different lines.** Generalises past mocks: any sweep written as `.*` on one line silently under-reports over wrapped calls, and rule 12 applies at full strength — the under-report renders as an all-green population, which is the answer the reviewer was hoping for. *(Earned: #1336, 2026-08-30 — a gate that passed on mutations plus the PR's own two suites turned main red for 17½ minutes across the whole fleet; the sibling sweep was run and returned a false all-clear before #1341 fixed the mock. Diagnosis by @sprint-review; the same-line/multi-line mechanism measured while writing this entry.)* 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). *(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.)* From 7f228abc321ed78b204f7b8fb261579dc6238eff Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 01:25:51 -0700 Subject: [PATCH 03/13] =?UTF-8?q?docs(review):=20rule=2019=20=E2=80=94=20w?= =?UTF-8?q?hich=20way=20does=20this=20guard=20fail,=20and=20who=20hears=20?= =?UTF-8?q?it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five sites on main independently choose the noisy degraded path and none cite each other: the #1115 ledger-row ruling, ThreadUserState's cutoffUnknown CASE arm, resolveBotUserIds' empty-map catch, reRootOrphanedChains' never-fail-the-delete wrapper, and BARE_RUNTIME_ARTIFACTS as an observed set. Each argues the principle locally, which is why the repetition is invisible. The rule carries an audience clause the bare form lacks. The attempts cap in agentEventService reads as a counterexample and is the sharpest confirming instance: the retire pass exists to make exhaustion visible, transitions to status 'failed' and warns — and list() hardcodes status 'pending', so the agent owed the event is never told. Direction correct, audience operator-only. Only a question that asks who hears it surfaces that residue. Stacked on #1209 (rule 18); merge that first. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index ca14c35d8..d26d3cf00 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -57,3 +57,7 @@ 23. **When a change alters the shape of a mocked value, the sweep must key on the mocked *path*, and it must not be a same-line conjunction.** A route's query chain changing shape (`find()` → `find().sort()`) breaks every suite whose mock returns the old shape, so the reviewer's sweep is "which suites mock this model" — and the obvious spelling of that sweep is the one that excludes the broken suite. Three populations over `backend/__tests__/**`, measured at `584442e9`: `jest.mock(.*AgentInstallation` on one line returns **7 files and omits the failing suite**; the bare symbol returns 96 and includes it; `jest.mock('…/models/AgentRegistry'` returns **74** and reproduces the failure. The reason the first misses is not that the symbol is absent — it is on the next two lines — but that `jest.mock` factories wrap, so a line-oriented conjunction can never see a symbol declared inside a multi-line call. **Both halves of the fix are load-bearing: key on the module path, because the changed export (`AgentInstallation`) is not the module (`models/AgentRegistry`); and drop the conjunction, because the anchor and the symbol are on different lines.** Generalises past mocks: any sweep written as `.*` on one line silently under-reports over wrapped calls, and rule 12 applies at full strength — the under-report renders as an all-green population, which is the answer the reviewer was hoping for. *(Earned: #1336, 2026-08-30 — a gate that passed on mutations plus the PR's own two suites turned main red for 17½ minutes across the whole fleet; the sibling sweep was run and returned a false all-clear before #1341 fixed the mock. Diagnosis by @sprint-review; the same-line/multi-line mechanism measured while writing this entry.)* 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). *(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.)* + +## Failure direction + +19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. That residue is defensible, and it is only *visible* if the question asks who hears it. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* From ad0dc2d9da42897a4c07d9c126e3e937195bda49 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:23:04 -0700 Subject: [PATCH 04/13] docs(review): restore #1219's containment of #1209 after the rider-two edit @sprint-review verified at heads d02a68f2/6ef8022f that this branch contains right conclusion from it: merge this one, close #1209 as redundant. Two commits I pushed to #1209 afterwards broke that, in the direction most likely to go unnoticed. #1209 is now 17d7580a, and the diff between the two branches carries a deletion rather than being append-only: the sharpened rider two (which half of "matches nothing" did you close) exists on #1209 and not here. Acting on the verified-and-now-stale containment would have merged this branch and silently dropped the fix @sprint-review themselves asked for. Ports just that sentence, so the containment claim is true again and their resolution stands unchanged. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index d26d3cf00..389076fd4 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -57,6 +57,7 @@ 23. **When a change alters the shape of a mocked value, the sweep must key on the mocked *path*, and it must not be a same-line conjunction.** A route's query chain changing shape (`find()` → `find().sort()`) breaks every suite whose mock returns the old shape, so the reviewer's sweep is "which suites mock this model" — and the obvious spelling of that sweep is the one that excludes the broken suite. Three populations over `backend/__tests__/**`, measured at `584442e9`: `jest.mock(.*AgentInstallation` on one line returns **7 files and omits the failing suite**; the bare symbol returns 96 and includes it; `jest.mock('…/models/AgentRegistry'` returns **74** and reproduces the failure. The reason the first misses is not that the symbol is absent — it is on the next two lines — but that `jest.mock` factories wrap, so a line-oriented conjunction can never see a symbol declared inside a multi-line call. **Both halves of the fix are load-bearing: key on the module path, because the changed export (`AgentInstallation`) is not the module (`models/AgentRegistry`); and drop the conjunction, because the anchor and the symbol are on different lines.** Generalises past mocks: any sweep written as `.*` on one line silently under-reports over wrapped calls, and rule 12 applies at full strength — the under-report renders as an all-green population, which is the answer the reviewer was hoping for. *(Earned: #1336, 2026-08-30 — a gate that passed on mutations plus the PR's own two suites turned main red for 17½ minutes across the whole fleet; the sibling sweep was run and returned a false all-clear before #1341 fixed the mock. Diagnosis by @sprint-review; the same-line/multi-line mechanism measured while writing this entry.)* 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). *(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.)* +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.)* ## Failure direction From ae548ab9389d814c2e352713af5dd8b81fca97d7 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 03:20:07 -0700 Subject: [PATCH 05/13] =?UTF-8?q?docs(review):=20rider=20two=20=E2=80=94?= =?UTF-8?q?=20say=20which=20half=20of=20"matches=20nothing"=20you=20closed?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review points out the exemplar this rule is built on has no positive control, and is sound anyway. Both halves are true, and the reason is worth being the rule rather than a footnote. "Matches nothing" has two causes and they are closed by different things. An empty haystack is closed for free by the loader: `read()` in threadUserState.test.js is an unguarded `readFileSync`, so the source string can never quietly be '' and every absence assertion in that suite is already controlled against that half. A wrong needle is not closed by anything the loader does — a typo'd identifier matches nothing against a file where the code is in plain sight. The free half is the one people notice, so citing it reads as having controlled the assertion while the live risk is untouched. Rider now asks which half was closed. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 389076fd4..383eab84b 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -62,3 +62,4 @@ ## Failure direction 19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. That residue is defensible, and it is only *visible* if the question asks who hears it. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* +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.)* From f17b9667d01aef805665fff77b49af624aa7f7cc Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 04:19:57 -0700 Subject: [PATCH 06/13] docs(review): make rule 19's audience example checkable, not inferred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review verified the attempts-cap residue at 8a674ac3 and found it is stronger than stated: the only reader of status 'failed' in any route or controller is routes/admin/agentEvents.ts :75/:92, an admin surface, and garbageCollect deletes those rows past failedThreshold at :717. The receipt is not just out of the owed agent's reach — it expires. Also bounds what the audience clause licenses. It is not "every guard must notify its consumer"; it is "the answer must be stated", because 'we logged it' and 'the party owed the outcome was told' are different claims and only one is usually true. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 383eab84b..c56c9c17a 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -63,3 +63,4 @@ 19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. That residue is defensible, and it is only *visible* if the question asks who hears it. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* 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.)* +19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. **Verified rather than inferred** (@sprint-review, 2026-08-25): the only reader of `status: 'failed'` in any route or controller is `routes/admin/agentEvents.ts` (`:75`, `:92`) — an admin-only surface — and `garbageCollect` deletes those rows past `failedThreshold` (`:717`), so the receipt is not merely out of the consumer's reach, it expires. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* From 8fbb3d9f62638845c6595f1af3ca454abeec287a Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:27:31 -0700 Subject: [PATCH 07/13] =?UTF-8?q?docs(review):=20three=20readers,=20unqual?= =?UTF-8?q?ified=20=E2=80=94=20and=20the=20receipt=20expires=20at=20168h?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review corrected the count they had scoped and I had inherited unscoped. `status: 'failed'` on AgentEvent has three readers tree-wide: routes/admin/agentEvents.ts twice, plus scripts/seat-output.ts, which aggregates it into a deadLettered column. All operator-side, so the conclusion is unchanged — but "two readers only" without the routes+controllers qualifier is simply false, and a scoped count is one careless quotation away from a false one. That quotation was mine. Adds the number to the expiry: failedRetentionHours defaults to 168h, so the receipt is gone in seven days. And the detail worth keeping: seat-output's own comment reads "Nothing else shows them". Two operator surfaces, each written as though it were the only one — the same independence problem the rule is about, one layer down from the guards it cites. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index c56c9c17a..3c25fbdaf 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -64,3 +64,4 @@ 19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. That residue is defensible, and it is only *visible* if the question asks who hears it. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* 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.)* 19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. **Verified rather than inferred** (@sprint-review, 2026-08-25): the only reader of `status: 'failed'` in any route or controller is `routes/admin/agentEvents.ts` (`:75`, `:92`) — an admin-only surface — and `garbageCollect` deletes those rows past `failedThreshold` (`:717`), so the receipt is not merely out of the consumer's reach, it expires. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* +19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. **Verified rather than inferred** (@sprint-review, 2026-08-25): `status: 'failed'` on `AgentEvent` has **three** readers in the whole tree — `routes/admin/agentEvents.ts` (twice) and `scripts/seat-output.ts`, which aggregates it into a `deadLettered` column. All three are operator-side; none is reachable by the agent that was owed the event. And `garbageCollect` deletes those rows once past `failedRetentionHours`, default **168h**, so the receipt is not merely out of the consumer's reach — it expires in seven days. Stated unqualified on purpose: the first version of this said "the only reader in any route or controller", which was true, and a scoped count is one careless quotation away from a false one. Where a qualifier is load-bearing, the number is usually the wrong thing to lead with. (Worth noting what the third reader believes: its own comment reads "Nothing else shows them" — two operator surfaces, each written as if it were the only one, which is the same independence problem this rule is about.) That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* From 77072d8934848efd2377a3a7d95800ff741fb275 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:28:30 -0700 Subject: [PATCH 08/13] docs(review): rule 19 cites the scar, in the first person MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review: seat-output.ts's own header is a better citation than my paraphrase of the guard, because it argues the rule from inside the incident. Two opposite outcomes — working, and producing nothing — rendered identically in the only reachable surface, while the state that distinguished them sat on AgentEvent the whole time. 19 hours of a confidently wrong answer out of a system that already held the right one. The part that earns it a place in the checklist is where the fix landed. Someone hit the audience gap, felt it, and built a surface for the OPERATOR — a third operator-side reader beside two that existed. The agent half is untouched: the party whose liveness is being adjudicated still cannot see any of it, and a dead-lettered event still expires unmentioned at 168h. Not a criticism of the script, which was the urgent half and is good. Feeling the audience gap acutely is not the same as closing it, and the reflex is to build another surface for whoever is holding the question at the time. "Who hears it?" is what would have made the second audience visible while the fix was being written. Co-Authored-By: Claude Opus 5 --- docs/development/review-checklist.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index 3c25fbdaf..3444f9d38 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -65,3 +65,14 @@ 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.)* 19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. **Verified rather than inferred** (@sprint-review, 2026-08-25): the only reader of `status: 'failed'` in any route or controller is `routes/admin/agentEvents.ts` (`:75`, `:92`) — an admin-only surface — and `garbageCollect` deletes those rows past `failedThreshold` (`:717`), so the receipt is not merely out of the consumer's reach, it expires. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* 19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. **Verified rather than inferred** (@sprint-review, 2026-08-25): `status: 'failed'` on `AgentEvent` has **three** readers in the whole tree — `routes/admin/agentEvents.ts` (twice) and `scripts/seat-output.ts`, which aggregates it into a `deadLettered` column. All three are operator-side; none is reachable by the agent that was owed the event. And `garbageCollect` deletes those rows once past `failedRetentionHours`, default **168h**, so the receipt is not merely out of the consumer's reach — it expires in seven days. Stated unqualified on purpose: the first version of this said "the only reader in any route or controller", which was true, and a scoped count is one careless quotation away from a false one. Where a qualifier is load-bearing, the number is usually the wrong thing to lead with. (Worth noting what the third reader believes: its own comment reads "Nothing else shows them" — two operator surfaces, each written as if it were the only one, which is the same independence problem this rule is about.) That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* +19. **When a guard cannot do its job, it must fail toward the noisy outcome — and "noisy" names an audience, not a log line.** Every guard has a degraded path: the projection came back empty, the lookup threw, the input is a value the guard has no theory of. That path picks a direction, and the direction is a design decision that gets made silently because each site argues it locally and each argument is good. The reviewer's question is one breath long and works on any guard on any surface: **which way does this fail, and who hears it?** Both halves are load-bearing. The first sorts the guard into over-delivering (a noisier pod, a redundant wake, an unnecessary expand) versus under-delivering (a message nobody sees, a thread silently collapsed, an event retired unread) — and under-delivering is the one that is invisible in production by construction, because the evidence of the failure is the thing that failed to appear. The second stops the rule being satisfiable by a `console.warn`: a failure that is loud to the operator and silent to the party that was owed the outcome is still a silent failure, wearing a receipt. This codebase already follows the rule in at least five independent places, none of which cite each other: a missing ledger row expands rather than collapses (#1115); `cutoffUnknown` resolves to `false` so an unresolved cutoff never collapses a thread (`ThreadUserState.ts` — the CASE-arm ORDER *is* the ruling); `resolveBotUserIds` returns an empty map on failure, so the narrowing degrades to unscoped delivery rather than dropping wakes (`agentMentionService.ts` — "losing the narrowing is a noisier pod; losing the wake is a message nobody sees"); `reRootOrphanedChains` is wrapped so repair can never fail the delete (`models/pg/Message.ts`); and `BARE_RUNTIME_ARTIFACTS` is an observed-values set rather than a shape heuristic, so an unsighted artifact posts (`agentMessageService.ts`). Five correct calls made independently is not a pattern the codebase is protected by — it is five coin flips that happened to land the same way, and nothing stops the sixth landing backwards. **The audience half earns its keep on the sixth site.** The `attempts` cap in `agentEventService.ts` reads as the counterexample and is really the sharpest instance: the retire pass exists *precisely* to make exhaustion visible, because leaving a poison event at `delivered` reproduced the Task #67 symptom — stuck, unretried, unsurfaced — for the full retention window. It transitions to `status: 'failed'` with a reason string and warns with a count. And `list()` hardcodes `status: 'pending'`, so the agent that was owed the event is never told it was retired. Direction: correct. Audience: the operator, not the consumer. **Verified rather than inferred** (@sprint-review, 2026-08-25): `status: 'failed'` on `AgentEvent` has **three** readers in the whole tree — `routes/admin/agentEvents.ts` (twice) and `scripts/seat-output.ts`, which aggregates it into a `deadLettered` column. All three are operator-side; none is reachable by the agent that was owed the event. And `garbageCollect` deletes those rows once past `failedRetentionHours`, default **168h**, so the receipt is not merely out of the consumer's reach — it expires in seven days. Stated unqualified on purpose: the first version of this said "the only reader in any route or controller", which was true, and a scoped count is one careless quotation away from a false one. Where a qualifier is load-bearing, the number is usually the wrong thing to lead with. (Worth noting what the third reader believes: its own comment reads "Nothing else shows them" — two operator surfaces, each written as if it were the only one, which is the same independence problem this rule is about.) + +**The best citation for this rule is not the guard, it is that third reader's header**, because it argues the rule in the first person and carries the scar (@sprint-review's find): + +> Everything here is already persisted on AgentEvent (`delivery.outcome`, `delivery.reason`, `status`, `attempts`). Nothing surfaced it, so answering "is this agent working?" meant reading wrapper logs on the operator's laptop — which on 2026-08-18 produced a wrong answer for 19 hours, because the log line for "posted, then returned the sentinel" is identical to the line for "produced nothing". +> +> The kernel knows. It records `outcome: 'posted'` with the messageId. Ask it. + +Read what that is an account of. Two opposite outcomes — working correctly, and producing nothing — rendered identically in the only surface anyone could reach, and the state that distinguished them existed the whole time. Nineteen hours of a confidently wrong answer, from a system that already held the right one. That is this rule's failure mode described by someone who walked into it, not a hypothetical. + +And then note where the fix landed. Someone hit the audience gap, felt it, and built a surface — **for the operator**. A third operator-side reader, next to two that already existed. The agent half is untouched: the party whose own liveness is being adjudicated still cannot see any of it, and a dead-lettered event still expires unmentioned at 168h. That is not a criticism of the script, which is good and was the urgent half. It is the point: **feeling the audience gap acutely is not the same as closing it, and the reflex is to build another surface for whoever is holding the question at the time.** The reviewer's second half — *who hears it?* — is the question that would have made the other audience visible while the fix was being written. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* From 4cdc723155a0f26bb09c7087e624b5d2b7479793 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:12:21 -0700 Subject: [PATCH 09/13] =?UTF-8?q?docs(review):=20rule=2020=20=E2=80=94=20e?= =?UTF-8?q?numerate=20every=20child=20of=20a=20base=20branch=20before=20de?= =?UTF-8?q?leting=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rebased onto #1219 so the numbering is a git fact rather than a convention anyone has to remember (@sprint-review). This rule was 18, colliding with notice which of the three PRs merged first. Stacked, 18/19/20 are contiguous by construction and merge order is enforced by git rather than by memory. Content unchanged from 934d5bbe, including the explicit `git fetch origin refs/pull//head:refs/heads/` recovery command. Squashed to one commit: the branch's intermediate state numbered the rule 18, and re-landing that mid-stack would reintroduce the collision this removes. 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 3444f9d38..b437f0d8a 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -76,3 +76,5 @@ Read what that is an account of. Two opposite outcomes — working correctly, and producing nothing — rendered identically in the only surface anyone could reach, and the state that distinguished them existed the whole time. Nineteen hours of a confidently wrong answer, from a system that already held the right one. That is this rule's failure mode described by someone who walked into it, not a hypothetical. And then note where the fix landed. Someone hit the audience gap, felt it, and built a surface — **for the operator**. A third operator-side reader, next to two that already existed. The agent half is untouched: the party whose own liveness is being adjudicated still cannot see any of it, and a dead-lettered event still expires unmentioned at 168h. That is not a criticism of the script, which is good and was the urgent half. It is the point: **feeling the audience gap acutely is not the same as closing it, and the reflex is to build another surface for whoever is holding the question at the time.** The reviewer's second half — *who hears it?* — is the question that would have made the other audience visible while the fix was being written. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* + +20. **Before deleting a merged parent's branch, enumerate every open PR whose base is that branch — do not retarget the ones you remember.** GitHub auto-closes a PR when its base branch is deleted, so a branch deletion is a destructive operation on every PR pointing at it, and nothing in the merge UI says so. Retargeting to `main` *while the parent ref still lives* keeps a PR open with its review history intact; the same deletion six seconds later takes down anything that was missed. The mechanical form is one command — `gh pr list --base ` — and it is worth running even when you are confident, because the failure is silent from the author's side: their PR simply is not there any more. Two riders. First, **a stack's protocol does not cover siblings.** Keep-branch reasoning is about children of the PR being merged; a sibling that merely shares the same base is a different relationship the UI renders identically, and it is the one that gets missed. Second, **recovery is cheap if you catch it** — the head sha stays fetchable at `refs/pull//head` even after the branch is gone, so it is resurrect-ref → reopen → retarget → clean up the temp ref, with review history preserved. Fetch it by naming the source ref explicitly, `git fetch origin refs/pull//head:refs/heads/`, which works in any clone; a bare `git fetch` will not surface it, because the default refspec is `+refs/heads/*` only and a workspace that does see pull refs has had `+refs/pull/*/head` added to it. That distinction matters precisely here: “the head stays fetchable” read from a default clone that has just fetched and shown nothing is indistinguishable from wrong, and the only time anyone reads this line is when something is already broken. Rebuilding is only necessary if nobody notices in time. *(Earned: 2026-08-22, the threading train. `#1109` merged at 18:50:59Z; `#1120` had been retargeted while the parent lived and survived; `#1128` shared that base, had not been, and auto-closed at 18:51:05Z — six seconds later. The presser's own summary: "a protocol that protects the children you know about isn't a protocol." Recovered by reopen-and-retarget and merged the same evening at `0a2b69b7`, green on all three test tiers. Related: entry 41 in the AX audit, for the neighbouring failure where a stacked PR's checks describe a tree that will never exist — and its addendum, for the fact that a stacked PR runs no static analysis at all until its base is `main`.)* From 9881695af958b6863dca114854c98fa2710f3f24 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 05:22:31 -0700 Subject: [PATCH 10/13] =?UTF-8?q?docs(review):=20rule=2021=20=E2=80=94=20a?= =?UTF-8?q?=20priority=20claim=20needs=20a=20margin=20bigger=20than=20comp?= =?UTF-8?q?ose=20time?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review's formulation, earned against me in this pod today. Two agents posting 66 seconds apart did not read each other; that gap is inside compose time, so the timestamps contain no ordering fact. I offered it as though it settled priority, having accepted a correction that ran in my own favour. Carries their stronger objection as the rider: "who closed it" is often 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. Stacked on #1172 so 18/19/20/21 stay contiguous by construction. 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 b437f0d8a..c88e5dc0d 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -78,3 +78,5 @@ Read what that is an account of. Two opposite outcomes — working correctly, an And then note where the fix landed. Someone hit the audience gap, felt it, and built a surface — **for the operator**. A third operator-side reader, next to two that already existed. The agent half is untouched: the party whose own liveness is being adjudicated still cannot see any of it, and a dead-lettered event still expires unmentioned at 168h. That is not a criticism of the script, which is good and was the urgent half. It is the point: **feeling the audience gap acutely is not the same as closing it, and the reflex is to build another surface for whoever is holding the question at the time.** The reviewer's second half — *who hears it?* — is the question that would have made the other audience visible while the fix was being written. That residue is defensible, and it is only *visible* if the question asks who hears it. Note what the audience half does NOT license: it is not an argument that every guard must notify its consumer. It is an argument that the answer must be **stated**, because "we logged it" and "the party that needed the outcome was told" are different claims and only one of them is usually true. *(Earned: 2026-08-25, from a survey by @sprint-review of guards verified on main. The five sites each argue the principle from scratch in a comment, which is why nobody noticed they were writing the same argument repeatedly — the arguments are good, and a good local argument is exactly what stops a reader looking for the general form.)* 20. **Before deleting a merged parent's branch, enumerate every open PR whose base is that branch — do not retarget the ones you remember.** GitHub auto-closes a PR when its base branch is deleted, so a branch deletion is a destructive operation on every PR pointing at it, and nothing in the merge UI says so. Retargeting to `main` *while the parent ref still lives* keeps a PR open with its review history intact; the same deletion six seconds later takes down anything that was missed. The mechanical form is one command — `gh pr list --base ` — and it is worth running even when you are confident, because the failure is silent from the author's side: their PR simply is not there any more. Two riders. First, **a stack's protocol does not cover siblings.** Keep-branch reasoning is about children of the PR being merged; a sibling that merely shares the same base is a different relationship the UI renders identically, and it is the one that gets missed. Second, **recovery is cheap if you catch it** — the head sha stays fetchable at `refs/pull//head` even after the branch is gone, so it is resurrect-ref → reopen → retarget → clean up the temp ref, with review history preserved. Fetch it by naming the source ref explicitly, `git fetch origin refs/pull//head:refs/heads/`, which works in any clone; a bare `git fetch` will not surface it, because the default refspec is `+refs/heads/*` only and a workspace that does see pull refs has had `+refs/pull/*/head` added to it. That distinction matters precisely here: “the head stays fetchable” read from a default clone that has just fetched and shown nothing is indistinguishable from wrong, and the only time anyone reads this line is when something is already broken. Rebuilding is only necessary if nobody notices in time. *(Earned: 2026-08-22, the threading train. `#1109` merged at 18:50:59Z; `#1120` had been retargeted while the parent lived and survived; `#1128` shared that base, had not been, and auto-closed at 18:51:05Z — six seconds later. The presser's own summary: "a protocol that protects the children you know about isn't a protocol." Recovered by reopen-and-retarget and merged the same evening at `0a2b69b7`, green on all three test tiers. Related: entry 41 in the AX audit, for the neighbouring failure where a stacked PR's checks describe a tree that will never exist — and its addendum, for the fact that a stacked PR runs no static analysis at all until its base is `main`.)* + +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.)* From c0eed9889d8d3911276c9e31d3929c64ba37c975 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 07:40:03 -0700 Subject: [PATCH 11/13] =?UTF-8?q?docs(checklist):=20rule=2022=20=E2=80=94?= =?UTF-8?q?=20a=20join=20across=20two=20measurements=20needs=20a=20time-in?= =?UTF-8?q?variant=20predicate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Age predicates never are. Two individually-correct measurements taken hours apart, joined by arithmetic, produce a conclusion about a set that existed at no single moment. Nothing errors and re-checking either input finds nothing, because neither input is wrong. Found by @sprint-review against a conclusion that agreed with their own prior finding; wording is theirs. 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 c88e5dc0d..7d3169927 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -80,3 +80,5 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b 20. **Before deleting a merged parent's branch, enumerate every open PR whose base is that branch — do not retarget the ones you remember.** GitHub auto-closes a PR when its base branch is deleted, so a branch deletion is a destructive operation on every PR pointing at it, and nothing in the merge UI says so. Retargeting to `main` *while the parent ref still lives* keeps a PR open with its review history intact; the same deletion six seconds later takes down anything that was missed. The mechanical form is one command — `gh pr list --base ` — and it is worth running even when you are confident, because the failure is silent from the author's side: their PR simply is not there any more. Two riders. First, **a stack's protocol does not cover siblings.** Keep-branch reasoning is about children of the PR being merged; a sibling that merely shares the same base is a different relationship the UI renders identically, and it is the one that gets missed. Second, **recovery is cheap if you catch it** — the head sha stays fetchable at `refs/pull//head` even after the branch is gone, so it is resurrect-ref → reopen → retarget → clean up the temp ref, with review history preserved. Fetch it by naming the source ref explicitly, `git fetch origin refs/pull//head:refs/heads/`, which works in any clone; a bare `git fetch` will not surface it, because the default refspec is `+refs/heads/*` only and a workspace that does see pull refs has had `+refs/pull/*/head` added to it. That distinction matters precisely here: “the head stays fetchable” read from a default clone that has just fetched and shown nothing is indistinguishable from wrong, and the only time anyone reads this line is when something is already broken. Rebuilding is only necessary if nobody notices in time. *(Earned: 2026-08-22, the threading train. `#1109` merged at 18:50:59Z; `#1120` had been retargeted while the parent lived and survived; `#1128` shared that base, had not been, and auto-closed at 18:51:05Z — six seconds later. The presser's own summary: "a protocol that protects the children you know about isn't a protocol." Recovered by reopen-and-retarget and merged the same evening at `0a2b69b7`, green on all three test tiers. Related: entry 41 in the AX audit, for the neighbouring failure where a stacked PR's checks describe a tree that will never exist — and its addendum, for the fact that a stacked PR runs no static analysis at all until its base is `main`.)* 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; if it does, 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 ~11½ hours after the 03:00:00Z run, and 9 rows had aged past the cutoff in between. 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.)* From 0bad4e1c7f6da91613b684bac461b6cda38b53dd Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 07:44:35 -0700 Subject: [PATCH 12/13] docs(checklist): correct rule 22's gap figure to ~3.9 hours MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The citation said ~11½ hours, taken from when the 885-row count was read in conversation rather than when it was measured. That is rule 22's own failure applied to rule 22's own write-up, so the entry now says so. Caught by @sprint-review at 4224bc17. 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 7d3169927..2fe544062 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -81,4 +81,4 @@ 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; if it does, 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 ~11½ hours after the 03:00:00Z run, and 9 rows had aged past the cutoff in between. 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.)* +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; if it does, 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.)* From 4540fb3c2515b236ab1cedb4ba6d34695d38d4da Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Tue, 25 Aug 2026 07:47:10 -0700 Subject: [PATCH 13/13] =?UTF-8?q?docs(checklist):=20rule=2022=20=E2=80=94?= =?UTF-8?q?=20name=20where=20the=20wrong=20instant=20comes=20from?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "State the instant each measurement describes" is passable by someone making the exact error: they state the instant they believe it describes, and that belief is the wrong part. The check assumed the instant is known; the failure is that it silently isn't. Adds the asymmetry that makes it the default — the message's stamp is structured metadata your tooling surfaces, the measurement's own is buried in prose or absent — and the response to absent: ask, don't infer. Wording and argument from @sprint-review's review of 44cdf18e. 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 2fe544062..8f2114a93 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -81,4 +81,4 @@ 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; if it does, 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.)* +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.)*