From 22b3ce8fefaa957e10fa7f5c458a0f9e6e675ca9 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 15:35:55 -0700 Subject: [PATCH 1/4] fix(github): consolidate review.go and identity.go into reviews.go (#116) Remove github/review.go and github/identity.go, replacing them with a consolidated github/reviews.go that: - Uses doJSONRequest for PostReview and DismissReview (cleaner than manual marshal + doRequestWithBody) - Adds paginated ListReviews with per_page=100 and max 100 pages - Consolidates GetAuthenticatedUser and userResponse type (previously duplicated in identity.go) - Preserves all sentinel errors (ErrCannotDeleteSubmittedReview, ErrConflictingCommitIDs), state translation, commit ID validation, and SupersedeReviews This prevents the redeclaration errors that occur when both review.go and reviews.go exist in the same package, as described in issue #116. Closes #116 --- github/identity.go | 29 --------- github/{review.go => reviews.go} | 101 +++++++++++++++++++++---------- 2 files changed, 68 insertions(+), 62 deletions(-) delete mode 100644 github/identity.go rename github/{review.go => reviews.go} (77%) diff --git a/github/identity.go b/github/identity.go deleted file mode 100644 index 46c26a8..0000000 --- a/github/identity.go +++ /dev/null @@ -1,29 +0,0 @@ -package github - -import ( - "context" - "encoding/json" - "fmt" -) - -// userResponse is the GitHub API response for the authenticated user. -type userResponse struct { - Login string `json:"login"` -} - -// GetAuthenticatedUser returns the login of the currently authenticated user. -func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { - reqURL := fmt.Sprintf("%s/user", c.baseURL) - - body, err := c.doGet(ctx, reqURL) - if err != nil { - return "", fmt.Errorf("get authenticated user: %w", err) - } - - var resp userResponse - if err := json.Unmarshal(body, &resp); err != nil { - return "", fmt.Errorf("parse user response: %w", err) - } - - return resp.Login, nil -} diff --git a/github/review.go b/github/reviews.go similarity index 77% rename from github/review.go rename to github/reviews.go index 1d043e5..6a129bd 100644 --- a/github/review.go +++ b/github/reviews.go @@ -5,12 +5,21 @@ import ( "encoding/json" "errors" "fmt" + "log/slog" "net/http" "net/url" "gitea.weiker.me/rodin/review-bot/vcs" ) +const ( + // reviewsPerPage is the number of reviews to fetch per API page. + reviewsPerPage = 100 + // maxReviewPages is the maximum number of pages to paginate through + // when listing reviews. Acts as a safeguard against infinite pagination. + maxReviewPages = 100 +) + // ErrCannotDeleteSubmittedReview is returned when DeleteReview is called on // a review that has already been submitted (APPROVED, REQUEST_CHANGES, COMMENT). // GitHub only allows deletion of PENDING reviews. Callers that need to replace @@ -54,6 +63,11 @@ type dismissReviewRequest struct { Event string `json:"event"` } +// userResponse is the GitHub API response for the authenticated user. +type userResponse struct { + Login string `json:"login"` +} + // translateGitHubReviewState translates a GitHub API review state to the // canonical vcs.Review.State value. func translateGitHubReviewState(state string) string { @@ -117,12 +131,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, }) } - data, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("marshal review request: %w", err) - } - - body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) + body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload) if err != nil { return nil, fmt.Errorf("post review: %w", err) } @@ -141,33 +150,51 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, }, nil } -// ListReviews retrieves all reviews for a pull request. +// ListReviews retrieves all reviews for a pull request with pagination. // GitHub review states are translated to canonical vcs values. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { - reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + var allReviews []vcs.Review - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews: %w", err) - } + for page := 1; page <= maxReviewPages; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewsPerPage, page) - var responses []reviewResponse - if err := json.Unmarshal(body, &responses); err != nil { - return nil, fmt.Errorf("parse reviews response: %w", err) - } + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews page %d: %w", page, err) + } - reviews := make([]vcs.Review, len(responses)) - for i, r := range responses { - reviews[i] = vcs.Review{ - ID: r.ID, - Body: r.Body, - User: vcs.UserInfo{Login: r.User.Login}, - State: translateGitHubReviewState(r.State), - CommitID: r.CommitID, + var responses []reviewResponse + if err := json.Unmarshal(body, &responses); err != nil { + return nil, fmt.Errorf("parse reviews response: %w", err) + } + + if len(responses) == 0 { + break + } + + for _, r := range responses { + allReviews = append(allReviews, vcs.Review{ + ID: r.ID, + Body: r.Body, + User: vcs.UserInfo{Login: r.User.Login}, + State: translateGitHubReviewState(r.State), + CommitID: r.CommitID, + }) + } + + if len(responses) < reviewsPerPage { + break + } + + if page == maxReviewPages { + slog.Warn("ListReviews hit page limit; results may be truncated", + "owner", owner, "repo", repo, "pr", number, + "maxPages", maxReviewPages, "reviewsFetched", len(allReviews)) } } - return reviews, nil + + return allReviews, nil } // DeleteReview deletes a pull request review. @@ -178,7 +205,6 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) - // nil body: the GitHub DELETE endpoint for reviews requires no request body. _, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil) if err != nil { var apiErr *APIError @@ -204,12 +230,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i Event: "DISMISS", } - data, err := json.Marshal(payload) - if err != nil { - return fmt.Errorf("marshal dismiss request: %w", err) - } - - _, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data) + _, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload) if err != nil { return fmt.Errorf("dismiss review: %w", err) } @@ -228,3 +249,17 @@ func (c *Client) SupersedeReviews(ctx context.Context, owner, repo string, prNum } return errors.Join(errs...) } + +// GetAuthenticatedUser returns the login name of the authenticated user. +func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { + reqURL := fmt.Sprintf("%s/user", c.baseURL) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return "", fmt.Errorf("get authenticated user: %w", err) + } + var resp userResponse + if err := json.Unmarshal(body, &resp); err != nil { + return "", fmt.Errorf("parse user response: %w", err) + } + return resp.Login, nil +} From 8e26c26f5fffa5d004058b04500b777bc7877ec3 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 16:03:01 -0700 Subject: [PATCH 2/4] fix(github): add pagination tests and fix truncation warning logic F1: Add comprehensive pagination tests for ListReviews covering: - Multi-page behaviour (2 full + 1 partial page) - Exact-multiple-of-pageSize (extra empty-page round-trip) - maxReviewPages cutoff (cap hit, results still returned) - Empty first page (PR with no reviews) F2: Fix truncation warning logic by moving it outside the loop with a 'truncated' flag. Previously, the warning fired inline at page==maxPages which could miss the case where the short-page break fires first on the cap page. Now it only fires when the loop exits because the cap was reached and the last page was full (indicating more data likely exists). Also adds SetReviewPagination to Client for test-time override of page size and max pages, following the existing SetRetryBackoff pattern. --- github/client.go | 12 +++ github/review_test.go | 193 ++++++++++++++++++++++++++++++++++++++++++ github/reviews.go | 32 +++++-- 3 files changed, 229 insertions(+), 8 deletions(-) diff --git a/github/client.go b/github/client.go index cbaf828..695e289 100644 --- a/github/client.go +++ b/github/client.go @@ -110,6 +110,11 @@ type Client struct { // retryBackoff[i] is the delay before attempt i+1 (after attempt i fails). // If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff. retryBackoff []time.Duration + + // reviewPageSize overrides reviewsPerPage for testing. Zero means use default. + reviewPageSize int + // reviewMaxPages overrides maxReviewPages for testing. Zero means use default. + reviewMaxPages int } // defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil). @@ -194,6 +199,13 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error { return nil } +// SetReviewPagination overrides the page size and max pages for ListReviews. +// Intended for testing only; must be called before any goroutines issue requests. +func (c *Client) SetReviewPagination(pageSize, maxPages int) { + c.reviewPageSize = pageSize + c.reviewMaxPages = maxPages +} + // requestOptions holds per-request configuration for doRequestCore. type requestOptions struct { // bodyFn returns a fresh io.Reader for the request body on each attempt. diff --git a/github/review_test.go b/github/review_test.go index ad14b9f..3459956 100644 --- a/github/review_test.go +++ b/github/review_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "fmt" "encoding/json" "errors" "io" @@ -482,3 +483,195 @@ func TestPostReview_RequestCommitID_FallbackToComment(t *testing.T) { t.Errorf("sent commit_id = %q, want %q (fallback from comment)", gotPayload.CommitID, "comment-sha") } } + +// --- ListReviews pagination tests --- + +func TestListReviews_MultiPage(t *testing.T) { + // Test multi-page pagination: 2 full pages + 1 partial page. + // pageSize=3, so pages return [3, 3, 2] reviews = 8 total. + const pageSize = 3 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != "GET" { + t.Fatalf("expected GET, got %s", r.Method) + } + callCount++ + + page := r.URL.Query().Get("page") + var reviews []map[string]interface{} + + switch page { + case "1": + for i := 1; i <= pageSize; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "APPROVED", "commit_id": "sha1", + "user": map[string]string{"login": "user1"}, + }) + } + case "2": + for i := pageSize + 1; i <= pageSize*2; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "COMMENTED", "commit_id": "sha1", + "user": map[string]string{"login": "user2"}, + }) + } + case "3": + // Partial page: only 2 reviews (less than pageSize) + for i := pageSize*2 + 1; i <= pageSize*2+2; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": i, "body": fmt.Sprintf("review %d", i), + "state": "CHANGES_REQUESTED", "commit_id": "sha1", + "user": map[string]string{"login": "user3"}, + }) + } + default: + t.Fatalf("unexpected page: %s", page) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, 10) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 8 { + t.Fatalf("expected 8 reviews, got %d", len(reviews)) + } + if callCount != 3 { + t.Errorf("expected 3 API calls, got %d", callCount) + } + + // Verify reviews are correctly concatenated in order + for i, r := range reviews { + expectedID := int64(i + 1) + if r.ID != expectedID { + t.Errorf("review[%d]: expected ID %d, got %d", i, expectedID, r.ID) + } + } +} + +func TestListReviews_ExactMultipleOfPageSize(t *testing.T) { + // When total reviews is an exact multiple of pageSize, an extra request + // returning 0 results terminates the loop. No truncation warning. + const pageSize = 2 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + page := r.URL.Query().Get("page") + + var reviews []map[string]interface{} + switch page { + case "1": + reviews = []map[string]interface{}{ + {"id": 1, "body": "r1", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u1"}}, + {"id": 2, "body": "r2", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u2"}}, + } + case "2": + reviews = []map[string]interface{}{ + {"id": 3, "body": "r3", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u3"}}, + {"id": 4, "body": "r4", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u4"}}, + } + case "3": + // Empty page — signals end of data + reviews = []map[string]interface{}{} + default: + t.Fatalf("unexpected page: %s", page) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, 10) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 4 { + t.Fatalf("expected 4 reviews, got %d", len(reviews)) + } + // 3 calls: page 1 (full), page 2 (full), page 3 (empty) + if callCount != 3 { + t.Errorf("expected 3 API calls, got %d", callCount) + } +} + +func TestListReviews_MaxPagesCutoff(t *testing.T) { + // When maxPages is hit and the last page is full, results are truncated + // and a warning would fire (we verify the reviews are still returned). + const pageSize = 2 + const maxPages = 2 + callCount := 0 + + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + page := r.URL.Query().Get("page") + + // Always return a full page (simulating more data exists) + var reviews []map[string]interface{} + var baseID int + switch page { + case "1": + baseID = 0 + case "2": + baseID = pageSize + default: + t.Fatalf("unexpected page %s (should not exceed maxPages)", page) + } + for i := 1; i <= pageSize; i++ { + reviews = append(reviews, map[string]interface{}{ + "id": baseID + i, "body": fmt.Sprintf("r%d", baseID+i), + "state": "APPROVED", "commit_id": "sha1", + "user": map[string]string{"login": "user"}, + }) + } + + json.NewEncoder(w).Encode(reviews) + }) + c.SetReviewPagination(pageSize, maxPages) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + // Should return all reviews fetched within the cap + expectedCount := pageSize * maxPages + if len(reviews) != expectedCount { + t.Fatalf("expected %d reviews, got %d", expectedCount, len(reviews)) + } + if callCount != maxPages { + t.Errorf("expected %d API calls, got %d", maxPages, callCount) + } + // Verify concatenation order + for i, r := range reviews { + if r.ID != int64(i+1) { + t.Errorf("review[%d]: expected ID %d, got %d", i, i+1, r.ID) + } + } +} + +func TestListReviews_EmptyFirstPage(t *testing.T) { + // PR with no reviews: first page returns empty array. + callCount := 0 + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + callCount++ + json.NewEncoder(w).Encode([]map[string]interface{}{}) + }) + c.SetReviewPagination(10, 5) + + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 0 { + t.Fatalf("expected 0 reviews, got %d", len(reviews)) + } + if callCount != 1 { + t.Errorf("expected 1 API call, got %d", callCount) + } +} diff --git a/github/reviews.go b/github/reviews.go index 6a129bd..b448d53 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -153,11 +153,21 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, // ListReviews retrieves all reviews for a pull request with pagination. // GitHub review states are translated to canonical vcs values. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { - var allReviews []vcs.Review + perPage := reviewsPerPage + if c.reviewPageSize > 0 { + perPage = c.reviewPageSize + } + maxPages := maxReviewPages + if c.reviewMaxPages > 0 { + maxPages = c.reviewMaxPages + } - for page := 1; page <= maxReviewPages; page++ { + var allReviews []vcs.Review + truncated := false + + for page := 1; page <= maxPages; page++ { reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewsPerPage, page) + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page) body, err := c.doGet(ctx, reqURL) if err != nil { @@ -183,17 +193,23 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int }) } - if len(responses) < reviewsPerPage { + if len(responses) < perPage { break } - if page == maxReviewPages { - slog.Warn("ListReviews hit page limit; results may be truncated", - "owner", owner, "repo", repo, "pr", number, - "maxPages", maxReviewPages, "reviewsFetched", len(allReviews)) + // If we just fetched page maxPages and it was full (the short-page break + // above didn't fire), more reviews likely exist beyond our limit. + if page == maxPages { + truncated = true } } + if truncated { + slog.Warn("ListReviews hit page limit; results may be truncated", + "owner", owner, "repo", repo, "pr", number, + "maxPages", maxPages, "reviewsFetched", len(allReviews)) + } + return allReviews, nil } From 2e2fcbabfc0442bc421d4a3af1102465fa30f54c Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 17:53:20 -0700 Subject: [PATCH 3/4] style: fix import ordering and restore nil-body comment - Reorder stdlib imports in review_test.go to alphabetical (goimports convention) - Restore explanatory comment for nil body in DeleteReview Addresses review comments #20533, #20534 on PR #119 --- github/review_test.go | 2 +- github/reviews.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/github/review_test.go b/github/review_test.go index 3459956..0c1e4d3 100644 --- a/github/review_test.go +++ b/github/review_test.go @@ -2,9 +2,9 @@ package github import ( "context" - "fmt" "encoding/json" "errors" + "fmt" "io" "net/http" "strings" diff --git a/github/reviews.go b/github/reviews.go index b448d53..3e87dbc 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -221,6 +221,7 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) + // nil body: the GitHub DELETE endpoint for reviews requires no request body. _, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil) if err != nil { var apiErr *APIError From 437e318240a0579f62ba567a3e33e469d0abe926 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 18:01:57 -0700 Subject: [PATCH 4/4] nit: clarify truncation detection comment in ListReviews Expand the inline comment at the page==maxPages check to more explicitly explain why a full final page implies truncation. --- github/reviews.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/github/reviews.go b/github/reviews.go index 3e87dbc..a49a915 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -197,8 +197,10 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int break } - // If we just fetched page maxPages and it was full (the short-page break - // above didn't fire), more reviews likely exist beyond our limit. + // Truncation detection: this runs on the final allowed iteration + // (page == maxPages) only when the page was full (the len < perPage + // early-break above didn't fire). A full final page means additional + // reviews likely exist beyond our pagination limit. if page == maxPages { truncated = true }