From decd0433fcda023f933eaacdfe23a99d8c882eb5 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sat, 25 Jul 2026 21:51:24 +0200 Subject: [PATCH 1/5] Classify -32022 and SSE targets as Legacy Two backend-revision signals disagreed with the supportedVersions rule adopted for go-sdk v1.7, each able to strand a backend on the wrong path. A -32022 (CodeUnsupportedProtocolVersion) was treated as proof of Modern. It means the opposite: the peer does not support the version we asked for. A backend answering it was cached Modern permanently and enumerated zero capabilities, since isRevisionMismatch never reclassified it. Decode the error's data.supported list instead and keep it Modern-positive only when 2026-07-28 is still advertised, mirroring go-sdk's own reference client (mcp/client.go), whose test for this case is named "unsupported protocol version falls back to initialize". -32020/-32021 stay unconditionally Modern: both require the peer to have parsed Modern _meta. probeRevision was also transport-blind. TransportType "sse" names the deprecated 2024-11-05 two-endpoint transport, whose BaseURL is the GET-only /sse path, and modernCall is a single-endpoint POST, so no Modern endpoint is reachable there. Classify it Legacy without a probe, which also avoids POSTing into a stream and hanging to the client timeout (errModernTransient is uncached, so that cost recurred on every call). Retrying under the corrected Legacy classification stays safe for tools/call: go-sdk rejects an unsupported per-request protocol version before dispatching to any method handler, so the backend never executed the request. Also record that revision-cache self-healing is asymmetric under v1.7 and that the stale value reaches the mcpRevision CRD field (#5992). Co-Authored-By: Claude Opus 5 --- pkg/vmcp/client/client.go | 47 +++++++++--- pkg/vmcp/client/modern.go | 119 ++++++++++++++++++++++--------- pkg/vmcp/client/revision_test.go | 45 +++++++++++- 3 files changed, 169 insertions(+), 42 deletions(-) diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index 70f71d6021..097ec6a6f9 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 is ASYMMETRIC 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 does NOT: the + // SDK's own client transparently negotiates Modern on the Legacy dispatch + // path, so calls keep succeeding and no error ever reaches reclassify's + // trigger (see TestListCapabilities_MisCachedLegacy_StillSucceedsViaSDKNegotiation + // in reclassify_test.go). The stale Legacy value still surfaces externally + // via CachedRevision -> health status -> the mcpRevision CRD field. Tracked + // as #5992; until fixed, a TTL/periodic re-probe would close this gap. revisions sync.Map // map[string]mcpparser.Revision } @@ -1057,8 +1065,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 +1100,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) diff --git a/pkg/vmcp/client/modern.go b/pkg/vmcp/client/modern.go index c5bfb9a7ae..9ab0a38b49 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 @@ -211,6 +227,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) } @@ -258,10 +277,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 +387,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/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 From b035dc3991ee01264faa6e9921201d586f461210 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sat, 25 Jul 2026 21:51:33 +0200 Subject: [PATCH 2/5] Sync Modern shim comments with go-sdk v1.7 Five comments justified the hand-rolled Modern shim on the grounds that go-sdk v1.6.1 cannot express the Modern wire shape. The repo is now on v1.7.0-pre.3, which implements the revision, so that premise reads as false to anyone who checks it. The shim is still required, but for a narrower reason: mcpcompat's public Client API has no no-initialize primitive, only the private Legacy-shaped resumeCall. Say that instead. The parser integration test explained its SSE behavior by claiming the harness cannot capture an initialize carried over the SSE message channel. That was self-contradictory, since tools/call and resources/read traverse the same channel and the same middleware and are captured. The real mechanism: mcpcompat's SSE server uses go-sdk's NewSSEHandler, whose transport implements no ProtocolVersionSupporter gate, so it advertises 2026-07-28, discover succeeds, and initialize is never sent on that arm. Assert that directly rather than inferring it, and drop the claim that the streamable arm sends no discover -- it does, and is negotiated down. Note also that HTTP+SSE is deprecated but NOT removed by 2026-07-28; what that revision removed is Streamable HTTP's GET stream and sessions. Co-Authored-By: Claude Opus 5 --- pkg/mcp/parser_integration_test.go | 35 +++++++++++++++--------------- pkg/vmcp/server/modern_dispatch.go | 10 +++++---- pkg/vmcp/server/modern_envelope.go | 21 +++++++++--------- pkg/vmcp/server/serve.go | 2 +- 4 files changed, 35 insertions(+), 33 deletions(-) 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/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 From 6151ef10c5da88e8e8030b260350eeb1334d446f Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Sat, 25 Jul 2026 21:51:43 +0200 Subject: [PATCH 3/5] Cover revision gaps in vMCP client tests The errModernNegotiatedDown arm of isRevisionMismatch had no truth-table row despite being the only surviving self-healing direction under go-sdk v1.7, so add both the Modern (mismatch) and Legacy (not a mismatch) cases. The reserved-_meta strip has two call sites but only the httpBackendClient one was tested; add the persistent-session counterpart, asserting reserved keys are stripped, non-reserved caller keys survive, and the caller's map is not mutated. Its fixture uses scalars because maps.Clone is shallow and would not have caught a nested mutation. Two robustness fixes: the SSE fake dropped a queued response after a five second timeout and returned an empty body, turning a failure into a hang against a deadline-free context, so fail loudly instead (both send sites, not just one). And the schema-fidelity test reached its Legacy path only because the fake's catch-all happens to answer discover with a body that lacks resultType; pin the revision so adding a field to that arm cannot silently route the test through the Modern shim and strand the injected clientFactory it exists to exercise. metaFor loses its method parameter (every caller passed tools/call, and a second calling function tripped unparam) and becomes toolCallMeta. Co-Authored-By: Claude Opus 5 --- pkg/vmcp/client/client_test.go | 5 ++ pkg/vmcp/client/reclassify_test.go | 28 +++++--- .../schema_ingestion_regression_test.go | 6 ++ .../backend/mcp_session_capabilities_test.go | 13 ++-- .../mcp_session_meta_propagation_test.go | 72 ++++++++++++++++++- 5 files changed, 104 insertions(+), 20 deletions(-) diff --git a/pkg/vmcp/client/client_test.go b/pkg/vmcp/client/client_test.go index 1f37fc8444..cd298aad11 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) { diff --git a/pkg/vmcp/client/reclassify_test.go b/pkg/vmcp/client/reclassify_test.go index 7b0dcb2e98..0b0dade573 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}, @@ -304,18 +308,20 @@ func TestReclassify_WarnsOnlyOnActualChange(t *testing.T) { // 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 and +// the cache stays Legacy. // -// 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. +// The staleness 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 an operator +// reading that field sees "legacy" for a backend that is actually being driven +// Modern on every call. The wire protocol itself is unaffected (the backend +// always receives the correct, Modern request), and the loss is one-directional: +// a mis-cached Modern->actually-Legacy backend still self-heals via +// errModernNegotiatedDown (see the revisions field doc in client.go). Tracked +// as #5992. func TestListCapabilities_MisCachedLegacy_StillSucceedsViaSDKNegotiation(t *testing.T) { t.Parallel() 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/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") +} From 40689c77b9ca6895c5f07a37654d273ae7e4fea8 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 09:44:11 +0200 Subject: [PATCH 4/5] Self-heal a stale Legacy revision from initialize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The revision cache healed in only one direction. A backend cached Modern that had negotiated down to Legacy recovered via reclassify, but one cached Legacy that had become Modern never did: go-sdk v1.7 negotiates Modern transparently on the Legacy dispatch path, so the call succeeds and the reclassify-on-error trigger never fires. The stale value is not internal bookkeeping — CachedRevision feeds health status and is republished into the mcpRevision CRD field on every tick, so a backend redeployed stateful to stateless reported 2025-11-25 indefinitely. Read the negotiated version off the initialize result instead of waiting for an error that never comes. initializeClient already received it and discarded it, and every Legacy dispatch builds a fresh client, so the mis-routed call itself re-negotiates and the flip costs no extra round trip. The value is genuine on both SDK paths: Connect() stores the discover-negotiated version, and the fallback stores the real initialize response. A backend without server/discover can never yield 2026-07-28, so this cannot promote a genuinely Legacy peer. Closes the Legacy-to-Modern half of #5992; the TTL re-probe now only has to cover whatever remains. Co-Authored-By: Claude Opus 5 --- pkg/vmcp/client/client.go | 46 +++++++++++++++++--------- pkg/vmcp/client/client_test.go | 2 +- pkg/vmcp/client/reclassify_test.go | 53 +++++++++++++++++++++--------- 3 files changed, 68 insertions(+), 33 deletions(-) diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index 097ec6a6f9..8fcffa83f9 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -157,16 +157,16 @@ type httpBackendClient struct { // (every probe agrees). A transient probe failure caches nothing // (probeRevision returns uncached), so a blip cannot pin a revision. // - // Self-healing is ASYMMETRIC under go-sdk v1.7: a mis-cached Modern backend - // that has actually negotiated down to Legacy self-heals via + // 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 does NOT: the - // SDK's own client transparently negotiates Modern on the Legacy dispatch - // path, so calls keep succeeding and no error ever reaches reclassify's - // trigger (see TestListCapabilities_MisCachedLegacy_StillSucceedsViaSDKNegotiation - // in reclassify_test.go). The stale Legacy value still surfaces externally - // via CachedRevision -> health status -> the mcpRevision CRD field. Tracked - // as #5992; until fixed, a TTL/periodic re-probe would close this gap. + // 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 } @@ -841,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, @@ -862,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. @@ -1439,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 cd298aad11..b1935adc2c 100644 --- a/pkg/vmcp/client/client_test.go +++ b/pkg/vmcp/client/client_test.go @@ -1776,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/reclassify_test.go b/pkg/vmcp/client/reclassify_test.go index 0b0dade573..d446f07ffe 100644 --- a/pkg/vmcp/client/reclassify_test.go +++ b/pkg/vmcp/client/reclassify_test.go @@ -303,26 +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. 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 and -// the cache stays Legacy. +// (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). // -// The staleness 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 an operator -// reading that field sees "legacy" for a backend that is actually being driven -// Modern on every call. The wire protocol itself is unaffected (the backend -// always receives the correct, Modern request), and the loss is one-directional: -// a mis-cached Modern->actually-Legacy backend still self-heals via -// errModernNegotiatedDown (see the revisions field doc in client.go). Tracked -// as #5992. -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) @@ -335,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) } From 8ecf86466e8cfe0e87a5a38ebb6b0ec00a4d4f65 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Mon, 27 Jul 2026 09:44:26 +0200 Subject: [PATCH 5/5] Encode Mcp-Name per spec and reuse the strip helper Two small conformance and duplication fixes on the Modern egress. The 2026-07-28 spec requires a client to Base64 sentinel-encode an Mcp-Name value that cannot be safely represented as a plain ASCII header, and to encode any plain-ASCII value that already matches the sentinel pattern so a server does not wrongly decode it. We sent the value raw, with only a comment acknowledging the gap. A non-ASCII tool name reaches a strict peer as unspecified behavior, and a URI containing CR or LF makes Go's transport reject the request outright. decodeSentinelName already existed with no counterpart, so add its exact mirror and use it. mergeModernMeta also hand-rolled the same maps.Clone plus delete loop that StripReservedModernMeta now performs. Two copies of one rule will drift as soon as a fourth reserved key is added, which is a live prospect -- io.modelcontextprotocol/logLevel is a real per-request reserved key we are deliberately deferring, because the key list currently does double duty as the ingress Modern-signal set and enrolling it there would change request classification. Co-Authored-By: Claude Opus 5 --- pkg/mcp/revision.go | 34 ++++++++++++++++++++++++++++++++++ pkg/mcp/revision_test.go | 36 ++++++++++++++++++++++++++++++++++++ pkg/vmcp/client/modern.go | 20 ++++++++------------ 3 files changed, 78 insertions(+), 12 deletions(-) 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/modern.go b/pkg/vmcp/client/modern.go index 9ab0a38b49..7b1e6b3133 100644 --- a/pkg/vmcp/client/modern.go +++ b/pkg/vmcp/client/modern.go @@ -186,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. @@ -262,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