feat - add unique check for tool names on run #1254
Conversation
gidonkessler
force-pushed
the
unique_tool_name
branch
from
88107f7 to
159b668
Compare
gidonkessler
force-pushed
the
unique_tool_name
branch
from
159b668 to
282a991
Compare
|
That's a good idea, and it occurs to me that we may need to add a optional prefix for mcp server. This allows users to not be blocked by this feature from using two mcp servers those have the same name tool. see: #1266 If #1266 is merged, I'd like to hint the users that they can set a prefix. Further, we can automatically help the user to set this prefix and tell: if user doesn't like it or if something goes wrong (some llm's have requirements for tool name, here we might only use some short random letters + numbers), users can configure the prefix manually. |
There was a problem hiding this comment.
@gidonkessler Sorry for the delay here! Can you please rebase on main and have a look at my comments?
| # TODO(Marcelo): We should check if the tool names are unique. If not, we should raise an error. | ||
| tool_defs: list[ToolDefinition] = await server.list_tools() | ||
|
|
||
| tool_name_counts: Counter[str] = Counter(tool_def.name for tool_def in tool_defs) |
There was a problem hiding this comment.
Can we also verify that the MCP server's tools don't clash with tools from other servers, as well as the user-defined tools? We can look at all of the function_tool_defs.
I suggest looking at the Agent._register_tool method that does this when adding a new user-defined tool.
Instead of using a Counter, I think we can turn function_tool_defs into a dict, and when iterating over the new tools, check if a key for that name already exists.
| ] | ||
| ) | ||
|
|
||
| class MockMCPServer(MCPServer): |
There was a problem hiding this comment.
I think we can simplify this and likely do away with so much mocking of implementation details that may change in the future if we instead test that an MCP tool name doesn't conflict with a user-defined tool, or another MCP server's tool. We can likely use the FastMCP server we define in tests/mcp_server.py and already use from (the latest version of this) test_mcp.py
I've never contributed to open source before but saw this todo thought I'd give it a try :)
Hopefully it looks close to what you guys were imagining and would appreciate some guidance on the test 🙏