-
-
Notifications
You must be signed in to change notification settings - Fork 296
fix(gemini): use thinkingLevel instead of thinkingBudget for Gemini 3… #359
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
… models - Add IsGemini3Model helper function to detect Gemini 3 models - Use thinkingLevel (low/high) for Gemini 3 instead of thinkingBudget - Gemini 3 medium/default uses dynamic thinking (no explicit level) - Update antigravity, gemini-cli, and gemini responses translators 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Summary of ChangesHello @nestharus, 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 updates the system's handling of Gemini 3 model configurations, specifically regarding how 'thinking' parameters are applied. It introduces a new mechanism that leverages Highlights
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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request correctly adapts the translators to use thinkingLevel for Gemini 3 models instead of thinkingBudget, introducing a new IsGemini3Model helper function. The changes are consistent across the antigravity, gemini-cli, and gemini-responses translators. My review focuses on improving code maintainability by addressing significant code duplication between translators and the use of magic strings. I've provided suggestions to refactor duplicated logic into a shared utility and to define constants for string literals to make the code more robust and easier to manage in the future.
| case "low": | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 1024)) | ||
| if util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingLevel", "low") | ||
| } else { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 1024)) | ||
| } | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.include_thoughts", true) | ||
| case "medium": | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 8192)) | ||
| if !util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 8192)) | ||
| } | ||
| // Gemini 3: no thinkingLevel for medium, uses dynamic thinking (auto) | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.include_thoughts", true) | ||
| case "high": | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 32768)) | ||
| if util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingLevel", "high") | ||
| } else { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 32768)) | ||
| } | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.include_thoughts", true) | ||
| default: | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", -1) | ||
| if !util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", -1) | ||
| } | ||
| // Gemini 3: no thinkingLevel for auto/default, uses dynamic thinking | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.include_thoughts", true) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic within these case statements is identical to the logic in internal/translator/gemini-cli/openai/chat-completions/gemini-cli_openai_request.go. Additionally, the logic for setting the default thinkingConfig for Gemini 3 models on lines 108-110 is also duplicated. This creates a maintenance burden, as any future changes will need to be applied in both places.
Consider extracting this duplicated logic into a shared helper function in a common package like internal/util to improve maintainability.
| if util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingLevel", "low") | ||
| } else { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 1024)) | ||
| } | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.include_thoughts", true) | ||
| case "medium": | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 8192)) | ||
| if !util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 8192)) | ||
| } | ||
| // Gemini 3: no thinkingLevel for medium, uses dynamic thinking (auto) | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.include_thoughts", true) | ||
| case "high": | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 32768)) | ||
| if util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingLevel", "high") | ||
| } else { | ||
| out, _ = sjson.SetBytes(out, "request.generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 32768)) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string literals "low" (line 52) and "high" (line 65) for thinkingLevel are magic strings. It's good practice to define these as constants in a shared package (e.g., util) to ensure consistency and prevent typos. This issue is also present in the other translator files modified in this PR.
Example:
// In a relevant package (e.g., util)
const (
ThinkingLevelLow = "low"
ThinkingLevelHigh = "high"
)You could then use util.ThinkingLevelLow and util.ThinkingLevelHigh.
| case "minimal": | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 1024)) | ||
| if util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingLevel", "low") | ||
| } else { | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 1024)) | ||
| } | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.include_thoughts", true) | ||
| case "low": | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 4096)) | ||
| if util.IsGemini3Model(modelName) { | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingLevel", "low") | ||
| } else { | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, 4096)) | ||
| } | ||
| out, _ = sjson.Set(out, "generationConfig.thinkingConfig.include_thoughts", true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case "minimal" and case "low" blocks are very similar. For Gemini 3 models, they both set thinkingLevel to "low", and they both set include_thoughts to true. You can combine these cases to reduce code duplication and make the logic more concise.
case "minimal", "low":
if util.IsGemini3Model(modelName) {
out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingLevel", "low")
} else {
budget := 1024
if reasoningEffort.String() == "low" {
budget = 4096
}
out, _ = sjson.Set(out, "generationConfig.thinkingConfig.thinkingBudget", util.NormalizeThinkingBudget(modelName, budget))
}
out, _ = sjson.Set(out, "generationConfig.thinkingConfig.include_thoughts", true)| // IsGemini3Model returns true if the model is a Gemini 3 model (uses thinkingLevel instead of thinkingBudget). | ||
| func IsGemini3Model(model string) bool { | ||
| lower := strings.ToLower(model) | ||
| return strings.HasPrefix(lower, "gemini-3-") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string literal "gemini-3-" is a magic string. It's better to define it as a constant at the package level to improve readability and maintainability, and to avoid potential typos if it's used elsewhere.
Example:
const Gemini3ModelPrefix = "gemini-3-"| return strings.HasPrefix(lower, "gemini-3-") | |
| return strings.HasPrefix(lower, Gemini3ModelPrefix) |
#353