Fix unstack to handle partially merged stacks (#574)

* fix unstack behavior to treat partial unstacks successfully

* increase test timeout

Author: skarim

.github/workflows/test.yml

@@ -27,4 +27,4 @@ jobs:
         run: go vet ./...
       - name: Test
         # The real-Git worktree cases can exceed Go's 10-minute package limit on Windows.
-        run: go test -race -count=1 -timeout=20m ./...
+        run: go test -race -count=1 -timeout=30m ./...

README.md

@@ -575,7 +575,7 @@ Provide a stack number (the identifier shown in the github.com stack UI) to unst
 
 Use `--local` to only remove local tracking without contacting GitHub.
 
-GitHub decides which pull requests can be unstacked: PRs that are queued for merge or have auto-merge enabled are left stacked. When some pull requests remain stacked, the stack is kept (and local tracking, if any, is unchanged).
+PRs that are merged, merging, or queued for merge cannot be removed from a stack on GitHub. After a successful unstack, local tracking is removed even when some PRs remain on GitHub.
 
 | Flag | Description |
 |------|-------------|

cmd/unstack.go

@@ -36,9 +36,9 @@ local tracking is removed as well.
 
 Use --local to only remove local tracking without touching remote (GitHub).
 
-GitHub decides which pull requests can be unstacked: PRs that are queued for
-merge or have auto-merge enabled are left stacked. When some pull requests
-remain stacked, the stack is kept (and local tracking, if any, is unchanged).`,
+GitHub decides which pull requests can be unstacked. Merged or queued PRs remain
+stacked on GitHub. After a successful unstack, local tracking is removed even
+when some PRs remain on GitHub.`,
 		Example: `  # Unstack the current stack locally and on GitHub
   $ gh stack unstack
 
@@ -110,18 +110,16 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error {
 	return unstackTrackedStack(cfg, opts, result)
 }
 
-// unstackTrackedStack unstacks a locally tracked stack: it removes the stack on
-// GitHub (unless --local) and then removes it from local tracking.
+// unstackTrackedStack unstacks eligible PRs on GitHub (unless --local) and then
+// removes the stack from local tracking.
 func unstackTrackedStack(cfg *config.Config, opts *unstackOptions, result *loadStackResult) error {
 	gitDir := result.GitDir
 
 	sf := result.StackFile
 	s := result.Stack
 
 	// 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.
+	// can be removed; successful partial unstacking still removes local tracking.
 	if !opts.local {
 		if s.ID == "" && s.Number == 0 {
 			cfg.Warningf("Stack has no remote ID — skipping server-side unstack")
@@ -140,16 +138,8 @@ func unstackTrackedStack(cfg *config.Config, opts *unstackOptions, result *loadS
 
 			if number == 0 {
 				cfg.Warningf("Stack not found on GitHub — continuing with local unstack")
-			} else {
-				keepLocal, err := unstackNumberOnGitHub(cfg, client, number, true)
-				if err != nil {
-					return err
-				}
-				if keepLocal {
-					// Some PRs remain stacked, so the stack still exists on
-					// GitHub. Keep local tracking so it continues to reflect it.
-					return nil
-				}
+			} else if err := unstackNumberOnGitHub(cfg, client, number, true); err != nil {
+				return err
 			}
 		}
 	}
@@ -181,24 +171,13 @@ func runRemoteUnstack(cfg *config.Config, number int) error {
 		cfg.Errorf("failed to create GitHub client: %s", err)
 		return ErrAPIFailure
 	}
-	if _, err := unstackNumberOnGitHub(cfg, client, number, false); err != nil {
-		return err
-	}
-	return nil
+	return unstackNumberOnGitHub(cfg, client, number, false)
 }
 
 // unstackNumberOnGitHub calls the Unstack API for the given stack number and
-// reports the outcome. hasLocalTracking indicates whether the caller has a
-// locally tracked stack to reconcile, which changes how a 404 and a partial
-// unstack are handled: with local tracking a 404 is an idempotent success (the
-// caller finishes removing local state) and a partial unstack keeps local
-// tracking; without it a 404 is a hard error because the user targeted a stack
-// that does not exist on GitHub.
-//
-// It returns keepLocal=true when local tracking should be preserved because a
-// partial unstack left some PRs stacked. keepLocal is only meaningful when
-// hasLocalTracking is true.
-func unstackNumberOnGitHub(cfg *config.Config, client github.ClientOps, number int, hasLocalTracking bool) (keepLocal bool, err error) {
+// reports the outcome. With local tracking, a 404 is an idempotent success so
+// the caller can finish local cleanup; otherwise it is a not-found error.
+func unstackNumberOnGitHub(cfg *config.Config, client github.ClientOps, number int, hasLocalTracking bool) error {
 	_, dissolved, err := client.Unstack(number)
 	if err != nil {
 		var httpErr *api.HTTPError
@@ -209,36 +188,28 @@ func unstackNumberOnGitHub(cfg *config.Config, client github.ClientOps, number i
 					// Stack already gone on GitHub — treat as success and let
 					// the caller finish removing local tracking.
 					cfg.Warningf("Stack not found on GitHub — continuing with local unstack")
-					return false, nil
+					return nil
 				}
 				// Remote-first: the targeted stack does not exist on GitHub.
 				cfg.Errorf("stack #%d not found on GitHub", number)
-				return false, ErrNotInStack
+				return ErrNotInStack
 			case 422:
-				// 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 false, ErrInvalidArgs
+				return ErrInvalidArgs
 			default:
 				cfg.Errorf("Failed to unstack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message)
-				return false, ErrAPIFailure
+				return ErrAPIFailure
 			}
 		}
 		cfg.Errorf("Failed to unstack on GitHub: %v", err)
-		return false, ErrAPIFailure
+		return ErrAPIFailure
 	}
 
-	if !dissolved {
-		// Some PRs (queued for merge or with auto-merge enabled) remain stacked
-		// on GitHub, so the stack still exists.
-		cfg.Warningf("Some pull requests are queued for merge or have auto-merge enabled and remain stacked on GitHub")
-		if hasLocalTracking {
-			cfg.Printf("The stack was left in place — local tracking is unchanged")
-			return true, nil
-		}
-		return false, nil
+	if dissolved {
+		cfg.Successf("Stack removed on GitHub%s", stackLabel(number))
+	} else {
+		cfg.Successf("Pull requests unstacked on GitHub%s", stackLabel(number))
+		cfg.Infof("Some pull requests remain stacked on GitHub")
 	}
-
-	cfg.Successf("Stack removed on GitHub%s", stackLabel(number))
-	return false, nil
+	return nil
 }

cmd/unstack_test.go

@@ -297,9 +297,7 @@ func TestUnstack_RemovesCorrectStackByPointer(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.
+	// The server rejects the unstack when no PRs can be removed.
 	gitDir := t.TempDir()
 	restore := git.SetOps(&git.MockOps{
 		GitDirFn:        func() (string, error) { return gitDir, nil },
@@ -322,7 +320,7 @@ func TestUnstack_AllLocked_ServerRejects(t *testing.T) {
 	cfg.GitHubClientOverride = &github.MockClient{
 		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"}
+			return nil, false, &api.HTTPError{StatusCode: 422, Message: "Pull requests #101, #102 cannot be removed from this stack"}
 		},
 	}
 
@@ -339,45 +337,67 @@ func TestUnstack_AllLocked_ServerRejects(t *testing.T) {
 	require.Len(t, sf.Stacks, 1)
 }
 
-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 },
-		CurrentBranchFn: func() (string, error) { return "b1", nil },
-	})
-	defer restore()
-
-	writeStackFile(t, gitDir, stack.Stack{
-		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}},
-		},
-	})
-
-	cfg, outR, errR := config.NewTestConfig()
-	cfg.GitHubClientOverride = &github.MockClient{
-		UnstackFn: func(int) (*github.RemoteStack, bool, error) {
-			return &github.RemoteStack{ID: 99, Number: 99, PullRequests: []int{102}}, false, nil
-		},
+func TestUnstack_PartialUnstack_RemovesLocalTracking(t *testing.T) {
+	tests := []struct {
+		name         string
+		stackNumber  int
+		remainingPRs []int
+	}{
+		{name: "merged", remainingPRs: []int{101}},
+		{name: "merged and queued", remainingPRs: []int{101, 102}},
+		{name: "by stack number", stackNumber: 99, remainingPRs: []int{101}},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			gitDir := t.TempDir()
+			restore := git.SetOps(&git.MockOps{
+				GitDirFn:        func() (string, error) { return gitDir, nil },
+				CurrentBranchFn: func() (string, error) { return "b2", nil },
+			})
+			defer restore()
+
+			writeTwoStacks(t, gitDir, stack.Stack{
+				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}},
+					{Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 103}},
+				},
+			}, stack.Stack{
+				ID:       "42",
+				Number:   42,
+				Trunk:    stack.BranchRef{Branch: "main"},
+				Branches: []stack.BranchRef{{Branch: "other"}},
+			})
+
+			cfg, outR, errR := config.NewTestConfig()
+			cfg.GitHubClientOverride = &github.MockClient{
+				UnstackFn: func(number int) (*github.RemoteStack, bool, error) {
+					assert.Equal(t, 99, number)
+					return &github.RemoteStack{ID: 99, Number: 99, PullRequests: tt.remainingPRs}, false, nil
+				},
+			}
+
+			err := runUnstack(cfg, &unstackOptions{stackNumber: tt.stackNumber})
+			output := collectOutput(cfg, outR, errR)
+
+			require.NoError(t, err)
+			assert.Contains(t, output, "Pull requests unstacked on GitHub")
+			assert.Contains(t, output, "Some pull requests remain stacked on GitHub")
+			assert.Contains(t, output, "Stack removed from local tracking")
+			assert.NotContains(t, output, "auto-merge")
+			assert.NotContains(t, output, "local tracking is unchanged")
+			assert.NotContains(t, output, "⚠")
+
+			sf, loadErr := stack.Load(gitDir)
+			require.NoError(t, loadErr)
+			require.Len(t, sf.Stacks, 1)
+			assert.Equal(t, 42, sf.Stacks[0].Number)
+			assert.Equal(t, []string{"other"}, sf.Stacks[0].BranchNames())
+		})
 	}
-
-	err := runUnstack(cfg, &unstackOptions{})
-	output := collectOutput(cfg, outR, errR)
-
-	require.NoError(t, err)
-	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_NumberLookupFailure_StopsDeletion(t *testing.T) {
@@ -542,8 +562,7 @@ func TestUnstack_ByStackNumber_RemoteOnly_NotFound(t *testing.T) {
 }
 
 func TestUnstack_ByStackNumber_RemoteOnly_Partial(t *testing.T) {
-	// Some PRs (queued for merge / auto-merge) remain stacked. There is no local
-	// tracking to keep, so the command reports the outcome and succeeds.
+	// A partial remote-only unstack succeeds without changing unrelated local tracking.
 	gitDir := t.TempDir()
 	restore := git.SetOps(&git.MockOps{
 		GitDirFn:        func() (string, error) { return gitDir, nil },
@@ -568,7 +587,10 @@ func TestUnstack_ByStackNumber_RemoteOnly_Partial(t *testing.T) {
 	output := collectOutput(cfg, outR, errR)
 
 	require.NoError(t, err)
-	assert.Contains(t, output, "remain stacked on GitHub")
+	assert.Contains(t, output, "Pull requests unstacked on GitHub")
+	assert.Contains(t, output, "Some pull requests remain stacked on GitHub")
+	assert.NotContains(t, output, "auto-merge")
+	assert.NotContains(t, output, "⚠")
 	// No local tracking is involved, so no local-tracking messaging is shown.
 	assert.NotContains(t, output, "local tracking is unchanged")
 	assert.NotContains(t, output, "Stack removed from local tracking")
@@ -579,8 +601,7 @@ func TestUnstack_ByStackNumber_RemoteOnly_Partial(t *testing.T) {
 }
 
 func TestUnstack_ByStackNumber_RemoteOnly_AllLocked(t *testing.T) {
-	// Every PR is queued for merge or has auto-merge enabled; the server rejects
-	// the unstack with a 422 and the command surfaces the error.
+	// The server rejects the unstack when no PRs can be removed.
 	gitDir := t.TempDir()
 	restore := git.SetOps(&git.MockOps{
 		GitDirFn:        func() (string, error) { return gitDir, nil },
@@ -598,7 +619,7 @@ func TestUnstack_ByStackNumber_RemoteOnly_AllLocked(t *testing.T) {
 	cfg, outR, errR := config.NewTestConfig()
 	cfg.GitHubClientOverride = &github.MockClient{
 		UnstackFn: func(int) (*github.RemoteStack, bool, error) {
-			return nil, false, &api.HTTPError{StatusCode: 422, Message: "all pull requests are queued for merge or have auto-merge enabled"}
+			return nil, false, &api.HTTPError{StatusCode: 422, Message: "Pull requests #101, #102 cannot be removed from this stack"}
 		},
 	}
 	err := runUnstack(cfg, &unstackOptions{stackNumber: 999})
@@ -700,8 +721,7 @@ func TestUnstack_ByStackNumber_LocalFlag_LegacyStack_NoRemoteCall(t *testing.T)
 
 func TestUnstack_ByStackNumber_LegacyStackResolvedByID(t *testing.T) {
 	// A stack tracked before the number was recorded (Number == 0) is resolved
-	// by mapping its internal ID to the remote stack number, and the backfilled
-	// number is persisted.
+	// by mapping its internal ID to the remote stack number.
 	gitDir := t.TempDir()
 	restore := git.SetOps(&git.MockOps{
 		GitDirFn:        func() (string, error) { return gitDir, nil },
@@ -726,7 +746,6 @@ func TestUnstack_ByStackNumber_LegacyStackResolvedByID(t *testing.T) {
 		},
 		UnstackFn: func(n int) (*github.RemoteStack, bool, error) {
 			unstackedNumber = n
-			// Some PRs remain stacked, so local tracking is kept.
 			return &github.RemoteStack{ID: 99, Number: 7, PullRequests: []int{102}}, false, nil
 		},
 	}
@@ -737,10 +756,9 @@ func TestUnstack_ByStackNumber_LegacyStackResolvedByID(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, 7, unstackedNumber, "should resolve the legacy stack and unstack by its remote number")
 	assert.Contains(t, output, "remain stacked on GitHub")
+	assert.Contains(t, output, "Stack removed from local tracking")
 
-	// The backfilled number is persisted to the stack file.
 	sf, loadErr := stack.Load(gitDir)
 	require.NoError(t, loadErr)
-	require.Len(t, sf.Stacks, 1)
-	assert.Equal(t, 7, sf.Stacks[0].Number, "the resolved stack number should be persisted")
+	assert.Empty(t, sf.Stacks)
 }

docs/src/content/docs/reference/cli.md

@@ -306,7 +306,7 @@ With no argument, the command targets the active stack — the one that contains
 
 Provide a stack number (the identifier shown in the github.com stack UI) to unstack a specific stack on GitHub. This works from anywhere in the repository, whether or not the stack is checked out locally — the stack is unstacked directly through the GitHub API. When the stack is also available locally, its local tracking is removed as well.
 
-PRs that are merged, merging, or queued for merge cannot be removed from a stack on GitHub and are left part of the stack. When every pull request is removed, the stack is dissolved and any local tracking is removed; when some pull requests remain stacked, the stack is kept and local tracking, if any, is unchanged. Use `--local` to skip the remote operation and only remove local tracking.
+PRs that are merged, merging, or queued for merge cannot be removed from a stack on GitHub. After a successful unstack, local tracking is removed even when some PRs remain on GitHub. Use `--local` to skip the remote operation and only remove local tracking.
 
 This is useful when you need to restructure a stack — remove a branch, insert a branch, reorder branches, rename branches, or make other large changes. After unstacking, use `gh stack init` to re-create the stack with the desired structure — existing branches are adopted automatically.
 

docs/src/content/docs/reference/rest-api.md

@@ -163,7 +163,7 @@ echo '{"pull_requests": [104]}' | \
 POST /repos/{owner}/{repo}/stacks/{stack_number}/unstack
 ```
 
-Removes the unmerged pull requests from a stack. This endpoint takes no request body. Pull requests that cannot be unstacked (those merged, merging, or queued for merge) are left in place.
+Removes eligible pull requests from a stack. This endpoint takes no request body. Merged or queued pull requests are left in place.
 
 - When pull requests remain in the stack, the updated stack is returned with `200 OK`.
 - When no pull requests remain, the stack is dissolved and `204 No Content` is returned.
@@ -174,7 +174,7 @@ gh api --method POST repos/OWNER/REPO/stacks/42/unstack
 
 ### The stack resource
 
-Each stack is represented by the following resource. The get, create, and add endpoints return a single stack; the list endpoint returns an array of them; and unstack returns the remaining merged stack, or `204 No Content` when the stack is dissolved.
+Each stack is represented by the following resource. The get, create, and add endpoints return a single stack; the list endpoint returns an array of them; and unstack returns the remaining stack, or `204 No Content` when the stack is dissolved.
 
 | Field | Type | Description |
 |-------|------|-------------|

internal/github/github.go

@@ -576,10 +576,10 @@ func (c *Client) AddToStack(stackNumber int, prNumbers []int) (*RemoteStack, err
 	return &rs, nil
 }
 
-// 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).
+// Unstack removes eligible pull requests from a stack. When PRs remain (e.g.
+// merged or queued PRs), the updated stack is returned with dissolved=false.
+// When none remain, the stack is destroyed and dissolved=true (HTTP 204).
+// Both outcomes indicate a successful unstack.
 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)