diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index daeef3f..e6c0636 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -266,41 +266,45 @@ 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 - warnSharedToken(existingReviews, sentinel) + // Detect shared-token misconfiguration: if detected, skip all + // 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) { - 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 - } + if existing != nil { + if reviewUnchanged(existingReviews, reviewBody, event, sentinel) { + log.Printf("Review unchanged from previous run; skipping to preserve threads") + 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) + + // 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 { - 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 } -// warnSharedToken logs a warning if another review-bot role posted under the -// same Gitea user. This detects misconfiguration where two roles share a token -// 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) { +// hasSharedToken detects if another review-bot role posted under the same +// Gitea user. This indicates misconfiguration where two roles share a token +// instead of having separate Gitea accounts. Returns true if shared token +// detected (caller should skip update-in-place logic to avoid clobbering). +func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool { ownLogin := "" for _, r := range reviews { if strings.Contains(r.Body, ownSentinel) { @@ -503,14 +507,15 @@ func warnSharedToken(reviews []gitea.Review, ownSentinel string) { } } if ownLogin == "" { - return + return false } for _, r := range reviews { if r.User.Login == ownLogin && strings.Contains(r.Body, "", + want: false, }, { - name: "no own review yet", + name: "no own review yet - cannot detect", reviews: []gitea.Review{ {ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: " body"}, }, sentinel: "", + want: false, }, { - name: "separate users - no warning", + name: "separate users - no shared token", reviews: []gitea.Review{ {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, {ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: " body"}, }, sentinel: "", + want: false, }, { - name: "shared token detected", + name: "shared token detected - same user different sentinels", reviews: []gitea.Review{ {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, {ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, }, sentinel: "", + want: true, + }, + { + name: "three roles same user", + reviews: []gitea.Review{ + {ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, + {ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, + {ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, + }, + sentinel: "", + want: true, }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - // Should not panic - warnSharedToken(tc.reviews, tc.sentinel) + got := hasSharedToken(tc.reviews, tc.sentinel) + if got != tc.want { + t.Errorf("hasSharedToken() = %v, want %v", got, tc.want) + } }) } }