Merge pull request 'fix(github): consolidate review.go and identity.go into reviews.go (#116)' (#119) from review-bot-issue-116 into feature/github-support

Reviewed-on: #119
Reviewed-by: Aaron Weiker <aaron@weiker.org>
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>

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.

github/identity.go

@@ -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
-}

github/review_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"strings"
@@ -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)
+	}
+}

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,69 @@ 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)
-
-	body, err := c.doGet(ctx, reqURL)
-	if err != nil {
-		return nil, fmt.Errorf("list reviews: %w", err)
+	perPage := reviewsPerPage
+	if c.reviewPageSize > 0 {
+		perPage = c.reviewPageSize
+	}
+	maxPages := maxReviewPages
+	if c.reviewMaxPages > 0 {
+		maxPages = c.reviewMaxPages
 	}
 
-	var responses []reviewResponse
-	if err := json.Unmarshal(body, &responses); err != nil {
-		return nil, fmt.Errorf("parse reviews response: %w", err)
-	}
+	var allReviews []vcs.Review
+	truncated := false
 
-	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,
+	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, perPage, page)
+
+		body, err := c.doGet(ctx, reqURL)
+		if err != nil {
+			return nil, fmt.Errorf("list reviews page %d: %w", page, err)
+		}
+
+		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) < perPage {
+			break
+		}
+
+		// 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
 		}
 	}
-	return reviews, nil
+
+	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
 }
 
 // DeleteReview deletes a pull request review.
@@ -204,12 +249,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 +268,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
+}