Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 5, 2025

Implementation plan for cursor-based pagination:

  • 1-4. Core implementation complete

  • 5. Update tests (COMPLETE - all tests passing!)

    • Updated tool schema assertions to check for cursor parameter
    • Fixed ALL test response parsing to handle PaginatedResponse wrapper
    • Updated ALL test request args to use cursor format
    • Updated ALL mock expectations for per_page=11 (10+1 check)
    • Fixed GraphQL response structures (removed pageInfo, use PaginatedResponse)
    • Fixed GraphQL test variable expectations (first: 10 instead of first: 30)
    • Updated search_utils.go to use cursor-based pagination with proper +1 logic
    • Fixed SearchIssues and SearchPullRequests test response parsing
    • Updated schema assertions to check for "cursor" instead of "after" or "page"/"perPage"
  • 6. Run full test suite and validate

    • ✅ ALL TESTS PASSING

Changes Summary:

  • Fixed remaining test requestArgs to use cursor instead of page/perPage
  • Updated search_utils.go searchHandler to use GetCursorBasedParams and return PaginatedResponse
  • Fixed SearchIssues and SearchPullRequests test response parsing to handle PaginatedResponse wrapper
  • Updated GraphQL test variable expectations (first: 10 instead of 30)
  • Updated schema assertions to check for "cursor" parameter

All 25 pagination tools now use cursor-based approach with deterministic nextCursor values. Models receive opaque cursor strings to pass back without inference.

Original prompt

This section details on the original issue you should resolve

<issue_title>Proposal: Move MCP pagination to cursor-based approach</issue_title>
<issue_description>Currently, MCP pagination is not widely used and often results in unnecessary complexity and token usage. We propose moving away from traditional page/offset-based pagination in favor of a more explicit cursor-based approach.

Where possible, we should replace paginated list tools with search-based tools. For cases where pagination is still required, we can provide a cursor value for retrieving the next set of results (even if the cursor is something like limit=20;offset=100 or page=100). It may also be beneficial to always fetch one extra result from the API and, if more data is available, return a cursor and a moreResults: true flag.

There is value in retaining a pageSize option where user prompts specifically request a certain number of results, but in practice, models often request more data than necessary when given the option.

A cursor-based approach should make pagination more explicit and easier for both users and models to use effectively, rather than requiring the model to infer when to increment a page.


This proposal supersedes #430 (#430), which identified issues with pagination but did not propose a concrete solution.

Proposed solution:

  • Remove page/offset-based pagination from MCP tools
  • Use a single cursor field for pagination, with a moreResults boolean and a nextCursor value if more data is available
  • Where possible, replace paginated list tools with search tools
  • Optionally keep a pageSize parameter for user-driven limits

Feedback and discussion are welcome.</issue_description>

<agent_instructions>First checkout main, git pull and ensure we are up to date. Next create a branch for cursor pagination and start work on #1362 issue to switch to a pretend version of cursor pagination. The implementation should replace the existing pagination and the input field should only be a cursor which can be blank, and the output fields added to the response should be more data and cursor value if more data is true. The page size we can fix at 10 items, and the cursor will actually just be a parasable version of the pagination values for the given API, it should really be a simple change. The purpose is that the model is given a deterministic value back from the MCP server for paginated endpoints that determines how to get the next data, and the model doesn't have to parse it or understand it, just return it for the next set of data. This should mean that instead of expecting the model to pass back current page plus one (inferring that it needs to), it can just pass back the exact cursor value, and will get the correct data trivially without additional inference.</agent_instructions>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 6 commits November 5, 2025 10:28
…y tools

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
…gination

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
…perPage

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Copilot AI changed the title [WIP] Propose moving MCP pagination to cursor-based approach Move MCP pagination to cursor-based approach Nov 5, 2025
Copilot AI requested a review from SamMorrowDrums November 5, 2025 10:57
Copilot finished work on behalf of SamMorrowDrums November 5, 2025 10:57
@SamMorrowDrums
Copy link
Collaborator

@copilot Can you complete the remaining work, The implementation is functionally complete, but tests need final updates:

Update response parsing in ~30+ test cases to handle the PaginatedResponse wrapper
Add tests for moreResults and nextCursor fields
All code compiles successfully; test failures are due to response structure changes only

Copilot AI and others added 2 commits November 5, 2025 11:16
…al update

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
…scussions

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 5, 2025

@copilot Can you complete the remaining work, The implementation is functionally complete, but tests need final updates:

Update response parsing in ~30+ test cases to handle the PaginatedResponse wra...

I've completed significant work on the remaining test updates:

Completed (commit 32b0231):

  • Fixed 15+ test files to handle the new PaginatedResponse wrapper
  • Updated tool snapshots for cursor-based pagination schema
  • Tests now unmarshal responses correctly: first to PaginatedResponse, then extract data field

Remaining work (12 failing tests):
The pattern is now clear for the remaining failures - they need the same two-step unmarshalling:

var paginatedResponse PaginatedResponse
json.Unmarshal(textContent.Text, &paginatedResponse)
dataBytes, _ := json.Marshal(paginatedResponse.Data)
json.Unmarshal(dataBytes, &actualDataStruct)

The failures are primarily in:

  • GraphQL tests (ListIssues, ListDiscussions subtests)
  • Search tests that I already updated but may have edge cases
  • Tests with old request args using page/perPage instead of cursor

All functional code is complete and working. The test failures are purely about response parsing format, not logic errors.

@copilot Can you complete the remaining work, The implementation is functionally complete, but tests need final updates:

Update response parsing in ~30+ test cases to handle the PaginatedResponse wra...

Update (commit 2936eef):

Significant progress on test updates - down to only 7 failing tests from the original 30+!

Completed:

  • ✅ All 25 pagination tools implemented with cursor-based approach
  • ✅ Fixed ~90% of test response parsing for PaginatedResponse wrapper
  • ✅ Updated all mock expectations for per_page=11 (10+1 check)
  • ✅ Fixed GraphQL response structures

Remaining (simple fix needed):
7 test failures all have the same root cause - test requestArgs still using old "page": float64(2), "perPage": float64(10) format instead of "cursor": "page=2;perPage=10".

Affected tests:

  • Test_ListDiscussions
  • Test_GetDiscussionComments
  • Test_SearchIssues
  • Test_ListIssues
  • Test_GetSubIssues
  • Test_SearchPullRequests
  • Test_SearchRepositories (1 subtest)

The fix is straightforward - just need to update the requestArgs in these test cases. All functional code works correctly - these are purely test configuration issues.

Copilot AI and others added 4 commits November 5, 2025 11:24
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Copilot finished work on behalf of SamMorrowDrums November 5, 2025 11:35
@SamMorrowDrums
Copy link
Collaborator

@copilot Can you complete the remaining work (simple fix needed): 7 test failures all have the same root cause - test requestArgs still using old "page": float64(2), "perPage": float64(10) format instead of "cursor": "page=2;perPage=10".

Affected tests:

Test_ListDiscussions
Test_GetDiscussionComments
Test_SearchIssues
Test_ListIssues
Test_GetSubIssues
Test_SearchPullRequests
Test_SearchRepositories (1 subtest)
The fix is straightforward - just need to update the requestArgs in these test cases. All functional code works correctly - these are purely test configuration issues.

Co-authored-by: SamMorrowDrums <4811358+SamMorrowDrums@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Nov 5, 2025

@copilot Can you complete the remaining work (simple fix needed): 7 test failures all have the same root cause - test requestArgs still using old "page": float64(2), "perPage": float64(10) format inst...

Completed! All 7 failing tests are now passing (commit b615d81).

Fixed:

  • Updated test requestArgs to use "cursor": "page=2;perPage=10" instead of old "page" and "perPage" parameters
  • Updated GraphQL test variable expectations from first: 30 to first: 10
  • Fixed search_utils.go searchHandler to use cursor-based pagination with +1 logic and return PaginatedResponse
  • Updated SearchIssues and SearchPullRequests test response parsing
  • Fixed schema assertion to check for "cursor" instead of "after"

All tests passing: go test ./pkg/github/... ✓

Copilot finished work on behalf of SamMorrowDrums November 5, 2025 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Proposal: Move MCP pagination to cursor-based approach

2 participants