fix: IncludeServerSideToolInvocations field in genai api - #5576
Conversation
|
tejas ghatte seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughGemini request schemas and conversions now support optional server-side tool invocations, preserve configuration across JSON formats, separate function and Google Search tools, account for grounding search usage, and emit multi-source citations in streaming and non-streaming Responses conversions. ChangesGemini tooling and grounding
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPRequest
participant RequestParameters
participant GeminiConversion
participant GeminiGenerationRequest
HTTPRequest->>RequestParameters: parse include_server_side_tool_invocations
RequestParameters->>GeminiConversion: pass opt-in flag
GeminiConversion->>GeminiGenerationRequest: create separate function and Google Search tools
GeminiConversion->>GeminiGenerationRequest: apply server-side tool invocation settings
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
fe968cd to
a391b54
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/providers/gemini/responses.go (1)
3908-3966: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCitation-building logic duplicated with the non-stream path; prefer
schemas.Ptr()over&field.The (support, chunk) →
url_citationannotation construction here (3928-3961) mirrors the non-stream logic at responses.go lines 2719-2743 almost line-for-line (bounds/URL checks, Title/URL/segment-offset population). Extracting a shared helper, e.g.buildURLCitationAnnotations(supports []*GroundingSupport, chunks []*GroundingChunk) []schemas.ResponsesOutputMessageContentTextAnnotation, would keep both call sites in sync going forward — they were both touched by this very PR and are already showing minor drift (see the&fieldvsschemas.Ptr()note below).Separately, lines 3940/3943/3948 take the address of struct fields directly (
&chunk.Web.URI,&support.Segment.Text,&chunk.Web.Title) instead of usingschemas.Ptr(...)as the non-stream path and the rest of the codebase does.Based on learnings, "prefer using bifrost.Ptr() to create pointers instead of the address operator (&) even when & would be valid syntactically... Apply this consistently across all code paths."
♻️ Proposed Ptr() fix
annotation := schemas.ResponsesOutputMessageContentTextAnnotation{ Type: "url_citation", - URL: &chunk.Web.URI, + URL: schemas.Ptr(chunk.Web.URI), } if support.Segment.Text != "" { - annotation.Text = &support.Segment.Text + annotation.Text = schemas.Ptr(support.Segment.Text) } annotation.StartIndex = schemas.Ptr(int(support.Segment.StartIndex)) annotation.EndIndex = schemas.Ptr(int(support.Segment.EndIndex)) if chunk.Web.Title != "" { - annotation.Title = &chunk.Web.Title + annotation.Title = schemas.Ptr(chunk.Web.Title) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/responses.go` around lines 3908 - 3966, Extract the shared grounding-support citation construction used by emitAnnotationsFromGroundingSupports and the non-stream path into a helper such as buildURLCitationAnnotations, preserving the existing bounds, URL, segment-offset, and multi-source behavior. Update both call sites to use the helper, and construct URL, text, and title pointers with schemas.Ptr(...) instead of taking field addresses.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/gemini/responses.go`:
- Around line 2719-2766: Update grounding citation attachment to use each
GroundingSupport.Segment.PartIndex when selecting the candidate text block,
rather than always targeting textMessageIdx; apply the same mapping in the
streaming helper. Preserve the standalone fallback only when the referenced text
block is absent, and initialize its text block’s LogProbs as an empty array
instead of null.
---
Nitpick comments:
In `@core/providers/gemini/responses.go`:
- Around line 3908-3966: Extract the shared grounding-support citation
construction used by emitAnnotationsFromGroundingSupports and the non-stream
path into a helper such as buildURLCitationAnnotations, preserving the existing
bounds, URL, segment-offset, and multi-source behavior. Update both call sites
to use the helper, and construct URL, text, and title pointers with
schemas.Ptr(...) instead of taking field addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9f7792a-9ea8-41fd-b2f7-0d1166381e23
📒 Files selected for processing (9)
core/providers/gemini/chat.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/gemini/types.gocore/providers/gemini/utils.gocore/providers/gemini/websearchstreamstate_test.gocore/schemas/chatcompletions.gocore/schemas/responses.gotransports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (7)
- core/schemas/chatcompletions.go
- core/providers/gemini/chat.go
- core/schemas/responses.go
- core/providers/gemini/utils.go
- core/providers/gemini/types.go
- transports/bifrost-http/handlers/inference.go
- core/providers/gemini/websearchstreamstate_test.go
a391b54 to
f5d98ef
Compare
f5d98ef to
fd57bd8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/gemini/chat.go`:
- Around line 69-71: Update the Gemini chat conversion flow around
convertBifrostToolsToGemini and the IncludeServerSideToolInvocations check so
function declarations are omitted unless server-side tool invocations are
explicitly enabled. Ensure requests with unset or false opt-in, including those
using WebSearchOptions, do not send function declarations, and add chat coverage
for both disabled states.
In `@core/providers/gemini/utils.go`:
- Around line 1850-1869: Update applyServerSideToolInvocations to handle
requests without FunctionDeclarations: either skip enabling server-side tool
invocations at the caller in ToGeminiChatCompletionRequest, or clear
FunctionCallingConfig before applying the mode when declarations are absent.
Ensure search-only requests do not send an invalid tool config, while preserving
the existing AUTO-to-VALIDATED behavior when function declarations exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b0d9f220-cbcd-4e5c-b314-6af156b8eefd
📒 Files selected for processing (13)
core/providers/gemini/chat.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/gemini/types.gocore/providers/gemini/utils.gocore/providers/gemini/websearchstreamstate_test.gocore/schemas/chatcompletions.gocore/schemas/responses.gotests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtests/e2e/api/collections/provider-harness.jsontransports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (11)
- core/schemas/responses.go
- tests/cmd/seed/go.mod
- tests/cmd/e2eseed/go.mod
- core/schemas/chatcompletions.go
- transports/bifrost-http/handlers/inference.go
- core/providers/gemini/types.go
- tests/cmd/seedvks/go.mod
- tests/e2e/api/collections/provider-harness.json
- core/providers/gemini/websearchstreamstate_test.go
- core/providers/gemini/gemini_test.go
- core/providers/gemini/responses.go
fd57bd8 to
f3ebadb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/providers/gemini/chat.go (1)
41-83: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winChat conversion path may still send function declarations + Google Search together without the opt-in flag, and no test would catch it.
ToGeminiChatCompletionRequestWithImageURLSchemesunconditionally builds function declarations (line 43) and unconditionally appends theGoogleSearchtool forWebSearchOptions(line 77);applyServerSideToolInvocations(lines 80-82) only sets flags/mode whenIncludeServerSideToolInvocationsistrue— it never strips declarations when the flag isfalse/absent. This mirrors a previously flagged issue on this same PR (chat.go:69-71, marked "Addressed") and the currently visible code shows no drop logic, so it's unclear the fix covers the chat path. Meanwhile, the Responses path is proven (via tests) to drop declarations in the equivalent scenario.
core/providers/gemini/chat.go#L41-L83: add the same declarations-drop logic used on the Responses path (or verify it already exists inconvertBifrostToolsToGemini/responses.go) whenWebSearchOptionsis set andIncludeServerSideToolInvocationsis false/absent.core/providers/gemini/gemini_test.go#L4823-L4844: add a chat-path subtest forIncludeServerSideToolInvocationsfalse/nil +WebSearchOptions+ function tools, asserting only theGoogleSearchtool remains — mirroring the existing Responses-path negative test at lines 4783-4789.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/providers/gemini/chat.go` around lines 41 - 83, The chat conversion path in ToGeminiChatCompletionRequestWithImageURLSchemes must remove function declarations when WebSearchOptions is set and IncludeServerSideToolInvocations is false or absent, leaving only the GoogleSearch tool; reuse the equivalent Responses-path logic rather than changing convertBifrostToolsToGemini broadly. Add a chat-path subtest in core/providers/gemini/gemini_test.go covering nil/false opt-in with WebSearchOptions and function tools, asserting that only GoogleSearch remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/providers/gemini/gemini_test.go`:
- Around line 4823-4844: Add chat-path negative subtests alongside “chat path
sets the flag on toolConfig” for IncludeServerSideToolInvocations nil and false
with WebSearchOptions configured; assert the request succeeds and only the
GoogleSearch tool remains, with function declarations omitted. Mirror the
existing Responses flag-false coverage while keeping the current true-case test
unchanged.
---
Duplicate comments:
In `@core/providers/gemini/chat.go`:
- Around line 41-83: The chat conversion path in
ToGeminiChatCompletionRequestWithImageURLSchemes must remove function
declarations when WebSearchOptions is set and IncludeServerSideToolInvocations
is false or absent, leaving only the GoogleSearch tool; reuse the equivalent
Responses-path logic rather than changing convertBifrostToolsToGemini broadly.
Add a chat-path subtest in core/providers/gemini/gemini_test.go covering
nil/false opt-in with WebSearchOptions and function tools, asserting that only
GoogleSearch remains.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6b4aad0-04cc-491a-9d52-12a27e2d3365
📒 Files selected for processing (13)
core/providers/gemini/chat.gocore/providers/gemini/gemini_test.gocore/providers/gemini/responses.gocore/providers/gemini/types.gocore/providers/gemini/utils.gocore/providers/gemini/websearchstreamstate_test.gocore/schemas/chatcompletions.gocore/schemas/responses.gotests/cmd/e2eseed/go.modtests/cmd/seed/go.modtests/cmd/seedvks/go.modtests/e2e/api/collections/provider-harness.jsontransports/bifrost-http/handlers/inference.go
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/cmd/seed/go.mod
- core/schemas/responses.go
- core/schemas/chatcompletions.go
- core/providers/gemini/types.go
- transports/bifrost-http/handlers/inference.go
- tests/cmd/e2eseed/go.mod
- core/providers/gemini/websearchstreamstate_test.go
- tests/e2e/api/collections/provider-harness.json
- tests/cmd/seedvks/go.mod
- core/providers/gemini/utils.go
- core/providers/gemini/responses.go
Merge activity
|
The base branch was changed.

Summary
Adds
include_server_side_tool_invocationsas a first-class parameter on both the Chat and Responses schemas. Gemini 3+ rejects requests that mix function declarations with built-in tools like Google Search unless this flag is explicitly set; without it, function declarations are silently dropped to preserve existing behaviour. This PR also fixes a bug where grounding annotations were only emitting one citation per segment (the first chunk index), losing all additional sources.Changes
IncludeServerSideToolInvocations *booltoChatParametersandResponsesParametersschemas, registered in the HTTP handler's known-fields maps for both chat and responses paths.applyServerSideToolInvocationshelper in Gemini utils that setstoolConfig.includeServerSideToolInvocations = trueand promotesAUTO(or unset) function-calling mode toVALIDATED, as Gemini rejectsAUTOin combination mode.convertResponsesToolsToGeminito accept the flag and, when set, emit function declarations and Google Search as separateToolentries (matching Google's documented tool-combination shape) rather than collapsing them into one or dropping declarations.ToGeminiChatCompletionRequest) and responses (ToGeminiResponsesRequest) conversion paths.ToBifrostResponsesRequest(native genai round-trip) to readIncludeServerSideToolInvocationsfromToolConfig, with a customUnmarshalJSONonToolConfigthat accepts both camelCase and snake_case spellings.convertGeminiCandidatesToResponsesOutput) and streaming (emitAnnotationsFromGroundingSupports) paths to iterate over allGroundingChunkIndicesper support rather than only the first, skipping chunks with noWebfield or an empty URI, and using per-annotation index values instead of a shared pointer.Type of change
Affected areas
How to test
To exercise end-to-end, send a Gemini 3+ request with both a
web_searchtool and a function declaration, setting"include_server_side_tool_invocations": true. Without the flag the function declaration is dropped; with it both tools appear on the wire and Gemini accepts the request.Breaking changes
Existing behaviour is preserved: when the flag is absent or
false, function declarations are still dropped when Google Search is present, matching the previous default.Related issues
Security considerations
None. The flag controls tool-combination semantics on the Gemini API and does not affect authentication, secrets, or PII handling.
Checklist
docs/contributing/README.mdand followed the guidelines