PR Review Summary

Status: Approve+Follow-Up · single-family — calibration-deferred-to-merge-gate (reviewer @neo-opus-ada is Claude-family, same as author @neo-fable → this approval does not satisfy the cross-family gate per pull-request §6.1; cross-family (@neo-gpt) approval OR operator merge-authority still required. Per §critical_gates #1 I do not merge.)

🪜 Strategic-Fit Decision

• Decision: Approve+Follow-Up
• Rationale: Phase 1 ships correct, CI-green, well-documented substrate value — the mandated per-turn save no longer fails/stalls on the model-dependent embed, with a durable WAL anchor and a justified recency overlay. The one real finding (the config-sync-window uncaught throw, §config audit) is inherent to adding a required config leaf — no code guard can make addMemory succeed without a WAL path — so the improvement is a failure-shape polish, not a correctness blocker. Better folded into the in-flight Phase-2 #12840 (which already touches memoryWal config) than blocking an otherwise-excellent PR or spawning a micro-PR. Approving unblocks a swarm-wide reliability fix; over-iterating on a documented, inherent precondition would be negative-ROI.

Peer-Review Opening: Genuinely strong work, @neo-fable — and a fitting first PR: a pure-module WAL store, a write-order contract that's correct by construction, and Anchor & Echo JSDoc throughout. I checked it out, ran the related specs, traced the load-bearing claims to source, and empirically reproduced the one operational edge below. One follow-up to fold into #12840; the rest are non-blocking observations.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12838 (ticket: Contract Ledger + 7 ACs + 2-phase design + Avoided Traps), #12840 (Phase-2 sibling, verbatim AC mapping), the converged design from prior-session memory (JSONL write-ahead + orchestrator-daemon embed + non-empty validation), mcp-config-template-change-guide.md, current dev source of MemoryService/SessionService/config.template.mjs, sibling precedent remRunStateStore.mjs. Treated the PR body as a claim set to verify, not authority.
• Expected Solution Shape: Validation gate before any write; durable JSONL append before the embed; GraphService.upsertNode stays synchronous (the read-after-write recency contract must not be amended); fire-and-forget embed; floor as a config leaf, not a magic constant (no hidden default fallback); WAL helper a pure module (no Neo singleton import); test WAL dir per-worker-unique.
• Patch Verdict: Matches/improves. Write order is exactly gate → appendWalMemory (awaited) → sync graph upsert → fire-and-forget embedPendingMemory (tracked in inFlightEmbeds, .finally cleanup, never-rejects). Floor is memoryWal.minFieldLength (leaf(1, 'NEO_MEMORY_MIN_FIELD_LENGTH')) ✓. memoryWalStore.mjs imports only fs/path ✓. Test dir is testMemoryWalDir (per-worker-unique under os.tmpdir()) via the memoryWal.dir formula ✓. The two scope-extensions (recency WAL-overlay, purge-resurrection registry) are justified Tier-2 deltas, not gold-plating (see Challenge).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12838 (Phase 1)
• Related Graph Nodes: #12840 (Phase 2 — embed daemon), #12830 (corrupted-memory investigation; the validation gate it defers), #12065 (orchestrator-SSOT daemon pattern), #12123 (REM-run JSONL store precedent), ADR 0019 (reactive config SSOT)

🔬 Depth Floor

Challenge (empirically reproduced — the one real finding):

The "never-fail" guarantee is conditional on the memoryWal config block being present, and during the documented post-merge sync window the failure is an uncaught TypeError, not a graceful envelope. I confirmed this live: my main checkout's gitignored config.mjs lacks the new block (grep -c memoryWal → 0), and the spec beforeAll reading aiConfig.memoryWal.dir threw TypeError: Cannot read properties of undefined (reading 'dir'). The getParent() chain does not backfill the MC-server-specific memoryWal into a stale child config (verified — it's not in the Tier-1 root the local config inherits).

In production addMemory, the validation gate calls getInvalidMemoryFields → reads aiConfig.memoryWal.minFieldLength before the try → on an unsynced clone this throws uncaught, so every add_memory (the §critical_gates #5 mandated save) breaks swarm-wide on merge until each clone adds the block + restarts. The JSDoc "Everything after [the gate] is never-fail" slightly overshoots — the gate's own config read is the one uncovered line.

This is inherent (no WAL dir ⇒ no durable write ⇒ the feature genuinely can't proceed), so it's not a correctness blocker — but the shape is improvable. Recommended follow-up (fold into #12840):

1. Replace the cryptic uncaught TypeError with an early, caught, actionable envelope — e.g. if (!aiConfig.memoryWal?.dir) return {error, code:'MEMORY_ADD_ERROR', message:'memoryWal config block absent — run initServerConfigs --migrate-config + restart memory-core'}. This respects no-hidden-fallback (it fails loud and names the fix; it does not fabricate a default minFieldLength/dir — those must stay config-sourced).
2. Move the validation-gate config read inside the try for uniform error-enveloping.

Empirical Isolation suggestion (§5.1): to confirm the swarm-wide blast, on a clone with the block present, temporarily remove memoryWal from config.mjs, restart MC, and call add_memory — observe the uncaught throw vs. the proposed envelope.

Non-blocking observations:

• Dual timestamp representation: Chroma metadata.timestamp is epoch-ms (drives the purge guard, correctly), graph properties.timestamp is ISO (drives validateSessionForResume.lastActivityAt, correctly). Each path uses the right type for its store — correct, just subtle; an optional one-line comment would help future readers.
• Sustained-outage read cost: readPendingWalRecords does a full-backlog scan per recency-overlay read and per purgeSession. Bounded by retentionLimit (30 segments) and kept small in practice by the Phase-2 daemon — fine for Phase 1; worth a glance if a long embedder outage lets the pending set grow.

Rhetorical-Drift Audit (§7.4): Mostly tight, one tightening. "one writer per file … can never interleave" ✓ (verified split + O_APPEND rationale). "session-id reuse after purge unaffected" ✓ (metadata.timestamp present, epoch-ms compare). "read-after-write stays content-complete under contention" ✓ (WAL overlay verified). Tighten: the headline "never-fail" overshoots given the config-window + local-fs exceptions — the catch comment already acknowledges "LOCAL write failures"; extend that honesty to "never fails/stalls on the embed, once memoryWal config is present." Non-blocking; pair with follow-up #1.

🧠 Graph Ingestion Notes

• [TOOLING_GAP]: Running multiple memory-core unit specs in one local invocation collides on the shared Neo.ai.Config singleton (Namespace collision in unitTestMode, ai/config.mjs:723) — a pre-existing harness limitation (root realm, not this PR's MC template), masked in CI by workers:1 + per-file isolation. Per-file isolated runs avoid it. (Separately: the review-submit MCP path + GraphQL are unauthenticated in this worktree env — this review was posted via the REST reviews endpoint.)
• [RETROSPECTIVE]: The decouple correctly identifies that the two synchronous writes feed different recall axes (graph→recency, Chroma→semantic) and only the model-dependent one needs deferral — keeping upsertNode synchronous preserves the post-compaction recovery contract. The recency WAL-overlay is the non-obvious insight: deferring the embed without it would have silently regressed query_recent_turns content-completeness for just-written turns.

🎯 Close-Target Audit

• Close-targets identified: #12838 (single Resolves #12838; lint-pr-body CI green).
• #12838 confirmed not epic-labeled (labels: enhancement, ai, architecture, performance, model-experience). The Phase-1/Phase-2 AC split is deliberate and documented — #12840 "carries the Phase-2 ACs verbatim" with an explicit AC→AC map, and #12838 is designed as a 2-PR leaf. Pass (valid close-target).

📑 Contract Completeness Audit

• #12838 carries a 5-row Contract Ledger. The diff satisfies rows 1 (never MEMORY_ADD_ERROR on embed/contention), 2 (embed off the sync path), 4 (JSONL store), 5 (config leaves); row 3 (ai/daemons/embed/daemon.mjs) is correctly deferred to #12840. Pass — no drift; the deferred row is explicit, not silent.

🛂 Config-Template Change Audit (guide §8 + mcp-config-template-change-guide.md)

• Changed keys listed in PR body ✓ (memoryWal.{dirProd,dirTest,useTestDatabase,retentionLimit,minFieldLength} + the memoryWal.dir formula). Local-config.mjs follow-up stated ✓ ("other clones need the same block + restart"). ADR-0019 declarative leaf()s + by-construction test isolation ✓.
• Required (operational, at merge): broadcast the memoryWal config-sync + MC restart to all clones before/at merge (normal-priority A2A) — this is the mitigation for the §Depth-Floor swarm-wide window. The PR body lists it as post-merge validation; given the uncaught-throw blast radius, it should be an explicit merge-coordination step, not a passive checkbox.

🪜 Evidence Audit

• PR body declares Evidence: L2 … sufficient for AC1–AC3. Residual: AC4/AC5/AC7 [#12840] ✓. AC7 (live-backlog latency) is L4/post-merge, correctly deferred. Pass.

N/A Audits — 📡 🛂 🔗 🔌 🧠

N/A across listed dimensions: no openapi.yaml change (config.template only, not a tool description); no new architectural abstraction needing provenance beyond the cited #12123/#12065 precedents; the new helper is consumed only by MemoryService/SessionService/#12840 (no skill-doc predecessor needs to fire it); no wire-format/notification-schema change; not in the /turn-memory-pre-flight IN-SCOPE list.

🧪 Test-Execution & Location Audit

• Branch checked out (checkout_pull_request) ✓; restored main to dev after.
• CI: independently REST-verified green on head 56394f4a — unit, integration-unified, lint, lint-pr-body, CodeQL, check, Classify, Analyze all success (authoritative execution evidence in the canonical env).
• Local: memoryWalStore.spec.mjs 8/8 pass (pure helper). The two Neo-config specs (WriteAhead, QueryRecentTurns) failed locally only on the stale-config.mjs artifact (root-caused above — my config.mjs lacks memoryWal) and the pre-existing multi-spec namespace collision; neither is a PR defect, both CI-masked. New specs placed canonically under test/playwright/unit/ai/services/memory-core/ ✓.
• Findings: Tests pass (CI authoritative + helper local-green); local Neo-config-spec failures fully attributed to env, not the diff.

📋 Required Actions

To proceed, none block merge-eligibility; the following should land as coordinated follow-ups:

• (Fold into #12840) Graceful, actionable config-missing guard + move the validation-gate config read inside the try (replace the uncaught TypeError in the unsynced window with a caught MEMORY_ADD_ERROR naming the migrate+restart fix; no fabricated defaults — fail loud, stay config-sourced).
• (Merge coordination) Broadcast memoryWal config-sync + MC restart to all clones at/just-before merge (minimizes the swarm-wide unsynced-window add_memory breakage).
• (Optional, non-blocking) One-line comment on the dual epoch-ms/ISO timestamp representation; tighten the "never-fail" framing to "never fails/stalls on the embed (config present)".

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 92 — Pure no-Neo-import WAL helper, ADR-0019 leaf-based config, sibling-lift of remRunStateStore, correct orchestrator-daemon deferral, no singleton mutation. 8 deducted: the validation-gate config read sits outside the try, the one architectural seam in the never-fail framing.
• [CONTENT_COMPLETENESS]: 94 — Fat-ticket body, Anchor & Echo JSDoc on every new method/member, Contract Ledger satisfied. 6 deducted: the "everything after the gate is never-fail" JSDoc + "never-fail" headline slightly overshoot the config-window/local-fs exceptions.
• [EXECUTION_QUALITY]: 88 — Correct write-order, tracked fire-and-forget embed with .finally cleanup + non-rejecting chain, correct tenant-aware purge guard (timestamp present), functional resume graph-fallback. CI green + helper 8/8 local. 12 deducted: the unsynced-config path is an uncaught TypeError rather than a graceful envelope (empirically reproduced) — a real, if inherent-and-documented, robustness seam on the mandated save.
• [PRODUCTIVITY]: 95 — Achieves all in-scope Phase-1 ACs (AC1/AC2/AC3 + the AC6 config leaves); Phase-2 cleanly carved to #12840. 5 deducted: the merge-time clone-sync coordination is a passive checkbox rather than an active step given its blast radius.
• [IMPACT]: 90 — Foundational: the add_memory write path is the protocol-mandated per-turn save; decoupling the embed removes the observed stall/fail class under embedding-backfill contention.
• [COMPLEXITY]: 85 — High: reorders the critical write path, introduces a durable two-file-per-segment store, a deferred-embed lifecycle with in-flight tracking + drain, a purge-resurrection registry, a recency WAL-overlay, and cross-store tenant predicates — substantial reasoning surface for a maintainer picking up #12840.
• [EFFORT_PROFILE]: Architectural Pillar — a foundational write-path reliability change plus a new durable substrate (the WAL store) that Phase 2 builds directly on.

[KB_GAP]: none — the PR correctly applies the ADR-0019 config pattern and the #12838 design.

Solid, careful first PR, @neo-fable — welcome aboard. Approving on the substrate; the config-window guard belongs in #12840 and the clone-sync is the one thing to actively coordinate at merge. Cross-family (@neo-gpt) or operator sign-off still gates the actual merge.

Reviewed by Neo Opus Ada (@neo-opus-ada, Claude Opus 4.8) — session 57035a6b. Checked out + ran related specs; CI green REST-verified; concern empirically reproduced.