address review comments

Author: skarim

cmd/sync_test.go

@@ -2060,6 +2060,133 @@ func TestSync_RemoteAhead_PullsNewBranches(t *testing.T) {
 	assert.Equal(t, []string{"b1", "b2", "b3", "b4", "b5"}, sf.Stacks[0].BranchNames())
 }
 
+// TestSync_RemoteAhead_QueuedBranchNotPushed verifies that a pulled branch whose
+// PR is in the merge queue has its transient queued state copied from the fresh
+// PR details during reconciliation, so it is not force-pushed by the later push
+// step.
+func TestSync_RemoteAhead_QueuedBranchNotPushed(t *testing.T) {
+	s := stack.Stack{
+		ID:    "9",
+		Trunk: stack.BranchRef{Branch: "main"},
+		Branches: []stack.BranchRef{
+			{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}},
+			{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
+		},
+	}
+	tmpDir := t.TempDir()
+	writeStackFile(t, tmpDir, s)
+
+	var created []string
+	var pushes []pushCall
+	mock := newSyncMockNoRebase(tmpDir, "b1")
+	mock.BranchExistsFn = func(name string) bool { return name != "b3" }
+	mock.CreateBranchFn = func(name, base string) error { created = append(created, name); return nil }
+	mock.SetUpstreamTrackingFn = func(string, string) error { return nil }
+	mock.PushFn = func(remote string, branches []string, force, atomic bool) error {
+		pushes = append(pushes, pushCall{remote, branches, force, atomic})
+		return nil
+	}
+
+	ghMock := &github.MockClient{
+		ListStacksFn: func() ([]github.RemoteStack, error) {
+			return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 103}}}, nil
+		},
+		FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
+			branch := map[int]string{101: "b1", 102: "b2", 103: "b3"}[n]
+			if branch == "" {
+				return nil, nil
+			}
+			pr := &github.PullRequest{
+				Number: n, ID: fmt.Sprintf("PR_%d", n),
+				URL:         fmt.Sprintf("https://github.com/o/r/pull/%d", n),
+				HeadRefName: branch, State: "OPEN",
+			}
+			if n == 103 {
+				pr.MergeQueueEntry = &github.MergeQueueEntry{ID: "MQ1"}
+			}
+			return pr, nil
+		},
+	}
+
+	_, err := runSyncCfg(t, mock, func(cfg *config.Config) { cfg.GitHubClientOverride = ghMock })
+	require.NoError(t, err)
+
+	assert.Contains(t, created, "b3", "the queued branch is still pulled into the local stack")
+	for _, pc := range pushes {
+		assert.NotContains(t, pc.branches, "b3", "a merge-queued branch must not be pushed")
+	}
+}
+
+// TestSync_RemoteAhead_DuplicateBranchAborts verifies that pulling a remote
+// addition whose branch is already owned by another local stack aborts rather
+// than writing the branch into two stacks.
+func TestSync_RemoteAhead_DuplicateBranchAborts(t *testing.T) {
+	tmpDir := t.TempDir()
+	writeStackFileMulti(t, tmpDir,
+		stack.Stack{
+			ID:    "9",
+			Trunk: stack.BranchRef{Branch: "main"},
+			Branches: []stack.BranchRef{
+				{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}},
+				{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
+			},
+		},
+		stack.Stack{
+			Trunk:    stack.BranchRef{Branch: "main"},
+			Branches: []stack.BranchRef{{Branch: "b3"}}, // another stack already owns b3
+		},
+	)
+
+	var created []string
+	mock := newSyncMockNoRebase(tmpDir, "b1")
+	mock.CreateBranchFn = func(name, base string) error { created = append(created, name); return nil }
+
+	ghMock := &github.MockClient{
+		ListStacksFn: func() ([]github.RemoteStack, error) {
+			return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102, 103}}}, nil
+		},
+		FindPRByNumberFn: prByNumberFinder(map[int]string{101: "b1", 102: "b2", 103: "b3"}),
+	}
+
+	output, err := runSyncCfg(t, mock, func(cfg *config.Config) { cfg.GitHubClientOverride = ghMock })
+
+	assert.Error(t, err)
+	assert.Contains(t, output, "Cannot pull b3")
+	assert.NotContains(t, created, "b3", "must not pull a branch owned by another stack")
+
+	sf, loadErr := stack.Load(tmpDir)
+	require.NoError(t, loadErr)
+	assert.Equal(t, []string{"b1", "b2"}, sf.Stacks[0].BranchNames(), "tracked stack unchanged")
+}
+
+// TestSync_Divergent_UseRemote_DirtyCheckErrorAborts verifies that when the
+// working-tree status cannot be determined, "use remote" aborts instead of
+// treating the tree as clean and running the destructive replace.
+func TestSync_Divergent_UseRemote_DirtyCheckErrorAborts(t *testing.T) {
+	tmpDir := t.TempDir()
+	divergentStack(t, tmpDir)
+
+	ghMock := divergentRemoteMock()
+	var created []string
+	mock := newSyncMockNoRebase(tmpDir, "b1")
+	mock.CreateBranchFn = func(name, base string) error { created = append(created, name); return nil }
+	mock.HasUncommittedChangesFn = func() (bool, error) { return false, fmt.Errorf("git status failed") }
+
+	output, err := runSyncCfg(t, mock, func(cfg *config.Config) {
+		cfg.GitHubClientOverride = ghMock
+		cfg.ForceInteractive = true
+		cfg.SelectFn = func(_, _ string, _ []string) (int, error) { return 0, nil }
+	})
+
+	assert.Error(t, err)
+	assert.Contains(t, output, "Could not determine whether the working tree is clean")
+	assert.Empty(t, created, "must not replace the local stack when the working-tree check fails")
+
+	sf, loadErr := stack.Load(tmpDir)
+	require.NoError(t, loadErr)
+	assert.Equal(t, []string{"b1", "b2", "b3"}, sf.Stacks[0].BranchNames(), "local stack untouched")
+}
+
 // TestSync_RemoteInSync_NoPull verifies that when local and remote match, no
 // branches are pulled and no divergence is reported.
 func TestSync_RemoteInSync_NoPull(t *testing.T) {

cmd/utils.go

@@ -1268,7 +1268,11 @@ func reconcileRemoteStack(cfg *config.Config, sf *stack.StackFile, s *stack.Stac
 
 	switch classifyRemoteStack(localActive, remoteActive) {
 	case remoteStackInSync, remoteStackLocalAhead:
-		// Nothing to pull; the existing flow pushes/updates the remote.
+		// Nothing to pull; the existing flow pushes/updates the remote. Copy the
+		// freshly fetched PR state (merged/queued) onto the local branches so the
+		// fast-forward/rebase/push steps skip merged and merge-queued branches
+		// rather than rewriting them before the later PR-sync step runs.
+		syncRemotePRState(s, prs)
 		return res, nil
 	case remoteStackCleanAhead:
 		return pullRemoteAdditions(cfg, sf, s, gitDir, remote, prs)
@@ -1365,6 +1369,21 @@ func pullRemoteAdditions(cfg *config.Config, sf *stack.StackFile, s *stack.Stack
 		return res, nil
 	}
 
+	// A remote-added branch must not collide with a branch already tracked by
+	// another local stack, or with an existing local branch we would otherwise
+	// adopt as "pulled" without actually fetching it. Abort rather than persist
+	// duplicate ownership or a stale branch.
+	for _, pr := range newPRs {
+		if err := sf.ValidateNoDuplicateBranch(pr.HeadRefName); err != nil {
+			cfg.Errorf("Cannot pull %s from the remote stack: %s", pr.HeadRefName, err)
+			return res, ErrSilent
+		}
+		if git.BranchExists(pr.HeadRefName) {
+			cfg.Errorf("Cannot pull %s from the remote stack: a local branch with that name already exists", pr.HeadRefName)
+			return res, ErrSilent
+		}
+	}
+
 	newBranchNames := make([]string, len(newPRs))
 	for i, pr := range newPRs {
 		newBranchNames[i] = pr.HeadRefName
@@ -1397,6 +1416,9 @@ func pullRemoteAdditions(cfg *config.Config, sf *stack.StackFile, s *stack.Stack
 	}
 
 	if added > 0 {
+		// Copy the freshly fetched PR state (including the transient queued flag)
+		// onto the pulled branches so the rebase/push steps skip merge-queued ones.
+		syncRemotePRState(s, prs)
 		updateBaseSHAs(s)
 		if err := stack.Save(gitDir, sf); err != nil {
 			return res, handleSaveError(cfg, err)
@@ -1469,7 +1491,15 @@ func resolveStackDivergence(cfg *config.Config, client github.ClientOps, sf *sta
 func resolveDivergenceUseRemote(cfg *config.Config, sf *stack.StackFile, s *stack.Stack, currentBranch, gitDir, remote string, prs []*github.PullRequest) (remoteReconcileResult, error) {
 	var res remoteReconcileResult
 
-	if dirty, err := git.HasUncommittedChanges(); err == nil && dirty {
+	// Replacing the local stack is destructive, so require a known-clean working
+	// tree. Treat an inability to inspect the tree as a reason to abort (a failed
+	// status must not be read as "clean").
+	dirty, err := git.HasUncommittedChanges()
+	if err != nil {
+		cfg.Errorf("Could not determine whether the working tree is clean: %v", err)
+		return res, ErrSilent
+	}
+	if dirty {
 		cfg.Errorf("You have uncommitted changes — commit or stash them before replacing your local stack with the remote")
 		return res, ErrSilent
 	}
@@ -1479,11 +1509,26 @@ func resolveDivergenceUseRemote(cfg *config.Config, sf *stack.StackFile, s *stac
 	oldBranches := s.BranchNames()
 
 	removeLocalStack(sf, s)
+
+	// A remote PR branch must not already be owned by another local stack, or
+	// importing it would write the same branch into two stacks. Validate against
+	// the remaining stacks (the current one has been removed above).
+	for _, pr := range prs {
+		if err := sf.ValidateNoDuplicateBranch(pr.HeadRefName); err != nil {
+			cfg.Errorf("Cannot adopt the remote stack: %s", err)
+			return res, ErrSilent
+		}
+	}
+
 	newStack, err := importRemoteStack(cfg, sf, gitDir, remote, trunk, prs, remoteStackID)
 	if err != nil {
 		return res, err
 	}
 
+	// Populate the transient queued/merged state so the rebase/push steps skip
+	// merge-queued or merged branches in the adopted stack.
+	syncRemotePRState(newStack, prs)
+
 	// If the user was on a branch that the remote stack no longer contains,
 	// move them to the nearest surviving branch so they don't end up detached
 	// from the stack.

internal/tui/submitview/mouse.go

@@ -151,7 +151,9 @@ func (m Model) leftStackButtonHit(x, y int) bool {
 	if y-m.panelTopRow() != m.leftVisibleHeight()+1 {
 		return false
 	}
-	return x < lipgloss.Width(m.renderStackButton(leftW-2))
+	// The button is rendered inside the panel's one-cell left border, so the
+	// hit target starts at screen column 1, not 0.
+	return x >= 1 && x < 1+lipgloss.Width(m.renderStackButton(leftW-2))
 }
 
 // handleClick routes a left click to a branch row (left map) or an editor