fix: guard possibly-empty array expansions so bash 3.2 can't fake a reviewer cli-error - #139
Merged
Conversation
…eviewer cli-error (#138) Stock macOS ships neither timeout(1) nor gtimeout, so TIMEOUT_CMD is legitimately empty there. Under /bin/bash 3.2 a bare "${ARR[@]}" on an empty array is an unset expansion, so with set -u it aborts with 'unbound variable' before the reviewer ever starts. Every file then returns RC=1 with empty output, the loop counts them all as REVIEW_ERRORS, and the pass resolves to cli-error -- a hard error that ~opt does not excuse -- blocking the merge on a PR no reviewer looked at. Switch TIMEOUT_CMD, MODEL_FLAG, and OLLAMA_FLAGS to the ${ARR[@]+"${ARR[@]}"} form across the ollama, local-agent, and enhance loops; document that an absent timeout binary is the common macOS case and a supported configuration rather than a reviewer failure; and add contract tests that fail if an unguarded expansion returns.
…nd hoist the rule into one partial The mechanical rewrite in the previous commit wrapped three already-guarded expansions a second time, publishing a competing spelling of the very rule this change exists to teach. Collapse them, and move the five near-identical copies of the explanation into lib/empty-array-expansion.md (the lib/gh-host.md convention) so the next amendment can't land in two files and rot in three. The contract test now scans by pattern instead of a hardcoded array-name list, so a newly introduced optional-argument array is covered the day it lands, and it also rejects the double-wrap slip. Its companion no longer pins the loops' prose wording -- it asserts the shared partial carries the contract and that each loop links to it. Registering the new partial in install.sh/uninstall.sh was caught by the existing curl-installer LIBS allowlist test.
…ed guard too
${A[@]+"${A[@]+…}"} is the same slip as ${A[@]+${A[@]+…}} but slipped past the
detector, so the contract test allowed exactly one spelling of the mistake it exists
to reject.
Merged
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
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.
Summary
On a machine with neither
timeoutnorgtimeout— stock macOS, wheretimeoutships only with GNU coreutils andgtimeoutonly via Homebrew —TIMEOUT_CMDis legitimately empty. Under bash 3.2 (still/bin/bashon macOS) a bare"${ARR[@]}"on an empty array is an unset expansion, so withset -uit aborts withunbound variablebefore the reviewer process ever starts.The failure is indistinguishable from a genuine reviewer failure: every file returns RC=1 with empty output, the loop counts them all in
REVIEW_ERRORS, and the pass resolves tocli-error— a hard error that~optdoes not excuse — so the merge is blocked on a PR no reviewer ever looked at.Verified on the actual
GNU bash, version 3.2.57(1)-release:The guarded form also preserves quoting when the array is non-empty, and behaves identically under zsh.
Changes
lib/empty-array-expansion.md(new) — the rule, the bash-3.2 mechanism, why an absenttimeout/gtimeoutis a supported configuration rather than a reviewer failure, and how to recognize the falsecli-errorwhen it appears. One partial, following thelib/gh-host.mdconvention, because the alternative is the same paragraph in three files drifting apart.lib/ollama-review-loop.md— theollama runinvocation now expands bothTIMEOUT_CMDandOLLAMA_FLAGSas${ARR[@]+"${ARR[@]}"}. The total-failure branch now rules out the shell-expansion false positive first: all-files-errored with onlyunbound variablein$ERR_FILEis an environment condition to report and re-run, notcli-error.lib/local-agent-review-loop.md— same expansion fix forTIMEOUT_CMDandMODEL_FLAG(both can legitimately be empty). The step-2 note that claimed "an empty array expands to zero words in bash and zsh alike" was the load-bearing wrong statement; it now says the guard is required, not decoration.lib/enhance-loop.md— same fix forTIMEOUT_CMDandMODEL_FLAG.test/review-loop-contract.test.js— two contract tests. The first scans the three loop partials by pattern (not a hardcoded array-name list, so a newly introduced optional-argument array is covered the day it lands) and rejects both an unguarded"${SOMEARRAY[@]}"and the double-wrapped${A[@]+${A[@]+"${A[@]}"}}slip. The second asserts the shared partial carries the contract and that each loop links to it, deliberately not pinning the loops' own prose wording.install.sh/uninstall.sh— register the new partial in theLIBSallowlist (the npm installer enumerateslib/dynamically; the curl installers do not). The existing allowlist test caught this.Audit of the other partials
A repo-wide sweep for
[@]}found exactly one remaining unguarded expansion:LIST_ARGSincommands/do/next.md:204. Left as-is deliberately — it is initializedLIST_ARGS=(--state open)and only ever appended to, so it can never be empty and has no failure mode here.Test plan
npm test— 187/187 pass (185 before, +2 new contract tests)LIBSallowlist test caught the unregistered new partialagy(antigravity); its findings matched the self-review pass and are applied in967b0b2Closes #138