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..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 @@ -156,7 +158,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 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 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..ac79b16 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -1466,6 +1466,112 @@ 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[*]:-}" + +# 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 +# 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; 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[@]}") +# `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"