Merge pull request 'fix: remove worst-wins escalation logic' (#31) from fix/28-remove-escalation into main

2026-05-02 16:46:05 +00:00
parent 3c785c5502 ceefa4c2e0
commit b51a19d8b9
3 changed files with 157 additions and 182 deletions
@@ -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:
@@ -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, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
-			return true
-		}
-	}
-	return false
-}
-
 // reviewUnchanged checks if an existing review with the same sentinel
 // already has identical body and state. Returns true if a re-post would
 // produce the same result (skip to preserve conversation threads).
@@ -539,6 +494,46 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)
 	return false
 }

+// 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) {
+			ownLogin = r.User.Login
+			break
+		}
+	}
+	if ownLogin == "" {
+		return false
+	}
+	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 true
+		}
+	}
+	return false
+}
+
+// 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 {
@@ -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: "<!-- review-bot:sonnet -->",
-			want:        false,
-		},
-		{
-			name: "sibling same user has REQUEST_CHANGES",
-			reviews: []gitea.Review{
-				makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:security -->"),
-			},
-			postedID:    100,
-			ownLogin:    "bot",
-			ownSentinel: "<!-- review-bot:sonnet -->",
-			want:        true,
-		},
-		{
-			name: "sibling different user has REQUEST_CHANGES (should NOT escalate)",
-			reviews: []gitea.Review{
-				makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:gpt -->"),
-			},
-			postedID:    100,
-			ownLogin:    "bot",
-			ownSentinel: "<!-- review-bot:sonnet -->",
-			want:        false,
-		},
-		{
-			name: "same user REQUEST_CHANGES but stale (should NOT escalate)",
-			reviews: []gitea.Review{
-				makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n<!-- review-bot:security -->"),
-			},
-			postedID:    100,
-			ownLogin:    "bot",
-			ownSentinel: "<!-- review-bot:sonnet -->",
-			want:        false,
-		},
-		{
-			name: "same user same sentinel (own stale review, should NOT escalate)",
-			reviews: []gitea.Review{
-				makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n<!-- review-bot:sonnet -->"),
-			},
-			postedID:    100,
-			ownLogin:    "bot",
-			ownSentinel: "<!-- review-bot:sonnet -->",
-			want:        false,
-		},
-		{
-			name: "same user APPROVED sibling (should NOT escalate)",
-			reviews: []gitea.Review{
-				makeReview(101, "bot", "APPROVED", false, "good\n<!-- review-bot:security -->"),
-			},
-			postedID:    100,
-			ownLogin:    "bot",
-			ownSentinel: "<!-- review-bot:sonnet -->",
-			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: "<!-- review-bot:sonnet -->",
-			want:        false,
-		},
-		{
-			name: "skip own posted ID",
-			reviews: []gitea.Review{
-				makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n<!-- review-bot:security -->"),
-			},
-			postedID:    100,
-			ownLogin:    "bot",
-			ownSentinel: "<!-- review-bot:sonnet -->",
-			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: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+		{
+			name: "no own review yet - cannot detect",
+			reviews: []gitea.Review{
+				{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
+			},
+			sentinel: "<!-- review-bot:sonnet -->",
+			want:     false,
+		},
+		{
+			name: "separate users - no shared token",
+			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 -->",
+			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: "<!-- 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 -->",
+			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 {
+		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
+	}{
+		{"<!-- 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)
+		}
+	}
+}