fix: handle npm registry URLs in update-tap SHA256 calculation#57
fix: handle npm registry URLs in update-tap SHA256 calculation#57Yan Xue (yanxue06) wants to merge 1 commit intomainfrom
Conversation
The get_sha256 function only handled GitHub and gitgate URLs, but the npm formula type passes npm registry URLs (registry.npmjs.org). These URLs were incorrectly parsed as GitHub URLs, causing gh release download to fail with garbage repo/tag/asset values. Added an npm registry URL branch that uses curl to download the tarball directly before computing the SHA256. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughA GitHub Actions workflow file is updated to add special-case handling in a SHA-256 computation helper function. When processing npm registry URLs, the function now downloads tarballs directly via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/blocks/update-tap/action.yaml (1)
82-89: Implementation looks correct for npm registry URLs.The logic properly detects npm URLs, downloads the tarball directly via
curl, computes the SHA256, and returns early. The curl flags (-sSfL) are appropriate.One consideration: if
curlfails (e.g., 404 or network error), the function will return an empty SHA256, which would then be written to the formula. This is consistent with the existing behavior forgh release downloadfailures, but you may want to add error checking in a follow-up.🛡️ Optional: Add error handling for download failures
if echo "$url" | grep -q "registry.npmjs.org"; then # NPM registry URL: download tarball directly local filename=$(basename "$url") - curl -sSfL "$url" -o "$tmpdir/$filename" + if ! curl -sSfL "$url" -o "$tmpdir/$filename"; then + echo "::error::Failed to download $url" + rm -rf "$tmpdir" + return 1 + fi shasum -a 256 "$tmpdir/$filename" | cut -d' ' -f1 rm -rf "$tmpdir" return fiThis would require the caller to check the return value, which would be a larger refactor to apply consistently to all download paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/blocks/update-tap/action.yaml around lines 82 - 89, The npm-registry branch detects registry.npmjs.org and downloads the tarball with curl but doesn't handle curl failure, which lets an empty SHA256 be returned; after the curl call (curl -sSfL "$url" -o "$tmpdir/$filename") check its exit status and, on failure, emit an error/log and exit/return non-zero (or otherwise abort the function) instead of proceeding to shasum and returning an empty value; ensure the same branch cleans up $tmpdir on error and does not write a blank checksum.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/blocks/update-tap/action.yaml:
- Around line 82-89: The npm-registry branch detects registry.npmjs.org and
downloads the tarball with curl but doesn't handle curl failure, which lets an
empty SHA256 be returned; after the curl call (curl -sSfL "$url" -o
"$tmpdir/$filename") check its exit status and, on failure, emit an error/log
and exit/return non-zero (or otherwise abort the function) instead of proceeding
to shasum and returning an empty value; ensure the same branch cleans up $tmpdir
on error and does not write a blank checksum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 443b6f39-0a92-484e-adb2-dd0835f13c5b
📒 Files selected for processing (1)
.github/blocks/update-tap/action.yaml
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
There was a problem hiding this comment.
Pull request overview
This PR updates the update-tap composite action to correctly compute SHA256 for Homebrew formulas whose source URLs point at the npm registry, avoiding incorrect GitHub URL parsing.
Changes:
- Detect
registry.npmjs.orgURLs inget_sha256()and download tarballs directly viacurlbefore hashing. - Preserve existing GitHub release /
gitgate.internalhandling paths for non-npm URLs.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| local tmpdir=$(mktemp -d) | ||
|
|
||
| if echo "$url" | grep -q "registry.npmjs.org"; then | ||
| # NPM registry URL: download tarball directly | ||
| local filename=$(basename "$url") |
There was a problem hiding this comment.
tmpdir cleanup is manual and only happens on the success path. With shell: bash (runs with -e), any failure in curl/shasum will exit and leave the temp directory behind. Consider adding a trap 'rm -rf "$tmpdir"' RETURN right after mktemp -d to guarantee cleanup.
| if echo "$url" | grep -q "registry.npmjs.org"; then | ||
| # NPM registry URL: download tarball directly | ||
| local filename=$(basename "$url") | ||
| curl -sSfL "$url" -o "$tmpdir/$filename" | ||
| shasum -a 256 "$tmpdir/$filename" | cut -d' ' -f1 |
There was a problem hiding this comment.
This conditional adds an npm special-case, but the fallback still implicitly assumes a GitHub release download URL for everything else. In this action, get_sha256 is also used with GitHub tag tarballs (.../archive/refs/tags/...), which will be mis-parsed and cause gh release download to fail. Consider gating the gh release download path on URLs that actually contain /releases/download/, and using a direct download (curl, possibly with auth) for other GitHub URLs.
Summary
get_sha256()function inupdate-taponly handles GitHub release URLs and gitgate URLshttps://registry.npmjs.org/foo/-/foo-1.0.0.tgz)repo,tag, andasset, causinggh release downloadto failcurlto download the tarball directlyWhat changed
update-tap/action.yaml: Added npm registry URL detection inget_sha256()that usescurlinstead ofgh release downloadTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit