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 <rodin@forgedthought.ai>
2026-05-14 20:43:09 +00:00
parent bbf3dfbf0d
commit 39f3326674
2 changed files with 888 additions and 4 deletions
@@ -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)
+		}
+	}
+}