From 92711b72a4fe8d48abf0c0c48869b04d47518e51 Mon Sep 17 00:00:00 2001 From: "melodic-automation-lanes[bot]" <337595936+melodic-automation-lanes[bot]@users.noreply.github.com> Date: Tue, 3 Nov 2026 03:19:16 +0000 Subject: [PATCH] Resolve issue #12504 Lane-Activity: work-resolve-issue/resolve-issue --- ...ssue-issue-12504-41563977457-1-35444ae0.md | 7 +++++ plugins/review/.claude-plugin/plugin.json | 5 +-- plugins/review/README.md | 5 +-- plugins/review/reference/config.md | 2 +- plugins/review/schemas/review.schema.json | 4 +-- plugins/review/skills/fanout/SKILL.md | 8 +++-- .../fanout/context/run-everything-mode.md | 5 +-- plugins/review/skills/fanout/evals/evals.json | 31 ++++++++++++++++++- plugins/review/skills/setup/SKILL.md | 3 +- .../skills/setup/scripts/setup-apply.test.sh | 9 ++++-- 10 files changed, 63 insertions(+), 16 deletions(-) create mode 100644 .changes/review/work-resolve-issue-issue-12504-41563977457-1-35444ae0.md diff --git a/.changes/review/work-resolve-issue-issue-12504-41563977457-1-35444ae0.md b/.changes/review/work-resolve-issue-issue-12504-41563977457-1-35444ae0.md new file mode 100644 index 00000000000..5cd45a6c70e --- /dev/null +++ b/.changes/review/work-resolve-issue-issue-12504-41563977457-1-35444ae0.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- `fanout_diversity` takes a third value, `codex`. Under it, `/review:fanout run-everything` runs the `lens` roster and uses the Codex reviewer, through `skills/fanout/scripts/codex-review.sh`, as the model-diversity panel in place of `model` mode's same-vendor model tiers. The surfaces it runs are the ones `lens` runs; it adds a `panel:` line, `panel: codex ran`, or `panel: codex skipped ()` when Codex is not installed, enabled or logged in, and the lens sweep still runs. The default stays `lens`. Set `codex` through the plugin option or `docs/conventions/review.yaml` (#12504). diff --git a/plugins/review/.claude-plugin/plugin.json b/plugins/review/.claude-plugin/plugin.json index 3e1fd7c5467..4fd61a4f4e8 100644 --- a/plugins/review/.claude-plugin/plugin.json +++ b/plugins/review/.claude-plugin/plugin.json @@ -43,11 +43,12 @@ "fanout_diversity": { "type": "string", "title": "Fanout diversity", - "description": "lens (default): /review:fanout run-everything runs one reviewer agent per concern. model: it sends one review prompt to each distinct fan-out model tier instead. A repository's docs/conventions/review.yaml fanout_diversity wins over this option.", + "description": "lens (default): /review:fanout run-everything runs one reviewer agent per concern. model: one review prompt per fan-out model tier. codex: the lens roster plus the Codex reviewer as the model panel. A repository's docs/conventions/review.yaml fanout_diversity wins over this option.", "default": "lens", "options": [ "lens", - "model" + "model", + "codex" ] } } diff --git a/plugins/review/README.md b/plugins/review/README.md index 47796c37561..9f1a45f4859 100644 --- a/plugins/review/README.md +++ b/plugins/review/README.md @@ -201,7 +201,8 @@ Two settings, each set per user through the option below or per repository in for its safety fact (`run`) or only states it (`report`). It is a policy floor: `report` in either layer wins, and the repository value is read from the default branch. `fanout_diversity` decides whether `/review:fanout run-everything` runs one reviewer agent per concern (`lens`, the default) -or sends one review prompt to each model tier the role map names (`model`). Keys, values and resolution: +or sends one review prompt to each model tier the role map names (`model`), or runs the lens roster +with the Codex reviewer as its model-diversity panel (`codex`). Keys, values and resolution: [`reference/config.md`](reference/config.md). @@ -216,7 +217,7 @@ reads it from. | --- | --- | --- | --- | --- | | `connected_page_guard_enabled` | boolean | `true` | `CLAUDE_PLUGIN_OPTION_CONNECTED_PAGE_GUARD_ENABLED` | Runs the PreToolUse hook that denies an Artifact publish of an HTML page whose content security policy names a loopback connect-src. On by default. Turn off to allow publishing such a page. | | `downstream_probe` | string | `"run"` | `CLAUDE_PLUGIN_OPTION_DOWNSTREAM_PROBE` | run (default): /review:quality-gate downstream mode writes one probe for its safety fact in a temp directory outside the tree and runs it. report: it shows the probe without running it. report here or in a repository's docs/conventions/review.yaml (default branch) wins over run. | -| `fanout_diversity` | string | `"lens"` | `CLAUDE_PLUGIN_OPTION_FANOUT_DIVERSITY` | lens (default): /review:fanout run-everything runs one reviewer agent per concern. model: it sends one review prompt to each distinct fan-out model tier instead. A repository's docs/conventions/review.yaml fanout_diversity wins over this option. | +| `fanout_diversity` | string | `"lens"` | `CLAUDE_PLUGIN_OPTION_FANOUT_DIVERSITY` | lens (default): /review:fanout run-everything runs one reviewer agent per concern. model: one review prompt per fan-out model tier. codex: the lens roster plus the Codex reviewer as the model panel. A repository's docs/conventions/review.yaml fanout_diversity wins over this option. | ### How to set these diff --git a/plugins/review/reference/config.md b/plugins/review/reference/config.md index db064d10ec3..9405328b0f9 100644 --- a/plugins/review/reference/config.md +++ b/plugins/review/reference/config.md @@ -9,7 +9,7 @@ file or local overlay sets these keys. `/review:setup apply` writes the reposito | Key | Values | Default | Reader | |---|---|---|---| | `downstream_probe` | `run`, `report` | `run` | `/review:quality-gate` downstream mode (Step 2): `run` writes the safety-fact probe in a temporary directory outside the tree and runs it; `report` states the probe without running it. Level rule: policy floor, `report` in any layer wins; the repository value is read from the default branch | -| `fanout_diversity` | `lens`, `model` | `lens` | `/review:fanout` run-everything step 5 and its fallback: `lens` runs one reviewer agent per concern; `model` sends one review prompt to each distinct fan-out model tier of the role map. Level rule: later layer; the repository layer is skipped when the reviewed change set edits `docs/conventions/review.yaml` | +| `fanout_diversity` | `lens`, `model`, `codex` | `lens` | `/review:fanout` run-everything steps 3 and 5 and its fallback: `lens` runs one reviewer agent per concern; `model` sends one review prompt to each distinct fan-out model tier of the role map; `codex` runs the surfaces `lens` runs, with no model tiers, and reports the Codex reviewer as the model-diversity panel in a `panel:` line, `skipped` when Codex is absent. Level rule: later layer; the repository layer is skipped when the reviewed change set edits `docs/conventions/review.yaml` | ## Resolution diff --git a/plugins/review/schemas/review.schema.json b/plugins/review/schemas/review.schema.json index b944681999b..b79c07cded4 100644 --- a/plugins/review/schemas/review.schema.json +++ b/plugins/review/schemas/review.schema.json @@ -22,8 +22,8 @@ "default": "run" }, "fanout_diversity": { - "description": "How /review:fanout run-everything spreads its leaf reviews. lens: one reviewer agent per concern (security, architecture, code, docs, each project criteria slice). model: the same review prompt sent once to each distinct fan-out model tier the role map resolves. Skipped for a change set that edits this file.", - "enum": ["lens", "model"], + "description": "How /review:fanout run-everything spreads its leaf reviews. lens: one reviewer agent per concern (security, architecture, code, docs, each project criteria slice). model: the same review prompt sent once to each distinct fan-out model tier the role map resolves. codex: the lens roster, with the Codex reviewer as the model-diversity panel in place of the model tiers. Skipped for a change set that edits this file.", + "enum": ["lens", "model", "codex"], "default": "lens" } } diff --git a/plugins/review/skills/fanout/SKILL.md b/plugins/review/skills/fanout/SKILL.md index 1a9694ebbbe..dcbc94db44d 100644 --- a/plugins/review/skills/fanout/SKILL.md +++ b/plugins/review/skills/fanout/SKILL.md @@ -52,9 +52,11 @@ A positional mode token (Step 0) plus one flag: `--yes` applies ONLY to the tree-mutating `fix` action; it is inert for the report-only review modes, which mutate nothing to gate. Its role for `fix` is the explicit headless opt-in: in a non-interactive session, `fix` applies only under this flag, without it the fix action emits its classification plan and STOPs ([context/fix-pass-mode.md](context/fix-pass-mode.md) "Step 3: Plan + confirmation gate"). **`fanout_diversity` (run-everything and its fallback only).** It sets how the leaf fan-out -spreads: `lens` runs one reviewer agent per concern, and `model` sends one review prompt to each -distinct fan-out model tier ([context/run-everything-mode.md](context/run-everything-mode.md) -step 5). The default mode does not read it. Resolve it before step 5, lowest layer first: the +spreads: `lens` runs one reviewer agent per concern, `model` sends one review prompt to each +distinct fan-out model tier, and `codex` runs the `lens` roster with the Codex reviewer as the +model-diversity panel in place of those tiers +([context/run-everything-mode.md](context/run-everything-mode.md) steps 3 and 5). The default +mode does not read it. Resolve it before step 5, lowest layer first: the default `lens`; the user option `${user_config.fanout_diversity}` (a literal, unexpanded placeholder means unset); the `PASS fanout_diversity:` line of `node "${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/setup-apply.mjs" --check`, run from the project diff --git a/plugins/review/skills/fanout/context/run-everything-mode.md b/plugins/review/skills/fanout/context/run-everything-mode.md index 10d434e1ac7..3a1c01db097 100644 --- a/plugins/review/skills/fanout/context/run-everything-mode.md +++ b/plugins/review/skills/fanout/context/run-everything-mode.md @@ -13,7 +13,8 @@ Trigger: `$ARGUMENTS` is `run-everything` / `everything` / `all`. Distinct from 5. **Leaf fan-out.** If the gate failed, take the coverage-parity fallback. Otherwise: - **Roles.** When `/multi-agent:route` resolves in this session, invoke it as `/multi-agent:route all session=` and keep the `roles` object of the JSON it prints. When it does not resolve, omit `args.roles`, and say once in the report that enabling the multi-agent plugin makes this routing configurable; the workflow's built-in fallbacks then apply. When the reviewed change set touches the team layer (`docs/conventions/multi-agent.md` or `.claude/multi-agent.yaml`), also omit `args.roles` and say so in `## Surfaces`: the change under review must not choose the models that review it. - **Pilot.** The workflow launches its whole roster in one call, so when `slices` is non-empty, pilot outside it (SKILL.md "Pilot a wide fan-out"): dispatch one slice alone on the main thread as `review:brief-reviewer`, with the slice prompt the workflow would send, and confirm usage remains. Then launch with that slice removed from `slices`, add its output to the orchestrator outputs for step 6's Stage 0, and list it in the `## Surfaces` `Ran` entry. - - **Launch** `Workflow({ name: "review:fanout-sweep", args: { diffBase, slices, roles, maxConcurrent, diversity, session, findingsPaths } })`: `findingsPaths` maps `code-reviewer` and `security-reviewer` to the findings paths resolved per [`reviewer-handback.md`](../../../reference/reviewer-handback.md) (omitted, those leaves reply by value), `diffBase` is the step-2 base, `slices` the step-4 slice names, `roles` the map above, `maxConcurrent` an optional integer wave size (clamped to 1-16, default 4), `diversity` the resolved `fanout_diversity` (SKILL.md "Arguments"), and `session` the same alias passed to `/multi-agent:route`, omitted when unknown. + - **Launch** `Workflow({ name: "review:fanout-sweep", args: { diffBase, slices, roles, maxConcurrent, diversity, session, findingsPaths } })`: `findingsPaths` maps `code-reviewer` and `security-reviewer` to the findings paths resolved per [`reviewer-handback.md`](../../../reference/reviewer-handback.md) (omitted, those leaves reply by value), `diffBase` is the step-2 base, `slices` the step-4 slice names, `roles` the map above, `maxConcurrent` an optional integer wave size (clamped to 1-16, default 4), `diversity` the resolved `fanout_diversity` (SKILL.md "Arguments"), except `lens` when it resolved `codex`, and `session` the same alias passed to `/multi-agent:route`, omitted when unknown. + - **`codex` diversity.** The panel is the Codex reviewer, not the model tiers: launch with `args.diversity` set to `lens`, since the workflow takes only `lens` or `model` and Codex runs on the main thread. Step 3's two `codex-review.sh` runs, plain and `--adversarial`, are the panel. The surfaces that run are the ones `lens` runs; what `codex` adds is the panel report. Report one line under the `fanout_diversity:` line: `panel: codex ran` when either run printed `codex: ran`, else `panel: codex skipped ()` or `panel: codex failed ()` from the first run's status line, `panel: codex failed (timed out)` when a run timed out or printed no status line, and `panel: codex skipped (permission denied)` when the Bash call was denied. A skipped or failed panel does not stop the sweep: the lens roster still runs, and the `## Surfaces` line names Codex as step 3 says. - **`model` diversity.** The workflow collects the distinct fan-out models in `roles` (orchestrator, worker, verifier, retrieval, first seen first), deduped by the model each resolves to: `inherit` is the `session` alias, and with no `session` it counts as frontier; `fable` and `best` are left out. With two or more it sends the one review prompt to each as `review:lane-verifier`, at the verifier fan-out effort, in place of the per-concern agents and slices. With fewer than two, or no `roles`, it runs `lens` and returns `diversity: "lens"`; report that in `## Surfaces`. When step 3's `codex` surface ran, list it in `## Surfaces` as one more model tier. - **Pointer**: for which model wins when a script's `agent()` call or an Agent dispatch names one and the agent definition says `inherit`, see [Choose a model](https://code.claude.com/docs/en/sub-agents#choose-a-model) and the model paragraph of [Cost](https://code.claude.com/docs/en/workflows#cost). - **As of**: 2026-10-11 @@ -40,7 +41,7 @@ If availability cannot be positively confirmed, fall back (fail-safe, not fail-o ## Coverage-parity fallback (Workflows unavailable) -Spawn the SAME roster on the main thread via parallel Agent-tool calls (the main thread CAN spawn agents), each by its agent type as the workflow names it (slices as `review:brief-reviewer`, Stages 0 and 3 as `review:stage-normalizer`, never a general-purpose subagent), using the same resolved review diff base, with the `code-reviewer` and `security-reviewer` leaves given the findings paths `findingsPaths` would carry and Stage 0 reading their reports from those files, then run Stages 0–4 main-thread. Under `model` diversity the roster is the workflow's tier list instead: one Agent dispatch of `review:lane-verifier` per tier with the same review prompt, passing the tier as the Agent `model` parameter (none for `inherit`; for the values that parameter takes, see [subagents: choose a model](https://code.claude.com/docs/en/sub-agents#choose-a-model), as of 2026-11-01, recheck when that section changes them), and the same fewer-than-two rule. Coverage and the findings contract are identical; what is lost: background execution, out-of-context intermediates, and resume caching. If the caller depends on a dropped property, STOP and surface it rather than silently downgrading. +Spawn the SAME roster on the main thread via parallel Agent-tool calls (the main thread CAN spawn agents), each by its agent type as the workflow names it (slices as `review:brief-reviewer`, Stages 0 and 3 as `review:stage-normalizer`, never a general-purpose subagent), using the same resolved review diff base, with the `code-reviewer` and `security-reviewer` leaves given the findings paths `findingsPaths` would carry and Stage 0 reading their reports from those files, then run Stages 0–4 main-thread. Under `model` diversity the roster is the workflow's tier list instead: one Agent dispatch of `review:lane-verifier` per tier with the same review prompt, passing the tier as the Agent `model` parameter (none for `inherit`; for the values that parameter takes, see [subagents: choose a model](https://code.claude.com/docs/en/sub-agents#choose-a-model), as of 2026-11-01, recheck when that section changes them), and the same fewer-than-two rule. Under `codex` diversity the roster is the lens roster, and the panel line is reported as step 5 says. Coverage and the findings contract are identical; what is lost: background execution, out-of-context intermediates, and resume caching. If the caller depends on a dropped property, STOP and surface it rather than silently downgrading. ## Degraded notice diff --git a/plugins/review/skills/fanout/evals/evals.json b/plugins/review/skills/fanout/evals/evals.json index e1f39ce739a..ec69d5ae8af 100644 --- a/plugins/review/skills/fanout/evals/evals.json +++ b/plugins/review/skills/fanout/evals/evals.json @@ -603,7 +603,7 @@ { "id": 48, "name": "fanout-diversity-invalid-repository-value-drops-to-default", - "prompt": "/review:fanout run-everything\n\n[Scenario: the Workflow tool is available; a reviewable branch of 140 changed lines under greenhouse/sensors/, none of them in docs/conventions/. The user option renders as `model`. The setup-apply --check run exits 1 and prints `PASS downstream_probe: (unset)` and a WARN line saying docs/conventions/review.yaml sets fanout_diversity=models, which is not one of lens, model.]", + "prompt": "/review:fanout run-everything\n\n[Scenario: the Workflow tool is available; a reviewable branch of 140 changed lines under greenhouse/sensors/, none of them in docs/conventions/. The user option renders as `model`. The setup-apply --check run exits 1 and prints `PASS downstream_probe: (unset)` and a WARN line saying docs/conventions/review.yaml sets fanout_diversity=models, which is not one of lens, model, codex.]", "expected_output": "Names the file docs/conventions/review.yaml, the key fanout_diversity and the value models, and drops the repository layer for that key. No valid layer above it remains, so the key takes its default: reports `fanout_diversity: lens (default)` and launches with args.diversity set to lens. It does not fall back to the user option's model, and the run continues.", "files": [], "narration": true, @@ -628,6 +628,35 @@ "Launches the review:fanout-sweep workflow with args.diversity \"lens\"", "Does NOT take model from the edited docs/conventions/review.yaml" ] + }, + { + "id": 50, + "name": "fanout-diversity-codex-panel-runs-codex-when-present", + "prompt": "/review:fanout run-everything\n\n[Scenario: the Workflow tool is available; a reviewable branch of 110 changed lines under orders/, none of them in docs/conventions/, review base origin/main. The skill text shows the user option as the literal placeholder ${user_config.fanout_diversity}. The setup-apply --check run prints `PASS downstream_probe: (unset)` and `PASS fanout_diversity: codex`. codex@openai-codex is installed and logged in, so `codex-review.sh --base origin/main` prints `codex: ran` and then one finding.]", + "expected_output": "Reports `fanout_diversity: codex (docs/conventions/review.yaml)`. Runs `codex-review.sh --base origin/main` and again with `--adversarial` on the main thread, never a /codex:* command, and reports `panel: codex ran`. Launches review:fanout-sweep with args.diversity set to lens, so the lens roster runs and no model-tier lane-verifier leaves are dispatched. The ## Surfaces Ran list names Codex, and Codex's finding enters normalization.", + "files": [], + "narration": true, + "expectations": [ + "Reports the line `fanout_diversity: codex (docs/conventions/review.yaml)`", + "Runs skills/fanout/scripts/codex-review.sh with --base origin/main, and again with --adversarial, rather than invoking /codex:review", + "Reports the line `panel: codex ran`", + "Launches the review:fanout-sweep workflow with args.diversity \"lens\", not \"model\" or \"codex\"", + "Lists Codex under the ## Surfaces Ran entry" + ] + }, + { + "id": 51, + "name": "fanout-diversity-codex-panel-reports-skipped-when-absent", + "prompt": "/review:fanout run-everything\n\n[Scenario: the Workflow tool is available; a reviewable branch of 80 changed lines under warehouse/, none of them in docs/conventions/, review base origin/main. The user option renders as `codex`. The setup-apply --check run prints `INFO docs/conventions/review.yaml: absent; every key resolves from userConfig or its default`. codex@openai-codex is not installed, so `codex-review.sh --base origin/main` prints only `codex: skipped (codex@openai-codex is not installed or not enabled)`.]", + "expected_output": "Reports `fanout_diversity: codex (user option)` and `panel: codex skipped (codex@openai-codex is not installed or not enabled)`. Does not list Codex under Ran; the ## Surfaces line ends with `codex: skipped (codex@openai-codex is not installed or not enabled)`. The sweep does not stop: it launches review:fanout-sweep with args.diversity set to lens and persists the lens roster's findings.", + "files": [], + "narration": true, + "expectations": [ + "Reports fanout_diversity as `codex` supplied by the user option layer", + "Reports the line `panel: codex skipped (codex@openai-codex is not installed or not enabled)`", + "Does NOT list Codex under the ## Surfaces Ran entry, and ends the ## Surfaces line with the codex: skipped status", + "Still launches the review:fanout-sweep workflow with args.diversity \"lens\" and persists the findings" + ] } ] } diff --git a/plugins/review/skills/setup/SKILL.md b/plugins/review/skills/setup/SKILL.md index c4f8bf58cbe..e70407d8d98 100644 --- a/plugins/review/skills/setup/SKILL.md +++ b/plugins/review/skills/setup/SKILL.md @@ -76,7 +76,8 @@ a regular file). `apply` is the fix. (`downstream_probe`: `run`, the default, unless the team wants `/review:quality-gate downstream` to state its probe without running it, in which case `report`; `fanout_diversity`: `lens`, the default, unless the team wants `/review:fanout run-everything` to run each review prompt on - several model tiers, in which case `model`). Never invent a key the schema + several model tiers, in which case `model`, or to use the Codex reviewer as its model-diversity + panel, in which case `codex`). Never invent a key the schema does not list. `downstream_probe` is a policy floor read from the default branch, so a value written here takes effect once it is merged there. 2. **Write.** One call with every value: diff --git a/plugins/review/skills/setup/scripts/setup-apply.test.sh b/plugins/review/skills/setup/scripts/setup-apply.test.sh index 32a0aa82442..776663a7fa8 100755 --- a/plugins/review/skills/setup/scripts/setup-apply.test.sh +++ b/plugins/review/skills/setup/scripts/setup-apply.test.sh @@ -1,7 +1,7 @@ #!/usr/bin/env bash # Tests for setup-apply.mjs: the narrow write of docs/conventions/review.yaml. # Each case builds its own repository root in a temp directory; expected values -# come from the schema's allowed values (run, report; lens, model) and the script's stated +# come from the schema's allowed values (run, report; lens, model, codex) and the script's stated # exit codes, not from the script's output. # test-scope: plugins/review/lib/parse-concern-value.sh plugins/review/lib/resolve-convention-home.sh set -uo pipefail @@ -623,13 +623,18 @@ out="$(run --root "$R24" --check --ref origin/main)" assert_eq "case 24: a committed bad value exits 1" "1" "$?" assert_contains "case 24: a committed bad value keeps the valid key" "$out" "PASS downstream_probe: report" -# --- Case 26: fanout_diversity takes lens or model (the schema's enum) --------- +# --- Case 26: fanout_diversity takes lens, model or codex (the schema's enum) -- R26="$(new_root c26)" run --root "$R26" fanout_diversity=model >/dev/null assert_eq "case 26: fanout_diversity=model exits 0" "0" "$?" assert_eq "case 26: the file holds fanout_diversity: model" "1" "$(grep -cx 'fanout_diversity: model' "$(yaml_of "$R26")")" out="$(run --root "$R26" --check)" assert_contains "case 26: --check reports fanout_diversity" "$out" "PASS fanout_diversity: model" +R26C="$(new_root c26c)" +run --root "$R26C" fanout_diversity=codex >/dev/null +assert_eq "case 26: fanout_diversity=codex exits 0" "0" "$?" +out="$(run --root "$R26C" --check)" +assert_contains "case 26: --check reports fanout_diversity codex" "$out" "PASS fanout_diversity: codex" R26B="$(new_root c26b)" printf 'downstream_probe: run\n' >"$(yaml_of "$R26B")" before="$(cat "$(yaml_of "$R26B")")"