fix(gemini): clean tool schemas, stream thinking passthrough, and strip trailing unanswered function calls#2474
Conversation
delegate schema sanitization to util.CleanJSONSchemaForGemini and drop the top-level eager_input_streaming key to prevent validation errors when sending claude tools to the gemini api
![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 refactors the ResponseRewriter to simplify thinking block suppression by replacing map-based tracking with a boolean flag, which now allows thinking blocks to pass through in streaming responses. Additionally, the Gemini-Claude request translator is updated to handle trailing model turns with unanswered function calls, clean JSON schemas for Gemini compatibility, and remove unsupported tool fields. Feedback includes removing a redundant nil check in the streaming logic and a suggestion to refine the model turn stripping to preserve text content while removing function calls.
| if rewritten == nil { | ||
| // Event suppressed (e.g. thinking block), skip event+data pair | ||
| i = dataIdx + 1 | ||
| continue | ||
| } |
There was a problem hiding this comment.
The check if rewritten == nil is now redundant because rewriteStreamEvent (line 274) always returns a non-nil slice. Previously, this check was used to skip events when thinking blocks were suppressed in the stream. Since thinking blocks are now allowed to pass through in streaming, this logic can be simplified.
| if rewritten == nil { | |
| // Event suppressed (e.g. thinking block), skip event+data pair | |
| i = dataIdx + 1 | |
| continue | |
| } | |
| rewritten := rw.rewriteStreamEvent(jsonData) | |
| // Emit event line | |
| out = append(out, line) | |
| // Emit blank lines between event and data | |
| for k := i + 1; k < dataIdx; k++ { | |
| out = append(out, lines[k]) | |
| } | |
| // Emit rewritten data | |
| out = append(out, append([]byte("data: "), rewritten...)) | |
| i = dataIdx + 1 | |
| continue |
| contents := gjson.GetBytes(out, "contents") | ||
| if contents.Exists() && contents.IsArray() { | ||
| arr := contents.Array() | ||
| if len(arr) > 0 { | ||
| last := arr[len(arr)-1] | ||
| if last.Get("role").String() == "model" { | ||
| hasFC := false | ||
| last.Get("parts").ForEach(func(_, part gjson.Result) bool { | ||
| if part.Get("functionCall").Exists() { | ||
| hasFC = true | ||
| return false | ||
| } | ||
| return true | ||
| }) | ||
| if hasFC { | ||
| out, _ = sjson.DeleteBytes(out, fmt.Sprintf("contents.%d", len(arr)-1)) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic to strip trailing model turns with unanswered function calls is effective for preventing Gemini empty responses. However, deleting the entire turn might lose valuable text content if the model turn contains both text and a function call. While this is a valid trade-off to ensure a successful response from Gemini, consider if only the functionCall parts should be removed while preserving text parts, provided the resulting turn is not empty.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
This is a solid set of fixes for AMP streaming behavior and Gemini tool/schema handling. I can’t approve yet because CI blocks translator edits.
Blocking
translator-path-guard / ensure-no-translator-changesis failing because this PR modifiesinternal/translator/**. The workflow currently forbids translator changes in PRs. Please split the AMP-only changes into this PR and route translator changes via the maintenance process (or land a separate PR that updates the guard policy).
Non-blocking
- Please run
gofmtoninternal/api/modules/amp/response_rewriter.go(alignment/blank lines). - Commit 3 subject is labeled
test(amp)but it changes translator code; might want to fix when squashing.
Test plan
go build -o test-output ./cmd/server(OK)
Follow-ups
- Consider a unit test for the “strip trailing model functionCall turn” logic (especially if the last model message contains both
textandfunctionCallparts).
|
i was actually just working on addressing the blockers and fixing a regression we caught in the streaming logic but i see this got merged |
2 participants
Summary
util.CleanJSONSchemaForGeminiand remove brittlebytes.Replacehackeager_input_streamingfrom Claude tool declarations before sending to GeminiProblem
Tool schema validation errors: The
geminitranslator sent Claude tool schemas directly to Gemini without cleaning unsupported keys ($schema,format,patternProperties,eager_input_streaming), causing 400 Bad Request errors. Mirrors fix from PR fix(gemini-cli): sanitize tool schemas and filter empty parts #2108 applied togemini-clitranslator.Broken SSE stream: The
ResponseRewriterwas suppressing thinking blocks during streaming, which corrupted the SSE event sequence (orphanedevent:lines, missingcontent_block_startfor indices Amp expected). Thinking blocks now pass through intact in streaming, matching ampcode.com behavior (ref Amp TUI crashes with 'undefined is not an object' when using local provider for Claude models with thinking #2471).Empty Gemini responses: When conversation history ended with a model turn containing an unanswered
functionCall, Gemini returned empty responses withfinishReason: STOP. The translator now strips these trailing turns.