mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-06-23 10:42:00 +00:00
fix(spotify): quarantine dead tokens on terminal refresh failure
resolve_spotify_runtime_credentials() called _refresh_spotify_oauth_state() without a try/except, so a terminal failure (HTTP 400/401, invalid_grant, refresh_token_reused) raised AuthError but left the dead refresh_token in auth.json. Every subsequent session re-read and retried the same token over the network, failing identically each time. Fix: wrap the refresh call and, when exc.relogin_required is True and a refresh_token is present, clear the dead OAuth fields (access_token, refresh_token, expires_at, expires_in, obtained_at) and write a last_auth_error quarantine marker to auth.json before re-raising. The next call sees no access_token and fails fast with spotify_access_token_missing — no network retry — and the user is prompted to re-authenticate. Mirrors the quarantine pattern already in place for Nous, xAI-OAuth, Codex-OAuth (#28116, #28118), and MiniMax-OAuth (#28119).
This commit is contained in:
parent
242962e1f5
commit
9bd5003d4f
2 changed files with 144 additions and 3 deletions
|
|
@ -2899,9 +2899,31 @@ def resolve_spotify_runtime_credentials(
|
|||
if not should_refresh and refresh_if_expiring:
|
||||
should_refresh = _is_expiring(state.get("expires_at"), refresh_skew_seconds)
|
||||
if should_refresh:
|
||||
state = _refresh_spotify_oauth_state(state)
|
||||
_store_provider_state(auth_store, "spotify", state, set_active=False)
|
||||
_save_auth_store(auth_store)
|
||||
try:
|
||||
state = _refresh_spotify_oauth_state(state)
|
||||
_store_provider_state(auth_store, "spotify", state, set_active=False)
|
||||
_save_auth_store(auth_store)
|
||||
except AuthError as exc:
|
||||
if exc.relogin_required and state.get("refresh_token"):
|
||||
# Terminal refresh failure — clear dead tokens from auth.json
|
||||
# so subsequent calls fail fast without a network retry.
|
||||
# Mirrors the Nous / xAI-OAuth / Codex-OAuth / MiniMax pattern.
|
||||
for _k in ("access_token", "refresh_token", "expires_at", "expires_in", "obtained_at"):
|
||||
state.pop(_k, None)
|
||||
state["last_auth_error"] = {
|
||||
"provider": "spotify",
|
||||
"code": exc.code or "refresh_failed",
|
||||
"message": str(exc),
|
||||
"reason": "runtime_refresh_failure",
|
||||
"relogin_required": True,
|
||||
"at": datetime.now(timezone.utc).isoformat(),
|
||||
}
|
||||
try:
|
||||
_store_provider_state(auth_store, "spotify", state, set_active=False)
|
||||
_save_auth_store(auth_store)
|
||||
except Exception as _save_exc:
|
||||
logger.debug("Spotify OAuth: failed to persist quarantined state: %s", _save_exc)
|
||||
raise
|
||||
|
||||
access_token = str(state.get("access_token", "") or "").strip()
|
||||
if not access_token:
|
||||
|
|
|
|||
|
|
@ -5,6 +5,7 @@ from types import SimpleNamespace
|
|||
import pytest
|
||||
|
||||
from hermes_cli import auth as auth_mod
|
||||
from hermes_cli.auth import AuthError, resolve_spotify_runtime_credentials
|
||||
|
||||
|
||||
def test_store_provider_state_can_skip_active_provider() -> None:
|
||||
|
|
@ -181,3 +182,121 @@ def test_spotify_interactive_setup_empty_aborts(
|
|||
env_path = tmp_path / ".env"
|
||||
if env_path.exists():
|
||||
assert "HERMES_SPOTIFY_CLIENT_ID" not in env_path.read_text()
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Quarantine: terminal refresh failure clears dead tokens (#28139)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
_STALE_SPOTIFY_STATE = {
|
||||
"client_id": "test-client",
|
||||
"redirect_uri": "http://127.0.0.1:43827/spotify/callback",
|
||||
"api_base_url": auth_mod.DEFAULT_SPOTIFY_API_BASE_URL,
|
||||
"accounts_base_url": auth_mod.DEFAULT_SPOTIFY_ACCOUNTS_BASE_URL,
|
||||
"scope": auth_mod.DEFAULT_SPOTIFY_SCOPE,
|
||||
"granted_scope": auth_mod.DEFAULT_SPOTIFY_SCOPE,
|
||||
"token_type": "Bearer",
|
||||
"access_token": "dead-access-token",
|
||||
"refresh_token": "dead-refresh-token",
|
||||
"expires_at": "2000-01-01T00:00:00+00:00",
|
||||
"expires_in": 3600,
|
||||
"obtained_at": "2000-01-01T00:00:00+00:00",
|
||||
"auth_type": "oauth_pkce",
|
||||
}
|
||||
|
||||
|
||||
def _seed_spotify_state(tmp_path, state: dict) -> None:
|
||||
with auth_mod._auth_store_lock():
|
||||
store = auth_mod._load_auth_store()
|
||||
store["active_provider"] = "nous"
|
||||
auth_mod._store_provider_state(store, "spotify", state, set_active=False)
|
||||
auth_mod._save_auth_store(store)
|
||||
|
||||
|
||||
def test_resolve_credentials_quarantines_dead_tokens_on_terminal_refresh_failure(
|
||||
tmp_path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""Terminal refresh failure (relogin_required=True + refresh_token present)
|
||||
must clear access_token/refresh_token/expires_* from auth.json and write a
|
||||
last_auth_error marker so subsequent calls fail fast without a network retry.
|
||||
Mirrors Nous / xAI-OAuth / Codex-OAuth / MiniMax quarantine pattern.
|
||||
"""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
_seed_spotify_state(tmp_path, dict(_STALE_SPOTIFY_STATE))
|
||||
|
||||
def _terminal_refresh(_state, **_kw):
|
||||
raise AuthError(
|
||||
"Spotify token refresh failed. Run `hermes auth spotify` again.",
|
||||
provider="spotify",
|
||||
code="spotify_refresh_failed",
|
||||
relogin_required=True,
|
||||
)
|
||||
|
||||
monkeypatch.setattr(auth_mod, "_refresh_spotify_oauth_state", _terminal_refresh)
|
||||
|
||||
with pytest.raises(AuthError) as exc_info:
|
||||
resolve_spotify_runtime_credentials(force_refresh=True)
|
||||
|
||||
assert exc_info.value.code == "spotify_refresh_failed"
|
||||
assert exc_info.value.relogin_required is True
|
||||
|
||||
persisted = auth_mod.get_provider_auth_state("spotify")
|
||||
assert persisted is not None
|
||||
|
||||
# Dead OAuth fields must be cleared.
|
||||
assert "access_token" not in persisted
|
||||
assert "refresh_token" not in persisted
|
||||
assert "expires_at" not in persisted
|
||||
assert "expires_in" not in persisted
|
||||
assert "obtained_at" not in persisted
|
||||
|
||||
# Non-credential metadata must be preserved.
|
||||
assert persisted["client_id"] == "test-client"
|
||||
assert persisted["api_base_url"] == auth_mod.DEFAULT_SPOTIFY_API_BASE_URL
|
||||
assert persisted["accounts_base_url"] == auth_mod.DEFAULT_SPOTIFY_ACCOUNTS_BASE_URL
|
||||
|
||||
# Structured diagnostic blob must be written.
|
||||
err = persisted.get("last_auth_error")
|
||||
assert isinstance(err, dict)
|
||||
assert err["provider"] == "spotify"
|
||||
assert err["code"] == "spotify_refresh_failed"
|
||||
assert err["reason"] == "runtime_refresh_failure"
|
||||
assert err["relogin_required"] is True
|
||||
assert "at" in err
|
||||
|
||||
# Active provider must be unchanged.
|
||||
assert auth_mod.get_active_provider() == "nous"
|
||||
|
||||
|
||||
def test_resolve_credentials_does_not_quarantine_on_transient_refresh_failure(
|
||||
tmp_path,
|
||||
monkeypatch: pytest.MonkeyPatch,
|
||||
) -> None:
|
||||
"""Transient refresh failure (relogin_required=False, e.g. 429 / 5xx) must
|
||||
NOT trigger the quarantine path — tokens stay on disk for the next attempt.
|
||||
"""
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
|
||||
_seed_spotify_state(tmp_path, dict(_STALE_SPOTIFY_STATE))
|
||||
|
||||
def _transient_refresh(_state, **_kw):
|
||||
raise AuthError(
|
||||
"Spotify token refresh failed: connection error",
|
||||
provider="spotify",
|
||||
code="spotify_refresh_failed",
|
||||
relogin_required=False,
|
||||
)
|
||||
|
||||
monkeypatch.setattr(auth_mod, "_refresh_spotify_oauth_state", _transient_refresh)
|
||||
|
||||
with pytest.raises(AuthError) as exc_info:
|
||||
resolve_spotify_runtime_credentials(force_refresh=True)
|
||||
|
||||
assert exc_info.value.relogin_required is False
|
||||
|
||||
# Tokens must be untouched — no quarantine on transient errors.
|
||||
persisted = auth_mod.get_provider_auth_state("spotify")
|
||||
assert persisted is not None
|
||||
assert persisted["refresh_token"] == "dead-refresh-token"
|
||||
assert persisted["access_token"] == "dead-access-token"
|
||||
assert "last_auth_error" not in persisted
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue