diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 5cee8b3231..854a325d04 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -22,6 +22,66 @@ import ( "github.com/github/github-mcp-server/pkg/utils" ) +// pullRequestReadPaginationKind describes which pagination mechanism a +// pull_request_read method honours. +type pullRequestReadPaginationKind int + +const ( + // paginationNone: the method returns a single object and paginates not at all. + paginationNone pullRequestReadPaginationKind = iota + // paginationOffset: the method uses REST offset pagination (page/perPage). + paginationOffset + // paginationCursor: the method uses GraphQL cursor pagination (perPage/after). + paginationCursor +) + +// pullRequestReadPaginationByMethod records the pagination mechanism of every +// pull_request_read method. Methods absent from this map are unknown and are +// left to the dispatch switch, which reports them as such. +var pullRequestReadPaginationByMethod = map[string]pullRequestReadPaginationKind{ + "get": paginationNone, + "get_diff": paginationNone, + "get_status": paginationNone, + "get_files": paginationOffset, + "get_commits": paginationOffset, + "get_reviews": paginationOffset, + "get_comments": paginationOffset, + "get_check_runs": paginationOffset, + "get_review_comments": paginationCursor, +} + +// validatePullRequestReadPagination rejects a pagination parameter that the +// selected method cannot honour. +// +// pull_request_read exposes both pagination mechanisms in a single schema, so +// without this guard the mechanism the method does not use is dropped silently +// and the caller receives the first page again. A tool caller has no reason to +// retry a call that reported success with a plausible payload, so the drop has +// to surface as an error rather than as guidance in the schema description. +// +// perPage is deliberately not guarded: both mechanisms honour it, and methods +// that paginate not at all are commonly called with a client's default page +// size, where rejecting the call would be surprising without being useful. +func validatePullRequestReadPagination(method string, args map[string]any) error { + kind, known := pullRequestReadPaginationByMethod[method] + if !known { + return nil + } + + if _, ok := args["after"]; ok && kind != paginationCursor { + if kind == paginationOffset { + return fmt.Errorf("method %q uses page/perPage pagination; \"after\" is not supported", method) + } + return fmt.Errorf("method %q does not support pagination; \"after\" is not supported", method) + } + + if _, ok := args["page"]; ok && kind == paginationCursor { + return fmt.Errorf("method %q uses cursor pagination; \"page\" is not supported, pass \"after\" instead", method) + } + + return nil +} + // PullRequestRead creates a tool to get details of a specific pull request. func PullRequestRead(t translations.TranslationHelperFunc) inventory.ServerTool { schema := &jsonschema.Schema{ @@ -97,6 +157,9 @@ Possible options: if err != nil { return utils.NewToolResultError(err.Error()), nil, nil } + if err := validatePullRequestReadPagination(method, args); err != nil { + return utils.NewToolResultError(err.Error()), nil, nil + } pagination, err := OptionalPaginationParams(args) if err != nil { return utils.NewToolResultError(err.Error()), nil, nil diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index c0e392aea6..5792477c5b 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -4877,3 +4877,113 @@ func TestResolveReviewThread(t *testing.T) { }) } } + +// failingRoundTripper fails the test if the handler reaches the GitHub API. +type failingRoundTripper struct{ t *testing.T } + +func (f *failingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + f.t.Fatalf("unexpected request to the GitHub API: %s %s", req.Method, req.URL.Path) + return nil, nil +} + +func Test_PullRequestRead_RejectsPaginationTheMethodCannotHonour(t *testing.T) { + tests := []struct { + name string + method string + paginationArgs map[string]any + expectedErrMsg string + }{ + { + name: "after on get_files", + method: "get_files", + paginationArgs: map[string]any{"perPage": float64(10), "after": "Y3Vyc29yOnYyOpHOAA"}, + expectedErrMsg: `method "get_files" uses page/perPage pagination; "after" is not supported`, + }, + { + name: "after on get_commits", + method: "get_commits", + paginationArgs: map[string]any{"after": "Y3Vyc29yOnYyOpHOAA"}, + expectedErrMsg: `method "get_commits" uses page/perPage pagination; "after" is not supported`, + }, + { + name: "after on get_reviews", + method: "get_reviews", + paginationArgs: map[string]any{"after": "Y3Vyc29yOnYyOpHOAA"}, + expectedErrMsg: `method "get_reviews" uses page/perPage pagination; "after" is not supported`, + }, + { + name: "after on get_comments", + method: "get_comments", + paginationArgs: map[string]any{"after": "Y3Vyc29yOnYyOpHOAA"}, + expectedErrMsg: `method "get_comments" uses page/perPage pagination; "after" is not supported`, + }, + { + name: "after on get_check_runs", + method: "get_check_runs", + paginationArgs: map[string]any{"after": "Y3Vyc29yOnYyOpHOAA"}, + expectedErrMsg: `method "get_check_runs" uses page/perPage pagination; "after" is not supported`, + }, + { + name: "after on a method that does not paginate", + method: "get", + paginationArgs: map[string]any{"after": "Y3Vyc29yOnYyOpHOAA"}, + expectedErrMsg: `method "get" does not support pagination; "after" is not supported`, + }, + { + name: "page on get_review_comments", + method: "get_review_comments", + paginationArgs: map[string]any{"page": float64(2), "perPage": float64(10)}, + expectedErrMsg: `method "get_review_comments" uses cursor pagination; "page" is not supported, pass "after" instead`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + args := map[string]any{ + "method": tc.method, + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + } + for k, v := range tc.paginationArgs { + args[k] = v + } + + // The guard must reject before any API call is made. + client := mustNewGHClient(t, &http.Client{Transport: &failingRoundTripper{t: t}}) + deps := BaseDeps{Client: client} + serverTool := PullRequestRead(translations.NullTranslationHelper) + handler := serverTool.Handler(deps) + + request := createMCPRequest(args) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + require.NoError(t, err) + require.True(t, result.IsError) + assert.Equal(t, tc.expectedErrMsg, getErrorResult(t, result).Text) + }) + } +} + +func Test_validatePullRequestReadPagination_Accepts(t *testing.T) { + tests := []struct { + name string + method string + args map[string]any + }{ + {"no pagination parameters", "get_files", map[string]any{}}, + {"page and perPage on an offset method", "get_files", map[string]any{"page": float64(2), "perPage": float64(10)}}, + {"after and perPage on the cursor method", "get_review_comments", map[string]any{"after": "Y3Vyc29yOnYyOpHOAA", "perPage": float64(10)}}, + {"perPage alone on a method that does not paginate", "get", map[string]any{"perPage": float64(10)}}, + {"page alone on a method that does not paginate", "get_diff", map[string]any{"page": float64(2)}}, + // An unrecognised method is left to the dispatch switch, which reports it + // as an unknown method rather than as a pagination problem. + {"unknown method", "get_nothing", map[string]any{"after": "Y3Vyc29yOnYyOpHOAA", "page": float64(2)}}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.NoError(t, validatePullRequestReadPagination(tc.method, tc.args)) + }) + } +}