-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Added mcp tool filtering and unit test #854
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
Conversation
Hi @DanieleMorotti! 👋 What a coincidence! I was also working on the exact same feature and just submitted PR #861. It's really interesting that we both identified this need and decided to tackle it simultaneously - great minds think alike! 😄 I really appreciate your initiative in addressing issues #830 and #851. After looking at both approaches, I'd love to get your thoughts on the different implementation strategies we've taken. From what I can see, your solution focuses on server-level filtering (which is definitely the right direction!). In my implementation, I tried to provide a bit more flexibility by offering a dual-layer filtering approach: Server-level filtering (similar to your approach): server = MCPServerStdio(
params={...},
allowed_tools=["read_file", "write_file"],
excluded_tools=["delete_file"]
) Agent-level filtering (additional layer): agent = Agent(
name="Assistant",
mcp_servers=[server1, server2],
mcp_config={
"allowed_tools": {
"server1": ["tool1", "tool2"]
},
"excluded_tools": {
"server2": ["dangerous_tool"]
}
}
) The idea is that server-level filtering controls what tools the server exposes, while agent-level filtering allows fine-grained control across multiple servers without modifying server configurations. I also included support for both What do you think about this approach? I'd really value your feedback - maybe there are aspects of your implementation that could improve mine, or perhaps we could combine the best of both approaches? Would you be interested in taking a look at PR #861 and sharing your thoughts? I'm definitely open to collaboration and would love to hear your perspective! 🙏 |
Hi @devtalker ! Thanks for your detailed reply and the work you put into PR #861 , it's indeed a funny coincidence we tackled this simultaneously! 😄 You're exactly right, my approach was focused purely on server-level filtering. However, your idea of adding flexibility at the agent-level certainly provides more granular control, which could be beneficial in some scenarios. I'm not 100% convinced about having both allowed_tools and excluded_tools simultaneously. It may be better to provide a single option, but that's only my opinion. In any case, let's wait to hear @rm-openai thoughts, in order to adopt the best approach to move forward. |
Hi @DanieleMorotti, Thanks again for the feedback and being open to chat about it! 😊 I get where you're coming from with For example:
It’s especially helpful when working with multiple servers or evolving configs over time — no need to rewrite everything just to tighten things up a bit. Of course, we could go with just one option if needed, but I’d lean toward keeping both unless there's a strong reason not to. I’ll be looking forward to hearing what @rm-openai thinks too — hopefully they can help us decide the best way forward! |
@DanieleMorotti @devtalker my personal opinion is a mix of your approaches:
|
@rm-openai Thank you for the feedback! 😊 I agree with your suggestions. I'll update my PR to incorporate these changes. |
I’m closing this PR, even though I opened it first, as @devtalker 's solution appears to be more complete. |
Summary
Considering multiple requests to add this feature #830, #851 , I added a parameter to MCP servers to filter out some tools.
Let me know if you want to implement something in a different way and if I have to update the documentation.
Test plan
I added a unit test to check that only the allowed tools are returned by the server.
Issue number
It should close #830 and #851 .
Checks
make lint
andmake format