From ab29ee78fbe6afb94817e551d0966ac362b9883c Mon Sep 17 00:00:00 2001 From: cluade-reviewer-andresmgsl Date: Tue, 28 Jul 2026 09:26:17 +0000 Subject: [PATCH] fix: the --version read cannot die ahead of its own refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Self-review of the whole head after the panel converged. One real defect, reproduced verbatim before it was touched. runner_version_of is a pipeline, and every forgejo-runner-* command runs under set -euo pipefail. When the binary exits non-zero the pipeline fails, and `PRESENT_VER="$(runner_version_of "$BIN")"` is an ASSIGNMENT — so install died at that line, exit 1, printing nothing at all. The lib documented the opposite ("empty when it cannot answer"), and the refusal written for exactly this case twenty lines later — "the download landed but cannot run" — was unreachable code that could never have fired. The shape is not hypothetical. `[ -x ]` yes, runnable no is what a truncated or wrong-arch download leaves behind, and a ci-box's template preinstalls the binary at mint — so the silent death sat on the one path this command family exists to serve. json_field, two files away, already carries `|| true` with this reasoning written above it: "callers run under set -e with pipefail, where a grep that matches nothing would otherwise kill the script with no message". The same guard, for the same reason, at both sites: the lib reader and install's own read of --version. The test could not have caught it. vparse drove the lib through `bash -c` with NO shell options, and its "bad" stub exits 0 — it proved garbage-in-empty-out in a permissive shell, which is not where the function is used. vparse now runs under set -euo pipefail, and a stub that exits non-zero is a third row. Mutation-checked: that row exits 1 against the unpatched lib and 0 against the fixed one. Install's site is grep-pinned, since reaching it for real needs root and a downloaded binary. 748 passed (was 746), release 31, drill 43, shellcheck clean. --- commands/forgejo-runner-install.sh | 5 ++++- commands/lib/forgejo-runner-config.sh | 14 +++++++++++++- test/cli.sh | 17 ++++++++++++++++- 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/commands/forgejo-runner-install.sh b/commands/forgejo-runner-install.sh index e3a6fb5..66d04dc 100755 --- a/commands/forgejo-runner-install.sh +++ b/commands/forgejo-runner-install.sh @@ -347,7 +347,10 @@ if [ "$NEED_DOWNLOAD" -eq 1 ]; then mv -f "$BIN.rig-new" "$BIN" log "installed ${BIN}" fi -INSTALLED_VER="$("$BIN" --version 2>/dev/null | head -n1)" +# `|| true` so the refusal BELOW is the one that fires. Under set -euo pipefail +# a bare `VAR="$(cmd | head)"` dies at the assignment when cmd exits non-zero, +# which is precisely the case this line exists to diagnose — see the lib. +INSTALLED_VER="$("$BIN" --version 2>/dev/null | head -n1 || true)" [ -n "$INSTALLED_VER" ] || die "${BIN} does not answer --version — the download landed but cannot run" # The converge actually took — asserted, not assumed. A pin that silently did # not land is exactly the failure --version exists to make impossible. diff --git a/commands/lib/forgejo-runner-config.sh b/commands/lib/forgejo-runner-config.sh index b2f7ea7..4874f3e 100644 --- a/commands/lib/forgejo-runner-config.sh +++ b/commands/lib/forgejo-runner-config.sh @@ -59,9 +59,21 @@ forgejo_runner_secure() { # reports, empty when it cannot answer. `forgejo-runner --version` prints # "forgejo-runner version v12.13.2"; the leading v is stripped so this compares # against a --version argument, which has its own v stripped at parse. +# +# `|| true` for json_field's reason, which bites harder here. Callers run under +# `set -euo pipefail`, where a pipeline whose FIRST stage exits non-zero fails +# the whole pipeline — and `PRESENT_VER="$(runner_version_of "$BIN")"` is an +# assignment, so the script dies AT THAT LINE, with no message. "Empty when it +# cannot answer" is only true if this says so out loud. +# +# That is not a hypothetical shape: the binary is `[ -x ]` but unrunnable +# exactly when a ci-box's template preinstall landed a truncated or wrong-arch +# download — the one path this command family exists for. Without this, install +# exits 1 in silence and the refusal written for that case ("the download +# landed but cannot run") is unreachable code. runner_version_of() { "$1" --version 2>/dev/null | head -n1 \ - | sed -nE 's/.*[Vv]ersion[[:space:]]+v?([0-9][0-9A-Za-z.+-]*).*/\1/p' + | sed -nE 's/.*[Vv]ersion[[:space:]]+v?([0-9][0-9A-Za-z.+-]*).*/\1/p' || true } # runner_download_decision diff --git a/test/cli.sh b/test/cli.sh index 3a57f0f..45556da 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -3389,13 +3389,28 @@ check "forgejo-runner: install routes through the shared checksum policy" 0 "fet # binary at mint — so a bare presence check would make --version dead on the # exact path this command exists for. FRLIB="$ROOT/commands/lib/forgejo-runner-config.sh" -vparse() { bash -c ". '$FRLIB'; runner_version_of '$1'"; } +# Driven under the CALLER'S shell options, and that is half the test. Every +# forgejo-runner-* command is `set -euo pipefail`; a reader that only returns +# empty in a permissive shell has not kept its contract where it is used. +vparse() { bash -c "set -euo pipefail; . '$FRLIB'; runner_version_of \"\$1\"" _ "$1"; } VSTUB="$(mktemp -d)" printf '#!/bin/sh\necho "forgejo-runner version v12.13.2"\n' > "$VSTUB/fr"; chmod +x "$VSTUB/fr" printf '#!/bin/sh\necho "garbage"\n' > "$VSTUB/bad"; chmod +x "$VSTUB/bad" +# `[ -x ]` yes, runnable no: a truncated or wrong-arch download, which is what a +# ci-box template preinstall leaves behind when it half-lands. Distinct from +# `bad` — that one EXITS 0 and merely says nothing parseable, so it never +# exercised the pipeline-failure path at all. +printf '#!/bin/sh\nexit 1\n' > "$VSTUB/dead"; chmod +x "$VSTUB/dead" check "forgejo-runner: the version reader strips the leading v" 0 "12.13.2" vparse "$VSTUB/fr" check "forgejo-runner: an unreadable version yields empty, not garbage" 0 "" vparse "$VSTUB/bad" +check "forgejo-runner: a binary that cannot RUN yields empty, not a silent set -e death" 0 "" \ + vparse "$VSTUB/dead" rm -rf "$VSTUB" +# The install site's own read of --version needs the same guard, or its refusal +# is unreachable: the assignment dies before the test below it can fire. Pinned +# by grep because reaching that line for real needs root and a downloaded binary. +check "forgejo-runner: the --version read cannot die ahead of its own refusal" 0 "|| true" \ + grep -o 'head -n1 || true' "$FR" # The decision itself, driven — this is the case review !110 caught, and a # grep could not have caught it. Each row is a real lifecycle situation.