From 6f9e5ed2908d4d9b056f75d36969f8d0248d55ad Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 27 Aug 2026 03:11:21 -0400 Subject: [PATCH 1/2] fix(scripts): fail a bare read_list --comments with rc 2 instead of looping `read_list::into --comments` with no mode value hung forever. The option arm consumed its value with `shift 2 || true`; when `--comments` is the LAST argument, bash's `shift 2` shifts nothing and returns non-zero, `|| true` swallowed that, `$#` stayed at 1, and the `while (($# > 0))` loop reprocessed `--comments` indefinitely. The function's existing rc-2 "mode is required" branch was therefore unreachable on that path. Shift only what is actually present so the loop drains and a bare `--comments` falls through to that existing error, the same answer `--comments ''` already gave. No caller in the repo hits this today; the cost of leaving it was that a future caller which forgot the mode value got a silent, unbounded stall in a hook or CI instead of a usage error. The regression test runs the missing-value invocation under `timeout` and distinguishes rc 124 from any other wrong answer: without the timeout a regression would not fail the suite, it would hang it. Closes #3363 Co-Authored-By: Claude Fable 5 --- scripts/lib/read-list.sh | 9 ++++++++- scripts/lib/read-list.test.sh | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/scripts/lib/read-list.sh b/scripts/lib/read-list.sh index bb0eed621b..9b05a360ba 100644 --- a/scripts/lib/read-list.sh +++ b/scripts/lib/read-list.sh @@ -67,7 +67,14 @@ read_list::into() { case "$1" in --comments) _rl_mode="${2-}" - shift 2 || true + # Shift only what is actually there. `shift 2` with `--comments` as the + # LAST argument shifts nothing and returns non-zero, and the `|| true` + # this replaces swallowed that: `$#` stayed at 1 and the loop reprocessed + # `--comments` forever (#3363) instead of ever reaching the rc-2 branch + # below. Draining to `$# == 0` lets a bare `--comments` fall through to + # the existing "mode is required" error, the same answer `--comments ''` + # already gave. + shift $(($# > 1 ? 2 : 1)) ;; *) printf 'read-list: unknown option %s\n' "$1" >&2 diff --git a/scripts/lib/read-list.test.sh b/scripts/lib/read-list.test.sh index 518aff7784..89acfbe656 100755 --- a/scripts/lib/read-list.test.sh +++ b/scripts/lib/read-list.test.sh @@ -100,6 +100,28 @@ if read_list::into entries "$f" --bogus 2>/dev/null; then else ok "an unknown option is rejected" fi + +# A BARE `--comments` (the flag present, its mode value missing) must reach the +# same rc-2 answer, and must reach it AT ALL. The pre-#3363 arm consumed its +# value with `shift 2 || true`; as the last argument that shifted nothing and +# returned non-zero, `|| true` hid the failure, `$#` stayed at 1, and the option +# loop reprocessed `--comments` forever. `timeout` is the load-bearing part of +# this assertion: without it a regression does not FAIL this suite, it HANGS it, +# which in CI is an unbounded stall rather than a red test. +missing_value_rc=0 +# shellcheck disable=SC2016 # $1/$2 are the inner `bash -c` positionals, deliberately unexpanded here +timeout 5 bash -c ' + . "$1" + declare -a probe=() + read_list::into probe "$2" --comments +' _ "$SELF_DIR/read-list.sh" "$f" 2>/dev/null || missing_value_rc=$? +if [[ "$missing_value_rc" -eq 2 ]]; then + ok "a bare --comments (no mode value) returns 2 promptly" +elif [[ "$missing_value_rc" -eq 124 ]]; then + fail "a bare --comments HUNG (timed out) instead of returning 2 (#3363)" +else + fail "a bare --comments returned $missing_value_rc, want 2" +fi rm -f "$f" # --- an unreadable file is loud, never an empty list ----------------------- From cab587f5d289322557e07245fc1b1484ce88e5b2 Mon Sep 17 00:00:00 2001 From: Kyle Sexton <153232337+kyle-sexton@users.noreply.github.com> Date: Thu, 27 Aug 2026 05:33:22 -0400 Subject: [PATCH 2/2] test(scripts): bound the read-list hang probe without GNU timeout `timeout` is coreutils, not POSIX, and a stock macOS userland does not ship it: there the invocation returns 127 and fails a correct library. scripts/check-shell-portability.sh names macOS as the one platform no runner here covers, so a CI-green test is not evidence this one works for a developer on a Mac. Replaced with a hand-rolled `sleep`/`kill`/`wait` watchdog. Same assertion, same bound, POSIX-only tools. A killed probe reports 128+SIGKILL (137) rather than 124, so the hang branch keys off `>= 128`. Three details that are load-bearing rather than incidental, each recorded at its line: the watchdog shell is SIGKILLed (SIGTERM would be deferred until its foreground `sleep` returned, costing the suite five seconds on the happy path); it is killed rather than left to fire against an already-reaped pid, so there is no pid-reuse hazard; and its output is redirected to /dev/null, without which the orphaned `sleep` holds this suite's stdout open and any reader consuming the suite through a pipe blocks for the full five seconds. Measured: 0.27s piped when the library is correct, 5.3s and FAIL=1 against the pre-fix library. Co-Authored-By: Claude Fable 5 --- scripts/lib/read-list.test.sh | 39 ++++++++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/scripts/lib/read-list.test.sh b/scripts/lib/read-list.test.sh index 89acfbe656..375583fd89 100755 --- a/scripts/lib/read-list.test.sh +++ b/scripts/lib/read-list.test.sh @@ -105,20 +105,45 @@ fi # same rc-2 answer, and must reach it AT ALL. The pre-#3363 arm consumed its # value with `shift 2 || true`; as the last argument that shifted nothing and # returned non-zero, `|| true` hid the failure, `$#` stayed at 1, and the option -# loop reprocessed `--comments` forever. `timeout` is the load-bearing part of -# this assertion: without it a regression does not FAIL this suite, it HANGS it, -# which in CI is an unbounded stall rather than a red test. +# loop reprocessed `--comments` forever. +# +# The WATCHDOG is the load-bearing part of this assertion, not decoration: +# without a bound, a regression does not FAIL this suite, it HANGS it, and an +# unbounded stall in CI is worse than a red test. It is hand-rolled from +# `kill`/`wait` rather than written as `timeout 5` because GNU coreutils +# `timeout` is absent from a stock macOS userland, which +# scripts/check-shell-portability.sh names as the platform no runner here +# covers: on a developer's Mac that invocation would return 127 and fail a +# correct library. `sleep`, `kill` and `wait` are POSIX, so this bounds the +# probe everywhere. A killed probe reports 128+SIGKILL, never 2. missing_value_rc=0 # shellcheck disable=SC2016 # $1/$2 are the inner `bash -c` positionals, deliberately unexpanded here -timeout 5 bash -c ' +bash -c ' . "$1" declare -a probe=() read_list::into probe "$2" --comments -' _ "$SELF_DIR/read-list.sh" "$f" 2>/dev/null || missing_value_rc=$? +' _ "$SELF_DIR/read-list.sh" "$f" 2>/dev/null & +probe_pid=$! +# SIGKILL on the watchdog SHELL, and no `wait` for it. SIGTERM would be +# deferred until its foreground `sleep` returned, costing the suite the full +# five seconds on the HAPPY path; SIGKILL cannot be deferred. Killing the shell +# rather than letting it fire against an already-reaped pid is also what keeps +# this free of a pid-reuse hazard: the orphaned `sleep` has nothing left to run +# the kill. +# The `>/dev/null 2>&1` is not cosmetic. Without it the orphaned `sleep` +# inherits this suite's stdout, and anything reading the suite through a pipe +# blocks for the full five seconds waiting for that last writer to close. +( + sleep 5 + kill -9 "$probe_pid" 2>/dev/null +) >/dev/null 2>&1 & +watchdog_pid=$! +wait "$probe_pid" || missing_value_rc=$? +kill -9 "$watchdog_pid" 2>/dev/null if [[ "$missing_value_rc" -eq 2 ]]; then ok "a bare --comments (no mode value) returns 2 promptly" -elif [[ "$missing_value_rc" -eq 124 ]]; then - fail "a bare --comments HUNG (timed out) instead of returning 2 (#3363)" +elif [[ "$missing_value_rc" -ge 128 ]]; then + fail "a bare --comments HUNG (watchdog killed it, rc=$missing_value_rc) instead of returning 2 (#3363)" else fail "a bare --comments returned $missing_value_rc, want 2" fi