From d4bf13eeab9e8a83d4246ceaa21c945cd9c2dd60 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 13:28:03 -0700 Subject: [PATCH 1/2] fix: supersede ALL old reviews, not just the most recent Previously findOwnReview returned only the single most-recent matching review, so on PRs with multiple force-pushes only the latest old review got superseded. The rest accumulated as unsuperseded stale reviews. Changes: - Add findAllOwnReviews() to collect all non-superseded matching reviews - Loop over all old reviews in the supersede phase - Add GetTimelineReviewCommentIDForReview() to find comment IDs by review ID (fetches review body, matches in timeline by prefix) - Each old review gets independently superseded and its inline comments resolved The old findOwnReview is kept for backward compat (tested, may be useful as a utility). --- cmd/review-bot/main.go | 101 +++++++++++++++++++----------------- cmd/review-bot/main_test.go | 21 ++++++++ gitea/client.go | 59 +++++++++++++++++++++ 3 files changed, 132 insertions(+), 49 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 8ed8ee5..d125af8 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -319,25 +319,16 @@ func main() { // 1. POST new review first (gets non-stale approval badge on HEAD) // 2. Then supersede old review with link to the new one // Order matters: post first so we have the new review's URL for the supersede message. - var existingReview *gitea.Review - var existingCommentID int64 + var oldReviews []gitea.Review + var sharedTokenMode bool if *reviewerName != "" { existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not list existing reviews", "pr", prNumber, "error", err) } else { - sharedToken := hasSharedToken(existingReviews, sentinel) - if !sharedToken { - existingReview = findOwnReview(existingReviews, sentinel) - if existingReview != nil { - cid, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) - if err != nil { - slog.Warn("could not find old review comment ID for supersede", "error", err) - existingReview = nil // can't supersede without comment ID - } else { - existingCommentID = cid - } - } + sharedTokenMode = hasSharedToken(existingReviews, sentinel) + if !sharedTokenMode { + oldReviews = findAllOwnReviews(existingReviews, sentinel) } else { slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review") } @@ -365,43 +356,42 @@ func main() { } slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber) - // Supersede old review with link to the new one - if existingReview != nil && existingCommentID > 0 { + // Supersede all old reviews with link to the new one + if len(oldReviews) > 0 { newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID) - supersededBody := buildSupersededBody(existingReview.Body, existingReview.CommitID, newReviewURL, sentinel) - supersedeOK := false - if err := giteaClient.EditComment(ctx, owner, repoName, existingCommentID, supersededBody); err != nil { - slog.Warn("could not mark old review as superseded", "comment_id", existingCommentID, "error", err) - } else { - slog.Info("marked old review as superseded", "old_state", existingReview.State, "new_review_id", posted.ID, "pr", prNumber) - supersedeOK = true - } - - // Resolve old review's inline comments only after successful supersede - if supersedeOK { - oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, existingReview.ID) + for _, oldReview := range oldReviews { + cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID) if err != nil { - slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err) - } else { - resolved, failed := 0, 0 - for _, c := range oldComments { - if c.ID == 0 { - continue - } - if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil { - slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) - failed++ - } else { - resolved++ - } + slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err) + continue + } + supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel) + if err := giteaClient.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil { + slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err) + continue + } + slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber) + + // Resolve old review's inline comments + oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID) + if err != nil { + slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err) + continue + } + resolved := 0 + for _, c := range oldComments { + if c.ID == 0 { + continue } - if resolved > 0 { - slog.Info("resolved old inline comments", "count", resolved, "pr", prNumber) - } - if failed > 0 { - slog.Warn("some inline comments could not be resolved", "failed", failed, "pr", prNumber) + if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil { + slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) + } else { + resolved++ } } + if resolved > 0 { + slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber) + } } } @@ -627,21 +617,34 @@ func extractSentinelName(body string) string { return rest[:end] } -// findOwnReview locates a review matching the given sentinel in its body. +// findOwnReview locates the most recent non-superseded review matching the sentinel. func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review { var best *gitea.Review for i := range reviews { if !strings.Contains(reviews[i].Body, sentinel) { continue } - // Skip superseded reviews (they contain our sentinel in the collapsed body) if strings.Contains(reviews[i].Body, "~~Original review~~") { continue } - // Take the highest ID (most recent) if best == nil || reviews[i].ID > best.ID { best = &reviews[i] } } return best } + +// findAllOwnReviews returns all non-superseded reviews matching the sentinel. +func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review { + var result []gitea.Review + for i := range reviews { + if !strings.Contains(reviews[i].Body, sentinel) { + continue + } + if strings.Contains(reviews[i].Body, "~~Original review~~") { + continue + } + result = append(result, reviews[i]) + } + return result +} diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index c822883..df1ce36 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -841,3 +841,24 @@ func cleanEnv() []string { } return env } + +func TestFindAllOwnReviews(t *testing.T) { + reviews := []gitea.Review{ + {ID: 1, Body: "\nfirst review"}, + {ID: 2, Body: "\nother bot"}, + {ID: 3, Body: "\nsecond review"}, + {ID: 4, Body: "~~Original review~~\n\nsuperseded"}, + {ID: 5, Body: "\nthird review"}, + } + + got := findAllOwnReviews(reviews, "") + if len(got) != 3 { + t.Fatalf("findAllOwnReviews() returned %d, want 3", len(got)) + } + wantIDs := []int64{1, 3, 5} + for i, r := range got { + if r.ID != wantIDs[i] { + t.Errorf("got[%d].ID = %d, want %d", i, r.ID, wantIDs[i]) + } + } +} diff --git a/gitea/client.go b/gitea/client.go index aa42f3b..333cf07 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -426,6 +426,65 @@ func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo str return 0, fmt.Errorf("no timeline event found with sentinel") } +// GetTimelineReviewCommentIDForReview finds the timeline comment ID for a +// specific review by matching its body content in the timeline. +func (c *Client) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, number int, reviewID int64) (int64, error) { + // Use the reviews API to get the review body, then find in timeline + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + number, + reviewID) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return 0, fmt.Errorf("get review %d: %w", reviewID, err) + } + var review struct { + Body string `json:"body"` + } + if err := json.Unmarshal(body, &review); err != nil { + return 0, fmt.Errorf("parse review %d: %w", reviewID, err) + } + if review.Body == "" { + return 0, fmt.Errorf("review %d has empty body", reviewID) + } + + // Use a prefix for matching (handles minor trailing whitespace differences) + matchPrefix := review.Body + if len(matchPrefix) > 200 { + matchPrefix = matchPrefix[:200] + } + + const pageSize = 50 + for page := 1; ; page++ { + timelineURL := 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) + tlBody, err := c.doGet(ctx, timelineURL) + if err != nil { + return 0, fmt.Errorf("get timeline (page %d): %w", page, err) + } + var events []TimelineEvent + if err := json.Unmarshal(tlBody, &events); err != nil { + return 0, fmt.Errorf("parse timeline (page %d): %w", page, err) + } + for _, ev := range events { + if ev.Type == "review" && strings.HasPrefix(ev.Body, matchPrefix) { + return ev.ID, nil + } + } + if len(events) < pageSize { + break + } + } + return 0, fmt.Errorf("no timeline event found for review %d", reviewID) +} + // 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", From a0fd882b0d2a8a7afee7664424350f9217d1dc00 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 13:31:59 -0700 Subject: [PATCH 2/2] fix: address review findings - Tighten timeline matching: also check ev.User.Login matches the review author (prevents collision on identical body prefix) - Remove unused sharedTokenMode variable (inline condition) - Aggregate resolution failures with warn-level summary --- cmd/review-bot/main.go | 14 ++++++++------ gitea/client.go | 5 ++++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index d125af8..777b6b4 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -320,17 +320,15 @@ func main() { // 2. Then supersede old review with link to the new one // Order matters: post first so we have the new review's URL for the supersede message. var oldReviews []gitea.Review - var sharedTokenMode bool if *reviewerName != "" { existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not list existing reviews", "pr", prNumber, "error", err) } else { - sharedTokenMode = hasSharedToken(existingReviews, sentinel) - if !sharedTokenMode { - oldReviews = findAllOwnReviews(existingReviews, sentinel) - } else { + if hasSharedToken(existingReviews, sentinel) { slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review") + } else { + oldReviews = findAllOwnReviews(existingReviews, sentinel) } } } @@ -378,13 +376,14 @@ func main() { slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err) continue } - resolved := 0 + resolved, failed := 0, 0 for _, c := range oldComments { if c.ID == 0 { continue } if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil { slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) + failed++ } else { resolved++ } @@ -392,6 +391,9 @@ func main() { if resolved > 0 { slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber) } + if failed > 0 { + slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber) + } } } diff --git a/gitea/client.go b/gitea/client.go index 333cf07..150b3c0 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -442,6 +442,9 @@ func (c *Client) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, } var review struct { Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` } if err := json.Unmarshal(body, &review); err != nil { return 0, fmt.Errorf("parse review %d: %w", reviewID, err) @@ -474,7 +477,7 @@ func (c *Client) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, return 0, fmt.Errorf("parse timeline (page %d): %w", page, err) } for _, ev := range events { - if ev.Type == "review" && strings.HasPrefix(ev.Body, matchPrefix) { + if ev.Type == "review" && ev.User.Login == review.User.Login && strings.HasPrefix(ev.Body, matchPrefix) { return ev.ID, nil } }