Skip to content

Add MCP Client tool predicate for filtering the MCP tools #3901

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ilayaperumalg
Copy link
Member

  • Introduce MCP Sync/Async client BiPredicate interface as a tool filter for the MCP Sync/Async ToolCallbackProvider to use when filtering the MCP tools
  • Update MCP ToolCallbackAutoConfiguration to use these BiPredicate beans when defined (default is to allow all)
  • Add test verifying the tool filter configuration on both sync and async toolcallback provider auto-configuration
  • Update the unit tests for the MCP toolcallback provider

 - Introduce MCP Sync/Async client BiPredicate interface as a tool filter for the MCP Sync/Async ToolCallbackProvider to use when filtering the MCP tools
 - Update MCP ToolCallbackAutoConfiguration to use these BiPredicate beans when defined (default is to allow all)
 - Add test verifying the tool filter configuration on both sync and async toolcallback provider auto-configuration
 - Update the unit tests for the MCP toolcallback provider

Signed-off-by: Ilayaperumal Gopinathan <[email protected]>
@@ -51,16 +56,21 @@ public class McpToolCallbackAutoConfiguration {
@Bean
@ConditionalOnProperty(prefix = McpClientCommonProperties.CONFIG_PREFIX, name = "type", havingValue = "SYNC",
matchIfMissing = true)
public SyncMcpToolCallbackProvider mcpToolCallbacks(ObjectProvider<List<McpSyncClient>> syncMcpClients) {
public SyncMcpToolCallbackProvider mcpToolCallbacks(ObjectProvider<McpSyncClientBiPredicate> syncClientsToolFilter,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation of the syncClientsToolFilter parameter needs to be supplemented in the method notes here, the rest looks fine to me, thanks for your excellent work!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sunyuhan1998 Thanks for the review!

@@ -51,16 +56,21 @@ public class McpToolCallbackAutoConfiguration {
@Bean
@ConditionalOnProperty(prefix = McpClientCommonProperties.CONFIG_PREFIX, name = "type", havingValue = "SYNC",
matchIfMissing = true)
public SyncMcpToolCallbackProvider mcpToolCallbacks(ObjectProvider<List<McpSyncClient>> syncMcpClients) {
public SyncMcpToolCallbackProvider mcpToolCallbacks(ObjectProvider<McpSyncClientBiPredicate> syncClientsToolFilter,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better to have list of filters instead of a single filter as one can provide multiple filters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be better to have list of filters instead of a single filter as one can provide multiple filters.

If we are to accept multiple filters here, does that mean the final filtering result is the intersection of all the filters' results?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it would be an intersection of all the filters. But, we can only have tool specific filtering, not a client specific ones. Given the current implementation returns the ToolCallbacks for all the clients, for now, it wouldn't be possible to have any client specific filtering logic. We need a separate story to track this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can only have tool specific filtering, not a client specific ones. Given the current implementation returns the ToolCallbacks for all the clients, for now, it wouldn't be possible to have any client specific filtering logic. We need a separate story to track this.

I agree that we should track separately the issue of how to filter different tools for different clients.

Regarding the current filter design, I suggest that keeping a single filter should be sufficient, as this would keep our interface clear and simple. If users truly have the need for combined filtering, they can fully implement such composite filtering logic within syncClientsToolFilter. Even nesting multiple Predicate instances inside syncClientsToolFilter would be perfectly acceptable. — This is just my suggestion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great feedback! Thank you. I like the idea of using nested Predicate instances if there is a need for multiple filters.

Signed-off-by: Ilayaperumal Gopinathan <[email protected]>
 - Add McpClientMetadata record which contains MCP client/server meta data that can be used for filtering the toolcallbacks
   - This provides a convenient approach to handling just the metadata from the client
 - Update the auto-configuration and tests

Signed-off-by: Ilayaperumal Gopinathan <[email protected]>
@ilayaperumalg
Copy link
Member Author

Updated the PR to introduce MCP client metadata for filtering.

@@ -147,7 +146,8 @@ public ToolCallback[] getToolCallbacks() {
ToolCallback[] toolCallbacks = mcpClient.listTools()
.map(response -> response.tools()
.stream()
.filter(tool -> this.toolFilter.test(mcpClient, tool))
.filter(tool -> this.toolFilter.test(new McpClientMetadata(mcpClient.getClientCapabilities(),
mcpClient.getClientInfo(), mcpClient.initialize().block()), tool))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of client.initialize() will be replaced once we have access to the client.getCurrentInitializationResult() from modelcontextprotocol/java-sdk#424

@@ -133,7 +131,8 @@ public ToolCallback[] getToolCallbacks() {
.flatMap(mcpClient -> mcpClient.listTools()
.tools()
.stream()
.filter(tool -> this.toolFilter.test(mcpClient, tool))
.filter(tool -> this.toolFilter.test(new McpClientMetadata(mcpClient.getClientCapabilities(),
mcpClient.getClientInfo(), mcpClient.initialize()), tool))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of client.initialize() will be replaced once we have access to the client.getCurrentInitializationResult() from modelcontextprotocol/java-sdk#424

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants