feat(auth): add CodeBuddy-CN browser OAuth authentication support#450
feat(auth): add CodeBuddy-CN browser OAuth authentication support#450lwiles692 wants to merge 3 commits intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, 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 introduces full support for CodeBuddy-CN, enabling users to authenticate via a browser-based OAuth flow. It integrates CodeBuddy's AI models and API interactions into the system, providing a seamless experience for accessing CodeBuddy services. The changes encompass new authentication logic, API executors, model definitions, and improved error feedback, expanding the platform's capabilities to include Tencent's CodeBuddy offerings. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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. 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
The pull request introduces support for CodeBuddy-CN browser OAuth authentication. The changes are well-structured, adding new files for authentication logic, error handling, token storage, and integrating them into the existing command, registry, and executor systems. The new code includes appropriate error handling and defensive programming practices. However, there are several instances of hardcoded values for URLs, domains, user agents, and API paths that could be made configurable or derived from application build information for better maintainability. Additionally, some error wrapping uses %v instead of %w, and a few errors are ignored, which could mask underlying issues. A Chinese comment was also found and should be translated for consistency.
| ) | ||
|
|
||
| const ( | ||
| BaseURL = "https://copilot.tencent.com" |
There was a problem hiding this comment.
This BaseURL is hardcoded. It would be more flexible and maintainable to make this configurable, especially if there are different environments (e.g., staging, production) or regional endpoints for CodeBuddy.
|
|
||
| const ( | ||
| BaseURL = "https://copilot.tencent.com" | ||
| DefaultDomain = "www.codebuddy.cn" |
There was a problem hiding this comment.
Similar to BaseURL, DefaultDomain is hardcoded. Consider making this configurable to support different CodeBuddy domains if needed in the future.
| const ( | ||
| BaseURL = "https://copilot.tencent.com" | ||
| DefaultDomain = "www.codebuddy.cn" | ||
| UserAgent = "CLI/2.63.2 CodeBuddy/2.63.2" |
There was a problem hiding this comment.
The UserAgent string is hardcoded with a specific version. If the CLI version changes, this constant will need manual updates. It would be more robust to derive this from the application's build information or a central version constant.
| codeBuddyStatePath = "/v2/plugin/auth/state" | ||
| codeBuddyTokenPath = "/v2/plugin/auth/token" | ||
| codeBuddyRefreshPath = "/v2/plugin/auth/token/refresh" | ||
| pollInterval = 5 * time.Second |
There was a problem hiding this comment.
The pollInterval is a hardcoded duration. Making this configurable would allow for fine-tuning the polling behavior without code changes, which can be useful for different network conditions or API rate limits.
| codeBuddyTokenPath = "/v2/plugin/auth/token" | ||
| codeBuddyRefreshPath = "/v2/plugin/auth/token/refresh" | ||
| pollInterval = 5 * time.Second | ||
| maxPollDuration = 5 * time.Minute |
There was a problem hiding this comment.
The maxPollDuration is hardcoded. This duration might need adjustment based on user experience expectations or server-side timeouts. Consider making it configurable.
| return nil, fmt.Errorf("codebuddy: empty data in refresh response") | ||
| } | ||
|
|
||
| newUserID, _ := a.DecodeUserID(result.Data.AccessToken) |
There was a problem hiding this comment.
The error returned by a.DecodeUserID is ignored here. Consider logging this error or handling it more explicitly, as a failure to decode the user ID from the access token could indicate a malformed token or an unexpected API response.
newUserID, err := a.DecodeUserID(result.Data.AccessToken)
if err != nil {
log.Debugf("codebuddy refresh: failed to decode user ID from access token: %v", err)
}|
|
||
| const ( | ||
| codeBuddyChatPath = "/v2/chat/completions" | ||
| codeBuddyAuthType = "codebuddy" |
There was a problem hiding this comment.
The codeBuddyChatPath is a hardcoded API path. If the API version or path changes, this constant will need to be updated. Consider making this configurable if there's a possibility of variation.
| }() | ||
|
|
||
| recordAPIResponseMetadata(ctx, e.cfg, httpResp.StatusCode, httpResp.Header.Clone()) | ||
| if !isHTTPSuccess(httpResp.StatusCode) { |
There was a problem hiding this comment.
The error returned by io.ReadAll(httpResp.Body) is ignored here. While the subsequent if err != nil block handles a later io.ReadAll error, it's best practice to check the error from this specific io.ReadAll call explicitly to ensure all potential errors are caught.
b, errRead := io.ReadAll(httpResp.Body)
if errRead != nil {
log.Debugf("codebuddy executor: failed to read error response body: %v", errRead)
err = fmt.Errorf("codebuddy executor: failed to read error response body: %w", errRead)
return resp, err
}| }() | ||
|
|
||
| scanner := bufio.NewScanner(httpResp.Body) | ||
| scanner.Buffer(nil, maxScannerBufferSize) |
There was a problem hiding this comment.
maxScannerBufferSize is used here but is not defined within this file or explicitly imported from a package. Please ensure it's correctly defined or imported to avoid potential compilation or runtime issues.
| req.Header.Set("Authorization", "Bearer "+accessToken) | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Accept", "application/json") | ||
| req.Header.Set("User-Agent", codebuddy.UserAgent) | ||
| req.Header.Set("X-User-Id", userID) | ||
| req.Header.Set("X-Domain", domain) | ||
| req.Header.Set("X-Product", "SaaS") | ||
| req.Header.Set("X-IDE-Type", "CLI") | ||
| req.Header.Set("X-IDE-Name", "CLI") | ||
| req.Header.Set("X-IDE-Version", "2.63.2") | ||
| req.Header.Set("X-Requested-With", "XMLHttpRequest") |
There was a problem hiding this comment.
Several headers like X-IDE-Type, X-IDE-Name, X-IDE-Version, and X-Product are hardcoded. Similar to the UserAgent in codebuddy_auth.go, these values could ideally be derived from application build information or configuration to improve maintainability and ensure they are always up-to-date with the CLI's version.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
No description provided.