diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index d005697..0fdccdc 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -67,7 +67,7 @@ inputs: required: false default: 'false' update-existing: - description: 'Delete previous review from same bot before posting (default true)' + description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)' required: false default: 'true' diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 39865b4..098a2da 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -196,34 +196,30 @@ func main() { return } - // Delete previous reviews from this bot if update-existing is enabled - if *updateExisting { - login, err := giteaClient.GetAuthenticatedUser(ctx) + log.Printf("Posting review (event=%s)...", event) + posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody) + if err != nil { + log.Fatalf("Failed to post review: %v", err) + } + log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) + + // Delete stale reviews from this bot if update-existing is enabled + if *updateExisting && posted.User.Login != "" { + reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { - log.Printf("Warning: could not determine reviewer identity: %v", err) + log.Printf("Warning: could not list existing reviews: %v", err) } else { - reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) - if err != nil { - log.Printf("Warning: could not list existing reviews: %v", err) - } else { - for _, r := range reviews { - if r.User.Login == login { - if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { - log.Printf("Warning: could not delete old review %d: %v", r.ID, err) - } else { - log.Printf("Deleted previous review %d", r.ID) - } + for _, r := range reviews { + if r.User.Login == posted.User.Login && r.ID != posted.ID { + if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { + log.Printf("Warning: could not delete old review %d: %v", r.ID, err) + } else { + log.Printf("Deleted stale review %d", r.ID) } } } } } - - log.Printf("Posting review (event=%s)...", event) - if err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody); err != nil { - log.Fatalf("Failed to post review: %v", err) - } - log.Printf("Review posted successfully!") } // fetchFileContext fetches the full content of modified files from the PR branch. @@ -359,7 +355,7 @@ func envOrDefaultInt(key string, defaultVal int) int { } func envOrDefaultBool(key string, defaultVal bool) bool { - v := os.Getenv(key) + v := strings.TrimSpace(strings.ToLower(os.Getenv(key))) if v == "" { return defaultVal } diff --git a/gitea/client.go b/gitea/client.go index f21b1d5..75bd096 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -129,9 +129,9 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r return string(body), nil } -// PostReview submits a review to a PR. +// PostReview submits a review to a PR and returns the created review. // event should be "APPROVED" or "REQUEST_CHANGES". -func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) error { +func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) (*Review, error) { reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number) payload := struct { @@ -144,27 +144,36 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, data, err := json.Marshal(payload) if err != nil { - return fmt.Errorf("marshal review payload: %w", err) + return nil, fmt.Errorf("marshal review payload: %w", err) } req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) if err != nil { - return fmt.Errorf("create review request: %w", err) + return nil, fmt.Errorf("create review request: %w", err) } req.Header.Set("Authorization", "token "+c.token) req.Header.Set("Content-Type", "application/json") resp, err := c.http.Do(req) if err != nil { - return fmt.Errorf("post review: %w", err) + return nil, fmt.Errorf("post review: %w", err) } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { respBody, _ := io.ReadAll(resp.Body) - return fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody)) + return nil, fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody)) } - return nil + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("read review response: %w", err) + } + var review Review + if err := json.Unmarshal(respBody, &review); err != nil { + return nil, fmt.Errorf("parse review response: %w", err) + } + return &review, nil } func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { @@ -277,41 +286,33 @@ type Review struct { Stale bool `json:"stale"` } -// GetAuthenticatedUser returns the login name of the token's owner. -func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { - reqURL := fmt.Sprintf("%s/api/v1/user", c.baseURL) - body, err := c.doGet(ctx, reqURL) - if err != nil { - return "", fmt.Errorf("get authenticated user: %w", err) - } - var user struct { - Login string `json:"login"` - } - if err := json.Unmarshal(body, &user); err != nil { - return "", fmt.Errorf("parse user response: %w", err) - } - if user.Login == "" { - return "", fmt.Errorf("empty login in user response") - } - return user.Login, nil -} - // ListReviews returns all reviews on a pull request. +// Paginates through all pages to ensure no reviews are missed. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) { - reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", - c.baseURL, - url.PathEscape(owner), - url.PathEscape(repo), - number) - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews: %w", err) + const pageSize = 50 + var all []Review + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews?limit=%d&page=%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + number, + pageSize, + page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews (page %d): %w", page, err) + } + var batch []Review + if err := json.Unmarshal(body, &batch); err != nil { + return nil, fmt.Errorf("parse reviews (page %d): %w", page, err) + } + all = append(all, batch...) + if len(batch) < pageSize { + break + } } - var reviews []Review - if err := json.Unmarshal(body, &reviews); err != nil { - return nil, fmt.Errorf("parse reviews: %w", err) - } - return reviews, nil + return all, nil } // DeleteReview deletes a review by ID. The token must belong to the review author. diff --git a/gitea/client_test.go b/gitea/client_test.go index fc93473..eabdd9e 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -123,15 +123,21 @@ func TestPostReview(t *testing.T) { } w.WriteHeader(http.StatusOK) - w.Write([]byte(`{}`)) + w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`)) })) defer server.Close() client := NewClient(server.URL, "test-token") - err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM") + review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM") if err != nil { t.Fatalf("unexpected error: %v", err) } + if review.ID != 100 { + t.Errorf("expected review ID 100, got %d", review.ID) + } + if review.User.Login != "review-bot" { + t.Errorf("expected user login %q, got %q", "review-bot", review.User.Login) + } } func TestGetPullRequest_Non200(t *testing.T) { @@ -169,7 +175,7 @@ func TestPostReview_Non200(t *testing.T) { defer server.Close() client := NewClient(server.URL, "test-token") - err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test") + _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test") if err == nil { t.Fatal("expected error for 403, got nil") } @@ -319,49 +325,18 @@ func TestEscapePath(t *testing.T) { } } -func TestGetAuthenticatedUser(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/v1/user" { - t.Errorf("unexpected path: %s", r.URL.Path) - } - if r.Method != "GET" { - t.Errorf("expected GET, got %s", r.Method) - } - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"login":"review-bot","id":42}`)) - })) - defer server.Close() - - client := NewClient(server.URL, "test-token") - login, err := client.GetAuthenticatedUser(context.Background()) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if login != "review-bot" { - t.Errorf("expected login %q, got %q", "review-bot", login) - } -} - -func TestGetAuthenticatedUser_EmptyLogin(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"login":"","id":0}`)) - })) - defer server.Close() - - client := NewClient(server.URL, "test-token") - _, err := client.GetAuthenticatedUser(context.Background()) - if err == nil { - t.Fatal("expected error for empty login, got nil") - } -} - func TestListReviews(t *testing.T) { + pageCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews" { t.Errorf("unexpected path: %s", r.URL.Path) } + if r.URL.Query().Get("limit") != "50" { + t.Errorf("expected limit=50, got %s", r.URL.Query().Get("limit")) + } + pageCount++ w.Header().Set("Content-Type", "application/json") + // Return 2 results (less than page size) to signal end w.Write([]byte(`[{"id":10,"user":{"login":"bot-a"},"state":"APPROVED","stale":false},{"id":11,"user":{"login":"bot-b"},"state":"REQUEST_CHANGES","stale":true}]`)) })) defer server.Close() @@ -377,8 +352,45 @@ func TestListReviews(t *testing.T) { if reviews[0].User.Login != "bot-a" { t.Errorf("expected bot-a, got %s", reviews[0].User.Login) } - if reviews[1].ID != 11 { - t.Errorf("expected id 11, got %d", reviews[1].ID) + if pageCount != 1 { + t.Errorf("expected 1 page fetch (results < page size), got %d", pageCount) + } +} + +func TestListReviews_Pagination(t *testing.T) { + pageCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + pageCount++ + page := r.URL.Query().Get("page") + w.Header().Set("Content-Type", "application/json") + if page == "1" { + // Return exactly 50 items to trigger next page fetch + items := "[" + for i := 0; i < 50; i++ { + if i > 0 { + items += "," + } + items += fmt.Sprintf(`{"id":%d,"user":{"login":"bot"},"state":"APPROVED","stale":false}`, i+1) + } + items += "]" + w.Write([]byte(items)) + } else { + // Page 2: return fewer than 50 to signal end + w.Write([]byte(`[{"id":51,"user":{"login":"bot"},"state":"APPROVED","stale":false}]`)) + } + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 51 { + t.Fatalf("expected 51 reviews across 2 pages, got %d", len(reviews)) + } + if pageCount != 2 { + t.Errorf("expected 2 page fetches, got %d", pageCount) } }