From c3653a20c611469e80ccfb57b99291f9a4e2b73a Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 13:22:19 -0400 Subject: [PATCH 1/4] Create/update the remote stack on sync and fix false "Stack synced" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `gh stack sync` reported "Stack synced" even when it had not created or updated the stack object on GitHub. After running `gh stack init` to adopt existing branches and then opening PRs outside the CLI, `gh stack sync` detected the open PRs and printed "Stack synced" — but no stack had ever been created on the server. There were two distinct bugs: 1. Sync never reconciled the remote stack object. `runSync` called `syncStackPRs`, which only *reads* PR state and links PRs to local branches; it never called the create/update path. So the branches were rebased and pushed and the PRs were detected, but the stack on GitHub was never created. 2. The final message was unconditional. `runSync` always printed "Stack synced", which is supposed to mean "the stack object on GitHub now reflects the local stack" — something that can only be true when two or more open PRs exist and the remote stack was actually created/updated. Fix Reconcile the remote stack from sync, and make the closing message reflect what actually happened. * cmd/sync.go - Add a reconciliation step (5b) after PR-state sync: when the stack has two or more open PRs, link them into a stack on GitHub via the new `syncRemoteStack` helper. It inspects existing stacks first and: - short-circuits quietly when a remote stack already lists exactly these PRs (records the ID, prints "Stack already up to date on GitHub") so routine syncs don't issue a redundant, misleading update; - otherwise delegates to `syncStack` to create a new stack, adopt an untracked one, or update a partially-formed one. Sync never opens PRs — that remains `gh stack submit`'s job. - Replace the unconditional "Stack synced" with a result-driven message: "Stack synced" when the remote stack object was created/updated/in sync, otherwise "Branches synced" (fewer than two PRs, stacked PRs unavailable, a cross-stack divergence, or no GitHub client). - Update the command's long description to document the stack-object step and the two possible closing messages. * cmd/submit.go - Thread a `synced bool` return through the existing, tested stack helpers so sync can tell whether the remote stack object now matches local: `syncStack`, `createNewStack`, and `updateStack` now return `bool`; `adoptRemoteStack` returns `(handled, synced)`; and `handleCreate422` returns `bool` (true only when the PRs are already stacked together). Extract the shared `stackPRNumbers` helper. - This is additive: submit's single call site ignores the new return value, so submit's behavior, output, and tests are unchanged. Reusing these helpers (instead of duplicating the 404/422 handling in sync) keeps the create/adopt/update logic in one tested place. Tests * cmd/sync_test.go — six new cases covering the reconciliation matrix: - TestSync_CreatesRemoteStackWhenPRsExist: open PRs but no remote stack -> CreateStack is called and the new ID is persisted to the stack file; output contains "Stack created on GitHub" and "Stack synced". - TestSync_AdoptsExistingEqualRemoteStack: a matching remote stack -> no create/update, ID recorded, "Stack synced". - TestSync_UpdatesPartialRemoteStack: a subset stack -> UpdateStack with the full PR list, "Stack synced". - TestSync_FewerThanTwoPRs_BranchesSynced: one PR -> no stack API calls, "Branches synced", not "Stack synced". - TestSync_StacksUnavailable_BranchesSynced: 404 on create -> warns, "Branches synced". - TestSync_PRsSpanMultipleStacks_BranchesSynced: PRs across two stacks -> divergence warning, no create/update, "Branches synced". Docs Document the new stack-object step and the "Stack synced" vs "Branches synced" distinction in: - README.md - docs/src/content/docs/reference/cli.md - skills/gh-stack/SKILL.md - docs/src/content/docs/introduction/overview.md - docs/src/content/docs/guides/stacked-prs.md - docs/src/content/docs/guides/workflows.md --- README.md | 3 +- cmd/submit.go | 97 ++++--- cmd/sync.go | 70 ++++- cmd/sync_test.go | 263 ++++++++++++++++++ docs/src/content/docs/guides/stacked-prs.md | 2 +- docs/src/content/docs/guides/workflows.md | 3 +- .../src/content/docs/introduction/overview.md | 2 +- docs/src/content/docs/reference/cli.md | 3 +- skills/gh-stack/SKILL.md | 6 +- 9 files changed, 401 insertions(+), 48 deletions(-) diff --git a/README.md b/README.md index 35b8494..4e63dc4 100644 --- a/README.md +++ b/README.md @@ -323,7 +323,8 @@ Performs a safe, non-interactive synchronization of the entire stack: 3. **Cascade rebase** — rebases all stack branches onto their updated parents (only if trunk moved). If a conflict is detected, all branches are restored to their original state and you are advised to run `gh stack rebase` to resolve conflicts interactively 4. **Push** — pushes all branches (uses `--force-with-lease` if a rebase occurred) 5. **Sync PRs** — syncs PR state from GitHub and reports the status of each PR -6. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to prune automatically +6. **Sync the stack** — links the stack's open PRs into a stack on GitHub, creating the remote stack object if it doesn't exist yet or updating it if it's partially formed. Only happens when two or more PRs exist; sync never opens PRs (use `gh stack submit` for that) +7. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to prune automatically | Flag | Description | |------|-------------| diff --git a/cmd/submit.go b/cmd/submit.go index 7daf067..810f6ce 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -684,42 +684,50 @@ func clearPendingModifyState(cfg *config.Config, gitDir string) { cfg.Successf("Stack recreated on GitHub to match local state") } -// 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 -// yet, it calls POST to create one. -// This is a best-effort operation: failures are reported as warnings but do -// not cause the submit command to fail (the PRs are already created). -func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) { - // 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. +// 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 +// yet, it calls POST to create one. +// This is a best-effort operation: failures are reported as warnings but do +// not cause the submit command to fail (the PRs are already created). +// +// It returns true when the remote stack object reflects the local stack +// (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) // The API requires at least 2 PRs to form a stack. if len(prNumbers) < 2 { - return + return false } if s.ID != "" { - updateStack(cfg, client, s, prNumbers) - return + return updateStack(cfg, client, s, prNumbers) } // No locally tracked stack ID. The stack may already exist on GitHub // (created from the web UI or another clone) without being recorded // locally. Adopt it instead of blindly creating a new one, which the API // rejects because the PRs are already part of a stack. - if adoptRemoteStack(cfg, client, s, prNumbers) { - return + if handled, synced := adoptRemoteStack(cfg, client, s, prNumbers); handled { + return synced } - createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s, prNumbers) } // adoptRemoteStack reconciles a locally untracked stack (s.ID == "") with the @@ -728,16 +736,17 @@ func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) { // adopt that stack rather than POST a new one (which the API rejects because // the PRs are already stacked). // -// It returns true when it has fully handled the sync — either by adopting and -// updating the existing stack, or by intentionally refusing to modify a -// divergent remote stack — and false when no matching remote stack exists and -// the caller should create a new one. -func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool { +// It returns (handled, synced). handled is true when it has fully handled the +// sync — either by adopting and updating the existing stack, or by +// intentionally refusing to modify a divergent remote stack — and false when no +// matching remote stack exists and the caller should create a new one. synced +// is true only when the remote stack object now reflects the local stack. +func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) (bool, 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 false + return false, false } matched, err := findMatchingStack(stacks, prNumbers) @@ -748,12 +757,12 @@ func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stac cfg.Warningf("Your PRs belong to multiple stacks on GitHub — reconcile them before submitting") cfg.Printf(" Run `%s` to import a stack, or unstack the PRs from the web", cfg.ColorCyan("gh stack checkout ")) - return true + return true, false } if matched == nil { // No existing stack contains any of our PRs — create a new one. - return false + return false, false } // A remote stack already contains some of our PRs. Refuse to silently drop @@ -763,7 +772,7 @@ func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stac formatPRList(dropped), plural(len(dropped), "is", "are")) cfg.Printf(" Run `%s` to import the full stack, then `%s`", cfg.ColorCyan("gh stack checkout "), cfg.ColorCyan("gh stack submit")) - return true + return true, false } // Every PR in the remote stack is tracked locally (and we may have added @@ -773,12 +782,11 @@ func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stac if slicesEqual(matched.PullRequests, prNumbers) { cfg.Successf("Linked to the existing stack on GitHub (%d PRs, already up to date)", len(prNumbers)) - return true + return true, true } cfg.Infof("Found the stack on GitHub — updating it to match your local stack") - updateStack(cfg, client, s, prNumbers) - return true + return true, updateStack(cfg, client, s, prNumbers) } // prsMissingFrom returns the numbers in remote that do not appear in local, @@ -800,7 +808,8 @@ func prsMissingFrom(remote, local []int) []int { // updateStack calls the PUT endpoint to sync the full PR list for an existing stack. // If the remote stack was deleted (404), it clears the local ID and falls through // to createNewStack so the user doesn't need to re-run the command. -func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { +// Returns true when the remote stack was updated (or recreated) successfully. +func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool { if err := client.UpdateStack(s.ID, prNumbers); err != nil { var httpErr *api.HTTPError if errors.As(err, &httpErr) { @@ -809,7 +818,7 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // Stack was deleted on GitHub — clear the stale ID and // immediately try to re-create it. s.ID = "" - createNewStack(cfg, client, s, prNumbers) + return createNewStack(cfg, client, s, prNumbers) case 422: // A merged branch whose ref has been deleted upstream breaks the // stack's base→head chain, so the update is rejected. This is @@ -818,7 +827,7 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr // than alarming the user with a raw API error. if strings.Contains(httpErr.Message, "must form a stack") && len(s.MergedBranches()) > 0 { cfg.Infof("Merged PRs have left the stack on GitHub, so it wasn't updated — your unmerged PRs were pushed and re-based onto the trunk") - return + return false } cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message) default: @@ -827,34 +836,38 @@ func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, pr } else { cfg.Warningf("Failed to update stack on GitHub: %v", err) } - return + return false } cfg.Successf("Stack updated on GitHub with %d PRs", len(prNumbers)) + return true } // createNewStack calls the POST endpoint to create a new stack, handling the // three types of 422 errors the API may return. -func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { +// Returns true when the stack was created or is confirmed already in sync. +func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) bool { stackID, err := client.CreateStack(prNumbers) if err == nil { s.ID = strconv.Itoa(stackID) cfg.Successf("Stack created on GitHub with %d PRs", len(prNumbers)) - return + return true } var httpErr *api.HTTPError if !errors.As(err, &httpErr) { cfg.Warningf("Failed to create stack on GitHub: %v", err) - return + return false } switch httpErr.StatusCode { case 422: - handleCreate422(cfg, httpErr, prNumbers) + return handleCreate422(cfg, httpErr, prNumbers) case 404: warnStacksUnavailableOrPAT(cfg) + return false default: cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message) + return false } } @@ -863,7 +876,10 @@ func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, // - "Stack must contain at least two pull requests" // - "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref" // - "Pull requests #123, #124, #125 are already stacked" -func handleCreate422(cfg *config.Config, httpErr *api.HTTPError, prNumbers []int) { +// +// Returns true only when the PRs are already stacked together (i.e. the remote +// stack already matches), which counts as in sync. +func handleCreate422(cfg *config.Config, httpErr *api.HTTPError, prNumbers []int) bool { msg := httpErr.Message if isAlreadyStackedError(msg) { @@ -872,22 +888,23 @@ func handleCreate422(cfg *config.Config, httpErr *api.HTTPError, prNumbers []int // If only a subset matches, the PRs are in a different stack. if allPRsInMessage(msg, prNumbers) { cfg.Successf("Stack with %d PRs is up to date", len(prNumbers)) - return + return true } cfg.Warningf("One or more PRs are already part of a different stack on GitHub") cfg.Printf(" Run `%s` to import the existing stack, or unstack the PRs from the web", cfg.ColorCyan("gh stack checkout ")) - return + return false } if strings.Contains(msg, "must form a stack") { cfg.Warningf("Cannot create stack: %s", msg) cfg.Printf(" Each PR's base branch must match the previous PR's head branch.") - return + return false } // "at least two" or any other validation error cfg.Warningf("Could not create stack: %s", msg) + return false } // allPRsInMessage checks whether every PR number in prNumbers appears diff --git a/cmd/sync.go b/cmd/sync.go index d09f550..0dfc781 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -3,11 +3,13 @@ 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" @@ -33,11 +35,20 @@ This command performs a safe, non-interactive synchronization: 3. Cascade-rebases stack branches onto their updated parents 4. Pushes all branches atomically (using --force-with-lease --atomic) 5. Syncs PR state from GitHub + 6. Links the stack's open PRs into a stack on GitHub (creating or updating + the remote stack object) when two or more PRs exist If a rebase conflict is detected, all branches are restored to their original state and you are advised to run "gh stack rebase" to resolve conflicts interactively. +Sync never opens pull requests — use "gh stack submit" for that. It only +links PRs that already exist. The final message reflects what happened: +"Stack synced" means the stack object on GitHub now matches your local +stack, while "Branches synced" means the branches were rebased and pushed +but no remote stack object was created or updated (for example, when fewer +than two PRs exist yet). + Use --prune to delete local branches for merged PRs. Stack metadata is preserved so that rebase and display logic continue to work correctly. If you are on a branch that would be pruned, your checkout is moved to @@ -220,6 +231,18 @@ func runSync(cfg *config.Config, opts *syncOptions) error { cfg.Printf("Merged: %s", strings.Join(names, ", ")) } + // --- Step 5b: Reconcile the remote stack object --- + // syncStackPRs above only refreshes local PR associations; it does not touch + // the stack object on GitHub. When the branches have open PRs, link them into + // a stack so the remote reflects the local stack. This never opens PRs — that + // is still `gh stack submit`'s job. stackSynced records whether the remote + // stack object actually reflects the local stack, which determines the final + // summary message below. + stackSynced := false + if client, err := cfg.GitHubClient(); err == nil { + stackSynced = syncRemoteStack(cfg, client, s) + } + // --- Step 6: Prune merged branches (optional) --- doPrune := opts.prune if !doPrune { @@ -316,10 +339,55 @@ func runSync(cfg *config.Config, opts *syncOptions) error { } cfg.Printf("") - cfg.Successf("Stack synced") + if stackSynced { + cfg.Successf("Stack synced") + } else { + // 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. + cfg.Successf("Branches synced") + } 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 first so a routine sync that has not changed the + // PR membership does not issue a redundant — and misleading — update. + stacks, err := client.ListStacks() + if err != nil { + // Couldn't inspect remote state; let syncStack attempt the operation and + // surface its own availability/PAT/create errors. + return syncStack(cfg, client, s) + } + + 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): create, adopt, or update. + return syncStack(cfg, client, s) +} + // restoreBranches resets each branch to its original SHA, collecting any errors. func restoreBranches(originalRefs map[string]string) []string { var errors []string diff --git a/cmd/sync_test.go b/cmd/sync_test.go index be43f6b..452c8c6 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -6,8 +6,10 @@ import ( "strings" "testing" + "github.com/cli/go-gh/v2/pkg/api" "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/stack" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -1603,3 +1605,264 @@ func TestSync_ExplicitPrune_SkipsPrompt(t *testing.T) { assert.NoError(t, err) assert.Equal(t, []string{"b1"}, deletedBranches) } + +// --- Remote stack object reconciliation ------------------------------------- + +// newSyncMockNoRebase returns a sync git mock whose trunk is already up to date, +// so no fast-forward or cascade rebase occurs and the run reaches the remote +// stack reconciliation step cleanly. +func newSyncMockNoRebase(tmpDir, currentBranch string) *git.MockOps { + m := newSyncMock(tmpDir, currentBranch) + m.RevParseFn = func(ref string) (string, error) { + if ref == "main" || ref == "origin/main" { + return "trunk-sha", nil + } + if strings.HasPrefix(ref, "origin/") { + return "sha-" + strings.TrimPrefix(ref, "origin/"), nil + } + return "sha-" + ref, nil + } + return m +} + +// openPRFinder returns a FindPRForBranch func that reports OPEN PRs for the +// branches in prFor (branch name -> PR number) and nil for any other branch. +func openPRFinder(prFor map[string]int) func(string) (*github.PullRequest, error) { + return func(branch string) (*github.PullRequest, error) { + n, ok := prFor[branch] + if !ok { + return nil, nil + } + return &github.PullRequest{ + Number: n, + State: "OPEN", + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n), + HeadRefName: branch, + }, nil + } +} + +// runSyncWithGitHub executes sync against tmpDir using the supplied git and +// GitHub mocks and returns the captured stderr output. +func runSyncWithGitHub(t *testing.T, gitMock *git.MockOps, ghMock *github.MockClient) string { + t.Helper() + restore := git.SetOps(gitMock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = ghMock + cmd := SyncCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + require.NoError(t, cmd.Execute()) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + return string(errOut) +} + +// TestSync_CreatesRemoteStackWhenPRsExist verifies the core fix: when the +// branches already have open PRs but no stack exists on GitHub, sync creates the +// stack object and reports "Stack synced". +func TestSync_CreatesRemoteStackWhenPRsExist(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var createdWith []int + ghMock := &github.MockClient{ + FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), + ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, + CreateStackFn: func(prNumbers []int) (int, error) { + createdWith = prNumbers + return 7, nil + }, + UpdateStackFn: func(string, []int) error { + t.Fatal("UpdateStack should not be called when no remote stack exists") + return nil + }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) + + assert.Equal(t, []int{101, 102}, createdWith, "should create the stack from both PR numbers") + assert.Contains(t, output, "Stack created on GitHub with 2 PRs") + assert.Contains(t, output, "Stack synced") + assert.NotContains(t, output, "Branches synced") + + // The new remote stack ID must be persisted to the stack file. + sf, err := stack.Load(tmpDir) + require.NoError(t, err) + require.Len(t, sf.Stacks, 1) + assert.Equal(t, "7", sf.Stacks[0].ID) +} + +// TestSync_AdoptsExistingEqualRemoteStack verifies that when a remote stack +// already lists exactly the local PRs, sync records its ID without issuing a +// redundant create/update and still reports "Stack synced". +func TestSync_AdoptsExistingEqualRemoteStack(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + ghMock := &github.MockClient{ + FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102}}}, nil + }, + CreateStackFn: func([]int) (int, error) { + t.Fatal("CreateStack should not be called when the remote stack matches") + return 0, nil + }, + UpdateStackFn: func(string, []int) error { + t.Fatal("UpdateStack should not be called when the remote stack matches") + return nil + }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) + + assert.Contains(t, output, "Stack already up to date on GitHub") + assert.Contains(t, output, "Stack synced") + assert.NotContains(t, output, "Branches synced") + + sf, err := stack.Load(tmpDir) + require.NoError(t, err) + assert.Equal(t, "9", sf.Stacks[0].ID, "should record the adopted stack ID") +} + +// TestSync_UpdatesPartialRemoteStack verifies that when a remote stack contains +// only some of the local PRs, sync updates it with the full list. +func TestSync_UpdatesPartialRemoteStack(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}, {Branch: "b3"}}, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var updatedID string + var updatedWith []int + ghMock := &github.MockClient{ + FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102, "b3": 103}), + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{{ID: 9, PullRequests: []int{101, 102}}}, nil + }, + CreateStackFn: func([]int) (int, error) { + t.Fatal("CreateStack should not be called when a matching stack exists") + return 0, nil + }, + UpdateStackFn: func(stackID string, prNumbers []int) error { + updatedID = stackID + updatedWith = prNumbers + return nil + }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) + + assert.Equal(t, "9", updatedID) + assert.Equal(t, []int{101, 102, 103}, updatedWith) + assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") + assert.Contains(t, output, "Stack synced") + assert.NotContains(t, output, "Branches synced") + + sf, err := stack.Load(tmpDir) + require.NoError(t, err) + assert.Equal(t, "9", sf.Stacks[0].ID) +} + +// TestSync_FewerThanTwoPRs_BranchesSynced verifies that with only one open PR +// (no stack is possible), sync skips all stack API calls and reports +// "Branches synced" rather than "Stack synced". +func TestSync_FewerThanTwoPRs_BranchesSynced(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var listCalled, createCalled bool + ghMock := &github.MockClient{ + FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101}), // b2 has no PR + ListStacksFn: func() ([]github.RemoteStack, error) { + listCalled = true + return nil, nil + }, + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) + + assert.False(t, listCalled, "ListStacks should not be called with fewer than two PRs") + assert.False(t, createCalled, "CreateStack should not be called with fewer than two PRs") + assert.Contains(t, output, "Branches synced") + assert.NotContains(t, output, "Stack synced") +} + +// TestSync_StacksUnavailable_BranchesSynced verifies that when the stacks API +// is unavailable (404 on create), sync warns and reports "Branches synced". +func TestSync_StacksUnavailable_BranchesSynced(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + ghMock := &github.MockClient{ + FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), + ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{StatusCode: 404, Message: "Not Found"} + }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) + + assert.Contains(t, output, "Branches synced") + assert.NotContains(t, output, "Stack synced") +} + +// TestSync_PRsSpanMultipleStacks_BranchesSynced verifies that when the local +// PRs belong to more than one remote stack, sync refuses to auto-resolve the +// divergence and reports "Branches synced". +func TestSync_PRsSpanMultipleStacks_BranchesSynced(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, + } + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var createCalled, updateCalled bool + ghMock := &github.MockClient{ + FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{ + {ID: 9, PullRequests: []int{101}}, + {ID: 10, PullRequests: []int{102}}, + }, nil + }, + CreateStackFn: func([]int) (int, error) { createCalled = true; return 0, nil }, + UpdateStackFn: func(string, []int) error { updateCalled = true; return nil }, + } + + output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) + + assert.False(t, createCalled, "CreateStack should not be called on divergence") + assert.False(t, updateCalled, "UpdateStack should not be called on divergence") + assert.Contains(t, output, "multiple stacks") + assert.Contains(t, output, "Branches synced") + assert.NotContains(t, output, "Stack synced") +} diff --git a/docs/src/content/docs/guides/stacked-prs.md b/docs/src/content/docs/guides/stacked-prs.md index 81e8eac..23fe269 100644 --- a/docs/src/content/docs/guides/stacked-prs.md +++ b/docs/src/content/docs/guides/stacked-prs.md @@ -51,4 +51,4 @@ gh stack sync - **`gh stack push`** pushes branches only (uses `--force-with-lease` for safety). It does not create or update PRs. - **`gh stack submit`** pushes branches and creates or updates PRs, linking them as a Stack on GitHub. -- **`gh stack sync`** is the all-in-one command: fetch, rebase, push, sync PR state, and optionally prune local branches for merged PRs. +- **`gh stack sync`** is the all-in-one command: fetch, rebase, push, sync PR state, link open PRs into a Stack on GitHub, and optionally prune local branches for merged PRs. diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index 6f4c0b2..65cd0be 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -134,7 +134,8 @@ This command: 3. Rebases all remaining stack branches onto the updated trunk 4. Pushes the updated branches 5. Syncs PR state from GitHub -6. Prompts to prune local branches for merged PRs (use `--prune` to prune automatically) +6. Links the open PRs into a Stack on GitHub (creating or updating the remote stack when two or more PRs exist) +7. Prompts to prune local branches for merged PRs (use `--prune` to prune automatically) If a conflict is detected during the rebase, all branches are restored to their original state, and you're advised to run `gh stack rebase` to resolve conflicts interactively. diff --git a/docs/src/content/docs/introduction/overview.md b/docs/src/content/docs/introduction/overview.md index 46a70e5..f3cb83a 100644 --- a/docs/src/content/docs/introduction/overview.md +++ b/docs/src/content/docs/introduction/overview.md @@ -88,7 +88,7 @@ While the PR UI provides the review and merge experience, the `gh stack` CLI han - **Pushing branches** — `gh stack push` pushes all branches to the remote. - **Creating PRs** — `gh stack submit` pushes branches and creates or updates PRs, linking them as a Stack on GitHub. - **Navigating the stack** — `gh stack up`, `down`, `top`, and `bottom` let you move between layers without remembering branch names. -- **Syncing everything** — `gh stack sync` fetches, rebases, pushes, and updates PR state in one command. +- **Syncing everything** — `gh stack sync` fetches, rebases, pushes, updates PR state, and links open PRs into a Stack on GitHub in one command. - **Restructuring stacks** — `gh stack modify` opens an interactive terminal UI to drop, fold, insert, rename, and reorder branches in a stack. - **Tearing down stacks** — `gh stack unstack` removes a stack from GitHub and local tracking. - **Checking out a stack** — `gh stack checkout ` pulls down a stack, with all its branches, from GitHub to your local machine. diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index bd21671..3cd99de 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -309,7 +309,8 @@ Performs a safe, non-interactive synchronization of the entire stack: 3. **Cascade rebase** — rebases all stack branches onto their updated parents (only if trunk moved). If a conflict is detected, all branches are restored to their original state, and you are advised to run `gh stack rebase` to resolve conflicts interactively. 4. **Push** — pushes all branches (uses `--force-with-lease` if a rebase occurred). 5. **Sync PRs** — syncs PR state from GitHub and reports the status of each PR. -6. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to prune automatically. +6. **Sync the stack** — links the stack's open PRs into a stack on GitHub, creating the remote stack object if it doesn't exist yet or updating it if it's partially formed. This only happens when two or more PRs exist; sync never opens PRs (use `gh stack submit` for that). +7. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to prune automatically. | Flag | Description | |------|-------------| diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 465d79b..4b14d50 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -632,7 +632,8 @@ gh stack sync [flags] 3. **Cascade rebase** all stack branches onto their updated parents (only if trunk moved). Handles merged PRs automatically. If a conflict is detected, **all branches are restored** to their pre-rebase state and the command exits with code 3 — see [Handle rebase conflicts](#handle-rebase-conflicts-agent-workflow) for the resolution workflow 4. **Push** all active branches atomically 5. **Sync PR state** from GitHub and report the status of each PR -6. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to skip the prompt. In non-interactive environments, pruning only happens when `--prune` is passed explicitly +6. **Sync the stack object** — link the open PRs into a stack on GitHub. If the PRs are not yet in a stack, a new stack is created; if some PRs are already in a stack, it is updated (additive only). This only happens when two or more PRs exist. Sync **never opens PRs** — use `gh stack submit` for that +7. **Prune** — in interactive terminals, prompts to delete local branches for merged PRs. Use `--prune` to skip the prompt. In non-interactive environments, pruning only happens when `--prune` is passed explicitly **Output (stderr):** @@ -642,8 +643,9 @@ gh stack sync [flags] - `✓ Pushed N branches` - `✓ PR #N () — 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) - `✓ Pruned (merged)` per pruned branch (when pruning) -- `✓ Stack synced` +- `✓ 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) --- From 0b9f511385a999cd90f3edc6e285713979b85405 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 14:29:23 -0400 Subject: [PATCH 2/4] Address PR review: one ListStacks per sync, command-neutral guidance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from the #156 review (both flagged optional / non-blocking). 1. Remove the redundant ListStacks round-trip on sync's create path. syncRemoteStack fetched the stack list for its already-up-to-date short-circuit, then delegated to syncStack -> adoptRemoteStack, which listed the stacks again — two GETs on the first-sync-create and membership-changed paths. Refactor adoptRemoteStack into a list-accepting reconcileUntrackedStack(cfg, client, s, prNumbers, stacks): syncStack now fetches the list once and passes it down, and syncRemoteStack reuses the list it already fetched. Net: exactly one ListStacks per sync. This also drops the (handled, synced) tuple. Submit's behavior is unchanged. 2. Make the divergence / dropped-PR guidance command-neutral. The shared helper emitted submit-specific wording ("reconcile them before submitting", "...then `gh stack submit`") that is now reachable from `gh stack sync`. Reword to "reconcile them first" and drop the trailing `gh stack submit` so it reads correctly from either command. Tests: assert exactly one ListStacks on the create path and that the divergence guidance is not submit-specific. --- cmd/submit.go | 58 +++++++++++++++++++++++------------------------- cmd/sync.go | 22 ++++++++++++------ cmd/sync_test.go | 8 ++++++- 3 files changed, 50 insertions(+), 38 deletions(-) diff --git a/cmd/submit.go b/cmd/submit.go index 810f6ce..9341d7a 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -721,48 +721,46 @@ 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. Adopt it instead of blindly creating a new one, which the API - // rejects because the PRs are already part of a stack. - if handled, synced := adoptRemoteStack(cfg, client, s, prNumbers); handled { - return synced - } - - return createNewStack(cfg, client, s, prNumbers) -} - -// adoptRemoteStack 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 must -// adopt that stack rather than POST a new one (which the API rejects because -// the PRs are already stacked). -// -// It returns (handled, synced). handled is true when it has fully handled the -// sync — either by adopting and updating the existing stack, or by -// intentionally refusing to modify a divergent remote stack — and false when no -// matching remote stack exists and the caller should create a new one. synced -// is true only when the remote stack object now reflects the local stack. -func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) (bool, bool) { + // 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. 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 false, false + 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 // belong to one stack, so this is a genuine divergence we can't resolve // automatically. - cfg.Warningf("Your PRs belong to multiple stacks on GitHub — reconcile them before submitting") + cfg.Warningf("Your PRs belong to multiple stacks on GitHub — reconcile them first") cfg.Printf(" Run `%s` to import a stack, or unstack the PRs from the web", cfg.ColorCyan("gh stack checkout ")) - return true, false + return false } if matched == nil { // No existing stack contains any of our PRs — create a new one. - return false, false + return createNewStack(cfg, client, s, prNumbers) } // A remote stack already contains some of our PRs. Refuse to silently drop @@ -770,9 +768,9 @@ func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stac if dropped := prsMissingFrom(matched.PullRequests, prNumbers); len(dropped) > 0 { cfg.Warningf("A stack on GitHub already contains %s, which %s not in your local stack", formatPRList(dropped), plural(len(dropped), "is", "are")) - cfg.Printf(" Run `%s` to import the full stack, then `%s`", - cfg.ColorCyan("gh stack checkout "), cfg.ColorCyan("gh stack submit")) - return true, false + cfg.Printf(" Run `%s` to import the full stack", + cfg.ColorCyan("gh stack checkout ")) + return false } // Every PR in the remote stack is tracked locally (and we may have added @@ -782,11 +780,11 @@ func adoptRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stac if slicesEqual(matched.PullRequests, prNumbers) { cfg.Successf("Linked to the existing stack on GitHub (%d PRs, already up to date)", len(prNumbers)) - return true, true + return true } cfg.Infof("Found the stack on GitHub — updating it to match your local stack") - return true, updateStack(cfg, client, s, prNumbers) + return updateStack(cfg, client, s, prNumbers) } // prsMissingFrom returns the numbers in remote that do not appear in local, diff --git a/cmd/sync.go b/cmd/sync.go index 0dfc781..ca5d884 100644 --- a/cmd/sync.go +++ b/cmd/sync.go @@ -366,13 +366,17 @@ func syncRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack return false } - // Inspect the remote stacks first so a routine sync that has not changed the - // PR membership does not issue a redundant — and misleading — update. + // 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; let syncStack attempt the operation and - // surface its own availability/PAT/create errors. - return syncStack(cfg, client, s) + // 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 && @@ -384,8 +388,12 @@ func syncRemoteStack(cfg *config.Config, client github.ClientOps, s *stack.Stack return true } - // Membership differs (or no stack exists yet): create, adopt, or update. - return syncStack(cfg, client, s) + // 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. diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 452c8c6..3fc39cb 100644 --- a/cmd/sync_test.go +++ b/cmd/sync_test.go @@ -1673,9 +1673,13 @@ func TestSync_CreatesRemoteStackWhenPRsExist(t *testing.T) { writeStackFile(t, tmpDir, s) var createdWith []int + var listCalls int ghMock := &github.MockClient{ FindPRForBranchFn: openPRFinder(map[string]int{"b1": 101, "b2": 102}), - ListStacksFn: func() ([]github.RemoteStack, error) { return nil, nil }, + ListStacksFn: func() ([]github.RemoteStack, error) { + listCalls++ + return nil, nil + }, CreateStackFn: func(prNumbers []int) (int, error) { createdWith = prNumbers return 7, nil @@ -1689,6 +1693,7 @@ func TestSync_CreatesRemoteStackWhenPRsExist(t *testing.T) { output := runSyncWithGitHub(t, newSyncMockNoRebase(tmpDir, "b1"), ghMock) assert.Equal(t, []int{101, 102}, createdWith, "should create the stack from both PR numbers") + assert.Equal(t, 1, listCalls, "should issue exactly one ListStacks on the create path (no redundant round-trip)") assert.Contains(t, output, "Stack created on GitHub with 2 PRs") assert.Contains(t, output, "Stack synced") assert.NotContains(t, output, "Branches synced") @@ -1863,6 +1868,7 @@ func TestSync_PRsSpanMultipleStacks_BranchesSynced(t *testing.T) { assert.False(t, createCalled, "CreateStack should not be called on divergence") assert.False(t, updateCalled, "UpdateStack should not be called on divergence") assert.Contains(t, output, "multiple stacks") + assert.NotContains(t, output, "submitting", "divergence guidance should be command-neutral, not submit-specific") assert.Contains(t, output, "Branches synced") assert.NotContains(t, output, "Stack synced") } From 43c715a2363a12fe9ced7d4251989b23a018e190 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 17:09:49 -0400 Subject: [PATCH 3/4] increment skill file version --- skills/gh-stack/SKILL.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 4b14d50..54ea54d 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -7,7 +7,7 @@ description: > branch chains, or incremental code review workflows. metadata: author: github - version: "0.0.6" + version: "0.0.7" --- # gh-stack From 97754d7610dde05a4ef72317afcef4f836b0c36f Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Mon, 29 Jun 2026 17:42:23 -0400 Subject: [PATCH 4/4] Simplify sync reconciliation: reuse syncStack instead of a parallel path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- cmd/submit.go | 56 +++++++++++++++++----------------------- cmd/sync.go | 50 +---------------------------------- cmd/sync_test.go | 2 +- skills/gh-stack/SKILL.md | 2 +- 4 files changed, 26 insertions(+), 84 deletions(-) diff --git a/cmd/submit.go b/cmd/submit.go index 9341d7a..a1ba004 100644 --- a/cmd/submit.go +++ b/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,9 +716,20 @@ 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 @@ -731,22 +737,6 @@ func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) bool 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 diff --git a/cmd/sync.go b/cmd/sync.go index ca5d884..70bd831 100644 --- a/cmd/sync.go +++ b/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 diff --git a/cmd/sync_test.go b/cmd/sync_test.go index 3fc39cb..746739e 100644 --- a/cmd/sync_test.go +++ b/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") diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 54ea54d..ca528ea 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -643,7 +643,7 @@ gh stack sync [flags] - `✓ Pushed N branches` - `✓ PR #N () — 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 (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)