adressing review comments

Author: skarim

cmd/init.go

@@ -307,11 +307,11 @@ func runInit(cfg *config.Config, opts *initOptions) error {
 	// Discover existing PRs for the new stack's branches.
 	// For adopt, only record open/draft PRs (ignore closed/merged).
 	// For non-adopt, use the standard sync which also detects merges.
-	newStack_ := &sf.Stacks[len(sf.Stacks)-1]
+	latestStack := &sf.Stacks[len(sf.Stacks)-1]
 	if opts.adopt {
 		if client, clientErr := cfg.GitHubClient(); clientErr == nil {
-			for i := range newStack_.Branches {
-				b := &newStack_.Branches[i]
+			for i := range latestStack.Branches {
+				b := &latestStack.Branches[i]
 				pr, err := client.FindPRForBranch(b.Branch)
 				if err != nil || pr == nil {
 					continue
@@ -324,7 +324,7 @@ func runInit(cfg *config.Config, opts *initOptions) error {
 			}
 		}
 	} else {
-		syncStackPRs(cfg, newStack_)
+		syncStackPRs(cfg, latestStack)
 	}
 
 	if err := stack.Save(gitDir, sf); err != nil {

cmd/unstack.go

@@ -44,12 +44,6 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error {
 	gitDir := result.GitDir
 	sf := result.StackFile
 	s := result.Stack
-	target := opts.target
-	if target == "" {
-		target = result.CurrentBranch
-	}
-
-	cfg.Printf("Stack branches: %v", s.BranchNames())
 
 	// Delete the stack on GitHub first (unless --local).
 	// Only proceed with local deletion after the remote operation succeeds.
@@ -65,18 +59,33 @@ func runUnstack(cfg *config.Config, opts *unstackOptions) error {
 			if err := client.DeleteStack(s.ID); err != nil {
 				var httpErr *api.HTTPError
 				if errors.As(err, &httpErr) {
-					cfg.Errorf("Failed to delete stack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message)
+					switch httpErr.StatusCode {
+					case 404:
+						// Stack already deleted on GitHub — treat as success.
+						cfg.Warningf("Stack not found on GitHub — continuing with local unstack")
+					default:
+						cfg.Errorf("Failed to delete stack on GitHub (HTTP %d): %s", httpErr.StatusCode, httpErr.Message)
+						return ErrAPIFailure
+					}
 				} else {
 					cfg.Errorf("Failed to delete stack on GitHub: %v", err)
+					return ErrAPIFailure
 				}
-				return ErrAPIFailure
+			} else {
+				cfg.Successf("Stack deleted on GitHub")
 			}
-			cfg.Successf("Stack deleted on GitHub")
 		}
 	}
 
-	// Remove from local tracking
-	sf.RemoveStackForBranch(target)
+	// Remove the exact resolved stack from local tracking by pointer identity,
+	// not by branch name — avoids removing the wrong stack when a trunk
+	// branch is shared across multiple stacks.
+	for i := range sf.Stacks {
+		if &sf.Stacks[i] == s {
+			sf.RemoveStack(i)
+			break
+		}
+	}
 	if err := stack.Save(gitDir, sf); err != nil {
 		return handleSaveError(cfg, err)
 	}

cmd/unstack_test.go

@@ -157,7 +157,7 @@ func TestUnstack_NoStackID_WarnsAndSkipsAPI(t *testing.T) {
 	assert.NotContains(t, output, "Stack deleted on GitHub")
 }
 
-func TestUnstack_API404_ShowsErrorAndStopsLocalDeletion(t *testing.T) {
+func TestUnstack_API404_TreatedAsIdempotentSuccess(t *testing.T) {
 	gitDir := t.TempDir()
 	restore := git.SetOps(&git.MockOps{
 		GitDirFn:        func() (string, error) { return gitDir, nil },
@@ -180,15 +180,14 @@ func TestUnstack_API404_ShowsErrorAndStopsLocalDeletion(t *testing.T) {
 	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 404)")
-	// Should NOT remove locally when remote fails
-	assert.NotContains(t, output, "Stack removed from local tracking")
+	// 404 means already deleted — 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")
 
-	// Stack should still exist locally
 	sf, err := stack.Load(gitDir)
 	require.NoError(t, err)
-	require.Len(t, sf.Stacks, 1)
+	assert.Empty(t, sf.Stacks)
 }
 
 func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) {
@@ -224,3 +223,37 @@ func TestUnstack_API409_ShowsErrorAndStopsLocalDeletion(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, sf.Stacks, 1)
 }
+
+func TestUnstack_RemovesCorrectStackByPointer(t *testing.T) {
+	// Two stacks share the same trunk "main". Targeting "b3" should remove
+	// only the second stack (b3,b4), leaving the first (b1,b2) intact.
+	// This verifies pointer-based removal instead of branch-name-based.
+	gitDir := t.TempDir()
+	restore := git.SetOps(&git.MockOps{
+		GitDirFn:        func() (string, error) { return gitDir, nil },
+		CurrentBranchFn: func() (string, error) { return "b3", nil },
+	})
+	defer restore()
+
+	s1 := stack.Stack{
+		Trunk:    stack.BranchRef{Branch: "main"},
+		Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}},
+	}
+	s2 := stack.Stack{
+		Trunk:    stack.BranchRef{Branch: "main"},
+		Branches: []stack.BranchRef{{Branch: "b3"}, {Branch: "b4"}},
+	}
+	writeTwoStacks(t, gitDir, s1, s2)
+
+	cfg, outR, errR := config.NewTestConfig()
+	err := runUnstack(cfg, &unstackOptions{target: "b3", local: true})
+	output := collectOutput(cfg, outR, errR)
+
+	require.NoError(t, err)
+	assert.Contains(t, output, "Stack removed from local tracking")
+
+	sf, err := stack.Load(gitDir)
+	require.NoError(t, err)
+	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")
+}