From d3c4d286ec1ed950de8dbb282f148aeb3a4f01ed Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 17:12:58 +0000 Subject: [PATCH 1/7] Remove the vMCP Modern dispatch kill-switch Refs #5959. Config.ModernDispatchEnabled (env TOOLHIVE_VMCP_MODERN_STATELESS, default off) was a temporary safety lever gating whether well-formed MCP 2026-07-28 ("Modern") stateless requests were served by classifyingHandler -> dispatchModern or fell through to the SDK path. Both of the issue's gates are now met: the dual-era conformance harness landed in #5837 and go-sdk v1.7 was adopted via #5993. Serve Modern unconditionally and delete the switch, its ServerConfig/Config plumbing, its env var, and the test helpers and specs that existed only to toggle it. This also removes #6009's -32022 UnsupportedVersionError refusal and its server/discover exemption. That refusal answered exactly one condition -- "vMCP supports this revision but has chosen not to serve it" -- which cannot arise once the switch is gone. Every genuine version-grounds rejection still happens, earlier and unchanged, in mcpparser.ClassifyRevision: a _meta protocolVersion naming any other version yields the same conformant -32022, so ClassifyRevision is now the single owner of that decision. server/discover no longer needs an exemption because nothing refuses it on version grounds; dispatchModern answers it via dispatchModernDiscover. authz_integration_test.go's buildCedarAuthzServer had to *set* the switch for TestIntegration_CedarAuthzDenial_ModernPath_IsAudited to reach dispatchModern's call gate at all -- a silently-vacuous test class that disappears along with the switch. A blocker documented when this commit was first written -- dispatchModern had no subscriptions/listen case, so once server/discover advertised Modern, a go-sdk v1.7 client negotiated 2026-07-28 and tore its connection down when the listen stream it opens (mcpcompat's Initialize installs list-changed handlers unconditionally) was answered -32601 -- was closed by #6050 (modern_subscriptions.go and Modern list pagination) before this landed: this commit sits on a base that already serves subscriptions/listen. --- pkg/vmcp/cli/serve.go | 23 ------ pkg/vmcp/client/modern_integration_test.go | 4 -- pkg/vmcp/server/authz_integration_test.go | 9 --- pkg/vmcp/server/bridge_regression_test.go | 18 +++-- pkg/vmcp/server/classification.go | 61 +++++----------- pkg/vmcp/server/classification_test.go | 70 +++++++------------ pkg/vmcp/server/derive.go | 1 - pkg/vmcp/server/derive_test.go | 2 - ...forwarding_realbackend_integration_test.go | 15 ++-- pkg/vmcp/server/modern_dispatch_test.go | 2 +- pkg/vmcp/server/modern_pagination.go | 6 +- .../modern_realbackend_integration_test.go | 47 ++++--------- pkg/vmcp/server/serve.go | 6 -- pkg/vmcp/server/serve_test.go | 1 - pkg/vmcp/server/server.go | 23 ++---- ...management_realbackend_integration_test.go | 53 ++++---------- test/e2e/vmcp_dual_era_helpers_test.go | 48 +++---------- test/e2e/vmcp_dual_era_test.go | 70 +------------------ 18 files changed, 105 insertions(+), 354 deletions(-) diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index e7749de759..0f7efa9bcb 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -15,7 +15,6 @@ import ( "net" "os" "path/filepath" - "strconv" "time" "go.opentelemetry.io/otel/trace" @@ -52,12 +51,6 @@ import ( vmcpstatus "github.com/stacklok/toolhive/pkg/vmcp/status" ) -// modernDispatchEnvVar is the kill-switch env var for direct-to-core dispatch -// of well-formed MCP 2026-07-28 ("Modern") stateless requests (default off; -// see server.Config.ModernDispatchEnabled). Env-only ahead of any CLI-flag or -// CRD wiring. -const modernDispatchEnvVar = "TOOLHIVE_VMCP_MODERN_STATELESS" - // ServeConfig holds all parameters needed to start the vMCP server. // Populated by the caller from Cobra flag values or equivalent. // At least one of ConfigPath or GroupRef must be non-empty; ConfigPath takes @@ -419,21 +412,6 @@ func Serve(ctx context.Context, cfg ServeConfig) error { }() } - // Read the Modern-stateless-dispatch kill-switch once, here at the - // composition root. Unset is the deliberate "off" default. A non-empty - // value that fails to parse as a bool is an operator typo (a misspelled - // "true"), not a request to enable the feature — warn and stay disabled - // rather than silently treating it as false. - modernDispatchEnabled := false - if raw := os.Getenv(modernDispatchEnvVar); raw != "" { - var err error - if modernDispatchEnabled, err = strconv.ParseBool(raw); err != nil { - slog.Warn(fmt.Sprintf("%s has an unrecognized value %q; Modern stateless dispatch stays disabled", - modernDispatchEnvVar, raw)) - modernDispatchEnabled = false - } - } - // Resolve transport defaults once here at the composition root: the // vMCP config edge is the single place flags/CRD/YAML become a fully-resolved // Config, so server.New, Serve, and the derive* helpers downstream are pure @@ -446,7 +424,6 @@ func Serve(ctx context.Context, cfg ServeConfig) error { Host: cfg.Host, Port: cfg.Port, SessionTTL: cfg.SessionTTL, - ModernDispatchEnabled: modernDispatchEnabled, AuthMiddleware: authMiddleware, AuthzMiddleware: authzMiddleware, AuthInfoHandler: authInfoHandler, diff --git a/pkg/vmcp/client/modern_integration_test.go b/pkg/vmcp/client/modern_integration_test.go index c248dbc1be..f24aa4b21c 100644 --- a/pkg/vmcp/client/modern_integration_test.go +++ b/pkg/vmcp/client/modern_integration_test.go @@ -95,10 +95,6 @@ func newModernVMCPServer(t *testing.T, backendURL string) *httptest.Server { SessionTTL: 5 * time.Minute, SessionFactory: vmcpsession.NewSessionFactory(authReg), Aggregator: agg, - // main re-added this kill-switch (default off, #5959); the harness - // must opt in so a well-formed Modern request reaches dispatchModern - // rather than falling through to the Legacy SDK path. - ModernDispatchEnabled: true, }, router.NewSessionRouter(&vmcp.RoutingTable{}), backendClient, diff --git a/pkg/vmcp/server/authz_integration_test.go b/pkg/vmcp/server/authz_integration_test.go index e51f67a604..a7580e8dd1 100644 --- a/pkg/vmcp/server/authz_integration_test.go +++ b/pkg/vmcp/server/authz_integration_test.go @@ -178,15 +178,6 @@ func buildCedarAuthzServer( Authz: authzCfg, AuditConfig: auditCfg, CodeModeConfig: codeModeCfg, - // Required for TestIntegration_CedarAuthzDenial_ModernPath_IsAudited to - // exercise what it claims: dispatchModern's re-homed call gate. Without - // it the kill switch refuses the Modern request in classifyingHandler and - // dispatchModern never runs. (Before the classifier learned to refuse an - // unserved revision, the request instead fell through to the SDK path, so - // that test passed on a denial from a different gate entirely.) Safe for - // the Legacy-shaped tests sharing this helper: classifyingHandler passes - // Legacy traffic through regardless of this flag. - ModernDispatchEnabled: true, }, router.NewSessionRouter(&vmcp.RoutingTable{}), backendClient, diff --git a/pkg/vmcp/server/bridge_regression_test.go b/pkg/vmcp/server/bridge_regression_test.go index d907ba4e23..93a13b6d2a 100644 --- a/pkg/vmcp/server/bridge_regression_test.go +++ b/pkg/vmcp/server/bridge_regression_test.go @@ -207,10 +207,9 @@ func newRecordingBackendProxy(t *testing.T, backendURL string) (string, func() [ return ts.URL + "/mcp", getRecords } -// newBridgeCellAServer builds a vMCP server with Modern dispatch's kill -// switch ON, routing a single Legacy backend (at proxyURL) through -// identityEchoStrategy so the credential that reaches the backend on each -// call is observable. Mirrors newRealModernTestHandler's construction +// newBridgeCellAServer builds a vMCP server routing a single Legacy backend +// (at proxyURL) through identityEchoStrategy so the credential that reaches +// the backend on each call is observable. Mirrors newRealTestHandler's construction // (session_management_realbackend_integration_test.go), diverging only in // the backend's AuthConfig -- that helper always uses "unauthenticated", // which would leave nothing for this file's tests to assert. @@ -245,12 +244,11 @@ func newBridgeCellAServer(t *testing.T, proxyURL string) *httptest.Server { srv, err := server.New( context.Background(), &server.Config{ - Host: "127.0.0.1", - Port: 0, - SessionTTL: 5 * time.Minute, - ModernDispatchEnabled: true, - SessionFactory: vmcpsession.NewSessionFactory(authReg), - Aggregator: agg, + Host: "127.0.0.1", + Port: 0, + SessionTTL: 5 * time.Minute, + SessionFactory: vmcpsession.NewSessionFactory(authReg), + Aggregator: agg, }, rt, backendClient, diff --git a/pkg/vmcp/server/classification.go b/pkg/vmcp/server/classification.go index 16e2386ad4..9d5db19754 100644 --- a/pkg/vmcp/server/classification.go +++ b/pkg/vmcp/server/classification.go @@ -9,21 +9,28 @@ import ( mcpparser "github.com/stacklok/toolhive/pkg/mcp" ) -// methodServerDiscover is the Modern (2026-07-28) capability-probe method. It is -// special-cased in two places — the kill-switch branch below and dispatchModern's -// method switch — because it is how a client learns which revisions a server -// supports, so it must never be refused on version grounds. +// methodServerDiscover is the Modern (2026-07-28) capability-probe method — how +// a client learns which revisions a server supports. dispatchModern's method +// switch answers it (dispatchModernDiscover); classifyingHandler treats it like +// any other Modern method. const methodServerDiscover = "server/discover" // classifyingHandler classifies a parsed MCP request as Legacy (2025-11-25) or // Modern (2026-07-28) at the decode seam, rejects a malformed Modern request -// with the correct JSON-RPC error before it reaches dispatch, and — when -// Config.ModernDispatchEnabled — routes a well-formed Modern request to -// dispatchModern instead of the SDK. Legacy traffic always falls through to -// next unchanged. While the switch is off (default), a well-formed Modern -// request is refused with a conformant -32022 rather than falling through — -// see the kill-switch branch for why, and for why server/discover is exempt -// and does still fall through. +// with the correct JSON-RPC error before it reaches dispatch, and routes a +// well-formed Modern request to dispatchModern instead of the SDK. Legacy +// traffic always falls through to next unchanged. +// +// Classification is exact-match on both the MCP-Protocol-Version header and the +// reserved io.modelcontextprotocol/* _meta keys (ClassifyRevision), so nothing +// that is not unambiguously Modern reaches dispatchModern: Legacy wire behavior +// is unaffected by this handler. +// +// There is deliberately no "vMCP has chosen not to serve this revision" refusal: +// vMCP serves Modern unconditionally since #5959 removed the temporary +// kill-switch. A request whose _meta names some OTHER protocol version is still +// refused with a conformant -32022 UnsupportedProtocolVersionError — by +// ClassifyRevision below, which owns every version-grounds rejection. // // ValidateHeaderConsistency (Mcp-Method/Mcp-Name) only applies to Modern // requests: a Legacy request carrying a stray Mcp-Method/Mcp-Name header @@ -74,38 +81,6 @@ func (s *Server) classifyingHandler(next http.Handler) http.Handler { return } - // TEMPORARY kill-switch (default off): until Modern dispatch is - // conformance-validated, vMCP does not serve the Modern revision unless - // explicitly enabled. See issue #5959. - // - // Answer that conformantly here rather than letting the request reach the - // SDK. The draft's Streamable HTTP "Protocol Version Header" section - // requires a server that does not implement a requested version -- "whether - // the version is unknown to the server, or is a known version the server has - // chosen not to support" -- to reply 400 with an UnsupportedProtocolVersionError - // listing the versions it does support. A disabled kill switch is exactly the - // second case. Falling through instead yields go-sdk's stateful-server - // rejection, which is a 400 with a PLAIN-TEXT body whose text is Go-API advice - // for the server author ("set StreamableHTTPOptions.Stateless = true") -- not - // parseable as a protocol error and carrying no version list. - // - // server/discover is deliberately exempt: it is how a client learns which - // revisions a server supports, so rejecting it on version grounds would leave - // the client no way to negotiate down. go-sdk exempts it for the same reason, - // and its stateful path answers discover correctly, so the fall-through is - // still right for that one method. - if !s.config.ModernDispatchEnabled { - if parsed.Method == methodServerDiscover { - next.ServeHTTP(w, r) - return - } - mcpparser.WriteClassificationError(w, parsed.ID, &mcpparser.UnsupportedVersionError{ - Requested: mcpparser.MCPVersionModern, - Supported: []string{mcpparser.MCPVersionLegacy}, - }) - return - } - s.dispatchModern(w, r, parsed) }) } diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index f727e627dd..479dc9b084 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -38,15 +38,13 @@ type classificationErrorBody struct { } // classifyingHandlerTestServer builds a minimal *Server for driving -// classifyingHandler in isolation, carrying only the two fields the handler -// reads beyond config scalars: the kill-switch and the core a switch-on -// dispatch routes to. -func classifyingHandlerTestServer(modernDispatchEnabled bool) *Server { +// classifyingHandler in isolation, carrying only what the handler reads beyond +// config scalars: the core a Modern dispatch routes to. +func classifyingHandlerTestServer() *Server { return &Server{ config: &Config{ - Name: testServerName, - Version: testServerVersion, - ModernDispatchEnabled: modernDispatchEnabled, + Name: testServerName, + Version: testServerVersion, }, core: &modernFakeCore{tools: []vmcp.Tool{{Name: "echo", InputSchema: map[string]any{"type": "object"}}}}, } @@ -56,13 +54,12 @@ func TestClassifyingHandler(t *testing.T) { t.Parallel() tests := []struct { - name string - parsed *mcpparser.ParsedMCPRequest - protocolHeader string - modernDispatchEnabled bool - wantPassthrough bool - wantDispatched bool - wantCode int64 + name string + parsed *mcpparser.ParsedMCPRequest + protocolHeader string + wantPassthrough bool + wantDispatched bool + wantCode int64 }{ { name: "nil parsed request passes through", @@ -91,36 +88,23 @@ func TestClassifyingHandler(t *testing.T) { { // tools/list is deliberately not in the Mcp-Name-required set, so this // case only needs Mcp-Method (required on every Modern request) to pass - // ValidateHeaderConsistency; with the kill-switch on, a well-formed - // Modern request then dispatches to the core instead of falling - // through to next. - name: "well-formed modern request dispatches to the core when the kill-switch is on", - parsed: wellFormedModernToolsList(), - protocolHeader: mcpparser.MCPVersionModern, - modernDispatchEnabled: true, - wantDispatched: true, - }, - { - // Same well-formed Modern request, but with the kill-switch at its - // default (off): vMCP does not serve the Modern revision, which the - // draft says must be answered with 400 + UnsupportedProtocolVersion - // listing the supported versions. It must NOT reach next: falling - // through lands on go-sdk's stateful rejection, a plain-text 400 no - // client can parse as a protocol error. - name: "well-formed modern request is refused with -32022 when the kill-switch is off", + // ValidateHeaderConsistency. vMCP serves the Modern revision + // unconditionally (#5959), so a well-formed Modern request always + // dispatches to the core rather than falling through to next. + name: "well-formed modern request dispatches to the core", parsed: wellFormedModernToolsList(), protocolHeader: mcpparser.MCPVersionModern, - wantCode: mcpparser.CodeUnsupportedProtocolVersion, + wantDispatched: true, }, { - // server/discover is the one exemption: it is how a client learns - // which revisions the server supports, so refusing it on version - // grounds would leave the client unable to negotiate down. go-sdk's - // stateful path answers discover, so the fall-through is correct. - name: "server/discover still falls through when the kill-switch is off", - parsed: wellFormedModernDiscover(), - protocolHeader: mcpparser.MCPVersionModern, - wantPassthrough: true, + // server/discover is dispatched like any other Modern method now that + // Modern is served unconditionally. It used to be exempted from a + // version refusal so a client could still negotiate down; with nothing + // refusing on version grounds, dispatchModern answers it directly. + name: "server/discover dispatches to the core", + parsed: wellFormedModernDiscover(), + protocolHeader: mcpparser.MCPVersionModern, + wantDispatched: true, }, { // A body that is otherwise a well-formed Modern request (valid _meta @@ -271,7 +255,7 @@ func TestClassifyingHandler(t *testing.T) { }) rec := httptest.NewRecorder() - classifyingHandlerTestServer(tt.modernDispatchEnabled).classifyingHandler(next).ServeHTTP(rec, req) + classifyingHandlerTestServer().classifyingHandler(next).ServeHTTP(rec, req) if tt.wantPassthrough { assert.True(t, nextCalled, "expected the request to fall through to next") @@ -312,8 +296,8 @@ func wellFormedModernToolsList() *mcpparser.ParsedMCPRequest { } } -// wellFormedModernDiscover is wellFormedModernToolsList for the one method the -// kill-switch branch exempts from the unsupported-version refusal. +// wellFormedModernDiscover is wellFormedModernToolsList for server/discover, the +// Modern capability probe. func wellFormedModernDiscover() *mcpparser.ParsedMCPRequest { return &mcpparser.ParsedMCPRequest{ Method: methodServerDiscover, diff --git a/pkg/vmcp/server/derive.go b/pkg/vmcp/server/derive.go index 15310ad52e..a1799babc7 100644 --- a/pkg/vmcp/server/derive.go +++ b/pkg/vmcp/server/derive.go @@ -77,7 +77,6 @@ func deriveServerConfig( EndpointPath: cfg.EndpointPath, SessionTTL: cfg.SessionTTL, HeartbeatInterval: cfg.HeartbeatInterval, - ModernDispatchEnabled: cfg.ModernDispatchEnabled, AuthMiddleware: cfg.AuthMiddleware, AuthInfoHandler: cfg.AuthInfoHandler, PassthroughHeaders: cfg.PassthroughHeaders, diff --git a/pkg/vmcp/server/derive_test.go b/pkg/vmcp/server/derive_test.go index a031e09aad..645d8cddbc 100644 --- a/pkg/vmcp/server/derive_test.go +++ b/pkg/vmcp/server/derive_test.go @@ -40,7 +40,6 @@ func populatedLegacyConfig() *Config { EndpointPath: "/custom", SessionTTL: 17 * time.Minute, HeartbeatInterval: 5 * time.Second, - ModernDispatchEnabled: true, AuthMiddleware: passthrough, AuthzMiddleware: passthrough, AuthInfoHandler: http.NewServeMux(), @@ -73,7 +72,6 @@ func TestDeriveServerConfigProjectsTransportFields(t *testing.T) { assert.Equal(t, "/custom", got.EndpointPath) assert.Equal(t, 17*time.Minute, got.SessionTTL) assert.Equal(t, 5*time.Second, got.HeartbeatInterval) - assert.True(t, got.ModernDispatchEnabled) assert.Equal(t, 11*time.Second, got.StatusReportingInterval) // Func/handler/pointer fields projected by reference. diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index e3605dabfb..4aa19880d8 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -155,14 +155,13 @@ func startForwardingBackend(t *testing.T) string { // // 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. +// docs/arch/10-virtual-mcp-architecture.md for the full disposition. Modern +// dispatch is now unconditional (#5959 removed the kill-switch), so without +// this pin the go-sdk-based client would negotiate Modern and the surface +// under test would vanish mid-test — failing at connect rather than at the +// behavior being asserted. Before that, the server's own version-omitting +// discover answer pinned these clients to Legacy incidentally; the pin makes +// the dependency explicit instead of relying on that accident. // // The pin lives in the transport because mcpcompat documents it cannot set a // protocol version (go-sdk's ClientSessionOptions.protocolVersion is diff --git a/pkg/vmcp/server/modern_dispatch_test.go b/pkg/vmcp/server/modern_dispatch_test.go index d4c4503ba0..fb0d0a0df9 100644 --- a/pkg/vmcp/server/modern_dispatch_test.go +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -304,7 +304,7 @@ func TestDispatchModern_PingRealParser(t *testing.T) { req.Header.Set("MCP-Protocol-Version", mcpparser.MCPVersionModern) req.Header.Set("Mcp-Method", "ping") - s := classifyingHandlerTestServer(true) + s := classifyingHandlerTestServer() nextCalled := false next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { nextCalled = true }) diff --git a/pkg/vmcp/server/modern_pagination.go b/pkg/vmcp/server/modern_pagination.go index 1259360ed5..98a8cda320 100644 --- a/pkg/vmcp/server/modern_pagination.go +++ b/pkg/vmcp/server/modern_pagination.go @@ -106,9 +106,9 @@ import ( // 1000-item first page). The Legacy side is not, because holding a client on // Legacy would need a negotiation-pinning mechanism that does not exist here -- // mcpcompat cannot request a protocol version (#5911) -- and -// test/integration/vmcp's Over1000Tools regression exercises the Legacy split -// only incidentally today, becoming a Modern test once the kill-switch is -// removed. +// test/integration/vmcp's Over1000Tools regression now exercises the Modern +// split rather than the Legacy one, since Modern dispatch became unconditional +// (#5959). // // So: treat the equality claimed above as an invariant maintained by REVIEW, not // by CI, and re-check it on any go-sdk bump. This deliberately does not name a diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go index 7c312b49db..5b60103a68 100644 --- a/pkg/vmcp/server/modern_realbackend_integration_test.go +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -123,7 +123,7 @@ func TestIntegration_Modern_RealBackend_ToolCall(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ "name": "echo", @@ -147,36 +147,13 @@ func TestIntegration_Modern_RealBackend_ToolCall(t *testing.T) { assert.NotEqual(t, true, result["isError"], "tool call must not be marked as an error") } -// TestIntegration_Modern_RealBackend_KillSwitchOff verifies that with the -// Modern dispatch kill-switch at its default (off), a well-formed Modern -// tools/call request is NOT served by dispatchModern: it falls through to the -// SDK path, which has no session for this request and so cannot produce a -// Modern envelope (no "resultType" in the response, and no 200 as -// TestIntegration_Modern_RealBackend_ToolCall gets with the switch on). -func TestIntegration_Modern_RealBackend_KillSwitchOff(t *testing.T) { - t.Parallel() - - backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) - - resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ - "name": "echo", - "arguments": map[string]any{"input": "hello modern"}, - }, 1, "echo") - defer resp.Body.Close() - - assert.NotEqual(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) - result, _ := decoded["result"].(map[string]any) - assert.NotContains(t, result, "resultType", "must not be served by dispatchModern: decoded: %+v", decoded) -} - // TestIntegration_Modern_RealBackend_ToolsList verifies tools/list against the // real backend's discovered tool set, with the Modern cacheability envelope. func TestIntegration_Modern_RealBackend_ToolsList(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/list", nil, 1, "") defer resp.Body.Close() @@ -203,7 +180,7 @@ func TestIntegration_Modern_RealBackend_Discover(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "server/discover", nil, 1, "") defer resp.Body.Close() @@ -235,7 +212,7 @@ func TestIntegration_Modern_RealBackend_Complete(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "completion/complete", map[string]any{ "ref": map[string]any{"type": "ref/prompt", "name": "nonexistent"}, @@ -261,7 +238,7 @@ func TestIntegration_Modern_RealBackend_Ping(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "ping", nil, 7, "") defer resp.Body.Close() @@ -281,7 +258,7 @@ func TestIntegration_Modern_RealBackend_Notification(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/list", nil, nil, "") defer resp.Body.Close() @@ -301,7 +278,7 @@ func TestIntegration_Modern_RealBackend_UnknownMethod(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "resources/subscribe", map[string]any{"uri": "file:///x"}, 1, "") defer resp.Body.Close() @@ -341,7 +318,7 @@ func TestIntegration_Modern_RealBackend_ElicitingToolFailsCleanly(t *testing.T) t.Parallel() backendURL := startForwardingBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) tests := []struct { name string @@ -396,7 +373,7 @@ func TestIntegration_Modern_RealBackend_ProgressDropped(t *testing.T) { t.Parallel() backendURL := startForwardingBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ "name": fwdProgressTool, @@ -439,7 +416,7 @@ func TestIntegration_Modern_RealBackend_LoggingContract(t *testing.T) { t.Parallel() backendURL := startForwardingBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) t.Run("well-formed setLevel is method-not-found", func(t *testing.T) { t.Parallel() @@ -496,7 +473,7 @@ func TestIntegration_Modern_RealBackend_MalformedArguments(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ "name": "echo", @@ -534,7 +511,7 @@ func TestIntegration_Modern_RealBackend_MidCallCapabilityContract(t *testing.T) t.Parallel() backendURL := startForwardingBackend(t) - ts := newRealModernTestServer(t, backendURL) + ts := newRealTestServer(t, backendURL) tests := []struct { name string diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index df991b69ed..97ca01793a 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -66,11 +66,6 @@ type ServerConfig struct { // connections (default: 30s when zero). HeartbeatInterval time.Duration - // ModernDispatchEnabled turns on direct dispatch of well-formed MCP - // 2026-07-28 ("Modern") stateless requests to the vMCP core, bypassing the - // SDK Serve/session layer (default false; see Config.ModernDispatchEnabled). - ModernDispatchEnabled bool - // AuthMiddleware is the optional authentication middleware applied to MCP routes. // If nil, no authentication is required. AuthMiddleware func(http.Handler) http.Handler @@ -419,7 +414,6 @@ func buildServeConfig(cfg *ServerConfig) *Config { EndpointPath: cfg.EndpointPath, SessionTTL: cfg.SessionTTL, HeartbeatInterval: cfg.HeartbeatInterval, - ModernDispatchEnabled: cfg.ModernDispatchEnabled, AuthMiddleware: cfg.AuthMiddleware, AuthInfoHandler: cfg.AuthInfoHandler, PassthroughHeaders: cfg.PassthroughHeaders, diff --git a/pkg/vmcp/server/serve_test.go b/pkg/vmcp/server/serve_test.go index e75ce34dff..cdfc7ebd22 100644 --- a/pkg/vmcp/server/serve_test.go +++ b/pkg/vmcp/server/serve_test.go @@ -363,7 +363,6 @@ func TestBuildServeConfigMapsSharedFields(t *testing.T) { EndpointPath: "/e", SessionTTL: time.Second, HeartbeatInterval: time.Second, - ModernDispatchEnabled: true, AuthMiddleware: func(h http.Handler) http.Handler { return h }, AuthInfoHandler: http.NewServeMux(), PassthroughHeaders: []string{"x-test"}, diff --git a/pkg/vmcp/server/server.go b/pkg/vmcp/server/server.go index 1062fa7776..2ec12bc59e 100644 --- a/pkg/vmcp/server/server.go +++ b/pkg/vmcp/server/server.go @@ -151,18 +151,6 @@ type Config struct { // later is a one-line change rather than a re-thread through the server. HeartbeatInterval time.Duration - // ModernDispatchEnabled turns on direct dispatch of well-formed MCP - // 2026-07-28 ("Modern") stateless requests to the vMCP core - // (classifyingHandler → dispatchModern), bypassing the SDK Serve/session - // layer. When false (the default), a well-formed Modern request falls - // through to the SDK path unchanged, byte-identical to the pre-Modern- - // dispatch wire behavior. - // - // TEMPORARY: this is a safety lever for the hand-rolled pre-release Modern - // envelope, off by default until Modern dispatch is conformance-validated. - // Remove once that validation lands; see issue #5959. - ModernDispatchEnabled bool - // AuthMiddleware is the optional authentication middleware to apply to MCP routes. // If nil, no authentication is required. // This should be a composed middleware chain (e.g., TokenValidator + MCP parser). @@ -646,12 +634,11 @@ func (s *Server) Handler(_ context.Context) (http.Handler, error) { var mcpHandler http.Handler = streamableServer // Classify Modern (2026-07-28) vs Legacy at the decode seam, reject - // malformed Modern requests before dispatch, and — when - // Config.ModernDispatchEnabled — route well-formed Modern requests to the - // vMCP core (dispatchModern) instead of the SDK. Applied before telemetry - // (i.e. it runs closer to the handler) so a rejection or a dispatcher 403 - // is still recorded by the telemetry middleware instead of bypassing it - // entirely. + // malformed Modern requests before dispatch, and route well-formed Modern + // requests to the vMCP core (dispatchModern) instead of the SDK. Applied + // before telemetry (i.e. it runs closer to the handler) so a rejection or a + // dispatcher 403 is still recorded by the telemetry middleware instead of + // bypassing it entirely. mcpHandler = s.classifyingHandler(mcpHandler) if s.config.TelemetryProvider != nil { diff --git a/pkg/vmcp/server/session_management_realbackend_integration_test.go b/pkg/vmcp/server/session_management_realbackend_integration_test.go index e95c534a77..bcd265aa74 100644 --- a/pkg/vmcp/server/session_management_realbackend_integration_test.go +++ b/pkg/vmcp/server/session_management_realbackend_integration_test.go @@ -36,29 +36,12 @@ import ( // startRealMCPBackend is defined in testutil_test.go as a shared test utility. // newRealTestHandler builds the full vMCP handler backed by the MCP server at -// backendURL, with Modern dispatch's kill-switch at its default (off): a -// well-formed Modern (2026-07-28) request falls through to the SDK path, same -// as a Legacy request. It is the low-level helper used by newRealTestServer -// and any test that needs control over the httptest.Server configuration -// (e.g. WriteTimeout). Use newRealModernTestHandler for a switch-on handler. +// backendURL. A well-formed Modern (2026-07-28) request routes through +// classifyingHandler -> dispatchModern; a Legacy request falls through to the +// SDK path. It is the low-level helper used by newRealTestServer and any test +// that needs control over the httptest.Server configuration (e.g. WriteTimeout). func newRealTestHandler(t *testing.T, backendURL string) http.Handler { t.Helper() - return newRealTestHandlerWithConfig(t, backendURL, false) -} - -// newRealModernTestHandler is newRealTestHandler with the Modern dispatch -// kill-switch on: a well-formed Modern request routes through -// classifyingHandler -> dispatchModern instead of falling through to the SDK. -func newRealModernTestHandler(t *testing.T, backendURL string) http.Handler { - t.Helper() - return newRealTestHandlerWithConfig(t, backendURL, true) -} - -// newRealTestHandlerWithConfig is the shared construction path for -// newRealTestHandler and newRealModernTestHandler, parameterized on the Modern -// dispatch kill-switch so the two stay in lockstep other than that one field. -func newRealTestHandlerWithConfig(t *testing.T, backendURL string, modernDispatchEnabled bool) http.Handler { - t.Helper() ctrl := gomock.NewController(t) t.Cleanup(ctrl.Finish) @@ -100,12 +83,11 @@ func newRealTestHandlerWithConfig(t *testing.T, backendURL string, modernDispatc srv, err := server.New( context.Background(), &server.Config{ - Host: "127.0.0.1", - Port: 0, - SessionTTL: 5 * time.Minute, - ModernDispatchEnabled: modernDispatchEnabled, - SessionFactory: factory, - Aggregator: agg, + Host: "127.0.0.1", + Port: 0, + SessionTTL: 5 * time.Minute, + SessionFactory: factory, + Aggregator: agg, }, rt, backendClient, @@ -120,9 +102,10 @@ func newRealTestHandlerWithConfig(t *testing.T, backendURL string, modernDispatc } // newRealTestServer builds a vMCP server with session management and a real -// SessionFactory, Modern dispatch's kill-switch off (the default). The -// BackendRegistry mock returns the backend at backendURL so that -// CreateSession() opens a real HTTP connection to the MCP server. +// SessionFactory. The BackendRegistry mock returns the backend at backendURL so +// that CreateSession() opens a real HTTP connection to the MCP server. Serves +// both revisions: Legacy through the SDK path, Modern through +// classifyingHandler -> dispatchModern. func newRealTestServer(t *testing.T, backendURL string) *httptest.Server { t.Helper() ts := httptest.NewServer(newRealTestHandler(t, backendURL)) @@ -130,16 +113,6 @@ func newRealTestServer(t *testing.T, backendURL string) *httptest.Server { return ts } -// newRealModernTestServer is newRealTestServer with the Modern dispatch -// kill-switch on, for tests that exercise the classifyingHandler -> -// dispatchModern path against a real backend. -func newRealModernTestServer(t *testing.T, backendURL string) *httptest.Server { - t.Helper() - ts := httptest.NewServer(newRealModernTestHandler(t, backendURL)) - t.Cleanup(ts.Close) - return ts -} - // waitForEchoTool polls tools/list until the "echo" tool appears or the // deadline elapses. It relies on require.Eventually so the test fails // immediately on timeout. diff --git a/test/e2e/vmcp_dual_era_helpers_test.go b/test/e2e/vmcp_dual_era_helpers_test.go index 6d3acafed2..fad8c11baf 100644 --- a/test/e2e/vmcp_dual_era_helpers_test.go +++ b/test/e2e/vmcp_dual_era_helpers_test.go @@ -20,11 +20,6 @@ import ( "github.com/stacklok/toolhive/test/e2e/images" ) -// modernDispatchEnvVar mirrors pkg/vmcp/cli/serve.go's modernDispatchEnvVar -// constant (unexported there, so duplicated here rather than imported -- -// this suite only needs the string value, not the package). -const modernDispatchEnvVar = "TOOLHIVE_VMCP_MODERN_STATELESS" - // launchYardstickLegacyOnPort starts a non-stateless (Legacy-only) yardstick // backend on the given port in the given group. Deliberately structured as a // near-duplicate of launchYardstickModernOnPort below rather than reusing the @@ -140,47 +135,20 @@ func appendHealthCheckConfig(path string) { Expect(err).ToNot(HaveOccurred()) } -// startDualEraVMCP starts `thv vmcp serve`, with the Modern dispatcher -// enabled or disabled per modernEnabled. Uses --config configPath when -// configPath is non-empty, else falls back to quick mode (--group groupName). -// This exists instead of reusing e2e.StartLongRunningTHVCommand solely to -// control that one env var: StartLongRunningTHVCommand always inherits a plain -// os.Environ() with no override hook. -// -// The variable is stripped from the inherited environment and then set -// explicitly for BOTH cases, rather than only appended when enabling. Leaving -// it inherited would make the kill-switch-off specs depend on the ambient -// environment: a CI runner or developer shell exporting -// TOOLHIVE_VMCP_MODERN_STATELESS=true would start those with Modern dispatch -// ON, and the spec would fail for a reason that has nothing to do with the -// code. Appending an override would also work (serve.go parses the value with -// strconv.ParseBool, and exec dedups Env keeping the last entry), but relying -// on that dedup order is too subtle to leave to a reader. -func startDualEraVMCP(config *e2e.TestConfig, groupName, configPath string, port int, modernEnabled bool) *exec.Cmd { - env := make([]string, 0, len(os.Environ())+1) - for _, kv := range os.Environ() { - if !strings.HasPrefix(kv, modernDispatchEnvVar+"=") { - env = append(env, kv) - } - } - env = append(env, fmt.Sprintf("%s=%t", modernDispatchEnvVar, modernEnabled)) - +// startDualEraVMCP starts `thv vmcp serve`, using --config configPath when +// configPath is non-empty, else quick mode (--group groupName). vMCP serves both +// MCP revisions unconditionally (#5959), so this only assembles the arguments — +// it no longer overrides the inherited environment, and so delegates the process +// start to e2e.StartLongRunningTHVCommand. +func startDualEraVMCP(config *e2e.TestConfig, groupName, configPath string, port int) *exec.Cmd { + GinkgoHelper() args := []string{"vmcp", "serve", "--port", strconv.Itoa(port)} if configPath != "" { args = append(args, "--config", configPath) } else { args = append(args, "--group", groupName) } - - //nolint:gosec // fixed subcommand, test-controlled args - cmd := exec.Command(config.THVBinary, args...) - cmd.Env = env - cmd.Stdout = GinkgoWriter - cmd.Stderr = GinkgoWriter - - err := cmd.Start() - ExpectWithOffset(1, err).ToNot(HaveOccurred(), "failed to start thv vmcp serve") - return cmd + return e2e.StartLongRunningTHVCommand(config, args...) } // vmcpStatusResponse decodes the subset of pkg/vmcp/server.StatusResponse diff --git a/test/e2e/vmcp_dual_era_test.go b/test/e2e/vmcp_dual_era_test.go index 9f5b35b260..5999f0a61c 100644 --- a/test/e2e/vmcp_dual_era_test.go +++ b/test/e2e/vmcp_dual_era_test.go @@ -49,7 +49,7 @@ import ( // an SSE body whenever that header is present (see mcp_raw_client.go). So the // bridge is proven here only under a non-conformant Accept header. var _ = Describe("vMCP Dual-Era Bridge", Label("vmcp", "dual-era", "e2e"), Serial, func() { - Context("Modern dispatcher enabled", func() { + Context("one Legacy and one Modern backend in the same group", func() { var ( config *e2e.TestConfig groupName string @@ -98,8 +98,8 @@ var _ = Describe("vMCP Dual-Era Bridge", Label("vmcp", "dual-era", "e2e"), Seria initVMCPConfig(config, groupName, configFilePath) appendHealthCheckConfig(configFilePath) - By("starting thv vmcp serve with the Modern dispatcher enabled") - vMCPCmd = startDualEraVMCP(config, groupName, configFilePath, vMCPPort, true) + By("starting thv vmcp serve") + vMCPCmd = startDualEraVMCP(config, groupName, configFilePath, vMCPPort) err = e2e.WaitForMCPServerReady(config, vMCPURL, "streamable-http", 60*time.Second) Expect(err).ToNot(HaveOccurred(), "vMCP server should become ready") @@ -204,70 +204,6 @@ var _ = Describe("vMCP Dual-Era Bridge", Label("vmcp", "dual-era", "e2e"), Seria } }) }) - - Context("Modern dispatcher disabled (kill switch off)", func() { - var ( - config *e2e.TestConfig - groupName string - backendName string - toolName string - vMCPCmd *exec.Cmd - vMCPPort int - vMCPURL string - rawClient *e2e.RawMCPClient - ) - - BeforeEach(func() { - config = e2e.NewTestConfig() - groupName = e2e.GenerateUniqueServerName("vmcp-dual-era-off-group") - backendName = e2e.GenerateUniqueServerName("vmcp-dual-era-off-backend") - toolName = backendName + "_echo" - vMCPCmd = nil - vMCPPort = allocateVMCPPort() - vMCPURL = vmcpEndpointURL(vMCPPort) - - err := e2e.CheckTHVBinaryAvailable(config) - Expect(err).ToNot(HaveOccurred(), "thv binary should be available") - - rawClient, err = e2e.NewRawMCPClient(20 * time.Second) - Expect(err).ToNot(HaveOccurred()) - - By("creating a group with one Modern-capable backend") - e2e.NewTHVCommand(config, "group", "create", groupName).ExpectSuccess() - startYardstickModernOnPort(config, groupName, backendName, allocateVMCPPort()) - - By("starting thv vmcp serve with the Modern dispatcher left at its default (off)") - vMCPCmd = startDualEraVMCP(config, groupName, "", vMCPPort, false) - err = e2e.WaitForMCPServerReady(config, vMCPURL, "streamable-http", 60*time.Second) - Expect(err).ToNot(HaveOccurred(), "vMCP server should become ready") - }) - - AfterEach(func() { - stopVMCPProcess(vMCPCmd) - vMCPCmd = nil - - if config.CleanupAfter { - _ = e2e.StopAndRemoveMCPServer(config, backendName) - _ = e2e.RemoveGroup(config, groupName) - } - }) - - It("does not serve a well-formed Modern request via the Modern dispatcher", func() { - // Mirrors TestIntegration_Modern_RealBackend_KillSwitchOff - // (pkg/vmcp/server/modern_realbackend_integration_test.go): with the - // kill-switch at its default (off), a well-formed Modern tools/call - // falls through to the SDK path instead of dispatchModern, which - // (confirmed live) rejects the Modern protocol version outright on a - // non-stateless server -- a plain-text HTTP 400 carrying no - // "resultType" at all. - resp := modernToolCall(context.Background(), rawClient, vMCPURL, toolName, "killswitchoff") - Expect(resp.StatusCode).To(Equal(400), "body: %s", resp.Body) - - var decoded map[string]any - _ = json.Unmarshal(resp.Body, &decoded) - Expect(decoded).ToNot(HaveKey("resultType"), "must not be served by dispatchModern: %s", resp.Body) - }) - }) }) // legacyInitialize sends a Legacy initialize request, followed by the From e331b1da59853443593186a5ab0e55c3d494d52e Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 28 Jul 2026 09:23:31 +0000 Subject: [PATCH 2/7] Gate Modern dispatch on instance capabilities Removing the kill-switch unconditionally exposed a real gap: go-sdk v1.7+ clients probe server/discover before initialize and upgrade to 2026-07-28 on advertisement, so clients of an optimizer-enabled vMCP landed on the stateless dispatch path and silently received the raw aggregated tool set instead of find_tool/call_tool (the optimizer is Serve-layer and session-scoped by design). Both e2e runs on this PR failed on exactly that. Replace the env var with modernDispatchBlockers: an explicit, commented enumeration of enabled features the stateless path cannot serve. Instances with no blockers advertise and dispatch Modern; a gated instance lets server/discover fall through to the SDK (whose stateful transport advertises a Legacy-only version list, steering Modern-first clients onto the Legacy handshake without an error) and refuses other Modern requests with a conformant -32022 listing Legacy. Redis session storage deliberately does not gate: Legacy clients keep shared, reconstructible sessions while Modern clients are sessionless by design, a coexistence virtualmcp_dual_era_redis_test.go asserts. That test's now-dead TOOLHIVE_VMCP_MODERN_STATELESS pod-template env is removed and its era-pinning comments updated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam --- docs/arch/10-virtual-mcp-architecture.md | 49 +++++++ pkg/vmcp/server/classification.go | 58 +++++++- pkg/vmcp/server/classification_test.go | 138 +++++++++++++++++- pkg/vmcp/server/modern_gate.go | 57 ++++++++ .../server/modern_gate_integration_test.go | 109 ++++++++++++++ pkg/vmcp/server/serve.go | 10 ++ .../virtualmcp_dual_era_redis_test.go | 58 +++----- 7 files changed, 427 insertions(+), 52 deletions(-) create mode 100644 pkg/vmcp/server/modern_gate.go create mode 100644 pkg/vmcp/server/modern_gate_integration_test.go diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index f083b96a90..9a7a9de392 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -412,6 +412,55 @@ The four Modern list verbs (`tools/list`, `resources/list`, `resources/templates The completion handler is a single global handler installed via `WithCompletionHandler`, so it recovers the session from the SDK request context rather than a per-session closure. Setting it makes the shim auto-advertise the `completions` capability at initialize. +### Served MCP revisions: the Modern capability gate + +vMCP serves two client-facing MCP revisions: **Legacy** (2025-11-25, the SDK +session path) always, and **Modern** (2026-07-28, `classifyingHandler → +dispatchModern`, stateless) **conditionally** — only when every enabled feature +of the instance is servable by the stateless dispatch path. The condition is +`modernDispatchBlockers` (`pkg/vmcp/server/modern_gate.go`), an explicit +enumeration that replaced the temporary `TOOLHIVE_VMCP_MODERN_STATELESS` +env-var kill-switch (#5959): instead of a global "don't serve Modern", the +instance serves Modern exactly when it can serve it correctly. + +Features that currently gate Modern off, and why: + +| Feature | Why the stateless path cannot serve it | +|---------|----------------------------------------| +| Optimizer (`find_tool`/`call_tool`) | The meta-tools are Serve-layer and **session-scoped** (`serve_optimizer.go`): each session builds an FTS5 index over its advertised set and swaps the two meta-tools in. The index is deliberately not in the stateless core, and `dispatchModern` serves `tools/*` straight from `core.ListTools`/`core.CallTool` — a Modern client would silently receive the raw aggregated tool set and `tools/call find_tool` would fail. Parity needs an identity- or instance-scoped index | + +"Cannot serve" means a Modern client would silently get different behavior than +the feature promises — not merely that the feature is session-flavored. +Redis-backed session sharing, for example, does **not** gate Modern: Legacy +clients keep their shared, reconstructible sessions while Modern clients are +sessionless by design and store nothing, a coexistence asserted end-to-end by +`test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go`. + +Wire behavior when the gate is closed: + +- **`server/discover` falls through to the SDK** instead of dispatching. go-sdk's + stateful transport filters 2026-07-28 out of its `supportedVersions` + (`SupportsProtocolVersion` requires `Stateless`), so the probe answer + advertises Legacy only. This matters because go-sdk v1.7+ clients are + Modern-first: `Connect` probes `server/discover` **before** `initialize` and + upgrades to whatever the server advertises. The fall-through answer is what + lands them on the Legacy handshake — where sessions, and every gated feature, + work. +- **Every other well-formed Modern request** is refused with a conformant + 400 + `-32022 UnsupportedProtocolVersionError` whose data lists the Legacy + version, the shape a client negotiates down from. It is answered in + `classifyingHandler` rather than falling through, because go-sdk's stateful + rejection for Modern traffic is a plain-text 400 carrying no version list. +- **Legacy traffic is untouched** either way; the gate only ever affects + requests that classified Modern. + +The gate is derived from construction-time configuration, logged once at +startup ("MCP 2026-07-28 (Modern) dispatch disabled…"), and pinned by +`TestModernDispatchBlockers`, `TestClassifyingHandler_ModernCapabilityGate`, +and the full-handler pair in `modern_gate_integration_test.go`. Achieving +Modern parity for a feature means deleting its entry and updating those tests — +nothing else needs to change. + ### Subscription limitation (ack-level) vMCP advertises `resources.subscribe: true` and answers `resources/subscribe` / `resources/unsubscribe` at **ack level**: the request is accepted (enforcing session binding and validating the URI is an advertised, admitted resource), and go-sdk records the subscription. vMCP does **not** currently propagate backend `notifications/resources/updated` to the subscribed client — doing so requires persistent per-session backend connections, which is out of scope. Clients that subscribe will receive a success ack but no update stream yet. diff --git a/pkg/vmcp/server/classification.go b/pkg/vmcp/server/classification.go index 9d5db19754..d4a7d15a94 100644 --- a/pkg/vmcp/server/classification.go +++ b/pkg/vmcp/server/classification.go @@ -10,9 +10,11 @@ import ( ) // methodServerDiscover is the Modern (2026-07-28) capability-probe method — how -// a client learns which revisions a server supports. dispatchModern's method -// switch answers it (dispatchModernDiscover); classifyingHandler treats it like -// any other Modern method. +// a client learns which revisions a server supports. It is special-cased in two +// places — the capability-gate branch in classifyingHandler and dispatchModern's +// method switch (dispatchModernDiscover) — because it must never be refused on +// version grounds: it is the one request a client needs answered to negotiate +// down. const methodServerDiscover = "server/discover" // classifyingHandler classifies a parsed MCP request as Legacy (2025-11-25) or @@ -26,11 +28,16 @@ const methodServerDiscover = "server/discover" // that is not unambiguously Modern reaches dispatchModern: Legacy wire behavior // is unaffected by this handler. // -// There is deliberately no "vMCP has chosen not to serve this revision" refusal: -// vMCP serves Modern unconditionally since #5959 removed the temporary -// kill-switch. A request whose _meta names some OTHER protocol version is still -// refused with a conformant -32022 UnsupportedProtocolVersionError — by -// ClassifyRevision below, which owns every version-grounds rejection. +// Whether vMCP serves the Modern revision at all is a per-instance capability +// question, not a global switch: #5959's temporary env-var kill-switch is +// replaced by modernDispatchBlockers (modern_gate.go), which enumerates the +// enabled features the stateless dispatch path cannot serve. When that list is +// empty (the common case) a well-formed Modern request dispatches; when it is +// not, the gate branch below refuses Modern with a conformant -32022 listing +// Legacy so capable clients negotiate down and reach the SDK path, where every +// enabled feature works. A request whose _meta names some OTHER protocol +// version is refused with the same -32022 by ClassifyRevision below, which +// owns every other version-grounds rejection. // // ValidateHeaderConsistency (Mcp-Method/Mcp-Name) only applies to Modern // requests: a Legacy request carrying a stray Mcp-Method/Mcp-Name header @@ -81,6 +88,41 @@ func (s *Server) classifyingHandler(next http.Handler) http.Handler { return } + // Capability gate: when this instance has enabled features the stateless + // dispatch path cannot serve (modernDispatchBlockers, modern_gate.go), + // vMCP does not serve the Modern revision. + // + // Answer that conformantly here rather than letting the request reach the + // SDK. The draft's Streamable HTTP "Protocol Version Header" section + // requires a server that does not implement a requested version -- "whether + // the version is unknown to the server, or is a known version the server has + // chosen not to support" -- to reply 400 with an UnsupportedProtocolVersionError + // listing the versions it does support. A gated instance is exactly the + // second case. Falling through instead yields go-sdk's stateful-server + // rejection, which is a 400 with a PLAIN-TEXT body whose text is Go-API advice + // for the server author ("set StreamableHTTPOptions.Stateless = true") -- not + // parseable as a protocol error and carrying no version list. + // + // server/discover is deliberately exempt: it is how a client learns which + // revisions a server supports, so rejecting it on version grounds would leave + // the client no way to negotiate down. go-sdk exempts it for the same reason, + // and its stateful path answers discover with the transport-filtered version + // list -- which excludes 2026-07-28 for a stateful transport -- so the + // fall-through is not just tolerable but the correct advertisement: a + // Modern-first client (go-sdk v1.7+ probes discover before initialize) + // reads it and lands on the Legacy handshake without ever seeing an error. + if blocked := s.modernDispatchBlockers(); len(blocked) > 0 { + if parsed.Method == methodServerDiscover { + next.ServeHTTP(w, r) + return + } + mcpparser.WriteClassificationError(w, parsed.ID, &mcpparser.UnsupportedVersionError{ + Requested: mcpparser.MCPVersionModern, + Supported: []string{mcpparser.MCPVersionLegacy}, + }) + return + } + s.dispatchModern(w, r, parsed) }) } diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index 479dc9b084..4212fbb08c 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" @@ -14,8 +15,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + mcpserver "github.com/stacklok/toolhive-core/mcpcompat/server" mcpparser "github.com/stacklok/toolhive/pkg/mcp" "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/optimizer" ) // Reserved Modern _meta keys, mirrored from pkg/mcp/revision.go's unexported @@ -88,19 +91,21 @@ func TestClassifyingHandler(t *testing.T) { { // tools/list is deliberately not in the Mcp-Name-required set, so this // case only needs Mcp-Method (required on every Modern request) to pass - // ValidateHeaderConsistency. vMCP serves the Modern revision - // unconditionally (#5959), so a well-formed Modern request always - // dispatches to the core rather than falling through to next. + // ValidateHeaderConsistency. This test server has no enabled feature + // the stateless path cannot serve (modernDispatchBlockers is empty), + // so a well-formed Modern request dispatches to the core rather than + // falling through to next. The gated counterpart is + // TestClassifyingHandler_ModernCapabilityGate. name: "well-formed modern request dispatches to the core", parsed: wellFormedModernToolsList(), protocolHeader: mcpparser.MCPVersionModern, wantDispatched: true, }, { - // server/discover is dispatched like any other Modern method now that - // Modern is served unconditionally. It used to be exempted from a - // version refusal so a client could still negotiate down; with nothing - // refusing on version grounds, dispatchModern answers it directly. + // With no capability-gate blockers, server/discover is dispatched like + // any other Modern method: dispatchModern answers it directly. When + // the gate is active it instead falls through to the SDK so the client + // can negotiate down — see TestClassifyingHandler_ModernCapabilityGate. name: "server/discover dispatches to the core", parsed: wellFormedModernDiscover(), protocolHeader: mcpparser.MCPVersionModern, @@ -310,3 +315,122 @@ func wellFormedModernDiscover() *mcpparser.ParsedMCPRequest { MCPMethodHeader: methodServerDiscover, } } + +// stubOptimizerFactory marks the optimizer as enabled for gate tests. The gate +// (modernDispatchBlockers) only checks the factory for nil-ness — no gated +// request ever builds an optimizer — so returning an error is safe and keeps +// the stub honest about never being invoked. +func stubOptimizerFactory(context.Context, []mcpserver.ServerTool) (optimizer.Optimizer, error) { + return nil, errors.New("stub optimizer factory must not be invoked by the capability gate") +} + +// TestModernDispatchBlockers pins the gate's contents: exactly which enabled +// features keep an instance off the Modern revision, by name. A parity change +// (deleting an entry in modern_gate.go) must flip a case here. +func TestModernDispatchBlockers(t *testing.T) { + t.Parallel() + + plain := classifyingHandlerTestServer() + assert.Empty(t, plain.modernDispatchBlockers(), + "an instance with no Serve-layer-only features must serve Modern") + + withOptimizer := classifyingHandlerTestServer() + withOptimizer.optimizerFactory = stubOptimizerFactory + assert.Equal(t, []string{"optimizer"}, withOptimizer.modernDispatchBlockers(), + "the session-scoped optimizer is not servable by the stateless Modern path") +} + +// TestClassifyingHandler_ModernCapabilityGate pins the wire behavior of the +// capability gate for an instance with a feature the stateless Modern path +// cannot serve (the optimizer): +// +// - A well-formed Modern request is refused with a conformant 400 + -32022 +// UnsupportedProtocolVersionError whose data lists the Legacy version, so a +// Modern-first client (go-sdk v1.7+ probes before initialize) negotiates +// down cleanly instead of hitting go-sdk's unparseable plain-text +// stateful rejection. +// - server/discover is exempt and falls through to the SDK path, whose +// stateful transport advertises the Legacy-only version list — the one +// request a client needs answered to negotiate down. +// - Legacy traffic falls through untouched: the gate only ever refuses +// requests that classified Modern. +// +// The ungated counterparts (same requests dispatching to the core) live in +// TestClassifyingHandler; together the two pin that the gate cannot be +// "simplified" away in either direction. +func TestClassifyingHandler_ModernCapabilityGate(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parsed *mcpparser.ParsedMCPRequest + protocolHeader string + wantPassthrough bool + }{ + { + name: "modern tools/list is refused with -32022 listing Legacy", + parsed: wellFormedModernToolsList(), + protocolHeader: mcpparser.MCPVersionModern, + }, + { + name: "server/discover falls through to the SDK so the client can negotiate down", + parsed: wellFormedModernDiscover(), + protocolHeader: mcpparser.MCPVersionModern, + wantPassthrough: true, + }, + { + name: "legacy request falls through untouched", + parsed: &mcpparser.ParsedMCPRequest{ + Method: "tools/list", + ID: "1", + IsRequest: true, + }, + protocolHeader: mcpparser.MCPVersionLegacy, + wantPassthrough: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + srv := classifyingHandlerTestServer() + srv.optimizerFactory = stubOptimizerFactory + + ctx := context.WithValue(t.Context(), mcpparser.MCPRequestContextKey, tt.parsed) + req := httptest.NewRequest(http.MethodPost, "/mcp", nil).WithContext(ctx) + req.Header.Set("MCP-Protocol-Version", tt.protocolHeader) + + nextCalled := false + next := http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + nextCalled = true + w.WriteHeader(http.StatusOK) + }) + + rec := httptest.NewRecorder() + srv.classifyingHandler(next).ServeHTTP(rec, req) + + if tt.wantPassthrough { + assert.True(t, nextCalled, "expected the request to fall through to next") + return + } + assert.False(t, nextCalled, "expected the gate to short-circuit before next") + assert.Equal(t, http.StatusBadRequest, rec.Code) + + var body struct { + Error struct { + Code int64 `json:"code"` + Data struct { + Requested string `json:"requested"` + Supported []string `json:"supported"` + } `json:"data"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + assert.Equal(t, mcpparser.CodeUnsupportedProtocolVersion, body.Error.Code) + assert.Equal(t, mcpparser.MCPVersionModern, body.Error.Data.Requested) + assert.Equal(t, []string{mcpparser.MCPVersionLegacy}, body.Error.Data.Supported, + "the refusal must list the Legacy version so the client can negotiate down") + }) + } +} diff --git a/pkg/vmcp/server/modern_gate.go b/pkg/vmcp/server/modern_gate.go new file mode 100644 index 0000000000..4b71348e5b --- /dev/null +++ b/pkg/vmcp/server/modern_gate.go @@ -0,0 +1,57 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +// modernDispatchBlockers enumerates the enabled features of THIS instance that +// the stateless Modern (2026-07-28) dispatch path cannot serve yet. It is the +// capability gate that decides whether vMCP advertises and serves the Modern +// revision: an empty result means every enabled feature is servable by +// dispatchModern and Modern requests are dispatched; a non-empty result means +// classifyingHandler keeps Modern-capable clients on Legacy (see the gate +// branch there for the wire mechanics). +// +// Contract for this list: +// +// - One entry per feature, each guarded by the narrowest signal that the +// feature is actually enabled on this instance, with a comment saying WHY +// the Modern path cannot serve it. "Cannot serve" means a Modern client +// would silently receive different behavior than the feature promises — +// not merely that the feature is session-flavored. A feature that Modern +// clients simply don't need (e.g. Redis-backed session sharing: Legacy +// clients keep their shared sessions, Modern clients are sessionless by +// design and store nothing) does NOT belong here; coexistence of that kind +// is asserted by test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go. +// +// - When Modern parity lands for a feature, delete its entry. The gate's +// behavior is pinned by TestModernDispatchBlockers and +// TestClassifyingHandler_ModernCapabilityGate (classification_test.go) plus +// the full-handler pair in modern_gate_integration_test.go; deleting an +// entry must flip cases there, so parity work cannot silently ship without +// updating them. +// +// The result is derived from construction-time configuration only, so it is +// constant for the life of the Server; Serve logs it once at startup. +func (s *Server) modernDispatchBlockers() []string { + var blocked []string + + // Optimizer: find_tool/call_tool are Serve-layer, session-scoped meta-tools + // (serve_optimizer.go). Each session builds an FTS5 index over its advertised + // set and swaps the two meta-tools in place of the raw tools; the index is + // transport/session state and is deliberately NOT in the stateless core. + // dispatchModern serves tools/list and tools/call straight from + // core.ListTools/core.CallTool, so a Modern client of an optimizer-enabled + // instance would silently receive the full raw aggregated tool set and + // `tools/call find_tool` would fail -32603 "not found" — the optimizer + // feature would be invisibly disabled for exactly the newest clients. + // Modern parity needs an identity- or instance-scoped index to replace the + // session-scoped one; until that lands, an optimizer-enabled instance is + // Legacy-only. s.optimizerFactory is the resolved factory and is non-nil on + // both composition paths (New and direct Serve) exactly when the optimizer + // is enabled. + if s.optimizerFactory != nil { + blocked = append(blocked, "optimizer") + } + + return blocked +} diff --git a/pkg/vmcp/server/modern_gate_integration_test.go b/pkg/vmcp/server/modern_gate_integration_test.go new file mode 100644 index 0000000000..9570c6aaa7 --- /dev/null +++ b/pkg/vmcp/server/modern_gate_integration_test.go @@ -0,0 +1,109 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server_test + +import ( + "context" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + mcpsdk "github.com/stacklok/toolhive-core/mcpcompat/server" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/optimizer" +) + +// supportedVersionsFromDiscover extracts result.supportedVersions from a decoded +// server/discover envelope, failing loudly when the field is absent or malformed +// so a "does not contain" assertion can never pass vacuously against the wrong +// response shape. +func supportedVersionsFromDiscover(t *testing.T, decoded map[string]any) []string { + t.Helper() + result, ok := decoded["result"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC result envelope, got %v", decoded) + raw, ok := result["supportedVersions"].([]any) + require.True(t, ok, "expected result.supportedVersions, got %v", result) + require.NotEmpty(t, raw, "supportedVersions must never be empty: %v", result) + versions := make([]string, 0, len(raw)) + for _, v := range raw { + s, ok := v.(string) + require.True(t, ok, "supportedVersions entries must be strings: %v", raw) + versions = append(versions, s) + } + return versions +} + +// TestIntegration_ModernGate_OptimizerKeepsClientsOnLegacy pins the capability +// gate (modernDispatchBlockers, modern_gate.go) through the full handler chain +// for an optimizer-enabled instance — the configuration whose find_tool/ +// call_tool meta-tools are session-scoped and therefore unservable by the +// stateless Modern dispatch path: +// +// 1. server/discover must NOT advertise 2026-07-28: the gate lets it fall +// through to the SDK, whose stateful transport filters Modern out of the +// version list. This is the leg that steers a Modern-first client (go-sdk +// v1.7+ probes discover before initialize) onto the Legacy handshake, +// where sessions advertise find_tool/call_tool. +// 2. Any other well-formed Modern request is refused with a conformant +// 400 + -32022 UnsupportedProtocolVersionError listing the Legacy version +// — never go-sdk's plain-text stateful rejection, which carries no +// version list a client could negotiate down from. +// +// The contrast leg (an instance without the optimizer advertises and serves +// Modern) is TestIntegration_ModernGate_PlainInstanceAdvertisesModern; the two +// together stop the gate from being "simplified" away in either direction. +// Deleting the optimizer entry from modernDispatchBlockers (Modern optimizer +// parity) must flip this test. +func TestIntegration_ModernGate_OptimizerKeepsClientsOnLegacy(t *testing.T) { + t.Parallel() + + tool := vmcp.Tool{Name: "echo", Description: "echoes", InputSchema: map[string]any{"type": "object"}} + ts := buildTestServerWithOptions(t, newNoopMockFactory(t), serverOptions{ + tools: []vmcp.Tool{tool}, + optimizerFactory: func(_ context.Context, _ []mcpsdk.ServerTool) (optimizer.Optimizer, error) { + return &fakeOptimizer{}, nil + }, + }) + + discoverResp, discoverBody := postModern(t, ts.URL, "server/discover", nil, 1, "") + defer discoverResp.Body.Close() + require.Equal(t, http.StatusOK, discoverResp.StatusCode, + "gated discover must still be answered (fall-through to the SDK): %+v", discoverBody) + versions := supportedVersionsFromDiscover(t, discoverBody) + assert.NotContains(t, versions, "2026-07-28", + "an optimizer-enabled instance must not advertise Modern") + assert.Contains(t, versions, "2025-11-25", + "the Legacy version must stay advertised so the client can negotiate down") + + listResp, listBody := postModern(t, ts.URL, "tools/list", nil, 2, "") + defer listResp.Body.Close() + assert.Equal(t, http.StatusBadRequest, listResp.StatusCode, "decoded: %+v", listBody) + errObj, ok := listBody["error"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC error envelope, got %+v", listBody) + assert.Equal(t, float64(-32022), errObj["code"], + "refusal must be the conformant UnsupportedProtocolVersionError") + data, ok := errObj["data"].(map[string]any) + require.True(t, ok, "expected error.data with the version lists, got %+v", errObj) + assert.Equal(t, []any{"2025-11-25"}, data["supported"], + "the refusal must list the Legacy version so the client can negotiate down") +} + +// TestIntegration_ModernGate_PlainInstanceAdvertisesModern is the contrast leg +// of TestIntegration_ModernGate_OptimizerKeepsClientsOnLegacy: with no enabled +// feature the stateless path cannot serve, the gate is open, server/discover is +// answered by dispatchModernDiscover, and 2026-07-28 is advertised. +func TestIntegration_ModernGate_PlainInstanceAdvertisesModern(t *testing.T) { + t.Parallel() + + tool := vmcp.Tool{Name: "echo", Description: "echoes", InputSchema: map[string]any{"type": "object"}} + ts := buildTestServerWithOptions(t, newNoopMockFactory(t), serverOptions{tools: []vmcp.Tool{tool}}) + + resp, decoded := postModern(t, ts.URL, "server/discover", nil, 1, "") + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + assert.Contains(t, supportedVersionsFromDiscover(t, decoded), "2026-07-28", + "a plain instance must advertise Modern") +} diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index 97ca01793a..635a3fd2e6 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -6,6 +6,7 @@ package server import ( "context" "fmt" + "log/slog" "net/http" "time" @@ -377,6 +378,15 @@ func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) srv.lazyInjectSessionTools(hookCtx) }) + // Surface the capability gate's verdict once at startup: the blocker list is + // derived from construction-time configuration and cannot change afterwards, + // so this single line is the operator-visible record of why Modern-capable + // clients of this instance negotiate down to Legacy (see modern_gate.go). + if blocked := srv.modernDispatchBlockers(); len(blocked) > 0 { + slog.Info("MCP 2026-07-28 (Modern) dispatch disabled: enabled features require the session (Legacy) path", + "features", blocked) + } + // Disarm the close-on-error guard: the Server is fully constructed. closeStorageOnErr = false return srv, nil diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go index 41a211240d..3d7fb2fec5 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go @@ -16,7 +16,6 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" @@ -38,12 +37,18 @@ const dualEraRedisKeyPrefix = "thv:vmcp:dualera:" // This spec is the Redis-backed counterpart of virtualmcp_dual_era_backends_test.go: // same mixed Legacy+Modern backend set behind one vMCP, but with session storage -// backed by Redis instead of the in-memory default, and the Modern client -// dispatcher explicitly enabled via pod-template env. It exists as the shared +// backed by Redis instead of the in-memory default. It exists as the shared // fixture for two follow-on specs (not yet added): concurrent Legacy+Modern // traffic against the Redis-backed session store, and a Redis-outage spec that // scales Redis to 0 replicas mid-test. // +// This file is also the end-to-end assertion that Redis session storage does +// NOT close vMCP's Modern capability gate (modernDispatchBlockers, +// pkg/vmcp/server/modern_gate.go): Legacy clients keep their shared, +// reconstructible sessions while Modern clients are served statelessly and +// store nothing. Redis therefore must never be added to the gate's blocker +// list — the Modern legs below fail with -32022 if it is. +// // Both backends run the SAME yardstick-server image and set BACKEND_MODE=echo // explicitly so that STATELESS is the ONLY difference between them. (TRANSPORT // is not set here at all: CreateMultipleMCPServersInParallel (helpers.go @@ -63,18 +68,19 @@ const dualEraRedisKeyPrefix = "thv:vmcp:dualera:" // IMPORTANT -- this spec must NOT use CreateInitializedMCPClient, WaitForExpectedTools, // or ToolsContainAll (all in helpers.go / wait_for_tools_helpers.go). Those build a // go-sdk/mcpcompat client, and go-sdk v1.7's Connect is Modern-first: it sends -// server/discover before anything else. With the Modern dispatcher on (this file's -// TOOLHIVE_VMCP_MODERN_STATELESS pod-template env), classifyingHandler routes -// server/discover to dispatchModern instead of the SDK's stateful fallback -// (pkg/vmcp/server/classification.go:97-110), so such a client always negotiates -// Modern and gets no session -- there is no way to make it open a Legacy session -// against this vMCP. The observed failure is mcpcompat's Initialize unconditionally -// installing list-changed handlers, which makes go-sdk open a "subscriptions/listen" -// stream vMCP does not implement, answered with HTTP 404 and surfaced as go-sdk's -// ErrSessionMissing ("session not found") despite Initialize having "succeeded". -// Use *e2e.RawMCPClient instead: it pins the era explicitly per request (Legacy via -// e2e.NewLegacyRequest + the MCP-Protocol-Version header), which is what actually -// gets a Legacy session from a Modern-dispatch-enabled vMCP. +// server/discover before anything else. This vMCP serves Modern (Redis session +// storage does not close the capability gate -- see the header above), so +// classifyingHandler routes server/discover to dispatchModern instead of the +// SDK's stateful fallback (pkg/vmcp/server/classification.go), and such a client +// always negotiates Modern and gets no session -- there is no way to make it open +// a Legacy session against this vMCP. The observed failure is mcpcompat's +// Initialize unconditionally installing list-changed handlers, which makes go-sdk +// open a "subscriptions/listen" stream vMCP does not implement, answered with +// HTTP 404 and surfaced as go-sdk's ErrSessionMissing ("session not found") +// despite Initialize having "succeeded". Use *e2e.RawMCPClient instead: it pins +// the era explicitly per request (Legacy via e2e.NewLegacyRequest + the +// MCP-Protocol-Version header), which is what actually gets a Legacy session +// from a Modern-serving vMCP. var _ = Describe("VirtualMCPServer Dual-Era Backends over Redis Session Storage", Ordered, func() { var ( timeout = 5 * time.Minute @@ -153,27 +159,6 @@ var _ = Describe("VirtualMCPServer Dual-Era Backends over Redis Session Storage" }, }, timeout, pollingInterval) - // The Modern client dispatcher is env-only with no CRD field and defaults - // to OFF (pkg/vmcp/cli/serve.go's modernDispatchEnvVar); without this - // override every Modern request below would return 400 regardless of - // Redis, and Steps 3/4's Modern legs would be testing nothing. Follows the - // PodTemplateSpec env-injection pattern in - // virtualmcp_auth_discovery_test.go:825-846: container name must be "vmcp". - podTemplateSpec := corev1.PodTemplateSpec{ - Spec: corev1.PodSpec{ - Containers: []corev1.Container{ - { - Name: "vmcp", - Env: []corev1.EnvVar{ - {Name: "TOOLHIVE_VMCP_MODERN_STATELESS", Value: "true"}, - }, - }, - }, - }, - } - podTemplateRaw, err := json.Marshal(podTemplateSpec) - Expect(err).ToNot(HaveOccurred()) - By("Creating VirtualMCPServer over the mixed backend set with Redis session storage") vmcpServer := v1beta1test.NewVirtualMCPServer(vmcpServerName, defaultNamespace, v1beta1test.WithVMCPGroupRef(mcpGroupName), @@ -225,7 +210,6 @@ var _ = Describe("VirtualMCPServer Dual-Era Backends over Redis Session Storage" // to -- so it is deliberately left unset rather than set to a value // that would do nothing. v1beta1test.WithVMCPReplicas(1), - v1beta1test.WithVMCPPodTemplateSpec(&runtime.RawExtension{Raw: podTemplateRaw}), v1beta1test.MutateVMCP(func(v *mcpv1beta1.VirtualMCPServer) { v.Spec.ServiceType = "NodePort" }), From de9cf0cc1e69844223d581d95da421f27ab8c342 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 28 Jul 2026 09:39:08 +0000 Subject: [PATCH 3/7] Pin operator vMCP session specs to Legacy explicitly Specs that assert session semantics -- Redis cross-pod reconstruction, pod-restart recovery, lazy termination eviction, upstreamInject restore, HMAC session token binding, and session independence -- were building their sessions through the mcpcompat client. That client wraps go-sdk, whose v1.7 Connect is Modern-first (it probes server/discover before initialize and upgrades on advertisement) and cannot be pinned to a protocol version (#5911). Now that vMCP serves 2026-07-28 -- which removed sessions -- whenever the capability gate is open, those clients negotiate Modern, get no session, and every sessionID assertion fails. Port them onto RawMCPClient with per-request Legacy pinning, following the convention virtualmcp_dual_era_redis_test.go's header prescribes and the precedent #6051 set: a spec about sessions now declares, at each call site, that it depends on the Legacy revision. Sessions are Legacy-only semantics under 2026-07-28, so the pin narrows nothing. The shared primitives live in legacy_session_helpers_test.go; the dual-era file's Legacy helpers delegate to them. MCPServer and MCPRemoteProxy scaling specs are untouched: the transparent proxy negotiates against the backend, which stays Legacy. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GvBj7nNWqDQ3rKYNeY6aam --- pkg/vmcp/server/classification_test.go | 2 +- test/e2e/thv-operator/virtualmcp/helpers.go | 12 + .../virtualmcp/legacy_session_helpers_test.go | 146 ++++++++++++ .../virtualmcp_dual_era_redis_test.go | 79 +----- .../virtualmcp_redis_session_test.go | 225 +++++++----------- .../virtualmcp_session_management_test.go | 171 +++++++------ 6 files changed, 349 insertions(+), 286 deletions(-) create mode 100644 test/e2e/thv-operator/virtualmcp/legacy_session_helpers_test.go diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index 4212fbb08c..cc3633b80e 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -347,7 +347,7 @@ func TestModernDispatchBlockers(t *testing.T) { // - A well-formed Modern request is refused with a conformant 400 + -32022 // UnsupportedProtocolVersionError whose data lists the Legacy version, so a // Modern-first client (go-sdk v1.7+ probes before initialize) negotiates -// down cleanly instead of hitting go-sdk's unparseable plain-text +// down cleanly instead of hitting go-sdk's unparsable plain-text // stateful rejection. // - server/discover is exempt and falls through to the SDK path, whose // stateful transport advertises the Legacy-only version list — the one diff --git a/test/e2e/thv-operator/virtualmcp/helpers.go b/test/e2e/thv-operator/virtualmcp/helpers.go index f2dc452bd9..a14bbb8a1c 100644 --- a/test/e2e/thv-operator/virtualmcp/helpers.go +++ b/test/e2e/thv-operator/virtualmcp/helpers.go @@ -112,6 +112,18 @@ func (c *InitializedMCPClient) Close() { // CreateInitializedMCPClient creates an MCP client, starts the transport, and initializes // the connection with the given client name. Returns an InitializedMCPClient that should // be closed when done using defer client.Close(). +// +// DO NOT use this (or any mcpcompat-built client) in a spec that asserts +// SESSION semantics against a vMCP. The underlying go-sdk client is +// Modern-first — it probes server/discover before initialize and upgrades to +// 2026-07-28 whenever the server advertises it — and cannot be pinned to +// Legacy (#5911). Against a Modern-serving vMCP it gets no session, and +// GetSessionId() is empty. Session specs must pin the era explicitly with the +// raw Legacy primitives in legacy_session_helpers_test.go instead. (Specs that +// only assert tool/resource behavior are era-agnostic and may use this freely; +// clients of the thv-proxyrunner transparent proxy negotiate against the +// BACKEND, so MCPServer/MCPRemoteProxy session specs are unaffected as long as +// the backend itself is Legacy.) func CreateInitializedMCPClient(nodePort int32, clientName string, timeout time.Duration) (*InitializedMCPClient, error) { serverURL := fmt.Sprintf("http://localhost:%d/mcp", nodePort) mcpClient, err := mcpclient.NewStreamableHttpClient(serverURL) diff --git a/test/e2e/thv-operator/virtualmcp/legacy_session_helpers_test.go b/test/e2e/thv-operator/virtualmcp/legacy_session_helpers_test.go new file mode 100644 index 0000000000..d5a50e969a --- /dev/null +++ b/test/e2e/thv-operator/virtualmcp/legacy_session_helpers_test.go @@ -0,0 +1,146 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package virtualmcp + +import ( + "context" + "encoding/json" + "fmt" + + coremcp "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/test/e2e" +) + +// This file holds the era-pinned Legacy (2025-11-25) session primitives shared +// by the specs that assert Legacy-session semantics — Redis-backed cross-pod +// reconstruction, pod-restart recovery, lazy termination eviction, +// upstreamInject identity restore, and the dual-era Redis suite. +// +// They exist because such specs must NOT use CreateInitializedMCPClient (or +// any mcpcompat-built client): mcpcompat wraps go-sdk, go-sdk v1.7's Connect +// is Modern-first — it probes server/discover before initialize and upgrades +// to 2026-07-28 whenever the server advertises it — and the shim cannot pin a +// protocol version (#5911). Against a Modern-serving vMCP such a client +// negotiates Modern, which has no sessions, and every session assertion fails. +// The raw client pins the era explicitly per request instead, which is the +// point: a spec about sessions declares, at each call site, that it depends on +// the Legacy revision (the #6051 convention). +// +// Both helpers return errors rather than asserting, so they are safe inside +// Eventually/Consistently retry loops and on goroutines without GinkgoRecover. + +// legacySessionInit performs the Legacy initialize + notifications/initialized +// handshake against url and returns the session ID vMCP assigned. extraHeaders +// (e.g. Authorization for OIDC-protected instances) are applied to every +// request of the handshake. +func legacySessionInit( + client *e2e.RawMCPClient, url, clientName string, extraHeaders map[string]string, +) (string, error) { + ctx := context.Background() + req := e2e.NewLegacyInitializeRequest(clientName, "1.0") + for k, v := range extraHeaders { + req.SetHeader(k, v) + } + resp, err := client.Send(ctx, url, req) + if err != nil { + return "", fmt.Errorf("initialize: %w", err) + } + if resp.StatusCode != 200 { + return "", fmt.Errorf("initialize: status %d, body: %s", resp.StatusCode, resp.Body) + } + sessionID := resp.Headers.Get(e2e.HeaderMCPSessionID) + if sessionID == "" { + return "", fmt.Errorf("initialize did not assign a session id") + } + + notifyReq, err := e2e.NewLegacyRequest("notifications/initialized", nil) + if err != nil { + return "", fmt.Errorf("build notifications/initialized: %w", err) + } + // WithID(nil) omits "id" entirely, making this a true JSON-RPC notification. + notifyReq.WithID(nil).WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) + for k, v := range extraHeaders { + notifyReq.SetHeader(k, v) + } + notifyResp, err := client.Send(ctx, url, notifyReq) + if err != nil { + return "", fmt.Errorf("notifications/initialized: %w", err) + } + if notifyResp.StatusCode != 202 { + return "", fmt.Errorf("notifications/initialized: status %d, body: %s", notifyResp.StatusCode, notifyResp.Body) + } + + return sessionID, nil +} + +// legacySessionCallTool sends a Legacy tools/call for toolName on sessionID +// and returns the raw response. It deliberately does NOT inspect +// resp.StatusCode/Error — a non-2xx response is a normal, successfully-received +// HTTP response some callers assert on (e.g. rejection specs); only an actual +// transport failure from Send is surfaced as the returned error. For a plain +// "the call worked and echoed X" assertion, pass the response to +// dualEraEchoErr(resp, wantOutput, "") — the empty resultType is what a Legacy +// client's envelope carries. +func legacySessionCallTool( + client *e2e.RawMCPClient, url, sessionID, toolName string, + args map[string]any, extraHeaders map[string]string, +) (*e2e.RawResponse, error) { + req, err := e2e.NewLegacyRequest("tools/call", map[string]any{ + "name": toolName, + "arguments": args, + }) + if err != nil { + return nil, fmt.Errorf("build tools/call: %w", err) + } + req.WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) + for k, v := range extraHeaders { + req.SetHeader(k, v) + } + return client.Send(context.Background(), url, req) +} + +// legacySessionListTools sends a Legacy tools/list on sessionID and returns +// the aggregated tool names. Sending it to a pod that has never seen sessionID +// is exactly what triggers a Redis-backed session reconstruction there, so the +// cross-pod specs call this against pod B with pod A's session. extraHeaders +// are applied to the request. +func legacySessionListTools( + client *e2e.RawMCPClient, url, sessionID string, extraHeaders map[string]string, +) ([]string, error) { + req, err := e2e.NewLegacyRequest("tools/list", nil) + if err != nil { + return nil, fmt.Errorf("build tools/list: %w", err) + } + req.WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) + for k, v := range extraHeaders { + req.SetHeader(k, v) + } + resp, err := client.Send(context.Background(), url, req) + if err != nil { + return nil, fmt.Errorf("tools/list: %w", err) + } + if resp.StatusCode != 200 { + return nil, fmt.Errorf("tools/list: status %d, body: %s", resp.StatusCode, resp.Body) + } + // A schema-rejected tools/list comes back as a JSON-RPC error inside a 200 + // (unlike tools/call, where yardstick reports validation failure via + // isError:true), so this check is meaningful. + if resp.Error != nil { + return nil, fmt.Errorf("tools/list: JSON-RPC error: %+v", resp.Error) + } + + var result struct { + Tools []struct { + Name string `json:"name"` + } `json:"tools"` + } + if err := json.Unmarshal(resp.Result, &result); err != nil { + return nil, fmt.Errorf("tools/list: unmarshal result: %w, raw: %s", err, resp.Result) + } + names := make([]string, len(result.Tools)) + for i, t := range result.Tools { + names[i] = t.Name + } + return names, nil +} diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go index 3d7fb2fec5..7ae1211b1e 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go @@ -636,77 +636,20 @@ func dualEraLegacyInit(mcpClient *e2e.RawMCPClient, url string) string { } // dualEraLegacyInitErr is the error-returning variant of dualEraLegacyInit, -// for use inside an Eventually retry loop. +// for use inside an Eventually retry loop. It delegates to the shared +// legacySessionInit primitive (legacy_session_helpers_test.go). func dualEraLegacyInitErr(mcpClient *e2e.RawMCPClient, url string) (string, error) { - ctx := context.Background() - req := e2e.NewLegacyInitializeRequest("dual-era-redis-legacy-client", "1.0") - resp, err := mcpClient.Send(ctx, url, req) - if err != nil { - return "", fmt.Errorf("initialize: %w", err) - } - if resp.StatusCode != 200 { - return "", fmt.Errorf("initialize: status %d, body: %s", resp.StatusCode, resp.Body) - } - sessionID := resp.Headers.Get(e2e.HeaderMCPSessionID) - if sessionID == "" { - return "", fmt.Errorf("initialize did not assign a session id") - } - - notifyReq, err := e2e.NewLegacyRequest("notifications/initialized", nil) - if err != nil { - return "", fmt.Errorf("build notifications/initialized: %w", err) - } - // WithID(nil) omits "id" entirely, making this a true JSON-RPC notification. - notifyReq.WithID(nil).WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) - notifyResp, err := mcpClient.Send(ctx, url, notifyReq) - if err != nil { - return "", fmt.Errorf("notifications/initialized: %w", err) - } - if notifyResp.StatusCode != 202 { - return "", fmt.Errorf("notifications/initialized: status %d, body: %s", notifyResp.StatusCode, notifyResp.Body) - } - - return sessionID, nil + return legacySessionInit(mcpClient, url, "dual-era-redis-legacy-client", nil) } // dualEraLegacyListTools sends a Legacy tools/list on sessionID and returns -// the aggregated tool names. +// the aggregated tool names. It delegates to the shared legacySessionListTools +// primitive (legacy_session_helpers_test.go). // // Returns an error rather than using Expect/Ginkgo assertions so it is safe // to call from inside an Eventually retry loop (e.g. the BeforeAll warm-up). func dualEraLegacyListTools(mcpClient *e2e.RawMCPClient, url, sessionID string) ([]string, error) { - req, err := e2e.NewLegacyRequest("tools/list", nil) - if err != nil { - return nil, fmt.Errorf("build tools/list: %w", err) - } - req.WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) - resp, err := mcpClient.Send(context.Background(), url, req) - if err != nil { - return nil, fmt.Errorf("tools/list: %w", err) - } - if resp.StatusCode != 200 { - return nil, fmt.Errorf("tools/list: status %d, body: %s", resp.StatusCode, resp.Body) - } - // A schema-rejected tools/list would come back as a JSON-RPC error here (unlike - // tools/call, which yardstick answers with isError:true inside a 200 -- see the - // dualEraLegacyCall/dualEraModernCall doc comment), so this check is meaningful. - if resp.Error != nil { - return nil, fmt.Errorf("tools/list: JSON-RPC error: %+v", resp.Error) - } - - var result struct { - Tools []struct { - Name string `json:"name"` - } `json:"tools"` - } - if err := json.Unmarshal(resp.Result, &result); err != nil { - return nil, fmt.Errorf("tools/list: unmarshal result: %w, raw: %s", err, resp.Result) - } - names := make([]string, len(result.Tools)) - for i, t := range result.Tools { - names[i] = t.Name - } - return names, nil + return legacySessionListTools(mcpClient, url, sessionID, nil) } // dualEraLegacyCall sends a Legacy tools/call for toolName on sessionID, @@ -743,15 +686,7 @@ func dualEraLegacyCall(mcpClient *e2e.RawMCPClient, url, sessionID, toolName, in // error; only an actual transport failure from Send (e.g. connection // refused) is surfaced as the returned error. func dualEraLegacyCallErr(mcpClient *e2e.RawMCPClient, url, sessionID, toolName, input string) (*e2e.RawResponse, error) { - req, err := e2e.NewLegacyRequest("tools/call", map[string]any{ - "name": toolName, - "arguments": map[string]any{"input": input}, - }) - if err != nil { - return nil, fmt.Errorf("build tools/call: %w", err) - } - req.WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) - return mcpClient.Send(context.Background(), url, req) + return legacySessionCallTool(mcpClient, url, sessionID, toolName, map[string]any{"input": input}, nil) } // dualEraModernCall sends a stateless Modern tools/call for toolName. Mirrors diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go index 0296e418d0..a2785b6ac5 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_redis_session_test.go @@ -11,7 +11,6 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" - "errors" "fmt" "net/http" "time" @@ -24,11 +23,10 @@ import ( "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" - mcpclient "github.com/stacklok/toolhive-core/mcpcompat/client" - "github.com/stacklok/toolhive-core/mcpcompat/client/transport" - "github.com/stacklok/toolhive-core/mcpcompat/mcp" mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1/v1beta1test" + coremcp "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/test/e2e" "github.com/stacklok/toolhive/test/e2e/images" ) @@ -278,18 +276,22 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() gomega.Expect(err).NotTo(gomega.HaveOccurred()) defer cleanupB() - ginkgo.By("Initializing session on pod A") - clientA, err := CreateInitializedMCPClient(int32(localPortA), "e2e-redis-test", 30*time.Second) + // Era-pinned raw client: this spec asserts Legacy-session semantics, so + // it must pin 2025-11-25 per request (see legacy_session_helpers_test.go + // for why the mcpcompat client cannot be used here). + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer clientA.Close() + serverURLA := fmt.Sprintf("http://localhost:%d/mcp", localPortA) + serverURLB := fmt.Sprintf("http://localhost:%d/mcp", localPortB) - sessionID := clientA.Client.GetSessionId() - gomega.Expect(sessionID).NotTo(gomega.BeEmpty(), "session ID must be assigned after Initialize") + ginkgo.By("Initializing a Legacy session on pod A") + sessionID, err := legacySessionInit(rawClient, serverURLA, "e2e-redis-test", nil) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "session ID must be assigned after Initialize") ginkgo.By("Listing tools on pod A") - toolsA, err := clientA.Client.ListTools(clientA.Ctx, mcp.ListToolsRequest{}) + toolsA, err := legacySessionListTools(rawClient, serverURLA, sessionID, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(toolsA.Tools).NotTo(gomega.BeEmpty(), "pod A must return tools") + gomega.Expect(toolsA).NotTo(gomega.BeEmpty(), "pod A must return tools") ginkgo.By("Verifying pod A stored backend session IDs in Redis") backendIDsBeforeRestore, err := readRedisSessionBackendIDs(redisName, "thv:vmcp:e2e:", sessionID) @@ -297,22 +299,11 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() gomega.Expect(backendIDsBeforeRestore).NotTo(gomega.BeEmpty(), "pod A must have written per-backend session IDs to Redis so pod B can use them as hints") - ginkgo.By(fmt.Sprintf("Connecting to pod B (%s) with the same session ID", podB.Name)) - serverURLB := fmt.Sprintf("http://localhost:%d/mcp", localPortB) - clientB, err := mcpclient.NewStreamableHttpClient(serverURLB, transport.WithSession(sessionID)) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer func() { _ = clientB.Close() }() - - startCtx, startCancel := context.WithTimeout(context.Background(), 30*time.Second) - defer startCancel() - gomega.Expect(clientB.Start(startCtx)).To(gomega.Succeed()) - - ginkgo.By("Listing tools on pod B using the session from pod A") - listCtx, listCancel := context.WithTimeout(context.Background(), 30*time.Second) - defer listCancel() - toolsB, err := clientB.ListTools(listCtx, mcp.ListToolsRequest{}) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(toolsB.Tools).NotTo(gomega.BeEmpty(), "pod B must return tools via Redis-reconstructed session") + ginkgo.By(fmt.Sprintf("Listing tools on pod B (%s) with the session from pod A", podB.Name)) + toolsB, err := legacySessionListTools(rawClient, serverURLB, sessionID, nil) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), + "pod B must serve the session it has never seen by reconstructing it from Redis") + gomega.Expect(toolsB).NotTo(gomega.BeEmpty(), "pod B must return tools via Redis-reconstructed session") ginkgo.By("Verifying backend session IDs in Redis are the same hints pod B received") backendIDsAfterRestore, err := readRedisSessionBackendIDs(redisName, "thv:vmcp:e2e:", sessionID) @@ -322,7 +313,7 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() "pod B used them as hints and the IDs must be stable") ginkgo.By("Verifying both pods return the same tool count") - gomega.Expect(toolsB.Tools).To(gomega.HaveLen(len(toolsA.Tools)), + gomega.Expect(toolsB).To(gomega.HaveLen(len(toolsA)), "pod B must see same session state as pod A") }) }) @@ -419,25 +410,25 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() ginkgo.By("Getting the NodePort for the VirtualMCPServer") vmcpNodePort := GetVMCPNodePort(ctx, k8sClient, vmcpName, defaultNamespace, timeout, pollInterval) - ginkgo.By("Initializing an MCP session") - mcpClientA, err := CreateInitializedMCPClient(vmcpNodePort, "e2e-redis-restart-test", 30*time.Second) + // Era-pinned raw client: this spec asserts Legacy-session semantics, so + // it must pin 2025-11-25 per request (see legacy_session_helpers_test.go). + // The raw client is also what makes the "pod killed, not clean client + // disconnect" simulation below trivial: it holds no connection state and + // never sends the DELETE /mcp a real client's Close() would, which would + // terminate the session in Redis before the pod restart and defeat the + // purpose of this test. Simply not sending anything models the kill. + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - sessionID := mcpClientA.Client.GetSessionId() - gomega.Expect(sessionID).NotTo(gomega.BeEmpty(), "session ID must be assigned after Initialize") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + + ginkgo.By("Initializing a Legacy MCP session") + sessionID, err := legacySessionInit(rawClient, serverURL, "e2e-redis-restart-test", nil) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), "session ID must be assigned after Initialize") ginkgo.By("Verifying tools are available before pod restart") - toolsBefore, err := mcpClientA.Client.ListTools(mcpClientA.Ctx, mcp.ListToolsRequest{}) + toolsBefore, err := legacySessionListTools(rawClient, serverURL, sessionID, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(toolsBefore.Tools).NotTo(gomega.BeEmpty()) - - // Cancel context to stop in-flight requests without sending DELETE. - // This simulates the pod being killed, not a clean client disconnect. - // We intentionally skip Client.Close() here because Close() sends a - // DELETE /mcp request that would terminate the session in Redis before - // the pod is restarted — defeating the purpose of this test. - // The transport's background goroutine (started by Start()) selects on - // ctx.Done(), so Cancel() is sufficient to stop it without leaking. - mcpClientA.Cancel() + gomega.Expect(toolsBefore).NotTo(gomega.BeEmpty()) ginkgo.By("Getting the running pod name before restart") var pods []corev1.Pod @@ -489,27 +480,15 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() return checkHTTPHealthReady(vmcpNodePort) }, timeout, pollInterval).Should(gomega.Succeed()) - ginkgo.By("Creating a new client with the SAME session ID") - serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - newClient, err := mcpclient.NewStreamableHttpClient(serverURL, transport.WithSession(sessionID)) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer func() { _ = newClient.Close() }() - - startCtx, startCancel := context.WithTimeout(context.Background(), 30*time.Second) - defer startCancel() - gomega.Expect(newClient.Start(startCtx)).To(gomega.Succeed()) - - // Send 5 requests to give confidence the fix holds: without Redis-backed - // session reconstruction, each request would fail because the new pod's - // in-memory cache is cold. + // Send 5 requests with the SAME session ID to give confidence the fix + // holds: without Redis-backed session reconstruction, each request would + // fail because the new pod's in-memory cache is cold. ginkgo.By("Sending 5 requests to verify the session is recovered from Redis on the new pod") for i := range 5 { - listCtx, listCancel := context.WithTimeout(context.Background(), 30*time.Second) - toolsAfter, listErr := newClient.ListTools(listCtx, mcp.ListToolsRequest{}) - listCancel() + toolsAfter, listErr := legacySessionListTools(rawClient, serverURL, sessionID, nil) gomega.Expect(listErr).NotTo(gomega.HaveOccurred(), "Request %d/5 should succeed after pod restart — session must be recovered from Redis", i+1) - gomega.Expect(toolsAfter.Tools).To(gomega.HaveLen(len(toolsBefore.Tools)), + gomega.Expect(toolsAfter).To(gomega.HaveLen(len(toolsBefore)), "Request %d/5 should return the same tools as before restart", i+1) } }) @@ -644,51 +623,40 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() gomega.Expect(err).NotTo(gomega.HaveOccurred()) defer cleanupB() - ginkgo.By("Initializing a session on pod A") - var clientA *InitializedMCPClient + // Era-pinned raw client: this spec asserts Legacy-session semantics, so + // it must pin 2025-11-25 per request (see legacy_session_helpers_test.go). + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) + gomega.Expect(err).NotTo(gomega.HaveOccurred()) + serverURLA := fmt.Sprintf("http://localhost:%d/mcp", localPortA) + serverURLB := fmt.Sprintf("http://localhost:%d/mcp", localPortB) + + ginkgo.By("Initializing a Legacy session on pod A") + var sessionID string gomega.Eventually(func() error { - if clientA != nil { - clientA.Cancel() - clientA = nil - } var initErr error - clientA, initErr = CreateInitializedMCPClient(int32(localPortA), "e2e-redis-term-test", 30*time.Second) + sessionID, initErr = legacySessionInit(rawClient, serverURLA, "e2e-redis-term-test", nil) return initErr }, timeout, pollInterval).Should(gomega.Succeed(), "pod A should accept session initialization once backend routing is ready") - sessionID := clientA.Client.GetSessionId() - gomega.Expect(sessionID).NotTo(gomega.BeEmpty(), "session ID must be assigned after Initialize") ginkgo.By("Verifying the session is usable on pod A") - toolsA, err := clientA.Client.ListTools(clientA.Ctx, mcp.ListToolsRequest{}) + toolsA, err := legacySessionListTools(rawClient, serverURLA, sessionID, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(toolsA.Tools).NotTo(gomega.BeEmpty()) + gomega.Expect(toolsA).NotTo(gomega.BeEmpty()) ginkgo.By(fmt.Sprintf("Reconstructing the session on pod B (%s) via Redis", podB.Name)) - serverURLB := fmt.Sprintf("http://localhost:%d/mcp", localPortB) - clientB, err := mcpclient.NewStreamableHttpClient(serverURLB, transport.WithSession(sessionID)) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer func() { _ = clientB.Close() }() - - startCtx, startCancel := context.WithTimeout(context.Background(), 30*time.Second) - defer startCancel() - gomega.Expect(clientB.Start(startCtx)).To(gomega.Succeed()) - - listCtxB, listCancelB := context.WithTimeout(context.Background(), 30*time.Second) - toolsB, err := clientB.ListTools(listCtxB, mcp.ListToolsRequest{}) - listCancelB() + toolsB, err := legacySessionListTools(rawClient, serverURLB, sessionID, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(toolsB.Tools).NotTo(gomega.BeEmpty(), + gomega.Expect(toolsB).NotTo(gomega.BeEmpty(), "pod B should serve the session before termination") - // Terminate the session on pod A by sending DELETE /mcp directly. - // We do this via raw HTTP rather than clientA.Close() to avoid the - // context-cancellation ordering in InitializedMCPClient.Close(). + // Terminate the session on pod A by sending DELETE /mcp directly (the + // raw client only speaks POST JSON-RPC, and DELETE is a plain HTTP + // method, not a JSON-RPC request). ginkgo.By("Terminating the session on pod A via DELETE /mcp") - deleteURL := fmt.Sprintf("http://localhost:%d/mcp", localPortA) deleteCtx, deleteCancel := context.WithTimeout(context.Background(), 10*time.Second) defer deleteCancel() - req, err := http.NewRequestWithContext(deleteCtx, http.MethodDelete, deleteURL, nil) + req, err := http.NewRequestWithContext(deleteCtx, http.MethodDelete, serverURLA, nil) gomega.Expect(err).NotTo(gomega.HaveOccurred()) req.Header.Set("Mcp-Session-Id", sessionID) resp, err := http.DefaultClient.Do(req) @@ -696,22 +664,29 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() _ = resp.Body.Close() gomega.Expect(resp.StatusCode).To(gomega.BeElementOf(http.StatusOK, http.StatusNoContent), "DELETE /mcp should return 200 or 204") - clientA.Cancel() // Pod B's in-memory cache still holds the session, but the ValidatingCache's // checkSession callback will find the key absent in Redis (deleted by the // Terminate call above) and return ErrExpired, triggering lazy eviction. - // The next request from pod B should therefore fail with a session-not-found error. + // The next request from pod B should therefore fail with HTTP 404 + // (session not found — what mcpcompat surfaces as ErrSessionTerminated). ginkgo.By("Verifying pod B rejects subsequent requests for the terminated session") gomega.Eventually(func() error { - listCtx, listCancel := context.WithTimeout(context.Background(), 5*time.Second) - defer listCancel() - _, listErr := clientB.ListTools(listCtx, mcp.ListToolsRequest{}) - if listErr == nil { + listReq, buildErr := e2e.NewLegacyRequest("tools/list", nil) + if buildErr != nil { + return fmt.Errorf("build tools/list: %w", buildErr) + } + listReq.WithSessionID(sessionID).SetHeader(e2e.HeaderMCPProtocolVersion, coremcp.MCPVersionLegacy) + listResp, listErr := rawClient.Send(context.Background(), serverURLB, listReq) + if listErr != nil { + return fmt.Errorf("tools/list: %w", listErr) + } + if listResp.StatusCode == http.StatusOK { return fmt.Errorf("expected pod B to reject the terminated session, but request succeeded") } - if !errors.Is(listErr, transport.ErrSessionTerminated) { - return fmt.Errorf("expected ErrSessionTerminated (404), got: %w", listErr) + if listResp.StatusCode != http.StatusNotFound { + return fmt.Errorf("expected 404 (session not found) for the terminated session, got %d, body: %s", + listResp.StatusCode, listResp.Body) } return nil }, timeout, pollInterval).Should(gomega.Succeed(), @@ -1084,36 +1059,23 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() "Authorization": fmt.Sprintf("Bearer %s", asToken), } - ginkgo.By("Initializing MCP session on pod A with embedded AS token") - serverURLPodA := fmt.Sprintf("http://localhost:%d/mcp", localPortA) - clientA, err := mcpclient.NewStreamableHttpClient(serverURLPodA, - transport.WithHTTPHeaders(authHeader)) + // Era-pinned raw client: this spec asserts Legacy-session semantics, so + // it must pin 2025-11-25 per request (see legacy_session_helpers_test.go). + // The embedded AS JWT rides along as an extra header on every request. + rawClient, err := e2e.NewRawMCPClient(60 * time.Second) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer func() { _ = clientA.Close() }() - - initCtxA, initCancelA := context.WithTimeout(context.Background(), 60*time.Second) - defer initCancelA() - gomega.Expect(clientA.Start(initCtxA)).To(gomega.Succeed(), - "MCP client should start on pod A — check embedded AS JWT validity and OIDC config") - - initRequest := mcp.InitializeRequest{} - initRequest.Params.ProtocolVersion = mcp.LATEST_PROTOCOL_VERSION - initRequest.Params.ClientInfo = mcp.Implementation{ - Name: "e2e-upstreamInject-test", - Version: "1.0.0", - } - _, err = clientA.Initialize(initCtxA, initRequest) - gomega.Expect(err).NotTo(gomega.HaveOccurred(), "Initialize on pod A should succeed") + serverURLPodA := fmt.Sprintf("http://localhost:%d/mcp", localPortA) - sessionID := clientA.GetSessionId() - gomega.Expect(sessionID).NotTo(gomega.BeEmpty(), "session ID must be assigned after Initialize") + ginkgo.By("Initializing a Legacy MCP session on pod A with embedded AS token") + sessionID, err := legacySessionInit(rawClient, serverURLPodA, "e2e-upstreamInject-test", authHeader) + gomega.Expect(err).NotTo(gomega.HaveOccurred(), + "Initialize on pod A should succeed and assign a session ID — "+ + "check embedded AS JWT validity and OIDC config") ginkgo.By("Listing tools on pod A to verify the session is functional") - toolsCtxA, toolsCancelA := context.WithTimeout(context.Background(), 30*time.Second) - defer toolsCancelA() - toolsA, err := clientA.ListTools(toolsCtxA, mcp.ListToolsRequest{}) + toolsA, err := legacySessionListTools(rawClient, serverURLPodA, sessionID, authHeader) gomega.Expect(err).NotTo(gomega.HaveOccurred()) - gomega.Expect(toolsA.Tools).NotTo(gomega.BeEmpty(), + gomega.Expect(toolsA).NotTo(gomega.BeEmpty(), "pod A must return tools after Initialize") ginkgo.By("Verifying backend received a Bearer token on pod A's Initialize (upstreamInject fired)") @@ -1131,27 +1093,14 @@ var _ = ginkgo.Describe("VirtualMCPServer Redis-Backed Session Sharing", func() }, 30*time.Second, 2*time.Second).Should(gomega.BeNumerically(">=", 1), "backend must have received at least one Initialize call from pod A") - ginkgo.By(fmt.Sprintf("Connecting to pod B (%s) with the same session ID (triggers RestoreSession)", podB.Name)) + ginkgo.By(fmt.Sprintf("Listing tools on pod B (%s) with the same session ID (triggers RestoreSession)", podB.Name)) serverURLPodB := fmt.Sprintf("http://localhost:%d/mcp", localPortB) - clientB, err := mcpclient.NewStreamableHttpClient(serverURLPodB, - transport.WithSession(sessionID), - transport.WithHTTPHeaders(authHeader)) - gomega.Expect(err).NotTo(gomega.HaveOccurred()) - defer func() { _ = clientB.Close() }() - - startCtxB, startCancelB := context.WithTimeout(context.Background(), 60*time.Second) - defer startCancelB() - gomega.Expect(clientB.Start(startCtxB)).To(gomega.Succeed()) - - ginkgo.By("Listing tools on pod B using the restored session") - listCtxB, listCancelB := context.WithTimeout(context.Background(), 30*time.Second) - defer listCancelB() - toolsB, err := clientB.ListTools(listCtxB, mcp.ListToolsRequest{}) + toolsB, err := legacySessionListTools(rawClient, serverURLPodB, sessionID, authHeader) gomega.Expect(err).NotTo(gomega.HaveOccurred(), "pod B must not return an error — with the context fix, the identity "+ "(including UpstreamTokens) is propagated to RestoreSession so the "+ "backend Initialize is authenticated; without the fix this 401s") - gomega.Expect(toolsB.Tools).NotTo(gomega.BeEmpty(), + gomega.Expect(toolsB).NotTo(gomega.BeEmpty(), "pod B must return tools via the Redis-reconstructed session") ginkgo.By("Verifying backend received a second Initialize call from pod B's RestoreSession") diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_session_management_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_session_management_test.go index 38c219cdfa..229a65090f 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_session_management_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_session_management_test.go @@ -21,12 +21,10 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" - mcpclient "github.com/stacklok/toolhive-core/mcpcompat/client" - "github.com/stacklok/toolhive-core/mcpcompat/client/transport" - "github.com/stacklok/toolhive-core/mcpcompat/mcp" mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1" "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1/v1beta1test" vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/test/e2e" "github.com/stacklok/toolhive/test/e2e/images" ) @@ -222,61 +220,84 @@ var _ = ginkgo.Describe("VirtualMCPServer Session Management", func() { gomega.Expect(hmacSecretEnvVar.ValueFrom.SecretKeyRef.Key).To(gomega.Equal("hmac-secret")) }) + // These specs assert Legacy-session semantics, so they pin 2025-11-25 per + // request via the raw client (see legacy_session_helpers_test.go for why + // the mcpcompat client cannot be used for session specs). ginkgo.It("Should allow multiple clients to connect with independent sessions", func() { - ginkgo.By("Creating first client") - firstClient, err := CreateInitializedMCPClient(vmcpNodePort, "client-first", 30*time.Second) + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - defer firstClient.Close() - - sessionIDFirst := firstClient.Client.GetSessionId() - gomega.Expect(sessionIDFirst).NotTo(gomega.BeEmpty(), "first client should have a session ID") + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - ginkgo.By("Creating second client") - secondClient, err := CreateInitializedMCPClient(vmcpNodePort, "client-second", 30*time.Second) - gomega.Expect(err).ToNot(gomega.HaveOccurred()) - defer secondClient.Close() + ginkgo.By("Establishing the first Legacy session") + sessionIDFirst, err := legacySessionInit(rawClient, serverURL, "client-first", nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred(), "first client should get a session ID") - sessionIDSecond := secondClient.Client.GetSessionId() - gomega.Expect(sessionIDSecond).NotTo(gomega.BeEmpty(), "second client should have a session ID") + ginkgo.By("Establishing the second Legacy session") + sessionIDSecond, err := legacySessionInit(rawClient, serverURL, "client-second", nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred(), "second client should get a session ID") ginkgo.By("Verifying sessions are independent (different IDs)") gomega.Expect(sessionIDFirst).NotTo(gomega.Equal(sessionIDSecond)) - ginkgo.By("Both clients can list tools from the backend") - toolsFirst, err := firstClient.Client.ListTools(firstClient.Ctx, mcp.ListToolsRequest{}) + ginkgo.By("Both sessions can list tools from the backend") + toolsFirst, err := legacySessionListTools(rawClient, serverURL, sessionIDFirst, nil) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(toolsFirst.Tools).NotTo(gomega.BeEmpty()) + gomega.Expect(toolsFirst).NotTo(gomega.BeEmpty()) - toolsSecond, err := secondClient.Client.ListTools(secondClient.Ctx, mcp.ListToolsRequest{}) + toolsSecond, err := legacySessionListTools(rawClient, serverURL, sessionIDSecond, nil) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(toolsSecond.Tools).NotTo(gomega.BeEmpty()) + gomega.Expect(toolsSecond).NotTo(gomega.BeEmpty()) - ginkgo.By("Both clients see the same tool catalog") - gomega.Expect(toolsFirst.Tools).To(gomega.HaveLen(len(toolsSecond.Tools))) + ginkgo.By("Both sessions see the same tool catalog") + gomega.Expect(toolsFirst).To(gomega.HaveLen(len(toolsSecond))) }) ginkgo.It("Should allow a client to make multiple calls on the same session", func() { - client, err := CreateInitializedMCPClient(vmcpNodePort, "multi-call-client", 30*time.Second) + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - defer client.Close() + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) - sessionID := client.Client.GetSessionId() - gomega.Expect(sessionID).NotTo(gomega.BeEmpty()) + sessionID, err := legacySessionInit(rawClient, serverURL, "multi-call-client", nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + // Every request below pins the SAME session ID explicitly, so "the + // session stays stable across calls" is asserted by each call being + // served (a dead or rotated session would 404). ginkgo.By("Listing tools multiple times on the same session") for i := range 3 { - tools, err := client.Client.ListTools(client.Ctx, mcp.ListToolsRequest{}) + tools, err := legacySessionListTools(rawClient, serverURL, sessionID, nil) gomega.Expect(err).ToNot(gomega.HaveOccurred(), "call %d should succeed", i+1) - gomega.Expect(tools.Tools).NotTo(gomega.BeEmpty()) - // Session ID must remain stable across calls - gomega.Expect(client.Client.GetSessionId()).To(gomega.Equal(sessionID)) + gomega.Expect(tools).NotTo(gomega.BeEmpty()) } }) ginkgo.It("Should route tool calls through the session to the backend", func() { - // TestToolListingAndCall discovers the actual (possibly-prefixed) tool name via - // ListTools and calls it with alphanumeric-only input (yardstick requirement). - TestToolListingAndCall(vmcpNodePort, "tool-call-client", "echo", "sessiontest") + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + + sessionID, err := legacySessionInit(rawClient, serverURL, "tool-call-client", nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Discover the actual (possibly-prefixed) echo tool name via tools/list, + // then call it on the session with alphanumeric-only input (yardstick + // requirement). + tools, err := legacySessionListTools(rawClient, serverURL, sessionID, nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + var echoToolName string + for _, name := range tools { + if strings.Contains(name, "echo") { + echoToolName = name + break + } + } + gomega.Expect(echoToolName).NotTo(gomega.BeEmpty(), "should find an echo tool in the tool list") + + resp, err := legacySessionCallTool(rawClient, serverURL, sessionID, echoToolName, + map[string]any{"input": "sessiontest"}, nil) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(dualEraEchoErr(resp, "sessiontest", "")).To(gomega.Succeed(), + "the session-routed tools/call must echo the input back") }) }) @@ -395,21 +416,27 @@ var _ = ginkgo.Describe("VirtualMCPServer Session Management", func() { return tokenResp.AccessToken } - // newAuthHTTPClient wraps an HTTP client that adds Bearer token to every request. - newAuthHTTPClient := func(token string) *http.Client { - return &http.Client{ - Transport: &authRoundTripper{token: token, transport: http.DefaultTransport}, - Timeout: 30 * time.Second, - } + // bearerHeader builds the extra-header map that carries a JWT on every + // raw Legacy request. Session token binding is Legacy-session semantics, + // so these specs pin 2025-11-25 per request via the raw client (see + // legacy_session_helpers_test.go for why the mcpcompat client cannot be + // used for session specs). + bearerHeader := func(token string) map[string]string { + return map[string]string{"Authorization": "Bearer " + token} } - // connectWithToken initialises an MCP client authenticated with the given JWT. - connectWithToken := func(serverURL, token string) *mcpclient.Client { - httpClient := newAuthHTTPClient(token) - mc := InitializeMCPClientWithRetries(serverURL, 2*time.Minute, - transport.WithHTTPBasicClient(httpClient), - ) - return mc + // legacySessionInitWithRetries establishes a Legacy session, retrying + // while the OIDC middleware and backend routing warm up (the role + // InitializeMCPClientWithRetries played for the mcpcompat client). + legacySessionInitWithRetries := func(rawClient *e2e.RawMCPClient, serverURL, clientName, token string) string { + var sessionID string + gomega.Eventually(func() error { + var initErr error + sessionID, initErr = legacySessionInit(rawClient, serverURL, clientName, bearerHeader(token)) + return initErr + }, 2*time.Minute, pollInterval).Should(gomega.Succeed(), + "vMCP should accept an authenticated session initialization once ready") + return sessionID } ginkgo.BeforeAll(func() { @@ -503,13 +530,12 @@ var _ = ginkgo.Describe("VirtualMCPServer Session Management", func() { ginkgo.It("Client using another client's session ID with a different token is rejected", func() { serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) ginkgo.By("Alice establishes a session") aliceToken := getJWTForSubject("alice") - aliceClient := connectWithToken(serverURL, aliceToken) - defer aliceClient.Close() - - aliceSessionID := aliceClient.GetSessionId() + aliceSessionID := legacySessionInitWithRetries(rawClient, serverURL, "alice-client", aliceToken) gomega.Expect(aliceSessionID).NotTo(gomega.BeEmpty()) ginkgo.By("Bob gets a different JWT") @@ -565,56 +591,51 @@ var _ = ginkgo.Describe("VirtualMCPServer Session Management", func() { ginkgo.It("Each client gets their own independent session", func() { serverURL := fmt.Sprintf("http://localhost:%d/mcp", vmcpNodePort) + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) - ginkgo.By("Alice and Bob each connect with their own token") + ginkgo.By("Alice and Bob each establish a session with their own token") aliceToken := getJWTForSubject("alice") bobToken := getJWTForSubject("bob") - aliceClient := connectWithToken(serverURL, aliceToken) - defer aliceClient.Close() - - bobClient := connectWithToken(serverURL, bobToken) - defer bobClient.Close() + aliceSessionID := legacySessionInitWithRetries(rawClient, serverURL, "alice-client", aliceToken) + bobSessionID := legacySessionInitWithRetries(rawClient, serverURL, "bob-client", bobToken) ginkgo.By("Verifying they have distinct session IDs") - aliceSessionID := aliceClient.GetSessionId() - bobSessionID := bobClient.GetSessionId() gomega.Expect(aliceSessionID).NotTo(gomega.BeEmpty()) gomega.Expect(bobSessionID).NotTo(gomega.BeEmpty()) gomega.Expect(aliceSessionID).NotTo(gomega.Equal(bobSessionID)) - ginkgo.By("Both clients can independently list tools") - toolsA, err := aliceClient.ListTools(ctx, mcp.ListToolsRequest{}) + ginkgo.By("Both sessions can independently list tools") + toolsA, err := legacySessionListTools(rawClient, serverURL, aliceSessionID, bearerHeader(aliceToken)) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(toolsA.Tools).NotTo(gomega.BeEmpty()) + gomega.Expect(toolsA).NotTo(gomega.BeEmpty()) - toolsB, err := bobClient.ListTools(ctx, mcp.ListToolsRequest{}) + toolsB, err := legacySessionListTools(rawClient, serverURL, bobSessionID, bearerHeader(bobToken)) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(toolsB.Tools).NotTo(gomega.BeEmpty()) + gomega.Expect(toolsB).NotTo(gomega.BeEmpty()) - ginkgo.By("Both clients can independently call tools on their own sessions") + ginkgo.By("Both sessions can independently call tools") // Discover the real tool name (may be prefixed as backendName_echo). // Use alphanumeric-only input — yardstick rejects values with hyphens. var echoToolName string - for _, tool := range toolsA.Tools { - if strings.Contains(tool.Name, "echo") { - echoToolName = tool.Name + for _, name := range toolsA { + if strings.Contains(name, "echo") { + echoToolName = name break } } gomega.Expect(echoToolName).NotTo(gomega.BeEmpty(), "should find an echo tool in the tool list") - callReq := mcp.CallToolRequest{} - callReq.Params.Name = echoToolName - callReq.Params.Arguments = map[string]any{"input": "aliceindependentcall"} - aliceResult, err := aliceClient.CallTool(ctx, callReq) + aliceResp, err := legacySessionCallTool(rawClient, serverURL, aliceSessionID, echoToolName, + map[string]any{"input": "aliceindependentcall"}, bearerHeader(aliceToken)) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(aliceResult.IsError).To(gomega.BeFalse()) + gomega.Expect(dualEraEchoErr(aliceResp, "aliceindependentcall", "")).To(gomega.Succeed()) - callReq.Params.Arguments = map[string]any{"input": "bobindependentcall"} - bobResult, err := bobClient.CallTool(ctx, callReq) + bobResp, err := legacySessionCallTool(rawClient, serverURL, bobSessionID, echoToolName, + map[string]any{"input": "bobindependentcall"}, bearerHeader(bobToken)) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(bobResult.IsError).To(gomega.BeFalse()) + gomega.Expect(dualEraEchoErr(bobResp, "bobindependentcall", "")).To(gomega.Succeed()) }) }) From 9dffb9164b6f326b0d3613eafa3ab276c90045bd Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 28 Jul 2026 11:06:10 +0000 Subject: [PATCH 4/7] Preserve coded errors on the Modern dispatch path With Modern dispatch now unconditional, a rate-limited tools/call reached Modern clients as an opaque -32603 instead of RFC THV-0057's -32029 with data.retryAfterSeconds: writeModernDispatchError laundered every non-authz error into the generic internal code, dropping the code and data that the Legacy seam preserves in structuredContent (conversion.ErrorToToolResult). Clients lost the machine-readable retry metadata exactly on the path that is now the default. Add a CodedError branch mirroring the Legacy seam's posture and #6061's capability-refusal classification: the dispatcher owns the envelope, so it emits the real JSON-RPC error object. HTTP status stays 200 because -32029's natural 429 is in go-sdk's transient retry set and would be silently retried instead of surfaced. The rate-limiting operator spec asserted the Legacy structuredContent rendering through a client that now negotiates Modern; it asserts the Modern surface instead, pinning the full envelope with an era-pinned raw request per the #6051 convention. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n --- docs/arch/10-virtual-mcp-architecture.md | 10 ++- pkg/vmcp/server/modern_dispatch.go | 17 +++++ pkg/vmcp/server/modern_dispatch_test.go | 72 +++++++++++++++++++ pkg/vmcp/server/modern_envelope.go | 39 ++++++++++ .../virtualmcp_rate_limiting_test.go | 47 +++++++++--- 5 files changed, 173 insertions(+), 12 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 9a7a9de392..1f3ac0eefb 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -434,7 +434,15 @@ the feature promises — not merely that the feature is session-flavored. Redis-backed session sharing, for example, does **not** gate Modern: Legacy clients keep their shared, reconstructible sessions while Modern clients are sessionless by design and store nothing, a coexistence asserted end-to-end by -`test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go`. +`test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go`. Rate +limiting does not gate Modern either: the limiter is a core decorator +(`pkg/vmcp/ratelimit`), so both eras meter the same `CallTool` seam, and the +Modern dispatcher preserves the limiter's coded error on the wire — a real +JSON-RPC error object, `-32029` with `data.retryAfterSeconds` +(`writeModernCodedError`, at HTTP 200 because 429 sits in go-sdk's transient +retry set) — where the Legacy SDK seam can only smuggle the same code and data +into an `IsError` tool result's `structuredContent` +(`conversion.ErrorToToolResult`). Wire behavior when the gate is closed: diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index 72860edae4..3b72780515 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -615,6 +615,18 @@ func writeModernMissingCapability(w http.ResponseWriter, id any, capName string) // therefore tested FIRST, before falling through to the generic internal // error. // +// A domain error that carries its own stable JSON-RPC code and data +// (mcpparser.CodedError — e.g. the rate limiter's -32029 with +// data.retryAfterSeconds) is written with that code rather than laundered +// into -32603. This is the Modern counterpart of the SDK path's +// conversion.ErrorToToolResult, whose CodedError branch preserves the same +// code/data in an IsError tool result's structuredContent because the SDK +// tool-handler seam cannot emit a custom JSON-RPC error object. This +// dispatcher owns the envelope, so it emits the real thing (mirroring how +// #6061 classified mid-call capability refusals as -32021 instead of +// -32603). Authorization denials are still tested first: a coded error can +// never mask a denial's 403. +// // The -32603 message reuses err.Error() verbatim. This matches the SDK path's // existing posture rather than inventing a new one: conversion.ErrorToToolResult's // generic branch, and the resources/read/prompts/get Serve handlers @@ -626,5 +638,10 @@ func writeModernDispatchError(w http.ResponseWriter, id any, denyMsg string, err writeModernDenied(w, id, denyMsg) return } + var coded mcpparser.CodedError + if errors.As(err, &coded) { + writeModernCodedError(w, id, err, coded) + return + } writeModernError(w, id, jsonRPCCodeInternalError, err.Error()) } diff --git a/pkg/vmcp/server/modern_dispatch_test.go b/pkg/vmcp/server/modern_dispatch_test.go index fb0d0a0df9..77895c8fcf 100644 --- a/pkg/vmcp/server/modern_dispatch_test.go +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -12,6 +12,7 @@ import ( "net/http" "net/http/httptest" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -19,6 +20,7 @@ import ( "github.com/stacklok/toolhive/pkg/audit" "github.com/stacklok/toolhive/pkg/auth" mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/ratelimit" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/pkg/vmcp/core" ) @@ -720,6 +722,76 @@ func TestDispatchModern_AuthzGate(t *testing.T) { } } +// TestDispatchModern_CodedErrorPreservesCodeAndData pins the CodedError branch +// of writeModernDispatchError: a domain error carrying its own stable JSON-RPC +// code and data (here the rate limiter's -32029 with data.retryAfterSeconds) +// must reach the Modern client with that code and data intact, not be +// laundered into an opaque -32603 — parity with the SDK path, where +// conversion.ErrorToToolResult preserves the same code/data in +// structuredContent. HTTP status stays 200: -32029's natural 429 is in +// go-sdk's transient retry set and would be silently retried instead of +// surfaced (see writeModernCodedError). +func TestDispatchModern_CodedErrorPreservesCodeAndData(t *testing.T) { + t.Parallel() + + limited := &ratelimit.RateLimitedError{RetryAfter: 5 * time.Second} + + cases := []struct { + method string + resourceID string + }{ + {method: "tools/call", resourceID: "echo"}, + {method: "resources/read", resourceID: "file://doc"}, + {method: "prompts/get", resourceID: "review"}, + } + + for _, c := range cases { + t.Run(c.method+"/coded error keeps its code and data", func(t *testing.T) { + t.Parallel() + parsed := &mcpparser.ParsedMCPRequest{Method: c.method, ResourceID: c.resourceID, IsRequest: true, ID: "1"} + fc := &modernFakeCore{callToolErr: limited, readResourceErr: limited, getPromptErr: limited} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Equal(t, float64(ratelimit.CodeRateLimited), errObj["code"]) + assert.Equal(t, ratelimit.MessageRateLimited, errObj["message"]) + data, ok := errObj["data"].(map[string]any) + require.True(t, ok, "coded error data must be preserved, got %v", errObj) + assert.Equal(t, float64(5), data["retryAfterSeconds"]) + }) + } + + t.Run("wrapping a coded error keeps the code and the wrapped message", func(t *testing.T) { + t.Parallel() + parsed := &mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "echo", IsRequest: true, ID: "1"} + fc := &modernFakeCore{callToolErr: fmt.Errorf("calling backend: %w", limited)} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Equal(t, float64(ratelimit.CodeRateLimited), errObj["code"], + "errors.As must unwrap to the coded error") + assert.Equal(t, "calling backend: "+ratelimit.MessageRateLimited, errObj["message"], + "message must keep the caller's wrap context") + }) + + t.Run("an authz denial wrapping never yields the coded branch", func(t *testing.T) { + t.Parallel() + // Belt-and-suspenders for the documented ordering: a denial keeps its + // 403 even if some future error value were both a denial and coded. + parsed := &mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "echo", IsRequest: true, ID: "1"} + fc := &modernFakeCore{callToolErr: fmt.Errorf("%w: no", vmcp.ErrAuthorizationFailed)} + + rec, _ := dispatchModernTest(t.Context(), t, fc, true, parsed) + assert.Equal(t, http.StatusForbidden, rec.Code) + }) +} + // TestDispatchModern_ArgumentsShape asserts the raw-params shape guard on // tools/call and prompts/get: the parser (handleNamedResourceMethod) // silently coerces a present-but-non-object "arguments" value to nil -- diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 7fae168b96..f51f157577 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -584,6 +584,45 @@ func writeModernError(w http.ResponseWriter, id any, code int, msg string) { writeModernEnvelope(w, status, envelope) } +// writeModernCodedError writes a JSON-RPC error envelope for a domain error +// carrying its own stable code and data (mcpparser.CodedError), preserving +// both on the wire: {"code":coded.Code(),"message":err.Error(),"data":...}. +// message uses err.Error() rather than a canned string so callers can wrap a +// coded error with context via %w without losing it (mirrors +// conversion.CodedErrorResult, the Legacy seam's rendering of the same +// errors). +// +// Status is HTTP 200 deliberately, for the same client-survival reason as +// writeModernMissingCapability's documented deviation (modern_dispatch.go): +// ToolHive's coded errors live in the implementation-defined -3202x space, so +// no spec MUSTs a 4xx for them, and the natural mapping for the rate +// limiter's -32029 (HTTP 429 + Retry-After, what pkg/ratelimit's HTTP +// middleware writes) is in go-sdk v1.7.0-pre.3's TRANSIENT retry set — a +// go-sdk client would silently retry the POST instead of surfacing the error +// with its retryAfterSeconds to the caller. The request was accepted and +// processed; the failure is an application-level JSON-RPC error riding the +// transport. +// +// id follows writeModernError: absent (via transportsession.HasJSONRPCID) is +// encoded by omitting the "id" key, never as null. +func writeModernCodedError(w http.ResponseWriter, id any, err error, coded mcpparser.CodedError) { + errObj := map[string]any{ + "code": coded.Code(), + "message": err.Error(), + } + if data := coded.Data(); data != nil { + errObj["data"] = data + } + envelope := map[string]any{ + "jsonrpc": "2.0", + "error": errObj, + } + if transportsession.HasJSONRPCID(id) { + envelope["id"] = id + } + writeModernEnvelope(w, http.StatusOK, envelope) +} + // writeModernDenied writes a JSON-RPC error envelope at HTTP 403 with // mcpparser.JSONRPCCodeDenied, mirroring the Legacy call gate // (call_gate.go) and pkg/authz.handleUnauthorized: the 403 status is what diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_rate_limiting_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_rate_limiting_test.go index 7a7c02134c..6f28221f2e 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_rate_limiting_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_rate_limiting_test.go @@ -27,6 +27,7 @@ import ( "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1/v1beta1test" "github.com/stacklok/toolhive/pkg/ratelimit" vmcpconfig "github.com/stacklok/toolhive/pkg/vmcp/config" + "github.com/stacklok/toolhive/test/e2e" "github.com/stacklok/toolhive/test/e2e/images" ) @@ -214,18 +215,42 @@ var _ = ginkgo.Describe("VirtualMCPServer Rate Limiting", ginkgo.Ordered, func() _, err = mcpClient.CallTool(ctx, req) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - result, err := mcpClient.CallTool(ctx, req) + // The mcpcompat client is go-sdk-backed and Modern-first: against a + // Modern-serving vMCP it negotiates 2026-07-28, where a rate-limited + // call surfaces as a real JSON-RPC error object carrying the domain + // code (writeModernCodedError) — not as the Legacy SDK seam's IsError + // tool result with the code smuggled into structuredContent + // (conversion.CodedErrorResult). + _, err = mcpClient.CallTool(ctx, req) + gomega.Expect(err).To(gomega.HaveOccurred(), "second tools/call must be rejected by the per-user limit") + gomega.Expect(err.Error()).To(gomega.ContainSubstring(ratelimit.MessageRateLimited)) + + // Pin the full Modern envelope with an era-pinned raw request (the + // #6051 convention): RFC THV-0057's -32029 with data.retryAfterSeconds + // must survive the Modern dispatch path, which the go-sdk client above + // can only surface as an error string. + ginkgo.By("Verifying the Modern rate-limit error envelope") + rawClient, err := e2e.NewRawMCPClient(30 * time.Second) gomega.Expect(err).ToNot(gomega.HaveOccurred()) - gomega.Expect(result.IsError).To(gomega.BeTrue()) - - structured, ok := result.StructuredContent.(map[string]any) - gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(structured["code"]).To(gomega.BeNumerically("==", ratelimit.CodeRateLimited)) - gomega.Expect(structured["message"]).To(gomega.Equal(ratelimit.MessageRateLimited)) - - data, ok := structured["data"].(map[string]any) - gomega.Expect(ok).To(gomega.BeTrue()) - gomega.Expect(data["retryAfterSeconds"]).To(gomega.BeNumerically(">", 0)) + rawReq, err := e2e.NewModernRequest("tools/call", map[string]any{ + "name": toolName, + "arguments": map[string]any{"input": "ratelimittest"}, + }) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + rawReq.SetHeader("Authorization", "Bearer "+token) + rawResp, err := rawClient.Send(ctx, fmt.Sprintf("http://localhost:%d/mcp", vmcpLocalPort), rawReq) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + gomega.Expect(rawResp.StatusCode).To(gomega.Equal(http.StatusOK), + "a rate-limited Modern call rides HTTP 200 (429 is in go-sdk's transient retry set), body: %s", rawResp.Body) + gomega.Expect(rawResp.Error).ToNot(gomega.BeNil(), "body: %s", rawResp.Body) + gomega.Expect(rawResp.Error.Code).To(gomega.Equal(ratelimit.CodeRateLimited)) + gomega.Expect(rawResp.Error.Message).To(gomega.Equal(ratelimit.MessageRateLimited)) + var retryData struct { + RetryAfterSeconds float64 `json:"retryAfterSeconds"` + } + gomega.Expect(json.Unmarshal(rawResp.Error.Data, &retryData)).To(gomega.Succeed(), + "error data: %s", rawResp.Error.Data) + gomega.Expect(retryData.RetryAfterSeconds).To(gomega.BeNumerically(">", 0)) ginkgo.By("Scraping non-zero rate limit metrics") gomega.Eventually(func() error { From bcd7941c4202abfad354da06a2868f0b0fa74f72 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 28 Jul 2026 11:06:21 +0000 Subject: [PATCH 5/7] Parse SSE-framed POST responses in the raw MCP client The era-pinned session specs rewritten off the mcpcompat client (#6051) fail against any pod serving a rehydrated cross-pod session: mcpcompat's rehydration path answers POSTs as text/event-stream regardless of the request's Accept header, because go-sdk v1.7.0-pre.3 does not export the transport-level JSONResponse knob (StreamableServerTransport.jsonResponse) that its top-level handler options set. The old mcpcompat test client tolerated SSE; the raw client left the envelope unparsed, so pod-B assertions failed with an empty result on an HTTP 200. Extract the JSON-RPC response event from an SSE body into the RawResponse envelope, skipping interleaved notifications, mirroring mcpcompat's own rehydration test helper (readFirstResult). Body keeps the raw stream for callers that inspect it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Y5WXPQr5Da9Cox2nZWhg6n --- test/e2e/mcp_raw_client.go | 69 +++++++++++++++++++++--- test/e2e/mcp_raw_client_test.go | 94 +++++++++++++++++++++++++++++++++ 2 files changed, 157 insertions(+), 6 deletions(-) diff --git a/test/e2e/mcp_raw_client.go b/test/e2e/mcp_raw_client.go index 886599fde4..352d9f15eb 100644 --- a/test/e2e/mcp_raw_client.go +++ b/test/e2e/mcp_raw_client.go @@ -12,6 +12,7 @@ import ( "io" "maps" "net/http" + "strings" "sync/atomic" "time" ) @@ -316,11 +317,18 @@ func NewRawMCPClient(timeout time.Duration) (*RawMCPClient, error) { } // Send marshals req and POSTs it to url. No Accept header is set by default: -// the ToolHive proxy returns a plain JSON body without it (which this client's -// populateEnvelope parses) and switches to an unparsable SSE response body -// whenever Accept lists text/event-stream. A request bound for a REAL -// streamable-HTTP MCP backend -- which rejects a POST lacking that Accept with -// HTTP 400 -- should opt in via req.WithStreamableAccept(). +// the ToolHive proxy returns a plain JSON body without it, and switches to an +// SSE response body whenever Accept lists text/event-stream. A request bound +// for a REAL streamable-HTTP MCP backend -- which rejects a POST lacking that +// Accept with HTTP 400 -- should opt in via req.WithStreamableAccept(). +// +// Either way the JSON-RPC envelope is parsed into the RawResponse: a +// Content-Type: text/event-stream body has its response event extracted first +// (see sseResponsePayload). Some servers answer a POST as SSE regardless of +// the request's Accept header -- mcpcompat's cross-replica rehydration path +// does, because go-sdk v1.7.0-pre.3 does not export the transport-level +// JSONResponse knob (StreamableServerTransport.jsonResponse) that its +// top-level handler options set -- so this parsing cannot be opt-in. func (c *RawMCPClient) Send(ctx context.Context, url string, req *RawRequest) (*RawResponse, error) { body, err := req.marshal() if err != nil { @@ -361,7 +369,11 @@ func (c *RawMCPClient) SendRaw(ctx context.Context, url string, headers map[stri Headers: resp.Header.Clone(), Body: respBody, } - populateEnvelope(respBody, result) + envelope := respBody + if strings.HasPrefix(resp.Header.Get("Content-Type"), "text/event-stream") { + envelope = sseResponsePayload(respBody) + } + populateEnvelope(envelope, result) return result, nil } @@ -404,6 +416,51 @@ func populateEnvelope(body []byte, out *RawResponse) { } } +// sseResponsePayload extracts the JSON-RPC response object from an SSE-framed +// (text/event-stream) POST response body. A streamable-HTTP server may +// interleave notifications on the same stream before the response, so the +// payload returned is the LAST event whose data is a JSON object carrying a +// top-level "result" or "error" key -- the one JSON-RPC response terminating +// the stream -- not simply the last event. Multi-line data fields are joined +// with newlines per the SSE spec. Returns nil when no event carries a +// response, leaving the envelope unpopulated (Body still holds the raw +// stream for the caller to inspect). +func sseResponsePayload(body []byte) []byte { + var response []byte + var data [][]byte + flush := func() { + if len(data) == 0 { + return + } + payload := bytes.Join(data, []byte("\n")) + data = nil + var obj map[string]json.RawMessage + if err := json.Unmarshal(payload, &obj); err != nil { + return + } + if _, ok := obj["result"]; ok { + response = payload + return + } + if _, ok := obj["error"]; ok { + response = payload + } + } + for _, line := range bytes.Split(body, []byte("\n")) { + line = bytes.TrimSuffix(line, []byte("\r")) + if len(line) == 0 { // blank line: event boundary + flush() + continue + } + if value, ok := bytes.CutPrefix(line, []byte("data:")); ok { + data = append(data, bytes.TrimPrefix(value, []byte(" "))) + } + // Other fields (event:, id:, retry:, comments) don't carry payload. + } + flush() // stream may end without a trailing blank line + return response +} + // decodeID decodes a raw JSON-RPC id, returning a json.Number for numeric // ids (so large int64 values round-trip exactly, unlike the float64 that // encoding/json's default decodes numbers into) or a string for string ids. diff --git a/test/e2e/mcp_raw_client_test.go b/test/e2e/mcp_raw_client_test.go index 9f33682f6c..57594ddf3e 100644 --- a/test/e2e/mcp_raw_client_test.go +++ b/test/e2e/mcp_raw_client_test.go @@ -345,6 +345,100 @@ func TestRawClientSend(t *testing.T) { }) } +// newSSEServer serves body verbatim as a text/event-stream POST response — +// the shape mcpcompat's cross-replica rehydration path produces regardless of +// the request's Accept header (go-sdk v1.7.0-pre.3 does not export the +// transport-level JSONResponse knob its top-level handler options set). +func newSSEServer(t *testing.T, body string) *httptest.Server { + t.Helper() + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/event-stream") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(body)) + })) + t.Cleanup(server.Close) + return server +} + +// TestRawClientSSEResponse pins that an SSE-framed POST response body has its +// JSON-RPC response event parsed into the envelope, so era-pinned raw-client +// specs work against a server that answers SSE unasked (the rehydrated +// cross-pod session path) exactly as they do against a plain-JSON one. +func TestRawClientSSEResponse(t *testing.T) { + t.Parallel() + + client, err := NewRawMCPClient(5 * time.Second) + require.NoError(t, err) + + t.Run("parses the response event, skipping preceding notifications", func(t *testing.T) { + t.Parallel() + server := newSSEServer(t, "event: message\n"+ + `data: {"jsonrpc":"2.0","method":"notifications/message","params":{}}`+"\n\n"+ + "event: message\n"+ + `data: {"jsonrpc":"2.0","id":7,"result":{"tools":[{"name":"echo"}]}}`+"\n\n") + + req, err := NewLegacyRequest("tools/list", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Equal(t, "2.0", resp.JSONRPC) + require.Equal(t, json.Number("7"), resp.ID) + require.JSONEq(t, `{"tools":[{"name":"echo"}]}`, string(resp.Result)) + require.Nil(t, resp.Error) + }) + + t.Run("parses an error response event, including multi-line data", func(t *testing.T) { + t.Parallel() + // The SSE spec joins consecutive data: lines with \n; a JSON payload + // split across them must still parse. + server := newSSEServer(t, "event: message\n"+ + `data: {"jsonrpc":"2.0","id":3,`+"\n"+ + `data: "error":{"code":-32029,"message":"Rate limit exceeded"}}`+"\n\n") + + req, err := NewLegacyRequest("tools/call", map[string]any{"name": "echo"}) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.NotNil(t, resp.Error) + require.EqualValues(t, -32029, resp.Error.Code) + require.Equal(t, "Rate limit exceeded", resp.Error.Message) + }) + + t.Run("a stream with no response event leaves the envelope empty, Body raw", func(t *testing.T) { + t.Parallel() + body := "event: message\n" + + `data: {"jsonrpc":"2.0","method":"notifications/message","params":{}}` + "\n\n" + server := newSSEServer(t, body) + + req, err := NewLegacyRequest("tools/list", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Empty(t, resp.JSONRPC) + require.Nil(t, resp.Result) + require.Nil(t, resp.Error) + require.Equal(t, body, string(resp.Body), "Body must keep the raw stream for the caller") + }) + + t.Run("a stream without a trailing blank line still yields the response", func(t *testing.T) { + t.Parallel() + server := newSSEServer(t, "event: message\n"+ + `data: {"jsonrpc":"2.0","id":1,"result":{}}`) + + req, err := NewLegacyRequest("ping", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Equal(t, json.Number("1"), resp.ID) + require.JSONEq(t, `{}`, string(resp.Result)) + }) +} + func TestNewRawMCPClientRejectsNonPositiveTimeout(t *testing.T) { t.Parallel() _, err := NewRawMCPClient(0) From 1cb7c409a2c661b61aaa3c163a719eeaf6779432 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 28 Jul 2026 13:08:45 +0000 Subject: [PATCH 6/7] Address capability-gate review feedback Review on #6033 found the gate fails open for a direct-Serve caller that configures an optimizer without FactoryConfig.AdvertiseFromCore: sessionmanager only surfaces OptimizerFactory() when the flag is set, while the (discarded) decorator path runs when it is not -- so the optimizer indexed the FTS5 store, served nobody, and reported no blocker. sessionmanager.New now rejects that configuration at startup, making a non-nil OptimizerFactory() a faithful "optimizer enabled" signal; the unreachable decorator branch stays until #6103 deletes it. The rest is comment, wording, and consistency corrections from the same review: the wrong 429/go-sdk-retry rationale (go-sdk rejects a non-200 before decoding the body, so a 4xx would discard retryAfterSeconds unread), the -32029 reserved-band note (#6101), the gate contract's loud-refusals bullet (elicitation workflow steps fail Modern clients with an explicit -32021; distinct from #6059) and per-entry issue-ref requirement (#6089), denial-first ordering in conversion.ErrorToToolResult to match the Modern writer, len(data)>0 across the coded-error renderers, HasJSONRPCID gating in writeModernMissingCapability, a case-insensitive SSE Content-Type match plus CRLF/no-space/batch parser tests, the regressed Over1000Tools comment (Legacy-only, #5911), stale "unconditional dispatch" and "cannot parse SSE" comments (#6104), the gate verdict logged at WARN, and the optimizer's Legacy pin documented on the config field, the CRD reference, and the tier table (#6089). Co-Authored-By: Claude Opus 4.8 --- docs/arch/10-virtual-mcp-architecture.md | 44 ++++++++++++---- docs/arch/vmcp-local.md | 2 + docs/operator/crd-api.md | 2 +- pkg/ratelimit/errors.go | 6 ++- pkg/vmcp/config/config.go | 4 ++ pkg/vmcp/conversion/errors.go | 12 +++-- pkg/vmcp/server/classification_test.go | 2 +- ...forwarding_realbackend_integration_test.go | 7 ++- pkg/vmcp/server/modern_dispatch.go | 18 +++++-- pkg/vmcp/server/modern_dispatch_test.go | 6 +-- pkg/vmcp/server/modern_envelope.go | 33 ++++++++---- pkg/vmcp/server/modern_gate.go | 30 +++++++++-- pkg/vmcp/server/modern_pagination.go | 21 ++++---- pkg/vmcp/server/serve.go | 39 ++++++-------- pkg/vmcp/server/serve_lifecycle_test.go | 5 +- pkg/vmcp/server/sessionmanager/factory.go | 29 ++++++----- .../sessionmanager/optimizer_gate_test.go | 35 +++++++------ .../server/sessionmanager/session_manager.go | 46 +++++++++------- test/e2e/mcp_raw_client.go | 9 ++-- test/e2e/mcp_raw_client_test.go | 52 ++++++++++++++++++- .../virtualmcp_dual_era_redis_test.go | 6 +-- test/e2e/vmcp_dual_era_helpers_test.go | 3 +- test/e2e/vmcp_dual_era_test.go | 12 +++-- 23 files changed, 283 insertions(+), 140 deletions(-) diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 1f3ac0eefb..d66b9c6541 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -439,21 +439,43 @@ limiting does not gate Modern either: the limiter is a core decorator (`pkg/vmcp/ratelimit`), so both eras meter the same `CallTool` seam, and the Modern dispatcher preserves the limiter's coded error on the wire — a real JSON-RPC error object, `-32029` with `data.retryAfterSeconds` -(`writeModernCodedError`, at HTTP 200 because 429 sits in go-sdk's transient -retry set) — where the Legacy SDK seam can only smuggle the same code and data -into an `IsError` tool result's `structuredContent` -(`conversion.ErrorToToolResult`). +(`writeModernCodedError`, at HTTP 200 because go-sdk rejects a non-200 +response before decoding the body, so on a 429 the error object — and its +`retryAfterSeconds` — would be discarded unread) — where the Legacy SDK seam +can only smuggle the same code and data into an `IsError` tool result's +`structuredContent` (`conversion.ErrorToToolResult`). Note that `-32029` sits +inside the draft spec's reserved band (`-32020`..`-32099`, reserved for +spec-defined codes); reallocating it is tracked in #6101. + +"Does not gate Modern" is not "costs the same", though. The limiter wraps only +the `CallTool` seam, so the list verbs and `server/discover` are unmetered on +both eras — but Legacy aggregates once per session registration, while Modern +re-runs the full backend fan-out on every request with no cache, by design +(`core_vmcp.go`'s `aggregatedView`). A Modern client can therefore loop +unmetered, uncached fan-outs — reachable unauthenticated when incoming auth is +anonymous. Rate-limiting the list/discover verbs, or a short-TTL per-identity +capability cache, is deferred until profiling shows the per-request fan-out +cost matters (#5761 — the same deferral recorded in `dispatchModernDiscover`'s +doc comment). Wire behavior when the gate is closed: - **`server/discover` falls through to the SDK** instead of dispatching. go-sdk's - stateful transport filters 2026-07-28 out of its `supportedVersions` - (`SupportsProtocolVersion` requires `Stateless`), so the probe answer - advertises Legacy only. This matters because go-sdk v1.7+ clients are - Modern-first: `Connect` probes `server/discover` **before** `initialize` and - upgrades to whatever the server advertises. The fall-through answer is what - lands them on the Legacy handshake — where sessions, and every gated feature, - work. + `filterSupportedVersions` keeps every version the transport's + `SupportsProtocolVersion` accepts, and the stateful transport excludes only + >= 2026-07-28 (those require `Stateless`), so the probe answer is the + transport-filtered list — everything except Modern: `[2025-11-25, + 2025-06-18, 2025-03-26, 2024-11-05]`. This matters because go-sdk v1.7+ + clients are Modern-first: `Connect` probes `server/discover` **before** + `initialize` and upgrades to whatever the server advertises. The + fall-through answer is what lands them on the Legacy handshake — where + sessions, and every gated feature, work. That answer over-advertises, + though: the `-32022` refusal below lists only 2025-11-25, and the refusal is + the accurate one — `mcpcompat`'s `handleInitialize` always responds with + `LATEST_PROTOCOL_VERSION` regardless of what the client requests, so the + three older revisions in the discover answer are not actually servable. The + mismatch is the SDK's discover answer to narrow, not the `-32022` list to + widen. - **Every other well-formed Modern request** is refused with a conformant 400 + `-32022 UnsupportedProtocolVersionError` whose data lists the Legacy version, the shape a client negotiates down from. It is answered in diff --git a/docs/arch/vmcp-local.md b/docs/arch/vmcp-local.md index ca5568511b..8cc5b19b5c 100644 --- a/docs/arch/vmcp-local.md +++ b/docs/arch/vmcp-local.md @@ -103,6 +103,8 @@ thv vmcp serve --config vmcp.yaml | 2 | `--optimizer-embedding` | FTS5 + TEI semantic | Managed TEI container | `find_tool`, `call_tool` only | | 3 | `optimizer.embeddingService` in config YAML | FTS5 + external embedding service | User-managed | `find_tool`, `call_tool` only | +Any tier >= 1 keeps clients on MCP 2025-11-25 (Legacy): the `find_tool`/`call_tool` meta-tools are session-scoped, so Modern-capable (2026-07-28) clients are negotiated down to Legacy by the capability gate (`pkg/vmcp/server/modern_gate.go`; Modern parity is tracked in #6089). + Tier 2 (`--optimizer-embedding`) implies `--optimizer`. The TEI container is started automatically and stopped on server shutdown. **Implementation**: `pkg/vmcp/optimizer/optimizer.go`, `pkg/vmcp/cli/embedding_manager.go` diff --git a/docs/operator/crd-api.md b/docs/operator/crd-api.md index 4a87c88f16..9215471187 100644 --- a/docs/operator/crd-api.md +++ b/docs/operator/crd-api.md @@ -413,7 +413,7 @@ _Appears in:_ | `metadata` _object (keys:string, values:string)_ | Refer to Kubernetes API documentation for fields of `metadata`. | | | | `telemetry` _[pkg.telemetry.Config](#pkgtelemetryconfig)_ | Telemetry configures OpenTelemetry-based observability for the Virtual MCP server
including distributed tracing, OTLP metrics export, and Prometheus metrics endpoint.
Deprecated (Kubernetes operator only): When deploying via the operator, use
VirtualMCPServer.spec.telemetryConfigRef to reference a shared MCPTelemetryConfig
resource instead. This field remains valid for standalone (non-operator) deployments. | | Optional: \{\}
| | `audit` _[pkg.audit.Config](#pkgauditconfig)_ | Audit configures audit logging for the Virtual MCP server.
When present, audit logs include MCP protocol operations.
See audit.Config for available configuration options. | | Optional: \{\}
| -| `optimizer` _[vmcp.config.OptimizerConfig](#vmcpconfigoptimizerconfig)_ | Optimizer configures the MCP optimizer for context optimization on large toolsets.
When enabled, vMCP exposes only find_tool and call_tool operations to clients
instead of all backend tools directly. This reduces token usage by allowing
LLMs to discover relevant tools on demand rather than receiving all tool definitions. | | Optional: \{\}
| +| `optimizer` _[vmcp.config.OptimizerConfig](#vmcpconfigoptimizerconfig)_ | Optimizer configures the MCP optimizer for context optimization on large toolsets.
When enabled, vMCP exposes only find_tool and call_tool operations to clients
instead of all backend tools directly. This reduces token usage by allowing
LLMs to discover relevant tools on demand rather than receiving all tool definitions.
Enabling the optimizer currently pins this instance to MCP 2025-11-25: the
find_tool/call_tool meta-tools are session-scoped, so Modern-capable (2026-07-28)
clients are negotiated down to the Legacy revision (see
pkg/vmcp/server/modern_gate.go; Modern parity is tracked in stacklok/toolhive#6089). | | Optional: \{\}
| | `codeMode` _[vmcp.config.CodeModeConfig](#vmcpconfigcodemodeconfig)_ | CodeMode configures vMCP code mode: server-side execution of Starlark scripts that
orchestrate multiple backend tool calls in a single request via the execute_tool_script
virtual tool. When enabled, execute_tool_script is advertised alongside the backend
tools; a script's inner tool calls are authorized individually, so a script can only
reach tools the caller is already permitted to use. Disabled by default. | | Optional: \{\}
| | `sessionStorage` _[vmcp.config.SessionStorageConfig](#vmcpconfigsessionstorageconfig)_ | SessionStorage configures session storage for stateful horizontal scaling.
When provider is "redis", the operator injects Redis connection parameters
(address, db, keyPrefix) here. The Redis password is provided separately via
the THV_SESSION_REDIS_PASSWORD environment variable. | | Optional: \{\}
| | `rateLimiting` _[ratelimit.types.RateLimitConfig](#ratelimittypesratelimitconfig)_ | RateLimiting defines rate limiting configuration for the Virtual MCP server.
Requires Redis session storage to be configured for distributed rate limiting. | | Optional: \{\}
| diff --git a/pkg/ratelimit/errors.go b/pkg/ratelimit/errors.go index 1f00743c67..b2d346f787 100644 --- a/pkg/ratelimit/errors.go +++ b/pkg/ratelimit/errors.go @@ -12,7 +12,11 @@ import ( const ( // CodeRateLimited is the JSON-RPC error code for rate-limited requests. - // Per RFC THV-0057: implementation-defined code in the -32000 to -32099 range. + // It was chosen (RFC THV-0057) when -32000..-32099 was uniformly + // implementation-defined, but the draft MCP spec now reserves + // -32020..-32099 exclusively for spec-defined codes, which this is not. + // The value is retained for wire compatibility; reallocation outside + // -32768..-32000 is tracked in #6101. CodeRateLimited int64 = -32029 // MessageRateLimited is the error message for rate-limited requests. diff --git a/pkg/vmcp/config/config.go b/pkg/vmcp/config/config.go index ab3b522c21..b580b97aa2 100644 --- a/pkg/vmcp/config/config.go +++ b/pkg/vmcp/config/config.go @@ -166,6 +166,10 @@ type Config struct { // When enabled, vMCP exposes only find_tool and call_tool operations to clients // instead of all backend tools directly. This reduces token usage by allowing // LLMs to discover relevant tools on demand rather than receiving all tool definitions. + // Enabling the optimizer currently pins this instance to MCP 2025-11-25: the + // find_tool/call_tool meta-tools are session-scoped, so Modern-capable (2026-07-28) + // clients are negotiated down to the Legacy revision (see + // pkg/vmcp/server/modern_gate.go; Modern parity is tracked in stacklok/toolhive#6089). // +optional Optimizer *OptimizerConfig `json:"optimizer,omitempty" yaml:"optimizer,omitempty"` diff --git a/pkg/vmcp/conversion/errors.go b/pkg/vmcp/conversion/errors.go index aaa38ef5bf..c3f3a057e7 100644 --- a/pkg/vmcp/conversion/errors.go +++ b/pkg/vmcp/conversion/errors.go @@ -17,13 +17,17 @@ import ( // Go errors: the SDK maps them to generic internal errors. For domain errors // that opt into CodedError, preserve the code/data in StructuredContent instead. func ErrorToToolResult(err error) *sdkmcp.CallToolResult { + // Denial first (matches writeModernDispatchError): an error that is both a + // CodedError and wraps ErrAuthorizationFailed must render as the denial, + // never as retry-shaped coded data (the sets are disjoint today; this + // ordering is the invariant). + if errors.Is(err, vmcp.ErrAuthorizationFailed) { + return sdkmcp.NewToolResultError(vmcp.DenyMessageToolCall) + } var coded thvmcp.CodedError if errors.As(err, &coded) { return CodedErrorResult(err, coded) } - if errors.Is(err, vmcp.ErrAuthorizationFailed) { - return sdkmcp.NewToolResultError(vmcp.DenyMessageToolCall) - } return sdkmcp.NewToolResultError(err.Error()) } @@ -36,7 +40,7 @@ func CodedErrorResult(err error, coded thvmcp.CodedError) *sdkmcp.CallToolResult "code": coded.Code(), "message": err.Error(), } - if data := coded.Data(); data != nil { + if data := coded.Data(); len(data) > 0 { structured["data"] = data } result.StructuredContent = structured diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index cc3633b80e..adcb82a0ec 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -336,7 +336,7 @@ func TestModernDispatchBlockers(t *testing.T) { withOptimizer := classifyingHandlerTestServer() withOptimizer.optimizerFactory = stubOptimizerFactory - assert.Equal(t, []string{"optimizer"}, withOptimizer.modernDispatchBlockers(), + assert.Equal(t, []string{blockerOptimizer}, withOptimizer.modernDispatchBlockers(), "the session-scoped optimizer is not servable by the stateless Modern path") } diff --git a/pkg/vmcp/server/forwarding_realbackend_integration_test.go b/pkg/vmcp/server/forwarding_realbackend_integration_test.go index 4aa19880d8..d6898fd94f 100644 --- a/pkg/vmcp/server/forwarding_realbackend_integration_test.go +++ b/pkg/vmcp/server/forwarding_realbackend_integration_test.go @@ -156,7 +156,9 @@ func startForwardingBackend(t *testing.T) string { // 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. Modern -// dispatch is now unconditional (#5959 removed the kill-switch), so without +// dispatch is served whenever the capability gate is open +// (modernDispatchBlockers, modern_gate.go; #5959 removed the kill-switch), and +// these fixtures configure no blockers, so without // this pin the go-sdk-based client would negotiate Modern and the surface // under test would vanish mid-test — failing at connect rather than at the // behavior being asserted. Before that, the server's own version-omitting @@ -168,7 +170,8 @@ func startForwardingBackend(t *testing.T) string { // 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 Modern dispatch is unconditional, this +// LOAD-BEARING after #6033: with the capability gate open (modern_gate.go) -- +// and these fixtures configure no blockers -- 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 diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index 3b72780515..27c2b9766c 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -14,6 +14,7 @@ import ( "github.com/stacklok/toolhive/pkg/audit" "github.com/stacklok/toolhive/pkg/auth" mcpparser "github.com/stacklok/toolhive/pkg/mcp" + transportsession "github.com/stacklok/toolhive/pkg/transport/session" "github.com/stacklok/toolhive/pkg/vmcp" ) @@ -587,10 +588,13 @@ func writeModernCallFailure( // caller that reacts to -32021 by declaring the capability on a retry // learns immediately that doing so will not help here (the declared case is served by // writeModernCallFailure's -32603 branch, not by MRTR). +// +// id follows writeModernError (modern_envelope.go): absent (via +// transportsession.HasJSONRPCID) is encoded by omitting the "id" key, never +// as null. func writeModernMissingCapability(w http.ResponseWriter, id any, capName string) { - writeModernEnvelope(w, http.StatusOK, map[string]any{ + envelope := map[string]any{ "jsonrpc": "2.0", - "id": id, "error": map[string]any{ "code": mcpparser.CodeMissingClientCapability, "message": fmt.Sprintf( @@ -602,7 +606,11 @@ func writeModernMissingCapability(w http.ResponseWriter, id any, capName string) "requiredCapabilities": map[string]any{capName: map[string]any{}}, }, }, - }) + } + if transportsession.HasJSONRPCID(id) { + envelope["id"] = id + } + writeModernEnvelope(w, http.StatusOK, envelope) } // writeModernDispatchError classifies a POST-dispatch error from @@ -634,6 +642,10 @@ func writeModernMissingCapability(w http.ResponseWriter, id any, capName string) // non-authz error. Re-sanitizing here would just diverge from what the SDK // path already exposes for the identical failure. func writeModernDispatchError(w http.ResponseWriter, id any, denyMsg string, err error) { + // Denial first (matches conversion.ErrorToToolResult): an error that is + // both a CodedError and wraps ErrAuthorizationFailed must render as the + // denial, never as retry-shaped coded data (the sets are disjoint today; + // this ordering is the invariant). if errors.Is(err, vmcp.ErrAuthorizationFailed) { writeModernDenied(w, id, denyMsg) return diff --git a/pkg/vmcp/server/modern_dispatch_test.go b/pkg/vmcp/server/modern_dispatch_test.go index 77895c8fcf..82f57a597d 100644 --- a/pkg/vmcp/server/modern_dispatch_test.go +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -728,9 +728,9 @@ func TestDispatchModern_AuthzGate(t *testing.T) { // must reach the Modern client with that code and data intact, not be // laundered into an opaque -32603 — parity with the SDK path, where // conversion.ErrorToToolResult preserves the same code/data in -// structuredContent. HTTP status stays 200: -32029's natural 429 is in -// go-sdk's transient retry set and would be silently retried instead of -// surfaced (see writeModernCodedError). +// structuredContent. HTTP status stays 200: go-sdk rejects a non-200 POST +// response before decoding its body, so a 429 would discard the error object +// -- and data.retryAfterSeconds -- unread (see writeModernCodedError). func TestDispatchModern_CodedErrorPreservesCodeAndData(t *testing.T) { t.Parallel() diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index f51f157577..52cc52d9b3 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -245,6 +245,15 @@ type modernDiscoverResult struct { // LATEST_PROTOCOL_VERSION regardless of what a client requests -- it does // not support older Legacy revisions either). A discover-first client uses // this list to decide whether it can skip the Legacy initialize handshake. +// +// CONSTRAINT: this function must only be reached with the Modern capability +// gate open. It hardcodes MCPVersionModern into SupportedVersions and never +// consults the gate; that is truthful only because classifyingHandler +// (classification.go) routes server/discover to the SDK when the gate is +// closed, and the SDK's stateful transport computes the truthful Legacy-only +// list there. Do NOT plumb the gate in here — that would create a second +// source of the version list. Anyone re-routing gated discover to +// dispatchModern must revisit this. func newModernDiscover( hasTools, hasResources, hasTemplates, hasPrompts bool, serverName, serverVersion string, ) modernDiscoverResult { @@ -592,16 +601,18 @@ func writeModernError(w http.ResponseWriter, id any, code int, msg string) { // conversion.CodedErrorResult, the Legacy seam's rendering of the same // errors). // -// Status is HTTP 200 deliberately, for the same client-survival reason as -// writeModernMissingCapability's documented deviation (modern_dispatch.go): -// ToolHive's coded errors live in the implementation-defined -3202x space, so -// no spec MUSTs a 4xx for them, and the natural mapping for the rate -// limiter's -32029 (HTTP 429 + Retry-After, what pkg/ratelimit's HTTP -// middleware writes) is in go-sdk v1.7.0-pre.3's TRANSIENT retry set — a -// go-sdk client would silently retry the POST instead of surfacing the error -// with its retryAfterSeconds to the caller. The request was accepted and -// processed; the failure is an application-level JSON-RPC error riding the -// transport. +// Status is HTTP 200 deliberately: go-sdk's streamable client +// (v1.7.0-pre.3) rejects a non-200 POST response in checkResponse BEFORE +// decoding its body, so on any 4xx the JSON-RPC error object below — +// including data.retryAfterSeconds on the rate limiter's -32029 — would be +// discarded unread. 200 is the only status on which the client surfaces the +// coded error to the caller. The request was accepted and processed; the +// failure is an application-level JSON-RPC error riding the transport. +// (writeModernMissingCapability in modern_dispatch.go documents a related +// but distinct 200-over-4xx deviation for -32021.) Separately, -32029 sits +// in the -32020..-32099 band the draft MCP spec reserves exclusively for +// spec-defined codes; it predates that partition and its reallocation is +// tracked in #6101. // // id follows writeModernError: absent (via transportsession.HasJSONRPCID) is // encoded by omitting the "id" key, never as null. @@ -610,7 +621,7 @@ func writeModernCodedError(w http.ResponseWriter, id any, err error, coded mcppa "code": coded.Code(), "message": err.Error(), } - if data := coded.Data(); data != nil { + if data := coded.Data(); len(data) > 0 { errObj["data"] = data } envelope := map[string]any{ diff --git a/pkg/vmcp/server/modern_gate.go b/pkg/vmcp/server/modern_gate.go index 4b71348e5b..21223ea1d2 100644 --- a/pkg/vmcp/server/modern_gate.go +++ b/pkg/vmcp/server/modern_gate.go @@ -23,6 +23,10 @@ package server // design and store nothing) does NOT belong here; coexistence of that kind // is asserted by test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go. // +// - Every entry's comment must cite the issue tracking its Modern parity, so +// a stale entry (parity shipped, entry forgotten) surfaces in issue triage +// rather than silently keeping the instance off Modern. +// // - When Modern parity lands for a feature, delete its entry. The gate's // behavior is pinned by TestModernDispatchBlockers and // TestClassifyingHandler_ModernCapabilityGate (classification_test.go) plus @@ -30,6 +34,16 @@ package server // entry must flip cases there, so parity work cannot silently ship without // updating them. // +// - Loud refusals are deliberately out of scope. A composite workflow with an +// elicitation step (config.WorkflowStepConfig type "elicitation") works over +// Legacy sessions and fails Modern clients with an explicit -32021 +// MissingRequiredClientCapabilityError (or -32603 when the client declared +// the capability) — see writeModernCallFailure. That is an honest error the +// client can act on, not silently different behavior, so it does not gate +// the whole instance off Modern for one workflow definition. Distinct from +// #6059, which covers a Modern BACKEND returning input_required; here vMCP +// itself is the elicitor. +// // The result is derived from construction-time configuration only, so it is // constant for the life of the Server; Serve logs it once at startup. func (s *Server) modernDispatchBlockers() []string { @@ -45,13 +59,19 @@ func (s *Server) modernDispatchBlockers() []string { // `tools/call find_tool` would fail -32603 "not found" — the optimizer // feature would be invisibly disabled for exactly the newest clients. // Modern parity needs an identity- or instance-scoped index to replace the - // session-scoped one; until that lands, an optimizer-enabled instance is - // Legacy-only. s.optimizerFactory is the resolved factory and is non-nil on - // both composition paths (New and direct Serve) exactly when the optimizer - // is enabled. + // session-scoped one; that work is tracked in #6089, and deleting this entry + // is its definition of done. Until it lands, an optimizer-enabled instance + // is Legacy-only. A non-nil s.optimizerFactory is a faithful "optimizer + // enabled" signal: sessionmanager.New's constructor guard rejects an + // optimizer without AdvertiseFromCore at startup, so the factory can never + // be enabled yet invisible here. if s.optimizerFactory != nil { - blocked = append(blocked, "optimizer") + blocked = append(blocked, blockerOptimizer) } return blocked } + +// blockerOptimizer names the optimizer's entry in the blocker list; +// TestModernDispatchBlockers asserts on it by name. +const blockerOptimizer = "optimizer" diff --git a/pkg/vmcp/server/modern_pagination.go b/pkg/vmcp/server/modern_pagination.go index 98a8cda320..636c8199ee 100644 --- a/pkg/vmcp/server/modern_pagination.go +++ b/pkg/vmcp/server/modern_pagination.go @@ -102,19 +102,20 @@ import ( // does not, silently falsifying the equality claimed above. // // KNOWN GAP, stated rather than papered over: nothing currently detects that -// drift. The Modern side IS pinned behaviourally (a 1001-item corpus must yield a -// 1000-item first page). The Legacy side is not, because holding a client on -// Legacy would need a negotiation-pinning mechanism that does not exist here -- -// mcpcompat cannot request a protocol version (#5911) -- and -// test/integration/vmcp's Over1000Tools regression now exercises the Modern -// split rather than the Legacy one, since Modern dispatch became unconditional -// (#5959). +// drift. The Modern side IS pinned behaviourally in-package (a 1001-item corpus +// must yield a 1000-item first page), but not end-to-end: +// test/integration/vmcp's Over1000Tools regression exercises the Legacy split +// only -- its helpers are mcpcompat-backed, and mcpcompat cannot request +// 2026-07-28 (#5911) -- so no in-tree integration test pins the Modern split +// end-to-end. (A per-request Legacy/Modern pinning mechanism now exists in +// test/e2e's RawMCPClient, but the integration-tier helpers are still +// mcpcompat-backed.) // // So: treat the equality claimed above as an invariant maintained by REVIEW, not // by CI, and re-check it on any go-sdk bump. This deliberately does not name a -// mechanism the Legacy twin should use; whether that is a pinning harness, a -// loud failure at negotiation, or something else is undecided, and this comment -// must not go stale by presuming one. +// mechanism the missing Modern end-to-end coverage should use; whether that is a +// pinning harness, a loud failure at negotiation, or something else is +// undecided, and this comment must not go stale by presuming one. const modernPageSize = 1000 // modernCursorMaxLen caps an inbound cursor before it is decoded. diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index 635a3fd2e6..ec05aa26de 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -28,14 +28,12 @@ import ( // It carries the subset of the legacy server.Config that configures the HTTP/SDK // runtime, plus the cross-cutting TelemetryProvider/AuditConfig that are consumed // by both the core (core.New) and the transport (Serve) — not a clean partition. -// The fields the core exclusively owns (Aggregator, Router, BackendClient, Authz) -// live on core.Config instead and are absent here. Two collaborators are shared -// rather than core-owned, so they appear on both configs: BackendRegistry (the -// Serve session layer also enumerates backends when creating a session) and the -// composite-tool WorkflowDefs (carried indirectly via SessionManagerConfig, whose -// ComposerFactory closes over the core-owned router/backend client). The -// composition root passes the same BackendRegistry instance to both core.New and -// Serve and assembles SessionManagerConfig. +// The fields the core exclusively owns (Aggregator, Router, BackendClient, Authz, +// composite-tool WorkflowDefs) live on core.Config instead and are absent here. +// One collaborator is shared rather than core-owned, so it appears on both +// configs: BackendRegistry (the Serve session layer also enumerates backends when +// creating a session). The composition root passes the same BackendRegistry +// instance to both core.New and Serve and assembles SessionManagerConfig. // // The name intentionally stutters as server.ServerConfig: it is mandated by the // New/Serve split, and the package's existing Config is the legacy transport @@ -110,17 +108,11 @@ type ServerConfig struct { // SessionManagerConfig is the pre-built construction config for the vMCP session // manager (sessionmanager.New). FactoryConfig.Base is the required underlying - // MultiSessionFactory; the config also carries the composite-tool WorkflowDefs and - // ComposerFactory, the optimizer factory/config, and the telemetry provider. - // Required; Serve returns an error when it is nil. The composition root assembles - // it because the ComposerFactory closes over the core-owned router and backend - // client. - // - // Caller responsibility: unlike server.New, Serve does NOT run validateWorkflows on - // FactoryConfig.WorkflowDefs — the composition root must validate composite-tool - // definitions before assembling this config (sessionmanager.New only checks the - // WorkflowDefs/ComposerFactory pairing). This responsibility moves here with the - // relocation and matters when server.New is routed through Serve in Phase 3. + // MultiSessionFactory; the config also carries the optimizer factory/config, the + // telemetry provider, the session-cache capacity, and the AdvertiseFromCore flag. + // Required; Serve returns an error when it is nil. Composite-tool workflows are + // NOT carried here — they are core-owned (core.Config.WorkflowDefs, validated in + // core.New). // // Caller responsibility (AC2, the single-aggregation contract): FactoryConfig.Base // MUST be constructed WITHOUT a session.WithAggregator option on the Serve path. On @@ -135,9 +127,10 @@ type ServerConfig struct { // // Caller responsibility (optimizer): to enable the optimizer on the Serve path, set // FactoryConfig.OptimizerConfig/OptimizerFactory AND FactoryConfig.AdvertiseFromCore. - // Serve then builds a per-session optimizer over the core's tools (serve_optimizer.go); - // AdvertiseFromCore suppresses the factory's own optimizer decorator so the shared FTS5 - // store is not double-indexed (see FactoryConfig.AdvertiseFromCore). + // Serve then builds a per-session optimizer over the core's tools (serve_optimizer.go). + // sessionmanager.New rejects an optimizer without AdvertiseFromCore, so a composition + // root that forgets the flag fails at startup instead of double-indexing the shared + // FTS5 store (see FactoryConfig.AdvertiseFromCore). SessionManagerConfig *sessionmanager.FactoryConfig // TelemetryProvider is the cross-cutting telemetry provider (also consumed by @@ -383,7 +376,7 @@ func Serve(ctx context.Context, v core.VMCP, cfg *ServerConfig) (*Server, error) // so this single line is the operator-visible record of why Modern-capable // clients of this instance negotiate down to Legacy (see modern_gate.go). if blocked := srv.modernDispatchBlockers(); len(blocked) > 0 { - slog.Info("MCP 2026-07-28 (Modern) dispatch disabled: enabled features require the session (Legacy) path", + slog.Warn("MCP 2026-07-28 (Modern) dispatch disabled: enabled features require the session (Legacy) path", "features", blocked) } diff --git a/pkg/vmcp/server/serve_lifecycle_test.go b/pkg/vmcp/server/serve_lifecycle_test.go index 609be45eee..89ea57cf5a 100644 --- a/pkg/vmcp/server/serve_lifecycle_test.go +++ b/pkg/vmcp/server/serve_lifecycle_test.go @@ -149,8 +149,9 @@ func TestServeWithOptimizerStartsAndStopsCleanly(t *testing.T) { cfg := testMinimalServeConfig() cfg.SessionManagerConfig = &sessionmanager.FactoryConfig{ - Base: testMinimalFactory(), - OptimizerConfig: &optimizer.Config{}, + Base: testMinimalFactory(), + OptimizerConfig: &optimizer.Config{}, + AdvertiseFromCore: true, } srv, err := Serve(context.Background(), &stubVMCP{}, cfg) diff --git a/pkg/vmcp/server/sessionmanager/factory.go b/pkg/vmcp/server/sessionmanager/factory.go index 38f1747e87..73bbc3e9f5 100644 --- a/pkg/vmcp/server/sessionmanager/factory.go +++ b/pkg/vmcp/server/sessionmanager/factory.go @@ -68,18 +68,20 @@ type FactoryConfig struct { // AdvertiseFromCore signals that the advertised capability set is sourced from // the core (the Serve path), not from this factory's per-session aggregation. // - // It is the single switch that selects WHICH layer indexes the shared FTS5 store, - // so the two can never both do it (the AC6 double-index): - // - true: New does NOT install the optimizer decorator on the session factory, - // and exposes the resolved factory via Manager.OptimizerFactory so the - // Serve layer builds a per-session optimizer over the core's tools. - // - false: New installs the optimizer decorator (legacy behavior) and - // Manager.OptimizerFactory returns nil, so a Serve composition root that - // enables the optimizer but forgets this flag gets no Serve-layer - // optimizer rather than a second, divergent upsert into the store. - // Either way New resolves the optimizer factory and owns its store/cleanup. Has no - // effect when the optimizer is disabled. The legacy server.New path leaves this - // false, so its optimizer decorator is unchanged. + // It is required whenever an optimizer is configured: + // - true: New exposes the resolved factory via Manager.OptimizerFactory so + // the Serve layer builds a per-session optimizer over the core's + // tools — the single writer of the shared FTS5 store (the AC6 + // no-double-index guarantee). + // - false: fine without an optimizer; with one, New rejects the config at + // construction, because the Serve layer discards the decorator's + // per-session tools (the optimizer would index the store yet serve + // nobody) and the Modern capability gate would fail open. + // New resolves the optimizer factory and owns its store/cleanup. server.New + // sets this unconditionally (server.go), so every in-tree composition + // advertises from the core; the flag exists for direct-Serve embedders. The + // decorator branch the false case used to select is now unreachable — its + // deletion is tracked in #6103. AdvertiseFromCore bool } @@ -148,6 +150,9 @@ func buildDecoratingFactory( // over the core's advertised set, so the factory's optimizer decorator is skipped // to avoid double-indexing the shared store (see FactoryConfig.AdvertiseFromCore). // Composite tools and their telemetry are owned by the core, not the factory. + // This branch is unreachable: New rejects an optimizer without AdvertiseFromCore, + // so optimizerFactory is nil whenever the flag is false. Deleting the decorator + // path is tracked in #6103. if optimizerFactory != nil && !cfg.AdvertiseFromCore { decorators = append(decorators, optimizerDecoratorFn(optimizerFactory, terminateSession)) } diff --git a/pkg/vmcp/server/sessionmanager/optimizer_gate_test.go b/pkg/vmcp/server/sessionmanager/optimizer_gate_test.go index 7b2bfacd18..d83834fa6d 100644 --- a/pkg/vmcp/server/sessionmanager/optimizer_gate_test.go +++ b/pkg/vmcp/server/sessionmanager/optimizer_gate_test.go @@ -15,12 +15,14 @@ import ( "github.com/stacklok/toolhive/pkg/vmcp/optimizer" ) -// TestOptimizerFactoryGatedOnAdvertiseFromCore locks in the AC6 double-index -// guarantee: New surfaces the optimizer factory via OptimizerFactory() ONLY when -// AdvertiseFromCore is true. That makes the session-factory decorator (installed iff -// !AdvertiseFromCore) and the Serve-layer getter mutually exclusive store writers, so a -// Serve composition root that enables the optimizer but forgets the flag gets a nil -// factory (no Serve-layer optimizer) instead of a silent second upsert into the store. +// TestOptimizerFactoryGatedOnAdvertiseFromCore locks in the constructor guard +// behind the AC6 single-writer guarantee and the Modern capability gate's +// enablement signal: New accepts an optimizer only together with +// AdvertiseFromCore, so the Serve layer (via OptimizerFactory()) is the sole +// writer of the shared FTS5 store and a non-nil OptimizerFactory() faithfully +// means "optimizer enabled" (pkg/vmcp/server/modern_gate.go depends on that). +// Optimizer-without-flag is rejected at construction instead of silently +// producing an optimizer that indexes the store but serves nobody. func TestOptimizerFactoryGatedOnAdvertiseFromCore(t *testing.T) { t.Parallel() @@ -31,10 +33,9 @@ func TestOptimizerFactoryGatedOnAdvertiseFromCore(t *testing.T) { tests := []struct { name string advertiseFromCore bool - wantSurfaced bool }{ - {"surfaced to Serve when advertising from core", true, true}, - {"not surfaced on the legacy path (decorator owns it)", false, false}, + {"surfaced to Serve when advertising from core", true}, + {"rejected at construction without AdvertiseFromCore", false}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -47,16 +48,16 @@ func TestOptimizerFactoryGatedOnAdvertiseFromCore(t *testing.T) { OptimizerFactory: optFactory, AdvertiseFromCore: tc.advertiseFromCore, }, newFakeRegistry()) - require.NoError(t, err) - t.Cleanup(func() { _ = cleanup(context.Background()) }) - if tc.wantSurfaced { - assert.NotNil(t, sm.OptimizerFactory(), - "the factory must be surfaced to the Serve layer when AdvertiseFromCore is set") - } else { - assert.Nil(t, sm.OptimizerFactory(), - "the factory must NOT be surfaced when AdvertiseFromCore is false (decorator owns the store)") + if !tc.advertiseFromCore { + require.ErrorContains(t, err, "AdvertiseFromCore", + "New must reject an optimizer without AdvertiseFromCore at construction") + return } + require.NoError(t, err) + t.Cleanup(func() { _ = cleanup(context.Background()) }) + assert.NotNil(t, sm.OptimizerFactory(), + "the factory must be surfaced to the Serve layer when AdvertiseFromCore is set") }) } } diff --git a/pkg/vmcp/server/sessionmanager/session_manager.go b/pkg/vmcp/server/sessionmanager/session_manager.go index 26126b7521..b1808e99ca 100644 --- a/pkg/vmcp/server/sessionmanager/session_manager.go +++ b/pkg/vmcp/server/sessionmanager/session_manager.go @@ -95,6 +95,12 @@ type Manager struct { // registry. It builds the decorating session factory from cfg, wiring the // optimizer and composite tool layers internally. // +// An optimizer (FactoryConfig.OptimizerFactory or OptimizerConfig) requires +// FactoryConfig.AdvertiseFromCore; New rejects the combination otherwise. The +// guard makes a non-nil OptimizerFactory() a faithful "optimizer enabled" +// signal, which the Modern capability gate in pkg/vmcp/server/modern_gate.go +// depends on. +// // The returned cleanup function releases any resources allocated during // construction (e.g. the optimizer's SQLite store). Callers must invoke it // on shutdown. If no cleanup is needed, a no-op function is returned. @@ -109,6 +115,10 @@ func New( if cfg.CacheCapacity < 0 { return nil, nil, fmt.Errorf("sessionmanager.New: CacheCapacity must be >= 0 (got %d)", cfg.CacheCapacity) } + if (cfg.OptimizerFactory != nil || cfg.OptimizerConfig != nil) && !cfg.AdvertiseFromCore { + return nil, nil, fmt.Errorf("sessionmanager.New: the optimizer requires " + + "FactoryConfig.AdvertiseFromCore (the Serve layer builds it over the core's tools)") + } capacity := cfg.CacheCapacity if capacity == 0 { capacity = defaultCacheCapacity @@ -126,14 +136,13 @@ func New( backendReg: backendRegistry, } - // Surface the resolved optimizer factory to the Serve path ONLY when - // AdvertiseFromCore is set. That makes the two store writers mutually exclusive: - // the session-factory decorator runs iff !AdvertiseFromCore (buildDecoratingFactory - // below), and OptimizerFactory() returns a non-nil factory iff AdvertiseFromCore — - // so a Serve composition root that enables the optimizer but forgets the flag gets a - // nil factory (no Serve-layer optimizer) rather than a silent double-index of the - // shared FTS5 store. The legacy server.New path leaves AdvertiseFromCore false and - // never calls OptimizerFactory(), so its decorator is unaffected. + // Surface the resolved optimizer factory to the Serve path. The constructor + // guard above rejects an optimizer without AdvertiseFromCore, so the shared + // FTS5 store has exactly one writer (the Serve layer) and OptimizerFactory() + // is non-nil exactly when the optimizer is enabled. server.New sets + // AdvertiseFromCore unconditionally (server.go), so every in-tree composition + // takes this branch; the flag exists for direct-Serve embedders, which New + // rejects when they configure an optimizer without setting it. if cfg.AdvertiseFromCore { sm.optimizerFactory = optimizerFactory } @@ -160,16 +169,17 @@ func New( return sm, cleanup, nil } -// OptimizerFactory returns the resolved (telemetry-wrapped) optimizer factory, or -// nil when the optimizer is disabled OR FactoryConfig.AdvertiseFromCore is false. -// -// It is consumed by the Serve path (FactoryConfig.AdvertiseFromCore), which builds -// a per-session optimizer over the core's advertised tool set rather than via the -// session decorator. Gating on AdvertiseFromCore makes the decorator and this getter -// mutually exclusive store writers, so the shared FTS5 store can never be double-indexed -// (see New). The optimizer's shared store and its cleanup remain owned by this Manager -// (the cleanup function returned from New). On the legacy server.New path the factory is -// applied internally via the session decorator and this getter is unused. +// OptimizerFactory returns the resolved (telemetry-wrapped) optimizer factory, +// or nil exactly when the optimizer is disabled: New rejects an optimizer +// without FactoryConfig.AdvertiseFromCore, so nil-ness here is a faithful +// "optimizer enabled" signal (the Modern capability gate in +// pkg/vmcp/server/modern_gate.go depends on it). +// +// It is consumed by the Serve path, which builds a per-session optimizer over +// the core's advertised tool set. The optimizer's shared store and its cleanup +// remain owned by this Manager (the cleanup function returned from New). +// server.New sets AdvertiseFromCore unconditionally, so this getter is live on +// every composition path. func (m *Manager) OptimizerFactory() func(context.Context, []mcpserver.ServerTool) (optimizer.Optimizer, error) { return m.optimizerFactory } diff --git a/test/e2e/mcp_raw_client.go b/test/e2e/mcp_raw_client.go index 352d9f15eb..455ea166cd 100644 --- a/test/e2e/mcp_raw_client.go +++ b/test/e2e/mcp_raw_client.go @@ -176,9 +176,10 @@ func (r *RawRequest) WithClientInfo(name, version string) *RawRequest { // WithStreamableAccept sets "Accept: application/json, text/event-stream". // A real go-sdk streamable-HTTP server rejects a POST without this header // (HTTP 400), so requests bound for a real backend (e.g. the k8s tier) must -// set it. Do NOT set it for requests to the ToolHive proxy: the proxy does not -// require it and switches to an SSE response body when it is present, which -// this client's plain-JSON parser cannot read. +// set it. Requests to the ToolHive proxy still omit it by convention: the +// proxy does not require it, and omitting it keeps responses plain JSON (the +// client parses SSE responses too -- see sseResponsePayload -- and flipping +// proxy-bound requests to the conformant Accept is tracked in #6104). func (r *RawRequest) WithStreamableAccept() *RawRequest { return r.SetHeader("Accept", "application/json, text/event-stream") } @@ -370,7 +371,7 @@ func (c *RawMCPClient) SendRaw(ctx context.Context, url string, headers map[stri Body: respBody, } envelope := respBody - if strings.HasPrefix(resp.Header.Get("Content-Type"), "text/event-stream") { + if strings.HasPrefix(strings.ToLower(resp.Header.Get("Content-Type")), "text/event-stream") { envelope = sseResponsePayload(respBody) } populateEnvelope(envelope, result) diff --git a/test/e2e/mcp_raw_client_test.go b/test/e2e/mcp_raw_client_test.go index 57594ddf3e..76e3c21542 100644 --- a/test/e2e/mcp_raw_client_test.go +++ b/test/e2e/mcp_raw_client_test.go @@ -333,7 +333,7 @@ func TestRawClientSend(t *testing.T) { _, err = client.Send(context.Background(), server.URL, req) require.NoError(t, err) require.Empty(t, captured.headers.Get("Accept"), - "no Accept by default: it makes the ToolHive proxy emit an SSE body this client can't parse") + "no Accept by default: keeps proxy responses plain JSON; conformant-Accept flip tracked in #6104") withAccept, err := NewModernRequest("tools/list", nil) require.NoError(t, err) @@ -437,6 +437,56 @@ func TestRawClientSSEResponse(t *testing.T) { require.Equal(t, json.Number("1"), resp.ID) require.JSONEq(t, `{}`, string(resp.Result)) }) + + t.Run("CRLF line endings parse identically to LF", func(t *testing.T) { + t.Parallel() + server := newSSEServer(t, "event: message\r\n"+ + `data: {"jsonrpc":"2.0","id":5,"result":{"tools":[{"name":"echo"}]}}`+"\r\n\r\n") + + req, err := NewLegacyRequest("tools/list", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Equal(t, "2.0", resp.JSONRPC) + require.Equal(t, json.Number("5"), resp.ID) + require.JSONEq(t, `{"tools":[{"name":"echo"}]}`, string(resp.Result)) + require.Nil(t, resp.Error) + }) + + t.Run("data: with no leading space still parses", func(t *testing.T) { + t.Parallel() + server := newSSEServer(t, "event: message\n"+ + `data:{"jsonrpc":"2.0","id":9,"result":{}}`+"\n\n") + + req, err := NewLegacyRequest("ping", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Equal(t, json.Number("9"), resp.ID) + require.JSONEq(t, `{}`, string(resp.Result)) + }) + + t.Run("a batch (array) data payload leaves the envelope empty, Body raw", func(t *testing.T) { + t.Parallel() + // A JSON array is not a single response object, so it never populates + // the envelope — matching RawResponse's batch contract (inspect Body). + body := "event: message\n" + + `data: [{"jsonrpc":"2.0","id":1,"result":{}},{"jsonrpc":"2.0","id":2,"result":{}}]` + "\n\n" + server := newSSEServer(t, body) + + req, err := NewLegacyRequest("tools/list", nil) + require.NoError(t, err) + resp, err := client.Send(context.Background(), server.URL, req) + require.NoError(t, err) + + require.Empty(t, resp.JSONRPC) + require.Nil(t, resp.ID) + require.Nil(t, resp.Result) + require.Nil(t, resp.Error) + require.Equal(t, body, string(resp.Body), "Body must keep the raw stream for the caller") + }) } func TestNewRawMCPClientRejectsNonPositiveTimeout(t *testing.T) { diff --git a/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go b/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go index 7ae1211b1e..08ed3e3fb4 100644 --- a/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go +++ b/test/e2e/thv-operator/virtualmcp/virtualmcp_dual_era_redis_test.go @@ -73,11 +73,7 @@ const dualEraRedisKeyPrefix = "thv:vmcp:dualera:" // classifyingHandler routes server/discover to dispatchModern instead of the // SDK's stateful fallback (pkg/vmcp/server/classification.go), and such a client // always negotiates Modern and gets no session -- there is no way to make it open -// a Legacy session against this vMCP. The observed failure is mcpcompat's -// Initialize unconditionally installing list-changed handlers, which makes go-sdk -// open a "subscriptions/listen" stream vMCP does not implement, answered with -// HTTP 404 and surfaced as go-sdk's ErrSessionMissing ("session not found") -// despite Initialize having "succeeded". Use *e2e.RawMCPClient instead: it pins +// a Legacy session against this vMCP. Use *e2e.RawMCPClient instead: it pins // the era explicitly per request (Legacy via e2e.NewLegacyRequest + the // MCP-Protocol-Version header), which is what actually gets a Legacy session // from a Modern-serving vMCP. diff --git a/test/e2e/vmcp_dual_era_helpers_test.go b/test/e2e/vmcp_dual_era_helpers_test.go index fad8c11baf..0a89a55bb4 100644 --- a/test/e2e/vmcp_dual_era_helpers_test.go +++ b/test/e2e/vmcp_dual_era_helpers_test.go @@ -137,7 +137,8 @@ func appendHealthCheckConfig(path string) { // startDualEraVMCP starts `thv vmcp serve`, using --config configPath when // configPath is non-empty, else quick mode (--group groupName). vMCP serves both -// MCP revisions unconditionally (#5959), so this only assembles the arguments — +// MCP revisions whenever the capability gate is open (modern_gate.go), and these +// specs configure no blockers, so this only assembles the arguments — // it no longer overrides the inherited environment, and so delegates the process // start to e2e.StartLongRunningTHVCommand. func startDualEraVMCP(config *e2e.TestConfig, groupName, configPath string, port int) *exec.Cmd { diff --git a/test/e2e/vmcp_dual_era_test.go b/test/e2e/vmcp_dual_era_test.go index 5999f0a61c..ec1fc83941 100644 --- a/test/e2e/vmcp_dual_era_test.go +++ b/test/e2e/vmcp_dual_era_test.go @@ -43,11 +43,13 @@ import ( // (mirrors the same finding already documented in dual_era_mixing_test.go // for the single-server transparent proxy). // -// Known harness limitation: no request in this file sets -// "Accept: application/json, text/event-stream" (a MUST on both revisions), -// since e2e.RawMCPClient has no SSE response parser and the proxy switches to -// an SSE body whenever that header is present (see mcp_raw_client.go). So the -// bridge is proven here only under a non-conformant Accept header. +// Deliberate harness choice: no request in this file sets +// "Accept: application/json, text/event-stream" (a MUST on both revisions). +// e2e.RawMCPClient now parses SSE-framed POST responses (sseResponsePayload, +// mcp_raw_client.go), but these specs still omit the header so Legacy +// responses stay plain JSON; flipping them to the conformant Accept is +// tracked in #6104. Until then the bridge is proven here only under a +// non-conformant Accept header. var _ = Describe("vMCP Dual-Era Bridge", Label("vmcp", "dual-era", "e2e"), Serial, func() { Context("one Legacy and one Modern backend in the same group", func() { var ( From 31cec89959ecc0a5972c21b18e34cb45845481f3 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Tue, 28 Jul 2026 13:21:28 +0000 Subject: [PATCH 7/7] Regenerate CRD manifests for optimizer field doc The Optimizer field's new Legacy-pin documentation flows into the controller-gen CRD manifests as well as crd-api.md; the Generate CRDs CI job diffs deploy/charts after task operator-manifests and caught the missed regeneration. Co-Authored-By: Claude Opus 4.8 --- .../crds/toolhive.stacklok.dev_virtualmcpservers.yaml | 8 ++++++++ .../toolhive.stacklok.dev_virtualmcpservers.yaml | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml index ffd1118f61..60e3f2e3f3 100644 --- a/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/files/crds/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1819,6 +1819,10 @@ spec: When enabled, vMCP exposes only find_tool and call_tool operations to clients instead of all backend tools directly. This reduces token usage by allowing LLMs to discover relevant tools on demand rather than receiving all tool definitions. + Enabling the optimizer currently pins this instance to MCP 2025-11-25: the + find_tool/call_tool meta-tools are session-scoped, so Modern-capable (2026-07-28) + clients are negotiated down to the Legacy revision (see + pkg/vmcp/server/modern_gate.go; Modern parity is tracked in stacklok/toolhive#6089). properties: embeddingHeaders: additionalProperties: @@ -5266,6 +5270,10 @@ spec: When enabled, vMCP exposes only find_tool and call_tool operations to clients instead of all backend tools directly. This reduces token usage by allowing LLMs to discover relevant tools on demand rather than receiving all tool definitions. + Enabling the optimizer currently pins this instance to MCP 2025-11-25: the + find_tool/call_tool meta-tools are session-scoped, so Modern-capable (2026-07-28) + clients are negotiated down to the Legacy revision (see + pkg/vmcp/server/modern_gate.go; Modern parity is tracked in stacklok/toolhive#6089). properties: embeddingHeaders: additionalProperties: diff --git a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml index 5b79959d56..726d68b965 100644 --- a/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml +++ b/deploy/charts/operator-crds/templates/toolhive.stacklok.dev_virtualmcpservers.yaml @@ -1822,6 +1822,10 @@ spec: When enabled, vMCP exposes only find_tool and call_tool operations to clients instead of all backend tools directly. This reduces token usage by allowing LLMs to discover relevant tools on demand rather than receiving all tool definitions. + Enabling the optimizer currently pins this instance to MCP 2025-11-25: the + find_tool/call_tool meta-tools are session-scoped, so Modern-capable (2026-07-28) + clients are negotiated down to the Legacy revision (see + pkg/vmcp/server/modern_gate.go; Modern parity is tracked in stacklok/toolhive#6089). properties: embeddingHeaders: additionalProperties: @@ -5269,6 +5273,10 @@ spec: When enabled, vMCP exposes only find_tool and call_tool operations to clients instead of all backend tools directly. This reduces token usage by allowing LLMs to discover relevant tools on demand rather than receiving all tool definitions. + Enabling the optimizer currently pins this instance to MCP 2025-11-25: the + find_tool/call_tool meta-tools are session-scoped, so Modern-capable (2026-07-28) + clients are negotiated down to the Legacy revision (see + pkg/vmcp/server/modern_gate.go; Modern parity is tracked in stacklok/toolhive#6089). properties: embeddingHeaders: additionalProperties: