fix: supersede ALL old reviews, not just the most recent #43

Merged
rodin merged 2 commits from fix/supersede-all-old-reviews into main 2026-05-02 20:35:23 +00:00
3 changed files with 138 additions and 50 deletions
+55 -50
View File
@@ -319,27 +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
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 {
Outdated
Review

[NIT] sharedTokenMode is only used to gate collection of oldReviews and to log a warning; the variable itself isn't used later. You could simplify by inlining the condition or removing the variable to reduce local state.

**[NIT]** sharedTokenMode is only used to gate collection of oldReviews and to log a warning; the variable itself isn't used later. You could simplify by inlining the condition or removing the variable to reduce local state.
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
}
}
} else {
if hasSharedToken(existingReviews, sentinel) {
slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review")
} else {
oldReviews = findAllOwnReviews(existingReviews, sentinel)
}
}
}
@@ -365,43 +354,46 @@ 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, failed := 0, 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)
Outdated
Review

[NIT] When resolving old inline comments, failures are only logged at debug level per comment and no summary of failures is emitted. Consider aggregating and logging a warning if some resolutions failed to aid observability.

**[NIT]** When resolving old inline comments, failures are only logged at debug level per comment and no summary of failures is emitted. Consider aggregating and logging a warning if some resolutions failed to aid observability.
failed++
} else {
resolved++
}
}
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)
}
}
}
@@ -627,21 +619,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.
Review

[NIT] findAllOwnReviews returns a slice of gitea.Review (by value), which copies potentially large Body strings. Returning []*gitea.Review would avoid copying and align with findOwnReview's pointer return; not critical but can reduce allocations for large sets of reviews.

**[NIT]** findAllOwnReviews returns a slice of gitea.Review (by value), which copies potentially large Body strings. Returning []*gitea.Review would avoid copying and align with findOwnReview's pointer return; not critical but can reduce allocations for large sets of reviews.
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
}
+21
View File
@@ -841,3 +841,24 @@ func cleanEnv() []string {
}
return env
}
func TestFindAllOwnReviews(t *testing.T) {
reviews := []gitea.Review{
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
{ID: 4, Body: "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"},
{ID: 5, Body: "<!-- review-bot:sonnet -->\nthird review"},
}
got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
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])
}
}
}
+62
View File
@@ -426,6 +426,68 @@ func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo str
return 0, fmt.Errorf("no timeline event found with sentinel")
Review

[MINOR] GetTimelineReviewCommentIDForReview matches timeline events by checking strings.HasPrefix on the first ~200 bytes of a review body. In the unlikely case of two reviews with identical prefixes, this could target the wrong timeline comment for editing, potentially modifying another review's summary. Consider tightening the match (e.g., include a unique sentinel in the prefix or match full body with a fallback) to reduce mis-targeting risk.

**[MINOR]** GetTimelineReviewCommentIDForReview matches timeline events by checking strings.HasPrefix on the first ~200 bytes of a review body. In the unlikely case of two reviews with identical prefixes, this could target the wrong timeline comment for editing, potentially modifying another review's summary. Consider tightening the match (e.g., include a unique sentinel in the prefix or match full body with a fallback) to reduce mis-targeting risk.
}
// 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"`
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)
}
if review.Body == "" {
Review

[NIT] For each old review, the code scans the entire issue timeline to find the matching comment ID. If many stale reviews accumulate, this results in repeated timeline fetches. Consider fetching the timeline once and mapping bodies/prefixes to IDs to reduce API calls.

**[NIT]** For each old review, the code scans the entire issue timeline to find the matching comment ID. If many stale reviews accumulate, this results in repeated timeline fetches. Consider fetching the timeline once and mapping bodies/prefixes to IDs to reduce API calls.
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]
}
Review

[MINOR] GetTimelineReviewCommentIDForReview matches timeline events by user and a 200-char prefix of the review body. If multiple historical reviews have identical beginnings (common for templated bot reviews), this can select the wrong timeline event, leading to editing one review's timeline comment while resolving another review's inline comments. Consider strengthening the match (e.g., also require the presence of the short commit SHA in ev.Body, compare a longer/hash-based signature, or disambiguate among multiple matches deterministically).

**[MINOR]** GetTimelineReviewCommentIDForReview matches timeline events by user and a 200-char prefix of the review body. If multiple historical reviews have identical beginnings (common for templated bot reviews), this can select the wrong timeline event, leading to editing one review's timeline comment while resolving another review's inline comments. Consider strengthening the match (e.g., also require the presence of the short commit SHA in ev.Body, compare a longer/hash-based signature, or disambiguate among multiple matches deterministically).
const pageSize = 50
for page := 1; ; page++ {
Review

[MINOR] GetTimelineReviewCommentIDForReview matches timeline events using only a HasPrefix on the review body. If multiple reviews share identical leading content, this could match the wrong event. Consider also verifying the event's user (ev.User.Login) and/or checking for the sentinel substring to reduce false positives.

**[MINOR]** GetTimelineReviewCommentIDForReview matches timeline events using only a HasPrefix on the review body. If multiple reviews share identical leading content, this could match the wrong event. Consider also verifying the event's user (ev.User.Login) and/or checking for the sentinel substring to reduce false positives.
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" && ev.User.Login == review.User.Login && 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",