-
Notifications
You must be signed in to change notification settings - Fork 545
Send MCP-Protocol-Version header with transport messages #493
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,4 +60,12 @@ public interface ITransport : IAsyncDisposable | |
/// </para> | ||
/// </remarks> | ||
Task SendMessageAsync(JsonRpcMessage message, CancellationToken cancellationToken = default); | ||
|
||
/// <summary>Gets or sets the protocol version that's in use.</summary> | ||
/// <remarks> | ||
/// Setting the protocol version does not change the protocol version actively employed by the transport. | ||
/// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: