From cbeb06f0f51a4393d38b7d52d978669dd4e17c87 Mon Sep 17 00:00:00 2001 From: Chao Wang <26245345+ChaoWao@users.noreply.github.com> Date: Sun, 26 Jul 2026 20:31:17 -0700 Subject: [PATCH] Fix: rebase before squashing in fix-pr, and check the diff with three dots MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `fix-pr` Step 6 told you to squash first and rebase after: the `> 1` row soft-resets to `$BASE_REF`, and only the following list says "rebase onto `$BASE_REF`". `commit-and-push` has the opposite (correct) order, so the two documents disagreed and the skill's order is the one that corrupts the commit. `git reset --soft "$BASE_REF"` moves HEAD to the base but keeps the branch's index. If the branch still sits on an older base — which it does whenever anything merged while the PR was open — every file the base gained meanwhile is recorded as a deletion by the PR. Rebasing afterwards does not repair it: the revert is by then part of the PR's own diff and replays cleanly onto the new base. It surfaces as unrelated files being reverted, easy to miss in a large diff and easy to merge. Hit while squashing #1506: `docs/troubleshooting/device-error-codes.md` came back as -86 lines because #1501 had merged since the branch point. Step 6 now fetches and rebases at the top, *before* the edit step rather than after it: git refuses to rebase a dirty worktree, so a rebase placed after the fix cannot run at all. Verification gains a three-dot diff of the file list, and it lives with the single-commit check after the commit exists — at the soft-reset itself `HEAD` is `$BASE_REF`, so that diff is empty by construction and would have verified nothing. Two dots would also list files the base has and the branch does not, rendering them as the branch's deletions: it both hides a real revert in the noise and invents fake ones. The single-commit check now separates its outcomes, since the surrounding table already treats them differently: `> 1` squashes again, `0` stops. `commit-and-push` §1 gains the fetch, and its soft-reset step states the precondition it silently relied on. Co-Authored-By: Claude Opus 5 (1M context) --- .claude/lib/github/commit-and-push.md | 21 ++++++++++++++++- .claude/skills/fix-pr/SKILL.md | 33 +++++++++++++++++++++++---- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/.claude/lib/github/commit-and-push.md b/.claude/lib/github/commit-and-push.md index fc425e8da7..13bba7313e 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 8173e6be4e..a6b3bc4037 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