rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests 1 Actions Packages Projects Releases 6 Wiki Activity

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

Browse Source 
- 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)
This commit is contained in:
claw
2026-05-13 03:29:45 -07:00
committed by Aaron Weiker
parent 511e32463b
commit c4d1631242
4 changed files with 83 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmd/review-bot/main.go
+14 -2
View File
@@ -2,6 +2,7 @@ package main
import (
"context"
"errors"
"flag"
"fmt"
"log/slog"
@@ -87,6 +88,9 @@ func main() {
// Register --gitea-url as a backward-compatible alias for --vcs-url.
// StringVar shares the *string pointer with vcsURL, so whichever flag is
// set last by flag.Parse wins — both point to the same underlying value.
// NOTE: If a user passes both --vcs-url and --gitea-url, the last one on
// the command line takes effect (standard flag package behavior). This is
// acceptable since --gitea-url is deprecated and both serve the same purpose.
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
flag.Parse()
@@ -528,14 +532,17 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) error {
switch provider {
case "github":
// Best-effort dismissal: attempt all reviews, join any errors.
var errs []error
for _, old := range oldReviews {
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
slog.Warn("failed to dismiss review", "id", old.ID, "error", err)
errs = append(errs, fmt.Errorf("dismiss review %d: %w", old.ID, err))
} else {
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
}
}
return nil
return errors.Join(errs...)
case "gitea":
// Continue to Gitea-specific logic below the switch.
default:
@@ -686,18 +693,20 @@ func isPatternFile(path string) bool {
}
// evaluateCIStatus checks if all CI statuses indicate success.
// Returns passed=true if no checks have failed (pending checks are not treated as failures).
func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string) {
if len(statuses) == 0 {
return true, "no CI statuses found"
}
var failed []string
var pending int
for _, s := range statuses {
switch s.Status {
case "success":
// good
case "pending":
// treat pending as not-failed
pending++
case "failure", "error":
failed = append(failed, fmt.Sprintf("%s: %s", s.Context, s.Description))
}
@@ -706,6 +715,9 @@ func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string)
if len(failed) > 0 {
return false, strings.Join(failed, "; ")
}
if pending > 0 {
return true, fmt.Sprintf("no failures (%d pending)", pending)
}
return true, "all checks passed"
}
cmd/review-bot/main_test.go
+1 -1
View File
@@ -547,7 +547,7 @@ func TestEvaluateCIStatus(t *testing.T) {
{Status: "success", Context: "ci/test", Description: "Tests passed"},
},
wantPassed: true,
wantSubstr: "all checks passed",
wantSubstr: "no failures",
},
{
name: "multiple failures",
github/client.go
+56
View File
@@ -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)}
}
github/reviews.go
+12 -59
View File
@@ -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)}
}