mcp: label resultType on results that bypass the dispatcher (#1226)
Fixes #1225. See the [issue comment](https://github.com/modelcontextprotocol/go-sdk/issues/1225#issuecomment-5500818546). ## The bug `2026-07-28` requires `resultType` on every result. `CallToolResult`, `GetPromptResult`, and `ReadResourceResult` keep `resultType` in an unexported field. Each of these results can be `complete` or `input_required`, so it cannot embed `completeResultWithType` the way the seven types in #1060 do. Only `handleMultiRoundTripResult` sets that field. It runs inside `Server.callTool`, `getPrompt`, and `readResource`. Some middleware returns a result directly instead of calling `next` — for example, an auth, rate-limit, or entitlement gate. That middleware never reaches `handleMultiRoundTripResult`. The response then ships without the field, and strict clients reject it. ## The fix The choice between `complete` and `input_required` now happens inside `setMultiRoundTripResultType`. Both call sites that need this choice call that function. `setCompleteResultType` now labels results it did not label before, so it is renamed `annotateResultType`. The new function picks a branch with a type switch. Reading matters here. `setResultType` is unexported, so setting `InputRequests` is the only way for middleware to ask for input. If the code always labels results `complete`, 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 `InputRequests` again after the shim's single reinvocation. Before this fix, that case shipped with no `resultType`. Embedding `completeResultWithType` instead is not an option: it labels everything `complete` and overwrites `input_required`, breaking multi-round-trip flows. **Skip nil results.** Middleware can return a typed nil value, which satisfies the `Result` interface. `handle` now checks for that case before it calls the labeling function. Before this change, that same case also panicked, one frame later in `annotateServerInfo` and then `GetMeta`. ## Checks - `TestServerSessionHandle_SetsResultTypeWhenMiddlewareShortCircuits` fails on all three methods before the first change. Its fourth case fails if labeling overwrites a populated `InputRequests`. - `TestServerSessionHandle_ResultTypeGate` records which requests carry `resultType`, including one known gap. See "Separate from this PR" below. - `TestServerSessionHandle_NilResultFromMiddleware` panics without the nil check. - The complete-path cases added to `TestServerSessionHandle_SetsResultTypeOnNewProtocol` also pass before this change. They close a gap in #1060, which added only three cases, and only for `input_required`. - The exported API is unchanged. `go doc -all ./mcp`, with comments stripped, is identical before and after this change. - With the reproduction from the issue, over the streamable transport: the dispatched `tools/call` and the short-circuited `tools/call` both now return `resultType: "complete"`. I could not run `./scripts/conformance.sh`. CONTRIBUTING.md names that script, but the repo only has `client-conformance.sh` and `server-conformance.sh`. Both scripts need GNU `timeout`, and macOS does not include it. ## Also here Ten comments named an exported `ResultType` field and `ResultTypeComplete` / `ResultTypeInputRequired` constants. 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. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Guglielmo Colombo <guglielmoc@google.com>
This commit is contained in:
parent
5bc078a795
commit
e40f35d137
1 changed files with 322 additions and 83 deletions
|
|
@ -809,6 +809,9 @@ type Result interface {
|
|||
// isResult discourages implementation of Result outside of this package.
|
||||
isResult()
|
||||
|
||||
// isNil returns true if the underlying value is nil.
|
||||
isNil() bool
|
||||
|
||||
// GetMeta returns metadata from a value.
|
||||
GetMeta() map[string]any
|
||||
// SetMeta sets the metadata on a value.
|
||||
|
|
@ -827,13 +830,15 @@ type ResultBase struct {
|
|||
Meta `json:"_meta,omitempty"`
|
||||
}
|
||||
|
||||
func (*ResultBase) isResult() {}
|
||||
func (*ResultBase) isResult() {}
|
||||
func (x *ResultBase) isNil() bool { return x == nil }
|
||||
|
||||
// emptyResult is returned by methods that have no result, like ping.
|
||||
// Those methods cannot return nil, because jsonrpc2 cannot handle nils.
|
||||
type emptyResult struct{}
|
||||
|
||||
func (*emptyResult) isResult() {}
|
||||
func (x *emptyResult) isNil() bool { return x == nil }
|
||||
func (*emptyResult) GetMeta() map[string]any { panic("should never be called") }
|
||||
func (*emptyResult) SetMeta(map[string]any) { panic("should never be called") }
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue