diff --git a/.claude/lib/github/commit-and-push.md b/.claude/lib/github/commit-and-push.md index fc425e8da..13bba7313 100644 --- a/.claude/lib/github/commit-and-push.md +++ b/.claude/lib/github/commit-and-push.md @@ -5,8 +5,12 @@ Prepares changes for PR: rebases, squashes commits, and pushes. ## 1. Rebase First, rebase onto the latest base branch to incorporate upstream changes. +**This must happen before the squash in §2, not after** — see the warning there. ```bash +# $BASE_REF is a remote-tracking ref and goes stale during a long session +git fetch upstream + # Save commit SHA before rebase BEFORE_REBASE=$(git rev-parse HEAD) @@ -66,18 +70,33 @@ COMMITS_AHEAD=$(git rev-list HEAD --not "$BASE_REF" --count 2>/dev/null || echo git reset --soft "$BASE_REF" ``` + **Only safe because §1 already rebased onto `$BASE_REF`.** The reset moves + HEAD to the base but keeps *your* index, so if the branch still sits on an + older base, every file the base gained meanwhile is recorded as **your + deletion** — and a later rebase will not undo it, because the revert is by + then part of your own diff. Step 5 verifies this; do not check it here, where + `HEAD` is `$BASE_REF` and a `$BASE_REF`-to-`HEAD` diff is empty by + construction. + 4. All changes are now staged. Delegate to `/git-commit`, passing the captured `/tmp/orig_pr_msg.txt` as the starting point so the single squashed commit **evolves** the original subject/body to cover the combined diff (see the message rule below) rather than inventing a generic one. If `OTHER_AUTHORS` is non-empty, append `Co-authored-by:` trailers for each human author. -5. **Re-verify** the count: +5. **Re-verify** the count, and that the squash reverted nothing: ```bash COMMITS_AHEAD=$(git rev-list HEAD --not "$BASE_REF" --count) # Must be exactly 1. If not, something went wrong — do NOT push. + + # Three dots, and only now that a commit exists — see step 3. + git diff "$BASE_REF"...HEAD --stat ``` + Every file listed must be one you meant to touch. Anything else is the + stale-base deletion described in step 3: restore it with + `git checkout "$BASE_REF" -- ` and amend. + **Important:** When squashing, the commit message must describe the final combined diff. Start from the original PR commit's message (keep its type/scope/subject and intent) and evolve it to absorb the folded-in fixes — do not replace it with a generic `fix(pr): …` message, and do not leave it frozen when the fix changed the commit's behavior or scope. ## 3. Push diff --git a/.claude/skills/fix-pr/SKILL.md b/.claude/skills/fix-pr/SKILL.md index 8173e6be4..a6b3bc403 100644 --- a/.claude/skills/fix-pr/SKILL.md +++ b/.claude/skills/fix-pr/SKILL.md @@ -205,6 +205,15 @@ git pull "$PUSH_REMOTE" "$HEAD_BRANCH" Run [checkout-fork-branch](../../lib/github/checkout-fork-branch.md) to create/switch to the local working branch and set the push refspec. +**Land on the current base before editing anything.** `$BASE_REF` moves while a +PR is open and everything below measures against it. Doing it here also keeps the +rebase off a dirty worktree, which git refuses to rebase: + +```bash +git fetch upstream # $BASE_REF is a remote-tracking ref — refresh it +git rebase "$BASE_REF" # see [commit-and-push](../../lib/github/commit-and-push.md) §1 +``` + **Fix:** 1. Read affected files, make changes with Edit tool @@ -215,14 +224,19 @@ The PR must stay as **one commit** (its original commit + your fixes squashed in), so reviewers see a single clean diff, not a running log of review churn. This is the default on **every** iteration. -Stage all changes, then squash based on how many commits the PR has ahead of -`$BASE_REF`: +Stage the fix and count what is ahead of the base you rebased onto above: ```bash git add -A COMMITS_AHEAD=$(git rev-list HEAD --not "$BASE_REF" --count) ``` +That rebase is why this is safe. `git reset --soft "$BASE_REF"` moves HEAD to the +base but keeps *your* index, so on a stale base every file the base gained since +you branched is recorded as **your deletion**. Rebasing afterwards does not undo +it — the revert is already part of your diff and replays cleanly. It surfaces as +unrelated files being reverted in the PR, easy to miss in a large diff. + | `COMMITS_AHEAD` | How to fold the fix in | | --------------- | ---------------------- | | `0` | **Error — stop.** Nothing ahead of base to fold into; do not rebase or push. Matches [commit-and-push](../../lib/github/commit-and-push.md) §2. | @@ -233,10 +247,19 @@ Do NOT run `/git-commit` to create a *new* commit here — that is what causes the appended-commit problem. `/git-commit` is only for regenerating the squashed message when `COMMITS_AHEAD > 1`. -Then push and verify exactly one commit landed: +Then verify and push: + +1. **Verify single commit:** `git rev-list HEAD --not "$BASE_REF" --count` must print `1` — never push a multi-commit PR. `> 1` means squash again; `0` means the fold produced nothing and is the same stop-and-investigate as in the table above, not something to re-squash. +2. **Verify you reverted nothing.** Review the file list with a **three-dot** diff: + + ```bash + git diff "$BASE_REF"...HEAD --stat # three dots: your changes only + ``` -1. Rebase onto `$BASE_REF` (see [commit-and-push](../../lib/github/commit-and-push.md) §1) -2. **Verify single commit:** `git rev-list HEAD --not "$BASE_REF" --count` must print `1`. If not, squash again before pushing — never push a multi-commit PR. + Two dots (`$BASE_REF..HEAD`) also reports files the base has and you do not, + rendering them as deletions you made — so it hides a real revert among noise + and invents fake ones. Any file here you did not intend to touch is a bug: + restore it with `git checkout "$BASE_REF" -- ` and amend. 3. Push (update push with `--force-with-lease` to `$PUSH_REMOTE`) **Commit message — evolve it, don't replace or freeze it.** The message must