fix(fallback): restore the primary model/auth route after any picker failure in hermes fallback add

evals/cli_fallback_add_picker_error.py

@@ -0,0 +1,135 @@
+"""Drive the real ``hermes fallback add`` under a Linux PTY into an ordinary picker error.
+
+Run from the checkout with the project Python. No provider requests are sent: the picker
+target is a saved custom provider with ``discover_models: false``. The auth store is made
+unreadable AFTER the provider menu renders (the pre-picker snapshot has already happened), so
+the canonical picker writes the temporary primary route to config.yaml and then fails inside
+``deactivate_provider`` with a plain ``PermissionError`` -- not ``SystemExit``.
+
+The invariant under test: config.yaml ``model`` must equal the pre-picker primary afterwards.
+"""
+import argparse
+import errno
+import json
+import os
+from pathlib import Path
+import pty
+import re
+import select
+import signal
+import struct
+import subprocess
+import sys
+import tempfile
+import termios
+import time
+import fcntl
+
+PRIMARY = {"provider": "openrouter", "default": "primary/model-a",
+           "base_url": "https://openrouter.ai/api/v1", "api_mode": "chat_completions"}
+CONFIG = (
+    "model:\n  provider: openrouter\n  default: primary/model-a\n"
+    "  base_url: https://openrouter.ai/api/v1\n  api_mode: chat_completions\n"
+    "custom_providers:\n  - name: LocalLab\n    base_url: http://127.0.0.1:9/v1\n"
+    "    model: lab-model\n    discover_models: false\n    models:\n      - lab-model\n"
+    "memory:\n  provider: ''\n")
+
+
+def _persisted_model(root: Path, env: dict) -> dict:
+    """``config.yaml`` ``model`` section as the CLI itself reads it (owner module, same env)."""
+    out = subprocess.run(
+        [sys.executable, "-c", "import json; from hermes_cli.config import load_config; "
+         "print(json.dumps(load_config().get('model')))"],
+        cwd=root, env=env, capture_output=True, text=True, check=True)
+    return json.loads(out.stdout.strip().splitlines()[-1])
+
+
+def run(root: Path, output: Path) -> dict:
+    with tempfile.TemporaryDirectory(prefix="hermes_test_fallback_") as home:
+        hh = Path(home) / ".hermes"
+        hh.mkdir()
+        (hh / "config.yaml").write_text(CONFIG, encoding="utf-8")
+        (hh / ".env").write_text("OPENROUTER_API_KEY=local-not-used\n", encoding="utf-8")
+        auth = hh / "auth.json"
+        auth.write_text(json.dumps({"version": 1, "providers": {}, "active_provider": "nous"}))
+        # A stub ``curses`` package forces every menu onto its numbered fallback so the PTY
+        # exchange is line-oriented (the curses UI is not what is under test here).
+        shim = Path(home) / "shim" / "curses"
+        shim.mkdir(parents=True)
+        (shim / "__init__.py").write_text("raise ImportError('curses disabled for PTY harness')\n")
+        env = {"PATH": os.environ["PATH"], "HOME": home, "HERMES_HOME": str(hh),
+               "PYTHONPATH": f"{shim.parent}{os.pathsep}{root}", "PYTHONUNBUFFERED": "1",
+               "TERM": "dumb", "LANG": "C.UTF-8"}
+        master, slave = pty.openpty()
+        fcntl.ioctl(slave, termios.TIOCSWINSZ, struct.pack("HHHH", 50, 120, 0, 0))
+        proc = subprocess.Popen([sys.executable, "-m", "hermes_cli.main", "fallback", "add"],
+                                cwd=root, env=env, stdin=slave, stdout=slave, stderr=slave,
+                                start_new_session=True)
+        os.close(slave)
+        data = bytearray()
+
+        def pump_until(predicate, timeout=60):
+            deadline = time.monotonic() + timeout
+            while time.monotonic() < deadline:
+                if predicate(bytes(data)):
+                    return True
+                if select.select([master], [], [], 0.1)[0]:
+                    try:
+                        chunk = os.read(master, 65536)
+                    except OSError as exc:
+                        if exc.errno == errno.EIO:
+                            return predicate(bytes(data))
+                        raise
+                    if not chunk:
+                        return predicate(bytes(data))
+                    data.extend(chunk)
+            return predicate(bytes(data))
+
+        try:
+            assert pump_until(lambda b: b"Choice [default" in b), data[-2000:]
+            text = bytes(data).decode(errors="replace")
+            row = re.search(r"(\d+)\. LocalLab", text)
+            assert row, text[-3000:]
+            # Snapshot is done (menu is up); now make the auth store unreadable so the picker's
+            # own deactivate_provider() fails with an ordinary OSError after writing the model.
+            auth.chmod(0)
+            os.write(master, f"{row.group(1)}\r".encode())
+            offset = len(data)
+            assert pump_until(lambda b: b"Choice [" in b[offset:]), data[-2000:]
+            os.write(master, b"1\r")
+            exited = pump_until(lambda b: proc.poll() is not None, 90)
+            proc.wait(timeout=30)
+            auth.chmod(0o600)
+            model_after = _persisted_model(root, env)
+            text = bytes(data).decode(errors="replace")
+            return {"exited": exited, "returncode": proc.returncode,
+                    "picker_error_surfaced": "PermissionError" in text,
+                    "model_after": model_after, "primary_restored": model_after == PRIMARY,
+                    "auth_active_provider": json.loads(auth.read_text()).get("active_provider"),
+                    "restore_note": "Could not fully restore" in text,
+                    "raw_path": str(output / "fallback-add-picker-error.pty")}
+        finally:
+            output.mkdir(parents=True, exist_ok=True)
+            (output / "fallback-add-picker-error.pty").write_bytes(data)
+            if proc.poll() is None:
+                os.killpg(proc.pid, signal.SIGKILL)
+                proc.wait(timeout=30)
+            os.close(master)
+
+
+def main():
+    parser = argparse.ArgumentParser()
+    parser.add_argument("--root", required=True)
+    parser.add_argument("--output", type=Path, required=True)
+    parser.add_argument("--expect", choices=("stranded", "restored"), required=True)
+    args = parser.parse_args()
+    result = run(Path(args.root).resolve(), args.output)
+    (args.output / "results.json").write_text(json.dumps(result, indent=2) + "\n")
+    print(json.dumps(result, indent=2))
+    assert result["exited"] and result["returncode"] != 0, result
+    assert result["picker_error_surfaced"], result
+    assert result["primary_restored"] == (args.expect == "restored"), result
+
+
+if __name__ == "__main__":
+    main()

hermes_cli/fallback_cmd.py

@@ -13,6 +13,9 @@
 _read_chain = get_fallback_chain
 
 
+_MISSING_ACTIVE_PROVIDER = object()
+
+
 def _identity(entry: Dict[str, Any]):
     """BackendIdentity for a ``{provider, model, base_url?}`` entry."""
     from agent.backend_identity import BackendIdentity
@@ -45,22 +48,25 @@ def _extract_fallback_from_model_cfg(model_cfg: Any) -> Optional[Dict[str, Any]]
 
 
 def _snapshot_auth_active_provider() -> Any:
-    """Current ``active_provider`` in auth.json, or None if unavailable."""
-    try:
-        from hermes_cli.auth import _load_auth_store
-        return _load_auth_store().get("active_provider")
-    except Exception:
-        return None
+    """Return the current ``active_provider`` in auth.json."""
+    from hermes_cli.auth import _auth_store_lock, _load_auth_store
+
+    with _auth_store_lock():
+        store = _load_auth_store()
+        return store.get("active_provider", _MISSING_ACTIVE_PROVIDER)
 
 
 def _restore_auth_active_provider(value: Any) -> None:
-    """Write back a snapshotted ``active_provider``; best-effort (user re-runs `hermes model`), never fails the add."""
-    try:
-        from hermes_cli.auth import _auth_store_lock, _load_auth_store, _save_auth_store
-        with _auth_store_lock():
-            _save_auth_store({**_load_auth_store(), "active_provider": value})
-    except Exception:
-        pass
+    """Write back a previously snapshotted ``active_provider`` value."""
+    from hermes_cli.auth import _auth_store_lock, _load_auth_store, _save_auth_store
+
+    with _auth_store_lock():
+        store = _load_auth_store()
+        if value is _MISSING_ACTIVE_PROVIDER:
+            store.pop("active_provider", None)
+        else:
+            store["active_provider"] = value
+        _save_auth_store(store)
 
 
 def _restore_model_cfg(model_before: Any) -> None:
@@ -73,6 +79,22 @@ def _restore_model_cfg(model_before: Any) -> None:
     save_config(cfg)
 
 
+def _restore_primary_route(model_before: Any, active_provider_before: Any) -> None:
+    """Attempt both halves of temporary picker-route restoration."""
+    errors: list[BaseException] = []
+    try:
+        _restore_model_cfg(model_before)
+    except BaseException as exc:
+        errors.append(exc)
+    try:
+        _restore_auth_active_provider(active_provider_before)
+    except BaseException as exc:
+        errors.append(exc)
+    if errors:
+        details = "; ".join(str(exc) for exc in errors)
+        raise RuntimeError(f"Could not fully restore the primary route: {details}") from errors[0]
+
+
 def _entries(n: int) -> str:
     return f"{n} {'entry' if n == 1 else 'entries'}"
 
@@ -122,45 +144,43 @@ def cmd_fallback_add(args) -> None:
     from hermes_cli.config import load_config, save_config
     _require_tty("fallback add")
 
-    # Snapshot BEFORE the picker runs: "picked" vs "cancelled" is decided by comparing before/after,
-    # and the primary must be restored either way.
+    # Snapshot BEFORE the picker runs; both route stores must be restored on every exit path.
     model_before = copy.deepcopy(load_config().get("model"))
     active_provider_before = _snapshot_auth_active_provider()
     print("\n  Adding a fallback provider.  The picker below is the same one used by\n"
           "  `hermes model` — select the provider + model you want as a fallback.\n")
 
-    def _restore() -> None:
-        _restore_model_cfg(model_before)
-
-        _restore_auth_active_provider(active_provider_before)
     try:
         select_provider_and_model(args=args)
-    except SystemExit:  # some provider flows exit on auth failure — restore state and re-raise
-        _restore()
+        after_cfg = load_config()
+        model_after = after_cfg.get("model")
+        new_entry = _extract_fallback_from_model_cfg(model_after)
+    except BaseException as picker_error:
+        try:
+            _restore_primary_route(model_before, active_provider_before)
+        except Exception as restore_error:
+            picker_error.add_note(
+                "Could not fully restore the primary route after fallback "
+                f"selection failed: {restore_error}"
+            )
         raise
-    new_entry = _extract_fallback_from_model_cfg(load_config().get("model"))
-    if not new_entry:  # picker didn't complete (user cancelled or flow bailed)
-        _restore()
+
+    # From here onward no identity/import/append failure can strand the temporary picker route.
+    _restore_primary_route(model_before, active_provider_before)
+
+    if not new_entry:
         print("\n  No fallback added.")
         return
 
-    # Same deployment as the primary → nothing to add. Identity semantics are owned by
-    # agent.backend_identity: same provider+model on a DIFFERENT explicit base_url is a different
-    # backend (multi-endpoint pool) and a legitimate fallback.
-    # Picker picked the same thing that's already the primary → nothing changed, and there's nothing useful
-    # to add as a fallback to itself. See #54250, #57584, #62984.
     from agent.backend_identity import same_deployment
     new_ident = _identity(new_entry)
     primary_entry = _extract_fallback_from_model_cfg(model_before)
     if primary_entry and same_deployment(_identity(primary_entry), new_ident):
-        _restore()
         print(f"\n  Selected model matches the current primary ({_format_entry(new_entry)}).")
         print("  A provider cannot be a fallback for itself — no change.")
         return
 
-    # Restore the primary, then re-load (rather than mutating the post-picker config) because the
-    # picker may have touched other top-level keys (custom_providers, credentials) we want to keep.
-    _restore()
+    # Reload after primary restoration; picker-created providers/credentials remain.
     final_cfg = load_config()
     chain = _read_chain(final_cfg)
     if any(same_deployment(_identity(existing), new_ident) for existing in chain):

tests/hermes_cli/test_fallback_cmd.py

@@ -15,7 +15,6 @@
 
 @pytest.fixture()
 def isolated_home(tmp_path, monkeypatch):
-    monkeypatch.setattr(Path, "home", lambda: tmp_path)
     home = tmp_path / ".hermes"
     home.mkdir(exist_ok=True)
     monkeypatch.setenv("HERMES_HOME", str(home))
@@ -199,6 +198,13 @@ def fake_picker(args=None):
                 "base_url": "https://openrouter.ai/api/v1",
                 "api_mode": "chat_completions",
             }
+            cfg["custom_providers"] = [
+                {
+                    "name": "Picker-created endpoint",
+                    "base_url": "https://picker.example/v1",
+                    "model": "picker-model",
+                }
+            ]
             save_config(cfg)
 
         with patch("hermes_cli.main.select_provider_and_model", side_effect=fake_picker), \
@@ -215,6 +221,74 @@ def fake_picker(args=None):
         # Fallback added
         assert len(cfg["fallback_providers"]) == 1
         assert cfg["fallback_providers"][0]["provider"] == "openrouter"
+        assert cfg["custom_providers"] == [
+            {
+                "name": "Picker-created endpoint",
+                "base_url": "https://picker.example/v1",
+                "model": "picker-model",
+            }
+        ]
+
+    def test_restore_preserves_absent_active_provider(self):
+        from contextlib import nullcontext
+
+        from hermes_cli import auth, fallback_cmd
+
+        store = {"version": 1, "providers": {}}
+
+        def save_auth(value):
+            store.clear()
+            store.update(value)
+
+        with patch.object(auth, "_load_auth_store", lambda: dict(store)), patch.object(
+            auth, "_save_auth_store", save_auth
+        ), patch.object(auth, "_auth_store_lock", nullcontext):
+            before = fallback_cmd._snapshot_auth_active_provider()
+            fallback_cmd._restore_auth_active_provider(before)
+
+        assert "active_provider" not in store
+
+    @pytest.mark.parametrize("picker_error", [LookupError("picker failed"), KeyboardInterrupt()],
+                             ids=["exception", "ctrl-c"])
+    def test_picker_failure_restores_persisted_primary_without_masking_error(
+        self, isolated_home, picker_error
+    ):
+        """An ordinary picker exception or a Ctrl+C mid-picker must leave config.yaml's
+        ``model`` exactly as it was before ``fallback add`` started (base only handled SystemExit)."""
+        from hermes_cli import fallback_cmd
+
+        primary_model = {
+            "provider": "anthropic",
+            "default": "claude-sonnet-4-6",
+            "base_url": "https://api.anthropic.com",
+            "api_mode": "anthropic_messages",
+        }
+        _write_config(isolated_home, {"model": primary_model, "theme": "midnight"})
+
+        def failing_picker(args=None):
+            from hermes_cli.config import load_config, save_config
+
+            cfg = load_config()
+            cfg["model"] = {
+                "provider": "openrouter",
+                "default": "anthropic/claude-sonnet-4.6",
+                "base_url": "https://openrouter.ai/api/v1",
+                "api_mode": "chat_completions",
+            }
+            save_config(cfg)
+            raise picker_error
+
+        with patch(
+            "hermes_cli.main.select_provider_and_model",
+            side_effect=failing_picker,
+        ), patch("hermes_cli.main._require_tty"):
+            with pytest.raises(type(picker_error)) as exc_info:
+                fallback_cmd.cmd_fallback_add(types.SimpleNamespace())
+
+        assert exc_info.value is picker_error
+        persisted = _read_config(isolated_home)
+        assert persisted["model"] == primary_model
+        assert persisted["theme"] == "midnight"
 
 
 # ---------------------------------------------------------------------------