From be3f696a70758d54e7cc3f4cf035ec6250cd8669 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 00:25:50 -0700
Subject: [PATCH 1/7] feat(github): implement Reviewer and Identity interfaces
 (#81)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Implement the remaining vcs.Client interface methods for github.Client:

Reviewer:
- PostReview: POST /repos/{owner}/{repo}/pulls/{number}/reviews
- ListReviews: GET /repos/{owner}/{repo}/pulls/{number}/reviews
  with state translation (CHANGES_REQUESTED → REQUEST_CHANGES, etc.)
- DeleteReview: DELETE /repos/{owner}/{repo}/pulls/{number}/reviews/{id}
  Returns ErrCannotDeleteSubmittedReview on 422
- DismissReview: PUT /repos/{owner}/{repo}/pulls/{number}/reviews/{id}/dismissals

Identity:
- GetAuthenticatedUser: GET /user

Infrastructure:
- Add doRequestWithBody helper for POST/PUT/DELETE with JSON bodies
- Update conformance_test.go: var _ vcs.Client = (*github.Client)(nil)

All unit tests pass including error cases (401, 404, 422, malformed).
---
 github/client.go           | 111 +++++++++++
 github/conformance_test.go |  10 +-
 github/identity.go         |  29 +++
 github/identity_test.go    |  46 +++++
 github/review.go           | 191 ++++++++++++++++++
 github/review_test.go      | 394 +++++++++++++++++++++++++++++++++++++
 6 files changed, 775 insertions(+), 6 deletions(-)
 create mode 100644 github/identity.go
 create mode 100644 github/identity_test.go
 create mode 100644 github/review.go
 create mode 100644 github/review_test.go

diff --git a/github/client.go b/github/client.go
index dc10c7a..9fd7eb8 100644
--- a/github/client.go
+++ b/github/client.go
@@ -4,6 +4,7 @@
 package github
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -342,3 +343,113 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
 func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 	return c.doRequest(ctx, http.MethodGet, reqURL, "")
 }
+
+// doRequestWithBody is like doRequest but sends a request body.
+// It accepts the raw body bytes and sets Content-Type to application/json.
+// Retry semantics match doRequest (retries on 429 with Retry-After support).
+func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) {
+	const maxRetryAfter = 120 * time.Second
+
+	defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
+	var backoff []time.Duration
+	if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
+		backoff = make([]time.Duration, len(c.retryBackoff))
+		copy(backoff, c.retryBackoff)
+	} else {
+		backoff = make([]time.Duration, len(defaultBackoff))
+		copy(backoff, defaultBackoff)
+	}
+
+	const maxErrorBodyBytes = 4 * 1024
+
+	if c.token != "" && !c.allowInsecureHTTP {
+		parsed, err := url.Parse(reqURL)
+		if err != nil {
+			return nil, fmt.Errorf("parse request URL: %w", err)
+		}
+		if !strings.EqualFold(parsed.Scheme, "https") {
+			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
+		}
+	}
+
+	var lastErr error
+	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
+		if attempt > 0 {
+			var delay time.Duration
+			if attempt-1 < len(backoff) {
+				delay = backoff[attempt-1]
+			}
+			if delay > 0 {
+				timer := time.NewTimer(delay)
+				select {
+				case <-timer.C:
+					timer.Stop()
+				case <-ctx.Done():
+					timer.Stop()
+					return nil, ctx.Err()
+				}
+			}
+		}
+
+		var bodyReader io.Reader
+		if body != nil {
+			bodyReader = bytes.NewReader(body)
+		}
+		req, err := http.NewRequestWithContext(ctx, method, reqURL, bodyReader)
+		if err != nil {
+			return nil, fmt.Errorf("create request: %w", err)
+		}
+		if c.token != "" {
+			req.Header.Set("Authorization", "Bearer "+c.token)
+		}
+		req.Header.Set("User-Agent", userAgent)
+		req.Header.Set("Accept", "application/vnd.github+json")
+		if body != nil {
+			req.Header.Set("Content-Type", "application/json")
+		}
+
+		resp, err := c.httpClient.Do(req)
+		if err != nil {
+			return nil, fmt.Errorf("do request: %w", err)
+		}
+
+		respStatus := resp.StatusCode
+		retryAfterHeader := resp.Header.Get("Retry-After")
+
+		respBody, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
+		if done {
+			return respBody, handleErr
+		}
+		lastErr = handleErr
+
+		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
+			if ra := retryAfterHeader; ra != "" {
+				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
+					delay := time.Duration(seconds) * time.Second
+					if delay > maxRetryAfter {
+						delay = maxRetryAfter
+					}
+					if attempt < len(backoff) {
+						backoff[attempt] = delay
+					}
+				} else if retryAt, err := http.ParseTime(ra); err == nil {
+					delay := time.Until(retryAt)
+					if delay < 0 {
+						delay = 0
+					}
+					if delay > maxRetryAfter {
+						delay = maxRetryAfter
+					}
+					if attempt < len(backoff) {
+						backoff[attempt] = delay
+					}
+				}
+			}
+			continue
+		}
+
+		return nil, lastErr
+	}
+
+	return nil, lastErr
+}
diff --git a/github/conformance_test.go b/github/conformance_test.go
index ca13188..3e06e2d 100644
--- a/github/conformance_test.go
+++ b/github/conformance_test.go
@@ -5,9 +5,7 @@ import (
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 
-// Compile-time interface conformance assertions.
-// These verify github.Client satisfies vcs.PRReader and vcs.FileReader.
-var (
-	_ vcs.PRReader   = (*github.Client)(nil)
-	_ vcs.FileReader = (*github.Client)(nil)
-)
+// Compile-time interface conformance assertion.
+// This verifies github.Client satisfies the full vcs.Client interface
+// (PRReader, FileReader, Reviewer, Identity).
+var _ vcs.Client = (*github.Client)(nil)
diff --git a/github/identity.go b/github/identity.go
new file mode 100644
index 0000000..46c26a8
--- /dev/null
+++ b/github/identity.go
@@ -0,0 +1,29 @@
+package github
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+)
+
+// userResponse is the GitHub API response for the authenticated user.
+type userResponse struct {
+	Login string `json:"login"`
+}
+
+// GetAuthenticatedUser returns the login of the currently authenticated user.
+func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
+	reqURL := fmt.Sprintf("%s/user", c.baseURL)
+
+	body, err := c.doGet(ctx, reqURL)
+	if err != nil {
+		return "", fmt.Errorf("get authenticated user: %w", err)
+	}
+
+	var resp userResponse
+	if err := json.Unmarshal(body, &resp); err != nil {
+		return "", fmt.Errorf("parse user response: %w", err)
+	}
+
+	return resp.Login, nil
+}
diff --git a/github/identity_test.go b/github/identity_test.go
new file mode 100644
index 0000000..9187ce9
--- /dev/null
+++ b/github/identity_test.go
@@ -0,0 +1,46 @@
+package github
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"testing"
+)
+
+func TestGetAuthenticatedUser_HappyPath(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "GET" {
+			t.Errorf("expected GET, got %s", r.Method)
+		}
+		if r.URL.Path != "/user" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		if r.Header.Get("Authorization") != "Bearer test-token" {
+			t.Errorf("unexpected auth header: %s", r.Header.Get("Authorization"))
+		}
+		json.NewEncoder(w).Encode(map[string]string{"login": "review-bot"})
+	})
+
+	login, err := c.GetAuthenticatedUser(context.Background())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if login != "review-bot" {
+		t.Errorf("expected login 'review-bot', got %q", login)
+	}
+}
+
+func TestGetAuthenticatedUser_401(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(401)
+		w.Write([]byte(`{"message":"Bad credentials"}`))
+	})
+
+	_, err := c.GetAuthenticatedUser(context.Background())
+	if err == nil {
+		t.Fatal("expected error for 401")
+	}
+	if !IsUnauthorized(err) {
+		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
+	}
+}
diff --git a/github/review.go b/github/review.go
new file mode 100644
index 0000000..e9e20cd
--- /dev/null
+++ b/github/review.go
@@ -0,0 +1,191 @@
+package github
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"fmt"
+	"net/url"
+
+	"gitea.weiker.me/rodin/review-bot/vcs"
+)
+
+// ErrCannotDeleteSubmittedReview is returned when DeleteReview is called on
+// a review that has already been submitted (APPROVED, REQUEST_CHANGES, COMMENT).
+// GitHub only allows deletion of PENDING reviews. Callers that need to replace
+// a submitted review should use DismissReview instead.
+var ErrCannotDeleteSubmittedReview = errors.New("cannot delete submitted review: use DismissReview instead")
+
+// postReviewRequest is the GitHub API request body for creating a review.
+type postReviewRequest struct {
+	CommitID string               `json:"commit_id,omitempty"`
+	Body     string               `json:"body"`
+	Event    string               `json:"event"`
+	Comments []reviewCommentEntry `json:"comments,omitempty"`
+}
+
+// reviewCommentEntry is a single inline comment in a review creation request.
+type reviewCommentEntry struct {
+	Path     string `json:"path"`
+	Position int    `json:"position"`
+	Body     string `json:"body"`
+}
+
+// reviewResponse is the GitHub API response for a review.
+type reviewResponse struct {
+	ID       int64  `json:"id"`
+	Body     string `json:"body"`
+	State    string `json:"state"`
+	CommitID string `json:"commit_id"`
+	User     struct {
+		Login string `json:"login"`
+	} `json:"user"`
+}
+
+// dismissReviewRequest is the GitHub API request body for dismissing a review.
+type dismissReviewRequest struct {
+	Message string `json:"message"`
+	Event   string `json:"event"`
+}
+
+// translateGitHubReviewState translates a GitHub API review state to the
+// canonical vcs.Review.State value.
+func translateGitHubReviewState(state string) string {
+	switch state {
+	case "APPROVED":
+		return "APPROVED"
+	case "CHANGES_REQUESTED":
+		return "REQUEST_CHANGES"
+	case "COMMENTED":
+		return "COMMENT"
+	case "DISMISSED":
+		return "DISMISSED"
+	default:
+		return state
+	}
+}
+
+// PostReview submits a review on a pull request.
+// The ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent
+// directly — they match GitHub's canonical event strings.
+// ReviewComment.Position maps directly to the GitHub API position field.
+func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
+	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
+		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
+
+	payload := postReviewRequest{
+		Body:  req.Body,
+		Event: string(req.Event),
+	}
+
+	// Populate CommitID from the first comment if available.
+	// All comments in a single review share the same commit_id.
+	for _, comment := range req.Comments {
+		if comment.CommitID != "" {
+			payload.CommitID = comment.CommitID
+			break
+		}
+	}
+
+	for _, comment := range req.Comments {
+		payload.Comments = append(payload.Comments, reviewCommentEntry{
+			Path:     comment.Path,
+			Position: comment.Position,
+			Body:     comment.Body,
+		})
+	}
+
+	data, err := json.Marshal(payload)
+	if err != nil {
+		return nil, fmt.Errorf("marshal review request: %w", err)
+	}
+
+	body, err := c.doRequestWithBody(ctx, "POST", reqURL, data)
+	if err != nil {
+		return nil, fmt.Errorf("post review: %w", err)
+	}
+
+	var resp reviewResponse
+	if err := json.Unmarshal(body, &resp); err != nil {
+		return nil, fmt.Errorf("parse review response: %w", err)
+	}
+
+	return &vcs.Review{
+		ID:       resp.ID,
+		Body:     resp.Body,
+		User:     vcs.UserInfo{Login: resp.User.Login},
+		State:    translateGitHubReviewState(resp.State),
+		CommitID: resp.CommitID,
+	}, nil
+}
+
+// ListReviews retrieves all reviews for a pull request.
+// GitHub review states are translated to canonical vcs values.
+func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
+	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
+		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
+
+	body, err := c.doGet(ctx, reqURL)
+	if err != nil {
+		return nil, fmt.Errorf("list reviews: %w", err)
+	}
+
+	var responses []reviewResponse
+	if err := json.Unmarshal(body, &responses); err != nil {
+		return nil, fmt.Errorf("parse reviews response: %w", err)
+	}
+
+	reviews := make([]vcs.Review, len(responses))
+	for i, r := range responses {
+		reviews[i] = vcs.Review{
+			ID:       r.ID,
+			Body:     r.Body,
+			User:     vcs.UserInfo{Login: r.User.Login},
+			State:    translateGitHubReviewState(r.State),
+			CommitID: r.CommitID,
+		}
+	}
+	return reviews, nil
+}
+
+// DeleteReview deletes a pull request review.
+// Only PENDING reviews can be deleted; attempting to delete a submitted review
+// (APPROVED, CHANGES_REQUESTED, COMMENTED) returns ErrCannotDeleteSubmittedReview.
+func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
+	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
+		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
+
+	_, err := c.doRequestWithBody(ctx, "DELETE", reqURL, nil)
+	if err != nil {
+		var apiErr *APIError
+		if errors.As(err, &apiErr) && apiErr.StatusCode == 422 {
+			return ErrCannotDeleteSubmittedReview
+		}
+		return fmt.Errorf("delete review: %w", err)
+	}
+	return nil
+}
+
+// DismissReview dismisses a submitted review on a pull request.
+// This is the correct way to "remove" a submitted review (APPROVED, REQUEST_CHANGES).
+// GitHub does not allow deleting submitted reviews — they must be dismissed.
+func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error {
+	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/dismissals",
+		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
+
+	payload := dismissReviewRequest{
+		Message: message,
+		Event:   "DISMISS",
+	}
+
+	data, err := json.Marshal(payload)
+	if err != nil {
+		return fmt.Errorf("marshal dismiss request: %w", err)
+	}
+
+	_, err = c.doRequestWithBody(ctx, "PUT", reqURL, data)
+	if err != nil {
+		return fmt.Errorf("dismiss review: %w", err)
+	}
+	return nil
+}
diff --git a/github/review_test.go b/github/review_test.go
new file mode 100644
index 0000000..50e6700
--- /dev/null
+++ b/github/review_test.go
@@ -0,0 +1,394 @@
+package github
+
+import (
+	"context"
+	"encoding/json"
+	"errors"
+	"io"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"gitea.weiker.me/rodin/review-bot/vcs"
+)
+
+func newTestClient(t *testing.T, handler http.HandlerFunc) *Client {
+	t.Helper()
+	srv := httptest.NewServer(handler)
+	t.Cleanup(srv.Close)
+	c := NewClient("test-token", srv.URL, AllowInsecureHTTP())
+	c.SetHTTPClient(srv.Client())
+	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
+	return c
+}
+
+// --- PostReview tests ---
+
+func TestPostReview_HappyPath(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "POST" {
+			t.Errorf("expected POST, got %s", r.Method)
+		}
+		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		if r.Header.Get("Content-Type") != "application/json" {
+			t.Errorf("expected Content-Type application/json, got %q", r.Header.Get("Content-Type"))
+		}
+
+		// Verify request body
+		body, _ := io.ReadAll(r.Body)
+		var req postReviewRequest
+		if err := json.Unmarshal(body, &req); err != nil {
+			t.Fatalf("unmarshal request: %v", err)
+		}
+		if req.Event != "APPROVE" {
+			t.Errorf("expected event APPROVE, got %q", req.Event)
+		}
+		if req.Body != "LGTM" {
+			t.Errorf("expected body 'LGTM', got %q", req.Body)
+		}
+		if req.CommitID != "abc123" {
+			t.Errorf("expected commit_id 'abc123', got %q", req.CommitID)
+		}
+		if len(req.Comments) != 1 {
+			t.Fatalf("expected 1 comment, got %d", len(req.Comments))
+		}
+		if req.Comments[0].Path != "main.go" {
+			t.Errorf("expected comment path 'main.go', got %q", req.Comments[0].Path)
+		}
+		if req.Comments[0].Position != 4 {
+			t.Errorf("expected comment position 4, got %d", req.Comments[0].Position)
+		}
+
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"id":        100,
+			"body":      "LGTM",
+			"state":     "APPROVED",
+			"commit_id": "abc123",
+			"user":      map[string]string{"login": "reviewer"},
+		})
+	})
+
+	review, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
+		Body:  "LGTM",
+		Event: vcs.ReviewEventApprove,
+		Comments: []vcs.ReviewComment{
+			{Path: "main.go", Position: 4, CommitID: "abc123", Body: "nit: rename"},
+		},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if review.ID != 100 {
+		t.Errorf("expected ID 100, got %d", review.ID)
+	}
+	if review.Body != "LGTM" {
+		t.Errorf("expected body 'LGTM', got %q", review.Body)
+	}
+	if review.State != "APPROVED" {
+		t.Errorf("expected state 'APPROVED', got %q", review.State)
+	}
+	if review.User.Login != "reviewer" {
+		t.Errorf("expected user 'reviewer', got %q", review.User.Login)
+	}
+	if review.CommitID != "abc123" {
+		t.Errorf("expected commit_id 'abc123', got %q", review.CommitID)
+	}
+}
+
+func TestPostReview_401(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(401)
+		w.Write([]byte(`{"message":"Bad credentials"}`))
+	})
+
+	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
+		Body:  "LGTM",
+		Event: vcs.ReviewEventApprove,
+	})
+	if err == nil {
+		t.Fatal("expected error for 401")
+	}
+	if !IsUnauthorized(err) {
+		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
+	}
+}
+
+func TestPostReview_422(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(422)
+		w.Write([]byte(`{"message":"Unprocessable Entity"}`))
+	})
+
+	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
+		Body:  "LGTM",
+		Event: vcs.ReviewEventApprove,
+	})
+	if err == nil {
+		t.Fatal("expected error for 422")
+	}
+	// 422 should surface as a wrapped APIError
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		t.Fatalf("expected *APIError, got %T: %v", err, err)
+	}
+	if apiErr.StatusCode != 422 {
+		t.Errorf("expected status 422, got %d", apiErr.StatusCode)
+	}
+}
+
+func TestPostReview_MalformedResponse(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(`not json`))
+	})
+
+	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
+		Body:  "LGTM",
+		Event: vcs.ReviewEventApprove,
+	})
+	if err == nil {
+		t.Fatal("expected error for malformed response")
+	}
+	if !containsStr(err.Error(), "parse review response") {
+		t.Errorf("expected parse error, got: %v", err)
+	}
+}
+
+// --- ListReviews tests ---
+
+func TestListReviews_HappyPath(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "GET" {
+			t.Errorf("expected GET, got %s", r.Method)
+		}
+		if r.URL.Path != "/repos/owner/repo/pulls/3/reviews" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		json.NewEncoder(w).Encode([]map[string]interface{}{
+			{
+				"id":        1,
+				"body":      "Approved",
+				"state":     "APPROVED",
+				"commit_id": "sha1",
+				"user":      map[string]string{"login": "user1"},
+			},
+			{
+				"id":        2,
+				"body":      "Needs work",
+				"state":     "CHANGES_REQUESTED",
+				"commit_id": "sha2",
+				"user":      map[string]string{"login": "user2"},
+			},
+			{
+				"id":        3,
+				"body":      "Comment only",
+				"state":     "COMMENTED",
+				"commit_id": "sha3",
+				"user":      map[string]string{"login": "user3"},
+			},
+			{
+				"id":        4,
+				"body":      "Old review",
+				"state":     "DISMISSED",
+				"commit_id": "sha4",
+				"user":      map[string]string{"login": "user4"},
+			},
+		})
+	})
+
+	reviews, err := c.ListReviews(context.Background(), "owner", "repo", 3)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(reviews) != 4 {
+		t.Fatalf("expected 4 reviews, got %d", len(reviews))
+	}
+
+	// Check state translation
+	expected := []struct {
+		id    int64
+		state string
+	}{
+		{1, "APPROVED"},
+		{2, "REQUEST_CHANGES"},
+		{3, "COMMENT"},
+		{4, "DISMISSED"},
+	}
+	for i, e := range expected {
+		if reviews[i].ID != e.id {
+			t.Errorf("review[%d]: expected ID %d, got %d", i, e.id, reviews[i].ID)
+		}
+		if reviews[i].State != e.state {
+			t.Errorf("review[%d]: expected state %q, got %q", i, e.state, reviews[i].State)
+		}
+	}
+}
+
+func TestListReviews_404(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(404)
+		w.Write([]byte(`{"message":"Not Found"}`))
+	})
+
+	_, err := c.ListReviews(context.Background(), "owner", "repo", 999)
+	if err == nil {
+		t.Fatal("expected error for 404")
+	}
+	if !IsNotFound(err) {
+		t.Errorf("expected IsNotFound=true, got error: %v", err)
+	}
+}
+
+func TestListReviews_401(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(401)
+		w.Write([]byte(`{"message":"Bad credentials"}`))
+	})
+
+	_, err := c.ListReviews(context.Background(), "owner", "repo", 3)
+	if err == nil {
+		t.Fatal("expected error for 401")
+	}
+	if !IsUnauthorized(err) {
+		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
+	}
+}
+
+// --- DeleteReview tests ---
+
+func TestDeleteReview_HappyPath(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "DELETE" {
+			t.Errorf("expected DELETE, got %s", r.Method)
+		}
+		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/42" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		w.WriteHeader(204)
+	})
+
+	err := c.DeleteReview(context.Background(), "owner", "repo", 5, 42)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDeleteReview_422_SubmittedReview(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(422)
+		w.Write([]byte(`{"message":"Can not delete a non pending review"}`))
+	})
+
+	err := c.DeleteReview(context.Background(), "owner", "repo", 5, 42)
+	if err == nil {
+		t.Fatal("expected error for 422")
+	}
+	if !errors.Is(err, ErrCannotDeleteSubmittedReview) {
+		t.Errorf("expected ErrCannotDeleteSubmittedReview, got: %v", err)
+	}
+}
+
+// --- DismissReview tests ---
+
+func TestDismissReview_HappyPath(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "PUT" {
+			t.Errorf("expected PUT, got %s", r.Method)
+		}
+		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/10/dismissals" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+
+		body, _ := io.ReadAll(r.Body)
+		var req dismissReviewRequest
+		if err := json.Unmarshal(body, &req); err != nil {
+			t.Fatalf("unmarshal request: %v", err)
+		}
+		if req.Message != "Superseded by new review" {
+			t.Errorf("expected message 'Superseded by new review', got %q", req.Message)
+		}
+		if req.Event != "DISMISS" {
+			t.Errorf("expected event 'DISMISS', got %q", req.Event)
+		}
+
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"id":    10,
+			"state": "DISMISSED",
+		})
+	})
+
+	err := c.DismissReview(context.Background(), "owner", "repo", 5, 10, "Superseded by new review")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDismissReview_404(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(404)
+		w.Write([]byte(`{"message":"Not Found"}`))
+	})
+
+	err := c.DismissReview(context.Background(), "owner", "repo", 5, 999, "dismiss")
+	if err == nil {
+		t.Fatal("expected error for 404")
+	}
+	if !IsNotFound(err) {
+		t.Errorf("expected IsNotFound=true, got error: %v", err)
+	}
+}
+
+func TestDismissReview_401(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(401)
+		w.Write([]byte(`{"message":"Bad credentials"}`))
+	})
+
+	err := c.DismissReview(context.Background(), "owner", "repo", 5, 10, "dismiss")
+	if err == nil {
+		t.Fatal("expected error for 401")
+	}
+	if !IsUnauthorized(err) {
+		t.Errorf("expected IsUnauthorized=true, got error: %v", err)
+	}
+}
+
+// --- State translation tests ---
+
+func TestTranslateGitHubReviewState(t *testing.T) {
+	tests := []struct {
+		input    string
+		expected string
+	}{
+		{"APPROVED", "APPROVED"},
+		{"CHANGES_REQUESTED", "REQUEST_CHANGES"},
+		{"COMMENTED", "COMMENT"},
+		{"DISMISSED", "DISMISSED"},
+		{"UNKNOWN_STATE", "UNKNOWN_STATE"},
+		{"", ""},
+	}
+	for _, tt := range tests {
+		got := translateGitHubReviewState(tt.input)
+		if got != tt.expected {
+			t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.expected)
+		}
+	}
+}
+
+// containsStr is a test helper for checking error messages.
+func containsStr(s, substr string) bool {
+	return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsSubstring(s, substr))
+}
+
+func containsSubstring(s, sub string) bool {
+	for i := 0; i <= len(s)-len(sub); i++ {
+		if s[i:i+len(sub)] == sub {
+			return true
+		}
+	}
+	return false
+}
-- 
2.47.3


From eba97321addf7c6327028a0c8b3988d1d53b6335 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 00:35:59 -0700
Subject: [PATCH 2/7] refactor(github): extract doRequestCore, address review
 feedback

- MAJOR: Extract doRequestCore to eliminate doRequest/doRequestWithBody
  duplication. Both now delegate to a shared implementation with the
  retry/backoff logic in a single place.

- MINOR: Replace custom containsStr/containsSubstring helpers with
  strings.Contains in review_test.go.

- MINOR: Use http.Method* constants (MethodPost, MethodDelete, MethodPut)
  in review.go for consistency with doGet.

- MINOR: Remove redundant APPROVED/DISMISSED cases from
  translateGitHubReviewState that were identical to the default passthrough.

- NIT: Clarify DeleteReview comment about COMMENTED being a GitHub API
  state name.

- DismissReview Event field verified as required by GitHub API docs;
  kept as-is.
---
 github/client.go      | 158 ++++++++++++------------------------------
 github/review.go      |  16 ++---
 github/review_test.go |  15 +---
 3 files changed, 54 insertions(+), 135 deletions(-)

diff --git a/github/client.go b/github/client.go
index 9fd7eb8..2d5204e 100644
--- a/github/client.go
+++ b/github/client.go
@@ -193,10 +193,24 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
 	return nil
 }
 
-// doRequest performs an HTTP request with retry on 429 rate limit responses.
-// It respects the Retry-After header when present (capped at maxRetryAfter).
-// Transport errors (network failures, context cancellation) are not retried.
-func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
+// requestOptions holds per-request configuration for doRequestCore.
+type requestOptions struct {
+	// bodyFn returns a fresh io.Reader for the request body on each attempt.
+	// Must be non-nil for requests that carry a body (POST, PUT, PATCH).
+	// Returning a fresh reader on each call allows retries to re-send the body.
+	bodyFn func() io.Reader
+
+	// accept overrides the default Accept header. Empty means "application/vnd.github+json".
+	accept string
+
+	// extraHeaders are additional headers to set on each request attempt.
+	extraHeaders map[string]string
+}
+
+// doRequestCore is the shared implementation for all HTTP requests with retry
+// on 429 rate limit responses. It respects the Retry-After header when present
+// (capped at maxRetryAfter). Transport errors are not retried.
+func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) {
 	const maxRetryAfter = 120 * time.Second
 
 	// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
@@ -247,7 +261,11 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			}
 		}
 
-		req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
+		var body io.Reader
+		if opts.bodyFn != nil {
+			body = opts.bodyFn()
+		}
+		req, err := http.NewRequestWithContext(ctx, method, reqURL, body)
 		if err != nil {
 			return nil, fmt.Errorf("create request: %w", err)
 		}
@@ -258,11 +276,14 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			req.Header.Set("Authorization", "Bearer "+c.token)
 		}
 		req.Header.Set("User-Agent", userAgent)
-		if accept != "" {
-			req.Header.Set("Accept", accept)
+		if opts.accept != "" {
+			req.Header.Set("Accept", opts.accept)
 		} else {
 			req.Header.Set("Accept", "application/vnd.github+json")
 		}
+		for k, v := range opts.extraHeaders {
+			req.Header.Set(k, v)
+		}
 
 		resp, err := c.httpClient.Do(req)
 		if err != nil {
@@ -273,11 +294,11 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		respStatus := resp.StatusCode
 		retryAfterHeader := resp.Header.Get("Retry-After")
 
-		body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
+		respBody, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
 		if done {
-			return body, err
+			return respBody, handleErr
 		}
-		lastErr = err
+		lastErr = handleErr
 
 		// Retry on 429 rate limit
 		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
@@ -315,6 +336,13 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 	return nil, lastErr
 }
 
+// doRequest performs an HTTP request with retry on 429 rate limit responses.
+// It respects the Retry-After header when present (capped at maxRetryAfter).
+// Transport errors (network failures, context cancellation) are not retried.
+func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
+	return c.doRequestCore(ctx, method, reqURL, requestOptions{accept: accept})
+}
+
 // handleResponse reads and closes the response body, returning the result.
 // It uses defer to ensure the body is always closed regardless of code path.
 // Returns (body, done, err) where done=true means the caller should return immediately.
@@ -347,109 +375,11 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 // doRequestWithBody is like doRequest but sends a request body.
 // It accepts the raw body bytes and sets Content-Type to application/json.
 // Retry semantics match doRequest (retries on 429 with Retry-After support).
-func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) {
-	const maxRetryAfter = 120 * time.Second
-
-	defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
-	var backoff []time.Duration
-	if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
-		backoff = make([]time.Duration, len(c.retryBackoff))
-		copy(backoff, c.retryBackoff)
-	} else {
-		backoff = make([]time.Duration, len(defaultBackoff))
-		copy(backoff, defaultBackoff)
+func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, reqBody []byte) ([]byte, error) {
+	var opts requestOptions
+	if reqBody != nil {
+		opts.bodyFn = func() io.Reader { return bytes.NewReader(reqBody) }
+		opts.extraHeaders = map[string]string{"Content-Type": "application/json"}
 	}
-
-	const maxErrorBodyBytes = 4 * 1024
-
-	if c.token != "" && !c.allowInsecureHTTP {
-		parsed, err := url.Parse(reqURL)
-		if err != nil {
-			return nil, fmt.Errorf("parse request URL: %w", err)
-		}
-		if !strings.EqualFold(parsed.Scheme, "https") {
-			return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
-		}
-	}
-
-	var lastErr error
-	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
-		if attempt > 0 {
-			var delay time.Duration
-			if attempt-1 < len(backoff) {
-				delay = backoff[attempt-1]
-			}
-			if delay > 0 {
-				timer := time.NewTimer(delay)
-				select {
-				case <-timer.C:
-					timer.Stop()
-				case <-ctx.Done():
-					timer.Stop()
-					return nil, ctx.Err()
-				}
-			}
-		}
-
-		var bodyReader io.Reader
-		if body != nil {
-			bodyReader = bytes.NewReader(body)
-		}
-		req, err := http.NewRequestWithContext(ctx, method, reqURL, bodyReader)
-		if err != nil {
-			return nil, fmt.Errorf("create request: %w", err)
-		}
-		if c.token != "" {
-			req.Header.Set("Authorization", "Bearer "+c.token)
-		}
-		req.Header.Set("User-Agent", userAgent)
-		req.Header.Set("Accept", "application/vnd.github+json")
-		if body != nil {
-			req.Header.Set("Content-Type", "application/json")
-		}
-
-		resp, err := c.httpClient.Do(req)
-		if err != nil {
-			return nil, fmt.Errorf("do request: %w", err)
-		}
-
-		respStatus := resp.StatusCode
-		retryAfterHeader := resp.Header.Get("Retry-After")
-
-		respBody, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
-		if done {
-			return respBody, handleErr
-		}
-		lastErr = handleErr
-
-		if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
-			if ra := retryAfterHeader; ra != "" {
-				if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
-					delay := time.Duration(seconds) * time.Second
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
-					if attempt < len(backoff) {
-						backoff[attempt] = delay
-					}
-				} else if retryAt, err := http.ParseTime(ra); err == nil {
-					delay := time.Until(retryAt)
-					if delay < 0 {
-						delay = 0
-					}
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
-					if attempt < len(backoff) {
-						backoff[attempt] = delay
-					}
-				}
-			}
-			continue
-		}
-
-		return nil, lastErr
-	}
-
-	return nil, lastErr
+	return c.doRequestCore(ctx, method, reqURL, opts)
 }
diff --git a/github/review.go b/github/review.go
index e9e20cd..785175b 100644
--- a/github/review.go
+++ b/github/review.go
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"net/http"
 	"net/url"
 
 	"gitea.weiker.me/rodin/review-bot/vcs"
@@ -52,15 +53,13 @@ type dismissReviewRequest struct {
 // canonical vcs.Review.State value.
 func translateGitHubReviewState(state string) string {
 	switch state {
-	case "APPROVED":
-		return "APPROVED"
 	case "CHANGES_REQUESTED":
 		return "REQUEST_CHANGES"
 	case "COMMENTED":
 		return "COMMENT"
-	case "DISMISSED":
-		return "DISMISSED"
 	default:
+		// States like APPROVED, DISMISSED, and PENDING pass through unchanged
+		// as they already match the canonical vcs representation.
 		return state
 	}
 }
@@ -100,7 +99,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 		return nil, fmt.Errorf("marshal review request: %w", err)
 	}
 
-	body, err := c.doRequestWithBody(ctx, "POST", reqURL, data)
+	body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
 	if err != nil {
 		return nil, fmt.Errorf("post review: %w", err)
 	}
@@ -150,12 +149,13 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
 
 // DeleteReview deletes a pull request review.
 // Only PENDING reviews can be deleted; attempting to delete a submitted review
-// (APPROVED, CHANGES_REQUESTED, COMMENTED) returns ErrCannotDeleteSubmittedReview.
+// (APPROVED, CHANGES_REQUESTED, or COMMENTED per GitHub API naming) returns
+// ErrCannotDeleteSubmittedReview.
 func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
 
-	_, err := c.doRequestWithBody(ctx, "DELETE", reqURL, nil)
+	_, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil)
 	if err != nil {
 		var apiErr *APIError
 		if errors.As(err, &apiErr) && apiErr.StatusCode == 422 {
@@ -183,7 +183,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
 		return fmt.Errorf("marshal dismiss request: %w", err)
 	}
 
-	_, err = c.doRequestWithBody(ctx, "PUT", reqURL, data)
+	_, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data)
 	if err != nil {
 		return fmt.Errorf("dismiss review: %w", err)
 	}
diff --git a/github/review_test.go b/github/review_test.go
index 50e6700..1c3d0ac 100644
--- a/github/review_test.go
+++ b/github/review_test.go
@@ -7,6 +7,7 @@ import (
 	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 	"time"
 
@@ -153,7 +154,7 @@ func TestPostReview_MalformedResponse(t *testing.T) {
 	if err == nil {
 		t.Fatal("expected error for malformed response")
 	}
-	if !containsStr(err.Error(), "parse review response") {
+	if !strings.Contains(err.Error(), "parse review response") {
 		t.Errorf("expected parse error, got: %v", err)
 	}
 }
@@ -379,16 +380,4 @@ func TestTranslateGitHubReviewState(t *testing.T) {
 	}
 }
 
-// containsStr is a test helper for checking error messages.
-func containsStr(s, substr string) bool {
-	return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsSubstring(s, substr))
-}
 
-func containsSubstring(s, sub string) bool {
-	for i := 0; i <= len(s)-len(sub); i++ {
-		if s[i:i+len(sub)] == sub {
-			return true
-		}
-	}
-	return false
-}
-- 
2.47.3


From 293296b50cbb6e88b675cd136a03a9ffb8a33917 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 00:54:21 -0700
Subject: [PATCH 3/7] address review feedback: wrap
 ErrCannotDeleteSubmittedReview, fix nits

- Wrap ErrCannotDeleteSubmittedReview with operation context via fmt.Errorf
  so callers get both sentinel identity and context (MINOR fix)
- Combine double iteration in PostReview into single loop (NIT)
- Remove extra trailing blank line in review_test.go (NIT)
- Clarify translateGitHubReviewState comment re: PENDING state (NIT)
- Update requestOptions.bodyFn comment to mention DELETE-with-body (NIT)
---
 github/client.go      |  3 ++-
 github/review.go      | 13 +++++--------
 github/review_test.go |  2 --
 3 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/github/client.go b/github/client.go
index 2d5204e..f958bfa 100644
--- a/github/client.go
+++ b/github/client.go
@@ -196,7 +196,8 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
 // requestOptions holds per-request configuration for doRequestCore.
 type requestOptions struct {
 	// bodyFn returns a fresh io.Reader for the request body on each attempt.
-	// Must be non-nil for requests that carry a body (POST, PUT, PATCH).
+	// Must be non-nil for any request that carries a body (POST, PUT, PATCH,
+	// or DELETE when a body is required by the API).
 	// Returning a fresh reader on each call allows retries to re-send the body.
 	bodyFn func() io.Reader
 
diff --git a/github/review.go b/github/review.go
index 785175b..7502fc7 100644
--- a/github/review.go
+++ b/github/review.go
@@ -59,7 +59,8 @@ func translateGitHubReviewState(state string) string {
 		return "COMMENT"
 	default:
 		// States like APPROVED, DISMISSED, and PENDING pass through unchanged
-		// as they already match the canonical vcs representation.
+		// as they already match the canonical vcs representation. PENDING appears
+		// on draft reviews that have not yet been submitted via the GitHub UI or API.
 		return state
 	}
 }
@@ -77,16 +78,12 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 		Event: string(req.Event),
 	}
 
-	// Populate CommitID from the first comment if available.
+	// Populate CommitID from the first comment and build the payload in one pass.
 	// All comments in a single review share the same commit_id.
 	for _, comment := range req.Comments {
-		if comment.CommitID != "" {
+		if payload.CommitID == "" && comment.CommitID != "" {
 			payload.CommitID = comment.CommitID
-			break
 		}
-	}
-
-	for _, comment := range req.Comments {
 		payload.Comments = append(payload.Comments, reviewCommentEntry{
 			Path:     comment.Path,
 			Position: comment.Position,
@@ -159,7 +156,7 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in
 	if err != nil {
 		var apiErr *APIError
 		if errors.As(err, &apiErr) && apiErr.StatusCode == 422 {
-			return ErrCannotDeleteSubmittedReview
+			return fmt.Errorf("delete review: %w", ErrCannotDeleteSubmittedReview)
 		}
 		return fmt.Errorf("delete review: %w", err)
 	}
diff --git a/github/review_test.go b/github/review_test.go
index 1c3d0ac..da6535d 100644
--- a/github/review_test.go
+++ b/github/review_test.go
@@ -379,5 +379,3 @@ func TestTranslateGitHubReviewState(t *testing.T) {
 		}
 	}
 }
-
-
-- 
2.47.3


From 8b256360bfb1392ec67fd9e6466c59d8fb06aa34 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 01:29:14 -0700
Subject: [PATCH 4/7] fix(github): clarify PostReview doc comment, rename test
 field to 'want'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Address review feedback from round-3 sonnet review:
- PostReview doc comment now accurately describes vcs.ReviewEvent → GitHub
  wire-format string cast and notes nil-Comments omitempty behavior.
- Rename 'expected' field to 'want' in TestTranslateGitHubReviewState to
  match the project's established naming convention.
---
 github/review.go      | 10 ++++++++--
 github/review_test.go |  8 ++++----
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/github/review.go b/github/review.go
index 7502fc7..ead197f 100644
--- a/github/review.go
+++ b/github/review.go
@@ -66,9 +66,15 @@ func translateGitHubReviewState(state string) string {
 }
 
 // PostReview submits a review on a pull request.
-// The ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent
-// directly — they match GitHub's canonical event strings.
+//
+// The vcs.ReviewEvent constants (ReviewEventApprove, ReviewEventRequestChanges,
+// ReviewEventComment) have string values that match GitHub's wire-format event
+// strings (APPROVE, REQUEST_CHANGES, COMMENT), so Event is cast directly to
+// string without translation.
+//
 // ReviewComment.Position maps directly to the GitHub API position field.
+// When req.Comments is empty, the payload omits the comments field entirely
+// (via the omitempty tag on postReviewRequest.Comments).
 func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
diff --git a/github/review_test.go b/github/review_test.go
index da6535d..fa1200c 100644
--- a/github/review_test.go
+++ b/github/review_test.go
@@ -362,8 +362,8 @@ func TestDismissReview_401(t *testing.T) {
 
 func TestTranslateGitHubReviewState(t *testing.T) {
 	tests := []struct {
-		input    string
-		expected string
+		input string
+		want  string
 	}{
 		{"APPROVED", "APPROVED"},
 		{"CHANGES_REQUESTED", "REQUEST_CHANGES"},
@@ -374,8 +374,8 @@ func TestTranslateGitHubReviewState(t *testing.T) {
 	}
 	for _, tt := range tests {
 		got := translateGitHubReviewState(tt.input)
-		if got != tt.expected {
-			t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.expected)
+		if got != tt.want {
+			t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.want)
 		}
 	}
 }
-- 
2.47.3


From 9dd5e8dbaca7a5e818540ebb1bec4c21298df968 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 01:44:11 -0700
Subject: [PATCH 5/7] fix(github): validate conflicting commit IDs and extract
 test helper

Address review findings from sonnet-review-bot (review 3086):

- PostReview now returns ErrConflictingCommitIDs when comments specify
  different non-empty CommitIDs, since the GitHub API accepts only a
  single commit_id per review. Previously the discrepancy was silently
  ignored, using only the first commit's ID.

- Extract newTestClient into helpers_test.go to make cross-file sharing
  between review_test.go and identity_test.go explicit.

Refs: #81
---
 github/helpers_test.go | 23 +++++++++++++++++++++++
 github/review.go       | 23 +++++++++++++++++++----
 github/review_test.go  | 35 +++++++++++++++++++++--------------
 3 files changed, 63 insertions(+), 18 deletions(-)
 create mode 100644 github/helpers_test.go

diff --git a/github/helpers_test.go b/github/helpers_test.go
new file mode 100644
index 0000000..6e56d35
--- /dev/null
+++ b/github/helpers_test.go
@@ -0,0 +1,23 @@
+package github
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+)
+
+// newTestClient creates a *Client backed by an httptest.Server running the
+// given handler. The server is automatically closed when the test finishes.
+// Shared across test files in package github.
+func newTestClient(t *testing.T, handler http.HandlerFunc) *Client {
+	t.Helper()
+	srv := httptest.NewServer(handler)
+	t.Cleanup(srv.Close)
+	c := NewClient("test-token", srv.URL, AllowInsecureHTTP())
+	c.SetHTTPClient(srv.Client())
+	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
+		t.Fatalf("SetRetryBackoff: %v", err)
+	}
+	return c
+}
diff --git a/github/review.go b/github/review.go
index ead197f..e85ae00 100644
--- a/github/review.go
+++ b/github/review.go
@@ -17,6 +17,11 @@ import (
 // a submitted review should use DismissReview instead.
 var ErrCannotDeleteSubmittedReview = errors.New("cannot delete submitted review: use DismissReview instead")
 
+// ErrConflictingCommitIDs is returned when PostReview receives comments with
+// differing non-empty CommitIDs. The GitHub API accepts only a single commit_id
+// per review submission; callers must ensure all comments target the same commit.
+var ErrConflictingCommitIDs = errors.New("comments contain conflicting commit IDs: all must target the same commit")
+
 // postReviewRequest is the GitHub API request body for creating a review.
 type postReviewRequest struct {
 	CommitID string               `json:"commit_id,omitempty"`
@@ -75,6 +80,11 @@ func translateGitHubReviewState(state string) string {
 // ReviewComment.Position maps directly to the GitHub API position field.
 // When req.Comments is empty, the payload omits the comments field entirely
 // (via the omitempty tag on postReviewRequest.Comments).
+//
+// The GitHub API accepts a single commit_id per review submission. PostReview
+// extracts it from the first comment with a non-empty CommitID. If any subsequent
+// comment specifies a different CommitID, PostReview returns ErrConflictingCommitIDs.
+// Comments with an empty CommitID are allowed and inherit the review-level value.
 func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
@@ -84,11 +94,16 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 		Event: string(req.Event),
 	}
 
-	// Populate CommitID from the first comment and build the payload in one pass.
-	// All comments in a single review share the same commit_id.
+	// Build the payload in one pass. The GitHub API accepts a single commit_id
+	// per review; we extract it from the first comment that supplies one and
+	// reject the request if any other comment disagrees.
 	for _, comment := range req.Comments {
-		if payload.CommitID == "" && comment.CommitID != "" {
-			payload.CommitID = comment.CommitID
+		if comment.CommitID != "" {
+			if payload.CommitID == "" {
+				payload.CommitID = comment.CommitID
+			} else if payload.CommitID != comment.CommitID {
+				return nil, ErrConflictingCommitIDs
+			}
 		}
 		payload.Comments = append(payload.Comments, reviewCommentEntry{
 			Path:     comment.Path,
diff --git a/github/review_test.go b/github/review_test.go
index fa1200c..2432cfc 100644
--- a/github/review_test.go
+++ b/github/review_test.go
@@ -6,26 +6,12 @@ import (
 	"errors"
 	"io"
 	"net/http"
-	"net/http/httptest"
 	"strings"
 	"testing"
-	"time"
 
 	"gitea.weiker.me/rodin/review-bot/vcs"
 )
 
-func newTestClient(t *testing.T, handler http.HandlerFunc) *Client {
-	t.Helper()
-	srv := httptest.NewServer(handler)
-	t.Cleanup(srv.Close)
-	c := NewClient("test-token", srv.URL, AllowInsecureHTTP())
-	c.SetHTTPClient(srv.Client())
-	if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
-		t.Fatalf("SetRetryBackoff: %v", err)
-	}
-	return c
-}
-
 // --- PostReview tests ---
 
 func TestPostReview_HappyPath(t *testing.T) {
@@ -379,3 +365,24 @@ func TestTranslateGitHubReviewState(t *testing.T) {
 		}
 	}
 }
+
+func TestPostReview_ConflictingCommitIDs(t *testing.T) {
+	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not be sent when commit IDs conflict")
+	})
+
+	_, err := c.PostReview(context.Background(), "owner", "repo", 5, vcs.ReviewRequest{
+		Body:  "Review",
+		Event: vcs.ReviewEventComment,
+		Comments: []vcs.ReviewComment{
+			{Path: "a.go", Position: 1, CommitID: "sha-1", Body: "first"},
+			{Path: "b.go", Position: 2, CommitID: "sha-2", Body: "second"},
+		},
+	})
+	if err == nil {
+		t.Fatal("expected error for conflicting commit IDs")
+	}
+	if !errors.Is(err, ErrConflictingCommitIDs) {
+		t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
+	}
+}
-- 
2.47.3


From cd8a1becb33cff90741cb0886e8a911146cf1409 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 02:04:06 -0700
Subject: [PATCH 6/7] test(github): use t.Run subtests in
 TestTranslateGitHubReviewState; doc: note nil body in DeleteReview

---
 github/review.go      |  1 +
 github/review_test.go | 23 +++++++++++++----------
 2 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/github/review.go b/github/review.go
index e85ae00..bd2e05a 100644
--- a/github/review.go
+++ b/github/review.go
@@ -173,6 +173,7 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
 
+	// nil body: the GitHub DELETE endpoint for reviews requires no request body.
 	_, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil)
 	if err != nil {
 		var apiErr *APIError
diff --git a/github/review_test.go b/github/review_test.go
index 2432cfc..33f8743 100644
--- a/github/review_test.go
+++ b/github/review_test.go
@@ -348,21 +348,24 @@ func TestDismissReview_401(t *testing.T) {
 
 func TestTranslateGitHubReviewState(t *testing.T) {
 	tests := []struct {
+		name  string
 		input string
 		want  string
 	}{
-		{"APPROVED", "APPROVED"},
-		{"CHANGES_REQUESTED", "REQUEST_CHANGES"},
-		{"COMMENTED", "COMMENT"},
-		{"DISMISSED", "DISMISSED"},
-		{"UNKNOWN_STATE", "UNKNOWN_STATE"},
-		{"", ""},
+		{"approved passes through", "APPROVED", "APPROVED"},
+		{"changes_requested maps to REQUEST_CHANGES", "CHANGES_REQUESTED", "REQUEST_CHANGES"},
+		{"commented maps to COMMENT", "COMMENTED", "COMMENT"},
+		{"dismissed passes through", "DISMISSED", "DISMISSED"},
+		{"unknown state passes through", "UNKNOWN_STATE", "UNKNOWN_STATE"},
+		{"empty string passes through", "", ""},
 	}
 	for _, tt := range tests {
-		got := translateGitHubReviewState(tt.input)
-		if got != tt.want {
-			t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.want)
-		}
+		t.Run(tt.name, func(t *testing.T) {
+			got := translateGitHubReviewState(tt.input)
+			if got != tt.want {
+				t.Errorf("translateGitHubReviewState(%q) = %q, want %q", tt.input, got, tt.want)
+			}
+		})
 	}
 }
 
-- 
2.47.3


From 027bad2f7cbaa314c7e04d515baaef4d67b79c92 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Wed, 13 May 2026 03:06:00 -0700
Subject: [PATCH 7/7] fix(github): add DismissReview Event comment; use
 t.Fatalf for routing assertions

- Add comment in DismissReview explaining why the Event field is required
  by the GitHub API even though DISMISS is the only valid value (#18652).
- Change t.Errorf to t.Fatalf for method/path routing assertions in test
  handlers so failures are immediately fatal instead of silently
  continuing handler execution (#18653).
---
 github/review.go      |  4 +++-
 github/review_test.go | 16 ++++++++--------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/github/review.go b/github/review.go
index bd2e05a..1a54fcf 100644
--- a/github/review.go
+++ b/github/review.go
@@ -194,7 +194,9 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
 
 	payload := dismissReviewRequest{
 		Message: message,
-		Event:   "DISMISS",
+		// Event is required by the GitHub API for dismissal requests, even though
+		// "DISMISS" is the only valid value for this endpoint.
+		Event: "DISMISS",
 	}
 
 	data, err := json.Marshal(payload)
diff --git a/github/review_test.go b/github/review_test.go
index 33f8743..da17d8e 100644
--- a/github/review_test.go
+++ b/github/review_test.go
@@ -17,10 +17,10 @@ import (
 func TestPostReview_HappyPath(t *testing.T) {
 	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != "POST" {
-			t.Errorf("expected POST, got %s", r.Method)
+			t.Fatalf("expected POST, got %s", r.Method)
 		}
 		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
+			t.Fatalf("unexpected path: %s", r.URL.Path)
 		}
 		if r.Header.Get("Content-Type") != "application/json" {
 			t.Errorf("expected Content-Type application/json, got %q", r.Header.Get("Content-Type"))
@@ -150,10 +150,10 @@ func TestPostReview_MalformedResponse(t *testing.T) {
 func TestListReviews_HappyPath(t *testing.T) {
 	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != "GET" {
-			t.Errorf("expected GET, got %s", r.Method)
+			t.Fatalf("expected GET, got %s", r.Method)
 		}
 		if r.URL.Path != "/repos/owner/repo/pulls/3/reviews" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
+			t.Fatalf("unexpected path: %s", r.URL.Path)
 		}
 		json.NewEncoder(w).Encode([]map[string]interface{}{
 			{
@@ -250,10 +250,10 @@ func TestListReviews_401(t *testing.T) {
 func TestDeleteReview_HappyPath(t *testing.T) {
 	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != "DELETE" {
-			t.Errorf("expected DELETE, got %s", r.Method)
+			t.Fatalf("expected DELETE, got %s", r.Method)
 		}
 		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/42" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
+			t.Fatalf("unexpected path: %s", r.URL.Path)
 		}
 		w.WriteHeader(204)
 	})
@@ -284,10 +284,10 @@ func TestDeleteReview_422_SubmittedReview(t *testing.T) {
 func TestDismissReview_HappyPath(t *testing.T) {
 	c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
 		if r.Method != "PUT" {
-			t.Errorf("expected PUT, got %s", r.Method)
+			t.Fatalf("expected PUT, got %s", r.Method)
 		}
 		if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/10/dismissals" {
-			t.Errorf("unexpected path: %s", r.URL.Path)
+			t.Fatalf("unexpected path: %s", r.URL.Path)
 		}
 
 		body, _ := io.ReadAll(r.Body)
-- 
2.47.3