Skip to content

Stop reporting rebases that git refused to run as successes#320

Open
skarim wants to merge 1 commit into
mainfrom
skarim/fix-silent-rebase-failures
Open

Stop reporting rebases that git refused to run as successes#320
skarim wants to merge 1 commit into
mainfrom
skarim/fix-silent-rebase-failures

Conversation

@skarim

@skarim skarim commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

The bug

gh stack rebase and gh stack sync could print a complete success report while leaving the stack exactly where it was — matching the user reports:

✓ Trunk main fast-forwarded to 84bacb3
Stack detected: (main) <- phil/trd-mark-edge-drills <- phil/implement-in-place-apis
✓ Rebased phil/trd-mark-edge-drills onto main
✓ Rebased phil/implement-in-place-apis onto phil/trd-mark-edge-drills
All branches in stack rebased locally with main

…while git merge-base --is-ancestor main HEAD still said main was not an ancestor.

Root cause

tryAutoResolveRebase in internal/git/git.go — the shared error path for every rebase — returned nil whenever no rebase was in progress:

for i := 0; i < 1000; i++ {
    if !IsRebaseInProgress() {
        return nil          // <- "success"
    }

That early return is only a valid success signal after an auto---continue. On the first check it means git rebase exited non-zero without ever starting. Verified against real repos, all of these hit it:

Cause git says
Dirty working tree error: cannot rebase: You have unstaged changes.
Branch checked out in another worktree fatal: 'b2' is already used by worktree at …
Trunk missing locally fatal: invalid upstream 'main'
Stale rebase in progress fatal: … rebase-merge directory already exists

Every one returned nil, so cascadeRebase printed ✓ Rebased X onto Y and carried on. It affected rebase, sync, and modify.

Reverting just this one change reproduces the reported log byte-for-byte in the new integration test.

Fixes

Silent failures

  • New typed *git.RebaseStartError for a rebase that never started, plus an up-front check so an already-running rebase isn't mistaken for this rebase's conflicts.
  • cascadeRebase (and modify) treat it as fatal rather than a conflict — no bogus rebase-state file, and git's own message is surfaced.
  • rebase/sync gain the preflight checks modify already had (no rebase in progress, clean working tree), with --autostash to opt out by passing --autostash straight through to git.
  • After every cascade, verifyStacked() confirms each branch really sits on top of its parent and fails instead of printing the success summary. sync checks before pushing, so an unrebased stack is never force-pushed.

Duplicate commits

The cascade passed the parent's current tip as the --onto upstream with no staleness guard, so a parent amended, reordered, or squash-merged out of band had its old commits replayed onto the child. The merged-PR path guarded this but fell back to merge-base(newBase, branch), which replays squashed commits. Both paths now use resolveOntoOldBase() — the latest commit the child genuinely contains. Without it the new test hits a spurious conflict on the parent's own file; with it the rebase is clean.

Found alongside

  • git rebase <option> --continue is a usage error (exit 129), so --preserve-dates made rebase --continue fail every time. git persists the option in .git/rebase-merge, so --continue alone honors it.
  • sync never called ensureLocalTrunk before rebasing onto the trunk, and printed "Fetched latest changes" even when the fetch failed.
  • A trunk whose remote branch is gone now says so instead of silently rebasing onto a stale local trunk.

Testing

  • Real-git integration tests (cmd/rebase_integration_test.go, internal/git/gitops_test.go) running the actual commands against real repos with real remotes: trunk is pulled in, dirty tree rejected, --autostash rebases and restores, foreign worktree fails loudly, amended parent doesn't duplicate commits.
  • Table-driven MockOps tests for failure classification, both preflights, --autostash pass-through, and post-cascade verification.
  • Unit tests for resolveOntoOldBase and verifyStacked.
  • go vet ./... and go test -race -count=1 ./... pass.

Not included

#225's trunk migration (re-pointing a stack whose trunk branch was squash-merged and deleted) is deferred to a follow-up, per discussion — it needs more nuanced semantics around changing a stack's trunk. This PR only makes that situation visible rather than a silent no-op.

`gh stack rebase` and `gh stack sync` could print a full success report
while leaving the stack exactly where it was:

    ✓ Trunk main fast-forwarded to 84bacb3
    ✓ Rebased phil/trd-mark-edge-drills onto main
    ✓ Rebased phil/implement-in-place-apis onto phil/trd-mark-edge-drills
    All branches in stack rebased locally with main

…after which `git merge-base --is-ancestor main HEAD` still reported that
main was not an ancestor.

The cause is in `tryAutoResolveRebase`, the shared error path for every
rebase: it returned nil whenever no rebase was in progress. That is only a
valid success signal *after* an auto-`--continue`. On the first check it
means `git rebase` exited non-zero without ever starting — a dirty working
tree, a branch checked out in another worktree, a missing local trunk, or a
stale rebase still in progress. All of those were silently swallowed and the
cascade carried on reporting success.

- Return a typed `*git.RebaseStartError` for a rebase that never started, and
  detect an already-running rebase up front so its leftover conflicts are not
  mistaken for this rebase's.
- Treat that error as fatal in `cascadeRebase` (and in `modify`) rather than a
  conflict, so no bogus rebase state is written and git's own message shows.
- Add the preflight checks `modify` already had — no rebase in progress, clean
  working tree — to `rebase` and `sync`, with `--autostash` to opt out of the
  clean-tree requirement by passing `--autostash` through to git.
- Verify after every cascade that each branch really does sit on top of its
  parent, and fail instead of printing the success summary. `sync` checks
  before pushing so an unrebased stack is never force-pushed.

Also fixes duplicate commits: the cascade passed the parent's current tip as
the `--onto` upstream with no staleness guard, so a parent amended, reordered,
or squash-merged out of band had its old commits replayed onto the child. The
merged-PR path guarded this but fell back to `merge-base(newBase, branch)`,
which replays squashed commits. Both paths now use the latest commit the child
genuinely contains.

Smaller fixes found alongside:

- `git rebase <option> --continue` is a usage error, so `--preserve-dates`
  made `rebase --continue` fail every time. git persists the option in the
  rebase state, so `--continue` alone honors it.
- `sync` never ensured the local trunk existed before rebasing onto it, and
  printed "Fetched latest changes" even when the fetch failed.
- A trunk whose remote branch is gone now says so instead of silently rebasing
  onto a stale local trunk (partial diagnostic for #225; migrating the trunk
  is a separate change).

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f4355094-2f25-4532-a17d-2b88fbf68131
Copilot AI review requested due to automatic review settings July 26, 2026 18:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Prevents failed Git rebases from being reported as successful and strengthens stack verification.

Changes:

  • Adds typed rebase-start failures and post-rebase ancestry checks.
  • Adds --autostash and safer sync/rebase preflights.
  • Improves old-base selection, fetch diagnostics, documentation, and tests.
Show a summary per file
File Description
internal/modify/apply.go Handles rebase start failures during modify.
internal/git/gitops.go Adds rebase flags and fetch error handling.
internal/git/git.go Classifies rebase failures and fixes continuation.
internal/git/gitops_test.go Adds real-Git rebase tests.
cmd/utils.go Adds preflights, verification, and old-base resolution.
cmd/utils_test.go Tests ancestry verification and base selection.
cmd/rebase.go Adds autostash and post-cascade verification.
cmd/rebase_test.go Tests rebase failure and preflight paths.
cmd/rebase_integration_test.go Adds command-level Git integration coverage.
cmd/sync.go Prevents pushing after failed verification.
cmd/sync_test.go Tests sync failure and autostash behavior.
docs/src/content/docs/reference/cli.md Documents updated CLI behavior.
docs/src/content/docs/guides/workflows.md Documents autostash workflows.

Review details

  • Files reviewed: 13/13 changed files
  • Comments generated: 4
  • Review effort level: Medium

Comment thread cmd/utils.go
Comment on lines +953 to +954
if autostash {
return nil
Comment thread internal/git/gitops.go
Comment on lines +165 to +167
err := runSilent("fetch", remote, rs)
if err == nil || isMissingRemoteRefError(err) {
continue
Comment thread cmd/utils.go
Comment on lines +919 to +923
cfg.Errorf("a rebase is currently in progress")
cfg.Printf("Complete the rebase with `%s` or abort with `%s`",
cfg.ColorCyan("gh stack rebase --continue"),
cfg.ColorCyan("gh stack rebase --abort"))
return ErrRebaseActive
Comment thread cmd/utils.go
Comment on lines +983 to +984
cfg.Printf(" If %s was merged and deleted on %s, re-point the stack with `%s`.",
trunk, remote, cfg.ColorCyan("gh stack init"))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants