From 0fd5340b14cda1a379c9665529bb0c2f207825b1 Mon Sep 17 00:00:00 2001 From: Emily Wise Date: Wed, 17 Jun 2026 20:58:40 +0000 Subject: [PATCH 1/3] unstack: preflight PR eligibility before delete and improve API errors.Block unstack delete only when all PRs in the stack are ineligible --- cmd/unstack.go | 68 ++++++++++++++++ cmd/unstack_test.go | 187 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 253 insertions(+), 2 deletions(-) diff --git a/cmd/unstack.go b/cmd/unstack.go index 3bdf40e..5ab7568 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" @@ -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(cfg, 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, have auto-merge enabled, 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(cfg *config.Config, 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) +} From c008d0bd386eba6ec4e43c6410b93c1928659359 Mon Sep 17 00:00:00 2001 From: Emily Wise Date: Wed, 17 Jun 2026 14:28:22 -0700 Subject: [PATCH 2/3] Update cmd/unstack.go Co-authored-by: Sameen Karim --- cmd/unstack.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/unstack.go b/cmd/unstack.go index 5ab7568..376cebd 100644 --- a/cmd/unstack.go +++ b/cmd/unstack.go @@ -73,7 +73,7 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { return ErrAPIFailure } if blocked { - cfg.Errorf("Unstacking not allowed. Pull requests that are queued for merge, have auto-merge enabled, or are already merged will remain in the stack.") + 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 } From 777915d6450e3f39bc1367d21490a81d04c14eb3 Mon Sep 17 00:00:00 2001 From: Emily Wise Date: Thu, 18 Jun 2026 04:03:44 +0000 Subject: [PATCH 3/3] remove cfg and update help text --- cmd/unstack.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cmd/unstack.go b/cmd/unstack.go index 376cebd..2292df6 100644 --- a/cmd/unstack.go +++ b/cmd/unstack.go @@ -23,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 @@ -67,7 +67,7 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { return ErrAPIFailure } - blocked, err := shouldBlockUnstackDelete(cfg, client, s) + blocked, err := shouldBlockUnstackDelete(client, s) if err != nil { cfg.Errorf("failed to check pull request states before unstack: %s", err) return ErrAPIFailure @@ -118,7 +118,7 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error { return nil } -func shouldBlockUnstackDelete(cfg *config.Config, client github.ClientOps, s *stack.Stack) (bool, error) { +func shouldBlockUnstackDelete(client github.ClientOps, s *stack.Stack) (bool, error) { if s == nil || len(s.Branches) == 0 { return false, nil }