From b78d9972ac27888525d1b8c6ee71e9c225ce0e4c Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 07:04:33 -0700 Subject: [PATCH 1/4] 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 From fd179b891ba5ffd199adb93444f77bc8318be9e0 Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 07:11:57 -0700 Subject: [PATCH 2/4] fix: detect shared-token misconfiguration and warn MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When two review-bot roles share the same Gitea user token (misconfiguration), log a WARNING identifying which sibling is sharing. The bot continues normally with its own honest verdict — no escalation, no deadlock. Operators see the warning in CI logs and can fix the token setup. Addresses Aaron's review feedback on #28: graceful degradation when someone doesn't follow the separate-token deployment instructions. --- cmd/review-bot/main.go | 42 ++++++++++++++++++++++++ cmd/review-bot/main_test.go | 65 +++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index aed9ba4..daeef3f 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -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 + warnSharedToken(existingReviews, sentinel) + 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 +// 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, "" + 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 { diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 6633f10..2643eb7 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -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: "", + }, + { + name: "no own review yet", + reviews: []gitea.Review{ + {ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: " body"}, + }, + sentinel: "", + }, + { + name: "separate users - no warning", + 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: "", + }, + { + name: "shared token detected", + 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: "", + }, + } + + 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 + }{ + {" rest", "sonnet"}, + {" rest", "security"}, + {"no sentinel here", "unknown"}, + {" 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) + } + } +} From b1f5dd4b5f902260903cb033786a8a894e28767d Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 07:21:46 -0700 Subject: [PATCH 3/4] fix: skip update-in-place when shared token detected When hasSharedToken() detects two roles sharing the same Gitea user, the bot now skips ALL update logic (PATCH, supersede) and always POSTs a fresh review. This prevents clobbering a sibling's review body or state when misconfigured. Tests now assert return values (true/false) rather than just verifying no panic. Added additional test case for three-roles-same-user scenario. Addresses review feedback: update logic and review state must not interact with sibling reviews under the same user. --- cmd/review-bot/main.go | 83 ++++++++++++++++++++----------------- cmd/review-bot/main_test.go | 31 ++++++++++---- 2 files changed, 67 insertions(+), 47 deletions(-) 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) + } }) } } From ceefa4c2e0e450f67ccdd993e293812343477cdf Mon Sep 17 00:00:00 2001 From: Rodin Date: Sat, 2 May 2026 07:25:43 -0700 Subject: [PATCH 4/4] ci: use separate SECURITY_REVIEW_TOKEN for security reviewer The security-review-bot Gitea user now has its own token. This completes the token separation so each reviewer role posts under its own identity, enabling native Gitea multi-reviewer blocking. --- .gitea/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index b429c56..3c95082 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -33,7 +33,7 @@ jobs: token_secret: GPT_REVIEW_TOKEN model: gpt-4.1 - name: security - token_secret: SONNET_REVIEW_TOKEN + token_secret: SECURITY_REVIEW_TOKEN model: gpt-5 system_prompt_file: SECURITY_REVIEW.md steps: