PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Correct, well-tested throttle with exactly the right safety semantics — caches only healthy canaries (so failures retry immediately, never hidden behind the TTL), keys on provider+dimension so a route change invalidates, and integrates with clearCache(). Complements #12537 (the other half of the interactive-embed contention mitigation). One non-blocking dead-code observation on the cache key.

Peer-Review Opening: This is the demand-side complement to #12537's supply-side queue — together they stop healthcheck polling from competing with interactive add_memory embeds. The healthy-only caching is the key correctness call and it's right: a degraded canary clears the cache and reprobes next poll, so the 60s TTL never masks a real write-path failure. Approving; one cache-key cleanup note below.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #12508 (5 ACs; labels enhancement/ai/performance/model-experience, no epic → leaf), the diff, the canary call-path (#applyEmbeddingWriteCanary → buildEmbeddingWriteCanaryBlock), the root + memory-core config leaf resolution, and the parent-chain inheritance the memory-core config relies on.
• Expected Solution Shape: AC-driven — measure/decide caching is warranted (AC1), add a bounded TTL so within-TTL polls reuse (AC2/AC4), and still report degraded/unhealthy when the embedder genuinely fails outside the success window (AC5). The dangerous failure mode would be caching a failed canary (hiding an outage); the correct design caches healthy only.
• Patch Verdict: Matches all ACs:
  • AC2/AC4 — 60s #embeddingWriteCanaryCache; the direct healthcheck reuses a recent healthy embedding write canary test proves embedCalls stays 1 across two within-TTL polls while collection counts still refresh (11) — i.e. the narrow canary cache doesn't block the broader observability refresh, which is the whole point.
  • AC5 — healthy-only caching + the expired … probes again and degrades on failure test (61s expiry → reprobe throws → degraded, not cached).
  • AC1 — satisfied via deterministic structural rationale (each direct poll re-runs one live probe without the cache) rather than runtime instrumentation; reasonable for a per-poll-deterministic relationship (see minor note).
  • The "## Deltas from ticket — private cache, no config leaf" is explicitly declared + justified (avoids a new operator surface for a 60s probe throttle) — same correct ledger-disposition discipline as #12537.

🕸️ Context & Graph Linking

• Target Issue ID: Resolves #12508
• Related Graph Nodes: Parent #12487, V1 PR #12502, sibling #12537 (#12509 QoS — the interactive-priority half), epic #12456 (AiConfig SSOT)

🔬 Depth Floor

Observation (non-blocking — dead-code cache-key fallback): the new cache key is ${aiConfig.embeddingProvider || 'openAiCompatible'}:${aiConfig.vectorDimension ?? ''}. The || 'openAiCompatible' and ?? '' are the ||/?? config-fallback pattern epic #12456 is actively removing and the operator has flagged before. I verified they're dead code, not a config-gap mask: the memory-core config loads the Tier-1 realm root for getParent() inheritance (ai/mcp/server/memory-core/config.template.mjs:1), the root leaves carry concrete defaults (embeddingProvider: leaf('openAiCompatible', …) @ai/config.template.mjs:155, vectorDimension: leaf(4096, …) @:276), and sibling sites already read aiConfig.embeddingProvider verbatim (HealthService.mjs:1179, ChromaManager.mjs:200) — so the leaves always resolve concretely and the fallbacks never fire. The new lines mirror the pre-existing :219 idiom (cfg.embeddingProvider || 'openAiCompatible', cfg.vectorDimension ?? null), so this isn't a novel regression.

• Recommendation (non-blocking): read the two leaves verbatim in the new cache key, and — cleaner — fold a small follow-up under #12456 to retire the whole HealthService embedding-provider fallback cluster (:192, :219, + this cache key) together, since the parent chain makes them all dead. I'm explicitly not asking you to fix the pre-existing sites in this PR; just flagging not to grow the cluster, and that the SSOT defaults already live in the templates.

Minor (non-blocking): AC1 literally asks to "measure or instrument … enough to decide." The body leans on the JSDoc structural rationale rather than a measurement; that's defensible (the per-poll → one-probe relationship is deterministic, not statistical), but one body sentence stating "no instrumentation needed — each direct poll deterministically issues one probe" would close AC1 explicitly.

Rhetorical-Drift Audit: Pass. "Throttle healthcheck embedding canary" matches the diff — a bounded healthy-result cache, nothing broader.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The right invariant for a health-probe cache is "cache success, never cache failure" — it collapses healthy-poll bursts without ever extending an outage's blind window. This PR gets it exactly right and proves it with the degrade-on-expiry test.
• [KB_GAP]: The HealthService embedding-provider fallback cluster (:192/:219/new cache key) is dead under the parent-chain getParent() inheritance — a clean, bounded #12456 cleanup target.

N/A Audits — 📡 🔗 📑

N/A: no OpenAPI/skill/MCP-tool surface; contract surface is the Contract Ledger (the no-config-leaf row is explicitly disposed of via the "## Deltas from ticket" section).

🧪 Test-Execution & Location Audit

• Checked out exact head 2bfdda604 and ran HealthService.spec.mjs → 55 passed (2.2s), including both new canary-cache tests (within-TTL reuse + expired-probe-degrades). CI all-green + MERGEABLE. Tests sit in the existing #12382 cached healthcheck freshness describe — correct co-location with the broader-cache coverage.

Findings: Tests pass and are genuinely discriminating — they prove both the throttle (embedCalls bounded) and the safety floor (failures degrade + aren't cached), not just the happy path.

📋 Required Actions

No required actions — eligible for human merge.

Recommended (non-blocking): read the cache-key leaves verbatim (drop the dead || 'openAiCompatible' / ?? ''), ideally via a small #12456 follow-up retiring the whole HealthService fallback cluster; optionally one AC1 sentence in the body.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 95 — 5 deducted: textbook health-probe throttle (cache-success-only) and correct ledger disposition, minus the dead-code ||/?? cache-key fallbacks that cut against epic #12456's direction (even though they mirror existing in-file style and are provably inert).
• [CONTENT_COMPLETENESS]: 90 — 10 deducted: excellent JSDoc + Deltas section; AC1's "measure to decide" is satisfied by structural rather than instrumented rationale and could be stated explicitly in the body.
• [EXECUTION_QUALITY]: 100 — healthy-only caching, key-invalidation, clearCache integration all correct; spec green at head and proves the safety floor.
• [PRODUCTIVITY]: 100 — resolves all #12508 ACs in one bounded HealthService change (+124/-3), no scope bleed into #12509/#12537.
• [IMPACT]: 65 — removes per-poll live-embed pressure on the interactive path during diagnostics; pairs with #12537 to actually protect add_memory latency under polling + KB-sync load.
• [COMPLEXITY]: 40 — descriptive: low-moderate; small cache with a subtle-but-correct healthy-only invariant; the reviewer load was verifying the cache-key fallbacks are dead (parent-chain resolution) rather than masking a gap.
• [EFFORT_PROFILE]: Quick Win — bounded, high-confidence throttle with the right safety semantics.

Approving — correct and well-tested. The cache-key ||/?? is a dead-code cleanup for #12456, not a gate.