From 70d9815bf57c9a53a29c4b71fbe25dc5258264b3 Mon Sep 17 00:00:00 2001 From: claw Date: Tue, 12 May 2026 21:36:45 -0700 Subject: [PATCH] fix: address review feedback on PR #102 - Separate maxPages into maxFilesPages and maxCheckRunPages constants for clarity (sonnet MINOR #1) - Add parallel to CheckRunConclusions subtests (sonnet MINOR #2) - Add TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed test covering check-runs 500 after statuses succeed (sonnet MINOR #2) - Expand mapCheckRunStatus doc comment with full mapping rules including cancelled/skipped/neutral rationale and unknown value behavior (sonnet MINOR #3, gpt MINOR #1) - Expand GetPullRequest doc comment to mention error types returned (sonnet NIT #4) - Add inline comment on Description field clarifying it holds raw conclusion value (gpt NIT #3) --- github/pr.go | 33 +++++++++++++++++++++++++-------- github/pr_test.go | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/github/pr.go b/github/pr.go index e9bea5a..c028506 100644 --- a/github/pr.go +++ b/github/pr.go @@ -51,7 +51,10 @@ type checkRunsResponse struct { } `json:"check_runs"` } -// GetPullRequest fetches PR metadata. +// GetPullRequest fetches PR metadata from the GitHub API. +// Returns an *APIError wrapping the HTTP status on non-2xx responses (e.g. +// IsNotFound for 404, IsUnauthorized for 401). Network and context errors +// are wrapped but not typed as *APIError. func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcs.PullRequest, error) { reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) body, err := c.doGet(ctx, reqURL) @@ -82,9 +85,15 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num return string(body), nil } -// maxPages is the upper bound on pagination loops to prevent unbounded iteration -// in case the server returns a full page indefinitely. -const maxPages = 100 +const ( + // maxFilesPages is the upper bound on pagination loops for PR file listing, + // preventing unbounded iteration if the server always returns a full page. + maxFilesPages = 100 + + // maxCheckRunPages is the upper bound on pagination loops for check-run listing, + // preventing unbounded iteration if the server always returns a full page. + maxCheckRunPages = 100 +) // GetPullRequestFiles fetches the list of files changed in a PR. // Paginates through all pages (100 per page) to collect all files. @@ -93,7 +102,7 @@ const maxPages = 100 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) { var allFiles []vcs.ChangedFile - for page := 1; page <= maxPages; page++ { + for page := 1; page <= maxFilesPages; page++ { reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page) body, err := c.doGet(ctx, reqURL) @@ -154,7 +163,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) } // Fetch check runs (paginated) - for checkPage := 1; checkPage <= maxPages; checkPage++ { + for checkPage := 1; checkPage <= maxCheckRunPages; checkPage++ { checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage) checkBody, err := c.doGet(ctx, checkURL) @@ -169,7 +178,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) result = append(result, vcs.CommitStatus{ Context: cr.Name, Status: mapCheckRunStatus(cr.Conclusion), - Description: derefString(cr.Conclusion), + Description: derefString(cr.Conclusion), // raw conclusion value (e.g. "success", "failure", "skipped") TargetURL: cr.HTMLURL, }) } @@ -181,9 +190,17 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) return result, nil } -// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string. +// mapCheckRunStatus maps a GitHub check run conclusion to a vcs.CommitStatus status string. // Conclusion alone determines the mapped state: nil conclusion means the run is // still in progress (pending), regardless of the status field value. +// +// Mapping rules: +// - nil → "pending" (run still in progress or queued) +// - "success" → "success" +// - "failure", "action_required", "timed_out" → "failure" +// - "cancelled", "skipped", "neutral" → "success" (non-blocking per GitHub check suite semantics) +// - "stale", "waiting" → "pending" +// - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete) func mapCheckRunStatus(conclusion *string) string { if conclusion == nil { // Still running or queued diff --git a/github/pr_test.go b/github/pr_test.go index 0e05a50..f79147b 100644 --- a/github/pr_test.go +++ b/github/pr_test.go @@ -545,6 +545,7 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) { name = *tt.conclusion } t.Run(name, func(t *testing.T) { + t.Parallel() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if strings.Contains(r.URL.Path, "/status") { json.NewEncoder(w).Encode(map[string]interface{}{ @@ -632,6 +633,44 @@ func TestGetCommitStatuses_MalformedJSON(t *testing.T) { } } +func TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.Contains(r.URL.Path, "/status"): + // Statuses succeed + json.NewEncoder(w).Encode(map[string]interface{}{ + "state": "success", + "statuses": []map[string]string{ + { + "context": "ci/build", + "state": "success", + "description": "Build passed", + "target_url": "https://ci.example.com/1", + }, + }, + }) + case strings.Contains(r.URL.Path, "/check-runs"): + // Check runs fail with 500 + w.WriteHeader(500) + w.Write([]byte(`{"message":"Internal Server Error"}`)) + default: + w.WriteHeader(404) + } + })) + defer srv.Close() + + c := NewClient("token", srv.URL, AllowInsecureHTTP()) + c.SetHTTPClient(srv.Client()) + + _, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123") + if err == nil { + t.Fatal("expected error when check-runs endpoint fails after statuses succeed") + } + if !strings.Contains(err.Error(), "fetch check runs") { + t.Errorf("expected check runs error, got: %v", err) + } +} + func stringPtr(s string) *string { return &s }