Skip to content

test(planning): add plan-reviewer-cut before/after eval cases - #15465

Merged
kyle-sexton merged 6 commits into
mainfrom
test/plan-reviewer-cut-evals
Nov 12, 2026
Merged

kyle-sexton merged 6 commits into
mainfrom
test/plan-reviewer-cut-evals

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Member

No related issue: eval cases for the measurement behind #15398, split out so that PR stays within one review's read budget.

Summary

Twelve plan-reviewer-cut-* eval cases for /planning:plan, plus the fixtures that build each plugin-version arm. They measured #15398's plan-reviewer cut before and after, and stay as that measurement's outcome cases (skill-criteria "Measurement" step 7).

Fix

  • Each case scaffolds a small Python repo on a feature branch with a seeded draft at .work/<slug>/PLAN.md holding two defects that the plan-reviewer checks and the main thread's scripts do not, a must-not-flag guard, and two indicators at weight 0.001: reviewer dispatch (tool_used on Agent) and the Blast-radius tier.
  • Six cases are designed LOW and six MEDIUM; in practice the model rated most MEDIUM drafts LOW, so MEDIUM stays unmeasured.
  • fixtures/plan-reviewer-cut/init-repo.sh commits each case repo with a local origin; prepare-arm.sh <ref> <dest> builds one arm, a copy of plugins/planning at a commit, outside any repository.
  • All graders are deterministic (regex and tool_used); no judge calls.

Verification

Related

Follow-ups

  • plan-reviewer-cut-medium-reorder-points/graders/out-of-scope-cut.md still excuses a phase bullet with an incidental "no"/"not" (the shape fixed in the temperature grader); narrow it the same way before that case decides anything. The low-risk re-run (Q308) does not use it.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ

kyle-sexton and others added 2 commits November 11, 2026 18:29
Twelve seeded plan drafts (six designed LOW, six MEDIUM), each with two
defects from what the plan-reviewer checks, a must-not-flag guard, and
reviewer-dispatch and blast-radius indicators. They measured #15398's
reviewer cut on LOW plans; fixtures/plan-reviewer-cut builds one arm per
plugin version (prepare-arm.sh) and sets up each case repo (init-repo.sh).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VQb9Q4F46UumaUgXeUdpYZ
@kyle-sexton
kyle-sexton marked this pull request as ready for review November 11, 2026 23: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-11T23:35:52.736710Z 5e8c1ff 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.

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

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

Security review: incremental, 9b9b09e to 5aa784a (PR #15465)

No security issues in this push. This lane already reviewed an earlier head, so this review was blocking only: it reports only CRITICAL and IMPORTANT findings. It found neither.

What I read. Every hunk of all 4 files in .claude-lane/incremental-files.txt, from .claude-lane/incremental.diff. They are the regression-test-named grader and its samples file for two eval cases:

  • plugins/planning/evals/plan-reviewer-cut-low-retry-backoff-cap/
  • plugins/planning/evals/plan-reviewer-cut-medium-overnight-shifts/

What changed:

  • Graders. The negative lookahead in each grader regex is narrower now. A bare no, not or without no longer rejects a list item. Only phrases that decline a test do, such as no (new) regression/test(s), without (a) (new) regression/test(s), none, skip* and carve-out.
  • Samples. Each samples file gains one new pass case: static plan markdown in a JSON string, where the regression test is phrased as a negative.

What I checked:

  • Trusted paths. No file in this push matches a trusted-path glob.
  • Pipeline files. The push touches no workflow, action, merge-hold script, trusted-actor list, CODEOWNERS, .claude/ file or plugin manifest. No trust control is involved.
  • Standing instructions. None is deleted or softened. These files are eval grading data, not instructions a lane follows.
  • What the files do. Nothing here runs a command, uses a secret or reaches the network. Nothing feeds outside input into a credentialed context. The patterns are fixed regexes that the eval harness runs against its own output files. Each new pattern is a bounded alternation inside a lookahead that applies to one line, and it adds no new backtracking risk on top of the existing pattern.
  • PR context. The PR context file contains no embedded instructions aimed at this lane.

Prior findings. This lane's earlier summary (#15465 (comment)) reported 0 findings, so nothing carries over.

critical-findings: 0

@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: 5 inline finding(s) on 5e8c1ff9c95891c43bf5430a78b6f327507e0759; the summary is in this lane's pull request comment.

claude-lane: claude-review

@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

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

claude-review: incremental, blocking-only review of 5aa784ad. This lane already reviewed earlier heads (last 9b9b09e9), so this review reports blocking findings only. I read every hunk of all 4 files in scope: the two regression-test-named.md graders and their two sample files.

Findings: 0.

What changed. Both graders replace the broad exclusion \b(?:no|none|not|without|skip…|carve-out)\b with a narrower one that matches a declined test: no [new] regression/test, none, without [a] [new] regression/test, skip…, carve-out. Each sample file gains a pass sample whose regression test is phrased as a negative. This answers the two 9b9b09e9 threads, which the owner resolved at this head. Those threads stay settled.

What I checked. I traced each regex against every sample by hand. I did not run validate-cases.py.

  • New pass samples match. In the retry-backoff case, does not exceed config.MAX_BACKOFF_SECONDS under a ### Phase heading clears the exclusion (not no longer counts). It meets lookahead 1 through regression and lookahead 2 through MAX_BACKOFF_SECONDS. In the overnight case, is not rejected and totals 8.0 meets lookahead 1 through regression and lookahead 2 through 22:00.
  • Earlier pass samples still match. None of their test bullets contains any of the narrowed decline terms.
  • Earlier fail samples still fail.
    • Retry-backoff: no regression test; … carve-out and no new regression test; … are excluded by \bno\s+(?:new\s+)?regression.
    • Overnight: none; … no overnight case yet is excluded by \bnone\b, and no regression test for the overnight case by \bno\s+regression.
    • In both cases, the seeded Phase bullets carry no test word, and the discover -s tests Sanity Checks are not bullets, so these samples still fail too.
  • JSON structure. Both sample files stay valid arrays: each new object follows a comma and closes its brace.

Context.

  • The PR closes no issue, so there were no acceptance criteria to check.
  • Any verification described in the PR is the author's claim. I did not verify it.
  • The PR context, settled threads and prior findings contain no embedded instructions.

Spend (not a finding): running these eval cases uses only prepaid API or subscription usage.

@kyle-sexton kyle-sexton added the do-not-merge Hard merge gate: do not merge while applied. label Nov 11, 2026
@kyle-sexton

Copy link
Copy Markdown
Member Author

Held while the five grader findings from the code-review lane are fixed: four graders can pass a plan that still carries its seeded defect, and one can fail a correct fix. The label comes off once the fixes are pushed and reviewed.

The code-review lane showed four graders passing a plan that still
carried its seeded defect and one failing a correct fix. Each pattern
now counts only bullets under a `### Phase` heading and stops at the
next heading; the regression-test graders require a test word and the
defect on one bullet; the temperature out-of-scope grader excuses only a
bullet that starts or ends with a cut verb. Each reviewer counterexample
is a new labeled sample.

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: low-risk re-run with fixed graders (owner decision Q308)

Written before any run. Cases at d4e2c5a (this PR), graders fixed per the five resolved review threads.

Why. #15398 merged on a LOW result of -0.03 (interval -0.08 to +0.03) graded by patterns that could pass a plan still carrying its seed, which inflates both arms and hides differences. The old runs cannot be re-graded.

Arms. Same flags, each case's two arms launched together, --ablation none, --model claude-opus-5-5, 3 runs per case:

Cases. The six plan-reviewer-cut-low-* cases.

Exclusions. As in #15398's pre-registration: wrong reviewer behaviour for the arm, no LOW Blast-radius line, an error or partial result, or an own-copy search denial. A case with fewer than 2 conforming runs in either arm is dropped.

Decision rule. noise-report.py <B> --baseline <A> --margin 0.05 over the six cases. B loses if any case drops by 1/3 or more, or the mean falls by 0.15 or more: #15398's LOW cut is reverted and the reviewer runs on every plan again. Otherwise the Q307 waiver stands on these numbers.

@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 d4e2c5a626f81d3cc7c226c499a9148c4b1886e6; the summary is in this lane's pull request comment.

claude-lane: claude-review

@kyle-sexton

Copy link
Copy Markdown
Member Author

Result: low-risk re-run with fixed graders (Q308)

Ran per the pre-registration above: 6 LOW cases x 3 runs x 2 arms, claude-opus-5-5, cases at d4e2c5a. All 12 result files VALID (only warning: no judge model recorded; the suite has no judge). No run errored; every run conformed (A dispatched plan-reviewer, B did not; all rated LOW).

Case A (before) B (after)
blog-permalink 1.00 1.00
ledger-delimiter 1.00 0.83
notes-since 1.00 1.00
retry-backoff-cap 0.67 0.50
temperature-decimals 0.50 0.67
till-price-lookup 1.00 1.00
mean 0.86 0.83

noise-report.py --baseline A --margin 0.05: change -0.03, 95% interval -0.13 to +0.07; no case drop; "not shown non-inferior at margin 0.05".

Decision rule. B loses only on a case drop of 1/3 or more or a mean fall of 0.15 or more; neither occurred, so the Q307 waiver for #15398's LOW cut stands. The fixed graders now catch misses in both arms (A scores 0.50-0.67 on two cases with the reviewer running).

The grader-fix hold is lifted: the five findings are fixed and resolved.

@kyle-sexton kyle-sexton removed the do-not-merge Hard merge gate: do not merge while applied. label Nov 12, 2026
Re-grading the kept re-run plans found graders failing plans that had
fixed their seed:
- 11 bullet graders counted only - and * items; they now also count
  numbered work items (1. or 1)).
- The retry regression-test grader accepted the wrong MAX_DELAY and not
  the real MAX_BACKOFF_SECONDS; it accepts both now.
- Both regression-test graders now also count items under a Test strategy
  heading, where the plan template puts tests.
- Both sanity-check graders matched a summary quoting the old vague
  check; they now match only Sanity Check lines.
Each false fail is a new labeled pass sample.

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

Correction: re-run re-graded with corrected graders

A further review thread showed the retry grader rejecting the real constant. Re-grading the re-run's kept plans offline (sandboxes kept with --keep-temp; regex graders re-evaluated on each plan, tool_used verdicts unchanged) found more false fails in both arms: numbered work items not counted, regression tests under a Test strategy heading not counted, and a sanity-check grader matching a summary that quotes the old vague check. All are fixed in af91466 with labeled pass samples; validate-cases.py shows no FAIL.

Case A (before) B (after)
blog-permalink 1.00 1.00
ledger-delimiter 1.00 1.00
notes-since 1.00 1.00
retry-backoff-cap 0.83 0.83
temperature-decimals 0.83 1.00
till-price-lookup 1.00 1.00
mean 0.94 0.97

The re-graded change is +0.03 (B over A), with no case drop. Under the pre-registered rule B does not lose, so the Q307 waiver for #15398's LOW cut stands. The one remaining B miss names AC2 ceiling tests without calling them a regression test; it is counted as a miss.

@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: 2 inline finding(s) on af914665e8aec26d35ebd0c220edb7b193aa707c; the summary is in this lane's pull request comment.

claude-lane: claude-review

Counting Test strategy items let a line that names the bug but declines
the regression test pass both regression-test graders. A bullet carrying
no, none, not, without, skip or carve-out no longer counts. The review's
three counterexamples are fail samples, and overnight-shifts gains a Test
strategy pass sample.

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

@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: 2 inline finding(s) on 9b9b09e936d714f1c6d9bd1f71d873d5ab5fcb2c; the summary is in this lane's pull request comment.

claude-lane: claude-review

The any-negation exclusion failed correct tests phrased as negatives
("does not exceed", "is not rejected"). Both regression-test graders now
reject only an actual decline: "no (new) regression/test", "none",
"without a test", skip, or carve-out. The review's negative phrasings
are pass samples; every earlier fail sample still fails.

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 12, 2026
@kyle-sexton

Copy link
Copy Markdown
Member Author

Pre-registration: fresh low-risk run on frozen graders (Q308)

Written before any run. Graders frozen at 5aa784a (the head the code-review lane read with 0 findings). No grader changes after this run's scores are read.

Why a fresh run. The earlier re-run was re-scored after its scores were read, with graders changed in between, and the last tightening was never applied to it. That breaks the pre-registered sequence, so it does not settle Q307's waiver. This run does.

Arms. Same flags, each case's two arms launched together, --ablation none, --model claude-opus-5-5, 3 runs per case:

Cases. The six plan-reviewer-cut-low-* cases.

Exclusions. Wrong reviewer behaviour for the arm, no LOW Blast-radius line, an error or partial result, or an own-copy search denial. A case with fewer than 2 conforming runs in either arm is dropped.

Decision rule. noise-report.py <B> --baseline <A> --margin 0.05 over the six cases, on the scores the CLI records. B loses if any case drops by 1/3 or more, or the mean falls by 0.15 or more: #15398's LOW cut is reverted and the reviewer runs on every plan again. Otherwise the Q307 waiver stands on this run.

Merged via the queue into main with commit 18b39fb Nov 12, 2026
57 checks passed
@kyle-sexton
kyle-sexton deleted the test/plan-reviewer-cut-evals branch November 12, 2026 01:15
@github-actions github-actions Bot added the merge-path: owner-agent Post-merge accounting record (README.md, Merge paths) label Nov 12, 2026
@kyle-sexton

Copy link
Copy Markdown
Member Author

Result: fresh low-risk run on frozen graders (Q308)

Ran per the pre-registration above, graders frozen at 5aa784a; no grader changed after scores were read. 6 LOW cases x 3 runs x 2 arms, claude-opus-5-5. All 12 result files VALID (only warning: no judge model recorded; the suite has no judge). No run errored; every run conformed.

Case A (before) B (after)
blog-permalink 1.00 1.00
ledger-delimiter 1.00 1.00
notes-since 1.00 1.00
retry-backoff-cap 1.00 0.83
temperature-decimals 0.83 0.83
till-price-lookup 1.00 1.00
mean 0.97 0.94

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".

Decision rule: B loses only on a case drop of 1/3 or more or a mean fall of 0.15 or more. Neither occurred, so the Q307 waiver for #15398's LOW cut stands on this run.

Spot-read: the B retry miss adds ceiling tests (backoff(7) and backoff(8) equal config.MAX_BACKOFF_SECONDS) in a Test strategy paragraph without calling them a regression test. The frozen grader scores it as a miss, and that score stands; counting it as a pass would only raise B.

@kyle-sexton

Copy link
Copy Markdown
Member Author

Arm isolation check. Arm B (cc4acc0) includes main merged into #15398's branch, so it differs from A (6df2738) by more than the reviewer cut. The paths /planning:plan runs on (skills/plan/, agents/, reference/config.md) differ only by #15398's own commits (9129583, e551404, a5ce584). The one main commit in that area, ffcb24e, changed only setup docs. About 24 other planning files differ (interview, prd, questionnaire, setup skills; surfaces; probes). These cases hand /planning:plan a draft with its Brief already written, so those skills should not load. That is UNVERIFIED: the traces were removed. A future run of this suite should build B from A plus only the PR's commits.

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