diff --git a/README.md b/README.md index 53c5e21..6315a2d 100644 --- a/README.md +++ b/README.md @@ -270,7 +270,7 @@ Reviewers run **in the order listed**, and whatever you list is exactly what run **Per-reviewer iteration caps** (`~max=` suffix): caps how many **review → fix → re-review cycles** that one reviewer runs. It is the per-entry form of `--review-iterations`, and unlike that flag it reaches every reviewer type — including `codex`/`agy`/`claude`/`grok`/`cursor` and `ollama`, whose caps are otherwise fixed at 3 — so a single run can budget each reviewer separately: `--review-with claude~max=2,ollama~max=1,codex~max=3`. `` is a non-negative integer; `0` means "loop until clean", bounded by a 10-iteration safety guardrail. A reviewer that stops because it spent a cap *you* set reports `capped`, which counts as clean for the merge gate — as opposed to `guardrail`, which is what a *built-in* cap reports when it cuts off a reviewer that was still finding real problems, and which blocks the merge. -**Per-reviewer reasoning effort** (`~effort=` suffix): specifies the reasoning effort level (`low`, `medium`, `high`, `xhigh`, `max`) for that reviewer: `--review-with codex[gpt-5.6-luna]~effort=max~opt`, `--review-with claude~effort=high~max=2`, `--review-with cursor[gpt-5]~effort=max`. For Cursor the suffix is folded into `--model` as `[effort=]` (the CLI has no `--effort` flag); pair it with a `cursor[]` bracket or a saved `--review-models cursor=…` default so there is a model to attach the variant to. +**Per-reviewer reasoning effort** (`~effort=` suffix): specifies the reasoning effort level (`low`, `medium`, `high`, `xhigh`, `max`) for that reviewer: `--review-with codex[gpt-5.6-luna]~effort=max~opt`, `--review-with claude~effort=high~max=2`, `--review-with cursor[gpt-5]~effort=max`. Each reviewer receives it in the form its own CLI accepts — `--effort` is **not** universal. `claude` and `grok` take the flag; **codex** takes `-c model_reasoning_effort=`; **Cursor** folds it into `--model` as `[effort=]`, so pair that one with a `cursor[]` bracket or a saved `--review-models cursor=…` default; and **agy** picks the matching model variant from whatever `agy models` lists. Where a reviewer offers no such control — or no level matching what you asked — the effort falls back to prompt guidance rather than failing the review. `~max` applies in `series` mode (the default). In `--review-mode parallel` each reviewer runs a single review-only pass and the orchestrator applies the union once, so there are no per-reviewer cycles to cap — `~max` is ignored there with a warning. diff --git a/commands/do/rpr.md b/commands/do/rpr.md index 7bd33c2..028182b 100644 --- a/commands/do/rpr.md +++ b/commands/do/rpr.md @@ -41,7 +41,7 @@ Parse `$ARGUMENTS` for `--issues` / `--no-issues` / `--issues-label `: whe Note whether **any** completed review exists (from a copilot bot, a human, or another bot) — call this `HAS_EXISTING_REVIEW` — and specifically whether a **completed** `copilot-pull-request-reviewer` review exists (a node in `reviews.nodes`, NOT merely a pending review request) — call this `HAS_COPILOT_REVIEW`. Track a Copilot review that is only **pending** (Copilot present in `reviewRequests.nodes[].requestedReviewer` with no completed Copilot review yet) separately as `COPILOT_REVIEW_PENDING` — a pending-only review must NOT set `HAS_COPILOT_REVIEW`, or the "completed review exists" branch below would fire and resolve threads before Copilot has posted anything. Then dispatch on `REVIEW_AGENTS`: - **If `REVIEW_AGENTS` is empty** (no `--review-with`, no saved default, or an explicit `none`): request **no** review — not Copilot, not anything else. Proceed straight to step 3 and resolve whatever unresolved threads the PR already carries. - - **If `REVIEW_AGENTS` contains a local CLI (`codex`/`agy`/`claude`/`grok`/`cursor`):** run the **local-agent review loop** (`lib/local-agent-review-loop.md`, referenced below) for each such agent against the PR branch, forwarding `REVIEWER_APPLIES`, that entry's resolved `{REVIEW_MODEL}` (bracket, else saved `review-models[slug]` default, else empty → built-in default — see the loop reference below), that entry's `{REVIEW_EFFORT}` (from its `~effort=`; empty when it carried none — Cursor folds this into `--model`, the other CLIs take it as `--effort`), and that entry's `{MAX_ITERATIONS}` / `{MAX_EXPLICIT}` (from its `~max=`; the built-in `3` / `false` when it carried none). This produces findings (and, in reviewer-applies mode, fixes) locally — it does **not** request a Copilot cloud review for those slugs. Then proceed to step 3 to fetch and resolve any pre-existing unresolved threads as well. (If `REVIEW_AGENTS` also contains `copilot`, additionally run the Copilot path below.) + - **If `REVIEW_AGENTS` contains a local CLI (`codex`/`agy`/`claude`/`grok`/`cursor`):** run the **local-agent review loop** (`lib/local-agent-review-loop.md`, referenced below) for each such agent against the PR branch, forwarding `REVIEWER_APPLIES`, that entry's resolved `{REVIEW_MODEL}` (bracket, else saved `review-models[slug]` default, else empty → built-in default — see the loop reference below), that entry's `{REVIEW_EFFORT}` (from its `~effort=`; empty when it carried none — the loop maps it to each CLI's accepted carrier), and that entry's `{MAX_ITERATIONS}` / `{MAX_EXPLICIT}` (from its `~max=`; the built-in `3` / `false` when it carried none). This produces findings (and, in reviewer-applies mode, fixes) locally — it does **not** request a Copilot cloud review for those slugs. Then proceed to step 3 to fetch and resolve any pre-existing unresolved threads as well. (If `REVIEW_AGENTS` also contains `copilot`, additionally run the Copilot path below.) - **If `REVIEW_AGENTS` contains `ollama`:** run the **Ollama review loop** (`lib/ollama-review-loop.md`, referenced below) for each `ollama` entry against the PR branch (which must be checked out locally — the loop reviews a local `git diff`), forwarding that entry's `{OLLAMA_MODEL}` (empty = auto-select), its `{OLLAMA_EFFORT}` (from its `~effort=`; empty when it carried none), and its `{MAX_ITERATIONS}` / `{MAX_EXPLICIT}` (from its `~max=`; the built-in `3` / `false` when it carried none). Like the local-CLI path it produces findings the orchestrator applies locally (Ollama is always review-only) and does **not** request a Copilot review. Then proceed to step 3 to resolve any pre-existing threads. (If `REVIEW_AGENTS` also contains `copilot`, additionally run the Copilot path below.) - **If `REVIEW_AGENTS` contains `copilot`** (only ever because you asked for it — typed or saved): a `copilot~max=` cap applies to this path too — see the cap-accounting rule in step 8. rpr accepts no `--review-iterations`, so `~max` is the only budget signal here, and each Copilot round below counts against it. - **A completed Copilot review exists** (`HAS_COPILOT_REVIEW`): skip requesting a new one — proceed to step 3 to address its threads. @@ -128,7 +128,7 @@ Parse `$ARGUMENTS` for `--issues` / `--no-issues` / `--issues-label `: whe ## Local-Agent Review Loop (for `--review-with codex|agy|claude|grok|cursor`) -When `REVIEW_AGENTS` names a local CLI, step 2 (and the step-8 re-request) runs that agent's review against the PR branch via the shared local-agent loop. Pass `{REVIEW_AGENT}`, `{REVIEWER_APPLIES}`, that entry's resolved `{REVIEW_MODEL}` (the `[]` bracket if the token carried one, else the saved `review-models[slug]` default resolved above — project over global, else empty → the reviewer's built-in default), that entry's `{REVIEW_EFFORT}` (from its `~effort=`; empty when unset — without it Cursor cannot fold effort into `--model` and the other CLIs cannot pass `--effort`), the PR branch (`headRefName`), the base branch (`baseRefName`), and the project `{BUILD_CMD}`. Forwarding `{REVIEW_MODEL}` is what makes `--review-with=codex[o3]` and a saved `review-models` default actually pin the model on rpr's local passes — without it those passes would silently run the CLI's default model. The loop verifies build + tests in the main thread before pushing; afterward, continue to step 3 to resolve any pre-existing threads. +When `REVIEW_AGENTS` names a local CLI, step 2 (and the step-8 re-request) runs that agent's review against the PR branch via the shared local-agent loop. Pass `{REVIEW_AGENT}`, `{REVIEWER_APPLIES}`, that entry's resolved `{REVIEW_MODEL}` (the `[]` bracket if the token carried one, else the saved `review-models[slug]` default resolved above — project over global, else empty → the reviewer's built-in default), that entry's `{REVIEW_EFFORT}` (from its `~effort=`; empty when unset — the loop's effort-carrier table maps it to each CLI's accepted form), the PR branch (`headRefName`), the base branch (`baseRefName`), and the project `{BUILD_CMD}`. Forwarding `{REVIEW_MODEL}` is what makes `--review-with=codex[o3]` and a saved `review-models` default actually pin the model on rpr's local passes — without it those passes would silently run the CLI's default model. The loop verifies build + tests in the main thread before pushing; afterward, continue to step 3 to resolve any pre-existing threads. !`cat ~/.claude/lib/local-agent-review-loop.md` diff --git a/lib/local-agent-review-loop.md b/lib/local-agent-review-loop.md index bd83ae1..5a95ea5 100644 --- a/lib/local-agent-review-loop.md +++ b/lib/local-agent-review-loop.md @@ -47,7 +47,7 @@ When to use this: 5. Record `{REVIEWER_APPLIES}` — boolean, defaults to `false`. Set to `true` when the orchestrating command was invoked with `--reviewer-applies`. This flag selects which side of the loop holds the editor: when `false` (default), the orchestrator applies fixes from the CLI's findings log; when `true`, the headless CLI applies fixes directly in the working tree and the orchestrator only verifies. 6. Record `{REVIEW_MODEL}` — the model to run this reviewer on, resolved by the caller (the multi-reviewer loop: explicit `[]` bracket → saved `review-models[slug]` default → empty). **May be empty**, which means "use the reviewer's built-in default" — for `codex`/`claude`/`grok`/`cursor` that is the CLI's own default model (no `--model` flag passed); for `agy` it is the pinned `AGY_REVIEW_MODEL` default resolved below. When set, it is passed through to the reviewer's invocation (`codex --model`, `claude --model` / the in-process `Agent` tool's `model`, `agy --model`, `grok --model`, or `cursor --model`) so a run/config can pin which model reviews. The value is free-form (model names churn and may contain spaces/parens, e.g. `Gemini 3.5 Flash (High)`) — do not validate it against an allowlist; pass it verbatim. 7. Record `{MAX_ITERATIONS}` — how many review → fix → re-review cycles this reviewer may run, resolved by the caller (the multi-reviewer loop: a per-entry `~max=` suffix on the `--review-with` token → this loop's built-in default of `3`). **Defaults to `3`** when the caller passes nothing, which is the historical behavior. `0` means **unlimited** — loop until the reviewer is clean or the convergence gate converges, bounded by the 10-iteration safety guardrail in Step 6. Also record `{MAX_EXPLICIT}` — boolean, `true` only when the cap came from a `~max=` the user typed (or saved), `false` when it is this loop's built-in `3`. Step 6 uses it to decide whether exhausting the cap is `capped` (a budget the user chose — clean-equivalent for the merge gate) or `guardrail` (a built-in ceiling nobody vouched for — inconclusive). Note the `--review-iterations` flag never reaches this loop; `~max` is the only way to move this cap. -8. Record `{REVIEW_EFFORT}` — optional reasoning effort string for this reviewer (`low`, `medium`, `high`, `xhigh`, `max`), resolved by the caller (the multi-reviewer loop: explicit `~effort=` suffix on the `--review-with` token → empty). **Defaults to empty** when unset. When set, it is appended as advisory reasoning effort to the prompt preamble and passed as `--effort` where supported — which is a CLI flag on the subprocess paths, folded into `--model` for `cursor`, and prompt-only for the in-process Claude sub-agent, which has no effort parameter to pass (Step 2). +8. Record `{REVIEW_EFFORT}` — optional reasoning effort string for this reviewer (`low`, `medium`, `high`, `xhigh`, `max`), resolved by the caller (the multi-reviewer loop: explicit `~effort=` suffix on the `--review-with` token → empty). **Defaults to empty** when unset. When set, it is appended as advisory reasoning effort to the prompt preamble and *also* passed to the CLI in whatever form that CLI accepts. The carriers differ per agent — see the effort-carrier table below, which the pre-flight `case` implements. Never assume `--effort` is universal. ### Editing mode @@ -131,30 +131,36 @@ elif command -v gtimeout >/dev/null 2>&1; then TIMEOUT_CMD=(gtimeout 1800); fi # (built-in default), so its flag is never empty. MODEL_FLAG=() [ -n "$REVIEW_MODEL" ] && MODEL_FLAG=(--model "$REVIEW_MODEL") +# Reasoning effort carrier. Each reviewer CLI takes effort in a DIFFERENT form, +# so build it per agent -- and default to NO flag, not to `--effort`. That +# default matters: `--effort` is correct for only two of these CLIs, and the +# unknown-agent arm must degrade to prompt-advisory effort (the "Target +# reasoning effort level" sentence $LOCAL_PROMPT already carries) rather than +# guess a flag. A wrong guess is not a weaker review -- it is a non-zero exit +# before the review runs, which fills that reviewer's merge-gate slot with a +# launch failure. See the effort-carrier table below for the per-agent forms. EFFORT_FLAG=() -[ -n "$REVIEW_EFFORT" ] && EFFORT_FLAG=(--effort "$REVIEW_EFFORT") - -# Cursor has no --effort flag (passing it exits non-zero). Its native effort -# control is a model-variant parameter — `gpt-5[effort=max]`, or -# `claude-opus-4-7[thinking=true,effort=high]`. Fold {REVIEW_EFFORT} into -# --model so `cursor[gpt-5]~effort=max` and a saved -# `--review-models cursor=gpt-5` plus `cursor~effort=max` actually change -# inference, matching the other reviewers' ~effort behavior. A model string -# that already carries `effort=` (typed in the bracket or saved in -# review-models) is left alone. Effort with no model stays prompt-advisory -# only — there is nothing to attach the variant to. -if [ "$REVIEW_AGENT" = cursor ]; then - EFFORT_FLAG=() - if [ -n "$REVIEW_EFFORT" ] && [ -n "$REVIEW_MODEL" ]; then - case "$REVIEW_MODEL" in - *effort=*) CURSOR_MODEL="$REVIEW_MODEL" ;; - *\[*\]) CURSOR_MODEL="${REVIEW_MODEL%]},effort=${REVIEW_EFFORT}]" ;; - *) CURSOR_MODEL="${REVIEW_MODEL}[effort=${REVIEW_EFFORT}]" ;; - esac - MODEL_FLAG=(--model "$CURSOR_MODEL") - fi +if [ -n "$REVIEW_EFFORT" ]; then + case "$REVIEW_AGENT" in + claude|grok) EFFORT_FLAG=(--effort "$REVIEW_EFFORT") ;; + codex) EFFORT_FLAG=(-c "model_reasoning_effort=$REVIEW_EFFORT") ;; + cursor) + # Effort is a model-variant parameter; fold it into --model. A model + # string that already carries `effort=` is left alone, and effort with no + # model stays prompt-advisory (nothing to attach the variant to). + if [ -n "$REVIEW_MODEL" ]; then + case "$REVIEW_MODEL" in + *effort=*) CURSOR_MODEL="$REVIEW_MODEL" ;; + *\[*\]) CURSOR_MODEL="${REVIEW_MODEL%]},effort=${REVIEW_EFFORT}]" ;; + *) CURSOR_MODEL="${REVIEW_MODEL}[effort=${REVIEW_EFFORT}]" ;; + esac + MODEL_FLAG=(--model "$CURSOR_MODEL") + fi + ;; + agy) : ;; # effort is a model variant, resolved from `agy models` below + *) : ;; # unknown agent: prompt-advisory only -- never guess a flag + esac fi - # agy only: pin the review model. A per-run/config model wins via {REVIEW_MODEL} # (the `agy[]` bracket or a saved `review-models` default), then the # AGY_REVIEW_MODEL env var, then the built-in default below. agy's DEFAULT can be a heavy "Thinking" model @@ -174,10 +180,35 @@ fi # (e.g. `agy models`) — a nested agy invocation inside a print session can stall. # Precedence: bracket/config-resolved {REVIEW_MODEL} > AGY_REVIEW_MODEL env > built-in default. AGY_REVIEW_MODEL="${REVIEW_MODEL:-${AGY_REVIEW_MODEL:-Gemini 3.5 Flash (High)}}" +# agy effort: resolved as a model variant (see the effort-carrier table), so +# print agy's own roster for the selection step below. Do not hardcode a level +# vocabulary or a name shape -- both change between agy releases. This runs in +# the ORCHESTRATOR's shell; the NOTE above bans `agy models` from the reviewer +# PROMPT (a nested agy call inside a print session stalls), not from here. +if [ "$REVIEW_AGENT" = agy ] && [ -n "$REVIEW_EFFORT" ] && [ -z "$AGY_MODEL_RESOLVED" ]; then + agy models 2>/dev/null +fi ``` Run the pre-flight block above verbatim. The `TIMEOUT_CMD` resolution is deterministic — do NOT think out loud about whether `timeout`/`gtimeout` is installed or about falling back; just execute it and move on. +**Effort carriers.** `{REVIEW_EFFORT}` reaches each reviewer in the one form its CLI accepts. The pre-flight `case` above builds it; this table is the rule, and the per-agent bullets under "Flag rationale" below record the verified failures behind it. **Never assume `--effort` is universal** — three of these reject it outright, and a rejected flag is a non-zero exit before the review runs, not a weaker review. + +| Agent | Effort carrier | +|-------|----------------| +| `claude` (subprocess) / `grok` | `--effort ` | +| `claude` (in-process sub-agent) | prompt-advisory only — the `Agent` tool has no effort parameter | +| `codex` | `-c model_reasoning_effort=` (top-level config override; **no** `--effort` flag exists) | +| `cursor` | folded into `--model` as `[effort=]` | +| `agy` | a model **variant** picked from `agy models` (see below) | +| anything else | prompt-advisory only — never guess a flag | + +**Selecting agy's effort variant** (only when `{REVIEW_AGENT}` is `agy` and `{REVIEW_EFFORT}` is set). agy encodes effort as a model variant and rejects `--effort` whenever `--model` is pinned, which this loop always does. The pre-flight printed `agy models` — one entry per line, id and display name. Choose from **that listing**, not from a remembered table (the roster and level names change between releases): + +- Take the entry that is the same base model as the resolved `AGY_REVIEW_MODEL` at the requested level. If agy offers no exact match, take the **closest level it does offer** and say which you took — `~effort=max` against a base topping out at "High" means High, since the intent is "as much reasoning as this reviewer has," not "abort because the ceiling is lower than asked." +- If the base has no variants, or `agy models` printed nothing (offline, not signed in), keep `AGY_REVIEW_MODEL` as-is — effort stays prompt-advisory. Never invent a variant that wasn't listed: a base that merely *looks* like it has variants becomes a model agy rejects, trading a degraded review for a launch failure. +- **Record the choice.** Set `AGY_REVIEW_MODEL` to the chosen entry and reuse that literal string in every Step 2 invocation for the rest of this loop, and set `AGY_MODEL_RESOLVED=1`. Pre-flight is re-materialized on each review → fix → re-review iteration (shell variables do not survive between Bash calls), so without this the roster is re-fetched and the choice re-derived every cycle. + Pick the invocation based on `{REVIEW_AGENT}` and `{REVIEWER_APPLIES}`: | Agent | Review-only (`REVIEWER_APPLIES=false`, default) | Reviewer-applies (`REVIEWER_APPLIES=true`) | @@ -188,7 +219,7 @@ Pick the invocation based on `{REVIEW_AGENT}` and `{REVIEWER_APPLIES}`: | `claude` | `claude -p "$LOCAL_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} --dangerously-skip-permissions` | `claude -p "$LOCAL_PROMPT" ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} --dangerously-skip-permissions` | | `codex` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} --sandbox read-only review --base "$BASE_BRANCH" --title "$REVIEW_TITLE"` | `codex ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} --sandbox danger-full-access -a never exec "$CODEX_APPLY_PROMPT"` | -| `agy` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} --print-timeout 30m -p "$LOCAL_PROMPT"` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} --print-timeout 30m -p "$LOCAL_PROMPT"` | +| `agy` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m -p "$LOCAL_PROMPT"` | `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m -p "$LOCAL_PROMPT"` | | `grok` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} -p "$LOCAL_PROMPT"` | `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} ${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"} -p "$LOCAL_PROMPT"` | | `cursor` | `"$REVIEW_BIN" -p --trust --mode=ask --output-format text ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} "$LOCAL_PROMPT"` | `"$REVIEW_BIN" -p --force --trust --output-format text --sandbox disabled ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} "$LOCAL_PROMPT"` | @@ -196,7 +227,7 @@ For `claude`, `agy`, `grok`, and `cursor`, the same `$LOCAL_PROMPT` drives both > **Pass the prompt as a positional argument — never via stdin.** `claude -p`, `agy -p` (`--print`), `grok -p` (`--single`), and `cursor-agent -p` (`--print`) all take the prompt as the argument directly after the flag: `agy --dangerously-skip-permissions -p "$LOCAL_PROMPT"`, `grok --permission-mode bypassPermissions -p "$LOCAL_PROMPT"`, `"$REVIEW_BIN" -p --trust --mode=ask "$LOCAL_PROMPT"`. They do **not** read the prompt from stdin. Do NOT write `echo "$LOCAL_PROMPT" | agy --dangerously-skip-permissions -p`, `agy -p < prompt.txt`, or `printf … | agy -p` — agy ignores piped stdin and exits with `agy --print takes the prompt as an argument, not stdin`, forcing a wasted second invocation. The `> "$LOG_FILE" 2> "$ERR_FILE"` redirect in Step 2 captures the reviewer's *output*; it is unrelated to how the prompt goes in. Keep `"$LOCAL_PROMPT"` as the quoted argument to `-p` exactly as shown in the invocation table. -**Pinning the reviewer's model (`${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` / `--model`) and reasoning effort (`${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"}` / `--effort`).** When `{REVIEW_MODEL}` / `{REVIEW_EFFORT}` is set (from an `[]` bracket, a `~effort=` suffix, or a saved default — resolved by the caller), the reviewer runs on that model and effort level; when empty, `MODEL_FLAG` / `EFFORT_FLAG` are empty arrays so `codex`/`claude`/`agy`/`grok`/`cursor` fall back to their default or session values. **Do not pass `EFFORT_FLAG` to `cursor`** — the Cursor CLI has no `--effort` flag and would exit non-zero. Instead the pre-flight block above **folds `{REVIEW_EFFORT}` into the `--model` value** as Cursor's native variant parameter (`gpt-5` + `~effort=max` → `--model gpt-5[effort=max]`; a bracket that already has params gets `,effort=` appended; a model string that already contains `effort=` is left alone). That is what makes `cursor[gpt-5]~effort=max` and `/do:config --review-models cursor=gpt-5` plus `cursor~effort=max` actually change inference, the same way `--effort` does for the other CLIs. The advisory "Target reasoning effort level" sentence in `$REVIEW_TASK` still fires (covers the no-model case, where there is nothing to fold into). For **codex**, `-m`/`--model` and `--effort` are **top-level** Codex options (like `--sandbox` and `-a`), so they MUST precede the `review`/`exec` subcommand — that is why `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` and `${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"}` sit before `--sandbox` in both codex invocations; passing them after the subcommand would exit 2 with an unexpected-argument error, exactly as `-a` does. (The two paths pass *different* sandbox policies — `read-only` for review-only, `danger-full-access` for reviewer-applies — see below.) For **claude**, `--model` is a session flag valid alongside `-p`. For **grok**, `-m`/`--model` is a session flag valid alongside `-p`, so `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` sits inline in the invocation (empty array → grok's own default). For **agy**, the model is always pinned via `--model "$AGY_REVIEW_MODEL"` (resolved above with `{REVIEW_MODEL}` taking precedence over the `AGY_REVIEW_MODEL` env and the built-in default) — agy's own default may be a slow "Thinking" tier, so it is never left unpinned. Because the model string may contain spaces/parens, `MODEL_FLAG` is a shell array (see the pre-flight block) — never a bare string. +**Pinning the reviewer's model (`${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` / `--model`) and reasoning effort (`${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"}` / `--effort`).** When `{REVIEW_MODEL}` / `{REVIEW_EFFORT}` is set (from an `[]` bracket, a `~effort=` suffix, or a saved default — resolved by the caller), the reviewer runs on that model and effort level; when empty, `MODEL_FLAG` / `EFFORT_FLAG` are empty arrays so `codex`/`claude`/`agy`/`grok`/`cursor` fall back to their default or session values. Effort is **not** a universal `--effort` — the pre-flight `case` builds the per-agent carrier listed in the effort-carrier table above (`cursor` and `agy` fold it into `--model`; `codex` takes `-c model_reasoning_effort=`), and the "Flag rationale" bullets below record why each. The advisory "Target reasoning effort level" sentence in `$REVIEW_TASK` still fires wherever a carrier can't be built. For **codex**, `-m`/`--model` and `-c` are **top-level** Codex options (like `--sandbox` and `-a`), so they MUST precede the `review`/`exec` subcommand — that is why `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` and `${EFFORT_FLAG[@]+"${EFFORT_FLAG[@]}"}` sit before `--sandbox` in both codex invocations; `-c` happens to be accepted by the subcommands too, but `-m`/`--model` after the subcommand would exit 2 with an unexpected-argument error, exactly as `-a` does. (The two paths pass *different* sandbox policies — `read-only` for review-only, `danger-full-access` for reviewer-applies — see below.) For **claude**, `--model` is a session flag valid alongside `-p`. For **grok**, `-m`/`--model` is a session flag valid alongside `-p`, so `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` sits inline in the invocation (empty array → grok's own default). For **agy**, the model is always pinned via `--model "$AGY_REVIEW_MODEL"` (resolved above with `{REVIEW_MODEL}` taking precedence over the `AGY_REVIEW_MODEL` env and the built-in default) — agy's own default may be a slow "Thinking" tier, so it is never left unpinned — and that pinned model is also where its effort lives. Because the model string may contain spaces/parens, `MODEL_FLAG` is a shell array (see the pre-flight block) — never a bare string. Notes on each invocation: - **claude / agy / grok / cursor** run the self-contained `$LOCAL_PROMPT` (a single-agent inline review), **not** slashdo's `/do-review` skill — the skill's sub-agent fan-out never re-syncs into a print-mode/headless response, so it would hang and emit zero findings (see the `$LOCAL_PROMPT` rationale above). Under Claude Code the `claude` reviewer is an in-process sub-agent (via the `Agent` tool) that runs `$LOCAL_PROMPT` directly, rather than a `claude -p` subprocess — and because the prompt is a single-agent inline review, it does not recursively spawn the skill's own sub-agents. In `REVIEWER_APPLIES=true` mode, `$LOCAL_PROMPT` tells the CLI to apply each fix, verify with build+tests, commit as `address review (): ` (`` = the reviewing CLI's slug, `claude`, `agy`, `grok`, or `cursor`), and NOT push (the orchestrating agent verifies and pushes). The parenthesized agent name records which reviewer surfaced the finding, useful when scanning the log of a release that ran multiple reviewers. In `REVIEWER_APPLIES=false` mode, `$LOCAL_PROMPT` tells the CLI to emit `FINDING :` blocks (or `NO FINDINGS`) to stdout for the orchestrator to parse — the orchestrator then commits the fixes using the same `address review (): ` form to preserve attribution. @@ -211,7 +242,8 @@ Flag rationale (reckless / unattended mode): - **review-only → `read-only`.** Verified: `codex --sandbox read-only review --base ` reads the diff, tracked-file list, commit graph and base tree and returns normal severity-tagged findings, while `printf … > file` inside the repo fails with `zsh:1: operation not permitted`. Review quality is unaffected and the contract becomes unbypassable. This matches `lib/enhance-loop.md`, which already runs codex `--sandbox read-only` for the same reason. - **reviewer-applies → `danger-full-access`.** This path must write fixes, run build/tests, and reach the network unattended, so full access is the intended posture on a trusted single-user machine (mirrors `claude --dangerously-skip-permissions` / `agy --dangerously-skip-permissions`). - `--sandbox` and `-a` are independent top-level flags and may be combined (`codex --sandbox danger-full-access -a never exec …`). -- `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m` — `--dangerously-skip-permissions` auto-approves all tool permission requests so the Antigravity CLI runs unattended (the headless equivalent of confirming every prompt). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight, default `Gemini 3.5 Flash (High)`, override via `AGY_REVIEW_MODEL`): without it agy picks its own default, which may be a heavy "Thinking" tier that spends many minutes in hidden reasoning and — depending on the model, emits little or no visible output meanwhile — makes a review look hung for 20-30 minutes; a fast capable model returns in well under a minute on a small diff. This is the agy successor to the Gemini CLI's `gemini --yolo` + `env GEMINI_SANDBOX=false`: agy folds both "auto-approve tools" and "no sandbox gate" into the single flag, and runs the prompt non-interactively via `-p` — which takes the prompt as its positional argument (`agy … -p "$LOCAL_PROMPT"`), **not** from stdin. Piping into `agy -p` (e.g. `echo … | agy -p`) fails with `agy --print takes the prompt as an argument, not stdin` and wastes an invocation; always pass the quoted prompt as the argument. `--print-timeout 30m` raises the print-mode wait above agy's 5-minute default so a real multi-file review isn't cut off, and — since stock macOS has no `timeout`/`gtimeout` and `TIMEOUT_CMD` is empty — is the effective bound on the invocation; it bounds the wait for the next response chunk, not the total runtime, so an actively-streaming review is never truncated. Unlike the old gemini invocation, no `env VAR=…` prefix is needed, so it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper at step 2 of the loop when one is present. +- `codex -c model_reasoning_effort=` — codex's **only** reasoning-effort control, and the reason the carrier table exists. codex has no `--effort` flag anywhere (not top-level, not on `review`, not on `exec`): passing one exits 2 with `error: unexpected argument '--effort' found` *before* any review runs, so the loop gets a launch failure where it expected a verdict. `-c key=value` is codex's config override, and `model_reasoning_effort` is the key. Pass it once and never pair it with a `--effort` "for good measure" — the pairing is the failure, not a safety net. +- `agy --dangerously-skip-permissions --model "$AGY_REVIEW_MODEL" --print-timeout 30m` — `--dangerously-skip-permissions` auto-approves all tool permission requests so the Antigravity CLI runs unattended (the headless equivalent of confirming every prompt). **Never pass `--effort` to agy**: agy treats it as mutually exclusive with `--model`, and this invocation always sets `--model`, so it is an unconditional launch failure rather than a degraded review — `--effort is not supported for model "…"`, at every level, including levels agy itself offers. Effort rides in the model name instead (carrier table above). `--model "$AGY_REVIEW_MODEL"` pins the reviewing model (resolved in pre-flight, default `Gemini 3.5 Flash (High)`, override via `AGY_REVIEW_MODEL`): without it agy picks its own default, which may be a heavy "Thinking" tier that spends many minutes in hidden reasoning and — depending on the model, emits little or no visible output meanwhile — makes a review look hung for 20-30 minutes; a fast capable model returns in well under a minute on a small diff. This is the agy successor to the Gemini CLI's `gemini --yolo` + `env GEMINI_SANDBOX=false`: agy folds both "auto-approve tools" and "no sandbox gate" into the single flag, and runs the prompt non-interactively via `-p` — which takes the prompt as its positional argument (`agy … -p "$LOCAL_PROMPT"`), **not** from stdin. Piping into `agy -p` (e.g. `echo … | agy -p`) fails with `agy --print takes the prompt as an argument, not stdin` and wastes an invocation; always pass the quoted prompt as the argument. `--print-timeout 30m` raises the print-mode wait above agy's 5-minute default so a real multi-file review isn't cut off, and — since stock macOS has no `timeout`/`gtimeout` and `TIMEOUT_CMD` is empty — is the effective bound on the invocation; it bounds the wait for the next response chunk, not the total runtime, so an actively-streaming review is never truncated. Unlike the old gemini invocation, no `env VAR=…` prefix is needed, so it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper at step 2 of the loop when one is present. - `grok --permission-mode bypassPermissions ${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"} -p` — `-p`/`--single` runs a single-turn headless prompt, prints the response to stdout, and exits (the grok analog of `claude -p` / `agy -p`). `--permission-mode bypassPermissions` auto-approves every tool execution so grok runs unattended (grok's equivalent of `--dangerously-skip-permissions`); it folds "auto-approve tools" into one flag, so no separate sandbox/`env VAR=…` prefix is needed and it composes cleanly with the `${TIMEOUT_CMD[@]+"${TIMEOUT_CMD[@]}"} {INVOCATION}` wrapper. `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` pins the reviewing model for a `grok[]` bracket (empty array → grok's own default; grok accepts the long `--model` form alongside `-p`). Like `agy -p`, `grok -p` takes the prompt as its positional argument — **not** from stdin (`grok … -p "$LOCAL_PROMPT"`); do not pipe into it. Grok has no `--print-timeout` equivalent, so the run is bounded by `TIMEOUT_CMD` (when present) and grok's own internal limits — the same background-launch + poll in Step 2 keeps it off the host's ~10-minute foreground cap. - `"$REVIEW_BIN" -p --trust …` (cursor) — `-p`/`--print` is Cursor Agent's headless mode (the analog of `claude -p` / `agy -p` / `grok -p`). `{REVIEW_BIN}` is `cursor-agent` when that name is on `$PATH`, else a probed `agent` that identified as Cursor — never a bare `agent` that is actually Grok (see the Cursor binary probe). `--trust` is required for headless runs in an untrusted workspace (Cursor fails those with guidance unless `--trust` or `--force` is passed). **Review-only** adds `--mode=ask` (Cursor's read-only exploration mode) and **omits `--force`**, so print mode only proposes changes — the same contract as the prompt, plus a mode flag that actually refuses writes. **Reviewer-applies** adds `--force` (alias `--yolo`) and `--sandbox disabled` so the agent can write fixes, run build/tests, and reach the network unattended. `--output-format text` is the default but is passed explicitly so stdout is a clean verdict document for Step 3. `${MODEL_FLAG[@]+"${MODEL_FLAG[@]}"}` pins a `cursor[]` bracket or a saved `review-models` default (empty array → Cursor's own default). A `~effort=` is folded into that same `--model` value as `[effort=]` (see the pre-flight block) — **do not pass `--effort`**, Cursor has no such flag and would exit non-zero. So `cursor[gpt-5]~effort=max` and `/do:config --review-models cursor=gpt-5` plus `--review-with cursor~effort=max` both become `--model gpt-5[effort=max]`. A model string that already encodes effort (`cursor[claude-opus-4-7[thinking=true,effort=high]]`, or a saved `review-models` value that already has `effort=`) is passed through unchanged. Effort with no model is prompt-advisory only. Like the other `-p` CLIs, the prompt is a positional argument (`… -p "$LOCAL_PROMPT"`), not stdin. Cursor has no `--print-timeout` equivalent, so the run is bounded by `TIMEOUT_CMD` (when present) and Cursor's own limits — the same background-launch + poll in Step 2 keeps it off the host's ~10-minute foreground cap. diff --git a/test/review-loop-contract.test.js b/test/review-loop-contract.test.js index 71332c1..bda14e9 100644 --- a/test/review-loop-contract.test.js +++ b/test/review-loop-contract.test.js @@ -5,8 +5,14 @@ const assert = require('node:assert/strict'); const fs = require('fs'); const path = require('path'); -const readLib = (name) => fs.readFileSync(path.join(__dirname, '..', 'lib', name), 'utf8'); -const readCommand = (name) => fs.readFileSync(path.join(__dirname, '..', 'commands', 'do', name), 'utf8'); +const _readCache = new Map(); +const _read = (...parts) => { + const f = path.join(__dirname, "..", ...parts); + if (!_readCache.has(f)) _readCache.set(f, fs.readFileSync(f, "utf8")); + return _readCache.get(f); +}; +const readLib = (name) => _read("lib", name); +const readCommand = (name) => _read("commands", "do", name); // The loop partials whose invocations carry arrays that can legitimately be empty // (TIMEOUT_CMD when no timeout/gtimeout is installed, MODEL_FLAG when no model is @@ -132,6 +138,60 @@ describe('review-loop parse contracts', () => { assert.match(ollama, /OLLAMA_EFFORT/); assert.match(ollama, /PROMPT="\$PROMPT Target reasoning effort level: \$OLLAMA_EFFORT\."/); }); + it('builds each reviewer a carrier its CLI actually accepts, defaulting to none', () => { + // `--effort` is correct for only claude/grok. Passing it to a CLI that + // rejects it is a non-zero exit BEFORE the review runs, so that reviewer's + // merge-gate slot holds a launch failure rather than a verdict: + // codex-cli 0.149.1: no --effort at any level (top-level, `review`, `exec`) + // -> error: unexpected argument '--effort' found + // agy 1.1.22: --effort is mutually exclusive with --model, which this loop + // always pins -> --effort is not supported for model "..." + // The pre-flight therefore dispatches per agent and defaults to NO flag; an + // agent nobody wrote an arm for must degrade to prompt-advisory effort, not + // inherit `--effort`. That inheritance is what broke codex and agy. + const loop = readLib('local-agent-review-loop.md'); + const preflight = loop.slice( + loop.indexOf('# Reasoning effort carrier.'), + loop.indexOf('# agy only: pin the review model'), + ); + assert.ok(preflight, 'the effort-carrier pre-flight block must exist'); + + // Per-agent carrier, asserted as a table so a new reviewer adds a row. + const CARRIERS = [ + ['claude|grok', /claude\|grok\) EFFORT_FLAG=\(--effort "\$REVIEW_EFFORT"\)/], + ['codex', /codex\)\s+EFFORT_FLAG=\(-c "model_reasoning_effort=\$REVIEW_EFFORT"\)/], + ['cursor', /CURSOR_MODEL="\$\{REVIEW_MODEL\}\[effort=\$\{REVIEW_EFFORT\}\]"/], + ['agy', /agy\) : ;;/], + ]; + for (const [agent, re] of CARRIERS) { + assert.match(preflight, re, `${agent} must get the carrier its CLI accepts`); + } + + // Fail closed: the default is no flag, and the unknown-agent arm guesses nothing. + assert.match(preflight, /^EFFORT_FLAG=\(\)$/m); + assert.match(preflight, /\*\)\s+: ;;/, 'unknown agents must not inherit a flag'); + assert.ok( + !/^\[ -n "\$REVIEW_EFFORT" \] && EFFORT_FLAG=\(--effort/m.test(preflight), + 'no unconditional --effort assignment may precede the per-agent dispatch', + ); + + // No invocation may pass a carrier its CLI rejects. + for (const agent of ['codex', 'agy', 'cursor']) { + const row = loop.split('\n').find((l) => l.startsWith(`| \`${agent}\` |`)); + assert.ok(row, `${agent} invocation row must exist`); + assert.ok( + agent === 'codex' || !row.includes('EFFORT_FLAG'), + `the ${agent} invocation must not pass EFFORT_FLAG`, + ); + } + + // The carrier table is the documented rule, and agy's variant is discovered + // at run time rather than baked into a level table that would go stale. + assert.match(loop, /\*\*Effort carriers\.\*\*/); + assert.match(loop, /\| `agy` \| a model \*\*variant\*\* picked from `agy models`/); + assert.match(loop, /not from a remembered table/); + assert.match(loop, /AGY_MODEL_RESOLVED/, 'the agy choice must persist across loop iterations'); + }); it('tells the in-process claude reviewer what to do with ~effort, and what not to reach for', () => { // The Agent tool takes a model but no reasoning effort, so a dispatching agent @@ -432,12 +492,11 @@ describe('review-loop parse contracts', () => { assert.match(loop, /Grok Build also installs an `agent` binary/); assert.match(loop, /--mode=ask/); assert.match(loop, /--force --trust/); - assert.match(loop, /Do not pass `EFFORT_FLAG` to `cursor`/); + assert.match(loop, /\| `cursor` \| folded into `--model` as `\[effort=\]`/); // ~effort must actually change Cursor inference: fold into --model as // [effort=], matching cursor[gpt-5]~effort=max and a saved // review-models cursor=gpt-5 plus cursor~effort=max. Never pass --effort. assert.match(loop, /CURSOR_MODEL="\$\{REVIEW_MODEL\}\[effort=\$\{REVIEW_EFFORT\}\]"/); - assert.match(loop, /folds `\{REVIEW_EFFORT\}` into the `--model` value/); assert.match(loop, /gpt-5\[effort=max\]/); // Review-only must not grant --force; reviewer-applies must. assert.match(loop, /omits `--force`/); @@ -446,9 +505,8 @@ describe('review-loop parse contracts', () => { // other reviewers — a saved review-models entry and a ~effort suffix. assert.match(readCommand('config.md'), /--review-models codex=o3,claude=claude-opus-4-8,cursor=gpt-5/); assert.match(readCommand('config.md'), /cursor\[gpt-5\]~effort=max/); - const readme = fs.readFileSync(path.join(__dirname, '..', 'README.md'), 'utf8'); - assert.match(readme, /cursor\[gpt-5\]~effort=max/); - assert.match(readme, /--review-models cursor=/); + assert.match(_read('README.md'), /cursor\[gpt-5\]~effort=max/); + assert.match(_read("README.md"), /--review-models cursor=/); assert.match(wrapper, /`cursor` \(alias `cursor-agent`\)/); assert.match(wrapper, /`codex` \| `agy` \| `claude` \| `grok` \| `cursor`/);