From 83298f1465faba3d918b5fffa915ce1318ab08b4 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 09:28:11 -0400 Subject: [PATCH] Adopt an existing remote stack on submit instead of erroring When a stack existed on GitHub but was not recorded in the local tracking file (`.git/gh-stack`, i.e. `s.ID == ""`) -- for example a stack created from the web UI or in another clone -- `gh stack submit` blindly called the create-stack API. The server rejected the request because the PRs were already stacked, surfacing a confusing, dead-end error: Could not create stack: Pull requests are already part of a stack. `syncStack()` only ever consulted the local `s.ID`. When it was empty it always POSTed to create, and the only handling for the rejection (`handleCreate422`) parsed the error string and at best printed "up to date" -- it never imported the remote stack ID, so the local/remote mismatch was never resolved and recurred on every submit. Detect and adopt the existing stack before creating one. When `s.ID` is empty and there are at least two PRs, `syncStack` now lists the repository's stacks and reconciles, mirroring the patterns already used by `checkout` and `link`: - No remote stack contains our PRs -> create a new one (unchanged). - The remote stack's PRs are all tracked locally (an exact match, or e.g. two of three already stacked with a third added on top) -> adopt the remote stack ID into local tracking and PUT the full, ordered PR list, appending any new PRs to the top. The adopted ID is persisted by the existing stack.Save in runSubmit. - The remote stack contains PRs we are not tracking locally -> refuse and warn rather than silently dropping them, pointing the user to `gh stack checkout ` to import the full stack first. - Our PRs are spread across multiple remote stacks (an unresolvable divergence, since a PR can belong to only one stack) -> warn and skip. The new logic reuses `findMatchingStack`, `formatPRList`, and `slicesEqual` from the link command; `syncStack`'s signature is unchanged, and adoption is best-effort: listing failures degrade to the previous create path and never fail the submit, since the PRs are already pushed and created. Also broaden the create-stack 422 fallback (`handleCreate422`) to recognize the server's "already part of a stack" phrasing in addition to "already stacked", so that if the stack listing is unavailable the message is still actionable instead of the raw "Could not create stack" error above. Tests: six new cases in cmd/submit_test.go cover exact-match adoption (ID imported, no create/update call), additive two-of-three adoption (update with the full list), remote-superset refusal, PRs spanning multiple stacks, the ListStacks-error fallthrough to create, and the broadened fallback phrasing. Existing syncStack tests are unchanged. --- cmd/submit.go | 104 +++++++++++++++++++- cmd/submit_test.go | 237 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 336 insertions(+), 5 deletions(-) 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"},