From 398634eeda15fc372d73be9f5fddf88f36f51660 Mon Sep 17 00:00:00 2001 From: joaomarcos Date: Wed, 8 Jul 2026 11:52:12 -0300 Subject: [PATCH] fix(cli,gateway): sync base_url/api_mode on global model switch persist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Same bug family as #47828, at the config-persistence layer instead of the in-memory agent layer: - cli.py (#25106): the --global /model handlers (both the typed-name path in _handle_model_switch and the picker path in _apply_model_switch_result) wrote model.default/model.provider to config.yaml but never touched base_url/api_mode at all. A provider switch left the OLD endpoint on disk; the next launch reconnected to the previous provider's host under the new model name. - gateway/slash_commands.py (#25107): both persist-global blocks (the picker-tap callback and the typed /model --global path) guarded the write with two INDEPENDENT ifs — `if result.base_url: ...` and `if target_provider != "custom": clear_model_endpoint_credentials(...)`. For named providers the second if always cleared stale values, masking the bug. For a custom provider with an empty resolved base_url, neither branch fired, so the previous custom endpoint's base_url/api_key/ api_mode survived untouched in config.yaml. Fix: explicit set-if-truthy/clear-if-falsy for base_url and api_mode at all four call sites, matching the already-correct pattern in tui_gateway/server.py:_persist_model_switch (fixed for #48305). Co-Authored-By: Claude Sonnet 5 --- cli.py | 12 ++ gateway/slash_commands.py | 29 ++- .../test_25107_stale_base_url_api_mode.py | 194 ++++++++++++++++++ ...lobal_switch_persists_base_url_api_mode.py | 120 +++++++++++ 4 files changed, 353 insertions(+), 2 deletions(-) create mode 100644 tests/gateway/test_25107_stale_base_url_api_mode.py create mode 100644 tests/hermes_cli/test_25106_global_switch_persists_base_url_api_mode.py diff --git a/cli.py b/cli.py index b7fe3261c2ec..6cb6345f4be4 100644 --- a/cli.py +++ b/cli.py @@ -7979,6 +7979,14 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): save_config_value("model.default", result.new_model) if result.provider_changed: save_config_value("model.provider", result.target_provider) + # base_url/api_mode were previously never persisted here, so a + # global switch left the OLD provider's endpoint/wire-protocol in + # config.yaml. result.base_url/api_mode are always freshly + # resolved for the target provider (see model_switch.py), so sync + # them every time; None clears a value the new provider doesn't + # need (#25106). + save_config_value("model.base_url", result.base_url or None) + save_config_value("model.api_mode", result.api_mode or None) _cprint(" Saved to config.yaml (--global)") else: _cprint(" (session only — add --global to persist)") @@ -8291,6 +8299,10 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): save_config_value("model.default", result.new_model) if result.provider_changed: save_config_value("model.provider", result.target_provider) + # See _apply_model_switch_result above for why base_url/api_mode + # must be synced on every global switch (#25106). + save_config_value("model.base_url", result.base_url or None) + save_config_value("model.api_mode", result.api_mode or None) _cprint(" Saved to config.yaml") else: _cprint(" (session only — add --global to persist)") diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 9223689bd32a..77ace8f0e96b 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1723,9 +1723,24 @@ class GatewaySlashCommandsMixin: _persist_cfg["model"] = _persist_model_cfg _persist_model_cfg["default"] = result.new_model _persist_model_cfg["provider"] = result.target_provider + # Named providers always resolve base_url/api_mode fresh, + # so any leftover is cleared unconditionally below. Custom + # providers have no registry entry to re-derive from, so + # they need an explicit set-or-clear here — the previous + # lone `if result.base_url:` left a stale base_url behind + # when switching to a custom provider whose resolver + # returned an empty base_url (#25107). + _is_custom_target = str(result.target_provider or "").strip().lower() == "custom" if result.base_url: _persist_model_cfg["base_url"] = result.base_url - if str(result.target_provider or "").strip().lower() != "custom": + elif _is_custom_target: + _persist_model_cfg.pop("base_url", None) + if _is_custom_target: + if result.api_mode: + _persist_model_cfg["api_mode"] = result.api_mode + else: + _persist_model_cfg.pop("api_mode", None) + else: clear_model_endpoint_credentials(_persist_model_cfg, clear_base_url=True) from hermes_cli.config import save_config save_config(_persist_cfg) @@ -1988,9 +2003,19 @@ class GatewaySlashCommandsMixin: cfg["model"] = model_cfg model_cfg["default"] = result.new_model model_cfg["provider"] = result.target_provider + # See the picker handler above for why custom providers need an + # explicit set-or-clear instead of the old lone truthy check (#25107). + _is_custom_target = str(result.target_provider or "").strip().lower() == "custom" if result.base_url: model_cfg["base_url"] = result.base_url - if str(result.target_provider or "").strip().lower() != "custom": + elif _is_custom_target: + model_cfg.pop("base_url", None) + if _is_custom_target: + if result.api_mode: + model_cfg["api_mode"] = result.api_mode + else: + model_cfg.pop("api_mode", None) + else: clear_model_endpoint_credentials(model_cfg, clear_base_url=True) from hermes_cli.config import save_config save_config(cfg) diff --git a/tests/gateway/test_25107_stale_base_url_api_mode.py b/tests/gateway/test_25107_stale_base_url_api_mode.py new file mode 100644 index 000000000000..781fc3c4bf68 --- /dev/null +++ b/tests/gateway/test_25107_stale_base_url_api_mode.py @@ -0,0 +1,194 @@ +"""Regression tests for #25107: gateway /model switch left a stale +``base_url``/never persisted ``api_mode`` in config.yaml when switching to a +custom provider whose resolver returned an empty ``base_url``. + +Root cause: both the picker-tap path (``_on_model_selected`` in +``gateway/slash_commands.py``) and the typed ``/model X --global`` path +(``_finish_switch``) guarded the persist block with two *independent* +``if``s: + + if result.base_url: + model_cfg["base_url"] = result.base_url + if target_provider != "custom": + clear_model_endpoint_credentials(model_cfg, clear_base_url=True) + +For a NAMED provider the second ``if`` always clears any stale value, so the +bug was invisible there. But for a ``custom`` provider with an empty +resolved ``base_url``, NEITHER branch fires: the old base_url survives +untouched, and ``api_mode`` was never written to ``model_cfg`` at all in +either branch (only present in the in-memory session override). + +Fix: an explicit set-or-clear for both fields when the target provider is +custom, so a genuine switch always leaves config.yaml coherent with the +resolved result. +""" + +import types + +import yaml +import pytest + +from gateway.config import Platform +from gateway.platforms.base import MessageEvent, MessageType +from gateway.run import GatewayRunner +from gateway.session import SessionSource + + +class _FakePickerAdapter: + def __init__(self): + self.captured_callback = None + + async def send_model_picker(self, *, on_model_selected, **kwargs): + self.captured_callback = on_model_selected + return types.SimpleNamespace(success=True) + + +def _make_runner(adapter=None): + runner = object.__new__(GatewayRunner) + runner.adapters = {Platform.TELEGRAM: adapter} if adapter else {} + runner._voice_mode = {} + runner._session_model_overrides = {} + runner._running_agents = {} + return runner + + +def _make_event(text): + return MessageEvent( + text=text, + message_type=MessageType.TEXT, + source=SessionSource(platform=Platform.TELEGRAM, chat_id="12345", chat_type="dm"), + ) + + +def _fake_switch_result(*, base_url="", api_mode=""): + from hermes_cli.model_switch import ModelSwitchResult + + return ModelSwitchResult( + success=True, + new_model="local-llama", + target_provider="custom", + provider_changed=True, + api_key="sk-local", + base_url=base_url, + api_mode=api_mode, + provider_label="Custom", + is_global=True, + ) + + +def _setup_isolated_home(tmp_path, monkeypatch, model_yaml_value, *, base_url="", api_mode=""): + import gateway.run as gateway_run + + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + cfg_path = hermes_home / "config.yaml" + cfg_path.write_text( + yaml.safe_dump({"model": model_yaml_value, "providers": {}}), + encoding="utf-8", + ) + + monkeypatch.setattr(gateway_run, "_hermes_home", hermes_home) + monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + monkeypatch.setattr( + "hermes_cli.model_switch.list_picker_providers", + lambda **kw: [{"slug": "custom", "name": "Custom", "models": ["local-llama"]}], + ) + monkeypatch.setattr( + "hermes_cli.model_switch.switch_model", + lambda **kw: _fake_switch_result(base_url=base_url, api_mode=api_mode), + ) + monkeypatch.setattr( + "hermes_cli.model_switch.resolve_display_context_length", + lambda *a, **k: 8192, + ) + monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: hermes_home) + monkeypatch.setattr("hermes_cli.config.get_hermes_home", lambda: hermes_home) + return cfg_path + + +_STALE_MODEL_CFG = { + "default": "old-custom-model", + "provider": "custom", + "base_url": "https://old-stale-endpoint.example/v1", + "api_key": "sk-stale", + "api_mode": "anthropic_messages", +} + + +# --------------------------------------------------------------------------- +# Typed `/model X --global` path (_finish_switch) +# --------------------------------------------------------------------------- + + +@pytest.mark.asyncio +async def test_typed_switch_to_custom_clears_stale_base_url_and_api_mode(tmp_path, monkeypatch): + """Switching to a custom provider whose resolver returns no base_url/ + api_mode must clear the previous custom endpoint's leftovers, not keep + routing at the old host/protocol (#25107).""" + cfg_path = _setup_isolated_home(tmp_path, monkeypatch, dict(_STALE_MODEL_CFG)) + + result = await _make_runner()._handle_model_command( + _make_event("/model local-llama --provider custom --global") + ) + + assert result is not None + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert written["model"]["default"] == "local-llama" + assert "base_url" not in written["model"], ( + "stale base_url from the old custom endpoint must be cleared" + ) + assert "api_mode" not in written["model"], ( + "stale api_mode from the old custom endpoint must be cleared" + ) + + +@pytest.mark.asyncio +async def test_typed_switch_to_custom_persists_resolved_base_url_and_api_mode(tmp_path, monkeypatch): + """The normal case: a custom-provider switch that DOES resolve a fresh + base_url/api_mode must persist both (api_mode was never written here + before the fix).""" + cfg_path = _setup_isolated_home( + tmp_path, + monkeypatch, + dict(_STALE_MODEL_CFG), + base_url="https://new-endpoint.example/v1", + api_mode="anthropic_messages", + ) + + result = await _make_runner()._handle_model_command( + _make_event("/model local-llama --provider custom --global") + ) + + assert result is not None + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert written["model"]["base_url"] == "https://new-endpoint.example/v1" + assert written["model"]["api_mode"] == "anthropic_messages" + + +# --------------------------------------------------------------------------- +# Picker-tap path (_on_model_selected) +# --------------------------------------------------------------------------- + + +async def _drive_picker(runner, event): + sent = await runner._handle_model_command(event) + assert sent is None + adapter = runner.adapters[Platform.TELEGRAM] + assert adapter.captured_callback is not None, "picker callback was not wired" + return await adapter.captured_callback("12345", "local-llama", "custom") + + +@pytest.mark.asyncio +async def test_picker_tap_to_custom_clears_stale_base_url_and_api_mode(tmp_path, monkeypatch): + """Same bug, picker-tap path: tapping a custom-provider model with an + empty resolved base_url must clear the previous custom endpoint (#25107). + """ + adapter = _FakePickerAdapter() + cfg_path = _setup_isolated_home(tmp_path, monkeypatch, dict(_STALE_MODEL_CFG)) + + confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model")) + + assert confirmation is not None + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert "base_url" not in written["model"] + assert "api_mode" not in written["model"] diff --git a/tests/hermes_cli/test_25106_global_switch_persists_base_url_api_mode.py b/tests/hermes_cli/test_25106_global_switch_persists_base_url_api_mode.py new file mode 100644 index 000000000000..6a1e2642f5ef --- /dev/null +++ b/tests/hermes_cli/test_25106_global_switch_persists_base_url_api_mode.py @@ -0,0 +1,120 @@ +"""Regression tests for #25106: CLI `/model --global` never persisted +``model.base_url``/``model.api_mode`` to config.yaml, so a global provider +switch left the PREVIOUS provider's endpoint/wire-protocol on disk. The next +`hermes` launch re-read the stale base_url and routed the new model at the +old host. + +Both ``_handle_model_switch`` (typed ``/model ``) and +``_apply_model_switch_result`` (interactive picker) shared the same gap: the +persistence block wrote ``model.default``/``model.provider`` but never +touched ``base_url``/``api_mode`` at all. Fix: sync both on every global +switch, clearing to ``None`` when the resolved result doesn't need them — +mirroring the already-correct ``tui_gateway/server.py:_persist_model_switch`` +pattern (fixed for #48305). +""" + +from unittest.mock import MagicMock, patch + +import pytest + +from hermes_cli.model_switch import ModelSwitchResult + + +def _make_result(*, base_url="https://api.minimax.io/v1", api_mode="chat_completions", provider_changed=True): + return ModelSwitchResult( + success=True, + new_model="MiniMax-M3", + target_provider="custom:minimax", + provider_changed=provider_changed, + api_key="sk-minimax", + base_url=base_url, + api_mode=api_mode, + warning_message="", + provider_label="MiniMax (custom)", + resolved_via_alias=False, + capabilities=None, + model_info=None, + is_global=True, + ) + + +class _StubCLI: + """Minimum attrs/methods `_handle_model_switch` reads or calls on self.""" + + agent = None + model = "old-model" + provider = "copilot" + requested_provider = "copilot" + api_key = "sk-old" + base_url = "https://api.githubcopilot.com" + api_mode = "chat_completions" + _explicit_api_key = "" + _explicit_base_url = "" + conversation_history = [] + _pending_model_switch_note = "" + + def _confirm_expensive_model_switch(self, result) -> bool: + return True + + def _open_model_picker(self, *a, **k): + raise AssertionError("picker should not open when a model name is given") + + +def _run_switch(monkeypatch, result, cmd="/model MiniMax-M3 --global"): + import cli as cli_mod + + monkeypatch.setattr(cli_mod, "_cprint", lambda *a, **k: None) + saved: dict[str, object] = {} + + def _fake_save(key, value): + saved[key] = value + + monkeypatch.setattr(cli_mod, "save_config_value", _fake_save) + monkeypatch.setattr("hermes_cli.model_switch.switch_model", lambda **kw: result) + monkeypatch.setattr( + "hermes_cli.inventory.load_picker_context", + lambda: (_ for _ in ()).throw(RuntimeError("no picker context in test")), + ) + cli_mod.HermesCLI._handle_model_switch(_StubCLI(), cmd) + return saved + + +def test_global_switch_persists_base_url_and_api_mode(monkeypatch): + """The core #25106 fix: a --global switch to a new provider/endpoint must + write the newly resolved base_url and api_mode, not just default/provider.""" + saved = _run_switch(monkeypatch, _make_result()) + + assert saved["model.default"] == "MiniMax-M3" + assert saved["model.provider"] == "custom:minimax" + assert saved["model.base_url"] == "https://api.minimax.io/v1" + assert saved["model.api_mode"] == "chat_completions" + + +def test_global_switch_clears_base_url_and_api_mode_when_unresolved(monkeypatch): + """When the resolver returns no base_url/api_mode for the new provider + (e.g. a named provider needing neither), any previous value must be + cleared (None) rather than silently left in config.yaml.""" + result = _make_result(base_url="", api_mode="") + saved = _run_switch(monkeypatch, result) + + assert saved["model.base_url"] is None + assert saved["model.api_mode"] is None + + +def test_session_only_switch_does_not_touch_config(monkeypatch): + """--session must not call save_config_value at all — persistence stays + entirely in-memory.""" + import cli as cli_mod + + monkeypatch.setattr(cli_mod, "_cprint", lambda *a, **k: None) + save_calls = [] + monkeypatch.setattr(cli_mod, "save_config_value", lambda *a, **k: save_calls.append(a)) + monkeypatch.setattr("hermes_cli.model_switch.switch_model", lambda **kw: _make_result()) + monkeypatch.setattr( + "hermes_cli.inventory.load_picker_context", + lambda: (_ for _ in ()).throw(RuntimeError("no picker context in test")), + ) + + cli_mod.HermesCLI._handle_model_switch(_StubCLI(), "/model MiniMax-M3 --session") + + assert save_calls == []