PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: The WAL-first acceptance boundary is correctly implemented, CI-green, well-tested, and mirrors the established memory-side precedent (#13288). The durability win is real and net-positive even alone — the message I root-caused as lost (de1d9f0b) can no longer be permanently lost. The drain-backstop + pending-read-overlay that #13288 bundled are deferred to #13892 — a real but bounded gap, better tracked as the named next sub than blocked here.

Peer-Review Opening: This is the durability fix for the exact incident I root-caused this session (de1d9f0b — a sunset handover created but lost across a restart). Clean WAL-first shape, and reusing the proven walAppendLock + memoryWal.dir substrate is the right call. Reviewing with deep in-session context on the root cause.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13891 intent (PR body — the local index isn't synced yet), #13889 (the epic I filed), my in-session de1d9f0b root-cause, the diff, and — load-bearing — the #13288 memory precedent surfaced by the prior-art sweep (the same WAL-first decoupling for memories, reviewed by Vega/Ada 2026-06-15).
• Expected Solution Shape: validate → durable WAL append (the acceptance) → best-effort graph projection (failure ⇒ pending, never lost) → a complete replayable WAL record. Must NOT add model-dependent work to the request path; must compose the existing WAL primitives. Per #13288, the complete pattern also needs a drain-backstop + a pending-read-overlay.
• Patch Verdict: Matches + improves the acceptance half. WAL append after pre-ack validation, before projection; projection wrapped in try/catch → projectionStatus: pending on failure (not throw); buildMessageWalRecord is complete + replayable (routing + optionalEdges + a broadcast-audience snapshot at send-time + graphProjectionVersion); readWalMessages is torn-line-tolerant. The #10284 "throw-on-unroutable" guard is reframed to accept-durably-pending — consistent with WAL-first and named in the test reframes.
• Premise Coherence: Coheres (verify-before-assert: the durability fix for a root-caused incident; friction→gold: closes the de1d9f0b loss). The honest caveat lives in the Depth Floor: the acceptance ships without the two halves #13288 proved necessary.

🕸️ Context & Graph Linking

• Target Issue: Resolves #13891 (leaf).
• Related Graph Nodes: #13889 (epic — correctly Related, not a close-target), #13890 / #13892 (sibling subs), #10150, and #13288 (the memory-side precedent of this exact pattern).

🔬 Depth Floor

Challenge (precedent-grounded — the load-bearing one): #13288 established this WAL-first decoupling for memories, and Ada's review forced a graph-projection drain-backstop (_startGraphProjectionDrainLoop re-projecting graph-pending WAL records on restart), with #13288 also bundling a recency-pending-overlay so pending rows stay queryable. This PR ships the acceptance but defers both to #13892. So between this merge and #13892:

• a projection-failed (or restart-lost) message is durable (WAL) but never re-projected (no backstop) and invisible to list_messages (no overlay → no node → not listed);
• combined with the #10284 reframe (loud-throw → accept-pending), a rare in-request projection failure now yields a durable-but-silently-undeliverable message (caller sees status:'sent' + projectionStatus:'pending'; the recipient sees nothing) until #13892 drains it.

This is net-positive even alone (durable ≫ permanently-lost), so it's a follow-up not a blocker — but #13892 must deliver both the drain-backstop and the pending-read-overlay (the #13288 template) for the durability to be end-to-end-useful. Strongly recommend #13892's ACs name both explicitly, citing #13288 so the message path doesn't re-derive what the memory path settled.

Rhetorical-Drift Audit: Pass. The PR body accurately scopes replay/idempotency to #13892 and drain topology to #13890; the test reframes honestly rename the #10284 throw-cases to the new accept-durably semantics.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: #13288 (add_memory WAL-first) is the exact precedent; this is its message-path twin. The reusable shape is durable-accept → decoupled best-effort projection → drain-backstop → pending-read-overlay — and the memory path proved the last two are not optional for end-to-end correctness.
• [KB_GAP]: That four-part pattern deserves a documented home so #13892 (and future durable-accept surfaces) inherit it rather than re-deriving the backstop+overlay each time Ada/a reviewer catches the gap.

🎯 Close-Target Audit

• Close-targets: Resolves #13891 (newline-isolated). #13891 is a recovery/enhancement leaf, not epic-labeled. Related: refs (#13889 epic, #13890, #13892, #10150) are non-closing. No stray Closes/Fixes.

Findings: Pass.

📑 Contract Completeness Audit

The WAL record shape (buildMessageWalRecord → {id, timestamp, message, routing, optionalEdges, graphProjectionVersion}) is the contract #13892's replay consumes. It's complete: routing (sentBy/to/senderUserId/broadcast-snapshot) + all optional edges + the version discriminator. The PR's own Post-Merge Validation asserts #13892 must consume this shape.

Findings: Pass — the replay contract is fully specified for #13892.

N/A Audits — 🛂 📡 🔗 🪜

N/A: composes #13288's pattern + existing WAL primitives (no new architectural-abstraction provenance), no OpenAPI/MCP surface, no skill/convention surface, ACs covered by unit tests (the live restart/replay AC is correctly a #13892 residual, not this leaf's).

🧪 Test-Execution & Location Audit

• The two #10284 throw-tests are correctly reframed to assert the new accept-durably behavior (status:'sent' + projectionStatus:'pending' + the WAL record present with the right routing/broadcast snapshot); a new positive assertion confirms the WAL record on the happy path. Placement is canonical (test/playwright/unit/ai/services/memory-core/).
• CI is fully green at head (unit: SUCCESS, integration + CodeQL SUCCESS; the PR body reports 71/77 local). Verified the reframed-test logic against the diff; relied on CI-green rather than a local re-run.

Findings: Tests correctly assert the behavior change; CI green.

📋 Required Actions

Non-blocking follow-ups (do not gate the human merge):

• #13892 must land BOTH the graph-projection drain-backstop AND the pending-read-overlay (the #13288 template) — until then a projection-failed/restart-lost message is durable but unrecovered + invisible to list_messages. Recommend naming both as explicit #13892 ACs citing #13288.
• (Minor) withAppendLock serializes concurrent sends to the same daily WAL segment — fine at A2A volume; worth a note if message throughput grows.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — mirrors the #13288 pattern, composes existing WAL primitives (walAppendLock, memoryWal.dir), keeps model-dependent work off the request path; reuses the proven substrate rather than inventing one.
• [CONTENT_COMPLETENESS]: 90 — thorough JSDoc + Fat-Ticket body + honest scoping to #13890/#13892/#10150; -10 because the #13288-precedent dependency (backstop+overlay) isn't named in the body as the completion gate.
• [EXECUTION_QUALITY]: 88 — clean WAL-first ordering, complete replayable record, torn-line-tolerant reads; -12 for the projection-failure window that ships before its backstop/overlay (deferred, but real until #13892).
• [PRODUCTIVITY]: 90 — delivers the full acceptance-boundary leaf with strong coverage.
• [IMPACT]: 80 — the durability half of the de1d9f0b fix; A2A message loss is a continuity-integrity failure, so making messages un-loseable is high-value substrate.
• [COMPLEXITY]: 45 — one new WAL helper + a focused addMessage restructure (+ broad alignment churn from the block-alignment linter); moderate reader load.
• [EFFORT_PROFILE]: Heavy Lift — a durable-acceptance substrate change on the core mailbox write path with full test reframing.

Approving — the acceptance boundary is correct, green, and a net durability win on its own. The one thing I'll be watching: #13892 carrying the backstop + overlay, because the memory path already taught us those aren't optional. Merge is the operator's gate.