From 2fe78d1ae31c2ae18edf1b97b7d0a1c9e77e9187 Mon Sep 17 00:00:00 2001 From: Evo Date: Sat, 20 Jun 2026 01:09:46 +0800 Subject: [PATCH 1/3] fix(gateway): persist inline-keyboard model-picker selections by default #49066 made /model text and the CLI picker persist to config.yaml by default, but the gateway (Telegram/Discord/Matrix) inline-keyboard picker callback stayed session-only. Mirror the text path's persist block so a tapped model survives across launches like a typed one. --- gateway/slash_commands.py | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index b2810096b2..afb5737151 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1152,7 +1152,7 @@ class GatewaySlashCommandsMixin: current_model=_cur_model, current_base_url=_cur_base_url, current_api_key=_cur_api_key, - is_global=False, + is_global=persist_global, explicit_provider=provider_slug, user_providers=user_provs, custom_providers=custom_provs, @@ -1216,6 +1216,34 @@ class GatewaySlashCommandsMixin: # stale cache signature to trigger a rebuild. _self._evict_cached_agent(_session_key) + # Persist to config (default) unless --session opted out, + # mirroring the text /model command path above so a picked + # model survives across sessions like a typed one (#49066). + if persist_global: + try: + if config_path.exists(): + with open(config_path, encoding="utf-8") as f: + _persist_cfg = yaml.safe_load(f) or {} + else: + _persist_cfg = {} + _raw_model = _persist_cfg.get("model") + if isinstance(_raw_model, dict): + _persist_model_cfg = _raw_model + elif isinstance(_raw_model, str) and _raw_model.strip(): + _persist_model_cfg = {"default": _raw_model.strip()} + _persist_cfg["model"] = _persist_model_cfg + else: + _persist_model_cfg = {} + _persist_cfg["model"] = _persist_model_cfg + _persist_model_cfg["default"] = result.new_model + _persist_model_cfg["provider"] = result.target_provider + if result.base_url: + _persist_model_cfg["base_url"] = result.base_url + from hermes_cli.config import save_config + save_config(_persist_cfg) + except Exception as e: + logger.warning("Failed to persist model switch: %s", e) + # Build confirmation text plabel = result.provider_label or result.target_provider lines = [t("gateway.model.switched", model=result.new_model)] @@ -1249,7 +1277,10 @@ class GatewaySlashCommandsMixin: if mi.has_cost_data(): lines.append(t("gateway.model.cost_label", cost=mi.format_cost())) lines.append(t("gateway.model.capabilities_label", capabilities=mi.format_capabilities())) - lines.append(t("gateway.model.session_only_hint")) + if persist_global: + lines.append(t("gateway.model.saved_global")) + else: + lines.append(t("gateway.model.session_only_hint")) return "\n".join(lines) metadata = self._thread_metadata_for_source(source, self._reply_anchor_for_event(event)) From 10fea06c19df7f6e4639043dd9f175b64e0d198d Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 20 Jun 2026 02:35:02 +0530 Subject: [PATCH 2/3] test(gateway): cover inline-keyboard model-picker persistence Add regression coverage for the picker persist fix: drive the real _handle_model_command with a fake picker-capable adapter that captures the on_model_selected callback, fire a 'tap', and assert config.yaml is written (bare /model), left untouched (--session), and that a flat-string model: is coerced to a nested dict on a tap. Mutation-checked: the persist and coercion assertions fail on pre-fix slash_commands.py and pass on the fix. --- tests/gateway/test_model_picker_persist.py | 176 +++++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 tests/gateway/test_model_picker_persist.py diff --git a/tests/gateway/test_model_picker_persist.py b/tests/gateway/test_model_picker_persist.py new file mode 100644 index 0000000000..0ff57f4bf3 --- /dev/null +++ b/tests/gateway/test_model_picker_persist.py @@ -0,0 +1,176 @@ +"""Regression tests for gateway inline-keyboard model-picker persistence. + +#49066 made the typed ``/model `` command persist the selected model to +``config.yaml`` by default. But the inline-keyboard picker callback +(``_on_model_selected`` in ``gateway/slash_commands.py``) was left session-only: +it hard-coded ``is_global=False`` and never wrote ``config.yaml``, so *tapping* a +model in the Telegram/Discord picker silently reverted on the next launch while +*typing* the same model persisted — a contradiction the same PR introduced. + +After the fix (#49176), the picker callback honors the resolved +``persist_global`` (defaults to ``True``, still respects ``--session``) and runs +the same read-modify-write block the text path uses, so a tapped model survives +across sessions like a typed one. + +These tests drive the real ``_handle_model_command`` with a fake picker-capable +adapter that captures the ``on_model_selected`` callback, then invoke that +callback and assert ``config.yaml`` is (or isn't) updated — exercising the exact +closure the PR changed, against a real temp ``HERMES_HOME``. +""" + +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 _FakePickerResult: + success = True + + +class _FakePickerAdapter: + """Minimal adapter that looks picker-capable and captures the callback. + + ``_handle_model_command`` gates the picker path on + ``getattr(type(adapter), "send_model_picker", None) is not None``, so the + method must exist on the class, not just the instance. + """ + + def __init__(self): + self.captured_callback = None + + async def send_model_picker(self, *, on_model_selected, **kwargs): + # Stash the closure the handler built so the test can fire a "tap". + self.captured_callback = on_model_selected + return _FakePickerResult() + + +def _make_runner(adapter): + runner = object.__new__(GatewayRunner) + runner.adapters = {Platform.TELEGRAM: adapter} + 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(): + """A successful ModelSwitchResult that bypasses real provider resolution.""" + from hermes_cli.model_switch import ModelSwitchResult + + return ModelSwitchResult( + success=True, + new_model="gpt-5.5", + target_provider="openrouter", + provider_changed=True, + api_key="sk-test", + base_url="https://openrouter.ai/api/v1", + api_mode="chat_completions", + provider_label="OpenRouter", + is_global=True, + ) + + +def _setup_isolated_home(tmp_path, monkeypatch, model_yaml_value): + """Write a config.yaml with the given ``model:`` value and stub heavy bits.""" + 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.switch_model", + lambda **kw: _fake_switch_result(), + ) + # save_config writes to ``get_hermes_home() / config.yaml`` — point it here. + monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: hermes_home) + monkeypatch.setattr("hermes_cli.config.get_hermes_home", lambda: hermes_home) + return cfg_path + + +async def _drive_picker(runner, event): + """Run the handler (which sends the picker) then fire the captured tap.""" + sent = await runner._handle_model_command(event) + # Bare /model returns None (picker sent); the adapter captured the callback. + assert sent is None + adapter = runner.adapters[Platform.TELEGRAM] + assert adapter.captured_callback is not None, "picker callback was not wired" + # Simulate the user tapping "gpt-5.5" under the openrouter provider. + return await adapter.captured_callback("12345", "gpt-5.5", "openrouter") + + +@pytest.mark.asyncio +async def test_picker_tap_persists_by_default(tmp_path, monkeypatch): + """Tapping a model in the picker (bare /model) persists to config.yaml, + matching the typed ``/model`` default — this is the #49176 fix.""" + adapter = _FakePickerAdapter() + cfg_path = _setup_isolated_home( + tmp_path, monkeypatch, {"default": "old-model", "provider": "openai-codex"} + ) + + confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model")) + + assert confirmation is not None + assert "gpt-5.5" in confirmation + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert written["model"]["default"] == "gpt-5.5" + assert written["model"]["provider"] == "openrouter" + assert written["model"]["base_url"] == "https://openrouter.ai/api/v1" + + +@pytest.mark.asyncio +async def test_picker_tap_session_flag_does_not_persist(tmp_path, monkeypatch): + """``/model --session`` then a picker tap stays in-memory only — config + untouched.""" + adapter = _FakePickerAdapter() + cfg_path = _setup_isolated_home( + tmp_path, monkeypatch, {"default": "old-model", "provider": "openai-codex"} + ) + + confirmation = await _drive_picker( + _make_runner(adapter), _make_event("/model --session") + ) + + assert confirmation is not None + assert "gpt-5.5" in confirmation + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + # Config untouched — the session override is in-memory only. + assert written["model"]["default"] == "old-model" + assert written["model"]["provider"] == "openai-codex" + + +@pytest.mark.asyncio +async def test_picker_tap_coerces_flat_string_model(tmp_path, monkeypatch): + """A flat-string ``model:`` in config.yaml is coerced to a nested dict on a + picker tap (the same scalar-``model:`` guard the text path has), instead of + raising ``TypeError`` on assignment.""" + adapter = _FakePickerAdapter() + cfg_path = _setup_isolated_home(tmp_path, monkeypatch, "deepseek-v4-flash") + + 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 isinstance(written["model"], dict), ( + "model: should be coerced to a dict, got %r" % (written["model"],) + ) + assert written["model"]["default"] == "gpt-5.5" + assert written["model"]["provider"] == "openrouter" From 2099c7b531ced8287f55ef150211e0e92131d060 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Sat, 20 Jun 2026 02:46:01 +0530 Subject: [PATCH 3/3] test(gateway): make picker-persist tests hermetic and parametrized Simplify pass on the picker-persist coverage: - Stub list_picker_providers + resolve_display_context_length so the tests no longer make real outbound HTTP calls (OpenRouter catalog + Ollama /api/show) during picker setup and confirmation rendering. Runtime drops from ~11s to ~0.4s and the tests are now deterministic. - Collapse the two positive persist cases into one parametrize over the config seed (nested-dict vs flat-string), asserting the nested-dict invariant in both. - Assert the in-memory session override is applied in the --session case, closing a 'passes for the wrong reason' gap (config untouched AND the switch still took effect). - _FakePickerResult -> types.SimpleNamespace. Mutation re-checked on the final test: both persist cases fail on pre-fix slash_commands.py; the --session case passes on both. --- tests/gateway/test_model_picker_persist.py | 87 +++++++++++++--------- 1 file changed, 53 insertions(+), 34 deletions(-) diff --git a/tests/gateway/test_model_picker_persist.py b/tests/gateway/test_model_picker_persist.py index 0ff57f4bf3..ff74fd53de 100644 --- a/tests/gateway/test_model_picker_persist.py +++ b/tests/gateway/test_model_picker_persist.py @@ -18,6 +18,8 @@ callback and assert ``config.yaml`` is (or isn't) updated — exercising the exa closure the PR changed, against a real temp ``HERMES_HOME``. """ +import types + import yaml import pytest @@ -27,10 +29,6 @@ from gateway.run import GatewayRunner from gateway.session import SessionSource -class _FakePickerResult: - success = True - - class _FakePickerAdapter: """Minimal adapter that looks picker-capable and captures the callback. @@ -45,7 +43,7 @@ class _FakePickerAdapter: async def send_model_picker(self, *, on_model_selected, **kwargs): # Stash the closure the handler built so the test can fire a "tap". self.captured_callback = on_model_selected - return _FakePickerResult() + return types.SimpleNamespace(success=True) def _make_runner(adapter): @@ -96,10 +94,30 @@ def _setup_isolated_home(tmp_path, monkeypatch, model_yaml_value): monkeypatch.setattr(gateway_run, "_hermes_home", hermes_home) monkeypatch.setattr("agent.models_dev.fetch_models_dev", lambda: {}) + # The picker-setup path calls list_picker_providers, which otherwise hits + # the network (OpenRouter model catalog). Stub it to a minimal list — these + # tests capture and fire the on_model_selected callback and don't assert on + # picker contents. The handler imports it as a local alias at call time, so + # patching the source-module attribute takes effect. + monkeypatch.setattr( + "hermes_cli.model_switch.list_picker_providers", + lambda **kw: [{"slug": "openrouter", "name": "OpenRouter", "models": ["gpt-5.5"]}], + ) + # switch_model is imported as a local alias inside the handler + # (`from hermes_cli.model_switch import switch_model as _switch_model`), + # so patching the source-module attribute takes effect at call time. monkeypatch.setattr( "hermes_cli.model_switch.switch_model", lambda **kw: _fake_switch_result(), ) + # The confirmation builder resolves context length for display, which + # otherwise makes real outbound HTTP calls (Ollama /api/show + the + # OpenRouter models catalog). Stub it — these tests don't assert on the + # displayed context, and the closure imports it lazily from this module. + monkeypatch.setattr( + "hermes_cli.model_switch.resolve_display_context_length", + lambda *a, **k: 272000, + ) # save_config writes to ``get_hermes_home() / config.yaml`` — point it here. monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: hermes_home) monkeypatch.setattr("hermes_cli.config.get_hermes_home", lambda: hermes_home) @@ -118,19 +136,33 @@ async def _drive_picker(runner, event): @pytest.mark.asyncio -async def test_picker_tap_persists_by_default(tmp_path, monkeypatch): +@pytest.mark.parametrize( + "seed_model", + [ + # Already-nested dict (common case). + {"default": "old-model", "provider": "openai-codex"}, + # Flat-string model: must be coerced to a nested dict on a tap (same + # scalar-``model:`` guard the text path has) instead of raising + # ``TypeError`` on assignment. + "deepseek-v4-flash", + ], + ids=["nested-dict", "flat-string"], +) +async def test_picker_tap_persists_by_default(tmp_path, monkeypatch, seed_model): """Tapping a model in the picker (bare /model) persists to config.yaml, - matching the typed ``/model`` default — this is the #49176 fix.""" + matching the typed ``/model`` default — this is the #49176 fix. The written + ``model:`` must always end up a nested dict regardless of the seed shape.""" adapter = _FakePickerAdapter() - cfg_path = _setup_isolated_home( - tmp_path, monkeypatch, {"default": "old-model", "provider": "openai-codex"} - ) + cfg_path = _setup_isolated_home(tmp_path, monkeypatch, seed_model) confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model")) assert confirmation is not None assert "gpt-5.5" in confirmation written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert isinstance(written["model"], dict), ( + "model: should be coerced to a dict, got %r" % (written["model"],) + ) assert written["model"]["default"] == "gpt-5.5" assert written["model"]["provider"] == "openrouter" assert written["model"]["base_url"] == "https://openrouter.ai/api/v1" @@ -139,38 +171,25 @@ async def test_picker_tap_persists_by_default(tmp_path, monkeypatch): @pytest.mark.asyncio async def test_picker_tap_session_flag_does_not_persist(tmp_path, monkeypatch): """``/model --session`` then a picker tap stays in-memory only — config - untouched.""" + untouched, but the in-memory session override must still be applied (the + switch worked, it just wasn't persisted).""" adapter = _FakePickerAdapter() cfg_path = _setup_isolated_home( tmp_path, monkeypatch, {"default": "old-model", "provider": "openai-codex"} ) + runner = _make_runner(adapter) - confirmation = await _drive_picker( - _make_runner(adapter), _make_event("/model --session") - ) + confirmation = await _drive_picker(runner, _make_event("/model --session")) assert confirmation is not None assert "gpt-5.5" in confirmation + # The session override IS applied in-memory (proves the path didn't no-op). + assert runner._session_model_overrides, "session override should be set" + assert any( + ov.get("model") == "gpt-5.5" + for ov in runner._session_model_overrides.values() + ) + # But config.yaml is untouched — the override is in-memory only. written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) - # Config untouched — the session override is in-memory only. assert written["model"]["default"] == "old-model" assert written["model"]["provider"] == "openai-codex" - - -@pytest.mark.asyncio -async def test_picker_tap_coerces_flat_string_model(tmp_path, monkeypatch): - """A flat-string ``model:`` in config.yaml is coerced to a nested dict on a - picker tap (the same scalar-``model:`` guard the text path has), instead of - raising ``TypeError`` on assignment.""" - adapter = _FakePickerAdapter() - cfg_path = _setup_isolated_home(tmp_path, monkeypatch, "deepseek-v4-flash") - - 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 isinstance(written["model"], dict), ( - "model: should be coerced to a dict, got %r" % (written["model"],) - ) - assert written["model"]["default"] == "gpt-5.5" - assert written["model"]["provider"] == "openrouter"