Compare commits

..

2 Commits

Author SHA1 Message Date
aweiker 7eeb3147db feat(github): implement FileReader interface (#80)
CI / test (pull_request) Successful in 18s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 58s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m25s
Implement FileReader conformance on the GitHub client: GetFileContent,
ListContents, path helpers, base64 decode. Includes compile-time
conformance checks for both PRReader and FileReader.

Requires PR B (#102). Part 3 of 3 for #80.
2026-05-13 04:13:00 +00:00
aweiker d468ea6022 feat(github): implement PRReader interface (#80)
CI / test (pull_request) Failing after 12s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Has been skipped
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Has been skipped
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Has been skipped
Implement PRReader conformance on the GitHub client: GetPullRequest,
GetPullRequestDiff, GetPullRequestFiles (paginated, populates Patch),
GetCommitStatuses (merges commit statuses + check runs).
Adds compile-time PRReader conformance check.

Requires PR A. Part 2 of 3 for #80.
2026-05-13 04:12:13 +00:00
10 changed files with 445 additions and 354 deletions
+9 -26
View File
@@ -104,10 +104,6 @@ inputs:
description: 'Path to custom persona JSON file'
required: false
default: ''
action-repo:
description: 'Repository hosting the review-bot binary (owner/name). Defaults to rodin/review-bot on Gitea, or strat/review-bot on GitHub.'
required: false
default: ''
runs:
using: 'composite'
@@ -116,21 +112,10 @@ runs:
id: version
shell: bash
run: |
SERVER_URL="${{ inputs.gitea-url || github.server_url }}"
# Detect VCS type: Gitea uses /api/v1/, GitHub uses /api/v3/
if echo "$SERVER_URL" | grep -qi 'gitea'; then
API_BASE="${SERVER_URL}/api/v1"
DEFAULT_ACTION_REPO="rodin/review-bot"
else
API_BASE="${SERVER_URL}/api/v3"
DEFAULT_ACTION_REPO="strat/review-bot"
fi
ACTION_REPO="${{ inputs.action-repo || '' }}"
if [ -z "$ACTION_REPO" ]; then
ACTION_REPO="$DEFAULT_ACTION_REPO"
fi
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
if [ "${{ inputs.version }}" = "latest" ]; then
VERSION=$(curl -sSf "${API_BASE}/repos/${ACTION_REPO}/releases?limit=1" \
VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \
| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
if [ -z "$VERSION" ]; then
echo "Failed to determine latest version" >&2
@@ -140,8 +125,6 @@ runs:
VERSION="${{ inputs.version }}"
fi
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "action-repo=${ACTION_REPO}" >> "$GITHUB_OUTPUT"
echo "server-url=${SERVER_URL}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary
id: cache
@@ -154,14 +137,14 @@ runs:
if: steps.cache.outputs.cache-hit != 'true'
shell: bash
run: |
SERVER_URL="${{ steps.version.outputs.server-url }}"
ACTION_REPO="${{ steps.version.outputs.action-repo }}"
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-linux-amd64"
curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/${BINARY}" \
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot"
curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/checksums.txt" \
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \
-o "${{ runner.temp }}/checksums.txt"
# Verify SHA-256 checksum
@@ -186,8 +169,8 @@ runs:
- name: Run review
shell: bash
env:
GITHUB_SERVER_URL: ${{ inputs.gitea-url || github.server_url }}
GITHUB_REPOSITORY: ${{ inputs.repo || github.repository }}
GITEA_URL: ${{ inputs.gitea-url || github.server_url }}
GITEA_REPO: ${{ inputs.repo || github.repository }}
PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ inputs.reviewer-token }}
REVIEWER_NAME: ${{ inputs.reviewer-name }}
+7 -24
View File
@@ -104,10 +104,6 @@ inputs:
description: 'Path to custom persona JSON file'
required: false
default: ''
action-repo:
description: 'Repository hosting the review-bot binary (owner/name). Defaults to rodin/review-bot on Gitea, or strat/review-bot on GitHub.'
required: false
default: ''
runs:
using: 'composite'
@@ -116,21 +112,10 @@ runs:
id: version
shell: bash
run: |
SERVER_URL="${{ inputs.gitea-url || github.server_url }}"
# Detect VCS type: Gitea uses /api/v1/, GitHub uses /api/v3/
if echo "$SERVER_URL" | grep -qi 'gitea'; then
API_BASE="${SERVER_URL}/api/v1"
DEFAULT_ACTION_REPO="rodin/review-bot"
else
API_BASE="${SERVER_URL}/api/v3"
DEFAULT_ACTION_REPO="strat/review-bot"
fi
ACTION_REPO="${{ inputs.action-repo || '' }}"
if [ -z "$ACTION_REPO" ]; then
ACTION_REPO="$DEFAULT_ACTION_REPO"
fi
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
if [ "${{ inputs.version }}" = "latest" ]; then
VERSION=$(curl -sSf "${API_BASE}/repos/${ACTION_REPO}/releases?limit=1" \
VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \
| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
if [ -z "$VERSION" ]; then
echo "Failed to determine latest version" >&2
@@ -140,8 +125,6 @@ runs:
VERSION="${{ inputs.version }}"
fi
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "action-repo=${ACTION_REPO}" >> "$GITHUB_OUTPUT"
echo "server-url=${SERVER_URL}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary
id: cache
@@ -154,14 +137,14 @@ runs:
if: steps.cache.outputs.cache-hit != 'true'
shell: bash
run: |
SERVER_URL="${{ steps.version.outputs.server-url }}"
ACTION_REPO="${{ steps.version.outputs.action-repo }}"
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-linux-amd64"
curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/${BINARY}" \
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot"
curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/checksums.txt" \
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \
-o "${{ runner.temp }}/checksums.txt"
# Verify SHA-256 checksum
-47
View File
@@ -1,47 +0,0 @@
# Self-review workflow for strat/review-bot on GitHub Enterprise Server.
# Tests that the composite action runs correctly on GitHub runners:
# - GITHUB_SERVER_URL and GITHUB_REPOSITORY env vars are set correctly
# - Binary is downloaded from gitea.weiker.me (where releases live)
# - Review is posted to the corresponding Gitea PR
name: Review
on:
pull_request:
types: [opened, synchronize]
jobs:
review:
runs-on: ubuntu-24.04
if: github.event_name == 'pull_request'
strategy:
matrix:
include:
- name: sonnet
token_secret: SONNET_REVIEW_TOKEN
model: anthropic--claude-4.6-sonnet
- name: gpt
token_secret: GPT_REVIEW_TOKEN
model: gpt-5
steps:
- uses: actions/checkout@v4
- name: Run ${{ matrix.name }} review
uses: ./.gitea/actions/review
with:
# Download binary from Gitea (releases live there, not on GHE)
gitea-url: https://gitea.weiker.me
# Post review to the corresponding Gitea repo
repo: rodin/review-bot
reviewer-token: ${{ secrets[matrix.token_secret] }}
reviewer-name: ${{ matrix.name }}
llm-model: ${{ matrix.model }}
llm-provider: aicore
aicore-client-id: ${{ secrets.AICORE_CLIENT_ID }}
aicore-client-secret: ${{ secrets.AICORE_CLIENT_SECRET }}
aicore-auth-url: ${{ secrets.AICORE_AUTH_URL }}
aicore-api-url: ${{ secrets.AICORE_API_URL }}
aicore-resource-group: ${{ secrets.AICORE_RESOURCE_GROUP }}
conventions-file: CONVENTIONS.md
patterns-repo: rodin/go-patterns
patterns-files: 'README.md,patterns/'
dry-run: 'true'
timeout: '600'
+8 -25
View File
@@ -21,10 +21,6 @@ const (
// maxResponseBytes limits successful response body reads to 10 MiB.
maxResponseBytes = 10 * 1024 * 1024
// maxRetryAttempts is the number of times doRequest will attempt a request.
// The retry backoff slice must have length maxRetryAttempts-1.
maxRetryAttempts = 3
)
// APIError represents an HTTP error response from the GitHub API.
@@ -182,33 +178,24 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
// SetRetryBackoff configures the retry backoff durations for testing.
// It must be called before any goroutines issue requests.
// The slice must have exactly maxRetryAttempts-1 entries (one delay per retry gap).
// In production the default {1s, 2s} applies.
func (c *Client) SetRetryBackoff(d []time.Duration) error {
if len(d) != maxRetryAttempts-1 {
return fmt.Errorf("github: backoff length %d does not match maxRetryAttempts-1 (%d)", len(d), maxRetryAttempts-1)
}
func (c *Client) SetRetryBackoff(d []time.Duration) {
c.retryBackoff = d
return nil
}
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present (capped at maxRetryAfter).
// Transport errors (network failures, context cancellation) are not retried.
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
const maxAttempts = 3
const maxRetryAfter = 120 * time.Second
// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
// Length must be maxRetryAttempts-1 (one entry per retry gap).
// SetRetryBackoff validates at configuration time; the default is always valid.
defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
var backoff []time.Duration
if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
if c.retryBackoff != nil {
backoff = make([]time.Duration, len(c.retryBackoff))
copy(backoff, c.retryBackoff)
} else {
backoff = make([]time.Duration, len(defaultBackoff))
copy(backoff, defaultBackoff)
backoff = []time.Duration{1 * time.Second, 2 * time.Second}
}
// maxErrorBodyBytes limits how much of an error response body is stored.
@@ -228,7 +215,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
}
var lastErr error
for attempt := 0; attempt < maxRetryAttempts; attempt++ {
for attempt := 0; attempt < maxAttempts; attempt++ {
if attempt > 0 {
var delay time.Duration
if attempt-1 < len(backoff) {
@@ -268,10 +255,6 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
return nil, fmt.Errorf("do request: %w", err)
}
// Capture response metadata before handleResponse takes body ownership.
respStatus := resp.StatusCode
retryAfterHeader := resp.Header.Get("Retry-After")
body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
if done {
return body, err
@@ -279,10 +262,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
lastErr = err
// Retry on 429 rate limit
if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 {
// Check for Retry-After header and override backoff if present.
// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
if ra := retryAfterHeader; ra != "" {
if ra := resp.Header.Get("Retry-After"); ra != "" {
if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
delay := time.Duration(seconds) * time.Second
if delay > maxRetryAfter {
@@ -326,7 +309,7 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
return nil, true, fmt.Errorf("read response body: %w", err)
}
if len(body) > maxRespBytes {
return nil, true, fmt.Errorf("response body exceeded %d bytes", maxRespBytes)
return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes)
}
return body, true, nil
}
+6 -44
View File
@@ -83,9 +83,7 @@ func TestDoRequest_429Retry(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
if err := c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond})
body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
@@ -110,9 +108,7 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil {
@@ -222,9 +218,7 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
// Use short backoff; Retry-After should override
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -265,9 +259,7 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
_, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil {
@@ -305,9 +297,7 @@ func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -348,9 +338,7 @@ func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
if err := c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second})
start := time.Now()
_, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -566,29 +554,3 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
}
}
func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) {
c := NewClient("token", "https://api.github.com")
// Too short
err := c.SetRetryBackoff([]time.Duration{1 * time.Second})
if err == nil {
t.Fatal("expected error for backoff length 1")
}
if !strings.Contains(err.Error(), "backoff length 1") {
t.Errorf("unexpected error message: %v", err)
}
// Too long
err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second, 3 * time.Second})
if err == nil {
t.Fatal("expected error for backoff length 3")
}
// Correct length succeeds
err = c.SetRetryBackoff([]time.Duration{1 * time.Second, 2 * time.Second})
if err != nil {
t.Fatalf("unexpected error for valid backoff: %v", err)
}
}
+6 -3
View File
@@ -5,6 +5,9 @@ import (
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time interface conformance assertion.
// Verifies github.Client satisfies vcs.PRReader.
var _ vcs.PRReader = (*github.Client)(nil)
// 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)
)
+80 -45
View File
@@ -6,28 +6,32 @@ import (
"encoding/json"
"fmt"
"net/url"
"path"
"strings"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// GetFileContent fetches a file from a repo at the given ref.
// Delegates to GetFileContentAtRef with the provided ref.
func (c *Client) GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error) {
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).
//
// Returns an error if the path contains dot-segments (".", "..") or
// attempts to traverse above the repository root.
func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath, ref string) (string, error) {
escaped, err := escapePath(filePath)
if err != nil {
return "", fmt.Errorf("invalid file path: %w", err)
}
// 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), escaped)
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", filePath, err)
return "", fmt.Errorf("fetch file %s: %w", path, err)
}
var resp struct {
Content string `json:"content"`
@@ -37,64 +41,95 @@ func (c *Client) GetFileContentAtRef(ctx context.Context, owner, repo, filePath,
return "", fmt.Errorf("parse file content JSON: %w", err)
}
if resp.Encoding != "base64" {
return "", fmt.Errorf("unexpected encoding %q for file %s", resp.Encoding, filePath)
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", filePath, err)
return "", fmt.Errorf("decode base64 content for %s: %w", path, err)
}
return decoded, nil
}
// escapePath validates and encodes a slash-separated file path for use in
// GitHub API URLs. Returns an error if the path contains dot-segments ("."
// or "..") or resolves to a path outside the repository root.
func escapePath(p string) (string, error) {
// Reject paths containing dot-segments rather than silently rewriting them.
for _, seg := range strings.Split(p, "/") {
if seg == "." || seg == ".." {
return "", fmt.Errorf("path contains dot-segment %q: %s", seg, p)
}
// 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
// a JSON object instead of an array; this is handled by returning a
// single-element slice.
//
// 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) 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)
}
// Use path.Clean for canonical form, then verify it doesn't escape root.
cleaned := path.Clean(p)
if cleaned == "." || strings.HasPrefix(cleaned, "..") {
return "", fmt.Errorf("path resolves outside repository root: %s", p)
type entry struct {
Name string `json:"name"`
Path string `json:"path"`
Type string `json:"type"`
}
// Encode each segment individually.
parts := strings.Split(cleaned, "/")
var encoded []string
for _, part := range parts {
if part == "" {
continue
// 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
if err2 := json.Unmarshal(body, &single); err2 != nil {
return nil, fmt.Errorf("parse contents JSON: as array: %v; as object: %w", err, err2)
}
encoded = append(encoded, url.PathEscape(part))
// Guard against empty objects ({}) or unexpected shapes that
// unmarshal successfully but carry no useful data.
if single.Name == "" && single.Path == "" && single.Type == "" {
return nil, fmt.Errorf("parse contents JSON: unexpected response format")
}
entries = []entry{single}
}
return strings.Join(encoded, "/"), nil
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
}
// maxFileContentSize is the maximum decoded file size (10 MB) to prevent
// resource exhaustion when decoding base64 content from the API.
const maxFileContentSize = 10 * 1024 * 1024
// 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 "..") and empty segments (from consecutive slashes like
// "a//b") are silently removed to prevent path traversal and produce canonical
// paths. This is intentional: callers may receive a different path than requested
// without error. The function is package-private, and all callers
// (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
for _, part := range parts {
if part == "." || part == ".." || part == "" {
continue
}
clean = append(clean, url.PathEscape(part))
}
return strings.Join(clean, "/")
}
// decodeBase64Content decodes base64-encoded content from the GitHub contents API.
// GitHub returns base64 content with line breaks for formatting; we strip \r and \n before decoding.
// Returns an error if the decoded content exceeds maxFileContentSize.
func decodeBase64Content(encoded string) (string, error) {
// GitHub inserts newlines in base64 content
cleaned := strings.NewReplacer("\n", "", "\r", "").Replace(encoded)
// Check estimated decoded size before allocating.
// Base64 encodes 3 bytes into 4 chars, so decoded ~ len*3/4.
if len(cleaned)*3/4 > maxFileContentSize {
return "", fmt.Errorf("file content too large: estimated %d bytes exceeds limit of %d", len(cleaned)*3/4, maxFileContentSize)
}
decoded, err := base64.StdEncoding.DecodeString(cleaned)
if err != nil {
return "", err
}
if len(decoded) > maxFileContentSize {
return "", fmt.Errorf("file content too large: %d bytes exceeds limit of %d", len(decoded), maxFileContentSize)
}
return string(decoded), nil
}
+313 -75
View File
@@ -2,95 +2,333 @@ package github
import (
"context"
"encoding/json"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
)
func TestEscapePath_ValidPaths(t *testing.T) {
t.Parallel()
tests := []struct {
name string
path string
want string
}{
{"simple file", "file.go", "file.go"},
{"nested path", "path/to/file.go", "path/to/file.go"},
{"special chars", "path/to/my file.go", "path/to/my%20file.go"},
{"leading slash stripped", "/path/to/file.go", "path/to/file.go"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
got, err := escapePath(tt.path)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if got != tt.want {
t.Errorf("escapePath(%q) = %q, want %q", tt.path, got, tt.want)
}
})
}
}
func TestEscapePath_DotSegments(t *testing.T) {
t.Parallel()
tests := []struct {
name string
path string
}{
{"single dot", "./file.go"},
{"double dot", "../file.go"},
{"dot in middle", "path/./file.go"},
{"parent traversal", "path/../file.go"},
{"only dots", ".."},
{"nested parent traversal", "a/b/../../c"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()
_, err := escapePath(tt.path)
if err == nil {
t.Fatalf("expected error for path %q, got nil", tt.path)
}
if !strings.Contains(err.Error(), "dot-segment") {
t.Errorf("expected error about dot-segment, got: %v", err)
}
})
}
}
func TestGetFileContentAtRef_DotSegmentError(t *testing.T) {
// Server should never be called — the error is caught before the request.
func TestGetFileContent_DelegatesToGetFileContentAtRef(t *testing.T) {
var gotRef string
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("server should not have been called")
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)
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "foo/../bar.go", "main")
if err == nil {
t.Fatal("expected error for path with dot-segments")
c := NewClient("token", srv.URL, AllowInsecureHTTP())
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 !strings.Contains(err.Error(), "invalid file path") {
t.Errorf("expected 'invalid file path' error, got: %v", err)
if content != "test" {
t.Errorf("expected 'test', got %q", content)
}
if gotRef != "" {
t.Errorf("expected empty ref, got %q", gotRef)
}
}
func TestDecodeBase64Content_SizeLimit(t *testing.T) {
t.Parallel()
// Create base64 content that would decode to > maxFileContentSize.
// maxFileContentSize is 10MB. Base64 of 11MB worth of zeros.
// We just need something big enough to trigger the estimated size check.
// 14MB of base64 chars (decodes to ~10.5MB).
huge := strings.Repeat("A", 14*1024*1024)
_, err := decodeBase64Content(huge)
if err == nil {
t.Fatal("expected error for oversized content")
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContent(context.Background(), "owner", "repo", "file.go", "abc123")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if !strings.Contains(err.Error(), "too large") {
t.Errorf("expected 'too large' error, got: %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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]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, AllowInsecureHTTP())
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")
}
}
func TestEscapePath_RejectsDotSegments(t *testing.T) {
tests := []struct {
input string
want string
}{
{"src/main.go", "src/main.go"},
{"../etc/passwd", "etc/passwd"},
{"./src/../main.go", "src/main.go"},
{"a/b/c", "a/b/c"},
{"file with spaces.go", "file%20with%20spaces.go"},
{"a/./b/../c", "a/b/c"},
}
for _, tt := range tests {
got := escapePath(tt.input)
if got != tt.want {
t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want)
}
}
}
func TestDecodeBase64Content_CRLF(t *testing.T) {
// Base64 of "hello world" with CRLF line breaks inserted
encoded := "aGVs\r\nbG8g\r\nd29y\r\nbGQ="
decoded, err := decodeBase64Content(encoded)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if decoded != "hello world" {
t.Errorf("expected 'hello world', got %q", decoded)
}
}
func TestListContents_SingleFile(t *testing.T) {
// GitHub Contents API returns a JSON object (not array) for single-file paths
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(200)
w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file"}`))
}))
defer srv.Close()
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
entries, err := c.ListContents(context.Background(), "owner", "repo", "README.md")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(entries) != 1 {
t.Fatalf("expected 1 entry, got %d", len(entries))
}
if entries[0].Name != "README.md" {
t.Errorf("expected name 'README.md', got %q", entries[0].Name)
}
if entries[0].Type != "file" {
t.Errorf("expected type 'file', got %q", entries[0].Type)
}
}
+16 -26
View File
@@ -51,10 +51,7 @@ type checkRunsResponse struct {
} `json:"check_runs"`
}
// GetPullRequest fetches PR metadata from the GitHub API.
// Returns an *APIError wrapping the HTTP status on non-2xx responses (e.g.
// IsNotFound for 404, IsUnauthorized for 401). Network and context errors
// are wrapped but not typed as *APIError.
// GetPullRequest fetches PR metadata.
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)
@@ -85,15 +82,9 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
return string(body), nil
}
const (
// maxFilesPages is the upper bound on pagination loops for PR file listing,
// preventing unbounded iteration if the server always returns a full page.
maxFilesPages = 100
// maxCheckRunPages is the upper bound on pagination loops for check-run listing,
// preventing unbounded iteration if the server always returns a full page.
maxCheckRunPages = 100
)
// maxPages is the upper bound on pagination loops to prevent unbounded iteration
// in case the server returns a full page indefinitely.
const maxPages = 100
// GetPullRequestFiles fetches the list of files changed in a PR.
// Paginates through all pages (100 per page) to collect all files.
@@ -102,7 +93,7 @@ const (
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
var allFiles []vcs.ChangedFile
for page := 1; page <= maxFilesPages; page++ {
for page := 1; page <= maxPages; page++ {
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)
@@ -163,7 +154,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
}
// Fetch check runs (paginated)
for checkPage := 1; checkPage <= maxCheckRunPages; checkPage++ {
for checkPage := 1; checkPage <= maxPages; checkPage++ {
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)
@@ -178,7 +169,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
result = append(result, vcs.CommitStatus{
Context: cr.Name,
Status: mapCheckRunStatus(cr.Conclusion),
Description: "", // check runs have no human-readable description; conclusion is captured in Status
Description: derefString(cr.Conclusion),
TargetURL: cr.HTMLURL,
})
}
@@ -190,17 +181,9 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
return result, nil
}
// mapCheckRunStatus maps a GitHub check run conclusion to a vcs.CommitStatus status string.
// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
// Conclusion alone determines the mapped state: nil conclusion means the run is
// still in progress (pending), regardless of the status field value.
//
// Mapping rules:
// - nil → "pending" (run still in progress or queued)
// - "success" → "success"
// - "failure", "action_required", "timed_out" → "failure"
// - "cancelled", "skipped", "neutral" → "success" (non-blocking per GitHub check suite semantics)
// - "stale" → "pending" (check run became stale before completing)
// - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete)
func mapCheckRunStatus(conclusion *string) string {
if conclusion == nil {
// Still running or queued
@@ -213,10 +196,17 @@ func mapCheckRunStatus(conclusion *string) string {
return "failure"
case "cancelled", "skipped", "neutral":
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
case "stale":
case "stale", "waiting":
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
}
-39
View File
@@ -545,7 +545,6 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
name = *tt.conclusion
}
t.Run(name, func(t *testing.T) {
t.Parallel()
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{}{
@@ -633,44 +632,6 @@ func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
}
}
func TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch {
case strings.Contains(r.URL.Path, "/status"):
// Statuses succeed
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"):
// Check runs fail with 500
w.WriteHeader(500)
w.Write([]byte(`{"message":"Internal Server Error"}`))
default:
w.WriteHeader(404)
}
}))
defer srv.Close()
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
if err == nil {
t.Fatal("expected error when check-runs endpoint fails after statuses succeed")
}
if !strings.Contains(err.Error(), "fetch check runs") {
t.Errorf("expected check runs error, got: %v", err)
}
}
func stringPtr(s string) *string {
return &s
}