feat(skills): add squash-merge skill for preserving commit history on upstream merges#5782
Conversation
singlerider
requested review from
JordanTheJet and
theonlyhennygod
as code owners
github-actions
bot
added
the
docs
singlerider
added
risk: low
B1: add CI gate pre-flight check (gh pr checks) B2: add reviewDecision check — CHANGES_REQUESTED blocks, REVIEW_REQUIRED warns B3: clarify Step 5 confirmation shows real expanded values, not placeholders B4: assign PR title/commits to shell variables to prevent injection via special chars B5: add error handling — stop and report verbatim on non-zero exit from gh pr merge H1: add mergeable/CONFLICTING pre-flight check H2: fix fallback base ref to use PR baseRefName not local 'master' H3: eliminate HEREDOC in favour of shell variable assignment (removes EOF collision risk) H4: covered by B4 variable approach M1: tighten trigger — require PR number or explicit squash-merge context M2: reframe self-merge rule — maintainers routinely self-merge, note it in confirmation M3: add Prerequisites section documenting gh >= 2.17.0 requirement M4: add branch cleanup command and guidance in Step 7 M5: detect single-commit PRs and omit redundant bullet body L1: rewrite motivation — replace 'useless' claim with accurate formatting/PR-number framing L2: note API commit ordering caveat and git log fallback for rebased histories L3: remove 'close as merged' from trigger description L4: show body in separate indented block in Step 5 confirmation, not inline
- Define $NUMBER variable explicitly in Step 1 via gh pr view --json number - Step 5 confirmation now uses shell variable syntax ($SUBJECT, $COMMITS, $NUMBER) instead of angle-bracket placeholders, resolving the 'no placeholders' contradiction - gh pr checks CI gate note: distinguish required vs optional (user confirms) - reviewDecision fallback: handle null via // "" in jq - fallback git log: fetch upstream + origin first; use origin/HEAD_REF; note fork limitation - Step 7 jq: guard mergeCommit null with if/then/else to prevent expression error
Branch cleanup is always the contributor's decision. Remove the deletion command suggestion entirely from Step 7 and Rules.
…ge skill - Replace all <NUMBER>, <NUMBER_OR_URL>, <sha> placeholders with $NUMBER variable - Fix single-commit SHA lookup to use gh API instead of placeholder - Remove redundant Step 4 (echo preview superseded by confirmation step) - Renumber steps: 5 steps now instead of 7 - Add auto-detect failure path: ask user for PR number if not on a PR branch - Remove unused mergeStateStatus from JSON fetch - Drop over-engineered CI pre-flight check (permission failures are fine at runtime)
MD034: wrap bare URL in markdown link syntax MD036: remove bold emphasis from confirmation prompt (not a heading)
WareWolf-MoonWall
left a comment
WareWolf-MoonWall
left a comment
There was a problem hiding this comment.
Review — PR #5782 feat(skills): add squash-merge skill
Reviewer: WareWolf-MoonWall
RFC authority: RFC #5615 (Contribution Culture), RFC #5653 (Zero Compromise in Practice)
✅ Commendation
The problem statement is accurate and well-understood: GitHub default squash merge omits the PR number from the subject; direct-pushing bypasses the merge mechanism entirely so the PR shows Closed rather than Merged with no issue auto-close. Both outcomes are bad for a project that relies on issue linkage and a navigable commit history. The skill solves both at once cleanly.
The confirmation gate (Step 4) is the right design for an irreversible action. Three details are worth naming for future skills that execute destructive actions:
- The gate shows the user the exact expanded command with real values substituted — not variable names, not placeholders.
- Consent is not inferred from silence, prior approval, or yes-but-first — all three are explicitly ruled out. This closes the most common LLM-specific bypass patterns.
- The accepted responses are enumerated: yes, y, go, do it. Anything else means stop.
The branch deletion prohibition is correct and the reasoning is given: branch cleanup is always a human decision. The self-merge note is appropriately handled — maintainers routinely merge their own PRs; surfacing it in the confirmation summary keeps it in the audit trail.
The single-commit PR handling in Step 2 is a thoughtful detail. A one-item bullet list adds no information; using the full commit body in that case produces a cleaner squash commit for the most common case.
CI summary
| Gate | Result |
|---|---|
| Security Audit | ✅ SUCCESS |
| Security Required Gate | ✅ SUCCESS |
| Docs Quality | ✅ SUCCESS |
| Lint / Strict Delta Lint | ✅ SUCCESS |
| All builds / tests / checks | ✅ SUCCESS |
Markdownlint confirmed independently: 0 errors.
🟡 Conditional — origin remote assumption in git fallback
Step 2 git fallback resolves the contributor branch via origin/HEAD_REF:
COMMITS=$(git log upstream/BASE_REF..origin/HEAD_REF --format="- %h %s")This assumes the contributor branch is on a remote named origin. That holds for the common setup where origin is the contributor fork and upstream is zeroclaw-labs/zeroclaw — but silently fails for any other remote layout. The skill acknowledges the fork limitation (if origin/HEAD_REF does not exist, the fallback cannot be used) but does not surface the assumption about what origin points to.
The GitHub API path is preferred and sufficient for the vast majority of cases. Suggest adding a one-line note: "This assumes origin is the contributor fork remote. If your remote layout differs, use the gh API output directly." Not a blocker — the failure mode is a silently empty commit list, not incorrect output.
Summary
The skill is correct and security-conscious. Confirmation gate design is solid. Shell variable quoting is correct (double-quoted variables prevent re-evaluation of embedded command substitutions in PR titles or commit messages). CI is fully green. The one conditional is a documentation gap in the git fallback path, not a code issue.
Approved. ✅
github-project-automation
bot
moved this from Backlog
to Ready to Merge
in ZeroClaw Project Board
github-project-automation
bot
moved this from Ready to Merge
to Shipped
in ZeroClaw Project Board
2 participants
Summary
master.claude/skills/squash-merge/SKILL.md— a new Claude Code skill that resolves the PR, collects its commit history, builds a conventional-commit subject line, shows the user the exactgh pr merge --squash --subject --bodycommand for confirmation, and only executes after an explicit yes.Label Snapshot (required)
risk: lowsize: XS(auto-managed/read-only)skillsChange Metadata
featuredocsLinked Issue
Validation Evidence (required)
Commands and result summary:
cargo fmt --all -- --check cargo clippy --all-targets -- -D warnings cargo testmarkdownlintnot present in environment (skipped)..claude/skills/squash-merge/SKILL.mdonly (markdown, no Rust).markdownlintunavailable in local environment; structure and heading hierarchy verified by visual inspection.Security Impact (required)
Privacy and Data Hygiene (required)
<NUMBER>,<branch>,<title>placeholders; no identity-like strings.Compatibility / Migration
i18n Follow-Through (required when docs or user-facing wording changes)
.claude/skills/is Claude Code agent tooling, not user-facing documentation or navigation. No SUMMARY.md or locale file impact.Human Verification (required)
gh pr mergecommand; execution blocked until explicit yes.ghCLI lacks merge permissions on the repo.Side Effects / Blast Radius (required)
/squash-mergeas a new Claude Code skill. No existing skills or workflows modified.gh pr mergeruns; Step 6 verifies merge state after completion.Agent Collaboration Notes (recommended)
AGENTS.md+CONTRIBUTING.md): YesRollback Plan (required)
git revert <merge-commit>on upstream/master, or delete.claude/skills/squash-merge/and push.gh pr mergecommand rejected by permission sandbox.Risks and Mitigations