fix: remove worst-wins escalation logic (#28)
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
This commit is contained in:
@@ -266,16 +266,7 @@ func main() {
|
|||||||
if err != nil {
|
if err != nil {
|
||||||
log.Printf("Warning: could not list existing reviews: %v", err)
|
log.Printf("Warning: could not list existing reviews: %v", err)
|
||||||
} else {
|
} else {
|
||||||
// Worst-wins: escalate if a sibling blocks (need own login from existing review)
|
|
||||||
ownLogin := ""
|
|
||||||
existing := findOwnReview(existingReviews, sentinel)
|
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 existing != nil {
|
||||||
if reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
|
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)
|
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.
|
// fetchFileContext fetches the full content of modified files from the PR branch.
|
||||||
@@ -501,26 +469,6 @@ func validateReviewerName(name string) error {
|
|||||||
return nil
|
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, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
|
|
||||||
return true
|
|
||||||
}
|
|
||||||
}
|
|
||||||
return false
|
|
||||||
}
|
|
||||||
|
|
||||||
// reviewUnchanged checks if an existing review with the same sentinel
|
// reviewUnchanged checks if an existing review with the same sentinel
|
||||||
// already has identical body and state. Returns true if a re-post would
|
// already has identical body and state. Returns true if a re-post would
|
||||||
// produce the same result (skip to preserve conversation threads).
|
// produce the same result (skip to preserve conversation threads).
|
||||||
|
|||||||
@@ -50,106 +50,6 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
|
|||||||
return r
|
return r
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestShouldEscalate(t *testing.T) {
|
|
||||||
tests := []struct {
|
|
||||||
name string
|
|
||||||
reviews []gitea.Review
|
|
||||||
postedID int64
|
|
||||||
ownLogin string
|
|
||||||
|
|
||||||
ownSentinel string
|
|
||||||
want bool
|
|
||||||
}{
|
|
||||||
{
|
|
||||||
name: "no reviews",
|
|
||||||
reviews: nil,
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
want: false,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "sibling same user has REQUEST_CHANGES",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:security -->"),
|
|
||||||
},
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
want: true,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "sibling different user has REQUEST_CHANGES (should NOT escalate)",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:gpt -->"),
|
|
||||||
},
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
want: false,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "same user REQUEST_CHANGES but stale (should NOT escalate)",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n<!-- review-bot:security -->"),
|
|
||||||
},
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
want: false,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "same user same sentinel (own stale review, should NOT escalate)",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n<!-- review-bot:sonnet -->"),
|
|
||||||
},
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
want: false,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "same user APPROVED sibling (should NOT escalate)",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(101, "bot", "APPROVED", false, "good\n<!-- review-bot:security -->"),
|
|
||||||
},
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
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: "<!-- review-bot:sonnet -->",
|
|
||||||
want: false,
|
|
||||||
},
|
|
||||||
{
|
|
||||||
name: "skip own posted ID",
|
|
||||||
reviews: []gitea.Review{
|
|
||||||
makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n<!-- review-bot:security -->"),
|
|
||||||
},
|
|
||||||
postedID: 100,
|
|
||||||
ownLogin: "bot",
|
|
||||||
ownSentinel: "<!-- review-bot:sonnet -->",
|
|
||||||
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) {
|
func TestReviewUnchanged(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
|
|||||||
Reference in New Issue
Block a user