Python: Fix agent option merge to support dict-defined tools

[moonbox3]

Motivation and Context

When tools are defined as raw dictionaries (e.g. {"type": "function", "function": {"name": "..."}}), the option merge logic used getattr(t, "name", None) which always returned None for dicts, causing all dict-defined tools to be silently dropped as false duplicates.

Fixes #4303

Description

The root cause was that _merge_options assumed tools were always objects with a .name attribute. Dict-defined tools store their name nested under tool["function"]["name"], so getattr never found it and every tool resolved to None, collapsing them during deduplication. The fix introduces a _get_tool_name helper that handles both object-style and dict-style tool definitions, and uses it in the deduplication logic. Tests cover dict-only, mixed, and deduplication scenarios.

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

---

Note: PR autogenerated by moonbox3's agent

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[moonbox3]

Automated Code Review

Reviewers: 3 | Confidence: 92%

✓ Correctness

The diff introduces a _get_tool_name helper to support both object-style tools (with a .name attribute) and dict-style tools (OpenAI {"function": {"name": ...}} format) for deduplication in _merge_options. The implementation is correct and the tests cover the key scenarios: dict-only merging, deduplication, and mixed object/dict tool lists. No bugs, race conditions, or incorrect API usage detected.

✓ Security Reliability

The diff introduces a helper _get_tool_name to support both dict-based and object-based tool definitions during merge deduplication. The implementation is safe: it uses isinstance checks, safe .get() accessors, and getattr with a default. No injection risks, secrets, resource leaks, or unsafe deserialization. One minor pre-existing reliability concern carries forward: tools whose name resolves to None (malformed dicts, objects without .name) all collide in the deduplication set, meaning only one nameless tool from the override list will survive if a nameless tool already exists in the base. This is not introduced by the PR but is worth noting.

✗ Test Coverage

The new tests cover dict-tool merging, dict-tool deduplication, and mixed object+dict merging, which is good. However, the most important scenario — deduplication across object and dict tools with the same name — is not tested, even though enabling that cross-format dedup is the whole point of the _get_tool_name refactor. Additionally, _get_tool_name is imported in the test file but has no direct unit tests for its edge cases (malformed dicts, missing keys, non-dict function values). These are not blocking but represent meaningful coverage gaps.

Blocking Issues

• No test verifies that a dict tool and an object tool sharing the same name are deduplicated during merge. This is the core behaviour the refactor enables and must be covered.

Suggestions

• Pre-existing minor concern (not introduced by this PR): if _get_tool_name returns None for a tool without a name, that None enters existing_names, causing all subsequent unnamed tools to be treated as duplicates. Consider filtering None from existing_names or documenting this as intentional.
• Consider discarding None from existing_names (e.g., existing_names = {_get_tool_name(t) for t in result["tools"]} - {None}) so that nameless/malformed tools are never silently deduplicated against each other. This is a pre-existing issue but easy to fix here.
• Add direct unit tests for _get_tool_name edge cases: dict with no 'function' key, dict where 'function' is not a dict, dict where 'function' has no 'name', object with no 'name' attribute, and a non-dict/non-object input.
• Consider testing the behaviour when multiple tools have None names (i.e., _get_tool_name returns None for both) — currently they would be falsely deduplicated because None matches in the set.

---

Automated review by moonbox3's agents

[moonbox3]

Automated Code Review

Reviewers: 3 | Confidence: 92%

✓ Correctness

The change correctly prevents false deduplication of tools whose names cannot be extracted (i.e., _get_tool_name returns None). By removing None from the existing_names set, nameless tools in the override list are no longer incorrectly matched against nameless tools in the base list. The accompanying tests thoroughly cover edge cases for _get_tool_name and the deduplication logic. No correctness issues found.

✓ Security Reliability

The diff is a small, well-scoped reliability fix that prevents tools with no extractable name (where _get_tool_name returns None) from being falsely deduplicated during option merging. The fix correctly removes None from the dedup set so that nameless tools from the override are always preserved. Accompanying tests thoroughly cover edge cases of _get_tool_name and the deduplication behavior. No security or reliability concerns found.

✓ Test Coverage

The diff adds a small but important fix to exclude None from the deduplication set in _merge_options, and provides thorough test coverage for both the fix and the _get_tool_name helper. The core bug scenario (nameless tools falsely deduplicated) is directly tested, and edge cases for _get_tool_name are well covered. One minor gap: there is no test combining nameless and named tools in the same merge to verify both behaviors work simultaneously.

Suggestions

• Consider adding a combined scenario test where base contains both a nameless tool and a named tool, and override contains both a nameless tool and a duplicate-named tool. This would verify that nameless tools are preserved while named duplicates are still correctly deduplicated in a single merge call.

---

Automated review by moonbox3's agents

[markwallace-microsoft]

Python Test Coverage Report •

File	Stmts	Miss	Cover	Missing
packages/core/agent_framework
_agents.py	338	43	87%	435, 439, 491, 856, 892, 908, 984–987, 1048–1050, 1171, 1187, 1189, 1202, 1208, 1244, 1246, 1255–1260, 1265, 1267, 1273–1274, 1281, 1283–1284, 1292–1293, 1296–1298, 1306–1307, 1309, 1314, 1316
TOTAL	22181	2762	87%

Python Unit Test Overview

Tests	Skipped	Failures	Errors	Time
4684	247 💤	0 ❌	0 🔥	1m 19s ⏱️