Skip to content

fix(doctor): build the gemini probe from the dispatch argv builder - #87

Open
kevin-agrology wants to merge 4 commits into
mainfrom
fix/gemini-probe-dispatch-argv
Open

fix(doctor): build the gemini probe from the dispatch argv builder#87
kevin-agrology wants to merge 4 commits into
mainfrom
fix/gemini-probe-dispatch-argv

Conversation

@kevin-agrology

Copy link
Copy Markdown
Contributor

TL;DR: multi-review-reviewer.sh doctor was testing gemini with a different command than the one it actually dispatches, so it could report "ready" for a setup that fails on every real review — and report a failure for a setup that would have worked fine. Both now come from a single builder, so they cannot drift apart again.

The problem

doctor's live probe and the real dispatch each built their own argv:

argv
probe gemini -m <model> -p "reply with OK"
dispatch [env GEMINI_CLI_TRUST_WORKSPACE=true] gemini -m <model> --approval-mode auto_edit -p <prompt>

Two independent divergences, one in each direction:

  1. The probe omitted --approval-mode auto_edit — a CLI that rejects that flag probes green while every dispatch is rejected.
  2. The probe omitted the autotrust env prefix — a workspace that only becomes trusted under MULTI_REVIEW_GEMINI_AUTOTRUST=1 probes red while dispatch would have worked.

Same "green signal, broken dispatch" class as #73 and #80.

The fix

A single gemini_argv <model> <prompt> builder. cmd_command emits its NUL stream to the dispatcher; gemini_live_probe reads the same stream into an array and runs it. The prompt is the only deliberate difference left, so a flag added later reaches both with no second edit.

No empty-array guard, deliberately (§1.2): gemini_argv structurally always emits ≥7 elements, and that guarantee — not a runtime check — is what keeps the bash-3.2 set -u expansion hazard unreachable.

Tests

Five assertions in multi-review-reviewer.test.sh, all run against a stubbed gemini on PATH:

  • G4/T1 — the probe carries --approval-mode auto_edit.
  • G4/T1b — with autotrust off, the probe carries no trust prefix.
  • G4/T2 — with autotrust on, it does.
  • G4/T3a — the dispatch argv begins with gemini.
  • G4/T3 — probe argv equals dispatch argv, modulo the prompt.

T1b exists because review caught a hole in T3: env execs the binary in place, so T3's offset-of-one comparison is blind to any leading wrapper. An unconditional trust prefix on the probe would have passed T1, T2 and T3 alike — the exact drift direction (2) above. T1b was verified to go red before being accepted.

Security-relevant (§3)

doctor's probe now runs at --approval-mode auto_edit, and under MULTI_REVIEW_GEMINI_AUTOTRUST=1 with the workspace trusted. A diagnostic gains edit approval it previously did not have. Bounded by: a fixed benign prompt (reply with OK) with no document attached, auto_edit approving edit tools only and never shell, a 30s kill escalation, and discarded output. In an untrusted workspace the flag is inert, since edits are disabled there regardless. Making the diagnostic match the thing it diagnoses is the point of the change, but it is a real widening and wants focused review. Documented in the README's gemini prereqs.

Verification

  • All 15 suites green under bash 5 and /bin/bash 3.2.57.
  • shellcheck --severity=warning clean on scripts/multi-review-*.sh and .githooks/pre-push.
  • multi-review-version-check.sh: 1.24.0 → 1.24.1.
  • Full mutation sweep: all 98 caught, including the new reviewer/gemini-probe-uses-dispatch-argv; zero SURVIVED/MISCREDITED/STALE.

Verification limit: gemini is not installed on the development machine, so every assertion runs against a stub. The argv agreement is proven; that the real CLI accepts that argv is not, and this PR does not claim it.

Process

Spec and plan each went through this repo's own multi-review (codex + fable) before implementation; the branch was then executed task-by-task with a review after each and a whole-branch review at the end.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant