Conversation
WalkthroughThis PR adds proxy-aware OAuth token operations by introducing Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
service/codex_oauth.go (2)
8-8:encoding/jsonis still directly imported — used indecodeJWTClaims(Line 313).The coding guidelines require using
commonwrappers instead ofencoding/json. While the usage at Line 313 is pre-existing and unchanged, you've already added thecommonimport in this PR. Consider migratingdecodeJWTClaimstocommon.Unmarshalto fully comply and allow dropping theencoding/jsonimport.Proposed fix
func decodeJWTClaims(token string) (map[string]any, bool) { parts := strings.Split(token, ".") if len(parts) != 3 { return nil, false } payloadRaw, err := base64.RawURLEncoding.DecodeString(parts[1]) if err != nil { return nil, false } var claims map[string]any - if err := json.Unmarshal(payloadRaw, &claims); err != nil { + if err := common.Unmarshal(payloadRaw, &claims); err != nil { return nil, false } return claims, true }This would also allow removing the
"encoding/json"import from line 8.As per coding guidelines: "Do NOT directly import or call
encoding/jsonin business code; use wrapper functions fromcommon/json.goinstead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/codex_oauth.go` at line 8, The decodeJWTClaims function currently uses encoding/json directly; replace that usage with the common.Unmarshal wrapper (call common.Unmarshal into the same target struct/value) and update error handling to match existing patterns, then remove the "encoding/json" import from the file; refer to the decodeJWTClaims function and common.Unmarshal helper when making the change so the file no longer imports encoding/json.
123-128: Status-code check after body decode loses the real error on non-JSON error responses.In both
refreshCodexOAuthToken(Line 123) andexchangeCodexAuthorizationCode(Line 184), the response body is decoded before checking the status code. If the server returns a non-2xx response with a non-JSON body (e.g., an HTML error page),DecodeJsonfails first, and the caller sees a JSON parse error instead of the meaningful HTTP status code.This is pre-existing logic (only the decode call changed), but since you're already touching these lines, consider swapping the order:
Proposed fix (same pattern applies to both functions)
For
refreshCodexOAuthToken:- if err := common.DecodeJson(resp.Body, &payload); err != nil { - return nil, err - } if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("codex oauth refresh failed: status=%d", resp.StatusCode) } + if err := common.DecodeJson(resp.Body, &payload); err != nil { + return nil, err + }For
exchangeCodexAuthorizationCode:- if err := common.DecodeJson(resp.Body, &payload); err != nil { - return nil, err - } if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("codex oauth code exchange failed: status=%d", resp.StatusCode) } + if err := common.DecodeJson(resp.Body, &payload); err != nil { + return nil, err + }Also applies to: 184-189
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/codex_oauth.go` around lines 123 - 128, The response body is being JSON-decoded before checking HTTP status, causing JSON parse errors to hide non-2xx responses; in both refreshCodexOAuthToken and exchangeCodexAuthorizationCode swap the order so you first check resp.StatusCode (treat non-2xx as an error and read/attach the raw body for context) and only then call common.DecodeJson into payload for 2xx responses, ensuring errors return the HTTP status and response body when available while successful flows still decode payload.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@service/codex_oauth.go`:
- Line 8: The decodeJWTClaims function currently uses encoding/json directly;
replace that usage with the common.Unmarshal wrapper (call common.Unmarshal into
the same target struct/value) and update error handling to match existing
patterns, then remove the "encoding/json" import from the file; refer to the
decodeJWTClaims function and common.Unmarshal helper when making the change so
the file no longer imports encoding/json.
- Around line 123-128: The response body is being JSON-decoded before checking
HTTP status, causing JSON parse errors to hide non-2xx responses; in both
refreshCodexOAuthToken and exchangeCodexAuthorizationCode swap the order so you
first check resp.StatusCode (treat non-2xx as an error and read/attach the raw
body for context) and only then call common.DecodeJson into payload for 2xx
responses, ensuring errors return the HTTP status and response body when
available while successful flows still decode payload.
1 participant
#2718
刷新授权/获取授权 走渠道代理设置
Summary by CodeRabbit
Release Notes