Repository navigation
fix(codex): permit spendable credits when subscription rate limit is exhausted - #6573
sungyongcho wants to merge 1 commit into
Conversation
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (4)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe quota parser and disk hydration no longer copy the included-plan allowance flag into credit snapshots. Spendable-credit eligibility no longer rejects credits when that flag is false. Integration tests cover credit eligibility after the weekly included limit is exhausted. ChangesSpendable credit eligibility
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change enables spendable credits after the included limit while retaining the separate credit checks. No actionable merge blocker is established by the available evidence. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Credit-enabled accounts can continue using fresh spendable credits after their included allowance is exhausted. Account-specific opt-in, ownership checks, credit freshness, and recovery safeguards remain. No introduced security concern was established, but the upstream allowance semantics and external compatibility are not fully documented. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
Ingwannu
left a comment
There was a problem hiding this comment.
The included-plan refusal must not veto opted-in credits, but removing the veto entirely loses a distinct credit spending-control refusal. I reproduced the counterexample below against this exact head with the actual parser/exhaustion predicate, inert fixtures, and fetch disabled. Please cover both the parser and main-account admission before readiness, and synchronize the owning credit-policy docs with the new contract. #6572 is a related draft alternative, not an approval or a reason to close this contributor PR as a duplicate; neither should merge until this distinction and the required independent review are settled.
Validation: offline Bun 1.4.0 probe under CPUQuota=75%, MemoryMax=1536M, swap disabled, private homes. With identical positive credits and included usage at 100%, both spend_control.reached=false and true return exhausted=false here. No real accounts, provider calls, or new scans were used.
| @@ -42,7 +41,7 @@ export function hasSpendableCodexCredits( | |||
| if (!credits || !Number.isFinite(credits.observedAt)) return false; | |||
| const age = now - credits.observedAt; | |||
| if (age < 0 || age > CODEX_CREDITS_FRESHNESS_MS | |||
There was a problem hiding this comment.
[P2] Preserve the credit-specific spending-control refusal. A fresh WHAM response with rate_limit.allowed=false, primary_window.used_percent=100, credits={has_credits:true,balance:"42.5",overage_limit_reached:false}, and spend_control.reached=true is now treated as spendable: parseUsageQuota(...) followed by isCodexQuotaExhausted(quota, "pro", true) returns false. That is not the #6571 case (which has reached=false); the spending control explicitly says the credit limit has been reached. The parser removes the included-plan flag but never reads this separate control, and this predicate no longer has a credit veto. Keep included-plan exhaustion non-vetoing while normalizing and honoring the credit-specific spending verdict; add raw-WHAM/main-admission regressions for reached=false versus true and invalid declared values. Please update the owning structure credit-policy docs too, rather than retaining the checked docs assertion with no doc changes.
|
Closing as superseded by #6572, merged into dev by lidge-jun at 2dd3f82 (PR head e9bcbe9; exact-head CI37209972954 passed). The landed fix decouples included-plan allowance from credit permission while retaining explicit spend-control refusal, malformed-control veto and control-only refusal retraction; it does not remove the credit-specific veto wholesale. Consent, freshness and independent hard-lock checks remain. Sungyong Cho's Co-authored-by attribution is retained in the landed commit. This closure does not approve or merge this alternative patch, waive the earlier refusal-control review, or claim a release/live credit-spending test. |
Summary
rate_limit.allowed: falseonceprimary_window.used_percent == 100. In v2.77.0,parseUsageQuotamappedrate_limit.allowedontoquota.credits.allowed, which causedhasSpendableCodexCredits()to returnfalsewhenever the subscription rate limit was exhausted, defeating credit failover.allowedfromCodexSpendableCredits/readMainPolicyQuotaand decoupledhasSpendableCodexCredits()fromrate_limit.allowed.Closes #6571
Verification
Ran focused regression test suites covering quota parsing, credit fallback, and cooldown recovery:
bun test tests/codex-integration/codex-quota-parser-parity.test.ts tests/codex-integration/codex-credits-after-limit-main.test.ts tests/codex-integration/codex-credits-after-limit.test.ts tests/codex-integration/codex-cooldown-recovery.test.ts(107/107 passing)bun test tests/ci-workflows/file-size-ratchet.test.ts(9/9 passing)bun run typecheckbun run privacy:scanbun run structure:check(Note: A full repository test run was not executed locally because
src/codex/quota.tscascades into several thousand tests taking over 10 minutes across concurrent worktrees, per AGENTS.md scoped testing guidance; all direct regression coverage and CI gates pass.)Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Required local validation passed; commands, results, and any full-suite exception are documented.
I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit