auth(xai-oauth): cross-profile shared token store, race fix - #33284
Closed
VinceZcrikl wants to merge 1 commit into
Closed
auth(xai-oauth): cross-profile shared token store, race fix#33284VinceZcrikl wants to merge 1 commit into
VinceZcrikl wants to merge 1 commit into
Conversation
xAI uses single-use rotating refresh tokens — every successful refresh
mints a new pair and the previous refresh_token is immediately
invalid server-side. When two profiles under the same hermes-root
both pick `model.provider: xai-oauth`, each keeps an INDEPENDENT
copy of the rotating refresh_token in its profile-local
`auth.json`: there's no cross-profile sync.
When both gateways are alive (the typical multi-profile orb / multi-
tab CLI setup), they race:
1. Profile A's token is near expiry → posts refresh_token R1 → xAI
mints R2, invalidates R1, A stores R2.
2. Profile B still has R1 cached locally → posts R1 → xAI sees
reuse → revokes the entire token family → `last_auth_error`
in both auth.jsons turns into `relogin_required: True`.
Real users hit this. The diagnostic signal in the field is
`last_auth_error.code == "xai_refresh_failed"` /
`message == "Refresh token has been revoked"`, often on a profile
that never actively misbehaved — it just happened to wake up after
a sibling refreshed.
The fix is the same pattern Nous already ships
(`_nous_shared_store_*` + `_try_import_shared_nous_state`): a single
authoritative copy of the OAuth tokens at
`<hermes-root>/shared/xai_auth.json` plus a cross-profile file lock
that serializes refreshes so siblings can't race on the single-use
token.
Helpers (new):
* `_xai_shared_auth_dir` / `_xai_shared_store_path` — same
`HERMES_SHARED_AUTH_DIR` override and pytest seat belt as Nous.
* `_xai_shared_store_lock` — file lock at
`xai_auth.json.lock`. Lock-ordering exception documented in the
docstring: held alone for the entire refresh+persist cycle, just
like `_try_import_shared_nous_state` does for Nous.
* `_write_shared_xai_state` — atomic O_EXCL 0o600 write via
`secure_parent_dir` + tmp + rename. Skips when refresh_token or
access_token is empty.
* `_read_shared_xai_state` — `None` on missing / malformed /
incomplete payload.
* `_clear_shared_xai_state` — wipe after terminal failure so dead
tokens don't poison siblings.
* `_merge_shared_xai_oauth_state` — copy fresher shared tokens into
a profile-local state when shared's `refresh_token` differs from
local's. Returns True iff the state changed.
Wiring:
* `_save_xai_oauth_tokens` now mirrors the rotated pair to the
shared store after the per-profile auth.json write. Failures
in the shared write are logged and swallowed — the local save
remains the source of truth.
* `resolve_xai_oauth_runtime_credentials` now performs its
refresh path under `_xai_shared_store_lock` instead of
`_auth_store_lock`. Under the lock it (a) re-reads local,
(b) merges fresher shared tokens in and persists them locally
(skipping the HTTP refresh entirely if shared has rotated since
we last looked), (c) only then makes the actual refresh HTTP
call. On terminal failure it clears the shared store in
addition to the per-profile quarantine, so a sibling profile
doesn't inherit dead tokens.
* `_auth_store_lock` is no longer held during the HTTP refresh —
persistence helpers acquire it briefly and internally.
Tests:
* `tests/hermes_cli/test_auth_xai_shared_store.py` (13 cases):
seat belt + env override path; read/write round-trip; permission
bits (0o600 where supported); write skipped when fields missing;
clear is idempotent; merge no-op vs merge-applies-fresher;
integration test verifying `_save_xai_oauth_tokens` mirrors to
the shared store.
* Mirrors `test_auth_nous_provider.py` shared-store conventions
(`shared_store_env` fixture redirecting
`HERMES_SHARED_AUTH_DIR` to `tmp_path`).
* Full xAI + Nous auth test suites still green (154/154).
Not in this change:
* MCP-server-style per-tool toggling for xAI tools — out of scope.
* Concurrent-thread race test — the lock primitive itself is
already covered by Nous's `_file_lock` tests; the new tests
exercise the integration points instead.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Closing as a duplicate of #28375 — flagged by the maintainer. Confirmed:
Sorry for the noise — I should have searched open PRs for "xai oauth share" / "refresh token" before opening this. Will check upstream first next time. If anything from this PR is useful to fold into #28375 (e.g., the |
9 tasks
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
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.
Problem
xAI uses single-use rotating refresh tokens — every successful refresh mints a new pair and the previous
refresh_tokenis immediately invalid server-side. When two profiles under the same hermes-root both pickmodel.provider: xai-oauth, each keeps an INDEPENDENT copy of the rotatingrefresh_tokenin its profile-localauth.json; there's no cross-profile sync.The race:
refresh_token R1→ xAI mintsR2, invalidatesR1, A storesR2.R1cached locally → postsR1→ xAI sees reuse → revokes the entire token family.last_auth_error.code == "xai_refresh_failed",relogin_required: True— and the user discovers it later when a tool (x_search, xAI TTS, etc.) silently disappears from the schema becausecheck_x_search_requirements()returns False.Real users hit this with no warning. The diagnostic in the field is the
"Refresh token has been revoked"last_auth_error.messageon a profile that never did anything "wrong" — it just happened to wake up after a sibling refreshed.Fix
Same pattern as the existing Nous shared store (
_nous_shared_store_*+_try_import_shared_nous_state): a single authoritative copy of the OAuth tokens at<hermes-root>/shared/xai_auth.jsonplus a cross-profile file lock that serializes refreshes so siblings can't race on the single-use token.New helpers (mirror Nous)
_xai_shared_auth_dir/_xai_shared_store_path— sameHERMES_SHARED_AUTH_DIRoverride and pytest seat belt as Nous._xai_shared_store_lock— file lock atxai_auth.json.lock. Held alone for the entire refresh+persist cycle (lock-ordering exception, documented in the docstring, just like_try_import_shared_nous_statedoes for Nous)._write_shared_xai_state— atomicO_EXCL0o600write viasecure_parent_dir+ tmp + rename. Skips whenrefresh_tokenoraccess_tokenis empty._read_shared_xai_state—Noneon missing / malformed / incomplete payload._clear_shared_xai_state— wipe after terminal failure so dead tokens don't poison siblings._merge_shared_xai_oauth_state— copy fresher shared tokens into a profile-local state when shared'srefresh_tokendiffers from local's. Returns True iff the state changed.Wiring
_save_xai_oauth_tokensnow mirrors the rotated pair to the shared store after the per-profileauth.jsonwrite. Failures in the shared write are logged and swallowed — the local save remains the source of truth.resolve_xai_oauth_runtime_credentialsnow performs its refresh path under_xai_shared_store_lockinstead of_auth_store_lock. Inside the lock it:invalid_grant/xai_refresh_failed), clears the shared store in addition to the per-profile quarantine, so a sibling doesn't inherit dead tokens._auth_store_lockis no longer held across the HTTP refresh — persistence helpers acquire it briefly and internally.Lock ordering invariant
Following the existing documented exception on
_nous_shared_store_lock: the runtime refresh path holds the shared lock alone for the whole refresh + persist cycle (callers must not enter it with_auth_store_lockalready held). The persistence helpers (_save_xai_oauth_tokens, the terminal-failure quarantine block) acquire_auth_store_lockinternally and briefly.Tests
tests/hermes_cli/test_auth_xai_shared_store.py(13 cases, all passing):HERMES_SHARED_AUTH_DIRredirect honored._read_shared_xai_statereturnsNonefor missing / malformed / incomplete payload.refresh_token/access_token/token_type/id_token/expires_in/discovery/last_refresh.0o600where the platform supportschmod.refresh_tokenoraccess_tokenis empty._clear_shared_xai_stateremoves the file and is idempotent on already-absent files._merge_shared_xai_oauth_stateno-op when refresh_tokens match._merge_shared_xai_oauth_statecopies fresher shared tokens in when refresh_tokens differ._merge_shared_xai_oauth_statereturns False when shared store is missing._save_xai_oauth_tokensmirrors to both per-profileauth.jsonand the shared store.Conventions match
test_auth_nous_provider.py's shared-store tests —shared_store_envfixture redirectingHERMES_SHARED_AUTH_DIRtotmp_path.Regression coverage
Full
tests/hermes_cli/test_auth_xai_oauth_provider.py+test_xai_oauth_pkce_token_exchange.py+test_auth_nous_provider.pysuites still green: 154/154 passed in 8.72s.Out of scope
_file_lockprimitive is already covered by Nous's existing suite; the new tests exercise the integration points.🤖 Generated with Claude Code