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 6cbc50e..e296146 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..e3520ed 100755 --- a/scripts/multi-review-reviewer.test.sh +++ b/scripts/multi-review-reviewer.test.sh @@ -181,7 +181,11 @@ 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 +# 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" \ && ok "prompt($p) instructs reading the protocol" || bad "prompt($p) lacks the read instruction" @@ -218,6 +222,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)"