fix(pull_request_read): expose 'after' cursor parameter in input schema#2489
Open
jluocsa wants to merge 1 commit into
Open
fix(pull_request_read): expose 'after' cursor parameter in input schema#2489jluocsa wants to merge 1 commit into
jluocsa wants to merge 1 commit into
Conversation
The `pull_request_read` tool description tells clients that `get_review_comments` uses cursor-based pagination (`perPage`, `after`), and the handler does plumb `after` through to the GraphQL query, but the input schema only declared `page` and `perPage` (via `WithPagination`). Because `after` was not advertised in `inputSchema`, MCP clients had no way to request it, leaving cursor pagination effectively broken: `perPage: 1` returned only the first thread with no way to advance, and `page` was silently ignored by the GraphQL path. This change adds `after` to the schema (string, optional) with a description making clear it only applies to `get_review_comments`. All other methods continue to ignore it. No handler behavior is changed. - Add `after` schema property after `WithPagination` in `PullRequestRead` - Regenerate `__toolsnaps__/pull_request_read.snap` and update README - Add a regression test asserting `after` is in the schema and a new table-driven case verifying the cursor is forwarded to the GraphQL query Fixes github#2122 (for the `get_review_comments` pagination part). The remaining concerns in github#2122 about unbounded response sizes for `get`, `get_diff`, and `get_reviews` are deferred to follow-up design.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
The
pull_request_readtool advertises in both its description and the documentation that theget_review_commentsmethod uses cursor-based pagination (perPage,after), and the handler does plumbafterthrough to the GraphQL query. But the tool'sinputSchemaonly declaredpageandperPage(viaWithPagination) —afterwas never exposed.As a result, MCP clients had no way to request the
aftercursor, leaving cursor pagination effectively broken:perPage: 1returned only the first thread with no way to advancepage: 2, 3, ...was silently ignored by the GraphQL pathperPage, which risks the unbounded-payload problem also tracked inpull_request_readmethods return unbounded payloads that crash IDE clients;get_review_commentspagination is broken (afterparam missing from schema) #2122This PR is a small, surgical fix for the schema bug.
Changes
pkg/github/pullrequests.go: After the existingWithPagination(schema)call inPullRequestRead, add anafterproperty (string, optional) to the schema with a description making clear it only applies toget_review_comments. All other methods continue to ignore it. No handler behavior is changed.pkg/github/__toolsnaps__/pull_request_read.snap: Regenerated viaUPDATE_TOOLSNAPS=true go test ./pkg/github.README.md: Regenerated viago run ./cmd/github-mcp-server generate-docs.pkg/github/pullrequests_test.go: Augment the schema-verification block inTest_GetPullRequestCommentsto assertafteris present (regression guard) and add a new table-driven case"after cursor is forwarded to GraphQL query"that verifies the cursor flows fromrequestArgs["after"]into the GraphQL variables.Validation
golangci-lintis not installed locally; CI will runscript/lint.The new test case provides explicit coverage: the GraphQL mock matcher expects
"after": githubv4.String("cursor-page-2")and the test passes"after": "cursor-page-2"inrequestArgs, so a regression that dropsafterfrom the schema or handler wiring would fail this test.Scope note
This PR only addresses the missing-
afterportion of #2122. The remaining concerns in that issue — unbounded payloads fromget,get_diff, andget_reviews— involve more invasive API surface choices (field-selection parameters, truncation, etc.) and are better handled in a follow-up after design discussion. Happy to take that on as a separate PR if maintainers prefer a particular approach.Fixes #2122 (partial)