From 8e143e7db38672cb1d073e813ae8b7e91f54705f Mon Sep 17 00:00:00 2001 From: Chris Murphy Date: Fri, 28 Aug 2026 09:26:03 +0800 Subject: [PATCH] Resolve /model to the alias's pinned model (#825) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit /model low reported success and renamed the session model to the alias string itself — an id no endpoint serves — because resolve_model_switch fell through to Keep for any unclassifiable id. Aliases that pin a model now resolve to that alias and its pinned model, via a new ModelSwitch::SwitchAlias variant that carries the model (Switch alone maps to a route whose model is the raw argument, which would have switched the client but kept the alias string as the model). Ranked below the exact-pin and gateway rules so a same-named model pin still wins, and only for aliases that PIN a model — everything else, including unknown non-alias ids, keeps today's permissive behaviour. The active provider's own alias flows through the route layer's already-live guard: no rebuild, no spurious switch note. cmd_model now echoes the model the session landed on rather than the raw argument. --- src/provider/resolve.rs | 163 +++++++++++++++++++++++++++++++++++++- src/provider/route.rs | 51 ++++++++++++ src/ui/slash/cmd/model.rs | 6 +- 3 files changed, 218 insertions(+), 2 deletions(-) diff --git a/src/provider/resolve.rs b/src/provider/resolve.rs index d537619ed..d8bca0d99 100644 --- a/src/provider/resolve.rs +++ b/src/provider/resolve.rs @@ -175,6 +175,12 @@ pub(super) enum ModelSwitch { Keep, /// Rebuild the client against this configured provider alias, then rename. Switch(String), + /// GH #825: the id named a configured provider ALIAS that pins a `model`. + /// Unlike [`ModelSwitch::Switch`], the model the session must land on is + /// NOT the id the user typed — the alias name itself is a string no + /// endpoint serves — so this carries the alias's pinned model alongside + /// the alias, sparing the route layer from re-deriving the pin. + SwitchAlias { alias: String, model: String }, /// The id's family maps to a provider kind with NO configured provider. /// Renaming on the active client would send it to the wrong endpoint, so /// the caller should warn instead. Carries the human family name. @@ -188,7 +194,10 @@ pub(super) enum ModelSwitch { /// 1. A model explicitly pinned on the active provider, or the active /// provider kind's built-in default, stays on the active client. /// 2. An exact pin on another provider's `model` switches to it. -/// 3. Otherwise infer the id's family ([`model_family`]): +/// 3. An id naming a configured provider alias that pins a `model` switches +/// to that alias's pinned model (GH #825). Ranked below the exact-pin +/// rules so an alias named like a real model id can't shadow the id. +/// 4. Otherwise infer the id's family ([`model_family`]): /// - unclassifiable, or same kind as the active provider → `Keep` (the /// active client already speaks this family; just rename). /// - a different kind with a configured provider of that kind → switch to @@ -238,6 +247,31 @@ pub(super) fn resolve_model_switch( return ModelSwitch::Keep; } + // GH #825: `/model ` — the id names a configured provider + // alias rather than a model. This used to fall through to the family + // inference below, come back `Keep`, and rename the session model to the + // alias string itself — an id no endpoint serves, so the next turn 400'd. + // Resolve it to the alias's pinned model instead. Only an alias that PINS + // a model is resolvable this way; one relying on its provider default + // falls through unchanged. Placed after the exact-pin and gateway rules + // so an alias whose name collides with a real model id never shadows the + // id's own pin, and deliberately NOT a whitelist: a non-alias id keeps + // today's permissive `Keep`, which is what lets a brand-new model id work + // before dirge knows it. + if let Some((alias, entry)) = providers + .get_key_value(model) + .or_else(|| providers.get_key_value(&model.to_ascii_lowercase())) + && let Some(pinned) = entry.model.as_deref() + { + // The active provider's own alias also lands here: the route layer's + // already-live guard makes that a rename onto the pinned model with + // no client rebuild and no spurious switch note. + return ModelSwitch::SwitchAlias { + alias: alias.clone(), + model: pinned.to_string(), + }; + } + let Some(family) = model_family(model) else { return ModelSwitch::Keep; }; @@ -1311,6 +1345,133 @@ mod resolve_model_switch_tests { ModelSwitch::Keep, ); } + + /// GH #825's shape: tier-style aliases, each pinning a model. + fn tiered_providers() -> HashMap { + HashMap::from([ + ( + "low".to_string(), + typed_entry("anthropic", Some("claude-haiku-4-5")), + ), + ( + "high".to_string(), + typed_entry("anthropic", Some("claude-opus-5")), + ), + ]) + } + + /// GH #825: `/model low` used to come back `Keep` and rename the session + /// model to the alias string itself — an id no endpoint serves. An alias + /// that pins a model now resolves to that alias and its pinned model. + #[test] + fn alias_resolves_to_its_pinned_model() { + assert_eq!( + resolve_model_switch(&tiered_providers(), "high", "low"), + ModelSwitch::SwitchAlias { + alias: "low".to_string(), + model: "claude-haiku-4-5".to_string(), + }, + ); + } + + /// Alias lookup follows the same case convention as the rest of this file + /// (`get` then `get` on the lowercased name). + #[test] + fn alias_lookup_is_case_insensitive() { + assert_eq!( + resolve_model_switch(&tiered_providers(), "high", "LOW"), + ModelSwitch::SwitchAlias { + alias: "low".to_string(), + model: "claude-haiku-4-5".to_string(), + }, + ); + } + + /// The active provider's own alias resolves to its own pinned model; the + /// route layer's already-live guard then makes applying it a no-op (no + /// client rebuild, no spurious switch note) — pinned at the route layer. + #[test] + fn own_alias_resolves_to_the_active_pin() { + assert_eq!( + resolve_model_switch(&tiered_providers(), "high", "high"), + ModelSwitch::SwitchAlias { + alias: "high".to_string(), + model: "claude-opus-5".to_string(), + }, + ); + } + + /// An alias with NO pinned model has nothing to resolve to — inventing a + /// model string would be guessing. It falls through to the unchanged + /// pre-#825 behavior (here: unclassifiable name → keep). + #[test] + fn alias_without_a_pinned_model_falls_through() { + let providers = HashMap::from([ + ("local".to_string(), typed_entry("ollama", None)), + ( + "high".to_string(), + typed_entry("anthropic", Some("claude-opus-5")), + ), + ]); + assert_eq!( + resolve_model_switch(&providers, "high", "local"), + ModelSwitch::Keep, + ); + } + + /// GH #825 must not narrow the deliberate permissiveness: an id that is + /// neither an alias nor classifiable still keeps the active client, which + /// is what lets a brand-new model id work before dirge knows it. + #[test] + fn unknown_non_alias_id_still_keeps_the_active_client() { + assert_eq!( + resolve_model_switch(&tiered_providers(), "high", "banana"), + ModelSwitch::Keep, + ); + } + + /// An alias whose NAME collides with a real model id must not shadow the + /// id's own exact pin — the exact-pin rules stay ahead of the alias rule. + #[test] + fn exact_pin_wins_over_a_same_named_alias() { + let providers = HashMap::from([ + // An alias unluckily named like a model id, pinning something else. + ("gpt-5.5".to_string(), typed_entry("glm", Some("glm-5.2"))), + // The provider that actually pins the id. + ( + "azure-gpt".to_string(), + typed_entry("openai", Some("gpt-5.5")), + ), + ( + "high".to_string(), + typed_entry("anthropic", Some("claude-opus-5")), + ), + ]); + assert_eq!( + resolve_model_switch(&providers, "high", "gpt-5.5"), + ModelSwitch::Switch("azure-gpt".to_string()), + ); + } + + /// The active provider's own pinned model stays rule 1 even when an alias + /// shares its name — `Keep` wins before the alias rule is reached. + #[test] + fn active_pin_wins_over_a_same_named_alias() { + let providers = HashMap::from([ + ( + "claude-opus-5".to_string(), + typed_entry("openai", Some("gpt-5.5")), + ), + ( + "high".to_string(), + typed_entry("anthropic", Some("claude-opus-5")), + ), + ]); + assert_eq!( + resolve_model_switch(&providers, "high", "claude-opus-5"), + ModelSwitch::Keep, + ); + } } #[cfg(test)] diff --git a/src/provider/route.rs b/src/provider/route.rs index 72faeb214..317159f42 100644 --- a/src/provider/route.rs +++ b/src/provider/route.rs @@ -109,6 +109,10 @@ pub fn resolve_model_route(cfg: &Config, active_provider: &str, model: &str) -> match super::resolve_model_switch(&cfg.providers_map(), active_provider, &model) { ModelSwitch::Keep => ModelRoute::Active(model), ModelSwitch::Switch(alias) => ModelRoute::Provider { alias, model }, + // GH #825: the id named a provider alias — the route must carry the + // alias's pinned model, NOT the alias string the user typed, or the + // session would land on a "model" no endpoint serves. + ModelSwitch::SwitchAlias { alias, model } => ModelRoute::Provider { alias, model }, ModelSwitch::NoProviderForFamily(family) => ModelRoute::Unroutable { model, family }, } } @@ -466,4 +470,51 @@ mod tests { "the window must follow the model, not stay stale", ); } + + /// GH #825: `/model ` lands the session on the alias's + /// PINNED model, not on the alias string — the failure mode where the + /// client switches but the session keeps the alias as its "model" would + /// 400 on the first turn just like the original bug. + #[test] + fn applying_an_alias_route_lands_on_the_pinned_model() { + let cfg = cfg(); + let mut client = client(&cfg, "gpt-sol"); + let mut session = session("gpt-sol", "gpt-5.5"); + + let switched = apply_model_route( + &cfg, + &mut client, + &mut session, + resolve_model_route(&cfg, "gpt-sol", "glm"), + ) + .expect("a configured alias must be routable"); + + assert_eq!(switched.as_deref(), Some("glm")); + assert!(matches!(client, AnyClient::Glm(_)), "client must move"); + assert_eq!(session.model, "glm-5.2", "the PINNED model, not `glm`"); + assert_eq!(session.provider, "glm"); + } + + /// GH #825: naming the ACTIVE provider's own alias is a no-op — no client + /// rebuild, no spurious switch note, and the session model stays the + /// alias's pinned model rather than becoming the alias string. + #[test] + fn applying_the_active_alias_route_is_a_no_op() { + let cfg = cfg(); + let mut client = client(&cfg, "gpt-sol"); + let mut session = session("gpt-sol", "gpt-5.5"); + + let switched = apply_model_route( + &cfg, + &mut client, + &mut session, + resolve_model_route(&cfg, "gpt-sol", "gpt-sol"), + ) + .unwrap(); + + assert_eq!(switched, None, "no client swap, so no switch note"); + assert!(matches!(client, AnyClient::OpenAI(_)), "client untouched"); + assert_eq!(session.model, "gpt-5.5"); + assert_eq!(session.provider, "gpt-sol"); + } } diff --git a/src/ui/slash/cmd/model.rs b/src/ui/slash/cmd/model.rs index 9b3cef90a..9b290a256 100644 --- a/src/ui/slash/cmd/model.rs +++ b/src/ui/slash/cmd/model.rs @@ -89,8 +89,12 @@ pub(crate) async fn cmd_model(ctx: &mut SlashCtx<'_>, parts: &[&str]) -> anyhow: .as_deref() .map(|a| format!(" · {a}")) .unwrap_or_default(); + // GH #825: report the model the session actually landed on, not the + // raw argument — `/model ` resolves to the alias's + // pinned model, so echoing the argument would print the alias string + // as if it were a model id. Identical on every non-alias path. ctx.renderer.write_line( - &format!("switched to model: {new_model}{provider_note}"), + &format!("switched to model: {}{provider_note}", ctx.session.model), c_agent(), )?; let reserve = ctx.cfg.resolve_reserve_tokens();