feat(skills): add review-loop skill with opt-in gates

skills/developing-a-feature/SKILL.md

@@ -46,6 +46,13 @@ For sequential single-PR work, skip to Step 4. For multi-PR work, dispatch paral
 
 Record the choice in the state file's frontmatter as `sub_pr_approval: autonomous` or `sub_pr_approval: manual`. The fan-out skill reads this field at every sub-PR ripening to decide whether to gate on user approval. Default if the field is missing in an older state file: `autonomous` (preserves the original behaviour).
 
+**Sub-PR review-loop (multi-PR only).** Immediately after the approval-mode choice, ask a second `AskUserQuestion`: should each sub-PR run an automated review-loop and come back clean before it is self-merged? This is opt-in and independent of the approval mode.
+
+- **On** — at each sub-PR's ripening, the orchestrator runs `feature-dev-workflow:review-loop` against the open sub-PR before the existing `review`-skill pass and self-merge. A comment the loop wants to push back on does not pause the fan-out; it is logged as a bubble-up concern and surfaced at the wave checkpoint.
+- **Off** (default) — ripening is unchanged.
+
+Record the choice as `sub_pr_review_loop: on` or `sub_pr_review_loop: off`. The fan-out skill reads it at every ripening. Default if the field is missing in an older state file: `off` (preserves the original behaviour).
+
 ### 3. Set up the implementation environment
 
 - **Multi-PR (feature-branch model)** — `feature/<slug>` already exists, created and pushed by `feature-dev-workflow:planning-a-feature` and carrying the committed spec/plan/state. **Reuse it; never re-create it** off `origin/main` — that errors (`fatal: a branch named 'feature/<slug>' already exists`) and would orphan the planning artifacts. If planning already made the integration worktree at `.claude/worktrees/<slug>`, just `cd` into it. Otherwise attach one to the existing branch:
@@ -102,6 +109,10 @@ Record the choice in the state file's frontmatter as `sub_pr_approval: autonomou
 
 Sub-PRs into the feature branch are owned by `feature-dev-workflow:fanning-out-with-worktrees`, not this step.
 
+Once the PR to main is open — the single-PR PR, or the multi-PR integration PR — ask the user (via `AskUserQuestion`) whether to run an automated review-loop on it before handing off for external human review. If yes, **OPTIONAL SUB-SKILL:** `feature-dev-workflow:review-loop` against this PR; it drives the PR's automated (Copilot) review to clean. This is the interactive context, so a comment the loop wants to push back on pauses for the user. If no, hand off as-is.
+
+Run the loop on the **final** PR diff. The Step 7 teardown is the last commit on the branch, so a review run before it goes stale against what external reviewers actually see. If the teardown will land after a clean review, defer the loop until after the teardown commit (or re-run it afterward) — and when CI gates the teardown, that means running the loop once the teardown commit is pushed and green, not at PR-open.
+
 ### 7. Tear down the planning artifacts
 
 Delete the plan + state file once the work is genuinely done. The spec stays — it's the durable ADR. The plan and state file are scratch; leaving them committed past readiness pollutes the repo with stale operational state that future `grep`s have to wade through.

skills/fanning-out-with-worktrees/SKILL.md

@@ -93,16 +93,17 @@ When a sub-PR is ready (subagent reports `ready` and the relevant verification c
 
 Per sub-PR, in order:
 
-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. **Automated review-loop, if the state file's `sub_pr_review_loop` is `on`.** **OPTIONAL SUB-SKILL:** `feature-dev-workflow:review-loop` against the sub-PR number, in fan-out mode — it requests the automated (Copilot) review, waits for it in the background, and drives the PR to review-clean. In fan-out mode a comment the loop wants to push back on does **not** pause the fan-out: log it as a bubble-up concern in the `## Bubble-up log` (surfaced at the wave checkpoint by `feature-dev-workflow:reviewing-feature-progress`) and continue. Skip this step when `sub_pr_review_loop` is `off` or missing. It runs before the orchestrator's own review so the diff reviewed below is the post-automated-review diff that will actually merge.
+2. **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.
+3. **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 4-6. 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" / "<N> findings, none blocking" / specific concerns), the PR's title and diff size, and a note that closing sub-issue `#<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.
-3. **Merge into the feature branch.** First **push any local state-file commits to `origin/feature/<slug>`** — the merge happens on GitHub's side and lands on origin's current tip, so unpushed local commits make the post-merge `git -C <feature_worktree> pull --ff-only` fail with "Not possible to fast-forward" (local and origin have diverged; recover with `git rebase origin/feature/<slug>`). Keep local == origin at every merge boundary. Then `gh pr merge <num> --merge` (or `--squash` / `--rebase` per project preference), and `git -C <feature_worktree> fetch origin && git merge --ff-only origin/feature/<slug>` to bring the merge back.
-4. **Close the sub-issue.** `gh issue close <sub-issue>`. Sub-PRs into a non-default branch don't trigger `Fixes`/`Closes` — manual close is the workaround. The body's `Towards #<sub-issue>` keyword left the issue open precisely so the orchestrator can close it here; the cross-reference from the merge commit (which references `#<sub-pr>`, which references `#<sub-issue>`) is preserved automatically without a custom comment.
-5. **Update the state file.** Flip the row's status to `self-merged`. If the sub-PR was the realization of a contract (e.g. a pre-merge stub PR), flip the contract row's status to `locked` and fill in the `Realized in` pointer.
+4. **Merge into the feature branch.** First **push any local state-file commits to `origin/feature/<slug>`** — the merge happens on GitHub's side and lands on origin's current tip, so unpushed local commits make the post-merge `git -C <feature_worktree> pull --ff-only` fail with "Not possible to fast-forward" (local and origin have diverged; recover with `git rebase origin/feature/<slug>`). Keep local == origin at every merge boundary. Then `gh pr merge <num> --merge` (or `--squash` / `--rebase` per project preference), and `git -C <feature_worktree> fetch origin && git merge --ff-only origin/feature/<slug>` to bring the merge back.
+5. **Close the sub-issue.** `gh issue close <sub-issue>`. Sub-PRs into a non-default branch don't trigger `Fixes`/`Closes` — manual close is the workaround. The body's `Towards #<sub-issue>` keyword left the issue open precisely so the orchestrator can close it here; the cross-reference from the merge commit (which references `#<sub-pr>`, which references `#<sub-issue>`) is preserved automatically without a custom comment.
+6. **Update the state file.** Flip the row's status to `self-merged`. If the sub-PR was the realization of a contract (e.g. a pre-merge stub PR), flip the contract row's status to `locked` and fill in the `Realized in` pointer.
 
 ### 6. Checkpoint review, then dispatch wave N+1
 
@@ -129,7 +130,7 @@ When every wave is complete (every sub-issue closed, every row `self-merged`, ev
 - **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.
-- **Letting the state file go stale.** A resumed session reads it as ground truth. Update every row as reality moves; commit **and push** the state-file diff per phase transition — an unpushed local commit diverges the feature branch the moment the next sub-PR squash-merges on GitHub (see Step 5.3).
+- **Letting the state file go stale.** A resumed session reads it as ground truth. Update every row as reality moves; commit **and push** the state-file diff per phase transition — an unpushed local commit diverges the feature branch the moment the next sub-PR squash-merges on GitHub (see Step 5.4).
 
 ## Red flags