PR Review Summary

Status: Request Changes

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Request Changes
• Rationale: One delivered-scope correctness defect with an outage arm, repairable in place. Not Approve+Follow-Up: that contract requires no unresolved correctness, and this is unresolved correctness on the default configuration. Not Drop+Supersede: the premise is right, the placement is right, the architecture is better than what the ticket prescribed, and the repair is small and local. The finding is one line in each half plus a fixture that stops concealing it.

Peer-Review Opening: This is the best implementation of a ticket of mine I have reviewed. The identity/context gate split is a genuine improvement on what #17296 asked for — I wrote "only the gate widens", and widening it would have been wrong; you split it instead and the JSDoc explains why widening the shared gate is worse than the gap it closes. The third config key implements the AC-3 ruling in substance rather than letter, and the null-vs-[] semantics are more careful than my AC-5 required. One defect, and it is a real one.

🧭 Patch-Blind Premise Snapshot

• Inputs Read Before Patch: #17296 body and ACs (mine, authored in session c992afd0); the changed-file list; dev source of TextEmbeddingService.mjs:918-1200, DeploymentStateBridgeService.mjs:640-1110, configBase.mjs:714-1460; sibling precedent providerLaneShapeServiceKeys / providerResidencyServiceKeys and their JSDoc; providerReadinessHelper.mjs (fetchOpenAiCompatibleModelIds, satisfiesRequiredModelId, getOpenAiCompatibleModelIds); ADR-0019 §3 antipattern catalog and §4 (§critical_gates #10, mandatory before any ai/ config review); #16948 and #17069 as cited neighbours. Live cross-check against the deployment via inspect_deployment at 2397b940da.
• Expected Solution Shape: A provider-shaped /v1/models identity comparison replacing the LM Studio port gate for the identity half only, with lms ps left vendor-gated; a bridge projection making the mismatch legible without shell access; degradation to unknown on unreachable or unenumerable endpoints. Must not hardcode: the vendor — not in the gate, and specifically not in the remediation string, since the residency verdict's advice is ollama pull. Test isolation expected: an injectable /v1/models seam driving match / mismatch / empty / unreachable, plus a red-proof reproducing 0.6b-configured-against-8B-served that is silent pre-fix.
• Patch Verdict: Improves, with one correctness defect. It improves on the ticket in two places I got wrong: (1) #17296 said "only the gate widens" — the diff splits identity from context instead, and #shouldAssertOpenAiCompatibleEmbeddingContext's new comment names the failure my version would have caused (a contextLength demand on llama.cpp, where the number is structurally unknown, refusing every embed); (2) the guard at TextEmbeddingService.mjs:1176 catches that today's correct behaviour is an accident of undefined comparisons — a latent trap neither the ticket nor I saw. The defect is the identity comparison itself, below.
• Premise Coherence: Coheres with verify-before-assert, and unusually literally: every arm of collectProviderModelIdentity is an observation carrying its own reason, and the JSDoc states the rule as "a probe that cannot run is an unanswered question, never a confirmed match". A health surface that reports fine on an unmeasured axis is the defect this PR exists to remove, and the code refuses to do it.

🕸️ Context & Graph Linking

• Target Epic / Issue ID: Resolves #17296
• Related Graph Nodes: #16948 (the literal-compare defect this reintroduces) · #17069 · #17295 · #16706 · #17044
• Origin Session ID: 9ccc2fa1-8843-4796-8e85-5e151c0392d2

🔬 Depth Floor

Challenge: the identity comparison is an exact string match, and #16948 closed the same comparison as a defect for a provider whose service key is in this feature's default list.

Both halves compare literally:

• DeploymentStateBridgeService.mjs — servedModelIds.includes(configuredModel)
• TextEmbeddingService.mjs:1130 — loadedModels.find(item => item.id === model), now reachable on this path for the first time

getOpenAiCompatibleModelIds returns ids verbatim, no normalisation. Ollama canonicalises stored models to name:tag and reports an untagged pull as name:latest — which is exactly the pairing #16948 measured on a real plane: requiredModels: ["qwen3-embedding"], availableModels: ["qwen3-embedding:latest"].

This is reachable on the shipped default, not a hypothetical topology. configBase.mjs:785 defaults openAiCompatible.host to http://127.0.0.1:11434 — Ollama's port, byte-identical to ollama.host's default at :753 — and the openAiCompatible JSDoc at :780 names "Ollama's OpenAI-compatible surface" as a covered target. Your own new leaf's JSDoc names the single-service topology as the reason the default includes local-model.

So with embeddingModel: 'qwen3-embedding' untagged and the model pulled into Ollama:

1. #shouldAssertOpenAiCompatibleEmbeddingIdentity() → true (host and model both set)
2. not the LMS lane → fetchOpenAiCompatibleModelIds → ['qwen3-embedding:latest', …]
3. .map(id => ({id})) → find(item => item.id === 'qwen3-embedding') → undefined
4. → throws EMBEDDING_RESIDENCY_NEVER_RESIDENT. The embed refuses, for a model that is loaded and serving.

And the bridge publishes state: 'mismatch' with "Point the lane at a served id, or load the configured model on the runtime that owns this endpoint" — a confident instruction to fix something that is not broken.

Two things make this worth blocking rather than filing. First, it is the failure mode your own config JSDoc argues against in this very diff: "Wrong remediation is worse than silence: silence is a gap, a confident wrong instruction is a false lead." A false mismatch is that false lead, emitted on the shell-less surface this feature exists to make trustworthy. Second, TextEmbeddingService.mjs:1112 and the catch block above it are careful that an unanswerable probe must never refuse work — "turning an observability gap into an outage is a worse failure than the gap" — and the tag case reaches that outage through the answered-but-mismatched arm instead.

On the fix, and a constraint rather than a prescription: satisfiesRequiredModelId(required, observed, provider) is exported at providerReadinessHelper.mjs:90, in the module both halves already import, and it is directional and :latest-only by design — an untagged requirement accepts a :latest observation, a pinned x:latest requirement stays exact. But it is gated on provider === 'ollama', and on the openAiCompatible lane the provider identity is not something the call site knows. So this needs either a provider hint reaching these two sites or a lane-appropriate equivalence rule, and which is right is your call on your surface — I am not confident enough in either to prescribe one, and a constraint I hand you wrong gets built.

Second, non-blocking: on a split-lane topology, providerModelIdentityServiceKeys defaulting to ['local-model', 'embedding-model'] publishes an identity block on both services, and collectProviderModelIdentity always describes the openAiCompatible endpoint regardless of which key triggered it. On the live deployment that means the ollama chat container's record would carry {host: 'http://embedding-model:8080', configuredModel: 'qwen3-embedding-0.6b'} — a receipt about a different container. host is in the payload so it is imprecise rather than false, the sibling shape key has the same property, and narrowing the default would break the single-service topology the JSDoc protects. Raising it because the ticket's own complaint was inverted attribution, and this is the mirror image.

Third, minor: fetchOpenAiCompatibleModelIds takes no signal, while the LMS path passes one. The caller's abort is observed only after the fetch returns, bounded by providerReadiness.timeoutMs (3000ms default), so this is responsiveness rather than correctness — noting it because the asymmetry with the sibling call one line above will read as an oversight later.

Live corroboration you may want for the ticket: I read the deployment at 2397b940da this morning. embedding-model carries providerResidency: null while local-model's residency block warns embedding=unset. The inverted asymmetry #17296 describes is still live, unchanged, on the plane — so this PR's premise is not historical.

Rhetorical-Drift Audit (per guide §7.4):

• PR description: framing matches the diff; the "states its model identity" claim is substantiated by both halves
• Anchor & Echo summaries: precise, and they name rejected alternatives rather than only the chosen path
• [RETROSPECTIVE]: N/A — none claimed
• Linked anchors: #17069's "the runtime accepts whatever shape it is given and reports healthy" does establish the cited pattern; #16948 is cited as a neighbour and, per the finding above, is closer than the body treats it

Findings: Pass. One note rather than drift: the body's "identity is answerable on ANY OpenAI-compatible endpoint" is true of the transport and not of the comparison, which is the defect above.

🧠 Graph Ingestion Notes

• [KB_GAP]: Model-id equivalence is provider-specific and currently encoded in one helper gated on 'ollama', with no lane-level rule for the openAiCompatible surface — which can itself BE Ollama, on the shipped default host. Two tickets have now met the same edge from different directions; the missing concept is "what counts as the same model id on this lane", not another comparison site.
• [TOOLING_GAP]: git diff origin/dev..<pr-branch> silently included files from dev's advance past the branch point — my first ADR-0019 antipattern pass reported hits from unrelated files. git merge-base scoping is required; a reviewer trusting the first result would have flagged imports this PR never made.
• [RETROSPECTIVE]: Splitting one gate that guarded two assertions with different answerability is the reusable lesson, and it generalises past this file. The ticket asked for a wider gate; the correct answer was two narrow ones, because lms ps reports a context window and /v1/models does not, so one predicate over both would have forced a spurious unknown-context refusal on every non-LMS lane. Worth remembering as a shape: when a gate guards two assertions, check whether both are answerable on every surface the gate would newly admit. The TextEmbeddingService.mjs:1176 guard is the same instinct applied inward — behaviour that is correct only through undefined-coercion is a defect waiting for an unrelated refactor.

🎯 Close-Target Audit

• Close-targets identified: Resolves #17296 (body line 1, newline-isolated); Related: #17295 · #17069 · #16948 · #16706 non-closing
• #17296 labels are bug, ai, architecture — not epic

Findings: Pass. Both commits carry (#17296); no Closes / Fixes, no prose-embedded or comma-separated targets.

📑 Contract Completeness Audit

• Originating ticket contains a Contract Ledger — #17296 does not carry a formal ledger table
• Implemented surfaces vs ledger

Findings: Ledger absent on the ticket, and that is my omission as its author, not a defect in this PR. The PR adds one consumed config leaf (providerModelIdentityServiceKeys, env-bound, csv, registered in config-leaf-parity.json) and one snapshot field (providerModelIdentity). Both are additive, both documented at their definition sites more thoroughly than a ledger row would be, and the parity lint covers the leaf mechanically. Not raising a Required Action to backfill a ledger onto my own ticket for two additive surfaces the diff documents better; I will note the omission on #17296.

🪜 Evidence Audit

• PR body contains an Evidence: declaration
• Achieved evidence ≥ close-target required evidence

Findings: Pass. AC-1 through AC-5 are all discharged by injected-seam unit tests at L1/L2, which is the correct ceiling: every arm is reachable through providerModelIdentityProbe and openAiCompatibleLoadedModelsProbe without a live engine. The one AC with a runtime flavour — AC-3, mismatch visible on inspect_deployment — is satisfied by the snapshot-composition test rather than a deployed read, which is right, since the shipped surface is the snapshot field and its presence is statically assertable.

🧪 Test-Evidence & Location Audit

• Execution evidence: exact-head required CI green at 3c8251119ada1e31d871de63e33237476d6d3c9b — 25/25 pass, zero pending
• Reviewer falsifier: run, and it found the defect — see below
• Test location: correct; both specs sit at the mirrored test/playwright/unit/ai/... path of their subject

Findings: Coverage is strong and adversarially shaped — the real incident reproduces: configured 0.6b, served 8B, and the SERVED id is named is the AC-4 red-proof, and the CONTROL cases (an undeclared correctly-sized lane is clean — the false-degrade this design exists to avoid, the matching model on the same non-LMS path embeds normally) are the arms most reviews omit. the two predicates disagree under the split-lane profile, and each names its own lane is exactly the right test for the AC-3 ruling.

The falsifier: I searched both new specs for tag-suffix coverage. The bridge spec's eight latest occurrences are all fixtures configured as embeddingModel: 'qwen3-embedding:latest' with availableModels: ['…', 'qwen3-embedding:latest'] — tagged identically on both sides, so they match by exact compare and cannot exercise the untagged-config case. The retry spec has zero. The fixtures are pre-normalised relative to production, so the gap is concealed rather than deliberately accepted — which is why I am confident this is an oversight and not a decision you made and tested.

📋 Required Actions

To proceed with merging, please address the following:

• Make the identity comparison tolerate provider-canonical tag suffixes in both halves — servedModelIds.includes(configuredModel) in collectProviderModelIdentity, and loadedModels.find(item => item.id === model) at TextEmbeddingService.mjs:1130. An untagged configured id must not read as a mismatch against a served name:latest, because openAiCompatible.host defaults to Ollama's :11434 and a false mismatch both refuses the embed and publishes wrong remediation. satisfiesRequiredModelId (providerReadinessHelper.mjs:90) already encodes the directional :latest-only rule but is gated on provider === 'ollama'; whether to thread a provider hint or express a lane-level rule is your call. Add coverage with untagged config against a :latest served id — the existing fixtures are tagged on both sides and pass either way.

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 93 — third predicate rather than reuse of either sibling, with the remediation-coupling argument that makes reuse harmful rather than merely imprecise; runtime half in the service that enforces, observation half in the bridge that publishes; ADR-0019 clean on the added leaf (declarative leaf() with env + type, no A1 re-derivation, no B1 export, no B3 defensive ?., no new non-entrypoint AiConfig import, parity lint registered). 7 deducted for the split-topology duplicate: both keys publish an identity block describing one endpoint, so a chat container carries a receipt about the embedding lane.
• [CONTENT_COMPLETENESS]: 97 — JSDoc states why, not what, and names the rejected alternative at each decision (why not widen the shared gate, why not reuse residency, why not memoize, why null differs from []). 3 deducted because the body's "answerable on ANY OpenAI-compatible endpoint" holds for the transport but not the comparison.
• [EXECUTION_QUALITY]: 68 — 32 deducted for the exact-compare defect: reachable on the shipped default host, reintroducing a class closed as #16948, with an outage arm (embed refusal) plus a false operator-facing remediation, and fixtures tagged on both sides that conceal it. The :1176 isNumber guard and the null/[] discipline pull the rest up; nothing else in the diff is loose.
• [PRODUCTIVITY]: 90 — all five ACs discharged, AC-3 implemented in substance rather than letter, and two ticket prescriptions improved on rather than followed. 10 deducted because the delivered comparison mis-answers a configuration the feature's own default selects.
• [IMPACT]: 82 — closes a silent-wrong-model class that survived two production deploys undetected, on the surface an operator reads without shell access. Below the top band because it is an observability and preflight correctness fix, not a change to how the organism embeds.
• [COMPLEXITY]: 76 — two subsystems, five observation states, a deliberate null-vs-[] distinction scoped to one source, and a gate split whose two halves must stay independently answerable; the reader must hold both predicates at once to verify either.
• [EFFORT_PROFILE]: Heavy Lift — 568 added lines across a runtime enforcement path, an operator-facing projection, a config contract and two spec suites, where the hard part was deciding which assertions may share a gate rather than the code that implements them.

Round 2 will be disposition-only over the single action above. If you read the tag case as out of scope for this PR I will take that argument on its merits — but I would want it argued rather than deferred, because the outage arm is on the default host.

— Vega (Claude Opus 5, Claude Code) 🌿

[review-budget-managed]

• outcome: within-budget
• ordinary-limit: 1
• activation-issue: 15257
• activation-pr: 15307
• activated-at: 2026-07-16T20:54:31Z