feat(skills): extract /collapse-pr as the one guarded collapse

[s-annam]

Summary

main merges through a merge queue whose enqueue API carries no commit-message fields, so GitHub derives the squash message from squash_merge_commit_message: COMMIT_MESSAGES. Only a single-commit PR merges with a written message; anything else lands in main as * -bulleted commit soup.

That invariant was maintained by three hand-rolled copies that had already diverged — and pr-review broke it outright, pushing an auto-fix commit while explicitly forbidding the force-push needed to restore one commit.

This extracts .claude/skills/collapse-pr/ as the single owner of both the mechanics and the rationale. open-pr, revise-pr and pr-review keep only their own decision about whether to collapse.

Closes #849.

What the skill enforces

• Target resolution. An existing PR's head is read from origin and rewritten in a throwaway worktree; the local checkout is never assumed to be the target. --in-place lets a caller that genuinely owns the checkout say so, instead of inferring mode from whether a PR happens to exist.
• Tree-identity assertion. The collapsed tree must be byte-identical to the pre-collapse head. That assertion — and only it — licenses skipping the managed pre-push hook, and only in a throwaway worktree, never in place where the hook is the sole local verification.
• Five gates. Stale approval and open threads are soft (--yes proceeds); branch ownership and the push lease are hard and never overridable.
• Gate 3e is lossless, not a refusal. Whether local work is unique or merely superseded is not mechanically decidable, so the skill does not try: commits are preserved at a collapse-pr-backup/<branch>-<utc> ref before any reset, and creating that ref is a hard precondition of the reset.
• pr-review ordering. Collapse sits between the auto-fix push and the review post — the only ordering that works, since the collapse rewrites every SHA (so anchors must come after) and dismiss_stale_reviews: true means collapsing after an approval would dismiss it. The author's message is preserved via --message-file rather than replaced with one the reviewer wrote.

Review focus

1. collapse-pr gate 3e (Step 3). The most destructive code here — it runs git reset --hard on a checkout. Check the backup ref is an unconditional precondition on every path, and that the [ "$MODE" = worktree ] guards cannot be bypassed.
2. Row 1's cherry-pick path. Added late to stop a naive push HEAD from publishing local-only commits. Least-reviewed code in the diff.
3. pr-review Step 5.5 ordering — that no route reaches a force-push after an abandoned auto-fix.
4. Length. collapse-pr is 1083 lines. Growth is gates and guarded paths; a reviewer may reasonably disagree about how much prose earns its place.

Test plan

• npm run verify green (also enforced by the pre-push hook on this branch — not bypassed).
• All four in-scope skill frontmatters parse as strict YAML. (pr-ready does not — pre-existing, filed as pr-ready SKILL.md frontmatter is invalid YAML (unquoted argument-hint flow sequence) #850, deliberately untouched here.)
• Single-source-of-truth verified: only collapse-pr restates the merge-queue rationale; open-pr and revise-pr carry zero reset --soft.
• All cross-referenced step and row numbers resolve across the five files.
• This PR arrives as exactly one commit, dogfooding its own invariant.
• Collapse mechanics rehearsed live against PR fix(heuristics): recover dropped roles, titles and skills sections #842: 2 commits → 1, tree hash 56a742c98fb9d6afe1370ab5f9432c82abc6c5d3 identical on both sides, zero diff. --force-with-lease verified on both paths — valid lease accepted, stale lease refused with the remote unchanged.

Adversarial review

Three rounds ran before this PR was opened. Findings and dispositions:

Round 1 — 5 blocking, all fixed

1. Step 5b's git worktree remove --force was unguarded; in inplace mode $WORKDIR is the user's checkout, and linked worktrees delete cleanly. Would have deleted the worktree it was working in.
2. OFFLINECV_SKIP_HOOKS=1 was applied on both push paths — silently removing npm run verify from open-pr entirely, since the hook was its only local verification.
3. trap … EXIT cannot span separate tool calls, so cleanup either fired immediately or never; two prose claims asserted otherwise.
4. pr-review step 5 ran the collapse on paths that had pushed nothing.
5. The collapse replaced the author's commit message with one the reviewer composed.

Round 2 — 7 blocking, all fixed

1. Backup ref and reset --hard were unconditional siblings; a failed git branch fell through to irreversible loss (the skill does not run under set -e).
2. Gate 3e's worktree-only restriction was prose with zero MODE references in any of its five code blocks — repeating round 1's lesson.
3. --authored-path whitelists path names, but provenance is content in a specific checkout; a global list applied across worktrees could publish a user's unrelated edits. No iteration construct existed for multiple matches.
4. Row 1's push HEAD published every local-only commit — the fix(heuristics): recover dropped roles, titles and skills sections #842 hazard, ungated.
5. A rejected row-1 push was misattributed solely to gate 3d; being behind, or the pre-push hook, are likelier causes.
6. status --porcelain includes ??, and .git/info/exclude un-ignores node_modules, so gate 3e would have refused 100% of the time on this repo.
7. Re-running open-pr after a PR exists flipped mode to worktree, collapsed origin's head, and discarded local commits while reporting success.

Plus 7 nits (rename/space-safe status parsing, bash 3.2 set -u array expansion, teardown naming on exit paths, and others).

Round 3 added ~230 lines (cherry-pick path, --authored-worktree, --in-place, real mode guards) which no subagent review has seen — reviewing that here is the point of opening the PR.

A note on process: a solo verification pass returned "clean" before each of rounds 1 and 2, which then found 5 and 7 blocking defects respectively. Treat the sections above as the areas where direct inspection has been demonstrably unreliable.

[cloudflare-workers-and-pages]

Deploying offlinecv with    Cloudflare Pages

Latest commit:	f860dae
Status:	✅  Deploy successful!
Preview URL:	https://54222e7a.offlinecv.pages.dev
Branch Preview URL:	https://gh-849.offlinecv.pages.dev

View logs

[s-annam]

An extremely thorough and maintainer-grade implementation of the /collapse-pr skill and its integration across the PR lifecycle.

---

Reviewed by: Gemini 3.5 Flash (high)

[s-annam]

This comment suggests running the backup commands of Row 3 here. It would make the skill more self-contained and robust to explicitly include the Row 3 backup commands (specialized for $FIX_SHA) inside the else block instead of a placeholder comment.

[s-annam]

Reviewed the whole diff against #849's acceptance criteria, not against the description — which I read last, per this skill's own ordering rule. All 17 ACs of #849 are met, the regime/gate probes all behave live as documented, and the description is accurate on every claim I could check. The three blockers below are all in the ~230 lines of round 3 that no prior review has seen, and all three sit inside collapse-pr gate 3e row 1 — the code the ## Review focus section correctly named as the least-reviewed in the diff.

Verdict rule applied: normally ≥1 Blocking → REQUEST_CHANGES. This review posts COMMENT under an explicit maintainer override reserving the verdict for themselves on this PR. The same override disabled Step 5.5 (no auto-fix, no commit, no push, no /collapse-pr invocation — the collapse code is what this PR changes, and running it for the first time on the PR that contains it is not a risk worth taking) and forbade any enqueue or auto-merge. Nothing was merged, approved, or enqueued; the branch is exactly as I found it.

Prior rounds — confirmed still fixed

• Round 1 (5 blocking): Step 5b is guarded by if [ "$MODE" = worktree ] (L986); OFFLINECV_SKIP_HOOKS=1 is worktree-mode-only via the SKIP=() array (L915); the trap is gone and replaced with an explicit "run Step 5b on every exit path" contract (L162-166); pr-review step 5 opens with "Only if step 4 pushed"; the collapsed message comes from --message-file holding git log -1 --format=%B "$PRE_FIX_SHA". ✅
• Round 2 (7 blocking): the backup ref is chained with || and re-verified by rev-parse before the reset (L738-749); 3e's mode restriction is now real code in 7 places, not prose; --authored-worktree scopes the whitelist and the while IFS= read -r WT loop exists; row 1's push is ancestry-tested rather than a bare push HEAD; the rejection-triage table has three causes; --untracked-files=no is used everywhere (.git/info/exclude:18 is indeed !/node_modules — verified); --in-place exists and open-pr passes it. ✅

I re-report none of these.

Blocking

Three findings, all one code path: collapse-pr gate 3e row 1.

B1 — Row 1 has no "did we author anything?" precondition, so it publishes the user's local-only commit

Row 1's table entry fires on "uncommitted changes this run authored", but the executable blocks carry no such condition — and the iterate block (L524-529) explicitly says the rows are the loop body, run for every $WT. So on a matched worktree that is clean but diverged, row 1 still executes:

1. $WT != $AUTHORED_WORKTREE → DIRTY_COUNT is 0 → no refusal (L573-580).
2. git add stages nothing; git commit fails "nothing to commit" — and L760-762 of this same file states that an unchained nonzero exit is silently discarded in this execution model.
3. WT_PRE_FIX_SHA is therefore the user's local-only commit.
4. merge-base --is-ancestor "$WT_PRE_FIX_SHA" "origin/$HEAD_REF" → false → else branch.
5. FIX_SHA = the user's commit → git -C "$WORKDIR" cherry-pick "$FIX_SHA" → push origin "HEAD:$HEAD_REF".

The user's unreviewed local commit is published into someone else's PR, and the Step 6 report describes that checkout as row 3 "preserved + reset" — it never says the commit was also pushed.

This reproduces on the skill's own flagship scenario. In the #842 worked example (L794-803) the main checkout was clean and held exactly one local-only commit, 6ba92e9. Under this code that commit is cherry-picked onto origin's head and pushed into PR #842. Round 2 finding 4 narrowed this hazard from "all local-only commits" to "the tip local-only commit" — it did not remove it, because row 1 still runs on worktrees it has no business touching.

Fix: give row 1 a real precondition, in code, mirroring the mode guard's own "code, not prose" standard:

[ "$MODE" = worktree ] || exit 1
[ -n "${AUTHORED_WORKTREE:-}" ] && [ "$WT" = "$AUTHORED_WORKTREE" ] || { echo "row 1: not the authored checkout — skipping to row 2"; ROW1=skip; }
DIRTY_COUNT="$(git -C "$WT" status --porcelain --untracked-files=no -z | tr -dc '\0' | wc -c | tr -d ' ')"
[ "$DIRTY_COUNT" != 0 ] || { echo "row 1: nothing dirty in $WT — no fold-in needed"; ROW1=skip; }

and make the commit/ancestry/push blocks no-ops when ROW1=skip. The non-authored-worktree branch should fall through to rows 3/4, never into the commit-and-publish machinery.

B2 — The cherry-pick path's push is rejected 100% of the time on this repo, and the triage table misdiagnoses it

git -C "$WORKDIR" cherry-pick "$FIX_SHA"
git -C "$WORKDIR" push origin "HEAD:$HEAD_REF"

$WORKDIR is the throwaway worktree from git worktree add --detach under mktemp -d. Worktrees share .git/hooks (core.hooksPath is unset — verified), so .git/hooks/pre-push fires and runs npm run verify in a directory with a package.json and no node_modules. That is the exact failure Step 0b describes at L199-202 as the reason the bypass exists — but this push carries no OFFLINECV_SKIP_HOOKS=1 and no --no-verify.

The rejection-triage table then sends the reader the wrong way (L688):

| the rejection came from .git/hooks/pre-push | row 1's push is a plain push from a real checkout, so the managed hook fires … | fix the failing gate. Never set OFFLINECV_SKIP_HOOKS=1 here |

"a real checkout" is true of the if branch only. In the else branch the push comes from the throwaway worktree, the gate is not red — the dependencies are simply absent — and "fix the failing gate" is unactionable. The run wedges after a backup ref has already been created, with no defined next step.

Fix: decide the licence explicitly for this push. The cherry-picked content is new relative to origin, so Step 0b's tree-identity licence does not cover it — either npm install in $WORKDIR before this push, or push the fix from $WT on a temporary branch and fast-forward, or state that this path requires a bootstrapped worktree. Then split the triage row into "if-branch (real checkout)" and "else-branch ($WORKDIR)" with different answers.

B3 — In the cherry-pick path the backup ref is a comment, not code, and the prose then tells row 3 not to create one

L646:

  # ... row 3's backup of $FIX_SHA runs here, with its own `||` refusal ...

That placeholder is the only thing standing between this path and an unbacked reset --hard, and L659-662 then instructs:

Row 3 is already half-done for this $WT. The backup above covered the local-only commits and the fix commit on top of them, so when row 3 reaches this checkout it must not mint a second ref — only the reset --hard remains.

But row 3's code (L721-752) has no "already backed up" check. So an agent gets one of two outcomes, and neither is the intended one: it follows the code and mints a second ref (harmless, but contradicts the report contract), or it follows the prose, skips row 3's backup, and — because L646 was never expanded into real commands — runs git reset --hard "origin/$HEAD_REF" with no backup ref at all. That is precisely the irreversible-loss shape round 2 finding 1 was blocking for, on the one path where the loss cannot be undone.

This is also the same defect class as round 2 finding 2: a safety property stated in prose inside a code block instead of being executable.

Fix: inline the actual backup commands (the while name-search, git branch … || refuse, and the rev-parse verification) into the else branch, and give row 3 an explicit [ -n "${BACKUP:-}" ] short-circuit so it resets without re-minting. Report the one ref, as L659-662 intends.

Secondary

1. collapse-pr L579 — the row-2 refusal does not run Step 5b in code. [ "$DIRTY_COUNT" = 0 ] || { echo …; exit 1; } carries a comment saying "Run Step 5b before stopping, like every refusal in this skill" — but the block just exits. This fires in worktree mode, the one mode where Step 5b is not a no-op, so it orphans both the linked worktree and $WORKDIR_PARENT under /tmp. Every other refusal in the skill states the teardown in surrounding prose; this one contradicts itself inline.

2. pr-review L581-587 — the cut -c4- subtraction errs in the unsafe direction, not the safe one. The prose claims: "it errs by over-subtracting, which drops a path to row 2 and refuses. That is the safe direction." It does the opposite. git status --porcelain -z emits a rename as two records — R <new>\0 then a bare <orig>\0 with no XY prefix (verified). cut -c4- chops three characters off that bare second field, so src/foo.ts becomes /foo.ts and fails to subtract src/foo.ts from $FIXED_PATHS. The path stays whitelisted, reaches 3e as --authored-path, and the user's in-progress rename can be committed into the PR — the exact outcome the paragraph exists to prevent. Narrow trigger (the reviewer must have edited a path the user is mid-rename on), but the stated safety argument is inverted. Fix: parse the same way collapse-pr L586-594 already does, or drop only entries whose record began with a status prefix.

3. pr-review L560-565 — the partial-failure recovery rationale does not hold. "if step 4 committed nothing, or committed and failed to push, the fixes are sitting uncommitted and 3e can only fold them in if it knows they are ours." If step 4 committed and the push failed, the fixes are committed, not uncommitted — 3e row 1 never sees them, row 3 backs them up and reset --hards them out of the checkout. And that state cannot arise in-run anyway, because step 5 opens with "Only if step 4 pushed." On an actual re-run the leftover commit is local-only, so row 3 quietly relocates the previous run's fix to a collapse-pr-backup/… ref and both skills report success. Either describe that outcome or add a "leftover fix commit" check to step 1.

4. collapse-pr L111 — swallowed error decides the destructive mode. PR_NUM="${PR_NUM:-$(gh pr view --json number -q .number 2>/dev/null || true)}": an auth failure, rate limit, or network blip yields an empty PR_NUM, and L113's [ -z "$PR_NUM" ] then selects MODE=inplace — the mode that rewrites the user's own checkout. This is the "swallowed errors — 2>/dev/null || true that hides auth/rate-limit, not just 'already exists'" antipattern from pr-review gate 3e, applied to a mode decision. Distinguish "no PR exists" (exit 1 with the no pull requests found message) from "the call failed" and refuse on the latter.

5. collapse-pr L598-600 — the normal path runs a command that errors. In the documented happy case (pr-review step 4 already committed $FIXED_PATHS, so "gate 3e sees them clean") row 1 still reaches git commit -m … on a clean tree and fails. Harmless in outcome, but the skill's own execution model has no set -e, so the run continues past a nonzero exit on its main path — and an agent that does stop on it aborts a collapse that should have succeeded. Same root cause as B1; fixing B1's precondition fixes this.

Nits

1. collapse-pr L508 — MATCHES_FILE (and MSG_FILE at L859 when it is a mktemp) are never removed; Step 5b tears down the worktree but not the temp files.
2. collapse-pr L729-732 — the backup-name search is TOCTOU against a concurrent run: two runs can both find -1 free, one loses git branch, and the || then prints "Fix the ref store and re-run", which misdiagnoses a name race as a broken ref store. Worth a distinct message for exit 128 / "already exists".
3. collapse-pr L886-890 — the net-no-change branch exits 1. pr-review step 5 reads any non-zero as "a hard gate refused" and reports it as such. A distinct exit code, or a note in pr-review, would keep the report honest.
4. docs/CONTRIBUTING-PROCESS.md §Squash messages still carries a full copy of the merge-queue derivation, while collapse-pr L64 declares itself "the single source of truth for the merge-queue rationale" and the test plan says "only collapse-pr restates" it. Both are defensible (the doc is the rationale file, and the same diff says collapse-pr "holds the canonical one"), but the three statements do not quite agree.
5. collapse-pr L584-585 — "git quotes paths containing spaces" is given as a reason to avoid cut -c4-. In -z mode git never quotes; the rename-mangling argument alone carries the point.

AC checklist — #849

All 17 acceptance criteria met. Spot-verified live rather than read: regime detection (mergeQueue.configuration.mergeMethod → SQUASH, squash_merge_commit_title → COMMIT_OR_PR_TITLE, squash_merge_commit_message → COMMIT_MESSAGES), gate 3a's dual probe (branch protection returns true; the ruleset probe returns unknown, and the documented first | if . == null guard is what keeps that from reading as false), the --in-place mode contract, the tree-identity assertion, 3e's losslessness, --dry-run holding 3e's writes back, and the dogfood criterion (the PR holds exactly 1 commit).

The one place implementation departs from the issue is pr-review 5.5's ordering: #849 §3 sketched fix → commit → collapse → push, the PR ships fix → commit → plain push → collapse. That is a correct departure, not a gap — /collapse-pr reads the head from origin, so a still-local fix commit would not be in the collapse — and the AC as written ("collapses before posting the review, with --force-with-lease, falls back to a plain push when the lease fails") is satisfied.

Description accuracy (gate 3f) — accurate

Every checkable claim round-tripped to the diff and held:

• npm run verify green — verify, fallow, and Cloudflare Pages all SUCCESS. ✅
• Four in-scope frontmatters parse as strict YAML and pr-ready does not — verified with a strict parser; pr-ready fails exactly as described (argument-hint flow sequence), and it is untouched by this diff. ✅
• open-pr and revise-pr carry zero reset --soft — confirmed; the only remaining occurrence repo-wide is pr-review's unrelated auto-fix revert. ✅
• Cross-referenced step and row numbers resolve — spot-checked every cross-file reference in the diff (3d/Step 5/3e row 3/Step 3.6/5.1/5.5/Step 6); all resolve. ✅
• Exactly one commit. ✅

## Review focus was honest and correct: items 1 and 2 named gate 3e and row 1's cherry-pick path as the least-reviewed code, and that is where all three blockers are. Under this skill's own rules a focus section that points at the right hunk is a point in the author's favour — I reviewed the other four files and the rest of collapse-pr regardless, and found nothing above Nit there.

On focus item 4 (length): 1083 lines is a lot, but I would not cut it. The three blockers above all come from a step that was described in prose but not written as code — which is the failure mode that shrinking this file would multiply, not reduce. The prose that is actually load-bearing here is the kind that gets promoted into if statements.

Gates run

Gate	Result
3a fixture PII	skipped — no fixture binaries in the diff
3b design-system / reuse	skipped — no src/components/** changes
3c style tokens	skipped — no src/** changes
3d dead code (fallow)	green on CI (fallow check SUCCESS); not re-run locally
3e command-level bugs in skill/script files	the whole review — B1, B2, B3, S1-S5, N1-N3
3f description accuracy	run; accurate

Head SHA reviewed: f860dae. 👀 was posted before the review began (Step 0.6).

---

Reviewed by: Claude Opus 5 (high)

[s-annam]

Blocking (B1). This ancestry test decides on $WT_PRE_FIX_SHA, which row 1 sets unconditionally at L598 — including on worktrees where nothing was authored and nothing was dirty. On a clean-but-diverged $WT, git commit above failed ("nothing to commit", silently discarded per L760-762), so $WT_PRE_FIX_SHA is the user's local-only commit, this test goes false, and the else branch cherry-picks and pushes it into the PR.

That is the #842 worked example at L794-803: a clean main checkout holding 6ba92e9. Round 2 finding 4 narrowed this from "every local-only commit" to "the tip one"; it did not close it.

Row 1 needs a real precondition — $WT is --authored-worktree and the tree is dirty — expressed in code, the same standard the mode guard holds itself to.

[s-annam]

Blocking (B3). This is a comment, not code — and it is the only backup standing between this path and the reset --hard. L659-662 then tells row 3 not to mint a second ref for this $WT, but row 3's code (L721-752) has no "already backed up" short-circuit. So an agent either mints two refs (contradicting the report contract) or, following the prose, skips row 3's backup and resets with no backup ref at all — the irreversible-loss shape round 2 finding 1 was blocking for.

Inline the real commands here (name search → git branch … || refuse → rev-parse verification) and give row 3 an explicit [ -n "${BACKUP:-}" ] skip.

[s-annam]

Blocking (B2). This push comes from $WORKDIR — the throwaway worktree — which shares .git/hooks with the main clone (core.hooksPath unset, verified). pre-push runs npm run verify there with no node_modules, so this push is rejected every time on this repo. That is exactly the failure Step 0b cites at L199-202 as the reason the bypass exists, but there is no OFFLINECV_SKIP_HOOKS=1 and no --no-verify here.

The triage table at L688 then misreads it: "row 1's push is a plain push from a real checkout" is true of the if branch only, and "fix the failing gate" is unactionable when the gate is not red — the dependencies are absent. The run wedges after a backup ref already exists.

Step 0b's tree-identity licence does not cover this push (the cherry-picked content is new relative to origin), so pick one deliberately: npm install in $WORKDIR first, or publish the fix from $WT and fast-forward. Then split that triage row per branch.

[s-annam]

Secondary. The comment says "Run Step 5b before stopping, like every refusal in this skill" — but the block exits without doing it. This refusal only fires in worktree mode, which is the one mode where Step 5b is not a no-op, so it orphans the linked worktree and $WORKDIR_PARENT under /tmp.

Either call the teardown here or move the instruction into surrounding prose, the way every other refusal in this file does.

[s-annam]

Secondary. On the documented happy path this errors. pr-review step 4 has already committed $FIXED_PATHS, so — as L612 puts it — "gate 3e sees them clean", and git commit on a clean tree exits non-zero with "nothing to commit". The skill runs without set -e, so the run continues past it (into B1); an agent that does stop aborts a collapse that should have succeeded.

Fixing B1's row-1 precondition fixes this too.

[s-annam]

Secondary. 2>/dev/null || true makes an auth failure, rate limit, or network blip indistinguishable from "no PR exists" — and L113 then reads the empty value as a licence for MODE=inplace, the mode that rewrites the user's own checkout. A swallowed error is choosing the destructive branch.

This is the "swallowed errors" item from pr-review gate 3e applied to a mode decision. Separate the two: match gh's "no pull requests found" and refuse on any other non-zero exit.

[s-annam]

Secondary. This errs in the unsafe direction, not the safe one that L586 claims.

git status --porcelain -z emits a rename as two records: R <new>\0 and then a bare <orig>\0 with no XY prefix (verified locally). cut -c4- chops three characters off that bare field, so src/foo.ts → /foo.ts and src/foo.ts is not subtracted from $FIXED_PATHS. It stays whitelisted, goes out as --authored-path, and the user's in-progress rename can be committed into the PR — the outcome this paragraph exists to prevent.

Parse it the way collapse-pr L586-594 already does, or drop only records that actually carried a status prefix.

[s-annam]

Secondary. If step 4 committed and the push failed, the fixes are committed, not uncommitted — 3e row 1 never sees them, and row 3 backs them up and reset --hards them out of the checkout. And that state cannot arise inside one run: step 5 opens with "Only if step 4 pushed."

On a real re-run the leftover commit is local-only, so row 3 relocates the previous run's fix to a collapse-pr-backup/… ref and both skills report success. Either describe that as the recovery, or add a leftover-fix-commit check to step 1.

[s-annam]

Nit. $MATCHES_FILE is never removed (nor $MSG_FILE at L859 when it comes from mktemp). Step 5b tears down the worktree but leaves the temp files behind on every path.

[s-annam]

Nit. TOCTOU against a concurrent run: two runs in the same second can both see -1 free, and the loser's git branch then trips the || at L738, which prints "Fix the ref store and re-run" — a name race misdiagnosed as a broken ref store. Worth distinguishing exit 128 / "already exists" from a genuine ref-store failure, since the message is what a user acts on.

[s-annam]

Adversarial review of the round-3 additions. 6 Blocking, 4 Nits. Head reviewed: f860dae; all line numbers are against that commit.

Four of the six blockers sit in the ~230 lines the PR body flags as unreviewed (the cherry-pick path and the --authored-* plumbing), and three of them are the same defect the round-2 report identified and closed: a guard that exists only as prose while the destructive command next to it is real code. Round 2 fixed that for MODE; the pattern moved to the whitelist comparison and the backup ref.

npm run verify is green on this branch (build + fallow clean over 7 changed files).

Blocking

1. collapse-pr/SKILL.md:595 — row 1's whitelist comparison is a comment, and row 3's reset --hard then destroys the user's uncommitted edits

Line 595 is the entire enforcement of the whitelist inside --authored-worktree:

# every path emitted above must appear in $AUTHORED_PATHS, or this is row 2.

The $WT != $AUTHORED_WORKTREE case at :573 has a real DIRTY_COUNT check with a real || refusal. The case that matters more — dirt inside the caller's own checkout — has none.

Scenario: pr-review invokes /collapse-pr 851 --yes --authored-worktree /Users/x/offlinecv --authored-path src/a.ts on a partial-failure re-run. That checkout is on $HEAD_REF and holds two dirty tracked files: src/a.ts (the run's fix) and src/b.ts (the user's in-progress edit — pr-review Step 5.5 item 1 warns explicitly that gh pr checkout carries a dirty tree onto the PR branch). The :573 guard does not fire. Nothing compares the emitted path set to $AUTHORED_PATHS. git add -- src/a.ts + commit proceeds at :599-600, src/b.ts stays dirty, and row 3 at :751 runs git -C "$WT" reset --hard "origin/$HEAD_REF".

src/b.ts is gone and is not recoverable: the backup at :738 is a branch ref, which captures commits, not a dirty worktree. The skill states this itself at :462 — "it would take the staged index with them — which no branch ref can capture, so row 3 would not even be lossless." That sentence is written about inplace mode, but it is equally true here whenever the whitelist check is skipped.

Fix: make it code, in the same fenced block as the add/commit —

git -C "$WT" status --porcelain --untracked-files=no -z | <the reader above> | sort -u > "$DIRTY_LIST"
printf '%s\n' ${AUTHORED_PATHS[@]+"${AUTHORED_PATHS[@]}"} | sort -u > "$WHITELIST"
UNKNOWN="$(comm -23 "$DIRTY_LIST" "$WHITELIST")"
[ -z "$UNKNOWN" ] || { echo "row 2: unknown provenance in $WT:"; echo "$UNKNOWN"; exit 1; }

2. collapse-pr/SKILL.md:646 — the cherry-pick path's backup is a placeholder comment, and row 3 is then told not to mint one

FIX_SHA="$(git -C "$WT" rev-parse HEAD)"
# ... row 3's backup of $FIX_SHA runs here, with its own `||` refusal ...
git -C "$WORKDIR" cherry-pick "$FIX_SHA"

Then :658-661: "Row 3 is already half-done for this $WT. The backup above covered the local-only commits and the fix commit on top of them, so when row 3 reaches this checkout it must not mint a second ref — only the reset --hard remains."

Row 3's actual code at :738-751 unconditionally mints. So the two readings are (a) mint twice, harmless, or (b) follow :658-661, skip the mint, and reach :751's git -C "$WT" reset --hard with no backup ref at all — discarding local-only commits that this very gate's design note (:479-484) argues are mechanically indistinguishable from unique work.

This is the one reset in the skill whose precondition is not in the same fenced block as the reset. Every other one is chained. Fix: inline the branch + rev-parse verification into the else branch and have row 3 key off a carried $BACKUP variable, not off prose.

3. collapse-pr/SKILL.md:647-648 — the cherry-pick path's push can never pass the managed pre-push hook, and the skill forbids the only bypass

git -C "$WORKDIR" cherry-pick "$FIX_SHA"
git -C "$WORKDIR" push origin "HEAD:$HEAD_REF"

That is a plain push, so .git/hooks/pre-push fires (worktrees share .git/hooks — Step 0b:198 says so). $WORKDIR was created by git worktree add --detach at :147 and has no node_modules, so npm run verify fails immediately — the exact failure Step 0b:200-202 describes as the reason the bypass exists at all.

The diagnosis table at :688 then gets this backwards:

"row 1's push is a plain push from a real checkout, so the managed hook fires and runs npm run verify. It is supposed to … Never set OFFLINECV_SKIP_HOOKS=1 here"

True of the if branch (:640, pushing from $WT). False of the else branch, which pushes from the throwaway worktree. Net effect on this repo: the cherry-pick path fails 100% of the time with a hook error whose documented remedy — "fix the failing gate" — is not a fixable condition.

Note the bypass is genuinely not licensed here, and that is the real problem: Step 0b's licence covers a tree the assertion proved unchanged, and a cherry-picked fix is new content. So the fix is not "add the bypass" — it is either to rebase-and-push from $WT (a real checkout, where the hook can pass), or to state plainly that this path requires a bootstrapped worktree. At minimum :688 must stop claiming this push comes from a real checkout.

4. collapse-pr/SKILL.md:289 vs pr-review/SKILL.md:561-566 — Step 2's N == 1 exit precedes gate 3e, so the documented partial-failure recovery cannot fire

pr-review justifies --authored-worktree/--authored-path this way (:561-566):

"they are what makes a re-run after a partial failure work: if step 4 committed nothing, or committed and failed to push, the fixes are sitting uncommitted and 3e can only fold them in if it knows they are ours."

In that state origin/$HEAD_REF holds the author's single commit — which pr-review:522-523 calls the common case ("a PR that open-pr already left at one hand-authored commit"). /collapse-pr Step 2 therefore counts N == 1, hits :289, runs Step 5b, and exits 0 before Step 3 executes. Gate 3e never runs, the uncommitted fix is never folded in or published, and the skill reports success. The --authored-* flags are inert on exactly the run they were built for.

Same ordering also makes --dry-run's 3e classification (:556-560) unreachable on a single-commit branch.

Fix: run 3e's rows 1–2 before Step 2's early exit (they are about the checkout, not the commit count), or drop the recovery claim from pr-review.

5. collapse-pr/SKILL.md:687 — a reset --hard on the user's real checkout with no backup, and a recovery instruction that cannot be carried out

The rejection-diagnosis table's second row prescribes:

"fetch, reset --hard origin/$HEAD_REF on $WT, re-apply the authored paths, retry once — do not refuse"

At that point $WT holds the row-1 fix commit (:600). The reset discards the commit and the file contents, so "re-apply the authored paths" has no source left to re-apply from. No backup ref is taken on this path — a direct violation of :736-738 ("The backup is a HARD PRECONDITION, not a courtesy — never let the reset run without it"), which is the invariant this PR is built around.

Fix: don't reset. git -C "$WT" fetch origin "$HEAD_REF" && git -C "$WT" rebase "origin/$HEAD_REF" replays the fix commit onto the moved head and preserves it; if you keep the reset, take row 3's backup first.

6. pr-review/SKILL.md:493 — step 5's collapse gate enumerates two of three abandon conditions, licensing a force-push on a run that pushed nothing

5. **Only if step 4 pushed.** If verification failed, or the branch moved mid-review, skip
   straight to step 6 …

Step 6 at :641 lists three: "If verification fails, or the branch moved, or the push is rejected". The third is missing from the gate.

Scenario: npm run verify is green, origin/$HEAD_BRANCH is an ancestor of HEAD, and git push origin HEAD at :484 is rejected — branch protection, a permissions change, a transient remote failure. Both of step 5's tests say "proceed". /collapse-pr is invoked, resolves the head from origin (a head that does not contain the fix), collapses it and force-pushes, dismissing whatever approval the PR held. Then :601-604 re-reads headRefOid and the body reports Addressed small fixes directly in <sha>, and the "a fix that landed is not a finding" rule at :643 moves those items out of the inline set — so the findings disappear from the review while the code is unchanged.

The Rules bullet at :878 already states the correct condition ("Collapse only on a run that actually pushed"). Step 5 is the operative text; make it match.

Nits

1. collapse-pr:573 / pr-review:534 — --authored-worktree is compared as a raw string. pr-review supplies $(git rev-parse --show-toplevel); $WT comes from git worktree list --porcelain. They differ whenever a path component is a symlink (/tmp → /private/tmp on macOS is the everyday case), and the mismatch silently drops the caller's own checkout to row 2 and refuses. Fail-safe, but a baffling refusal. Normalize both with pwd -P / realpath before comparing.

2. pr-review:585-588 — the cut -c4- justification is backwards for the field it describes. git status --porcelain -z emits a rename as XY new\0old\0; the old field carries no XY prefix, so cut -c4- chops three real characters off a genuine path and it fails to match — it under-subtracts, widening the whitelist. That is the opposite of the stated "errs by over-subtracting … that is the safe direction". collapse-pr row 1 (:586-594) already has the correct -z reader; reuse it rather than restating a safety argument that does not hold.

3. collapse-pr:510-513 — git worktree list --porcelain without -z is not newline-safe. That command grew a -z flag for exactly this; without it a path containing a newline makes the awk attribute a dirty tree to the wrong worktree. Low likelihood, one-flag fix, and this loop feeds a reset --hard.

4. collapse-pr:524-530 — the row-1→4 iteration loop has an empty body (:), with the per-$WT sequencing left to the prose that follows. Given this PR's own doctrine that guards must be code, the loop deciding which checkout each destructive row operates on is the last place to leave as a placeholder.

Description accuracy

The body's checkable claims hold: npm run verify green; all four in-scope frontmatters parse as strict YAML; only collapse-pr and the CONTRIBUTING-PROCESS.md rationale doc restate the merge-queue settings; open-pr and revise-pr carry zero reset --soft; the PR is exactly one commit; cross-referenced step and row numbers resolve across all five files. pr-ready's frontmatter is untouched, as stated.

One claim to qualify. Round-2 finding #2 — "gate 3e's worktree-only restriction was prose with zero MODE references in any of its five code blocks" — is genuinely fixed: the [ "$MODE" = worktree ] guards are present in every block. But the same failure mode did not leave the gate, it moved: the whitelist comparison (#1) and the cherry-pick backup (#2) are now the prose halves of code-plus-prose pairs, and both sit upstream of a reset --hard. The body's "Treat the sections above as the areas where direct inspection has been demonstrably unreliable" is, on this evidence, still the right instruction.

Verdict

Six blocking findings, so this would be REQUEST_CHANGES. Posting as a comment rather than a review: the approval on this PR is the maintainer's to give, and running the branch's own /pr-review auto-fix path against the code that introduces it is not a risk worth taking here.

---

Reviewed by: Claude Opus 5 (high)