From 50e5514f8ce6605a2445316c6b9ac4bb0b82ae77 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Sat, 11 Jul 2026 06:58:12 -0400 Subject: [PATCH 1/2] Don't treat queued PRs as merged when rebasing the stack MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit gh stack rebase and gh stack sync share cascadeRebase, which skipped branches via IsSkipped() (merged or queued) and then switched to a `git rebase --onto` that drops the skipped branch's commits from every downstream branch. That is right for a merged PR — its commits are already in trunk — but wrong for a queued PR: its commits only exist on its own branch, which is frozen in the merge queue, so the branches above it were rebased onto trunk and lost work they depend on. Handle the two cases separately. A merged branch still activates --onto so its commits are dropped. A queued branch is still skipped (its branch is frozen and is not rebased or pushed), but onto mode is reset so downstream branches rebase normally onto the queued branch, keeping its commits underneath. The --onto target search, the runRebase --onto seed, and the continueRebase display base now key on IsMerged() instead of IsSkipped(), so a queued predecessor no longer forces downstream branches onto trunk. gh stack sync is fixed through the same shared helper. Add rebase coverage for a queued branch mid-stack, a merged branch below a queued branch, and --upstack above a queued branch, plus a sync test that also asserts the queued branch is excluded from the push. The transient queued state is injected through the GitHub mock's merge-queue entry. --- cmd/rebase.go | 12 +-- cmd/rebase_test.go | 183 +++++++++++++++++++++++++++++++++++++++++++++ cmd/sync_test.go | 77 +++++++++++++++++++ cmd/utils.go | 22 ++++-- 4 files changed, 282 insertions(+), 12 deletions(-) diff --git a/cmd/rebase.go b/cmd/rebase.go index d0c2814..dfd7c21 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -198,14 +198,14 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { return fmt.Errorf("resolving branch refs: %w", err) } - // Get --onto state from merged/queued branches below the rebase range. - // Ensures that when --upstack excludes skipped branches, we still check - // the immediate predecessor and use --onto if needed. + // Get --onto state from a merged branch immediately below the rebase range. + // Ensures that when --upstack excludes merged branches, we still check the + // immediate predecessor and use --onto if needed. needsOnto := false var ontoOldBase string if startIdx > 0 { prev := s.Branches[startIdx-1] - if prev.IsSkipped() { + if prev.IsMerged() { if sha, ok := originalRefs[prev.Branch]; ok { needsOnto = true ontoOldBase = sha @@ -334,10 +334,10 @@ func continueRebase(cfg *config.Config, gitDir string) error { var baseBranch string if state.UseOnto { - // The --onto path targets the first non-skipped ancestor, or trunk. + // The --onto path targets the first non-merged ancestor, or trunk. baseBranch = s.Trunk.Branch for j := state.CurrentBranchIndex - 1; j >= 0; j-- { - if !s.Branches[j].IsSkipped() { + if !s.Branches[j].IsMerged() { baseBranch = s.Branches[j].Branch break } diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index 611f2b4..a3365cc 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -11,6 +11,7 @@ import ( "github.com/github/gh-stack/internal/config" "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/github" "github.com/github/gh-stack/internal/stack" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -680,6 +681,188 @@ func TestRebase_SkipsMergedBranches(t *testing.T) { assert.Equal(t, "b2", rebaseCalls[0].branch) } +// queuedPRClient returns a MockClient whose FindPRByNumber reports the given PR +// numbers as queued (in a merge queue, open, not merged) and finds no PR by +// branch name. Used to drive the transient Queued state through syncStackPRs in +// rebase/sync tests. +func queuedPRClient(headByNumber map[int]string) *github.MockClient { + return &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + head, ok := headByNumber[n] + if !ok { + return nil, nil + } + return &github.PullRequest{ + Number: n, + HeadRefName: head, + State: "OPEN", + Merged: false, + MergeQueueEntry: &github.MergeQueueEntry{ID: fmt.Sprintf("MQ_%d", n)}, + }, nil + }, + FindPRForBranchFn: func(string) (*github.PullRequest, error) { return nil, nil }, + } +} + +// TestRebase_QueuedBranch_DownstreamStaysStacked verifies the #144 fix: a queued +// PR is NOT treated as merged. Its branch is skipped (frozen in the merge queue), +// but downstream branches stay stacked on top of it — they rebase onto the queued +// branch, not --onto trunk with the queued commits dropped. +func TestRebase_QueuedBranch_DownstreamStaysStacked(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"}) + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.Contains(t, output, "Skipping b1") + assert.Contains(t, output, "queued") + assert.NotContains(t, output, "adjusted for merged PR", + "queued branches must not trigger the merged --onto path") + + // b2 stays stacked on the queued b1 (not rebased --onto main); b3 onto b2. + require.Len(t, rebaseCalls, 2) + assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseCalls[0], + "b2 should rebase onto the queued branch b1, keeping its commits") + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1], + "b3 should rebase onto b2") +} + +// TestRebase_MergedBelowQueued_KeepsStackedOnQueued verifies that when a merged +// branch sits below a queued branch, the branch above the queued one stays +// stacked on the queued branch. The queued branch is frozen and still carries the +// merged branch's commits, so downstream cannot drop them via --onto. +func TestRebase_MergedBelowQueued_KeepsStackedOnQueued(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b3") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{11: "b2"}) + cmd := RebaseCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.Contains(t, output, "Skipping b1") + assert.Contains(t, output, "PR #10 merged") + assert.Contains(t, output, "Skipping b2") + assert.Contains(t, output, "queued") + + // b1 merged and b2 queued are both skipped. b3 stays stacked on the queued + // b2 — it must NOT be rebased --onto main (which would drop b2's + b1's + // commits while b2 is frozen). + require.Len(t, rebaseCalls, 1) + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[0], + "b3 should rebase onto the queued b2, not --onto main") + assert.NotContains(t, output, "adjusted for merged PR") +} + +// TestRebase_UpstackAboveQueuedBranch verifies the onto-seed fix: with --upstack +// starting just above a queued branch, the first in-range branch rebases normally +// onto the queued predecessor rather than dropping its commits via --onto. +func TestRebase_UpstackAboveQueuedBranch(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b2") + mock.BranchExistsFn = func(name string) bool { return true } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"}) + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--upstack"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + // upstack from b2 = [b2, b3]; b1 (queued) is below the range. + require.Len(t, rebaseCalls, 2) + assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseCalls[0], + "b2 should rebase onto the queued predecessor b1, not --onto main") + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[1], + "b3 should rebase onto b2") + assert.NotContains(t, output, "adjusted for merged PR") +} + // TestRebase_StateRoundTrip verifies that rebase state can be saved and loaded // back with all fields preserved, including the --onto fields. func TestRebase_StateRoundTrip(t *testing.T) { diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 746739e..6eaebcb 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -745,6 +745,83 @@ func TestSync_MergedBranch_UsesOnto(t *testing.T) { assert.True(t, pushCalls[0].force) } +// TestSync_QueuedBranch_DownstreamStaysStacked verifies the #144 fix in the sync +// path: a queued branch is skipped from push (frozen in the merge queue) but +// downstream branches stay stacked on top of it — they are NOT rebased --onto +// trunk with the queued commits dropped. +func TestSync_QueuedBranch_DownstreamStaysStacked(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var rebaseOntoCalls []rebaseCall + var pushCalls []pushCall + + mock := newSyncMock(tmpDir, "b2") + // Trunk behind remote to trigger rebase; branches match their remote. + mock.RevParseFn = func(ref string) (string, error) { + switch ref { + case "main": + return "local-sha", nil + case "origin/main": + return "remote-sha", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + } + mock.UpdateBranchRefFn = func(string, string) error { return nil } + mock.CheckoutBranchFn = func(string) error { return nil } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseOntoCalls = append(rebaseOntoCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{10: "b1"}) + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Out.Close() + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.Contains(t, output, "queued") + + // b1 is queued → skipped, but downstream stays stacked on it: + // b2 onto b1 (not --onto main), b3 onto b2. + require.Len(t, rebaseOntoCalls, 2) + assert.Equal(t, rebaseCall{"b1", "sha-b1", "b2"}, rebaseOntoCalls[0], + "b2 should rebase onto the queued b1, keeping its commits") + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseOntoCalls[1], + "b3 should rebase onto b2") + + // The queued branch is excluded from push; only b2 and b3 are pushed. + require.Len(t, pushCalls, 1) + assert.Equal(t, []string{"b2", "b3"}, pushCalls[0].branches, + "queued b1 must not be pushed") +} + // TestSync_StaleOntoOldBase_FallsBackToMergeBase verifies that when a branch // was already rebased past the merged branch's tip, sync detects the stale // ontoOldBase and falls back to merge-base for the correct divergence point. diff --git a/cmd/utils.go b/cmd/utils.go index 3a822b0..4266461 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -850,23 +850,33 @@ func cascadeRebase(opts cascadeRebaseOpts) cascadeRebaseResult { base = s.Branches[absIdx-1].Branch } - // Skip merged and queued branches. + // Skip merged and queued branches — but treat them differently for + // downstream rebasing. if br.IsSkipped() { - ontoOldBase = originalRefs[br.Branch] - needsOnto = true if br.IsMerged() { + // A merged PR's commits are already in trunk, so downstream + // branches must drop them by rebasing --onto the first + // non-merged ancestor. + ontoOldBase = originalRefs[br.Branch] + needsOnto = true cfg.Successf("Skipping %s (PR %s merged)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) - } else if br.IsQueued() { + } else { + // A queued PR is frozen in the merge queue and its commits are + // NOT yet in trunk. Downstream branches must stay stacked on top + // of it, so do not switch to --onto (which would drop its + // commits). Reset onto state in case a merged branch set it. + needsOnto = false cfg.Successf("Skipping %s (PR %s queued)", br.Branch, cfg.PRLink(br.PullRequest.Number, br.PullRequest.URL)) } continue } if needsOnto { - // Find --onto target: first non-skipped ancestor, or trunk. + // Find --onto target: first non-merged ancestor, or trunk. Queued + // ancestors keep their commits, so they are valid --onto targets. newBase := s.Trunk.Branch for j := absIdx - 1; j >= 0; j-- { - if !s.Branches[j].IsSkipped() { + if !s.Branches[j].IsMerged() { newBase = s.Branches[j].Branch break } From 8e7863af2245081ec94a6c9fe7e79c7b5b4cf93c Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Sat, 11 Jul 2026 07:36:50 -0400 Subject: [PATCH 2/2] Refresh queued PR state when continuing a stack rebase MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit continueRebase reloads the stack from disk, where the Queued flag is transient (json:"-") and therefore lost, and it only called syncStackPRs after the cascade. So if the initial rebase conflicted on a branch below a queued branch, `gh stack rebase --continue` resumed with that branch seen as active: it rebased the frozen merge-queue branch and rebuilt the downstream branches on a local history that differs from the queued branch. Call syncStackPRs right after resolving the stack — before selecting the base and cascading the remaining branches — mirroring the refresh runRebase already does before its cascade. The queued flag is repopulated, so queued branches stay skipped and downstream branches stay stacked on them. Add TestRebase_Continue_QueuedBranchBelowConflict, which conflicts below a queued branch and asserts the frozen branch is not rebased and the branch above stays stacked on it. Verified to fail without the refresh. --- cmd/rebase.go | 8 +++++ cmd/rebase_test.go | 74 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+) diff --git a/cmd/rebase.go b/cmd/rebase.go index dfd7c21..9b79efd 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -315,6 +315,14 @@ func continueRebase(cfg *config.Config, gitDir string) error { return fmt.Errorf("no stack found for branch %s", state.OriginalBranch) } + // Refresh PR state before selecting the base and cascading the remaining + // branches. The queued flag is transient (not persisted), so it was lost + // when the stack was reloaded from disk above. Without this, a queued + // branch in the remaining cascade would be treated as active and its + // frozen merge-queue branch would be rebased. Mirrors the syncStackPRs + // call in runRebase before its cascade. + _ = syncStackPRs(cfg, s) + // The branch that had the conflict is stored in state; fall back to // looking it up by index for backwards compatibility with older state files. conflictBranch := state.ConflictBranch diff --git a/cmd/rebase_test.go b/cmd/rebase_test.go index a3365cc..683bfe3 100644 --- a/cmd/rebase_test.go +++ b/cmd/rebase_test.go @@ -974,6 +974,80 @@ func TestRebase_Continue_RebasesRemainingBranches(t *testing.T) { assert.Contains(t, checkouts, "b1", "should checkout original branch") } +// TestRebase_Continue_QueuedBranchBelowConflict verifies that a queued branch is +// still skipped when the cascade resumes via --continue after a conflict below +// it. The Queued flag is transient and lost when continueRebase reloads the +// stack from disk, so it must be refreshed before the remaining cascade — else +// the frozen merge-queue branch would be rebased. +func TestRebase_Continue_QueuedBranchBelowConflict(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 20}}, + {Branch: "b3"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + // State: b1 (below the queued b2) conflicted; b2 and b3 remain. + state := &rebaseState{ + CurrentBranchIndex: 0, + ConflictBranch: "b1", + RemainingBranches: []string{"b2", "b3"}, + OriginalBranch: "b3", + OriginalRefs: map[string]string{ + "main": "main-orig-sha", + "b1": "sha-b1", + "b2": "sha-b2", + "b3": "sha-b3", + }, + } + stateData, _ := json.MarshalIndent(state, "", " ") + require.NoError(t, os.WriteFile(filepath.Join(tmpDir, "gh-stack-rebase-state"), stateData, 0644)) + + var rebaseCalls []rebaseCall + + mock := newRebaseMock(tmpDir, "b1") + mock.BranchExistsFn = func(name string) bool { return true } + mock.IsRebaseInProgressFn = func() bool { return true } + mock.RebaseContinueFn = func(opts git.RebaseOpts) error { return nil } + mock.RebaseOntoFn = func(newBase, oldBase, branch string, opts git.RebaseOpts) error { + rebaseCalls = append(rebaseCalls, rebaseCall{newBase, oldBase, branch}) + return nil + } + mock.CheckoutBranchFn = func(string) error { return nil } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = queuedPRClient(map[int]string{20: "b2"}) + cmd := RebaseCmd(cfg) + cmd.SetArgs([]string{"--continue"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.Contains(t, output, "Skipping b2") + assert.Contains(t, output, "queued") + + // Only b3 is rebased, onto the queued b2. The queued b2 itself must not be + // rebased (its branch is frozen in the merge queue). + require.Len(t, rebaseCalls, 1) + assert.Equal(t, rebaseCall{"b2", "sha-b2", "b3"}, rebaseCalls[0]) + for _, c := range rebaseCalls { + assert.NotEqual(t, "b2", c.branch, "the frozen queued branch must not be rebased") + } +} + // TestRebase_Continue_OntoMode verifies the --continue path when UseOnto is // set (merged branches upstream). With no remaining branches, only // RebaseContinue runs and the state is cleaned up.