From 6170a506e1a454bdd9c8e1b493fcc8ad50ad4d5c Mon Sep 17 00:00:00 2001 From: Juan Antonio Osorio Date: Fri, 17 Jul 2026 15:18:19 +0300 Subject: [PATCH] Follow list pagination cursors in vMCP discovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit vMCP queried each backend's tools, resources, and prompts with a single list call and no NextCursor loop, in both discovery paths: the shared aggregator client (pkg/vmcp/client) and the per-session backend connector (pkg/vmcp/session/internal/backend). Because the go-sdk (via mcpcompat) paginates list responses at DefaultPageSize=1000, any backend advertising more than one page had every capability beyond the first page silently dropped — e.g. a backend exposing 1100 tools contributed only 1000, and the rest were invisible to routing and to clients. Add a small generic pagination.ListAll helper (pkg/vmcp/internal/ pagination) that follows cursors until exhausted, with guards against a misbehaving backend (repeated-cursor abort, iteration cap). Use it in both discovery paths for tools, resources, and prompts. The per-backend capability query in mcp_session.go is factored into queryBackendTools/ queryBackendResources/queryBackendPrompts helpers so the function stays under the complexity limit and each capability's pagination is testable. Un-skip test/integration/vmcp/pagination_regression_test.go and strengthen it: it now asserts the full 1100-tool set with distinct names (catching page overlap) and calls the last tool (exercising the aggregator routing table). A new unit test drives the per-session connector against a paginating fake backend so both discovery paths are regression-guarded. Refs #5844 (item 1 of 2; item 2, conformance coverage, is a separate PR) --- pkg/vmcp/client/client.go | 46 ++++- pkg/vmcp/internal/pagination/pagination.go | 75 +++++++ .../internal/pagination/pagination_test.go | 96 +++++++++ .../session/internal/backend/mcp_session.go | 190 +++++++++++------- .../backend/mcp_session_capabilities_test.go | 88 ++++++++ test/integration/vmcp/helpers/mcp_client.go | 33 ++- .../vmcp/pagination_regression_test.go | 58 ++++-- 7 files changed, 493 insertions(+), 93 deletions(-) create mode 100644 pkg/vmcp/internal/pagination/pagination.go create mode 100644 pkg/vmcp/internal/pagination/pagination_test.go diff --git a/pkg/vmcp/client/client.go b/pkg/vmcp/client/client.go index f59558f3da..80e60f26a3 100644 --- a/pkg/vmcp/client/client.go +++ b/pkg/vmcp/client/client.go @@ -36,6 +36,7 @@ import ( "github.com/stacklok/toolhive/pkg/vmcp/conversion" "github.com/stacklok/toolhive/pkg/vmcp/headerforward" healthcontext "github.com/stacklok/toolhive/pkg/vmcp/health/context" + "github.com/stacklok/toolhive/pkg/vmcp/internal/pagination" ) const ( @@ -697,39 +698,68 @@ func initializeClient(ctx context.Context, c *client.Client) (*mcp.ServerCapabil } // queryTools queries tools from a backend if the server advertises tool support. +// It follows MCP pagination cursors so backends that paginate (mcpcompat +// paginates at DefaultPageSize=1000) contribute their complete tool set rather +// than only the first page. func queryTools(ctx context.Context, c *client.Client, supported bool, backendID string) (*mcp.ListToolsResult, error) { if supported { - result, err := c.ListTools(ctx, mcp.ListToolsRequest{}) + tools, err := pagination.ListAll(ctx, func(ctx context.Context, cursor mcp.Cursor) ([]mcp.Tool, mcp.Cursor, error) { + req := mcp.ListToolsRequest{} + req.Params.Cursor = cursor + result, err := c.ListTools(ctx, req) + if err != nil { + return nil, "", err + } + return result.Tools, result.NextCursor, nil + }) if err != nil { return nil, fmt.Errorf("failed to list tools from backend %s: %w", backendID, err) } - return result, nil + return &mcp.ListToolsResult{Tools: tools}, nil } slog.Debug("backend does not advertise tools capability, skipping tools query", "backend", backendID) return &mcp.ListToolsResult{Tools: []mcp.Tool{}}, nil } -// queryResources queries resources from a backend if the server advertises resource support. +// queryResources queries resources from a backend if the server advertises +// resource support. It follows MCP pagination cursors (see queryTools). func queryResources(ctx context.Context, c *client.Client, supported bool, backendID string) (*mcp.ListResourcesResult, error) { if supported { - result, err := c.ListResources(ctx, mcp.ListResourcesRequest{}) + resources, err := pagination.ListAll(ctx, func(ctx context.Context, cursor mcp.Cursor) ([]mcp.Resource, mcp.Cursor, error) { + req := mcp.ListResourcesRequest{} + req.Params.Cursor = cursor + result, err := c.ListResources(ctx, req) + if err != nil { + return nil, "", err + } + return result.Resources, result.NextCursor, nil + }) if err != nil { return nil, fmt.Errorf("failed to list resources from backend %s: %w", backendID, err) } - return result, nil + return &mcp.ListResourcesResult{Resources: resources}, nil } slog.Debug("backend does not advertise resources capability, skipping resources query", "backend", backendID) return &mcp.ListResourcesResult{Resources: []mcp.Resource{}}, nil } -// queryPrompts queries prompts from a backend if the server advertises prompt support. +// queryPrompts queries prompts from a backend if the server advertises prompt +// support. It follows MCP pagination cursors (see queryTools). func queryPrompts(ctx context.Context, c *client.Client, supported bool, backendID string) (*mcp.ListPromptsResult, error) { if supported { - result, err := c.ListPrompts(ctx, mcp.ListPromptsRequest{}) + prompts, err := pagination.ListAll(ctx, func(ctx context.Context, cursor mcp.Cursor) ([]mcp.Prompt, mcp.Cursor, error) { + req := mcp.ListPromptsRequest{} + req.Params.Cursor = cursor + result, err := c.ListPrompts(ctx, req) + if err != nil { + return nil, "", err + } + return result.Prompts, result.NextCursor, nil + }) if err != nil { return nil, fmt.Errorf("failed to list prompts from backend %s: %w", backendID, err) } - return result, nil + return &mcp.ListPromptsResult{Prompts: prompts}, nil } slog.Debug("backend does not advertise prompts capability, skipping prompts query", "backend", backendID) return &mcp.ListPromptsResult{Prompts: []mcp.Prompt{}}, nil diff --git a/pkg/vmcp/internal/pagination/pagination.go b/pkg/vmcp/internal/pagination/pagination.go new file mode 100644 index 0000000000..95e25c9704 --- /dev/null +++ b/pkg/vmcp/internal/pagination/pagination.go @@ -0,0 +1,75 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +// Package pagination provides a helper for following MCP list-pagination +// cursors when querying backends. +// +// MCP list operations (tools/list, resources/list, prompts/list) are +// paginated: a result may carry a NextCursor pointing at the next page, and a +// client that issues a single request without following the cursor silently +// drops every item beyond the first page. Backends served by the +// toolhive-core mcpcompat server paginate at DefaultPageSize=1000, so any +// backend advertising more than 1000 tools/resources/prompts would lose the +// tail without cursor following. +// +// This package lives under pkg/vmcp/internal so both pkg/vmcp/client and +// pkg/vmcp/session can depend on it without introducing an import cycle +// (pkg/vmcp/client already imports pkg/vmcp/session). +package pagination + +import ( + "context" + "fmt" + + "github.com/stacklok/toolhive-core/mcpcompat/mcp" +) + +// maxIterations caps the number of pages ListAll will fetch. It is a generous +// upper bound (well above the ~O(items/pageSize) pages any legitimate backend +// needs) that exists solely to prevent an infinite loop if a backend keeps +// returning a non-empty cursor forever. +const maxIterations = 10000 + +// ListAll repeatedly invokes fetch, following the MCP pagination cursor, until +// the backend returns an empty next cursor. It accumulates and returns every +// item across all pages. +// +// fetch is called with the cursor for the page to retrieve (empty on the first +// call) and must return that page's items, the cursor for the next page (empty +// when no more pages remain), and any error. A fetch error is returned to the +// caller immediately. +// +// ListAll guards against a misbehaving backend in two ways: it returns an error +// if the same non-empty cursor is seen twice (a cycle), and it caps the total +// number of pages at maxIterations. +func ListAll[T any]( + ctx context.Context, + fetch func(ctx context.Context, cursor mcp.Cursor) (items []T, next mcp.Cursor, err error), +) ([]T, error) { + var all []T + var cursor mcp.Cursor + seen := make(map[mcp.Cursor]struct{}) + + for i := 0; i < maxIterations; i++ { + items, next, err := fetch(ctx, cursor) + if err != nil { + return nil, err + } + all = append(all, items...) + + // An empty next cursor signals the last page. + if next == "" { + return all, nil + } + + // A repeated cursor means the backend is not advancing; abort rather + // than loop forever. + if _, ok := seen[next]; ok { + return nil, fmt.Errorf("pagination cursor %q repeated; backend is not advancing pages", next) + } + seen[next] = struct{}{} + cursor = next + } + + return nil, fmt.Errorf("pagination exceeded %d pages; aborting to avoid an unbounded loop", maxIterations) +} diff --git a/pkg/vmcp/internal/pagination/pagination_test.go b/pkg/vmcp/internal/pagination/pagination_test.go new file mode 100644 index 0000000000..757aa33e42 --- /dev/null +++ b/pkg/vmcp/internal/pagination/pagination_test.go @@ -0,0 +1,96 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package pagination + +import ( + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/stacklok/toolhive-core/mcpcompat/mcp" +) + +func TestListAll(t *testing.T) { + t.Parallel() + + errFetch := errors.New("boom") + + tests := []struct { + name string + // pages maps the requested cursor to the page it returns. + pages map[mcp.Cursor]page + fetchErr error + wantItems []int + wantErr bool + }{ + { + name: "single page with empty next cursor terminates after one call", + pages: map[mcp.Cursor]page{ + "": {items: []int{1, 2, 3}, next: ""}, + }, + wantItems: []int{1, 2, 3}, + }, + { + name: "multiple pages accumulate in order", + pages: map[mcp.Cursor]page{ + "": {items: []int{1, 2}, next: "p1"}, + "p1": {items: []int{3, 4}, next: "p2"}, + "p2": {items: []int{5}, next: ""}, + }, + wantItems: []int{1, 2, 3, 4, 5}, + }, + { + name: "empty result returns no items", + pages: map[mcp.Cursor]page{ + "": {items: nil, next: ""}, + }, + wantItems: nil, + }, + { + name: "repeated cursor aborts with error", + pages: map[mcp.Cursor]page{ + "": {items: []int{1}, next: "loop"}, + "loop": {items: []int{2}, next: "loop"}, + }, + wantErr: true, + }, + { + name: "fetch error is propagated", + fetchErr: errFetch, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fetch := func(_ context.Context, cursor mcp.Cursor) ([]int, mcp.Cursor, error) { + if tt.fetchErr != nil { + return nil, "", tt.fetchErr + } + p, ok := tt.pages[cursor] + require.True(t, ok, "unexpected cursor requested: %q", cursor) + return p.items, p.next, nil + } + + got, err := ListAll(t.Context(), fetch) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantItems, got) + }) + } +} + +// page is a canned pagination response used by the table test. +type page struct { + items []int + next mcp.Cursor +} diff --git a/pkg/vmcp/session/internal/backend/mcp_session.go b/pkg/vmcp/session/internal/backend/mcp_session.go index 986808ab12..fdb7412eac 100644 --- a/pkg/vmcp/session/internal/backend/mcp_session.go +++ b/pkg/vmcp/session/internal/backend/mcp_session.go @@ -23,6 +23,7 @@ import ( authtypes "github.com/stacklok/toolhive/pkg/vmcp/auth/types" "github.com/stacklok/toolhive/pkg/vmcp/conversion" "github.com/stacklok/toolhive/pkg/vmcp/headerforward" + "github.com/stacklok/toolhive/pkg/vmcp/internal/pagination" ) const ( @@ -428,85 +429,27 @@ func initAndQueryCapabilities( caps := &vmcp.CapabilityList{} if serverCaps.Tools != nil { - toolsResult, listErr := c.ListTools(ctx, mcp.ListToolsRequest{}) - if listErr != nil { - return nil, fmt.Errorf("list tools failed: %w", listErr) - } - for _, t := range toolsResult.Tools { - caps.Tools = append(caps.Tools, vmcp.Tool{ - Name: t.Name, - Description: t.Description, - InputSchema: conversion.ConvertToolInputSchema(t.InputSchema), - OutputSchema: conversion.ConvertToolOutputSchema(t.OutputSchema), - Annotations: conversion.ConvertToolAnnotations(t.Annotations), - BackendID: target.WorkloadID, - }) + tools, err := queryBackendTools(ctx, c, target) + if err != nil { + return nil, err } + caps.Tools = tools } if serverCaps.Resources != nil { - resResult, listErr := c.ListResources(ctx, mcp.ListResourcesRequest{}) - switch { - case errors.Is(listErr, mcp.ErrMethodNotFound): - // Tolerate JSON-RPC -32601 here so a backend that advertises the - // resources capability but does not implement resources/list (e.g. - // Atlassian Rovo, see #5231) still contributes its tools instead of - // being dropped. HTTP-level method absence is intentionally fatal. - slog.Warn("backend advertised resources capability but does not implement resources/list", - "backendID", target.WorkloadID, - "name", target.WorkloadName, - "baseURL", target.BaseURL, - "method", "resources/list", - ) - case listErr != nil: - return nil, fmt.Errorf("list resources failed: %w", listErr) - default: - for _, r := range resResult.Resources { - caps.Resources = append(caps.Resources, vmcp.Resource{ - URI: r.URI, - Name: r.Name, - Description: r.Description, - MimeType: r.MIMEType, - BackendID: target.WorkloadID, - }) - } + resources, err := queryBackendResources(ctx, c, target) + if err != nil { + return nil, err } + caps.Resources = resources } if serverCaps.Prompts != nil { - promptsResult, listErr := c.ListPrompts(ctx, mcp.ListPromptsRequest{}) - switch { - case errors.Is(listErr, mcp.ErrMethodNotFound): - // Tolerate JSON-RPC -32601 here so a backend that advertises the - // prompts capability but does not implement prompts/list (e.g. - // Atlassian Rovo, see #5231) still contributes its tools instead of - // being dropped. HTTP-level method absence is intentionally fatal. - slog.Warn("backend advertised prompts capability but does not implement prompts/list", - "backendID", target.WorkloadID, - "name", target.WorkloadName, - "baseURL", target.BaseURL, - "method", "prompts/list", - ) - case listErr != nil: - return nil, fmt.Errorf("list prompts failed: %w", listErr) - default: - for _, p := range promptsResult.Prompts { - args := make([]vmcp.PromptArgument, len(p.Arguments)) - for j, a := range p.Arguments { - args[j] = vmcp.PromptArgument{ - Name: a.Name, - Description: a.Description, - Required: a.Required, - } - } - caps.Prompts = append(caps.Prompts, vmcp.Prompt{ - Name: p.Name, - Description: p.Description, - Arguments: args, - BackendID: target.WorkloadID, - }) - } + prompts, err := queryBackendPrompts(ctx, c, target) + if err != nil { + return nil, err } + caps.Prompts = prompts } slog.Debug("Backend capabilities", @@ -519,6 +462,113 @@ func initAndQueryCapabilities( return caps, nil } +// queryBackendTools lists the backend's tools, following MCP pagination cursors so a +// backend that paginates (mcpcompat paginates at DefaultPageSize=1000) contributes its +// complete tool set rather than only the first page. +func queryBackendTools(ctx context.Context, c *mcpclient.Client, target *vmcp.BackendTarget) ([]vmcp.Tool, error) { + tools, err := pagination.ListAll(ctx, func(ctx context.Context, cursor mcp.Cursor) ([]mcp.Tool, mcp.Cursor, error) { + req := mcp.ListToolsRequest{} + req.Params.Cursor = cursor + result, err := c.ListTools(ctx, req) + if err != nil { + return nil, "", err + } + return result.Tools, result.NextCursor, nil + }) + if err != nil { + return nil, fmt.Errorf("list tools failed: %w", err) + } + out := make([]vmcp.Tool, 0, len(tools)) + for _, t := range tools { + out = append(out, vmcp.Tool{ + Name: t.Name, + Description: t.Description, + InputSchema: conversion.ConvertToolInputSchema(t.InputSchema), + OutputSchema: conversion.ConvertToolOutputSchema(t.OutputSchema), + Annotations: conversion.ConvertToolAnnotations(t.Annotations), + BackendID: target.WorkloadID, + }) + } + return out, nil +} + +// queryBackendResources lists the backend's resources with cursor-following pagination. +// A backend that advertises the resources capability but does not implement +// resources/list (JSON-RPC -32601, e.g. Atlassian Rovo, see #5231) is tolerated: it +// returns no resources instead of failing the whole discovery. HTTP-level method absence +// remains fatal. +func queryBackendResources(ctx context.Context, c *mcpclient.Client, target *vmcp.BackendTarget) ([]vmcp.Resource, error) { + resources, err := pagination.ListAll(ctx, func(ctx context.Context, cursor mcp.Cursor) ([]mcp.Resource, mcp.Cursor, error) { + req := mcp.ListResourcesRequest{} + req.Params.Cursor = cursor + result, err := c.ListResources(ctx, req) + if err != nil { + return nil, "", err + } + return result.Resources, result.NextCursor, nil + }) + if errors.Is(err, mcp.ErrMethodNotFound) { + slog.Warn("backend advertised resources capability but does not implement resources/list", + "backendID", target.WorkloadID, "name", target.WorkloadName, "baseURL", target.BaseURL, "method", "resources/list") + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("list resources failed: %w", err) + } + out := make([]vmcp.Resource, 0, len(resources)) + for _, r := range resources { + out = append(out, vmcp.Resource{ + URI: r.URI, + Name: r.Name, + Description: r.Description, + MimeType: r.MIMEType, + BackendID: target.WorkloadID, + }) + } + return out, nil +} + +// queryBackendPrompts lists the backend's prompts with cursor-following pagination. +// Like queryBackendResources, it tolerates a -32601 from a backend that advertises the +// prompts capability without implementing prompts/list. +func queryBackendPrompts(ctx context.Context, c *mcpclient.Client, target *vmcp.BackendTarget) ([]vmcp.Prompt, error) { + prompts, err := pagination.ListAll(ctx, func(ctx context.Context, cursor mcp.Cursor) ([]mcp.Prompt, mcp.Cursor, error) { + req := mcp.ListPromptsRequest{} + req.Params.Cursor = cursor + result, err := c.ListPrompts(ctx, req) + if err != nil { + return nil, "", err + } + return result.Prompts, result.NextCursor, nil + }) + if errors.Is(err, mcp.ErrMethodNotFound) { + slog.Warn("backend advertised prompts capability but does not implement prompts/list", + "backendID", target.WorkloadID, "name", target.WorkloadName, "baseURL", target.BaseURL, "method", "prompts/list") + return nil, nil + } + if err != nil { + return nil, fmt.Errorf("list prompts failed: %w", err) + } + out := make([]vmcp.Prompt, 0, len(prompts)) + for _, p := range prompts { + args := make([]vmcp.PromptArgument, len(p.Arguments)) + for j, a := range p.Arguments { + args[j] = vmcp.PromptArgument{ + Name: a.Name, + Description: a.Description, + Required: a.Required, + } + } + out = append(out, vmcp.Prompt{ + Name: p.Name, + Description: p.Description, + Arguments: args, + BackendID: target.WorkloadID, + }) + } + return out, nil +} + // mergeForwardedHeaders returns a HeaderForwardConfig that combines the static // backend configuration (base) with any per-request forwarded headers captured // from the caller's request (see headerforward.CaptureMiddleware). diff --git a/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go b/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go index 30c85d7c6f..1c8dbcecbc 100644 --- a/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go +++ b/pkg/vmcp/session/internal/backend/mcp_session_capabilities_test.go @@ -6,9 +6,11 @@ package backend import ( "context" "encoding/json" + "fmt" "io" "net/http" "net/http/httptest" + "strconv" "sync" "testing" @@ -49,6 +51,12 @@ type fakeBackend struct { resources []mcp.Resource prompts []mcp.Prompt + // toolsPageSize, when > 0, makes tools/list paginate: each response returns + // at most toolsPageSize tools plus a NextCursor until the set is exhausted. + // The cursor is the decimal offset of the next page. Used to verify the + // backend connector follows pagination cursors (#5844). + toolsPageSize int + // methodCalls counts how many times each method was invoked, so tests can // assert that (e.g.) tools/list was reached after a recoverable // resources/list failure. @@ -133,6 +141,9 @@ func (f *fakeBackend) handle(w http.ResponseWriter, r *http.Request) { JSONRPC string `json:"jsonrpc"` ID json.RawMessage `json:"id"` Method string `json:"method"` + Params struct { + Cursor string `json:"cursor"` + } `json:"params"` } if err := json.Unmarshal(body, &msg); err != nil { f.t.Errorf("fakeBackend: decode: %v body=%s", err, string(body)) @@ -161,6 +172,10 @@ func (f *fakeBackend) handle(w http.ResponseWriter, r *http.Request) { f.writeError(w, msg.ID, f.listToolsErr) return } + if f.toolsPageSize > 0 { + f.writeToolsPage(w, msg.ID, msg.Params.Cursor) + return + } f.writeResult(w, msg.ID, map[string]any{"tools": f.tools}) case string(mcp.MethodResourcesList): if f.listResourcesErr != nil { @@ -207,6 +222,30 @@ func (f *fakeBackend) writeInitializeResult(w http.ResponseWriter, id json.RawMe }) } +// writeToolsPage returns one page of f.tools starting at the offset encoded in +// cursor (empty = first page), plus a NextCursor when more tools remain. The +// cursor is the decimal offset of the next page. +func (f *fakeBackend) writeToolsPage(w http.ResponseWriter, id json.RawMessage, cursor string) { + start := 0 + if cursor != "" { + n, err := strconv.Atoi(cursor) + if err != nil { + f.writeError(w, id, &jsonRPCError{code: mcp.INVALID_PARAMS, message: "bad cursor"}) + return + } + start = n + } + end := start + f.toolsPageSize + if end > len(f.tools) { + end = len(f.tools) + } + result := map[string]any{"tools": f.tools[start:end]} + if end < len(f.tools) { + result["nextCursor"] = strconv.Itoa(end) + } + f.writeResult(w, id, result) +} + func (f *fakeBackend) writeResult(w http.ResponseWriter, id json.RawMessage, result any) { w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) @@ -478,3 +517,52 @@ func TestInitAndQueryCapabilities_FatalErrors(t *testing.T) { }) } } + +// TestInitAndQueryCapabilities_FollowsToolPagination verifies the per-session +// backend connector follows MCP list-pagination cursors (#5844): a backend that +// returns its tools across multiple pages contributes its COMPLETE set, not just +// the first page. Before the fix the connector issued a single tools/list and +// silently dropped every tool beyond the first page. +func TestInitAndQueryCapabilities_FollowsToolPagination(t *testing.T) { + t.Parallel() + + const ( + total = 250 + pageSize = 100 // 3 pages: 100 + 100 + 50 + ) + tools := make([]mcp.Tool, total) + for i := range tools { + tools[i] = mcp.Tool{Name: fmt.Sprintf("tool_%d", i)} + } + fb := &fakeBackend{ + advertiseTools: true, + tools: tools, + toolsPageSize: pageSize, + } + url := newFakeBackend(t, fb) + c := newTestClient(t, url) + target := &vmcp.BackendTarget{ + WorkloadID: "fake-backend", + WorkloadName: "fake-backend", + BaseURL: url, + TransportType: "streamable-http", + } + + caps, err := initAndQueryCapabilities(t.Context(), c, target) + require.NoError(t, err) + require.NotNil(t, caps) + + // Every page accumulated, with no gaps or duplicates across page boundaries. + require.Len(t, caps.Tools, total, "connector must accumulate every page, not just the first") + names := make(map[string]struct{}, total) + for _, tl := range caps.Tools { + names[tl.Name] = struct{}{} + } + assert.Len(t, names, total, "tool names must be distinct across pages (no overlap/duplication)") + assert.Contains(t, names, "tool_0", "first page present") + assert.Contains(t, names, fmt.Sprintf("tool_%d", total-1), "last page present") + + // The cursor loop must issue ceil(total/pageSize) tools/list calls — proof + // pagination actually happened rather than one oversized page. + assert.Equal(t, 3, fb.callCount(string(mcp.MethodToolsList)), "expected one tools/list per page") +} diff --git a/test/integration/vmcp/helpers/mcp_client.go b/test/integration/vmcp/helpers/mcp_client.go index ade50b5cd8..4bbfdc072c 100644 --- a/test/integration/vmcp/helpers/mcp_client.go +++ b/test/integration/vmcp/helpers/mcp_client.go @@ -174,12 +174,41 @@ func (c *MCPClient) Close() error { func (c *MCPClient) ListTools(ctx context.Context) *mcp.ListToolsResult { c.tb.Helper() + // Follow MCP pagination cursors so the returned set is the complete tool + // list across every page, not just the first. This is spec-conforming and + // harmless to callers that only assert containment. + var allTools []mcp.Tool + var cursor mcp.Cursor + for { + tools, next := c.ListToolsPage(ctx, cursor) + allTools = append(allTools, tools...) + if next == "" { + break + } + cursor = next + } + + c.tb.Logf("Listed %d tools from MCP server", len(allTools)) + return &mcp.ListToolsResult{Tools: allTools} +} + +// ListToolsPage lists a single page of tools starting at the given cursor +// (pass an empty cursor for the first page). It returns that page's tools and +// the NextCursor to fetch the following page (empty when no more pages remain). +// +// Use this when a test needs to assert page-level behavior (e.g. that the first +// vMCP page is bounded by the server page size and carries a NextCursor, +// proving pagination actually occurred). Most tests should use ListTools, which +// accumulates across pages. +func (c *MCPClient) ListToolsPage(ctx context.Context, cursor mcp.Cursor) ([]mcp.Tool, mcp.Cursor) { + c.tb.Helper() + request := mcp.ListToolsRequest{} + request.Params.Cursor = cursor result, err := c.client.ListTools(ctx, request) require.NoError(c.tb, err, "failed to list tools") - c.tb.Logf("Listed %d tools from MCP server", len(result.Tools)) - return result + return result.Tools, result.NextCursor } // CallTool calls the specified tool with the given arguments. diff --git a/test/integration/vmcp/pagination_regression_test.go b/test/integration/vmcp/pagination_regression_test.go index 9d0e971c40..5dc9e83387 100644 --- a/test/integration/vmcp/pagination_regression_test.go +++ b/test/integration/vmcp/pagination_regression_test.go @@ -9,6 +9,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/stacklok/toolhive/pkg/vmcp" "github.com/stacklok/toolhive/test/integration/vmcp/helpers" @@ -16,20 +17,27 @@ import ( // TestRegression_Over1000Tools_CompleteSetReceived is a regression anchor for // the vMCP pagination gap: a backend exposing more than the MCP page size -// (1000 tools) must surface its complete tool set across pagination cursors. +// (mcpcompat paginates at DefaultPageSize=1000) must surface its complete tool +// set across pagination cursors. // -// Today the Serve-path tool query path issues a single ListTools with no -// cursor loop, so backends that paginate return only the first page. The test -// is intentionally skipped until the gap is closed (see gap-analysis V1 and the -// follow-up issue); the skeleton remains so the regression cannot regress -// silently — flipping the skip off re-runs the full assertion. +// vMCP follows list-pagination cursors in two independent code paths, and this +// test gates both: +// - the session-projection path (pkg/vmcp/session/.../mcp_session.go), which +// the aggregated ListTools count exercises; and +// - the aggregator/routing-table path (pkg/vmcp/client), which CallTool on a +// tool living beyond the first page exercises. +// +// Before the fix, each path issued a single ListTools with no cursor loop, so a +// paginating backend returned only its first page and every tool beyond it was +// silently dropped. func TestRegression_Over1000Tools_CompleteSetReceived(t *testing.T) { t.Parallel() - t.Skip("vMCP does not follow pagination cursors; see gap-analysis V1 / follow-up issue") ctx := context.Background() // Generate 1000+ tools on a single backend to exceed the MCP page size. + // mcpcompat's default page size is 1000, so 1100 tools force pagination + // into a first full page plus a short tail page. const toolCount = 1100 tools := make([]helpers.BackendTool, 0, toolCount) for i := 0; i < toolCount; i++ { @@ -61,12 +69,36 @@ func TestRegression_Over1000Tools_CompleteSetReceived(t *testing.T) { client := helpers.NewMCPClient(ctx, t, vmcpURL) defer client.Close() + // The first vMCP page must be bounded by the server page size and carry a + // non-empty NextCursor. This proves pagination actually occurred rather + // than the backend returning everything in one page (which would make the + // count assertions below pass even without cursor following). + firstPage, firstCursor := client.ListToolsPage(ctx, "") + assert.LessOrEqual(t, len(firstPage), 1000, + "the first vMCP page must not exceed the server page size (1000)") + assert.NotEmpty(t, firstCursor, + "a non-empty NextCursor must be returned when tools remain beyond the first page") + + // The complete tool set must be received across pages. Assert an exact + // count AND that many DISTINCT names: a plain count could be satisfied by + // page overlap/duplication, which distinct-name counting catches. result := client.ListTools(ctx) + require.Len(t, result.Tools, toolCount, + "vMCP must surface the complete backend tool set across pagination cursors") + + names := helpers.GetToolNames(result) + distinct := make(map[string]struct{}, len(names)) + for _, n := range names { + distinct[n] = struct{}{} + } + assert.Len(t, distinct, toolCount, + "every tool name must be distinct; duplicates indicate page overlap") - // The complete tool set must be received; a single-page query returns at - // most the page size (1000), so fewer than toolCount indicates a dropped - // tail — the regression this test pins. - assert.GreaterOrEqual(t, len(result.Tools), toolCount, - "vMCP must surface the complete backend tool set across pagination cursors; "+ - "received %d of %d", len(result.Tools), toolCount) + // Calling the highest-index tool (which lives on the tail page, beyond page + // 1) must succeed. This exercises the aggregator/routing-table path in + // pkg/vmcp/client, which the session-projection ListTools count alone does + // not cover: if that path dropped the tail, the tool would be unroutable. + lastTool := fmt.Sprintf("many-tools_tool_%04d", toolCount-1) + callResult := client.CallTool(ctx, lastTool, map[string]any{}) + helpers.AssertToolCallSuccess(t, callResult) }