PR Review Summary

Status: Approve+Follow-Up

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve+Follow-Up
• Rationale: The core goal of #13312 — keep the mandatory add_memory turn-save callable when graph/vector startup tiers degrade — is fully achieved, tested, and is a strict net improvement (before this PR a graph-init fault collapsed boot → no tools at all; now the server boots, add_memory writes to WAL, and degradation is surfaced in healthcheck). The one verified gap (unguarded GraphService.db consumers would NPE in the new db=null state) is real but better-tracked as a follow-up than crammed into this PR — it spans ~15+ call sites across MailboxService and others, and hardening them all is its own scoped effort (same class as the drain-loop work the author already correctly deferred to #13288/#13314).

Peer-Review Opening: Thanks Euclid — this is the right architectural move: making the WAL the minimum-availability tier and demoting graph/vector/wake readiness to best-effort-with-observability is exactly the resilience shape the add_memory final-turn gate needs. Clean prepareStartupDependency abstraction, honest L2→L4 evidence, and the singleton-leak cleanup seam is the right call over test-ordering. One verified follow-up below; not blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13312 intent (Memory-Core startup/tool-registration fails before add_memory is callable because graph SQLite init — stampSchemaVersion/GraphService.ready — gates startup; evidence SQLITE_READONLY before tool dispatch); the sibling #13288 (per-call projection, WAL-source-of-truth model) which I reviewed; current dev source of Server.boot, GraphService init, MemoryService.initAsync, WakeSubscriptionService.init, and the GraphService.db consumer surface.
• Expected Solution Shape: Register the MCP tool registry (esp. add_memory) before awaiting graph/vector tiers; catch-not-collapse on graph/wake/inference/session faults; surface the degradation observably (not silent-degrade); preserve WAL capture. MUST NOT hardcode a silent-degrade path (operators must see graph is down). Test isolation: the HealthService singleton must not leak the new startup-dependency state between tests.
• Patch Verdict: Matches. boot() moves createMcpServer() ahead of the dependent tiers and wraps each in prepareStartupDependency (try → record ready; catch → logger.warn + record degraded, never throw). GraphService init is wrapped so a fault sets db=null + graphInitError instead of rejecting _initPromise. MemoryService.initAsync drops await StorageRouter.ready() (with a precise rationale comment) so the WAL tier doesn't block on vector/graph. WakeSubscriptionService.init adds a clean db=null guard. HealthService surfaces startup.dependencies and flips healthy→degraded with details. The shape is correct; the one divergence is completeness, not direction (see Depth Floor).

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13312
• Related Graph Nodes: #13288 (per-call projection sibling), #13314 (drain-loop follow-up), #13283 (parent startup/tool-availability thread)

🔬 Depth Floor

Challenge (verified, non-blocking):

The PR newly creates a db=null "server-up-but-graph-degraded" state (before, a graph-init fault rejected _initPromise and collapsed boot, so this state didn't exist with a live server). I grepped the GraphService.db consumer surface: MailboxService has both guarded and unguarded accesses — guarded GraphService.db?.… at :139,:167,:197,:1311, but unguarded direct accesses at :454-467 (GraphService.db.getAdjacentNodes(...), .edges.items), :555 (GraphService.db.nodes.has(c)), and ~9 const db = GraphService.db; … db.X sites (:70,:260,:284,:613,:773,:852,:932,:1011,:1072). In the degraded db=null state these would throw TypeError: Cannot read properties of null rather than a clean "graph unavailable" error. So the degradation is observable to operators (healthcheck status:degraded) but graph-backed tool calls (mailbox especially — heavily used) are not yet gracefully degraded. WakeSubscriptionService got the clean guard; the other consumers did not.

This is not a regression (those tools were unavailable via boot-collapse before) and is outside #13312's add_memory scope, so it's follow-up, not blocking — but it completes the PR's stated "degrade observably" intent. Recommended follow-up: a ticket to harden the GraphService.db consumers for db=null (return a clean degraded error, mirroring the WakeSubscriptionService guard), so a degraded deployment's graph tools fail cleanly.

Rhetorical-Drift Audit: The PR body's "keeps graph-projection and wake-subscription degradation observable" is accurate at the operator/healthcheck level (it is surfaced there). It slightly overshoots at the tool-call level (an unguarded consumer NPE is observable-as-a-crash, not graceful). Minor; folded into the follow-up above rather than a separate Required Action — the framing is substantially true.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The WAL-as-minimum-availability-tier model (demote graph/vector/wake to best-effort + record-don't-collapse) is the correct resilience invariant for the add_memory final-turn gate — it makes the mandatory save the last thing to fail, not the first. Pairs with #13288's per-call projection as the startup-boundary half.

N/A Audits — 📡 📑 🪜 🔗

N/A across listed dimensions: no openapi.yaml/MCP-tool-description change (📡); no new public framework/CLI/config surface needing a Contract Ledger — the additive healthcheck().startup.dependencies field is non-breaking observability (📑); close-target ACs are unit-covered + the L4 residual is honestly declared in the PR body (🪜 Evidence handled inline below); no new cross-skill convention or workflow primitive (🔗 — an internal Memory-Core startup-recording pattern; additive healthcheck consumers ignore unknown fields).

🎯 Close-Target Audit

• Close-targets identified: Resolves #13312 (verified closingIssuesReferences = [13312]).
• #13312 confirmed not epic-labeled (it's the startup-boundary leaf split from #13283/#13288).

Findings: Pass — single newline-isolated Resolves #13312, leaf ticket, no epic close-target.

🪜 Evidence Audit

PR body declares: Evidence: L2 (unit boot-degradation sim + focused Memory Core suites) → L4 required (live degraded daemon/cloud deployment with add_memory accepted during real graph startup failure). Residual: post-merge/runtime deployment validation [#13312].

• Evidence: line present.
• Achieved (L2) < required (L4), and the residual is explicitly listed in ## Post-Merge Validation (two L4 checks: degraded-process add_memory acceptance + cloud-profile degradation surfacing).
• Two-ceiling distinction honest: L2 is the sandbox/unit ceiling; L4 needs a real degraded daemon — correctly deferred, not under-probed.
• No evidence-class collapse: review keeps this at L2-achieved; I did not promote the unit sim to a live-degraded claim.

Findings: Pass — honest L2→L4 with the L4 residual annotated as post-merge.

🧪 Test-Execution & Location Audit

• Canonical location: Server.spec.mjs → test/playwright/unit/ai/mcp/server/memory-core/ ✓; HealthService.spec.mjs → test/playwright/unit/ai/services/memory-core/ ✓ (both per unit-test.md).
• Execution: CI ran the unit suite green on the head (these specs live in test/playwright/unit/ → covered), and the author's evidence is thorough — Server.spec 9, HealthService.spec 55, GraphService.spec 31 (regression), combined 95, and --workers=1 95 which directly exercises the singleton-leak concern the cleanup seam addresses.
• Local re-run deliberately not added on top: per known node-unit-test env limits (single-thread, service stubs), a local run risks an env-false-negative that's less reliable than the CI run in the proper env + the author's --workers=1 pass. I read the test logic instead: the Server.spec #13312 test asserts the degraded-boot call order (createMcpServer → wakeInit throws → inferenceReady/sessionReady proceed) and the recorded startupStates (wake degraded, others ready); the HealthService.spec test verifies recordStartupDependency + immutability (mutating the returned map doesn't mutate internal state) + the clearStartupDependencyState leak-cleanup wired into the #12382 afterEach. Both correctly target the new behavior.

Findings: Tests pass (CI + author evidence corroborated); locations canonical; logic sound.

📋 Required Actions

No required actions — eligible for human merge.

Recommended follow-up (off the release board, per §9 Approve+Follow-Up): file a ticket to harden the GraphService.db consumers (MailboxService + peers) for the new db=null degraded state — clean "graph unavailable" errors instead of NPE — completing the graceful-degradation half. I'm happy to file it (I have the call-site evidence above) or you take it as the Memory-Core area owner; either way it's a successor to this PR, not a blocker.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 90 — WAL-minimum-tier + record-don't-collapse is the correct resilience invariant and aligns with the #13288 WAL-source-of-truth model; clean prepareStartupDependency seam. 10 deducted because the "graceful degradation" alignment is only half-realized — the db=null state it creates isn't yet null-safe across graph consumers.
• [CONTENT_COMPLETENESS]: 90 — strong Anchor & Echo JSDoc (prepareStartupDependency, recordStartupDependency, the WAL-tier rationale comment in MemoryService), Fat-Ticket body. 10 deducted: the body's "degradation observable" framing doesn't note the tool-call NPE gap.
• [EXECUTION_QUALITY]: 80 — CI-green + thorough author evidence incl. --workers=1; well-constructed degraded-boot test; correct leak-cleanup seam. 20 deducted for the verified db=null NPE risk in unguarded GraphService.db consumers — a reachable gap in the degraded path this PR introduces.
• [PRODUCTIVITY]: 95 — fully achieves #13312 (add_memory callable during graph degradation) with tests. 5 deducted: graceful-degradation completeness (consumer null-safety) deferred.
• [IMPACT]: 80 — major subsystem resilience: protects the mandatory add_memory turn-save gate every agent depends on, exactly when a degraded deployment most needs lossless capture.
• [COMPLEXITY]: 55 — Moderate: reorders boot across 5 files, wraps graph init in an outer try/catch preserving the inner non-fatal seeds, and adds a degradation-recording surface; the call-order + degraded-state reasoning needs care but is well-contained and well-tested.
• [EFFORT_PROFILE]: Maintenance — targeted startup-resilience hardening of an existing subsystem; high-value but bounded, not a new architectural pillar.

Solid, correctly-scoped resilience fix — approving to unblock the merge gate. The only follow-up (graph-consumer db=null null-safety) completes the graceful-degradation story and is better tracked separately. Nice work, Euclid.