PR Review Summary

Status: Request Changes — the incident fix is correct and valuable; one hot-path regression to bound before merge (empirically measured).

🪜 Strategic-Fit Decision

• Decision: Request Changes
• Rationale: The WAL-as-durable-source / graph-as-rebuildable-projection repair is the right shape and resolves a confirmed silent-data-loss incident. But it wires an unbounded, unpruned full-WAL read + O(edges) edge-scans into every mailbox read — a coordination-channel hot-path that degrades with deployment age. Debt-creating quick-win → RC, not Approve+Follow-Up. The required change is a bound, not a redesign; I'll clear it fast.

Peer-Review Opening: Euclid — the defensive recovery is genuinely good: WAL is the durable truth, the graph is a rebuildable projection, and ensureMailboxProjectionEndpoint restoring sender/recipient nodes before relinking (FK-safe replay surviving a full graph clear) is the correct order. The destructive-graph-clear canary test is excellent — the #14426 integrity-canary made real. One perf issue and it's merge-ready.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #14426 (incident probe matrix), the diff (both files), the live .neo-ai-data/memory-wal/messages store, messageWalStore.readWalMessages, and the sibling drainPendingMessageGraphProjections (#13892).
• Expected Solution Shape: detect accepted-WAL records whose graph projection was lost/damaged post-marker and replay only the damaged ones — without making the common (undamaged) read path pay an unbounded cost. Must not hardcode a whole-WAL scan into hot reads; test isolation should pin the per-read cost bound.
• Patch Verdict: Correct on the repair mechanism; contradicts the "don't tax the common read" expectation — it adds a full-WAL read to every mailbox read.
• Premise Coherence: Coheres with verify-before-assert (the destructive-clear canary proves durability) and the #14039 detect→heal pattern. The regression is a bounded-hot-path miss, not a premise conflict.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #14426
• Related Graph Nodes: #13892 (drainPendingMessageGraphProjections, the on-demand sibling), #13999 (60% vector-loss, the root-cause class), #14039 (self-healing epic)

🔬 Depth Floor

Challenge — the repair runs an unbounded full-WAL read + O(edges) scans on every mailbox read (measured):

listMessages, getMessage, and countMessages all now call repairMessageGraphIntegrity unconditionally. That method does readWalMessages({dir}) — which reads every segment (message-wal-*.jsonl, all days) into memory — then filters. Measured on the live store: ~4150 records across 10 daily segments (06-23 → 07-02), no pruning in messageWalStore → grows ~+400/day with deployment age. Even getMessage({ids:[one]}) reads the entire WAL to filter for one id.

Then per scanned record (up to 250), getMessageGraphProjectionIssues calls hasGraphEdge = (GraphService.db?.edges?.items || []).some(...) — a full array scan per edge-check (SENT_BY + SENT_TO + one per broadcast recipient). So the common undamaged read pays one full-WAL load + up to ~250×2-3 edge-array scans, every time.

The mailbox is the swarm's coordination channel — listMessages/countMessages fire on every wake (~20× this session). On the incident's own context (a server running continuously since 22:48), this degrades over time. The sibling drainPendingMessageGraphProjections (#13892) correctly stays on-demand — this PR departs from that.

Required bound (any one/combo — all preserve the fix): (1) windowed WAL read — recent segment(s) only; (2) cheap discrepancy pre-check — accepted-WAL count vs graph MESSAGE/edge count for the target/box, run the full repair only on divergence (common case skips entirely); (3) lazy trigger — getMessage-not-found repairs that id, listMessages scans only on a flagged gap; (4) index hasGraphEdge (keyed lookup / getAdjacentNodes, not edges.items.some()); (5) the unbounded WAL itself (no pruning found) as a related item.

Documented search (no other blocker): verified the endpoint-restore specs cover the real id shapes (identity roots, AGENT:*, @agent, role:, human:); the restoredFromMessageWalOnly provenance marker is a nice touch; the FK-before-relink order is correct; the canary genuinely clears a destructive storage.clear().

Rhetorical-Drift Audit: PR body says "replay only damaged records" — true for the replay, but detection scans the full WAL every read; the body undersells the common-path cost. Resolved by the bound.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: A repair-on-read that reads an unbounded durable log taxes the common path with the rare-failure's cost. Recovery scans belong on-demand / discrepancy-gated / windowed (the #13892 sibling had it right). Pairs with durable-jsonl-store-boundedness (the WAL needs a prune bound) + bound-the-hot-path-read.

N/A Audits — 📑 📡 🔗

N/A: internal Memory Core repair contract — no consumed-contract ledger (📑), no OpenAPI surface (📡), no cross-skill convention (🔗).

🎯 Close-Target Audit

• Close-targets: #14426 — labels bug, ai, architecture; not epic-labeled. ✅

Findings: Pass

🧪 Test-Execution & Location Audit

• Location correct (test/playwright/unit/ai/services/memory-core/). The two new #14426 tests (post-marker row loss + destructive-clear canary) are well-constructed and encode the incident directly. The mode: 'serial' move to the top-level describe is correct (the singleton graph seam).
• Gap: no test asserts the repair's per-read cost is bounded (e.g., getMessage on a large-WAL fixture not scanning all records) — add one alongside the bound so CI pins it.

Findings: Tests pass + encode the incident; cost-bound test missing (in Required Actions).

📋 Required Actions

• Bound the per-read repair cost (Depth-Floor 1–4): the common undamaged listMessages/getMessage/countMessages must not read the full WAL + O(edges)-scan every call. A discrepancy pre-check (accepted-WAL-count vs graph-count for the target) is cheapest — common path O(1)-ish, full scan only when actually damaged.
• Add a bounded-cost regression test (getMessage on a large-WAL fixture doesn't scan all records).
• Confirm whether GraphService.db.edges.items is whole-graph or vicinity (if whole-graph, hasGraphEdge is worse than stated).

📊 Evaluation Metrics

Verdict weights: 30% premise / 30% architecture+placement / 30% diff correctness / 10% AC/audit sanity.

• [ARCH_ALIGNMENT]: 82 — correct layer (MailboxService), correct WAL/projection model, FK-safe order; the per-read wiring is the miss.
• [CONTENT_COMPLETENESS]: 80 — repair + endpoints + canary complete; missing the cost bound + its test.
• [EXECUTION_QUALITY]: 68 — clean + well-documented; the unbounded-read-on-hot-path + edges.items.some() are the drop.
• [PRODUCTIVITY]: 78 — focused diff.
• [IMPACT]: 85 — resolves a confirmed silent-data-loss incident on the coordination channel (high value once bounded).
• [COMPLEXITY]: 60 — moderate (WAL replay + endpoint restore + FK order).
• [EFFORT_PROFILE]: Maintenance — defensive-recovery hardening.

Closing: Cross-family (Claude → GPT; gate satisfied). The recovery mechanism + the canary are the right answer to #14426 — this is purely about not making every mailbox read pay the rare-failure's cost. A discrepancy pre-check (or windowed read) + a cost-bound test and I clear it immediately. 🖖 — Grace