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)
This commit is contained in:
claw
+14
-2
@@ -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:
|
||||
@@ -687,18 +694,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))
|
||||
}
|
||||
@@ -707,6 +716,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"
|
||||
}
|
||||
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -5,6 +5,7 @@ package github
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"encoding/json"
|
||||
"context"
|
||||
"errors"
|
||||
"fmt"
|
||||
@@ -383,4 +384,58 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r
|
||||
opts.extraHeaders = map[string]string{"Content-Type": "application/json"}
|
||||
}
|
||||
return c.doRequestCore(ctx, method, reqURL, opts)
|
||||
|
||||
}
|
||||
// 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,237 +0,0 @@
|
||||
package github
|
||||
|
||||
import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
|
||||
"gitea.weiker.me/rodin/review-bot/vcs"
|
||||
)
|
||||
|
||||
// reviewResponse is the GitHub API response for a pull request review.
|
||||
type reviewResponse struct {
|
||||
ID int64 `json:"id"`
|
||||
Body string `json:"body"`
|
||||
User struct {
|
||||
Login string `json:"login"`
|
||||
} `json:"user"`
|
||||
State string `json:"state"`
|
||||
CommitID string `json:"commit_id"`
|
||||
}
|
||||
|
||||
// reviewCreateRequest is the GitHub API request body for creating a pull request review.
|
||||
type reviewCreateRequest struct {
|
||||
Body string `json:"body"`
|
||||
Event string `json:"event"`
|
||||
Comments []reviewCommentCreate `json:"comments,omitempty"`
|
||||
CommitID string `json:"commit_id,omitempty"`
|
||||
}
|
||||
|
||||
// reviewCommentCreate is a single inline comment in a review creation request.
|
||||
type reviewCommentCreate struct {
|
||||
Path string `json:"path"`
|
||||
Position int `json:"position"`
|
||||
Body string `json:"body"`
|
||||
}
|
||||
|
||||
// dismissReviewRequest is the GitHub API request body for dismissing a review.
|
||||
type dismissReviewRequest struct {
|
||||
Message string `json:"message"`
|
||||
}
|
||||
|
||||
// userResponse is the GitHub API response for the authenticated user.
|
||||
type userResponse struct {
|
||||
Login string `json:"login"`
|
||||
}
|
||||
|
||||
// translateReviewEvent converts a vcs.ReviewEvent to the GitHub API event string.
|
||||
func translateReviewEvent(event vcs.ReviewEvent) string {
|
||||
switch event {
|
||||
case vcs.ReviewEventApprove:
|
||||
return "APPROVE"
|
||||
case vcs.ReviewEventRequestChanges:
|
||||
return "REQUEST_CHANGES"
|
||||
case vcs.ReviewEventComment:
|
||||
return "COMMENT"
|
||||
default:
|
||||
return "COMMENT"
|
||||
}
|
||||
}
|
||||
|
||||
// PostReview creates a new review on a pull request.
|
||||
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
|
||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
|
||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||
|
||||
payload := reviewCreateRequest{
|
||||
Body: req.Body,
|
||||
Event: translateReviewEvent(req.Event),
|
||||
}
|
||||
|
||||
for _, comment := range req.Comments {
|
||||
rc := reviewCommentCreate{
|
||||
Path: comment.Path,
|
||||
Position: comment.Position,
|
||||
Body: comment.Body,
|
||||
}
|
||||
payload.Comments = append(payload.Comments, rc)
|
||||
// Use CommitID from the first comment that has one.
|
||||
// All comments in a single review are expected to reference the same commit.
|
||||
if payload.CommitID == "" && comment.CommitID != "" {
|
||||
payload.CommitID = comment.CommitID
|
||||
}
|
||||
}
|
||||
|
||||
body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("post review: %w", err)
|
||||
}
|
||||
|
||||
var resp reviewResponse
|
||||
if err := json.Unmarshal(body, &resp); err != nil {
|
||||
return nil, fmt.Errorf("parse review response: %w", err)
|
||||
}
|
||||
|
||||
return &vcs.Review{
|
||||
ID: resp.ID,
|
||||
Body: resp.Body,
|
||||
User: vcs.UserInfo{Login: resp.User.Login},
|
||||
State: resp.State,
|
||||
CommitID: resp.CommitID,
|
||||
}, nil
|
||||
}
|
||||
|
||||
// ListReviews lists all reviews on a pull request.
|
||||
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)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("list reviews page %d: %w", page, err)
|
||||
}
|
||||
var reviews []reviewResponse
|
||||
if err := json.Unmarshal(body, &reviews); err != nil {
|
||||
return nil, fmt.Errorf("parse reviews JSON: %w", err)
|
||||
}
|
||||
if len(reviews) == 0 {
|
||||
break
|
||||
}
|
||||
for _, r := range reviews {
|
||||
allReviews = append(allReviews, vcs.Review{
|
||||
ID: r.ID,
|
||||
Body: r.Body,
|
||||
User: vcs.UserInfo{Login: r.User.Login},
|
||||
State: r.State,
|
||||
CommitID: r.CommitID,
|
||||
})
|
||||
}
|
||||
if len(reviews) < 100 {
|
||||
break
|
||||
}
|
||||
}
|
||||
|
||||
return allReviews, nil
|
||||
}
|
||||
|
||||
// DeleteReview permanently deletes a review from a pull request.
|
||||
// Use DismissReview instead when the review should remain visible but marked as dismissed
|
||||
// (e.g., superseding an outdated review while preserving history).
|
||||
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
|
||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
|
||||
_, err := c.doRequest(ctx, http.MethodDelete, reqURL, "")
|
||||
if err != nil {
|
||||
return fmt.Errorf("delete review: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// DismissReview dismisses a review on a pull request with a message.
|
||||
func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
|
||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/dismissals",
|
||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
|
||||
|
||||
payload := dismissReviewRequest{
|
||||
Message: message,
|
||||
}
|
||||
|
||||
_, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload)
|
||||
if err != nil {
|
||||
return fmt.Errorf("dismiss review: %w", err)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
|
||||
// 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)}
|
||||
}
|
||||
Reference in New Issue
Block a user