-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add ability to remove tools #1322
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
feat: add ability to remove tools #1322
Conversation
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 think this is very reasonable. @Kludex opinions?
It seems something is missing, since the user doesn't have access to the underlying How is the user supposed to remove a tool? |
I was under the impression that one could obtain it from doing It feels natural to me that this code should exist, as |
That's a significant objection. |
Then, as a maintainer, do you have thoughts on how you would rather enable this behavior? I am not as familiar with your project. It seems reasonably clear to me that a user may want to alter the tools which are available to the MCP, in a way that is not solely static. I don't see why the tool manager must be private, nor why we could not surface some of these methods through the API. |
I think the current implementation (besides my comment about the exceptions) makes sense. It's just incomplete. Is the idea here to disable the tool for the whole app lifecycle or for the session? Does it even makes sense to have it disable for the app lifecycle, since the app may be running in multiple machines? I guess we want to expose a method you can call from the Context object anyway. |
I don't think ToolManager should be exposed. |
In my use case, it would actually be most ideal if a tool could be enabled or disabled for specific sessions (meaning, differing tools for differing consumers of a single server). I wanted to downscope this, as this is certainly more complicated than I feel I am seeking to implement right now. My understanding is that currently In my use case, I want users who are hosting their own servers (instances of the application) to be able to decide if they want to allow certain tools to be available to the consumers of their server. The best way to do this currently is with conditional declaration, which I view to be quite heavy handed. If we do not expose |
Oh, I now see that python-sdk/src/mcp/server/fastmcp/server.py Line 121 in 47d35f0
|
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.
Chiming in here, I think this is indeed the missing piece:
Oh, I now see that
FastMCP
exposes a thin wrapper aroundadd_tool
. Any objections to me doing the same forremove_tool
?python-sdk/src/mcp/server/fastmcp/server.py
Line 121 in 47d35f0
class FastMCP(Generic[LifespanResultT]):
python-sdk/src/mcp/server/fastmcp/server.py
Line 343 in 7629fe6
def add_tool( |
Instead of exposing ToolManager
(which I agree we shouldn't), having an utility on FastMCP
to remove_tool
s makes sense to me without adding huge complexity.
Would you have bandwidth to implement that @brandonspark?
Add a remove_tool method to the FastMCP class that allows tools to be dynamically removed from a server at runtime. This enables use cases where tools should only be available based on initialization state or user actions. The implementation includes: - remove_tool() method on FastMCP class that wraps the existing ToolManager.remove_tool() functionality - Comprehensive test coverage for tool removal scenarios - Proper error handling when attempting to remove non-existent tools 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Github-Issue: modelcontextprotocol#1322
Hey @brandonspark I've pushed up the changes that @felixweinberger requested to help move the PR across the line since the changes were quite simple. |
Dismissing my own review to check out the updates
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.
LGTM, thanks for updating @maxisbey
Any thoughts on when this will make it into a release? |
Should be in the next release on Thursday. |
Motivation and Context
I am interested in being able to dynamically have a server offer only certain tools based on information received during its initialization. I could conditionally declare the tool, for instance:
but I perceive this to be relatively gross. I would prefer to be able to remove a tool within the API.
How Has This Been Tested?
I can achieve comparable behavior by inserting an
if
before defining a function with themcp.tool()
decorator.Breaking Changes
No.
Types of changes
Checklist
Additional context