fix: remove worst-wins escalation logic #31

Merged
rodin merged 4 commits from fix/28-remove-escalation into main 2026-05-02 16:46:06 +00:00
2 changed files with 67 additions and 47 deletions
Showing only changes of commit b1f5dd4b5f - Show all commits
+44 -39
View File
@@ -266,41 +266,45 @@ 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 {
// Detect shared-token misconfiguration: warn if sibling sentinel exists from same user // Detect shared-token misconfiguration: if detected, skip all
Review

[MINOR] When shared-token mode is detected, the code skips all update-in-place logic but also skips checking reviewUnchanged. This can lead to duplicate identical reviews being posted under the same login and sentinel. Consider still performing a reviewUnchanged check and returning early to avoid re-posting in this misconfiguration scenario.

**[MINOR]** When shared-token mode is detected, the code skips all update-in-place logic but also skips checking reviewUnchanged. This can lead to duplicate identical reviews being posted under the same login and sentinel. Consider still performing a reviewUnchanged check and returning early to avoid re-posting in this misconfiguration scenario.
warnSharedToken(existingReviews, sentinel) // update logic (PATCH/supersede) to avoid clobbering a sibling's review.
sharedToken := hasSharedToken(existingReviews, sentinel)
if sharedToken {
log.Printf("Shared token mode: skipping update-in-place logic to avoid clobbering sibling review")
} else {
existing := findOwnReview(existingReviews, sentinel)
existing := findOwnReview(existingReviews, sentinel) if existing != nil {
if reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
if existing != nil { log.Printf("Review unchanged from previous run; skipping to preserve threads")
if reviewUnchanged(existingReviews, reviewBody, event, sentinel) { return
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 find review comment ID, falling back to new post: %v", err)
} else {
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 // Same state → PATCH in place
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) if existing.State == event {
if err != nil { commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
log.Printf("Warning: could not find old review comment ID: %v", err) if err != nil {
} else { log.Printf("Warning: could not find review comment ID, falling back to new post: %v", err)
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 { } else {
log.Printf("Marked old review as superseded (state was %s, now %s)", existing.State, event) 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)
}
} }
} }
} }
@@ -490,11 +494,11 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)
return false return false
Review

[MINOR] hasSharedToken does not filter out stale reviews when detecting shared-token misuse. If a stale sibling review from a prior misconfiguration remains under the same user, this may trigger a false positive and permanently skip update-in-place logic even after the configuration is fixed. Consider ignoring r.Stale in the detection loop.

**[MINOR]** hasSharedToken does not filter out stale reviews when detecting shared-token misuse. If a stale sibling review from a prior misconfiguration remains under the same user, this may trigger a false positive and permanently skip update-in-place logic even after the configuration is fixed. Consider ignoring r.Stale in the detection loop.
} }
// warnSharedToken logs a warning if another review-bot role posted under the // hasSharedToken detects if another review-bot role posted under the same
// same Gitea user. This detects misconfiguration where two roles share a token // Gitea user. This indicates misconfiguration where two roles share a token
// instead of having separate Gitea accounts. The bot continues normally (posts // instead of having separate Gitea accounts. Returns true if shared token
// its own honest verdict) but warns operators to fix the token setup. // detected (caller should skip update-in-place logic to avoid clobbering).
func warnSharedToken(reviews []gitea.Review, ownSentinel string) { func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool {
ownLogin := "" ownLogin := ""
for _, r := range reviews { for _, r := range reviews {
if strings.Contains(r.Body, ownSentinel) { if strings.Contains(r.Body, ownSentinel) {
@@ -503,14 +507,15 @@ func warnSharedToken(reviews []gitea.Review, ownSentinel string) {
} }
} }
if ownLogin == "" { if ownLogin == "" {
return return false
} }
for _, r := range reviews { for _, r := range reviews {
if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) { if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
log.Printf("WARNING: shared token detected — another review-bot role (%s) is using the same Gitea user %q. Each role should have its own token/user for proper multi-reviewer blocking.", extractSentinelName(r.Body), ownLogin) log.Printf("WARNING: shared token detected — another review-bot role (%s) is using the same Gitea user %q. Each role should have its own token/user for proper multi-reviewer blocking.", extractSentinelName(r.Body), ownLogin)
return return true
} }
} }
return false
} }
// extractSentinelName pulls the reviewer name from a sentinel comment. // extractSentinelName pulls the reviewer name from a sentinel comment.
+23 -8
View File
@@ -189,48 +189,63 @@ func TestFindOwnReview(t *testing.T) {
} }
} }
func TestWarnSharedToken(t *testing.T) { func TestHasSharedToken(t *testing.T) {
// warnSharedToken should not panic and should handle edge cases gracefully.
// It only logs — we verify it doesn't crash.
tests := []struct { tests := []struct {
name string name string
reviews []gitea.Review reviews []gitea.Review
sentinel string sentinel string
want bool
}{ }{
{ {
name: "no reviews", name: "no reviews",
reviews: nil, reviews: nil,
sentinel: "<!-- review-bot:sonnet -->", sentinel: "<!-- review-bot:sonnet -->",
want: false,
}, },
{ {
name: "no own review yet", name: "no own review yet - cannot detect",
reviews: []gitea.Review{ reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"}, {ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
}, },
sentinel: "<!-- review-bot:sonnet -->", sentinel: "<!-- review-bot:sonnet -->",
want: false,
}, },
{ {
name: "separate users - no warning", name: "separate users - no shared token",
reviews: []gitea.Review{ reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"}, {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"}, {ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
}, },
sentinel: "<!-- review-bot:sonnet -->", sentinel: "<!-- review-bot:sonnet -->",
want: false,
}, },
{ {
name: "shared token detected", name: "shared token detected - same user different sentinels",
reviews: []gitea.Review{ reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"}, {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"}, {ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
}, },
sentinel: "<!-- review-bot:sonnet -->", sentinel: "<!-- review-bot:sonnet -->",
want: true,
},
{
name: "three roles same user",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
{ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
}, },
} }
for _, tc := range tests { for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) { t.Run(tc.name, func(t *testing.T) {
// Should not panic got := hasSharedToken(tc.reviews, tc.sentinel)
warnSharedToken(tc.reviews, tc.sentinel) if got != tc.want {
t.Errorf("hasSharedToken() = %v, want %v", got, tc.want)
}
}) })
} }
} }