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
+}

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,

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)
+	}
+}