From 11cb9e571f34a5bb4feacd523da90bf56ed401d7 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sat, 18 Jul 2026 12:59:57 -0700 Subject: [PATCH] fix: harden /model --once against persistence and config-sync leaks Fixes the two review defects that kept PR #29923 open, plus docs: - gateway: exclude --once from the session-store write-through. The once-override lived only in memory before, but the write-through persisted it, so a gateway restart before the finally-restore rehydrated a supposedly one-turn model permanently. - TUI: skip _sync_agent_model_with_config while a one-turn restore is pending. The once-model is deliberately not pinned as a session model_override, so the config sync saw a model mismatch and clobbered the once-override back to the config model before the turn ran. - tests: real _handle_model_command drive asserting --once never touches set_model_override while --session still does; restore-pop idempotency. - docs: /model --once in configuring-models.md with an honest prompt-cache cost note (one-shot switch breaks the cached prefix twice; wins for short sessions and cheap-to-expensive escalation). --- .../emails/deusyu@users.noreply.github.com | 2 + gateway/slash_commands.py | 27 ++-- .../gateway/test_model_switch_persistence.py | 118 ++++++++++++++++++ tui_gateway/server.py | 10 +- website/docs/user-guide/configuring-models.md | 7 ++ 5 files changed, 154 insertions(+), 10 deletions(-) create mode 100644 contributors/emails/deusyu@users.noreply.github.com diff --git a/contributors/emails/deusyu@users.noreply.github.com b/contributors/emails/deusyu@users.noreply.github.com new file mode 100644 index 000000000000..85cc4dfc367c --- /dev/null +++ b/contributors/emails/deusyu@users.noreply.github.com @@ -0,0 +1,2 @@ +deusyu +# PR #29923 salvage diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 7aa4aac73eb5..dc708638db8c 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1985,15 +1985,24 @@ class GatewaySlashCommandsMixin: # the session store so the override survives a gateway restart. # api_key/api_mode are never persisted — they are re-resolved via # runtime provider resolution on rehydration. - try: - await self.async_session_store.set_model_override( - session_key, - self._session_model_overrides[session_key], - ) - except Exception: - logger.debug( - "Failed to persist session model override", exc_info=True - ) + # + # /model --once is intentionally EXCLUDED from the write-through: + # a one-turn override must never survive a restart. The persisted + # value stays at the pre-once state (the prior session override, + # or nothing), which is exactly what the finally-restore reverts + # the in-memory dict to. (#29923 review defect: the original + # implementation wrote through, so a crash before the restore + # rehydrated the once-model permanently.) + if not one_turn: + try: + await self.async_session_store.set_model_override( + session_key, + self._session_model_overrides[session_key], + ) + except Exception: + logger.debug( + "Failed to persist session model override", exc_info=True + ) # Evict cached agent so the next turn creates a fresh agent from the # override rather than relying on cache signature mismatch detection. diff --git a/tests/gateway/test_model_switch_persistence.py b/tests/gateway/test_model_switch_persistence.py index 25f4a0349611..a4224566201b 100644 --- a/tests/gateway/test_model_switch_persistence.py +++ b/tests/gateway/test_model_switch_persistence.py @@ -15,6 +15,7 @@ from datetime import datetime from types import SimpleNamespace from unittest.mock import AsyncMock, MagicMock +import pytest from gateway.config import GatewayConfig, Platform, PlatformConfig from gateway.session import SessionEntry, SessionSource, build_session_key @@ -283,3 +284,120 @@ class TestOneTurnModelOverrideRestore: runner._restore_session_model_override(sk, snapshot) assert sk not in runner._session_model_overrides + + def test_restore_pending_one_turn_pops_and_applies(self): + runner = _make_runner() + sk = build_session_key(_make_source()) + runner._pending_one_turn_model_restores[sk] = { + "had_override": False, + "override": None, + } + runner._session_model_overrides[sk] = {"model": "temp/model"} + + runner._restore_pending_one_turn_model_override(sk) + + assert sk not in runner._session_model_overrides + assert sk not in runner._pending_one_turn_model_restores + # Second call is a no-op (snapshot already consumed). + runner._restore_pending_one_turn_model_override(sk) + + +class TestOneTurnNeverPersisted: + """/model --once must never write through to the session store. + + Regression guard for the #29923 review defect: the original + implementation wrote the once-override through set_model_override, so a + gateway restart before the finally-restore rehydrated a supposedly + one-turn model permanently. Drives the real _handle_model_command with + a mocked switch pipeline and asserts on the store boundary. + """ + + @staticmethod + def _runner_with_store(tmp_path, monkeypatch): + import yaml as _yaml + + import gateway.run as gateway_run + from gateway.run import GatewayRunner + from hermes_cli.model_switch import ModelSwitchResult + + hermes_home = tmp_path / ".hermes" + hermes_home.mkdir() + (hermes_home / "config.yaml").write_text( + _yaml.safe_dump( + {"model": {"default": "old-model", "provider": "openrouter"}} + ), + 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: ModelSwitchResult( + success=True, + new_model="gpt-5.5", + target_provider="openrouter", + provider_changed=False, + api_key="sk-test", + base_url="https://openrouter.ai/api/v1", + api_mode="chat_completions", + provider_label="OpenRouter", + ), + ) + monkeypatch.setattr("hermes_constants.get_hermes_home", lambda: hermes_home) + monkeypatch.setattr("hermes_cli.config.get_hermes_home", lambda: hermes_home) + + runner = object.__new__(GatewayRunner) + runner.adapters = {} + runner._voice_mode = {} + runner._session_model_overrides = {} + runner._pending_one_turn_model_restores = {} + runner._running_agents = {} + # async_session_store is a property over session_store; install the + # mock behind the private cache attribute it reads. + _store = MagicMock() + _store.set_model_override = AsyncMock() + _store._store = None + runner.session_store = None + runner._async_session_store = _store + return runner + + @staticmethod + def _event(text): + from gateway.platforms.base import MessageEvent, MessageType + + return MessageEvent( + text=text, + message_type=MessageType.TEXT, + source=_make_source(), + ) + + @pytest.mark.asyncio + async def test_once_skips_session_store_write_through( + self, tmp_path, monkeypatch + ): + runner = self._runner_with_store(tmp_path, monkeypatch) + sk = build_session_key(_make_source()) + + result = await runner._handle_model_command( + self._event("/model gpt-5.5 --once") + ) + + assert result is not None and "gpt-5.5" in result + # In-memory override installed for the next turn + restore queued... + assert runner._session_model_overrides[sk]["model"] == "gpt-5.5" + assert sk in runner._pending_one_turn_model_restores + # ...but NEVER written through to the persistent session store. + runner.async_session_store.set_model_override.assert_not_awaited() + + @pytest.mark.asyncio + async def test_session_switch_still_writes_through( + self, tmp_path, monkeypatch + ): + runner = self._runner_with_store(tmp_path, monkeypatch) + + result = await runner._handle_model_command( + self._event("/model gpt-5.5 --session") + ) + + assert result is not None + runner.async_session_store.set_model_override.assert_awaited_once() diff --git a/tui_gateway/server.py b/tui_gateway/server.py index 039b658ecdde..5f90717da6b4 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -9857,7 +9857,15 @@ def _run_prompt_submit(rid, sid: str, session: dict, text: Any) -> None: # the sudo.request overlay. (secret capture is a module global, so # re-running is a harmless no-op.) _wire_callbacks(sid) - _sync_agent_model_with_config(sid, session) + # Skip the config-model sync while a /model --once override is + # active: the once-model is intentionally not pinned as a session + # model_override (it must not persist), so without this guard the + # sync would see "agent model != config model" and clobber the + # once-override back to the config model before the turn runs + # (#29923 review defect). Any config.yaml change is adopted on + # the NEXT turn, after the finally-restore below. + if not one_turn_restore: + _sync_agent_model_with_config(sid, session) cwd = _session_cwd(session) _register_session_cwd(session) cols = session.get("cols", 80) diff --git a/website/docs/user-guide/configuring-models.md b/website/docs/user-guide/configuring-models.md index b5cc8611fc57..e220b5349833 100644 --- a/website/docs/user-guide/configuring-models.md +++ b/website/docs/user-guide/configuring-models.md @@ -192,10 +192,17 @@ Inside any `hermes chat` session: ``` /model gpt-5.4 --provider openrouter # session-only /model gpt-5.4 --provider openrouter --global # also persists to config.yaml +/model claude-opus-4.6 --once # next turn only, then auto-restores ``` `--global` does the same thing the dashboard's **Change** button does, plus it switches the running session in-place. +`--once` switches for a single turn and restores the previous model afterward — on success, error, or interrupt alike. Nothing is persisted: a gateway restart mid-turn comes back on the original model. Useful for escalating one hard question to an expensive model ("ask Opus just this once") or dropping to a cheap model for a throwaway query. + +:::note Prompt-cache cost +A one-turn switch breaks the provider's prompt-cache prefix twice (switching out and back). In a long session on a cached-prefix provider (Anthropic, OpenAI), the next turn re-pays full input cost — `--once` wins for short sessions or cheap→expensive escalation, but a quick side question inside a long expensive session can cost more than it saves. +::: + ### Custom aliases Define your own short names for models you reach for often, then use `/model ` in the CLI or any messaging platform. There are two equivalent formats — pick whichever fits your workflow.