From 9d08620191f7c01b0e1e344c1939e5121b2f3276 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 19:18:54 +0000 Subject: [PATCH 1/4] Implement Modern subscriptions/listen in vMCP MCP 2026-07-28 removed the standalone HTTP GET stream, so subscriptions/ listen is the revision's only server->client push channel. vMCP's Modern dispatcher had no case for it, which made vMCP unreachable for any go-sdk v1.7 client: Connect is Modern-first, and once server/discover succeeds it opens a listen stream whenever a list-changed handler is registered -- which mcpcompat's Initialize does unconditionally. The resulting -32601 failed Connect outright and tore the session down. The honored set is computed by intersecting the client's requested notification types against the capability advertisement server/discover publishes, now extracted as newModernCapabilities so the two answers cannot drift. Every push-related flag there is deliberately false, so the honored set is empty today and the acknowledgement says so explicitly. That is the spec's own mechanism for declaring unsupported notification types, not a silent no-op: nothing is ever reported as subscribed and then dropped. No push delivery exists yet (#5743); vMCP must start advertising a push capability before this stream can carry anything. Measured against the kill-switch removal in #6033: 20 regressions before, 6 after. The remaining 6 are unrelated to this channel -- five need server-initiated elicitation/sampling that 2026-07-28 removes outright, and one needs Modern list pagination. Refs #5959, #6033, #5743 --- pkg/vmcp/server/modern_dispatch.go | 5 + pkg/vmcp/server/modern_envelope.go | 49 +++- pkg/vmcp/server/modern_subscriptions.go | 275 ++++++++++++++++++ pkg/vmcp/server/modern_subscriptions_test.go | 281 +++++++++++++++++++ 4 files changed, 598 insertions(+), 12 deletions(-) create mode 100644 pkg/vmcp/server/modern_subscriptions.go create mode 100644 pkg/vmcp/server/modern_subscriptions_test.go diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index 71075c2fa2..b253c008ae 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -97,6 +97,11 @@ func (s *Server) dispatchModern(w http.ResponseWriter, r *http.Request, parsed * s.dispatchModernPromptGet(ctx, w, parsed, identity) case "completion/complete": s.dispatchModernComplete(ctx, w, parsed, identity) + case methodSubscriptionsListen: + // Ungated, same bucket as the list verbs and discover -- see + // dispatchModernSubscriptionsListen for why, and for the one future + // change that would require gating it. + s.dispatchModernSubscriptionsListen(ctx, w, parsed, identity) case "ping": // ping is deliberately ungated (unauthenticated liveness, same bucket // as initialize -- no Check*) and carries NEITHER resultType NOR diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 53a7e1b693..4be2ccec5b 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -255,6 +255,42 @@ type modernDiscoverResult struct { func newModernDiscover( hasTools, hasResources, hasTemplates, hasPrompts bool, serverName, serverVersion string, ) modernDiscoverResult { + return modernDiscoverResult{ + ResultType: modernResultTypeComplete, + modernCacheable: newModernCacheable(), + SupportedVersions: []string{mcpparser.MCPVersionModern, mcp.LATEST_PROTOCOL_VERSION}, + Capabilities: newModernCapabilities(hasTools, hasResources, hasTemplates, hasPrompts), + Meta: newModernMeta(serverName, serverVersion), + } +} + +// newModernCapabilities shapes the four admission-filtered presence checks into +// the wire capability flags. It is the SINGLE source of truth for what this +// dispatcher advertises: server/discover publishes it verbatim (above), and +// subscriptions/listen intersects a client's requested notification set against +// it (honoredSubscriptions, modern_subscriptions.go). Keeping one builder is +// what makes those two answers impossible to drift apart -- a client can never +// be told a push capability exists by one verb and denied it by the other. +// +// Every push-related flag below is deliberately false, and each `false` is a +// load-bearing statement about what vMCP can actually do, not an oversight: +// +// - Tools/Prompts/Resources ListChanged: dispatchModern creates no session, so +// the per-session list_changed machinery (buildListChangedSink, wired only +// from handleSessionRegistrationImpl in server.go) never runs for a Modern +// client, whatever revision its backends speak. +// - Resources Subscribe: 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. vMCP does +// not forward backend resources/updated on the Legacy path either +// (ack-level only; see docs/arch/10-virtual-mcp-architecture.md). +// +// Flipping any of them to true is a promise to deliver that notification type, +// which today nothing in vMCP can keep. Implement delivery in the SAME change, +// or a Modern client will subscribe successfully and then wait forever. See +// #5743. +func newModernCapabilities(hasTools, hasResources, hasTemplates, hasPrompts bool) mcp.ServerCapabilities { var caps mcp.ServerCapabilities if hasTools { caps.Tools = &struct { @@ -262,11 +298,6 @@ func newModernDiscover( }{} } 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"` @@ -278,13 +309,7 @@ func newModernDiscover( }{} } caps.Completions = &struct{}{} - return modernDiscoverResult{ - ResultType: modernResultTypeComplete, - modernCacheable: newModernCacheable(), - SupportedVersions: []string{mcpparser.MCPVersionModern, mcp.LATEST_PROTOCOL_VERSION}, - Capabilities: caps, - Meta: newModernMeta(serverName, serverVersion), - } + return caps } // newModernToolsList builds the tools/list wire result from the core's diff --git a/pkg/vmcp/server/modern_subscriptions.go b/pkg/vmcp/server/modern_subscriptions.go new file mode 100644 index 0000000000..d77d4bb413 --- /dev/null +++ b/pkg/vmcp/server/modern_subscriptions.go @@ -0,0 +1,275 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "context" + "encoding/json" + "fmt" + "log/slog" + "net/http" + + "github.com/stacklok/toolhive-core/mcpcompat/mcp" + "github.com/stacklok/toolhive/pkg/auth" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" +) + +// This file implements MCP 2026-07-28 ("Modern") subscriptions/listen, the +// revision's ONLY server->client push channel. Modern removed the standalone +// HTTP GET stream, so a client that wants unsolicited notifications opens one +// of these instead. +// +// WHY IT EXISTS EVEN THOUGH vMCP HAS NOTHING TO PUSH. Without a handler for +// this method a go-sdk v1.7 client cannot talk to vMCP at all: its Connect is +// Modern-first, and once server/discover succeeds it opens a listen stream +// whenever any list-changed handler is registered -- which mcpcompat's +// Initialize does unconditionally (installNotificationHandlers). A -32601 there +// fails Connect outright and tears the session down. So this handler is what +// stands between a Modern client and a usable session; it is not a facility +// nobody reaches. +// +// WHAT IT HONORS: nothing, today, and it says so on the wire. The honored set is +// computed by intersecting the client's requested notification types against +// newModernCapabilities -- the same builder server/discover publishes -- every +// push-related flag of which is deliberately false (see its doc comment for why, +// per flag). An empty honored set is the spec's own way to say "I support none +// of these", not a stub: SEP-2575 specifies the acknowledgement as carrying +// "the subset of the requested notifications the server has agreed to honor", +// and requires unsupported types to be omitted from it. go-sdk's reference +// server does exactly this, filtering the requested set through its own +// capabilities (allowedSubscriptions, server.go:1254+ in go-sdk@v1.7.0-pre.3). +// +// The distinction that matters: this is a truthful NEGATIVE declaration, not an +// empty success. It never reports a subscription as established and then +// silently drops notifications -- it enumerates, in the acknowledgement, that +// nothing is honored. Real delivery is tracked separately (#5743) and requires +// vMCP to first start ADVERTISING a push capability in newModernCapabilities; +// until then there is deliberately nothing for this stream to carry. +// +// Known limitation worth stating plainly: go-sdk's client ignores the +// acknowledgement (callSubscriptionsAckHandler returns (nil, nil), +// client.go:1457-1459), so against that client this honest declaration is not +// observable in behavior. It is correct on the wire regardless, and the +// alternative -- holding a stream open forever delivering nothing -- is strictly +// worse. + +// Modern subscription method and notification names, plus the reserved _meta +// key deliveries are tagged with. +// +// Provenance, since it differs per constant: the two method/notification names +// and the per-type opt-in shape are specified in SEP-2575 prose. The +// subscriptionId key and the convention of using the listen request's own +// JSON-RPC id as the subscription identity are NOT in the spec text -- they are +// taken from go-sdk@v1.7.0-pre.3 (MetaKeySubscriptionID at protocol.go:2377-2379; +// server.go:1187-1250 keys subscriptions by the request id and stamps it into +// both the acknowledgement and the final result). That is the reference +// implementation rather than normative prose, so treat it as the de facto wire +// contract and re-check it when the SEP lands. +const ( + methodSubscriptionsListen = "subscriptions/listen" + notificationSubscriptionsAcked = "notifications/subscriptions/acknowledged" + modernSubscriptionIDKey = "io.modelcontextprotocol/subscriptionId" +) + +// notificationSubscriptions is the per-type, per-URI opt-in set carried by both +// subscriptions/listen params and the acknowledgement. It mirrors go-sdk's +// NotificationSubscriptions field-for-field (protocol.go:2070-2082); mcpcompat +// does not re-export the type, so like the rest of the Modern envelope this is a +// hand-rolled parallel serializer. +// +// These four are the ENTIRE subscribable universe under SEP-2575. Progress, +// logging, elicitation, and sampling are structurally absent -- they are not +// notification types a client can opt into here, which is why this channel +// cannot carry them however it is implemented. +type notificationSubscriptions struct { + ToolsListChanged bool `json:"toolsListChanged,omitempty"` + PromptsListChanged bool `json:"promptsListChanged,omitempty"` + ResourcesListChanged bool `json:"resourcesListChanged,omitempty"` + ResourceSubscriptions []string `json:"resourceSubscriptions,omitempty"` +} + +// isEmpty reports whether nothing at all is subscribed. +func (n notificationSubscriptions) isEmpty() bool { + return !n.ToolsListChanged && !n.PromptsListChanged && + !n.ResourcesListChanged && len(n.ResourceSubscriptions) == 0 +} + +// subscriptionsListenParams is the decoded subscriptions/listen request params. +// Notifications is a pointer so an absent field is distinguishable from an +// explicit empty object: the spec makes it REQUIRED, and go-sdk rejects a nil +// one with invalid-params (server.go:1193-1195), so vMCP must too. +type subscriptionsListenParams struct { + Notifications *notificationSubscriptions `json:"notifications"` +} + +// subscriptionsAcknowledgedParams is the acknowledgement notification's params: +// the honored subset, plus the subscription id this stream is keyed by. +type subscriptionsAcknowledgedParams struct { + Notifications notificationSubscriptions `json:"notifications"` + Meta map[string]any `json:"_meta"` +} + +// subscriptionsListenResult is the response to subscriptions/listen, signalling +// that the subscription ended gracefully. Mirrors go-sdk's +// SubscriptionsListenResult (protocol.go:2110-2118): resultType "complete" plus +// a subscriptionId-tagged _meta. +type subscriptionsListenResult struct { + ResultType string `json:"resultType"` + Meta map[string]any `json:"_meta"` +} + +// dispatchModernSubscriptionsListen serves subscriptions/listen. +// +// It is UNGATED, in the same bucket as the four list verbs and server/discover: +// there is no Check* for it because it performs no write, reaches no backend, +// and discloses nothing beyond what server/discover already does -- the honored +// set it returns is derived from core.Discover, which is itself +// admission-filtered per identity. Should this ever begin honoring a +// subscription (i.e. actually delivering resource updates), revisit that: a +// per-URI resource subscription that delivers content WOULD need +// CheckResourceRead-equivalent gating per URI. +// +// The response is an SSE stream, not a single JSON body, because the protocol +// requires two messages on it: the acknowledgement notification first, then the +// result. go-sdk forces SSE for this method for the same reason +// (streamable.go:1645-1650). The stream is closed as soon as both are written +// because the honored set is empty and there is consequently nothing to keep it +// open for -- matching go-sdk's reference server, which blocks on the request +// context only when it has agreed to honor at least one subscription +// (server.go:1246-1250). +func (s *Server) dispatchModernSubscriptionsListen( + ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, +) { + var params subscriptionsListenParams + if err := json.Unmarshal(parsed.Params, ¶ms); err != nil || params.Notifications == nil { + writeModernError(w, parsed.ID, jsonRPCCodeInvalidParams, + "invalid subscriptions/listen params: missing required 'notifications' field") + return + } + + // Resolve what this identity may reach, then shape it through the same + // capability builder server/discover publishes. Costs one backend fan-out, + // like dispatchModernDiscover; see its note on the absent cross-request cache. + caps, err := s.core.Discover(ctx, identity) + if err != nil { + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) + return + } + advertised := newModernCapabilities(caps.HasTools, caps.HasResources, caps.HasResourceTemplates, caps.HasPrompts) + honored := honoredSubscriptions(*params.Notifications, advertised) + + // The subscription id is the listen request's own JSON-RPC id. Modern has no + // sessions, so this -- not an Mcp-Session-Id -- is what a delivery + // implementation would key streams by. + subscriptionMeta := map[string]any{modernSubscriptionIDKey: parsed.ID} + + if err := writeModernListenStream(w, parsed.ID, honored, subscriptionMeta); err != nil { + // The client hung up or the stream could not be flushed. Nothing is + // recoverable at this point (headers are already committed), so log and + // return rather than attempting an error envelope on a dead stream. + slog.DebugContext(ctx, "vmcp modern dispatch: subscriptions/listen stream ended early", + "error", err) + return + } + + if !honored.isEmpty() { + // Unreachable while newModernCapabilities advertises no push capability, + // and deliberately not built out here: keeping a stream open is only half + // of honoring a subscription, and the delivery half does not exist yet. + // Whoever flips a capability flag owns adding both (#5743) -- this WARN + // exists so that, if a flag is flipped without it, the gap is loud in the + // logs instead of presenting as a silently idle client subscription. + slog.WarnContext(ctx, "vmcp modern dispatch: subscriptions/listen honored a subscription "+ + "but vMCP has no delivery mechanism; notifications will not arrive", + "subscription_id", parsed.ID, "honored", honored) + } +} + +// honoredSubscriptions intersects a client's requested notification set with +// what vMCP advertises, returning only the types it can actually honor. +// +// This is the whole of the per-type AND per-URI opt-in filtering SEP-2575 +// mandates ("the server MUST NOT send notification types the client has not +// explicitly requested"): a type survives only if the client asked for it AND +// the matching capability is advertised. Because it takes capabilities as a +// parameter rather than reading a package-level constant, it needs no edit when +// a capability flips -- and it is independently testable across combinations +// the live advertisement cannot currently produce. +// +// resourceSubscriptions is filtered as a unit rather than per-URI: the +// Subscribe capability is server-wide, so it either gates all requested URIs or +// none. Per-URI admission (dropping URIs this identity may not read) belongs +// with delivery, since it is only observable once updates actually flow. +func honoredSubscriptions( + want notificationSubscriptions, advertised mcp.ServerCapabilities, +) notificationSubscriptions { + var honored notificationSubscriptions + if want.ToolsListChanged && advertised.Tools != nil && advertised.Tools.ListChanged { + honored.ToolsListChanged = true + } + if want.PromptsListChanged && advertised.Prompts != nil && advertised.Prompts.ListChanged { + honored.PromptsListChanged = true + } + if want.ResourcesListChanged && advertised.Resources != nil && advertised.Resources.ListChanged { + honored.ResourcesListChanged = true + } + if len(want.ResourceSubscriptions) > 0 && advertised.Resources != nil && advertised.Resources.Subscribe { + honored.ResourceSubscriptions = want.ResourceSubscriptions + } + return honored +} + +// writeModernListenStream writes the two-frame SSE response: the mandatory +// initial acknowledgement notification, then the terminating result. +// +// Both frames are marshalled before any header is written, so a marshal failure +// cannot leave a half-written response -- the same build-then-write ordering +// writeModernEnvelope uses. +func writeModernListenStream( + w http.ResponseWriter, id any, honored notificationSubscriptions, subscriptionMeta map[string]any, +) error { + ack, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "method": notificationSubscriptionsAcked, + "params": subscriptionsAcknowledgedParams{ + Notifications: honored, + Meta: subscriptionMeta, + }, + }) + if err != nil { + return fmt.Errorf("marshalling subscriptions acknowledgement: %w", err) + } + result, err := json.Marshal(map[string]any{ + "jsonrpc": "2.0", + "id": id, + "result": subscriptionsListenResult{ + ResultType: modernResultTypeComplete, + Meta: subscriptionMeta, + }, + }) + if err != nil { + return fmt.Errorf("marshalling subscriptions listen result: %w", err) + } + + // A ResponseWriter that cannot flush would buffer both frames until the + // handler returned, which for a stream the client reads incrementally is + // indistinguishable from a hang. Fail before committing headers instead. + flusher, ok := w.(http.Flusher) + if !ok { + return fmt.Errorf("response writer does not support streaming") + } + + w.Header().Set("Content-Type", "text/event-stream") + w.Header().Set("Cache-Control", "no-cache, no-transform") + w.Header().Set("Connection", "keep-alive") + w.WriteHeader(http.StatusOK) + + for _, frame := range [][]byte{ack, result} { + if _, err := fmt.Fprintf(w, "event: message\ndata: %s\n\n", frame); err != nil { + return fmt.Errorf("writing subscriptions/listen frame: %w", err) + } + flusher.Flush() + } + return nil +} diff --git a/pkg/vmcp/server/modern_subscriptions_test.go b/pkg/vmcp/server/modern_subscriptions_test.go new file mode 100644 index 0000000000..3a463dabe7 --- /dev/null +++ b/pkg/vmcp/server/modern_subscriptions_test.go @@ -0,0 +1,281 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/mcpcompat/mcp" + mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp/core" +) + +// pushCaps builds an mcp.ServerCapabilities advertising the given push flags. +// The anonymous struct types must be spelled out to match mcp.ServerCapabilities +// field-for-field; this helper keeps that noise out of the table below and lets +// tests construct advertisements the live newModernCapabilities cannot currently +// produce (every push flag it emits is false). +func pushCaps(toolsLC, promptsLC, resourcesLC, subscribe bool) mcp.ServerCapabilities { + var caps mcp.ServerCapabilities + caps.Tools = &struct { + ListChanged bool `json:"listChanged,omitempty"` + }{ListChanged: toolsLC} + caps.Prompts = &struct { + ListChanged bool `json:"listChanged,omitempty"` + }{ListChanged: promptsLC} + caps.Resources = &struct { + Subscribe bool `json:"subscribe,omitempty"` + ListChanged bool `json:"listChanged,omitempty"` + }{Subscribe: subscribe, ListChanged: resourcesLC} + return caps +} + +// allWanted is a client asking for every subscribable type there is. +func allWanted() notificationSubscriptions { + return notificationSubscriptions{ + ToolsListChanged: true, + PromptsListChanged: true, + ResourcesListChanged: true, + ResourceSubscriptions: []string{"file:///a", "file:///b"}, + } +} + +// TestHonoredSubscriptions is the central assertion of this change: a +// notification type is honored if and only if the client asked for it AND the +// matching capability is advertised. That intersection is the whole of SEP-2575's +// per-type/per-URI opt-in filtering, and it is what guarantees the +// acknowledgement can never promise more than server/discover advertises. +// +// The advertised side is a parameter rather than read from a constant, so this +// covers the honoring combinations that the live advertisement cannot currently +// reach -- which is exactly what makes the handler correct-by-construction the +// day a capability flag flips. +func TestHonoredSubscriptions(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + want notificationSubscriptions + advertised mcp.ServerCapabilities + expect notificationSubscriptions + }{ + { + name: "live advertisement honors nothing even when everything is requested", + want: allWanted(), + advertised: newModernCapabilities(true, true, true, true), + expect: notificationSubscriptions{}, + }, + { + name: "all advertised and all wanted honors everything", + want: allWanted(), + advertised: pushCaps(true, true, true, true), + expect: allWanted(), + }, + { + name: "nothing wanted honors nothing even when all advertised", + want: notificationSubscriptions{}, + advertised: pushCaps(true, true, true, true), + expect: notificationSubscriptions{}, + }, + { + name: "advertised but not wanted is not honored", + want: notificationSubscriptions{ToolsListChanged: true}, + advertised: pushCaps(true, true, true, true), + expect: notificationSubscriptions{ToolsListChanged: true}, + }, + { + name: "wanted but not advertised is dropped per type", + want: allWanted(), + advertised: pushCaps(true, false, false, false), + expect: notificationSubscriptions{ToolsListChanged: true}, + }, + { + name: "resource subscriptions are gated by subscribe, not by resources listChanged", + want: notificationSubscriptions{ResourceSubscriptions: []string{"file:///a"}}, + advertised: pushCaps(false, false, true, false), + expect: notificationSubscriptions{}, + }, + { + name: "resource subscriptions survive when subscribe is advertised", + want: notificationSubscriptions{ResourceSubscriptions: []string{"file:///a"}}, + advertised: pushCaps(false, false, false, true), + expect: notificationSubscriptions{ResourceSubscriptions: []string{"file:///a"}}, + }, + { + name: "absent capability pointers honor nothing", + want: allWanted(), + advertised: mcp.ServerCapabilities{}, + expect: notificationSubscriptions{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.expect, honoredSubscriptions(tt.want, tt.advertised)) + }) + } +} + +// listenSSE drives dispatchModern for a subscriptions/listen request and splits +// the SSE body into its decoded JSON-RPC frames. It goes through dispatchModern +// rather than calling the handler directly so the method-switch wiring is +// covered too. +func listenSSE(t *testing.T, fakeCore *modernFakeCore, params any, id any) (*httptest.ResponseRecorder, []map[string]any) { + t.Helper() + + raw, err := json.Marshal(params) + require.NoError(t, err) + + s := &Server{ + config: &Config{Name: testServerName, Version: testServerVersion}, + core: fakeCore, + } + req := httptest.NewRequest(http.MethodPost, "/mcp", nil).WithContext(context.Background()) + rec := httptest.NewRecorder() + + s.dispatchModern(rec, req, &mcpparser.ParsedMCPRequest{ + ID: id, + Method: methodSubscriptionsListen, + Params: raw, + }) + + var frames []map[string]any + for _, block := range strings.Split(rec.Body.String(), "\n\n") { + for _, line := range strings.Split(block, "\n") { + data, ok := strings.CutPrefix(line, "data: ") + if !ok { + continue + } + var frame map[string]any + require.NoError(t, json.Unmarshal([]byte(data), &frame)) + frames = append(frames, frame) + } + } + return rec, frames +} + +// TestDispatchModernSubscriptionsListen_AcknowledgesEmptyHonoredSet covers the +// full wire contract for the only path the live advertisement can produce: an +// SSE response carrying the mandatory acknowledgement first and the terminating +// result second, both tagged with the subscription id, and an honored set that +// is explicitly empty rather than absent. +// +// discoverCaps is all-true so the test proves the empty honored set comes from +// the capability advertisement (which advertises no push flags) and not merely +// from the identity having nothing to reach. +func TestDispatchModernSubscriptionsListen_AcknowledgesEmptyHonoredSet(t *testing.T) { + t.Parallel() + + fakeCore := &modernFakeCore{discoverCaps: core.DiscoverCapabilities{ + HasTools: true, HasResources: true, HasResourceTemplates: true, HasPrompts: true, + }} + rec, frames := listenSSE(t, fakeCore, subscriptionsListenParams{ + Notifications: ¬ificationSubscriptions{ + ToolsListChanged: true, + PromptsListChanged: true, + ResourcesListChanged: true, + ResourceSubscriptions: []string{"file:///a"}, + }, + }, "sub-1") + + assert.Equal(t, http.StatusOK, rec.Code) + assert.Equal(t, "text/event-stream", rec.Header().Get("Content-Type")) + require.Len(t, frames, 2, "expected an acknowledgement frame followed by a result frame") + + // Frame 1: the acknowledgement MUST come first, and MUST report an empty + // honored set. An absent "notifications" object would be a different (and + // wrong) statement -- "I did not answer" rather than "I honor none". + ack := frames[0] + assert.Equal(t, notificationSubscriptionsAcked, ack["method"]) + ackParams, ok := ack["params"].(map[string]any) + require.True(t, ok, "acknowledgement must carry params") + honored, ok := ackParams["notifications"].(map[string]any) + require.True(t, ok, "acknowledgement must carry an explicit notifications object") + assert.Empty(t, honored, "no push capability is advertised, so nothing may be honored") + + ackMeta, ok := ackParams["_meta"].(map[string]any) + require.True(t, ok, "acknowledgement must be tagged with the subscription id") + assert.Equal(t, "sub-1", ackMeta[modernSubscriptionIDKey]) + + // Frame 2: the terminating result, same subscription id. Its presence is what + // makes the stream honestly finite: there is nothing to deliver, so the + // subscription ends immediately instead of idling open forever. + result := frames[1] + assert.Equal(t, "sub-1", result["id"]) + resultBody, ok := result["result"].(map[string]any) + require.True(t, ok, "result frame must carry a result") + assert.Equal(t, modernResultTypeComplete, resultBody["resultType"]) + resultMeta, ok := resultBody["_meta"].(map[string]any) + require.True(t, ok, "result must be tagged with the subscription id") + assert.Equal(t, "sub-1", resultMeta[modernSubscriptionIDKey]) +} + +// TestDispatchModernSubscriptionsListen_Errors covers the two rejections that +// must NOT open a stream: a missing required "notifications" field, and a failed +// capability resolution. +func TestDispatchModernSubscriptionsListen_Errors(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + params any + discoverErr error + wantCode float64 + wantStatus int + }{ + { + name: "missing notifications field", + params: map[string]any{}, + wantCode: jsonRPCCodeInvalidParams, + wantStatus: http.StatusBadRequest, + }, + { + name: "explicitly null notifications field", + params: map[string]any{"notifications": nil}, + wantCode: jsonRPCCodeInvalidParams, + wantStatus: http.StatusBadRequest, + }, + { + name: "capability resolution failure", + params: map[string]any{"notifications": map[string]any{"toolsListChanged": true}}, + discoverErr: errors.New("backend fan-out failed"), + wantCode: jsonRPCCodeInternalError, + wantStatus: http.StatusOK, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fakeCore := &modernFakeCore{discoverErr: tt.discoverErr} + rec, _ := listenSSE(t, fakeCore, tt.params, 7) + + assert.Equal(t, tt.wantStatus, rec.Code) + assert.NotEqual(t, "text/event-stream", rec.Header().Get("Content-Type"), + "a rejected listen must not open a stream") + + var body map[string]any + require.NoError(t, json.Unmarshal(rec.Body.Bytes(), &body)) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "expected a JSON-RPC error envelope") + assert.Equal(t, tt.wantCode, errObj["code"]) + + // The generic internal-error text must not leak aggregation detail. + if tt.discoverErr != nil { + assert.NotContains(t, errObj["message"], "backend fan-out failed") + } + }) + } +} From abfb38120b6fc82ad5e2c4c367e2ccda932b35b9 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 19:33:37 +0000 Subject: [PATCH 2/4] Paginate Modern list verbs with stateless cursors dispatchModern's four list verbs returned the full aggregated set and never emitted nextCursor, so a client had no way to page and vMCP silently ignored any cursor it was sent. On the Legacy path the SDK's session-scoped feature store does this split; the Modern dispatcher bypasses the SDK, so it has to do it itself. Modern has no sessions, so a cursor may not denote server-held iteration state. The spec makes cursors opaque to clients -- they MUST NOT parse, modify, or persist them -- which is exactly what lets the server encode position INTO the token instead of remembering it. So the cursor carries the unique key of the last item delivered and the next page is the items sorted strictly above it: keyset pagination, stateless, and the same scheme go-sdk's own server uses. Aggregation across backends needs no per-backend positions. core.List* already returns one flat, conflict-resolved set whose keys are unique across backends, so the cursor encodes a position in the aggregated ordering and adding or removing a backend cannot invalidate it. Page size matches the SDK default (1000) so pages line up across revisions, and an undecodable or wrong-verb cursor is rejected as -32602 rather than being reinterpreted into a plausible but wrong page. The upstream cursor-following in pkg/vmcp/client and the session backend (#5851) is the opposite direction -- vMCP as client -- and is untouched. Refs #5959, #6033 --- pkg/vmcp/server/modern_dispatch.go | 114 ++++++-- pkg/vmcp/server/modern_envelope.go | 34 ++- pkg/vmcp/server/modern_envelope_test.go | 16 +- pkg/vmcp/server/modern_pagination.go | 218 ++++++++++++++++ pkg/vmcp/server/modern_pagination_test.go | 300 ++++++++++++++++++++++ 5 files changed, 646 insertions(+), 36 deletions(-) create mode 100644 pkg/vmcp/server/modern_pagination.go create mode 100644 pkg/vmcp/server/modern_pagination_test.go diff --git a/pkg/vmcp/server/modern_dispatch.go b/pkg/vmcp/server/modern_dispatch.go index b253c008ae..72860edae4 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -103,13 +103,30 @@ func (s *Server) dispatchModern(w http.ResponseWriter, r *http.Request, parsed * // change that would require gating it. s.dispatchModernSubscriptionsListen(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. + // ping does NOT exist in 2026-07-28: `ping` appears nowhere in + // schema/draft/schema.ts, and go-sdk lists it among the methods removed + // for this revision (server.go:1880), answering -32601. So answering it + // at all is deliberate LENIENCY toward a client that pings anyway, not + // spec conformance -- an earlier version of this comment claimed the + // latter, which was wrong twice over: the method is gone, and the bare {} + // below omits `resultType`, which schema.ts's Result MUSTs on every + // result ("Servers implementing this protocol version MUST include this + // field"). + // + // Kept as-is rather than switched to -32601 because that is a behavior + // change to a pre-existing path, out of scope for the change that + // introduced this comment fix. It is inert in practice: a go-sdk Modern + // client never pings (startKeepalive sits past an early return), so + // nothing observable depends on either answer today. If you are here to + // tighten it, -32601 is the conformant answer. + // + // It is ungated (unauthenticated liveness, same bucket as initialize -- + // no Check*). Do not route it through the envelope builders above: they + // would stamp resultType and _meta.serverInfo, which the Legacy SDK path + // does not do for ping either (annotateServerInfo and + // setCompleteResultType both early-return for it, go-sdk + // server.go:1929-1945,1992), so the bare {} at least keeps the two paths + // answering alike. writeModernResult(w, parsed.ID, struct{}{}) default: writeModernError(w, parsed.ID, jsonRPCCodeMethodNotFound, "method not found") @@ -117,19 +134,21 @@ 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 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 +// resources/templates/list, prompts/list) each page the full +// admission-filtered set from the matching core.List* through paginateModern, +// emitting a nextCursor while items remain. The cursor is stateless keyset +// pagination over the aggregated ordering -- see modern_pagination.go for the +// wire contract and why a self-describing cursor is what a sessionless +// revision requires. 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. +// leaking that detail to callers. An invalid CURSOR is the exception: it is +// caller input rather than a server-side fault, so writeModernPageError maps +// it to -32602 instead. func (s *Server) dispatchModernToolsList( ctx context.Context, w http.ResponseWriter, parsed *mcpparser.ParsedMCPRequest, identity *auth.Identity, ) { @@ -138,7 +157,18 @@ func (s *Server) dispatchModernToolsList( writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } - result, err := newModernToolsList(tools, s.config.Name, s.config.Version) + cursor, err := modernRequestCursor(parsed.Params) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + page, next, err := paginateModern(tools, func(t vmcp.Tool) string { return t.Name }, + cursorKindTools, cursor) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + result, err := newModernToolsList(page, s.config.Name, s.config.Version, next) if err != nil { writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return @@ -154,7 +184,18 @@ func (s *Server) dispatchModernResourcesList( writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } - writeModernResult(w, parsed.ID, newModernResourcesList(resources, s.config.Name, s.config.Version)) + cursor, err := modernRequestCursor(parsed.Params) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + page, next, err := paginateModern(resources, func(r vmcp.Resource) string { return r.URI }, + cursorKindResources, cursor) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + writeModernResult(w, parsed.ID, newModernResourcesList(page, s.config.Name, s.config.Version, next)) } func (s *Server) dispatchModernResourceTemplatesList( @@ -165,7 +206,18 @@ func (s *Server) dispatchModernResourceTemplatesList( writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } - writeModernResult(w, parsed.ID, newModernResourceTemplatesList(templates, s.config.Name, s.config.Version)) + cursor, err := modernRequestCursor(parsed.Params) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + page, next, err := paginateModern(templates, func(t vmcp.ResourceTemplate) string { return t.URITemplate }, + cursorKindResourceTemplates, cursor) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + writeModernResult(w, parsed.ID, newModernResourceTemplatesList(page, s.config.Name, s.config.Version, next)) } func (s *Server) dispatchModernPromptsList( @@ -176,7 +228,18 @@ func (s *Server) dispatchModernPromptsList( writeModernListError(ctx, w, parsed.ID, parsed.Method, err) return } - writeModernResult(w, parsed.ID, newModernPromptsList(prompts, s.config.Name, s.config.Version)) + cursor, err := modernRequestCursor(parsed.Params) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + page, next, err := paginateModern(prompts, func(p vmcp.Prompt) string { return p.Name }, + cursorKindPrompts, cursor) + if err != nil { + writeModernPageError(ctx, w, parsed.ID, parsed.Method, err) + return + } + writeModernResult(w, parsed.ID, newModernPromptsList(page, s.config.Name, s.config.Version, next)) } // dispatchModernDiscover serves server/discover, Modern's replacement for @@ -438,6 +501,21 @@ func writeModernListError(ctx context.Context, w http.ResponseWriter, id any, me writeModernError(w, id, jsonRPCCodeInternalError, "internal error") } +// writeModernPageError classifies a paginateModern failure. An invalid cursor is +// bad caller input, so it gets -32602 -- matching the spec's "handle invalid +// cursors gracefully" and go-sdk, which returns ErrInvalidParams for a cursor it +// cannot decode. The message deliberately does not say WHY the cursor was +// rejected: clients must treat cursors as opaque, so describing the internal +// encoding would invite them to construct one. Anything else here is an encode +// failure, i.e. a server-side fault, and falls through to -32603. +func writeModernPageError(ctx context.Context, w http.ResponseWriter, id any, method string, err error) { + if errors.Is(err, errInvalidModernCursor) { + writeModernError(w, id, jsonRPCCodeInvalidParams, "invalid cursor") + return + } + writeModernListError(ctx, w, id, method, err) +} + // writeModernCallFailure classifies a POST-dispatch error from the three call // verbs (tools/call, resources/read, prompts/get), layering the mid-call // capability-refusal case on top of writeModernDispatchError's authz/generic diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index 4be2ccec5b..bd179f4a8d 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -107,16 +107,18 @@ func newModernCacheable() modernCacheable { type modernToolsListResult struct { ResultType string `json:"resultType"` modernCacheable - Tools []mcp.Tool `json:"tools"` - Meta modernMeta `json:"_meta"` + Tools []mcp.Tool `json:"tools"` + NextCursor string `json:"nextCursor,omitempty"` + 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"` + Resources []mcp.Resource `json:"resources"` + NextCursor string `json:"nextCursor,omitempty"` + Meta modernMeta `json:"_meta"` } // modernResourceTemplatesListResult is the resources/templates/list wire result. @@ -124,6 +126,7 @@ type modernResourceTemplatesListResult struct { ResultType string `json:"resultType"` modernCacheable ResourceTemplates []mcp.ResourceTemplate `json:"resourceTemplates"` + NextCursor string `json:"nextCursor,omitempty"` Meta modernMeta `json:"_meta"` } @@ -131,8 +134,9 @@ type modernResourceTemplatesListResult struct { type modernPromptsListResult struct { ResultType string `json:"resultType"` modernCacheable - Prompts []mcp.Prompt `json:"prompts"` - Meta modernMeta `json:"_meta"` + Prompts []mcp.Prompt `json:"prompts"` + NextCursor string `json:"nextCursor,omitempty"` + Meta modernMeta `json:"_meta"` } // modernCallToolResult is the tools/call wire result. Unlike the four list @@ -314,7 +318,9 @@ func newModernCapabilities(hasTools, hasResources, hasTemplates, hasPrompts bool // 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) { +func newModernToolsList( + tools []vmcp.Tool, nextCursor, serverName, serverVersion string, +) (modernToolsListResult, error) { wireTools := make([]mcp.Tool, 0, len(tools)) for _, t := range tools { wireTool, err := modernToolFromDomain(t) @@ -327,13 +333,16 @@ func newModernToolsList(tools []vmcp.Tool, serverName, serverVersion string) (mo ResultType: modernResultTypeComplete, modernCacheable: newModernCacheable(), Tools: wireTools, + NextCursor: nextCursor, 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 { +func newModernResourcesList( + resources []vmcp.Resource, nextCursor, serverName, serverVersion string, +) modernResourcesListResult { wireResources := make([]mcp.Resource, 0, len(resources)) for _, r := range resources { wireResources = append(wireResources, modernResourceFromDomain(r)) @@ -342,6 +351,7 @@ func newModernResourcesList(resources []vmcp.Resource, serverName, serverVersion ResultType: modernResultTypeComplete, modernCacheable: newModernCacheable(), Resources: wireResources, + NextCursor: nextCursor, Meta: newModernMeta(serverName, serverVersion), } } @@ -349,7 +359,7 @@ func newModernResourcesList(resources []vmcp.Resource, 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, + templates []vmcp.ResourceTemplate, nextCursor, serverName, serverVersion string, ) modernResourceTemplatesListResult { wireTemplates := make([]mcp.ResourceTemplate, 0, len(templates)) for _, t := range templates { @@ -359,13 +369,16 @@ func newModernResourceTemplatesList( ResultType: modernResultTypeComplete, modernCacheable: newModernCacheable(), ResourceTemplates: wireTemplates, + NextCursor: nextCursor, 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 { +func newModernPromptsList( + prompts []vmcp.Prompt, nextCursor, serverName, serverVersion string, +) modernPromptsListResult { wirePrompts := make([]mcp.Prompt, 0, len(prompts)) for _, p := range prompts { wirePrompts = append(wirePrompts, modernPromptFromDomain(p)) @@ -374,6 +387,7 @@ func newModernPromptsList(prompts []vmcp.Prompt, serverName, serverVersion strin ResultType: modernResultTypeComplete, modernCacheable: newModernCacheable(), Prompts: wirePrompts, + NextCursor: nextCursor, Meta: newModernMeta(serverName, serverVersion), } } diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index 2dee69a518..bacb98355d 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -43,7 +43,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { Name: "greet", Description: "say hi", InputSchema: map[string]any{"type": "object"}, - }}, testServerName, testServerVersion) + }}, "", testServerName, testServerVersion) require.NoError(t, err) return result }, @@ -54,7 +54,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernResourcesList([]vmcp.Resource{ {Name: "info", URI: "embedded:info", MimeType: "text/plain"}, - }, testServerName, testServerVersion) + }, "", testServerName, testServerVersion) }, wantCacheable: true, }, @@ -63,7 +63,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernResourceTemplatesList([]vmcp.ResourceTemplate{ {Name: "logs", URITemplate: "file:///logs/{date}.txt"}, - }, testServerName, testServerVersion) + }, "", testServerName, testServerVersion) }, wantCacheable: true, }, @@ -72,7 +72,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernPromptsList([]vmcp.Prompt{ {Name: "code_review", Arguments: []vmcp.PromptArgument{{Name: "Code", Required: true}}}, - }, testServerName, testServerVersion) + }, "", testServerName, testServerVersion) }, wantCacheable: true, }, @@ -328,7 +328,7 @@ func TestModernEnvelopeEmptyCollections(t *testing.T) { field: "tools", build: func(t *testing.T) any { t.Helper() - result, err := newModernToolsList(nil, testServerName, testServerVersion) + result, err := newModernToolsList(nil, "", testServerName, testServerVersion) require.NoError(t, err) return result }, @@ -336,19 +336,19 @@ func TestModernEnvelopeEmptyCollections(t *testing.T) { { name: "resources/list", field: "resources", - build: func(*testing.T) any { return newModernResourcesList(nil, testServerName, testServerVersion) }, + 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) + return newModernResourceTemplatesList(nil, "", testServerName, testServerVersion) }, }, { name: "prompts/list", field: "prompts", - build: func(*testing.T) any { return newModernPromptsList(nil, testServerName, testServerVersion) }, + build: func(*testing.T) any { return newModernPromptsList(nil, "", testServerName, testServerVersion) }, }, { name: "tools/call content", diff --git a/pkg/vmcp/server/modern_pagination.go b/pkg/vmcp/server/modern_pagination.go new file mode 100644 index 0000000000..66371d1872 --- /dev/null +++ b/pkg/vmcp/server/modern_pagination.go @@ -0,0 +1,218 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "slices" +) + +// Client-facing list pagination for the Modern (2026-07-28) dispatcher. +// +// WHY THIS IS NOT A PORT OF THE LEGACY BEHAVIOR. vMCP already follows +// pagination cursors in two places, but both are UPSTREAM -- vMCP acting as a +// client, walking a backend's pages to assemble the aggregated view +// (pkg/vmcp/client, pkg/vmcp/session/internal/backend, via +// pkg/vmcp/internal/pagination.ListAll; #5851). Nothing there is reusable here, +// which is a different direction entirely: this is vMCP acting as a SERVER, +// splitting its own already-aggregated list into pages for a downstream client. +// On the Legacy path that split is done for vMCP by the SDK's session-scoped +// feature store; dispatchModern bypasses the SDK, so it must do it itself. +// +// THE STATELESS CONSTRAINT, AND WHY THE SPEC PERMITS THIS. Modern has no +// sessions, so a cursor may not denote server-held iteration state -- there is +// nowhere to keep it, and any two requests may land on different replicas. The +// spec's pagination rules make a self-describing cursor the natural fit: +// nextCursor is "an opaque string token representing a position in the result +// set", clients "MUST treat cursors as opaque tokens" and must not parse, +// modify, or persist them across sessions, and servers "SHOULD provide stable +// cursors and handle invalid cursors gracefully". Opaque-to-the-client is +// precisely what lets the server encode position INTO the token instead of +// remembering it. +// +// So this is keyset pagination: the cursor carries the unique key of the last +// item already delivered, and the next page is the items sorted strictly above +// it. That satisfies "stable" in the sense that matters -- inserting or removing +// an item elsewhere in the set never causes a cursor to skip or repeat a +// different item, which an offset-based cursor would. It is also the scheme +// go-sdk's own server uses (paginateList/encodeCursor, server.go:2055-2120 in +// go-sdk@v1.7.0-pre.3), so Modern and Legacy pages behave alike. +// +// AGGREGATION ACROSS BACKENDS is handled by construction rather than by encoding +// per-backend positions. core.List* returns the complete, admission-filtered, +// conflict-resolved set in one call, so by the time pagination runs there is a +// single flat list whose keys the conflict resolver has already made unique +// across backends. The cursor therefore encodes a position in the AGGREGATED +// ordering; it never needs to name a backend, and adding or removing a backend +// cannot invalidate it. The cost is that each page re-runs the aggregation -- +// the same per-request fan-out dispatchModernDiscover already documents, not a +// new one. + +// modernPageSize is the maximum number of items in one Modern list page. +// +// It matches go-sdk's DefaultPageSize (server.go:36-37, value 1000), which is +// what the Legacy/SDK path uses for this same split: vMCP never calls +// mcpcompat's WithPageSize, so the SDK default is in force there. Keeping the +// two equal means a client sees the same page boundaries whichever revision it +// negotiates. mcpcompat does not re-export the constant, hence the local copy. +const modernPageSize = 1000 + +// Cursor kinds, one per paginated list verb. The kind is carried inside the +// cursor and checked on decode so a cursor minted for one list cannot be +// replayed against another: without it, a tools cursor sent to prompts/list +// would be silently reinterpreted as a prompt name and return a plausible but +// meaningless page. +const ( + cursorKindTools = "tools" + cursorKindResources = "resources" + cursorKindResourceTemplates = "resourceTemplates" + cursorKindPrompts = "prompts" +) + +// errInvalidModernCursor marks a cursor that is malformed, or valid but minted +// for a different list verb. Callers map it to -32602, per the spec's +// "handle invalid cursors gracefully" and matching go-sdk, which returns +// ErrInvalidParams for an undecodable cursor (server.go:2091-2094). +var errInvalidModernCursor = errors.New("invalid pagination cursor") + +// modernCursor is the decoded cursor payload. It is deliberately minimal: the +// position is fully described by the last key delivered, so no timestamp, +// offset, or result-set fingerprint is needed -- and none may be added that +// would require server-side state to interpret. +// +// The JSON field names are single letters only to keep the encoded token short; +// nothing outside this file may depend on them, since the token is opaque to +// clients by spec. +type modernCursor struct { + Kind string `json:"k"` + LastKey string `json:"l"` +} + +// encodeModernCursor builds the opaque token handed to the client as +// nextCursor. base64url keeps it safe in any JSON string and signals opacity; +// it is emphatically NOT encryption, and nothing secret may be put in it. +func encodeModernCursor(kind, lastKey string) (string, error) { + payload, err := json.Marshal(modernCursor{Kind: kind, LastKey: lastKey}) + if err != nil { + return "", fmt.Errorf("encoding pagination cursor: %w", err) + } + return base64.RawURLEncoding.EncodeToString(payload), nil +} + +// decodeModernCursor recovers the last delivered key from a client-supplied +// cursor, rejecting anything that is not a well-formed cursor for wantKind. +// +// Every failure collapses to errInvalidModernCursor rather than surfacing the +// decode detail: the cursor is client-controlled input, and echoing back why it +// failed to parse tells a caller about the internal encoding it is required to +// treat as opaque. +func decodeModernCursor(wantKind, cursor string) (string, error) { + raw, err := base64.RawURLEncoding.DecodeString(cursor) + if err != nil { + return "", errInvalidModernCursor + } + var decoded modernCursor + if err := json.Unmarshal(raw, &decoded); err != nil { + return "", errInvalidModernCursor + } + // A syntactically valid decode is not enough: the kind must match the verb + // being served, and an empty LastKey would make the "strictly above" scan + // below return the whole list, silently turning a page request into a + // full-list request. + if decoded.Kind != wantKind || decoded.LastKey == "" { + return "", errInvalidModernCursor + } + return decoded.LastKey, nil +} + +// paginateModern returns the page of items following cursor, plus the cursor for +// the page after it (empty when the returned page is the last one). +// +// keyOf must yield a key that is unique across the whole set -- Tool.Name, +// Resource.URI, ResourceTemplate.URITemplate, and Prompt.Name all are, since the +// aggregator's conflict resolver has already de-duplicated them across backends. +// Uniqueness is what makes "strictly above the last key" advance by exactly one +// page with no gap and no overlap; a duplicated key would silently drop every +// item sharing it. +// +// items is never mutated: it is cloned before sorting, because it comes straight +// from core.List* and the caller's slice must not be reordered underneath it. +func paginateModern[T any](items []T, keyOf func(T) string, kind, cursor string) ([]T, string, error) { + // Sorting is what makes the ordering deterministic across requests, which + // keyset pagination requires: the aggregator's own order is not guaranteed + // stable between calls (backend fan-out completes concurrently), and an + // unstable order would let a cursor skip or repeat items. + sorted := slices.Clone(items) + slices.SortFunc(sorted, func(a, b T) int { + return cmpString(keyOf(a), keyOf(b)) + }) + + start := 0 + if cursor != "" { + lastKey, err := decodeModernCursor(kind, cursor) + if err != nil { + return nil, "", err + } + // Strictly above: the item named by the cursor was already delivered. + // A cursor whose key is no longer present (its item was removed between + // pages) still lands correctly here -- the scan resumes at the next key + // above it rather than failing, which is the graceful degradation the + // spec's "stable cursors" guidance is about. + start = len(sorted) + for i, item := range sorted { + if keyOf(item) > lastKey { + start = i + break + } + } + } + + end := min(start+modernPageSize, len(sorted)) + page := sorted[start:end] + + // A next cursor is emitted only when items actually remain. Emitting one on + // the final page would make a conforming client issue a guaranteed-empty + // extra round trip. + if end == len(sorted) || len(page) == 0 { + return page, "", nil + } + next, err := encodeModernCursor(kind, keyOf(page[len(page)-1])) + if err != nil { + return nil, "", err + } + return page, next, nil +} + +// modernRequestCursor extracts the optional cursor from a list request's params. +// An absent, null, or non-string cursor reads as "first page": the field is +// optional, and a malformed params object has already been rejected upstream. +func modernRequestCursor(params json.RawMessage) string { + if len(params) == 0 { + return "" + } + var raw struct { + Cursor string `json:"cursor"` + } + if err := json.Unmarshal(params, &raw); err != nil { + return "" + } + return raw.Cursor +} + +// cmpString orders two keys. Spelled out rather than using strings.Compare so +// the comparison here provably matches the `>` used in paginateModern's scan -- +// the two must agree, or a page boundary could skip an item. +func cmpString(a, b string) int { + switch { + case a < b: + return -1 + case a > b: + return 1 + default: + return 0 + } +} diff --git a/pkg/vmcp/server/modern_pagination_test.go b/pkg/vmcp/server/modern_pagination_test.go new file mode 100644 index 0000000000..a211edccb3 --- /dev/null +++ b/pkg/vmcp/server/modern_pagination_test.go @@ -0,0 +1,300 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/base64" + "encoding/json" + "fmt" + "net/http" + "slices" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + mcpparser "github.com/stacklok/toolhive/pkg/mcp" + "github.com/stacklok/toolhive/pkg/vmcp" +) + +// identityKey keys a []string test corpus by its own element value. +func identityKey(s string) string { return s } + +// makeKeys builds n keys that sort lexicographically in generation order, so a +// test can assert exact page contents without depending on sort subtleties. +func makeKeys(n int) []string { + keys := make([]string, 0, n) + for i := range n { + keys = append(keys, fmt.Sprintf("k%05d", i)) + } + return keys +} + +// drainPages walks paginateModern to exhaustion, returning every key delivered +// and the number of pages it took. It fails if pagination does not terminate +// within a generous bound, so a cursor that fails to advance surfaces as a clear +// failure rather than an infinite loop. +func drainPages(t *testing.T, corpus []string) ([]string, int) { + t.Helper() + + // Non-nil so an empty corpus compares equal to makeKeys(0) rather than + // tripping on nil-vs-empty, which is not a distinction under test here. + seen := []string{} + cursor := "" + for pages := 1; pages <= 100; pages++ { + page, next, err := paginateModern(corpus, identityKey, cursorKindTools, cursor) + require.NoError(t, err) + seen = append(seen, page...) + if next == "" { + return seen, pages + } + cursor = next + } + t.Fatal("pagination did not terminate within 100 pages; cursor is not advancing") + return nil, 0 +} + +// TestPaginateModern_DeliversEveryItemExactlyOnce is the central assertion of the +// pagination half: walking the cursors must yield the complete set, in order, +// with no gap and no duplicate. That is the property the whole scheme exists to +// provide, and the one a broken page boundary silently violates. +func TestPaginateModern_DeliversEveryItemExactlyOnce(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + total int + wantPages int + }{ + {name: "empty set is a single empty page", total: 0, wantPages: 1}, + {name: "single item fits one page", total: 1, wantPages: 1}, + {name: "exactly one full page emits no cursor", total: modernPageSize, wantPages: 1}, + {name: "one item beyond a page spills to a second", total: modernPageSize + 1, wantPages: 2}, + {name: "the regression case: 1100 items", total: 1100, wantPages: 2}, + {name: "multiple full pages plus a tail", total: modernPageSize*2 + 7, wantPages: 3}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + corpus := makeKeys(tt.total) + seen, pages := drainPages(t, corpus) + + assert.Equal(t, tt.wantPages, pages, "unexpected page count") + assert.Equal(t, corpus, seen, "every item must be delivered exactly once, in sorted order") + + distinct := make(map[string]struct{}, len(seen)) + for _, k := range seen { + distinct[k] = struct{}{} + } + assert.Len(t, distinct, tt.total, "duplicates indicate overlapping pages") + }) + } +} + +// TestPaginateModern_PageBoundaries pins the boundary behaviour the walk above +// exercises only indirectly: page size is capped, and a nextCursor appears if and +// only if items actually remain. +func TestPaginateModern_PageBoundaries(t *testing.T) { + t.Parallel() + + t.Run("first page is capped at the page size and carries a cursor", func(t *testing.T) { + t.Parallel() + page, next, err := paginateModern(makeKeys(1100), identityKey, cursorKindTools, "") + require.NoError(t, err) + assert.Len(t, page, modernPageSize) + assert.NotEmpty(t, next, "items remain, so a cursor is required") + }) + + t.Run("final page carries no cursor", func(t *testing.T) { + t.Parallel() + _, next, err := paginateModern(makeKeys(1100), identityKey, cursorKindTools, "") + require.NoError(t, err) + page, next2, err := paginateModern(makeKeys(1100), identityKey, cursorKindTools, next) + require.NoError(t, err) + assert.Len(t, page, 100) + assert.Empty(t, next2, "nothing remains, so emitting a cursor would force a wasted round trip") + }) + + t.Run("caller slice is not reordered", func(t *testing.T) { + t.Parallel() + // Deliberately unsorted, mirroring the aggregator's unordered fan-out. + corpus := []string{"zeta", "alpha", "mu"} + original := slices.Clone(corpus) + + page, _, err := paginateModern(corpus, identityKey, cursorKindTools, "") + require.NoError(t, err) + + assert.Equal(t, original, corpus, "paginateModern must not sort the caller's slice in place") + assert.Equal(t, []string{"alpha", "mu", "zeta"}, page, "the page itself must be sorted") + }) +} + +// TestPaginateModern_CursorValidation covers the cursor as untrusted input. A +// cursor is client-controlled, so every malformed or mismatched shape must be +// rejected rather than reinterpreted into a plausible-looking page. +func TestPaginateModern_CursorValidation(t *testing.T) { + t.Parallel() + + validOtherKind, err := encodeModernCursor(cursorKindPrompts, "k00001") + require.NoError(t, err) + emptyKey, err := encodeModernCursor(cursorKindTools, "") + require.NoError(t, err) + + tests := []struct { + name string + cursor string + wantErr bool + }{ + {name: "not base64", cursor: "!!!not-base64!!!", wantErr: true}, + {name: "base64 of non-JSON", cursor: base64.RawURLEncoding.EncodeToString([]byte("plain")), wantErr: true}, + {name: "cursor minted for a different list verb", cursor: validOtherKind, wantErr: true}, + {name: "cursor with an empty key", cursor: emptyKey, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + _, _, err := paginateModern(makeKeys(10), identityKey, cursorKindTools, tt.cursor) + require.ErrorIs(t, err, errInvalidModernCursor) + }) + } + + t.Run("cursor naming a since-removed item resumes at the next key", func(t *testing.T) { + t.Parallel() + // "k00002" is gone from the corpus; the scan must continue from the next + // key above it rather than erroring or restarting. + cursor, err := encodeModernCursor(cursorKindTools, "k00002") + require.NoError(t, err) + + page, _, err := paginateModern([]string{"k00001", "k00003", "k00004"}, identityKey, cursorKindTools, cursor) + require.NoError(t, err) + assert.Equal(t, []string{"k00003", "k00004"}, page) + }) +} + +// TestModernCursor_RoundTripAndOpacity checks the codec, and that the token does +// not leak its payload in plaintext -- clients are required to treat it as +// opaque, so it must not invite parsing. +func TestModernCursor_RoundTripAndOpacity(t *testing.T) { + t.Parallel() + + encoded, err := encodeModernCursor(cursorKindResources, "file:///a/b.txt") + require.NoError(t, err) + + got, err := decodeModernCursor(cursorKindResources, encoded) + require.NoError(t, err) + assert.Equal(t, "file:///a/b.txt", got) + + assert.NotContains(t, encoded, "file:///", "the encoded cursor must not carry its key in plaintext") +} + +// TestModernRequestCursor covers cursor extraction from request params, including +// the shapes that must read as "first page" rather than erroring. +func TestModernRequestCursor(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + params string + want string + }{ + {name: "absent params", params: "", want: ""}, + {name: "empty object", params: `{}`, want: ""}, + {name: "null cursor", params: `{"cursor":null}`, want: ""}, + {name: "non-string cursor", params: `{"cursor":42}`, want: ""}, + {name: "malformed params", params: `{`, want: ""}, + {name: "present cursor", params: `{"cursor":"abc"}`, want: "abc"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + assert.Equal(t, tt.want, modernRequestCursor(json.RawMessage(tt.params))) + }) + } +} + +// TestDispatchModernList_Pagination drives the four list verbs through +// dispatchModern to prove the wire result actually carries nextCursor and that a +// bad cursor is rejected as -32602 rather than -32603. The per-verb table also +// pins that each verb pages on its own unique key, so a cursor from one cannot +// silently page another. +func TestDispatchModernList_Pagination(t *testing.T) { + t.Parallel() + + const total = modernPageSize + 5 + + tools := make([]vmcp.Tool, 0, total) + resources := make([]vmcp.Resource, 0, total) + templates := make([]vmcp.ResourceTemplate, 0, total) + prompts := make([]vmcp.Prompt, 0, total) + for i := range total { + id := fmt.Sprintf("k%05d", i) + tools = append(tools, vmcp.Tool{Name: id, InputSchema: map[string]any{"type": "object"}}) + resources = append(resources, vmcp.Resource{URI: "file:///" + id, Name: id}) + templates = append(templates, vmcp.ResourceTemplate{URITemplate: "file:///{x}/" + id, Name: id}) + prompts = append(prompts, vmcp.Prompt{Name: id}) + } + fakeCore := &modernFakeCore{tools: tools, resources: resources, templates: templates, prompts: prompts} + + tests := []struct { + method string + itemsField string + }{ + {method: "tools/list", itemsField: "tools"}, + {method: "resources/list", itemsField: "resources"}, + {method: "resources/templates/list", itemsField: "resourceTemplates"}, + {method: "prompts/list", itemsField: "prompts"}, + } + + for _, tt := range tests { + t.Run(tt.method, func(t *testing.T) { + t.Parallel() + + // Page 1: capped, and carries a cursor. + _, body := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ + ID: 1, Method: tt.method, Params: json.RawMessage(`{}`), + }) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "expected a result envelope, got %v", body) + items, ok := result[tt.itemsField].([]any) + require.True(t, ok, "expected %q in the result", tt.itemsField) + assert.Len(t, items, modernPageSize) + + cursor, ok := result["nextCursor"].(string) + require.True(t, ok, "a nextCursor must be present while items remain") + require.NotEmpty(t, cursor) + + // Page 2: the tail, and no further cursor. + params, err := json.Marshal(map[string]any{"cursor": cursor}) + require.NoError(t, err) + _, body2 := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ + ID: 2, Method: tt.method, Params: params, + }) + result2, ok := body2["result"].(map[string]any) + require.True(t, ok, "expected a result envelope, got %v", body2) + items2, ok := result2[tt.itemsField].([]any) + require.True(t, ok) + assert.Len(t, items2, 5, "the tail page must hold the remaining items") + assert.NotContains(t, result2, "nextCursor", "the final page must omit nextCursor entirely") + }) + } + + t.Run("invalid cursor is invalid params, not internal error", func(t *testing.T) { + t.Parallel() + + rec, body := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ + ID: 3, Method: "tools/list", Params: json.RawMessage(`{"cursor":"!!!bogus!!!"}`), + }) + assert.Equal(t, http.StatusBadRequest, 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(jsonRPCCodeInvalidParams), errObj["code"]) + assert.Equal(t, "invalid cursor", errObj["message"], + "the message must not describe the cursor encoding clients are told to treat as opaque") + }) +} From 4de308253c0058dccabc67a582d8e00a5edfc1dd Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 20:29:41 +0000 Subject: [PATCH 3/4] Make Modern pagination collision-safe and pin the wire Review found the paginator asserting a precondition that holds for one of its four callers. Only Tool.Name is unique -- ResolveToolConflicts keys a map -- while resources, resource templates and prompts are plain-appended across backends under "no conflict resolution for these yet". With a bare "resume strictly above lastKey" scan, a key shared at a page boundary made the scan skip every copy, so an item appeared on no page at all. Two honest backends both exposing file:///README.md was enough. A 1100-item corpus delivered 1097. The cursor now carries how many items sharing the last key were already sent, so a page can resume inside a run of equal keys. Items sharing a key are interchangeable for that purpose, which keeps the walk correct without a secondary sort key the generic signature cannot see. The comment claiming uniqueness is corrected rather than merged: it now states which callers can collide and that the paginator does not rely on uniqueness. The test corpus was the reason this shipped. Every case drew from a helper minting strictly unique keys, so the one precondition the code called load-bearing was the only thing untested. Collisions must also be placed by SORTED position -- a distinctively named duplicate key sorts away from the boundary and tests nothing, which a first attempt at the fixture did. Cap an inbound cursor before decoding it. A well-formed 6MB cursor decoded successfully at ~295ms of CPU on a verb that is not rate-limited, and a legitimate cursor is tens of bytes. Skip the capability fan-out in subscriptions/listen when nothing could be honored. core.Discover is un-rate-limited and, while no push capability is advertised, its result cannot change the answer -- so N no-op listens cost N fan-outs, now worse since list verbs page. Probing the ceiling first makes that O(1). This renders the fan-out error path unreachable, so its test is removed rather than left asserting a branch the code cannot enter. Assert the SSE framing. Replacing the blank-line event terminator with a single newline previously left the whole suite green -- including against a real client -- while a conformant parser would dispatch neither frame, so the ack-then-result contract was unverifiable. The helper now parses strictly, and the method name, acknowledgement name, subscriptionId key and jsonrpc field are asserted as literals; mutating any of them was silent. Add the negative capability test. The design rests on the advertisement honoring nothing, which was enforced by no test at all -- only by a runtime WARN that fires in production after the offending line ships. Now asserted across all sixteen presence combinations. Use http.ResponseController rather than a Flusher assertion: every ResponseWriter wrapper in the chain defines Flush unconditionally, so the assertion only established that a wrapper was present. Correct the comment that claimed otherwise, and the one claiming headers were committed on all four failure paths when three precede WriteHeader. Log the honored set by count, not by client-supplied URIs. Update docs/arch: subscriptions/listen is no longer unimplemented, serverStreamRegistry is the seam for delivery rather than for the handler, and both the pagination behaviour and the tools-only uniqueness property are now recorded. Refs #5959, #6033, #5743 --- docs/arch/03-transport-architecture.md | 39 +- docs/arch/10-virtual-mcp-architecture.md | 67 ++- pkg/vmcp/server/modern_dispatch_test.go | 11 +- pkg/vmcp/server/modern_envelope.go | 49 +- pkg/vmcp/server/modern_envelope_test.go | 16 +- pkg/vmcp/server/modern_pagination.go | 363 +++++++++---- pkg/vmcp/server/modern_pagination_test.go | 535 +++++++++++++++++-- pkg/vmcp/server/modern_subscriptions.go | 187 +++++-- pkg/vmcp/server/modern_subscriptions_test.go | 237 ++++++-- 9 files changed, 1221 insertions(+), 283 deletions(-) diff --git a/docs/arch/03-transport-architecture.md b/docs/arch/03-transport-architecture.md index 0c2f029f44..75db441c78 100644 --- a/docs/arch/03-transport-architecture.md +++ b/docs/arch/03-transport-architecture.md @@ -677,20 +677,31 @@ path for server->client messages. send-on-closed-channel panic if a concurrent broadcast/dispatch races eviction/shutdown. -**Forward-compatibility note**: the 2026-07-28 (Modern) MCP revision (see -`mcp.MCPVersionModern` and `pkg/mcp/revision.go`) introduces -`subscriptions/listen`, a POST method whose response stream stays open for -server-pushed subscription events. `serverStreamRegistry` is a reasonable -decoupled fan-out seam to build a future Modern handler for this method on top -of, but it is **not** a drop-in reuse. Per the 2026-07-28 spec, real adaptation is -required: `subscriptions/listen` is a long-lived POST, not a GET; Modern has no -sessions, so streams must be keyed per-subscription-id rather than per session -ID; delivery requires per-notification-type AND per-URI opt-in filtering, not -blanket fan-out; an initial `notifications/subscriptions/acknowledged` must be -sent; and deliveries must be tagged with `io.modelcontextprotocol/subscriptionId`. -`dispatcher_streams.go` is intentionally kept transport-agnostic (no HTTP -types) so that this adaptation, when it happens, does not also require -rewriting the underlying fan-out primitives. +**Relationship to Modern `subscriptions/listen`**: the 2026-07-28 (Modern) +revision (see `mcp.MCPVersionModern` and `pkg/mcp/revision.go`) replaces the +standalone GET with `subscriptions/listen`, a POST method whose response stream +stays open for server-pushed subscription events. + +vMCP now **serves** that method on its client edge +(`pkg/vmcp/server/modern_subscriptions.go`), and deliberately does **not** use +`serverStreamRegistry`. The reason is that the handler honors no subscriptions: +the honored set is intersected against the capabilities `server/discover` +advertises, every push flag there is false, so the acknowledged set is always +empty and the stream terminates immediately. With nothing to fan out, wiring in a +fan-out registry from another package would add an abstraction with zero +consumers. What the handler does implement is the wire contract — a long-lived +POST rather than a GET, keyed per-subscription-id (the listen request's own +JSON-RPC id) rather than per session ID since Modern has no sessions, an initial +`notifications/subscriptions/acknowledged`, and +`io.modelcontextprotocol/subscriptionId` tagging. + +`serverStreamRegistry` remains the sensible seam for the piece still missing: +actual **delivery** of notifications on a Modern listen stream (#5743). That work +needs per-notification-type AND per-URI opt-in filtering rather than blanket +fan-out, and per `schema/draft/schema.ts` must tag *every* notification with the +subscription id, not just the acknowledgement. `dispatcher_streams.go` is +therefore still intentionally kept transport-agnostic (no HTTP types), so that +when delivery lands it does not also require rewriting the fan-out primitives. ## Error Handling diff --git a/docs/arch/10-virtual-mcp-architecture.md b/docs/arch/10-virtual-mcp-architecture.md index 0314859708..e7e5a676e1 100644 --- a/docs/arch/10-virtual-mcp-architecture.md +++ b/docs/arch/10-virtual-mcp-architecture.md @@ -136,6 +136,19 @@ When backends expose tools with the same name, vMCP resolves the conflict using | **priority** | First backend in priority order wins, others hidden | | **manual** | Explicit mapping for each conflict | +**Conflict resolution covers tools only.** Resources, resource templates, and +prompts are concatenated across backends with no de-duplication +(`default_aggregator.go`, "no conflict resolution for these yet"), so +`Resource.URI`, `ResourceTemplate.URITemplate` and `Prompt.Name` can each repeat +in the aggregated view — two backends both exposing `file:///README.md` is +enough. Only `Tool.Name` is unique, because tool resolution keys a map. + +Anything that treats one of those fields as an identity must account for that. +The Modern list paginator does: its cursor names a position within a run of equal +keys rather than assuming keys are distinct, because a plain "resume after this +key" scan silently dropped an item whenever a duplicate landed on a page boundary +(see [Client-facing list pagination](#client-facing-list-pagination)). + ### Tool Filtering Beyond conflict resolution, vMCP can filter which tools are exposed through allow/deny lists, renaming, and description overrides. @@ -300,7 +313,10 @@ Beyond tools, vMCP aggregates and serves the full complement of MCP capabilities | Resource templates (`resources/templates/list`) | Yes | Templated reads route through the same `ReadResource` path; the router matches an expanded URI against the aggregated templates, and an exact template-string key routes to its backend; covered by the same `notifications/resources/list_changed` propagation as resources (MCP 2025-11-25 has no separate wire method for template changes) | | Prompts (`prompts/list`, `prompts/get`) | Yes | Served per-session; a backend's asynchronous `notifications/prompts/list_changed` propagates ADDITIONS (not removals) to already-registered sessions — see below | | Completions (`completion/complete`) | Yes | A `ref/prompt` routes via the prompts table; a `ref/resource` carries the URI-template string per the spec and routes via the resource-templates table (exact template-string key, with exact-resource and template-expansion fallbacks). Unroutable refs return an empty result (lenient completion); admission denial returns an error | -| Resource subscriptions (`resources/subscribe`, `resources/unsubscribe`) | Ack-level only | vMCP accepts and records the subscription (after session-binding and resource-admission checks) but does **not** yet forward backend `notifications/resources/updated` — see the limitation below | +| Resource subscriptions (`resources/subscribe`, `resources/unsubscribe`) | Ack-level only | Legacy edge. vMCP accepts and records the subscription (after session-binding and resource-admission checks) but does **not** yet forward backend `notifications/resources/updated` — see the limitation below | +| Subscriptions (`subscriptions/listen`) | Ack-level only | Modern (2026-07-28) edge, the revision's only server→client push channel. Answered with an SSE stream carrying the mandatory `notifications/subscriptions/acknowledged` and then a terminating result, both tagged `io.modelcontextprotocol/subscriptionId` and keyed by the listen request's JSON-RPC id (Modern has no sessions). The honored set is intersected against the advertised capabilities, all of whose push flags are false, so it is always **empty** and the stream closes immediately rather than idling. Serving it is what lets a go-sdk v1.7 client complete `Connect` at all — see below | + +The four Modern list verbs (`tools/list`, `resources/list`, `resources/templates/list`, `prompts/list`) **paginate**, emitting `nextCursor` while items remain and capping a page at 1000 items to match the page size the SDK applies on the Legacy path. See [Client-facing list pagination](#client-facing-list-pagination). The completion handler is a single global handler installed via `WithCompletionHandler`, so it recovers the session from the SDK request context rather than a per-session closure. Setting it makes the shim auto-advertise the `completions` capability at initialize. @@ -308,6 +324,42 @@ The completion handler is a single global handler installed via `WithCompletionH vMCP advertises `resources.subscribe: true` and answers `resources/subscribe` / `resources/unsubscribe` at **ack level**: the request is accepted (enforcing session binding and validating the URI is an advertised, admitted resource), and go-sdk records the subscription. vMCP does **not** currently propagate backend `notifications/resources/updated` to the subscribed client — doing so requires persistent per-session backend connections, which is out of scope. Clients that subscribe will receive a success ack but no update stream yet. +### Client-facing list pagination + +On the Legacy edge the SDK's session-scoped feature store splits list results into +pages. `dispatchModern` bypasses the SDK, so the Modern edge paginates itself +(`pkg/vmcp/server/modern_pagination.go`). + +Modern has no sessions, so a cursor may not denote server-held iteration state. +The draft pagination page makes cursors **opaque to clients** ("MUST treat cursors +as opaque tokens"), which is precisely what allows the server to encode position +*into* the token instead of remembering it. So the cursor is self-describing: +base64url over a small JSON payload naming the list kind, the last key delivered, +and how many items sharing that key have already been sent. + +Three properties worth knowing: + +- **Ordering.** Keyset paging needs a deterministic total order, and the + aggregator's fan-out order is not stable between calls, so Modern list results + are sorted by the item's key (`Tool.Name`, `Resource.URI`, + `ResourceTemplate.URITemplate`, `Prompt.Name`). Legacy ordering is unchanged. +- **Duplicate keys are tolerated, not assumed away.** Only tool names are unique + (see [Conflict Resolution](#conflict-resolution)); resource URIs, template + strings and prompt names can repeat. The cursor therefore resumes *within* a run + of equal keys, because a plain "resume after this key" scan skipped every copy + and permanently dropped items whose key collided at a page boundary. +- **End of results omits `nextCursor` entirely.** The draft states that "an empty + string is a valid cursor and thus MUST NOT be treated as the end of results", so + emitting `""` would make a conformant client re-request and loop on page one. + +The cursor encodes a position in the **aggregated** ordering and never names a +backend, so adding or removing a backend cannot invalidate one. This is unrelated +to the aggregator's *upstream* cursor-following (#5851), which is vMCP acting as a +client walking a backend's pages. + +An invalid cursor — malformed, over-length, or minted for a different list verb — +is rejected with `-32602`, per the draft's error-handling rule. + ### Tools/resources/prompts list_changed propagation (#5748, #5969) Unlike the per-call backend client (`pkg/vmcp/client`), the **persistent** @@ -338,8 +390,17 @@ above for how that revision is resolved and cached. This is correct, not a gap i `initialize` and `Mcp-Session-Id`, so there is no Legacy-shaped persistent connection to hold and no standalone GET stream a Modern backend could push on. Modern's own server-push mechanism is `subscriptions/listen` (see -[Transport Architecture](03-transport-architecture.md)), which vMCP does not -implement yet — so `list_changed` propagation is currently Legacy-only. +[Transport Architecture](03-transport-architecture.md)). vMCP **serves** that +method on its client edge (`pkg/vmcp/server/modern_subscriptions.go`), but only +at acknowledgement level: it computes the honored subscription set by +intersecting the client's request against the capabilities `server/discover` +advertises, and since every push-related flag there is deliberately false +(`newModernCapabilities`), the honored set is always empty and no notification is +ever pushed. So `list_changed` **delivery** remains Legacy-only, on both edges — +what the Modern client edge gained is a conformant, explicitly-empty answer +instead of a `-32601` that tore the client's connection down. Real Modern +delivery is tracked in #5743 and requires vMCP to start advertising a push +capability first. The sink is built once per session, at registration (`pkg/vmcp/server`'s `buildListChangedSink`), closing over the SDK `ClientSession`, the session ID, diff --git a/pkg/vmcp/server/modern_dispatch_test.go b/pkg/vmcp/server/modern_dispatch_test.go index 03ed9469da..d4c4503ba0 100644 --- a/pkg/vmcp/server/modern_dispatch_test.go +++ b/pkg/vmcp/server/modern_dispatch_test.go @@ -38,6 +38,13 @@ type modernFakeCore struct { discoverCaps core.DiscoverCapabilities discoverErr error + // discoverCalls counts Discover invocations, so a test can assert the + // subscriptions/listen pre-check skipped the backend fan-out entirely (the + // response is identical either way, so only the count is observable). + discoverCalls int + // discoverIdentity records the identity Discover was called with, so + // per-identity filtering of a computed set can be pinned rather than assumed. + discoverIdentity *auth.Identity checkToolErr, checkResourceErr, checkPromptErr error callToolErr, readResourceErr, getPromptErr error @@ -73,7 +80,9 @@ func (f *modernFakeCore) ListPrompts(context.Context, *auth.Identity) ([]vmcp.Pr return f.prompts, nil } -func (f *modernFakeCore) Discover(context.Context, *auth.Identity) (core.DiscoverCapabilities, error) { +func (f *modernFakeCore) Discover(_ context.Context, identity *auth.Identity) (core.DiscoverCapabilities, error) { + f.discoverCalls++ + f.discoverIdentity = identity return f.discoverCaps, f.discoverErr } diff --git a/pkg/vmcp/server/modern_envelope.go b/pkg/vmcp/server/modern_envelope.go index bd179f4a8d..7fae168b96 100644 --- a/pkg/vmcp/server/modern_envelope.go +++ b/pkg/vmcp/server/modern_envelope.go @@ -233,21 +233,10 @@ type modernDiscoverResult struct { 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). +// newModernDiscover builds the server/discover wire result. The capability +// flags themselves are shaped by newModernCapabilities below -- see its doc +// comment for what each flag means and why the push-related ones are false. +// This function only assembles the envelope around them. // // SupportedVersions enumerates every protocol version this vMCP endpoint // actually serves, not just Modern: mcpparser.MCPVersionModern (this @@ -269,8 +258,22 @@ func newModernDiscover( } // newModernCapabilities shapes the four admission-filtered presence checks into -// the wire capability flags. It is the SINGLE source of truth for what this -// dispatcher advertises: server/discover publishes it verbatim (above), and +// the wire capability flags. +// +// hasTools/hasResources/hasTemplates/hasPrompts are the caller's +// len(core.List*(ctx, identity))>0 checks, so an advertisement 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 flag. +// +// Completions is set unconditionally, unlike the identity-filtered flags: +// 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 -- matching how +// the Legacy/SDK path always wires server.WithCompletionHandler regardless of +// identity (serve.go). +// +// It is the SINGLE source of truth for what this dispatcher advertises: server/discover publishes it verbatim (above), and // subscriptions/listen intersects a client's requested notification set against // it (honoredSubscriptions, modern_subscriptions.go). Keeping one builder is // what makes those two answers impossible to drift apart -- a client can never @@ -318,8 +321,12 @@ func newModernCapabilities(hasTools, hasResources, hasTemplates, hasPrompts bool // newModernToolsList builds the tools/list wire result from the core's // admission-filtered domain tools. +// nextCursor is LAST on purpose: it sits among same-typed string parameters, and +// when it was in the middle a transposition with serverName compiled silently and +// put a server name in the cursor field. Keep new string params appended, not +// inserted. func newModernToolsList( - tools []vmcp.Tool, nextCursor, serverName, serverVersion string, + tools []vmcp.Tool, serverName, serverVersion, nextCursor string, ) (modernToolsListResult, error) { wireTools := make([]mcp.Tool, 0, len(tools)) for _, t := range tools { @@ -341,7 +348,7 @@ func newModernToolsList( // newModernResourcesList builds the resources/list wire result from the // core's admission-filtered domain resources. func newModernResourcesList( - resources []vmcp.Resource, nextCursor, serverName, serverVersion string, + resources []vmcp.Resource, serverName, serverVersion, nextCursor string, ) modernResourcesListResult { wireResources := make([]mcp.Resource, 0, len(resources)) for _, r := range resources { @@ -359,7 +366,7 @@ func newModernResourcesList( // newModernResourceTemplatesList builds the resources/templates/list wire // result from the core's admission-filtered domain resource templates. func newModernResourceTemplatesList( - templates []vmcp.ResourceTemplate, nextCursor, serverName, serverVersion string, + templates []vmcp.ResourceTemplate, serverName, serverVersion, nextCursor string, ) modernResourceTemplatesListResult { wireTemplates := make([]mcp.ResourceTemplate, 0, len(templates)) for _, t := range templates { @@ -377,7 +384,7 @@ func newModernResourceTemplatesList( // newModernPromptsList builds the prompts/list wire result from the core's // admission-filtered domain prompts. func newModernPromptsList( - prompts []vmcp.Prompt, nextCursor, serverName, serverVersion string, + prompts []vmcp.Prompt, serverName, serverVersion, nextCursor string, ) modernPromptsListResult { wirePrompts := make([]mcp.Prompt, 0, len(prompts)) for _, p := range prompts { diff --git a/pkg/vmcp/server/modern_envelope_test.go b/pkg/vmcp/server/modern_envelope_test.go index bacb98355d..c63c3abc6f 100644 --- a/pkg/vmcp/server/modern_envelope_test.go +++ b/pkg/vmcp/server/modern_envelope_test.go @@ -43,7 +43,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { Name: "greet", Description: "say hi", InputSchema: map[string]any{"type": "object"}, - }}, "", testServerName, testServerVersion) + }}, testServerName, testServerVersion, "") require.NoError(t, err) return result }, @@ -54,7 +54,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernResourcesList([]vmcp.Resource{ {Name: "info", URI: "embedded:info", MimeType: "text/plain"}, - }, "", testServerName, testServerVersion) + }, testServerName, testServerVersion, "") }, wantCacheable: true, }, @@ -63,7 +63,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernResourceTemplatesList([]vmcp.ResourceTemplate{ {Name: "logs", URITemplate: "file:///logs/{date}.txt"}, - }, "", testServerName, testServerVersion) + }, testServerName, testServerVersion, "") }, wantCacheable: true, }, @@ -72,7 +72,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) { build: func(*testing.T) any { return newModernPromptsList([]vmcp.Prompt{ {Name: "code_review", Arguments: []vmcp.PromptArgument{{Name: "Code", Required: true}}}, - }, "", testServerName, testServerVersion) + }, testServerName, testServerVersion, "") }, wantCacheable: true, }, @@ -328,7 +328,7 @@ func TestModernEnvelopeEmptyCollections(t *testing.T) { field: "tools", build: func(t *testing.T) any { t.Helper() - result, err := newModernToolsList(nil, "", testServerName, testServerVersion) + result, err := newModernToolsList(nil, testServerName, testServerVersion, "") require.NoError(t, err) return result }, @@ -336,19 +336,19 @@ func TestModernEnvelopeEmptyCollections(t *testing.T) { { name: "resources/list", field: "resources", - build: func(*testing.T) any { return newModernResourcesList(nil, "", testServerName, testServerVersion) }, + 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) + return newModernResourceTemplatesList(nil, testServerName, testServerVersion, "") }, }, { name: "prompts/list", field: "prompts", - build: func(*testing.T) any { return newModernPromptsList(nil, "", testServerName, testServerVersion) }, + build: func(*testing.T) any { return newModernPromptsList(nil, testServerName, testServerVersion, "") }, }, { name: "tools/call content", diff --git a/pkg/vmcp/server/modern_pagination.go b/pkg/vmcp/server/modern_pagination.go index 66371d1872..1259360ed5 100644 --- a/pkg/vmcp/server/modern_pagination.go +++ b/pkg/vmcp/server/modern_pagination.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "slices" + "strings" ) // Client-facing list pagination for the Modern (2026-07-28) dispatcher. @@ -17,50 +18,116 @@ import ( // pagination cursors in two places, but both are UPSTREAM -- vMCP acting as a // client, walking a backend's pages to assemble the aggregated view // (pkg/vmcp/client, pkg/vmcp/session/internal/backend, via -// pkg/vmcp/internal/pagination.ListAll; #5851). Nothing there is reusable here, -// which is a different direction entirely: this is vMCP acting as a SERVER, -// splitting its own already-aggregated list into pages for a downstream client. -// On the Legacy path that split is done for vMCP by the SDK's session-scoped -// feature store; dispatchModern bypasses the SDK, so it must do it itself. +// pkg/vmcp/internal/pagination.ListAll; #5851). The giveaway that none of it is +// reusable here is the shape of the entry point: ListAll takes a `fetch` +// CALLBACK and drives it until the upstream stops yielding cursors, whereas this +// file takes a SLICE already fully in hand. ListAll holds no encode/decode logic +// and never mints a cursor -- it only consumes them. This is the opposite +// direction: vMCP as a SERVER, splitting its own already-aggregated list into +// pages for a downstream client. On the Legacy path the SDK's session-scoped +// feature store does that split; dispatchModern bypasses the SDK, so it must do +// it itself. +// +// Cursor TOKENS are consequently not interchangeable between the two revisions +// even though page BOUNDARIES are (see modernPageSize): go-sdk's Legacy server +// mints padded base64.URLEncoding over gob, this mints RawURLEncoding over JSON. +// Neither side ever needs to read the other's -- a client re-lists after +// negotiating, and the opacity rule forbids it from trying. // // THE STATELESS CONSTRAINT, AND WHY THE SPEC PERMITS THIS. Modern has no // sessions, so a cursor may not denote server-held iteration state -- there is -// nowhere to keep it, and any two requests may land on different replicas. The -// spec's pagination rules make a self-describing cursor the natural fit: -// nextCursor is "an opaque string token representing a position in the result -// set", clients "MUST treat cursors as opaque tokens" and must not parse, -// modify, or persist them across sessions, and servers "SHOULD provide stable -// cursors and handle invalid cursors gracefully". Opaque-to-the-client is -// precisely what lets the server encode position INTO the token instead of -// remembering it. -// -// So this is keyset pagination: the cursor carries the unique key of the last -// item already delivered, and the next page is the items sorted strictly above -// it. That satisfies "stable" in the sense that matters -- inserting or removing -// an item elsewhere in the set never causes a cursor to skip or repeat a -// different item, which an offset-based cursor would. It is also the scheme -// go-sdk's own server uses (paginateList/encodeCursor, server.go:2055-2120 in -// go-sdk@v1.7.0-pre.3), so Modern and Legacy pages behave alike. -// -// AGGREGATION ACROSS BACKENDS is handled by construction rather than by encoding -// per-backend positions. core.List* returns the complete, admission-filtered, -// conflict-resolved set in one call, so by the time pagination runs there is a -// single flat list whose keys the conflict resolver has already made unique -// across backends. The cursor therefore encodes a position in the AGGREGATED -// ordering; it never needs to name a backend, and adding or removing a backend -// cannot invalidate it. The cost is that each page re-runs the aggregation -- -// the same per-request fan-out dispatchModernDiscover already documents, not a -// new one. +// nowhere to keep it, and any two requests may land on different replicas. +// +// The governing text is the DRAFT pagination page +// (docs/specification/draft/server/utilities/pagination.mdx), not the 2025-11-25 +// one: they differ, and the draft is what 2026-07-28 ships. Its normative rules: +// +// - the cursor is "an opaque string token, representing a position in the +// result set" (Pagination Model); +// - clients "MUST treat cursors as opaque tokens" -- no assumptions about +// format, no parsing, no modifying (Implementation Guidelines 3); +// - servers "SHOULD provide stable cursors and handle invalid cursors +// gracefully" (Implementation Guidelines 1); +// - "Invalid cursors SHOULD result in an error with code -32602" (Error +// Handling). +// +// That opacity MUST, on its own, licenses this design: if a client may not +// interpret the token, the server is free to encode position INTO it rather than +// remembering it. No further justification is needed. +// +// ONE DRAFT-ONLY AMENDMENT, and it is load-bearing here. The 2025-11-25 bullet +// "Don't persist cursors across sessions" is GONE, replaced by: "Don't make any +// determination based on cursor value other than whether a non-null value was +// provided (e.g. an empty string is a valid cursor and thus MUST NOT be treated +// as the end of results)." The reason for the deletion is recorded in +// docs/seps/2567-sessionless-mcp.md under "Consequential spec edits" -- note that +// SEP is a list of directed edits, i.e. rationale, NOT the normative surface; +// the draft page above is. +// +// The consequence here: end-of-results MUST be signalled by OMITTING nextCursor, +// never by emitting an empty string, because a conformant client treats "" as a +// valid cursor and re-requests with it. Hence `omitempty` on every list result's +// NextCursor field (modern_envelope.go) -- without it a client loops forever on +// page one. +// +// A NOTE ON A TEMPTING ARGUMENT, flagged so nobody re-derives it as doctrine: +// one could argue a self-describing cursor is *required* now, since with sessions +// gone a server-held cursor has no protocol lifetime bounding it. The engineering +// half is true and worth knowing -- a stateful scheme would have to invent its own +// TTL and eviction. But it is NOT a spec consequence: the deleted bullet was +// advice to CLIENTS about persistence, and removing it withdraws a client-side +// caveat rather than creating a server-side obligation. SEP-2567 explicitly +// declines to speak here, putting "cursor stability and snapshot consistency" +// out of scope. Treat the TTL point as our reasoning; treat opacity as the +// authority. +// +// AGGREGATION ACROSS BACKENDS needs no per-backend positions. core.List* returns +// the complete, admission-filtered set in one call, so the cursor encodes a +// position in the AGGREGATED ordering; it never names a backend, and adding or +// removing one cannot invalidate it. Each page re-runs the aggregation -- the same +// per-request fan-out dispatchModernDiscover already documents, not a new one. +// Re-sorting per page is O(n log n) per request, measured at 0.76ms for n=1100, +// i.e. dwarfed by that fan-out. Do not "optimise" it with a cache. // modernPageSize is the maximum number of items in one Modern list page. // // It matches go-sdk's DefaultPageSize (server.go:36-37, value 1000), which is -// what the Legacy/SDK path uses for this same split: vMCP never calls -// mcpcompat's WithPageSize, so the SDK default is in force there. Keeping the -// two equal means a client sees the same page boundaries whichever revision it -// negotiates. mcpcompat does not re-export the constant, hence the local copy. +// what the Legacy/SDK path uses for this same split: vMCP never calls mcpcompat's +// WithPageSize, so the SDK default is in force there. Keeping the two equal means +// a client sees the same page boundaries whichever revision it negotiates. +// +// This is a COPY, not a reference -- mcpcompat does not re-export the constant +// (raising that upstream is tracked separately). Provenance is not detection: if +// go-sdk changes its default, the Legacy path follows automatically and this one +// does not, silently falsifying the equality claimed above. +// +// KNOWN GAP, stated rather than papered over: nothing currently detects that +// drift. The Modern side IS pinned behaviourally (a 1001-item corpus must yield a +// 1000-item first page). The Legacy side is not, because holding a client on +// Legacy would need a negotiation-pinning mechanism that does not exist here -- +// mcpcompat cannot request a protocol version (#5911) -- and +// test/integration/vmcp's Over1000Tools regression exercises the Legacy split +// only incidentally today, becoming a Modern test once the kill-switch is +// removed. +// +// So: treat the equality claimed above as an invariant maintained by REVIEW, not +// by CI, and re-check it on any go-sdk bump. This deliberately does not name a +// mechanism the Legacy twin should use; whether that is a pinning harness, a +// loud failure at negotiation, or something else is undecided, and this comment +// must not go stale by presuming one. const modernPageSize = 1000 +// modernCursorMaxLen caps an inbound cursor before it is decoded. +// +// A legitimate cursor is tens of bytes (kind, one key, a small count). The +// request body cap is 8MB and list verbs are not rate-limited +// (ratelimit/decorator.go covers CallTool only), so without this a client could +// hand over a well-formed multi-megabyte cursor that decodes successfully -- +// measured at ~295ms of CPU for a 6MB key -- and repeat it unmetered. Rejecting +// on LENGTH before decoding keeps that O(1). 512 is generous: it accommodates a +// ~370-byte key, far beyond any real URI. +const modernCursorMaxLen = 512 + // Cursor kinds, one per paginated list verb. The kind is carried inside the // cursor and checked on decode so a cursor minted for one list cannot be // replayed against another: without it, a tools cursor sent to prompts/list @@ -73,101 +140,145 @@ const ( cursorKindPrompts = "prompts" ) -// errInvalidModernCursor marks a cursor that is malformed, or valid but minted -// for a different list verb. Callers map it to -32602, per the spec's -// "handle invalid cursors gracefully" and matching go-sdk, which returns -// ErrInvalidParams for an undecodable cursor (server.go:2091-2094). +// errInvalidModernCursor marks a cursor that is over-long, malformed, not a JSON +// string, or well-formed but minted for a different list verb. Callers map it to +// -32602, which the draft pagination page's Error Handling section requires +// ("Invalid cursors SHOULD result in an error with code -32602") and which +// schema.ts classifies under invalid-params ("Pagination: Invalid or expired +// cursor values"). go-sdk does the same, returning ErrInvalidParams for a cursor +// it cannot decode (server.go:2091-2094). var errInvalidModernCursor = errors.New("invalid pagination cursor") -// modernCursor is the decoded cursor payload. It is deliberately minimal: the -// position is fully described by the last key delivered, so no timestamp, -// offset, or result-set fingerprint is needed -- and none may be added that -// would require server-side state to interpret. +// modernCursor is the decoded cursor payload: a position in the sorted aggregated +// list, fully self-described so no server state is needed to interpret it. // -// The JSON field names are single letters only to keep the encoded token short; -// nothing outside this file may depend on them, since the token is opaque to -// clients by spec. +// The JSON field names are single letters only to keep the token short; nothing +// outside this file may depend on them, since the token is opaque by spec. type modernCursor struct { Kind string `json:"k"` LastKey string `json:"l"` + // Delivered is how many items sharing LastKey have already been sent, counted + // across every page so far. It is what makes the paginator safe against + // DUPLICATE keys -- see paginateModern for why a bare "strictly above + // LastKey" scan silently loses items. + Delivered int `json:"d,omitempty"` } -// encodeModernCursor builds the opaque token handed to the client as -// nextCursor. base64url keeps it safe in any JSON string and signals opacity; -// it is emphatically NOT encryption, and nothing secret may be put in it. -func encodeModernCursor(kind, lastKey string) (string, error) { - payload, err := json.Marshal(modernCursor{Kind: kind, LastKey: lastKey}) +// encodeModernCursor builds the opaque token handed to the client as nextCursor. +// base64url keeps it safe in any JSON string; it is emphatically NOT encryption +// or authentication, and nothing secret may go in it. A client can trivially +// decode and forge one, which is harmless: a cursor only selects a position +// within a set the caller is already entitled to (admission filtering runs in +// core.List* before this file sees anything) and carries no identity, backend, or +// capability of its own. +func encodeModernCursor(kind, lastKey string, delivered int) (string, error) { + payload, err := json.Marshal(modernCursor{Kind: kind, LastKey: lastKey, Delivered: delivered}) if err != nil { return "", fmt.Errorf("encoding pagination cursor: %w", err) } return base64.RawURLEncoding.EncodeToString(payload), nil } -// decodeModernCursor recovers the last delivered key from a client-supplied -// cursor, rejecting anything that is not a well-formed cursor for wantKind. +// decodeModernCursor recovers a position from a client-supplied cursor, rejecting +// anything that is not a well-formed cursor for wantKind. // // Every failure collapses to errInvalidModernCursor rather than surfacing the // decode detail: the cursor is client-controlled input, and echoing back why it // failed to parse tells a caller about the internal encoding it is required to // treat as opaque. -func decodeModernCursor(wantKind, cursor string) (string, error) { +func decodeModernCursor(wantKind, cursor string) (string, int, error) { + // Length first, so an oversized cursor costs nothing to reject. + if len(cursor) > modernCursorMaxLen { + return "", 0, errInvalidModernCursor + } raw, err := base64.RawURLEncoding.DecodeString(cursor) if err != nil { - return "", errInvalidModernCursor + return "", 0, errInvalidModernCursor } var decoded modernCursor if err := json.Unmarshal(raw, &decoded); err != nil { - return "", errInvalidModernCursor + return "", 0, errInvalidModernCursor } // A syntactically valid decode is not enough: the kind must match the verb - // being served, and an empty LastKey would make the "strictly above" scan - // below return the whole list, silently turning a page request into a - // full-list request. - if decoded.Kind != wantKind || decoded.LastKey == "" { - return "", errInvalidModernCursor + // being served, an empty LastKey would make the scan below return the whole + // list (silently turning a page request into a full-list request), and a + // negative Delivered would rewind. + if decoded.Kind != wantKind || decoded.LastKey == "" || decoded.Delivered < 0 { + return "", 0, errInvalidModernCursor } - return decoded.LastKey, nil + return decoded.LastKey, decoded.Delivered, nil } // paginateModern returns the page of items following cursor, plus the cursor for // the page after it (empty when the returned page is the last one). // -// keyOf must yield a key that is unique across the whole set -- Tool.Name, -// Resource.URI, ResourceTemplate.URITemplate, and Prompt.Name all are, since the -// aggregator's conflict resolver has already de-duplicated them across backends. -// Uniqueness is what makes "strictly above the last key" advance by exactly one -// page with no gap and no overlap; a duplicated key would silently drop every -// item sharing it. +// KEY UNIQUENESS IS NOT ASSUMED, and this is the subtle part. An earlier version +// documented these keys as globally unique "since the aggregator's conflict +// resolver has already de-duplicated them across backends", and skipped forward +// with a bare `key > lastKey`. That precondition holds for exactly ONE of the +// four callers: +// +// - Tool.Name IS unique: ResolveToolConflicts returns map[string]*ResolvedTool, +// and the no-resolver fallback also keys a map (aggregator/default_aggregator.go). +// - Resource.URI, ResourceTemplate.URITemplate and Prompt.Name are NOT: they +// are plain-appended across backends under the comment "no conflict +// resolution for these yet", and nothing dedups them downstream (FilterResources +// only filters). Two honest backends both exposing file:///README.md collide. +// +// With a bare `>` scan, a collision at a page boundary permanently DROPS an item: +// the page ends on the first copy, the cursor names that key, and the scan then +// skips every copy of it, so the second appears on no page at all. Measured +// before this fix, a 1100-resource set with one duplicated URI delivered 1099. +// +// So the cursor carries (LastKey, Delivered) and the scan resumes *within* a run +// of equal keys rather than past it. Items sharing a key are interchangeable for +// this purpose -- Delivered is a position in the run, not an identity -- which +// keeps the walk correct without a secondary sort key this generic signature +// cannot see. A Delivered larger than the run (a copy vanished between pages) +// degrades to "resume at the next key" rather than failing. +// +// keyOf must still yield the item's natural identity; it just no longer has to be +// unique. // // items is never mutated: it is cloned before sorting, because it comes straight // from core.List* and the caller's slice must not be reordered underneath it. func paginateModern[T any](items []T, keyOf func(T) string, kind, cursor string) ([]T, string, error) { - // Sorting is what makes the ordering deterministic across requests, which - // keyset pagination requires: the aggregator's own order is not guaranteed - // stable between calls (backend fan-out completes concurrently), and an - // unstable order would let a cursor skip or repeat items. + // Sorting makes the ordering deterministic across requests, which keyset + // pagination requires: the aggregator's own order is not guaranteed stable + // between calls (backend fan-out completes concurrently). A STABLE sort keeps + // equal-keyed items in their incoming relative order, which costs nothing and + // makes the duplicate case reproducible within a snapshot. sorted := slices.Clone(items) - slices.SortFunc(sorted, func(a, b T) int { - return cmpString(keyOf(a), keyOf(b)) + slices.SortStableFunc(sorted, func(a, b T) int { + return strings.Compare(keyOf(a), keyOf(b)) }) start := 0 + carried := 0 + cursorKey := "" if cursor != "" { - lastKey, err := decodeModernCursor(kind, cursor) + lastKey, delivered, err := decodeModernCursor(kind, cursor) if err != nil { return nil, "", err } - // Strictly above: the item named by the cursor was already delivered. - // A cursor whose key is no longer present (its item was removed between - // pages) still lands correctly here -- the scan resumes at the next key + cursorKey = lastKey + // Binary search for the first key >= lastKey rather than scanning. The + // scan was O(n) per page, so a full walk was O(n^2); this makes it + // O(log n). A cursor whose key is no longer present lands at the next key // above it rather than failing, which is the graceful degradation the - // spec's "stable cursors" guidance is about. - start = len(sorted) - for i, item := range sorted { - if keyOf(item) > lastKey { - start = i + // "stable cursors" guidance is about -- BinarySearchFunc's insertion point + // gives that for free. + start, _ = slices.BinarySearchFunc(sorted, lastKey, func(item T, target string) int { + return strings.Compare(keyOf(item), target) + }) + // Then past the copies of lastKey already delivered, bounded by the run + // itself so a stale count cannot overshoot into the next key. + for range delivered { + if start >= len(sorted) || keyOf(sorted[start]) != lastKey { break } + start++ + carried++ } } @@ -175,12 +286,26 @@ func paginateModern[T any](items []T, keyOf func(T) string, kind, cursor string) page := sorted[start:end] // A next cursor is emitted only when items actually remain. Emitting one on - // the final page would make a conforming client issue a guaranteed-empty - // extra round trip. + // the final page would make a conforming client issue a guaranteed-empty extra + // round trip -- and emitting an empty string rather than omitting the field + // would make it loop on page one (see the file header). if end == len(sorted) || len(page) == 0 { return page, "", nil } - next, err := encodeModernCursor(kind, keyOf(page[len(page)-1])) + + // Count the items sharing the last delivered key in this page, plus anything + // already counted for that same key on earlier pages, so a run spanning + // several pages accumulates instead of resetting. + lastKey := keyOf(page[len(page)-1]) + delivered := 0 + for i := len(page) - 1; i >= 0 && keyOf(page[i]) == lastKey; i-- { + delivered++ + } + if cursorKey == lastKey { + delivered += carried + } + + next, err := encodeModernCursor(kind, lastKey, delivered) if err != nil { return nil, "", err } @@ -188,31 +313,55 @@ func paginateModern[T any](items []T, keyOf func(T) string, kind, cursor string) } // modernRequestCursor extracts the optional cursor from a list request's params. -// An absent, null, or non-string cursor reads as "first page": the field is -// optional, and a malformed params object has already been rejected upstream. -func modernRequestCursor(params json.RawMessage) string { +// +// An absent or explicitly null cursor reads as "first page" -- the field is +// optional, so neither is an error. A cursor that is PRESENT but not a string +// (e.g. `{"cursor": 123}`) is rejected with errInvalidModernCursor rather than +// silently read as "first page": the draft's Error Handling section requires +// invalid cursors to yield -32602, and schema.ts lists "Pagination: Invalid or +// expired cursor values" under the invalid-params class. +// +// Discarding that decode error was a real defect, not a style point: "a +// malformed params object was already rejected upstream" is false for this shape, +// because `{"cursor": 42}` is well-formed JSON and nothing upstream inspects the +// cursor's TYPE. The practical cost is a client with a serialization bug +// re-reading page one forever, each iteration paying a full unmetered backend +// fan-out. +// +// Decoding into json.RawMessage rather than straight into a string is deliberate, +// and it removes a whole hazard class rather than just relocating it. With a +// `Cursor string` field, encoding/json can populate the field AND return an error +// on the same call -- verified: `{"cursor":42,"cursor":"valid"}` yields +// ("valid", type error) -- so a discarded error there threw away a value already +// decoded, and an honoured one would reject a cursor it had successfully read. +// RawMessage captures bytes without type-checking, so there is no partial-decode +// state: duplicate keys resolve last-wins (Go's behaviour everywhere), and the +// single explicit string check below is the only place a type verdict is reached. +// +// Note the asymmetry with an empty-STRING cursor, which is deliberate and not the +// same case: `{"cursor": ""}` is "first page", matching go-sdk's server +// (`params.cursorPtr() == nil || *params.cursorPtr() == ""` selects the full +// sequence). vMCP never mints an empty cursor, so an empty string can only mean +// "no cursor supplied". +func modernRequestCursor(params json.RawMessage) (string, error) { if len(params) == 0 { - return "" + return "", nil } var raw struct { - Cursor string `json:"cursor"` + Cursor json.RawMessage `json:"cursor"` } - if err := json.Unmarshal(params, &raw); err != nil { - return "" + if err := json.Unmarshal(params, &raw); err != nil || len(raw.Cursor) == 0 { + // A params envelope that does not decode at all, or carries no cursor + // field, is "first page"; a broken envelope is rejected upstream by the + // parser. + return "", nil } - return raw.Cursor -} - -// cmpString orders two keys. Spelled out rather than using strings.Compare so -// the comparison here provably matches the `>` used in paginateModern's scan -- -// the two must agree, or a page boundary could skip an item. -func cmpString(a, b string) int { - switch { - case a < b: - return -1 - case a > b: - return 1 - default: - return 0 + if string(raw.Cursor) == "null" { + return "", nil + } + var cursor string + if err := json.Unmarshal(raw.Cursor, &cursor); err != nil { + return "", errInvalidModernCursor } + return cursor, nil } diff --git a/pkg/vmcp/server/modern_pagination_test.go b/pkg/vmcp/server/modern_pagination_test.go index a211edccb3..b53f42d349 100644 --- a/pkg/vmcp/server/modern_pagination_test.go +++ b/pkg/vmcp/server/modern_pagination_test.go @@ -7,8 +7,10 @@ import ( "encoding/base64" "encoding/json" "fmt" + "math/rand/v2" "net/http" "slices" + "strings" "testing" "github.com/stretchr/testify/assert" @@ -21,8 +23,14 @@ import ( // identityKey keys a []string test corpus by its own element value. func identityKey(s string) string { return s } -// makeKeys builds n keys that sort lexicographically in generation order, so a -// test can assert exact page contents without depending on sort subtleties. +// makeKeys builds n distinct keys that sort lexicographically in generation +// order, so a test can assert exact page contents without depending on sort +// subtleties. +// +// Corpora built ONLY from this helper are what hid the duplicate-key drop: it +// mints strictly unique keys, so no test built on it can exercise a collision. +// Anything asserting a completeness property must also be run against +// makeKeysWithDuplicates below. func makeKeys(n int) []string { keys := make([]string, 0, n) for i := range n { @@ -31,6 +39,49 @@ func makeKeys(n int) []string { return keys } +// makeKeysWithDuplicates builds an adversarial corpus of n items whose keys +// deliberately collide at positions that matter. Resources, resource templates +// and prompts are plain-appended across backends with no conflict resolution, so +// this is the realistic shape rather than a synthetic edge case: two backends +// exposing the same URI produce exactly this. +// +// The collisions are placed by SORTED position, which is the subtle part and the +// reason a first attempt at this fixture was useless. paginateModern sorts before +// paging, so a distinctively-named duplicate key like "dup-boundary" does not stay +// where it was generated — it sorts among the "d" keys, nowhere near the page +// boundary, and the boundary case goes untested while looking tested. Collisions +// must therefore be introduced by overwriting a key with its own neighbour's +// value, which keeps the corpus sorted and pins the duplicate to the intended +// index. +// +// Two shapes are built: +// - a run spanning the page-1 boundary (indices pageSize-2 .. pageSize+2 share +// one key), so a page ends part-way through a run of equal keys. This is the +// shape that silently dropped an item. +// - a three-way tie well inside page 1. +func makeKeysWithDuplicates(n int) []string { + keys := make([]string, 0, n) + for i := range n { + keys = append(keys, fmt.Sprintf("k%05d", i)) + } + // Three-way tie inside page 1: indices 10, 11, 12 all take index 10's key. + for _, i := range []int{11, 12} { + if i < n { + keys[i] = keys[10] + } + } + // A run straddling the page-1 boundary: every index from pageSize-2 to + // pageSize+2 takes the key at pageSize-2, so the run begins on page 1 and + // continues onto page 2. + base := modernPageSize - 2 + if base >= 0 && base < n { + for i := base + 1; i <= modernPageSize+2 && i < n; i++ { + keys[i] = keys[base] + } + } + return keys +} + // drainPages walks paginateModern to exhaustion, returning every key delivered // and the number of pages it took. It fails if pagination does not terminate // within a generous bound, so a cursor that fails to advance surfaces as a clear @@ -38,6 +89,17 @@ func makeKeys(n int) []string { func drainPages(t *testing.T, corpus []string) ([]string, int) { t.Helper() + // SHUFFLE before paging. This closes a blind spot that hid a second defect + // class: makeKeys returns pre-sorted keys, so deleting the sort from + // paginateModern entirely left this test green -- meaning the deterministic + // total order the whole keyset scheme rests on was unverified. The aggregator's + // real output is unordered (concurrent backend fan-out), so an unsorted input + // is also the realistic one. Deterministic seed keeps failures reproducible. + shuffled := slices.Clone(corpus) + rng := rand.New(rand.NewPCG(1, 2)) + rng.Shuffle(len(shuffled), func(i, j int) { shuffled[i], shuffled[j] = shuffled[j], shuffled[i] }) + corpus = shuffled + // Non-nil so an empty corpus compares equal to makeKeys(0) rather than // tripping on nil-vs-empty, which is not a distinction under test here. seen := []string{} @@ -55,11 +117,20 @@ func drainPages(t *testing.T, corpus []string) ([]string, int) { return nil, 0 } -// TestPaginateModern_DeliversEveryItemExactlyOnce is the central assertion of the -// pagination half: walking the cursors must yield the complete set, in order, -// with no gap and no duplicate. That is the property the whole scheme exists to -// provide, and the one a broken page boundary silently violates. -func TestPaginateModern_DeliversEveryItemExactlyOnce(t *testing.T) { +// TestPaginateModern_StaticSetDeliveredExactlyOnce is the central assertion of +// the pagination half: walking the cursors over an UNCHANGING set must yield the +// complete set, in order, with no gap and no duplicate. That is the property the +// whole scheme exists to provide, and the one a broken page boundary silently +// violates. +// +// The name says "static set" deliberately. The exactly-once guarantee holds only +// while the underlying set does not change mid-walk: with keyset pagination an +// item inserted BELOW the cursor after a page was served is never reported, since +// the scan only moves forward. That is inherent to the scheme (go-sdk's server +// behaves identically) and is explicitly out of scope per SEP-2567's "cursor +// stability and snapshot consistency" note -- conformant, not a bug. Do not widen +// this name back to an unqualified promise it cannot keep. +func TestPaginateModern_StaticSetDeliveredExactlyOnce(t *testing.T) { t.Parallel() tests := []struct { @@ -83,6 +154,10 @@ func TestPaginateModern_DeliversEveryItemExactlyOnce(t *testing.T) { seen, pages := drainPages(t, corpus) assert.Equal(t, tt.wantPages, pages, "unexpected page count") + // makeKeys is generated in sorted order, so the corpus doubles as the + // expected delivery order -- and because drainPages shuffles its input, + // matching it proves the paginator restored a deterministic total order + // rather than passing the input through. assert.Equal(t, corpus, seen, "every item must be delivered exactly once, in sorted order") distinct := make(map[string]struct{}, len(seen)) @@ -94,6 +169,75 @@ func TestPaginateModern_DeliversEveryItemExactlyOnce(t *testing.T) { } } +// TestPaginateModern_DuplicateKeysAreNotDropped is the regression test for the +// blocking defect: with a bare "strictly above lastKey" scan, two items sharing a +// key at a page boundary caused BOTH to be skipped, so an item appeared on no +// page at all. A 1100-item resource corpus with one duplicated URI delivered 1099. +// +// Key uniqueness holds only for tools (ResolveToolConflicts keys a map). Resource +// URIs, resource-template URI templates and prompt names are plain-appended +// across backends with no conflict resolution, so collisions are ordinary -- two +// backends exposing file:///README.md is enough. The paginator therefore must not +// depend on uniqueness, and this test is what holds it to that. +// +// The corpus is adversarial by construction (see makeKeysWithDuplicates): a pair +// straddling the page-1 boundary, a run spanning it, and a three-way tie. Total +// count is asserted, because "delivered exactly n" is the property that fails +// when a collision eats an item. +func TestPaginateModern_DuplicateKeysAreNotDropped(t *testing.T) { + t.Parallel() + + for _, total := range []int{modernPageSize + 100, modernPageSize + 1, modernPageSize * 2} { + t.Run(fmt.Sprintf("run-spanning-boundary/total=%d", total), func(t *testing.T) { + t.Parallel() + + corpus := makeKeysWithDuplicates(total) + require.Len(t, corpus, total, "fixture must produce the requested item count") + + // Sanity-check the fixture actually collides; a non-colliding corpus + // would make this test silently vacuous, which is how the original + // defect survived. + counts := map[string]int{} + for _, k := range corpus { + counts[k]++ + } + boundaryKey := fmt.Sprintf("k%05d", modernPageSize-2) + assert.Greater(t, counts[boundaryKey], 1, + "fixture must duplicate a key spanning the page boundary") + assert.Equal(t, 3, counts[fmt.Sprintf("k%05d", 10)], + "fixture must include a three-way key tie") + + // The run must genuinely straddle the boundary: some copies on page 1, + // some on page 2. Otherwise this test cannot catch the drop. + sortedCorpus := slices.Clone(corpus) + slices.Sort(sortedCorpus) + onPage1 := 0 + for i := 0; i < modernPageSize && i < len(sortedCorpus); i++ { + if sortedCorpus[i] == boundaryKey { + onPage1++ + } + } + assert.Greater(t, onPage1, 0, "run must start on page 1") + assert.Less(t, onPage1, counts[boundaryKey], "run must continue onto page 2") + + seen, pages := drainPages(t, corpus) + + // The headline: nothing is lost. + assert.Len(t, seen, total, + "a duplicated key must not drop items; every item must be delivered") + assert.Greater(t, pages, 1, "corpus must actually paginate for this test to mean anything") + + // And the multiset matches exactly -- same keys, same multiplicities. + wantSorted := slices.Clone(corpus) + slices.Sort(wantSorted) + gotSorted := slices.Clone(seen) + slices.Sort(gotSorted) + assert.Equal(t, wantSorted, gotSorted, + "delivered multiset must equal the corpus, including duplicate multiplicities") + }) + } +} + // TestPaginateModern_PageBoundaries pins the boundary behaviour the walk above // exercises only indirectly: page size is capped, and a nextCursor appears if and // only if items actually remain. @@ -138,20 +282,28 @@ func TestPaginateModern_PageBoundaries(t *testing.T) { func TestPaginateModern_CursorValidation(t *testing.T) { t.Parallel() - validOtherKind, err := encodeModernCursor(cursorKindPrompts, "k00001") + validOtherKind, err := encodeModernCursor(cursorKindPrompts, "k00001", 1) require.NoError(t, err) - emptyKey, err := encodeModernCursor(cursorKindTools, "") + emptyKey, err := encodeModernCursor(cursorKindTools, "", 1) require.NoError(t, err) + negativeDelivered, err := encodeModernCursor(cursorKindTools, "k00001", -1) + require.NoError(t, err) + + // A cursor over the length cap, rejected before any decode work happens. + oversized := base64.RawURLEncoding.EncodeToString( + []byte(`{"k":"tools","l":"` + strings.Repeat("A", modernCursorMaxLen*2) + `"}`)) + require.Greater(t, len(oversized), modernCursorMaxLen) tests := []struct { - name string - cursor string - wantErr bool + name string + cursor string }{ - {name: "not base64", cursor: "!!!not-base64!!!", wantErr: true}, - {name: "base64 of non-JSON", cursor: base64.RawURLEncoding.EncodeToString([]byte("plain")), wantErr: true}, - {name: "cursor minted for a different list verb", cursor: validOtherKind, wantErr: true}, - {name: "cursor with an empty key", cursor: emptyKey, wantErr: true}, + {name: "not base64", cursor: "!!!not-base64!!!"}, + {name: "base64 of non-JSON", cursor: base64.RawURLEncoding.EncodeToString([]byte("plain"))}, + {name: "cursor minted for a different list verb", cursor: validOtherKind}, + {name: "cursor with an empty key", cursor: emptyKey}, + {name: "cursor with a negative delivered count", cursor: negativeDelivered}, + {name: "cursor longer than the length cap", cursor: oversized}, } for _, tt := range tests { @@ -166,67 +318,104 @@ func TestPaginateModern_CursorValidation(t *testing.T) { t.Parallel() // "k00002" is gone from the corpus; the scan must continue from the next // key above it rather than erroring or restarting. - cursor, err := encodeModernCursor(cursorKindTools, "k00002") + cursor, err := encodeModernCursor(cursorKindTools, "k00002", 1) require.NoError(t, err) page, _, err := paginateModern([]string{"k00001", "k00003", "k00004"}, identityKey, cursorKindTools, cursor) require.NoError(t, err) assert.Equal(t, []string{"k00003", "k00004"}, page) }) + + t.Run("delivered count larger than the run resumes at the next key", func(t *testing.T) { + t.Parallel() + // Claims 9 copies of "dup" were delivered but only 2 exist: must not + // overshoot past "zed". + cursor, err := encodeModernCursor(cursorKindTools, "dup", 9) + require.NoError(t, err) + + page, _, err := paginateModern([]string{"dup", "dup", "zed"}, identityKey, cursorKindTools, cursor) + require.NoError(t, err) + assert.Equal(t, []string{"zed"}, page) + }) } -// TestModernCursor_RoundTripAndOpacity checks the codec, and that the token does -// not leak its payload in plaintext -- clients are required to treat it as -// opaque, so it must not invite parsing. -func TestModernCursor_RoundTripAndOpacity(t *testing.T) { +// TestModernCursor_RoundTrip checks the codec. It deliberately does NOT claim to +// test opacity: base64 provides encoding, not secrecy, and encodeModernCursor's +// own comment says it is "emphatically NOT encryption". Opacity is a rule imposed +// on CLIENTS by the spec, not a property the server can enforce, so asserting the +// key is absent from the token would be near-vacuous theatre. +func TestModernCursor_RoundTrip(t *testing.T) { t.Parallel() - encoded, err := encodeModernCursor(cursorKindResources, "file:///a/b.txt") + encoded, err := encodeModernCursor(cursorKindResources, "file:///a/b.txt", 3) require.NoError(t, err) - got, err := decodeModernCursor(cursorKindResources, encoded) + gotKey, gotDelivered, err := decodeModernCursor(cursorKindResources, encoded) require.NoError(t, err) - assert.Equal(t, "file:///a/b.txt", got) - - assert.NotContains(t, encoded, "file:///", "the encoded cursor must not carry its key in plaintext") + assert.Equal(t, "file:///a/b.txt", gotKey) + assert.Equal(t, 3, gotDelivered) } // TestModernRequestCursor covers cursor extraction from request params, including -// the shapes that must read as "first page" rather than erroring. +// the shapes that must read as "first page" and the shapes that must error. func TestModernRequestCursor(t *testing.T) { t.Parallel() tests := []struct { - name string - params string - want string + name string + params string + want string + wantErr bool }{ {name: "absent params", params: "", want: ""}, {name: "empty object", params: `{}`, want: ""}, {name: "null cursor", params: `{"cursor":null}`, want: ""}, - {name: "non-string cursor", params: `{"cursor":42}`, want: ""}, {name: "malformed params", params: `{`, want: ""}, {name: "present cursor", params: `{"cursor":"abc"}`, want: "abc"}, + // An empty-string cursor is "first page", not an error: vMCP never mints + // one (nextCursor is omitted entirely at end-of-results), so it can only + // mean "no cursor supplied". Matches go-sdk's server. + {name: "empty string cursor is first page", params: `{"cursor":""}`, want: ""}, + // Present but not a string: must be an error, never a silent restart at + // page one. A client with a serialization bug needs a diagnosable -32602. + {name: "number cursor is rejected", params: `{"cursor":42}`, wantErr: true}, + {name: "object cursor is rejected", params: `{"cursor":{"k":"v"}}`, wantErr: true}, + {name: "array cursor is rejected", params: `{"cursor":["a"]}`, wantErr: true}, + {name: "bool cursor is rejected", params: `{"cursor":true}`, wantErr: true}, + // Duplicate JSON keys resolve last-wins, which is Go's behaviour + // everywhere and is why decoding into json.RawMessage matters: there is no + // partial-decode state to mishandle. Whichever value comes last is the one + // type-checked, so the same input is accepted or rejected purely on that + // value -- not on the presence of the duplicate. + {name: "duplicate cursor key takes the last value", params: `{"cursor":42,"cursor":"abc"}`, want: "abc"}, + {name: "duplicate cursor key rejects a bad last value", params: `{"cursor":"abc","cursor":42}`, wantErr: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - assert.Equal(t, tt.want, modernRequestCursor(json.RawMessage(tt.params))) + got, err := modernRequestCursor(json.RawMessage(tt.params)) + if tt.wantErr { + require.ErrorIs(t, err, errInvalidModernCursor) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) }) } } -// TestDispatchModernList_Pagination drives the four list verbs through -// dispatchModern to prove the wire result actually carries nextCursor and that a -// bad cursor is rejected as -32602 rather than -32603. The per-verb table also -// pins that each verb pages on its own unique key, so a cursor from one cannot -// silently page another. -func TestDispatchModernList_Pagination(t *testing.T) { - t.Parallel() - - const total = modernPageSize + 5 - +// paginationFakeCore builds a fresh fake core holding `total` items of every +// kind. Each subtest gets its own instance rather than sharing one: modernFakeCore +// carries write-capable fields (checkCalled, callCalled, gotCtx), so a shared +// value would be a data race the moment a parallel subtest exercised a verb that +// writes them. +// +// Resource NAMES deliberately collide across all items while URIs stay distinct, +// and template names likewise. That makes the choice of pagination key +// observable: keying resources on Name instead of URI collapses the corpus and +// fails, where a fixture with unique names would have accepted either key. +func paginationFakeCore(total int) *modernFakeCore { tools := make([]vmcp.Tool, 0, total) resources := make([]vmcp.Resource, 0, total) templates := make([]vmcp.ResourceTemplate, 0, total) @@ -234,11 +423,20 @@ func TestDispatchModernList_Pagination(t *testing.T) { for i := range total { id := fmt.Sprintf("k%05d", i) tools = append(tools, vmcp.Tool{Name: id, InputSchema: map[string]any{"type": "object"}}) - resources = append(resources, vmcp.Resource{URI: "file:///" + id, Name: id}) - templates = append(templates, vmcp.ResourceTemplate{URITemplate: "file:///{x}/" + id, Name: id}) + resources = append(resources, vmcp.Resource{URI: "file:///" + id, Name: "same-name"}) + templates = append(templates, vmcp.ResourceTemplate{URITemplate: "file:///{x}/" + id, Name: "same-name"}) prompts = append(prompts, vmcp.Prompt{Name: id}) } - fakeCore := &modernFakeCore{tools: tools, resources: resources, templates: templates, prompts: prompts} + return &modernFakeCore{tools: tools, resources: resources, templates: templates, prompts: prompts} +} + +// TestDispatchModernList_Pagination drives the four list verbs through +// dispatchModern to prove the wire result actually carries nextCursor and that a +// bad cursor is rejected as -32602 rather than -32603. +func TestDispatchModernList_Pagination(t *testing.T) { + t.Parallel() + + const total = modernPageSize + 5 tests := []struct { method string @@ -253,6 +451,7 @@ func TestDispatchModernList_Pagination(t *testing.T) { for _, tt := range tests { t.Run(tt.method, func(t *testing.T) { t.Parallel() + fakeCore := paginationFakeCore(total) // Page 1: capped, and carries a cursor. _, body := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ @@ -283,18 +482,244 @@ func TestDispatchModernList_Pagination(t *testing.T) { }) } - t.Run("invalid cursor is invalid params, not internal error", func(t *testing.T) { - t.Parallel() + // Both an undecodable string cursor and a cursor of the wrong JSON type must + // surface as -32602. The non-string case is the one that used to fall through + // to "first page", silently serving page 1 to a client with a serialization + // bug instead of giving it a diagnosable error. + for _, tt := range []struct { + name string + params string + }{ + {name: "undecodable string cursor", params: `{"cursor":"!!!bogus!!!"}`}, + {name: "non-string cursor", params: `{"cursor":42}`}, + } { + t.Run("invalid cursor is invalid params, not internal error: "+tt.name, func(t *testing.T) { + t.Parallel() + + rec, body := dispatchModernTest(t.Context(), t, paginationFakeCore(10), false, &mcpparser.ParsedMCPRequest{ + ID: 3, Method: "tools/list", Params: json.RawMessage(tt.params), + }) + assert.Equal(t, http.StatusBadRequest, 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(jsonRPCCodeInvalidParams), errObj["code"]) + assert.Equal(t, "invalid cursor", errObj["message"], + "the message must not describe the cursor encoding clients are told to treat as opaque") + + _, isResult := body["result"] + assert.False(t, isResult, "a rejected cursor must not also serve a page") + }) + } +} + +// TestDispatchModernList_CursorNotReplayableAcrossVerbs is the end-to-end twin of +// the kind check. Without it the kind field is self-consistently verified -- +// encode and decode both read the same site-local constant, so mutating one verb +// to mint another verb's kind passes every unit test while, in production, a +// prompts/list cursor would be accepted by tools/list. +// +// This replays a real cursor from one verb against every other verb through +// dispatchModern, which is the only way to catch that class of mistake. +func TestDispatchModernList_CursorNotReplayableAcrossVerbs(t *testing.T) { + t.Parallel() + + const total = modernPageSize + 5 + verbs := []string{"tools/list", "resources/list", "resources/templates/list", "prompts/list"} - rec, body := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ - ID: 3, Method: "tools/list", Params: json.RawMessage(`{"cursor":"!!!bogus!!!"}`), + // Mint one real cursor per verb, through the dispatcher. + cursors := make(map[string]string, len(verbs)) + for _, verb := range verbs { + _, body := dispatchModernTest(t.Context(), t, paginationFakeCore(total), false, &mcpparser.ParsedMCPRequest{ + ID: 1, Method: verb, Params: json.RawMessage(`{}`), }) - assert.Equal(t, http.StatusBadRequest, rec.Code) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "%s: expected a result, got %v", verb, body) + cursor, ok := result["nextCursor"].(string) + require.True(t, ok, "%s: expected a nextCursor", verb) + cursors[verb] = cursor + } + + for _, minted := range verbs { + for _, replayed := range verbs { + if minted == replayed { + continue + } + t.Run(minted+" cursor rejected by "+replayed, func(t *testing.T) { + t.Parallel() - errObj, ok := body["error"].(map[string]any) - require.True(t, ok, "expected a JSON-RPC error envelope, got %v", body) - assert.Equal(t, float64(jsonRPCCodeInvalidParams), errObj["code"]) - assert.Equal(t, "invalid cursor", errObj["message"], - "the message must not describe the cursor encoding clients are told to treat as opaque") + params, err := json.Marshal(map[string]any{"cursor": cursors[minted]}) + require.NoError(t, err) + + rec, body := dispatchModernTest(t.Context(), t, paginationFakeCore(total), false, + &mcpparser.ParsedMCPRequest{ID: 9, Method: replayed, Params: params}) + + assert.Equal(t, http.StatusBadRequest, rec.Code) + errObj, ok := body["error"].(map[string]any) + require.True(t, ok, "expected an error envelope, got %v", body) + assert.Equal(t, float64(jsonRPCCodeInvalidParams), errObj["code"]) + + _, isResult := body["result"] + assert.False(t, isResult, "a cross-verb cursor must not serve a page") + }) + } + } +} + +// TestModernList_EndOfResultsOmitsCursor pins the draft-only pagination +// amendment: end-of-results MUST be signalled by OMITTING nextCursor, never by +// emitting an empty string. Per the draft's Implementation Guidelines, "an empty +// string is a valid cursor and thus MUST NOT be treated as the end of results" -- +// so a client receiving `nextCursor: ""` would re-request with `cursor: ""`, +// which reads as "first page", and loop forever being served page one. +// +// This is a wire-serialization guarantee (the `omitempty` struct tags in +// modern_envelope.go), so it is asserted on the marshalled JSON rather than on +// the Go struct, where the zero value is indistinguishable from a dropped field. +func TestModernList_EndOfResultsOmitsCursor(t *testing.T) { + t.Parallel() + + for _, method := range []string{"tools/list", "resources/list", "resources/templates/list", "prompts/list"} { + t.Run(method, func(t *testing.T) { + t.Parallel() + + // A single short page: there is no next page, so no cursor may appear. + rec, body := dispatchModernTest(t.Context(), t, paginationFakeCore(1), false, + &mcpparser.ParsedMCPRequest{ID: 1, Method: method, Params: json.RawMessage(`{}`)}) + + result, ok := body["result"].(map[string]any) + require.True(t, ok, "expected a result envelope, got %v", body) + + assert.NotContains(t, result, "nextCursor", + "end-of-results must omit nextCursor entirely, not emit an empty string") + assert.NotContains(t, rec.Body.String(), `"nextCursor"`, + "the key must be absent from the wire bytes, not merely empty") + }) + } +} + +// TestModernPageSize_MatchesSDKDefaultBoundary pins the page-size value +// behaviourally rather than by reading the constant, so it fails on a real +// behaviour change instead of tautologically agreeing with whatever the constant +// says. +// +// modernPageSize is a hand-copied duplicate of go-sdk's DefaultPageSize (1000), +// which is what the Legacy/SDK path applies to the same split. mcpcompat does not +// re-export the constant, so nothing links the two: if go-sdk changes its +// default, Legacy follows and Modern does not. +// +// This pins the Modern half — a 1001-item corpus must produce exactly a 1000-item +// first page and a 1-item second. The Legacy half is NOT pinned here; doing so +// needs a harness that can hold a client on Legacy, which this package lacks +// (see the constant's doc comment). Asserting only the reachable half, and saying +// so, beats asserting a pairing this test cannot observe. +func TestModernPageSize_MatchesSDKDefaultBoundary(t *testing.T) { + t.Parallel() + + const sdkDefaultPageSize = 1000 + require.Equal(t, sdkDefaultPageSize, modernPageSize, + "modernPageSize must track go-sdk's DefaultPageSize; if go-sdk changed, update both and re-check Legacy") + + fakeCore := paginationFakeCore(sdkDefaultPageSize + 1) + + _, body := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ + ID: 1, Method: "tools/list", Params: json.RawMessage(`{}`), }) + result, ok := body["result"].(map[string]any) + require.True(t, ok, "expected a result envelope, got %v", body) + items, ok := result["tools"].([]any) + require.True(t, ok) + assert.Len(t, items, sdkDefaultPageSize, "the first page must hold exactly the SDK default page size") + + cursor, ok := result["nextCursor"].(string) + require.True(t, ok, "one item remains, so a cursor is required") + + params, err := json.Marshal(map[string]any{"cursor": cursor}) + require.NoError(t, err) + _, body2 := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ + ID: 2, Method: "tools/list", Params: params, + }) + result2, ok := body2["result"].(map[string]any) + require.True(t, ok) + items2, ok := result2["tools"].([]any) + require.True(t, ok) + assert.Len(t, items2, 1, "the tail page must hold the single remaining item") +} + +// TestPaginateModern_DuplicateShapes covers the remaining two shapes of the +// duplicate-key defect, whose loss profile differs sharply and whose rule is +// easy to state wrongly. +// +// The general rule: every item whose key equals the LAST key delivered on a page +// was permanently dropped. So loss scaled with the size of the duplicate run +// straddling a page boundary, and a duplicate NOT at a boundary was harmless -- +// which is exactly why the bug was easy to miss. +// +// - single duplicate exactly at the boundary: 1 item lost +// - duplicate mid-page: 0 lost (harmless, but must stay correct) +// - run of 50 at the boundary: 50 lost +// - every key identical: an entire page lost, and page 2 came back empty +func TestPaginateModern_DuplicateShapes(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + corpus func() []string + }{ + { + name: "single duplicate exactly at the page boundary", + corpus: func() []string { + keys := makeKeys(modernPageSize + 100) + keys[modernPageSize] = keys[modernPageSize-1] + return keys + }, + }, + { + name: "duplicate mid-page is harmless but must stay correct", + corpus: func() []string { + keys := makeKeys(modernPageSize + 100) + keys[500] = keys[499] + return keys + }, + }, + { + name: "run of 50 copies straddling the boundary", + corpus: func() []string { + keys := makeKeys(modernPageSize + 100) + for i := modernPageSize - 25; i < modernPageSize+25; i++ { + keys[i] = keys[modernPageSize-25] + } + return keys + }, + }, + { + name: "every key identical", + corpus: func() []string { + keys := make([]string, modernPageSize+100) + for i := range keys { + keys[i] = "same" + } + return keys + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + corpus := tt.corpus() + seen, _ := drainPages(t, corpus) + + assert.Len(t, seen, len(corpus), + "every item must be delivered; a duplicated key must never drop one") + + wantSorted := slices.Clone(corpus) + slices.Sort(wantSorted) + gotSorted := slices.Clone(seen) + slices.Sort(gotSorted) + assert.Equal(t, wantSorted, gotSorted, + "delivered multiset must match the corpus, duplicate multiplicities included") + }) + } } diff --git a/pkg/vmcp/server/modern_subscriptions.go b/pkg/vmcp/server/modern_subscriptions.go index d77d4bb413..cab8d61117 100644 --- a/pkg/vmcp/server/modern_subscriptions.go +++ b/pkg/vmcp/server/modern_subscriptions.go @@ -57,15 +57,27 @@ import ( // Modern subscription method and notification names, plus the reserved _meta // key deliveries are tagged with. // -// Provenance, since it differs per constant: the two method/notification names -// and the per-type opt-in shape are specified in SEP-2575 prose. The -// subscriptionId key and the convention of using the listen request's own -// JSON-RPC id as the subscription identity are NOT in the spec text -- they are -// taken from go-sdk@v1.7.0-pre.3 (MetaKeySubscriptionID at protocol.go:2377-2379; -// server.go:1187-1250 keys subscriptions by the request id and stamps it into -// both the acknowledgement and the final result). That is the reference -// implementation rather than normative prose, so treat it as the de facto wire -// contract and re-check it when the SEP lands. +// All three are NORMATIVE in schema/draft/schema.ts -- do not weaken them to +// "SDK convention". The relevant declarations: +// +// - SubscriptionsListenResultMeta requires (non-optional) +// "io.modelcontextprotocol/subscriptionId", whose value is "the JSON-RPC ID +// of the subscriptions/listen request that opened the stream (and equals +// this response's id)" -- so request-id-as-subscription-identity is +// specified, not invented. +// - SubscriptionsAcknowledgedNotification: "This notification MUST be the +// first message the server sends carrying the subscription's ID ... The +// server MUST NOT send any notification on the subscription before +// acknowledging it." +// - NotificationMetaObject: "The server MUST include this key on every +// notification delivered via a subscriptions/listen stream." That third one +// binds a DELIVERY implementation rather than this handler (which delivers +// nothing); it is restated at the #5743 hand-off below so it is not lost. +// +// go-sdk agrees and can be read as a cross-check (MetaKeySubscriptionID at +// protocol.go:2377-2379; server.go:1187-1250), but the schema is the authority. +// An earlier version of this comment attributed these to the SDK because the +// SEP-2575 prose does not restate them -- the schema does, and it governs. const ( methodSubscriptionsListen = "subscriptions/listen" notificationSubscriptionsAcked = "notifications/subscriptions/acknowledged" @@ -149,25 +161,48 @@ func (s *Server) dispatchModernSubscriptionsListen( } // Resolve what this identity may reach, then shape it through the same - // capability builder server/discover publishes. Costs one backend fan-out, - // like dispatchModernDiscover; see its note on the absent cross-request cache. - caps, err := s.core.Discover(ctx, identity) - if err != nil { - writeModernListError(ctx, w, parsed.ID, parsed.Method, err) - return + // capability builder server/discover publishes. + // + // The pre-check first: core.Discover is an un-rate-limited backend fan-out + // (ratelimit/decorator.go meters CallTool only), and while no push capability + // is advertised its result cannot change the answer -- every possible + // DiscoverCapabilities combination yields the same empty honored set, because + // the has* booleans only decide whether a capability POINTER is non-nil, never + // the listChanged/subscribe flags inside it. Calling it anyway meant N no-op + // requests cost N fan-outs, and that got worse once list verbs began paging. + // So probe the ceiling -- everything present -- and skip the fan-out when even + // that honors nothing. This is a pure optimisation: it cannot change the + // response, only what it costs. + ceiling := honoredSubscriptions(*params.Notifications, newModernCapabilities(true, true, true, true)) + honored := ceiling + if !ceiling.isEmpty() { + caps, err := s.core.Discover(ctx, identity) + if err != nil { + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) + return + } + advertised := newModernCapabilities( + caps.HasTools, caps.HasResources, caps.HasResourceTemplates, caps.HasPrompts) + honored = honoredSubscriptions(*params.Notifications, advertised) } - advertised := newModernCapabilities(caps.HasTools, caps.HasResources, caps.HasResourceTemplates, caps.HasPrompts) - honored := honoredSubscriptions(*params.Notifications, advertised) // The subscription id is the listen request's own JSON-RPC id. Modern has no // sessions, so this -- not an Mcp-Session-Id -- is what a delivery // implementation would key streams by. subscriptionMeta := map[string]any{modernSubscriptionIDKey: parsed.ID} - if err := writeModernListenStream(w, parsed.ID, honored, subscriptionMeta); err != nil { - // The client hung up or the stream could not be flushed. Nothing is - // recoverable at this point (headers are already committed), so log and - // return rather than attempting an error envelope on a dead stream. + // Build first: a marshal failure here is still reportable as -32603, because + // nothing has been written yet. + frames, err := buildModernListenFrames(parsed.ID, honored, subscriptionMeta) + if err != nil { + writeModernListError(ctx, w, parsed.ID, parsed.Method, err) + return + } + + if err := writeModernListenStream(w, frames); err != nil { + // Only reachable mid-stream: the frames were built successfully and the + // status line is already sent, so there is no way to turn this into a + // JSON-RPC error. Log it and let the connection drop. slog.DebugContext(ctx, "vmcp modern dispatch: subscriptions/listen stream ended early", "error", err) return @@ -180,9 +215,25 @@ func (s *Server) dispatchModernSubscriptionsListen( // Whoever flips a capability flag owns adding both (#5743) -- this WARN // exists so that, if a flag is flipped without it, the gap is loud in the // logs instead of presenting as a silently idle client subscription. + // + // Hand-off note for that work, since it is a MUST and easy to miss: per + // schema.ts's NotificationMetaObject, the server MUST include + // io.modelcontextprotocol/subscriptionId on EVERY notification delivered + // via a subscriptions/listen stream -- not just on the acknowledgement + // this handler already tags. A delivery implementation that pushes a bare + // notifications/tools/list_changed onto the stream is non-conformant even + // though the subscription itself was acknowledged correctly. + // + // Logged by COUNT, never by URI: resourceSubscriptions is client-supplied + // and only length-capped, so echoing the array into the log sink on an + // unmetered verb would let a caller choose how much it writes per request. slog.WarnContext(ctx, "vmcp modern dispatch: subscriptions/listen honored a subscription "+ "but vMCP has no delivery mechanism; notifications will not arrive", - "subscription_id", parsed.ID, "honored", honored) + "subscription_id", parsed.ID, + "tools_list_changed", honored.ToolsListChanged, + "prompts_list_changed", honored.PromptsListChanged, + "resources_list_changed", honored.ResourcesListChanged, + "resource_subscription_count", len(honored.ResourceSubscriptions)) } } @@ -220,15 +271,21 @@ func honoredSubscriptions( return honored } -// writeModernListenStream writes the two-frame SSE response: the mandatory -// initial acknowledgement notification, then the terminating result. +// buildModernListenFrames marshals the two frames the listen response carries: +// the mandatory initial acknowledgement notification, then the terminating +// result. // -// Both frames are marshalled before any header is written, so a marshal failure -// cannot leave a half-written response -- the same build-then-write ordering -// writeModernEnvelope uses. -func writeModernListenStream( - w http.ResponseWriter, id any, honored notificationSubscriptions, subscriptionMeta map[string]any, -) error { +// It is separate from writeModernListenStream so that everything which can fail +// BEFORE any byte is written stays on a path that can still produce a proper +// JSON-RPC error. Folding these into the writer meant a marshal failure returned +// after headers were nominally "committed" when in fact nothing had been written +// at all, so the client received a bare HTTP 200 with an empty body for a request +// carrying an id. Now the caller maps a build failure to -32603, matching +// writeModernEnvelope's own build-then-write ordering, and only a genuine +// mid-stream write failure is unreportable. +func buildModernListenFrames( + id any, honored notificationSubscriptions, subscriptionMeta map[string]any, +) ([][]byte, error) { ack, err := json.Marshal(map[string]any{ "jsonrpc": "2.0", "method": notificationSubscriptionsAcked, @@ -238,7 +295,7 @@ func writeModernListenStream( }, }) if err != nil { - return fmt.Errorf("marshalling subscriptions acknowledgement: %w", err) + return nil, fmt.Errorf("marshalling subscriptions acknowledgement: %w", err) } result, err := json.Marshal(map[string]any{ "jsonrpc": "2.0", @@ -249,27 +306,73 @@ func writeModernListenStream( }, }) if err != nil { - return fmt.Errorf("marshalling subscriptions listen result: %w", err) + return nil, fmt.Errorf("marshalling subscriptions listen result: %w", err) } + return [][]byte{ack, result}, nil +} - // A ResponseWriter that cannot flush would buffer both frames until the - // handler returned, which for a stream the client reads incrementally is - // indistinguishable from a hang. Fail before committing headers instead. - flusher, ok := w.(http.Flusher) - if !ok { - return fmt.Errorf("response writer does not support streaming") - } +// writeModernListenStream writes pre-marshalled frames as an SSE response and +// closes it. Everything it can return is a mid-stream failure -- the frames are +// already built and the status line is already gone -- so its error is only ever +// loggable, never reportable to the client. +func writeModernListenStream(w http.ResponseWriter, frames [][]byte) error { + // http.ResponseController rather than a w.(http.Flusher) type assertion. + // + // The assertion would not establish what it appears to. Every ResponseWriter + // wrapper in vMCP's middleware chain -- audit/auditor.go, + // telemetry/middleware.go, bodylimit/middleware.go, mcp/tool_filter.go -- + // defines Flush() UNCONDITIONALLY and forwards only if the writer it wraps + // supports it. So the assertion succeeds whenever a wrapper is present, + // whatever the writer underneath can do, and says nothing about whether a + // flush will actually reach the socket. ResponseController unwraps the chain + // (via Unwrap) and reports a real answer. + // + // The tradeoff, stated because it costs something: there is no longer a + // PRE-write capability check. ResponseController exposes no "can you flush" + // predicate -- the only way to find out is to call Flush, which implicitly + // commits the header -- so an unflushable writer now surfaces as a mid-stream + // error rather than a -32603 before anything is written. That is deliberate: + // the old assertion answered early but WRONGLY, and an early wrong answer on a + // path no deployment can reach is worse than a correct late one. If Go ever + // grows a flushability predicate, move this back ahead of WriteHeader. + rc := http.NewResponseController(w) w.Header().Set("Content-Type", "text/event-stream") - w.Header().Set("Cache-Control", "no-cache, no-transform") + // no-store as well as no-cache: no-cache still permits a shared cache to + // STORE the response and revalidate, which for a per-identity stream is wrong. + w.Header().Set("Cache-Control", "no-cache, no-store, no-transform, private") w.Header().Set("Connection", "keep-alive") + // Draft Streamable HTTP, "When initiating an SSE stream, servers SHOULD + // include the X-Accel-Buffering: no header", so a reverse proxy (nginx and + // friends) does not accumulate events before forwarding them. This is the + // only SSE-emitting path in the codebase, so there is nowhere else to copy + // the header from. + w.Header().Set("X-Accel-Buffering", "no") w.WriteHeader(http.StatusOK) - for _, frame := range [][]byte{ack, result} { + for _, frame := range frames { + // Frame format is load-bearing, not cosmetic: an SSE event is terminated + // by a BLANK line, so the trailing "\n\n" is what makes a parser dispatch + // the event. Emitting a single "\n" leaves both frames inside one + // never-dispatched event -- the client connects and then silently receives + // nothing, which is exactly the mutation that escaped both this package's + // tests and the real-client integration suite. + // + // The "event: message" line is NOT mandated: the draft Streamable HTTP + // page specifies the content type and the stream lifecycle but never an + // SSE event name, and it explicitly drops resumability ("Resumable SSE + // streams via Last-Event-ID are not supported"), which is why no "id:" + // field is emitted either. The name is kept for symmetry with go-sdk's + // framing; do not add an "id:" to match it. if _, err := fmt.Fprintf(w, "event: message\ndata: %s\n\n", frame); err != nil { return fmt.Errorf("writing subscriptions/listen frame: %w", err) } - flusher.Flush() + // A writer that genuinely cannot flush would buffer both frames until the + // handler returned, which for a stream read incrementally is + // indistinguishable from a hang. Report it rather than hanging. + if err := rc.Flush(); err != nil { + return fmt.Errorf("flushing subscriptions/listen frame: %w", err) + } } return nil } diff --git a/pkg/vmcp/server/modern_subscriptions_test.go b/pkg/vmcp/server/modern_subscriptions_test.go index 3a463dabe7..b22b243ff1 100644 --- a/pkg/vmcp/server/modern_subscriptions_test.go +++ b/pkg/vmcp/server/modern_subscriptions_test.go @@ -6,7 +6,6 @@ package server import ( "context" "encoding/json" - "errors" "net/http" "net/http/httptest" "strings" @@ -88,7 +87,10 @@ func TestHonoredSubscriptions(t *testing.T) { expect: notificationSubscriptions{}, }, { - name: "advertised but not wanted is not honored", + // Named for what it asserts: everything is advertised, only tools is + // wanted, so ONLY tools comes back -- the three advertised-but-unwanted + // types must be absent. Exact equality is what pins that. + name: "only the wanted subset is honored when all are advertised", want: notificationSubscriptions{ToolsListChanged: true}, advertised: pushCaps(true, true, true, true), expect: notificationSubscriptions{ToolsListChanged: true}, @@ -150,19 +152,12 @@ func listenSSE(t *testing.T, fakeCore *modernFakeCore, params any, id any) (*htt Params: raw, }) - var frames []map[string]any - for _, block := range strings.Split(rec.Body.String(), "\n\n") { - for _, line := range strings.Split(block, "\n") { - data, ok := strings.CutPrefix(line, "data: ") - if !ok { - continue - } - var frame map[string]any - require.NoError(t, json.Unmarshal([]byte(data), &frame)) - frames = append(frames, frame) - } + // Only strict-parse an actual stream; a rejected listen answers with a plain + // JSON error envelope and has no frames. + if rec.Header().Get("Content-Type") != "text/event-stream" { + return rec, nil } - return rec, frames + return rec, parseSSEStrict(t, rec.Body.String()) } // TestDispatchModernSubscriptionsListen_AcknowledgesEmptyHonoredSet covers the @@ -227,12 +222,19 @@ func TestDispatchModernSubscriptionsListen_AcknowledgesEmptyHonoredSet(t *testin func TestDispatchModernSubscriptionsListen_Errors(t *testing.T) { t.Parallel() + // NOTE: there is deliberately no "capability resolution failure" case here. + // The M2 pre-check skips core.Discover entirely while no push capability is + // advertised, so the fan-out -- and its error path -- is unreachable from this + // handler today. Testing it would require a capability flag that + // newModernCapabilities never sets, and asserting a branch the code cannot + // reach would be theatre. TestNewModernCapabilities_AdvertisesNoPushCapability + // is what pins the precondition that makes it unreachable; if a flag is ever + // flipped, the error path becomes live and wants a case here. tests := []struct { - name string - params any - discoverErr error - wantCode float64 - wantStatus int + name string + params any + wantCode float64 + wantStatus int }{ { name: "missing notifications field", @@ -246,21 +248,13 @@ func TestDispatchModernSubscriptionsListen_Errors(t *testing.T) { wantCode: jsonRPCCodeInvalidParams, wantStatus: http.StatusBadRequest, }, - { - name: "capability resolution failure", - params: map[string]any{"notifications": map[string]any{"toolsListChanged": true}}, - discoverErr: errors.New("backend fan-out failed"), - wantCode: jsonRPCCodeInternalError, - wantStatus: http.StatusOK, - }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { t.Parallel() - fakeCore := &modernFakeCore{discoverErr: tt.discoverErr} - rec, _ := listenSSE(t, fakeCore, tt.params, 7) + rec, _ := listenSSE(t, &modernFakeCore{}, tt.params, 7) assert.Equal(t, tt.wantStatus, rec.Code) assert.NotEqual(t, "text/event-stream", rec.Header().Get("Content-Type"), @@ -271,11 +265,190 @@ func TestDispatchModernSubscriptionsListen_Errors(t *testing.T) { errObj, ok := body["error"].(map[string]any) require.True(t, ok, "expected a JSON-RPC error envelope") assert.Equal(t, tt.wantCode, errObj["code"]) + }) + } +} + +// parseSSEStrict parses an SSE body the way a compliant client does, and is +// deliberately STRICT where the previous helper was lenient. +// +// That leniency hid a real gap: it split on "\n\n" and then fell back to matching +// any line beginning "data: ", so replacing the blank-line event terminator with +// a single "\n" left the whole suite green -- including the real-client +// integration suite -- even though a conformant parser would see one +// never-terminated event and dispatch neither frame. The ack-then-result contract +// was therefore unverifiable. +// +// Rules enforced here, per the SSE grammar: events are separated by a blank +// line, every event must be terminated (the body ends with a blank line), and +// each event carries exactly one "data:" field. +func parseSSEStrict(t *testing.T, body string) []map[string]any { + t.Helper() + + require.NotEmpty(t, body, "expected an SSE body") + require.True(t, strings.HasSuffix(body, "\n\n"), + "SSE body must end with a blank line; an unterminated final event is never dispatched by a client") + + var frames []map[string]any + for _, block := range strings.Split(strings.TrimSuffix(body, "\n\n"), "\n\n") { + require.NotEmpty(t, block, "empty SSE event block: two consecutive blank lines") - // The generic internal-error text must not leak aggregation detail. - if tt.discoverErr != nil { - assert.NotContains(t, errObj["message"], "backend fan-out failed") + var data string + seenData := false + for _, line := range strings.Split(block, "\n") { + field, value, ok := strings.Cut(line, ":") + require.True(t, ok, "malformed SSE line %q: no field separator", line) + value = strings.TrimPrefix(value, " ") + switch field { + case "event": + assert.Equal(t, "message", value, "MCP SSE frames must use the 'message' event type") + case "data": + require.False(t, seenData, "more than one data field in a single SSE event") + data, seenData = value, true + default: + t.Fatalf("unexpected SSE field %q", field) } - }) + } + require.True(t, seenData, "SSE event carried no data field") + + var frame map[string]any + require.NoError(t, json.Unmarshal([]byte(data), &frame), "SSE data must be valid JSON") + frames = append(frames, frame) + } + return frames +} + +// TestDispatchModernSubscriptionsListen_WireContract pins the literal strings and +// envelope fields the protocol requires. Every one of these was previously free: +// mutating the method name, the acknowledgement name, or the subscriptionId key +// left the whole package green, and those last two are precisely the constants +// whose provenance was least certain. +func TestDispatchModernSubscriptionsListen_WireContract(t *testing.T) { + t.Parallel() + + // Literal, not the constant: asserting against the constant would pass even if + // the constant itself were mistyped, which is the mutation that escaped. + assert.Equal(t, "subscriptions/listen", methodSubscriptionsListen) + assert.Equal(t, "notifications/subscriptions/acknowledged", notificationSubscriptionsAcked) + assert.Equal(t, "io.modelcontextprotocol/subscriptionId", modernSubscriptionIDKey) + + rec, frames := listenSSE(t, &modernFakeCore{}, subscriptionsListenParams{ + Notifications: ¬ificationSubscriptions{ToolsListChanged: true}, + }, "sub-wire") + + assert.Equal(t, http.StatusOK, rec.Code) + require.Len(t, frames, 2) + + // Both frames must declare JSON-RPC 2.0 -- unasserted before, and a protocol + // violation if dropped. + for i, frame := range frames { + assert.Equal(t, "2.0", frame["jsonrpc"], "frame %d must carry jsonrpc 2.0", i) } + + // The acknowledgement must be FIRST and must be the ack method: schema.ts + // makes both a MUST ("This notification MUST be the first message the server + // sends carrying the subscription's ID"). + assert.Equal(t, "notifications/subscriptions/acknowledged", frames[0]["method"], + "the first frame must be the acknowledgement, by literal name") + assert.NotContains(t, frames[0], "id", "a notification must not carry an id") + + // The result frame closes the subscription and echoes the request id. + assert.Equal(t, "sub-wire", frames[1]["id"]) + assert.NotContains(t, frames[1], "method", "a response must not carry a method") + + // The subscriptionId key, by literal string, on both frames. + ackParams, ok := frames[0]["params"].(map[string]any) + require.True(t, ok) + ackMeta, ok := ackParams["_meta"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "sub-wire", ackMeta["io.modelcontextprotocol/subscriptionId"]) + + resultBody, ok := frames[1]["result"].(map[string]any) + require.True(t, ok) + resultMeta, ok := resultBody["_meta"].(map[string]any) + require.True(t, ok) + assert.Equal(t, "sub-wire", resultMeta["io.modelcontextprotocol/subscriptionId"]) +} + +// TestNewModernCapabilities_AdvertisesNoPushCapability is the guard the whole +// empty-honored-set design rests on. +// +// The argument for this handler being conformant rather than a silent no-op has +// two halves: (1) the advertisement and the honored set are computed from one +// source, which the code enforces, and (2) that source advertises nothing +// pushable -- which, before this test, was enforced by nothing at all. The safety +// property was "no line of source sets these true", and a runtime WARN only fires +// after such a line ships, in production, per request, naming a developer action +// an operator cannot remedy. +// +// So it is asserted here, at build time, for every combination of the presence +// flags. +// +// IF YOU FLIP ONE OF THESE: you are promising to deliver that notification type. +// Nothing in vMCP can keep that promise yet -- dispatchModern creates no session, +// so the list_changed machinery never runs for a Modern client, and backend +// resources/updated is not forwarded even on Legacy. Implement delivery in the +// same change (#5743), including the schema MUST that every notification on a +// listen stream carries io.modelcontextprotocol/subscriptionId, and update this +// test deliberately rather than to make it pass. +func TestNewModernCapabilities_AdvertisesNoPushCapability(t *testing.T) { + t.Parallel() + + for _, hasTools := range []bool{false, true} { + for _, hasResources := range []bool{false, true} { + for _, hasTemplates := range []bool{false, true} { + for _, hasPrompts := range []bool{false, true} { + caps := newModernCapabilities(hasTools, hasResources, hasTemplates, hasPrompts) + + if caps.Tools != nil { + assert.False(t, caps.Tools.ListChanged, + "tools.listChanged must stay false until delivery exists (#5743)") + } + if caps.Prompts != nil { + assert.False(t, caps.Prompts.ListChanged, + "prompts.listChanged must stay false until delivery exists (#5743)") + } + if caps.Resources != nil { + assert.False(t, caps.Resources.ListChanged, + "resources.listChanged must stay false until delivery exists (#5743)") + assert.False(t, caps.Resources.Subscribe, + "resources.subscribe must stay false until delivery exists (#5743)") + } + + // And the consequence: a client asking for everything is + // honored nothing, whatever it can reach. + assert.True(t, honoredSubscriptions(allWanted(), caps).isEmpty(), + "no capability combination may honor a subscription") + } + } + } + } +} + +// TestDispatchModernSubscriptionsListen_SkipsFanOutWhenNothingHonorable pins the +// pre-check: core.Discover is an un-rate-limited backend fan-out whose result +// cannot change the answer while no push capability is advertised, so it must not +// be called at all. Without this, N no-op listens cost N fan-outs. +// +// discoverCalls counting is the point -- the response is identical either way, so +// only the call count distinguishes the optimisation being present from absent. +func TestDispatchModernSubscriptionsListen_SkipsFanOutWhenNothingHonorable(t *testing.T) { + t.Parallel() + + fakeCore := &modernFakeCore{discoverCaps: core.DiscoverCapabilities{ + HasTools: true, HasResources: true, HasResourceTemplates: true, HasPrompts: true, + }} + + _, frames := listenSSE(t, fakeCore, subscriptionsListenParams{ + Notifications: ¬ificationSubscriptions{ + ToolsListChanged: true, + PromptsListChanged: true, + ResourcesListChanged: true, + ResourceSubscriptions: []string{"file:///a", "file:///b"}, + }, + }, "sub-nofanout") + + require.Len(t, frames, 2, "the response must be unaffected by the pre-check") + assert.Zero(t, fakeCore.discoverCalls, + "core.Discover must not be called when no push capability is advertised") } From 1588e6a4177e71d1133a1a0c7533bbef815ab168 Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 20:50:06 +0000 Subject: [PATCH 4/4] Document the unreachable paths a capability flip re-enables Three comment corrections from review follow-up, no behaviour change. Name what flipping a push capability flag re-enables: the core.Discover call the ceiling pre-check currently skips, that call's error path whose test was removed rather than left asserting an unenterable branch, and the handler's non-empty-honored branch. All three are uncovered today because they cannot be entered, so whoever flips a flag needs the list. State the cost of moving to http.ResponseController: there is no longer a pre-write capability check, because Go exposes no flushability predicate and calling Flush commits the header. An unflushable writer now surfaces mid-stream instead of as -32603. Deliberate -- the old type assertion answered early but wrongly, since every ResponseWriter wrapper in the chain defines Flush unconditionally. Stop the page-size comment presuming how the Legacy twin will be pinned. Whether that is a pinning harness or a loud failure at negotiation is undecided, and naming one would let the comment go stale. --- pkg/vmcp/server/modern_subscriptions_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/vmcp/server/modern_subscriptions_test.go b/pkg/vmcp/server/modern_subscriptions_test.go index b22b243ff1..36513551d8 100644 --- a/pkg/vmcp/server/modern_subscriptions_test.go +++ b/pkg/vmcp/server/modern_subscriptions_test.go @@ -391,6 +391,16 @@ func TestDispatchModernSubscriptionsListen_WireContract(t *testing.T) { // same change (#5743), including the schema MUST that every notification on a // listen stream carries io.modelcontextprotocol/subscriptionId, and update this // test deliberately rather than to make it pass. +// +// FLIPPING A FLAG ALSO RE-ENABLES CODE THAT IS CURRENTLY UNREACHABLE, and it +// carries no test coverage today precisely because it cannot be entered. Restore +// coverage for all three when you flip: +// +// - the core.Discover call in dispatchModernSubscriptionsListen (the ceiling +// pre-check skips it entirely while nothing can be honored), AND +// - that call's error path, which maps a fan-out failure to -32603. Its test was +// removed rather than left asserting an unenterable branch. +// - the handler's non-empty-honored branch, including its WARN. func TestNewModernCapabilities_AdvertisesNoPushCapability(t *testing.T) { t.Parallel()