Fix RequestOptions writing an integer progress token as a JSON string - #1832
Fix RequestOptions writing an integer progress token as a JSON string#1832dfedoryshchev wants to merge 1 commit into
Conversation
luisangelrod
left a comment
There was a problem hiding this comment.
Verified the failure mode and the fix against current main (609499b2). GetMetaForRequest() previously converted a numeric ProgressToken through ToString(), which created a JSON string; after the peer parsed and echoed that token, the boxed string no longer compared equal to the caller's boxed long, so the progress notification was ignored.
Using the registered ProgressToken type info preserves the JSON number and the peer-side RequestParams.ProgressToken round-trips as the original numeric token. I also checked the default-valued token edge case: the new empty-string representation follows the existing converter's behavior and avoids emitting a protocol-invalid null token.
Focused verification:
RequestOptionsTests: 18 passed on each ofnet472,net8.0,net9.0, andnet10.0(72 total)- All four target builds completed without warnings during the test run
git diff --checkpassed
Approving. This is a narrowly scoped protocol-correctness fix with an effective regression test.
Note
This review was prepared with AI assistance and verified against the source, protocol flow, and focused test results.
RequestOptions.GetMetaForRequest()writes an integer progress token as a JSON string.RequestOptions.cs:82callsProgressToken.ToString()rather than going throughProgressToken's own converter, sonew ProgressToken(42)leaves as"42".That is not cosmetic, because
ProgressTokencompares by boxed value. The peer reads the field back asProgressToken("42")(RequestParams.cs:62-72triesstringfirst), then stamps that token on every progress notification. The caller compares"42"against its own42, boxedlongnever equalsstring, and progress notifications match nothing. They are dropped silently, with no error.It is reachable from every high-level request that takes a
RequestOptions; fourteen call sites feedoptions?.GetMetaForRequest()into the request params.The existing test for exactly this case,
GetMetaForRequest_OnlyProgressTokenSetAsLong_ReturnsNewObjectWithToken, misses it because it asserts onactual["progressToken"]?.ToString(), andJsonNode.ToString()unquotes a string node.Fix: serialize through the converter instead.
[JsonSerializable(typeof(ProgressToken))]is already registered in the source-generated context (McpJsonUtilities.cs:196), so this stays trim and AOT safe and cannot drift from the converter again.One behaviour change worth flagging: a
ProgressTokenwith a null inner value now serializes as""rather thannull. The SDK's own reader throws onnullhere, so this is an improvement, but it is a change on the same line.Verification:
Expected: Number, Actual: String.RequestOptionsTests: 17 passed / 1 failed, to 18 passed.ModelContextProtocol.Testson net10.0: 2298 passed, 0 failed, 2 skipped. ExcludesClientIntegrationTestsandDockerEverythingServerTests, which neednpxand Docker and fail identically before and after.ModelContextProtocol.Corebuilds clean on net10.0, net9.0, net8.0 and netstandard2.0 with 0 warnings under warnings-as-errors.