plan-review: ship campaign-selected dual-reviewer engine as a .claude/skills/ skill (workflow-eval step 3) - #725
Conversation
Records the outcome of the first plan-review measurement campaign and pins the arm config to the production reviewer. - config/configs.json: A/B/C/E Claude side re-pinned claude-fable-5 -> claude-opus-4-8 (production default moved to Opus 4.8); cli_version bumped to the campaign build 2.1.217. D stays claude-sonnet-5 (the cheaper-Claude probe). All single-field contrasts preserved (A-vs-B variant, B-vs-C mode, B-vs-D model, C-vs-E codex model). - verdicts/campaign-1.md (new): committed aggregate result — sensitivity scorecard (dual+codex-sol 7/9 must-catch vs 1/9 single), precision (hallucination rate 3.4% dual / 0% single), the codex-sol-vs-terra and criteria-rewrite contrasts, and the negative-control base_sha-contamination lesson. Numbers and lessons only; the real-plan corpus stays gitignored. - DECISION_RULE.md: "Campaign 1 outcome" note — the pre-registered dual-vs-single and default-model questions are now data-backed; records that the FP gate was not applied (contaminated negatives) and precision was re-measured as true-vs-false hallucination rate. - test_plan_review_eval.py: the provenance-snapshot test reads arm A's model from the config instead of hardcoding it, so it survives the pin change.
Promotes the Campaign 1 winner (candidate criteria + dual review: Claude @ Opus 4.8 + codex gpt-5.6-sol, merge/verify) into a first-class Agent Skill and retires the single-reviewer commands. The content-hash ExitPlanMode gate and snapshot helper are engine-agnostic and untouched. - .claude/skills/plan-review/: SKILL.md owns the review->revise cycle; render.py (strict __TOKEN__ renderer mirroring criteria_source.render, byte- equivalent — never free-text substitution) with a CLI so prompts render in tested Python; codex_review.py runs reviewer 2 with the validated pins (gpt-5.6-sol/xhigh/600s) and loud-fallback exit codes; criteria.md, reviewer_prompt.md, merge_verify.md bundled as-validated (byte-identical to candidates/). Dual: Opus reviewer subagent + codex-sol, blind, then a merge/ verify subagent; codex absent/timeout/error -> loud single-Claude fallback. - CLAUDE.md "Plan Review Before Approval": the gate now invokes the skill instead of spawning a single review agent over review-plan.md. - Retire .claude/commands/review-plan.md + revise-plan.md; their plan-review contract tests migrate to tests/test_plan_review_skill.py (targeting SKILL.md + CLAUDE.md), and review-plan/revise-plan drop out of test_command_contract.py's _HARDENED set. The harness control arm still sources the old criteria via `git show <sha>:` from history, so the deletion breaks nothing. - tests/test_plan_review_skill.py: artifact byte-match to the graded candidates, render byte-equivalence + raise-on-unfilled-token, the codex/ Claude reviewer pins, snapshot routing, and the migrated command contracts. - CHANGELOG: one internal-tooling entry.
…de, reviewer names Two locked pre-campaign decisions that the Step-3 plan under-implemented, now restored (both verified against the decision ledger): - Adaptive three-way recommendation: the ExitPlanMode gate ALWAYS offers Dual / Single / Skip, with the (Recommended) tag chosen adaptively by plan complexity (substantive/high-risk -> Dual; localized/mechanical -> Single; trivial -> Skip), not a fixed default. SKILL.md gains a deliberate single-reviewer mode (Opus alone), distinct from the codex-unavailable fallback. - Names in the persisted file: production merge_verify names the reviewer on single-reviewer findings ([single reviewer: claude] / [single reviewer: codex]) so a human can weigh the complementary strengths the campaign found (codex on methodology/math, Claude on codebase-structure facts). criteria.md + reviewer_prompt.md stay byte-locked to the graded copies; only merge_verify's OUTPUT LABELING is un-blinded (the campaign blinded it for grading) — detection/verify logic unchanged, no re-validation. Two carried-open design questions resolved to no-change: the initial review does not re-dump the plan (already in context from plan mode; revise phase shows both), and --pr feedback-coverage stays an explicit capability passed by read-feedback-revise, not auto-detected in the gate. Tests updated: adaptive-offer + single-mode assertions, merge_verify production-adaptation, and the detection-prompt byte-lock split from merge_verify.
…all Assessment onward' (smoke-surfaced)
Resolve the findings from the local codex review of the plan-review skill. Decisions (user-directed): - CQ1 descope: remove the advertised-but-unimplemented `--updated`/`--pr` modes from SKILL.md Modes; they are named only as a tracked TODO follow-up. - M2 timeout: widen the codex ceiling 600s -> 1200s and loosen the claim - the timeout is an interactive-gate choice, NOT a campaign-validated pin (the campaign ran unattended at 3600s); only model/effort are campaign pins. Mechanical fixes: - CQ2: Step 8 is now advisory (surface findings only). Plan edits move to the Revise phase, which re-reviews and re-stamps - editing after persist would invalidate plan_sha256 and re-deny ExitPlanMode. - MT1: the Revise re-review re-runs in the SAME engine mode the existing review used (dual vs deliberate-single), read from the review body - no silent upgrade to dual. - MT2: Revise honors the helper's `fresh` flag - a stale review (plan already changed) triggers a fresh Review pass before revising. - TD1: a snapshot-lifecycle callout requires `plan_snapshot.py abort` on ANY post-snapshot failure/stop, not only persist exit 3. - S1: codex_review.py runs openai_review's sensitive-file scan + notice before call_codex (the direct entry bypassed main() where the /ai-review-local codex path prints it). - Perf: reviewer 1 (Opus subagent) and reviewer 2 (codex) launch concurrently. - M1: campaign-1 verdict precision table adds the `unverif.` column + a Σ row so every row reconciles (findings = true + false + unverifiable; 213 total). Tests: bump the timeout pin assertion to 1200; add a descope contract test and behavioral tests for codex_review.main (pins forwarded, sensitive-notice fires, exit-code contract 0/2/3). TODO.md gains the descope, clean-negative-rerun, and trop-silent-drop follow-up rows.
…sessment, engine provenance Address the round-2 local codex review (⛔) of the plan-review skill. P0 — concurrency integrity: the four intermediate prompt/review files used fixed $SCRATCH names, so two concurrent same-worktree reviews could cross-wire and persist one plan's merged report under another plan's certified hash, silently defeating the gate. Each invocation now gets a private `<work_dir>` (mktemp) that holds all intermediates; it is removed on persist and on every abort/failure path. (The helper's own snapshot/state/body/meta paths were already nonce-unique.) P0 — codex read surface (user decision: keep parity + tracking): document the codex `--sandbox read-only` cross-repo read surface in-skill, the same surface /ai-review-local already documents and accepts, cross-linking the existing TODO.md OS-isolation follow-up. No feature gate. P2 fixes: - Assessment is now derived by a deterministic P-count -> label rule (P0/P1 -> Significant; P2 -> Minor; P3/none -> Ready); malformed reviewer output aborts instead of persisting a guessed assessment. - Engine provenance is a machine-readable `<!-- plan-review-engine: ... -->` body marker (dual / single / single-fallback); Revise reads the marker rather than inferring the mode from fragile prose. Tests: contract tests for invocation-scoping, the deterministic assessment rule + malformed-abort, the provenance marker + revise wiring, and the codex read-surface documentation. TODO.md gains the dual-mode peer-cancellation follow-up (the remaining P2, deferred). 96 tests pass across the skill/gate/ helper/contract suites.
…und-3 fixes Address the round-3 local codex review (⚠️ ) of the plan-review skill. Campaign claims (user decision: recalibrate + ship). The campaign supports "dual >> single" on sensitivity (7/9 vs 1/9, 0 regressions B->C), but it is NON-GATING under its own pre-registration: the s3 negative controls were base_sha-contaminated (FP gate invalid, corpus floor unmet), the candidate criteria carried a must-catch regression (trop-silent-drop, a NO-GO by gate 1), and the run used claude-opus-4-8 where the arms register claude-fable-5. The shipped engine also differs from the graded one (un-blinded merge prompt, opus alias). So "validated / data-backed / as-validated" overclaimed. Recalibrated that language to an honest "exploratory / campaign-selected, NON-GATING" framing that names the deviations and points to the tracked clean re-validation, across DECISION_RULE.md, verdicts/campaign-1.md, the eval README, SKILL.md, CLAUDE.md, and CHANGELOG. Dual stays the default (the sensitivity signal is robust and independent of the unmet FP gate); we ship arm C as run (candidate criteria + dual), carrying trop-silent-drop as a tracked follow-up. The bundled-prompt description now distinguishes the byte-identical detection prompts from the production-adapted (un-blinded) merge_verify.md. Mechanical fixes: - Skip branch (CLAUDE.md) now `mkdir -p "$SCRATCH"` before writing plan-path.txt — on a fresh worktree .git/plan-review does not exist, so the Write failed. - plan_snapshot.py `abort` is now idempotent when the state file is absent: persist self-cleans on exit 3 / altered-snapshot, so a follow-up abort finds nothing and must succeed (not error). SKILL.md step 7 now aborts uniformly on any non-zero persist + rm -rf work_dir. - Eval README status updated from "campaign has not run" to the exploratory / NON-GATING outcome. Tests: CLAUDE.md Skip-branch scratch-init contract; idempotent-abort lifecycle test. 98 tests pass across the skill/gate/helper/contract suites; 106 eval-harness tests pass; black + ruff clean.
Address the round-4 local codex review (⚠️ ) of the plan-review skill. P1 (executability) — the SKILL.md and CLAUDE.md ingress relied on `$SCRATCH`/ `$SKILL` shell variables surviving across separate Bash tool calls and the Write tool expanding `$SCRATCH/plan-path.txt`; neither holds (the codebase's own submit-pr/pr_prepare convention). Fixed every ingress: the skill dir is now the literal `.claude/skills/plan-review/` (no `$SKILL`), the scratch dir is derived + `echo`'d so its printed literal is what the Write tool receives, and every snapshot/check Bash block re-derives the scratch path inline via `$(git rev-parse --git-path plan-review)`. Covers the dual, single, revise, and skip paths. P2 fixes: - Both Skip paths (SKILL.md revise-Skipped, CLAUDE.md Skip) now `abort` on a pre-persist failure so the helper-managed snapshot is not retained. - Model provenance: `model: "opus"` is documented as a runtime family alias (the Task tool takes aliases, not exact IDs), NOT an immutable pin — matching the recalibrated campaign claims. - Test rigor: `test_reviewer_invocations_are_pinned` now asserts BOTH the reviewer and merge subagent sections select opus (not one match anywhere); new contract tests assert ingress self-containment and Skip-path abort-on-failure. P3: CHANGELOG now says the hash-gate + snapshot/persist contract are unchanged while `abort` is idempotent (was inaccurately "unchanged"). 100 tests pass across the skill/gate/helper/contract suites; black + ruff clean.
… note Address the genuine (mechanical) findings of the round-5 local codex review. The round-5 ⛔ is driven by the codex read-surface P0, which the user already adjudicated (round 2: keep parity + tracking — the same surface /ai-review-local ships with, tracked at TODO.md); no code change there. P2 — `abort` over-permissive: round 3 asked for idempotent abort; round 5 noted idempotent-by-default silently accepts a wrong/typo'd/cross-wired state token, masking a leaked snapshot. Resolved both with an explicit flag: `abort` now FAILS (exit 2) on a nonexistent state by default, and only `--allow-missing` downgrades that to a no-op. The skill's post-snapshot cleanup calls pass `--allow-missing` (persist self-cleans on exit 3 / altered-snapshot), so it can clean up uniformly without masking a wrong token. Strict default protects the eval harness and any direct/future caller. P2 — configs.json protocol drift: arms A/B/C/E declare claude-opus-4-8 while DECISION_RULE.md registers claude-fable-5. Added a `_deviation_note` making this explicit: the file is the AS-RUN exploratory Campaign 1 config (Opus override); the immutable pre-registration stays fable-5; a gating re-validation would add a fresh versioned config. Consistent with the recalibrated NON-GATING framing. Also clarified the codex-surface test: it verifies the accepted-surface DISCLOSURE (per the parity+tracking decision), not a filesystem boundary. 179 tests pass across the skill/gate/helper/contract/eval suites; black + ruff clean; configs.json valid.
Overall AssessmentExecutive Summary
MethodologyNo findings. No causal methods or Registry sections are affected; paper cross-checking is not applicable. Code QualityP2 — Snapshot cleanup can be silently skippedImpact: On an ingress race, the skill reruns without aborting the first snapshot. Additionally, every cleanup uses Location: Concrete fix: Strictly abort the mismatched snapshot before rerunning. Use unflagged P2 — Assessment validation trusts the reviewer’s tableImpact: A report containing a P0/P1 finding but an inconsistent zero-count table can be persisted as Location: Concrete fix: Add a Python parser that counts finding lines, validates the summary table against them, and derives metadata; reject any mismatch. PerformanceP3 — Dual-review peer cancellation is deferredImpact: A failed reviewer may leave its peer running for up to 1200 seconds. Concrete fix: None required for this PR; tracked in MaintainabilityNo additional findings. Tech DebtP3 — Tracked limitationsThe retired SecurityP1 — Printed paths are reparsed as shell syntaxImpact: Location: Concrete fix: Keep full paths out of generated shell source. Prefer a Python orchestrator using argv arrays, or pass only validated safe nonce tokens and derive paths inside Python. Add regression coverage using a git/worktree path containing literal P3 — Codex read isolationThe broader filesystem read surface is disclosed and tracked in Documentation / TestsP2 — Tests miss production cleanup and path interpolationImpact: Tests verify strict abort only without Location: Concrete fix: Add behavioral tests for wrong production state tokens, ingress-mismatch cleanup, and metacharacter-bearing worktree paths. P3 — Changelog overstates abort idempotenceImpact: Location: Concrete fix: Name the flag explicitly. Path to Approval
|
…nt + assessment hardening Address the authoritative CI codex review (⚠️ ). Note: the recalibration worked — the codex read-surface finding is now correctly P3/tracked, not the local reviewer's insisted P0. P1 (security) — printed paths reparsed as shell. The round-4 ingress rework had the agent paste a `mktemp`-under-`$(git rev-parse --git-path plan-review)` work dir into later Bash commands; command substitution executes inside double quotes, so a repo/worktree checked out at a path containing `$()`/backticks would execute it. Fixed at the root: `plan_snapshot.py` now EMITS the per- invocation work dir under its own snapshots dir (same strict [a-f0-9.] leaf, HOME-prefixed), and `_cleanup_invocation` removes it (persist + abort). The skill uses the helper-emitted `work_dir` — no repo/worktree path is composed into a shell command anywhere. This also drops the manual `rm -rf` and the mktemp step. P2 — `--allow-missing` over-applied. Reserved it for the ONE post-persist abort (where persist may have self-cleaned); pre-persist aborts are plain, so a mistyped/cross-wired state token fails loudly instead of silently no-op'ing. P2 — assessment trusted the reviewer's table. Now the label is derived from the actual finding LINES (counted by severity tag), cross-checked against the summary table; a mismatch or malformed report aborts rather than persisting a guessed assessment. P2 (minor) — an ingress-race rerun now aborts the first (orphaned) snapshot before re-running. CHANGELOG corrected to describe the helper accurately (emits/cleans a work dir; strict `abort` + `--allow-missing`). Tests: work_dir emission + persist/abort cleanup, the abort-placement contract, and updated scoping/assessment contracts. 210 tests pass across the skill/gate/helper/contract/eval suites; black + ruff clean.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall AssessmentExecutive Summary
MethodologyNo findings. The PR changes internal workflow tooling only; Code QualityP2 — Post-persist cleanup can still mask a wrong tokenImpact: After any failed Location: Concrete fix: Use P2 — Work-directory creation is not transactionalImpact: The snapshot is written before Location: Concrete fix: Wrap snapshot, work-directory, and state creation in rollback logic that removes every partially created artifact. PerformanceP3 — Dual-review peer cancellation is trackedImpact: A failed reviewer can leave its peer running for up to 1200 seconds. Concrete fix: None required here; tracked in MaintainabilityNo findings. The prior assessment-table issue is addressed by line counting, table cross-checking, and fail-closed handling. Tech DebtP3 — Tracked limitationsImpact: Retired flags, clean-negative validation, and the Concrete fix: None required for approval; tracked in SecurityP1 — Prior shell-path injection remains through
|
…ctional snapshot Address the authoritative CI codex re-review (⚠️ ). It confirmed the git-path injection was fixed and accepted the assessment hardening; the remaining P1 was the same class one prefix deeper (the helper's HOME-derived path prefix), plus two P2s. Per the chosen approach (fail-closed validation gate): P1 (security) — HOME-prefix path injection. Every path plan_snapshot.py emits is pasted into the caller's shell as a quoted literal; the digest+nonce leaf is safe by construction but the HOME-derived prefix was not (a `$HOME` containing `$()`/backticks executes under command substitution). `_plans_home()` now validates the resolved plans dir and FAILS CLOSED if it carries a shell metacharacter ($ ` " \\ or newline), so no unsafe path is ever emitted. All snapshot/persist/check/abort paths inherit the guard. P2 — post-persist cleanup could mask a wrong token. Made `persist` self-clean the ENTIRE invocation (snapshot + work_dir + sidecars) on every post-load failure, so the skill no longer runs any abort AFTER persist. `--allow-missing` is removed entirely and `abort` is strict (a missing state token is an error), ending the round-3→5→CI oscillation on that flag. P2 — non-transactional snapshot. Snapshot + work dir + state are now created as a unit; any failure rolls back every partial artifact rather than orphan a snapshot with no state token to abort. SKILL.md / CLAUDE.md updated (plain pre-persist abort only; no post-persist abort; no `--allow-missing`). CHANGELOG corrected. Tests: fail-closed validation gate, persist-self-cleans-on-failure, transactional/work-dir cleanup, strict abort. 211 tests pass across the skill/gate/helper/contract/eval suites; black + ruff clean.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — Prior P1 path-injection and cleanup findings are resolved. Only P2/P3 documentation and test gaps remain. Executive Summary
MethodologyNo findings. No methods are affected, and Code QualityNo findings. The prior cleanup issues are addressed in PerformanceP3 — Peer cancellation remains trackedImpact: A surviving reviewer can continue until its timeout after its peer fails. Concrete fix: None required for approval; tracked in MaintainabilityP3 — Helper is incorrectly labeled “UNCHANGED”Impact: The skill says the snapshot helper is unchanged even though this PR adds Location: Concrete fix: Say the hash-certification contract is unchanged while the helper lifecycle is extended. Tech DebtP3 — Deferred workflow limitations are trackedImpact: Retired flags, clean-negative validation, and the known criteria regression remain pending. Concrete fix: None required; tracked in SecurityP3 — Codex read isolation is trackedImpact: Read-only mode does not prevent Codex from reading outside the repository. Concrete fix: None required here; disclosed and tracked in The prior shell-path P1 is resolved by Documentation / TestsP2 — Non-gating campaign is still called “validated”Impact: Maintainers could mistake exploratory, non-gating results for successful validation. Location: Concrete fix: Replace “campaign-validated” with “campaign-graded,” “as-run,” or “selected from exploratory sensitivity results.” P2 — Unsafe-home regression covers
|
…e helper, backtick test Final P2/P3 items from the ✅ CI review (no P0/P1 outstanding): - Replace the last 'campaign-validated' remnants in codex_review.py, render.py, and the skill-test docstring/assert with 'campaign-graded' / 'as-run' / 'unmeasured' — consistent with the exploratory/NON-GATING framing. - SKILL.md no longer calls the snapshot helper 'UNCHANGED': its hash-certification contract is unchanged, but the lifecycle is extended (work dir, transactional creation, self-cleaning persist, strict abort). - The fail-closed-home regression test now covers backticks as well as $(). 49 targeted tests pass; black + ruff clean.
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment✅ Looks good — No unmitigated P0/P1 findings. Prior review findings are resolved; remaining limitations are explicitly tracked. Executive Summary
MethodologyNo findings. Code QualityNo findings. Transactional creation and self-cleaning persistence are consistently implemented in PerformanceP3 — Peer cancellation is trackedImpact: A surviving reviewer may run until the 1200-second timeout after its peer fails. Concrete fix: Surface the Codex process handle and cancel the peer on failure, as tracked in MaintainabilityNo findings. The prior inaccurate “unchanged helper” wording is corrected in Tech DebtP3 — Deferred workflow and validation work is trackedImpact: Delta/PR modes, clean-negative precision measurement, and the known criteria blind spot remain deferred. Concrete fix: Follow the concrete entries in SecurityP3 — Codex read isolation remains trackedImpact: Read-only mode prevents writes but does not restrict reads to the repository. Concrete fix: Add OS-level isolation as tracked in Documentation / TestsNo findings. The prior campaign-language issue is corrected in |
1 participant
Summary
/review-plan+/revise-planfor a first-class.claude/skills/plan-review/skill that runs the dual review engine (two blind reviewers — Claude @ Opus 4.8 + codexgpt-5.6-sol— then a merge/verify pass), degrading loudly to a single-Claude review when codex is unavailable. This is workflow-eval step 3 (the engine swap); the content-hashExitPlanModegate from step 1 is untouched.trop-silent-dropcandidate-criteria regression, and an Opus-vs-registered-claude-fable-5model deviation). Every "validated"/"data-backed" claim was recalibrated to that honest framing across the verdict,DECISION_RULE.md, the eval README,SKILL.md,CLAUDE.md, andCHANGELOG; a clean re-validation is tracked.render.py(strict__TOKEN__renderer, byte-equivalent to the eval harness),codex_review.py(campaign-pinned codex model/effort; the Claude side uses theopusruntime family alias — the Task tool takes aliases, not exact IDs — documented as such, not an immutable pin), andmerge_verify.md(production-adapted to name the two reviewers). The detection promptscriteria.md+reviewer_prompt.mdare byte-identical to what the campaign graded.plan_snapshot.pysnapshot/persist/check + the hash-gate contract are unchanged;abortis now strict by default with an explicit--allow-missingno-op for post-persist cleanup.Methodology references
docs/methodology/REGISTRY.mdchanges.Validation
tests/test_plan_review_skill.py(prompt byte-match, render equivalence, behavioralcodex_reviewtests, lifecycle contracts),tests/test_plan_snapshot.py(strict/--allow-missingabort lifecycle),tests/test_command_contract.py(migrated the retired-command contracts),tests/test_plan_review_eval.py(config pin). 179 tests pass across the skill/gate/helper/contract/eval suites;black+ruffclean.Known / tracked
--sandbox read-only, which blocks writes but does not confine READS to the repository — the same accepted surface/ai-review-local --backend codexalready ships with. It is disclosed inSKILL.mdand tracked for OS-level isolation inTODO.md; plans are authored in the user's own session.trop-silent-dropcriteria re-validation, dual-mode peer cancellation, and the (descoped)--updated/--prmodes.Security / privacy