fix(#550): tag the squash commit on main + ancestry guard in /release#565
Conversation
Step 6 (Tag + push) now explicitly: - Requires tagging upstream/main (the squash commit) not the release-branch HEAD - Includes a mandatory ancestry guard before pushing the tag: git merge-base --is-ancestor vA.B.C upstream/main exits non-zero → error + abort before the mis-placed tag is pushed - Adds a post-tag release checklist with three assertions (ancestry, git-describe discoverability, and exact-tip equality) - Notes that v2.3.0 re-pointing is operational / maintainer-handled (out of scope) Rule 6 updated to name the squash-merge mechanics and reference step 6. Refs #550
atlas-apex
left a comment
There was a problem hiding this comment.
Code Review: PR #565
Commit: `1c7b261f3a94a7c2af93308f6a8c4744e0118f7c`
Summary
Rewrites step 6 (Tag + push) of the /release skill so the vA.B.C tag is placed on the squash commit that lands on main (git tag vA.B.C upstream/main) rather than the release-branch HEAD, adds a pre-push ancestry guard (git merge-base --is-ancestor, exit 1 on failure), and a post-tag verification checklist. Rule #6 is updated to match. Fixes the mis-tag class from #550. The one-time v2.3.0 re-point is correctly scoped out.
Checklist Results
- Architecture & Design: Pass (docs/skill-only)
- Code Quality: Pass
- Testing: Pass (markdownlint clean; guard logic verified empirically)
- Security: Pass
- Performance: N/A
- PR Description & Glossary: Pass (4-term glossary, narrative summary bullets)
- Technical Decisions (AgDR):N/A (no new library/pattern/architecture)
Verification against the review brief
Tagging target + guard — correct & unambiguous. Tags upstream/main (explicitly named as "the squash commit"), with prose stating "never the release-branch HEAD." The guard asserts git merge-base --is-ancestor vA.B.C upstream/main.
Bash sound — fetch → tag → guard → push. Ordering is correct: the guard runs and exit 1s before git push upstream vA.B.C, so a mis-tag can never reach the public remote. Dropping the main arg from git fetch upstream main → git fetch upstream is an improvement — fetching all refs reliably updates the upstream/main remote-tracking ref the next two commands depend on.
Squash mechanics prose — correct. Accurately states that a squash-merge creates a single new commit on main whose SHA differs from every source-branch commit, and that the release-branch HEAD is therefore no longer in main's ancestry (breaking git describe / git merge-base).
Checklist assertions — verified empirically. git merge-base --is-ancestor is reflexive (exit 0 when tag == tip), so tagging upstream/main directly always satisfies the guard. git rev-parse vA.B.C^{} peels a lightweight tag to its commit and equals git rev-parse upstream/main at a clean release; the third bullet correctly caveats the post-merge-commits case.
No contradiction with the rest of the skill. Rule #6 now reads "never tag before merge, and never tag the release-branch HEAD" and cross-references step 6 — consistent. Step 3.5 (pre-merge site-version bump in the release commit) and step 9 (/release-sync vA.B.C merging upstream/main into dev) both align with the new step 6's upstream/main target; no overlap or conflict.
markdownlint: 0 errors (markdownlint-cli2 v0.22.1).
No leaks: only me2resh/apexyard + generic version placeholders.
Issues Found
None.
Suggestions
- nit (non-blocking): the
--tag vA.B.Cinvocation in the step-6 lead-in still implies/release --tagruns these commands; the manual block is what actually changed. Fine as-is — the parenthetical "(or runs the suggested commands manually)" covers it.
Verdict
APPROVED
🤖 Reviewed by Rex (Code Reviewer Agent)
📌 Reviewed commit: `1c7b261f3a94a7c2af93308f6a8c4744e0118f7c`
Summary
main's ancestry. The updated bash block fetchesupstreamfirst, then tagsupstream/main(the exact squash commit), making the mis-tag mechanically impossible when the prescribed commands are followed.git merge-base --is-ancestor vA.B.C upstream/mainruns immediately after tagging and aborts with a descriptive error if the tag is off-ancestry. This catches any remaining mis-tag (e.g. a manual override) before it reaches the public remote.merge-base,git describe, and exact-tip equality) give the operator a repeatable verification step before announcing the release, preventing silent ancestry drift.Testing
npx markdownlint-cli2 .claude/skills/release/SKILL.md— confirmed 0 errors./releaseskill (it is a markdown/prose spec, unlike/release-syncwhich hastest_release_sync.sh). Verification is markdownlint-clean output + correctness of the guard logic.git merge-base --is-ancestor <tag> upstream/mainexits 0 only when the tag commit is reachable fromupstream/main. Taggingupstream/maindirectly (as prescribed) always satisfies this, while tagging the release-branch HEAD (the previous accidental pattern) does not — confirmed by the repro steps in [Bug] release tag can land off main's ancestry (v2.3.0 mis-tagged) #550.Refs #550
Glossary
mainwhen a PR is squash-merged; its SHA differs from every commit on the source branch and is the only valid tagging target after a squash-merge.git describegit describe --tags upstream/mainandgit merge-base --is-ancestorboth depend on this property to work correctly.release/vX.Y.Zbranch before it is merged; after a squash-merge this commit is not inmain's history and must not be used as a tagging target.git merge-base --is-ancestorassertion added in step 6 that aborts the tag-push workflow if the newly created tag is not reachable fromupstream/main, preventing a mis-placed tag from reaching the public remote.