diff --git a/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md b/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md new file mode 100644 index 0000000000..4f4cfb8fd2 --- /dev/null +++ b/.changes/implementation/feat-8331-phase-boundary-review-dispatch-5d2a7c41.md @@ -0,0 +1,7 @@ +--- +bump: minor +--- + +### Added + +- A plan phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` now gets that review at its boundary. `/implementation:implement` and `/implementation:implement-dispatch` invoke `/review:quality-gate architecture`, `security`, `code-design` or `slice concurrency` with the previous phase-boundary commit as `--base`, the plan's `Quality bar:` line when it has one, `--report-only`, and, for a worker's worktree, `--worktree`, and pass no rationale. The sequence is review, fix inside the phase fence, one re-review against the same base, then the acceptance verdict last. Any CRITICAL on the re-review goes to `DEVIATIONS.md` as a blocking `human-decision` and is escalated once, never looped. An untagged phase gets no review. Under an unattended permission mode that denies the review's worktree reads, the run logs the review as not run (`unverified`), lists it in the final report, and continues to the phase-verifier. diff --git a/plugins/implementation/evals/pb-regression-critical-stops/case.yaml b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml new file mode 100644 index 0000000000..5b7e80ee75 --- /dev/null +++ b/plugins/implementation/evals/pb-regression-critical-stops/case.yaml @@ -0,0 +1,2 @@ +schema_version: "1.1" +name: pb-regression-critical-stops diff --git a/plugins/implementation/evals/pb-regression-critical-stops/graders/blocking-human-decision.md b/plugins/implementation/evals/pb-regression-critical-stops/graders/blocking-human-decision.md new file mode 100644 index 0000000000..477c81fdaf --- /dev/null +++ b/plugins/implementation/evals/pb-regression-critical-stops/graders/blocking-human-decision.md @@ -0,0 +1,6 @@ +--- +type: regex +pattern: 'human[- ]decision[\s\S]{0,200}?(?` **and pushed**, never in the orchestrator's default checkout; the push keeps the worktree tip in sync with the remote, which `/source-control:pull-request create --pushed`'s HEAD-equals-remote precondition requires. When marking changes the plan, refresh its paste in the pull request body or the linked issue, before the next push or after the last push's CI run has finished, never right after a push (`/implementation:implement` Step 4 says why). +**Phase-boundary review before the verifier.** A phase tagged `Review: architecture`, `security`, `code-design` or `concurrency` runs `/implementation:implement` Step 4 "Phase-boundary review, before the ritual" (its tag-to-mode table and sequence) once its returns pass the build/test gate and before its `phase-verifier`, which stays last, and before the boundary commit. Each tag runs `/review:quality-gate --worktree --base --report-only`, adding `--quality-bar ` when the plan has a `Quality bar:` line; the mode is the tag's own name, except `concurrency`, which runs `slice concurrency`. The fix goes out as one fix brief under the phase's own fence (Gotchas "Dispatch one fix brief per head"), its return verified and gated like any other. The gate's `git -C ` reads run under the same permission posture this skill already relies on for its own `git -C ` reads of worker worktrees; no allow rule is added for them. When an unattended run's permission mode denies those reads, the review did not run: append a `discovery` entry marked `unverified` naming the phase and tag to `DEVIATIONS.md`, list that phase and tag as not reviewed in Step 5's report, continue to the `phase-verifier`, and never retry the review in another shape. + **Fresh-context verifier before marking a phase `[DONE]`:** in orchestrated runs the Step 4 item 1 verdict is *dispatched*. Dispatch this plugin's `phase-verifier` agent (subagent type `implementation:phase-verifier`; its `model` frontmatter binds it at least as capable as the implementer it checks) to check the phase's acceptance criteria against the actual diff, handed binary criteria and the diff with your rationale withheld, plus, when holdout tests are on, their path and run command. Frontmatter binds a floor and cannot follow a phase routed upward, so when the phase's implementer ran above that binding (the frontier alias for security-surface work, or a session model above it), pass that one verifier a per-invocation `model` at or above the model the implementer ran on (the checked-work row of the ladder in the marketplace's `docs/plugin-philosophy.md` "Model tiers"). Upward only. Inside a wave held at `opus` the verifier's `opus` binding already meets this rule; a phase that needs a frontier verifier runs alone. Where the phase's outcome is high-stakes and correlated blind spots are the risk, prefer a cross-vendor advisor for that verifier **when one is installed 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 same-vendor verifier as the stated fallback, never a route to a command that may not resolve (`docs/plugin-philosophy.md` "Fresh-eyes checkpoints"). It applies in every mode: dispatch it for any phase beyond a mechanical, behavior-preserving change, and, unless `verify_mechanical_phases` resolves `true`, verify a mechanical, behavior-preserving phase from the diff plus the build/test signal. The verifier does not substitute for implement Step 5's end gate. When the phase lies inside a declared planned breakage span, hand the verifier the `**Planned breakage:**` line with the criteria: a red step passes that criterion only when every failure lies in the declared paths or test filter, any other failure is a FAIL, and the phase that closes the span (Phase ``) must show the declared failures green. An inconclusive return is not a verdict (Gates). **A verdict holds for one head SHA.** Record each phase verdict, the verifier's or the orchestrator's mechanical-phase one, with the head SHA it judged, in the plan mark and in the boundary's `DEVIATIONS.md` entry when the run keeps that log. Under `orchestrator` the verifier judged an uncommitted tree, so record the verdict against the phase-boundary commit that captures that tree unchanged. A rebase, restack or any other new commit on the branch before the phase is marked `[DONE]` voids the verdict: rerun the build/test gate, verify again on the new head SHA, and mark `[DONE]` only on a verdict recorded against the current head. @@ -270,7 +272,7 @@ A clear gets the full Step 4 ritual, ending in the resume prompt; a resident bou | Phase is inline-routed or routing table absent, autonomous mode | Synthesize a worker row with no `Model` value (so `implementation:implementer`) and dispatch; the orchestrator never does volume edits | | Worker divergence report | Severity-assess per `/implementation:implement`'s "Step 3: Divergence Detection"; Major → `/planning:plan review` via the Skill tool when installed | | Every worker return | Verify against direct evidence, then invoke `/toolchain:check` via the Skill tool main-side (else the project's own build) | -| Phase sanity check passes | `/implementation:implement`'s "Step 4" ritual (its item-1 verifier gate applies in every mode; orchestrated runs dispatch it, see Phase boundaries) | +| Phase sanity check passes | A `Review:`-tagged phase first gets its phase-boundary review (Phase boundaries); then `/implementation:implement`'s "Step 4" ritual (its item-1 verifier gate applies in every mode; orchestrated runs dispatch it, see Phase boundaries) | | All phases complete | Invoke `/implementation:implement` via the Skill tool for its "Step 5: Completion and Handoff", handing it the PLAN.md path (none under `--items`; outcome verification is that step's `/verification:confirm` route). A source commit made after the last phase's `[DONE]` first gets the post-phase verifier; an autonomous run then gets the trail review | This skill does not edit inline (the inline cadence, commit discipline and mode context files are `/implementation:implement`'s) and does not create or revise plans. diff --git a/plugins/implementation/skills/implement/SKILL.md b/plugins/implementation/skills/implement/SKILL.md index a63f7262cf..0d40990a83 100644 --- a/plugins/implementation/skills/implement/SKILL.md +++ b/plugins/implementation/skills/implement/SKILL.md @@ -209,6 +209,17 @@ For non-trivial implementations (3+ steps), track each major logical block from In-session task state lives in the harness and does not survive a context clear. The durable mirror is the plan artifact plus handoff notes. Where these live: per the lifecycle artifact protocol ([`${CLAUDE_PLUGIN_ROOT}/reference/artifact-protocol.md`](${CLAUDE_PLUGIN_ROOT}/reference/artifact-protocol.md)). Plan progress is marked in the plan file in the memory slice (`//PLAN.md`, default `.work/`), handoff entries are timestamped notes in the memory slice's handoffs home. Both are self-ignored and never committed, so when marking changes the plan, refresh its paste in the pull request body or the linked issue. Write a pull-request body refresh before the push that carries the phase's commit, or after the last push's CI run has finished, never as its own edit right after a push: a body edit re-runs every workflow that triggers on `edited`, and one that lands while the push's run is still going adds a second run on the same head. +**Phase-boundary review, before the ritual.** A phase whose plan entry carries a `Review:` concern tag gets one review per tag, invoked via the Skill tool before item 1's verdict and item 3's commit; a phase with no tag gets none, and a `Review:` value outside this table (an evidence description) dispatches nothing. + +| `Review:` tag | Invocation | +|---|---| +| `architecture` | `/review:quality-gate architecture` | +| `security` | `/review:quality-gate security` | +| `code-design` | `/review:quality-gate code-design` | +| `concurrency` | `/review:quality-gate slice concurrency` | + +Each call passes `--base ` and `--report-only`, plus `--quality-bar ` only when the plan has a `Quality bar:` line, and carries no rationale: the gate's fresh-context reviewer gets the phase diff, the tag and that line. Sequence: review, fix inside the phase fence, one re-review against the same base, then item 1's verdict last. Any CRITICAL on the re-review goes to `DEVIATIONS.md` as a blocking `human-decision` entry and is escalated once; it never loops. Inputs, fixes, the re-review and the escalation: [context/phase-review.md](context/phase-review.md). + **At every phase boundary** (the phase's sanity check passes), perform this ritual atomically: 1. **Verify acceptance criteria, then mark plan progress**. Before setting the completed phase's tag to `[DONE]`, confirm the phase's acceptance criteria hold. Self-review is the floor; for any phase beyond a mechanical, behavior-preserving change (where an objective build/test/lint pass is verification enough), and for every phase, mechanical ones included, when `verify_mechanical_phases` resolves `true`, that verdict is rendered by an agent that did NOT produce the phase's changes: a fresh-context verifier handed binary criteria and the diff, withholding your rationale, or the cross-vendor option `/verification:confirm` names, never the producing context auditing itself, which converges on approval rather than detection. Resolve `verify_mechanical_phases` once per run, before the first phase boundary, from the default `false`, `${user_config.verify_mechanical_phases}` and the repository's `docs/conventions/implementation.yaml`, by the floor, root and invalid-value rules in [`${CLAUDE_PLUGIN_ROOT}/reference/config.md`](${CLAUDE_PLUGIN_ROOT}/reference/config.md), and report the value and the layer that supplied it in one line. Then set the tag to `[DONE]` in the plan artifact and tick its step boxes; keep any parent/roadmap documents that mirror phase status in sync in the same turn @@ -267,7 +278,7 @@ When all planned work is done: |-----------|--------| | Before writing first test | Invoke `/tdd:principles` via Skill tool (when installed) for test design guidance | | After each logical block | Invoke `/toolchain:check` via Skill tool (when the `toolchain` plugin is installed; else the project's own build) | -| At every phase boundary | Run the Step 4 ritual (plan marks + status + commit, then route the continuation last; a handoff entry with its resume prompt only on Step 4's own stop conditions or when the router routes there) | +| At every phase boundary | A `Review:`-tagged phase first gets its Step 4 phase-boundary review; then run the Step 4 ritual (plan marks + status + commit, then route the continuation last; a handoff entry with its resume prompt only on Step 4's own stop conditions or when the router routes there) | | Worker-routed phase or autonomous orchestration | Invoke `/implementation:implement-dispatch` via Skill tool | | Divergence detected (major) | Route back to the planning skill (invoke `/planning:plan review` via the Skill tool when installed) | | Technical question mid-implementation | Invoke `/discovery:research` via the Skill tool (when installed), otherwise disciplined multi-source research | diff --git a/plugins/implementation/skills/implement/context/phase-review.md b/plugins/implementation/skills/implement/context/phase-review.md new file mode 100644 index 0000000000..8766f14802 --- /dev/null +++ b/plugins/implementation/skills/implement/context/phase-review.md @@ -0,0 +1,53 @@ +# Phase-boundary review + +A plan phase tagged `Review: ` gets that review at its boundary, before the acceptance +verdict and the commit. SKILL.md Step 4 holds the tag-to-mode table; this file holds the rest. + +## Inputs + +- **Tags.** Each `Review:` line under the phase header names one concern. Run one gate call per + tag in the table, in the order the plan lists them. A value outside the table dispatches nothing; + say so in the boundary report. +- **Base.** `--base `: the previous phase-boundary commit, or for the first phase the commit + HEAD was on when Step 1 passed. The change set is everything since that commit, uncommitted + edits included. +- **Quality bar.** `--quality-bar ` copied from the plan's + `Quality bar:` line. With no such line, pass nothing; the gate rates as production and says so. +- **Report only.** `--report-only` always: the gate writes no findings file and asks nothing, and + this skill keeps the result. +- **No rationale.** Pass no plan reasoning, conversation or account of the change. The gate's + reviewer runs in fresh context on the diff, the tag and the quality-bar line alone. + +## Sequence + +1. **Review.** Run every tagged call against the same head, and record that head's SHA, or `dirty` + when the reviewed tree was uncommitted. +2. **Fix.** Fix each CRITICAL and IMPORTANT finding with an edit inside the phase's own files. + A finding whose fix needs a file outside the phase is a divergence (Step 3), not a fix. + SUGGESTION findings are reported, not fixed. With no CRITICAL or IMPORTANT finding, skip to + step 4. +3. **Re-review, once.** Rerun only the tags whose findings were fixed, with the same `--base` as + the first call. The whole phase diff stays in scope, so a fix that missed its finding still + rates it, and a defect the fix added rates too. The gate takes no list of earlier findings: + match its findings to the first call's by file and defect yourself. There is no second + re-review. +4. **Verdict.** Step 4 item 1 runs last, on the fixed tree. + +## A CRITICAL on the re-review + +Any CRITICAL the re-review rates stops the loop: the original finding the fix left in place, a +regression the fix introduced, or a defect the first call missed. A CRITICAL that matches no +first-call finding is not a new finding to fix: it is never logged as a `discovery` and never +gets a fix brief. + +- Append a `human-decision` entry marked blocking to `DEVIATIONS.md` in the memory slice: the tag, + the finding with its `file:line`, whether it repeats a first-call finding, the fix that was + tried, and the head SHA. +- Escalate once. Interactive: put the finding to the user with the options (fix another way, + accept the risk, replan). Unattended: the entry is the escalation; leave the phase unmarked and + park the run, which is blocked on a call only a person can make. +- Send no further fix brief and run no further review at this boundary, and never mark the phase + `[DONE]` over it. + +An IMPORTANT finding that survives the re-review goes into the boundary report and, when the run +keeps the log, a `discovery` entry; the phase continues.