Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugins/review-toolkit/skills/code-review-fanout/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,10 +63,11 @@ Write the ranked report to `<findings-location>/<UTC-timestamp>-<topic>.md` (`da

## Orchestrator plugins

Two optional orchestrator plugins from the `claude-plugins-official` marketplace add adversarial breadth. Both run on the MAIN THREAD (they fan out their own agents; a subagent cannot dependably do that). Each is a graceful enhancement, not a hard dependency:
Three optional orchestrator plugins add adversarial breadth — two same-vendor Claude plugins from the `claude-plugins-official` marketplace, plus the OpenAI Codex plugin (`codex@openai-codex`) as a different-model surface. All run on the MAIN THREAD (they fan out their own agents; a subagent cannot dependably do that). Each is a graceful enhancement, not a hard dependency:

- **`pr-review-toolkit`** — `/pr-review-toolkit:review-pr`: aspect-scoped agent fan-out. Absent → this plugin's leaf agents cover most of the same dimensions; note that orchestrator breadth was skipped.
- **`code-review`** — `/code-review:code-review`: parallel reviewers + confidence scorer for an existing PR. **PR-mutation gate:** its PR mode posts findings as a PR comment, which violates the review modes' report-only contract; when the branch has an open PR, dispatch it only on explicit user opt-in ("post the review comment"), otherwise skip it and name the skip in `## Surfaces`. Absent → note the skip; a repository's own CI review bot (when present) still provides PR coverage.
- **`codex`** (OpenAI Codex) — `/codex:review`: read-only cross-vendor review, so it satisfies the review modes' report-only contract with no PR-mutation gate; `/codex:adversarial-review`: red-teams the diff, fitting the intentional adversarial-breadth intent. The first surface backed by a **different model** — its blind spots are uncorrelated with the same-vendor leaf agents and Claude orchestrators, so a finding only Codex raises is signal the rest structurally cannot see. Invoke it with `--wait` so the review runs in the foreground and returns findings in the same turn (its default prompts and may run in a background task the synchronous normalization step would miss), and pass `--base <review-base>` carrying this skill's resolved review diff base ("Shared inputs") so Codex diffs the same change set as every other dispatched surface — without it Codex auto-picks the working tree or default branch. Absent → note the skip; the same-vendor surfaces still cover most dimensions.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Don't claim Codex covers dirty default reviews

Fresh evidence after the prior fix: the dirty-tree caveat was added only to run-everything-mode.md, but this shared SKILL.md bullet still governs default medium/large orchestrator dispatch and says --base makes Codex diff the same change set. In a default review of an ahead branch with staged/unstaged edits, the leaves use git diff <review-base> and include those edits, while I checked codex-plugin-cc's companion code and any baseRef resolves to branch mode, whose branch context diffs only <merge-base>..HEAD; the persisted report will therefore overstate Codex coverage unless the same gap is documented here or default mode runs a separate working-tree Codex pass.

Useful? React with 👍 / 👎.


## What this skill does NOT do

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@ Trigger: `$ARGUMENTS` is `run-everything` / `everything` / `all`. Distinct from
## Flow

1. **Pre-launch availability gate** (below) — run BEFORE any launch.
2. **Main-thread orchestrators** — sequentially invoke the optional orchestrator plugins per SKILL.md "Orchestrator plugins". They fan out their OWN agents from the main thread; a Workflow `agent()` is a subagent and cannot dependably spawn them.
3. **Resolve the roster** — run the discovery recipe in `leaf-roster.md` to get the slice list, and resolve the review diff base (SKILL.md "Shared inputs").
4. **Leaf fan-out** — if the gate passed, substitute the resolved diff base into `REVIEW_DIFF` and the discovered slice names into `OWNERLESS_SLICES` in the script below, then launch it via the Workflow tool. Else take the coverage-parity fallback.
5. **Normalize main-thread** — gather the Workflow's extracted leaf records + the raw orchestrator outputs; run Stage 0 on the orchestrator outputs (the Workflow only extracted the leaf branch), then Stages 1–4 of `findings-normalization.md` over the combined record set. Reconcile per surface against the Workflow's `raw` array: any surface whose raw output is non-empty but yielded zero extracted records gets Stage 0 re-run main-thread on that raw text; whatever still fails to parse goes verbatim into `## Unparsed` — partial extraction never silently drops a surface.
6. **Persist** per `default-mode.md` "Findings-writer contract"; prepend the DEGRADED block when the fallback was taken.

**Diffability pre-check (before step 4):** the untracked-only diagnostic from `default-mode.md` applies here too — with nothing diffable, every leaf would diff an empty tree and return nothing. Emit the diagnostic and skip the launch; do NOT stage files.
2. **Resolve the review diff base** (SKILL.md "Shared inputs") — resolve it up front, because the orchestrator step passes it to Codex via `--base` and the leaf fan-out substitutes it into `REVIEW_DIFF`; every surface must diff the same base.
3. **Main-thread orchestrators** — sequentially invoke the optional orchestrator plugins per SKILL.md "Orchestrator plugins". They fan out their OWN agents from the main thread; a Workflow `agent()` is a subagent and cannot dependably spawn them. This exhaustive sweep is where the cross-vendor `codex` surface earns its cost most — when the plugin is present, invoke `/codex:review --wait --base <review-base>` (and `/codex:adversarial-review --wait --base <review-base>` for red-team breadth), since a different model is the one source of uncorrelated blind spots the Claude leaves and orchestrators structurally share. Two flags are load-bearing: `--wait` keeps the review in the foreground (without a flag the command prompts or backgrounds, returning only a status handle, so the step-6 synchronous normalization would see an empty surface and silently drop Codex), and `--base` carries the step-2 review diff base so Codex diffs the SAME change set as every other surface — without it Codex auto-picks the working tree or default branch and reviews a different diff on any PR whose base is not the default branch. Coverage boundary: `--base` runs Codex in branch mode (`git diff <base>..HEAD`, committed only), which coincides with the leaves' `git diff <base>` on a clean branch (the review case) but NOT when the branch also carries uncommitted tracked edits — there Codex covers the committed diff while the leaves additionally cover the dirty tree. Name that gap in `## Surfaces` for a mixed branch+dirty run rather than assuming identical change sets.
4. **Resolve the roster** — run the discovery recipe in `leaf-roster.md` to get the slice list.
5. **Leaf fan-out** — if the gate passed, substitute the step-2 diff base into `REVIEW_DIFF` and the discovered slice names into `OWNERLESS_SLICES` in the script below, then launch it via the Workflow tool. Else take the coverage-parity fallback.
6. **Normalize main-thread** — gather the Workflow's extracted leaf records + the raw orchestrator outputs; run Stage 0 on the orchestrator outputs (the Workflow only extracted the leaf branch), then Stages 1–4 of `findings-normalization.md` over the combined record set. Reconcile per surface against the Workflow's `raw` array: any surface whose raw output is non-empty but yielded zero extracted records gets Stage 0 re-run main-thread on that raw text; whatever still fails to parse goes verbatim into `## Unparsed` — partial extraction never silently drops a surface.
7. **Persist** per `default-mode.md` "Findings-writer contract"; prepend the DEGRADED block when the fallback was taken.

**Diffability pre-check (before step 5):** the untracked-only diagnostic from `default-mode.md` applies here too — with nothing diffable, every leaf would diff an empty tree and return nothing. Emit the diagnostic and skip the launch; do NOT stage files.

## Pre-launch availability gate

Expand Down
Loading