diff --git a/cli.py b/cli.py index e0a8676ce..52bfe6cdb 100644 --- a/cli.py +++ b/cli.py @@ -6959,24 +6959,43 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): self._close_model_picker() def _handle_model_switch(self, cmd_original: str): - """Handle /model command — switch model for this session. + """Handle /model command — switch model. Supports: /model — show current model + usage hints - /model — switch for this session only - /model --global — switch and persist to config.yaml + /model — switch model (persists by default) + /model --session — switch for this session only + /model --global — switch and persist (explicit) /model --provider — switch provider + model /model --provider — switch to provider, auto-detect model + + Persistence defaults to on (``model.persist_switch_by_default`` in + config.yaml, default True). Use ``--session`` for a one-off switch. """ - from hermes_cli.model_switch import switch_model, parse_model_flags + from hermes_cli.model_switch import ( + switch_model, + parse_model_flags, + resolve_persist_behavior, + ) from hermes_cli.providers import get_label # Parse args from the original command parts = cmd_original.split(None, 1) # split off '/model' raw_args = parts[1].strip() if len(parts) > 1 else "" - # Parse --provider, --global, and --refresh flags - model_input, explicit_provider, persist_global, force_refresh = parse_model_flags(raw_args) + # Parse --provider, --global, --session, and --refresh flags + ( + model_input, + explicit_provider, + is_global_flag, + force_refresh, + is_session, + ) = parse_model_flags(raw_args) + # Resolve the effective persistence once: --session overrides the + # config-gated default, --global forces persist, otherwise defer to + # model.persist_switch_by_default (defaults to True so /model survives + # across sessions). + persist_global = resolve_persist_behavior(is_global_flag, is_session) # --refresh: wipe the on-disk picker cache before building the # provider list. Forces a live re-fetch of every authed provider's @@ -7024,7 +7043,8 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): if not providers: _cprint(" No authenticated providers found.") _cprint("") - _cprint(" /model switch model") + _cprint(" /model switch model (persists)") + _cprint(" /model --session switch for this session only") _cprint(" /model --provider switch provider") _cprint(" /model --refresh re-fetch live model lists") return @@ -7144,7 +7164,7 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): save_config_value("model.default", result.new_model) if result.provider_changed: save_config_value("model.provider", result.target_provider) - _cprint(" Saved to config.yaml (--global)") + _cprint(" Saved to config.yaml") else: _cprint(" (session only — add --global to persist)") @@ -11917,7 +11937,13 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin): # --- /model picker modal --- if self._model_picker_state: try: - self._handle_model_picker_selection() + # Picker selections persist by default (same default as + # /model ); honour model.persist_switch_by_default. + from hermes_cli.model_switch import resolve_persist_behavior + + self._handle_model_picker_selection( + persist_global=resolve_persist_behavior(False, False) + ) except Exception as _exc: _cprint(f" ✗ Model selection failed: {_exc}") self._close_model_picker() diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 04c3f4ca8..4b25d96fd 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1030,12 +1030,13 @@ class GatewaySlashCommandsMixin: ) async def _handle_model_command(self, event: MessageEvent) -> Optional[str]: - """Handle /model command — switch model for this session. + """Handle /model command — switch model. Supports: /model — interactive picker (Telegram/Discord) or text list - /model — switch for this session only - /model --global — switch and persist to config.yaml + /model — switch model (persists by default) + /model --session — switch for this session only + /model --global — switch and persist (explicit) /model --provider — switch provider + model /model --provider — switch to provider, auto-detect model """ @@ -1043,6 +1044,7 @@ class GatewaySlashCommandsMixin: import yaml from hermes_cli.model_switch import ( switch_model as _switch_model, parse_model_flags, + resolve_persist_behavior, list_authenticated_providers, list_picker_providers, ) @@ -1050,8 +1052,15 @@ class GatewaySlashCommandsMixin: raw_args = event.get_command_args().strip() - # Parse --provider, --global, and --refresh flags - model_input, explicit_provider, persist_global, force_refresh = parse_model_flags(raw_args) + # Parse --provider, --global, --session, and --refresh flags + ( + model_input, + explicit_provider, + is_global_flag, + force_refresh, + is_session, + ) = parse_model_flags(raw_args) + persist_global = resolve_persist_behavior(is_global_flag, is_session) # --refresh: bust the disk cache so the picker shows live data. if force_refresh: @@ -1362,7 +1371,7 @@ class GatewaySlashCommandsMixin: # override rather than relying on cache signature mismatch detection. self._evict_cached_agent(session_key) - # Persist to config if --global + # Persist to config (default) unless --session opted out if persist_global: try: if config_path.exists(): diff --git a/hermes_cli/commands.py b/hermes_cli/commands.py index 514e7f659..42e51f299 100644 --- a/hermes_cli/commands.py +++ b/hermes_cli/commands.py @@ -123,8 +123,8 @@ COMMAND_REGISTRY: list[CommandDef] = [ # Configuration CommandDef("config", "Show current configuration", "Configuration", cli_only=True), - CommandDef("model", "Switch model for this session", "Configuration", - args_hint="[model] [--provider name] [--global] [--refresh]"), + CommandDef("model", "Switch model (persists by default)", "Configuration", + args_hint="[model] [--provider name] [--global|--session] [--refresh]"), CommandDef("codex-runtime", "Toggle codex app-server runtime for OpenAI/Codex models", "Configuration", aliases=("codex_runtime",), args_hint="[auto|codex_app_server]"), diff --git a/hermes_cli/model_switch.py b/hermes_cli/model_switch.py index 2ed5b1479..7f6fe70d9 100644 --- a/hermes_cli/model_switch.py +++ b/hermes_cli/model_switch.py @@ -299,34 +299,46 @@ class ModelSwitchResult: # Flag parsing # --------------------------------------------------------------------------- -def parse_model_flags(raw_args: str) -> tuple[str, str, bool, bool]: - """Parse --provider, --global, and --refresh flags from /model command args. +def parse_model_flags(raw_args: str) -> tuple[str, str, bool, bool, bool]: + """Parse --provider, --global, --session, and --refresh flags from /model command args. - Returns (model_input, explicit_provider, is_global, force_refresh). + Returns ``(model_input, explicit_provider, is_global, force_refresh, is_session)``. + + ``is_global`` and ``is_session`` are independent flag presences; the + *effective* persistence decision is resolved by + :func:`resolve_persist_behavior` so the config-gated default + (``model.persist_switch_by_default``) is applied in one place. Examples:: - "sonnet" -> ("sonnet", "", False, False) - "sonnet --global" -> ("sonnet", "", True, False) - "sonnet --provider anthropic" -> ("sonnet", "anthropic", False, False) - "--provider my-ollama" -> ("", "my-ollama", False, False) - "--refresh" -> ("", "", False, True) - "sonnet --provider anthropic --global" -> ("sonnet", "anthropic", True, False) + "sonnet" -> ("sonnet", "", False, False, False) + "sonnet --global" -> ("sonnet", "", True, False, False) + "sonnet --session" -> ("sonnet", "", False, False, True) + "sonnet --provider anthropic" -> ("sonnet", "anthropic", False, False, False) + "--provider my-ollama" -> ("", "my-ollama", False, False, False) + "--refresh" -> ("", "", False, True, False) + "sonnet --provider anthropic --global" -> ("sonnet", "anthropic", True, False, False) """ is_global = False explicit_provider = "" force_refresh = False + is_session = False # Normalize Unicode dashes (Telegram/iOS auto-converts -- to em/en dash) # A single Unicode dash before a flag keyword becomes "--" import re as _re - raw_args = _re.sub(r'[\u2012\u2013\u2014\u2015](provider|global|refresh)', r'--\1', raw_args) + raw_args = _re.sub(r'[\u2012\u2013\u2014\u2015](provider|global|session|refresh)', r'--\1', raw_args) # Extract --global if "--global" in raw_args: is_global = True raw_args = raw_args.replace("--global", "").strip() + # Extract --session (explicit session-only; overrides the persist default) + if "--session" in raw_args: + is_session = True + raw_args = raw_args.replace("--session", "").strip() + # Extract --refresh (bust the model picker disk cache before listing) if "--refresh" in raw_args: force_refresh = True @@ -345,7 +357,37 @@ def parse_model_flags(raw_args: str) -> tuple[str, str, bool, bool]: i += 1 model_input = " ".join(filtered).strip() - return (model_input, explicit_provider, is_global, force_refresh) + return (model_input, explicit_provider, is_global, force_refresh, is_session) + + +def resolve_persist_behavior(is_global: bool, is_session: bool) -> bool: + """Decide whether a ``/model`` switch should persist to ``config.yaml``. + + Resolution order: + + 1. ``--session`` explicitly opts out → ``False`` (this session only). + 2. ``--global`` explicitly opts in → ``True``. + 3. Otherwise defer to ``model.persist_switch_by_default`` in + ``config.yaml`` (defaults to ``True``, so a plain ``/model `` + survives across sessions — the behavior users expect). + + The config read is defensive: on a fresh install ``model`` may be a + flat string rather than a dict, in which case the built-in default + (``True``) applies. + """ + if is_session: + return False + if is_global: + return True + try: + from hermes_cli.config import load_config + + model_cfg = load_config().get("model") + if isinstance(model_cfg, dict): + return bool(model_cfg.get("persist_switch_by_default", True)) + except Exception: + pass + return True # --------------------------------------------------------------------------- diff --git a/tests/gateway/test_model_command_flat_string_config.py b/tests/gateway/test_model_command_flat_string_config.py index 38d6ea11d..9934d9806 100644 --- a/tests/gateway/test_model_command_flat_string_config.py +++ b/tests/gateway/test_model_command_flat_string_config.py @@ -156,3 +156,46 @@ async def test_model_global_persists_when_config_has_proper_dict_model(tmp_path, written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) assert written["model"]["default"] == "gpt-5.5" assert written["model"]["provider"] == "openrouter" + + +@pytest.mark.asyncio +async def test_model_no_flag_persists_by_default(tmp_path, monkeypatch): + """A plain ``/model X`` (no --global) now persists to config.yaml. + + This is the user-facing fix: switching models in one session survives + into the next without re-typing the switch every time. + """ + cfg_path = _setup_isolated_home( + tmp_path, + monkeypatch, + {"default": "old-model", "provider": "openai-codex"}, + ) + + result = await _make_runner()._handle_model_command( + _make_event("/model gpt-5.5") + ) + + assert result is not None + assert "gpt-5.5" in result + written = yaml.safe_load(cfg_path.read_text(encoding="utf-8")) + assert written["model"]["default"] == "gpt-5.5" + + +@pytest.mark.asyncio +async def test_model_session_flag_does_not_persist(tmp_path, monkeypatch): + """``/model X --session`` opts out of persistence even under the new default.""" + cfg_path = _setup_isolated_home( + tmp_path, + monkeypatch, + {"default": "old-model", "provider": "openai-codex"}, + ) + + result = await _make_runner()._handle_model_command( + _make_event("/model gpt-5.5 --session") + ) + + assert result is not None + assert "gpt-5.5" in result + 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" diff --git a/tests/hermes_cli/test_model_picker_expensive_confirm.py b/tests/hermes_cli/test_model_picker_expensive_confirm.py index b827be3c9..222968dae 100644 --- a/tests/hermes_cli/test_model_picker_expensive_confirm.py +++ b/tests/hermes_cli/test_model_picker_expensive_confirm.py @@ -55,10 +55,12 @@ def test_prompt_toolkit_model_picker_defers_confirmation_off_key_handler(monkeyp lambda *_args: captured.setdefault("ran_inline", True) ) - _bound(cli_mod.HermesCLI._handle_model_picker_selection, self_)() + # The key handler now resolves persistence via resolve_persist_behavior, + # which defaults to True (persist-by-default). Simulate that call. + _bound(cli_mod.HermesCLI._handle_model_picker_selection, self_)(persist_global=True) assert self_._model_picker_state is None assert captured["started"] is True assert captured["daemon"] is True - assert captured["args"] == (result, False) + assert captured["args"] == (result, True) assert "ran_inline" not in captured diff --git a/tests/hermes_cli/test_model_switch_persist_default.py b/tests/hermes_cli/test_model_switch_persist_default.py new file mode 100644 index 000000000..912bd7afe --- /dev/null +++ b/tests/hermes_cli/test_model_switch_persist_default.py @@ -0,0 +1,122 @@ +"""Tests for persist-by-default model switching. + +Covers: +- ``parse_model_flags`` recognises ``--session`` (and keeps ``--global``). +- ``resolve_persist_behavior`` applies the config-gated default and the + ``--session`` / ``--global`` overrides. +- The default (no flags) persists, which is the user-facing fix: a plain + ``/model `` survives across sessions. +""" + +from unittest.mock import patch + +from hermes_cli.model_switch import parse_model_flags, resolve_persist_behavior + + +# --------------------------------------------------------------------------- +# parse_model_flags +# --------------------------------------------------------------------------- + + +class TestParseModelFlagsSession: + def test_no_flags(self): + assert parse_model_flags("sonnet") == ("sonnet", "", False, False, False) + + def test_global_flag(self): + assert parse_model_flags("sonnet --global") == ("sonnet", "", True, False, False) + + def test_session_flag(self): + assert parse_model_flags("sonnet --session") == ( + "sonnet", + "", + False, + False, + True, + ) + + def test_session_with_provider(self): + assert parse_model_flags("sonnet --provider anthropic --session") == ( + "sonnet", + "anthropic", + False, + False, + True, + ) + + def test_refresh_flag_still_parsed(self): + assert parse_model_flags("--refresh") == ("", "", False, True, False) + + def test_unicode_dash_session_normalized(self): + # Telegram/iOS auto-converts -- to en/em dashes. + assert parse_model_flags("sonnet \u2013session") == ( + "sonnet", + "", + False, + False, + True, + ) + + +# --------------------------------------------------------------------------- +# resolve_persist_behavior +# --------------------------------------------------------------------------- + + +class TestResolvePersistBehavior: + def test_session_flag_always_session_only(self): + # --session opts out even if the config default is True. + with _config({"model": {"persist_switch_by_default": True}}): + assert resolve_persist_behavior(False, True) is False + + def test_global_flag_always_persists(self): + # --global forces persist even if the config default is False. + with _config({"model": {"persist_switch_by_default": False}}): + assert resolve_persist_behavior(True, False) is True + + def test_default_persists_when_config_missing(self): + # No model section at all → built-in default (True). + with _config({}): + assert resolve_persist_behavior(False, False) is True + + def test_default_persists_when_key_true(self): + with _config({"model": {"persist_switch_by_default": True}}): + assert resolve_persist_behavior(False, False) is True + + def test_default_session_only_when_key_false(self): + with _config({"model": {"persist_switch_by_default": False}}): + assert resolve_persist_behavior(False, False) is False + + def test_default_when_model_is_flat_string(self): + # Fresh install: ``model: ""`` (not a dict) → built-in default True. + with _config({"model": ""}): + assert resolve_persist_behavior(False, False) is True + + def test_session_overrides_global_when_both_set(self): + # --session is the explicit opt-out and wins over --global. + with _config({"model": {"persist_switch_by_default": True}}): + assert resolve_persist_behavior(True, True) is False + + +# --------------------------------------------------------------------------- +# helper +# --------------------------------------------------------------------------- + + +class _config: + """Context manager that patches ``load_config`` to return a fixed dict.""" + + def __init__(self, cfg: dict): + self.cfg = cfg + + def __enter__(self): + self._patch = patch( + "hermes_cli.config.load_config", + return_value=self.cfg, + ) + # resolve_persist_behavior imports load_config lazily inside the + # function, so patching the source module is sufficient. + self._patch.start() + return self + + def __exit__(self, *exc): + self._patch.stop() diff --git a/tests/tui_gateway/test_make_agent_provider.py b/tests/tui_gateway/test_make_agent_provider.py index 9cd5b0d5f..94b606dbd 100644 --- a/tests/tui_gateway/test_make_agent_provider.py +++ b/tests/tui_gateway/test_make_agent_provider.py @@ -443,7 +443,9 @@ def test_apply_model_switch_does_not_leak_process_env(): with ( patch("hermes_cli.model_switch.parse_model_flags", - return_value=("glm-5.1", None, False, False)), + return_value=("glm-5.1", None, False, False, True)), + patch("hermes_cli.model_switch.resolve_persist_behavior", + return_value=False), patch("hermes_cli.model_switch.switch_model", return_value=_FakeResult()), patch("tui_gateway.server._emit"), patch("tui_gateway.server._restart_slash_worker"), diff --git a/tui_gateway/server.py b/tui_gateway/server.py index d65cdf493..1ea3331b8 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -2139,14 +2139,25 @@ def _apply_model_switch( *, confirm_expensive_model: bool = False, pin_session_override: bool = True, - parsed_flags: tuple[str, str, bool, bool] | None = None, + parsed_flags: tuple[str, str, bool, bool, bool] | None = None, ) -> dict: - from hermes_cli.model_switch import parse_model_flags, switch_model + from hermes_cli.model_switch import ( + parse_model_flags, + resolve_persist_behavior, + switch_model, + ) from hermes_cli.runtime_provider import resolve_runtime_provider if parsed_flags is None: parsed_flags = parse_model_flags(raw_input) - model_input, explicit_provider, persist_global, _force_refresh = parsed_flags + ( + model_input, + explicit_provider, + is_global_flag, + _force_refresh, + is_session, + ) = parsed_flags + persist_global = resolve_persist_behavior(is_global_flag, is_session) if not model_input: raise ValueError("model value required") @@ -7596,7 +7607,7 @@ def _(rid, params: dict) -> dict: from hermes_cli.model_switch import parse_model_flags parsed_flags = parse_model_flags(value) - _model_input, explicit_provider, _persist_global, _force_refresh = parsed_flags + _model_input, explicit_provider, _persist_global, _force_refresh, _is_session = parsed_flags if session.get("agent") is None and not explicit_provider.strip(): session_id = params.get("session_id", "") _start_agent_build(session_id, session)