From e63da95318bc8e76bfa3bc1d87a05f6e58ee8251 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 18 Sep 2026 04:03:12 -0700 Subject: [PATCH] fix(cli): auth.json-only login with a benched credential is explained, not sent to the wizard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps from review of #113720's fix: 1. A profile logged in via auth.json (active_provider: nous) with no model.provider in config.yaml resolves as "auto". The ladder's OAuth rung swallowed the AuthError for "auto" and fell through to the keyless OpenRouter fallback, so the startup probe returned (False, None) and the first-run wizard ran anyway. The ladder now catches the AuthError in _ladder_rungs, still falls through for "auto", but stamps the swallowed error on a keyless fallback as `auth_error`; _probe_runtime_credentials returns it so the notice names the real failure. 2. The gate itself lived only in cli.py::_tui_print_startup and was untested at the seam (reverting cli.py left the suite green). It is now one mixin method, _maybe_offer_first_run_setup (tty check → probe → explain → offer), called from _tui_print_startup, and both tests drive that method with stdin.isatty patched True and _offer_first_run_setup asserting it is not called. Tests: the benched-credential test now covers the gate and the cooldown headline wording; the blank-install control is folded into the new auth.json-only test. --- cli.py | 5 +- hermes_cli/cli_agent_setup_mixin.py | 16 +++- hermes_cli/runtime_provider.py | 32 ++++--- tests/hermes_cli/test_cli_first_run_setup.py | 99 ++++++++++++++------ 4 files changed, 105 insertions(+), 47 deletions(-) diff --git a/cli.py b/cli.py index e6718e935b..1287d90ce0 100644 --- a/cli.py +++ b/cli.py @@ -3661,10 +3661,7 @@ class HermesCLI(CLIProcessNotificationsMixin, CLIAgentSetupMixin, CLICommandsMix # a chat that spins ~30s and fails with a provider-specific error. TTY only. A # configured profile whose credential is benched or signed out gets the reason instead. try: - if sys.stdin.isatty(): - ready, error = self._probe_runtime_credentials() - if not ready and not self._explain_unusable_credentials(error): - self._offer_first_run_setup() + self._maybe_offer_first_run_setup() except Exception: logger.debug("first-run setup offer failed", exc_info=True) diff --git a/hermes_cli/cli_agent_setup_mixin.py b/hermes_cli/cli_agent_setup_mixin.py index beb829c675..c7d5f69683 100644 --- a/hermes_cli/cli_agent_setup_mixin.py +++ b/hermes_cli/cli_agent_setup_mixin.py @@ -394,8 +394,9 @@ class CLIAgentSetupMixin: return self._probe_runtime_credentials()[0] def _probe_runtime_credentials(self) -> tuple: - """``(ready, error)``: *error* is the exception that stopped resolution, ``None`` when a - provider resolved (usable or merely keyless). Never prints or mutates CLI state.""" + """``(ready, error)``: *error* is the exception that stopped resolution — raised, or + swallowed by the "auto" ladder and stamped on a keyless fallback — ``None`` when a provider + resolved (usable or merely keyless). Never prints or mutates CLI state.""" from hermes_cli.runtime_provider import resolve_runtime_provider try: runtime = resolve_runtime_provider( @@ -409,7 +410,16 @@ class CLIAgentSetupMixin: base_url = runtime.get("base_url") if callable(api_key) or (isinstance(api_key, str) and api_key): return bool(base_url), None - return _keyless_custom_base(base_url), None + return _keyless_custom_base(base_url), runtime.get("auth_error") + + def _maybe_offer_first_run_setup(self) -> None: + """Interactive startup gate: a blank install goes to the provider wizard; a configured + profile whose credential is benched or signed out gets the reason instead (#113720).""" + if not sys.stdin.isatty(): + return + ready, error = self._probe_runtime_credentials() + if not ready and not self._explain_unusable_credentials(error): + self._offer_first_run_setup() def _explain_unusable_credentials(self, error) -> bool: """A configured profile whose credential is benched, quarantined or signed out is not a diff --git a/hermes_cli/runtime_provider.py b/hermes_cli/runtime_provider.py index bfb0a0e464..9a39758e65 100644 --- a/hermes_cli/runtime_provider.py +++ b/hermes_cli/runtime_provider.py @@ -680,17 +680,10 @@ _OAUTH_RUNTIME_PROVIDERS: Dict[str, _OAuthRuntimeSpec] = { def _resolve_oauth_runtime(provider, requested_provider, model_cfg, target_model) -> Optional[Dict[str, Any]]: - """Runtime from an ``_OAUTH_RUNTIME_PROVIDERS`` spec. On AuthError: re-raise for an explicit - request; for "auto" (auto-detected but credentials stale/revoked) log and return None so the - ladder falls through to env-var providers (e.g. OpenRouter).""" + """Runtime from an ``_OAUTH_RUNTIME_PROVIDERS`` spec; raises AuthError when the credential is + stale/revoked/benched (``_ladder_rungs`` decides whether an "auto" request falls through).""" spec = _OAUTH_RUNTIME_PROVIDERS[provider] - try: - creds = spec.resolve() - except AuthError: - if requested_provider != "auto": - raise - logger.info("%s; falling through to next provider.", spec.failure_msg) - return None + creds = spec.resolve() api_mode = spec.api_mode(_effective_model(model_cfg, target_model)) if callable(spec.api_mode) else spec.api_mode return _runtime(provider, api_mode, (creds.get("base_url") or "").rstrip("/") or spec.default_base_url, creds.get("api_key", ""), source=creds.get("source", spec.default_source), @@ -879,7 +872,8 @@ def resolve_runtime_provider(*, requested: Optional[str] = None, explicit_api_ke 4. local-endpoint bypass (no explicit creds, config base_url at a non-cloud host) 5. ``auth.resolve_provider`` → explicit --api-key/--base-url path 6. credential pool (OpenRouter pool only without custom endpoint/override) - 7. OAuth specs (nous/codex/xai/qwen; "auto" swallows AuthError and logs) → minimax-oauth + 7. OAuth specs (nous/codex/xai/qwen; "auto" swallows AuthError, logs, and stamps it on a + keyless fallback as ``auth_error``) → minimax-oauth → external-process → anthropic env → bedrock → registry api_key providers 8. OpenRouter / bare-custom fallback target_model overrides model_cfg["default"] when computing provider-specific api_mode (e.g. @@ -930,8 +924,17 @@ def _ladder_rungs(requested_provider, explicit_api_key, explicit_base_url, targe explicit_api_key=explicit_api_key, explicit_base_url=explicit_base_url, target_model=target_model) yield _resolve_from_pool(provider, requested_provider, model_cfg, explicit_api_key, explicit_base_url, target_model) + swallowed_auth_error = None if provider in _OAUTH_RUNTIME_PROVIDERS: - yield _resolve_oauth_runtime(provider, requested_provider, model_cfg, target_model) + try: + yield _resolve_oauth_runtime(provider, requested_provider, model_cfg, target_model) + except AuthError as exc: + # Auto-detected login with stale/revoked/benched credentials: fall through to the env-var + # providers, but keep the error so a keyless fallback can still say what is wrong. + if requested_provider != "auto": + raise + logger.info("%s; falling through to next provider.", _OAUTH_RUNTIME_PROVIDERS[provider].failure_msg) + swallowed_auth_error = exc if provider == "minimax-oauth": yield _minimax_oauth_runtime(provider, requested_provider) if _is_external_process_provider(provider): @@ -943,7 +946,10 @@ def _ladder_rungs(requested_provider, explicit_api_key, explicit_base_url, targe pconfig = PROVIDER_REGISTRY.get(provider) if pconfig and pconfig.auth_type == "api_key": yield _api_key_provider_runtime(provider, pconfig, requested_provider, model_cfg, target_model) - yield _openrouter_fallback(requested_provider, explicit_api_key, explicit_base_url) + fallback = _openrouter_fallback(requested_provider, explicit_api_key, explicit_base_url) + if swallowed_auth_error is not None and not fallback.get("api_key"): + fallback["auth_error"] = swallowed_auth_error + yield fallback def format_runtime_provider_error(error: Exception) -> str: diff --git a/tests/hermes_cli/test_cli_first_run_setup.py b/tests/hermes_cli/test_cli_first_run_setup.py index d4738d6148..4912b3e59e 100644 --- a/tests/hermes_cli/test_cli_first_run_setup.py +++ b/tests/hermes_cli/test_cli_first_run_setup.py @@ -12,6 +12,7 @@ Covers: """ import importlib +import os import sys import types @@ -277,12 +278,33 @@ def test_empty_key_error_names_actual_provider(monkeypatch, capsys): # --------------------------------------------------------------------------- -def test_benched_credential_prints_cooldown_instead_of_wizard(monkeypatch, capsys): - """A profile whose only credential is cooling down is not a blank install: the startup notice - names the failure and the remaining cooldown, and the first-run wizard is not offered.""" +def _bench_nous_pool(monkeypatch, **entry_fields): import time from agent.credential_pool import STATUS_EXHAUSTED, CredentialPool, PooledCredential + benched = PooledCredential(id="e1", provider="nous", auth_type="oauth", access_token="x", + refresh_token="r", label="portal", source="manual:device_code", + priority=0, last_status=STATUS_EXHAUSTED, last_status_at=time.time() - 5, + **entry_fields) + pool = CredentialPool.__new__(CredentialPool) + monkeypatch.setattr(pool, "has_credentials", lambda: True, raising=False) + monkeypatch.setattr(pool, "has_available", lambda **kw: False, raising=False) + monkeypatch.setattr(pool, "next_available_at", lambda **kw: time.time() + 55, raising=False) + monkeypatch.setattr(pool, "entries", lambda: [benched], raising=False) + monkeypatch.setattr("agent.credential_pool.load_pool", lambda provider: pool) + + +def _forbid_wizard(monkeypatch, shell): + monkeypatch.setattr("hermes_cli.main.select_provider_and_model", + lambda: (_ for _ in ()).throw(AssertionError("wizard must not run"))) + monkeypatch.setattr(shell, "_offer_first_run_setup", + lambda: (_ for _ in ()).throw(AssertionError("wizard must not be offered"))) + + +def test_benched_credential_prints_cooldown_instead_of_wizard(monkeypatch, capsys): + """A profile whose only credential is cooling down is not a blank install: the interactive + startup gate prints the cooldown (with why and how long) as the headline, without telling the + user to re-authenticate, and never offers the first-run wizard.""" cli = _import_cli() shell = _make_shell(cli, monkeypatch) shell.requested_provider = "nous" @@ -292,37 +314,60 @@ def test_benched_credential_prints_cooldown_instead_of_wizard(monkeypatch, capsy code="nous_auth_missing", relogin_required=True) monkeypatch.setattr("hermes_cli.runtime_provider.resolve_runtime_provider", _raise) - benched = PooledCredential(id="e1", provider="nous", auth_type="oauth", access_token="x", - refresh_token="r", label="portal", source="manual:device_code", - priority=0, last_status=STATUS_EXHAUSTED, last_status_at=time.time() - 5) - pool = CredentialPool.__new__(CredentialPool) - monkeypatch.setattr(pool, "has_credentials", lambda: True, raising=False) - monkeypatch.setattr(pool, "has_available", lambda **kw: False, raising=False) - monkeypatch.setattr(pool, "next_available_at", lambda **kw: time.time() + 55, raising=False) - monkeypatch.setattr(pool, "entries", lambda: [benched], raising=False) - monkeypatch.setattr("agent.credential_pool.load_pool", lambda provider: pool) - monkeypatch.setattr("hermes_cli.main.select_provider_and_model", - lambda: (_ for _ in ()).throw(AssertionError("wizard must not run"))) + _bench_nous_pool(monkeypatch, last_error_code=429, last_error_reason="rate_limited") + _forbid_wizard(monkeypatch, shell) + monkeypatch.setattr(sys.stdin, "isatty", lambda: True) + + shell._maybe_offer_first_run_setup() - ready, error = shell._probe_runtime_credentials() - assert ready is False - assert shell._explain_unusable_credentials(error) is True out = capsys.readouterr().out assert "No inference provider is configured yet" not in out + headline = next(line for line in out.splitlines() if line.strip()) + assert "cooling down after a rate-limit or quota response" in headline and "about 1m" in headline + assert "failed token refresh" not in out assert "not logged into Nous Portal" in out - assert "cooling down" in out and "about 1m" in out + assert "re-authenticate" not in out and "hermes model" not in out -def test_nothing_configured_still_reaches_wizard(monkeypatch): - """Control: the resolver's ``no_provider_configured`` is the blank-install signal — nothing - to explain, so the first-run wizard is offered as before.""" +def test_auth_json_only_login_explains_instead_of_wizard(monkeypatch, capsys, tmp_path): + """auth.json-only shape: logged into Nous but no ``model.provider`` (requested "auto"). The + ladder swallows the AuthError and falls through to a keyless OpenRouter fallback; the gate + must still explain the real failure rather than treat the profile as a blank install. + Control: the resolver's ``no_provider_configured`` still reaches the wizard.""" + import dataclasses + + import hermes_cli.runtime_provider as rp + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + (tmp_path / "config.yaml").write_text("model:\n default: some-model\n", encoding="utf-8") + for key in [k for k in os.environ if k.endswith("_API_KEY")]: + monkeypatch.delenv(key, raising=False) + + def _nous_fail(): + raise AuthError("Hermes is not logged into Nous Portal.", provider="nous", + code="nous_auth_missing", relogin_required=True) + + monkeypatch.setattr(rp, "resolve_provider", lambda *a, **kw: "nous") + monkeypatch.setitem(rp._OAUTH_RUNTIME_PROVIDERS, "nous", + dataclasses.replace(rp._OAUTH_RUNTIME_PROVIDERS["nous"], resolve=_nous_fail)) + cli = _import_cli() shell = _make_shell(cli, monkeypatch) + shell.requested_provider = "auto" + shell._explicit_api_key = None + shell._explicit_base_url = None + _forbid_wizard(monkeypatch, shell) + monkeypatch.setattr(sys.stdin, "isatty", lambda: True) - def _raise(**kwargs): - raise AuthError("Hermes is not connected to any AI provider yet.", code="no_provider_configured") + shell._maybe_offer_first_run_setup() + out = capsys.readouterr().out + assert "not logged into Nous Portal" in out + assert "No inference provider is configured yet" not in out - monkeypatch.setattr("hermes_cli.runtime_provider.resolve_runtime_provider", _raise) - ready, error = shell._probe_runtime_credentials() - assert ready is False - assert shell._explain_unusable_credentials(error) is False + offered = [] + monkeypatch.setattr(shell, "_offer_first_run_setup", lambda: offered.append(True) or True) + monkeypatch.setattr("hermes_cli.runtime_provider.resolve_runtime_provider", lambda **kw: (_ for _ in ()).throw( + AuthError("Hermes is not connected to any AI provider yet.", code="no_provider_configured"))) + shell._maybe_offer_first_run_setup() + assert offered == [True] + assert "not logged into" not in capsys.readouterr().out