From 2708a6ab80c99cdb2df999252cbca3008ad47b68 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 1 Jul 2025 19:56:00 -0400 Subject: [PATCH 1/3] Enhance get_pull_request_diff tool with pagination and file filtering options Signed-off-by: Tiger Kaovilai --- README.md | 5 + .../__toolsnaps__/get_pull_request_diff.snap | 22 +- pkg/github/pullrequests.go | 139 ++++++++++- pkg/github/pullrequests_test.go | 223 ++++++++++++++++++ 4 files changed, 387 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 44a829601..4471f9aef 100644 --- a/README.md +++ b/README.md @@ -729,6 +729,11 @@ export GITHUB_MCP_TOOL_ADD_ISSUE_COMMENT_DESCRIPTION="an alternative description - `owner`: Repository owner (string, required) - `pullNumber`: Pull request number (number, required) - `repo`: Repository name (string, required) + - `fileList`: If true, only return the list of changed files without diffs (boolean, optional) + - `files`: List of specific files to include in the diff (e.g., ['src/main.go', 'README.md']) (array, optional) + - `pathPrefix`: Only include files with this path prefix (e.g., 'src/' or 'docs/') (string, optional) + - `page`: Page number for file-based pagination (when used with fileList) (number, optional) + - `perPage`: Number of files per page (max 100, default 30) when using fileList (number, optional) - **get_pull_request_files** - Get pull request files - `owner`: Repository owner (string, required) diff --git a/pkg/github/__toolsnaps__/get_pull_request_diff.snap b/pkg/github/__toolsnaps__/get_pull_request_diff.snap index e054eab92..f28e34b0b 100644 --- a/pkg/github/__toolsnaps__/get_pull_request_diff.snap +++ b/pkg/github/__toolsnaps__/get_pull_request_diff.snap @@ -3,13 +3,33 @@ "title": "Get pull request diff", "readOnlyHint": true }, - "description": "Get the diff of a pull request.", + "description": "Get the diff of a pull request. Supports pagination through file filtering to handle large PRs.", "inputSchema": { "properties": { + "fileList": { + "description": "If true, only return the list of changed files without diffs", + "type": "boolean" + }, + "files": { + "description": "List of specific files to include in the diff (e.g., ['src/main.go', 'README.md'])", + "type": "array" + }, "owner": { "description": "Repository owner", "type": "string" }, + "page": { + "description": "Page number for file-based pagination (when used with fileList)", + "type": "number" + }, + "pathPrefix": { + "description": "Only include files with this path prefix (e.g., 'src/' or 'docs/')", + "type": "string" + }, + "perPage": { + "description": "Number of files per page (max 100, default 30) when using fileList", + "type": "number" + }, "pullNumber": { "description": "Pull request number", "type": "number" diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index bad822b13..3db3dfc2b 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -6,6 +6,7 @@ import ( "fmt" "io" "net/http" + "strings" "github.com/go-viper/mapstructure/v2" "github.com/google/go-github/v72/github" @@ -1574,7 +1575,7 @@ func DeletePendingPullRequestReview(getGQLClient GetGQLClientFn, t translations. func GetPullRequestDiff(getClient GetClientFn, t translations.TranslationHelperFunc) (mcp.Tool, server.ToolHandlerFunc) { return mcp.NewTool("get_pull_request_diff", - mcp.WithDescription(t("TOOL_GET_PULL_REQUEST_DIFF_DESCRIPTION", "Get the diff of a pull request.")), + mcp.WithDescription(t("TOOL_GET_PULL_REQUEST_DIFF_DESCRIPTION", "Get the diff of a pull request. Supports pagination through file filtering to handle large PRs.")), mcp.WithToolAnnotation(mcp.ToolAnnotation{ Title: t("TOOL_GET_PULL_REQUEST_DIFF_USER_TITLE", "Get pull request diff"), ReadOnlyHint: ToBoolPtr(true), @@ -1591,12 +1592,32 @@ func GetPullRequestDiff(getClient GetClientFn, t translations.TranslationHelperF mcp.Required(), mcp.Description("Pull request number"), ), + mcp.WithBoolean("fileList", + mcp.Description("If true, only return the list of changed files without diffs"), + ), + mcp.WithArray("files", + mcp.Description("List of specific files to include in the diff (e.g., ['src/main.go', 'README.md'])"), + ), + mcp.WithString("pathPrefix", + mcp.Description("Only include files with this path prefix (e.g., 'src/' or 'docs/')"), + ), + mcp.WithNumber("page", + mcp.Description("Page number for file-based pagination (when used with fileList)"), + ), + mcp.WithNumber("perPage", + mcp.Description("Number of files per page (max 100, default 30) when using fileList"), + ), ), func(ctx context.Context, request mcp.CallToolRequest) (*mcp.CallToolResult, error) { var params struct { Owner string Repo string PullNumber int32 + FileList bool + Files []string + PathPrefix string + Page int + PerPage int } if err := mapstructure.Decode(request.Params.Arguments, ¶ms); err != nil { return mcp.NewToolResultError(err.Error()), nil @@ -1607,6 +1628,122 @@ func GetPullRequestDiff(getClient GetClientFn, t translations.TranslationHelperF return mcp.NewToolResultError(fmt.Sprintf("failed to get GitHub client: %v", err)), nil } + // If fileList is requested, return paginated file list + if params.FileList { + perPage := params.PerPage + if perPage == 0 { + perPage = 30 + } + if perPage > 100 { + perPage = 100 + } + page := params.Page + if page == 0 { + page = 1 + } + + opts := &github.ListOptions{ + PerPage: perPage, + Page: page, + } + files, resp, err := client.PullRequests.ListFiles(ctx, params.Owner, params.Repo, int(params.PullNumber), opts) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to get pull request files", + resp, + err, + ), nil + } + defer func() { _ = resp.Body.Close() }() + + // Filter by path prefix if specified + if params.PathPrefix != "" { + var filtered []*github.CommitFile + for _, file := range files { + if file.Filename != nil && len(*file.Filename) >= len(params.PathPrefix) && (*file.Filename)[:len(params.PathPrefix)] == params.PathPrefix { + filtered = append(filtered, file) + } + } + files = filtered + } + + result := map[string]interface{}{ + "files": files, + "pagination": map[string]interface{}{ + "page": page, + "perPage": perPage, + "totalPages": resp.LastPage, + "hasNext": resp.NextPage > 0, + "hasPrev": resp.PrevPage > 0, + }, + } + + r, err := json.Marshal(result) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + return mcp.NewToolResultText(string(r)), nil + } + + // If specific files are requested, get individual file diffs + if len(params.Files) > 0 || params.PathPrefix != "" { + // First, get all changed files + var allFiles []*github.CommitFile + opts := &github.ListOptions{PerPage: 100} + for { + files, resp, err := client.PullRequests.ListFiles(ctx, params.Owner, params.Repo, int(params.PullNumber), opts) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, + "failed to get pull request files", + resp, + err, + ), nil + } + allFiles = append(allFiles, files...) + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + + // Filter files based on criteria + var filteredFiles []*github.CommitFile + if len(params.Files) > 0 { + fileMap := make(map[string]bool) + for _, f := range params.Files { + fileMap[f] = true + } + for _, file := range allFiles { + if file.Filename != nil && fileMap[*file.Filename] { + filteredFiles = append(filteredFiles, file) + } + } + } else if params.PathPrefix != "" { + for _, file := range allFiles { + if file.Filename != nil && len(*file.Filename) >= len(params.PathPrefix) && (*file.Filename)[:len(params.PathPrefix)] == params.PathPrefix { + filteredFiles = append(filteredFiles, file) + } + } + } + + // Build a partial diff from the filtered files + var diffBuilder strings.Builder + for _, file := range filteredFiles { + if file.Patch != nil { + diffBuilder.WriteString(fmt.Sprintf("diff --git a/%s b/%s\n", *file.Filename, *file.Filename)) + if file.Status != nil { + diffBuilder.WriteString(fmt.Sprintf("--- a/%s\n", *file.Filename)) + diffBuilder.WriteString(fmt.Sprintf("+++ b/%s\n", *file.Filename)) + } + diffBuilder.WriteString(*file.Patch) + diffBuilder.WriteString("\n") + } + } + + return mcp.NewToolResultText(diffBuilder.String()), nil + } + + // Default behavior: get full diff (with warning for large PRs) raw, resp, err := client.PullRequests.GetRaw( ctx, params.Owner, diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 30341e86c..b6771c3fc 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/github-mcp-server/internal/toolsnaps" "github.com/github/github-mcp-server/pkg/translations" "github.com/google/go-github/v72/github" + "github.com/mark3labs/mcp-go/mcp" "github.com/shurcooL/githubv4" "github.com/migueleliasweb/go-github-mock/src/mock" @@ -2590,3 +2591,225 @@ func getLatestPendingReviewQuery(p getLatestPendingReviewQueryParams) githubv4mo ), ) } + +func Test_GetPullRequestDiff(t *testing.T) { + // Verify tool definition once + mockClient := github.NewClient(nil) + tool, _ := GetPullRequestDiff(stubGetClientFn(mockClient), translations.NullTranslationHelper) + + assert.Equal(t, "get_pull_request_diff", tool.Name) + assert.NotEmpty(t, tool.Description) + assert.Contains(t, tool.InputSchema.Properties, "owner") + assert.Contains(t, tool.InputSchema.Properties, "repo") + assert.Contains(t, tool.InputSchema.Properties, "pullNumber") + assert.Contains(t, tool.InputSchema.Properties, "fileList") + assert.Contains(t, tool.InputSchema.Properties, "files") + assert.Contains(t, tool.InputSchema.Properties, "pathPrefix") + assert.Contains(t, tool.InputSchema.Properties, "page") + assert.Contains(t, tool.InputSchema.Properties, "perPage") + assert.ElementsMatch(t, tool.InputSchema.Required, []string{"owner", "repo", "pullNumber"}) + + // Test fileList mode with pagination + t.Run("fileList mode with pagination", func(t *testing.T) { + mockFiles := []*github.CommitFile{ + { + Filename: github.Ptr("src/main.go"), + Additions: github.Ptr(10), + Deletions: github.Ptr(5), + Changes: github.Ptr(15), + Status: github.Ptr("modified"), + }, + { + Filename: github.Ptr("src/utils.go"), + Additions: github.Ptr(20), + Deletions: github.Ptr(0), + Changes: github.Ptr(20), + Status: github.Ptr("added"), + }, + } + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsFilesByOwnerByRepoByPullNumber, + mockFiles, + ), + ) + mockClient := github.NewClient(mockedHTTPClient) + + _, handler := GetPullRequestDiff(stubGetClientFn(mockClient), translations.NullTranslationHelper) + result, err := handler(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Arguments: map[string]interface{}{ + "owner": "testowner", + "repo": "testrepo", + "pullNumber": 42, + "fileList": true, + "page": 1, + "perPage": 30, + }, + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + + var response map[string]interface{} + err = json.Unmarshal([]byte(result.Content[0].(mcp.TextContent).Text), &response) + require.NoError(t, err) + + assert.Contains(t, response, "files") + assert.Contains(t, response, "pagination") + + files := response["files"].([]interface{}) + assert.Len(t, files, 2) + + pagination := response["pagination"].(map[string]interface{}) + assert.Equal(t, float64(1), pagination["page"]) + assert.Equal(t, float64(30), pagination["perPage"]) + }) + + // Test with path prefix filter + t.Run("fileList mode with path prefix", func(t *testing.T) { + mockFiles := []*github.CommitFile{ + { + Filename: github.Ptr("src/main.go"), + Additions: github.Ptr(10), + Deletions: github.Ptr(5), + Changes: github.Ptr(15), + Status: github.Ptr("modified"), + }, + { + Filename: github.Ptr("docs/README.md"), + Additions: github.Ptr(5), + Deletions: github.Ptr(0), + Changes: github.Ptr(5), + Status: github.Ptr("added"), + }, + } + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsFilesByOwnerByRepoByPullNumber, + mockFiles, + ), + ) + mockClient := github.NewClient(mockedHTTPClient) + + _, handler := GetPullRequestDiff(stubGetClientFn(mockClient), translations.NullTranslationHelper) + result, err := handler(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Arguments: map[string]interface{}{ + "owner": "testowner", + "repo": "testrepo", + "pullNumber": 42, + "fileList": true, + "pathPrefix": "src/", + }, + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + + var response map[string]interface{} + err = json.Unmarshal([]byte(result.Content[0].(mcp.TextContent).Text), &response) + require.NoError(t, err) + + files := response["files"].([]interface{}) + assert.Len(t, files, 1) // Only src/main.go should be included + }) + + // Test specific files diff + t.Run("specific files diff", func(t *testing.T) { + mockFiles := []*github.CommitFile{ + { + Filename: github.Ptr("src/main.go"), + Additions: github.Ptr(10), + Deletions: github.Ptr(5), + Changes: github.Ptr(15), + Status: github.Ptr("modified"), + Patch: github.Ptr("@@ -1,5 +1,10 @@\n package main\n \n import \"fmt\"\n+import \"log\"\n \n func main() {"), + }, + { + Filename: github.Ptr("src/utils.go"), + Additions: github.Ptr(20), + Deletions: github.Ptr(0), + Changes: github.Ptr(20), + Status: github.Ptr("added"), + Patch: github.Ptr("@@ -0,0 +1,20 @@\n+package main\n+\n+func helper() string {"), + }, + } + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsFilesByOwnerByRepoByPullNumber, + mockFiles, + ), + ) + mockClient := github.NewClient(mockedHTTPClient) + + _, handler := GetPullRequestDiff(stubGetClientFn(mockClient), translations.NullTranslationHelper) + result, err := handler(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Arguments: map[string]interface{}{ + "owner": "testowner", + "repo": "testrepo", + "pullNumber": 42, + "files": []string{"src/main.go"}, + }, + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + + diff := result.Content[0].(mcp.TextContent).Text + assert.Contains(t, diff, "src/main.go") + assert.Contains(t, diff, "import \"log\"") + assert.NotContains(t, diff, "src/utils.go") + }) + + // Test full diff (default behavior) + t.Run("full diff", func(t *testing.T) { + expectedDiff := `diff --git a/src/main.go b/src/main.go +index abcd123..efgh456 100644 +--- a/src/main.go ++++ b/src/main.go +@@ -1,5 +1,10 @@ + package main + + import "fmt" ++import "log" + + func main() {` + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetReposPullsByOwnerByRepoByPullNumber, + http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(expectedDiff)) + }), + ), + ) + mockClient := github.NewClient(mockedHTTPClient) + + _, handler := GetPullRequestDiff(stubGetClientFn(mockClient), translations.NullTranslationHelper) + result, err := handler(context.Background(), mcp.CallToolRequest{ + Params: mcp.CallToolParams{ + Arguments: map[string]interface{}{ + "owner": "testowner", + "repo": "testrepo", + "pullNumber": 42, + }, + }, + }) + + require.NoError(t, err) + require.NotNil(t, result) + + diff := result.Content[0].(mcp.TextContent).Text + assert.Equal(t, expectedDiff, diff) + }) +} From 8de3c76f20aa3498912fb0da423ea0ab5b9613e6 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 1 Jul 2025 21:31:41 -0400 Subject: [PATCH 2/3] Update pkg/github/pullrequests.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/github/pullrequests.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 3db3dfc2b..3ece74f17 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1660,7 +1660,7 @@ func GetPullRequestDiff(getClient GetClientFn, t translations.TranslationHelperF if params.PathPrefix != "" { var filtered []*github.CommitFile for _, file := range files { - if file.Filename != nil && len(*file.Filename) >= len(params.PathPrefix) && (*file.Filename)[:len(params.PathPrefix)] == params.PathPrefix { + if file.Filename != nil && strings.HasPrefix(*file.Filename, params.PathPrefix) { filtered = append(filtered, file) } } From 6d469bd1ef751523555d13bccefd1c680c392650 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 1 Jul 2025 21:34:20 -0400 Subject: [PATCH 3/3] Update pkg/github/pullrequests.go Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- pkg/github/pullrequests.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index 3ece74f17..f84876fd3 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -1720,7 +1720,7 @@ func GetPullRequestDiff(getClient GetClientFn, t translations.TranslationHelperF } } else if params.PathPrefix != "" { for _, file := range allFiles { - if file.Filename != nil && len(*file.Filename) >= len(params.PathPrefix) && (*file.Filename)[:len(params.PathPrefix)] == params.PathPrefix { + if file.Filename != nil && strings.HasPrefix(*file.Filename, params.PathPrefix) { filteredFiles = append(filteredFiles, file) } }