fix(github): address review findings from round 2880/2883

Sonnet MINOR #1: Stop timer after <-timer.C fires for idiomatic cleanup.
Sonnet MINOR #2: Document that empty array from contents API is valid (empty dir).
Sonnet MINOR #3: Document that GetPullRequestFiles returns nil for no files.
Sonnet NIT #4: Strengthen SetHTTPClient/SetRetryBackoff docs to clarify test-only intent.
Sonnet NIT #5: Document GetCommitStatuses fail-fast behavior.
Sonnet NIT #6: Document double-slash collapsing in escapePath.
Security MINOR #1: Document redirect policy responsibility when providing custom client.
Security MINOR #2: Reduce maxErrorBodyBytes from 64KB to 4KB to limit sensitive data exposure.

github/client.go

@@ -27,9 +27,10 @@ const (
 // It carries the status code so callers can distinguish between
 // different failure modes (e.g. 404 vs 500).
 //
-// Note: Error() includes up to 200 bytes of the response body for debugging.
+// The Body field stores up to 4 KiB of the raw response for programmatic
-// Callers should avoid logging raw error messages in production if the upstream
+// inspection. Error() truncates to 200 bytes for safe logging, but callers
-// server may return sensitive details in error responses.
+// should avoid logging or propagating Body directly in production since it may
+// contain sensitive details from the upstream server.
 type APIError struct {
 	StatusCode int
 	Body       string
@@ -87,8 +88,9 @@ func AllowInsecureHTTP() ClientOption {
 }
 
 // Client interacts with the GitHub API.
-// A Client is safe for concurrent use by multiple goroutines;
+// A Client is safe for concurrent use by multiple goroutines.
-// however, SetHTTPClient and SetRetryBackoff must not be called concurrently with requests.
+// SetHTTPClient and SetRetryBackoff are intended for test setup only and must
+// be called before any goroutines issue requests; they have no synchronization.
 type Client struct {
 	baseURL           string
 	token             string
@@ -141,9 +143,15 @@ func NewClient(token, baseURL string, opts ...ClientOption) *Client {
 }
 
 // SetHTTPClient sets the underlying HTTP client used for requests.
-// This is intended for testing to inject mock transports.
+// This is intended for test setup only to inject mock transports; it must be
+// called before any goroutines issue requests.
+//
 // Passing nil restores the default client (30s timeout + auth-stripping
 // CheckRedirect policy matching NewClient).
+//
+// Callers providing a non-nil client are responsible for configuring a safe
+// CheckRedirect policy. Without one, the default net/http behavior will follow
+// redirects and may forward the Authorization header to untrusted hosts.
 func (c *Client) SetHTTPClient(hc *http.Client) {
 	if hc == nil {
 		hc = &http.Client{
@@ -155,6 +163,7 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
 }
 
 // SetRetryBackoff configures the retry backoff durations for testing.
+// It must be called before any goroutines issue requests.
 // In production the default {1s, 2s} applies.
 func (c *Client) SetRetryBackoff(d []time.Duration) {
 	c.retryBackoff = d
@@ -175,7 +184,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
 	}
 
-	const maxErrorBodyBytes = 64 * 1024
+	// maxErrorBodyBytes limits how much of an error response body is stored.
+	// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
+	// log APIError.Body directly. Error() further truncates to 200 bytes.
+	const maxErrorBodyBytes = 4 * 1024
 
 	// Reject non-HTTPS URLs early since the URL is immutable across retries.
 	if c.token != "" && !c.allowInsecureHTTP {
@@ -199,7 +211,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 				timer := time.NewTimer(delay)
 				select {
 				case <-timer.C:
-					// Backoff elapsed, proceed with retry.
+					timer.Stop() // no-op after fire, releases runtime resources promptly
 				case <-ctx.Done():
 					timer.Stop()
 					return nil, ctx.Err()

github/files.go

@@ -42,6 +42,8 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 
 	// The GitHub contents API returns an array for directories and an object
 	// for single files. Try array first (common case), then fall back to object.
+	// An empty array ([]) is valid — it represents an empty directory — and
+	// results in a zero-length slice returned without error.
 	var entries []entry
 	if err := json.Unmarshal(body, &entries); err != nil {
 		var single entry
@@ -69,11 +71,12 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 
 // escapePath escapes each segment of a relative file path for use in URLs.
 // Slashes are preserved as path separators; other special characters are escaped.
-// Dot-segments ("." and "..") are silently removed to prevent path traversal.
+// Dot-segments ("." and "..") and empty segments (from consecutive slashes like
-// This is intentional: callers may receive a different path than requested without
+// "a//b") are silently removed to prevent path traversal and produce canonical
-// error. The function is package-private, and all callers (GetFileContentAtRef,
+// paths. This is intentional: callers may receive a different path than requested
-// ListContents) already handle missing-file errors from the API if the cleaned
+// without error. The function is package-private, and all callers
-// path doesn't match what the caller intended.
+// (GetFileContentAtRef, ListContents) already handle missing-file errors from the
+// API if the cleaned path doesn't match what the caller intended.
 func escapePath(p string) string {
 	parts := strings.Split(p, "/")
 	var clean []string

github/pr.go

@@ -89,6 +89,8 @@ const maxPages = 100
 
 // GetPullRequestFiles fetches the list of files changed in a PR.
 // Paginates through all pages (100 per page) to collect all files.
+// Returns nil (not an empty slice) when the PR has no changed files.
+// Callers can safely range over or check len() on a nil slice.
 func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
 	var allFiles []vcs.ChangedFile
 
@@ -156,6 +158,8 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, path, ref
 
 // GetCommitStatuses fetches both commit statuses and check runs for a SHA,
 // merging them into a unified []vcs.CommitStatus slice.
+// If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
+// function returns immediately without attempting the check-runs endpoint.
 func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
 	var result []vcs.CommitStatus