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