Skip to content

fix(gemini): abort default injection on existing thinking keys #862

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.

Sign up for GitHub

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

Merged
luispater merged 1 commit into from
Jan 5, 2026
Merged

fix(gemini): abort default injection on existing thinking keys #862

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 28 additions & 22 deletions internal/util/gemini_thinking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,13 @@ func ApplyGeminiThinkingConfig(body []byte, budget *int, includeThoughts *bool)
incl = &defaultInclude
}
if incl != nil {
valuePath := "generationConfig.thinkingConfig.include_thoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
if !gjson.GetBytes(updated, "generationConfig.thinkingConfig.includeThoughts").Exists() &&
!gjson.GetBytes(updated, "generationConfig.thinkingConfig.include_thoughts").Exists() {
valuePath := "generationConfig.thinkingConfig.include_thoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
}
}
Comment on lines +74 to 81
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This change correctly prevents overwriting an existing includeThoughts key. However, it misses an opportunity for normalization. This function is for models that expect include_thoughts (snake_case). If a request contains includeThoughts (camelCase), this logic allows it to pass through unmodified. The API might ignore this setting if it strictly expects snake_case. It would be more robust to normalize the key to include_thoughts.

Comment on lines +74 to 81
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic to conditionally set includeThoughts is duplicated across four functions in this file (ApplyGeminiThinkingConfig, ApplyGeminiCLIThinkingConfig, ApplyGeminiThinkingLevel, ApplyGeminiCLIThinkingLevel). To improve maintainability and reduce redundancy, please consider extracting this into a single helper function.

}
return updated
Expand All @@ -99,10 +102,13 @@ func ApplyGeminiCLIThinkingConfig(body []byte, budget *int, includeThoughts *boo
incl = &defaultInclude
}
if incl != nil {
valuePath := "request.generationConfig.thinkingConfig.include_thoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
if !gjson.GetBytes(updated, "request.generationConfig.thinkingConfig.includeThoughts").Exists() &&
!gjson.GetBytes(updated, "request.generationConfig.thinkingConfig.include_thoughts").Exists() {
valuePath := "request.generationConfig.thinkingConfig.include_thoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
}
}
Comment on lines +105 to 112
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to ApplyGeminiThinkingConfig, this logic does not normalize includeThoughts (camelCase) to the expected include_thoughts (snake_case) for this model family. This could lead to the setting being ignored by the API. Please consider adding normalization logic.

}
return updated
Expand Down Expand Up @@ -130,15 +136,15 @@ func ApplyGeminiThinkingLevel(body []byte, level string, includeThoughts *bool)
incl = &defaultInclude
}
if incl != nil {
valuePath := "generationConfig.thinkingConfig.includeThoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
if !gjson.GetBytes(updated, "generationConfig.thinkingConfig.includeThoughts").Exists() &&
!gjson.GetBytes(updated, "generationConfig.thinkingConfig.include_thoughts").Exists() {
valuePath := "generationConfig.thinkingConfig.includeThoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
}
}
Comment on lines +139 to 146
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

While this change correctly avoids overwriting an existing key, it removes the previous (though buggy) attempt at normalization. This function is for Gemini 3 models, which expect includeThoughts (camelCase). If a request provides include_thoughts (snake_case), this new logic allows it to pass through. The API will likely ignore the setting. Please reintroduce logic to normalize include_thoughts to includeThoughts.

}
if it := gjson.GetBytes(body, "generationConfig.thinkingConfig.include_thoughts"); it.Exists() {
updated, _ = sjson.DeleteBytes(updated, "generationConfig.thinkingConfig.include_thoughts")
}
if tb := gjson.GetBytes(body, "generationConfig.thinkingConfig.thinkingBudget"); tb.Exists() {
updated, _ = sjson.DeleteBytes(updated, "generationConfig.thinkingConfig.thinkingBudget")
}
Expand Down Expand Up @@ -167,15 +173,15 @@ func ApplyGeminiCLIThinkingLevel(body []byte, level string, includeThoughts *boo
incl = &defaultInclude
}
if incl != nil {
valuePath := "request.generationConfig.thinkingConfig.includeThoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
if !gjson.GetBytes(updated, "request.generationConfig.thinkingConfig.includeThoughts").Exists() &&
!gjson.GetBytes(updated, "request.generationConfig.thinkingConfig.include_thoughts").Exists() {
valuePath := "request.generationConfig.thinkingConfig.includeThoughts"
rewritten, err := sjson.SetBytes(updated, valuePath, *incl)
if err == nil {
updated = rewritten
}
}
Comment on lines +176 to 183
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

As with ApplyGeminiThinkingLevel, this change fixes the overwriting issue but removes key normalization. For Gemini 3 models, include_thoughts should be converted to includeThoughts to ensure the API processes the setting correctly. Please add logic to handle this normalization.

}
if it := gjson.GetBytes(body, "request.generationConfig.thinkingConfig.include_thoughts"); it.Exists() {
updated, _ = sjson.DeleteBytes(updated, "request.generationConfig.thinkingConfig.include_thoughts")
}
if tb := gjson.GetBytes(body, "request.generationConfig.thinkingConfig.thinkingBudget"); tb.Exists() {
updated, _ = sjson.DeleteBytes(updated, "request.generationConfig.thinkingConfig.thinkingBudget")
}
Expand Down