Merge PR #6693 (fix/provider-switch-keeps-configured-model) into integrate/0.10.1

CHANGELOG.md

@@ -29,6 +29,16 @@ 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, or a stale legacy root `model` that the
+  root default had been shadowing
+  ([#6693](https://github.com/Hmbown/Codewhale/pull/6693)).
+
 ### 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)).

crates/tui/CHANGELOG.md

@@ -29,6 +29,16 @@ 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, or a stale legacy root `model` that the
+  root default had been shadowing
+  ([#6693](https://github.com/Hmbown/Codewhale/pull/6693)).
+
 ### 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)).

crates/tui/src/config.rs

@@ -6243,6 +6243,53 @@ 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). The alias is `default_text_model`
+    /// or, when that is unset, the legacy root `model` it falls back to. A
+    /// persisted writer that moves it clears both roots
+    /// (`config_persistence::unset_root_model_aliases`); clearing only the
+    /// first would resurrect the second on the incoming route after reload.
+    pub(crate) fn root_model_alias_owned_by_outgoing(
+        &self,
+        incoming: &ProviderIdentity,
+    ) -> Option<(ProviderIdentity, String)> {
+        // `default_model` reads the legacy root `model` whenever
+        // `default_text_model` is unset, so the effective alias is either one.
+        let value = self
+            .default_text_model
+            .as_deref()
+            .or(self.legacy_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() {

crates/tui/src/config_persistence.rs

@@ -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,11 +276,26 @@ pub(crate) fn reconcile_root_model_aliases(
     if incoming.provider == ApiProvider::Custom && incoming.persisted_id().is_none() {
         return Ok(());
     }
-    // Only `default_text_model` is read here. The legacy root `model` key is
+    // 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_root_model_aliases(doc)?;
+        return Ok(());
+    }
+    // Below, only `default_text_model` is read. 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
     // `Config::validate` does not consult it, so relocating it would move a
-    // value nothing is asking about.
+    // value nothing is asking about. It is still cleared together with the
+    // alias that shadows it (`unset_root_model_aliases`).
     const ROOT_KEY: &str = "default_text_model";
     let Some(value) = doc
         .get(ROOT_KEY)
@@ -306,7 +324,7 @@ pub(crate) fn reconcile_root_model_aliases(
     // An empty or control-bearing alias names no saved model. Nothing to
     // relocate, and clearing it loses nothing.
     if value.trim().is_empty() || value.chars().any(char::is_control) {
-        unset_document_value(doc, &[ROOT_KEY])?;
+        unset_root_model_aliases(doc)?;
         return Ok(());
     }
     // No leaf can hold this value. Keep the only copy of the user's choice
@@ -344,7 +362,20 @@ pub(crate) fn reconcile_root_model_aliases(
     // The outgoing route either already saved its own choice or has just
     // received this one, so the alias is now a shadowed duplicate that only
     // blocks the incoming route.
-    unset_document_value(doc, &[ROOT_KEY])?;
+    unset_root_model_aliases(doc)?;
+    Ok(())
+}
+
+/// Clear the root model alias: `default_text_model` and the legacy root
+/// `model` that `Config::default_model` falls back to when the former is
+/// unset. While `default_text_model` is present the legacy key resolves on no
+/// route, so removing only the alias would resurrect a stale legacy model on
+/// the incoming route (a pass-through route would then use a model nobody
+/// chose for it). When the legacy key *is* the alias, the caller has already
+/// moved its value onto the outgoing route's leaf.
+fn unset_root_model_aliases(doc: &mut toml_edit::DocumentMut) -> anyhow::Result<()> {
+    unset_document_value(doc, &["default_text_model"])?;
+    unset_document_value(doc, &["model"])?;
     Ok(())
 }
 
@@ -1822,6 +1853,47 @@ slot = 1
         );
     }
 
+    #[test]
+    fn moving_root_default_off_the_root_does_not_resurrect_a_shadowed_legacy_model() {
+        use crate::test_support::{EnvVarGuard, lock_test_env};
+        let _lock = lock_test_env();
+        let home = tempfile::tempdir().unwrap();
+        let _home = EnvVarGuard::set("CODEWHALE_HOME", home.path());
+        let _model_guard = ModelEnvGuard::new();
+        let path = home.path().join("config.toml");
+        // `default_text_model` shadows the legacy root `model` on every route.
+        // Moving the alias onto openai's leaf must not un-shadow the legacy
+        // value for the pass-through incoming route.
+        fs::write(
+            &path,
+            "route_preferences_version = 1\nprovider = 'openai'\nmodel = 'deepseek-v4-flash'\ndefault_text_model = 'gpui-fixture'\n",
+        )
+        .unwrap();
+        persist_provider_selection(Some(&path), ApiProvider::Openrouter, "openrouter", None)
+            .unwrap();
+        let doc: toml::Value = toml::from_str(&fs::read_to_string(&path).unwrap()).unwrap();
+        assert!(doc.get("default_text_model").is_none(), "{doc:?}");
+        assert!(doc.get("model").is_none(), "{doc:?}");
+        assert_eq!(
+            doc["providers"]["openai"]["model"].as_str(),
+            Some("gpui-fixture")
+        );
+        let switched = crate::config::Config::load(Some(path.clone()), None).unwrap();
+        assert_eq!(switched.api_provider(), ApiProvider::Openrouter);
+        // OpenRouter normalizes the stale value to `deepseek/deepseek-v4-flash`.
+        assert!(
+            !switched.default_model().contains("deepseek-v4-flash"),
+            "{}",
+            switched.default_model()
+        );
+        assert_ne!(switched.default_model(), "gpui-fixture");
+
+        persist_provider_selection(Some(&path), ApiProvider::Openai, "openai", None).unwrap();
+        let returned = crate::config::Config::load(Some(path.clone()), None).unwrap();
+        assert_eq!(returned.api_provider(), ApiProvider::Openai);
+        assert_eq!(returned.default_model(), "gpui-fixture");
+    }
+
     #[test]
     fn switching_provider_away_and_back_preserves_every_route_selection() {
         use crate::test_support::{EnvVarGuard, lock_test_env};
@@ -1847,14 +1919,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 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")

crates/tui/src/route_preferences.rs

@@ -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);

crates/tui/src/runtime_api/tests.rs

@@ -23399,3 +23399,127 @@ 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(())
+}
+
+#[tokio::test]
+async fn switch_provider_does_not_resurrect_a_shadowed_legacy_root_model() -> Result<()> {
+    // Review regression (#6693): the legacy root `model` was shadowed by
+    // `default_text_model`. Moving only the alias off the root let the
+    // pass-through openrouter route resolve `deepseek-v4-flash`.
+    let root = std::env::temp_dir().join(format!("codewhale-switch-legacy-{}", Uuid::new_v4()));
+    fs::create_dir_all(&root)?;
+    let config_file = root.join("custom-config.toml");
+    fs::write(
+        &config_file,
+        r#"model = "deepseek-v4-flash"
+default_text_model = "gpui-fixture"
+provider = "openai"
+
+[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}");
+    // OpenRouter normalizes the stale value to `deepseek/deepseek-v4-flash`.
+    assert!(
+        !away["model"]
+            .as_str()
+            .unwrap_or_default()
+            .contains("deepseek-v4-flash"),
+        "body: {away}"
+    );
+    assert_ne!(away["model"].as_str(), Some("gpui-fixture"), "body: {away}");
+
+    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}");
+
+    handle.abort();
+    Ok(())
+}

crates/tui/src/tui/ui/provider_routes.rs

@@ -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) => {

crates/tui/src/tui/ui/tests.rs

@@ -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");
+}