mcp: label resultType on results that bypass the dispatcher - #1226
Open
roncodingenthusiast wants to merge 4 commits into
Open
Conversation
roncodingenthusiast
force-pushed
the
ronald/gh-1225/stamp-resulttype-on-mrtr-results
branch
from
September 1, 2026 21:47
4480282 to
3593e94
Compare
roncodingenthusiast
changed the title
roncodingenthusiast
force-pushed
the
ronald/gh-1225/stamp-resulttype-on-mrtr-results
branch
2 times, most recently
from
September 1, 2026 21:56
8005597 to
87e1f29
Compare
roncodingenthusiast
marked this pull request as ready for review
September 2, 2026 19:05
roncodingenthusiast
force-pushed
the
ronald/gh-1225/stamp-resulttype-on-mrtr-results
branch
2 times, most recently
from
September 2, 2026 19:26
e6a3678 to
9d15c1a
Compare
Protocol revision 2026-07-28 requires resultType on every result. CallToolResult, GetPromptResult and ReadResourceResult hold it in an unexported field, since they can be complete or input_required and so cannot embed completeResultWithType as the seven types in modelcontextprotocol#1060 do. Only handleMultiRoundTripResult writes that field, and it runs inside Server.callTool, getPrompt and readResource. A receiving middleware that returns a result instead of calling next -- an auth, rate limit or entitlement gate -- never reaches it, so the response ships without the field and strict clients reject it as malformed. Move the choice of complete versus input_required into setMultiRoundTripResultType, and call it from both places that need it. setCompleteResultType, which now labels results it did not before, is renamed annotateResultType and picks a branch by type switch. It reads inputRequests() rather than assuming complete, which matters because setResultType is unexported: populating InputRequests is a middleware's only way to ask for input, and labeling it complete would make a conforming client drop the elicitation. Recomputing is safe because a result that did reach the dispatcher was labeled from the same test. Embedding completeResultWithType instead would label every result complete and clobber input_required, breaking multi-round-trip flows. A middleware can also return a typed nil, which satisfies Result, and annotating one panics. That panic predates this change; it happened one frame further on, in annotateServerInfo. Result gains isNil, matching what Params already does, and ServerSession.handle skips annotation for a nil result. Result is sealed by isResult, so adding an unexported method to it is not a breaking change. ServerSession.handle still gates on the request's _meta while handleMultiRoundTripResult gates on the session's negotiated version, so a session that negotiated 2026-07-28 and sends no _meta gets no resultType from a short-circuited request. Only a client that mixes protocol versions can reach that, so TestServerSessionHandle_ResultTypeGate records it rather than changing behavior to cover it. Also correct ten comments naming an exported ResultType field and ResultTypeComplete/ResultTypeInputRequired constants, none of which exist. Two were unresolvable [CallToolResult.ResultType] doc links and now point at [CallToolResult.NeedsInput]. The exported API is unchanged and the conformance goldens do not move. Fixes modelcontextprotocol#1225 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
roncodingenthusiast
force-pushed
the
ronald/gh-1225/stamp-resulttype-on-mrtr-results
branch
from
September 2, 2026 19:31
9d15c1a to
7ef0cb3
Compare
Applies ASD-STE100 Simplified Technical English to the comments touched in the resultType labeling change: complete sentences with explicit subjects, no -ing verb clauses, and "must" instead of "should" for the InputRequests requirement.
…ulttype-on-mrtr-results
Contributor
Author
|
@guglielmo-san i am assuming you will take care of merging this on your end? not sure if there is anything else i should do other than just wait for you to review, approve/merge |
…ulttype-on-mrtr-results
Comment on lines
53
to
57
| if clientSupportsMultiRoundTrip(ss) { | ||
| // For older clients the resultType is left unset. Input requests will be handled | ||
| // by serverMultiRoundTripMiddleware client calls and handler reinvocation. | ||
| if hasInputRequests { | ||
| res.setResultType(resultTypeInputRequired) | ||
| } else { | ||
| res.setResultType(resultTypeComplete) | ||
| } | ||
| setMultiRoundTripResultType(res) | ||
| } |
Contributor
There was a problem hiding this comment.
why do we still need to set this inside mrtr ? Now it should be enough the single call from handle
Contributor
Author
There was a problem hiding this comment.
We cannot remove this call from the code as it is now. The dispatcher reads resultType before handle runs.
- L1018 keeps
"content"in the JSON response asnulland not[]. - L1087 returns early, before the Go check
res.Contents == nilgives an error.
Removing the mrtr call breaks a bunch of pre-existing tests: (eg. TestServerConformance/mrtr.txtar, the three TestMultiRoundTrip_ReadResource_* tests...).
To remove it, I would also have to change the two dispatcher lines to read InputRequests and not resultType. I tried this approach and it seemed to work. The result is cleaner, but it changes input_required detection on paths that are not related to #1225 or the current PR.
I am happy to do it in this PR if you want or keep this as is. What do you think?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1225. See the issue comment.
The bug
2026-07-28requiresresultTypeon every result.CallToolResult,GetPromptResult, andReadResourceResultkeepresultTypein an unexported field. Each of these results can becompleteorinput_required, so it cannot embedcompleteResultWithTypethe way the seven types in #1060 do.Only
handleMultiRoundTripResultsets that field. It runs insideServer.callTool,getPrompt, andreadResource. Some middleware returns a result directly instead of callingnext— for example, an auth, rate-limit, or entitlement gate. That middleware never reacheshandleMultiRoundTripResult. The response then ships without the field, and strict clients reject it.The fix
The choice between
completeandinput_requirednow happens insidesetMultiRoundTripResultType. Both call sites that need this choice call that function.setCompleteResultTypenow labels results it did not label before, so it is renamedannotateResultType. The new function picks a branch with a type switch.Reading matters here.
setResultTypeis unexported, so settingInputRequestsis the only way for middleware to ask for input. If the code always labels resultscomplete, the response ships as{"resultType":"complete","inputRequests":{...}}. A client that trusts the discriminator then drops the elicitation request.Recomputing the label every time is safe. A result that did reach the dispatcher already carries the same label from that test. This also covers one edge case: a handler that returns
InputRequestsagain after the shim's single reinvocation. Before this fix, that case shipped with noresultType.Embedding
completeResultWithTypeinstead is not an option: it labels everythingcompleteand overwritesinput_required, breaking multi-round-trip flows.Skip nil results. Middleware can return a typed nil value, which satisfies the
Resultinterface.handlenow checks for that case before it calls the labeling function. Before this change, that same case also panicked, one frame later inannotateServerInfoand thenGetMeta.Checks
TestServerSessionHandle_SetsResultTypeWhenMiddlewareShortCircuitsfails on all three methods before the first change. Its fourth case fails if labeling overwrites a populatedInputRequests.TestServerSessionHandle_ResultTypeGaterecords which requests carryresultType, including one known gap. See "Separate from this PR" below.TestServerSessionHandle_NilResultFromMiddlewarepanics without the nil check.TestServerSessionHandle_SetsResultTypeOnNewProtocolalso pass before this change. They close a gap in mcp: include resultType on new-protocol responses #1060, which added only three cases, and only forinput_required.go doc -all ./mcp, with comments stripped, is identical before and after this change.tools/calland the short-circuitedtools/callboth now returnresultType: "complete".I could not run
./scripts/conformance.sh. CONTRIBUTING.md names that script, but the repo only hasclient-conformance.shandserver-conformance.sh. Both scripts need GNUtimeout, and macOS does not include it.Also here
Ten comments named an exported
ResultTypefield andResultTypeComplete/ResultTypeInputRequiredconstants. None of those exist. Two of the comments were[CallToolResult.ResultType]doc links, and those links cannot resolve. They now point at[CallToolResult.NeedsInput]instead, because an unexported field is also not a valid link target.