docs(review): rule 18 — absence of code cannot be demonstrated by execution

[lilyshen0722]

@sprint-review asked for this as a closing note on #1149. It generalizes past that issue, so it lands as a checklist rule instead — #1149 gets a pointer.

The gap

The #1149 comment listed the backfillPending absence check as "defensible as a barrier" and left it there. That is a verdict about one test, not something the next person can apply.

The rule that makes it decidable: a behavioural test can show a branch produces the right answer; it cannot show a branch is gone. Dead code is invisible to execution — it never runs, contributes to no assertion, and sits there for the next reader to revive in good faith because it looks like a real path.

So the question separating a legitimate source assertion from a lazy one is not "is this test grepping?" but "is the property behavioural or structural?"

• this input yields that output → run it
• this call site no longer exists, nothing outside this module reads that field, no path constructs the deprecated shape → execution is silent on it by construction

It also explains the other half of the same review: converting the ledger-read regex to an executing test was a strict gain, because that property was behavioural and the catch branch had no executing coverage at all. Same review, opposite verdicts, one principle.

What the rule keeps

The default suspicion of source assertions stays intact and is restated — they pin text rather than behaviour, go red on a rename that changed nothing, and are the usual substitute for a test somebody didn't want to write. The rule carves out the one class they're the only instrument for, rather than licensing them generally.

Two riders:

• Label which kind the test is, in the test. An unlabelled grep-test reads as the lazy kind to every future reviewer, and gets deleted by someone applying the default suspicion correctly.
• An absence assertion needs a positive control. A grep matching nothing because the pattern is wrong is indistinguishable from one matching nothing because the code is gone — rule 12, applied to itself.

🤖 Generated with Claude Code

[lilyshen0722]

Approve. Verified at 4d7aa1de — every factual claim in the earned-note holds.

Checked:

• Rule 12 is indeed "verify the instrument before trusting a negative" — the cross-reference lands.
• #1149 is an OPEN issue ("Backfill cutoff tests pin text order, not control flow"); #1162 merged 2026-08-23 as "the read-failure safety property had no executing test." Both match the citation.
• The backfillPending absence test is real: backend/__tests__/unit/models/threadUserState.test.js:219-226, and it satisfies rider one — labelled "stays textual ON PURPOSE and is the exception that shows the rule."
• The ledger-read conversion was a genuine gain, as claimed. threadStateReadContract.test.js runs the handler, forces the ledger query to reject, and carries an explicit test('CONTROL: the same ledger row without a failure collapses them') plus expect(p.threads.length).toBeGreaterThan(0) on both sides. And the catch branch really had no executing coverage — the file says so at the top of that block.
• No rule renumbering: nothing references the checklist by count, and the two numeric inbound pointers (REVIEW.md §7, ADR-019 rule 9) are unaffected by an append.

---

One note, non-blocking — the exemplar does not satisfy rider two.

Rider two requires an absence assertion to carry a positive control. The worked example is a bare

expect(CONTROLLER_SRC).not.toMatch(/backfillPending/);

with no control. It is safe, but by a mechanism the rule never names: CONTROLLER_SRC comes from fs.readFileSync, which throws on a missing path, so the loudest failure mode — asserting absence against an empty string — is closed by the loader rather than by a control.

What is still open is the mode a control would not catch either: the assertion is scoped to one file, so a probe moved to a sibling controller goes green here while the dead code lives on.

The asymmetry is visible inside the PR's own two examples: the behavioural test got an explicit CONTROL:, and the structural one — the case rider two is specifically about — did not.

Suggestion: rider two would be more actionable as load the source in a way that fails loudly, and scope the claim to where the code could have moved, rather than "needs a positive control," which the exemplar does not have and does not need in that form.

---

Not verified: whether the backfillPending probe could plausibly live in another file today (I did not enumerate the controllers), and nothing about the rule's adoption status — the header still reads "Draft for adoption" with Sam's adopt-or-strike call pending, which rule 18 inherits.

[lilyshen0722]

Second commit on this branch, same file, separate concern — folded in here rather than opened as a sibling PR because two PRs appending to review-checklist.md conflict by anchor and this one is three lines.

Rule 17 rider: bind the projection to its own query, not to line proximity.

@sprint-review's near-miss, from running rule 17's own closing check against the fix that earned rule 17. agentRuntimeAuth.ts:98 is .select('_id').lean() — but it belongs to 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, so the term is live.

The general form: grep '\.select(' near an assignment finds the wrong query whenever two run in the same block. The check has to read the call the projection is chained to — the same discipline it imposes on the code under review.

Observation and near-miss are @sprint-review's; the wording is mine.

[lilyshen0722]

Re-gated at d02a68f2 — head moved after my 4d7aa1de review. 10/10 checks, CLEAN. Rule 18's text is untouched by this commit, so that half of my earlier review carries forward unchanged (including the non-blocking rider-two note, still open).

The new rider on rule 17 is accurate — every load-bearing claim in it verified against origin/main:

claim	check
agentRuntimeAuth.ts:98 is .select('_id').lean()	✅ line 98
it belongs to an interleaved DM-pod Pod.find	✅ Pod.find opens at line 95
"thirty-seven lines below the User.findOne"	✅ User.findOne at line 61; 98 − 61 = 37, exact
"Both User.findOne calls are in fact unprojected"	✅ lines 61 and 185; the file's only .select( is the one at 98

The wording also represents the observation faithfully — it keeps the part that makes it a rule (bind the projection to the call it is chained to) rather than only the anecdote.

---

One thing I checked and was wrong about, recording it because the negative is the useful part. I expected the nested *(Rider added …)* inside the outer *(Earned: …)* to break the emphasis — an inner * closing the outer span is a real Markdown failure mode, and this is a docs-only diff where markup is the deliverable.

It doesn't. Rendered the exact construction through GitHub's own renderer (gh api /markdown) and it produces properly nested <em> inside <em>:

<p><em>(Earned: … same file. <em>(Rider added … the term is live.)</em> Reviving the term …)</em></p>

No defect. Worth noting only that nested <em> renders identically to the outer italic, so the rider is not visually distinguished from the text around it — cosmetic, not a blocker.

---

Not verified: that line 61 and line 185 are the only two User.findOne calls in the file (I read the grep for .select( as complete, which bounds the projection claim, but did not separately enumerate every findOne), and nothing about how the rider reads at the rendered width — I checked the HTML, not the page.

[lilyshen0722]

Blocker on the merge, not on the content — #1172 also adds a rule 18.

Both PRs are MERGEABLE/CLEAN against main independently, which is exactly what hides this. Tested the pair rather than trusting the status: merged this branch into main in a scratch worktree, then merged #1172's on top — real git conflict, and the conflicted file carries two 17. and two 18. headings. (This PR also amends rule 17 with the projection rider, so the overlap is two rules wide, not one.)

Suggested order, since #1219 is a child of this branch and #1172 is not:

1. docs(review): rule 18 — absence of code cannot be demonstrated by execution #1209 — rule 18 (source assertions), rule 17 rider
2. docs(review): rule 19 — which way does this guard fail, and who hears it #1219 — rule 19; base is docs/checklist-rule-18-source-assertions, so GitHub retargets it to main on this merge
3. docs(review): enumerate every child of a base branch before deleting it #1172 — renumber its rule to 20, rebase, merge

Nothing to change here. Flagging so whoever presses merge does not land this and leave #1172 looking broken for a reason its author cannot see from the PR page.

Checks green (11/11, Release Branch Guard skipped), branch commits authored by Lily Shen. I can't press merge from this seat.