gemini tools + structured output fix - #3633
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughGemini conversion now avoids sending JSON structured-output hints for tool-calling on non-Gemini-3.0+ models. Bifrost HTTP router centralizes JSON parsing and marks connections to close and respond with HTTP 400 on request parse failures; tests cover parsing, conversion, and keep-alive socket closure. ChangesRequest Handling and Provider Compatibility
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant RequestParser
participant ResponseWriter
Client->>Router: POST /v1/chat/completions (body)
alt custom RequestParser set
Router->>RequestParser: invoke custom parser
RequestParser--XRouter: parse error
Router->>Router: SetConnectionClose()
Router->>ResponseWriter: newBifrostErrorWithCode(400, "failed to parse request")
else default JSON parse
Router->>Router: parseJSONRequestBody(body)
Router--XRouter: sonic.Unmarshal fails
Router->>Router: SetConnectionClose()
Router->>ResponseWriter: newBifrostErrorWithCode(400, "Invalid JSON request body (len=...)")
else success
Router->>ResponseWriter: proceed to RequestConverter/handler
end
ResponseWriter->>Client: 400 + Connection: close (on errors) / normal response (on success)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Confidence Score: 4/5The connection-close and HTTP router changes are clean; the Gemini utils fix has a one-line discrepancy that will cause one of the four new tests to fail. The core/providers/gemini/utils.go — the tools + structured output conflict guard at line 1251 Important Files Changed
Reviews (2): Last reviewed commit: "Apply suggestions from code review" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Merge activity
|
| if len(params.Tools) > 0 && | ||
| config.ResponseMIMEType == "application/json" && | ||
| !isGemini3Plus(model) { | ||
| config.ResponseMIMEType = "" | ||
| config.ResponseJSONSchema = nil | ||
| } |
There was a problem hiding this comment.
config.ResponseJSONSchema = nil (line 1251) clears the schema for every model/tool combination, including json_schema format. But the test Gemini2.5_ToolsWithJSONSchema_DropsResponseMimeType asserts assert.NotNil(t, result.GenerationConfig.ResponseJSONSchema, "responseJsonSchema should still be forwarded"), and the PR description explicitly states it "still forwards responseJsonSchema". As a result, the json_schema + tools sub-test will fail: ResponseMIMEType is correctly cleared but ResponseJSONSchema is also wiped, causing the NotNil assertion to fail.
| if len(params.Tools) > 0 && | |
| config.ResponseMIMEType == "application/json" && | |
| !isGemini3Plus(model) { | |
| config.ResponseMIMEType = "" | |
| config.ResponseJSONSchema = nil | |
| } | |
| if len(params.Tools) > 0 && | |
| config.ResponseMIMEType == "application/json" && | |
| !isGemini3Plus(model) { | |
| config.ResponseMIMEType = "" | |
| // ResponseJSONSchema is intentionally kept: Gemini 2.x rejects the pairing | |
| // of responseMimeType + function declarations, but responseJsonSchema alone | |
| // (without responseMimeType) is still forwarded for schema-constrained output. | |
| } |
2 participants
Summary
Fixes two distinct issues: (1) Gemini 2.5 and earlier models reject requests that combine function-calling tools with
responseMimeType: "application/json"(structured output / JSON mode). This PR drops theresponseMimeTypefield for those models when tools are present, while still forwardingresponseJsonSchemaso schema-constrained output continues to work. Gemini 3.x supports the combination and is left unchanged. (2) Malformed JSON request bodies now return HTTP 400 withConnection: close, preventing the server from leaving keep-alive sockets open after a parse failure.Changes
convertParamsToGenerationConfig, clearResponseMIMETypewhen tools are present and the model is older than Gemini 3.x, matching the Gemini API's documented constraint.parseJSONRequestBodyhelper that wrapssonic.Unmarshalwith a descriptive error including the body length.RequestParserpath and the default JSON parsing path now callctx.SetConnectionClose()and return HTTP 400 (vianewBifrostErrorWithCode) on parse failure, rather than leaving the status code unset and the connection open.TestStructuredOutputWithToolsConflictcovering four scenarios: Gemini 2.5 with tools +json_object, Gemini 2.5 with tools +json_schema, Gemini 3.x with tools +json_schema, and Gemini 2.5 withjson_schemabut no tools.Connection: close, that valid keep-alive requests reuse the socket, and that a malformed request closes the socket and makes subsequent reads fail.Type of change
Affected areas
How to test
Expected: all new and existing tests pass. Specifically:
Gemini2.5_ToolsWithJSONObject_DropsResponseMimeTypeandGemini2.5_ToolsWithJSONSchema_DropsResponseMimeTypeassertResponseMIMETypeis empty.Gemini3_ToolsWithJSONSchema_KeepsResponseFormatassertsResponseMIMETypeis"application/json".TestCreateHandler_ParseFailureClosesKeepAliveSocket/malformed_request_closes_the_socketassertsresp.Close == trueand that a subsequent read on the same connection returns an error.Breaking changes
Related issues
Security considerations
None beyond the connection-close fix, which prevents a client from potentially reusing a connection that was left in an ambiguous state after a bad request.
Checklist
docs/contributing/README.mdand followed the guidelines