mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:18:55 +00:00
fix(model): default /model switches to session scope everywhere
Flip the resolve_persist_behavior() fallback from persist-to-config to session-only. A plain /model <name> (typed or via any picker — CLI, TUI/Desktop, gateway) now affects only the current session; --global persists explicitly, and model.persist_switch_by_default: true restores the old opt-out behavior for users who want switches to stick. This is the root cause behind the recurring 'session switch applied globally' bug class (#61458, #63083, #58290, #61190): every surface funnels its no-flag default through this one function, so per-surface patches kept missing paths. Fixing the default fixes all surfaces at once: CLI typed + picker, TUI/Desktop config.set + slash, gateway typed + inline picker. Builds on liuhao1024's #58371 (--provider session scoping, cherry-picked as the previous commit) and supersedes the per-surface #61488.
This commit is contained in:
parent
0d6d73525d
commit
8b6fde3a35
8 changed files with 75 additions and 43 deletions
24
cli.py
24
cli.py
|
|
@ -8189,16 +8189,16 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
|||
|
||||
Supports:
|
||||
/model — show current model + usage hints
|
||||
/model <name> — switch model (persists by default)
|
||||
/model <name> — switch model (this session only)
|
||||
/model <name> --once — switch for the next turn only
|
||||
/model <name> --session — switch for this session only
|
||||
/model <name> --global — switch and persist (explicit)
|
||||
/model <name> --session — switch for this session only (explicit)
|
||||
/model <name> --global — switch and persist to config.yaml
|
||||
/model <name> --provider <provider> — switch provider + model
|
||||
/model --provider <provider> — switch to provider, auto-detect model
|
||||
|
||||
Persistence defaults to on (``model.persist_switch_by_default`` in
|
||||
config.yaml, default True). Use ``--session`` for this CLI session or
|
||||
``--once`` for the next turn only.
|
||||
Persistence defaults to off (``model.persist_switch_by_default`` in
|
||||
config.yaml, default False — switches are session-scoped). Use
|
||||
``--global`` to persist, or ``--once`` for the next turn only.
|
||||
"""
|
||||
from hermes_cli.model_switch import (
|
||||
switch_model,
|
||||
|
|
@ -8225,10 +8225,10 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
|||
if one_turn and not model_input and not explicit_provider:
|
||||
_cprint(" ✗ /model --once requires a model or provider.")
|
||||
return
|
||||
# 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).
|
||||
# Resolve the effective persistence once: --global forces persist,
|
||||
# --session/--once force session-scope, otherwise defer to
|
||||
# model.persist_switch_by_default (defaults to False so /model is
|
||||
# session-scoped unless the user opts in).
|
||||
persist_global = resolve_persist_behavior(
|
||||
is_global_flag, is_session, is_once=one_turn,
|
||||
explicit_provider=explicit_provider,
|
||||
|
|
@ -13160,8 +13160,8 @@ class HermesCLI(CLIAgentSetupMixin, CLICommandsMixin, CLIBillingMixin):
|
|||
# --- /model picker modal ---
|
||||
if self._model_picker_state:
|
||||
try:
|
||||
# Picker selections persist by default (same default as
|
||||
# /model <name>); honour model.persist_switch_by_default.
|
||||
# Picker selections follow the same session-scoped default
|
||||
# as /model <name>; honour model.persist_switch_by_default.
|
||||
from hermes_cli.model_switch import resolve_persist_behavior
|
||||
|
||||
self._handle_model_picker_selection(
|
||||
|
|
|
|||
|
|
@ -1437,10 +1437,10 @@ class GatewaySlashCommandsMixin:
|
|||
|
||||
Supports:
|
||||
/model — interactive picker (Telegram/Discord) or text list
|
||||
/model <name> — switch model (persists by default)
|
||||
/model <name> — switch model (this session only)
|
||||
/model <name> --once — switch for the next turn only
|
||||
/model <name> --session — switch for this session only
|
||||
/model <name> --global — switch and persist (explicit)
|
||||
/model <name> --session — switch for this session only (explicit)
|
||||
/model <name> --global — switch and persist to config.yaml
|
||||
/model <name> --provider <provider> — switch provider + model
|
||||
/model --provider <provider> — switch to provider, auto-detect model
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -131,7 +131,7 @@ COMMAND_REGISTRY: list[CommandDef] = [
|
|||
# Configuration
|
||||
CommandDef("config", "Show current configuration", "Configuration",
|
||||
cli_only=True),
|
||||
CommandDef("model", "Switch model (persists by default)", "Configuration",
|
||||
CommandDef("model", "Switch model (session-scoped; --global to persist)", "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",),
|
||||
|
|
|
|||
|
|
@ -570,12 +570,14 @@ def resolve_persist_behavior(
|
|||
user is trying a different backend for this conversation, not
|
||||
reconfiguring the default. ``--global`` can still force persist.
|
||||
5. Otherwise defer to ``model.persist_switch_by_default`` in
|
||||
``config.yaml`` (defaults to ``True``, so a plain ``/model <name>``
|
||||
survives across sessions — the behavior users expect).
|
||||
``config.yaml`` (defaults to ``False``: a plain ``/model <name>``
|
||||
affects only the current session). Users who want the old
|
||||
persist-by-default behavior can set the key to ``true``; a one-off
|
||||
``--global`` always persists.
|
||||
|
||||
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.
|
||||
(``False``) applies.
|
||||
"""
|
||||
if is_once:
|
||||
return False
|
||||
|
|
@ -590,10 +592,10 @@ def resolve_persist_behavior(
|
|||
|
||||
model_cfg = load_config().get("model")
|
||||
if isinstance(model_cfg, dict):
|
||||
return bool(model_cfg.get("persist_switch_by_default", True))
|
||||
return bool(model_cfg.get("persist_switch_by_default", False))
|
||||
except Exception:
|
||||
pass
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
|
|
|
|||
|
|
@ -186,7 +186,7 @@ async def test_picker_tap_to_custom_clears_stale_base_url_and_api_mode(tmp_path,
|
|||
adapter = _FakePickerAdapter()
|
||||
cfg_path = _setup_isolated_home(tmp_path, monkeypatch, dict(_STALE_MODEL_CFG))
|
||||
|
||||
confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model"))
|
||||
confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model --global"))
|
||||
|
||||
assert confirmation is not None
|
||||
written = yaml.safe_load(cfg_path.read_text(encoding="utf-8"))
|
||||
|
|
|
|||
|
|
@ -159,11 +159,11 @@ async def test_model_global_persists_when_config_has_proper_dict_model(tmp_path,
|
|||
|
||||
|
||||
@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.
|
||||
async def test_model_no_flag_is_session_scoped_by_default(tmp_path, monkeypatch):
|
||||
"""A plain ``/model X`` (no --global) does NOT persist 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.
|
||||
This is the user-facing fix: switches are session-scoped unless the user
|
||||
opts in with ``--global`` or sets ``model.persist_switch_by_default: true``.
|
||||
"""
|
||||
cfg_path = _setup_isolated_home(
|
||||
tmp_path,
|
||||
|
|
@ -178,7 +178,7 @@ async def test_model_no_flag_persists_by_default(tmp_path, monkeypatch):
|
|||
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"
|
||||
assert written["model"]["default"] == "old-model"
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
|
|
|
|||
|
|
@ -8,9 +8,10 @@ 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.
|
||||
``persist_global`` and runs the same read-modify-write block the text path
|
||||
uses, so a tapped model behaves exactly like a typed one. Since the
|
||||
session-scope-by-default change, both default to session-only and persist
|
||||
only with ``--global`` (or ``model.persist_switch_by_default: true``).
|
||||
|
||||
These tests drive the real ``_handle_model_command`` with a fake picker-capable
|
||||
adapter that captures the ``on_model_selected`` callback, then invoke that
|
||||
|
|
@ -176,14 +177,14 @@ async def _drive_picker(runner, event):
|
|||
],
|
||||
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. The written
|
||||
``model:`` must always end up a nested dict regardless of the seed shape."""
|
||||
async def test_picker_tap_global_flag_persists(tmp_path, monkeypatch, seed_model):
|
||||
"""Tapping a model in a ``/model --global`` picker persists to config.yaml,
|
||||
matching the typed ``/model --global`` path. 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, seed_model)
|
||||
|
||||
confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model"))
|
||||
confirmation = await _drive_picker(_make_runner(adapter), _make_event("/model --global"))
|
||||
|
||||
assert confirmation is not None
|
||||
assert "gpt-5.5" in confirmation
|
||||
|
|
@ -198,6 +199,34 @@ async def test_picker_tap_persists_by_default(tmp_path, monkeypatch, seed_model)
|
|||
assert "api_mode" not in written["model"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_picker_tap_is_session_scoped_by_default(tmp_path, monkeypatch):
|
||||
"""Tapping a model in a bare ``/model`` picker applies an in-memory session
|
||||
override and does NOT touch config.yaml — switches are session-scoped
|
||||
unless the user opts in with ``--global`` (or sets
|
||||
``model.persist_switch_by_default: true``)."""
|
||||
adapter = _FakePickerAdapter()
|
||||
cfg_path = _setup_isolated_home(
|
||||
tmp_path, monkeypatch, {"default": "old-model", "provider": "openrouter"}
|
||||
)
|
||||
runner = _make_runner(adapter)
|
||||
|
||||
confirmation = await _drive_picker(runner, _make_event("/model"))
|
||||
|
||||
assert confirmation is not None
|
||||
assert "gpt-5.5" in confirmation
|
||||
# The session override IS applied in-memory (the switch worked).
|
||||
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 — session-scoped by default.
|
||||
written = yaml.safe_load(cfg_path.read_text(encoding="utf-8"))
|
||||
assert written["model"]["default"] == "old-model"
|
||||
assert written["model"]["provider"] == "openrouter"
|
||||
|
||||
|
||||
@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
|
||||
|
|
|
|||
|
|
@ -1,11 +1,12 @@
|
|||
"""Tests for persist-by-default model switching.
|
||||
"""Tests for session-scoped-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 <name>`` survives across sessions.
|
||||
- The default (no flags) is session-only, which is the user-facing fix: a
|
||||
plain ``/model <name>`` affects only the current session unless the user
|
||||
passes ``--global`` or sets ``model.persist_switch_by_default: true``.
|
||||
"""
|
||||
|
||||
from unittest.mock import patch
|
||||
|
|
@ -73,10 +74,10 @@ class TestResolvePersistBehavior:
|
|||
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).
|
||||
def test_default_session_only_when_config_missing(self):
|
||||
# No model section at all → built-in default (False, session-only).
|
||||
with _config({}):
|
||||
assert resolve_persist_behavior(False, False) is True
|
||||
assert resolve_persist_behavior(False, False) is False
|
||||
|
||||
def test_default_persists_when_key_true(self):
|
||||
with _config({"model": {"persist_switch_by_default": True}}):
|
||||
|
|
@ -87,9 +88,9 @@ class TestResolvePersistBehavior:
|
|||
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.
|
||||
# Fresh install: ``model: ""`` (not a dict) → built-in default False.
|
||||
with _config({"model": ""}):
|
||||
assert resolve_persist_behavior(False, False) is True
|
||||
assert resolve_persist_behavior(False, False) is False
|
||||
|
||||
def test_session_overrides_global_when_both_set(self):
|
||||
# --session is the explicit opt-out and wins over --global.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue