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.

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.

1 participant