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.go b/pkg/vmcp/server/modern_dispatch.go index 71075c2fa2..72860edae4 100644 --- a/pkg/vmcp/server/modern_dispatch.go +++ b/pkg/vmcp/server/modern_dispatch.go @@ -97,14 +97,36 @@ 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 - // _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") @@ -112,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, ) { @@ -133,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 @@ -149,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( @@ -160,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( @@ -171,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 @@ -433,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_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 53a7e1b693..7fae168b96 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 @@ -229,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 @@ -255,6 +248,56 @@ 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. +// +// 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 +// 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 +305,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,18 +316,18 @@ 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 // admission-filtered domain tools. -func newModernToolsList(tools []vmcp.Tool, serverName, serverVersion string) (modernToolsListResult, error) { +// 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, serverName, serverVersion, nextCursor string, +) (modernToolsListResult, error) { wireTools := make([]mcp.Tool, 0, len(tools)) for _, t := range tools { wireTool, err := modernToolFromDomain(t) @@ -302,13 +340,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, serverName, serverVersion, nextCursor string, +) modernResourcesListResult { wireResources := make([]mcp.Resource, 0, len(resources)) for _, r := range resources { wireResources = append(wireResources, modernResourceFromDomain(r)) @@ -317,6 +358,7 @@ func newModernResourcesList(resources []vmcp.Resource, serverName, serverVersion ResultType: modernResultTypeComplete, modernCacheable: newModernCacheable(), Resources: wireResources, + NextCursor: nextCursor, Meta: newModernMeta(serverName, serverVersion), } } @@ -324,7 +366,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, serverName, serverVersion, nextCursor string, ) modernResourceTemplatesListResult { wireTemplates := make([]mcp.ResourceTemplate, 0, len(templates)) for _, t := range templates { @@ -334,13 +376,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, serverName, serverVersion, nextCursor string, +) modernPromptsListResult { wirePrompts := make([]mcp.Prompt, 0, len(prompts)) for _, p := range prompts { wirePrompts = append(wirePrompts, modernPromptFromDomain(p)) @@ -349,6 +394,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..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 new file mode 100644 index 0000000000..1259360ed5 --- /dev/null +++ b/pkg/vmcp/server/modern_pagination.go @@ -0,0 +1,367 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/base64" + "encoding/json" + "errors" + "fmt" + "slices" + "strings" +) + +// 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). 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 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. +// +// 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 +// 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 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: 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 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; 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 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, 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 "", 0, errInvalidModernCursor + } + var decoded modernCursor + if err := json.Unmarshal(raw, &decoded); err != nil { + return "", 0, errInvalidModernCursor + } + // A syntactically valid decode is not enough: the kind must match the verb + // 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, 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). +// +// 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 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.SortStableFunc(sorted, func(a, b T) int { + return strings.Compare(keyOf(a), keyOf(b)) + }) + + start := 0 + carried := 0 + cursorKey := "" + if cursor != "" { + lastKey, delivered, err := decodeModernCursor(kind, cursor) + if err != nil { + return nil, "", err + } + 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 + // "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++ + } + } + + 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 -- 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 + } + + // 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 + } + return page, next, nil +} + +// modernRequestCursor extracts the optional cursor from a list request's params. +// +// 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 "", nil + } + var raw struct { + Cursor json.RawMessage `json:"cursor"` + } + 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 + } + 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 new file mode 100644 index 0000000000..b53f42d349 --- /dev/null +++ b/pkg/vmcp/server/modern_pagination_test.go @@ -0,0 +1,725 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "encoding/base64" + "encoding/json" + "fmt" + "math/rand/v2" + "net/http" + "slices" + "strings" + "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 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 { + keys = append(keys, fmt.Sprintf("k%05d", i)) + } + 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 +// failure rather than an infinite loop. +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{} + 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_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 { + 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") + // 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)) + for _, k := range seen { + distinct[k] = struct{}{} + } + assert.Len(t, distinct, tt.total, "duplicates indicate overlapping pages") + }) + } +} + +// 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. +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", 1) + require.NoError(t, err) + 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 + }{ + {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 { + 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", 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_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", 3) + require.NoError(t, err) + + gotKey, gotDelivered, err := decodeModernCursor(cursorKindResources, encoded) + require.NoError(t, err) + 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" and the shapes that must error. +func TestModernRequestCursor(t *testing.T) { + t.Parallel() + + tests := []struct { + 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: "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() + 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) + }) + } +} + +// 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) + 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: "same-name"}) + templates = append(templates, vmcp.ResourceTemplate{URITemplate: "file:///{x}/" + id, Name: "same-name"}) + prompts = append(prompts, vmcp.Prompt{Name: id}) + } + 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 + 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() + fakeCore := paginationFakeCore(total) + + // 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") + }) + } + + // 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"} + + // 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(`{}`), + }) + 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() + + 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 new file mode 100644 index 0000000000..cab8d61117 --- /dev/null +++ b/pkg/vmcp/server/modern_subscriptions.go @@ -0,0 +1,378 @@ +// 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. +// +// 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" + 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. + // + // 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) + } + + // 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} + + // 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 + } + + 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. + // + // 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, + "tools_list_changed", honored.ToolsListChanged, + "prompts_list_changed", honored.PromptsListChanged, + "resources_list_changed", honored.ResourcesListChanged, + "resource_subscription_count", len(honored.ResourceSubscriptions)) + } +} + +// 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 +} + +// buildModernListenFrames marshals the two frames the listen response carries: +// the mandatory initial acknowledgement notification, then the terminating +// result. +// +// 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, + "params": subscriptionsAcknowledgedParams{ + Notifications: honored, + Meta: subscriptionMeta, + }, + }) + if err != nil { + return nil, 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 nil, fmt.Errorf("marshalling subscriptions listen result: %w", err) + } + return [][]byte{ack, result}, nil +} + +// 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") + // 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 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) + } + // 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 new file mode 100644 index 0000000000..36513551d8 --- /dev/null +++ b/pkg/vmcp/server/modern_subscriptions_test.go @@ -0,0 +1,464 @@ +// SPDX-FileCopyrightText: Copyright 2026 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package server + +import ( + "context" + "encoding/json" + "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{}, + }, + { + // 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}, + }, + { + 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, + }) + + // 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, parseSSEStrict(t, rec.Body.String()) +} + +// 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() + + // 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 + 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, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + 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"), + "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"]) + }) + } +} + +// 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") + + 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. +// +// 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() + + 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") +}