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 <ubxty@users.noreply.github.com>
This commit is contained in:
Brooklyn Nicholson
committed by
brooklyn!
brooklyn!
parent
d61d60d493
commit
665a42bb08
@@ -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)
|
||||
@@ -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,
|
||||
|
||||
@@ -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]:
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user