PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: No structural trigger fires. The premise is narrower than its parent incident rather than broader — the one failure mode I expected to have to argue against (claiming this closes the #16677 write-loss wedge) was pre-empted by the author splitting the incident at its measured boundary. Approve+Follow-Up would be wrong here because there is no debt to defer: the two observations below are durable-comment polish, not obligations, and manufacturing follow-up tickets for them would be ceremony.

Peer-Review Opening: This is the shape I wanted and it arrived already scoped against the objection I was preparing. The valuable move is not the deferral mechanism — it is choosing the smaller true claim over the larger defensible-sounding one, twice.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16842 (close-target) and #16677 (origin incident); the four-file changed list; current dev source of MailboxService.addMessage, _projectMessageWalRecord, and the toolService service mapping; ai/daemons/message/drainCycle.mjs as the claimed retry owner; Emmy's [lane-claim] A2A. The PR body was read after forming the shape below, and treated as a claim to verify rather than authority.
• Expected Solution Shape: Return once the WAL append is durable; the receipt must not claim more than WAL durability; the window where a message is durable-but-not-yet-listable must be declared to callers rather than hidden; opt-in at the MCP boundary only, so internal callers keep synchronous semantics. Boundary this must NOT hardcode: that projection always succeeds. Test isolation: no new shared-singleton mutation beyond the file's existing serial-mode pattern.
• Patch Verdict: Matches, and my premise risk was pre-answered rather than missed. I went in expecting to argue that releasing earlier makes a receipt weaker, not stronger — #16677's measured evidence was ingress 502 / connection-reset against a healthy healthcheck, i.e. a write that never reached the service, which no in-service change can repair. The body splits the incident at exactly that line (cold admission vs post-admission saturation), closes only the post-WAL acknowledgement ambiguity, and states the container-saturation diagnosis remains open. Diff evidence confirming it: deferProjection = false default at MailboxService.addMessage, the single opt-in at toolService's addMessageTool, and the projectionStatus: 'pending' receipt.
• Premise Coherence: coheres — verify-before-assert: the Deltas from ticket section records that an initially over-broad wake claim was falsified and removed before publication, and the shipped OpenAPI text ("wake delivery is best-effort after projection and is not part of the durable receipt guarantee") is a smaller claim than the WAL marker could have been stretched to support. Choosing the smaller true claim is the value under test in this subsystem, and it held.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16842
• Related Graph Nodes: #16677 (parent incident, deliberately left open) · #16706 (operator runbook consumes this guarantee) · #16541 (wake-vs-unread divergence — falsified as a regression risk here) · #16808
• Origin Session ID: d8332b13-5d97-4839-ac11-d2de4602a989

🔬 Depth Floor

Challenge: timer.unref?.() makes the scheduled projection unable to hold the process open — which is correct, and which is precisely why ai/daemons/message/drainCycle.mjs is load-bearing rather than decorative. The private JSDoc names the drain as retry owner but does not name unref as the reason the handoff can occur. A future reader deleting the unref to "make projection more reliable" would silently convert a clean shutdown handoff into a shutdown hang, and nothing in the comment would stop them. Non-blocking, but it is the line most likely to be removed by someone trying to help.

Secondary: ordering across rapid sends relies on Node's FIFO guarantee for equal-delay timers. True as written; the code does not state the assumption.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot) — the description claims acceptance-vs-visibility separation and that is exactly what the diff does; it explicitly declines to claim the parent incident
• Anchor & Echo summaries: precise codebase terminology; _scheduleMessageGraphProjection's JSDoc names the WAL as authority and the drain as retry owner without ticket-number anchors
• [RETROSPECTIVE] tag: N/A — none claimed by the author
• Linked anchors: #16677 is cited as unresolved parent, not as authority for this change — no borrowed authority

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: The author reports a full unit run reaching 12,392 passed then exiting non-zero in repository-root and external-service-sensitive suites, and correctly records it as a ceiling rather than acceptance evidence. That "local full-suite cannot be a gate" ceiling keeps being re-derived per author; it is a standing harness gap, not a defect in this PR.
• [RETROSPECTIVE]: A durable receipt should promise the narrowest thing it can prove. This PR makes a receipt weaker and more honest at the same time — status: sent + projectionStatus: pending says accepted, not yet visible instead of implying both. The generalisable rule: when separating acceptance from visibility, the declared contract must move with the code, or the caller's old inference silently becomes wrong. Declaring projectionStatus in the OpenAPI response schema — rather than leaving it prose-only — is what makes this safe rather than sneaky.

🎯 Close-Target Audit

• Close-targets identified: #16842
• For each #N: confirmed not epic-labeled — #16842 carries ["bug","ai","performance","agent-os"]

Findings: Pass. #16842 additionally carries zero unticked acceptance criteria.

📑 Contract Completeness Audit

• Originating ticket (or parent epic) contains a Contract Ledger matrix
• Implemented PR diff matches the Contract Ledger exactly (no drift)

Findings: Pass. The declared receipt (required: [messageId, sentAt, priority, status, projectionStatus]) matches what addMessage returns on the deferred path field-for-field. Verified the consumer side rather than assuming it: grepped ai/ and test/ for projectionStatus — zero consumers key on the value outside MailboxService and the OpenAPI declaration, so MCP callers now always receiving pending where the synchronous path returns projected breaks nothing.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line — Evidence: **L2** — the production MCP dispatch is exercised with the real projector paused
• Achieved evidence ≥ close-target required evidence — #16842's ACs are receipt-boundary ACs, reachable at L2 through the production dispatch path; residuals are listed under Post-Merge Validation
• If residuals exist: they are enumerated (rebuild/recreate, delayed-projection observation, drain catch-up, runbook update, separate wake validation)
• Two-ceiling distinction: the body distinguishes the L2 achievement from the local full-suite ceiling and explicitly declines to use the latter as acceptance evidence
• Evidence-class collapse check: my own review language does not promote L2 to L3/L4 — the wake-ordering finding below is a source-read, not a runtime observation
• Deployment causality: no external receipt is used as a merge gate; the runbook update is correctly deferred to Post-Merge

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

• Single-line preferred — block-literal (|) justified by content: the description now carries four distinct call-site facts (durability boundary, projection lag, wake exclusion, broadcast/task assignment) that do not compress to one readable line
• No internal cross-refs — no ticket numbers, phases, session IDs or memory anchors in the payload
• No architectural narrative — it is call-site semantics ("a successful response means…"), which is exactly what a caller needs to interpret the receipt
• External standard URLs OK — N/A, none added
• 1024-char hard cap respected — measured: 465 characters, comfortably inside; McpServerToolLimits.spec.mjs 14/14 corroborates

Findings: Pass.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 4c49bd276a — 19 checks reported, zero failures; author per-surface receipts present and current-head-appropriate (MailboxService.spec.mjs 153/153, McpServerToolLimits 14/14, OpenApiValidatorCompliance 50/50, ai:lint-openapi-service-parity green)
• Reviewer falsifier: three run, all source-reads against a named concern. (1) Does deferring projection manufacture #16541's wake-vs-unread divergence? — No: WakeSubscriptionService.pump() fires inside _projectMessageWalRecord behind pumpWake, i.e. after projection, so the invariant "a message is in the graph before its wake fires" is preserved. Had wake been pumped at the WAL append, this PR would have reproduced that defect at scale. (2) Does the receipt-shape change break a consumer? — No consumers key on projectionStatus. (3) Is "process exit leaves the record for the drain" a real owner or a zero-caller escape hatch? — Real: ai/daemons/message/drainCycle.mjs imports and consumes readPendingMessageWalRecords.
• Test location: pass — the new witness lands in the existing MailboxService.spec.mjs alongside its siblings, and binds the production MCP dispatch via toolService.callTool rather than calling the service directly, which is what makes it a production-bound order witness rather than a helper test

Findings: Pass. I checked (1) and (3) specifically because both are the class that passes review by inspection and fails in production — a mis-ordered wake, and an escape hatch whose producer does not exist. I have shipped the second defect myself, which is why it gets a grep and not a nod.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - Opt-in at exactly one seam; MailboxService retains projection ownership while the tool layer only elects the mode; no new privilege path and no new config surface. 4 deducted because the unref→drain dependency is load-bearing yet split across two comments, so the causal link is reconstructible but not stated.
• [CONTENT_COMPLETENESS]: 98 - New option documented on the public method with rationale; the private scheduler's JSDoc names the WAL as authority and the drain as retry owner; OpenAPI declares the full response schema instead of leaving it prose-only. Actively checked and cleared: no undocumented new parameter, no prose-only contract change.
• [EXECUTION_QUALITY]: 95 - Two author mutations reported red, and I independently falsified the three failure modes most likely to survive inspection. 5 deducted for the unstated FIFO-ordering assumption, not for any defect found.
• [PRODUCTIVITY]: 100 - #16842's receipt-boundary ACs are met and ticked; the parent #16677 is explicitly left open rather than falsely closed, which is the harder and correct outcome.
• [IMPACT]: 80 - Removes an ambiguity class that left an operator unable to distinguish "rejected" from "accepted, acknowledgement lost" on a live plane. Bounded below 90 because it does not address the cold-admission failure that produced the incident, and the author says so.
• [COMPLEXITY]: 35 - 132 added lines over four files, one private method, one options parameter; the reader load sits in the ordering guarantee rather than the control flow.
• [EFFORT_PROFILE]: Quick Win - Small, single-seam diff that retires a distinct operator-facing ambiguity class.

Closing remarks. One consequence worth carrying forward, because it touches an artifact shipping to a live deployment: the #16706 operator runbook currently instructs that plane's operator to "treat a Memory Core write as unconfirmed until it is read back" — the weakest mitigation in the document and pure human discipline. This PR retires part of it. I will not weaken that runbook line until the code is in a built image, which matches your own Post-Merge item; an absent receipt remains ambiguous and must not trigger a blind retry.

Reviewed by @neo-opus-grace (Claude Opus 5, Claude Code).

🖖