-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: send params as empty object for list methods without cursor #1453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Some external MCP servers implement strict JSON-RPC validation requiring the params field to always be present. Previously, the Python SDK omitted params entirely when cursor=None, causing validation errors. Now sends params: {} when no cursor is provided, matching TypeScript SDK behavior and fixing compatibility with strict servers. Reported-by: justin-yi-wang
Re-evaluating the approach here, as we're basically making the // explicit {} param
const result = await client.listTools({});
// undefined param, gets ommited by JSON.stringify()
const result = await client.listTools(); Whereas with this change we would now always force The spec defines interface ListToolsRequest {
method: “tools/list”;
params?: { cursor?: string };
} https://modelcontextprotocol.io/specification/2025-06-18/schema#listtoolsrequest An alternative solution could be to change the signature of async def list_tools(
self,
cursor: str | None = None, # deprecated
*,
params: types.PaginatedRequestParams | None = None,
) -> types.ListToolsResult: Which would be more complete and equivalent to TS capabilities here: https://github.com/modelcontextprotocol/typescript-sdk/blob/f4b8a48ded019a54a38d3d150a013427d6cbdbc6/src/client/index.ts#L500-L514 |
Deprecate the cursor parameter in list_tools, list_prompts, list_resources, and list_resource_templates methods, replacing it with a new params parameter that accepts PaginatedRequestParams. This change maintains complete backwards compatibility while providing opt-in support for strict MCP servers that require the params field to always be present in JSON-RPC requests (even if empty). When params is not provided or is None, the SDK continues to omit the params field entirely, matching the previous behavior. The cursor parameter now issues a DeprecationWarning directing developers to use the new params parameter. This matches the TypeScript SDK's API design. Reported-by: justin-yi-wang
Add comprehensive tests for the new params parameter across all four list methods (list_tools, list_resources, list_prompts, list_resource_templates). Tests verify: 1. params parameter works correctly when omitted, set to None, or set to empty 2. params parameter correctly passes cursor values 3. params parameter takes precedence over deprecated cursor parameter when both are provided, ensuring a safe migration path for existing code These tests ensure the backwards compatibility guarantees are preserved and prevent regressions in precedence behavior.
Replace 13 individual test functions with 3 parametrized test functions that cover all four list methods (list_tools, list_resources, list_prompts, list_resource_templates). Changes: - Created full_featured_server fixture with tools, resources, prompts, and templates - test_list_methods_cursor_parameter: parametrized version of 4 cursor tests - test_list_methods_params_parameter: parametrized version of 4 params tests - test_list_methods_params_takes_precedence_over_cursor: parametrized precedence tests - Kept test_list_tools_with_strict_server_validation separate (different scenario) Result: Reduced from 600+ lines to ~240 lines while maintaining identical coverage (13 test cases, all passing).
Updated this PR with the approach described in the above comment. This PR is probably easier to review looking at each commit, as I also took the chance to refactor tests for less repetition in the last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think doing something like this would help as it gives errors in the IDE if you use the wrong signature:
@overload
async def list_resources(self, cursor: str) -> types.ListResourcesResult: ...
@overload
async def list_resources(self, *, params: types.PaginatedRequestParams) -> types.ListResourcesResult: ...
@overload
async def list_resources(self) -> types.ListResourcesResult: ...
@deprecated
async def list_resources(
self,
cursor: str | None = None,
*,
params: types.PaginatedRequestParams | None = None,
) -> types.ListResourcesResult:
"""Send a resources/list request.
Args:
cursor: Simple cursor string for pagination (deprecated, use params instead)
params: Full pagination parameters including cursor and any future fields
"""
if params is not None and cursor is not None:
raise ValueError("Cannot specify both cursor and params")
if params is not None:
request_params = params
elif cursor is not None:
request_params = types.PaginatedRequestParams(cursor=cursor)
else:
request_params = None
return await self.send_request(
types.ClientRequest(types.ListResourcesRequest(params=request_params)),
types.ListResourcesResult,
)
For testing here's a script which will show errors in pyright and your IDE:
"""Test file to verify overload typing works correctly."""
import asyncio
from mcp.client.session import ClientSession
from mcp.types import PaginatedRequestParams
async def test_overload_typing() -> None:
"""Test different ways of calling list_resources."""
# This is just for type checking, we don't actually run it
session: ClientSession = None # type: ignore
# Should work: no arguments
result1 = await session.list_resources()
# Should work: cursor as positional string
result2 = await session.list_resources("some_cursor")
# Should work: cursor as keyword string
result3 = await session.list_resources(cursor="some_cursor")
# Should work: params as keyword argument
result4 = await session.list_resources(params=PaginatedRequestParams(cursor="some_cursor"))
# Should ERROR: both cursor and params
result5 = await session.list_resources(cursor="some_cursor", params=PaginatedRequestParams(cursor="other"))
# Should ERROR: cursor=None (overload expects str, not str | None)
result6 = await session.list_resources(cursor=None)
# Should ERROR: params as positional
result7 = await session.list_resources(PaginatedRequestParams(cursor="some_cursor"))
if __name__ == "__main__":
asyncio.run(test_overload_typing())
Implements feedback from maxisbey on PR #1453. This change improves type safety and deprecation handling for list methods (list_tools, list_resources, list_prompts, list_resource_templates): - Add @overload decorators with three signatures to provide clear IDE typing: - cursor: str (positional argument) - params: PaginatedRequestParams (keyword-only argument) - no arguments - Replace warnings.warn() with @deprecated decorator from typing_extensions for cleaner deprecation signaling - Add validation to raise ValueError when both cursor and params are provided, preventing ambiguous parameter combinations - Update tests to expect ValueError when both parameters are specified This ensures the Python SDK has type checking equivalent to the TypeScript SDK and provides better developer experience through IDE assistance and clear deprecation messaging.
@maxisbey implemented the However I think the decorator was in the wrong place - it should only be on the old signature, i.e. I.e. I think the type errors should look like this (what it looks like with the current branch): ![]() What this looks like on hover: ![]() |
Implements feedback from maxisbey on PR #1453. This change improves type safety and deprecation handling for list methods (list_tools, list_resources, list_prompts, list_resource_templates): - Add @overload decorators with three signatures to provide clear IDE typing: - cursor: str (positional argument) - params: PaginatedRequestParams (keyword-only argument) - no arguments - Replace warnings.warn() with @deprecated decorator from typing_extensions for cleaner deprecation signaling - Add validation to raise ValueError when both cursor and params are provided, preventing ambiguous parameter combinations - Update tests to expect ValueError when both parameters are specified This ensures the Python SDK has type checking equivalent to the TypeScript SDK and provides better developer experience through IDE assistance and clear deprecation messaging.
daca273
to
9aa6cfd
Compare
Implements feedback from maxisbey on PR #1453. This change improves type safety and deprecation handling for list methods (list_tools, list_resources, list_prompts, list_resource_templates): - Add @overload decorators with three signatures to provide clear IDE typing: - cursor: str (positional argument) - params: PaginatedRequestParams (keyword-only argument) - no arguments - Replace warnings.warn() with @deprecated decorator from typing_extensions for cleaner deprecation signaling - Add validation to raise ValueError when both cursor and params are provided, preventing ambiguous parameter combinations - Update tests to expect ValueError when both parameters are specified This ensures the Python SDK has type checking equivalent to the TypeScript SDK and provides better developer experience through IDE assistance and clear deprecation messaging.
9aa6cfd
to
3217282
Compare
See: https://docs.python.org/3.13/library/warnings.html#warnings.deprecated @deprecated combined with @overload is specifically called out to be placed _after_ the @overload.
Motivation and Context
Some external MCP servers implement strict JSON-RPC validation requiring the params field to always be present. Previously, the Python SDK omitted params entirely when
cursor=None
, causing validation errors. There is no way to forceparams={}
with the current implementation oflist_tools()
, making it not possible to interact with these servers.This change allows sending
params: {}
by allowing the caller to specify a fulltypes.PaginatedRequestParams
rather than justcursor
, matching TypeScript SDK flexibility and allowing more compatibility with strict servers. We achieve this by introducing a new optional paramparams: types.PaginatedRequestParams | None = None
which will be the new way to interact with this API.How Has This Been Tested?
Added a test demonstrating the specific behavior as well the new param.
Refactored the existing tests to be parameterized.
Breaking Changes
This change is fully backwards compatible as existing behavior stays the same and the new param is completely optional.
Types of changes
Checklist
Additional context