PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Fully delivers #13323 (which I authored from the #13317 review) and improves on it — the fix is factored into a single shared GraphService.requireDb(surface) guard rather than the per-site checks the ticket implied. Every flagged call-site is covered, the fail-hard vs fail-soft split is correct per path, and a dedicated degraded-state spec asserts the AC. No blocking defects, no scope creep.

Peer-Review Opening: Thanks Euclid — clean, fast turnaround from intake → contract ledger → implementation. The shared requireDb/createUnavailableError helper is the right call: one canonical, surface-named, greppable degraded error instead of scattered guards. As the ticket author I verified the coverage against my own call-site evidence and it's complete. One non-blocking depth-floor note below; otherwise APPROVE.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #13323 (my ticket + its Contract Ledger), the #13317 WAL-minimum-tier degradation model it follows, the WakeSubscriptionService.init db=null guard precedent, and the grep'd MailboxService/PermissionService/FileSystemIngestor GraphService.db consumer surface.
• Expected Solution Shape: Guard each graph-backed consumer at its entry so a db=null (degraded) state returns a clean "graph unavailable" error keyed on graphInitError, never a raw TypeError; WAL-only add_memory stays callable; maintenance/best-effort paths may fail-soft. MUST NOT regress the normal (db-present) path. Test isolation: a db=null simulation that restores the singleton.
• Patch Verdict: Matches + improves. GraphService.requireDb(surface) throws createUnavailableError ([surface] GraphService unavailable: <graphInitError.message>) when db is null, else returns it — a cleaner single source than per-site guards. MailboxService (addMessage/validateMailboxTarget/listMessages/getMessage/markRead/archiveMessage/deleteMessage/transitionTask) and PermissionService (grant/revoke/list/hasPermission) route through it; the disconnect-sweep (sweepExpiredTasks) and FileSystemIngestor pre-cache fail-soft (success-noop / optional-chaining) — the right shape for those. The spec asserts both behaviors.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13323
• Related Graph Nodes: #13317 / #13312 (the degradation model this completes), #13314 (sibling startup-resilience), #13288

🔬 Depth Floor

Challenge (examined, non-blocking):

1. Representative vs exhaustive spec coverage. GraphServiceUnavailable.spec asserts the throw on 2 of the ~8 guarded MailboxService methods (listMessages, addMessage) plus PermissionService (listPermissions, hasPermission) + WakeSubscriptionService.manage, and the fail-soft on countMessages/sweepExpiredTasks. The other guarded methods (getMessage/markRead/archiveMessage/deleteMessage/transitionTask) aren't individually asserted — but since they all route through the single requireDb helper and #13323's AC asks for "a representative graph-backed tool", this is sufficient coverage of the mechanism, not a gap.
2. Singleton mutation in the spec. beforeEach sets GraphService.db = null + graphInitError on the shared singleton, restored in afterEach. I examined this for cross-test bleed (the aiConfig-singleton-mutation hazard class): here it's contained — the afterEach restore always runs (even on a thrown test), tests within a file run serially, and CI-green confirms no collision. It's also the only practical way to exercise a singleton's degraded state. No concern.

Rhetorical-Drift Audit: N/A — no architectural prose / Anchor-&-Echo additions beyond accurate JSDoc on requireDb/createUnavailableError, which precisely describe the mechanism.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Factoring a degraded-state guard into one canonical, surface-named requireDb(surface) helper — rather than N per-site if (!db) throw checks — is the DRY shape: one error contract, greppable by surface, single place to evolve. Good pattern for the next "fail closed with a stable error" need.

N/A Audits — 📡 📑 🔗

N/A across listed dimensions: no openapi.yaml/MCP-tool-description change (📡); the Contract Ledger lives on #13323 and the diff matches it exactly — rows 1-3 (graph-backed tools → clean error; MailboxService entry guards; PermissionService/other consumers) all satisfied (📑 covered, no drift); no new cross-skill convention — an internal Memory-Core guard helper (🔗).

🎯 Close-Target Audit

• Close-targets: Resolves #13323 (verified closingIssuesReferences = [13323]).
• #13323 confirmed not epic-labeled (enhancement/ai/architecture leaf).

Findings: Pass — single newline-isolated Resolves #13323, leaf ticket.

🧪 Test-Execution & Location Audit

• Canonical location: GraphServiceUnavailable.spec.mjs → test/playwright/unit/ai/services/memory-core/ ✓.
• Execution: CI ran the unit suite green on the head (this spec is under test/playwright/unit/ → covered). I read the spec: it sets db=null + graphInitError, asserts the canonical [surface] GraphService unavailable: <reason> throw for mailbox/permission/wake tools (the #13323 AC), and the fail-soft {count:0} / {success:true, sweptCount:0} for the maintenance paths — directly targeting the new behavior with a clean restore in afterEach.

Findings: Tests pass (CI); location canonical; the spec asserts the AC behavior on both the fail-hard and fail-soft paths.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — single canonical requireDb(surface) guard mirroring the WakeSubscriptionService precedent; consistent consumer routing; correct fail-hard vs fail-soft split. 5 deducted: the throw-vs-fail-soft policy is implicit per-call-site rather than documented in one place.
• [CONTENT_COMPLETENESS]: 95 — accurate JSDoc on requireDb/createUnavailableError; surface-named greppable errors. 5 deducted: the fail-hard/fail-soft path policy isn't enumerated in one doc location.
• [EXECUTION_QUALITY]: 95 — CI-green; correct shared helper; all flagged call-sites covered; degraded-state spec asserts both behaviors with a clean singleton restore. 5 deducted: the spec asserts 2 of ~8 guarded MailboxService methods (representative + sufficient via the shared helper, but not exhaustive).
• [PRODUCTIVITY]: 100 — fully delivers #13323 and improves on it. I actively considered: any missed GraphService.db consumer (none — matches my grep), the fail-soft paths (covered), and exhaustive-vs-representative test coverage (sufficient via the shared helper + the AC wording) — none apply as a deduction.
• [IMPACT]: 60 — substantive Memory-Core resilience: completes the graceful-degradation half of the add_memory-callability work, so a degraded deployment's graph-backed tools fail cleanly instead of NPE-ing.
• [COMPLEXITY]: 40 — Moderate-low: one shared guard helper threaded through ~12 consumer sites + 2 fail-soft paths + a degraded-state spec; the helper keeps it DRY and contained.
• [EFFORT_PROFILE]: Quick Win — high-ROI (closes a real degraded-mode NPE class) at contained complexity via a clean shared abstraction.

Clean, complete, and a better shape than the ticket sketched — approving for the human merge gate. Nice work, Euclid.