Skip to content

Surface Modern client consumption helpers in mcpcompat - #189

Merged
JAORMX merged 2 commits into
mainfrom
feat/mcpcompat-modern-client-surface
Jul 25, 2026
Merged

Surface Modern client consumption helpers in mcpcompat#189
JAORMX merged 2 commits into
mainfrom
feat/mcpcompat-modern-client-surface

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Why: PR3 of the Phase 1 mcpcompat stateless work (after #185, #187). Gives downstream ToolHive the additive shim surface to consume MCP 2026-07-28 backends and classify Modern responses without importing go-sdk directly. go-sdk v1.7's client Connect already does the Modern wire negotiation, so this is re-exporting/adapting, not new protocol logic.

What (all additive — no existing exported signature changed):

  • Re-export the 2026-07-28 error codes as shim-owned constants: HEADER_MISMATCH=-32020, MISSING_REQUIRED_CLIENT_CAPABILITIES=-32021, UNSUPPORTED_PROTOCOL_VERSION=-32022. Kept as local literals (matching the file's pattern, not aliases); a test pins them to the exported gosdk.Code* values so a renumber on a future SDK bump fails loudly.
  • CallToolResult.NeedsInput() — capture the wire resultType discriminator in UnmarshalJSON (go-sdk emits "input_required" for an MRTR input-required result) and expose an accessor mirroring go-sdk's. MarshalJSON unchanged (the shim result is consumed/classified, never re-forwarded).
  • client.WithoutMultiRoundTrip() — opt out of go-sdk's automatic multi-round-trip so an input-required result surfaces to the caller instead of being auto-fulfilled. Detection only for now (documented): the shim doesn't yet model InputRequests/RequestState, so fulfilling/retrying through the shim isn't supported.
  • Documented the per-backend protocol-version pin gap (#5911): the shim passes nil ClientSessionOptions to Connect and can't force a version because the field is unexported upstream — upstream ask recommended.

Deferred (no consumer yet — tracked here, deliberately not left as code TODOs)

  • DiscoverResult/SupportedVersions surfacing, per-request _meta.protocolVersion echo — go-sdk's Connect handles negotiation internally.
  • Cacheable/cacheScope/ttlMs on client-consumed results — no client-side cache consumer.
  • InputRequests/RequestState on the shim CallToolResult — needed only to act on/forward an input-required result (NeedsInput needs only resultType). If ToolHive later forwards Modern input-required results, it must add resultType+InputRequests+RequestState together and update MarshalJSON.
  • NeedsInput is CallTool-scoped for PR3 (not GetPrompt/ReadResource) — intended.

Review

Two Opus panels (MCP-correctness + code-quality/API-compat) confirmed correct/additive; their cleanups are applied (dead const removed, error-code test pinned to go-sdk consts, WithoutMultiRoundTrip doc softened to detection-only).

Depends on

Follows #185/#187 (merged). Same pre-release constraint (release guard blocks tagging until go-sdk v1.7.0 final).

Test plan

  • task test (race) — Modern error-code constants pinned to go-sdk; NeedsInput() across wire shapes; MRTR opt-out surfaces input-required (err==nil, NeedsInput()==true, handler once) vs default MRTR errors
  • task lint — 0 issues
  • task license-check — clean
  • go build ./... — clean

🤖 Generated with Claude Code

Add the additive shim surface downstream ToolHive needs to consume MCP
2026-07-28 backends and classify Modern responses without importing go-sdk
directly. PR3 of the Phase 1 mcpcompat stateless work; go-sdk v1.7's client
Connect already does the Modern wire negotiation, so this is re-exporting and
adapting, not new protocol logic.

- Re-export the 2026-07-28 JSON-RPC error codes as shim-owned constants
  (HEADER_MISMATCH -32020, MISSING_REQUIRED_CLIENT_CAPABILITIES -32021,
  UNSUPPORTED_PROTOCOL_VERSION -32022), so downstream can classify without a
  go-sdk import. Kept as local literals (not aliases) matching the file's
  existing pattern; a test pins them to the exported go-sdk values so a
  renumber on a future SDK bump fails loudly.
- CallToolResult.NeedsInput(): capture the wire "resultType" discriminator in
  UnmarshalJSON (go-sdk emits "input_required" for an MRTR input-required
  result) and expose a NeedsInput accessor mirroring go-sdk's. MarshalJSON is
  unchanged: the shim result is a consumed/classification value, never
  re-forwarded.
- client.WithoutMultiRoundTrip(): opt out of go-sdk's automatic multi-round-
  trip handling so an input-required result surfaces to the caller instead of
  being auto-fulfilled. Detection only for now — the shim does not yet model
  InputRequests/RequestState, so fulfilling/retrying through the shim is not
  supported (documented on the option).
- Document the per-backend protocol-version pin gap (#5911): the shim passes
  nil ClientSessionOptions to Connect and cannot force a version because the
  field is unexported upstream.

Not surfaced (no consumer yet, tracked for follow-up, not left as code TODOs):
DiscoverResult/SupportedVersions, per-request _meta.protocolVersion echo,
Cacheable/cacheScope/ttlMs on client-consumed results, and
InputRequests/RequestState (needed only to act on/forward an input-required
result; would require resultType+InputRequests+RequestState together plus a
MarshalJSON change).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MRh394VZgnqbYjJwjrcJWm
@jhrozek

jhrozek commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Reviewed this and put the fixes in #190, stacked on this branch rather than pushed here — that way you keep the merge decision and I keep my review on #189. Merge it, cherry-pick it, or tell me to rebase it into your commit; whatever you prefer.

One real bug. WithoutMultiRoundTrip() disables go-sdk's MRTR handling for all three MRTR-capable methods — the middleware intercepts tools/call, prompts/get and resources/read — but only CallToolResult models resultType. CallTool survives because it converts via jsonConvert; convertGetPromptResult and convertReadResourceResult are hand-written field-by-field, so they drop the field. Result: with the opt-out set, an input-required prompt or resource read returns err == nil with zero messages/contents, indistinguishable from an empty success. Reproduced against a stateless 2026-07-28 backend before fixing. #190 adds NeedsInput to both types and populates it in both converters.

Six doc/test items, the notable one being HEADER_MISMATCH: its comment described matching against "the initial InitializeRequest", but 2026-07-28 is stateless with no handshake — -32020 validates headers against the same request's own body. go-sdk's own comment has it right; the shim's rewording lost the meaning. Also marked -32002/-32042 as legacy (the draft spec says this revision MUST NOT emit either), fixed the WithoutMultiRoundTrip "consulted when the client connects" rationale (go-sdk reads it in NewClient, which Initialize calls), added a regression test for the documented MarshalJSON-drops-resultType invariant, and pinned the assert.Error-only MRTR subtest to the actual -32602 / "client does not support elicitation" contract.

One thing I deliberately left for you rather than deciding on your behalf: whether the classification should be an unexported field + SetNeedsInput(bool) (what #190 does, matching your CallToolResult shape) or an exported ResultType string tagged json:"-" with exported consts. There's a decent argument for the latter — SetNeedsInput(true) on a server-built result silently does nothing since MarshalJSON drops the field, and this package already uses exported + json:"-" for RawInputSchema/Extra. It'd touch your CallToolResult field too, so it's your call. Details in #190.

Nothing else blocking from my side. The resultType-not-re-emitted decision checks out, by the way — I traced toolhive's forwarding paths and nothing re-forwards a shim CallToolResult today (vMCP converts through vmcp.ToolCallResult, which has no equivalent field; pkg/authz/response_filter.go still uses mark3labs types). Worth noting in the migration issue as the one spot where an unexported non-re-emitted field could become a silent regression.

Review fixes for #189. The substantive one is a silent-wrong-result bug;
the rest are doc corrections and test hardening.

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 -- but only
CallToolResult modeled the "resultType" classification. CallTool works
because it converts via jsonConvert; convertGetPromptResult and
convertReadResourceResult are hand-written field-by-field (their Content /
ResourceContents interface fields cannot survive a JSON round-trip), so they
dropped resultType before the caller could ever see it.

Reproduced before the fix: GetPrompt against a stateless 2026-07-28 backend
whose handler returns InputRequests yields err == nil and zero Messages,
indistinguishable from an empty completed result. The draft spec lists all
three operations as returning InputRequiredResult, and go-sdk's own
MultiRoundTripOptions.Disabled doc names all three NeedsInput methods as the
caller's contract.

- Add NeedsInput to GetPromptResult and ReadResourceResult, mirroring
  CallToolResult, and populate it in both converters from go-sdk's
  NeedsInput(). Both the live and isResume() paths route through the
  converters, so both are covered.
- Correct the HEADER_MISMATCH doc: it described matching against "the initial
  InitializeRequest", but 2026-07-28 is stateless with no handshake. -32020 is
  validation of headers against the same request's own body. go-sdk's own
  comment (shared.go:386) already had this right.
- Order the MCP error-code block numerically and mark -32002 and -32042 as
  legacy: the draft spec says implementations of this revision MUST NOT emit
  either, though clients SHOULD still accept -32002 from older servers.
- Note on NeedsInput that a present-but-unrecognized resultType also reports
  false, which the spec treats as invalid rather than complete. Documented
  rather than fixed: go-sdk's NeedsInput is the identical comparison and the
  shim mirrors it, so diverging would make the shim reject what go-sdk accepts.
- Correct WithoutMultiRoundTrip's 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.
- Add a regression test that CallToolResult.MarshalJSON does not re-emit
  resultType. The invariant was documented and honored but untested; a proxy
  that unmarshals, mutates and re-marshals a result would silently emit a
  stale classification if it broke.
- Strengthen "default MRTR errors without a fulfilling handler" from a bare
  assert.Error to pin the actual contract (*jsonrpc.Error, code -32602,
  "client does not support elicitation"), so a transport EOF no longer
  satisfies it.
- Qualify the cross-repo issue reference as stacklok/toolhive#5911.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@JAORMX
JAORMX merged commit d258c4a into main Jul 25, 2026
5 checks passed
@JAORMX
JAORMX deleted the feat/mcpcompat-modern-client-surface branch July 25, 2026 11:39
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.

2 participants