From af58e8eba70b00717b4d317fdee90896fd82c190 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 13 Apr 2026 02:19:04 +0000 Subject: [PATCH] fix(cmd): defer submit push until PR intent is known Submit used to force-push every stack branch before the PR phase, so branches were published even when the user skipped opening a PR. Plan each branch first (GitHub adopt check, prompts, --update-only), then push only branches that will get PR work or must exist on the remote as a base for a descendant PR. PR execution reuses the plan. Add a unit test for ancestor push propagation on skip. Co-authored-by: christopher.hiller --- cmd/submit.go | 296 +++++++++++++++++++++++++----------- cmd/submit_internal_test.go | 30 ++++ 2 files changed, 235 insertions(+), 91 deletions(-) diff --git a/cmd/submit.go b/cmd/submit.go index 2beb656..932a6d7 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -30,7 +30,7 @@ parent-before-child order. Use --from to limit the scope to a subtree. Phases: 1. Restack: rebase affected branches onto their parents -2. Push: force-push all affected branches (with --force-with-lease) +2. Push: force-push branches that will get a PR (or that are bases for one), with --force-with-lease 3. PR: create PRs for branches without them, update PR bases for those that have them If a rebase conflict occurs, resolve it and run 'gh stack continue'.`, @@ -47,8 +47,35 @@ var ( submitFromFlag string ) -// ErrPRSkipped is returned when a user skips PR creation by pressing ESC. -var ErrPRSkipped = errors.New("PR creation skipped by user") +// prAction describes what we will do for a branch in the PR phase after push. +type prAction int + +const ( + prActionSkip prAction = iota + prActionUpdate + prActionAdopt + prActionCreate +) + +// prDecision is the outcome of planning one stack branch before any push. +type prDecision struct { + node *tree.Node + parent string + action prAction + // prActionUpdate + prNum int + // prActionAdopt + adoptPR *github.PR + // prActionCreate + title string + body string + draft bool + // pushAnyway is true when action is prActionSkip but a descendant branch + // will get a PR, so the branch must still exist on the remote as a base. + pushAnyway bool + // skipReason is set when action is prActionSkip ("update-only" or "user"). + skipReason string +} func init() { submitCmd.Flags().BoolVar(&submitDryRunFlag, "dry-run", false, "show what would be done without doing it") @@ -205,9 +232,41 @@ func runSubmit(cmd *cobra.Command, args []string) error { // doSubmitPushAndPR handles push and PR creation/update phases. // This is called after cascade succeeds (or from continue after conflict resolution). func doSubmitPushAndPR(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tree.Node, dryRun, updateOnly, openWeb, pushOnly bool, s *style.Style) error { - // Phase 2: Push all branches + var decisions []*prDecision + if !pushOnly { + trunk, err := cfg.GetTrunk() + if err != nil { + return err + } + var ghClient *github.Client + if !dryRun { + var clientErr error + ghClient, clientErr = github.NewClient() + if clientErr != nil { + return clientErr + } + } + decisions = planPRDecisions(g, cfg, ghClient, trunk, branches, dryRun, updateOnly, s) + applyMustPushForSkippedAncestors(decisions) + } + + // Phase 2: Push branches that will participate in PRs (or all if --push-only). fmt.Println(s.Bold("\n=== Phase 2: Push ===")) for _, b := range branches { + var d *prDecision + if !pushOnly { + for _, x := range decisions { + if x.node.Name == b.Name { + d = x + break + } + } + } + shouldPush := pushOnly || d == nil || d.action != prActionSkip || d.pushAnyway + if !shouldPush { + fmt.Printf("Skipping push for %s %s\n", s.Branch(b.Name), s.Muted("(no PR for this branch)")) + continue + } if dryRun { fmt.Printf("%s Would push %s -> origin/%s (forced)\n", s.Muted("dry-run:"), s.Branch(b.Name), s.Branch(b.Name)) } else { @@ -220,17 +279,104 @@ func doSubmitPushAndPR(g *git.Git, cfg *config.Config, root *tree.Node, branches } } - // Phase 3: Create/update PRs if pushOnly { fmt.Println(s.Bold("\n=== Phase 3: PRs ===")) fmt.Println(s.Muted("Skipped (--push-only)")) return nil } - return doSubmitPRs(g, cfg, root, branches, dryRun, updateOnly, openWeb, s) + return executePRDecisions(g, cfg, root, decisions, dryRun, openWeb, s) +} + +// planPRDecisions resolves what to do for each branch before any push. +// When dryRun is true, ghClient may be nil (no GitHub lookups for adopt). +func planPRDecisions(g *git.Git, cfg *config.Config, ghClient *github.Client, trunk string, branches []*tree.Node, dryRun, updateOnly bool, s *style.Style) []*prDecision { + out := make([]*prDecision, 0, len(branches)) + for _, b := range branches { + parent, _ := cfg.GetParent(b.Name) //nolint:errcheck // empty is fine + if parent == "" { + parent = trunk + } + existingPR, _ := cfg.GetPR(b.Name) //nolint:errcheck // 0 is fine + switch { + case existingPR > 0: + out = append(out, &prDecision{node: b, parent: parent, action: prActionUpdate, prNum: existingPR}) + case updateOnly: + out = append(out, &prDecision{node: b, parent: parent, action: prActionSkip, skipReason: "update-only"}) + case dryRun: + out = append(out, planPRDecisionDryRun(g, b, parent, trunk, s)) + default: + out = append(out, planPRDecisionInteractive(g, ghClient, b, parent, trunk, s)) + } + } + return out } -// doSubmitPRs handles PR creation/update for all branches. -func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tree.Node, dryRun, updateOnly, openWeb bool, s *style.Style) error { +func planPRDecisionDryRun(g *git.Git, b *tree.Node, parent, trunk string, s *style.Style) *prDecision { + draft := parent != trunk + defaultTitle := generateDefaultTitle(g, parent, b.Name) + defaultBody, bodyErr := generatePRBody(g, parent, b.Name) + if bodyErr != nil { + fmt.Printf("%s could not generate PR body: %v\n", s.WarningIcon(), bodyErr) + defaultBody = "" + } + return &prDecision{ + node: b, + parent: parent, + action: prActionCreate, + title: defaultTitle, + body: defaultBody, + draft: draft, + } +} + +func planPRDecisionInteractive(g *git.Git, ghClient *github.Client, b *tree.Node, parent, trunk string, s *style.Style) *prDecision { + branch := b.Name + existingPR, err := ghClient.FindPRByHead(branch) + if err != nil { + fmt.Printf("%s could not check for existing PR: %v\n", s.WarningIcon(), err) + } else if existingPR != nil { + return &prDecision{node: b, parent: parent, action: prActionAdopt, adoptPR: existingPR} + } + + draft := parent != trunk + defaultTitle := generateDefaultTitle(g, parent, branch) + defaultBody, bodyErr := generatePRBody(g, parent, branch) + if bodyErr != nil { + fmt.Printf("%s could not generate PR body: %v\n", s.WarningIcon(), bodyErr) + defaultBody = "" + } + title, body, skipped, err := promptForPRDetails(branch, defaultTitle, defaultBody, s) + if err != nil { + fmt.Printf("%s failed to get PR details for %s: %v\n", s.WarningIcon(), s.Branch(branch), err) + return &prDecision{node: b, parent: parent, action: prActionSkip, skipReason: "user"} + } + if skipped { + return &prDecision{node: b, parent: parent, action: prActionSkip, skipReason: "user"} + } + return &prDecision{node: b, parent: parent, action: prActionCreate, title: title, body: body, draft: draft} +} + +// applyMustPushForSkippedAncestors marks skipped branches that must still be +// pushed because a descendant will open or update a PR that uses them as base. +func applyMustPushForSkippedAncestors(decisions []*prDecision) { + byName := make(map[string]*prDecision, len(decisions)) + for _, d := range decisions { + byName[d.node.Name] = d + } + for _, d := range decisions { + if d.action == prActionSkip { + continue + } + for cur := d.node.Parent; cur != nil; cur = cur.Parent { + if x := byName[cur.Name]; x != nil { + x.pushAnyway = true + } + } + } +} + +// executePRDecisions runs the PR phase from pre-planned decisions (after push). +func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisions []*prDecision, dryRun, openWeb bool, s *style.Style) error { fmt.Println(s.Bold("\n=== Phase 3: PRs ===")) trunk, err := cfg.GetTrunk() @@ -238,7 +384,6 @@ func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tr return err } - // In dry-run mode, we don't need a GitHub client var ghClient *github.Client if !dryRun { var clientErr error @@ -248,64 +393,77 @@ func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tr } } - // Build remote branches set for stack comment filtering. - // This uses locally-cached tracking refs which are up-to-date after Phase 2 push. var remoteBranches map[string]bool if !dryRun { var rbErr error remoteBranches, rbErr = g.ListRemoteBranches() if rbErr != nil { - // Non-fatal: we can still render without filtering fmt.Printf("%s could not list remote branches, stack comments may reference local-only branches: %v\n", s.WarningIcon(), rbErr) } } - // Collect PR URLs for --web flag var prURLs []string - for _, b := range branches { - parent, _ := cfg.GetParent(b.Name) //nolint:errcheck // empty is fine - if parent == "" { - parent = trunk - } - - existingPR, _ := cfg.GetPR(b.Name) //nolint:errcheck // 0 is fine + for _, d := range decisions { + b := d.node + parent := d.parent - switch { - case existingPR > 0: - // Update existing PR + switch d.action { + case prActionSkip: if dryRun { - fmt.Printf("%s Would update PR #%d base to %s\n", s.Muted("dry-run:"), existingPR, s.Branch(parent)) + if d.skipReason == "update-only" { + fmt.Printf("Skipping %s %s\n", s.Branch(b.Name), s.Muted("(no existing PR, --update-only)")) + } + continue + } + switch d.skipReason { + case "update-only": + fmt.Printf("Skipping %s %s\n", s.Branch(b.Name), s.Muted("(no existing PR, --update-only)")) + case "user": + fmt.Printf("Skipped PR for %s %s\n", s.Branch(b.Name), s.Muted("(skipped)")) + } + case prActionUpdate: + if dryRun { + fmt.Printf("%s Would update PR #%d base to %s\n", s.Muted("dry-run:"), d.prNum, s.Branch(parent)) } else { - fmt.Printf("Updating PR #%d for %s (base: %s)... ", existingPR, s.Branch(b.Name), s.Branch(parent)) - if err := ghClient.UpdatePRBase(existingPR, parent); err != nil { + fmt.Printf("Updating PR #%d for %s (base: %s)... ", d.prNum, s.Branch(b.Name), s.Branch(parent)) + if err := ghClient.UpdatePRBase(d.prNum, parent); err != nil { fmt.Println(s.Error("failed")) - fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), existingPR, err) + fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), d.prNum, err) } else { fmt.Println(s.Success("ok")) if openWeb { - prURLs = append(prURLs, ghClient.PRURL(existingPR)) + prURLs = append(prURLs, ghClient.PRURL(d.prNum)) } } - // Update stack comment - if err := ghClient.GenerateAndPostStackComment(root, b.Name, trunk, existingPR, remoteBranches); err != nil { - fmt.Printf("%s failed to update stack comment for PR #%d: %v\n", s.WarningIcon(), existingPR, err) + if err := ghClient.GenerateAndPostStackComment(root, b.Name, trunk, d.prNum, remoteBranches); err != nil { + fmt.Printf("%s failed to update stack comment for PR #%d: %v\n", s.WarningIcon(), d.prNum, err) + } + maybeMarkPRReady(ghClient, d.prNum, b.Name, parent, trunk, s) + } + case prActionAdopt: + if dryRun { + fmt.Printf("%s Would adopt PR #%d for %s (base: %s)\n", s.Muted("dry-run:"), d.adoptPR.Number, s.Branch(b.Name), s.Branch(parent)) + } else { + prNum, adoptErr := adoptExistingPRDirect(ghClient, cfg, root, b.Name, parent, trunk, remoteBranches, d.adoptPR, s) + switch { + case adoptErr != nil: + fmt.Printf("%s failed to adopt PR for %s: %v\n", s.WarningIcon(), s.Branch(b.Name), adoptErr) + default: + fmt.Printf("%s Adopted PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) + if openWeb { + prURLs = append(prURLs, ghClient.PRURL(prNum)) + } } - - // If PR is a draft and now targets trunk, offer to publish - maybeMarkPRReady(ghClient, existingPR, b.Name, parent, trunk, s) } - case !updateOnly: - // Create new PR + case prActionCreate: if dryRun { fmt.Printf("%s Would create PR for %s (base: %s)\n", s.Muted("dry-run:"), s.Branch(b.Name), s.Branch(parent)) } else { - prNum, adopted, err := createPRForBranch(g, ghClient, cfg, root, b.Name, parent, trunk, remoteBranches, s) + prNum, adopted, execErr := executePRCreate(g, ghClient, cfg, root, b.Name, parent, trunk, d.title, d.body, d.draft, remoteBranches, s) switch { - case errors.Is(err, ErrPRSkipped): - fmt.Printf("Skipped PR for %s %s\n", s.Branch(b.Name), s.Muted("(skipped)")) - case err != nil: - fmt.Printf("%s failed to create PR for %s: %v\n", s.WarningIcon(), s.Branch(b.Name), err) + case execErr != nil: + fmt.Printf("%s failed to create PR for %s: %v\n", s.WarningIcon(), s.Branch(b.Name), execErr) case adopted: fmt.Printf("%s Adopted PR #%d for %s (%s)\n", s.SuccessIcon(), prNum, s.Branch(b.Name), ghClient.PRURL(prNum)) if openWeb { @@ -318,12 +476,9 @@ func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tr } } } - default: - fmt.Printf("Skipping %s %s\n", s.Branch(b.Name), s.Muted("(no existing PR, --update-only)")) } } - // Open PRs in browser if requested if openWeb && len(prURLs) > 0 { b := browser.New("", os.Stdout, os.Stderr) for _, url := range prURLs { @@ -336,53 +491,15 @@ func doSubmitPRs(g *git.Git, cfg *config.Config, root *tree.Node, branches []*tr return nil } -// createPRForBranch creates a PR for the given branch and stores the PR number. -// If a PR already exists for the branch, it adopts the existing PR instead. -// Returns (prNumber, adopted, error) where adopted is true if we adopted an existing PR. -func createPRForBranch(g *git.Git, ghClient *github.Client, cfg *config.Config, root *tree.Node, branch, base, trunk string, remoteBranches map[string]bool, s *style.Style) (int, bool, error) { - // Check for existing PR on GitHub BEFORE prompting user for title/description. - // This avoids the confusing UX where we prompt then immediately say "oh, PR already exists". - existingPR, err := ghClient.FindPRByHead(branch) - if err != nil { - // Non-fatal: proceed with creation attempt which will give clearer error if needed - fmt.Printf("%s could not check for existing PR: %v\n", s.WarningIcon(), err) - } else if existingPR != nil { - // PR exists on GitHub but wasn't in local config - adopt it without prompting - prNum, adoptErr := adoptExistingPRDirect(ghClient, cfg, root, branch, base, trunk, remoteBranches, existingPR, s) - return prNum, true, adoptErr - } - - // Determine if draft (not targeting trunk = middle of stack) - draft := base != trunk - - // Generate default title from first commit message (falls back to branch name) - defaultTitle := generateDefaultTitle(g, base, branch) - - // Generate PR body from commits - defaultBody, bodyErr := generatePRBody(g, base, branch) - if bodyErr != nil { - // Non-fatal: just skip auto-body - fmt.Printf("%s could not generate PR body: %v\n", s.WarningIcon(), bodyErr) - defaultBody = "" - } - - // Get title and body (prompt if interactive and --yes not set) - title, body, skipped, err := promptForPRDetails(branch, defaultTitle, defaultBody, s) - if err != nil { - return 0, false, fmt.Errorf("failed to get PR details: %w", err) - } - if skipped { - return 0, false, ErrPRSkipped - } - +// executePRCreate opens a PR with the given title/body (branch must be on the remote). +// Returns adopted true if an existing PR was adopted instead of creating. +func executePRCreate(g *git.Git, ghClient *github.Client, cfg *config.Config, root *tree.Node, branch, base, trunk, title, body string, draft bool, remoteBranches map[string]bool, s *style.Style) (prNum int, adopted bool, err error) { pr, err := ghClient.CreateSubmitPR(branch, base, title, body, draft) if err != nil { - // Check if PR already exists - if so, adopt it if strings.Contains(err.Error(), "pull request already exists") { - prNum, adoptErr := adoptExistingPR(ghClient, cfg, root, branch, base, trunk, remoteBranches, s) - return prNum, true, adoptErr + n, adoptErr := adoptExistingPR(ghClient, cfg, root, branch, base, trunk, remoteBranches, s) + return n, true, adoptErr } - // Detect missing base branch on remote and provide an actionable message if isBaseBranchInvalidError(err) { return 0, false, fmt.Errorf( "base branch %q does not exist on the remote; push it first or run 'gh stack submit' to push the entire stack: %w", @@ -391,17 +508,14 @@ func createPRForBranch(g *git.Git, ghClient *github.Client, cfg *config.Config, return 0, false, err } - // Store PR number in config if err := cfg.SetPR(branch, pr.Number); err != nil { return pr.Number, false, fmt.Errorf("PR created but failed to store number: %w", err) } - // Update the tree node's PR number so stack comments render correctly if node := tree.FindNode(root, branch); node != nil { node.PR = pr.Number } - // Add stack navigation comment if err := ghClient.GenerateAndPostStackComment(root, branch, trunk, pr.Number, remoteBranches); err != nil { fmt.Printf("%s failed to add stack comment to PR #%d: %v\n", s.WarningIcon(), pr.Number, err) } @@ -529,7 +643,7 @@ func adoptExistingPR(ghClient *github.Client, cfg *config.Config, root *tree.Nod } // adoptExistingPRDirect adopts an already-fetched PR into the stack. -// This is the implementation shared by adoptExistingPR and the early-detection path in createPRForBranch. +// This is the implementation shared by adoptExistingPR and the adopt path in executePRDecisions. func adoptExistingPRDirect(ghClient *github.Client, cfg *config.Config, root *tree.Node, branch, base, trunk string, remoteBranches map[string]bool, existingPR *github.PR, s *style.Style) (int, error) { // Store PR number in config if err := cfg.SetPR(branch, existingPR.Number); err != nil { diff --git a/cmd/submit_internal_test.go b/cmd/submit_internal_test.go index c3b2b11..d7a44da 100644 --- a/cmd/submit_internal_test.go +++ b/cmd/submit_internal_test.go @@ -10,6 +10,8 @@ import ( "errors" "fmt" "testing" + + "github.com/boneskull/gh-stack/internal/tree" ) func TestUnwrapParagraphs(t *testing.T) { @@ -315,3 +317,31 @@ func TestIsBaseBranchInvalidError(t *testing.T) { }) } } + +func TestApplyMustPushForSkippedAncestors(t *testing.T) { + main := &tree.Node{Name: "main"} + featA := &tree.Node{Name: "feat-a", Parent: main} + featB := &tree.Node{Name: "feat-b", Parent: featA} + + t.Run("skipped_parent_pushed_when_child_gets_PR", func(t *testing.T) { + decisions := []*prDecision{ + {node: featA, action: prActionSkip, skipReason: "user"}, + {node: featB, action: prActionCreate, title: "t", body: "b", draft: false}, + } + applyMustPushForSkippedAncestors(decisions) + if !decisions[0].pushAnyway { + t.Error("feat-a should be marked pushAnyway when feat-b gets a PR") + } + }) + + t.Run("skipped_branch_not_marked_when_no_descendant_PR", func(t *testing.T) { + decisions := []*prDecision{ + {node: featA, action: prActionSkip, skipReason: "user"}, + {node: featB, action: prActionSkip, skipReason: "user"}, + } + applyMustPushForSkippedAncestors(decisions) + if decisions[0].pushAnyway || decisions[1].pushAnyway { + t.Error("no pushAnyway when entire subtree is skipped") + } + }) +}