Skip to content

Pin tools/list pagination completeness for >1000-tool vMCP sets - #6021

Merged
JAORMX merged 2 commits into
mainfrom
test/5742-tools-list-pagination
Jul 27, 2026
Merged

Pin tools/list pagination completeness for >1000-tool vMCP sets#6021
JAORMX merged 2 commits into
mainfrom
test/5742-tools-list-pagination

Conversation

@JAORMX

@JAORMX JAORMX commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Part of #5742 (the mcp-go → go-sdk regression gate), test-matrix §5 — pagination behavior:

A vMCP server exposing >1000 tools: verify downstream clients receive the complete set (go-sdk paginates at 1000; mcp-go returned everything).

This was the only matrix item with zero existing coverage (§1, §2, §3, §4, §6 are all pinned by existing tests — verified by surveying pkg/vmcp/server/*_test.go, pkg/transport/bridge_test.go, and the proxy regression suites). Note the direction: everything that existed pins the inbound side (vMCP following a backend's cursor via pagination.ListAll); a downstream client following vMCP's own cursor was genuinely uncovered.

The contract being pinned: completeness. Pre-migration, mcp-go returned a vMCP's full aggregated tool set in one page; the go-sdk-backed server paginates server-side at DefaultPageSize = 1000, emitting nextCursor. A downstream client that issues a single tools/list and ignores nextCursor silently drops the tail.

§5 sanctions either branch — "either the server page size is raised or the test exercises cursor-following" — and mcpcompat's WithPageSize doc tells aggregators with >1000 tools to raise it. So this test deliberately pins completeness, not the pagination mechanism: it drives the cursor loop and asserts the recovered set, which is invariant whether the server paginates at 1000 or a future WithPageSize returns everything in one page. (An earlier draft asserted nextCursor fires; review correctly flagged that this would go red on exactly that recommended fix.)

The test. Stands up a Serve-path vMCP advertising 1500 deterministic tools (tool-0000…tool-1499) and drives tools/list over the Streamable HTTP handler exactly as a real client does — first request cursorless, then echoing each non-empty nextCursor — asserting the recovered set equals the advertised set with no duplicates or omissions. Uses the existing readServeJSONRPC / toolNamesFromListResult helpers (POST responses are application/json, not SSE, since mcpcompat hardcodes JSONResponse: true).

Mutation-verified: dropping the last 500 tools from the advertised set fails exactly on the completeness assertion — the test is not vacuously green.

Type of change

  • Other (describe): test-only

Test plan

  • Unit tests (task test) — full pkg/vmcp/server package green with -race
  • Linting (task lint-fix) — 0 issues

API Compatibility

  • This PR does not break the v1beta1 API.

No production code changed.

Does this introduce a user-facing change?

No.

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.
@github-actions github-actions Bot added the size/S Small PR: 100-299 lines changed label Jul 27, 2026
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.20%. Comparing base (f1dae94) to head (cacddf3).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6021      +/-   ##
==========================================
+ Coverage   72.10%   72.20%   +0.09%     
==========================================
  Files         720      721       +1     
  Lines       74745    74987     +242     
==========================================
+ Hits        53898    54143     +245     
+ Misses      16991    16968      -23     
- Partials     3856     3876      +20     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

amirejaz
amirejaz previously approved these changes Jul 27, 2026

@jhrozek jhrozek left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch on the coverage gap — the outbound direction really was untested. Everything that existed pins the inbound side (vMCP following a backend's cursor: pagination.ListAll, modern_enumerate_test.go, mcp_session_capabilities_test.go:548), so a downstream client following vMCP's own cursor was genuinely uncovered.

Most of what's below is polish. The one I'd actually want changed before merge is the sawCursor assertion — as written it fails on the fix mcpcompat tells aggregators to apply. Details inline.

One thing not worth its own thread: "id": pages + 1 is an opaque off-by-one to dodge the initialize's id: 1; a plain counter reads clearer.

Comment thread pkg/vmcp/server/serve_session_test.go Outdated
Comment thread pkg/vmcp/server/serve_session_test.go Outdated
Comment thread pkg/vmcp/server/serve_session_test.go Outdated
Comment thread pkg/vmcp/server/serve_session_test.go
Comment thread pkg/vmcp/server/serve_session_test.go Outdated
Comment thread pkg/vmcp/server/serve_session_test.go Outdated
Comment thread pkg/vmcp/server/serve_session_test.go Outdated
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.
@github-actions github-actions Bot added size/S Small PR: 100-299 lines changed and removed size/S Small PR: 100-299 lines changed labels Jul 27, 2026
@JAORMX
JAORMX merged commit a8745b8 into main Jul 27, 2026
65 of 66 checks passed
@JAORMX
JAORMX deleted the test/5742-tools-list-pagination branch July 27, 2026 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S Small PR: 100-299 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants