Add UseMcpClient #1086
Add UseMcpClient #1086
Conversation
There was a problem hiding this comment.
The tests are in this project because it wasn't possible for me to mock the McpClient so I had to use integration tests.
There was a problem hiding this comment.
Can you elaborate on the challenges with mocking McpClient? It'd be good to understand and fix blockers.
There was a problem hiding this comment.
Not sure if those are the same challenges, but mocking McpClient used to confuse AI assistants (Claude, GitHub Copilot, etc.) as they couldn't figure it out and did some really odd stuff with some of the common mocking frameworks. I seem to remember coding up an initial example of doing it and that made it click for the model, but I don't know if they still struggle with the mocking.
Apologies, if there's an actual issue with mocking McpClient currently and I hope this doesn't come across in the wrong way. But it caught me off guard more than once, and I recall the mocking actually being a little tricky with 0.3.0-preview4.
There was a problem hiding this comment.
I would also like to understand this. The new code we're trying to test calls McpClient.CreateAsync directly, so the only way to mock it would be to adding indirection there for testing or to mock a lower level like the MCP transport level or HttpClient level.
Since the HttpClient is already pluggable, I recommend creating a new test class based on KestrelInMemoryTest rather than continuing to expand on SseServerIntegrationTestFixture. The SseServerIntegrationTestFixture is mostly the way it is because it's testing the legacy transport and I haven't gotten around to updating it.
KestrelInMemoryTest already has a MockLoggerProvider and XunitLoggerProvider wired up like all LoggedTests, and you can run multiple servers with the same in-memory Kestrel transport as demonstrated by OAuthTestBase. This allows you to define all your server-side and client-side code inside a single test method so we don't end up with a massive test server project with different code scattered about that are relevant to different tests far away in the source code.
There was a problem hiding this comment.
I meant that I have to test it in ModelContextProtocol.AspNetCore.Tests instead of ModelContextProtocol.Tests because all infrastructure to mock the http transport depends on AspNetCore. This misaligns with the src project where I'm intending to add the middleware.
There was a problem hiding this comment.
If we are OK with the test and src placement, I can go ahead and move to use @halter73's suggestion which is cleaner than my naive approach.
stephentoub
left a comment
stephentoub
left a comment
There was a problem hiding this comment.
Nice. Seems like this generally works out?
| /// Use this method as an alternative when working with chat providers that don't have built-in support for hosted MCP servers. | ||
| /// </para> | ||
| /// </remarks> | ||
| public static ChatClientBuilder UseMcpClient( |
There was a problem hiding this comment.
We should mark the public surface are for this as [Experimental]. We could do so either with "MEAI001", so that it matches with the MCP types on which its based (and in which case you wouldn't need the earlier suppression) or have some new "MCP003" or something.
There was a problem hiding this comment.
It would be nice to align this with the ID we end up using for McpServer members. cc @jeffhandley.
| /// </remarks> | ||
| public static ChatClientBuilder UseMcpClient( | ||
| this ChatClientBuilder builder, | ||
| HttpClient? httpClient = null, |
There was a problem hiding this comment.
It's interesting to consider what should be accepted here and whether that has any impact on the shape of the options we expose in the MCP library. We can't just accept an HttpClientTransport, as that's tied to a specific endpoint, but otherwise we effectively would want most of the options exposed on that, I think.
There was a problem hiding this comment.
Agreed. You wouldn't want to share the ITokenCache, but the remaining ClientOAuthOptions could make a lot of sense. Maybe we should move the TokenCache out of ClientOAuthOptions so we could flow the rest of it through.
Headers are a bit in-between. I could see wanting to be able to configure a mapping for known MCP server URL prefixes for things like API keys, but that becomes a lot more of a complex API. At that point maybe we'd recommend putting custom logic in a DelegatingHandler.
There was a problem hiding this comment.
Could we have a Func<HostedMcpServerTool, HttpClientTransportOptions?>? transportOptionsProvider = null? Someone could return options for specific servers, their own defaults, or null.
There was a problem hiding this comment.
Or maybe Action<HostedMcpServerTool, HttpClientTransportOptions>
|
This is a really awesome extension. ❤ |
| }); | ||
| } | ||
|
|
||
| [Experimental("MEAI001")] |
There was a problem hiding this comment.
Do we need to mark private types as experimental?
There was a problem hiding this comment.
No, I added it to not have to suppress usage of the experimental API with #pragma warning disable MEAI001, the only usage of this class was also annotated as [Experimental("MEAI001")].
I will have to switch the ID to MCPXXX in UseMcpClient so I will have to switch the source suppression here anyway.
| var mcpClient = await CreateMcpClientAsync(mcpTool.ServerAddress, parsedAddress, mcpTool.ServerName, mcpTool.AuthorizationToken).ConfigureAwait(false); | ||
| var mcpFunctions = await mcpClient.ListToolsAsync(cancellationToken: cancellationToken).ConfigureAwait(false); | ||
|
|
||
| // Add the listed functions to our list of tools we'll pass to the inner client. |
There was a problem hiding this comment.
How does this approach account for the list of tools potentially changing after this initialization step?
There was a problem hiding this comment.
It doesn't. We are yet to define lifespan of listed tools and how to handle protocol errors on invoked tools e.g. tool no longer exists.
5 participants
Contributes to dotnet/extensions#6492
cc @westey-m