Conversation
tests/ModelContextProtocol.AspNetCore.Tests/UseMcpClientWithTestSseServerTests.cs
Outdated
Show resolved
Hide resolved
stephentoub
left a comment
There was a problem hiding this comment.
Nice. Seems like this generally works out?
|
This is a really awesome extension. ❤ |
Add more tests Add Retry logic Add configureTransportOptions
| return await base.InvokeCoreAsync(arguments, cancellationToken).ConfigureAwait(false); | ||
| } | ||
| catch (HttpRequestException) { } | ||
|
|
||
| bool result = _chatClient.RemoveMcpClientFromCache(_hostedMcpTool.ServerAddress, out var removedTask); | ||
| Debug.Assert(result && removedTask!.Status == TaskStatus.RanToCompletion); | ||
| _ = removedTask!.Result.Client.DisposeAsync().AsTask(); | ||
|
|
||
| var freshTool = await GetCurrentToolAsync().ConfigureAwait(false); | ||
| return await freshTool.InvokeAsync(arguments, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
I added an LRU cache. Clients will be evicted if limit is reached or if the tool invocation throws an HttpRequestException, as per the spec, servers return 404 when a session is revoked, in that latter case, we immediately retry with a new client.
| return tools.FirstOrDefault(t => t.Name == Name) ?? | ||
| throw new McpProtocolException($"Tool '{Name}' no longer exists on the MCP server.", McpErrorCode.InvalidParams); |
There was a problem hiding this comment.
I also chose to look for the tool by name and throw (if is not there) the McpProtocolException which is the same exception that would be thrown if the server doesn't find the tool you asked to invoke.
| // public const string Tasks_DiagnosticId = "MCP5001"; | ||
| // public const string Tasks_Message = "The Tasks feature is experimental within specification version 2025-11-25 and is subject to change. See SEP-1686 for more information."; | ||
| // public const string Tasks_Url = "https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1686"; | ||
|
|
||
| public const string UseMcpClient_DiagnosticId = "MCP5002"; | ||
| public const string UseMcpClient_Message = "The UseMcpClient middleware for integrating hosted MCP servers with IChatClient is experimental and subject to change."; |
There was a problem hiding this comment.
Adjustment to this plan. We will reuse a single ID while still defining the consts for our own code hygiene.
| // public const string Tasks_DiagnosticId = "MCP5001"; | |
| // public const string Tasks_Message = "The Tasks feature is experimental within specification version 2025-11-25 and is subject to change. See SEP-1686 for more information."; | |
| // public const string Tasks_Url = "https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1686"; | |
| public const string UseMcpClient_DiagnosticId = "MCP5002"; | |
| public const string UseMcpClient_Message = "The UseMcpClient middleware for integrating hosted MCP servers with IChatClient is experimental and subject to change."; | |
| // All experimental APIs share the same ID of MCP0001 but the consts define logical features | |
| // public const string Tasks_DiagnosticId = "MCP0001"; | |
| // public const string Tasks_Message = "The Tasks feature is experimental within specification version 2025-11-25 and is subject to change. See SEP-1686 for more information."; | |
| // public const string Tasks_Url = "https://github.com/modelcontextprotocol/modelcontextprotocol/issues/1686"; | |
| public const string UseMcpClient_DiagnosticId = "MCP0001"; | |
| public const string UseMcpClient_Message = "The UseMcpClient middleware for integrating hosted MCP servers with IChatClient is experimental and subject to change."; |
Contributes to dotnet/extensions#6492
cc @westey-m