address review comments

Author: skarim

cmd/checkout.go

@@ -110,7 +110,7 @@ func runCheckout(cfg *config.Config, opts *checkoutOptions) error {
 			return err
 		}
 	} else if prNumber, parseErr := strconv.Atoi(opts.target); parseErr == nil && prNumber > 0 {
-		// Target is a pure integer — try local PR, then remote API, then branch name
+		// Target is a pure integer — try stack number, then PR, then branch name
 		s, targetBranch, err = resolveNumericTarget(cfg, sf, gitDir, prNumber, opts.target)
 		if err != nil {
 			return err
@@ -145,18 +145,23 @@ func runCheckout(cfg *config.Config, opts *checkoutOptions) error {
 	return nil
 }
 
-// resolveNumericTarget handles the case where the user passes a pure integer.
-// The integer is interpreted as, in order:
+// resolveNumericTarget handles the case where the user passes a pure integer or
+// a PR URL. The number is interpreted as, in order:
 //  1. A stack number (the primary identifier)
 //  2. A locally tracked PR number
 //  3. A PR number whose stack is discovered from GitHub
 //  4. A branch name (for numeric branch names like "123")
+//
+// Stack, PR, and issue numbers share a single repo-scoped numberspace,
+// so a given number is only ever one object type; a number that is not a stack
+// simply misses at step 1 and resolves at a later step.
 func resolveNumericTarget(cfg *config.Config, sf *stack.StackFile, gitDir string, number int, raw string) (*stack.Stack, string, error) {
 	// 1. Try as a stack number (the primary identifier).
 	if s, targetBranch, err := checkoutStackByNumber(cfg, sf, gitDir, number); err == nil {
 		return s, targetBranch, nil
 	} else if !errors.Is(err, errStackNumberNotFound) {
-		// A real error (composition conflict, interrupted import, etc.) — surface it.
+		// A real error during import/reconcile (composition conflict, interrupted
+		// import, etc.) — surface it rather than trying other interpretations.
 		return nil, "", err
 	}
 
@@ -254,7 +259,10 @@ var errStackNumberNotFound = errors.New("stack number not found")
 // checkoutStackByNumber discovers a stack from GitHub by its stack number,
 // reconciles it with any local state, and checks out the top-most unmerged
 // branch. It returns errStackNumberNotFound when the number does not resolve to
-// a stack so the caller can fall back to other interpretations.
+// a stack so the caller can fall back to other interpretations. Because stack,
+// PR, and issue numbers share one repo-scoped numberspace, a number that
+// belongs to a PR (or nothing) simply misses here and is resolved by the
+// caller's later steps.
 func checkoutStackByNumber(cfg *config.Config, sf *stack.StackFile, gitDir string, stackNumber int) (*stack.Stack, string, error) {
 	client, err := cfg.GitHubClient()
 	if err != nil {

cmd/checkout_test.go

@@ -354,6 +354,53 @@ func TestCheckout_ByStackNumber(t *testing.T) {
 	assert.Equal(t, 7, sf.Stacks[0].Number)
 }
 
+func TestCheckout_ByStackNumber_404FallsThroughToPR(t *testing.T) {
+	// A 404 from GetStack means no such stack, so the number is tried as a PR.
+	gitDir := t.TempDir()
+	var checkedOut string
+	restore := git.SetOps(&git.MockOps{
+		GitDirFn:              func() (string, error) { return gitDir, nil },
+		CurrentBranchFn:       func() (string, error) { return "main", nil },
+		BranchExistsFn:        func(name string) bool { return name == "main" },
+		FetchFn:               func(string) error { return nil },
+		CreateBranchFn:        func(string, string) error { return nil },
+		SetUpstreamTrackingFn: func(string, string) error { return nil },
+		RevParseFn:            func(string) (string, error) { return "abc123", nil },
+		ResolveRemoteFn:       func(string) (string, error) { return "origin", nil },
+		CheckoutBranchFn: func(name string) error {
+			checkedOut = name
+			return nil
+		},
+	})
+	defer restore()
+
+	writeStackFile(t, gitDir, stack.Stack{})
+
+	cfg, outR, errR := config.NewTestConfig()
+	cfg.GitHubClientOverride = &github.MockClient{
+		GetStackFn: func(int) (*github.RemoteStack, error) {
+			return nil, &api.HTTPError{StatusCode: 404, Message: "Not Found"}
+		},
+		FindStackForPRFn: func(int) (*github.RemoteStack, error) {
+			return &github.RemoteStack{ID: 1, Number: 1, PullRequests: []int{11, 12}}, nil
+		},
+		FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
+			prs := map[int]*github.PullRequest{
+				11: {ID: "PR_11", Number: 11, HeadRefName: "feat-2", BaseRefName: "main", URL: "https://github.com/o/r/pull/11"},
+				12: {ID: "PR_12", Number: 12, HeadRefName: "feat-3", BaseRefName: "feat-2", URL: "https://github.com/o/r/pull/12"},
+			}
+			return prs[n], nil
+		},
+	}
+
+	err := runCheckout(cfg, &checkoutOptions{target: "11"})
+	output := collectOutput(cfg, outR, errR)
+
+	require.NoError(t, err)
+	assert.Equal(t, "feat-2", checkedOut, "the number should resolve as PR #11 after a stack 404")
+	assert.Contains(t, output, "Imported stack with 2 branches")
+}
+
 func TestCheckout_NumericTarget_BranchExistsNoStack(t *testing.T) {
 	gitDir := t.TempDir()
 	var checkedOut string

cmd/unstack_test.go

@@ -483,3 +483,50 @@ func TestUnstack_ByStackNumber_NotTrackedLocally(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, sf.Stacks, 1)
 }
+
+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.
+	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", // legacy: internal ID present, Number unset (0)
+		Trunk: stack.BranchRef{Branch: "main"},
+		Branches: []stack.BranchRef{
+			{Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 101}},
+			{Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 102}},
+		},
+	})
+
+	var unstackedNumber int
+	cfg, outR, errR := config.NewTestConfig()
+	cfg.GitHubClientOverride = &github.MockClient{
+		ListStacksFn: func() ([]github.RemoteStack, error) {
+			return []github.RemoteStack{{ID: 99, Number: 7, PullRequests: []int{101, 102}}}, nil
+		},
+		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
+		},
+	}
+
+	err := runUnstack(cfg, &unstackOptions{stackNumber: 7})
+	output := collectOutput(cfg, outR, errR)
+
+	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")
+
+	// 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")
+}

cmd/utils.go

@@ -260,8 +260,11 @@ func loadStack(cfg *config.Config, branch string) (*loadStackResult, error) {
 }
 
 // loadStackByNumber loads the locally tracked stack whose stack number matches
-// the given value. It prints a helpful error and returns a non-nil error when
-// no local stack has that number.
+// the given value. Stack files created before the number was tracked store only
+// the internal ID (Number == 0); such legacy stacks are resolved by mapping
+// their ID to a remote stack number so they can still be targeted by number. It
+// prints a helpful error and returns a non-nil error when no local stack
+// resolves to that number.
 func loadStackByNumber(cfg *config.Config, number int) (*loadStackResult, error) {
 	gitDir, err := git.GitDir()
 	if err != nil {
@@ -275,6 +278,28 @@ func loadStackByNumber(cfg *config.Config, number int) (*loadStackResult, error)
 		return nil, fmt.Errorf("failed to load stack state: %w", err)
 	}
 
+	// Direct match on the tracked stack number.
+	if result := stackResultByNumber(sf, gitDir, number); result != nil {
+		return result, nil
+	}
+
+	// No direct match — backfill legacy stacks' numbers from the remote and
+	// retry, so `gh stack unstack <number>` also works for stacks tracked
+	// before the number was recorded locally.
+	if backfillLegacyStackNumbers(cfg, sf, gitDir) {
+		if result := stackResultByNumber(sf, gitDir, number); result != nil {
+			return result, nil
+		}
+	}
+
+	cfg.Errorf("stack #%d is not tracked locally", number)
+	cfg.Printf("Run `%s` to check it out first", cfg.ColorCyan(fmt.Sprintf("gh stack checkout %d", number)))
+	return nil, fmt.Errorf("stack #%d is not tracked locally", number)
+}
+
+// stackResultByNumber returns a loadStackResult for the locally tracked stack
+// whose Number matches, or nil when none does.
+func stackResultByNumber(sf *stack.StackFile, gitDir string, number int) *loadStackResult {
 	for i := range sf.Stacks {
 		if sf.Stacks[i].Number == number {
 			currentBranch, _ := git.CurrentBranch()
@@ -283,13 +308,59 @@ func loadStackByNumber(cfg *config.Config, number int) (*loadStackResult, error)
 				StackFile:     sf,
 				Stack:         &sf.Stacks[i],
 				CurrentBranch: currentBranch,
-			}, nil
+			}
 		}
 	}
+	return nil
+}
 
-	cfg.Errorf("stack #%d is not tracked locally", number)
-	cfg.Printf("Run `%s` to check it out first", cfg.ColorCyan(fmt.Sprintf("gh stack checkout %d", number)))
-	return nil, fmt.Errorf("stack #%d is not tracked locally", number)
+// backfillLegacyStackNumbers fills in the human-facing Number for locally
+// tracked stacks that predate it (Number == 0 but ID set) by mapping their
+// internal ID to the remote stack list, persisting any updates. Returns true
+// when at least one number was filled in. Best-effort: returns false on any
+// client or API error rather than failing the caller.
+func backfillLegacyStackNumbers(cfg *config.Config, sf *stack.StackFile, gitDir string) bool {
+	needsResolve := false
+	for i := range sf.Stacks {
+		if sf.Stacks[i].Number == 0 && sf.Stacks[i].ID != "" {
+			needsResolve = true
+			break
+		}
+	}
+	if !needsResolve {
+		return false
+	}
+
+	client, err := cfg.GitHubClient()
+	if err != nil {
+		return false
+	}
+	stacks, err := client.ListStacks()
+	if err != nil {
+		return false
+	}
+	numberByID := make(map[string]int, len(stacks))
+	for _, rs := range stacks {
+		numberByID[strconv.Itoa(rs.ID)] = rs.Number
+	}
+
+	changed := false
+	for i := range sf.Stacks {
+		if sf.Stacks[i].Number != 0 || sf.Stacks[i].ID == "" {
+			continue
+		}
+		if n, ok := numberByID[sf.Stacks[i].ID]; ok && n != 0 {
+			sf.Stacks[i].Number = n
+			changed = true
+		}
+	}
+	if changed {
+		if err := stack.Save(gitDir, sf); err != nil {
+			// Non-fatal: the in-memory backfill still lets us resolve the target.
+			cfg.Warningf("could not persist stack numbers: %v", err)
+		}
+	}
+	return changed
 }
 
 // handleSaveError translates a stack.Save error into the appropriate user
@@ -608,6 +679,12 @@ func syncStackPRsFromRemote(client github.ClientOps, s *stack.Stack) (map[string
 	for _, rs := range stacks {
 		if strconv.Itoa(rs.ID) == s.ID {
 			remotePRNumbers = rs.PRNumbers()
+			// Backfill the human-facing stack number for stack files created
+			// before it was tracked, so callers (view, submit TUI) can display
+			// it. Persisted by whichever command later saves the stack file.
+			if s.Number == 0 {
+				s.Number = rs.Number
+			}
 			break
 		}
 	}

cmd/utils_test.go

@@ -540,6 +540,43 @@ func TestSyncStackPRs_RemoteStack_UsesStackAPI(t *testing.T) {
 	assert.True(t, s.Branches[1].PullRequest.Merged)
 }
 
+func TestSyncStackPRs_BackfillsStackNumber(t *testing.T) {
+	// A stack tracked before the number was recorded (Number == 0) gets its
+	// number backfilled from the remote during the shared sync, so callers can
+	// display it.
+	s := &stack.Stack{
+		ID:    "100", // legacy: Number unset
+		Trunk: stack.BranchRef{Branch: "main"},
+		Branches: []stack.BranchRef{
+			{Branch: "b1"},
+			{Branch: "b2"},
+		},
+	}
+
+	cfg, outR, errR := config.NewTestConfig()
+	cfg.GitHubClientOverride = &github.MockClient{
+		ListStacksFn: func() ([]github.RemoteStack, error) {
+			return []github.RemoteStack{
+				{ID: 100, Number: 5, PullRequests: []int{10, 11}},
+			}, nil
+		},
+		FindPRByNumberFn: func(number int) (*github.PullRequest, error) {
+			switch number {
+			case 10:
+				return &github.PullRequest{Number: 10, HeadRefName: "b1", State: "OPEN"}, nil
+			case 11:
+				return &github.PullRequest{Number: 11, HeadRefName: "b2", State: "OPEN"}, nil
+			}
+			return nil, nil
+		},
+	}
+
+	_ = syncStackPRs(cfg, s)
+	collectOutput(cfg, outR, errR)
+
+	assert.Equal(t, 5, s.Number, "the stack number should be backfilled from the remote")
+}
+
 func TestSyncStackPRs_RemoteStack_ClosedPRStaysAssociated(t *testing.T) {
 	// When using the stack API, a closed (not merged) PR should remain
 	// associated — the stack API is the source of truth, not PR state.