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: diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 8163ea1..e6c0636 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -266,47 +266,45 @@ 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" - } + // 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) - 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) + } } } } @@ -322,29 +320,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 +476,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, "" + 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 4c0e7fd..7c96fcd 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -50,106 +50,6 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re 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: "", - 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 @@ -288,3 +188,83 @@ func TestFindOwnReview(t *testing.T) { }) } } + +func TestHasSharedToken(t *testing.T) { + tests := []struct { + name string + reviews []gitea.Review + sentinel string + want bool + }{ + { + name: "no reviews", + reviews: nil, + sentinel: "", + want: false, + }, + { + 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 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 - 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) { + got := hasSharedToken(tc.reviews, tc.sentinel) + if got != tc.want { + t.Errorf("hasSharedToken() = %v, want %v", got, tc.want) + } + }) + } +} + +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) + } + } +}