feat(github): implement PRReader + FileReader client (#80) #93

Closed
rodin wants to merge 16 commits from review-bot-issue-80 into feature/github-support
8 changed files with 1630 additions and 0 deletions
Showing only changes of commit d1ef1e21e5 - Show all commits
+177
View File
@@ -0,0 +1,177 @@
// Package github provides a client for the GitHub API.
// It supports pull request operations, file content retrieval,
// and review submission for both github.com and GitHub Enterprise.
package github
import (
"context"
"errors"
"fmt"
"io"
"net/http"
"strconv"
"strings"
"time"
Review

[NIT] Package comment mentions "review submission" but this PR doesn't include review endpoints. Consider updating the comment or adding a TODO/ref to avoid misleading users.

**[NIT]** Package comment mentions "review submission" but this PR doesn't include review endpoints. Consider updating the comment or adding a TODO/ref to avoid misleading users.
)
const defaultBaseURL = "https://api.github.com"
// APIError represents an HTTP error response from the GitHub API.
// It carries the status code so callers can distinguish between
// different failure modes (e.g. 404 vs 500).
type APIError struct {
StatusCode int
Body string
}
func (e *APIError) Error() string {
body := e.Body
if len(body) > 200 {
body = body[:200] + "...(truncated)"
}
return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
}
// IsNotFound reports whether an error is an API 404 response.
func IsNotFound(err error) bool {
if apiErr, ok := asAPIError(err); ok {
return apiErr.StatusCode == http.StatusNotFound
Review

[MINOR] APIError.Error includes the response body content in the error string (truncated to 200 bytes). If callers log errors verbatim, this can leak information from upstream responses. Consider redacting or omitting bodies by default, or making inclusion configurable.

**[MINOR]** APIError.Error includes the response body content in the error string (truncated to 200 bytes). If callers log errors verbatim, this can leak information from upstream responses. Consider redacting or omitting bodies by default, or making inclusion configurable.
}
Review

[MINOR] APIError.Error() includes up to 200 characters of the upstream response body in the error message. While sanitized and truncated, this can still leak information into logs if generic error logging prints err.Error(). Consider omitting body content from Error() or making inclusion opt-in to reduce information exposure.

**[MINOR]** APIError.Error() includes up to 200 characters of the upstream response body in the error message. While sanitized and truncated, this can still leak information into logs if generic error logging prints err.Error(). Consider omitting body content from Error() or making inclusion opt-in to reduce information exposure.
return false
}
// IsUnauthorized reports whether an error is an API 401 response.
func IsUnauthorized(err error) bool {
if apiErr, ok := asAPIError(err); ok {
return apiErr.StatusCode == http.StatusUnauthorized
}
return false
}
Review

[NIT] The APIError.Body field is exported, meaning callers can access it directly. However, the Error() method truncates to 200 bytes while the Body field contains the full text. This asymmetry is intentional (truncation is for human-readable error messages) but could surprise callers who log err.Error() thinking they see the full body. The existing doc comment addresses this adequately; no code change needed, just noting the asymmetry.

**[NIT]** The `APIError.Body` field is exported, meaning callers can access it directly. However, the `Error()` method truncates to 200 bytes while the `Body` field contains the full text. This asymmetry is intentional (truncation is for human-readable error messages) but could surprise callers who log `err.Error()` thinking they see the full body. The existing doc comment addresses this adequately; no code change needed, just noting the asymmetry.
func asAPIError(err error) (*APIError, bool) {
if err == nil {
return nil, false
}
var target *APIError
if errors.As(err, &target) {
return target, true
}
return nil, false
Review

[NIT] The APIError.Body field is exported (Body string) which means callers can directly access potentially sensitive upstream response content. The doc comment warns against this, but since APIError is a public type, there's no enforcement. This is acceptable given the documentation but worth noting as a conscious design trade-off.

**[NIT]** The `APIError.Body` field is exported (`Body string`) which means callers can directly access potentially sensitive upstream response content. The doc comment warns against this, but since `APIError` is a public type, there's no enforcement. This is acceptable given the documentation but worth noting as a conscious design trade-off.
}
// Client interacts with the GitHub API.
// A Client is safe for concurrent use by multiple goroutines.
Review

[MINOR] The exported RetryBackoff field is an exposed implementation detail intended primarily for testing. The doc comment says 'Set to shorter durations in tests', but exposing it publicly allows any caller to mutate it after construction. A cleaner approach for the production API would be a WithRetryBackoff([]time.Duration) option or making it unexported and providing a test helper. That said, the test TestDoRequest_RetryAfterDoesNotMutateBackoff correctly verifies the internal copy-on-use behavior, so the current design is safe — just not idiomatic for a public field that is primarily a test hook. Per configuration pattern #2 (Options Struct), test-only configuration hooks should typically not be part of the public API surface.

**[MINOR]** The exported `RetryBackoff` field is an exposed implementation detail intended primarily for testing. The doc comment says 'Set to shorter durations in tests', but exposing it publicly allows any caller to mutate it after construction. A cleaner approach for the production API would be a `WithRetryBackoff([]time.Duration)` option or making it unexported and providing a test helper. That said, the test `TestDoRequest_RetryAfterDoesNotMutateBackoff` correctly verifies the internal copy-on-use behavior, so the current design is safe — just not idiomatic for a public field that is primarily a test hook. Per configuration pattern #2 (Options Struct), test-only configuration hooks should typically not be part of the public API surface.
Review

[NIT] The struct field http *http.Client shadows the net/http package name within the struct definition. While Go allows this and the compiler handles it correctly, it's a minor readability concern. A name like httpClient or hc would avoid the shadowing.

**[NIT]** The struct field `http *http.Client` shadows the `net/http` package name within the struct definition. While Go allows this and the compiler handles it correctly, it's a minor readability concern. A name like `httpClient` or `hc` would avoid the shadowing.
type Client struct {
baseURL string
Review

[NIT] Comment states the Client is safe for concurrent use, but SetHTTPClient and SetRetryBackoff mutate configuration and are not concurrency-safe. Consider clarifying that configuration methods are for setup/testing and should not be called concurrently with requests.

**[NIT]** Comment states the Client is safe for concurrent use, but SetHTTPClient and SetRetryBackoff mutate configuration and are not concurrency-safe. Consider clarifying that configuration methods are for setup/testing and should not be called concurrently with requests.
token string
Review

[NIT] baseURL is configurable. While typically set to GitHub/GHE, if this were to be influenced by untrusted input it could be used for SSRF or to target internal services. Ensure at integration points that baseURL is sourced from a trusted allowlist and not user-controlled.

**[NIT]** baseURL is configurable. While typically set to GitHub/GHE, if this were to be influenced by untrusted input it could be used for SSRF or to target internal services. Ensure at integration points that baseURL is sourced from a trusted allowlist and not user-controlled.
http *http.Client
// RetryBackoff defines the delays between retry attempts for 429 responses.
// RetryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
// If nil, defaults to {1s, 2s}. Set to shorter durations in tests.
RetryBackoff []time.Duration
}
// NewClient creates a new GitHub API client.
// If baseURL is empty, it defaults to https://api.github.com.
// For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3).
func NewClient(token, baseURL string) *Client {
Review

[MINOR] AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext. Consider additional safeguards (e.g., explicit environment gate or failing fast unless a dedicated test flag is present).

**[MINOR]** AllowInsecureHTTP option permits sending credentials over HTTP when enabled. Although documented for trusted/internal use, accidental enablement in production would expose tokens over cleartext. Consider additional safeguards (e.g., explicit environment gate or failing fast unless a dedicated test flag is present).
if baseURL == "" {
baseURL = defaultBaseURL
}
return &Client{
baseURL: strings.TrimRight(baseURL, "/"),
token: token,
http: &http.Client{Timeout: 30 * time.Second},
}
}
// SetHTTPClient sets the underlying HTTP client used for requests.
// This is intended for testing to inject mock transports.
func (c *Client) SetHTTPClient(hc *http.Client) {
c.http = hc
}
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present.
func (c *Client) doRequest(ctx context.Context, method, url string, accept string) ([]byte, error) {
const maxAttempts = 3
backoff := c.RetryBackoff
if backoff == nil {
backoff = []time.Duration{1 * time.Second, 2 * time.Second}
}
const maxErrorBodyBytes = 64 * 1024
var lastErr error
for attempt := 0; attempt < maxAttempts; attempt++ {
if attempt > 0 {
var delay time.Duration
Review

[MINOR] defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts.

**[MINOR]** defaultCheckRedirect allows cross-host redirects (with Authorization stripped). Although token leakage is mitigated, following cross-host redirects can facilitate SSRF-like behavior if baseURL is misconfigured or points to a compromised server. Consider rejecting cross-host redirects by default or enforcing an allowlist of trusted hosts.
if attempt-1 < len(backoff) {
delay = backoff[attempt-1]
}
if delay > 0 {
timer := time.NewTimer(delay)
select {
Review

[MINOR] defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades.

**[MINOR]** defaultCheckRedirect follows HTTPS→HTTP redirects after stripping Authorization. While credentials are protected, this still permits plaintext requests to proceed, which can leak metadata and expands attack surface if a misconfigured or compromised server issues such redirects. Prefer failing closed on protocol downgrades.
case <-timer.C:
Review

[MINOR] The doc comment on defaultCheckRedirect says it "strips the Authorization header on cross-host redirects or protocol downgrades (HTTPS→HTTP) to prevent credential leakage, while still following the redirect." However, a protocol downgrade from HTTPS to HTTP is a genuine security issue — stripping the header and still following is debatable. Consider returning an error on HTTPS→HTTP downgrade rather than silently following. This is a design choice that has security implications, not a bug per se, but worth flagging.

**[MINOR]** The doc comment on `defaultCheckRedirect` says it "strips the Authorization header on cross-host redirects or protocol downgrades (HTTPS→HTTP) to prevent credential leakage, while still following the redirect." However, a protocol downgrade from HTTPS to HTTP is a genuine security issue — stripping the header and still following is debatable. Consider returning an error on HTTPS→HTTP downgrade rather than silently following. This is a design choice that has security implications, not a bug per se, but worth flagging.
Review

[MINOR] defaultCheckRedirect indexes via[len(via)-1] without guarding for len(via) == 0. net/http currently guarantees at least one prior request in via, but adding a len(via) check would make this more robust against misuse.

**[MINOR]** defaultCheckRedirect indexes via[len(via)-1] without guarding for len(via) == 0. net/http currently guarantees at least one prior request in via, but adding a len(via) check would make this more robust against misuse.
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
}
}
}
req, err := http.NewRequestWithContext(ctx, method, url, nil)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+c.token)
if accept != "" {
req.Header.Set("Accept", accept)
} else {
req.Header.Set("Accept", "application/vnd.github+json")
}
resp, err := c.http.Do(req)
if err != nil {
return nil, fmt.Errorf("do request: %w", err)
}
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
body, err := io.ReadAll(resp.Body)
resp.Body.Close()
if err != nil {
return nil, fmt.Errorf("read response body: %w", err)
}
Review

[MINOR] SetHTTPClient allows replacing the underlying http.Client without enforcing a safe CheckRedirect policy, which could cause Authorization to be forwarded to untrusted hosts on redirects. The docstring warns about this, but adding a runtime safeguard (e.g., rejecting clients with nil CheckRedirect or wrapping it) would further reduce misuse risk.

**[MINOR]** SetHTTPClient allows replacing the underlying http.Client without enforcing a safe CheckRedirect policy, which could cause Authorization to be forwarded to untrusted hosts on redirects. The docstring warns about this, but adding a runtime safeguard (e.g., rejecting clients with nil CheckRedirect or wrapping it) would further reduce misuse risk.
return body, nil
}
errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
resp.Body.Close()
lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
// Retry on 429 rate limit
if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 {
// Check for Retry-After header and override backoff if present
if ra := resp.Header.Get("Retry-After"); ra != "" {
if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
if attempt < len(backoff) {
backoff[attempt] = time.Duration(seconds) * time.Second
}
}
}
continue
}
// Don't retry other errors
return nil, lastErr
}
return nil, lastErr
}
// doGet is a convenience wrapper for GET requests with the default Accept header.
func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
return c.doRequest(ctx, http.MethodGet, url, "")
}
+224
View File
@@ -0,0 +1,224 @@
package github
import (
"context"
"net/http"
"net/http/httptest"
"testing"
"time"
)
func TestNewClient_DefaultBaseURL(t *testing.T) {
c := NewClient("test-token", "")
if c.baseURL != "https://api.github.com" {
t.Errorf("expected default base URL, got %q", c.baseURL)
}
}
func TestNewClient_CustomBaseURL(t *testing.T) {
c := NewClient("test-token", "https://github.concur.com/api/v3")
if c.baseURL != "https://github.concur.com/api/v3" {
t.Errorf("expected custom base URL, got %q", c.baseURL)
}
}
func TestNewClient_TrimsTrailingSlash(t *testing.T) {
c := NewClient("test-token", "https://github.concur.com/api/v3/")
if c.baseURL != "https://github.concur.com/api/v3" {
t.Errorf("expected trailing slash trimmed, got %q", c.baseURL)
}
}
func TestDoRequest_SetsAuthHeader(t *testing.T) {
var gotAuth string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotAuth = r.Header.Get("Authorization")
w.WriteHeader(200)
w.Write([]byte("{}"))
}))
defer srv.Close()
c := NewClient("my-token", srv.URL)
c.SetHTTPClient(srv.Client())
_, _ = c.doGet(context.Background(), srv.URL+"/test")
if gotAuth != "Bearer my-token" {
t.Errorf("expected Bearer auth, got %q", gotAuth)
}
}
func TestDoRequest_SetsDefaultAcceptHeader(t *testing.T) {
var gotAccept string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotAccept = r.Header.Get("Accept")
w.WriteHeader(200)
w.Write([]byte("{}"))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, _ = c.doGet(context.Background(), srv.URL+"/test")
if gotAccept != "application/vnd.github+json" {
t.Errorf("expected default Accept header, got %q", gotAccept)
}
}
func TestDoRequest_429Retry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts == 1 {
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
return
}
w.WriteHeader(200)
w.Write([]byte(`{"ok":true}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{10 * time.Millisecond, 10 * time.Millisecond}
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(body) != `{"ok":true}` {
t.Errorf("unexpected body: %s", body)
}
if attempts != 2 {
t.Errorf("expected 2 attempts, got %d", attempts)
}
}
func TestDoRequest_429ExhaustsRetries(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error after exhausting retries")
}
apiErr, ok := err.(*APIError)
if !ok {
t.Fatalf("expected *APIError, got %T", err)
}
if apiErr.StatusCode != 429 {
t.Errorf("expected 429, got %d", apiErr.StatusCode)
}
if attempts != 3 {
t.Errorf("expected 3 attempts, got %d", attempts)
}
}
func TestDoRequest_404NoRetry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
Review

[NIT] TestDoRequest_429ExhaustsRetries type-asserts err.(*APIError) directly rather than using errors.As. Since GetPullRequest and other methods wrap errors with fmt.Errorf("...: %w", err), but doGet/doRequest return the *APIError directly (not wrapped), the direct assertion works here. But it's inconsistent with the project's own IsNotFound/IsUnauthorized helpers which use errors.As. Test code should prefer errors.As for resilience.

**[NIT]** `TestDoRequest_429ExhaustsRetries` type-asserts `err.(*APIError)` directly rather than using `errors.As`. Since `GetPullRequest` and other methods wrap errors with `fmt.Errorf("...: %w", err)`, but `doGet`/`doRequest` return the `*APIError` directly (not wrapped), the direct assertion works here. But it's inconsistent with the project's own `IsNotFound`/`IsUnauthorized` helpers which use `errors.As`. Test code should prefer `errors.As` for resilience.
w.WriteHeader(404)
w.Write([]byte(`{"message":"not found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error for 404")
}
if attempts != 1 {
t.Errorf("expected 1 attempt (no retry on 404), got %d", attempts)
}
}
func TestDoRequest_401NoRetry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
w.WriteHeader(401)
w.Write([]byte(`{"message":"bad credentials"}`))
}))
defer srv.Close()
Review

[NIT] TestDoRequest_429RetryAfterHeader and TestDoRequest_RetryAfterDoesNotMutateBackoff are gated behind testing.Short() but TestDoRequest_429RetryAfterHTTPDate (which waits ~2 seconds) is not. This test will slow down non-short test runs. Consider adding testing.Short() skip to TestDoRequest_429RetryAfterHTTPDate as well for consistency.

**[NIT]** `TestDoRequest_429RetryAfterHeader` and `TestDoRequest_RetryAfterDoesNotMutateBackoff` are gated behind `testing.Short()` but `TestDoRequest_429RetryAfterHTTPDate` (which waits ~2 seconds) is not. This test will slow down non-short test runs. Consider adding `testing.Short()` skip to `TestDoRequest_429RetryAfterHTTPDate` as well for consistency.
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
t.Fatal("expected error for 401")
}
if attempts != 1 {
t.Errorf("expected 1 attempt (no retry on 401), got %d", attempts)
}
}
func TestIsNotFound(t *testing.T) {
err := &APIError{StatusCode: 404, Body: "not found"}
Review

[NIT] TestDoRequest_429RetryAfterHeader waits 1 full second (Retry-After: 1) making it a slow test. The test comment acknowledges this, but consider using testing.Short() to skip it or documenting it as intentionally slow.

**[NIT]** TestDoRequest_429RetryAfterHeader waits 1 full second (Retry-After: 1) making it a slow test. The test comment acknowledges this, but consider using testing.Short() to skip it or documenting it as intentionally slow.
if !IsNotFound(err) {
t.Error("expected IsNotFound to return true for 404")
}
err2 := &APIError{StatusCode: 500, Body: "server error"}
if IsNotFound(err2) {
t.Error("expected IsNotFound to return false for 500")
}
}
func TestIsUnauthorized(t *testing.T) {
err := &APIError{StatusCode: 401, Body: "bad credentials"}
if !IsUnauthorized(err) {
t.Error("expected IsUnauthorized to return true for 401")
}
}
func TestDoRequest_429RetryAfterHeader(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts == 1 {
w.Header().Set("Retry-After", "1")
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
return
}
w.WriteHeader(200)
w.Write([]byte(`{"ok":true}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
// Use short backoff; Retry-After should override
c.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test")
elapsed := time.Since(start)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if string(body) != `{"ok":true}` {
t.Errorf("unexpected body: %s", body)
}
if attempts != 2 {
t.Errorf("expected 2 attempts, got %d", attempts)
}
// Retry-After: 1 means at least 1 second delay
if elapsed < 900*time.Millisecond {
t.Errorf("expected ~1s delay from Retry-After, got %v", elapsed)
}
}
+13
View File
@@ -0,0 +1,13 @@
package github_test
import (
"gitea.weiker.me/rodin/review-bot/github"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time interface conformance assertions.
// These verify github.Client satisfies vcs.PRReader and vcs.FileReader.
var (
_ vcs.PRReader = (*github.Client)(nil)
_ vcs.FileReader = (*github.Client)(nil)
)
+68
View File
@@ -0,0 +1,68 @@
package github
import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"net/url"
"strings"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// GetFileContent fetches a file from the default branch of a repo.
// Delegates to GetFileContentAtRef with an empty ref.
func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
Review

[NIT] GetFileContent simply delegates to GetFileContentAtRef. The doc comment says 'Delegates to GetFileContentAtRef with the provided ref' which is accurate but redundant given that the implementation is a one-liner. The vcs.FileReader interface likely has a different signature than vcs.PRReader's method — this delegation is the right approach, but the doc comment could explain why this wrapper exists (to satisfy the FileReader interface which has a different signature from GetFileContentAtRef).

**[NIT]** `GetFileContent` simply delegates to `GetFileContentAtRef`. The doc comment says 'Delegates to GetFileContentAtRef with the provided ref' which is accurate but redundant given that the implementation is a one-liner. The `vcs.FileReader` interface likely has a different signature than `vcs.PRReader`'s method — this delegation is the right approach, but the doc comment could explain *why* this wrapper exists (to satisfy the FileReader interface which has a different signature from GetFileContentAtRef).
return c.GetFileContentAtRef(ctx, owner, repo, path, ref)
Review

[MINOR] GetFileContent's signature includes a ref parameter but the doc comment says 'Delegates to GetFileContentAtRef with an empty ref', which contradicts the actual implementation (it passes ref through). The doc comment is wrong and should say 'Delegates to GetFileContentAtRef with the provided ref'.

**[MINOR]** GetFileContent's signature includes a `ref` parameter but the doc comment says 'Delegates to GetFileContentAtRef with an empty ref', which contradicts the actual implementation (it passes `ref` through). The doc comment is wrong and should say 'Delegates to GetFileContentAtRef with the provided ref'.
Review

[MINOR] GetFileContent is defined in files.go but it's a pure delegation to GetFileContentAtRef which lives in pr.go. Both implement the FileReader interface, but splitting the delegation from the implementation across files is mildly confusing. Both could live in files.go or the delegation could be removed and GetFileContentAtRef used directly. Minor organization issue.

**[MINOR]** GetFileContent is defined in files.go but it's a pure delegation to GetFileContentAtRef which lives in pr.go. Both implement the FileReader interface, but splitting the delegation from the implementation across files is mildly confusing. Both could live in files.go or the delegation could be removed and GetFileContentAtRef used directly. Minor organization issue.
}
Review

[NIT] GetFileContent doc says 'Delegates to GetFileContentAtRef with the provided ref' — this is accurate but the function adds no value over calling GetFileContentAtRef directly. This is presumably required to satisfy a FileReader interface. If that's the case, a comment noting it satisfies the interface would help readers understand why this thin wrapper exists.

**[NIT]** `GetFileContent` doc says 'Delegates to GetFileContentAtRef with the provided ref' — this is accurate but the function adds no value over calling `GetFileContentAtRef` directly. This is presumably required to satisfy a `FileReader` interface. If that's the case, a comment noting it satisfies the interface would help readers understand why this thin wrapper exists.
// ListContents lists files and directories at a given path in a repo.
// Returns the directory listing from the GitHub contents API.
func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]vcs.ContentEntry, error) {
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 []struct {
Name string `json:"name"`
Path string `json:"path"`
Type string `json:"type"`
}
if err := json.Unmarshal(body, &entries); err != nil {
return nil, fmt.Errorf("parse contents JSON: %w", err)
}
result := make([]vcs.ContentEntry, len(entries))
for i, e := range entries {
result[i] = vcs.ContentEntry{
Name: e.Name,
Path: e.Path,
Type: e.Type,
}
}
return result, nil
}
// 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, "/")
}
// decodeBase64Content decodes base64-encoded content from the GitHub contents API.
// GitHub returns base64 content with newlines for formatting, which we strip before decoding.
func decodeBase64Content(encoded string) (string, error) {
// GitHub inserts newlines in base64 content
cleaned := strings.ReplaceAll(encoded, "\n", "")
decoded, err := base64.StdEncoding.DecodeString(cleaned)
if err != nil {
Review

[NIT] The escapePath function removes dot-segments silently. The doc comment acknowledges this and explains it's intentional, which is good. But the test case {"../etc/passwd", "etc/passwd"} documents that a path traversal attempt is silently resolved to etc/passwd rather than returning an error. Depending on threat model, callers may want to know the path was modified. Since this is intentional and documented, this is a NIT-level observation for a future design consideration.

**[NIT]** The `escapePath` function removes dot-segments silently. The doc comment acknowledges this and explains it's intentional, which is good. But the test case `{"../etc/passwd", "etc/passwd"}` documents that a path traversal attempt is silently resolved to `etc/passwd` rather than returning an error. Depending on threat model, callers may want to know the path was modified. Since this is intentional and documented, this is a NIT-level observation for a future design consideration.
return "", err
}
return string(decoded), nil
}
+277
View File
@@ -0,0 +1,277 @@
package github
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"testing"
"time"
)
func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) {
var gotRef string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotRef = r.URL.Query().Get("ref")
json.NewEncoder(w).Encode(map[string]string{
"content": "dGVzdA==", // "test" in base64
"encoding": "base64",
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
// Call with empty ref — should not include ref param
content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if content != "test" {
t.Errorf("expected 'test', got %q", content)
}
if gotRef != "" {
t.Errorf("expected empty ref, got %q", gotRef)
}
}
func TestGetFileContent_WithRef(t *testing.T) {
var gotRef string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotRef = r.URL.Query().Get("ref")
json.NewEncoder(w).Encode(map[string]string{
"content": "dGVzdA==",
"encoding": "base64",
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "abc123")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotRef != "abc123" {
t.Errorf("expected ref 'abc123', got %q", gotRef)
}
}
func TestGetFileContent_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContent(context.Background(), "owner", "repo", "missing.go", "")
if err == nil {
t.Fatal("expected error for 404")
}
}
func TestGetFileContent_401(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(401)
w.Write([]byte(`{"message":"Bad credentials"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
if err == nil {
t.Fatal("expected error for 401")
}
}
func TestGetFileContent_429Retry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts == 1 {
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
return
}
json.NewEncoder(w).Encode(map[string]string{
"content": "b2s=",
"encoding": "base64",
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond}
content, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if content != "ok" {
t.Errorf("expected 'ok', got %q", content)
}
if attempts != 2 {
t.Errorf("expected 2 attempts, got %d", attempts)
}
}
func TestGetFileContent_MalformedJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`not json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "")
if err == nil {
t.Fatal("expected error for malformed JSON")
}
}
func TestListContents_HappyPath(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/repos/owner/repo/contents/src" {
t.Errorf("unexpected path: %s", r.URL.Path)
}
json.NewEncoder(w).Encode([]map[string]string{
{"name": "main.go", "path": "src/main.go", "type": "file"},
{"name": "lib", "path": "src/lib", "type": "dir"},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
entries, err := c.ListContents(context.Background(), "owner", "repo", "src")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(entries) != 2 {
t.Fatalf("expected 2 entries, got %d", len(entries))
}
if entries[0].Name != "main.go" {
t.Errorf("expected name 'main.go', got %q", entries[0].Name)
}
if entries[0].Path != "src/main.go" {
t.Errorf("expected path 'src/main.go', got %q", entries[0].Path)
}
if entries[0].Type != "file" {
t.Errorf("expected type 'file', got %q", entries[0].Type)
}
if entries[1].Name != "lib" {
t.Errorf("expected name 'lib', got %q", entries[1].Name)
}
if entries[1].Type != "dir" {
t.Errorf("expected type 'dir', got %q", entries[1].Type)
}
}
func TestListContents_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.ListContents(context.Background(), "owner", "repo", "missing")
if err == nil {
t.Fatal("expected error for 404")
}
}
func TestListContents_401(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(401)
w.Write([]byte(`{"message":"Bad credentials"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.ListContents(context.Background(), "owner", "repo", "src")
if err == nil {
t.Fatal("expected error for 401")
}
}
func TestListContents_429Retry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts == 1 {
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
return
}
json.NewEncoder(w).Encode([]map[string]string{
{"name": "file.go", "path": "file.go", "type": "file"},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond}
entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(entries) != 1 {
t.Fatalf("expected 1 entry, got %d", len(entries))
}
if attempts != 2 {
t.Errorf("expected 2 attempts, got %d", attempts)
}
}
func TestListContents_MalformedJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`not json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.ListContents(context.Background(), "owner", "repo", "src")
if err == nil {
t.Fatal("expected error for malformed JSON")
}
}
func TestDecodeBase64Content(t *testing.T) {
// Test with newlines (GitHub's format)
encoded := "cGFja2FnZSBt\nYWlu"
decoded, err := decodeBase64Content(encoded)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if decoded != "package main" {
t.Errorf("expected 'package main', got %q", decoded)
}
}
func TestDecodeBase64Content_Invalid(t *testing.T) {
_, err := decodeBase64Content("not!!!valid!!!base64")
if err == nil {
t.Fatal("expected error for invalid base64")
}
}
+233
View File
@@ -0,0 +1,233 @@
package github
import (
"context"
"encoding/json"
"fmt"
"net/http"
"net/url"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// pullRequestResponse is the GitHub API response for a pull request.
type pullRequestResponse struct {
Number int `json:"number"`
Title string `json:"title"`
Body string `json:"body"`
Head struct {
SHA string `json:"sha"`
Ref string `json:"ref"`
} `json:"head"`
Base struct {
Ref string `json:"ref"`
} `json:"base"`
}
// changedFileResponse is the GitHub API response for a changed file in a PR.
type changedFileResponse struct {
Filename string `json:"filename"`
Status string `json:"status"`
Patch string `json:"patch"`
}
// commitStatusResponse is the GitHub combined status API response.
type commitStatusResponse struct {
State string `json:"state"`
Statuses []struct {
Context string `json:"context"`
State string `json:"state"`
Description string `json:"description"`
TargetURL string `json:"target_url"`
} `json:"statuses"`
}
// checkRunsResponse is the GitHub check runs API response.
type checkRunsResponse struct {
TotalCount int `json:"total_count"`
CheckRuns []struct {
Name string `json:"name"`
Conclusion *string `json:"conclusion"`
Status string `json:"status"`
Review

[MINOR] The HTMLURL field name in checkRunsResponse deviates from the Acronym Capitalization pattern (style.md §2) which mandates all-caps acronyms: it should be HTMLURL for an unexported struct field following htmlURL convention, but since this is an unexported type used only for JSON unmarshaling, the real concern is cosmetic. However HTMLURL (two consecutive all-caps acronyms) is actually the correct form per Go conventions — this is not a real issue. Noting for completeness: the field name is correct.

**[MINOR]** The `HTMLURL` field name in `checkRunsResponse` deviates from the Acronym Capitalization pattern (style.md §2) which mandates all-caps acronyms: it should be `HTMLURL` for an unexported struct field following `htmlURL` convention, but since this is an unexported type used only for JSON unmarshaling, the real concern is cosmetic. However `HTMLURL` (two consecutive all-caps acronyms) is actually the correct form per Go conventions — this is not a real issue. Noting for completeness: the field name is correct.
HTMLURL string `json:"html_url"`
} `json:"check_runs"`
}
// GetPullRequest fetches PR metadata.
Review

[MINOR] The commitStatusResponse.State field is parsed from the API response but never used — only the individual Statuses slice entries are iterated. This is a minor dead field. Either use State to short-circuit when overall state is known, or remove it from the struct to avoid confusion about its purpose.

**[MINOR]** The `commitStatusResponse.State` field is parsed from the API response but never used — only the individual `Statuses` slice entries are iterated. This is a minor dead field. Either use `State` to short-circuit when overall state is known, or remove it from the struct to avoid confusion about its purpose.
func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcs.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)
}
Review

[MINOR] The checkRunsResponse.TotalCount field is parsed but never used. The pagination termination relies on len(checkResp.CheckRuns) < 100 rather than TotalCount. This is fine for correctness, but the unused field adds noise. Either use it or remove it.

**[MINOR]** The `checkRunsResponse.TotalCount` field is parsed but never used. The pagination termination relies on `len(checkResp.CheckRuns) < 100` rather than `TotalCount`. This is fine for correctness, but the unused field adds noise. Either use it or remove it.
var resp pullRequestResponse
if err := json.Unmarshal(body, &resp); err != nil {
return nil, fmt.Errorf("parse PR JSON: %w", err)
}
return &vcs.PullRequest{
Number: resp.Number,
Title: resp.Title,
Body: resp.Body,
Head: vcs.HeadRef{SHA: resp.Head.SHA, Ref: resp.Head.Ref},
Base: vcs.BaseRef{Ref: resp.Base.Ref},
}, nil
}
Review

[MINOR] The mapCheckRunStatus function signature takes a second _ string parameter that is explicitly ignored and documented as intentionally unused. Per the convention patterns, this is acceptable as documentation, but it would be cleaner to either remove the parameter entirely (if the function is only called internally) or keep it for forward-compatibility with a comment. Since it's unexported and only called in one place, removing the dead parameter would be cleaner.

**[MINOR]** The `mapCheckRunStatus` function signature takes a second `_ string` parameter that is explicitly ignored and documented as intentionally unused. Per the convention patterns, this is acceptable as documentation, but it would be cleaner to either remove the parameter entirely (if the function is only called internally) or keep it for forward-compatibility with a comment. Since it's unexported and only called in one place, removing the dead parameter would be cleaner.
// GetPullRequestDiff fetches the unified diff for a PR.
// Uses Accept: application/vnd.github.diff to get raw diff text.
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)
Review

[NIT] The maxPages = 100 constant limits to 10,000 files (100 pages × 100 per page). GitHub's API caps PRs at 3,000 changed files, so this bound is fine in practice. However, the constant is shared between GetPullRequestFiles and the check-runs pagination in GetCommitStatuses, which have different semantics. These could be separate constants with explanatory comments for clarity.

**[NIT]** The `maxPages = 100` constant limits to 10,000 files (100 pages × 100 per page). GitHub's API caps PRs at 3,000 changed files, so this bound is fine in practice. However, the constant is shared between `GetPullRequestFiles` and the check-runs pagination in `GetCommitStatuses`, which have different semantics. These could be separate constants with explanatory comments for clarity.
}
return string(body), nil
}
// GetPullRequestFiles fetches the list of files changed in a PR.
// Paginates through all pages (100 per page) to collect all files.
Review

[MINOR] The maxPages = 100 constant caps pagination at 100 pages × 100 files = 10,000 files for PRs, and 100 pages × 100 check runs = 10,000 check runs. This constant is shared between two very different concerns (PR files and check runs). A PR with 10,000 files is pathological but possible in generated-code repos; silently truncating without returning an error or warning could cause incorrect reviews. Consider either documenting this limit explicitly or returning an error when the cap is hit.

**[MINOR]** The `maxPages = 100` constant caps pagination at 100 pages × 100 files = 10,000 files for PRs, and 100 pages × 100 check runs = 10,000 check runs. This constant is shared between two very different concerns (PR files and check runs). A PR with 10,000 files is pathological but possible in generated-code repos; silently truncating without returning an error or warning could cause incorrect reviews. Consider either documenting this limit explicitly or returning an error when the cap is hit.
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
var allFiles []vcs.ChangedFile
page := 1
for {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return nil, fmt.Errorf("fetch PR files page %d: %w", page, err)
}
var files []changedFileResponse
if err := json.Unmarshal(body, &files); err != nil {
Review

[MINOR] The maxPages constant (100) is shared between GetPullRequestFiles and the check-runs pagination in GetCommitStatuses. A PR with exactly 10,000 files (100 pages × 100 per page) would silently return incomplete results with no error or warning. The GitHub API itself caps PRs at 3000 files, so in practice this is fine, but the silent truncation at the limit is a latent correctness issue. Consider logging a warning or returning an error when the page cap is hit.

**[MINOR]** The `maxPages` constant (100) is shared between `GetPullRequestFiles` and the check-runs pagination in `GetCommitStatuses`. A PR with exactly 10,000 files (100 pages × 100 per page) would silently return incomplete results with no error or warning. The GitHub API itself caps PRs at 3000 files, so in practice this is fine, but the silent truncation at the limit is a latent correctness issue. Consider logging a warning or returning an error when the page cap is hit.
return nil, fmt.Errorf("parse PR files JSON: %w", err)
}
Review

[MINOR] GetPullRequestFiles pagination relies on len(files) < 100 to stop. This works but can be brittle. For maximum correctness, consider using the Link response header to determine if a next page exists.

**[MINOR]** GetPullRequestFiles pagination relies on len(files) < 100 to stop. This works but can be brittle. For maximum correctness, consider using the Link response header to determine if a next page exists.
if len(files) == 0 {
break
}
for _, f := range files {
allFiles = append(allFiles, vcs.ChangedFile{
Filename: f.Filename,
Review

[MINOR] The mapCheckRunStatus function signature takes a second parameter _ string that is explicitly discarded. Since Go allows unused blank-identifier parameters, this is idiomatic, but the function could alternatively just take conclusion *string directly. The current form is acceptable but the doc comment explaining why the second param is ignored is good practice.

**[MINOR]** The `mapCheckRunStatus` function signature takes a second parameter `_ string` that is explicitly discarded. Since Go allows unused blank-identifier parameters, this is idiomatic, but the function could alternatively just take `conclusion *string` directly. The current form is acceptable but the doc comment explaining why the second param is ignored is good practice.
Status: f.Status,
Patch: f.Patch,
})
}
if len(files) < 100 {
break
}
page++
}
return allFiles, nil
}
// GetFileContentAtRef fetches a file at a specific ref from a repo.
// If ref is empty, the query parameter is omitted (uses default branch).
func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref string) (string, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
if ref != "" {
reqURL += "?ref=" + url.QueryEscape(ref)
}
body, err := c.doGet(ctx, reqURL)
Review

[NIT] The GetPullRequestFiles comment says 'Returns nil (not an empty slice) when the PR has no changed files' but if the first page returns an empty array, allFiles remains nil — this is correct. However the GitHub API actually returns an empty array for PRs with 0 changed files (valid edge case), so the nil vs empty distinction in the doc comment is accurate but subtle. No code change needed.

**[NIT]** The `GetPullRequestFiles` comment says 'Returns nil (not an empty slice) when the PR has no changed files' but if the first page returns an empty array, `allFiles` remains nil — this is correct. However the GitHub API actually returns an empty array for PRs with 0 changed files (valid edge case), so the nil vs empty distinction in the doc comment is accurate but subtle. No code change needed.
if err != nil {
return "", fmt.Errorf("fetch file %s: %w", path, err)
}
var resp struct {
Content string `json:"content"`
Encoding string `json:"encoding"`
}
if err := json.Unmarshal(body, &resp); err != nil {
return "", fmt.Errorf("parse file content JSON: %w", err)
Review

[MINOR] GetCommitStatuses check-runs pagination uses the same <100 sentinel to stop, which could loop unbounded if a server always returns 100 items. Consider using Link headers, total_count, or a maximum page cap as a safety limit.

**[MINOR]** GetCommitStatuses check-runs pagination uses the same <100 sentinel to stop, which could loop unbounded if a server always returns 100 items. Consider using Link headers, total_count, or a maximum page cap as a safety limit.
}
if resp.Encoding != "base64" {
return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path)
}
Review

[MINOR] The GetPullRequestFiles pagination loop does not return an empty non-nil slice when there are no files (the first page returns 0 items), it returns nil. This is idiomatic Go for slices but may surprise callers who do len(files) == 0 vs a nil check. Not a bug, just worth documenting or ensuring callers handle nil consistently.

**[MINOR]** The `GetPullRequestFiles` pagination loop does not return an empty non-nil slice when there are no files (the first page returns 0 items), it returns `nil`. This is idiomatic Go for slices but may surprise callers who do `len(files) == 0` vs a nil check. Not a bug, just worth documenting or ensuring callers handle nil consistently.
decoded, err := decodeBase64Content(resp.Content)
if err != nil {
return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
Review

[NIT] mapCheckRunStatus accepts a second parameter _ string that is intentionally unused. The function signature is slightly misleading for callers. Since this is unexported and only called from one place, simplifying to mapCheckRunStatus(conclusion *string) string would be cleaner. The current approach was likely chosen to match the struct fields but the unused parameter adds noise.

**[NIT]** `mapCheckRunStatus` accepts a second parameter `_ string` that is intentionally unused. The function signature is slightly misleading for callers. Since this is unexported and only called from one place, simplifying to `mapCheckRunStatus(conclusion *string) string` would be cleaner. The current approach was likely chosen to match the struct fields but the unused parameter adds noise.
Review

[MINOR] The GetPullRequestFiles function is documented to return nil (not empty slice) when there are no changed files, which is correct for nil-safe ranging. However, GetCommitStatuses similarly returns a nil result slice when there are no statuses (via var result []vcs.CommitStatus plus no appends), but this is not documented in the function comment. Minor documentation inconsistency.

**[MINOR]** The `GetPullRequestFiles` function is documented to return `nil` (not empty slice) when there are no changed files, which is correct for nil-safe ranging. However, `GetCommitStatuses` similarly returns a `nil` result slice when there are no statuses (via `var result []vcs.CommitStatus` plus no appends), but this is not documented in the function comment. Minor documentation inconsistency.
}
return decoded, nil
}
// GetCommitStatuses fetches both commit statuses and check runs for a SHA,
// merging them into a unified []vcs.CommitStatus slice.
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
var result []vcs.CommitStatus
Review

[NIT] The mapCheckRunStatus function signature accepts _ string (unused status field) and explains this in the comment. This is fine for now, but the stale and waiting conclusions are mapped to pendingwaiting is not a GitHub API conclusion value (it's an internal concept). This won't cause bugs but may be dead code.

**[NIT]** The `mapCheckRunStatus` function signature accepts `_ string` (unused status field) and explains this in the comment. This is fine for now, but the `stale` and `waiting` conclusions are mapped to `pending` — `waiting` is not a GitHub API conclusion value (it's an internal concept). This won't cause bugs but may be dead code.
// Fetch commit statuses
statusURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/status",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha))
statusBody, err := c.doGet(ctx, statusURL)
if err != nil {
return nil, fmt.Errorf("fetch commit statuses: %w", err)
}
var statusResp commitStatusResponse
if err := json.Unmarshal(statusBody, &statusResp); err != nil {
return nil, fmt.Errorf("parse commit statuses JSON: %w", err)
}
for _, s := range statusResp.Statuses {
result = append(result, vcs.CommitStatus{
Context: s.Context,
Review

[MINOR] In GetCommitStatuses, when the check-runs endpoint fails, the function returns nil, err but has already accumulated commit statuses in result. The doc comment says 'If the commit statuses endpoint fails... the function returns immediately without attempting the check-runs endpoint' but doesn't document what happens when the check-runs endpoint fails after statuses succeed. This is a partial-result scenario that callers cannot distinguish from a total failure. Consider either returning the partial result or documenting this behavior explicitly.

**[MINOR]** In `GetCommitStatuses`, when the check-runs endpoint fails, the function returns `nil, err` but has already accumulated commit statuses in `result`. The doc comment says 'If the commit statuses endpoint fails... the function returns immediately without attempting the check-runs endpoint' but doesn't document what happens when the check-runs endpoint fails after statuses succeed. This is a partial-result scenario that callers cannot distinguish from a total failure. Consider either returning the partial result or documenting this behavior explicitly.
Status: s.State,
Description: s.Description,
TargetURL: s.TargetURL,
})
Review

[NIT] The derefString helper is a small utility that could theoretically live in a shared internal utilities package if it's needed elsewhere. For now it's fine in pr.go but if the codebase grows, consider an internal/ package. Not a current issue.

**[NIT]** The `derefString` helper is a small utility that could theoretically live in a shared internal utilities package if it's needed elsewhere. For now it's fine in `pr.go` but if the codebase grows, consider an `internal/` package. Not a current issue.
}
Review

[NIT] mapCheckRunStatus takes a second parameter (status) that is unused. Consider removing it or documenting/using it to avoid confusion for readers.

**[NIT]** mapCheckRunStatus takes a second parameter (status) that is unused. Consider removing it or documenting/using it to avoid confusion for readers.
// Fetch check runs (paginated)
checkPage := 1
for {
checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage)
checkBody, err := c.doGet(ctx, checkURL)
if err != nil {
return nil, fmt.Errorf("fetch check runs page %d: %w", checkPage, err)
}
var checkResp checkRunsResponse
if err := json.Unmarshal(checkBody, &checkResp); err != nil {
return nil, fmt.Errorf("parse check runs JSON: %w", err)
}
Review

[NIT] The mapCheckRunStatus function accepts a second parameter _ string (the status field) but ignores it. The function signature signature documents intent to use status but doesn't. In practice, status (e.g., 'in_progress', 'queued') could provide additional fidelity when conclusion is nil. The ignored parameter adds dead weight to the function signature. Either use it or remove it.

**[NIT]** The `mapCheckRunStatus` function accepts a second parameter `_ string` (the status field) but ignores it. The function signature signature documents intent to use `status` but doesn't. In practice, `status` (e.g., 'in_progress', 'queued') could provide additional fidelity when `conclusion` is nil. The ignored parameter adds dead weight to the function signature. Either use it or remove it.
for _, cr := range checkResp.CheckRuns {
result = append(result, vcs.CommitStatus{
Review

[NIT] In GetCommitStatuses, if the commit statuses endpoint returns an error (e.g., 404), the function returns immediately without attempting the check-runs endpoint. Depending on the intended semantics, callers may expect partial results or a combined error. The current behavior (fail fast) is reasonable but undocumented.

**[NIT]** In `GetCommitStatuses`, if the commit statuses endpoint returns an error (e.g., 404), the function returns immediately without attempting the check-runs endpoint. Depending on the intended semantics, callers may expect partial results or a combined error. The current behavior (fail fast) is reasonable but undocumented.
Context: cr.Name,
Status: mapCheckRunStatus(cr.Conclusion, cr.Status),
Description: derefString(cr.Conclusion),
TargetURL: cr.HTMLURL,
Review

[MINOR] The mapCheckRunStatus mapping treats cancelled, skipped, and neutral as "success" (non-blocking). This is a policy decision that should be documented more prominently — cancelled in particular could reasonably be mapped to failure in some contexts. The comment // non-blocking is brief; a note explaining the design rationale (e.g. 'these do not indicate a blocking failure per GitHub's check suite semantics') would help future maintainers.

**[MINOR]** The `mapCheckRunStatus` mapping treats `cancelled`, `skipped`, and `neutral` as `"success"` (non-blocking). This is a policy decision that should be documented more prominently — `cancelled` in particular could reasonably be mapped to `failure` in some contexts. The comment `// non-blocking` is brief; a note explaining the design rationale (e.g. 'these do not indicate a blocking failure per GitHub's check suite semantics') would help future maintainers.
})
}
if len(checkResp.CheckRuns) < 100 {
break
}
checkPage++
}
return result, nil
}
Review

[NIT] In GetCommitStatuses, the check run pagination loop uses checkPage as the variable name while the PR files loop uses page. Naming is internally consistent within each function, but the inconsistency between the two sibling pagination loops is a minor style nit.

**[NIT]** In `GetCommitStatuses`, the check run pagination loop uses `checkPage` as the variable name while the PR files loop uses `page`. Naming is internally consistent within each function, but the inconsistency between the two sibling pagination loops is a minor style nit.
// mapCheckRunStatus maps a check run conclusion+status to a vcs.CommitStatus status string.
func mapCheckRunStatus(conclusion *string, status string) string {
if conclusion == nil {
// Still running or queued
return "pending"
}
switch *conclusion {
case "success":
return "success"
case "failure", "action_required", "timed_out":
return "failure"
case "cancelled", "skipped", "neutral":
return "success" // non-blocking
case "in_progress", "queued":
return "pending"
default:
return "pending"
}
}
// derefString safely dereferences a string pointer, returning empty string if nil.
func derefString(s *string) string {
if s == nil {
return ""
}
return *s
}
+637
View File
@@ -0,0 +1,637 @@
package github
import (
"context"
"encoding/json"
"fmt"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
)
func TestGetPullRequest_HappyPath(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)
}
json.NewEncoder(w).Encode(map[string]interface{}{
"number": 42,
"title": "Test PR",
"body": "Description",
"head": map[string]string{"sha": "abc123", "ref": "feature-branch"},
"base": map[string]string{"ref": "main"},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 42)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if pr.Number != 42 {
t.Errorf("expected number 42, got %d", pr.Number)
}
if pr.Title != "Test PR" {
t.Errorf("expected title 'Test PR', got %q", pr.Title)
}
if pr.Body != "Description" {
t.Errorf("expected body 'Description', got %q", pr.Body)
}
if pr.Head.SHA != "abc123" {
t.Errorf("expected head SHA 'abc123', got %q", pr.Head.SHA)
}
if pr.Head.Ref != "feature-branch" {
t.Errorf("expected head ref 'feature-branch', got %q", pr.Head.Ref)
}
if pr.Base.Ref != "main" {
t.Errorf("expected base ref 'main', got %q", pr.Base.Ref)
}
}
func TestGetPullRequest_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 999)
if err == nil {
t.Fatal("expected error for 404")
}
if !IsNotFound(err) {
t.Errorf("expected IsNotFound=true, got error: %v", err)
}
}
func TestGetPullRequest_401(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(401)
w.Write([]byte(`{"message":"Bad credentials"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
if err == nil {
t.Fatal("expected error for 401")
}
if !IsUnauthorized(err) {
t.Errorf("expected IsUnauthorized=true, got error: %v", err)
}
}
func TestGetPullRequest_429Retry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts == 1 {
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
return
}
json.NewEncoder(w).Encode(map[string]interface{}{
"number": 1,
"title": "PR",
"body": "",
"head": map[string]string{"sha": "abc", "ref": "br"},
"base": map[string]string{"ref": "main"},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond}
pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if pr.Number != 1 {
t.Errorf("expected number 1, got %d", pr.Number)
}
if attempts != 2 {
t.Errorf("expected 2 attempts, got %d", attempts)
}
}
func TestGetPullRequest_MalformedJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
Review

[NIT] The table-driven TestGetCommitStatuses_CheckRunConclusions test creates a new httptest.Server for each subtest case rather than parameterizing the handler. This works but is slightly heavier than necessary. A single server whose handler reads from tt via closure capture (the subtests are sequential since t.Run without t.Parallel() runs sequentially) would be cleaner and follow the table-driven pattern more closely. Minor style concern.

**[NIT]** The table-driven `TestGetCommitStatuses_CheckRunConclusions` test creates a new `httptest.Server` for each subtest case rather than parameterizing the handler. This works but is slightly heavier than necessary. A single server whose handler reads from `tt` via closure capture (the subtests are sequential since `t.Run` without `t.Parallel()` runs sequentially) would be cleaner and follow the table-driven pattern more closely. Minor style concern.
w.WriteHeader(200)
w.Write([]byte(`{invalid json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
if err == nil {
t.Fatal("expected error for malformed JSON")
}
if !strings.Contains(err.Error(), "parse PR JSON") {
t.Errorf("expected parse error, got: %v", err)
}
}
func TestGetPullRequestDiff_HappyPath(t *testing.T) {
expectedDiff := "diff --git a/file.go b/file.go\n--- a/file.go\n+++ b/file.go\n@@ -1,3 +1,4 @@\n+// new line\n"
var gotAccept string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
gotAccept = r.Header.Get("Accept")
w.WriteHeader(200)
w.Write([]byte(expectedDiff))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
diff, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 42)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if diff != expectedDiff {
t.Errorf("unexpected diff: %q", diff)
}
if gotAccept != "application/vnd.github.diff" {
t.Errorf("expected diff Accept header, got %q", gotAccept)
}
}
func TestGetPullRequestDiff_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 999)
if err == nil {
t.Fatal("expected error for 404")
}
}
func TestGetPullRequestDiff_401(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(401)
w.Write([]byte(`{"message":"Bad credentials"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
if err == nil {
t.Fatal("expected error for 401")
}
}
func TestGetPullRequestFiles_HappyPath(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewEncoder(w).Encode([]map[string]interface{}{
{"filename": "main.go", "status": "modified", "patch": "@@ -1,3 +1,4 @@\n+line"},
{"filename": "test.go", "status": "added", "patch": "@@ -0,0 +1,5 @@\n+new file"},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(files) != 2 {
t.Fatalf("expected 2 files, got %d", len(files))
}
if files[0].Filename != "main.go" {
t.Errorf("expected filename 'main.go', got %q", files[0].Filename)
}
if files[0].Status != "modified" {
t.Errorf("expected status 'modified', got %q", files[0].Status)
}
if files[0].Patch != "@@ -1,3 +1,4 @@\n+line" {
t.Errorf("unexpected patch: %q", files[0].Patch)
}
}
func TestGetPullRequestFiles_Pagination(t *testing.T) {
// Simulate > 100 files requiring pagination
page1Files := make([]map[string]string, 100)
for i := 0; i < 100; i++ {
page1Files[i] = map[string]string{
"filename": fmt.Sprintf("file%d.go", i),
"status": "modified",
"patch": fmt.Sprintf("patch%d", i),
}
}
page2Files := []map[string]string{
{"filename": "file100.go", "status": "added", "patch": "patch100"},
}
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
page := r.URL.Query().Get("page")
if page == "" || page == "1" {
json.NewEncoder(w).Encode(page1Files)
} else {
json.NewEncoder(w).Encode(page2Files)
}
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(files) != 101 {
t.Errorf("expected 101 files (paginated), got %d", len(files))
}
if files[100].Filename != "file100.go" {
t.Errorf("expected last file 'file100.go', got %q", files[100].Filename)
}
if files[100].Patch != "patch100" {
t.Errorf("expected last patch 'patch100', got %q", files[100].Patch)
}
}
func TestGetPullRequestFiles_BinaryFile_NoPatch(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Binary files have no patch field in GitHub response
json.NewEncoder(w).Encode([]map[string]interface{}{
{"filename": "image.png", "status": "added"},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(files) != 1 {
t.Fatalf("expected 1 file, got %d", len(files))
}
if files[0].Patch != "" {
t.Errorf("expected empty patch for binary file, got %q", files[0].Patch)
}
}
func TestGetPullRequestFiles_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 999)
if err == nil {
t.Fatal("expected error for 404")
}
}
func TestGetPullRequestFiles_MalformedJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`not json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
if err == nil {
t.Fatal("expected error for malformed JSON")
}
}
func TestGetFileContentAtRef_HappyPath(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/repos/owner/repo/contents/path/to/file.go" {
t.Errorf("unexpected path: %s", r.URL.Path)
}
if r.URL.Query().Get("ref") != "abc123" {
t.Errorf("unexpected ref: %s", r.URL.Query().Get("ref"))
}
json.NewEncoder(w).Encode(map[string]string{
"content": "cGFja2FnZSBtYWlu", // "package main" in base64
"encoding": "base64",
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "path/to/file.go", "abc123")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if content != "package main" {
t.Errorf("expected 'package main', got %q", content)
}
}
func TestGetFileContentAtRef_EmptyRef(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Query().Get("ref") != "" {
t.Errorf("expected no ref param, got %q", r.URL.Query().Get("ref"))
}
json.NewEncoder(w).Encode(map[string]string{
"content": "aGVsbG8=", // "hello" in base64
"encoding": "base64",
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.txt", "")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if content != "hello" {
t.Errorf("expected 'hello', got %q", content)
}
}
func TestGetFileContentAtRef_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "missing.go", "main")
if err == nil {
t.Fatal("expected error for 404")
}
}
func TestGetFileContentAtRef_401(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(401)
w.Write([]byte(`{"message":"Bad credentials"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
if err == nil {
t.Fatal("expected error for 401")
}
}
func TestGetFileContentAtRef_MalformedJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`not valid json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
if err == nil {
t.Fatal("expected error for malformed JSON")
}
}
Review

[NIT] In TestGetCommitStatuses_CheckRunConclusions, each table entry creates a new httptest server inside a subtest. This is correct but could use t.Cleanup(srv.Close) instead of defer srv.Close() per the testing patterns (defer runs at function end, t.Cleanup runs at subtest end). In practice, defer works correctly here since the server is created inside the t.Run closure function body, but using t.Cleanup is more idiomatic per the documented patterns.

**[NIT]** In `TestGetCommitStatuses_CheckRunConclusions`, each table entry creates a new httptest server inside a subtest. This is correct but could use `t.Cleanup(srv.Close)` instead of `defer srv.Close()` per the testing patterns (defer runs at function end, t.Cleanup runs at subtest end). In practice, `defer` works correctly here since the server is created inside the `t.Run` closure function body, but using `t.Cleanup` is more idiomatic per the documented patterns.
func TestGetFileContentAtRef_429Retry(t *testing.T) {
attempts := 0
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
attempts++
if attempts == 1 {
w.WriteHeader(429)
w.Write([]byte(`{"message":"rate limit"}`))
return
}
json.NewEncoder(w).Encode(map[string]string{
"content": "b2s=", // "ok" in base64
"encoding": "base64",
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
c.RetryBackoff = []time.Duration{1 * time.Millisecond}
content, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if content != "ok" {
t.Errorf("expected 'ok', got %q", content)
}
if attempts != 2 {
t.Errorf("expected 2 attempts, got %d", attempts)
}
}
func TestGetCommitStatuses_HappyPath(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch {
case strings.Contains(r.URL.Path, "/status"):
json.NewEncoder(w).Encode(map[string]interface{}{
"state": "success",
"statuses": []map[string]string{
{
"context": "ci/build",
"state": "success",
"description": "Build passed",
"target_url": "https://ci.example.com/1",
},
},
})
case strings.Contains(r.URL.Path, "/check-runs"):
conclusion := "success"
json.NewEncoder(w).Encode(map[string]interface{}{
"total_count": 1,
"check_runs": []map[string]interface{}{
{
"name": "lint",
"conclusion": &conclusion,
"status": "completed",
"html_url": "https://github.com/check/1",
},
},
})
default:
t.Errorf("unexpected path: %s", r.URL.Path)
w.WriteHeader(404)
}
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(statuses) != 2 {
t.Fatalf("expected 2 statuses, got %d", len(statuses))
}
// First should be from commit statuses
if statuses[0].Context != "ci/build" {
t.Errorf("expected context 'ci/build', got %q", statuses[0].Context)
}
if statuses[0].Status != "success" {
t.Errorf("expected status 'success', got %q", statuses[0].Status)
}
// Second should be from check runs
if statuses[1].Context != "lint" {
t.Errorf("expected context 'lint', got %q", statuses[1].Context)
}
if statuses[1].Status != "success" {
t.Errorf("expected status 'success', got %q", statuses[1].Status)
}
}
func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
tests := []struct {
conclusion *string
status string
want string
}{
{strPtr("success"), "completed", "success"},
{strPtr("failure"), "completed", "failure"},
{strPtr("action_required"), "completed", "failure"},
{strPtr("timed_out"), "completed", "failure"},
{strPtr("cancelled"), "completed", "success"},
{strPtr("skipped"), "completed", "success"},
{strPtr("neutral"), "completed", "success"},
{nil, "in_progress", "pending"},
{nil, "queued", "pending"},
}
for _, tt := range tests {
name := "nil"
if tt.conclusion != nil {
name = *tt.conclusion
}
t.Run(name, func(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.Contains(r.URL.Path, "/status") {
json.NewEncoder(w).Encode(map[string]interface{}{
"state": "success",
"statuses": []interface{}{},
})
return
}
json.NewEncoder(w).Encode(map[string]interface{}{
"total_count": 1,
"check_runs": []map[string]interface{}{
{
"name": "check",
"conclusion": tt.conclusion,
"status": tt.status,
"html_url": "https://github.com/check/1",
},
},
})
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha1")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(statuses) != 1 {
t.Fatalf("expected 1 status, got %d", len(statuses))
}
if statuses[0].Status != tt.want {
t.Errorf("expected status %q, got %q", tt.want, statuses[0].Status)
}
})
}
}
func TestGetCommitStatuses_404(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(404)
w.Write([]byte(`{"message":"Not Found"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "badsha")
if err == nil {
t.Fatal("expected error for 404")
}
}
func TestGetCommitStatuses_401(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(401)
w.Write([]byte(`{"message":"Bad credentials"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
if err == nil {
t.Fatal("expected error for 401")
}
}
func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`not json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL)
c.SetHTTPClient(srv.Client())
Review

[NIT] strPtr helper is defined in pr_test.go but the same helper could be needed in other test files. Since all files are in package github (internal tests), this is fine as-is but worth noting for future maintainability.

**[NIT]** strPtr helper is defined in pr_test.go but the same helper could be needed in other test files. Since all files are in package `github` (internal tests), this is fine as-is but worth noting for future maintainability.
Review

[NIT] stringPtr helper is defined in pr_test.go (package github) but TestGetCommitStatuses_CheckRunConclusions also uses it. Since tests are in the same package, this is fine, but the helper could be placed in a shared test helper file to avoid potential future duplication if more test files are added to the package.

**[NIT]** `stringPtr` helper is defined in `pr_test.go` (package `github`) but `TestGetCommitStatuses_CheckRunConclusions` also uses it. Since tests are in the same package, this is fine, but the helper could be placed in a shared test helper file to avoid potential future duplication if more test files are added to the package.
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
if err == nil {
t.Fatal("expected error for malformed JSON")
Review

[NIT] strPtr is defined in pr_test.go (package github). The same helper (or a very similar one) might be useful in other test files. Consider moving it to a shared test helper file (e.g., helpers_test.go) within the package to avoid potential duplication if other test files need it.

**[NIT]** `strPtr` is defined in `pr_test.go` (package `github`). The same helper (or a very similar one) might be useful in other test files. Consider moving it to a shared test helper file (e.g., `helpers_test.go`) within the package to avoid potential duplication if other test files need it.
}
Review

[NIT] strPtr helper is defined in pr_test.go but files_test.go is in the same package and could theoretically need it. Both are in package github (white-box tests), so there's no duplication issue here. However, there is a duplicate strPtr in neither file — only pr_test.go has it. Fine as-is.

**[NIT]** strPtr helper is defined in pr_test.go but files_test.go is in the same package and could theoretically need it. Both are in `package github` (white-box tests), so there's no duplication issue here. However, there is a duplicate strPtr in neither file — only pr_test.go has it. Fine as-is.
Review

[NIT] strPtr is defined in pr_test.go but files_test.go and client_test.go are in the same package. If any other test file ever needs this helper, it will collide. It's already fine since both are in package github, but naming it something more descriptive (e.g., stringPtr) would match the codebase's convention of meaningful names over abbreviations per the style conventions.

**[NIT]** `strPtr` is defined in `pr_test.go` but `files_test.go` and `client_test.go` are in the same package. If any other test file ever needs this helper, it will collide. It's already fine since both are in `package github`, but naming it something more descriptive (e.g., `stringPtr`) would match the codebase's convention of meaningful names over abbreviations per the style conventions.
Review

[NIT] stringPtr helper is defined in pr_test.go. If future tests in files_test.go or client_test.go need similar helpers, there could be duplication. Consider moving to a shared test helper file, though for now it's fine since it's only used in pr_test.go.

**[NIT]** `stringPtr` helper is defined in `pr_test.go`. If future tests in `files_test.go` or `client_test.go` need similar helpers, there could be duplication. Consider moving to a shared test helper file, though for now it's fine since it's only used in `pr_test.go`.
}
Review

[NIT] strPtr helper is defined in pr_test.go but files_test.go is in the same package (package github). If files_test.go ever needs this helper, there would be a duplicate definition. Consider putting shared test helpers in a helpers_test.go file. Not a current problem since files_test.go doesn't use it, but worth considering for consistency.

**[NIT]** `strPtr` helper is defined in `pr_test.go` but `files_test.go` is in the same package (`package github`). If `files_test.go` ever needs this helper, there would be a duplicate definition. Consider putting shared test helpers in a `helpers_test.go` file. Not a current problem since `files_test.go` doesn't use it, but worth considering for consistency.
func strPtr(s string) *string {
return &s
}
+1
View File
@@ -44,6 +44,7 @@ type PullRequest struct {
type ChangedFile struct {
Filename string `json:"filename"`
Status string `json:"status"`
Patch string `json:"patch"`
}
// ContentEntry represents a file or directory entry from the contents API.