MiniMax OAuth: source-aware shared-profile refresh - #66764
Open
rzyns wants to merge 7 commits into
Open
Conversation
… and quarantine Bring minimax-oauth credential-pool refresh to the same cross-profile OAuth safety already present for Nous, OpenAI Codex, and xAI Grok (issue NousResearch#48415 — the MiniMax analog). Changes in hermes_cli/auth.py: - Extract refresh_minimax_oauth_pure() from _refresh_minimax_oauth_state(): performs the single token-endpoint POST without mutating auth state, so the pool path can serialize the whole sync -> POST -> write-back under _auth_store_lock. The eager-resolve path keeps its save-through behavior. - Add _is_terminal_minimax_oauth_refresh_error() mirroring the Codex/xAI terminal-error detectors (invalid_grant / refresh_token_reused / no_refresh_token / refresh_failed with relogin_required=True; transient failures do not qualify). Changes in agent/credential_pool.py: - _refresh_entry: serialize minimax-oauth through _auth_store_lock with an in-lock re-sync that collapses concurrent refreshes to exactly one POST. - _refresh_entry_impl: add minimax-oauth branch calling refresh_minimax_oauth_pure, with single-use-token resync before the POST. - _sync_minimax_oauth_entry_from_auth_store: adopt fresher tokens from auth.json when another process refreshed the singleton. - _sync_device_code_entry_to_auth_store: handle minimax-oauth (flat state shape like nous), set_active=False so a pool refresh never flips active_provider, and write-through to global root when the profile borrows the grant from root fallback. - _entry_needs_refresh: proactive refresh based on expires_at + skew. - _available_entries: resync frozen minimax-oauth entries from auth.json. - Terminal quarantine: on invalid_grant / refresh_token_reused, wipe dead tokens from profile and root, write diagnostic blob, and remove the singleton-seeded pool entry. Write-through quarantine to root when the profile reads from root fallback. Tests (tests/agent/test_credential_pool_minimax_oauth.py): - Write-through to root when profile reads root fallback. - Profile-local shadow not promoted to root. - Lock held across the token POST (concurrency invariant). - Concurrent refresh produces exactly one POST. - Terminal refresh quarantines both profile and root. - Transient refresh failure retains credentials (no quarantine). - Terminal-error detector unit tests. All tests use temp filesystem stores with the network POST stubbed; no live credentials are read, printed, or exchanged.
Repair every Review A MUST-FIX on top of the source-aware pool refresh: 1. Distinct profiles sharing a root grant are now mutually serialized. The minimax-oauth refresh path runs inside _provider_state_transaction, which holds both the active profile lock AND the global-root source lock (kernel flock keyed by root auth.json path), so two distinct profiles holding distinct profile locks cannot both POST the same single-use refresh token. Added a subprocess-based cross-profile test with genuinely distinct profile auth paths contending on one root path (no thread-unsafe process-global monkeypatch). 2. Borrowed-root ownership stays with root across successive refreshes. _sync_minimax_oauth_entry_to_source writes directly to root when the source was the global-root fallback, never creating a profile-local providers.minimax-oauth shadow that would break future write-throughs. Two-successive-refresh regression test confirms root stays owned. 3. Singleton-seeded pool entries now populate the expiry representation actually consumed by proactive refresh. _seed_from_singletons sets both expires_at (ISO string, checked by _entry_needs_refresh) and expires_at_ms (epoch ms). load_pool test verifies the seeded shape. 4. Terminal quarantine preserves active_provider. The quarantine path uses persist=False + _persist_provider_state_to_store(set_active=False) so a background pool failure cannot flip the user's chosen provider. Test asserts active_provider survives terminal quarantine. Non-blocking: replaced HERMES_MINIMAX_REFRESH_TIMEOUT_SECONDS env var with an internal MINIMAX_OAUTH_REFRESH_TIMEOUT_SECONDS constant, per the policy that non-secret behavioral settings belong in config.yaml, not .env. All existing Codex/xAI/Nous behavior and tests preserved: minimax-oauth is split out of the shared openai-codex/xai-oauth lock-wrapped path into its own source-aware branch, leaving the Codex/xAI path unchanged.
…d-root pool persistence guard - Route runtime/token-provider and resolve_minimax_oauth_runtime_credentials refresh/quarantine through source_path/set_active=False so borrowed-root grants write back to global root, not a profile-local shadow. - Add _resolve_minimax_oauth_source_path so we only fail closed when the source was actually removed from a non-active (root) store, preserving ordinary single-store sessions. - Avoid persisting root-owned MiniMax OAuth secrets into the active profile's credential pool by marking borrowed-root entries with source_auth_path and expanding is_borrowed_credential_source / PooledCredential.to_dict to strip access/refresh tokens before disk write. - Harden refresh_token preservation when the MiniMax endpoint omits a new refresh token in a partial success. - Update existing minimax-oauth tests for the new keyword-only source-aware helper signatures. Broad checks: ruff clean; credential/local-path scan clean; focused auth and credential-pool suites pass (247 tests).
No semantic change; applies project formatter to the Review C repair files and tests so the branch passes ruff format --check.
alt-glitch
added
type/bug
tonydwb
reviewed
Jul 18, 2026
tonydwb
left a comment
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
+3501 additions / 7 files. MiniMax OAuth source-aware shared-profile refresh. The change adds source_auth_path awareness to is_borrowed_credential_source() so that MiniMax OAuth entries seeded from the global-root fallback are treated as borrowed (not persisted locally), and adds a refresh mechanism to keep the token pair current across profile switches.
Scope: Changes to agent/credential_persistence.py, agent/credential_pool.py, and related modules. The additions are substantial. No tests visible in the diff — if there are existing tests covering this path, a pointer would help. The description references a real problem (shared profile token staleness), but the diff itself is large enough that a human reviewer should validate the full credential flow end-to-end before merge.
Low-risk observation: The is_borrowed_credential_source check now receives a payload argument — callers that were not updated to pass it would silently bypass the new check. Verify all call sites were updated.
Reviewed by Hermes Agent
teknium1
reviewed
Jul 18, 2026
teknium1
left a comment
teknium1
left a comment
Contributor
There was a problem hiding this comment.
Thanks for the thorough source-aware refresh work. The current-main premise is real: agent/credential_pool.py:2181-2218 seeds MiniMax OAuth without source ownership, while hermes_cli/auth.py:7777-7840 refreshes and saves to the active store.
Problems
agent/credential_pool.py:3124still callsis_borrowed_credential_source(entry.source, entry.provider)without thesource_auth_pathpayload introduced by this PR. After a root-borrowed grant is removed, a restarted profile retains its sanitizedsource="oauth"row rather than pruning it; a future expiry can then make that tokenless entry selectable. Add a reload-after-root-removal regression and make pruning payload-aware.tests/agent/test_credential_pool_minimax_oauth.py:596importsfcntlin the subprocess worker. That is unavailable on native Windows and the test has no platform guard. Use the repository's cross-platform lock abstraction or an equivalent Windows path.
Suggested changes
- Prune MiniMax rows carrying
source_auth_pathwhenever their OAuth singleton is no longer active. - Make the subprocess counter lock cross-platform.
This is an automated hermes-sweeper review.
| and not auth_mod._same_path(source_path, active_path) | ||
| ) | ||
| if borrowed_root: | ||
| payload["source_auth_path"] = str(source_path) |
Contributor
There was a problem hiding this comment.
Recording source_auth_path makes this row reference-only on disk, but _prune_stale_seeded_entries() still calls is_borrowed_credential_source(entry.source, entry.provider) without the entry payload. If root later removes this grant, a restarted profile retains this tokenless oauth row; with a future persisted expiry it can be selected. Make stale pruning payload-aware and add a reload-after-root-removal regression.
| # profile auth.json but sharing one global-root auth.json. The source-aware | ||
| # transaction must serialize both on the root flock so only one POST occurs. | ||
| _CROSS_PROFILE_WORKER = """ | ||
| import json, os, sys, time, fcntl |
Contributor
There was a problem hiding this comment.
This subprocess worker imports fcntl, which is unavailable on native Windows, and the test has no platform guard. Please use the existing cross-platform auth-lock abstraction (or an msvcrt-compatible implementation) for the counter synchronization.
teknium1
added
sweeper:risk-compatibility
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Brings
minimax-oauthto parity with the source-aware cross-profile OAuth behavior used by Nous, OpenAI Codex, and xAI OAuth. The change:invalid_grant,refresh_token_reused, andinvalid_refresh_tokenat the authoritative source while preserving unrelated auth state and provider selection.Related work
This addresses the cross-profile rotating-token hazard adjacent to #48415 and #43589 and the borrowed-credential boundary discussed in #65940. No dedicated upstream issue was filed for this branch.
A fresh search of open upstream issues and PRs on 2026-07-18 found no exact duplicate for MiniMax OAuth source-aware refresh, root write-through, or terminal quarantine. Adjacent work intentionally remains out of scope:
oauth_minimaxrouting work such as fix(auxiliary): support minimax-oauth in auxiliary client router (fixes #21521, #36091) #40122 and fix: minimax-oauth auth_type unhandled — all 12 auxiliary tasks silently no-op (vision, compression, title gen, web_extract, skills_hub, approval, mcp, memory_query_rewrite, tts_audio_tags, triage_specifier, kanban_decomposer, profile_describer) #61585.Type of change
Changes
agent/credential_pool.py_provider_state_transaction()with an in-lock authoritative re-read.active_provider.agent/credential_persistence.pyhermes_cli/auth.pyTests
load_pool -> select -> refreshpath for both profile-owned and root-borrowed grants, including in-memory refresh-token hydration and borrowed-secret disk sanitization.main.Test plan
scripts/run_tests.sh \ tests/agent/test_credential_pool_minimax_oauth.py \ tests/test_minimax_oauth.py \ tests/agent/test_credential_pool_oauth_writethrough.py \ tests/hermes_cli/test_auth_profile_fallback.py \ tests/agent/test_credential_pool_no_entries_log_throttle.py -q scripts/run_tests.sh \ tests/agent/test_credential_pool.py \ tests/agent/test_credential_pool_routing.py \ tests/agent/test_credential_pool_provider_boundary.py \ tests/agent/test_credential_pool_no_entries_log_throttle.py \ tests/tools/test_credential_pool_env_fallback.py \ tests/run_agent/test_credential_pool_interrupt.py \ tests/hermes_cli/test_auth*.py -q ruff check agent/credential_persistence.py agent/credential_pool.py hermes_cli/auth.py \ tests/agent/test_credential_pool_minimax_oauth.py \ tests/agent/test_credential_pool_oauth_writethrough.py \ tests/hermes_cli/test_auth_profile_fallback.py tests/test_minimax_oauth.py ruff format --check agent/credential_persistence.py agent/credential_pool.py hermes_cli/auth.py \ tests/agent/test_credential_pool_minimax_oauth.py \ tests/agent/test_credential_pool_oauth_writethrough.py \ tests/hermes_cli/test_auth_profile_fallback.py tests/test_minimax_oauth.pyLocal evidence
git diff --check: passed.upstream/main; merge-base equals the recorded base and the branch is seven commits ahead, zero behind.32b89c1faadded a RED-to-GREEN real-load regression and the minimal hydration fix. Re-review of that exact repaired head returned PASS with no new findings.The full repository suite was attempted before the final replay but could not be claimed green on this host because of unrelated optional ACP, local Ollama/audio/path, and load-sensitive failures. The exact impacted suites above are green after the replay.
Compatibility and risk
Checklist
AI-assisted disclosure
AI tools were used to inspect the credential-pool/auth-store code, implement the source-aware refresh and quarantine paths, prepare tests, and draft this PR body. An independent agent context reviewed the rebased diff for concurrency, source ownership, secret handling, and upstream-conflict preservation; its initial blocking finding was repaired and the exact repaired head passed re-review. Final human review approved publication of this exact branch and proposed PR body before the fork push and PR creation. No merge, release, deployment, live OAuth, or runtime action was performed.