From b85c4605403648d0087ae788af38926ddbfb5e37 Mon Sep 17 00:00:00 2001 From: kyssta-exe Date: Wed, 24 Jun 2026 18:51:49 +0530 Subject: [PATCH] fix(tui): targeted save_config_value for model persistence (#48305) The TUI model-switch persistence (_persist_model_switch) rewrote the entire model config block via save_config(), destroying sibling keys the user set under model: (model_slots, model_fallback, base_url, ...) on every switch. Use targeted, atomic, comment-preserving save_config_value("model.default" / "model.provider" / "model.base_url") writes instead, so a model switch only touches the keys it changes. Salvaged from #48391 by kyssta-exe (authorship preserved). Fixes #48305 --- tests/test_tui_gateway_server.py | 82 ++++++++++++++++++++++++++++++-- tui_gateway/server.py | 26 +++++----- 2 files changed, 91 insertions(+), 17 deletions(-) diff --git a/tests/test_tui_gateway_server.py b/tests/test_tui_gateway_server.py index 93b2610e2..528e191ed 100644 --- a/tests/test_tui_gateway_server.py +++ b/tests/test_tui_gateway_server.py @@ -3266,7 +3266,7 @@ def test_config_set_model_global_persists(monkeypatch): warning_message="", ) seen = {} - saved = {} + saved_values = {} def _switch_model(**kwargs): seen.update(kwargs) @@ -3276,7 +3276,9 @@ def test_config_set_model_global_persists(monkeypatch): monkeypatch.setattr("hermes_cli.model_switch.switch_model", _switch_model) monkeypatch.setattr(server, "_restart_slash_worker", lambda sid, session: None) monkeypatch.setattr(server, "_emit", lambda *args, **kwargs: None) - monkeypatch.setattr("hermes_cli.config.save_config", lambda cfg: saved.update(cfg)) + # _persist_model_switch uses targeted save_config_value writes (#48305) so it + # preserves sibling model.* keys instead of rewriting the whole block. + monkeypatch.setattr("cli.save_config_value", lambda key, value: saved_values.__setitem__(key, value) or True) resp = server.handle_request( { @@ -3292,9 +3294,9 @@ def test_config_set_model_global_persists(monkeypatch): assert resp["result"]["value"] == "anthropic/claude-sonnet-4.6" assert seen["is_global"] is True - assert saved["model"]["default"] == "anthropic/claude-sonnet-4.6" - assert saved["model"]["provider"] == "anthropic" - assert saved["model"]["base_url"] == "https://api.anthropic.com" + assert saved_values["model.default"] == "anthropic/claude-sonnet-4.6" + assert saved_values["model.provider"] == "anthropic" + assert saved_values["model.base_url"] == "https://api.anthropic.com" def test_config_set_model_explicit_provider_skips_broken_default_init(monkeypatch): @@ -7988,3 +7990,73 @@ def test_get_usage_safe_when_active_count_raises(monkeypatch): # Field omitted, but the rest of the payload is intact. assert "active_subagents" not in usage assert usage["model"] == "x" + + +def test_persist_model_switch_preserves_sibling_model_keys(tmp_path, monkeypatch): + """#48305: switching models from the TUI must NOT destroy sibling keys under + `model:` (model_slots, model_fallback, etc.). _persist_model_switch now uses + targeted save_config_value writes instead of rewriting the whole block.""" + import types + import yaml + import cli + + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text( + "model:\n" + " default: old-model\n" + " provider: openai\n" + " model_slots:\n" + " fast: gpt-5-mini\n" + " model_fallback:\n" + " - claude-haiku\n" + "agent:\n" + " system_prompt: keepme\n" + ) + # save_config_value() resolves the config path from cli._hermes_home, which + # is captured at import time — patch it directly (set_hermes_home_override + # does NOT affect this snapshot). + monkeypatch.setattr(cli, "_hermes_home", tmp_path) + + result = types.SimpleNamespace( + new_model="new-model", target_provider="anthropic", base_url=None + ) + server._persist_model_switch(result) + saved = yaml.safe_load(cfg_path.read_text()) + + # The switched fields updated... + assert saved["model"]["default"] == "new-model" + assert saved["model"]["provider"] == "anthropic" + # ...and the sibling keys SURVIVED (the bug was that they got wiped). + assert saved["model"]["model_slots"] == {"fast": "gpt-5-mini"} + assert saved["model"]["model_fallback"] == ["claude-haiku"] + assert saved["agent"]["system_prompt"] == "keepme" + + +def test_persist_model_switch_clears_stale_base_url(tmp_path, monkeypatch): + """#48305: switching from a custom endpoint (which set model.base_url) to a + provider with no base_url must CLEAR the stale base_url, not leave it + pointing at the old host.""" + import types + import yaml + import cli + + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text( + "model:\n" + " default: local-model\n" + " provider: custom:mylocal\n" + " base_url: http://localhost:1234/v1\n" + ) + monkeypatch.setattr(cli, "_hermes_home", tmp_path) + + # Switch to a native provider with no base_url. + result = types.SimpleNamespace( + new_model="claude-haiku", target_provider="anthropic", base_url=None + ) + server._persist_model_switch(result) + saved = yaml.safe_load(cfg_path.read_text()) + + assert saved["model"]["default"] == "claude-haiku" + assert saved["model"]["provider"] == "anthropic" + # Stale custom base_url must be cleared (null coalesces to absent on read). + assert not saved["model"].get("base_url"), saved["model"].get("base_url") diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 93ad41a2d..d882133fd 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -2301,21 +2301,23 @@ def _restart_slash_worker(sid: str, session: dict): def _persist_model_switch(result) -> None: - from hermes_cli.config import save_config + # Use targeted, atomic key writes (comment/ordering-preserving) instead of + # rewriting the whole `model:` block. A full-block rewrite via save_config() + # destroys sibling keys the user set under `model:` — `model_slots`, + # `model_fallback`, etc. — when switching models from the TUI (#48305). + from cli import save_config_value - cfg = _load_cfg() - model_cfg = cfg.get("model") - if not isinstance(model_cfg, dict): - model_cfg = {} - cfg["model"] = model_cfg - - model_cfg["default"] = result.new_model - model_cfg["provider"] = result.target_provider + save_config_value("model.default", result.new_model) + save_config_value("model.provider", result.target_provider) if result.base_url: - model_cfg["base_url"] = result.base_url + save_config_value("model.base_url", result.base_url) else: - model_cfg.pop("base_url", None) - save_config(cfg) + # Clear any stale base_url when switching to a provider that doesn't use + # one (e.g. custom endpoint -> native provider). Reads coalesce null to + # absent (`model_cfg.get("base_url") or ""`), so a null is equivalent to + # removal without needing a key-delete. Leaving the old value would + # route the new model at the previous custom host (#48305). + save_config_value("model.base_url", None) def _apply_model_switch(