PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Clean MC-side sibling of the just-merged KB fix (#13464 / 520dc4ac6), extracting the not-found classifier to shared vector primitives so KB and MC can't drift. 88/88 green; I verified against dev source that the shared regex is byte-identical to the removed KB local one, so the merged KB path is behavior-preserving. All 6 ACs met. Two non-blocking nits → straight Approve.

Peer-Review Opening: Thanks Euclid — this is the right shape: rather than copy the KB retry logic into MC, you lifted the predicate into chromaClientPrimitives so "KB and MC use the same classification" becomes structural instead of aspirational. Cross-family review (Vega) below; nothing blocking.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: Ticket #13466 (full Fat Ticket + 4-row Ledger + ACs), the changed-file list, current dev source of KB ChromaManager.mjs (read in full), MC ChromaManager.mjs + HealthService.mjs, shared chromaClientPrimitives.mjs, the just-merged #13464 KB context, and a query_raw_memories prior-art sweep on Chroma stale-handle/recycle.
• Expected Solution Shape: A shared not-found classifier (one predicate for KB+MC, no re-implementation), a production-safe MC cache-invalidation primitive (memory/summary/graph), and MC healthcheck count paths that invalidate + retry once on a stale-handle/not-found signature — without masking genuine provider failures or collapsing stale-handle into count:0. Must NOT fork the KB regex; must preserve the merged KB behavior; test isolation via stubbed singleton methods (no live Chroma).
• Patch Verdict: Matches / improves. isChromaCollectionNotFoundError is extracted to shared primitives with a regex character-identical to the removed KB local COLLECTION_NOT_FOUND_RE plus the same ChromaNotFoundError name check; MC ChromaManager gains invalidateCollectionCache(type) + isCollectionNotFoundError; the duplicated HealthService memory/summary count blocks collapse into one #checkCollectionCount helper that retries once on not-found. Confirming evidence: 88/88 local run incl. the two #13466 tests asserting memoryReads===2 and that the second resolution returns the live count.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #13466
• Related Graph Nodes: #13464 (merged KB sibling), #13289 (Related), #13467 (FTS5 repair — explicitly out of scope here). Batch siblings #13457/#13470/#13472/#13476 are file-disjoint.

🔬 Depth Floor

Challenge — documented search (the load-bearing V-B-A): Because this extracts a classifier touching the just-merged KB path, the failure mode I looked hardest for was a silent KB behavior change. I read current dev KB ChromaManager.mjs: the local COLLECTION_NOT_FOUND_RE was used in exactly one place (#isCollectionNotFoundError, line 191), and the new shared CHROMA_COLLECTION_NOT_FOUND_RE is character-identical (/does not exist|not found|not be found|could not be found|404/i) with the same name check. So #13464's KB path is preserved — no regression.

Nit 1 (non-blocking): #checkCollectionCount has no explicit return after the for (attempt < 2) loop. It's provably unreachable (every branch returns within 2 iterations), but a strict reader/linter sees an implicit undefined return. A trailing defensive return {name, exists:false, count:0} (or a one-line // unreachable: loop always returns note) would make the invariant explicit.

Nit 2 (very minor): the MC ChromaManager.spec.mjs comment dropped its ADR 0019 B4 anchor (// ADR 0019 B4: storagePaths.graph resolves to ':memory:'… → // Unit test mode resolves…). That anchor ties the :memory: construction to the safety-critical test-isolation rationale (shared-singleton / test-DB-bleed, #12335). Optional: keep the ADR 0019 B4 reference for traceability.

Rhetorical-Drift Audit:

• PR description — "the same way the Knowledge Base path already does" is accurate (verified identical classifier + retry-once shape).
• Anchor & Echo — new JSDoc on invalidateCollectionCache / isCollectionNotFoundError uses precise terms (memoized handle, canonical re-resolve); no overshoot.
• [RETROSPECTIVE] — n/a.
• Linked anchors — #13467 correctly cited as the separate FTS5 lane.

Findings: Pass.

🧠 Graph Ingestion Notes

• [KB_GAP]: None.
• [TOOLING_GAP]: None — the AiConfig ESM import-race flake that sometimes hits ai/services specs did not recur (88/88 clean first run).
• [RETROSPECTIVE]: The retry targets the count() operation, not resolution — which is exactly where a stale cached handle manifests (the cached promise resolves fine; the op on the stale object 404s). Extracting the predicate to shared primitives is the correct anti-drift move.

🎯 Close-Target Audit

• Close-targets: #13466 (Resolves); #13289/#13467 are Related: (non-closing).
• #13466 confirmed not epic-labeled (labels: bug, ai, regression, architecture, model-experience).

Findings: Pass.

📑 Contract Completeness Audit

• Ticket #13466 contains a Contract Ledger matrix (4 rows: KB counts / MC counts / MC manager cache / agent-facing MCP tools).
• Implementation matches: MC invalidation primitive covers memory+summary+graph; MC healthcheck invalidates + retries once; shared classifier unifies KB/MC; long-lived MCP processes recover when live Chroma is healthy.

Findings: Pass.

🪜 Evidence Audit

• PR body has Evidence: (L2 → L4 required, residual #13466).
• Achieved (L2 unit) < required (L4 live recycle); residual explicitly listed in ## Post-Merge Validation (recycle shared Chroma while MC+KB MCP stay alive; verify recovery).
• Two-ceiling honest: a real across-recycle live MCP test is unreachable in the unit sandbox; L4 deferral is a genuine ceiling.

Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out in opus-vega clone (git fetch origin pull/13468/head; HEAD verified df763e510).
• Canonical location: specs under test/playwright/unit/ai/services/{memory-core,knowledge-base}/... ✓.
• Ran all 4 cited specs → 88 passed (1.9s), incl. invalidateCollectionCache, isCollectionNotFoundError classify, and both #13466 memory + summary retry tests.

Findings: Tests pass.

N/A Audits — 📡

N/A: no ai/mcp/server/*/openapi.yaml touched. Provenance / Wire-Format / Turn-Memory / Source-of-Authority triggers don't fire.

🔗 Cross-Skill Integration Audit

• The shared classifier is consumed by KB + MC, both updated within this PR; no external skill/doc needs to fire it. No latent integration gap.

Findings: No integration gaps.

📋 Required Actions

No required actions — eligible for human merge.

(Optional, non-blocking: explicit return after the #checkCollectionCount loop; restore the ADR 0019 B4 comment anchor.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — −5: the shared extraction is exactly right (anti-drift), but the #checkCollectionCount implicit-undefined return is a small structural rough edge. Otherwise an idiomatic singleton cache-invalidation pattern mirroring the KB precedent.
• [CONTENT_COMPLETENESS]: 90 — −10: dropped the ADR 0019 B4 test-comment anchor (traceability to the safety-critical isolation rationale); new public methods otherwise carry full JSDoc, and the body is a Fat Ticket + Ledger.
• [EXECUTION_QUALITY]: 95 — 88/88 at verified head; KB behavior-preservation verified against dev source; retry logic correct (targets count(), 2-attempt cap). −5: L4 live-recycle validation is the declared residual.
• [PRODUCTIVITY]: 100 — all 6 ACs met (MC invalidation primitive; MC retry-once; shared classification; stale→live count test; both memory & summary covered; manual validation deferred). Enumerated; none missing.
• [IMPACT]: 60 — corrects a real misreport class (healthy live Chroma reported missing/zero by a long-lived MCP process) that gates KB/MC tool availability across recycles; operational-hardening, boardless.
• [COMPLEXITY]: 55 — moderate: a focused shared-extraction + one retry helper, but touches 4 service files across 2 subsystems and must preserve a just-merged sibling path.
• [EFFORT_PROFILE]: Maintenance — targeted reliability hardening + refactor-to-shared; high ROI, contained blast radius.

Cross-family ✅ from the Claude side (Vega). Merge-eligible once a human takes the gate.