Skip to content

Add fine-grained control for MCP server(s) provided tools #1434

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

Merged
merged 1 commit into from
May 6, 2025

Conversation

mariofusco
Copy link
Contributor

This is an implementation for #1409

At the moment the McpToolBox annotation is only used to limit the MCP servers that a AI service can use. In other words if you have configured the MCP servers A, B and C, the following service:

@RegisterAiService
public interface MyService {

    @McpToolBox({"A", "C"})
    String chat(String query);
}

will be provided only for tools in the server A and C, but not B. If the McpToolBox annotation is present with no values or is not present at all it will behave in the same way, automatically providing all the tools for all the configured MCP servers. I did this to keep backward compatibility, but in reality what I would like to do is giving access to the MCP provided tools only to method annotated with McpToolBox, only the named ones or all if used without values. Any feedback on this is welcome.

I implemented this feature purely on the extension side and for this reason I had add to add a QuarkusToolProviderRequest and a QuarkusMcpToolProvider, but maybe we may this also in LangChain4j, so I could move the changes that I introduced with those classes directly there? /cc @dliubarskyi

Finally I'm struggling to find a way to properly test this feature, so any suggestion about this will be also very welcome.

/cc @jmartisk @geoand

@mariofusco mariofusco requested a review from a team as a code owner April 15, 2025 16:59
@mariofusco mariofusco marked this pull request as draft April 15, 2025 16:59
@geoand
Copy link
Collaborator

geoand commented Apr 16, 2025

This is very interesting! I can definitely this being useful.

As for testing, maybe @jmartisk has ideas?

@dliubarskyi
Copy link
Contributor

@mariofusco nice! For the upstream, perhaps we can add a programmatic config on the McpToolProvider level which will keep only specified tools or exclude only specified tools, WDYT?

@jmartisk
Copy link
Collaborator

jmartisk commented Apr 16, 2025

Yea I'd say we could change the upstream McpToolProvider to allow some filtering of the 'allowed' MCP clients instead of creating a new Quarkus-specific class, no? Like, let its constructor accept a Predicate<McpClient> and Quarkus would implement this predicate that performs the filtering. It would seem cleaner than further straying away from the upstream. We wouldn't need QuarkusToolProviderRequest then either.

@jmartisk
Copy link
Collaborator

jmartisk commented Apr 16, 2025

The testing side will be a bit tricky. On the upstream langchain4j side, we have full-fledged MCP servers running using JBang, but here, we only have a mock MCP server that is exposed in the form of a REST endpoint - we might want to switch to doing something more similar to the upstream in the future... But with the current approach, I could imagine adding two more mock MCP servers that are simpler than MockHttpMcpServer and only offer a specific tool. But I guess you would also need a mock ChatLanguageModel (unless we want the tests to use an actual one) that receives the prompt along with a list of tools and it would verify that only the 'allowed' tools were passed to it. It's not super simple but I don't know if there's anything that is simpler yet provides a lot of value.

@mariofusco
Copy link
Contributor Author

Yea I'd say we could change the upstream McpToolProvider to allow some filtering of the 'allowed' MCP clients instead of creating a new Quarkus-specific class, no? Like, let its constructor accept a Predicate<McpClient> and Quarkus would implement this predicate that performs the filtering. It would seem cleaner than further straying away from the upstream. We wouldn't need QuarkusToolProviderRequest then either.

@jmartisk @dliubarskyi I'm trying to follow this suggestion and more in general to move (in a more general way, e.g. introducing that predicate suggested by Jan) the logic that I implemented in the QuarkusMcpToolProvider back in langchain4j in order to get rid of it and of the QuarkusToolProviderRequest too, but I'm having hard time doing so and at this point I believe that this feature is entirely quarkus-specific and doesn't fit langchain4j use cases at all, for a twofold reason:

  1. In reality I think that the problem doesn't exist at all in langchain4j. There the user programmatically creates a McpToolProvider with the list of MCP clients he wants to use. He normally has a different tool provider for each AI service, so it is already possible to define the list of clients that should be used for a specific service. Conversely in quarkus-langchain4j there is only one single tool provider automatically created here and shared among all AI services, which is also the reason why I decided to put the list of the names of the MCP clients used by a specific service in the QuarkusToolProviderRequest.
  2. Filtering the McpClient s to be used for a specific request, for instance through a Predicate<McpClient>, won't be enough to cover this use case for the simple fact that they don't have a name in langchain4j. The McpClient's name is purely a quarkus feature, mostly deriving from how they are configured in the application.properties. Adding a (optional?) name to the langchain4j McpClient interface is also doable, but it seems to me a bit artificial and not very necessary to langchain4j needs alone.

WDYT? Is there anything that I could implement in langchain4j anyway or do we want to keep this only on the quarkus extension side? /cc @geoand

@dliubarskyi
Copy link
Contributor

@mariofusco I am not sure about Predicate<McpClient>, but I was suggesting to have something like this:

McpToolProvider.builder()
.mcpClients(...)

.includeTools("calc", "weather") // either by name
.includeTools(Predicate<ToolSpecification>) // or by predicate

.excludeTools("cmd") // either by name
.excludeTools(Predicate<ToolSpecification>) // or by predicate

.build()

Perhaps we are talking about 2 different things here?
My concern is that some MCP servers expose too many tools (e.g., GitHub's has 26) and there is no way to control this...

@dliubarskyi
Copy link
Contributor

My bad, I have misread the PR description:

@RegisterAiService
public interface MyService {

    @McpToolBox({"A", "C"})
    String chat(String query);
}

I wrongly assumed that A and C are names of tools...

@mariofusco
Copy link
Contributor Author

My bad, I have misread the PR description:

@RegisterAiService
public interface MyService {

    @McpToolBox({"A", "C"})
    String chat(String query);
}

I wrongly assumed that A and C are names of tools...

A and C in my example are the name of the MCP clients as they have been configured in the quarkus application.properties file. As I wrote MCP clients don't have a name in langchain4j so it is a bit difficult to map this feature there (and probably it doesn't make much sense).

Filtering the tools provided by a single MCP server also makes lot's of sense tbh, I'm also seeing some of them providing too many tools, but I believe this could be something orthogonal e not strictly dependent on what proposed here. @dliubarskyi I can also start working on this second feature if you agree and see some value in this.

@dliubarskyi
Copy link
Contributor

I'm also seeing some of them providing too many tools, but I believe this could be something orthogonal e not strictly dependent on what proposed here.

Sure, it is a separate issue

@dliubarskyi I can also start working on this second feature if you agree and see some value in this.

I think it is a metter of time before someone will ask for this feature, but there is also no rush and I would finalize 1.0 before starting new features

@mariofusco
Copy link
Contributor Author

@jmartisk @geoand According to what also discussed with @dliubarskyi this feature doesn't make much sense in vanilla langchain4j, but I still believe it is useful here in the extension, so I would keep developing it here. Please let me know if you agree and in this case I will try to add at least one test for this.

@geoand
Copy link
Collaborator

geoand commented Apr 30, 2025

I believe this is a useful feature

@dliubarskyi
Copy link
Contributor

@mariofusco seems that users want to filter MCP tools as well as modify their descriptions: devoxx/DevoxxGenieIDEAPlugin#670

@mariofusco mariofusco force-pushed the mcp-tool-box branch 2 times, most recently from 97a5da9 to 386ceaf Compare April 30, 2025 17:49
@mariofusco
Copy link
Contributor Author

The testing side will be a bit tricky. On the upstream langchain4j side, we have full-fledged MCP servers running using JBang, but here, we only have a mock MCP server that is exposed in the form of a REST endpoint - we might want to switch to doing something more similar to the upstream in the future... But with the current approach, I could imagine adding two more mock MCP servers that are simpler than MockHttpMcpServer and only offer a specific tool. But I guess you would also need a mock ChatLanguageModel (unless we want the tests to use an actual one) that receives the prompt along with a list of tools and it would verify that only the 'allowed' tools were passed to it. It's not super simple but I don't know if there's anything that is simpler yet provides a lot of value.

@jmartisk I added tests trying to follow your suggestions, thanks a lot for them. Please give it a look.

@mariofusco
Copy link
Contributor Author

@mariofusco seems that users want to filter MCP tools as well as modify their descriptions: devoxx/DevoxxGenieIDEAPlugin#670

Nice, but that's a filter of tools in the single MCP server. As discussed this is an orthogonal (and complementary) feature and can be implemented directly in langchain4j. I will give this a try.

@mariofusco
Copy link
Contributor Author

The last doubt I have (and the reason why I'm still leaving this as a draft for now) is how to express the fact that I may NOT want any MCP client for a specific AI service. At the moment annotating a service with @McpToolBox without any value or not annotating it all has the same effect of providing the service with all configured MCP clients. I did this to preserve backward compatibility, but I'm not sure this is a good idea. The 2 alternatives I see are:

  1. By default a AI service doesn't use any MCP clients. You need to annotate it with @McpToolBox, specifying which clients you want to use, or not providing any value if you want to use them all.
  2. By default a AI service uses all available MCP clients as it happens now. We could have some special value for the @McpToolBox annotation to say that we don't want any of them, something like @McpToolBox("none").

Personally I found option 1. much clearer and less error prone, wdyt? Do you see any other possibility?

@geoand
Copy link
Collaborator

geoand commented May 2, 2025

Thanks a lot @mariofusco. I'm actually torn on this.

On one hand option 1 is closer to what the experience with Claude Desktop or Goose is like, but on the other hand option 2 is in line with what @AiService(tools) and @ToolBox do.

@mariofusco
Copy link
Contributor Author

On one hand option 1 is closer to what the experience with Claude Desktop or Goose is like, but on the other hand option 2 is in line with what @AiService(tools) and @ToolBox do.

Can you please clarify why you wrote that option 2. is in line with what @ToolBox do? If I understand correctly a AI service doesn't use any internal tool by default and you explicitly have to annotate the service with the tools that you want to use. In this sense I believe that @ToolBox behaviour is much more similar to what I'm suggesting in option 1.: a service would be opt-out by default in using MCP tools and you will have to explicitly opt-in to enable them. Am I missing something?

@geoand
Copy link
Collaborator

geoand commented May 2, 2025

Oops, I reversed the order :).

My point is that I don't really have a strong opinion, but I guess if I were forced to choose, I would go with 1

@mariofusco
Copy link
Contributor Author

My point is that I don't really have a strong opinion, but I guess if I were forced to choose, I would go with 1

Agreed, I will go with 1.

@mariofusco mariofusco marked this pull request as ready for review May 2, 2025 12:49
@mariofusco
Copy link
Contributor Author

@geoand @jmartisk I think I implemented all that was discussed, please review.

@geoand
Copy link
Collaborator

geoand commented May 2, 2025

Thanks!

I'll let @jmartisk review this as it directly touches his work

This comment has been minimized.

This comment has been minimized.

Copy link

quarkus-bot bot commented May 5, 2025

Status for workflow Build (on pull request)

This is the status report for running Build (on pull request) on commit 530abf6.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@geoand geoand requested a review from jmartisk May 5, 2025 17:29
@jmartisk
Copy link
Collaborator

jmartisk commented May 6, 2025

Not completely sure whether it wouldn't be cleaner to just let the user to create their own tool providers for this purpose (if they want to allow just specific MCPs for a specific AI service/method, create a specific tool provider that uses only the desired clients). An annotation looks simpler, but look at how much complexity it is adding. Anyway, I don't object, if others find it useful, then so be it.

@geoand geoand merged commit 4742e53 into quarkiverse:main May 6, 2025
72 checks passed
@mariofusco
Copy link
Contributor Author

Not completely sure whether it wouldn't be cleaner to just let the user to create their own tool providers for this purpose (if they want to allow just specific MCPs for a specific AI service/method, create a specific tool provider that uses only the desired clients). An annotation looks simpler, but look at how much complexity it is adding. Anyway, I don't object, if others find it useful, then so be it.

I totally see your point about the added complexity, but under a users point of view I also believe that a very small fraction of them will decide to create their own ToolProvider and they would prefer the simplicity of this annotation. Also what I don't like of the current situation is that once you have configured one or more MCP server, each and every AI service will use all of them by default.

Anyway I'm also not strongly opinionated on this and I will let @geoand and @dliubarskyi to take the final decision.

@geoand
Copy link
Collaborator

geoand commented May 6, 2025

Let's add this for now and see how it goes

@mariofusco mariofusco deleted the mcp-tool-box branch May 6, 2025 14:01
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.

4 participants