diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 55b6281..b37f448 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -257,56 +257,70 @@ func main() { log.Printf("Attaching %d inline comments", len(inlineComments)) } - // Check if existing review is unchanged — skip to preserve conversation threads + // --- Review update strategy --- + // 1. No existing review → POST new + // 2. Existing review, same state → PATCH body in place (preserves threads) + // 3. Existing review, state change → PATCH old to "Superseded", POST new if *updateExisting && *reviewerName != "" { existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) - if err == nil && reviewUnchanged(existingReviews, reviewBody, event, sentinel) { - log.Printf("Review unchanged from previous run; skipping to preserve threads") - return - } - } - - log.Printf("Posting review (event=%s)...", event) - posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) - 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 using sentinel matching - if *updateExisting && *reviewerName != "" { - 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.ID != posted.ID && r.User.Login == posted.User.Login && strings.Contains(r.Body, sentinel) { - 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) - } - } + // Worst-wins: escalate if a sibling blocks (need own login from existing review) + ownLogin := "" + existing := findOwnReview(existingReviews, sentinel) + if existing != nil { + ownLogin = existing.User.Login + } + if event == "APPROVED" && shouldEscalate(existingReviews, 0, ownLogin, sentinel) { + log.Printf("Sibling review has REQUEST_CHANGES; escalating to REQUEST_CHANGES") + event = "REQUEST_CHANGES" } - // Worst-wins: if we posted APPROVE but a sibling review from the - // same user (same token, different role) has REQUEST_CHANGES, - // delete ours and re-post as REQUEST_CHANGES to maintain the block. - if event == "APPROVED" && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) { - log.Printf("Sibling review has REQUEST_CHANGES; escalating") - if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID); err != nil { - log.Printf("Warning: could not delete review for escalation: %v", err) - } else { - _, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody, inlineComments) + if existing != nil { + if reviewUnchanged(existingReviews, reviewBody, event, sentinel) { + log.Printf("Review unchanged from previous run; skipping to preserve threads") + return + } + + // Same state → PATCH in place + if existing.State == event { + commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) if err != nil { - log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err) + log.Printf("Warning: could not find review comment ID, falling back to new post: %v", err) } else { - log.Printf("Review escalated to REQUEST_CHANGES") + if err := giteaClient.EditComment(ctx, owner, repoName, commentID, reviewBody); err != nil { + log.Printf("Warning: could not edit review, falling back to new post: %v", err) + } else { + log.Printf("Review updated in place (comment_id=%d)", commentID) + return + } + } + } else { + // State change → mark old as superseded, post new below + commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) + if err != nil { + log.Printf("Warning: could not find old review comment ID: %v", err) + } else { + supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel) + if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil { + log.Printf("Warning: could not mark old review as superseded: %v", err) + } else { + log.Printf("Marked old review as superseded (state was %s, now %s)", existing.State, event) + } } } } } } + + // POST new review (first run, or state transition fallthrough) + log.Printf("Posting review (event=%s)...", event) + _, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) + if 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. @@ -463,12 +477,20 @@ func validateReviewerName(name string) error { return nil } -// shouldEscalate checks if the current APPROVED review should be escalated -// to REQUEST_CHANGES because a sibling bot review (same user, different role) -// already has REQUEST_CHANGES. -func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSentinel string) bool { +// shouldEscalate checks if any sibling bot review from the same user +// (different sentinel, same token) has REQUEST_CHANGES. +// ownLogin is the bot user login; if empty, escalation check is skipped. +// postedID is excluded from consideration (0 means no exclusion needed). +func shouldEscalate(reviews []gitea.Review, postedID int64, ownLogin, ownSentinel string) bool { + if ownLogin == "" { + return false + } for _, r := range reviews { - if r.ID != postedID && !r.Stale && r.User.Login == postedLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "", want: false, }, @@ -73,7 +74,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: true, }, @@ -83,7 +84,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -93,7 +94,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -103,7 +104,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -113,7 +114,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "APPROVED", false, "good\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -123,7 +124,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", false, "please fix this"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -133,7 +134,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -141,7 +142,7 @@ func TestShouldEscalate(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := shouldEscalate(tc.reviews, tc.postedID, tc.postedLogin, tc.ownSentinel) + got := shouldEscalate(tc.reviews, tc.postedID, tc.ownLogin, tc.ownSentinel) if got != tc.want { t.Errorf("shouldEscalate() = %v, want %v", got, tc.want) } @@ -227,3 +228,63 @@ func TestReviewUnchanged(t *testing.T) { }) } } + +func TestFindOwnReview(t *testing.T) { + tests := []struct { + name string + reviews []gitea.Review + sentinel string + wantID int64 + wantNil bool + }{ + { + name: "no reviews", + reviews: nil, + sentinel: "", + wantNil: true, + }, + { + name: "found by sentinel", + reviews: []gitea.Review{ + makeReview(42, "bot", "APPROVED", false, "review body\n"), + }, + sentinel: "", + wantID: 42, + }, + { + name: "wrong sentinel", + reviews: []gitea.Review{ + makeReview(42, "bot", "APPROVED", false, "body\n"), + }, + sentinel: "", + wantNil: true, + }, + { + name: "multiple reviews, returns first match", + reviews: []gitea.Review{ + makeReview(10, "bot", "APPROVED", false, "old\n"), + makeReview(20, "bot", "APPROVED", false, "new\n"), + }, + sentinel: "", + wantID: 20, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := findOwnReview(tc.reviews, tc.sentinel) + if tc.wantNil { + if got != nil { + t.Errorf("findOwnReview() = %v, want nil", got) + } + } else { + if got == nil { + t.Fatal("findOwnReview() = nil, want non-nil") + } + if got.ID != tc.wantID { + t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID) + } + } + }) + } +} diff --git a/gitea/client.go b/gitea/client.go index 4185f18..ee15e9a 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -353,3 +353,78 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in } return nil } + +// TimelineEvent represents an entry from the issue timeline API. +type TimelineEvent struct { + ID int64 `json:"id"` + Type string `json:"type"` + Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` +} + +// GetTimelineReviewCommentID finds the comment ID for a review body by +// scanning the issue timeline for a review event containing the sentinel. +func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo string, number int, sentinel string) (int64, error) { + const pageSize = 50 + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?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 0, fmt.Errorf("get timeline (page %d): %w", page, err) + } + var events []TimelineEvent + if err := json.Unmarshal(body, &events); err != nil { + return 0, fmt.Errorf("parse timeline (page %d): %w", page, err) + } + for _, ev := range events { + if ev.Type == "review" && strings.Contains(ev.Body, sentinel) { + return ev.ID, nil + } + } + if len(events) < pageSize { + break + } + } + return 0, fmt.Errorf("no timeline event found with sentinel") +} + +// EditComment updates the body of an issue/review comment. +func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/comments/%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + commentID) + + payload := struct { + Body string `json:"body"` + }{Body: newBody} + data, _ := json.Marshal(payload) + + req, err := http.NewRequestWithContext(ctx, http.MethodPatch, reqURL, bytes.NewReader(data)) + if err != nil { + return fmt.Errorf("create edit 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("edit comment: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("edit comment failed (status %d): %s", resp.StatusCode, body) + } + return nil +} diff --git a/gitea/client_test.go b/gitea/client_test.go index 415e83b..be9b94e 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -426,3 +426,82 @@ func TestDeleteReview_Forbidden(t *testing.T) { t.Fatal("expected error for 403, got nil") } } + +func TestEditComment(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPatch { + t.Errorf("expected PATCH, got %s", r.Method) + } + if r.URL.Path != "/api/v1/repos/owner/repo/issues/comments/42" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + + var payload struct { + Body string `json:"body"` + } + json.NewDecoder(r.Body).Decode(&payload) + if payload.Body != "updated body" { + t.Errorf("unexpected body: %s", payload.Body) + } + + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"id": 42, "body": "updated body"}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.EditComment(context.Background(), "owner", "repo", 42, "updated body") + if err != nil { + t.Fatalf("EditComment() error = %v", err) + } +} + +func TestEditComment_Forbidden(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message": "not allowed"}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.EditComment(context.Background(), "owner", "repo", 42, "new body") + if err == nil { + t.Fatal("expected error for 403 response") + } +} + +func TestGetTimelineReviewCommentID(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/repos/owner/repo/issues/5/timeline" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + w.Write([]byte(`[ + {"id": 100, "type": "comment", "body": "random"}, + {"id": 200, "type": "review", "body": "other review "}, + {"id": 300, "type": "review", "body": "our review "} + ]`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + id, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "") + if err != nil { + t.Fatalf("GetTimelineReviewCommentID() error = %v", err) + } + if id != 300 { + t.Errorf("got id=%d, want 300", id) + } +} + +func TestGetTimelineReviewCommentID_NotFound(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`[{"id": 100, "type": "review", "body": "no match"}]`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + _, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "") + if err == nil { + t.Fatal("expected error when sentinel not found") + } +}