fix(review): address bot review feedback on PR #106

- Document --gitea-url/--vcs-url last-one-wins behavior when both flags are passed simultaneously (sonnet MINOR #1) - Move doJSONRequest from github/reviews.go to github/client.go where other HTTP helpers live (sonnet MINOR #2) - Return joined error from supersedeOldReviews GitHub case instead of silently swallowing DismissReview failures (sonnet MINOR #3) - Fix evaluateCIStatus to distinguish 'all checks passed' from 'no failures (N pending)' to avoid misleading status (gpt MINOR #2) - Extract reviewsPerPage and maxReviewPages named constants for ListReviews pagination (gpt NIT #3)
2026-05-13 03:29:45 -07:00
parent e3b9ade5cd
commit 83a538f138
4 changed files with 83 additions and 62 deletions
@@ -4,6 +4,8 @@
 package github

 import (
+	"bytes"
+	"encoding/json"
 	"context"
 	"errors"
 	"fmt"
@@ -342,3 +344,57 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
 func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 	return c.doRequest(ctx, http.MethodGet, reqURL, "")
 }
+
+// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
+// It handles HTTPS validation, authentication, and response reading.
+// This is a general-purpose helper used by any method that needs to send JSON payloads
+// (e.g. PostReview, DismissReview).
+func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
+	const maxErrorBodyBytes = 4 * 1024
+
+	jsonBody, err := json.Marshal(payload)
+	if err != nil {
+		return nil, fmt.Errorf("marshal request body: %w", err)
+	}
+
+	if c.token != "" && !c.allowInsecureHTTP {
+		parsed, err := url.Parse(reqURL)
+		if err != nil {
+			return nil, fmt.Errorf("parse request URL: %w", err)
+		}
+		if !strings.EqualFold(parsed.Scheme, "https") {
+			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
+		}
+	}
+
+	req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody))
+	if err != nil {
+		return nil, fmt.Errorf("create request: %w", err)
+	}
+	if c.token != "" {
+		req.Header.Set("Authorization", "Bearer "+c.token)
+	}
+	req.Header.Set("User-Agent", userAgent)
+	req.Header.Set("Accept", "application/vnd.github+json")
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := c.httpClient.Do(req)
+	if err != nil {
+		return nil, fmt.Errorf("do request: %w", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
+		body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1))
+		if err != nil {
+			return nil, fmt.Errorf("read response body: %w", err)
+		}
+		if len(body) > maxResponseBytes {
+			return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes)
+		}
+		return body, nil
+	}
+
+	errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes)))
+	return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
+}
@@ -1,18 +1,23 @@
 package github

 import (
-	"bytes"
 	"context"
 	"encoding/json"
 	"fmt"
-	"io"
 	"net/http"
 	"net/url"
-	"strings"

 	"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
+)
+
 // reviewResponse is the GitHub API response for a pull request review.
 type reviewResponse struct {
 	ID   int64  `json:"id"`
@@ -110,9 +115,9 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
 	var allReviews []vcs.Review

-	for page := 1; page <= 100; page++ {
-		reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=100&page=%d",
-			c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page)
+	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)
 		body, err := c.doGet(ctx, reqURL)
 		if err != nil {
 			return nil, fmt.Errorf("list reviews page %d: %w", page, err)
@@ -133,7 +138,7 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
 				CommitID: r.CommitID,
 			})
 		}
-		if len(reviews) < 100 {
+		if len(reviews) < reviewsPerPage {
 			break
 		}
 	}
@@ -183,55 +188,3 @@ func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
 	}
 	return resp.Login, nil
 }
-
-// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
-// It handles HTTPS validation, authentication, and response reading.
-func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
-	const maxErrorBodyBytes = 4 * 1024
-
-	jsonBody, err := json.Marshal(payload)
-	if err != nil {
-		return nil, fmt.Errorf("marshal request body: %w", err)
-	}
-
-	if c.token != "" && !c.allowInsecureHTTP {
-		parsed, err := url.Parse(reqURL)
-		if err != nil {
-			return nil, fmt.Errorf("parse request URL: %w", err)
-		}
-		if !strings.EqualFold(parsed.Scheme, "https") {
-			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
-		}
-	}
-
-	req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody))
-	if err != nil {
-		return nil, fmt.Errorf("create request: %w", err)
-	}
-	if c.token != "" {
-		req.Header.Set("Authorization", "Bearer "+c.token)
-	}
-	req.Header.Set("User-Agent", userAgent)
-	req.Header.Set("Accept", "application/vnd.github+json")
-	req.Header.Set("Content-Type", "application/json")
-
-	resp, err := c.httpClient.Do(req)
-	if err != nil {
-		return nil, fmt.Errorf("do request: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-		body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1))
-		if err != nil {
-			return nil, fmt.Errorf("read response body: %w", err)
-		}
-		if len(body) > maxResponseBytes {
-			return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes)
-		}
-		return body, nil
-	}
-
-	errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes)))
-	return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
-}