fix(auth): replace misleading "No Codex credentials" with rate-limit reset message - #28182
fix(auth): replace misleading "No Codex credentials" with rate-limit reset message#28182bensnelldev wants to merge 1 commit into
Conversation
…reset message
When a pool-stored OAuth credential (Codex, xAI, Qwen, Gemini, Nous,
Anthropic) is rate-limited or otherwise in exhaustion cooldown, the
runtime resolver was falling through to legacy `_read_*_tokens` helpers
that read a different storage location (top-level `providers.<name>`),
which is empty for credentials that live only in `credential_pool`.
The resulting `AuthError("No Codex credentials stored. Run `hermes
auth` to authenticate.")` is doubly misleading — the credential IS
stored, and re-authenticating wouldn't help while the quota window is
still active.
Instead, when `pool.has_credentials() and not pool.has_available()`,
raise a structured `AuthError(kind="rate_limit"|"auth_failed", reset_at=...)`
that includes the soonest reset time. The gateway, CLI, and cron
scheduler all use a new `describe_primary_failure(error)` helper so the
warning surfaces the actionable reason (rate-limited until 1h 12m vs.
auth failed needing re-login) rather than a generic "auth failed".
The shared pool-status classifier/formatter previously in
`hermes_cli/auth_commands.py` are moved to `agent/credential_status.py`
so non-CLI callers (resolver, gateway logger) can import them without
pulling in CLI command code. `hermes_cli/auth_commands.py` re-exports
the originals to preserve external imports.
The legacy `_read_codex_tokens` (and siblings) are intentionally
unchanged: they're still called by refresh + persistence paths that
expect the `providers` shape, and the early `pool.select()` branch
already handles healthy pool-only credentials correctly. The bug only
ever surfaced in the cooldown fall-through case, which the new
exhaustion check intercepts.
Tests:
- `tests/agent/test_credential_status.py` (new): unit-tests for
`summarize_pool_exhaustion`, `classify_exhausted_status`,
`format_pool_exhaustion_message`, `format_remaining`.
- `tests/hermes_cli/test_runtime_provider_pool_exhaustion.py` (new):
resolver raises structured `AuthError` with `kind`/`reset_at` when
pool is all-exhausted; falls through normally for non-pool providers.
- `tests/gateway/test_auth_fallback_log_message.py` (new): the gateway
warning now includes "rate-limited (resets in Xh Ym)" and the
misleading "No Codex credentials stored" no longer appears.
- `tests/hermes_cli/test_auth_commands.py`: monkeypatch retargeted to
the new module location for the formatter's `time.time()`.
- `tests/gateway/test_session_model_override_routing.py`: fixture
updated to use a realistic structured `AuthError(kind="rate_limit")`
instead of the legacy stand-in.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
outsourc-e
left a comment
There was a problem hiding this comment.
The auth fix itself looks good, but this PR is stacked on unrelated mobile dashboard/web work from #28127 (6fa1701bd). Please do not merge this branch as-is.
I opened a clean replacement PR with only the auth/runtime/logging slice plus the targeted regression tests:
Local validation on the clean replacement:
scripts/run_tests.sh tests/agent/test_credential_status.py tests/hermes_cli/test_runtime_provider_pool_exhaustion.py tests/gateway/test_auth_fallback_log_message.py tests/gateway/test_session_model_override_routing.py tests/hermes_cli/test_auth_commands.py- 71 passed
|
Hit this in a live Telegram gateway session today via Observed behavior:
So there seems to be one additional sharp edge beyond the misleading wording while currently rate-limited: cached exhausted status can continue to block Suggested UX/behavior: when resolving |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the detailed fix and tests. I would not merge this PR branch as-is.
Problems
- This PR has already been reviewed as stacked on unrelated mobile dashboard/web work; the existing review points to #28185 as the clean replacement. I verified #28185 is still open/unmerged with
gh pr view 28185. - Current main has moved around this area: f1422ff added Codex 429 classification via
CODEX_RATE_LIMITED_CODE/is_rate_limited_auth_error(hermes_cli/auth.py:718,gateway/run.py:1416), and 69dfcdc added a Codex pool fallback inresolve_codex_runtime_credentials()(hermes_cli/auth.py:3749). Salvage needs to reconcile with those existing paths rather than applying this branch directly. - Current main also has
STATUS_DEADpool entries (agent/credential_pool.py:63) that are excluded from availability (agent/credential_pool.py:1282). The proposedsummarize_pool_exhaustion()only summarizes entries withlast_status == STATUS_EXHAUSTED, so an all-DEAD pool would still bypass the structured message path.
Suggested changes
- Prefer the clean replacement PR (#28185) or a fresh narrow salvage branch.
- Fold the structured pool-status UX into the existing rate-limit/AuthError classification now on main.
- Include DEAD/all-unavailable pool cases in the tests if this is salvaged.
Automated hermes-sweeper review.
| return None | ||
| if any(entry.last_status != STATUS_EXHAUSTED for entry in entries): | ||
| return None | ||
|
|
There was a problem hiding this comment.
Current main has STATUS_DEAD entries that are also unavailable to pool.select(); limiting the summary to STATUS_EXHAUSTED means an all-DEAD pool will still fall through to the legacy singleton-auth path. A salvage should account for DEAD explicitly or document why it is intentionally excluded.
| def format_auth_error(error: Exception) -> str: | ||
| """Map auth failures to concise user-facing guidance.""" | ||
| if not isinstance(error, AuthError): | ||
| return str(error) |
There was a problem hiding this comment.
Current main already has CODEX_RATE_LIMITED_CODE/is_rate_limited_auth_error for quota failures. If this is salvaged, this new kind-based branch should be reconciled with that existing classifier rather than becoming a parallel rate-limit mechanism.
| # (rate_limit / auth_failed / exhausted) lets us log something | ||
| # actionable instead of a generic "auth failed". | ||
| from hermes_cli.auth import describe_primary_failure | ||
|
|
There was a problem hiding this comment.
Current main now logs CODEX_RATE_LIMITED_CODE separately in this handler. Salvage should preserve that behavior while adding pool-exhaustion wording, instead of replacing it wholesale with a new helper that does not know about the existing classifier.
Problem
When the OpenAI Codex (ChatGPT-subscription) OAuth credential hits its 5-hour usage cap, the Hermes gateway logs this repeatedly until the window resets:
Two things wrong with that message:
hermes auth listsimultaneously and correctly showsopenai-codex-oauth-1 oauth device_code rate-limited usage_limit_reached (429) (1h 12m left).hermes authis actively misleading.The same misleading flow exists for
xai-oauth,qwen-oauth,google-gemini-cli,nous, andanthropicpool entries — anywhere a single credential lives only incredential_pool.<provider>rather than the legacy top-levelproviders.<provider>location.Root cause
In
hermes_cli/runtime_provider.pyresolve_runtime_provider():pool.has_credentials()isTrue(one entry incredential_pool.openai-codex).pool.select()returnsNonebecause the only entry is in exhaustion cooldown.if provider == "openai-codex":→resolve_codex_runtime_credentials())._read_codex_tokens()which readsproviders.openai-codex(the legacy storage location, which is empty for pool-only credentials).AuthError("No Codex credentials stored. Run \hermes auth` to authenticate.", code="codex_auth_missing", relogin_required=True)`.Fix
Detect the "pool has entries but none are usable" case before the legacy fall-through, and raise a structured
AuthErrorcarrying the soonest reset time. The gateway/CLI/cron-scheduler all use a newdescribe_primary_failure(error)helper to render the warning so it surfaces the actual reason.Specifically:
agent/credential_status.py— movesclassify_exhausted_statusandformat_exhausted_statusout ofhermes_cli/auth_commands.pyso non-CLI callers (resolver, gateway logger) can import them without pulling in CLI command code. Addssummarize_pool_exhaustion(pool)andformat_pool_exhaustion_message(provider, summary).auth_commands.pyre-exports the originals to preserve any external imports.AuthErrorgains optionalkind,reset_at,retry_afterfields plus afrom_pool_exhaustion(provider, summary)classmethod that builds a message likeopenai-codex rate-limited (usage_limit_reached, 429) - resets in 1h 12m.pool.select()branch and the provider-specific legacy fall-throughs (hermes_cli/runtime_provider.py). Coversopenai-codex,xai-oauth,qwen-oauth,google-gemini-cli,nous,anthropic.describe_primary_failure(auth_exc), producing e.g.primary provider openai-codex rate-limited (usage_limit_reached, 429) - resets in 1h 12m — trying fallback. PlainAuthErrorwithoutkindkeeps its existing log shape (back-compat).Why not just teach
_read_codex_tokensto read from the pool?That helper is also called by refresh + persistence paths that expect the
providersshape. Mixing reads creates a write/read asymmetry. The earlypool.select()branch already handles healthy pool-only credentials correctly — the bug only ever surfaces in the cooldown fall-through path, which is exactly where the new check fires.Affected providers
openai-codexxai-oauthqwen-oauthgoogle-gemini-clinousanthropicBefore / after
Before, in gateway log:
After:
Fallback still triggers identically (e.g. through to Copilot via gh CLI). The behavior change is purely the message; downstream consumers that branch on
AuthErroritself are unaffected unless they opt-in to the newkind/reset_atfields.Test plan
New tests:
tests/agent/test_credential_status.py— 16 cases coveringclassify_exhausted_status,format_remaining,format_exhausted_status,summarize_pool_exhaustion(empty pool, any-healthy → None, all-rate-limited picking soonest reset, mixed prefers rate-limit label, all-auth-failed), andformat_pool_exhaustion_message.tests/hermes_cli/test_runtime_provider_pool_exhaustion.py— resolver raises structuredAuthError(kind="rate_limit", reset_at=…)when the codex pool is rate-limit-exhausted; raiseskind="auth_failed"withrelogin_required=Truefor auth-failure-only entries; no-op fall-through for non-pool providers (openrouter).tests/gateway/test_auth_fallback_log_message.py— asserts the new "rate-limited (resets in …)" wording appears in the gateway warning and "No Codex credentials stored" does not; plainAuthErrorstill logs the generic shape.Updated tests:
tests/hermes_cli/test_auth_commands.py— monkeypatch retargeted fromhermes_cli.auth_commands.time.timetoagent.credential_status.time.timeto match the formatter's new home.tests/gateway/test_session_model_override_routing.py— fixture upgraded from the legacy stand-inAuthError("No Codex credentials stored. …")to a realistic structuredAuthError(kind="rate_limit", …). Asserted behavior (fallback triggers) is unchanged.Suite run (touched-area):
🤖 Generated with Claude Code