fix(restack): prevent stale fork points from triggering incorrect --onto rebases

[boneskull]

When a stored fork point fell behind the actual divergence point (e.g. after a manual git rebase or a conflict-then-continue), the useOnto logic would incorrectly trigger git rebase --onto with the stale fork point as the upstream boundary. This replayed commits already present in the parent, causing spurious conflicts and dropped patches.

Three fixes:

• Ancestor check in useOnto decision: distinguish a stale fork point (ancestor of merge-base → use simple rebase) from a genuinely rewritten parent (divergent history → --onto is correct).
• Refresh fork point on "already up to date": when NeedsRebase returns false, update the fork point to the parent tip so external rebases don't leave it permanently stale.
• Update fork point in continue: after resolving a conflict, the branch that was being rebased never had its fork point refreshed; now it does.

Adds Git.IsAncestor() helper and five new tests covering all three paths plus a regression guard for the legitimate --onto case.

[boneskull]

📚 Pull Request Stack

• branch: main
  • fix(restack): prevent stale fork points from triggering incorrect --onto rebases #83 - branch: fix-restack (this PR)

---

Managed by gh-stack

[claude]

Claude finished @boneskull's task in 3m 38s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze logic correctness and edge cases
• Post review feedback

---

The fix is conceptually sound and the implementation is clean. Here's my analysis of each component:

---

internal/git/git.go — IsAncestor

func (g *Git) IsAncestor(ancestor, descendant string) bool {
    err := g.runSilent("merge-base", "--is-ancestor", ancestor, descendant)
    return err == nil
}

Clean wrapper. One subtle note: git merge-base --is-ancestor returns exit code 1 for "not an ancestor" and exit code 128+ for invalid object. Since CommitExists(storedForkPoint) is checked before every call site, and currentMergeBase comes from a successful GetMergeBase call, the bad-SHA path is unreachable in practice. No issue.

---

cmd/cascade.go — Ancestor check in useOnto

The three-way logic (== already excluded by guard, ancestor → stale → simple rebase, non-ancestor → rewritten → --onto) is correct.

Trace through the key scenarios:

Scenario	storedFP vs mergeBase	IsAncestor?	Result
No change	equal	(skipped)	no --onto ✓
External rebase (stale FP)	FP is ancestor of mergeBase	true	simple rebase ✓
Parent amend/squash (rewritten)	FP not ancestor of mergeBase	false	--onto ✓

Minor observation — GetTip(parent) is called twice in the "already up to date" path: once inside NeedsRebase and again in the new refresh block. Negligible in practice, but worth knowing if performance ever matters.

---

cmd/continue.go — Fork point update after conflict resolution

if parent, parentErr := cfg.GetParent(st.Current); parentErr == nil {
    if parentTip, tipErr := g.GetTip(parent); tipErr == nil {
        _ = cfg.SetForkPoint(st.Current, parentTip)
    }
}

Placed correctly: after RebaseContinue() succeeds but before re-running the cascade on Pending branches. Uses g (main repo) for GetTip, which is correct — branch refs are shared across worktrees. ✓

Edge case covered: If the cascade had previously failed on the second branch of a three-branch stack (A→B→C, failure on B), A's fork point was already updated by the cascade loop before the failure. B's fork point is updated by this new code, and C's is updated when the cascade resumes. All branches covered. ✓

---

e2e/cascade_test.go — Five new tests

TestCascadeStaleForkPointFromManualRebase: exercises the "already up to date" refresh path. The flow is correct — the first restack fires NeedsRebase=false and refreshes the fork point, making the second restack use simple rebase instead of --onto.

TestCascadeStaleForkPointDetectedDuringRebase: exercises the ancestor check in the useOnto logic directly, skipping the "already up to date" path. Correctly verifies IsAncestor(storedFP, mergeBase)=true → no --onto.

TestCascadeForkPointUpdatedAfterContinue: The feature-a fork point assertion at line 238 is actually testing the existing cascade loop behavior (feature-a's fork point is set during the conflict-free rebase of A, before the conflict on B). The genuinely new behavior being tested is the feature-b assertion. This is fine, but the comment "feature-a was the one that got rebased before the conflict on feature-b" could note that feature-a's fork point is verified as a pre-condition, while feature-b's is the actual regression guard.

TestCascadeOntoUsedForRewrittenParent: Good regression guard. One note — this relies on the old feature-a tip not being GC'd (CommitExists check). In a test environment GC never auto-runs, so it's reliable. But this is a pre-existing fragility in the --onto path overall, not introduced here.

---

One potential edge case not covered by tests

Scenario: storedForkPoint has been garbage collected after history was genuinely rewritten. In that case, CommitExists(storedForkPoint) returns false, the entire useOnto block is skipped, and a simple rebase runs — potentially dropping commits or causing spurious conflicts. This is a pre-existing gap (the ancestor check doesn't make it worse), but it's worth a comment near CommitExists to make the silent-failure mode explicit.

Fix this →

---

Overall

The logic is correct, the tests cover the important paths, and the comments in cascade.go explaining the three-way fork-point distinction are good. The implementation is minimal and targeted. Ready to merge once CI passes.

[claude]

Claude encountered an error —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze logic correctness and edge cases
• Post review feedback

[claude]

Claude finished @boneskull's task in 2m 26s —— View job

---

Code Review

• Gather context and understand the PR
• Read changed files
• Analyze logic correctness and edge cases
• Post review feedback

---

Two commits in this push. c02a18d is the core fix (ancestor check, fork-point refresh on up-to-date, fork-point refresh in continue). afd1255 is a follow-up that addresses the silent-failure case I flagged in the previous review — the GC'd fork point now emits a warning before falling back.

---

afd1255 — GC'd fork point warning

The new !CommitExists branch is correct and the comment is thorough. One style concern against colors.md:

// current
fmt.Printf("  %s\n", s.Muted(fmt.Sprintf(
    "warning: stored fork point %s is no longer available (garbage collected?); falling back to simple rebase",
    git.AbbrevSHA(storedForkPoint),
)))

Muted (gray) is specified for "secondary text, hints, muted info". A diagnostic that explains why --onto is being skipped — and may foreshadow spurious conflicts — reads more like a genuine warning. The project convention for warnings is WarningIcon() + Warning():

// matches existing style used elsewhere in cascade.go / saveUndoSnapshot
fmt.Printf("%s stored fork point %s unavailable (garbage collected?); falling back to simple rebase\n",
    s.WarningIcon(), git.AbbrevSHA(storedForkPoint))

There's also a minor formatting oddity: the message is indented with 2 spaces and appears before "Restacking X onto Y…" is printed, so it looks like a child of nothing. Without the indent it would sit flush at the same level as the restack line that follows.

Fix this →

---

Logic correctness (unchanged from previous review — still holds)

All three decision paths remain correct:

Scenario	storedFP vs mergeBase	IsAncestor?	Result
No change	equal	(skipped)	no --onto ✓
External rebase (stale FP)	FP is ancestor of mergeBase	true	simple rebase ✓
Parent amend/squash (rewritten)	FP not ancestor of mergeBase	false	--onto ✓
GC'd fork point	—	(skipped)	warning + simple rebase ✓

---

Tests

TestCascadeForkPointUpdatedAfterContinue (line 248–253): The featureAFP == mainTip assertion at line 252 validates pre-existing cascade-loop behavior (feature-a was rebased cleanly before the conflict, so the loop already set its fork point). The genuinely new behavior being guarded is the feature-b assertion at line 259. The comment at line 248 ("feature-a was the one that got rebased before the conflict on feature-b") is accurate but could be clearer that this is a pre-condition check, not the regression guard — otherwise a future reader may attribute it to the continue.go change. Not a blocker.

All other tests are correct (verified the scenarios in detail):

• TestCascadeStaleForkPointFromManualRebase: "already up-to-date" refresh path ✓
• TestCascadeStaleForkPointDetectedDuringRebase: ancestor check path ✓
• TestCascadeOntoUsedForRewrittenParent: legitimate --onto regression guard ✓
• TestIsAncestor: unit covers ancestor, non-ancestor, self, and divergent branches ✓

---

Overall

The core logic is correct and well-tested. The follow-up commit (afd1255) successfully addresses the silent-failure gap from the prior review. The style note on the GC warning is the only remaining feedback — whether to use Muted or WarningIcon is a judgment call, but WarningIcon better matches the convention. Ready to merge once you decide on that.