PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Delivers the #12752 intent cleanly — the A2A mailbox/permission/wake surface stays reachable when summarization/vector degrades, so the swarm can coordinate the very recovery — with a precise gate-bypass test, plus a verified-complete cleanup of the dead start_database/stop_database references. No blocking defects. The one challenge (exempt-list data-independence assumption) is a watch-item, not a merge blocker.

Opening: Clean, well-tested decoupling. The "mailbox stays up during a vector/summary incident" invariant is exactly the right resilience shape, and folding in the dead start/stop-DB reference cleanup is tidy. Notes below; no blockers.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: ticket #12752 (bug/regression/architecture), the changed-file list, getHealthExemptTools + the health-gate dispatch path in both Server.mjs files, HealthService.mjs on current dev, the MemoryCore docs.
• Expected Solution Shape: decouple the A2A mailbox tools from summarization/vector readiness — the mailbox handlers must NOT be gated on the health check that depends on Chroma/embedding; a test should prove mailbox tools execute while the gate is degraded and that vector-dependent writes stay gated. Should not hardcode the boundary in a way that silently drifts.
• Patch Verdict: Matches (improves). MC getHealthExemptTools() now exempts the 12 graph/SQLite-scoped A2A tools; the gate-bypass test proves list_messages skips ensureHealthy while add_memory stays gated and surfaces the degraded reason. The start/stop-DB retirement is a coherent adjacent cleanup (exempt lists + docs + the HealthService error string + log tips).

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12752
• Related Graph Nodes: #12740 (cost-safety epic), #12742, #12741, #12746, #12748

🔬 Depth Floor

Challenge (non-blocking): The exemption is a hardcoded 12-tool enumeration resting on the assumption that every listed tool is vector/Chroma-independent (graph/SQLite-only). The MC test proves the gate mechanics (list_messages skips ensureHealthy, add_memory is gated) but stubs getToolService, so it does not verify that each exempt tool is actually Chroma-independent at runtime. If any listed tool has (or grows) a hidden vector path — e.g. a future add_message that embeds for semantic mailbox search — exempting it would let it run and fail cryptically during the very degradation the gate protects against. This aligns with the mailbox(SQLite)/memory(Chroma) architecture split, so it's almost certainly sound today — but it's the load-bearing assumption. Two watch-items (neither blocks merge):

• (a) Worth an empirical isolation confirmation (§5.1) that none of the 12 (esp. add_message, transition_task) touches Chroma.
• (b) The manual exempt list can drift as new A2A tools are added — a tool-level data-dependency tag (dataScope: 'sqlite' | 'vector') would make the exemption derived rather than hand-maintained. Reasonable follow-up if the surface grows.

Rhetorical-Drift Audit: Pass. The JSDoc ("The A2A mailbox/permission surface is graph/SQLite-scoped and must remain reachable during summarization/vector-provider incidents") accurately describes the mechanical change (those tools added to the exempt list). The docs' "Database lifecycle is managed outside the MCP tool surface" is verified accurate — see Cross-Skill audit. No overshoot.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: Health-gating should distinguish data-dependency classes — a vector/Chroma outage must not sever the SQLite/graph-scoped A2A coordination surface the swarm uses to recover from that very outage. "Mailbox stays up" is a resilience primitive, not a convenience.

🎯 Close-Target Audit

• Close-targets identified: #12752 (newline-isolated Resolves).
• #12752 confirmed bug/ai/regression/architecture/model-experience — not epic. ✓

Findings: Pass. Related: #12740/#12742/#12741/#12746/#12748 are non-closing extras.

🔗 Cross-Skill Integration Audit

The PR retires the start_database/stop_database tool references and updates the docs. I V-B-A'd the retirement completeness (the key risk: docs claiming "retired" while the tools stay callable):

• grep for start_database/stop_database/startDatabase/stopDatabase across both openapi.yaml files → zero matches. The only references in ai/mcp/server/ + ai/services/ were the 5 sites this PR removes (exempt lists, log tips, the HealthService error string). No handler, no openapi definition — they were already dead name-references, not live tools.
• So the docs ("Database lifecycle is managed outside the MCP tool surface") are now accurate, and MemoryCore.md / MemoryCoreMcpApi.md are correctly updated to match.

Findings: All checks pass — no integration gap. The cleanup is complete (dead references removed), not a partial docs-drift.

🧪 Test-Execution & Location Audit

• Branch checked out locally (gh pr checkout 12755, head 7706c1353).
• Canonical Location: test/playwright/unit/ai/mcp/server/{knowledge-base,memory-core}/Server.spec.mjs — correct MCP test dir per unit-test.md.
• Ran the related specs: npm run test-unit -- .../memory-core/Server.spec.mjs .../knowledge-base/Server.spec.mjs → 7 passed (incl. the #12752 mailbox-bypass + retired-tools tests).

Findings: Tests pass; correct location.

N/A Audits — 📑 📡 🛂 🔌 🪜

N/A across listed dimensions: no Contract Ledger drift (the delivered behavior matches #12752's intent and the docs are synced; the retirement is a dead-reference removal, not a new consumed surface); no openapi.yaml description: touched (📡); no major new abstraction (🛂); no wire-format/schema change — the exempt list is internal dispatch policy (🔌); ACs are dispatch-policy, fully L2 unit-covered, no runtime-only residuals (🪜, matching the PR's Evidence: L2 declaration).

📋 Required Actions

No required actions — eligible for human merge. (The two Depth-Floor watch-items are non-blocking follow-ups, not merge gates.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 deducted because the data-dependency boundary is enforced by a hand-maintained tool list rather than a derived dataScope tag; the resilience model itself (SQLite-scoped survives vector outage) is the right paradigm.
• [CONTENT_COMPLETENESS]: 95 — 5 deducted: the exempt-list JSDoc states the why well, but a one-line note on the data-independence assumption (and its drift risk) at the list site would future-proof it. Docs + Fat Ticket otherwise complete.
• [EXECUTION_QUALITY]: 92 — 8 deducted because the gate-bypass test stubs tool execution, so it proves dispatch policy but not per-tool Chroma-independence (the load-bearing assumption). Tests green locally (7/7), unit green on CI.
• [PRODUCTIVITY]: 100 — I confirmed the #12752 dispatch-policy ACs (mailbox bypasses, memory writes gated) are met by the tests, the retirement is complete, and docs are synced; no AC left unaddressed.
• [IMPACT]: 80 — a targeted resilience fix on a cost-safety incident path (#12740): keeps swarm A2A coordination alive during a vector/summary degradation. Major for incident recovery, scoped to the dispatch gate.
• [COMPLEXITY]: 45 — Low-moderate: one dispatch-policy method per server + a focused handler-level test with a stubbed health service; the reasoning load is the data-dependency distinction, not the LOC.
• [EFFORT_PROFILE]: Quick Win — high resilience ROI for a small, well-tested dispatch-policy change plus a clean dead-reference sweep.

[KB_GAP] / [TOOLING_GAP]: none.

Thanks for this — the mailbox-survives-degradation invariant is a genuinely good resilience primitive, and I verified the start/stop-DB cleanup is complete (no orphaned openapi/handler). The only thing I'd keep an eye on is the exempt list's data-independence assumption as the A2A surface grows.