stack: restack the rest of the stack after a squash land #237
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/stack-ship-restack-after-land"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
fj stack ship --squashleft the stack broken. A squash merge replaces the item's commits with one new commit, so every branch above the landed item stayed parented on history the trunk no longer has: the PR above shows the landed diff a second time, and you had to rebase and force-push the remainder by hand. Ship now finishes the job.Ship is still a client sequence of merge calls, not an atomic land. That is now stated plainly in the help and in docs/stacks.md rather than left to be inferred.
What changed
--squash), replay the branches above onto the new trunk tip with onegit rebase --onto <new trunk> <landed tip> <top branch> --update-refs(the plumbingstack absorbalready uses), force-push each with an explicit--force-with-lease=<ref>:<pre-replay sha>, and re-sync the PR bases. A conflict rolls back: nothing pushed, no PR retargeted, ship stops with the commands to finish by hand.--no-restackopts out of the replay and the local checks it depends on (clone without the branches, or git < 2.38). Merge-commit lands are unchanged: the original commits stay reachable, so only the next PR's base moves.git::Repohandle addressed by path, so the replay runs at the repo root and is testable against a throwaway repo.stack ship --jsongainsrestackedper item,stack review --jsongainshead_sha.Testing
cargo build,make lint,cargo testgreen (719 unit tests, +25). New tests build real git repos with a bare "origin" in a temp dir and assert the branches above a simulated squash land come out reparented, one commit each, force-pushed, and with their PR bases re-pointed against a mock forge; the conflict path leaves every ref and PR untouched. Pure-function tests cover the pre-flight refusals (stale trunk, broken link, missing branch, branch/PR head mismatch) and the re-validation verdicts (conflicts, closed, someone else pushed, review or checks regressed, restacked head accepted with its re-armed CI). Real-git tests skip themselves when git is older than 2.38.Follow-ups, deliberately not here
--upto <n>for a partial land, and--auto(merge_when_checks_succeed, already plumbed atsrc/api/pull_core.rs:378) to arm a stack to land as checks go green, which is also the answer for waiting on the CI a restack re-arms.Forseti review
2 blocking finding(s) from the lead reviewer (advisory: not gating merges yet).
Findings
P1
src/cli/stack.rs:1887— The new docs and recovery text promise that a conflicting replay rolls back with no PR retargeted, but theErr(e)path afterrestack_onto_trunkunconditionally edits the PR directly above the landed item to targettrunkand updates the local stack base. If the replay failed before moving the remaining branches, this leaves the forge/base metadata changed while the branches are still based on the pre-squash commits, recreating the broken-diff state the PR is intended to avoid and contradicting the documented rollback behavior.P1
src/cli/stack.rs:1840— After a restack,ExpectedHeadcontains both the preflight SHA and the newly pushed SHA.revalidate_blockertherefore returnsNonewhen the forge still reports the old pre-restack head, and this loop immediately breaks onblocker.is_none(). That means a lagging PR read can allow the next merge call to run before the forge has observed the force-pushed restacked head, defeating the purpose of replaying the branch onto the new trunk tip and potentially merging the stale stack history.PR:
rasterstate/fj#237Head SHA:
226c63719fbdReview job:
sha256:e434dd25bbaa875d042303631d41494b03f1741163ee1690f84a3b365137cc7dProvider pair:
openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001Blocking findings:
2Inline findings:
2Token source:
GITHUB_TOKENRunner:
de13e5c632e6Run: https://rasterhub.com/rasterstate/fj/actions/runs/414
Forseti review
2 blocking finding(s) from the lead reviewer (advisory: not gating merges yet).
Findings
P1
src/cli/stack.rs:1887— The new docs and recovery text promise that a conflicting replay rolls back with no PR retargeted, but theErr(e)path afterrestack_onto_trunkunconditionally edits the PR directly above the landed item to targettrunkand updates the local stack base. If the replay failed before moving the remaining branches, this leaves the forge/base metadata changed while the branches are still based on the pre-squash commits, recreating the broken-diff state the PR is intended to avoid and contradicting the documented rollback behavior.P1
src/cli/stack.rs:1840— After a restack,ExpectedHeadcontains both the preflight SHA and the newly pushed SHA.revalidate_blockertherefore returnsNonewhen the forge still reports the old pre-restack head, and this loop immediately breaks onblocker.is_none(). That means a lagging PR read can allow the next merge call to run before the forge has observed the force-pushed restacked head, defeating the purpose of replaying the branch onto the new trunk tip and potentially merging the stale stack history.PR:
rasterstate/fj#237Head SHA:
226c63719fbdReview job:
sha256:e434dd25bbaa875d042303631d41494b03f1741163ee1690f84a3b365137cc7dProvider pair:
openai:gpt-5.5+anthropic:claude-haiku-4-5-20251001Blocking findings:
2Inline findings:
2Token source:
GITHUB_TOKENRunner:
de13e5c632e6Run: https://rasterhub.com/rasterstate/fj/actions/runs/414
The linear-history pre-flight was meant to refuse a stale or non-linear stack up front rather than fail mid-merge with the bottom item already landed. It did not, whenever a PR head could not be read. `ship` takes its heads from one unpaginated page of open PRs (limit 30, no sort), so any stack whose PRs sit outside that page had no head to compare against. Both places that consume the map treated "not read" as "nothing to report": - the pre-flight's comparison fell through a `_ => None` arm, so `linear_history_blocker` had nothing to refuse on and the check passed; - under --force, which skips the per-PR gate entirely, the expected head fell back to `unwrap_or_default()`, producing an empty sha list that made `revalidate_blocker`'s head-moved guard vacuous. Together those let a stale bottom item squash-land, after which the replay's force-push was rejected on a stale lease, leaving the stack half-repaired. The correlation ran the wrong way too: a stack whose PRs are not recent enough to sit on page 1 is the stack most likely to be stale locally. Fill the gaps with a per-PR read before the pre-flight (free on repos whose open PRs fit one page), and make both consumers fail safe if a head is still unknown: `HeadCheck::Unknown` refuses in the pre-flight, and `forced_expected_heads` refuses instead of merging on an expectation it cannot check. The pre-flight observation moves into `observe_stack` so the refusal is directly testable. Also distinguish a rejected lease from a transport failure when publishing a replayed branch. Both used to print "The local refs are already correct; push the remaining branches with --force-with-lease", which is wrong for a stale lease: the local refs are precisely what is missing the concurrent push, and escalating to --force from there destroys it. `git::StaleLease` is now a typed error so the caller can tell the two apart.Forseti review
No blocking findings from the lead reviewer.
No inline findings.
rasterstate/fj#2371e2880c0140bsha256:596eb9a1472b81484ee7b47fda1c945d76f2fddc41ab17625a8952c7f4140131openai:gpt-5.5+anthropic:claude-haiku-4-5-2025100100GITHUB_TOKEN34f1a8310153Forseti review
No blocking findings from the lead reviewer.
No inline findings.
rasterstate/fj#2371e2880c0140bsha256:596eb9a1472b81484ee7b47fda1c945d76f2fddc41ab17625a8952c7f4140131openai:gpt-5.5+anthropic:claude-haiku-4-5-2025100100GITHUB_TOKEN34f1a8310153Forseti review
No blocking findings from the lead reviewer.
No inline findings.
rasterstate/fj#2371ca2a264f5c9sha256:55dd14da0f34e40bd54ee6ee8a6a50104de35b1d9e78f509efcefe755fe2baf5openai:gpt-5.5+anthropic:claude-haiku-4-5-2025100100GITHUB_TOKEN33b352ff6ee0Forseti review
No blocking findings from the lead reviewer.
No inline findings.
rasterstate/fj#2371ca2a264f5c9sha256:55dd14da0f34e40bd54ee6ee8a6a50104de35b1d9e78f509efcefe755fe2baf5openai:gpt-5.5+anthropic:claude-haiku-4-5-2025100100GITHUB_TOKEN33b352ff6ee0Both P1s from the review at
226c63719fbdare addressed in1ca2a26. One was a real logic defect and is fixed in code. The other identified a real contradiction but pointed at the wrong half, so the fix is in the prose.Finding 2,
src/cli/stack.rs:1840, the re-validation loop: confirmed, fixed in codeThe mechanism is exactly as described.
ExpectedHead.shasaccumulates:validated()seeds it with the pre-flight sha andpushed()appends each force-pushed one.revalidate_blockeronly objected when the reported head was in neither, so a forge still serving the pre-restack head returnedNone, and the loop broke onblocker.is_none()beforeentry.restackedwas ever consulted. The retry budget was dead code for precisely the race the comment aboveREVALIDATE_ATTEMPTSdescribes.Worth recording how it got there, because it was not an oversight. The acceptance was deliberate and there was a test asserting it, in
revalidate_accepts_the_head_fj_force_pushed_and_its_re_armed_checks:That is a correct observation with the wrong remedy. Two mechanisms were built for the same lag, and the lenient one silently disabled the careful one. That assertion is now inverted.
On the consequence, where I do not follow the finding's framing
The review says this risks "merging the stale stack history". I do not think that is what happens, and it is worth being precise because the real hazard is a different one.
By the time
revalidateruns for the next item,restack_onto_trunkhas already returnedOk. That means the force-push completed andapi::pull::editretargeted the base, both awaited. The remote ref is already at the new sha.api::pull::mergesends nohead_commit_id(MergeOptionshas no such field), so the merge is by ref, and the ref has moved. The content that merges is the replayed content, not the pre-squash history.What is actually stale is the PR record, and that is the problem, because
revalidateis a gate that reads its whole verdict off that record.state,mergeable, and the checks are all taken from a snapshot computed for the pre-restack head against the pre-retarget base.mergeableis the dangerous one, and it is worse than "computed for the old head": fj invalidated both of its inputs milliseconds earlier, the head by force-pushing and the base by retargeting it to the trunk. Amergeable: truefrom that snapshot carries no information about whether the replayed head merges into the trunk. The gate whose entire job is to catch "this stopped merging cleanly after something below it landed" is reading stale data in exactly the situation a restack creates.The failure mode that follows is a degraded one rather than corruption: fj proceeds, the forge refuses the conflicting merge,
api::pull::mergeerrors, and the user getsrecovery_message("a merge failed partway, repair by hand") when the design intendsstopped_message("stopped before merging, here is what landed, nothing else was touched"). A clean stop turns into a mid-run failure with the bottom already merged. Real, but I would not describe it as merging the wrong tree.One amplification neither the finding nor I spotted at first. Every land restacks again and calls
pushed()again, so an entry deep in the stack accumulates several shas fj pushed. Under the old membership test the lag tolerance compounded with depth: by the third item, three different superseded heads all read as ready.revalidate_lag_tolerance_does_not_compound_down_the_stackcovers this.The fix
The acceptance set was answering two questions with one membership test. "Did somebody else move this head" wants the whole set; "has the forge observed my force-push yet" wants only the newest. Those are now separate:
Stale: re-read, do not judge anything else off that snapshotBlocked: stop now, no retry, unchanged from todayStalethat outlivesREVALIDATE_ATTEMPTSbecomes a blocker rather than a pass, because a forge that never caught up is itself a reason not to mergeReturning early on
Staleis the part that closes themergeablehole: no gate is evaluated on a record describing a shape the restack already replaced. Once the forge catches up, the same gates run against a snapshot of the post-restack, post-retarget state, which is what they were always meant to see.Since
Stalerequiresexpected.restacked, the non-restacked path cannot reach it, so the common case takes no new latency.Tests
Four new cases plus the inverted one, and I mutation-checked rather than trusting green. Restoring the old
contains()acceptance fails 4 of the new tests and leaves all 8 pre-existing ones passing, which is the check that the fix did not simply widen acceptance. Separately, mutatingRevalidation::blockerso an exhausted retry budget returnsNonefails the test that asserts a stale read is terminal.Finding 1,
src/cli/stack.rs:1887, retarget on replay failure: contradiction confirmed, direction refuted, fixed in the docsThe review is right that the code and the prose disagree, and right about which lines.
CHANGELOG.mdanddocs/stacks.mdboth said a conflicting replay means no PR is retargeted, while theErr(e)arm callsapi::pull::edit(numbers[i + 1], base: Some(&trunk))and writes that base into the stack state.The code is the correct half, so acting on the implied direction (roll the retarget back too) would be a regression.
The item below has already merged at that point. Its branch is going away whatever happened to the replay, and Forgejo deletes or orphans it. A PR left pointing at a deleted base branch is a worse end state than the one
--no-restackproduces, and it is not recoverable by re-running ship, since ship's own pre-flight would then refuse the stack it created. The retarget is the same thing the_ =>arm does for merge-commit lands, where no replay happens at all; theErrarm is converging on that known-good state, not inventing one. The comment above the block already says this.So the prose is what was wrong. Both files now scope the rollback claim to the rebase, which is what is actually rolled back, and state separately that the PR directly above the landed item still moves to the trunk, with the reason.
While correcting it I found the claim was overstated in a second way the finding did not reach. The sentence is conditioned on "if the replay conflicts", but the
Errarm catches every failure fromrestack_onto_trunk, including a force-push or a re-point that fails partway through the remainder. In those cases "nothing is force-pushed" is also false: the branches already pushed stay pushed.docs/stacks.mdnow says so.I checked
stopped_messageandrecovery_messagefor the same overclaim. Neither carries it; both are generic about what stopped and what already landed, and make no claim about retargeting. No change needed there. I did add a scope note torestack_onto_trunk_rolls_back_and_retargets_nothing_on_conflict, whose name reads like the general claim but which only exercisesrestack_onto_trunkin isolation, where retargeting nothing is correct.Not changed
I checked whether
numbers[i + 1]in theErrarm can panic on the last item. It cannot:plan_restackreturnsNonewhenabove.is_empty(), so the(true, Some(plan))arm guarantees an item above. No defensiveget()added.State
cargo test726 passing,cargo clippy --all-targets -D warningsclean,cargo fmt --checkclean.One caveat on the Forseti signal, since it would be easy to over-read. Both P1s were already reported as cleared at
1e2880c, before this fix landed, and that commit changed neither the acceptance set nor the docs sentence. So the clean run at1ca2a26is consistent with the fix but is not by itself evidence for it. The mutation checks above are the load-bearing part.