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

@@ -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/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 {
@@ -112,12 +126,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 		})
 	}
 
-	data, err := json.Marshal(payload)
+	body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload)
-	if err != nil {
-		return nil, fmt.Errorf("marshal review request: %w", err)
-	}
-
-	body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
 	if err != nil {
 		return nil, fmt.Errorf("post review: %w", err)
 	}
@@ -136,33 +145,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",
+	var allReviews []vcs.Review
-		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 
-	body, err := c.doGet(ctx, reqURL)
+	for page := 1; page <= maxReviewPages; page++ {
-	if err != nil {
+		reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d",
-		return nil, fmt.Errorf("list reviews: %w", err)
+			c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewsPerPage, page)
-	}
 
-	var responses []reviewResponse
+		body, err := c.doGet(ctx, reqURL)
-	if err := json.Unmarshal(body, &responses); err != nil {
+		if err != nil {
-		return nil, fmt.Errorf("parse reviews response: %w", err)
+			return nil, fmt.Errorf("list reviews page %d: %w", page, err)
-	}
+		}
 
-	reviews := make([]vcs.Review, len(responses))
+		var responses []reviewResponse
-	for i, r := range responses {
+		if err := json.Unmarshal(body, &responses); err != nil {
-		reviews[i] = vcs.Review{
+			return nil, fmt.Errorf("parse reviews response: %w", err)
-			ID:       r.ID,
+		}
-			Body:     r.Body,
+
-			User:     vcs.UserInfo{Login: r.User.Login},
+		if len(responses) == 0 {
-			State:    translateGitHubReviewState(r.State),
+			break
-			CommitID: r.CommitID,
+		}
+
+		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.
@@ -173,7 +200,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
@@ -199,12 +225,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
 		Event: "DISMISS",
 	}
 
-	data, err := json.Marshal(payload)
+	_, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload)
-	if err != nil {
-		return fmt.Errorf("marshal dismiss request: %w", err)
-	}
-
-	_, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data)
 	if err != nil {
 		return fmt.Errorf("dismiss review: %w", err)
 	}
@@ -223,3 +244,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
+}