Merge pull request 'fix: supersede ALL old reviews, not just the most recent' (#43) from fix/supersede-all-old-reviews into main

- Tighten timeline matching: also check ev.User.Login matches
  the review author (prevents collision on identical body prefix)
- Remove unused sharedTokenMode variable (inline condition)
- Aggregate resolution failures with warn-level summary

cmd/review-bot/main.go

@@ -319,31 +319,32 @@ func main() {
 	// 1. POST new review first (gets non-stale approval badge on HEAD)
 	// 2. Then supersede old review with link to the new one
 	// Order matters: post first so we have the new review's URL for the supersede message.
-	var existingReview *gitea.Review
-	var existingCommentID int64
+	var oldReviews []gitea.Review
 	if *reviewerName != "" {
 		existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
 		if err != nil {
 			slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
 		} else {
-			sharedToken := hasSharedToken(existingReviews, sentinel)
-			if !sharedToken {
-				existingReview = findOwnReview(existingReviews, sentinel)
-				if existingReview != nil {
-					cid, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
-					if err != nil {
-						slog.Warn("could not find old review comment ID for supersede", "error", err)
-						existingReview = nil // can't supersede without comment ID
-					} else {
-						existingCommentID = cid
-					}
-				}
-			} else {
+			if hasSharedToken(existingReviews, sentinel) {
 				slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review")
+			} else {
+				oldReviews = findAllOwnReviews(existingReviews, sentinel)
 			}
 		}
 	}
 
+	// Self-request as reviewer (ensures we appear in required-reviewer checks)
+	authUser, err := giteaClient.GetAuthenticatedUser(ctx)
+	if err != nil {
+		slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
+	} else if authUser != "" {
+		if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
+			slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
+		} else {
+			slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
+		}
+	}
+
 	// POST new review
 	slog.Info("posting review", "event", event, "pr", prNumber)
 	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
@@ -353,39 +354,46 @@ func main() {
 	}
 	slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
 
-	// Supersede old review with link to the new one
-	if existingReview != nil && existingCommentID > 0 {
+	// Supersede all old reviews with link to the new one
+	if len(oldReviews) > 0 {
 		newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID)
-		supersededBody := buildSupersededBody(existingReview.Body, existingReview.CommitID, newReviewURL, sentinel)
-		supersedeOK := false
-		if err := giteaClient.EditComment(ctx, owner, repoName, existingCommentID, supersededBody); err != nil {
-			slog.Warn("could not mark old review as superseded", "comment_id", existingCommentID, "error", err)
-		} else {
-			slog.Info("marked old review as superseded", "old_state", existingReview.State, "new_review_id", posted.ID, "pr", prNumber)
-			supersedeOK = true
-		}
-
-		// Resolve old review's inline comments only after successful supersede
-		if supersedeOK {
-			oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, existingReview.ID)
+		for _, oldReview := range oldReviews {
+			cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
 			if err != nil {
-				slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err)
-			} else {
-				resolved := 0
-				for _, c := range oldComments {
-					if c.ID == 0 {
-						continue
-					}
-					if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
-						slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
-					} else {
-						resolved++
-					}
+				slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
+				continue
+			}
+			supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
+			if err := giteaClient.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil {
+				slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
+				continue
+			}
+			slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber)
+
+			// Resolve old review's inline comments
+			oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID)
+			if err != nil {
+				slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
+				continue
+			}
+			resolved, failed := 0, 0
+			for _, c := range oldComments {
+				if c.ID == 0 {
+					continue
 				}
-				if resolved > 0 {
-					slog.Info("resolved old inline comments", "count", resolved, "pr", prNumber)
+				if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
+					slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
+					failed++
+				} else {
+					resolved++
 				}
 			}
+			if resolved > 0 {
+				slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
+			}
+			if failed > 0 {
+				slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
+			}
 		}
 	}
 
@@ -611,21 +619,34 @@ func extractSentinelName(body string) string {
 	return rest[:end]
 }
 
-// findOwnReview locates a review matching the given sentinel in its body.
+// findOwnReview locates the most recent non-superseded review matching the sentinel.
 func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
 	var best *gitea.Review
 	for i := range reviews {
 		if !strings.Contains(reviews[i].Body, sentinel) {
 			continue
 		}
-		// Skip superseded reviews (they contain our sentinel in the collapsed body)
 		if strings.Contains(reviews[i].Body, "~~Original review~~") {
 			continue
 		}
-		// Take the highest ID (most recent)
 		if best == nil || reviews[i].ID > best.ID {
 			best = &reviews[i]
 		}
 	}
 	return best
 }
+
+// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
+func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
+	var result []gitea.Review
+	for i := range reviews {
+		if !strings.Contains(reviews[i].Body, sentinel) {
+			continue
+		}
+		if strings.Contains(reviews[i].Body, "~~Original review~~") {
+			continue
+		}
+		result = append(result, reviews[i])
+	}
+	return result
+}

cmd/review-bot/main_test.go

@@ -841,3 +841,24 @@ func cleanEnv() []string {
 	}
 	return env
 }
+
+func TestFindAllOwnReviews(t *testing.T) {
+	reviews := []gitea.Review{
+		{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
+		{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
+		{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
+		{ID: 4, Body: "~~Original review~~\n<!-- review-bot:sonnet -->\nsuperseded"},
+		{ID: 5, Body: "<!-- review-bot:sonnet -->\nthird review"},
+	}
+
+	got := findAllOwnReviews(reviews, "<!-- review-bot:sonnet -->")
+	if len(got) != 3 {
+		t.Fatalf("findAllOwnReviews() returned %d, want 3", len(got))
+	}
+	wantIDs := []int64{1, 3, 5}
+	for i, r := range got {
+		if r.ID != wantIDs[i] {
+			t.Errorf("got[%d].ID = %d, want %d", i, r.ID, wantIDs[i])
+		}
+	}
+}

gitea/client.go

@@ -426,6 +426,68 @@ func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo str
 	return 0, fmt.Errorf("no timeline event found with sentinel")
 }
 
+// GetTimelineReviewCommentIDForReview finds the timeline comment ID for a
+// specific review by matching its body content in the timeline.
+func (c *Client) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, number int, reviewID int64) (int64, error) {
+	// Use the reviews API to get the review body, then find in timeline
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d",
+		c.baseURL,
+		url.PathEscape(owner),
+		url.PathEscape(repo),
+		number,
+		reviewID)
+	body, err := c.doGet(ctx, reqURL)
+	if err != nil {
+		return 0, fmt.Errorf("get review %d: %w", reviewID, err)
+	}
+	var review struct {
+		Body string `json:"body"`
+		User struct {
+			Login string `json:"login"`
+		} `json:"user"`
+	}
+	if err := json.Unmarshal(body, &review); err != nil {
+		return 0, fmt.Errorf("parse review %d: %w", reviewID, err)
+	}
+	if review.Body == "" {
+		return 0, fmt.Errorf("review %d has empty body", reviewID)
+	}
+
+	// Use a prefix for matching (handles minor trailing whitespace differences)
+	matchPrefix := review.Body
+	if len(matchPrefix) > 200 {
+		matchPrefix = matchPrefix[:200]
+	}
+
+	const pageSize = 50
+	for page := 1; ; page++ {
+		timelineURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?limit=%d&page=%d",
+			c.baseURL,
+			url.PathEscape(owner),
+			url.PathEscape(repo),
+			number,
+			pageSize,
+			page)
+		tlBody, err := c.doGet(ctx, timelineURL)
+		if err != nil {
+			return 0, fmt.Errorf("get timeline (page %d): %w", page, err)
+		}
+		var events []TimelineEvent
+		if err := json.Unmarshal(tlBody, &events); err != nil {
+			return 0, fmt.Errorf("parse timeline (page %d): %w", page, err)
+		}
+		for _, ev := range events {
+			if ev.Type == "review" && ev.User.Login == review.User.Login && strings.HasPrefix(ev.Body, matchPrefix) {
+				return ev.ID, nil
+			}
+		}
+		if len(events) < pageSize {
+			break
+		}
+	}
+	return 0, fmt.Errorf("no timeline event found for review %d", reviewID)
+}
+
 // EditComment updates the body of an issue/review comment.
 func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/comments/%d",
@@ -462,6 +524,59 @@ func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID
 	return nil
 }
 
+// GetAuthenticatedUser returns the login of the user authenticated by the token.
+func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
+	reqURL := fmt.Sprintf("%s/api/v1/user", c.baseURL)
+	body, err := c.doGet(ctx, reqURL)
+	if err != nil {
+		return "", fmt.Errorf("get authenticated user: %w", err)
+	}
+	var result struct {
+		Login string `json:"login"`
+	}
+	if err := json.Unmarshal(body, &result); err != nil {
+		return "", fmt.Errorf("parse user response: %w", err)
+	}
+	return result.Login, nil
+}
+
+// RequestReviewer adds the given user as a requested reviewer on a pull request.
+// This is idempotent — requesting an already-requested reviewer is a no-op.
+func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/requested_reviewers",
+		c.baseURL,
+		url.PathEscape(owner),
+		url.PathEscape(repo),
+		number)
+
+	payload := struct {
+		Reviewers []string `json:"reviewers"`
+	}{Reviewers: []string{reviewer}}
+	data, err := json.Marshal(payload)
+	if err != nil {
+		return fmt.Errorf("marshal reviewer request: %w", err)
+	}
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
+	if err != nil {
+		return fmt.Errorf("create reviewer request: %w", err)
+	}
+	req.Header.Set("Authorization", "token "+c.token)
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return fmt.Errorf("request reviewer: %w", err)
+	}
+	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
+		body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
+		return fmt.Errorf("request reviewer failed (status %d): %s", resp.StatusCode, body)
+	}
+	return nil
+}
+
 // ListReviewComments returns the inline comments attached to a specific review.
 // Paginates through all pages.
 func (c *Client) ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]ReviewComment, error) {
@@ -513,7 +628,7 @@ func (c *Client) ResolveComment(ctx context.Context, owner, repo string, comment
 	defer resp.Body.Close()
 
 	if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
-		body, _ := io.ReadAll(resp.Body)
+		body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
 		return fmt.Errorf("resolve comment failed (status %d): %s", resp.StatusCode, body)
 	}
 	return nil

gitea/client_test.go

@@ -5,8 +5,10 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 )
 
@@ -603,8 +605,91 @@ func TestIsNotFound(t *testing.T) {
 	}
 }
 
+func TestGetAuthenticatedUser(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/api/v1/user" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+			http.NotFound(w, r)
+			return
+		}
+		if r.Header.Get("Authorization") != "token test-token" {
+			t.Error("missing or wrong auth header")
+		}
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprint(w, `{"login":"my-bot","id":42}`)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	login, err := client.GetAuthenticatedUser(context.Background())
+	if err != nil {
+		t.Fatalf("GetAuthenticatedUser() error = %v", err)
+	}
+	if login != "my-bot" {
+		t.Errorf("login = %q, want %q", login, "my-bot")
+	}
+}
+
+func TestRequestReviewer(t *testing.T) {
+	var gotBody []byte
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != http.MethodPost {
+			t.Errorf("expected POST, got %s", r.Method)
+		}
+		expected := "/api/v1/repos/owner/repo/pulls/7/requested_reviewers"
+		if r.URL.Path != expected {
+			t.Errorf("path = %q, want %q", r.URL.Path, expected)
+		}
+		gotBody, _ = io.ReadAll(r.Body)
+		w.WriteHeader(http.StatusCreated)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.RequestReviewer(context.Background(), "owner", "repo", 7, "bot-user")
+	if err != nil {
+		t.Fatalf("RequestReviewer() error = %v", err)
+	}
+	if !strings.Contains(string(gotBody), `"bot-user"`) {
+		t.Errorf("body = %s, want to contain bot-user", gotBody)
+	}
+}
+
+func TestRequestReviewer_204(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNoContent)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
+	if err != nil {
+		t.Fatalf("RequestReviewer() should accept 204, got error = %v", err)
+	}
+}
+
+func TestRequestReviewer_Error(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusForbidden)
+		fmt.Fprint(w, "no permission")
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
+	if err == nil {
+		t.Fatal("expected error for 403 response")
+	}
+	if !strings.Contains(err.Error(), "403") {
+		t.Errorf("error should mention status code: %v", err)
+	}
+}
+
 func TestListReviewComments(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if !strings.Contains(r.URL.Path, "/pulls/1/reviews/42/comments") {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
 		w.Header().Set("Content-Type", "application/json")
 		fmt.Fprint(w, `[{"id":100,"path":"main.go","new_position":5,"body":"finding"},{"id":101,"path":"lib.go","new_position":10,"body":"another"}]`)
 	}))
@@ -631,9 +716,8 @@ func TestResolveComment(t *testing.T) {
 		if r.Method != http.MethodPost {
 			t.Errorf("expected POST, got %s", r.Method)
 		}
-		expected := "/api/v1/repos/owner/repo/pulls/comments/99/resolve"
-		if r.URL.Path != expected {
-			t.Errorf("path = %q, want %q", r.URL.Path, expected)
+		if !strings.Contains(r.URL.Path, "/pulls/comments/99/resolve") {
+			t.Errorf("unexpected path: %s", r.URL.Path)
 		}
 		w.WriteHeader(http.StatusOK)
 	}))