fix: distinguish 404 in GetAllFilesInPath, make uploads idempotent #33

2026-05-02T16:50:57Z

rodin commented

2026-05-02 16:50:57 +00:00

Summary

Fixes two issues:

Issue #8: Distinguish 404 from other errors in GetAllFilesInPath

Added APIError type with StatusCode field to carry HTTP status codes from Gitea API responses
Added IsNotFound helper function for ergonomic 404 checks
GetAllFilesInPath now only falls back to single-file fetch when the error is a 404 (path is a file, not a directory)
All other errors (auth failures, 500s, rate limits) are properly propagated instead of being masked

Issue #10: Make release asset uploads idempotent

Release workflow now checks for existing assets with the same name before uploading
If an asset already exists, it is deleted first, then re-uploaded
This makes workflow re-runs safe — no more failures from duplicate asset names

Tests

TestGetAllFilesInPath_404FallsBackToFile — verifies 404 triggers file fallback
TestGetAllFilesInPath_500Propagates — verifies 500 is propagated (no fallback)
TestGetAllFilesInPath_403Propagates — verifies 403 is propagated (no fallback)
TestIsNotFound — table test for the helper function

Closes #8
Closes #10

## Summary Fixes two issues: ### Issue #8: Distinguish 404 from other errors in GetAllFilesInPath - Added `APIError` type with `StatusCode` field to carry HTTP status codes from Gitea API responses - Added `IsNotFound` helper function for ergonomic 404 checks - `GetAllFilesInPath` now only falls back to single-file fetch when the error is a 404 (path is a file, not a directory) - All other errors (auth failures, 500s, rate limits) are properly propagated instead of being masked ### Issue #10: Make release asset uploads idempotent - Release workflow now checks for existing assets with the same name before uploading - If an asset already exists, it is deleted first, then re-uploaded - This makes workflow re-runs safe — no more failures from duplicate asset names ### Tests - `TestGetAllFilesInPath_404FallsBackToFile` — verifies 404 triggers file fallback - `TestGetAllFilesInPath_500Propagates` — verifies 500 is propagated (no fallback) - `TestGetAllFilesInPath_403Propagates` — verifies 403 is propagated (no fallback) - `TestIsNotFound` — table test for the helper function Closes #8 Closes #10

rodin added 1 commit 2026-05-02 16:50:57 +00:00

fix: distinguish 404 in GetAllFilesInPath, make uploads idempotent

CI / test (pull_request) Successful in 14s

Details

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 58s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m7s

Details

f047c994bf

- 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

gpt-review-bot approved these changes 2026-05-02 16:51:35 +00:00

gpt-review-bot left a comment

First-time contributor

Gpt Review

Summary

This change robustly addresses 404 handling and release asset idempotency, introducing clear error typing and strong test coverage. Implementation is idiomatic and aligns with Go and repo conventions.

Recommendation

APPROVE — The update meaningfully improves error distinction in Gitea API handling by adding an idiomatic APIError type with status code propagation, and leverages this to prevent fallback-to-file for non-404 errors in GetAllFilesInPath. The new IsNotFound helper provides a clear ergonomic API for checking 404s, and the workflow improvement ensures release asset uploads are idempotent. Unit test coverage is comprehensive and table-driven as per project guidelines. All changes are well-designed, readable, and conform to standard Go error handling and style. No issues found—merge is recommended.

Review by gpt

# Gpt Review ## Summary This change robustly addresses 404 handling and release asset idempotency, introducing clear error typing and strong test coverage. Implementation is idiomatic and aligns with Go and repo conventions. ## Recommendation **APPROVE** — The update meaningfully improves error distinction in Gitea API handling by adding an idiomatic APIError type with status code propagation, and leverages this to prevent fallback-to-file for non-404 errors in GetAllFilesInPath. The new IsNotFound helper provides a clear ergonomic API for checking 404s, and the workflow improvement ensures release asset uploads are idempotent. Unit test coverage is comprehensive and table-driven as per project guidelines. All changes are well-designed, readable, and conform to standard Go error handling and style. No issues found—merge is recommended. --- *Review by gpt*

security-review-bot approved these changes 2026-05-02 16:52:11 +00:00

Dismissed

security-review-bot left a comment

Security Review

Summary

Changes introduce structured HTTP error handling (APIError) and a precise 404 check for safe fallback logic, plus idempotent asset uploads in the release workflow. CI has passed and no exploitable security issues are evident in the diff.

Recommendation

APPROVE — The introduction of APIError and IsNotFound improves error semantics without expanding the attack surface, and the release workflow’s pre-delete step makes uploads idempotent without exposing secrets or enabling injection. Inputs to API calls continue to be safely escaped, timeouts are in place, and no sensitive data is logged beyond server responses as before. Consider hardening the release asset lookup to robustly handle unusual filenames and HTTP failures, but this is not security-critical. Overall, the changes are safe to merge.

Review by security

# Security Review ## Summary Changes introduce structured HTTP error handling (APIError) and a precise 404 check for safe fallback logic, plus idempotent asset uploads in the release workflow. CI has passed and no exploitable security issues are evident in the diff. ## Recommendation **APPROVE** — The introduction of APIError and IsNotFound improves error semantics without expanding the attack surface, and the release workflow’s pre-delete step makes uploads idempotent without exposing secrets or enabling injection. Inputs to API calls continue to be safely escaped, timeouts are in place, and no sensitive data is logged beyond server responses as before. Consider hardening the release asset lookup to robustly handle unusual filenames and HTTP failures, but this is not security-critical. Overall, the changes are safe to merge. --- *Review by security*

sonnet-review-bot approved these changes 2026-05-02 16:52:20 +00:00

sonnet-review-bot left a comment

First-time contributor

Sonnet Review

Summary

The changes cleanly introduce a structured APIError with an IsNotFound helper and correctly adjust GetAllFilesInPath to only fall back on 404s, improving error handling. The release workflow gains idempotent asset uploads. Tests comprehensively cover the new behavior, and CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	`.gitea/workflows/release.yml`	86	The asset upload now uses jq to URI-encode filenames. This adds an extra dependency that may not be present in all runners. Since python3 is already used earlier in the job, consider using Python's urllib.parse.quote to avoid relying on jq.
2	[NIT]	`.gitea/workflows/release.yml`	82	On deleting existing assets, curl is invoked with -sSf and will fail the job on non-2xx responses (e.g., transient 404/race). If you prefer the workflow to proceed in such cases, consider handling 404s gracefully or ignoring DELETE failures when the asset disappears between list and delete.

Recommendation

APPROVE — Overall the implementation is solid and adheres to idiomatic Go error handling patterns. The new APIError type and IsNotFound helper allow precise control flow in GetAllFilesInPath, and the fix to wrap the correct error on file fallback is correct. Tests validate the new behavior for 404/403/500 conditions and IsNotFound. The release workflow improvements make uploads idempotent; consider replacing the jq usage with Python’s urllib.parse.quote to avoid an extra dependency and optionally make DELETE failures non-fatal if desired. With CI passing and no major issues found, this is good to merge.

Review by sonnet

# Sonnet Review ## Summary The changes cleanly introduce a structured APIError with an IsNotFound helper and correctly adjust GetAllFilesInPath to only fall back on 404s, improving error handling. The release workflow gains idempotent asset uploads. Tests comprehensively cover the new behavior, and CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `.gitea/workflows/release.yml` | 86 | The asset upload now uses jq to URI-encode filenames. This adds an extra dependency that may not be present in all runners. Since python3 is already used earlier in the job, consider using Python's urllib.parse.quote to avoid relying on jq. | | 2 | [NIT] | `.gitea/workflows/release.yml` | 82 | On deleting existing assets, curl is invoked with -sSf and will fail the job on non-2xx responses (e.g., transient 404/race). If you prefer the workflow to proceed in such cases, consider handling 404s gracefully or ignoring DELETE failures when the asset disappears between list and delete. | ## Recommendation **APPROVE** — Overall the implementation is solid and adheres to idiomatic Go error handling patterns. The new APIError type and IsNotFound helper allow precise control flow in GetAllFilesInPath, and the fix to wrap the correct error on file fallback is correct. Tests validate the new behavior for 404/403/500 conditions and IsNotFound. The release workflow improvements make uploads idempotent; consider replacing the jq usage with Python’s urllib.parse.quote to avoid an extra dependency and optionally make DELETE failures non-fatal if desired. With CI passing and no major issues found, this is good to merge. --- *Review by sonnet*

.gitea/workflows/release.yml

						
				@@ -74,2 +74,4 @@

				            filename=$(basename "$file")

				            echo "Uploading ${filename}..."

				            # Check if asset already exists and delete it

sonnet-review-bot commented

2026-05-02 16:52:20 +00:00

First-time contributor

[NIT] The inline Python filter compares asset names using a shell-expanded ${filename} inside single quotes. If a filename ever contained a single quote, this would break the command. While current artifact names are safe, consider more robust quoting or using a tool like jq.

**[NIT]** The inline Python filter compares asset names using a shell-expanded ${filename} inside single quotes. If a filename ever contained a single quote, this would break the command. While current artifact names are safe, consider more robust quoting or using a tool like jq.

gitea/client.go Outdated

						
				@@ -258,3 +281,4 @@

						// 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)

sonnet-review-bot commented

2026-05-02 16:52:20 +00:00

First-time contributor

[MINOR] In GetAllFilesInPath, when GetFileContent fails, the returned error wraps the earlier ListContents error (err) instead of the actual fileErr. This loses the specific cause of the file fetch failure. Prefer wrapping fileErr for better diagnostics.

**[MINOR]** In GetAllFilesInPath, when GetFileContent fails, the returned error wraps the earlier ListContents error (err) instead of the actual fileErr. This loses the specific cause of the file fetch failure. Prefer wrapping fileErr for better diagnostics.

rodin referenced this issue from a commit

2026-05-02 16:58:43 +00:00

fix: address review findings from PR #33

rodin added 1 commit 2026-05-02 16:58:43 +00:00

fix: address review findings from PR #33

CI / test (pull_request) Successful in 13s

Details

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 22s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m9s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m42s

Details

bfca28b2b2

- Wrap fileErr instead of err in GetAllFilesInPath fallback (MINOR)
- Use env var for asset name in release workflow to avoid quoting issues (NIT)

security-review-bot approved these changes 2026-05-02 17:00:40 +00:00

Dismissed

security-review-bot left a comment

Security Review

Summary

Security-focused changes look sound: API errors now preserve status codes to prevent masking non-404 failures, and the release workflow’s asset upload is made idempotent without exposing secrets. CI passed.

Findings

#	Severity	File	Line	Finding
1	[MINOR]	`.gitea/workflows/release.yml`	78	The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter.
2	[NIT]	`gitea/client.go`	20	APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details.

Recommendation

APPROVE — The changes correctly distinguish 404s using a typed APIError and prevent masking other HTTP failures, improving security posture around error handling. The idempotent release upload logic avoids duplicate asset errors without exposing the token in logs. To harden further, URL-encode the asset filename when constructing the upload URL in the release workflow, and consider truncating response bodies in APIError messages if there’s any risk of verbose or sensitive content being logged. Otherwise, the PR is safe to merge.

Review by security

# Security Review ## Summary Security-focused changes look sound: API errors now preserve status codes to prevent masking non-404 failures, and the release workflow’s asset upload is made idempotent without exposing secrets. CI passed. ## Findings | # | Severity | File | Line | Finding | |---|----------|------|------|--------| | 1 | [MINOR] | `.gitea/workflows/release.yml` | 78 | The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter. | | 2 | [NIT] | `gitea/client.go` | 20 | APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details. | ## Recommendation **APPROVE** — The changes correctly distinguish 404s using a typed APIError and prevent masking other HTTP failures, improving security posture around error handling. The idempotent release upload logic avoids duplicate asset errors without exposing the token in logs. To harden further, URL-encode the asset filename when constructing the upload URL in the release workflow, and consider truncating response bodies in APIError messages if there’s any risk of verbose or sensitive content being logged. Otherwise, the PR is safe to merge. --- *Review by security*

.gitea/workflows/release.yml

						
				@@ -75,1 +75,4 @@

				            echo "Uploading ${filename}..."

				            # Check if asset already exists and delete it

				            EXISTING_ID=$(export ASSET_NAME="${filename}"; curl -sS \

security-review-bot commented

2026-05-02 17:00:40 +00:00

[MINOR] The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter.

**[MINOR]** The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter.

gitea/client.go

						
				@@ -16,6 +17,24 @@ import (

					"time"

				)

				// APIError represents an HTTP error response from the Gitea API.

security-review-bot commented

2026-05-02 17:00:40 +00:00

[NIT] APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details.

**[NIT]** APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details.

rodin added 1 commit 2026-05-02 17:02:06 +00:00

fix: URL-encode asset filename, truncate error body in APIError

CI / review (gpt-4.1, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 23s

Details

CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 51s

Details

CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m21s

Details

CI / test (pull_request) Successful in 14s

Details

2339999d37

- 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)

security-review-bot approved these changes 2026-05-02 17:03:12 +00:00

security-review-bot left a comment

Security Review

Summary

Changes introduce a typed API error with status codes and improve idempotency of release asset uploads. No security vulnerabilities are apparent; the updates generally improve error handling and robustness.

Recommendation

APPROVE — CI passed and the modifications are security-neutral or improving: APIError enables precise 404 handling without masking other errors, and the release workflow now safely URL-encodes asset names and deletes any existing asset before upload. No exploitable issues were identified. Consider continuing to limit sensitive information in error messages and ensure the release environment has required tools (jq, python3) installed, but these are non-security concerns.

Review by security

# Security Review ## Summary Changes introduce a typed API error with status codes and improve idempotency of release asset uploads. No security vulnerabilities are apparent; the updates generally improve error handling and robustness. ## Recommendation **APPROVE** — CI passed and the modifications are security-neutral or improving: APIError enables precise 404 handling without masking other errors, and the release workflow now safely URL-encodes asset names and deletes any existing asset before upload. No exploitable issues were identified. Consider continuing to limit sensitive information in error messages and ensure the release environment has required tools (jq, python3) installed, but these are non-security concerns. --- *Review by security*

rodin merged commit d640eb6e71 into main

2026-05-02 17:07:23 +00:00

rodin deleted branch fix/8-10-error-handling-idempotent-upload

2026-05-02 17:07:23 +00:00

Sign in to join this conversation.

No Reviewers

gpt-review-bot

sonnet-review-bot

security-review-bot

4 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rodin/review-bot#33