Repository navigation
fix(oauth): bound and classify ChatGPT token refresh failures - #6792
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughChatGPT OAuth token exchange and refresh now use bounded requests with deadlines, cancellation handling, and sanitized errors. Provider refresh uses explicit terminal error classification. Tests and documentation cover these changes. Planning documents record the related carry PR scope and verification. ChangesChatGPT OAuth hardening
Carry-PR planning
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Provider as ChatGPT registered provider
participant Refresh as refreshChatGPTToken
participant Request as bounded token-request helper
participant Endpoint as OAuth endpoint
Provider->>Refresh: refresh token and AbortSignal
Refresh->>Request: form request and timeout option
Request->>Endpoint: fetch with caller cancellation and deadline
Endpoint-->>Request: headers and response body
Request-->>Refresh: bounded response or sanitized error
Refresh-->>Provider: token result or classified error
Merge Risk: ⚪ Minimal · up to No confirmed merge-blocking issue remains; the planned exact-head checks can proceed. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 5 files. (7 skipped: 7 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
![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: 2c0fc34c84
ℹ️ 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".
2c0fc34 to
d9f3991
Compare
d9f3991 to
e5da7e4
Compare
1 participant
Summary
The registered ChatGPT OAuth refresh provider dropped caller cancellation, had no fetch deadline, read token-endpoint bodies without a size bound, and classified refresh failures incompletely, so a stalled token service could hang a refresh and some failures could be treated the wrong way. This carries #6745 by @luvs01 onto current
dev(after #6743's registry refactor) and adds the fixes found in review.ChatGptTokenErrorcarries HTTP status and an allowlisted OAuth code. A failure is terminal only when an allowlisted terminal grant code is present (flat or nestedinvalid_grant,refresh_token_reused,refresh_token_invalidated,token_invalidated,refresh_token_expired); provider free-text such aserror_descriptionor a nestederror.messagenever forces re-login. Terminal refusals mark only the matching credential generationneedsReauth, so a refresh that fails after the credential was replaced cannot quarantine the replacement. The shared Codex classifier insrc/codex/chatgpt-refresh-failure.tsis unchanged for its other callers.AbortError/TimeoutError/generic) without the raw reason or cause, and never mark reauthentication. The registry's own stale-flightOAuthTokenRefreshStaleErrorpath is preserved.structure/providers-and-adapters.mdand the accounts reference page.Carries #6745. Plan and review record:
devlog/_plan/261009_L5-auth-quota/.Co-authored-by: 김상훈 luvs01@hanmail.net
Verification
bun test tests/oauth/chatgpt-token-expiry.test.ts tests/oauth/oauth-refresh-generic-lock.test.ts tests/oauth/chatgpt-device-auth.test.ts tests/oauth/chatgpt-token-boundaries.test.ts tests/codex-integration/codex-account-store-refresh-classification.test.ts tests/codex-integration/codex-main-account-refresh.test.ts tests/ci-workflows/file-size-ratchet.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts tests/lab/core-lab-boundary.test.ts→ 162 pass, 0 fail.tests/oauth/chatgpt-token-boundaries.test.ts(registered in both layout files) covers headers-then-stalled-body deadlines, cancellation during body reads, byte ceilings, and an instrumented caller signal asserting zero outstanding listeners after success, HTTP failure, timeout, transport failure and parse failure.bun run typecheck,bun run privacy:scan,bun run structure:check→ pass.bun run testand the docs-site build were not run locally (minimal local testing for this lane; several worktrees share the machine). Both are left to exact-head hosted CI. All token-endpoint responses in tests are synthetic; no real credentials or accounts were used.Checklist
MAINTAINERS.mdstill applies before merge.)Summary by CodeRabbit