diff --git a/cmd/unstack.go b/cmd/unstack.go index 3bdf40e..2292df6 100644 --- a/cmd/unstack.go +++ b/cmd/unstack.go @@ -2,9 +2,11 @@ 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" @@ -21,7 +23,7 @@ func UnstackCmd(cfg *config.Config) *cobra.Command { Use: "unstack", Aliases: []string{"delete"}, Short: "Delete a stack locally and on GitHub", - Long: "Remove the current active stack from local tracking and delete it on GitHub. Use --local to only remove local tracking.", + Long: "Remove the current active stack from local tracking and delete it on GitHub. Use --local to only remove local tracking. Full unstack is blocked when every pull request is queued for merge, merging, or already merged", Example: ` # Delete the stack locally and on GitHub $ gh stack unstack @@ -64,6 +66,17 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { cfg.Errorf("failed to create GitHub client: %s", err) return ErrAPIFailure } + + blocked, err := shouldBlockUnstackDelete(client, s) + if err != nil { + cfg.Errorf("failed to check pull request states before unstack: %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 { var httpErr *api.HTTPError if errors.As(err, &httpErr) { @@ -71,6 +84,9 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { case 404: // Stack already deleted 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 default: cfg.Errorf("Failed to delete stack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message) return ErrAPIFailure @@ -101,3 +117,55 @@ 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 5f74883..31f6303 100644 --- a/cmd/unstack_test.go +++ b/cmd/unstack_test.go @@ -2,6 +2,7 @@ package cmd import ( "encoding/json" + "errors" "os" "path/filepath" "testing" @@ -137,7 +138,10 @@ func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) { writeStackFile(t, gitDir, stack.Stack{ ID: "99", Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, + }, }) cfg, outR, errR := config.NewTestConfig() @@ -170,7 +174,10 @@ func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) { writeStackFile(t, gitDir, stack.Stack{ ID: "99", Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, + }, }) cfg, outR, errR := config.NewTestConfig() @@ -226,3 +233,179 @@ func TestUnstack_RemovesCorrectStackByPointer(t *testing.T) { require.Len(t, sf.Stacks, 1, "should remove exactly one stack") assert.Equal(t, []string{"b1", "b2"}, sf.Stacks[0].BranchNames(), "should keep the OTHER stack intact") } + +func TestUnstack_PreflightBlocksDelete_WhenAllPRsIneligible(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, Merged: true}}, + {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", MergeQueueEntry: &github.MergeQueueEntry{ID: "MQE_1"}}, nil + default: + return nil, nil + } + }, + DeleteStackFn: func(stackID string) error { + deleteCalled = true + return nil + }, + } + + err := runUnstack(cfg, &unstackOptions{}) + 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.Contains(t, output, "Unstacking not allowed") + assert.NotContains(t, output, "Stack removed from local tracking") + + sf, loadErr := stack.Load(gitDir) + require.NoError(t, loadErr) + require.Len(t, sf.Stacks, 1) +} + +func TestUnstack_PreflightAllowsDelete_WhenMixedEligibility(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}}, + {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 + }, + } + + err := runUnstack(cfg, &unstackOptions{}) + 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.NotContains(t, output, "Stack removed from local tracking") + + sf, loadErr := stack.Load(gitDir) + require.NoError(t, loadErr) + require.Len(t, sf.Stacks, 1) +} + +func TestUnstack_API422_ShowsInformativeErrorAndStopsLocalDeletion(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}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}}, + }, + }) + + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(number int) (*github.PullRequest, error) { + return &github.PullRequest{Number: number, State: "OPEN"}, nil + }, + DeleteStackFn: func(stackID string) error { + return &api.HTTPError{StatusCode: 422, Message: "some pull requests cannot be removed from stack"} + }, + } + 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.NotContains(t, output, "Stack removed from local tracking") + + sf, loadErr := stack.Load(gitDir) + require.NoError(t, loadErr) + require.Len(t, sf.Stacks, 1) +}