mirror of
https://github.com/codewhale-hq/Codewhale.git
synced 2026-11-08 17:10:50 -10:00
fix(provider): keep a root default model with the provider that owns it
Cause: with `provider = "openai"`, `default_text_model = "gpui-fixture"` and no `[providers.openai].model`, the root alias is the active route's fallback and nothing records which provider it belongs to. - TUI: a session-local `/provider <other>` left the alias at the root, so `/provider openai` afterwards resolved with `openai != api_provider()` and fell through to the catalog default `gpt-5.6`. - Engine: `reconcile_root_model_aliases` relocated the alias only when the incoming route failed validation. A pass-through incoming route (openrouter, zai, ...) inherited `gpui-fixture`, and `GET /v1/providers` advertised OpenAI's `default_model` as `gpt-5.6`, which is what the desktop model chip picked for "OpenAI-compatible". Fix: `Config::root_model_alias_owned_by_outgoing` names the outgoing route and value when that route has no leaf and was actually resolving the alias. The persisted writer moves it onto the outgoing leaf and clears the root; the TUI switch applies the same move in memory. The resolution order is then providers.<p>.model, the root alias while its own provider is active, then the catalog default. Aliases the outgoing route never resolved (shadowed by a leaf, `auto`, foreign ids dropped by `default_model`) keep their previous handling. Two persistence tests that asserted the alias stays at the root in the no-leaf case now assert it moves to the outgoing leaf; switching back is still checked. Tests (focused, CARGO_BUILD_JOBS=4): - new: tui::ui::tests::provider_switch_back_lands_on_root_default_owned_by_that_provider and runtime_api::tests::switch_provider_away_and_back_keeps_the_root_default_with_its_route fail without the fix (left: "gpt-5.6" right: "gpui-fixture"; openrouter body model "gpui-fixture"): test result: FAILED. 3 passed; 2 failed; 0 ignored; 0 measured; 13796 filtered out - with the fix, filter root_default: test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 13796 filtered out - filters config_persistence default_text_model alias route_save save_route provider_picker route_preferences provider_switch switch_provider root_default: test result: ok. 469 passed; 0 failed; 0 ignored; 0 measured; 13332 filtered out - filters config::tests model_inventory route_runtime runtime_api::tests::switch runtime_api::tests::get_config provider: test result: ok. 1302 passed; 0 failed; 1 ignored; 0 measured; 12498 filtered out Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_014ZwqatxgVFxHvovngywnks
This commit is contained in:
1 parent
e4c94c8495
commit
1394c299e7
8 files changed
+238
-21
No files matched your search
@@ -29,6 +29,14 @@ quieter, and Fleet runs can be checked before they spend anything.
|
||||
no-follow reads and writes. Compaction loads pinned anchors only from
|
||||
trusted workspaces. Validate registry skill names before selecting cache paths.
|
||||
|
||||
### Fixed
|
||||
|
||||
- Switching providers keeps a model set only in the root `default_text_model`
|
||||
with the provider it belongs to. Switching away and back (`/provider` in the
|
||||
TUI, or the desktop app's model chip) used to land on the provider's catalog
|
||||
default such as `gpt-5.6`, and a pass-through provider switched to in between
|
||||
inherited the other provider's model.
|
||||
|
||||
### Contributors
|
||||
|
||||
- **[@gaord](https://github.com/gaord)** — let a client fork a thread at a named turn ([#6580](https://github.com/Hmbown/Codewhale/pull/6580)), let undo roll back files for the turn it is undoing ([#6483](https://github.com/Hmbown/Codewhale/pull/6483)), stopped resume and fork from duplicating threads and sessions ([#6406](https://github.com/Hmbown/Codewhale/pull/6406)), and exposed user-defined provider routes to native clients ([#6404](https://github.com/Hmbown/Codewhale/pull/6404)).
|
||||
|
||||
@@ -29,6 +29,14 @@ quieter, and Fleet runs can be checked before they spend anything.
|
||||
no-follow reads and writes. Compaction loads pinned anchors only from
|
||||
trusted workspaces. Validate registry skill names before selecting cache paths.
|
||||
|
||||
### Fixed
|
||||
|
||||
- Switching providers keeps a model set only in the root `default_text_model`
|
||||
with the provider it belongs to. Switching away and back (`/provider` in the
|
||||
TUI, or the desktop app's model chip) used to land on the provider's catalog
|
||||
default such as `gpt-5.6`, and a pass-through provider switched to in between
|
||||
inherited the other provider's model.
|
||||
|
||||
### Contributors
|
||||
|
||||
- **[@gaord](https://github.com/gaord)** — let a client fork a thread at a named turn ([#6580](https://github.com/Hmbown/Codewhale/pull/6580)), let undo roll back files for the turn it is undoing ([#6483](https://github.com/Hmbown/Codewhale/pull/6483)), stopped resume and fork from duplicating threads and sessions ([#6406](https://github.com/Hmbown/Codewhale/pull/6406)), and exposed user-defined provider routes to native clients ([#6404](https://github.com/Hmbown/Codewhale/pull/6404)).
|
||||
|
||||
@@ -6243,6 +6243,43 @@ impl Config {
|
||||
deepseek_alias_deprecation(&alias)
|
||||
}
|
||||
|
||||
/// The root `default_text_model` alias is the *active* route's fallback.
|
||||
/// When a switch to `incoming` leaves a route that has no model leaf of its
|
||||
/// own and was actually resolving the alias, return that outgoing identity
|
||||
/// and value: the choice belongs to the outgoing route and must follow it
|
||||
/// onto its own leaf, rather than stay at the root where the incoming route
|
||||
/// would inherit it and the outgoing route would forget it (falling back to
|
||||
/// its catalog default on the way back).
|
||||
pub(crate) fn root_model_alias_owned_by_outgoing(
|
||||
&self,
|
||||
incoming: &ProviderIdentity,
|
||||
) -> Option<(ProviderIdentity, String)> {
|
||||
let value = self.default_text_model.as_deref()?.trim();
|
||||
if value.is_empty()
|
||||
|| value.eq_ignore_ascii_case("auto")
|
||||
|| value.chars().any(char::is_control)
|
||||
{
|
||||
return None;
|
||||
}
|
||||
let outgoing = self.active_provider_identity(self.api_provider()).ok()?;
|
||||
// An unnamed custom route stores its model in the alias itself.
|
||||
if outgoing == *incoming
|
||||
|| (outgoing.provider == ApiProvider::Custom && outgoing.persisted_id().is_none())
|
||||
{
|
||||
return None;
|
||||
}
|
||||
let mut scoped = self.clone();
|
||||
scoped.scope_to_provider_identity(&outgoing);
|
||||
if scoped
|
||||
.provider_config_for(outgoing.provider)
|
||||
.and_then(|entry| entry.model.as_deref())
|
||||
.is_some()
|
||||
{
|
||||
return None;
|
||||
}
|
||||
(scoped.default_model() == value).then(|| (outgoing, value.to_string()))
|
||||
}
|
||||
|
||||
#[must_use]
|
||||
pub fn default_model(&self) -> String {
|
||||
if self.default_text_model.is_none() && self.legacy_model.is_some() {
|
||||
|
||||
@@ -248,13 +248,16 @@ pub(crate) fn persist_provider_selection(
|
||||
/// when the selected route has no model of its own. A switch that leaves the
|
||||
/// incoming route without a leaf therefore hands it an alias naming the route
|
||||
/// on its way out, and `Config::load` rejects the file the caller just wrote —
|
||||
/// a committed switch that produces an unloadable config.
|
||||
/// a committed switch that produces an unloadable config — or, on a
|
||||
/// pass-through incoming route that loads it, a model the incoming route never
|
||||
/// chose while the outgoing route comes back on its catalog default.
|
||||
///
|
||||
/// Relocate that value onto the outgoing route's own canonical leaf, because
|
||||
/// it is real saved state, and only then clear the alias. An alias the
|
||||
/// incoming route already shadows with its own leaf is inert and stays:
|
||||
/// deleting a saved choice to satisfy validation of a value nothing resolves
|
||||
/// is data loss, not a repair. Likewise an unnamed custom route stores its
|
||||
/// When the outgoing route has no leaf and was resolving the alias, relocate
|
||||
/// that value onto the outgoing route's own canonical leaf, because it is real
|
||||
/// saved state, and only then clear the alias. Any other alias the incoming
|
||||
/// route already shadows with its own leaf is inert and stays: deleting a
|
||||
/// saved choice to satisfy validation of a value nothing resolves is data
|
||||
/// loss, not a repair. Likewise an unnamed custom route stores its
|
||||
/// model in the alias itself and has no leaf to receive it. If the incoming
|
||||
/// route cannot shadow that value, refuse the switch atomically and ask for an
|
||||
/// explicit destination model instead of saving an unloadable configuration.
|
||||
@@ -273,6 +276,20 @@ pub(crate) fn reconcile_root_model_aliases(
|
||||
if incoming.provider == ApiProvider::Custom && incoming.persisted_id().is_none() {
|
||||
return Ok(());
|
||||
}
|
||||
// The outgoing route was resolving the alias as its own model. Move it
|
||||
// onto that route's leaf even when the incoming route could load it: a
|
||||
// pass-through incoming route would otherwise inherit a model it never
|
||||
// chose, and the outgoing route would come back on its catalog default.
|
||||
if let Some((outgoing, value)) = previous.root_model_alias_owned_by_outgoing(incoming) {
|
||||
set_provider_model_document(
|
||||
doc,
|
||||
outgoing.provider,
|
||||
outgoing.persisted_id().unwrap_or(&outgoing.key),
|
||||
&value,
|
||||
)?;
|
||||
unset_document_value(doc, &["default_text_model"])?;
|
||||
return Ok(());
|
||||
}
|
||||
// Only `default_text_model` is read here. The legacy root `model` key is
|
||||
// never what blocks a load: `Config::default_model` already refuses to
|
||||
// route a foreign legacy value to a provider that cannot serve it, and
|
||||
@@ -1847,14 +1864,24 @@ slot = 1
|
||||
)
|
||||
.unwrap();
|
||||
let doc: toml::Value = toml::from_str(&fs::read_to_string(&path).unwrap()).unwrap();
|
||||
// The incoming route owns its own leaf, so the outgoing root
|
||||
// alias is inert. It is a saved choice: deleting it to satisfy
|
||||
// validation of a value nothing resolves would be data loss.
|
||||
assert_eq!(
|
||||
doc[root_key].as_str(),
|
||||
Some("GLM-4.6"),
|
||||
"an inert root fallback must survive the switch"
|
||||
);
|
||||
// The outgoing route was resolving the root alias only when it
|
||||
// had no leaf; then the choice moves onto that leaf. Otherwise
|
||||
// the alias is inert saved state and stays. Neither case may
|
||||
// delete it.
|
||||
if root_key == "default_text_model" && existing.is_none() {
|
||||
assert!(doc.get(root_key).is_none(), "{doc:?}");
|
||||
assert_eq!(
|
||||
doc["providers"]["zai"]["model"].as_str(),
|
||||
Some("GLM-4.6"),
|
||||
"the outgoing route's choice must follow it onto its leaf"
|
||||
);
|
||||
} else {
|
||||
assert_eq!(
|
||||
doc[root_key].as_str(),
|
||||
Some("GLM-4.6"),
|
||||
"an inert root fallback must survive the switch"
|
||||
);
|
||||
}
|
||||
assert_eq!(
|
||||
doc["providers"]["deepseek"]["model"].as_str(),
|
||||
Some("deepseek-v4-pro")
|
||||
|
||||
@@ -576,18 +576,17 @@ model = "Other-X"
|
||||
.collect();
|
||||
let path = home.path().join("config.toml");
|
||||
|
||||
// The incoming route owns its own leaf, so the outgoing root fallback
|
||||
// is inert saved state. A CLI switch must not delete it, and switching
|
||||
// back must still find it.
|
||||
// The outgoing route was resolving the root fallback as its own
|
||||
// model, so a CLI switch moves it onto that route's leaf (never
|
||||
// deletes it), and switching back must still find it.
|
||||
std::fs::write(
|
||||
&path,
|
||||
"route_preferences_version = 1\nprovider = \"zai\"\ndefault_text_model = \"GLM-4.6\"\n[providers.deepseek]\nmodel = \"deepseek-v4-pro\"\n",
|
||||
)?;
|
||||
set(&path, "provider", "deepseek")?;
|
||||
assert_eq!(
|
||||
document(&path)["default_text_model"].as_str(),
|
||||
Some("GLM-4.6")
|
||||
);
|
||||
let doc = document(&path);
|
||||
assert!(doc.get("default_text_model").is_none());
|
||||
assert_eq!(doc["providers"]["zai"]["model"].as_str(), Some("GLM-4.6"));
|
||||
let switched = Config::load(Some(path.clone()), None)
|
||||
.expect("a CLI provider switch must remain loadable");
|
||||
assert_eq!(switched.api_provider(), ApiProvider::Deepseek);
|
||||
|
||||
@@ -23399,3 +23399,75 @@ mod thread_snapshot_ownership {
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn switch_provider_away_and_back_keeps_the_root_default_with_its_route() -> Result<()> {
|
||||
// Device-test regression: `provider = "openai"` with the model only in the
|
||||
// root `default_text_model`. Switching to a pass-through route used to leave
|
||||
// the alias at the root, so that route inherited `gpui-fixture`, the
|
||||
// provider list advertised OpenAI's catalog default (`gpt-5.6`), and the
|
||||
// desktop model chip landed there.
|
||||
let root = std::env::temp_dir().join(format!("codewhale-switch-root-alias-{}", Uuid::new_v4()));
|
||||
fs::create_dir_all(&root)?;
|
||||
let config_file = root.join("custom-config.toml");
|
||||
fs::write(
|
||||
&config_file,
|
||||
r#"provider = "openai"
|
||||
default_text_model = "gpui-fixture"
|
||||
|
||||
[providers.openai]
|
||||
api_key = "sk-test"
|
||||
base_url = "http://127.0.0.1:9/v1"
|
||||
|
||||
[providers.openrouter]
|
||||
api_key = "sk-or-test"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let Some((addr, _runtime_threads, handle)) =
|
||||
spawn_test_server_with_config_path(config_file.clone()).await?
|
||||
else {
|
||||
return Ok(());
|
||||
};
|
||||
let client = crate::tls::reqwest_client();
|
||||
|
||||
let (status, away) =
|
||||
post_switch_provider(&client, &addr, "openrouter", &serde_json::json!({})).await;
|
||||
assert_eq!(status, StatusCode::OK, "body: {away}");
|
||||
assert_ne!(
|
||||
away["model"].as_str(),
|
||||
Some("gpui-fixture"),
|
||||
"openrouter must not inherit openai's root default: {away}"
|
||||
);
|
||||
let persisted = fs::read_to_string(&config_file)?;
|
||||
assert!(
|
||||
!persisted.contains("default_text_model"),
|
||||
"the alias must move off the root. Actual config:\n{persisted}"
|
||||
);
|
||||
|
||||
let providers: serde_json::Value = client
|
||||
.get(format!("http://{addr}/v1/providers"))
|
||||
.send()
|
||||
.await?
|
||||
.json()
|
||||
.await?;
|
||||
let openai = providers["providers"]
|
||||
.as_array()
|
||||
.and_then(|entries| entries.iter().find(|entry| entry["id"] == "openai"))
|
||||
.cloned()
|
||||
.unwrap_or_default();
|
||||
assert_eq!(
|
||||
openai["default_model"].as_str(),
|
||||
Some("gpui-fixture"),
|
||||
"the inactive openai entry must keep its configured model, not the catalog default: {openai}"
|
||||
);
|
||||
|
||||
let (status, back) =
|
||||
post_switch_provider(&client, &addr, "openai", &serde_json::json!({})).await;
|
||||
assert_eq!(status, StatusCode::OK, "body: {back}");
|
||||
assert_eq!(back["model"].as_str(), Some("gpui-fixture"), "body: {back}");
|
||||
assert_eq!(get_config(&client, &addr).await["model"], "gpui-fixture");
|
||||
|
||||
handle.abort();
|
||||
Ok(())
|
||||
}
|
||||
@@ -624,6 +624,20 @@ pub(crate) async fn switch_provider(
|
||||
previous_api_key_env_only: app.api_key_env_only,
|
||||
});
|
||||
|
||||
// A session-local switch keeps the same ownership rule the persisted
|
||||
// writers apply (`reconcile_root_model_aliases`): the root alias the
|
||||
// outgoing route was using moves onto that route's own leaf, so coming
|
||||
// back lands on it instead of the catalog default. Every failure path
|
||||
// below restores `previous_config`.
|
||||
if let Some((outgoing, value)) = config
|
||||
.active_provider_identity(target)
|
||||
.ok()
|
||||
.and_then(|incoming| config.root_model_alias_owned_by_outgoing(&incoming))
|
||||
{
|
||||
config.set_provider_model_override(outgoing.provider, Some(value));
|
||||
config.default_text_model = None;
|
||||
}
|
||||
|
||||
let resolved_route = match resolve_runtime_route(config, target, model_override.as_deref()) {
|
||||
Ok(route) => route,
|
||||
Err(reason) => {
|
||||
|
||||
@@ -31744,3 +31744,55 @@ fn background_review_failed_shell_receipt_uses_context_neutral_copy() {
|
||||
"shell · false · échec · code de sortie 2"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn provider_switch_back_lands_on_root_default_owned_by_that_provider() {
|
||||
// Device-test regression: with the openai model only in the root
|
||||
// `default_text_model`, a session-local `/provider deepseek` left the alias
|
||||
// at the root, so `/provider openai` came back on the catalog default.
|
||||
let _home = SettingsHomeGuard::new();
|
||||
let mut app = create_test_app();
|
||||
app.api_provider = ApiProvider::Openai;
|
||||
app.model = "gpui-fixture".to_string();
|
||||
let mut engine = mock_engine_handle();
|
||||
let mut config = Config {
|
||||
provider: Some("openai".to_string()),
|
||||
default_text_model: Some("gpui-fixture".to_string()),
|
||||
providers: Some(ProvidersConfig {
|
||||
openai: ProviderConfig {
|
||||
api_key: Some("sk-test".to_string()),
|
||||
base_url: Some("http://127.0.0.1:9/v1".to_string()),
|
||||
..Default::default()
|
||||
},
|
||||
..Default::default()
|
||||
}),
|
||||
..Default::default()
|
||||
}
|
||||
.with_legacy_root(Some("deepseek-key".to_string()), None);
|
||||
|
||||
assert!(
|
||||
switch_provider(
|
||||
&mut app,
|
||||
&mut engine.handle,
|
||||
&mut config,
|
||||
ApiProvider::Deepseek,
|
||||
None,
|
||||
)
|
||||
.await
|
||||
);
|
||||
assert_eq!(app.api_provider, ApiProvider::Deepseek);
|
||||
assert_ne!(app.model, "gpui-fixture");
|
||||
|
||||
assert!(
|
||||
switch_provider(
|
||||
&mut app,
|
||||
&mut engine.handle,
|
||||
&mut config,
|
||||
ApiProvider::Openai,
|
||||
None,
|
||||
)
|
||||
.await
|
||||
);
|
||||
assert_eq!(app.api_provider, ApiProvider::Openai);
|
||||
assert_eq!(app.model, "gpui-fixture");
|
||||
}
|
||||
Reference in new issue
Block a user