Conversation
feat(proxy): centralize proxy handling with `proxyutil` package and enhance test coverage - Added `proxyutil` package to simplify proxy handling across the codebase. - Refactored various components (`executor`, `cliproxy`, `auth`, etc.) to use `proxyutil` for consistent and reusable proxy logic. - Introduced support for "direct" proxy mode to explicitly bypass all proxies. - Updated tests to validate proxy behavior (e.g., `direct`, HTTP/HTTPS, and SOCKS5). - Enhanced YAML configuration documentation for proxy options.
…cy and readability
feat(config/codex): Add Codex header defaults (`user-agent`: override; `beta-features`: default)
when Amp or Claude Code sends functionResponse with an empty name in Gemini conversation history, the Gemini API rejects the request with 400 "Name cannot be empty". this fix backfills empty names from the corresponding preceding functionCall parts using positional matching. covers all three Gemini translator paths: - gemini/gemini (direct API key) - antigravity/gemini (OAuth) - gemini-cli/gemini (Gemini CLI) also switches fixCLIToolResponse pending group matching from LIFO to FIFO to correctly handle multiple sequential tool call groups. fixes #1903
…sponse-names fix: backfill empty functionResponse.name from preceding functionCall
Refactor Antigravity model handling and improve logging
feat(model_registry): enhance model registration and refresh mechanisms
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 significantly refactors and enhances the application's proxy handling, model management, and API translation capabilities. It centralizes proxy configuration into a new utility package, enabling more flexible and explicit proxy bypass options. The model registry now supports dynamic, periodic updates for Antigravity models and provides a callback for reacting to these changes. Additionally, it introduces configurable default headers for Codex OAuth requests and improves Gemini function call translation for better API compatibility. 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
Ignored Files
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 significant refactoring of proxy handling by centralizing it into a new sdk/proxyutil package, which is a great improvement for maintainability. It also adds periodic remote model catalog refreshing, making the service more dynamic. Additionally, there are several good fixes for function call handling in translators and a new fetch_antigravity_models tool. My review focuses on improving the new tool's implementation and suggesting a refactoring for some duplicated code in the new header handling logic.
| fmt.Fprintf(os.Stderr, "error: cannot get working directory: %v\n", err) | ||
| os.Exit(1) |
There was a problem hiding this comment.
For consistency with the logging setup in this file, please use the log package for handling errors and warnings instead of writing directly to os.Stderr.
- For fatal errors like this one,
log.Fatalf("error: cannot get working directory: %v", err)can be used. It will print the message and exit the program, simplifying the code. This applies to lines 98-99, 102-103, 118-119, 150-151, and 155-156 as well. - For warnings (line 132), use
log.Warnf(...). - For non-fatal errors (line 165), use
log.Errorf(...).
| fmt.Fprintf(os.Stderr, "error: cannot get working directory: %v\n", err) | |
| os.Exit(1) | |
| log.Fatalf("error: cannot get working directory: %v", err) |
| payload = []byte(`{}`) | ||
| } | ||
|
|
||
| httpReq, errReq := http.NewRequestWithContext(ctx, http.MethodPost, modelsURL, strings.NewReader(string(payload))) |
There was a problem hiding this comment.
Converting the byte slice payload to a string and then using strings.NewReader creates an unnecessary copy. You can use bytes.NewReader(payload) directly to avoid this allocation and improve efficiency. You will need to add "bytes" to your imports.
| httpReq, errReq := http.NewRequestWithContext(ctx, http.MethodPost, modelsURL, strings.NewReader(string(payload))) | |
| httpReq, errReq := http.NewRequestWithContext(ctx, http.MethodPost, modelsURL, bytes.NewReader(payload)) |
| httpReq.Header.Set("Authorization", "Bearer "+accessToken) | ||
| httpReq.Header.Set("User-Agent", "antigravity/1.19.6 darwin/arm64") | ||
|
|
||
| httpClient := &http.Client{Timeout: 30 * time.Second} |
There was a problem hiding this comment.
The context passed to http.NewRequestWithContext on line 184 already has a 30-second timeout. Setting a separate Timeout on the http.Client is redundant and can lead to unclear behavior regarding which timeout takes precedence. It's generally recommended to rely on the context for request cancellation and timeouts.
| httpClient := &http.Client{Timeout: 30 * time.Second} | |
| httpClient := &http.Client{} |
| func ensureHeaderWithPriority(target http.Header, source http.Header, key, configValue, fallbackValue string) { | ||
| if target == nil { | ||
| return | ||
| } | ||
| if strings.TrimSpace(target.Get(key)) != "" { | ||
| return | ||
| } | ||
| if source != nil { | ||
| if val := strings.TrimSpace(source.Get(key)); val != "" { | ||
| target.Set(key, val) | ||
| return | ||
| } | ||
| } | ||
| if val := strings.TrimSpace(configValue); val != "" { | ||
| target.Set(key, val) | ||
| return | ||
| } | ||
| if val := strings.TrimSpace(fallbackValue); val != "" { | ||
| target.Set(key, val) | ||
| } | ||
| } | ||
|
|
||
| func ensureHeaderWithConfigPrecedence(target http.Header, source http.Header, key, configValue, fallbackValue string) { | ||
| if target == nil { | ||
| return | ||
| } | ||
| if strings.TrimSpace(target.Get(key)) != "" { | ||
| return | ||
| } | ||
| if val := strings.TrimSpace(configValue); val != "" { | ||
| target.Set(key, val) | ||
| return | ||
| } | ||
| if source != nil { | ||
| if val := strings.TrimSpace(source.Get(key)); val != "" { | ||
| target.Set(key, val) | ||
| return | ||
| } | ||
| } | ||
| if val := strings.TrimSpace(fallbackValue); val != "" { | ||
| target.Set(key, val) | ||
| } | ||
| } |
There was a problem hiding this comment.
The functions ensureHeaderWithPriority and ensureHeaderWithConfigPrecedence are very similar, with the only difference being the order of precedence between configValue and source. This duplication could be avoided by creating a single, more generic helper function that accepts an ordered list of value sources. This would make the code more maintainable and less prone to copy-paste errors.
4 participants
No description provided.