fix(model-resolver): preserve @provider:model picks across cold catalogs - #3950
starship-s wants to merge 4 commits into
Conversation
… default on cold catalog The @Provider:model branch of _resolve_compatible_session_model_state() falls through to `return default_model` whenever the hinted provider is absent from the catalog snapshot it was handed. For a non-first-party provider (ollama-cloud, deepseek, xai, … — these normalize to "" and discover their models live), the cached/minimal catalog used on the hot GET /api/session path frequently lacks the group even though the provider is configured. The result: an explicitly-picked model like `@ollama-cloud:minimax-m3` silently snaps back to the global default on the 2nd+ turn and on chat switch. This was latent since v0.50.224 but became live on 2026-06-08 (v0.51.340, 26e133e), which switched the GET /api/session display resolver to prefer_cached_catalog=True for performance — exposing the blind spot. Fix: before the default-revert, preserve the selection when either (a) it was an explicit user pick (mirrors the bare-model branch's nesquena#3737 guard), or (b) the provider hint is non-first-party (provider_normalized == "") AND the bare id is not a first-party family name (gpt/claude/ gemini). This matches the slash-qualified branch, which already passes models whose provider normalizes to "" through unchanged. A genuinely stale misrouted first-party model (e.g. @copilot:claude-opus-4.6 after switching to openai-codex) still falls through to the default-repair. No change to catalog loading — the prefer_cached_catalog performance win is fully retained; only the fallback decision is corrected. Adds regression coverage in tests/test_provider_mismatch.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
| Filename | Overview |
|---|---|
| api/config.py | New _provider_is_known_or_configured() helper is well-scoped: checks static registry + config state only, with clear rationale for excluding credential/auth evidence. |
| api/routes.py | Two targeted additions to _resolve_compatible_session_model_state: explicit-pick early return and non-explicit preservation block, both with correct ordering and documented limitations. |
| tests/test_provider_mismatch.py | Six new tests cover the full matrix of explicit vs. non-explicit, known vs. removed provider, family-name collision, and the documented prefix-heuristic limitation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["@provider:model input"] --> B{provider_raw or bare_model empty?}
B -- yes --> C[return model unchanged]
B -- no --> D{explicit_model_pick?}
D -- yes --> E["NEW: return model, provider_raw, False"]
D -- no --> F{hint matches active provider?}
F -- yes --> G[return model, provider_raw, False]
F -- no --> H{catalog has provider?}
H -- yes --> I[return model, provider_raw, False]
H -- no --> J{bare model matches active family?}
J -- yes --> K[strip prefix and reroute to active]
J -- no --> L{"NEW: not provider_normalized AND not first-party name AND provider known/configured?"}
L -- yes --> M["NEW: return model, provider_raw, False"]
L -- no --> N{default_model available?}
N -- yes --> O[revert to default]
N -- no --> P[return model, provider_raw, False]
Reviews (4): Last reviewed commit: "review: honor explicit @provider picks a..." | Re-trigger Greptile
Addresses PR review (PR nesquena#3950): - Rewrote the @Provider:model guard comment in _resolve_compatible_session_model_state to be self-contained and to call out the KNOWN LIMITATION of the bare-name prefix heuristic: a third-party model whose name starts with gpt/claude/gemini (e.g. "@ollama:gpt4all-mini") is still mis-classified as first-party and reverted on non-explicit paths. A name-only check can't disambiguate this; only configured-provider-aware resolution could. (No heuristic swap: the static-catalog helper is stale — it lacks current models like claude-opus-4.8 — so it would trade this false-positive for worse false-negatives on genuine first-party models.) - Added test_at_provider_first_party_named_third_party_model_known_limitation to pin that boundary so the limitation is tracked, not silent (and to show an explicit pick still escapes the heuristic). - The explicit-pick test now also asserts the returned provider ("copilot"), confirming the @-qualified hint is preserved rather than rewritten. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the careful review — all fair. Pushed f6f1c50 addressing points 1–3; declining the heuristic swap, with rationale below. The Why I didn't switch to the precise helper. I evaluated
So swapping would fix What's in f6f1c50:
No CHANGELOG entry included — leaving that to the release stamping process, but glad to add one if preferred. |
|
Thanks @starship-s — the concept is right (explicit CORE — must fix: a genuinely-removed provider now routes to an unconfigured providerThe non-explicit guard preserves the selection for any provider whose group is absent from the catalog snapshot: if explicit_model_pick or (not provider_normalized and not _bare_is_first_party_family):
return model, provider_raw, FalseBut catalog-absence covers two distinct cases, and only one of them should be preserved:
I verified this empirically against the staged branch: Suggested fix: split the guard so Please also add a regression test pinning Already handled on my side (no action needed from you)
Reopen for re-gate whenever the configured-provider guard is in — happy to take it straight to release at that point. 🙏 |
…nown/configured providers Addresses the CORE review finding on PR nesquena#3950: the non-explicit guard preserved an explicitly-qualified selection for ANY provider absent from the catalog snapshot, including a genuinely removed/unconfigured one — so a stale session pointing at e.g. "@removed:mistral-large" would route chat/start to a provider that no longer exists instead of repairing to the default. Split the guard: * explicit_model_pick is always honored on its own (a fresh, deliberate pick). * the non-explicit cold-catalog preservation now also requires the provider to be known or configured, via the new config-state helper _provider_is_known_or_configured() — decided from the static provider registry + custom_providers config, NOT from the cold catalog snapshot (re-deriving that live would defeat the prefer_cached_catalog hot-path win). So a cold live-discovery provider (ollama-cloud configured, group missing from this snapshot) is still preserved, while a genuinely removed/unknown provider falls through to the default-repair. Adds test_at_provider_removed_provider_still_reverts_to_default pinning both the non-explicit revert and the explicit-pick escape hatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed the CORE issue — pushed 86d8157. You're right: catalog-absence conflated "cold but configured" with "genuinely removed," and the guard preserved both. Split the guard exactly as suggested: # explicit pick is always honored on its own
if explicit_model_pick:
return model, provider_raw, False
# non-explicit preservation now also requires the provider to be known/configured
if (not provider_normalized
and not _bare_is_first_party_family
and _provider_is_known_or_configured(provider_raw)):
return model, provider_raw, False
# else → fall through to default-repairNew helper Verified against your empirical case: Added I left the first-party-family |
|
Thanks @starship-s — the guard split is exactly right and the new I staged this for release alongside another fix and ran the full gate. Both reviewers cleared the prior CORE finding; one residual edge came up that I'd like tightened before it ships (the other reviewer judged it acceptable-as-designed, so I'm flagging it as a should-fix, not a hard block — your call on the approach): Should-fix: a built-in provider whose credentials were removed is still preserved
The docstring deliberately chooses "known builtin → preserve so the user gets a clear runtime error rather than a silent model swap," and that's a defensible position (one reviewer accepted it as-designed). But the stricter reading is that catalog-absent preservation should require actual configured/authenticated provider evidence (an API key in Two acceptable resolutions (your pick):
I lean toward (1) (revert is the safer default for a provider that demonstrably can't run), but (2) is legitimate if you'd rather surface a clear error. Minor (non-blocking, pre-existing): explicit-pick guard sits below the family-repair branchThe new comment says explicit picks are "never second-guessed," but the Reopen for re-gate whenever the config-evidence tightening is in — this is close. 🙏 |
…erate known-builtin preservation Two follow-ups from PR nesquena#3950 review: MINOR (branch order): the `if explicit_model_pick: return` guard sat *below* the _model_matches_active_provider_family repair, so an explicit pick like "@ollama-cloud:gpt-oss-120b" under an OpenAI-active agent was stripped to bare "gpt-oss-120b" and rerouted to OpenAI (the family match fires on the "gpt" prefix). Moved the explicit-pick guard to the top of the @Provider:model branch — a fresh, deliberate pick is by definition not a stale artifact and must never be rerouted. Adds test_at_provider_explicit_pick_not_rerouted_by_family_match. SHOULD-FIX (known-but-unconfigured builtin, Option 2 — document + pin): the reviewer noted a known builtin (deepseek/minimax/ollama-cloud) is preserved on a cold catalog even when no key is configured for it. We keep this behavior on purpose: the only fully-reliable "is this provider authenticated" signal is the live auth store / catalog rebuild — exactly the cost this hot path avoids — and a cheap env/config-only credential check would mis-classify OAuth/auth-store providers (ollama-cloud among them) and re-introduce the original silent-revert bug. A known-but-unconfigured pick is kept and surfaces a clear run-time auth error rather than a silent swap. Documented the deliberate scope in the helper docstring + guard comment, and pinned it with test_at_provider_known_unconfigured_builtin_is_intentionally_preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Confirmed both findings empirically and pushed ca58486. Minor (explicit pick below family-repair) — fixedReproduced exactly: Moved Should-fix (known-but-unconfigured builtin) — went with Option 2 (document + pin)Confirmed:
So a known-but-unconfigured pick is kept on purpose; the user gets a clear run-time auth error rather than a silent swap to the default (the lesser evil, and consistent with respecting an explicit pick). Documented the deliberate scope in the If you'd rather take the stricter revert-when-unconfigured semantics despite that risk, I'm happy to do Option 1 + auth-store check (consult the auth store for OAuth providers so |
|
Absorbed and shipped in v0.51.354 (Release LR, deployed live) via the batched release #3951 — rebased onto fresh master with attribution, full-suite (8588) + Codex + Opus gated, both SAFE. Across three review rounds you nailed it: the guard split + Thanks @starship-s! 🙏 |
Release LR — v0.51.354 (nesquena#3950 preserve @Provider:model picks across cold catalogs)
Thinking Path
@ollama-cloud:minimax-m3was instead snapping back to the global default when the current catalog snapshot did not include theollama-cloudgroup.@provider:modelselections.GET /api/sessionand related side-effect-free paths.@provider:modelselections when the user chose them, or when the provider is a non-first-party hint and the bare model is not a first-party family model.What Changed
_resolve_compatible_session_model_state()so the@provider:modelbranch no longer defaults away an explicit user pick just because the provider is absent from the current catalog snapshot.@ollama-cloud:minimax-m3, when the bare model does not look like a first-party family model (gpt*,claude*,gemini*).@provider:modelsurviving a cold/partial catalog on explicit and non-explicit paths;Why It Matters
Verification
Targeted resolver regression coverage:
Adjacent model/session resolver coverage:
Hygiene checks:
Risks / Follow-ups
@provider:modelsession can now remain selected and fail at runtime instead of being silently replaced by the default. That is intentional here: the cached catalog cannot reliably distinguish “provider removed” from “provider configured but not present in this snapshot,” and preserving the explicit selection is less surprising than silently changing it.Model Used