Skill to improve commit history and make PR reviews easier #158
Skill to improve commit history and make PR reviews easier #158umair-ably wants to merge 1 commit intomainfrom
Conversation
… and forth agent assisted sessions
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughA new documentation file introduces the clean-commits skill, which defines a five-phase workflow (Analyze, Plan, Execute, Verify, Push) for rewriting git history into clean, logical commits while preserving the final tree state. Changes
Estimated Code Review Effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
.claude/skills/clean-commits/SKILL.md (1)
14-21: Consider parsing the JSON output or clarifying the intent.Line 17 uses
gh pr view --json baseRefNamebut doesn't show how to extract the value. If the intent is just to check whether a PR exists (using exit code), the comment could clarify that. Otherwise, consider showing the complete command to extract and use the base ref name.💡 Example of parsing the JSON output
- # Check if the branch tracks a remote with a PR target - gh pr view --json baseRefName 2>/dev/null + # Try to get the base branch from the PR + BASE_REF=$(gh pr view --json baseRefName -q .baseRefName 2>/dev/null) + BASE_COMMIT=$(git rev-parse "origin/$BASE_REF" 2>/dev/null) # Fall back to merge-base with main/master - git merge-base HEAD main 2>/dev/null || git merge-base HEAD master + [[ -z "$BASE_COMMIT" ]] && BASE_COMMIT=$(git merge-base HEAD main 2>/dev/null || git merge-base HEAD master)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/clean-commits/SKILL.md around lines 14 - 21, Clarify whether the intent of the gh call is just to test for a PR (exit code) or to actually obtain the base ref name; if you need the base ref use gh pr view --json baseRefName and parse the JSON output (e.g., use gh's -q/JQ or jq to extract .baseRefName) and assign that value for the subsequent merge-base logic, otherwise update the comment to state that gh pr view --json baseRefName is used only for its exit status and fall back to git merge-base HEAD main || git merge-base HEAD master; reference the invocation gh pr view --json baseRefName and the fallback git merge-base commands in the clarification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/clean-commits/SKILL.md:
- Around line 142-152: Update the fenced code block that shows the backup branch
cleanup in "Phase 5: Push" so it includes the language specifier "bash"; locate
the block containing the literal line "git branch -d ${BRANCH}-backup # when
you're happy with the result" and change the opening fence to specify bash
(e.g., ```bash) to satisfy MD040 and markdownlint-cli2.
- Around line 52-61: The fenced code block under the "**Present the plan to the
user as a table:**" section is missing a language tag causing MD040; update the
opening fence for the sample table to include a language specifier (for example
change "```" to "```text" or "```markdown") so the example table is properly
recognized by markdownlint and rendered correctly.
---
Nitpick comments:
In @.claude/skills/clean-commits/SKILL.md:
- Around line 14-21: Clarify whether the intent of the gh call is just to test
for a PR (exit code) or to actually obtain the base ref name; if you need the
base ref use gh pr view --json baseRefName and parse the JSON output (e.g., use
gh's -q/JQ or jq to extract .baseRefName) and assign that value for the
subsequent merge-base logic, otherwise update the comment to state that gh pr
view --json baseRefName is used only for its exit status and fall back to git
merge-base HEAD main || git merge-base HEAD master; reference the invocation gh
pr view --json baseRefName and the fallback git merge-base commands in the
clarification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1263136d-3207-4c27-9a09-31b873b278a3
📒 Files selected for processing (1)
.claude/skills/clean-commits/SKILL.md
| **Present the plan to the user as a table:** | ||
| ``` | ||
| | # | Commit message | Files | Purpose | | ||
| |---|---------------|-------|---------| | ||
| | 1 | [PREFIX] Add X utility | 1 | Core utility function | | ||
| | 2 | [PREFIX] Add Y to base command | 3 | Framework methods | | ||
| | 3 | [PREFIX] Migrate commands to use X | ~80 | Mechanical — same pattern everywhere | | ||
| | ... | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Add language specifier to the fenced code block.
The code block showing the example table should specify a language (e.g., markdown or text) for proper rendering and to satisfy markdown linting rules.
📝 Proposed fix
**Present the plan to the user as a table:**
-```
+```text
| # | Commit message | Files | Purpose |As per coding guidelines, static analysis tool markdownlint-cli2 reports: "Fenced code blocks should have a language specified (MD040)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| **Present the plan to the user as a table:** | |
| ``` | |
| | # | Commit message | Files | Purpose | | |
| |---|---------------|-------|---------| | |
| | 1 | [PREFIX] Add X utility | 1 | Core utility function | | |
| | 2 | [PREFIX] Add Y to base command | 3 | Framework methods | | |
| | 3 | [PREFIX] Migrate commands to use X | ~80 | Mechanical — same pattern everywhere | | |
| | ... | |
| ``` | |
| **Present the plan to the user as a table:** |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 53-53: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/clean-commits/SKILL.md around lines 52 - 61, The fenced code
block under the "**Present the plan to the user as a table:**" section is
missing a language tag causing MD040; update the opening fence for the sample
table to include a language specifier (for example change "```" to "```text" or
"```markdown") so the example table is properly recognized by markdownlint and
rendered correctly.
| ### Phase 5: Push | ||
|
|
||
| Ask the user if they want to force push. Only after explicit approval: | ||
| ```bash | ||
| git push --force-with-lease origin "$BRANCH" | ||
| ``` | ||
|
|
||
| Remind the user the backup branch is available locally: | ||
| ``` | ||
| git branch -d ${BRANCH}-backup # when you're happy with the result | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to the fenced code block.
The code block showing the backup branch cleanup command should specify bash as the language for proper rendering and to satisfy markdown linting rules.
📝 Proposed fix
Remind the user the backup branch is available locally:
-```
+```bash
git branch -d ${BRANCH}-backup # when you're happy with the result</details>
As per coding guidelines, static analysis tool markdownlint-cli2 reports: "Fenced code blocks should have a language specified (MD040)".
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 150-150: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/clean-commits/SKILL.md around lines 142 - 152, Update the
fenced code block that shows the backup branch cleanup in "Phase 5: Push" so it
includes the language specifier "bash"; locate the block containing the literal
line "git branch -d ${BRANCH}-backup # when you're happy with the result" and
change the opening fence to specify bash (e.g., ```bash) to satisfy MD040 and
markdownlint-cli2.
</details>
<!-- fingerprinting:phantom:triton:puma -->
<!-- This is an auto-generated comment by CodeRabbit -->
|
Why is this in this repo? |
There was a problem hiding this comment.
Pull request overview
Adds a new Claude “clean-commits” skill document to guide users through rewriting messy feature-branch history into a small set of logical, reviewer-friendly commits while preserving the final tree state.
Changes:
- Introduces a 5-phase workflow (Analyze → Plan → Execute → Verify → Push) for commit history cleanup.
- Documents safety practices (backup branch, user approval gates, diff verification, force-with-lease).
- Provides command examples and a suggested commit-grouping rubric for reviewable history.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Check if the branch tracks a remote with a PR target | ||
| gh pr view --json baseRefName 2>/dev/null | ||
| # Fall back to merge-base with main/master | ||
| git merge-base HEAD main 2>/dev/null || git merge-base HEAD master | ||
| ``` | ||
| Only care about commits between the base and HEAD. Ignore commits that are already on the base branch. | ||
|
|
||
| 2. **Read the history.** For each commit on this branch: | ||
| ```bash | ||
| git log <base>..HEAD --reverse --format="%h %s" --stat |
There was a problem hiding this comment.
The “Detect the base” section runs gh pr view --json baseRefName but doesn’t show how to extract the branch name (or compute the merge-base from it). As written it just prints JSON and isn’t actionable for determining <base>. Consider using --jq to extract .baseRefName and then running git merge-base HEAD <baseRefName> (or document the exact next step).
| # Check if the branch tracks a remote with a PR target | |
| gh pr view --json baseRefName 2>/dev/null | |
| # Fall back to merge-base with main/master | |
| git merge-base HEAD main 2>/dev/null || git merge-base HEAD master | |
| ``` | |
| Only care about commits between the base and HEAD. Ignore commits that are already on the base branch. | |
| 2. **Read the history.** For each commit on this branch: | |
| ```bash | |
| git log <base>..HEAD --reverse --format="%h %s" --stat | |
| # Try to detect the PR's base branch (e.g., main) and compute the merge-base | |
| BASE_BRANCH="$(gh pr view --json baseRefName --jq '.baseRefName' 2>/dev/null || echo '')" | |
| if [ -n "$BASE_BRANCH" ]; then | |
| BASE_COMMIT="$(git merge-base HEAD "$BASE_BRANCH")" | |
| else | |
| # Fall back to merge-base with main/master | |
| BASE_COMMIT="$(git merge-base HEAD main 2>/dev/null || git merge-base HEAD master)" | |
| fi | |
| echo "Using base commit: $BASE_COMMIT" |
Only care about commits between the base and HEAD. Ignore commits that are already on the base branch.
- Read the history. For each commit on this branch:
git log "$BASE_COMMIT"..HEAD --reverse --format="%h %s" --stat
| | Priority | Category | Why separate | | ||
| |----------|----------|-------------| | ||
| | 1 | Infrastructure / utilities | Small, foundational — reviewer needs to understand these first | | ||
| | 2 | Base class / framework changes | The API that other code depends on | | ||
| | 3 | Mechanical migrations | Same pattern across many files — reviewer can skim. Note file count in the plan | | ||
| | 4 | Behavioral / logic changes | The interesting stuff — small diffs that deserve careful review | | ||
| | 5 | Test infrastructure + test updates | Tests grouped together, after the code they test | | ||
| | 6 | E2e / integration test fixes | Different environment, different review | | ||
| | 7 | Documentation | Docs last so reviewer sees the spec after the code | |
There was a problem hiding this comment.
The table under “Common groupings” uses || at the start of each row, which renders as an extra empty column (and may not render as a table in some Markdown parsers). Use standard Markdown table syntax with single leading pipes (or no leading pipe) so it renders consistently.
| git diff --name-only <base> <final> -- src/some/path/ | \ | ||
| grep -v 'special-file' | \ | ||
| while IFS= read -r f; do git checkout <final> -- "$f"; done | ||
| SCRIPT |
There was a problem hiding this comment.
In the checkout script example, placeholders switch between <base>/<final> and earlier variables like BASE_COMMIT / ORIGINAL_HEAD. This inconsistency makes the steps harder to follow and easier to copy/paste incorrectly. Consider using the same placeholder names throughout (or reference the variables already introduced).
|
|
||
| 1. **Diff check — this is non-negotiable.** The final tree MUST match the original: | ||
| ```bash | ||
| git diff "$BRANCH" "${BRANCH}-backup" -- <relevant paths> |
There was a problem hiding this comment.
Phase 4 says the diff check is “non-negotiable” and that the final tree MUST match, but the suggested command scopes the diff to <relevant paths>, which could miss discrepancies elsewhere. To guarantee identical tree state, run an unscoped diff (or explicitly state that it must be run without a pathspec).
| git diff "$BRANCH" "${BRANCH}-backup" -- <relevant paths> | |
| git diff "$BRANCH" "${BRANCH}-backup" |
New skill to help clean up commit history. Useful for those long back and forth agent assisted sessions
Summary by CodeRabbit