Skip to content

fix(mcp): follow nextCursor in prompts/list, resources/list and resources/templates/list - #859

Merged
gloryfromca merged 2 commits into
EverMind-AI:mainfrom
yudongyouqing:fix/mcp_list_pagination
Oct 10, 2026
Merged

gloryfromca merged 2 commits into
EverMind-AI:mainfrom
yudongyouqing:fix/mcp_list_pagination

Conversation

@yudongyouqing

Copy link
Copy Markdown
Contributor

Summary

  • New raven/mcp/paging.py walks a paginated list verb page by page with params=PaginatedRequestParams(cursor=...), stopping on an absent or a repeated cursor (the rule fix(mcp): page through tools/list nextCursor #825 introduced for tools/list). The public surface is one typed walker per verb (walk_tools / walk_resources / walk_resource_templates / walk_prompts), so the items a caller loops over carry their real SDK type rather than the Any a session yields at these call sites; the page shape stays behind one private engine.
  • The three remaining verbs now walk through it: resources/list and resources/templates/list in raven/mcp/resources.py, and prompts/list in raven/mcp/prompts.py. Each site keeps its existing _with_timeout around the whole walk, so a server that never stops paging fails visibly instead of stalling the turn.
  • The tools/list loop from fix(mcp): page through tools/list nextCursor #825 in raven/mcp/client.py collapses onto the same helper, so all four verbs share one implementation.
  • Design note, for the author to settle: the module exposes one typed walker per verb over a single private engine, rather than one generic helper. Typed returns give the items their real SDK type (IDE completion, and future-proof the day an attribute rule switches on); under the repo's current ty config, a call-correctness gate with unresolved-attribute off, both shapes pass identically, and collapsing the wrappers into one generic helper is a small change if that reads better.
  • Single-page behaviour is unchanged: existing tests pass without modification beyond the fakes accepting the params argument the walker always passes.

Type

  • Fix

Verification

  • Relevant tests pass locally: uv run pytest tests/test_mcp_resources.py tests/test_mcp_prompts.py tests/test_mcp_client.py tests/test_mcp_manager.py -> 88 passed, including four new tests (multi-page resources, multi-page templates, repeated cursor, multi-page prompts)
  • The new tests fail on current main: stashing only the raven/ changes and running them -> 4 failed; restoring the fix -> pass
  • Relevant lint / type checks pass locally: ruff check + ruff format --check on the touched files; ty check at the pre-change baseline (18 platform-only diagnostics on Windows, no new); lint-imports -> 10 contracts kept, 0 broken
  • User-facing docs or screenshots are updated when needed: not needed, no user-visible copy changes

Risk

  • Security impact considered: reads more of what a server already serves; no new inputs, credentials or surfaces
  • Backward compatibility considered: single-page servers answer one request exactly as before, and the repeated-cursor stop rule is the one fix(mcp): page through tools/list nextCursor #825 already shipped for tools
  • Rollback path is clear: plain revert restores the single-page behaviour

Related Issues

Fixes #855

gloryfromca
gloryfromca previously approved these changes Oct 10, 2026

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Taking this one over #876, which fixes the same thing. Both follow nextCursor on the three verbs, both are CI-green; the difference is where the walk lives.

  • Here: one raven/mcp/paging.py holding _walk plus four typed walkers, and the tools/list loop from #825 moved onto it, so client.py loses its private copy. One implementation, four call sites.
  • #876: _walk_paginated_list copied verbatim into prompts.py and resources.py, client.py untouched. Three copies of the same walk.

#855's proposed fix asked for the first shape in as many words: "One shared helper in raven/mcp/", and "move the tools/list loop from #825 onto it, so all four verbs share one implementation."

Against the issue's acceptance criteria:

Criterion Where it is met
Single-page servers behave exactly as before the existing _Session fakes are unchanged apart from accepting params=None, and still pass
Multi-page prompt / resource / template lists return every item test_a_paging_server_hands_over_every_page in both test files, plus the templates variant
A repeated cursor ends the walk test_a_repeated_cursor_ends_the_walk
tools/list keeps its behaviour after the move tests/test_mcp_client.py:143-157 already covers the walk and the repeated-cursor stop, and now exercises walk_tools
No deprecated cursor= overload params=types.PaginatedRequestParams(cursor=cursor) in _walk

One criterion the issue listed that is worth naming explicitly, because it is easy to get wrong by wrapping one page instead of the walk: the 30s gate stays around the whole thing, _with_timeout(walk_prompts(session)). A server that never stops paging fails visibly rather than stalling the turn.

The _PagingSession fakes answer by the cursor they were asked for and carry assert len(self.cursors) <= len(self._pages), so a walk that lost its stop rule fails loudly instead of looping. That is the part that makes these tests able to fail.

Not a blocker, just naming the trade: _walk is list[Any] inside and the four public walkers put the real SDK type back on at the boundary. For a page shape that differs only in which attribute carries the items, that is the right place to lose and regain the type.

One disclosure so this is not read as more coverage than it is: mrbot has not reviewed this PR. Its GitHub watch is scoped to MEMBER and OWNER authors, so no automated pass ran on any of the outside-contributor PRs. This is a read of the diff, not a second opinion on top of one.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocking: The new paging module must remove its issue-number references to satisfy AGENTS.md section 1.1.

I found one repository-rule blocker; the runtime behavior itself looks correct. I covered the complete github/main...HEAD diff, the three model-facing callers and the existing tools/list connection path through their returned values, the #825 implementation history and #855 acceptance criteria, all live PR review/comment channels, AGENTS.md / CLAUDE.md / CONTEXT-MAP.md, compatibility with the pinned MCP SDK, current-main path overlap and merge-tree, and whether existing tests or assertions were weakened (none were).

Verification:

  • uv run --frozen --all-extras pytest tests/test_mcp_resources.py tests/test_mcp_prompts.py tests/test_mcp_client.py tests/test_mcp_manager.py -q -> 88 passed. The first default-extra run executed no test bodies and produced 88 setup errors because the fresh worktree lacked the raven_everos dev plugin; the all-extras rerun installed the repository-declared dev plugins and passed.
  • Ruff check and format check on all six changed files -> passed.
  • make lint-types -> passed.
  • uv run --frozen --all-extras lint-imports -> 10 contracts kept, 0 broken.
  • Commit-message validation and the source-language check against github/main..HEAD -> passed.
  • Direct SDK inspection confirmed all four methods accept params=PaginatedRequestParams(...) and expose the expected item field plus nextCursor; a direct two-cursor-cycle probe terminated after [None, A, B].
  • git diff --check and git merge-tree --write-tree HEAD github/main -> clean; current main has no overlapping changes in these six files.
  • All current GitHub check runs for this head are completed and green or intentionally skipped.

Comment thread raven/mcp/paging.py Outdated
AGENTS.md section 1.1 keeps source comments free of PR and issue
references. The docstring keeps its behavioral explanation and loses the
ticket numbers, so the module documents the durable pagination constraint
without transient issue context.

@gloryfromca gloryfromca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No blockers; this can merge as far as I am concerned.

Reviewed the full diff and affected callers against AGENTS.md and the context map, including history, backward compatibility, timeout and error behavior, repeated-cursor termination, architecture boundaries, and whether tests were weakened. The shared walker preserves the existing tools/list behavior and correctly follows nextCursor for prompts, resources, and templates. The SDK call shapes and result fields match mcp 1.30.0.

Verification: uv run pytest tests/test_mcp_resources.py tests/test_mcp_prompts.py tests/test_mcp_client.py tests/test_mcp_manager.py -q passed with 88 tests; targeted Ruff, Ruff format, ty, source-language, commit-message, and diff-whitespace checks also passed.

@gloryfromca
gloryfromca merged commit 289426c into EverMind-AI:main Oct 10, 2026
27 checks passed
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.

fix(mcp): follow nextCursor in prompts/list, resources/list and resources/templates/list

2 participants