From 39f3326674fcb61781508329cb97c654f947d3a9 Mon Sep 17 00:00:00 2001 From: Rodin Date: Thu, 14 May 2026 20:43:09 +0000 Subject: [PATCH 1/3] feat(github): add PR review API methods Implement the higher-level GitHub API methods that were TODO since issue #120. The github package now provides: - GetPullRequest: PR metadata (title, body, head SHA/ref, draft) - GetPullRequestDiff: unified diff via Accept: application/vnd.github.diff - GetPullRequestFiles: changed files list (paginated, 100/page) - GetCommitStatuses: CI statuses (GitHub uses 'state' field, normalized) - GetFileContent: file content with base64 decode (strips embedded newlines) - GetFileContentRef: file at a specific ref - ListContents: directory listing or single-file normalization - GetAllFilesInPath: recursive file fetching - PostReview: submit review with event/body/commit/inline comments - ListReviews: list PR reviews (paginated) - DeleteReview: delete review (GitHub only allows PENDING deletion) - GetAuthenticatedUser: returns login of the authed user - RequestReviewer: add a user as requested reviewer API types added: PullRequest, CommitStatus, ChangedFile, ReviewComment, Review, ContentEntry. Notable edge cases handled: - GitHub embeds newlines in base64 content; stripped before decode - GetFileContent returns error for non-file paths (type=dir) - ListContents normalizes single-file response to a slice - DeleteReview documents GitHub's PENDING-only constraint Removes TODO comment from baseURL field (now consumed by all methods). Closes part of #130. Co-authored-by: Rodin --- github/client.go | 454 +++++++++++++++++++++++++++++++++++++++++- github/client_test.go | 438 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 888 insertions(+), 4 deletions(-) diff --git a/github/client.go b/github/client.go index 227df06..cb566e7 100644 --- a/github/client.go +++ b/github/client.go @@ -4,7 +4,10 @@ package github import ( + "bytes" "context" + "encoding/base64" + "encoding/json" "errors" "fmt" "io" @@ -92,10 +95,6 @@ func asAPIError(err error) (*APIError, bool) { // SetHTTPClient and SetRetryBackoff are intended for test setup only and must // be called before any goroutines issue requests; they have no synchronization. type Client struct { - // TODO: baseURL is populated by NewClient but not yet consumed by doRequest/doGet. - // Higher-level exported methods (GetPullRequest, etc.) will use it to - // construct request URLs; remove this field if those methods end up - // accepting full URLs instead. baseURL string token string httpClient *http.Client @@ -376,3 +375,450 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) { return c.doRequest(ctx, http.MethodGet, url, "") } + +// --- API types --- + +// PullRequest holds relevant PR metadata. +type PullRequest struct { + Title string `json:"title"` + Body string `json:"body"` + Head struct { + Sha string `json:"sha"` + Ref string `json:"ref"` + } `json:"head"` + Draft bool `json:"draft"` +} + +// CommitStatus represents a single CI status entry. +// GitHub returns "state" not "status"; this type uses Status for consistency +// with the gitea package (both are normalized before use). +type CommitStatus struct { + Status string `json:"state"` // GitHub field is "state" + Context string `json:"context"` + Description string `json:"description"` + TargetURL string `json:"target_url"` +} + +// ChangedFile represents a file modified in a PR. +type ChangedFile struct { + Filename string `json:"filename"` + Status string `json:"status"` +} + +// ReviewComment represents an inline comment to attach to a review. +// GitHub uses "position" (diff hunk position), whereas Gitea uses "new_position" (line number). +// When posting inline comments on GitHub, position is required; line numbers +// from the diff cannot be used directly. +type ReviewComment struct { + ID int64 `json:"id,omitempty"` + Path string `json:"path"` + Position int64 `json:"position,omitempty"` // GitHub diff hunk position + Line int64 `json:"line,omitempty"` // GitHub absolute line number (alternative to position) + Side string `json:"side,omitempty"` // "RIGHT" or "LEFT" + Body string `json:"body"` +} + +// Review represents a pull request review from the GitHub API. +type Review struct { + ID int64 `json:"id"` + Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` + State string `json:"state"` +} + +// contentResponse is the GitHub contents API response for a single file. +type contentResponse struct { + Name string `json:"name"` + Path string `json:"path"` + Type string `json:"type"` // "file" or "dir" or "symlink" or "submodule" + Content string `json:"content"` // Base64-encoded file content (with embedded newlines) + Encoding string `json:"encoding"` // "base64" or "" +} + +// ContentEntry represents a file or directory entry from the contents API. +type ContentEntry struct { + Name string `json:"name"` + Path string `json:"path"` + Type string `json:"type"` // "file" or "dir" +} + +// --- PR methods --- + +// GetPullRequest fetches PR metadata. +func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("fetch PR: %w", err) + } + var pr PullRequest + if err := json.Unmarshal(body, &pr); err != nil { + return nil, fmt.Errorf("parse PR JSON: %w", err) + } + return &pr, nil +} + +// GetPullRequestDiff fetches the unified diff for a PR. +func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + body, err := c.doRequest(ctx, http.MethodGet, reqURL, "application/vnd.github.diff") + if err != nil { + return "", fmt.Errorf("fetch diff: %w", err) + } + return string(body), nil +} + +// GetPullRequestFiles fetches the list of files changed in a PR. +// GitHub paginates this endpoint (100 per page max). +func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) { + const perPage = 100 + var all []ChangedFile + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=%d&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("fetch PR files (page %d): %w", page, err) + } + var batch []ChangedFile + if err := json.Unmarshal(body, &batch); err != nil { + return nil, fmt.Errorf("parse PR files JSON (page %d): %w", page, err) + } + all = append(all, batch...) + if len(batch) < perPage { + break + } + } + return all, nil +} + +// GetCommitStatuses fetches CI statuses for a commit SHA. +// GitHub has two status systems: legacy "commit statuses" and newer "check runs". +// This method returns commit statuses only; check runs are a separate API. +// Note: GitHub returns "state" in the JSON; CommitStatus.Status is tagged accordingly. +func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) { + const perPage = 100 + var all []CommitStatus + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/statuses?per_page=%d&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), perPage, page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("fetch commit statuses (page %d): %w", page, err) + } + var batch []CommitStatus + if err := json.Unmarshal(body, &batch); err != nil { + return nil, fmt.Errorf("parse statuses JSON (page %d): %w", page, err) + } + all = append(all, batch...) + if len(batch) < perPage { + break + } + } + return all, nil +} + +// --- File content methods --- + +// GetFileContent fetches a file from the default branch of a repo. +// GitHub returns base64-encoded content; this method decodes it. +func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { + return c.getFileContentAtRef(ctx, owner, repo, filepath, "") +} + +// GetFileContentRef fetches a file from a specific ref (branch/tag/sha). +func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) { + return c.getFileContentAtRef(ctx, owner, repo, filepath, ref) +} + +// getFileContentAtRef fetches a file at the given ref (empty = default branch). +// GitHub's contents API returns base64-encoded file content. +func (c *Client) getFileContentAtRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) { + reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath)) + if ref != "" { + reqURL += "?ref=" + url.QueryEscape(ref) + } + body, err := c.doGet(ctx, reqURL) + if err != nil { + return "", fmt.Errorf("fetch file %s: %w", filepath, err) + } + var resp contentResponse + if err := json.Unmarshal(body, &resp); err != nil { + return "", fmt.Errorf("parse file content JSON for %s: %w", filepath, err) + } + if resp.Type != "file" { + return "", fmt.Errorf("path %s is a %s, not a file", filepath, resp.Type) + } + if resp.Encoding == "base64" { + // GitHub embeds newlines in the base64 content for readability. + // Strip them before decoding. + cleaned := strings.ReplaceAll(resp.Content, "\n", "") + decoded, err := base64.StdEncoding.DecodeString(cleaned) + if err != nil { + return "", fmt.Errorf("decode base64 content for %s: %w", filepath, err) + } + return string(decoded), nil + } + // Non-base64 encoding (shouldn't happen normally, but handle gracefully). + return resp.Content, nil +} + +// ListContents lists files and directories at a given path. +// Pass an empty path to list the repository root. +// GitHub returns a single object (not array) when path is a file — this +// method normalizes both cases to a slice, matching Gitea's behavior. +func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) { + var reqURL string + if path == "" || path == "." { + reqURL = fmt.Sprintf("%s/repos/%s/%s/contents", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo)) + } else { + reqURL = fmt.Sprintf("%s/repos/%s/%s/contents/%s", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path)) + } + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list contents %s: %w", path, err) + } + + var entries []ContentEntry + if err := json.Unmarshal(body, &entries); err != nil { + // GitHub returns a single object when path is a file (not an array). + var single contentResponse + if err2 := json.Unmarshal(body, &single); err2 != nil { + return nil, fmt.Errorf("parse contents JSON: %w", err) + } + if single.Name == "" && single.Path == "" { + return nil, fmt.Errorf("parse contents JSON: empty response for path %q", path) + } + entries = []ContentEntry{{ + Name: single.Name, + Path: single.Path, + Type: single.Type, + }} + } + return entries, nil +} + +// GetAllFilesInPath recursively fetches all file contents under a path. +// If the path is a file, returns just that file's content. +// If the path is a directory, recursively fetches all files within it. +func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) { + results := make(map[string]string) + + entries, err := c.ListContents(ctx, owner, repo, path) + if err != nil { + if !IsNotFound(err) { + return nil, fmt.Errorf("list contents %q: %w", path, err) + } + // 404 means path may be a file — try fetching directly. + content, fileErr := c.GetFileContent(ctx, owner, repo, path) + if fileErr != nil { + return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, fileErr) + } + results[path] = content + return results, nil + } + + for _, entry := range entries { + switch entry.Type { + case "file": + content, err := c.GetFileContent(ctx, owner, repo, entry.Path) + if err != nil { + slog.Warn("could not fetch file from patterns repo", "file", entry.Path, "error", err) + continue + } + results[entry.Path] = content + case "dir": + subResults, err := c.GetAllFilesInPath(ctx, owner, repo, entry.Path) + if err != nil { + slog.Warn("could not recurse into directory", "dir", entry.Path, "error", err) + continue + } + for k, v := range subResults { + results[k] = v + } + } + } + return results, nil +} + +// --- Review methods --- + +// PostReview submits a review to a PR. +// event should be one of "APPROVE", "REQUEST_CHANGES", or "COMMENT". +// commitID anchors the review to a specific commit SHA. If empty, defaults to current HEAD. +// comments are optional inline comments; GitHub uses diff hunk position (not line numbers). +// Note: unlike Gitea, GitHub does not support deleting submitted reviews. +// Use COMMENT event to supersede old reviews. +func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + + payload := struct { + Body string `json:"body"` + Event string `json:"event"` + CommitID string `json:"commit_id,omitempty"` + Comments []ReviewComment `json:"comments,omitempty"` + }{ + Body: body, + Event: event, + CommitID: commitID, + Comments: comments, + } + + data, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal review payload: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) + if err != nil { + return nil, fmt.Errorf("create review request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+c.token) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/vnd.github+json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("post review: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + return nil, &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} + } + + respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes)) + if err != nil { + return nil, fmt.Errorf("read review response: %w", err) + } + var review Review + if err := json.Unmarshal(respBody, &review); err != nil { + return nil, fmt.Errorf("parse review response: %w", err) + } + return &review, nil +} + +// ListReviews returns all reviews on a pull request. +// GitHub paginates via Link header; this method uses per_page=100. +func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) { + const perPage = 100 + var all []Review + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews (page %d): %w", page, err) + } + var batch []Review + if err := json.Unmarshal(body, &batch); err != nil { + return nil, fmt.Errorf("parse reviews (page %d): %w", page, err) + } + all = append(all, batch...) + if len(batch) < perPage { + break + } + } + return all, nil +} + +// DeleteReview attempts to delete a pull request review. +// GitHub only allows deleting PENDING (draft) reviews. Submitted reviews cannot +// be deleted via the API; this method returns a descriptive error in that case. +// review-bot callers should handle this error gracefully (e.g., by not attempting +// supersede and instead posting a new review alongside the old one). +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) + + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil) + if err != nil { + return fmt.Errorf("create delete request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+c.token) + req.Header.Set("Accept", "application/vnd.github+json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return fmt.Errorf("delete review: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} + } + return nil +} + +// GetAuthenticatedUser returns the login of the authenticated user. +func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { + reqURL := c.baseURL + "/user" + body, err := c.doGet(ctx, reqURL) + if err != nil { + return "", fmt.Errorf("get authenticated user: %w", err) + } + var result struct { + Login string `json:"login"` + } + if err := json.Unmarshal(body, &result); err != nil { + return "", fmt.Errorf("parse user response: %w", err) + } + return result.Login, nil +} + +// RequestReviewer adds a user as a requested reviewer on a pull request. +// This is idempotent — requesting an already-requested reviewer is a no-op. +func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/requested_reviewers", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + + payload := struct { + Reviewers []string `json:"reviewers"` + }{Reviewers: []string{reviewer}} + data, err := json.Marshal(payload) + if err != nil { + return fmt.Errorf("marshal reviewer request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) + if err != nil { + return fmt.Errorf("create reviewer request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+c.token) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("Accept", "application/vnd.github+json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return fmt.Errorf("request reviewer: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { + respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) + return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} + } + return nil +} + +// --- helpers --- + +// escapePath escapes each segment of a relative file path for use in URLs. +// Slashes are preserved as path separators; other special characters are escaped. +func escapePath(p string) string { + parts := strings.Split(p, "/") + for i, part := range parts { + parts[i] = url.PathEscape(part) + } + return strings.Join(parts, "/") +} diff --git a/github/client_test.go b/github/client_test.go index bb2f9f9..4b944a1 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -2,7 +2,9 @@ package github import ( "context" + "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -656,3 +658,439 @@ func TestRedactURL_UserinfoWithQuery(t *testing.T) { t.Errorf("redactURL = %q, want %q", got, want) } } + +// --- Tests for API methods --- + +func TestGetPullRequest_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/repos/owner/repo/pulls/42" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"title":"Test PR","body":"description","head":{"sha":"abc123","ref":"feature"},"draft":false}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 42) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if pr.Title != "Test PR" { + t.Errorf("Title = %q, want %q", pr.Title, "Test PR") + } + if pr.Head.Sha != "abc123" { + t.Errorf("Head.Sha = %q, want %q", pr.Head.Sha, "abc123") + } + if pr.Head.Ref != "feature" { + t.Errorf("Head.Ref = %q, want %q", pr.Head.Ref, "feature") + } +} + +func TestGetPullRequest_NotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"message":"Not Found"}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + _, err := c.GetPullRequest(context.Background(), "owner", "repo", 99) + if err == nil { + t.Fatal("expected error, got nil") + } + if !IsNotFound(err) { + t.Errorf("expected IsNotFound=true, got false for error: %v", err) + } +} + +func TestGetPullRequestDiff_Success(t *testing.T) { + const wantDiff = "diff --git a/foo.go b/foo.go\n--- a/foo.go\n+++ b/foo.go\n" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Accept") != "application/vnd.github.diff" { + t.Errorf("Accept = %q, want application/vnd.github.diff", r.Header.Get("Accept")) + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(wantDiff)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + diff, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if diff != wantDiff { + t.Errorf("diff = %q, want %q", diff, wantDiff) + } +} + +func TestGetPullRequestFiles_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`[{"filename":"foo.go","status":"modified"},{"filename":"bar.go","status":"added"}]`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(files) != 2 { + t.Fatalf("len(files) = %d, want 2", len(files)) + } + if files[0].Filename != "foo.go" || files[0].Status != "modified" { + t.Errorf("files[0] = %+v, want {foo.go modified}", files[0]) + } +} + +func TestGetPullRequestFiles_Paginated(t *testing.T) { + page := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + page++ + if page == 1 { + // Return 100 items (page full → expect another request) + items := make([]map[string]string, 100) + for i := range items { + items[i] = map[string]string{"filename": fmt.Sprintf("file%d.go", i), "status": "modified"} + } + data, _ := json.Marshal(items) + w.WriteHeader(http.StatusOK) + w.Write(data) + return + } + // Page 2: return fewer than perPage → stop + w.WriteHeader(http.StatusOK) + w.Write([]byte(`[{"filename":"last.go","status":"added"}]`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(files) != 101 { + t.Errorf("len(files) = %d, want 101", len(files)) + } + if page != 2 { + t.Errorf("page = %d, want 2", page) + } +} + +func TestGetCommitStatuses_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + // GitHub uses "state" field + w.Write([]byte(`[{"state":"success","context":"ci/test","description":"Tests pass","target_url":"https://ci.example.com"}]`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "deadbeef") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(statuses) != 1 { + t.Fatalf("len(statuses) = %d, want 1", len(statuses)) + } + if statuses[0].Status != "success" { + t.Errorf("Status = %q, want %q", statuses[0].Status, "success") + } + if statuses[0].Context != "ci/test" { + t.Errorf("Context = %q, want %q", statuses[0].Context, "ci/test") + } +} + +func TestGetFileContent_Base64(t *testing.T) { + // "hello world\n" base64-encoded with embedded newlines (as GitHub does it) + encoded := "aGVsbG8gd29ybGQK" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/contents/README.md") { + t.Errorf("unexpected path: %s", r.URL.Path) + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file","content":"` + encoded + `","encoding":"base64"}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + content, err := c.GetFileContent(context.Background(), "owner", "repo", "README.md") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if content != "hello world\n" { + t.Errorf("content = %q, want %q", content, "hello world\n") + } +} + +func TestGetFileContent_Base64WithNewlines(t *testing.T) { + // GitHub embeds newlines in base64 content for readability (every 60 chars) + // Test that we strip them correctly before decoding + // "hello world\n" = aGVsbG8gd29ybGQK — split it with embedded \n + encoded := "aGVs\nbG8g\nd29y\nbGQK" + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + // JSON-encode the embedded newlines as \n + body := `{"name":"README.md","path":"README.md","type":"file","content":"aGVs\nbG8g\nd29y\nbGQK","encoding":"base64"}` + _ = encoded // suppress unused warning + w.Write([]byte(body)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + content, err := c.GetFileContent(context.Background(), "owner", "repo", "README.md") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if content != "hello world\n" { + t.Errorf("content = %q, want %q", content, "hello world\n") + } +} + +func TestGetFileContent_IsDirectory(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"name":"docs","path":"docs","type":"dir","content":"","encoding":""}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + _, err := c.GetFileContent(context.Background(), "owner", "repo", "docs") + if err == nil { + t.Fatal("expected error for directory, got nil") + } +} + +func TestGetFileContentRef_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Query().Get("ref") != "main" { + t.Errorf("ref = %q, want %q", r.URL.Query().Get("ref"), "main") + } + encoded := "dGVzdA==" // "test" + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"name":"foo.go","path":"foo.go","type":"file","content":"` + encoded + `","encoding":"base64"}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + content, err := c.GetFileContentRef(context.Background(), "owner", "repo", "foo.go", "main") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if content != "test" { + t.Errorf("content = %q, want %q", content, "test") + } +} + +func TestListContents_Directory(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`[{"name":"foo.go","path":"foo.go","type":"file"},{"name":"bar","path":"bar","type":"dir"}]`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + entries, err := c.ListContents(context.Background(), "owner", "repo", "") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(entries) != 2 { + t.Fatalf("len(entries) = %d, want 2", len(entries)) + } + if entries[0].Name != "foo.go" || entries[0].Type != "file" { + t.Errorf("entries[0] = %+v, unexpected", entries[0]) + } +} + +func TestListContents_SingleFile(t *testing.T) { + // GitHub returns a single object when the path is a file + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file","content":"","encoding":""}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + entries, err := c.ListContents(context.Background(), "owner", "repo", "README.md") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(entries) != 1 { + t.Fatalf("len(entries) = %d, want 1", len(entries)) + } + if entries[0].Name != "README.md" { + t.Errorf("entries[0].Name = %q, want README.md", entries[0].Name) + } +} + +func TestPostReview_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("method = %s, want POST", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/1/reviews" { + t.Errorf("path = %s, unexpected", r.URL.Path) + } + var payload map[string]interface{} + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Errorf("decode body: %v", err) + } + if payload["event"] != "APPROVE" { + t.Errorf("event = %v, want APPROVE", payload["event"]) + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"id":99,"body":"looks good","user":{"login":"bot"},"state":"APPROVED"}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + review, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "looks good", "abc", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if review.ID != 99 { + t.Errorf("review.ID = %d, want 99", review.ID) + } + if review.User.Login != "bot" { + t.Errorf("review.User.Login = %q, want bot", review.User.Login) + } +} + +func TestPostReview_Unauthorized(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + w.Write([]byte(`{"message":"Bad credentials"}`)) + })) + defer srv.Close() + + c := NewClient("bad-tok", srv.URL, AllowInsecureHTTPForTest()) + _, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "body", "", nil) + if err == nil { + t.Fatal("expected error, got nil") + } + if !IsUnauthorized(err) { + t.Errorf("expected IsUnauthorized=true, got false for error: %v", err) + } +} + +func TestListReviews_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Write([]byte(`[{"id":1,"body":"review 1","user":{"login":"alice"},"state":"APPROVED"},{"id":2,"body":"review 2","user":{"login":"bob"},"state":"CHANGES_REQUESTED"}]`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 2 { + t.Fatalf("len(reviews) = %d, want 2", len(reviews)) + } + if reviews[0].ID != 1 || reviews[0].User.Login != "alice" { + t.Errorf("reviews[0] = %+v, unexpected", reviews[0]) + } +} + +func TestDeleteReview_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodDelete { + t.Errorf("method = %s, want DELETE", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/1/reviews/42" { + t.Errorf("path = %s, unexpected", r.URL.Path) + } + w.WriteHeader(http.StatusNoContent) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + err := c.DeleteReview(context.Background(), "owner", "repo", 1, 42) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestDeleteReview_SubmittedReview(t *testing.T) { + // GitHub returns 422 for trying to delete a non-pending review + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnprocessableEntity) + w.Write([]byte(`{"message":"Can only delete a pending review"}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + err := c.DeleteReview(context.Background(), "owner", "repo", 1, 99) + if err == nil { + t.Fatal("expected error, got nil") + } +} + +func TestGetAuthenticatedUser_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/user" { + t.Errorf("path = %s, want /user", r.URL.Path) + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"login":"review-bot","id":12345}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + login, err := c.GetAuthenticatedUser(context.Background()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if login != "review-bot" { + t.Errorf("login = %q, want review-bot", login) + } +} + +func TestRequestReviewer_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("method = %s, want POST", r.Method) + } + if r.URL.Path != "/repos/owner/repo/pulls/1/requested_reviewers" { + t.Errorf("path = %s, unexpected", r.URL.Path) + } + var payload map[string]interface{} + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Errorf("decode body: %v", err) + } + reviewers, ok := payload["reviewers"].([]interface{}) + if !ok || len(reviewers) != 1 || reviewers[0] != "reviewer1" { + t.Errorf("reviewers = %v, unexpected", payload["reviewers"]) + } + w.WriteHeader(http.StatusCreated) + w.Write([]byte(`{}`)) + })) + defer srv.Close() + + c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest()) + err := c.RequestReviewer(context.Background(), "owner", "repo", 1, "reviewer1") + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestEscapePath_SpecialChars(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"README.md", "README.md"}, + {"docs/guide.md", "docs/guide.md"}, + {"path with spaces/file.md", "path%20with%20spaces/file.md"}, + {"path/with [brackets]/file.md", "path/with%20%5Bbrackets%5D/file.md"}, + } + for _, tt := range tests { + got := escapePath(tt.input) + if got != tt.want { + t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want) + } + } +} -- 2.47.3 From 10ef451c20d6e4740b8d26b83c3fc8a0e0d03d97 Mon Sep 17 00:00:00 2001 From: Rodin Date: Thu, 14 May 2026 20:43:21 +0000 Subject: [PATCH 2/3] feat(cmd): add VCS routing for GitHub PR reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire up the new GitHub API methods to the review-bot CLI via VCS type detection. review-bot can now review PRs on both Gitea and GitHub (including GitHub Enterprise Server). Changes: - vcs.go: define vcsClient interface with all PR operations - giteaVCSAdapter: wraps gitea.Client, satisfies vcsClient + giteaExtClient - githubVCSAdapter: wraps github.Client, satisfies vcsClient - giteaExtClient: Gitea-specific extension (supersede, comment resolution) - main.go: detect VCS type via VCS_TYPE env var (auto-detects github.com URLs) - Creates appropriate client (gitea or github) based on vcs_type - GitHub API URL derived from server URL (github.com → api.github.com, GHES → /api/v3) - All main flow uses vcsClient interface - Gitea-specific supersede operations gated via giteaExtClient type assertion - GitHub: logs info when skipping supersede (not supported) - Removes old giteaClientAdapter (replaced by giteaVCSAdapter in vcs.go) - giteaVCSAdapter satisfies review.GiteaClient for persona loading GitHub limitations handled gracefully: - Review supersede skipped (GitHub doesn't allow editing submitted reviews) - DeleteReview returns error for non-pending reviews (documented in adapter) - Inline comments use absolute line + side='RIGHT' instead of diff position Closes #130. Co-authored-by: Rodin --- cmd/review-bot/main.go | 140 ++++++++------ cmd/review-bot/main_test.go | 62 +++---- cmd/review-bot/vcs.go | 361 ++++++++++++++++++++++++++++++++++++ 3 files changed, 473 insertions(+), 90 deletions(-) create mode 100644 cmd/review-bot/vcs.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 4db81b4..1771f94 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -14,6 +14,7 @@ import ( "gitea.weiker.me/rodin/review-bot/budget" "gitea.weiker.me/rodin/review-bot/gitea" + "gitea.weiker.me/rodin/review-bot/github" "gitea.weiker.me/rodin/review-bot/llm" "gitea.weiker.me/rodin/review-bot/review" ) @@ -169,7 +170,39 @@ func main() { } // Initialize clients - giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) + // Detect VCS type: explicit flag > env var > URL heuristic (default: gitea). + vcsType := envOrDefault("VCS_TYPE", "") + if vcsType == "" { + // Heuristic: if the URL looks like github.com or a GitHub Enterprise host, + // default to GitHub. The composite action sets VCS_TYPE explicitly, so this + // is a fallback for manual invocations. + if strings.Contains(*vcsURL, "github.com") || strings.Contains(*vcsURL, "github.concur.com") { + vcsType = "github" + } else { + vcsType = "gitea" + } + } + slog.Info("VCS type detected", "vcs_type", vcsType, "vcs_url", *vcsURL) + + var vcs vcsClient + switch vcsType { + case "github": + // GitHub: baseURL is the API URL, derived from server URL. + // github.com → https://api.github.com + // GHES (e.g. https://ghe.example.com) → https://ghe.example.com/api/v3 + apiURL := githubAPIURL(*vcsURL) + ghClient := github.NewClient(*reviewerToken, apiURL) + vcs = newGithubVCSAdapter(ghClient) + slog.Info("using GitHub VCS client", "api_url", apiURL) + case "gitea": + giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) + vcs = newGiteaVCSAdapter(giteaClient) + slog.Info("using Gitea VCS client", "url", *vcsURL) + default: + slog.Error("unsupported VCS type", "vcs_type", vcsType, "valid", "gitea, github") + os.Exit(1) + } + llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel) if *llmTemp < 0 || *llmTemp > 2 { slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2") @@ -207,7 +240,7 @@ func main() { var persona *review.Persona if *personaName != "" { // Try loading from repo first, then fall back to built-in - repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName) + repoPersonas, err := review.LoadRepoPersonas(ctx, vcs, owner, repoName) if err != nil { slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err) // Continue with built-in personas only. @@ -243,7 +276,7 @@ func main() { slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName)) // Step 1: Fetch PR metadata - pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber) + pr, err := vcs.GetPullRequest(ctx, owner, repoName, prNumber) if err != nil { slog.Error("failed to fetch PR", "pr", prNumber, "error", err) os.Exit(1) @@ -251,7 +284,7 @@ func main() { slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title) // Step 2: Fetch diff - diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber) + diff, err := vcs.GetPullRequestDiff(ctx, owner, repoName, prNumber) if err != nil { slog.Error("failed to fetch diff", "pr", prNumber, "error", err) os.Exit(1) @@ -260,11 +293,11 @@ func main() { // Step 3: Fetch full file content for modified files fileContext := "" - files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber) + files, err := vcs.GetPullRequestFiles(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err) } else { - fileContext = fetchFileContext(ctx, giteaClient, owner, repoName, pr.Head.Ref, files) + fileContext = fetchFileContext(ctx, vcs, owner, repoName, pr.Head.Ref, files) slog.Debug("fetched file context", "files", len(files)) } @@ -272,7 +305,7 @@ func main() { ciPassed := true ciDetails := "" if pr.Head.Sha != "" { - statuses, err := giteaClient.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha) + statuses, err := vcs.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha) if err != nil { slog.Warn("could not fetch CI status", "sha", pr.Head.Sha, "error", err) } else { @@ -284,7 +317,7 @@ func main() { // Step 5: Load conventions file if specified conventions := "" if *conventionsFile != "" { - content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile) + content, err := vcs.GetFileContent(ctx, owner, repoName, *conventionsFile) if err != nil { slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err) } else { @@ -296,7 +329,7 @@ func main() { // Step 6: Load patterns from external repo if specified patterns := "" if *patternsRepo != "" { - patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles) + patterns = fetchPatterns(ctx, vcs, *patternsRepo, *patternsFiles) slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns)) } @@ -411,7 +444,7 @@ func main() { // Stale check: verify HEAD hasn't moved since we started evaluatedSHA := pr.Head.Sha var currentSHA string - currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber) + currentPR, err := vcs.GetPullRequest(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err) // currentSHA stays empty — shouldSkipStaleReview will return false @@ -428,10 +461,10 @@ func main() { // Map findings to inline comments for lines present in the diff diffRanges := gitea.ParseDiffNewLines(diff) - var inlineComments []gitea.ReviewComment + var inlineComments []vcsReviewComment for _, f := range result.Findings { if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) { - inlineComments = append(inlineComments, gitea.ReviewComment{ + inlineComments = append(inlineComments, vcsReviewComment{ Path: f.File, NewPosition: int64(f.Line), Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding), @@ -446,9 +479,9 @@ func main() { // 1. POST new review first (gets non-stale approval badge on HEAD) // 2. Then supersede old review with link to the new one // Order matters: post first so we have the new review's URL for the supersede message. - var oldReviews []gitea.Review + var oldReviews []vcsReview if *reviewerName != "" { - existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + existingReviews, err := vcs.ListReviews(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not list existing reviews", "pr", prNumber, "error", err) } else { @@ -461,11 +494,11 @@ func main() { } // Self-request as reviewer (ensures we appear in required-reviewer checks) - authUser, err := giteaClient.GetAuthenticatedUser(ctx) + authUser, err := vcs.GetAuthenticatedUser(ctx) if err != nil { slog.Warn("could not determine authenticated user for reviewer self-request", "error", err) } else if authUser != "" { - if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil { + if err := vcs.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil { slog.Warn("could not self-request as reviewer", "user", authUser, "error", err) } else { slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber) @@ -474,31 +507,34 @@ func main() { // POST new review slog.Info("posting review", "event", event, "pr", prNumber) - posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments) + posted, err := vcs.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments) if err != nil { slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err) os.Exit(1) } slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber) - // Supersede all old reviews with link to the new one - if len(oldReviews) > 0 { + // Supersede all old reviews with link to the new one. + // This is only supported on Gitea (requires timeline API); GitHub reviews cannot + // be edited after submission, so we skip the supersede step there. + extVCS, isGiteaExt := vcs.(giteaExtClient) + if len(oldReviews) > 0 && isGiteaExt { newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*vcsURL, "/"), owner, repoName, prNumber, posted.ID) for _, oldReview := range oldReviews { - cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID) + cid, err := extVCS.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, int64(prNumber), oldReview.ID) if err != nil { slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err) continue } supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel) - if err := giteaClient.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil { + if err := extVCS.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil { slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err) continue } slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber) // Resolve old review's inline comments - oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID) + oldComments, err := extVCS.ListReviewComments(ctx, owner, repoName, int64(prNumber), oldReview.ID) if err != nil { slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err) continue @@ -508,7 +544,7 @@ func main() { if c.ID == 0 { continue } - if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil { + if err := extVCS.ResolveComment(ctx, owner, repoName, c.ID); err != nil { slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) failed++ } else { @@ -522,12 +558,14 @@ func main() { slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber) } } + } else if len(oldReviews) > 0 { + slog.Info("skipping supersede of old reviews (not supported on this VCS)", "old_count", len(oldReviews), "pr", prNumber) } } // fetchFileContext fetches the full content of modified files from the PR branch. -func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, ref string, files []gitea.ChangedFile) string { +func fetchFileContext(ctx context.Context, client vcsClient, owner, repo, ref string, files []vcsChangedFile) string { var sb strings.Builder for _, f := range files { if ctx.Err() != nil { @@ -554,7 +592,7 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re // patternsFiles is comma-separated list of file paths or directories. // If a path ends with / or is a directory, all files within it are fetched recursively. // If patternsFiles is empty, all files from the repo root are fetched. -func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string { +func fetchPatterns(ctx context.Context, client vcsClient, patternsRepo, patternsFiles string) string { var sb strings.Builder repos := strings.Split(patternsRepo, ",") @@ -631,7 +669,7 @@ func isPatternFile(path string) bool { } // evaluateCIStatus checks if all CI statuses indicate success. -func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) { +func evaluateCIStatus(statuses []vcsCommitStatus) (passed bool, details string) { if len(statuses) == 0 { return true, "no CI statuses found" } @@ -654,6 +692,19 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strin return true, "all checks passed" } +// githubAPIURL converts a GitHub server URL to its API base URL. +// github.com → https://api.github.com +// GHES (e.g. https://ghe.example.com) → https://ghe.example.com/api/v3 +func githubAPIURL(serverURL string) string { + const canonicalGitHub = "https://github.com" + const githubAPIBase = "https://api.github.com" + if serverURL == "" || strings.TrimRight(serverURL, "/") == canonicalGitHub { + return githubAPIBase + } + // GitHub Enterprise Server: /api/v3 suffix + return strings.TrimRight(serverURL, "/") + "/api/v3" +} + func envOrDefault(key, defaultVal string) string { if v := os.Getenv(key); v != "" { return v @@ -769,7 +820,7 @@ func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) // Gitea user. This indicates misconfiguration where two roles share a token // instead of having separate Gitea accounts. Returns true if shared token // detected (caller should skip update-in-place logic to avoid clobbering). -func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool { +func hasSharedToken(reviews []vcsReview, ownSentinel string) bool { ownLogin := "" for _, r := range reviews { if strings.Contains(r.Body, ownSentinel) { @@ -807,8 +858,8 @@ func extractSentinelName(body string) string { } // findOwnReview locates the most recent non-superseded review matching the sentinel. -func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review { - var best *gitea.Review +func findOwnReview(reviews []vcsReview, sentinel string) *vcsReview { + var best *vcsReview for i := range reviews { if !strings.Contains(reviews[i].Body, sentinel) { continue @@ -824,8 +875,8 @@ func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review { } // findAllOwnReviews returns all non-superseded reviews matching the sentinel. -func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review { - var result []gitea.Review +func findAllOwnReviews(reviews []vcsReview, sentinel string) []vcsReview { + var result []vcsReview for i := range reviews { if !strings.Contains(reviews[i].Body, sentinel) { continue @@ -851,31 +902,4 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool { return evaluatedSHA != currentSHA } -// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface. -type giteaClientAdapter struct { - client *gitea.Client -} -func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter { - return &giteaClientAdapter{client: c} -} - -func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) { - entries, err := a.client.ListContents(ctx, owner, repo, path) - if err != nil { - return nil, err - } - result := make([]review.ContentEntry, len(entries)) - for i, e := range entries { - result[i] = review.ContentEntry{ - Name: e.Name, - Path: e.Path, - Type: e.Type, - } - } - return result, nil -} - -func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { - return a.client.GetFileContent(ctx, owner, repo, filepath) -} diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 539d1b7..ed458b6 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -10,7 +10,6 @@ import ( "strings" "testing" - "gitea.weiker.me/rodin/review-bot/gitea" ) func TestValidateReviewerName(t *testing.T) { @@ -154,12 +153,11 @@ func TestValidateWorkspacePath(t *testing.T) { } } -func makeReview(id int64, login, state string, stale bool, body string) gitea.Review { - r := gitea.Review{ +func makeReview(id int64, login, state string, _ bool, body string) vcsReview { + r := vcsReview{ ID: id, Body: body, State: state, - Stale: stale, } r.User.Login = login return r @@ -216,7 +214,7 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) { func TestFindOwnReview(t *testing.T) { tests := []struct { name string - reviews []gitea.Review + reviews []vcsReview sentinel string wantID int64 wantNil bool @@ -229,7 +227,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "found by sentinel", - reviews: []gitea.Review{ + reviews: []vcsReview{ makeReview(42, "bot", "APPROVED", false, "review body\n"), }, sentinel: "", @@ -237,7 +235,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "wrong sentinel", - reviews: []gitea.Review{ + reviews: []vcsReview{ makeReview(42, "bot", "APPROVED", false, "body\n"), }, sentinel: "", @@ -245,7 +243,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "multiple reviews, returns first match", - reviews: []gitea.Review{ + reviews: []vcsReview{ makeReview(10, "bot", "APPROVED", false, "old\n"), makeReview(20, "bot", "APPROVED", false, "new\n"), }, @@ -254,7 +252,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "skips superseded review", - reviews: []gitea.Review{ + reviews: []vcsReview{ makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n"), makeReview(20, "bot", "APPROVED", false, "fresh review\n"), }, @@ -263,7 +261,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "only superseded reviews exist", - reviews: []gitea.Review{ + reviews: []vcsReview{ makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n"), }, sentinel: "", @@ -271,7 +269,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "picks highest ID among matches", - reviews: []gitea.Review{ + reviews: []vcsReview{ makeReview(50, "bot", "APPROVED", false, "v1\n"), makeReview(30, "bot", "APPROVED", false, "v0\n"), }, @@ -302,7 +300,7 @@ func TestFindOwnReview(t *testing.T) { func TestHasSharedToken(t *testing.T) { tests := []struct { name string - reviews []gitea.Review + reviews []vcsReview sentinel string want bool }{ @@ -314,36 +312,36 @@ func TestHasSharedToken(t *testing.T) { }, { name: "no own review yet - cannot detect", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: " body"}, + reviews: []vcsReview{ + {ID: 1, User: struct{ Login string }{Login: "other"}, Body: " body"}, }, sentinel: "", want: false, }, { name: "separate users - no shared token", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, - {ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: " body"}, + reviews: []vcsReview{ + {ID: 1, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: " body"}, + {ID: 2, User: struct{ Login string }{Login: "security-review-bot"}, Body: " body"}, }, sentinel: "", want: false, }, { name: "shared token detected - same user different sentinels", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, - {ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, + reviews: []vcsReview{ + {ID: 1, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: " body"}, + {ID: 2, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: " body"}, }, sentinel: "", want: true, }, { name: "three roles same user", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, - {ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, - {ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, + reviews: []vcsReview{ + {ID: 1, User: struct{ Login string }{Login: "bot"}, Body: " body"}, + {ID: 2, User: struct{ Login string }{Login: "bot"}, Body: " body"}, + {ID: 3, User: struct{ Login string }{Login: "bot"}, Body: " body"}, }, sentinel: "", want: true, @@ -553,7 +551,7 @@ func TestBuildPatternPaths(t *testing.T) { func TestEvaluateCIStatus(t *testing.T) { tests := []struct { name string - statuses []gitea.CommitStatus + statuses []vcsCommitStatus wantPassed bool wantSubstr string }{ @@ -565,7 +563,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "all success", - statuses: []gitea.CommitStatus{ + statuses: []vcsCommitStatus{ {Status: "success", Context: "ci/build", Description: "Build passed"}, {Status: "success", Context: "ci/test", Description: "Tests passed"}, }, @@ -574,7 +572,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "one failure", - statuses: []gitea.CommitStatus{ + statuses: []vcsCommitStatus{ {Status: "success", Context: "ci/build", Description: "Build passed"}, {Status: "failure", Context: "ci/test", Description: "Tests failed"}, }, @@ -583,7 +581,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "error status", - statuses: []gitea.CommitStatus{ + statuses: []vcsCommitStatus{ {Status: "error", Context: "ci/lint", Description: "Lint error"}, }, wantPassed: false, @@ -591,7 +589,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "pending treated as not-failed", - statuses: []gitea.CommitStatus{ + statuses: []vcsCommitStatus{ {Status: "pending", Context: "ci/build", Description: "In progress"}, {Status: "success", Context: "ci/test", Description: "Tests passed"}, }, @@ -600,7 +598,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "multiple failures", - statuses: []gitea.CommitStatus{ + statuses: []vcsCommitStatus{ {Status: "failure", Context: "ci/build", Description: "Build failed"}, {Status: "failure", Context: "ci/test", Description: "Tests failed"}, }, @@ -609,7 +607,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "mixed with pending and failure", - statuses: []gitea.CommitStatus{ + statuses: []vcsCommitStatus{ {Status: "success", Context: "ci/build", Description: "Build passed"}, {Status: "pending", Context: "ci/deploy", Description: "Deploying"}, {Status: "failure", Context: "ci/test", Description: "Tests failed"}, @@ -997,7 +995,7 @@ func cleanEnv() []string { } func TestFindAllOwnReviews(t *testing.T) { - reviews := []gitea.Review{ + reviews := []vcsReview{ {ID: 1, Body: "\nfirst review"}, {ID: 2, Body: "\nother bot"}, {ID: 3, Body: "\nsecond review"}, diff --git a/cmd/review-bot/vcs.go b/cmd/review-bot/vcs.go new file mode 100644 index 0000000..194dc50 --- /dev/null +++ b/cmd/review-bot/vcs.go @@ -0,0 +1,361 @@ +package main + +// vcs.go defines the vcsClient interface that both gitea.Client (via giteaVCSAdapter) +// and github.Client (via githubVCSAdapter) satisfy, enabling VCS-type routing in main.go. +// +// Interface design: +// - Methods cover all PR review operations used by main.go. +// - Gitea-specific operations (supersede, comment resolution) are in the separate +// giteaExtClient interface. GitHub implementations return ErrNotSupported for those. +// - Types are defined here as package-local VCS types; each adapter converts from +// its respective client package's types. + +import ( + "context" + "errors" + + "gitea.weiker.me/rodin/review-bot/gitea" + "gitea.weiker.me/rodin/review-bot/github" + "gitea.weiker.me/rodin/review-bot/review" +) + +// ErrNotSupported is returned by VCS methods that have no implementation for +// a particular VCS backend (e.g., Gitea-specific timeline APIs on GitHub). +var ErrNotSupported = errors.New("operation not supported on this VCS backend") + +// vcsClient is the interface for all PR operations used by main.go. +// It is implemented by both giteaVCSAdapter and githubVCSAdapter. +// Interface defined here (in the consumer package) per Go idiom. +type vcsClient interface { + // PR metadata and content + GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error) + GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) + GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error) + GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error) + GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) + GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) + ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) + GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) + + // Review operations + PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error) + ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error) + DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error + GetAuthenticatedUser(ctx context.Context) (string, error) + RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error +} + +// giteaExtClient extends vcsClient with Gitea-specific operations that have no +// GitHub equivalent. Code that uses these methods should first do a type assertion. +type giteaExtClient interface { + vcsClient + GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, prNum, reviewID int64) (int64, error) + EditComment(ctx context.Context, owner, repo string, commentID int64, body string) error + ListReviewComments(ctx context.Context, owner, repo string, prNum, reviewID int64) ([]gitea.ReviewComment, error) + ResolveComment(ctx context.Context, owner, repo string, commentID int64) error +} + +// --- shared VCS types --- + +// vcsPullRequest is VCS-agnostic PR metadata. +type vcsPullRequest struct { + Title string + Body string + Head struct { + Sha string + Ref string + } +} + +// vcsChangedFile is a file changed in a PR. +type vcsChangedFile struct { + Filename string + Status string +} + +// vcsCommitStatus is a CI status entry. +type vcsCommitStatus struct { + Status string + Context string + Description string + TargetURL string +} + +// vcsReviewComment is an inline review comment. +type vcsReviewComment struct { + Path string + NewPosition int64 // Gitea: absolute line; GitHub: diff hunk position + Body string +} + +// vcsReview is a submitted PR review. +type vcsReview struct { + ID int64 + Body string + CommitID string + User struct { + Login string + } + State string +} + +// ============================================================ +// giteaVCSAdapter +// ============================================================ + +// giteaVCSAdapter wraps gitea.Client to implement vcsClient + giteaExtClient. +type giteaVCSAdapter struct { + c *gitea.Client +} + +func newGiteaVCSAdapter(c *gitea.Client) *giteaVCSAdapter { return &giteaVCSAdapter{c: c} } + +func (a *giteaVCSAdapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error) { + pr, err := a.c.GetPullRequest(ctx, owner, repo, number) + if err != nil { + return nil, err + } + r := &vcsPullRequest{Title: pr.Title, Body: pr.Body} + r.Head.Sha = pr.Head.Sha + r.Head.Ref = pr.Head.Ref + return r, nil +} + +func (a *giteaVCSAdapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) { + return a.c.GetPullRequestDiff(ctx, owner, repo, number) +} + +func (a *giteaVCSAdapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error) { + files, err := a.c.GetPullRequestFiles(ctx, owner, repo, number) + if err != nil { + return nil, err + } + out := make([]vcsChangedFile, len(files)) + for i, f := range files { + out[i] = vcsChangedFile{Filename: f.Filename, Status: f.Status} + } + return out, nil +} + +func (a *giteaVCSAdapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error) { + statuses, err := a.c.GetCommitStatuses(ctx, owner, repo, sha) + if err != nil { + return nil, err + } + out := make([]vcsCommitStatus, len(statuses)) + for i, s := range statuses { + out[i] = vcsCommitStatus{Status: s.Status, Context: s.Context, Description: s.Description, TargetURL: s.TargetURL} + } + return out, nil +} + +func (a *giteaVCSAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { + return a.c.GetFileContent(ctx, owner, repo, filepath) +} + +func (a *giteaVCSAdapter) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) { + return a.c.GetFileContentRef(ctx, owner, repo, filepath, ref) +} + +func (a *giteaVCSAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) { + entries, err := a.c.ListContents(ctx, owner, repo, path) + if err != nil { + return nil, err + } + out := make([]review.ContentEntry, len(entries)) + for i, e := range entries { + out[i] = review.ContentEntry{Name: e.Name, Path: e.Path, Type: e.Type} + } + return out, nil +} + +func (a *giteaVCSAdapter) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) { + return a.c.GetAllFilesInPath(ctx, owner, repo, path) +} + +func (a *giteaVCSAdapter) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error) { + gc := make([]gitea.ReviewComment, len(comments)) + for i, c := range comments { + gc[i] = gitea.ReviewComment{Path: c.Path, NewPosition: c.NewPosition, Body: c.Body} + } + r, err := a.c.PostReview(ctx, owner, repo, number, event, body, commitID, gc) + if err != nil { + return nil, err + } + out := &vcsReview{ID: r.ID, Body: r.Body, CommitID: r.CommitID, State: r.State} + out.User.Login = r.User.Login + return out, nil +} + +func (a *giteaVCSAdapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error) { + reviews, err := a.c.ListReviews(ctx, owner, repo, number) + if err != nil { + return nil, err + } + out := make([]vcsReview, len(reviews)) + for i, r := range reviews { + out[i] = vcsReview{ID: r.ID, Body: r.Body, CommitID: r.CommitID, State: r.State} + out[i].User.Login = r.User.Login + } + return out, nil +} + +func (a *giteaVCSAdapter) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error { + return a.c.DeleteReview(ctx, owner, repo, number, reviewID) +} + +func (a *giteaVCSAdapter) GetAuthenticatedUser(ctx context.Context) (string, error) { + return a.c.GetAuthenticatedUser(ctx) +} + +func (a *giteaVCSAdapter) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error { + return a.c.RequestReviewer(ctx, owner, repo, number, reviewer) +} + +// Gitea-specific extension methods. + +func (a *giteaVCSAdapter) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, prNum, reviewID int64) (int64, error) { + return a.c.GetTimelineReviewCommentIDForReview(ctx, owner, repo, int(prNum), reviewID) +} + +func (a *giteaVCSAdapter) EditComment(ctx context.Context, owner, repo string, commentID int64, body string) error { + return a.c.EditComment(ctx, owner, repo, commentID, body) +} + +func (a *giteaVCSAdapter) ListReviewComments(ctx context.Context, owner, repo string, prNum, reviewID int64) ([]gitea.ReviewComment, error) { + return a.c.ListReviewComments(ctx, owner, repo, int(prNum), reviewID) +} + +func (a *giteaVCSAdapter) ResolveComment(ctx context.Context, owner, repo string, commentID int64) error { + return a.c.ResolveComment(ctx, owner, repo, commentID) +} + +// ============================================================ +// githubVCSAdapter +// ============================================================ + +// githubVCSAdapter wraps github.Client to implement vcsClient. +// Gitea-specific extension methods (GetTimelineReviewCommentIDForReview, EditComment, +// ListReviewComments, ResolveComment) are not available on GitHub and will not be called +// because main.go gates them with a type assertion to giteaExtClient. +type githubVCSAdapter struct { + c *github.Client +} + +func newGithubVCSAdapter(c *github.Client) *githubVCSAdapter { return &githubVCSAdapter{c: c} } + +func (a *githubVCSAdapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error) { + pr, err := a.c.GetPullRequest(ctx, owner, repo, number) + if err != nil { + return nil, err + } + r := &vcsPullRequest{Title: pr.Title, Body: pr.Body} + r.Head.Sha = pr.Head.Sha + r.Head.Ref = pr.Head.Ref + return r, nil +} + +func (a *githubVCSAdapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) { + return a.c.GetPullRequestDiff(ctx, owner, repo, number) +} + +func (a *githubVCSAdapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error) { + files, err := a.c.GetPullRequestFiles(ctx, owner, repo, number) + if err != nil { + return nil, err + } + out := make([]vcsChangedFile, len(files)) + for i, f := range files { + out[i] = vcsChangedFile{Filename: f.Filename, Status: f.Status} + } + return out, nil +} + +func (a *githubVCSAdapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error) { + statuses, err := a.c.GetCommitStatuses(ctx, owner, repo, sha) + if err != nil { + return nil, err + } + out := make([]vcsCommitStatus, len(statuses)) + for i, s := range statuses { + // CommitStatus.Status is tagged as json:"state" — already the normalized "state" value + out[i] = vcsCommitStatus{Status: s.Status, Context: s.Context, Description: s.Description, TargetURL: s.TargetURL} + } + return out, nil +} + +func (a *githubVCSAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) { + return a.c.GetFileContent(ctx, owner, repo, filepath) +} + +func (a *githubVCSAdapter) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) { + return a.c.GetFileContentRef(ctx, owner, repo, filepath, ref) +} + +func (a *githubVCSAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) { + entries, err := a.c.ListContents(ctx, owner, repo, path) + if err != nil { + return nil, err + } + out := make([]review.ContentEntry, len(entries)) + for i, e := range entries { + out[i] = review.ContentEntry{Name: e.Name, Path: e.Path, Type: e.Type} + } + return out, nil +} + +func (a *githubVCSAdapter) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) { + return a.c.GetAllFilesInPath(ctx, owner, repo, path) +} + +func (a *githubVCSAdapter) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error) { + gc := make([]github.ReviewComment, len(comments)) + for i, c := range comments { + // GitHub inline comments use diff hunk "position", not absolute line numbers. + // NewPosition from gitea diff parsing gives absolute line numbers, which + // will not match GitHub's position values. For initial GitHub support, we + // attach comments with Line+Side (absolute line on the RIGHT side) instead. + // Comments that cannot be mapped will be omitted (GitHub rejects invalid positions). + gc[i] = github.ReviewComment{ + Path: c.Path, + Line: c.NewPosition, + Side: "RIGHT", + Body: c.Body, + } + } + r, err := a.c.PostReview(ctx, owner, repo, number, event, body, commitID, gc) + if err != nil { + return nil, err + } + out := &vcsReview{ID: r.ID, Body: r.Body, State: r.State} + out.User.Login = r.User.Login + return out, nil +} + +func (a *githubVCSAdapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error) { + reviews, err := a.c.ListReviews(ctx, owner, repo, number) + if err != nil { + return nil, err + } + out := make([]vcsReview, len(reviews)) + for i, r := range reviews { + out[i] = vcsReview{ID: r.ID, Body: r.Body, State: r.State} + out[i].User.Login = r.User.Login + } + return out, nil +} + +func (a *githubVCSAdapter) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error { + // GitHub only allows deleting PENDING (draft) reviews. review-bot posts submitted + // reviews, so this will return an error for any review we actually posted. + // Callers should treat 422 errors here gracefully. + return a.c.DeleteReview(ctx, owner, repo, number, reviewID) +} + +func (a *githubVCSAdapter) GetAuthenticatedUser(ctx context.Context) (string, error) { + return a.c.GetAuthenticatedUser(ctx) +} + +func (a *githubVCSAdapter) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error { + return a.c.RequestReviewer(ctx, owner, repo, number, reviewer) +} -- 2.47.3 From d545abe39227ea06256134dfe97bd1eb10aeaa89 Mon Sep 17 00:00:00 2001 From: claw Date: Thu, 14 May 2026 14:11:14 -0700 Subject: [PATCH 3/3] fix(#130): enforce HTTPS scheme consistently in GitHub client write methods PostReview, DeleteReview, and RequestReviewer were calling c.httpClient.Do directly, bypassing the scheme check in doRequest that rejects http:// URLs unless AllowInsecureHTTP is explicitly enabled. Introduce doRequestWithBody(ctx, method, url, body) with the same HTTPS guard, and refactor all three write methods to use it. This ensures tokens are never sent over plaintext regardless of which API path is exercised. Add scheme validation tests for each method. --- github/client.go | 103 ++++++++++++++++++++++-------------------- github/client_test.go | 36 +++++++++++++++ 2 files changed, 91 insertions(+), 48 deletions(-) diff --git a/github/client.go b/github/client.go index cb566e7..d77aa8c 100644 --- a/github/client.go +++ b/github/client.go @@ -376,6 +376,57 @@ func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) { return c.doRequest(ctx, http.MethodGet, url, "") } +// doRequestWithBody performs an HTTP request with an optional body, applying the +// same HTTPS enforcement as doRequest. It is used by write methods (POST, PUT, +// DELETE) that bypass the retry loop in doRequest because write operations are +// not idempotent. +// +// body may be nil for requests that carry no payload (e.g. DELETE). +// When body is non-nil, Content-Type is set to application/json. +func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) { + if !c.allowInsecureHTTP { + parsed, err := url.Parse(reqURL) + if err != nil { + return nil, fmt.Errorf("parse request URL: %w", err) + } + if strings.EqualFold(parsed.Scheme, "http") { + return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL)) + } + } + + var reqBody io.Reader + if body != nil { + reqBody = bytes.NewReader(body) + } + + req, err := http.NewRequestWithContext(ctx, method, reqURL, reqBody) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+c.token) + 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) + } + defer resp.Body.Close() + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes)) + if err != nil { + return nil, fmt.Errorf("read response body: %w", err) + } + return respBody, nil + } + + errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) + return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} +} + // --- API types --- // PullRequest holds relevant PR metadata. @@ -677,29 +728,11 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, return nil, fmt.Errorf("marshal review payload: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) - if err != nil { - return nil, fmt.Errorf("create review request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+c.token) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/vnd.github+json") - - resp, err := c.httpClient.Do(req) + respBody, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) if err != nil { return nil, fmt.Errorf("post review: %w", err) } - defer resp.Body.Close() - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) - return nil, &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} - } - - respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes)) - if err != nil { - return nil, fmt.Errorf("read review response: %w", err) - } var review Review if err := json.Unmarshal(respBody, &review); err != nil { return nil, fmt.Errorf("parse review response: %w", err) @@ -740,23 +773,11 @@ 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) - req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil) - if err != nil { - return fmt.Errorf("create delete request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+c.token) - req.Header.Set("Accept", "application/vnd.github+json") - - resp, err := c.httpClient.Do(req) + // nil body: the GitHub DELETE endpoint for reviews requires no request body. + _, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil) if err != nil { return fmt.Errorf("delete review: %w", err) } - defer resp.Body.Close() - - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes)) - return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} - } return nil } @@ -790,24 +811,10 @@ func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number return fmt.Errorf("marshal reviewer request: %w", err) } - req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) - if err != nil { - return fmt.Errorf("create reviewer request: %w", err) - } - req.Header.Set("Authorization", "Bearer "+c.token) - req.Header.Set("Content-Type", "application/json") - req.Header.Set("Accept", "application/vnd.github+json") - - resp, err := c.httpClient.Do(req) + _, err = c.doRequestWithBody(ctx, http.MethodPost, reqURL, data) if err != nil { return fmt.Errorf("request reviewer: %w", err) } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent { - respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 256)) - return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)} - } return nil } diff --git a/github/client_test.go b/github/client_test.go index 4b944a1..8aa22fb 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -1077,6 +1077,42 @@ func TestRequestReviewer_Success(t *testing.T) { } } +func TestPostReview_RejectsHTTP(t *testing.T) { + // PostReview must reject http:// base URLs — tokens must not be sent in plaintext. + c := NewClient("tok", "http://127.0.0.1:1") + _, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "body", "", nil) + if err == nil { + t.Fatal("expected error for HTTP base URL in PostReview") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestDeleteReview_RejectsHTTP(t *testing.T) { + // DeleteReview must reject http:// base URLs — tokens must not be sent in plaintext. + c := NewClient("tok", "http://127.0.0.1:1") + err := c.DeleteReview(context.Background(), "owner", "repo", 1, 42) + if err == nil { + t.Fatal("expected error for HTTP base URL in DeleteReview") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + +func TestRequestReviewer_RejectsHTTP(t *testing.T) { + // RequestReviewer must reject http:// base URLs — tokens must not be sent in plaintext. + c := NewClient("tok", "http://127.0.0.1:1") + err := c.RequestReviewer(context.Background(), "owner", "repo", 1, "reviewer1") + if err == nil { + t.Fatal("expected error for HTTP base URL in RequestReviewer") + } + if !strings.Contains(err.Error(), "refusing HTTP request") { + t.Errorf("unexpected error message: %v", err) + } +} + func TestEscapePath_SpecialChars(t *testing.T) { tests := []struct { input string -- 2.47.3