Skip to content

fix(codex): permit spendable credits when subscription rate limit is exhausted - #6573

Closed
sungyongcho wants to merge 1 commit into
lidge-jun:devfrom
sungyongcho:fix/codex-spendable-credits-rate-limit-allowed
Closed

sungyongcho wants to merge 1 commit into
lidge-jun:devfrom
sungyongcho:fix/codex-spendable-credits-rate-limit-allowed

Conversation

@sungyongcho

@sungyongcho sungyongcho commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Fixes an issue where accounts with "Use credits" enabled were incorrectly treated as exhausted and rejected with 429 when their primary rate limit window reached 100% ([Bug] 2.77.0: "Use credits" never applies after weekly limit — credits.allowed copied from rate_limit.allowed #6571).
  • WHAM returns rate_limit.allowed: false once primary_window.used_percent == 100. In v2.77.0, parseUsageQuota mapped rate_limit.allowed onto quota.credits.allowed, which caused hasSpendableCodexCredits() to return false whenever the subscription rate limit was exhausted, defeating credit failover.
  • Removed allowed from CodexSpendableCredits / readMainPolicyQuota and decoupled hasSpendableCodexCredits() from rate_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 typecheck
  • bun run privacy:scan
  • bun run structure:check

(Note: A full repository test run was not executed locally because src/codex/quota.ts cascades 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

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

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

  • Bug Fixes
    • Accounts with available credits can now remain selectable when an upstream limit is marked as reached or access is marked as disallowed. Existing credit balance and eligibility checks still apply.

@github-actions

github-actions Bot commented Oct 4, 2026

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 4, 2026
@sungyongcho
sungyongcho marked this pull request as draft October 4, 2026 10:47
@github-actions

github-actions Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (0/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 0/4).

Review readiness checklist

  • ⬜ 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.

0/4 boxes ticked.

This PR stays in draft until every box above is ticked.

@coderabbitai

coderabbitai Bot commented Oct 4, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

🧰 Additional context used
📚 Code guidelines (1)
src/AGENTS.md — auto-discovered

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 751a70bf-9ac5-4765-a094-f27b69ef700f
📥 Commits

Reviewing files that changed from the base of the PR and between 33185c2 and a4087af.

📒 Files selected for processing (4)
  • src/codex/quota-types.ts
  • src/codex/quota.ts
  • tests/codex-integration/codex-credits-after-limit-main.test.ts
  • tests/codex-integration/codex-quota-parser-parity.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The 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.

Changes

Spendable credit eligibility

Layer / File(s) Summary
Remove allowance flag from credit snapshots
src/codex/quota-types.ts, src/codex/quota.ts
The credit type no longer declares allowed. Disk hydration and parseUsageQuota no longer copy the flag into credit snapshots.
Update eligibility and test exhausted-limit cases
src/codex/quota-types.ts, tests/codex-integration/codex-credits-after-limit-main.test.ts, tests/codex-integration/codex-quota-parser-parity.test.ts
hasSpendableCodexCredits no longer rejects credits when allowed is false. Integration tests verify that a main account with a positive balance remains selectable after the included limit is exhausted.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to a4087

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 Review

Security architecture risk: 🔵 Low · up to a4087

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The effective exposure is additional upstream requests using main or pool accounts already configured for credit spending when their included allowance is exhausted. The eligibility change does not itself grant access to another account identity or remove credential admission checks.

Trust Boundaries and Controls

  • observed — Local quota eligibility remains separate from credential authority. Main requests retain caller and account-policy checks; stored-main credential substitution retains admission ownership; materialization validates the selected account and rechecks policy before returning upstream credentials.

Resilience and Maintainability Implications

  • observed — Quota publication retains current-writer, identity, credential-generation, and provenance checks. Refresh failures do not manufacture a fresh credit observation, and inspected refusal paths retain probe or selection release behavior. These controls constrain stale or interrupted work independently of the removed allowance veto.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 4 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: allow spendable credits when the subscription rate limit is exhausted.
Linked Issues check ✅ Passed Issue [#6571] requires main-account credit use to remain available when the included usage window is exhausted but credits remain spendable. src/codex/quota-types.ts removes allowed from `CodexSpe…
Out of Scope Changes check ✅ Passed The reported changes are limited to credit eligibility parsing, its type, and focused regression tests in src/codex/quota-types.ts, src/codex/quota.ts, `tests/codex-integration/codex-credits-after…
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/codex/quota-types.ts
@@ -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

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

@Ingwannu

Ingwannu commented Oct 4, 2026

Copy link
Copy Markdown
Owner

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.

@Ingwannu Ingwannu closed this Oct 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants