Skip to content

fix: heal profile Codex credential cooldowns from global pool - #41704

Open
trevornk wants to merge 2 commits into
NousResearch:mainfrom
trevornk:fix/codex-profile-global-health
Open

fix: heal profile Codex credential cooldowns from global pool#41704
trevornk wants to merge 2 commits into
NousResearch:mainfrom
trevornk:fix/codex-profile-global-health

Conversation

@trevornk

@trevornk trevornk commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Summary

  • allows profile-local OpenAI Codex OAuth pool entries with stale exhausted status to heal from matching healthy global entries
  • treats both device_code and manual:device_code sources as resyncable OAuth credentials
  • preserves profile entry priority while copying healthier token/status fields

Why

A profile can materialize a local copy of globally inherited Codex OAuth credentials after runtime status updates. If that local copy remains exhausted while the same global OAuth entry has been reset or re-authed, the profile appears out of credentials even though a usable matching global credential exists.

Test Plan

  • ./venv/bin/python -m pytest tests/hermes_cli/test_auth_profile_fallback.py tests/agent/test_credential_pool.py tests/agent/test_credential_pool_routing.py -q -o 'addopts='

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have area/auth Authentication, OAuth, credential pools provider/openai OpenAI / Codex Responses API codex labels Jun 8, 2026
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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 14, 2026
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for isolating the profile/global pool scenario. The underlying gap still exists on current main for a profile-local exhausted manual Codex entry: hermes_cli/auth.py:1361-1366 makes non-empty profile entries shadow root, while agent/credential_pool.py:1449-1455 only resyncs singleton-seeded device_code entries.

Problems

  • The agent/credential_pool.py change in b8c4accb also makes _sync_codex_entry_from_auth_store() process every manual:device_code entry. That helper adopts providers.openai-codex.tokens, so it can overwrite independent accounts. Current main explicitly preserves those accounts at hermes_cli/auth.py:3300-3328; regression coverage asserts the invariant at tests/hermes_cli/test_auth_codex_provider.py:398-405.
  • The new availability helper differs from the pool's real cooldown logic: runtime honors the fallback TTL from last_status_at in agent/credential_pool.py:338-346 and 1497-1513.
  • The added test only verifies read_credential_pool(). Please cover load_pool(...).select() as well, since that is the production selection path (agent/credential_pool.py:2389-2402).

Suggested changes

  • Keep singleton auth-store sync restricted to device_code; rework the profile/global health merge without treating all manual entries as singleton aliases.
  • Reuse equivalent cooldown semantics and add coverage for independent manual accounts.

Automated hermes-sweeper review.

@teknium1 teknium1 added the area/profiles Multi-profile isolation, HERMES_HOME scoping label Jul 19, 2026
@trevornk
trevornk force-pushed the fix/codex-profile-global-health branch from 7fa1218 to b2d730d Compare August 26, 2026 22:56
trevornk added a commit to trevornk/hermes-agent that referenced this pull request Aug 26, 2026
… parity, coverage

Three real issues from the automated review, all confirmed against current
code before fixing:

1. _sync_codex_entry_from_auth_store's broadened manual:device_code sync had
   no equivalent to hermes_cli/auth.py::_save_codex_tokens's previous-token
   match guard (regression protection for NousResearch#39236). Adopting singleton tokens
   unconditionally for manual entries can silently overwrite an independent
   account's live credentials when a re-auth targets a different account.
   Reverted the scope of this sync back to device_code only; profile-local
   manual:device_code healing already has a correct, ID-matched path via
   _merge_profile_entries_with_global_health in the same PR.

2. _credential_pool_entry_available's exhausted-cooldown fallback (blank
   reset_at -> available iff last_status_at is unset) didn't match the real
   pool runtime's actual fallback (last_status_at + _exhausted_ttl(error_code),
   a 401/429/default TTL). Now delegates to agent.credential_pool's own
   _exhausted_until via PooledCredential.from_dict, with a deferred import
   to avoid the existing credential_pool.py -> hermes_cli.auth circular
   dependency, keeping the two cooldown semantics from silently diverging.

3. Added test coverage for load_pool(...).select() (agent/credential_pool.py's
   CredentialPool), the actual production selection path -- previously only
   read_credential_pool()'s raw dict slice was tested, not the wrapped
   dataclass/select() path requests actually go through.

All existing tests still pass (42/42 in the touched auth test files);
verified the one pre-existing unrelated failure in
test_anthropic_adapter.py::TestRunOauthSetupToken (MagicMock/json mismatch)
reproduces identically on unmodified main, unrelated to this change.
@trevornk

Copy link
Copy Markdown
Contributor Author

Rebased onto current main. One of my own earlier commits here was wrong and has been reversed — worth stating plainly.

Reversed: the manual:device_code narrowing. My second commit (responding to review) narrowed _sync_codex_entry_from_auth_store back to device_code only, arguing that adopting singleton auth.json tokens for manual:device_code entries can't distinguish a legacy singleton-alias from an independent account added via hermes auth add openai-codex, and so risks clobbering a live account (#39236).

That reasoning describes a real sharp edge, but the conclusion was wrong in context: main has since widened that exact guard in 7380b48, deliberately, after the narrow version made refresh-token adoption unreachable for manual:device_code — the source string hermes auth add openai-codex actually produces. Per that commit, the reporter confirmed a 12-of-16 fleet outage on Aug 1. Landing my narrowing would have re-broken a confirmed production fix to guard a hypothetical. Main's guard is kept as-is; I've rewritten the docstring to describe the real trade-off (including the residual ambiguity and how _save_codex_tokens handles it on the write path) instead of arguing for behavior the code no longer has.

The healing feature itself is unaffected, and it never depended on that narrowing. _merge_profile_entries_with_global_health matches profile↔global entries by pool-entry id, which is a per-entry uuid4().hex[:6]. A shared id means the profile row was materialized from that global row; independently-added accounts get distinct ids. So the cross-wire concern that motivated the narrowing doesn't apply to this code path by construction.

Also dropped: the rebase wanted to restore test_per_provider_shadowing_is_independent, which main deleted in its test-pruning waves (3997561). Resurrecting a deliberately pruned test as rebase collateral would be wrong, so it's gone. test_codex_profile_load_pool_select_heals_via_production_path is kept — it covers load_pool(...).select(), the path request routing actually uses, rather than only the raw dict read.

Retained from commit 2: _credential_pool_entry_available delegating to agent.credential_pool._exhausted_until rather than re-deriving cooldown semantics. That matters — the old local fallback treated a missing last_error_reset_at differently from the real pool, which computes last_status_at + _exhausted_ttl(last_error_code). Divergence there means an entry reads as healable here while the selection path still excludes it, silently defeating the fix.

Tests: test_auth_profile_fallback.py, test_credential_pool.py, test_credential_pool_routing.py — 85 passed; test_auth_codex_provider.py (the #39236 guard) — 11 passed. Negative control: stubbing _merge_profile_entries_with_global_health to a no-op fails exactly the two new healing tests and leaves the other 7 in that file green.

… parity, coverage

Three real issues from the automated review, all confirmed against current
code before fixing:

1. _sync_codex_entry_from_auth_store's broadened manual:device_code sync had
   no equivalent to hermes_cli/auth.py::_save_codex_tokens's previous-token
   match guard (regression protection for NousResearch#39236). Adopting singleton tokens
   unconditionally for manual entries can silently overwrite an independent
   account's live credentials when a re-auth targets a different account.
   Reverted the scope of this sync back to device_code only; profile-local
   manual:device_code healing already has a correct, ID-matched path via
   _merge_profile_entries_with_global_health in the same PR.

2. _credential_pool_entry_available's exhausted-cooldown fallback (blank
   reset_at -> available iff last_status_at is unset) didn't match the real
   pool runtime's actual fallback (last_status_at + _exhausted_ttl(error_code),
   a 401/429/default TTL). Now delegates to agent.credential_pool's own
   _exhausted_until via PooledCredential.from_dict, with a deferred import
   to avoid the existing credential_pool.py -> hermes_cli.auth circular
   dependency, keeping the two cooldown semantics from silently diverging.

3. Added test coverage for load_pool(...).select() (agent/credential_pool.py's
   CredentialPool), the actual production selection path -- previously only
   read_credential_pool()'s raw dict slice was tested, not the wrapped
   dataclass/select() path requests actually go through.

All existing tests still pass (42/42 in the touched auth test files);
verified the one pre-existing unrelated failure in
test_anthropic_adapter.py::TestRunOauthSetupToken (MagicMock/json mismatch)
reproduces identically on unmodified main, unrelated to this change.
@trevornk
trevornk force-pushed the fix/codex-profile-global-health branch from b2d730d to 5d9730b Compare September 4, 2026 00:18
@trevornk

trevornk commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (63279301bc) — was CONFLICTING/DIRTY, now MERGEABLE/CLEAN at 5d9730bc99. Both commits preserved. Final shape is 3 files, +259/-3.

One resolution worth calling out explicitly, because the naive rebase would have been a regression: main had deliberately widened the Codex sync guard, and narrowing it back to device_code would have re-broken a reporter-confirmed production fix. A narrow guard makes refresh-token adoption unreachable for the source string that hermes auth add openai-codex actually produces. I kept main's widened guard and revised the docstring around it instead. Profile-to-global healing still matches on unique per-entry uuid4-derived pool-entry IDs, so independent accounts cannot cross-wire.

Verified locally on Python 3.11.16:

tests/hermes_cli/test_auth_profile_fallback.py .... 9 passed in 1.01s

Mutation check: temporarily short-circuiting the healing path made the new tests fail, and restoring it returned them to green — so the coverage is load-bearing rather than decorative. Also scanned the diff for credential literals; none present.

Broad-suite runs were skipped: this machine hit its memory ceiling and the OOM-killer was SIGKILLing pytest, so those numbers would have been noise. Note also that tests/hermes_cli has a known independent hazard (#71719run_gateway() can call os._exit() mid-pytest and abort the run with a green exit code), so broad results from that directory need care regardless.

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 area/profiles Multi-profile isolation, HERMES_HOME scoping codex P3 Low — cosmetic, nice to have 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