Compare commits

...

11 Commits

Author SHA1 Message Date
Rodin db13078196 chore: use dry-run in review.yml (GHE has no releases yet, validate infra)
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 29s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m2s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m31s
The self-test workflow on github.concur.com runs the action with:
- gitea-url=https://gitea.weiker.me (binary download source)
- dry-run=true (avoids PR# mismatch between GHE and Gitea)

This validates:
- Binary download and checksum verification works from a GitHub runner
- GITHUB_SERVER_URL/GITHUB_REPOSITORY env vars are correctly passed
- AiCore provider authenticates and LLM call succeeds

When strat/review-bot has its own releases, remove gitea-url override and dry-run.
2026-05-14 04:07:01 +00:00
Rodin 3ac5e5dcca fix(#120): detect VCS host for releases API and derive action-repo
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 29s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m40s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m3s
Both composite actions hardcoded:
- Gitea's /api/v1/ releases endpoint path
- 'rodin/review-bot' as the action source repo
- .gitea version used GITEA_URL/GITEA_REPO env vars instead of
  GITHUB_SERVER_URL/GITHUB_REPOSITORY

Fix:
- Add 'action-repo' input (default: empty) to allow explicit override
- Auto-detect VCS host from server_url: URLs containing 'gitea' use
  /api/v1/ (Gitea format), all others use /api/v3/ (GitHub format)
- Set smart default action-repo: 'rodin/review-bot' on Gitea,
  'strat/review-bot' on GitHub
- Pass server-url and action-repo as step outputs to avoid re-computing
- Fix .gitea action's 'Run review' env to use GITHUB_SERVER_URL and
  GITHUB_REPOSITORY (matching .github version and review-bot binary
  env var expectations)
- Add .github/workflows/review.yml for self-testing on github.concur.com

Backward compatible: existing Gitea callers using default inputs continue
to resolve rodin/review-bot via /api/v1/ unchanged.

Closes #120
2026-05-14 04:03:56 +00:00
aweiker 71bb33b6fd Merge pull request 'feat(github): implement PRReader interface' (#102) from issue-80-b-pr-reader into feature/github-support
Reviewed-on: #102
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 05:30:37 +00:00
claw 55366b3431 fix: address review feedback on PRReader implementation
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 45s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m55s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m55s
- Add maxFileContentSize (10 MB) limit to decodeBase64Content to prevent
  resource exhaustion from oversized file content (security MINOR)
- Fix reversed NewClient arg order in TestGetFileContentAtRef_DotSegmentError
  (GPT MINOR + Sonnet NIT)
- Remove 'waiting' from mapCheckRunStatus conclusion cases since it is a
  status value not a conclusion, update comment (GPT NIT)
- Add TestDecodeBase64Content_SizeLimit test
2026-05-12 22:17:32 -07:00
claw 3cd5ae594e fix(github): escapePath returns error on dot-segments, fix Description semantics
PR Ready Gate / clear-labels (pull_request) Successful in 1s
CI / test (pull_request) Successful in 23s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 33s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m24s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m44s
- escapePath now returns an error when paths contain dot-segments
  (".", "..") instead of silently rewriting them. This prevents
  subtle API misses where callers pass "foo/../bar" expecting to
  hit "bar" but the old code produced "foo/bar".
- Uses path.Clean for canonical form after validation.
- CommitStatus.Description for check runs is now empty string
  instead of the raw conclusion enum. The conclusion is already
  captured in the Status field via mapCheckRunStatus; storing it
  again in Description was semantically inconsistent with commit
  statuses where Description carries a human-readable narrative.
- Removed unused derefString helper.
- Added tests for escapePath valid paths, dot-segment rejection,
  and GetFileContentAtRef dot-segment error propagation.
2026-05-12 22:03:52 -07:00
claw eaccc96073 fix: address review feedback on PR #102
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 27s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 42s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m11s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m13s
- Separate maxPages into maxFilesPages and maxCheckRunPages constants
  for clarity (sonnet MINOR #1)
- Add parallel to CheckRunConclusions subtests (sonnet MINOR #2)
- Add TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed test
  covering check-runs 500 after statuses succeed (sonnet MINOR #2)
- Expand mapCheckRunStatus doc comment with full mapping rules including
  cancelled/skipped/neutral rationale and unknown value behavior
  (sonnet MINOR #3, gpt MINOR #1)
- Expand GetPullRequest doc comment to mention error types returned
  (sonnet NIT #4)
- Add inline comment on Description field clarifying it holds raw
  conclusion value (gpt NIT #3)
2026-05-13 04:47:15 +00:00
claw 289b400bfd fix(github): add GetFileContentAtRef and fix conformance test
- Implement GetFileContentAtRef on *Client to satisfy vcs.PRReader interface
- Add escapePath and decodeBase64Content helpers
- Fix conformance_test.go to properly import and qualify github.Client
  (was using unqualified Client in package github_test)

Fixes CI failure: the PRReader interface requires GetFileContentAtRef
but it was missing from this PR (only present in the file-reader PR).
2026-05-13 04:47:15 +00:00
aweiker d0b7f09772 feat(github): implement PRReader interface (#80)
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:47:15 +00:00
aweiker 377da8ca3a Merge pull request 'feat(github): implement GitHub API client foundation' (#101) from issue-80-a-client into feature/github-support
Reviewed-on: #101
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 04:46:46 +00:00
claw 61819ac3e3 fix(github): address review findings - remove panic, validate at config time
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 17s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m35s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m7s
- MAJOR #1: Replace panic in doRequest with safe default fallback.
  Validation now happens in SetRetryBackoff (returns error on invalid
  length). doRequest gracefully falls back to default backoff if the
  configured slice is somehow invalid.

- MINOR #2: SetRetryBackoff validates slice length at configuration
  time, making the coupling between maxRetryAttempts and backoff
  explicit and catching mismatches early with a clear error.

- MINOR #4: Reword oversized response error to remove '(truncated)'
  which implied truncated data was returned when actually only an
  error is returned.

- MINOR #5: Functional options kept as-is - idiomatic Go pattern
  that allows future growth without breaking the API.
2026-05-12 21:31:45 -07:00
claw 3d1260d3b2 fix(github): clarify response ownership and validate backoff length
PR Ready Gate / clear-labels (pull_request) Successful in 2s
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, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m22s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m51s
Address review feedback on PR #101:

1. Capture resp.StatusCode and Retry-After header *before* passing resp
   to handleResponse, making ownership transfer explicit. Previously the
   caller read resp.StatusCode after handleResponse had closed the body —
   correct but fragile coupling.

2. Add panic guard ensuring backoff slice length equals maxAttempts-1.
   Previously the relationship was implicit and could silently break if
   maxAttempts were changed without updating the default backoff.
2026-05-12 21:26:39 -07:00
10 changed files with 1270 additions and 30 deletions
+26 -9
View File
@@ -104,6 +104,10 @@ inputs:
description: 'Path to custom persona JSON file' description: 'Path to custom persona JSON file'
required: false required: false
default: '' 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: runs:
using: 'composite' using: 'composite'
@@ -112,10 +116,21 @@ runs:
id: version id: version
shell: bash shell: bash
run: | run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}" SERVER_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}" # 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
if [ "${{ inputs.version }}" = "latest" ]; then if [ "${{ inputs.version }}" = "latest" ]; then
VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \ VERSION=$(curl -sSf "${API_BASE}/repos/${ACTION_REPO}/releases?limit=1" \
| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')") | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
if [ -z "$VERSION" ]; then if [ -z "$VERSION" ]; then
echo "Failed to determine latest version" >&2 echo "Failed to determine latest version" >&2
@@ -125,6 +140,8 @@ runs:
VERSION="${{ inputs.version }}" VERSION="${{ inputs.version }}"
fi fi
echo "version=${VERSION}" >> "$GITHUB_OUTPUT" echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "action-repo=${ACTION_REPO}" >> "$GITHUB_OUTPUT"
echo "server-url=${SERVER_URL}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary - name: Cache review-bot binary
id: cache id: cache
@@ -137,14 +154,14 @@ runs:
if: steps.cache.outputs.cache-hit != 'true' if: steps.cache.outputs.cache-hit != 'true'
shell: bash shell: bash
run: | run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}" SERVER_URL="${{ steps.version.outputs.server-url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}" ACTION_REPO="${{ steps.version.outputs.action-repo }}"
VERSION="${{ steps.version.outputs.version }}" VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-linux-amd64" BINARY="review-bot-linux-amd64"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \ curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot" -o "${{ runner.temp }}/review-bot"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \ curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/checksums.txt" \
-o "${{ runner.temp }}/checksums.txt" -o "${{ runner.temp }}/checksums.txt"
# Verify SHA-256 checksum # Verify SHA-256 checksum
@@ -169,8 +186,8 @@ runs:
- name: Run review - name: Run review
shell: bash shell: bash
env: env:
GITEA_URL: ${{ inputs.gitea-url || github.server_url }} GITHUB_SERVER_URL: ${{ inputs.gitea-url || github.server_url }}
GITEA_REPO: ${{ inputs.repo || github.repository }} GITHUB_REPOSITORY: ${{ inputs.repo || github.repository }}
PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }} PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ inputs.reviewer-token }} REVIEWER_TOKEN: ${{ inputs.reviewer-token }}
REVIEWER_NAME: ${{ inputs.reviewer-name }} REVIEWER_NAME: ${{ inputs.reviewer-name }}
+24 -7
View File
@@ -104,6 +104,10 @@ inputs:
description: 'Path to custom persona JSON file' description: 'Path to custom persona JSON file'
required: false required: false
default: '' 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: runs:
using: 'composite' using: 'composite'
@@ -112,10 +116,21 @@ runs:
id: version id: version
shell: bash shell: bash
run: | run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}" SERVER_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}" # 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
if [ "${{ inputs.version }}" = "latest" ]; then if [ "${{ inputs.version }}" = "latest" ]; then
VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \ VERSION=$(curl -sSf "${API_BASE}/repos/${ACTION_REPO}/releases?limit=1" \
| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')") | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
if [ -z "$VERSION" ]; then if [ -z "$VERSION" ]; then
echo "Failed to determine latest version" >&2 echo "Failed to determine latest version" >&2
@@ -125,6 +140,8 @@ runs:
VERSION="${{ inputs.version }}" VERSION="${{ inputs.version }}"
fi fi
echo "version=${VERSION}" >> "$GITHUB_OUTPUT" echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "action-repo=${ACTION_REPO}" >> "$GITHUB_OUTPUT"
echo "server-url=${SERVER_URL}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary - name: Cache review-bot binary
id: cache id: cache
@@ -137,14 +154,14 @@ runs:
if: steps.cache.outputs.cache-hit != 'true' if: steps.cache.outputs.cache-hit != 'true'
shell: bash shell: bash
run: | run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}" SERVER_URL="${{ steps.version.outputs.server-url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}" ACTION_REPO="${{ steps.version.outputs.action-repo }}"
VERSION="${{ steps.version.outputs.version }}" VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-linux-amd64" BINARY="review-bot-linux-amd64"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \ curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot" -o "${{ runner.temp }}/review-bot"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \ curl -sSfL "${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}/checksums.txt" \
-o "${{ runner.temp }}/checksums.txt" -o "${{ runner.temp }}/checksums.txt"
# Verify SHA-256 checksum # Verify SHA-256 checksum
+47
View File
@@ -0,0 +1,47 @@
# 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'
+25 -8
View File
@@ -21,6 +21,10 @@ const (
// maxResponseBytes limits successful response body reads to 10 MiB. // maxResponseBytes limits successful response body reads to 10 MiB.
maxResponseBytes = 10 * 1024 * 1024 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. // APIError represents an HTTP error response from the GitHub API.
@@ -178,24 +182,33 @@ func (c *Client) SetHTTPClient(hc *http.Client) {
// SetRetryBackoff configures the retry backoff durations for testing. // SetRetryBackoff configures the retry backoff durations for testing.
// It must be called before any goroutines issue requests. // 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. // In production the default {1s, 2s} applies.
func (c *Client) SetRetryBackoff(d []time.Duration) { 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)
}
c.retryBackoff = d c.retryBackoff = d
return nil
} }
// doRequest performs an HTTP request with retry on 429 rate limit responses. // doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present (capped at maxRetryAfter). // It respects the Retry-After header when present (capped at maxRetryAfter).
// Transport errors (network failures, context cancellation) are not retried. // Transport errors (network failures, context cancellation) are not retried.
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) { func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
const maxAttempts = 3
const maxRetryAfter = 120 * time.Second 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 var backoff []time.Duration
if c.retryBackoff != nil { if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
backoff = make([]time.Duration, len(c.retryBackoff)) backoff = make([]time.Duration, len(c.retryBackoff))
copy(backoff, c.retryBackoff) copy(backoff, c.retryBackoff)
} else { } else {
backoff = []time.Duration{1 * time.Second, 2 * time.Second} backoff = make([]time.Duration, len(defaultBackoff))
copy(backoff, defaultBackoff)
} }
// maxErrorBodyBytes limits how much of an error response body is stored. // maxErrorBodyBytes limits how much of an error response body is stored.
@@ -215,7 +228,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
} }
var lastErr error var lastErr error
for attempt := 0; attempt < maxAttempts; attempt++ { for attempt := 0; attempt < maxRetryAttempts; attempt++ {
if attempt > 0 { if attempt > 0 {
var delay time.Duration var delay time.Duration
if attempt-1 < len(backoff) { if attempt-1 < len(backoff) {
@@ -255,6 +268,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
return nil, fmt.Errorf("do request: %w", err) 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) body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
if done { if done {
return body, err return body, err
@@ -262,10 +279,10 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
lastErr = err lastErr = err
// Retry on 429 rate limit // Retry on 429 rate limit
if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 { if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
// Check for Retry-After header and override backoff if present. // Check for Retry-After header and override backoff if present.
// Supports both integer seconds (common) and HTTP-date format (RFC 7231). // Supports both integer seconds (common) and HTTP-date format (RFC 7231).
if ra := resp.Header.Get("Retry-After"); ra != "" { if ra := retryAfterHeader; ra != "" {
if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 { if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
delay := time.Duration(seconds) * time.Second delay := time.Duration(seconds) * time.Second
if delay > maxRetryAfter { if delay > maxRetryAfter {
@@ -309,7 +326,7 @@ func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrByt
return nil, true, fmt.Errorf("read response body: %w", err) return nil, true, fmt.Errorf("read response body: %w", err)
} }
if len(body) > maxRespBytes { if len(body) > maxRespBytes {
return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes) return nil, true, fmt.Errorf("response body exceeded %d bytes", maxRespBytes)
} }
return body, true, nil return body, true, nil
} }
+44 -6
View File
@@ -83,7 +83,9 @@ func TestDoRequest_429Retry(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP()) c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}) if err := c.SetRetryBackoff([]time.Duration{10 * time.Millisecond, 10 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
body, err := c.doGet(context.Background(), srv.URL+"/test") body, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil { if err != nil {
@@ -108,7 +110,9 @@ func TestDoRequest_429ExhaustsRetries(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP()) c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
_, err := c.doGet(context.Background(), srv.URL+"/test") _, err := c.doGet(context.Background(), srv.URL+"/test")
if err == nil { if err == nil {
@@ -218,7 +222,9 @@ func TestDoRequest_429RetryAfterHeader(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP()) c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
// Use short backoff; Retry-After should override // Use short backoff; Retry-After should override
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
start := time.Now() start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test") body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -259,7 +265,9 @@ func TestDoRequest_RetryAfterDoesNotMutateBackoff(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP()) c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
_, err := c.doGet(context.Background(), srv.URL+"/test") _, err := c.doGet(context.Background(), srv.URL+"/test")
if err != nil { if err != nil {
@@ -297,7 +305,9 @@ func TestDoRequest_429RetryAfterHTTPDate(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP()) c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
start := time.Now() start := time.Now()
body, err := c.doGet(context.Background(), srv.URL+"/test") body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -338,7 +348,9 @@ func TestDoRequest_429RetryAfterHTTPDateInPast(t *testing.T) {
c := NewClient("token", srv.URL, AllowInsecureHTTP()) c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client()) c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}) if err := c.SetRetryBackoff([]time.Duration{5 * time.Second, 5 * time.Second}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
start := time.Now() start := time.Now()
_, err := c.doGet(context.Background(), srv.URL+"/test") _, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -554,3 +566,29 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)") 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)
}
}
+10
View File
@@ -0,0 +1,10 @@
package github_test
import (
"gitea.weiker.me/rodin/review-bot/github"
"gitea.weiker.me/rodin/review-bot/vcs"
)
// Compile-time interface conformance assertion.
// Verifies github.Client satisfies vcs.PRReader.
var _ vcs.PRReader = (*github.Client)(nil)
+100
View File
@@ -0,0 +1,100 @@
package github
import (
"context"
"encoding/base64"
"encoding/json"
"fmt"
"net/url"
"path"
"strings"
)
// 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)
}
reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escaped)
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)
}
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, filePath)
}
decoded, err := decodeBase64Content(resp.Content)
if err != nil {
return "", fmt.Errorf("decode base64 content for %s: %w", filePath, 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)
}
}
// 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)
}
// Encode each segment individually.
parts := strings.Split(cleaned, "/")
var encoded []string
for _, part := range parts {
if part == "" {
continue
}
encoded = append(encoded, url.PathEscape(part))
}
return strings.Join(encoded, "/"), 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
// 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) {
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
}
+96
View File
@@ -0,0 +1,96 @@
package github
import (
"context"
"net/http"
"net/http/httptest"
"strings"
"testing"
)
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.
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
t.Fatal("server should not have been called")
}))
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")
}
if !strings.Contains(err.Error(), "invalid file path") {
t.Errorf("expected 'invalid file path' error, got: %v", err)
}
}
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")
}
if !strings.Contains(err.Error(), "too large") {
t.Errorf("expected 'too large' error, got: %v", err)
}
}
+222
View File
@@ -0,0 +1,222 @@
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 {
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 {
CheckRuns []struct {
Name string `json:"name"`
Conclusion *string `json:"conclusion"`
Status string `json:"status"`
HTMLURL string `json:"html_url"`
} `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.
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)
}
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
}
// 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)
}
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
)
// 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
for page := 1; page <= maxFilesPages; 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)
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 {
return nil, fmt.Errorf("parse PR files JSON: %w", err)
}
if len(files) == 0 {
break
}
for _, f := range files {
allFiles = append(allFiles, vcs.ChangedFile{
Filename: f.Filename,
Status: f.Status,
Patch: f.Patch,
})
}
if len(files) < 100 {
break
}
}
return allFiles, 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.
// If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
// function returns immediately without attempting the check-runs endpoint.
// If the check-runs endpoint fails after statuses were fetched successfully,
// the function returns an error (not a partial result) so callers always get
// either a complete view or a clear error signal.
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
var result []vcs.CommitStatus
// 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,
Status: s.State,
Description: s.Description,
TargetURL: s.TargetURL,
})
}
// Fetch check runs (paginated)
for checkPage := 1; checkPage <= maxCheckRunPages; 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)
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)
}
for _, cr := range checkResp.CheckRuns {
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
TargetURL: cr.HTMLURL,
})
}
if len(checkResp.CheckRuns) < 100 {
break
}
}
return result, nil
}
// mapCheckRunStatus maps a GitHub 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
return "pending"
}
switch *conclusion {
case "success":
return "success"
case "failure", "action_required", "timed_out":
return "failure"
case "cancelled", "skipped", "neutral":
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
case "stale":
return "pending"
default:
return "pending"
}
}
+676
View File
@@ -0,0 +1,676 @@
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]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) {
w.WriteHeader(200)
w.Write([]byte(`{invalid json`))
}))
defer srv.Close()
c := NewClient("token", srv.URL, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
_, err := c.GetFileContentAtRef(context.Background(), "owner", "repo", "file.go", "main")
if err == nil {
t.Fatal("expected error for malformed JSON")
}
}
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
c.SetRetryBackoff([]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, AllowInsecureHTTP())
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
}{
{stringPtr("success"), "completed", "success"},
{stringPtr("failure"), "completed", "failure"},
{stringPtr("action_required"), "completed", "failure"},
{stringPtr("timed_out"), "completed", "failure"},
{stringPtr("cancelled"), "completed", "success"},
{stringPtr("skipped"), "completed", "success"},
{stringPtr("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) {
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{}{
"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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
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, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "sha")
if err == nil {
t.Fatal("expected error for malformed JSON")
}
}
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
}