From 17b80ae41229628c79a392f88a9802dd65b96a88 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 23:08:27 +0000 Subject: [PATCH 1/3] fix: guard possibly-empty array expansions so bash 3.2 can't fake a reviewer 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. --- lib/enhance-loop.md | 15 +++++++++----- lib/local-agent-review-loop.md | 29 +++++++++++++++------------ lib/ollama-review-loop.md | 9 +++++---- test/review-loop-contract.test.js | 33 +++++++++++++++++++++++++++++++ 4 files changed, 64 insertions(+), 22 deletions(-) diff --git a/lib/enhance-loop.md b/lib/enhance-loop.md index e74d102..3f7b4f9 100644 --- a/lib/enhance-loop.md +++ b/lib/enhance-loop.md @@ -107,7 +107,12 @@ logic; run it, don't narrate it: # no wrapper (rely on each CLI's own limits). An ARRAY, not a string: zsh (a common # host shell) does not word-split an unquoted expansion, so a two-word string like # 'timeout 1800' would be executed as one bogus command name; the array expands to -# separate words in bash and zsh alike, and to zero words when empty. Enhancement is +# separate words in bash and zsh alike. Stock macOS ships NEITHER `timeout` nor +# `gtimeout`, so the empty array is the common case — always expand it (and MODEL_FLAG) +# as ${ARR[@]+"${ARR[@]}"}, never as a bare "${ARR[@]}": under bash 3.2 (macOS +# /bin/bash) the latter is an unset expansion that aborts with `unbound variable` +# under `set -u` before the CLI ever runs. The guarded form yields zero words +# when empty, on bash 3.2+ and zsh alike. Enhancement is # lighter than a full review (no build/test), but a large draft on a heavy model can # still exceed the ~10-min host foreground cap, so the same background+poll launch # below is used. @@ -151,11 +156,11 @@ as a positional argument (never via stdin) and prints the improved draft to stdo | `claude` | Dispatch an in-process sub-agent via the `Agent` tool (`subagent_type: "general-purpose"`, prompt `$ENHANCE_PROMPT`, `model` = `{ENH_MODEL}` when set) — **not** `claude -p`, so it stays on the host session's plan billing instead of hitting the API. Its returned message is the agent's stdout. | -| `claude` | `claude -p "$ENHANCE_PROMPT" "${MODEL_FLAG[@]}" --dangerously-skip-permissions` | +| `claude` | `claude -p "$ENHANCE_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --dangerously-skip-permissions` | -| `codex` | `codex "${MODEL_FLAG[@]}" --sandbox read-only -a never exec "$ENHANCE_PROMPT"` — `exec` (free-form prompt) is the right subcommand here, not `codex review`; `-m`/`--model`, `--sandbox`, and `-a` are all top-level flags that MUST precede `exec`. `--sandbox read-only` enforces the read-only contract at the sandbox level while still allowing tree reads and git queries — the same posture `lib/local-agent-review-loop.md` uses for its review-only codex pass (only its *reviewer-applies* path needs `danger-full-access`). | +| `codex` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --sandbox read-only -a never exec "$ENHANCE_PROMPT"` — `exec` (free-form prompt) is the right subcommand here, not `codex review`; `-m`/`--model`, `--sandbox`, and `-a` are all top-level flags that MUST precede `exec`. `--sandbox read-only` enforces the read-only contract at the sandbox level while still allowing tree reads and git queries — the same posture `lib/local-agent-review-loop.md` uses for its review-only codex pass (only its *reviewer-applies* path needs `danger-full-access`). | | `agy` | `agy --dangerously-skip-permissions --model "$AGY_ENH_MODEL" --print-timeout 30m -p "$ENHANCE_PROMPT"` | -| `grok` | `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p "$ENHANCE_PROMPT"` | +| `grok` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p "$ENHANCE_PROMPT"` | **Grok flag rationale.** `grok -p`/`--single ` runs a single-turn headless prompt, prints the response to stdout, and exits — the grok analog of `claude -p` / @@ -247,7 +252,7 @@ one's output): # after <<>> to end-of-output", so any stderr the CLI emits after # the answer (telemetry warnings, timing/shutdown lines, update nags) would be # pasted verbatim into the enhanced draft and end up in the filed issue. - "${TIMEOUT_CMD[@]}" {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" + ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" ``` Then wait with **bounded blocking-chunk foreground calls** — do NOT end your turn to wait for a notification (a stopped subagent is dead, not waiting): diff --git a/lib/local-agent-review-loop.md b/lib/local-agent-review-loop.md index 5365413..7aadd8e 100644 --- a/lib/local-agent-review-loop.md +++ b/lib/local-agent-review-loop.md @@ -78,10 +78,13 @@ fi # codex doesn't have slashdo installed, so describe the task directly). CODEX_APPLY_PROMPT="Review the diff from $BASE_BRANCH to HEAD in this repo for logic issues (correctness, security, test coverage, contract drift). The linter, type-checker, and test suite already run separately — do NOT spend effort on syntax, lint, formatting, or build errors, and do NOT raise style/rename/extract-a-helper suggestions; report only behavior bugs you can tie to a concrete wrong outcome. For each finding, apply the fix in the working tree, then run \`$BUILD_CMD\` (skip if empty) and \`$TEST_CMD\` to verify, and commit each fix with message 'address review (codex): '. Do not introduce changes beyond the scope of fixing the findings. Do not skip tests or weaken assertions." -# Resolve the timeout wrapper used by the step-2 invocation (`"${TIMEOUT_CMD[@]}" {INVOCATION}`). -# macOS ships no `timeout(1)` unless coreutils is installed, so probing is required: -# bare `timeout 1800 …` would exit 127 before the reviewer runs. Empty array = no wrapper -# (rely on the CLI's own internal limits). An ARRAY, not a string, for the same zsh +# Resolve the timeout wrapper used by the step-2 invocation +# (`${TIMEOUT_CMD[@]+${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}} {INVOCATION}`). +# Stock macOS ships NEITHER `timeout(1)` (GNU coreutils) nor `gtimeout` (Homebrew +# coreutils), so an empty array is the common case here, not an edge case; bare +# `timeout 1800 …` would exit 127 before the reviewer runs. Empty array = no wrapper +# (rely on the CLI's own internal limits) — a supported configuration, never a +# reviewer failure. An ARRAY, not a string, for the same zsh # reason as MODEL_FLAG below: zsh does not word-split an unquoted expansion, so a # two-word string ('timeout 1800') would be executed as one bogus command name and # fail every invocation precisely on machines that HAVE coreutils installed. @@ -132,17 +135,17 @@ Pick the invocation based on `{REVIEW_AGENT}` and `{REVIEWER_APPLIES}`: | `claude` | Dispatch an in-process sub-agent via the `Agent` tool with `subagent_type: "general-purpose"` and `$LOCAL_PROMPT` (see Step 2) — **not** `claude -p`, so it stays on plan billing | Same sub-agent dispatch; the sub-agent applies and commits fixes directly in the shared working tree | -| `claude` | `claude -p "$LOCAL_PROMPT" "${MODEL_FLAG[@]}" --dangerously-skip-permissions` | `claude -p "$LOCAL_PROMPT" "${MODEL_FLAG[@]}" --dangerously-skip-permissions` | +| `claude` | `claude -p "$LOCAL_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --dangerously-skip-permissions` | `claude -p "$LOCAL_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --dangerously-skip-permissions` | -| `codex` | `codex "${MODEL_FLAG[@]}" --sandbox read-only review --base "$BASE_BRANCH" --title "$REVIEW_TITLE"` | `codex "${MODEL_FLAG[@]}" --sandbox danger-full-access -a never exec "$CODEX_APPLY_PROMPT"` | +| `codex` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --sandbox read-only review --base "$BASE_BRANCH" --title "$REVIEW_TITLE"` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} --sandbox danger-full-access -a never exec "$CODEX_APPLY_PROMPT"` | | `agy` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m -p "$LOCAL_PROMPT"` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m -p "$LOCAL_PROMPT"` | -| `grok` | `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p "$LOCAL_PROMPT"` | `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p "$LOCAL_PROMPT"` | +| `grok` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p "$LOCAL_PROMPT"` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p "$LOCAL_PROMPT"` | For `claude`, `agy`, and `grok`, the same `$LOCAL_PROMPT` drives both modes — it already encodes the mode (review-only vs reviewer-applies) directly, branching on `$REVIEWER_APPLIES` above. For `codex`, the invocation itself swaps because `codex review` (review-only) and `codex exec` (apply-fixes) are different subcommands with incompatible flag sets. `--print-timeout 30m` raises agy's print-mode wait above its 5-minute default so a real review of a multi-file diff isn't cut off mid-stream; on stock macOS (no `timeout`/`gtimeout`, so `TIMEOUT_CMD` is empty) it is also the only *shell-level* bound on the invocation. **But these bounds only take effect when the invocation runs in the background (Step 2).** Run as a blocking foreground Bash call, the run is killed first by the host tool's ~10-minute foreground cap — earlier than either `timeout 1800` or `--print-timeout 30m` — which is the timeout consumers were hitting. `--print-timeout 30m` does NOT cut off an actively-streaming agent — it bounds the wait for the *next* response chunk — which is why it's safe to set generously, and why it never masked the old skill hang (that hang was the orchestrator sitting idle waiting on background sub-agents, not a slow stream). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight): agy's *default* may be a heavy "Thinking" tier that spends many minutes in hidden reasoning plus multi-round tool calls. How much output is visible meanwhile is **model-dependent** — lighter models narrate their actions incrementally, heavy thinking tiers can emit nothing until the final answer — so on a slow model a routine review shows little or no output for 20-30 minutes and is easily mistaken for a hang. A quiet log during Step 2's poll is therefore NOT evidence the reviewer is stuck; only a `$DONE_FILE` with a non-zero code, or a 30-minute overrun, is. Pinning a fast-but-capable model keeps reviews prompt; bump `AGY_REVIEW_MODEL` to a heavier tier when you want more depth and accept the longer wait (the background launch + 30-minute bound cover it). > **Pass the prompt as a positional argument — never via stdin.** `claude -p`, `agy -p` (`--print`), and `grok -p` (`--single`) all take the prompt as the argument directly after the flag: `agy --dangerously-skip-permissions -p "$LOCAL_PROMPT"`, `grok --permission-mode bypassPermissions -p "$LOCAL_PROMPT"`. They do **not** read the prompt from stdin. Do NOT write `echo "$LOCAL_PROMPT" | agy --dangerously-skip-permissions -p`, `agy -p < prompt.txt`, or `printf … | agy -p` — agy ignores piped stdin and exits with `agy --print takes the prompt as an argument, not stdin`, forcing a wasted second invocation. The `> "$LOG_FILE" 2> "$ERR_FILE"` redirect in Step 2 captures the reviewer's *output*; it is unrelated to how the prompt goes in. Keep `"$LOCAL_PROMPT"` as the quoted argument to `-p` exactly as shown in the invocation table. -**Pinning the reviewer's model (`"${MODEL_FLAG[@]}"` / `--model`).** When `{REVIEW_MODEL}` is set (from an `[]` bracket or a saved `review-models` default — resolved by the caller), the reviewer runs on that model; when empty, `MODEL_FLAG` is an empty array so `codex`/`claude`/`grok` fall back to the CLI's own default. For **codex**, `-m`/`--model` is a **top-level** Codex option (like `--sandbox` and `-a`), so it MUST precede the `review`/`exec` subcommand — that is why `"${MODEL_FLAG[@]}"` sits before `--sandbox` in both codex invocations; passing it after the subcommand would exit 2 with an unexpected-argument error, exactly as `-a` does. (The two paths pass *different* sandbox policies — `read-only` for review-only, `danger-full-access` for reviewer-applies — see below.) For **claude**, `--model` is a session flag valid alongside `-p`. For **grok**, `-m`/`--model` is a session flag valid alongside `-p`, so `"${MODEL_FLAG[@]}"` sits inline in the invocation (empty array → grok's own default). For **agy**, the model is always pinned via `--model "$AGY_REVIEW_MODEL"` (resolved above with `{REVIEW_MODEL}` taking precedence over the `AGY_REVIEW_MODEL` env and the built-in default) — agy's own default may be a slow "Thinking" tier, so it is never left unpinned. Because the model string may contain spaces/parens, `MODEL_FLAG` is a shell array (see the pre-flight block) — never a bare string. +**Pinning the reviewer's model (`${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` / `--model`).** When `{REVIEW_MODEL}` is set (from an `[]` bracket or a saved `review-models` default — resolved by the caller), the reviewer runs on that model; when empty, `MODEL_FLAG` is an empty array so `codex`/`claude`/`grok` fall back to the CLI's own default. For **codex**, `-m`/`--model` is a **top-level** Codex option (like `--sandbox` and `-a`), so it MUST precede the `review`/`exec` subcommand — that is why `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` sits before `--sandbox` in both codex invocations; passing it after the subcommand would exit 2 with an unexpected-argument error, exactly as `-a` does. (The two paths pass *different* sandbox policies — `read-only` for review-only, `danger-full-access` for reviewer-applies — see below.) For **claude**, `--model` is a session flag valid alongside `-p`. For **grok**, `-m`/`--model` is a session flag valid alongside `-p`, so `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` sits inline in the invocation (empty array → grok's own default). For **agy**, the model is always pinned via `--model "$AGY_REVIEW_MODEL"` (resolved above with `{REVIEW_MODEL}` taking precedence over the `AGY_REVIEW_MODEL` env and the built-in default) — agy's own default may be a slow "Thinking" tier, so it is never left unpinned. Because the model string may contain spaces/parens, `MODEL_FLAG` is a shell array (see the pre-flight block) — never a bare string. Notes on each invocation: - **claude / agy / grok** run the self-contained `$LOCAL_PROMPT` (a single-agent inline review), **not** slashdo's `/do-review` skill — the skill's sub-agent fan-out never re-syncs into a print-mode/headless response, so it would hang and emit zero findings (see the `$LOCAL_PROMPT` rationale above). Under Claude Code the `claude` reviewer is an in-process sub-agent (via the `Agent` tool) that runs `$LOCAL_PROMPT` directly, rather than a `claude -p` subprocess — and because the prompt is a single-agent inline review, it does not recursively spawn the skill's own sub-agents. In `REVIEWER_APPLIES=true` mode, `$LOCAL_PROMPT` tells the CLI to apply each fix, verify with build+tests, commit as `address review (): ` (`` = the reviewing CLI's slug, `claude`, `agy`, or `grok`), and NOT push (the orchestrating agent verifies and pushes). The parenthesized agent name records which reviewer surfaced the finding, useful when scanning the log of a release that ran multiple reviewers. In `REVIEWER_APPLIES=false` mode, `$LOCAL_PROMPT` tells the CLI to emit `FINDING :` blocks (or `NO FINDINGS`) to stdout for the orchestrator to parse — the orchestrator then commits the fixes using the same `address review (): ` form to preserve attribution. @@ -157,8 +160,8 @@ Flag rationale (reckless / unattended mode): - **review-only → `read-only`.** Verified: `codex --sandbox read-only review --base ` reads the diff, tracked-file list, commit graph and base tree and returns normal severity-tagged findings, while `printf … > file` inside the repo fails with `zsh:1: operation not permitted`. Review quality is unaffected and the contract becomes unbypassable. This matches `lib/enhance-loop.md`, which already runs codex `--sandbox read-only` for the same reason. - **reviewer-applies → `danger-full-access`.** This path must write fixes, run build/tests, and reach the network unattended, so full access is the intended posture on a trusted single-user machine (mirrors `claude --dangerously-skip-permissions` / `agy --dangerously-skip-permissions`). - `--sandbox` and `-a` are independent top-level flags and may be combined (`codex --sandbox danger-full-access -a never exec …`). -- `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m` — `--dangerously-skip-permissions` auto-approves all tool permission requests so the Antigravity CLI runs unattended (the headless equivalent of confirming every prompt). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight, default `Gemini 3.5 Flash (High)`, override via `AGY_REVIEW_MODEL`): without it agy picks its own default, which may be a heavy "Thinking" tier that spends many minutes in hidden reasoning and — depending on the model, emits little or no visible output meanwhile — makes a review look hung for 20-30 minutes; a fast capable model returns in well under a minute on a small diff. This is the agy successor to the Gemini CLI's `gemini --yolo` + `env GEMINI_SANDBOX=false`: agy folds both "auto-approve tools" and "no sandbox gate" into the single flag, and runs the prompt non-interactively via `-p` — which takes the prompt as its positional argument (`agy … -p "$LOCAL_PROMPT"`), **not** from stdin. Piping into `agy -p` (e.g. `echo … | agy -p`) fails with `agy --print takes the prompt as an argument, not stdin` and wastes an invocation; always pass the quoted prompt as the argument. `--print-timeout 30m` raises the print-mode wait above agy's 5-minute default so a real multi-file review isn't cut off, and — since stock macOS has no `timeout`/`gtimeout` and `TIMEOUT_CMD` is empty — is the effective bound on the invocation; it bounds the wait for the next response chunk, not the total runtime, so an actively-streaming review is never truncated. Unlike the old gemini invocation, no `env VAR=…` prefix is needed, so it composes cleanly with the `"${TIMEOUT_CMD[@]}" {INVOCATION}` wrapper at step 2 of the loop when one is present. -- `grok --permission-mode bypassPermissions "${MODEL_FLAG[@]}" -p` — `-p`/`--single` runs a single-turn headless prompt, prints the response to stdout, and exits (the grok analog of `claude -p` / `agy -p`). `--permission-mode bypassPermissions` auto-approves every tool execution so grok runs unattended (grok's equivalent of `--dangerously-skip-permissions`); it folds "auto-approve tools" into one flag, so no separate sandbox/`env VAR=…` prefix is needed and it composes cleanly with the `"${TIMEOUT_CMD[@]}" {INVOCATION}` wrapper. `"${MODEL_FLAG[@]}"` pins the reviewing model for a `grok[]` bracket (empty array → grok's own default; grok accepts the long `--model` form alongside `-p`). Like `agy -p`, `grok -p` takes the prompt as its positional argument — **not** from stdin (`grok … -p "$LOCAL_PROMPT"`); do not pipe into it. Grok has no `--print-timeout` equivalent, so the run is bounded by `TIMEOUT_CMD` (when present) and grok's own internal limits — the same background-launch + poll in Step 2 keeps it off the host's ~10-minute foreground cap. +- `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m` — `--dangerously-skip-permissions` auto-approves all tool permission requests so the Antigravity CLI runs unattended (the headless equivalent of confirming every prompt). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight, default `Gemini 3.5 Flash (High)`, override via `AGY_REVIEW_MODEL`): without it agy picks its own default, which may be a heavy "Thinking" tier that spends many minutes in hidden reasoning and — depending on the model, emits little or no visible output meanwhile — makes a review look hung for 20-30 minutes; a fast capable model returns in well under a minute on a small diff. This is the agy successor to the Gemini CLI's `gemini --yolo` + `env GEMINI_SANDBOX=false`: agy folds both "auto-approve tools" and "no sandbox gate" into the single flag, and runs the prompt non-interactively via `-p` — which takes the prompt as its positional argument (`agy … -p "$LOCAL_PROMPT"`), **not** from stdin. Piping into `agy -p` (e.g. `echo … | agy -p`) fails with `agy --print takes the prompt as an argument, not stdin` and wastes an invocation; always pass the quoted prompt as the argument. `--print-timeout 30m` raises the print-mode wait above agy's 5-minute default so a real multi-file review isn't cut off, and — since stock macOS has no `timeout`/`gtimeout` and `TIMEOUT_CMD` is empty — is the effective bound on the invocation; it bounds the wait for the next response chunk, not the total runtime, so an actively-streaming review is never truncated. Unlike the old gemini invocation, no `env VAR=…` prefix is needed, so it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper at step 2 of the loop when one is present. +- `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p` — `-p`/`--single` runs a single-turn headless prompt, prints the response to stdout, and exits (the grok analog of `claude -p` / `agy -p`). `--permission-mode bypassPermissions` auto-approves every tool execution so grok runs unattended (grok's equivalent of `--dangerously-skip-permissions`); it folds "auto-approve tools" into one flag, so no separate sandbox/`env VAR=…` prefix is needed and it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper. `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` pins the reviewing model for a `grok[]` bracket (empty array → grok's own default; grok accepts the long `--model` form alongside `-p`). Like `agy -p`, `grok -p` takes the prompt as its positional argument — **not** from stdin (`grok … -p "$LOCAL_PROMPT"`); do not pipe into it. Grok has no `--print-timeout` equivalent, so the run is bounded by `TIMEOUT_CMD` (when present) and grok's own internal limits — the same background-launch + poll in Step 2 keeps it off the host's ~10-minute foreground cap. Because these flags grant the headless CLI full unattended write access to the working tree — and the Claude-Code sub-agent likewise shares this working tree — the verify step in this loop (build + tests + diff inspection by the main thread) is mandatory and non-skippable — it is the only line of defense between the reviewing agent's output and the remote branch. This applies in *both* editing modes: in review-only mode the orchestrator's own fixes are still verified before push, because the orchestrator may misread the CLI's findings or introduce its own regressions. @@ -207,7 +210,7 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro LOG_FILE="$(mktemp -t local-review-${REVIEW_AGENT}.XXXXXX.log)" ERR_FILE="${LOG_FILE}.err" DONE_FILE="${LOG_FILE}.exit" - "${TIMEOUT_CMD[@]}" {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" + ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE"; echo $? > "$DONE_FILE" ``` **Keep stderr OUT of `$LOG_FILE` (`2> "$ERR_FILE"`, never `2>&1`).** Step 3 validates `$LOG_FILE` as a *strict* verdict document — it must hold nothing but `NO FINDINGS` or complete `FINDING :` blocks, and anything else is a parse failure. Every CLI writes non-verdict chatter to stderr (startup and deprecation banners, auth notices, agy/grok progress narration, a `timeout` kill message), so merging the streams would let one stray banner turn a perfectly clean review into a parse failure that blocks the merge. Same split, same reason, as `lib/ollama-review-loop.md`. @@ -227,11 +230,11 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro ```bash LOG_FILE="$(mktemp -t local-review-${REVIEW_AGENT}.XXXXXX.log)" ERR_FILE="${LOG_FILE}.err" - "${TIMEOUT_CMD[@]}" {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE" + ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION} > "$LOG_FILE" 2> "$ERR_FILE" EXIT_CODE=$? ``` - - `TIMEOUT_CMD` was already resolved during pre-flight (the array `(timeout 1800)`, `(gtimeout 1800)`, or empty). Just expand it as `"${TIMEOUT_CMD[@]}"` — an empty array expands to zero words in bash and zsh alike, becoming a direct invocation. No re-checking or commentary needed. + - `TIMEOUT_CMD` was already resolved during pre-flight (the array `(timeout 1800)`, `(gtimeout 1800)`, or empty; on stock macOS it is empty, which is a supported configuration and never a reviewer failure). Expand it exactly as `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}` — the `${ARR[@]+…}` guard is required, **not** decoration: an unguarded `"${ARR[@]}"` on an empty array is an *unset* expansion under **bash 3.2** (still `/bin/bash` on macOS), so with `set -u` it aborts with `unbound variable` before the reviewer starts, turning every file into an empty RC=1 result and a false `cli-error` that blocks the merge. The guarded form expands to zero words — a direct invocation — under bash 3.2+, bash 4/5, and zsh alike. Same rule for `MODEL_FLAG` and any other array that can legitimately be empty. No re-checking or commentary needed. - If `EXIT_CODE != 0` and the CLI produced no commits, set `STATUS=cli-error`, print the last 80 lines of **`$ERR_FILE`** (that is where a failing CLI writes its diagnostics now that the streams are split — fall back to `$LOG_FILE` if `$ERR_FILE` is empty), and exit the loop. Surface both paths so the user can inspect. A `124` exit (from `timeout`/`gtimeout`) or an empty log after the poll loop gave up means the review genuinely ran past 30 minutes — report it as `cli-error` with the log paths, do not record `clean`. 3. **Detect changes and apply fixes** (logic depends on `{REVIEWER_APPLIES}`): diff --git a/lib/ollama-review-loop.md b/lib/ollama-review-loop.md index 89f8f45..63dee60 100644 --- a/lib/ollama-review-loop.md +++ b/lib/ollama-review-loop.md @@ -22,12 +22,13 @@ When to use this: 3. **Resolve `{OLLAMA_MODEL}`** (see "Model resolution" below). If resolution yields no usable model, set `STATUS=skipped` and return. 4. Force review-only: set `REVIEWER_APPLIES=false` regardless of what the caller passed. If the caller passed `--reviewer-applies`, print: `--reviewer-applies has no effect on the ollama pass; Ollama is non-agentic, so the orchestrator always applies the fixes.` 5. Record `{REPO_DIR}` (`git rev-parse --show-toplevel`), `{BRANCH_NAME}` (`git branch --show-current`), `{BASE_BRANCH}`, `{BUILD_CMD}`, and `{TEST_CMD}`. Also record `{MAX_ITERATIONS}` — how many review → fix → re-review cycles this reviewer may run, resolved by the caller (the multi-reviewer loop: a per-entry `~max=` suffix on the `--review-with` token → this loop's built-in default of `3`). **Defaults to `3`**; `0` means **unlimited**, bounded by the 10-iteration safety guardrail in the Loop's step 6. Local models are the most common reason to want a small cap — `ollama~max=1` buys one review-and-fix pass without paying for re-review rounds on slow hardware. Record `{MAX_EXPLICIT}` alongside it — `true` only when the cap came from a `~max=` the user typed or saved — which step 6 uses to distinguish `capped` (a budget the user chose, clean-equivalent for the merge gate) from `guardrail` (a built-in ceiling, inconclusive). The `--review-iterations` flag never reaches this loop; `~max` is the only way to move this cap. -6. **Resolve the timeout wrapper.** macOS ships no `timeout(1)` unless coreutils is installed, so probing is required; an empty array = no wrapper (rely on Ollama's own limits). An ARRAY, not a string, for the same zsh reason as `OLLAMA_FLAGS` below: zsh does not word-split an unquoted expansion, so a two-word string (`timeout 600`) would be executed as one bogus command name and fail every invocation precisely on machines that HAVE coreutils installed. Settled logic — run it verbatim, do NOT narrate the probe or the fallback: +6. **Resolve the timeout wrapper.** Stock macOS ships **neither** `timeout(1)` (GNU coreutils) nor `gtimeout` (the Homebrew-prefixed coreutils build), so an empty `TIMEOUT_CMD` is the *common* case on a Mac, not an edge case. An empty array = no wrapper (rely on Ollama's own limits), which is a **supported configuration**: the review runs unbounded and must never be recorded as a reviewer failure. An ARRAY, not a string, for the same zsh reason as `OLLAMA_FLAGS` below: zsh does not word-split an unquoted expansion, so a two-word string (`timeout 600`) would be executed as one bogus command name and fail every invocation precisely on machines that HAVE coreutils installed. Settled logic — run it verbatim, do NOT narrate the probe or the fallback: ```bash TIMEOUT_CMD=() if command -v timeout >/dev/null 2>&1; then TIMEOUT_CMD=(timeout 600) elif command -v gtimeout >/dev/null 2>&1; then TIMEOUT_CMD=(gtimeout 600); fi ``` + **Always expand a possibly-empty array as `${ARR[@]+"${ARR[@]}"}`, never as a bare `"${ARR[@]}"`.** Under **bash 3.2** — still `/bin/bash` on macOS — a plain `"${ARR[@]}"` on an empty array is an *unset* expansion, so with `set -u` it aborts with `unbound variable` **before the command ever runs**. Every invocation then returns RC=1 with empty output, the loop counts them all in `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 ever looked at. The `${ARR[@]+…}` form suppresses the expansion entirely when the array is unset/empty and is safe under bash 3.2+, bash 4/5, and zsh alike. This applies to `TIMEOUT_CMD` and `OLLAMA_FLAGS` below, and to any other array that can legitimately be empty. 7. **Select the structured-output format.** The review asks the model for JSON so the orchestrator parses a data structure instead of scraping a free-text format. Define the schema once and pick the strongest mode the installed Ollama supports — schema-constrained outputs require Ollama ≥ 0.5.0: ```bash FINDINGS_SCHEMA='{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"file":{"type":"string"},"line":{"type":"integer"},"severity":{"type":"string","enum":["CRITICAL","IMPROVEMENT","NIT"]},"description":{"type":"string"},"fix":{"type":"string"}},"required":["file","line","severity","description","fix"]}}},"required":["findings"]}' @@ -118,16 +119,16 @@ If the diff has no logic issues worth raising, return {\"findings\": []}. --- DIFF --- $FILE_DIFF" - RESP=$(printf '%s' "$PROMPT" | "${TIMEOUT_CMD[@]}" ollama run "${OLLAMA_FLAGS[@]}" "$OLLAMA_MODEL" 2>> "$ERR_FILE") + RESP=$(printf '%s' "$PROMPT" | ${TIMEOUT_CMD[@]+${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}} ollama run ${OLLAMA_FLAGS[@]+${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"}} "$OLLAMA_MODEL" 2>> "$ERR_FILE") RC=$? printf '\n===== FILE: %s =====\n%s\n' "$F" "$RESP" >> "$LOG_FILE" ``` Three things keep the captured findings a clean, parseable data structure rather than buried in terminal noise: - **`--format "$OLLAMA_FORMAT"` (probed into `$OLLAMA_FLAGS`).** When supported, grammar-constrains the output to JSON (and, in schema mode, to the exact field names and the `severity` enum). This is the single biggest reliability win: the orchestrator parses a structure instead of scraping a free-text format that local models adhere to unreliably. On a client too old to support `--format` the flag is dropped (see the pre-flight probe) and the loop falls back to prompt-only JSON, parsed leniently. The delimiter lines (`===== FILE: =====`, the path embedded per file) let you split `$LOG_FILE` into one JSON object per reviewed file — split on the regex `^===== FILE: (.+) =====$`, capturing the path. - **`2>> "$ERR_FILE"` (not `2>&1`).** `ollama run` writes the actual model response to **stdout** but renders its progress spinner — braille frames (`⠙ ⠹ ⠼`) wrapped in ANSI cursor codes (`\e[?25l`, `\e[1G`, `\e[K`) — to **stderr**. Merging the two with `2>&1` is what fills the log with spinner garbage. Send stderr to a separate file so `$LOG_FILE` holds only model JSON; no ANSI stripping pass is then needed. - - **`"${OLLAMA_FLAGS[@]}"` (`--format`, `--hidethinking`, `--nowordwrap`).** Probed for support in pre-flight and included only when present (as a shell array, so it expands to separate words under both bash and zsh, and to nothing when empty) — passing a flag an older `ollama run` doesn't recognize makes it exit non-zero and error the whole pass. `--hidethinking` suppresses reasoning models' (e.g. `qwen3`, `deepseek-r1`) chain-of-thought, which can otherwise precede the constrained JSON even under `--format`; it is a no-op on non-thinking *models* but is absent on older ollama *versions* (~0.5–0.8), hence the probe. `--nowordwrap` stops Ollama hard-wrapping long lines to the terminal width, which would otherwise inject newlines into a long `fix` string. + - **`${OLLAMA_FLAGS[@]+${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"}}` (`--format`, `--hidethinking`, `--nowordwrap`).** Probed for support in pre-flight and included only when present (as a shell array, so it expands to separate words under both bash and zsh, and — in this `${ARR[@]+…}` form — to nothing when empty, without tripping `set -u` under bash 3.2; see pre-flight step 6) — passing a flag an older `ollama run` doesn't recognize makes it exit non-zero and error the whole pass. `--hidethinking` suppresses reasoning models' (e.g. `qwen3`, `deepseek-r1`) chain-of-thought, which can otherwise precede the constrained JSON even under `--format`; it is a no-op on non-thinking *models* but is absent on older ollama *versions* (~0.5–0.8), hence the probe. `--nowordwrap` stops Ollama hard-wrapping long lines to the terminal width, which would otherwise inject newlines into a long `fix` string. 4. Treat a file as a failed (zero-coverage) review when **either** `RC != 0` **or** `$RESP` is empty/whitespace-only (`[ -z "$(printf '%s' "$RESP" | tr -d '[:space:]')" ]`). The empty-but-exit-0 case is real and silent: a reasoning model can spend its whole token budget on hidden thinking (`--hidethinking`) and emit no JSON, yet `ollama run` still exits 0 — observed with `qwen3.6:35b`. Without this guard the empty section parses as "no findings" and the file is miscounted as cleanly reviewed. On either condition, append a `[ollama error reviewing $F — RC=$RC, empty=$([ -z "$(printf '%s' "$RESP" | tr -d '[:space:]')" ] && echo yes || echo no); see $ERR_FILE]` marker to the log, **increment `REVIEW_ERRORS`**, and continue to the next file (one file's failure should not abort the whole review). Coverage accounting after the loop — first recompute `REVIEWABLE=$((TOTAL_FILES - SKIPPED_EMPTY))` (the files actually sent to the model; empty-diff skips never reached it), then define a coverage gap as any reviewable file that errored or was truncated (`REVIEW_ERRORS + TRUNCATED > 0`): - - If *every reviewable* file errored (`REVIEWABLE > 0` and `REVIEW_ERRORS == REVIEWABLE`), set `STATUS=cli-error`, print the last 80 lines of `$ERR_FILE` (genuine ollama errors live there, not in `$LOG_FILE`; but in the exit-0 empty-response failure mode `$ERR_FILE` may hold only spinner noise, so also surface the per-file `[ollama error reviewing …]` markers from `$LOG_FILE`), and exit — nothing was reviewed. (Use `REVIEWABLE`, not `TOTAL_FILES`: an empty-diff skip would otherwise make `REVIEW_ERRORS == TOTAL_FILES` unreachable and misclassify a total failure as merely `incomplete`.) + - If *every reviewable* file errored (`REVIEWABLE > 0` and `REVIEW_ERRORS == REVIEWABLE`), set `STATUS=cli-error`, print the last 80 lines of `$ERR_FILE` (genuine ollama errors live there, not in `$LOG_FILE`; but in the exit-0 empty-response failure mode `$ERR_FILE` may hold only spinner noise, so also surface the per-file `[ollama error reviewing …]` markers from `$LOG_FILE`), and exit — nothing was reviewed. **Before reporting it, rule out the shell-expansion false positive**: every file erroring with an empty response and nothing but `unbound variable` in `$ERR_FILE` is the bash-3.2 empty-array symptom (pre-flight step 6), not a reviewer failure — an absent `timeout`/`gtimeout` is a supported environment condition, so print `ollama: no timeout/gtimeout on this machine — running unbounded` once and re-run the pass with the `${ARR[@]+"${ARR[@]}"}` expansion rather than recording `cli-error`. (Use `REVIEWABLE`, not `TOTAL_FILES`: an empty-diff skip would otherwise make `REVIEW_ERRORS == TOTAL_FILES` unreachable and misclassify a total failure as merely `incomplete`.) - JSON parse errors are accounted for after the per-file invocations (see the defensive parsing rule below). Like invocation errors and truncation, they are coverage gaps: a non-empty response that cannot be parsed is not evidence that the file was clean. - If there is any coverage gap but not a total failure (`REVIEW_ERRORS + PARSE_ERRORS + TRUNCATED > 0` and `REVIEW_ERRORS + PARSE_ERRORS < REVIEWABLE`), the diff was only **partially** reviewed. Still process the findings from the parts that were reviewed (apply their fixes in step 3), but the pass **must not report `clean`** — at the point step 3 would set `STATUS=clean`, set `STATUS=incomplete` instead (see step 3). `incomplete` is treated as inconclusive by the multi-reviewer aggregate (not eligible to merge), because part of the change was never reviewed. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 1402ec9..110aa0a 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -241,4 +241,37 @@ describe('review-loop parse contracts', () => { assert.match(pr, /\s+exit 1/); assert.match(pr, /Never merge on `dirty`\/`inconclusive`, never merge while the branch has unpushed commits/); }); + + it('guards every possibly-empty array expansion against bash 3.2 + set -u', () => { + // Stock macOS has neither `timeout` nor `gtimeout`, so TIMEOUT_CMD is legitimately + // empty there — and under /bin/bash 3.2 a bare "${ARR[@]}" on an empty array is an + // UNSET expansion that aborts with `unbound variable` before the reviewer ever runs. + // Every file then comes back RC=1 with empty output, the loop counts them all as + // REVIEW_ERRORS, and the pass resolves to `cli-error` — a hard error `~opt` does not + // excuse — blocking the merge on a PR no reviewer looked at. Only the + // ${ARR[@]+"${ARR[@]}"} form is safe on bash 3.2, bash 4/5, and zsh alike. + const arrays = ['TIMEOUT_CMD', 'MODEL_FLAG', 'OLLAMA_FLAGS']; + for (const name of ['local-agent-review-loop.md', 'ollama-review-loop.md', 'enhance-loop.md']) { + const body = readLib(name); + for (const arr of arrays) { + assert.doesNotMatch( + body, + new RegExp(String.raw`(? { + // An absent timeout binary is an environment condition, not a reviewer failure — + // the loops must say so, or the next reader re-reports it as a genuine cli-error. + const ollama = readLib('ollama-review-loop.md'); + assert.match(ollama, /Stock macOS ships \*\*neither\*\* `timeout\(1\)`.*nor `gtimeout`/); + assert.match(ollama, /supported configuration\*\*: the review runs unbounded and must never be recorded as a reviewer failure/); + + const local = readLib('local-agent-review-loop.md'); + assert.match(local, /Stock macOS ships NEITHER `timeout\(1\)`/); + assert.match(local, /a supported configuration and never a reviewer failure/); + }); }); From 967b0b2fa8fec19a49573559f7718aa9258b2fa4 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 23:14:24 +0000 Subject: [PATCH 2/3] address review (claude+antigravity): collapse double-wrapped guards and 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. --- install.sh | 2 +- lib/empty-array-expansion.md | 41 ++++++++++++++++++++ lib/enhance-loop.md | 19 +++++----- lib/local-agent-review-loop.md | 8 ++-- lib/ollama-review-loop.md | 8 ++-- test/review-loop-contract.test.js | 62 ++++++++++++++++++++++--------- uninstall.sh | 2 +- 7 files changed, 105 insertions(+), 37 deletions(-) create mode 100644 lib/empty-array-expansion.md diff --git a/install.sh b/install.sh index a8707a9..8c2da6d 100755 --- a/install.sh +++ b/install.sh @@ -60,7 +60,7 @@ OLD_COMMANDS=(cam good makegoals makegood optimize-md) # enumerates lib/ dynamically, so it doesn't need updating. LIBS=( ci-flake-handling code-review-checklist copilot-review-loop - enhance-loop epic-children + empty-array-expansion enhance-loop epic-children finding-disposition fix-regression-guard gh-host github-reviewer-loop graphql-escaping local-agent-review-loop multi-reviewer-loop ollama-review-loop diff --git a/lib/empty-array-expansion.md b/lib/empty-array-expansion.md new file mode 100644 index 0000000..66e2649 --- /dev/null +++ b/lib/empty-array-expansion.md @@ -0,0 +1,41 @@ +## Expanding a possibly-empty shell array (`${ARR[@]+"${ARR[@]}"}`) + +**The rule.** Any array that can legitimately be empty — `TIMEOUT_CMD`, `MODEL_FLAG`, +`OLLAMA_FLAGS`, or any other optional-argument array — must be expanded as: + +```bash +${ARR[@]+"${ARR[@]}"} # correct +"${ARR[@]}" # WRONG when ARR can be empty +``` + +**Why the guard is required, not decoration.** Under **bash 3.2** — still `/bin/bash` +on macOS — a bare `"${ARR[@]}"` on an *empty* array is an **unset** expansion, so with +`set -u` the shell aborts with `ARR[@]: unbound variable` **before the command runs**: + +``` +$ /bin/bash -c 'set -u; A=(); "${A[@]}" echo ok' +/bin/bash: A[@]: unbound variable # exit 127 — echo never ran + +$ /bin/bash -c 'set -u; A=(); ${A[@]+"${A[@]}"} echo ok' +ok # exit 0 +``` + +The `${ARR[@]+…}` form suppresses the expansion entirely when the array is unset or +empty, and is identical to `"${ARR[@]}"` when it is not — quoting and word boundaries +are preserved, so an element containing spaces still arrives as one argument. It is +safe on bash 3.2+, bash 4/5, and zsh alike, so it is the only form to write. + +**Why it matters here specifically.** Stock macOS ships **neither** `timeout(1)` (GNU +coreutils) nor `gtimeout` (the Homebrew-prefixed coreutils build), so a probing +`TIMEOUT_CMD=()` stays empty on a typical Mac — the *common* case, not an edge case. +Running without a timeout wrapper is a **supported configuration** (the invocation is +bounded by the CLI's own limits instead) and must never be recorded as a reviewer +failure. But with an unguarded expansion the review loops fail in the worst possible +way: every invocation aborts before the reviewer starts, returning RC=1 with empty +output, so the loop counts each file as a review error and the pass resolves to +`cli-error` — a hard error that `~opt` does **not** excuse — blocking the merge on a +PR no reviewer ever looked at. + +**Diagnosing it.** Every file erroring with an empty response, and nothing but +`unbound variable` in the captured stderr, is this bug — an environment condition, not +a reviewer failure. Fix the expansion and re-run the pass; do not record `cli-error`. diff --git a/lib/enhance-loop.md b/lib/enhance-loop.md index 3f7b4f9..4be6bdc 100644 --- a/lib/enhance-loop.md +++ b/lib/enhance-loop.md @@ -103,16 +103,15 @@ Resolve the timeout wrapper — the only genuinely run-once piece; this is settl logic; run it, don't narrate it: ```bash -# macOS ships no timeout(1) unless coreutils is installed; probe for it. Empty array = -# no wrapper (rely on each CLI's own limits). An ARRAY, not a string: zsh (a common -# host shell) does not word-split an unquoted expansion, so a two-word string like -# 'timeout 1800' would be executed as one bogus command name; the array expands to -# separate words in bash and zsh alike. Stock macOS ships NEITHER `timeout` nor -# `gtimeout`, so the empty array is the common case — always expand it (and MODEL_FLAG) -# as ${ARR[@]+"${ARR[@]}"}, never as a bare "${ARR[@]}": under bash 3.2 (macOS -# /bin/bash) the latter is an unset expansion that aborts with `unbound variable` -# under `set -u` before the CLI ever runs. The guarded form yields zero words -# when empty, on bash 3.2+ and zsh alike. Enhancement is +# Stock macOS ships NEITHER timeout(1) (GNU coreutils) nor gtimeout (Homebrew +# coreutils), so probe for them and expect the empty array — the common case, not an +# edge case. Empty array = no wrapper (rely on each CLI's own limits). An ARRAY, not a +# string: zsh (a common host shell) does not word-split an unquoted expansion, so a +# two-word string like 'timeout 1800' would be executed as one bogus command name; the +# array expands to separate words in bash and zsh alike. Expand it (and MODEL_FLAG) +# only in the guarded ${ARR[@]+"${ARR[@]}"} form — the bare form aborts under bash 3.2 +# + `set -u` before the CLI runs; see ~/.claude/lib/empty-array-expansion.md. +# Enhancement is # lighter than a full review (no build/test), but a large draft on a heavy model can # still exceed the ~10-min host foreground cap, so the same background+poll launch # below is used. diff --git a/lib/local-agent-review-loop.md b/lib/local-agent-review-loop.md index 7aadd8e..3704583 100644 --- a/lib/local-agent-review-loop.md +++ b/lib/local-agent-review-loop.md @@ -79,12 +79,14 @@ fi CODEX_APPLY_PROMPT="Review the diff from $BASE_BRANCH to HEAD in this repo for logic issues (correctness, security, test coverage, contract drift). The linter, type-checker, and test suite already run separately — do NOT spend effort on syntax, lint, formatting, or build errors, and do NOT raise style/rename/extract-a-helper suggestions; report only behavior bugs you can tie to a concrete wrong outcome. For each finding, apply the fix in the working tree, then run \`$BUILD_CMD\` (skip if empty) and \`$TEST_CMD\` to verify, and commit each fix with message 'address review (codex): '. Do not introduce changes beyond the scope of fixing the findings. Do not skip tests or weaken assertions." # Resolve the timeout wrapper used by the step-2 invocation -# (`${TIMEOUT_CMD[@]+${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}} {INVOCATION}`). +# (`${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}`). # Stock macOS ships NEITHER `timeout(1)` (GNU coreutils) nor `gtimeout` (Homebrew # coreutils), so an empty array is the common case here, not an edge case; bare # `timeout 1800 …` would exit 127 before the reviewer runs. Empty array = no wrapper # (rely on the CLI's own internal limits) — a supported configuration, never a -# reviewer failure. An ARRAY, not a string, for the same zsh +# reviewer failure; expand it (and MODEL_FLAG) only in the guarded +# ${ARR[@]+"${ARR[@]}"} form — see ~/.claude/lib/empty-array-expansion.md. +# An ARRAY, not a string, for the same zsh # reason as MODEL_FLAG below: zsh does not word-split an unquoted expansion, so a # two-word string ('timeout 1800') would be executed as one bogus command name and # fail every invocation precisely on machines that HAVE coreutils installed. @@ -234,7 +236,7 @@ Initialize `ITERATION=0`, `STATUS=""`, and `MAX_ITERATIONS` / `MAX_EXPLICIT` fro EXIT_CODE=$? ``` - - `TIMEOUT_CMD` was already resolved during pre-flight (the array `(timeout 1800)`, `(gtimeout 1800)`, or empty; on stock macOS it is empty, which is a supported configuration and never a reviewer failure). Expand it exactly as `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}` — the `${ARR[@]+…}` guard is required, **not** decoration: an unguarded `"${ARR[@]}"` on an empty array is an *unset* expansion under **bash 3.2** (still `/bin/bash` on macOS), so with `set -u` it aborts with `unbound variable` before the reviewer starts, turning every file into an empty RC=1 result and a false `cli-error` that blocks the merge. The guarded form expands to zero words — a direct invocation — under bash 3.2+, bash 4/5, and zsh alike. Same rule for `MODEL_FLAG` and any other array that can legitimately be empty. No re-checking or commentary needed. + - `TIMEOUT_CMD` was already resolved during pre-flight (the array `(timeout 1800)`, `(gtimeout 1800)`, or empty; on stock macOS it is empty, which is a supported configuration and never a reviewer failure). Expand it exactly as `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}` — the guard is required, **not** decoration: the bare form aborts under bash 3.2 + `set -u` before the reviewer starts, which surfaces as a false `cli-error` that blocks the merge (see `~/.claude/lib/empty-array-expansion.md`). Same rule for `MODEL_FLAG`. No re-checking or commentary needed. - If `EXIT_CODE != 0` and the CLI produced no commits, set `STATUS=cli-error`, print the last 80 lines of **`$ERR_FILE`** (that is where a failing CLI writes its diagnostics now that the streams are split — fall back to `$LOG_FILE` if `$ERR_FILE` is empty), and exit the loop. Surface both paths so the user can inspect. A `124` exit (from `timeout`/`gtimeout`) or an empty log after the poll loop gave up means the review genuinely ran past 30 minutes — report it as `cli-error` with the log paths, do not record `clean`. 3. **Detect changes and apply fixes** (logic depends on `{REVIEWER_APPLIES}`): diff --git a/lib/ollama-review-loop.md b/lib/ollama-review-loop.md index 63dee60..f187be6 100644 --- a/lib/ollama-review-loop.md +++ b/lib/ollama-review-loop.md @@ -28,7 +28,7 @@ When to use this: if command -v timeout >/dev/null 2>&1; then TIMEOUT_CMD=(timeout 600) elif command -v gtimeout >/dev/null 2>&1; then TIMEOUT_CMD=(gtimeout 600); fi ``` - **Always expand a possibly-empty array as `${ARR[@]+"${ARR[@]}"}`, never as a bare `"${ARR[@]}"`.** Under **bash 3.2** — still `/bin/bash` on macOS — a plain `"${ARR[@]}"` on an empty array is an *unset* expansion, so with `set -u` it aborts with `unbound variable` **before the command ever runs**. Every invocation then returns RC=1 with empty output, the loop counts them all in `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 ever looked at. The `${ARR[@]+…}` form suppresses the expansion entirely when the array is unset/empty and is safe under bash 3.2+, bash 4/5, and zsh alike. This applies to `TIMEOUT_CMD` and `OLLAMA_FLAGS` below, and to any other array that can legitimately be empty. + **Always expand a possibly-empty array as `${ARR[@]+"${ARR[@]}"}`, never as a bare `"${ARR[@]}"`** — the guard is required, not decoration: the bare form aborts under bash 3.2 + `set -u` before the command runs, which surfaces as a false `cli-error`. Applies to `TIMEOUT_CMD` and `OLLAMA_FLAGS` below, and to any other array that can legitimately be empty. See `~/.claude/lib/empty-array-expansion.md`. 7. **Select the structured-output format.** The review asks the model for JSON so the orchestrator parses a data structure instead of scraping a free-text format. Define the schema once and pick the strongest mode the installed Ollama supports — schema-constrained outputs require Ollama ≥ 0.5.0: ```bash FINDINGS_SCHEMA='{"type":"object","properties":{"findings":{"type":"array","items":{"type":"object","properties":{"file":{"type":"string"},"line":{"type":"integer"},"severity":{"type":"string","enum":["CRITICAL","IMPROVEMENT","NIT"]},"description":{"type":"string"},"fix":{"type":"string"}},"required":["file","line","severity","description","fix"]}}},"required":["findings"]}' @@ -119,16 +119,16 @@ If the diff has no logic issues worth raising, return {\"findings\": []}. --- DIFF --- $FILE_DIFF" - RESP=$(printf '%s' "$PROMPT" | ${TIMEOUT_CMD[@]+${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"}} ollama run ${OLLAMA_FLAGS[@]+${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"}} "$OLLAMA_MODEL" 2>> "$ERR_FILE") + RESP=$(printf '%s' "$PROMPT" | ${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} ollama run ${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"} "$OLLAMA_MODEL" 2>> "$ERR_FILE") RC=$? printf '\n===== FILE: %s =====\n%s\n' "$F" "$RESP" >> "$LOG_FILE" ``` Three things keep the captured findings a clean, parseable data structure rather than buried in terminal noise: - **`--format "$OLLAMA_FORMAT"` (probed into `$OLLAMA_FLAGS`).** When supported, grammar-constrains the output to JSON (and, in schema mode, to the exact field names and the `severity` enum). This is the single biggest reliability win: the orchestrator parses a structure instead of scraping a free-text format that local models adhere to unreliably. On a client too old to support `--format` the flag is dropped (see the pre-flight probe) and the loop falls back to prompt-only JSON, parsed leniently. The delimiter lines (`===== FILE: =====`, the path embedded per file) let you split `$LOG_FILE` into one JSON object per reviewed file — split on the regex `^===== FILE: (.+) =====$`, capturing the path. - **`2>> "$ERR_FILE"` (not `2>&1`).** `ollama run` writes the actual model response to **stdout** but renders its progress spinner — braille frames (`⠙ ⠹ ⠼`) wrapped in ANSI cursor codes (`\e[?25l`, `\e[1G`, `\e[K`) — to **stderr**. Merging the two with `2>&1` is what fills the log with spinner garbage. Send stderr to a separate file so `$LOG_FILE` holds only model JSON; no ANSI stripping pass is then needed. - - **`${OLLAMA_FLAGS[@]+${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"}}` (`--format`, `--hidethinking`, `--nowordwrap`).** Probed for support in pre-flight and included only when present (as a shell array, so it expands to separate words under both bash and zsh, and — in this `${ARR[@]+…}` form — to nothing when empty, without tripping `set -u` under bash 3.2; see pre-flight step 6) — passing a flag an older `ollama run` doesn't recognize makes it exit non-zero and error the whole pass. `--hidethinking` suppresses reasoning models' (e.g. `qwen3`, `deepseek-r1`) chain-of-thought, which can otherwise precede the constrained JSON even under `--format`; it is a no-op on non-thinking *models* but is absent on older ollama *versions* (~0.5–0.8), hence the probe. `--nowordwrap` stops Ollama hard-wrapping long lines to the terminal width, which would otherwise inject newlines into a long `fix` string. + - **`${OLLAMA_FLAGS[@]+"${OLLAMA_FLAGS[@]}"}` (`--format`, `--hidethinking`, `--nowordwrap`).** Probed for support in pre-flight and included only when present (as a shell array, so it expands to separate words under both bash and zsh, and — in this `${ARR[@]+…}` form — to nothing when empty, without tripping `set -u` under bash 3.2; see pre-flight step 6) — passing a flag an older `ollama run` doesn't recognize makes it exit non-zero and error the whole pass. `--hidethinking` suppresses reasoning models' (e.g. `qwen3`, `deepseek-r1`) chain-of-thought, which can otherwise precede the constrained JSON even under `--format`; it is a no-op on non-thinking *models* but is absent on older ollama *versions* (~0.5–0.8), hence the probe. `--nowordwrap` stops Ollama hard-wrapping long lines to the terminal width, which would otherwise inject newlines into a long `fix` string. 4. Treat a file as a failed (zero-coverage) review when **either** `RC != 0` **or** `$RESP` is empty/whitespace-only (`[ -z "$(printf '%s' "$RESP" | tr -d '[:space:]')" ]`). The empty-but-exit-0 case is real and silent: a reasoning model can spend its whole token budget on hidden thinking (`--hidethinking`) and emit no JSON, yet `ollama run` still exits 0 — observed with `qwen3.6:35b`. Without this guard the empty section parses as "no findings" and the file is miscounted as cleanly reviewed. On either condition, append a `[ollama error reviewing $F — RC=$RC, empty=$([ -z "$(printf '%s' "$RESP" | tr -d '[:space:]')" ] && echo yes || echo no); see $ERR_FILE]` marker to the log, **increment `REVIEW_ERRORS`**, and continue to the next file (one file's failure should not abort the whole review). Coverage accounting after the loop — first recompute `REVIEWABLE=$((TOTAL_FILES - SKIPPED_EMPTY))` (the files actually sent to the model; empty-diff skips never reached it), then define a coverage gap as any reviewable file that errored or was truncated (`REVIEW_ERRORS + TRUNCATED > 0`): - - If *every reviewable* file errored (`REVIEWABLE > 0` and `REVIEW_ERRORS == REVIEWABLE`), set `STATUS=cli-error`, print the last 80 lines of `$ERR_FILE` (genuine ollama errors live there, not in `$LOG_FILE`; but in the exit-0 empty-response failure mode `$ERR_FILE` may hold only spinner noise, so also surface the per-file `[ollama error reviewing …]` markers from `$LOG_FILE`), and exit — nothing was reviewed. **Before reporting it, rule out the shell-expansion false positive**: every file erroring with an empty response and nothing but `unbound variable` in `$ERR_FILE` is the bash-3.2 empty-array symptom (pre-flight step 6), not a reviewer failure — an absent `timeout`/`gtimeout` is a supported environment condition, so print `ollama: no timeout/gtimeout on this machine — running unbounded` once and re-run the pass with the `${ARR[@]+"${ARR[@]}"}` expansion rather than recording `cli-error`. (Use `REVIEWABLE`, not `TOTAL_FILES`: an empty-diff skip would otherwise make `REVIEW_ERRORS == TOTAL_FILES` unreachable and misclassify a total failure as merely `incomplete`.) + - If *every reviewable* file errored (`REVIEWABLE > 0` and `REVIEW_ERRORS == REVIEWABLE`), set `STATUS=cli-error`, print the last 80 lines of `$ERR_FILE` (genuine ollama errors live there, not in `$LOG_FILE`; but in the exit-0 empty-response failure mode `$ERR_FILE` may hold only spinner noise, so also surface the per-file `[ollama error reviewing …]` markers from `$LOG_FILE`), and exit — nothing was reviewed. **Before reporting it, rule out the shell-expansion false positive**: every file erroring with an empty response and nothing but `unbound variable` in `$ERR_FILE` is the bash-3.2 empty-array symptom (`~/.claude/lib/empty-array-expansion.md`), not a reviewer failure — so print `ollama: no timeout/gtimeout on this machine — running unbounded` once and re-run the pass with the guarded expansion rather than recording `cli-error`. (Use `REVIEWABLE`, not `TOTAL_FILES`: an empty-diff skip would otherwise make `REVIEW_ERRORS == TOTAL_FILES` unreachable and misclassify a total failure as merely `incomplete`.) - JSON parse errors are accounted for after the per-file invocations (see the defensive parsing rule below). Like invocation errors and truncation, they are coverage gaps: a non-empty response that cannot be parsed is not evidence that the file was clean. - If there is any coverage gap but not a total failure (`REVIEW_ERRORS + PARSE_ERRORS + TRUNCATED > 0` and `REVIEW_ERRORS + PARSE_ERRORS < REVIEWABLE`), the diff was only **partially** reviewed. Still process the findings from the parts that were reviewed (apply their fixes in step 3), but the pass **must not report `clean`** — at the point step 3 would set `STATUS=clean`, set `STATUS=incomplete` instead (see step 3). `incomplete` is treated as inconclusive by the multi-reviewer aggregate (not eligible to merge), because part of the change was never reviewed. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 110aa0a..06d5754 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -8,6 +8,15 @@ const path = require('path'); const readLib = (name) => fs.readFileSync(path.join(__dirname, '..', 'lib', name), 'utf8'); const readCommand = (name) => fs.readFileSync(path.join(__dirname, '..', 'commands', 'do', name), 'utf8'); +// The loop partials whose invocations carry arrays that can legitimately be empty +// (TIMEOUT_CMD when no timeout/gtimeout is installed, MODEL_FLAG when no model is +// pinned, OLLAMA_FLAGS on an ollama too old for the optional flags). +const LOOPS_WITH_OPTIONAL_ARRAYS = [ + 'local-agent-review-loop.md', + 'ollama-review-loop.md', + 'enhance-loop.md', +]; + describe('review-loop parse contracts', () => { it('requires structured local-agent verdicts without weakening Codex handling', () => { const body = readLib('local-agent-review-loop.md'); @@ -250,28 +259,45 @@ describe('review-loop parse contracts', () => { // REVIEW_ERRORS, and the pass resolves to `cli-error` — a hard error `~opt` does not // excuse — blocking the merge on a PR no reviewer looked at. Only the // ${ARR[@]+"${ARR[@]}"} form is safe on bash 3.2, bash 4/5, and zsh alike. - const arrays = ['TIMEOUT_CMD', 'MODEL_FLAG', 'OLLAMA_FLAGS']; - for (const name of ['local-agent-review-loop.md', 'ollama-review-loop.md', 'enhance-loop.md']) { + // Scan by PATTERN, not by a hardcoded array-name list, so a newly introduced + // optional-argument array is covered the day it lands. + for (const name of LOOPS_WITH_OPTIONAL_ARRAYS) { const body = readLib(name); - for (const arr of arrays) { - assert.doesNotMatch( - body, - new RegExp(String.raw`(? { - // An absent timeout binary is an environment condition, not a reviewer failure — - // the loops must say so, or the next reader re-reports it as a genuine cli-error. - const ollama = readLib('ollama-review-loop.md'); - assert.match(ollama, /Stock macOS ships \*\*neither\*\* `timeout\(1\)`.*nor `gtimeout`/); - assert.match(ollama, /supported configuration\*\*: the review runs unbounded and must never be recorded as a reviewer failure/); + it('keeps the empty-array rule in one partial the loops point at', () => { + // An absent timeout binary is an environment condition, not a reviewer failure. + // The explanation lives in ONE partial (the lib/gh-host.md convention) — five + // near-identical copies is how the rule drifted mid-PR the first time. Assert the + // partial carries the contract and that each loop links to it; do NOT assert on + // the loops' own prose wording, which is theirs to copy-edit. + const partial = readLib('empty-array-expansion.md'); + assert.match(partial, /\$\{ARR\[@\]\+"\$\{ARR\[@\]\}"\}\s+# correct/); + assert.match(partial, /Stock macOS ships \*\*neither\*\* `timeout\(1\)`/); + assert.match(partial, /supported configuration\*\*/); + assert.match(partial, /unbound variable/); - const local = readLib('local-agent-review-loop.md'); - assert.match(local, /Stock macOS ships NEITHER `timeout\(1\)`/); - assert.match(local, /a supported configuration and never a reviewer failure/); + for (const name of LOOPS_WITH_OPTIONAL_ARRAYS) { + assert.match( + readLib(name), + /lib\/empty-array-expansion\.md/, + `${name} must point at the shared empty-array-expansion partial rather than restating it`, + ); + } }); }); diff --git a/uninstall.sh b/uninstall.sh index 88f9857..94ac6dd 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -33,7 +33,7 @@ OLD_COMMANDS=(cam good makegoals makegood optimize-md) # NOTE: keep in sync with install.sh LIBS — see comment there. LIBS=( ci-flake-handling code-review-checklist copilot-review-loop - enhance-loop epic-children + empty-array-expansion enhance-loop epic-children finding-disposition fix-regression-guard gh-host github-reviewer-loop graphql-escaping local-agent-review-loop multi-reviewer-loop ollama-review-loop From 8dbd1da3a1deea3462a20514d46b138dba7b4f26 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Wed, 12 Aug 2026 23:20:47 +0000 Subject: [PATCH 3/3] address review (antigravity): catch the quoted form of a double-wrapped guard too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ${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. --- test/review-loop-contract.test.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 06d5754..8b479c5 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -275,7 +275,9 @@ describe('review-loop parse contracts', () => { // ...and the opposite slip: a mechanical rewrite that wraps an already-guarded // expansion a second time. Harmless to bash, but it publishes a second "correct" // spelling of the rule this partial exists to teach, which is how it drifts. - const doubled = body.match(/\$\{([A-Z][A-Z0-9_]*)\[@\]\+\$\{\1\[@\]\+/g); + // `"?` because the second wrap may or may not quote the inner expansion — + // ${A[@]+${A[@]+…}} and ${A[@]+"${A[@]+…}"} are both the same slip. + const doubled = body.match(/\$\{([A-Z][A-Z0-9_]*)\[@\]\+"?\$\{\1\[@\]\+/g); assert.equal(doubled, null, `${name}: double-wrapped guard ${doubled && doubled.join(', ')} — one \${ARR[@]+…} is enough`); } });