Fix protocol version tag for stateless server spans - #1810
Conversation
|
Thanks for submitting this, @luisangelrod! Tagging @tarekgh for review. |
| Assert.Contains(activities, activity => | ||
| activity.DisplayName == "tools/list" && | ||
| activity.Kind == ActivityKind.Server && | ||
| activity.GetTagItem("mcp.protocol.version") as string == McpProtocolVersions.July2026ProtocolVersion); |
There was a problem hiding this comment.
From what I can tell, this assert could have a false-positive here if the negotiated version is also the July2026ProtocolVersion.
Please augment the test(s) to force scenarios of the negotiated version and the per-request version being different (in both directions if possible) and also being the same on multiple versions, asserting the tag item and also asserting the negotiated version.
There was a problem hiding this comment.
I updated the tests to address the false positive and the coverage you asked for:
- Replaced the single-version fact with a theory over
2025-11-25and2026-07-28in stateless mode. In stateless mode each request is served by a fresh, un-negotiated server, soNegotiatedProtocolVersionis null when the span is tagged. That means the tag can only come from the per-request header/_metavalue, and running two versions rules out a coincidental match on a single version. It would regress to an absent tag if the per-request fallback were removed. - Both tests now assert the tag item and also capture and assert
NegotiatedProtocolVersion. - Added a stateful test (
2025-11-25) that runs in both stateful and stateless modes. In stateful mode the session actually holds a negotiated version, and the test asserts the span tag and the negotiated version agree.
On forcing the negotiated and per-request versions to differ: I looked into this and did not add it, because I do not think it can be exercised through a valid flow. A single MCP session must not change protocol versions, so the server rejects any follow-up request whose header/_meta version differs from the negotiated one, and a conformant client never sends a differing header. The only way to observe divergence is to hand-craft a request that fails version-change validation, and asserting the tag on that path would pin the current ordering of tagging relative to validation, which is an implementation detail rather than a guarantee. Instead I cover the two inputs to the tag independently: the per-request path in stateless mode (negotiated is null at tagging time) and the negotiated-only fallback over a header-less transport (DiagnosticTests).
If you would still like an explicit divergence assertion via a raw crafted request, I am happy to add one. Let me know which you prefer.
Cover per-request protocol version tagging across multiple versions in stateless mode, assert the negotiated version alongside the tag, and add a stateful test asserting the negotiated version drives the tag.
tarekgh
left a comment
There was a problem hiding this comment.
I pushed small test changes, LGTM otherwise.
Thanks @luisangelrod for helping with this fix.
Summary
Fixes #1800
Testing