Skip to content

Fix RequestOptions writing an integer progress token as a JSON string - #1832

Open
dfedoryshchev wants to merge 1 commit into
modelcontextprotocol:mainfrom
dfedoryshchev:fix/progress-token-json-number
Open

Fix RequestOptions writing an integer progress token as a JSON string#1832
dfedoryshchev wants to merge 1 commit into
modelcontextprotocol:mainfrom
dfedoryshchev:fix/progress-token-json-number

Conversation

@dfedoryshchev

@dfedoryshchev dfedoryshchev commented Aug 21, 2026

Copy link
Copy Markdown

RequestOptions.GetMetaForRequest() writes an integer progress token as a JSON string. RequestOptions.cs:82 calls ProgressToken.ToString() rather than going through ProgressToken's own converter, so new ProgressToken(42) leaves as "42".

That is not cosmetic, because ProgressToken compares by boxed value. The peer reads the field back as ProgressToken("42") (RequestParams.cs:62-72 tries string first), then stamps that token on every progress notification. The caller compares "42" against its own 42, boxed long never equals string, 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 feed options?.GetMetaForRequest() into the request params.

The existing test for exactly this case, GetMetaForRequest_OnlyProgressTokenSetAsLong_ReturnsNewObjectWithToken, misses it because it asserts on actual["progressToken"]?.ToString(), and JsonNode.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 ProgressToken with a null inner value now serializes as "" rather than null. The SDK's own reader throws on null here, so this is an improvement, but it is a change on the same line.

Verification:

  • New test fails on unpatched source with Expected: Number, Actual: String.
  • RequestOptionsTests: 17 passed / 1 failed, to 18 passed.
  • Full ModelContextProtocol.Tests on net10.0: 2298 passed, 0 failed, 2 skipped. Excludes ClientIntegrationTests and DockerEverythingServerTests, which need npx and Docker and fail identically before and after.
  • net9.0 target (reflection disabled, source-gen only): 39 passed. This is the run that shows the generated path is used.
  • ModelContextProtocol.Core builds clean on net10.0, net9.0, net8.0 and netstandard2.0 with 0 warnings under warnings-as-errors.

@luisangelrod luisangelrod left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 of net472, net8.0, net9.0, and net10.0 (72 total)
  • All four target builds completed without warnings during the test run
  • git diff --check passed

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants