-
Notifications
You must be signed in to change notification settings - Fork 1.9k
fix(mcp): Initialize tool_name_prefix in MCPToolse #2823
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
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Summary of Changes
Hello @shsha4, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request enhances the McpToolset
by adding a tool_name_prefix
attribute. This allows users to specify a prefix for all tools managed by a McpToolset
instance, which is crucial for avoiding naming conflicts and maintaining better organization, especially in complex environments where multiple toolsets might be in use. The change involves modifications to the McpToolset
class and its configuration, along with the addition of dedicated unit tests to ensure functionality.
Highlights
- New Feature: Tool Name Prefixing: Introduced a
tool_name_prefix
attribute toMcpToolset
andMcpToolsetConfig
to allow adding a configurable prefix to tool names within a toolset. This helps in preventing naming collisions and improving organization. - Configuration Integration: The
tool_name_prefix
can now be configured viaMcpToolsetConfig
and is properly handled duringMcpToolset
initialization and when loading configurations. - Enhanced Test Coverage: A new unit test file (
tests/unittests/tools/test_mcp_toolset.py
) has been added, includingtest_mcp_toolset_with_prefix
, to validate the correct application of the tool name prefixing functionality.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Response from ADK Triaging Agent Hello @shsha4, thank you for creating this PR! It looks like you haven't signed the Contributor License Agreement (CLA) yet. Please make sure to sign it, as we can't accept contributions without it. You can find more information in the "checks" section of this pull request. Thanks! |
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.
Code Review
This pull request correctly introduces the tool_name_prefix
functionality to McpToolset
and McpToolsetConfig
, allowing for better organization and avoidance of tool name collisions. The changes are consistently applied, and a new unit test is added to verify the feature. My review includes one suggestion to improve the robustness of the new test to more accurately reflect its intent.
toolset = McpToolset( | ||
connection_params=mock_connection_params, | ||
tool_name_prefix="my_prefix", | ||
) | ||
|
||
# Replace the internal session manager with our mock | ||
toolset._mcp_session_manager = mock_session_manager | ||
|
||
# Get the tools from the toolset | ||
tools = await toolset.get_tools() | ||
|
||
# The get_tools method in McpToolset returns MCPTool objects, which are | ||
# instances of BaseTool. The prefixing is handled by the BaseToolset, | ||
# so we need to call get_tools_with_prefix to get the prefixed tools. | ||
prefixed_tools = await toolset.get_tools_with_prefix() | ||
|
||
# Assert that the tools are prefixed correctly | ||
assert len(prefixed_tools) == 2 | ||
assert prefixed_tools[0].name == "my_prefix_tool1" | ||
assert prefixed_tools[1].name == "my_prefix_tool2" | ||
|
||
# Assert that the original tools are not modified | ||
assert tools[0].name == "tool1" | ||
assert tools[1].name == "tool2" |
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.
This test correctly verifies that get_tools_with_prefix()
returns prefixed tools. However, the assertion that 'the original tools are not modified' isn't being tested as robustly as it could be. Because McpToolset.get_tools()
creates new tool instances on each call, the test doesn't guarantee non-mutation if get_tools()
were to be changed to cache instances in the future. The suggested change ensures get_tools_with_prefix()
operates on the same tool instances that are later checked for mutation, making the test more robust.
toolset = McpToolset(
connection_params=mock_connection_params,
tool_name_prefix="my_prefix",
)
# Replace the internal session manager with our mock
toolset._mcp_session_manager = mock_session_manager
# Get the original tools first to serve as the ground truth
original_tools = await toolset.get_tools()
# Mock get_tools to return the same instances, to test for mutation.
toolset.get_tools = AsyncMock(return_value=original_tools)
# Get the prefixed tools
prefixed_tools = await toolset.get_tools_with_prefix()
# Assert that the tools are prefixed correctly
assert len(prefixed_tools) == 2
assert prefixed_tools[0].name == "my_prefix_tool1"
assert prefixed_tools[1].name == "my_prefix_tool2"
# Ensure they are new objects (copies)
assert prefixed_tools[0] is not original_tools[0]
assert prefixed_tools[1] is not original_tools[1]
# Assert that the original tool instances were not modified
assert original_tools[0].name == "tool1"
assert original_tools[1].name == "tool2"
When applying this pull request, a "Tool not found" error will occur if the MCP does not have the prefix. For improved agent behavior, the prefix should be included to resolve naming conflicts in the declaration. However, when making a request to the MCP, the agent should use the tool's name without the prefix. Check this PR as example |
Hi, the only change that this PR needs to be merged is the following:
|
Hi @shsha4, can you apply the changes that I suggested above and push them into this PR? I don't have the permissions to do so :) |
@alberto-mate |
Thanks for the contribution! |
5e21bc2
to
1cd9ff4
Compare
@seanzhou1023 |
please fix the test failure. follow other mcp test to skip mcp tests when python version is less than 3.10 |
c987331
to
a33b54a
Compare
- Change self.name to self._mcp_tool.name in call_tool invocation - Ensures MCP server receives unprefixed tool name - Addresses review comment from alberto-mate
- Change tool_name_prefix default from empty string to None - Update type annotation to Optional[str] in both McpToolset and McpToolsetConfig - Addresses review comment from seanzhou1023
- Add Python version check to test_mcp_toolset.py - Follow existing MCP test pattern with pytestmark skipif decorator - Add import error handling with dummy classes for test collection - Addresses review comment from seanzhou1023
a33b54a
to
76e1122
Compare
@seanzhou1023 |
Description
This change introduces a tool_name_prefix attribute to McpToolset and McpToolsetConfig. This allows for adding a prefix to the
names of all tools within the toolset, which can help avoid naming collisions and provide better organization.
The implementation involves updating the McpToolset's init and from_config methods to handle the new tool_name_prefix and
adding the corresponding field to McpToolsetConfig.
Testing Plan
A new unit test file has been added to ensure the functionality works as expected.
tests/unittests/tools/test_mcp_toolset.py
:retrieved from the toolset.
Related Issue