Unstack Updates: preflight PR eligibility before delete and improve API errors (#136)

* unstack: preflight PR eligibility before delete and improve API errors.Block unstack delete only when all PRs in the stack are ineligible * Update cmd/unstack.go Co-authored-by: Sameen Karim <skarim@github.com> * remove cfg and update help text --------- Co-authored-by: Sameen Karim <skarim@github.com>
2026-09-14 20:26:28 +08:00 · 2026-06-19 11:07:51 -07:00
parent 4759126a51
commit 67d118fbf4
2 changed files with 254 additions and 3 deletions
@@ -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
+}
@@ -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)
+}