fix(agent): rotate Codex credentials on model entitlement rejection - #71973
fix(agent): rotate Codex credentials on model entitlement rejection#71973kilhyeonjun wants to merge 6 commits into
Conversation
alt-glitch
added
type/bug
|
Reviewed against the #71970 spec (static review, not run locally). This looks correct and complete — it maps cleanly onto every expected-behavior bullet in the issue:
One minor, non-blocking note: when neither Verdict: fixes the issue as specified, with genuine failing-then-green regression coverage. Nice work @kilhyeonjun. |
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused classifier and credential-pool work. The reported gap is real on current main: HTTP 400 reaches _classify_400() at agent/error_classifier.py:1107 and currently falls through to generic format_error at :1430.
Problems
agent/credential_pool.py:1948in this diff falls back to_current_unlocked()when no credential identity is supplied. That contradicts the stated fail-closed boundary and can mark an unrelated credential; current recovery explicitly documents why the shared current pointer is unsafe for attribution inagent/agent_runtime_helpers.py:980-990.- The persisted marker is not used by normal selection. Current
CredentialPool.select()accepts no model (agent/credential_pool.py:1593) and_available_entries()does not inspectunavailable_models(:1603-1726), so reloading the pool can select the previously rejected account for the same model.
Suggested changes
- Require an exactly resolved credential identity before writing a marker, with a no-hint regression test.
- Thread the requested model through normal selection and add a reload test proving model-specific exclusion while retaining eligibility for another model.
- Cover the all-marked case that returns control to provider fallback.
Automated hermes-sweeper review.
| if entry is None and identity_supplied: | ||
| return None | ||
| entry = entry or self._current_unlocked() | ||
| if entry is None: |
There was a problem hiding this comment.
This fallback violates the stated fail-closed rule: when neither hint identifies the issuer, _current_unlocked() is shared mutable state and may be a different healthy account. Return None unless an exact ID or key hint resolved the entry.
| self._replace_entry(entry, updated) | ||
| self._persist() | ||
| candidates = [ | ||
| candidate for candidate in self._available_entries() |
There was a problem hiding this comment.
This filters only the immediate recovery call. CredentialPool.select() on current main has no model argument and _available_entries() ignores unavailable_models, so a reloaded pool can select this marker-bearing credential again for the same model. Make the normal selection path model-aware and add a reload regression test.
teknium1
added
sweeper:risk-security-boundary
SummaryTwelve PRs address distinct layers of the same Codex model-compatibility failure family: provider/model normalization, auxiliary routing, authenticated and fallback catalogs, error classification, and credential-scoped entitlement recovery. The diffs range from already-landed catalog and routing fixes to open patches that still need current-main updates or correctness fixes before they can close the remaining issues. Related pull requests
Duplicates#695 and #696 are substantially the same #651 normalization fix and were superseded by 95b1130. #17544 and #17765 address the same #17533 auxiliary fallback failure, with #17765 replacing the stale-constant approach. #23175 and #33424 remove the same dead fallback slugs for #23097, with #33424 landed; #23189 covers the separate live-catalog synthesis path. The catalog portion of #69022 duplicates #61665, while its classifier portion overlaps the narrower entitlement classification in #71973. Suggested consolidationAuthor action: rebase onto main, or split out the part that can merge. For #71973, first resolve the blocking contributor review by removing Complex graphflowchart LR
classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
classDef best stroke-width:3px,stroke:#b45309
classDef target stroke-width:3px,stroke:#4338ca
I61660(["issue #61660 (open)"])
I71970(["issue #71970 (open)"])
P71973["PR #71973 (open)"]
P71973 -.->|partial| I61660
P71973 -->|best fix| I71970
class I61660 open
class I71970 open
class P71973 open
class P71973 best
class P71973 target
click I61660 "https://github.com/NousResearch/hermes-agent/issues/61660"
click I71970 "https://github.com/NousResearch/hermes-agent/issues/71970"
click P71973 "https://github.com/NousResearch/hermes-agent/pull/71973"
Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed (state tag in the node label). Cross-PR triage: Reviewed 12 pull requests and 7 issues in this complex. Each diff was read against this issue; Assessment working set: 103 kB of PR diffs, 49 kB of issue/PR text, 25 kB of discussion (28 comments), 24 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
5 participants
Summary
openai-codexpool entry and rotate once to the next eligible credentialFixes #71970
Root cause
The account-specific Codex HTTP 400 was treated as a generic request/format failure. Credential-pool recovery therefore had no narrow reason or model-scoped state with which to rotate a different credential, even when another configured ChatGPT subscription could use the requested model.
Safety boundaries
The '<model>' model is not supported when using Codex with a ChatGPT account.is classified as entitlementTests
ty checkon the touched large modules reports the repository's existing advisory diagnostics; the blocking Ruff gate and focused behavioral suites pass. Upstream CI can provide the canonical base-vs-head type-diagnostic diff.Related work
This complements static model compatibility fixes #61660 / #61665 and credential-pool work #31032 / #47096 / #68520. It is intentionally separate from quota-based
usage_limit_reachedhandling in #45646 / #46915.Platform
Tested on macOS arm64 with Python 3.12 through the repository
uvenvironment. No live credential or provider request was used by the tests.