From b78d9972ac27888525d1b8c6ee71e9c225ce0e4c Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 07:04:33 -0700 Subject: [PATCH] fix: remove worst-wins escalation logic (#28) --- cmd/review-bot/main.go | 52 ------------------- cmd/review-bot/main_test.go | 100 ------------------------------------ 2 files changed, 152 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 8163ea1..aed9ba4 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -266,16 +266,7 @@ func main() { if err != nil { log.Printf("Warning: could not list existing reviews: %v", err) } else { - // 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" - } if existing != nil { if reviewUnchanged(existingReviews, reviewBody, event, sentinel) { @@ -322,29 +313,6 @@ func main() { } log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) - // Post-posting escalation: if we just posted APPROVED but a sibling - // from the same user has REQUEST_CHANGES, mark ours as superseded and - // re-post as REQUEST_CHANGES. This handles the first-run case where - // we don't know our login until after posting. - if event == "APPROVED" && *updateExisting && *reviewerName != "" { - reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) - if err == nil && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) { - log.Printf("Post-posting escalation: sibling has REQUEST_CHANGES") - // Mark our just-posted review as superseded - commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) - if err == nil { - supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel) - giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody) - } - // Re-post as REQUEST_CHANGES - _, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody, inlineComments) - if err != nil { - log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err) - } else { - log.Printf("Review escalated to REQUEST_CHANGES") - } - } - } } // fetchFileContext fetches the full content of modified files from the PR branch. @@ -501,26 +469,6 @@ func validateReviewerName(name string) error { return nil } -// 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 { - continue - } - // Sibling = same user, has a review-bot sentinel, but not OUR sentinel - if r.User.Login == ownLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "", - want: false, - }, - { - name: "sibling same user has REQUEST_CHANGES", - reviews: []gitea.Review{ - makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: true, - }, - { - name: "sibling different user has REQUEST_CHANGES (should NOT escalate)", - reviews: []gitea.Review{ - makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: false, - }, - { - name: "same user REQUEST_CHANGES but stale (should NOT escalate)", - reviews: []gitea.Review{ - makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: false, - }, - { - name: "same user same sentinel (own stale review, should NOT escalate)", - reviews: []gitea.Review{ - makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: false, - }, - { - name: "same user APPROVED sibling (should NOT escalate)", - reviews: []gitea.Review{ - makeReview(101, "bot", "APPROVED", false, "good\n"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: false, - }, - { - name: "human REQUEST_CHANGES no sentinel (should NOT escalate)", - reviews: []gitea.Review{ - makeReview(101, "bot", "REQUEST_CHANGES", false, "please fix this"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: false, - }, - { - name: "skip own posted ID", - reviews: []gitea.Review{ - makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n"), - }, - postedID: 100, - ownLogin: "bot", - ownSentinel: "", - want: false, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := shouldEscalate(tc.reviews, tc.postedID, tc.ownLogin, tc.ownSentinel) - if got != tc.want { - t.Errorf("shouldEscalate() = %v, want %v", got, tc.want) - } - }) - } -} - func TestReviewUnchanged(t *testing.T) { tests := []struct { name string