Force-push rewritten branches from sync, and stop signing off when it fails

Running `gh stack rebase` and then `gh stack sync` left the stack unpushed.
sync derived the force flag purely from whether it had rebased in that same
run, so after an earlier rebase it attempted a plain push; the branches had
rewritten history, the atomic push was rejected as a whole, and sync still
finished with "✓ Stack synced".

The flag now also accounts for branches that already diverged from their
remote-tracking refs, which is the state any earlier rebase leaves behind.
--force-with-lease still protects against overwriting someone else's work.

A push that does not land is also no longer reported as a synced stack: the
remote does not have the commits, so nothing downstream of the push reflects
the local stack. sync now says so and points at `gh stack push`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: f5959297-80fd-4732-aeae-aa9a4b6a7755

Author: skarim

cmd/stale_trunk_test.go

@@ -514,3 +514,133 @@ func TestRebase_Autostash_KeepsStashOnConflict(t *testing.T) {
 	assert.True(t, state.Stashed, "recovery state must record the pending stash")
 	assert.Equal(t, "main", state.TrunkRef, "recovery must resume against the same trunk ref")
 }
+
+// A `gh stack rebase` before `gh stack sync` leaves the branches diverged from
+// their remote refs. sync used to derive the force flag purely from whether it
+// had rebased in that same run, so the atomic push failed and the stack stayed
+// unpushed while sync still reported success.
+func TestSync_ForcePushesBranchesRebasedEarlier(t *testing.T) {
+	tmpDir := t.TempDir()
+	writeStackFile(t, tmpDir, twoBranchStack())
+
+	var pushCalls []pushCall
+
+	mock := newSyncMock(tmpDir, "b1")
+	mock.RevParseFn = func(ref string) (string, error) {
+		if ref == "main" || ref == "origin/main" {
+			return "trunk-sha", nil
+		}
+		// The local branches carry rewritten history.
+		if strings.HasPrefix(ref, "origin/") {
+			return "old-" + strings.TrimPrefix(ref, "origin/"), nil
+		}
+		return "new-" + ref, nil
+	}
+	// The stack is already correctly stacked, so no rebase happens this run,
+	// but the remote tips are not contained in the local branches.
+	mock.IsAncestorFn = func(ancestor, descendant string) (bool, error) {
+		if strings.HasPrefix(ancestor, "old-") {
+			return false, nil
+		}
+		return true, nil
+	}
+	mock.PushFn = func(remote string, branches []string, force, atomic bool) error {
+		pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic})
+		return nil
+	}
+
+	restore := git.SetOps(mock)
+	defer restore()
+
+	cfg, _, errR := config.NewTestConfig()
+	cmd := SyncCmd(cfg)
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	err := cmd.Execute()
+
+	cfg.Err.Close()
+	out, _ := io.ReadAll(errR)
+
+	assert.NoError(t, err)
+	require.Len(t, pushCalls, 1)
+	assert.True(t, pushCalls[0].force,
+		"branches rebased before this run still need --force-with-lease")
+	assert.Contains(t, string(out), "Pushed")
+}
+
+// When the push does not land, the stack on the remote does not reflect the
+// local one, so sync must not sign off as if it did.
+func TestSync_PushFailure_DoesNotReportSuccess(t *testing.T) {
+	tmpDir := t.TempDir()
+	writeStackFile(t, tmpDir, twoBranchStack())
+
+	mock := newSyncMock(tmpDir, "b1")
+	mock.PushFn = func(string, []string, bool, bool) error {
+		return errors.New("remote rejected")
+	}
+
+	restore := git.SetOps(mock)
+	defer restore()
+
+	cfg, _, errR := config.NewTestConfig()
+	cmd := SyncCmd(cfg)
+	cmd.SetOut(io.Discard)
+	cmd.SetErr(io.Discard)
+	err := cmd.Execute()
+
+	cfg.Err.Close()
+	out, _ := io.ReadAll(errR)
+	output := string(out)
+
+	assert.NoError(t, err, "a failed push is a warning, not a fatal error")
+	assert.Contains(t, output, "were not pushed")
+	assert.NotContains(t, output, "Stack synced")
+	assert.NotContains(t, output, "Branches synced")
+}
+
+func TestBranchesNeedForcePush(t *testing.T) {
+	tests := []struct {
+		name     string
+		ancestor func(string, string) (bool, error)
+		revParse func(string) (string, error)
+		want     bool
+	}{
+		{
+			name:     "remote tip contained locally needs no force",
+			ancestor: func(string, string) (bool, error) { return true, nil },
+			want:     false,
+		},
+		{
+			name:     "rewritten history needs force",
+			ancestor: func(string, string) (bool, error) { return false, nil },
+			want:     true,
+		},
+		{
+			name:     "a branch with no remote ref is ignored",
+			revParse: func(string) (string, error) { return "", errors.New("unknown revision") },
+			ancestor: func(string, string) (bool, error) { return false, nil },
+			want:     false,
+		},
+		{
+			name:     "unknown ancestry does not force",
+			ancestor: func(string, string) (bool, error) { return false, errors.New("boom") },
+			want:     false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			revParse := tt.revParse
+			if revParse == nil {
+				revParse = func(ref string) (string, error) { return "sha-" + ref, nil }
+			}
+			restore := git.SetOps(&git.MockOps{
+				RevParseFn:   revParse,
+				IsAncestorFn: tt.ancestor,
+			})
+			defer restore()
+
+			assert.Equal(t, tt.want, branchesNeedForcePush("origin", []string{"b1", "b2"}))
+		})
+	}
+}

cmd/sync.go

@@ -266,6 +266,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	// --- Step 4: Push ---
 	cfg.Printf("")
 	branches := activeBranchNames(s)
+	pushed := true
 
 	if mergedCount := len(s.MergedBranches()); mergedCount > 0 {
 		cfg.Printf("Skipping %d merged %s", mergedCount, plural(mergedCount, "branch", "branches"))
@@ -277,11 +278,14 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	if len(branches) == 0 {
 		cfg.Printf("No active branches to push (all merged)")
 	} else {
-		// After rebase, force-with-lease is required (history rewritten).
-		// Without rebase, try a normal push first.
-		force := rebased
+		// Rewritten history needs --force-with-lease. That is not only true of
+		// a rebase this run performed: a `gh stack rebase` beforehand leaves
+		// exactly the same divergence, and a plain push then fails for the
+		// whole atomic set.
+		force := rebased || branchesNeedForcePush(remote, branches)
 		cfg.Printf("Pushing %d %s to %s...", len(branches), plural(len(branches), "branch", "branches"), remote)
 		if err := git.Push(remote, branches, force, true); err != nil {
+			pushed = false
 			if !force {
 				cfg.Warningf("Push failed — branches may need force push after rebase")
 				cfg.Printf("  Run `%s` to push with --force-with-lease.",
@@ -436,9 +440,16 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	}
 
 	cfg.Printf("")
-	if stackSynced {
+	switch {
+	case !pushed:
+		// The branches were rebased and the PR state refreshed, but the remote
+		// does not have the new commits, so nothing downstream of the push
+		// actually reflects the local stack yet.
+		cfg.Warningf("Synced locally, but the branches were not pushed")
+		cfg.Printf("  Run `%s` to push them.", cfg.ColorCyan("gh stack push"))
+	case stackSynced:
 		cfg.Successf("Stack synced")
-	} else {
+	default:
 		// The branches were fetched, rebased, and pushed, but no stack object on
 		// GitHub was created or updated (no PRs, fewer than two PRs, stacked PRs
 		// unavailable, or a divergence). Report only what actually happened.
@@ -448,6 +459,26 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	return nil
 }
 
+// branchesNeedForcePush reports whether pushing any of the branches would
+// rewrite history on the remote, which requires --force-with-lease.
+//
+// A branch qualifies when its remote-tracking ref is not contained in the local
+// branch — the state a rebase leaves behind. Branches with no remote ref yet,
+// or whose ancestry cannot be determined, do not qualify: a plain push is the
+// safer default and its failure is reported.
+func branchesNeedForcePush(remote string, branches []string) bool {
+	for _, b := range branches {
+		remoteSHA, err := git.RevParse(remote + "/" + b)
+		if err != nil {
+			continue
+		}
+		if isAnc, err := git.IsAncestor(remoteSHA, b); err == nil && !isAnc {
+			return true
+		}
+	}
+	return false
+}
+
 // restoreBranches resets each branch to its original SHA, collecting any errors.
 func restoreBranches(originalRefs map[string]string) []string {
 	var errors []string