Skip to content

fix(auth): harden Codex quota probe token refresh - #72690

Open
kchuang1015 wants to merge 3 commits into
NousResearch:mainfrom
kchuang1015:fix/codex-quota-probe-oauth-lifecycle
Open

fix(auth): harden Codex quota probe token refresh#72690
kchuang1015 wants to merge 3 commits into
NousResearch:mainfrom
kchuang1015:fix/codex-quota-probe-oauth-lifecycle

Conversation

@kchuang1015

Copy link
Copy Markdown
Contributor

What does this PR do?

Hardens the OpenAI Codex quota-restored probe so an expired access token can be refreshed safely before the usage request.

The refresh path now:

  • resolves the credential's true owner store;
  • acquires that store's cross-process lock before re-reading or exchanging a rotating refresh token;
  • persists the complete access/refresh/timestamp replacement chain atomically;
  • keeps canonical device-code and pool state synchronized;
  • preserves quota/cooldown state unless the usage probe is explicitly positive;
  • classifies terminal refresh failures separately from transient failures;
  • persists transient-failure throttling across processes;
  • conditionally merges concurrent reauthentication and newer lifecycle state so stale pool snapshots cannot overwrite them;
  • keeps profile-local and global fallback ownership separate.

Motivation

The quota-restored probe can run while a pooled Codex credential is still under a persisted cooldown. If the stored access token expires during that cooldown, probing with it returns an authentication error and cannot demonstrate that upstream quota recovered.

Refreshing outside the owning store's lock is unsafe because Codex refresh tokens rotate and are single-use. A concurrent process or a stale whole-pool write can otherwise replay an old refresh token, split the canonical and pool token chains, or restore stale cooldown state after a newer login.

This change treats authentication and quota as independent lifecycles: successful refresh only restores a usable probe credential; only an explicit positive usage response clears cooldown.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)
  • Performance improvement
  • Test improvement
  • Other (please describe):

Related Issues

Testing

  • Added regression coverage for expired-token refresh, same-access-token refresh-token rotation, owner-store locking, singleton/pool synchronization, concurrent reauthentication, terminal and transient refresh failures, cross-process throttling, stale whole-pool persistence, newer-generation preservation, global fallback ownership, and local takeover.
  • scripts/run_tests.sh -j 4 tests/hermes_cli/test_auth_codex_quota_probe.py tests/agent/test_credential_pool.py tests/agent/test_credential_pool_oauth_writethrough.py tests/hermes_cli/test_auth_profile_fallback.py tests/hermes_cli/test_auth_codex_provider.py -q — 204 passed.
  • Clean rebased-head affected and upstream-delta suite — 365 passed, 0 failed.
  • scripts/run_tests.sh -j 4 -q — completed, but not fully green locally; see the baseline analysis below.
  • ruff check on all changed files.
  • scripts/check-windows-footguns.py on all changed files.
  • git diff --check.
  • Baseline-aware ty check on changed files — 0 new diagnostics relative to the same origin/main base.
  • Manual end-to-end Codex OAuth request before and after a graceful gateway reload; no related authentication or quota error was observed.
Local full-suite baseline analysis

The complete local runner finished with 8 failing files / 19 failing tests. None of the changed auth/pool regression files failed.

The same eight files were then rerun with the same Python environment and -j 1 in two clean disposable worktrees: current origin/main and the rebased PR head.

  • Both trees failed the same six unrelated files (session/SQLite recovery, gateway startup race, Qwen provider fallthrough, Honcho cache busting, Codex-response SQLite persistence, and execute-code approval routing).
  • Two failures from the active worktree (managed_uv call count and bundled plugin discovery) passed in both clean worktrees; the latter was caused by an untracked local plugin directory.
  • The only node-count difference was a Honcho cache-busting test that independently alternated between pass and fail on both base and head across five fresh-process repetitions.

This produced no deterministic head-only failure. GitHub CI remains authoritative for the clean x86 test matrix.

Screenshots / Demos

Not applicable; this is an authentication lifecycle fix with no user-interface changes.

Checklist

  • My code follows the project's style guidelines
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation where necessary (no user-facing documentation change is required)
  • My changes generate no new warnings relative to the current baseline
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published

Notes for Reviewers

Codex refresh tokens are rotating and single-use, so the owner-store lock intentionally spans the latest-state re-read, token exchange, and atomic save. The usage request itself remains outside that lock; its final write therefore uses conditional merge logic rather than persisting the pre-probe snapshot.

This overlaps with #64572 around global/root ownership, but it is not a complete duplicate: this change is specifically a current-main follow-up to the quota-restored probe and includes probe refresh, quota/auth separation, persistent transient-failure throttling, terminal lifecycle handling, and stale post-probe/whole-pool write protection. Maintainer guidance on consolidating shared ownership helpers with #64572 is welcome.

@alt-glitch alt-glitch added type/bug Something isn't working comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard provider/openai OpenAI / Codex Responses API area/auth Authentication, OAuth, credential pools sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data P2 Medium — degraded but workaround exists labels Jul 27, 2026
@kchuang1015
kchuang1015 marked this pull request as ready for review July 27, 2026 14:37
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the thorough lifecycle coverage. The underlying defect is still present on current main: hermes_cli/auth.py:3908-3912 probes the cooldown-frozen pool access token without refreshing it first, and agent/credential_pool.py:1693-1723 likewise probes before reaching the ordinary refresh path. The PR directly addresses both paths while retaining quota cooldowns until a positive usage response.

The reviewed PR head has successful required CI, including all Python test slices and the blocking Windows-footguns check. GitHub currently marks the branch merge-conflicting against main, so salvage will require resolving the surrounding auth/pool changes rather than a clean merge.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 30, 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 comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists provider/openai OpenAI / Codex Responses API sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants