diff --git a/cmd/checkout.go b/cmd/checkout.go index 9a1df2e3..98016f45 100644 --- a/cmd/checkout.go +++ b/cmd/checkout.go @@ -538,24 +538,10 @@ func findLocalStackForRemotePRs(sf *stack.StackFile, prs []*github.PullRequest) return nil } -// stackCompositionMatches checks if a local stack's PR numbers match -// the remote stack's PR numbers in the same order. +// stackCompositionMatches ignores merged history that belongs to a previous stack. func stackCompositionMatches(localStack *stack.Stack, remotePRNumbers []int) bool { - var localPRNumbers []int - for _, b := range localStack.Branches { - if b.PullRequest != nil { - localPRNumbers = append(localPRNumbers, b.PullRequest.Number) - } - } - if len(localPRNumbers) != len(remotePRNumbers) { - return false - } - for i := range localPRNumbers { - if localPRNumbers[i] != remotePRNumbers[i] { - return false - } - } - return true + localPRNumbers := stackPRNumbers(localStack, &github.RemoteStack{PullRequests: remotePRNumbers}) + return slicesEqual(localPRNumbers, remotePRNumbers) } // handleCompositionConflict prompts the user to resolve a mismatch between diff --git a/cmd/checkout_test.go b/cmd/checkout_test.go index 898c0a49..e62de03b 100644 --- a/cmd/checkout_test.go +++ b/cmd/checkout_test.go @@ -1341,6 +1341,28 @@ func TestStackCompositionMatches(t *testing.T) { remote: []int{10, 11}, matches: true, }, + { + name: "merged history from a previous stack", + local: &stack.Stack{ + Branches: []stack.BranchRef{ + {Branch: "a", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + }, + remote: []int{11}, + matches: true, + }, + { + name: "merged member of the current stack", + local: &stack.Stack{ + Branches: []stack.BranchRef{ + {Branch: "a", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + }, + remote: []int{10, 11}, + matches: true, + }, { name: "different order", local: &stack.Stack{ diff --git a/cmd/submit.go b/cmd/submit.go index f310c00f..ad75973d 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -3,6 +3,7 @@ package cmd import ( "errors" "fmt" + "slices" "strconv" "strings" @@ -147,17 +148,6 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { // Sync PR state to detect merged/queued PRs before pushing. prDetails := syncStackPRs(cfg, s) - // If the active branches now sit on top of a fully-merged base, they can no - // longer extend the existing remote stack. Fork them into a fresh stack - // rooted at the trunk and continue the submit with that new stack. - if stacksAvailable { - s, err = maybeForkFromMergedBase(cfg, client, sf, s, gitDir) - if err != nil { - cfg.Errorf("%s", err) - return ErrSilent - } - } - // Resolve remote for pushing remote, err := pickRemote(cfg, currentBranch, opts.remote) if err != nil { @@ -198,6 +188,20 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { } } + // A modify retry may still have the old catalog ID. Refresh detached PRs + // after clearing it so merged or queued PRs are not pushed again. + if pendingModify { + prDetails = syncStackPRs(cfg, s) + } else if stacksAvailable { + s, err = maybeForkFromMergedBase(cfg, client, sf, s, gitDir) + if err != nil { + cfg.Errorf("%s", err) + return ErrSilent + } + } + activeBranches = activeBranchNames(s) + queued = s.QueuedBranches() + // Best-effort fetch to update tracking refs (helps --force-with-lease // in shallow clones). Silently ignored if branches don't exist on the // remote yet. @@ -283,7 +287,7 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { return stackSaveError(cfg, err) } if pendingModify { - if !allPRsSubmitted || (!stackSynced && len(s.Branches) != 1) { + if !allPRsSubmitted || (!stackSynced && len(activeBranches) > 1) { cfg.Errorf("modify is not fully submitted; recovery state was retained") cfg.Printf("Finish submitting all remaining PRs, then retry `%s`", cfg.ColorCyan("gh stack submit")) return ErrSilent @@ -565,7 +569,11 @@ func maybeForkFromMergedBase(cfg *config.Config, client github.ClientOps, sf *st return s, nil } - remotePRs := remoteStackPRs(client, s.ID) + remote, err := remoteStackByID(client, s.ID) + if err != nil || remote == nil { + return s, nil + } + remotePRs := remote.PRNumbers() if len(remotePRs) == 0 { return s, nil } @@ -651,19 +659,17 @@ func maybeForkFromMergedBase(cfg *config.Config, client github.ClientOps, sf *st return &sf.Stacks[len(sf.Stacks)-1], nil } -// remoteStackPRs returns the PR numbers that are officially part of the remote -// stack identified by stackID, or nil if it can't be determined. -func remoteStackPRs(client github.ClientOps, stackID string) []int { +func remoteStackByID(client github.ClientOps, stackID string) (*github.RemoteStack, error) { stacks, err := client.ListStacks() if err != nil { - return nil + return nil, err } - for _, rs := range stacks { - if strconv.Itoa(rs.ID) == stackID { - return rs.PRNumbers() + for i := range stacks { + if strconv.Itoa(stacks[i].ID) == stackID { + return &stacks[i], nil } } - return nil + return nil, nil } // mergedPRNumbers returns the set of PR numbers whose local branch is marked @@ -679,9 +685,36 @@ func mergedPRNumbers(s *stack.Stack) map[int]bool { return merged } +// prepareStackRecreation recognizes a merged-only remote remnant and records +// fresh merge state without removing any local branches. +func prepareStackRecreation(s *stack.Stack, remaining *github.RemoteStack) bool { + if remaining == nil || len(remaining.PRNumbers()) == 0 { + return false + } + merged := mergedPRNumbers(s) + for _, pr := range remaining.PRDetails { + if pr.IsMerged() { + merged[pr.Number] = true + } + } + for _, number := range remaining.PRNumbers() { + if !merged[number] { + return false + } + } + for i := range s.Branches { + b := &s.Branches[i] + if b.PullRequest != nil && merged[b.PullRequest.Number] { + b.PullRequest.Merged = true + b.Queued = false + } + } + return true +} + // handlePendingModify handles the stack recreation after a modify operation. -// It deletes the old remote stack and clears s.ID so syncStack creates a new -// one. The bool identifies a matching pending modification. Its journal is +// It unstacks the old PRs and clears s.ID so syncStack creates a replacement. +// The bool identifies a matching pending modification. Its journal is // cleared only after the caller verifies all required submission outcomes. func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.Stack, gitDir string) (bool, error) { state, err := modify.LoadState(gitDir) @@ -719,17 +752,19 @@ func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.S } } - // Delete the old remote stack + // Release the old stack's unmerged PRs before submitting the replacement. if state.PriorRemoteStackID != "" { - number, found, lookupErr := stackNumberByID(client, state.PriorRemoteStackID) + remote, lookupErr := remoteStackByID(client, state.PriorRemoteStackID) if lookupErr != nil { cfg.Warningf("Failed to look up existing stack: %v", lookupErr) cfg.Printf("Run `%s` again to retry", cfg.ColorCyan("gh stack submit")) return true, lookupErr } - if !found { + if remote == nil { cfg.Printf("Previous stack already deleted on GitHub") - } else if _, dissolved, err := client.Unstack(number); err != nil { + } else if prepareStackRecreation(s, remote) { + cfg.Infof("Only merged pull requests remain in the previous stack on GitHub") + } else if remaining, dissolved, err := client.Unstack(remote.Number); err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { cfg.Printf("Previous stack already deleted on GitHub") @@ -738,11 +773,14 @@ func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.S cfg.Printf("Run `%s` again to retry", cfg.ColorCyan("gh stack submit")) return true, err } - } else if !dissolved { - cfg.Errorf("the previous stack still has pull requests queued for merge or with auto-merge enabled; it cannot be recreated yet") + } else if !dissolved && !prepareStackRecreation(s, remaining) { + cfg.Errorf("Some pull requests still belong to the previous stack on GitHub; it cannot be recreated yet") return true, ErrConflict - } else { + } else if dissolved { cfg.Successf("Cleared existing stack on GitHub") + } else { + cfg.Successf("Unstacked pull requests from the previous stack on GitHub") + cfg.Infof("Merged pull requests remain in the previous stack on GitHub") } } // Record branch identity before the replacement receives a different ID. @@ -778,10 +816,8 @@ func clearPendingModifyState(cfg *config.Config, s *stack.Stack, gitDir string) return nil } -// syncStack creates or updates a stack on GitHub from the active PRs. -// If the stack already exists (s.ID is set), it calls the PUT endpoint with -// the full list of PRs to keep the remote stack in sync. If no stack exists -// yet, it calls POST to create one. +// syncStack creates or updates a stack on GitHub. Existing stacks keep their +// merged members; new stacks exclude merged history from previous stacks. // This is a best-effort operation: failures are reported as warnings but do // not cause the submit command to fail (the PRs are already created). // @@ -789,30 +825,43 @@ func clearPendingModifyState(cfg *config.Config, s *stack.Stack, gitDir string) // (created, updated, or already in sync) and false otherwise (fewer than two // PRs, an unresolved divergence, stacked PRs unavailable, or an API failure). func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool { - // Collect PR numbers in stack order (bottom to top), including merged PRs. - // The API expects the full list — omitting merged PRs causes a - // "Stack contents have changed" rejection. - var prNumbers []int + prCount := 0 for _, b := range s.Branches { if b.PullRequest != nil { - prNumbers = append(prNumbers, b.PullRequest.Number) + prCount++ } } // The API requires at least 2 PRs to form a stack. - if len(prNumbers) < 2 { + if prCount < 2 { return false } if s.ID != "" { - return updateStack(cfg, client, s, prNumbers) + return updateStack(cfg, client, s) } // No locally tracked stack ID. The stack may already exist on GitHub // (created from the web UI or another clone) without being recorded // locally. Adopt it instead of blindly creating a new one, which the API // rejects because the PRs are already part of a stack. - return reconcileUntrackedStack(cfg, client, s, prNumbers) + return reconcileUntrackedStack(cfg, client, s, stackPRNumbers(s, nil)) +} + +// stackPRNumbers includes merged PRs only when they belong to the remote stack +// being updated, rather than to a previous stack retained after unstacking. +func stackPRNumbers(s *stack.Stack, remote *github.RemoteStack) []int { + var members []int + if remote != nil { + members = remote.PRNumbers() + } + var numbers []int + for _, b := range s.Branches { + if b.PullRequest != nil && (!b.IsMerged() || slices.Contains(members, b.PullRequest.Number)) { + numbers = append(numbers, b.PullRequest.Number) + } + } + return numbers } // reconcileUntrackedStack reconciles a locally untracked stack (s.ID == "") @@ -828,7 +877,7 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta if err != nil { // Couldn't inspect remote state — fall back to the create path, which // reports its own errors (handleCreate422 covers "already stacked"). - return createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s) } matched, err := findMatchingStack(stacks, prNumbers) @@ -844,9 +893,10 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta if matched == nil { // No existing stack contains any of our PRs — create a new one. - return createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s) } + prNumbers = stackPRNumbers(s, matched) // A remote stack already contains some of our PRs. Refuse to silently drop // any PRs it holds that we aren't tracking locally; let the user reconcile. if dropped := prsMissingFrom(matched.PRNumbers(), prNumbers); len(dropped) > 0 { @@ -869,7 +919,7 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta } cfg.Infof("Found the stack on GitHub — updating it to match your local stack") - return updateStack(cfg, client, s, prNumbers) + return updateStack(cfg, client, s) } // prsMissingFrom returns the numbers in remote that do not appear in local, @@ -895,13 +945,13 @@ func prsMissingFrom(remote, local []int) []int { // leaves the remote stack untouched. If the remote stack is gone (404) it // clears the local ID and re-creates it. Returns true when the remote stack // reflects the local stack (updated, already in sync, or recreated). -func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool { +func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool { number, err := ensureStackNumber(client, s) if err != nil || number == 0 { // Can't resolve the remote stack — treat as missing and (re)create. s.ID = "" s.Number = 0 - return createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s) } remote, err := client.GetStack(number) @@ -910,13 +960,20 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { s.ID = "" s.Number = 0 - return createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s) } cfg.Warningf("Failed to read stack on GitHub: %v", err) return false } + if prepareStackRecreation(s, remote) && len(s.ActiveBranches()) > 0 { + s.ID = "" + s.Number = 0 + return createNewStack(cfg, client, s) + } + current := remote.PRNumbers() + prNumbers := stackPRNumbers(s, remote) if slicesEqual(current, prNumbers) { s.ID = strconv.Itoa(remote.ID) s.Number = remote.Number @@ -947,7 +1004,7 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // immediately try to re-create it. s.ID = "" s.Number = 0 - return createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s) case 422: // A merged branch whose ref has been deleted upstream breaks the // stack's base→head chain, so the update is rejected. This is @@ -977,7 +1034,11 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // createNewStack calls the POST endpoint to create a new stack, handling the // three types of 422 errors the API may return. // Returns true when the stack was created or is confirmed already in sync. -func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool { +func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool { + prNumbers := stackPRNumbers(s, nil) + if len(prNumbers) < 2 { + return false + } rs, err := client.CreateStack(prNumbers) if err == nil { s.ID = strconv.Itoa(rs.ID) diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 4918d595..f37ff2b3 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -653,7 +653,7 @@ func TestUpdateStack_BrokenChainAfterMerge(t *testing.T) { AddToStackFn: func(int, []int) (*github.RemoteStack, error) { return nil, mustFormErr() }, } cfg, _, errR := config.NewTestConfig() - updateStack(cfg, mock, s, []int{1, 2, 3}) + updateStack(cfg, mock, s) cfg.Err.Close() out, _ := io.ReadAll(errR) output := string(out) @@ -678,7 +678,7 @@ func TestUpdateStack_BrokenChainAfterMerge(t *testing.T) { AddToStackFn: func(int, []int) (*github.RemoteStack, error) { return nil, mustFormErr() }, } cfg, _, errR := config.NewTestConfig() - updateStack(cfg, mock, s, []int{1, 2}) + updateStack(cfg, mock, s) cfg.Err.Close() out, _ := io.ReadAll(errR) output := string(out) @@ -1274,29 +1274,62 @@ func TestSyncStack_SkippedForSinglePR(t *testing.T) { assert.False(t, updateCalled, "UpdateStack should not be called with fewer than 2 PRs") } -func TestSyncStack_IncludesMergedBranches(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", PullRequest: &stack.PullRequestRef{Number: 12}}, - }, - } - - var gotNumbers []int - mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { - gotNumbers = prNumbers - return &github.RemoteStack{ID: 42, Number: 42}, nil - }, +func TestSyncStack_MergedMembership(t *testing.T) { + for _, tc := range []struct { + name string + tracked bool + remotePRs []int + wantCreate []int + wantAdd []int + }{ + {name: "create without historical stack", remotePRs: []int{10}, wantCreate: []int{11, 12}}, + {name: "recreate a tracked remnant", tracked: true, remotePRs: []int{10}, wantCreate: []int{11, 12}}, + {name: "update keeps merged member", tracked: true, remotePRs: []int{10, 11}, wantAdd: []int{12}}, + {name: "update excludes historical nonmember", tracked: true, remotePRs: []int{11, 12}}, + {name: "adopt keeps merged member", remotePRs: []int{10, 11, 12}}, + } { + t.Run(tc.name, func(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", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + if tc.tracked { + s.ID, s.Number = "77", 7 + } + remote := &github.RemoteStack{ID: 77, Number: 7, PullRequests: tc.remotePRs} + var created, added []int + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{*remote}, nil + }, + GetStackFn: func(int) (*github.RemoteStack, error) { return remote, nil }, + CreateStackFn: func(numbers []int) (*github.RemoteStack, error) { + created = numbers + return &github.RemoteStack{ID: 88, Number: 8, PullRequests: numbers}, nil + }, + AddToStackFn: func(number int, numbers []int) (*github.RemoteStack, error) { + assert.Equal(t, 7, number) + added = numbers + return &github.RemoteStack{ID: 77, Number: 7, PullRequests: append(tc.remotePRs, numbers...)}, nil + }, + } + cfg, outR, errR := config.NewTestConfig() + assert.True(t, syncStack(cfg, mock, s)) + commandOutput(t, cfg, outR, errR) + assert.Equal(t, tc.wantCreate, created) + assert.Equal(t, tc.wantAdd, added) + if tc.wantCreate != nil { + assert.Equal(t, "88", s.ID) + } else { + assert.Equal(t, "77", s.ID) + } + assert.Equal(t, []string{"b1", "b2", "b3"}, s.BranchNames()) + }) } - - cfg, _, _ := config.NewTestConfig() - syncStack(cfg, mock, s) - cfg.Err.Close() - - assert.Equal(t, []int{10, 11, 12}, gotNumbers, "should include merged PRs to keep API in sync") } func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { @@ -1831,6 +1864,9 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { newPR bool failure string priorDeleted bool + mergedBase bool + retry bool + mergeOnRetry bool }{ {name: "one existing PR", branches: 1}, {name: "one new PR", branches: 1, newPR: true}, @@ -1844,7 +1880,12 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { {name: "one PR failed ready update", branches: 1, failure: "ready"}, {name: "one PR failed catalog save", branches: 1, failure: "save"}, {name: "one PR lost during final refresh", branches: 1, failure: "refresh"}, + {name: "one PR after partial unstack", branches: 1, mergedBase: true}, {name: "multiple PRs complete", branches: 3}, + {name: "multiple PRs after partial unstack", branches: 3, mergedBase: true}, + {name: "retry push after partial unstack", branches: 3, mergedBase: true, failure: "push", retry: true}, + {name: "released PR merged before retry", branches: 3, mergedBase: true, failure: "push", retry: true, mergeOnRetry: true}, + {name: "retry creation after partial unstack", branches: 3, mergedBase: true, failure: "stack", retry: true}, {name: "multiple PRs failed lookup", branches: 3, failure: "lookup"}, {name: "multiple PRs failed creation", branches: 3, newPR: true, failure: "create"}, {name: "multiple PRs failed base update", branches: 3, failure: "base"}, @@ -1856,6 +1897,15 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { prs := make(map[string]*github.PullRequest) oldNumbers := []int{99, 100} target := fmt.Sprintf("b%d", tc.branches) + if tc.mergedBase { + s.Branches = append(s.Branches, stack.BranchRef{ + Branch: "merged", PullRequest: &stack.PullRequestRef{Number: 9, Merged: true}, + }) + prs["merged"] = &github.PullRequest{ + Number: 9, HeadRefName: "merged", BaseRefName: "main", State: "MERGED", Merged: true, + } + oldNumbers = append(oldNumbers, 9) + } for i := 0; i < tc.branches; i++ { name := fmt.Sprintf("b%d", i+1) br := stack.BranchRef{Branch: name} @@ -1915,10 +1965,14 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { if !oldDeleted { return []github.RemoteStack{{ID: 42, Number: 7, PullRequests: oldNumbers}}, nil } + var stacks []github.RemoteStack + if tc.mergedBase { + stacks = append(stacks, github.RemoteStack{ID: 42, Number: 7, PullRequests: []int{9}}) + } if replacement != nil { - return []github.RemoteStack{*replacement}, nil + stacks = append(stacks, *replacement) } - return nil, nil + return stacks, nil }, GetStackFn: func(number int) (*github.RemoteStack, error) { if number == 7 && !oldDeleted { @@ -1944,12 +1998,18 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { if branch == target && (tc.failure == "lookup" || (tc.failure == "refresh" && baseUpdates > 0)) { return nil, assert.AnError } + if pr := prs[branch]; pr != nil && pr.Merged { + return nil, nil + } return prs[branch], nil }, UnstackFn: func(number int) (*github.RemoteStack, bool, error) { require.Equal(t, 7, number) unstackCalls++ oldDeleted = true + if tc.mergedBase { + return &github.RemoteStack{ID: 42, Number: 7, PullRequests: []int{9}}, false, nil + } return nil, true, nil }, CreatePRFn: func(base, head, _, _ string, _ bool) (*github.PullRequest, error) { @@ -1982,6 +2042,7 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { MarkPRReadyForReviewFn: func(string) error { return assert.AnError }, CreateStackFn: func(numbers []int) (*github.RemoteStack, error) { require.GreaterOrEqual(t, len(numbers), 2) + assert.NotContains(t, numbers, 9, "merged history must not join the replacement stack") if tc.failure == "stack" { return nil, assert.AnError } @@ -2011,8 +2072,10 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { assert.Len(t, pushes, tc.branches) saved, loadErr := stack.Load(dir) require.NoError(t, loadErr) - require.NotNil(t, saved.Stacks[0].Branches[tc.branches-1].PullRequest) - assert.Equal(t, prs[target].Number, saved.Stacks[0].Branches[tc.branches-1].PullRequest.Number) + assert.Equal(t, s.BranchNames(), saved.Stacks[0].BranchNames()) + targetIndex := saved.Stacks[0].IndexOf(target) + require.NotNil(t, saved.Stacks[0].Branches[targetIndex].PullRequest) + assert.Equal(t, prs[target].Number, saved.Stacks[0].Branches[targetIndex].PullRequest.Number) if tc.branches == 1 { assert.Empty(t, saved.Stacks[0].ID) assert.Nil(t, replacement) @@ -2025,6 +2088,37 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) { assert.Equal(t, 1, baseUpdates) } } + if tc.retry { + if tc.mergeOnRetry { + before, loadErr := stack.Load(dir) + require.NoError(t, loadErr) + assert.Equal(t, "42", before.Stacks[0].ID) + prs["b2"].Merged = true + prs["b2"].State = "MERGED" + } + tc.failure = "" + pushStart := len(pushes) + retryCfg, retryOut, retryErr := config.NewTestConfig() + retryCfg.GitHubClientOverride = client + err := runSubmit(retryCfg, &submitOptions{auto: true}) + _, diagnostics := commandOutput(t, retryCfg, retryOut, retryErr) + require.NoError(t, err, diagnostics) + assert.NoFileExists(t, modify.StatePath(dir)) + saved, loadErr := stack.Load(dir) + require.NoError(t, loadErr) + require.Len(t, saved.Stacks, 1) + assert.Equal(t, s.BranchNames(), saved.Stacks[0].BranchNames()) + assert.Equal(t, "77", saved.Stacks[0].ID) + if tc.mergeOnRetry { + assert.Equal(t, []string{"b1", "b3"}, pushes[pushStart:]) + assert.Zero(t, createdPRs) + require.NotNil(t, replacement) + assert.Equal(t, []int{10, 12}, replacement.PullRequests) + mergedIndex := saved.Stacks[0].IndexOf("b2") + require.NotEqual(t, -1, mergedIndex) + assert.True(t, saved.Stacks[0].Branches[mergedIndex].IsMerged()) + } + } if tc.priorDeleted { assert.Zero(t, unstackCalls) } else { @@ -2128,27 +2222,72 @@ func TestPendingModify_RetryAfterOldStackDeletion(t *testing.T) { commandOutput(t, cfg, outR, errR) } -func TestPendingModify_PartialUnstackPreservesState(t *testing.T) { - dir := t.TempDir() - saveModifyState(t, dir, newPendingSubmitState("123")) - s := &stack.Stack{ID: "123", Number: 7} - cfg, outR, errR := config.NewTestConfig() - client := &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 123, Number: 7}}, nil - }, - UnstackFn: func(int) (*github.RemoteStack, bool, error) { return nil, false, nil }, +func TestPendingModify_PartialUnstack(t *testing.T) { + for _, tc := range []struct { + name string + remaining []int + alreadyUnstacked bool + freshlyMerged bool + wantConflict bool + }{ + {name: "merged", remaining: []int{10}}, + {name: "merged during unstack", remaining: []int{10}, freshlyMerged: true}, + {name: "already unstacked", remaining: []int{10}, alreadyUnstacked: true}, + {name: "queued", remaining: []int{11}, wantConflict: true}, + {name: "merged and queued", remaining: []int{10, 11}, wantConflict: true}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + s := &stack.Stack{ + ID: "123", Number: 7, Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: !tc.freshlyMerged}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + state := newPendingSubmitState("123") + state.RecordStack(s) + saveModifyState(t, dir, state) + remaining := &github.RemoteStack{ID: 123, Number: 7, PullRequests: tc.remaining} + if tc.freshlyMerged { + mergedAt := "2026-01-01T00:00:00Z" + remaining.PRDetails = []github.RemoteStackPR{{Number: 10, MergedAt: &mergedAt}} + } + unstackCalled := false + client := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + if tc.alreadyUnstacked { + return []github.RemoteStack{*remaining}, nil + } + return []github.RemoteStack{{ID: 123, Number: 7, PullRequests: []int{10, 11, 12}}}, nil + }, + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + unstackCalled = true + return remaining, false, nil + }, + } + cfg, outR, errR := config.NewTestConfig() + handled, err := handlePendingModify(cfg, client, s, dir) + commandOutput(t, cfg, outR, errR) + assert.True(t, handled) + assert.Equal(t, !tc.alreadyUnstacked, unstackCalled) + state, loadErr := modify.LoadState(dir) + require.NoError(t, loadErr) + require.NotNil(t, state, "keep recovery until submission completes") + if tc.wantConflict { + assert.ErrorIs(t, err, ErrConflict) + assert.Equal(t, "123", s.ID) + assert.Equal(t, 7, s.Number) + assert.Equal(t, "123", state.PriorRemoteStackID) + } else { + require.NoError(t, err) + assert.Empty(t, s.ID) + assert.Zero(t, s.Number) + assert.Empty(t, state.PriorRemoteStackID) + assert.True(t, s.Branches[0].IsMerged()) + } + }) } - handled, err := handlePendingModify(cfg, client, s, dir) - assert.ErrorIs(t, err, ErrConflict) - assert.True(t, handled) - assert.Equal(t, "123", s.ID) - assert.Equal(t, 7, s.Number) - state, err := modify.LoadState(dir) - require.NoError(t, err) - require.NotNil(t, state) - assert.Equal(t, "123", state.PriorRemoteStackID) - commandOutput(t, cfg, outR, errR) } func TestSubmit_UnrelatedPendingModifyIsPreserved(t *testing.T) {