mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-07-08 13:12:08 +00:00
fix(tui): stop hermes --tui -m from persisting the model globally (#59805)
The -m flag seeds HERMES_MODEL/HERMES_INFERENCE_MODEL for the launched TUI process only. But the per-turn config sync (_sync_agent_model_with_config) computed its target via _config_model_target(), which fell back to those env vars whenever config.yaml had no model.default — the normal state for custom-provider-only setups. The sync then replayed the -m model as a /model switch, and with model.persist_switch_by_default (default true) _persist_model_switch wrote model.default/provider/base_url into config.yaml. A one-shot CLI flag became the permanent global model, visible in every new session and every model picker. Two-sided fix: - _config_model_target() no longer falls back to the env seed. Empty model = config expresses no preference = sync is a no-op. The agent keeps the session-scoped -m model; config.yaml edits still sync. - _apply_model_switch() gains persist_override; all three internal callers (config sync, /moa one-shot swap, /moa post-turn restore) pass persist_override=False so session-mechanical switches can never write config.yaml regardless of the persist-by-default setting. User-typed /model keeps its existing flag/config behavior. E2E-verified against an isolated HERMES_HOME with a custom-provider-only config + -m env seed: sync no longer fires, config.yaml byte-identical, _resolve_model() still returns the seed for the session's own agent.
This commit is contained in:
Teknium
GitHub
parent
dd7198e71d
commit
70c6ae609e
2 changed files with 111 additions and 11 deletions
|
|
@ -1416,7 +1416,11 @@ def test_config_sync_switches_unpinned_session(monkeypatch):
|
|||
(
|
||||
"sid",
|
||||
"new/model --provider nous",
|
||||
{"confirm_expensive_model": True, "pin_session_override": False},
|
||||
{
|
||||
"confirm_expensive_model": True,
|
||||
"pin_session_override": False,
|
||||
"persist_override": False,
|
||||
},
|
||||
)
|
||||
]
|
||||
assert session["config_model_seen"] == ("new/model", "nous")
|
||||
|
|
@ -1536,6 +1540,78 @@ def test_config_sync_config_wins_over_env_seed(monkeypatch):
|
|||
assert session["config_model_seen"] == ("new/model", "")
|
||||
|
||||
|
||||
def test_config_sync_ignores_env_seed_without_config_model(monkeypatch):
|
||||
# `hermes --tui -m <model>` sets HERMES_MODEL/HERMES_INFERENCE_MODEL as a
|
||||
# launch-scoped seed. When config.yaml has NO model.default (typical
|
||||
# custom-provider-only setup), the sync must NOT adopt the env seed as a
|
||||
# config target — doing so replayed the -m flag as a /model switch and
|
||||
# (with persist_switch_by_default=True) wrote it into config.yaml
|
||||
# permanently.
|
||||
monkeypatch.setenv("HERMES_MODEL", "one-shot/model")
|
||||
monkeypatch.setenv("HERMES_INFERENCE_MODEL", "one-shot/model")
|
||||
monkeypatch.setattr(
|
||||
server, "_load_cfg", lambda: {"model": {"provider": "custom:mylocal"}}
|
||||
)
|
||||
session = _sync_test_session()
|
||||
monkeypatch.setattr(
|
||||
server,
|
||||
"_apply_model_switch",
|
||||
lambda *a, **k: pytest.fail("env seed must not trigger a config sync switch"),
|
||||
)
|
||||
|
||||
server._sync_agent_model_with_config("sid", session)
|
||||
|
||||
|
||||
def test_config_model_target_never_reads_env(monkeypatch):
|
||||
monkeypatch.setenv("HERMES_MODEL", "seed/model")
|
||||
monkeypatch.setenv("HERMES_INFERENCE_MODEL", "seed/model")
|
||||
monkeypatch.setattr(server, "_load_cfg", lambda: {"model": {"provider": "nous"}})
|
||||
|
||||
assert server._config_model_target() == ("", "nous")
|
||||
|
||||
|
||||
def test_apply_model_switch_persist_override_false_never_persists(monkeypatch):
|
||||
# Internal callers (config sync, /moa one-shot + restore) pass
|
||||
# persist_override=False; even with persist_switch_by_default=True the
|
||||
# switch must not write config.yaml.
|
||||
import types as _types
|
||||
|
||||
result = _types.SimpleNamespace(
|
||||
success=True,
|
||||
new_model="new/model",
|
||||
target_provider="nous",
|
||||
base_url="",
|
||||
api_key="key",
|
||||
api_mode="chat_completions",
|
||||
warning_message="",
|
||||
model_info=None,
|
||||
error_message="",
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.model_switch.switch_model", lambda **kw: result
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.model_switch.resolve_persist_behavior",
|
||||
lambda *a: pytest.fail("persist_override must bypass resolve_persist_behavior"),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
server, "_persist_model_switch",
|
||||
lambda _r: pytest.fail("persist_override=False must not persist"),
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"hermes_cli.model_cost_guard.expensive_model_warning",
|
||||
lambda *a, **k: None,
|
||||
)
|
||||
session = {"agent": None}
|
||||
|
||||
out = server._apply_model_switch(
|
||||
"sid", session, "new/model --provider nous", persist_override=False
|
||||
)
|
||||
|
||||
assert out["value"] == "new/model"
|
||||
assert session["model_override"]["model"] == "new/model"
|
||||
|
||||
|
||||
def test_startup_runtime_uses_tui_provider_env(monkeypatch):
|
||||
monkeypatch.setenv("HERMES_MODEL", "nous/hermes-test")
|
||||
monkeypatch.setenv("HERMES_TUI_PROVIDER", "nous")
|
||||
|
|
|
|||
|
|
@ -1994,14 +1994,14 @@ def _resolve_model() -> str:
|
|||
|
||||
|
||||
def _config_model_target() -> tuple[str, str]:
|
||||
"""(model, provider) currently selected by config (env as fallback).
|
||||
"""(model, provider) currently selected by config.yaml — and ONLY config.
|
||||
|
||||
config.yaml wins over HERMES_MODEL / HERMES_INFERENCE_MODEL here, the
|
||||
reverse of `_resolve_model()`'s startup order. Those env vars are a
|
||||
provision-time seed (hosted instances set HERMES_INFERENCE_MODEL in the
|
||||
container env); if they outranked config.yaml, the per-turn sync would
|
||||
stay pinned to the seed forever and dashboard/CLI model changes would
|
||||
never reach an open chat — the exact bug this sync exists to fix.
|
||||
Unlike `_resolve_model()`, this never reads HERMES_MODEL /
|
||||
HERMES_INFERENCE_MODEL. Those env vars are a launch-scoped seed
|
||||
(`hermes --tui -m <model>`, hosted-instance provisioning); if they
|
||||
fed the per-turn sync, the seed would be replayed as a /model switch
|
||||
and persisted globally, or would pin the session so dashboard/CLI
|
||||
model changes never reach an open chat.
|
||||
"""
|
||||
cfg_model = _load_cfg().get("model")
|
||||
model = ""
|
||||
|
|
@ -2013,8 +2013,15 @@ def _config_model_target() -> tuple[str, str]:
|
|||
provider = ""
|
||||
elif isinstance(cfg_model, str):
|
||||
model = cfg_model.strip()
|
||||
if not model:
|
||||
model = _resolve_model()
|
||||
# No fallback to _resolve_model() here: that reads HERMES_MODEL /
|
||||
# HERMES_INFERENCE_MODEL, which `hermes --tui -m <model>` sets as a
|
||||
# session-scoped seed for THIS launch. When config.yaml has no
|
||||
# model.default (custom-provider-only setups), falling back to the env
|
||||
# seed made the per-turn sync treat the -m flag as "the configured
|
||||
# model" and replay it as a /model switch — which then persisted the
|
||||
# one-shot flag into config.yaml globally (#-m leak). An empty model
|
||||
# simply means "config expresses no preference": the sync is a no-op
|
||||
# and the agent keeps whatever it was built with.
|
||||
return model, provider
|
||||
|
||||
|
||||
|
|
@ -2660,6 +2667,7 @@ def _apply_model_switch(
|
|||
confirm_expensive_model: bool = False,
|
||||
pin_session_override: bool = True,
|
||||
parsed_flags: tuple[str, str, bool, bool, bool] | None = None,
|
||||
persist_override: bool | None = None,
|
||||
) -> dict:
|
||||
from hermes_cli.model_switch import (
|
||||
parse_model_flags,
|
||||
|
|
@ -2677,7 +2685,11 @@ def _apply_model_switch(
|
|||
_force_refresh,
|
||||
is_session,
|
||||
) = parsed_flags
|
||||
persist_global = resolve_persist_behavior(is_global_flag, is_session)
|
||||
persist_global = (
|
||||
persist_override
|
||||
if persist_override is not None
|
||||
else resolve_persist_behavior(is_global_flag, is_session)
|
||||
)
|
||||
if not model_input:
|
||||
raise ValueError("model value required")
|
||||
|
||||
|
|
@ -2869,6 +2881,12 @@ def _sync_agent_model_with_config(sid: str, session: dict) -> None:
|
|||
raw,
|
||||
confirm_expensive_model=True,
|
||||
pin_session_override=False,
|
||||
# This sync ADOPTS a config.yaml change into the live session; it
|
||||
# must never write config back. Without this, the flag/config
|
||||
# default (persist_switch_by_default=True) re-persisted whatever
|
||||
# target the sync computed — the path that leaked `hermes --tui -m`
|
||||
# into config.yaml as the permanent global model.
|
||||
persist_override=False,
|
||||
)
|
||||
except Exception as e:
|
||||
_emit(
|
||||
|
|
@ -8700,6 +8718,9 @@ def _run_prompt_submit(rid, sid: str, session: dict, text: Any) -> None:
|
|||
_raw,
|
||||
confirm_expensive_model=False,
|
||||
pin_session_override=bool(_prev_override),
|
||||
# Session-internal restore after the /moa
|
||||
# one-shot — never persist to config.yaml.
|
||||
persist_override=False,
|
||||
)
|
||||
except Exception as _moa_restore_exc:
|
||||
logger.warning(
|
||||
|
|
@ -11559,6 +11580,9 @@ def _(rid, params: dict) -> dict:
|
|||
f"{preset} --provider moa",
|
||||
confirm_expensive_model=False,
|
||||
pin_session_override=True,
|
||||
# One-shot turn-scoped swap — never persist the MoA
|
||||
# virtual provider to config.yaml.
|
||||
persist_override=False,
|
||||
)
|
||||
except Exception as exc:
|
||||
session.pop("moa_one_shot_restore", None)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue