rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

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

Merged
rodin merged 3 commits from fix/8-10-error-handling-idempotent-upload into main 2026-05-02 17:07:23 +00:00
Conversation 5 Commits 3 Files Changed 3
+3 -3
2 changed files with 3 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit bfca28b2b2 - Show all commits
.gitea/workflows/release.yml
+2 -2
View File
@@ -75,10 +75,10 @@ jobs:
echo "Uploading ${filename}..."
# Check if asset already exists and delete it
sonnet-review-bot commented 2026-05-02 16:52:20 +00:00
Review
Copy Link
Copy Source

[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.
EXISTING_ID=$(curl -sS \
EXISTING_ID=$(export ASSET_NAME="${filename}"; curl -sS \
security-review-bot commented 2026-05-02 17:00:40 +00:00
Review
Copy Link
Copy Source

[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.
-H "Authorization: token ${GITEA_TOKEN}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets" \
| python3 -c "import json,sys; assets=json.load(sys.stdin); print(next((str(a['id']) for a in assets if a['name']=='${filename}'),''))" 2>/dev/null)
| 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..."
gitea/client.go
+1 -1
View File
1
 @@ -281,7 +281,7 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
// 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)
sonnet-review-bot commented 2026-05-02 16:52:20 +00:00
Outdated
Review
Copy Link
Copy Source

[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.
}
results[path] = content
return results, nil