feat: support Tool Choice for Gemini in Python - #3551
Conversation
Port the tool_choice mapping from the TypeScript SDK's GoogleModel so a Strands ToolChoice reaches the Gemini API as a tool config: - auto -> FunctionCallingConfigMode.AUTO - any -> FunctionCallingConfigMode.ANY - tool -> FunctionCallingConfigMode.ANY + allowed_function_names Closes strands-agents#1129
…aram A tool config supplied through the params config was winning over the per-request tool_choice, which silently defeated the forced retry the event loop uses for structured output. The per-request choice is the more specific instruction, matching how BedrockModel drops a conflicting thinking config when a tool is forced.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Assessment: Approve Clean, focused fix that closes a real py↔ts parity gap. The Review notes
Nicely scoped PR with clear rationale in the description and matching test evidence. |
|
@strandly-the-agent review this pr |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Heads up before anything else: this is a self-review. I wrote this branch (at opieter-aws's request on #1129), so I'm reviewing my own code. I mitigated that with six independent fresh-context passes and then re-verified the two strongest findings myself — but weight the evidence below, not my own rationale in the PR body.
Verdict: changes requested — one real defect, in the precedence rule I added in the second commit.
🟡 gemini.py:349-351 replaces a user's params["tool_config"] for every recognized tool_choice — including {"auto": {}}, which imposes no constraint. An explicit allowed_function_names narrowing silently vanishes, as does any orthogonal field (retrieval_config). Reproduced independently, twice. Fix is 8 lines, verified green — inline.
🟡 A new integ test asserts exact-list equality on a Gemini ANY-forced call; ANY doesn't guarantee call count, so it's a flake risk — inline.
⚪ I've endorsed the existing unresolved github-actions docstring thread rather than duplicating it.
Also: the two tests_integ cases have never been executed by anyone — no GOOGLE_API_KEY in my sandbox then or now, and the Testing section doesn't say so. Please let CI exercise them before merge. Suggested label: api/needs-review (solo-approvable, not an API meeting).
Verification ledger, three non-blocking questions, and what I suppressed
Ledger
| ✅ Reviewed | branch agent-tasks/1129 @ a720d2c8, base 2b84dfa5 |
| ✅ Tests | pytest tests/strands/models/test_gemini.py -q → 64/64, python 3.12.13, google-genai 2.15.0 |
| ✅ Lint/types | ruff check + ruff format --check clean; mypy shows 1 error, pre-existing and byte-identical on 2b84dfa5 (count_tokens arg-type, unrelated) |
| ✅ Repro 1 | auto-clobber confirmed on a mocked client (details inline) |
| ✅ Repro 2 | retrieval_config set via params is discarded by any recognized tool_choice, incl. {"any":{}} on the forced-retry path |
| ✅ Suggested fix | applied + run: 64/64, ruff clean, mypy unchanged, both repros fixed |
| ✅ PR's core claim | accurate — a real Agent(structured_output_model=…) shows call #1 tool_config: null → call #2 (forced retry) mode: ANY; on main call #2 carried nothing |
✅ kw-only * |
safe — no caller in the tree passes tool_choice positionally; AGENTS.md:205 mandates the *; already shipped as non-breaking for other providers |
| ✅ Dep floor | suite green against the declared floor google-genai==1.67.0 |
| ❓ Not verified | the 2 tests_integ cases — no GOOGLE_API_KEY, never run by anyone |
Questions (all non-blocking)
- Precedence scope.
gemini.py:349makes Gemini the only provider where a per-requesttool_choicebeats staticparams— every sibling (anthropic.py:256/258,openai.py:520/521,bedrock.py:356) and TS (model.ts:344-346) let static config win. #1129 asked us to stop droppingtool_choice, not to set a conflict-resolution policy. My own first commit (877e2d72) usedsetdefaultand matched everyone; the second reversed it. Worth an explicit sign-off plus a shortDECISIONS.mdentry, and a tracking issue forAnthropicModel's identical latent gap (anthropic.py:258splats last, defeating the force atanthropic.py:541)? - Shared bug with TS.
model.ts:344-346resolves this same question the opposite way, untested on either side. Rather than this PR turning a shared latent bug into a silent cross-SDK divergence, worth astrands-tsissue so both converge on one contract? - PR body. The Bedrock analogy I wrote ("how BedrockModel resolves a conflicting thinking config") doesn't hold up:
bedrock.py:362-388is forcing-only and field-scoped — it removes one key, it doesn't replace the object. Worth tightening that sentence so the next provider author doesn't inherit it as precedent.
Suppressed (kept off the PR deliberately)
gemini_tools+tool_choicesilent drop — looks like a bug, isn't:gemini.py:684-703forbidsFunctionDeclarations ingemini_tools, so there's never a function to force. Gating ontool_specsis correct.- Malformed
{"tool": {}}raisesKeyErrorinside the provider — convention-consistent withanthropic.py:271, unreachable via typed callers. - kw-only
*pinned by no test (mutation survivor) — repo-wide gap; no provider suite pins it. Not this PR's job. if tool_specssilent no-op vswarn_on_tool_choice_not_supported— adjudicated: keep silent. Matches TS, and the orchestrator always pairstool_choicewithtool_specs, so a warning nobody can act on is noise.- Private
_format_request_configgaining a 4th positional arg — private helper, pay-for-play perDECISIONS.md. - No
site/page documents per-providertool_choice— pre-existing and SDK-wide (no provider page mentions it); the checklist claim is accurate. - Fixture placement,
GeminiConfig.paramsdocstring imprecision, test granularity — style/pre-existing.
Mutation testing (separate pass): 7 mutants, 6 killed — the new tests genuinely have teeth. The one survivor is deleting the *.
…field A per-request tool choice replaced the whole tool config from params, so an "auto" choice - which imposes no constraint - discarded an explicit allowed_function_names narrowing, and any choice discarded fields a Strands ToolChoice cannot express, such as retrieval_config. On the framework's forced structured-output retry that happened with no user action at all. A choice now overrides only when it forces a tool, and only the function calling config it can actually express; the rest of the user's tool config is carried over. The docstrings say which half of the rule applies where, and the tool config fixture carries an allowlist plus a field no choice can express so the tests pin both. Also relax an integration assertion that required exactly one tool call: Gemini's ANY mode may emit several, and only the narrowing is guaranteed.
|
Assessment: Approve Follow-up review of What changed and how I verified it
Two carry-over items for the human owner before merge (unchanged from the prior pass, not blockers): the two |
|
@strandly-the-agent can you review again? |
strandly-the-agent
left a comment
There was a problem hiding this comment.
Approving 29ad316b. All three findings from my previous pass are genuinely fixed — I re-ran the original repros against the new HEAD rather than taking the bot's word for it. Still a self-review (I wrote this branch), so the evidence below is the point, not my sign-off.
One new 🟡 the fix left behind, and it's not why I'm approving anyway: the forcing branch replaces the whole function_calling_config, so a user's stream_function_call_arguments is still dropped on any/tool — the same species as the bug we just fixed, one level deeper. ~10-line fix, verified green, inline. I'm not blocking on it: it's a niche field on a narrow path, everything I actually asked for landed, and blocking twice on progressively smaller instances of one issue would be disproportionate. Your call whether it rides along or becomes a follow-up.
Verification ledger — what I re-ran, plus carry-overs and what I suppressed
Re-verified myself on 29ad316b (delta a720d2c8..HEAD only, 21 src lines)
| ✅ Tests | 67/67 test_gemini.py; ruff check + ruff format --check clean; mypy back to 1 error, the pre-existing count_tokens one also present on base 2b84dfa5 |
| ✅ auto-clobber fixed | None and {"auto":{}} now both leave the params config fully intact (mode: ANY, allowed_function_names: [safe_tool]); {"any":{}} narrows to mode: ANY; {"tool":{name}} sets the allowlist — and retrieval_config survives all four |
| ✅ no leak | params["tool_config"] byte-identical and same object id after forcing calls. 60 concurrent interleaved calls on one model → exactly 2 distinct wire shapes (40 preserved / 20 forced), zero cross-talk, config pristine |
| ✅ dict-shaped params | params={"tool_config": {...}} (a plain dict, not a ToolConfig) goes through the new model_validate path correctly and the user's dict is not mutated |
| ✅ integ set-comparison | correct relaxation, not a weakened assertion — still fails for the wrong tool, a second different tool, or no tool at all |
| ✅ docstrings | all three now state the accurate conditional rule; my earlier accuracy complaint is resolved |
| ✅ new tests have teeth | mutation battery on the new logic: 5/7 killed. Both survivors are non-defects — if not existing: is semantically inert, and dropping the tool_choice is not None guard is caught by mypy, not the suite |
| ❓ Not verified | the 2 tests_integ cases — still no GOOGLE_API_KEY, still never executed by anyone |
Carry-overs for the human owner (unchanged, not blockers)
- CI needs to actually exercise the two integ tests.
- The
api/needs-reviewlabel still isn't applied. - Someone needs to own the "reviewed every line" checkbox — this branch is agent-authored.
❓ Non-blocking design question
auto can no longer relax a params tool config — a caller asking for {"auto": {}} against params={"tool_config": ToolConfig(mode=NONE)} gets NONE, i.e. no tool is callable despite asking for auto. Bedrock would honour the auto (bedrock.py:317 lets the per-request choice win unconditionally). The hybrid is defensible — "per-request always wins" is exactly what destroyed retrieval_config — and the forcing predicate is character-identical to bedrock.py:379. Worth one docstring line saying auto can't be used to relax a params config?
Suppressed (attacked, survived — recorded so nobody re-runs them)
- Aliasing is real but unexploitable:
ToolConfig.model_validate(x)does return the same object andmodel_copyis shallow, so the copy sharesretrieval_configwith the user's object — but_format_requestcalls.to_json_dict()before anything reaches the client, so no live object escapes. - Garbage/partially-valid
params["tool_config"]: error parity holds across 9 shapes × 3 choices — nothing that used to work now raises, nothing that used to raise now silently passes. params={"tool_config": None}and an emptyToolConfig()both take a sane branch.
…oice A per-request tool choice now applies only when params sets no tool config of its own, matching the other providers and the TypeScript SDK, which all let the explicit config win. The merge this replaces had to decide which fields a ToolChoice may overwrite, and each answer left another field behind - the allowlist, then retrieval_config, then stream_function_call_arguments. The consequence is the same as everywhere else: a tool config in params also outranks the forced structured-output retry, so a model that answers in plain text can still raise StructuredOutputException. That is parity rather than a Gemini quirk, and worth fixing for every provider at once. Tests pin the contract from both sides: a params tool config survives every choice, an explicit None still owns the key, and a choice never persists into a later request.
|
Implemented the precedence decision in
I checked parity empirically rather than assuming it: Behaviour, evidence, and what still needs a humanTests pin the contract from three sides:
Still needs a human before merge, unchanged from my earlier pass: the two |
|
Assessment: Approve (one doc-accuracy nit) Follow-up review of Verification of the reversal
Nit (non-blocking): the PR description's DevX Changes section is now stale — it still states the previous direction ("A per-request Same two carry-over items for the human owner before merge (unchanged, not blockers): CI needs |
Co-authored-by: strandly-the-agent <strandly-the-agent@users.noreply.github.com>
Description
GeminiModel.stream()accepted atool_choiceand discarded it — the docstring readNote: Currently unused.and nothing downstream consumed it. The practical cost is the event loop's forced structured-output retry: it re-invokes withtool_choice={"any": {}}expecting an API-level guarantee of a tool call, and on Gemini that retry was forced in name only, leavingStructuredOutputExceptionreachable where Anthropic and Bedrock guarantee a call. This ports the mapping that already exists in the TypeScript SDK (strands-ts/src/models/google/model.ts), closing a py↔ts parity gap.ToolChoiceFunctionCallingConfigMode{"auto": {}}AUTO{"any": {}}ANY{"tool": {"name": "X"}}ANY+allowed_function_names=["X"]A
tool_choicewith no tool specs is a no-op, matching TS — there is nothing to choose from.Public API Changes
No new surface:
tool_choicewas already onGeminiModel.stream()and on the baseModelprotocol. Two behavioral notes for reviewers.An explicit
tool_configinparamstakes precedence over a per-requesttool_choice.Two earlier commits on this branch tried the other direction, then a field-level merge; both were abandoned because a GeminiToolConfigcarries fields a StrandsToolChoicecannot express (retrieval_config,stream_function_call_arguments), so each merge rule left another user-set field behind. Letting the explicit config win passes it through whole and matches the other providers and the TypeScript SDK, which all spreadparamslast.tool_choiceis now keyword-only, matchingModel.stream()and every other provider. Gemini was the only one missing the*, so a positional fourth argument was already outside the declared interface, and was silently ignored:Related Issues
Resolves #1129
Documentation PR
N/A
Type of Change
New feature
Testing
Summary: 64/64
test_gemini.pypass (57 pre-existing + 7 new), 265 pass across the model/event-loop/structured-output suites,ruff format --check+ruff check+mypyclean, and the same suite passes against the declaredgoogle-genaifloor (1.67.0) as well as 2.14.0.Checklist
site/page covers per-providertool_choicetest_stream_tool_choice_no_warningBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.