Add AspNetCoreSseServer sample #81
Conversation
halter73
requested review from
twenzel,
stephentoub,
eiriktsarpalis and
PederHP
|
#73 has had a bit of clean-up and the partial multi-client support has been removed to be added properly in a future PR. Feel free to merge it with yours as we discussed, otherwise I can merge the clean-up in a subsequent PR. |
| /// to <see cref="Console.Out"/>, as that will interfere with the transport's output. | ||
| /// </para> | ||
| /// </remarks> | ||
| public StdioServerTransport(IOptions<McpServerOptions> serverOptions, ILoggerFactory? loggerFactory = null) |
There was a problem hiding this comment.
What's the benefit of having a ctor that takes an IOptions<McpServerOptions> when there's already an overload that takes an McpServerOptions?
There was a problem hiding this comment.
This part wasn't important to the AspNetCoreSseServer sample which doesn't use this transport. I put it in a separate "Fix TestServerWithHosting" ee96253 (#81) commit since it isn't required for this sample.
Before this change, the TestServerWithHosting sample didn't work because the McpServerFactory was unable to resolve the IServerTransport, StdioServerTransport, due to being able to find a service for McpServerOptions. IOptions<McpServerOptions> works and also allows you to call methods supported by the options pattern like .Configure<McpServerOptions>(options => ...)
| byte[] buffer = Encoding.UTF8.GetBytes("Accepted"); | ||
| response.OutputStream.Write(buffer, 0, buffer.Length); | ||
| #pragma warning restore CA2016 // Forward the 'CancellationToken' parameter to methods | ||
| #pragma warning disable CA1835 // Prefer the 'Memory'-based overloads for 'ReadAsync' and 'WriteAsync' |
There was a problem hiding this comment.
We should use the Memory-based overload under an ifdef for NET
|
The PR approval from earlier got dismissed because I rebased on top of |
…ory overhaul commit
|
|
||
| /// <summary> | ||
| /// Server capabilities to advertise to the server. | ||
| /// </summary> | ||
| public ServerCapabilities? Capabilities { get; init; } | ||
| public ServerCapabilities? Capabilities { get; set; } |
There was a problem hiding this comment.
If we're making all of these mutable, can you follow-up after this to make other stuff mutable as well? e.g. seems like there's no an inconsistency between McpServerOptions and McpClientOptions. We should have the same story for all these types.
There was a problem hiding this comment.
Sure. Do you want me to do that before merging or in a follow up PR?
This PR demonstrates how you could use a modified
AddMcpServerAPI in ASP.NET Core sample app to implement aMapMcpSse()API (name tbd) that exposes tools registered using.WithTools(). It's inspired a bit by https://github.com/modelcontextprotocol/servers/blob/7034c3d13c72eaf2f188908e9cd54e6d6b091272/src/everything/sse.ts. It doesn't handle anything advanced like concurrent sessions yet.I haven't added any projects other than the new sample app for now. However, the
AspNetCoreSseMcpTransportcould be renamed to something likeResponseStreamSseTransportand moved into the main mcpdotnet package. It wouldn't require adding any ASP.NET Core dependencies, but we would need to update the System.Net.ServerSentEvents dependency to be at least 10.0.0-preview.1.25080.5 which might be awkward for a non-preview package.Contributes to #24