fix(codex): bound reset-credit lookup responses - #1396
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ 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:
📝 WalkthroughWalkthroughThe reset-credit lookup now reads upstream JSON within a byte limit, handles invalid or truncated bodies, propagates client cancellation, cancels streams, validates response fields, and returns sanitized 502 responses for invalid payloads. ChangesReset-credit response handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResetCreditLookup
participant Upstream
participant BoundedJsonReader
Client->>ResetCreditLookup: request reset-credit data
ResetCreditLookup->>Upstream: fetch with linked abort signal
Upstream-->>BoundedJsonReader: return response body
BoundedJsonReader-->>ResetCreditLookup: validated fields or parsing failure
ResetCreditLookup-->>Client: data or sanitized 502 response
Possibly related PRs
Suggested reviewers: 🚥 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 |
github-actions
Bot
added
bug
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Wibias
added
the
maintainer-sponsored
github-actions
Bot
removed
the
intake: hygiene-blocked
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/auth-api.ts`:
- Around line 1723-1752: Update the fetch flow in the rate-limit reset handler
around linkedSignal and fetch so rejections caused by linkedSignal.signal
abortion return the sanitized “Invalid upstream reset-credit response” JSON with
status 502. Preserve the existing outer-catch behavior for unrelated fetch
failures, and add a regression test where the mocked fetch rejects in response
to the passed abort signal.
🪄 Autofix
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: 070b74a5-feb3-4f9d-bd90-1eda93778abc
📒 Files selected for processing (2)
src/codex/auth-api.tstests/codex-auth-api.test.ts
512c254 to
96ccc26
Compare
|
|
|
Tip For best results, initiate chat on the files or code changes.
You are interacting with an AI system. |
|
|
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Thanks @luvs01. This is a useful hardening change: reset-credit lookup now has a strict response-size boundary, follows client cancellation, rejects malformed upstream data safely, and only exposes the fields management clients actually need. That reduces memory/DoS risk and avoids leaking upstream or abort details through this management surface. I reviewed the current head against latest dev; the prior abort-path finding is fixed, there are no unresolved review threads, and exact-head CI is green. LGTM.
The A-phase audit of WP7 found the unit still described a train that had stopped. Corrections: - 011 records the re-pick to 9c05134 and voids the omission risk acceptance: #1398, #1396, and #1010 are all ancestors of the new RC, so nothing is being left out. Old-RC gate evidence is explicitly not reused. - 012 states that the reviewed tree and the released tree differ only by 012 itself, proves 0de4fd2 is a real ancestor, and marks the owner decision packet resolved rather than pending. - 010 names the concrete clean main worktree and drops the stale out-of-scope line about #1398. Also withholds the file path of SEC-03, which is still unfixed. Naming the file of an open finding is the disclosure the rest of the record avoids; the audit that reviewed these docs missed it.
2 participants
Summary
Verification
bun test tests/codex-auth-api.test.ts --test-name-pattern "reset-credit lookup"— 9 passed, 0 failedbun run typecheck— passedbun run privacy:scan— passedgit diff --check— passedbun run test— the changed lookup tests passed; the Windows run later encountered unrelated identity/catalog fixture failures and a Bun 1.3.14 panicChecklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit