Repository navigation
fix(api): follow pagination in MCP tools/list so later pages aren't dropped - #43867
goingforstudying-ctrl wants to merge 9 commits into
Conversation
tools/list (and the other list endpoints) can return a nextCursor, but the client never sent it back, so anything past page one never reached the provider. Follow the cursor in MCPClient.list_tools and let the session list methods take one. Stop if the server echoes a cursor.
github-actions
Bot
added
the
size:L
ghfind-review
Bot
added
the
review: medium
Pyrefly Diffbase → PR--- /tmp/pyrefly_base.txt 2026-11-01 04:17:23.201371830 +0000
+++ /tmp/pyrefly_pr.txt 2026-11-01 04:17:00.525233620 +0000
@@ -4051,19 +4051,19 @@
ERROR Argument `(c: Unknown) -> None` is not assignable to parameter `list_roots_callback` with type `ListRootsFnT | None` in function `core.mcp.session.client_session.ClientSession.__init__` [bad-argument-type]
--> tests/unit_tests/core/mcp/session/test_client_session.py:570:97
ERROR Object of class `CreateMessageResult` has no attribute `code` [missing-attribute]
- --> tests/unit_tests/core/mcp/session/test_client_session.py:1041:16
+ --> tests/unit_tests/core/mcp/session/test_client_session.py:1103:16
ERROR Object of class `CreateMessageResult` has no attribute `message` [missing-attribute]
- --> tests/unit_tests/core/mcp/session/test_client_session.py:1042:35
+ --> tests/unit_tests/core/mcp/session/test_client_session.py:1104:35
ERROR Object of class `ListRootsResult` has no attribute `code` [missing-attribute]
- --> tests/unit_tests/core/mcp/session/test_client_session.py:1048:16
+ --> tests/unit_tests/core/mcp/session/test_client_session.py:1110:16
ERROR Object of class `ListRootsResult` has no attribute `message` [missing-attribute]
- --> tests/unit_tests/core/mcp/session/test_client_session.py:1049:35
+ --> tests/unit_tests/core/mcp/session/test_client_session.py:1111:35
WARN Identity comparison `True is False` is always False [unnecessary-comparison]
- --> tests/unit_tests/core/mcp/test_mcp_client.py:459:39
+ --> tests/unit_tests/core/mcp/test_mcp_client.py:568:39
WARN Identity comparison `True is False` is always False [unnecessary-comparison]
- --> tests/unit_tests/core/mcp/test_mcp_client.py:477:39
+ --> tests/unit_tests/core/mcp/test_mcp_client.py:586:39
WARN Identity comparison `True is False` is always False [unnecessary-comparison]
- --> tests/unit_tests/core/mcp/test_mcp_client.py:508:39
+ --> tests/unit_tests/core/mcp/test_mcp_client.py:617:39
ERROR Argument `list[FromClause]` is not assignable to parameter `tables` with type `Sequence[Table] | None` in function `sqlalchemy.sql.schema.MetaData.create_all` [bad-argument-type]
--> tests/unit_tests/core/memory/test_token_buffer_memory.py:59:16
ERROR `in` is not supported between `int` and `object` [not-iterable]
|
Pyrefly Type Coverage
|
goingforstudying-ctrl
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #43867 +/- ##
=======================================
Coverage 89.85% 89.85%
=======================================
Files 6249 6249
Lines 398125 398125
Branches 80223 80223
=======================================
+ Hits 357739 357744 +5
+ Misses 34076 34071 -5
Partials 6310 6310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
… mocks CI's no-new-session-mock guard rejects fresh Mock() sessions in tests. Talking to a real ClientSession over queues covers the wire format too.
CryoThrust
left a comment
CryoThrust
left a comment
There was a problem hiding this comment.
Checked f81a5a1. The pagination direction looks right, but the termination branch combines protocol completion with two different incomplete-list cases. I exercised the exact list_tools and list_provider_tools method bodies with the repository's real MCP Pydantic models, a fake session and a provider/flush recorder: normal pagination and later-page error controls passed; four checks for empty cursors, repeated/cyclic cursors and preserving the old provider directory failed. A local change using cursor is None for completion and MCPConnectionError for a repeated cursor makes all six pass. This is a method-boundary probe, not Dify's full pytest suite, a live server or a database integration test. Please carry the regressions into the existing client/service tests.
| cursor = response.nextCursor | ||
| # A server echoing the same cursor would loop forever, so stop there. | ||
| if not cursor or cursor in seen_cursors: | ||
| break |
There was a problem hiding this comment.
[P2] Fail a cursor cycle instead of returning a partial tool directory. With pages alpha -> next and beta -> next, this break returns [alpha, beta] as a successful complete result. MCPToolManageService.list_provider_tools() then assigns that list to db_provider.tools and flushes it, replacing any previously saved later-page tools. The guard should still prevent another request, but raise an MCPError-compatible exception (the existing MCPConnectionError fits), so the service takes its failure path before assignment/flush. The method-body fixture observes one flush and a replaced provider list on this head; throwing instead preserves the old list with zero flushes. Cover a repeated cursor and an A/B/A cycle, plus the existing normal-pagination control.
| tools.extend(response.tools) | ||
| cursor = response.nextCursor | ||
| # A server echoing the same cursor would loop forever, so stop there. | ||
| if not cursor or cursor in seen_cursors: |
There was a problem hiding this comment.
[P2] Distinguish an absent cursor from an empty opaque cursor. Cursor = str and PaginatedResult.nextCursor: Cursor | None in the repository allow nextCursor: ""; ClientSession also correctly checks cursor is not None when serializing request params. The truthiness check here stops after the first page for that accepted value. In a two-page fixture with an empty first nextCursor, only alpha is returned and the beta page is never requested. Use cursor is None as the terminal condition, then apply the cycle guard independently, and assert that the second request carries cursor: "".
Preserve empty opaque cursors and fail cursor cycles before a partial tool directory can replace the saved provider tools. Cover wire serialization, client pagination, and service persistence on success and failure.
Keep upstream test consolidation, relocate cursor serialization checks into the current session test class, and retain real database pagination tests in the matching service test module.
Summary
Addresses the MCP tool-list pagination gap in #43841.
MCPClient.list_tools()fires a singletools/listrequest and returns the first response, so tools on later pages are omitted. The session layer also needs to accept a cursor to request subsequent pages.The fix is pretty mechanical: the four
list_*methods onClientSession(tools/list,resources/list,resources/templates/list,prompts/list) now accept an optional cursor, and request params stay off the wire entirely when no cursor is passed, so existing servers see the exact same request as before for page one.MCPClient.list_tools()followsnextCursoruntil it's exhausted, collecting each page. I also stop if the server echoes back a cursor we already saw, since that would otherwise spin forever.MCPClientWithAuthRetrypicks this up for free since it wrapssuper().list_tools().Tests: wire-level checks that the cursor actually lands in the request params for all four list methods (and that page one still sends no params), plus client-level tests for multi-page collection, an empty first page, and the repeated-cursor guard.
Not 100% sure about the repeated-cursor guard; the alternative is trusting the server and looping until
nextCursoris null, but one buggy server hanging every tool sync felt worse. Can drop it if you'd rather keep it simple.Screenshots
Checklist