Skip to content

Commit 1b45de0

Browse files
author
Yogthos
committed
Say when /model does not recognise the id (#831)
`resolve_model_switch` ends in a deliberate permissive fallthrough: an id matching no configured alias and no known model family returns `Keep`, which is what lets `/model claude-opus-6` work on release day. #826 narrowed the trap to genuinely unknown strings rather than closing it, on purpose, and that stays. But `Keep` is also what `/model off` gets, and reaching for `off` is a natural slip — it is a valid argument to both `/effort off` and `/agent off`, and `/model` has no "go back". It reported success and left `session.model = "off"`, with nothing said until the next request 400s. Keep applying it, and say so. The unrecognised case now returns its own `ModelSwitch::KeepUnrecognized`, carried through to `ModelRoute::Active { model, recognized }`, and `/model` appends one clause: switched to model: off (unrecognised — your provider may not serve it) The clause never fires on a configured alias, an exact pin, the gateway-dialect rule, or an id whose family `model_family` knows, so in practice it fires on typos and on genuinely new models. It asserts RECOGNITION, not validity — only the provider knows whether an id is servable, and the two come apart for a new-but-valid id — so the consequence stays conditional. Same clause on the ACP `/model` path.
1 parent 3736bb0 commit 1b45de0

6 files changed

Lines changed: 148 additions & 20 deletions

File tree

CHANGELOG.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,18 @@ adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
1818
reset instant when the provider gave one, and the provider's own text is kept
1919
verbatim as the cause. Nothing about the retry behaviour changes — a cap was
2020
already non-retryable. (#818)
21+
- `/model <id>` says when it does not recognise the id, instead of reporting a
22+
clean switch onto a string no endpoint serves. `resolve_model_switch` ends in a
23+
deliberate permissive fallthrough — an id matching no configured alias and no
24+
known model family still applies, which is what lets `/model claude-opus-6`
25+
work on release day — and that stays. But it is also what `/model off` got, and
26+
reaching for `off` is a natural slip: it is a valid argument to both `/effort`
27+
and `/agent`, and `/model` has no "go back". The switch now carries one clause,
28+
`(unrecognised — your provider may not serve it)`, which never fires on a
29+
configured alias, an exact pin, the gateway dialect, or an id whose family
30+
dirge knows. It asserts recognition rather than validity: only the provider
31+
knows whether an id is servable, and the two come apart for a new-but-valid
32+
id. Same clause on the ACP `/model` path. (#831)
2133
- A run that spins on no-op shell commands is now caught by the repeat-loop
2234
guard. A model that had decided it needed a different tool but kept reaching
2335
for `bash` anyway issued a long run of `echo "ready"` / `echo done` / `true`

src/extras/acp/mod.rs

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,17 @@ async fn acp_model(
939939
let refusal = RouteRefusal::NoProviderForFamily { model, family };
940940
return format!("{refusal} Keeping model '{current_model}' on '{current_provider}'.",);
941941
}
942-
ModelRoute::Active(_) => (None, String::new()),
942+
// GH #831: an id matching no configured alias and no known model family
943+
// still applies (see `ModelSwitch::KeepUnrecognized`), but say so — the
944+
// alternative is reporting a clean switch onto a string like `off`.
945+
ModelRoute::Active { recognized, .. } => (
946+
None,
947+
if recognized {
948+
String::new()
949+
} else {
950+
" (unrecognised — your provider may not serve it)".to_string()
951+
},
952+
),
943953
};
944954

945955
let mut map = sessions.lock().await;
@@ -1693,9 +1703,11 @@ mod tests {
16931703
assert!(list.contains("current model: current-x"), "got {list}");
16941704

16951705
// `llama-3.1` has no recognized family → ModelRoute::Active → override
1696-
// set, provider left alone.
1706+
// set, provider left alone. GH #831: it still applies, and now says the
1707+
// id is one dirge does not recognise.
16971708
let set = acp_model(&sessions, &cfg, id, "llama-3.1", "openrouter", "current-x").await;
16981709
assert!(set.contains("switched to model: llama-3.1"), "got {set}");
1710+
assert!(set.contains("unrecognised"), "got {set}");
16991711
let (model_ovr, provider_ovr, _) = session_overrides(&sessions, id).await;
17001712
assert_eq!(model_ovr.as_deref(), Some("llama-3.1"));
17011713
assert!(

src/provider/build.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1052,7 +1052,7 @@ pub fn resolve_profile_model(
10521052
};
10531053

10541054
match resolve_model_route(cfg, active_provider, &model) {
1055-
ModelRoute::Active(model) => Some(active_client.completion_model(model)),
1055+
ModelRoute::Active { model, .. } => Some(active_client.completion_model(model)),
10561056
ModelRoute::Provider { alias, model } => {
10571057
if !clients.contains_key(&alias) {
10581058
match build_route_client(cfg, &alias, &model) {

src/provider/resolve.rs

Lines changed: 60 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -173,6 +173,16 @@ pub(super) enum ModelSwitch {
173173
/// Keep the current client — the id belongs to the active provider (or
174174
/// carries no cross-provider signal). Just rename the model.
175175
Keep,
176+
/// GH #831: keep the current client, exactly like [`ModelSwitch::Keep`],
177+
/// but the id matched no configured provider alias and no model family
178+
/// [`model_family`] knows.
179+
///
180+
/// It still applies. The permissive fallthrough is deliberate — it is what
181+
/// lets `/model claude-opus-6` work on release day, before dirge has heard
182+
/// of the id — and a whitelist would be the wrong fix. The distinction
183+
/// exists so the caller can SAY the id is unrecognised rather than report a
184+
/// clean switch onto a string like `off`.
185+
KeepUnrecognized,
176186
/// Rebuild the client against this configured provider alias, then rename.
177187
Switch(String),
178188
/// GH #825: the id named a configured provider ALIAS that pins a `model`.
@@ -273,7 +283,10 @@ pub(super) fn resolve_model_switch(
273283
}
274284

275285
let Some(family) = model_family(model) else {
276-
return ModelSwitch::Keep;
286+
// GH #831: nothing recognised this id — not a pin, not a gateway
287+
// dialect, not an alias, not a family. Still `Keep` (see the variant's
288+
// doc), but flagged so `/model` can say so.
289+
return ModelSwitch::KeepUnrecognized;
277290
};
278291
if active_kind == Some(family) {
279292
return ModelSwitch::Keep;
@@ -1094,14 +1107,45 @@ mod resolve_model_switch_tests {
10941107
);
10951108
}
10961109

1110+
/// GH #831: no family signal → still keep the current client (unchanged
1111+
/// pre-fix behavior, so a local / alt same-provider id isn't disturbed),
1112+
/// but flagged as unrecognised so `/model` can say so rather than report a
1113+
/// clean switch onto a string no endpoint serves.
10971114
#[test]
1098-
fn unclassifiable_id_keeps_current_client() {
1115+
fn unclassifiable_id_keeps_current_client_and_is_flagged() {
10991116
let providers = user_like_providers();
1100-
// No family signal → keep current client (unchanged pre-fix behavior),
1101-
// so a local / alt same-provider id isn't disturbed.
11021117
assert_eq!(
11031118
resolve_model_switch(&providers, "deepseek", "some-local-model"),
1104-
ModelSwitch::Keep
1119+
ModelSwitch::KeepUnrecognized
1120+
);
1121+
// The reported slip: `off` is a valid argument to `/effort` and
1122+
// `/agent`, so reaching for `/model off` is natural.
1123+
assert_eq!(
1124+
resolve_model_switch(&providers, "deepseek", "off"),
1125+
ModelSwitch::KeepUnrecognized
1126+
);
1127+
}
1128+
1129+
/// The flag must NOT fire on the paths that DO recognise the id: a
1130+
/// same-family free-form id, an exact pin, a configured alias, or the
1131+
/// gateway dialect. Those all stay plain `Keep`/`Switch`.
1132+
#[test]
1133+
fn recognised_ids_are_not_flagged() {
1134+
let providers = user_like_providers();
1135+
assert_eq!(
1136+
resolve_model_switch(&providers, "glm", "glm-4.6"),
1137+
ModelSwitch::Keep,
1138+
"same family as the active provider",
1139+
);
1140+
assert_eq!(
1141+
resolve_model_switch(&providers, "deepseek", "deepseek-v4-pro"),
1142+
ModelSwitch::Keep,
1143+
"the active provider's own pin",
1144+
);
1145+
assert_eq!(
1146+
resolve_model_switch(&providers, "deepseek", "glm-5.2"),
1147+
ModelSwitch::Switch("glm".to_string()),
1148+
"an exact pin elsewhere",
11051149
);
11061150
}
11071151

@@ -1267,11 +1311,15 @@ mod resolve_model_switch_tests {
12671311
);
12681312
}
12691313

1314+
/// `gpt-oss-*` is deliberately unclassified (OpenAI authorship does not
1315+
/// imply an OpenAI endpoint), so it keeps the active client — flagged
1316+
/// unrecognised since GH #831, which changes what `/model` SAYS, not where
1317+
/// the id runs.
12701318
#[test]
12711319
fn cerebras_zero_config_gpt_oss_keeps_the_active_client() {
12721320
assert_eq!(
12731321
resolve_model_switch(&HashMap::new(), "cerebras", "gpt-oss-120b"),
1274-
ModelSwitch::Keep,
1322+
ModelSwitch::KeepUnrecognized,
12751323
);
12761324
}
12771325

@@ -1282,7 +1330,7 @@ mod resolve_model_switch_tests {
12821330

12831331
assert_eq!(
12841332
resolve_model_switch(&providers, "fast-cerebras", "gpt-oss-120b"),
1285-
ModelSwitch::Keep,
1333+
ModelSwitch::KeepUnrecognized,
12861334
);
12871335
}
12881336

@@ -1297,7 +1345,7 @@ mod resolve_model_switch_tests {
12971345
}
12981346
assert_eq!(
12991347
resolve_model_switch(&HashMap::new(), "deepseek", "gpt-oss-120b"),
1300-
ModelSwitch::Keep,
1348+
ModelSwitch::KeepUnrecognized,
13011349
);
13021350
}
13031351

@@ -1415,18 +1463,19 @@ mod resolve_model_switch_tests {
14151463
]);
14161464
assert_eq!(
14171465
resolve_model_switch(&providers, "high", "local"),
1418-
ModelSwitch::Keep,
1466+
ModelSwitch::KeepUnrecognized,
14191467
);
14201468
}
14211469

14221470
/// GH #825 must not narrow the deliberate permissiveness: an id that is
14231471
/// neither an alias nor classifiable still keeps the active client, which
1424-
/// is what lets a brand-new model id work before dirge knows it.
1472+
/// is what lets a brand-new model id work before dirge knows it. GH #831
1473+
/// flags it so `/model` can say so — the routing is unchanged.
14251474
#[test]
14261475
fn unknown_non_alias_id_still_keeps_the_active_client() {
14271476
assert_eq!(
14281477
resolve_model_switch(&tiered_providers(), "high", "banana"),
1429-
ModelSwitch::Keep,
1478+
ModelSwitch::KeepUnrecognized,
14301479
);
14311480
}
14321481

src/provider/route.rs

Lines changed: 36 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,17 @@ use super::AnyClient;
3333
pub enum ModelRoute {
3434
/// Build on the ACTIVE client — the id belongs to the active provider's
3535
/// family, or carries no cross-provider signal.
36-
Active(String),
36+
Active {
37+
model: String,
38+
/// GH #831: false when the id matched no configured provider alias and
39+
/// no model family dirge knows.
40+
///
41+
/// The route still applies — dirge cannot know whether an id is valid,
42+
/// only whether it RECOGNISES it, and those come apart for a
43+
/// new-but-valid id — but a caller holding this can say so instead of
44+
/// reporting a clean switch onto a string no endpoint serves.
45+
recognized: bool,
46+
},
3747
/// Build on `alias`'s client: the id's family differs from the active
3848
/// client's and a provider of that family is configured.
3949
Provider { alias: String, model: String },
@@ -47,7 +57,7 @@ impl ModelRoute {
4757
/// The model id this route carries, whatever the variant.
4858
pub fn model(&self) -> &str {
4959
match self {
50-
ModelRoute::Active(model)
60+
ModelRoute::Active { model, .. }
5161
| ModelRoute::Provider { model, .. }
5262
| ModelRoute::Unroutable { model, .. } => model,
5363
}
@@ -107,7 +117,16 @@ pub fn resolve_model_route(cfg: &Config, active_provider: &str, model: &str) ->
107117
use super::ModelSwitch;
108118
let model = model.to_string();
109119
match super::resolve_model_switch(&cfg.providers_map(), active_provider, &model) {
110-
ModelSwitch::Keep => ModelRoute::Active(model),
120+
ModelSwitch::Keep => ModelRoute::Active {
121+
model,
122+
recognized: true,
123+
},
124+
// GH #831: applied like any other `Keep`, but the caller is told the id
125+
// is one dirge does not recognise.
126+
ModelSwitch::KeepUnrecognized => ModelRoute::Active {
127+
model,
128+
recognized: false,
129+
},
111130
ModelSwitch::Switch(alias) => ModelRoute::Provider { alias, model },
112131
// GH #825: the id named a provider alias — the route must carry the
113132
// alias's pinned model, NOT the alias string the user typed, or the
@@ -145,7 +164,7 @@ pub fn swap_client_for_route(
145164
route: &ModelRoute,
146165
) -> Result<Option<String>, RouteRefusal> {
147166
match route {
148-
ModelRoute::Active(_) => Ok(None),
167+
ModelRoute::Active { .. } => Ok(None),
149168
// Already on that provider — this is a rename, not a swap. Rebuilding
150169
// would be wasted work and can fail outright: the rebuild re-resolves
151170
// credentials from config/env, so a session launched with `--api-key`
@@ -249,7 +268,19 @@ mod tests {
249268
);
250269
assert_eq!(
251270
resolve_model_route(&cfg, "gpt-sol", "gpt-5.5-mini"),
252-
ModelRoute::Active("gpt-5.5-mini".into()),
271+
ModelRoute::Active {
272+
model: "gpt-5.5-mini".into(),
273+
recognized: true,
274+
},
275+
);
276+
// GH #831: an id nothing classifies still routes to the active client,
277+
// but carries the flag `/model` uses to say so.
278+
assert_eq!(
279+
resolve_model_route(&cfg, "gpt-sol", "off"),
280+
ModelRoute::Active {
281+
model: "off".into(),
282+
recognized: false,
283+
},
253284
);
254285
assert_eq!(
255286
resolve_model_route(&cfg, "gpt-sol", "claude-opus-4"),

src/ui/slash/cmd/model.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,18 @@ pub(crate) async fn cmd_model(ctx: &mut SlashCtx<'_>, parts: &[&str]) -> anyhow:
6666
// Same-provider and unclassifiable ids keep the current client.
6767
let old_ctx = ctx.session.context_window;
6868
let route = resolve_model_route(ctx.cfg, ctx.session.provider.as_str(), new_model.as_str());
69+
// GH #831: read before the route is consumed. An id matching no
70+
// configured alias and no known model family still applies — that
71+
// permissive fallthrough is what lets a brand-new model id work before
72+
// dirge knows it — but `/model off` reporting a clean switch onto a
73+
// string no endpoint serves is what the report is about.
74+
let unrecognized = matches!(
75+
route,
76+
crate::provider::ModelRoute::Active {
77+
recognized: false,
78+
..
79+
}
80+
);
6981
// A refusal leaves the session untouched: renaming onto a client that
7082
// can't serve the id would just point the session at a model that can't
7183
// work. Keep it functional and say how to make the id routable.
@@ -93,8 +105,20 @@ pub(crate) async fn cmd_model(ctx: &mut SlashCtx<'_>, parts: &[&str]) -> anyhow:
93105
// raw argument — `/model <provider-alias>` resolves to the alias's
94106
// pinned model, so echoing the argument would print the alias string
95107
// as if it were a model id. Identical on every non-alias path.
108+
// The clause asserts RECOGNITION, not validity: only the provider
109+
// knows whether an id is servable, and the two come apart for a
110+
// new-but-valid id (`claude-opus-6` on release day). So it says what
111+
// dirge knows and leaves the consequence conditional.
112+
let unknown_note = if unrecognized {
113+
" (unrecognised — your provider may not serve it)"
114+
} else {
115+
""
116+
};
96117
ctx.renderer.write_line(
97-
&format!("switched to model: {}{provider_note}", ctx.session.model),
118+
&format!(
119+
"switched to model: {}{provider_note}{unknown_note}",
120+
ctx.session.model
121+
),
98122
c_agent(),
99123
)?;
100124
let reserve = ctx.cfg.resolve_reserve_tokens();

0 commit comments

Comments
 (0)