diff --git a/skills/fanning-out-with-worktrees/SKILL.md b/skills/fanning-out-with-worktrees/SKILL.md index f8b3549..fbae2b4 100644 --- a/skills/fanning-out-with-worktrees/SKILL.md +++ b/skills/fanning-out-with-worktrees/SKILL.md @@ -93,7 +93,10 @@ When a sub-PR is ready (subagent reports `ready` and the relevant verification c Per sub-PR, in order: -1. **Review.** **REQUIRED SUB-SKILL:** the `review` skill — invoke it against the sub-PR number to walk the diff with full PR context. This review is weaker than external review (which lands at the integration PR) but stronger than nothing; it catches issues that would otherwise pile onto the integration PR reviewer. Beyond bugs, check the diff against the `## Conventions` block: does its layout, naming, and vocabulary match the block and the sibling PRs already merged? A convention violation caught here is one the integration reviewer never sees. +1. **Two-stage review — spec-compliance first, then code quality.** Both stages are the `review` skill, run against the sub-PR number with full PR context, scoped differently. This review is weaker than external review (which lands at the integration PR) but stronger than nothing; it catches issues that would otherwise pile onto the integration PR reviewer. The order is a gate, not a preference: a PR that doesn't yet implement its sub-issue is going back regardless, so quality findings on code about to change are wasted effort and noise. One blended "looks good" pass is the failure this split exists to prevent. + 1. **Spec-compliance pass (the gate).** **REQUIRED SUB-SKILL:** the `review` skill, scoped to *intent*: walk the sub-issue's acceptance criteria one by one and map each to real code in the diff **and** a test that exercises it. Confirm nothing is missing and nothing extra was built — unrequested scope is a finding, not a bonus (it may collide with a sibling PR or belong elsewhere). This pass must come back clean before the quality pass starts. + 2. **Code-quality pass.** **REQUIRED SUB-SKILL:** the `review` skill, scoped to *craft*: bugs the criteria didn't name, edge cases, error handling, test quality, simplification/reuse. Beyond bugs, check the diff against the `## Conventions` block — does its layout, naming, and vocabulary match the block and the sibling PRs already merged? A convention violation caught here is one the integration reviewer never sees. + - **Fix-loop.** When either pass returns findings, the orchestrator does **not** fix them in place — it routes them back to the worktree subagent via `SendMessage` (the author has the context, and silent orchestrator fixes rob the parallel agents of shared understanding), then re-runs the *same* pass against the changed surface. Spec-compliance findings re-run the spec pass; only once it is clean does the quality pass run. Repeat until both passes are clean. The worktree subagent fixes; the orchestrator reviews — never the reverse. 2. **Approval gate, per the state file's `sub_pr_approval` mode.** Every gate covers the **bundle**: merge + sub-issue close + state-file update. The close is bodyless (no `--comment` flag) — GitHub automatically cross-references the sub-issue from the merge commit via the sub-PR's body keyword, so no custom comment is needed and there's no "specific body about to land" for the close mutation. - **`autonomous`** (default) — proceed straight through the bundle in steps 3-5. The user opted into the mechanical bundle (review → merge → bodyless close → state update) in `feature-dev-workflow:developing-a-feature` Step 2. - **`manual`** — pause and ask the user for explicit approval before the bundle. The prompt MUST surface: a one-line summary of the review findings ("review clean" / " findings, none blocking" / specific concerns), the PR's title and diff size, and a note that closing sub-issue `#` follows the merge. Wait for an explicit yes. On push-back, route the concern back to the worktree subagent via `SendMessage` instead of merging. @@ -120,6 +123,9 @@ When every wave is complete (every sub-issue closed, every row `self-merged`, ev - **Dispatching without the conventions block.** Contracts make the work compile; conventions make it cohere. Hand a subagent contracts but no `## Conventions` and it invents its own layout and names — the drift surfaces at the integration PR when it's expensive. If the plan has no conventions block, go back to `planning-a-feature` Step 6. - **Skipping wave dependencies.** Consumers dispatched before producers' contracts are realized produce code against an imagined shape. Dispatch wave N+1 only after wave N is fully self-merged and contracts locked. - **Self-merging without orchestrator review.** The integration PR is where external review lands, but sub-PRs still need a review pass before going into the feature branch. Skipping it dumps issues onto the integration PR reviewer. +- **Collapsing the two stages into one "looks good" pass.** Spec-compliance and code-quality answer different questions — "does it do the job" vs "is it well-built". Reviewing them together lets a momentum-driven skim pass a PR that quietly misses an acceptance criterion or builds unrequested scope. Run the spec gate to clean first, then quality. +- **Quality-reviewing before spec-compliance is clean.** Polishing code that's about to change because it doesn't meet the spec wastes the pass and buries the real finding in nits. The spec pass is a gate; the quality pass waits behind it. +- **Orchestrator fixing review findings in place.** The worktree subagent wrote the code and holds the context; it fixes, then the orchestrator re-reviews. A silent orchestrator fix skips the author and erodes the shared understanding the parallel agents run on. - **Letting the worktree subagent review its own PR.** A subagent reviewing the code it just wrote has the same blind spots in review that it had in implementation. The orchestrator owns sub-PR review precisely because it didn't write the code. - **Letting a bubble-up concern die in one subagent.** Propagation is the whole point of the watch loop. - **Forgetting to manually close the sub-issue.** The keyword doesn't fire on non-default-branch merges; the issue page silently shows "open" even though the work shipped. @@ -134,5 +140,7 @@ When every wave is complete (every sub-issue closed, every row `self-merged`, ev | "The subagent will figure out the worktree on its own" | Create the worktree yourself and embed `cd && pwd && git branch --show-current` in the dispatch. | | "Self-review feels redundant, the integration PR catches everything" | Integration PR reviewer can't catch every issue across 5 sub-PRs in one sitting. Catch them per PR. | | "The subagent already verified the diff, no need for the orchestrator to review again" | Verification (tests pass) is not review (does the diff express intent cleanly). Different signals. | +| "Tests pass and the diff looks clean — one review pass is enough" | Spec-compliance and quality are different questions. The spec gate runs to clean first, then quality; one blended pass skims past a missed acceptance criterion. | +| "The spec pass found a gap but I'll quality-review now to save a round-trip" | Quality findings on code about to change are noise. Send the gap back, re-run the spec pass to clean, then quality. | | "Both sub-PRs pass CI, so the naming difference between them is fine" | Contract checks don't see naming. Two schemes for one kind of thing is drift — reconcile it at the wave, not the integration PR. | | "I'll close the sub-issue once the integration PR merges" | Issues that read "open" while their work has shipped clutter the triage view. Close manually at self-merge. |