diff --git a/cmd/checkout.go b/cmd/checkout.go index 526e18f..368e1ed 100644 --- a/cmd/checkout.go +++ b/cmd/checkout.go @@ -188,12 +188,13 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string, return nil, "", ErrAPIFailure } - // Step 1: List stacks and find one containing the target PR - remoteStack, err := findRemoteStackForPR(client, prNumber) + // Step 1: Find the stack containing the target PR via the list endpoint's + // server-side pull_request filter. + remoteStack, err := client.FindStackForPR(prNumber) if err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { - warnStacksUnavailableOrPAT(cfg) + warnStacksUnavailable(cfg) return nil, "", ErrAPIFailure } cfg.Errorf("failed to list stacks: %v", err) @@ -205,7 +206,7 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string, } // Step 2: Fetch PR details for every PR in the remote stack - prs, err := fetchStackPRDetails(client, remoteStack.PullRequests) + prs, err := fetchStackPRDetails(client, remoteStack.PRNumbers()) if err != nil { cfg.Errorf("failed to fetch PR details: %v", err) return nil, "", ErrAPIFailure @@ -245,11 +246,14 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string, syncRemotePRState(localStack, prs) // Case A: branch is in a local stack — check composition - if stackCompositionMatches(localStack, remoteStack.PullRequests) { + if stackCompositionMatches(localStack, remoteStack.PRNumbers()) { // Composition matches — checkout - if localStack.ID == "" { - localStack.ID = remoteStackID - } + // remoteStack is authoritative for both identifiers here, so + // refresh them together. Updating only one (e.g. the number while + // keeping a stale ID) breaks later ID-based discovery when the old + // remote stack was replaced by a new one holding the same PRs. + localStack.ID = remoteStackID + localStack.Number = remoteStack.Number if err := stack.Save(gitDir, sf); err != nil { return nil, "", handleSaveError(cfg, err) } @@ -274,7 +278,7 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string, return nil, "", ErrSilent } - s, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID) + s, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID, remoteStack.Number) if err != nil { return nil, "", err } @@ -286,23 +290,6 @@ func checkoutRemoteStack(cfg *config.Config, sf *stack.StackFile, gitDir string, return s, targetBranch, nil } -// findRemoteStackForPR queries the list stacks API and returns the stack -// containing the given PR number, or nil if no stack contains it. -func findRemoteStackForPR(client github.ClientOps, prNumber int) (*github.RemoteStack, error) { - stacks, err := client.ListStacks() - if err != nil { - return nil, err - } - for i := range stacks { - for _, n := range stacks[i].PullRequests { - if n == prNumber { - return &stacks[i], nil - } - } - } - return nil, nil -} - // fetchStackPRDetails fetches PR details for each number in the stack. // Returns PRs in the same order as the input numbers. func fetchStackPRDetails(client github.ClientOps, prNumbers []int) ([]*github.PullRequest, error) { @@ -418,7 +405,7 @@ func handleCompositionConflict( return nil, ErrSilent } - s, importErr := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID) + s, importErr := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID, remoteStack.Number) if importErr != nil { return nil, importErr } @@ -429,19 +416,26 @@ func handleCompositionConflict( return s, nil case 1: - // Delete remote stack, keep local - if err := client.DeleteStack(remoteStackID); err != nil { + // Unstack the remote stack, keep local + _, dissolved, err := client.Unstack(remoteStack.Number) + if err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { - cfg.Warningf("Remote stack already deleted") + cfg.Warningf("Remote stack already removed") + } else if errors.As(err, &httpErr) && httpErr.StatusCode == 422 { + cfg.Errorf("Cannot unstack remote stack: %s", httpErr.Message) + return nil, ErrAPIFailure } else { - cfg.Errorf("failed to delete remote stack: %v", err) + cfg.Errorf("failed to unstack remote stack: %v", err) return nil, ErrAPIFailure } + } else if dissolved { + cfg.Successf("Remote stack removed") } else { - cfg.Successf("Remote stack deleted") + cfg.Warningf("Some pull requests could not be unstacked and remain on GitHub") } localStack.ID = "" + localStack.Number = 0 if err := stack.Save(gitDir, sf); err != nil { return nil, handleSaveError(cfg, err) } @@ -475,6 +469,7 @@ func importRemoteStack( trunk string, prs []*github.PullRequest, remoteStackID string, + remoteStackNumber int, ) (*stack.Stack, error) { // Fetch latest refs from remote if err := git.Fetch(remote); err != nil { @@ -512,7 +507,8 @@ func importRemoteStack( trunkSHA, _ := git.RevParse(trunk) newStack := stack.Stack{ - ID: remoteStackID, + ID: remoteStackID, + Number: remoteStackNumber, Trunk: stack.BranchRef{ Branch: trunk, Head: trunkSHA, diff --git a/cmd/checkout_test.go b/cmd/checkout_test.go index b6e2113..d7fbcef 100644 --- a/cmd/checkout_test.go +++ b/cmd/checkout_test.go @@ -158,9 +158,9 @@ func TestCheckout_NumericTarget_StacksNotAvailable(t *testing.T) { require.NoError(t, stack.Save(gitDir, &stack.StackFile{SchemaVersion: 1, Stacks: []stack.Stack{}})) cfg, outR, errR := config.NewTestConfig() - setTestTokenForHost(cfg, "gho_test_oauth_token") + setTestRepo(cfg) cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + FindStackForPRFn: func(int) (*github.RemoteStack, error) { return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"} }, } @@ -184,10 +184,8 @@ func TestCheckout_NumericTarget_PRNotInStack(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 1, PullRequests: []int{10, 11}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return nil, nil // PR 99 is not part of any stack }, } @@ -243,10 +241,8 @@ func TestCheckout_NumericTarget_NewStack(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 42, PullRequests: []int{10, 11, 12}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{10, 11, 12}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -331,10 +327,8 @@ func TestCheckout_NumericTarget_BranchExistsNoStack(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 99, PullRequests: []int{10, 11}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -445,11 +439,9 @@ func TestCheckout_NumericTarget_LocalMiss_RemoteMatch(t *testing.T) { apiCalled := false cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + FindStackForPRFn: func(int) (*github.RemoteStack, error) { apiCalled = true - return []github.RemoteStack{ - {ID: 99, PullRequests: []int{10, 11}}, - }, nil + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -464,7 +456,7 @@ func TestCheckout_NumericTarget_LocalMiss_RemoteMatch(t *testing.T) { _ = collectOutput(cfg, outR, errR) require.NoError(t, err) - assert.True(t, apiCalled, "should have called ListStacks API when local miss") + assert.True(t, apiCalled, "should have queried the remote stack API when local miss") assert.Equal(t, "feat-2", checkedOut) } @@ -493,8 +485,8 @@ func TestCheckout_NumericTarget_FallbackToBranchName(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{}, nil // no remote stacks + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return nil, nil // no remote stack contains this PR }, } @@ -527,11 +519,9 @@ func TestCheckout_NumericTarget_CompositionMismatch_NonInteractive(t *testing.T) cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + FindStackForPRFn: func(int) (*github.RemoteStack, error) { // Remote stack has PRs 10, 11, 12 (extra PR added) - return []github.RemoteStack{ - {ID: 42, PullRequests: []int{10, 11, 12}}, - }, nil + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{10, 11, 12}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -590,10 +580,8 @@ func TestCheckout_NumericTarget_ClosedMergedPR(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 50, PullRequests: []int{10, 11}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 50, Number: 50, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -662,10 +650,8 @@ func TestCheckout_NumericTarget_MergedBranchDeletedFromRemote(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 60, PullRequests: []int{10, 11}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 60, Number: 60, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -698,10 +684,8 @@ func TestCheckout_NumericTarget_AllPRsMerged(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 70, PullRequests: []int{10, 11}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 70, Number: 70, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { prs := map[int]*github.PullRequest{ @@ -732,7 +716,7 @@ func TestCheckout_NumericTarget_APIError(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { + FindStackForPRFn: func(int) (*github.RemoteStack, error) { return nil, fmt.Errorf("network error") }, } @@ -796,8 +780,8 @@ func TestCheckout_NumericTarget_EmptyStacks(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{}, nil // no stacks at all + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return nil, nil // no stacks at all }, } @@ -903,34 +887,6 @@ func TestStackCompositionMatches(t *testing.T) { } } -func TestFindRemoteStackForPR(t *testing.T) { - mock := &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 1, PullRequests: []int{10, 11}}, - {ID: 2, PullRequests: []int{20, 21, 22}}, - }, nil - }, - } - - // Found in first stack - rs, err := findRemoteStackForPR(mock, 11) - require.NoError(t, err) - require.NotNil(t, rs) - assert.Equal(t, 1, rs.ID) - - // Found in second stack - rs, err = findRemoteStackForPR(mock, 21) - require.NoError(t, err) - require.NotNil(t, rs) - assert.Equal(t, 2, rs.ID) - - // Not found - rs, err = findRemoteStackForPR(mock, 99) - require.NoError(t, err) - assert.Nil(t, rs) -} - func TestCheckout_ByPRURL_Local(t *testing.T) { // When a PR URL resolves to a locally tracked stack, no API call needed gitDir := t.TempDir() @@ -973,14 +929,14 @@ func TestCheckout_ByPRURL_Remote(t *testing.T) { } restore := git.SetOps(&git.MockOps{ - GitDirFn: func() (string, error) { return gitDir, nil }, - CurrentBranchFn: func() (string, error) { return "main", nil }, - BranchExistsFn: func(name string) bool { return name == "main" }, - FetchFn: func(string) error { return nil }, - CreateBranchFn: func(string, string) error { return nil }, + GitDirFn: func() (string, error) { return gitDir, nil }, + CurrentBranchFn: func() (string, error) { return "main", nil }, + BranchExistsFn: func(name string) bool { return name == "main" }, + FetchFn: func(string) error { return nil }, + CreateBranchFn: func(string, string) error { return nil }, SetUpstreamTrackingFn: func(string, string) error { return nil }, - RevParseFn: func(string) (string, error) { return "abc123", nil }, - ResolveRemoteFn: func(string) (string, error) { return "origin", nil }, + RevParseFn: func(string) (string, error) { return "abc123", nil }, + ResolveRemoteFn: func(string) (string, error) { return "origin", nil }, CheckoutBranchFn: func(name string) error { checkedOut = name return nil @@ -993,10 +949,8 @@ func TestCheckout_ByPRURL_Remote(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{ - {ID: 1, PullRequests: []int{10, 11}}, - }, nil + FindStackForPRFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 1, Number: 1, PullRequests: []int{10, 11}}, nil }, FindPRByNumberFn: func(n int) (*github.PullRequest, error) { if pr, ok := prDB[n]; ok { diff --git a/cmd/link.go b/cmd/link.go index 93989f0..0fc9aa7 100644 --- a/cmd/link.go +++ b/cmd/link.go @@ -321,7 +321,7 @@ func findExistingPR(cfg *config.Config, client github.ClientOps, arg string) (*r func validatePREligibility(cfg *config.Config, found []*resolvedArg, targetStack *github.RemoteStack) error { inTargetStack := make(map[int]bool) if targetStack != nil { - for _, n := range targetStack.PullRequests { + for _, n := range targetStack.PRNumbers() { inTargetStack[n] = true } } @@ -366,7 +366,7 @@ func listStacksSafe(cfg *config.Config, client github.ClientOps) ([]github.Remot if err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { - warnStacksUnavailableOrPAT(cfg) + warnStacksUnavailable(cfg) return nil, ErrStacksUnavailable } cfg.Errorf("failed to list stacks: %v", err) @@ -392,7 +392,7 @@ func prevalidateStack(cfg *config.Config, matchedStack *github.RemoteStack, know } var dropped []int - for _, n := range matchedStack.PullRequests { + for _, n := range matchedStack.PRNumbers() { if !knownSet[n] { dropped = append(dropped, n) } @@ -401,7 +401,7 @@ func prevalidateStack(cfg *config.Config, matchedStack *github.RemoteStack, know if len(dropped) > 0 { cfg.Errorf("Cannot update stack: this would remove %s from the stack", formatPRList(dropped)) - cfg.Printf("Current stack: %s", formatPRList(matchedStack.PullRequests)) + cfg.Printf("Current stack: %s", formatPRList(matchedStack.PRNumbers())) cfg.Printf("Include all existing PRs in the command to update the stack") return ErrInvalidArgs } @@ -535,7 +535,7 @@ func findMatchingStack(stacks []github.RemoteStack, prNumbers []int) (*github.Re var matched *github.RemoteStack for i := range stacks { - for _, n := range stacks[i].PullRequests { + for _, n := range stacks[i].PRNumbers() { if prSet[n] { if matched != nil && matched.ID != stacks[i].ID { return nil, fmt.Errorf("PRs belong to multiple stacks — unstack them first, then re-link") @@ -551,8 +551,7 @@ func findMatchingStack(stacks []github.RemoteStack, prNumbers []int) (*github.Re // createLink creates a new stack with the given PR numbers. func createLink(cfg *config.Config, client github.ClientOps, prNumbers []int) error { - _, err := client.CreateStack(prNumbers) - if err != nil { + if _, err := client.CreateStack(prNumbers); err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) { switch httpErr.StatusCode { @@ -560,7 +559,7 @@ func createLink(cfg *config.Config, client github.ClientOps, prNumbers []int) er cfg.Errorf("Cannot create stack: %s", httpErr.Message) return ErrAPIFailure case 404: - warnStacksUnavailableOrPAT(cfg) + warnStacksUnavailable(cfg) return ErrStacksUnavailable default: cfg.Errorf("Failed to create stack (HTTP %d): %s", httpErr.StatusCode, httpErr.Message) @@ -576,10 +575,14 @@ func createLink(cfg *config.Config, client github.ClientOps, prNumbers []int) er } // updateLink updates an existing stack with the given PR numbers. -// The update is additive-only: it errors if any existing PRs would be removed. +// The update is additive-only: it errors if any existing PRs would be removed, +// and (because the add endpoint appends to the top) if the existing PRs are not +// an ordered prefix of the desired list. func updateLink(cfg *config.Config, client github.ClientOps, existing *github.RemoteStack, prNumbers []int) error { + current := existing.PRNumbers() + // Check if the input exactly matches the existing stack. - if slicesEqual(existing.PullRequests, prNumbers) { + if slicesEqual(current, prNumbers) { cfg.Successf("Stack with %d PRs is already up to date", len(prNumbers)) return nil } @@ -591,7 +594,7 @@ func updateLink(cfg *config.Config, client github.ClientOps, existing *github.Re } var dropped []int - for _, n := range existing.PullRequests { + for _, n := range current { if !newSet[n] { dropped = append(dropped, n) } @@ -600,13 +603,21 @@ func updateLink(cfg *config.Config, client github.ClientOps, existing *github.Re if len(dropped) > 0 { cfg.Errorf("Cannot update stack: this would remove %s from the stack", formatPRList(dropped)) - cfg.Printf("Current stack: %s", formatPRList(existing.PullRequests)) + cfg.Printf("Current stack: %s", formatPRList(current)) cfg.Printf("Include all existing PRs in the command to update the stack") return ErrInvalidArgs } - stackID := strconv.Itoa(existing.ID) - if err := client.UpdateStack(stackID, prNumbers); err != nil { + // The add endpoint appends to the top of the stack, so the existing PRs + // must be an ordered prefix of the desired list. + delta, ok := appendDelta(current, prNumbers) + if !ok { + cfg.Errorf("Cannot update stack: new PRs must be added to the top of the existing stack") + cfg.Printf("Current stack: %s", formatPRList(current)) + return ErrInvalidArgs + } + + if _, err := client.AddToStack(existing.Number, delta); err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) { switch httpErr.StatusCode { @@ -642,6 +653,22 @@ func slicesEqual(a, b []int) bool { return true } +// appendDelta returns the PR numbers that must be appended to current to reach +// desired, with ok=true, when current is an exact ordered prefix of desired. +// When desired diverges from current (a reorder or removal), ok is false. This +// mirrors the Stacks add endpoint, which only appends to the top of a stack. +func appendDelta(current, desired []int) (delta []int, ok bool) { + if len(current) > len(desired) { + return nil, false + } + for i, n := range current { + if desired[i] != n { + return nil, false + } + } + return desired[len(current):], true +} + func formatPRList(numbers []int) string { if len(numbers) == 0 { return "" diff --git a/cmd/link_test.go b/cmd/link_test.go index 7c6ccdd..15aeb23 100644 --- a/cmd/link_test.go +++ b/cmd/link_test.go @@ -47,9 +47,9 @@ func TestLink_PRNumbers_CreateNewStack(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createdPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -69,7 +69,7 @@ func TestLink_PRNumbers_CreateNewStack(t *testing.T) { } func TestLink_PRNumbers_UpdateExistingStack(t *testing.T) { - var updatedID string + var updatedNumber int var updatedPRs []int cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ @@ -83,13 +83,13 @@ func TestLink_PRNumbers_UpdateExistingStack(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{10, 20}}, + {ID: 7, Number: 7, PullRequests: []int{10, 20}}, }, nil }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - updatedID = stackID + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { + updatedNumber = stackNumber updatedPRs = prNumbers - return nil + return &github.RemoteStack{ID: 7, Number: stackNumber, PullRequests: []int{10, 20, 30}}, nil }, } @@ -104,8 +104,8 @@ func TestLink_PRNumbers_UpdateExistingStack(t *testing.T) { output := string(errOut) assert.NoError(t, err) - assert.Equal(t, "7", updatedID) - assert.Equal(t, []int{10, 20, 30}, updatedPRs) + assert.Equal(t, 7, updatedNumber) + assert.Equal(t, []int{30}, updatedPRs) assert.Contains(t, output, "Updated stack to 3 PRs") } @@ -122,12 +122,12 @@ func TestLink_PRNumbers_ExactMatch_NoOp(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{10, 20, 30}}, + {ID: 7, Number: 7, PullRequests: []int{10, 20, 30}}, }, nil }, - UpdateStackFn: func(string, []int) error { - t.Fatal("UpdateStack should not be called for exact match") - return nil + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack should not be called for exact match") + return nil, nil }, } @@ -158,7 +158,7 @@ func TestLink_PRNumbers_WouldRemovePRs(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{10, 20, 30}}, + {ID: 7, Number: 7, PullRequests: []int{10, 20, 30}}, }, nil }, } @@ -190,8 +190,8 @@ func TestLink_PRNumbers_MultipleStacks(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 1, PullRequests: []int{10, 20}}, - {ID: 2, PullRequests: []int{30, 40}}, + {ID: 1, Number: 1, PullRequests: []int{10, 20}}, + {ID: 2, Number: 2, PullRequests: []int{30, 40}}, }, nil }, } @@ -244,7 +244,7 @@ func TestLink_DuplicateArgs(t *testing.T) { func TestLink_StacksUnavailable(t *testing.T) { cfg, _, errR := config.NewTestConfig() - setTestTokenForHost(cfg, "gho_test_oauth_token") + setTestRepo(cfg) cfg.GitHubClientOverride = &github.MockClient{ FindPRByNumberFn: func(n int) (*github.PullRequest, error) { return &github.PullRequest{Number: n, HeadRefName: "b", BaseRefName: "main"}, nil @@ -277,8 +277,8 @@ func TestLink_Create422(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 0, &api.HTTPError{ + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 0, Number: 0}, &api.HTTPError{ StatusCode: 422, Message: "Pull requests must form a stack", } @@ -379,9 +379,9 @@ func TestLink_RejectsQueuedPR(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { t.Fatal("CreateStack should not be called for ineligible PRs") - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil }, } @@ -419,9 +419,9 @@ func TestLink_RejectsAutoMergeEnabledPR(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { t.Fatal("CreateStack should not be called for ineligible PRs") - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil }, } @@ -561,7 +561,7 @@ func TestLink_ReportsMultipleIneligiblePRs(t *testing.T) { // Regression test to ensure a queued PR that is already a member of // the target stack does not block adding new PRs to that same stack. func TestLink_AllowsQueuedPRAlreadyInStack(t *testing.T) { - var updatedID string + var updatedNumber int var updatedPRs []int cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ @@ -581,17 +581,17 @@ func TestLink_AllowsQueuedPRAlreadyInStack(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{100}}, + {ID: 7, Number: 7, PullRequests: []int{100}}, }, nil }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - updatedID = stackID + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { + updatedNumber = stackNumber updatedPRs = prNumbers - return nil + return &github.RemoteStack{ID: 7, Number: stackNumber, PullRequests: []int{100, 101, 102}}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { t.Fatal("CreateStack should not be called when updating an existing stack") - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil }, } @@ -606,8 +606,8 @@ func TestLink_AllowsQueuedPRAlreadyInStack(t *testing.T) { output := string(errOut) require.NoError(t, err) - assert.Equal(t, "7", updatedID) - assert.Equal(t, []int{100, 101, 102}, updatedPRs) + assert.Equal(t, 7, updatedNumber) + assert.Equal(t, []int{101, 102}, updatedPRs) assert.NotContains(t, output, "cannot be added to a stack") } @@ -633,12 +633,12 @@ func TestLink_AllowsMergedPRAlreadyInStack(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 8, PullRequests: []int{100}}, + {ID: 8, Number: 8, PullRequests: []int{100}}, }, nil }, - UpdateStackFn: func(_ string, prNumbers []int) error { + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { updatedPRs = prNumbers - return nil + return &github.RemoteStack{ID: 8, Number: stackNumber, PullRequests: []int{100, 101}}, nil }, } @@ -653,7 +653,7 @@ func TestLink_AllowsMergedPRAlreadyInStack(t *testing.T) { output := string(errOut) require.NoError(t, err) - assert.Equal(t, []int{100, 101}, updatedPRs) + assert.Equal(t, []int{101}, updatedPRs) assert.NotContains(t, output, "cannot be added to a stack") } @@ -678,12 +678,12 @@ func TestLink_AllowsAutoMergePRAlreadyInStack(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 9, PullRequests: []int{100}}, + {ID: 9, Number: 9, PullRequests: []int{100}}, }, nil }, - UpdateStackFn: func(_ string, prNumbers []int) error { + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { updatedPRs = prNumbers - return nil + return &github.RemoteStack{ID: 9, Number: stackNumber, PullRequests: []int{100, 101}}, nil }, } @@ -698,7 +698,7 @@ func TestLink_AllowsAutoMergePRAlreadyInStack(t *testing.T) { output := string(errOut) require.NoError(t, err) - assert.Equal(t, []int{100, 101}, updatedPRs) + assert.Equal(t, []int{101}, updatedPRs) assert.NotContains(t, output, "cannot be added to a stack") } @@ -724,12 +724,12 @@ func TestLink_RejectsQueuedPRNotInStack_WhenAddingToExistingStack(t *testing.T) }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{100}}, + {ID: 7, Number: 7, PullRequests: []int{100}}, }, nil }, - UpdateStackFn: func(string, []int) error { - t.Fatal("UpdateStack should not be called when a new PR is ineligible") - return nil + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack should not be called when a new PR is ineligible") + return nil, nil }, } @@ -776,9 +776,9 @@ func TestLink_BranchNames_AllHavePRs(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { stackedPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -840,9 +840,9 @@ func TestLink_BranchNames_CreatesMissingPRs(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { stackedPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -900,8 +900,8 @@ func TestLink_BranchNames_AllNeedPRs(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -960,7 +960,7 @@ func TestLink_BranchNames_DefaultDraft(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1004,7 +1004,7 @@ func TestLink_BranchNames_OpenFlag(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1062,7 +1062,7 @@ func TestLink_OpenFlag_ConvertsDraftPRs(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1109,9 +1109,9 @@ func TestLink_MixedArgs_PRNumberAndBranch(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { stackedPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1161,9 +1161,9 @@ func TestLink_NumericArg_PRNotFound_TreatedAsBranch(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { stackedPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1227,7 +1227,7 @@ func TestLink_FixesBaseBranches(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 42, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 42, Number: 42}, nil }, } cmd := LinkCmd(cfg) @@ -1282,15 +1282,15 @@ func TestLink_UpdateDeletedStack_FallsBackToCreate(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{10}}, + {ID: 7, Number: 7, PullRequests: []int{10}}, }, nil }, - UpdateStackFn: func(string, []int) error { - return &api.HTTPError{StatusCode: 404, Message: "Not Found"} + AddToStackFn: func(stackNumber int, _ []int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"} }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { created = true - return 99, nil + return &github.RemoteStack{ID: 99, Number: 99}, nil }, } @@ -1338,7 +1338,7 @@ func TestLink_PushesBranchesBeforeResolution(t *testing.T) { return &github.PullRequest{Number: n, HeadRefName: fmt.Sprintf("b%d", n), BaseRefName: "main"}, nil }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1383,7 +1383,7 @@ func TestLink_RemoteFlag(t *testing.T) { return &github.PullRequest{Number: n, HeadRefName: fmt.Sprintf("b%d", n), BaseRefName: "main"}, nil }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1414,7 +1414,7 @@ func TestLink_SkipsPushForPRNumbersOnly(t *testing.T) { return &github.PullRequest{Number: n, HeadRefName: "b", BaseRefName: "main"}, nil }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1453,7 +1453,7 @@ func TestLink_PrevalidatesBeforeCreatingPRs(t *testing.T) { }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 7, PullRequests: []int{104, 105, 106}}, + {ID: 7, Number: 7, PullRequests: []int{104, 105, 106}}, }, nil }, } @@ -1636,7 +1636,7 @@ func TestLink_SkipsBaseFix_ForNewlyCreatedPRs(t *testing.T) { }, nil }, ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := LinkCmd(cfg) @@ -1687,7 +1687,7 @@ func TestLink_BranchNames_UsesPRTemplate(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 42, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 42, Number: 42}, nil }, } cmd := LinkCmd(cfg) @@ -1742,7 +1742,7 @@ func TestLink_IgnoresSymlinkedPRTemplate(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 42, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 42, Number: 42}, nil }, } cmd := LinkCmd(cfg) @@ -1795,7 +1795,7 @@ func TestLink_PRNumbers_NoTemplateUsesFooter(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func([]int) (int, error) { return 42, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 42, Number: 42}, nil }, } cmd := LinkCmd(cfg) @@ -1828,9 +1828,9 @@ func TestLink_PRURLs_CreateNewStack(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createdPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1893,9 +1893,9 @@ func TestLink_MixedURLsAndNumbers(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createdPRs = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } diff --git a/cmd/submit.go b/cmd/submit.go index a8b5bf2..919054d 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -113,16 +113,11 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { return ErrAPIFailure } - // Pre-flight: abort early if the user is authenticating with a PAT. - if cfg.WarnIfPAT() { - return ErrStacksUnavailable - } - // Verify that the repository has stacked PRs enabled. stacksAvailable := s.ID != "" if !stacksAvailable { if _, err := client.ListStacks(); err != nil { - warnStacksUnavailableOrPAT(cfg) + warnStacksUnavailable(cfg) if cfg.IsInteractive() { p := prompter.New(cfg.In, cfg.Out, cfg.Err) proceed, promptErr := p.Confirm("Would you still like to create regular PRs?", false) @@ -609,7 +604,7 @@ func remoteStackPRs(client github.ClientOps, stackID string) []int { } for _, rs := range stacks { if strconv.Itoa(rs.ID) == stackID { - return rs.PullRequests + return rs.PRNumbers() } } return nil @@ -661,7 +656,15 @@ func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.S // Delete the old remote stack if state.PriorRemoteStackID != "" { - if err := client.DeleteStack(state.PriorRemoteStackID); err != nil { + number, found, lookupErr := stackNumberByID(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 lookupErr + } + if !found { + cfg.Printf("Previous stack already deleted on GitHub") + } else if _, _, err := client.Unstack(number); err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { cfg.Printf("Previous stack already deleted on GitHub") @@ -675,6 +678,7 @@ func handlePendingModify(cfg *config.Config, client github.ClientOps, s *stack.S } // Clear the old stack ID so syncStack creates a new one s.ID = "" + s.Number = 0 } return nil @@ -761,7 +765,7 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta // 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 { + if dropped := prsMissingFrom(matched.PRNumbers(), 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", @@ -773,8 +777,9 @@ func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *sta // 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) + s.Number = matched.Number - if slicesEqual(matched.PullRequests, prNumbers) { + if slicesEqual(matched.PRNumbers(), prNumbers) { cfg.Successf("Linked to the existing stack on GitHub (%d PRs, already up to date)", len(prNumbers)) return true } @@ -799,12 +804,57 @@ func prsMissingFrom(remote, local []int) []int { return missing } -// updateStack calls the PUT endpoint to sync the full PR list for an existing stack. -// If the remote stack was deleted (404), it clears the local ID and falls through -// to createNewStack so the user doesn't need to re-run the command. -// Returns true when the remote stack was updated (or recreated) successfully. +// updateStack brings the remote stack in line with the local PR list by +// appending any new PRs via the add endpoint. It reads the current remote stack +// to compute the delta; when the desired list isn't a clean append onto the +// remote stack (a reorder or a removal, e.g. merged PRs leaving the stack) it +// 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 { - if err := client.UpdateStack(s.ID, prNumbers); err != nil { + 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) + } + + remote, err := client.GetStack(number) + if err != nil { + var httpErr *api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { + s.ID = "" + s.Number = 0 + return createNewStack(cfg, client, s, prNumbers) + } + cfg.Warningf("Failed to read stack on GitHub: %v", err) + return false + } + + current := remote.PRNumbers() + if slicesEqual(current, prNumbers) { + s.ID = strconv.Itoa(remote.ID) + s.Number = remote.Number + cfg.Successf("Stack on GitHub is up to date with %d PRs", len(prNumbers)) + return true + } + + delta, isAppend := appendDelta(current, prNumbers) + if !isAppend || len(delta) == 0 { + // The desired list isn't a clean append onto the remote stack — the add + // endpoint can't express a reorder or removal. This is expected once + // part of the stack has landed and merged PRs have left the stack. + if len(s.MergedBranches()) > 0 { + cfg.Infof("Merged PRs have left the stack on GitHub, so it wasn't updated — your unmerged PRs were pushed and re-based onto the trunk") + } else { + cfg.Warningf("The stack on GitHub differs from your local stack and couldn't be updated automatically") + } + return false + } + + rs, err := client.AddToStack(number, delta) + if err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) { switch httpErr.StatusCode { @@ -812,6 +862,7 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // Stack was deleted on GitHub — clear the stale ID and // immediately try to re-create it. s.ID = "" + s.Number = 0 return createNewStack(cfg, client, s, prNumbers) case 422: // A merged branch whose ref has been deleted upstream breaks the @@ -832,6 +883,9 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr } return false } + + s.ID = strconv.Itoa(rs.ID) + s.Number = rs.Number cfg.Successf("Stack updated on GitHub with %d PRs", len(prNumbers)) return true } @@ -840,9 +894,10 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // 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 { - stackID, err := client.CreateStack(prNumbers) + rs, err := client.CreateStack(prNumbers) if err == nil { - s.ID = strconv.Itoa(stackID) + s.ID = strconv.Itoa(rs.ID) + s.Number = rs.Number cfg.Successf("Stack created on GitHub with %d PRs", len(prNumbers)) return true } @@ -857,7 +912,7 @@ func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, case 422: return handleCreate422(cfg, httpErr, prNumbers) case 404: - warnStacksUnavailableOrPAT(cfg) + warnStacksUnavailable(cfg) return false default: cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message) diff --git a/cmd/submit_test.go b/cmd/submit_test.go index 75c93b5..74cf8a1 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -134,8 +134,8 @@ func TestSubmit_CreatesPRsAndStack(t *testing.T) { URL: fmt.Sprintf("https://github.com/owner/repo/pull/%d", prCounter), }, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -194,7 +194,7 @@ func TestSubmit_DefaultDraft(t *testing.T) { createdDraft = draft return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := SubmitCmd(cfg) @@ -236,7 +236,7 @@ func TestSubmit_OpenFlag(t *testing.T) { createdDraft = draft return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := SubmitCmd(cfg) @@ -293,7 +293,7 @@ func TestSubmit_OpenFlag_ConvertsDraftPRs(t *testing.T) { markedReady = append(markedReady, prID) return nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := SubmitCmd(cfg) @@ -412,8 +412,9 @@ func TestSubmit_ForksWhenRemoteStackFullyMerged(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { s := stack.Stack{ - ID: "42", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "42", + Number: 42, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2, Merged: true}}, @@ -469,9 +470,9 @@ func TestSubmit_ForksWhenRemoteStackFullyMerged(t *testing.T) { HeadRefName: head, }, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createStackPRs = prNumbers - return 99, nil + return &github.RemoteStack{ID: 99, Number: 99}, nil }, } @@ -562,7 +563,7 @@ func TestSubmit_NoForkWhenRemoteStackHasOpenPR(t *testing.T) { cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 42, PullRequests: []int{1, 2, 3}}}, nil + return []github.RemoteStack{{ID: 42, Number: 42, PullRequests: []int{1, 2, 3}}}, nil }, FindPRByNumberFn: func(n int) (*github.PullRequest, error) { switch n { @@ -585,12 +586,15 @@ func TestSubmit_NoForkWhenRemoteStackHasOpenPR(t *testing.T) { HeadRefName: head, }, nil }, - UpdateStackFn: func(string, []int) error { + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{1, 2, 3}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { // Merged-and-deleted base branches break the chain on GitHub. - return &api.HTTPError{ + return nil, &api.HTTPError{ StatusCode: 422, Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/42"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks/42/add"}, } }, } @@ -627,21 +631,27 @@ func TestUpdateStack_BrokenChainAfterMerge(t *testing.T) { return &api.HTTPError{ StatusCode: 422, Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/42"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks/42/add"}, } } t.Run("merged branches present is reported calmly", func(t *testing.T) { s := &stack.Stack{ - ID: "42", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "42", + Number: 42, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2}}, {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 3}}, }, } - mock := &github.MockClient{UpdateStackFn: func(string, []int) error { return mustFormErr() }} + mock := &github.MockClient{ + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{1, 2}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { return nil, mustFormErr() }, + } cfg, _, errR := config.NewTestConfig() updateStack(cfg, mock, s, []int{1, 2, 3}) cfg.Err.Close() @@ -653,14 +663,20 @@ func TestUpdateStack_BrokenChainAfterMerge(t *testing.T) { t.Run("no merged branches still warns", func(t *testing.T) { s := &stack.Stack{ - ID: "42", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "42", + Number: 42, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 2}}, }, } - mock := &github.MockClient{UpdateStackFn: func(string, []int) error { return mustFormErr() }} + mock := &github.MockClient{ + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{1}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { return nil, mustFormErr() }, + } cfg, _, errR := config.NewTestConfig() updateStack(cfg, mock, s, []int{1, 2}) cfg.Err.Close() @@ -731,9 +747,9 @@ func TestSyncStack_NewStack_CreateSuccess(t *testing.T) { var gotNumbers []int mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { gotNumbers = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -751,8 +767,9 @@ func TestSyncStack_NewStack_CreateSuccess(t *testing.T) { func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) { s := &stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -760,18 +777,21 @@ func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) { }, } - var gotStackID string + var gotStackNumber int var gotNumbers []int createCalled := false mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: stackNumber, Number: stackNumber, PullRequests: []int{10, 11}}, nil }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - gotStackID = stackID + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { + gotStackNumber = stackNumber gotNumbers = prNumbers - return nil + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11, 12}}, nil }, } @@ -783,15 +803,16 @@ func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) { output := string(errOut) assert.False(t, createCalled, "CreateStack should not be called when s.ID is set") - assert.Equal(t, "99", gotStackID) - assert.Equal(t, []int{10, 11, 12}, gotNumbers) + assert.Equal(t, 99, gotStackNumber) + assert.Equal(t, []int{12}, gotNumbers) assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") } func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) { s := &stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -799,11 +820,14 @@ func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) { } mock := &github.MockClient{ - UpdateStackFn: func(string, []int) error { - return &api.HTTPError{ + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 422, Message: "Validation failed", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks/99/add"}, } }, } @@ -820,8 +844,9 @@ func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) { func TestSyncStack_ExistingStack_Update404(t *testing.T) { s := &stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -830,16 +855,19 @@ func TestSyncStack_ExistingStack_Update404(t *testing.T) { var createCalled bool mock := &github.MockClient{ - UpdateStackFn: func(string, []int) error { - return &api.HTTPError{ + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 404, Message: "Not Found", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks/99/add"}, } }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createCalled = true - return 55, nil + return &github.RemoteStack{ID: 55, Number: 55}, nil }, } @@ -866,11 +894,11 @@ func TestSyncStack_AlreadyStacked_OurStack(t *testing.T) { } mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ + CreateStackFn: func([]int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 422, Message: "Pull requests #10, #11 are already stacked", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks"}, } }, } @@ -898,11 +926,11 @@ func TestSyncStack_AlreadyStacked_DifferentStack(t *testing.T) { } mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ + CreateStackFn: func([]int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 422, Message: "Pull requests #10, #11 are already stacked", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks"}, } }, } @@ -933,15 +961,15 @@ func TestSyncStack_AdoptsExistingRemoteStack_ExactMatch(t *testing.T) { var createCalled, updateCalled bool mock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 77, PullRequests: []int{10, 11}}}, nil + return []github.RemoteStack{{ID: 77, Number: 77, PullRequests: []int{10, 11}}}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil }, - UpdateStackFn: func(string, []int) error { + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { updateCalled = true - return nil + return &github.RemoteStack{ID: 77, Number: 77, PullRequests: []int{10, 11}}, nil }, } @@ -973,20 +1001,23 @@ func TestSyncStack_AdoptsExistingRemoteStack_AddsNewPR(t *testing.T) { } var createCalled bool - var gotStackID string + var gotStackNumber int var gotNumbers []int mock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 77, PullRequests: []int{10, 11}}}, nil + return []github.RemoteStack{{ID: 77, Number: 77, PullRequests: []int{10, 11}}}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 77, Number: stackNumber, PullRequests: []int{10, 11}}, nil }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - gotStackID = stackID + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { + gotStackNumber = stackNumber gotNumbers = prNumbers - return nil + return &github.RemoteStack{ID: 77, Number: 77, PullRequests: []int{10, 11, 12}}, nil }, } @@ -999,8 +1030,8 @@ func TestSyncStack_AdoptsExistingRemoteStack_AddsNewPR(t *testing.T) { 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.Equal(t, 77, gotStackNumber, "should update the adopted stack") + assert.Equal(t, []int{12}, gotNumbers, "should send only the new PR delta") assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") } @@ -1018,15 +1049,15 @@ func TestSyncStack_RemoteStackHasExtraPRs_Refuses(t *testing.T) { var createCalled, updateCalled bool mock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 77, PullRequests: []int{10, 11, 12}}}, nil + return []github.RemoteStack{{ID: 77, Number: 77, PullRequests: []int{10, 11, 12}}}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil }, - UpdateStackFn: func(string, []int) error { + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { updateCalled = true - return nil + return &github.RemoteStack{ID: 77, Number: 77, PullRequests: []int{10, 11, 12}}, nil }, } @@ -1058,17 +1089,17 @@ func TestSyncStack_PRsSpanMultipleRemoteStacks_Warns(t *testing.T) { mock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 1, PullRequests: []int{10}}, - {ID: 2, PullRequests: []int{11}}, + {ID: 1, Number: 1, PullRequests: []int{10}}, + {ID: 2, Number: 2, PullRequests: []int{11}}, }, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 0, nil + return &github.RemoteStack{ID: 0, Number: 0}, nil }, - UpdateStackFn: func(string, []int) error { + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { updateCalled = true - return nil + return &github.RemoteStack{ID: 1, Number: 1}, nil }, } @@ -1101,9 +1132,9 @@ func TestSyncStack_ListStacksError_FallsThroughToCreate(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return nil, fmt.Errorf("network down") }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 88, nil + return &github.RemoteStack{ID: 88, Number: 88}, nil }, } @@ -1134,11 +1165,11 @@ func TestSyncStack_AlreadyPartOfAStack_FallbackPhrasing(t *testing.T) { mock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ + CreateStackFn: func([]int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 422, Message: "Pull requests are already part of a stack", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks"}, } }, } @@ -1165,11 +1196,11 @@ func TestSyncStack_InvalidChain_422(t *testing.T) { } mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ + CreateStackFn: func([]int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 422, Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks"}, } }, } @@ -1195,11 +1226,11 @@ func TestSyncStack_NotAvailable(t *testing.T) { } mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ + CreateStackFn: func([]int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{ StatusCode: 404, Message: "Not Found", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks"}, } }, } @@ -1225,13 +1256,13 @@ func TestSyncStack_SkippedForSinglePR(t *testing.T) { createCalled := false updateCalled := false mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, - UpdateStackFn: func(string, []int) error { + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { updateCalled = true - return nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1255,9 +1286,9 @@ func TestSyncStack_IncludesMergedBranches(t *testing.T) { var gotNumbers []int mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { gotNumbers = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1280,9 +1311,9 @@ func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { var gotNumbers []int mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { gotNumbers = prNumbers - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1343,8 +1374,8 @@ func TestSubmit_UpdatesBaseBranch(t *testing.T) { }{number, base}) return nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1370,8 +1401,9 @@ func TestSubmit_UpdatesBaseBranch(t *testing.T) { func TestSubmit_SkipsBaseUpdateWhenStacked(t *testing.T) { // Stack already exists (s.ID is set), so base updates should be skipped. s := stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -1410,8 +1442,11 @@ func TestSubmit_SkipsBaseUpdateWhenStacked(t *testing.T) { updateCalled = true return nil }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - return nil + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{10, 11}}, nil }, } @@ -1494,8 +1529,8 @@ func TestSubmit_CreatesMissingPRsAndUpdatesExisting(t *testing.T) { }{number, base}) return nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1555,9 +1590,7 @@ func TestSubmit_PreflightCheck_404_BailsOut(t *testing.T) { }, } - // Use an OAuth token so the PAT pre-flight check passes and we - // exercise the ListStacks 404 path. - setTestTokenForHost(cfg, "gho_test_oauth_token") + setTestRepo(cfg) cmd := SubmitCmd(cfg) cmd.SetArgs([]string{"--auto"}) @@ -1610,9 +1643,7 @@ func TestSubmit_PreflightCheck_404_Interactive_UserDeclinesAborts(t *testing.T) }, } - // Use an OAuth token so the PAT pre-flight check passes and we - // exercise the ListStacks 404 path. - setTestTokenForHost(cfg, "gho_test_oauth_token") + setTestRepo(cfg) cmd := SubmitCmd(cfg) cmd.SetArgs([]string{"--auto"}) @@ -1642,9 +1673,9 @@ func TestSyncStack_SkippedWhenStacksUnavailable(t *testing.T) { createCalled := false mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createCalled = true - return 42, nil + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -1699,7 +1730,7 @@ func TestSubmit_PreflightCheck_EmptyList_Proceeds(t *testing.T) { CreatePRFn: func(base, head, title, body string, draft bool) (*github.PullRequest, error) { return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil }, - CreateStackFn: func([]int) (int, error) { return 99, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 99, Number: 99}, nil }, } cmd := SubmitCmd(cfg) @@ -1717,8 +1748,9 @@ func TestSubmit_PreflightCheck_EmptyList_Proceeds(t *testing.T) { func TestSubmit_PreflightCheck_SkippedWhenStackIDSet(t *testing.T) { s := stack.Stack{ - ID: "42", // Existing stack — pre-flight check should be skipped. - Trunk: stack.BranchRef{Branch: "main"}, + ID: "42", // Existing stack — pre-flight check should be skipped. + Number: 42, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -1738,7 +1770,7 @@ func TestSubmit_PreflightCheck_SkippedWhenStackIDSet(t *testing.T) { cfg.GitHubClientOverride = &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { listStacksCallCount++ - return []github.RemoteStack{{ID: 42, PullRequests: []int{10, 11}}}, nil + return []github.RemoteStack{{ID: 42, Number: 42, PullRequests: []int{10, 11}}}, nil }, FindPRByNumberFn: func(number int) (*github.PullRequest, error) { switch number { @@ -1752,7 +1784,12 @@ func TestSubmit_PreflightCheck_SkippedWhenStackIDSet(t *testing.T) { FindPRForBranchFn: func(string) (*github.PullRequest, error) { return &github.PullRequest{Number: 10, URL: "https://github.com/o/r/pull/10"}, nil }, - UpdateStackFn: func(string, []int) error { return nil }, + GetStackFn: func(int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{10}}, nil + }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42, PullRequests: []int{10, 11}}, nil + }, } cmd := SubmitCmd(cfg) @@ -1790,15 +1827,18 @@ func newPendingSubmitState(priorStackID string) *modify.StateFile { func TestHandlePendingModify_DeletesOldStack(t *testing.T) { gitDir := t.TempDir() - saveModifyState(t, gitDir, newPendingSubmitState("stack-123")) + saveModifyState(t, gitDir, newPendingSubmitState("123")) - s := &stack.Stack{ID: "stack-123", Trunk: stack.BranchRef{Branch: "main"}} + s := &stack.Stack{ID: "123", Number: 42, Trunk: stack.BranchRef{Branch: "main"}} - var deletedStackID string + var unstackedNumber int client := &github.MockClient{ - DeleteStackFn: func(id string) error { - deletedStackID = id - return nil + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 123, Number: 42}}, nil + }, + UnstackFn: func(number int) (*github.RemoteStack, bool, error) { + unstackedNumber = number + return nil, true, nil }, } @@ -1808,7 +1848,7 @@ func TestHandlePendingModify_DeletesOldStack(t *testing.T) { err := handlePendingModify(cfg, client, s, gitDir) require.NoError(t, err) - assert.Equal(t, "stack-123", deletedStackID) + assert.Equal(t, 42, unstackedNumber) assert.Equal(t, "", s.ID) } @@ -1820,9 +1860,9 @@ func TestHandlePendingModify_NoStateFile(t *testing.T) { deleteCalled := false client := &github.MockClient{ - DeleteStackFn: func(id string) error { + UnstackFn: func(int) (*github.RemoteStack, bool, error) { deleteCalled = true - return nil + return nil, true, nil }, } @@ -1832,7 +1872,7 @@ func TestHandlePendingModify_NoStateFile(t *testing.T) { err := handlePendingModify(cfg, client, s, gitDir) assert.NoError(t, err) - assert.False(t, deleteCalled, "DeleteStack should not be called when no state file exists") + assert.False(t, deleteCalled, "Unstack should not be called when no state file exists") assert.Equal(t, "stack-123", s.ID, "stack ID should remain unchanged") } @@ -1850,9 +1890,9 @@ func TestHandlePendingModify_WrongPhase(t *testing.T) { deleteCalled := false client := &github.MockClient{ - DeleteStackFn: func(id string) error { + UnstackFn: func(int) (*github.RemoteStack, bool, error) { deleteCalled = true - return nil + return nil, true, nil }, } @@ -1862,20 +1902,23 @@ func TestHandlePendingModify_WrongPhase(t *testing.T) { err := handlePendingModify(cfg, client, s, gitDir) assert.NoError(t, err) - assert.False(t, deleteCalled, "DeleteStack should not be called for non-pending_submit phase") + assert.False(t, deleteCalled, "Unstack should not be called for non-pending_submit phase") assert.Equal(t, "stack-99", s.ID, "stack ID should remain unchanged") } func TestHandlePendingModify_DeleteFails(t *testing.T) { gitDir := t.TempDir() - saveModifyState(t, gitDir, newPendingSubmitState("stack-456")) + saveModifyState(t, gitDir, newPendingSubmitState("456")) - s := &stack.Stack{ID: "stack-456", Trunk: stack.BranchRef{Branch: "main"}} + s := &stack.Stack{ID: "456", Number: 43, Trunk: stack.BranchRef{Branch: "main"}} client := &github.MockClient{ - DeleteStackFn: func(id string) error { - return fmt.Errorf("server error") + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 456, Number: 43}}, nil + }, + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + return nil, false, fmt.Errorf("server error") }, } @@ -1885,22 +1928,25 @@ func TestHandlePendingModify_DeleteFails(t *testing.T) { err := handlePendingModify(cfg, client, s, gitDir) assert.Error(t, err) - assert.Equal(t, "stack-456", s.ID, "stack ID should NOT be cleared on delete failure") + assert.Equal(t, "456", s.ID, "stack ID should NOT be cleared on delete failure") } func TestHandlePendingModify_Delete404(t *testing.T) { gitDir := t.TempDir() - saveModifyState(t, gitDir, newPendingSubmitState("stack-gone")) + saveModifyState(t, gitDir, newPendingSubmitState("404")) - s := &stack.Stack{ID: "stack-gone", Trunk: stack.BranchRef{Branch: "main"}} + s := &stack.Stack{ID: "404", Number: 44, Trunk: stack.BranchRef{Branch: "main"}} client := &github.MockClient{ - DeleteStackFn: func(id string) error { - return &api.HTTPError{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 404, Number: 44}}, nil + }, + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + return nil, false, &api.HTTPError{ StatusCode: 404, Message: "Not Found", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/stack-gone"}, + RequestURL: &url.URL{Path: "/repos/o/r/stacks/44"}, } }, } @@ -1943,8 +1989,9 @@ func TestClearPendingModifyState_NoFile(t *testing.T) { func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) { s := stack.Stack{ - ID: "old-stack-42", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "42", + Number: 7, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, @@ -1954,7 +2001,7 @@ func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) { tmpDir := t.TempDir() writeStackFile(t, tmpDir, s) - saveModifyState(t, tmpDir, newPendingSubmitState("old-stack-42")) + saveModifyState(t, tmpDir, newPendingSubmitState("42")) // Track call ordering var callOrder []string @@ -1970,15 +2017,17 @@ func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) { restore := git.SetOps(mock) defer restore() - var deletedStackID string + var unstackedNumber int var createdStackPRs []int + unstacked := false cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - DeleteStackFn: func(id string) error { - deletedStackID = id - callOrder = append(callOrder, "delete:"+id) - return nil + UnstackFn: func(number int) (*github.RemoteStack, bool, error) { + unstackedNumber = number + unstacked = true + callOrder = append(callOrder, fmt.Sprintf("unstack:%d", number)) + return nil, true, nil }, FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { switch branch { @@ -2006,13 +2055,17 @@ func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) { } return nil, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createdStackPRs = prNumbers callOrder = append(callOrder, "create_stack") - return 99, nil + return &github.RemoteStack{ID: 99, Number: 99}, nil }, ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{}, nil + // The old stack exists until it is unstacked, then it is gone. + if unstacked { + return []github.RemoteStack{}, nil + } + return []github.RemoteStack{{ID: 42, Number: 7, PullRequests: []int{10, 11, 12}}}, nil }, } @@ -2027,8 +2080,8 @@ func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) { assert.NoError(t, err) - // DeleteStack called with old stack ID - assert.Equal(t, "old-stack-42", deletedStackID) + // Unstack called with old stack number + assert.Equal(t, 7, unstackedNumber) // Push called per-branch (3 separate calls, not 1 atomic call) require.Len(t, pushCalls, 3, "should push each branch individually") @@ -2042,13 +2095,13 @@ func TestSubmit_WithPendingModify_SequentialPush(t *testing.T) { // CreateStack called with all 3 PRs assert.Equal(t, []int{10, 11, 12}, createdStackPRs) - // Verify ordering: delete before push, push before create_stack + // Verify ordering: unstack before push, push before create_stack assert.True(t, len(callOrder) >= 5, "expected at least 5 calls, got %d: %v", len(callOrder), callOrder) deleteIdx := -1 firstPushIdx := -1 createIdx := -1 for i, c := range callOrder { - if c == "delete:old-stack-42" && deleteIdx == -1 { + if c == "unstack:7" && deleteIdx == -1 { deleteIdx = i } if c == "push:b1" && firstPushIdx == -1 { @@ -2109,8 +2162,8 @@ func TestSubmit_FetchesBeforePush(t *testing.T) { ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{}, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -2168,7 +2221,7 @@ func TestSubmit_UsesPRTemplate(t *testing.T) { capturedBody = body return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := SubmitCmd(cfg) @@ -2228,7 +2281,7 @@ func TestSubmit_IgnoresSymlinkedPRTemplate(t *testing.T) { capturedBody = body return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := SubmitCmd(cfg) @@ -2274,7 +2327,7 @@ func TestSubmit_NoTemplate_UsesFooter(t *testing.T) { capturedBody = body return &github.PullRequest{Number: 1, ID: "PR_1", URL: "https://github.com/o/r/pull/1"}, nil }, - CreateStackFn: func([]int) (int, error) { return 1, nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { return &github.RemoteStack{ID: 1, Number: 1}, nil }, } cmd := SubmitCmd(cfg) @@ -2288,91 +2341,6 @@ func TestSubmit_NoTemplate_UsesFooter(t *testing.T) { assert.Contains(t, capturedBody, feedbackURL) } -func TestSubmit_PreflightCheck_PAT_BailsOut(t *testing.T) { - s := stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1"}, - {Branch: "b2"}, - }, - } - - tmpDir := t.TempDir() - writeStackFile(t, tmpDir, s) - - pushed := false - mock := newSubmitMock(tmpDir, "b1") - mock.PushFn = func(string, []string, bool, bool) error { - pushed = true - return nil - } - restore := git.SetOps(mock) - defer restore() - - listStacksCalled := false - cfg, _, errR := config.NewTestConfig() - cfg.GitHubClientOverride = &github.MockClient{ - ListStacksFn: func() ([]github.RemoteStack, error) { - listStacksCalled = true - return nil, nil - }, - } - - // Simulate a classic PAT — the pre-flight check should abort. - setTestTokenForHost(cfg, "ghp_classic_pat_token") - - cmd := SubmitCmd(cfg) - cmd.SetArgs([]string{"--auto"}) - cmd.SetOut(io.Discard) - cmd.SetErr(io.Discard) - err := cmd.Execute() - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.ErrorIs(t, err, ErrStacksUnavailable) - assert.Contains(t, output, "Personal access tokens are not supported by gh stack") - assert.Contains(t, output, "gh auth login") - assert.False(t, pushed, "should not push when using a PAT") - assert.False(t, listStacksCalled, "should not call ListStacks when PAT detected") -} - -func TestSubmit_PreflightCheck_FinegrainedPAT_BailsOut(t *testing.T) { - s := stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1"}, - {Branch: "b2"}, - }, - } - - tmpDir := t.TempDir() - writeStackFile(t, tmpDir, s) - - mock := newSubmitMock(tmpDir, "b1") - restore := git.SetOps(mock) - defer restore() - - cfg, _, errR := config.NewTestConfig() - cfg.GitHubClientOverride = &github.MockClient{} - - setTestTokenForHost(cfg, "github_pat_11AABBCC_xxxx") - - cmd := SubmitCmd(cfg) - cmd.SetArgs([]string{"--auto"}) - cmd.SetOut(io.Discard) - cmd.SetErr(io.Discard) - err := cmd.Execute() - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.ErrorIs(t, err, ErrStacksUnavailable) - assert.Contains(t, output, "Personal access tokens are not supported by gh stack") -} - func TestSubmit_DisablesAutoMergeOnExistingPR(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, @@ -2418,8 +2386,8 @@ func TestSubmit_DisablesAutoMergeOnExistingPR(t *testing.T) { disabledAutoMergePRIDs = append(disabledAutoMergePRIDs, prID) return nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -2470,8 +2438,8 @@ func TestSubmit_DisableAutoMergeFailure_ContinuesWithWarning(t *testing.T) { DisableAutoMergeFn: func(prID string) error { return fmt.Errorf("permission denied") }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } @@ -2522,8 +2490,8 @@ func TestSubmit_NoAutoMerge_SkipsDisable(t *testing.T) { t.Fatal("DisableAutoMerge should not be called when auto-merge is not enabled") return nil }, - CreateStackFn: func(prNumbers []int) (int, error) { - return 42, nil + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 42, Number: 42}, nil }, } diff --git a/cmd/sync_test.go b/cmd/sync_test.go index ca9acd7..4f76d8e 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1757,13 +1757,13 @@ func TestSync_CreatesRemoteStackWhenPRsExist(t *testing.T) { listCalls++ return nil, nil }, - CreateStackFn: func(prNumbers []int) (int, error) { + CreateStackFn: func(prNumbers []int) (*github.RemoteStack, error) { createdWith = prNumbers - return 7, nil + return &github.RemoteStack{ID: 7, Number: 7}, nil }, - UpdateStackFn: func(string, []int) error { - t.Fatal("UpdateStack should not be called when no remote stack exists") - return nil + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack should not be called when no remote stack exists") + return nil, nil }, } @@ -1796,15 +1796,15 @@ func TestSync_AdoptsExistingEqualRemoteStack(t *testing.T) { ghMock := &github.MockClient{ FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102}}}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { t.Fatal("CreateStack should not be called when the remote stack matches") - return 0, nil + return nil, nil }, - UpdateStackFn: func(string, []int) error { - t.Fatal("UpdateStack should not be called when the remote stack matches") - return nil + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack should not be called when the remote stack matches") + return nil, nil }, } @@ -1829,28 +1829,32 @@ func TestSync_UpdatesPartialRemoteStack(t *testing.T) { tmpDir := t.TempDir() writeStackFile(t, tmpDir, s) - var updatedID string - var updatedWith []int + var updatedNumber int + var addedWith []int ghMock := &github.MockClient{ FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102, "b3": 103}), ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102}}}, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { t.Fatal("CreateStack should not be called when a matching stack exists") - return 0, nil + return nil, nil }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - updatedID = stackID - updatedWith = prNumbers - return nil + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + assert.Equal(t, 9, stackNumber) + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102}}, nil + }, + AddToStackFn: func(stackNumber int, prNumbers []int) (*github.RemoteStack, error) { + updatedNumber = stackNumber + addedWith = prNumbers + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}, nil }, } output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) - assert.Equal(t, "9", updatedID) - assert.Equal(t, []int{101, 102, 103}, updatedWith) + assert.Equal(t, 9, updatedNumber) + assert.Equal(t, []int{103}, addedWith) assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") assert.Contains(t, output, "Stack synced") assert.NotContains(t, output, "Branches synced") @@ -1878,9 +1882,9 @@ func TestSync_FewerThanTwoPRs_BranchesSynced(t *testing.T) { listCalled = true return nil, nil }, - CreateStackFn: func([]int) (int, error) { + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true - return 0, nil + return &github.RemoteStack{}, nil }, } @@ -1905,8 +1909,8 @@ func TestSync_StacksUnavailable_BranchesSynced(t *testing.T) { ghMock := &github.MockClient{ FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{StatusCode: 404, Message: "Not Found"} + CreateStackFn: func([]int) (*github.RemoteStack, error) { + return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"} }, } @@ -1932,18 +1936,18 @@ func TestSync_PRsSpanMultipleStacks_BranchesSynced(t *testing.T) { FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), ListStacksFn: func() ([]github.RemoteStack, error) { return []github.RemoteStack{ - {ID: 9, PullRequests: []int{101}}, - {ID: 10, PullRequests: []int{102}}, + {ID: 9, Number: 9, PullRequests: []int{101}}, + {ID: 10, Number: 10, PullRequests: []int{102}}, }, nil }, - CreateStackFn: func([]int) (int, error) { createCalled = true; return 0, nil }, - UpdateStackFn: func(string, []int) error { updateCalled = true; return nil }, + CreateStackFn: func([]int) (*github.RemoteStack, error) { createCalled = true; return nil, nil }, + AddToStackFn: func(int, []int) (*github.RemoteStack, error) { updateCalled = true; return nil, nil }, } output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) assert.False(t, createCalled, "CreateStack should not be called on divergence") - assert.False(t, updateCalled, "UpdateStack should not be called on divergence") + assert.False(t, updateCalled, "AddToStack should not be called on divergence") assert.Contains(t, output, "multiple stacks") assert.NotContains(t, output, "submitting", "divergence guidance should be command-neutral, not submit-specific") assert.Contains(t, output, "Branches synced") @@ -2041,7 +2045,11 @@ func TestSync_RemoteAhead_PullsNewBranches(t *testing.T) { ghMock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 103, 104, 105}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 103, 104, 105}}}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + assert.Equal(t, 9, stackNumber) + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103, 104, 105}}, nil }, FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2", 103: "b3", 104: "b4", 105: "b5"}), } @@ -2089,7 +2097,11 @@ func TestSync_RemoteAhead_QueuedBranchNotPushed(t *testing.T) { ghMock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 103}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + assert.Equal(t, 9, stackNumber) + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}, nil }, FindPRByNumberFn: func(n int) (*github.PullRequest, error) { branch := map[int]string{101: "b1", 102: "b2", 103: "b3"}[n] @@ -2143,7 +2155,7 @@ func TestSync_RemoteAhead_DuplicateBranchAborts(t *testing.T) { ghMock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 103}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}}, nil }, FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2", 103: "b3"}), } @@ -2207,7 +2219,11 @@ func TestSync_RemoteInSync_NoPull(t *testing.T) { ghMock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102}}}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + assert.Equal(t, 9, stackNumber) + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102}}, nil }, FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2"}), } @@ -2240,7 +2256,10 @@ func divergentStack(t *testing.T, tmpDir string) { func divergentRemoteMock() *github.MockClient { return &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 104}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 104}}}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 104}}, nil }, FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2", 103: "b3", 104: "b4"}), } @@ -2260,9 +2279,18 @@ func TestSync_Divergent_NonInteractive_Aborts(t *testing.T) { mock := newSyncMockNoRebase(tmpDir, "b1") mock.CreateBranchFn = func(name, base string) error { created = append(created, name); return nil } mock.PushFn = func(string, []string, bool, bool) error { pushed = true; return nil } - ghMock.CreateStackFn = func([]int) (int, error) { t.Fatal("CreateStack must not be called"); return 0, nil } - ghMock.UpdateStackFn = func(string, []int) error { t.Fatal("UpdateStack must not be called"); return nil } - ghMock.DeleteStackFn = func(string) error { t.Fatal("DeleteStack must not be called"); return nil } + ghMock.CreateStackFn = func([]int) (*github.RemoteStack, error) { + t.Fatal("CreateStack must not be called") + return nil, nil + } + ghMock.AddToStackFn = func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack must not be called") + return nil, nil + } + ghMock.UnstackFn = func(int) (*github.RemoteStack, bool, error) { + t.Fatal("Unstack must not be called") + return nil, false, nil + } output, err := runSyncCfg(t, mock, func(cfg *config.Config) { cfg.GitHubClientOverride = ghMock }) require.NoError(t, err) @@ -2407,12 +2435,22 @@ func TestSync_Divergent_DeleteRemote(t *testing.T) { divergentStack(t, tmpDir) deleted := false - var deletedID string + var deletedNumber int var pushed bool ghMock := divergentRemoteMock() - ghMock.DeleteStackFn = func(id string) error { deleted = true; deletedID = id; return nil } - ghMock.CreateStackFn = func([]int) (int, error) { t.Fatal("CreateStack must not be called"); return 0, nil } - ghMock.UpdateStackFn = func(string, []int) error { t.Fatal("UpdateStack must not be called"); return nil } + ghMock.UnstackFn = func(number int) (*github.RemoteStack, bool, error) { + deleted = true + deletedNumber = number + return nil, true, nil + } + ghMock.CreateStackFn = func([]int) (*github.RemoteStack, error) { + t.Fatal("CreateStack must not be called") + return nil, nil + } + ghMock.AddToStackFn = func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack must not be called") + return nil, nil + } mock := newSyncMockNoRebase(tmpDir, "b1") mock.PushFn = func(string, []string, bool, bool) error { pushed = true; return nil } @@ -2424,7 +2462,7 @@ func TestSync_Divergent_DeleteRemote(t *testing.T) { require.NoError(t, err) assert.True(t, deleted, "remote stack should be deleted") - assert.Equal(t, "9", deletedID) + assert.Equal(t, 9, deletedNumber) assert.False(t, pushed, "sync should stop after deleting the remote stack") assert.Contains(t, output, "Deleted the stack on GitHub") assert.Contains(t, output, "gh stack submit") @@ -2443,9 +2481,18 @@ func TestSync_Divergent_Cancel(t *testing.T) { divergentStack(t, tmpDir) ghMock := divergentRemoteMock() - ghMock.DeleteStackFn = func(string) error { t.Fatal("DeleteStack must not be called"); return nil } - ghMock.CreateStackFn = func([]int) (int, error) { t.Fatal("CreateStack must not be called"); return 0, nil } - ghMock.UpdateStackFn = func(string, []int) error { t.Fatal("UpdateStack must not be called"); return nil } + ghMock.UnstackFn = func(int) (*github.RemoteStack, bool, error) { + t.Fatal("Unstack must not be called") + return nil, false, nil + } + ghMock.CreateStackFn = func([]int) (*github.RemoteStack, error) { + t.Fatal("CreateStack must not be called") + return nil, nil + } + ghMock.AddToStackFn = func(int, []int) (*github.RemoteStack, error) { + t.Fatal("AddToStack must not be called") + return nil, nil + } var pushed bool mock := newSyncMockNoRebase(tmpDir, "b1") mock.PushFn = func(string, []string, bool, bool) error { pushed = true; return nil } @@ -2490,7 +2537,11 @@ func TestSync_MergedBranchPruned_NoFalseDivergence(t *testing.T) { ghMock := &github.MockClient{ ListStacksFn: func() ([]github.RemoteStack, error) { - return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 103}}}, nil + return []github.RemoteStack{{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}}, nil + }, + GetStackFn: func(stackNumber int) (*github.RemoteStack, error) { + assert.Equal(t, 9, stackNumber) + return &github.RemoteStack{ID: 9, Number: 9, PullRequests: []int{101, 102, 103}}, nil }, FindPRByNumberFn: func(n int) (*github.PullRequest, error) { branch := map[int]string{101: "b1", 102: "b2", 103: "b3"}[n] diff --git a/cmd/unstack.go b/cmd/unstack.go index 2292df6..3354217 100644 --- a/cmd/unstack.go +++ b/cmd/unstack.go @@ -2,11 +2,9 @@ package cmd import ( "errors" - "fmt" "github.com/cli/go-gh/v2/pkg/api" "github.com/github/gh-stack/internal/config" - "github.com/github/gh-stack/internal/github" "github.com/github/gh-stack/internal/modify" "github.com/github/gh-stack/internal/stack" "github.com/spf13/cobra" @@ -55,11 +53,13 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { sf := result.StackFile s := result.Stack - // Delete the stack on GitHub first (unless --local). - // Only proceed with local deletion after the remote operation succeeds. + // Unstack on GitHub first (unless --local). The server decides which PRs + // can be unstacked; PRs that are queued for merge or have auto-merge enabled + // are left in place and the stack is kept. Local tracking is only removed + // when the remote stack is fully dissolved. if !opts.local { - if s.ID == "" { - cfg.Warningf("Stack has no remote ID — skipping server-side deletion") + if s.ID == "" && s.Number == 0 { + cfg.Warningf("Stack has no remote ID — skipping server-side unstack") } else { client, err := cfg.GitHubClient() if err != nil { @@ -67,36 +67,43 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { return ErrAPIFailure } - blocked, err := shouldBlockUnstackDelete(client, s) + number, err := ensureStackNumber(client, s) if err != nil { - cfg.Errorf("failed to check pull request states before unstack: %s", err) + cfg.Errorf("failed to look up stack on GitHub: %s", err) return ErrAPIFailure } - if blocked { - cfg.Errorf("Unstacking not allowed. Pull requests that are queued for merge, are merging, or are already merged will remain in the stack.") - return ErrInvalidArgs - } - if err := client.DeleteStack(s.ID); err != nil { + if number == 0 { + cfg.Warningf("Stack not found on GitHub — continuing with local unstack") + } else if _, dissolved, err := client.Unstack(number); err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) { switch httpErr.StatusCode { case 404: - // Stack already deleted on GitHub — treat as success. + // Stack already gone on GitHub — treat as success. cfg.Warningf("Stack not found on GitHub — continuing with local unstack") case 422: - cfg.Errorf("Cannot delete stack on GitHub: %s", httpErr.Message) - return ErrAPIFailure + // The server refused: every PR is queued for merge or has + // auto-merge enabled, so nothing can be unstacked. + cfg.Errorf("Unstacking not allowed: %s", httpErr.Message) + return ErrInvalidArgs default: - cfg.Errorf("Failed to delete stack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message) + cfg.Errorf("Failed to unstack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message) return ErrAPIFailure } } else { - cfg.Errorf("Failed to delete stack on GitHub: %v", err) + cfg.Errorf("Failed to unstack on GitHub: %v", err) return ErrAPIFailure } + } else if !dissolved { + // Some PRs (queued for merge or with auto-merge enabled) remain + // stacked on GitHub, so the stack still exists. Keep local + // tracking so it continues to reflect the remote stack. + cfg.Warningf("Some pull requests are queued for merge or have auto-merge enabled and remain stacked on GitHub") + cfg.Printf("The stack was left in place — local tracking is unchanged") + return nil } else { - cfg.Successf("Stack deleted on GitHub") + cfg.Successf("Stack removed on GitHub") } } } @@ -117,55 +124,3 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { return nil } - -func shouldBlockUnstackDelete(client github.ClientOps, s *stack.Stack) (bool, error) { - if s == nil || len(s.Branches) == 0 { - return false, nil - } - - eligible := 0 - ineligible := 0 - for _, b := range s.Branches { - // Respect stored merged status when available in local stack metadata. - if b.PullRequest != nil && b.PullRequest.Merged { - ineligible++ - continue - } - - var ( - pr *github.PullRequest - err error - ) - - if b.PullRequest != nil && b.PullRequest.Number > 0 { - pr, err = client.FindPRByNumber(b.PullRequest.Number) - if err != nil { - return false, fmt.Errorf("checking PR #%d for branch %s: %w", b.PullRequest.Number, b.Branch, err) - } - } else { - pr, err = client.FindPRForBranch(b.Branch) - if err != nil { - return false, fmt.Errorf("checking PR for branch %s: %w", b.Branch, err) - } - } - - // If the PR no longer exists (or branch has no open PR), do not block unstacking. - if pr == nil { - eligible++ - continue - } - - switch { - case pr.State == "MERGED": - ineligible++ - case pr.IsQueued(): - ineligible++ - case pr.IsAutoMergeEnabled(): - ineligible++ - default: - eligible++ - } - } - - return ineligible > 0 && eligible == 0, nil -} diff --git a/cmd/unstack_test.go b/cmd/unstack_test.go index 31f6303..74e9128 100644 --- a/cmd/unstack_test.go +++ b/cmd/unstack_test.go @@ -37,6 +37,7 @@ func TestUnstack_RemovesStack(t *testing.T) { s1 := stack.Stack{ ID: "42", + Number: 42, Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, } @@ -46,12 +47,12 @@ func TestUnstack_RemovesStack(t *testing.T) { } writeTwoStacks(t, gitDir, s1, s2) - var deletedStackID string + var unstackedNumber int cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - DeleteStackFn: func(stackID string) error { - deletedStackID = stackID - return nil + UnstackFn: func(n int) (*github.RemoteStack, bool, error) { + unstackedNumber = n + return nil, true, nil // dissolved }, } err := runUnstack(cfg, &unstackOptions{}) @@ -59,8 +60,8 @@ func TestUnstack_RemovesStack(t *testing.T) { require.NoError(t, err) assert.Contains(t, output, "Stack removed from local tracking") - assert.Contains(t, output, "Stack deleted on GitHub") - assert.Equal(t, "42", deletedStackID) + assert.Contains(t, output, "Stack removed on GitHub") + assert.Equal(t, 42, unstackedNumber) sf, err := stack.Load(gitDir) require.NoError(t, err) @@ -88,7 +89,7 @@ func TestUnstack_Local(t *testing.T) { require.NoError(t, err) assert.Contains(t, output, "Stack removed") // With --local, the GitHub API should NOT be called. - assert.NotContains(t, output, "Stack deleted on GitHub") + assert.NotContains(t, output, "Stack removed on GitHub") sf, err := stack.Load(gitDir) require.NoError(t, err) @@ -103,7 +104,7 @@ func TestUnstack_NoStackID_WarnsAndSkipsAPI(t *testing.T) { }) defer restore() - // Stack with no ID (never synced to GitHub) + // Stack with no ID/Number (never synced to GitHub) writeStackFile(t, gitDir, stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, @@ -112,9 +113,9 @@ func TestUnstack_NoStackID_WarnsAndSkipsAPI(t *testing.T) { apiCalled := false cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - DeleteStackFn: func(stackID string) error { + UnstackFn: func(int) (*github.RemoteStack, bool, error) { apiCalled = true - return nil + return nil, true, nil }, } err := runUnstack(cfg, &unstackOptions{}) @@ -124,7 +125,49 @@ func TestUnstack_NoStackID_WarnsAndSkipsAPI(t *testing.T) { assert.False(t, apiCalled, "API should not be called when stack has no ID") assert.Contains(t, output, "no remote ID") assert.Contains(t, output, "Stack removed from local tracking") - assert.NotContains(t, output, "Stack deleted on GitHub") + assert.NotContains(t, output, "Stack removed on GitHub") +} + +func TestUnstack_ResolvesNumberFromID(t *testing.T) { + // A local stack that predates the Number field (only ID stored) resolves + // its stack number from the remote list before unstacking. + gitDir := t.TempDir() + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return gitDir, nil }, + CurrentBranchFn: func() (string, error) { return "b1", nil }, + }) + defer restore() + + writeStackFile(t, gitDir, stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, + }, + }) + + var unstackedNumber int + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 99, Number: 7, PullRequests: []int{101, 102}}}, nil + }, + UnstackFn: func(n int) (*github.RemoteStack, bool, error) { + unstackedNumber = n + return nil, true, nil + }, + } + err := runUnstack(cfg, &unstackOptions{}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, 7, unstackedNumber, "should resolve the stack number from the internal ID") + assert.Contains(t, output, "Stack removed from local tracking") + + sf, err := stack.Load(gitDir) + require.NoError(t, err) + assert.Empty(t, sf.Stacks) } func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) { @@ -136,8 +179,9 @@ func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) { defer restore() writeStackFile(t, gitDir, stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, @@ -146,14 +190,14 @@ func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - DeleteStackFn: func(stackID string) error { - return &api.HTTPError{StatusCode: 404, Message: "Not Found"} + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + return nil, false, &api.HTTPError{StatusCode: 404, Message: "Not Found"} }, } err := runUnstack(cfg, &unstackOptions{}) output := collectOutput(cfg, outR, errR) - // 404 means already deleted — should succeed and remove locally + // 404 means already gone — should succeed and remove locally require.NoError(t, err) assert.Contains(t, output, "continuing with local unstack") assert.Contains(t, output, "Stack removed from local tracking") @@ -163,7 +207,7 @@ func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) { assert.Empty(t, sf.Stacks) } -func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) { +func TestUnstack_ServerError_StopsLocalDeletion(t *testing.T) { gitDir := t.TempDir() restore := git.SetOps(&git.MockOps{ GitDirFn: func() (string, error) { return gitDir, nil }, @@ -172,8 +216,9 @@ func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) { defer restore() writeStackFile(t, gitDir, stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, @@ -182,15 +227,15 @@ func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - DeleteStackFn: func(stackID string) error { - return &api.HTTPError{StatusCode: 409, Message: "Stack is currently being modified"} + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + return nil, false, &api.HTTPError{StatusCode: 409, Message: "Stack is currently being modified"} }, } err := runUnstack(cfg, &unstackOptions{}) output := collectOutput(cfg, outR, errR) assert.ErrorIs(t, err, ErrAPIFailure) - assert.Contains(t, output, "Failed to delete stack on GitHub (HTTP 409)") + assert.Contains(t, output, "Failed to unstack on GitHub (HTTP 409)") // Should NOT remove locally when remote fails assert.NotContains(t, output, "Stack removed from local tracking") @@ -234,7 +279,10 @@ func TestUnstack_RemovesCorrectStackByPointer(t *testing.T) { assert.Equal(t, []string{"b1", "b2"}, sf.Stacks[0].BranchNames(), "should keep the OTHER stack intact") } -func TestUnstack_PreflightBlocksDelete_WhenAllPRsIneligible(t *testing.T) { +func TestUnstack_AllLocked_ServerRejects(t *testing.T) { + // Every PR is queued for merge or has auto-merge enabled. The server + // (not the client) rejects the unstack with a 422; the command surfaces the + // error and leaves local tracking in place. gitDir := t.TempDir() restore := git.SetOps(&git.MockOps{ GitDirFn: func() (string, error) { return gitDir, nil }, @@ -243,30 +291,21 @@ func TestUnstack_PreflightBlocksDelete_WhenAllPRsIneligible(t *testing.T) { defer restore() writeStackFile(t, gitDir, stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}}, + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, }, }) - deleteCalled := false + unstackCalled := false cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - FindPRByNumberFn: func(number int) (*github.PullRequest, error) { - switch number { - case 101: - return &github.PullRequest{Number: 101, State: "MERGED"}, nil - case 102: - return &github.PullRequest{Number: 102, State: "OPEN", MergeQueueEntry: &github.MergeQueueEntry{ID: "MQE_1"}}, nil - default: - return nil, nil - } - }, - DeleteStackFn: func(stackID string) error { - deleteCalled = true - return nil + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + unstackCalled = true + return nil, false, &api.HTTPError{StatusCode: 422, Message: "all pull requests are queued for merge or have auto-merge enabled"} }, } @@ -274,7 +313,7 @@ func TestUnstack_PreflightBlocksDelete_WhenAllPRsIneligible(t *testing.T) { output := collectOutput(cfg, outR, errR) assert.ErrorIs(t, err, ErrInvalidArgs) - assert.False(t, deleteCalled, "DeleteStack should not be called when all PRs are ineligible") + assert.True(t, unstackCalled, "the server decides eligibility, so Unstack is called") assert.Contains(t, output, "Unstacking not allowed") assert.NotContains(t, output, "Stack removed from local tracking") @@ -283,7 +322,9 @@ func TestUnstack_PreflightBlocksDelete_WhenAllPRsIneligible(t *testing.T) { require.Len(t, sf.Stacks, 1) } -func TestUnstack_PreflightAllowsDelete_WhenMixedEligibility(t *testing.T) { +func TestUnstack_PartialUnstack_KeepsLocalTracking(t *testing.T) { + // Some PRs (queued for merge / auto-merge) remain stacked, so the server + // returns the surviving stack (dissolved=false). Local tracking is kept. gitDir := t.TempDir() restore := git.SetOps(&git.MockOps{ GitDirFn: func() (string, error) { return gitDir, nil }, @@ -292,30 +333,19 @@ func TestUnstack_PreflightAllowsDelete_WhenMixedEligibility(t *testing.T) { defer restore() writeStackFile(t, gitDir, stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, + ID: "99", + Number: 99, + Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}, {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, }, }) - deleteCalled := false cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - FindPRByNumberFn: func(number int) (*github.PullRequest, error) { - switch number { - case 101: - return &github.PullRequest{Number: 101, State: "MERGED"}, nil - case 102: - return &github.PullRequest{Number: 102, State: "OPEN"}, nil - default: - return nil, nil - } - }, - DeleteStackFn: func(stackID string) error { - deleteCalled = true - return nil + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{102}}, false, nil }, } @@ -323,55 +353,19 @@ func TestUnstack_PreflightAllowsDelete_WhenMixedEligibility(t *testing.T) { output := collectOutput(cfg, outR, errR) require.NoError(t, err) - assert.True(t, deleteCalled, "DeleteStack should be called when at least one PR is eligible") - assert.Contains(t, output, "Stack deleted on GitHub") - assert.Contains(t, output, "Stack removed from local tracking") - - sf, loadErr := stack.Load(gitDir) - require.NoError(t, loadErr) - assert.Empty(t, sf.Stacks) -} - -func TestUnstack_PreflightLookupFailure_StopsDeletion(t *testing.T) { - gitDir := t.TempDir() - restore := git.SetOps(&git.MockOps{ - GitDirFn: func() (string, error) { return gitDir, nil }, - CurrentBranchFn: func() (string, error) { return "b1", nil }, - }) - defer restore() - - writeStackFile(t, gitDir, stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}}, - }) - - deleteCalled := false - cfg, outR, errR := config.NewTestConfig() - cfg.GitHubClientOverride = &github.MockClient{ - FindPRByNumberFn: func(number int) (*github.PullRequest, error) { - return nil, errors.New("graphql timeout") - }, - DeleteStackFn: func(stackID string) error { - deleteCalled = true - return nil - }, - } - - err := runUnstack(cfg, &unstackOptions{}) - output := collectOutput(cfg, outR, errR) - - assert.ErrorIs(t, err, ErrAPIFailure) - assert.False(t, deleteCalled, "DeleteStack should not be called if preflight fails") - assert.Contains(t, output, "failed to check pull request states before unstack") + assert.Contains(t, output, "remain stacked on GitHub") + assert.Contains(t, output, "local tracking is unchanged") assert.NotContains(t, output, "Stack removed from local tracking") + // The stack still exists remotely, so local tracking is preserved. sf, loadErr := stack.Load(gitDir) require.NoError(t, loadErr) require.Len(t, sf.Stacks, 1) } -func TestUnstack_API422_ShowsInformativeErrorAndStopsLocalDeletion(t *testing.T) { +func TestUnstack_NumberLookupFailure_StopsDeletion(t *testing.T) { + // Resolving the stack number from its ID fails (list API error), so the + // command aborts without touching local tracking. gitDir := t.TempDir() restore := git.SetOps(&git.MockOps{ GitDirFn: func() (string, error) { return gitDir, nil }, @@ -382,27 +376,27 @@ func TestUnstack_API422_ShowsInformativeErrorAndStopsLocalDeletion(t *testing.T) writeStackFile(t, gitDir, stack.Stack{ ID: "99", Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, - }, + Branches: []stack.BranchRef{{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}}}, }) + unstackCalled := false cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ - FindPRByNumberFn: func(number int) (*github.PullRequest, error) { - return &github.PullRequest{Number: number, State: "OPEN"}, nil + ListStacksFn: func() ([]github.RemoteStack, error) { + return nil, errors.New("network error") }, - DeleteStackFn: func(stackID string) error { - return &api.HTTPError{StatusCode: 422, Message: "some pull requests cannot be removed from stack"} + UnstackFn: func(int) (*github.RemoteStack, bool, error) { + unstackCalled = true + return nil, true, nil }, } + err := runUnstack(cfg, &unstackOptions{}) output := collectOutput(cfg, outR, errR) assert.ErrorIs(t, err, ErrAPIFailure) - assert.Contains(t, output, "Cannot delete stack on GitHub") - assert.Contains(t, output, "cannot be removed") + assert.False(t, unstackCalled, "Unstack should not be called if number lookup fails") + assert.Contains(t, output, "failed to look up stack on GitHub") assert.NotContains(t, output, "Stack removed from local tracking") sf, loadErr := stack.Load(gitDir) diff --git a/cmd/utils.go b/cmd/utils.go index 61bd91e..16d524d 100644 --- a/cmd/utils.go +++ b/cmd/utils.go @@ -72,16 +72,49 @@ func printInterrupt(cfg *config.Config) { cfg.Infof("Received interrupt, aborting operation") } -// warnStacksUnavailableOrPAT prints an appropriate warning when a stacks API -// call returns 404. If the token is a PAT the message focuses on the auth -// issue; otherwise it falls back to the generic "not enabled" message. -func warnStacksUnavailableOrPAT(cfg *config.Config) { - if cfg.WarnIfPAT() { - return - } +// warnStacksUnavailable prints a warning when a stacks API call returns 404, +// indicating stacked PRs are not enabled for the repository. +func warnStacksUnavailable(cfg *config.Config) { cfg.Warningf("Stacked PRs are not enabled for this repository") } +// stackNumberByID resolves an internal stack ID (as stored in the local stack +// file) to its human-facing stack number by consulting the remote stack list. +// Returns ok=false when no remote stack matches the ID (e.g. it was deleted). +func stackNumberByID(client github.ClientOps, id string) (number int, ok bool, err error) { + if id == "" { + return 0, false, nil + } + stacks, err := client.ListStacks() + if err != nil { + return 0, false, err + } + for _, rs := range stacks { + if strconv.Itoa(rs.ID) == id { + return rs.Number, true, nil + } + } + return 0, false, nil +} + +// ensureStackNumber returns the stack number for s, resolving and caching it +// from the remote stack list by internal ID when the local model predates the +// Number field (older stack files stored only the ID). Returns 0 when the stack +// number can't be determined. +func ensureStackNumber(client github.ClientOps, s *stack.Stack) (int, error) { + if s.Number != 0 { + return s.Number, nil + } + number, found, err := stackNumberByID(client, s.ID) + if err != nil { + return 0, err + } + if found { + s.Number = number + } + return number, nil +} + // inputWithPrefill prompts the user for text input with the given prefill // already editable in the input field. Unlike survey.Input's Default (which // shows in parentheses), this places the prefill text directly in the @@ -531,7 +564,7 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string var remotePRNumbers []int for _, rs := range stacks { if strconv.Itoa(rs.ID) == s.ID { - remotePRNumbers = rs.PullRequests + remotePRNumbers = rs.PRNumbers() break } } @@ -1248,7 +1281,7 @@ func reconcileRemoteStack(cfg *config.Config, sf *stack.StackFile, s *stack.Stac found := false for _, rs := range stacks { if strconv.Itoa(rs.ID) == s.ID { - remotePRNumbers = rs.PullRequests + remotePRNumbers = rs.PRNumbers() found = true break } @@ -1506,6 +1539,7 @@ func resolveDivergenceUseRemote(cfg *config.Config, sf *stack.StackFile, s *stac trunk := s.Trunk.Branch remoteStackID := s.ID + remoteStackNumber := s.Number oldBranches := s.BranchNames() removeLocalStack(sf, s) @@ -1520,7 +1554,7 @@ func resolveDivergenceUseRemote(cfg *config.Config, sf *stack.StackFile, s *stac } } - newStack, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID) + newStack, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID, remoteStackNumber) if err != nil { return res, err } @@ -1584,19 +1618,25 @@ func nearestBranchAfterReplace(oldBranches []string, currentBranch string, newSt func resolveDivergenceDeleteRemote(cfg *config.Config, client github.ClientOps, sf *stack.StackFile, s *stack.Stack, gitDir string) (remoteReconcileResult, error) { res := remoteReconcileResult{stop: true} - if err := client.DeleteStack(s.ID); err != nil { + number, err := ensureStackNumber(client, s) + if err != nil || number == 0 { + cfg.Warningf("Remote stack already deleted") + } else if _, dissolved, unstackErr := client.Unstack(number); unstackErr != nil { var httpErr *api.HTTPError - if errors.As(err, &httpErr) && httpErr.StatusCode == 404 { + if errors.As(unstackErr, &httpErr) && httpErr.StatusCode == 404 { cfg.Warningf("Remote stack already deleted") } else { - cfg.Errorf("failed to delete remote stack: %v", err) + cfg.Errorf("failed to delete remote stack: %v", unstackErr) return res, ErrAPIFailure } - } else { + } else if dissolved { cfg.Successf("Deleted the stack on GitHub") + } else { + cfg.Warningf("Some pull requests could not be unstacked and remain on GitHub") } s.ID = "" + s.Number = 0 if err := stack.Save(gitDir, sf); err != nil { return res, handleSaveError(cfg, err) } diff --git a/cmd/utils_test.go b/cmd/utils_test.go index 75f38c4..d2b510f 100644 --- a/cmd/utils_test.go +++ b/cmd/utils_test.go @@ -691,39 +691,21 @@ func TestStackNeedsRebase_SkipsMergedBranches(t *testing.T) { assert.False(t, stackNeedsRebase(s), "should skip merged branches and find stack up to date") } -// setTestTokenForHost sets cfg.TokenForHostFn to return the given token for -// any host. Also sets RepoOverride so tests don't depend on real git context. -func setTestTokenForHost(cfg *config.Config, token string) { - cfg.TokenForHostFn = func(string) (string, string) { return token, "test" } +// setTestRepo sets RepoOverride so tests don't depend on real git context. +func setTestRepo(cfg *config.Config) { cfg.RepoOverride = &repository.Repository{Host: "github.com", Owner: "o", Name: "r"} } -func TestWarnStacksUnavailableOrPAT_ShowsPATMessage(t *testing.T) { +func TestWarnStacksUnavailable_ShowsNotEnabled(t *testing.T) { cfg, _, errR := config.NewTestConfig() - setTestTokenForHost(cfg, "github_pat_fine_grained") - warnStacksUnavailableOrPAT(cfg) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Contains(t, output, "Personal access tokens are not supported by gh stack") - assert.NotContains(t, output, "Stacked PRs are not enabled") -} - -func TestWarnStacksUnavailableOrPAT_ShowsNotEnabledForOAuth(t *testing.T) { - cfg, _, errR := config.NewTestConfig() - setTestTokenForHost(cfg, "gho_oauth_token") - - warnStacksUnavailableOrPAT(cfg) + warnStacksUnavailable(cfg) cfg.Err.Close() errOut, _ := io.ReadAll(errR) output := string(errOut) assert.Contains(t, output, "Stacked PRs are not enabled for this repository") - assert.NotContains(t, output, "Personal access tokens") } func TestEnsureLocalTrunk_AlreadyExists(t *testing.T) { diff --git a/internal/config/auth.go b/internal/config/auth.go deleted file mode 100644 index 8fcc38b..0000000 --- a/internal/config/auth.go +++ /dev/null @@ -1,65 +0,0 @@ -package config - -import ( - "strings" - - "github.com/cli/go-gh/v2/pkg/auth" -) - -// tokenForHost returns the auth token for the given host, using the -// test override if set or falling back to the real auth.TokenForHost. -func (cfg *Config) tokenForHost(host string) (string, string) { - if cfg.TokenForHostFn != nil { - return cfg.TokenForHostFn(host) - } - return auth.TokenForHost(host) -} - -// IsPersonalAccessToken reports whether the active token for the current -// repository's host is a personal access token (classic or fine-grained) -// rather than an OAuth token from `gh auth login`. -// -// Token prefix conventions: -// -// gho_ → OAuth token (supported) -// ghs_ → GitHub App installation token (supported) -// ghp_ → Classic personal access token (NOT supported) -// github_pat_ → Fine-grained personal access token (NOT supported) -func (cfg *Config) IsPersonalAccessToken() bool { - host := cfg.RepoHost() - if host == "" { - return false - } - return cfg.isPersonalAccessTokenForHost(host) -} - -// isPersonalAccessTokenForHost checks the token prefix for the given host. -func (cfg *Config) isPersonalAccessTokenForHost(host string) bool { - token, _ := cfg.tokenForHost(host) - if token == "" { - return false - } - return strings.HasPrefix(token, "ghp_") || strings.HasPrefix(token, "github_pat_") -} - -// RepoHost returns the GitHub host for the current repository, or an empty -// string if it cannot be determined (e.g. not inside a git repo). -func (cfg *Config) RepoHost() string { - repo, err := cfg.Repo() - if err != nil { - return "" - } - return repo.Host -} - -// WarnIfPAT checks whether the active token is a personal access token and, -// if so, prints a warning explaining that PATs are not supported by gh stack. -// Returns true when a PAT is detected. -func (cfg *Config) WarnIfPAT() bool { - if !cfg.IsPersonalAccessToken() { - return false - } - cfg.Warningf("Personal access tokens are not supported by gh stack during private preview") - cfg.Printf(" Run %s to authenticate with OAuth instead.", cfg.ColorCyan("gh auth login")) - return true -} diff --git a/internal/config/auth_test.go b/internal/config/auth_test.go deleted file mode 100644 index 797e2f5..0000000 --- a/internal/config/auth_test.go +++ /dev/null @@ -1,69 +0,0 @@ -package config - -import ( - "io" - "testing" - - "github.com/cli/go-gh/v2/pkg/repository" - "github.com/stretchr/testify/assert" -) - -// testRepo is a fake repository used in tests to avoid depending on the -// real git repo context (which may not exist in CI). -var testRepo = &repository.Repository{Host: "github.com", Owner: "o", Name: "r"} - -func TestIsPersonalAccessToken(t *testing.T) { - tests := []struct { - name string - token string - want bool - }{ - {"oauth token", "gho_abc123", false}, - {"app installation token", "ghs_abc123", false}, - {"classic PAT", "ghp_abc123", true}, - {"fine-grained PAT", "github_pat_abc123", true}, - {"empty token", "", false}, - {"unknown prefix", "some_other_token", false}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - cfg := &Config{ - TokenForHostFn: func(string) (string, string) { return tt.token, "test" }, - } - got := cfg.isPersonalAccessTokenForHost("github.com") - assert.Equal(t, tt.want, got) - }) - } -} - -func TestWarnIfPAT_DetectsPAT(t *testing.T) { - cfg, _, errR := NewTestConfig() - cfg.RepoOverride = testRepo - cfg.TokenForHostFn = func(string) (string, string) { return "ghp_classic_pat_token", "test" } - - result := cfg.WarnIfPAT() - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.True(t, result) - assert.Contains(t, output, "Personal access tokens are not supported by gh stack") - assert.Contains(t, output, "gh auth login") -} - -func TestWarnIfPAT_IgnoresOAuth(t *testing.T) { - cfg, _, errR := NewTestConfig() - cfg.RepoOverride = testRepo - cfg.TokenForHostFn = func(string) (string, string) { return "gho_oauth_token", "test" } - - result := cfg.WarnIfPAT() - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.False(t, result) - assert.Empty(t, output) -} diff --git a/internal/config/config.go b/internal/config/config.go index 02dbff3..3fb57e8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -47,11 +47,6 @@ type Config struct { // terminal. Used in tests to simulate text input prompts. InputFn func(prompt, defaultValue string) (string, error) - // TokenForHostFn, when non-nil, is called instead of auth.TokenForHost - // to retrieve the auth token for a given GitHub host. Used in tests to - // simulate different token types (OAuth vs PAT). - TokenForHostFn func(host string) (string, string) - // RepoOverride, when non-nil, is returned by Repo() instead of // calling repository.Current(). Used in tests to avoid depending on // the real git repo context. diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index 7ff9bdb..7e61381 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -12,9 +12,11 @@ type ClientOps interface { MarkPRReadyForReview(prID string) error DisableAutoMerge(prID string) error ListStacks() ([]RemoteStack, error) - CreateStack(prNumbers []int) (int, error) - UpdateStack(stackID string, prNumbers []int) error - DeleteStack(stackID string) error + FindStackForPR(prNumber int) (*RemoteStack, error) + GetStack(stackNumber int) (*RemoteStack, error) + CreateStack(prNumbers []int) (*RemoteStack, error) + AddToStack(stackNumber int, prNumbers []int) (*RemoteStack, error) + Unstack(stackNumber int) (*RemoteStack, bool, error) } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/github.go b/internal/github/github.go index 78d3370..2bd577c 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -5,6 +5,7 @@ import ( "encoding/json" "fmt" "math" + "net/http" "github.com/cli/go-gh/v2/pkg/api" graphql "github.com/cli/shurcooL-graphql" @@ -399,16 +400,96 @@ func toGraphQLInt(n int) (graphql.Int, error) { return graphql.Int(n), nil } +// RemoteStackBase describes the base ref (and optionally SHA) of a stack. +type RemoteStackBase struct { + Ref string `json:"ref"` + Sha string `json:"sha,omitempty"` +} + +// RemoteStackPRHead describes the head ref of a pull request in a stack. +type RemoteStackPRHead struct { + Ref string `json:"ref"` + Sha string `json:"sha"` +} + +// RemoteStackPR is a pull request entry within a remote stack, as returned by +// the Stacks REST API list/detail endpoints. +type RemoteStackPR struct { + Number int `json:"number"` + State string `json:"state"` // open, closed + Draft bool `json:"draft"` + MergedAt *string `json:"merged_at"` + Head RemoteStackPRHead `json:"head"` +} + +// IsMerged reports whether the pull request has been merged. +func (p RemoteStackPR) IsMerged() bool { + return p.MergedAt != nil && *p.MergedAt != "" +} + +// RemoteStack represents a stack of pull requests as returned by the public +// Stacks REST API (GET/POST /repos/{owner}/{repo}/stacks...). ID is the +// internal identifier; Number is the human-facing stack number shown in the +// github.com UI and used to address the stack in API paths. +// +// The API returns pull_requests as an array of objects; UnmarshalJSON flattens +// them to the ordered PullRequests numbers (bottom to top) and preserves the +// full entries in PRDetails for callers that need head refs or PR state. type RemoteStack struct { - ID int `json:"id"` - PullRequests []int `json:"pull_requests"` + ID int `json:"id"` + Number int `json:"number"` + NodeID string `json:"node_id"` + URL string `json:"url"` + Base RemoteStackBase `json:"base"` + Open bool `json:"open"` + CreatedAt string `json:"created_at"` + PullRequests []int `json:"-"` + PRDetails []RemoteStackPR `json:"-"` +} + +// UnmarshalJSON decodes the Stacks REST API representation, deriving the +// ordered PullRequests numbers from the pull_requests objects. +func (s *RemoteStack) UnmarshalJSON(data []byte) error { + type wire struct { + ID int `json:"id"` + Number int `json:"number"` + NodeID string `json:"node_id"` + URL string `json:"url"` + Base RemoteStackBase `json:"base"` + Open bool `json:"open"` + CreatedAt string `json:"created_at"` + PullRequests []RemoteStackPR `json:"pull_requests"` + } + var w wire + if err := json.Unmarshal(data, &w); err != nil { + return err + } + s.ID = w.ID + s.Number = w.Number + s.NodeID = w.NodeID + s.URL = w.URL + s.Base = w.Base + s.Open = w.Open + s.CreatedAt = w.CreatedAt + s.PRDetails = w.PullRequests + s.PullRequests = make([]int, len(w.PullRequests)) + for i, p := range w.PullRequests { + s.PullRequests[i] = p.Number + } + return nil } -// ListStacks returns all stacks in the repository. -// Returns an empty slice if no stacks exist. -// A 404 response indicates stacked PRs are not enabled for this repository. +// PRNumbers returns the ordered pull request numbers in the stack, from bottom +// to top. +func (s *RemoteStack) PRNumbers() []int { + return s.PullRequests +} + +// ListStacks returns all stacks in the repository, ordered by stack number +// (descending). Returns an empty slice if no stacks exist. A 404 response +// indicates stacked PRs are not enabled for this repository. func (c *Client) ListStacks() ([]RemoteStack, error) { - path := fmt.Sprintf("repos/%s/%s/cli_internal/pulls/stacks", c.owner, c.repo) + path := fmt.Sprintf("repos/%s/%s/stacks", c.owner, c.repo) var stacks []RemoteStack if err := c.rest.Get(path, &stacks); err != nil { return nil, err @@ -419,58 +500,92 @@ func (c *Client) ListStacks() ([]RemoteStack, error) { return stacks, nil } -// CreateStack creates a stack on GitHub from an ordered list of PR numbers. -// The PR numbers must be ordered from bottom to top of the stack and must -// form a valid base-to-head chain. Returns the server-assigned stack ID. -func (c *Client) CreateStack(prNumbers []int) (int, error) { - type createStackRequest struct { - PullRequestNumbers []int `json:"pull_request_numbers"` +// FindStackForPR returns the stack that contains the given pull request number, +// using the list endpoint's server-side pull_request filter. Returns nil +// (without error) when the PR is not part of any stack. +func (c *Client) FindStackForPR(prNumber int) (*RemoteStack, error) { + path := fmt.Sprintf("repos/%s/%s/stacks?pull_request=%d", c.owner, c.repo, prNumber) + var stacks []RemoteStack + if err := c.rest.Get(path, &stacks); err != nil { + return nil, err } - - body, err := json.Marshal(createStackRequest{PullRequestNumbers: prNumbers}) - if err != nil { - return 0, fmt.Errorf("marshaling request: %w", err) + if len(stacks) == 0 { + return nil, nil } + return &stacks[0], nil +} - path := fmt.Sprintf("repos/%s/%s/cli_internal/pulls/stacks", c.owner, c.repo) +// GetStack fetches a single stack by its stack number. +func (c *Client) GetStack(stackNumber int) (*RemoteStack, error) { + path := fmt.Sprintf("repos/%s/%s/stacks/%d", c.owner, c.repo, stackNumber) + var rs RemoteStack + if err := c.rest.Get(path, &rs); err != nil { + return nil, err + } + return &rs, nil +} - var response struct { - ID int `json:"id"` +// CreateStack creates a stack on GitHub from an ordered list of PR numbers. +// The PR numbers must be ordered from bottom to top of the stack (at least two) +// and must form a valid base-to-head chain. Returns the created stack. +func (c *Client) CreateStack(prNumbers []int) (*RemoteStack, error) { + type createStackRequest struct { + PullRequests []int `json:"pull_requests"` } - if err := c.rest.Post(path, bytes.NewReader(body), &response); err != nil { - return 0, err + body, err := json.Marshal(createStackRequest{PullRequests: prNumbers}) + if err != nil { + return nil, fmt.Errorf("marshaling request: %w", err) } - return response.ID, nil + path := fmt.Sprintf("repos/%s/%s/stacks", c.owner, c.repo) + var rs RemoteStack + if err := c.rest.Post(path, bytes.NewReader(body), &rs); err != nil { + return nil, err + } + return &rs, nil } -// UpdateStack adds pull requests to an existing stack on GitHub. -// The stack is identified by stackID. The full list of PR numbers in the -// updated stack must be provided, including existing and new PRs, ordered -// from bottom to top. -func (c *Client) UpdateStack(stackID string, prNumbers []int) error { - type updateStackRequest struct { - PullRequestNumbers []int `json:"pull_request_numbers"` +// AddToStack appends pull requests to the top of an existing stack. Only the +// new PR numbers (the delta) should be provided, ordered from the current top +// of the stack upward. Returns the updated stack. +func (c *Client) AddToStack(stackNumber int, prNumbers []int) (*RemoteStack, error) { + type addToStackRequest struct { + PullRequests []int `json:"pull_requests"` } - body, err := json.Marshal(updateStackRequest{PullRequestNumbers: prNumbers}) + body, err := json.Marshal(addToStackRequest{PullRequests: prNumbers}) if err != nil { - return fmt.Errorf("marshaling request: %w", err) + return nil, fmt.Errorf("marshaling request: %w", err) } - path := fmt.Sprintf("repos/%s/%s/cli_internal/pulls/stacks/%s", c.owner, c.repo, stackID) + path := fmt.Sprintf("repos/%s/%s/stacks/%d/add", c.owner, c.repo, stackNumber) + var rs RemoteStack + if err := c.rest.Post(path, bytes.NewReader(body), &rs); err != nil { + return nil, err + } + return &rs, nil +} - var response struct { - ID int `json:"id"` +// Unstack removes unlocked pull requests from a stack. The server leaves PRs +// that cannot be unstacked (queued for merge or with auto-merge enabled) in +// place. When PRs remain, the updated stack is returned with dissolved=false; +// when none remain the stack is destroyed and dissolved=true (HTTP 204). +func (c *Client) Unstack(stackNumber int) (rs *RemoteStack, dissolved bool, err error) { + path := fmt.Sprintf("repos/%s/%s/stacks/%d/unstack", c.owner, c.repo, stackNumber) + resp, err := c.rest.Request(http.MethodPost, path, nil) + if err != nil { + return nil, false, err } + defer func() { _ = resp.Body.Close() }() - return c.rest.Put(path, bytes.NewReader(body), &response) -} + if resp.StatusCode == http.StatusNoContent { + return nil, true, nil + } -// DeleteStack deletes a stack on GitHub. -// The stack is identified by stackID. Returns nil on success (204). -func (c *Client) DeleteStack(stackID string) error { - path := fmt.Sprintf("repos/%s/%s/cli_internal/pulls/stacks/%s", c.owner, c.repo, stackID) - return c.rest.Delete(path, nil) + var remaining RemoteStack + if decErr := json.NewDecoder(resp.Body).Decode(&remaining); decErr != nil { + return nil, false, fmt.Errorf("decoding unstack response: %w", decErr) + } + return &remaining, false, nil } diff --git a/internal/github/github_test.go b/internal/github/github_test.go index a7e7822..1f18edf 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -1,10 +1,12 @@ package github import ( + "encoding/json" "testing" graphql "github.com/cli/shurcooL-graphql" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestPRURL(t *testing.T) { @@ -80,3 +82,69 @@ func TestToGraphQLInt(t *testing.T) { assert.Error(t, err) }) } + +// TestRemoteStack_UnmarshalJSON verifies the custom decoder that flattens the +// Stacks REST API's pull_requests object array into ordered PullRequests +// numbers while preserving the full entries in PRDetails. This is the only path +// that populates those fields from the wire, so a shape regression here would +// silently break every stack endpoint. +func TestRemoteStack_UnmarshalJSON(t *testing.T) { + payload := `{ + "id": 6154, + "number": 360, + "node_id": "S_kwABCD", + "url": "https://api.github.com/repos/o/r/stacks/360", + "base": {"ref": "main", "sha": "basesha"}, + "open": true, + "created_at": "2026-01-01T00:00:00Z", + "pull_requests": [ + {"number": 12, "state": "open", "draft": true, "merged_at": null, "head": {"ref": "feat-1", "sha": "sha1"}}, + {"number": 15, "state": "closed", "draft": false, "merged_at": "2026-01-02T00:00:00Z", "head": {"ref": "feat-2", "sha": "sha2"}} + ] + }` + + var s RemoteStack + require.NoError(t, json.Unmarshal([]byte(payload), &s)) + + // Top-level metadata. + assert.Equal(t, 6154, s.ID) + assert.Equal(t, 360, s.Number) + assert.Equal(t, "S_kwABCD", s.NodeID) + assert.Equal(t, "https://api.github.com/repos/o/r/stacks/360", s.URL) + assert.Equal(t, "main", s.Base.Ref) + assert.Equal(t, "basesha", s.Base.Sha) + assert.True(t, s.Open) + assert.Equal(t, "2026-01-01T00:00:00Z", s.CreatedAt) + + // Ordered PR numbers (bottom to top) derived from the object array. + assert.Equal(t, []int{12, 15}, s.PullRequests) + assert.Equal(t, []int{12, 15}, s.PRNumbers()) + + // Full PR entries preserved in PRDetails, including nullable merged_at. + require.Len(t, s.PRDetails, 2) + assert.Equal(t, 12, s.PRDetails[0].Number) + assert.Equal(t, "open", s.PRDetails[0].State) + assert.True(t, s.PRDetails[0].Draft) + assert.Nil(t, s.PRDetails[0].MergedAt) + assert.False(t, s.PRDetails[0].IsMerged()) + assert.Equal(t, "feat-1", s.PRDetails[0].Head.Ref) + assert.Equal(t, "sha1", s.PRDetails[0].Head.Sha) + + assert.Equal(t, 15, s.PRDetails[1].Number) + assert.Equal(t, "closed", s.PRDetails[1].State) + require.NotNil(t, s.PRDetails[1].MergedAt) + assert.Equal(t, "2026-01-02T00:00:00Z", *s.PRDetails[1].MergedAt) + assert.True(t, s.PRDetails[1].IsMerged()) + assert.Equal(t, "feat-2", s.PRDetails[1].Head.Ref) +} + +// TestRemoteStack_UnmarshalJSON_EmptyPRs ensures a stack with no pull_requests +// decodes to empty (non-nil) slices rather than panicking. +func TestRemoteStack_UnmarshalJSON_EmptyPRs(t *testing.T) { + var s RemoteStack + require.NoError(t, json.Unmarshal([]byte(`{"id": 1, "number": 2, "pull_requests": []}`), &s)) + assert.Equal(t, 1, s.ID) + assert.Equal(t, 2, s.Number) + assert.Empty(t, s.PullRequests) + assert.Empty(t, s.PRDetails) +} diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 5bff2b8..6c3a467 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -12,9 +12,11 @@ type MockClient struct { MarkPRReadyForReviewFn func(string) error DisableAutoMergeFn func(string) error ListStacksFn func() ([]RemoteStack, error) - CreateStackFn func([]int) (int, error) - UpdateStackFn func(string, []int) error - DeleteStackFn func(string) error + FindStackForPRFn func(int) (*RemoteStack, error) + GetStackFn func(int) (*RemoteStack, error) + CreateStackFn func([]int) (*RemoteStack, error) + AddToStackFn func(int, []int) (*RemoteStack, error) + UnstackFn func(int) (*RemoteStack, bool, error) } // Compile-time check that MockClient satisfies ClientOps. @@ -76,23 +78,37 @@ func (m *MockClient) ListStacks() ([]RemoteStack, error) { return nil, nil } -func (m *MockClient) CreateStack(prNumbers []int) (int, error) { +func (m *MockClient) FindStackForPR(prNumber int) (*RemoteStack, error) { + if m.FindStackForPRFn != nil { + return m.FindStackForPRFn(prNumber) + } + return nil, nil +} + +func (m *MockClient) GetStack(stackNumber int) (*RemoteStack, error) { + if m.GetStackFn != nil { + return m.GetStackFn(stackNumber) + } + return &RemoteStack{}, nil +} + +func (m *MockClient) CreateStack(prNumbers []int) (*RemoteStack, error) { if m.CreateStackFn != nil { return m.CreateStackFn(prNumbers) } - return 0, nil + return &RemoteStack{}, nil } -func (m *MockClient) UpdateStack(stackID string, prNumbers []int) error { - if m.UpdateStackFn != nil { - return m.UpdateStackFn(stackID, prNumbers) +func (m *MockClient) AddToStack(stackNumber int, prNumbers []int) (*RemoteStack, error) { + if m.AddToStackFn != nil { + return m.AddToStackFn(stackNumber, prNumbers) } - return nil + return &RemoteStack{}, nil } -func (m *MockClient) DeleteStack(stackID string) error { - if m.DeleteStackFn != nil { - return m.DeleteStackFn(stackID) +func (m *MockClient) Unstack(stackNumber int) (*RemoteStack, bool, error) { + if m.UnstackFn != nil { + return m.UnstackFn(stackNumber) } - return nil + return nil, false, nil } diff --git a/internal/stack/schema.json b/internal/stack/schema.json index 3e9f2ab..a7c2e48 100644 --- a/internal/stack/schema.json +++ b/internal/stack/schema.json @@ -28,7 +28,11 @@ "properties": { "id": { "type": "string", - "description": "Identifier for this stack, populated from the API when available." + "description": "Global identifier for this stack, populated from the API when available." + }, + "number": { + "type": "integer", + "description": "Repo-scoped number identifying this stack, displayed in the GitHub UI. Used as the primary way to reference a stack." }, "prefix": { "type": "string", diff --git a/internal/stack/stack.go b/internal/stack/stack.go index a363820..657fa4a 100644 --- a/internal/stack/stack.go +++ b/internal/stack/stack.go @@ -43,6 +43,7 @@ type BranchRef struct { // Stack represents a single stack of branches. type Stack struct { ID string `json:"id,omitempty"` + Number int `json:"number,omitempty"` Prefix string `json:"prefix,omitempty"` Numbered bool `json:"numbered,omitempty"` Trunk BranchRef `json:"trunk"` diff --git a/internal/stack/stack_test.go b/internal/stack/stack_test.go index 5bdfbb2..cc47a79 100644 --- a/internal/stack/stack_test.go +++ b/internal/stack/stack_test.go @@ -235,6 +235,7 @@ func TestLoad_Save_RoundTrip(t *testing.T) { Stacks: []Stack{ { ID: "s1", + Number: 7, Prefix: "feat", Trunk: BranchRef{Branch: "main", Head: "abc123"}, Branches: []BranchRef{ @@ -256,6 +257,7 @@ func TestLoad_Save_RoundTrip(t *testing.T) { s := loaded.Stacks[0] assert.Equal(t, "s1", s.ID) + assert.Equal(t, 7, s.Number) assert.Equal(t, "feat", s.Prefix) assert.Equal(t, "main", s.Trunk.Branch) assert.Equal(t, "abc123", s.Trunk.Head)