ci: sync branch feat/clean-base-1698-strip-empty-messages-rebase#517
ci: sync branch feat/clean-base-1698-strip-empty-messages-rebase#517KooshaPari merged 15 commits intomainfrom
Conversation
- Add 60 requests/minute rate limiting per credential using sliding window - Detect insufficient_quota errors and set cooldown until next day (Beijing time) - Map quota errors (HTTP 403/429) to 429 with retryAfter for conductor integration - Cache Beijing timezone at package level to avoid repeated syscalls - Add redactAuthID function to protect credentials in logs - Extract wrapQwenError helper to consolidate error handling
…ndling-clean feat(qwen): add rate limiting and quota error handling
…-refresh-interval fix(auth): respect configured auto-refresh interval
…resh-token-reused-no-retry fix(codex): stop retrying refresh_token_reused errors
…paction-compat feat: add codex responses compatibility for compaction payloads
- Expand OAuth scope to include read:user for full profile access - Add GitHubUserInfo struct with Login, Email, Name fields - Update FetchUserInfo to return complete user profile - Add Email and Name fields to CopilotTokenStorage and CopilotAuthBundle - Fix provider string bug: 'github' -> 'github-copilot' in auth_files.go - Fix semantic bug: email field was storing username - Update Label to prefer email over username in both CLI and Web API paths - Add 9 unit tests covering new functionality
- Add 'github-user' fallback in WaitForAuthorization when FetchUserInfo returns empty Login (fixes malformed 'github-copilot-.json' filenames) - Standardize Web API file name to 'github-copilot-<user>.json' to match CLI path convention (was 'github-<user>.json') Addresses Gemini Code Assist review comments on PR #291.
feat(copilot): fetch and persist user email and display name on login
Cherry-picked from merge/1698-strip-empty-messages-openai-to-claude into aligned base
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (13)
📝 WalkthroughWalkthroughChanges enhance GitHub Copilot authentication by replacing the "github" provider identifier with "github-copilot," adding user email and name fields to token storage and metadata, and fetching complete user information. OpenAI authentication now handles non-retryable refresh errors to prevent unnecessary retries. Qwen executor introduces comprehensive rate limiting with quota detection. Translator layers add empty-field guards and compatibility field stripping. Minor refactoring relaxes interval clamping logic. Changes
Sequence DiagramssequenceDiagram
actor Client
participant CopilotAuth as Copilot Auth Handler
participant GitHub as GitHub API
participant Storage as Token Storage
Client->>CopilotAuth: Initiate authentication
CopilotAuth->>GitHub: Request device code (scope: user:email, read:user)
GitHub-->>CopilotAuth: Return device code
CopilotAuth-->>Client: Display device code
Client->>GitHub: Approve authorization (user flow)
Client->>CopilotAuth: Poll for authorization completion
CopilotAuth->>GitHub: Exchange device code for access token
GitHub-->>CopilotAuth: Return access token
CopilotAuth->>GitHub: Fetch user info (Login, Email, Name)
GitHub-->>CopilotAuth: Return user details
CopilotAuth->>Storage: Save token with user info (email, name)
Storage-->>CopilotAuth: Token persisted
CopilotAuth-->>Client: Return auth bundle with user info
sequenceDiagram
actor Client
participant Executor as Qwen Executor
participant RateLimiter as Rate Limiter
participant Qwen as Qwen API
participant ErrorHandler as Error Handler
Client->>Executor: Execute request (credential ID)
Executor->>RateLimiter: Check sliding-window rate limit
alt Rate limit exceeded
RateLimiter-->>Executor: Reject (retry-after calculated)
Executor-->>ErrorHandler: Map to quota error with retry guidance
ErrorHandler-->>Client: Return 429-like error
else Within limits
Executor->>Qwen: Send request
Qwen-->>Executor: Receive response
alt Quota exceeded response
Executor->>ErrorHandler: Detect quota error code
ErrorHandler-->>Executor: Map to retry semantics with reset time
Executor-->>Client: Return normalized quota error
else Success
Executor-->>Client: Return response
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
Summary of ChangesHello @KooshaPari, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily focuses on refining authentication processes for GitHub Copilot and improving API interaction logic for Qwen and Codex. It enhances user data handling for Copilot, introduces robust rate limiting and quota management for Qwen, and ensures better compatibility for OpenAI Responses API requests with Codex. Additionally, it refines token refresh error handling for Codex and optimizes request translation for Claude. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces several valuable improvements across the codebase. Key changes include enhancing the GitHub Copilot authentication to fetch more user details, adding robust rate limiting and quota handling to the Qwen executor, and improving the Codex auth refresh logic to handle non-retryable errors. Additionally, the Claude translator now correctly strips empty messages, and a bug in the auth conductor's auto-refresh interval has been fixed. The inclusion of new tests for these features is a great practice. I've identified one medium-severity issue in the new Qwen rate-limiting logic that could lead to a minor memory leak and have provided a suggestion for a fix. Overall, these are solid improvements to the system's reliability and functionality.
| if len(validTimestamps) >= qwenRateLimitPerMin { | ||
| // Calculate when the oldest request will expire | ||
| oldestInWindow := validTimestamps[0] | ||
| retryAfter := oldestInWindow.Add(qwenRateLimitWindow).Sub(now) | ||
| if retryAfter < time.Second { | ||
| retryAfter = time.Second | ||
| } | ||
| retryAfterSec := int(retryAfter.Seconds()) | ||
| return statusErr{ | ||
| code: http.StatusTooManyRequests, | ||
| msg: fmt.Sprintf(`{"error":{"code":"rate_limit_exceeded","message":"Qwen rate limit: %d requests/minute exceeded, retry after %ds","type":"rate_limit_exceeded"}}`, qwenRateLimitPerMin, retryAfterSec), | ||
| retryAfter: &retryAfter, | ||
| } | ||
| } |
There was a problem hiding this comment.
There's a potential for a minor memory leak here. If the rate limit is exceeded, the function returns without pruning the expired timestamps from qwenRateLimiter.requests[authID]. The expired timestamps will remain in memory until a request is allowed for this authID.
To fix this, you should update the timestamps slice for the authID with the pruned validTimestamps before returning the rate limit error.
| if len(validTimestamps) >= qwenRateLimitPerMin { | |
| // Calculate when the oldest request will expire | |
| oldestInWindow := validTimestamps[0] | |
| retryAfter := oldestInWindow.Add(qwenRateLimitWindow).Sub(now) | |
| if retryAfter < time.Second { | |
| retryAfter = time.Second | |
| } | |
| retryAfterSec := int(retryAfter.Seconds()) | |
| return statusErr{ | |
| code: http.StatusTooManyRequests, | |
| msg: fmt.Sprintf(`{"error":{"code":"rate_limit_exceeded","message":"Qwen rate limit: %d requests/minute exceeded, retry after %ds","type":"rate_limit_exceeded"}}`, qwenRateLimitPerMin, retryAfterSec), | |
| retryAfter: &retryAfter, | |
| } | |
| } | |
| if len(validTimestamps) >= qwenRateLimitPerMin { | |
| // Prune expired timestamps to prevent memory leak while rate-limited. | |
| if len(validTimestamps) < len(timestamps) { | |
| qwenRateLimiter.requests[authID] = validTimestamps | |
| } | |
| // Calculate when the oldest request will expire | |
| oldestInWindow := validTimestamps[0] | |
| retryAfter := oldestInWindow.Add(qwenRateLimitWindow).Sub(now) | |
| if retryAfter < time.Second { | |
| retryAfter = time.Second | |
| } | |
| retryAfterSec := int(retryAfter.Seconds()) | |
| return statusErr{ | |
| code: http.StatusTooManyRequests, | |
| msg: fmt.Sprintf("{\"error\":{\"code\":\"rate_limit_exceeded\",\"message\":\"Qwen rate limit: %d requests/minute exceeded, retry after %ds\",\"type\":\"rate_limit_exceeded\"}}", qwenRateLimitPerMin, retryAfterSec), | |
| retryAfter: &retryAfter, | |
| } | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (4)
internal/translator/claude/openai/chat-completions/claude_openai_request.go (1)
22-26: 🧹 Nitpick | 🔵 TrivialPre-existing: Package-level mutable state without synchronization.
The global variables
user,account,sessionare initialized lazily inConvertOpenAIRequestToClaude(lines 48-59) without synchronization. If this function is called concurrently, there's a potential data race. Consider usingsync.Oncefor thread-safe initialization.This is outside the scope of the current PR changes but worth addressing separately.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/translator/claude/openai/chat-completions/claude_openai_request.go` around lines 22 - 26, The package-level variables user, account, and session are lazily initialized in ConvertOpenAIRequestToClaude which can race under concurrent calls; replace the ad-hoc lazy init with a thread-safe one-time initializer using sync.Once (e.g., add a package-level sync.Once variable and move the initialization logic for user/account/session into a function invoked via once.Do), ensuring ConvertOpenAIRequestToClaude reads those vars only after the once.Do has run.sdk/auth/github_copilot.go (1)
35-118: 🛠️ Refactor suggestion | 🟠 MajorRefactor
Logininto smaller units to meet function-size policy.The function is significantly over the 40-line limit and now combines device-flow UX, browser launching, token verification, metadata construction, and record assembly.
As per coding guidelines,
Maximum function length: 40 lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/auth/github_copilot.go` around lines 35 - 118, The Login method in GitHubCopilotAuthenticator is too long and should be split into focused helpers: extract device-flow initiation and user prompt into a helper (e.g., startDeviceFlowAndPrompt(ctx, authSvc, opts) that returns deviceCode), move the polling/authorization into waitForAuthorization(ctx, authSvc, deviceCode) (returns authBundle), isolate Copilot token verification into verifyCopilotAccess(ctx, authSvc, accessToken) (returns apiToken), and build metadata + assemble the coreauth.Auth into buildAuthResult(authBundle, tokenStorage, apiToken) so that Login delegates to these smaller functions (preserve existing symbols: GitHubCopilotAuthenticator.Login, copilot.NewCopilotAuth, authSvc.StartDeviceFlow, authSvc.WaitForAuthorization, authSvc.GetCopilotAPIToken, authSvc.CreateTokenStorage) and ensure Login becomes a short orchestrator that calls the new helpers.internal/auth/copilot/oauth.go (1)
225-271: 🛠️ Refactor suggestion | 🟠 MajorSplit
FetchUserInfointo smaller helpers.This function now exceeds the 40-line max and is handling request construction, transport, status handling, decode, and validation in one block. Please extract at least response parsing/validation into helpers.
As per coding guidelines,
Maximum function length: 40 lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/auth/copilot/oauth.go` around lines 225 - 271, FetchUserInfo is too long; split response parsing/validation into helpers to meet the 40-line rule: extract decoding/validation into a function like parseGitHubUserInfo(resp *http.Response) (GitHubUserInfo, error) that reads/decodes the JSON, checks raw.Login != "" and returns the typed GitHubUserInfo or a NewAuthenticationError, and optionally extract request creation into buildUserInfoRequest(ctx context.Context, accessToken string) (*http.Request, error); then simplify FetchUserInfo to call these helpers, keep the HTTP call and response close there, and return the helper's result so FetchUserInfo stays under 40 lines.internal/api/handlers/management/auth_files.go (1)
1924-2015: 🛠️ Refactor suggestion | 🟠 Major
RequestGitHubTokenshould be decomposed to satisfy function-length policy.This handler is far above the configured 40-line maximum and now contains multiple responsibilities (polling, user-info fallback, storage, record persistence, session completion).
As per coding guidelines,
Maximum function length: 40 lines.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handlers/management/auth_files.go` around lines 1924 - 2015, RequestGitHubToken is too long and mixes responsibilities (device initiation, polling, user-info handling, token storage, session updates); refactor by extracting the goroutine body into smaller helpers: e.g., InitGitHubDeviceFlow(ctx, deviceClient) to perform RequestDeviceCode and return (deviceCode, authURL, userCode), PollAndFetchToken(ctx, deviceClient, deviceCode) to handle PollForToken and FetchUserInfo, BuildTokenRecord(userInfo, tokenData) to construct the coreauth.Auth and copilot.CopilotTokenStorage, and SaveAndCompleteSession(ctx, h, state, record) to call h.saveTokenRecord, SetOAuthSessionError, CompleteOAuthSession and CompleteOAuthSessionsByProvider. Keep RequestGitHubToken to orchestration only (call RegisterOAuthSession, start the goroutine that calls these helpers, and return the initial JSON), ensuring RegisterOAuthSession, SetOAuthSessionError, CompleteOAuthSession, CompleteOAuthSessionsByProvider, and h.saveTokenRecord usages remain in the appropriate helpers so the public handler stays under 40 lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/auth/copilot/token.go`:
- Around line 29-32: The new PII fields Email and Name are persisted by
SaveTokenToFile which currently uses os.Create (respecting umask) and can
produce world-readable files; modify SaveTokenToFile to open the file with
os.OpenFile and explicit owner-only permissions (0o600 / 0600) when creating the
token file, ensuring you pass os.O_WRONLY|os.O_CREATE|os.O_TRUNC and
FileMode(0o600) so the token containing Email/Name is guaranteed owner-only;
locate and update the SaveTokenToFile function in token.go and remove reliance
on os.Create so owner-only permissions are enforced at creation.
In `@internal/runtime/executor/qwen_executor.go`:
- Around line 71-76: The current early return when authID == "" in the
rate-limit check (see the authID conditional and log.Debug call) creates a
bypass; instead, remove the return and derive a stable fallback bucket key for
rate limiting (e.g., an "anon" bucket or a token-derived hash) so requests with
empty or malformed authID still hit the limiter; update the rate-limit call to
use that fallback key and keep the debug log but do not skip enforcement.
- Around line 70-120: Split checkQwenRateLimit into small helpers: implement
pruneTimestamps(authID string, windowStart time.Time) []time.Time that reads
qwenRateLimiter.requests[authID], returns the pruned valid timestamps and
deletes the map entry if empty; implement calculateRetryAfter(oldest time.Time,
now time.Time) time.Duration that computes the retryAfter (with a minimum of 1s)
using qwenRateLimitWindow; and implement recordRequest(authID string, timestamps
[]time.Time, now time.Time) to append now and write back to
qwenRateLimiter.requests; keep the lock/unlock inside checkQwenRateLimit, call
these helpers in sequence, and preserve the existing error return using
statusErr, qwenRateLimitPerMin and other semantics exactly as before.
- Around line 49-54: Replace the custom sliding-window qwenRateLimiter struct
and its associated logic with per-auth rate.Limiter instances from
golang.org/x/time/rate: create a map[string]*rate.Limiter (e.g., qwenLimiters)
protected by a mutex and a GetLimiter(authID) helper that returns/initializes a
limiter with the desired rate/burst, ensure empty authID is treated as a valid
key (do not bypass limiting), and implement TTL-based cleanup (store lastSeen
timestamps alongside limiters and run a goroutine to remove entries not used for
X duration). Update functions that referenced the old qwenRateLimiter (e.g., any
allow/request-check functions) to call GetLimiter(authID).Allow/Reserve and
remove the old sliding-window timestamp pruning code.
In
`@internal/translator/codex/openai/responses/codex_openai-responses_request_test.go`:
- Around line 284-305: The test TestContextManagementCompactionCompatibility
currently asserts removal of both "context_management" and "truncation"; narrow
its scope by removing the redundant assertions that check for "truncation" (the
gjson.Get(..., "truncation").Exists() check and its t.Fatalf), since truncation
removal is already covered by TestTruncationRemovedForCodexCompatibility; keep
the existing call to ConvertOpenAIResponsesRequestToCodex("gpt-5.2", inputJSON,
false) and the assertion that "context_management" is removed.
---
Outside diff comments:
In `@internal/api/handlers/management/auth_files.go`:
- Around line 1924-2015: RequestGitHubToken is too long and mixes
responsibilities (device initiation, polling, user-info handling, token storage,
session updates); refactor by extracting the goroutine body into smaller
helpers: e.g., InitGitHubDeviceFlow(ctx, deviceClient) to perform
RequestDeviceCode and return (deviceCode, authURL, userCode),
PollAndFetchToken(ctx, deviceClient, deviceCode) to handle PollForToken and
FetchUserInfo, BuildTokenRecord(userInfo, tokenData) to construct the
coreauth.Auth and copilot.CopilotTokenStorage, and SaveAndCompleteSession(ctx,
h, state, record) to call h.saveTokenRecord, SetOAuthSessionError,
CompleteOAuthSession and CompleteOAuthSessionsByProvider. Keep
RequestGitHubToken to orchestration only (call RegisterOAuthSession, start the
goroutine that calls these helpers, and return the initial JSON), ensuring
RegisterOAuthSession, SetOAuthSessionError, CompleteOAuthSession,
CompleteOAuthSessionsByProvider, and h.saveTokenRecord usages remain in the
appropriate helpers so the public handler stays under 40 lines.
In `@internal/auth/copilot/oauth.go`:
- Around line 225-271: FetchUserInfo is too long; split response
parsing/validation into helpers to meet the 40-line rule: extract
decoding/validation into a function like parseGitHubUserInfo(resp
*http.Response) (GitHubUserInfo, error) that reads/decodes the JSON, checks
raw.Login != "" and returns the typed GitHubUserInfo or a
NewAuthenticationError, and optionally extract request creation into
buildUserInfoRequest(ctx context.Context, accessToken string) (*http.Request,
error); then simplify FetchUserInfo to call these helpers, keep the HTTP call
and response close there, and return the helper's result so FetchUserInfo stays
under 40 lines.
In `@internal/translator/claude/openai/chat-completions/claude_openai_request.go`:
- Around line 22-26: The package-level variables user, account, and session are
lazily initialized in ConvertOpenAIRequestToClaude which can race under
concurrent calls; replace the ad-hoc lazy init with a thread-safe one-time
initializer using sync.Once (e.g., add a package-level sync.Once variable and
move the initialization logic for user/account/session into a function invoked
via once.Do), ensuring ConvertOpenAIRequestToClaude reads those vars only after
the once.Do has run.
In `@sdk/auth/github_copilot.go`:
- Around line 35-118: The Login method in GitHubCopilotAuthenticator is too long
and should be split into focused helpers: extract device-flow initiation and
user prompt into a helper (e.g., startDeviceFlowAndPrompt(ctx, authSvc, opts)
that returns deviceCode), move the polling/authorization into
waitForAuthorization(ctx, authSvc, deviceCode) (returns authBundle), isolate
Copilot token verification into verifyCopilotAccess(ctx, authSvc, accessToken)
(returns apiToken), and build metadata + assemble the coreauth.Auth into
buildAuthResult(authBundle, tokenStorage, apiToken) so that Login delegates to
these smaller functions (preserve existing symbols:
GitHubCopilotAuthenticator.Login, copilot.NewCopilotAuth,
authSvc.StartDeviceFlow, authSvc.WaitForAuthorization,
authSvc.GetCopilotAPIToken, authSvc.CreateTokenStorage) and ensure Login becomes
a short orchestrator that calls the new helpers.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
internal/api/handlers/management/auth_files.gointernal/auth/codex/openai_auth.gointernal/auth/codex/openai_auth_test.gointernal/auth/copilot/copilot_auth.gointernal/auth/copilot/oauth.gointernal/auth/copilot/oauth_test.gointernal/auth/copilot/token.gointernal/runtime/executor/qwen_executor.gointernal/translator/claude/openai/chat-completions/claude_openai_request.gointernal/translator/codex/openai/responses/codex_openai-responses_request.gointernal/translator/codex/openai/responses/codex_openai-responses_request_test.gosdk/auth/github_copilot.gosdk/cliproxy/auth/conductor.go
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: NEVER create a v2 file - refactor the original instead
NEVER create a new class if an existing one can be made generic
NEVER create custom implementations when an OSS library exists - search pkg.go.dev for existing libraries before writing code
Build generic building blocks (provider interface + registry) before application logic
Use chi for HTTP routing (NOT custom routers)
Use zerolog for logging (NOT fmt.Print)
Use viper for configuration (NOT manual env parsing)
Use go-playground/validator for validation (NOT manual if/else validation)
Use golang.org/x/time/rate for rate limiting (NOT custom limiters)
Use template strings for messages instead of hardcoded messages and config-driven logic instead of code-driven
Zero new lint suppressions without inline justification
All new code must pass: go fmt, go vet, golint
Maximum function length: 40 lines
No placeholder TODOs in committed code
Files:
internal/auth/copilot/token.gosdk/cliproxy/auth/conductor.gointernal/translator/codex/openai/responses/codex_openai-responses_request_test.gointernal/auth/copilot/copilot_auth.gointernal/auth/copilot/oauth.gosdk/auth/github_copilot.gointernal/auth/copilot/oauth_test.gointernal/runtime/executor/qwen_executor.gointernal/auth/codex/openai_auth_test.gointernal/translator/codex/openai/responses/codex_openai-responses_request.gointernal/translator/claude/openai/chat-completions/claude_openai_request.gointernal/api/handlers/management/auth_files.gointernal/auth/codex/openai_auth.go
🧠 Learnings (1)
📚 Learning: 2026-02-25T10:11:41.448Z
Learnt from: CR
Repo: KooshaPari/cliproxyapi-plusplus PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-25T10:11:41.448Z
Learning: Applies to **/*.go : Use golang.org/x/time/rate for rate limiting (NOT custom limiters)
Applied to files:
internal/runtime/executor/qwen_executor.go
🧬 Code graph analysis (7)
internal/translator/codex/openai/responses/codex_openai-responses_request_test.go (1)
internal/translator/codex/openai/responses/codex_openai-responses_request.go (1)
ConvertOpenAIResponsesRequestToCodex(10-39)
internal/auth/copilot/copilot_auth.go (1)
internal/auth/copilot/token.go (1)
CopilotAuthBundle(48-57)
internal/auth/copilot/oauth.go (2)
internal/api/modules/modules.go (1)
Context(17-22)internal/auth/copilot/errors.go (2)
NewAuthenticationError(123-130)ErrUserInfoFailed(115-119)
sdk/auth/github_copilot.go (1)
sdk/cliproxy/auth/types.go (1)
Auth(46-96)
internal/auth/copilot/oauth_test.go (1)
internal/auth/copilot/oauth.go (2)
DeviceFlowClient(35-38)GitHubUserInfo(215-222)
internal/auth/codex/openai_auth_test.go (1)
internal/auth/codex/openai_auth.go (1)
CodexAuth(33-35)
internal/api/handlers/management/auth_files.go (2)
internal/api/handlers/management/oauth_sessions.go (2)
RegisterOAuthSession(176-176)CompleteOAuthSessionsByProvider(182-184)sdk/cliproxy/auth/types.go (1)
Auth(46-96)
🪛 GitHub Check: Analyze (Go) (go)
sdk/auth/github_copilot.go
[failure] 90-90:
authBundle.Name undefined (type *copilot.CopilotAuthBundle has no field or method Name)
[failure] 89-89:
authBundle.Email undefined (type *copilot.CopilotAuthBundle has no field or method Email)
[failure] 103-103:
authBundle.Email undefined (type *copilot.CopilotAuthBundle has no field or method Email)
🪛 GitHub Check: build
sdk/auth/github_copilot.go
[failure] 90-90:
authBundle.Name undefined (type *copilot.CopilotAuthBundle has no field or method Name)
[failure] 89-89:
authBundle.Email undefined (type *copilot.CopilotAuthBundle has no field or method Email)
[failure] 103-103:
authBundle.Email undefined (type *copilot.CopilotAuthBundle has no field or method Email)
🔇 Additional comments (15)
internal/translator/claude/openai/chat-completions/claude_openai_request.go (2)
159-164: LGTM!Good defensive guard that prevents emitting empty text blocks. Extracting
textContentonce and reusing it is cleaner than callingpart.Get("text").String()twice.
185-190: LGTM!Consistent with the system message handling above. This correctly filters out empty text parts from user/assistant messages.
internal/translator/codex/openai/responses/codex_openai-responses_request.go (2)
29-30: LGTM!The truncation field deletion and the new compatibility function call are appropriately placed after the other field deletions and before the user field deletion. The ordering is logical.
41-56: LGTM!The function is well-documented with clear rationale for the compatibility handling. The early return pattern and consistent error handling align with the rest of the file.
internal/translator/codex/openai/responses/codex_openai-responses_request_test.go (1)
307-320: LGTM!Focused test that clearly validates truncation field removal for Codex compatibility. Good coverage for the feature added at line 29 of the implementation.
internal/runtime/executor/qwen_executor.go (1)
149-159: Quota normalization to 429 withretryAfterpropagation looks solid.The mapping and downstream propagation are consistent across sync and stream paths.
Also applies to: 292-295, 397-403
sdk/cliproxy/auth/conductor.go (1)
1831-1833: Interval fallback logic looks correct.Line 1831 now defaults only invalid values and preserves explicit positive intervals, which matches the intended refresh configuration behavior.
internal/auth/copilot/copilot_auth.go (1)
85-100: Email/Name propagation across bundle, validation, and storage is consistent.The updated flow correctly carries
Login/Email/Namefrom user-info fetch throughCopilotAuthBundleand into persisted token storage.Also applies to: 159-165, 174-176
internal/api/handlers/management/auth_files.go (1)
1945-2005: Provider/session rename and metadata enrichment are coherent.
github-copilotis now used consistently for session registration/completion and persisted auth records, and theemail/nameadditions are propagated correctly.internal/auth/copilot/oauth_test.go (1)
30-124: Strong targeted coverage for the new GitHub profile fields and edge cases.These tests validate both happy-path profile mapping and critical edge behavior (
email: null, empty login/token, HTTP failures), plus JSON persistence semantics forName.Also applies to: 126-213
internal/auth/codex/openai_auth.go (1)
279-282: Non-retryable refresh detection is a good reliability fix.Short-circuiting on
refresh_token_reusedavoids redundant retries and surfaces the actionable error earlier.Also applies to: 291-297
sdk/auth/github_copilot.go (1)
89-90: No action needed on Email/Name fields.The
CopilotAuthBundlestruct ininternal/auth/copilot/token.goproperly defines bothNameas string fields. The code at lines 89–90 and 103–106 correctly accesses these fields; no compile error should occur.Likely an incorrect or invalid review comment.
internal/auth/copilot/oauth.go (1)
253-260: Go's JSON decoder does not fail on null email; it silently treats it as empty string.JSON
nullunmarshalled into a non-pointerstringfield does not error—it simply leaves the field unchanged (as the zero value""). If GitHub returnsemail: nullfor private accounts, the current code will decode it to an empty string without raising an error. Only use*stringif the application must distinguish betweennull(user chose privacy) and""(no email provided). If empty string is acceptable for both cases, no change is required.Likely an incorrect or invalid review comment.
internal/auth/codex/openai_auth_test.go (2)
18-44: Good regression coverage for non-retryable refresh errors.The test clearly validates immediate failure behavior and confirms retry suppression via single-attempt assertion.
12-16: No changes needed; customRoundTripperis the idiomatic pattern for this test.The
roundTripFunchelper (lines 12-16) is a standard, idiomatic Go approach for mockinghttp.Client.Transportat the request level. It directly implements thehttp.RoundTripperinterface to intercept and control client behavior in unit tests—which is the exact use case here.
net/http/httptestis designed for different purposes: testing HTTP server handlers viaNewRequest/ResponseRecorder, or spinning up real in-process test servers viaNewServer. It does not replace transport-layer mocking via customRoundTripperimplementations.The guideline against custom implementations applies to replacing existing libraries/functionality, not to implementing standard interfaces for test mocking. This pattern is correct and widely used across Go codebases.
Likely an incorrect or invalid review comment.
| // Email is the GitHub email address associated with this token. | ||
| Email string `json:"email,omitempty"` | ||
| // Name is the GitHub display name associated with this token. | ||
| Name string `json:"name,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/auth/copilot/token.go | sed -n '25,40p'Repository: KooshaPari/cliproxyapi-plusplus
Length of output: 925
🏁 Script executed:
cat -n internal/auth/copilot/token.go | sed -n '80,105p'Repository: KooshaPari/cliproxyapi-plusplus
Length of output: 866
🏁 Script executed:
rg -n 'SaveTokenToFile|os\.Create|os\.OpenFile' internal/auth/copilot/token.go -A 3 -B 1Repository: KooshaPari/cliproxyapi-plusplus
Length of output: 746
Explicit 0600 file mode required when persisting PII.
Lines 29–32 add PII fields (Email, Name) to the token storage. However, SaveTokenToFile at line 89 uses os.Create(), which respects the process umask and may create world-readable files. Replace with os.OpenFile() and explicit 0o600 mode to guarantee owner-only access.
Proposed fix
- f, err := os.Create(authFilePath)
+ f, err := os.OpenFile(authFilePath, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, 0o600)
if err != nil {
return fmt.Errorf("failed to create token file: %w", err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/auth/copilot/token.go` around lines 29 - 32, The new PII fields
Email and Name are persisted by SaveTokenToFile which currently uses os.Create
(respecting umask) and can produce world-readable files; modify SaveTokenToFile
to open the file with os.OpenFile and explicit owner-only permissions (0o600 /
0600) when creating the token file, ensuring you pass
os.O_WRONLY|os.O_CREATE|os.O_TRUNC and FileMode(0o600) so the token containing
Email/Name is guaranteed owner-only; locate and update the SaveTokenToFile
function in token.go and remove reliance on os.Create so owner-only permissions
are enforced at creation.
| var qwenRateLimiter = struct { | ||
| sync.Mutex | ||
| requests map[string][]time.Time // authID -> request timestamps | ||
| }{ | ||
| requests: make(map[string][]time.Time), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n internal/runtime/executor/qwen_executor.go | head -170Repository: KooshaPari/cliproxyapi-plusplus
Length of output: 7233
🏁 Script executed:
# Check for x/time/rate imports and custom limiter usage
rg -n 'import|golang.org/x/time/rate|qwenRateLimiter|checkQwenRateLimit|rate\.Limiter|rate\.NewLimiter' internal/runtime/executor/qwen_executor.goRepository: KooshaPari/cliproxyapi-plusplus
Length of output: 657
🏁 Script executed:
# Get the exact function checkQwenRateLimit to count lines
rg -A 100 'func checkQwenRateLimit' internal/runtime/executor/qwen_executor.go | head -80Repository: KooshaPari/cliproxyapi-plusplus
Length of output: 3019
Replace the custom rate limiter with golang.org/x/time/rate to follow Go best practices.
This custom sliding-window implementation (lines 49–54, 70–120) violates the established guideline requiring golang.org/x/time/rate for rate limiting in Go. Additionally:
- The function exceeds the 40-line maximum (currently 51 lines)
- Empty
authIDbypasses rate limiting entirely (lines 71–76) - The pruning logic (line 94) does not update the map with filtered timestamps, leaving stale entries resident indefinitely
Refactor to use per-auth rate.Limiter entries with TTL-based cleanup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/runtime/executor/qwen_executor.go` around lines 49 - 54, Replace the
custom sliding-window qwenRateLimiter struct and its associated logic with
per-auth rate.Limiter instances from golang.org/x/time/rate: create a
map[string]*rate.Limiter (e.g., qwenLimiters) protected by a mutex and a
GetLimiter(authID) helper that returns/initializes a limiter with the desired
rate/burst, ensure empty authID is treated as a valid key (do not bypass
limiting), and implement TTL-based cleanup (store lastSeen timestamps alongside
limiters and run a goroutine to remove entries not used for X duration). Update
functions that referenced the old qwenRateLimiter (e.g., any allow/request-check
functions) to call GetLimiter(authID).Allow/Reserve and remove the old
sliding-window timestamp pruning code.
| func checkQwenRateLimit(authID string) error { | ||
| if authID == "" { | ||
| // Empty authID should not bypass rate limiting in production | ||
| // Use debug level to avoid log spam for certain auth flows | ||
| log.Debug("qwen rate limit check: empty authID, skipping rate limit") | ||
| return nil | ||
| } | ||
|
|
||
| now := time.Now() | ||
| windowStart := now.Add(-qwenRateLimitWindow) | ||
|
|
||
| qwenRateLimiter.Lock() | ||
| defer qwenRateLimiter.Unlock() | ||
|
|
||
| // Get and filter timestamps within the window | ||
| timestamps := qwenRateLimiter.requests[authID] | ||
| var validTimestamps []time.Time | ||
| for _, ts := range timestamps { | ||
| if ts.After(windowStart) { | ||
| validTimestamps = append(validTimestamps, ts) | ||
| } | ||
| } | ||
|
|
||
| // Always prune expired entries to prevent memory leak | ||
| // Delete empty entries, otherwise update with pruned slice | ||
| if len(validTimestamps) == 0 { | ||
| delete(qwenRateLimiter.requests, authID) | ||
| } | ||
|
|
||
| // Check if rate limit exceeded | ||
| if len(validTimestamps) >= qwenRateLimitPerMin { | ||
| // Calculate when the oldest request will expire | ||
| oldestInWindow := validTimestamps[0] | ||
| retryAfter := oldestInWindow.Add(qwenRateLimitWindow).Sub(now) | ||
| if retryAfter < time.Second { | ||
| retryAfter = time.Second | ||
| } | ||
| retryAfterSec := int(retryAfter.Seconds()) | ||
| return statusErr{ | ||
| code: http.StatusTooManyRequests, | ||
| msg: fmt.Sprintf(`{"error":{"code":"rate_limit_exceeded","message":"Qwen rate limit: %d requests/minute exceeded, retry after %ds","type":"rate_limit_exceeded"}}`, qwenRateLimitPerMin, retryAfterSec), | ||
| retryAfter: &retryAfter, | ||
| } | ||
| } | ||
|
|
||
| // Record this request and update the map with pruned timestamps | ||
| validTimestamps = append(validTimestamps, now) | ||
| qwenRateLimiter.requests[authID] = validTimestamps | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split checkQwenRateLimit into smaller helpers to satisfy the function-size rule.
This function is 51 lines. Break out pruning, retry-after calculation, and state update into focused helpers.
As per coding guidelines, "Maximum function length: 40 lines".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/runtime/executor/qwen_executor.go` around lines 70 - 120, Split
checkQwenRateLimit into small helpers: implement pruneTimestamps(authID string,
windowStart time.Time) []time.Time that reads qwenRateLimiter.requests[authID],
returns the pruned valid timestamps and deletes the map entry if empty;
implement calculateRetryAfter(oldest time.Time, now time.Time) time.Duration
that computes the retryAfter (with a minimum of 1s) using qwenRateLimitWindow;
and implement recordRequest(authID string, timestamps []time.Time, now
time.Time) to append now and write back to qwenRateLimiter.requests; keep the
lock/unlock inside checkQwenRateLimit, call these helpers in sequence, and
preserve the existing error return using statusErr, qwenRateLimitPerMin and
other semantics exactly as before.
| if authID == "" { | ||
| // Empty authID should not bypass rate limiting in production | ||
| // Use debug level to avoid log spam for certain auth flows | ||
| log.Debug("qwen rate limit check: empty authID, skipping rate limit") | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Empty auth IDs currently bypass throttling.
Returning early for empty authID creates a bypass path; malformed or missing IDs can evade the limiter entirely. Use a fallback bucket key (or token-derived stable key) instead of skipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/runtime/executor/qwen_executor.go` around lines 71 - 76, The current
early return when authID == "" in the rate-limit check (see the authID
conditional and log.Debug call) creates a bypass; instead, remove the return and
derive a stable fallback bucket key for rate limiting (e.g., an "anon" bucket or
a token-derived hash) so requests with empty or malformed authID still hit the
limiter; update the rate-limit call to use that fallback key and keep the debug
log but do not skip enforcement.
| func TestContextManagementCompactionCompatibility(t *testing.T) { | ||
| inputJSON := []byte(`{ | ||
| "model": "gpt-5.2", | ||
| "context_management": [ | ||
| { | ||
| "type": "compaction", | ||
| "compact_threshold": 12000 | ||
| } | ||
| ], | ||
| "input": [{"role":"user","content":"hello"}] | ||
| }`) | ||
|
|
||
| output := ConvertOpenAIResponsesRequestToCodex("gpt-5.2", inputJSON, false) | ||
| outputStr := string(output) | ||
|
|
||
| if gjson.Get(outputStr, "context_management").Exists() { | ||
| t.Fatalf("context_management should be removed for Codex compatibility") | ||
| } | ||
| if gjson.Get(outputStr, "truncation").Exists() { | ||
| t.Fatalf("truncation should be removed for Codex compatibility") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider narrowing the test scope to match the function name.
TestContextManagementCompactionCompatibility tests both context_management and truncation removal, but the name suggests it focuses only on context management compaction. Since TestTruncationRemovedForCodexCompatibility already covers truncation, the assertion at lines 302-304 is redundant and could be removed for clarity.
💡 Suggested simplification
func TestContextManagementCompactionCompatibility(t *testing.T) {
inputJSON := []byte(`{
"model": "gpt-5.2",
"context_management": [
{
"type": "compaction",
"compact_threshold": 12000
}
],
"input": [{"role":"user","content":"hello"}]
}`)
output := ConvertOpenAIResponsesRequestToCodex("gpt-5.2", inputJSON, false)
outputStr := string(output)
if gjson.Get(outputStr, "context_management").Exists() {
t.Fatalf("context_management should be removed for Codex compatibility")
}
- if gjson.Get(outputStr, "truncation").Exists() {
- t.Fatalf("truncation should be removed for Codex compatibility")
- }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestContextManagementCompactionCompatibility(t *testing.T) { | |
| inputJSON := []byte(`{ | |
| "model": "gpt-5.2", | |
| "context_management": [ | |
| { | |
| "type": "compaction", | |
| "compact_threshold": 12000 | |
| } | |
| ], | |
| "input": [{"role":"user","content":"hello"}] | |
| }`) | |
| output := ConvertOpenAIResponsesRequestToCodex("gpt-5.2", inputJSON, false) | |
| outputStr := string(output) | |
| if gjson.Get(outputStr, "context_management").Exists() { | |
| t.Fatalf("context_management should be removed for Codex compatibility") | |
| } | |
| if gjson.Get(outputStr, "truncation").Exists() { | |
| t.Fatalf("truncation should be removed for Codex compatibility") | |
| } | |
| } | |
| func TestContextManagementCompactionCompatibility(t *testing.T) { | |
| inputJSON := []byte(`{ | |
| "model": "gpt-5.2", | |
| "context_management": [ | |
| { | |
| "type": "compaction", | |
| "compact_threshold": 12000 | |
| } | |
| ], | |
| "input": [{"role":"user","content":"hello"}] | |
| }`) | |
| output := ConvertOpenAIResponsesRequestToCodex("gpt-5.2", inputJSON, false) | |
| outputStr := string(output) | |
| if gjson.Get(outputStr, "context_management").Exists() { | |
| t.Fatalf("context_management should be removed for Codex compatibility") | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@internal/translator/codex/openai/responses/codex_openai-responses_request_test.go`
around lines 284 - 305, The test TestContextManagementCompactionCompatibility
currently asserts removal of both "context_management" and "truncation"; narrow
its scope by removing the redundant assertions that check for "truncation" (the
gjson.Get(..., "truncation").Exists() check and its t.Fatalf), since truncation
removal is already covered by TestTruncationRemovedForCodexCompatibility; keep
the existing call to ConvertOpenAIResponsesRequestToCodex("gpt-5.2", inputJSON,
false) and the assertion that "context_management" is removed.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
KooshaPari
deleted the
feat/clean-base-1698-strip-empty-messages-rebase
branch
6 participants
Automated PR for branch feat/clean-base-1698-strip-empty-messages-rebase as requested.
Summary by CodeRabbit
New Features
Bug Fixes
Tests