-
Notifications
You must be signed in to change notification settings - Fork 53
feat: Add MCP client tool #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Related strands-agents/agent-builder#4 |
- Add streamable_http transport option alongside stdio and sse - Support headers, timeout, sse_read_timeout, terminate_on_close, and auth parameters - Add comprehensive tests for streamable HTTP functionality - Update README with streamable HTTP example
elif isinstance(result, list): | ||
# If it's already a list of content items, use it directly | ||
content = result | ||
elif isinstance(result, dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make sure we support other types here like images?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Approving and leaving a room to team to take a look!
@@ -52,6 +52,7 @@ Strands Agents Tools provides a powerful set of tools for your agents to use. It | |||
- ⏱️ **Task Scheduling** - Schedule and manage cron jobs | |||
- 🧠 **Advanced Reasoning** - Tools for complex thinking and reasoning capabilities | |||
- 🐝 **Swarm Intelligence** - Coordinate multiple AI agents for parallel problem solving with shared memory | |||
- 🔌 **MCP Client** - Connect to any Model Context Protocol server and access remote tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Can we edit this to callout that we are allowing an agent to autonomously perform this connection and dynamic loading. This was brought up on a client call and, as expected, this was called out as being potentially dangerous.
This tool is useful, I want to prevent customers from accidentally doing something like the following instead of using the MCPClient approach in the SDK
agent = Agent(tools=[mcp_client, use_browser])
agent.tool.mcp_client("connect")
# Agent uses the browser, then connects and downloads tools from an attackers mcp_server.
This be be documentation, but I also have concerns with this tool being called mcp_client. I would bet that we will get customers asking us "should I use mcp_client tool or MCPClient" because of this.
# Suppress warnings from MCP SDK about unknown notification types | ||
# This is particularly useful for test servers that send non-standard notifications | ||
# Users can override this by setting their own logging configuration | ||
logging.getLogger("mcp.shared.session").setLevel(logging.ERROR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am using strands with mcp pinned at a particular version, how would I get a deprecation warning with this set to Error? Are we doing this elsewhere?
This forced suppression seems strange to me. Couldn't we just remove this and then if the user thinks it is noisy because they are using standard notifications override it themselves?
connection_id: str, | ||
tool_name: str, | ||
tool_spec: ToolSpec, | ||
mcp_client: Any, # Required: direct MCP client reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why Any?
tool_name: str, | ||
tool_spec: ToolSpec, | ||
mcp_client: Any, # Required: direct MCP client reference | ||
name_prefix: bool = True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: name prefix as this param doesn't seem clear to me
super().__init__() | ||
self.connection_id = connection_id | ||
self.original_tool_name = tool_name | ||
self.mcp_client = mcp_client # Direct client reference |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "# Direct client reference" does this add anything?
return {"toolUseId": tool_use_id, "status": "error", "content": [{"text": result["error"]}]} | ||
|
||
# Convert MCP result to Strands format | ||
content = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have https://github.com/strands-agents/sdk-python/blob/main/src/strands/tools/mcp/mcp_client.py#L262, is it possible to expose that so we can reuse it here rather than reimplement?
_connections[connection_id].last_error = last_error | ||
|
||
|
||
class MCPToolWrapper(AgentTool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit confused by the pattern here, we have MCPToolWrapper but we also have mcp_client
. And both can be loaded by an Agent class as MCPToolWrapper implements AgentTool and mcp_client is annotated with @tool
When should I use one and not the other? Should there just be one?
Edit: it looks like this may be a naming related confusion where this is an MCPTool that was generated by the MCPClient
Still, curious why we opted for the class based approach here but the annotation one below.
try: | ||
# Use the stored client | ||
result = config.mcp_client.call_tool_sync( | ||
tool_use_id=f"mcp_{connection_id}_{tool_name}_{uuid.uuid4().hex[:8]}", name=tool_name, arguments=tool_args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a length limitation here, my fear is that our uniqueness strategy of using uuid is no impacted by using the [:8] this may impact customer traceability
agent: Optional[Any] = None, # Agent instance passed by SDK | ||
) -> Dict[str, Any]: | ||
""" | ||
MCP client tool for connecting to any MCP server with simplified configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to above I think we need some serious editing here and callouts of the risks involved with using this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add an integration test as well
Description
This PR introduces a new MCP (Model Context Protocol) client tool for Strands Agents that provides a high-level interface for connecting to any MCP server with simplified configuration and enhanced functionality.
Key Features:
Usage Examples:
Related Issues
N/A
Documentation PR
N/A - Documentation is included in the README.md update
Type of Change
Testing
The implementation includes comprehensive unit tests covering:
Testing commands run:
hatch fmt --linter
✓hatch fmt --formatter
✓hatch test --all
✓Manual Testing
Key achievements:
Checklist
I have read the CONTRIBUTING document
I have added tests that prove my fix is effective or my feature works
I have updated the documentation accordingly
I have added an appropriate example to the documentation to outline the feature
My changes generate no new warnings
Any dependent changes have been merged and published
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.