Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #502 +/- ##
==========================================
+ Coverage 76.65% 76.74% +0.09%
==========================================
Files 153 153
Lines 6407 6407
==========================================
+ Hits 4911 4917 +6
+ Misses 1496 1490 -6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughAdds a new Bash installer script ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as install.sh
participant GitHub as GitHub API
participant FS as File System
participant Shell as Shell Environment
User->>Script: invoke (tag, --user/--system)
Script->>Script: parse_args & detect_target
Script->>GitHub: request release JSON (resolve_tag)
GitHub-->>Script: release metadata
Script->>Script: resolve_release_asset for platform
Script->>GitHub: download asset
GitHub-->>FS: write archive
Script->>FS: extract archive to tmpdir
Script->>Script: validate `bin/ndk` and `lib/`
Script->>Shell: run runtime sanity check (LD_LIBRARY_PATH -> tmpdir/lib)
Script->>Script: check_sudo_needed & choose dest paths
Script->>FS: install binaries and libs (use sudo if required)
Script->>Shell: print PATH / LD_LIBRARY_PATH hints
Script->>User: completion status
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 3
🧹 Nitpick comments (1)
install.sh (1)
296-302: Use delimiter-aware PATH checks instead of substringgrep.Current checks can false-match on substrings. Prefer
case ":$VAR:"membership checks.💡 Proposed refactor
- if ! echo "$PATH" | grep -q "$INSTALL_DIR"; then + case ":$PATH:" in + *":$INSTALL_DIR:"*) ;; + *) echo "" echo " Add this to your shell profile (~/.bashrc / ~/.zshrc):" echo "" echo " export PATH=\"\$PATH:${INSTALL_DIR}\"" echo "" - fi + ;; + esac - if ! echo "${LD_LIBRARY_PATH:-}" | grep -q "$USER_LIB_DIR"; then + case ":${LD_LIBRARY_PATH:-}:" in + *":$USER_LIB_DIR:"*) ;; + *) echo "" echo " Add this to your shell profile (~/.bashrc / ~/.zshrc):" echo "" echo " export LD_LIBRARY_PATH=\"${USER_LIB_DIR}:\$LD_LIBRARY_PATH\"" echo "" - fi + ;; + esacAlso applies to: 304-310
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 296 - 302, Replace the substring grep PATH check with a delimiter-aware membership test using the shell case pattern to avoid false matches: change the block that tests PATH for $INSTALL_DIR (the if that currently uses echo "$PATH" | grep -q "$INSTALL_DIR") to test against ":$PATH:" with a pattern like ":$INSTALL_DIR:" (using case ":$PATH:" in *":$INSTALL_DIR:"*) so it only matches whole path elements; apply the same change to the similar check referenced around the other block (the later if at lines 304-310) so both PATH membership tests use the case ":$PATH:" pattern-match approach instead of grep substring matching.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 233-237: Downloaded artifacts are extracted and executed without
integrity checks; add strict checksum/signature verification before extraction.
After downloading "${tmp_dir}/${asset_name}" from "$download_url", download or
obtain the corresponding checksum/signature file and verify the artifact (e.g.,
validate SHA256 with sha256sum --check or verify a detached GPG signature) and
make the script exit non-zero on any mismatch or verification failure; only
proceed to tar -xzf into "$extract_dir" when verification passes. Apply the same
verification guard to the other download/extract block that also uses
download_url/tmp_dir/asset_name/extract_dir.
- Around line 253-276: The script silently downgrades a requested system install
on non-Linux targets because the linux-only check ([ "${target#linux-}" !=
"$target" ]) prevents honoring INSTALL_MODE="system"; update the logic that
handles INSTALL_MODE and target to explicitly fail when INSTALL_MODE is "system"
but the detected target is not Linux: when INSTALL_MODE="system" and target does
not start with "linux-" call err with a clear message (mentioning INSTALL_MODE
and BIN_NAME) and exit, instead of falling back to a user install; adjust the
conditional around the existing linux branch (the block using run_priv,
SYSTEM_BIN_DIR, SYSTEM_LIB_DIR, lib_installed and ldconfig) so it only runs on
Linux and add the explicit err path for non-Linux system installs.
- Around line 214-215: The temporary directory variable tmp_dir is declared
local inside main(), so the EXIT trap cannot see it when the shell exits and the
tmp dir is leaked; make TMP_DIR a global variable (e.g., initialize TMP_DIR=''
near the top), remove tmp_dir from the local declaration, set TMP_DIR in main()
instead of tmp_dir, and update all uses (the EXIT trap cleanup, and any
references to tmp_dir in functions or code) to reference TMP_DIR (e.g., rm -rf
"${TMP_DIR:-}"). Ensure the trap uses the global TMP_DIR and that TMP_DIR is
assigned before any traps that rely on it run.
---
Nitpick comments:
In `@install.sh`:
- Around line 296-302: Replace the substring grep PATH check with a
delimiter-aware membership test using the shell case pattern to avoid false
matches: change the block that tests PATH for $INSTALL_DIR (the if that
currently uses echo "$PATH" | grep -q "$INSTALL_DIR") to test against ":$PATH:"
with a pattern like ":$INSTALL_DIR:" (using case ":$PATH:" in
*":$INSTALL_DIR:"*) so it only matches whole path elements; apply the same
change to the similar check referenced around the other block (the later if at
lines 304-310) so both PATH membership tests use the case ":$PATH:"
pattern-match approach instead of grep substring matching.
Comment on lines
+214
to
+215
| local target tag release_json asset_name download_url tmp_dir extract_dir | ||
| local bin_src lib_src lib_installed |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git ls-files | grep -E "install\.sh$"Repository: relaystr/ndk
Length of output: 124
🏁 Script executed:
head -250 install.sh | tail -50Repository: relaystr/ndk
Length of output: 1866
🏁 Script executed:
# Let's examine the trap setup and main function structure
sed -n '200,250p' install.shRepository: relaystr/ndk
Length of output: 1877
🏁 Script executed:
# Check how main() is called
grep -n "^main\|^.*main(" install.sh | head -20Repository: relaystr/ndk
Length of output: 82
🏁 Script executed:
# Look for other trap definitions
grep -n "trap" install.shRepository: relaystr/ndk
Length of output: 95
🏁 Script executed:
# Check script structure and sourcing
head -50 install.shRepository: relaystr/ndk
Length of output: 1220
🏁 Script executed:
# Check if there's any cleanup at the end of main() before return
sed -n '208,320p' install.sh | tail -50Repository: relaystr/ndk
Length of output: 1555
🏁 Script executed:
# Look for the actual end of main() function
awk '/^main\(\) {/,/^}/ {print NR": "$0}' install.sh | tail -30Repository: relaystr/ndk
Length of output: 1093
🏁 Script executed:
# Let's verify the trap behavior with a test to confirm the bug
cat > /tmp/trap_test.sh << 'EOF'
#!/bin/bash
test_local_trap() {
local tmp_dir
tmp_dir="$(mktemp -d)"
echo "Created temp dir: $tmp_dir"
trap 'rm -rf "${tmp_dir:-}"' EXIT
# Function returns, tmp_dir goes out of scope
}
test_global_trap() {
TMP_DIR=""
TMP_DIR="$(mktemp -d)"
echo "Created global temp dir: $TMP_DIR"
trap 'rm -rf "${TMP_DIR:-}"' EXIT
}
echo "Testing local variable trap:"
test_local_trap
echo "After function returns"
ls /tmp/tmp.* 2>/dev/null || echo "No temp dirs left (bug not present)"
# Clean up for next test
trap - EXIT
echo ""
echo "Testing global variable trap:"
test_global_trap
EOF
chmod +x /tmp/trap_test.sh
bash /tmp/trap_test.shRepository: relaystr/ndk
Length of output: 221
🏁 Script executed:
# Verify the script is called at the end (line 313)
tail -5 install.shRepository: relaystr/ndk
Length of output: 85
🏁 Script executed:
# Check if temp dirs from the test still exist
ls -la /tmp/tmp.* 2>&1 | head -20Repository: relaystr/ndk
Length of output: 162
🏁 Script executed:
# Verify the proposed fix approach works correctly by checking the global scope
grep -n "SYSTEM_LIB_DIR\|^[A-Z_]*=" install.sh | head -15Repository: relaystr/ndk
Length of output: 796
Temporary directory cleanup fails due to local variable going out of scope when main() returns.
When main() returns, the local tmp_dir variable is unset. The EXIT trap then runs at shell exit and attempts to expand ${tmp_dir:-}, which yields an empty string (since the variable is no longer in scope), causing rm -rf "" to silently do nothing. The temporary directory is leaked.
Declare TMP_DIR as a global variable and update all references (lines 228–231 and 233–237).
Proposed fix
SYSTEM_LIB_DIR="${NDK_SYSTEM_LIB_DIR:-/usr/lib}"
+TMP_DIR=""
main() {
parse_args "$@"
need curl
need tar
- local target tag release_json asset_name download_url tmp_dir extract_dir
+ local target tag release_json asset_name download_url extract_dir
local bin_src lib_src lib_installed
target="$(detect_target)"
tag="$(resolve_tag)"
release_json="$(get_release_json "$tag")"
IFS=$'\t' read -r asset_name download_url <<< "$(resolve_release_asset "$release_json" "$target")"
[ -n "${asset_name:-}" ] || err "could not find archive asset matching '*-${target}.tar.gz' (or .tgz) for tag ${tag}"
[ -n "${download_url:-}" ] || err "could not determine asset download URL for tag ${tag}"
info "installing ${BIN_NAME} ${tag} (${target})"
info "from ${asset_name}"
- tmp_dir="$(mktemp -d)"
- extract_dir="${tmp_dir}/extract"
- trap 'rm -rf "${tmp_dir:-}"' EXIT
+ TMP_DIR="$(mktemp -d)"
+ extract_dir="${TMP_DIR}/extract"
+ trap 'rm -rf "${TMP_DIR:-}"' EXIT
mkdir -p "$extract_dir"
- curl -fsSL --progress-bar "$download_url" -o "${tmp_dir}/${asset_name}" \
+ curl -fsSL --progress-bar "$download_url" -o "${TMP_DIR}/${asset_name}" \
|| err "download failed — check that release asset exists: ${download_url}"
- tar -xzf "${tmp_dir}/${asset_name}" -C "$extract_dir" \
+ tar -xzf "${TMP_DIR}/${asset_name}" -C "$extract_dir" \
|| err "failed to extract archive: ${asset_name}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| local target tag release_json asset_name download_url tmp_dir extract_dir | |
| local bin_src lib_src lib_installed | |
| SYSTEM_LIB_DIR="${NDK_SYSTEM_LIB_DIR:-/usr/lib}" | |
| TMP_DIR="" | |
| main() { | |
| parse_args "$@" | |
| need curl | |
| need tar | |
| local target tag release_json asset_name download_url extract_dir | |
| local bin_src lib_src lib_installed | |
| target="$(detect_target)" | |
| tag="$(resolve_tag)" | |
| release_json="$(get_release_json "$tag")" | |
| IFS=$'\t' read -r asset_name download_url <<< "$(resolve_release_asset "$release_json" "$target")" | |
| [ -n "${asset_name:-}" ] || err "could not find archive asset matching '*-${target}.tar.gz' (or .tgz) for tag ${tag}" | |
| [ -n "${download_url:-}" ] || err "could not determine asset download URL for tag ${tag}" | |
| info "installing ${BIN_NAME} ${tag} (${target})" | |
| info "from ${asset_name}" | |
| TMP_DIR="$(mktemp -d)" | |
| extract_dir="${TMP_DIR}/extract" | |
| trap 'rm -rf "${TMP_DIR:-}"' EXIT | |
| mkdir -p "$extract_dir" | |
| curl -fsSL --progress-bar "$download_url" -o "${TMP_DIR}/${asset_name}" \ | |
| || err "download failed — check that release asset exists: ${download_url}" | |
| tar -xzf "${TMP_DIR}/${asset_name}" -C "$extract_dir" \ | |
| || err "failed to extract archive: ${asset_name}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 214 - 215, The temporary directory variable tmp_dir
is declared local inside main(), so the EXIT trap cannot see it when the shell
exits and the tmp dir is leaked; make TMP_DIR a global variable (e.g.,
initialize TMP_DIR='' near the top), remove tmp_dir from the local declaration,
set TMP_DIR in main() instead of tmp_dir, and update all uses (the EXIT trap
cleanup, and any references to tmp_dir in functions or code) to reference
TMP_DIR (e.g., rm -rf "${TMP_DIR:-}"). Ensure the trap uses the global TMP_DIR
and that TMP_DIR is assigned before any traps that rely on it run.
Comment on lines
+233
to
+237
| curl -fsSL --progress-bar "$download_url" -o "${tmp_dir}/${asset_name}" \ | ||
| || err "download failed — check that release asset exists: ${download_url}" | ||
|
|
||
| tar -xzf "${tmp_dir}/${asset_name}" -C "$extract_dir" \ | ||
| || err "failed to extract archive: ${asset_name}" |
There was a problem hiding this comment.
Downloaded artifact is extracted and executed without integrity verification.
The installer currently trusts network-delivered assets before extraction and execution. Add checksum/signature verification first (and fail closed) to reduce supply-chain risk.
💡 Proposed hardening direction
+ # Verify archive integrity before extraction/execution.
+ # Example: require a sidecar checksum asset and validate here.
+ # (Use sha256sum/shasum depending on platform.)
+ # err if checksum is unavailable or mismatched.
+ # Reject unsafe tar paths before extraction.
+ tar -tzf "${TMP_DIR}/${asset_name}" \
+ | awk '($0 ~ /^\//) || ($0 ~ /(^|\/)\.\.(\/|$)/) { bad=1 } END { exit bad }' \
+ || err "archive contains unsafe paths"Also applies to: 246-250
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 233 - 237, Downloaded artifacts are extracted and
executed without integrity checks; add strict checksum/signature verification
before extraction. After downloading "${tmp_dir}/${asset_name}" from
"$download_url", download or obtain the corresponding checksum/signature file
and verify the artifact (e.g., validate SHA256 with sha256sum --check or verify
a detached GPG signature) and make the script exit non-zero on any mismatch or
verification failure; only proceed to tar -xzf into "$extract_dir" when
verification passes. Apply the same verification guard to the other
download/extract block that also uses
download_url/tmp_dir/asset_name/extract_dir.
Comment on lines
+253
to
+276
| if [ "${target#linux-}" != "$target" ] && [ "$INSTALL_MODE" = "system" ]; then | ||
| USE_SUDO="0" | ||
| if [ "$(id -u)" -ne 0 ] && check_sudo_needed "$SYSTEM_BIN_DIR" "$SYSTEM_LIB_DIR"; then | ||
| need sudo | ||
| USE_SUDO="1" | ||
| fi | ||
|
|
||
| run_priv mkdir -p "$SYSTEM_BIN_DIR" "$SYSTEM_LIB_DIR" | ||
| run_priv install -m 0755 "$bin_src" "${SYSTEM_BIN_DIR}/${BIN_NAME}" | ||
|
|
||
| lib_installed="0" | ||
| for lib_src in "${extract_dir}/lib"/*; do | ||
| [ -f "$lib_src" ] || continue | ||
| run_priv install -m 0644 "$lib_src" "${SYSTEM_LIB_DIR}/$(basename "$lib_src")" | ||
| lib_installed="1" | ||
| done | ||
|
|
||
| [ "$lib_installed" = "1" ] || err "archive does not contain shared libraries under lib/" | ||
| run_priv ldconfig || true | ||
|
|
||
| ok "${BIN_NAME} installed to ${SYSTEM_BIN_DIR}/${BIN_NAME}" | ||
| ok "shared libraries installed to ${SYSTEM_LIB_DIR}" | ||
| return | ||
| fi |
There was a problem hiding this comment.
--system is silently ignored on non-Linux targets.
If users pass --system on macOS, the script falls back to user install without warning. It should error explicitly.
💡 Proposed fix
target="$(detect_target)"
+ if [ "$INSTALL_MODE" = "system" ] && [ "${target#linux-}" = "$target" ]; then
+ err "--system install mode is only supported on Linux"
+ fi
tag="$(resolve_tag)"
release_json="$(get_release_json "$tag")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 253 - 276, The script silently downgrades a
requested system install on non-Linux targets because the linux-only check ([
"${target#linux-}" != "$target" ]) prevents honoring INSTALL_MODE="system";
update the logic that handles INSTALL_MODE and target to explicitly fail when
INSTALL_MODE is "system" but the detected target is not Linux: when
INSTALL_MODE="system" and target does not start with "linux-" call err with a
clear message (mentioning INSTALL_MODE and BIN_NAME) and exit, instead of
falling back to a user install; adjust the conditional around the existing linux
branch (the block using run_priv, SYSTEM_BIN_DIR, SYSTEM_LIB_DIR, lib_installed
and ldconfig) so it only runs on Linux and add the explicit err path for
non-Linux system installs.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
install.sh (3)
253-276:⚠️ Potential issue | 🟡 MinorFail explicitly when
--systemis requested on non-Linux targets.Right now,
INSTALL_MODE=systemon macOS falls through to user install. That should error instead of silently downgrading behavior.Suggested fix
target="$(detect_target)" + if [ "$INSTALL_MODE" = "system" ] && [ "${target#linux-}" = "$target" ]; then + err "--system install mode is only supported on Linux" + fi tag="$(resolve_tag)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 253 - 276, The installer currently treats INSTALL_MODE="system" on non-Linux targets as a user install; change the conditional handling around the existing check (the block that tests if [ "${target#linux-}" != "$target" ] && [ "$INSTALL_MODE" = "system" ]) so that when INSTALL_MODE="system" but the target is not Linux you call err with a clear message and exit instead of falling through; update the logic near that conditional (referencing INSTALL_MODE, target, and the err helper) to explicitly fail (err "... --system is only supported on Linux") before any user-install code runs.
214-231:⚠️ Potential issue | 🟠 MajorMake temp-dir cleanup trap reference a global variable.
Line 214 declares
tmp_diras local. TheEXITtrap executes aftermain()returns, so${tmp_dir:-}can be empty and cleanup can be skipped.Suggested fix
SYSTEM_LIB_DIR="${NDK_SYSTEM_LIB_DIR:-/usr/lib}" +TMP_DIR="" main() { @@ - local target tag release_json asset_name download_url tmp_dir extract_dir + local target tag release_json asset_name download_url extract_dir @@ - tmp_dir="$(mktemp -d)" - extract_dir="${tmp_dir}/extract" - trap 'rm -rf "${tmp_dir:-}"' EXIT + TMP_DIR="$(mktemp -d)" + extract_dir="${TMP_DIR}/extract" + trap 'rm -rf "${TMP_DIR:-}"' EXIT mkdir -p "$extract_dir" - curl -fsSL --progress-bar "$download_url" -o "${tmp_dir}/${asset_name}" \ + curl -fsSL --progress-bar "$download_url" -o "${TMP_DIR}/${asset_name}" \ @@ - tar -xzf "${tmp_dir}/${asset_name}" -C "$extract_dir" \ + tar -xzf "${TMP_DIR}/${asset_name}" -C "$extract_dir" \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 214 - 231, The cleanup trap can see an empty ${tmp_dir:-} because tmp_dir is declared local; remove tmp_dir from the local declaration (or promote it to a global variable like INSTALL_TMP_DIR) so the EXIT trap 'rm -rf "${tmp_dir:-}"' can always reference the same global variable, and update references (tmp_dir, extract_dir, trap) to use that global name consistently (e.g., tmp_dir -> INSTALL_TMP_DIR) in the install logic (resolve_tag/get_release_json/resolve_release_asset block) so temp dir is always cleaned up after main returns.
233-250:⚠️ Potential issue | 🔴 CriticalVerify release artifact integrity before extraction and execution.
Lines 233-250 download, extract, and run an archive without checksum/signature verification. This is a supply-chain risk.
Suggested hardening (checksum + safe tar path check)
+ # resolve checksum asset URL (expects `${asset_name}.sha256` in the release) + checksum_url="$( + printf "%s\n" "$release_json" | awk -v n="${asset_name}.sha256" ' + BEGIN { name="" } + /"name":/ { + if (match($0, /"name":[[:space:]]*"([^"]+)"/, m)) name=m[1] + next + } + /"browser_download_url":/ { + if (name == n && match($0, /"browser_download_url":[[:space:]]*"([^"]+)"/, u)) { + print u[1]; exit + } + }' + )" + [ -n "${checksum_url:-}" ] || err "missing checksum asset: ${asset_name}.sha256" + curl -fsSL --progress-bar "$download_url" -o "${TMP_DIR}/${asset_name}" \ || err "download failed — check that release asset exists: ${download_url}" + curl -fsSL --progress-bar "$checksum_url" -o "${TMP_DIR}/${asset_name}.sha256" \ + || err "failed to download checksum file" + + if command -v sha256sum >/dev/null 2>&1; then + (cd "$TMP_DIR" && sha256sum -c "${asset_name}.sha256") \ + || err "checksum verification failed" + elif command -v shasum >/dev/null 2>&1; then + expected="$(awk '{print $1}' "${TMP_DIR}/${asset_name}.sha256")" + actual="$(shasum -a 256 "${TMP_DIR}/${asset_name}" | awk '{print $1}')" + [ "$expected" = "$actual" ] || err "checksum verification failed" + else + err "no SHA256 tool found (need sha256sum or shasum)" + fi + + tar -tzf "${TMP_DIR}/${asset_name}" \ + | awk '($0 ~ /^\//) || ($0 ~ /(^|\/)\.\.(\/|$)/) { bad=1 } END { exit bad }' \ + || err "archive contains unsafe paths"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 233 - 250, Before extracting and running the downloaded archive, fetch and verify its integrity/signature: download the corresponding checksum or signature (e.g. from a CHECKSUM_URL or .sig for the release) and validate the downloaded file "${tmp_dir}/${asset_name}" against that checksum/signature, aborting via err on mismatch; after checksum validation but before tar -xzf, perform a safe-path check on the archive contents to prevent path traversal (ensure no entries contain absolute paths or .. components) and only then continue to extract to "$extract_dir" and proceed to chmod +x "$bin_src" and the runtime checks; reference variables download_url, tmp_dir, asset_name, extract_dir, BIN_NAME and the err handler when adding these checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Line 10: Add validation in parse_args() to check the resolved INSTALL_MODE
(from NDK_INSTALL_MODE or default) and reject any value other than "user" or
"system": locate where INSTALL_MODE is set
(INSTALL_MODE="${NDK_INSTALL_MODE:-user}") and in parse_args() verify
INSTALL_MODE is exactly "user" or "system"; if not, print a clear error and exit
non‑zero. Ensure the error message references the invalid value and shows the
allowed options so callers can correct NDK_INSTALL_MODE.
---
Duplicate comments:
In `@install.sh`:
- Around line 253-276: The installer currently treats INSTALL_MODE="system" on
non-Linux targets as a user install; change the conditional handling around the
existing check (the block that tests if [ "${target#linux-}" != "$target" ] && [
"$INSTALL_MODE" = "system" ]) so that when INSTALL_MODE="system" but the target
is not Linux you call err with a clear message and exit instead of falling
through; update the logic near that conditional (referencing INSTALL_MODE,
target, and the err helper) to explicitly fail (err "... --system is only
supported on Linux") before any user-install code runs.
- Around line 214-231: The cleanup trap can see an empty ${tmp_dir:-} because
tmp_dir is declared local; remove tmp_dir from the local declaration (or promote
it to a global variable like INSTALL_TMP_DIR) so the EXIT trap 'rm -rf
"${tmp_dir:-}"' can always reference the same global variable, and update
references (tmp_dir, extract_dir, trap) to use that global name consistently
(e.g., tmp_dir -> INSTALL_TMP_DIR) in the install logic
(resolve_tag/get_release_json/resolve_release_asset block) so temp dir is always
cleaned up after main returns.
- Around line 233-250: Before extracting and running the downloaded archive,
fetch and verify its integrity/signature: download the corresponding checksum or
signature (e.g. from a CHECKSUM_URL or .sig for the release) and validate the
downloaded file "${tmp_dir}/${asset_name}" against that checksum/signature,
aborting via err on mismatch; after checksum validation but before tar -xzf,
perform a safe-path check on the archive contents to prevent path traversal
(ensure no entries contain absolute paths or .. components) and only then
continue to extract to "$extract_dir" and proceed to chmod +x "$bin_src" and the
runtime checks; reference variables download_url, tmp_dir, asset_name,
extract_dir, BIN_NAME and the err handler when adding these checks.
| INSTALL_DIR="${NDK_INSTALL_DIR:-$HOME/.local/bin}" | ||
| VERSION="${NDK_VERSION:-latest}" | ||
| PRERELEASE="${NDK_PRERELEASE:-0}" | ||
| INSTALL_MODE="${NDK_INSTALL_MODE:-user}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -type f -name "install.sh" | head -20Repository: relaystr/ndk
Length of output: 226
🏁 Script executed:
fd -t f -name "install.sh"Repository: relaystr/ndk
Length of output: 286
🏁 Script executed:
find . -name "install.sh" -type fRepository: relaystr/ndk
Length of output: 68
🏁 Script executed:
cat -n ./install.sh | head -220Repository: relaystr/ndk
Length of output: 6982
🏁 Script executed:
cat -n ./install.sh | sed -n '205,300p'Repository: relaystr/ndk
Length of output: 4081
Add validation for NDK_INSTALL_MODE environment variable.
INSTALL_MODE can be set from the environment to an invalid value and will silently fall back to the default user install mode instead of rejecting it. Add validation in parse_args() to ensure only valid values (user or system) are accepted.
Suggested fix
parse_args() {
while [ $# -gt 0 ]; do
case "$1" in
--system) INSTALL_MODE="system" ;;
--user) INSTALL_MODE="user" ;;
-h | --help)
cat <<EOF
Usage: install.sh [--user|--system]
Install mode (Linux):
--user Install to user paths (default):
- binary: NDK_INSTALL_DIR (default: ~/.local/bin)
- libs: NDK_USER_LIB_DIR (default: ~/.local/lib)
--system Install to system paths:
- binary: NDK_SYSTEM_BIN_DIR (default: /usr/bin)
- libs: NDK_SYSTEM_LIB_DIR (default: /usr/lib)
Env vars:
NDK_VERSION Version or tag. Also supports latest, latest-pre.
NDK_PRERELEASE When truthy and NDK_VERSION=latest, picks latest pre-release.
NDK_INSTALL_MODE user|system (Linux)
EOF
exit 0
;;
*) err "unknown argument: $1" ;;
esac
shift
done
+
+ case "$INSTALL_MODE" in
+ user|system) ;;
+ *) err "invalid install mode: ${INSTALL_MODE} (expected 'user' or 'system')" ;;
+ esac
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| INSTALL_MODE="${NDK_INSTALL_MODE:-user}" | |
| parse_args() { | |
| while [ $# -gt 0 ]; do | |
| case "$1" in | |
| --system) INSTALL_MODE="system" ;; | |
| --user) INSTALL_MODE="user" ;; | |
| -h | --help) | |
| cat <<EOF | |
| Usage: install.sh [--user|--system] | |
| Install mode (Linux): | |
| --user Install to user paths (default): | |
| - binary: NDK_INSTALL_DIR (default: ~/.local/bin) | |
| - libs: NDK_USER_LIB_DIR (default: ~/.local/lib) | |
| --system Install to system paths: | |
| - binary: NDK_SYSTEM_BIN_DIR (default: /usr/bin) | |
| - libs: NDK_SYSTEM_LIB_DIR (default: /usr/lib) | |
| Env vars: | |
| NDK_VERSION Version or tag. Also supports latest, latest-pre. | |
| NDK_PRERELEASE When truthy and NDK_VERSION=latest, picks latest pre-release. | |
| NDK_INSTALL_MODE user|system (Linux) | |
| EOF | |
| exit 0 | |
| ;; | |
| *) err "unknown argument: $1" ;; | |
| esac | |
| shift | |
| done | |
| case "$INSTALL_MODE" in | |
| user|system) ;; | |
| *) err "invalid install mode: ${INSTALL_MODE} (expected 'user' or 'system')" ;; | |
| esac | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` at line 10, Add validation in parse_args() to check the resolved
INSTALL_MODE (from NDK_INSTALL_MODE or default) and reject any value other than
"user" or "system": locate where INSTALL_MODE is set
(INSTALL_MODE="${NDK_INSTALL_MODE:-user}") and in parse_args() verify
INSTALL_MODE is exactly "user" or "system"; if not, print a clear error and exit
non‑zero. Ensure the error message references the invalid value and shows the
allowed options so callers can correct NDK_INSTALL_MODE.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
install.sh (3)
222-223:⚠️ Potential issue | 🟡 Minor
--systemsilently degrades to a user install on non-Linux targets.The only
systembranch is Linux-only, soINSTALL_MODE=systemon macOS just falls through to the default user-install path instead of failing. That makes the script do something different from what the caller requested.Suggested fix
target="$(detect_target)" + if [ "$INSTALL_MODE" = "system" ] && [ "${target#linux-}" = "$target" ]; then + err "--system install mode is only supported on Linux" + fi tag="$(resolve_tag)" release_json="$(get_release_json "$tag")"Also applies to: 258-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 222 - 223, The script silently falls back to user install when INSTALL_MODE=system on non-Linux hosts because the only system-install branch is executed only for Linux; update the logic that uses detect_target and INSTALL_MODE to explicitly detect non-Linux targets and abort with a clear error when INSTALL_MODE is set to "system" on unsupported platforms. Specifically, after computing target="$(detect_target)" (and similarly in the later install handling at the other block around the install decision), add a check that if INSTALL_MODE = "system" and target is not a Linux variant then print an error and exit non‑zero so the caller's requested mode is not silently downgraded. Ensure the same guard is applied in both places referenced in the diff (the target/ tag resolution block and the install decision block).
238-255:⚠️ Potential issue | 🟠 MajorVerify the archive before
tarorndktouches it.The current flow downloads, extracts,
chmod +x, and executes a release artifact without any checksum/signature verification. A corrupted or substituted asset becomes code execution inside the installer path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 238 - 255, Add integrity verification before extracting or executing the downloaded asset: after the curl step that writes "${tmp_dir}/${asset_name}" from "$download_url", verify the file using a checksum or signature mechanism (e.g., compare against a known SHA256/SHA512 value or verify a detached/embedded GPG signature) and fail via err if verification fails; only proceed to tar -xzf and chmod +x "$bin_src" and the subsequent LD_LIBRARY_PATH checks when the verification succeeds. Use the existing variables download_url, tmp_dir, asset_name and preserve the err(...) error path for failures, and ensure the verification step is clearly logged and aborts on mismatch to prevent extracting/running untrusted binaries referenced by BIN_NAME.
219-235:⚠️ Potential issue | 🟡 Minor
EXITcleanup losestmp_dirwhenmain()returns.
tmp_diris local tomain(), but the trap fires at shell exit. By then${tmp_dir:-}expands empty, so the temporary directory is leaked.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@install.sh` around lines 219 - 235, The trap uses single quotes so ${tmp_dir:-} is expanded only at shell exit when tmp_dir is out of scope (it’s declared local in main()), causing the temp dir to be leaked; fix by capturing the path when setting the trap (or make tmp_dir non-local). Concretely, after creating tmp_dir set the trap with an already-expanded value (e.g. use double-quote/expanded form: trap "rm -rf '${tmp_dir:-}'" EXIT) or remove the local qualifier for tmp_dir so the cleanup command can see it, and keep the cleanup command referencing tmp_dir (symbol names: tmp_dir and the trap call in main()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.sh`:
- Around line 136-157: The awk parser in the resolve_release_asset() install
script is capturing the release-level "name" before entering the assets[] array
so the first asset is skipped; change the awk logic inside the shown block to
only record "name" when inside the assets array (e.g., track an assets flag set
when a /"assets":/ line is seen and clear it after the corresponding array
ends), use that flag when matching /"name":/ and /"browser_download_url":/
(variables name and url) and ensure name is reset after emitting a match; this
ensures the first asset's name/url pair is selectable.
---
Duplicate comments:
In `@install.sh`:
- Around line 222-223: The script silently falls back to user install when
INSTALL_MODE=system on non-Linux hosts because the only system-install branch is
executed only for Linux; update the logic that uses detect_target and
INSTALL_MODE to explicitly detect non-Linux targets and abort with a clear error
when INSTALL_MODE is set to "system" on unsupported platforms. Specifically,
after computing target="$(detect_target)" (and similarly in the later install
handling at the other block around the install decision), add a check that if
INSTALL_MODE = "system" and target is not a Linux variant then print an error
and exit non‑zero so the caller's requested mode is not silently downgraded.
Ensure the same guard is applied in both places referenced in the diff (the
target/ tag resolution block and the install decision block).
- Around line 238-255: Add integrity verification before extracting or executing
the downloaded asset: after the curl step that writes "${tmp_dir}/${asset_name}"
from "$download_url", verify the file using a checksum or signature mechanism
(e.g., compare against a known SHA256/SHA512 value or verify a detached/embedded
GPG signature) and fail via err if verification fails; only proceed to tar -xzf
and chmod +x "$bin_src" and the subsequent LD_LIBRARY_PATH checks when the
verification succeeds. Use the existing variables download_url, tmp_dir,
asset_name and preserve the err(...) error path for failures, and ensure the
verification step is clearly logged and aborts on mismatch to prevent
extracting/running untrusted binaries referenced by BIN_NAME.
- Around line 219-235: The trap uses single quotes so ${tmp_dir:-} is expanded
only at shell exit when tmp_dir is out of scope (it’s declared local in main()),
causing the temp dir to be leaked; fix by capturing the path when setting the
trap (or make tmp_dir non-local). Concretely, after creating tmp_dir set the
trap with an already-expanded value (e.g. use double-quote/expanded form: trap
"rm -rf '${tmp_dir:-}'" EXIT) or remove the local qualifier for tmp_dir so the
cleanup command can see it, and keep the cleanup command referencing tmp_dir
(symbol names: tmp_dir and the trap call in main()).
Comment on lines
+136
to
+157
| printf "%s\n" "$release_json" | awk -v target="$target" ' | ||
| BEGIN { name="" } | ||
| /"name":/ { | ||
| if (name == "") { | ||
| name = $0 | ||
| sub(/^.*"name":[[:space:]]*"/, "", name) | ||
| sub(/".*$/, "", name) | ||
| } | ||
| next | ||
| } | ||
| /"browser_download_url":/ { | ||
| if (name != "") { | ||
| url = $0 | ||
| sub(/^.*"browser_download_url":[[:space:]]*"/, "", url) | ||
| sub(/".*$/, "", url) | ||
| if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) { | ||
| print name "\t" url | ||
| exit | ||
| } | ||
| } | ||
| name = "" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Releases API response JSON structure assets array field order
💡 Result:
The GitHub Releases API response JSON structure includes an 'assets' array in release objects (e.g., from GET /repos/{owner}/{repo}/releases, GET /repos/{owner}/{repo}/releases/{release_id}). Official documentation does not specify any guaranteed order for the elements in this array. No sorting criteria (such as by ID, name, created_at, or upload order) is documented. In practice, it is likely unordered or sorted by creation time or ID (ascending), as is common for database-backed lists without explicit sorting, but this is not guaranteed and may vary. For reliable ordering, clients should sort the array themselves if order matters. Note that the dedicated List release assets endpoint (GET /repos/{owner}/{repo}/releases/{release_id}/assets) supports pagination but also lacks documented sort order.
Citations:
- 1: https://docs.github.com/rest/releases/releases
- 2: https://docs.github.com/en/rest/releases?apiVersion=2022-11-28
- 3: https://docs.github.com/enterprise-cloud@latest/rest/releases/assets
- 4: https://docs.github.com/en/rest/releases/assets
- 5: https://docs.github.com/en/rest/releases/releases
- 6: https://docs.github.com/en/rest/releases/releases?apiVersion=2022-11-28
- 7: https://developer.github.com/enterprise/2.6/v3/repos/releases/
🏁 Script executed:
# Let's also create a minimal test to trace through the AWK logic
cat > /tmp/test_awk.sh << 'EOF'
#!/bin/bash
# Sample GitHub Releases API response (simplified)
release_json='
{
"id": 123,
"tag_name": "v1.0.0",
"name": "Release v1.0.0",
"assets": [
{
"id": 456,
"name": "my-tool-linux-amd64.tar.gz",
"browser_download_url": "https://example.com/my-tool-linux-amd64.tar.gz"
},
{
"id": 789,
"name": "my-tool-darwin-arm64.tar.gz",
"browser_download_url": "https://example.com/my-tool-darwin-arm64.tar.gz"
}
]
}
'
target="linux-amd64"
echo "=== Testing ORIGINAL code ==="
printf "%s\n" "$release_json" | awk -v target="$target" '
BEGIN { name="" }
/"name":/ {
if (name == "") {
name = $0
sub(/^.*"name":[[:space:]]*"/, "", name)
sub(/".*$/, "", name)
}
next
}
/"browser_download_url":/ {
if (name != "") {
url = $0
sub(/^.*"browser_download_url":[[:space:]]*"/, "", url)
sub(/".*$/, "", url)
if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) {
print name "\t" url
exit
}
}
name = ""
}
'
echo ""
echo "=== Testing SUGGESTED FIX ==="
printf "%s\n" "$release_json" | awk -v target="$target" '
BEGIN { in_assets=0; name="" }
/"assets":[[:space:]]*\[/ { in_assets=1; next }
in_assets && /^[[:space:]]*\],?$/ { exit }
!in_assets { next }
/"name":/ {
if (name == "") {
name = $0
sub(/^.*"name":[[:space:]]*"/, "", name)
sub(/".*$/, "", name)
}
next
}
/"browser_download_url":/ {
if (name != "") {
url = $0
sub(/^.*"browser_download_url":[[:space:]]*"/, "", url)
sub(/".*$/, "", url)
if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) {
print name "\t" url
exit
}
}
name = ""
}
'
EOF
bash /tmp/test_awk.sh
EOFRepository: relaystr/ndk
Length of output: 252
🏁 Script executed:
# Corrected test script
release_json='
{
"id": 123,
"tag_name": "v1.0.0",
"name": "Release v1.0.0",
"assets": [
{
"id": 456,
"name": "my-tool-linux-amd64.tar.gz",
"browser_download_url": "https://example.com/my-tool-linux-amd64.tar.gz"
},
{
"id": 789,
"name": "my-tool-darwin-arm64.tar.gz",
"browser_download_url": "https://example.com/my-tool-darwin-arm64.tar.gz"
}
]
}
'
target="linux-amd64"
echo "=== ORIGINAL CODE (from install.sh lines 136-157) ==="
printf "%s\n" "$release_json" | awk -v target="$target" '
BEGIN { name="" }
/"name":/ {
if (name == "") {
name = $0
sub(/^.*"name":[[:space:]]*"/, "", name)
sub(/".*$/, "", name)
}
next
}
/"browser_download_url":/ {
if (name != "") {
url = $0
sub(/^.*"browser_download_url":[[:space:]]*"/, "", url)
sub(/".*$/, "", url)
if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) {
print name "\t" url
exit
}
}
name = ""
}
'
echo "(Exit code: $?)"
echo ""
echo "=== SUGGESTED FIX ==="
printf "%s\n" "$release_json" | awk -v target="$target" '
BEGIN { in_assets=0; name="" }
/"assets":[[:space:]]*\[/ { in_assets=1; next }
in_assets && /^[[:space:]]*\],?$/ { exit }
!in_assets { next }
/"name":/ {
if (name == "") {
name = $0
sub(/^.*"name":[[:space:]]*"/, "", name)
sub(/".*$/, "", name)
}
next
}
/"browser_download_url":/ {
if (name != "") {
url = $0
sub(/^.*"browser_download_url":[[:space:]]*"/, "", url)
sub(/".*$/, "", url)
if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) {
print name "\t" url
exit
}
}
name = ""
}
'
echo "(Exit code: $?)"Repository: relaystr/ndk
Length of output: 236
The first release asset is never selectable.
Line 138 captures the release-level "name" before the parser reaches the assets[] array, causing the first asset's "name" to be skipped. If the target archive is listed first, resolve_release_asset() returns nothing and the install fails at lines 226–228.
Suggested fix
- printf "%s\n" "$release_json" | awk -v target="$target" '
- BEGIN { name="" }
+ printf "%s\n" "$release_json" | awk -v target="$target" '
+ BEGIN { in_assets=0; name="" }
+ /"assets":[[:space:]]*\[/ { in_assets=1; next }
+ in_assets && /^[[:space:]]*\],?$/ { exit }
+ !in_assets { next }
/"name":/ {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| printf "%s\n" "$release_json" | awk -v target="$target" ' | |
| BEGIN { name="" } | |
| /"name":/ { | |
| if (name == "") { | |
| name = $0 | |
| sub(/^.*"name":[[:space:]]*"/, "", name) | |
| sub(/".*$/, "", name) | |
| } | |
| next | |
| } | |
| /"browser_download_url":/ { | |
| if (name != "") { | |
| url = $0 | |
| sub(/^.*"browser_download_url":[[:space:]]*"/, "", url) | |
| sub(/".*$/, "", url) | |
| if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) { | |
| print name "\t" url | |
| exit | |
| } | |
| } | |
| name = "" | |
| } | |
| printf "%s\n" "$release_json" | awk -v target="$target" ' | |
| BEGIN { in_assets=0; name="" } | |
| /"assets":[[:space:]]*\[/ { in_assets=1; next } | |
| in_assets && /^[[:space:]]*\],?$/ { exit } | |
| !in_assets { next } | |
| /"name":/ { | |
| if (name == "") { | |
| name = $0 | |
| sub(/^.*"name":[[:space:]]*"/, "", name) | |
| sub(/".*$/, "", name) | |
| } | |
| next | |
| } | |
| /"browser_download_url":/ { | |
| if (name != "") { | |
| url = $0 | |
| sub(/^.*"browser_download_url":[[:space:]]*"/, "", url) | |
| sub(/".*$/, "", url) | |
| if (name ~ ("-" target "\\.(tar\\.gz|tgz)$")) { | |
| print name "\t" url | |
| exit | |
| } | |
| } | |
| name = "" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.sh` around lines 136 - 157, The awk parser in the
resolve_release_asset() install script is capturing the release-level "name"
before entering the assets[] array so the first asset is skipped; change the awk
logic inside the shown block to only record "name" when inside the assets array
(e.g., track an assets flag set when a /"assets":/ line is seen and clear it
after the corresponding array ends), use that flag when matching /"name":/ and
/"browser_download_url":/ (variables name and url) and ensure name is reset
after emitting a match; this ensures the first asset's name/url pair is
selectable.
1-leo
approved these changes
Mar 26, 2026
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #501
Summary by CodeRabbit