diff --git a/hermes_state_sessions.py b/hermes_state_sessions.py index 57b42c4162..12da656034 100644 --- a/hermes_state_sessions.py +++ b/hermes_state_sessions.py @@ -677,9 +677,17 @@ class SessionSessionsMixin: self, session_id: str, model: str, provider: Optional[str] = None, *, base_url: Optional[str] = None, api_mode: Optional[str] = None, ) -> None: - """Set the model after a mid-session /model switch (unconditionally), null system_prompt so - stale Model:/Provider: footers rebuild, and drop any Browser runtime lock (lineage markers - survive). + """Set the model after a mid-session /model switch (unconditionally) and drop any Browser + runtime lock (lineage markers survive). + + The stored system prompt is deliberately PRESERVED: it is the session's provider cache + prefix, and a rebuilt prompt never matches those bytes — clearing the row re-bills the whole + prefix on a commit that may not even have moved the route (the picker re-committing the + session's current model), and the next turn then reads a NULL row and takes the broken-row + branch in ``agent.conversation_loop`` (a WARNING blaming the previous turn's + ``update_system_prompt`` write path). The stale ``Model:``/``Provider:`` footer this used to + null the row for is caught by ``_stored_prompt_matches_runtime`` on the next turn: a real + switch rebuilds there (INFO, then re-persists), a same-route commit reuses the bytes. When *provider* is given the whole route is written, in both shapes resume reads (top-level keys for the TUI/Desktop, ``gateway_runtime`` for the CLI), so a later resume recombines the @@ -698,8 +706,7 @@ class SessionSessionsMixin: route = {"provider": provider, "base_url": base_url or None, "api_mode": api_mode or None} patch.update(route, gateway_runtime=route) self._write_model_config_patch( - session_id, patch, "UPDATE sessions SET model = ?, model_config = ?, " - "system_prompt = NULL, system_prompt_hash = NULL WHERE id = ?", + session_id, patch, "UPDATE sessions SET model = ?, model_config = ? WHERE id = ?", lambda merged: (model, merged, session_id), ) @@ -709,14 +716,12 @@ class SessionSessionsMixin: params: Optional[Callable[[Optional[str]], tuple]] = None, ) -> None: """Merge ``patch`` into model_config then run ``sql`` with ``params(merged)`` in one write - transaction; no-op when the row doesn't exist. Custom ``sql`` (prompt-nulling) also GCs prompts.""" + transaction; no-op when the row doesn't exist.""" def _do(conn): merged = self._merge_model_config_json(conn, session_id, patch) if merged is _MODEL_CONFIG_ROW_MISSING: return conn.execute(sql, params(merged) if params else (merged, session_id)) - if params is not None: - self._delete_unreferenced_system_prompts(conn) self._execute_write(_do) def _merge_model_config_json( @@ -755,8 +760,10 @@ class SessionSessionsMixin: model_options: Optional[Dict[str, Any]] = None, route_source: Optional[str] = None, confirmed: bool = False, ) -> None: - """Persist a Browser / API-client runtime lock into model_config (lineage markers survive); null - system_prompt so cached footers cannot lie.""" + """Persist a Browser / API-client runtime lock into model_config (lineage markers survive). + The stored system prompt stays: a lock that moves the model is caught by the next turn's + ``_stored_prompt_matches_runtime`` identity check, and one that does not must keep the + session's cache prefix byte-identical.""" lock = { "provider": provider or "", "model": model or "", "model_options": model_options or {}, "route_source": route_source or "", "confirmed": bool(confirmed), "updated_at": time.time(), @@ -765,9 +772,7 @@ class SessionSessionsMixin: session_id, {"browser_model_lock": lock}, """UPDATE sessions SET model_config = ?, - model = COALESCE(?, model), - system_prompt = NULL, - system_prompt_hash = NULL + model = COALESCE(?, model) WHERE id = ?""", lambda merged: (merged, model, session_id), ) diff --git a/hermes_state_usage.py b/hermes_state_usage.py index 06ea514c55..41de7e4616 100644 --- a/hermes_state_usage.py +++ b/hermes_state_usage.py @@ -86,8 +86,11 @@ class SessionUsageMixin: self, session_id: str, *, provider: str, base_url: str, billing_mode: Optional[str] = None, ) -> None: """Unconditionally set the billing route (``update_token_counts`` only COALESCE-fills - NULLs) so the dashboard reflects the latest /model switch; also nulls - ``system_prompt`` so the cached snapshot header is rebuilt. + NULLs) so the dashboard reflects the latest /model switch. + + The stored system prompt is left alone — it is the session's cache prefix and the route + change is caught by the next turn's ``_stored_prompt_matches_runtime`` identity check, which + rebuilds only when the footer it embeds is actually stale. See #48173, #48248. """ @@ -98,11 +101,8 @@ class SessionUsageMixin: conn.execute("""UPDATE sessions SET billing_provider = ?, billing_base_url = ?, - billing_mode = COALESCE(?, billing_mode), - system_prompt = NULL, - system_prompt_hash = NULL + billing_mode = COALESCE(?, billing_mode) WHERE id = ?""", (provider, base_url, billing_mode, session_id)) - self._delete_unreferenced_system_prompts(conn) self._execute_write(_do) def queue_token_counts(self, session_id: str, **kwargs) -> None: diff --git a/tests/agent/test_system_prompt_switch_invalidation.py b/tests/agent/test_system_prompt_switch_invalidation.py new file mode 100644 index 0000000000..0cf59b1b8f --- /dev/null +++ b/tests/agent/test_system_prompt_switch_invalidation.py @@ -0,0 +1,183 @@ +"""A /model (or route) commit must not null a continuing session's stored system prompt. + +Regression for the WARNING ``Stored system prompt for session X is null; rebuilding from +scratch this turn ... Investigate the previous turn's update_system_prompt write path``: +three switch-path writers cleared ``system_prompt``/``system_prompt_hash`` "so stale +Model:/Provider: footers rebuild" — + + * ``SessionDB.update_session_model`` (every /model commit) + * ``SessionDB.update_session_runtime_lock`` (Browser / API-client lock) + * ``SessionDB.update_session_billing_route`` (billing route, also on provider fallbacks) + +— so the next turn read a NULL row and took the broken-row branch of +``agent.conversation_loop._restore_or_build_system_prompt``: a WARNING blaming the previous +turn's ``update_system_prompt`` write path, plus a full prompt rebuild (the whole provider +prefix re-billed) even on a commit that never moved the route — the picker re-committing the +session's *current* model, which the production log shows verbatim +(``switched from deepseek-v4.1-flash to deepseek-v4.1-flash via hyper``). + +The runtime-identity check (``_stored_prompt_matches_runtime``) is the single mechanism that +already rebuilds exactly when the stored footer is stale, so the writers preserve the row and +the check decides: a real switch rebuilds (INFO) on the next turn, a same-route commit reuses +the bytes verbatim, and the WARNING goes back to meaning "the stored prompt was lost". + +Real ``SessionDB`` on a temp file; no mock stands in for the DB layer. +""" + +from __future__ import annotations + +import logging +from unittest.mock import MagicMock + +import pytest + +from agent.conversation_loop import _restore_or_build_system_prompt +from hermes_state import SessionDB + +SESSION_ID = "switch-session" + + +def _stored_prompt(model: str, provider: str) -> str: + return ( + "You are Hermes Agent.\n\n" + "Conversation started: Thursday, September 24, 2026\n" + f"Model: {model}\n" + f"Provider: {provider}" + ) + + +@pytest.fixture() +def db(tmp_path, monkeypatch): + """Real SessionDB on a temp state.db, isolated from the live HERMES_HOME.""" + monkeypatch.setenv("HERMES_HOME", str(tmp_path / "hermes-home")) + session_db = SessionDB(db_path=tmp_path / "state.db") + yield session_db + session_db.close() + + +def _make_agent(db, *, model: str, provider: str, prebuilt: str) -> MagicMock: + """The minimal agent ``_restore_or_build_system_prompt`` needs; the DB is real.""" + agent = MagicMock() + agent._cached_system_prompt = None + agent.session_id = SESSION_ID + agent.model = model + agent.provider = provider + agent.platform = "discord" + agent._session_db = db + agent._use_prompt_caching = False + agent._persist_disabled = True # no on_session_start hook, no tool-pin rewrite + agent.enabled_toolsets = agent.disabled_toolsets = None + agent.tools = [] + agent._build_system_prompt = MagicMock(return_value=prebuilt) + return agent + + +def _continue_turn(db, *, model: str, provider: str, prebuilt: str, caplog): + """Run the next turn of a continuing session and return (agent, warnings).""" + agent = _make_agent(db, model=model, provider=provider, prebuilt=prebuilt) + with caplog.at_level(logging.INFO, logger="agent.conversation_loop"): + _restore_or_build_system_prompt(agent, None, [{"role": "user", "content": "hi"}]) + warnings = [r for r in caplog.records if r.levelno >= logging.WARNING] + return agent, warnings + + +class TestSwitchCommitsKeepTheStoredPrompt: + """Each switch-path writer leaves the prompt snapshot (and its dedup row) in place.""" + + def _seed(self, db) -> str: + prompt = _stored_prompt("x-ai/grok-4.5", "nous") + db.create_session(SESSION_ID, source="discord", model="x-ai/grok-4.5") + db.update_system_prompt(SESSION_ID, prompt) + return prompt + + def test_model_commit_keeps_the_snapshot(self, db): + prompt = self._seed(db) + + db.update_session_model( + SESSION_ID, "anthropic/claude-opus-4.8", provider="anthropic", base_url="https://a/v1", + ) + + assert db.get_session(SESSION_ID)["system_prompt"] == prompt + # Content-addressed storage intact: the row still resolves through its hash. + raw = db._conn.execute( + "SELECT system_prompt, system_prompt_hash FROM sessions WHERE id = ?", (SESSION_ID,) + ).fetchone() + assert raw["system_prompt"] is None + assert raw["system_prompt_hash"] is not None + assert db._conn.execute( + "SELECT COUNT(*) FROM system_prompts WHERE hash = ?", (raw["system_prompt_hash"],) + ).fetchone()[0] == 1 + + def test_runtime_lock_keeps_the_snapshot(self, db): + prompt = self._seed(db) + + db.update_session_runtime_lock( + SESSION_ID, model="anthropic/claude-opus-4.8", provider="anthropic", confirmed=True, + ) + + assert db.get_session(SESSION_ID)["system_prompt"] == prompt + + def test_billing_route_keeps_the_snapshot(self, db): + prompt = self._seed(db) + + db.update_session_billing_route(SESSION_ID, provider="openrouter", base_url="https://o/v1") + + assert db.get_session(SESSION_ID)["system_prompt"] == prompt + + +class TestNextTurnAfterASwitchCommit: + def test_same_route_commit_reuses_the_stored_bytes(self, db, caplog): + """The picker re-committing the session's current route must not cost a rebuild.""" + prompt = _stored_prompt("glm-5.3", "zro") + db.create_session(SESSION_ID, source="discord", model="glm-5.3") + db.update_system_prompt(SESSION_ID, prompt) + + db.update_session_model(SESSION_ID, "glm-5.3", provider="zro", base_url="https://hyper/v1") + + agent, warnings = _continue_turn( + db, model="glm-5.3", provider="zro", prebuilt="REBUILT", caplog=caplog, + ) + + assert agent._cached_system_prompt == prompt + agent._build_system_prompt.assert_not_called() + assert warnings == [] + + def test_route_change_rebuilds_through_the_identity_check(self, db, caplog): + """A real switch still rebuilds — on the next turn, via the stale footer, not a null row.""" + db.create_session(SESSION_ID, source="discord", model="deepseek-v4.1-flash") + db.update_system_prompt(SESSION_ID, _stored_prompt("deepseek-v4.1-flash", "hyper")) + + db.update_session_model( + SESSION_ID, "glm-5.3", provider="zro", base_url="https://hyper/v1", + ) + + rebuilt = _stored_prompt("glm-5.3", "zro") + agent, warnings = _continue_turn( + db, model="glm-5.3", provider="zro", prebuilt=rebuilt, caplog=caplog, + ) + + agent._build_system_prompt.assert_called_once() + assert agent._cached_system_prompt == rebuilt + # The rebuilt bytes are persisted so the following turns reuse them verbatim. + assert db.get_session(SESSION_ID)["system_prompt"] == rebuilt + assert warnings == [] + assert any( + r.levelno == logging.INFO and "stale runtime identity" in r.getMessage() + for r in caplog.records + ) + + def test_a_genuinely_lost_prompt_still_warns(self, db, caplog): + """The WARNING keeps its real meaning: nobody switched — the row was lost.""" + db.create_session(SESSION_ID, source="discord", model="glm-5.3") + db.update_system_prompt(SESSION_ID, _stored_prompt("glm-5.3", "zro")) + db.update_system_prompt(SESSION_ID, None) # a write that lost the prompt + + agent, warnings = _continue_turn( + db, model="glm-5.3", provider="zro", prebuilt="REBUILT", caplog=caplog, + ) + + assert agent._cached_system_prompt == "REBUILT" + assert any( + "is null; rebuilding" in w.getMessage() and "update_system_prompt write path" in w.getMessage() + for w in warnings + ) diff --git a/tests/hermes_cli/test_session_system_prompt_dedup.py b/tests/hermes_cli/test_session_system_prompt_dedup.py index a82617b79b..2ed3dc11fe 100644 --- a/tests/hermes_cli/test_session_system_prompt_dedup.py +++ b/tests/hermes_cli/test_session_system_prompt_dedup.py @@ -59,7 +59,7 @@ def test_prompt_snapshots_are_deduplicated_and_hydrated_for_readers(db): assert db.list_pending_handoffs()[0]["system_prompt"] == prompt -def test_prompt_replacement_and_route_changes_collect_only_orphans(db): +def test_route_changes_keep_the_prompt_and_replacement_collects_orphans(db): shared_prompt = "Model: x-ai/grok-4.5\nProvider: nous" db.create_session( "s1", @@ -70,6 +70,8 @@ def test_prompt_replacement_and_route_changes_collect_only_orphans(db): ) db.create_session("s2", "cli", system_prompt=shared_prompt) + # A route change keeps the stored snapshot: it is the session's cache prefix, and the next + # turn's runtime-identity check rebuilds only when the footer it embeds is actually stale. db.update_session_runtime_lock( "s1", model="anthropic/claude-opus-4.8", @@ -77,7 +79,7 @@ def test_prompt_replacement_and_route_changes_collect_only_orphans(db): confirmed=True, ) s1 = db.get_session("s1") - assert s1["system_prompt"] is None + assert s1["system_prompt"] == shared_prompt assert json.loads(s1["model_config"])["_branched_from"] == "parent" assert db.get_session("s2")["system_prompt"] == shared_prompt assert _prompt_count(db) == 1 @@ -87,13 +89,16 @@ def test_prompt_replacement_and_route_changes_collect_only_orphans(db): provider="openrouter", base_url="https://example.test/v1", ) - assert db.get_session("s2")["system_prompt"] is None - assert _prompt_count(db) == 0 + assert db.get_session("s2")["system_prompt"] == shared_prompt + assert _prompt_count(db) == 1 - db.update_system_prompt("s2", "replacement") - assert db.get_session("s2")["system_prompt"] == "replacement" + # Replacing the snapshot (a rebuild) is what orphans the previous bytes. + db.update_system_prompt("s1", "replacement") + assert db.get_session("s1")["system_prompt"] == "replacement" + assert _prompt_count(db) == 2 # "replacement" + the still-referenced shared prompt db.update_system_prompt("s2", None) - assert _prompt_count(db) == 0 + assert db.get_session("s2")["system_prompt"] is None + assert _prompt_count(db) == 1 def test_existing_session_enrichment_does_not_leak_unused_prompt(db):