-
Notifications
You must be signed in to change notification settings - Fork 224
Add MCP server tool filtering support to agents-js #164
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
🦋 Changeset detectedLatest commit: 161965f The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@seratch can you review please? |
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.
Thanks for sending this! I haven't verified the actual behavior yet but let me share a few quick comments
thanks for the comments, @seratch! esp. the point about moving filtering logic that makes sense for keeping the server implementation focused on transport. |
Thanks @seratch. That makes sense and i’ve removed the |
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.
Sorry, one more request from me
4dce2d5
to
47ae347
Compare
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.
Did thorough review and I found your current implementation actually does nothing for allowing/blocking tools. I've implemented draft version of change here: seratch@73eb37a Hope sharing this clarifies things!
examples/mcp/tool-filter-example.ts
Outdated
console.log('\nAttempting to write a file (should be blocked):'); | ||
result = await run( | ||
agent, | ||
'Create a file named test.txt with the text "hello"', |
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.
currently this is blocked either way because the mcp server has the access only to sample_files/
my diff changed this as well
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.
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.
thanks for sharing your draft, @seratch! You’re absolutely right, my plumbing additions never actually invoked the filter, so allow/block lists weren’t doing anything.
I’ve since merged your implementation from 73eb37a and used that as the foundation. Appreciate the clarity your diff brought to this.
One question: should we also update the test to explicitly include toolUseBehavior: 'stop_on_first_tool'
in the agent config? That setting ensures the run exits immediately with a "Tool \"write_file\" is blocked by MCP filter."
error rather than continuing or looping, which might make the filter behavior clearer and more predictable in test output.
happy to add that if helpful!
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.
Thanks for pointing this out. I agree the agent should behave in the way you mentioned for the scenario. Adding test patterns would be appreciated!
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.
Just added an integration test, i.e., mcpToolFilter.integration.test.ts
that spins up the stub FS server and verifies that list_directory
(allowed) works and write_file
(blocked) gets immediately rejected with the expected message using toolUseBehavior: 'stop_on_first_tool'
.
I also expanded the original unit tests to cover no filter, allow-only, block-only, async filters, thrown errors, and cache invalidation. This should give us better coverage across edge cases.
Lastly, I added a script to package.json
so the example can now be run via pnpm examples:tool-filter
.
Thoughts? Let me know if we should have anything else covered!
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.
Noticed that CI had failed on the merge commit (b5ff9ac) due to a type issue with modelResponses in the integration test. I fixed it by adding the correct ModelResponse[] type. It should be good now.
b73a090
to
5ab34ae
Compare
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.
The changes are now all good, but there are a few things to adjust 🙏
docs/src/content/docs/guides/mcp.mdx
Outdated
You can restrict which tools are exposed from each server. Pass either a static filter | ||
using `createMCPToolStaticFilter` or a custom function: | ||
|
||
```ts |
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 consistently use compiled code snippets in docs. can you do this?
- Add
examples/docs/mcp/tool-filter.ts
, which compiles and works - Add
import stdioExample from '../../../../../examples/docs/mcp/tool-filter.ts?raw';
and use it here
If you're unsure or don't have time for this, please just remove this change.
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.
Co-authored-by: Kazuhiro Sera <[email protected]>
Co-authored-by: Kazuhiro Sera <[email protected]>
…sts and server impl
- Moved tool filtering logic to agent/runner layer - Removed server-side filtering and context coupling - Updated test suite to reflect new behavior
Reverts prior tool filtering interface changes and updates corresponding tests.
* Handle function call messages with empty content * Resolve test failure * Create heavy-yaks-mate.md --------- Co-authored-by: Dominik Kundel <[email protected]>
… to getAllMcpTools
bb41f61
to
161965f
Compare
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
@vrtnis Thank you so much for working on this PR 👏 |
Thanks @seratch, appreciate the thoughtful reviews and guidance throughout. Learned a lot working through this one! 🙌 |
--------- Co-authored-by: Kazuhiro Sera <[email protected]>
Hey all - this PR brings a new tool filtering feature to our MCP integrations. This resolves #162 based on openai/openai-agents-python#861
You can now:
Use
createStaticToolFilter
for quick allow/block listsProvide a custom filter function to make dynamic decisions based on
runContext
Updated core API so
getAllTools
now accepts yourrunContext
, ensuring filters are applied correctlyDocumentation enhancements in
docs/guides/mcp.mdx
New example (
examples/mcp/tool-filter-example.ts
) to demonstrate both static and dynamic filteringComprehensive tests covering static lists, callable filters, multi-server hierarchies, and cache interactions
All tests pass and you can try it out by
pnpm -F mcp start:tool-filter