Skip to content

Conversation

nbarbettini
Copy link
Contributor

Motivation and Context

Fixes #735 by:

  • Updating request params to correctly follow the pagination spec
  • Updated tests that assert the payload sent to the server conforms to the spec (regardless of whether the server supports it)

See also: PR comments inline
I'm happy to update the approach if it doesn't fit the style of this repo.

How Has This Been Tested?

  • Updated unit tests
  • Manual verification

Breaking Changes

None - internal changes only, no client interface changes.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

Comment on lines -82 to -87
class PaginatedRequest(Request[RequestParamsT, MethodT]):
cursor: Cursor | None = None
"""
An opaque token representing the current pagination position.
If provided, the server should return results starting after this cursor.
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this, since it was only used for the methods in question.

Comment on lines +88 to +90
@pytest.fixture
def stream_spy():
"""Fixture that provides spies for both client and server write streams.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Built this as a fixture so that it can be easily reused in multiple tests.

I needed a way to verify the request payload "on the wire" and this seemed like a good approach, since it did not require any modification of the actual client code (only test fixtures).

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you so much for the fix!

Would be great to keep types consistent with the spec
once that done, ready to merge

@nbarbettini
Copy link
Contributor Author

Thanks for the guidance @ihrpr! I've pushed these changes:

  • Added a PaginatedRequest class to match the spec's inheritance hierarchy
  • Updated the list_xyz() methods to omit params if cursor is None (tests still pass)

Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you so much @nbarbettini for the quick turnaround.
I think we can remove params: PaginatedRequestParams | None = None?

@nbarbettini nbarbettini requested a review from ihrpr May 21, 2025 17:27
Copy link
Contributor

@ihrpr ihrpr left a comment

Choose a reason for hiding this comment

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

Thank you soo much!

@ihrpr ihrpr merged commit e80c015 into modelcontextprotocol:main May 21, 2025
10 checks passed
@nbarbettini nbarbettini deleted the nate/fix-pagination-cursor branch May 21, 2025 22:25
@nbarbettini
Copy link
Contributor Author

No problem, happy to help!

saqadri pushed a commit to saqadri/stdio-fixes that referenced this pull request Aug 6, 2025
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.

Cursors are not sent to the server correctly

2 participants