From 665a42bb08b53b2fef6134675b74738f367d3ef6 Mon Sep 17 00:00:00 2001 From: Brooklyn Nicholson Date: Mon, 5 Oct 2026 20:11:21 -0500 Subject: [PATCH] fix(auth): serialize MiniMax OAuth refresh and keep it a credential-only write teknium1's request-changes review of #133534, all four findings plus nits: 1. Concurrent refresh logged the user out (major): _minimax_fresh_state now holds _auth_store_lock across read -> refresh -> save and re-reads state inside the lock, so a second concurrent consumer (main agent + aux task) sees the first caller's new expiry instead of replaying the single-use refresh token into refresh_token_reused and quarantining the winner's fresh tokens. Lock timeout outlives the refresh POST (codex pattern). 2. Aux refresh overwrote active_provider (medium): _minimax_save_auth_state gains set_active; the refresh and quarantine paths persist with set_active=False (the _save_provider_state_to_source rule), only _minimax_oauth_login sets active. 3. Warning spam when logged out (medium, #21521's complaint): the arm's expected absent/not-logged-in/AuthError fall-throughs are debug-once (contract test_auxiliary_client_resolve_dedup.py); only a genuinely unexpected exception keeps exc_info, logged once. 4. Probe mode made a network call (minor): under _aux_probe_active() the arm reads get_provider_auth_state("minimax-oauth") raw and stubs when access_token + inference_base_url are present, skipping the refresh (mirrors credential_pool.py::_seed_minimax_singleton). Nits: the arm body lives directly in auxiliary_client_registry.py (one new module, agent/auxiliary_client_minimax.py deleted), the in-tree try/except ImportError wrappers are gone (only the build_anthropic_client SDK-absent boundary remains, matching _try_anthropic), and the misnamed ..._with_oauth_semantics test is now ..._third_party_wire. Tests: concurrent loopback-portal refresh race (1 POST, 0 errors, tokens intact), aux refresh preserves active_provider, login still sets it, logged-out resolutions are debug-once without traceback, probe mode with an expired token makes 0 HTTP calls. Co-authored-by: Ubxty --- agent/auxiliary_client_minimax.py | 60 ----- agent/auxiliary_client_registry.py | 91 +++++++- hermes_cli/auth_minimax.py | 63 +++-- .../test_auxiliary_client_minimax_oauth.py | 82 ++++++- tests/hermes_cli/test_minimax_oauth.py | 217 +++++++++++++++++- 5 files changed, 424 insertions(+), 89 deletions(-) delete mode 100644 agent/auxiliary_client_minimax.py diff --git a/agent/auxiliary_client_minimax.py b/agent/auxiliary_client_minimax.py deleted file mode 100644 index 447879b94b..0000000000 --- a/agent/auxiliary_client_minimax.py +++ /dev/null @@ -1,60 +0,0 @@ -"""MiniMax OAuth arm of the auxiliary registry branch (``auth_type == "oauth_minimax"``). - -MiniMax OAuth resolves to an Anthropic-compatible inference endpoint with a callable -bearer: tokens live ~15 minutes and the Anthropic SDK re-invokes the provider on each -request, so a static string would 401 mid-session. ``is_oauth`` comes from -``anthropic_route_is_oauth`` — the MiniMax host is a third-party Anthropic-protocol -endpoint, so the wrapper must NOT carry the native Claude Code OAuth identity (mcp__ -tool-name wire transforms, identity rewrites, response prefix stripping); those are -api.anthropic.com-only (#114967). -""" -from __future__ import annotations - -import logging -from typing import TYPE_CHECKING - -if TYPE_CHECKING: - from agent.auxiliary_client import _ResolveRequest, _ResolveResult - -logger = logging.getLogger(__name__) - - -def resolve_minimax_oauth_client(req: _ResolveRequest) -> _ResolveResult: - """``minimax-oauth`` → AnthropicAuxiliaryClient over a callable bearer; (None, None) when absent.""" - from agent.auxiliary_client import ( - AnthropicAuxiliaryClient, _AuxProbeClientStub, _aux_probe_active, - _get_aux_model_for_provider, _normalize_resolved_model, _route_client, - ) - try: - from agent.anthropic_adapter import build_anthropic_client - from agent.anthropic_credentials import anthropic_route_is_oauth - from hermes_cli.auth import resolve_minimax_oauth_runtime_credentials - except ImportError: - return None, None - try: - credentials = resolve_minimax_oauth_runtime_credentials(as_token_provider=True) - except Exception as exc: - # Deliberate boundary: the resolver contract is "absent → (None, None), never an - # exception" (the aux ladder then falls through to its Step-2 chain), so every - # failure ends the arm here; exc_info keeps the traceback diagnosable. - logger.warning( - "resolve_provider_client: minimax-oauth runtime resolution failed: %s", exc, - exc_info=True) - return None, None - token_provider = credentials.get("api_key") - base_url = str(credentials.get("base_url") or "").strip().rstrip("/") - if not callable(token_provider) or not base_url: - return None, None - final_model = _normalize_resolved_model( - req.model or _get_aux_model_for_provider(req.provider) or "MiniMax-M3", req.provider, - ) - if _aux_probe_active(): - return _AuxProbeClientStub(api_key="", base_url=base_url), final_model - try: - real_client = build_anthropic_client(token_provider, base_url) - except ImportError: - return None, None - client = AnthropicAuxiliaryClient( - real_client, final_model, token_provider, base_url, - is_oauth=anthropic_route_is_oauth(base_url, token_provider)) - return _route_client(req, client, final_model) diff --git a/agent/auxiliary_client_registry.py b/agent/auxiliary_client_registry.py index 31caa0b0c6..ec344e687b 100644 --- a/agent/auxiliary_client_registry.py +++ b/agent/auxiliary_client_registry.py @@ -8,11 +8,19 @@ facade (siblings never form a module-level cycle) so the seam every test patches """ from __future__ import annotations +import logging from typing import TYPE_CHECKING, Callable, Dict if TYPE_CHECKING: from agent.auxiliary_client import _ResolveRequest, _ResolveResult +logger = logging.getLogger("agent.auxiliary_client") + +# First occurrence surfaces for diagnostics; per-call retries stay silent (the +# contract test_auxiliary_client_resolve_dedup.py pins for every fall-through). +_LOGGED_MINIMAX_ABSENT_KEYS: set = set() +_LOGGED_MINIMAX_UNEXPECTED_KEYS: set = set() + def _resolve_vertex_arm(req: _ResolveRequest) -> _ResolveResult: from agent.auxiliary_client import _build_vertex_client, _route_client @@ -26,14 +34,91 @@ def _resolve_bedrock_arm(req: _ResolveRequest) -> _ResolveResult: return _route_client(req, client, final_model) if client is not None else (None, None) +def _log_once_debug(seen: set, key: str, msg: str, *args: object) -> None: + if key not in seen: + seen.add(key) + logger.debug(msg, *args) + + def _resolve_minimax_oauth_arm(req: _ResolveRequest) -> _ResolveResult: - from agent.auxiliary_client_minimax import resolve_minimax_oauth_client - return resolve_minimax_oauth_client(req) + """``minimax-oauth`` → AnthropicAuxiliaryClient over a callable bearer; (None, None) when absent. + + MiniMax OAuth resolves to an Anthropic-compatible inference endpoint with a callable + bearer: tokens live ~15 minutes and the Anthropic SDK re-invokes the provider on each + request, so a static string would 401 mid-session. ``is_oauth`` comes from + ``anthropic_route_is_oauth`` — the MiniMax host is a third-party Anthropic-protocol + endpoint, so the wrapper must NOT carry the native Claude Code OAuth identity (mcp__ + tool-name wire transforms, identity rewrites, response prefix stripping); those are + api.anthropic.com-only (#114967). + """ + from hermes_cli.auth_constants import AuthError + + from agent.auxiliary_client import ( + AnthropicAuxiliaryClient, _AuxProbeClientStub, _aux_probe_active, + _get_aux_model_for_provider, _normalize_resolved_model, _route_client, + ) + from agent.anthropic_credentials import anthropic_route_is_oauth + from hermes_cli.auth import get_provider_auth_state, resolve_minimax_oauth_runtime_credentials + + # Probe mode answers "resolvable?" for availability gates and must not touch the + # network: read the raw persisted state (access_token + inference_base_url present → + # stub) and skip the refresh an expired token would otherwise trigger (mirrors + # credential_pool.py::_seed_minimax_singleton). + if _aux_probe_active(): + state = get_provider_auth_state("minimax-oauth") + base_url = str((state or {}).get("inference_base_url") or "").strip().rstrip("/") + if not ((state or {}).get("access_token") and base_url): + _log_once_debug(_LOGGED_MINIMAX_ABSENT_KEYS, "minimax-oauth", + "resolve_provider_client: minimax-oauth not logged in") + return None, None + final_model = _normalize_resolved_model( + req.model or _get_aux_model_for_provider(req.provider) or "MiniMax-M3", req.provider, + ) + return _route_client(req, _AuxProbeClientStub(api_key="", base_url=base_url), final_model) + + try: + credentials = resolve_minimax_oauth_runtime_credentials(as_token_provider=True) + except AuthError as exc: + # Expected absent/not-logged-in/quarantined states: the resolver contract is + # "absent → (None, None), never an exception" (the aux ladder then falls through + # to its Step-2 chain). Debug-once, like every other fall-through — a per-call + # WARNING would recreate the #21521 log spam once a quarantine wipes the tokens. + _log_once_debug(_LOGGED_MINIMAX_ABSENT_KEYS, "minimax-oauth", + "resolve_provider_client: minimax-oauth unavailable: %s", exc) + return None, None + except Exception as exc: + # Deliberate boundary (BLE001): a genuinely unexpected failure still ends the arm + # with (None, None) — never an exception into the aux ladder — but keeps the + # traceback, logged once. + if "minimax-oauth" not in _LOGGED_MINIMAX_UNEXPECTED_KEYS: + _LOGGED_MINIMAX_UNEXPECTED_KEYS.add("minimax-oauth") + logger.warning( + "resolve_provider_client: unexpected minimax-oauth resolution failure: %s", exc, + exc_info=True) + return None, None + token_provider = credentials.get("api_key") + base_url = str(credentials.get("base_url") or "").strip().rstrip("/") + if not callable(token_provider) or not base_url: + _log_once_debug(_LOGGED_MINIMAX_ABSENT_KEYS, "minimax-oauth", + "resolve_provider_client: minimax-oauth credentials incomplete") + return None, None + final_model = _normalize_resolved_model( + req.model or _get_aux_model_for_provider(req.provider) or "MiniMax-M3", req.provider, + ) + from agent.anthropic_adapter import build_anthropic_client # SDK-absent boundary, like _try_anthropic + try: + real_client = build_anthropic_client(token_provider, base_url) + except ImportError: + return None, None + client = AnthropicAuxiliaryClient( + real_client, final_model, token_provider, base_url, + is_oauth=anthropic_route_is_oauth(base_url, token_provider)) + return _route_client(req, client, final_model) # ``api_key`` / ``external_process`` arms stay on the facade (they share its credential # resolvers); the table maps the auth types whose arms are build-and-route one-liners -# or topical siblings. +# or live here as topical arms. REGISTRY_AUTHTYPE_ARMS: Dict[str, Callable[[_ResolveRequest], _ResolveResult]] = { "vertex": _resolve_vertex_arm, "aws_sdk": _resolve_bedrock_arm, diff --git a/hermes_cli/auth_minimax.py b/hermes_cli/auth_minimax.py index 630b620a9b..0b84d31e31 100644 --- a/hermes_cli/auth_minimax.py +++ b/hermes_cli/auth_minimax.py @@ -158,10 +158,19 @@ def _minimax_poll_token( raise _minimax_err("MiniMax OAuth timed out before authorization completed.", "timeout") -def _minimax_save_auth_state(auth_state: Dict[str, Any]) -> None: - """Persist MiniMax OAuth state to Hermes auth store (~/.hermes/auth.json).""" - from hermes_cli.auth import _save_active_provider_state - _save_active_provider_state("minimax-oauth", auth_state) +def _minimax_save_auth_state(auth_state: Dict[str, Any], *, set_active: bool = True) -> None: + """Persist MiniMax OAuth state to Hermes auth store (~/.hermes/auth.json). + + ``set_active=False`` rewrites credentials without making minimax-oauth the + active provider (``_save_provider_state_to_source``'s rule: a token refresh + rewrites credentials, not the user's choice of provider); only + ``_minimax_oauth_login`` — the user just chose the provider — sets active. + """ + from hermes_cli.auth import _auth_store_lock, _load_auth_store, _save_auth_store, _store_provider_state + with _auth_store_lock(): + auth_store = _load_auth_store() + _store_provider_state(auth_store, "minimax-oauth", auth_state, set_active=set_active) + _save_auth_store(auth_store) def _minimax_oauth_login(*, region: str = "global", open_browser: bool = True, timeout_seconds: float = 15.0) -> Dict[str, Any]: @@ -261,7 +270,9 @@ def _refresh_minimax_oauth_state(state: Dict[str, Any], *, timeout_seconds: floa "refresh_token": payload.get("refresh_token", state["refresh_token"]), **_minimax_expiry_fields(payload["expired_in"]), } - _minimax_save_auth_state(new_state) + # set_active=False: an aux-triggered refresh rewrites credentials, not the user's + # choice of provider (same rule as _save_provider_state_to_source). + _minimax_save_auth_state(new_state, set_active=False) return new_state @@ -272,24 +283,40 @@ def _minimax_oauth_quarantine_on_terminal_refresh(state: Dict[str, Any], exc: Au return _quarantine_flat_oauth_state(state, "minimax-oauth", exc) try: - _minimax_save_auth_state(state) + # set_active=False: wiping dead tokens is hygiene, not a provider choice. + _minimax_save_auth_state(state, set_active=False) except Exception as _save_exc: logger.debug("MiniMax OAuth: failed to persist quarantined state: %s", _save_exc) def _minimax_fresh_state() -> Dict[str, Any]: - """Load the MiniMax OAuth state and refresh it if near expiry; quarantine on terminal failure.""" - from hermes_cli.auth import _refresh_minimax_oauth_state, get_provider_auth_state - state = get_provider_auth_state("minimax-oauth") - if not state or not state.get("access_token"): - raise _minimax_err( - "Not logged into MiniMax OAuth. Run `hermes model` and select MiniMax (OAuth).", "not_logged_in", relogin=True, - ) - try: - return _refresh_minimax_oauth_state(state) - except AuthError as exc: - _minimax_oauth_quarantine_on_terminal_refresh(state, exc) - raise + """Load the MiniMax OAuth state and refresh it if near expiry; quarantine on terminal failure. + + The whole read → refresh → save runs inside ``_auth_store_lock`` (reentrant, so the + inner ``_minimax_save_auth_state`` re-locks harmlessly): MiniMax refresh tokens are + single-use and rotate, so two concurrent consumers (main agent + an aux task) that + both see a near-expiry token would otherwise race — the loser replays the consumed + token, gets ``refresh_token_reused``, and the quarantine wipes the winner's fresh + tokens. Re-reading inside the lock makes the second caller see the first caller's + new expiry and skip its own refresh. The lock timeout outlives the refresh POST so + a waiter adopts the rotated pair instead of timing out (codex pattern). + """ + from hermes_cli.auth import ( + AUTH_LOCK_TIMEOUT_SECONDS, _auth_store_lock, _refresh_minimax_oauth_state, + get_provider_auth_state, + ) + with _auth_store_lock(timeout_seconds=max(AUTH_LOCK_TIMEOUT_SECONDS, 15.0 + 5.0)): + state = get_provider_auth_state("minimax-oauth") + if not state or not state.get("access_token"): + raise _minimax_err( + "Not logged into MiniMax OAuth. Run `hermes model` and select MiniMax (OAuth).", + "not_logged_in", relogin=True, + ) + try: + return _refresh_minimax_oauth_state(state) + except AuthError as exc: + _minimax_oauth_quarantine_on_terminal_refresh(state, exc) + raise def build_minimax_oauth_token_provider() -> Callable[[], str]: diff --git a/tests/agent/test_auxiliary_client_minimax_oauth.py b/tests/agent/test_auxiliary_client_minimax_oauth.py index bcb8766158..2ea38479da 100644 --- a/tests/agent/test_auxiliary_client_minimax_oauth.py +++ b/tests/agent/test_auxiliary_client_minimax_oauth.py @@ -18,12 +18,19 @@ bearer mints a fresh access token per outbound request because MiniMax's tokens MiniMax host is a third-party Anthropic-protocol endpoint, so the wrapper must NOT carry the Claude Code OAuth identity (mcp__ tool-name wire transforms, system-prompt rewrites, response prefix stripping) — those are native api.anthropic.com-only (#114967). +The arm lives in ``agent/auxiliary_client_registry.py`` (one new module, not a +trampoline into a second sibling); absent/not-logged-in credentials follow the +``_log_once_debug`` contract (``test_auxiliary_client_resolve_dedup.py``) so a logged-out +user gets one debug line, not a WARNING-with-traceback per aux resolution (#21521). """ from __future__ import annotations +import logging from types import SimpleNamespace from unittest.mock import MagicMock, patch +from hermes_cli.auth_constants import AuthError + INFERENCE_BASE_URL = "https://api.minimax.io/anthropic" @@ -37,7 +44,7 @@ def _runtime_creds(): } -def test_resolve_minimax_oauth_builds_anthropic_wrapper_with_oauth_semantics(): +def test_resolve_minimax_oauth_builds_anthropic_wrapper_with_third_party_wire(): """Happy path: token-provider + base_url → AnthropicAuxiliaryClient with the third-party is_oauth invariant (False for api.minimax.io), Anthropic SDK built with the callable bearer (not a static string), and the resolved model passed @@ -134,20 +141,83 @@ def test_resolve_minimax_oauth_tool_names_unprefixed_on_wire(): ) -def test_resolve_minimax_oauth_missing_credentials_returns_none_without_raising(): - """AuthError from the runtime resolver → (None, None), no exception. +def test_resolve_minimax_oauth_missing_credentials_log_once_debug(caplog): + """Absent / not-logged-in / AuthError → (None, None), no exception, and one + DEBUG record per process — never a per-call WARNING with traceback. The resolver contract is "absent → call_llm's fallback chain", never "absent → exception"; the compression step must fall through to its - Step-2 providers instead of crashing the turn. + Step-2 providers instead of crashing the turn. And because every aux task + (compression, title generation, background review) re-resolves on each + call, a per-call WARNING would recreate the exact log-spam complaint + #21521 was filed about — worse once a quarantine wipes the tokens. """ + import agent.auxiliary_client_registry as acr from agent.auxiliary_client import resolve_provider_client + acr._LOGGED_MINIMAX_ABSENT_KEYS.clear() + acr._LOGGED_MINIMAX_UNEXPECTED_KEYS.clear() + resolved: tuple = (None, None) with patch( "hermes_cli.auth.resolve_minimax_oauth_runtime_credentials", - side_effect=Exception("not logged in"), + side_effect=AuthError( + "Not logged into MiniMax OAuth.", provider="minimax-oauth", + code="not_logged_in", relogin_required=True), ): - client, model = resolve_provider_client("minimax-oauth", "MiniMax-M3") + with caplog.at_level(logging.DEBUG, logger="agent.auxiliary_client"): + for _ in range(5): + resolved = resolve_provider_client("minimax-oauth", "MiniMax-M3") + + client, model = resolved assert client is None assert model is None + recs = [r for r in caplog.records if "minimax-oauth" in r.getMessage()] + # Five resolutions → exactly one debug record, zero warnings, zero tracebacks. + assert len(recs) == 1, ( + f"expected exactly one deduped log record, got {len(recs)}: " + f"{[r.getMessage() for r in recs]}" + ) + assert recs[0].levelno == logging.DEBUG, ( + "logged-out minimax-oauth must be debug-once, not a warning" + ) + assert not any(r.levelno >= logging.WARNING for r in caplog.records), ( + "logged-out minimax-oauth must not warn per aux resolution (#21521 spam)" + ) + assert not any(r.exc_info for r in caplog.records), ( + "an expected not-logged-in state never carries a traceback" + ) + + +def test_resolve_minimax_oauth_probe_reads_raw_state_no_refresh(): + """``aux_probe_mode()`` answers "resolvable?" for availability gates and must + not touch the network: the arm reads ``get_provider_auth_state("minimax-oauth")`` + raw and stubs when access_token + inference_base_url are present, skipping the + refresh that an expired token would otherwise trigger (mirrors + ``credential_pool.py::_seed_minimax_singleton``). + """ + import agent.auxiliary_client as aux + from agent.auxiliary_client import resolve_provider_client + + expired_state = { + "access_token": "expired-but-present", + "refresh_token": "rt", + "expires_at": "2001-01-01T00:00:00+00:00", # long expired + "inference_base_url": INFERENCE_BASE_URL, + } + + with patch( + "hermes_cli.auth.get_provider_auth_state", return_value=expired_state, + ) as mock_state, patch( + "hermes_cli.auth.resolve_minimax_oauth_runtime_credentials", + ) as mock_resolve: + with aux.aux_probe_mode(): + client, model = resolve_provider_client("minimax-oauth", "MiniMax-M3") + + assert model == "MiniMax-M3" + assert client is not None and isinstance(client, aux._AuxProbeClientStub), ( + "probe mode must stub minimax-oauth from the raw persisted state" + ) + assert client.base_url == INFERENCE_BASE_URL + mock_resolve.assert_not_called() + mock_state.assert_called_once_with("minimax-oauth") diff --git a/tests/hermes_cli/test_minimax_oauth.py b/tests/hermes_cli/test_minimax_oauth.py index 9dcfa61e45..705a8a88f2 100644 --- a/tests/hermes_cli/test_minimax_oauth.py +++ b/tests/hermes_cli/test_minimax_oauth.py @@ -13,7 +13,9 @@ from __future__ import annotations import base64 import hashlib import json +import threading import time +import urllib.parse from datetime import datetime, timezone from unittest.mock import MagicMock, patch @@ -210,7 +212,7 @@ def test_resolve_credentials_quarantines_dead_tokens_on_terminal_refresh_failure } saved_states = [] - def _capture_save(s): + def _capture_save(s, **_kwargs): saved_states.append(dict(s)) def _terminal_refresh(_state): @@ -399,7 +401,7 @@ def test_token_provider_quarantines_state_on_terminal_refresh(): patch("httpx.Client") as mock_client_class, \ patch( "hermes_cli.auth._minimax_save_auth_state", - side_effect=lambda s: saved_states.append(dict(s)), + side_effect=lambda s, **_k: saved_states.append(dict(s)), ): mock_instance = MagicMock() mock_instance.__enter__ = MagicMock(return_value=mock_instance) @@ -494,3 +496,214 @@ def test_refresh_error_body_bounded_and_readable_with_real_client(): # Bounded: 16KB limit + truncation marker, never the full 64KB body. assert len(msg) < 20 * 1024 assert "...[truncated]" in msg + + +# --------------------------------------------------------------------------- +# Concurrent refresh + active-provider invariants (teknium1 review of #133534) +# --------------------------------------------------------------------------- + +def _minimax_state(tmp_path, *, expires_in: int = 30, refresh_token: str = "r1", + access_token: str = "tok1") -> dict: + from hermes_cli.auth import MINIMAX_OAUTH_CLIENT_ID + + state = { + "provider": "minimax-oauth", + "region": "global", + "portal_base_url": "https://portal.minimax.io", + "inference_base_url": "https://api.minimax.io/anthropic", + "client_id": MINIMAX_OAUTH_CLIENT_ID, + "token_type": "Bearer", + "access_token": access_token, + "refresh_token": refresh_token, + **_minimax_expiry_fields_for_test(expires_in), + } + (tmp_path / "auth.json").write_text(json.dumps({ + "version": 1, "active_provider": "nous", + "providers": {"minimax-oauth": state}, + }), encoding="utf-8") + return state + + +def _minimax_expiry_fields_for_test(expires_in: int) -> dict: + from hermes_cli.auth_minimax import _minimax_expiry_fields + + return _minimax_expiry_fields(expires_in) + + +class _RotatingPortal: + """Loopback MiniMax portal: rotates the refresh token on each successful + refresh and rejects reuse of a consumed token (``refresh_token_reused``), + the upstream behavior our quarantine treats as relogin-required.""" + + def __init__(self): + import http.server + import socketserver + + self.refresh_calls: list[str] = [] + self.reuse_rejections = 0 + self._current_refresh = "r1" + self._lock = threading.Lock() + portal = self + + class Handler(http.server.BaseHTTPRequestHandler): + def do_POST(self): + length = int(self.headers.get("Content-Length", "0")) + body = urllib.parse.parse_qs(self.rfile.read(length).decode("utf-8")) + used = (body.get("refresh_token") or [""])[0] + with portal._lock: + portal.refresh_calls.append(used) + if used != portal._current_refresh: + portal.reuse_rejections += 1 + status, payload = 400, {"base_resp": {"status_msg": "refresh_token_reused"}} + else: + portal._current_refresh = "r" + str(int(portal._current_refresh[1:]) + 1) + status, payload = 200, { + "status": "success", + "access_token": f"tok-{portal._current_refresh}", + "refresh_token": portal._current_refresh, + "expired_in": 900, + } + raw = json.dumps(payload).encode("utf-8") + self.send_response(status) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(raw))) + self.end_headers() + self.wfile.write(raw) + + def log_message(self, format, *args): + pass + + self._server = socketserver.TCPServer(("127.0.0.1", 0), Handler) + + @property + def url(self) -> str: + return f"http://127.0.0.1:{self._server.server_address[1]}" + + def start(self): + self._thread = threading.Thread(target=self._server.serve_forever, daemon=True) + self._thread.start() + return self + + def stop(self): + self._server.shutdown() + self._server.server_close() + + +def test_concurrent_refresh_rotates_token_once_and_keeps_login(tmp_path, monkeypatch): + """Two token providers racing on a near-expiry token must produce exactly one + refresh POST, no errors, and intact tokens in auth.json. + + MiniMax refresh tokens are single-use and rotate; without a lock across + read → refresh → save in ``_minimax_fresh_state``, both racers POST the + same still-valid ``r1``, the portal rejects the second (``refresh_token_reused``), + and the quarantine wipes the winner's fresh tokens (the review's case D). + """ + from hermes_cli.auth import build_minimax_oauth_token_provider + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + _minimax_state(tmp_path, expires_in=30, refresh_token="r1") + portal = _RotatingPortal().start() + try: + # Point the persisted state at the loopback portal. + store = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8")) + store["providers"]["minimax-oauth"]["portal_base_url"] = portal.url + (tmp_path / "auth.json").write_text(json.dumps(store), encoding="utf-8") + + # Both callers must observe the same near-expiry state before either + # refreshes (the race window): release them together, no sleeps. + barrier = threading.Barrier(3, timeout=30) + results: dict = {} + + def _provider_call(name: str): + provider = build_minimax_oauth_token_provider() + barrier.wait() + try: + results[name] = ("ok", provider()) + except Exception as exc: # noqa: BLE001 -- test witness, recorded not swallowed + results[name] = ("error", repr(exc)) + + threads = [threading.Thread(target=_provider_call, args=(f"t{i}",)) for i in range(2)] + for t in threads: + t.start() + barrier.wait() + for t in threads: + t.join(timeout=30) + assert not any(t.is_alive() for t in threads), "a provider call hung" + + # Exactly one refresh POST, zero refresh_token_reused, zero errors. + assert len(portal.refresh_calls) == 1, ( + f"expected exactly 1 refresh POST, got {portal.refresh_calls}" + ) + assert portal.refresh_calls[0] == "r1" + assert portal.reuse_rejections == 0, ( + "a second caller replayed the single-use refresh token (unlocked race)" + ) + statuses = {name: outcome for name, (outcome, _) in results.items()} + assert statuses == {"t0": "ok", "t1": "ok"}, ( + f"a concurrent provider call failed: {results}" + ) + tokens = sorted(str(v) for outcome, v in results.values() if outcome == "ok") + assert tokens == ["tok-r2", "tok-r2"], ( + f"both callers must see the winner's fresh token: {results}" + ) + + # auth.json kept the rotated pair, not a quarantine wipe. + final = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8")) + final_state = final["providers"]["minimax-oauth"] + assert final_state["access_token"] == "tok-r2", ( + f"auth.json lost the fix's fresh token: {final_state}" + ) + assert final_state["refresh_token"] == "r2" + assert "last_auth_error" not in final_state + finally: + portal.stop() + + +def test_aux_refresh_preserves_active_provider(tmp_path, monkeypatch): + """An aux-triggered refresh rewrites credentials, not the user's provider + choice: ``auth.json.active_provider`` stays ``nous`` (review case B'; the + rule ``_save_provider_state_to_source`` already documents).""" + from hermes_cli.auth import build_minimax_oauth_token_provider, get_active_provider + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + _minimax_state(tmp_path, expires_in=30, refresh_token="r1") + portal = _RotatingPortal().start() + try: + store = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8")) + store["providers"]["minimax-oauth"]["portal_base_url"] = portal.url + (tmp_path / "auth.json").write_text(json.dumps(store), encoding="utf-8") + + assert get_active_provider() == "nous" + token = build_minimax_oauth_token_provider()() + assert token == "tok-r2", "the refresh must actually run over the loopback portal" + + final = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8")) + assert final["active_provider"] == "nous", ( + "an aux-side refresh must not flip the user's active provider" + ) + assert final["providers"]["minimax-oauth"]["access_token"] == "tok-r2" + finally: + portal.stop() + + +def test_minimax_oauth_login_sets_active_provider(tmp_path, monkeypatch): + """``_minimax_oauth_login`` is the one path that legitimately makes + minimax-oauth the active provider (the user just chose it).""" + from hermes_cli import auth_minimax + + monkeypatch.setenv("HERMES_HOME", str(tmp_path)) + store = {"version": 1, "active_provider": "", "providers": {}} + (tmp_path / "auth.json").write_text(json.dumps(store), encoding="utf-8") + + token_data = {"access_token": "tok", "refresh_token": "r1", "expired_in": 900} + with patch("hermes_cli.auth._minimax_pkce_pair", return_value=("v", "c", "s")), \ + patch("hermes_cli.auth._minimax_request_user_code", return_value={ + "verification_uri": "https://portal.minimax.io/device", "user_code": "ABCD", + "expired_in": 600, "interval": 1000}), \ + patch("hermes_cli.auth._print_device_code_instructions"), \ + patch.object(auth_minimax, "_minimax_poll_token", return_value=token_data): + auth_minimax._minimax_oauth_login(region="global", open_browser=False) + + final = json.loads((tmp_path / "auth.json").read_text(encoding="utf-8")) + assert final["active_provider"] == "minimax-oauth" + assert final["providers"]["minimax-oauth"]["access_token"] == "tok"