Merge pull request 'feat: resolve old inline comments when superseding review' (#42) from feat/27-resolve-inline-comments into main

Closes #27

After superseding an old review, resolves all its inline comments via
POST /pulls/comments/{id}/resolve. This clears unresolved conversation
markers from the PR timeline and diff view.

New API methods:
- ListReviewComments: paginated GET /repos/.../pulls/{n}/reviews/{id}/comments
- ResolveComment: POST /repos/.../pulls/comments/{id}/resolve

Behavior:
- Only resolves after successful supersede (gated on supersedeOK)
- Aggregates failures and logs at warn level
- Truncates error bodies to 256 bytes (security)
- Non-fatal: review still posts even if resolution fails

cmd/review-bot/main.go

@@ -316,29 +316,26 @@ func main() {
 	}
 
 	// --- Review update strategy ---
-	// Always post a fresh review on the current commit. If a previous review
-	// exists, mark it as superseded (preserves old threads as navigable history).
-	// Never skip posting — the fresh review must land on HEAD to clear stale badges.
+	// 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
 	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 {
-			// In shared-token mode, skip superseding to avoid clobbering sibling reviews.
 			sharedToken := hasSharedToken(existingReviews, sentinel)
 			if !sharedToken {
-				existing := findOwnReview(existingReviews, sentinel)
-				if existing != nil {
-					commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
+				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 {
-						supersededBody := buildSupersededBody(existing.Body, existing.CommitID, sentinel)
-						if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil {
-							slog.Warn("could not mark old review as superseded", "comment_id", commentID, "error", err)
-						} else {
-							slog.Info("marked old review as superseded", "old_state", existing.State, "pr", prNumber)
-						}
+						existingCommentID = cid
 					}
 				}
 			} else {
@@ -347,7 +344,19 @@ func main() {
 		}
 	}
 
-	// POST new review (first run, or state transition fallthrough)
+	// 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)
 	if err != nil {
@@ -356,6 +365,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 {
+		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)
+			if err != nil {
+				slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err)
+			} else {
+				resolved, failed := 0, 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)
+						failed++
+					} else {
+						resolved++
+					}
+				}
+				if resolved > 0 {
+					slog.Info("resolved old inline comments", "count", resolved, "pr", prNumber)
+				}
+				if failed > 0 {
+					slog.Warn("some inline comments could not be resolved", "failed", failed, "pr", prNumber)
+				}
+			}
+		}
+	}
+
 }
 
 // fetchFileContext fetches the full content of modified files from the PR branch.
@@ -514,14 +563,16 @@ func validateReviewerName(name string) error {
 
 // buildSupersededBody creates the body for a superseded review: struck-through banner
 // with collapsed original content and the commit it was evaluated against.
-func buildSupersededBody(originalBody, commitSHA, sentinel string) string {
+func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
 	shortSHA := commitSHA
 	if len(shortSHA) > 8 {
 		shortSHA = shortSHA[:8]
 	}
 	var sb strings.Builder
 	sb.WriteString("~~Original review~~\n\n")
-	sb.WriteString("**Superseded** \u2014 see current review for up-to-date findings.\n\n")
+	sb.WriteString("**Superseded** \u2014 [see current review](")
+	sb.WriteString(newReviewURL)
+	sb.WriteString(") for up-to-date findings.\n\n")
 	if shortSHA != "" {
 		sb.WriteString("<details><summary>Previous findings (commit ")
 		sb.WriteString(shortSHA)

cmd/review-bot/main_test.go

@@ -60,17 +60,21 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
 func TestBuildSupersededBody(t *testing.T) {
 	original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
 	sentinel := "<!-- review-bot:sonnet -->"
+	newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99"
 
-	result := buildSupersededBody(original, "abcdef1234567890", sentinel)
+	result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel)
 
 	// Should contain the struck-through banner
 	if !strings.Contains(result, "~~Original review~~") {
 		t.Error("missing struck-through banner")
 	}
-	// Should contain superseded notice
+	// Should contain superseded notice with link
 	if !strings.Contains(result, "**Superseded**") {
 		t.Error("missing superseded notice")
 	}
+	if !strings.Contains(result, "[see current review]("+newURL+")") {
+		t.Error("missing link to new review")
+	}
 	// Should contain collapsed original
 	if !strings.Contains(result, "<details>") {
 		t.Error("missing details/collapse")
@@ -95,7 +99,7 @@ func TestBuildSupersededBody(t *testing.T) {
 
 func TestBuildSupersededBodyShortSHA(t *testing.T) {
 	// Short SHA should pass through without panic
-	result := buildSupersededBody("body", "abc", "<!-- review-bot:x -->")
+	result := buildSupersededBody("body", "abc", "https://example.com/review", "<!-- review-bot:x -->")
 	if !strings.Contains(result, "abc") {
 		t.Error("short SHA not preserved")
 	}

gitea/client.go

@@ -82,6 +82,7 @@ type ChangedFile struct {
 
 // ReviewComment represents an inline comment to attach to a review.
 type ReviewComment struct {
+	ID          int64  `json:"id,omitempty"`
 	Path        string `json:"path"`
 	NewPosition int64  `json:"new_position"`
 	Body        string `json:"body"`
@@ -460,3 +461,113 @@ 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) {
+	const pageSize = 50
+	var all []ReviewComment
+	for page := 1; ; page++ {
+		reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments?limit=%d&page=%d",
+			c.baseURL,
+			url.PathEscape(owner),
+			url.PathEscape(repo),
+			prNumber,
+			reviewID,
+			pageSize,
+			page)
+		body, err := c.doGet(ctx, reqURL)
+		if err != nil {
+			return nil, fmt.Errorf("list review comments (page %d): %w", page, err)
+		}
+		var batch []ReviewComment
+		if err := json.Unmarshal(body, &batch); err != nil {
+			return nil, fmt.Errorf("parse review comments (page %d): %w", page, err)
+		}
+		all = append(all, batch...)
+		if len(batch) < pageSize {
+			break
+		}
+	}
+	return all, nil
+}
+
+// ResolveComment marks an inline review comment as resolved.
+func (c *Client) ResolveComment(ctx context.Context, owner, repo string, commentID int64) error {
+	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/comments/%d/resolve",
+		c.baseURL,
+		url.PathEscape(owner),
+		url.PathEscape(repo),
+		commentID)
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, nil)
+	if err != nil {
+		return fmt.Errorf("create resolve request: %w", err)
+	}
+	req.Header.Set("Authorization", "token "+c.token)
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return fmt.Errorf("resolve comment: %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("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"
 )
 
@@ -602,3 +604,142 @@ 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"}]`)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	comments, err := client.ListReviewComments(context.Background(), "owner", "repo", 1, 42)
+	if err != nil {
+		t.Fatalf("ListReviewComments() error = %v", err)
+	}
+	if len(comments) != 2 {
+		t.Fatalf("got %d comments, want 2", len(comments))
+	}
+	if comments[0].ID != 100 {
+		t.Errorf("comments[0].ID = %d, want 100", comments[0].ID)
+	}
+	if comments[1].Path != "lib.go" {
+		t.Errorf("comments[1].Path = %q, want %q", comments[1].Path, "lib.go")
+	}
+}
+
+func TestResolveComment(t *testing.T) {
+	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)
+		}
+		if !strings.Contains(r.URL.Path, "/pulls/comments/99/resolve") {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.ResolveComment(context.Background(), "owner", "repo", 99)
+	if err != nil {
+		t.Fatalf("ResolveComment() error = %v", err)
+	}
+}
+
+func TestResolveComment_Error(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusNotFound)
+		fmt.Fprint(w, "not found")
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	err := client.ResolveComment(context.Background(), "owner", "repo", 99)
+	if err == nil {
+		t.Fatal("expected error for 404 response")
+	}
+}