Skip to content

feat(planning): run the plan reviewer only for risky plans - #15398

Merged
kyle-sexton merged 9 commits into
mainfrom
feat/plan-reviewer-risk-gated
Nov 11, 2026
Merged

kyle-sexton merged 9 commits into
mainfrom
feat/plan-reviewer-risk-gated

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Nov 11, 2026 •

Copy link
Copy Markdown
Member

No related issue: owner-confirmed decisions Q303 and Q307 (Wayfinder absorb).

Summary

/planning:plan dispatched a fresh-context plan-reviewer subagent on every plan it presented. Anthropic's current guidance says separate verification steps and "use a subagent to verify" cause over-verification, and removing them "reduces wasted tokens with no loss in quality" (Prompting Claude Opus 5, Task scope and over-verification). The reviewer now runs only for risky plans.

Fix

  • Reviewer rule (Gates bullet), first match wins: verification light/off skips; plan_stress_test devils-advocate skips; reviewer, both or verification strict runs; under auto it runs when the Step 3b blast radius is MEDIUM or higher, a stress-test trigger matches, or the user asks for a review (a /planning:plan review re-plan included). Otherwise it is skipped and the plan says Reviewer: skipped (<reason>); a skip is never replaced by inline self-review.
  • Under auto, Step 3 makes the blast-radius and trigger assessment first; Step 3b presents it. Step numbers are unchanged.
  • Checklist: Step 3 is no longer an unconditional tick. config.md, schema, README, triggers file and the verification-level contract row say the same.
  • New eval case plan-low-risk-skips-reviewer: a LOW plan with no config writes the skip line and dispatches no reviewer.

Verification

  • Planning tests: plan-stress-test 77/0, reattach-slice 25/0, standards-binding 9/0, plan-size-keys 17/0, interview-defenses 185/0.
  • Affected suites: check-matchers OK, check-summary-reader-parity 35/0, validate-plugin-contracts 232/0, check-data-root 18/0.
  • scripts/check-changed-skills.sh HEAD: pass (pre-existing size warnings). check-changelog-fragments.sh --check: valid. markdownlint: 0 issues. validate-cases.py plugins/planning/evals: no FAIL; the new case's graders pass against its samples offline.
  • Local verify passed (check-affected.sh --tests) with a real node first on PATH; the mise node shim fails under a fake HOME, which had failed 4 setup-apply.test.sh cases locally on main too.
  • Before/after eval (pre-registered and reported in this PR's comments; cases in the separate test/plan-reviewer-cut-evals PR): on LOW plans the change measured -0.03 (95% interval -0.08 to +0.03), with no case drop. Non-inferiority at 0.05 was not shown, and MEDIUM went unmeasured, so the owner waived the no-loss rule for LOW only and MEDIUM keeps the reviewer.

Related

Follow-ups

  • The cut applies to plans the model rates LOW, which is what the eval measured: MEDIUM-designed drafts the model rated LOW scored 1.00 without the reviewer. The eval used small sample repos, so measure how often real plans rate LOW after this merges before relying on the MEDIUM reviewer as a safety net.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ

Base automatically changed from feat/planning-fidelity-appetite to main November 11, 2026 19:26
Under plan_stress_test auto, /planning:plan dispatches the plan-reviewer
subagent only when the blast radius is HIGH or CRITICAL, a stress-test
trigger matches, or the user asks for a review; other plans skip it and
say so in a Reviewer: line. reviewer, both and verification strict still
force it; devils-advocate and verification light/off still skip it. A new
eval case checks that a low-risk plan skips the reviewer.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton
kyle-sexton force-pushed the feat/plan-reviewer-risk-gated branch from 1b51fab to 9129583 Compare November 11, 2026 19:28
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 11, 2026 19:33
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-11T19:35:58.045520Z 9129583 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 91295837c5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread plugins/planning/skills/plan/SKILL.md Resolved
@kyle-sexton kyle-sexton added the do-not-merge Hard merge gate: do not merge while applied. label Nov 11, 2026
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

claude-security-review reviewed cc4acc02ca985107674de979c3ef851453c5cfce: read 16 of 16 changed file(s); 0 finding(s), 0 as inline comments.

Security review: PR #15398, feat(planning): run the plan reviewer only for risky plans

No security issues found.

critical-findings: 0

Scope

I read every hunk of all 16 changed files in .claude-lane/pr.diff. No changed file matches a TRUSTED PATHS glob. docs/contracts/verification-level/README.md sits under docs/contracts/, which is not docs/conventions/ or docs/standards/. The diff touches no .claude-plugin/plugin.json or marketplace.json, so there are no manifest lines. It touches no workflow, action, token request, trusted-actor list, kill switch or merge-hold script, so no trust control is widened.

Instruction-surface lens

The diff softens a standing skill rule. In plugins/planning/skills/plan/SKILL.md, the Gates "Reviewer" bullet used to say: always dispatch the fresh-context plan-reviewer for any plan you present. Under plan_stress_test: auto it now skips the reviewer for LOW blast-radius plans with no stress-test trigger. I checked the removed rule against the six Gate 0 classes that docs/contracts/instruction-exception-register/README.md adopts: irreversible-action, secret-handling, data-integrity, external-publication, legal-compliance and agent-authority. It matches none of them, judged by what happens when the rule is gone:

  • The reviewer is a plan-quality check. It does not gate any action.
  • Controls that stay in force:
    • the Step 5 user approval gate;
    • "Hard-to-reverse decisions escalate EARLY regardless of confidence";
    • the stress-test triggers in context/stress-test-triggers.md, which cover irreversibility and infrastructure and now also force the reviewer under auto;
    • MEDIUM or higher blast radius, which still runs it;
    • verification strict, reviewer and both, which still always run it.
  • The rule against inline self-review is kept: a skip is stated, never replaced by inline self-attack.

No protected class loses enforcement.

Other files

  • plugins/planning/evals/plan-low-risk-skips-reviewer/scaffold.sh is an eval sandbox setup script. All its content is fixed: heredocs and printf with literal strings. It pushes to a local bare remote under .work/ and takes no outside or untrusted input, so there is nothing to inject into.
  • The schema, config.md, README, changelog fragment, checklist, evals.json and the verification-level contract row change wording only.

Settled threads and prior findings

  • The two settled threads are about plan quality and staleness, not security, and the current diff raises no security point on those lines.
  • This lane has no earlier findings on this PR, and blocking only: no is set.

PR text

I found no embedded text in the PR context or the settled threads telling the lane to pass the PR, skip a check, drop a finding or post somewhere else.

Spend

The change cuts prepaid model and subscription usage: on LOW, no-trigger plans, one bounded plan-reviewer sub-agent run is no longer spent. It adds no billing. Not a hold.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

claude-review: 1 inline finding(s) on 91295837c5ad698759b655fbdc3f629de03830ec; the summary is in this lane's pull request comment.

claude-lane: claude-review

Comment thread plugins/planning/skills/plan/SKILL.md Resolved
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

claude-review reviewed cc4acc02ca985107674de979c3ef851453c5cfce: read 16 of 16 changed file(s); 0 finding(s), 0 as inline comments.

claude-review (blocking only: this lane already reviewed an earlier head, so only blocking findings are reported here and advisory ones are left out).

No blocking findings at cc4acc02. I read every hunk of all 16 files in the diff.

What I checked:

  • The reviewer rule (Gates, plugins/planning/skills/plan/SKILL.md L66), where the first matching rule wins. Verification light/off skips the reviewer. devils-advocate skips it. reviewer/both/strict run it. Under auto it runs on MEDIUM or higher, on a trigger match, or when the user asks. Step 3 (L230-L234), Step 3b (L246-L257), the Step 5 reviewer line, reference/config.md, the schema description, the README row, context/stress-test-triggers.md, the checklist template and its skip criteria, and the verification-level contract row all say the same thing. Under auto, MEDIUM-or-higher plans and trigger matches still send the plan to Step 4 as before.
  • Earlier threads. My earlier stale-assessment finding (SKILL.md L234/L246) is fixed at this head: Step 3b now reassesses when an item-5 fix changed the plan's files or scope. The thread on eval coverage for the cut (original L66) was settled by owner decision Q304, so I did not raise it again.
  • Removed wording. None of the deleted phrases are still referenced anywhere outside CHANGELOGs, and no test greps for them. Nothing else in plugins/planning still says the reviewer always runs.
  • New eval case plan-low-risk-skips-reviewer. The tool_used grader (Agent, input_match: plan-reviewer, max 0) and the skip-line regex both agree with their pass/fail samples when I read through them by hand. I did not run anything. The author's test, eval and lint results in the PR body are their own claims and I have not checked them.
  • Changelog fragment. bump: minor matches a change to default behaviour. No plugin manifest changes, so there are no manifest lines.

Spend (not a hold): the new eval case runs 3 times, each capped at 60 turns and 1200 s, every time the eval suite runs. Under the default auto, LOW plans no longer dispatch the plan-reviewer, which cuts per-plan subagent use. Both draw on prepaid API or subscription usage only.

… plan

Under auto, Step 3 rates blast radius before dispatching the reviewer and
Step 3b presents that rating. A reviewer fix can widen the plan, so Step
3b now reassesses when an item-5 fix changed the plan's files or scope.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton

Copy link
Copy Markdown
Member Author

Held for eval evidence of no loss (docs/review/skills-and-agents.md:6-8). This PR cuts the plan reviewer from low- and medium-risk plans, so the owner chose (2026-11-11) to measure before and after first.

Next: a pre-registered design comment on this PR (cases, arms, negative control, decision rule), then the paid run, then the paired result. The label comes off through the eval-pair record when the run is VALID and shows no loss; if the suite cannot detect a loss, the hold stays and the question returns to the owner.

kyle-sexton and others added 3 commits November 11, 2026 17:16
Twelve plan requests (six rated LOW by design, six MEDIUM), each a seeded
draft with two defects the plan-reviewer checks and the main thread's
scripts do not, plus a must-not-flag guard and an arm-conformance check on
reviewer dispatch. Frozen for the before/after measurement this PR is held
on; fixtures/plan-reviewer-cut builds each arm and the degraded-reviewer
control.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
…ases

Spell "unparsable" in the notes-since case and its samples, spell out the
"exclud" regex stem in two out-of-scope graders (same matches), and drop
the status-marker word from init-repo.sh's comment.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton

Copy link
Copy Markdown
Member Author

Pre-registration: plan-reviewer cut, before vs after

Written before any after-version run. Cases frozen at b684380 (plugins/planning/evals/plan-reviewer-cut-*).

Question. Does removing the always-on plan-reviewer dispatch on LOW and MEDIUM plans (this PR) lose plan quality?

Arms. Same cases, same flags, launched together, --ablation none, --model claude-opus-5-5, 3 runs per case:

  • A (before): plugins/planning at 6df2738 (this PR's merge base).
  • B (after): plugins/planning at b684380 (this PR's head).

Cases. 12 seeded drafts, 2 defects each from what plan-reviewer checks, plus a must-not-flag guard. Score per run = defects fixed / 2. Deterministic graders only.

Calibration (pre-freeze, A and a degraded-reviewer copy, 4 cases, 1 run). A fixed 8 of 8 seeds; the degraded reviewer (agent body and dispatch prompt replaced by text with no procedure) fixed 7 of 8 and, in the trace read, still surfaced the seeded defect itself. Both smoke "medium" cases rated themselves LOW.

Why no detection control decides this. The protocol's control is the before text with the changed rule removed (plugins/evals/skills/plugin-eval/reference/reading-results.md, Detection control). For this PR that is B on LOW plans, so no result can show "no loss" under docs/review/skills-and-agents.md. The owner chose (2026-11-11) to measure A against B anyway.

Grouping. By the Blast-radius rating each run writes. MEDIUM is reported only if runs rate MEDIUM; otherwise MEDIUM is stated unmeasured.

Exclusions. A run is excluded when its arm's reviewer behaviour is wrong (A did not dispatch planning:plan-reviewer, or B did), it wrote no Blast-radius line, it errored or was partial, or it has an own-copy search denial. A case with fewer than 2 conforming runs in either arm is dropped from the comparison. B's exclusion rate is reported.

Decision rule. noise-report.py <B> --baseline <A> --margin 0.05, per observed tier.

  1. B loses if any case drops by 1/3 or more, or the mean falls by 0.15 or more: feat(planning): run the plan reviewer only for risky plans #15398 is closed and the reviewer stays.
  2. Otherwise the numbers go to the owner, who decides whether to waive the no-loss rule for this PR. This record releases nothing on its own.

The owner chose a before/after compare without the degraded-reviewer arm,
so its two fixture texts and prepare-arm.sh's --degrade-reviewer option
go (orphaned-fixtures). Case descriptions say "before behavior", not the
retired "contract" sense near "Brief" (vocabulary).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton

Copy link
Copy Markdown
Member Author

Result: plan-reviewer cut, before vs after

Ran per the pre-registration above: 12 cases x 3 runs x 2 arms, claude-opus-5-5, Claude Code 2.1.322, cases frozen at b684380. All 24 result files VALID (run-validity.py --runs 3 --scaffolded; only warning: no judge model recorded, and the suite has no judge). No run errored; no result partial.

LOW-designed cases (6; every run rated LOW and conformed):

  • A (before, reviewer always): mean 1.00.
  • B (after, no reviewer on LOW): mean 0.97; one run of 18 fixed one of its two seeds.
  • noise-report.py --baseline A --margin 0.05: change -0.03, 95% interval -0.08 to +0.03; no case drop; "not shown non-inferior at margin 0.05".

MEDIUM-designed cases (6): A rated MEDIUM in 3 of 18 runs, never twice in one case, so MEDIUM is unmeasured. By case, over conforming runs: B scored 1.00 everywhere; A scored 0.83 (catalogue-author-search) and 0.67 (reorder-points) with the reviewer running. B dispatched plan-reviewer in 5 of 18 runs (invoice-discounts 2, quiet-hours 2, overnight-shifts 1), all rated MEDIUM; those runs are excluded as non-conforming.

Decision rule. Rule 1 (B loses: a case drop of 1/3 or more, or a mean fall of 0.15 or more) is not met. By rule 2, the numbers go to the owner, who decides whether to waive docs/review/skills-and-agents.md's no-loss rule for this PR. No eval-pair release record is written: the run cannot show no loss, because the protocol's detection control is this PR on LOW plans.

kyle-sexton and others added 2 commits November 11, 2026 18:22
The before/after eval measured only LOW plans (change -0.03, interval
-0.08 to +0.03, no case drop); MEDIUM went unmeasured. So auto now skips
the plan-reviewer on a LOW plan with no trigger only, and runs it at
MEDIUM or higher, as the owner decided (ship only measured parts).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton

Copy link
Copy Markdown
Member Author

Hold lifted by owner decision (2026-11-11, in session): waive the no-loss rule for the LOW cut only, on the eval result above. The reviewer now skips only LOW plans with no trigger; MEDIUM and higher keep it until MEDIUM is measured (c4edfd2). Removing the label and re-arming auto-merge at this head.

@kyle-sexton kyle-sexton removed the do-not-merge Hard merge gate: do not merge while applied. label Nov 11, 2026
158 case files pushed this PR past one review's read budget, so both
review lanes read it only in part. The cases land in a separate PR on
branch test/plan-reviewer-cut-evals; this PR keeps the skill change and
the plan-low-risk-skips-reviewer case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton
kyle-sexton added this pull request to the merge queue Nov 11, 2026
Merged via the queue into main with commit 53c48cc Nov 11, 2026
73 checks passed
@kyle-sexton
kyle-sexton deleted the feat/plan-reviewer-risk-gated branch November 11, 2026 23:44
@github-actions github-actions Bot added the merge-path: owner-agent Post-merge accounting record (README.md, Merge paths) label Nov 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-path: owner-agent Post-merge accounting record (README.md, Merge paths)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant