diff --git a/pkg/mcp/parser_integration_test.go b/pkg/mcp/parser_integration_test.go index 9e0fb1782d..2e585d0830 100644 --- a/pkg/mcp/parser_integration_test.go +++ b/pkg/mcp/parser_integration_test.go @@ -144,27 +144,26 @@ func TestParsingMiddlewareWithRealMCPClients(t *testing.T) { assert.NotEmpty(t, readResult.Contents) // Verify that all requests were parsed by the middleware - assert.GreaterOrEqual(t, len(parsedRequests), 4, "Expected at least 4 parsed requests (initialize, list tools, call tool, list resources, read resource)") - - // Verify specific parsed requests. Session establishment is - // transport-aware under MCP 2026-07-28 (#5754): the mcpcompat/go-sdk - // v1.7 client is Modern-first (SEP-2575), so over SSE it sends - // server/discover first and gets no answer from this Legacy-only test - // server, falling back — but the fallback initialize handshake is - // itself carried over the SSE transport's message channel, which this - // harness does not capture as a distinct parsed "initialize" the way - // streamable-HTTP does. The streamable-HTTP arm is unaffected (no - // discover-first probe observed here) and still parses "initialize". + assert.GreaterOrEqual(t, len(parsedRequests), 4, "Expected at least 4 parsed requests (session establishment, list tools, call tool, list resources, read resource)") + + // Verify specific parsed requests. mcpcompat's client is Modern-first + // (SEP-2575) on every transport, so both arms send server/discover + // first. The SSE server transport advertises 2026-07-28 unconditionally + // (no ProtocolVersionSupporter gate), so discover succeeds and + // "initialize" is never sent there. The streamable-HTTP server here is + // stateful (no WithStateless), which gates 2026-07-28 on being + // stateless, so its discover negotiates down and it additionally sends + // "initialize". foundSessionEstablished := false foundToolCall := false foundResourceRead := false + var methodsSeen []string for _, parsed := range parsedRequests { + methodsSeen = append(methodsSeen, parsed.Method) switch parsed.Method { case "initialize": - if tc.transport != "sse" { - foundSessionEstablished = true - assert.Equal(t, "test-client", parsed.ResourceID) - } + foundSessionEstablished = true + assert.Equal(t, "test-client", parsed.ResourceID) case "server/discover": if tc.transport == "sse" { // parser.go maps server/discover to the static ResourceID @@ -180,10 +179,10 @@ func TestParsingMiddlewareWithRealMCPClients(t *testing.T) { assert.Equal(t, "test://resource", parsed.ResourceID) } } + assert.True(t, foundSessionEstablished, "session establishment request (initialize or server/discover) should have been parsed") if tc.transport == "sse" { - assert.True(t, foundSessionEstablished, "server/discover request should have been parsed (2026-07-28 HTTP+SSE, #5754)") - } else { - assert.True(t, foundSessionEstablished, "Initialize request should have been parsed") + assert.Contains(t, methodsSeen, "server/discover", "SSE arm should negotiate via server/discover") + assert.NotContains(t, methodsSeen, "initialize", "SSE arm is Modern-first and should never fall back to initialize here") } assert.True(t, foundToolCall, "Tool call request should have been parsed") assert.True(t, foundResourceRead, "Resource read request should have been parsed") diff --git a/pkg/mcp/revision.go b/pkg/mcp/revision.go index 9b23b8f0d2..09a391c029 100644 --- a/pkg/mcp/revision.go +++ b/pkg/mcp/revision.go @@ -551,3 +551,37 @@ func decodeSentinelName(v string) (string, error) { } return string(decoded), nil } + +// EncodeSentinelName encodes v into the draft spec's base64 sentinel format +// (=?base64??=) when required — the mirror of decodeSentinelName. +// Encoding is required when EITHER: +// - v is not safely representable as a plain ASCII header value: any byte +// falls outside printable ASCII 0x21-0x7E, which also covers leading/ +// trailing whitespace and CR/LF (both fall below 0x21 and would +// otherwise make net/http reject the request outright), or +// - v already matches the sentinel pattern (has both sentinelPrefix and +// sentinelSuffix), which must be escaped so the server doesn't mistake a +// literal name for an encoded payload. +// +// Otherwise v is returned unchanged. The result always round-trips through +// decodeSentinelName back to v. +func EncodeSentinelName(v string) string { + if !needsSentinelEncoding(v) { + return v + } + return sentinelPrefix + base64.StdEncoding.EncodeToString([]byte(v)) + sentinelSuffix +} + +// needsSentinelEncoding reports whether v requires sentinel encoding; see +// EncodeSentinelName for the two conditions checked. +func needsSentinelEncoding(v string) bool { + if strings.HasPrefix(v, sentinelPrefix) && strings.HasSuffix(v, sentinelSuffix) { + return true + } + for i := 0; i < len(v); i++ { + if v[i] < 0x21 || v[i] > 0x7E { + return true + } + } + return false +} diff --git a/pkg/mcp/revision_test.go b/pkg/mcp/revision_test.go index 9d48f9a384..027a39498c 100644 --- a/pkg/mcp/revision_test.go +++ b/pkg/mcp/revision_test.go @@ -619,6 +619,42 @@ func TestDecodeSentinelName(t *testing.T) { } } +// TestEncodeSentinelName pins EncodeSentinelName as the exact mirror of +// decodeSentinelName: every case must round-trip back to the original value. +func TestEncodeSentinelName(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + wantSame bool // true if the value must pass through unchanged + }{ + {name: "plain ASCII name unchanged", input: "my-tool", wantSame: true}, + {name: "URI with colon and slash unchanged", input: "file:///tmp/foo.txt", wantSame: true}, + {name: "accented character encoded", input: "café-résumé", wantSame: false}, + {name: "CJK character encoded", input: "工具", wantSame: false}, + {name: "CR in value encoded", input: "bad\rname", wantSame: false}, + {name: "LF in value encoded", input: "bad\nname", wantSame: false}, + {name: "value already shaped like a sentinel is escaped", input: sentinelEncode("my-tool"), wantSame: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := EncodeSentinelName(tt.input) + if tt.wantSame { + assert.Equal(t, tt.input, got) + } else { + assert.NotEqual(t, tt.input, got) + } + + decoded, err := decodeSentinelName(got) + require.NoError(t, err) + assert.Equal(t, tt.input, decoded, "must round-trip through decodeSentinelName") + }) + } +} + // TestStripReservedModernMeta pins the copy-before-mutate contract: it removes // exactly the reserved io.modelcontextprotocol/* keys, never mutates the // caller's map, and returns nil for empty input. diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index 70f71d6021..8fcffa83f9 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -154,11 +154,19 @@ type httpBackendClient struct { // // NOTE: never evicted, no singleflight/CAS. Concurrent first-probes are // last-writer-wins-safe: writes are idempotent for a deterministic backend - // (every probe agrees), and a flapping backend self-heals via - // dispatch->reclassify (a call revealing the other era re-probes and flips). - // A transient probe failure caches nothing (probeRevision returns uncached), - // so a blip cannot pin a revision; a TTL/periodic re-probe is deferred until - // flapping backends surface. + // (every probe agrees). A transient probe failure caches nothing + // (probeRevision returns uncached), so a blip cannot pin a revision. + // + // Self-healing now works both ways under go-sdk v1.7. A mis-cached Modern + // backend that has actually negotiated down to Legacy self-heals via + // dispatch->reclassify (errModernNegotiatedDown re-probes and flips it). A + // mis-cached Legacy backend that has actually become Modern self-heals + // in-band instead: the SDK's own client transparently negotiates Modern on + // the Legacy dispatch path, and legacyInit reads the genuinely-negotiated + // protocol version off every Initialize result and flips the cache directly + // when it is Modern — no error or reclassify round trip needed (see + // TestListCapabilities_MisCachedLegacy_SelfHealsViaSDKNegotiation in + // reclassify_test.go). revisions sync.Map // map[string]mcpparser.Revision } @@ -833,9 +841,12 @@ func wrapBackendError(err error, backendID string, operation string) error { vmcp.ErrBackendUnavailable, operation, backendID, err) } -// initializeClient performs MCP protocol initialization handshake and returns server capabilities. -// This allows the caller to determine which optional features the server supports. -func initializeClient(ctx context.Context, c *client.Client) (*mcp.ServerCapabilities, error) { +// initializeClient performs MCP protocol initialization handshake and returns +// server capabilities plus the actually-negotiated protocol version. The +// latter comes straight from the SDK's InitializeResult, so it reflects the +// real negotiation outcome (e.g. a Modern-first Connect() that negotiated +// down) even though ProtocolVersion is requested as mcp.LATEST_PROTOCOL_VERSION. +func initializeClient(ctx context.Context, c *client.Client) (*mcp.ServerCapabilities, string, error) { result, err := c.Initialize(ctx, mcp.InitializeRequest{ Params: mcp.InitializeParams{ ProtocolVersion: mcp.LATEST_PROTOCOL_VERSION, @@ -854,9 +865,9 @@ func initializeClient(ctx context.Context, c *client.Client) (*mcp.ServerCapabil }, }) if err != nil { - return nil, err + return nil, "", err } - return &result.Capabilities, nil + return &result.Capabilities, result.ProtocolVersion, nil } // queryTools queries tools from a backend if the server advertises tool support. @@ -1057,8 +1068,10 @@ func discoverModernCapabilities(ctx context.Context, hc *http.Client, endpoint s // one Modern wire shape, so a backend must advertise exactly it to be driven // Modern. This is intentionally stricter than go-sdk's reference client // (negotiated >= 2026-07-28). TRIPWIRE: when a newer Modern revision is - // added, this must become a set/range check or a newer-only backend is - // wrongly classified Legacy — TestProbeRevision_RealBackends will catch it. + // added, this must become a set/range check (alongside the sibling + // exact-match in classifyUnsupportedProtocolVersion, modern.go) or a + // newer-only backend is wrongly classified Legacy — TestProbeRevision_RealBackends + // will catch it. if !slices.Contains(discover.SupportedVersions, mcpparser.MCPVersionModern) { return nil, fmt.Errorf("%w: supportedVersions=%v", errModernNegotiatedDown, discover.SupportedVersions) } @@ -1090,13 +1103,34 @@ func discoverModernCapabilities(ctx context.Context, hc *http.Client, endpoint s // A hard error is also returned when the backend transport cannot be built at all // (e.g. invalid auth/CA config); that is a genuine misconfiguration. Note that // dispatch, the sole caller, does not distinguish these error classes — it falls -// back to Legacy uncached on any error this function returns. +// back to Legacy uncached on any error this function returns. A "sse" target +// (see below) returns before that check runs, so its transport is first +// validated on the Legacy call path instead. // The resolved capabilities are intentionally not returned: callers that need // them (ListCapabilities) re-fetch via modernDiscover, so probeRevision only // classifies and caches the revision. +// +// A "sse" target is classified Legacy without a network call. TransportType == +// "sse" specifically names the deprecated 2024-11-05 two-endpoint transport +// (GET /sse + POST /messages) — see ssecommon.HTTPSSEEndpoint and the GET-only +// httpsse proxy — not "any backend that happens to use SSE" (Modern itself uses +// SSE response streams). modernCall is a single-endpoint POST, so it can never +// reach a Modern endpoint through an /sse BaseURL, even for a dual-era server +// that also hosts a Modern endpoint, because that lives at a different path. +// This also sidesteps POSTing into a GET-only stream and hanging to the client +// timeout (errModernTransient, uncached, re-probed on every call). func (h *httpBackendClient) probeRevision( ctx context.Context, target *vmcp.BackendTarget, ) (mcpparser.Revision, error) { + if target.TransportType == "sse" { + // The 2024-11-05 two-endpoint transport (GET /sse + POST /messages) has no + // Modern endpoint to discover at this BaseURL — see the doc comment above. + // Note: HTTP+SSE is Deprecated per the MCP spec, not removed; this gate is + // ToolHive routing, not a protocol requirement. + h.setRevision(target.WorkloadID, mcpparser.RevisionLegacy) + return mcpparser.RevisionLegacy, nil + } + hc, err := h.buildModernHTTPClient(ctx, target) if err != nil { return 0, fmt.Errorf("failed to build transport for backend %s: %w", target.WorkloadID, err) @@ -1408,13 +1442,24 @@ var errLegacyInitFailed = errors.New("legacy initialize step failed") // errLegacyInitFailed (in addition to wrapBackendError's classification) so the // revision-mismatch check can tell an initialize rejection apart from a // data-plane error later in the same call. -func (*httpBackendClient) legacyInit( +// +// It also self-heals a mis-cached Legacy->Modern backend in-band: under go-sdk +// v1.7 the SDK's own client transparently negotiates Modern on this path (see +// the revisions field comment), so the call succeeds and dispatch's +// reclassify-on-error trigger never fires. The negotiated protocol version +// returned by initializeClient is genuine either way (discover or plain +// initialize), so when it equals MCPVersionModern the cache is flipped here +// instead of waiting for an error that will never come. +func (h *httpBackendClient) legacyInit( ctx context.Context, c *client.Client, backendID string, ) (*mcp.ServerCapabilities, error) { - caps, err := initializeClient(ctx, c) + caps, negotiatedVersion, err := initializeClient(ctx, c) if err != nil { return nil, fmt.Errorf("%w: %w", errLegacyInitFailed, wrapBackendError(err, backendID, "initialize client")) } + if negotiatedVersion == mcpparser.MCPVersionModern { + h.setRevision(backendID, mcpparser.RevisionModern) + } return caps, nil } diff --git a/pkg/vmcp/client/client_test.go b/pkg/vmcp/client/client_test.go index 1f37fc8444..b1935adc2c 100644 --- a/pkg/vmcp/client/client_test.go +++ b/pkg/vmcp/client/client_test.go @@ -1686,6 +1686,9 @@ func TestDefaultClientFactory_SSEForwarding(t *testing.T) { select { case events <- respBytes: case <-time.After(5 * time.Second): + // t.Errorf, not t.Fatal: this runs on the server's own goroutine + // (Fatal's runtime.Goexit would kill the wrong one). + t.Errorf("timed out queuing server/discover fallback response for SSE delivery") } return } @@ -1715,6 +1718,8 @@ func TestDefaultClientFactory_SSEForwarding(t *testing.T) { select { case events <- respBytes: case <-time.After(5 * time.Second): + // t.Errorf, not t.Fatal: this runs on the server's own goroutine. + t.Errorf("timed out queuing initialize response for SSE delivery") } }) mux.HandleFunc("/sse", func(w http.ResponseWriter, r *http.Request) { @@ -1771,7 +1776,7 @@ func TestDefaultClientFactory_SSEForwarding(t *testing.T) { // corresponding handler is installed. initCtx, cancel := context.WithTimeout(t.Context(), 10*time.Second) defer cancel() - _, err = initializeClient(initCtx, c) + _, _, err = initializeClient(initCtx, c) require.NoError(t, err) mu.Lock() diff --git a/pkg/vmcp/client/modern.go b/pkg/vmcp/client/modern.go index c5bfb9a7ae..7b1e6b3133 100644 --- a/pkg/vmcp/client/modern.go +++ b/pkg/vmcp/client/modern.go @@ -13,6 +13,7 @@ import ( "io" "maps" "net/http" + "slices" "strings" "sync/atomic" @@ -63,11 +64,13 @@ var errLegacyResponseBody = errors.New("backend returned a Legacy-shaped body (n var errModernInputRequired = errors.New("modern response requires additional input (multi-round retrieval unsupported)") // errModernProtocolError wraps a well-formed JSON-RPC error whose code is one of -// the Modern-specific codes (-32020/-32021/-32022): the peer validated our -// Modern headers/_meta and rejected them, so it IS Modern even though the call -// failed. It is a positive Modern signal, distinct from errWrongEra and from a -// generic JSON-RPC error (-32600/-32603, which do not prove Modern). probeRevision -// classifies it as Modern. +// the Modern-specific codes (-32020/-32021 unconditionally; -32022 only when +// classifyUnsupportedProtocolVersion finds the peer still advertises +// MCPVersionModern): the peer validated our Modern headers/_meta and rejected +// them, so it IS Modern even though the call failed. It is a positive Modern +// signal, distinct from errWrongEra and from a generic JSON-RPC error +// (-32600/-32603, which do not prove Modern). probeRevision classifies it as +// Modern. var errModernProtocolError = errors.New("modern backend rejected the request with a Modern protocol error") // errModernAuth is returned for an HTTP 401/403 to a Modern request (auth @@ -83,22 +86,34 @@ var errModernAuth = errors.New("modern backend returned an auth status") // outage must not be mistaken for a not-Modern signal. var errModernTransient = errors.New("modern backend returned a transient error") -// errModernNegotiatedDown is returned by modernDiscover when the backend -// answers server/discover with a well-formed Modern envelope (err == nil from -// modernCall) whose supportedVersions does NOT contain mcpparser.MCPVersionModern -// (including an absent or empty list). Per SEP-2575 (and go-sdk's own reference -// client, mcp/client.go:428-444), supportedVersions — not a clean discover -// response alone — is the authoritative signal of whether a peer actually -// speaks the Modern (2026-07-28) revision: a go-sdk v1.7 shim answers -// server/discover even for a backend negotiating down to Legacy, so a clean -// discover response is NOT by itself proof of Modern. +// errModernNegotiatedDown has two sources, both carrying a valid Modern +// envelope whose advertised versions do NOT include mcpparser.MCPVersionModern: +// +// 1. modernDiscover / discoverModernCapabilities: a well-formed server/discover +// response (err == nil from modernCall) whose supportedVersions omits it +// (including an absent or empty list). +// 2. classifyUnsupportedProtocolVersion: a -32022 (CodeUnsupportedProtocolVersion) +// JSON-RPC error whose `data.supported` list omits it (including an absent +// or undecodable data payload). +// +// Per SEP-2575 (and go-sdk's own reference client, mcp/client.go:360-369), +// the advertised version list — not a clean discover response or a bare +// -32022 alone — is the authoritative signal of whether a peer actually +// speaks the Modern (2026-07-28) revision: a go-sdk v1.7 shim answers both +// server/discover and protocol errors even for a backend negotiating down to +// Legacy, so neither on its own is proof of Modern. // // This is a definitive Legacy signal carried in a valid Modern envelope — // distinct from errWrongEra (peer does not speak Modern's wire shape at all) -// and from the other Modern-positive/inconclusive sentinels. Discover is -// read-only (no side effects), so retrying under a corrected Legacy -// classification is always safe. -var errModernNegotiatedDown = errors.New("modern backend negotiated down: supportedVersions lacks 2026-07-28") +// and from the other Modern-positive/inconclusive sentinels. Retrying under +// the corrected Legacy classification is always safe, for different reasons +// per source: (1) is inherently safe, since server/discover has no side +// effects; (2) is reached from interpretModernResult for ANY method, including +// side-effecting ones like tools/call, but go-sdk's ServerSession.handle +// (mcp/server.go) rejects an unsupported per-request protocol version BEFORE +// the switch that dispatches to a method handler, so the backend provably +// never executed the request regardless of which method was called. +var errModernNegotiatedDown = errors.New("modern backend negotiated down: advertised versions lack 2026-07-28") // modernRequestID supplies monotonically increasing JSON-RPC request ids. Each // modernCall is a single request/response, so the id only has to be unique @@ -108,11 +123,12 @@ var modernRequestID atomic.Int64 // modernCall issues a single MCP 2026-07-28 ("Modern") stateless JSON-RPC request // over HTTP POST and decodes the Modern response envelope into out. // -// It hand-rolls the Modern wire shape that mcpcompat/go-sdk v1.6.1 cannot express -// (its only no-initialize primitive is the private, Legacy-shaped resumeCall), -// mirroring the server envelope in pkg/vmcp/server/modern_envelope.go: no -// initialize handshake, no Mcp-Session-Id, protocol metadata carried per-request -// in _meta, and a Mcp-Method header on every call. +// It hand-rolls the Modern wire shape that mcpcompat's public Client API still +// cannot express, even now that the underlying go-sdk is v1.7.0-pre.3 (its only +// no-initialize primitive is the private, Legacy-shaped resumeCall), mirroring +// the server envelope in pkg/vmcp/server/modern_envelope.go: no initialize +// handshake, no Mcp-Session-Id, protocol metadata carried per-request in +// _meta, and a Mcp-Method header on every call. // // params may carry a caller "_meta"; its three reserved io.modelcontextprotocol/* // keys are stripped and vMCP's authoritative values overlaid last (vMCP, not the @@ -170,11 +186,7 @@ func modernCall( // Mcp-Method is required on EVERY Modern request (ValidateHeaderConsistency). req.Header.Set("Mcp-Method", method) if name != "" && mcpparser.IsNameRequiredMethod(method) { - // NOTE: sent raw; non-ASCII identifiers are not sentinel-encoded yet. - // URIs and ASCII names are safe; a non-ASCII name is unspecified behavior - // (a strict peer MAY reject or misinterpret the header). Add =?base64?..?= - // encoding if such names appear. - req.Header.Set("Mcp-Name", name) + req.Header.Set("Mcp-Name", mcpparser.EncodeSentinelName(name)) } // Mcp-Session-Id is deliberately never set: Modern is stateless. @@ -211,6 +223,9 @@ func interpretModernResult(result json.RawMessage, rpcErr *modernRPCError, metho if isModernProtocolCode(rpcErr.Code) { return fmt.Errorf("%w: %s (rpc code %d)", errModernProtocolError, rpcErr.Message, rpcErr.Code) } + if int64(rpcErr.Code) == mcpparser.CodeUnsupportedProtocolVersion { + return classifyUnsupportedProtocolVersion(rpcErr) + } return fmt.Errorf("modern %s: rpc error %d: %s", method, rpcErr.Code, rpcErr.Message) } @@ -243,14 +258,14 @@ func interpretModernResult(result json.RawMessage, rpcErr *modernRPCError, metho // mergeModernMeta strips the reserved io.modelcontextprotocol/* keys from a // caller-supplied _meta (if any) and overlays vMCP's authoritative values last. -// The caller's _meta is never mutated (maps.Clone). +// The caller's _meta is never mutated (StripReservedModernMeta clones it). func mergeModernMeta(callerMeta any) map[string]any { - meta := map[string]any{} - if m, ok := callerMeta.(map[string]any); ok { - meta = maps.Clone(m) - for _, k := range mcpparser.ReservedModernMetaKeys { - delete(meta, k) - } + m, _ := callerMeta.(map[string]any) + meta := mcpparser.StripReservedModernMeta(m) + if meta == nil { + // StripReservedModernMeta returns nil for empty/nil input; this needs a + // non-nil map to overlay vMCP's authoritative values onto below. + meta = map[string]any{} } for k, v := range mcpparser.ModernRequestMeta(modernClientName, versions.Version) { meta[k] = v @@ -258,10 +273,14 @@ func mergeModernMeta(callerMeta any) map[string]any { return meta } -// modernRPCError is the JSON-RPC error object. +// modernRPCError is the JSON-RPC error object. Data carries the SEP-2575 +// error-specific payload (e.g. UnsupportedProtocolVersionData for -32022, +// classified by classifyUnsupportedProtocolVersion); absent for the other +// Modern-specific codes. type modernRPCError struct { - Code int `json:"code"` - Message string `json:"message"` + Code int `json:"code"` + Message string `json:"message"` + Data json.RawMessage `json:"data,omitempty"` } // modernRPCEnvelope is the outer JSON-RPC response envelope. Method is set only @@ -364,17 +383,47 @@ func modernIDMatches(raw json.RawMessage, wantID int64) bool { return json.Unmarshal(raw, &n) == nil && n == wantID } -// isModernProtocolCode reports whether code is one of the Modern-specific -// JSON-RPC error codes (header/meta validation, -3202x). Only these prove the -// peer is Modern; generic JSON-RPC codes (-32600/-32601/-32603) do not, since a -// Legacy backend also returns them. +// isModernProtocolCode reports whether code is one of the two Modern-specific +// JSON-RPC error codes that unconditionally prove the peer is Modern +// (-32020/-32021: header/meta validation). Generic JSON-RPC codes +// (-32600/-32601/-32603) do not, since a Legacy backend also returns them. +// +// CodeUnsupportedProtocolVersion (-32022) is deliberately excluded: unlike the +// other two, it does not by itself prove Modern — it means "I don't support +// the version you asked for" and can equally mean the peer negotiated down to +// Legacy. See classifyUnsupportedProtocolVersion, which resolves it by +// decoding the error's advertised supported-versions list. func isModernProtocolCode(code int) bool { switch int64(code) { - case mcpparser.CodeHeaderMismatch, - mcpparser.CodeMissingClientCapability, - mcpparser.CodeUnsupportedProtocolVersion: + case mcpparser.CodeHeaderMismatch, mcpparser.CodeMissingClientCapability: return true default: return false } } + +// classifyUnsupportedProtocolVersion resolves a -32022 +// (CodeUnsupportedProtocolVersion) error into a Modern-positive or +// negotiated-down signal. go-sdk v1.7's reference client (mcp/client.go:360-369) +// decodes the error's `data.supported` list and retries Modern when it finds a +// mutually-supported version >= 2026-07-28 (a range check), falling back to +// Legacy initialize otherwise. This diverges from that: it exact-matches +// MCPVersionModern instead of a range, same as discoverModernCapabilities' +// exact-match (client.go) and safe for the same reason — vMCP's shim only +// speaks that one Modern wire shape, and a dual-era peer offering a future +// Modern revision also lists 2025-11-25 for us to fall back to. TRIPWIRE: when +// a newer Modern revision is added, this must become a set/range check +// alongside discoverModernCapabilities' — this is the second exact-match site. +// +// Otherwise — including an absent or undecodable data payload — it is +// errModernNegotiatedDown. +func classifyUnsupportedProtocolVersion(rpcErr *modernRPCError) error { + var data struct { + Supported []string `json:"supported"` + } + _ = json.Unmarshal(rpcErr.Data, &data) // best-effort; empty Supported on any decode failure + if slices.Contains(data.Supported, mcpparser.MCPVersionModern) { + return fmt.Errorf("%w: %s (rpc code %d)", errModernProtocolError, rpcErr.Message, rpcErr.Code) + } + return fmt.Errorf("%w: data.supported=%v", errModernNegotiatedDown, data.Supported) +} diff --git a/pkg/vmcp/client/reclassify_test.go b/pkg/vmcp/client/reclassify_test.go index 7b0dcb2e98..d446f07ffe 100644 --- a/pkg/vmcp/client/reclassify_test.go +++ b/pkg/vmcp/client/reclassify_test.go @@ -49,6 +49,10 @@ func TestIsRevisionMismatch(t *testing.T) { fmt.Errorf("wrap: %w", mcp.ErrMethodNotFound), false}, {"modern errLegacySSE (not a modern signal) -> NOT mismatch", mcpparser.RevisionModern, fmt.Errorf("wrap: %w", transport.ErrLegacySSEServer), false}, + {"modern negotiated-down -> mismatch", mcpparser.RevisionModern, + fmt.Errorf("wrap: %w", errModernNegotiatedDown), true}, + {"legacy negotiated-down (not a legacy signal) -> NOT mismatch", mcpparser.RevisionLegacy, + fmt.Errorf("wrap: %w", errModernNegotiatedDown), false}, {"auth: ErrUnauthorized -> NOT mismatch", mcpparser.RevisionLegacy, transport.ErrUnauthorized, false}, {"auth: ErrAuthorizationRequired -> NOT mismatch", mcpparser.RevisionLegacy, transport.ErrAuthorizationRequired, false}, {"auth: ErrUpstreamTokenNotFound -> NOT mismatch", mcpparser.RevisionLegacy, authtypes.ErrUpstreamTokenNotFound, false}, @@ -299,24 +303,24 @@ func TestReclassify_WarnsOnlyOnActualChange(t *testing.T) { assert.Empty(t, buf.String(), "a no-op re-probe must not WARN") } -// TestListCapabilities_MisCachedLegacy_StillSucceedsViaSDKNegotiation: a backend +// TestListCapabilities_MisCachedLegacy_SelfHealsViaSDKNegotiation: a backend // pinned Legacy by a stale cache is still queried successfully, because the // underlying go-sdk v1.7 client (used by the Legacy/session-based code path, // legacyListCapabilities) is itself Modern-first (SEP-2575): its Connect() tries // server/discover before any legacy initialize, so a genuinely Modern backend -// answers correctly even when dispatch hands it the stale Legacy revision. +// answers correctly even when dispatch hands it the stale Legacy revision. The +// call succeeds on the first attempt, so dispatch's reclassify-on-mismatch path +// (which only fires on error; see dispatch's doc comment) is never entered — +// but legacyInit reads the genuinely-negotiated protocol version off the +// Initialize result and flips the cache to Modern in-band regardless (see the +// revisions field doc in client.go). // -// This differs from the pre-v1.7 behavior (and this test's original name/intent): -// back then, a Legacy attempt against a Modern-only backend failed at the -// initialize step (errLegacyInitFailed+ErrLegacySSEServer), which dispatch -// detected as a mismatch and corrected via reclassify+retry. Under v1.7 that -// failure no longer occurs — the call now succeeds on the first attempt via the -// SDK's own negotiation — so dispatch's mismatch path is never entered and the -// cache is NOT flipped to Modern (dispatch only reclassifies on error; see its -// doc comment). This is a bookkeeping staleness, not a wire-protocol -// correctness issue: the backend still receives the correct (Modern) protocol -// on every call regardless of what this repo's revision cache says. -func TestListCapabilities_MisCachedLegacy_StillSucceedsViaSDKNegotiation(t *testing.T) { +// The staleness this closes is not purely internal bookkeeping: CachedRevision +// surfaces it to health status (pkg/vmcp/health/status.go:260), which +// republishes it on every health tick into the mcpRevision CRD status field — +// so before this self-heal, an operator reading that field would see "legacy" +// for a backend actually being driven Modern on every call. +func TestListCapabilities_MisCachedLegacy_SelfHealsViaSDKNegotiation(t *testing.T) { t.Parallel() srv := modernDiscoverServer(t) @@ -329,8 +333,31 @@ func TestListCapabilities_MisCachedLegacy_StillSucceedsViaSDKNegotiation(t *test require.Len(t, caps.Tools, 1) assert.Equal(t, "echo", caps.Tools[0].Name) - // The cache is unchanged: no error occurred to trigger dispatch's - // reclassify-on-mismatch path (see doc comment above). + // legacyInit flips the cache directly off the negotiated Initialize result, + // without needing an error to trigger dispatch's reclassify path. rev, _ := h.cachedRevision(target.WorkloadID) - assert.Equal(t, mcpparser.RevisionLegacy, rev) + assert.Equal(t, mcpparser.RevisionModern, rev) +} + +// TestLegacyInit_SelfHealsRevisionCache exercises legacyInit directly (rather +// than through the full ListCapabilities/query pipeline) to isolate the +// self-heal contract: a mis-cached Legacy backend that is genuinely Modern +// ends up cached Modern after a single Legacy-path Initialize call. +func TestLegacyInit_SelfHealsRevisionCache(t *testing.T) { + t.Parallel() + + srv := modernDiscoverServer(t) + h := newProbeClient(t) + target := &vmcp.BackendTarget{WorkloadID: "b", BaseURL: srv.URL, TransportType: "streamable-http"} + h.setRevision(target.WorkloadID, mcpparser.RevisionLegacy) // mis-cached + + c, err := h.clientFactory(context.Background(), target, false) + require.NoError(t, err) + t.Cleanup(func() { _ = c.Close() }) + + _, err = h.legacyInit(context.Background(), c, target.WorkloadID) + require.NoError(t, err, "the SDK client negotiates Modern transparently on the Legacy path") + + rev, _ := h.cachedRevision(target.WorkloadID) + assert.Equal(t, mcpparser.RevisionModern, rev) } diff --git a/pkg/vmcp/client/revision_test.go b/pkg/vmcp/client/revision_test.go index c33fdc5002..2f8e50603a 100644 --- a/pkg/vmcp/client/revision_test.go +++ b/pkg/vmcp/client/revision_test.go @@ -126,11 +126,29 @@ func TestProbeRevision_TruthTable(t *testing.T) { wantRev: mcpparser.RevisionLegacy, }, { - name: "recognized Modern protocol error (-32022) -> Modern", + // -32022 (CodeUnsupportedProtocolVersion) alone does NOT prove Modern — + // it means "I don't support the version you asked for", which a backend + // negotiating down to Legacy also returns. Only when `data.supported` + // still lists 2026-07-28 does it prove Modern (see the next case); an + // absent data payload must classify Legacy, mirroring go-sdk's own + // reference client falling back to a Legacy initialize here. + name: "-32022 with no data.supported -> Legacy", handler: func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"error":{"code":-32022,"message":"unsupported version"}}`)) }, + wantRev: mcpparser.RevisionLegacy, + }, + { + // -32022 whose data.supported still lists 2026-07-28 proves the peer is + // Modern (it validated our Modern _meta and is merely picky about the + // exact requested version), so it must classify Modern despite the error. + name: "-32022 with data.supported including 2026-07-28 -> Modern", + handler: func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"jsonrpc":"2.0","id":1,"error":{"code":-32022,"message":"unsupported version",` + + `"data":{"supported":["2026-07-28"],"requested":"2099-01-01"}}}`)) + }, wantRev: mcpparser.RevisionModern, }, { @@ -209,6 +227,31 @@ func TestProbeRevision_TruthTable(t *testing.T) { } } +// TestProbeRevision_SSEGate verifies an "sse" TransportType target classifies +// Legacy without ever attempting the Modern server/discover probe: TransportType +// == "sse" names the deprecated 2024-11-05 two-endpoint transport, which has no +// Modern endpoint to discover at this BaseURL (see probeRevision's doc comment). +// The handler fails the test if it is ever hit, proving no network call is made. +func TestProbeRevision_SSEGate(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) { + t.Errorf("probeRevision must not make a network call for an sse target") + })) + t.Cleanup(srv.Close) + + h := newProbeClient(t) + target := &vmcp.BackendTarget{WorkloadID: "sse-backend", BaseURL: srv.URL, TransportType: "sse"} + + rev, err := h.probeRevision(context.Background(), target) + require.NoError(t, err) + assert.Equal(t, mcpparser.RevisionLegacy, rev) + + cached, ok := h.cachedRevision(target.WorkloadID) + require.True(t, ok) + assert.Equal(t, mcpparser.RevisionLegacy, cached) +} + // TestProbeRevision_TransientLeavesUnprobed verifies a dead backend (connection // refused) is INCONCLUSIVE: probeRevision returns the error uncached rather than // caching Legacy, so a transient outage cannot poison the revision cache and the diff --git a/pkg/vmcp/client/schema_ingestion_regression_test.go b/pkg/vmcp/client/schema_ingestion_regression_test.go index e44f18d1c3..1686b19ae0 100644 --- a/pkg/vmcp/client/schema_ingestion_regression_test.go +++ b/pkg/vmcp/client/schema_ingestion_regression_test.go @@ -17,6 +17,7 @@ import ( "github.com/stacklok/toolhive-core/mcpcompat/client" mcptransport "github.com/stacklok/toolhive-core/mcpcompat/client/transport" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/vmcp" ) @@ -129,6 +130,11 @@ func TestRegression_ToolSchemaFidelity_PreservesCompositors(t *testing.T) { BaseURL: srv.URL, TransportType: "streamable-http", } + // This test is about schema ingestion, not revision probing: pin Legacy + // directly rather than relying on the fake's catch-all default arm + // (`case default: ... Result: "{}"`) to incidentally classify it that way, + // and skip the wasted server/discover probe round-trip. + h.setRevision(target.WorkloadID, mcpparser.RevisionLegacy) caps, err := h.ListCapabilities(context.Background(), target) require.NoError(t, err) diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index fe7d920a7f..2efa391bec 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -298,10 +298,12 @@ func (s *Server) dispatchModernPromptGet( // directly from parsed.Params. It mirrors go-sdk's CompleteParams/ // CompleteReference/CompleteParamsArgument/CompleteContext field-for-field // (protocol.go:577-648 in go-sdk@v1.7.0-pre.3) rather than reusing an SDK -// type: mcp-go v1.6.1 (ToolHive's import) predates the Modern completion -// shapes, so there is nothing to reuse. It stays local rather than adding -// JSON tags to vmcp.CompletionRef -- the domain type intentionally carries no -// wire coupling (anti-pattern #5, no mcp-go types crossing the core boundary). +// type: mcpcompat/mcp (ToolHive's import, now built on go-sdk v1.7.0-pre.3) +// carries an equivalent CompleteParams, but its Ref field is untyped (any), so +// there is no strongly-typed Modern reference shape to decode into directly. +// It stays local rather than adding JSON tags to vmcp.CompletionRef -- the +// domain type intentionally carries no wire coupling (anti-pattern #5, no +// mcp-go types crossing the core boundary). type modernCompleteWireParams struct { Ref *struct { Type string `json:"type"` diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index c726e7e0fe..7d9e57a23f 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -19,14 +19,14 @@ import ( // This file hand-rolls the MCP 2026-07-28 ("Modern") stateless response // envelope: the wire shape go-sdk@v1.7.0-pre.3 produces for a single-shot // tools/list, resources/list, resources/templates/list, prompts/list, -// tools/call, resources/read, and prompts/get. ToolHive imports go-sdk -// v1.6.1, whose Modern types don't exist (or are unexported), so this is a -// durable parallel serializer, not a stopgap: resultType and _meta.serverInfo -// are set by unexported SDK functions (setCompleteResultType, -// annotateServerInfo) that run inside the exact ServerSession dispatch this -// package bypasses for Modern stateless requests. A future go-sdk bump cannot -// just delete these structs and marshal SDK result types directly -- the -// Modern annotations would vanish with them. +// tools/call, resources/read, and prompts/get. Even in that version -- the one +// mcpcompat (ToolHive's go-sdk import path) now depends on -- these Modern +// types remain unexported, so this is a durable parallel serializer, not a +// stopgap: resultType and _meta.serverInfo are set by unexported SDK functions +// (setCompleteResultType, annotateServerInfo) that run inside the exact +// ServerSession dispatch this package bypasses for Modern stateless requests. +// A future go-sdk bump cannot just delete these structs and marshal SDK result +// types directly -- the Modern annotations would vanish with them. // // modernResultTypeComplete is the sole value dispatchModern ever needs: this // package only performs single-shot dispatch, never the elicitation retry @@ -36,8 +36,9 @@ import ( const modernResultTypeComplete = "complete" // modernServerInfoKey is the go-sdk's MetaKeyServerInfo -// (protocol.go:2367 in go-sdk@v1.7.0-pre.3), reproduced by hand since v1.6.1 -// does not export it. +// (protocol.go:2367 in go-sdk@v1.7.0-pre.3), reproduced by hand since +// mcpcompat/mcp (ToolHive's mcp import) does not re-export it, even though +// go-sdk itself does. const modernServerInfoKey = "io.modelcontextprotocol/serverInfo" // modernServerInfo mirrors the go-sdk's Implementation type. diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index 9e53ef5dfd..df991b69ed 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -261,7 +261,7 @@ func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) // initialize, violating MCP 2025-11-25's "Servers that support prompts MUST // declare the prompts capability during initialization". // - // R1 (verified against toolhive-core v0.0.32 / go-sdk v1.6.1): go-sdk's own + // R1 (verified against toolhive-core v0.0.34 / go-sdk v1.7.0-pre.3): go-sdk's own // AddTool/RemoveTools (and the resource/prompt equivalents) call a shared // changeAndNotify that debounces (10ms) and then broadcasts the // notification to EVERY session currently connected to this *gosdk.Server diff --git a/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go b/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go index ab325a378a..5832f02664 100644 --- a/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go +++ b/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go @@ -70,7 +70,7 @@ type fakeBackend struct { // metaByMethod records the raw params._meta keyed by JSON-RPC method name. // Tests asserting outbound W3C trace context propagation (SEP-414) use - // metaFor(method) to inspect the _meta a backend actually saw on the wire. + // toolCallMeta() to inspect the _meta a backend actually saw on the wire. metaByMethod map[string]json.RawMessage } @@ -122,13 +122,14 @@ func (f *fakeBackend) headersFor(method string) http.Header { return h.Clone() } -// metaFor returns the raw params._meta recorded for the most recent JSON-RPC -// request with the given method, or nil if no such request was seen or it -// carried no _meta field. -func (f *fakeBackend) metaFor(method string) json.RawMessage { +// toolCallMeta returns the raw params._meta recorded for the most recent +// tools/call request, or nil if no such request was seen or it carried no +// _meta field. metaByMethod is keyed by method, but every test so far only +// inspects tools/call; add a sibling accessor if another method needs one. +func (f *fakeBackend) toolCallMeta() json.RawMessage { f.mu.Lock() defer f.mu.Unlock() - return f.metaByMethod[method] + return f.metaByMethod[string(mcp.MethodToolsCall)] } // handle implements the JSON-RPC subset needed for backend init. The diff --git a/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go b/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go index 176bc04e83..714f2b9303 100644 --- a/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go +++ b/pkg/vmcp/session/internal/backend/mcp_session_meta_propagation_test.go @@ -6,6 +6,7 @@ package backend import ( "context" "encoding/json" + "maps" "testing" "time" @@ -16,6 +17,7 @@ import ( sdktrace "go.opentelemetry.io/otel/sdk/trace" "github.com/stacklok/toolhive-core/mcpcompat/mcp" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/vmcp" ) @@ -72,7 +74,7 @@ func TestHTTPSession_CallTool_InjectsTraceparent(t *testing.T) { //nolint:parall _, err := sess.CallTool(spanCtx, "echo", map[string]any{}, nil) require.NoError(t, err) - raw := fb.metaFor(string(mcp.MethodToolsCall)) + raw := fb.toolCallMeta() require.NotEmpty(t, raw, "expected params._meta to be sent") var meta map[string]any @@ -101,7 +103,7 @@ func TestHTTPSession_CallTool_InjectsTraceparent(t *testing.T) { //nolint:parall _, err := sess.CallTool(spanCtx, "echo", map[string]any{}, map[string]any{"progressToken": "tok"}) require.NoError(t, err) - raw := fb.metaFor(string(mcp.MethodToolsCall)) + raw := fb.toolCallMeta() require.NotEmpty(t, raw, "expected params._meta to be sent") var meta map[string]any @@ -119,7 +121,71 @@ func TestHTTPSession_CallTool_InjectsTraceparent(t *testing.T) { //nolint:parall _, err := sess.CallTool(context.Background(), "echo", map[string]any{}, nil) require.NoError(t, err) - raw := fb.metaFor(string(mcp.MethodToolsCall)) + raw := fb.toolCallMeta() assert.Empty(t, raw, "expected no _meta to be sent without an active trace context") }) } + +// TestHTTPSession_CallTool_StripsReservedModernMeta pins the session-path +// counterpart to pkg/vmcp/client's TestLegacyCallTool_StripsReservedMeta_RealBackend: +// mcp_session.go's CallTool (mcp_session.go:211) must strip the reserved +// io.modelcontextprotocol/* _meta keys before forwarding a caller's _meta to +// this Legacy (session-based, stateful) backend — go-sdk v1.7 hard-rejects any +// _meta.protocolVersion on a stateful server regardless of its value — while +// non-reserved caller keys (a custom key and progressToken) survive, and the +// caller-supplied map is never mutated. +func TestHTTPSession_CallTool_StripsReservedModernMeta(t *testing.T) { + t.Parallel() + + fb := &fakeBackend{advertiseTools: true, tools: []mcp.Tool{{Name: "echo"}}} + url := newFakeBackend(t, fb) + + target := &vmcp.BackendTarget{ + WorkloadID: "strip-meta-backend", + WorkloadName: "strip-meta-backend", + BaseURL: url, + TransportType: "streamable-http", + } + + registry := newTestRegistry(t) + connector := NewHTTPConnector(registry) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + sess, _, err := connector(ctx, target, nil, "", nil) + require.NoError(t, err) + t.Cleanup(func() { _ = sess.Close() }) + + // Values are scalars (not nested maps/objects) even for the reserved keys: + // their exact shape is irrelevant here (they get stripped either way), and + // keeping the fixture flat lets maps.Clone's shallow copy below prove real + // immutability instead of merely catching top-level key deletion. + callerMeta := map[string]any{ + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/clientInfo": "downstream-modern-client", + "io.modelcontextprotocol/clientCapabilities": "none", + "custom-caller-key": "custom-value", + "progressToken": "tok", + } + // Snapshot before the call to prove the caller's own map is untouched after + // it (go-style.md: copy before mutating caller input). + wantCallerMeta := maps.Clone(callerMeta) + + _, err = sess.CallTool(ctx, "echo", map[string]any{}, callerMeta) + require.NoError(t, err) + + assert.Equal(t, wantCallerMeta, callerMeta, "the caller-supplied _meta map must not be mutated") + + raw := fb.toolCallMeta() + require.NotEmpty(t, raw, "expected params._meta to be sent") + + var gotMeta map[string]any + require.NoError(t, json.Unmarshal(raw, &gotMeta)) + + for _, k := range mcpparser.ReservedModernMetaKeys { + assert.NotContains(t, gotMeta, k, "reserved Modern _meta key %q must be stripped before the Legacy hop", k) + } + assert.Equal(t, "custom-value", gotMeta["custom-caller-key"], "non-reserved caller _meta must survive") + assert.Equal(t, "tok", gotMeta["progressToken"], "progressToken must survive") +}