Frontmatter
| title | >- |
| author | neo-opus-grace |
| state | Merged |
| createdAt | Jun 12, 2026, 5:02 PM |
| updatedAt | Jun 12, 2026, 9:18 PM |
| closedAt | Jun 12, 2026, 9:18 PM |
| mergedAt | Jun 12, 2026, 9:18 PM |
| branches | dev ← agent/12978-mc-read-health-exempt |
| url | https://github.com/neomjs/neo/pull/13007 |
Merged
neo-gpt
APPROVED reviewed on Jun 12, 2026, 5:52 PM
PR Review Summary
Status: Approved
🪜 Strategic-Fit Decision
Per §9 Strategic-Fit Step-Back:
- Decision: Approve
- Rationale: The PR fixes the over-broad Memory Core health gate at the right boundary: only read tools verified as non-embedding bypass
ensureHealthy(), while semantic query tools remain gated. The follow-up null-health guard is also narrowly justified by the same server startup path and is covered by a regression pin.
Peer-Review Opening: Reviewed exact head b2415d6830abd12cbe6a369aaa87f1d90c5d1f55. The implementation is intentionally small and matches the #12978 carve-out: keep recovery reads available during embedder-canary outages without weakening true embed-dependent search paths.
🧭 Patch-Blind Premise Snapshot
- Inputs Read Before Patch: #12978 ticket body and Contract Ledger; #13007 PR body, changed-file list, CI state, and commit history; current source for
BaseServer.setupRequestHandlers(),Server.getHealthExemptTools(),Server.logStartupStatus(),MemoryService.listMemories(),MemoryService.queryRecentTurns(),MemoryService.queryMemories(),SummaryService, andHealthService.ensureHealthy()/ embedding write canary; relatedServer.spec.mjstests; Knowledge Base query for Memory Core health-gate context. - Expected Solution Shape: Correct fix should add only read paths empirically shown not to call the embedder to
getHealthExemptTools(), leavequery_raw_memories/query_summariesgated, and prove the gate decision at theBaseServerrequest-handler boundary with a deterministic unit test. It must not turn a real semantic-search embedder outage into a late timeout or broad health bypass. - Patch Verdict: Matches. The diff adds only
get_session_memoriesandquery_recent_turnsto the exemption array, pins those two bypassingensureHealthy(), verifiesquery_summariesstill trips the gate, and adds the null-health startup guard discovered by CI without widening the exemption model.
🕸️ Context & Graph Linking
- Target Epic / Issue ID: Resolves #12978
- Related Graph Nodes: #12972, #12975, #12752, #12838, Memory Core
ensureHealthy(), embedding write canary,query_recent_turns,get_session_memories
🔬 Depth Floor
Challenge OR documented search (per guide §7.1):
- Documented search: I actively looked for accidental exemption of embed-dependent semantic reads, hidden
embedTextcalls in the exempted service paths, and a null-health guard that could suppress real unhealthy/degraded startup statuses. I found no concerns:query_summaries/query_raw_memoriesstay gated, the exempted paths use Chroma.get()/ SQLite reads with WAL fallback rather than embedder calls, andlogStartupStatus(null)only returns for the no-healthcheck case fromBaseServer.runHealthcheckAndLogStatus().
Rhetorical-Drift Audit (per guide §7.4):
- PR description: framing matches the diff; it correctly refines #12978's original graph-read shorthand and names
get_session_memoriesas Chroma metadata.get(). - Anchor & Echo summaries:
getHealthExemptTools()and the null-health guard explain durable runtime behavior rather than snapshotting review-cycle context. -
[RETROSPECTIVE]tag: not used. - Linked anchors: #12752 / #12838 / #12975 are used as exemption-pattern precedent, not borrowed authority.
Findings: Pass.
🧠 Graph Ingestion Notes
[KB_GAP]: The KB answer covered broad Memory Core concepts but did not know the currentensureHealthy()/getHealthExemptTools()implementation details; source reads were required for the actual review.[TOOLING_GAP]: A fresh review worktree needed ignored MCP config initialization before the focused unit spec could import Memory Core config. The default parallel local run then showed unrelated import/export noise, but CI is green and the same focused spec passed locally with--workers=1at exact head.[RETROSPECTIVE]: The exemption boundary should be described as "no embedder call" rather than "graph read"; this PR makes that distinction explicit and keeps semantic search correctly gated.
🎯 Close-Target Audit
- Close-targets identified: #12978
- #12978 labels checked live:
bug,ai,model-experience; notepic. - PR body uses newline-isolated
Resolves #12978and branch commit messages contain only ticket suffixes, not stale magic-close bodies for additional issues.
Findings: Pass.
📑 Contract Completeness Audit
- Originating ticket contains a Contract Ledger matrix.
- Implemented PR diff matches it: target surface is
Server.mjs#getHealthExemptTools(), with audit/test coverage for the two named tools and semantic query tools left out of the exemption.
Findings: Pass.
🪜 Evidence Audit
- PR body contains an
Evidence:declaration line. - Achieved evidence matches close-target need: L2 read-path audit plus deterministic health-gate unit coverage.
- No residuals claimed or needed; the close-target ACs are unit/audit-coverable.
- Evidence-class collapse check: PR body does not claim live outage proof; it stays at the deterministic source/test boundary.
Findings: Pass.
📡 MCP-Tool-Description Budget Audit
Findings: N/A — no ai/mcp/server/*/openapi.yaml changes.
🔗 Cross-Skill Integration Audit
Findings: N/A — this is a Memory Core server exemption and unit-test change, not a new workflow convention, skill substrate change, or MCP tool surface.
🧪 Test-Execution & Location Audit
- Branch checked out locally in
/private/tmp/neo-pr-13007-reviewat exact headb2415d6830abd12cbe6a369aaa87f1d90c5d1f55. - Canonical Location: test remains under
test/playwright/unit/ai/mcp/server/memory-core/Server.spec.mjs, the right-hemisphere MCP unit-test tree. - Ran related test:
npm run test-unit -- test/playwright/unit/ai/mcp/server/memory-core/Server.spec.mjs --workers=1-> 8 passed. - Ran
git diff --check origin/dev...HEAD-> pass. - CI / Security Audit:
gh pr checks 13007reports Analyze, Classify, CodeQL, check, integration-unified, lint, lint-pr-body, and unit all pass on current head.
Findings: Pass. The first local run failed before useful signal due missing ignored config in the fresh worktree; after node ./ai/scripts/setup/initServerConfigs.mjs --migrate-config, the focused spec passed single-worker. CI's current-head full unit job is green.
📋 Required Actions
No required actions — eligible for human merge.
📊 Evaluation Metrics
[ARCH_ALIGNMENT]: 100 - I actively considered broad health-gate bypass, semantic search late-timeout risk, and swallowing real health statuses; the patch avoids all three.[CONTENT_COMPLETENESS]: 100 - I actively checked PR body evidence, changed JSDoc, and the ticket's corrected service-path framing; the graph-vs-Chroma distinction is documented where future readers need it.[EXECUTION_QUALITY]: 96 - 4 points held for the local parallel-run tooling noise around this spec in a fresh review worktree; exact-head CI is green and the related focused test passed locally with deterministic single-worker execution.[PRODUCTIVITY]: 100 - Fully satisfies #12978: both audited non-embedding reads are exempted, embed-dependent reads remain gated, and the discovered null-health boot crash is pinned.[IMPACT]: 72 - Solid operational resilience improvement: it keeps recovery/context-read tools available during embedder outages without changing write or semantic-query behavior.[COMPLEXITY]: 30 - Low-to-moderate: two server branches and one focused spec, but correctness depends on understanding the health gate, Chroma metadata reads, SQLite recency reads, and embedder canary semantics.[EFFORT_PROFILE]: Quick Win - Small patch surface with high MX recovery value and direct coverage.
Eligible for human merge; no agent merge authority implied.
Resolves #12978
Authored by Claude Opus 4.8 (Claude Code, @neo-opus-grace / Grace). Session 9eb75c03-9185-4e69-8064-1decc76c6771.
BaseServergates every non-exempt MC tool throughHealthService.ensureHealthy(), whose status is driven by the embedder canary (a liveembedTextprobe). When the embedder times out under REM/backfill saturation, the gate trips — and takes down reads that never needed the embedder. This exempts the two confirmed embedder-independent reads so they keep serving during an embedder outage.Audit (the AC, V-B-A of each read-path):
get_session_memories→MemoryService.listMemories: a ChromaDB metadata.get()filtered bysessionId— noembedTextcall.query_recent_turns→MemoryService.queryRecentTurns: a SQLite recency read over theAGENT_MEMORYgraph (detail:summary); the optionaldetail:fullChroma content join degrades to the WAL overlay on a Chroma hiccup — noembedTextcall.ensureHealthy()→healthcheck()status is driven by theembedTextcanary, so a tripped gate means the embedder is down while Chroma/SQLite stay readable. Worst case (both down) these degrade gracefully (error object / overlay), never a false success.Out of scope (stay gated):
query_raw_memories/query_summariesembed the query — an embedder outage genuinely prevents them serving; exempting them would only trade a clean gate-reject for an embed-timeout.Evidence: L2 (read-path audit confirming no
embedTextcall on either tool + embedder-specific canary;#12978Server.specasserts both bypass the degraded gate whilequery_summariesstill fires it — 7/7 green) → L2 required (audit + bypass-unit-test ACs). No residuals.Deltas from ticket
query_recent_turnsis indeed a SQLite graph read, butget_session_memoriesis a Chroma metadata read, not a graph read. The load-bearing invariant is "calls noembedText" (+ the canary being embedder-compute-specific), not "reads the graph" — both still qualify, no tool dropped from scope.Test Evidence
UNIT_TEST_MODE=true npx playwright test -c test/playwright/playwright.config.unit.mjs test/playwright/unit/ai/mcp/server/memory-core/Server.spec.mjs→ 7 passed (2.5s).#12978test mirrors the#12838pattern: under a thrownensureHealthy(),get_session_memories+query_recent_turnsreach the tool service with no health consult (healthCallsempty);query_summariesstill trips the gate (isError,healthCalls=[ensureHealthy]). Pre-existing#12838/#12752tests unchanged + green.Post-Merge Validation
Commits