From b16ce776cdb2fd6c3a95b43019f047f8c0517c8d Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Wed, 12 Aug 2026 15:10:02 -0500 Subject: [PATCH 1/4] fix(doctor): build the gemini probe from the dispatch argv builder --- scripts/multi-review-reviewer.sh | 56 +++++++++------ scripts/multi-review-reviewer.test.sh | 99 +++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 20 deletions(-) diff --git a/scripts/multi-review-reviewer.sh b/scripts/multi-review-reviewer.sh index 2153143..457cfa5 100755 --- a/scripts/multi-review-reviewer.sh +++ b/scripts/multi-review-reviewer.sh @@ -550,6 +550,33 @@ cmd_prompt() { # --reviewer emit_prompt "$(abs_path "$doc")" "$has_skill" "$kind" } +# The gemini dispatch argv, built in ONE place. `cmd_command` emits it for the dispatcher; the +# doctor's live probe RUNS it with a trivial prompt. Two independent builders let doctor and +# dispatch disagree in BOTH directions — a probe missing `--approval-mode` passes on a CLI that +# rejects the dispatch, and one missing the autotrust prefix fails on a workspace the dispatch +# trusts. The prompt is the ONLY deliberate difference between the two callers, so a flag added +# here reaches both without a second edit. +# +# `model` always carries a value (registry default or the env override), so the model is +# always explicit — we never fall through to the CLI's own default tier. +# +# `--approval-mode auto_edit` is the analogue of the codex route's `--write`. Without it +# `gemini -p` runs at approval mode `default` ("prompt for approval") with nobody there +# to prompt, so file-modification tools are disabled: observed live, the reviewer emitted +# its findings as prose and never touched the doc, leaving the marker unflipped. Chosen +# over `yolo` deliberately — `auto_edit` approves edit tools only, never shell. +# Opt-in auto-trust (MULTI_REVIEW_GEMINI_AUTOTRUST=1) scopes GEMINI_CLI_TRUST_WORKSPACE=true to +# THIS dispatch via an `env` prefix, so users needn't set it in their profile. Default +# (unset/≠1) is byte-identical to before. SECURITY: trusting a workspace lets the CLI honor its +# .env / settings and auto-edit — opt-in only, never a cloned/untrusted repo (see README). +gemini_argv() { # -> NUL-delimited argv + if [[ "${MULTI_REVIEW_GEMINI_AUTOTRUST:-}" == "1" ]]; then + printf '%s\0' "env" "GEMINI_CLI_TRUST_WORKSPACE=true" "gemini" "-m" "$1" "--approval-mode" "auto_edit" "-p" "$2" + else + printf '%s\0' "gemini" "-m" "$1" "--approval-mode" "auto_edit" "-p" "$2" + fi +} + cmd_command() { # --reviewer -> NUL-delimited argv local doc="${1:-}" [[ -n "$doc" ]] || die "usage: multi-review-reviewer.sh command --reviewer " 2 @@ -570,24 +597,7 @@ cmd_command() { # --reviewer -> NUL-delimited argv # never re-parse this through a shell. Consumer idiom (bash 3.2 safe, no mapfile): # argv=(); while IFS= read -r -d '' a; do argv+=("$a"); done < <(… command "$doc") case "$id" in - gemini) - # `model` always carries a value (registry default or the env override), so the model is - # always explicit — we never fall through to the CLI's own default tier. - # - # `--approval-mode auto_edit` is the analogue of the codex route's `--write`. Without it - # `gemini -p` runs at approval mode `default` ("prompt for approval") with nobody there - # to prompt, so file-modification tools are disabled: observed live, the reviewer emitted - # its findings as prose and never touched the doc, leaving the marker unflipped. Chosen - # over `yolo` deliberately — `auto_edit` approves edit tools only, never shell. - # Opt-in auto-trust (MULTI_REVIEW_GEMINI_AUTOTRUST=1) scopes GEMINI_CLI_TRUST_WORKSPACE=true to - # THIS dispatch via an `env` prefix, so users needn't set it in their profile. Default - # (unset/≠1) is byte-identical to before. SECURITY: trusting a workspace lets the CLI honor its - # .env / settings and auto-edit — opt-in only, never a cloned/untrusted repo (see README). - if [[ "${MULTI_REVIEW_GEMINI_AUTOTRUST:-}" == "1" ]]; then - printf '%s\0' "env" "GEMINI_CLI_TRUST_WORKSPACE=true" "gemini" "-m" "$model" "--approval-mode" "auto_edit" "-p" "$prompt" - else - printf '%s\0' "gemini" "-m" "$model" "--approval-mode" "auto_edit" "-p" "$prompt" - fi ;; + gemini) gemini_argv "$model" "$prompt" ;; *) die "no shell command defined for reviewer provider '${id}'" 2 ;; esac @@ -794,11 +804,17 @@ cmd_doctor() { # can't hang) and reap direct children. GEMINI_PROBE_MSG="" gemini_live_probe() { - local model out pid waited bound + local model out pid waited bound a model="$(provider_row gemini | cut -d'|' -f4)" bound="${MULTI_REVIEW_PROBE_TIMEOUT:-30}" out="$(mktemp)" - ( gemini -m "$model" -p "reply with OK" >"$out" 2>&1 ) & pid=$! + # The probe RUNS the dispatch argv (G4), so doctor certifies the command dispatch actually + # uses rather than a second one that can drift from it. The read loop is deliberately ONE + # line: the mutation entry replaces it, and `replace` operates on a single line that has to + # parse on its own. + local argv=() + while IFS= read -r -d '' a; do argv+=("$a"); done < <(gemini_argv "$model" "reply with OK") + ( "${argv[@]}" >"$out" 2>&1 ) & pid=$! waited=0 while kill -0 "$pid" 2>/dev/null && (( waited < bound )); do sleep 1; waited=$((waited+1)); done if kill -0 "$pid" 2>/dev/null; then diff --git a/scripts/multi-review-reviewer.test.sh b/scripts/multi-review-reviewer.test.sh index e3520ed..73d7bdd 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -1466,6 +1466,105 @@ HOME="${CD_}/home" PATH="${CD_}/bin:$PATH" bash "$SUT" check --reviewer codex -- [[ $? -eq 2 ]] && ok "check: --doc with no value exits 2" \ || bad "--doc with no value did not exit 2" +# --- G4: doctor's live probe must run the DISPATCH argv, not a second one --------------------- +# The probe and cmd_command built their argv independently, so they could disagree in BOTH +# directions: a probe without `--approval-mode auto_edit` passes on a CLI that rejects the +# dispatch, and one without the autotrust `env` prefix fails on a workspace the dispatch trusts. +# Same "green signal, broken dispatch" class as #73/#80. +GA="${WORK}/gargv"; mkdir -p "$GA/bin" "$GA/repo/home" +( cd "$GA/repo" && git init -q ) +# The stub records the argv it was invoked with (NUL-delimited) plus whether the trust variable +# reached its environment. `$@` never contains `$0`, so the recorded stream begins at `-m`. +cat > "$GA/bin/gemini" <<'STUB' +#!/usr/bin/env bash +: > "$GA_ARGV" +for a in "$@"; do printf '%s\0' "$a" >> "$GA_ARGV"; done +printf '%s' "${GEMINI_CLI_TRUST_WORKSPACE-}" > "$GA_TRUST" +echo OK +STUB +chmod +x "$GA/bin/gemini" +# Pre-create both sinks: if the probe never launches, the assertions must fail on their own +# message rather than on a redirect against a missing file. +: > "$GA/argv.bin"; : > "$GA/trust.txt" + +# bash 3.2 has no mapfile. Populates the global array `nulargv`. +read_nul() { nulargv=(); local a; while IFS= read -r -d '' a; do nulargv+=("$a"); done < "$1"; } + +# Run doctor in the fixture with the stub on PATH. Extra assignments come from the caller and +# MUST go through `env`: a bare `VAR=x "$@" bash …` would not treat an assignment arriving via +# "$@" as an assignment at all — the parser fixes the assignment prefixes before "$@" expands, +# so bash would look for a command literally named `MULTI_REVIEW_GEMINI_AUTOTRUST=1`. +ga_doctor() { + ( cd "$GA/repo" && env HOME="$GA/repo/home" PATH="${GA}/bin:$PATH" \ + GA_ARGV="$GA/argv.bin" GA_TRUST="$GA/trust.txt" MULTI_REVIEW_PROBE_TIMEOUT=5 \ + "$@" bash "$SUT" doctor >/dev/null 2>&1 ) +} + +# T1 — the probe carries the dispatch's `--approval-mode auto_edit`. +unset GEMINI_CLI_TRUST_WORKSPACE MULTI_REVIEW_GEMINI_AUTOTRUST +ga_doctor +read_nul "$GA/argv.bin" +probe_argv=(); (( ${#nulargv[@]} )) && probe_argv=("${nulargv[@]}") +# Arithmetic for-loop, NOT `for i in $(seq 0 $(( len - 1 )))`. BSD seq counts DOWN when the end +# is below the start, so an empty array yields `seq 0 -1` -> "0 -1" and the body runs against an +# unbound index: under `set -u` on /bin/bash 3.2 that aborts the whole test FILE with exit 127, +# masking every assertion after it. GNU seq prints nothing there, so the Ubuntu leg would pass +# straight through and only the macOS leg would break. The `i + 1 < len` bound also keeps both +# indices in range, so neither needs a `:-` default. +t1_appr=0 +for (( i = 0; i + 1 < ${#probe_argv[@]}; i++ )); do + [[ "${probe_argv[$i]}" == "--approval-mode" && "${probe_argv[$((i+1))]}" == "auto_edit" ]] && t1_appr=1 +done +(( t1_appr )) \ + && ok "G4/T1: doctor's probe carries --approval-mode auto_edit, like dispatch" \ + || bad "G4/T1: probe argv lacks --approval-mode auto_edit: ${probe_argv[*]:-}" + +# T2 — under autotrust the probe gets the `env GEMINI_CLI_TRUST_WORKSPACE=true` prefix. +# The unset is PART OF THE TEST, not an assumption about the shell: this suite clears only +# MULTI_REVIEW_REVIEWER_MODEL globally, and an ambient export would make this pass against a probe +# that carries no prefix at all — green for a reason unrelated to the fix. +unset GEMINI_CLI_TRUST_WORKSPACE +ga_doctor MULTI_REVIEW_GEMINI_AUTOTRUST=1 +[[ "$(cat "$GA/trust.txt")" == "true" ]] \ + && ok "G4/T2: autotrust reaches the probe's environment, like dispatch" \ + || bad "G4/T2: probe saw GEMINI_CLI_TRUST_WORKSPACE='$(cat "$GA/trust.txt")' (want true)" + +# T3 (anti-drift) — the probe argv IS the dispatch argv, modulo the prompt. Autotrust OFF, so +# neither stream carries the `env` prefix; `env` execs the binary in any case, so the prefix could +# never reach the stub's `$@`. T2 covers that direction. +unset GEMINI_CLI_TRUST_WORKSPACE MULTI_REVIEW_GEMINI_AUTOTRUST +ga_doctor +read_nul "$GA/argv.bin" +probe_argv=(); (( ${#nulargv[@]} )) && probe_argv=("${nulargv[@]}") +# `command` needs no marker in the doc (cmd_command checks only that the file exists, then builds +# the prompt from the path) and the path is absolute, so this is cwd-independent — a failure here +# is a real one, not fixture drift. +printf '# t\n' > "$GA/repo/d.md" +bash "$SUT" command "$GA/repo/d.md" --reviewer gemini > "$GA/cmd.bin" 2>/dev/null +read_nul "$GA/cmd.bin" +cmd_argv=(); (( ${#nulargv[@]} )) && cmd_argv=("${nulargv[@]}") + +# The stub records `$@`, which never contains `$0`, while the builder stream starts at `gemini`. +# Drop that element before comparing, and assert separately that it IS `gemini`. +[[ "${cmd_argv[0]:-}" == "gemini" ]] \ + && ok "G4/T3a: dispatch argv element 0 is the gemini binary" \ + || bad "G4/T3a: dispatch argv element 0 is '${cmd_argv[0]:-}' (want gemini)" + +drift="" +if (( ${#probe_argv[@]} != ${#cmd_argv[@]} - 1 )); then + drift="length: probe ${#probe_argv[@]} vs dispatch $(( ${#cmd_argv[@]} - 1 ))" +else + # Arithmetic for-loop for the same reason as T1's, and bounded at len-1 so the trailing -p + # payload — the one deliberate difference — is simply never compared. + for (( i = 0; i < ${#probe_argv[@]} - 1; i++ )); do + [[ "${probe_argv[$i]}" == "${cmd_argv[$((i+1))]}" ]] \ + || drift="element $i: probe '${probe_argv[$i]}' vs dispatch '${cmd_argv[$((i+1))]}'" + done +fi +[[ -z "$drift" ]] \ + && ok "G4/T3: doctor's probe argv is the dispatch argv, modulo the prompt" \ + || bad "G4/T3: probe and dispatch argv have drifted — $drift" + echo if (( fails > 0 )); then echo "FAILED: $fails"; exit 1; fi echo "all passed" From a1c2bc7992c44c862ed77b2b2db4c3b97d089a99 Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Wed, 12 Aug 2026 15:18:00 -0500 Subject: [PATCH 2/4] test(mutation): cover the probe/dispatch argv guard --- scripts/multi-review-mutation-check.sh | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/scripts/multi-review-mutation-check.sh b/scripts/multi-review-mutation-check.sh index 83cbdbd..09a1e27 100755 --- a/scripts/multi-review-mutation-check.sh +++ b/scripts/multi-review-mutation-check.sh @@ -1141,6 +1141,27 @@ mutations() { 'a multi-line-block-commented respectGitIgnore read as a live opt-out' 'multi-review-reviewer.test.sh' \ ' if (inblock) {' \ ' if (0) {' + + # ---- G4: doctor's probe must RUN the dispatch argv ------------------------------------------- + + # Reverting the probe to its own literal restores the two-builder split, where doctor and + # dispatch could disagree in both directions. T3 is the assertion that encodes this guard + # directly (argv equality); T1 also fails, on the dropped `--approval-mode`, but that is one + # consequence rather than the guard itself. Expecting T3 is safe because this suite is NOT + # fail-fast — `bad()` increments a counter and returns, so every assertion runs and T3's FAIL + # line is emitted even though T1 fails first. + # + # The replacement restores the old BEHAVIOUR on one parseable line, not merely the old text. + # Substituting the literal into a multi-line `while … \` + `< <(…)` construct would leave either + # unparseable bash (which the runner rejects as "failing for the wrong reason") or an empty + # `argv` — the bash-3.2 `set -u` hazard the probe relies on being unreachable. Either way the + # entry would "catch" something while proving nothing about T3. That is why the probe writes + # the read loop on a single line. + mutate 'reviewer/gemini-probe-uses-dispatch-argv' 'scripts/multi-review-reviewer.sh' replace \ + 'probe and dispatch argv have drifted' 'multi-review-reviewer.test.sh' \ + ' while IFS= read -r -d '"'"''"'"' a; do argv+=("$a"); done < <(gemini_argv "$model" "reply with OK")' \ + ' argv=(gemini -m "$model" -p "reply with OK")' + # ---- convergence integrity ------------------------------------------------------------------ # THE SELF-RESPONSE GUARD. This is what makes the review a review rather than a self-review: it From d1cb197a3d4847ef271aa5d06fd05a46a556ee1e Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Wed, 12 Aug 2026 15:22:28 -0500 Subject: [PATCH 3/4] docs(readme): autotrust now covers doctor's probe; bump 1.24.1 --- .claude-plugin/plugin.json | 2 +- README.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 47b27ea..593758d 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "multi-review", "description": "Human-gated star review: Claude (the neutral primary) fans a spec/plan/PR out to N independent secondary reviewers (in-harness fable by default — disable with MULTI_REVIEW_FABLE=off — plus cross-vendor codex/gemini), adjudicates their findings, and converges — always ending at a human approval gate.", - "version": "1.24.0", + "version": "1.24.1", "author": { "name": "Agrology" }, diff --git a/README.md b/README.md index f48dd08..4e81e77 100644 --- a/README.md +++ b/README.md @@ -156,7 +156,7 @@ Run **`/multi-review --check-reviewers`** to verify every reviewer's setup at a | `MULTI_REVIEW_MAX_ROUNDS` | `5` | round **ceiling** (each round costs N dispatches; convergence is adaptive) | | `MULTI_REVIEW_REVIEWER_MODEL` | *(provider default)* | pin a provider's model (`codex`→`gpt-5.6-terra`, `fable`→`fable`, `gemini`→`gemini-pro-latest`) | | `MULTI_REVIEW_DOC_DIRS` | `docs/specs docs/plans docs/superpowers/specs docs/superpowers/plans` | where bare-name local docs are resolved. Covers the plain and `superpowers` layouts out of the box. Bare-name resolution **warns** when a newer dated doc sits in a directory it did not search — the egress guard cannot catch that, since the doc it picked is legitimately inside the configured dirs. | -| `MULTI_REVIEW_GEMINI_AUTOTRUST` | *(off)* | `=1` scopes `GEMINI_CLI_TRUST_WORKSPACE=true` to the gemini dispatch (no profile edit needed). **Security:** trusting a workspace lets gemini honor its `.env`/settings and auto-edit — enable only for repos you trust, never a freshly-cloned one. | +| `MULTI_REVIEW_GEMINI_AUTOTRUST` | *(off)* | `=1` scopes `GEMINI_CLI_TRUST_WORKSPACE=true` to the gemini dispatch **and to `doctor`'s live probe, which runs the same argv** (no profile edit needed). **Security:** trusting a workspace lets gemini honor its `.env`/settings and auto-edit — enable only for repos you trust, never a freshly-cloned one. | The last explicitly-named reviewer combo is remembered per repo in **`.multi-review/reviewers.pref`** (gitignored). It is written when reviewers are named (flag or prose) — **except** when From 2ebce7db3566fff8bb8aa3cc8169165719b8b6a2 Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Wed, 12 Aug 2026 16:58:27 -0500 Subject: [PATCH 4/4] fix(test): close the G4 probe/dispatch argv drift review gaps T3's fixed offset-of-one absorbs any leading exec-wrapper on the probe side, not just $0, so a future unconditional env prefix would pass T1, T2, and T3 alike. Add G4/T1b to assert the probe carries no trust prefix when autotrust is off, and reword T3's comment to say what it actually proves. Reset argv.bin before T3's own doctor run so a stale file from T2 can't pass on data T3 never produced. Document that doctor's probe now runs --approval-mode auto_edit, same as dispatch. --- README.md | 2 ++ scripts/multi-review-reviewer.test.sh | 11 +++++++++-- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 4e81e77..e645f4b 100644 --- a/README.md +++ b/README.md @@ -133,6 +133,8 @@ refuses to run until you remove it, and `--check-reviewers`/`doctor` will flag t 1. **`export GEMINI_CLI_TRUST_WORKSPACE=true`** (or trust the folder once). An untrusted workspace makes the CLI skip `.env` (so it can't authenticate — the error misleadingly says "set an Auth method") *and* disables file edits, so the reviewer can't write the doc. + `doctor`'s probe runs the same command line as a real dispatch, including + `--approval-mode auto_edit`, so its verdict reflects what dispatch will actually do. 2. An API key — `export GEMINI_API_KEY=…`, or drop `GEMINI_API_KEY=…` in `~/.gemini/.env` (or your repo's `.env`). **It auto-loads once the workspace is trusted.** 3. `.gemini/settings.json` → `{"context":{"fileFiltering":{"respectGitIgnore":false}}}`. Needed diff --git a/scripts/multi-review-reviewer.test.sh b/scripts/multi-review-reviewer.test.sh index 73d7bdd..ac79b16 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -1519,6 +1519,11 @@ done && ok "G4/T1: doctor's probe carries --approval-mode auto_edit, like dispatch" \ || bad "G4/T1: probe argv lacks --approval-mode auto_edit: ${probe_argv[*]:-}" +# T1b — with autotrust OFF (this run's setup), the probe carries no trust prefix at all. +[[ "$(cat "$GA/trust.txt")" == "" ]] \ + && ok "G4/T1b: without autotrust the probe carries no trust prefix" \ + || bad "G4/T1b: probe saw GEMINI_CLI_TRUST_WORKSPACE='$(cat "$GA/trust.txt")' (want unset)" + # T2 — under autotrust the probe gets the `env GEMINI_CLI_TRUST_WORKSPACE=true` prefix. # The unset is PART OF THE TEST, not an assumption about the shell: this suite clears only # MULTI_REVIEW_REVIEWER_MODEL globally, and an ambient export would make this pass against a probe @@ -1530,9 +1535,11 @@ ga_doctor MULTI_REVIEW_GEMINI_AUTOTRUST=1 || bad "G4/T2: probe saw GEMINI_CLI_TRUST_WORKSPACE='$(cat "$GA/trust.txt")' (want true)" # T3 (anti-drift) — the probe argv IS the dispatch argv, modulo the prompt. Autotrust OFF, so -# neither stream carries the `env` prefix; `env` execs the binary in any case, so the prefix could -# never reach the stub's `$@`. T2 covers that direction. +# neither stream carries the `env` prefix; but `env` execs the binary in place, so ANY leading +# wrapper — not just this one — is invisible to `$@` and this comparison's fixed offset of one. +# T1b (absence when autotrust is off) and T2 (presence when it's on) actually pin the prefix. unset GEMINI_CLI_TRUST_WORKSPACE MULTI_REVIEW_GEMINI_AUTOTRUST +: > "$GA/argv.bin" ga_doctor read_nul "$GA/argv.bin" probe_argv=(); (( ${#nulargv[@]} )) && probe_argv=("${nulargv[@]}")