Conversation
…ction declarations with built-in tools The Gemini Developer API requires tool_config.include_server_side_tool_invocations when both function declarations and built-in tools (e.g. GoogleSearch) are present in the same request. Neither SDK set this flag, causing every such request to fail with 400 INVALID_ARGUMENT. Set the flag automatically in both Python and TypeScript when the combination is detected. The flag is skipped on Vertex AI, which does not support it, and is not set when the caller provides an explicit tool_config via params. Closes strands-agents#3639
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@strandly-the-agent Review this PR |
|
Assessment: Comment Focused, well-scoped bug fix with good cross-SDK parity on the core condition (toolSpecs + built-in tools + not-Vertex) and solid test coverage on both sides (no-choice, auto, Vertex-skip, built-in-only). A couple of non-blocking items below. Review Categories
Nice job skipping the flag on Vertex and covering it with a dedicated test in both SDKs. |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Changes requested — one 🔴: the Vertex skip only inspects client_args, so the other two ways of selecting Vertex now hard-fail a request that succeeds on main.
🔴 gemini.py:358-360 — GOOGLE_GENAI_USE_VERTEXAI=true and the non-legacy client_args={"enterprise": True} both leave client_args["vertexai"] unset, so the flag is sent to Vertex and google-genai raises ValueError: include_server_side_tool_invocations parameter is only supported in Gemini Developer API mode… before the request leaves the process. TS gets this right by reading this._client.vertexai. Inline.
🟡 strands-py/pyproject.toml:50 — floor is google-genai>=1.67.0, but the field first exists in 1.68.0; at the floor gemini.py:365 raises AttributeError: 'ToolConfig' object has no attribute 'include_server_side_tool_invocations' on every mixed-tools request. One-line bump (closed #3646 had it). TS needs nothing: @google/genai 2.6.0 already declares the field.
🟡 A caller-supplied tool_config gets three different treatments for one semantic input — ToolConfig instance → flag injected into the caller's own object; equivalent dict → skipped; TS → dropped entirely. Inline.
The core condition, the Vertex-skip intent and the test matrix are otherwise right, and all three fixes are small.
Verified, questions, appendix
✅ Verified at 2abb0171 (base 0ab57b24)
cd strands-py && uv run --extra gemini --extra dev pytest tests/strands/models/test_gemini.py -q→ 79 passed;cd strands-ts && npx vitest run --project unit-node src/models/__tests__/google.test.ts→ 87 passed;ruff check+ruff format --checkclean on the touched Python files.- Vertex really does reject the flag (not taking the PR description's word):
google/genai/models.py:4008-4012raises inside_ToolConfig_to_vertex, and the@google/genaidist carries the mirrorthrow. So sending it on Vertex is a hard failure, not a silent no-op — which is what makes the 🔴 bite. - 🔴 regression proof (
vertex-env-regression.txt): identical config —GOOGLE_GENAI_USE_VERTEXAI=true, one agent tool +gemini_tools=[Tool(google_search=…)]. Base0ab57b24gets as far as the wire (fails only on missing ADC);pr-4754raises theValueErrorlocally. Also reproduced viaclient_args={"enterprise": True}(repro-output.txtcase C). - Floor (
google-genai-floor-check.txt): at 1.67.0ToolConfig has field: Falseand_format_request_config→AttributeError; at 1.68.0 the flag is set.@google/genai2.6.0'sdist/genai.d.tsdeclaresincludeServerSideToolInvocations?: booleanonToolConfig, so^2.6.0is fine. - Precedence (
repro-output.txt,ts-toolconfig-precedence.txt): instance → flag injected,sent is caller_tc→True, caller object mutated; dict →None; TS →{"functionCallingConfig":{"mode":"NONE"}}, no flag. - TS structured output is covered by the same condition:
StructuredOutputToolis registered on the tool registry (agent.ts:1580) andtoolSpecsare read from that registry (agent.ts:2083), soAgent({ structuredOutputSchema, model: new GoogleModel({ builtInTools }) })gets the flag with zero user tools. Python'sstructured_outputusesresponse_schemawithtool_specs=None, so it never mixes.
Questions
- (drives the 🟡, otherwise non-blocking) Is "the caller owns
tool_config" the right rule for a field that is a transport requirement rather than a policy choice? Someone who setsparams={"tool_config": …}for an unrelated reason keeps exactly the 400 this PR fixes. If the answer is "the flag always goes on", both SDKs should merge it in; if it's "caller wins", Python shouldn't inject into a suppliedToolConfigeither. - (non-blocking) Gating on
gemini_toolsbeing non-empty rather than on the built-ins actually being server-side (url_context, etc.) is broader than the API's trigger. Harmless as far as I can tell — deliberate?
Reading order
gemini.py:357-366 → model.ts:345-352 → the two Vertex tests.
Appendix — non-blocking (3)
- The new tests don't reference #3639; AGENTS.md:57 asks a regression test for a discovered bug to link the issue it guards against.
test_stream_gemini_tools_flag_skipped_when_params_owns_tool_configproves "the caller owns it" only for the dict shape; theToolConfig-instance shape behaves the opposite way and is untested (see the 🟡).- Docs/changelog: nothing needed that I can find —
site/src/content/changelog/sdk/is per-release rather than per-PR, andgoogle.mdx:174-209already presents built-in + function tools as a supported combination.
abde1b2 to
76ddf6a
Compare
|
@strandly-the-agent Review this PR |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Follow-up at 76ddf6a5: the 🔴 is fixed and both SDKs now share one tool_config rule. Two 🟡 left before I'd approve — one still open from last round, one introduced by the fix (my suggestion, so partly on me).
- 🔴 Vertex detection → fixed.
GOOGLE_GENAI_USE_VERTEXAI=trueandclient_args={"enterprise": True}both now send no flag (re-ran the repro). - 🟡 caller
tool_configmutated / dict skipped / TS dropped → fixed. Instance is copied viato_json_dict(), dict is merged, TS merges after the params spread and keeps an explicitfalse; both SDKs behave the same and each case has a test. - 🟡
google-genai>=1.67.0floor → still open (the thread was resolved butpyproject.toml:50is unchanged). At 1.67.0 the new code fails differently:ValidationError: tool_config.include_server_side_tool_invocations — Extra inputs are not permitted(ToolConfigisextra='forbid'there). One-line bump to>=1.68.0. - 🟡 new:
is_vertex = self._get_client().vertexairuns unconditionally, so every Gemini request now builds a secondgenai.Client(stream()builds its own two lines later). Inline, with a fix.
Verified, appendix
✅ Verified at 76ddf6a5
uv run --extra gemini --extra dev pytest tests/strands/models/test_gemini.py -q→ 81 passed;npx vitest run --project unit-node src/models/__tests__/google.test.ts→ 88 passed;ruff check/ruff format --check/mypyclean ongemini.py.- Repro re-run (formatter level,
_format_request(...)["config"]["tool_config"]): env-var Vertex →None;enterprise=True→None; callerToolConfiginstance → flag sent,caller_tc.include_server_side_tool_invocationsstaysNone; dicttool_config→{'function_calling_config': {'mode': 'AUTO'}, 'include_server_side_tool_invocations': True}. - Floor: scratch venv pinned to
google-genai==1.67.0→ValidationErrorabove; 1.68.0+ → flag set. - Extra client per request:
_format_request_config→_get_client()→genai.Client(**client_args)(gemini.py:142) on every call, including requests with nogemini_tools. In Vertex mode with no explicitprojectthat constructor also runsgoogle.auth.default()(google/genai/_api_client.py:789-794), so that now happens twice per request.
Appendix — non-blocking (2)
- New tests still don't reference #3639 (AGENTS.md:57 asks regression tests to link the issue they guard).
- Gating on
gemini_toolsbeing non-empty rather than on the built-ins actually being server-side is broader than the API's trigger — deliberate? Harmless as far as I can tell.
Co-authored-by: Strandly (The Agent) <strands-agents+strandly@amazon.com>
|
Assessment: Approve Re-reviewed after the latest revision — all three items from my previous round are addressed, and I verified the tests locally (11 relevant Python tests pass, all 88 TS Resolved from prior review
Note on the Codecov reportThe Codecov "0% patch coverage / 6 missing lines" on Dependency bump to |
|
@strandly-the-agent Review this PR |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Approve at f7804c5f — both remaining 🟡 are fixed; nothing open from me.
google-genaifloor → fixed,>=1.68.0(pyproject.toml:50).- Extra
genai.Clientper request → fixed:_get_client()is now the last operand of the short-circuit, so it only runs for requests with bothtool_specsandgemini_tools. Verified by countinggenai.Clientconstructions in_format_request: 0 withoutgemini_tools, 0 withgemini_toolsbut no tools, 1 with both.
Verified
uv run --extra gemini --extra dev pytest tests/strands/models/test_gemini.py -q→ 81 passed;ruff check/ruff format --check/mypyclean ongemini.py. TS unchanged since76ddf6a5(88 passed there).- Delta
76ddf6a5..f7804c5fis only the floor bump and the short-circuit reorder; earlier verified fixes (Vertex via resolved client, non-mutatingtool_configmerge, TS merge after params spread) untouched. uv.lockis untracked in this repo, so nothing to update for the bump.
|
Assessment: Approve (re-confirmed after Reviewed the latest Verified locally:
No outstanding concerns. Good to merge. 🚀 |
|
Assessment: Approve (re-confirmed on Reviewed the newly force-pushed Re-ran on the new HEAD: 79 Python Nothing further from me — this remains good to merge. 🚀 |
Description
The Gemini Developer API requires
tool_config.include_server_side_tool_invocations = truewhen both function declarations and built-in tools (e.g.googleSearch,codeExecution) are present in the same request. Neither SDK set this flag, causing every such request to fail:This PR makes both SDKs detect the combination and set the flag automatically. The flag is skipped on Vertex AI, which does not support it. This also bumps gemini to
google-genai>=1.68.0, which is the version introducing the flag.Related Issues
Closes #3639
Type of Change
Bug fix
Testing
hatch run preparenpm run checkChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.