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
This commit is contained in:
parent
2187fd884c
commit
b85c460540
2 changed files with 91 additions and 17 deletions
|
|
@ -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")
|
||||
|
|
|
|||
|
|
@ -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(
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue