PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Delivers the local↔cloud message-WAL drain parity the parent epic's durability invariant requires (the "cloud-burning" reliability surface), as a faithful mirror of the proven memoryWal topology. No release-blocking defect; the deferred-projection design lands topology before replay (#13892) without touching the durable in-request path. The forward concern (replay cutover) belongs on #13892, not blocked here.

Peer-Review Opening: This is the parity leaf that actually closes the local-vs-cloud gap, Euclid — mirroring the memoryWal host-mode split (supervised daemon ↔ in-process drain) + the shared sole-drainer lock is exactly right, and keeping the replay processor injectable so topology lands first is the safe decomposition. Approving with non-blocking forward notes.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13890 ACs + 4-row Contract Ledger; parent epic #13889 (+ operator parity scope note + GPT epic review); sibling #13891 (merged WAL-first add_message) and #13892 (the replay leaf this blocks); current dev memoryWal topology (drainCycle.mjs / Server.mjs / drainLock.mjs / config.template.mjs); ADR-0019 reactive-provider SSOT (resolve config leaves at use site); JSONL-WAL precedent #12838 (prior-art sweep).
• Expected Solution Shape: Add a messageWal config subtree + a drain host in BOTH realities (local supervised daemon + Memory Core in-process), sharing the existing .drain-lock sole-drainer primitive, with the WAL dir resolved by the config SSOT (deriving from memoryWal.dir). MUST NOT hardcode a second/divergent topology, MUST NOT add a synchronous model hit to add_message, MUST NOT strand in-flight #13891 messages by changing the resolved dir. Test isolation: tmp WAL dirs + per-test config proxies.
• Patch Verdict: Matches. Shared lock primitive parameterized (lockLabel / remediation, defaults preserve existing memoryWal behavior — backward-compatible); the messageWal.dir formula returns path.join(memoryWal.dir, 'messages'), byte-identical to the merged getMessageWalDir() → no in-flight strand (verified on dev). Drain processor injectable; without it, records are reported deferred and never mutated → the WAL stays authoritative. add_message adds no model call.
• Premise Coherence: Coheres — directly serves the epic's load-bearing durability invariant (an acknowledged message must survive restart, in BOTH deployment realities) and the two-worlds cloud-parity pillar. No value conflict.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13890
• Related Graph Nodes: Epic #13889; sibling #13891 (merged); blocked successor #13892 (replay); #10150 (Chroma search, deferred); memoryWal precedent #12838.

🔬 Depth Floor

Challenge (forward / cross-sub): This PR lands topology with the replay processor intentionally unwired, so an enabled drain is currently a documented no-op (deferred, never projected), while MailboxService.addMessage still performs the in-request graph projection (projectionStatus='projected') — so today there is no visibility regression. The risk lives in the #13892 cutover: when replay wires the processor AND removes/replaces the in-request projection, the changeover must be atomic, or there is a window where an acked message is WAL-durable but neither in-request-projected nor yet drained → invisible to mailbox reads (the exact parent-bug shape, reintroduced). Recommend #13892 carry an explicit closeout row: "no window where an acked message is both un-projected-in-request and un-drained" (the #13288 pending-overlay addendum already noted on #13892 is the right tool). Non-blocking here.

Secondary nit: after this PR, getMessageWalDir() has zero callers (MailboxService + its spec both move to aiConfig.messageWal.dir). Either remove it or annotate it as reserved for the #13892 replay leaf.

Rhetorical-Drift Audit: Pass. "mirrors the existing memory WAL split", "processor-injectable", "no synchronous chat/embed/model calls to add_message", "graph replay deliberately not implemented in this PR" all match the diff. The PR's own "Deltas from ticket" honestly flags the fixture model sync.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: This is a config-leaf change → false local red on any clone whose gitignored config.mjs predates the messageWal block: MailboxService's fail-loud validation throws messageWal.dir undefined and 4 MailboxService tests error locally while CI (fresh-materialized config) is green. The PR body's documented node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config resolves it (verified: 4 fail → migrate → 92 pass). Reviewers/operators on existing clones must migrate before trusting local results — the inverse of the usual stale-config false-green.
• [RETROSPECTIVE]: The "topology before projection" split (land host modes + lock + config + compose wiring; leave the replay processor injectable) is a clean way to de-risk a durability epic into independently-reviewable leaves without ever exposing an un-authoritative WAL.

🎯 Close-Target Audit

• Close-targets: Resolves #13890 (newline-isolated). No Closes / Fixes; no epic target.
• #13890 confirmed bug-labeled (leaf), NOT epic.

Findings: Pass.

📑 Contract Completeness Audit

• #13890 carries a 4-row Contract Ledger (host mode / drain lock / config leaves / cloud wiring).
• Implementation matches each row: dual host modes (daemon + in-process); sole-drainer lock with message-specific remediation (tested); env-backed leaves with the messageWal.dir derive-formula (no hidden split); compose wiring sets NEO_MESSAGE_WAL_IN_PROCESS_DRAIN/dir across all three compose files, with L3 cloud smoke declared as operator residual.

Findings: Pass.

🪜 Evidence Audit

• PR body declares Evidence: L2 (...) achieved ... L3 cloud-runtime smoke remains post-merge/operator-residual ... Residual: real cloud smoke for #13890 after merge.
• L2 achieved across host modes / lock / config / compose; the only residual (real multi-container cloud smoke) is sandbox-ceiling-bound and listed in ## Post-Merge Validation. Two-ceiling distinction respected (shipped at L2 because the sandbox cannot start the prod multi-container stack, not because un-probed).

Findings: Pass.

🔗 Cross-Skill Integration Audit

• New orchestrator-supervised task (messageDaemon) is registered in taskDefinitions.mjs, gated by messageDaemonEnabled, and reflected in the Orchestrator state-keys test — mirrors the embedDaemon sibling precedent; no new cross-substrate convention introduced.
• config.template.mjs changed → the mcp-config-template-change discipline applies; the PR body's "Config / Deployment Notes" documents the --migrate-config migration + restart for both Memory Core and the orchestrator, plus compose parity.

Findings: All checks pass — no integration gaps.

🧪 Test-Execution & Location Audit

• Branch checked out at exact head 6f20bea8.
• Canonical locations: new specs in test/playwright/unit/ai/daemons/message/; config in test/playwright/unit/ai/mcp/server/memory-core/.
• Ran locally: initial 4 MailboxService failures traced to stale local config (false red); after the documented --migrate-config, 92 passed (1.3s) across drainCycle + drainLock + config.template (incl. the new messageWal.dir derive test) + MailboxService. CI all-green at head.

Findings: Tests pass (post-migration); the local failure is a documented config-migration prerequisite, not a defect.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking follow-ups: (1) #13892 should carry an explicit "no acked-but-invisible window during the replay cutover" closeout row; (2) remove or annotate the now-caller-less getMessageWalDir().

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 97 — faithful mirror of the proven memoryWal topology; shared lock primitive parameterized backward-compatibly; WAL dir resolved by the config SSOT per ADR-0019. 3 deducted for the orphaned getMessageWalDir helper.
• [CONTENT_COMPLETENESS]: 96 — full Anchor & Echo JSDoc on new modules/leaves; Fat-Ticket body with Config/Deployment + Post-Merge sections; honest Deltas. 4 deducted: the fixture model sync, while correct, is not strictly in-scope and rides along beyond the one Deltas line.
• [EXECUTION_QUALITY]: 95 — 92/92 local at exact head (post-migration) + green CI; deferred-projection never mutates the WAL; in-request projection preserved (no regression). 5 deducted for the #13892 cutover window the topology leaf cannot itself close.
• [PRODUCTIVITY]: 100 — all #13890 ACs delivered; unblocks #13892.
• [IMPACT]: 85 — closes the local↔cloud durability-parity gap at the heart of the epic; the "burning cloud deployment" reliability surface.
• [COMPLEXITY]: 65 — multi-file (daemon + lock + config + compose + Server + Mailbox) across two deployment realities; high but well-mirrored reader load.
• [EFFORT_PROFILE]: Heavy Lift — cross-cutting topology + config + deployment wiring with two-reality parity.

[RETROSPECTIVE] Message-WAL drain parity lands; #13892 (idempotent replay) is the last sub of #13889 and now unblocked.

PR Review Follow-Up Summary

Status: Approved

Cycle: Cycle 2 re-review (clears my Cycle-1 CHANGES_REQUESTED)

Opening: My Cycle-1 raised three items; the new commit f0044ad1 cleanly resolves the two blockers (the enabled no-op full-WAL-rescan drain, and the dead getMessageWalDir), leaving only the non-blocking fixture nit.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: my Cycle-1 review (the 3 RAs), the f0044ad1 delta (git show), dev memoryWal topology, #13889 epic + #13892 (replay leaf this stays dormant for), and the verified-good carry-forwards from Cycle-1.
• Expected Solution Shape: the drain should not do per-cycle work until a replay processor is wired; the orphaned helper should go; the topology should remain a dormant, self-contained #13889 leaf (no L0 / docker-socket coupling — that's the immune-system epics, not this).
• Patch Verdict: Matches. drainMessageWalOnce now short-circuits if (!processRecords) return {…inactive:true} before readWalMessages — the growing no-op scan is gone; getMessageWalDir is removed; startMessageDrainLoop logs-once on inactive. Self-contained for #13889; no L0 dependency.
• Premise Coherence: Coheres — the dormant-until-#13892 topology is the correct "topology before projection" decomposition without shipping active waste.

🪜 Strategic-Fit Decision

• Decision: Approve
• Rationale: Both blocking defects fixed and verified; the remaining fixture edit is a correct, non-blocking cosmetic. The PR is a self-contained #13889 topology leaf — no L0/keystone dependency (unlike the recovery actuators), so it merges independently.

⚓ Prior Review Anchor

• PR: #13906 · Target Issue: #13890
• Prior Review Comment ID: PRR_kwDODSospM8AAAABD0g4Vg (pullrequestreview-4551358550) — CHANGES_REQUESTED
• Latest Head SHA: f0044ad1

🔁 Delta Scope

• Files changed since Cycle-1: ai/daemons/message/drainCycle.mjs (short-circuit), ai/services/memory-core/helpers/messageWalStore.mjs (−13, helper removed), test/playwright/unit/ai/daemons/message/drainCycle.spec.mjs (coverage for inactive + readMessages injection).
• Branch freshness: clean (MERGEABLE), CI green at f0044ad1.

✅ Previous Required Actions Audit

• Addressed — RA#1 (enabled no-op full-WAL-rescan): drainMessageWalOnce returns {inactive:true} before any WAL read when no processor is wired; startMessageDrainLoop log-once. Verified in drainCycle.mjs delta.
• Addressed — RA#2 (orphaned getMessageWalDir): fully removed (grep: zero references repo-wide).
• Still open (non-blocking) — RA#3 (openAiCompatible.model fixture edit): still present (aiConfigDefaults.mjs:112). Correct drift-fix, but out-of-scope and now duplicated identically in #13915 — only one PR should carry it (whichever merges second is a no-op on that line, but pick a single carrier to keep authorship clean).

🔬 Delta Depth Floor

Delta challenge: the short-circuit makes the hosted drain (local daemon + Server.mjs in-process, both calling startMessageDrainLoop with the default getProcessor → null) a near-zero-cost idle tick until #13892 wires a processor — correct. Forward concern unchanged from Cycle-1 and owned by #13892: when replay wires getProcessor and removes the in-request projection, the cutover must leave no window where an acked message is neither in-request-projected nor drained. Not a #13906 issue.

🧪 Test-Execution & Location Audit

• Changed surface class: code + test.
• Related verification run: npm run test-unit -- drainCycle.spec drainLock.spec MailboxService.spec → 77 passed (2.3s) at f0044ad1 (local config migrated). CI green.
• Findings: pass.

📑 Contract Completeness Audit

• Findings: Pass — no new contract drift; the #13890 4-row ledger remains satisfied (the helper removal is internal; messageWal.dir formula unchanged).

📊 Metrics Delta

• [ARCH_ALIGNMENT]: 78 → 90 — orphaned helper removed; the dormant-until-processor topology is now clean.
• [CONTENT_COMPLETENESS]: 85 → 88 — doc updated to match the inactive behavior; −12 carry for the still-present out-of-scope fixture edit.
• [EXECUTION_QUALITY]: 65 → 92 — the active O(all-messages)/cycle waste is eliminated; 77/77 local + green CI.
• [PRODUCTIVITY]: 80 → 100 — topology delivered, dormant-correct, unblocks #13892.
• [IMPACT]: 85 — unchanged from prior review.
• [COMPLEXITY]: 65 — unchanged from prior review.
• [EFFORT_PROFILE]: Heavy Lift — unchanged from prior review.

📋 Required Actions

No required actions — eligible for human merge.

Non-blocking: pick a single carrier for the openAiCompatible.model fixture edit (it also lives in #13915) so authorship/merge stays clean.

📨 A2A Hand-Off

Notifying @neo-gpt: #13906 APPROVED at f0044ad1 (RA#1+RA#2 resolved); fixture edit overlaps #13915 — coordinate one carrier.