Review fixes for #189: detect input-required prompt/resource results

[jhrozek]

Stacked on top of #189 (base is feat/mcpcompat-modern-client-surface, not main) so @JAORMX keeps control of the merge and I keep my review on #189. Merge or cherry-pick at your discretion; if you'd rather these were squashed into your commit, say so and I'll rebase them in.

The one that matters: input-required prompt/resource results were undetectable

WithoutMultiRoundTrip() opts out of go-sdk's automatic MRTR handling for all three MRTR-capable methods — go-sdk's clientMultiRoundTripMiddleware intercepts tools/call, prompts/get and resources/read (mrtr.go:76-79), and its own MultiRoundTripOptions.Disabled doc names all three NeedsInput methods as the caller's contract. But only CallToolResult modeled resultType.

CallTool works because it converts via jsonConvert. convertGetPromptResult / convertReadResourceResult are hand-written field-by-field (their Content / ResourceContents interface fields can't survive a JSON round-trip), so they dropped resultType before the caller could see it.

Reproduced before the fix — GetPrompt against a stateless 2026-07-28 backend whose handler returns InputRequests:

GetPrompt err=<nil>
shim result: desc="" messages=0

err == nil, zero messages, no way to tell it apart from an empty completed result. The draft spec's MRTR page lists all three operations as returning InputRequiredResult.

Fix: NeedsInput on GetPromptResult and ReadResourceResult mirroring CallToolResult, populated in both converters from go-sdk's NeedsInput(). The live and isResume() paths both route through the converters, so both are covered (go-sdk's own UnmarshalJSON populates resultType on the resume path before the converter runs).

Doc corrections

• HEADER_MISMATCH said headers must match "the initial InitializeRequest". 2026-07-28 is stateless with no handshake; -32020 validates headers against the same request's own body. go-sdk's comment (shared.go:386) already had this right — the shim reworded it and lost the meaning.
• Legacy codes. Error-code block is now in numeric order, and -32002 / -32042 are marked legacy: the draft spec says implementations of this revision MUST NOT emit either, though clients SHOULD still accept -32002 from older servers. They were sitting unmarked next to the three spec-current codes.
• NeedsInput caveat. A present-but-unrecognized resultType also reports false, which the spec requires clients to treat as invalid, not complete. Documented rather than fixed — go-sdk's NeedsInput is the identical comparison, and diverging would make the shim reject what go-sdk accepts. Flagging in case you want to raise it upstream.
• WithoutMultiRoundTrip rationale. go-sdk reads ClientOptions.MultiRoundTrip in NewClient (which Initialize calls), not in Connect. "Must be set before Initialize" was right for the wrong reason.
• #5911 → stacklok/toolhive#5911.

Test hardening

• MarshalJSON not re-emitting resultType was documented and honored but untested. Now guarded — a proxy that unmarshals/mutates/re-marshals a result would otherwise silently emit a stale classification if it broke.
• default MRTR errors without a fulfilling handler asserted only assert.Error, which a transport EOF would satisfy. Now pins *jsonrpc.Error / -32602 / "client does not support elicitation".

Open question for you — not decided here

GetPromptResult / ReadResourceResult needed some way for mcpcompat/client to set a classification that lives in package mcp. This PR uses an unexported resultType + exported SetNeedsInput(bool), matching the shape you chose on CallToolResult.

One reviewer argued for an exported ResultType string tagged json:"-" plus exported ResultTypeComplete/ResultTypeInputRequired consts instead, deleting both setters. The case for it:

1. SetNeedsInput(true) on a server-built result compiles, reads as meaningful, and does nothing — MarshalJSON drops the field. An imperative verb that makes no observable change is a footgun.
2. json:"-" on an exported field is exactly as wire-safe (encoding/json never touches unexported fields either way), and this package already does that in three places: RawInputSchema, RawOutputSchema, Extra.
3. A bool permanently collapses the spec's open string union; the exported field would surface the raw string on the CallToolResult path where it actually exists, and let the "not otherwise exposed" caveat be deleted.

Counter-argument: some cross-package channel is structurally unavoidable, and the bool loses nothing go-sdk exposes publicly anyway. Roughly +25/−40 and mechanical if you want it, but it changes your CallToolResult field too, so it's your call, not mine. Happy to push it to this branch if you'd like it.

Verification

go build ./..., go test -race ./mcpcompat/..., task lint (0 issues), go vet, task license-check — all clean. Both new tests were confirmed to fail when the corresponding fix is reverted. Unrelated: TestOnConnectionLost_RegisterAfterInitialize flaked once under full-package load (a CloseIdleConnections transport race in conn_lost_test.go, untouched here) — pre-existing, worth a separate look.

🤖 Generated with Claude Code