Add decompose-pr skill#649
Conversation
Teach future Claude how to break a large feature branch into a series of self-contained PRs that each base directly on `main` — never on each other — so reviewers can evaluate each one independently and the queue keeps moving. Skill body covers: identifying the atomic unit before splitting; classifying each change category (protos, configs, refactor, new utility, behavior, tests, docs, unrelated cleanup) with a per-category rule; ordering without stacking via the orphan-code / feature-flag / deliberate-duplication escape-hatch hierarchy; concrete git workflow including the `git restore --source -p` escape for interleaved same-file changes; self-contained PR descriptions that forbid forward references like "Depends on #N" or "Final integration PR"; and presenting the user an explicit review order with one-line reasoning per PR. Includes a Common Mistakes table and Red Flags list seeded from baseline subagent pressure tests, plus a worked example that walks through a hypothetical 940-LOC mixed-concerns branch. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
User clarified that the no-stacking rule was being interpreted too strictly. Git base must be `main`, but a dependent PR's branch may (and should) contain cherry-picked copies of predecessor PRs' commits so the PR builds, type-checks, and is reviewable standalone. The output of decomposition is a dependency DAG of PRs, not a linear stack. When PR_A merges, dependent branches rebase onto the updated main and Git drops the duplicated commits automatically — each dependent PR's diff "cleans up" as predecessors land. Both PRs are independent merge candidates in GitHub. Edits: - Intro and core principle: clarify git-base vs diff-content distinction. - Step 3 (Order via a dependency tree, not a stack): full rewrite. Replaces "land A, rebase B, then review B" with the simultaneous-open / cherry-pick / rebase-after-merge model. Adds ASCII DAG diagram. Removes the "diamond is a smell" claim; diamonds are fine under this model. Names the real smells: deep chains, high in-degree, many roots. - Step 6: output format updated to render a dependency tree plus a suggested review order that respects it. - Worked example: adds a "Cherry-picks" column; renders the DAG explicitly; shows the integration PR cherry-picking all predecessors. Notes the transient diff bloat and how rebase resolves it. - Quick Reference: rewords the "Can two PRs be based on each other" row to specify git-base; updates the PR-B-needs-PR-A's-code row; adds a row describing the DAG output. - Common Mistakes: updates the "stack them" entry to explain the cherry-pick alternative; adds entries for "PR_B can't be opened until PR_A merges" and "a diamond is a smell." - Red Flags: tightens the "Never" entry to specify GitHub-stacking. Verified by a fresh REFACTOR subagent pressure pass on a synthetic diamond-dependency scenario (BatchSpec → sampler + loader → trainer); subagent produced a clean DAG, cherry-picked predecessors, kept `--base main`, and correctly did not flag the diamond as a smell. `uv run mdformat --check .claude/skills/decompose-pr/SKILL.md` passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five changes per user notes annotated in the skill: 1. Plan-only is now the default. The skill stops at Step 6 with the dependency tree + per-PR breakdown — no branches created, no cherry-picks, no PRs opened — so the user can workshop the breakdown before any git work happens. New Step 7 "Execute" runs the git commands, gated on the `--execute` flag or explicit user confirmation. `gh pr create` is per-PR confirmed. 2. Removed Section 1 "When to use, when not to" entirely and tightened the `description:` line to "Invoke only when the user explicitly asks." The skill must not auto-trigger on diff size or perceived complexity. 3. LOC is now framed as a heuristic, not a rule. A complex 600-LOC single algorithm is fine atomic. Tests don't count toward the LOC budget (they still ship with their code, but a 100-prod + 250-test PR is a 100-LOC PR). 4. Reversed the configs-go-separate rule. New configs bundle with the consumer that reads them — full stop — across all config locations (deployment/configs, task configs, examples). The only config-only PR is one where the config change is the only change in the branch. Removed two Common Mistakes entries that reinforced the old rule; added a new entry calling out the old rule as obsolete. Removed the "Bundle resource/task configs with the feature code" Red Flag. 5. Updated the worked example: PR_E (separate configs PR) is gone; its content bundles into PR_F. LOC counts are production-only with test deltas noted separately. The DAG is now 5 PRs (A, B, C, D, F) with the diamond preserved at F. Also renumbered sections after removing the original Section 1. `uv run mdformat --check .claude/skills/decompose-pr/SKILL.md` passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…, skip-blocks-descendants Three execution-correctness fixes from the Codex review (issues #2, #3, #4 — all High): 1. Clean-worktree preflight in Step 7. Before any `git checkout` in --execute mode, require `git status --short` to be empty. Tracked local edits can otherwise follow a checkout into the new decomposition branch and get staged or committed by mistake. Skill now stops and asks the user to stash / commit / discard before continuing. 2. Cherry-pick semantics: each PR's OWN commits, exactly once. The previous phrasing ("cherry-pick predecessor commits") was ambiguous and could be read as "cherry-pick the range `main..decomp/C`". That range includes C's own ancestors (e.g. shared root B), so picking it after already picking B directly causes B's commits to replay and conflict. The skill now says explicitly: cherry-pick each ancestor PR's OWN commits — never full branch ranges — in topological order. Shared ancestors (top of a diamond) get cherry-picked exactly once total per dependent branch. Updated: - Step 3 basic-build example to use `<PR_A's OWN commits>` and add a paragraph spelling out the rule. - Step 3 diamond paragraph: "each ancestor PR's own commits exactly once, in topological order." - Worked example F construction: per-line comments noting that C and D each contain B's commits but we don't re-include them. - Step 7 bash block to mirror the same convention. 3. Skipping a PR may block its descendants. Per user clarification: skipping a parent PR defers the entire subtree rooted at that PR. Descendants can't be merged either — their cherry-picked copy of the predecessor's code is unreviewed at HEAD until the predecessor lands. Only independent siblings remain unblocked. Skill now explicitly tells the user "Skipping #N defers #M and #K (its dependents) too." Also added three Common Mistakes entries covering each of the above so the rules survive deadline-pressure rationalization. mdformat clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
svij-sc
left a comment
svij-sc
left a comment
There was a problem hiding this comment.
Thanks for introducing this skill. RE: stacked PRs or not to stack PRs - I can see the reasoning behind them. I'm not fully persuaded yet, though, and I'd genuinely like to understand your perspective better.
I suspect a real-time conversation might get us on the same page faster than a long thread here. Let's chat.
| @@ -0,0 +1,530 @@ | |||
| ______________________________________________________________________ | |||
There was a problem hiding this comment.
There is a lot of tokens here in the metadata - which are always loaded and pollute the context window.
See guidelines: https://platform.claude.com/docs/en/agents-and-tools/agent-skills/overview#level-1-metadata-always-loaded
and: https://platform.claude.com/docs/en/agents-and-tools/agent-skills/overview#skill-structure
We may need to update other skills too if they are following a similar pattern and they snuck in.
| | New utility / infrastructure | New module under `gigl/`, no caller yet on `main` | Orphan-code PR | | ||
| | Behavioral change ("the feature") | Code that flips a code path, adds a new pipeline stage, changes outputs | Feature PR (bundled with its new configs) | | ||
| | Tests for existing code | `tests/unit/**` that exercise code already on `main` | Tests-ahead PR or fold into related PR | | ||
| | Docs-only / docstring | `*.md`, unrelated docstring cleanups, comments unrelated to a feature | Docs PR | |
There was a problem hiding this comment.
This is very context dependent.
If it is related to files that are already being touched / refactored - why not do this?
It just adds extra churn and overloads reviewers.
| **Never:** | ||
|
|
||
| - GitHub-stack a PR (set its `--base` to another open PR's branch). Every PR's base is `main`; predecessor code lives in | ||
| the dependent branch via `git cherry-pick`. |
There was a problem hiding this comment.
I disagree with this.
This is a foundational modality in working with git. It helps break out PRs and expedites author continuing to work on changes (not be blocked) while waiting on reviews.
In fact, github is rolling out a new feature to make UX easier here for the author: https://github.github.com/gh-stack/
| **Always:** | ||
|
|
||
| - Name the atomic unit (Step 0) before slicing. | ||
| - Base every PR on `main`. |
|
|
||
| | Rationalization | Reality | | ||
| | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | "Stack them (git-base PR_B on PR_A) so each is independently reviewable." | GitHub-stacking forces serial merge order and ties review state across the chain. Instead, **cherry-pick PR_A's commits into PR_B's branch while keeping PR_B's git base at `main`**. PR_B builds standalone; the duplicated code drops out on rebase after PR_A merges. Both PRs are independent merge candidates in GitHub. | |
There was a problem hiding this comment.
GitHub-stacking forces serial merge order and ties review state across the chain. Instead
I don't get why this is a con?
You get simple diffs across multiple PRs, one building on top of the last.
Given your changes are "atomic" a reviewer need not know the context of prior PR - but preferred.
| | Rationalization | Reality | | ||
| | --------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | "Stack them (git-base PR_B on PR_A) so each is independently reviewable." | GitHub-stacking forces serial merge order and ties review state across the chain. Instead, **cherry-pick PR_A's commits into PR_B's branch while keeping PR_B's git base at `main`**. PR_B builds standalone; the duplicated code drops out on rebase after PR_A merges. Both PRs are independent merge candidates in GitHub. | | ||
| | "PR_B can't be opened until PR_A merges, because PR_B needs PR_A's code." | Both halves are partially right. PR_B IS openable immediately — as a **draft** (`gh pr create --draft`). CI runs, the PR number exists, the branch builds because it cherry-picks PR_A's commits. But PR_B is NOT sent for review (no reviewers, draft status) until PR_A merges into `main`. After PR_A merges, rebase PR_B (cherry-picked A commits drop out) and convert from draft to ready-for-review (`gh pr ready <N>`). | |
There was a problem hiding this comment.
But PR_B is NOT sent for review (no reviewers, draft status) until PR_A merges into
main
The reality is if we operate like this it will just slow down review cycles.
In my personal experience, if I have 30 minutes 2 times a day to review PRs - I can get through 2 stacked PRs. Or 1 PR today (wait to merge) and 1 PR tomorrow.
Stacked PRs also help maintain context as I can understand how changes in one PR are being leveraged in another.
In my personal experience, non stacked PRs --> cherry-picking into multiple other working branches also makes extra effort for authors to either 1. cherry-pick changes to all their open PRs, handling issues, conflicts, et al. 2. As a side effect of cherry-picking being hard, differing it to refactoring in the future - opening up a lot more "refactoring" PRs to address prior comments - where now adding extra burden on reviewer to go back and revisit original comments days after.
| | "TODO(author) note IS a downstream reference if it says 'follow-up'." | "Follow-up" is intent, not a reference. The line crosses when the TODO names a PR number or describes the follow-up's scope. | | ||
| | "Wide-and-deep is fine if the PR is overall small." | If it's small the rule doesn't bind. If it's not small, pick one. | | ||
| | "I'll stack just this once because rebasing is annoying." | Stacking shifts the rebase cost from the author to every reviewer in the chain. The rule exists exactly to prevent that. | | ||
| | "Splitting will create too many PRs and overwhelm reviewers." | Reviewers prefer five 100-LOC PRs over one 500-LOC PR. Microsoft's 15M-PR analysis confirms throughput improves with smaller PRs even at higher count. | |
There was a problem hiding this comment.
Microsoft's 15M-PR analysis confirms throughput improves
What is this?
Reviewers prefer five 100-LOC PRs over one 500-LOC PR
I would just leave this out. We are contradicting above statements btw on what makes a good spittable PR and not to consider LOCs and now we are considering LOCs.
Agents are unfortunately bad at reading the exact intent here thus, the stamtents made should not easy to contradict each other - lets audit thie table take out what is unecessary and could be inferred from other statements already presented. (This and others here)
3 participants
Adding skill so we can easily breakup large changes / features into smaller prs more easily.