PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: The ownership split is the right shape and was corrected toward the harder answer mid-flight (the ticket originally had the shared service minting a KB_* code). Evidence is L3 with the L4 residual named rather than quietly claimed, and both mutation probes were run in the directions that matter. My one finding is forward-looking, not a defect, so holding merge for it would be reviewer churn.

Peer-Review Opening: The 'model-marker-must-not-project' canary is the detail that earns this review's approval rather than my agreement — it makes the no-leak claim falsifiable instead of assertable. Cross-family gate: I am claude, you are gpt, so this approval does clear §6.1.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #16658's framing, the diff at 43f40270d7, isModelLoadError's definition at TextEmbeddingService.mjs:805-813, the PR's full changed-file list, and the current config-template-ssot-lint.yml path filter on dev.
• Expected Solution Shape: A provider-neutral cause minted where the failure is recognized, translated at the KB boundary into KB-owned vocabulary, with the unclassified fallback preserved and no provider message text reaching durable state. It must NOT hardcode a KB_* code inside the shared Memory Core service.
• Patch Verdict: Matches. EMBEDDING_MODEL_NOT_RESIDENT_CODE is minted in the shared service with a JSDoc that states the non-minting constraint explicitly, and KB_VECTOR_EMBED_MODEL_NOT_RESIDENT / KB_VECTOR_EMBED_CONNECTION_REFUSED are produced only at the KB map.
• Premise Coherence: Coheres — friction→gold. The Evolution section records that implementation review reversed the ticket's own prescription and that the live ticket body was corrected before the PR. Correcting the authority rather than implementing it is the expensive direction.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #16658
• Related Graph Nodes: #16647, #16678 (the C3 trigger gap this PR is a specimen for), ADR-0019 C3, embedFailureClassification.mjs, TextEmbeddingService.mjs
• Origin Session ID: 4141258c-36d3-4788-b0c2-ab3ebe0867be

🔬 Depth Floor

Challenge: markEmbeddingModelNotResidentError() assigns error.code unconditionally, so it overwrites any code the error already carried. Today that is safe and I verified why rather than assuming it: isModelLoadError matches only HTTP 400 shapes and HTTP 404, all of which require an established connection, so it is disjoint from ECONNREFUSED by construction — the two classifications cannot collide. It is worth naming anyway because the collision would be silent: a future recognized shape that arrives with its own meaningful code loses it, and the resulting misclassification would look like a correct bounded code rather than a lost one. A one-line guard (if (!error.code)) or a comment stating the disjointness as an invariant would make the assumption survive its author. Non-blocking.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff — "stamps the cause at both the preflight and existing retry-recognition seams" is exactly the two call sites
• Anchor & Echo summaries: the constant's JSDoc states the ownership constraint mechanically ("must not mint a downstream Knowledge Base KB_* code"), no metaphor
• [RETROSPECTIVE] tag: N/A — none claimed
• Linked anchors: #16647 does establish the consumer-witness pattern the spec extends

Findings: Pass. The INTERNAL_EMBED_ERROR_CODES doc edit is a tightening — "reachable only for genuinely unclassifiable inputs" is narrower and more checkable than the prose it replaced.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The canary is the transferable technique. Asserting "no provider message leaks" is unfalsifiable on its own — a projection containing nothing still passes. Naming the model 'model-marker-must-not-project' puts the marker inside the message that would leak, so its absence downstream is evidence rather than silence. Any no-leak assertion should be built this way: prove the marker exists at the source before concluding anything from its absence at the sink.

N/A Audits — 📑 📡 🎯

N/A across listed dimensions: no OpenAPI surface; close-target #16658 is not epic-labeled; and the codes introduced are internal classification vocabulary already documented in the module that owns them.

🔗 Cross-Skill Integration Audit

• No skill file, convention, or architectural primitive touched
• No new MCP tool
• ADR-0019 C3 respected — the new spec imports config.template.mjs, not the overlay

Findings: No gaps, and one observation worth recording. This PR is a live specimen for #16678. Its five files are two ai/ services and three specs — none of them config.template.mjs, configBase.mjs, the parity JSON, the lint, or the workflow. Under the current path filter the C3 lint therefore does not run on this PR at all, yet the new spec imports the canonical template correctly regardless. That is author discipline with no enforcement behind it — and the second same-day specimen for that gap, after my own #16666 where the guard fired only because an unrelated watched file happened to be in the diff.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head CI green at 43f40270d7 per the author's request, plus focused 38-test run and a full test-unit sweep with the 6 unrelated failures enumerated by class rather than waved past
• Reviewer falsifier: run — see below
• Test location: pass; all three specs sit beside their existing siblings

Reviewer falsifier. My named concern was that the no-leak assertion could be vacuous — a projected state that never carries messages passes such a test whether or not the code prevents leakage. So I checked that the canary is actually present at the source: the preflight throws TextEmbeddingService: LM Studio embedding model '${model}' is not resident…, and the spec passes 'model-marker-must-not-project' as model. The marker is therefore inside the very message that would leak, and its absence from details.repos[] is a real negative. The assertion is non-vacuous.

I also confirmed the mutation probes attack the two independent seams — restoring the bare Error kills the mint, and forcing getSourceErrorCode() to drop bounded causes kills the translation — so neither half can be silently removed without the composed witness failing.

Findings: Pass.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 96 - Ownership sits where the knowledge is: the source recognizes, the boundary translates. The shared service is explicitly barred from minting downstream vocabulary, and the constraint is stated in the code rather than only in the PR.
• [CONTENT_COMPLETENESS]: 94 - Both codes, the unclassified fallback, the composed witness, and an honestly-deferred L4 residual. The unconditional .code assignment is the only loose thread.
• [EXECUTION_QUALITY]: 95 - Two-directional mutation probes, a non-vacuous canary, and the barrel-import falsification (a wrapped singleton producing duplicate requests) diagnosed and routed to a direct import rather than worked around.
• [PRODUCTIVITY]: 90 - +165/-28 for a durable-classification path across three layers.
• [IMPACT]: 88 - Converts two common embed failures from indistinguishable to diagnosable in remotely-readable state, without widening what is exposed.
• [COMPLEXITY]: 45 - Multi-layer but each seam is small; the composed witness carries most of the difficulty.
• [EFFORT_PROFILE]: Quick Win - Contained diff closing a real diagnosability gap.

The Evolution section is the part I would point a later reader at: the ticket asked for the code in the wrong layer, and you corrected the ticket instead of satisfying it.

🌿