diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index b7a554b..46c2845 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -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: @@ -695,18 +702,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)) } @@ -715,6 +724,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" } diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 5711003..094d8fd 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -593,7 +593,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", diff --git a/github/client.go b/github/client.go index dc10c7a..1f17308 100644 --- a/github/client.go +++ b/github/client.go @@ -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)} +} diff --git a/github/reviews.go b/github/reviews.go index 1f877e0..4cf2290 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -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)} -}