feat(auth): canonical shared xAI OAuth store for multi-profile single-use refresh tokens (#65394) - #67261
feat(auth): canonical shared xAI OAuth store for multi-profile single-use refresh tokens (#65394)#67261KharonLaQua wants to merge 3 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for re-scoping activation to config.yaml; the independent-root rotation premise is real on current main: xAI resolves the active store before only a same-install root fallback (hermes_cli/auth.py:4011-4015), and refresh serialization uses that local auth lock (hermes_cli/auth.py:4499).
Problems
- The new shared-store suite does not exercise the acceptance boundary from #65394. Its concurrent waiter test uses
threading.Threadin one process (tests/hermes_cli/test_xai_shared_auth_store.py:272-339); it does not validate two OS processes with distinctHERMES_HOMEvalues, one shared directory, and one refresh POST. That distinction matters for environment initialization, path resolution, and advisory locking.
Suggested changes
- Add the two-process regression described in the #65394 discussion: separate temporary roots, shared
HERMES_SHARED_AUTH_DIR, a barrier before resolution, one mocked refresh endpoint, and assertions that the waiting process adopts the winner without retaining a local refresh-token fork.
Automated hermes-sweeper review.
| assert shared["generation"] == 2 | ||
|
|
||
|
|
||
| def test_concurrent_waiters_second_adopts(shared_env, monkeypatch): |
There was a problem hiding this comment.
This concurrency test uses threads in one process. Please add a separate OS-process regression with distinct HERMES_HOME roots and one shared auth directory; the feature request specifically depends on cross-process path and advisory-lock behavior.
There was a problem hiding this comment.
Addressed — thanks, this was the right call.
Added tests/hermes_cli/test_xai_shared_auth_two_process.py (commit 28826f9e1), which spawns two genuine child interpreters via subprocess.Popen with distinct HERMES_HOME roots pointing at one shared HERMES_SHARED_AUTH_DIR, so the cross-process path and the advisory lock are exercised for real rather than simulated with threads. The parent HERMES_HOME is explicitly popped from the child env so neither process can inherit it.
Key assertion is test_two_os_processes_shared_store_one_refresh_loser_adopts: exactly one process performs the refresh, the loser adopts the winner's result, and neither HERMES_HOME retains a durable local refresh-token fork.
The thread-based test you flagged is kept as the narrower in-process case rather than as the primary evidence.
Note this branch was rebased onto current main to clear merge conflicts, so your comment is showing as outdated — the commit is present in the new history.
|
Added the two-process regression you asked for:
Assertions: exactly one successful refresh POST, The existing threaded test stays as a fast in-process check; this adds the real cross-process boundary. Thanks for the catch. |
82cc154 to
4920779
Compare
|
Updated 2026-07-24:
The requested acceptance gap is addressed. Please re-review the refreshed head |
|
Refreshed the branch and re-verified its four commits against today's current upstream tip |
Fixes multi-profile death for xAI Grok OAuth (NousResearch#65394): xAI issues single-use rotating refresh tokens, so per-profile auth.json forks kill the grant family on the first concurrent refresh (invalid_grant). Engine (unchanged, reviewed R1–R7): one canonical store under HERMES_SHARED_AUTH_DIR (default ~/.hermes/shared/xai_oauth.json + lock), atomic election/sweep, fleet sole-owner marker, quarantine compare-and-clear, full consumer routing (runtime, pool, aux, proxy, tools, plugins). _xai_shared_auth_enabled() still reads the internal env vars only. Activation re-scoped to config.yaml (addresses NousResearch#67243 env-var-for-config closure; AGENTS.md: non-secret flags in config.yaml, bridge to internal env — terminal.cwd → TERMINAL_CWD precedent): shared_auth: providers: [xai-oauth] Bridge force-exports HERMES_XAI_SHARED_AUTH / HERMES_SHARED_AUTH_PROVIDERS at CLI, gateway, cron, dashboard/serve, and TUI startup. Absent/empty shared_auth leaves env alone (byte-identical legacy). hermes auth xai enable-shared / disable-shared write config.yaml; docs point at the config key, not .env.
…tore (NousResearch#65394) Add a genuine two-OS-process acceptance test for the shared xAI OAuth store: distinct HERMES_HOME roots, one HERMES_SHARED_AUTH_DIR, real flock contention, single-use local refresh stub, and loser-adopts assertions. Complements the in-process threading coverage without amending product code.
c3d2640 to
77a6835
Compare
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Seven PRs address the same xAI single-use refresh-token race across profiles. #28375 and #44604 route xAI state through the root auth store, #33284/#37243/#47290 introduce variants of a separate shared store, #67243 proposes a broad canonical sole-owner store but violates the repository’s configuration policy, and #67261 is its config.yaml-based resubmission, although no diff is available here to verify its claimed acceptance-gap fixes.
Related pull requests
- #28375 [closed]
related— (+280/-60) — implemented on main: routes xAI OAuth reads, rotations, locking, and credential-pool synchronization through the global root auth store, directly preventing profile-local copies from racing; it remains relevant as the closed reference superseded by merged #46614. - #33284 [closed]
related— (+683/-3) — duplicate of #28375: serializes refreshes through a separate shared xAI store and merges rotated tokens into profile state, addressing the same race but adding parallel storage infrastructure; it remains relevant as a closed alternative whose author accepted consolidation on #28375. - #37243 [closed]
related— (+358/-25) — duplicate of #28375: redirects named-profile reads, writes, refreshes, and credential-pool synchronization to a shared xAI store while preserving classic mode; it remains relevant because its author closed it after accepting #28375 as the broader consolidation target. - #44604 [closed]
related— (+290/-30) — prior cleanly reviewed root-store implementation: makes global xAI provider and pool state canonical for profiles, uses path-scoped locks, and quarantines terminal failures in the global store; it remains relevant as a closed reference, but its narrower root-auth design overlaps behavior already implemented on main. - #47290 [closed]
related— (+415/-61) — duplicate shared-store variant: moves named profiles to shared/xai_oauth.json, removes profile token copies, and routes credential-pool synchronization through that store; it remains relevant as a closed implementation of substantially the same change as #37243. - #67243 [closed]
related— (+7787/-191) — superseded predecessor of #67261: implements a broad canonical sole-owner store across runtime, pool, proxy, tools, migration, logout, and recovery paths, but the blocking contributor review rejected its user-facing HERMES_* activation because repository policy requires config.yaml settings. - #67261
related— (+9030/-191) — keep open pending refreshed maintainer verification: the PR description says it re-scopes #67243 activation to config.yaml, and the author reports adding the requested two-process regression, but no #67261 diff is available here to verify the claimed one-refresh, loser-adoption, or no-local-fork behavior. This follows the visible keep_open/salvageability=medium review rather than treating author comments as proof that its acceptance gap is resolved.
Duplicates
#33284, #37243, and #47290 are substantially duplicate shared-store fixes; #28375 and #44604 substantially overlap on making the global root auth store canonical. #67243 is the closed predecessor of #67261, not an independent merge candidate.
Suggested consolidation
Do not merge #67261 yet; keep it open and request a refreshed maintainer review against its current diff and executed two-process acceptance test. The visible keep_open/salvageability=medium review identified a missing cross-process boundary, and the available data contains only author claims—not a #67261 diff or refreshed verification verdict—saying that gap is closed. #33284, #37243, and #47290 can remain closed as duplicates, #67243 remains superseded by #67261 due to its configuration-policy conflict, and #28375/#44604 remain closed references because the root-store behavior is already represented on main via #46614.
Cross-PR triage: Reviewed 7 pull requests and 0 issues in this complex. Diffs were read for 6 of 7 PRs (rest unavailable); Assessment working set: 481 kB of PR diffs, 26 kB of issue/PR text, 14 kB of discussion (16 comments), 4 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
Fixes multi-profile death for xAI Grok OAuth (issue #65394): xAI issues single-use rotating refresh tokens, so when each Hermes profile forks its own copy into
auth.json, the first profile to refresh revokes every other copy (invalid_grant). This PR introduces an opt-in canonical shared store that all profiles read and refresh under one shared lock — one grant, one refresher, no per-profile forking.Activation is config.yaml (not a user-facing env var). This addresses the
env-var-for-configclosure of #67243 (AGENTS.md: non-secret feature flags belong inconfig.yaml; bridge to an internal env var if the mechanism needs one — same pattern asterminal.cwd→TERMINAL_CWD). The activation move leaves the core election/sweep/marker/lock engine unchanged; this refresh additionally hardens auxiliary shared-mode resolution so stale legacy pool entries can never bypass canonical or profile-disable state.Problem
Today each profile (and the root
~/.hermes/auth.json) can hold its own xAI OAuth grant. xAI rotates the refresh token on every successful refresh:invalid_grant.This is the multi-profile fork/death path described in #65394. Write-through helpers and pool entries that also persist raw refresh tokens make the blast radius worse.
Solution
A canonical sole-owner store (not a Nous-style convenience layer) at:
All profiles read / refresh / persist through one resolver under that lock. Profile and root stores keep only a non-secret
source: shared:xai-oauthreference (no raw RT forking).Opt-in gate (config.yaml user-facing; internal env bridge)
Shared xAI mode does not activate just because
HERMES_SHARED_AUTH_DIRis set (that directory is already used for Nous shared auth).User-facing activation (non-secret feature flag in config.yaml):
At process startup (CLI, gateway, cron, dashboard/serve, TUI), Hermes force-exports the internal bridge targets from that config (AGENTS.md env-var-for-config /
terminal.cwd→TERMINAL_CWDprecedent):HERMES_XAI_SHARED_AUTH=1when providers includes an xAI aliasHERMES_SHARED_AUTH_PROVIDERS=<comma-joined list>HERMES_SHARED_AUTH_DIRonly whenshared_auth.diris setThe engine gate (
_xai_shared_auth_enabled()) still reads only those env vars — they remain the internal mechanism so tests and power-user overrides keep working. Gate-off (absent/emptyshared_authin config) does not set or modify those env vars → byte-identical legacy behavior.Key properties
shared_auth.providers; env vars are internal bridge targets onlyEnablement + migration
Logout semantics under shared mode:
hermes auth xai disable-shared— removesxai-oauthfromshared_auth.providersin config.yaml (canonical grant stays on disk)hermes logout --provider xai-oauth— per-profile disable marker while shared mode remains on; canonical grant stays for other profileshermes logout --provider xai-oauth --global— deletes the grant for every profile (intentionally noisy)Requires a local filesystem with reliable advisory locking (not NFS/SMB). Every gateway/cron/desktop process must load the same config.yaml (shell-only exports are not the activation path).
Testing
The provider overlay was integrated and promoted as candidate
fce3427db0baabe316686d2bd3923cd20ddc4dbf, which contains upstream07e97d2f5dc3d2092cfe693ef07b2527a36cd2d8. That merge completed withouttextual conflicts. The same four commits in this PR have also been replayed
onto the upstream tip observed during this refresh,
32fd9d65cf091269709c5a6301b25aadac681aa8, without manual conflictresolution. All 9 changed Python test files then passed: 131 passed.
The integration lane passed all of its normal gates:
accepted and the positive fixture was detected
The pytest instruments were also tested against genuine zero collection. The
229-suite, 330-suite, and 13-control commands each returned pytest exit
5when their selectors collected no tests. A green receipt from these gates
therefore cannot be a vacuous zero-test pass.
Deliberately poisoned candidates proved that the semantic gate detects and
names lost capabilities:
auxiliary xAI shared credentials fail-closedshared-auth config bridgexAI store capability as missing
The historical 229-test suite alone still passed on a poisoned candidate.
That result is why the lane requires the semantic capability gate instead of
treating the historical suite as sufficient.
tests/hermes_cli/test_shared_auth_config_bridge.py— config → env bridge, gate-off byte-identical, power-user env override preserved, enable/disable write config.yamltests/hermes_cli/test_xai_shared_auth_store.py— gate off/on, generation bump, fail-loud persist, concurrent waiters adopt winner, quarantine compare-and-clear, migrate/strip sole-owner, election fail-closed on unreadable/wrong-shape stores, concurrent logout vs promote, fleet marker digest races, no-resurrection after quarantine, gate-off byte-identical legacytests/hermes_cli/test_xai_shared_auth_two_process.py— genuine two-OS-process, cross-HERMES_HOMElock contention; one refresh winner and loser-adopts assertions. This directly exercises the OS-process boundary and real advisory locking raised in teknium1's review, rather than using threads as a proxy.tests/agent/test_auxiliary_xai_shared_recovery.py— aux auth-error recovery with rejected bearer / generation plus canonical-first, no-legacy-fallback regressionstests/agent/test_credential_pool_oauth_writethrough.py/test_credential_sources_xai_remove.py— pool write-through + source removal under shared modetests/hermes_cli/test_xai_oauth_writethrough.py— OAuth write-through boundariestests/tools/test_xai_http_shared_mode.py— tool HTTP path uses canonical resolvertests/plugins/video_gen/test_xai_plugin.py— plugin routing under shared modeThe gate-off path remains byte-identical to legacy behavior. Coverage also
includes deterministic concurrency (election→commit race, strip→inventory
race), fail-closed invalid shapes, and no-resurrection guarantees.
Production soak
The promoted change set has been running since 2026-07-25 in a real
multi-profile deployment spanning 8 gateways and 33 profiles. Live xAI and
Anthropic probes pass, and the xAI auth monitor reports
XAI_AUTH_HEALTHYwith no affected profiles. This is operational soakevidence from the promoted fork deployment; it is not a claim that upstream
CI has run or passed on this PR.
Docs
website/docs/guides/xai-grok-oauth.md— Shared-store mode uses config.yaml /enable-sharedwebsite/docs/user-guide/configuration.md—shared_authsectionwebsite/docs/reference/environment-variables.md—HERMES_XAI_SHARED_AUTH/HERMES_SHARED_AUTH_PROVIDERSlabeled internal bridge targets (do not set by hand)Scope notes
shared_auth.providersincludes an xAI alias (or a power-user sets the internal env var)._xai_shared_auth_enabled()still reads env only; auxiliary shared-mode entry is additionally hardened to resolve canonical-first and fail closed.env-var-for-config).Checklist
HERMES_*.env flag)