fix: address review findings

- Gate inline comment resolution on successful supersede
- Add pagination to ListReviewComments
- Add tests for ListReviewComments and ResolveComment

cmd/review-bot/main.go

@@ -344,18 +344,6 @@ func main() {
 		}
 	}
 
-	// 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)
@@ -383,14 +371,13 @@ func main() {
 			if err != nil {
 				slog.Warn("could not list old review comments for resolution", "review_id", existingReview.ID, "error", err)
 			} else {
-				resolved, failed := 0, 0
+				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)
-						failed++
 					} else {
 						resolved++
 					}
@@ -398,9 +385,6 @@ func main() {
 				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)
-				}
 			}
 		}
 	}

gitea/client.go

@@ -462,59 +462,6 @@ 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) {
@@ -566,7 +513,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(io.LimitReader(resp.Body, 256))
+		body, _ := io.ReadAll(resp.Body)
 		return fmt.Errorf("resolve comment failed (status %d): %s", resp.StatusCode, body)
 	}
 	return nil

gitea/client_test.go

@@ -5,10 +5,8 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
-	"io"
 	"net/http"
 	"net/http/httptest"
-	"strings"
 	"testing"
 )
 
@@ -605,91 +603,8 @@ 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"}]`)
 	}))
@@ -716,8 +631,9 @@ func TestResolveComment(t *testing.T) {
 		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)
+		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)
 		}
 		w.WriteHeader(http.StatusOK)
 	}))