mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-21 16:18:55 +00:00
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).
This commit is contained in:
parent
3f84b7a163
commit
11cb9e571f
5 changed files with 154 additions and 10 deletions
2
contributors/emails/deusyu@users.noreply.github.com
Normal file
2
contributors/emails/deusyu@users.noreply.github.com
Normal file
|
|
@ -0,0 +1,2 @@
|
|||
deusyu
|
||||
# PR #29923 salvage
|
||||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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 <alias>` in the CLI or any messaging platform. There are two equivalent formats — pick whichever fits your workflow.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue