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

feat(action): derive binary name from uname for multi-arch support (#124) #127

Merged
rodin merged 2 commits from issue-124 into main 2026-05-14 05:53:16 +00:00
Conversation 10 Commits 2 Files Changed 1
+34 -5
1 changed files with 34 additions and 5 deletions
Download Patch File Download Diff File Expand all files Collapse all files
.gitea/actions/review/action.yml
+34 -5
View File
@@ -124,14 +124,38 @@ runs:
else else
VERSION="${{ inputs.version }}" VERSION="${{ inputs.version }}"
fi fi
# Detect OS and architecture for platform-specific binary download
sonnet-review-bot commented 2026-05-14 05:49:07 +00:00
Review
Copy Link
Copy Source

[NIT] The OS case statement lowercases uname -s output via tr into OS_RAW, then matches linux and darwin and assigns identical strings. The tr and case together are correct but slightly redundant — you could match Linux and Darwin directly without tr. Not a bug, just minor ceremony. Current approach is defensively safe and readable.

**[NIT]** The OS case statement lowercases `uname -s` output via `tr` into `OS_RAW`, then matches `linux` and `darwin` and assigns identical strings. The `tr` and `case` together are correct but slightly redundant — you could match `Linux` and `Darwin` directly without `tr`. Not a bug, just minor ceremony. Current approach is defensively safe and readable.
OS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')
case "$OS_RAW" in
linux) OS="linux" ;;
sonnet-review-bot commented 2026-05-14 05:46:25 +00:00
Review
Copy Link
Copy Source

[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.

**[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 2026-05-14 05:49:07 +00:00
Review
Copy Link
Copy Source

[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.

**[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 "version=${VERSION}" >> "$GITHUB_OUTPUT"
echo "os=${OS}" >> "$GITHUB_OUTPUT"
echo "arch=${ARCH}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary - name: Cache review-bot binary
id: cache id: cache
uses: actions/cache@v4 uses: actions/cache@v4
with: with:
path: ${{ runner.temp }}/review-bot 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 - name: Install review-bot
if: steps.cache.outputs.cache-hit != 'true' if: steps.cache.outputs.cache-hit != 'true'
@@ -140,7 +164,7 @@ runs:
GITEA_URL="${{ inputs.gitea-url || github.server_url }}" GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}" REPO="${{ inputs.repo || 'rodin/review-bot' }}"
VERSION="${{ steps.version.outputs.version }}" 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}" \ curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot" -o "${{ runner.temp }}/review-bot"
@@ -149,8 +173,13 @@ runs:
security-review-bot commented 2026-05-14 05:49:46 +00:00
Review
Copy Link
Copy 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.

**[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 # Verify SHA-256 checksum
sonnet-review-bot commented 2026-05-14 05:46:25 +00:00
Review
Copy Link
Copy Source

[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.

**[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 }}" cd "${{ runner.temp }}"
gpt-review-bot commented 2026-05-14 05:47:20 +00:00
Review
Copy Link
Copy Source

[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.

**[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}') EXPECTED=$(grep -E "^[[:xdigit:]]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
ACTUAL=$(sha256sum review-bot | 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 if [ -z "$EXPECTED" ]; then
echo "Error: no checksum found for ${BINARY}" >&2 echo "Error: no checksum found for ${BINARY}" >&2
@@ -164,7 +193,7 @@ runs:
fi fi
chmod +x "${{ runner.temp }}/review-bot" 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 - name: Run review
shell: bash shell: bash