Simplify sync reconciliation: reuse syncStack instead of a parallel path

Review feedback noted the change felt heavier than the fix warranted.
The weight came from `syncRemoteStack` (cmd/sync.go), a near-duplicate of
submit's `syncStack` — same <2-PR guard, ListStacks, and update/create
dispatch — that existed only to add an "already up to date" short-circuit.
That one optimization is what spawned the second entry point, the
pre-fetched-list threading, and the double-ListStacks it then required.

Collapse it to a single reconciliation path:

- Remove `syncRemoteStack`; `gh stack sync` now calls the shared
  `syncStack` directly. One path, one ListStacks per sync.
- Fold `createNewStack` into `reconcileUntrackedStack` (renamed from
  `adoptRemoteStack`) so it returns a single `synced bool` instead of a
  `(handled, synced)` tuple and owns its own ListStacks again.
- Inline `stackPRNumbers` back into `syncStack` (it was only extracted to
  share with the now-removed `syncRemoteStack`).
- Drop the now-unused `strconv`/`github` imports from cmd/sync.go.

Behavior note: a routine re-sync of an already-tracked stack now prints
"Stack updated on GitHub with N PRs" instead of "Stack already up to date
on GitHub". This is accurate (sync does PUT the current state) and matches
submit. The "Stack synced" / "Branches synced" summary is unchanged, and
submit's behavior is unchanged.

Author: skarim

cmd/submit.go

@@ -684,19 +684,6 @@ func clearPendingModifyState(cfg *config.Config, gitDir string) {
 	cfg.Successf("Stack recreated on GitHub to match local state")
 }
 
-// stackPRNumbers returns the PR numbers for a stack in order (bottom to top),
-// including merged PRs. The stacks API expects the full list — omitting merged
-// PRs causes a "Stack contents have changed" rejection.
-func stackPRNumbers(s *stack.Stack) []int {
-	var prNumbers []int
-	for _, b := range s.Branches {
-		if b.PullRequest != nil {
-			prNumbers = append(prNumbers, b.PullRequest.Number)
-		}
-	}
-	return prNumbers
-}
-
 // syncStack creates or updates a stack on GitHub from the active PRs.
 // If the stack already exists (s.ID is set), it calls the PUT endpoint with
 // the full list of PRs to keep the remote stack in sync. If no stack exists
@@ -708,7 +695,15 @@ func stackPRNumbers(s *stack.Stack) []int {
 // (created, updated, or already in sync) and false otherwise (fewer than two
 // PRs, an unresolved divergence, stacked PRs unavailable, or an API failure).
 func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool {
-	prNumbers := stackPRNumbers(s)
+	// Collect PR numbers in stack order (bottom to top), including merged PRs.
+	// The API expects the full list — omitting merged PRs causes a
+	// "Stack contents have changed" rejection.
+	var prNumbers []int
+	for _, b := range s.Branches {
+		if b.PullRequest != nil {
+			prNumbers = append(prNumbers, b.PullRequest.Number)
+		}
+	}
 
 	// The API requires at least 2 PRs to form a stack.
 	if len(prNumbers) < 2 {
@@ -721,32 +716,27 @@ func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool
 
 	// No locally tracked stack ID. The stack may already exist on GitHub
 	// (created from the web UI or another clone) without being recorded
-	// locally. Inspect the remote stacks and adopt a match instead of blindly
-	// creating a new one, which the API rejects because the PRs are already
-	// part of a stack.
+	// locally. Adopt it instead of blindly creating a new one, which the API
+	// rejects because the PRs are already part of a stack.
+	return reconcileUntrackedStack(cfg, client, s, prNumbers)
+}
+
+// reconcileUntrackedStack reconciles a locally untracked stack (s.ID == "")
+// with the stacks that already exist on GitHub. The PRs in s may already belong
+// to a remote stack created from the web UI or another clone; in that case we
+// adopt that stack rather than POST a new one (which the API rejects because the
+// PRs are already stacked). It creates a new stack when none match, refuses to
+// modify a divergent or PR-dropping stack, adopts a matching stack, or updates a
+// partially-formed one. It returns true when the remote stack object now
+// reflects the local stack.
+func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool {
 	stacks, err := client.ListStacks()
 	if err != nil {
 		// Couldn't inspect remote state — fall back to the create path, which
 		// reports its own errors (handleCreate422 covers "already stacked").
 		return createNewStack(cfg, client, s, prNumbers)
 	}
 
-	return reconcileUntrackedStack(cfg, client, s, prNumbers, stacks)
-}
-
-// reconcileUntrackedStack reconciles a locally untracked stack (s.ID == "")
-// against the already-fetched remote stacks. The PRs in s may already belong to
-// a remote stack created from the web UI or another clone; in that case we adopt
-// that stack rather than POST a new one (which the API rejects because the PRs
-// are already stacked). It creates a new stack when none match, refuses to
-// modify a divergent or PR-dropping stack, adopts a matching stack, or updates a
-// partially-formed one.
-//
-// Callers pass the pre-fetched stack list so a single ListStacks round-trip is
-// shared across the reconciliation flow (sync fetches the list once for its
-// already-up-to-date short-circuit and reuses it here). It returns true when the
-// remote stack object now reflects the local stack.
-func reconcileUntrackedStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int, stacks []github.RemoteStack) bool {
 	matched, err := findMatchingStack(stacks, prNumbers)
 	if err != nil {
 		// Our PRs are spread across more than one remote stack. A PR can only

cmd/sync.go

@@ -3,13 +3,11 @@ package cmd
 import (
 	"errors"
 	"fmt"
-	"strconv"
 	"strings"
 
 	"github.com/cli/go-gh/v2/pkg/prompter"
 	"github.com/github/gh-stack/internal/config"
 	"github.com/github/gh-stack/internal/git"
-	"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"
@@ -240,7 +238,7 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	// summary message below.
 	stackSynced := false
 	if client, err := cfg.GitHubClient(); err == nil {
-		stackSynced = syncRemoteStack(cfg, client, s)
+		stackSynced = syncStack(cfg, client, s)
 	}
 
 	// --- Step 6: Prune merged branches (optional) ---
@@ -350,52 +348,6 @@ func runSync(cfg *config.Config, opts *syncOptions) error {
 	return nil
 }
 
-// syncRemoteStack reconciles the stack object on GitHub with the local stack's
-// open PRs. It only links existing PRs into a stack — it never opens PRs (use
-// `gh stack submit` for that). It returns true when the remote stack object now
-// reflects the local stack (created, updated, adopted, or already in sync), and
-// false when there is nothing to sync or the remote stack could not be
-// reconciled (fewer than two PRs, stacked PRs unavailable, a divergence across
-// multiple stacks, or an API failure).
-//
-// A stack on GitHub requires at least two open PRs, so a single-PR or PR-less
-// stack reconciles to false and the caller reports only the branches as synced.
-func syncRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool {
-	prNumbers := stackPRNumbers(s)
-	if len(prNumbers) < 2 {
-		return false
-	}
-
-	// Inspect the remote stacks once so a routine sync that has not changed the
-	// PR membership does not issue a redundant — and misleading — update, and so
-	// the create/adopt path can reuse the same list (one ListStacks per sync).
-	stacks, err := client.ListStacks()
-	if err != nil {
-		// Couldn't inspect remote state; attempt a direct create/update and let
-		// those helpers surface their own availability/PAT/create errors.
-		if s.ID != "" {
-			return updateStack(cfg, client, s, prNumbers)
-		}
-		return createNewStack(cfg, client, s, prNumbers)
-	}
-
-	if matched, mErr := findMatchingStack(stacks, prNumbers); mErr == nil &&
-		matched != nil && slicesEqual(matched.PullRequests, prNumbers) {
-		// The remote stack already lists exactly these PRs — record its ID so
-		// future operations stay cheap and report it as in sync.
-		s.ID = strconv.Itoa(matched.ID)
-		cfg.Successf("Stack already up to date on GitHub")
-		return true
-	}
-
-	// Membership differs (or no stack exists yet). Reuse the list we already
-	// fetched for the create/adopt/update path.
-	if s.ID != "" {
-		return updateStack(cfg, client, s, prNumbers)
-	}
-	return reconcileUntrackedStack(cfg, client, s, prNumbers, stacks)
-}
-
 // restoreBranches resets each branch to its original SHA, collecting any errors.
 func restoreBranches(originalRefs map[string]string) []string {
 	var errors []string

cmd/sync_test.go

@@ -1733,7 +1733,7 @@ func TestSync_AdoptsExistingEqualRemoteStack(t *testing.T) {
 
 	output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock)
 
-	assert.Contains(t, output, "Stack already up to date on GitHub")
+	assert.Contains(t, output, "already up to date")
 	assert.Contains(t, output, "Stack synced")
 	assert.NotContains(t, output, "Branches synced")
 

skills/gh-stack/SKILL.md

@@ -643,7 +643,7 @@ gh stack sync [flags]
 - `✓ Pushed N branches`
 - `✓ PR #N (<branch>) — Open` per branch
 - `Merged: #N, #M` for merged branches
-- `✓ Stack created on GitHub with N PRs` / `✓ Stack updated on GitHub with N PRs` / `✓ Stack already up to date on GitHub` (when two or more PRs exist)
+- `✓ Stack created on GitHub with N PRs` / `✓ Stack updated on GitHub with N PRs` / `✓ Linked to the existing stack on GitHub` (when two or more PRs exist)
 - `✓ Pruned <branch> (merged)` per pruned branch (when pruning)
 - `✓ Stack synced` when the stack object on GitHub was created/updated to match local, or `✓ Branches synced` when only the branches were synced (fewer than two PRs, stacked PRs unavailable, or a divergence)