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

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

@@ -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:

cmd/review-bot/main.go

@@ -266,16 +266,13 @@ 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 := ""
+			// 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 {
-				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) {
@@ -313,6 +310,7 @@ func main() {
 				}
 			}
 		}
+	}
 
 	// POST new review (first run, or state transition fallthrough)
 	log.Printf("Posting review (event=%s)...", 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 {

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: "<!-- 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)
+		}
+	}
+}

gitea/client.go

@@ -66,7 +66,7 @@ type ReviewComment struct {
 
 // GetPullRequest fetches PR metadata.
 func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch PR: %w", err)
@@ -80,7 +80,7 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number
 
 // GetPullRequestDiff fetches the unified diff for a PR.
 func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch diff: %w", err)
@@ -90,7 +90,7 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
 
 // GetPullRequestFiles fetches the list of files changed in a PR.
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch PR files: %w", err)
@@ -104,7 +104,7 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
 
 // GetCommitStatuses fetches CI statuses for a commit SHA.
 func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, owner, repo, sha)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return nil, fmt.Errorf("fetch commit statuses: %w", err)
@@ -118,7 +118,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
 
 // GetFileContent fetches a file from the default branch of a repo.
 func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, owner, repo, escapePath(filepath))
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch file %s: %w", filepath, err)
@@ -128,7 +128,7 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath strin
 
 // GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo.
 func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, owner, repo, escapePath(filepath), url.QueryEscape(ref))
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath), url.QueryEscape(ref))
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch file %s@%s: %w", filepath, ref, err)
@@ -140,7 +140,7 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
 // event should be "APPROVED" or "REQUEST_CHANGES".
 // comments are optional inline comments attached to specific lines.
 func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
-	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number)
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 
 	payload := struct {
 		Body     string          `json:"body"`
@@ -230,9 +230,9 @@ type ContentEntry struct {
 func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
 	var reqURL string
 	if path == "" {
-		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, owner, repo)
+		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
 	} else {
-		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path))
+		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
 	}
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {