PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The sibling-carrier asymmetry #15957 named is closed at exactly the documented contract; legacy receipt shape pinned by exact-keys assertions; the RED witness bites at the bug's own line (verified on dev by this reviewer); CI green. No delivered-scope defect, no deferred correctness.

Peer-Review Opening: Three PRs tonight, three clean arcs — this one closes the two-branch write with the quieter of the two carriers and the same discipline: the witness fails on the bug's own line before it passes. ADR-0019 read as the gate demands; the diff is clean against the full catalog. Notes below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #15957 (the direct-DM branch returning a bare receipt — the fix's own JSDoc naming its sibling), predecessor #15824 (the broadcast half, merged 07-24), the ticket's Contract Ledger Matrix, ADR-0019 in full (mandatory read — ai/ surface + the PR cites it for the isolation claim), the sibling persistReceiptEdge precedent on dev, and the §critical_gates-10 catalog check.
• Expected Solution Shape: mirror the edge-carrier durability contract on the MESSAGE-node carrier WITHOUT unifying the two intentionally different carriers; a boolean-returning persistence helper; receiptWithDurability reused so the happy path stays byte-identical and only degradation adds durable: false + warning; storage-truth assertions bypassing the in-memory cache; zero AiConfig leaves touched, zero singleton mutation (B4), no config re-derivation anywhere.
• Patch Verdict: Matches — one persistReceiptNode helper + two carrier-specific setters mirroring the edge path; the reference-stable properties contract documented as the reason the in-place mutation is load-bearing; the spec's readMessageFromStorage bypasses the cache exactly like its edge sibling.
• Premise Coherence: coheres: verify-before-assert — the premise "a receipt must not out-claim its durable write" is now enforced on both carriers, and the RED witness proves the enforcement bites (dev: storage readAt null against the claimed timestamp).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #15957
• Related Graph Nodes: #15824 (broadcast-half predecessor), #15821 (the durability umbrella), #15825 (explicitly out-of-scope resurfacing mechanism — honest residual), #15936 (A2A.md carrier docs), ADR 0019 (isolation authority)

🔬 Depth Floor

Challenge (non-blocking): the new direct-DM path drops GraphService.upsertNode's side effect the old comment named — "save to file backing store and notify listeners". persistReceiptNode does storage.addNodes + acknowledgeLocalMutations, and the direct-DM observation semantics rely on reference-stable properties, so existing holders see the mutation — but if anything in-graph subscribed to upsert events for MESSAGE nodes (a wake or reindex path), it no longer fires on direct-DM receipt mutations. The sibling persistReceiptEdge (#15824, merged) has the identical shape, so precedent almost certainly covers this — naming it so the author can point at that precedent explicitly (or name the non-existent subscriber). Second, smaller: db?.storage is not a B3 defensive-read — that catalog entry governs AiConfig reads only; here the handle legitimately carries null (the honest-degradation branch). Checked against the ADR explicitly so the distinction is on record.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches what the diff substantiates (no overshoot)
• Anchor & Echo summaries: precise codebase terminology, no metaphor or source-code snapshot anchor that overshoots durable intent
• [RETROSPECTIVE] tag: accurately characterizes what shipped (no inflation of architectural significance)
• Linked anchors: cited tickets/PRs actually establish the claimed pattern (no borrowed authority)

Findings: Pass — "No AiConfig state is mutated" verified (the spec mutates db handles, never AiConfig leaves); "mirroring without unifying" verified in the diff; the #15825 residual is scoped honestly.

🧠 Graph Ingestion Notes

• [KB_GAP]: none — the carrier asymmetry and the autoSave category error are both understood and documented in-place.
• [TOOLING_GAP]: none.
• [RETROSPECTIVE]: The autoSave gate-as-category-error framing (load echoes vs user-originated writes) is the durable lesson: a gate built for one write class silently discarded another, and the receipt layer inherited the lie. Both halves of the contract now name their write class.

🎯 Close-Target Audit

• Close-targets identified: Resolves #15957 (newline-isolated, valid leaf, not epic-labeled).

Findings: Pass

📑 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 — #15957's two-row ledger (DM mark_read / archive_message receipts gain durable: false + warning on skip, happy path byte-identical, status unchanged) matches the shipped implementation exactly; the exact-keys assertions pin the happy-path half.

🪜 Evidence Audit

• PR body contains an Evidence: declaration line
• Achieved evidence ≥ close-target required evidence, OR residuals are explicitly listed
• If residuals exist: close-target issue body has the residuals annotated
• Two-ceiling distinction: PR body distinguishes sandbox ceiling from author didn't probe further
• Evidence-class collapse check: review language does NOT promote L1/L2 evidence to L3/L4 framing
• Deployment causality: any external/runtime receipt used as a merge gate is reachable from this exact unmerged head; otherwise Post-Merge Validation

Findings: Pass — Evidence: L2 (real SQLite storage reads through the isolated unit harness, bypassing the in-memory graph cache) → L2 required; the #15825 residual is explicitly named as not-claimed.

N/A Audits — 📡 🔗 🛂 📜 🔌 🧠

N/A across listed dimensions: no OpenAPI description surface (receipt shape only, descriptions untouched), no cross-substrate convention introduced, no new architectural abstraction (a mirrored helper), no authority-cited demands, no wire-format/schema change, no turn-memory-scope file.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 8e5cba79af (lint ×2, lint-pr-body, lint-pr-review-body, CodeQL, components, integration-unified, unit) + author non-CI receipts (baseline 7/7, two named RED states, focused 11/11) present.
• Reviewer falsifier (two, both reproduced): (1) focused MailboxService.ReceiptDurability.spec.mjs at exact head → 11/11 green; (2) her spec file applied onto plain dev → RED at the storage-truth assertion: readMessageFromStorage(messageId).readAt = null against the receipt's claimed timestamp 2026-07-26T05:40:17.491Z — the witness bites at the bug's own line, not at a proxy.
• Test location: owning spec extended in place — canonical.

Findings: Pass

📋 Required Actions

No required actions — eligible for human merge.

Follow-up note (non-blocking): the upsert-listener question in Depth Floor — one line naming the precedent or the non-existent subscriber would close it on the record.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 100 — checked and cleared: carriers mirrored without unifying (the ticket's own constraint), helper at the service tier, receiptWithDurability reused, zero ADR-0019 catalog hits (A/B/C groups all checked against the diff and the spec).
• [CONTENT_COMPLETENESS]: 100 — checked and cleared: JSDoc on all three new functions carrying the why (the autoSave category error; the reference-stable mutation contract), updated method docs for both tools, deltas naming what was deliberately NOT done (no upsertNode contract change, no carrier unification).
• [EXECUTION_QUALITY]: 100 — checked and cleared: 11/11 reproduced at exact head; RED verified biting at the bug's own line on dev; legacy shape pinned by exact-keys assertions; serial-mode isolation consistent with the file's existing discipline.
• [PRODUCTIVITY]: 100 — both ledger rows delivered with receipts; the residual is scoped out honestly rather than silently.
• [IMPACT]: 78 — closes the false-receipt class for direct DMs (the agent-facing trust surface of the mailbox); bounded blast radius (one service + one spec).
• [COMPLEXITY]: 28 — two files, one helper + two setters + four tests; the depth is in the carrier asymmetry, not the code volume.
• [EFFORT_PROFILE]: Quick Win — a false-receipt class closed with a mirrored helper and a biting witness.

Closing: merge-safe at green CI. The third clean arc of the night from this author — bisect honesty, boundary idiom, and now carrier symmetry, each with a witness that fails on the defect's own line first.