Skip to content

Commit e2ca49b

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 cc5b856 commit e2ca49b

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
@@ -388,19 +388,32 @@ func GetPullRequestCheckRuns(ctx context.Context, client *github.Client, owner,
388388
return utils.NewToolResultText(string(r)), nil
389389
}
390390

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

398399
if resp.StatusCode != http.StatusOK {
399400
body, err := io.ReadAll(resp.Body)
400401
if err != nil {
401402
return nil, fmt.Errorf("failed to read response body: %w", err)
402403
}
403-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request reviewers", resp, body), nil
404+
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, message, resp, body), nil
405+
}
406+
407+
return nil, nil
408+
}
409+
410+
func GetPullRequestReviewers(ctx context.Context, client *github.Client, owner, repo string, pullNumber int) (*mcp.CallToolResult, error) {
411+
reviewers, resp, err := client.PullRequests.ListReviewers(ctx, owner, repo, pullNumber)
412+
if err != nil {
413+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request reviewers", resp, err), nil
414+
}
415+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get pull request reviewers"); err != nil || errResult != nil {
416+
return errResult, err
404417
}
405418

406419
result := MinimalPRReviewers{}
@@ -431,62 +444,65 @@ func GetPullRequestStatusChecks(ctx context.Context, client *github.Client, owne
431444
if err != nil {
432445
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get pull request", resp, err), nil
433446
}
434-
defer func() { _ = resp.Body.Close() }()
435-
436-
if resp.StatusCode != http.StatusOK {
437-
body, err := io.ReadAll(resp.Body)
438-
if err != nil {
439-
return nil, fmt.Errorf("failed to read response body: %w", err)
440-
}
441-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get pull request", resp, body), nil
447+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get pull request"); err != nil || errResult != nil {
448+
return errResult, err
442449
}
443450

444451
sha := pr.GetHead().GetSHA()
445452

446-
combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, nil)
447-
if err != nil {
448-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil
449-
}
450-
defer func() { _ = resp.Body.Close() }()
453+
result := MinimalStatusChecks{}
451454

452-
if resp.StatusCode != http.StatusOK {
453-
body, err := io.ReadAll(resp.Body)
455+
// Page through the combined commit statuses so the unified view is complete
456+
// rather than limited to the first page.
457+
statusOpts := &github.ListOptions{PerPage: 100}
458+
for {
459+
combinedStatus, resp, err := client.Repositories.GetCombinedStatus(ctx, owner, repo, sha, statusOpts)
454460
if err != nil {
455-
return nil, fmt.Errorf("failed to read response body: %w", err)
461+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get combined status", resp, err), nil
462+
}
463+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get combined status"); err != nil || errResult != nil {
464+
return errResult, err
456465
}
457-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get combined status", resp, body), nil
458-
}
459466

460-
checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, nil)
461-
if err != nil {
462-
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil
463-
}
464-
defer func() { _ = resp.Body.Close() }()
467+
// The combined state is identical across pages; capture it once.
468+
if result.CombinedState == "" {
469+
result.CombinedState = combinedStatus.GetState()
470+
}
465471

466-
if resp.StatusCode != http.StatusOK {
467-
body, err := io.ReadAll(resp.Body)
468-
if err != nil {
469-
return nil, fmt.Errorf("failed to read response body: %w", err)
472+
for _, s := range combinedStatus.Statuses {
473+
result.Statuses = append(result.Statuses, MinimalCommitStatus{
474+
State: s.GetState(),
475+
Context: s.GetContext(),
476+
Description: s.GetDescription(),
477+
TargetURL: s.GetTargetURL(),
478+
})
470479
}
471-
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to get check runs", resp, body), nil
472-
}
473480

474-
result := MinimalStatusChecks{
475-
CombinedState: combinedStatus.GetState(),
481+
if resp.NextPage == 0 {
482+
break
483+
}
484+
statusOpts.Page = resp.NextPage
476485
}
477486

478-
for _, s := range combinedStatus.Statuses {
479-
ms := MinimalCommitStatus{
480-
State: s.GetState(),
481-
Context: s.GetContext(),
482-
Description: s.GetDescription(),
483-
TargetURL: s.GetTargetURL(),
487+
// Page through the check runs for the same reason.
488+
checkOpts := &github.ListCheckRunsOptions{ListOptions: github.ListOptions{PerPage: 100}}
489+
for {
490+
checkRuns, resp, err := client.Checks.ListCheckRunsForRef(ctx, owner, repo, sha, checkOpts)
491+
if err != nil {
492+
return ghErrors.NewGitHubAPIErrorResponse(ctx, "failed to get check runs", resp, err), nil
493+
}
494+
if errResult, err := closeStatusResponse(ctx, resp, "failed to get check runs"); err != nil || errResult != nil {
495+
return errResult, err
484496
}
485-
result.Statuses = append(result.Statuses, ms)
486-
}
487497

488-
for _, cr := range checkRuns.CheckRuns {
489-
result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr))
498+
for _, cr := range checkRuns.CheckRuns {
499+
result.CheckRuns = append(result.CheckRuns, convertToMinimalCheckRun(cr))
500+
}
501+
502+
if resp.NextPage == 0 {
503+
break
504+
}
505+
checkOpts.Page = resp.NextPage
490506
}
491507

492508
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
@@ -2132,6 +2132,85 @@ func Test_GetPullRequestStatusChecks(t *testing.T) {
21322132
}
21332133
}
21342134

2135+
func Test_GetPullRequestStatusChecks_Pagination(t *testing.T) {
2136+
mockPR := &github.PullRequest{
2137+
Number: github.Ptr(42),
2138+
Head: &github.PullRequestBranch{SHA: github.Ptr("abcd1234")},
2139+
}
2140+
2141+
// Combined status served across two pages.
2142+
statusPages := map[string]*github.CombinedStatus{
2143+
"": {
2144+
State: github.Ptr("pending"),
2145+
Statuses: []*github.RepoStatus{{State: github.Ptr("success"), Context: github.Ptr("ci/one")}},
2146+
},
2147+
"2": {
2148+
State: github.Ptr("pending"),
2149+
Statuses: []*github.RepoStatus{{State: github.Ptr("pending"), Context: github.Ptr("ci/two")}},
2150+
},
2151+
}
2152+
// Check runs served across two pages.
2153+
checkPages := map[string]*github.ListCheckRunsResults{
2154+
"": {Total: github.Ptr(2), CheckRuns: []*github.CheckRun{{Name: github.Ptr("build")}}},
2155+
"2": {Total: github.Ptr(2), CheckRuns: []*github.CheckRun{{Name: github.Ptr("test")}}},
2156+
}
2157+
2158+
// paginatedHandler returns page 1 with a Link rel="next" header pointing at
2159+
// page 2, and page 2 without a Link header, mirroring the GitHub REST API.
2160+
paginatedHandler := func(pages map[string]any) http.HandlerFunc {
2161+
return func(w http.ResponseWriter, r *http.Request) {
2162+
page := r.URL.Query().Get("page")
2163+
if page == "" {
2164+
next := *r.URL
2165+
q := next.Query()
2166+
q.Set("page", "2")
2167+
next.RawQuery = q.Encode()
2168+
w.Header().Set("Link", "<https://api.github.com"+next.RequestURI()+">; rel=\"next\"")
2169+
}
2170+
w.WriteHeader(http.StatusOK)
2171+
b, _ := json.Marshal(pages[page])
2172+
_, _ = w.Write(b)
2173+
}
2174+
}
2175+
2176+
client := mustNewGHClient(t, MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
2177+
GetReposPullsByOwnerByRepoByPullNumber: mockResponse(t, http.StatusOK, mockPR),
2178+
GetReposCommitsStatusByOwnerByRepoByRef: paginatedHandler(map[string]any{
2179+
"": statusPages[""], "2": statusPages["2"],
2180+
}),
2181+
GetReposCommitsCheckRunsByOwnerByRepoByRef: paginatedHandler(map[string]any{
2182+
"": checkPages[""], "2": checkPages["2"],
2183+
}),
2184+
}))
2185+
2186+
serverTool := PullRequestRead(translations.NullTranslationHelper)
2187+
deps := BaseDeps{
2188+
Client: client,
2189+
RepoAccessCache: stubRepoAccessCache(nil, 5*time.Minute),
2190+
Flags: stubFeatureFlags(map[string]bool{"lockdown-mode": false}),
2191+
}
2192+
handler := serverTool.Handler(deps)
2193+
2194+
request := createMCPRequest(map[string]any{
2195+
"method": "get_status_checks",
2196+
"owner": "owner",
2197+
"repo": "repo",
2198+
"pullNumber": float64(42),
2199+
})
2200+
result, err := handler(ContextWithDeps(context.Background(), deps), &request)
2201+
require.NoError(t, err)
2202+
require.False(t, result.IsError)
2203+
2204+
var returned MinimalStatusChecks
2205+
require.NoError(t, json.Unmarshal([]byte(getTextResult(t, result).Text), &returned))
2206+
2207+
// Both pages of statuses and check runs must be aggregated.
2208+
require.Len(t, returned.Statuses, 2)
2209+
require.Len(t, returned.CheckRuns, 2)
2210+
assert.Equal(t, 4, returned.TotalCount)
2211+
assert.Equal(t, "pending", returned.CombinedState)
2212+
}
2213+
21352214
func Test_UpdatePullRequestBranch(t *testing.T) {
21362215
// Verify tool definition once
21372216
serverTool := UpdatePullRequestBranch(translations.NullTranslationHelper)

0 commit comments

Comments
 (0)