feat(gemini-cli): add Google One login and improve auto-discovery #1543
Conversation
Add Google One personal account login to Gemini CLI OAuth flow: - CLI --login shows mode menu (Code Assist vs Google One) - Web management API accepts project_id=GOOGLE_ONE sentinel - Auto-discover project via onboardUser without cloudaicompanionProject when project is unresolved Improve robustness of auto-discovery and token handling: - Add context-aware auto-discovery polling (30s timeout, 2s interval) - Distinguish network errors from project-selection-required errors - Refresh expired access tokens in readAuthFile before project lookup - Extend project_id auto-fill to gemini auth type (was antigravity-only) Unify credential file naming to geminicli- prefix for both CLI and web. Add extractAccessToken unit tests (9 cases).
Summary of ChangesHello @sususu98, 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 significantly enhances the Gemini CLI's authentication experience by integrating Google One personal account login, allowing users without existing GCP projects to seamlessly onboard. It also refines the auto-discovery process for projects, making it more resilient and user-friendly through context-aware polling and improved error handling. These changes broaden accessibility and improve the reliability of the authentication flow for all users. 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. 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 a valuable feature by adding Google One login and auto-discovery for Gemini projects, enhancing the user experience for those without pre-existing GCP projects. The implementation correctly adds this new flow to both the CLI and the web management API. However, there are several areas for improvement. The most significant issue is the large-scale code duplication of the auto-discovery logic between the CLI and API handlers, which poses a maintenance risk. Additionally, the new token refresh functionality could be made more robust by improving error handling and using safer type assertions. Addressing these points will improve the code's maintainability and reliability.
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| body, _ := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
The error returned by io.ReadAll is being ignored. If reading the response body fails, body could be incomplete or nil, which could lead to a confusing JSON unmarshalling error later. The error should be checked and handled.
body, errRead := io.ReadAll(resp.Body)
if errRead != nil {
return "", fmt.Errorf("failed to read refresh response body: %w", errRead)
}| if strings.TrimSpace(ts.ProjectID) == "" { | ||
| log.Error("Google One auto-discovery returned empty project ID") | ||
| SetOAuthSessionError(state, "Google One auto-discovery returned empty project ID") | ||
| return | ||
| } | ||
| isChecked, errCheck := checkCloudAPIIsEnabled(ctx, gemClient, ts.ProjectID) | ||
| if errCheck != nil { | ||
| log.Errorf("Failed to verify Cloud AI API status: %v", errCheck) | ||
| SetOAuthSessionError(state, "Failed to verify Cloud AI API status") | ||
| return | ||
| } | ||
| ts.Checked = isChecked | ||
| if !isChecked { | ||
| log.Error("Cloud AI API is not enabled for the auto-discovered project") | ||
| SetOAuthSessionError(state, "Cloud AI API not enabled") | ||
| return | ||
| } |
There was a problem hiding this comment.
This block of code for checking the project ID and ensuring the Cloud AI API is enabled is nearly identical to the logic in the else block that follows (lines 1222-1239 in the full file). Duplicating this logic increases maintenance overhead. Consider extracting this shared logic into a helper function or restructuring the if-else chain to perform this check once for both the GOOGLE_ONE and single-project cases to avoid repetition.
| autoCtx, autoCancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer autoCancel() | ||
| for attempt := 1; ; attempt++ { | ||
| var onboardResp map[string]any | ||
| if errOnboard := callGeminiCLI(autoCtx, httpClient, "onboardUser", autoOnboardReq, &onboardResp); errOnboard != nil { | ||
| return fmt.Errorf("auto-discovery onboardUser: %w", errOnboard) | ||
| } | ||
|
|
||
| if done, okDone := onboardResp["done"].(bool); okDone && done { | ||
| if resp, okResp := onboardResp["response"].(map[string]any); okResp { | ||
| switch v := resp["cloudaicompanionProject"].(type) { | ||
| case string: | ||
| projectID = strings.TrimSpace(v) | ||
| case map[string]any: | ||
| if id, okID := v["id"].(string); okID { | ||
| projectID = strings.TrimSpace(id) | ||
| } | ||
| } | ||
| } | ||
| break | ||
| } | ||
|
|
||
| log.Debugf("Auto-discovery: onboarding in progress, attempt %d...", attempt) | ||
| select { | ||
| case <-autoCtx.Done(): | ||
| return &projectSelectionRequiredError{} | ||
| case <-time.After(2 * time.Second): | ||
| } |
There was a problem hiding this comment.
The timeout duration (30 seconds) and polling interval (2 seconds) are hardcoded. It's a good practice to define these as constants at the top of the file or package. This improves readability and makes it easier to change these values in the future without searching through the code.
For example:
const (
autoDiscoveryTimeout = 30 * time.Second
autoDiscoveryInterval = 2 * time.Second
)| autoCtx, autoCancel := context.WithTimeout(ctx, 30*time.Second) | ||
| defer autoCancel() | ||
| for attempt := 1; ; attempt++ { | ||
| var onboardResp map[string]any | ||
| if errOnboard := callGeminiCLI(autoCtx, httpClient, "onboardUser", autoOnboardReq, &onboardResp); errOnboard != nil { | ||
| return fmt.Errorf("auto-discovery onboardUser: %w", errOnboard) | ||
| } | ||
|
|
||
| if done, okDone := onboardResp["done"].(bool); okDone && done { | ||
| if resp, okResp := onboardResp["response"].(map[string]any); okResp { | ||
| switch v := resp["cloudaicompanionProject"].(type) { | ||
| case string: | ||
| projectID = strings.TrimSpace(v) | ||
| case map[string]any: | ||
| if id, okID := v["id"].(string); okID { | ||
| projectID = strings.TrimSpace(id) | ||
| } | ||
| } | ||
| } | ||
| break | ||
| } | ||
|
|
||
| log.Debugf("Auto-discovery: onboarding in progress, attempt %d...", attempt) | ||
| select { | ||
| case <-autoCtx.Done(): | ||
| return &projectSelectionRequiredError{} | ||
| case <-time.After(2 * time.Second): | ||
| } |
There was a problem hiding this comment.
The timeout duration (30 seconds) and polling interval (2 seconds) are hardcoded. It's a good practice to define these as constants at the top of the file or package. This improves readability and makes it easier to change these values in the future without searching through the code.
For example:
const (
autoDiscoveryTimeout = 30 * time.Second
autoDiscoveryInterval = 2 * time.Second
)This logic is also duplicated in internal/api/handlers/management/auth_files.go, which reinforces the need for a shared implementation.
| if refreshed, errRefresh := refreshGeminiAccessToken(tokenMap, http.DefaultClient); errRefresh == nil { | ||
| accessToken = refreshed | ||
| } |
There was a problem hiding this comment.
The error errRefresh from refreshGeminiAccessToken is silently ignored if the refresh fails. While the code correctly falls back to the existing access token, logging this error would be valuable for debugging purposes, as it would provide insight into why a token refresh might be failing (e.g., invalid refresh token, network issues, permission changes).
Consider adding a log statement in an else block to record the failure.
if refreshed, errRefresh := refreshGeminiAccessToken(tokenMap, http.DefaultClient); errRefresh == nil {
accessToken = refreshed
} else {
// log.Warnf("Failed to refresh gemini access token: %v", errRefresh) // Consider adding a logger if available in this package.
}| refreshToken, _ := tokenMap["refresh_token"].(string) | ||
| clientID, _ := tokenMap["client_id"].(string) | ||
| clientSecret, _ := tokenMap["client_secret"].(string) | ||
| tokenURI, _ := tokenMap["token_uri"].(string) | ||
|
|
||
| if refreshToken == "" || clientID == "" || clientSecret == "" { | ||
| return "", fmt.Errorf("missing refresh credentials") |
There was a problem hiding this comment.
The type assertions for refresh_token, client_id, and client_secret ignore the ok boolean. If any of these fields exist but are not strings, the assertion will fail, the variable will be an empty string, and the function will return a generic "missing refresh credentials" error. It would be more robust to check the ok value from the type assertion to provide a more specific error if the type is incorrect.
refreshToken, ok1 := tokenMap["refresh_token"].(string)
clientSecret, ok2 := tokenMap["client_secret"].(string)
clientID, ok3 := tokenMap["client_id"].(string)
tokenURI, _ := tokenMap["token_uri"].(string)
if !ok1 || !ok2 || !ok3 || refreshToken == "" || clientID == "" || clientSecret == "" {
return "", fmt.Errorf("missing or invalid refresh credentials")
}| newAccessToken, _ := result["access_token"].(string) | ||
| if newAccessToken == "" { | ||
| return "", fmt.Errorf("no access_token in refresh response") |
There was a problem hiding this comment.
The type assertion for access_token ignores the ok boolean. If the access_token field is present but not a string, the assertion will fail, and the function will incorrectly report that the token is missing. Checking the ok value provides better error handling and makes the code more robust.
newAccessToken, ok := result["access_token"].(string)
if !ok || newAccessToken == "" {
return "", fmt.Errorf("no access_token in refresh response")
}1 participant
Summary
onboardUser--loginshows a mode menu (Code Assist vs Google One); Web management API acceptsproject_id=GOOGLE_ONEsentinel valuereadAuthFilebefore project lookup; extend project_id auto-fill to gemini auth typegeminicli-prefix for both CLI and webextractAccessTokenunit tests (9 cases)Changes
internal/cmd/login.goperformGeminiCLISetupinternal/api/handlers/management/auth_files.goGOOGLE_ONEbranch inRequestGeminiCLIToken, same auto-discovery logicsdk/auth/filestore.goextractAccessTokenhelper, project_id auto-fill extended to geminiinternal/auth/gemini/gemini_token.gogemini-→geminicli-sdk/auth/filestore_test.goextractAccessTokenHow It Works
When
project_idis empty afterloadCodeAssist, the code callsonboardUserwithoutcloudaicompanionProjectto let Google auto-provision a project. This matches the Gemini CLI headless behavior and Antigravity'sFetchProjectIDpattern.CLI
Web Management API
Send
project_id=GOOGLE_ONE(case-insensitive) as a query parameter toGET /gemini-cli-auth-url.Backward Compatibility
typefield, not filenameproject_id=""andproject_id=ALLflows are unchangedGOOGLE_ONEis a new sentinel that was never sent before, so zero breaking changes