diff --git a/github/client.go b/github/client.go index dc10c7a..f958bfa 100644 --- a/github/client.go +++ b/github/client.go @@ -4,6 +4,7 @@ package github import ( + "bytes" "context" "errors" "fmt" @@ -192,10 +193,25 @@ 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 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 + + // 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. @@ -246,7 +262,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) } @@ -257,11 +277,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 { @@ -272,11 +295,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 { @@ -314,6 +337,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. @@ -342,3 +372,15 @@ 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, 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"} + } + return c.doRequestCore(ctx, method, reqURL, opts) +} 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/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/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..1a54fcf --- /dev/null +++ b/github/review.go @@ -0,0 +1,212 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "net/http" + "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") + +// 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"` + 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 "CHANGES_REQUESTED": + return "REQUEST_CHANGES" + case "COMMENTED": + return "COMMENT" + default: + // States like APPROVED, DISMISSED, and PENDING pass through unchanged + // 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 + } +} + +// PostReview submits a review on a pull request. +// +// 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). +// +// 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) + + payload := postReviewRequest{ + Body: req.Body, + Event: string(req.Event), + } + + // 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 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, + 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, http.MethodPost, 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, 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) + + // 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 + if errors.As(err, &apiErr) && apiErr.StatusCode == 422 { + return fmt.Errorf("delete review: %w", 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 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) + if err != nil { + return fmt.Errorf("marshal dismiss request: %w", err) + } + + _, err = c.doRequestWithBody(ctx, http.MethodPut, 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..da17d8e --- /dev/null +++ b/github/review_test.go @@ -0,0 +1,391 @@ +package github + +import ( + "context" + "encoding/json" + "errors" + "io" + "net/http" + "strings" + "testing" + + "gitea.weiker.me/rodin/review-bot/vcs" +) + +// --- PostReview tests --- + +func TestPostReview_HappyPath(t *testing.T) { + c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.Method != "POST" { + t.Fatalf("expected POST, got %s", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/5/reviews" { + 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")) + } + + // 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 !strings.Contains(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.Fatalf("expected GET, got %s", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/3/reviews" { + t.Fatalf("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.Fatalf("expected DELETE, got %s", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/42" { + t.Fatalf("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.Fatalf("expected PUT, got %s", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/5/reviews/10/dismissals" { + t.Fatalf("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 { + name string + input string + want string + }{ + {"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 { + 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) + } + }) + } +} + +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) + } +}