Compare commits

...

12 Commits

Author SHA1 Message Date
Rodin 6c46220a53 docs: document runner requirements for composite action
CI / test (push) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
Add a Runner Requirements section to the README documenting that
the composite action needs python3, sha256sum, and curl on the
runner. All are pre-installed on ubuntu-* runners but custom
images need to provide them.

Closes #12
2026-05-02 10:21:53 -07:00
rodin d640eb6e71 Merge pull request 'fix: distinguish 404 in GetAllFilesInPath, make uploads idempotent' (#33) from fix/8-10-error-handling-idempotent-upload into main
CI / test (push) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
2026-05-02 17:07:22 +00:00
Rodin 2339999d37 fix: URL-encode asset filename, truncate error body in APIError
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 51s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m21s
- URL-encode filename in release upload query param (MINOR)
- Truncate APIError.Body to 200 chars in Error() to avoid leaking
  verbose server responses into logs (NIT)
2026-05-02 10:02:03 -07:00
Rodin bfca28b2b2 fix: address review findings from PR #33
CI / test (pull_request) Successful in 13s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m9s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m42s
- Wrap fileErr instead of err in GetAllFilesInPath fallback (MINOR)
- Use env var for asset name in release workflow to avoid quoting issues (NIT)
2026-05-02 09:58:41 -07:00
Rodin f047c994bf fix: distinguish 404 in GetAllFilesInPath, make uploads idempotent
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 58s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m7s
- Add APIError type with StatusCode field so callers can inspect HTTP
  status codes from Gitea API responses
- Add IsNotFound helper for ergonomic 404 checks
- GetAllFilesInPath now only falls back to single-file fetch on 404;
  all other errors (auth failures, server errors, rate limits) propagate
- Release workflow asset uploads are now idempotent: existing assets
  with the same name are deleted before re-upload on workflow re-runs

Closes #8
Closes #10
2026-05-02 09:50:35 -07:00
rodin b51a19d8b9 Merge pull request 'fix: remove worst-wins escalation logic' (#31) from fix/28-remove-escalation into main
CI / test (push) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
2026-05-02 16:46:05 +00:00
Rodin ceefa4c2e0 ci: use separate SECURITY_REVIEW_TOKEN for security reviewer
CI / test (pull_request) Successful in 13s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 41s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 58s
The security-review-bot Gitea user now has its own token. This
completes the token separation so each reviewer role posts under
its own identity, enabling native Gitea multi-reviewer blocking.
2026-05-02 07:25:43 -07:00
Rodin b1f5dd4b5f fix: skip update-in-place when shared token detected
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m21s
When hasSharedToken() detects two roles sharing the same Gitea user,
the bot now skips ALL update logic (PATCH, supersede) and always POSTs
a fresh review. This prevents clobbering a sibling's review body or
state when misconfigured.

Tests now assert return values (true/false) rather than just verifying
no panic. Added additional test case for three-roles-same-user scenario.

Addresses review feedback: update logic and review state must not
interact with sibling reviews under the same user.
2026-05-02 07:21:46 -07:00
Rodin fd179b891b fix: detect shared-token misconfiguration and warn
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 24s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 42s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 55s
When two review-bot roles share the same Gitea user token (misconfiguration),
log a WARNING identifying which sibling is sharing. The bot continues normally
with its own honest verdict — no escalation, no deadlock. Operators see the
warning in CI logs and can fix the token setup.

Addresses Aaron's review feedback on #28: graceful degradation when someone
doesn't follow the separate-token deployment instructions.
2026-05-02 07:11:57 -07:00
Rodin b78d9972ac fix: remove worst-wins escalation logic (#28)
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 36s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
2026-05-02 07:04:33 -07:00
rodin 3c785c5502 Merge pull request 'fix: consistent url.PathEscape across all Gitea client endpoints' (#30) from fix/consistent-path-escape into main
CI / test (push) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (push) Has been skipped
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (push) Has been skipped
2026-05-02 14:01:53 +00:00
Rodin c2595d0263 fix: consistent url.PathEscape across all Gitea client endpoints
CI / test (pull_request) Successful in 14s
CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 21s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SONNET_REVIEW_TOKEN) (pull_request) Successful in 34s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 35s
Apply url.PathEscape to owner, repo, and sha path segments in all
methods that were previously interpolating raw values. Methods already
using PathEscape (ListReviews, DeleteReview, GetTimelineReviewCommentID,
EditComment) are unchanged.

This eliminates an inconsistency flagged in PRs #17, #20, and #22 and
prevents potential path-injection bugs for names with special characters.

Closes #24
2026-05-02 02:41:51 -07:00
7 changed files with 322 additions and 196 deletions
+1 -1
View File
@@ -33,7 +33,7 @@ jobs:
token_secret: GPT_REVIEW_TOKEN
model: gpt-4.1
- name: security
token_secret: SONNET_REVIEW_TOKEN
token_secret: SECURITY_REVIEW_TOKEN
model: gpt-5
system_prompt_file: SECURITY_REVIEW.md
steps:
+16 -2
View File
@@ -69,14 +69,28 @@ jobs:
echo "Release ID: ${RELEASE_ID}"
# Upload each asset
# Upload each asset (idempotent: delete existing asset with same name first)
for file in dist/*; do
filename=$(basename "$file")
echo "Uploading ${filename}..."
# Check if asset already exists and delete it
EXISTING_ID=$(export ASSET_NAME="${filename}"; curl -sS \
-H "Authorization: token ${GITEA_TOKEN}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets" \
| python3 -c "import json,sys,os; name=os.environ['ASSET_NAME']; assets=json.load(sys.stdin); print(next((str(a['id']) for a in assets if a['name']==name),''))" 2>/dev/null)
if [ -n "$EXISTING_ID" ]; then
echo " Asset ${filename} already exists (id=${EXISTING_ID}), deleting..."
curl -sSf -X DELETE \
-H "Authorization: token ${GITEA_TOKEN}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets/${EXISTING_ID}"
fi
curl -sSf -X POST \
-H "Authorization: token ${GITEA_TOKEN}" \
-H "Content-Type: application/octet-stream" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=${filename}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=$(printf '%s' "${filename}" | jq -sRr @uri)" \
--data-binary "@${file}"
done
+12
View File
@@ -188,6 +188,18 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
| `update-existing` | No | `true` | Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no |
| `version` | No | `latest` | review-bot version to install |
## Runner Requirements
The composite action requires these tools on the runner:
| Tool | Used For |
|------|----------|
| `python3` | JSON parsing during version detection |
| `sha256sum` | Checksum verification of downloaded binary |
| `curl` | Downloading releases and querying the API |
All three are pre-installed on `ubuntu-*` runners (e.g. `ubuntu-24.04`). If you use a custom runner image, ensure these are available.
## How Review Cleanup Works
When `reviewer-name` is set, the bot embeds a hidden sentinel in each review:
+47 -52
View File
@@ -266,16 +266,13 @@ func main() {
if err != nil {
log.Printf("Warning: could not list existing reviews: %v", err)
} else {
// Worst-wins: escalate if a sibling blocks (need own login from existing review)
ownLogin := ""
// Detect shared-token misconfiguration: if detected, skip all
// update logic (PATCH/supersede) to avoid clobbering a sibling's review.
sharedToken := hasSharedToken(existingReviews, sentinel)
if sharedToken {
log.Printf("Shared token mode: skipping update-in-place logic to avoid clobbering sibling review")
} else {
existing := findOwnReview(existingReviews, sentinel)
if existing != nil {
ownLogin = existing.User.Login
}
if event == "APPROVED" && shouldEscalate(existingReviews, 0, ownLogin, sentinel) {
log.Printf("Sibling review has REQUEST_CHANGES; escalating to REQUEST_CHANGES")
event = "REQUEST_CHANGES"
}
if existing != nil {
if reviewUnchanged(existingReviews, reviewBody, event, sentinel) {
@@ -313,6 +310,7 @@ func main() {
}
}
}
}
// POST new review (first run, or state transition fallthrough)
log.Printf("Posting review (event=%s)...", event)
@@ -322,29 +320,6 @@ func main() {
}
log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login)
// Post-posting escalation: if we just posted APPROVED but a sibling
// from the same user has REQUEST_CHANGES, mark ours as superseded and
// re-post as REQUEST_CHANGES. This handles the first-run case where
// we don't know our login until after posting.
if event == "APPROVED" && *updateExisting && *reviewerName != "" {
reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
if err == nil && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) {
log.Printf("Post-posting escalation: sibling has REQUEST_CHANGES")
// Mark our just-posted review as superseded
commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel)
if err == nil {
supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel)
giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody)
}
// Re-post as REQUEST_CHANGES
_, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody, inlineComments)
if err != nil {
log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err)
} else {
log.Printf("Review escalated to REQUEST_CHANGES")
}
}
}
}
// fetchFileContext fetches the full content of modified files from the PR branch.
@@ -501,26 +476,6 @@ func validateReviewerName(name string) error {
return nil
}
// shouldEscalate checks if any sibling bot review from the same user
// (different sentinel, same token) has REQUEST_CHANGES.
// ownLogin is the bot user login; if empty, escalation check is skipped.
// postedID is excluded from consideration (0 means no exclusion needed).
func shouldEscalate(reviews []gitea.Review, postedID int64, ownLogin, ownSentinel string) bool {
if ownLogin == "" {
return false
}
for _, r := range reviews {
if r.ID == postedID || r.Stale {
continue
}
// Sibling = same user, has a review-bot sentinel, but not OUR sentinel
if r.User.Login == ownLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
return true
}
}
return false
}
// reviewUnchanged checks if an existing review with the same sentinel
// already has identical body and state. Returns true if a re-post would
// produce the same result (skip to preserve conversation threads).
@@ -539,6 +494,46 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string)
return false
}
// hasSharedToken detects if another review-bot role posted under the same
// Gitea user. This indicates misconfiguration where two roles share a token
// instead of having separate Gitea accounts. Returns true if shared token
// detected (caller should skip update-in-place logic to avoid clobbering).
func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool {
ownLogin := ""
for _, r := range reviews {
if strings.Contains(r.Body, ownSentinel) {
ownLogin = r.User.Login
break
}
}
if ownLogin == "" {
return false
}
for _, r := range reviews {
if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
log.Printf("WARNING: shared token detected — another review-bot role (%s) is using the same Gitea user %q. Each role should have its own token/user for proper multi-reviewer blocking.", extractSentinelName(r.Body), ownLogin)
return true
}
}
return false
}
// extractSentinelName pulls the reviewer name from a sentinel comment.
func extractSentinelName(body string) string {
const prefix = "<!-- review-bot:"
const suffix = " -->"
idx := strings.Index(body, prefix)
if idx < 0 {
return "unknown"
}
rest := body[idx+len(prefix):]
end := strings.Index(rest, suffix)
if end < 0 {
return "unknown"
}
return rest[:end]
}
// findOwnReview locates a review matching the given sentinel in its body.
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
for i := range reviews {
+80 -100
View File
@@ -50,106 +50,6 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
return r
}
func TestShouldEscalate(t *testing.T) {
tests := []struct {
name string
reviews []gitea.Review
postedID int64
ownLogin string
ownSentinel string
want bool
}{
{
name: "no reviews",
reviews: nil,
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "sibling same user has REQUEST_CHANGES",
reviews: []gitea.Review{
makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:security -->"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: true,
},
{
name: "sibling different user has REQUEST_CHANGES (should NOT escalate)",
reviews: []gitea.Review{
makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n<!-- review-bot:gpt -->"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "same user REQUEST_CHANGES but stale (should NOT escalate)",
reviews: []gitea.Review{
makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n<!-- review-bot:security -->"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "same user same sentinel (own stale review, should NOT escalate)",
reviews: []gitea.Review{
makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n<!-- review-bot:sonnet -->"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "same user APPROVED sibling (should NOT escalate)",
reviews: []gitea.Review{
makeReview(101, "bot", "APPROVED", false, "good\n<!-- review-bot:security -->"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "human REQUEST_CHANGES no sentinel (should NOT escalate)",
reviews: []gitea.Review{
makeReview(101, "bot", "REQUEST_CHANGES", false, "please fix this"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "skip own posted ID",
reviews: []gitea.Review{
makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n<!-- review-bot:security -->"),
},
postedID: 100,
ownLogin: "bot",
ownSentinel: "<!-- review-bot:sonnet -->",
want: false,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := shouldEscalate(tc.reviews, tc.postedID, tc.ownLogin, tc.ownSentinel)
if got != tc.want {
t.Errorf("shouldEscalate() = %v, want %v", got, tc.want)
}
})
}
}
func TestReviewUnchanged(t *testing.T) {
tests := []struct {
name string
@@ -288,3 +188,83 @@ func TestFindOwnReview(t *testing.T) {
})
}
}
func TestHasSharedToken(t *testing.T) {
tests := []struct {
name string
reviews []gitea.Review
sentinel string
want bool
}{
{
name: "no reviews",
reviews: nil,
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "no own review yet - cannot detect",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "separate users - no shared token",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "shared token detected - same user different sentinels",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
},
{
name: "three roles same user",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
{ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
got := hasSharedToken(tc.reviews, tc.sentinel)
if got != tc.want {
t.Errorf("hasSharedToken() = %v, want %v", got, tc.want)
}
})
}
}
func TestExtractSentinelName(t *testing.T) {
tests := []struct {
body string
want string
}{
{"<!-- review-bot:sonnet --> rest", "sonnet"},
{"<!-- review-bot:security --> rest", "security"},
{"no sentinel here", "unknown"},
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
}
for _, tc := range tests {
got := extractSentinelName(tc.body)
if got != tc.want {
t.Errorf("extractSentinelName(%q) = %q, want %q", tc.body, got, tc.want)
}
}
}
+40 -12
View File
@@ -7,6 +7,7 @@ import (
"bytes"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"log"
@@ -16,6 +17,28 @@ import (
"time"
)
// APIError represents an HTTP error response from the Gitea API.
// It carries the status code so callers can distinguish between
// different failure modes (e.g. 404 vs 500).
type APIError struct {
StatusCode int
Body string
}
func (e *APIError) Error() string {
body := e.Body
if len(body) > 200 {
body = body[:200] + "...(truncated)"
}
return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
}
// IsNotFound reports whether an error is an API 404 response.
func IsNotFound(err error) bool {
var apiErr *APIError
return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
}
// Client interacts with the Gitea API.
// A Client is safe for concurrent use by multiple goroutines.
type Client struct {
@@ -66,7 +89,7 @@ type ReviewComment struct {
// GetPullRequest fetches PR metadata.
func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number)
reqURL := fmt.Sprintf("%s/api/v1/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)
@@ -80,7 +103,7 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number
// GetPullRequestDiff fetches the unified diff for a PR.
func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, owner, repo, number)
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return "", fmt.Errorf("fetch diff: %w", err)
@@ -90,7 +113,7 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
// GetPullRequestFiles fetches the list of files changed in a PR.
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, owner, repo, number)
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return nil, fmt.Errorf("fetch PR files: %w", err)
@@ -104,7 +127,7 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
// GetCommitStatuses fetches CI statuses for a commit SHA.
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, owner, repo, sha)
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha))
body, err := c.doGet(ctx, reqURL)
if err != nil {
return nil, fmt.Errorf("fetch commit statuses: %w", err)
@@ -118,7 +141,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
// GetFileContent fetches a file from the default branch of a repo.
func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, owner, repo, escapePath(filepath))
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath))
body, err := c.doGet(ctx, reqURL)
if err != nil {
return "", fmt.Errorf("fetch file %s: %w", filepath, err)
@@ -128,7 +151,7 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath strin
// GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo.
func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, owner, repo, escapePath(filepath), url.QueryEscape(ref))
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath), url.QueryEscape(ref))
body, err := c.doGet(ctx, reqURL)
if err != nil {
return "", fmt.Errorf("fetch file %s@%s: %w", filepath, ref, err)
@@ -140,7 +163,7 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
// event should be "APPROVED" or "REQUEST_CHANGES".
// comments are optional inline comments attached to specific lines.
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number)
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
payload := struct {
Body string `json:"body"`
@@ -201,7 +224,7 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
body, _ := io.ReadAll(resp.Body)
return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body))
return nil, &APIError{StatusCode: resp.StatusCode, Body: string(body)}
}
return io.ReadAll(resp.Body)
}
@@ -230,9 +253,9 @@ type ContentEntry struct {
func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
var reqURL string
if path == "" {
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, owner, repo)
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
} else {
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path))
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
}
body, err := c.doGet(ctx, reqURL)
if err != nil {
@@ -254,10 +277,15 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
// Try listing as directory first
entries, err := c.ListContents(ctx, owner, repo, path)
if err != nil {
// Might be a file, try fetching directly
// Only fall back to single-file fetch on 404 (path is a file, not a dir).
// Propagate all other errors (auth failures, server errors, rate limits).
if !IsNotFound(err) {
return nil, fmt.Errorf("list contents %q: %w", path, err)
}
// 404 means the path might be a file — try fetching directly
content, fileErr := c.GetFileContent(ctx, owner, repo, path)
if fileErr != nil {
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, err)
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, fileErr)
}
results[path] = content
return results, nil
+97
View File
@@ -3,6 +3,7 @@ package gitea
import (
"context"
"encoding/json"
"errors"
"fmt"
"net/http"
"net/http/httptest"
@@ -505,3 +506,99 @@ func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
t.Fatal("expected error when sentinel not found")
}
}
func TestGetAllFilesInPath_404FallsBackToFile(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch r.URL.Path {
case "/api/v1/repos/owner/repo/contents/README.md":
// Contents API returns 404 for files (not a directory)
w.WriteHeader(http.StatusNotFound)
w.Write([]byte(`{"message":"not found"}`))
case "/api/v1/repos/owner/repo/raw/README.md":
w.Write([]byte("# Hello\n"))
default:
w.WriteHeader(http.StatusNotFound)
w.Write([]byte(`{"message":"not found"}`))
}
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
if err != nil {
t.Fatalf("expected fallback to file on 404, got error: %v", err)
}
if len(files) != 1 {
t.Fatalf("expected 1 file, got %d", len(files))
}
if files["README.md"] != "# Hello\n" {
t.Errorf("unexpected content: %q", files["README.md"])
}
}
func TestGetAllFilesInPath_500Propagates(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// Simulate a server error from ListContents
w.WriteHeader(http.StatusInternalServerError)
w.Write([]byte(`{"message":"internal server error"}`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "somepath")
if err == nil {
t.Fatal("expected error to propagate for 500, got nil")
}
// Should NOT fall back to file fetch — error should propagate
var apiErr *APIError
if !errors.As(err, &apiErr) {
t.Fatalf("expected APIError in chain, got: %v", err)
}
if apiErr.StatusCode != http.StatusInternalServerError {
t.Errorf("expected status 500, got %d", apiErr.StatusCode)
}
}
func TestGetAllFilesInPath_403Propagates(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusForbidden)
w.Write([]byte(`{"message":"token has insufficient scope"}`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "private/stuff")
if err == nil {
t.Fatal("expected error to propagate for 403, got nil")
}
var apiErr *APIError
if !errors.As(err, &apiErr) {
t.Fatalf("expected APIError in chain, got: %v", err)
}
if apiErr.StatusCode != http.StatusForbidden {
t.Errorf("expected status 403, got %d", apiErr.StatusCode)
}
}
func TestIsNotFound(t *testing.T) {
tests := []struct {
name string
err error
want bool
}{
{"nil error", nil, false},
{"non-API error", fmt.Errorf("network timeout"), false},
{"404 APIError", &APIError{StatusCode: 404, Body: "not found"}, true},
{"500 APIError", &APIError{StatusCode: 500, Body: "server error"}, false},
{"wrapped 404", fmt.Errorf("list contents: %w", &APIError{StatusCode: 404, Body: "not found"}), true},
{"wrapped 500", fmt.Errorf("list contents: %w", &APIError{StatusCode: 500, Body: "err"}), false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
got := IsNotFound(tt.err)
if got != tt.want {
t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want)
}
})
}
}