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 107 additions and 0 deletions
Showing only changes of commit fd179b891b - Show all commits
+42
View File
@@ -266,6 +266,9 @@ func main() {
if err != nil {
log.Printf("Warning: could not list existing reviews: %v", err)
} else {
// Detect shared-token misconfiguration: warn if sibling sentinel exists from same user
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)
Outdated
Review

this should also make it fall into a mode where it only writes new reviews; never updates any review as we don't want to do that logic. What about review state? That might get clobbered too.

this should also make it fall into a mode where it only writes new reviews; never updates any review as we don't want to do that logic. What about review state? That might get clobbered too.
existing := findOwnReview(existingReviews, sentinel)
if existing != nil {
@@ -487,6 +490,45 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)
return false
}
// warnSharedToken logs a warning if another review-bot role posted under the
// same Gitea user. This detects misconfiguration where two roles share a token
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.
// instead of having separate Gitea accounts. The bot continues normally (posts
// its own honest verdict) but warns operators to fix the token setup.
func warnSharedToken(reviews []gitea.Review, ownSentinel string) {
ownLogin := ""
for _, r := range reviews {
if strings.Contains(r.Body, ownSentinel) {
ownLogin = r.User.Login
break
}
}
if ownLogin == "" {
return
}
for _, r := range reviews {
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)
return
}
}
}
// extractSentinelName pulls the reviewer name from a sentinel comment.
func extractSentinelName(body string) string {
const prefix = "<!-- review-bot:"
const suffix = " -->"
idx := strings.Index(body, prefix)
if idx < 0 {
return "unknown"
}
rest := body[idx+len(prefix):]
end := strings.Index(rest, suffix)
if end < 0 {
return "unknown"
}
return rest[:end]
}
// findOwnReview locates a review matching the given sentinel in its body.
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
for i := range reviews {
+65
View File
@@ -188,3 +188,68 @@ func TestFindOwnReview(t *testing.T) {
})
}
}
func TestWarnSharedToken(t *testing.T) {
// warnSharedToken should not panic and should handle edge cases gracefully.
// It only logs — we verify it doesn't crash.
tests := []struct {
name string
reviews []gitea.Review
sentinel string
}{
{
name: "no reviews",
reviews: nil,
sentinel: "<!-- review-bot:sonnet -->",
},
{
name: "no own review yet",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
},
{
name: "separate users - no warning",
reviews: []gitea.Review{
{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"},
},
sentinel: "<!-- review-bot:sonnet -->",
},
{
name: "shared token detected",
reviews: []gitea.Review{
{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"},
},
sentinel: "<!-- review-bot:sonnet -->",
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
// Should not panic
warnSharedToken(tc.reviews, tc.sentinel)
})
}
}
func TestExtractSentinelName(t *testing.T) {
tests := []struct {
body string
want string
}{
{"<!-- review-bot:sonnet --> rest", "sonnet"},
{"<!-- review-bot:security --> rest", "security"},
{"no sentinel here", "unknown"},
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
}
for _, tc := range tests {
got := extractSentinelName(tc.body)
if got != tc.want {
t.Errorf("extractSentinelName(%q) = %q, want %q", tc.body, got, tc.want)
}
}
}