feat(action): derive binary name from uname for multi-arch support (#124) #127
sonnet-review-bot
security-review-bot
gpt-review-bot
aweiker
Reference in New Issue
Block a user
Delete Branch "issue-124"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements #124 — derive binary name from runner platform using
unameinstead of hard-codinglinux-amd64.Problem
The composite action hard-coded
review-bot-linux-amd64in both the cache key and the Install step. On arm64 runners (Graviton, Apple Silicon) or macOS, the action would attempt to download a binary that does not match the runner architecture.Changes
Determine versionstep (new)uname -s→linux/darwinuname -m→amd64(x86_64) /arm64(aarch64 or arm64)casestatement; unsupported OS or arch exits with a clear error messageosandarchas step outputs alongsideversionCache review-bot binarystepreview-bot-{os}-{arch}-{version}(wasreview-bot-linux-amd64-{version})Install review-botstepBINARYis nowreview-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}grepanchored to exact filename (start+end of line)sha256sumon linux,shasum -a 256on darwin (macOS does not ship GNU sha256sum)Supported platforms
Matches what the release workflow builds:
linux-amd64linux-arm64darwin-amd64darwin-arm64Progress
Phase 1: Detection + outputs + cache key + Install step
os,archoutputs from Determine version stepsteps.version.outputs.osandsteps.version.outputs.archlinux-amd64hard-codePhase 2: Review + fixes
sha256sumnot on macOS → fixed with OS-conditional^f8b9d7d)f8b9d7dPreviously the Install step hard-coded 'review-bot-linux-amd64'. This fails on arm64 runners (Graviton, Apple Silicon) where uname -m returns 'aarch64' or 'arm64'. Changes: - Add OS/arch detection in 'Determine version' step using uname -s/-m - Map uname output to asset name format: x86_64→amd64, aarch64/arm64→arm64, linux→linux, darwin→darwin - Emit 'os' and 'arch' as step outputs alongside 'version' - Update cache key: review-bot-{os}-{arch}-{version} - Update Install step: BINARY derived from step outputs - Anchor checksum grep to exact filename (not substring match) - Unsupported OS or arch exits with a clear error message Supported platforms: linux-amd64, linux-arm64, darwin-amd64, darwin-arm64 (matches what the release workflow builds)Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
7a8fc166)Sonnet Review
Summary
This PR correctly implements multi-architecture support for the composite action by detecting OS and arch via
unameand using the results in the cache key, binary name, and checksum verification. The implementation is clean, handles the aarch64/arm64 normalization correctly, and the checksum grep is now properly anchored to prevent substring matches. CI passes.Findings
.gitea/actions/review/action.yml$(uname -s)(uppercase) after already computing `OS_RAW=$(uname -s.gitea/actions/review/action.yml[[:xdigit:]]+[[:space:]]+\*?${BINARY}$does not anchor the hex portion to the start of the line (^). A line likesomejunk abc123 review-bot-linux-arm64would not match (awk would still grab field 1), but the intent of exact matching is clearer with^[[:xdigit:]]. Minor defensive concern only.Recommendation
APPROVE — Approve. The core logic is correct: OS detection normalises to lowercase before the case statement, ARM arch aliases (aarch64 and arm64) are both handled, cache key is properly parameterised, and the checksum grep is anchored to the filename end to prevent prefix-match false positives. The two nit-level findings are cosmetic and do not affect correctness or security. No changes required.
Review by sonnet
Evaluated against
7a8fc166@@ -127,0 +128,4 @@# Detect OS and architecture for platform-specific binary downloadOS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')case "$OS_RAW" inlinux) OS="linux" ;;[NIT] The error message for unsupported OS re-invokes
$(uname -s)(uppercase) after already computingOS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]'). This is a minor inconsistency — using$OS_RAW(before lowercasing) or the original$(uname -s)output is equivalent in practice, but it means a second subprocess is forked. Cosmetic only.@@ -149,7 +173,7 @@ runs:# Verify SHA-256 checksum[NIT] The checksum regex
[[:xdigit:]]+[[:space:]]+\*?${BINARY}$does not anchor the hex portion to the start of the line (^). A line likesomejunk abc123 review-bot-linux-arm64would not match (awk would still grab field 1), but the intent of exact matching is clearer with^[[:xdigit:]]. Minor defensive concern only.Self-Review: PR #127
Self-review against
7a8fc166ecPhase 1: Independent Findings
No blocking findings. One style note:
echo "Error: unsupported OS: $(uname -s)"callsuname -sa second time instead of using$OS_RAW. Not a bug — just a minor inefficiency. Not worth fixing.Phase 2: Prior Review Verification
No prior reviews yet (CI just started). This is a fresh PR; no prior findings to verify.
Checklist verification:
uname -slowercased viatr '[:upper:]' '[:lower:]'Linux→linux,Darwin→darwinlinux/darwin, exit 1 for othersuname -mnormalized:x86_64→amd64,aarch64/arm64→arm64|OR*case with clear error messageosandarchemitted as step outputsecho "os=..."andecho "arch=..."presentreview-bot-{os}-{arch}-{version}linux-amd64hard-codeBINARYuses step outputs (not re-derived)review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}-E "[[:xdigit:]]+[[:space:]]+\*?${BINARY}$"is correct EREuname -s→linux,uname -m→amd64, BINARY=review-bot-linux-amd64linux-amd64hard-code remaininggrepNotes:
aarch64 | arm64)in case: POSIX-valid OR pattern — handles both Linux and macOS ARM naming. ✅${{ steps.version.outputs.os }}in composite actionrun:step: valid — Actions runner evaluates expressions before shell executes. ✅set -euo pipefailnot present: pre-existing condition from base branch (PR #121 adds it but is unmerged). Not introduced by this PR.Assessment: ✅ Clean
The changes correctly implement multi-arch binary download. OS and arch are detected via
uname, validated with case statements that exit on unsupported platforms, emitted as step outputs, and consumed in the cache key and binary name. The existing linux-amd64 path is unaffected. No correctness issues found.Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
7a8fc166)Security Review
Summary
The changes correctly derive and validate OS/architecture and use them consistently in the cache key, binary selection, and checksum verification. The added platform detection is safely constrained and does not introduce new security risks.
Recommendation
APPROVE — This update hardens platform handling and improves checksum matching without introducing security vulnerabilities. The case-based normalization of uname values prevents unexpected strings from propagating, and anchoring the checksum grep reduces ambiguity. Consider, in future iterations, ensuring any values written to $GITHUB_OUTPUT are robust against edge cases (e.g., newline handling) and that inputs like gitea-url/repo are treated as trusted configuration to avoid supply chain risks. Overall, this PR is good to merge.
Review by security
Evaluated against
7a8fc166Original reviewSuperseded — see current review for up-to-date findings.
Previous findings (commit
7a8fc166)Gpt Review
Summary
Good multi-arch detection and cache key updates. However, the checksum verification uses sha256sum, which is not available by default on macOS, so the action will fail on darwin runners despite claiming darwin support.
Findings
.gitea/actions/review/action.ymlsha256sum, which is not present by default on macOS. On darwin runners, `ACTUAL=$(sha256sum review-bot.gitea/actions/review/action.yml^as well to reduce any risk of matching unintended lines if the checksums format changes.Recommendation
REQUEST_CHANGES — The overall approach to derive OS/arch via uname and propagate them to the cache key and binary name looks correct and should work on linux-amd64 and linux-arm64. To fully support darwin as intended, update the checksum computation to be portable across platforms. For example:
Detect OS and choose the appropriate tool:
ACTUAL=$(sha256sum review-bot | awk '{print $1}')ACTUAL=$(shasum -a 256 review-bot | awk '{print $1}')Or implement a small conditional based on
${{ steps.version.outputs.os }}inside the install step to select the correct command.Additionally, consider tightening the checksum grep by anchoring the start of the line, e.g.
EXPECTED=$(grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}').Once these are addressed, the action should function as intended on linux and darwin across amd64/arm64 while maintaining integrity checks and correct caching.
Review by gpt
Evaluated against
7a8fc166@@ -149,7 +173,7 @@ runs:# Verify SHA-256 checksumcd "${{ runner.temp }}"[MINOR] The checksum grep pattern anchors the end of the line but not the start. Consider anchoring with
^as well to reduce any risk of matching unintended lines if the checksums format changes.@@ -150,3 +174,3 @@# Verify SHA-256 checksumcd "${{ runner.temp }}"EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')EXPECTED=$(grep -E "[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')[MAJOR] Checksum computation uses
sha256sum, which is not present by default on macOS. On darwin runners,ACTUAL=$(sha256sum review-bot | awk '{print $1}')will fail, breaking the install step on a supported platform.Fix Plan against
7a8fc166ec:Summary
2 findings from gpt-review-bot. Both are FIX.
Finding #1 [MAJOR] —
sha256sumnot available on macOSAction: FIX
sha256sumis a GNU coreutils command not present by default on macOS. macOS usesshasum -a 256. Since we already have${{ steps.version.outputs.os }}in scope, use a conditional in the Install step:This keeps it simple and explicit — no subshell-conditional gymnastics.
Finding #2 [MINOR] — Checksum grep not anchored at start
Action: FIX
Change
grep -E "[[:xdigit:]]+[[:space:]]+\*?${BINARY}$"togrep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$"to anchor the start of line.Changes
sha256sumvsshasumconditional based onsteps.version.outputs.os^anchor at start of ERE patternResponses to gpt-review-bot findings (
f8b9d7d)Fixed (2 items)
sha256sumnot available on macOSif [ "${{ steps.version.outputs.os }}" = "darwin" ]conditional to useshasum -a 256on darwin,sha256sumon linux^anchor:grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$"Sonnet Review
Summary
This PR correctly implements multi-arch support for the composite action by detecting OS and architecture via
unameand emitting them as step outputs. The logic is clean, the error handling is explicit, and the checksum verification correctly accounts for the macOS vs Linuxsha256sumdifference. CI passed.Findings
.gitea/actions/review/action.ymluname -soutput viatrintoOS_RAW, then matcheslinuxanddarwinand assigns identical strings. Thetrandcasetogether are correct but slightly redundant — you could matchLinuxandDarwindirectly withouttr. Not a bug, just minor ceremony. Current approach is defensively safe and readable..gitea/actions/review/action.yml$(uname -s)(uppercased original) rather than printing$OS_RAW(already computed). This is intentional and arguably better since it shows the raw value the user's system returned, but it adds a secondunamefork. Cosmetic only.Recommendation
APPROVE — Approve. The implementation is correct and complete. OS and arch detection handles all documented platforms (linux-amd64, linux-arm64, darwin-amd64, darwin-arm64), the cache key is properly namespaced per architecture, the checksum grep is anchored to avoid false matches on similarly-named binaries, and the darwin/linux
sha256sumdivergence is handled correctly. The two NIT findings are cosmetic and do not affect correctness or safety.Review by sonnet
Evaluated against
f8b9d7d2@@ -125,2 +125,4 @@VERSION="${{ inputs.version }}"fi# Detect OS and architecture for platform-specific binary download[NIT] The OS case statement lowercases
uname -soutput viatrintoOS_RAW, then matcheslinuxanddarwinand assigns identical strings. Thetrandcasetogether are correct but slightly redundant — you could matchLinuxandDarwindirectly withouttr. Not a bug, just minor ceremony. Current approach is defensively safe and readable.@@ -127,0 +132,4 @@darwin) OS="darwin" ;;*)echo "Error: unsupported OS: $(uname -s)" >&2exit 1[NIT] The error message for unsupported OS re-runs
$(uname -s)(uppercased original) rather than printing$OS_RAW(already computed). This is intentional and arguably better since it shows the raw value the user's system returned, but it adds a secondunamefork. Cosmetic only.Security Review
Summary
The changes correctly detect OS/arch at runtime, update cache keys and binary selection, and improve checksum verification anchoring. No new security vulnerabilities are introduced, and CI has passed.
Findings
.gitea/actions/review/action.ymlRecommendation
APPROVE — Approve the PR. The platform detection and cache/binary derivation are implemented safely, and the anchored checksum grep reduces risk of matching incorrect files. As a hardening measure, consider constraining or validating the gitea-url/repo inputs (e.g., allowlist trusted origins and enforce HTTPS) and verifying checksums via a trusted signature or pre-known hash to mitigate supply chain risks when downloading and executing remote binaries.
Review by security
Evaluated against
f8b9d7d2@@ -149,8 +173,13 @@ runs:[MINOR] Integrity check relies on checksums.txt fetched from the same remote as the binary (derived from user-controllable inputs gitea-url/repo). A malicious input could point to an attacker-controlled host serving both a forged binary and matching checksum, defeating the verification. Consider pinning allowed hosts/repos, enforcing HTTPS, and verifying signed checksums or comparing hashes against a trusted source.
Gpt Review
Summary
The changes correctly generalize the binary download to the host platform by detecting OS and architecture via uname, update the cache key accordingly, and strengthen checksum matching. The implementation is sound and aligns with the stated supported platforms.
Recommendation
APPROVE — CI has passed and the PR addresses the multi-arch issue cleanly. The OS/arch detection logic covers the supported matrices (linux/darwin with amd64/arm64), cache keys are architecture-specific to avoid cross-arch collisions, and checksum verification is more robust by anchoring the filename and accommodating macOS shasum. Consider, as a non-blocking follow-up, adding a brief log when the cache is hit and optionally using set -euo pipefail at the top of shell blocks for stricter error handling. Otherwise, this is ready to merge.
Review by gpt
Evaluated against
f8b9d7d2Self-Review: PR #127
Self-review against
f8b9d7d282Phase 1: Independent Findings
Diff from
7a8fc16tof8b9d7d(the fix commit):shasum -a 256on darwin,sha256sumon linux. This is correct — GNUsha256sumis not present on macOS.^anchor to thegrep -Epattern for checksums. Correct improvement.${{ steps.version.outputs.os }}which is evaluated by Actions runner before the shell, so it will expand to "linux" or "darwin" literally. ✅No new findings.
Phase 2: Prior Review Verification
sha256sumnot on macOS [MAJOR]shasum -a 256on darwin^added to ERE patternAssessment: ✅ Clean
Both findings from gpt-review-bot addressed correctly. CI passes. All bot reviews APPROVED on this HEAD. The PR is ready for human review.