From e9ff9cd33155a1b585dd9970d651349906f113a9 Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Tue, 11 Aug 2026 18:29:14 -0500 Subject: [PATCH 1/3] fix(gemini): judge and dispatch against the same root (#66's other half) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The command file documented this gap rather than closing it: "gemini remains exposed to the same check-cwd-vs-dispatch-cwd drift this fixed for codex". gemini-cli's workspace is the cwd of the process at LAUNCH, and it refuses to read or write outside it. Two things followed from never pinning that: THE CHECK JUDGED BY ITS OWN CWD. The basis was `repo_root()` — the git toplevel of the CHECK invocation — on the stated reasoning that gemini "runs as a shell command in the CURRENT cwd" so that was the right basis. The premise holds only if check and dispatch stand in the same place, and nothing enforced it. The arm was therefore wrong in BOTH directions, and the suite now asserts both: it MISSED a copy outside the root dispatch would inherit (the #66 failure), and it FALSELY hinted on a copy that was inside that root but outside whatever repo the check happened to be standing in. The second was the surprise — a spurious hint teaches the reader to ignore the real one. THE DISPATCH PINNED NOTHING. The shell branch set no directory at all, so the workspace was whatever cwd that Bash call inherited. Now `cd ""` inside a subshell, scoped to the dispatch, leaving the primary's own cwd alone. Both halves are needed: pinning only the check would validate a root the dispatch does not use, which is exactly the shape of #66. `fable` needs no basis — it runs in-harness with no workspace of its own — so `--session-root` is now consumed by both arms that have one, and the scope note saying otherwise is removed. A stale claim like that is how the gap survived: written down instead of fixed. Guards: three entries — the check's basis, the dispatch's cwd, and the scope claim itself, so the documentation cannot quietly revert to describing the bug. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE --- .claude-plugin/plugin.json | 2 +- commands/multi-review.md | 23 +++++++++++----- scripts/multi-review-mutation-check.sh | 23 ++++++++++++++++ scripts/multi-review-packaging.test.sh | 23 ++++++++++++++++ scripts/multi-review-reviewer.sh | 28 +++++++++++++------ scripts/multi-review-reviewer.test.sh | 37 ++++++++++++++++++++++++++ 6 files changed, 121 insertions(+), 15 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 244eae7..bc7c64f 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.22.0", + "version": "1.22.1", "author": { "name": "Agrology" }, diff --git a/commands/multi-review.md b/commands/multi-review.md index 1862935..1060693 100644 --- a/commands/multi-review.md +++ b/commands/multi-review.md @@ -237,11 +237,17 @@ re-resolve later (a mutable env var could otherwise swap providers mid-review un silently restore the wrong basis, and a failed capture is the one place an empty value comes from. That is why the capture step above tells you to stop instead of passing it on. - **Scope: only the codex arm consumes `--session-root` today.** The flag is accepted on every - `check` invocation so the command line stays uniform, but the gemini arm still judges against - its own cwd repo. So a silent `check` for gemini means "nothing else looked wrong", not "the - reviewer can reach this copy" — gemini remains exposed to the same check-cwd-vs-dispatch-cwd - drift this fixed for codex (fable-rd2-r2, tracked under #66). + **Both external arms consume `--session-root`.** codex is bound to one root per session; + gemini's workspace is the cwd of the process at LAUNCH, and the shell branch below pins that + to the same ``. So for either provider a silent `check` now means "the reviewer + can reach this copy", not merely "you happen to be standing next to it". + + That was not always true of gemini: the check judged against its own cwd repo, on the + reasoning that gemini "runs as a shell command in the CURRENT cwd". The premise holds only if + check and dispatch stand in the same place, and nothing enforced that — so the arm was wrong + in BOTH directions, missing a copy outside the dispatch root and falsely hinting on one + inside it (fable-rd2-r2, under #66). `fable` needs no basis: it runs in-harness with no + workspace of its own. - Tell the engineer: "multi-review armed on `` — secondaries: `` (round bound ``)" — and append any dropped-reviewer relay, e.g. "`gemini` dropped: unavailable in this repo". @@ -439,7 +445,12 @@ re-resolve later (a mutable env var could otherwise swap providers mid-review un # argv means the command could not be built — treat it as a dispatch failure for # this provider (quarantine it) rather than expanding. (( ${#argv[@]} )) || { : quarantine "could not build reviewer command"; } - (( ${#argv[@]} )) && "${argv[@]}" + # PIN THE LAUNCH CWD. gemini-cli's workspace is the cwd of the process at LAUNCH, and + # it refuses to read or write outside it — so without this `cd` the workspace is + # whatever directory this Bash call happens to inherit, which is not necessarily the + # root holding `.`. Same drift `--session-root` fixes for codex (#66); the + # subshell keeps it scoped to the dispatch and leaves your own cwd alone. + (( ${#argv[@]} )) && ( cd "" && "${argv[@]}" ) All same-turn subagent dispatches go in the SAME response block as each other. 5. **Bound the wait, per copy — and never quarantine on the first bound hit.** diff --git a/scripts/multi-review-mutation-check.sh b/scripts/multi-review-mutation-check.sh index 28f9452..473edda 100755 --- a/scripts/multi-review-mutation-check.sh +++ b/scripts/multi-review-mutation-check.sh @@ -323,6 +323,29 @@ mutations() { ' local id; id="$(printf '"'"'%s'"'"' "$1" | LC_ALL=C tr '"'"'[:upper:]'"'"' '"'"'[:lower:]'"'"')"' \ ' local id; id="$1"' + # ---- G2: gemini's half of #66 ---------------------------------------------------------------- + + # The check's BASIS. Reverted to the helper's cwd repo, the arm is wrong in both directions: + # it misses a copy outside the root dispatch will inherit, and falsely hints on one inside it. + mutate 'reviewer/gemini-session-root-basis' 'scripts/multi-review-reviewer.sh' replace \ + 'the #66 drift is still open for gemini' 'multi-review-reviewer.test.sh' \ + ' if [[ -n "$session_root" ]]; then gbase="$session_root"; else gbase="$rr"; fi' \ + ' gbase="$rr"' + + # The DISPATCH cwd. Without the pin, gemini-cli's workspace is whatever directory the Bash call + # inherits — so the check can be correct and the dispatch still land somewhere else. + mutate 'command/gemini-dispatch-cwd-pinned' 'commands/multi-review.md' replace \ + 'shell dispatch sets no cwd' 'multi-review-packaging.test.sh' \ + ' (( ${#argv[@]} )) && ( cd "" && "${argv[@]}" )' \ + ' (( ${#argv[@]} )) && "${argv[@]}"' + + # The doc must not re-acquire the stale claim that only codex consumes the flag — that sentence + # is how the gap stayed open: documented instead of closed. + mutate 'command/session-root-scope-claim' 'commands/multi-review.md' replace \ + 'still says only codex consumes --session-root' 'multi-review-packaging.test.sh' \ + ' **Both external arms consume `--session-root`.** codex is bound to one root per session;' \ + ' **Scope: only the codex arm consumes `--session-root` today.** The gemini arm judges by cwd;' + # ---- #66 wiring, the collision gate, and core.sh's first entries ------------------------------ # ensure-skill's root must be the CAPTURED session root. An inline substitution resolves in the diff --git a/scripts/multi-review-packaging.test.sh b/scripts/multi-review-packaging.test.sh index deeb5c2..77488a1 100755 --- a/scripts/multi-review-packaging.test.sh +++ b/scripts/multi-review-packaging.test.sh @@ -544,4 +544,27 @@ if [[ -f "$f" ]]; then fi fi +# --- the shell (gemini) dispatch must pin its launch cwd (G2, the other half of #66) --- +# +# gemini-cli's workspace is the cwd of the process at LAUNCH, and it refuses reads and writes +# outside it. The shell branch set no directory, so the workspace was whatever cwd that Bash call +# happened to have — which is the same check-cwd-vs-dispatch-cwd drift #66 fixed for codex, and +# which this file used to DOCUMENT for gemini instead of closing. +DR="${ROOT}/commands/multi-review.md" +if [[ -f "$DR" ]]; then + n="$(grep -n '\*\*`shell`\*\*' "$DR" | head -1 | cut -d: -f1)" + if [[ -z "$n" ]]; then + bad "no shell-kind dispatch branch found in $(basename "$DR")" + else + blk="$(sed -n "${n},$((n+22))p" "$DR")" + grep -qE 'cd ""' <<<"$blk" \ + && ok "shell dispatch pins its launch cwd to " \ + || bad "shell dispatch sets no cwd — gemini's workspace is then whatever directory the call inherits (G2/#66)" + fi + # ...and the file must stop claiming the gap it no longer has. + grep -qiE 'only the codex arm consumes' "$DR" \ + && bad "multi-review.md still says only codex consumes --session-root — stale now that the gemini arm does too" \ + || ok "no stale 'only codex consumes --session-root' claim" +fi + echo "packaging: $fails failure(s)"; [[ $fails -eq 0 ]] diff --git a/scripts/multi-review-reviewer.sh b/scripts/multi-review-reviewer.sh index 44802d2..94bce5f 100755 --- a/scripts/multi-review-reviewer.sh +++ b/scripts/multi-review-reviewer.sh @@ -368,16 +368,28 @@ cmd_check() { # --reviewer [--doc ] [--session-root ] -> 0 dispa local unreadable; unreadable="$(gemini_unreadable_paths "$rr")" [[ -z "$unreadable" ]] \ || hint gemini "gemini will refuse to read a review doc under $(fmt_paths "$unreadable") — git-ignored in this repo, and gemini-cli honors gitignore. Set context.fileFiltering.respectGitIgnore:false in .gemini/settings.json" - # Issue #60. gemini runs as a shell command in the CURRENT cwd, so unlike codex its - # workspace genuinely follows the shell and repo_root() is the right basis. The wording - # deliberately does not claim codex's per-session binding — gemini's sandbox constraint is - # reasoned by symmetry, not reproduced. + # BASIS: `--session-root` when the caller supplies one, else this helper's cwd repo. + # + # Issue #60 established the check; the basis was wrong in the same way codex's was (#66), + # and the earlier comment here argued the opposite — that because "gemini runs as a shell + # command in the CURRENT cwd", `repo_root()` was right. That holds only if the check and the + # dispatch stand in the same place, and nothing enforces it. gemini-cli's workspace is the + # cwd of the process at LAUNCH, so the root that matters is the one the DISPATCH will have, + # which is the session root the fan-out pins (see the command's shell branch). + # + # Using the helper's cwd was wrong in both directions, and the suite asserts both: it MISSED + # a copy outside the dispatch root (the failure #66 is about), and it FALSELY hinted on a + # copy that was inside the dispatch root but outside whatever repo the check happened to be + # standing in. + # + # Unresolvable basis -> SILENCE, not a hint on everything — same rule as the codex arm. if [[ -n "$doc" ]]; then - local grr_c gddir - grr_c="$(canon "$rr")" + local gbase gbase_c gddir + if [[ -n "$session_root" ]]; then gbase="$session_root"; else gbase="$rr"; fi + gbase_c="$(canon "$gbase")" gddir="$(dirname "$doc")" - if [[ -n "$grr_c" ]] && ! path_contains "$grr_c" "$gddir"; then - hint gemini "the review copy is outside this workspace ($(canon "$gddir") vs ${grr_c}) — gemini reads and writes within the workspace it is launched in, so it may be unable to write the copy. Run the review from the repo that contains it." + if [[ -n "$gbase_c" ]] && ! path_contains "$gbase_c" "$gddir"; then + hint gemini "the review copy is outside the workspace gemini will be launched in ($(canon "$gddir") vs ${gbase_c}) — gemini reads and writes only within that workspace, so it will likely be unable to write the copy. Dispatch from the root that contains it." fi fi ;; diff --git a/scripts/multi-review-reviewer.test.sh b/scripts/multi-review-reviewer.test.sh index 7bb98c2..21995c6 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -1361,6 +1361,43 @@ HOME="${CD_}/home" PATH="${CD_}/bin:$PATH" \ [[ $? -eq 2 ]] && ok "check --session-root: an EMPTY value exits 2, not a silent fallback (fable-rd1-r3)" \ || bad "check --session-root '' silently fell back to the companion basis" +# --- gemini consumes --session-root too (G2, the other half of #66) --- +# +# gemini-cli's workspace is the cwd of the process at LAUNCH, and it refuses reads and writes +# outside it. The check judged against `repo_root()` — the git toplevel of the CHECK invocation's +# cwd — which is only the same thing when check and dispatch happen to stand in one place, and +# nothing enforced that. The command file admitted the gap in writing rather than closing it: +# "gemini remains exposed to the same check-cwd-vs-dispatch-cwd drift this fixed for codex". +# +# Same trap as codex's S1: the doc lives in a repo the CHECK is standing in, while the session +# root the dispatched process will actually inherit does NOT contain it. The hint must fire. +GD="$(mktemp -d)" +mkdir -p "${GD}/ws" "${GD}/elsewhere/docs" "${GD}/bin" +( cd "${GD}/ws" && git init -q ) +( cd "${GD}/elsewhere" && git init -q ) +printf '# d\n' > "${GD}/elsewhere/docs/d.md" +printf '#!/usr/bin/env bash\n:\n' > "${GD}/bin/gemini"; chmod +x "${GD}/bin/gemini" + +out="$(cd "${GD}/elsewhere" && HOME="${GD}" GEMINI_CLI_TRUST_WORKSPACE=true PATH="${GD}/bin:$PATH" \ + bash "$SUT" check --reviewer gemini --doc "${GD}/elsewhere/docs/d.md" \ + --session-root "${GD}/ws" 2>&1 >/dev/null)"; rc=$? +{ [[ "$out" == *"hint (gemini)"* && "$out" == *"outside"* ]]; } \ + && ok "check gemini --session-root: fires when the copy is outside the root dispatch inherits (G2)" \ + || bad "gemini judged against its own cwd, not the session root — the #66 drift is still open for gemini (out='$out')" +[[ $rc -eq 0 ]] && ok "check gemini --session-root: the hint stays advisory (exit 0)" \ + || bad "gemini session-root hint became a gate (rc=$rc)" + +# ...and stays SILENT when the copy really is inside the root dispatch will inherit, even though +# the check is being run from somewhere else entirely. That is the whole point of the flag. +mkdir -p "${GD}/ws/docs"; printf '# d\n' > "${GD}/ws/docs/d.md" +out="$(cd "${GD}/elsewhere" && HOME="${GD}" GEMINI_CLI_TRUST_WORKSPACE=true PATH="${GD}/bin:$PATH" \ + bash "$SUT" check --reviewer gemini --doc "${GD}/ws/docs/d.md" \ + --session-root "${GD}/ws" 2>&1 >/dev/null)" +[[ "$out" != *"outside"* ]] \ + && ok "check gemini --session-root: silent when the copy is inside that root" \ + || bad "gemini hinted on a copy inside the session root (false positive): '$out'" +rm -rf "$GD" + # (8) gemini uses repo_root(), NOT the codex companion: a doc outside the cwd repo is hinted GD="${CD_}/grepo"; mkdir -p "$GD"; ( cd "$GD" && git init -q . ) out="$(cd "$GD" && HOME="${CD_}/home" FAKE_WS="${CD_}/ws" GEMINI_CLI_TRUST_WORKSPACE=true \ From 156968b69a9508f2fcdacb404fea28acd3a208dd Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Tue, 11 Aug 2026 18:57:30 -0500 Subject: [PATCH 2/3] perf(prompt): deliver the contract to fable by path, not inlined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inlining exists for ONE reason (issue #22): gemini-cli refuses any read outside its workspace root, and the protocol lives under the plugin cache — always outside the reviewed repo. That constraint belongs to dispatch-kind `shell`. It has never applied to `fable`, which runs IN-HARNESS as a subagent and can Read an absolute path anywhere. Applying gemini's sandbox to fable cost 14 KB on the most-dispatched prompt in the system: fable is the guaranteed floor reviewer, so it is the one provider present in every round of every run. fable 17,008 B -> 3,056 B (82% of it was the inlined contract) gemini 17,008 B unchanged (the #22 constraint is real) codex 2,702 B unchanged (byte-frozen golden still matches) prompt_head now has THREE shapes — skill / inline / path — keyed on dispatch-kind rather than a provider id, so a future shell-kind provider inherits the right behaviour without touching this. WHY THIS IS SOUND AND NOT MERELY CHEAPER: the prompt tail states the operative grammar in full regardless of shape, so a fable turn that never opens the file still emits findings `merge` can read. The file adds marker/scope detail, not the enforceable shape. The suite asserts each grammar token — the finding form, [no-findings], and the via/risk/evidence lines — survives WITHOUT the inline, and the prompt now tells the reviewer to stop and say so if the read fails rather than guess. Deliberate test change (§4): the inline assertions ran over `fable gemini` and pinned "no filesystem path may stand in for it". That encoded gemini's constraint as a universal, which is the bug. The loop is narrowed to gemini — where it is still exactly right and still asserted — and fable gets its own block asserting the path shape plus the grammar-survives-without-the-file property. Nothing was weakened: the gemini path keeps every assertion it had. Also drops the "Cost is ~7KB of prompt per dispatch" comment, which had drifted to 14 KB as the protocol grew. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE --- .claude-plugin/plugin.json | 2 +- scripts/multi-review-mutation-check.sh | 9 ++++ scripts/multi-review-reviewer.sh | 62 ++++++++++++++++++-------- scripts/multi-review-reviewer.test.sh | 38 +++++++++++++++- 4 files changed, 91 insertions(+), 20 deletions(-) diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index bc7c64f..37d92cf 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.22.1", + "version": "1.22.2", "author": { "name": "Agrology" }, diff --git a/scripts/multi-review-mutation-check.sh b/scripts/multi-review-mutation-check.sh index 473edda..e2ef57f 100755 --- a/scripts/multi-review-mutation-check.sh +++ b/scripts/multi-review-mutation-check.sh @@ -323,6 +323,15 @@ mutations() { ' local id; id="$(printf '"'"'%s'"'"' "$1" | LC_ALL=C tr '"'"'[:upper:]'"'"' '"'"'[:lower:]'"'"')"' \ ' local id; id="$1"' + # ---- C2: contract delivery is keyed on what the reviewer can reach --------------------------- + + # Collapse the three shapes back to two and fable inlines again — 14 KB on the most-dispatched + # prompt in the system, since fable is the floor reviewer present in every round of every run. + mutate 'reviewer/fable-contract-by-path' 'scripts/multi-review-reviewer.sh' replace \ + 'still inlines the whole contract' 'multi-review-reviewer.test.sh' \ + ' elif [[ "$2" == "shell" ]]; then' \ + ' elif true; then' + # ---- G2: gemini's half of #66 ---------------------------------------------------------------- # The check's BASIS. Reverted to the helper's cwd repo, the arm is wrong in both directions: diff --git a/scripts/multi-review-reviewer.sh b/scripts/multi-review-reviewer.sh index 94bce5f..2153143 100755 --- a/scripts/multi-review-reviewer.sh +++ b/scripts/multi-review-reviewer.sh @@ -423,22 +423,37 @@ protocol_body() { # review model (star) and one finding grammar — it is stated unconditionally in emit_prompt # below, never gated on a mode the reviewer has to detect. # -# Inline rather than by path (issue #22): $PROTOCOL is rooted at the PLUGIN root, which under a -# normal install is ~/.claude/plugins/cache//multi-review//... — always outside -# the repo being reviewed. gemini-cli refuses any read outside its workspace root -# ("Path not in workspace"), and no user setting relaxes that, so the one provider dispatched as -# an external CLI could never open the file. The in-repo copy is no fallback either: it is -# git-ignored twice over by ensure-skill (`.git/info/exclude` + an in-dir `.gitignore`), and -# gemini-cli honors gitignore by default. Inlining removes the filesystem from the path entirely, -# so the contract reaches every reviewer regardless of workspace root, ignore rules, or trust -# settings. Cost is ~7KB of prompt per dispatch. -prompt_head() { # +# THREE delivery shapes, chosen by what the reviewer can actually reach — not by a binary. +# +# skill (codex) — pointed at its own materialized bundle. +# inline (gemini) — the whole contract in the prompt. Issue #22: $PROTOCOL is rooted at the +# PLUGIN root, under a normal install ~/.claude/plugins/cache//multi-review//, +# always outside the repo being reviewed. gemini-cli refuses any read outside its workspace +# root ("Path not in workspace"), no user setting relaxes it, and the in-repo copy is no +# fallback either — ensure-skill git-ignores it twice over and gemini-cli honors gitignore. +# Inlining removes the filesystem from the path entirely. +# path (fable) — the absolute protocol path, read with its own tool. +# +# The inline shape used to cover fable too, and that was gemini's constraint applied to a provider +# that never had it: fable runs IN-HARNESS as a subagent and can Read an absolute path anywhere. +# It cost 14 KB on the most-dispatched prompt in the system, since fable is the guaranteed floor +# reviewer and is therefore present in every round of every run. +# +# Keyed on dispatch-kind rather than provider id: `shell` means an external CLI with its own +# workspace sandbox, which is exactly the population that cannot be sent to a path. A future +# shell-kind provider inherits the right behaviour without touching this. +# +# SAFETY NET, and the reason this is sound rather than merely cheaper: the prompt tail states the +# operative grammar in full regardless of shape. A fable turn that never opens the file still +# emits findings `merge` can read — the file adds marker/scope detail, not the enforceable shape. +# The suite asserts each grammar token survives without the inline. +prompt_head() { # if [[ "$1" == "yes" ]]; then cat <<'HEAD' You are a secondary reviewer in this repo's multi-review star review. Use your multi-review skill (it reads docs/multi-review.md and follows the star protocol). HEAD - else + elif [[ "$2" == "shell" ]]; then local body body="$(protocol_body)" || die "protocol contract not found at ${PROTOCOL}" 1 [[ -n "$body" ]] || die "protocol contract at ${PROTOCOL} is empty" 1 @@ -452,12 +467,23 @@ the copy-marker handoff. Follow it for the rest of this turn. ----- BEGIN MULTI-REVIEW PROTOCOL ----- ${body} ----- END MULTI-REVIEW PROTOCOL ----- +HEAD + else + [[ -f "$PROTOCOL" ]] || die "protocol contract not found at ${PROTOCOL}" 1 + cat < - local abs="$1" has_skill="$2" authority +emit_prompt() { # + local abs="$1" has_skill="$2" kind="${3:-}" authority # Who defines the finding grammar for this reviewer. Saying "your skill" to a skill-less # reviewer contradicts the head block, which just told it to read the protocol file. # The codex wording is byte-frozen; only the skill-less variant differs. @@ -466,7 +492,7 @@ emit_prompt() { # else authority="the protocol contract you just read" fi - prompt_head "$has_skill" + prompt_head "$has_skill" "$kind" cat < --reviewer [[ -n "$doc" ]] || die "usage: multi-review-reviewer.sh prompt --reviewer " 2 shift [[ -f "$doc" ]] || die "doc not found: $doc" 2 - local row has_skill + local row has_skill kind row="$(resolve_row "$@")" || exit 2 - has_skill="$(field "$row" 5)" - emit_prompt "$(abs_path "$doc")" "$has_skill" + has_skill="$(field "$row" 5)"; kind="$(field "$row" 3)" + emit_prompt "$(abs_path "$doc")" "$has_skill" "$kind" } cmd_command() { # --reviewer -> NUL-delimited argv @@ -539,7 +565,7 @@ cmd_command() { # --reviewer -> NUL-delimited argv # protocol contract) exits only that subshell. Unchecked, we would emit a perfectly # well-formed argv carrying a truncated prompt — the reviewer would run with no contract at # all. Propagate instead. (Same subshell-die hazard resolve_row documents above.) - prompt="$(emit_prompt "$(abs_path "$doc")" "$has_skill")" || exit $? + prompt="$(emit_prompt "$(abs_path "$doc")" "$has_skill" "$kind")" || exit $? # NUL-delimited: the prompt is multi-line and doc paths contain spaces, so the caller must # 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") diff --git a/scripts/multi-review-reviewer.test.sh b/scripts/multi-review-reviewer.test.sh index 21995c6..8a33ad7 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -181,7 +181,7 @@ grep -qi 'multi-review skill' <<<"$out" && ok "codex prompt references its skill # path may stand in for it. protocol_src="$(cd "$(dirname "$SUT")/.." && pwd)/.agents/skills/multi-review/protocol/multi-review.md" [[ -f "$protocol_src" ]] || { echo "FIXTURE SETUP FAILED: protocol not at $protocol_src"; exit 1; } -for p in fable gemini; do +for p in gemini; do out="$(bash "$SUT" prompt "$D" --reviewer "$p" 2>/dev/null)" grep -qiE 'read the protocol contract in full' <<<"$out" \ && ok "prompt($p) instructs reading the protocol" || bad "prompt($p) lacks the read instruction" @@ -218,6 +218,42 @@ for p in fable gemini; do || bad "prompt($p) does not name the protocol contract as the mode authority" done +# --- fable gets the contract BY PATH, not inlined (C2) --- +# +# Inlining exists for ONE reason (issue #22): gemini-cli refuses any read outside its workspace +# root, and the protocol lives under the plugin cache, always outside the reviewed repo. That +# constraint is dispatch-kind `shell`; it has never applied to `fable`, which runs in-harness as a +# subagent and can Read an absolute path. Applying gemini's sandbox to fable cost 14 KB on the +# MOST-dispatched prompt in the system — fable is the guaranteed floor reviewer, so it is the one +# provider present in every round of every run. +# +# The safety net is what makes this sound rather than merely cheaper: the head+tail still states +# the operative grammar in full, so a fable turn that never opens the file still emits findings +# `merge` can read. The file adds the marker/scope detail, not the enforceable shape. +out="$(bash "$SUT" prompt "$D" --reviewer fable 2>/dev/null)" +! grep -qF 'BEGIN MULTI-REVIEW PROTOCOL' <<<"$out" \ + && ok "prompt(fable) does not inline the contract" \ + || bad "prompt(fable) still inlines the whole contract — 82% of the most-dispatched prompt" +grep -qF 'protocol/multi-review.md' <<<"$out" \ + && ok "prompt(fable) names the protocol path to read" \ + || bad "prompt(fable) neither inlines the contract nor says where to find it" +grep -qiE 'read it (first|in full)|before (you )?(do anything|editing)' <<<"$out" \ + && ok "prompt(fable) instructs reading it up front" || bad "prompt(fable) does not say to read the contract first" +grep -qiE 'cannot read|unable to read' <<<"$out" \ + && ok "prompt(fable) says what to do if the read fails" \ + || bad "prompt(fable) has no instruction for an unreadable contract — a silent skip is a malformed turn" +# the grammar must survive WITHOUT the file, or a failed read costs the round +for tok in '[finding:|]' '[no-findings]' '— via' '— risk:' '— evidence:'; do + grep -qF "$tok" <<<"$out" \ + && ok "prompt(fable) states '${tok}' without the inlined contract" \ + || bad "prompt(fable) lost '${tok}' — a failed read would now produce an unreadable turn" +done +fb_b=$(printf '%s' "$out" | wc -c | tr -d ' ') +gm_b=$(bash "$SUT" prompt "$D" --reviewer gemini 2>/dev/null | wc -c | tr -d ' ') +(( fb_b * 3 < gm_b )) \ + && ok "prompt(fable) is far smaller than the inlined one (${fb_b} B vs ${gm_b} B)" \ + || bad "prompt(fable) is ${fb_b} B against gemini's ${gm_b} B — the inlining was not actually removed" + # --- prompt: never hardcodes the RETIRED asymmetric/peer-review grammar (superseded by star) --- for p in codex fable gemini; do out="$(bash "$SUT" prompt "$D" --reviewer "$p" 2>/dev/null)" From 309001965dccdcb47e3da6e188e6e42bc1a208f1 Mon Sep 17 00:00:00 2001 From: Kevin Kelly Date: Tue, 11 Aug 2026 20:13:23 -0500 Subject: [PATCH 3/3] style: silence SC2043 on the now-single-element inline loop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrowing the inline assertions from `fable gemini` to `gemini` left a one-element loop, which shellcheck flags as probably-a-mistake. It is not: the loop is over a POPULATION — every provider whose contract must be inlined — which has exactly one member today because `shell` is the only dispatch-kind that cannot be sent to a path. Kept as a loop so a second shell-kind provider joins by being added to the list and inherits every assertion, with the reason recorded next to the disable. Caught by CI, not locally: shellcheck is not installed on this machine, so the gate's shellcheck step is the only place this could surface. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Nm3hNXA4WHbgKjFbxX7VUE --- scripts/multi-review-reviewer.test.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/multi-review-reviewer.test.sh b/scripts/multi-review-reviewer.test.sh index 8a33ad7..e3520ed 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -181,6 +181,10 @@ grep -qi 'multi-review skill' <<<"$out" && ok "codex prompt references its skill # path may stand in for it. protocol_src="$(cd "$(dirname "$SUT")/.." && pwd)/.agents/skills/multi-review/protocol/multi-review.md" [[ -f "$protocol_src" ]] || { echo "FIXTURE SETUP FAILED: protocol not at $protocol_src"; exit 1; } +# Kept as a loop over a POPULATION — "every provider whose contract is inlined" — which today has +# exactly one member because `shell` is the only dispatch-kind that cannot be sent to a path. A +# second shell-kind provider joins by being added here, and inherits every assertion below. +# shellcheck disable=SC2043 # single-element by circumstance, not by mistake for p in gemini; do out="$(bash "$SUT" prompt "$D" --reviewer "$p" 2>/dev/null)" grep -qiE 'read the protocol contract in full' <<<"$out" \