Python: Fix single-tool input handling in OpenAIResponsesClient._prepare_tools_for_openai#4312
Conversation
moonbox3
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 3 | Confidence: 84%
✓ Correctness
The diff adds support for passing a single tool (not wrapped in a list) to
_prepare_tools_for_openaiby usingnormalize_toolsto coerce the input. The change is straightforward and the tests cover the new behavior. There are no blocking correctness issues. One minor observation: the type annotationSequence[Any] | Any | Noneis effectively justAnysinceAnyalready subsumes all other types, which makes the signature less informative than intended. Theif not tools:guard on line 440 runs beforenormalize_tools, which is fine forNoneand empty sequences, but could theoretically swallow a single falsy tool object (e.g., an empty dict); in practice this is extremely unlikely for valid tool inputs.
✓ Security Reliability
This diff broadens the
_prepare_tools_for_openaisignature to accept a single tool in addition to a sequence, delegating normalization tonormalize_tools. The change is small and well-tested. No security or reliability issues were found: there are no injection risks, no secret exposure, and no resource leaks. Theif not tools:early-return guard executes beforenormalize_tools, which is safe because a single valid tool object will always be truthy.
✓ Test Coverage
The diff adds support for passing a single tool (not wrapped in a list) to
_prepare_tools_for_openaiby usingnormalize_tools(). Two new tests cover the happy path for a singleFunctionTooland a single dict tool. However, the tests are missing edge cases: passingNone, passing an empty list, and passing a mixed list that includes both single tools and sequences should all be validated. The existing tests may already cover some of these, but a test forNoneinput after the signature change (nowAny | Noneinstead ofSequence[Any] | None) would be valuable to confirm the early return still works correctly withnormalize_toolsin the path.
Suggestions
- The type annotation
Sequence[Any] | Any | Nonecollapses to justAny. Consider a more precise union likeTool | Sequence[Tool] | None(with an appropriateTooltype alias) to provide meaningful type information to callers. - The
if not tools:guard on line 440 runs beforenormalize_tools. If a caller ever passes a single falsy tool object (e.g., an empty dict{}), it would be silently swallowed. Consider moving the emptiness check afternormalize_tools:tools_list = normalize_tools(tools); if not tools_list: return []. - The type hint
Sequence[Any] | Any | Noneis technically redundant sinceAnyalready subsumes bothSequence[Any]andNone. Consider using a more precise union (e.g.,Sequence[Tool] | Tool | None) if concrete tool types are available, to preserve type-checker value. - Add a test for
_prepare_tools_for_openai(None)to verify the early return still works correctly now thatnormalize_toolsis in the code path. Ifnormalize_tools(None)is called before theif not toolsguard, this could raise an error. - Add a test passing a single string or other non-tool type to verify
normalize_toolshandles or rejects invalid input gracefully. - Consider verifying the
parametersandstrictfields intest_prepare_tools_for_openai_single_function_toolto match the assertion depth of existing tests liketest_prepare_tools_for_openai_with_function_tool.
Automated review by moonbox3's agents
python/packages/core/agent_framework/openai/_responses_client.py
Outdated
Show resolved
Hide resolved
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
Fixes a bug in the Python OpenAIResponsesClient tool-preparation path where passing a single tool object (instead of a list) could be iterated incorrectly, leading to mis-parsed tool definitions or runtime errors.
Changes:
- Normalize
toolsinput inOpenAIResponsesClient._prepare_tools_for_openaivianormalize_tools(...)before iteration. - Widen
_prepare_tools_for_openai’s accepted input type to allow single-tool inputs. - Add unit tests covering single
FunctionTooland single dict-tool inputs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/core/agent_framework/openai/_responses_client.py | Normalizes tool inputs before iteration to correctly handle single-tool values. |
| python/packages/core/tests/openai/test_openai_responses_client.py | Adds regression tests ensuring single-tool inputs are treated as one tool, not iterated. |
python/packages/core/tests/openai/test_openai_responses_client.py
Outdated
Show resolved
Hide resolved
python/packages/core/agent_framework/openai/_responses_client.py
Outdated
Show resolved
Hide resolved
- Use precise type annotation matching normalize_tools/OpenAIChatClient signature instead of collapsed Sequence[Any] | Any | None - Move emptiness guard after normalize_tools() call so single falsy tool objects are not silently swallowed - Import ToolTypes for the type annotation - Expand test_prepare_tools_for_openai_single_function_tool assertions to verify parameters, strict, and parameter schema fields - Add test_prepare_tools_for_openai_none to verify None input returns [] Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
moonbox3
left a comment
There was a problem hiding this comment.
Automated Code Review
Reviewers: 3 | Confidence: 89%
✓ Correctness
The diff refactors
_prepare_tools_for_openaito normalize the tools list before checking emptiness, which is a correctness improvement over the old code that checked the raw input's truthiness (e.g., an empty list vs. None vs. a single falsy-looking object). The type signature is tightened usingToolTypes, and new tests cover bothNoneinput and deeper schema assertions. No bugs, race conditions, or incorrect API usage detected.
✓ Security Reliability
This diff is a minor refactor that narrows type annotations on
_prepare_tools_for_openaiand reorders the call tonormalize_toolsso it happens before the emptiness check. The change is behaviorally equivalent for all practical inputs and introduces no new security or reliability concerns. No secrets, injection vectors, resource leaks, or unsafe deserialization patterns are present. The new tests appropriately cover theNoneinput case and validate tool schema properties.
✓ Test Coverage
The diff refactors
_prepare_tools_for_openaito callnormalize_toolsbefore the emptiness check and broadens the type signature to explicitly acceptToolTypes,Callable, and sequences thereof. Test coverage is improved with enhanced assertions on the FunctionTool test and a newNone-input test. However, there are notable gaps: no test for an empty list[](which exercises the early-return path differently thanNone), no test for passing a bareCallable(now explicitly in the type signature but never tested), and no test for a mixed-type sequence (e.g., a FunctionTool alongside a dict tool).
Suggestions
- Minor: consider adding a test for an empty list input (
[]) to_prepare_tools_for_openaialongside theNonetest, to ensurenormalize_toolshandles both empty cases consistently. - Add a test for empty list input (
_prepare_tools_for_openai([])) to verify the early-return path whennormalize_toolsreturns an empty list. - Add a test passing a bare callable (plain function, not wrapped in
FunctionTool) sinceCallable[..., Any]is now explicitly in the type signature — verify it is correctly converted. - Add a test for a mixed-type sequence (e.g.,
[function_tool, dict_tool]) to confirm heterogeneous tool lists are handled correctly after the refactor.
Automated review by moonbox3's agents
Motivation and Context
When a single tool (e.g., a
FunctionToolor a dict) was passed directly toOpenAIResponsesClientinstead of wrapped in a list,_prepare_tools_for_openaiwould iterate over the object's fields/characters instead of treating it as one tool. This caused silent misinterpretation of tool definitions and runtime failures.Fixes #4304
Description
The root cause was that
_prepare_tools_for_openaiiterated directly over itstoolsparameter assuming it was always a sequence, but callers could pass a single tool object. The fix uses the existingnormalize_toolsutility to coerce a single tool into a one-element list before iteration, and widens the type signature toSequence[Any] | Any | Noneto reflect the accepted inputs. Two new tests verify that both a singleFunctionTooland a single dict tool are correctly normalized into a one-element list of prepared tools.Contribution Checklist