diff --git a/.changes/planning/feat-plan-reviewer-risk-gated-140d7eb6.md b/.changes/planning/feat-plan-reviewer-risk-gated-140d7eb6.md new file mode 100644 index 00000000000..a39c54621bc --- /dev/null +++ b/.changes/planning/feat-plan-reviewer-risk-gated-140d7eb6.md @@ -0,0 +1,8 @@ +--- +bump: minor +--- + +### Changed + +- `/planning:plan` no longer dispatches the fresh-context `plan-reviewer` on LOW plans. Under the default `plan_stress_test: auto` it runs when the blast radius is MEDIUM or higher, a stress-test trigger matches, or the user asks for a review; a before/after eval on LOW plans measured a change of -0.03 (95% interval -0.08 to +0.03) with no case drop, and MEDIUM keeps the reviewer until it is measured; the blast radius is now assessed before that decision, and the Step 5 presentation states a skip and its reason (`Reviewer: skipped (blast radius LOW, no trigger)`). `plan_stress_test` `reviewer` and `both` and verification `strict` still always run it; `light` and `off` still skip it. Basis: Anthropic's [Prompting Claude Opus 5, "Task scope and over-verification"](https://platform.claude.com/docs/en/build-with-claude/prompt-engineering/prompting-claude-opus-5#task-scope-and-over-verification): explicit verification steps and "use a subagent to verify" cause over-verification, and removing them reduces wasted tokens with no loss in quality. +- New eval case `plan-low-risk-skips-reviewer`: a LOW blast-radius draft with default settings dispatches no plan-reviewer and reports the skip line. diff --git a/docs/contracts/verification-level/README.md b/docs/contracts/verification-level/README.md index ee90497de72..6792b3c9d03 100644 --- a/docs/contracts/verification-level/README.md +++ b/docs/contracts/verification-level/README.md @@ -81,7 +81,7 @@ A skill that is itself the independent check when invoked does not read the leve | `/discovery:research`, `/discovery:explore`, `/discovery:trace-intent` | the sibling verifier (parent contract, "The sibling verifier, stated once"); `strict` widens research's grading to every row and claim | | `/discipline:do-your-research`, `/discipline:recheck-against-upstream`, `/discipline:sweep-all` | the fresh-context confirmation of a clean audit on load-bearing work (shared method, step 2), the fresh-context re-derivation (step 3) and the fan-out tiers' dispatch; `strict` takes do-your-research's fan-out tier at `full` depth | | `/education:quiz-me` | the fresh-context answer-key pass | -| `/planning:plan` | the plan-reviewer dispatch in Step 3 | +| `/planning:plan` | the plan-reviewer dispatch in Step 3; `strict` dispatches it for every plan, which `plan_stress_test`'s `auto` otherwise skips on LOW plans | | `/session-flow:orchestrate`, `/session-flow:workflow`, `/session-flow:retro` | imperative 3's verifier in a primed session; the fresh-context reviewer in pre-PR step 2, without which Stage 10 is not done; retro's single-pass analysis and scoring under `retro_lenses: 1` | | `/harness-config:audit-automation-gaps` | the fresh-context refusal gate (2.3), whose verdicts `light` lists as `unverified`, and the Phase 4 step 6 reviewer | | `/harness-config:audit-instructions` | the Phase C verifiers (per lane, B2 and I33) | diff --git a/plugins/planning/README.md b/plugins/planning/README.md index 2b45f679870..fd5de3bb7e1 100644 --- a/plugins/planning/README.md +++ b/plugins/planning/README.md @@ -19,7 +19,7 @@ where artifacts land in the consuming repo. | `/planning:design` | Design space | Explores types, contracts, module boundaries, and package topology through collaborative discussion rounds, producing capability-matrix / type-inventory / design-threads / topology artifacts; its `handoff` action delegates to `/planning:design-handoff`. Its `data`, `integration`, and `system` artifacts are typed: each carries a `scope` + `dialect` label and a fenced block in the dialect the `diagram_dialect` convention key resolves to (mermaid `erDiagram` or DBML for data; mermaid `sequenceDiagram` plus an OpenAPI 3.1 sketch for integration; a LikeC4 or C4-PlantUML container view for system, and no C4 view at all when that key is unset). `library` and `module` emit no typed artifact. | | `/planning:design-handoff` | Design→plan gate | Gates a finished design for `/planning:plan`. The gate is a binary check that every `design-threads.md` thread is RESOLVED, directional, or TAGGED-DEFERRED. Then it packages the plan-ready summary and resume prompt, or FAILs and routes back to `/planning:design`. On PASS and FAIL alike it also emits an advisory six-dimension coverage table (what, how, where, who, when, why) that never contributes to the verdict; uncovered dimensions ride along in the handoff summary and the resume prompt. | | `/planning:devils-advocate` | Adversarial review | Stress-tests plans via assumption extraction, evidence checks, failure scenarios, and operational-gotcha sweeps. Every finding evidence-backed, never generic warnings. An `incumbent` mode turns the same lens on the status quo: an Alternatives Sweep that stress-tests keeping an incumbent tool/approach against alternatives (native > official > vetted ladder, coupling priced, KEEP / MIGRATE / RESEARCH verdict), exploring the incumbent first-hand in a fresh sub-agent. | -| `/planning:plan` | Implementation plan | Produces a structured plan (goal, approach, test strategy, blast radius, parallelism analysis, tagged unilateral decisions) with a fresh-context `plan-reviewer` stress-test, on by default and set per mode by `plan_stress_test`, and a user approval gate, persisted to PLAN.md. | +| `/planning:plan` | Implementation plan | Produces a structured plan (goal, approach, test strategy, blast radius, parallelism analysis, tagged unilateral decisions) with a fresh-context `plan-reviewer` stress-test, run by default on MEDIUM-or-higher plans and on request and set per mode by `plan_stress_test`, and a user approval gate, persisted to PLAN.md. | | `/planning:setup` | Configuration | `check` inspects the standards index read-only; `apply` interviews the consumer and bootstraps the standards index (idempotent; re-run to reconfigure). | The pipeline composes end-to-end. `wayfind` charts the fog upstream when an effort diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/case.yaml b/plugins/planning/evals/plan-low-risk-skips-reviewer/case.yaml new file mode 100644 index 00000000000..e47697e60bf --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/case.yaml @@ -0,0 +1,4 @@ +schema_version: "1.1" +name: plan-low-risk-skips-reviewer +context: + scaffold_script: scaffold.sh diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/graders/no-reviewer-dispatch.md b/plugins/planning/evals/plan-low-risk-skips-reviewer/graders/no-reviewer-dispatch.md new file mode 100644 index 00000000000..95f685c2c18 --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/graders/no-reviewer-dispatch.md @@ -0,0 +1,8 @@ +--- +type: tool_used +tool: Agent +input_match: 'plan-reviewer' +min: 0 +max: 0 +arm: both +--- diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/graders/writes-skip-line.md b/plugins/planning/evals/plan-low-risk-skips-reviewer/graders/writes-skip-line.md new file mode 100644 index 00000000000..c8249bece92 --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/graders/writes-skip-line.md @@ -0,0 +1,5 @@ +--- +type: regex +pattern: 'reviewer[*_`]*:[*_`\s]*skipped' +flags: i +--- diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/prompt.md b/plugins/planning/evals/plan-low-risk-skips-reviewer/prompt.md new file mode 100644 index 00000000000..c373de19815 --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/prompt.md @@ -0,0 +1,11 @@ +--- +description: "A LOW blast-radius, one-function draft with no stress-test trigger, in a repository with default settings, dispatches no plan-reviewer in Step 3; the reply that reports Step 3 carries the reviewer skip line" +tags: [plan, plan-reviewer, setting] +runs: 3 +max_turns: 60 +timeout_seconds: 1200 +allowed_tools: [Read, Glob, Grep, Skill, Bash, Edit, Write, Agent] +expected_outcome: "Resolves plan_stress_test to auto and verification to standard, assesses blast radius LOW with no trigger matching, dispatches no planning:plan-reviewer, and states Reviewer: skipped (blast radius LOW, no trigger) in the reply that reports Step 3" +--- + +/planning:plan Continue the draft at `.work/slugify-max-length/PLAN.md` from Step 3. The Brief and the design gate are settled; `.work/slugify-max-length/design/design-resolution.md` records the design early exit. Run Steps 3 and 3b as this skill and this repository's settings say, then report what each step did. Stop there: no Step 4 onward, and edit no code. diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/samples/no-reviewer-dispatch.json b/plugins/planning/evals/plan-low-risk-skips-reviewer/samples/no-reviewer-dispatch.json new file mode 100644 index 00000000000..27a08770083 --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/samples/no-reviewer-dispatch.json @@ -0,0 +1,14 @@ +{ + "pass": [ + { + "answer": [{"tool": "Read", "input": {"file_path": ".work/slugify-max-length/PLAN.md"}}], + "why": "no agent dispatched" + } + ], + "fail": [ + { + "answer": [{"tool": "Agent", "input": {"subagent_type": "planning:plan-reviewer", "description": "Review plan", "prompt": "Review .work/slugify-max-length/PLAN.md"}}], + "why": "dispatched the plan-reviewer" + } + ] +} diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/samples/writes-skip-line.json b/plugins/planning/evals/plan-low-risk-skips-reviewer/samples/writes-skip-line.json new file mode 100644 index 00000000000..a1d4467eaa1 --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/samples/writes-skip-line.json @@ -0,0 +1,26 @@ +{ + "pass": [ + { + "answer": "Blast radius: LOW\nReviewer: skipped (blast radius LOW, no trigger)\nStress-test needed: No", + "why": "the skip line beside the assessment" + }, + { + "answer": "- **Reviewer:** skipped (blast radius LOW, no trigger matched)", + "why": "bold label" + } + ], + "fail": [ + { + "answer": "", + "why": "empty reply" + }, + { + "answer": "Reviewer: ran (blast radius LOW). Dispatched planning:plan-reviewer; no plan gaps found.", + "why": "the reviewer ran" + }, + { + "answer": "verification: skipped (light)", + "why": "a different skip reason" + } + ] +} diff --git a/plugins/planning/evals/plan-low-risk-skips-reviewer/scaffold.sh b/plugins/planning/evals/plan-low-risk-skips-reviewer/scaffold.sh new file mode 100755 index 00000000000..7e9fb5ebd7d --- /dev/null +++ b/plugins/planning/evals/plan-low-risk-skips-reviewer/scaffold.sh @@ -0,0 +1,102 @@ +#!/usr/bin/env bash +# A sandbox repository with no planning settings (verification standard, plan_stress_test auto), +# pushed to a local bare origin, holding a small textkit package with a test, a feature branch that +# changes it, and a settled one-function draft plan with LOW blast radius and no stress-test trigger. +set -euo pipefail + +git init -q -b main +mkdir -p textkit tests +printf '# textkit\n\nSmall text helpers. slugify lives in textkit/slug.py.\n' >README.md +: >textkit/__init__.py +cat >textkit/slug.py <<'PY' +def slugify(text): + return "-".join(text.lower().split()) +PY +cat >tests/test_slug.py <<'PY' +import unittest + +from textkit.slug import slugify + + +class SlugifyTest(unittest.TestCase): + def test_lowercases_and_hyphenates(self): + self.assertEqual(slugify("Hello World"), "hello-world") + + +if __name__ == "__main__": + unittest.main() +PY +printf '.work/\n__pycache__/\n' >.gitignore +git add . +git -c user.name=eval -c user.email=eval@example.invalid commit -q -m "scaffold textkit" +mkdir -p .work/origin.git +git init -q --bare -b main .work/origin.git +git remote add origin "$PWD/.work/origin.git" +git push -q origin main +git checkout -q -b feat/strip-punctuation +cat >textkit/slug.py <<'PY' +import re + + +def slugify(text): + words = re.sub(r"[^\w\s-]", "", text).lower().split() + return "-".join(words) +PY +git -c user.name=eval -c user.email=eval@example.invalid commit -q -am "feat: strip punctuation in slugify" + +# A settled draft plan and a design early exit, so the run reaches Step 3 without open questions. +mkdir -p .work/slugify-max-length/design +cat >.work/slugify-max-length/PLAN.md <<'MD' +# slugify max_length + +## Brief + +### Problem +Slugs from long titles make URLs too long for the share links that embed them. + +### Goal +`slugify` takes an optional `max_length` and returns a slug no longer than it, cut on a word boundary. + +### Acceptance criteria +- AC1: `slugify(text)` with no `max_length` returns what it returns today. +- AC2: `slugify(text, max_length=n)` drops whole trailing words so the result is at most `n` + characters and never ends in a hyphen; every hyphen in the output counts as a word boundary. +- AC3: When the first word alone is longer than `n`, the result is that word cut to `n` characters. +- AC4: `max_length` below 1 raises `ValueError`. + +### Out-of-scope +- Unicode transliteration. + +## Plan + +### Behavior +- B1: no limit keeps today's output (AC1). Delivered by Phase 1. +- B2: a limit cuts whole words, or the first word, to fit (AC2, AC3). Delivered by Phase 1. +- B3: a limit below 1 is refused (AC4). Delivered by Phase 1. + +### Phase 1: Add max_length [STATUS] +**Satisfies:** AC1, AC2, AC3, AC4 +**Delivers:** B1, B2, B3 + +- In `textkit/slug.py`, add `max_length=None`; after joining, when it is set, cut the slug at the + last hyphen at or before `max_length`, or at `max_length` when there is none. +- Raise `ValueError` when `max_length` is below 1. +- Add one test per criterion to `tests/test_slug.py`. + +**Sanity Check:** `python -m unittest discover -s tests` exits 0. + +### Files affected +| File | Action | +|---|---| +| `textkit/slug.py` | modify | +| `tests/test_slug.py` | modify | +MD +# The phase heading takes its status tag here, outside the heredoc. +plan=.work/slugify-max-length/PLAN.md +text="$(cat "$plan")" +printf '%s\n' "${text//'[STATUS]'/[TODO]}" >"$plan" +cat >.work/slugify-max-length/design/design-resolution.md <<'MD' +outcome: early-exit +tier: C +reason: One optional parameter on one existing function; no new types. +MD diff --git a/plugins/planning/reference/config.md b/plugins/planning/reference/config.md index aac001d648c..51ca90316c7 100644 --- a/plugins/planning/reference/config.md +++ b/plugins/planning/reference/config.md @@ -166,8 +166,9 @@ layer. Its values: -- `auto`: the `plan-reviewer` agent always; `/planning:devils-advocate` in plan-review mode when - Step 3b's blast radius is MEDIUM or higher or a trigger matches. +- `auto`: the `plan-reviewer` agent and `/planning:devils-advocate` in plan-review mode when Step + 3b's blast radius is MEDIUM or higher or a trigger matches; the `plan-reviewer` agent also when the + user asks for a review. A LOW plan with no trigger runs neither. - `reviewer`: the `plan-reviewer` agent only, whatever the blast radius. - `devils-advocate`: `/planning:devils-advocate` only, whatever the blast radius. - `both`: both, whatever the blast radius. diff --git a/plugins/planning/schemas/planning.schema.json b/plugins/planning/schemas/planning.schema.json index e067cdc23df..4d23b764085 100644 --- a/plugins/planning/schemas/planning.schema.json +++ b/plugins/planning/schemas/planning.schema.json @@ -65,7 +65,7 @@ "default": 27 }, "plan_stress_test": { - "description": "Per mode of the draft, which stress-tests /planning:plan runs. auto: plan-reviewer always, /planning:devils-advocate when the blast-radius assessment calls for it. reviewer: plan-reviewer only. devils-advocate: /planning:devils-advocate only. both: both, whatever the blast radius. Only /planning:plan reads this key: a direct /planning:devils-advocate request ignores it and runs the mode its own arguments select.", + "description": "Per mode of the draft, which stress-tests /planning:plan runs. auto: plan-reviewer and /planning:devils-advocate when the blast radius is MEDIUM or higher or a stress-test trigger matches, plan-reviewer also when the user asks for a review; a LOW plan with no trigger runs neither. reviewer: plan-reviewer only. devils-advocate: /planning:devils-advocate only. both: both, whatever the blast radius. Only /planning:plan reads this key: a direct /planning:devils-advocate request ignores it and runs the mode its own arguments select.", "type": "object", "additionalProperties": false, "properties": { diff --git a/plugins/planning/skills/plan/SKILL.md b/plugins/planning/skills/plan/SKILL.md index 9d2ff7b9d70..ffa488d6c4e 100644 --- a/plugins/planning/skills/plan/SKILL.md +++ b/plugins/planning/skills/plan/SKILL.md @@ -38,7 +38,7 @@ Every implementation starts with a clear design, evidence, a test strategy, and ## Emit checklist -For multi-step planning (almost always steps 1-5), copy `${CLAUDE_PLUGIN_ROOT}/skills/plan/templates/checklist.md` to `//plan-checklist.md` (default `.work/`) by reading the template with the Read tool and writing the copy with the Write tool, never `cp`, `sed` or a shell redirect from a plugin file, and tick each `- [ ]` as its step completes. Step 3 (fresh-context plan stress-test) and Step 5 (present for approval) are non-negotiable ticks, except that `plan_stress_test`'s `devils-advocate` value moves Step 3's tick to Step 4. Stress-test before presenting, except where the Step 3 verification level (`light` or `off`) skips it. +For multi-step planning (almost always steps 1-5), copy `${CLAUDE_PLUGIN_ROOT}/skills/plan/templates/checklist.md` to `//plan-checklist.md` (default `.work/`) by reading the template with the Read tool and writing the copy with the Write tool, never `cp`, `sed` or a shell redirect from a plugin file, and tick each `- [ ]` as its step completes. Step 5 (present for approval) is a non-negotiable tick. Step 3 is ticked once the reviewer rule (Gates) is applied: the reviewer ran, or its skip and reason are recorded. **Skip when:** the one-sentence skip (Purpose) applied, so no plan is written. On a mid-flight `review` replan, append a dated scope-change note and revise the PLAN phases instead; do not spawn a second checklist file. @@ -63,7 +63,7 @@ These hold after a compaction re-attach. Later steps say how to carry them out. **Verification.** These gates sit near the top so they stay inside the part of this skill that is re-attached after auto-compaction; `tests/reattach-slice.test.sh` stands in for that part with the first 20,000 bytes. How much is re-attached: [Skill content lifecycle](https://code.claude.com/docs/en/skills#skill-content-lifecycle), as of 2026-11-10; recheck when that section changes how much of a skill's invocation is re-attached, or stops describing a per-skill re-attach. -- **Reviewer.** Before assessing blast radius, dispatch a fresh-context plan-reviewer sub-agent for any plan you present, unless `plan_stress_test` (Step 3) sets `devils-advocate` for the draft's mode or the `verification` level (Step 3) is `light` or `off`. The producing thread does not self-attack the plan inline. A change skipped under the one-sentence rule (Purpose) has no plan to review. +- **Reviewer.** Before presenting, dispatch a fresh-context plan-reviewer sub-agent only when the first matching rule says so: the `verification` level (Step 3) `light` or `off` skips it; `plan_stress_test` (Step 3) `devils-advocate` for the draft's mode skips it; `reviewer` or `both`, or the `verification` level `strict`, runs it; under `auto` it runs when the Step 3b blast radius is MEDIUM or higher, any stress-test trigger matches, or the user asks for a plan review or stress-test (a `/planning:plan review` re-plan included), and is skipped otherwise. A skip is stated, never filled by the producing thread self-attacking the plan inline. A change skipped under the one-sentence rule (Purpose) has no plan to review. - **Hard-to-reverse decisions escalate EARLY** regardless of confidence. Below-bar judgment calls go to an interview round before the plan locks. - **Agent teams.** Route a phase to an agent team only when the parallel-safe workers must message each other and agent teams are enabled; otherwise use sub-agent workers or sequential. Disjoint file ownership between teammates is mandatory; for why, see [agent teams: avoid file conflicts](https://code.claude.com/docs/en/agent-teams#avoid-file-conflicts) (as of 2026-11-01; recheck when that section changes how teammates share files). @@ -102,7 +102,7 @@ The plan is a proposal, not a commitment. The user approves, modifies, or reject 1. The structured plan (Step 2, updated by Steps 3-4 if applicable) 2. Blast-radius assessment (Step 3b) -3. Stress-test summary (Step 4, if run), or "Skipped: blast radius LOW, no triggers matched", or "Skipped: `plan_stress_test` is `reviewer`" +3. Reviewer line (Step 3): `Reviewer: ran ()` or `Reviewer: skipped ()`, e.g. `Reviewer: skipped (blast radius LOW, no trigger)`; then the stress-test summary (Step 4, if run), or "Skipped: blast radius LOW, no triggers matched", or "Skipped: `plan_stress_test` is `reviewer`" 4. **Execution shape** (Step 4.5): the four-dimension table, parallelism shape AND per-phase routing table, with each `multi-shape` phase's attempt constraints and selection rule. Skipped for single-phase plans 5. **Displaced answers and new external effects** ("Plan changes after the Brief"). One row per change: `Q` or `none`, what the user said, what the plan now proposes, the new external effect, and the source (`reviewer fix`, `research update`, or `stress-test finding`). Omit the block when empty. The approval request names every row as needing its own reply 6. **Decisions made (gate-passed)** (Step 4.6). TABLE per [context/tag-decisions.md](context/tag-decisions.md) "Presentation contract": `Decision | What it changes in the plan | Basis (evidence) | Source`, one row per gate-passed `[EXEC-SHAPE]` / `[FALLBACK]` tag and per deferred Brief question the plan resolved (Source `deferred Q`), written for a cold reader (no session shorthand). Below-bar decisions never appear here. An empty section ("no unilateral decisions. Every PLAN item traces to brief") is valid @@ -227,29 +227,30 @@ For the removal step, invoke `/code-tidying:audit-dead-code` through the Skill t ### Step 3: Plan Stress-Test -**Which stress-tests run (`plan_stress_test`).** Name the draft's mode (`plan`, `proposal` or `incumbent`), then resolve that mode's value per [`${CLAUDE_PLUGIN_ROOT}/reference/config.md`](${CLAUDE_PLUGIN_ROOT}/reference/config.md) "Convention-home keys", which defines the modes and values (no `userConfig` layer; the default is `auto`). Report one line, e.g. `plan_stress_test.incumbent: both (docs/conventions/planning.yaml)`. `auto`, `reviewer` and `both` run this step; `devils-advocate` skips it and goes to Step 3b. +**Which stress-tests run (`plan_stress_test`).** Name the draft's mode (`plan`, `proposal` or `incumbent`), then resolve that mode's value per [`${CLAUDE_PLUGIN_ROOT}/reference/config.md`](${CLAUDE_PLUGIN_ROOT}/reference/config.md) "Convention-home keys", which defines the modes and values (no `userConfig` layer; the default is `auto`). Report one line, e.g. `plan_stress_test.incumbent: both (docs/conventions/planning.yaml)`. `reviewer` and `both` run this step; `auto` runs it when the reviewer rule (Gates) says so; `devils-advocate` skips it and goes to Step 3b. -**The verification level** decides what happens inside this step; `plan_stress_test` still decides whether it runs. Resolve it with `bash "${CLAUDE_PLUGIN_ROOT}/lib/read-verification.sh" --plugin planning --skill plan --file docs/conventions/planning.yaml` and report its line ([levels](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/contracts/verification-level/README.md)). Under `light`, and `off`, which acts as `light` here, skip items 2 through 5, dispatch no reviewer and never stress-test the plan inline in its place: the reply that reports this step, and the Step 5 presentation, state `verification: skipped ()`, and the run goes to Step 3b, which no level governs. +**The verification level** decides what happens inside this step; the reviewer rule (Gates) decides whether it runs. Resolve it with `bash "${CLAUDE_PLUGIN_ROOT}/lib/read-verification.sh" --plugin planning --skill plan --file docs/conventions/planning.yaml` and report its line ([levels](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/contracts/verification-level/README.md)). Under `light`, and `off`, which acts as `light` here, skip items 2 through 5, dispatch no reviewer and never stress-test the plan inline in its place: the reply that reports this step, and the Step 5 presentation, state `verification: skipped ()`, and the run goes to Step 3b, which no level governs. -Apply the reviewer rule under Planning Process before blast radius or presentation. The producing main thread MUST NOT self-attack the plan inline: the model that just wrote the plan rubber-stamps it. Where the plan is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor **when one is enabled and set up** (e.g. the OpenAI Codex plugin, when its documented surface can take this artifact, invoked per its own docs), with the fresh-context plan-reviewer sub-agent as the stated fallback, never a route to a command that may not resolve (per `docs/plugin-philosophy.md` "Fresh-eyes checkpoints" in the marketplace repository). +**Whether the reviewer runs.** Apply the reviewer rule (Gates). Under `auto`, make the Step 3b blast-radius and trigger assessment first, before any dispatch; Step 3b then presents that assessment rather than redoing it, unless a fix in item 5 changed the plan's files or scope. When the rule skips the reviewer, skip items 2 through 5, and the reply that reports this step, and the Step 5 presentation, state `Reviewer: skipped ()`, e.g. `Reviewer: skipped (blast radius LOW, no trigger)`, and a skip the verification level causes states `verification: skipped ()` as above; the run goes to Step 3b. The producing main thread MUST NOT self-attack the plan inline: the model that just wrote the plan rubber-stamps it. Where the plan is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor **when one is enabled and set up** (e.g. the OpenAI Codex plugin, when its documented surface can take this artifact, invoked per its own docs), with the fresh-context plan-reviewer sub-agent as the stated fallback, never a route to a command that may not resolve (per `docs/plugin-philosophy.md` "Fresh-eyes checkpoints" in the marketplace repository). 1. Gather the plan draft + design artifacts (or `design-resolution.md`) + the Brief -2. **Surface the cost.** Tell the user, in one sentence, that a fresh-context plan review runs now whatever they answer: one bounded sub-agent (default `effort: medium`, capped turns). +2. **Surface the cost.** Tell the user, in one sentence, that a fresh-context plan review runs now and why (`Reviewer: ran ()`): one bounded sub-agent (default `effort: medium`, capped turns). 3. Dispatch the plugin's **`plan-reviewer`** agent (`agents/plan-reviewer.md`) with the prompt from [context/plan-reviewer.md](context/plan-reviewer.md). Do not substitute a generic read-only sub-agent, and pass no `effort` on the call: the agent definition carries the reviewer's bounded `effort` and `maxTurns` (the effort record in `agents/plan-reviewer.md`). A complete report ends with its `### Summary` counts, or is the literal `No plan gaps found.` for a clean pass; treat any other return as incomplete whether or not a partial marker is present (whether a `maxTurns` stop is marked depends on the client: the `maxTurns` record in `agents/plan-reviewer.md`): resume it (or re-dispatch with a narrowed brief); never treat a partial table as a clean pass. 4. **Verify reviewer findings** against the actual code/files before applying fixes. Sub-agent findings are synthesis, not ground truth 5. Fix every confirmed gap BEFORE proceeding; never present a plan with known gaps. A fix that displaces a user answer or adds an external effect follows "Plan changes after the Brief" below -Trivial single-file plans (3–5 bullets, no new types): the reviewer brief may be shortened to structural-integrity checks only; still dispatch fresh context, never inline self-critique. +When the reviewer runs on a trivial single-file plan (3–5 bullets, no new types), its brief may be shortened to structural-integrity checks only; still dispatch fresh context, never inline self-critique. ### Step 3b: Assess Blast Radius -Every plan gets a blast-radius check. Read [context/stress-test-triggers.md](context/stress-test-triggers.md) and assess whether this plan warrants a formal stress-test via `/planning:devils-advocate`. +Every plan gets a blast-radius check. Read [context/stress-test-triggers.md](context/stress-test-triggers.md) and assess whether this plan warrants a formal stress-test via `/planning:devils-advocate`; under `auto`, Step 3 already made this assessment, so present it, reassessing when a Step 3 fix changed the plan's files or scope. Present the assessment: ``` Blast radius: [LOW / MEDIUM / HIGH / CRITICAL] -Stress-test needed: [Yes — invoking /planning:devils-advocate / No — plan-reviewer sub-agent + research validation is sufficient] +Reviewer: [ran / skipped] () +Stress-test needed: [Yes — invoking /planning:devils-advocate / No — research validation is sufficient] Reason: <1-2 sentences> ``` @@ -262,7 +263,7 @@ Runs only when Step 3b sends the plan here; the deeper, formal version of Step 3 1. **Surface the cost.** Tell the user a formal `/planning:devils-advocate` run is starting in fresh context (another serial sub-agent, typically higher turn budget than Step 3) before dispatching. 2. **Dispatch `/planning:devils-advocate` via the Skill tool to a fresh-context sub-agent**. Under `plan_stress_test`'s `devils-advocate` or `both`, an `incumbent` draft runs the skill's `incumbent` mode, with `incumbent ` as its arguments; every other run is plan-review mode. Keep the brief short: findings table, not a narrative. Hand it the plan (plus the Brief, the interview ledger `//interview-checklist.md` when one exists, and any design artifacts), not your rationale for it. The producing main thread MUST NOT run the stress-test inline, for the same reason as Step 3. Verify its findings against the actual code/files before acting on them; sub-agent findings are synthesis, not ground truth - **Audit judgment (untested):** at MEDIUM or higher blast radius, Step 3 and Step 4 may dispatch against the same plan draft in one message when Step 4's criteria read off the draft rather than Step 3's findings. Document when used; default remains serial. + **Audit judgment (untested):** when both the reviewer and Step 4 run, Step 3 and Step 4 may dispatch against the same plan draft in one message when Step 4's criteria read off the draft rather than Step 3's findings. Document when used; default remains serial. 3. **Evaluate findings**. If `/planning:devils-advocate` produces CRITICAL or HIGH findings: - Run targeted research on the specific issues (invoke `/discovery:research` via the Skill tool if enabled, or the strongest research capability available) diff --git a/plugins/planning/skills/plan/context/stress-test-triggers.md b/plugins/planning/skills/plan/context/stress-test-triggers.md index d1df6f7601e..b985c74d783 100644 --- a/plugins/planning/skills/plan/context/stress-test-triggers.md +++ b/plugins/planning/skills/plan/context/stress-test-triggers.md @@ -1,6 +1,6 @@ # Stress-Test Trigger Criteria -When to invoke `/planning:devils-advocate` on a plan under `plan_stress_test`'s `auto`; the other values decide without these criteria (SKILL.md Step 3b). The goal is to catch plans that carry enough risk to warrant systematic adversarial review, without stress-testing every trivial change. +When to invoke `/planning:devils-advocate` on a plan under `plan_stress_test`'s `auto`, where a matching trigger, or a MEDIUM or higher blast radius, also runs the Step 3 `plan-reviewer`; the other values decide without these criteria (SKILL.md Step 3b). The goal is to catch plans that carry enough risk to warrant systematic adversarial review, without stress-testing every trivial change. ## Always stress-test when ANY of these match diff --git a/plugins/planning/skills/plan/evals/evals.json b/plugins/planning/skills/plan/evals/evals.json index 00771522b53..9c760eb320a 100644 --- a/plugins/planning/skills/plan/evals/evals.json +++ b/plugins/planning/skills/plan/evals/evals.json @@ -20,7 +20,7 @@ "id": 2, "name": "mandatory-fresh-context-stress-test", "prompt": "/planning:plan exploration and research are done, and I explicitly accept planning without a separate /design pass (no new types or modules). The task: add structured logging around every outbound HTTP call across the existing services — cross-cutting but mechanical. Formulate the plan and get it ready to present.", - "expected_output": "The design gate is satisfied by the explicit override, so planning proceeds. Before presenting the plan, the skill dispatches a fresh-context plan-reviewer sub-agent (Step 3) rather than self-critiquing the plan inline, and verifies the reviewer's findings against the actual code before applying fixes.", + "expected_output": "The design gate is satisfied by the explicit override, so planning proceeds. Cross-cutting logging matches a stress-test trigger, so before presenting the plan the skill dispatches a fresh-context plan-reviewer sub-agent (Step 3) rather than self-critiquing the plan inline, and verifies the reviewer's findings against the actual code before applying fixes.", "files": [], "expectations": [ "Output dispatches the plan-reviewer agent (or an equivalent fresh-context reviewer) rather than the authoring thread reviewing its own plan inline", diff --git a/plugins/planning/skills/plan/templates/checklist.md b/plugins/planning/skills/plan/templates/checklist.md index ee37f91bbe2..f6fb7e5864a 100644 --- a/plugins/planning/skills/plan/templates/checklist.md +++ b/plugins/planning/skills/plan/templates/checklist.md @@ -6,8 +6,8 @@ Copy into `//plan-checklist.md` (default `.work/`; the t - [ ] Step 1: Prerequisite check. Brief locked (PLAN.md Brief section exists OR equivalent crisp framing); exploration + research done or explicitly waived; design gate evaluated - [ ] Step 2: Formulate the plan. Phases with verifiable Sanity Checks per phase; estimate scope; identify parallelism -- [ ] Step 3: Plan stress-test. Resolve `plan_stress_test` for the draft's mode; unless it is `devils-advocate`, surface cost, then dispatch the plan-reviewer agent per context/plan-reviewer.md -- [ ] Step 3b: Assess blast radius (LOW / MEDIUM / HIGH / CRITICAL). Under `auto` this gates whether Step 4 runs +- [ ] Step 3: Plan stress-test. Resolve `plan_stress_test` for the draft's mode and apply the reviewer rule (SKILL.md Gates); when it runs the reviewer, surface cost, then dispatch the plan-reviewer agent per context/plan-reviewer.md; otherwise record `Reviewer: skipped ()` +- [ ] Step 3b: Assess blast radius (LOW / MEDIUM / HIGH / CRITICAL). Under `auto` it is assessed before Step 3's reviewer decision and gates both the reviewer and Step 4 - [ ] Step 4: Formal stress-test + research-iterate (CONDITIONAL on Step 3b). Invoke `/planning:devils-advocate` and targeted research on contested claims - [ ] Step 4.5: Execution-shape analysis (default ON for multi-phase plans). Emit scope-fencing tables + per-phase routing table - [ ] Step 4.6: Tag unilateral decisions. Flag any choice made without explicit user approval; interview below-bar decisions @@ -16,7 +16,7 @@ Copy into `//plan-checklist.md` (default `.work/`; the t ## Skip criteria -- Step 3 SKIPPED only under `plan_stress_test`'s `devils-advocate`, which runs Step 4 instead (stress-test before presenting) +- Step 3's reviewer SKIPPED under `plan_stress_test`'s `devils-advocate` (Step 4 runs instead), under verification `light` or `off`, and under `auto` when Step 3b's blast radius is LOW, no trigger matches, verification is not `strict` and the user did not ask for a review; tick Step 3 with the skip and its reason - Step 4 SKIPPED under `plan_stress_test`'s `reviewer`, and under `auto` when Step 3b verdict = LOW blast radius and no trigger matches - Step 4.5 SKIPPED when the plan is single-phase (no parallelism axis, all-main-session default) - Step 4.6 SKIPPED when no unilateral decisions were made (user approved every choice during Q&A)