Fix: rebase before squashing in fix-pr, and check the diff with three dots - #1516
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe documentation now requires refreshing and rebasing onto ChangesGit workflow documentation
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/lib/github/commit-and-push.md:
- Around line 73-79: The verification command is positioned incorrectly after
git reset --soft "$BASE_REF", where the three-dot diff is empty regardless of
staged deletions. Move the git diff "$BASE_REF"...HEAD --stat check to
immediately after the squashed commit is created, or use git diff --cached
"$BASE_REF" --stat at the current staged state, while preserving the warning and
push guidance.
In @.claude/skills/fix-pr/SKILL.md:
- Around line 244-246: Update the verification instructions around COMMITS_AHEAD
so zero commits terminates the workflow as an error, while only values greater
than one require squashing before pushing. Preserve the requirement that exactly
one commit must be present for a valid push.
- Around line 218-226: Update the rebase sequence in the fix workflow so edits
made in the preceding step are preserved before running git rebase "$BASE_REF".
Either move this rebase earlier than the edit step or stash tracked and
untracked changes, fetch and rebase, then restore the stash before git add -A;
keep the existing commit-count calculation after restoration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3cf6b19-17eb-4b44-80a0-fe7145ca154e
📒 Files selected for processing (2)
.claude/lib/github/commit-and-push.md.claude/skills/fix-pr/SKILL.md
748f49d to
cbeb06f
Compare
… dots `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 hw-native-sys#1506: `docs/troubleshooting/device-error-codes.md` came back as -86 lines because hw-native-sys#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) <noreply@anthropic.com>
|
@coderabbitai All three addressed — thanks, two of them were bugs this very PR introduced:
|
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
What
fix-prStep 6 told you to squash first and rebase after;commit-and-pushhasthe opposite (correct) order. The two documents disagreed, and the skill's order
is the one that corrupts the commit. Step 6 now fetches and rebases before it
stages anything, and verification uses a three-dot diff.
Why it matters
git reset --soft "$BASE_REF"moves HEAD to the base but keeps the branch'sindex. 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.
Hit for real while squashing #1506:
docs/troubleshooting/device-error-codes.mdcame back as −86 lines, because #1501 had merged since the branch point. Caught
by eye during a file-list review; nothing in the workflow would have flagged it.
Also
Verification now uses
git diff "$BASE_REF"...HEAD --stat(three dots). Two dotsalso lists files the base has and the branch does not, rendering them as the
branch's deletions — so it simultaneously hides a real revert in the noise and
invents fake ones. I chased two phantom "deletions" that way in the same session
before realising the diff form was wrong.
commit-and-push§1 gains thegit fetch($BASE_REF is a remote-tracking refand goes stale in a long session), and its soft-reset step now states the
precondition it silently relied on.
Docs only — no code paths touched.
🤖 Generated with Claude Code