From 509350f96e59777f7250306c6a553823de91785f Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 16:42:50 +0300 Subject: [PATCH 1/2] Pin tools/list pagination completeness for >1000-tool vMCP sets MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Part of #5742 (regression gate §5). mcp-go returned a vMCP's full aggregated tool set in one page; the go-sdk-backed server paginates server-side at DefaultPageSize=1000 and emits nextCursor. A downstream client that issues a single tools/list and ignores the cursor silently loses the tail — the regression the migration risked, and the matrix's only completely uncovered item. Stand up a Serve-path vMCP advertising 1500 tools and drive a real paginated tools/list, asserting: the server paginates (emits nextCursor, does not truncate), and cursor-following recovers the complete 1500-tool set with no duplicates or omissions. Mutation- verified: removing pagination fails the nextCursor assertion. --- pkg/vmcp/server/serve_session_test.go | 149 ++++++++++++++++++++++++++ 1 file changed, 149 insertions(+) diff --git a/pkg/vmcp/server/serve_session_test.go b/pkg/vmcp/server/serve_session_test.go index 820387642e..4bceeb646e 100644 --- a/pkg/vmcp/server/serve_session_test.go +++ b/pkg/vmcp/server/serve_session_test.go @@ -451,6 +451,155 @@ func TestServeRegistersSessionHooks(t *testing.T) { "tools/list must reuse the fixed-at-initialize set, not re-aggregate via the core") } +// TestServeToolsListPagination_CompleteSet pins #5742 §5: when vMCP's aggregated +// tool set exceeds go-sdk's DefaultPageSize (1000), a downstream client that +// follows the MCP pagination cursor receives the COMPLETE set — the server must +// paginate (emit nextCursor) rather than truncate, and every advertised tool +// must be reachable across the pages. +// +// go-sdk paginates server-side list responses at DefaultPageSize=1000 +// (mcp/server.go), emitting nextCursor. mcp-go (pre-migration) returned the +// whole set in one page, so a naive reader that issues a single tools/list and +// ignores nextCursor would silently drop the tail — this is the regression the +// migration risked. vMCP does NOT override the page size (no WithPageSize on +// the Serve path), so the served set IS paginated; the contract this pins is +// that cursor-following recovers all of it. +// +// The set is generated deterministically (tool-0000 … tool-1499) so the test +// asserts the exact membership, not just the count. +func TestServeToolsListPagination_CompleteSet(t *testing.T) { + t.Parallel() + + const totalTools = 1500 // > go-sdk DefaultPageSize (1000), so the server paginates + + ctrl := gomock.NewController(t) + tools := make([]vmcp.Tool, totalTools) + for i := range tools { + tools[i] = vmcp.Tool{Name: fmt.Sprintf("tool-%04d", i), Description: "pagination test tool"} + } + factory, state := newToolSessionFactory(t, ctrl, tools) + fc := &fakeCore{tools: tools} + + srv, err := Serve(context.Background(), fc, &ServerConfig{ + SessionTTL: time.Minute, + SessionManagerConfig: &sessionmanager.FactoryConfig{Base: factory}, + BackendRegistry: vmcp.NewImmutableRegistry([]vmcp.Backend{}), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = srv.Stop(context.Background()) }) + + streamable := server.NewStreamableHTTPServer( + srv.mcpServer, + server.WithEndpointPath("/mcp"), + server.WithSessionIdManager(srv.vmcpSessionMgr), + ) + ts := httptest.NewServer(streamable) + t.Cleanup(ts.Close) + + initResp := postServeMCP(t, ts.URL, map[string]any{ + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": map[string]any{ + "protocolVersion": "2025-06-18", + "capabilities": map[string]any{}, + "clientInfo": map[string]any{"name": "test", "version": "1.0"}, + }, + }, "") + defer initResp.Body.Close() + require.Equal(t, http.StatusOK, initResp.StatusCode) + sessionID := initResp.Header.Get("Mcp-Session-Id") + require.NotEmpty(t, sessionID) + require.Eventually(t, state.makeWithIDCalled.Load, 2*time.Second, 10*time.Millisecond) + + // Follow the pagination cursor, accumulating tool names until nextCursor is + // empty. A response is a JSON-RPC envelope (possibly SSE-framed), so decode + // the result payload rather than string-matching. + var ( + gotNames []string + cursor string + pages int + sawCursor bool + ) + for { + pages++ + listResp := postServeMCP(t, ts.URL, map[string]any{ + "jsonrpc": "2.0", + "id": pages + 1, + "method": "tools/list", + "params": map[string]any{"cursor": cursor}, + }, sessionID) + require.Equal(t, http.StatusOK, listResp.StatusCode, "tools/list page %d should succeed", pages) + + body, err := io.ReadAll(listResp.Body) + listResp.Body.Close() + require.NoError(t, err) + + names, next := decodeListToolsPage(t, body) + gotNames = append(gotNames, names...) + if next == "" { + break + } + sawCursor = true + cursor = next + require.LessOrEqual(t, pages, 10, "pagination must terminate (no infinite cursor)") + } + + assert.True(t, sawCursor, + "a >1000-tool set must paginate (emit nextCursor); a single-page response would mean the page size was raised or the set truncated") + assert.Greater(t, pages, 1, "a 1500-tool set at page size 1000 spans multiple pages") + + // The complete set must be reachable across the pages, with no duplicates and + // no omissions. + require.Len(t, gotNames, totalTools, + "cursor-following must recover every tool, not just the first page") + sort.Strings(gotNames) + for i, name := range gotNames { + assert.Equal(t, fmt.Sprintf("tool-%04d", i), name) + } +} + +// decodeListToolsPage extracts the tool names and the next pagination cursor +// from a tools/list response body. The Serve path streams responses as +// Server-Sent Events (the request Accepts text/event-stream), so the JSON-RPC +// envelope is on the SSE `data:` line; a plain-JSON body is also accepted for +// robustness. Returns (toolNames, nextCursor) — nextCursor is empty on the +// last page. +func decodeListToolsPage(t *testing.T, body []byte) ([]string, string) { + t.Helper() + + // Unwrap the SSE frame if present: take the first `data:` line's payload. + payload := body + for _, line := range strings.Split(string(body), "\n") { + if data, ok := strings.CutPrefix(line, "data: "); ok { + payload = []byte(strings.TrimSpace(data)) + break + } + } + + var envelope struct { + Result struct { + Tools []struct { + Name string `json:"name"` + } `json:"tools"` + NextCursor string `json:"nextCursor"` + } `json:"result"` + Error *struct { + Code int `json:"code"` + Message string `json:"message"` + } `json:"error"` + } + require.NoError(t, json.Unmarshal(payload, &envelope), + "tools/list response must be a JSON-RPC envelope; body: %s", string(body)) + require.Nil(t, envelope.Error, "tools/list must not return a JSON-RPC error: %s", string(body)) + + names := make([]string, 0, len(envelope.Result.Tools)) + for _, tool := range envelope.Result.Tools { + names = append(names, tool.Name) + } + return names, envelope.Result.NextCursor +} + // fakeSDKSession is a minimal server.ClientSession + server.SessionWithTools used to // drive lazyInjectSessionTools directly (the SDK's session-context plumbing is otherwise // only reachable over HTTP, via MCPServer.WithContext). Its tool store is the observable From cacddf335d092eba0d6437b58eb507a0d6db939b Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Mon, 27 Jul 2026 18:05:06 +0300 Subject: [PATCH 2/2] Address review: pin completeness, not the pagination mechanism MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit jhrozek's review of #6021: asserting nextCursor would fail on the exact fix mcpcompat prescribes for >1000-tool aggregations (WithPageSize), so the gate pointed backwards at the improvement. §5 sanctions either branch. Rework to pin the real contract — cursor-following recovers the complete advertised set — and drop the mechanism assertions. Also: reuse readServeJSONRPC/toolNamesFromListResult (removes the SSE-unwrap helper, which was dead code — mcpcompat hardcodes JSONResponse: true, so POST responses are not SSE); send no cursor on the first request (empty string is go-sdk leniency, not spec); move the page cap to the loop top; collapse the per-index assert loop into one require.Equal diff; fix response-body Close ordering. --- pkg/vmcp/server/serve_session_test.go | 137 ++++++++++---------------- 1 file changed, 54 insertions(+), 83 deletions(-) diff --git a/pkg/vmcp/server/serve_session_test.go b/pkg/vmcp/server/serve_session_test.go index 4bceeb646e..078043b291 100644 --- a/pkg/vmcp/server/serve_session_test.go +++ b/pkg/vmcp/server/serve_session_test.go @@ -452,25 +452,32 @@ func TestServeRegistersSessionHooks(t *testing.T) { } // TestServeToolsListPagination_CompleteSet pins #5742 §5: when vMCP's aggregated -// tool set exceeds go-sdk's DefaultPageSize (1000), a downstream client that -// follows the MCP pagination cursor receives the COMPLETE set — the server must -// paginate (emit nextCursor) rather than truncate, and every advertised tool -// must be reachable across the pages. +// tool set exceeds a single page, a downstream client that follows the MCP +// pagination cursor receives the COMPLETE set — every advertised tool reachable, +// no duplicates, no omissions. That completeness is the real contract; how the +// server chunks it is not. // -// go-sdk paginates server-side list responses at DefaultPageSize=1000 -// (mcp/server.go), emitting nextCursor. mcp-go (pre-migration) returned the -// whole set in one page, so a naive reader that issues a single tools/list and -// ignores nextCursor would silently drop the tail — this is the regression the -// migration risked. vMCP does NOT override the page size (no WithPageSize on -// the Serve path), so the served set IS paginated; the contract this pins is -// that cursor-following recovers all of it. +// Background: go-sdk paginates server-side list responses at DefaultPageSize +// (1000), emitting nextCursor; mcp-go (pre-migration) returned the whole set in +// one page, so a naive reader issuing a single tools/list and ignoring +// nextCursor silently drops the tail — the regression the migration risked. +// §5 sanctions either branch ("either the server page size is raised OR the +// test exercises cursor-following"), and mcpcompat's own WithPageSize doc tells +// aggregators with >1000 tools to raise it. So this test deliberately does NOT +// assert that pagination happened — asserting nextCursor would go red on +// exactly that recommended fix even though downstream clients are strictly +// better off and completeness still holds. It drives the cursor loop and +// asserts only the recovered set, which is invariant under both branches. // -// The set is generated deterministically (tool-0000 … tool-1499) so the test -// asserts the exact membership, not just the count. +// The set is generated deterministically (tool-0000 … tool-1499) so the +// assertion checks exact membership, not just the count. totalTools is a local +// constant rather than derived from go-sdk's DefaultPageSize: the mcpcompat +// import doesn't re-export that constant and go-sdk is an indirect dependency, +// so pinning 1500 > 1000 by hand is cheaper than promoting it for one test. func TestServeToolsListPagination_CompleteSet(t *testing.T) { t.Parallel() - const totalTools = 1500 // > go-sdk DefaultPageSize (1000), so the server paginates + const totalTools = 1500 // > go-sdk DefaultPageSize (1000) today, so the server paginates ctrl := gomock.NewController(t) tools := make([]vmcp.Tool, totalTools) @@ -512,92 +519,56 @@ func TestServeToolsListPagination_CompleteSet(t *testing.T) { require.NotEmpty(t, sessionID) require.Eventually(t, state.makeWithIDCalled.Load, 2*time.Second, 10*time.Millisecond) - // Follow the pagination cursor, accumulating tool names until nextCursor is - // empty. A response is a JSON-RPC envelope (possibly SSE-framed), so decode - // the result payload rather than string-matching. + // Drive the cursor loop exactly as a real client does: the first request + // carries NO cursor (an empty string is only accepted by go-sdk leniency — + // mcp/server.go short-circuits "" before decodeCursor — not by any protocol + // guarantee, and the spec shows the initial call cursorless), then each + // non-empty nextCursor is echoed back until the server stops issuing one. + const maxPages = 10 // generous bound; 1500 tools at page size 1000 needs 2 var ( - gotNames []string - cursor string - pages int - sawCursor bool + gotNames []string + cursor string ) - for { - pages++ + for pages := 1; ; pages++ { + require.LessOrEqual(t, pages, maxPages, "pagination must terminate (no infinite cursor)") + + params := map[string]any{} + if cursor != "" { + params["cursor"] = cursor + } listResp := postServeMCP(t, ts.URL, map[string]any{ "jsonrpc": "2.0", - "id": pages + 1, + "id": pages + 100, // stay clear of the initialize id "method": "tools/list", - "params": map[string]any{"cursor": cursor}, + "params": params, }, sessionID) require.Equal(t, http.StatusOK, listResp.StatusCode, "tools/list page %d should succeed", pages) - body, err := io.ReadAll(listResp.Body) + env, _ := readServeJSONRPC(t, listResp) listResp.Body.Close() - require.NoError(t, err) + require.Nil(t, env["error"], "tools/list must not return a JSON-RPC error: %v", env) - names, next := decodeListToolsPage(t, body) - gotNames = append(gotNames, names...) + gotNames = append(gotNames, toolNamesFromListResult(t, env)...) + result, ok := env["result"].(map[string]any) + require.True(t, ok) + next, _ := result["nextCursor"].(string) if next == "" { break } - sawCursor = true cursor = next - require.LessOrEqual(t, pages, 10, "pagination must terminate (no infinite cursor)") - } - - assert.True(t, sawCursor, - "a >1000-tool set must paginate (emit nextCursor); a single-page response would mean the page size was raised or the set truncated") - assert.Greater(t, pages, 1, "a 1500-tool set at page size 1000 spans multiple pages") - - // The complete set must be reachable across the pages, with no duplicates and - // no omissions. - require.Len(t, gotNames, totalTools, - "cursor-following must recover every tool, not just the first page") - sort.Strings(gotNames) - for i, name := range gotNames { - assert.Equal(t, fmt.Sprintf("tool-%04d", i), name) - } -} - -// decodeListToolsPage extracts the tool names and the next pagination cursor -// from a tools/list response body. The Serve path streams responses as -// Server-Sent Events (the request Accepts text/event-stream), so the JSON-RPC -// envelope is on the SSE `data:` line; a plain-JSON body is also accepted for -// robustness. Returns (toolNames, nextCursor) — nextCursor is empty on the -// last page. -func decodeListToolsPage(t *testing.T, body []byte) ([]string, string) { - t.Helper() - - // Unwrap the SSE frame if present: take the first `data:` line's payload. - payload := body - for _, line := range strings.Split(string(body), "\n") { - if data, ok := strings.CutPrefix(line, "data: "); ok { - payload = []byte(strings.TrimSpace(data)) - break - } } - var envelope struct { - Result struct { - Tools []struct { - Name string `json:"name"` - } `json:"tools"` - NextCursor string `json:"nextCursor"` - } `json:"result"` - Error *struct { - Code int `json:"code"` - Message string `json:"message"` - } `json:"error"` + // The complete set must be reachable across the pages, with no duplicates + // and no omissions. require.Equal on the sorted slices dumps a single diff + // on mismatch — far more readable in CI than 1500 per-index assert failures + // when a dropped-and-duplicated pair shifts every later index. + want := make([]string, totalTools) + for i := range want { + want[i] = fmt.Sprintf("tool-%04d", i) } - require.NoError(t, json.Unmarshal(payload, &envelope), - "tools/list response must be a JSON-RPC envelope; body: %s", string(body)) - require.Nil(t, envelope.Error, "tools/list must not return a JSON-RPC error: %s", string(body)) - - names := make([]string, 0, len(envelope.Result.Tools)) - for _, tool := range envelope.Result.Tools { - names = append(names, tool.Name) - } - return names, envelope.Result.NextCursor + sort.Strings(gotNames) + require.Equal(t, want, gotNames, + "cursor-following must recover exactly the advertised set — no duplicates, no omissions") } // fakeSDKSession is a minimal server.ClientSession + server.SessionWithTools used to