-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat: implement auto-pagination for MCP server list operations and address review feedback #4086
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1125,10 +1125,39 @@ async def list_tools( | |
| tools = self._tools_list | ||
| else: | ||
| # Fetch the tools from the server | ||
| result = await self._run_with_retries( | ||
| lambda: self._maybe_serialize_request(lambda: session.list_tools()) | ||
| ) | ||
| self._tools_list = result.tools | ||
| all_tools = [] | ||
| cursor = None | ||
| seen_cursors: set[str | None] = set() | ||
| while True: | ||
| if cursor in seen_cursors: | ||
| logger.warning( | ||
| "MCP server %s returned repeated cursor during list_tools. " | ||
| "Breaking to prevent infinite loop.", | ||
| self._error_name, | ||
| ) | ||
| break | ||
| seen_cursors.add(cursor) | ||
| _cursor: str | None = cursor | ||
|
|
||
| async def fetch_tools_page(page_cursor: str | None = _cursor) -> Any: | ||
| return await self._maybe_serialize_request( | ||
| lambda: session.list_tools(cursor=page_cursor) | ||
| ) | ||
|
|
||
| result = await self._run_with_retries(fetch_tools_page) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Because Useful? React with 👍 / 👎. |
||
| next_cursor = result.nextCursor | ||
| if next_cursor is not None and next_cursor in seen_cursors: | ||
| logger.warning( | ||
| "MCP server %s returned repeated cursor during list_tools. " | ||
| "Breaking to prevent infinite loop.", | ||
| self._error_name, | ||
| ) | ||
| break | ||
| all_tools.extend(result.tools) | ||
|
akshay183 marked this conversation as resolved.
|
||
| if next_cursor is None: | ||
| break | ||
| cursor = next_cursor | ||
| self._tools_list = all_tools | ||
| self._cache_dirty = False | ||
| tools = self._tools_list | ||
|
|
||
|
|
@@ -1265,9 +1294,53 @@ async def list_prompts( | |
| raise UserError("Server not initialized. Make sure you call `connect()` first.") | ||
| session = self.session | ||
| assert session is not None | ||
| return await self._run_request_with_transport_error_redaction( | ||
| "list prompts", | ||
| lambda: self._maybe_serialize_request(lambda: session.list_prompts()), | ||
| all_prompts = [] | ||
| cursor = None | ||
| seen_cursors: set[str | None] = set() | ||
| first_result: ListPromptsResult | None = None | ||
| combined_meta: dict[str, Any] = {} | ||
| while True: | ||
| if cursor in seen_cursors: | ||
| logger.warning( | ||
| "MCP server %s returned repeated cursor during list_prompts. " | ||
| "Breaking to prevent infinite loop.", | ||
| self._error_name, | ||
| ) | ||
| break | ||
| seen_cursors.add(cursor) | ||
| _cursor: str | None = cursor | ||
|
|
||
| async def fetch_prompts_page(page_cursor: str | None = _cursor) -> ListPromptsResult: | ||
| return await self._maybe_serialize_request( | ||
| lambda: session.list_prompts(cursor=page_cursor) | ||
| ) | ||
|
|
||
| result = await self._run_request_with_transport_error_redaction( | ||
| "list prompts", fetch_prompts_page | ||
| ) | ||
| next_cursor = result.nextCursor | ||
| if next_cursor is not None and next_cursor in seen_cursors: | ||
| logger.warning( | ||
| "MCP server %s returned repeated cursor during list_prompts. " | ||
| "Breaking to prevent infinite loop.", | ||
| self._error_name, | ||
| ) | ||
| break | ||
| if first_result is None: | ||
| first_result = result | ||
| all_prompts.extend(result.prompts) | ||
| if result.meta: | ||
| combined_meta.update(result.meta) | ||
| if next_cursor is None: | ||
| break | ||
| cursor = next_cursor | ||
| assert first_result is not None | ||
| return first_result.model_copy( | ||
| update={ | ||
| "prompts": all_prompts, | ||
| "nextCursor": None, | ||
| "meta": combined_meta or None, | ||
| } | ||
| ) | ||
|
|
||
| async def get_prompt( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,146 @@ | ||
| from unittest.mock import AsyncMock, MagicMock, call | ||
|
|
||
| import pytest | ||
| from mcp.types import ListPromptsResult, ListToolsResult, Prompt, Tool | ||
|
|
||
| from agents.mcp.server import MCPServerStreamableHttp | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def server(): | ||
| return MCPServerStreamableHttp({"url": "http://localhost"}) | ||
|
|
||
|
|
||
| def _tool(name: str) -> Tool: | ||
| return Tool(name=name, description="", inputSchema={"type": "object", "properties": {}}) | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_tools_accumulates_pages(server: MCPServerStreamableHttp): | ||
| mock_session = MagicMock() | ||
| mock_session.list_tools = AsyncMock( | ||
| side_effect=[ | ||
| ListToolsResult(tools=[_tool("tool_1")], nextCursor="page_2"), | ||
| ListToolsResult(tools=[_tool("tool_2")]), | ||
| ] | ||
| ) | ||
| server.session = mock_session | ||
|
|
||
| result = await server.list_tools() | ||
|
|
||
| assert [tool.name for tool in result] == ["tool_1", "tool_2"] | ||
| assert mock_session.list_tools.await_args_list == [ | ||
| call(cursor=None), | ||
| call(cursor="page_2"), | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_tools_does_not_append_repeated_cursor_page(server: MCPServerStreamableHttp): | ||
| mock_session = MagicMock() | ||
| mock_session.list_tools = AsyncMock( | ||
| side_effect=[ | ||
| ListToolsResult(tools=[_tool("tool_1")], nextCursor="tok_loop"), | ||
| ListToolsResult(tools=[_tool("duplicate")], nextCursor="tok_loop"), | ||
| ] | ||
| ) | ||
| server.session = mock_session | ||
|
|
||
| result = await server.list_tools() | ||
|
|
||
| assert [tool.name for tool in result] == ["tool_1"] | ||
| assert mock_session.list_tools.await_count == 2 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_prompts_accumulates_pages_and_merges_metadata( | ||
| server: MCPServerStreamableHttp, | ||
| ): | ||
| mock_session = MagicMock() | ||
| mock_session.list_prompts = AsyncMock( | ||
| side_effect=[ | ||
| ListPromptsResult( | ||
| prompts=[Prompt(name="prompt_1", description="")], | ||
| nextCursor="page_2", | ||
| _meta={"page_1": True, "shared": "first"}, | ||
| ), | ||
| ListPromptsResult( | ||
| prompts=[Prompt(name="prompt_2", description="")], | ||
| _meta={"page_2": True, "shared": "second"}, | ||
| ), | ||
| ] | ||
| ) | ||
| server.session = mock_session | ||
|
|
||
| result = await server.list_prompts() | ||
|
|
||
| assert [prompt.name for prompt in result.prompts] == ["prompt_1", "prompt_2"] | ||
| assert result.nextCursor is None | ||
| assert result.meta == {"page_1": True, "page_2": True, "shared": "second"} | ||
| assert mock_session.list_prompts.await_args_list == [ | ||
| call(cursor=None), | ||
| call(cursor="page_2"), | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_prompts_does_not_append_repeated_cursor_page( | ||
| server: MCPServerStreamableHttp, | ||
| ): | ||
| mock_session = MagicMock() | ||
| mock_session.list_prompts = AsyncMock( | ||
| side_effect=[ | ||
| ListPromptsResult( | ||
| prompts=[Prompt(name="prompt_1", description="")], | ||
| nextCursor="tok_loop", | ||
| _meta={"page": 1}, | ||
| ), | ||
| ListPromptsResult( | ||
| prompts=[Prompt(name="duplicate", description="")], | ||
| nextCursor="tok_loop", | ||
| _meta={"page": 2}, | ||
| ), | ||
| ] | ||
| ) | ||
| server.session = mock_session | ||
|
|
||
| result = await server.list_prompts() | ||
|
|
||
| assert [prompt.name for prompt in result.prompts] == ["prompt_1"] | ||
| assert result.meta == {"page": 1} | ||
| assert mock_session.list_prompts.await_count == 2 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_prompts_returns_none_metadata_when_pages_have_none( | ||
| server: MCPServerStreamableHttp, | ||
| ): | ||
| mock_session = MagicMock() | ||
| mock_session.list_prompts = AsyncMock( | ||
| return_value=ListPromptsResult(prompts=[Prompt(name="prompt", description="")]) | ||
| ) | ||
| server.session = mock_session | ||
|
|
||
| result = await server.list_prompts() | ||
|
|
||
| assert result.meta is None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_list_tools_supports_empty_string_next_cursor(server: MCPServerStreamableHttp): | ||
| mock_session = MagicMock() | ||
| mock_session.list_tools = AsyncMock( | ||
| side_effect=[ | ||
| ListToolsResult(tools=[_tool("tool_1")], nextCursor=""), | ||
| ListToolsResult(tools=[_tool("tool_2")]), | ||
| ] | ||
| ) | ||
| server.session = mock_session | ||
|
|
||
| result = await server.list_tools() | ||
|
|
||
| assert [tool.name for tool in result] == ["tool_1", "tool_2"] | ||
| assert mock_session.list_tools.await_args_list == [ | ||
| call(cursor=None), | ||
| call(cursor=""), | ||
| ] |
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.
This new cursor keyword needs corresponding updates to the repo's existing session doubles:
DummySession.list_tools(self)intests/mcp/test_client_session_retries.pyhas nocursorparameter, andtest_list_tools_unlimited_retriesconfiguresmax_retry_attempts=-1, so the resulting deterministicTypeErroris retried forever instead of completing. The same signature mismatch exists for the prompt doubles reached by the new prompt pagination path, so update those fakes/adapters to acceptcursorbefore merging.Useful? React with 👍 / 👎.