fix(google): map tool_choice onto functionCallingConfig - #968
Conversation
📝 WalkthroughWalkthroughThe Google adapter serializes tool choices into Gemini ChangesGemini tool-choice serialization
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant buildRequest
participant toolChoiceToGeminiToolConfig
participant resolveToolChoiceWireName
participant GeminiAPI
buildRequest->>toolChoiceToGeminiToolConfig: pass declared tools and toolChoice
toolChoiceToGeminiToolConfig->>resolveToolChoiceWireName: resolve named tool choice
resolveToolChoiceWireName-->>toolChoiceToGeminiToolConfig: return wire function name
toolChoiceToGeminiToolConfig-->>buildRequest: return Gemini toolConfig
buildRequest->>GeminiAPI: send request with toolConfig when tools exist
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46756f5e50
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const toolConfig = tools ? toolChoiceToGeminiToolConfig(parsed) : undefined; | ||
| if (toolConfig) body.toolConfig = toolConfig; |
There was a problem hiding this comment.
When googleMode === "cloud-code-assist" routes to a Claude model, a request with tool_choice: "none" and a non-empty tool catalog gets mode: "NONE" here, but the later Claude Antigravity block overwrites that mode to VALIDATED while leaving body.tools intact. In that scenario the upstream still receives callable declarations and may emit tool calls even though the client explicitly disabled tools; preserve the no-tools contract by suppressing declarations for none or otherwise avoiding the VALIDATED override for that case.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
|
Please put your Pull-Request on Ready for Review, once you are finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/google-adapter.test.ts (1)
192-240: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTest the complete byte-stability contract.
The test title claims that
allowedTools + autois byte-identical. That choice filtersbody.tools, so it is not identical to an unrestricted request. The current assertions only checktoolConfig; regressions incontents,systemInstruction, or declarations would pass. (raw.githubusercontent.com)Rename the test. Compare serialized bodies for
"auto"and absent choices. Assert the filtered declaration names separately forallowedTools + auto.Proposed test adjustment
- test('"auto", absent, and allowedTools+auto stay byte-identical (no toolConfig)', async () => { - expect((await geminiBody(parsedWithChoice("auto"))).toolConfig).toBeUndefined(); - expect((await geminiBody(parsedWithChoice(undefined))).toolConfig).toBeUndefined(); - expect((await geminiBody(parsedWithChoice({ allowedTools: ["get_weather"], mode: "auto" }))).toolConfig).toBeUndefined(); + test('"auto" and absent stay byte-identical; allowedTools+auto omits toolConfig', async () => { + const autoBody = await geminiBody(parsedWithChoice("auto")); + const absentBody = await geminiBody(parsedWithChoice(undefined)); + expect(JSON.stringify(autoBody)).toBe(JSON.stringify(absentBody)); + expect(autoBody.toolConfig).toBeUndefined(); + + const allowedAutoBody = await geminiBody( + parsedWithChoice({ allowedTools: ["get_weather"], mode: "auto" }), + ); + expect(allowedAutoBody.toolConfig).toBeUndefined(); + const declared = (allowedAutoBody.tools as { functionDeclarations: { name: string }[] }[]) + [0].functionDeclarations.map(d => d.name); + expect(declared).toEqual(["get_weather"]); });🤖 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 `@tests/google-adapter.test.ts` around lines 192 - 240, Update the test named `"auto", absent, and allowedTools+auto stay byte-identical (no toolConfig)` to compare serialized bodies from `"auto"` and an absent choice, covering contents, systemInstruction, tools, and all other fields. Rename the test to reflect that only auto and absent choices are byte-identical, then separately assert that the allowedTools-plus-auto request contains only the expected filtered declaration names while retaining the existing no-toolConfig assertion.
🤖 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 `@tests/google-adapter.test.ts`:
- Around line 256-260: Update the Gemini request assertions near geminiRequest
to verify tools equals the exact declaration catalog ["get_weather",
"mcp__chrome__shot"] instead of only checking that tools is defined; retain the
existing toolConfig assertion.
---
Outside diff comments:
In `@tests/google-adapter.test.ts`:
- Around line 192-240: Update the test named `"auto", absent, and
allowedTools+auto stay byte-identical (no toolConfig)` to compare serialized
bodies from `"auto"` and an absent choice, covering contents, systemInstruction,
tools, and all other fields. Rename the test to reflect that only auto and
absent choices are byte-identical, then separately assert that the
allowedTools-plus-auto request contains only the expected filtered declaration
names while retaining the existing no-toolConfig assertion.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7a020c23-e82a-4f00-9b3b-de838de6999f
📒 Files selected for processing (2)
src/adapters/google.tstests/google-adapter.test.ts
|
You are putting it on Ready for Review and changing it afterwards. Please only change it to ready for review once you are completely finished. |
|
Sorry! That was just me addressing a CodeRabbit review comment that came in after I marked it ready. Nothing else changed. |
[GD] Verdict: changes-requestedTLDR
Full verdictSemantic propagationConcept 1 — client tool-choice → Gemini wire
Concept 2 — Claude-on-Antigravity tool availability for
Semantic propagation verdict
UsefulnessReal, user-visible bug: a Google-routed request with Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Simplification (for the PR owner)Foreign PR — nothing was edited or pushed. Two bounded, behavior-preserving candidates:
Skipped with rationale: the Claude- Gate
Bottom lineUseful, correct, well-scoped fix with good regression coverage — the Google adapter now actually enforces |
|
Carried into #973 (stack 6/6), with authorship preserved ( This is a good catch: the wire compiler already validated Checked specifically for interaction with your #943, which is already carried on this stack and touches the same file — #943 changes terminal truncation handling, #968 changes request-side compilation, separate hunks, Verified on the stack: |
3 participants
Summary
The google adapter dropped a client's tool_choice: "none", "required", and a forced tool all produced a wire body identical to auto, with only a system-prompt nudge stating the contract in prose. The wire compiler already validates and compiles toolConfig.functionCallingConfig; the adapter just never built it. buildRequest now maps the parsed choice for all three google modes: "none" to NONE, "required" to ANY, a forced tool to ANY with its resolved wire name in allowedFunctionNames, and the allowedTools variant's "required" half to ANY. "auto" and absent choices stay byte-identical, the config is only sent when declarations exist, and Claude on Antigravity keeps its protocol-required VALIDATED mode over a client choice.
Verification
tests/google-adapter.test.ts: every mapping, dotted-alias resolution to the namespaced wire name, byte-stable auto, the no-declaration guard, and the Antigravity VALIDATED interplay.bun run test,typecheck,lint:gui,privacy:scan.Checklist
Summary by CodeRabbit
New Features
Bug Fixes