Stack 1/3: fix(spr): detect maintainer edits to PR head branches before force-push#516
Open
jucor wants to merge 2 commits into
Open
Stack 1/3: fix(spr): detect maintainer edits to PR head branches before force-push#516jucor wants to merge 2 commits into
jucor wants to merge 2 commits into
Conversation
goreleaser v2.16.0 (2026-05-24) turned the deprecated `brews:` key in .goreleaser.yml into a hard error. With `version: latest`, every push since fails CI's `goreleaser check` step regardless of the code change. v2.15.4 (2026-04-21) is the latest release that still accepts `brews:` with only a warning and also understands the current `archives.format_overrides.formats` syntax. Pinning to it restores green CI without touching .goreleaser.yml. ## Migration path (for whoever unpins) The goreleaser deprecation page recommends migrating `brews:` → `homebrew_casks:`. For simple cases this is a key rename, plus a `hooks.post.install` block to strip the macOS quarantine xattr (because the binaries aren't signed/notarized). End-user impact: - macOS: transparent. The binary still lands at `/opt/homebrew/bin/git-spr` (via cask `binary` stanza) — same PATH, same usage. The install command may need `--cask` explicitly in the readme. - Linux: regression. Casks are macOS-only; Linux brew users currently installing via `brew install ejoffe/tap/spr` would have to switch to another channel (binary release, apt, Nix, source). - Tap repo: `Formula/spr.rb` becomes `Casks/spr.rb`. The `ejoffe/homebrew-tap` repo would need a transition plan. Alternative: drop `brews:` entirely and remove the brew install path from the readme. Simpler but breaks the channel outright. Either way it's a deliberate distribution decision, not a mechanical CI fix, which is why this PR pins rather than migrates. Unpin once the `brews:` block is migrated or removed.
…sh (Stage 1)
Before this change `spr update` silently force-pushed the local stack over
any commits that had been added to PR head branches on the remote — for
example, commits a maintainer pushed via GitHub's "Allow edits by
maintainers" toggle. The check in `syncCommitStackToGitHub::commitUpdated`
was a symmetric SHA inequality with no awareness of which side was ahead,
and the push was an unconditional `--force`. Whichever side had the newer
SHA, local always won; the maintainer's work disappeared without warning.
This change introduces Stage 1 of the fix: detect divergence pre-push,
refuse with a per-PR resolution guide that spells out the manual
fold-into-local-commit recipe. Stage 2 (automatic fold) will follow as a
separate PR.
Detection algorithm (stateless, lives in `git.DetectDivergence`):
For each local commit X with commit-id <cid>:
branch = "<prefix>/<target>/<cid>"
if remote tip == local SHA: skip
walk back up to MaxDivergenceWalkDepth commits from remote tip,
looking for a commit carrying our commit-id trailer:
- found at the tip: amend-in-place; can't be distinguished from
a local user amend without per-branch state. Proceed.
- found N>0 commits back: those N commits are likely the
maintainer's contribution. Flag as DivergenceForeignCommits.
- hit a commit with a *different* commit-id trailer first:
flag DivergenceCidMismatch (cherry-pick or hand-edit).
- exhausted depth without a match: flag DivergenceCidNotFound
(deep rewrite).
Detection runs against the raw local stack so it sees every commit that
might have a corresponding remote head ref, not just the head-of-PR
subset that alignLocalCommits returns later. It also uses an explicit
per-branch fetch refspec so a restrictive `remote.origin.fetch` can't
silently disable detection.
Policy is config-driven via the new `onRemoteDivergence` field on
RepoConfig:
"ask" (default) — interactive prompt on TTY; on non-TTY, refuse with
the resolution guide. The guide prints
copy-pasteable git commands that preserve the
commit-id trailer.
"drop" — silently force-push local (the pre-fix behavior;
kept as an explicit opt-in for users who genuinely
don't want maintainer edits incorporated).
"merge" — reserved for Stage 2 auto-fold. Until that lands,
falls back to "ask" with a printed notice.
The push itself is switched from `git push --force` to
`git push --force-with-lease`, which catches the same-detection-window
race where the maintainer pushes between our fetch and our push.
Justifies itself outside the maintainer-edit story too (concurrent spr
invocations, manual pushes between runs).
Test layering:
Layer 1 (pure): git/divergence_test.go covers DetectDivergence over
every branch (no-divergence, foreign-commits-on-top,
mid-stack side branch, multiple PRs at once, cid
mismatch, depth cap, amend-in-place, custom prefix).
13 tests + 9 commit-id-extraction sub-cases.
Layer 2 (I/O): git/divergence_io_test.go drives ReadRemoteHeads
through mockgit, verifying the batched explicit-refspec
fetch, the per-ref `git log -z` walk, the parse, and
the missing-ref/default-depth/non-default-remote
variants. 5 tests.
Policy: spr/divergence_test.go exercises applyDivergencePolicy
for each value of OnRemoteDivergence, with an injected
prompter for the ask path and TTY-detection routed
through the bytes.Buffer-as-non-TTY trick.
12 tests.
Integration: 19 existing tests in spr_test.go updated with the new
ExpectDivergenceCheckClean expectation (no-op-by-shape
divergence detection that matches the local stack
exactly — confirms detection runs and stays silent
when there's nothing for it to flag).
The readme gains an "onRemoteDivergence" config row and a
"Maintainer edits to PR head branches" subsection explaining the model
and the deferred base-change limitation (separate follow-up).
Known limitation (called out in the readme): a maintainer changing the
*base* branch of a PR via the GitHub UI is silently reset by spr's
UpdatePullRequest call. Same class of bug as the head-branch wipe but
rarer in practice; addressed in a separate change.
jucor
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
spr updatesilently force-pushes the local stack over any commits a maintainer added to a PR head branch (e.g. via GitHub's "Allow edits by maintainers").commitUpdatedis a symmetric SHA inequality with no awareness of which side is ahead; the push is unconditional--force. Local always wins, maintainer work disappears silently.Fix (Stage 1)
Detect divergence pre-push, refuse with a per-PR resolution guide. Stage 2 (auto-fold) is a separate follow-up.
Walk back from each remote head looking for the local commit's
commit-id:trailer:DivergenceForeignCommits.commit-id:trailer first →DivergenceCidMismatch.DivergenceCidNotFound.Stateless; uses an explicit per-ref fetch so a restrictive
remote.origin.fetchcan't disable detection.Policy
New
onRemoteDivergenceconfig onRepoConfig:ask(default) — interactive prompt on TTY; on non-TTY, refuse with copy-pasteable resolution commands.drop— silently force-push local (pre-fix behavior, explicit opt-in).merge— reserved for Stack 2/3; currently falls back toask.Push switches from
--forceto--force-with-lease, closing the race between detection and push.Tests
git/divergence_test.go): 13 algorithm tests + 9 trailer-extraction sub-cases.git/divergence_io_test.go): 5 tests ofReadRemoteHeadsdriven againstmockgit.spr/divergence_test.go): 12 tests ofapplyDivergencePolicyand the TTY/prompter paths.spr_test.gotests updated withExpectDivergenceCheckClean.Known limitation
Maintainer changes to a PR's base branch (via the GitHub UI) are silently reset by
UpdatePullRequest. Same class of bug, much rarer in practice — readme calls it out, separate follow-up.A note on size
+1247/-7. Roughly half is tests (593 LOC), a third is the algorithm (271) and policy/UI (215), the rest is mockgit helpers (103), wiring (35), config (19), docs (13).
Suggested review order:
git/divergence.go::DetectDivergenceandspr/divergence.go::applyDivergencePolicycapture the whole behaviour. Tests organised one-layer-per-file; skim by file.Full transparency: I made heavy use of Claude Code (Opus 4.7, high effort) for this PR, as I do for my own work these days.