feat(skills/developing-a-feature): add post-review change choreography for history-sharing PRs

skills/developing-a-feature/SKILL.md

@@ -126,6 +126,16 @@ Run the loop on the **final** PR diff. The Step 7 teardown is the last commit on
 
 **The merge to main is the user's — always.** This step's terminal state is *ready-to-merge*: PR open, review loop clean (if run), teardown landed per Step 7. The orchestrator never runs `gh pr merge` on the single-PR feature PR or the integration PR. The only merges this workflow executes itself are the sub-PR merges governed by the state file's explicit `sub_pr_approval`/`sub_pr_target` configuration (`feature-dev-workflow:fanning-out-with-worktrees` Step 5) — configuration the user answered as specific questions, not inferred consent. A blanket autonomy grant — "do this all autonomously", "I'm going AFK" — authorizes everything up to and including the review loop and the teardown, and ends there. Merge-ish phrasing in passing ("merge it when you're done", "merge to the main worktree") is not the merge button either: it usually means "bring the result into the local checkout and verify it", and even when it doesn't, merging the feature to main is a fresh decision the user takes looking at the specific, final PR. Finish everything else, report the PR ready-to-merge, and stop — an open clean PR waiting for the user costs nothing; an unwanted merge to main costs a revert on the default branch.
 
+### Review-driven changes while several open PRs share history
+
+Multi-PR features put several open PRs on branches that contain each other's commits — sub-PRs stacked on a parent branch, feature-branch sub-PRs open concurrently, wave N+1 branched off what wave N merged. When a human reviews those PRs and streams findings, three rules govern every change:
+
+- **Fix where the code lives, not where the comment sits.** A reviewer flags code on whichever PR they happen to be reading; the branch whose PR *introduced* the code is where the fix commits. Establish ownership before editing (`git log -S '<snippet>' --format='%h %D %s' -- <file>`, then find the earliest stack branch containing that commit): a file being visible in a descendant's worktree is history inheritance, not ownership. Fixing at the point of visibility pollutes the descendant's diff with a change to code it didn't introduce, and lets the ancestor PR merge with the defect intact.
+- **Propagate by merging forward, never rebasing.** Once the origin branch is fixed and pushed, each descendant merges its updated parent in stack order and pushes. Rebasing rewrites SHAs under live review and orphans the reviewer's comment anchors.
+- **Propagate the decision, not just the code.** A review decision that changes approach, scope, or a contract touches more than the diff. Walk the full list at each decision, while the why is fresh: the spec and plan (amend), the state file (bubble-up entry + the head SHAs of every branch the propagation moved), affected issue bodies (`feature-dev-workflow:writing-github-issues` Step 2D), and every open PR body the change makes stale (`feature-dev-workflow:opening-a-pull-request`'s reconciliation). A wrap-up sweep is a backstop, not the mechanism.
+
+**Scripted state-file edits assert, then write.** Long review sessions update the state file's dense PR-table rows many times, so the edits get scripted. A guarded substitution ("replace the SHA if this pattern matches") fails silently: the no-op produces no diff hunk, and an eyeball pass can't see an absence. A scripted state-file edit is keyed on the row's stable cell (branch name or PR number), asserts that every targeted row matched and that exactly one token was replaced per row, aborts without writing when any assertion fails, and prints old → new per row after writing. The head SHAs it records come from the remote at write time (`git ls-remote origin <branch>` or `gh pr view <num> --json headRefOid`), never from memory of the push: the table describes what a resumed session will find on GitHub.
+
 ### 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.
@@ -145,6 +155,7 @@ The teardown does not change where the flow ends. In the models that end in a fi
 - **Mixing single-PR and multi-PR flows mid-feature.** Once the plan declares multi-PR, the feature-branch model is on. Don't quietly merge "just this small fix" directly to main while the feature branch is live — it skips external review on the integration PR and forks the work.
 - **Skipping `verification-before-completion` because "tests passed in my package".** The full test suite runs the whole project because cross-package wiring breaks on edits that look local.
 - **Letting the state file drift from reality.** A resumed session reads the state file as ground truth. Update it on every transition (worktree assigned, PR opened, sub-PR self-merged, phase changed, feature shipped).
+- **Fixing a review finding on the branch where it was reported.** The reviewer's comment sits on whichever PR they were reading; the fix belongs on the branch whose PR introduced the code, merged forward through the descendants (see §Review-driven changes while several open PRs share history).
 - **Re-implementing fan-out logic inline.** Parallel dispatch, multi-wave ordering, the watch loop, per-sub-PR self-review and self-merge — all of that is in `feature-dev-workflow:fanning-out-with-worktrees`. Don't paste it into the dispatch prompt or the developing-a-feature flow; reference the sub-skill instead.
 - **Merging the final PR to main yourself.** The single-PR feature PR and the integration PR are external-review surfaces; the user holds that merge button. No blanket autonomy grant covers it — the workflow's terminal state is ready-to-merge, not merged (see the merge guard in Step 6).
 
@@ -167,3 +178,6 @@ The teardown does not change where the flow ends. In the models that end in a fi
 | "The redesign is in the PR description, the issue can stay as-is"     | The PR carries the design; the issue's thread is the decision trail — what its stated approach/criteria were and why they changed. A divergence from the issue gets recorded on it while it's fresh (`feature-dev-workflow:writing-github-issues` Step 2D). |
 | "It auto-closes on merge, so a stale issue body doesn't matter"      | A closed issue and its comments are the durable record people read later. A stale body misleads and the 'why' is gone. Reconcile and record the decision before the merge (`feature-dev-workflow:writing-github-issues` Step 2D). |
 | "Design belongs in the PR, so I leave the issue's Approach alone"    | That bars dumping new line-level design into the issue — not keeping its stated content true. A now-false Approach is reconciled and the decision recorded (`feature-dev-workflow:writing-github-issues` Step 2D). |
+| "The function is right here in this worktree, I'll fix it here"      | Visibility is history inheritance, not ownership. Trace which PR introduced the code and fix there, then merge forward — or the ancestor merges with the defect intact. |
+| "My replace script is guarded, and I'll eyeball the diff after"      | A guarded substitution that doesn't match no-ops silently: no hunk appears, and an eyeball pass can't see an absence. Assert every row matched before writing; abort on any anomaly. |
+| "I just pushed these branches, I know what the heads are"            | Record what the remote actually has (`git ls-remote` / `gh pr view --json headRefOid`), not what you remember pushing. The state file describes what a resumed session will find on GitHub. |