From beb5d3f48cff70bc357cd2c0ee1da2c844ccf0dd Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 19:24:38 +0000 Subject: [PATCH 1/4] Make forwarding tests' Legacy dependency explicit The server->client forwarding integration tests (mid-call elicitation, sampling, progress, logging) exercise a surface that exists only on a Legacy (2025-11-25) session: the 2026-07-28 revision removed server-initiated requests, replacing them with client-polled multi-round retrieval (SEP-2322), and SEP-2577 deprecates sampling and logging outright. Today these tests land on Legacy only incidentally, because the Modern dispatch kill-switch (#5959) rejects the go-sdk client's Modern-first probe; once the switch is removed (#6033) the client negotiates Modern and all of them fail at connect. Pin the downstream test clients to Legacy explicitly via a transport-level RoundTripper that answers server/discover with the same -32022 rejection the kill-switch produces (mcpcompat cannot pin a protocol version, #5911), and pin the Modern-client contract for the same eliciting backend in a new integration test: an explicit JSON-RPC error naming the refused request, never a hang, a fabricated success, or an input_required envelope. Document the client-edge limitation and the intended future MRTR shape (#5743) in the vMCP architecture doc. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam --- docs/arch/10-virtual-mcp-architecture.md | 62 ++++++++ ...forwarding_realbackend_integration_test.go | 138 +++++++++++++++++- .../modern_realbackend_integration_test.go | 49 +++++++ 3 files changed, 245 insertions(+), 4 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 0314859708..09b8f9f881 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -289,6 +289,68 @@ multi-round tool retrieval (MRTR), so the fix is shaped MRTR-first rather than b extending the SSE standalone-stream model — see the epic (#5743) and the mid-call forwarding section below for the Legacy behaviour this contrasts with. +### Limitation: elicitation and sampling are unavailable to Modern clients + +The client edge mirrors the backend edge. The Modern dispatcher +(`pkg/vmcp/server`'s `dispatchModern`) is single-shot: every result it builds is +`resultType: "complete"`, and it never emits `"input_required"` — MRTR +(SEP-2322) is unimplemented on this edge too. When a backend tool issues a +mid-call server-initiated request during a **Modern** client's `tools/call`, +there is no client session to forward it to, so the call fails with an explicit +`-32603` whose message names the refused request (pinned by +`TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly`). This is a +deliberate honest-unsupported error, not a gap left by accident: + +- The 2026-07-28 revision **removed** server-initiated requests; go-sdk's + `ServerSession.assertServerInitiatedRequestAllowed` refuses + elicitation/sampling/roots purely by negotiated protocol version, so no + capability negotiation can restore the Legacy forwarding model for Modern + clients. +- A server that never returns `input_required` is fully SEP-2575-conformant: + the per-request `clientCapabilities` a client declares are an offer the + server may use, not an obligation. +- SEP-2577 deprecates sampling (and logging and roots) outright as of + 2026-07-28, with direct LLM-provider integration as the sanctioned + replacement — so elicitation is the only durable consumer a future MRTR + implementation would serve. + +Legacy clients keep the full mid-call forwarding behaviour unchanged; the +forwarding integration tests pin their downstream clients to Legacy explicitly +(`legacyPinningRoundTripper` in `pkg/vmcp/server`) because that surface exists +only on a Legacy session. + +**Bridging was considered, costed, and rejected — this is a design position, +not a backlog item.** Serving MRTR to Modern clients on top of a *Legacy* +backend would require parking the live, mid-flight backend call server-side +(the blocked goroutine and its open session cannot be serialized into the +opaque `requestState` the SEP designed for handler re-invocation) and keying +the resume on an unguessable token — per-round server state with TTL/eviction, +identity binding on a token that becomes a capability to resume someone else's +in-flight call, and replica affinity with no `Mcp-Session-Id` to route on. That +is a session in disguise: exactly the construct the 2026-07-28 revision +removed. The spec's own sanctioned path for genuinely stateful +`input_required` work is the **Tasks** extension (SEP-1686: `tools/call` +returns a `taskId`; the client polls `tasks/get`/`tasks/result` and answers via +`tasks/input_response`) — if Modern-client elicitation over Legacy backends is +ever truly demanded, that is the machinery to reach for, not parked +`tools/call`. + +The coherent future MRTR shape for a re-aggregating gateway is +**Modern-client ↔ Modern-backend pass-through** — relay a Modern backend's +`inputRequests`/`requestState` to the client and the client's +`inputResponses` back, genuinely stateless at vMCP. It requires the egress +half first (today a Modern backend's `input_required` surfaces as +`errModernInputRequired`, the seam left in `pkg/vmcp/client`), and by the time +Modern backends exist to relay from, SEP-2577's deprecations make elicitation +its only durable consumer; see #5743. + +Progress and log notifications toward Modern clients are a separate concern +from MRTR: they remain spec-legal as request-scoped notifications on the +POST-initiated SSE response stream (SEP-2260 requires messages on that stream +to relate to the originating request; `progressToken` is unchanged), which the +single-shot dispatcher does not produce today — a vMCP streaming-dispatch gap, +not a spec absence. + ## Served MCP Capabilities Beyond tools, vMCP aggregates and serves the full complement of MCP capabilities. Every served capability flows through the domain **core** (`pkg/vmcp/core`), so the same admission decision that filters `tools/list` also gates reads, gets, and completions. diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index 704ab313fd..d477febfb0 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -4,7 +4,10 @@ package server_test import ( + "bytes" "context" + "encoding/json" + "io" "net/http" "net/http/httptest" "sync/atomic" @@ -18,12 +21,18 @@ import ( "github.com/stacklok/toolhive-core/mcpcompat/client/transport" mcpmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" mcpserver "github.com/stacklok/toolhive-core/mcpcompat/server" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" ) // Forwarding integration fixtures. These exercise the server->client forwarding // cluster end-to-end: a real in-process backend, mid tools/call, drives // elicitation/sampling/progress/logging back at its caller; vMCP must relay that -// traffic to the real downstream client on the same session. +// traffic to the real downstream client on the same session. That session is +// necessarily Legacy (2025-11-25): the Modern revision removed server-initiated +// requests, so the downstream clients here are explicitly pinned to Legacy — +// see legacyPinningRoundTripper. The Modern-client behavior for the same +// eliciting backend is asserted separately in +// TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly. const ( fwdElicitTool = "elicit_tool" fwdSampleTool = "sample_tool" @@ -130,9 +139,73 @@ func startForwardingBackend(t *testing.T) string { return ts.URL + "/mcp" } +// legacyPinningRoundTripper pins a downstream mcpcompat client to the Legacy +// (2025-11-25) revision by answering its Modern-first server/discover probe +// with the same HTTP 400 / JSON-RPC -32022 UnsupportedProtocolVersionError +// body vMCP's kill-switch produces (reusing the production +// mcpparser.ClassificationErrorResponse builder), which drives go-sdk's +// Connect down its documented fall-back-to-Legacy-initialize path. Every +// other request passes through to base untouched. +// +// WHY PIN: these fixtures assert the Legacy-only server-initiated surface — +// mid-call elicitation/sampling requests and progress/logging notifications +// relayed over a live session. The 2026-07-28 revision removes server- +// initiated requests entirely (go-sdk's assertServerInitiatedRequestAllowed +// refuses them by protocol version; the replacement is client-polled +// multi-round retrieval, SEP-2322) and SEP-2577 deprecates sampling and +// logging outright. While the Modern dispatch kill-switch (#5959) is on, vMCP +// itself rejects the probe and these clients land on Legacy incidentally; +// once it is removed (#6033) the go-sdk-based client would negotiate Modern +// and this surface would vanish mid-test — failing at connect on +// subscriptions/listen, not at the behavior under test. Pinning makes the +// tests' Legacy dependency explicit instead of incidental. +// +// The pin lives in the transport because mcpcompat documents it cannot set a +// protocol version (go-sdk's ClientSessionOptions.protocolVersion is +// unexported — see the LIMITATION note in mcpcompat/client and #5911). +// Replace this with a real client option if #5911 lands. +// +// LOAD-BEARING after #6033: once the kill-switch is gone, this RoundTripper +// is the ONLY thing keeping these downstream clients on Legacy. It is not +// leftover kill-switch scaffolding — deleting it silently flips every test +// in this file to Modern and voids what they assert. +type legacyPinningRoundTripper struct{ base http.RoundTripper } + +func (rt *legacyPinningRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + if req.Method != http.MethodPost || req.Body == nil { + return rt.base.RoundTrip(req) + } + body, err := io.ReadAll(req.Body) + _ = req.Body.Close() + if err != nil { + return nil, err + } + req.Body = io.NopCloser(bytes.NewReader(body)) + var probe struct { + ID any `json:"id"` + Method string `json:"method"` + } + if json.Unmarshal(body, &probe) == nil && probe.Method == "server/discover" { + return mcpparser.ClassificationErrorResponse(req, probe.ID, &mcpparser.UnsupportedVersionError{ + Requested: mcpparser.MCPVersionModern, + Supported: []string{mcpparser.MCPVersionLegacy}, + }), nil + } + return rt.base.RoundTrip(req) +} + +// legacyPinnedHTTPClient returns an *http.Client for +// transport.WithHTTPBasicClient that pins the mcpcompat client to Legacy — +// see legacyPinningRoundTripper. +func legacyPinnedHTTPClient() *http.Client { + return &http.Client{Transport: &legacyPinningRoundTripper{base: http.DefaultTransport}} +} + // downstreamClient builds a real mcpcompat client against the vMCP endpoint, // wired for server->client traffic (elicitation/sampling handlers, continuous // listening) and collecting forwarded notifications on the returned channel. +// It is pinned to the Legacy revision (see legacyPinningRoundTripper): the +// forwarded traffic it collects exists only on a Legacy session. type downstreamClient struct { c *client.Client notifCh chan mcpmcp.JSONRPCNotification @@ -185,7 +258,10 @@ func newDownstreamClient(ctx context.Context, t *testing.T, vmcpURL string, with c, err := client.NewStreamableHttpClientWithOpts( vmcpURL, - []transport.StreamableHTTPCOption{transport.WithContinuousListening()}, + []transport.StreamableHTTPCOption{ + transport.WithContinuousListening(), + transport.WithHTTPBasicClient(legacyPinnedHTTPClient()), + }, clientOpts, ) require.NoError(t, err) @@ -235,6 +311,11 @@ func (dc *downstreamClient) waitNotification(ctx context.Context, t *testing.T, // TestForwarding_Elicitation_RealBackend verifies a backend's mid-call // elicitation/create is relayed to the downstream client and its response // carried back into the tool result. +// +// Legacy-pinned: a server-initiated elicitation/create toward the client does +// not exist on Modern (2026-07-28) — SEP-2322 replaces it with client-polled +// inputRequests, which vMCP deliberately does not serve (see the "unavailable +// to Modern clients" limitation in docs/arch/10-virtual-mcp-architecture.md). func TestForwarding_Elicitation_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -265,6 +346,11 @@ func TestForwarding_Elicitation_RealBackend(t *testing.T) { // TestForwarding_Sampling_RealBackend verifies a backend's mid-call // sampling/createMessage is relayed to the downstream client and the sampled // message carried back into the tool result. +// +// Legacy-pinned: like elicitation, server-initiated sampling does not exist on +// Modern (SEP-2322) — and SEP-2577 additionally deprecates the sampling +// feature itself as of 2026-07-28 (sanctioned replacement: direct LLM-provider +// integration), so this surface is Legacy-only by spec, not by omission. func TestForwarding_Sampling_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -288,6 +374,17 @@ func TestForwarding_Sampling_RealBackend(t *testing.T) { // TestForwarding_Progress_RealBackend verifies a backend's mid-call // notifications/progress is relayed to the downstream client, arriving before // the tool result is read. +// +// Legacy-pinned — but NOT because Modern lacks a progress channel. It does +// not: on Modern, progress remains spec-legal as a request-scoped +// notification riding the POST-initiated SSE response stream (SEP-2260 +// tightened exactly that channel; progressToken is unchanged in the draft +// schema). What is missing is on vMCP's side: dispatchModern is single-shot — +// writeModernResult emits one JSON body and cannot stream — so nothing riding +// the per-request stream is deliverable to a Modern client today. That is a +// vMCP "streaming Modern dispatch" gap (future work), distinct from both MRTR +// and subscriptions/listen (whose subscribable set is fixed at four +// list-changed/subscription types and excludes progress). func TestForwarding_Progress_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -312,6 +409,22 @@ func TestForwarding_Progress_RealBackend(t *testing.T) { // TestForwarding_Logging_RealBackend verifies that vMCP requests debug logging // from the backend (so it emits) and relays the backend's notifications/message // to the downstream client, which has itself set a logging level. +// +// Legacy-pinned, for three stacked reasons — record them so this is not +// re-litigated as a vMCP bug: +// 1. Upstream go-sdk defect: SetLoggingLevel omits injectRequestMeta +// (client.go:1303 in v1.7.0-pre.3; every other Modern-aware method +// injects), so a Modern logging/setLevel carries the MCP-Protocol-Version +// header but no _meta.protocolVersion. vMCP's ClassifyRevision CORRECTLY +// rejects that with -32020; accepting the malformed request to make this +// test pass would be worse than the failure. Filed upstream separately. +// 2. Even with (1) fixed, Modern log delivery is gated on the per-request +// logLevel _meta key (SEP-2575: absent means the server must not send log +// notifications for that request) and rides the same per-request SSE +// response stream the single-shot dispatcher cannot produce — the same +// streaming gap as TestForwarding_Progress_RealBackend. +// 3. SEP-2577 deprecates the logging feature itself as of 2026-07-28 +// (sanctioned replacement: stderr / OpenTelemetry). func TestForwarding_Logging_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -340,6 +453,14 @@ func TestForwarding_Logging_RealBackend(t *testing.T) { // when the downstream client did not advertise sampling, the backend's // sampling request fails cleanly (a failed tools/call) rather than hanging until // the deadline. +// +// Legacy-pinned even though it happens to pass on Modern: capability-gated +// failure on a live session only exists on Legacy. On Modern the same call +// fails with the sessionless -32603 REGARDLESS of what the client advertises +// (withHandlers makes no difference), so the lenient assertion here would be +// satisfied for a reason unrelated to the gating this test exists to +// exercise. The pin keeps it testing what its name says. (Verified against a +// working subscriptions/listen build: unpinned, this test passes vacuously.) func TestForwarding_Sampling_NoDownstreamCapability(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -365,6 +486,7 @@ func TestForwarding_Sampling_NoDownstreamCapability(t *testing.T) { // TestForwarding_Sampling_NoDownstreamCapability: when the downstream client did // not advertise elicitation, the backend's mid-call elicitation/create fails // cleanly (a failed tools/call) rather than hanging until the deadline. +// Legacy-pinned for the same vacuous-pass reason as its sampling twin above. func TestForwarding_Elicitation_NoDownstreamCapability(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -397,14 +519,18 @@ type samplingClient struct { // newSamplingClient connects a downstream client to vmcpURL whose sampling // handler always answers with the given summary and model (so the tool result // distinguishes which client's handler served the request) and increments a -// per-client counter. +// per-client counter. Pinned to Legacy like newDownstreamClient — see +// legacyPinningRoundTripper. func newSamplingClient(ctx context.Context, t *testing.T, vmcpURL, summary, model string) *samplingClient { t.Helper() sc := &samplingClient{} c, err := client.NewStreamableHttpClientWithOpts( vmcpURL, - []transport.StreamableHTTPCOption{transport.WithContinuousListening()}, + []transport.StreamableHTTPCOption{ + transport.WithContinuousListening(), + transport.WithHTTPBasicClient(legacyPinnedHTTPClient()), + }, []client.ClientOption{ client.WithSamplingHandler(client.SamplingHandlerFunc( func(_ context.Context, _ mcpmcp.CreateMessageRequest) (*mcpmcp.CreateMessageResult, error) { @@ -448,6 +574,10 @@ func newSamplingClient(ctx context.Context, t *testing.T, vmcpURL, summary, mode // call's sampling request were relayed to B's session, A's tool result would show // "summary-B" (mismatching the "summary-A" assertion) and the handler counters // would be lopsided (A=0, B=2 instead of 1/1) — either check trips. +// +// Legacy-pinned for the same reasons as TestForwarding_Sampling_RealBackend: +// per-session routing of a server-initiated request presupposes sessions, +// which Modern removed along with the requests themselves. func TestForwarding_Sampling_RealBackend_SessionIsolation(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go index 58828f44c5..b368206f40 100644 --- a/pkg/vmcp/server/modern_realbackend_integration_test.go +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -306,6 +306,55 @@ func TestIntegration_Modern_RealBackend_UnknownMethod(t *testing.T) { assert.EqualValues(t, -32601, errObj["code"]) } +// TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly pins the +// Modern-client behavior for a backend tool that issues a mid-call +// server-initiated request (elicitation/create, sampling/createMessage): the +// call MUST resolve promptly to an explicit JSON-RPC error naming the refused +// request — never a hang, never a fabricated success, and never a resultType +// "input_required" envelope, which this dispatcher does not emit (client-polled +// multi-round retrieval, SEP-2322, is unimplemented; modernResultTypeComplete +// is the only resultType modern_envelope.go builds). +// +// This is the honest-unsupported contract for the surface the 2026-07-28 +// revision removed: server-initiated requests need a live session, Modern +// requests have none by design, and SEP-2577 additionally deprecates sampling +// outright. The Legacy-session behavior for the same backend tools is covered +// by the TestForwarding_* fixtures, whose downstream clients pin Legacy +// explicitly (see legacyPinningRoundTripper). +func TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly(t *testing.T) { + t.Parallel() + + backendURL := startForwardingBackend(t) + ts := newRealModernTestServer(t, backendURL) + + tests := []struct { + name string + tool string + want string // substring naming the refused server-initiated request + }{ + {name: "elicitation", tool: fwdElicitTool, want: "elicitation/create"}, + {name: "sampling", tool: fwdSampleTool, want: "sampling/createMessage"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{"name": tc.tool}, 1, tc.tool) + defer resp.Body.Close() + + // A -32603 rides HTTP 200 per writeModernError's status mapping: the + // request was accepted and processed; the failure is application-level. + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + require.NotContains(t, decoded, "result", + "must not fabricate a success or an input_required envelope: %+v", decoded) + errObj, ok := decoded["error"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.EqualValues(t, -32603, errObj["code"]) + assert.Contains(t, errObj["message"], tc.want, + "the error must name the refused server-initiated request") + }) + } +} + // TestIntegration_Modern_RealBackend_MalformedArguments verifies a // syntactically valid tools/call whose "arguments" is present but not a JSON // object is rejected with 400 + JSON-RPC -32602, per hasNonObjectArguments' From cb650ae9eace3e5c07b06d1d7017275a854eb983 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 20:26:20 +0000 Subject: [PATCH 2/4] Address panel review findings on test disposition Four-reviewer panel findings on the forwarding-test disposition, applied without changing what the tests disposition: - Correct the pin comment's mechanism claim: vMCP never rejects server/discover (the kill-switch exempts it); it answers 200 with a version-omitting result and the client negotiates down. The RoundTripper substitutes a -32022 rejection to force the same outcome deterministically. - Clone the request in RoundTrip instead of mutating the caller's (net/http RoundTripper contract), and count discover interceptions, asserted after every connect - on a kill-switch base the pin is otherwise behaviorally inert and indistinguishable from dead code. - Name the refused request in the NoDownstreamCapability assertions, closing their vacuous-pass route. - Stop pinning -32603 in the Modern honest-failure test: the spec MUSTs -32021 for the undeclared case, but at HTTP 400, which go-sdk's client escalates to permanent session death - so the code change is deferred to a follow-up serving -32021 at 200 as a documented deviation, and the test asserts the durable contract (explicit error naming the refused request, no input_required, bounded, no hang) rather than the code we know is wrong. The backend-ID leak in the echoed message is characterized with flip-to-NotContains instructions. - Add bounded Modern pins for the two prose-only dispositions: progress is dropped and the call completes (a vMCP streaming-dispatch gap, not a spec absence), and logging/setLevel is -32601 when well-formed and -32020 for the malformed shape go-sdk actually sends. - Give postModern a real deadline so "no hang" is asserted, not implied. - Fix the Tasks citation to SEP-2663 (tasks/get + inputResponses on tasks/update; SEP-1686's blocking tasks/result was removed), rewrite the logging clause that implied setLevel still exists on Modern, and document in the arch doc why the spec-mandated 400 is unshippable and that a clean error does not mean the backend executed nothing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam --- docs/arch/10-virtual-mcp-architecture.md | 40 ++++- ...forwarding_realbackend_integration_test.go | 120 ++++++++++--- .../modern_realbackend_integration_test.go | 165 ++++++++++++++++-- 3 files changed, 279 insertions(+), 46 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 09b8f9f881..0a9dd13569 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -301,6 +301,34 @@ there is no client session to forward it to, so the call fails with an explicit `TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly`). This is a deliberate honest-unsupported error, not a gap left by accident: +**The `-32603` is a documented deviation, not the spec's answer — and the +spec's answer is unshippable today.** For a client that did NOT declare the +needed capability in its per-request `clientCapabilities`, SEP-2575 MUSTs a +`-32021` `MissingRequiredClientCapabilityError` at **HTTP 400**, with +explicitly execution-time language ("if processing a request requires a +capability…") — go-sdk's own doc comment on the error type tells handlers to +return it mid-execution, so the mid-call timing is not the problem. The +problem is the transport: go-sdk's streamable client treats any non-transient +4xx (its transient set is only 500/502/503/504/429) as a **connection** +failure — `checkResponse` → `fail()` → a one-shot, permanent session death — +so a conformant 400 would tear down the entire client session to punish one +call. And for a client that DID declare the capability, the 2026-07-28 +vocabulary has no conformant code at all: no "operation not supported", MRTR +is not a server-advertised capability, and SEP-2322 has no decline mechanism. +The planned follow-up therefore serves `-32021` (with +`data.requiredCapabilities`, and a message naming both the capability and the +gateway limitation) at **HTTP 200** for the undeclared case — deviating from +the mandated 400 for exactly the reason above — and keeps `-32603` for the +declared case as a documented spec gap. Until that lands, both cases surface +as `-32603`. + +**A clean error does not mean nothing happened.** The refusal reaches the +backend mid-call, so a real backend tool may have executed — including side +effects — up to the point it demanded input. A Modern client receiving this +error must not assume the call was side-effect-free. (The integration +fixture's tools elicit as their first action, so the tests cannot exhibit +this; production tools can.) + - The 2026-07-28 revision **removed** server-initiated requests; go-sdk's `ServerSession.assertServerInitiatedRequestAllowed` refuses elicitation/sampling/roots purely by negotiated protocol version, so no @@ -329,11 +357,13 @@ identity binding on a token that becomes a capability to resume someone else's in-flight call, and replica affinity with no `Mcp-Session-Id` to route on. That is a session in disguise: exactly the construct the 2026-07-28 revision removed. The spec's own sanctioned path for genuinely stateful -`input_required` work is the **Tasks** extension (SEP-1686: `tools/call` -returns a `taskId`; the client polls `tasks/get`/`tasks/result` and answers via -`tasks/input_response`) — if Modern-client elicitation over Legacy backends is -ever truly demanded, that is the machinery to reach for, not parked -`tools/call`. +`input_required` work is the **Tasks** extension (SEP-2663: `tools/call` +returns `resultType: "task"` with a `taskId`; the client polls `tasks/get` and +answers outstanding `inputRequests` via `inputResponses` on `tasks/update`; +note SEP-2663 supersedes SEP-1686 and removed the blocking `tasks/result` +method for the same reasons argued here) — if Modern-client elicitation over +Legacy backends is ever truly demanded, that is the machinery to reach for, +not parked `tools/call`. The coherent future MRTR shape for a re-aggregating gateway is **Modern-client ↔ Modern-backend pass-through** — relay a Modern backend's diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index d477febfb0..188296eb0e 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -141,24 +141,32 @@ func startForwardingBackend(t *testing.T) string { // legacyPinningRoundTripper pins a downstream mcpcompat client to the Legacy // (2025-11-25) revision by answering its Modern-first server/discover probe -// with the same HTTP 400 / JSON-RPC -32022 UnsupportedProtocolVersionError -// body vMCP's kill-switch produces (reusing the production -// mcpparser.ClassificationErrorResponse builder), which drives go-sdk's +// with an HTTP 400 / JSON-RPC -32022 UnsupportedProtocolVersionError body +// (built with the production mcpparser.ClassificationErrorResponse so the +// fake cannot drift from the real error shape), which drives go-sdk's // Connect down its documented fall-back-to-Legacy-initialize path. Every // other request passes through to base untouched. // +// MECHANISM, precisely: this is NOT what vMCP itself does. vMCP never rejects +// server/discover — the kill-switch (#5959) explicitly exempts it, so vMCP +// answers HTTP 200 with a discover RESULT whose supportedVersions omits +// 2026-07-28, and the go-sdk client negotiates down from that. The +// RoundTripper substitutes a -32022 rejection to force the same Legacy +// outcome deterministically, independent of what versions the server under +// test advertises. +// // WHY PIN: these fixtures assert the Legacy-only server-initiated surface — // mid-call elicitation/sampling requests and progress/logging notifications // relayed over a live session. The 2026-07-28 revision removes server- // initiated requests entirely (go-sdk's assertServerInitiatedRequestAllowed // refuses them by protocol version; the replacement is client-polled // multi-round retrieval, SEP-2322) and SEP-2577 deprecates sampling and -// logging outright. While the Modern dispatch kill-switch (#5959) is on, vMCP -// itself rejects the probe and these clients land on Legacy incidentally; -// once it is removed (#6033) the go-sdk-based client would negotiate Modern -// and this surface would vanish mid-test — failing at connect on -// subscriptions/listen, not at the behavior under test. Pinning makes the -// tests' Legacy dependency explicit instead of incidental. +// logging outright. While the kill-switch is on, the version-omitting +// discover result pins these clients to Legacy incidentally; once it is +// removed (#6033) the go-sdk-based client would negotiate Modern and this +// surface would vanish mid-test — failing at connect on subscriptions/listen, +// not at the behavior under test. Pinning makes the tests' Legacy dependency +// explicit instead of incidental. // // The pin lives in the transport because mcpcompat documents it cannot set a // protocol version (go-sdk's ClientSessionOptions.protocolVersion is @@ -168,24 +176,40 @@ func startForwardingBackend(t *testing.T) string { // LOAD-BEARING after #6033: once the kill-switch is gone, this RoundTripper // is the ONLY thing keeping these downstream clients on Legacy. It is not // leftover kill-switch scaffolding — deleting it silently flips every test -// in this file to Modern and voids what they assert. -type legacyPinningRoundTripper struct{ base http.RoundTripper } +// in this file to Modern and voids what they assert. The intercepted counter +// (asserted after every connect) exists to catch exactly that: if go-sdk ever +// stops probing server/discover first, the pin becomes dead code and the +// counter assertion fails instead of the tests silently changing meaning. +type legacyPinningRoundTripper struct { + base http.RoundTripper + // intercepted counts server/discover probes answered with the -32022 + // rejection. Asserted > 0 after connect — proof the pin FIRED, not merely + // that it exists. + intercepted atomic.Int32 +} func (rt *legacyPinningRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { if req.Method != http.MethodPost || req.Body == nil { return rt.base.RoundTrip(req) } + // Clone before touching anything: RoundTrippers must not modify the + // caller's request beyond consuming the body (net/http contract; see also + // the copy-before-mutating rule). The body is the one thing a routing + // interceptor must read, so consume it from the original and give the + // clone a fresh reader. body, err := io.ReadAll(req.Body) _ = req.Body.Close() if err != nil { return nil, err } + req = req.Clone(req.Context()) req.Body = io.NopCloser(bytes.NewReader(body)) var probe struct { ID any `json:"id"` Method string `json:"method"` } if json.Unmarshal(body, &probe) == nil && probe.Method == "server/discover" { + rt.intercepted.Add(1) return mcpparser.ClassificationErrorResponse(req, probe.ID, &mcpparser.UnsupportedVersionError{ Requested: mcpparser.MCPVersionModern, Supported: []string{mcpparser.MCPVersionLegacy}, @@ -194,11 +218,26 @@ func (rt *legacyPinningRoundTripper) RoundTrip(req *http.Request) (*http.Respons return rt.base.RoundTrip(req) } -// legacyPinnedHTTPClient returns an *http.Client for -// transport.WithHTTPBasicClient that pins the mcpcompat client to Legacy — +// newLegacyPinnedHTTPClient returns an *http.Client for +// transport.WithHTTPBasicClient that pins the mcpcompat client to Legacy, +// plus the RoundTripper itself so callers can assert the pin actually fired — // see legacyPinningRoundTripper. -func legacyPinnedHTTPClient() *http.Client { - return &http.Client{Transport: &legacyPinningRoundTripper{base: http.DefaultTransport}} +func newLegacyPinnedHTTPClient() (*http.Client, *legacyPinningRoundTripper) { + rt := &legacyPinningRoundTripper{base: http.DefaultTransport} + return &http.Client{Transport: rt}, rt +} + +// requirePinFired asserts the Legacy pin intercepted at least one +// server/discover probe during connect. Without this, the pin is +// indistinguishable from dead code: on a kill-switch base the tests pass +// even with the RoundTripper deleted (the server's own version-omitting +// discover answer pins the client), and if go-sdk ever stopped probing +// server/discover first the tests would silently stop testing what they +// claim. +func requirePinFired(t *testing.T, rt *legacyPinningRoundTripper) { + t.Helper() + require.Positive(t, rt.intercepted.Load(), + "Legacy pin never fired: the client did not probe server/discover; the pin may be dead code") } // downstreamClient builds a real mcpcompat client against the vMCP endpoint, @@ -256,11 +295,12 @@ func newDownstreamClient(ctx context.Context, t *testing.T, vmcpURL string, with ) } + hc, pinRT := newLegacyPinnedHTTPClient() c, err := client.NewStreamableHttpClientWithOpts( vmcpURL, []transport.StreamableHTTPCOption{ transport.WithContinuousListening(), - transport.WithHTTPBasicClient(legacyPinnedHTTPClient()), + transport.WithHTTPBasicClient(hc), }, clientOpts, ) @@ -284,6 +324,7 @@ func newDownstreamClient(ctx context.Context, t *testing.T, vmcpURL string, with }, }) require.NoError(t, err) + requirePinFired(t, pinRT) return dc } @@ -418,9 +459,11 @@ func TestForwarding_Progress_RealBackend(t *testing.T) { // header but no _meta.protocolVersion. vMCP's ClassifyRevision CORRECTLY // rejects that with -32020; accepting the malformed request to make this // test pass would be worse than the failure. Filed upstream separately. -// 2. Even with (1) fixed, Modern log delivery is gated on the per-request -// logLevel _meta key (SEP-2575: absent means the server must not send log -// notifications for that request) and rides the same per-request SSE +// 2. Even with (1) fixed, the RPC no longer exists on Modern: the per-request +// logLevel _meta key REPLACES logging/setLevel (draft schema; go-sdk's +// server answers methodSetLevel with -32601 under 2026-07-28, server.go +// "method removed in the new protocol"). Delivery of notifications/message +// for a request that does carry logLevel rides the per-request SSE // response stream the single-shot dispatcher cannot produce — the same // streaming gap as TestForwarding_Progress_RealBackend. // 3. SEP-2577 deprecates the logging feature itself as of 2026-07-28 @@ -474,12 +517,33 @@ func TestForwarding_Sampling_NoDownstreamCapability(t *testing.T) { Params: mcpmcp.CallToolParams{Name: fwdSampleTool}, }) - // The call must resolve (error or IsError), never hang until the deadline. + // The call must resolve (error or IsError), never hang until the deadline — + // and the failure must NAME the refused request, so this cannot go vacuous + // by passing on an unrelated failure (e.g. a connect error). require.NotErrorIs(t, err, context.DeadlineExceeded, "sampling without a downstream handler must fail cleanly, not time out") - if err == nil { - assert.True(t, res.IsError, "backend sampling must fail when downstream lacks the capability") + assert.Contains(t, callFailureText(t, res, err), "sampling/createMessage", + "the failure must name the refused sampling request, not an unrelated error") +} + +// callFailureText extracts the human-readable failure text from a failed +// CallTool: the error message when the call errored, otherwise the text +// content of an IsError result. Fails the test if the call did not fail at +// all — callers use it precisely to assert WHAT failed. +func callFailureText(t *testing.T, res *mcpmcp.CallToolResult, err error) string { + t.Helper() + if err != nil { + return err.Error() } + require.NotNil(t, res) + require.True(t, res.IsError, "the call must fail (error or IsError result)") + var text string + for _, c := range res.Content { + if tc, ok := mcpmcp.AsTextContent(c); ok { + text += tc.Text + } + } + return text } // TestForwarding_Elicitation_NoDownstreamCapability is the elicitation twin of @@ -500,12 +564,12 @@ func TestForwarding_Elicitation_NoDownstreamCapability(t *testing.T) { Params: mcpmcp.CallToolParams{Name: fwdElicitTool}, }) - // The call must resolve (error or IsError), never hang until the deadline. + // Same shape as the sampling twin: resolve promptly AND name the refused + // request, closing the vacuous-pass route. require.NotErrorIs(t, err, context.DeadlineExceeded, "elicitation without a downstream handler must fail cleanly, not time out") - if err == nil { - assert.True(t, res.IsError, "backend elicitation must fail when downstream lacks the capability") - } + assert.Contains(t, callFailureText(t, res, err), "elicitation/create", + "the failure must name the refused elicitation request, not an unrelated error") } // samplingClient is a downstream client whose sampling handler returns a @@ -525,11 +589,12 @@ func newSamplingClient(ctx context.Context, t *testing.T, vmcpURL, summary, mode t.Helper() sc := &samplingClient{} + hc, pinRT := newLegacyPinnedHTTPClient() c, err := client.NewStreamableHttpClientWithOpts( vmcpURL, []transport.StreamableHTTPCOption{ transport.WithContinuousListening(), - transport.WithHTTPBasicClient(legacyPinnedHTTPClient()), + transport.WithHTTPBasicClient(hc), }, []client.ClientOption{ client.WithSamplingHandler(client.SamplingHandlerFunc( @@ -560,6 +625,7 @@ func newSamplingClient(ctx context.Context, t *testing.T, vmcpURL, summary, mode }, }) require.NoError(t, err) + requirePinFired(t, pinRT) return sc } diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go index b368206f40..65c79a7e73 100644 --- a/pkg/vmcp/server/modern_realbackend_integration_test.go +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -11,6 +11,7 @@ import ( "maps" "net/http" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -75,8 +76,13 @@ func postModern( payload, err := json.Marshal(body) require.NoError(t, err) - req, err := http.NewRequestWithContext( - context.Background(), http.MethodPost, baseURL+"/mcp", bytes.NewReader(payload)) + // Bounded: several callers assert "resolves promptly, never hangs", and + // without a deadline a hang would only be caught by go test's package + // timeout. 30s is generous for an in-process round-trip even under CI + // -race load. + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + t.Cleanup(cancel) + req, err := http.NewRequestWithContext(ctx, http.MethodPost, baseURL+"/mcp", bytes.NewReader(payload)) require.NoError(t, err) req.Header.Set("Content-Type", "application/json") req.Header.Set("MCP-Protocol-Version", "2026-07-28") @@ -307,13 +313,24 @@ func TestIntegration_Modern_RealBackend_UnknownMethod(t *testing.T) { } // TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly pins the -// Modern-client behavior for a backend tool that issues a mid-call +// Modern-client CONTRACT for a backend tool that issues a mid-call // server-initiated request (elicitation/create, sampling/createMessage): the -// call MUST resolve promptly to an explicit JSON-RPC error naming the refused -// request — never a hang, never a fabricated success, and never a resultType -// "input_required" envelope, which this dispatcher does not emit (client-polled -// multi-round retrieval, SEP-2322, is unimplemented; modernResultTypeComplete -// is the only resultType modern_envelope.go builds). +// call MUST resolve promptly (postModern is deadline-bounded) to an explicit +// JSON-RPC error naming the refused request — never a hang, never a +// fabricated success, and never a resultType "input_required" envelope, which +// this dispatcher does not emit (client-polled multi-round retrieval, +// SEP-2322, is unimplemented; modernResultTypeComplete is the only resultType +// modern_envelope.go builds). +// +// Deliberately NOT pinned: the specific error code. Today it is -32603, but +// the spec MUSTs -32021 MissingRequiredClientCapability for the undeclared +// case (at HTTP 400, which go-sdk's client escalates to permanent session +// death — the follow-up therefore plans -32021 at HTTP 200 as a documented +// deviation; see the "unavailable to Modern clients" section of +// docs/arch/10-virtual-mcp-architecture.md). Pinning -32603 here would make +// that spec-correcting follow-up look like a regression. TODO(follow-up): +// once the two-path capability-error contract lands, assert -32021 + +// data.requiredCapabilities for the undeclared case. // // This is the honest-unsupported contract for the surface the 2026-07-28 // revision removed: server-initiated requests need a live session, Modern @@ -341,20 +358,140 @@ func TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly(t *testing.T) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{"name": tc.tool}, 1, tc.tool) defer resp.Body.Close() - // A -32603 rides HTTP 200 per writeModernError's status mapping: the - // request was accepted and processed; the failure is application-level. - require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) require.NotContains(t, decoded, "result", "must not fabricate a success or an input_required envelope: %+v", decoded) errObj, ok := decoded["error"].(map[string]any) - require.True(t, ok, "decoded: %+v", decoded) - assert.EqualValues(t, -32603, errObj["code"]) - assert.Contains(t, errObj["message"], tc.want, + require.True(t, ok, "the failure must be an explicit JSON-RPC error: %+v", decoded) + msg, _ := errObj["message"].(string) + assert.Contains(t, msg, tc.want, "the error must name the refused server-initiated request") + + // KNOWN LEAK, deliberately characterized rather than endorsed: the + // -32603 message echoes err.Error() verbatim (writeModernDispatchError's + // documented posture), which today includes the backend workload ID — + // contradicting writeModernListError's leak policy 25 lines above it in + // the same file. The leak predates this test and affects both + // revisions; the follow-up capability-error contract replaces this + // message with a crafted one. When it does, FLIP this assertion to + // NotContains so the fix is a conscious, test-visible event. + assert.Contains(t, msg, "real-backend", + "characterizes the pre-existing backend-ID leak; flip to NotContains when the message is sanitized") }) } } +// TestIntegration_Modern_RealBackend_ProgressDropped pins today's Modern +// behavior for a backend tool that emits notifications/progress mid-call: the +// notification is silently dropped and the call still completes promptly with +// its result. There is nowhere for it to go — a Modern response is a single +// JSON body (writeModernResult), and request-scoped notifications would ride +// a per-request SSE response stream (SEP-2260) that the single-shot +// dispatcher does not produce. That is a vMCP streaming-dispatch gap, NOT a +// spec absence — this test converts that prose claim (see +// TestForwarding_Progress_RealBackend's pin comment and the arch doc) into an +// executable one, and MUST be revisited when streaming Modern dispatch is +// implemented: at that point the progress notification becomes deliverable +// and this test's premise changes. +// +// Promptness matters here: on a Legacy session the same fixture relays the +// notification; on Modern, a client waiting for it would hang to its own +// deadline. postModern's bounded context is the hang guard. +func TestIntegration_Modern_RealBackend_ProgressDropped(t *testing.T) { + t.Parallel() + + backendURL := startForwardingBackend(t) + ts := newRealModernTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ + "name": fwdProgressTool, + // A progressToken is exactly what would solicit progress delivery if a + // channel existed; its presence makes "dropped" the strongest claim. + "_meta": map[string]any{"progressToken": fwdProgressToken}, + }, 1, fwdProgressTool) + defer resp.Body.Close() + + // The call completes with the tool's result; the mid-call progress + // notification had no channel and was dropped. A single JSON body can + // carry nothing else, so completing at all IS the drop assertion. + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + result, ok := decoded["result"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.Equal(t, "complete", result["resultType"]) + content, ok := result["content"].([]any) + require.True(t, ok && len(content) == 1, "decoded: %+v", decoded) + assert.Equal(t, "done", content[0].(map[string]any)["text"]) +} + +// TestIntegration_Modern_RealBackend_LoggingContract pins today's Modern +// contract for logging/setLevel, which the 2026-07-28 revision removed (the +// per-request logLevel _meta key replaces it): +// +// - A WELL-FORMED Modern logging/setLevel is an unknown method to +// dispatchModern: 404 + -32601, matching go-sdk's own server ("method +// removed in the new protocol"). +// - The request go-sdk v1.7.0-pre.3 ACTUALLY sends is malformed — its +// SetLoggingLevel omits the per-request _meta injection (upstream bug), +// producing a Modern header with no _meta protocolVersion — and vMCP's +// classifier CORRECTLY rejects that shape with 400 + -32020 +// (HeaderMismatch). Loosening this to accept the malformed request would +// be worse than the failing upstream call. +// +// See TestForwarding_Logging_RealBackend's pin comment for the full +// three-cause disposition (upstream bug, method removal/streaming gap, +// SEP-2577 deprecation). +func TestIntegration_Modern_RealBackend_LoggingContract(t *testing.T) { + t.Parallel() + + backendURL := startForwardingBackend(t) + ts := newRealModernTestServer(t, backendURL) + + t.Run("well-formed setLevel is method-not-found", func(t *testing.T) { + t.Parallel() + resp, decoded := postModern(t, ts.URL, "logging/setLevel", map[string]any{"level": "debug"}, 1, "") + defer resp.Body.Close() + + require.Equal(t, http.StatusNotFound, resp.StatusCode, "decoded: %+v", decoded) + errObj, ok := decoded["error"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.EqualValues(t, -32601, errObj["code"]) + }) + + t.Run("go-sdk's malformed setLevel is rejected -32020", func(t *testing.T) { + t.Parallel() + // Replicate the upstream bug's wire shape by hand: Modern header, but + // params carrying NO _meta protocolVersion (postModern would inject it, + // so build the request raw). + payload, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "method": "logging/setLevel", + "params": map[string]any{"level": "debug"}, + }) + require.NoError(t, err) + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + req, err := http.NewRequestWithContext(ctx, http.MethodPost, ts.URL+"/mcp", bytes.NewReader(payload)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("MCP-Protocol-Version", "2026-07-28") + req.Header.Set("Mcp-Method", "logging/setLevel") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + + require.Equal(t, http.StatusBadRequest, resp.StatusCode, "body: %s", body) + var decoded map[string]any + require.NoError(t, json.Unmarshal(body, &decoded), "body: %s", body) + errObj, ok := decoded["error"].(map[string]any) + require.True(t, ok, "body: %s", body) + assert.EqualValues(t, -32020, errObj["code"], + "the malformed request must be rejected as a header/_meta mismatch") + }) +} + // TestIntegration_Modern_RealBackend_MalformedArguments verifies a // syntactically valid tools/call whose "arguments" is present but not a JSON // object is rejected with 400 + JSON-RPC -32602, per hasNonObjectArguments' From 1be60a3da8349af6709cbc84ea4c337ab187ab2a Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 20:42:46 +0000 Subject: [PATCH 3/4] Answer the Legacy pin with a DiscoverResult per review Maintainer review (jhrozek) on #6051, applied: - The pin now answers server/discover with a successful DiscoverResult whose supportedVersions lists only 2025-11-25 - literally what a Legacy-only server sends and the same shape vMCP's SDK path answers today (classification.go exempts the probe rather than rejecting it, so the previous -32022 mimicry described a response production never produces for this method). go-sdk negotiates against the version list and falls back to Legacy initialize either way. - Drop assertions on go-sdk's error-wrapping substrings (the refused request's name reaches the message only via the SDK's "calling %q" wrapper): the NoDownstreamCapability tests now assert structurally (round-trip on the live session + IsError), and the Modern honest-failure test defers the named-request property to the follow-up's vMCP-owned message. The backend-ID characterization stays - that string is ToolHive's. - Compress the per-test disposition comments to pointers at the arch doc, keeping only the two non-obvious facts in place (progress: vMCP cannot stream, Modern has the channel; logging: go-sdk sends a malformed request and the -32020 rejection is correct). Drop a stale line-number citation and an unverifiable build reference. Use "Modern dispatch enabled/disabled" over the ambiguous "kill-switch on/off". - Arch doc: cross-link the Modern-client limitation from the mid-call forwarding section (which otherwise still reads as if forwarding is unconditional), fix the test-package reference, and soften tone. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam --- docs/arch/10-virtual-mcp-architecture.md | 19 +- ...forwarding_realbackend_integration_test.go | 221 +++++++++--------- .../modern_realbackend_integration_test.go | 36 +-- 3 files changed, 141 insertions(+), 135 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 0a9dd13569..d4453081db 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -344,19 +344,19 @@ this; production tools can.) Legacy clients keep the full mid-call forwarding behaviour unchanged; the forwarding integration tests pin their downstream clients to Legacy explicitly -(`legacyPinningRoundTripper` in `pkg/vmcp/server`) because that surface exists -only on a Legacy session. +(`legacyPinningRoundTripper` in `pkg/vmcp/server`'s external test package) +because that surface exists only on a Legacy session. -**Bridging was considered, costed, and rejected — this is a design position, -not a backlog item.** Serving MRTR to Modern clients on top of a *Legacy* +**Bridging was considered, costed, and rejected.** Serving MRTR to Modern +clients on top of a *Legacy* backend would require parking the live, mid-flight backend call server-side (the blocked goroutine and its open session cannot be serialized into the opaque `requestState` the SEP designed for handler re-invocation) and keying the resume on an unguessable token — per-round server state with TTL/eviction, identity binding on a token that becomes a capability to resume someone else's in-flight call, and replica affinity with no `Mcp-Session-Id` to route on. That -is a session in disguise: exactly the construct the 2026-07-28 revision -removed. The spec's own sanctioned path for genuinely stateful +would reintroduce, in different clothes, the per-request server state the +2026-07-28 revision removed. The spec's own sanctioned path for genuinely stateful `input_required` work is the **Tasks** extension (SEP-2663: `tools/call` returns `resultType: "task"` with a `taskId`; the client polls `tasks/get` and answers outstanding `inputRequests` via `inputResponses` on `tasks/update`; @@ -576,6 +576,13 @@ While a backend `tools/call` (or other request) is in flight, the backend may is **Implementation**: `pkg/vmcp/forwarding.go`, `pkg/vmcp/client/forwarding.go`, `pkg/vmcp/server/serve_handlers.go` +**Known limitation (Modern clients)**: everything in this section describes a +**Legacy (2025-11-25) client session**. For Modern (2026-07-28) clients there +is no session and no server-initiated request channel, so none of this +forwarding applies — see +[Limitation: elicitation and sampling are unavailable to Modern clients](#limitation-elicitation-and-sampling-are-unavailable-to-modern-clients) +for what a Modern caller gets instead. + **Known limitation (logging level)**: forwarded backend logging is not yet filtered to the downstream client's requested `logging/setLevel`. vMCP requests debug-level logging from the backend so it emits `notifications/message`, and every such notification is forwarded — the downstream client's own level preference is not applied to the relayed stream. **Known limitation (resource-template authorization)**: a resource template is advertised on the template-string entity (e.g. `file:///logs/{date}.txt`), but a concrete read is admission-checked on the **expanded** URI (e.g. `file:///logs/2025-01-01.txt`). Operators should therefore author resource authorization policies against concrete URI patterns, not the template string. diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index 188296eb0e..e3605dabfb 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -21,7 +21,6 @@ import ( "github.com/stacklok/toolhive-core/mcpcompat/client/transport" mcpmcp "github.com/stacklok/toolhive-core/mcpcompat/mcp" mcpserver "github.com/stacklok/toolhive-core/mcpcompat/server" - mcpparser "github.com/stacklok/toolhive/pkg/mcp" ) // Forwarding integration fixtures. These exercise the server->client forwarding @@ -141,50 +140,47 @@ func startForwardingBackend(t *testing.T) string { // legacyPinningRoundTripper pins a downstream mcpcompat client to the Legacy // (2025-11-25) revision by answering its Modern-first server/discover probe -// with an HTTP 400 / JSON-RPC -32022 UnsupportedProtocolVersionError body -// (built with the production mcpparser.ClassificationErrorResponse so the -// fake cannot drift from the real error shape), which drives go-sdk's -// Connect down its documented fall-back-to-Legacy-initialize path. Every -// other request passes through to base untouched. +// with a successful DiscoverResult whose supportedVersions lists ONLY +// 2025-11-25 — literally what a Legacy-only server sends, and the same shape +// vMCP's SDK path answers today: classification.go deliberately EXEMPTS +// server/discover from Modern-dispatch gating (rejecting the probe would +// leave a client no way to negotiate down), so the probe reaches the SDK +// stateful path and gets a 200 whose version list omits 2026-07-28. go-sdk's +// Connect finds no mutually supported Modern version in that list and falls +// back to the Legacy initialize handshake (mcp/client.go's discover +// negotiation; any non-nil discover error takes the same fallback). This +// RoundTripper reproduces that version-omitting success answer +// deterministically, independent of what the server under test advertises. +// Every other request passes through to base untouched. // -// MECHANISM, precisely: this is NOT what vMCP itself does. vMCP never rejects -// server/discover — the kill-switch (#5959) explicitly exempts it, so vMCP -// answers HTTP 200 with a discover RESULT whose supportedVersions omits -// 2026-07-28, and the go-sdk client negotiates down from that. The -// RoundTripper substitutes a -32022 rejection to force the same Legacy -// outcome deterministically, independent of what versions the server under -// test advertises. -// -// WHY PIN: these fixtures assert the Legacy-only server-initiated surface — -// mid-call elicitation/sampling requests and progress/logging notifications -// relayed over a live session. The 2026-07-28 revision removes server- -// initiated requests entirely (go-sdk's assertServerInitiatedRequestAllowed -// refuses them by protocol version; the replacement is client-polled -// multi-round retrieval, SEP-2322) and SEP-2577 deprecates sampling and -// logging outright. While the kill-switch is on, the version-omitting -// discover result pins these clients to Legacy incidentally; once it is -// removed (#6033) the go-sdk-based client would negotiate Modern and this -// surface would vanish mid-test — failing at connect on subscriptions/listen, -// not at the behavior under test. Pinning makes the tests' Legacy dependency -// explicit instead of incidental. +// WHY PIN: these fixtures assert the Legacy-only server-initiated surface; +// see the file header above and the client-edge limitation in +// docs/arch/10-virtual-mcp-architecture.md for the full disposition. While +// Modern dispatch is disabled (ModernDispatchEnabled: false, today's +// default), the server's own version-omitting discover answer pins these +// clients to Legacy incidentally; once #6033 makes Modern dispatch +// unconditional, the go-sdk-based client would negotiate Modern and this +// surface would vanish mid-test — failing at connect, not at the behavior +// under test. Pinning makes the tests' Legacy dependency explicit instead of +// incidental. // // The pin lives in the transport because mcpcompat documents it cannot set a // protocol version (go-sdk's ClientSessionOptions.protocolVersion is // unexported — see the LIMITATION note in mcpcompat/client and #5911). // Replace this with a real client option if #5911 lands. // -// LOAD-BEARING after #6033: once the kill-switch is gone, this RoundTripper -// is the ONLY thing keeping these downstream clients on Legacy. It is not -// leftover kill-switch scaffolding — deleting it silently flips every test -// in this file to Modern and voids what they assert. The intercepted counter +// LOAD-BEARING after #6033: once Modern dispatch is unconditional, this +// RoundTripper is the ONLY thing keeping these downstream clients on Legacy. +// It is not leftover scaffolding — deleting it silently flips every test in +// this file to Modern and voids what they assert. The intercepted counter // (asserted after every connect) exists to catch exactly that: if go-sdk ever // stops probing server/discover first, the pin becomes dead code and the // counter assertion fails instead of the tests silently changing meaning. type legacyPinningRoundTripper struct { base http.RoundTripper - // intercepted counts server/discover probes answered with the -32022 - // rejection. Asserted > 0 after connect — proof the pin FIRED, not merely - // that it exists. + // intercepted counts server/discover probes answered with the Legacy-only + // DiscoverResult. Asserted > 0 after connect — proof the pin FIRED, not + // merely that it exists. intercepted atomic.Int32 } @@ -210,14 +206,44 @@ func (rt *legacyPinningRoundTripper) RoundTrip(req *http.Request) (*http.Respons } if json.Unmarshal(body, &probe) == nil && probe.Method == "server/discover" { rt.intercepted.Add(1) - return mcpparser.ClassificationErrorResponse(req, probe.ID, &mcpparser.UnsupportedVersionError{ - Requested: mcpparser.MCPVersionModern, - Supported: []string{mcpparser.MCPVersionLegacy}, - }), nil + return legacyOnlyDiscoverResponse(req, probe.ID) } return rt.base.RoundTrip(req) } +// legacyOnlyDiscoverResponse builds the HTTP 200 server/discover answer of a +// Legacy-only server: a successful JSON-RPC result whose supportedVersions +// lists only 2025-11-25. go-sdk's Connect negotiates against that list, finds +// no Modern version, and falls back to the Legacy initialize handshake — the +// same path vMCP's own version-omitting discover answer drives today. +func legacyOnlyDiscoverResponse(req *http.Request, id any) (*http.Response, error) { + body, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": id, + "result": map[string]any{ + "resultType": "complete", + "supportedVersions": []string{"2025-11-25"}, + "capabilities": map[string]any{}, + }, + }) + if err != nil { + return nil, err + } + hdr := make(http.Header) + hdr.Set("Content-Type", "application/json") + return &http.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + Proto: "HTTP/1.1", + ProtoMajor: 1, + ProtoMinor: 1, + Header: hdr, + ContentLength: int64(len(body)), + Body: io.NopCloser(bytes.NewReader(body)), + Request: req, + }, nil +} + // newLegacyPinnedHTTPClient returns an *http.Client for // transport.WithHTTPBasicClient that pins the mcpcompat client to Legacy, // plus the RoundTripper itself so callers can assert the pin actually fired — @@ -229,9 +255,10 @@ func newLegacyPinnedHTTPClient() (*http.Client, *legacyPinningRoundTripper) { // requirePinFired asserts the Legacy pin intercepted at least one // server/discover probe during connect. Without this, the pin is -// indistinguishable from dead code: on a kill-switch base the tests pass -// even with the RoundTripper deleted (the server's own version-omitting -// discover answer pins the client), and if go-sdk ever stopped probing +// indistinguishable from dead code: with Modern dispatch disabled (today's +// default) the tests pass even with the RoundTripper deleted (the server's +// own version-omitting discover answer pins the client), and if go-sdk ever +// stopped probing // server/discover first the tests would silently stop testing what they // claim. func requirePinFired(t *testing.T, rt *legacyPinningRoundTripper) { @@ -353,10 +380,8 @@ func (dc *downstreamClient) waitNotification(ctx context.Context, t *testing.T, // elicitation/create is relayed to the downstream client and its response // carried back into the tool result. // -// Legacy-pinned: a server-initiated elicitation/create toward the client does -// not exist on Modern (2026-07-28) — SEP-2322 replaces it with client-polled -// inputRequests, which vMCP deliberately does not serve (see the "unavailable -// to Modern clients" limitation in docs/arch/10-virtual-mcp-architecture.md). +// Legacy-pinned: see legacyPinningRoundTripper and the client-edge limitation +// in docs/arch/10-virtual-mcp-architecture.md. func TestForwarding_Elicitation_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -388,10 +413,9 @@ func TestForwarding_Elicitation_RealBackend(t *testing.T) { // sampling/createMessage is relayed to the downstream client and the sampled // message carried back into the tool result. // -// Legacy-pinned: like elicitation, server-initiated sampling does not exist on -// Modern (SEP-2322) — and SEP-2577 additionally deprecates the sampling -// feature itself as of 2026-07-28 (sanctioned replacement: direct LLM-provider -// integration), so this surface is Legacy-only by spec, not by omission. +// Legacy-pinned: see legacyPinningRoundTripper and the client-edge limitation +// in docs/arch/10-virtual-mcp-architecture.md (which also covers SEP-2577's +// deprecation of sampling itself). func TestForwarding_Sampling_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -416,16 +440,12 @@ func TestForwarding_Sampling_RealBackend(t *testing.T) { // notifications/progress is relayed to the downstream client, arriving before // the tool result is read. // -// Legacy-pinned — but NOT because Modern lacks a progress channel. It does -// not: on Modern, progress remains spec-legal as a request-scoped -// notification riding the POST-initiated SSE response stream (SEP-2260 -// tightened exactly that channel; progressToken is unchanged in the draft -// schema). What is missing is on vMCP's side: dispatchModern is single-shot — -// writeModernResult emits one JSON body and cannot stream — so nothing riding -// the per-request stream is deliverable to a Modern client today. That is a -// vMCP "streaming Modern dispatch" gap (future work), distinct from both MRTR -// and subscriptions/listen (whose subscribable set is fixed at four -// list-changed/subscription types and excludes progress). +// Legacy-pinned because dispatchModern cannot stream a response, NOT because +// Modern lacks a progress channel — it has one (request-scoped notifications +// on the POST SSE response stream, SEP-2260). Full disposition in the +// client-edge limitation in docs/arch/10-virtual-mcp-architecture.md; today's +// Modern behavior (notification dropped, call completes) is pinned by +// TestIntegration_Modern_RealBackend_ProgressDropped. func TestForwarding_Progress_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -451,23 +471,16 @@ func TestForwarding_Progress_RealBackend(t *testing.T) { // from the backend (so it emits) and relays the backend's notifications/message // to the downstream client, which has itself set a logging level. // -// Legacy-pinned, for three stacked reasons — record them so this is not -// re-litigated as a vMCP bug: -// 1. Upstream go-sdk defect: SetLoggingLevel omits injectRequestMeta -// (client.go:1303 in v1.7.0-pre.3; every other Modern-aware method -// injects), so a Modern logging/setLevel carries the MCP-Protocol-Version -// header but no _meta.protocolVersion. vMCP's ClassifyRevision CORRECTLY -// rejects that with -32020; accepting the malformed request to make this -// test pass would be worse than the failure. Filed upstream separately. -// 2. Even with (1) fixed, the RPC no longer exists on Modern: the per-request -// logLevel _meta key REPLACES logging/setLevel (draft schema; go-sdk's -// server answers methodSetLevel with -32601 under 2026-07-28, server.go -// "method removed in the new protocol"). Delivery of notifications/message -// for a request that does carry logLevel rides the per-request SSE -// response stream the single-shot dispatcher cannot produce — the same -// streaming gap as TestForwarding_Progress_RealBackend. -// 3. SEP-2577 deprecates the logging feature itself as of 2026-07-28 -// (sanctioned replacement: stderr / OpenTelemetry). +// Legacy-pinned. The one fact worth keeping AT this test so nobody "fixes" +// vMCP to make it pass on Modern: go-sdk's SetLoggingLevel (v1.7.0-pre.3) +// omits the per-request _meta injection every other Modern-aware method +// performs, so the Modern request it sends is MALFORMED (header without +// _meta.protocolVersion) and vMCP's -32020 rejection is CORRECT — accepting +// it would be worse than the failing call. Upstream bug, filed separately. +// The rest of the disposition (the RPC is removed on Modern, logLevel _meta +// replaces it, SEP-2577 deprecates logging) lives in the client-edge +// limitation in docs/arch/10-virtual-mcp-architecture.md; today's Modern +// contract is pinned by TestIntegration_Modern_RealBackend_LoggingContract. func TestForwarding_Logging_RealBackend(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -499,11 +512,10 @@ func TestForwarding_Logging_RealBackend(t *testing.T) { // // Legacy-pinned even though it happens to pass on Modern: capability-gated // failure on a live session only exists on Legacy. On Modern the same call -// fails with the sessionless -32603 REGARDLESS of what the client advertises -// (withHandlers makes no difference), so the lenient assertion here would be +// fails with the sessionless error REGARDLESS of what the client advertises +// (withHandlers makes no difference), so a lenient assertion here would be // satisfied for a reason unrelated to the gating this test exists to -// exercise. The pin keeps it testing what its name says. (Verified against a -// working subscriptions/listen build: unpinned, this test passes vacuously.) +// exercise. The pin keeps it testing what its name says. func TestForwarding_Sampling_NoDownstreamCapability(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) @@ -517,33 +529,16 @@ func TestForwarding_Sampling_NoDownstreamCapability(t *testing.T) { Params: mcpmcp.CallToolParams{Name: fwdSampleTool}, }) - // The call must resolve (error or IsError), never hang until the deadline — - // and the failure must NAME the refused request, so this cannot go vacuous - // by passing on an unrelated failure (e.g. a connect error). - require.NotErrorIs(t, err, context.DeadlineExceeded, - "sampling without a downstream handler must fail cleanly, not time out") - assert.Contains(t, callFailureText(t, res, err), "sampling/createMessage", - "the failure must name the refused sampling request, not an unrelated error") -} - -// callFailureText extracts the human-readable failure text from a failed -// CallTool: the error message when the call errored, otherwise the text -// content of an IsError result. Fails the test if the call did not fail at -// all — callers use it precisely to assert WHAT failed. -func callFailureText(t *testing.T, res *mcpmcp.CallToolResult, err error) string { - t.Helper() - if err != nil { - return err.Error() - } + // Structural, not substring: the call must ROUND-TRIP on the live Legacy + // session (err == nil proves it was not a connect failure — the vacuity + // route) and fail as a tool error. The failure text is deliberately not + // asserted: its identifying substrings are go-sdk's error wrapping, not + // ToolHive's, and testing a dependency's strings breaks on upstream + // rewords (testing.md, test scope). + require.NoError(t, err, + "the call must round-trip on the live session, not die at transport level") require.NotNil(t, res) - require.True(t, res.IsError, "the call must fail (error or IsError result)") - var text string - for _, c := range res.Content { - if tc, ok := mcpmcp.AsTextContent(c); ok { - text += tc.Text - } - } - return text + assert.True(t, res.IsError, "backend sampling must fail when downstream lacks the capability") } // TestForwarding_Elicitation_NoDownstreamCapability is the elicitation twin of @@ -564,12 +559,11 @@ func TestForwarding_Elicitation_NoDownstreamCapability(t *testing.T) { Params: mcpmcp.CallToolParams{Name: fwdElicitTool}, }) - // Same shape as the sampling twin: resolve promptly AND name the refused - // request, closing the vacuous-pass route. - require.NotErrorIs(t, err, context.DeadlineExceeded, - "elicitation without a downstream handler must fail cleanly, not time out") - assert.Contains(t, callFailureText(t, res, err), "elicitation/create", - "the failure must name the refused elicitation request, not an unrelated error") + // Same structural shape as the sampling twin — see the rationale there. + require.NoError(t, err, + "the call must round-trip on the live session, not die at transport level") + require.NotNil(t, res) + assert.True(t, res.IsError, "backend elicitation must fail when downstream lacks the capability") } // samplingClient is a downstream client whose sampling handler returns a @@ -641,9 +635,8 @@ func newSamplingClient(ctx context.Context, t *testing.T, vmcpURL, summary, mode // "summary-B" (mismatching the "summary-A" assertion) and the handler counters // would be lopsided (A=0, B=2 instead of 1/1) — either check trips. // -// Legacy-pinned for the same reasons as TestForwarding_Sampling_RealBackend: -// per-session routing of a server-initiated request presupposes sessions, -// which Modern removed along with the requests themselves. +// Legacy-pinned: see legacyPinningRoundTripper — per-session routing of a +// server-initiated request presupposes sessions, which Modern removed. func TestForwarding_Sampling_RealBackend_SessionIsolation(t *testing.T) { t.Parallel() ctx, cancel := context.WithTimeout(t.Context(), forwardingRealBackendTimeout) diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go index 65c79a7e73..b0166f7d76 100644 --- a/pkg/vmcp/server/modern_realbackend_integration_test.go +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -316,11 +316,10 @@ func TestIntegration_Modern_RealBackend_UnknownMethod(t *testing.T) { // Modern-client CONTRACT for a backend tool that issues a mid-call // server-initiated request (elicitation/create, sampling/createMessage): the // call MUST resolve promptly (postModern is deadline-bounded) to an explicit -// JSON-RPC error naming the refused request — never a hang, never a -// fabricated success, and never a resultType "input_required" envelope, which -// this dispatcher does not emit (client-polled multi-round retrieval, -// SEP-2322, is unimplemented; modernResultTypeComplete is the only resultType -// modern_envelope.go builds). +// JSON-RPC error — never a hang, never a fabricated success, and never a +// resultType "input_required" envelope, which this dispatcher does not emit +// (client-polled multi-round retrieval, SEP-2322, is unimplemented; +// modernResultTypeComplete is the only resultType modern_envelope.go builds). // // Deliberately NOT pinned: the specific error code. Today it is -32603, but // the spec MUSTs -32021 MissingRequiredClientCapability for the undeclared @@ -347,10 +346,9 @@ func TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly(t *testing.T) tests := []struct { name string tool string - want string // substring naming the refused server-initiated request }{ - {name: "elicitation", tool: fwdElicitTool, want: "elicitation/create"}, - {name: "sampling", tool: fwdSampleTool, want: "sampling/createMessage"}, + {name: "elicitation", tool: fwdElicitTool}, + {name: "sampling", tool: fwdSampleTool}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -363,17 +361,25 @@ func TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly(t *testing.T) errObj, ok := decoded["error"].(map[string]any) require.True(t, ok, "the failure must be an explicit JSON-RPC error: %+v", decoded) msg, _ := errObj["message"].(string) - assert.Contains(t, msg, tc.want, - "the error must name the refused server-initiated request") + // The refused request's NAME (elicitation/create etc.) is deliberately + // NOT asserted: today it appears in the message only via go-sdk's + // "calling %q" error wrapping — a dependency's string, which an + // upstream reword would break for no ToolHive reason (testing.md, + // test scope). The follow-up capability-error contract emits a + // vMCP-owned message; assert the named-request property THERE, on our + // own string. + // // KNOWN LEAK, deliberately characterized rather than endorsed: the // -32603 message echoes err.Error() verbatim (writeModernDispatchError's // documented posture), which today includes the backend workload ID — - // contradicting writeModernListError's leak policy 25 lines above it in - // the same file. The leak predates this test and affects both - // revisions; the follow-up capability-error contract replaces this - // message with a crafted one. When it does, FLIP this assertion to - // NotContains so the fix is a conscious, test-visible event. + // contradicting writeModernListError's leak policy in the same file. + // The leak predates this test and affects both revisions; the + // follow-up replaces this message with a crafted one. When it does, + // FLIP this assertion to NotContains so the fix is a conscious, + // test-visible event. ("real-backend" IS a ToolHive string: the + // backend ID this fixture registers, echoed through vMCP's own + // "tool call failed on backend %s" wrapping.) assert.Contains(t, msg, "real-backend", "characterizes the pre-existing backend-ID leak; flip to NotContains when the message is sanitized") }) From 5c901a9d8822c4abc8741bbc6f0bca1c08f3a632 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 20:58:15 +0000 Subject: [PATCH 4/4] Drop the backend-ID leak characterization assertion The leak fix (vMCP-owned messages in the capability-error contract, #6061) is imminent, so an assertion whose own comment says to flip it shortly is churn that makes the fix noisier rather than clearer. The message is now asserted on nothing at all - deliberately: its only identifying substrings are either go-sdk's error wrapping (a dependency's strings) or the leak itself; both message properties are asserted in #6061 where vMCP owns the text. The HTTP 200 assertion returns in its place, valid across the fix. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam --- .../modern_realbackend_integration_test.go | 34 +++++++------------ 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go index b0166f7d76..7c312b49db 100644 --- a/pkg/vmcp/server/modern_realbackend_integration_test.go +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -356,32 +356,22 @@ func TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly(t *testing.T) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{"name": tc.tool}, 1, tc.tool) defer resp.Body.Close() + // Both today's -32603 and the follow-up's -32021 (documented HTTP + // 200 deviation) ride HTTP 200, so this holds across the fix. + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) require.NotContains(t, decoded, "result", "must not fabricate a success or an input_required envelope: %+v", decoded) - errObj, ok := decoded["error"].(map[string]any) + _, ok := decoded["error"].(map[string]any) require.True(t, ok, "the failure must be an explicit JSON-RPC error: %+v", decoded) - msg, _ := errObj["message"].(string) - // The refused request's NAME (elicitation/create etc.) is deliberately - // NOT asserted: today it appears in the message only via go-sdk's - // "calling %q" error wrapping — a dependency's string, which an - // upstream reword would break for no ToolHive reason (testing.md, - // test scope). The follow-up capability-error contract emits a - // vMCP-owned message; assert the named-request property THERE, on our - // own string. - // - // KNOWN LEAK, deliberately characterized rather than endorsed: the - // -32603 message echoes err.Error() verbatim (writeModernDispatchError's - // documented posture), which today includes the backend workload ID — - // contradicting writeModernListError's leak policy in the same file. - // The leak predates this test and affects both revisions; the - // follow-up replaces this message with a crafted one. When it does, - // FLIP this assertion to NotContains so the fix is a conscious, - // test-visible event. ("real-backend" IS a ToolHive string: the - // backend ID this fixture registers, echoed through vMCP's own - // "tool call failed on backend %s" wrapping.) - assert.Contains(t, msg, "real-backend", - "characterizes the pre-existing backend-ID leak; flip to NotContains when the message is sanitized") + // Nothing about the MESSAGE is asserted, deliberately. The refused + // request's name (elicitation/create etc.) reaches it only via + // go-sdk's "calling %q" error wrapping — a dependency's string + // (testing.md, test scope) — and the message also carries the + // pre-existing backend-workload-ID leak, which the follow-up + // capability-error contract fixes with vMCP-owned messages. Both + // message properties (names our string, no backend ID) are asserted + // THERE, where vMCP owns the text. }) } }