diff --git a/cmd/submit.go b/cmd/submit.go index 59ed811..a8c8b0b 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -563,9 +563,93 @@ func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) { if s.ID != "" { updateStack(cfg, client, s, prNumbers) - } else { - createNewStack(cfg, client, s, prNumbers) + return + } + + // 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. + if adoptRemoteStack(cfg, client, s, prNumbers) { + return } + + createNewStack(cfg, client, s, prNumbers) +} + +// adoptRemoteStack reconciles a locally untracked stack (s.ID == "") with the +// stacks that already exist on GitHub. The PRs in s may already belong to a +// remote stack created from the web UI or another clone; in that case we must +// adopt that stack rather than POST a new one (which the API rejects because +// the PRs are already stacked). +// +// It returns true when it has fully handled the sync — either by adopting and +// updating the existing stack, or by intentionally refusing to modify a +// divergent remote stack — and false when no matching remote stack exists and +// the caller should create a new one. +func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool { + stacks, err := client.ListStacks() + if err != nil { + // Couldn't inspect remote state — fall back to the create path, which + // reports its own errors (handleCreate422 covers "already stacked"). + return false + } + + matched, err := findMatchingStack(stacks, prNumbers) + if err != nil { + // Our PRs are spread across more than one remote stack. A PR can only + // belong to one stack, so this is a genuine divergence we can't resolve + // automatically. + cfg.Warningf("Your PRs belong to multiple stacks on GitHub — reconcile them before submitting") + cfg.Printf(" Run `%s` to import a stack, or unstack the PRs from the web", + cfg.ColorCyan("gh stack checkout ")) + return true + } + + if matched == nil { + // No existing stack contains any of our PRs — create a new one. + return false + } + + // 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.PullRequests, prNumbers); len(dropped) > 0 { + cfg.Warningf("A stack on GitHub already contains %s, which %s not in your local stack", + formatPRList(dropped), plural(len(dropped), "is", "are")) + cfg.Printf(" Run `%s` to import the full stack, then `%s`", + cfg.ColorCyan("gh stack checkout "), cfg.ColorCyan("gh stack submit")) + return true + } + + // Every PR in the remote stack is tracked locally (and we may have added + // more on top). Adopt the remote stack ID — recording it locally — and + // update the stack with our full, ordered PR list to append any new PRs. + s.ID = strconv.Itoa(matched.ID) + + if slicesEqual(matched.PullRequests, prNumbers) { + cfg.Successf("Linked to the existing stack on GitHub (%d PRs, already up to date)", len(prNumbers)) + return true + } + + cfg.Infof("Found the stack on GitHub — updating it to match your local stack") + updateStack(cfg, client, s, prNumbers) + return true +} + +// prsMissingFrom returns the numbers in remote that do not appear in local, +// preserving remote order. +func prsMissingFrom(remote, local []int) []int { + localSet := make(map[int]bool, len(local)) + for _, n := range local { + localSet[n] = true + } + var missing []int + for _, n := range remote { + if !localSet[n] { + missing = append(missing, n) + } + } + return missing } // updateStack calls the PUT endpoint to sync the full PR list for an existing stack. @@ -626,7 +710,7 @@ func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, func handleCreate422(cfg *config.Config, httpErr *api.HTTPError, prNumbers []int) { msg := httpErr.Message - if strings.Contains(msg, "already stacked") { + if isAlreadyStackedError(msg) { // Check if the error lists exactly the same PRs we're trying to // stack. If so, they're already in a stack together — nothing to do. // If only a subset matches, the PRs are in a different stack. @@ -635,8 +719,8 @@ func handleCreate422(cfg *config.Config, httpErr *api.HTTPError, prNumbers []int return } cfg.Warningf("One or more PRs are already part of a different stack on GitHub") - cfg.Printf(" To fix this, unstack the PRs from the web, then `%s`", - cfg.ColorCyan("gh stack submit")) + cfg.Printf(" Run `%s` to import the existing stack, or unstack the PRs from the web", + cfg.ColorCyan("gh stack checkout ")) return } @@ -661,3 +745,13 @@ func allPRsInMessage(msg string, prNumbers []int) bool { } return true } + +// isAlreadyStackedError reports whether a create-stack 422 message indicates +// the PRs already belong to a stack. The server has used more than one phrasing +// ("Pull requests #1, #2 are already stacked", "Pull requests are already part +// of a stack"), so match on the stable substrings rather than an exact string. +func isAlreadyStackedError(msg string) bool { + m := strings.ToLower(msg) + return strings.Contains(m, "already stacked") || + strings.Contains(m, "already part of a stack") +} diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 06f0091..4676bed 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -643,6 +643,243 @@ func TestSyncStack_AlreadyStacked_DifferentStack(t *testing.T) { assert.NotContains(t, output, "up to date") } +func TestSyncStack_AdoptsExistingRemoteStack_ExactMatch(t *testing.T) { + // The stack exists on GitHub but isn't recorded locally (s.ID == ""). + // All local PRs match the remote stack exactly — adopt the ID without + // creating or updating anything. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var createCalled, updateCalled bool + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 77, PullRequests: []int{10, 11}}}, nil + }, + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + UpdateStackFn: func(string, []int) error { + updateCalled = true + return nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.False(t, createCalled, "should not create when the stack already exists on GitHub") + assert.False(t, updateCalled, "should not update when local matches remote exactly") + assert.Equal(t, "77", s.ID, "should adopt the remote stack ID into local tracking") + assert.Contains(t, output, "Linked to the existing stack on GitHub") + assert.Contains(t, output, "up to date") +} + +func TestSyncStack_AdoptsExistingRemoteStack_AddsNewPR(t *testing.T) { + // Two of our three PRs already form a remote stack; the third was added + // locally on top. Adopt the remote ID and update the stack to include the + // new PR at the top. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var createCalled bool + var gotStackID string + var gotNumbers []int + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 77, PullRequests: []int{10, 11}}}, nil + }, + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + UpdateStackFn: func(stackID string, prNumbers []int) error { + gotStackID = stackID + gotNumbers = prNumbers + return nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.False(t, createCalled, "should adopt and update, not create") + assert.Equal(t, "77", s.ID, "should adopt the remote stack ID") + assert.Equal(t, "77", gotStackID, "should update the adopted stack") + assert.Equal(t, []int{10, 11, 12}, gotNumbers, "should send the full local PR list") + assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") +} + +func TestSyncStack_RemoteStackHasExtraPRs_Refuses(t *testing.T) { + // The remote stack contains a PR we aren't tracking locally. Syncing to + // match local would drop it, so refuse and warn instead. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var createCalled, updateCalled bool + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 77, PullRequests: []int{10, 11, 12}}}, nil + }, + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + UpdateStackFn: func(string, []int) error { + updateCalled = true + return nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.False(t, createCalled, "should not create over an existing stack") + assert.False(t, updateCalled, "should not drop remote-only PRs") + assert.Equal(t, "", s.ID, "should not adopt a divergent remote stack") + assert.Contains(t, output, "#12") + assert.Contains(t, output, "not in your local stack") +} + +func TestSyncStack_PRsSpanMultipleRemoteStacks_Warns(t *testing.T) { + // Our PRs are split across two remote stacks — an unresolvable divergence. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var createCalled, updateCalled bool + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{ + {ID: 1, PullRequests: []int{10}}, + {ID: 2, PullRequests: []int{11}}, + }, nil + }, + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + UpdateStackFn: func(string, []int) error { + updateCalled = true + return nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.False(t, createCalled, "should not create when PRs span multiple stacks") + assert.False(t, updateCalled, "should not update when PRs span multiple stacks") + assert.Equal(t, "", s.ID) + assert.Contains(t, output, "multiple stacks") +} + +func TestSyncStack_ListStacksError_FallsThroughToCreate(t *testing.T) { + // If we can't inspect remote stacks, fall back to the create path rather + // than blocking the submit. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var createCalled bool + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return nil, fmt.Errorf("network down") + }, + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 88, nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.True(t, createCalled, "should fall through to CreateStack when ListStacks fails") + assert.Equal(t, "88", s.ID) + assert.Contains(t, output, "Stack created on GitHub with 2 PRs") +} + +func TestSyncStack_AlreadyPartOfAStack_FallbackPhrasing(t *testing.T) { + // Fallback path: ListStacks returns no match (so adoption is skipped), but + // the create endpoint still rejects with the server's "already part of a + // stack" phrasing (no PR numbers). The message must be actionable rather + // than the raw "Could not create stack". + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests are already part of a stack", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "already part of a") + assert.Contains(t, output, "gh stack checkout") + assert.NotContains(t, output, "Could not create stack") +} + func TestSyncStack_InvalidChain_422(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"},