diff --git a/README.md b/README.md index 25c3295..529cac1 100644 --- a/README.md +++ b/README.md @@ -54,6 +54,7 @@ holds only its own slice of context instead of the whole feature at once. | `reviewing-feature-progress` | Orchestration checkpoints: between fan-out waves, and before the integration PR. | | `testing-a-feature` | Writing tests for any non-trivial change. Decides the assertion shape (black-box against the contract). | | `opening-a-pull-request` | About to `gh pr create`/`edit`. Draft and ready body templates, issue-linking keywords. | +| `maintaining-architectural-coherence` | Work split across PRs/agents/waves must read as one author. Invoked when agreeing conventions before parallel work, and when reviewing the merged union for structural, interface, naming, and vocabulary drift. | ## How it works diff --git a/skills/fanning-out-with-worktrees/SKILL.md b/skills/fanning-out-with-worktrees/SKILL.md index 068fa5f..f8b3549 100644 --- a/skills/fanning-out-with-worktrees/SKILL.md +++ b/skills/fanning-out-with-worktrees/SKILL.md @@ -12,32 +12,25 @@ description: When you're the orchestrator for multi-PR feature work and these prerequisites are met: -- The `feature/` integration branch and the main feature worktree exist (set up by `feature-dev-workflow:developing-a-feature` before - invoking this skill). -- The plan (`docs/superpowers/plans/--plan.md`) has a `## Contracts` section with a Realization strategy - per row. +- The `feature/` integration branch and the main feature worktree exist (set up by `feature-dev-workflow:developing-a-feature` before invoking this skill). +- The plan (`docs/superpowers/plans/--plan.md`) has a `## Contracts` section with a Realization strategy per row. +- The plan also has a `## Conventions` block (directory layout, naming scheme, locked vocabulary) — it's mandatory dispatch context for every subagent (Step 2), so the fan-out can't start without it. - The state file (`docs/superpowers/states/--state.md`) has rows for each sub-issue. Skip for single-PR features — there's no fan-out, just one branch off main. ## Why this exists -Parallel fan-out compounds in complexity quickly: contract handoff, isolation, wave dependencies, propagation, state -tracking, per-PR ripening. Wrapping the choreography in one skill keeps `feature-dev-workflow:developing-a-feature` focused on the -single-vs-multi decision and the integration-PR endgame, and gives any future use case (parallel migration work, -parallel refactor, cross-module sweep) a reusable orchestration entry point. +Parallel fan-out compounds in complexity quickly: contract handoff, isolation, wave dependencies, propagation, state tracking, per-PR ripening. Wrapping the choreography in one skill keeps `feature-dev-workflow:developing-a-feature` focused on the single-vs-multi decision and the integration-PR endgame, and gives any future use case (parallel migration work, parallel refactor, cross-module sweep) a reusable orchestration entry point. ## Workflow ### 1. Plan the waves -Look at the plan's `## Contracts` table. Sub-PRs that are contract **producers** go in the FOUNDATIONAL wave; sub-PRs -that are contract **consumers** go in subsequent waves, keyed by when their contract is realized: +Look at the plan's `## Contracts` table. Sub-PRs that are contract **producers** go in the FOUNDATIONAL wave; sub-PRs that are contract **consumers** go in subsequent waves, keyed by when their contract is realized: -- `data-only` and `pre-merge stub PR` realizations → consumers can start as soon as the producer's stub PR has merged - into the feature branch (or the data-only contract is documented). -- `stub-on-producer-branch` → consumers branch from the producer's branch (not the feature branch); dispatch is timed - to after the producer's PR is open with the stub in place. +- `data-only` and `pre-merge stub PR` realizations → consumers can start as soon as the producer's stub PR has merged into the feature branch (or the data-only contract is documented). +- `stub-on-producer-branch` → consumers branch from the producer's branch (not the feature branch); dispatch is timed to after the producer's PR is open with the stub in place. Sub-PRs with no contract dependency → all in one wave. @@ -51,148 +44,86 @@ For each sub-PR in the wave, the orchestrator creates the worktree first: git worktree add .claude/worktrees/-- -b ``` -`` is `feature/` for default sub-PRs, `feature/` after the stub PR merged (for `pre-merge stub -PR` consumers), or the producer's branch (for `stub-on-producer-branch` consumers). +`` is `feature/` for default sub-PRs, `feature/` after the stub PR merged (for `pre-merge stub PR` consumers), or the producer's branch (for `stub-on-producer-branch` consumers). Then dispatch one subagent per sub-PR. **REQUIRED SUB-SKILL:** `superpowers:dispatching-parallel-agents`. Each dispatch prompt MUST include: -1. **Isolation verification as the first action.** `cd && pwd && git branch --show-current` — the - subagent confirms it's on the sub-branch in the right worktree before any edit. Commits land on the wrong branch - otherwise. -2. **Context handoff.** State file path, plan path, spec path, the issue number it's working, and the relevant - contract row(s) (Name + Producer + Consumer + Shape + Realization). The subagent implements **against the - contract** — it does not re-discover or re-design it. +1. **Isolation verification as the first action.** `cd && pwd && git branch --show-current` — the subagent confirms it's on the sub-branch in the right worktree before any edit. Commits land on the wrong branch otherwise. +2. **Context handoff.** State file path, plan path, spec path, the issue number it's working, the relevant contract row(s) (Name + Producer + Consumer + Shape + Realization), **and the plan's `## Conventions` block**. The subagent implements **against the contract and the conventions** — it does not re-discover or re-design either, and it does not invent its own directory layout or naming scheme. A subagent handed contracts but not conventions will name and structure locally, and the merged feature reads as written by a committee (see `feature-dev-workflow:maintaining-architectural-coherence`). 3. **Implementation skills.** `superpowers:test-driven-development` + `feature-dev-workflow:testing-a-feature` for every change. -4. **PR completion.** When the implementation is done and verified, the subagent invokes `feature-dev-workflow:opening-a-pull-request` - with `--base feature/` and `Towards #` in the body (NOT `Fixes` / `Closes` — those don't fire on - non-default-branch merges; `Towards` is the explicit "keep this issue open until the orchestrator closes it - manually" keyword). The subagent reports the PR URL back to the orchestrator. +4. **PR completion.** When the implementation is done and verified, the subagent invokes `feature-dev-workflow:opening-a-pull-request` with `--base feature/` and `Towards #` in the body (NOT `Fixes` / `Closes` — those don't fire on non-default-branch merges; `Towards` is the explicit "keep this issue open until the orchestrator closes it manually" keyword). The subagent reports the PR URL back to the orchestrator. ### 3. Update the state file as subagents start work -As each subagent surfaces its worktree path and branch, the orchestrator fills in the row in the state file's -`## PRs / worktrees` table. When a subagent opens its draft PR, the orchestrator fills in the PR column with the -base ref (`# → feature/`) and flips status to `draft`. +As each subagent surfaces its worktree path and branch, the orchestrator fills in the row in the state file's `## PRs / worktrees` table. When a subagent opens its draft PR, the orchestrator fills in the PR column with the base ref (`# → feature/`) and flips status to `draft`. A stale row is worse than no row — a resumed session reads the state file as ground truth. ### 4. Watch loop -While subagents run, the orchestrator is the integration point — not an idle waiter. Watch for concerns that bubble -up from any subagent and propagate the resolution across every subagent the concern touches. Silent divergence is -the failure mode this watch loop exists to prevent. +While subagents run, the orchestrator is the integration point — not an idle waiter. Watch for concerns that bubble up from any subagent and propagate the resolution across every subagent the concern touches. Silent divergence is the failure mode this watch loop exists to prevent. Categories of concerns to watch for: -- **Contract drift.** A row in the `## Contracts` table needs to shift (reviewer feedback, an edge case the producer - hit). Pause every affected consumer, update the plan's row, propagate. -- **Spec ambiguity surfaced mid-implementation.** A subagent hits a case the spec didn't cover. Surface to the user, - get a decision, amend the spec (or add a note to the plan), and propagate to every subagent whose scope touches - the same surface. -- **Discovered cross-PR dependency.** A subagent finds it needs a helper, type, or behaviour from another PR that the - plan didn't enumerate. Decide whether the helper becomes a new contract (file an issue, add a contract row), - inlines into the current PR, or is something one of the other subagents is already producing. -- **Test failure in shared infrastructure.** One subagent breaks a test that another subagent's PR relies on. - Coordinate the fix into the right PR; don't let both subagents fix it independently. -- **External dependency change.** A dependency version bump, a library update, an API shift — affects every running - subagent. -- **Resource conflict.** Two subagents both editing the same file or symbol. Re-scope one to avoid the collision, or - serialize the work. +- **Contract drift.** A row in the `## Contracts` table needs to shift (reviewer feedback, an edge case the producer hit). Pause every affected consumer, update the plan's row, propagate. +- **Naming / layout divergence.** A subagent introduces a path, package, file-naming scheme, identifier pattern, or fixture name that doesn't match the `## Conventions` block or what a sibling PR already shipped. Reconcile before the merge — pick the convention (update the block if the new choice is better), and propagate to every subagent on the divergent pattern. Naming drift is invisible to contract checks; catching it here is far cheaper than at the integration PR. +- **Spec ambiguity surfaced mid-implementation.** A subagent hits a case the spec didn't cover. Surface to the user, get a decision, amend the spec (or add a note to the plan), and propagate to every subagent whose scope touches the same surface. +- **Discovered cross-PR dependency.** A subagent finds it needs a helper, type, or behaviour from another PR that the plan didn't enumerate. Decide whether the helper becomes a new contract (file an issue, add a contract row), inlines into the current PR, or is something one of the other subagents is already producing. +- **Test failure in shared infrastructure.** One subagent breaks a test that another subagent's PR relies on. Coordinate the fix into the right PR; don't let both subagents fix it independently. +- **External dependency change.** A dependency version bump, a library update, an API shift — affects every running subagent. +- **Resource conflict.** Two subagents both editing the same file or symbol. Re-scope one to avoid the collision, or serialize the work. How to propagate the resolution: -- **Subagent still running** → `SendMessage` with the subagent's id to push the resolution with full context. The - subagent resumes with the update applied. +- **Subagent still running** → `SendMessage` with the subagent's id to push the resolution with full context. The subagent resumes with the update applied. - **Subagent finished, PR still open** → re-dispatch a focused follow-up with the PR number and the specific change. - **Subagent not yet dispatched (later wave)** → update its dispatch prompt's context block before launching. -Append a dated entry to the state file's `## Bubble-up log` (newest at top) naming the concern, the resolution, and -the propagation path used. The orchestrator owns propagation; a concern raised by one subagent and not propagated to -the others is how this whole pattern fails. +Append a dated entry to the state file's `## Bubble-up log` (newest at top) naming the concern, the resolution, and the propagation path used. The orchestrator owns propagation; a concern raised by one subagent and not propagated to the others is how this whole pattern fails. ### 5. Per-sub-PR ripening — all orchestrator-driven -When a sub-PR is ready (subagent reports `ready` and the relevant verification commands pass), the **orchestrator** -takes over for the rest of the lifecycle. Every action below — review, merge, sub-issue close, state-file update — -is the orchestrator's, not the worktree subagent's. Three reasons this is the right division of labour: +When a sub-PR is ready (subagent reports `ready` and the relevant verification commands pass), the **orchestrator** takes over for the rest of the lifecycle. Every action below — review, merge, sub-issue close, state-file update — is the orchestrator's, not the worktree subagent's. Three reasons this is the right division of labour: -- **Review independence.** The subagent that wrote the code is the wrong reviewer for the same code; the - orchestrator's distance from the implementation is the whole point. -- **Global view.** Only the orchestrator holds the merge-order context (which contract rows are `locked`, which - sibling PRs are still in flight, which wave we're in). A subagent merging on its own would commit to ordering it - can't see. -- **Worktree topology.** Subagents live in their per-sub-PR worktrees; only the orchestrator's main feature worktree - has `feature/` checked out, so the merge naturally happens on the orchestrator's side. +- **Review independence.** The subagent that wrote the code is the wrong reviewer for the same code; the orchestrator's distance from the implementation is the whole point. +- **Global view.** Only the orchestrator holds the merge-order context (which contract rows are `locked`, which sibling PRs are still in flight, which wave we're in). A subagent merging on its own would commit to ordering it can't see. +- **Worktree topology.** Subagents live in their per-sub-PR worktrees; only the orchestrator's main feature worktree has `feature/` checked out, so the merge naturally happens on the orchestrator's side. 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. -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. -3. **Merge into the feature branch.** First **push any local state-file commits to `origin/feature/`** — the - merge happens on GitHub's side and lands on origin's current tip, so unpushed local commits make the post-merge - `git -C pull --ff-only` fail with "Not possible to fast-forward" (local and origin have - diverged; recover with `git rebase origin/feature/`). Keep local == origin at every merge boundary. Then - `gh pr merge --merge` (or `--squash` / `--rebase` per project preference), and - `git -C fetch origin && git merge --ff-only origin/feature/` to bring the merge back. -4. **Close the sub-issue.** `gh issue close `. Sub-PRs into a non-default - branch don't trigger `Fixes`/`Closes` — manual close is the workaround. The body's `Towards #` - keyword left the issue open precisely so the orchestrator can close it here; the cross-reference from the merge - commit (which references `#`, which references `#`) 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. +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. +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. +3. **Merge into the feature branch.** First **push any local state-file commits to `origin/feature/`** — the merge happens on GitHub's side and lands on origin's current tip, so unpushed local commits make the post-merge `git -C pull --ff-only` fail with "Not possible to fast-forward" (local and origin have diverged; recover with `git rebase origin/feature/`). Keep local == origin at every merge boundary. Then `gh pr merge --merge` (or `--squash` / `--rebase` per project preference), and `git -C fetch origin && git merge --ff-only origin/feature/` to bring the merge back. +4. **Close the sub-issue.** `gh issue close `. Sub-PRs into a non-default branch don't trigger `Fixes`/`Closes` — manual close is the workaround. The body's `Towards #` keyword left the issue open precisely so the orchestrator can close it here; the cross-reference from the merge commit (which references `#`, which references `#`) 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. ### 6. Checkpoint review, then dispatch wave N+1 When every sub-PR in wave N is `self-merged` AND every contract tied to wave N's producers is `locked`: -1. **REQUIRED SUB-SKILL:** `feature-dev-workflow:reviewing-feature-progress` — run the alignment checkpoint at the wave boundary. Drift - accumulated across wave N is cheapest to fix here, not at the integration PR. If the checkpoint surfaces gaps, - address them (follow-up sub-PR in wave N, plan/issue refinement, or both) before moving on. -2. Once the checkpoint is clean, dispatch wave N+1. Wave N+1's subagents pick up the realized contracts because the - orchestrator updated the state file's `## Contracts` table. +1. **REQUIRED SUB-SKILL:** `feature-dev-workflow:reviewing-feature-progress` — run the alignment checkpoint at the wave boundary. Drift accumulated across wave N is cheapest to fix here, not at the integration PR. If the checkpoint surfaces gaps, address them (follow-up sub-PR in wave N, plan/issue refinement, or both) before moving on. +2. Once the checkpoint is clean, dispatch wave N+1. Wave N+1's subagents pick up the realized contracts because the orchestrator updated the state file's `## Contracts` table. Repeat Steps 2 → 6 for each wave. ### 7. All waves complete → hand back -When every wave is complete (every sub-issue closed, every row `self-merged`, every contract `locked`), update the -state file's frontmatter `status:` to `review` and return control to `feature-dev-workflow:developing-a-feature`. The next step is the -integration PR (`feature/` → `main` with `Closes #`) which `feature-dev-workflow:developing-a-feature` owns. +When every wave is complete (every sub-issue closed, every row `self-merged`, every contract `locked`), update the state file's frontmatter `status:` to `review` and return control to `feature-dev-workflow:developing-a-feature`. The next step is the integration PR (`feature/` → `main` with `Closes #`) which `feature-dev-workflow:developing-a-feature` owns. ## Anti-patterns -- **Dispatching parallel agents without contracts.** "Two subagents on these two PRs" with no contract = divergent - implementations that block at integration. If the plan didn't define a contract, don't dispatch — go back to - `feature-dev-workflow:planning-a-feature` Step 6 and add one. -- **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. -- **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. +- **Dispatching parallel agents without contracts.** "Two subagents on these two PRs" with no contract = divergent implementations that block at integration. If the plan didn't define a contract, don't dispatch — go back to `feature-dev-workflow:planning-a-feature` Step 6 and add one. +- **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. +- **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). +- **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). ## Red flags @@ -203,4 +134,5 @@ integration PR (`feature/` → `main` with `Closes #`) which `featur | "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. | +| "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. | diff --git a/skills/maintaining-architectural-coherence/SKILL.md b/skills/maintaining-architectural-coherence/SKILL.md new file mode 100644 index 0000000..4d43c70 --- /dev/null +++ b/skills/maintaining-architectural-coherence/SKILL.md @@ -0,0 +1,72 @@ +--- +name: maintaining-architectural-coherence +description: + Use when work is split across multiple PRs, agents, or waves and the merged + result must read as if one author wrote it — when agreeing conventions before + parallel work starts, when dispatching it, and when reviewing what merged; or + when a structural, interface, naming, or vocabulary inconsistency surfaces as + a smell. +--- + +# maintaining-architectural-coherence + +## Overview + +Work split across contributors drifts. Each piece is built in isolation against its own slice of the problem, so each contributor optimizes locally — and the union reads as written by a committee: two ways to structure the same thing, two error conventions, an interface shaped one way here and another there. **Coherence is the property that the whole reads as one author would have written it.** + +The trap: coherence is invisible to per-piece checks. Every PR passes its own review and CI; every contract and acceptance criterion is satisfied; the drift lives only in the *relationships between* pieces. So it has to be designed in up front and audited across the union — neither of which a single diff review does. In decomposed work that is the orchestrator's job, because the orchestrator is the only role that sees the whole surface. + +## The dimensions of coherence + +Naming is the most visible dimension, not the only one. Read the union across all of them: + +- **Structure / layout** — parallel things live in parallel places; new files and packages land by one rule; no piece sits flat where a sibling nests. +- **Interfaces / API shape** — sibling functions, types, and options share a shape; return / error / option conventions match; no asymmetric outlier without a recorded reason. +- **Layering / separation** — the same concern is handled in the same layer across pieces (validation, auth, persistence, config access); no piece reaches across a boundary the others respect. +- **Naming** — one scheme per kind of thing: files, directories, packages, types, functions, variables, constants, fixtures, tests. Plus the naming firewall below. +- **Vocabulary** — the domain's locked terms, with no synonyms for the same concept. +- **Idiom** — the same problem solved the same way each time: error handling, logging, config access, test layout. + +## The discipline + +1. **Agree the conventions before dispatch.** Pin the decisions every piece inherits — the dimensions above, for the surfaces this work touches — in one place (in this workflow, a plan's `## Conventions` block). Most are already implied by the existing codebase; the work is *writing them down* so N contributors reach the same answer instead of guessing. A convention you can't state in one line isn't agreed yet — agree it before dispatch, not after merge. +2. **Hand them down.** Every dispatched piece gets the conventions and builds against them, not against its own invention. +3. **Audit the union, not the pieces.** Read what all the pieces shipped *together* and ask: would one author have written it this way? Inconsistency is a **signal**, not cosmetic debt — two schemes for one kind of thing, an asymmetric interface, or a structure that's fine for one file but bakes in a no-growth shape usually means a better structure is waiting. Rework toward it now; it is far cheaper than after an external reviewer hits it. + +## The naming firewall + +**Organizing labels are navigational only. They never become identifiers.** + +`Flow 1`, `Phase 2`, `Wave 3`, `Part A` are scaffolding the plan uses to sequence work and the tracker uses to group it. They are an artifact of *how* the work was decomposed — not a property of the thing being built. The moment that ordinal lands in a directory name, a runtime id, a function, a package, or a sentinel string, it has leaked: a reader of the code now has to know the planning history to parse it, and the name lies as soon as the plan is renumbered or the thing is reused. + +| Surface | `Flow N` allowed? | Name it instead by | +| --- | --- | --- | +| Epic / sub-issue title prose | yes (a clean prefix: `Flow 2: investigation happy path`) | — | +| Plan / spec narrative and section headings | yes (it indexes the decomposition) | — | +| Branch / PR title | no | the capability (`feature/investigation-e2e`) | +| Directory / file name | no | the content or behaviour | +| Package / type / function / variable / constant | no | what it is or does | +| Fixture / scenario name | no | the state or behaviour it encodes | +| Test name | no | the behaviour under test | +| Sentinel / marker string, config key, env var | no | the thing it marks | + +The firewall is a one-way membrane: a label may *appear in* a title or a spec heading, but nothing downstream may *inherit* it as a name. When you catch yourself about to write `flow2-fixture`, `TestFlow2…`, or `…_FLOW2_…`, that is the leak — name the thing for what it is. + +## Quick reference + +| Dimension | Smell | Fix | +| --- | --- | --- | +| Structure | one piece flat, a sibling nested | one layout rule | +| Interface | sibling APIs shaped differently | align, or record why it differs | +| Layering | one concern handled in different layers | one layer for the concern | +| Naming | two schemes for one kind of thing | one scheme | +| Naming | an organizing label (`Flow N`) in an identifier | name by what the thing is | +| Vocabulary | synonyms for one concept | the locked term | +| Idiom | the same problem solved several ways | one way | + +## Common mistakes + +- **Declaring "all clean" on per-piece checks.** Contracts, CI, and acceptance criteria never see cross-piece coherence. Read the union before claiming clean. +- **Deferring inconsistency as cosmetic.** It is a signal that a better structure exists; rework toward it rather than shipping the drift. +- **Dispatching parallel work with no agreed conventions.** Each piece invents its own layout and shape; the drift surfaces at integration, where it is expensive. +- **Auditing only names.** Structure, interfaces, layering, vocabulary, and idiom drift too — and an inconsistent interface costs more than an inconsistent name. diff --git a/skills/planning-a-feature/SKILL.md b/skills/planning-a-feature/SKILL.md index 106617a..4c37101 100644 --- a/skills/planning-a-feature/SKILL.md +++ b/skills/planning-a-feature/SKILL.md @@ -9,9 +9,7 @@ description: ## When to invoke -At the start of any feature larger than a one-off fix or refactor. If you're about to brainstorm, write a spec, file -a tracking issue, or write an implementation plan, this skill is the choreography that sequences those tools so the -artifacts land in the right order. +At the start of any feature larger than a one-off fix or refactor. If you're about to brainstorm, write a spec, file a tracking issue, or write an implementation plan, this skill is the choreography that sequences those tools so the artifacts land in the right order. Skip for: - One-off bug fixes (file a `bug` issue directly via `feature-dev-workflow:writing-github-issues`). @@ -20,50 +18,32 @@ Skip for: ## Why this exists -A senior engineering team's pre-implementation pipeline is **visible, fast, and parallelizable** — not a linear queue -of "brainstorm → spec → tickets → plan → code" run by one person in their head. Each step is explicit, hands off -between sub-skills cleanly, and surfaces the parallelism decision as a first-class output so implementation can fan -out instead of dragging. +A senior engineering team's pre-implementation pipeline is **visible, fast, and parallelizable** — not a linear queue of "brainstorm → spec → tickets → plan → code" run by one person in their head. Each step is explicit, hands off between sub-skills cleanly, and surfaces the parallelism decision as a first-class output so implementation can fan out instead of dragging. ## Workflow ### 1. Brainstorm intent -**REQUIRED SUB-SKILL:** Invoke `superpowers:brainstorming` to nail down intent, scope, and design choices before any -artifact is written. The brainstorming skill explores the problem — it does **not** author files. +**REQUIRED SUB-SKILL:** Invoke `superpowers:brainstorming` to nail down intent, scope, and design choices before any artifact is written. The brainstorming skill explores the problem — it does **not** author files. ### 2. Author the spec — and any cross-cutting ADR -**Spec.** Capture the brainstorm's conclusions in a spec at `docs/superpowers/specs/YYYY-MM-DD--design.md`. The -spec is the feature's design record — WHAT we're building and WHY, with goals, non-goals, design choices, risks, -alternatives considered. It does **not** enumerate PRs, commits, or parallelism contracts (those live in the plan, -see step 6). Slug is short ("multi-tenant-profiles", not "implement-multi-tenant-profile-system"); date is the -conception date. - -**ADR (only if the brainstorm surfaced a project-wide architectural decision).** A spec captures feature-scoped design. -An ADR captures **cross-cutting** decisions that affect multiple features — choosing one architectural approach over -another for reasons that future readers will need. Examples of ADR-worthy decisions: introducing a new IPC -channel, picking a state-of-the-art pattern over the project's current default, rejecting a tempting alternative. -Examples of NOT ADR-worthy: feature-specific design (that's the spec), operational conventions (CLAUDE.md), file -naming (CLAUDE.md). - -If the brainstorm surfaced an ADR-worthy decision, author the ADR at `docs/adrs/-.md` using the format in -`docs/adrs/README.md` (Decision / Context / Consequences). Number sequentially — pick the next free `NNNN`. Add an -entry to the index in `docs/adrs/README.md`. Most features don't produce an ADR; if you're forcing one, the -decision probably belongs in the spec instead. - -Both the spec and any ADR(s) are tracked source artifacts (`docs/superpowers/specs/` and `docs/adrs/` are not -gitignored) — but **never commit a planning artifact to `main`.** The feature owns its branch from birth. The branch -name is the same whether the work ships as one PR or many (the step-4 PR-shape decision doesn't change it), so create -it off `origin/main` before the first commit: +**Spec.** Capture the brainstorm's conclusions in a spec at `docs/superpowers/specs/YYYY-MM-DD--design.md`. The spec is the feature's design record — WHAT we're building and WHY, with goals, non-goals, design choices, risks, alternatives considered. It does **not** enumerate PRs, commits, or parallelism contracts (those live in the plan, see step 6). Slug is short ("multi-tenant-profiles", not "implement-multi-tenant-profile-system"); date is the conception date. + +The spec's section headings may use the decomposition's organizing labels (`Flow 1`, `Flow 2`, …) to index the work — that's navigational. But nothing downstream (fixture names, ids, functions, files, marker strings) may inherit those labels as identifiers — the naming firewall in `feature-dev-workflow:maintaining-architectural-coherence`. + +**ADR (only if the brainstorm surfaced a project-wide architectural decision).** A spec captures feature-scoped design. An ADR captures **cross-cutting** decisions that affect multiple features — choosing one architectural approach over another for reasons that future readers will need. Examples of ADR-worthy decisions: introducing a new IPC channel, picking a state-of-the-art pattern over the project's current default, rejecting a tempting alternative. Examples of NOT ADR-worthy: feature-specific design (that's the spec), operational conventions (CLAUDE.md), file naming (CLAUDE.md). + +If the brainstorm surfaced an ADR-worthy decision, author the ADR at `docs/adrs/-.md` using the format in `docs/adrs/README.md` (Decision / Context / Consequences). Number sequentially — pick the next free `NNNN`. Add an entry to the index in `docs/adrs/README.md`. Most features don't produce an ADR; if you're forcing one, the decision probably belongs in the spec instead. + +Both the spec and any ADR(s) are tracked source artifacts (`docs/superpowers/specs/` and `docs/adrs/` are not gitignored) — but **never commit a planning artifact to `main`.** The feature owns its branch from birth. The branch name is the same whether the work ships as one PR or many (the step-4 PR-shape decision doesn't change it), so create it off `origin/main` before the first commit: ``` git fetch origin main git switch -c feature/ origin/main ``` -For a non-trivial (multi-PR) feature, run planning from a dedicated worktree on that branch instead, so the whole -feature — planning docs included — lives in one isolated checkout: +For a non-trivial (multi-PR) feature, run planning from a dedicated worktree on that branch instead, so the whole feature — planning docs included — lives in one isolated checkout: ``` git fetch origin main @@ -71,33 +51,22 @@ git worktree add .claude/worktrees/ -b feature/ origin/main cd .claude/worktrees/ ``` -Either way, stay on `feature/` for the rest of planning; every artifact commit lands there. Don't push yet — -that happens at step 8, after the user approves the spec. `feature-dev-workflow:developing-a-feature` reuses this branch (and the -worktree, if you created one); it never re-creates it off `origin/main`, and `main` receives the feature only through -the integration/feature PR. +Either way, stay on `feature/` for the rest of planning; every artifact commit lands there. Don't push yet — that happens at step 8, after the user approves the spec. `feature-dev-workflow:developing-a-feature` reuses this branch (and the worktree, if you created one); it never re-creates it off `origin/main`, and `main` receives the feature only through the integration/feature PR. ### 3. User reviews the spec (and any ADR) -Pause. Surface the spec path and wait for explicit "approved" or redirection before moving on. A spec the user hasn't -read is a draft, and drafts don't get tickets filed against them. +Pause. Surface the spec path and wait for explicit "approved" or redirection before moving on. A spec the user hasn't read is a draft, and drafts don't get tickets filed against them. ### 4. PR-shape decision Decide whether the work ships as: - **One PR** — single self-contained change, one reviewer pass, one merge to main. -- **Multiple PRs** — multiple feature-sized chunks, each independently reviewable, possibly parallelizable. Multi-PR - features land via the **feature-branch model**: a long-lived `feature/` branch off main; every sub-PR is a - real GitHub PR targeting `feature/` (not main); when every sub-PR has been self-merged into the feature - branch, a final **integration PR** from `feature/` to main collects the whole feature for external review. - Main stays shippable throughout the work; each sub-PR retains full GitHub visibility (comments, reviews, history). +- **Multiple PRs** — multiple feature-sized chunks, each independently reviewable, possibly parallelizable. Multi-PR features land via the **feature-branch model**: a long-lived `feature/` branch off main; every sub-PR is a real GitHub PR targeting `feature/` (not main); when every sub-PR has been self-merged into the feature branch, a final **integration PR** from `feature/` to main collects the whole feature for external review. Main stays shippable throughout the work; each sub-PR retains full GitHub visibility (comments, reviews, history). -The PR-shape judgment is grounded in **reviewer cost**: a 2000-line PR is unreviewable even if the work is "one -thing". If you can name two independent surfaces that ship value separately, that's two PRs and the feature-branch -model applies. +The PR-shape judgment is grounded in **reviewer cost**: a 2000-line PR is unreviewable even if the work is "one thing". If you can name two independent surfaces that ship value separately, that's two PRs and the feature-branch model applies. -The decision is noted in chat (or as a one-line `## Implementation breakdown` paragraph in the spec naming the PRs — -NOT the contracts between them). The spec stays a clean ADR otherwise. +The decision is noted in chat (or as a one-line `## Implementation breakdown` paragraph in the spec naming the PRs — NOT the contracts between them). The spec stays a clean ADR otherwise. ### 5. File the tracking issue(s) @@ -106,70 +75,44 @@ NOT the contracts between them). The spec stays a clean ADR otherwise. - Single PR → file one `feature` (or `bug`) issue. - Multiple PRs → file an `epic` with one sub-issue per PR. -Sub-issues reference only the parent epic in their `## Context` section — GitHub's sub-issue linkage threads them and -the epic is the design hub. Don't link the spec or plan from any issue: issues are durable, those files move and get -deleted, and the spec is referenced from the plan, not the ticket. +Sub-issues reference only the parent epic in their `## Context` section — GitHub's sub-issue linkage threads them and the epic is the design hub. Don't link the spec or plan from any issue: issues are durable, those files move and get deleted, and the spec is referenced from the plan, not the ticket. ### 6. Write the plan (and discover contracts) -**REQUIRED SUB-SKILL:** Invoke `superpowers:writing-plans` to produce -`docs/superpowers/plans/YYYY-MM-DD--plan.md`. The plan is HOW: ordered tasks, dependencies, review checkpoints, -PR-by-PR breakdown. +**REQUIRED SUB-SKILL:** Invoke `superpowers:writing-plans` to produce `docs/superpowers/plans/YYYY-MM-DD--plan.md`. The plan is HOW: ordered tasks, dependencies, review checkpoints, PR-by-PR breakdown. -**During plan-writing, identify parallelism and define contracts.** For every pair of PRs that could run in parallel, -ask: what does each side need to know about the other's wire shape, API surface, or data layout to start work -without blocking? That's a contract. Document each in a `## Contracts` section of the plan: +**During plan-writing, identify parallelism and define contracts.** For every pair of PRs that could run in parallel, ask: what does each side need to know about the other's wire shape, API surface, or data layout to start work without blocking? That's a contract. Document each in a `## Contracts` section of the plan: | Name | Producer (issue) | Consumer (issue) | Shape | Realization | | -------------------------- | ---------------- | ---------------- | ----------------------------------------------------- | ------------------------ | | `cache-storage-layout` | #22 | #24, #25 | path `~/.config///sessions//...` | data-only | | `config-loader-signature` | #22 | #24, #25 | `config.Resolve(name string) (*Config, error)` | pre-merge stub PR (#21) | -Each contract names the wire / signature / layout the consumer can write code against today, without waiting for the -producer's implementation. "TBD" is not a contract — block on it until it's concrete. - -**Realization strategy.** A contract row is conceptual; for parallel work to actually compile, the interface has to -exist as code or as data before either side starts. Pick one per row and put it in the Realization column: - -- **Pre-merge stub PR** — file a tiny scaffold PR that exports the symbol the consumers need (Go interface or - function signature with `panic("unimplemented")` body, TypeScript type, HTTP path constant). It targets the - feature branch (in multi-PR features) or main (in single-PR features) and merges BEFORE the implementation PRs - branch off. Producer and consumers then all branch from the post-stub state and import the real symbol. Best - default for code-shaped contracts (Go signatures, TS types); costs one trivial extra PR; payoff is both sides - compile from day one. Reference the stub PR's number in the Realization column once it's open. -- **Stub-on-producer-branch** — the producer's own PR opens with just the interface + panic-bodies; consumers branch - from the producer's branch (not main) and rebase as the producer fills in the body. Avoids the extra PR but - couples consumers to the producer's branch lifetime — rebase pain when the interface evolves. Use only when the - interface and one implementation are inherently coupled (shared unexported package, sibling files in one module). -- **Data-only** — when the contract is a path layout, file format, wire protocol, or env-var name, no code stub is - needed. Consumers write against the contract row directly — strings and paths are just strings and paths. Mark - the row `data-only`. +Each contract names the wire / signature / layout the consumer can write code against today, without waiting for the producer's implementation. "TBD" is not a contract — block on it until it's concrete. + +**Realization strategy.** A contract row is conceptual; for parallel work to actually compile, the interface has to exist as code or as data before either side starts. Pick one per row and put it in the Realization column: + +- **Pre-merge stub PR** — file a tiny scaffold PR that exports the symbol the consumers need (Go interface or function signature with `panic("unimplemented")` body, TypeScript type, HTTP path constant). It targets the feature branch (in multi-PR features) or main (in single-PR features) and merges BEFORE the implementation PRs branch off. Producer and consumers then all branch from the post-stub state and import the real symbol. Best default for code-shaped contracts (Go signatures, TS types); costs one trivial extra PR; payoff is both sides compile from day one. Reference the stub PR's number in the Realization column once it's open. +- **Stub-on-producer-branch** — the producer's own PR opens with just the interface + panic-bodies; consumers branch from the producer's branch (not main) and rebase as the producer fills in the body. Avoids the extra PR but couples consumers to the producer's branch lifetime — rebase pain when the interface evolves. Use only when the interface and one implementation are inherently coupled (shared unexported package, sibling files in one module). +- **Data-only** — when the contract is a path layout, file format, wire protocol, or env-var name, no code stub is needed. Consumers write against the contract row directly — strings and paths are just strings and paths. Mark the row `data-only`. Sequential work doesn't need contracts. Only document them where two workers genuinely run in parallel. +**Then pin the conventions.** Contracts keep parallel work *compiling*; conventions keep it *coherent*. Add a `## Conventions` section to the plan capturing the decisions every sub-PR inherits — the dimensions `feature-dev-workflow:maintaining-architectural-coherence` names (layout, naming, interfaces, vocabulary, idiom) for the surfaces this work touches. You're not inventing conventions; you're writing down the ones the brainstorm and the existing codebase already imply, so parallel workers reach the same answer instead of guessing. A convention you can't state in one line isn't agreed yet — agree it now, not after merge. Single-PR features can keep this terse; multi-PR fan-out depends on it. + ### 7. Refine issues if planning surfaced changes -Planning often surfaces scope changes. If the issues from step 5 no longer match the plan's breakdown, update them -**now**, before implementation starts. +Planning often surfaces scope changes. If the issues from step 5 no longer match the plan's breakdown, update them **now**, before implementation starts. -**REQUIRED SUB-SKILL:** Re-invoke `feature-dev-workflow:writing-github-issues` Step 2B (update) for any issue whose acceptance criteria, -scope, or dependencies shifted. Present and confirm each affected issue **one at a time** — one issue's gap list + -proposed body per confirmation prompt, then its `gh issue edit`, then the next. Don't batch several issues into a -single "yes to all": a wall of bodies gets rubber-stamped instead of read. +**REQUIRED SUB-SKILL:** Re-invoke `feature-dev-workflow:writing-github-issues` Step 2B (update) for any issue whose acceptance criteria, scope, or dependencies shifted. Present and confirm each affected issue **one at a time** — one issue's gap list + proposed body per confirmation prompt, then its `gh issue edit`, then the next. Don't batch several issues into a single "yes to all": a wall of bodies gets rubber-stamped instead of read. ### 8. Initialize the orchestration state file -Before handing off, create `docs/superpowers/states/YYYY-MM-DD--state.md` from -`${CLAUDE_PLUGIN_ROOT}/skills/planning-a-feature/templates/feature-state.md`. This is the **orchestration state** — phases, PRs, worktrees, contract realization, -bubble-up log. It's what a future Claude session reads first to resume the work without a massive user prompt. +Before handing off, create `docs/superpowers/states/YYYY-MM-DD--state.md` from `${CLAUDE_PLUGIN_ROOT}/skills/planning-a-feature/templates/feature-state.md`. This is the **orchestration state** — phases, PRs, worktrees, contract realization, bubble-up log. It's what a future Claude session reads first to resume the work without a massive user prompt. -The state file is scratch (same lifecycle as the plan): tracked in git so it survives sessions / worktrees / -machines, and deleted in the orchestrator's last commit once every sub-issue is closed and the feature has shipped. -Update it as the work progresses — see `feature-dev-workflow:developing-a-feature` for the update choreography. +The state file is scratch (same lifecycle as the plan): tracked in git so it survives sessions / worktrees / machines, and deleted in the orchestrator's last commit once every sub-issue is closed and the feature has shipped. Update it as the work progresses — see `feature-dev-workflow:developing-a-feature` for the update choreography. -Commit the spec, the plan, and the state file together as the planning artifact set on `feature/` (created in -step 2). Confirm you're not on `main`, then publish the branch so `feature-dev-workflow:developing-a-feature` can attach its integration -worktree to it: +Commit the spec, the plan, and the state file together as the planning artifact set on `feature/` (created in step 2). Confirm you're not on `main`, then publish the branch so `feature-dev-workflow:developing-a-feature` can attach its integration worktree to it: ``` git branch --show-current # must be feature/, never main @@ -178,19 +121,17 @@ git push -u origin feature/ ### 9. Hand off to implementation -Spec + plan + state file committed and pushed on `feature/`, issues aligned, contracts written → invoke -`feature-dev-workflow:developing-a-feature` to start the work. The state file is the entry-point artifact for every session that touches this feature afterward. +Spec + plan + state file committed and pushed on `feature/`, issues aligned, contracts written → invoke `feature-dev-workflow:developing-a-feature` to start the work. The state file is the entry-point artifact for every session that touches this feature afterward. ## Anti-patterns -- **Putting PR breakdown or parallelism contracts in the spec.** Spec is a durable ADR. PR breakdown is short-lived - scaffolding; contracts are implementation coordination. Both belong in the plan. +- **Putting PR breakdown or parallelism contracts in the spec.** Spec is a durable ADR. PR breakdown is short-lived scaffolding; contracts are implementation coordination. Both belong in the plan. - **Filing issues before the spec is reviewed.** Wastes time when scope shifts during review. - **Writing a plan before deciding PR shape.** The plan's structure depends on whether you're shipping one PR or many. -- **Declaring "we'll parallelize this" without writing the contract.** "Two workers in parallel" turns into "two - workers blocked on each other after day one" without explicit shapes. -- **Treating "no parallelism possible" as a defeat.** Sequential work is fine. The cost of inventing fake parallelism - exceeds the wall-clock saved. +- **Declaring "we'll parallelize this" without writing the contract.** "Two workers in parallel" turns into "two workers blocked on each other after day one" without explicit shapes. +- **Treating "no parallelism possible" as a defeat.** Sequential work is fine. The cost of inventing fake parallelism exceeds the wall-clock saved. +- **Pinning wire contracts but not conventions.** Contracts make parallel work compile; without a `## Conventions` block, each subagent invents its own layout and naming and the merged feature reads as written by a committee. +- **Letting an organizing label become an identifier.** `Flow 2` belongs in titles and spec headings, not in `flow2-fixture`, `TestFlow2…`, or `…_FLOW2_…`. The label is navigational; the code names the thing. ## Red flags diff --git a/skills/reviewing-feature-progress/SKILL.md b/skills/reviewing-feature-progress/SKILL.md index 9e3cf55..dca86a1 100644 --- a/skills/reviewing-feature-progress/SKILL.md +++ b/skills/reviewing-feature-progress/SKILL.md @@ -12,21 +12,15 @@ description: Three checkpoints during a feature in flight: -1. **Between fan-out waves.** After wave N is fully self-merged and before wave N+1 dispatches — catches drift while - it's still cheap to fix. -2. **Before opening the integration PR.** The integration PR is the external-review surface; its first impression - has to be right. -3. **Before flipping a draft integration PR to ready.** Any feedback addressed since the draft opened needs to be - re-aligned against the spec/plan. +1. **Between fan-out waves.** After wave N is fully self-merged and before wave N+1 dispatches — catches drift while it's still cheap to fix. +2. **Before opening the integration PR.** The integration PR is the external-review surface; its first impression has to be right. +3. **Before flipping a draft integration PR to ready.** Any feedback addressed since the draft opened needs to be re-aligned against the spec/plan. Skip for ad-hoc fixes that didn't go through `feature-dev-workflow:planning-a-feature`. ## Why this exists -Per-sub-PR review (`review` skill, run by the orchestrator inside `feature-dev-workflow:fanning-out-with-worktrees`) checks one diff at -one moment. It doesn't check whether all sub-PRs together still implement what the spec promised, whether the -acceptance criteria are covered, or whether the feature branch as a whole compiles and tests cleanly. Drift -accumulates silently across waves; this skill catches it at the boundary. +Per-sub-PR review (`review` skill, run by the orchestrator inside `feature-dev-workflow:fanning-out-with-worktrees`) checks one diff at one moment. It doesn't check whether all sub-PRs together still implement what the spec promised, whether they *cohere with each other* (naming, structure, vocabulary — drift that's invisible to any single diff or contract), whether the acceptance criteria are covered, or whether the feature branch as a whole compiles and tests cleanly. Drift accumulates silently across waves; this skill catches it at the boundary. ## Workflow @@ -35,8 +29,7 @@ accumulates silently across waves; this skill catches it at the boundary. Open these in order: - The state file (`docs/superpowers/states/--state.md`). -- The plan (`docs/superpowers/plans/--plan.md`), with focus on the `## Contracts` section and the - PR-by-PR breakdown. +- The plan (`docs/superpowers/plans/--plan.md`), with focus on the `## Contracts` section and the PR-by-PR breakdown. - The spec (`docs/superpowers/specs/--design.md`), with focus on goals + non-goals. - Each closed sub-issue's `## Acceptance criteria` section (`gh issue view `). @@ -44,41 +37,39 @@ Open these in order: For every row in the state file's `## PRs / worktrees` table with status `self-merged`: -- **Diff vs plan.** Use the `review` skill against the sub-PR number to walk the diff with full context. Cross-check - against the sub-issue's acceptance criteria — does the diff cover every bullet? -- **Contract realization.** If the sub-PR was a contract producer, inspect the symbol / wire / data layout that - actually shipped against what the contract row in the plan documented. The contract row's `Status` should be - `locked` and `Realized in` should point at the merged sub-PR. +- **Diff vs plan.** Use the `review` skill against the sub-PR number to walk the diff with full context. Cross-check against the sub-issue's acceptance criteria — does the diff cover every bullet? +- **Contract realization.** If the sub-PR was a contract producer, inspect the symbol / wire / data layout that actually shipped against what the contract row in the plan documented. The contract row's `Status` should be `locked` and `Realized in` should point at the merged sub-PR. -### 3. Acceptance-criteria coverage +### 3. Cross-PR coherence sweep + +Steps 1-2 check each sub-PR against an *external* reference (plan, contract, acceptance criteria). This step checks the sub-PRs against *each other* — the drift that's invisible to every per-PR check because each divergent choice is individually contract-satisfying. + +**REQUIRED SUB-SKILL:** `feature-dev-workflow:maintaining-architectural-coherence`. Apply it to the union of the self-merged sub-PRs (against the plan's `## Conventions` block and against each other) across every dimension it names — structure, interfaces, layering, naming + the firewall, vocabulary, idiom. Classify each finding **align now** (a convergence follow-up sub-PR before the integration PR — the cheap fix) or **deliberate, justified** (record the one-line why in the plan so the reviewer doesn't re-litigate it). Don't carry unexplained drift into the integration PR. + +### 4. Acceptance-criteria coverage For every sub-issue (whether `self-merged` or still open): - Is every bullet in the issue's `## Acceptance criteria` section now testable / observable in the feature branch? - If a criterion isn't covered, classify: - **Missing implementation** — file or surface a follow-up sub-PR; do not open the integration PR yet. - - **De-scoped during planning** — update the issue body to remove the criterion (via `feature-dev-workflow:writing-github-issues` Step - 2B, with confirmation); do not silently drop it. - - **Rephrased / equivalent** — the implementation satisfies the criterion under a different name; update the - criterion to match the language used in the diff. + - **De-scoped during planning** — update the issue body to remove the criterion (via `feature-dev-workflow:writing-github-issues` Step 2B, with confirmation); do not silently drop it. + - **Rephrased / equivalent** — the implementation satisfies the criterion under a different name; update the criterion to match the language used in the diff. -### 4. State-file integrity check +### 5. State-file integrity check Walk the state file and verify reality against record: -- Every `self-merged` row's PR has actually merged into the feature branch (`gh pr view --json mergedAt --jq - .mergedAt`). +- Every `self-merged` row's PR has actually merged into the feature branch (`gh pr view --json mergedAt --jq .mergedAt`). - Every `locked` contract row's `Realized in` PR is in fact merged. - Every `## Bubble-up log` entry has a propagation path recorded — no concerns left unresolved. -- The `feature_branch` and `feature_worktree` frontmatter still point at real things on disk - (`git rev-parse --verify feature/` + `ls `). +- The `feature_branch` and `feature_worktree` frontmatter still point at real things on disk (`git rev-parse --verify feature/` + `ls `). If anything is out of sync, fix the state file before continuing — the resumed-session contract depends on it. -### 5. End-to-end verification on the feature branch +### 6. End-to-end verification on the feature branch -**REQUIRED SUB-SKILL:** `superpowers:verification-before-completion`. Run the project-wide checks on the main feature -worktree (which holds the integration state — sub-worktrees only hold their own sub-branch): +**REQUIRED SUB-SKILL:** `superpowers:verification-before-completion`. Run the project-wide checks on the main feature worktree (which holds the integration state — sub-worktrees only hold their own sub-branch): ``` cd @@ -87,38 +78,31 @@ git pull origin feature/ # discovered from the project's CLAUDE.md / AGENTS.md or build config ``` -Paste the output. The feature branch must be green end to end before the integration PR opens — a sub-PR's isolated -CI passing doesn't guarantee the integration compiles, since each sub-PR's tests ran against its own branch state, -not the post-merge state. +Paste the output. The feature branch must be green end to end before the integration PR opens — a sub-PR's isolated CI passing doesn't guarantee the integration compiles, since each sub-PR's tests ran against its own branch state, not the post-merge state. -### 6. Synthesize the gap list +### 7. Synthesize the gap list Produce a short summary for the orchestrator (and the user, if this is a pre-integration-PR or pre-ready checkpoint): - **Drift found** — one bullet per discrepancy between spec/plan and what the diffs actually do. -- **Acceptance criteria uncovered** — one bullet per missing or rephrased criterion, with the classification from - Step 3. +- **Coherence findings** — one bullet per cross-PR inconsistency from Step 3 (naming, structure, API shape, vocabulary), each marked `align now` or `deliberate, justified`. +- **Acceptance criteria uncovered** — one bullet per missing or rephrased criterion, with the classification from Step 4. - **State-file fixes** — one bullet per row corrected. - **Verification status** — test / lint / typecheck pass/fail. Decide: - **All clean** → open the integration PR (or flip ready) per `feature-dev-workflow:developing-a-feature`. -- **Coverable by a follow-up sub-PR** → re-enter `feature-dev-workflow:developing-a-feature` Step 4 and dispatch a follow-up subagent into - a new sub-worktree. Update the state file with the new row. -- **Needs spec/plan refinement** → re-invoke `feature-dev-workflow:planning-a-feature` Steps 6/7 (write/update the plan, refine the - issues), surface the changes to the user, then continue. +- **Coverable by a follow-up sub-PR** → re-enter `feature-dev-workflow:developing-a-feature` Step 4 and dispatch a follow-up subagent into a new sub-worktree. Update the state file with the new row. +- **Needs spec/plan refinement** → re-invoke `feature-dev-workflow:planning-a-feature` Steps 6/7 (write/update the plan, refine the issues), surface the changes to the user, then continue. ## Anti-patterns -- **Skipping the check at phase transitions.** Each wave's drift compounds; surfacing it at the boundary is the - cheapest place to fix it. -- **Running verification on a sub-worktree instead of the main feature worktree.** Sub-worktrees only have their own - sub-branch checked out. The feature branch — where the integration shows up — lives in the main feature worktree. -- **Marking the integration PR ready without re-running this checkpoint after external feedback.** Reviewer-requested - changes can re-introduce drift the original review missed. -- **Treating the acceptance-criteria gap as a docs problem.** A missing criterion is either missing implementation or - a planning oversight. Don't silently delete it; classify and act. +- **Skipping the check at phase transitions.** Each wave's drift compounds; surfacing it at the boundary is the cheapest place to fix it. +- **Running verification on a sub-worktree instead of the main feature worktree.** Sub-worktrees only have their own sub-branch checked out. The feature branch — where the integration shows up — lives in the main feature worktree. +- **Marking the integration PR ready without re-running this checkpoint after external feedback.** Reviewer-requested changes can re-introduce drift the original review missed. +- **Calling it "all clean" on contract + acceptance + verification alone.** Those are external-reference checks; they pass while the merged surface drifts. Run the Step 3 coherence sweep before any "all clean". +- **Treating the acceptance-criteria gap as a docs problem.** A missing criterion is either missing implementation or a planning oversight. Don't silently delete it; classify and act. ## Red flags @@ -129,3 +113,4 @@ Decide: | "Acceptance criteria mismatch is fine, the spirit is the same" | Acceptance criteria are checkable conditions. Either they're met or they're not. Update the issue if the criterion changed. | | "I'll skip the state-file walk, I've been keeping it current" | The resumed-session contract is what the state file SAYS — verify it; don't trust your memory. | | "Lint passed in my sub-worktree, no need to re-run on feature" | Lint can be scoped to changed files; project-wide issues only surface on the integrated branch. | +| "Contracts, criteria, and CI are all green — all clean" | Those never see cross-PR coherence. Run the Step 3 sweep before declaring clean. | diff --git a/skills/writing-github-issues/SKILL.md b/skills/writing-github-issues/SKILL.md index 1180b13..0af0caa 100644 --- a/skills/writing-github-issues/SKILL.md +++ b/skills/writing-github-issues/SKILL.md @@ -10,8 +10,7 @@ description: ## When to invoke -Whenever you're about to file or edit a GitHub issue, **or** immediately after a brainstorming -session lands a decision that needs an issue to hold the work. +Whenever you're about to file or edit a GitHub issue, **or** immediately after a brainstorming session lands a decision that needs an issue to hold the work. Three branches: @@ -21,12 +20,9 @@ Three branches: ## Core principle: user-in-the-loop for every GitHub mutation -Don't create, modify, or link an issue without an explicit confirmation **for the specific body about to land**. This -applies even if the user said "file an issue" earlier in the conversation; generic intent earlier is not standing -consent for the specific body now. +Don't create, modify, or link an issue without an explicit confirmation **for the specific body about to land**. This applies even if the user said "file an issue" earlier in the conversation; generic intent earlier is not standing consent for the specific body now. -By default, assign the user to any issue created or touched (`--assignee @me`, which `gh` resolves to the authenticated -user). Surface the assignment in the same confirmation prompt; if they decline, note it and move on. +By default, assign the user to any issue created or touched (`--assignee @me`, which `gh` resolves to the authenticated user). Surface the assignment in the same confirmation prompt; if they decline, note it and move on. Every confirmation shows the user: @@ -35,16 +31,13 @@ Every confirmation shows the user: - Which label(s) will be attached. - Whether you intend to assign them (default: yes). -Wait for an explicit "yes" before any `gh issue create / edit` or sub-issue-linkage call. Treat absence of objection as -a no. +Wait for an explicit "yes" before any `gh issue create / edit` or sub-issue-linkage call. Treat absence of objection as a no. -GitHub doesn't render HTML comments, so leaving the template guidance in place is harmless — don't burn a step -removing it. +GitHub doesn't render HTML comments, so leaving the template guidance in place is harmless — don't burn a step removing it. ## Step 1: pick the template -Three templates live under `templates/`. The choice is a single judgment call — make it explicit; don't default to -the smallest shape. +Three templates live under `templates/`. The choice is a single judgment call — make it explicit; don't default to the smallest shape. ``` Is the work one self-contained change that ships in one PR? @@ -57,14 +50,9 @@ Is the work one self-contained change that ships in one PR? └─ No (plumbing, refactor, test work) ─────────→ FEATURE template, label: task ``` -For brainstorm output specifically: ask whether the outcome spans multiple feature-sized chunks. If yes, file an -**epic** plus one **feature/task/bug** per chunk; link each child as a sub-issue. If no, file a single **feature** -(or bug) and break it into PR-sized phases inside the Approach section. +For brainstorm output specifically: ask whether the outcome spans multiple feature-sized chunks. If yes, file an **epic** plus one **feature/task/bug** per chunk; link each child as a sub-issue. If no, file a single **feature** (or bug) and break it into PR-sized phases inside the Approach section. -**Tiebreaker for feature vs task** (when work touches user-observable state but reads mostly as plumbing): ask -whether the headline change a no-context reader would describe is user-observable. Yes → `feature`. No → `task`. -Example: a storage-layout migration that changes the on-disk format is `task` (the headline is "refactor the layout"); -the settings UI that lets the user pick a theme is `feature` (the headline is "you can now pick a theme"). +**Tiebreaker for feature vs task** (when work touches user-observable state but reads mostly as plumbing): ask whether the headline change a no-context reader would describe is user-observable. Yes → `feature`. No → `task`. Example: a storage-layout migration that changes the on-disk format is `task` (the headline is "refactor the layout"); the settings UI that lets the user pick a theme is `feature` (the headline is "you can now pick a theme"). | Template | When | Label | | ----------------- | ------------------------------------------------------------------------------------ | ------------------ | @@ -72,22 +60,27 @@ the settings UI that lets the user pick a theme is `feature` (the headline is "y | `feature.md` | One self-contained capability or refactor | `feature` or `task`| | `bug.md` | Unintended behaviour; broken contract; regression | `bug` | -## Don't hard-wrap body prose +## Don't hard-wrap markdown prose -Write each paragraph as a single line and put a blank line between paragraphs. Write each list item as a single line too. GitHub renders issue bodies far wider than an editor's 80–100 column guide, so a body hard-wrapped at ~90 columns reflows into ragged short lines in the GitHub view — the line breaks you inserted become visible noise the reader didn't ask for. Let GitHub do the wrapping. +Write each paragraph as a single line and put a blank line between paragraphs. Write each list item as a single line too. This applies to everything you author: the issue/PR body you publish AND markdown source files (these skills, their templates, READMEs, docs). There is no column cap to respect — GitHub and editors soft-wrap on their own, so any newline you insert mid-paragraph just reflows into ragged short lines in the rendered view and in every diff. Let the renderer wrap. Tables, fenced code, and YAML frontmatter keep their own line structure. -This governs the body you publish, not this skill's source files: the template comments and this document are themselves hard-wrapped for the editor, which is a source-file convention, not a model for the body. When you lift prose out of a template comment into the body, strip the wrapping — collapse it to one line per paragraph. +## Title hygiene + +A title is the one line a no-context reader scans in the issue list. Make it a concise, human-readable headline of the capability or fix. A clean organizing prefix is fine when the work is part of a decomposed set — `Flow 2: investigation session happy path` reads well. What doesn't: + +- **A trailing scaffolding parenthetical.** `Assert every launcher boot flag has its documented effect (e2e Flow 1)` buries the headline and tacks the plan's bookkeeping onto a durable artifact. Put the organizing label up front as a prefix, or leave it out — never as a `(… Flow N)` suffix. +- **Decorative Unicode in the title.** Arrows (`→`, `↔`), bullets, and box-drawing characters belong in body prose and diagrams, not in the title the reader scans. Use plain words. +- **A sentence that buries the lede.** If the headline noun-phrase comes after a clause of setup, cut the setup. + +### The naming firewall + +Organizing labels — `Flow N`, `Phase N`, `Wave N` — may appear in an issue title prefix or an epic's prose, because that's where humans group the work. They must **never** propagate into the code the issue produces. When the issue's acceptance criteria or approach name concrete artifacts, name them for what they are (`resumable-investigation`, not `flow2-fixture`). The full rule and the allowed-surface table live in `feature-dev-workflow:maintaining-architectural-coherence`. ## Diagrams: visualize architectural changes -When the change is architectural — new components, data or control flow crossing module boundaries, a phase/lifecycle -progression, or sequencing between processes — embed a mermaid diagram in the issue body. A diagram earns its place -when prose alone forces the reader to reconstruct the topology in their head; skip it when the change is one file with -no cross-module shape. +When the change is architectural — new components, data or control flow crossing module boundaries, a phase/lifecycle progression, or sequencing between processes — embed a mermaid diagram in the issue body. A diagram earns its place when prose alone forces the reader to reconstruct the topology in their head; skip it when the change is one file with no cross-module shape. -Applies to **feature** and **epic** bodies only (a bug is "what broke", not an architecture). Place it inside the -section that already carries the implementation shape: the epic's `## Design overview` or the feature's `## Approach`. -GitHub renders fenced ` ```mermaid ` blocks natively. +Applies to **feature** and **epic** bodies only (a bug is "what broke", not an architecture). Place it inside the section that already carries the implementation shape: the epic's `## Design overview` or the feature's `## Approach`. GitHub renders fenced `mermaid` blocks natively. Pick the diagram type that matches what's hard to see in prose: @@ -97,24 +90,16 @@ Pick the diagram type that matches what's hard to see in prose: | `sequenceDiagram` | Request/handoff ordering between processes (client ↔ server ↔ worker over time). | | `stateDiagram-v2` | Phase or lifecycle progressions (job states, request lifecycle, session state). | -Keep it honest: the diagram supplements the prose, it does not replace Design overview / Approach. Label nodes with -the real component names the prose and code use (the actual file, module, and type names) — a diagram of -invented boxes is worse than no diagram. The diagram is part of the body, so it lands in the same confirmation prompt -as everything else (§Core principle). +Keep it honest: the diagram supplements the prose, it does not replace Design overview / Approach. Label nodes with the real component names the prose and code use (the actual file, module, and type names) — a diagram of invented boxes is worse than no diagram. The diagram is part of the body, so it lands in the same confirmation prompt as everything else (§Core principle). ## Step 2A: No issue yet (create) -1. **Draft the issue body** from the chosen template. Each section earns its keep — read the `` guidance in - the template for what belongs where. The cross-template common shape: +1. **Draft the issue body** from the chosen template. Each section earns its keep — read the `` guidance in the template for what belongs where. The cross-template common shape: - **Title**: human-readable sentence a no-context reader can parse. - - **Problem**: a few sentences opening with the elevator pitch (the what) and naming the operational reason it - matters (the why). No solution; that belongs in the PR description (bug) or in Approach / Design overview - (feature / epic). + - **Problem**: a few sentences opening with the elevator pitch (the what) and naming the operational reason it matters (the why). No solution; that belongs in the PR description (bug) or in Approach / Design overview (feature / epic). - Template-specific sections: see the relevant template file. -2. **Confirm with the user, with the body inline.** Paste the drafted body into chat under a "About to create a - `