fix(codex): classify refresh failures on the structured code and stop per-entry store reads - #4814
Conversation
…rminal prose A transient token-endpoint failure could still quarantine a healthy account. The verdict at src/codex/account-store.ts matched "invalidated", "revoked" and "expired" anywhere in the combined code+description text, unconditionally, so a `server_error` whose description merely said "the token was revoked" was retired as terminal -- the false quarantine #2887 exists to prevent, reached through the description rather than through the code. The comment directly above it already required the exact structured code; only invalid_grant was actually held to it. A body that carries a structured `error` code is now classified by that code alone. The substring fallback survives only where no structured code exists at all -- an `error_description`-only body, or one the parser could not decode -- because there the prose is the only signal upstream gave us, and removing it would leave a genuinely dead grant retrying forever. Each terminal word is pinned with its own negative case in a new file: the existing coverage only pinned a description mentioning invalid_grant, which the exact-code check already handled, so the three words were unguarded.
…r cache entry `cachedDeniedCodexAccountIdsForModel` is synchronous and on the request path for the flagship models, and it resolved a credential identity for every cached (account, client version) entry. `readCodexAccountRecord` reloads, reparses and renormalizes the whole `codex-accounts.json` per call, so with the cache at its documented budget -- 64 accounts at MODEL_ROSTER_CACHE_MAX, four versions each at MODEL_ROSTER_VERSIONS_PER_ACCOUNT_MAX -- one warm request could perform up to 256 synchronous full-store reads before it chose an account. The read is hoisted, not the check. `credentialIdentityResolver` applies the same prefix rule, the same tombstone and missing-credential rejection and the same `pool:<generation>:<chatgptAccountId>` shape; it just loads each backing store at most once per pass, lazily, so an excluded account or an all-Direct pass still opens nothing. The loop has no suspension point, so a per-pass snapshot is no staler than per-entry reads and is strictly more coherent: a foreign writer landing mid-loop can no longer hand earlier entries one generation and later ones another. `loadCodexAccountRecordSnapshot` is the new account-store export this needs. `loadCodexAccountStore` could not serve it -- it drops tombstones and `generation`, which are exactly what the identity is made of.
`tests/codex-integration/codex-model-entitlements.test.ts` imported `readCodexAccountRecord` and `saveCodexAccountCredential` twice, in two separate declarations. Duplicate imported bindings are an early SyntaxError under ESM and TS2300 under `tsc --noEmit`, so the file cannot load and its entire suite -- including the denial-reader coverage the same commit added -- never runs. Introduced by 89bdf5f, whose message records that no local suite or typecheck was run. Removing the second declaration restores the file; the surviving multi-line import already provides both names.
The read count is invisible to every behavioral assertion, so nothing stopped it regressing back to one full-store reload per cache entry. These cases assert it directly by counting `readFileSync` against `codex-accounts.json` across one `cachedDeniedCodexAccountIdsForModel` call: six entries over three accounts cost one read, and a pass whose every entry is fenced out costs none, which is what proves the resolver stayed lazy rather than merely shared. The validation the count must not have bought is asserted beside it: a roster recorded under a superseded identity is still rejected, and an account with no stored record is still left unknown rather than dropped. New file rather than an addition to codex-model-entitlements.test.ts, which is already past 1.9k lines, and registered in both layout maps.
…s passes `ensureCodexEntitlementFreshness`'s prologue, `getCodexModelEntitlementStatus` and `isCodexModelEntitlementSnapshotCurrent` each resolve an identity per candidate inside one synchronous loop, which is the same full-store reload per iteration the denial pass had. They share the pass-scoped resolver now. The two remaining single-id call sites stay single-id deliberately: both live in async continuations after an upstream fetch, where the point of the read is that it is taken AFTER the await. A resolver there would answer from a snapshot taken before the suspension, which is the one place this hoist would weaken the check rather than leave it alone. `loadCodexAccountRecordSnapshot` names its record type through the imported `CodexAccountCredentialRecord` rather than the module-local `CodexAccountStore` alias, so the exported signature stays nameable under declaration checking.
`spyOn(fs, "readFileSync").mockImplementation` has to satisfy an overloaded target, which a single Parameters<>-typed signature does not. The counter only needs to observe the call and pass it through, so the pass-through is typed structurally and cast once.
Adversarial review of the resolver hoist. `isCodexModelEntitlementSnapshotCurrent` is a fail-closed publication gate whose whole question is whether a snapshot is STILL current, so the freshest per-account answer is the point of the read. A pass-wide snapshot is a coherence win in the denial and candidate passes and a small weakening here: it could answer "current" for a later account from a record a concurrent reauth had already replaced. Reverted to per-id there and said why. The candidate-loop hoists stay. `ensureCodexEntitlementFreshness` explicitly captures its identity vector BEFORE the await and rechecks each id individually after it, and `getCodexModelEntitlementStatus` is a read-only management projection. The read counter now carries the in-process precedent for spying `node:fs` against an ESM named import -- codex-account-delete-atomicity.test.ts asserts a production read through the same seam -- and a calibration case, so the zero-expecting fence assertion can no longer pass by seeing nothing.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe change adds a credential snapshot API, tightens OAuth refresh-error classification, and reuses account data during entitlement identity checks. New integration tests cover refresh outcomes, identity validation, read counts, exclusion fences, and test-layout registration. ChangesCodex account refresh classification
Codex entitlement identity resolution
Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant EntitlementCheck
participant CredentialIdentityResolver
participant AccountStore
participant codex-accounts.json
EntitlementCheck->>CredentialIdentityResolver: resolve candidate identities
CredentialIdentityResolver->>AccountStore: load account snapshot lazily
AccountStore->>codex-accounts.json: read records once
codex-accounts.json-->>AccountStore: return records
AccountStore-->>CredentialIdentityResolver: return snapshot
CredentialIdentityResolver-->>EntitlementCheck: return validated identities
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 76 / 80이 PR은 2.57.0 두 번째 회귀 감사에서 나온 Codex 쪽 두 가지를 고칩니다. 첫째, 토큰 갱신 실패를 “폐기/만료”로 분류할 때 설명 문구( 둘째, entitlement denial/identity 조회가 캐시 엔트리마다 테스트가 계약을 잘 고정합니다. 라인 / 심볼 수준의 메모입니다.
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9bfcb59b26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export function loadCodexAccountRecordSnapshot(): Readonly<Record<string, CodexAccountCredentialRecord>> { | ||
| return loadCodexAccountRecordStore(); |
There was a problem hiding this comment.
This adds account-store snapshot and entitlement-identity semantics under src/codex/, but the commit updates none of the structure/ documents mapped to that source area. Update the owning documents in the same change so the repository's credential and entitlement architecture remains synchronized with the implementation.
AGENTS.md reference: src/AGENTS.md:L11-L11
Useful? React with 👍 / 👎.
… per-entry store reads (lidge-jun#4814) Release-blocker fix for 2.57.0 from the second regression audit. Exact head has a green aggregate ci check. A transient refresh failure whose description mentioned revoked, invalidated or expired was still classified terminal even when the structured code was server_error, which re-created the false quarantine lidge-jun#2887 exists to prevent; classification now uses the structured code whenever one exists and keeps the substring fallback only for bodies that carry no code at all. Separately a warm flagship request could perform up to 256 synchronous account-store reads because credential identity resolved per cache entry; it now resolves once per denial pass without weakening the check that an entry belongs to the credential it claims. Host-owned merge decision; no local suite, typecheck, build, or install was run.
1 participant
Summary
Two blockers the second 2.57.0 regression audit found, plus one break the candidate itself introduced and which would have failed the suite.
A transient refresh failure could still quarantine a healthy account. The verdict classified any description containing revoked, invalidated or expired as terminal even when the structured error code was server_error, which contradicts the comment above it and re-creates the false quarantine #2887 exists to prevent. Classification now uses the structured code alone whenever one exists; the substring match survives only when the body carries no structured code at all, which is the single case where prose is the only signal upstream gave. Each terminal word is pinned with its own negative case in a new test file covering both the flat and nested error shapes, with the description-only and unparseable-body cases pinning the fallback so it cannot disappear in the other direction.
A warm flagship request could perform up to 256 synchronous account-store reads. The entitlement cache permits 64 accounts with four versions each and the denial loop resolved credential identity per cache entry, with pool identity reading the store each time. Identity now resolves once per denial pass, and the same hoist is applied to the other synchronous passes, without weakening the check that an entry belongs to the credential it claims.
The third commit set repairs a duplicate account-store import in the candidate that cannot parse, and keeps the snapshot-current gate reading per account rather than inheriting the hoisted value where that would change meaning.
Verification
Checklist
Summary by CodeRabbit
Bug Fixes
Tests