feat(codex): classify reset-eligible quota rejection - #866
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCodex pre-stream responses now receive strict structured rejection classification. The classifier fails closed for invalid, unavailable, oversized, consumed, or cancelled bodies. Fatal UTF-8 decoding is supported. The existing 402/429 pool-account retry predicate is now exported. Tests cover schemas, status categories, malformed bodies, cancellation, and response preservation. ChangesCodex quota retry handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant CodexResponse
participant QuotaClassifier
participant BoundedBody
participant RejectionResult
CodexResponse->>QuotaClassifier: status and response body
QuotaClassifier->>BoundedBody: clone and decode bounded body
BoundedBody-->>QuotaClassifier: decoded JSON or read failure
QuotaClassifier->>RejectionResult: classified rejection and eligibility
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Pre-merge review result: not merge-ready yet. CI is green and the bounded body handling, privacy posture, and Bun compatibility all check out — but the classifier does not hold the fail-closed boundary the PR description promises:
Please: exact-match allowlisted values within explicitly supported response schemas, reject on conflict, and add the negative tests. This is the semantic foundation for the #657 recovery family, so it is worth getting airtight — happy to re-review quickly once updated. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/server/responses/core.ts`:
- Around line 250-255: Update shouldRetryCodexPoolAccountQuota to be synchronous
and return true only when response.status is 402 or 429, avoiding
classifyCodexPreStreamRejection and any response-body read. Remove the
now-unused classifier import while preserving the existing boolean retry
contract.
In `@tests/codex-quota-rejection.test.ts`:
- Around line 12-13: Extend the test suite in “Codex pre-stream quota rejection
classification” with handleResponses integration coverage for Codex pool
retries. Exercise the reset-eligible 429 and 402 paths in handleResponses,
asserting each retries on an alternate account, and add a non-retryable-status
case asserting the original status and response body are preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3d172e90-de04-4759-a458-eb24d70ac61a
📒 Files selected for processing (3)
src/codex/quota-rejection.tssrc/server/responses/core.tstests/codex-quota-rejection.test.ts
aef44bd to
cfe9bd0
Compare
|
All previously requested changes have been applied to the current head (fe44ece), all checks are green, and no review threads remain. When convenient, could you please re-review this revision? Thank you. |
[shipping-github] Verdict: changes-requestedPR: Semantic propagation
Linked: refs UsefulnessUseful and correctly scoped groundwork: this is the maintainer-requested first slice of #657 (semantic classification only, no credit spending, no waiting, no policy), and the classifier is deliberately strict - exact structured codes on 429/402 only, fail-closed on ambiguity, malformed, oversized, consumed, or cancelled bodies. The earlier maintainer findings (case/whitespace normalization, root/nested ambiguity, missing negatives) are all addressed on this head. Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
GateShip gate: Bottom lineThe code is sound, well-tested, and security-clean: the classifier is fail-closed, #584 behavior is provably unchanged, and CI is green on the head. Three items stand between this PR and merge-ready: (1) @lidge-jun re-review to clear the gate (the author already asked), (2) a one-bullet PR-description correction so the summary matches the unwired classifier on the head, (3) optional but cheap - classifier-level negatives for oversized/truncated/consumed bodies, plus an update from |
fe44ece to
d5519db
Compare
|
Updated the current revision to
The new fork workflow runs are awaiting maintainer approval and contain no jobs yet: Cross-platform CI and React Doctor. @lidge-jun, when convenient, could you please approve those runs and re-review this revision? Thank you. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/bounded-body.ts (1)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that
fatalUtf8: truemakesreadBoundedResponseBodyreject.With
fatal: true,TextDecoder.decode()throws aTypeErroron malformed or truncated UTF-8. Both call sites (Line 163 and Line 176) calldecodeUtf8insidereturnexpressions. The throw therefore propagates out of thetryblock, is re-thrown by thecatchat Line 221, and reaches the caller as a rejected promise. Thefinallyblock still cancels the reader and releases the lock, so there is no resource leak.The consequence is a contract change that is not visible from the option name. On the timeout path the caller no longer receives
{ timedOut: true, displaySafe: false }; it receives a rejection instead. The current consumersrc/codex/quota-rejection.ts:177-189wraps the call in try/catch and fails closed, so present behavior is correct. Future callers can miss this.Update the option doc so the rejection path is explicit.
📝 Proposed doc clarification
- /** Reject malformed UTF-8 instead of replacing it. Defaults to false. */ + /** + * Reject malformed UTF-8 instead of replacing it. Defaults to false. + * When true, decoding throws a `TypeError` for malformed or truncated + * UTF-8, and this function rejects instead of returning a result. The + * reader is still cancelled and released. + */ fatalUtf8?: boolean;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/bounded-body.ts` around lines 80 - 87, Update the documentation for the fatalUtf8 option used by readBoundedResponseBody to explicitly state that enabling it rejects the promise with a TypeError when malformed or truncated UTF-8 is encountered, including on the timeout path, rather than returning a timedOut result. Preserve the existing cleanup and decoding behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codex/quota-rejection.ts`:
- Around line 209-220: No code change is required: leave
classifyCodexPreStreamRejection and its current bounded-body read unchanged
because alternate-account retry is not yet wired to this classifier.
- Around line 129-137: Rename hasDuplicateJsonObjectKeys to a predicate that
reflects all unsafe outcomes, such as isUnsafeJsonDocument, and update its call
site at the quota-rejection logic accordingly. Preserve the existing return
conditions and fail-closed behavior without inverting or otherwise changing the
predicate.
- Around line 3-5: Make the reset-eligible exhaustion codes single-source by
declaring the literals in a const tuple and deriving
CodexResetEligibleExhaustionCode from that tuple. Remove the duplicate Set
definition and update exactResetEligibleCode to use the resulting
RESET_ELIGIBLE_CODE_SET, preserving its existing behavior.
- Around line 177-183: Extend the tests for the quota-rejection flow around the
response-body guards to directly cover oversized/display-unsafe bodies,
inactivity-truncated bodies, empty bodies, clone failures, and invalid UTF-8,
asserting each is rejected without reset eligibility. Add duplicate-key cases
for root and nested objects, plus conflicting code/type payloads, and verify all
fail closed without triggering the irreversible reset-credit operation.
- Around line 83-85: Replace the number parsing around the scalar scanner with a
module-level sticky regex that matches directly at the current input offset. Set
the regex’s lastIndex to start immediately before exec, then use the match
result and matched length to compute next; preserve the existing SyntaxError and
duplicate:false behavior when no number matches.
---
Outside diff comments:
In `@src/lib/bounded-body.ts`:
- Around line 80-87: Update the documentation for the fatalUtf8 option used by
readBoundedResponseBody to explicitly state that enabling it rejects the promise
with a TypeError when malformed or truncated UTF-8 is encountered, including on
the timeout path, rather than returning a timedOut result. Preserve the existing
cleanup and decoding behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 355d2400-aded-4995-9dc6-79ca84eb4a1e
📒 Files selected for processing (2)
src/codex/quota-rejection.tssrc/lib/bounded-body.ts
|
✅ PR quality gates passed This pull request now targets The title was left unchanged. The pull request has been marked ready for review again. |
|
Latest review follow-up is complete on
The PR quality gate is green again and this revision is ready for maintainer re-review. |
[GD] Verdict: changes-requestedPR: Semantic propagation
Linked: UsefulnessUseful and correctly scoped: this is the maintainer-requested first slice of Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Nothing was edited or pushed (foreign PR). Bounded, optional candidates:
Rejected candidates: narrowing the duplicate-key scan to root/ GateShip gate (review mode): Bottom lineSound, well-tested, security-clean groundwork that matches |
454a7c6 to
d142f06
Compare
|
Thanks @luvs01 — merging this. Why it helps: |
Summary
usage_limit_exceededandinsufficient_quotacodes on HTTP 429/402 as reset-credit eligibleSafety boundary
Explicit non-goals
This PR deliberately does not implement:
Those remain separate follow-up slices under the maintainer direction in #657.
Compatibility
The existing #584 behavior is preserved: pre-stream 429/402 still permits one bounded alternate-account attempt even when the rejection is generic or unverified. The new semantic classifier has no production call site in this PR; it only establishes the boundary that future irreversible recovery must require.
Verification
1.4.0-canary.1: 119 focused tests passed across quota classification, server auth/retry, bounded-body, and error-fidelity suites1.3.14: the same 119 focused tests passedgit diff --checkpassedRefs #657
Summary by CodeRabbit