Skip to content

Commit 4de3082

Browse files
committed
Make Modern pagination collision-safe and pin the wire
Review found the paginator asserting a precondition that holds for one of its four callers. Only Tool.Name is unique -- ResolveToolConflicts keys a map -- while resources, resource templates and prompts are plain-appended across backends under "no conflict resolution for these yet". With a bare "resume strictly above lastKey" scan, a key shared at a page boundary made the scan skip every copy, so an item appeared on no page at all. Two honest backends both exposing file:///README.md was enough. A 1100-item corpus delivered 1097. The cursor now carries how many items sharing the last key were already sent, so a page can resume inside a run of equal keys. Items sharing a key are interchangeable for that purpose, which keeps the walk correct without a secondary sort key the generic signature cannot see. The comment claiming uniqueness is corrected rather than merged: it now states which callers can collide and that the paginator does not rely on uniqueness. The test corpus was the reason this shipped. Every case drew from a helper minting strictly unique keys, so the one precondition the code called load-bearing was the only thing untested. Collisions must also be placed by SORTED position -- a distinctively named duplicate key sorts away from the boundary and tests nothing, which a first attempt at the fixture did. Cap an inbound cursor before decoding it. A well-formed 6MB cursor decoded successfully at ~295ms of CPU on a verb that is not rate-limited, and a legitimate cursor is tens of bytes. Skip the capability fan-out in subscriptions/listen when nothing could be honored. core.Discover is un-rate-limited and, while no push capability is advertised, its result cannot change the answer -- so N no-op listens cost N fan-outs, now worse since list verbs page. Probing the ceiling first makes that O(1). This renders the fan-out error path unreachable, so its test is removed rather than left asserting a branch the code cannot enter. Assert the SSE framing. Replacing the blank-line event terminator with a single newline previously left the whole suite green -- including against a real client -- while a conformant parser would dispatch neither frame, so the ack-then-result contract was unverifiable. The helper now parses strictly, and the method name, acknowledgement name, subscriptionId key and jsonrpc field are asserted as literals; mutating any of them was silent. Add the negative capability test. The design rests on the advertisement honoring nothing, which was enforced by no test at all -- only by a runtime WARN that fires in production after the offending line ships. Now asserted across all sixteen presence combinations. Use http.ResponseController rather than a Flusher assertion: every ResponseWriter wrapper in the chain defines Flush unconditionally, so the assertion only established that a wrapper was present. Correct the comment that claimed otherwise, and the one claiming headers were committed on all four failure paths when three precede WriteHeader. Log the honored set by count, not by client-supplied URIs. Update docs/arch: subscriptions/listen is no longer unimplemented, serverStreamRegistry is the seam for delivery rather than for the handler, and both the pagination behaviour and the tools-only uniqueness property are now recorded. Refs #5959, #6033, #5743
1 parent abfb381 commit 4de3082

9 files changed

Lines changed: 1221 additions & 283 deletions

docs/arch/03-transport-architecture.md

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -677,20 +677,31 @@ path for server->client messages.
677677
send-on-closed-channel panic if a concurrent broadcast/dispatch races
678678
eviction/shutdown.
679679

680-
**Forward-compatibility note**: the 2026-07-28 (Modern) MCP revision (see
681-
`mcp.MCPVersionModern` and `pkg/mcp/revision.go`) introduces
682-
`subscriptions/listen`, a POST method whose response stream stays open for
683-
server-pushed subscription events. `serverStreamRegistry` is a reasonable
684-
decoupled fan-out seam to build a future Modern handler for this method on top
685-
of, but it is **not** a drop-in reuse. Per the 2026-07-28 spec, real adaptation is
686-
required: `subscriptions/listen` is a long-lived POST, not a GET; Modern has no
687-
sessions, so streams must be keyed per-subscription-id rather than per session
688-
ID; delivery requires per-notification-type AND per-URI opt-in filtering, not
689-
blanket fan-out; an initial `notifications/subscriptions/acknowledged` must be
690-
sent; and deliveries must be tagged with `io.modelcontextprotocol/subscriptionId`.
691-
`dispatcher_streams.go` is intentionally kept transport-agnostic (no HTTP
692-
types) so that this adaptation, when it happens, does not also require
693-
rewriting the underlying fan-out primitives.
680+
**Relationship to Modern `subscriptions/listen`**: the 2026-07-28 (Modern)
681+
revision (see `mcp.MCPVersionModern` and `pkg/mcp/revision.go`) replaces the
682+
standalone GET with `subscriptions/listen`, a POST method whose response stream
683+
stays open for server-pushed subscription events.
684+
685+
vMCP now **serves** that method on its client edge
686+
(`pkg/vmcp/server/modern_subscriptions.go`), and deliberately does **not** use
687+
`serverStreamRegistry`. The reason is that the handler honors no subscriptions:
688+
the honored set is intersected against the capabilities `server/discover`
689+
advertises, every push flag there is false, so the acknowledged set is always
690+
empty and the stream terminates immediately. With nothing to fan out, wiring in a
691+
fan-out registry from another package would add an abstraction with zero
692+
consumers. What the handler does implement is the wire contract — a long-lived
693+
POST rather than a GET, keyed per-subscription-id (the listen request's own
694+
JSON-RPC id) rather than per session ID since Modern has no sessions, an initial
695+
`notifications/subscriptions/acknowledged`, and
696+
`io.modelcontextprotocol/subscriptionId` tagging.
697+
698+
`serverStreamRegistry` remains the sensible seam for the piece still missing:
699+
actual **delivery** of notifications on a Modern listen stream (#5743). That work
700+
needs per-notification-type AND per-URI opt-in filtering rather than blanket
701+
fan-out, and per `schema/draft/schema.ts` must tag *every* notification with the
702+
subscription id, not just the acknowledgement. `dispatcher_streams.go` is
703+
therefore still intentionally kept transport-agnostic (no HTTP types), so that
704+
when delivery lands it does not also require rewriting the fan-out primitives.
694705

695706
## Error Handling
696707

docs/arch/10-virtual-mcp-architecture.md

Lines changed: 64 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,19 @@ When backends expose tools with the same name, vMCP resolves the conflict using
136136
| **priority** | First backend in priority order wins, others hidden |
137137
| **manual** | Explicit mapping for each conflict |
138138

139+
**Conflict resolution covers tools only.** Resources, resource templates, and
140+
prompts are concatenated across backends with no de-duplication
141+
(`default_aggregator.go`, "no conflict resolution for these yet"), so
142+
`Resource.URI`, `ResourceTemplate.URITemplate` and `Prompt.Name` can each repeat
143+
in the aggregated view — two backends both exposing `file:///README.md` is
144+
enough. Only `Tool.Name` is unique, because tool resolution keys a map.
145+
146+
Anything that treats one of those fields as an identity must account for that.
147+
The Modern list paginator does: its cursor names a position within a run of equal
148+
keys rather than assuming keys are distinct, because a plain "resume after this
149+
key" scan silently dropped an item whenever a duplicate landed on a page boundary
150+
(see [Client-facing list pagination](#client-facing-list-pagination)).
151+
139152
### Tool Filtering
140153

141154
Beyond conflict resolution, vMCP can filter which tools are exposed through allow/deny lists, renaming, and description overrides.
@@ -300,14 +313,53 @@ Beyond tools, vMCP aggregates and serves the full complement of MCP capabilities
300313
| 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) |
301314
| 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 |
302315
| 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 |
303-
| 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 |
316+
| 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 |
317+
| 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 |
318+
319+
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).
304320

305321
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.
306322

307323
### Subscription limitation (ack-level)
308324

309325
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.
310326

327+
### Client-facing list pagination
328+
329+
On the Legacy edge the SDK's session-scoped feature store splits list results into
330+
pages. `dispatchModern` bypasses the SDK, so the Modern edge paginates itself
331+
(`pkg/vmcp/server/modern_pagination.go`).
332+
333+
Modern has no sessions, so a cursor may not denote server-held iteration state.
334+
The draft pagination page makes cursors **opaque to clients** ("MUST treat cursors
335+
as opaque tokens"), which is precisely what allows the server to encode position
336+
*into* the token instead of remembering it. So the cursor is self-describing:
337+
base64url over a small JSON payload naming the list kind, the last key delivered,
338+
and how many items sharing that key have already been sent.
339+
340+
Three properties worth knowing:
341+
342+
- **Ordering.** Keyset paging needs a deterministic total order, and the
343+
aggregator's fan-out order is not stable between calls, so Modern list results
344+
are sorted by the item's key (`Tool.Name`, `Resource.URI`,
345+
`ResourceTemplate.URITemplate`, `Prompt.Name`). Legacy ordering is unchanged.
346+
- **Duplicate keys are tolerated, not assumed away.** Only tool names are unique
347+
(see [Conflict Resolution](#conflict-resolution)); resource URIs, template
348+
strings and prompt names can repeat. The cursor therefore resumes *within* a run
349+
of equal keys, because a plain "resume after this key" scan skipped every copy
350+
and permanently dropped items whose key collided at a page boundary.
351+
- **End of results omits `nextCursor` entirely.** The draft states that "an empty
352+
string is a valid cursor and thus MUST NOT be treated as the end of results", so
353+
emitting `""` would make a conformant client re-request and loop on page one.
354+
355+
The cursor encodes a position in the **aggregated** ordering and never names a
356+
backend, so adding or removing a backend cannot invalidate one. This is unrelated
357+
to the aggregator's *upstream* cursor-following (#5851), which is vMCP acting as a
358+
client walking a backend's pages.
359+
360+
An invalid cursor — malformed, over-length, or minted for a different list verb —
361+
is rejected with `-32602`, per the draft's error-handling rule.
362+
311363
### Tools/resources/prompts list_changed propagation (#5748, #5969)
312364

313365
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
338390
`initialize` and `Mcp-Session-Id`, so there is no Legacy-shaped persistent
339391
connection to hold and no standalone GET stream a Modern backend could push
340392
on. Modern's own server-push mechanism is `subscriptions/listen` (see
341-
[Transport Architecture](03-transport-architecture.md)), which vMCP does not
342-
implement yet — so `list_changed` propagation is currently Legacy-only.
393+
[Transport Architecture](03-transport-architecture.md)). vMCP **serves** that
394+
method on its client edge (`pkg/vmcp/server/modern_subscriptions.go`), but only
395+
at acknowledgement level: it computes the honored subscription set by
396+
intersecting the client's request against the capabilities `server/discover`
397+
advertises, and since every push-related flag there is deliberately false
398+
(`newModernCapabilities`), the honored set is always empty and no notification is
399+
ever pushed. So `list_changed` **delivery** remains Legacy-only, on both edges —
400+
what the Modern client edge gained is a conformant, explicitly-empty answer
401+
instead of a `-32601` that tore the client's connection down. Real Modern
402+
delivery is tracked in #5743 and requires vMCP to start advertising a push
403+
capability first.
343404

344405
The sink is built once per session, at registration (`pkg/vmcp/server`'s
345406
`buildListChangedSink`), closing over the SDK `ClientSession`, the session ID,

pkg/vmcp/server/modern_dispatch_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,13 @@ type modernFakeCore struct {
3838

3939
discoverCaps core.DiscoverCapabilities
4040
discoverErr error
41+
// discoverCalls counts Discover invocations, so a test can assert the
42+
// subscriptions/listen pre-check skipped the backend fan-out entirely (the
43+
// response is identical either way, so only the count is observable).
44+
discoverCalls int
45+
// discoverIdentity records the identity Discover was called with, so
46+
// per-identity filtering of a computed set can be pinned rather than assumed.
47+
discoverIdentity *auth.Identity
4148

4249
checkToolErr, checkResourceErr, checkPromptErr error
4350
callToolErr, readResourceErr, getPromptErr error
@@ -73,7 +80,9 @@ func (f *modernFakeCore) ListPrompts(context.Context, *auth.Identity) ([]vmcp.Pr
7380
return f.prompts, nil
7481
}
7582

76-
func (f *modernFakeCore) Discover(context.Context, *auth.Identity) (core.DiscoverCapabilities, error) {
83+
func (f *modernFakeCore) Discover(_ context.Context, identity *auth.Identity) (core.DiscoverCapabilities, error) {
84+
f.discoverCalls++
85+
f.discoverIdentity = identity
7786
return f.discoverCaps, f.discoverErr
7887
}
7988

pkg/vmcp/server/modern_envelope.go

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -233,21 +233,10 @@ type modernDiscoverResult struct {
233233
Meta modernMeta `json:"_meta"`
234234
}
235235

236-
// newModernDiscover builds the server/discover wire result. hasTools/
237-
// hasResources/hasPrompts are the caller's len(core.List*(ctx, identity))>0
238-
// admission-filtered presence checks -- this function only shapes them into
239-
// the wire capability flags, so a discover response reflects exclusively
240-
// what this identity may reach, same as the four list results. hasTemplates
241-
// folds into the "resources" capability: the wire protocol has no separate
242-
// resource-templates capability flag.
243-
//
244-
// Completions is set unconditionally, unlike the three identity-filtered
245-
// flags above: completion/complete has no per-identity admission of its own
246-
// (core.Complete authorizes the underlying prompt/resource ref at dispatch,
247-
// not the completions feature itself), and this dispatcher serves it for
248-
// every caller once Modern dispatch is enabled at all -- matching how the
249-
// Legacy/SDK path always wires server.WithCompletionHandler regardless of
250-
// identity (serve.go).
236+
// newModernDiscover builds the server/discover wire result. The capability
237+
// flags themselves are shaped by newModernCapabilities below -- see its doc
238+
// comment for what each flag means and why the push-related ones are false.
239+
// This function only assembles the envelope around them.
251240
//
252241
// SupportedVersions enumerates every protocol version this vMCP endpoint
253242
// actually serves, not just Modern: mcpparser.MCPVersionModern (this
@@ -269,8 +258,22 @@ func newModernDiscover(
269258
}
270259

271260
// newModernCapabilities shapes the four admission-filtered presence checks into
272-
// the wire capability flags. It is the SINGLE source of truth for what this
273-
// dispatcher advertises: server/discover publishes it verbatim (above), and
261+
// the wire capability flags.
262+
//
263+
// hasTools/hasResources/hasTemplates/hasPrompts are the caller's
264+
// len(core.List*(ctx, identity))>0 checks, so an advertisement reflects
265+
// exclusively what this identity may reach, same as the four list results.
266+
// hasTemplates folds into the "resources" capability: the wire protocol has no
267+
// separate resource-templates flag.
268+
//
269+
// Completions is set unconditionally, unlike the identity-filtered flags:
270+
// completion/complete has no per-identity admission of its own (core.Complete
271+
// authorizes the underlying prompt/resource ref at dispatch, not the completions
272+
// feature itself), and this dispatcher serves it for every caller -- matching how
273+
// the Legacy/SDK path always wires server.WithCompletionHandler regardless of
274+
// identity (serve.go).
275+
//
276+
// It is the SINGLE source of truth for what this dispatcher advertises: server/discover publishes it verbatim (above), and
274277
// subscriptions/listen intersects a client's requested notification set against
275278
// it (honoredSubscriptions, modern_subscriptions.go). Keeping one builder is
276279
// what makes those two answers impossible to drift apart -- a client can never
@@ -318,8 +321,12 @@ func newModernCapabilities(hasTools, hasResources, hasTemplates, hasPrompts bool
318321

319322
// newModernToolsList builds the tools/list wire result from the core's
320323
// admission-filtered domain tools.
324+
// nextCursor is LAST on purpose: it sits among same-typed string parameters, and
325+
// when it was in the middle a transposition with serverName compiled silently and
326+
// put a server name in the cursor field. Keep new string params appended, not
327+
// inserted.
321328
func newModernToolsList(
322-
tools []vmcp.Tool, nextCursor, serverName, serverVersion string,
329+
tools []vmcp.Tool, serverName, serverVersion, nextCursor string,
323330
) (modernToolsListResult, error) {
324331
wireTools := make([]mcp.Tool, 0, len(tools))
325332
for _, t := range tools {
@@ -341,7 +348,7 @@ func newModernToolsList(
341348
// newModernResourcesList builds the resources/list wire result from the
342349
// core's admission-filtered domain resources.
343350
func newModernResourcesList(
344-
resources []vmcp.Resource, nextCursor, serverName, serverVersion string,
351+
resources []vmcp.Resource, serverName, serverVersion, nextCursor string,
345352
) modernResourcesListResult {
346353
wireResources := make([]mcp.Resource, 0, len(resources))
347354
for _, r := range resources {
@@ -359,7 +366,7 @@ func newModernResourcesList(
359366
// newModernResourceTemplatesList builds the resources/templates/list wire
360367
// result from the core's admission-filtered domain resource templates.
361368
func newModernResourceTemplatesList(
362-
templates []vmcp.ResourceTemplate, nextCursor, serverName, serverVersion string,
369+
templates []vmcp.ResourceTemplate, serverName, serverVersion, nextCursor string,
363370
) modernResourceTemplatesListResult {
364371
wireTemplates := make([]mcp.ResourceTemplate, 0, len(templates))
365372
for _, t := range templates {
@@ -377,7 +384,7 @@ func newModernResourceTemplatesList(
377384
// newModernPromptsList builds the prompts/list wire result from the core's
378385
// admission-filtered domain prompts.
379386
func newModernPromptsList(
380-
prompts []vmcp.Prompt, nextCursor, serverName, serverVersion string,
387+
prompts []vmcp.Prompt, serverName, serverVersion, nextCursor string,
381388
) modernPromptsListResult {
382389
wirePrompts := make([]mcp.Prompt, 0, len(prompts))
383390
for _, p := range prompts {

pkg/vmcp/server/modern_envelope_test.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) {
4343
Name: "greet",
4444
Description: "say hi",
4545
InputSchema: map[string]any{"type": "object"},
46-
}}, "", testServerName, testServerVersion)
46+
}}, testServerName, testServerVersion, "")
4747
require.NoError(t, err)
4848
return result
4949
},
@@ -54,7 +54,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) {
5454
build: func(*testing.T) any {
5555
return newModernResourcesList([]vmcp.Resource{
5656
{Name: "info", URI: "embedded:info", MimeType: "text/plain"},
57-
}, "", testServerName, testServerVersion)
57+
}, testServerName, testServerVersion, "")
5858
},
5959
wantCacheable: true,
6060
},
@@ -63,7 +63,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) {
6363
build: func(*testing.T) any {
6464
return newModernResourceTemplatesList([]vmcp.ResourceTemplate{
6565
{Name: "logs", URITemplate: "file:///logs/{date}.txt"},
66-
}, "", testServerName, testServerVersion)
66+
}, testServerName, testServerVersion, "")
6767
},
6868
wantCacheable: true,
6969
},
@@ -72,7 +72,7 @@ func TestModernEnvelopeCommonFields(t *testing.T) {
7272
build: func(*testing.T) any {
7373
return newModernPromptsList([]vmcp.Prompt{
7474
{Name: "code_review", Arguments: []vmcp.PromptArgument{{Name: "Code", Required: true}}},
75-
}, "", testServerName, testServerVersion)
75+
}, testServerName, testServerVersion, "")
7676
},
7777
wantCacheable: true,
7878
},
@@ -328,27 +328,27 @@ func TestModernEnvelopeEmptyCollections(t *testing.T) {
328328
field: "tools",
329329
build: func(t *testing.T) any {
330330
t.Helper()
331-
result, err := newModernToolsList(nil, "", testServerName, testServerVersion)
331+
result, err := newModernToolsList(nil, testServerName, testServerVersion, "")
332332
require.NoError(t, err)
333333
return result
334334
},
335335
},
336336
{
337337
name: "resources/list",
338338
field: "resources",
339-
build: func(*testing.T) any { return newModernResourcesList(nil, "", testServerName, testServerVersion) },
339+
build: func(*testing.T) any { return newModernResourcesList(nil, testServerName, testServerVersion, "") },
340340
},
341341
{
342342
name: "resources/templates/list",
343343
field: "resourceTemplates",
344344
build: func(*testing.T) any {
345-
return newModernResourceTemplatesList(nil, "", testServerName, testServerVersion)
345+
return newModernResourceTemplatesList(nil, testServerName, testServerVersion, "")
346346
},
347347
},
348348
{
349349
name: "prompts/list",
350350
field: "prompts",
351-
build: func(*testing.T) any { return newModernPromptsList(nil, "", testServerName, testServerVersion) },
351+
build: func(*testing.T) any { return newModernPromptsList(nil, testServerName, testServerVersion, "") },
352352
},
353353
{
354354
name: "tools/call content",

0 commit comments

Comments
 (0)