Python: Support MCP tool binding by reference in declarative YAML - #5052
Closed
hashwnath wants to merge 2 commits into
Closed
Python: Support MCP tool binding by reference in declarative YAML#5052hashwnath wants to merge 2 commits into
hashwnath wants to merge 2 commits into
Conversation
…crosoft#4927) Add binding resolution for McpTool in AgentFactory._parse_tool(), matching the existing FunctionTool pattern. MCP tools can now be resolved either by explicit bindings list or by matching the tool name against factory bindings, allowing pre-built MCP tool dicts to be passed via AgentFactory(bindings=...) instead of requiring full connection details in every YAML file. Fixes microsoft#4927
Mark Wallace (markwallace-microsoft)
added
the
python
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes declarative YAML tool parsing so MCP tools can be resolved from AgentFactory(bindings=...) (by explicit bindings: entries or by tool name), avoiding accidental construction of invalid MCP configs when YAML omits url.
Changes:
- Add binding resolution logic for
McpToolinAgentFactory._parse_tool(), mirroring the existingFunctionToolbinding approach. - Add tests covering explicit binding resolution, name-based resolution, the #4927 regression scenario, and fall-through behavior when no binding matches.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| python/packages/declarative/agent_framework_declarative/_loader.py | Resolves MCP tool configs from factory bindings (explicit list first, then by tool name), otherwise falls back to existing dict-building behavior. |
| python/packages/declarative/tests/test_declarative_loader.py | Adds targeted tests verifying MCP binding-by-reference behavior and regression coverage for #4927. |
Contributor
|
hashwnath please have a look at the failing CI/CD items. Thanks. |
Cast bound_tool return value to the declared return type to satisfy mypy strict mode (warn_return_any). Add edge case tests for explicit binding priority and None bindings fallthrough.
Contributor
Author
|
Fixed the mypy issue! The other CI failures are from the orchestration tests broken on main, unrelated to the declarative changes here. |
Contributor
|
Peter Ibekwe (@peibekwe) can you have a look, please? |
Member
|
Closing this as we will spend time on the declarative package soon, the issue will remain open for that. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #4927
AgentFactory._parse_tool()ignoresself.bindingsfor theMcpToolcase, unlikeFunctionToolwhere bindings correctly resolve the callable. This causes pre-built MCP tool dicts passed viaAgentFactory(bindings={"product-mcp": mcp_tool})to be silently ignored — the factory always constructs the tool from YAML fields, resulting inserver_url: ''and a 500 error when nourlis specified in the YAML.This PR adds binding resolution for
McpToolin_parse_tool(), mirroring the existingFunctionToolpattern:tool_resource.bindingslist againstself.bindingsdicttool_resource.namedirectly againstself.bindingskeysIf a binding resolves, the pre-built tool dict is returned directly. If no binding matches, the existing dict-building logic runs unchanged.
Changes
_loader.py: Added binding-by-reference resolution in theMcpToolcase of_parse_tool()test_declarative_loader.py: Added 4 tests:allowedTools, no url)Test plan
server_url: '')