Repository navigation
Preserve per-request client capability extensions when the session declares its own - #1921
nathangobinet wants to merge 5 commits into
Conversation
…clares its own InjectRequestMeta overlaid the session capabilities onto the request's _meta.clientCapabilities one top-level key at a time, so a session that declares any extension replaced the request's whole "extensions" object and dropped the io.modelcontextprotocol/tasks opt-in added by CallToolAsTaskAsync. Merge JSON objects recursively instead. Fixes modelcontextprotocol#1920 Co-Authored-By: Claude Opus 5.5 <[email protected]>
tair800
left a comment
tair800
left a comment
There was a problem hiding this comment.
Thanks for the clear write-up in #1920 and the focused fix.
An AI-assisted reproduction (a standalone console harness with the previous merge loop and the new MergeJsonObject copied verbatim; the repository itself was not built and its test suite was not run) shows the reported case fixed: with session capabilities {"extensions":{"io.modelcontextprotocol/ui":{}}}, the per-request io.modelcontextprotocol/tasks opt-in is now kept alongside the session's extension.
One question inline about how far the recursive merge should reach.
Note
This review was prepared with AI assistance, including the code analysis, the standalone reproduction harness and the wording.
| { | ||
| foreach (var property in source.ToArray()) | ||
| { | ||
| if (target[property.Key] is JsonObject targetChild && property.Value is JsonObject sourceChild) |
There was a problem hiding this comment.
Question on scope: this recursion reaches every key that is an object on both sides, at any depth, not only the extensions map from #1920. In the standalone harness (this method copied verbatim):
- Standard capabilities now merge per request. Session
{"elicitation":{"form":{}}}plus request{"elicitation":{"url":{}}}gives{"elicitation":{"url":{},"form":{}}}(previously{"elicitation":{"form":{}}}). - Settings of an extension declared on both sides are merged field by field. Request
{"com.example/ext":{"mode":"request","limit":10}}plus session{"com.example/ext":{"mode":"session"}}gives{"mode":"session","limit":10}, a combination neither side declared.
Is that the intended scope? (1) matches how request-only top-level keys already survive, so it may well be. For (2), since each extension defines its own settings schema, would it be safer to merge only the set of extension ids and take each extension's settings object whole from one side? Either way, a test pinning the chosen behaviour would document it.
Note
AI-assisted comment; see the note in the review summary.
There was a problem hiding this comment.
Good catch, thanks. The recursion was wider than #1920 needs, and (2) could produce settings that neither side declared.
I narrowed it in 557352e: only the extensions map is merged, by extension identifier. Every other capability, and the settings object of an extension declared on both sides, is taken whole from the session, which is what the overlay did before this PR. So (1) no longer merges and (2) gives {"mode":"session"}.
CallToolAsync_WithRequestAndSessionCapabilities_MergesExtensionsByIdentifierOnly pins both cases.
Note
This comment was AI-generated (Claude Code).
The recursive merge reached every object present on both sides, so a standard capability declared per request was merged with the session's, and the settings of an extension declared on both sides were merged field by field. Merge the "extensions" map by extension identifier only. Every other capability, and the settings object of an extension declared on both sides, is taken whole from the session. Co-Authored-By: Claude Fable 5.2 <[email protected]>
Fixes #1920
Problem
On the 2026-07-28 revision,
McpSessionHandler.InjectRequestMetaoverlays the session capabilities onto the request's_meta.clientCapabilitiesone top-level key at a time. When the session declares any extension, itsextensionsobject replaces the request's. That drops the per-requestio.modelcontextprotocol/tasksopt-in added byCallToolAsTaskAsync, and the server answers synchronously.Change
extensionsmap by extension identifier, so an extension declared only on the request survives. Every other capability, and the settings object of an extension declared on both sides, is still taken whole from the session.McpClientTaskMethodsTests.CallToolAsTaskAsync_WithSessionLevelExtensions_StillReturnsCreatedTask. It fails without the fix.McpClientTaskMethodsTests.CallToolAsync_WithRequestAndSessionCapabilities_MergesExtensionsByIdentifierOnly.Testing
dotnet buildsucceeds with 0 warnings.ModelContextProtocol.Tests(net10.0): 3632 passed, 8 skipped, 2 failed. The two failures areDockerEverythingServerTestsSSE tests, where the local Docker image answersUnknown tool: trigger-sampling-request.ModelContextProtocol.AspNetCore.Testswas not rerun after the last commit.Note
This pull request was AI-generated (Claude Code).