Skip to content

Commit 2403b0a

Browse files
salakonradsalakonrad
committed
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.
1 parent 7c277bf commit 2403b0a

2 files changed

Lines changed: 142 additions & 47 deletions

File tree

‎pkg/github/pullrequests.go‎

Lines changed: 63 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -366,19 +366,32 @@ func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner,
366366
return utils.NewToolResultText(string(r)), nil
367367
}
368368

369-
func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) {
370-
reviewers, resp, err := client.PullRequests.ListReviewers(ctx, owner, repo, pullNumber)
371-
if err != nil {
372-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request reviewers", resp, err), nil
373-
}
369+
// closeStatusResponse closes the response body and, for any non-200 status,
370+
// returns a tool error result. It returns (nil, nil) when the response is OK so
371+
// the caller can proceed. Closing the body synchronously here (rather than via a
372+
// deferred closure over a reused resp variable) avoids leaking earlier response
373+
// bodies when several API calls are made in sequence.
374+
func closeStatusResponse(ctx context.Context, resp *github.Response, message string) (*mcp.CallToolResult, error) {
374375
defer func() { _ = resp.Body.Close() }()
375376

376377
if resp.StatusCode != http.StatusOK {
377378
body, err := io.ReadAll(resp.Body)
378379
if err != nil {
379380
return nil, fmt.Errorf("failed to read response body: %w", err)
380381
}
381-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request reviewers", resp, body), nil
382+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, message, resp, body), nil
383+
}
384+
385+
return nil, nil
386+
}
387+
388+
func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) {
389+
reviewers, resp, err := client.PullRequests.ListReviewers(ctx, owner, repo, pullNumber)
390+
if err != nil {
391+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request reviewers", resp, err), nil
392+
}
393+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get pull request reviewers"); err != nil || errResult != nil {
394+
return errResult, err
382395
}
383396

384397
result := MinimalPRReviewers{}
@@ -409,62 +422,65 @@ func GetPullRequestStatusChecks(ctx context.Context, client *github.Client, owne
409422
if err != nil {
410423
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request", resp, err), nil
411424
}
412-
defer func() { _ = resp.Body.Close() }()
413-
414-
if resp.StatusCode != http.StatusOK {
415-
body, err := io.ReadAll(resp.Body)
416-
if err != nil {
417-
return nil, fmt.Errorf("failed to read response body: %w", err)
418-
}
419-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request", resp, body), nil
425+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get pull request"); err != nil || errResult != nil {
426+
return errResult, err
420427
}
421428

422429
sha := pr.GetHead().GetSHA()
423430

424-
combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, nil)
425-
if err != nil {
426-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil
427-
}
428-
defer func() { _ = resp.Body.Close() }()
431+
result := MinimalStatusChecks{}
429432

430-
if resp.StatusCode != http.StatusOK {
431-
body, err := io.ReadAll(resp.Body)
433+
// Page through the combined commit statuses so the unified view is complete
434+
// rather than limited to the first page.
435+
statusOpts := &github.ListOptions{PerPage: 100}
436+
for {
437+
combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, statusOpts)
432438
if err != nil {
433-
return nil, fmt.Errorf("failed to read response body: %w", err)
439+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil
440+
}
441+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get combined status"); err != nil || errResult != nil {
442+
return errResult, err
434443
}
435-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get combined status", resp, body), nil
436-
}
437444

438-
checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, nil)
439-
if err != nil {
440-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil
441-
}
442-
defer func() { _ = resp.Body.Close() }()
445+
// The combined state is identical across pages; capture it once.
446+
if result.CombinedState == "" {
447+
result.CombinedState = combinedStatus.GetState()
448+
}
443449

444-
if resp.StatusCode != http.StatusOK {
445-
body, err := io.ReadAll(resp.Body)
446-
if err != nil {
447-
return nil, fmt.Errorf("failed to read response body: %w", err)
450+
for _, s := range combinedStatus.Statuses {
451+
result.Statuses = append(result.Statuses, MinimalCommitStatus{
452+
State: s.GetState(),
453+
Context: s.GetContext(),
454+
Description: s.GetDescription(),
455+
TargetURL: s.GetTargetURL(),
456+
})
448457
}
449-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get check runs", resp, body), nil
450-
}
451458

452-
result := MinimalStatusChecks{
453-
CombinedState: combinedStatus.GetState(),
459+
if resp.NextPage == 0 {
460+
break
461+
}
462+
statusOpts.Page = resp.NextPage
454463
}
455464

456-
for _, s := range combinedStatus.Statuses {
457-
ms := MinimalCommitStatus{
458-
State: s.GetState(),
459-
Context: s.GetContext(),
460-
Description: s.GetDescription(),
461-
TargetURL: s.GetTargetURL(),
465+
// Page through the check runs for the same reason.
466+
checkOpts := &github.ListCheckRunsOptions{ListOptions: github.ListOptions{PerPage: 100}}
467+
for {
468+
checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, checkOpts)
469+
if err != nil {
470+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil
471+
}
472+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get check runs"); err != nil || errResult != nil {
473+
return errResult, err
462474
}
463-
result.Statuses = append(result.Statuses, ms)
464-
}
465475

466-
for _, cr := range checkRuns.CheckRuns {
467-
result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr))
476+
for _, cr := range checkRuns.CheckRuns {
477+
result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr))
478+
}
479+
480+
if resp.NextPage == 0 {
481+
break
482+
}
483+
checkOpts.Page = resp.NextPage
468484
}
469485

470486
result.TotalCount = len(result.Statuses) + len(result.CheckRuns)

‎pkg/github/pullrequests_test.go‎

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2025,6 +2025,85 @@ func Test_GetPullRequestStatusChecks(t *testing.T) {
20252025
}
20262026
}
20272027

2028+
func Test_GetPullRequestStatusChecks_Pagination(t *testing.T) {
2029+
mockPR := &github.PullRequest{
2030+
Number: github.Ptr(42),
2031+
Head: &github.PullRequestBranch{SHA: github.Ptr("abcd1234")},
2032+
}
2033+
2034+
// Combined status served across two pages.
2035+
statusPages := map[string]*github.CombinedStatus{
2036+
"": {
2037+
State: github.Ptr("pending"),
2038+
Statuses: []*github.RepoStatus{{State: github.Ptr("success"), Context: github.Ptr("ci/one")}},
2039+
},
2040+
"2": {
2041+
State: github.Ptr("pending"),
2042+
Statuses: []*github.RepoStatus{{State: github.Ptr("pending"), Context: github.Ptr("ci/two")}},
2043+
},
2044+
}
2045+
// Check runs served across two pages.
2046+
checkPages := map[string]*github.ListCheckRunsResults{
2047+
"": {Total: github.Ptr(2), CheckRuns: []*github.CheckRun{{Name: github.Ptr("build")}}},
2048+
"2": {Total: github.Ptr(2), CheckRuns: []*github.CheckRun{{Name: github.Ptr("test")}}},
2049+
}
2050+
2051+
// paginatedHandler returns page 1 with a Link rel="next" header pointing at
2052+
// page 2, and page 2 without a Link header, mirroring the GitHub REST API.
2053+
paginatedHandler := func(pages map[string]any) http.HandlerFunc {
2054+
return func(w http.ResponseWriter, r *http.Request) {
2055+
page := r.URL.Query().Get("page")
2056+
if page == "" {
2057+
next := *r.URL
2058+
q := next.Query()
2059+
q.Set("page", "2")
2060+
next.RawQuery = q.Encode()
2061+
w.Header().Set("Link", "<https://api.github.com"+next.RequestURI()+">; rel=\"next\"")
2062+
}
2063+
w.WriteHeader(http.StatusOK)
2064+
b, _ := json.Marshal(pages[page])
2065+
_, _ = w.Write(b)
2066+
}
2067+
}
2068+
2069+
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
2070+
GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR),
2071+
GetReposCommitsStatusByOwnerByRepoByRef: paginatedHandler(map[string]any{
2072+
"": statusPages[""], "2": statusPages["2"],
2073+
}),
2074+
GetReposCommitsCheckRunsByOwnerByRepoByRef: paginatedHandler(map[string]any{
2075+
"": checkPages[""], "2": checkPages["2"],
2076+
}),
2077+
}))
2078+
2079+
serverTool := PullRequestRead(translations.NullTranslationHelper)
2080+
deps := BaseDeps{
2081+
Client: client,
2082+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
2083+
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
2084+
}
2085+
handler := serverTool.Handler(deps)
2086+
2087+
request := createMCPRequest(map[string]any{
2088+
"method": "get_status_checks",
2089+
"owner": "owner",
2090+
"repo": "repo",
2091+
"pullNumber": float64(42),
2092+
})
2093+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
2094+
require.NoError(t, err)
2095+
require.False(t, result.IsError)
2096+
2097+
var returned MinimalStatusChecks
2098+
require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned))
2099+
2100+
// Both pages of statuses and check runs must be aggregated.
2101+
require.Len(t, returned.Statuses, 2)
2102+
require.Len(t, returned.CheckRuns, 2)
2103+
assert.Equal(t, 4, returned.TotalCount)
2104+
assert.Equal(t, "pending", returned.CombinedState)
2105+
}
2106+
20282107
func Test_UpdatePullRequestBranch(t *testing.T) {
20292108
// Verify tool definition once
20302109
serverTool := UpdatePullRequestBranch(translations.NullTranslationHelper)

0 commit comments

Comments
 (0)