From 6e8e7448169e639b46cf06f7694011f2ffafb485 Mon Sep 17 00:00:00 2001
From: claw <claw@weiker.me>
Date: Tue, 12 May 2026 19:40:30 -0700
Subject: [PATCH] fix(github): address self-review findings from 1194bc75

- Handle io.ReadAll error on error body read (client.go:265)
- Remove unused State field from commitStatusResponse (pr.go)
- Guard via slice access in defaultCheckRedirect (client.go:117)
- Move GetFileContentAtRef from pr.go to files.go (logical home)
---
 github/client.go | 10 +++++++++-
 github/files.go  | 33 +++++++++++++++++++++++++++++++++
 github/pr.go     | 34 ----------------------------------
 3 files changed, 42 insertions(+), 35 deletions(-)

diff --git a/github/client.go b/github/client.go
index eedfdc7..64976fd 100644
--- a/github/client.go
+++ b/github/client.go
@@ -114,6 +114,11 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
 	if len(via) >= 10 {
 		return fmt.Errorf("stopped after 10 redirects")
 	}
+	// Guard: net/http guarantees len(via) >= 1 but this is undocumented;
+	// defend against zero-length to avoid panic on index out of range.
+	if len(via) == 0 {
+		return nil
+	}
 	prev := via[len(via)-1]
 	// Reject protocol downgrade: HTTPS→HTTP leaks request metadata over plaintext.
 	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
@@ -262,7 +267,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 			return body, nil
 		}
 
-		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
+		errBody, readErr := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
+		if readErr != nil && len(errBody) == 0 {
+			errBody = []byte(fmt.Sprintf("[error reading response body: %v]", readErr))
+		}
 		resp.Body.Close()
 
 		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
diff --git a/github/files.go b/github/files.go
index 0b12c4e..2531b8e 100644
--- a/github/files.go
+++ b/github/files.go
@@ -17,6 +17,39 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref stri
 	return c.GetFileContentAtRef(ctx, owner, repo, path, ref)
 }
 
+// GetFileContentAtRef fetches a file at a specific ref from a repo.
+// If ref is empty, the query parameter is omitted (uses default branch).
+//
+// Note: dot-segments ("." and "..") in the path are silently removed to
+// prevent path traversal. This means a path like "foo/../bar" resolves
+// to "foo/bar" rather than "bar".
+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)
+	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)
+	}
+	if resp.Encoding != "base64" {
+		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path)
+	}
+	decoded, err := decodeBase64Content(resp.Content)
+	if err != nil {
+		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
+	}
+	return decoded, nil
+}
+
 // ListContents lists files and directories at a given path in a repo.
 // Returns the directory listing from the GitHub contents API.
 // If the path points to a single file (not a directory), the API returns
diff --git a/github/pr.go b/github/pr.go
index 89a3d99..2db9304 100644
--- a/github/pr.go
+++ b/github/pr.go
@@ -33,7 +33,6 @@ type changedFileResponse struct {
 
 // 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"`
@@ -123,39 +122,6 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
 	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).
-//
-// Note: dot-segments ("." and "..") in the path are silently removed to
-// prevent path traversal. This means a path like "foo/../bar" resolves
-// to "foo/bar" rather than "bar".
-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)
-	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)
-	}
-	if resp.Encoding != "base64" {
-		return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, path)
-	}
-	decoded, err := decodeBase64Content(resp.Content)
-	if err != nil {
-		return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
-	}
-	return decoded, nil
-}
-
 // GetCommitStatuses fetches both commit statuses and check runs for a SHA,
 // merging them into a unified []vcs.CommitStatus slice.
 // Returns nil (not an empty slice) when there are no statuses or check runs.