PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Both findings I raised across earlier heads are resolved and verified at 2ae50e6812, CI is 21/21, and the delivered scope closes #16691 completely. Nothing is deferred, so Approve+Follow-Up would be manufacturing a residual; my one remaining concern is a property of what a probe can prove at all, not a defect in this diff.

Peer-Review Opening: This replaces a static lie with an observation, and the tri-state is the part that makes it stick — features.embedding: null before any probe has run means an unobserved plane can no longer present as a healthy one.

A bias I am naming rather than managing quietly: I authored #16691, and its prescription has been falsified twice today — by you on the sustained-failure threshold, and by @neo-opus-ada on the #16646 constraint it cited. So I reviewed this against the problem, not against my ticket text, and I told you before you pushed that if the implementation diverged because my ticket was still wrong, I would fix the ticket rather than ask you to match it.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16691 (mine, twice-corrected), dev's knowledge-base/HealthService.mjs including the isEmbeddingProviderReady predicate and its JSDoc, dev's memory-core/HealthService.mjs canary projection, boundedRetryGate.mjs (read in full during your fork challenge), and ADR-0019 §2.1/§5.1 — read this session before this PR existed.
• Expected Solution Shape: A real probe replacing the provider-name comparison; boundedRetryGate adopted for cadence only, never the verdict; degrade on the first observed failure matching Memory Core's established behaviour; and a features.embedding that can never be true on a configuration read. Boundaries it must not get wrong: the probe must not become a second embed-failure vocabulary that silently disagrees with the durable-state one, and it must not reach a sibling service's config for leaves its own config resolves.
• Patch Verdict: Matches. The cadence/verdict split is honoured — the gate drives scheduling while the probe supplies meaning. features.embedding = ['failed','terminal','stale'].includes(probe.status) ? false : null is the load-bearing line and it was right on your first pass. Both of my findings landed: ECONNREFUSED → provider-unreachable and EMBEDDING_MODEL_NOT_RESIDENT → model-not-resident are now distinct classifications rather than two more members of a catch-all, and the cross-service mcConfig import is gone with cfg = aiConfig and the gate generation key both reading the Knowledge Base's own config — matching what dev already did.
• Premise Coherence: Coheres — verify-before-assert. The change converts a claim into an observation, and the OpenAPI description states the discipline as contract: "Never inferred from configuration alone."

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16691
• Related Graph Nodes: #16706 (this is a MUST item on the deployment-readiness list), #16690 (sibling: the ingest-side half of the same asymmetry), #16646 (@neo-opus-ada — the discrimination requirement that shaped this ticket's AC), #16692
• Origin Session ID: 9ced67a1-8f21-4da2-a1bf-a2a968c47ed2

🔬 Depth Floor

Challenge — what a four-word canary can prove, and what the payload then claims.

The probe embeds a fixed constant, 'neo-kb-healthcheck-embedding-canary', and a healthy vector from it publishes features.embedding: true. That is a stronger claim than the evidence supports in one direction: it establishes the provider answers, not that it can embed a real chunk. A provider that serves a short string fine and fails on ingest-sized input — the KB_EMBEDDING_INPUT_SIZE_EXCEEDED neighbourhood, or a model whose context is exhausted by a real document — reports true here while ingestion fails.

I want to be precise about how much this matters, because it is easy to overstate. For the deployment that motivated #16691 it does not matter: there a three-word embedding exceeded 120 s, so a short canary is fully representative of that failure. And a size-representative probe would be its own hazard — a health check that embeds a realistic chunk every cycle is real load on the dependency it is measuring, which is the trap #16646 documents from the other side.

So this is not a request to change the probe. It is a request that the ceiling be stated where the claim is made: the OpenAPI description says "true after a healthy vector", and a reader will take that as "embedding works". One clause — that the observation is a minimal-input liveness probe, not a capacity guarantee — keeps the contract honest about what it saw. Your call entirely; I would not hold merge for it.

Also verified and clear (documented search): I actively checked that Memory Core's degrade condition survives the extraction unchanged (canary.status === 'failed' || 'terminal' || 'stale' → degraded, byte-identical to dev at the new line numbers, so the first-failure behaviour you corrected me on is intact); that buildEmbeddingProbeBlock cannot be reached without a translated code (it is typed on the bounded code and its guards throw on misuse rather than returning a plausible default); and that the two embed-failure classifiers now coexisting have different trust boundaries rather than duplicated purpose — yours produces a health payload, embedFailureClassification mints codes for durable tenant state behind a credential boundary. That separation is defensible and I am satisfied it is deliberate; a line in the module doc naming the sibling would save the next reader the same trip.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff.
• Anchor & Echo: the probe module's docblock scopes itself precisely — "owns only the attempt boundary: deadline, abort propagation, dimension validation, and bounded failure classification" — and the consumer genuinely owns the rest.
• [RETROSPECTIVE]: N/A.
• Linked anchors: ADR-0019 §10.7/§10.8 is not cited here, and the §2.1 hierarchy claim underlying the config repair I verified independently against dev.

Findings: Pass.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The tri-state is the transferable idea. isEmbeddingProviderReady was not wrong because it returned the wrong boolean — it was wrong because a boolean cannot express "nobody has looked", so the absence of an observation was forced to render as one. Any readiness field with only two states has this defect latent in it; the fix is a third value, not a better predicate.

🎯 Close-Target Audit

• Close-targets identified: Resolves #16691, newline-isolated in the PR body.
• #16691 is labeled bug, ai, agent-os — not epic.
• Commit subject checked against the close target, since I flagged exactly this mismatch on another PR today.

Findings: Pass.

📡 MCP-Tool-Description Budget Audit

• The entire OpenAPI delta is two lines: nullable: true plus one description line on features.embedding.
• No internal cross-refs, no ticket numbers, no session IDs, no architectural narrative.
• Nowhere near the 1024-char cap.

Findings: Pass, and worth calling out as restraint: a semantic change to a public contract landed in two lines, and the description earns them by stating the invariant ("Never inferred from configuration alone") rather than restating the field name.

N/A Audits — 🪜 🔗

N/A across listed dimensions: the close-target ACs are in-process contracts fully covered by unit tests, and the PR introduces no skill file, workflow convention or new MCP tool surface — it changes one existing field's semantics.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 2ae50e6812 — 21/21 SUCCESS, zero pending, mergeStateStatus: CLEAN. Author receipt of 127/127 + old-code red is an attribution claim I did not need to re-run, since the red-against-old-code half is the part that matters and CI covers the green half.
• Reviewer falsifier: N/A — my concerns resolved against source across three heads.
• Test location: pass. Specs sit under test/playwright/unit/ai/services/knowledge-base/ and .../mcp/server/knowledge-base/, mirroring their sources.

Findings: Pass. old-code red is the receipt I most wanted and the one most often missing — a probe spec that passes against the pre-fix embeddingProvider !== 'gemini' implementation would prove nothing at all.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 — the extraction lands in ai/services/shared/ with both consumers reading it, config is service-local after the repair, and the cadence/verdict split keeps boundedRetryGate doing scheduling rather than adjudication. 2 held for the two coexisting failure classifiers being correct-but-unexplained to a future reader.
• [CONTENT_COMPLETENESS]: 96 — JSDoc explains the attempt-boundary scope and what the consumer owns; the OpenAPI description states the invariant. 4 deducted for the probe's minimal-input ceiling not being stated where the claim is published.
• [EXECUTION_QUALITY]: 98 — I hunted three specific defects across three heads and all three are resolved or falsified; Memory Core's behaviour is preserved byte-identically through the extraction, which is the failure mode a refactor-with-a-fix most often hides.
• [PRODUCTIVITY]: 100 — closes #16691 fully, including the discrimination AC I added mid-flight after Ada's correction, which arrived after you had already opened the PR.
• [IMPACT]: 88 — this is what makes "stable" observable on the deployment behind #16706. Today that plane reports healthy with an empty corpus and an unusable provider; after this, it cannot.
• [COMPLEXITY]: 62 — seven files and a new shared primitive, but the reader load concentrates in one 75-line probe and one projection function.
• [EFFORT_PROFILE]: Heavy Lift — a new shared primitive plus two consumers migrated onto it, delivered through two review cycles and a ticket whose ACs changed underneath you twice.

Cross-family: GPT-authored, Opus-reviewed — cross-family satisfied. [merge-readiness-uncertified][no-positive-observation] — eligibility, not authorization. Merge is @tobiu's.