Complete Modern client-facing dispatch: listen + pagination - #6050
Complete Modern client-facing dispatch: listen + pagination#6050JAORMX wants to merge 7 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6050 +/- ##
==========================================
+ Coverage 72.20% 72.22% +0.02%
==========================================
Files 721 723 +2
Lines 75062 75277 +215
==========================================
+ Hits 54198 54370 +172
- Misses 17003 17008 +5
- Partials 3861 3899 +38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
JAORMX
changed the title
github-actions
Bot
added
size/XL
MCP 2026-07-28 removed the standalone HTTP GET stream, so subscriptions/ listen is the revision's only server->client push channel. vMCP's Modern dispatcher had no case for it, which made vMCP unreachable for any go-sdk v1.7 client: Connect is Modern-first, and once server/discover succeeds it opens a listen stream whenever a list-changed handler is registered -- which mcpcompat's Initialize does unconditionally. The resulting -32601 failed Connect outright and tore the session down. The honored set is computed by intersecting the client's requested notification types against the capability advertisement server/discover publishes, now extracted as newModernCapabilities so the two answers cannot drift. Every push-related flag there is deliberately false, so the honored set is empty today and the acknowledgement says so explicitly. That is the spec's own mechanism for declaring unsupported notification types, not a silent no-op: nothing is ever reported as subscribed and then dropped. No push delivery exists yet (#5743); vMCP must start advertising a push capability before this stream can carry anything. Measured against the kill-switch removal in #6033: 20 regressions before, 6 after. The remaining 6 are unrelated to this channel -- five need server-initiated elicitation/sampling that 2026-07-28 removes outright, and one needs Modern list pagination. Refs #5959, #6033, #5743
dispatchModern's four list verbs returned the full aggregated set and never emitted nextCursor, so a client had no way to page and vMCP silently ignored any cursor it was sent. On the Legacy path the SDK's session-scoped feature store does this split; the Modern dispatcher bypasses the SDK, so it has to do it itself. Modern has no sessions, so a cursor may not denote server-held iteration state. The spec makes cursors opaque to clients -- they MUST NOT parse, modify, or persist them -- which is exactly what lets the server encode position INTO the token instead of remembering it. So the cursor carries the unique key of the last item delivered and the next page is the items sorted strictly above it: keyset pagination, stateless, and the same scheme go-sdk's own server uses. Aggregation across backends needs no per-backend positions. core.List* already returns one flat, conflict-resolved set whose keys are unique across backends, so the cursor encodes a position in the aggregated ordering and adding or removing a backend cannot invalidate it. Page size matches the SDK default (1000) so pages line up across revisions, and an undecodable or wrong-verb cursor is rejected as -32602 rather than being reinterpreted into a plausible but wrong page. The upstream cursor-following in pkg/vmcp/client and the session backend (#5851) is the opposite direction -- vMCP as client -- and is untouched. Refs #5959, #6033
8a8108d to
7046faa
Compare
github-actions
Bot
added
size/XL
jhrozek
left a comment
jhrozek
left a comment
There was a problem hiding this comment.
Went through this in detail (protocol-compliance, Go quality, architecture/docs, and test coverage) plus checked it against the vmcp anti-pattern list separately. Overall this is solid -- no correctness or spec issues that need to hold up merging. Left a handful of inline notes below, plus two things that don't fit as inline comments because they're not touched by this diff:
docs/arch/10-virtual-mcp-architecture.md(~line 340-342) still says vMCP "does not implement"subscriptions/listenyet -- worth a follow-up correcting that to describe the ack-only, zero-capability handler this PR adds (thelist_changed-is-Legacy-only conclusion there is still accurate, just the premise needs a tweak).docs/arch/03-transport-architecture.md's "Forward-compatibility note" (~line 680-689) still frames a Modernsubscriptions/listenhandler as future work and floatsserverStreamRegistryas the fan-out seam for it. Might be worth a short update now that the handler exists and deliberately doesn't reuse that registry (empty honored-set today, no fan-out consumer yet).
Neither blocks this PR, just flagging so they don't get lost.
|
|
||
| if err := writeModernListenStream(w, parsed.ID, honored, subscriptionMeta); err != nil { | ||
| // The client hung up or the stream could not be flushed. Nothing is | ||
| // recoverable at this point (headers are already committed), so log and |
There was a problem hiding this comment.
This comment isn't quite right for all the error cases writeModernListenStream can return. The fmt.Fprintf failure mid-loop is genuinely unrecoverable since headers are already committed by then -- but the two marshal errors and the Flusher type-assertion failure all happen before w.WriteHeader is called, so those could still go out as a proper -32603 envelope instead of silently returning an empty 200. writeModernEnvelope next door already does this (marshal-failure fallback) -- might be worth splitting the pre-write checks out of writeModernListenStream so their failures route through writeModernListError here, and reserving this log-and-return path for the actual mid-stream write failure.
There was a problem hiding this comment.
Applied, and your design suggestion was better than the comment correction I'd planned — thank you.
buildModernListenFrames is split out, and both marshal failures now route through writeModernListError as a proper -32603, matching writeModernEnvelope's fallback next door. Log-and-return is reserved for the genuine mid-stream fmt.Fprintf/Flush failure, which is the only case where headers really are committed.
One part of your ask I could not do, and I'd rather name it than let this read as fully done: the writer-capability check is now gone rather than routed. http.ResponseController exposes no flushability predicate — the only way to find out is to call Flush, which commits the header. So an unflushable writer surfaces mid-stream instead of as a pre-write -32603.
I judged that the better trade, because the old http.Flusher type assertion answered early but wrongly: every wrapper in the chain (audit/auditor.go, telemetry/middleware.go, bodylimit/middleware.go, mcp/tool_filter.go) defines Flush() unconditionally and forwards only if the inner writer supports it, so the assertion only ever proved a wrapper was present. It never actually detected unflushability. The tradeoff is documented at the call site with a note to move the check back ahead of WriteHeader if Go ever grows a predicate.
|
|
||
| w.Header().Set("Content-Type", "text/event-stream") | ||
| w.Header().Set("Cache-Control", "no-cache, no-transform") | ||
| w.Header().Set("Connection", "keep-alive") |
There was a problem hiding this comment.
Missing X-Accel-Buffering: no here -- the draft SSE transport spec recommends it on every SSE response so a proxy in front doesn't buffer the stream. This is the only SSE-emitting path in the whole codebase, so probably worth adding.
| w.Header().Set("Connection", "keep-alive") | |
| w.Header().Set("Connection", "keep-alive") | |
| w.Header().Set("X-Accel-Buffering", "no") |
| // above it rather than failing, which is the graceful degradation the | ||
| // spec's "stable cursors" guidance is about. | ||
| start = len(sorted) | ||
| for i, item := range sorted { |
There was a problem hiding this comment.
Minor, non-blocking: since sorted is already sorted by this same comparator, this could be sort.Search for O(log n) instead of scanning linearly for the resume point. Not urgent given the dominant cost per page is already the O(n log n) sort/clone above it, but free to pick up if you're touching this again.
| t.Parallel() | ||
|
|
||
| rec, body := dispatchModernTest(t.Context(), t, fakeCore, false, &mcpparser.ParsedMCPRequest{ | ||
| ID: 3, Method: "tools/list", Params: json.RawMessage(`{"cursor":"!!!bogus!!!"}`), |
There was a problem hiding this comment.
This covers a syntactically-bogus cursor through the actual dispatch path, which is good, but the PR description's "-32602 for wrong-verb cursors" claim is otherwise only exercised one layer down, directly against paginateModern (the "cursor minted for a different list verb" case above). Might be worth adding one case here too -- take the page-1 cursor tools/list returns and submit it to resources/list, assert -32602 -- so the end-to-end guarantee is actually pinned by a test at this level, not just the codec.
| // cmpString orders two keys. Spelled out rather than using strings.Compare so | ||
| // the comparison here provably matches the `>` used in paginateModern's scan -- | ||
| // the two must agree, or a page boundary could skip an item. | ||
| func cmpString(a, b string) int { |
There was a problem hiding this comment.
Nit: this duplicates exactly what cmp.Compare[string] returns. The comment's reasoning (spelled out so it "provably" matches the > scan) doesn't really buy anything extra since cmp.Compare is defined to agree with </> already. Could just be return cmp.Compare(a, b) (needs "cmp" added to imports) -- optional cleanup, not blocking.
| var raw struct { | ||
| Cursor string `json:"cursor"` | ||
| } | ||
| if err := json.Unmarshal(params, &raw); err != nil { |
There was a problem hiding this comment.
Small asymmetry worth a look: a malformed cursor string correctly surfaces as -32602 via decodeModernCursor, but a malformed params envelope here just silently falls back to "first page" instead of also erroring. The comment says this is deliberate since a bad params object is already rejected upstream -- if that's reliably true for every caller of this function, fine as-is, but if not, mapping the unmarshal error to -32602 too would make the two failure modes consistent.
Spec-conformance review found the pagination comment citing the 2025-11-25 page for a Modern implementation. The draft page is what 2026-07-28 ships and it differs in one substantive way: "Don't persist cursors across sessions" is 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)". Cite the draft page as the rule and SEP-2567 as the reason for the deletion, by path rather than line since that SEP exists in two copies with different numbering. That amendment makes the existing omitempty on nextCursor load-bearing rather than incidental: emitting an empty cursor would have a conformant client re-request with it, which reads as "first page", looping forever on page one. Pinned explicitly now. Reject a cursor that is present but not a JSON string instead of silently reading it as "first page". The draft requires invalid cursors to yield -32602 and schema.ts classifies cursor values under invalid params, so a client with a serialization bug was getting a silent full-list restart in place of a diagnosable error. An empty-string cursor still means "first page", matching go-sdk, since vMCP never mints one. Stop attributing the subscriptionId key, request-id-as-identity, and acknowledgement ordering to the go-sdk: all three are normative in schema/draft/schema.ts. The comment as written invited a maintainer to weaken a MUST. Also restate the delivery-side MUST -- every notification on a listen stream carries the key -- at the #5743 hand-off, since no code here delivers notifications and it would otherwise be lost. Correct the ping comment, which claimed spec support that does not exist: ping was removed in 2026-07-28 and the bare {} also omits the MUSTed resultType. Behavior left unchanged as out of scope; the conformant answer is noted for whoever tightens it. Rename TestPaginateModern_DeliversEveryItemExactlyOnce to say "static set": the exactly-once guarantee holds only while the set does not change mid-walk, which is inherent to keyset paging and out of scope per SEP-2567. Refs #5959, #6033
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
Fold in the remaining review findings, including one the panel could not reach and two fixture blind spots. Set X-Accel-Buffering: no on the SSE response. The draft Streamable HTTP page states servers SHOULD send it so a reverse proxy does not accumulate events before forwarding, and this is the only SSE-emitting path in the codebase. The panel missed it because the draft moved transports.mdx into a directory, so every path it tried 404'd; the page is now at docs/specification/draft/basic/transports/streamable-http.mdx. Checking it also settles two open questions: the SSE event name is NOT mandated anywhere, and resumability is explicitly unsupported, which is why no id: field is emitted. Split frame marshalling out of the stream writer. Both marshals could previously fail on a path documented as "headers already committed" when nothing had been written at all, so the client got a bare 200 with an empty body for a request carrying an id. Building first means those failures are still reportable as -32603, and only a genuine mid-stream write failure is unreportable. Shuffle the corpus before paging in drainPages. Deleting the sort from paginateModern entirely used to fail exactly one subtest -- the only one whose input was unsorted -- so the deterministic total order the whole keyset scheme depends on was effectively unverified. The aggregator's real output is unordered anyway, so a shuffled input is also the realistic one. The same mutation now fails four test functions. Cover the remaining duplicate-key shapes, whose loss profiles differ sharply: a single duplicate at the boundary lost one item, one mid-page lost none, a run of 50 lost 50, and an all-identical corpus lost a whole page and returned an empty second page. The rule is that every item sharing the last key delivered on a page was dropped, so loss scaled with the run straddling the boundary -- and a duplicate away from a boundary was harmless, which is what made it easy to miss. Binary-search the resume point instead of scanning, making a full walk O(n log n) rather than O(n^2). The insertion point also gives the since-removed-key behaviour for free. Refs #5959, #6033, #5743
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
Three comment corrections from review follow-up, no behaviour change. Name what flipping a push capability flag re-enables: the core.Discover call the ceiling pre-check currently skips, that call's error path whose test was removed rather than left asserting an unenterable branch, and the handler's non-empty-honored branch. All three are uncovered today because they cannot be entered, so whoever flips a flag needs the list. State the cost of moving to http.ResponseController: there is no longer a pre-write capability check, because Go exposes no flushability predicate and calling Flush commits the header. An unflushable writer now surfaces mid-stream instead of as -32603. Deliberate -- the old type assertion answered early but wrongly, since every ResponseWriter wrapper in the chain defines Flush unconditionally. Stop the page-size comment presuming how the Legacy twin will be pinned. Whether that is a pinning harness or a loud failure at negotiation is undecided, and naming one would let the comment go stale.
github-actions
Bot
added
size/XL
|
/retest |
|
@jhrozek all six of your inline notes are applied, plus both doc items from your review body. Consolidating here since the threads are marked outdated against the current head (
Two things your questions led to that are worth recording, because both closed gaps we could not otherwise verify: Your A blocking defect was found after your review and is fixed here, so it's worth flagging since you reviewed the earlier tree: the paginator assumed item keys were unique, citing the aggregator's conflict resolver. That holds for tools only — Happy to take anything else. Note the merge is currently blocked on these conversations being resolved. |
|
/retest |
|
@JAORMX I have a feeling the e2e tests failure is real as it's failed on 2 retries |
github-actions
Bot
added
size/XL
4 participants
Summary
Why. vMCP's Modern (2026-07-28) dispatcher had two client-facing gaps that together made it unusable for a real SDK client. Both are exposed the moment vMCP serves Modern unconditionally (#6033); neither was introduced by it.
Delivered as one larger PR per the CLAUDE.md large-PR exception — these are the two halves of "Modern client-facing dispatch works".
Refs #5959, #6033, #5743
Part 1 —
subscriptions/listenModern removed the standalone HTTP GET stream, making
subscriptions/listenthe revision's only server→client push channel.dispatchModernhad no case for it, so it answered-32601. That made vMCP unreachable for any go-sdk v1.7 client:Connectis Modern-first, and onceserver/discoversucceeds it opens a listen stream whenever a list-changed handler is registered — whichmcpcompat'sInitializedoes unconditionally. The-32601failedConnectoutright and tore the session down.server/discoverpublishes — extracted asnewModernCapabilitiesso the two cannot drift.notifications/subscriptions/acknowledgedfirst, then a terminating result, both taggedio.modelcontextprotocol/subscriptionIdand keyed by the listen request's own JSON-RPC id (Modern has no sessions).server/discover.No push delivery exists, and this PR does not add any. Every push flag in
newModernCapabilitiesis deliberately false —dispatchModerncreates no session, so the per-sessionlist_changedmachinery never runs for a Modern client, and backendresources/updatedis not forwarded even on Legacy. The honored set is therefore always empty, and the acknowledgement says so explicitly. That is the spec's own mechanism for declaring unsupported types, not a silent no-op: nothing is reported as subscribed and then quietly dropped. Real delivery is #5743.Part 2 —
nextCursorpagination for the list verbsThe four Modern list verbs returned the full aggregated set and never emitted
nextCursor, so a client could not page and any cursor it sent was ignored. On Legacy the SDK's session-scoped feature store does this split; the Modern dispatcher bypasses the SDK.Modern is stateless, so a cursor may not denote server-held iteration state. The draft pagination page (not the 2025-11-25 one — they differ) makes cursors opaque to clients, which is precisely what lets the server encode position into the token instead of remembering it.
Tool.Nameis unique (ResolveToolConflictskeys a map); resource URIs, template strings and prompt names are plain-appended across backends with no de-duplication, so collisions are ordinary.nextCursorentirely. The draft states an empty string is a valid cursor and MUST NOT be treated as the end of results, so emitting""would make a conformant client loop on page one.-32602.Ordering note: keyset paging needs a deterministic total order and the aggregator's fan-out order is not stable, so Modern list results are now sorted by key. Legacy ordering is unchanged.
Type of change
Test plan
task test) — only the three known environmental failures (TestNewServer_ReadTimeoutConfigured,TestSecurityHeaders,TestCompositeProvider_RealProviders), all pre-existing onmainand all passing in CI, which has a container runtime.task lint) — 0 issues.go vet ./...andgo build ./...run separately: clean.fd3b6a9b.Measured effect on #6033
test/integration/vmcppkg/vmcp/serverforwardingsubscriptions/listentest/integration/vmcpis fully green,TestRegression_Over1000Tools_CompleteSetReceivedincluded.The remaining 5 are the forwarding tests and are not addressable here: they exercise server-initiated elicitation/sampling, which 2026-07-28 removes outright — go-sdk's
assertServerInitiatedRequestAllowedrefuses them on protocol version alone, never consulting client capabilities. Progress and logging are not subscribable types under SEP-2575. Those need the MRTR work.Duplicate-key walk counts (H1)
API Compatibility
v1beta1API.No CRD or
api/v1*type is touched. Legacy behaviour is unchanged; every change is confined to the Modern dispatch path, except thenewModernCapabilitiesextraction, a pure refactor of whatserver/discoveralready emitted.Does this introduce a user-facing change?
Yes, reachable once vMCP serves Modern (still behind the kill-switch on
main, removed by #6033):Connectinstead of having its session torn down, and receives an explicit acknowledgement that no notification subscriptions are honored.Special notes for reviewers
Things that are correct and easy to mistake for bugs — please don't re-flag these; each was checked against the draft spec:
closeevent and noid:field. The draft explicitly drops resumability: "Resumable SSE streams viaLast-Event-IDare not supported", and instructs servers to ignore the header. Emittingid:would imply a resumability we don't offer.event: messagename is not mandated. The draft specifies the content type and stream lifecycle but never an SSE event name. It's kept for symmetry with go-sdk's framing.resultType: "complete"on the listen result is required byResultinschema/draft/schema.ts("Servers implementing this protocol version MUST include this field").{}for an empty honored set is the spec's way to say "I honor none of these", not an omission.notifications/cancelledwould fall todefault:→-32601→ 404 → dead session.pingreturns a bare{}.pingdoes not exist in 2026-07-28 (absent from the draft schema; go-sdk lists it among removed methods), so answering it at all is deliberate leniency, not conformance — and the bare{}also omitsresultType. Left as-is because it is a pre-existing path and inert (a go-sdk Modern client never pings); the comment now says-32601is the conformant answer for whoever tightens it.Where to push back:
callSubscriptionsAckHandlerreturns(nil, nil)), so against that client the declaration is not observable in behaviour. It is still correct on the wire, and the alternative — holding a stream open forever delivering nothing — is strictly worse.WARNfails loud where blocking fails silent. The branch is unreachable today, and theWARNexists so a future capability flip without delivery is loud in logs rather than an idle subscription.serverStreamRegistry.dispatcher_streams.gosuggests it as the fan-out seam. With an empty honored set there is no fan-out, so adapting an unexported registry from another package would add an abstraction with zero consumers (vMCP anti-pattern Start simple hardcoded registry #8). It stays the right seam for delivery; the doc now says so.modernPageSizeis a hand-copy of go-sdk'sDefaultPageSize. The Modern side is pinned behaviourally (1001 → 1000 + 1); the matching Legacy assertion is not, because pinning a client to Legacy needs a negotiation-pinning harness this package lacks (mcpcompat cannot request a protocol version, Handle per-backend MCP revision in vMCP (client-side dual protocol) #5911). Stated in the constant's doc comment as an invariant maintained by review, not CI. Follow-ups: re-export the constant from mcpcompat, and add the Legacy twin alongside the MRTR work.Known property introduced by pagination: backend-chosen names now influence page-1 membership. 1000 tools named
aaa_*will fill the first page and push others to page 2, where previously everything arrived at once. Only affects clients that ignorenextCursor; noting it because it did not exist before this change.Follow-up filed upstream: go-sdk's
ClientSession.SetLoggingLevelomitsinjectRequestMetawhile still sending theMCP-Protocol-Version: 2026-07-28header, so a conformant server must reject it-32020and a Modern go-sdk client cannot calllogging/setLevelat all — modelcontextprotocol/go-sdk#1116.Generated with Claude Code