Conversation
refactor(translator): improve tools handling by separating functionDeclarations and googleSearch nodes
Summary of ChangesHello @nhatbien, 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 management of authentication files by introducing a dedicated validation mechanism. It provides both an explicit API endpoint and an optional query parameter for listing auth files to check token validity. This ensures that authentication statuses are accurately reflected and allows for proactive identification of expired or invalid credentials. Concurrently, it refines the internal handling of tool definitions in API request translations and improves the fairness of authentication provider selection. 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. 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 for validating authentication files, both through an option in the existing ListAuthFiles endpoint and a new, more powerful ValidateAuthFiles endpoint. The implementation includes structured error handling with tokenRefreshError, concurrent validation processing, and logic to update authentication status based on the results. Additionally, it contains important fixes for correctly translating multiple tools in OpenAI-compatible requests and ensuring fair, deterministic round-robin selection of authentication candidates.
Overall, the changes are well-implemented. My review focuses on a critical issue regarding ignored errors during state persistence, which could lead to inconsistencies, and a couple of medium-severity suggestions to improve performance and reduce code duplication.
| auth.Status = coreauth.StatusActive | ||
| auth.StatusMessage = "" | ||
| } | ||
| _, _ = h.authManager.Update(ctx, auth) |
There was a problem hiding this comment.
The error returned by h.authManager.Update(ctx, auth) is being ignored. If updating the auth record fails (e.g., due to file permission issues), the in-memory state of the auth object will be inconsistent with the persisted state. This could lead to unexpected behavior, as subsequent operations might use stale data. This error should be handled, at least by logging it.
if _, errUpdate := h.authManager.Update(ctx, auth); errUpdate != nil {
log.Warnf("Failed to update auth %s after successful login check: %v", auth.ID, errUpdate)
}| } | ||
| } | ||
|
|
||
| _, _ = h.authManager.Update(ctx, auth) |
There was a problem hiding this comment.
The error returned by h.authManager.Update(ctx, auth) is being ignored. If updating the auth record fails (e.g., due to file permission issues), the in-memory state of the auth object will be inconsistent with the persisted state. This could lead to unexpected behavior, as subsequent operations might use stale data. This error should be handled, at least by logging it.
if _, errUpdate := h.authManager.Update(ctx, auth); errUpdate != nil {
log.Warnf("Failed to update auth %s after failed login check: %v", auth.ID, errUpdate)
}| auths := h.authManager.List() | ||
|
|
||
| validateRaw := strings.ToLower(strings.TrimSpace(c.Query("validate"))) | ||
| if validateRaw != "" && validateRaw != "0" && validateRaw != "false" && validateRaw != "no" && validateRaw != "off" { |
There was a problem hiding this comment.
This condition to check for a 'truthy' query parameter is complex and similar logic is used for the force and all parameters in ValidateAuthFiles. To improve readability and maintainability, consider extracting this into a helper function.
For example:
func isTruthy(s string) bool {
s := strings.ToLower(strings.TrimSpace(s))
return s != "" && s != "0" && s != "false" && s != "no" && s != "off"
}This would simplify the condition to if isTruthy(validateRaw).
| for _, auth := range auths { | ||
| if auth == nil || auth.Disabled || auth.Status == coreauth.StatusDisabled { | ||
| continue | ||
| } | ||
| provider := strings.ToLower(strings.TrimSpace(auth.Provider)) | ||
| if provider != "antigravity" && provider != "gemini-cli" { | ||
| continue | ||
| } | ||
| checkCtx, cancel := context.WithTimeout(c.Request.Context(), 15*time.Second) | ||
| _, errToken := h.resolveTokenForAuth(checkCtx, auth, forceRefresh) | ||
| cancel() | ||
| h.applyLoginCheckResult(c.Request.Context(), auth, errToken == nil, errToken, statusCodeForRefreshError(errToken), now) | ||
| } |
There was a problem hiding this comment.
The validation of auth files in this loop is performed sequentially. If there are many authentications to validate, this could lead to a slow API response. The new /v0/management/auth-files/validate endpoint implements concurrent validation using goroutines and a semaphore, which is a more performant approach. Consider applying a similar concurrent pattern here to improve the performance of ListAuthFiles when validation is requested.
2 participants
No description provided.