From 32273b8118e981bf6749f9acb9f6bae795388687 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 9 Sep 2026 11:44:01 +0530 Subject: [PATCH] refactor(cli): one stored_session_route for interactive and one-shot resume MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _apply_stored_session_runtime was a line-for-line copy of the first half of _restore_session_model (stored-model guard, session_gateway_runtime, bare-custom heal, model/provider-changed check). Extract that pure decision into cli_model_switch_mixin.stored_session_route and have both resume paths call it; the one-shot keeps only the _ModelChoice mapping and the drop-ambient-key rule. main.py stops re-normalising `resume` — _resolve_chat_session_args already did. Tests trimmed from 20 to 13: near-duplicate unit tests of the private helpers go, the end-to-end _run_agent contracts (stored runtime + reopen; explicit --model wins) and the empty-session-keeps-id case stay. --- hermes_cli/cli_model_switch_mixin.py | 37 ++++++++----- hermes_cli/main.py | 2 +- hermes_cli/oneshot.py | 37 +++++-------- tests/hermes_cli/test_oneshot_resume.py | 73 ------------------------- 4 files changed, 37 insertions(+), 112 deletions(-) diff --git a/hermes_cli/cli_model_switch_mixin.py b/hermes_cli/cli_model_switch_mixin.py index 5176c75c15..dd4381e32d 100644 --- a/hermes_cli/cli_model_switch_mixin.py +++ b/hermes_cli/cli_model_switch_mixin.py @@ -28,6 +28,25 @@ def _runtime_fields(cli) -> dict: return {key: getattr(cli, key, None) for key in _RUNTIME_FIELDS} +def stored_session_route(session_meta, *, current_model, current_provider): + """The route a resumed session should run on, or ``None`` when the stored one is absent or + already current. Returns ``(model, provider, base_url, api_mode, provider_changed)``; the + canonical row reader is ``SessionDB.session_gateway_runtime`` (``model_config.gateway_runtime``, + else the TUI's top-level keys). Bare ``custom`` is healed because the CLI resolve path + hard-fails on it (the TUI gateway keeps it when a base_url exists).""" + stored_model = str((session_meta or {}).get("model") or "").strip() + if not stored_model: + return None + from hermes_state import SessionDB as _SessionDB + runtime = _SessionDB.session_gateway_runtime(session_meta) + base_url = runtime.get("base_url") or None + provider = _heal_bare_custom_provider(runtime.get("provider") or None, base_url=base_url, model=stored_model) + provider_changed = bool(provider) and provider != current_provider + if stored_model == current_model and not provider_changed: + return None + return stored_model, provider, base_url, (runtime.get("api_mode") or None), provider_changed + + def _heal_bare_custom_provider(provider, *, base_url, model): """Bare ``custom`` is a billing class, not a routable identity: persisting/restoring it makes a later resume hard-fail once the config default leaves the custom endpoint. Recover the durable @@ -348,22 +367,12 @@ class CLIModelSwitchMixin: so the session still opens (the first turn surfaces the auth error). """ from cli import logger - stored_model = (session_meta or {}).get("model") - if not stored_model or getattr(self, "_explicit_model_override", False): + if getattr(self, "_explicit_model_override", False): return - # Canonical row reader: model_config.gateway_runtime, else the TUI's top-level keys. - from hermes_state import SessionDB as _SessionDB - _stored_runtime = _SessionDB.session_gateway_runtime(session_meta) - stored_base_url = _stored_runtime.get("base_url") or None - stored_api_mode = _stored_runtime.get("api_mode") or None - # Stricter than the TUI gateway's recovery (which keeps bare "custom" when a - # base_url exists) — the CLI's resolve path would hard-fail on it. - stored_provider = _heal_bare_custom_provider( - _stored_runtime.get("provider") or None, base_url=stored_base_url, model=stored_model) - model_changed = stored_model != self.model - provider_changed = bool(stored_provider) and stored_provider != self.provider - if not model_changed and not provider_changed: + route = stored_session_route(session_meta, current_model=self.model, current_provider=self.provider) + if route is None: return + stored_model, stored_provider, stored_base_url, stored_api_mode, provider_changed = route self.model = stored_model if stored_provider: self.provider = stored_provider diff --git a/hermes_cli/main.py b/hermes_cli/main.py index a5f5971a62..1aff355cd5 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -152,7 +152,7 @@ def _run_and_exit_oneshot( toolsets=toolsets, skills=skills, usage_file=usage_file, - resume=resume if isinstance(resume, str) and resume.strip() else None, + resume=resume, ) except KeyboardInterrupt: rc = 130 diff --git a/hermes_cli/oneshot.py b/hermes_cli/oneshot.py index a47c1803dd..a6ed16273c 100644 --- a/hermes_cli/oneshot.py +++ b/hermes_cli/oneshot.py @@ -383,36 +383,25 @@ def _load_resume_target(session_db, resume: Optional[str]) -> tuple[Optional[str def _apply_stored_session_runtime( choice: _ModelChoice, session_meta: Optional[dict], *, explicit_model: bool, ) -> _ModelChoice: - """Run a resumed one-shot on the session's stored runtime, not the ambient config. - - Same contract as the interactive ``_restore_session_model`` (cli_model_switch_mixin): no - stored model, or an explicit ``--model``, keeps the resolved ambient choice; otherwise the - stored model/provider/base_url/api_mode replace it (a resumed transcript must not be sent - to whatever the ambient config happens to point at). A changed provider drops the resolved - ``api_key`` — it belongs to the ambient/alias endpoint, and api_key is never persisted to - the session DB, so runtime resolution re-fetches credentials for the restored provider. - No-op when the stored route already matches the resolved one. - """ - stored_model = str((session_meta or {}).get("model") or "").strip() - if not stored_model or explicit_model: + """Run a resumed one-shot on the session's stored runtime, not the ambient config — the same + contract as the interactive ``_restore_session_model``, via the shared ``stored_session_route``. + An explicit ``--model`` keeps the ambient choice. A changed provider drops the resolved + ``api_key``: it belongs to the ambient endpoint and is never persisted, so runtime resolution + re-fetches credentials for the restored provider.""" + if explicit_model: return choice - from hermes_state import SessionDB as _SessionDB - from hermes_cli.cli_model_switch_mixin import _heal_bare_custom_provider + from hermes_cli.cli_model_switch_mixin import stored_session_route - stored_runtime = _SessionDB.session_gateway_runtime(session_meta) - stored_base_url = stored_runtime.get("base_url") or None - # Stricter than the TUI gateway's recovery — the CLI's resolve path hard-fails on bare "custom". - stored_provider = _heal_bare_custom_provider( - stored_runtime.get("provider") or None, base_url=stored_base_url, model=stored_model) - if stored_model == choice.model and (not stored_provider or stored_provider == choice.provider): + route = stored_session_route(session_meta, current_model=choice.model, current_provider=choice.provider) + if route is None: return choice - choice.model = stored_model - if stored_provider and stored_provider != choice.provider: + choice.model, stored_provider, stored_base_url, stored_api_mode, provider_changed = route + if provider_changed: choice.provider = stored_provider choice.base_url = stored_base_url choice.api_key = None - if stored_runtime.get("api_mode"): - choice.api_mode = str(stored_runtime["api_mode"]) + if stored_api_mode: + choice.api_mode = str(stored_api_mode) return choice diff --git a/tests/hermes_cli/test_oneshot_resume.py b/tests/hermes_cli/test_oneshot_resume.py index 90ba7ba857..960f94288f 100644 --- a/tests/hermes_cli/test_oneshot_resume.py +++ b/tests/hermes_cli/test_oneshot_resume.py @@ -69,18 +69,6 @@ class TestLoadResumeTarget: finally: db.close() - def test_session_meta_rows_are_dropped(self, tmp_path): - db = _db_with_session( - tmp_path, "s1", - messages=[("session_meta", "model switch"), ("user", "hello")], - ) - try: - sid, history, _meta = _load_resume_target(db, "s1") - assert sid == "s1" - assert [m["role"] for m in history] == ["user"] - finally: - db.close() - def test_empty_session_keeps_resolved_id(self, tmp_path): # Chat's contract: a resumed session with no messages starts fresh (no rows to # replay) but the turn is recorded under the SELECTED id. Dropping the id here @@ -95,20 +83,6 @@ class TestLoadResumeTarget: finally: db.close() - def test_empty_compression_head_without_rows_keeps_head_id(self, tmp_path): - # A chain head with no descendant rows must not fall through to a fresh id either: - # the turn stays anchored to the resolved head (redirect only happens when the - # descendant actually holds messages). - db = SessionDB(db_path=tmp_path / "state.db") - db.create_session(session_id="head", source="cli") - db.create_session(session_id="child", source="cli", parent_session_id="head") - try: - sid, history, _meta = _load_resume_target(db, "head") - assert sid == "head" - assert history == [] - finally: - db.close() - def test_compression_chain_redirects_to_child_with_messages(self, tmp_path): # Compression ends a session and forks a child that holds the rows; the loader # must land on the child, not the empty parent (resolve_resume_session_id). @@ -140,19 +114,6 @@ class TestLoadResumeTarget: finally: db.close() - def test_reopen_failure_is_best_effort(self, tmp_path, monkeypatch): - db = _db_with_session(tmp_path, "s1", messages=[("user", "hi")]) - try: - def _boom(_sid): - raise RuntimeError("reopen unavailable") - monkeypatch.setattr(db, "reopen_session", _boom) - sid, history, _meta = _load_resume_target(db, "s1") - assert sid == "s1" - assert [m["role"] for m in history] == ["user"] - finally: - db.close() - - class TestApplyStoredSessionRuntime: """A resumed one-shot must run on the session's stored runtime, not the ambient config (review on #105957): with ambient ``openrouter/ambient-model`` and a stored @@ -196,14 +157,6 @@ class TestApplyStoredSessionRuntime: assert choice.provider == "openrouter" assert choice.api_key is self._AMBIENT_KEY - def test_no_stored_model_keeps_ambient_choice(self, tmp_path): - meta = self._stored_db(tmp_path, model=None) - choice = _apply_stored_session_runtime( - _ModelChoice("ambient-model", "openrouter", api_key=self._AMBIENT_KEY), meta, explicit_model=False) - assert choice.model == "ambient-model" - assert choice.provider == "openrouter" - assert choice.api_key is self._AMBIENT_KEY - def test_matching_route_is_noop_and_keeps_credentials(self, tmp_path): meta = self._stored_db(tmp_path, route={"provider": "openrouter"}) choice = _apply_stored_session_runtime( @@ -212,20 +165,6 @@ class TestApplyStoredSessionRuntime: assert choice.api_key is self._AMBIENT_KEY assert choice.api_mode is None - def test_none_meta_is_a_noop(self): - choice = _ModelChoice("ambient-model", "openrouter") - assert _apply_stored_session_runtime(choice, None, explicit_model=False) == choice - - def test_bare_custom_provider_is_healed_or_dropped(self, tmp_path): - # Bare "custom" persisted by older builds is not a routable identity; the CLI's - # resolve path would hard-fail on it, so it is healed via the base_url or dropped. - meta = self._stored_db(tmp_path, route={"provider": "custom"}) - choice = _apply_stored_session_runtime( - _ModelChoice("ambient-model", "openrouter"), meta, explicit_model=False) - assert choice.model == "stored-model" - assert choice.provider in (None, "openrouter") or choice.provider.startswith("custom:") - - class TestRunAgentResumeRuntime: """End-to-end wiring: ``_run_agent`` must hand AIAgent the session's stored runtime and a reopened session row (both regressions from the review on #105957).""" @@ -327,15 +266,3 @@ class TestRunOneshotForwardsResume: assert rc == 0 assert captured["prompt"] == "hello" assert captured["resume"] == "sess-1" - - def test_no_resume_forwards_none(self, monkeypatch): - captured = {} - - def _fake_run_agent(prompt, **kwargs): - captured.update(kwargs) - return "ok", {"final_response": "ok"} - - monkeypatch.setattr("hermes_cli.oneshot._run_agent", _fake_run_agent) - rc = run_oneshot("hello", model="m", provider="custom") - assert rc == 0 - assert captured.get("resume") is None