Python: fix(core): treat a single mapping tool as one tool in merge_chat_options - #8732
Anish Mehta (anishmehta24) wants to merge 3 commits into
Conversation
GitHub Actions
Active
GitHub Actions
Active
agent-framework-automation
Bot
added
the
python
|
|
||
| def _as_tool_list(tools: Any) -> list[Any]: | ||
| """Tools as a list, treating a single mapping (e.g. {"type": "web_search"}) as one tool.""" | ||
| if isinstance(tools, Mapping) or not isinstance(tools, Iterable): |
There was a problem hiding this comment.
Anish Mehta (@anishmehta24) I would rather keep all tool-shape normalization inside normalize_tools instead of adding a second parsing path in merge_chat_options. _as_tool_list already diverges from normalize_tools for supported provider-native specs such as Pydantic BaseModel tools, which are iterable and get expanded into field/value tuples here. Could we route both sides through the existing normalization logic and add coverage for merging a list with one Pydantic provider tool?
There was a problem hiding this comment.
Dropped _as_tool_list and both sides of the merge now go through normalize_tools, so there's only one place that decides tool shapes. Added a test merging a Pydantic provider tool, inside a list and on its own, on either side. One thing to flag: normalize_tools wraps plain callables in a new FunctionTool each time, so the same undecorated function passed on both sides won't dedupe anymore. @tool-decorated ones still do. Happy to handle that if you think it matters.
GitHub Actions
Active
|
/review |
| # Add tools that aren't already present | ||
| merged_tools = list(base_tools) | ||
| for tool in value if isinstance(value, Iterable) else [value]: # type: ignore[reportUnknownVariableType] | ||
| merged_tools = normalize_tools(base_tools) |
There was a problem hiding this comment.
Anish Mehta (@anishmehta24) The callable dedupe regression you noted is material here. normalize_tools(base_tools) and normalize_tools(value) wrap the same undecorated function into two distinct FunctionTool objects, so tool not in merged_tools keeps both and providers can receive duplicate tool names. Please preserve identity across the merge or dedupe normalized tools by the repository’s tool-name rules, with a regression using the same plain callable on both sides.
There was a problem hiding this comment.
Fixed, merged tools are now deduped by tool name, so the same plain function on both sides ends up once. Added a regression for it.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
MAF Automated Review — Iteration 1
Result: No findings
Scope: full PR (2 commit(s)): 94758a589908, 810bed732feb
Model: gpt-5.6-sol-fast
Overview
This PR routes both sides of a populated tools merge through the canonical normalize_tools path, preventing mapping and Pydantic provider-tool specifications from being expanded into their iterable contents. New tests cover both merge directions and wrapped or unwrapped provider tools, while the normal Agent execution path independently enforces tool-name uniqueness before provider dispatch. No new Critical, High, or Medium defect remains after deduplication against the supplied unresolved feedback.
Reviewed the supplied pull-request change set across correctness, security/reliability, architecture, and failure behavior.
No publishable findings remained after source verification for this scope.
GitHub Actions
Active
| names = {name for t in merged_tools if (name := _get_tool_name(t))} | ||
| for tool in normalize_tools(value): | ||
| name = _get_tool_name(tool) | ||
| if tool in merged_tools or (name is not None and name in names): |
There was a problem hiding this comment.
Anish Mehta (@anishmehta24) This name-only dedupe also drops a distinct override tool that happens to share the base tool’s name, silently keeping the base implementation, schema, and approval settings instead. Elsewhere _append_unique_tools treats distinct same-name tools as an error. Could this only collapse the same original callable/tool case, or otherwise preserve the duplicate-name error?
Motivation & Context
merge_chat_optionsmergestoolswithfor tool in value if isinstance(value, Iterable) else [value]. A single tool given as a mapping, like a hosted tool{"type": "web_search"}, is iterable, so it gets spread into its keys:normalize_toolsalready treats a mapping as one tool; the merge didn't.Description & Review Guide
_as_tool_listhelper that wraps a mapping (or any non-iterable) in a list, used for both the base and override tools in the merge.Related Issue
No existing issue.
Contribution Checklist
Test:
test_merge_chat_options_single_mapping_tool_is_not_spread_into_keysintests/core/test_types.py, both directions; it fails on main.pytest packages/core/tests/core/test_types.pypasses, and ruff and the repo's pre-commit checks are clean.