fix(codex): accept monthly-classified snapshots and probe-owned refresh generations (#955 review) - #967
Conversation
…ned refresh generations Addresses the two unresolved Codex review threads on lidge-jun#955: - isCompleteCodexQuotaRecoverySnapshot() required weeklyPercent for every non-Go/Free plan by plan name, but the parser classifies windows by duration: a Team response with an explicitly monthly primary window parses to monthlyPercent only, so those accounts could never recover early and stayed cooled until their predicted expiry. - settleCodexQuotaRecoveryProbe() required the claim-time credential generation to match exactly. A probe-owned token refresh inside getValidCodexToken() advances the generation by one before WHAM completes, so a successful fresh reading was rejected and the account waited another probe interval. replacedAt is preserved by refresh and stamped by external replacement, so it fences the +1 transition.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![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: 79d2164e26
ℹ️ 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".
| : proofGeneration === claim.credentialGeneration + 1 | ||
| && currentRecord?.replacedAt === claim.credentialReplacedAt | ||
| && isCodexAccountGenerationLive(claim.accountId, proofGeneration)); |
There was a problem hiding this comment.
When another caller such as the token guardian is already refreshing this cooled account, the probe can join that getValidCodexToken() flight; the owner persists generation N+1, then the same-account join path at src/codex/account-store.ts:406-422 persists the returned credential again and reports N+2. The fresh WHAM result is therefore proven under a live, unchanged-replacedAt generation but this exact +1 check rejects it, leaving the cooldown in place for at least another probe interval. Since replacedAt already distinguishes external replacement, accept any later live generation with the same claim-time lineage (or avoid the redundant same-account generation bump).
Useful? React with 👍 / 👎.
| if (codexQuotaWindowForPlan(plan) === "monthly") { | ||
| return typeof quota.monthlyPercent === "number" && Number.isFinite(quota.monthlyPercent); | ||
| } | ||
| return hasKnownQuotaValue(quota); |
There was a problem hiding this comment.
For a weekly plan such as Team, parseUsageQuota() can produce monthlyPercent from a tertiary window even when the weekly primary/secondary window is entirely absent; tests/codex-routing.test.ts:1199-1205 explicitly preserves that tertiary-only representation. This fallback now treats such a response as complete recovery evidence, so a low optional 30-day reading can clear a reset-derived shared cooldown without any fresh reading for the weekly quota that caused it, immediately routing traffic back to an account that may still be exhausted. Preserve whether the monthly value came from an explicitly monthly primary and accept monthly-only recovery for weekly-named plans only in that case, rather than accepting every parsed monthly value.
Useful? React with 👍 / 👎.
|
Please put your Pull-Request on Ready for Review, once you are finished. |
|
Carried into #973 (stack 6/6). You found two real defects in my #955 code and both reproduce — thank you for reviewing it properly rather than rubber-stamping it. Verified before carrying: $ bun run .tmp/probe_967.ts # on the #955 head, BEFORE your fix
parsed = {"monthlyPercent":12,"monthlyResetAt":1900000000}
recoverable? = false <-- Team monthly account could never recoverYour diagnosis is exact on both counts. P1: I picked the required window from the plan name while One correction on top, which is why the carried commit is not the last word. Your P1 remedy — accept whichever window the parser wrote — is too permissive in the other direction. A tertiary-only response also writes Those two shapes were indistinguishable, so no amount of tightening at the predicate could separate them. #973 adds provenance at the source instead: An independent audit then found the flag was being dropped at three further copy sites — Your commit is carried with |
3 participants
Summary
Follow-up to #955 that addresses both unresolved Codex review threads on the cooldown early-recovery probe:
1. Duration-classified monthly snapshots were never accepted (P1).
isCompleteCodexQuotaRecoverySnapshot()requiredweeklyPercentfor every non-Go/Free plan based on the plan name. ButparseUsageQuota()classifies windows by duration: a Team plan response whose primary window is explicitly monthly (e.g.limit_window_seconds: 2628000, no secondary) parses tomonthlyPercentonly. The probe therefore rejected every successful fresh read for those accounts and they stayed cooled until the predicted expiry — the same "cooled forever" defect this work exists to fix, reintroduced for monthly-window plans.2. The probe's own token refresh was treated as a replacement (P2).
getValidCodexToken()refreshes a near-expiry access token inside the probe fetch and advances the credential generation by exactly one before WHAM completes.settleCodexQuotaRecoveryProbe()required the claim-time generation to match exactly, so a successful fresh reading under the new live generation was rejected and recovery waited another probe interval.saveCodexAccountCredentialIfGeneration()preservesreplacedAtwhilesaveCodexAccountCredential()stamps a new one, soreplacedAtfences the +1 transition against external replacement.Tests
Verification
bun x tsc --noEmit— exit 0tests/codex-cooldown-recovery.test.ts— 20 pass (both new tests were red before the fix)tests/rate-limit-reset-credits.test.ts— 31 passtests/codex-routing.test.ts tests/codex-pool-rotation.test.ts tests/codex-main-rotation.test.ts— 143 pass