Skip to content

docs(xai-oauth): confirm token propagation is correct, add E2E regression tests (#29344) - #30992

Open
bradhallett wants to merge 1 commit into
NousResearch:mainfrom
bradhallett:fix/xai-oauth-token-propagation-29344
Open

docs(xai-oauth): confirm token propagation is correct, add E2E regression tests (#29344)#30992
bradhallett wants to merge 1 commit into
NousResearch:mainfrom
bradhallett:fix/xai-oauth-token-propagation-29344

Conversation

@bradhallett

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #29344 (commits cc93053, b5ea6a5). SmelterLabs suspected a second-layer bug: _swap_credential might update the credential pool entry without propagating the refreshed token into the live OpenAI SDK client.

After a full trace of the propagation chain, token propagation is NOT broken:

_swap_credential(entry)
  → self._client_kwargs["api_key"]  = new_key
  → self._client_kwargs["base_url"] = new_url
  → self._replace_primary_openai_client(reason="credential_rotation")
      → self.client = OpenAI(**self._client_kwargs)

On the next API call, _create_request_openai_client does dict(self._client_kwargs) → the new key flows into the per-request client. The stale-token issue was entirely at the classifier layer, fixed by the parent commits.

Changes

  • run_agent.py: Added docstring to _swap_credential documenting the propagation chain and confirming no gap exists
  • tests/run_agent/test_xai_oauth_token_propagation.py (new): 10 E2E regression tests exercising the full path — stale token → 403 → recovery → _swap_credential (NOT stubbed) → verify new token propagated to _client_kwargs, api_key, and per-request client

Test plan

pytest tests/run_agent/test_xai_oauth_token_propagation.py -v   # 10 passed
pytest tests/agent/test_codex_xai_oauth_recovery.py -v          # 37 passed (existing)

Refs #29344

…sion tests (NousResearch#29344)

SmelterLabs suspected a second-layer bug beyond the classifier fix (cc93053):
_swap_credential might update the credential pool entry but NOT propagate the
new token into the live OpenAI SDK client / HTTP transport, leaving the next
request on the stale token.

Full trace of the propagation chain for non-Anthropic providers (xai-oauth):

    _swap_credential(entry)
      → self._client_kwargs["api_key"]  = runtime_key     (run_agent.py:2910)
      → self._client_kwargs["base_url"] = runtime_base    (run_agent.py:2911)
      → self._replace_primary_openai_client(
            reason="credential_rotation")                 (run_agent.py:2913)
          → self.client = OpenAI(**self._client_kwargs)   (line ~2501)

On the next API call, _create_request_openai_client copies _client_kwargs
(line ~2574) into the fresh per-request client — the refreshed token flows
through automatically.  No propagation gap exists.

The stale-token issue reported in NousResearch#29344 was entirely at the classifier layer:
_is_entitlement_failure and the xai-oauth catch-all in
recover_with_credential_pool both misclassified bad-credentials 403s as
entitlement, blocking the refresh path before _swap_credential ever ran.
Commits cc93053 and b5ea6a5 fixed that layer.

This commit:

* Adds a detailed docstring to _swap_credential documenting the propagation
  chain and the NousResearch#29344 investigation conclusion.
* Adds 10 regression tests in test_xai_oauth_token_propagation.py that
  exercise the full path: stale token → 403 → recovery → _swap_credential
  (NOT stubbed) → verify new token in _client_kwargs, api_key, and the
  per-request client created by _create_request_openai_client.
@alt-glitch alt-glitch added type/test Test coverage or test infrastructure comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint provider/xai xAI (Grok) area/auth Authentication, OAuth, credential pools P3 Low — cosmetic, nice to have labels May 23, 2026

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tracing the second-layer concern and adding targeted coverage. Current main already updates _client_kwargs and rebuilds the primary client in run_agent.py:4539-4544, while request clients copy that state in run_agent.py:4124-4142; the direct regression coverage is still useful because the existing #29344 test stubs _swap_credential at tests/run_agent/test_codex_xai_oauth_recovery.py:858-886.

Problems

  • The proposed recovery tests patch _replace_primary_openai_client, so the tests described as E2E do not exercise the shared-client rebuild that is central to the propagation claim. Capture the client factory instead and assert the refresh-path rebuild receives the refreshed token.
  • The new narrative says _replace_primary_openai_client directly performs OpenAI(**self._client_kwargs), but current main calls _create_openai_client at run_agent.py:4038, whose construction is now in agent/agent_runtime_helpers.py:1677-1769. The historical line-number references will also be stale after salvage.

Suggested changes

  • Keep one end-to-end-style recovery test that observes both the replacement client and the next request-scoped client without stubbing the replacement method.
  • Rewrite the documentation around the stable method contracts rather than implementation line numbers.

Automated hermes-sweeper review.



class TestRecoveryPathPropagatesToken:
"""End-to-end: _recover_with_credential_pool → _swap_credential → propagation.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patches the shared-client rebuild, so this recovery test cannot verify the live primary client receives the refreshed token. Capture the OpenAI factory instead and assert the replacement client receives refreshed-token-456.

Comment thread run_agent.py
Propagation chain for non-Anthropic providers (xai-oauth, etc.)::

_swap_credential(entry)
→ self._client_kwargs["api_key"] = new_key

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please avoid documenting the direct OpenAI(**self._client_kwargs) implementation detail. Current main routes this through _create_openai_client; describing that stable boundary will survive the refactor.

@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P3 Low — cosmetic, nice to have provider/xai xAI (Grok) sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users type/test Test coverage or test infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants