Python: fix(bedrock): don't send toolChoice when no tools are configured#5172
Python: fix(bedrock): don't send toolChoice when no tools are configured#5172Bahtya wants to merge 3 commits intomicrosoft:mainfrom
Conversation
BedrockChatClient was sending toolConfig.toolChoice even when no tools were configured (tools=None). AWS Bedrock requires toolConfig.tools to be present whenever toolChoice is specified, causing a 400 validation error. Only set toolChoice when tool_config has a 'tools' key present. Fixes microsoft#5165 Signed-off-by: bahtya <bahtyar153@qq.com>
eavanvalkenburg
left a comment
There was a problem hiding this comment.
Could you add a test to verify the behavior?
|
@eavanvalkenburg Thanks for the review! I will add a test to verify that toolChoice is not sent when no tools are provided. Will push the update shortly. |
- test_prepare_options_tool_choice_auto_without_tools_omits_tool_config - test_prepare_options_tool_choice_required_without_tools_omits_tool_config Verifies that toolConfig is omitted when tool_choice is set but no tools are provided, preventing ParamValidationError from Bedrock.
|
@Bahtya please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
chetantoshniwal
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 94%
✗ Correctness
The core fix in _chat_client.py is correct: it guards toolChoice assignment behind
tool_config and "tools" in tool_config, preventing Bedrock ParamValidationError when tool_choice is set but no tools are provided. The old code usedtool_config = tool_config or {}which would create an empty dict with only toolChoice and no tools, which Bedrock rejects. The new tests in test_bedrock_client.py properly cover both auto and required modes without tools. However, test_addition.py at the repo root is a stray duplicate test file with no imports — it would fail if discovered by a test runner and should not be committed.
✗ Security Reliability
The core fix in
_chat_client.pyis a valid reliability improvement: it prevents sending atoolChoicedirective to Bedrock when no tools are defined, which would cause aParamValidationError. The logic change is sound—it guardstoolChoiceassignment behind a check fortool_config and 'tools' in tool_config. However, a test-only filetest_addition.pywas added at the repository root that is non-functional (missing imports, no test infrastructure), appears to be an accidental leftover, and should be removed.
✗ Test Coverage
The production code change correctly guards against sending toolChoice without tools to Bedrock. The two new tests in test_bedrock_client.py properly cover the 'auto' and 'required' no-tools scenarios. However, there is a stray
test_addition.pyfile at the repo root that duplicates these tests but is broken (missing imports and helpers), and there is a missing test for the 'required' mode with a specificrequired_function_namewhen no tools are provided.
✗ Design Approach
The core fix in
_chat_client.pycorrectly prevents sending atoolChoiceto Bedrock when no tools are registered, avoidingParamValidationError. However, silently swallowing the constraint for therequiredmode (which semantically means 'the model MUST call a tool') masks a likely caller misconfiguration — aValueErrorwould be more appropriate there. More critically,test_addition.pywas accidentally committed to the repo root: it is a broken duplicate of the tests already added totest_bedrock_client.py, missing all imports and the_make_clienthelper, and will fail immediately if pytest discovers it.
Automated review by chetantoshniwal's agents
| @@ -0,0 +1,36 @@ | |||
|
|
|||
There was a problem hiding this comment.
This file is a stray duplicate of the tests already added to python/packages/bedrock/tests/test_bedrock_client.py. It is missing all necessary imports (_make_client, Message, Content, Any) and will fail with NameError if any test runner discovers it. Please remove this file from the PR.
| tool_config["toolChoice"] = {"auto": {}} | ||
| if tool_config and "tools" in tool_config: | ||
| tool_config["toolChoice"] = {"auto": {}} | ||
| case "required": |
There was a problem hiding this comment.
Silently ignoring tool_choice='required' when no tools are present masks a caller misconfiguration. required means the model must invoke a tool; having no tools makes this a logical contradiction. Raising a ValueError here surfaces the bug to the caller rather than silently degrading behaviour.
| case "required": | |
| case "required": | |
| if not (tool_config and "tools" in tool_config): | |
| raise ValueError( | |
| "tool_choice='required' was specified but no tools were provided." | |
| ) | |
| if required_name := tool_mode.get("required_function_name"): | |
| tool_config["toolChoice"] = {"tool": {"name": required_name}} | |
| else: | |
| tool_config["toolChoice"] = {"any": {}} |
…eError for required without tools 1. Remove test_addition.py — stray duplicate of tests already in python/packages/bedrock/tests/test_bedrock_client.py, missing all necessary imports and would fail with NameError. 2. Change tool_choice='required' handling to raise ValueError when no tools are configured instead of silently falling through. Using 'required' without tools is a logical contradiction — the model must invoke a tool but none exist — so surfacing this as a ValueError helps callers catch the misconfiguration early. 3. Update the corresponding test to expect ValueError instead of silently omitted toolConfig.
|
Thanks for the detailed review @chetantoshniwal! I have addressed both points of feedback: 1. Removed stray duplicate test file ( 2. case "required":
if not (tool_config and "tools" in tool_config):
raise ValueError(
"tool_choice='required' requires at least one tool to be configured, "
"but no tools were provided."
)This surfaces the caller misconfiguration immediately rather than silently degrading behaviour. The corresponding test ( |
Problem
When using
BedrockChatClientwith anAgentthat has no tools configured, the client still sendstoolConfig.toolChoiceto the Bedrock API. AWS Bedrock requirestoolConfig.toolsto be present whenevertoolChoiceis specified, causing a 400 validation error.Root Cause
In
_prepare_run_options, whentool_choiceis set but_prepare_toolsreturnsNone(no tools), the code creates an empty dict withtool_config or {}and addstoolChoiceto it. This results in sending{"toolChoice": {"auto": {}}}without anytoolskey.Fix
Only set
toolChoicewhentool_configis not None and has a"tools"key. When no tools are configured,toolChoiceis silently skipped.Fixes #5165