Send MCP-Protocol-Version header with transport messages #493
Conversation
| { | ||
| return; | ||
| headers.Add("MCP-Protocol-Version", protocolVersion); |
There was a problem hiding this comment.
Nit:
| headers.Add("MCP-Protocol-Version", protocolVersion); | |
| headers.Add("mcp-protocol-version", protocolVersion); |
| /// It provides that information to the transport for situations where the transport needs to be able to | ||
| /// propagate the version information, for example as part of HTTP headers or for logging and diagnostic purposes. | ||
| /// </remarks> | ||
| string? ProtocolVersion { get; set; } |
There was a problem hiding this comment.
Having this as a public settable property is a little weird when, as the remarks note, it doesn't change the behavior of the transport.
I was thinking we should try to minimize the change to the public API surface and do something more similar to what I instructed copilot to do in halter73#11. It does end up double-deserializing the InitializeResult, but I think that's better layering.
If we think it's useful to have a public property, I think it should have an internal setter, and we should call it in McpServer as well. And it should probably be on IMcpEndpoint rather than ITransport. However, I think we should probably just get rid of this property alltogether for now, and contain the product changes to just a few lines in StreamableHttpClientSessionTransport.cs.
There was a problem hiding this comment.
I don't have a strong preference.
I wasn't aware you were already working on it. I'll just close this one and you can continue with the other approach.
This adds the "MCP-Protocol-Version" header to the client's HTTP requests, per the spec.
I did not update the server following modelcontextprotocol/modelcontextprotocol#548 change from:
to
as that's presumably going to break a bunch of clients. @dsp-ant, are we sure we want that spec change?