Compare commits
3 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| 329d68e4b4 | |||
| 3995fa3136 | |||
| d468ea6022 |
@@ -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 }}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
@@ -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
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
+16
-48
@@ -6,28 +6,24 @@ import (
|
||||
"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)
|
||||
}
|
||||
// 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 +33,36 @@ 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)
|
||||
}
|
||||
}
|
||||
|
||||
// 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
|
||||
// escapePath encodes each segment of a slash-separated path, stripping
|
||||
// dot-segments to prevent path traversal.
|
||||
func escapePath(p string) string {
|
||||
parts := strings.Split(p, "/")
|
||||
var clean []string
|
||||
for _, part := range parts {
|
||||
if part == "" {
|
||||
if part == "." || part == ".." || part == "" {
|
||||
continue
|
||||
}
|
||||
encoded = append(encoded, url.PathEscape(part))
|
||||
clean = append(clean, url.PathEscape(part))
|
||||
}
|
||||
return strings.Join(encoded, "/"), nil
|
||||
return strings.Join(clean, "/")
|
||||
}
|
||||
|
||||
// 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
|
||||
}
|
||||
|
||||
@@ -1,96 +0,0 @@
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
||||
+10
-3
@@ -178,7 +178,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), // raw conclusion value (e.g. "success", "failure", "skipped")
|
||||
TargetURL: cr.HTMLURL,
|
||||
})
|
||||
}
|
||||
@@ -199,7 +199,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
|
||||
// - "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)
|
||||
// - "stale", "waiting" → "pending"
|
||||
// - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete)
|
||||
func mapCheckRunStatus(conclusion *string) string {
|
||||
if conclusion == nil {
|
||||
@@ -213,10 +213,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
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user