From cc5b856718711b00f3e863b72a898f7bcc939e72 Mon Sep 17 00:00:00 2001 From: salakonrad Date: Mon, 8 Jun 2026 08:33:55 +0200 Subject: [PATCH 1/2] feat: add get_reviewers and get_status_checks methods to pull_request_read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the pull_request_read tool with two new methods: - get_reviewers: returns pending reviewer requests (both individual users and teams) using the PullRequests.ListReviewers API. This fills a gap in the existing toolset — requested team reviewers were previously not exposed anywhere in the MCP server. - get_status_checks: returns a unified view of all status checks for a PR's head commit by combining legacy commit statuses (e.g. Atlantis plan/policy results) and modern GitHub Actions check runs into a single response with a top-level combined_state. New output types added to minimal_types.go: - MinimalReviewerUser, MinimalReviewerTeam, MinimalPRReviewers - MinimalCommitStatus, MinimalStatusChecks Adds unit tests for both methods (success and API-failure paths), regenerates the pull_request_read toolsnap, and updates the generated README tool documentation. Co-Authored-By: Claude Sonnet 4.6 --- README.md | 2 + .../__toolsnaps__/pull_request_read.snap | 6 +- pkg/github/helper_test.go | 1 + pkg/github/minimal_types.go | 35 +++ pkg/github/projects_resolver_test.go | 2 +- pkg/github/pullrequests.go | 121 +++++++- pkg/github/pullrequests_test.go | 259 ++++++++++++++++++ 7 files changed, 422 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 10d987a262..b4949ef7e5 100644 --- a/README.md +++ b/README.md @@ -1189,6 +1189,8 @@ The following sets of tools are available: 7. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned. 8. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned. 9. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR. + 10. get_reviewers - Get the list of requested reviewers (users and teams) for a pull request who have not yet submitted a review. + 11. get_status_checks - Get a unified view of all status checks for a pull request, combining legacy commit statuses and modern check runs. (string, required) - `owner`: Repository owner (string, required) - `page`: Page number for pagination (min 1) (number, optional) diff --git a/pkg/github/__toolsnaps__/pull_request_read.snap b/pkg/github/__toolsnaps__/pull_request_read.snap index 41bc90b597..9f92b416d2 100644 --- a/pkg/github/__toolsnaps__/pull_request_read.snap +++ b/pkg/github/__toolsnaps__/pull_request_read.snap @@ -12,7 +12,7 @@ "type": "string" }, "method": { - "description": "Action to specify what pull request data needs to be retrieved from GitHub. \nPossible options: \n 1. get - Get details of a specific pull request.\n 2. get_diff - Get the diff of a pull request.\n 3. get_status - Get combined commit status of a head commit in a pull request.\n 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.\n 5. get_commits - Get the list of commits on a pull request. Use with pagination parameters to control the number of results returned.\n 6. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.\n 7. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned.\n 8. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.\n 9. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.\n", + "description": "Action to specify what pull request data needs to be retrieved from GitHub. \nPossible options: \n 1. get - Get details of a specific pull request.\n 2. get_diff - Get the diff of a pull request.\n 3. get_status - Get combined commit status of a head commit in a pull request.\n 4. get_files - Get the list of files changed in a pull request. Use with pagination parameters to control the number of results returned.\n 5. get_commits - Get the list of commits on a pull request. Use with pagination parameters to control the number of results returned.\n 6. get_review_comments - Get review threads on a pull request. Each thread contains logically grouped review comments made on the same code location during pull request reviews. Returns threads with metadata (isResolved, isOutdated, isCollapsed) and their associated comments. Use cursor-based pagination (perPage, after) to control results.\n 7. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned.\n 8. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned.\n 9. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR.\n 10. get_reviewers - Get the list of requested reviewers (users and teams) for a pull request who have not yet submitted a review.\n 11. get_status_checks - Get a unified view of all status checks for a pull request, combining legacy commit statuses and modern check runs.\n", "enum": [ "get", "get_diff", @@ -22,7 +22,9 @@ "get_review_comments", "get_reviews", "get_comments", - "get_check_runs" + "get_check_runs", + "get_reviewers", + "get_status_checks" ], "type": "string" }, diff --git a/pkg/github/helper_test.go b/pkg/github/helper_test.go index c5a73d9667..2f95a425aa 100644 --- a/pkg/github/helper_test.go +++ b/pkg/github/helper_test.go @@ -82,6 +82,7 @@ const ( PutReposPullsMergeByOwnerByRepoByPullNumber = "PUT /repos/{owner}/{repo}/pulls/{pull_number}/merge" PutReposPullsUpdateBranchByOwnerByRepoByPullNumber = "PUT /repos/{owner}/{repo}/pulls/{pull_number}/update-branch" PostReposPullsRequestedReviewersByOwnerByRepoByPullNumber = "POST /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers" + GetReposPullsRequestedReviewersByOwnerByRepoByPullNumber = "GET /repos/{owner}/{repo}/pulls/{pull_number}/requested_reviewers" PostReposPullsCommentsByOwnerByRepoByPullNumber = "POST /repos/{owner}/{repo}/pulls/{pull_number}/comments" PostReposPullsCommentsReactionsByOwnerByRepoByCommentID = "POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions" diff --git a/pkg/github/minimal_types.go b/pkg/github/minimal_types.go index 75bc8f48f1..eb772b1b62 100644 --- a/pkg/github/minimal_types.go +++ b/pkg/github/minimal_types.go @@ -1827,6 +1827,41 @@ type MinimalCheckRunsResult struct { CheckRuns []MinimalCheckRun `json:"check_runs"` } +// MinimalReviewerUser is a user requested to review a pull request. +type MinimalReviewerUser struct { + Login string `json:"login"` + HTMLURL string `json:"html_url,omitempty"` +} + +// MinimalReviewerTeam is a team requested to review a pull request. +type MinimalReviewerTeam struct { + Slug string `json:"slug"` + Name string `json:"name,omitempty"` + HTMLURL string `json:"html_url,omitempty"` +} + +// MinimalPRReviewers contains the list of requested reviewers for a pull request. +type MinimalPRReviewers struct { + Users []MinimalReviewerUser `json:"users"` + Teams []MinimalReviewerTeam `json:"teams"` +} + +// MinimalCommitStatus represents a single commit status check. +type MinimalCommitStatus struct { + State string `json:"state"` + Context string `json:"context"` + Description string `json:"description,omitempty"` + TargetURL string `json:"target_url,omitempty"` +} + +// MinimalStatusChecks contains a unified view of all status checks for a PR. +type MinimalStatusChecks struct { + CombinedState string `json:"combined_state"` + Statuses []MinimalCommitStatus `json:"statuses"` + CheckRuns []MinimalCheckRun `json:"check_runs"` + TotalCount int `json:"total_count"` +} + // convertToMinimalCheckRun converts a GitHub API CheckRun to MinimalCheckRun func convertToMinimalCheckRun(checkRun *github.CheckRun) MinimalCheckRun { minimalCheckRun := MinimalCheckRun{ diff --git a/pkg/github/projects_resolver_test.go b/pkg/github/projects_resolver_test.go index e701441c1e..4325aa5636 100644 --- a/pkg/github/projects_resolver_test.go +++ b/pkg/github/projects_resolver_test.go @@ -358,7 +358,7 @@ func Test_ResolveFieldNamesToIDs_Success(t *testing.T) { assert.Equal(t, []int64{100, 200}, ids) } -// Field and single-select option name matching is case-insensitive so agents passing lowercase +// Field and single-select option name matching is case-insensitive so agents passing lowercase // names like "status" or "in progress" resolve to "Status" and "In Progress" respectively. func Test_ResolveProjectFieldByName_CaseInsensitive(t *testing.T) { mocked := githubv4mock.NewMockedHTTPClient( diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index daf3b97331..d302a4ad7a 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -41,8 +41,10 @@ Possible options: 7. get_reviews - Get the reviews on a pull request. When asked for review comments, use get_review_comments method. Use with pagination parameters to control the number of results returned. 8. get_comments - Get comments on a pull request. Use this if user doesn't specifically want review comments. Use with pagination parameters to control the number of results returned. 9. get_check_runs - Get check runs for the head commit of a pull request. Check runs are the individual CI/CD jobs and checks that run on the PR. + 10. get_reviewers - Get the list of requested reviewers (users and teams) for a pull request who have not yet submitted a review. + 11. get_status_checks - Get a unified view of all status checks for a pull request, combining legacy commit statuses and modern check runs. `, - Enum: []any{"get", "get_diff", "get_status", "get_files", "get_commits", "get_review_comments", "get_reviews", "get_comments", "get_check_runs"}, + Enum: []any{"get", "get_diff", "get_status", "get_files", "get_commits", "get_review_comments", "get_reviews", "get_comments", "get_check_runs", "get_reviewers", "get_status_checks"}, }, "owner": { Type: "string", @@ -154,6 +156,12 @@ Possible options: case "get_check_runs": result, err := GetPullRequestCheckRuns(ctx, client, owner, repo, pullNumber, pagination) return attachIFC(result), nil, err + case "get_reviewers": + result, err := GetPullRequestReviewers(ctx, client, owner, repo, pullNumber) + return attachIFC(result), nil, err + case "get_status_checks": + result, err := GetPullRequestStatusChecks(ctx, client, owner, repo, pullNumber) + return attachIFC(result), nil, err default: return utils.NewToolResultError(fmt.Sprintf("unknown method: %s", method)), nil, nil } @@ -380,6 +388,117 @@ func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner, return utils.NewToolResultText(string(r)), nil } +func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { + reviewers, resp, err := client.PullRequests.ListReviewers(ctx, owner, repo, pullNumber) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request reviewers", resp, err), nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request reviewers", resp, body), nil + } + + result := MinimalPRReviewers{} + for _, u := range reviewers.Users { + result.Users = append(result.Users, MinimalReviewerUser{ + Login: u.GetLogin(), + HTMLURL: u.GetHTMLURL(), + }) + } + for _, t := range reviewers.Teams { + result.Teams = append(result.Teams, MinimalReviewerTeam{ + Slug: t.GetSlug(), + Name: t.GetName(), + HTMLURL: t.GetHTMLURL(), + }) + } + + r, err := json.Marshal(result) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + return utils.NewToolResultText(string(r)), nil +} + +func GetPullRequestStatusChecks(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { + pr, resp, err := client.PullRequests.Get(ctx, owner, repo, pullNumber) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request", resp, err), nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request", resp, body), nil + } + + sha := pr.GetHead().GetSHA() + + combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, nil) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get combined status", resp, body), nil + } + + checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, nil) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("failed to read response body: %w", err) + } + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get check runs", resp, body), nil + } + + result := MinimalStatusChecks{ + CombinedState: combinedStatus.GetState(), + } + + for _, s := range combinedStatus.Statuses { + ms := MinimalCommitStatus{ + State: s.GetState(), + Context: s.GetContext(), + Description: s.GetDescription(), + TargetURL: s.GetTargetURL(), + } + result.Statuses = append(result.Statuses, ms) + } + + for _, cr := range checkRuns.CheckRuns { + result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr)) + } + + result.TotalCount = len(result.Statuses) + len(result.CheckRuns) + + r, err := json.Marshal(result) + if err != nil { + return nil, fmt.Errorf("failed to marshal response: %w", err) + } + + return utils.NewToolResultText(string(r)), nil +} + func GetPullRequestFiles(ctx context.Context, client *github.Client, deps ToolDependencies, owner, repo string, pullNumber int, pagination PaginationParams) (*mcp.CallToolResult, error) { if restricted, err := enforcePullRequestLockdown(ctx, client, deps, owner, repo, pullNumber); restricted != nil || err != nil { return restricted, err diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index ace47c666b..5964a90f33 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -1873,6 +1873,265 @@ func Test_GetPullRequestCheckRuns(t *testing.T) { } } +func Test_GetPullRequestReviewers(t *testing.T) { + // Verify tool definition once + serverTool := PullRequestRead(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + mockReviewers := &github.Reviewers{ + Users: []*github.User{ + {Login: github.Ptr("octocat"), HTMLURL: github.Ptr("https://github.com/octocat")}, + }, + Teams: []*github.Team{ + {Slug: github.Ptr("core-team"), Name: github.Ptr("Core Team"), HTMLURL: github.Ptr("https://github.com/orgs/owner/teams/core-team")}, + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedReviewers *github.Reviewers + expectedErrMsg string + }{ + { + name: "successful reviewers fetch", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsRequestedReviewersByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockReviewers), + }), + requestArgs: map[string]any{ + "method": "get_reviewers", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectError: false, + expectedReviewers: mockReviewers, + }, + { + name: "reviewers fetch fails", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsRequestedReviewersByOwnerByRepoByPullNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + }), + requestArgs: map[string]any{ + "method": "get_reviewers", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(999), + }, + expectError: true, + expectedErrMsg: "failed to get pull request reviewers", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + serverTool := PullRequestRead(translations.NullTranslationHelper) + deps := BaseDeps{ + Client: client, + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectError { + require.NoError(t, err) + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + return + } + + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + + var returned MinimalPRReviewers + err = json.Unmarshal([]byte(textContent.Text), &returned) + require.NoError(t, err) + + require.Len(t, returned.Users, len(tc.expectedReviewers.Users)) + for i, u := range returned.Users { + assert.Equal(t, tc.expectedReviewers.Users[i].GetLogin(), u.Login) + assert.Equal(t, tc.expectedReviewers.Users[i].GetHTMLURL(), u.HTMLURL) + } + require.Len(t, returned.Teams, len(tc.expectedReviewers.Teams)) + for i, tm := range returned.Teams { + assert.Equal(t, tc.expectedReviewers.Teams[i].GetSlug(), tm.Slug) + assert.Equal(t, tc.expectedReviewers.Teams[i].GetName(), tm.Name) + assert.Equal(t, tc.expectedReviewers.Teams[i].GetHTMLURL(), tm.HTMLURL) + } + }) + } +} + +func Test_GetPullRequestStatusChecks(t *testing.T) { + // Verify tool definition once + serverTool := PullRequestRead(translations.NullTranslationHelper) + tool := serverTool.Tool + require.NoError(t, toolsnaps.Test(tool.Name, tool)) + + mockPR := &github.PullRequest{ + Number: github.Ptr(42), + Head: &github.PullRequestBranch{ + SHA: github.Ptr("abcd1234"), + Ref: github.Ptr("feature-branch"), + }, + } + + mockCombinedStatus := &github.CombinedStatus{ + State: github.Ptr("success"), + Statuses: []*github.RepoStatus{ + { + State: github.Ptr("success"), + Context: github.Ptr("atlantis/plan"), + Description: github.Ptr("Plan succeeded"), + TargetURL: github.Ptr("https://atlantis.example.com/plan"), + }, + }, + } + + mockCheckRuns := &github.ListCheckRunsResults{ + Total: github.Ptr(1), + CheckRuns: []*github.CheckRun{ + { + ID: github.Ptr(int64(1)), + Name: github.Ptr("build"), + Status: github.Ptr("completed"), + Conclusion: github.Ptr("success"), + }, + }, + } + + tests := []struct { + name string + mockedClient *http.Client + requestArgs map[string]any + expectError bool + expectedErrMsg string + }{ + { + name: "successful status checks fetch", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + GetReposCommitsStatusByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCombinedStatus), + GetReposCommitsCheckRunsByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCheckRuns), + }), + requestArgs: map[string]any{ + "method": "get_status_checks", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectError: false, + }, + { + name: "PR fetch fails", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message": "Not Found"}`)) + }), + }), + requestArgs: map[string]any{ + "method": "get_status_checks", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(999), + }, + expectError: true, + expectedErrMsg: "failed to get pull request", + }, + { + name: "combined status fetch fails", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + GetReposCommitsStatusByOwnerByRepoByRef: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message": "Internal Server Error"}`)) + }), + }), + requestArgs: map[string]any{ + "method": "get_status_checks", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectError: true, + expectedErrMsg: "failed to get combined status", + }, + { + name: "check runs fetch fails", + mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + GetReposCommitsStatusByOwnerByRepoByRef: mockResponse(t, http.StatusOK, mockCombinedStatus), + GetReposCommitsCheckRunsByOwnerByRepoByRef: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(`{"message": "Internal Server Error"}`)) + }), + }), + requestArgs: map[string]any{ + "method": "get_status_checks", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }, + expectError: true, + expectedErrMsg: "failed to get check runs", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + client := mustNewGHClient(t, tc.mockedClient) + serverTool := PullRequestRead(translations.NullTranslationHelper) + deps := BaseDeps{ + Client: client, + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(tc.requestArgs) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + + if tc.expectError { + require.NoError(t, err) + require.True(t, result.IsError) + errorContent := getErrorResult(t, result) + assert.Contains(t, errorContent.Text, tc.expectedErrMsg) + return + } + + require.NoError(t, err) + require.False(t, result.IsError) + + textContent := getTextResult(t, result) + + var returned MinimalStatusChecks + err = json.Unmarshal([]byte(textContent.Text), &returned) + require.NoError(t, err) + + assert.Equal(t, mockCombinedStatus.GetState(), returned.CombinedState) + require.Len(t, returned.Statuses, len(mockCombinedStatus.Statuses)) + assert.Equal(t, mockCombinedStatus.Statuses[0].GetContext(), returned.Statuses[0].Context) + require.Len(t, returned.CheckRuns, len(mockCheckRuns.CheckRuns)) + assert.Equal(t, mockCheckRuns.CheckRuns[0].GetName(), returned.CheckRuns[0].Name) + assert.Equal(t, len(mockCombinedStatus.Statuses)+len(mockCheckRuns.CheckRuns), returned.TotalCount) + }) + } +} + func Test_UpdatePullRequestBranch(t *testing.T) { // Verify tool definition once serverTool := UpdatePullRequestBranch(translations.NullTranslationHelper) From e2ca49bfba39d202540e7a6d224e78dbca55ed8f Mon Sep 17 00:00:00 2001 From: salakonrad Date: Thu, 16 Jul 2026 13:47:39 +0200 Subject: [PATCH 2/2] fix: address review feedback on pull_request_read status checks Resolve Copilot review comments on get_status_checks: - Fix response body leak: deferred closures captured the reused `resp` variable, so earlier bodies were never closed. Add a closeStatusResponse helper that closes each body synchronously. - Paginate GetCombinedStatus and ListCheckRunsForRef so the unified view includes all statuses and check runs, not just the first page. - Collapse the duplicated status-error handling into the shared helper. --- pkg/github/pullrequests.go | 110 ++++++++++++++++++-------------- pkg/github/pullrequests_test.go | 79 +++++++++++++++++++++++ 2 files changed, 142 insertions(+), 47 deletions(-) diff --git a/pkg/github/pullrequests.go b/pkg/github/pullrequests.go index d302a4ad7a..14f3a46283 100644 --- a/pkg/github/pullrequests.go +++ b/pkg/github/pullrequests.go @@ -388,11 +388,12 @@ func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner, return utils.NewToolResultText(string(r)), nil } -func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { - reviewers, resp, err := client.PullRequests.ListReviewers(ctx, owner, repo, pullNumber) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request reviewers", resp, err), nil - } +// closeStatusResponse closes the response body and, for any non-200 status, +// returns a tool error result. It returns (nil, nil) when the response is OK so +// the caller can proceed. Closing the body synchronously here (rather than via a +// deferred closure over a reused resp variable) avoids leaking earlier response +// bodies when several API calls are made in sequence. +func closeStatusResponse(ctx context.Context, resp *github.Response, message string) (*mcp.CallToolResult, error) { defer func() { _ = resp.Body.Close() }() if resp.StatusCode != http.StatusOK { @@ -400,7 +401,19 @@ func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, if err != nil { return nil, fmt.Errorf("failed to read response body: %w", err) } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request reviewers", resp, body), nil + return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, message, resp, body), nil + } + + return nil, nil +} + +func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) { + reviewers, resp, err := client.PullRequests.ListReviewers(ctx, owner, repo, pullNumber) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request reviewers", resp, err), nil + } + if errResult, err := closeStatusResponse(ctx, resp, "failed to get pull request reviewers"); err != nil || errResult != nil { + return errResult, err } result := MinimalPRReviewers{} @@ -431,62 +444,65 @@ func GetPullRequestStatusChecks(ctx context.Context, client *github.Client, owne if err != nil { return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request", resp, err), nil } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) - } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request", resp, body), nil + if errResult, err := closeStatusResponse(ctx, resp, "failed to get pull request"); err != nil || errResult != nil { + return errResult, err } sha := pr.GetHead().GetSHA() - combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, nil) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil - } - defer func() { _ = resp.Body.Close() }() + result := MinimalStatusChecks{} - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) + // Page through the combined commit statuses so the unified view is complete + // rather than limited to the first page. + statusOpts := &github.ListOptions{PerPage: 100} + for { + combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, statusOpts) if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil + } + if errResult, err := closeStatusResponse(ctx, resp, "failed to get combined status"); err != nil || errResult != nil { + return errResult, err } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get combined status", resp, body), nil - } - checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, nil) - if err != nil { - return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil - } - defer func() { _ = resp.Body.Close() }() + // The combined state is identical across pages; capture it once. + if result.CombinedState == "" { + result.CombinedState = combinedStatus.GetState() + } - if resp.StatusCode != http.StatusOK { - body, err := io.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("failed to read response body: %w", err) + for _, s := range combinedStatus.Statuses { + result.Statuses = append(result.Statuses, MinimalCommitStatus{ + State: s.GetState(), + Context: s.GetContext(), + Description: s.GetDescription(), + TargetURL: s.GetTargetURL(), + }) } - return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get check runs", resp, body), nil - } - result := MinimalStatusChecks{ - CombinedState: combinedStatus.GetState(), + if resp.NextPage == 0 { + break + } + statusOpts.Page = resp.NextPage } - for _, s := range combinedStatus.Statuses { - ms := MinimalCommitStatus{ - State: s.GetState(), - Context: s.GetContext(), - Description: s.GetDescription(), - TargetURL: s.GetTargetURL(), + // Page through the check runs for the same reason. + checkOpts := &github.ListCheckRunsOptions{ListOptions: github.ListOptions{PerPage: 100}} + for { + checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, checkOpts) + if err != nil { + return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil + } + if errResult, err := closeStatusResponse(ctx, resp, "failed to get check runs"); err != nil || errResult != nil { + return errResult, err } - result.Statuses = append(result.Statuses, ms) - } - for _, cr := range checkRuns.CheckRuns { - result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr)) + for _, cr := range checkRuns.CheckRuns { + result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr)) + } + + if resp.NextPage == 0 { + break + } + checkOpts.Page = resp.NextPage } result.TotalCount = len(result.Statuses) + len(result.CheckRuns) diff --git a/pkg/github/pullrequests_test.go b/pkg/github/pullrequests_test.go index 5964a90f33..3bc89c0889 100644 --- a/pkg/github/pullrequests_test.go +++ b/pkg/github/pullrequests_test.go @@ -2132,6 +2132,85 @@ func Test_GetPullRequestStatusChecks(t *testing.T) { } } +func Test_GetPullRequestStatusChecks_Pagination(t *testing.T) { + mockPR := &github.PullRequest{ + Number: github.Ptr(42), + Head: &github.PullRequestBranch{SHA: github.Ptr("abcd1234")}, + } + + // Combined status served across two pages. + statusPages := map[string]*github.CombinedStatus{ + "": { + State: github.Ptr("pending"), + Statuses: []*github.RepoStatus{{State: github.Ptr("success"), Context: github.Ptr("ci/one")}}, + }, + "2": { + State: github.Ptr("pending"), + Statuses: []*github.RepoStatus{{State: github.Ptr("pending"), Context: github.Ptr("ci/two")}}, + }, + } + // Check runs served across two pages. + checkPages := map[string]*github.ListCheckRunsResults{ + "": {Total: github.Ptr(2), CheckRuns: []*github.CheckRun{{Name: github.Ptr("build")}}}, + "2": {Total: github.Ptr(2), CheckRuns: []*github.CheckRun{{Name: github.Ptr("test")}}}, + } + + // paginatedHandler returns page 1 with a Link rel="next" header pointing at + // page 2, and page 2 without a Link header, mirroring the GitHub REST API. + paginatedHandler := func(pages map[string]any) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + page := r.URL.Query().Get("page") + if page == "" { + next := *r.URL + q := next.Query() + q.Set("page", "2") + next.RawQuery = q.Encode() + w.Header().Set("Link", "; rel=\"next\"") + } + w.WriteHeader(http.StatusOK) + b, _ := json.Marshal(pages[page]) + _, _ = w.Write(b) + } + } + + client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{ + GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR), + GetReposCommitsStatusByOwnerByRepoByRef: paginatedHandler(map[string]any{ + "": statusPages[""], "2": statusPages["2"], + }), + GetReposCommitsCheckRunsByOwnerByRepoByRef: paginatedHandler(map[string]any{ + "": checkPages[""], "2": checkPages["2"], + }), + })) + + serverTool := PullRequestRead(translations.NullTranslationHelper) + deps := BaseDeps{ + Client: client, + RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute), + Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}), + } + handler := serverTool.Handler(deps) + + request := createMCPRequest(map[string]any{ + "method": "get_status_checks", + "owner": "owner", + "repo": "repo", + "pullNumber": float64(42), + }) + result, err := handler(ContextWithDeps(context.Background(), deps), &request) + require.NoError(t, err) + require.False(t, result.IsError) + + var returned MinimalStatusChecks + require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned)) + + // Both pages of statuses and check runs must be aggregated. + require.Len(t, returned.Statuses, 2) + require.Len(t, returned.CheckRuns, 2) + assert.Equal(t, 4, returned.TotalCount) + assert.Equal(t, "pending", returned.CombinedState) +} + func Test_UpdatePullRequestBranch(t *testing.T) { // Verify tool definition once serverTool := UpdatePullRequestBranch(translations.NullTranslationHelper)