fix(scripts): fail a bare read_list --comments with rc 2 instead of looping

[kyle-sexton]

Summary

read_list::into <arr> <file> --comments with the mode value missing hung
forever instead of returning the rc 2 the function already had a branch for.

The option loop's --comments arm consumed its value with shift 2 || true
(scripts/lib/read-list.sh:70). When --comments is the last positional,
bash's shift 2 with only one positional left shifts nothing and returns
non-zero; || true swallowed that failure, so $# stayed at 1 and
while (($# > 0)) reprocessed --comments indefinitely. The rc-2
"mode is required" branch below it was unreachable on that path.

No in-repo caller hits this today. The cost of leaving it was that any future
caller which forgot the mode value got a silent, unbounded stall in a hook or
in CI rather than a usage error.

Fix

scripts/lib/read-list.sh shifts only what is actually present:

shift $(($# > 1 ? 2 : 1))

The loop now drains to $# == 0, and a bare --comments falls through to the
existing --comments is required (inline|leading); there is no default error
with rc 2, which is the same answer --comments '' already gave. Deliberately
reusing that branch rather than adding a second message: the two inputs are the
same mistake and should not diverge in wording.

Verification

New case in scripts/lib/read-list.test.sh runs the missing-value invocation
under a bound and asserts rc 2. The bound is the load-bearing part: without
it a regression does not fail this suite, it hangs it, and an unbounded
stall in CI is worse than a red test.

The bound is a hand-rolled sleep/kill/wait watchdog rather than
timeout 5. GNU coreutils timeout is absent from a stock macOS userland, and
scripts/check-shell-portability.sh:2-12 names macOS as exactly the platform
no runner here covers, so a timeout-based test would return 127 and fail a
correct library on a developer's Mac with CI none the wiser. (Raised by
chatgpt-codex-connector on the first push; fixed in cab587f.) Three details in
the watchdog are load-bearing and commented at their lines: the watchdog shell
is SIGKILLed (SIGTERM is deferred until its foreground sleep returns, costing
five seconds on the happy path), it is killed rather than left to fire at an
already-reaped pid (no pid-reuse window), and its output goes to /dev/null
(otherwise the orphaned sleep holds the suite's stdout open and any reader
consuming it through a pipe blocks five seconds -- measured: 7.2s before the
redirect, 0.27s after).

• With the fix: PASS=30 FAIL=0, 0.27s piped.
• Proven to fail pre-fix: with scripts/lib/read-list.sh reverted to main,
  the suite reports
  FAIL: a bare --comments HUNG (watchdog killed it, rc=137) instead of returning 2 (#3363)
  (PASS=29 FAIL=1), bounded at 5.3s.
• scripts/affected-tests.sh --explain scripts/lib/read-list.sh scripts/lib/read-list.test.sh maps both files to suites (no unmapped file).
  Every suite that sources or names read-list.sh was run green:
  read-list 30/0, affected-tests 47/0, check-changelog-parity 84/0,
  check-docs-only 21/0, check-hook-userconfig-argv 17/0,
  check-orphaned-fixtures 13/0, check-skill-portability 92/0,
  check-shell-portability 336/0.
• scripts/check-shell-portability.sh --paths on both changed files: clean.
• shellcheck --rcfile .shellcheckrc and shfmt -d on both files: clean.

Related

Closes #3363

Found by the batch-simplify sweep on claude/code-tidying-batch-simplify-s7ljbi
and deliberately left unfixed there because that sweep was behavior-preserving.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 1m 46s —— View job

---

Claude finished reviewing this PR

• Gather context (diff, changed files, PR description)
• Run /review:code-review procedure
• Post inline findings (if any)
• Post summary review

Summary

This is a small, well-scoped fix: scripts/lib/read-list.sh:70-77 changes the --comments arm's shift 2 || true to shift $(($# > 1 ? 2 : 1)).

I traced the fix by hand against both call shapes:

• --comments <mode> ($# == 2 at the top of that case arm): $# > 1 is true → shift 2, identical to the old behavior. All ten in-repo callers (check-purged-em-dashes.sh, check-orphaned-fixtures.sh, check-hook-userconfig-argv.sh, check-docs-only.sh, check-shell-portability.sh, affected-tests.sh, check-skill-portability.sh, check-changelog-parity.sh, plus the test file) always pass a mode value, so none of them are affected by this change.
• bare --comments ($# == 1, nothing after it): $# > 1 is false → shift 1, draining $# to 0 instead of leaving it at 1. The loop exits instead of reprocessing --comments forever, _rl_mode stays "" (from ${2-} on a missing $2), and control reaches the existing --comments is required branch at line 88, returning rc 2. This matches the --comments '' behavior exactly, which is the right call — same input class, same diagnostic.

The new test (scripts/lib/read-list.test.sh:104-125) correctly wraps the missing-value invocation in timeout 5 and distinguishes rc 2 (pass) from rc 124 (explicit hang failure) from anything else — that distinction is what makes a future regression fail loudly instead of just stalling CI. The bash -c '...' _ "$SELF_DIR/read-list.sh" "$f" 2>/dev/null idiom to isolate the sourced function in a subprocess (so a real hang is actually killable by timeout) is sound, and the SC2016 disable is correctly scoped to the single-quoted heredoc where $1/$2 are deliberately unexpanded outer-shell references.

No correctness issues found. No inline comments to post — the change is internally consistent and the shift arithmetic is exhaustively covered by the trace above.

Note on verification: this review lane's Bash tool declined to execute bash scripts/lib/read-list.test.sh or shellcheck here (commands against these specific files required approval that wasn't available non-interactively), so I verified correctness by static trace rather than by re-running the suite. The PR body already documents a pre/post-fix run (PASS=29 FAIL=1 reverted, PASS=30 FAIL=0 fixed) plus clean shellcheck/shfmt output, which is consistent with what the diff shows.

[claude]

Claude finished @kyle-sexton's task in 33s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Run skip-gate check
• Review changed files for security-relevant issues
• Post findings (or a clean-review note)

No security issues found.

This PR changes only the internal option-parsing loop in read_list::into (scripts/lib/read-list.sh) and adds a regression test (scripts/lib/read-list.test.sh). The fix replaces shift 2 || true with shift $(($# > 1 ? 2 : 1)) so a bare --comments (missing its mode value) drains the argument loop and falls through to the existing rc-2 "mode is required" error instead of looping forever.

There is no user-controlled input, no injection surface (command/SQL/path/template), no credential or token handling, no authorization logic, and no GitHub Actions workflow file touched. The change is a pure argument-parsing correctness fix scoped to a sourced shell library used internally by other repo scripts — no security-relevant surface in this diff.

[github-actions]

Last security-reviewed head: cab587f5d289322557e07245fc1b1484ce88e5b2. On the next push, the relevance gate compares only the commits since this SHA; delete this comment to force a full re-review.

[github-actions]

Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6f9e5ed290

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 23s —— View job

---

Claude finished the code-review lane for this PR

• Gather context (diff, changed files, PR description, prior review comments)
• Run /review:code-review procedure
• Post inline findings (if any)
• Post summary review

Summary

This PR now has two commits. The first (6f9e5ed, previously reviewed) fixes the read_list::into hang by replacing shift 2 || true with shift $(($# > 1 ? 2 : 1)) in scripts/lib/read-list.sh:70-77. I re-traced that logic and it's still correct: with $# == 1 (bare --comments), shift 1 drains the loop to $# == 0 instead of looping forever, and control falls through to the existing --comments is required rc-2 branch.

The second commit (cab587f) directly answers the one actionable finding from the earlier Codex review — that wrapping the hang probe in timeout 5 would return 127 on a stock macOS userland (no GNU coreutils timeout), silently failing the suite on a platform this repo's own check-shell-portability.sh treats as covered. I diffed 6f9e5ed..cab587f (scripts/lib/read-list.test.sh only, +32/-7) and traced the replacement:

• A hand-rolled watchdog (scripts/lib/read-list.test.sh:119-149) backgrounds the probe (bash -c '...' &, capturing probe_pid), backgrounds a (sleep 5; kill -9 "$probe_pid") & watchdog, then waits on the probe and SIGKILLs the watchdog shell as soon as wait returns.
• On the happy path (fixed library), the probe returns rc 2 quickly, wait returns immediately, and the watchdog shell is killed before its sleep 5 elapses — no artificial 5s tax on the suite.
• On a regression (hung probe), the watchdog's kill -9 "$probe_pid" fires after 5s, wait reports the SIGKILL exit status (128+9=137), and the assertion's -ge 128 branch reports the explicit "HUNG (watchdog killed it...)" failure rather than a false pass.
• The pid-reuse hazard is handled correctly: SIGKILL targets the watchdog's own shell, not just letting its sleep run to completion — so if the probe's pid were reused by an unrelated process in the interim, there's no surviving code path left to send it a stray kill -9. sleep/kill/wait are all POSIX, so this bounds the probe on every platform check-shell-portability.sh claims to cover.
• The >/dev/null 2>&1 on the watchdog subshell is correctly explained as non-cosmetic (an orphaned sleep inheriting the suite's stdout would otherwise hold a pipe reader open for the full 5s).

I did not find any new correctness issues in either commit. No inline comments to post.

Verification note: as in the prior review pass, this lane's Bash tool declined to run bash scripts/lib/read-list.test.sh or shellcheck here non-interactively, so the above is a static trace, not a re-executed test run. The PR body's PASS=30 FAIL=0 result is author-claimed and unverified by me.
·

[claude]

Claude finished @kyle-sexton's task in 53s —— View job

---

Security review complete

• Gather context (diff, changed files, PR description)
• Run /review:security-review procedure
• Post findings (or a clean-review note)

No security issues found.

The commits since the last security-reviewed head (6f9e5ed) add only test-only changes: cab587f replaces the timeout 5 wrapper in the new regression test with a hand-rolled bash watchdog (background sleep 5 + kill -9 "$probe_pid"), addressing the portability concern that GNU timeout isn't available on a stock macOS userland. scripts/lib/read-list.sh (the fix itself, shift 2 || true → shift $(($# > 1 ? 2 : 1))) is unchanged since the prior review.

This watchdog operates entirely on local subprocess PIDs captured via $! — no user-controlled input, no injection surface, no secrets/credential handling, and no GitHub Actions workflow file touched. It's a pure test-harness correctness change (avoiding a portability-driven false failure), not a change to any security-relevant surface.