feat(action): derive binary name from uname for multi-arch support (#124) #127
@@ -124,14 +124,38 @@ runs:
|
||||
else
|
||||
VERSION="${{ inputs.version }}"
|
||||
fi
|
||||
|
||||
# Detect OS and architecture for platform-specific binary download
|
||||
|
|
||||
OS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')
|
||||
case "$OS_RAW" in
|
||||
linux) OS="linux" ;;
|
||||
sonnet-review-bot
commented
[NIT] The error message for unsupported OS re-invokes **[NIT]** The error message for unsupported OS re-invokes `$(uname -s)` (uppercase) after already computing `OS_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.
|
||||
darwin) OS="darwin" ;;
|
||||
*)
|
||||
echo "Error: unsupported OS: $(uname -s)" >&2
|
||||
exit 1
|
||||
|
sonnet-review-bot
commented
[NIT] The error message for unsupported OS re-runs **[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 second `uname` fork. Cosmetic only.
|
||||
;;
|
||||
esac
|
||||
|
||||
RAW_ARCH=$(uname -m)
|
||||
case "$RAW_ARCH" in
|
||||
x86_64) ARCH="amd64" ;;
|
||||
aarch64 | arm64) ARCH="arm64" ;;
|
||||
*)
|
||||
echo "Error: unsupported architecture: $RAW_ARCH" >&2
|
||||
exit 1
|
||||
;;
|
||||
esac
|
||||
|
||||
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
|
||||
echo "os=${OS}" >> "$GITHUB_OUTPUT"
|
||||
echo "arch=${ARCH}" >> "$GITHUB_OUTPUT"
|
||||
|
||||
- name: Cache review-bot binary
|
||||
id: cache
|
||||
uses: actions/cache@v4
|
||||
with:
|
||||
path: ${{ runner.temp }}/review-bot
|
||||
key: review-bot-linux-amd64-${{ steps.version.outputs.version }}
|
||||
key: review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}-${{ steps.version.outputs.version }}
|
||||
|
||||
- name: Install review-bot
|
||||
if: steps.cache.outputs.cache-hit != 'true'
|
||||
@@ -140,7 +164,7 @@ runs:
|
||||
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
|
||||
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
|
||||
VERSION="${{ steps.version.outputs.version }}"
|
||||
BINARY="review-bot-linux-amd64"
|
||||
BINARY="review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}"
|
||||
|
||||
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
|
||||
-o "${{ runner.temp }}/review-bot"
|
||||
@@ -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. **[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.
|
||||
# Verify SHA-256 checksum
|
||||
|
sonnet-review-bot
commented
[NIT] The checksum regex **[NIT]** The checksum regex `[[:xdigit:]]+[[:space:]]+\*?${BINARY}$` does not anchor the hex portion to the start of the line (`^`). A line like `somejunk abc123 review-bot-linux-arm64` would not match (awk would still grab field 1), but the intent of exact matching is clearer with `^[[:xdigit:]]`. Minor defensive concern only.
|
||||
cd "${{ runner.temp }}"
|
||||
gpt-review-bot
commented
[MINOR] The checksum grep pattern anchors the end of the line but not the start. Consider anchoring with **[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.
|
||||
EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
|
||||
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
|
||||
EXPECTED=$(grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
|
||||
# sha256sum (GNU) is not available on macOS; use shasum -a 256 on darwin.
|
||||
if [ "${{ steps.version.outputs.os }}" = "darwin" ]; then
|
||||
ACTUAL=$(shasum -a 256 review-bot | awk '{print $1}')
|
||||
else
|
||||
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
|
||||
fi
|
||||
|
||||
if [ -z "$EXPECTED" ]; then
|
||||
echo "Error: no checksum found for ${BINARY}" >&2
|
||||
@@ -164,7 +193,7 @@ runs:
|
||||
fi
|
||||
|
||||
chmod +x "${{ runner.temp }}/review-bot"
|
||||
echo "Installed review-bot ${VERSION} (checksum verified)"
|
||||
echo "Installed review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }} ${VERSION} (checksum verified)"
|
||||
|
||||
- name: Run review
|
||||
shell: bash
|
||||
|
||||
[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.