From 625ec2e4f67e442595bac97f0085019005ccdc3c Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 23 Jul 2026 20:32:24 +0200 Subject: [PATCH 1/9] Add core.Discover and result backend attribution Two core additions supporting the Modern (2026-07-28) dispatch path: - core.VMCP.Discover computes server/discover's capability flags from one aggregatedView, deriving each flag through the same admission filters the List* verbs use, so the flags stay post-admission-filtered per identity and cannot drift. Mirrors the ListBackends(filterUnauthorized) precedent. - ToolCallResult/ResourceReadResult/PromptGetResult gain a BackendID, set by the core from the routed target (empty for composite tools). It is audit-only (json:"-", never serialized) and lets the transport layer label which backend served a call, matching the Serve path. Co-Authored-By: Claude Opus 4.8 --- pkg/vmcp/core/core.go | 25 ++++++++++ pkg/vmcp/core/core_calls.go | 21 ++++++-- pkg/vmcp/core/core_calls_test.go | 4 ++ pkg/vmcp/core/core_vmcp.go | 39 +++++++++++++++ pkg/vmcp/core/core_vmcp_test.go | 70 +++++++++++++++++++++++++++ pkg/vmcp/server/serve_session_test.go | 4 ++ pkg/vmcp/server/serve_test.go | 3 ++ pkg/vmcp/types.go | 13 +++++ 8 files changed, 176 insertions(+), 3 deletions(-) diff --git a/pkg/vmcp/core/core.go b/pkg/vmcp/core/core.go index 3d6f997025..4c2a9be74a 100644 --- a/pkg/vmcp/core/core.go +++ b/pkg/vmcp/core/core.go @@ -32,6 +32,17 @@ import ( "github.com/stacklok/toolhive/pkg/vmcp/router" ) +// DiscoverCapabilities summarizes, for one identity, whether each capability +// kind has at least one admission-filtered entry. It carries no descriptor +// arrays -- only presence flags -- unlike the []vmcp.Tool/Resource/etc. slices +// ListTools/ListResources/ListResourceTemplates/ListPrompts return. +type DiscoverCapabilities struct { + HasTools bool + HasResources bool + HasResourceTemplates bool + HasPrompts bool +} + // VMCP is the core Virtual MCP domain object. // // Contract: @@ -197,6 +208,20 @@ type VMCP interface { // corresponding authorized list would not show. LookupBackend(ctx context.Context, identity *auth.Identity, backendID string) (*vmcp.Backend, error) + // Discover returns identity's capability-presence flags -- whether it is + // admitted to at least one tool, resource, resource template, and prompt -- + // from a SINGLE aggregation of backend capabilities, for server/discover. + // + // It applies the exact same admission-filtered code paths + // ListTools/ListResources/ListResourceTemplates/ListPrompts use against one + // shared aggregated view, so a flag is true iff the ADMISSION-FILTERED set + // for that capability is non-empty -- never derived from the raw aggregate, + // which would leak capabilities identity cannot reach. This mirrors the + // post-admission-summary precedent ListBackends(filterUnauthorized=true) + // established, applied to presence flags instead of a backend list. See + // ListTools for the nil/anonymous identity semantics. + Discover(ctx context.Context, identity *auth.Identity) (DiscoverCapabilities, error) + // BackendHealth returns the backend health reporter the core owns, or nil when health // monitoring is disabled. The core builds, starts, and (via Close) stops the monitor and // filters capabilities with it; the transport layer uses this only to report on or sync diff --git a/pkg/vmcp/core/core_calls.go b/pkg/vmcp/core/core_calls.go index 993ccd7ed9..0505073ece 100644 --- a/pkg/vmcp/core/core_calls.go +++ b/pkg/vmcp/core/core_calls.go @@ -66,7 +66,12 @@ func (c *coreVMCP) CallTool( } return nil, fmt.Errorf("routing tool %q: %w", name, err) } - return c.backendClient.CallTool(ctx, target, name, argsCopy, metaCopy) + result, err := c.backendClient.CallTool(ctx, target, name, argsCopy, metaCopy) + if err != nil { + return nil, err + } + result.BackendID = target.WorkloadID + return result, nil } // ReadResource reads the resource at uri from its backend. Returns @@ -95,7 +100,12 @@ func (c *coreVMCP) ReadResource( } // Pass the advertised URI; the backend client owns the single translation to // the backend's capability name (client.go:874), matching CallTool. - return c.backendClient.ReadResource(ctx, target, uri) + result, err := c.backendClient.ReadResource(ctx, target, uri) + if err != nil { + return nil, err + } + result.BackendID = target.WorkloadID + return result, nil } // GetPrompt retrieves the named prompt from its backend. args is treated as @@ -126,7 +136,12 @@ func (c *coreVMCP) GetPrompt( } // Pass the advertised name; the backend client owns the single translation to // the backend's capability name (client.go:927), matching CallTool. - return c.backendClient.GetPrompt(ctx, target, name, maps.Clone(args)) + result, err := c.backendClient.GetPrompt(ctx, target, name, maps.Clone(args)) + if err != nil { + return nil, err + } + result.BackendID = target.WorkloadID + return result, nil } // Complete resolves argument-completion candidates for the referenced prompt or diff --git a/pkg/vmcp/core/core_calls_test.go b/pkg/vmcp/core/core_calls_test.go index 7d02ef4069..a2102d8aad 100644 --- a/pkg/vmcp/core/core_calls_test.go +++ b/pkg/vmcp/core/core_calls_test.go @@ -45,6 +45,7 @@ func TestCallTool_RoutesToBackend(t *testing.T) { got, err := c.CallTool(context.Background(), nil, "tool_a", map[string]any{"a": 1}, nil) require.NoError(t, err) assert.Equal(t, want, got) + assert.Equal(t, testBackendID, got.BackendID, "CallTool must stamp the routed target's backend onto the result") } func TestCallTool_NotFound(t *testing.T) { @@ -121,6 +122,7 @@ func TestCallTool_CompositeWorkflow(t *testing.T) { require.NotNil(t, got) assert.False(t, got.IsError) assert.Equal(t, true, got.StructuredContent["ok"]) + assert.Empty(t, got.BackendID, "a composite tool has no single serving backend") } func TestCallTool_CompositeNotAccessible(t *testing.T) { @@ -160,6 +162,7 @@ func TestReadResource(t *testing.T) { got, err := c.ReadResource(context.Background(), nil, "file://a") require.NoError(t, err) assert.Equal(t, want, got) + assert.Equal(t, testBackendID, got.BackendID, "ReadResource must stamp the routed target's backend onto the result") } func TestReadResource_NotFound(t *testing.T) { @@ -194,6 +197,7 @@ func TestGetPrompt(t *testing.T) { got, err := c.GetPrompt(context.Background(), nil, "p1", map[string]any{"x": 1}) require.NoError(t, err) assert.Equal(t, want, got) + assert.Equal(t, testBackendID, got.BackendID, "GetPrompt must stamp the routed target's backend onto the result") } func TestGetPrompt_CopyBeforeMutate(t *testing.T) { diff --git a/pkg/vmcp/core/core_vmcp.go b/pkg/vmcp/core/core_vmcp.go index 0e9a904798..98c5523fca 100644 --- a/pkg/vmcp/core/core_vmcp.go +++ b/pkg/vmcp/core/core_vmcp.go @@ -349,6 +349,45 @@ func (c *coreVMCP) ListPrompts(ctx context.Context, identity *auth.Identity) ([] return c.admission.FilterPrompts(ctx, identity, agg.Prompts) } +// Discover aggregates backend capabilities ONCE and derives all four +// capability-presence flags from that single view, applying the exact same +// admission-filter code paths ListTools/ListResources/ListResourceTemplates/ +// ListPrompts each apply independently (advertisedTools+FilterTools, +// FilterResources, filterResourceTemplates, FilterPrompts). Sharing those +// helpers against one aggregatedView call -- rather than reimplementing the +// filtering -- is what guarantees a flag can never drift from what the +// corresponding List* verb would show for the same identity. +func (c *coreVMCP) Discover(ctx context.Context, identity *auth.Identity) (DiscoverCapabilities, error) { + agg, err := c.aggregatedView(ctx) + if err != nil { + return DiscoverCapabilities{}, err + } + + tools, err := c.admission.FilterTools(ctx, identity, c.advertisedTools(agg)) + if err != nil { + return DiscoverCapabilities{}, err + } + resources, err := c.admission.FilterResources(ctx, identity, agg.Resources) + if err != nil { + return DiscoverCapabilities{}, err + } + templates, err := c.filterResourceTemplates(ctx, identity, agg.ResourceTemplates) + if err != nil { + return DiscoverCapabilities{}, err + } + prompts, err := c.admission.FilterPrompts(ctx, identity, agg.Prompts) + if err != nil { + return DiscoverCapabilities{}, err + } + + return DiscoverCapabilities{ + HasTools: len(tools) > 0, + HasResources: len(resources) > 0, + HasResourceTemplates: len(templates) > 0, + HasPrompts: len(prompts) > 0, + }, nil +} + // LookupTool resolves an advertised tool name (incl. composite tools) to its // capability without invoking it. It delegates to ListTools, so it applies the // same health/advertising AND admission view: a name that is unknown, unadvertised, diff --git a/pkg/vmcp/core/core_vmcp_test.go b/pkg/vmcp/core/core_vmcp_test.go index 7e43939226..5506635fba 100644 --- a/pkg/vmcp/core/core_vmcp_test.go +++ b/pkg/vmcp/core/core_vmcp_test.go @@ -352,6 +352,76 @@ func TestListResourceTemplates_Empty(t *testing.T) { assert.Empty(t, templates) } +// TestDiscover_SingleAggregation verifies Discover derives all four +// capability-presence flags from ONE aggregation call (reg.List + +// AggregateCapabilities each Times(1)) rather than the four independent +// fan-outs ListTools/ListResources/ListResourceTemplates/ListPrompts would +// cost if called separately. +func TestDiscover_SingleAggregation(t *testing.T) { + t.Parallel() + cfg, m := baseConfig(t) + + backends := []vmcp.Backend{{ID: testBackendID, HealthStatus: vmcp.BackendHealthy}} + m.reg.EXPECT().List(gomock.Any()).Return(backends).Times(1) + m.agg.EXPECT().AggregateCapabilities(gomock.Any(), backends).Return(&aggregator.AggregatedCapabilities{ + Tools: []vmcp.Tool{backendTool("echo")}, + Resources: []vmcp.Resource{{URI: "file://a", BackendID: testBackendID}}, + ResourceTemplates: []vmcp.ResourceTemplate{{URITemplate: "file:///logs/{date}.txt", BackendID: testBackendID}}, + Prompts: []vmcp.Prompt{{Name: "p1", BackendID: testBackendID}}, + RoutingTable: &vmcp.RoutingTable{}, + }, nil).Times(1) + + c, err := New(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = c.Close() }) + + caps, err := c.Discover(context.Background(), nil) + require.NoError(t, err) + assert.Equal(t, DiscoverCapabilities{ + HasTools: true, HasResources: true, HasResourceTemplates: true, HasPrompts: true, + }, caps) +} + +// TestDiscover_EmptyAggregate verifies an empty aggregated view yields every +// flag false, not an error. +func TestDiscover_EmptyAggregate(t *testing.T) { + t.Parallel() + cfg, m := baseConfig(t) + + m.reg.EXPECT().List(gomock.Any()).Return(nil) + m.agg.EXPECT().AggregateCapabilities(gomock.Any(), gomock.Any()).Return(&aggregator.AggregatedCapabilities{}, nil) + + c, err := New(cfg) + require.NoError(t, err) + t.Cleanup(func() { _ = c.Close() }) + + caps, err := c.Discover(context.Background(), nil) + require.NoError(t, err) + assert.Equal(t, DiscoverCapabilities{}, caps) +} + +// TestDiscover_DeniedIdentityHidesTools is the security-critical case: a +// backend advertising a tool must NOT set HasTools for an identity the +// admission seam denies that tool to. Deriving the flag from the raw +// aggregate instead of the admission-filtered set would leak the tool's +// existence to an identity that cannot call it. +func TestDiscover_DeniedIdentityHidesTools(t *testing.T) { + t.Parallel() + _, m := baseConfig(t) + + authorizer := &mockAuthorizer{results: map[string]mockResult{"echo": {authorized: false}}} + c := checkCore(m, newCedarAdmission(authorizer)) + expectAggregationAnyTimes(m, &aggregator.AggregatedCapabilities{ + Tools: []vmcp.Tool{backendTool("echo")}, + RoutingTable: &vmcp.RoutingTable{}, + }) + + caps, err := c.Discover(t.Context(), cedarIdentity()) + require.NoError(t, err) + assert.False(t, caps.HasTools, + "a denied identity must not see HasTools=true even though the backend advertises a tool") +} + func TestListTools_AggregationError(t *testing.T) { t.Parallel() cfg, m := baseConfig(t) diff --git a/pkg/vmcp/server/serve_session_test.go b/pkg/vmcp/server/serve_session_test.go index cb8c596dce..34d8704de9 100644 --- a/pkg/vmcp/server/serve_session_test.go +++ b/pkg/vmcp/server/serve_session_test.go @@ -299,6 +299,10 @@ func (*fakeCore) LookupBackend(context.Context, *auth.Identity, string) (*vmcp.B return nil, vmcp.ErrNotFound } +func (*fakeCore) Discover(context.Context, *auth.Identity) (core.DiscoverCapabilities, error) { + return core.DiscoverCapabilities{}, nil +} + func (*fakeCore) Close() error { return nil } func (*fakeCore) BackendHealth() health.Reporter { return nil } diff --git a/pkg/vmcp/server/serve_test.go b/pkg/vmcp/server/serve_test.go index 0497417820..cdfc7ebd22 100644 --- a/pkg/vmcp/server/serve_test.go +++ b/pkg/vmcp/server/serve_test.go @@ -82,6 +82,9 @@ func (*stubVMCP) ListBackends(context.Context, *auth.Identity, bool) ([]vmcp.Bac func (*stubVMCP) LookupBackend(context.Context, *auth.Identity, string) (*vmcp.Backend, error) { return nil, nil } +func (*stubVMCP) Discover(context.Context, *auth.Identity) (core.DiscoverCapabilities, error) { + return core.DiscoverCapabilities{}, nil +} func (s *stubVMCP) Close() error { s.closed = true; return nil } func (*stubVMCP) BackendHealth() health.Reporter { return nil } func (*stubVMCP) InvalidateCapabilityCache() {} diff --git a/pkg/vmcp/types.go b/pkg/vmcp/types.go index adef88dbe9..95e43f63e6 100644 --- a/pkg/vmcp/types.go +++ b/pkg/vmcp/types.go @@ -557,6 +557,11 @@ type ToolCallResult struct { // This includes progressToken, trace context, and custom backend metadata. // Per MCP specification, this field is optional and may be nil. Meta map[string]any + + // BackendID is the logical backend that served the call, set by the core + // after routing. Empty for a composite tool (no single serving backend). + // Audit-only: never serialized to the client. + BackendID string `json:"-"` } // ResourceContent represents a single resource content item, @@ -585,6 +590,10 @@ type ResourceReadResult struct { // because they return []ResourceContents directly, not a result wrapper. // This field is preserved for future SDK improvements but may be nil. Meta map[string]any + + // BackendID is the logical backend that served the read, set by the core + // after routing. Audit-only: never serialized to the client. + BackendID string `json:"-"` } // PromptMessage represents a single message in a prompt response, @@ -611,6 +620,10 @@ type PromptGetResult struct { // This includes progressToken, trace context, and custom backend metadata. // Per MCP specification, this field is optional and may be nil. Meta map[string]any + + // BackendID is the logical backend that served the get, set by the core + // after routing. Audit-only: never serialized to the client. + BackendID string `json:"-"` } // Completion reference type constants, matching the MCP completion/complete From f8a14861f2e0499a7dabe37adc0ee9f051d2138d Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 23 Jul 2026 20:32:24 +0200 Subject: [PATCH 2/9] Serve MCP 2026-07-28 Modern stateless requests through vMCP Serve Modern (2026-07-28) stateless requests by bypassing the SDK Serve/ session layer and dispatching straight to the already-stateless vMCP core. classifyingHandler routes a well-formed Modern request to a hand-rolled dispatcher; Legacy and malformed-Modern requests behave exactly as before. The dispatcher hand-rolls the Modern wire envelope (resultType, _meta serverInfo, Cacheable with cacheScope "private" for identity-scoped results) because the imported go-sdk v1.6.1 has no Modern shapes. It serves tools/resources/prompts list + call/read/get, server/discover, ping and completion/complete; notifications -> 202, unknown -> 404/-32601, malformed arguments (and a missing completion argument name) -> 400/-32602. Because it bypasses the SDK server, it re-homes that server's pre-dispatch authorization gate: Check* before dispatch and a dispatch-time ErrAuthorizationFailed both map to HTTP 403 + the matching DenyMessage (so a denial audits as "denied"), fail-open on infra errors. The incoming request context is passed through unmodified so forwarded-header backend auth works. Successful tools/call, resources/read and prompts/get label the audit backend_name (success path only; see comment). server/discover returns post-admission capability flags via core.Discover (no descriptor arrays). Co-Authored-By: Claude Opus 4.8 --- pkg/vmcp/server/classification.go | 42 +- pkg/vmcp/server/classification_test.go | 83 +- pkg/vmcp/server/modern_dispatch.go | 427 +++++++++ pkg/vmcp/server/modern_dispatch_test.go | 875 ++++++++++++++++++ pkg/vmcp/server/modern_envelope.go | 570 ++++++++++++ pkg/vmcp/server/modern_envelope_test.go | 719 ++++++++++++++ pkg/vmcp/server/server.go | 14 +- ...management_realbackend_integration_test.go | 13 +- pkg/vmcp/server/telemetry_integration_test.go | 6 +- 9 files changed, 2706 insertions(+), 43 deletions(-) create mode 100644 pkg/vmcp/server/modern_dispatch.go create mode 100644 pkg/vmcp/server/modern_dispatch_test.go create mode 100644 pkg/vmcp/server/modern_envelope.go create mode 100644 pkg/vmcp/server/modern_envelope_test.go diff --git a/pkg/vmcp/server/classification.go b/pkg/vmcp/server/classification.go index e8c19d3414..daebaf49e0 100644 --- a/pkg/vmcp/server/classification.go +++ b/pkg/vmcp/server/classification.go @@ -9,26 +9,25 @@ import ( mcpparser "github.com/stacklok/toolhive/pkg/mcp" ) -// classificationMiddleware classifies a parsed MCP request as Legacy -// (2025-11-25) or Modern (2026-07-28) at the decode seam and rejects a -// malformed Modern request with the correct JSON-RPC error before it reaches -// dispatch. The classified mcpparser.Revision is used only to gate -// ValidateHeaderConsistency (see below); it is not otherwise stashed in -// context or anywhere else, since no downstream consumer reads it yet. -// Legacy traffic and well-formed Modern requests both fall through to the -// same next handler unchanged — Modern-specific dispatch is a later phase -// (toolhive issue #5756). +// 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 routes a +// well-formed Modern request to dispatchModern instead of the SDK. Modern +// dispatch is unconditional: a well-formed Modern request always reaches +// dispatchModern. Legacy traffic always falls through to next unchanged. // // ValidateHeaderConsistency (Mcp-Method/Mcp-Name) only applies to Modern // requests: a Legacy request carrying a stray Mcp-Method/Mcp-Name header // (e.g. from a misbehaving proxy) must not be rejected for it, since Legacy // clients never send these headers and have no obligation to omit them. // -// This middleware makes no authentication/authorization decision and confers -// no elevated trust on requests that pass it — it only validates protocol -// shape. It must run after ParsingMiddleware (so GetParsedMCPRequest is -// populated) and is expected to run after any auth middleware in the chain. -func classificationMiddleware(next http.Handler) http.Handler { +// This handler makes no authentication/authorization decision of its own and +// confers no elevated trust on requests that pass it — it only validates +// protocol shape and routes. It must run after ParsingMiddleware (so +// GetParsedMCPRequest is populated) and is expected to run after any auth +// middleware in the chain, so a Modern dispatch that gets gated 403 is still +// audited as "denied". +func (s *Server) classifyingHandler(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { parsed := mcpparser.GetParsedMCPRequest(r.Context()) if parsed == nil { @@ -43,13 +42,16 @@ func classificationMiddleware(next http.Handler) http.Handler { return } - if rev == mcpparser.RevisionModern { - if err := mcpparser.ValidateHeaderConsistency(parsed); err != nil { - mcpparser.WriteClassificationError(w, parsed.ID, err) - return - } + if rev != mcpparser.RevisionModern { + next.ServeHTTP(w, r) + return + } + + if err := mcpparser.ValidateHeaderConsistency(parsed); err != nil { + mcpparser.WriteClassificationError(w, parsed.ID, err) + return } - next.ServeHTTP(w, r) + s.dispatchModern(w, r, parsed) }) } diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index 215219fac4..f530e0c95a 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -15,6 +15,7 @@ import ( "github.com/stretchr/testify/require" mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" ) // Reserved Modern _meta keys, mirrored from pkg/mcp/revision.go's unexported @@ -31,11 +32,25 @@ func sentinelEncode(v string) string { type classificationErrorBody struct { Error struct { - Code int64 `json:"code"` + Code int64 `json:"code"` + Message string `json:"message"` } `json:"error"` } -func TestClassificationMiddleware(t *testing.T) { +// classifyingHandlerTestServer builds a minimal *Server for driving +// classifyingHandler in isolation, carrying only the field the handler reads +// beyond config scalars: the core a well-formed Modern request dispatches to. +func classifyingHandlerTestServer() *Server { + return &Server{ + config: &Config{ + Name: testServerName, + Version: testServerVersion, + }, + core: &modernFakeCore{tools: []vmcp.Tool{{Name: "echo", InputSchema: map[string]any{"type": "object"}}}}, + } +} + +func TestClassifyingHandler(t *testing.T) { t.Parallel() tests := []struct { @@ -43,6 +58,7 @@ func TestClassificationMiddleware(t *testing.T) { parsed *mcpparser.ParsedMCPRequest protocolHeader string wantPassthrough bool + wantDispatched bool wantCode int64 }{ { @@ -57,17 +73,42 @@ func TestClassificationMiddleware(t *testing.T) { }, wantPassthrough: true, }, + { + // A non-Modern MCP-Protocol-Version header, with no reserved _meta key, + // is not a Modern signal (ClassifyRevision requires an exact match on + // MCPVersionModern): this must still reach next, never dispatchModern, + // now that Modern dispatch is unconditional for well-formed requests. + name: "legacy request with an old protocol version header still passes through", + parsed: &mcpparser.ParsedMCPRequest{ + Method: "tools/call", + }, + protocolHeader: "2025-11-25", + wantPassthrough: true, + }, { // 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. - name: "modern header and complete meta pass through", + // case only needs Mcp-Method (required on every Modern request) to pass + // ValidateHeaderConsistency; a well-formed Modern request then dispatches + // to the core unconditionally rather than falling through to next. + name: "well-formed modern request dispatches to the core", + parsed: wellFormedModernToolsList(), + protocolHeader: mcpparser.MCPVersionModern, + wantDispatched: true, + }, + { + // initialize is forced Legacy unconditionally (ClassifyRevision), even + // with a full spoofed Modern signal on both header and _meta -- mirrors + // revision_test.go's "legacy: initialize wins over spoofed modern meta + // and header" at the classifyingHandler boundary, so a future change + // ahead of the ClassifyRevision call can't silently route this to + // dispatchModern. + name: "initialize with spoofed modern signal still passes through", parsed: &mcpparser.ParsedMCPRequest{ - Method: "tools/list", + Method: "initialize", Meta: map[string]any{ metaKeyProtocolVersion: mcpparser.MCPVersionModern, metaKeyClientCapabilities: map[string]any{}, }, - MCPMethodHeader: "tools/list", }, protocolHeader: mcpparser.MCPVersionModern, wantPassthrough: true, @@ -190,17 +231,43 @@ func TestClassificationMiddleware(t *testing.T) { }) rec := httptest.NewRecorder() - classificationMiddleware(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") return } - assert.False(t, nextCalled, "expected classification to short-circuit before next") + + if tt.wantDispatched { + var body map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "expected a Modern result envelope, got %v", body) + assert.Equal(t, "complete", result["resultType"]) + return + } + var body classificationErrorBody require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) assert.Equal(t, tt.wantCode, body.Error.Code) }) } } + +// wellFormedModernToolsList returns a well-formed Modern tools/list request: +// tools/list is deliberately not in the Mcp-Name-required set, so only +// Mcp-Method (required on every Modern request) is needed for it to pass +// ValidateHeaderConsistency. +func wellFormedModernToolsList() *mcpparser.ParsedMCPRequest { + return &mcpparser.ParsedMCPRequest{ + Method: "tools/list", + ID: "1", + IsRequest: true, + Meta: map[string]any{ + metaKeyProtocolVersion: mcpparser.MCPVersionModern, + metaKeyClientCapabilities: map[string]any{}, + }, + MCPMethodHeader: "tools/list", + } +} diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go new file mode 100644 index 0000000000..66e2adac01 --- /dev/null +++ b/pkg/vmcp/server/modern_dispatch.go @@ -0,0 +1,427 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "context" + "encoding/json" + "errors" + "log/slog" + "net/http" + + "github.com/stacklok/toolhive/pkg/audit" + "github.com/stacklok/toolhive/pkg/auth" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// Standard JSON-RPC 2.0 reserved error codes (spec-fixed, never change). Kept +// as a local block, used by both this file and modern_envelope.go's +// writeModernError status mapping, rather than imported from mcpcompat (which +// defines equivalents like mcp.METHOD_NOT_FOUND): mcpcompat is the SDK's +// wire-protocol vocabulary, while the Modern vMCP layer already sources its +// own codes from two other places -- mcpparser.JSONRPCCodeDenied (403, shared +// with the Legacy call gate) and the classifier's app-space -3202x codes. +// Pulling these four from mcpcompat too would split one small, unchanging set +// of constants across three packages for no benefit. +const ( + jsonRPCCodeInvalidRequest = -32600 + jsonRPCCodeMethodNotFound = -32601 + jsonRPCCodeInvalidParams = -32602 + jsonRPCCodeInternalError = -32603 +) + +// dispatchModern serves a single MCP 2026-07-28 ("Modern") stateless request +// by dispatching directly to the stateless vMCP core, bypassing the SDK +// Serve/session layer entirely. classifyingHandler routes here for every +// well-formed Modern request. +// +// Because this path bypasses the SDK server, it re-homes the SDK's +// pre-dispatch authorization gate itself (see the per-method blocks below) -- +// mirroring authzCallGate exactly, including its fail-open posture on +// non-authorization Check* errors. Do not add a case here without also +// deciding its gating: an ungated write would let a Cedar-denied call reach a +// backend. +func (s *Server) dispatchModern(w http.ResponseWriter, r *http.Request, parsed *mcpparser.ParsedMCPRequest) { + // A notification (no id) MUST get 202 with no body and no dispatch, per the + // Streamable HTTP spec's handling of a POST body containing only + // responses/notifications. parser.go's real parse path sets IsRequest true + // for every decoded jsonrpc2.Request -- calls AND notifications alike -- so + // IsRequest cannot distinguish them; absent id (nil) is the actual + // notification signal (parseMCPRequest leaves ParsedMCPRequest.ID nil when + // the JSON-RPC id is absent). + if parsed.ID == nil { + w.WriteHeader(http.StatusAccepted) + return + } + + // ponytail: defensive/unreachable today -- ParsingMiddleware rejects a + // JSON-RPC batch (leading '[') with HTTP 400 / -32600 before a + // ParsedMCPRequest is ever built (parser.go's IsBatchRequest check, ~line + // 119), so dispatchModern never sees one and IsBatch is hardcoded false + // (parser.go ~line 61-65). This also closes the batch blind spot + // call_gate.go used to document. Do not build batch parsing here; this + // guard is just a backstop if the parser ever stops rejecting batches + // upstream. + if parsed.IsBatch { + writeModernError(w, parsed.ID, jsonRPCCodeInvalidRequest, "batch requests are not supported") + return + } + + ctx := r.Context() + // Sanctioned transport-boundary identity read (matches authzCallGate and + // every Serve-path handler). ctx itself is passed unmodified into every + // core.* call below: the stateless backend client reads forwarded headers + // off this exact context per call, so detaching or wrapping it would + // silently break forwarded-header backend auth. + identity, _ := auth.IdentityFromContext(ctx) + + switch parsed.Method { + case "tools/list": + s.dispatchModernToolsList(ctx, w, parsed, identity) + case "resources/list": + s.dispatchModernResourcesList(ctx, w, parsed, identity) + case "resources/templates/list": + s.dispatchModernResourceTemplatesList(ctx, w, parsed, identity) + case "prompts/list": + s.dispatchModernPromptsList(ctx, w, parsed, identity) + case "server/discover": + s.dispatchModernDiscover(ctx, w, parsed, identity) + case "tools/call": + s.dispatchModernToolCall(ctx, w, parsed, identity) + case "resources/read": + s.dispatchModernResourceRead(ctx, w, parsed, identity) + case "prompts/get": + s.dispatchModernPromptGet(ctx, w, parsed, identity) + case "completion/complete": + s.dispatchModernComplete(ctx, w, parsed, identity) + case "ping": + // ping is deliberately ungated (unauthenticated liveness, same bucket + // as initialize -- no Check*) and carries NEITHER resultType NOR + // _meta.serverInfo on the wire: the SDK's ping handler returns + // emptyResult, and both annotateServerInfo and setCompleteResultType + // early-return/no-op for it (go-sdk server.go:1929-1945,1992). Do not + // route this through the envelope builders above -- a bare {} is the + // correct, spec-matching result. + writeModernResult(w, parsed.ID, struct{}{}) + default: + writeModernError(w, parsed.ID, jsonRPCCodeMethodNotFound, "method not found") + } +} + +// The four list-dispatch helpers below (tools/list, resources/list, +// resources/templates/list, prompts/list) always return the full +// admission-filtered set from the matching core.List* and never set the +// envelope's nextCursor (it's omitempty) -- client-facing cursor pagination +// is unimplemented, and any cursor a Modern client sends is ignored. This is +// unrelated to the aggregator's UPSTREAM cursor-following for internal +// discovery (#5851); that's a different layer. +func (s *Server) dispatchModernToolsList( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + tools, err := s.core.ListTools(ctx, identity) + if err != nil { + writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + return + } + result, err := newModernToolsList(tools, s.config.Name, s.config.Version) + if err != nil { + writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + return + } + writeModernResult(w, parsed.ID, result) +} + +func (s *Server) dispatchModernResourcesList( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + resources, err := s.core.ListResources(ctx, identity) + if err != nil { + writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + return + } + writeModernResult(w, parsed.ID, newModernResourcesList(resources, s.config.Name, s.config.Version)) +} + +func (s *Server) dispatchModernResourceTemplatesList( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + templates, err := s.core.ListResourceTemplates(ctx, identity) + if err != nil { + writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + return + } + writeModernResult(w, parsed.ID, newModernResourceTemplatesList(templates, s.config.Name, s.config.Version)) +} + +func (s *Server) dispatchModernPromptsList( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + prompts, err := s.core.ListPrompts(ctx, identity) + if err != nil { + writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + return + } + writeModernResult(w, parsed.ID, newModernPromptsList(prompts, s.config.Name, s.config.Version)) +} + +// dispatchModernDiscover serves server/discover, Modern's replacement for +// initialize+capability negotiation, as a post-admission capability-flags +// envelope: it calls core.Discover, which applies the same admission-filtered +// code paths the four list verbs use and collapses each to presence/absence, +// returning NO descriptor arrays -- so the response reflects only what this +// identity may reach (ListBackends's filterUnauthorized=true is the existing +// post-admission-presence precedent, core_vmcp.go:446). Like the list verbs, +// this is ungated: there is no separate Check* for discover, since it leaks +// no more than tools/list already does. +// +// This runs ONE backend fan-out per call via core.Discover (aggregatedView is +// uncached, so calling ListTools/ListResources/ListResourceTemplates/ +// ListPrompts independently here used to cost four -- and those four weren't +// even a consistent snapshot of the aggregated view). A single fan-out per +// request is fine for now, but a probe the spec expects to be cheap across +// requests too. ponytail: no cross-request cache; add a short-TTL +// per-identity capability cache only if profiling shows the per-request +// fan-out cost matters (#5761, tracked separately, not blocking here). +func (s *Server) dispatchModernDiscover( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + caps, err := s.core.Discover(ctx, identity) + if err != nil { + writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + return + } + result := newModernDiscover( + caps.HasTools, caps.HasResources, caps.HasResourceTemplates, caps.HasPrompts, + s.config.Name, s.config.Version, + ) + writeModernResult(w, parsed.ID, result) +} + +// dispatchModernToolCall re-homes authzCallGate's tools/call branch plus the +// post-dispatch TOCTOU reclassification (see writeModernDispatchError). +func (s *Server) dispatchModernToolCall( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + if hasNonObjectArguments(parsed.Params) { + writeModernError(w, parsed.ID, jsonRPCCodeInvalidParams, "arguments must be an object") + return + } + if s.authzGateEnabled && gateDenied(ctx, parsed.Method, + s.core.CheckToolCall(ctx, identity, parsed.ResourceID, parsed.Arguments)) { + writeModernDenied(w, parsed.ID, vmcp.DenyMessageToolCall) + return + } + result, err := s.core.CallTool(ctx, identity, parsed.ResourceID, parsed.Arguments, parsed.Meta) + if err != nil { + writeModernDispatchError(w, parsed.ID, vmcp.DenyMessageToolCall, err) + return + } + // Label the audit backend on the success path only. The stateless dispatcher + // cannot pre-resolve the backend (routing is core-internal), so unlike the + // Legacy handlers -- which set the label before the call and thus keep it on + // backend-call failures -- a Modern backend-call failure audits without + // backend_name. Accepted: the event still records the tool and the outcome. + // (Same applies to resources/read and prompts/get below.) + if result.BackendID != "" { + if bi, ok := audit.BackendInfoFromContext(ctx); ok && bi != nil { + bi.BackendName = s.backendDisplayName(ctx, result.BackendID) + } + } + writeModernResult(w, parsed.ID, newModernCallToolResult(result, s.config.Name, s.config.Version)) +} + +// dispatchModernResourceRead re-homes authzCallGate's resources/read branch +// plus the post-dispatch TOCTOU reclassification (see writeModernDispatchError). +func (s *Server) dispatchModernResourceRead( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + if s.authzGateEnabled && gateDenied(ctx, parsed.Method, + s.core.CheckResourceRead(ctx, identity, parsed.ResourceID)) { + writeModernDenied(w, parsed.ID, vmcp.DenyMessageResourceRead) + return + } + result, err := s.core.ReadResource(ctx, identity, parsed.ResourceID) + if err != nil { + writeModernDispatchError(w, parsed.ID, vmcp.DenyMessageResourceRead, err) + return + } + if result.BackendID != "" { + if bi, ok := audit.BackendInfoFromContext(ctx); ok && bi != nil { + bi.BackendName = s.backendDisplayName(ctx, result.BackendID) + } + } + writeModernResult(w, parsed.ID, newModernReadResourceResult(result, s.config.Name, s.config.Version)) +} + +// dispatchModernPromptGet re-homes authzCallGate's prompts/get branch plus the +// post-dispatch TOCTOU reclassification (see writeModernDispatchError). +func (s *Server) dispatchModernPromptGet( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + // hasNonObjectArguments only checks object-ness, not per-value typing: the + // SDK's GetPromptParams.Arguments is map[string]string, so it also rejects + // a non-string argument VALUE (e.g. {"x":123}) at decode. Modern accepts + // that shape -- narrower parity (object-shape only), not a behavior gap + // worth closing here. + if hasNonObjectArguments(parsed.Params) { + writeModernError(w, parsed.ID, jsonRPCCodeInvalidParams, "arguments must be an object") + return + } + if s.authzGateEnabled && gateDenied(ctx, parsed.Method, + s.core.CheckPromptGet(ctx, identity, parsed.ResourceID)) { + writeModernDenied(w, parsed.ID, vmcp.DenyMessagePromptGet) + return + } + result, err := s.core.GetPrompt(ctx, identity, parsed.ResourceID, parsed.Arguments) + if err != nil { + writeModernDispatchError(w, parsed.ID, vmcp.DenyMessagePromptGet, err) + return + } + if result.BackendID != "" { + if bi, ok := audit.BackendInfoFromContext(ctx); ok && bi != nil { + bi.BackendName = s.backendDisplayName(ctx, result.BackendID) + } + } + writeModernResult(w, parsed.ID, newModernGetPromptResult(result, s.config.Name, s.config.Version)) +} + +// modernCompleteWireParams is the completion/complete request params, decoded +// directly from parsed.Params. It mirrors go-sdk's CompleteParams/ +// CompleteReference/CompleteParamsArgument/CompleteContext field-for-field +// (protocol.go:577-648 in go-sdk@v1.7.0-pre.3) rather than reusing an SDK +// type: mcp-go v1.6.1 (ToolHive's import) predates the Modern completion +// shapes, so there is nothing to reuse. It stays local rather than adding +// JSON tags to vmcp.CompletionRef -- the domain type intentionally carries no +// wire coupling (anti-pattern #5, no mcp-go types crossing the core boundary). +type modernCompleteWireParams struct { + Ref *struct { + Type string `json:"type"` + Name string `json:"name,omitempty"` + URI string `json:"uri,omitempty"` + } `json:"ref"` + Argument struct { + Name string `json:"name"` + Value string `json:"value"` + } `json:"argument"` + Context *struct { + Arguments map[string]string `json:"arguments,omitempty"` + } `json:"context,omitempty"` +} + +// dispatchModernComplete serves completion/complete. Unlike tools/call, +// resources/read, and prompts/get, there is no pre-dispatch Check* gate here +// -- call_gate.go documents this as a conscious choice: core.Complete +// authorizes the underlying prompt/resource ref at dispatch (the same +// get/read decision GetPrompt/ReadResource enforce), so gating on the wire +// would just duplicate that check ahead of an admission decision that isn't +// argument-conditional the way the gate's fast path assumes. An admission +// denial from core.Complete still reclassifies to 403 via +// writeModernDispatchError, exactly like the three gated verbs. +// +// This handler does not label the audit BackendInfo the way the three gated +// verbs above do: the Legacy coreCompletionHandler never set backend_name +// either, so completion is a pre-existing gap on both paths, not something +// introduced here. +func (s *Server) dispatchModernComplete( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + var params modernCompleteWireParams + if err := json.Unmarshal(parsed.Params, ¶ms); err != nil || params.Ref == nil || params.Ref.Type == "" { + writeModernError(w, parsed.ID, jsonRPCCodeInvalidParams, "invalid completion/complete params: missing ref") + return + } + if params.Argument.Name == "" { + writeModernError(w, parsed.ID, jsonRPCCodeInvalidParams, "invalid completion/complete params: missing argument.name") + return + } + ref := vmcp.CompletionRef{Type: params.Ref.Type, Name: params.Ref.Name, URI: params.Ref.URI} + + var contextArgs map[string]string + if params.Context != nil { + contextArgs = params.Context.Arguments + } + + result, err := s.core.Complete(ctx, identity, ref, params.Argument.Name, params.Argument.Value, contextArgs) + if err != nil { + writeModernDispatchError(w, parsed.ID, completionDenyMessage(ref.Type), err) + return + } + writeModernResult(w, parsed.ID, newModernComplete(result, s.config.Name, s.config.Version)) +} + +// hasNonObjectArguments reports whether parsed.Params carries an "arguments" +// field that is present but NOT a JSON object (e.g. a string or array). +// +// The parser (handleNamedResourceMethod, parser.go:307) type-asserts +// paramsMap["arguments"].(map[string]interface{}) and silently drops the +// value to nil on a mismatch -- indistinguishable from "arguments absent" by +// the time ParsedMCPRequest.Arguments is built. The SDK path also rejects +// this shape before authz/the core is ever reached (coreToolHandler +// shape-checks req.Params.Arguments in serve_handlers.go; prompts/get gets +// the same pre-dispatch rejection for free because mcpcompat's +// GetPromptParams.Arguments is a concrete map[string]string, so a non-object +// value fails JSON decode before the handler runs) -- this function matches +// that TIMING, not the SDK's wire shape: the SDK's tools/call rejection +// surfaces as a 200 IsError tool result (conversion path), whereas this is a +// genuine JSON-RPC -32602, consistent with Modern's other protocol-level +// rejections (-32600/-32601). Modern must reject the same shape here, on the +// raw params, before that type information is lost -- otherwise a non-object +// arguments value silently authorizes and dispatches as a no-args call, +// diverging from the SDK path and potentially changing an +// argument-conditional authz decision. An absent or explicit-null "arguments" +// is a legitimate no-args call and is not rejected. +func hasNonObjectArguments(params json.RawMessage) bool { + var raw struct { + Arguments json.RawMessage `json:"arguments"` + } + if err := json.Unmarshal(params, &raw); err != nil || raw.Arguments == nil { + return false + } + var obj map[string]any + return json.Unmarshal(raw.Arguments, &obj) != nil +} + +// gateDenied runs the PRE-dispatch admission classification for a gated +// method's Check* result, mirroring authzCallGate exactly: only an +// errors.Is(checkErr, vmcp.ErrAuthorizationFailed) denial returns true. Any +// other error is infrastructure (aggregation/backend plumbing), so the gate +// fails OPEN -- it logs and admits, rather than converting an authorizer +// outage into a false 403. This WARN is the only operational signal of that +// outage admitting traffic; do not remove it. +func gateDenied(ctx context.Context, method string, checkErr error) bool { + if checkErr == nil { + return false + } + if errors.Is(checkErr, vmcp.ErrAuthorizationFailed) { + return true + } + slog.WarnContext(ctx, "vmcp authz gate: non-authorization error, admitting request", + "method", method, "error", checkErr) + return false +} + +// writeModernDispatchError classifies a POST-dispatch error from +// CallTool/ReadResource/GetPrompt. Check* and the real call each re-aggregate +// independently (documented "aggregates twice" on CheckToolCall), so a +// concurrent backend health flip, cache refresh, or annotation change +// (TOCTOU) can have Check* allow and the call itself deny. That denial MUST +// still surface as 403 + denyMsg -- the same as the pre-dispatch gate -- so +// the audit middleware logs it as "denied" rather than "failure"; it is +// therefore tested FIRST, before falling through to the generic internal +// error. +// +// 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 +// (serve_handlers.go), already surface the raw error text for a non-coded, +// 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) { + if errors.Is(err, vmcp.ErrAuthorizationFailed) { + writeModernDenied(w, id, denyMsg) + 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 new file mode 100644 index 0000000000..f659eb559f --- /dev/null +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -0,0 +1,875 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive/pkg/audit" + "github.com/stacklok/toolhive/pkg/auth" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/core" +) + +// modernFakeCore is a core.VMCP whose only exercised methods are the ones +// dispatchModern can reach. The embedded nil interface satisfies the rest and +// panics if dispatch ever calls a method it should not (e.g. a list call for a +// notification/batch/unknown-method request that must short-circuit before +// reaching the core at all). +type modernFakeCore struct { + core.VMCP + + tools []vmcp.Tool + resources []vmcp.Resource + templates []vmcp.ResourceTemplate + prompts []vmcp.Prompt + + discoverCaps core.DiscoverCapabilities + discoverErr error + + checkToolErr, checkResourceErr, checkPromptErr error + callToolErr, readResourceErr, getPromptErr error + completeErr error + listErr error // returned by every List* method + + checkCalled, callCalled bool + + // backendID is stamped onto CallTool/ReadResource/GetPrompt's returned + // result, mirroring the real core's post-routing BackendID assignment + // (core_calls.go). Empty by default, matching a fake that never resolves + // a backend. + backendID string + + // gotCtx captures the context handed to CallTool, so a test can assert a + // value set on the inbound request survives into the core call unmodified. + gotCtx context.Context +} + +func (f *modernFakeCore) ListTools(context.Context, *auth.Identity) ([]vmcp.Tool, error) { + return f.tools, f.listErr +} + +func (f *modernFakeCore) ListResources(context.Context, *auth.Identity) ([]vmcp.Resource, error) { + return f.resources, nil +} + +func (f *modernFakeCore) ListResourceTemplates(context.Context, *auth.Identity) ([]vmcp.ResourceTemplate, error) { + return f.templates, nil +} + +func (f *modernFakeCore) ListPrompts(context.Context, *auth.Identity) ([]vmcp.Prompt, error) { + return f.prompts, nil +} + +func (f *modernFakeCore) Discover(context.Context, *auth.Identity) (core.DiscoverCapabilities, error) { + return f.discoverCaps, f.discoverErr +} + +func (f *modernFakeCore) CheckToolCall(context.Context, *auth.Identity, string, map[string]any) error { + f.checkCalled = true + return f.checkToolErr +} + +func (f *modernFakeCore) CheckResourceRead(context.Context, *auth.Identity, string) error { + f.checkCalled = true + return f.checkResourceErr +} + +func (f *modernFakeCore) CheckPromptGet(context.Context, *auth.Identity, string) error { + f.checkCalled = true + return f.checkPromptErr +} + +func (f *modernFakeCore) CallTool( + ctx context.Context, _ *auth.Identity, _ string, _ map[string]any, _ map[string]any, +) (*vmcp.ToolCallResult, error) { + f.callCalled = true + f.gotCtx = ctx + if f.callToolErr != nil { + return nil, f.callToolErr + } + return &vmcp.ToolCallResult{Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "ok"}}, BackendID: f.backendID}, nil +} + +func (f *modernFakeCore) ReadResource( + ctx context.Context, _ *auth.Identity, uri string, +) (*vmcp.ResourceReadResult, error) { + f.callCalled = true + f.gotCtx = ctx + if f.readResourceErr != nil { + return nil, f.readResourceErr + } + return &vmcp.ResourceReadResult{Contents: []vmcp.ResourceContent{{URI: uri, Text: "body"}}, BackendID: f.backendID}, nil +} + +func (f *modernFakeCore) GetPrompt( + ctx context.Context, _ *auth.Identity, _ string, _ map[string]any, +) (*vmcp.PromptGetResult, error) { + f.callCalled = true + f.gotCtx = ctx + if f.getPromptErr != nil { + return nil, f.getPromptErr + } + return &vmcp.PromptGetResult{ + Messages: []vmcp.PromptMessage{{Role: "user", Content: vmcp.Content{Type: vmcp.ContentTypeText, Text: "hi"}}}, + BackendID: f.backendID, + }, nil +} + +func (f *modernFakeCore) Complete( + ctx context.Context, _ *auth.Identity, _ vmcp.CompletionRef, _, _ string, _ map[string]string, +) (*vmcp.CompletionResult, error) { + f.callCalled = true + f.gotCtx = ctx + if f.completeErr != nil { + return nil, f.completeErr + } + return &vmcp.CompletionResult{Values: []string{"opt1", "opt2"}, Total: 2}, nil +} + +// dispatchModernTest builds a Server over fakeCore and drives dispatchModern +// directly (Step 3 wires this into the real handler chain), returning the +// decoded JSON-RPC envelope and the recorder for status/header assertions. +func dispatchModernTest( + reqCtx context.Context, t *testing.T, fakeCore *modernFakeCore, authzEnabled bool, parsed *mcpparser.ParsedMCPRequest, +) (*httptest.ResponseRecorder, map[string]any) { + t.Helper() + + s := &Server{ + config: &Config{Name: testServerName, Version: testServerVersion}, + core: fakeCore, + authzGateEnabled: authzEnabled, + } + + req := httptest.NewRequest(http.MethodPost, "/mcp", nil).WithContext(reqCtx) + rec := httptest.NewRecorder() + + s.dispatchModern(rec, req, parsed) + + var body map[string]any + if rec.Body.Len() > 0 { + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + } + return rec, body +} + +// TestDispatchModern_ControlFlow covers the guards that must short-circuit +// before any core call: a batch gets -32600, and an unrecognized method gets +// -32601. None of these may reach the core (modernFakeCore's embedded nil +// core.VMCP panics if they did). The notification guard is NOT in this table +// -- see TestDispatchModern_NotificationRealParser: a hand-built +// ParsedMCPRequest{IsRequest:false} would not have caught the regression +// where the real parser always sets IsRequest true. +func TestDispatchModern_ControlFlow(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parsed *mcpparser.ParsedMCPRequest + wantStatus int + wantCode float64 + }{ + { + name: "batch request returns -32600 invalid request", + parsed: &mcpparser.ParsedMCPRequest{Method: "tools/call", IsRequest: true, IsBatch: true, ID: "1"}, + wantStatus: http.StatusOK, + wantCode: -32600, + }, + { + name: "unknown method returns -32601 method not found", + parsed: &mcpparser.ParsedMCPRequest{Method: "roots/list", IsRequest: true, ID: "1"}, + wantStatus: http.StatusNotFound, + wantCode: -32601, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec, body := dispatchModernTest(t.Context(), t, &modernFakeCore{}, true, tt.parsed) + + assert.Equal(t, tt.wantStatus, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC error envelope") + assert.Equal(t, tt.wantCode, errObj["code"]) + }) + } +} + +// TestDispatchModern_NotificationRealParser drives the REAL pkg/mcp parser +// (ParsingMiddleware, the same middleware installed in front of dispatchModern +// in production) over a genuine no-id JSON-RPC notification body, rather than +// a hand-built ParsedMCPRequest. This matters because parser.go's real parse +// path (parseMCPRequest) always sets IsRequest:true for a decoded +// jsonrpc2.Request -- calls AND notifications alike -- so `!parsed.IsRequest` +// never fires for a real notification; only an absent id does. A fabricated +// ParsedMCPRequest{IsRequest:false} would pass either check and silently mask +// that bug, which is exactly what happened here. +func TestDispatchModern_NotificationRealParser(t *testing.T) { + t.Parallel() + + body := []byte(`{"jsonrpc":"2.0","method":"notifications/progress","params":{}}`) + req := httptest.NewRequest(http.MethodPost, "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + + var parsed *mcpparser.ParsedMCPRequest + handler := mcpparser.ParsingMiddleware(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + parsed = mcpparser.GetParsedMCPRequest(r.Context()) + })) + handler.ServeHTTP(httptest.NewRecorder(), req) + + require.NotNil(t, parsed, "the real parser must produce a ParsedMCPRequest for a notification body") + require.True(t, parsed.IsRequest, "the real parser sets IsRequest true even for a notification") + require.Nil(t, parsed.ID, "a notification has no JSON-RPC id") + + fc := &modernFakeCore{} + rec, _ := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusAccepted, rec.Code) + assert.Empty(t, rec.Body.Bytes()) + assert.False(t, fc.callCalled, "a notification must not reach the core") +} + +// TestDispatchModern_PingRealParser drives ping through the REAL +// ParsingMiddleware + classifyingHandler chain with genuine Modern signaling +// (MCP-Protocol-Version header, Mcp-Method header, body _meta) rather than a +// hand-built ParsedMCPRequest -- guarding against the ping case regressing +// back to the unrecognized-method default (-32601/404), which a fabricated +// request would not catch. ping is deliberately absent from +// nameRequiredMethods, so no Mcp-Name header is sent. +func TestDispatchModern_PingRealParser(t *testing.T) { + t.Parallel() + + body := []byte(`{ + "jsonrpc": "2.0", + "id": 1, + "method": "ping", + "params": { + "_meta": { + "io.modelcontextprotocol/protocolVersion": "2026-07-28", + "io.modelcontextprotocol/clientCapabilities": {} + } + } + }`) + req := httptest.NewRequest(http.MethodPost, "/mcp", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("MCP-Protocol-Version", mcpparser.MCPVersionModern) + req.Header.Set("Mcp-Method", "ping") + + s := classifyingHandlerTestServer() + nextCalled := false + next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { nextCalled = true }) + + rec := httptest.NewRecorder() + mcpparser.ParsingMiddleware(s.classifyingHandler(next)).ServeHTTP(rec, req) + + require.False(t, nextCalled, "a well-formed Modern ping must dispatch, not fall through to the SDK") + require.Equal(t, http.StatusOK, rec.Code, "must not be the 404 a mis-routed -32601 would produce") + require.JSONEq(t, `{"jsonrpc":"2.0","id":1,"result":{}}`, rec.Body.String()) + + fc, ok := s.core.(*modernFakeCore) + require.True(t, ok) + assert.False(t, fc.callCalled, "ping must not reach the core") +} + +// TestDispatchModern_MethodRouting spot-checks that each method routes to the +// matching core call and produces an envelope with the expected resultType +// and top-level result key -- deep envelope shape (cacheable, serverInfo, +// empty-collection marshalling) is already covered by Step 1's +// modern_envelope_test.go. ping is the one exception: it wants a bare {} +// result (see the case's doc comment in modern_dispatch.go), so it opts out +// of the resultType/wantKey assertions via wantBareResult. +func TestDispatchModern_MethodRouting(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + parsed *mcpparser.ParsedMCPRequest + fakeCore *modernFakeCore + wantKey string // key expected inside result + wantBareResult bool // ping: result == {}, no resultType/_meta + }{ + { + name: "tools/list", + parsed: &mcpparser.ParsedMCPRequest{Method: "tools/list", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{tools: []vmcp.Tool{{Name: "echo", InputSchema: map[string]any{"type": "object"}}}}, + wantKey: "tools", + }, + { + name: "resources/list", + parsed: &mcpparser.ParsedMCPRequest{Method: "resources/list", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{resources: []vmcp.Resource{{Name: "info", URI: "embedded:info"}}}, + wantKey: "resources", + }, + { + name: "resources/templates/list", + parsed: &mcpparser.ParsedMCPRequest{Method: "resources/templates/list", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{ + templates: []vmcp.ResourceTemplate{{Name: "logs", URITemplate: "file:///{date}.txt"}}, + }, + wantKey: "resourceTemplates", + }, + { + name: "prompts/list", + parsed: &mcpparser.ParsedMCPRequest{Method: "prompts/list", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{prompts: []vmcp.Prompt{{Name: "review"}}}, + wantKey: "prompts", + }, + { + name: "server/discover", + parsed: &mcpparser.ParsedMCPRequest{Method: "server/discover", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{tools: []vmcp.Tool{{Name: "echo", InputSchema: map[string]any{"type": "object"}}}}, + wantKey: "capabilities", + }, + { + name: "tools/call", + parsed: &mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "echo", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{}, + wantKey: "content", + }, + { + name: "resources/read", + parsed: &mcpparser.ParsedMCPRequest{Method: "resources/read", ResourceID: "embedded:info", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{}, + wantKey: "contents", + }, + { + name: "prompts/get", + parsed: &mcpparser.ParsedMCPRequest{Method: "prompts/get", ResourceID: "review", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{}, + wantKey: "messages", + }, + { + name: "completion/complete", + parsed: &mcpparser.ParsedMCPRequest{ + Method: "completion/complete", IsRequest: true, ID: "1", + Params: json.RawMessage(`{"ref":{"type":"ref/prompt","name":"review"},"argument":{"name":"style","value":"terse"}}`), + }, + fakeCore: &modernFakeCore{}, + wantKey: "completion", + }, + { + name: "ping", + parsed: &mcpparser.ParsedMCPRequest{Method: "ping", IsRequest: true, ID: "1"}, + fakeCore: &modernFakeCore{}, + wantBareResult: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + rec, body := dispatchModernTest(t.Context(), t, tt.fakeCore, false, tt.parsed) + + require.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC result envelope, got %v", body) + if tt.wantBareResult { + assert.Empty(t, result, "ping must return a bare {} result with no resultType/_meta") + assert.False(t, tt.fakeCore.callCalled, "ping must not reach the core") + return + } + assert.Equal(t, "complete", result["resultType"]) + assert.Contains(t, result, tt.wantKey) + }) + } +} + +// TestDispatchModern_ListError asserts a core List* error maps to -32603 +// (HTTP 200), same posture as the other error paths -- guards the list +// branch against silently swallowing an aggregation failure. +func TestDispatchModern_ListError(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "tools/list", IsRequest: true, ID: "1"} + fc := &modernFakeCore{listErr: errors.New("aggregation exploded")} + + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC error envelope, got %v", body) + assert.Equal(t, float64(jsonRPCCodeInternalError), errObj["code"]) +} + +// TestDispatchModern_Discover asserts server/discover's flags reflect exactly +// what the fake core admits: an empty admitted set advertises no capability +// (no descriptor arrays either way), and a populated set flags only the +// features that came back non-empty. +func TestDispatchModern_Discover(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "server/discover", IsRequest: true, ID: "1"} + + t.Run("nothing admitted -- only the static completions capability", func(t *testing.T) { + t.Parallel() + + rec, body := dispatchModernTest(t.Context(), t, &modernFakeCore{}, false, parsed) + + require.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + caps, ok := result["capabilities"].(map[string]any) + require.True(t, ok) + assert.Equal(t, map[string]any{"completions": map[string]any{}}, caps, + "completions is unconditional; no admitted list-backed feature must advertise nothing else") + }) + + t.Run("admitted view flags only the populated features", func(t *testing.T) { + t.Parallel() + + fc := &modernFakeCore{ + discoverCaps: core.DiscoverCapabilities{HasTools: true, HasResourceTemplates: true}, + } + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + require.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + caps, ok := result["capabilities"].(map[string]any) + require.True(t, ok) + assert.Contains(t, caps, "tools") + assert.Contains(t, caps, "resources", "a resource template alone must still set the resources flag") + assert.Contains(t, caps, "completions", "completions is unconditional") + assert.NotContains(t, caps, "prompts") + }) + + t.Run("a core Discover error maps to -32603", func(t *testing.T) { + t.Parallel() + + fc := &modernFakeCore{discoverErr: errors.New("aggregation exploded")} + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC error envelope, got %v", body) + assert.Equal(t, float64(jsonRPCCodeInternalError), errObj["code"]) + }) +} + +// TestDispatchModern_Complete covers the completion/complete dispatch path: +// successful routing to core.Complete with no pre-dispatch Check* gate (see +// call_gate.go's comment on why completion/complete is intentionally not +// wire-gated), ref-type-scoped deny-message reclassification on +// ErrAuthorizationFailed (mirroring the SDK path's coreCompletionHandler), a +// non-authz core error mapping to -32603, and malformed params rejected +// before the core is ever reached. +func TestDispatchModern_Complete(t *testing.T) { + t.Parallel() + + validPromptRef := json.RawMessage( + `{"ref":{"type":"ref/prompt","name":"review"},"argument":{"name":"style","value":"terse"}}`) + validResourceRef := json.RawMessage( + `{"ref":{"type":"ref/resource","uri":"file:///{date}.txt"},"argument":{"name":"date","value":"2026"}}`) + + t.Run("success returns the completion envelope with no Check* call", func(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "completion/complete", IsRequest: true, ID: "1", Params: validPromptRef} + fc := &modernFakeCore{} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + require.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Equal(t, "complete", result["resultType"]) + completion, ok := result["completion"].(map[string]any) + require.True(t, ok) + assert.Equal(t, []any{"opt1", "opt2"}, completion["values"]) + assert.True(t, fc.callCalled) + assert.False(t, fc.checkCalled, "completion/complete has no pre-dispatch Check* gate") + }) + + t.Run("ErrAuthorizationFailed on a prompt ref reclassifies to 403 with the prompt-get deny message", func(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "completion/complete", IsRequest: true, ID: "1", Params: validPromptRef} + fc := &modernFakeCore{completeErr: fmt.Errorf("%w: denied", vmcp.ErrAuthorizationFailed)} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusForbidden, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok) + assert.Equal(t, float64(mcpparser.JSONRPCCodeDenied), errObj["code"]) + assert.Equal(t, vmcp.DenyMessagePromptGet, errObj["message"]) + }) + + t.Run("ErrAuthorizationFailed on a resource ref reclassifies to 403 with the resource-read deny message", func(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "completion/complete", IsRequest: true, ID: "1", Params: validResourceRef} + fc := &modernFakeCore{completeErr: fmt.Errorf("%w: denied", vmcp.ErrAuthorizationFailed)} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusForbidden, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok) + assert.Equal(t, vmcp.DenyMessageResourceRead, errObj["message"]) + }) + + t.Run("a non-authz core error maps to -32603", func(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "completion/complete", IsRequest: true, ID: "1", Params: validPromptRef} + fc := &modernFakeCore{completeErr: errors.New("backend exploded")} + + 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) + assert.Equal(t, float64(jsonRPCCodeInternalError), errObj["code"]) + }) + + t.Run("malformed params rejected before the core is reached", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + params json.RawMessage + }{ + {name: "missing ref", params: json.RawMessage(`{"argument":{"name":"x","value":"y"}}`)}, + { + name: "ref missing type", + params: json.RawMessage(`{"ref":{"name":"review"},"argument":{"name":"x","value":"y"}}`), + }, + { + name: "ref not an object", + params: json.RawMessage(`{"ref":"review","argument":{"name":"x","value":"y"}}`), + }, + { + name: "missing argument.name", + params: json.RawMessage(`{"ref":{"type":"ref/prompt","name":"review"},"argument":{"value":"y"}}`), + }, + { + name: "empty argument.name", + params: json.RawMessage(`{"ref":{"type":"ref/prompt","name":"review"},"argument":{"name":"","value":"y"}}`), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{ + Method: "completion/complete", IsRequest: true, ID: "1", Params: tt.params, + } + fc := &modernFakeCore{} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Equal(t, float64(jsonRPCCodeInvalidParams), errObj["code"]) + assert.False(t, fc.callCalled, "malformed params must not reach the core") + }) + } + }) +} + +// TestDispatchModern_AuthzGate is the security-load-bearing table: it drives +// the re-homed gate for each of the three gated methods (tools/call, +// resources/read, prompts/get) through every branch authzCallGate itself +// supports, PLUS the dispatch-time (TOCTOU) reclassification that only exists +// here because dispatchModern -- unlike the gate -- also owns the real call. +func TestDispatchModern_AuthzGate(t *testing.T) { + t.Parallel() + + deny := fmt.Errorf("%w: policy said no", vmcp.ErrAuthorizationFailed) + infra := errors.New("aggregation exploded") + + type gatedCase struct { + method string + resourceID string + wantKey string + denyMsg string + } + cases := []gatedCase{ + {method: "tools/call", resourceID: "secret-tool", wantKey: "content", denyMsg: vmcp.DenyMessageToolCall}, + {method: "resources/read", resourceID: "file://secret", wantKey: "contents", denyMsg: vmcp.DenyMessageResourceRead}, + {method: "prompts/get", resourceID: "secret-prompt", wantKey: "messages", denyMsg: vmcp.DenyMessagePromptGet}, + } + + for _, c := range cases { + parsed := &mcpparser.ParsedMCPRequest{Method: c.method, ResourceID: c.resourceID, IsRequest: true, ID: "1"} + + t.Run(c.method+"/denied pre-dispatch returns 403", func(t *testing.T) { + t.Parallel() + fc := &modernFakeCore{checkToolErr: deny, checkResourceErr: deny, checkPromptErr: deny} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusForbidden, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok) + assert.Equal(t, float64(mcpparser.JSONRPCCodeDenied), errObj["code"]) + assert.Equal(t, c.denyMsg, errObj["message"]) + assert.True(t, fc.checkCalled, "Check* must have been invoked") + assert.False(t, fc.callCalled, "the real call must NOT run once Check* denies") + }) + + t.Run(c.method+"/[HIGH] denied at dispatch time (TOCTOU) still returns 403 not -32603", func(t *testing.T) { + t.Parallel() + // Check* allows (nil), but the real call denies -- the aggregation + // re-run at dispatch time disagrees with the pre-flight check. + fc := &modernFakeCore{callToolErr: deny, readResourceErr: deny, getPromptErr: deny} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusForbidden, rec.Code, "a dispatch-time authz denial must audit as 403/denied") + errObj, ok := body["error"].(map[string]any) + require.True(t, ok) + assert.Equal(t, float64(mcpparser.JSONRPCCodeDenied), errObj["code"], + "must be the denial code, never -32603 -- that would audit as failure instead of denied") + assert.Equal(t, c.denyMsg, errObj["message"]) + assert.True(t, fc.checkCalled) + assert.True(t, fc.callCalled, "the real call must still run when Check* allows") + }) + + t.Run(c.method+"/infra error at Check* fails open, dispatch still happens", func(t *testing.T) { + t.Parallel() + fc := &modernFakeCore{checkToolErr: infra, checkResourceErr: infra, checkPromptErr: infra} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusOK, rec.Code, "an infra error at Check* must not become a 403") + result, ok := body["result"].(map[string]any) + require.True(t, ok, "dispatch must still have proceeded and returned a result, got %v", body) + assert.Contains(t, result, c.wantKey) + assert.True(t, fc.callCalled) + }) + + t.Run(c.method+"/non-authz error at dispatch time maps to -32603 not 403", func(t *testing.T) { + t.Parallel() + fc := &modernFakeCore{callToolErr: infra, readResourceErr: infra, getPromptErr: infra} + + 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) + assert.Equal(t, float64(jsonRPCCodeInternalError), errObj["code"]) + }) + + t.Run(c.method+"/authz disabled skips Check* and dispatches", func(t *testing.T) { + t.Parallel() + // Even a would-be-denying Check* must never run when the gate is off. + fc := &modernFakeCore{checkToolErr: deny, checkResourceErr: deny, checkPromptErr: deny} + + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Contains(t, result, c.wantKey) + assert.False(t, fc.checkCalled, "Check* must not run when authzGateEnabled is false") + assert.True(t, fc.callCalled) + }) + } +} + +// 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 -- +// indistinguishable from "absent" -- so the guard must inspect parsed.Params +// directly, and must run BEFORE the authz gate and the real call. +// resources/read has no arguments and is intentionally not covered here. +func TestDispatchModern_ArgumentsShape(t *testing.T) { + t.Parallel() + + cases := []struct { + method string + resourceID string + wantKey string + }{ + {method: "tools/call", resourceID: "echo", wantKey: "content"}, + {method: "prompts/get", resourceID: "review", wantKey: "messages"}, + } + + for _, c := range cases { + t.Run(c.method+"/non-object arguments rejected before Check* or the call", func(t *testing.T) { + t.Parallel() + for _, raw := range []string{`"a string"`, `[1,2,3]`} { + parsed := &mcpparser.ParsedMCPRequest{ + Method: c.method, ResourceID: c.resourceID, IsRequest: true, ID: "1", + Params: json.RawMessage(`{"name":"` + c.resourceID + `","arguments":` + raw + `}`), + } + fc := &modernFakeCore{} + + rec, body := dispatchModernTest(t.Context(), t, fc, true, parsed) + + assert.Equal(t, http.StatusBadRequest, rec.Code, "-32602 invalid params maps to HTTP 400") + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC error envelope for arguments=%s, got %v", raw, body) + assert.Equal(t, float64(jsonRPCCodeInvalidParams), errObj["code"]) + assert.False(t, fc.checkCalled, "Check* must not run for invalid arguments shape") + assert.False(t, fc.callCalled, "the real call must not run for invalid arguments shape") + } + }) + + t.Run(c.method+"/absent arguments proceeds", func(t *testing.T) { + t.Parallel() + parsed := &mcpparser.ParsedMCPRequest{ + Method: c.method, ResourceID: c.resourceID, IsRequest: true, ID: "1", + Params: json.RawMessage(`{"name":"` + c.resourceID + `"}`), + } + fc := &modernFakeCore{} + + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Contains(t, result, c.wantKey) + assert.True(t, fc.callCalled) + }) + + t.Run(c.method+"/object arguments proceeds", func(t *testing.T) { + t.Parallel() + parsed := &mcpparser.ParsedMCPRequest{ + Method: c.method, ResourceID: c.resourceID, IsRequest: true, ID: "1", + Params: json.RawMessage(`{"name":"` + c.resourceID + `","arguments":{"x":1}}`), + } + fc := &modernFakeCore{} + + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Contains(t, result, c.wantKey) + assert.True(t, fc.callCalled) + }) + + t.Run(c.method+"/explicit null arguments proceeds", func(t *testing.T) { + t.Parallel() + // hasNonObjectArguments relies on json.Unmarshal("null", &raw.Arguments) + // leaving raw.Arguments nil with no error -- an explicit JSON null is a + // legitimate no-args call, same as an absent field, not a rejected shape. + parsed := &mcpparser.ParsedMCPRequest{ + Method: c.method, ResourceID: c.resourceID, IsRequest: true, ID: "1", + Params: json.RawMessage(`{"name":"` + c.resourceID + `","arguments":null}`), + } + fc := &modernFakeCore{} + + rec, body := dispatchModernTest(t.Context(), t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "got %v", body) + assert.Contains(t, result, c.wantKey) + assert.True(t, fc.callCalled) + }) + } +} + +// contextProbeKey is a private key used only to prove r.Context() reaches the +// core call unmodified (not detached, not re-wrapped): headerforward reads +// forwarded-header state off this exact context per call, so a detach would +// silently break backend auth on every Modern request. +type contextProbeKey struct{} + +func TestDispatchModern_ContextPassthrough(t *testing.T) { + t.Parallel() + + ctx := context.WithValue(t.Context(), contextProbeKey{}, "probe-value") + parsed := &mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "echo", IsRequest: true, ID: "1"} + fc := &modernFakeCore{} + + _, _ = dispatchModernTest(ctx, t, fc, false, parsed) + + require.NotNil(t, fc.gotCtx) + assert.Equal(t, "probe-value", fc.gotCtx.Value(contextProbeKey{}), + "dispatchModern must pass r.Context() through to core.CallTool unmodified") +} + +// TestDispatchModern_LabelsAuditBackend mirrors TestServeHandlersLabelAuditBackend +// (serve_session_test.go) for the Modern path: after a successful tools/call, +// resources/read, or prompts/get, the dispatcher must write the registry-resolved +// backend name into the audit BackendInfo carried in the request context, using the +// same s.backendDisplayName resolution the Serve path's handlers use. completion/complete +// is deliberately excluded -- see dispatchModernComplete's doc comment on why that gap +// is pre-existing on both paths, not something this fix should introduce for Modern only. +func TestDispatchModern_LabelsAuditBackend(t *testing.T) { + t.Parallel() + + // BackendID "backend-x" != Name "github-mcp": a pass-through would record the ID. + reg := vmcp.NewImmutableRegistry([]vmcp.Backend{{ID: "backend-x", Name: "github-mcp"}}) + + tests := []struct { + name string + parsed *mcpparser.ParsedMCPRequest + }{ + {name: "tools/call", parsed: &mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "echo", IsRequest: true, ID: "1"}}, + { + name: "resources/read", + parsed: &mcpparser.ParsedMCPRequest{ + Method: "resources/read", ResourceID: "file://a", IsRequest: true, ID: "1", + }, + }, + { + name: "prompts/get", + parsed: &mcpparser.ParsedMCPRequest{ + Method: "prompts/get", ResourceID: "review", IsRequest: true, ID: "1", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + s := &Server{ + config: &Config{Name: testServerName, Version: testServerVersion}, + core: &modernFakeCore{backendID: "backend-x"}, + backendRegistry: reg, + } + + bi := &audit.BackendInfo{} + ctx := audit.WithBackendInfo(t.Context(), bi) + req := httptest.NewRequest(http.MethodPost, "/mcp", nil).WithContext(ctx) + rec := httptest.NewRecorder() + + s.dispatchModern(rec, req, tt.parsed) + + require.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "github-mcp", bi.BackendName, + "the Modern dispatcher must label the audit event with the registry-resolved backend name") + }) + } +} + +// TestDispatchModern_NoBackendIDSkipsAuditLabel locks in the composite-tool case: a +// result with an empty BackendID (executeComposite never sets one, core_calls.go) must +// not touch the audit BackendInfo at all, matching the "no single serving backend" +// semantics documented on vmcp.ToolCallResult.BackendID. +func TestDispatchModern_NoBackendIDSkipsAuditLabel(t *testing.T) { + t.Parallel() + + parsed := &mcpparser.ParsedMCPRequest{Method: "tools/call", ResourceID: "echo", IsRequest: true, ID: "1"} + fc := &modernFakeCore{} // backendID left empty + + bi := &audit.BackendInfo{} + ctx := audit.WithBackendInfo(t.Context(), bi) + rec, _ := dispatchModernTest(ctx, t, fc, false, parsed) + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Empty(t, bi.BackendName, "an empty BackendID must leave the audit label untouched") +} diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go new file mode 100644 index 0000000000..5fefc8968e --- /dev/null +++ b/pkg/vmcp/server/modern_envelope.go @@ -0,0 +1,570 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/json" + "fmt" + "log/slog" + "maps" + "net/http" + + "github.com/stacklok/toolhive-core/mcpcompat/mcp" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" + "github.com/stacklok/toolhive/pkg/vmcp/conversion" +) + +// This file hand-rolls the MCP 2026-07-28 ("Modern") stateless response +// envelope: the wire shape go-sdk@v1.7.0-pre.3 produces for a single-shot +// tools/list, resources/list, resources/templates/list, prompts/list, +// tools/call, resources/read, and prompts/get. ToolHive imports go-sdk +// v1.6.1, whose Modern types don't exist (or are unexported), so this is a +// durable parallel serializer, not a stopgap: resultType and _meta.serverInfo +// are set by unexported SDK functions (setCompleteResultType, +// annotateServerInfo) that run inside the exact ServerSession dispatch this +// package bypasses for Modern stateless requests. A future go-sdk bump cannot +// just delete these structs and marshal SDK result types directly -- the +// Modern annotations would vanish with them. +// +// modernResultTypeComplete is the sole value dispatchModern ever needs: this +// package only performs single-shot dispatch, never the elicitation retry +// loop (resultType "input_required"), so every result built here is +// unconditionally "complete". Do not add Legacy-conditional branching to +// these structs -- this file is reached only for Modern requests. +const modernResultTypeComplete = "complete" + +// modernServerInfoKey is the go-sdk's MetaKeyServerInfo +// (protocol.go:2367 in go-sdk@v1.7.0-pre.3), reproduced by hand since v1.6.1 +// does not export it. +const modernServerInfoKey = "io.modelcontextprotocol/serverInfo" + +// modernServerInfo mirrors the go-sdk's Implementation type. +type modernServerInfo struct { + Name string `json:"name"` + Version string `json:"version"` +} + +// modernMeta carries the _meta.serverInfo entry the SDK attaches to every +// Modern result, unconditionally and together with resultType. +type modernMeta struct { + ServerInfo modernServerInfo `json:"io.modelcontextprotocol/serverInfo"` +} + +func newModernMeta(serverName, serverVersion string) modernMeta { + return modernMeta{ServerInfo: modernServerInfo{Name: serverName, Version: serverVersion}} +} + +// newModernResultMeta builds _meta for tools/call, resources/read, and +// prompts/get, the three builders whose domain result carries its own +// backend Meta (vmcp.ToolCallResult.Meta etc.) -- unlike the four list +// results and server/discover, which have no per-result backend meta and use +// the serverInfo-only newModernMeta above. The SDK path preserves backend +// meta via conversion.ToMCPMeta (serve_handlers.go); dropping it here would +// silently discard whatever the backend attached (progress tokens, trace +// ids, ...). backendMeta is cloned before the serverInfo key is added (copy +// before mutating caller input) so the domain result's map is never touched. +// A Modern backend could in principle return that same namespaced key; the +// unconditional overwrite is still correct here because the client's actual +// MCP peer is vMCP, not the backend, so vMCP's own serverInfo must win. +// +// Every other backendMeta key -- including any other io.modelcontextprotocol/* +// reserved key a backend happens to set -- is forwarded unfiltered, matching +// the Legacy path's conversion.ToMCPMeta. Stripping reserved keys is a +// tracked follow-up; it must land in a helper shared by both paths, not here +// only, or Legacy and Modern would drift. +func newModernResultMeta(backendMeta map[string]any, serverName, serverVersion string) map[string]any { + meta := maps.Clone(backendMeta) + if meta == nil { + meta = make(map[string]any, 1) + } + meta[modernServerInfoKey] = modernServerInfo{Name: serverName, Version: serverVersion} + return meta +} + +// modernCacheable mirrors the go-sdk's Cacheable struct (protocol.go:1168), +// with no omitempty: both fields are always present on the wire. +// +// CacheScope is deliberately hardcoded to "private", diverging from the SDK's +// "public" default. The four list results and resources/read vary by caller +// identity (admission.FilterTools/Resources/Prompts, AllowResourceRead), so +// advertising "public" would let a shared cache/intermediary serve one +// identity's admission-filtered view to another. TTLMs is 0 ("immediately +// stale") as an interim value for a re-aggregating gateway with no cache +// invalidation signal yet. +type modernCacheable struct { + TTLMs int `json:"ttlMs"` + CacheScope string `json:"cacheScope"` +} + +func newModernCacheable() modernCacheable { + return modernCacheable{TTLMs: 0, CacheScope: "private"} +} + +// modernToolsListResult is the tools/list wire result. +type modernToolsListResult struct { + ResultType string `json:"resultType"` + modernCacheable + Tools []mcp.Tool `json:"tools"` + Meta modernMeta `json:"_meta"` +} + +// modernResourcesListResult is the resources/list wire result. +type modernResourcesListResult struct { + ResultType string `json:"resultType"` + modernCacheable + Resources []mcp.Resource `json:"resources"` + Meta modernMeta `json:"_meta"` +} + +// modernResourceTemplatesListResult is the resources/templates/list wire result. +type modernResourceTemplatesListResult struct { + ResultType string `json:"resultType"` + modernCacheable + ResourceTemplates []mcp.ResourceTemplate `json:"resourceTemplates"` + Meta modernMeta `json:"_meta"` +} + +// modernPromptsListResult is the prompts/list wire result. +type modernPromptsListResult struct { + ResultType string `json:"resultType"` + modernCacheable + Prompts []mcp.Prompt `json:"prompts"` + Meta modernMeta `json:"_meta"` +} + +// modernCallToolResult is the tools/call wire result. Unlike the four list +// results and resources/read, it does NOT embed modernCacheable -- a tool +// call is an action, not a cacheable read, and the SDK never attaches +// Cacheable to CallToolResult. Meta is a map (not modernMeta) because it must +// carry both the backend's own result meta AND serverInfo -- see +// newModernResultMeta. +type modernCallToolResult struct { + ResultType string `json:"resultType"` + Content []mcp.Content `json:"content"` + StructuredContent any `json:"structuredContent,omitempty"` + IsError bool `json:"isError,omitempty"` + Meta map[string]any `json:"_meta"` +} + +// modernReadResourceResult is the resources/read wire result. Meta is a map +// for the same reason as modernCallToolResult -- see newModernResultMeta. +type modernReadResourceResult struct { + ResultType string `json:"resultType"` + modernCacheable + Contents []mcp.ResourceContents `json:"contents"` + Meta map[string]any `json:"_meta"` +} + +// modernGetPromptResult is the prompts/get wire result. Like tools/call, it +// does NOT embed modernCacheable -- the SDK never attaches Cacheable to +// GetPromptResult. Meta is a map for the same reason as modernCallToolResult +// -- see newModernResultMeta. +type modernGetPromptResult struct { + ResultType string `json:"resultType"` + Description string `json:"description,omitempty"` + Messages []mcp.PromptMessage `json:"messages"` + Meta map[string]any `json:"_meta"` +} + +// modernCompletionDetails mirrors the go-sdk's CompletionResultDetails +// (protocol.go:653 in go-sdk@v1.7.0-pre.3): Values has no omitempty tag +// there, so it doesn't here either -- an empty result still marshals +// "values":[] rather than omitting the field. +type modernCompletionDetails struct { + Values []string `json:"values"` + Total int `json:"total,omitempty"` + HasMore bool `json:"hasMore,omitempty"` +} + +// modernCompleteResult is the completion/complete wire result. Like +// tools/call and prompts/get, it does NOT embed modernCacheable -- the SDK +// never attaches Cacheable to CompleteResult. Meta is modernMeta (not a map) +// because vmcp.CompletionResult carries no backend meta to preserve -- unlike +// newModernResultMeta's callers, there is nothing to clone here. +type modernCompleteResult struct { + ResultType string `json:"resultType"` + Completion modernCompletionDetails `json:"completion"` + Meta modernMeta `json:"_meta"` +} + +// newModernComplete builds the completion/complete wire result from the +// core's CompletionResult. Values is initialized to a non-nil slice so an +// empty result marshals as [] not null, matching the four list builders. +// +// result is expected non-nil (core.Complete never returns a nil result +// without an error); the nil check below is defense-in-depth against a +// plausible aggregator bug, not an expected input. +func newModernComplete(result *vmcp.CompletionResult, serverName, serverVersion string) modernCompleteResult { + if result == nil { + result = &vmcp.CompletionResult{} + } + values := result.Values + if values == nil { + values = []string{} + } + return modernCompleteResult{ + ResultType: modernResultTypeComplete, + Completion: modernCompletionDetails{ + Values: values, + Total: result.Total, + HasMore: result.HasMore, + }, + Meta: newModernMeta(serverName, serverVersion), + } +} + +// modernDiscoverResult is the server/discover wire result -- Modern's +// replacement for initialize+capability negotiation. Unlike the four list +// results, it carries no descriptor arrays: Capabilities reuses mcpcompat's +// mcp.ServerCapabilities, whose per-feature fields are themselves the "flag" +// (a pointer field present iff the identity can reach that feature at all). +type modernDiscoverResult struct { + ResultType string `json:"resultType"` + modernCacheable + SupportedVersions []string `json:"supportedVersions"` + Capabilities mcp.ServerCapabilities `json:"capabilities"` + Meta modernMeta `json:"_meta"` +} + +// newModernDiscover builds the server/discover wire result. hasTools/ +// hasResources/hasPrompts are the caller's len(core.List*(ctx, identity))>0 +// admission-filtered presence checks -- this function only shapes them into +// the wire capability flags, so a discover response reflects exclusively +// what this identity may reach, same as the four list results. hasTemplates +// folds into the "resources" capability: the wire protocol has no separate +// resource-templates capability flag. +// +// Completions is set unconditionally, unlike the three identity-filtered +// flags above: completion/complete has no per-identity admission of its own +// (core.Complete authorizes the underlying prompt/resource ref at dispatch, +// not the completions feature itself), and this dispatcher serves it for +// every caller once Modern dispatch is enabled at all -- matching how the +// Legacy/SDK path always wires server.WithCompletionHandler regardless of +// identity (serve.go). +// +// SupportedVersions enumerates every protocol version this vMCP endpoint +// actually serves, not just Modern: mcpparser.MCPVersionModern (this +// dispatcher) plus mcp.LATEST_PROTOCOL_VERSION, the one Legacy version the +// SDK path negotiates (mcpcompat's handleInitialize always responds with +// 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. +func newModernDiscover( + hasTools, hasResources, hasTemplates, hasPrompts bool, serverName, serverVersion string, +) modernDiscoverResult { + var caps mcp.ServerCapabilities + if hasTools { + caps.Tools = &struct { + ListChanged bool `json:"listChanged,omitempty"` + }{} + } + if hasResources || hasTemplates { + // Subscribe is left false (omitted on the wire): this stateless + // single-shot dispatcher has no persistent connection to a client to + // push a server-initiated resources/updated notification over, so + // resources/subscribe is not advertised here and returns -32601 by + // design, not by oversight. + caps.Resources = &struct { + Subscribe bool `json:"subscribe,omitempty"` + ListChanged bool `json:"listChanged,omitempty"` + }{} + } + if hasPrompts { + caps.Prompts = &struct { + ListChanged bool `json:"listChanged,omitempty"` + }{} + } + caps.Completions = &struct{}{} + return modernDiscoverResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + SupportedVersions: []string{mcpparser.MCPVersionModern, mcp.LATEST_PROTOCOL_VERSION}, + Capabilities: caps, + Meta: newModernMeta(serverName, serverVersion), + } +} + +// newModernToolsList builds the tools/list wire result from the core's +// admission-filtered domain tools. +func newModernToolsList(tools []vmcp.Tool, serverName, serverVersion string) (modernToolsListResult, error) { + wireTools := make([]mcp.Tool, 0, len(tools)) + for _, t := range tools { + wireTool, err := modernToolFromDomain(t) + if err != nil { + return modernToolsListResult{}, err + } + wireTools = append(wireTools, wireTool) + } + return modernToolsListResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + Tools: wireTools, + Meta: newModernMeta(serverName, serverVersion), + }, nil +} + +// newModernResourcesList builds the resources/list wire result from the +// core's admission-filtered domain resources. +func newModernResourcesList(resources []vmcp.Resource, serverName, serverVersion string) modernResourcesListResult { + wireResources := make([]mcp.Resource, 0, len(resources)) + for _, r := range resources { + wireResources = append(wireResources, modernResourceFromDomain(r)) + } + return modernResourcesListResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + Resources: wireResources, + Meta: newModernMeta(serverName, serverVersion), + } +} + +// newModernResourceTemplatesList builds the resources/templates/list wire +// result from the core's admission-filtered domain resource templates. +func newModernResourceTemplatesList( + templates []vmcp.ResourceTemplate, serverName, serverVersion string, +) modernResourceTemplatesListResult { + wireTemplates := make([]mcp.ResourceTemplate, 0, len(templates)) + for _, t := range templates { + wireTemplates = append(wireTemplates, modernResourceTemplateFromDomain(t)) + } + return modernResourceTemplatesListResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + ResourceTemplates: wireTemplates, + Meta: newModernMeta(serverName, serverVersion), + } +} + +// newModernPromptsList builds the prompts/list wire result from the core's +// admission-filtered domain prompts. +func newModernPromptsList(prompts []vmcp.Prompt, serverName, serverVersion string) modernPromptsListResult { + wirePrompts := make([]mcp.Prompt, 0, len(prompts)) + for _, p := range prompts { + wirePrompts = append(wirePrompts, modernPromptFromDomain(p)) + } + return modernPromptsListResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + Prompts: wirePrompts, + Meta: newModernMeta(serverName, serverVersion), + } +} + +// newModernCallToolResult builds the tools/call wire result from the core's +// ToolCallResult. StructuredContent is omitted entirely (not merely +// omitempty-false) when the core did not set it, matching the SDK's +// omitempty behavior. +// +// result is expected non-nil (core.CallTool never returns a nil result +// without an error); the nil check below is defense-in-depth against a +// plausible aggregator bug, not an expected input. +func newModernCallToolResult(result *vmcp.ToolCallResult, serverName, serverVersion string) modernCallToolResult { + if result == nil { + result = &vmcp.ToolCallResult{} + } + var structuredContent any + if len(result.StructuredContent) > 0 { + structuredContent = result.StructuredContent + } + return modernCallToolResult{ + ResultType: modernResultTypeComplete, + Content: conversion.ToMCPContents(result.Content), + StructuredContent: structuredContent, + IsError: result.IsError, + Meta: newModernResultMeta(result.Meta, serverName, serverVersion), + } +} + +// newModernReadResourceResult builds the resources/read wire result from the +// core's ResourceReadResult. +// +// result is expected non-nil (core.ReadResource never returns a nil result +// without an error); the nil check below is defense-in-depth against a +// plausible aggregator bug, not an expected input. +func newModernReadResourceResult( + result *vmcp.ResourceReadResult, serverName, serverVersion string, +) modernReadResourceResult { + if result == nil { + result = &vmcp.ResourceReadResult{} + } + return modernReadResourceResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + Contents: conversion.ToMCPResourceContents(result.Contents), + Meta: newModernResultMeta(result.Meta, serverName, serverVersion), + } +} + +// newModernGetPromptResult builds the prompts/get wire result from the +// core's PromptGetResult. +// +// result is expected non-nil (core.GetPrompt never returns a nil result +// without an error); the nil check below is defense-in-depth against a +// plausible aggregator bug, not an expected input. +func newModernGetPromptResult(result *vmcp.PromptGetResult, serverName, serverVersion string) modernGetPromptResult { + if result == nil { + result = &vmcp.PromptGetResult{} + } + return modernGetPromptResult{ + ResultType: modernResultTypeComplete, + Description: result.Description, + Messages: conversion.ToMCPPromptMessages(result.Messages), + Meta: newModernResultMeta(result.Meta, serverName, serverVersion), + } +} + +// modernToolFromDomain converts a vmcp.Tool to the wire mcp.Tool used in a +// tools/list result. Mirrors coreSessionTools' Legacy adaptation +// (serve_handlers.go) field-for-field, so Legacy and Modern report identical +// tool shapes -- do not reinvent this mapping if the domain type changes. +func modernToolFromDomain(t vmcp.Tool) (mcp.Tool, error) { + schemaJSON, err := json.Marshal(t.InputSchema) + if err != nil { + return mcp.Tool{}, fmt.Errorf("marshal input schema for tool %s: %w", t.Name, err) + } + wireTool := mcp.Tool{ + Name: t.Name, + Description: t.Description, + RawInputSchema: schemaJSON, + // ponytail: a tool with no annotations still marshals "annotations":{} + // because mcpcompat's Tool.MarshalJSON (mcpcompat/mcp/tools.go:343) + // writes the field unconditionally. Shared with Legacy's + // coreSessionTools, so the real fix belongs in mcpcompat, not here -- + // fixing it only in this file would break Legacy/Modern parity. + Annotations: conversion.ToMCPToolAnnotations(t.Annotations), + } + // Unlike the required InputSchema above, OutputSchema is best-effort: on + // failure the tool is still advertised without it (matches + // coreSessionTools). + if t.OutputSchema != nil { + if outputSchemaJSON, marshalErr := json.Marshal(t.OutputSchema); marshalErr != nil { + slog.Warn("failed to marshal tool output schema", "tool", t.Name, "error", marshalErr) + } else { + wireTool.RawOutputSchema = outputSchemaJSON + } + } + return wireTool, nil +} + +// modernResourceFromDomain converts a vmcp.Resource to the wire mcp.Resource +// used in a resources/list result. +func modernResourceFromDomain(r vmcp.Resource) mcp.Resource { + return mcp.Resource{ + Name: r.Name, + URI: r.URI, + Description: r.Description, + MIMEType: r.MimeType, + } +} + +// modernResourceTemplateFromDomain converts a vmcp.ResourceTemplate to the +// wire mcp.ResourceTemplate used in a resources/templates/list result. +func modernResourceTemplateFromDomain(t vmcp.ResourceTemplate) mcp.ResourceTemplate { + return mcp.ResourceTemplate{ + Name: t.Name, + URITemplate: t.URITemplate, + Description: t.Description, + MIMEType: t.MimeType, + } +} + +// modernPromptFromDomain converts a vmcp.Prompt to the wire mcp.Prompt used +// in a prompts/list result. +func modernPromptFromDomain(p vmcp.Prompt) mcp.Prompt { + arguments := make([]mcp.PromptArgument, 0, len(p.Arguments)) + for _, arg := range p.Arguments { + arguments = append(arguments, mcp.PromptArgument{ + Name: arg.Name, + Description: arg.Description, + Required: arg.Required, + }) + } + return mcp.Prompt{ + Name: p.Name, + Description: p.Description, + Arguments: arguments, + } +} + +// writeModernResult writes the JSON-RPC success envelope +// {"jsonrpc":"2.0","id":,"result":} as a single HTTP 200 +// application/json response -- never SSE, no Mcp-Session-Id -- matching the +// Modern stateless wire contract. +func writeModernResult(w http.ResponseWriter, id, result any) { + writeModernEnvelope(w, http.StatusOK, map[string]any{ + "jsonrpc": "2.0", + "id": id, + "result": result, + }) +} + +// writeModernError writes a JSON-RPC error envelope, deriving the HTTP +// status from the JSON-RPC code -- mirroring go-sdk's extractErrorStatus +// (streamable.go:1033-1061): +// - -32601 (method not found) -> 404: the 2026-07-28 spec MUSTs 404 for an +// unimplemented method. +// - -32602 (invalid params) -> 400: matches the SDK's own mapping. +// - everything else (-32600 batch, -32603 internal) -> 200: matches +// extractErrorStatus returning 0 for those codes -- the request was +// accepted and processed, so the failure is an application-level +// JSON-RPC error riding the transport, not a wire-level rejection. +// +// This is a protocol-level mapping, unrelated to authorization: only +// writeModernDenied changes status for a POLICY reason (403). +func writeModernError(w http.ResponseWriter, id any, code int, msg string) { + status := http.StatusOK + switch code { + case jsonRPCCodeMethodNotFound: + status = http.StatusNotFound + case jsonRPCCodeInvalidParams: + status = http.StatusBadRequest + } + writeModernEnvelope(w, status, map[string]any{ + "jsonrpc": "2.0", + "id": id, + "error": map[string]any{ + "code": code, + "message": msg, + }, + }) +} + +// 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 +// makes the audit middleware log the request as denied rather than failed. +func writeModernDenied(w http.ResponseWriter, id any, msg string) { + writeModernEnvelope(w, http.StatusForbidden, map[string]any{ + "jsonrpc": "2.0", + "id": id, + "error": map[string]any{ + "code": mcpparser.JSONRPCCodeDenied, + "message": msg, + }, + }) +} + +// writeModernEnvelope marshals envelope before writing headers/status, so a +// marshal failure never leaves a response half-written (mirrors +// classificationErrorBody's build-then-write ordering). +func writeModernEnvelope(w http.ResponseWriter, status int, envelope map[string]any) { + body, err := json.Marshal(envelope) + if err != nil { + // Unreachable in practice: every value passed to these writers is a + // wire struct defined in this file, a domain string, or an int code -- + // all JSON-marshalable. Fall back to a valid JSON-RPC error body + // rather than writing nothing. + slog.Error("failed to marshal Modern JSON-RPC envelope", "error", err) + body = []byte(`{"jsonrpc":"2.0","id":null,"error":{"code":-32603,"message":"Internal error"}}`) + } + w.Header().Set("Content-Type", "application/json") + // Belt-and-suspenders behind the JSON-level cacheScope:"private" hint: + // an MCP-unaware intermediary (CDN, corporate proxy) wouldn't look at the + // body, so every Modern response also asserts it at the HTTP layer. + w.Header().Set("Cache-Control", "private, no-store") + w.WriteHeader(status) + //nolint:gosec // G104: writing a JSON-RPC response to an HTTP client + _, _ = w.Write(body) +} diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go new file mode 100644 index 0000000000..51ee9ffd4f --- /dev/null +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -0,0 +1,719 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + + mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +const ( + testServerName = "toolhive-vmcp" + testServerVersion = "0.1.0" +) + +// TestModernEnvelopeCommonFields asserts the invariants that apply to every +// Modern result: resultType:"complete", _meta.serverInfo, and Cacheable +// present on the four lists + resources/read but absent on tools/call and +// prompts/get. +func TestModernEnvelopeCommonFields(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + build func(t *testing.T) any + wantCacheable bool + }{ + { + name: "tools/list", + build: func(t *testing.T) any { + t.Helper() + result, err := newModernToolsList([]vmcp.Tool{{ + Name: "greet", + Description: "say hi", + InputSchema: map[string]any{"type": "object"}, + }}, testServerName, testServerVersion) + require.NoError(t, err) + return result + }, + wantCacheable: true, + }, + { + name: "resources/list", + build: func(*testing.T) any { + return newModernResourcesList([]vmcp.Resource{ + {Name: "info", URI: "embedded:info", MimeType: "text/plain"}, + }, testServerName, testServerVersion) + }, + wantCacheable: true, + }, + { + name: "resources/templates/list", + build: func(*testing.T) any { + return newModernResourceTemplatesList([]vmcp.ResourceTemplate{ + {Name: "logs", URITemplate: "file:///logs/{date}.txt"}, + }, testServerName, testServerVersion) + }, + wantCacheable: true, + }, + { + name: "prompts/list", + build: func(*testing.T) any { + return newModernPromptsList([]vmcp.Prompt{ + {Name: "code_review", Arguments: []vmcp.PromptArgument{{Name: "Code", Required: true}}}, + }, testServerName, testServerVersion) + }, + wantCacheable: true, + }, + { + name: "tools/call", + build: func(*testing.T) any { + return newModernCallToolResult(&vmcp.ToolCallResult{ + Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "hello"}}, + }, testServerName, testServerVersion) + }, + wantCacheable: false, + }, + { + name: "resources/read", + build: func(*testing.T) any { + return newModernReadResourceResult(&vmcp.ResourceReadResult{ + Contents: []vmcp.ResourceContent{{URI: "embedded:info", MimeType: "text/plain", Text: "hi"}}, + }, testServerName, testServerVersion) + }, + wantCacheable: true, + }, + { + name: "prompts/get", + build: func(*testing.T) any { + return newModernGetPromptResult(&vmcp.PromptGetResult{ + Messages: []vmcp.PromptMessage{{Role: "user", Content: vmcp.Content{Type: vmcp.ContentTypeText, Text: "hi"}}}, + }, testServerName, testServerVersion) + }, + wantCacheable: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(tt.build(t)) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(raw, &decoded)) + + require.Equal(t, modernResultTypeComplete, decoded["resultType"], "resultType") + + meta, ok := decoded["_meta"].(map[string]any) + require.True(t, ok, "_meta must be present") + serverInfo, ok := meta[modernServerInfoKey].(map[string]any) + require.True(t, ok, "_meta.%s must be present", modernServerInfoKey) + require.Equal(t, testServerName, serverInfo["name"]) + require.Equal(t, testServerVersion, serverInfo["version"]) + + _, hasTTL := decoded["ttlMs"] + _, hasScope := decoded["cacheScope"] + require.Equal(t, tt.wantCacheable, hasTTL, "ttlMs presence") + require.Equal(t, tt.wantCacheable, hasScope, "cacheScope presence") + if tt.wantCacheable { + require.InDelta(t, 0, decoded["ttlMs"], 0) + require.Equal(t, "private", decoded["cacheScope"], + "vMCP results are admission-filtered per identity; \"public\" would leak across identities") + } + }) + } +} + +// TestModernResultMetaPreservesBackendMeta asserts _meta on tools/call, +// resources/read, and prompts/get carries BOTH the backend's own result.Meta +// keys AND serverInfo. Overwriting _meta with only serverInfo would silently +// discard whatever the backend attached (progress tokens, trace ids, ...), +// diverging from the SDK path's preservation via conversion.ToMCPMeta +// (serve_handlers.go). The no-backend-meta case (nil result.Meta) is already +// covered by TestModernEnvelopeCommonFields, which only asserts serverInfo. +func TestModernResultMetaPreservesBackendMeta(t *testing.T) { + t.Parallel() + + backendMeta := map[string]any{"progressToken": "tok-1", "traceId": "abc"} + + tests := []struct { + name string + build func() any + }{ + { + name: "tools/call", + build: func() any { + return newModernCallToolResult(&vmcp.ToolCallResult{Meta: backendMeta}, testServerName, testServerVersion) + }, + }, + { + name: "resources/read", + build: func() any { + return newModernReadResourceResult(&vmcp.ResourceReadResult{Meta: backendMeta}, testServerName, testServerVersion) + }, + }, + { + name: "prompts/get", + build: func() any { + return newModernGetPromptResult(&vmcp.PromptGetResult{Meta: backendMeta}, testServerName, testServerVersion) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(tt.build()) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(raw, &decoded)) + + meta, ok := decoded["_meta"].(map[string]any) + require.True(t, ok, "_meta must be present") + require.Equal(t, "tok-1", meta["progressToken"], "backend meta key must survive") + require.Equal(t, "abc", meta["traceId"], "backend meta key must survive") + + serverInfo, ok := meta[modernServerInfoKey].(map[string]any) + require.True(t, ok, "_meta.%s must still be present alongside backend meta", modernServerInfoKey) + require.Equal(t, testServerName, serverInfo["name"]) + require.Equal(t, testServerVersion, serverInfo["version"]) + + // backendMeta itself must be untouched (copy before mutating caller input). + require.Len(t, backendMeta, 2, "the caller's Meta map must not be mutated") + }) + } +} + +// TestModernResultMetaOverwritesSpoofedServerInfo is a regression case for +// newModernResultMeta's clone-then-set order: if a backend result.Meta +// already contains the "io.modelcontextprotocol/serverInfo" key (e.g. an +// untrusted or misbehaving backend spoofing vMCP's own identity), the real +// vMCP serverInfo must still win on the wire. One builder is enough to pin +// the shared newModernResultMeta logic all three call/read/get builders use. +func TestModernResultMetaOverwritesSpoofedServerInfo(t *testing.T) { + t.Parallel() + + spoofed := map[string]any{ + modernServerInfoKey: map[string]any{"name": "attacker-server", "version": "666"}, + } + + raw, err := json.Marshal(newModernCallToolResult(&vmcp.ToolCallResult{Meta: spoofed}, testServerName, testServerVersion)) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(raw, &decoded)) + + meta, ok := decoded["_meta"].(map[string]any) + require.True(t, ok, "_meta must be present") + serverInfo, ok := meta[modernServerInfoKey].(map[string]any) + require.True(t, ok) + require.Equal(t, testServerName, serverInfo["name"], "vMCP's real serverInfo must overwrite a backend-supplied one") + require.Equal(t, testServerVersion, serverInfo["version"]) +} + +// TestModernEnvelopeEmptyCollections asserts that an empty domain slice +// marshals to a JSON array ([]), never null. +func TestModernEnvelopeEmptyCollections(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + field string + build func(t *testing.T) any + }{ + { + name: "tools/list", + field: "tools", + build: func(t *testing.T) any { + t.Helper() + result, err := newModernToolsList(nil, testServerName, testServerVersion) + require.NoError(t, err) + return result + }, + }, + { + name: "resources/list", + field: "resources", + build: func(*testing.T) any { return newModernResourcesList(nil, testServerName, testServerVersion) }, + }, + { + name: "resources/templates/list", + field: "resourceTemplates", + build: func(*testing.T) any { + return newModernResourceTemplatesList(nil, testServerName, testServerVersion) + }, + }, + { + name: "prompts/list", + field: "prompts", + build: func(*testing.T) any { return newModernPromptsList(nil, testServerName, testServerVersion) }, + }, + { + name: "tools/call content", + field: "content", + build: func(*testing.T) any { + return newModernCallToolResult(&vmcp.ToolCallResult{}, testServerName, testServerVersion) + }, + }, + { + name: "resources/read contents", + field: "contents", + build: func(*testing.T) any { + return newModernReadResourceResult(&vmcp.ResourceReadResult{}, testServerName, testServerVersion) + }, + }, + { + name: "prompts/get messages", + field: "messages", + build: func(*testing.T) any { + return newModernGetPromptResult(&vmcp.PromptGetResult{}, testServerName, testServerVersion) + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(tt.build(t)) + require.NoError(t, err) + + var decoded map[string]json.RawMessage + require.NoError(t, json.Unmarshal(raw, &decoded)) + require.JSONEq(t, "[]", string(decoded[tt.field]), "%s must marshal as [] not null", tt.field) + }) + } +} + +// TestModernCallToolResult covers tools/call-specific behavior not shared +// with the other builders: isError passthrough and conditional +// structuredContent. +func TestModernCallToolResult(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + domainResult *vmcp.ToolCallResult + wantIsError bool + wantStructured bool + }{ + { + name: "success", + domainResult: &vmcp.ToolCallResult{Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "ok"}}}, + }, + { + name: "isError true passthrough", + domainResult: &vmcp.ToolCallResult{ + Content: []vmcp.Content{{Type: vmcp.ContentTypeText, Text: "boom"}}, + IsError: true, + }, + wantIsError: true, + }, + { + name: "structuredContent present when set", + domainResult: &vmcp.ToolCallResult{StructuredContent: map[string]any{"count": float64(1)}}, + wantStructured: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(newModernCallToolResult(tt.domainResult, testServerName, testServerVersion)) + require.NoError(t, err) + + var decoded map[string]any + require.NoError(t, json.Unmarshal(raw, &decoded)) + + gotIsError, _ := decoded["isError"].(bool) + require.Equal(t, tt.wantIsError, gotIsError) + + _, hasStructured := decoded["structuredContent"] + require.Equal(t, tt.wantStructured, hasStructured) + }) + } +} + +// TestModernPointerBuildersTolerateNil asserts the four pointer-domain +// builders don't panic on a nil result -- defense-in-depth against a +// plausible aggregator bug, not an expected input (see the doc comments on +// newModernCallToolResult/newModernReadResourceResult/newModernGetPromptResult/ +// newModernComplete). +func TestModernPointerBuildersTolerateNil(t *testing.T) { + t.Parallel() + + require.NotPanics(t, func() { + newModernCallToolResult(nil, "other-server", "9.9.9") + }) + require.NotPanics(t, func() { + newModernReadResourceResult(nil, "other-server", "9.9.9") + }) + require.NotPanics(t, func() { + newModernGetPromptResult(nil, "other-server", "9.9.9") + }) + require.NotPanics(t, func() { + newModernComplete(nil, "other-server", "9.9.9") + }) +} + +// TestModernComplete pins the completion/complete wire shape from +// newModernComplete against the SDK's CompleteResult/CompletionResultDetails +// (protocol.go:653,660 in go-sdk@v1.7.0-pre.3): a "completion" object with +// values/total/hasMore, resultType, and _meta.serverInfo -- no Cacheable, +// matching the SDK (CompleteResult never embeds it). JSONEq's exact-match +// means an errant Cacheable field would fail these cases on its own, without +// a separate assertion. +func TestModernComplete(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + result *vmcp.CompletionResult + want string + }{ + { + name: "nil result marshals an empty completion", + result: nil, + want: `{ + "resultType": "complete", + "completion": {"values": []}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "nil Values slice marshals as [] not null", + result: &vmcp.CompletionResult{}, + want: `{ + "resultType": "complete", + "completion": {"values": []}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "values/total/hasMore pass through", + result: &vmcp.CompletionResult{Values: []string{"a", "b"}, Total: 5, HasMore: true}, + want: `{ + "resultType": "complete", + "completion": {"values": ["a", "b"], "total": 5, "hasMore": true}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(newModernComplete(tt.result, testServerName, testServerVersion)) + require.NoError(t, err) + require.JSONEq(t, tt.want, string(raw)) + }) + } +} + +// TestModernWriters covers the three HTTP write helpers. writeModernError's +// status derives from the JSON-RPC code (404 for method-not-found, 400 for +// invalid-params, 200 for everything else -- mirroring go-sdk's +// extractErrorStatus); writeModernDenied always changes the HTTP status to +// 403 for a POLICY reason -- that's the signal the audit middleware's +// determineOutcome keys off to log outcome:"denied", unrelated to the +// protocol-level mapping above. +func TestModernWriters(t *testing.T) { + t.Parallel() + + t.Run("writeModernResult", func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + writeModernResult(rec, "req-1", map[string]any{"ok": true}) + + require.Equal(t, http.StatusOK, rec.Code) + require.Equal(t, "application/json", rec.Header().Get("Content-Type")) + require.Equal(t, "private, no-store", rec.Header().Get("Cache-Control")) + + var decoded struct { + JSONRPC string `json:"jsonrpc"` + ID string `json:"id"` + Result map[string]any `json:"result"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &decoded)) + require.Equal(t, "2.0", decoded.JSONRPC) + require.Equal(t, "req-1", decoded.ID) + require.Equal(t, true, decoded.Result["ok"]) + }) + + t.Run("writeModernError", func(t *testing.T) { + t.Parallel() + + codeToStatus := []struct { + code int + wantStatus int + }{ + {jsonRPCCodeMethodNotFound, http.StatusNotFound}, + {jsonRPCCodeInvalidParams, http.StatusBadRequest}, + {jsonRPCCodeInvalidRequest, http.StatusOK}, + {jsonRPCCodeInternalError, http.StatusOK}, + } + + for _, tc := range codeToStatus { + rec := httptest.NewRecorder() + writeModernError(rec, "req-2", tc.code, "some message") + + require.Equal(t, tc.wantStatus, rec.Code, "code %d", tc.code) + require.Equal(t, "application/json", rec.Header().Get("Content-Type")) + require.Equal(t, "private, no-store", rec.Header().Get("Cache-Control")) + + var decoded struct { + Error struct { + Code int `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &decoded)) + require.Equal(t, tc.code, decoded.Error.Code) + require.Equal(t, "some message", decoded.Error.Message) + } + }) + + t.Run("writeModernDenied", func(t *testing.T) { + t.Parallel() + + rec := httptest.NewRecorder() + writeModernDenied(rec, "req-3", "denied by policy") + + require.Equal(t, http.StatusForbidden, rec.Code, "a denial must change the HTTP status for audit outcome:\"denied\"") + require.Equal(t, "application/json", rec.Header().Get("Content-Type")) + require.Equal(t, "private, no-store", rec.Header().Get("Cache-Control")) + + var decoded struct { + Error struct { + Code int `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &decoded)) + require.Equal(t, int(mcpparser.JSONRPCCodeDenied), decoded.Error.Code) + require.Equal(t, "denied by policy", decoded.Error.Message) + }) +} + +// TestModernDiscover asserts server/discover's capability-flags shape: a +// capability field is present iff the corresponding admitted list was +// non-empty, resources/templates fold into the single "resources" flag, and +// no descriptor arrays ever appear on the wire. +func TestModernDiscover(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + hasTools, hasResources, hasTemplates, hasPrompts bool + want string + }{ + { + name: "nothing admitted -- only the static completions capability advertised", + want: `{ + "resultType": "complete", + "ttlMs": 0, + "cacheScope": "private", + "supportedVersions": ["2026-07-28", "2025-11-25"], + "capabilities": {"completions": {}}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "tools only", + hasTools: true, + want: `{ + "resultType": "complete", + "ttlMs": 0, + "cacheScope": "private", + "supportedVersions": ["2026-07-28", "2025-11-25"], + "capabilities": {"tools": {}, "completions": {}}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "resources only", + hasResources: true, + want: `{ + "resultType": "complete", + "ttlMs": 0, + "cacheScope": "private", + "supportedVersions": ["2026-07-28", "2025-11-25"], + "capabilities": {"resources": {}, "completions": {}}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "templates only still sets resources flag", + hasTemplates: true, + want: `{ + "resultType": "complete", + "ttlMs": 0, + "cacheScope": "private", + "supportedVersions": ["2026-07-28", "2025-11-25"], + "capabilities": {"resources": {}, "completions": {}}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "prompts only", + hasPrompts: true, + want: `{ + "resultType": "complete", + "ttlMs": 0, + "cacheScope": "private", + "supportedVersions": ["2026-07-28", "2025-11-25"], + "capabilities": {"prompts": {}, "completions": {}}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + { + name: "everything admitted", + hasTools: true, + hasResources: true, + hasTemplates: true, + hasPrompts: true, + want: `{ + "resultType": "complete", + "ttlMs": 0, + "cacheScope": "private", + "supportedVersions": ["2026-07-28", "2025-11-25"], + "capabilities": {"tools": {}, "resources": {}, "prompts": {}, "completions": {}}, + "_meta": {"io.modelcontextprotocol/serverInfo": {"name": "toolhive-vmcp", "version": "0.1.0"}} + }`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(newModernDiscover( + tt.hasTools, tt.hasResources, tt.hasTemplates, tt.hasPrompts, testServerName, testServerVersion, + )) + require.NoError(t, err) + require.JSONEq(t, tt.want, string(raw)) + }) + } +} + +// TestModernDescriptorFieldMapping pins the domain->wire descriptor mapping +// (the list-item shape, not the envelope wrapping it): tool name/description/ +// inputSchema (including the RawInputSchema round-trip)/outputSchema/ +// annotations, resource uri/name/mimeType, resource template uriTemplate, +// and prompt arguments[].required with omitempty. +func TestModernDescriptorFieldMapping(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + build func(t *testing.T) any + want string + }{ + { + name: "tool with annotations and output schema", + build: func(t *testing.T) any { + t.Helper() + readOnly := true + wireTool, err := modernToolFromDomain(vmcp.Tool{ + Name: "greet", + Description: "say hi", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{"name": map[string]any{"type": "string"}}, + }, + OutputSchema: map[string]any{"type": "object"}, + Annotations: &vmcp.ToolAnnotations{ReadOnlyHint: &readOnly}, + }) + require.NoError(t, err) + return wireTool + }, + want: `{ + "name": "greet", + "description": "say hi", + "inputSchema": {"type": "object", "properties": {"name": {"type": "string"}}}, + "outputSchema": {"type": "object"}, + "annotations": {"readOnlyHint": true} + }`, + }, + { + // Pins the known ponytail-noted behavior at the mapping site: a + // tool with no annotations still emits "annotations":{} because + // mcpcompat's Tool.MarshalJSON writes the field unconditionally. + name: "tool with no annotations still emits annotations:{}", + build: func(t *testing.T) any { + t.Helper() + wireTool, err := modernToolFromDomain(vmcp.Tool{ + Name: "noop", + InputSchema: map[string]any{"type": "object"}, + }) + require.NoError(t, err) + return wireTool + }, + want: `{"name": "noop", "inputSchema": {"type": "object"}, "annotations": {}}`, + }, + { + name: "resource", + build: func(*testing.T) any { + return modernResourceFromDomain(vmcp.Resource{ + Name: "info", URI: "embedded:info", Description: "info doc", MimeType: "text/plain", + }) + }, + want: `{"uri":"embedded:info","name":"info","description":"info doc","mimeType":"text/plain"}`, + }, + { + name: "resource template", + build: func(*testing.T) any { + return modernResourceTemplateFromDomain(vmcp.ResourceTemplate{ + Name: "logs", URITemplate: "file:///logs/{date}.txt", Description: "daily logs", MimeType: "text/plain", + }) + }, + want: `{"uriTemplate":"file:///logs/{date}.txt","name":"logs","description":"daily logs","mimeType":"text/plain"}`, + }, + { + name: "prompt arguments[].required with omitempty", + build: func(*testing.T) any { + return modernPromptFromDomain(vmcp.Prompt{ + Name: "code_review", + Description: "do a code review", + Arguments: []vmcp.PromptArgument{ + {Name: "Code", Required: true}, + {Name: "Language", Description: "optional hint"}, + }, + }) + }, + want: `{ + "name": "code_review", + "description": "do a code review", + "arguments": [ + {"name": "Code", "required": true}, + {"name": "Language", "description": "optional hint"} + ] + }`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + raw, err := json.Marshal(tt.build(t)) + require.NoError(t, err) + require.JSONEq(t, tt.want, string(raw)) + }) + } +} diff --git a/pkg/vmcp/server/server.go b/pkg/vmcp/server/server.go index 31271ad1c7..238f71becf 100644 --- a/pkg/vmcp/server/server.go +++ b/pkg/vmcp/server/server.go @@ -628,13 +628,13 @@ 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 and reject - // malformed Modern requests before dispatch. No routing change: Legacy - // and well-formed Modern requests both fall through to the same handler - // (Modern dispatch lands in Phase 2, #5756). Applied before telemetry - // (i.e. it runs closer to the handler) so a rejection is still recorded - // by the telemetry middleware instead of bypassing it entirely. - mcpHandler = classificationMiddleware(mcpHandler) + // Classify Modern (2026-07-28) vs Legacy at the decode seam, reject + // 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 { mcpHandler = s.config.TelemetryProvider.Middleware(s.config.Name, "streamable-http")(mcpHandler) diff --git a/pkg/vmcp/server/session_management_realbackend_integration_test.go b/pkg/vmcp/server/session_management_realbackend_integration_test.go index bb48668887..1f66f4f81c 100644 --- a/pkg/vmcp/server/session_management_realbackend_integration_test.go +++ b/pkg/vmcp/server/session_management_realbackend_integration_test.go @@ -38,6 +38,9 @@ import ( // newRealTestHandler builds the full vMCP handler backed by the MCP server at // backendURL. It is the low-level helper used by newRealTestServer and any test // that needs control over the httptest.Server configuration (e.g. WriteTimeout). +// A well-formed Modern (2026-07-28) request always routes through +// classifyingHandler -> dispatchModern; a Legacy request is unaffected and +// still falls through to the SDK. func newRealTestHandler(t *testing.T, backendURL string) http.Handler { t.Helper() @@ -238,15 +241,15 @@ func TestIntegration_RealBackend_NonSSEGetRejectedWithNotAcceptable(t *testing.T } // TestIntegration_RealBackend_ModernRequestRejectedByClassification verifies -// the classificationMiddleware wiring end-to-end through the real chain (not -// just the unit-level table covering classificationMiddleware in isolation): +// the classifyingHandler wiring end-to-end through the real chain (not +// just the unit-level table covering classifyingHandler in isolation): // a request that signals Modern (2026-07-28) via a reserved _meta key, but // carries a mismatched MCP-Protocol-Version header, is rejected with a // -32020 (HeaderMismatch) JSON-RPC error before ever reaching the backend. func TestIntegration_RealBackend_ModernRequestRejectedByClassification(t *testing.T) { t.Parallel() - // Rejected by classificationMiddleware before dispatch, so no real MCP + // Rejected by classifyingHandler before dispatch, so no real MCP // backend is needed. ts := newRealTestServer(t, "http://127.0.0.1:0") @@ -297,7 +300,7 @@ func TestIntegration_RealBackend_ModernRequestRejectedByClassification(t *testin // valid clientCapabilities, and a matching MCP-Protocol-Version header — is // still rejected with -32020 (HeaderMismatch) when its Mcp-Method HTTP header // disagrees with the JSON-RPC body's actual method. This exercises the real -// ParsingMiddleware -> classificationMiddleware flow (unlike +// ParsingMiddleware -> classifyingHandler flow (unlike // TestClassificationMiddleware in classification_test.go, which injects a // pre-built ParsedMCPRequest and bypasses ParsingMiddleware), and covers a // genuine Mcp-Method/body mismatch rather than the protocolVersion mismatch @@ -305,7 +308,7 @@ func TestIntegration_RealBackend_ModernRequestRejectedByClassification(t *testin func TestIntegration_RealBackend_ModernRequestRejectedByHeaderMismatch(t *testing.T) { t.Parallel() - // Rejected by classificationMiddleware before dispatch, so no real MCP + // Rejected by classifyingHandler before dispatch, so no real MCP // backend is needed. ts := newRealTestServer(t, "http://127.0.0.1:0") diff --git a/pkg/vmcp/server/telemetry_integration_test.go b/pkg/vmcp/server/telemetry_integration_test.go index 581cc2cf9c..80b35a7666 100644 --- a/pkg/vmcp/server/telemetry_integration_test.go +++ b/pkg/vmcp/server/telemetry_integration_test.go @@ -388,9 +388,9 @@ func TestIntegration_TelemetryMiddleware(t *testing.T) { } // TestIntegration_TelemetryRunsBeforeClassificationRejection is a regression -// guard for the middleware ordering in server.go: classificationMiddleware +// guard for the middleware ordering in server.go: classifyingHandler // must stay closer to the handler than the telemetry middleware, so a -// request that classificationMiddleware rejects is still recorded as an +// request that classifyingHandler rejects is still recorded as an // incoming request instead of being dropped before telemetry ever sees it. // If classification is ever reordered in front of telemetry, this test // starts failing because the rejected request would never reach it. @@ -469,7 +469,7 @@ func TestIntegration_TelemetryRunsBeforeClassificationRejection(t *testing.T) { // TestIntegration_RealBackend_ModernRequestRejectedByClassification: a // reserved _meta key signals Modern, but no valid protocolVersion is // present and the header names a different (Legacy) version, so - // classificationMiddleware rejects with -32020 before dispatch. + // classifyingHandler rejects with -32020 before dispatch. body := map[string]any{ "jsonrpc": "2.0", "id": 1, From 109fdc3b3b9891a07141add675df963cf1e606b0 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 23 Jul 2026 20:32:24 +0200 Subject: [PATCH 3/9] Allow-list server/discover on the single-server path Now that server/discover is served, allow-list it in pkg/authz.Middleware: DiscoverResult carries the same Capabilities/Instructions shape InitializeResult does, and initialize is already always-allowed on this path, so discover adds no new exposure class. This map governs only the single- server / proxy-runner path; vMCP's Modern dispatcher does not consult it. Co-Authored-By: Claude Opus 4.8 --- pkg/authz/middleware.go | 22 +++++++++++++++++----- pkg/authz/middleware_test.go | 21 +++++++++------------ 2 files changed, 26 insertions(+), 17 deletions(-) diff --git a/pkg/authz/middleware.go b/pkg/authz/middleware.go index 1cc93ab65d..8850ed5ba7 100644 --- a/pkg/authz/middleware.go +++ b/pkg/authz/middleware.go @@ -54,11 +54,23 @@ var MCPMethodToFeatureOperation = map[string]featureOperation{ "features/list": {Feature: "", Operation: authorizers.MCPOperationList}, // Capability discovery "roots/list": {Feature: "", Operation: ""}, // Root directory discovery - // server/discover is intentionally NOT allow-listed: it default-denies (403) for now. - // Its response enumerates tool/resource descriptors and would bypass - // ResponseFilteringWriter (which only filters tools/list, prompts/list, resources/list, - // and find_tool). When Modern serving is wired up (#5830), add it as allow + - // response-filter, not always-allowed. + // server/discover, Modern's (2026-07-28) replacement for initialize+capability + // negotiation, is always-allowed on THIS path (the single-server pkg/runner HTTP + // authz Middleware -- vMCP's Modern dispatcher never consults this map at all, it + // re-homes admission through core.Check*/core.List* directly). The always-allowed + // choice rests on initialize parity, not on any per-request filtering this map + // enforces: DiscoverResult carries the exact same Capabilities *ServerCapabilities + // (+ Instructions) shape InitializeResult does, and "initialize" above has always + // been always-allowed in this map. discover therefore adds no new exposure class -- + // note ServerCapabilities.Experimental/.Extensions (arbitrary backend-authored maps) + // and Instructions (free text) are already freeform fields a backend can populate on + // the always-allowed initialize response today, so "no descriptors" is a property of + // how vMCP's dispatcher happens to build the value, not a guarantee this wire shape + // makes on its own. Classifying it as MCPOperationList instead would be safe too -- + // response_filter.go hardcodes an exact 4-method filter list (tools/list, + // prompts/list, resources/list, find_tool), so server/discover would just pass + // through unfiltered -- but always-allowed is simpler and equally safe here. + "server/discover": {Feature: "", Operation: ""}, // Subscriptions - always allowed for now. This method carries no single resource // identifier the parser extracts (params are a notification-type filter with an diff --git a/pkg/authz/middleware_test.go b/pkg/authz/middleware_test.go index ff32beb33f..7c3ff11cb0 100644 --- a/pkg/authz/middleware_test.go +++ b/pkg/authz/middleware_test.go @@ -331,15 +331,15 @@ func TestMiddleware(t *testing.T) { expectAuthorized: false, }, { - name: "Server discover default-denies (not allow-listed)", + name: "Server discover is always allowed", method: "server/discover", params: map[string]interface{}{}, claims: jwt.MapClaims{ "sub": "user123", "name": "John Doe", }, - expectStatus: http.StatusForbidden, - expectAuthorized: false, + expectStatus: http.StatusOK, + expectAuthorized: true, }, { name: "Subscriptions listen is always allowed", @@ -467,16 +467,13 @@ func TestSubscriptionsListenIsAllowlistedPendingDelivery(t *testing.T) { require.Equal(t, featureOperation{}, MCPMethodToFeatureOperation["subscriptions/listen"]) } -// TestServerDiscoverIsNotAllowlisted guards a deliberate omission: server/discover must -// stay absent from MCPMethodToFeatureOperation so it default-denies (403) until Modern -// serving is wired up with proper response filtering (#5830). Its response enumerates -// tool/resource descriptors, and re-adding it as always-allowed would let a Cedar-restricted -// client bypass ResponseFilteringWriter and enumerate the full catalog. This test forces a -// conscious decision if someone re-adds the entry. -func TestServerDiscoverIsNotAllowlisted(t *testing.T) { +// TestServerDiscoverIsAllowlisted guards the now-safe allow-listing of server/discover: +// its Modern envelope is post-admission capability flags (booleans), never per-resource +// descriptors, so unlike tools/list or prompts/list there is nothing here for +// ResponseFilteringWriter to filter -- always-allowed is correct, not a bypass. +func TestServerDiscoverIsAllowlisted(t *testing.T) { t.Parallel() - _, ok := MCPMethodToFeatureOperation["server/discover"] - require.False(t, ok, "server/discover must not be allow-listed until Modern serving with response filtering lands (#5830)") + require.Equal(t, featureOperation{}, MCPMethodToFeatureOperation["server/discover"]) } // TestMiddlewareWithGETRequest tests that the middleware doesn't panic with GET requests. From 7071267f19029177fcc3b0c91917c65f0c76590e Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Thu, 23 Jul 2026 20:32:24 +0200 Subject: [PATCH 4/9] Add Modern stateless integration tests Drive real Modern (2026-07-28) JSON-RPC-over-HTTP through the fully assembled server into a real core aggregating a real backend, asserting the wire bytes a client sees. A hand-rolled raw HTTP client sends Modern requests (go-sdk v1.7 cannot be imported without an MVS bump). Covers tools/call round-trip with no Mcp-Session-Id, tools/list and server/discover through the admission seam, completion/complete, ping, notification 202, unknown 404/-32601, malformed arguments 400/-32602. Co-Authored-By: Claude Opus 4.8 --- .../modern_realbackend_integration_test.go | 300 ++++++++++++++++++ 1 file changed, 300 insertions(+) create mode 100644 pkg/vmcp/server/modern_realbackend_integration_test.go diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go new file mode 100644 index 0000000000..35743c5d60 --- /dev/null +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -0,0 +1,300 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server_test + +import ( + "bytes" + "context" + "encoding/json" + "io" + "maps" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --------------------------------------------------------------------------- +// Modern (2026-07-28) raw HTTP client helper +// --------------------------------------------------------------------------- + +// postModern sends a single Modern (2026-07-28) stateless JSON-RPC request to +// baseURL+"/mcp", hand-rolled per pkg/mcp/revision.go's wire contract rather +// than through go-sdk (importing it here would force an MVS bump): the +// MCP-Protocol-Version and Mcp-Method headers are mandatory on every Modern +// POST, Mcp-Name is set when non-empty (required only for tools/call, +// resources/read, prompts/get -- see nameRequiredMethods in revision.go), and +// _meta carries the reserved io.modelcontextprotocol/protocolVersion and +// clientCapabilities keys ClassifyRevision requires to admit the request as +// Modern in the first place. +// +// id == nil sends a notification: the "id" key is omitted from the body +// entirely (not set to JSON null), matching how parseMCPRequest distinguishes +// a call from a notification (dispatchModern, and jsonrpc2 before it, key off +// id being ABSENT, not nil). +// +// Returns the raw *http.Response (body re-readable: it is buffered and +// restored) alongside the JSON-RPC envelope decoded into a generic map, or a +// nil map for a body-less response (e.g. a notification's 202). +func postModern( + t *testing.T, baseURL, method string, params map[string]any, id any, mcpName string, +) (*http.Response, map[string]any) { + t.Helper() + + // Copy before mutating caller input (go-style rule): we inject _meta below, + // so clone the caller's params and any nested _meta rather than writing through. + params = maps.Clone(params) + if params == nil { + params = map[string]any{} + } + meta, _ := params["_meta"].(map[string]any) + meta = maps.Clone(meta) + if meta == nil { + meta = map[string]any{} + } + meta["io.modelcontextprotocol/protocolVersion"] = "2026-07-28" + meta["io.modelcontextprotocol/clientCapabilities"] = map[string]any{} + params["_meta"] = meta + + body := map[string]any{ + "jsonrpc": "2.0", + "method": method, + "params": params, + } + if id != nil { + body["id"] = id + } + payload, err := json.Marshal(body) + require.NoError(t, err) + + req, err := http.NewRequestWithContext( + context.Background(), http.MethodPost, baseURL+"/mcp", bytes.NewReader(payload)) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("MCP-Protocol-Version", "2026-07-28") + req.Header.Set("Mcp-Method", method) + if mcpName != "" { + req.Header.Set("Mcp-Name", mcpName) + } + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + + respBody, err := io.ReadAll(resp.Body) + require.NoError(t, err) + resp.Body.Close() + resp.Body = io.NopCloser(bytes.NewReader(respBody)) + + // Best-effort decode: a request that never reaches the Modern dispatcher + // (e.g. a malformed request rejected before dispatch) can come back as a + // plain-text error rather than JSON. Callers that expect a JSON-RPC + // envelope assert on decoded's fields directly, which fails informatively + // (nil map) if decoding didn't happen. + var decoded map[string]any + if len(respBody) > 0 { + _ = json.Unmarshal(respBody, &decoded) + } + return resp, decoded +} + +// --------------------------------------------------------------------------- +// Integration tests -- Modern (2026-07-28) stateless dispatch, real backend +// --------------------------------------------------------------------------- + +// TestIntegration_Modern_RealBackend_ToolCall verifies the load-bearing +// end-to-end round-trip: a Modern tools/call request travels through the real +// middleware chain (parsing/classification/dispatch), a real core, and a real +// backend, and comes back as a Modern envelope with no session established. +func TestIntegration_Modern_RealBackend_ToolCall(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() + + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + assert.Empty(t, resp.Header.Get("Mcp-Session-Id"), "Modern responses must never carry a session ID") + + result, ok := decoded["result"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.Equal(t, "complete", result["resultType"]) + content, ok := result["content"].([]any) + require.True(t, ok && len(content) == 1) + first := content[0].(map[string]any) + assert.Equal(t, "text", first["type"]) + assert.Equal(t, "hello modern", first["text"]) + // IsError has an omitempty JSON tag, so a successful call omits the key + // entirely rather than marshaling it as false. + assert.NotEqual(t, true, result["isError"], "tool call must not be marked as an error") +} + +// 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 := newRealTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "tools/list", nil, 1, "") + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + result, ok := decoded["result"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.Equal(t, "complete", result["resultType"]) + assert.Equal(t, "private", result["cacheScope"]) + _, hasTTL := result["ttlMs"] + assert.True(t, hasTTL, "ttlMs must be present even when zero") + + tools, ok := result["tools"].([]any) + require.True(t, ok && len(tools) == 1, "expected exactly the echo tool: %+v", result) + assert.Equal(t, "echo", tools[0].(map[string]any)["name"]) +} + +// TestIntegration_Modern_RealBackend_Discover verifies server/discover reports +// capability presence derived from the real backend's actual tool set: tools +// and completions present (the echo backend has a tool; completions is +// unconditional), resources and prompts absent (the echo backend exposes +// neither). +func TestIntegration_Modern_RealBackend_Discover(t *testing.T) { + t.Parallel() + + backendURL := startRealMCPBackend(t) + ts := newRealTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "server/discover", nil, 1, "") + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + result, ok := decoded["result"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.Equal(t, "private", result["cacheScope"]) + + caps, ok := result["capabilities"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + _, hasTools := caps["tools"] + _, hasCompletions := caps["completions"] + _, hasResources := caps["resources"] + _, hasPrompts := caps["prompts"] + assert.True(t, hasTools, "echo backend has a tool") + assert.True(t, hasCompletions, "completions is advertised unconditionally") + assert.False(t, hasResources, "echo backend exposes no resources") + assert.False(t, hasPrompts, "echo backend exposes no prompts") +} + +// TestIntegration_Modern_RealBackend_Complete verifies completion/complete +// routes to the core rather than 404ing as an unknown method. The echo +// backend has no prompts, so the referenced name is unroutable; core.Complete +// treats that leniently (empty candidates, not an error -- see +// coreVMCP.Complete), so this asserts a clean 200 completion object rather +// than a protocol-level rejection. +func TestIntegration_Modern_RealBackend_Complete(t *testing.T) { + t.Parallel() + + backendURL := startRealMCPBackend(t) + ts := newRealTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "completion/complete", map[string]any{ + "ref": map[string]any{"type": "ref/prompt", "name": "nonexistent"}, + "argument": map[string]any{"name": "a", "value": ""}, + }, 1, "") + defer resp.Body.Close() + + require.NotEqual(t, http.StatusNotFound, resp.StatusCode, + "completion/complete must not be treated as an unknown method: decoded: %+v", decoded) + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + result, ok := decoded["result"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + completion, ok := result["completion"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.Equal(t, []any{}, completion["values"], "unroutable ref yields empty candidates, not an error") +} + +// TestIntegration_Modern_RealBackend_Ping verifies ping returns a bare +// {"jsonrpc":"2.0","id":..,"result":{}} -- no resultType, no _meta -- per +// dispatchModern's documented deliberate bypass of the envelope builders for +// this method. +func TestIntegration_Modern_RealBackend_Ping(t *testing.T) { + t.Parallel() + + backendURL := startRealMCPBackend(t) + ts := newRealTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "ping", nil, 7, "") + defer resp.Body.Close() + + require.Equal(t, http.StatusOK, resp.StatusCode, "decoded: %+v", decoded) + assert.Equal(t, map[string]any{ + "jsonrpc": "2.0", + "id": float64(7), + "result": map[string]any{}, + }, decoded) +} + +// TestIntegration_Modern_RealBackend_Notification verifies a Modern request +// with no "id" (a notification) is acknowledged with 202 and no body, per +// dispatchModern's ID-nil check -- which runs before any method dispatch. +func TestIntegration_Modern_RealBackend_Notification(t *testing.T) { + t.Parallel() + + backendURL := startRealMCPBackend(t) + ts := newRealTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "tools/list", nil, nil, "") + defer resp.Body.Close() + + assert.Equal(t, http.StatusAccepted, resp.StatusCode) + assert.Nil(t, decoded) + leftover, err := io.ReadAll(resp.Body) + require.NoError(t, err) + assert.Empty(t, leftover, "a notification response must carry no body") +} + +// TestIntegration_Modern_RealBackend_UnknownMethod verifies a syntactically +// well-formed Modern request naming a method dispatchModern does not +// recognize is rejected with 404 + JSON-RPC -32601, per the draft spec's +// MUST-404-unimplemented-method rule (writeModernError). +func TestIntegration_Modern_RealBackend_UnknownMethod(t *testing.T) { + t.Parallel() + + backendURL := startRealMCPBackend(t) + ts := newRealTestServer(t, backendURL) + + resp, decoded := postModern(t, ts.URL, "resources/subscribe", map[string]any{"uri": "file:///x"}, 1, "") + defer resp.Body.Close() + + require.Equal(t, http.StatusNotFound, resp.StatusCode, "decoded: %+v", decoded) + errObj, ok := decoded["error"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.EqualValues(t, -32601, errObj["code"]) +} + +// TestIntegration_Modern_RealBackend_MalformedArguments verifies a +// syntactically valid tools/call whose "arguments" is present but not a JSON +// object is rejected with 400 + JSON-RPC -32602, per hasNonObjectArguments' +// pre-dispatch shape check. +func TestIntegration_Modern_RealBackend_MalformedArguments(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": "not-an-object", + }, 1, "echo") + defer resp.Body.Close() + + require.Equal(t, http.StatusBadRequest, resp.StatusCode, "decoded: %+v", decoded) + errObj, ok := decoded["error"].(map[string]any) + require.True(t, ok, "decoded: %+v", decoded) + assert.EqualValues(t, -32602, errObj["code"]) +} From 37023ee757dcbb11b593a6b5d0d328e5a4a32448 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 24 Jul 2026 05:48:23 +0000 Subject: [PATCH 5/9] Reject header-less Modern requests with -32020 A request that classifies Modern (2026-07-28) via its reserved io.modelcontextprotocol/* _meta keys but omits the mandatory MCP-Protocol-Version HTTP header was dispatched and answered 200. The draft Streamable HTTP "Server Validation" rules make a missing required standard header a -32020 rejection, so a header-less Modern POST must be refused, not served. ClassifyRevision is transport-agnostic (it also serves header-less stdio) and deliberately defers the header-presence rule to the HTTP layer, per its own TODO. Enforce it in classifyingHandler: once a request classifies Modern with no error, an empty MCP-Protocol-Version header is rejected as -32020 before dispatch. This touches only the otherwise-accepted path, so the existing -32602/-32022/-32020 rejection codes are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79 --- pkg/vmcp/server/classification.go | 42 ++++++++++++++++++++++++++ pkg/vmcp/server/classification_test.go | 13 ++++++++ 2 files changed, 55 insertions(+) diff --git a/pkg/vmcp/server/classification.go b/pkg/vmcp/server/classification.go index daebaf49e0..f7460c3907 100644 --- a/pkg/vmcp/server/classification.go +++ b/pkg/vmcp/server/classification.go @@ -47,6 +47,19 @@ func (s *Server) classifyingHandler(next http.Handler) http.Handler { return } + // A Modern (2026-07-28) request over HTTP MUST carry the + // MCP-Protocol-Version header (draft Streamable HTTP "Server Validation"). + // ClassifyRevision is transport-agnostic -- it also serves header-less + // stdio -- so it admits a Modern request signaled only by the reserved + // io.modelcontextprotocol/* _meta keys and defers the header-presence rule + // to the HTTP layer (see the TODO in pkg/mcp/revision.go). Without this + // check a well-formed Modern _meta with no header would dispatch and return + // 200 instead of the mandated -32020 rejection. + if protoHeader == "" { + mcpparser.WriteClassificationError(w, parsed.ID, errMissingProtocolVersionHeader) + return + } + if err := mcpparser.ValidateHeaderConsistency(parsed); err != nil { mcpparser.WriteClassificationError(w, parsed.ID, err) return @@ -55,3 +68,32 @@ func (s *Server) classifyingHandler(next http.Handler) http.Handler { s.dispatchModern(w, r, parsed) }) } + +// missingProtocolVersionHeaderError is returned when a request classifies Modern +// (2026-07-28) over HTTP but omits the mandatory MCP-Protocol-Version header. The +// draft Streamable HTTP "Server Validation" rules make a missing required standard +// header a -32020 (HeaderMismatch) condition, so this maps to the same wire code +// and HTTP 400 as the header/body mismatch mcp.ClassifyRevision already produces +// when the header is present but wrong. +// +// The enforcement lives here, at the HTTP layer, rather than in the +// transport-agnostic mcp.ClassifyRevision (which also serves header-less stdio and +// cannot know the header was required); see the TODO in pkg/mcp/revision.go. It +// implements mcp.CodedError so mcp.WriteClassificationError renders it correctly. +type missingProtocolVersionHeaderError struct{} + +func (missingProtocolVersionHeaderError) Error() string { + return "MCP-Protocol-Version header is required for Modern (2026-07-28) requests" +} + +// Code implements mcp.CodedError. +func (missingProtocolVersionHeaderError) Code() int64 { return mcpparser.CodeHeaderMismatch } + +// Data implements mcp.CodedError. +func (missingProtocolVersionHeaderError) Data() map[string]any { + return map[string]any{"header": "MCP-Protocol-Version"} +} + +// errMissingProtocolVersionHeader is the singleton rejection for a Modern HTTP +// request that omits the mandatory MCP-Protocol-Version header. +var errMissingProtocolVersionHeader = missingProtocolVersionHeaderError{} diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index f530e0c95a..42219de90c 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -95,6 +95,19 @@ func TestClassifyingHandler(t *testing.T) { protocolHeader: mcpparser.MCPVersionModern, wantDispatched: true, }, + { + // A body that is otherwise a well-formed Modern request (valid _meta + // protocolVersion + clientCapabilities) but omits the mandatory + // MCP-Protocol-Version header MUST be rejected with -32020, not + // dispatched: the draft Streamable HTTP spec requires the header on + // every Modern POST. Without the header-presence check in + // classifyingHandler this would classify Modern with a nil error and + // return 200. + name: "well-formed modern body missing the protocol version header is rejected", + parsed: wellFormedModernToolsList(), + protocolHeader: "", + wantCode: mcpparser.CodeHeaderMismatch, + }, { // initialize is forced Legacy unconditionally (ClassifyRevision), even // with a full spoofed Modern signal on both header and _meta -- mirrors From 87d918d84bb8b8a2efc59df953dd470b4c552500 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 24 Jul 2026 05:48:23 +0000 Subject: [PATCH 6/9] Test Modern authz denial through assembled server The Modern stateless dispatcher bypasses the SDK server and its CallGate, so it re-homes the pre-dispatch authorization gate itself. That gate was covered only by dispatchModern unit tests; the assembled-server denial path (audit -> parsing -> classification -> dispatchModern) had no coverage, unlike the Legacy path. Add the Modern counterpart of TestIntegration_CedarAuthzDenialIsAudited: a policy-denied Modern tools/call must return HTTP 403 + JSON-RPC 403 and be audited with outcome "denied", proving the audit middleware wraps the re-homed gate exactly as it wraps the SDK CallGate. Drop the now-constant eventType parameter from the shared audit-log helper. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79 --- pkg/vmcp/server/authz_integration_test.go | 55 ++++++++++++++++++++--- 1 file changed, 49 insertions(+), 6 deletions(-) diff --git a/pkg/vmcp/server/authz_integration_test.go b/pkg/vmcp/server/authz_integration_test.go index b9ba48d4a8..1c9c404a80 100644 --- a/pkg/vmcp/server/authz_integration_test.go +++ b/pkg/vmcp/server/authz_integration_test.go @@ -468,17 +468,60 @@ func TestIntegration_CedarAuthzDenialIsAudited(t *testing.T) { // The audit event is written after the response is flushed, so poll briefly. require.Eventually(t, func() bool { - return findAuditEvent(t, auditLogPath, "mcp_tool_call") != nil + return findToolCallAuditEvent(t, auditLogPath) != nil }, 5*time.Second, 50*time.Millisecond, "a tools/call audit event must be emitted for the denied call") - event := findAuditEvent(t, auditLogPath, "mcp_tool_call") + event := findToolCallAuditEvent(t, auditLogPath) assert.Equal(t, "denied", event["outcome"], "a policy-denied tools/call must be audited with outcome denied") } -// findAuditEvent reads the newline-delimited JSON audit log at path and returns -// the first event whose "type" matches eventType, or nil if none is present yet. -func findAuditEvent(t *testing.T, path string, eventType string) map[string]any { +// TestIntegration_CedarAuthzDenial_ModernPath_IsAudited is the Modern (2026-07-28) +// counterpart of TestIntegration_CedarAuthzDenialIsAudited. The Modern stateless +// dispatcher bypasses the SDK server (and its CallGate), so it re-homes the +// pre-dispatch authorization gate itself; this proves that gate end-to-end through +// the fully assembled server rather than only in the dispatchModern unit tests. A +// policy-denied Modern tools/call must surface as HTTP 403 + JSON-RPC 403 AND be +// audited with outcome "denied" (audit -> parsing -> classification -> +// dispatchModern), confirming the audit middleware wraps the re-homed gate exactly +// as it wraps the SDK CallGate on the Legacy path. +func TestIntegration_CedarAuthzDenial_ModernPath_IsAudited(t *testing.T) { + t.Parallel() + + backendURL := startRealMCPBackend(t) + auditLogPath := filepath.Join(t.TempDir(), "audit.log") + // Permit only an unrelated tool: "echo" is default-denied, so the re-homed gate + // in dispatchModern rejects the call before it reaches the backend. + ts := buildCedarAuthzServer(t, backendURL, nil, + &audit.Config{Component: "vmcp-server", LogFile: auditLogPath}, + `permit(principal, action == Action::"call_tool", resource == Tool::"unrelated");`) + + 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() + + require.Equal(t, http.StatusForbidden, resp.StatusCode, "decoded: %+v", decoded) + errObj, ok := decoded["error"].(map[string]any) + require.True(t, ok, "the 403 must carry a JSON-RPC error envelope: %+v", decoded) + assert.EqualValues(t, mcpparser.JSONRPCCodeDenied, errObj["code"], + "a policy-denied Modern tools/call must surface JSON-RPC 403") + + // The audit event is written after the response is flushed, so poll briefly. + require.Eventually(t, func() bool { + return findToolCallAuditEvent(t, auditLogPath) != nil + }, 5*time.Second, 50*time.Millisecond, + "a tools/call audit event must be emitted for the denied Modern call") + + event := findToolCallAuditEvent(t, auditLogPath) + assert.Equal(t, "denied", event["outcome"], + "a policy-denied Modern tools/call must be audited with outcome denied") +} + +// findToolCallAuditEvent reads the newline-delimited JSON audit log at path and +// returns the first "mcp_tool_call" event, or nil if none is present yet. +func findToolCallAuditEvent(t *testing.T, path string) map[string]any { t.Helper() data, err := os.ReadFile(path) @@ -493,7 +536,7 @@ func findAuditEvent(t *testing.T, path string, eventType string) map[string]any if err := json.Unmarshal([]byte(line), &event); err != nil { continue } - if event["type"] == eventType { + if event["type"] == "mcp_tool_call" { return event } } From b401fb613af3d341910fd98996fdd459e70612fa Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 24 Jul 2026 08:27:01 +0200 Subject: [PATCH 7/9] Address review nits on the Modern dispatch path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - List/discover -32603 responses now return a generic "internal error" to the client and log the full error server-side, instead of echoing wrapped aggregation/routing detail (backend IDs, upstream addressing) — the Legacy path never surfaces a list error to the client at all, and security.md forbids leaking internal plumbing. tools/call/resources/read/prompts/get and completion keep err.Error() (documented SDK/Legacy parity). - Drop the unreachable result==nil guards in the call/read/get envelope builders (callers deref result.BackendID first, so a nil panics upstream); newModernComplete keeps its guard, which is live. - Clarify gateDenied's comment: only tools/call's CheckToolCall re-aggregates and can fail open on a non-authz error; CheckResourceRead/CheckPromptGet always classify as ErrAuthorizationFailed. Co-Authored-By: Claude Opus 4.8 --- pkg/vmcp/server/modern_dispatch.go | 41 +++++++++++++++++++------ pkg/vmcp/server/modern_envelope.go | 27 ++++++---------- pkg/vmcp/server/modern_envelope_test.go | 23 +++++--------- 3 files changed, 48 insertions(+), 43 deletions(-) diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index 66e2adac01..318e737ff4 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -117,17 +117,23 @@ func (s *Server) dispatchModern(w http.ResponseWriter, r *http.Request, parsed * // is unimplemented, and any cursor a Modern client sends is ignored. This is // unrelated to the aggregator's UPSTREAM cursor-following for internal // discovery (#5851); that's a different layer. +// +// A List*/Discover failure logs the full error server-side and returns a +// generic -32603 message to the client (writeModernListError below): unlike +// the call/read/get verbs, these errors come from aggregation and routing +// plumbing (backend IDs, upstream addressing), and security.md forbids +// leaking that detail to callers. func (s *Server) dispatchModernToolsList( ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, ) { tools, err := s.core.ListTools(ctx, identity) if err != nil { - writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } result, err := newModernToolsList(tools, s.config.Name, s.config.Version) if err != nil { - writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } writeModernResult(w, parsed.ID, result) @@ -138,7 +144,7 @@ func (s *Server) dispatchModernResourcesList( ) { resources, err := s.core.ListResources(ctx, identity) if err != nil { - writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } writeModernResult(w, parsed.ID, newModernResourcesList(resources, s.config.Name, s.config.Version)) @@ -149,7 +155,7 @@ func (s *Server) dispatchModernResourceTemplatesList( ) { templates, err := s.core.ListResourceTemplates(ctx, identity) if err != nil { - writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } writeModernResult(w, parsed.ID, newModernResourceTemplatesList(templates, s.config.Name, s.config.Version)) @@ -160,7 +166,7 @@ func (s *Server) dispatchModernPromptsList( ) { prompts, err := s.core.ListPrompts(ctx, identity) if err != nil { - writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } writeModernResult(w, parsed.ID, newModernPromptsList(prompts, s.config.Name, s.config.Version)) @@ -189,7 +195,7 @@ func (s *Server) dispatchModernDiscover( ) { caps, err := s.core.Discover(ctx, identity) if err != nil { - writeModernError(w, parsed.ID, jsonRPCCodeInternalError, err.Error()) + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } result := newModernDiscover( @@ -386,10 +392,15 @@ func hasNonObjectArguments(params json.RawMessage) bool { // gateDenied runs the PRE-dispatch admission classification for a gated // method's Check* result, mirroring authzCallGate exactly: only an // errors.Is(checkErr, vmcp.ErrAuthorizationFailed) denial returns true. Any -// other error is infrastructure (aggregation/backend plumbing), so the gate -// fails OPEN -- it logs and admits, rather than converting an authorizer -// outage into a false 403. This WARN is the only operational signal of that -// outage admitting traffic; do not remove it. +// other error falls through to the WARN+admit branch below, but only +// CheckToolCall can actually produce one: it re-aggregates +// (c.aggregatedView) and returns that error unwrapped on failure, so a +// tools/call gate can fail OPEN on an aggregation/backend-plumbing outage. +// CheckResourceRead and CheckPromptGet need no aggregated view and always +// wrap their error as vmcp.ErrAuthorizationFailed (core_checks.go), so their +// gates never take this fail-open path in practice. This WARN is the only +// operational signal of that fail-open outage admitting traffic; do not +// remove it. func gateDenied(ctx context.Context, method string, checkErr error) bool { if checkErr == nil { return false @@ -402,6 +413,16 @@ func gateDenied(ctx context.Context, method string, checkErr error) bool { return false } +// writeModernListError logs a List*/Discover failure server-side with the +// full error and writes a generic -32603 message to the client. Unlike +// writeModernDispatchError's call/read/get verbs, these errors surface +// aggregation and routing plumbing (backend IDs, upstream addressing), and +// security.md forbids exposing that detail to callers. +func writeModernListError(ctx context.Context, w http.ResponseWriter, id any, method string, err error) { + slog.ErrorContext(ctx, "vmcp modern dispatch: list/discover failed", "method", method, "error", err) + writeModernError(w, id, jsonRPCCodeInternalError, "internal error") +} + // writeModernDispatchError classifies a POST-dispatch error from // CallTool/ReadResource/GetPrompt. Check* and the real call each re-aggregate // independently (documented "aggregates twice" on CheckToolCall), so a diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 5fefc8968e..dddc8b1c5f 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -357,13 +357,10 @@ func newModernPromptsList(prompts []vmcp.Prompt, serverName, serverVersion strin // omitempty-false) when the core did not set it, matching the SDK's // omitempty behavior. // -// result is expected non-nil (core.CallTool never returns a nil result -// without an error); the nil check below is defense-in-depth against a -// plausible aggregator bug, not an expected input. +// result is non-nil on every call: dispatchModernToolCall (modern_dispatch.go) +// dereferences result.BackendID before calling this builder, so a nil result +// would already have panicked upstream. func newModernCallToolResult(result *vmcp.ToolCallResult, serverName, serverVersion string) modernCallToolResult { - if result == nil { - result = &vmcp.ToolCallResult{} - } var structuredContent any if len(result.StructuredContent) > 0 { structuredContent = result.StructuredContent @@ -380,15 +377,12 @@ func newModernCallToolResult(result *vmcp.ToolCallResult, serverName, serverVers // newModernReadResourceResult builds the resources/read wire result from the // core's ResourceReadResult. // -// result is expected non-nil (core.ReadResource never returns a nil result -// without an error); the nil check below is defense-in-depth against a -// plausible aggregator bug, not an expected input. +// result is non-nil on every call: dispatchModernResourceRead +// (modern_dispatch.go) dereferences result.BackendID before calling this +// builder, so a nil result would already have panicked upstream. func newModernReadResourceResult( result *vmcp.ResourceReadResult, serverName, serverVersion string, ) modernReadResourceResult { - if result == nil { - result = &vmcp.ResourceReadResult{} - } return modernReadResourceResult{ ResultType: modernResultTypeComplete, modernCacheable: newModernCacheable(), @@ -400,13 +394,10 @@ func newModernReadResourceResult( // newModernGetPromptResult builds the prompts/get wire result from the // core's PromptGetResult. // -// result is expected non-nil (core.GetPrompt never returns a nil result -// without an error); the nil check below is defense-in-depth against a -// plausible aggregator bug, not an expected input. +// result is non-nil on every call: dispatchModernPromptGet +// (modern_dispatch.go) dereferences result.BackendID before calling this +// builder, so a nil result would already have panicked upstream. func newModernGetPromptResult(result *vmcp.PromptGetResult, serverName, serverVersion string) modernGetPromptResult { - if result == nil { - result = &vmcp.PromptGetResult{} - } return modernGetPromptResult{ ResultType: modernResultTypeComplete, Description: result.Description, diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index 51ee9ffd4f..22c6aa594f 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -347,23 +347,16 @@ func TestModernCallToolResult(t *testing.T) { } } -// TestModernPointerBuildersTolerateNil asserts the four pointer-domain -// builders don't panic on a nil result -- defense-in-depth against a -// plausible aggregator bug, not an expected input (see the doc comments on -// newModernCallToolResult/newModernReadResourceResult/newModernGetPromptResult/ -// newModernComplete). -func TestModernPointerBuildersTolerateNil(t *testing.T) { +// TestModernCompleteTolerateNil asserts newModernComplete doesn't panic on a +// nil result -- defense-in-depth against a plausible aggregator bug, not an +// expected input (see its doc comment). newModernCallToolResult, +// newModernReadResourceResult, and newModernGetPromptResult carry no +// equivalent guard: their callers in modern_dispatch.go dereference +// result.BackendID before invoking the builder, so a nil result already +// panics upstream. +func TestModernCompleteTolerateNil(t *testing.T) { t.Parallel() - require.NotPanics(t, func() { - newModernCallToolResult(nil, "other-server", "9.9.9") - }) - require.NotPanics(t, func() { - newModernReadResourceResult(nil, "other-server", "9.9.9") - }) - require.NotPanics(t, func() { - newModernGetPromptResult(nil, "other-server", "9.9.9") - }) require.NotPanics(t, func() { newModernComplete(nil, "other-server", "9.9.9") }) From 8dc447661840a8ca25406792b13a8aca4ecc3300 Mon Sep 17 00:00:00 2001 From: Jakub Hrozek Date: Fri, 24 Jul 2026 08:46:08 +0200 Subject: [PATCH 8/9] Restore default-off Modern dispatch kill-switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per maintainer review (#5953): serving Modern (2026-07-28) requests unconditionally is risky while the wire envelope is hand-rolled against go-sdk v1.7.0-pre.3 — a version this module does not import, so the shapes have no compile-time check and are validated only against hand-written test JSON, not real SDK bytes. Restore a startup kill-switch so an operator can fall back to the SDK path (a safe version-downgrade) via config rather than revert-and-redeploy, until Modern is conformance-validated. classifyingHandler dispatches a well-formed Modern request to the core only when Config.ModernDispatchEnabled is set (default false); otherwise it falls through to the SDK path. Fed once at the composition root from TOOLHIVE_VMCP_MODERN_STATELESS (unset/invalid -> off, with a warning). Legacy and malformed-Modern behavior is unchanged (the gate sits after classification). The Modern dispatch/integration tests enable the switch; a KillSwitchOff case asserts fall-through. This kill-switch is temporary and tracked for removal by #5959 (once the #5837 conformance harness lands or mcpcompat adopts go-sdk v1.7). Co-Authored-By: Claude Opus 4.8 --- pkg/vmcp/cli/serve.go | 23 ++++++++ pkg/vmcp/server/classification.go | 17 ++++-- pkg/vmcp/server/classification_test.go | 51 +++++++++++------- pkg/vmcp/server/derive.go | 1 + pkg/vmcp/server/derive_test.go | 2 + pkg/vmcp/server/modern_dispatch_test.go | 2 +- .../modern_realbackend_integration_test.go | 39 +++++++++++--- pkg/vmcp/server/serve.go | 6 +++ pkg/vmcp/server/serve_test.go | 1 + pkg/vmcp/server/server.go | 23 ++++++-- ...management_realbackend_integration_test.go | 52 ++++++++++++++----- 11 files changed, 169 insertions(+), 48 deletions(-) diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index e25bdc0a64..b4cf72e601 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -15,6 +15,7 @@ import ( "net" "os" "path/filepath" + "strconv" "time" "go.opentelemetry.io/otel/trace" @@ -51,6 +52,12 @@ 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 @@ -408,6 +415,21 @@ 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 (e.g. "ture"), + // 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 @@ -420,6 +442,7 @@ 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/server/classification.go b/pkg/vmcp/server/classification.go index f7460c3907..4e4a66a283 100644 --- a/pkg/vmcp/server/classification.go +++ b/pkg/vmcp/server/classification.go @@ -11,10 +11,11 @@ import ( // 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 routes a -// well-formed Modern request to dispatchModern instead of the SDK. Modern -// dispatch is unconditional: a well-formed Modern request always reaches -// dispatchModern. Legacy traffic always falls through to next unchanged. +// 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, as does a well-formed Modern request while the switch is +// off (default), matching pre-Modern-dispatch wire behavior byte for byte. // // ValidateHeaderConsistency (Mcp-Method/Mcp-Name) only applies to Modern // requests: a Legacy request carrying a stray Mcp-Method/Mcp-Name header @@ -65,6 +66,14 @@ func (s *Server) classifyingHandler(next http.Handler) http.Handler { return } + // TEMPORARY kill-switch (default off): until Modern dispatch is + // conformance-validated, a well-formed Modern request falls through to + // the SDK path unless explicitly enabled. See issue #5959. + if !s.config.ModernDispatchEnabled { + next.ServeHTTP(w, r) + return + } + s.dispatchModern(w, r, parsed) }) } diff --git a/pkg/vmcp/server/classification_test.go b/pkg/vmcp/server/classification_test.go index 42219de90c..5e15a59bfa 100644 --- a/pkg/vmcp/server/classification_test.go +++ b/pkg/vmcp/server/classification_test.go @@ -38,13 +38,15 @@ type classificationErrorBody struct { } // classifyingHandlerTestServer builds a minimal *Server for driving -// classifyingHandler in isolation, carrying only the field the handler reads -// beyond config scalars: the core a well-formed Modern request dispatches to. -func classifyingHandlerTestServer() *Server { +// 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 { return &Server{ config: &Config{ - Name: testServerName, - Version: testServerVersion, + Name: testServerName, + Version: testServerVersion, + ModernDispatchEnabled: modernDispatchEnabled, }, core: &modernFakeCore{tools: []vmcp.Tool{{Name: "echo", InputSchema: map[string]any{"type": "object"}}}}, } @@ -54,12 +56,13 @@ func TestClassifyingHandler(t *testing.T) { t.Parallel() tests := []struct { - name string - parsed *mcpparser.ParsedMCPRequest - protocolHeader string - wantPassthrough bool - wantDispatched bool - wantCode int64 + name string + parsed *mcpparser.ParsedMCPRequest + protocolHeader string + modernDispatchEnabled bool + wantPassthrough bool + wantDispatched bool + wantCode int64 }{ { name: "nil parsed request passes through", @@ -88,12 +91,24 @@ 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; a well-formed Modern request then dispatches - // to the core unconditionally rather than falling through to next. - name: "well-formed modern request dispatches to the core", - parsed: wellFormedModernToolsList(), - protocolHeader: mcpparser.MCPVersionModern, - wantDispatched: true, + // 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): dispatch must not happen and the request falls + // through to the SDK path unchanged, byte-identical to pre-Modern- + // dispatch wire behavior. + name: "well-formed modern request falls through to next when the kill-switch is off", + parsed: wellFormedModernToolsList(), + protocolHeader: mcpparser.MCPVersionModern, + wantPassthrough: true, }, { // A body that is otherwise a well-formed Modern request (valid _meta @@ -244,7 +259,7 @@ func TestClassifyingHandler(t *testing.T) { }) rec := httptest.NewRecorder() - classifyingHandlerTestServer().classifyingHandler(next).ServeHTTP(rec, req) + classifyingHandlerTestServer(tt.modernDispatchEnabled).classifyingHandler(next).ServeHTTP(rec, req) if tt.wantPassthrough { assert.True(t, nextCalled, "expected the request to fall through to next") diff --git a/pkg/vmcp/server/derive.go b/pkg/vmcp/server/derive.go index a1799babc7..15310ad52e 100644 --- a/pkg/vmcp/server/derive.go +++ b/pkg/vmcp/server/derive.go @@ -77,6 +77,7 @@ 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 645d8cddbc..a031e09aad 100644 --- a/pkg/vmcp/server/derive_test.go +++ b/pkg/vmcp/server/derive_test.go @@ -40,6 +40,7 @@ func populatedLegacyConfig() *Config { EndpointPath: "/custom", SessionTTL: 17 * time.Minute, HeartbeatInterval: 5 * time.Second, + ModernDispatchEnabled: true, AuthMiddleware: passthrough, AuthzMiddleware: passthrough, AuthInfoHandler: http.NewServeMux(), @@ -72,6 +73,7 @@ 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/modern_dispatch_test.go b/pkg/vmcp/server/modern_dispatch_test.go index f659eb559f..b606a6e4df 100644 --- a/pkg/vmcp/server/modern_dispatch_test.go +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -269,7 +269,7 @@ func TestDispatchModern_PingRealParser(t *testing.T) { req.Header.Set("MCP-Protocol-Version", mcpparser.MCPVersionModern) req.Header.Set("Mcp-Method", "ping") - s := classifyingHandlerTestServer() + s := classifyingHandlerTestServer(true) nextCalled := false next := http.HandlerFunc(func(http.ResponseWriter, *http.Request) { nextCalled = true }) diff --git a/pkg/vmcp/server/modern_realbackend_integration_test.go b/pkg/vmcp/server/modern_realbackend_integration_test.go index 35743c5d60..9514c3228a 100644 --- a/pkg/vmcp/server/modern_realbackend_integration_test.go +++ b/pkg/vmcp/server/modern_realbackend_integration_test.go @@ -111,7 +111,7 @@ func TestIntegration_Modern_RealBackend_ToolCall(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ "name": "echo", @@ -135,13 +135,36 @@ 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 := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/list", nil, 1, "") defer resp.Body.Close() @@ -168,7 +191,7 @@ func TestIntegration_Modern_RealBackend_Discover(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "server/discover", nil, 1, "") defer resp.Body.Close() @@ -200,7 +223,7 @@ func TestIntegration_Modern_RealBackend_Complete(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "completion/complete", map[string]any{ "ref": map[string]any{"type": "ref/prompt", "name": "nonexistent"}, @@ -226,7 +249,7 @@ func TestIntegration_Modern_RealBackend_Ping(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "ping", nil, 7, "") defer resp.Body.Close() @@ -246,7 +269,7 @@ func TestIntegration_Modern_RealBackend_Notification(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/list", nil, nil, "") defer resp.Body.Close() @@ -266,7 +289,7 @@ func TestIntegration_Modern_RealBackend_UnknownMethod(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "resources/subscribe", map[string]any{"uri": "file:///x"}, 1, "") defer resp.Body.Close() @@ -285,7 +308,7 @@ func TestIntegration_Modern_RealBackend_MalformedArguments(t *testing.T) { t.Parallel() backendURL := startRealMCPBackend(t) - ts := newRealTestServer(t, backendURL) + ts := newRealModernTestServer(t, backendURL) resp, decoded := postModern(t, ts.URL, "tools/call", map[string]any{ "name": "echo", diff --git a/pkg/vmcp/server/serve.go b/pkg/vmcp/server/serve.go index fa8ce773c8..0f7ca08aa7 100644 --- a/pkg/vmcp/server/serve.go +++ b/pkg/vmcp/server/serve.go @@ -66,6 +66,11 @@ 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 @@ -399,6 +404,7 @@ 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 cdfc7ebd22..e75ce34dff 100644 --- a/pkg/vmcp/server/serve_test.go +++ b/pkg/vmcp/server/serve_test.go @@ -363,6 +363,7 @@ 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 238f71becf..354f7d9dbc 100644 --- a/pkg/vmcp/server/server.go +++ b/pkg/vmcp/server/server.go @@ -151,6 +151,18 @@ 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). @@ -629,11 +641,12 @@ 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 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 — 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. 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 1f66f4f81c..e95c534a77 100644 --- a/pkg/vmcp/server/session_management_realbackend_integration_test.go +++ b/pkg/vmcp/server/session_management_realbackend_integration_test.go @@ -36,13 +36,29 @@ 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. It is the low-level helper used by newRealTestServer and any test -// that needs control over the httptest.Server configuration (e.g. WriteTimeout). -// A well-formed Modern (2026-07-28) request always routes through -// classifyingHandler -> dispatchModern; a Legacy request is unaffected and -// still falls through to the SDK. +// 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. 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) @@ -84,11 +100,12 @@ func newRealTestHandler(t *testing.T, backendURL string) http.Handler { srv, err := server.New( context.Background(), &server.Config{ - Host: "127.0.0.1", - Port: 0, - SessionTTL: 5 * time.Minute, - SessionFactory: factory, - Aggregator: agg, + Host: "127.0.0.1", + Port: 0, + SessionTTL: 5 * time.Minute, + ModernDispatchEnabled: modernDispatchEnabled, + SessionFactory: factory, + Aggregator: agg, }, rt, backendClient, @@ -103,8 +120,9 @@ func newRealTestHandler(t *testing.T, backendURL string) http.Handler { } // newRealTestServer builds a vMCP server with session management and a real -// SessionFactory. The BackendRegistry mock returns the backend at backendURL -// so that CreateSession() opens a real HTTP connection to the MCP server. +// 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. func newRealTestServer(t *testing.T, backendURL string) *httptest.Server { t.Helper() ts := httptest.NewServer(newRealTestHandler(t, backendURL)) @@ -112,6 +130,16 @@ 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. From efb742154bd530d97d8e0301d6d35ba76d1c8f41 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 24 Jul 2026 13:38:28 +0000 Subject: [PATCH 9/9] Fix codespell CI failure and tidy comments The rebased branch tripped the codespell CI check on the illustrative "ture" typo in the kill-switch comment. Reword it to avoid the flagged token rather than add "ture" to the repo-wide ignore list, which would suppress a genuinely common misspelling everywhere. Also drop the ad-hoc comment markers that are not a ToolHive convention (keeping the explanatory prose) and correct a stale comment that referenced a nextCursor envelope field that does not exist. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_016WG3mSjVGWNc8nfgbkdd79 --- pkg/vmcp/cli/serve.go | 6 +++--- pkg/vmcp/server/modern_dispatch.go | 19 ++++++++++--------- pkg/vmcp/server/modern_envelope.go | 2 +- pkg/vmcp/server/modern_envelope_test.go | 2 +- 4 files changed, 15 insertions(+), 14 deletions(-) diff --git a/pkg/vmcp/cli/serve.go b/pkg/vmcp/cli/serve.go index b4cf72e601..6282e5ff19 100644 --- a/pkg/vmcp/cli/serve.go +++ b/pkg/vmcp/cli/serve.go @@ -417,9 +417,9 @@ 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 (e.g. "ture"), - // not a request to enable the feature — warn and stay disabled rather - // than silently treating it as false. + // 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 diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index 318e737ff4..fe7d920a7f 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -56,7 +56,7 @@ func (s *Server) dispatchModern(w http.ResponseWriter, r *http.Request, parsed * return } - // ponytail: defensive/unreachable today -- ParsingMiddleware rejects a + // Defensive/unreachable today -- ParsingMiddleware rejects a // JSON-RPC batch (leading '[') with HTTP 400 / -32600 before a // ParsedMCPRequest is ever built (parser.go's IsBatchRequest check, ~line // 119), so dispatchModern never sees one and IsBatch is hardcoded false @@ -112,11 +112,12 @@ func (s *Server) dispatchModern(w http.ResponseWriter, r *http.Request, parsed * // The four list-dispatch helpers below (tools/list, resources/list, // resources/templates/list, prompts/list) always return the full -// admission-filtered set from the matching core.List* and never set the -// envelope's nextCursor (it's omitempty) -- client-facing cursor pagination -// is unimplemented, and any cursor a Modern client sends is ignored. This is -// unrelated to the aggregator's UPSTREAM cursor-following for internal -// discovery (#5851); that's a different layer. +// admission-filtered set from the matching core.List* and do not emit a +// nextCursor: the Modern list-result envelopes carry no cursor field at all +// (PaginatedResult.nextCursor is optional, so omitting it is spec-valid), +// client-facing cursor pagination is unimplemented, and any cursor a Modern +// client sends is ignored. This is unrelated to the aggregator's UPSTREAM +// cursor-following for internal discovery (#5851); that's a different layer. // // A List*/Discover failure logs the full error server-side and returns a // generic -32603 message to the client (writeModernListError below): unlike @@ -187,9 +188,9 @@ func (s *Server) dispatchModernPromptsList( // ListPrompts independently here used to cost four -- and those four weren't // even a consistent snapshot of the aggregated view). A single fan-out per // request is fine for now, but a probe the spec expects to be cheap across -// requests too. ponytail: no cross-request cache; add a short-TTL -// per-identity capability cache only if profiling shows the per-request -// fan-out cost matters (#5761, tracked separately, not blocking here). +// requests too. There is no cross-request cache; add a short-TTL per-identity +// capability cache only if profiling shows the per-request fan-out cost +// matters (#5761, tracked separately, not blocking here). func (s *Server) dispatchModernDiscover( ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, ) { diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index dddc8b1c5f..c726e7e0fe 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -419,7 +419,7 @@ func modernToolFromDomain(t vmcp.Tool) (mcp.Tool, error) { Name: t.Name, Description: t.Description, RawInputSchema: schemaJSON, - // ponytail: a tool with no annotations still marshals "annotations":{} + // A tool with no annotations still marshals "annotations":{} // because mcpcompat's Tool.MarshalJSON (mcpcompat/mcp/tools.go:343) // writes the field unconditionally. Shared with Legacy's // coreSessionTools, so the real fix belongs in mcpcompat, not here -- diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index 22c6aa594f..bbcef45416 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -644,7 +644,7 @@ func TestModernDescriptorFieldMapping(t *testing.T) { }`, }, { - // Pins the known ponytail-noted behavior at the mapping site: a + // Pins the known documented behavior at the mapping site: a // tool with no annotations still emits "annotations":{} because // mcpcompat's Tool.MarshalJSON writes the field unconditionally. name: "tool with no annotations still emits annotations:{}",