Skip to content

Extract /collapse-pr: make the one-commit merge-queue invariant a single guarded operation #849

Description

@s-annam

Problem

main merges through a merge queue, and the queue's enqueue API
(EnqueuePullRequestInput) carries no commit-message fields — it accepts only
pullRequestId / jump / expectedHeadOid. GitHub therefore derives the squash
message from repo settings, which are (verified on offlinecv/OfflineCV):

squash_merge_commit_title:   COMMIT_OR_PR_TITLE
squash_merge_commit_message: COMMIT_MESSAGES
ruleset "main-merge-queue":  active, merge_method SQUASH

The lever that gives us: a PR holding exactly one commit merges with that commit's
subject and body verbatim.
A PR holding more than one merges with * -bulleted
commit soup in main's history.

So "the branch must arrive at the queue as exactly one commit" is a load-bearing
invariant
, not a style preference. Today that invariant is maintained by three
separate hand-rolled copies of the same logic, and they have already diverged:

Skill Collapses? Rule it applies
open-pr Step 3.6 yes always, before the PR exists
revise-pr Step 5.1 yes final round only — don't collapse while threads are open on the target, because the reviewer needs to diff just the delta
pr-review Step 5.5 no pushes an auto-fix commit and explicitly forbids force-push

pr-review is the gap. When it auto-fixes nits at 0 blockers it pushes a second
commit and leaves the branch multi-commit — silently breaking the invariant that
open-pr just established. Nobody notices until the soup is in main.

Each copy also re-derives the merge-queue rationale from scratch, so a fourth caller
(the planned implement-issue draft-PR review loop) would mean a fourth copy.

Why a skill, not a helper script

The collapse is not a mechanical git incantation — it is a judgment call with
safety gates
, and the gates differ per caller:

  • The combined message must be written, not concatenated. It describes the change
    as a whole, not the sequence of steps that produced it. wip / fix lint /
    address review commits are process, not change, and must not survive.
  • Collapsing is a force-push. main has dismiss_stale_reviews: true (verified), so
    collapsing after an approval dismisses that approval and deadlocks the PR — it
    can no longer enqueue without a fresh review.
  • Force-pushing a branch authored by someone else destroys their local work with no
    warning.

Design

1. New skill: .claude/skills/collapse-pr/SKILL.md

/collapse-pr [<pr>] [--message-file <f>] [--dry-run] [--yes]

Step 0 — Resolve the target, and never trust the local checkout.

Two call shapes reach this skill and they have different notions of "the branch":

Caller Target
open-pr Step 3.6 pre-push, no PR exists yet — the current checkout is the target
revise-pr 5.1, pr-review 5.5, standalone /collapse-pr <N> an existing PR, whose head lives at origin and may have nothing to do with local state

For every shape except the open-pr one, resolve the target from the remote:

gh pr view "$PR" --json headRefName,headRefOid,headRepositoryOwner
git fetch origin "$HEAD_REF"
BASE_SHA="$(git merge-base "origin/$HEAD_REF" "origin/$BASE_REF")"

and do the reset --soft + commit + push --force-with-lease in a throwaway worktree
checked out at origin/$HEAD_REF
— never in the user's checkout. Then remove the worktree.

This is not hypothetical. Dry-running the gates against live PR #842 found the local
checkout sitting on that PR's branch (epic-811-parser-lane) and diverged from origin:
local held one commit, origin held two, and the local commit was a superseded copy sharing
a subject line with one of origin's. Collapsing from that checkout would have force-pushed
the stale tree and destroyed a Windows path-separator test fix that existed only on origin.

Gate 3e resolves this losslessly — it does not ask a human. An earlier draft of this
issue made divergence a refusal. That was wrong twice over: it is friction in the common
case (the local copy is simply an older draft), and the question it defers — "is this local
work unique, or superseded?" — is not mechanically decidable. Verified: on #842 both
git cherry and a reverse-apply of the local patch report the local commit as not upstream,
even though origin held a strictly better amended version of the same change. Supersession is
semantic, not byte-identical. Do not try to classify it.

So make the operation lossless instead. If nothing can be lost, nothing needs deciding:

Local state Resolution Human?
Local-only commits on the target branch preserve at a real ref, then reset --hard origin/$HEAD_REF no
Uncommitted changes this run authored (explicit path list, e.g. pr-review's $FIXED_PATHS) stage by path, commit on top, let the collapse fold it in no
Uncommitted changes of unknown provenance refuse — absorbing stray edits into someone else's PR commit is a human's call yes
STAMP="$(date -u +%Y%m%dT%H%M%SZ)"
BACKUP="collapse-pr-backup/$HEAD_REF-$STAMP"
git branch "$BACKUP" "$(git rev-parse HEAD)"     # a real ref: reflog expires, this doesn't
git reset --hard "origin/$HEAD_REF"

Row 2 is not a bolted-on special case — it is exactly what pr-review Step 5.5 already does
(auto-fix → commit → collapse), so the gate must not refuse where 5.5 proceeds. Row 3 is the
repo's existing "never absorb stray edits" rule from open-pr and implement-issue; stage by
explicit path, never git add -A. Classify for the report only (behind / already-upstream /
possibly-unique) and never let that unreliable classification gate the action.

Validated live on #842: the local commit was preserved at
collapse-pr-backup/epic-811-parser-lane-20260815T234430Z (6ba92e9), the checkout reset to
72358ab, divergence went to 0 0, and the gate passed — with no human decision required.

Step 0b — The tree-identity assertion, and what it licenses.

A collapse is a pure history rewrite: reset --soft + re-commit changes which commits
exist
, never what the files contain. So the resulting commit's tree object must be
byte-identical to the pre-collapse head's tree. Assert it, and abort if it ever fails:

[ "$(git rev-parse "$PRE"^{tree})" = "$(git rev-parse HEAD^{tree})" ] || abort

This assertion is cheap, and it is the strongest correctness check available — it proves
content preservation directly rather than inferring it from a diff.

It also licenses skipping the pre-push hook. offlinecv installs a managed
.git/hooks/pre-push that runs the full npm run verify (bypass: OFFLINECV_SKIP_HOOKS=1),
and worktrees share .git/hooks, so it fires on a collapse push from a throwaway worktree —
where it fails immediately for want of node_modules. Bootstrapping node_modules there
just to re-run a suite is minutes of work to re-verify a tree that by construction did not
change
. So: assert tree identity first, and only then push with OFFLINECV_SKIP_HOOKS=1.
If the assertion fails, the tree did change, the skip is not licensed, and the whole
collapse must abort — never skip the hook unconditionally.

Step 1 — Detect the regime. Query squash_merge_commit_title /
squash_merge_commit_message and whether a merge-queue ruleset is active. If the repo
merges via plain squash where gh pr merge --squash --subject --body can supply the
message directly, collapsing is unnecessary — say so and exit 0. This is what keeps
the skill portable to any future repo instead of hard-coding offlinecv's settings.

Step 2 — Count commits. git log --oneline "origin/$BASE..HEAD" | wc -l. If 1,
no-op, exit 0. Idempotent by construction — safe to run twice.

Step 3 — Safety gates. Refuse (or require explicit --yes plus a printed warning)
when any of:

Gate Why
An approval exists and dismiss_stale_reviews is on the force-push dismisses it → PR can't enqueue → deadlock
Unresolved review threads exist on the target revise-pr 5.1's rule — the reviewer still needs to diff just the delta
PR head is not authored by the current user or an agent force-push silently destroys a contributor's local branch
The --force-with-lease lease would fail someone pushed while we worked; never fight it

Step 4 — Compose the message. Written, not concatenated. Conventional-commit
subject, Closes #N / Refs #N trailer, no AI attribution trailer (repo
convention). Accept --message-file so a human can hand-author it. Print it for
confirmation. --dry-run stops here.

Step 5 — Execute.

git reset --soft "$(git merge-base HEAD "origin/$BASE")"
git commit -F <message-file>
git push --force-with-lease origin HEAD     # never bare --force

2. Rewire the three existing callers

  • open-pr Step 3.6 → delegate to /collapse-pr. Keep the step (it runs
    pre-push, before the PR exists, so most gates are trivially satisfied) but stop
    re-deriving the rationale.
  • revise-pr Step 5.1 → delegate, keeping its own "final round only" decision.
    The decision stays in revise-pr; only the mechanics move.
  • pr-review Step 5.5new behavior, see below.

3. pr-review Step 5.5: collapse before the approval

The ordering is the whole point. pr-review Step 5 already documents the principle —
"the push happens before the approval, so there is no prior approval to dismiss"
and the collapse must ride in that same slot:

fix nits → commit → collapse to one commit → push --force-with-lease
        → derive inline anchors      (Step 5.5 already requires this order)
        → post APPROVE on the collapsed head

This requires relaxing one existing rule. Step 5.5 currently says
"never --force / --force-with-lease here." That rule exists to protect against
the author pushing mid-review — but --force-with-lease is precisely the mechanism
that detects that case. New rule:

Use --force-with-lease. If the lease fails, the author pushed while the review ran:
skip the collapse, fall back to the plain push, and note it in the review body.
Never bare --force.

Same protection, strictly more capability.

Author-class gate — collapse only when the PR head is agent-authored or owned by
the maintainer:

Author Auto-fix push Collapse (force-push)
Maintainer / agent-authored, in-repo branch yes yes
Named contributor, in-repo branch yes, announced no — destroys their local branch
Outside contributor (fork) usually blocked by permissions n/a

4. pr-review: suggestion blocks when pushing isn't allowed

Today a fork PR (or a contributor branch we won't force-push) falls back to
comments-only. GitHub's ```suggestion blocks are strictly better for that
case: the author applies them with one click, authorship and consent are preserved,
and no branch is touched.

Emit suggestion blocks for every Nit/Secondary finding that is a localized textual
replacement
in the diff, whenever Step 5.5 decides not to push. Findings that need
a behavioral change are not suggestions — keep those as prose.

Acceptance criteria

  • .claude/skills/collapse-pr/SKILL.md exists and implements Steps 1–5 above.
  • Regime detection exits cleanly (no-op, exit 0) on a repo where --subject/--body
    can supply the squash message — the skill is not offlinecv-hard-coded.
  • Running /collapse-pr on an already-single-commit branch is a no-op (idempotent).
  • All four safety gates from Step 3 are implemented and each states its reason when
    it fires.
  • Step 0 target resolution: for an existing PR, the head is resolved from origin
    and the rewrite happens in a throwaway worktree at origin/$HEAD_REF, never in the
    user's checkout. The worktree is removed afterward.
  • Tree-identity assertion: the collapsed commit's tree hash is compared to the
    pre-collapse head's and the collapse aborts on mismatch.
  • Pre-push hook handling: the push sets OFFLINECV_SKIP_HOOKS=1, and does so only
    after the tree-identity assertion passes — never unconditionally.
  • Gate 3e is lossless, not a refusal: local-only commits are preserved at a real
    collapse-pr-backup/<branch>-<utc> ref and the checkout is reset automatically; the
    report names the backup ref. Uncommitted changes this run authored are committed on
    top rather than blocking. Only uncommitted changes of unknown provenance refuse.
  • Gate 3e never blocks on a classification of whether local work is "unique" — that is
    not mechanically decidable and must not gate the action.
  • --dry-run prints the composed message and changes nothing.
  • --message-file lets a human supply the message verbatim.
  • open-pr 3.6 and revise-pr 5.1 delegate to /collapse-pr instead of carrying
    their own reset --soft + rationale. revise-pr retains its "final round only"
    decision.
  • pr-review 5.5 collapses before posting the review, with --force-with-lease,
    and falls back to a plain push when the lease fails.
  • pr-review 5.5 applies the author-class gate and never force-pushes a branch it
    does not own.
  • pr-review emits ```suggestion blocks for localized nits whenever it
    does not push.
  • No skill re-derives the merge-queue rationale; collapse-pr is the single source
    of truth and the others link to it.
  • The PR opened for this issue itself arrives at the queue as exactly one commit
    (dogfood).

Out of scope / follow-ups

  • /pr-watch — a self-pacing loop over pr-sweep with head-SHA state so unchanged
    PRs are not re-reviewed. Lives in the maintainer's ~/tools, not this repo.
  • Auto-merge gating (gh pr merge --auto behind an author-class + sensitive-path
    check). Depends on /pr-watch existing; file separately once this has run for a while.
  • Changing repo squash settings to PR_BODY/PR_TITLE. That would make commit count
    irrelevant, but the PR body would then land verbatim in git log (Summary / Review
    focus / Test plan sections included). Rejected as more invasive than the collapse.

Metadata

Metadata

Assignees

Labels

architectureSystem design / coupling / representation decisionsimprovementEnhancing existing functionality

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions