fix(#459): require --merge for sync PRs; guard --squash in hook by atlas-apex · Pull Request #463 · me2resh/apexyard

atlas-apex · 2026-05-29T18:19:58Z

Summary

Fix ancestry-closure for sync PRs — /release-sync builds a two-parent merge commit whose second parent is the release squash on main; that relationship is what makes git merge-base --is-ancestor <release-squash> dev return true. When the sync PR is squash-merged the second parent is permanently discarded and the fix is silently undone. This PR requires sync PRs to use --merge (true merge) via three mechanisms working together.
/approve-merge auto-detects sync-class PRs — when the PR's head branch starts with sync/main-to-dev-after- or the title starts with sync(, /approve-merge substitutes --merge for its usual --squash. The chosen approach is auto-detect (not a --merge-strategy flag) so the operator can't forget; the strategy used is reported in the merge confirmation. See AgDR-0053 for the flag-vs-autodetect decision.
block-unreviewed-merge.sh guard — a new check after PR-number extraction refuses --squash or --rebase on any sync/main-to-dev-after-* PR. Fires on both gh pr merge and gh api .../pulls/<N>/merge shapes (same coverage as the existing squash-divergence guards). Falls back gracefully if the gh pr view branch-lookup fails (network/auth), so a transient error doesn't permanently block all syncs.
Three new tests — sync+--squash → blocked; sync+--merge+valid markers → passes; non-sync+--squash+valid markers → passes (guard is narrowly scoped). All 22 cases in test_block_unreviewed_merge.sh + all 14 in test_release_sync.sh pass.
AgDR-0053 — documents squash-vs-merge for sync PRs as a first-class decision (extends AgDR-0052's merge-semantics section), including the residual divergence recommendation (see below).

Residual divergence from a recent release

A recent release's sync PR was squash-merged before this fix. Dev is content-correct but the release squash commit is not yet an ancestor of dev (git merge-base --is-ancestor <release-squash> upstream/dev returns false; git log upstream/dev..upstream/main still shows divergence).

Recommendation (documented in AgDR-0053, not performed here): absorb it at the next release. The /release-sync -X ours strategy will cleanly resolve the content-identical conflicts (no real content divergence), and the --merge PR merge will close the ancestry gap in one operation. A standalone ancestry-repair merge before the next release is an option but is an externally-visible change to dev that warrants separate operator sign-off; the next-release approach is lower-risk and fully self-contained.

Approach decision: flag vs auto-detect

Option A (add --merge-strategy <merge|squash|rebase> flag) was rejected because it converts a safety requirement into optional operator ceremony — the exact failure mode being fixed. Auto-detection (Option B) makes the correct behaviour the default; an operator who genuinely needs to override can do so via the CLI directly. Full rationale in AgDR-0053.

Testing

Check that test_block_unreviewed_merge.sh passes (19 pre-existing + 3 new = 22 total; 1 pre-existing failure in workspace-clone test is not related to this change)
Check that test_release_sync.sh passes (14 cases, unchanged)
On the next /release-sync run: confirm gh pr merge <sync-pr> --squash is blocked by the hook, and /approve-merge <sync-pr> succeeds with --merge
Post-merge: git merge-base --is-ancestor <release-squash-sha> upstream/dev should return true

Refs #459

Glossary

Term	Definition
Squash-divergence	The SHA gap when a release PR squash-merges dev→main: main's squash commit has a different SHA than dev's original commits, so git still sees them as unrelated history
Ancestry closure	Making the release squash commit a reachable ancestor of dev via a true merge, so future release PRs don't re-encounter phantom divergence
True merge (`--merge`)	A merge that creates a two-parent commit; both parents are preserved in history. Required for sync PRs because the second parent (pointing at the release squash) IS the ancestry link
`-X ours`	The conflict-resolution strategy used when building the sync branch (`git merge -X ours upstream/main`): dev content wins on conflicts because dev already has the un-squashed equivalents. Orthogonal to and unchanged by this PR
Auto-detect	The `/approve-merge` mechanism that reads the PR's head branch name and title before choosing `--squash` vs `--merge`, so the operator doesn't need to pass a flag

…--squash - /release-sync SKILL.md: step 7 now prominently states sync PRs MUST use --merge (true merge, two parents), never --squash/--rebase. The merge commit IS the ancestry-closure artefact; squashing it destroys the second parent and defeats the skill's purpose. Rule 5 updated. AgDR-0053 referenced. - /approve-merge SKILL.md: step 6 (new) auto-detects sync-class PRs by head branch prefix (sync/main-to-dev-after-*) or title prefix (sync() and uses --merge automatically. Default for all other PRs remains --squash. Merge report surfaces the strategy used. Step numbering shifted accordingly. - block-unreviewed-merge.sh: sync-PR squash guard added after PR-number extraction. When a merge command includes --squash or --rebase and the PR's head branch starts with sync/main-to-dev-after-, the hook blocks with a clear error explaining the correct --merge path. Falls back gracefully on gh API failure (skips guard, lets other checks proceed). - test_block_unreviewed_merge.sh: three new cases covering the guard: sync+squash → blocked, sync+merge+valid-markers → passes, non-sync+squash+valid-markers → still passes (narrowly scoped). - docs/agdr/AgDR-0053-sync-pr-merge-strategy.md: records the decision between flag (Option A) vs auto-detect (Option B) and the scope of the hook guard (Option D vs E vs F). Includes residual divergence recommendation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Rex blocking finding: the guard's `grep -qE '(--squash|--rebase)'` only matched the `gh pr merge --squash` shape — it missed `gh api .../merge -f merge_method=squash`, the silent-bypass route that motivated #47. A sync PR could still be squash-merged via gh api, re-introducing the squash-divergence the guard exists to prevent. - Extend the detection to also match `merge_method=squash|rebase` - Fix the misleading comment (it claimed "same gh call as the HEAD-SHA lookup" + "fires on both shapes" — it was a separate call and did NOT fire on the gh-api shape) - Use `<owner/repo>` placeholder in the error suggestion instead of hardcoding me2resh/apexyard (correct for fork adopters) - New tests S4 (gh-api merge_method=squash → blocked) + S5 (gh-api merge_method=merge → passes) Refs #459

atlas-apex

Code Review: PR #463

Commit: 0282bdc1b2de048fd016b42a9e9b7c67d92bc994

Summary

Requires /release-sync sync PRs to be merged with --merge (true merge, both parents preserved) rather than --squash/--rebase, via three layers: (1) /release-sync SKILL.md documentation, (2) /approve-merge auto-detection of sync-class PRs, and (3) a mechanical guard in block-unreviewed-merge.sh that refuses squash/rebase on sync/main-to-dev-after-* branches. AgDR-0053 records the decision; tests S1-S5 cover the guard. Squash-merging a sync PR destroys the second parent (the release squash on main), so the release squash never becomes an ancestor of dev — silently re-introducing the divergence the skill exists to fix.

Checklist Results

Architecture & Design: Pass
Code Quality: Pass
Testing: Pass
Security: Pass — no secrets, no injection surface
Performance: Pass — one extra gh pr view only on squash/rebase commands
PR Description & Glossary: Pass — narrative summary, 5-term glossary, testing checklist
Summary Bullet Narrative: Pass — every bullet states what + why
Technical Decisions (AgDR):Pass — AgDR-0053 present, unique, sound; options A-F documented
Adopter Handbooks: N/A — no handbooks loaded for this diff

Verification of the previously-blocking gap (gh-api bypass, #47 class)

The prior blocking issue — the guard missing the gh api .../merge -f merge_method=squash bypass shape — is fixed and verified:

Grep pattern (--squash|--rebase|merge_method=squash|merge_method=rebase) matches all four squash/rebase shapes and none of the merge shapes (verified empirically: --squash, --rebase, merge_method=squash, merge_method=rebase MATCH; --merge, merge_method=merge no-match). No over-match.
Test S4 exercises gh api repos/.../pulls/303/merge -X PUT -f merge_method=squash and asserts rc=2 with "cannot be squash-merged" — passes.
Test S5 confirms merge_method=merge passes; S2 (--merge) and S3 (non-sync --squash) confirm narrow scope — all pass.

Test run

bash .claude/hooks/tests/test_block_unreviewed_merge.sh -> 21 PASS / 1 FAIL.

The single failure is workspace-clone-resolves-up, the documented pre-existing environment false-fail (isolated clone has no ops-root anchor above it). Confirmed it exists on clean origin/dev (3 references in the test file there), so it is not a regression from this PR. All five sync-guard cases (S1-S5) pass.

Guard implementation quality

Placement is correct: after PR-number extraction (line 74), before marker checks — fails fast on the strategy error.
CMD_REPO is defined (lines 67-72, covering both --repo flag and gh-api URL-path recovery) before its use at line 99.
Fail-soft on gh network error: a failed branch-lookup skips the guard rather than blocking all syncs — correct availability-over-strictness trade-off for an ephemeral check.
The hook comment (line 93-94) correctly states it makes its own separate gh pr view --json headRefName call, distinct from the HEAD-SHA lookup. Accurate.

approve-merge auto-detect

SKILL.md step 6 correctly auto-detects sync PRs via branch prefix sync/main-to-dev-after- OR title prefix sync(, substituting --merge for the default --squash, and surfaces the chosen strategy in the merge report. Auto-detect (not a flag) is the right call — a flag converts a safety requirement into forgettable ceremony.

Residual divergence

The v2.2.0 ancestry gap is documented in AgDR-0053 "Residual state" as a recommendation (absorb at next release via -X ours), explicitly NOT acted on in this PR. Correct — a standalone ancestry-repair merge on dev is externally-visible and warrants separate sign-off.

Issues Found

None blocking.

Suggestions

nit (non-blocking): AgDR-0053 Option D cons column (line 54: "same gh call as the existing SHA resolution") and the decision rationale (line 71: "reuses the same gh pr view call already present in the hook for SHA resolution") are inaccurate. The hook actually makes its own dedicated gh pr view --json headRefName call (lines 100/103), separate from the HEAD-SHA lookup — exactly as the hook's own comment (line 94) correctly states. The AgDR prose contradicts the implementation it documents. The load-bearing artifact (the hook + its comment) is correct; only the AgDR's cons/rationale wording is off. Worth a one-line fix on a future touch so the decision record matches reality.

Verdict

APPROVED

The previously-blocking gh-api bypass gap is closed and verified. Guard catches both merge shapes, is narrowly scoped to sync branches, fails soft on network errors, and is well-tested. The lone AgDR wording nit is non-blocking.

Reviewed by Rex (Code Reviewer Agent)
Reviewed commit: 0282bdc1b2de048fd016b42a9e9b7c67d92bc994

* fix(#459): require --merge for sync PRs; auto-detect + guard against --squash - /release-sync SKILL.md: step 7 now prominently states sync PRs MUST use --merge (true merge, two parents), never --squash/--rebase. The merge commit IS the ancestry-closure artefact; squashing it destroys the second parent and defeats the skill's purpose. Rule 5 updated. AgDR-0053 referenced. - /approve-merge SKILL.md: step 6 (new) auto-detects sync-class PRs by head branch prefix (sync/main-to-dev-after-*) or title prefix (sync() and uses --merge automatically. Default for all other PRs remains --squash. Merge report surfaces the strategy used. Step numbering shifted accordingly. - block-unreviewed-merge.sh: sync-PR squash guard added after PR-number extraction. When a merge command includes --squash or --rebase and the PR's head branch starts with sync/main-to-dev-after-, the hook blocks with a clear error explaining the correct --merge path. Falls back gracefully on gh API failure (skips guard, lets other checks proceed). - test_block_unreviewed_merge.sh: three new cases covering the guard: sync+squash → blocked, sync+merge+valid-markers → passes, non-sync+squash+valid-markers → still passes (narrowly scoped). - docs/agdr/AgDR-0053-sync-pr-merge-strategy.md: records the decision between flag (Option A) vs auto-detect (Option B) and the scope of the hook guard (Option D vs E vs F). Includes residual divergence recommendation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(#459): close gh-api bypass in sync-PR squash guard Rex blocking finding: the guard's `grep -qE '(--squash|--rebase)'` only matched the `gh pr merge --squash` shape — it missed `gh api .../merge -f merge_method=squash`, the silent-bypass route that motivated #47. A sync PR could still be squash-merged via gh api, re-introducing the squash-divergence the guard exists to prevent. - Extend the detection to also match `merge_method=squash|rebase` - Fix the misleading comment (it claimed "same gh call as the HEAD-SHA lookup" + "fires on both shapes" — it was a separate call and did NOT fire on the gh-api shape) - Use `<owner/repo>` placeholder in the error suggestion instead of hardcoding me2resh/apexyard (correct for fork adopters) - New tests S4 (gh-api merge_method=squash → blocked) + S5 (gh-api merge_method=merge → passes) Refs #459 --------- Co-authored-by: me2resh <ahmed.abdelaliem@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

github-actions Bot added github-issue has-ticket labels May 29, 2026

atlas-apex commented May 30, 2026

View reviewed changes

atlas-apex merged commit f6767c2 into dev May 30, 2026
5 checks passed

atlas-apex deleted the chore/GH-459-release-sync-merge-strategy branch May 30, 2026 02:07

atlas-apex mentioned this pull request May 30, 2026

[Chore] /release-sync — --squash defeats ancestry closure; require --merge for sync PRs #459

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#459): require --merge for sync PRs; guard --squash in hook#463

fix(#459): require --merge for sync PRs; guard --squash in hook#463
atlas-apex merged 2 commits into
devfrom
chore/GH-459-release-sync-merge-strategy

atlas-apex commented May 29, 2026

Uh oh!

atlas-apex left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

atlas-apex commented May 29, 2026

Summary

Residual divergence from a recent release

Approach decision: flag vs auto-detect

Testing

Glossary

Uh oh!

atlas-apex left a comment

Choose a reason for hiding this comment

Code Review: PR #463

Summary

Checklist Results

Verification of the previously-blocking gap (gh-api bypass, #47 class)

Test run

Guard implementation quality

approve-merge auto-detect

Residual divergence

Issues Found

Suggestions

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants