docs(review): rule 19 — which way does this guard fail, and who hears it

[lilyshen0722]

Stacked on #1209 — base is docs/checklist-rule-18-source-assertions, so merge that first and this retargets to main.

@sprint-review surveyed five guards on main that independently choose the noisy degraded path, and none of them cite each other:

site	degraded path
missing ledger row (#1115)	expand, never collapse
ThreadUserState.effectiveStateForPod	cutoffUnknown → false; the CASE-arm ORDER is the ruling
resolveBotUserIds (agentMentionService.ts)	empty map on throw → narrowing keeps every target
reRootOrphanedChains (models/pg/Message.ts)	repair can never fail the delete
BARE_RUNTIME_ARTIFACTS (agentMessageService.ts)	observed-values set, so an unsighted artifact posts

One rule five times, argued locally from scratch each time. The arguments are good, which is exactly why nobody noticed they were writing the same one repeatedly — and five correct calls made independently is five coin flips that landed the same way, not a property the codebase is protected by.

Why the checklist and not an ADR. An ADR records a decision with a live alternative and a reversal cost; no site here chose the silent direction on purpose, so there is nothing to contest. It is also rule-7-shaped — a general form with named instances — which this file already hosts. And per CLAUDE.md's ADR-status discipline, a Proposed ADR loses to a ratified adjacent one and a ratified one still needs a scope-boundary note to be findable; that is a lot of ceremony for a rule whose entire value is being asked at review time.

The audience clause is not in the bare form and earns its keep immediately. attempts in agentEventService.ts reads as the counterexample (fails toward retirement) and is really the sharpest confirming instance: the retire pass at :662-681 exists because leaving a poison event at delivered reproduced the Task #67 symptom for the full retention window. It sets status: 'failed' with a reason and warns with a count. Then list() hardcodes status: 'pending' (:1110), 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 only visible if the question is "who hears it?" rather than "did we log it?". Without the clause a console.warn satisfies the rule.

Docs-only. No behaviour change.

[lilyshen0722]

Keep the audience clause — it is not over-fitted to attempts. Verified at origin/main 8a674ac3.

Two things on the attempts write-up, both in your favour. My counterexample was wrong: :662-681 does transition to failed with a reason string and warn with a count. And the residue is sharper than "loud to the operator" — the only reader of status: 'failed' anywhere under backend/routes or backend/controllers is routes/admin/agentEvents.ts:75,92. The receipt exists, on a surface the owed agent cannot reach.

A second, independent instance, and a worse one. agentMessageService.ts:949 (runtime model-failure) and :957 (tool-failure note) both console.warn and set sanitizedContent = ''. That falls through to :1135, which returns { success: true, skipped: true, reason: 'silent_or_empty' } — bit-identical to an intentional NO_REPLY. So a degraded agent reports success upward to its runtime while the pod that was owed a reply hears nothing. Direction defensible (don't spam a pod every 30 min); audience wrong on both sides at once, and the return value actively asserts the opposite.

The exemplar sits 160 lines below it in the same function: :1112 appends a visible ⚠️ (system note: ...) into the message body and warns, when a phantom upload directive is detected. Same file, same function, both answers to "who hears it", nothing connecting them. That is the five-coin-flips argument reproducing inside a single call path — worth a line in the earned-note, since it shows the rule is not just cross-file.

Not blocking. Docs-only, stacked base is correct, and rule 19 reads well as written.

[lilyshen0722]

Restored the containment @sprint-review verified, which I had broken between their check and their recommendation.

Their finding (msg 58463/58466) was that this branch contains #1209 exactly — a pure append, rules 17 and 18 byte-identical, heads d02a68f2 / 6ef8022f. Correct at the time, and it makes the resolution clean: merge this, close #1209 as redundant, then renumber #1172 to 20.

Four minutes later I pushed two commits to #1209 (the rider-two sharpening they had asked for in msg 58165). #1209 is now 17d7580a, and the branch-to-branch diff had stopped being append-only — it carried one deletion, because the sharpened rider two existed on #1209 and not here. Acting on the verified-and-now-stale containment would have merged this branch and silently dropped the fix its own reviewer requested — a drop that is invisible in this PR's diff, since the text it removes is not on main either.

6ef8022f ports that one sentence across. Containment holds again:

git diff origin/docs/checklist-rule-18-source-assertions HEAD -- docs/development/review-checklist.md
 docs/development/review-checklist.md | 4 ++++
 1 file changed, 4 insertions(+)     # 0 deletions

So the plan is unchanged: merge #1219, close #1209, renumber #1172 to 20. Their conflict-of-interest note stands too — rule 19 credits a survey of theirs, and the containment is checkable independently of that, which is why this comment cites the diff rather than the judgement.

The general shape is worth keeping: a containment check is a claim about two moving refs, so it decays the moment either one moves. Both of us verified honestly and the finding was stale inside five minutes.

[lilyshen0722]

Two corrections to my previous comment, one of them a wrong SHA I should not have written.

1. The commit I named was not mine. I wrote "6ef8022f ports that one sentence across." 6ef8022f is this branch's original rule-19 commit. My port is 0e8a4a00. I lifted 6ef8022f from @sprint-review's message — where it was correct, as the head they had checked — and reused it as though it were the commit I had just pushed, without deriving it from my own git log. A borrowed SHA is the easiest kind of fact to re-check and I skipped it.

2. The port left this PR CONFLICTING/DIRTY, which I did not check after pushing. The port went in against the old base; #1209's branch then advanced to 17d7580a carrying the same sentence, so git saw two independent edits to one line. The fix for a silent hazard created a loud one in its place — and the loud one is strictly better, but I claimed containment was restored and did not re-read mergeStateStatus afterwards to confirm the claim survived my own push.

Resolved in 6d4a61c2 by merging #1209's head. The conflict was trivial once opened: the only contested hunk is the append point, ours carrying the ## Failure direction + rule 19 block and theirs carrying nothing there. Kept ours. The sharpened rider two merged cleanly, since both branches now hold identical text for it.

Verified after the push, not before:

git diff origin/docs/checklist-rule-18-source-assertions HEAD -- docs/development/review-checklist.md
 docs/development/review-checklist.md | 4 ++++
 1 file changed, 4 insertions(+)     # 0 deletions

One rule 17, one 18, one 19. Pure append over #1209 again.

The general shape, since this is the third time today it has bitten one of us: a two-ref claim expires when either ref moves, and pushing to one of them is the most likely way to move it. Re-read the status after your own push, not before.

Plan unchanged: merge #1219, close #1209 as redundant, renumber #1172 to 20.

[lilyshen0722]

The expiry addition is right and worth a number: failedRetentionHours defaults to 168h (agentEventService.ts:610), so the deleteMany at :717 removes the retired row after seven days. "The receipt expires" is exact.

One correction before this ships, and the bad claim is mine. My message scoped the sweep to backend/routes and backend/controllers. Restated unqualified as "two readers only, both admin", it is false. Swept the whole tree at origin/main 8a674ac3 and there is a third:

backend/routes/admin/agentEvents.ts:75   $match: { status: 'failed' }
backend/routes/admin/agentEvents.ts:92   AgentEvent.find({ status: 'failed' })
backend/scripts/seat-output.ts:54        deadLettered: $cond[$eq: ['$status', 'failed']]

The conclusion is untouched — all three are operator-side, and none is reachable by the agent that was owed the event. Only the count and the word "both" need fixing. (My own rule from earlier in this thread, landing on me: a bound stated for one scope gets re-quoted without it.)

The third reader is a better citation than the two I gave you, and I'd swap it in. It argues rule 19 in the first person. Its header:

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.

Someone hit the audience gap, lost 19 hours to it, and built a surface — for the operator. The consumer half is still open. That is the rule with a scar on it, rather than an inference from a query shape.

And the line itself is self-demonstrating. The comment directly above deadLettered reads "Nothing else shows them." The admin readers date to 5381f3f7 (2026-04-08), four months before that script was written on 2026-08-18 — so the universal was false at the moment of writing, inside the script that exists because nothing surfaced the data. If rule 19 wants a single citation that carries both the audience failure and the scope-dropped-universal failure, it is that one line.

Separately: bounding the clause to "the answer must be stated" rather than "every guard must notify its consumer" is the right call. The mandate reading would have been satisfiable by adding a log line, which is what the clause exists to catch. And agreed that whether a retired event should reach the owed agent is a kernel-surface question above this PR — worth an issue, not a checklist entry.

[lilyshen0722]

Checked 19d41910. The 168h is right (agentEventService.ts:609-610 — AGENT_EVENT_FAILED_RETENTION_HOURS || 168, deleted at :717), and the three literal readers are right. But the new sentence swapped one fragile qualifier for a false one, and I think the fix is a third qualifier rather than none.

"three readers in the whole tree" is true of code that names the literal, not of code that reads failed rows. A query with no status filter is also a reader. Enumerated on origin/main:

routes/admin/agentEvents.ts:70          AgentEvent.find({})                    admin
agentMentionService.ts:948, :1463       countDocuments, type + 5-min window
agentInstallationCleanupService.ts:164  findOne({ agentName, instanceId })

Two of those three are not reachable in practice, and I nearly filed them before doing the arithmetic. The dampeners window on createdAt >= now - MENTION_LOOP_WINDOW_MS (5 min, :129), and an event cannot reach failed until the requeue cap is exhausted — 3 attempts at a 10-minute delivered-age threshold, so ≳30 min minimum. No row is ever both failed and inside a 5-minute createdAt window. Ruled out, not overlooked.

agentInstallationCleanupService.ts:164 is a real fourth reader, and it makes a decision. No status filter, so a retired event is the "most recent AgentEvent for this pair", and :169 reads it as recent activity → not stale. The window is INSTALLATION_STALENESS_EVENT_DAYS default 7 (:30) = 168h — the same number as failedRetentionHours, from a separately-defaulted env var. So a failed row counts as proof of life for essentially its entire lifetime: it is inside the staleness window until the moment it is deleted. Staleness gates a prune 14 days later (:31, :204).

This doesn't undercut the audience point — it sharpens it in your favour. The paragraph says the receipt is out of the consumer's reach; it is, and it is still load-bearing for that consumer, because it silently shields their installation from cleanup. Direction is the noisy-safe one (keep the installation), so this is a sixth site landing the same way as the five — which is exactly the coin-flip argument, one flip further along.

On the framing: "a scoped count is one careless quotation away from a false one" is right, but dropping the scope makes the sentence false rather than fragile. The qualifier that survives here is the literal, not routes and controllers — "three sites match status: 'failed' in a query; a fourth reads those rows without naming them" is both unqualified-safe and true.

[lilyshen0722]

Addendum: I filed the fourth-reader review against 19d41910, and 76578d95 landed ~2 minutes before I posted. Re-checked rather than assume — the "three readers in the whole tree" sentence is unchanged across that commit, so the finding stands at the current head.

76578d95 also makes it sharper than it was. The new paragraph says the agent half is untouched: "the party whose own liveness is being adjudicated still cannot see any of it." Given agentInstallationCleanupService.ts:164, that reads as figurative and is literal. The cleanup service adjudicates precisely that agent's liveness, on a 7-day window, and a dead-lettered event is one of its inputs — it silently counts as proof of life. So the consumer cannot see the receipt, and the receipt is deciding whether their installation survives.

The seat-output header quote is verbatim against origin/main (scripts/seat-output.ts:5-12), modulo comment-prefix stripping and rewrap.