fix(submit): only prompt to publish draft when base transitions to trunk (#100)

* fix(submit): only prompt to publish draft when base transitions to trunk

Resolves #95. Previously, `gh stack submit` would prompt to publish a
draft PR every time it ran, as long as the PR was a draft and targeted
trunk. This got annoying fast.

The fix adds a `stackPRBase` key to `.git/config` (per branch) that
persists the last-known remote base after each successful submit run.
The publish prompt now fires only when the stored base differs from
trunk — i.e., the PR's base is genuinely moving to trunk for the first
time since the last submit.

Cleanup is consistent: `unlink`, `orphan`, and sync's merged-branch
handlers all clear `stackPRBase` alongside the other per-branch config
keys. Sync's retarget-to-trunk path also persists the new base so
subsequent submit runs don't re-prompt.

* fix(submit): check trunk transition before persisting new PR base

The `prActionUpdate` path in `executePRDecisions` was calling
`SetPRBase` before `maybeMarkPRReady`, which meant
`isTransitionToTrunk` always read the value we had just written
(trunk) and returned false. The publish-draft prompt was therefore
silently suppressed for all existing PRs transitioning to trunk.

Fix: call `maybeMarkPRReady` first, then persist the new base.
While here, move `GenerateAndPostStackComment` inside the success
branch so it doesn't run when `UpdatePRBase` fails.

Add `TestIsTransitionToTrunkOrderingInvariant` to document and
guard the required call order.

Author: boneskull

cmd/orphan.go

@@ -76,6 +76,7 @@ func runOrphan(cmd *cobra.Command, args []string) error {
 			_ = cfg.RemoveParent(desc.Name)    //nolint:errcheck // best effort cleanup
 			_ = cfg.RemovePR(desc.Name)        //nolint:errcheck // best effort cleanup
 			_ = cfg.RemoveForkPoint(desc.Name) //nolint:errcheck // best effort cleanup
+			_ = cfg.RemovePRBase(desc.Name)    //nolint:errcheck // best effort cleanup
 			fmt.Printf("%s Orphaned %s\n", s.SuccessIcon(), s.Branch(desc.Name))
 		}
 	}
@@ -84,6 +85,7 @@ func runOrphan(cmd *cobra.Command, args []string) error {
 	_ = cfg.RemoveParent(branchName)    //nolint:errcheck // best effort cleanup
 	_ = cfg.RemovePR(branchName)        //nolint:errcheck // best effort cleanup
 	_ = cfg.RemoveForkPoint(branchName) //nolint:errcheck // best effort cleanup
+	_ = cfg.RemovePRBase(branchName)    //nolint:errcheck // best effort cleanup
 	fmt.Printf("%s Orphaned %s\n", s.SuccessIcon(), s.Branch(branchName))
 
 	return nil

cmd/submit.go

@@ -468,14 +468,17 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio
 					fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), d.prNum, err)
 				} else {
 					fmt.Println(s.Success("ok"))
+					// Check for trunk transition BEFORE persisting the new base, so
+					// isTransitionToTrunk compares against the previous stored value.
+					maybeMarkPRReady(ghClient, cfg, d.prNum, b.Name, parent, trunk, s)
+					_ = cfg.SetPRBase(b.Name, parent) //nolint:errcheck // best effort — used for transition detection only
+					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)
+					}
 					if opts.OpenWeb {
 						prURLs = append(prURLs, ghClient.PRURL(d.prNum))
 					}
 				}
-				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 opts.DryRun {
@@ -561,6 +564,9 @@ func executePRCreate(pCtx prContext, branch, base, title, body string, draft boo
 		return pr.Number, false, fmt.Errorf("PR created but failed to store number: %w", err)
 	}
 
+	// Persist the base so future submit runs can detect trunk transitions.
+	_ = pCtx.cfg.SetPRBase(branch, base) //nolint:errcheck // best effort
+
 	if node := tree.FindNode(pCtx.root, branch); node != nil {
 		node.PR = pr.Number
 	}
@@ -711,23 +717,51 @@ func adoptExistingPRDirect(pCtx prContext, branch, base string, existingPR *gith
 		}
 	}
 
+	// Persist the new base for transition detection on future submit runs.
+	_ = pCtx.cfg.SetPRBase(branch, base) //nolint:errcheck // best effort
+
 	// Add/update stack navigation comment
 	if err := pCtx.ghClient.GenerateAndPostStackComment(pCtx.root, branch, pCtx.trunk, existingPR.Number, pCtx.remoteBranches); err != nil {
 		fmt.Printf("%s failed to update stack comment: %v\n", pCtx.s.WarningIcon(), err)
 	}
 
-	// If adopted PR is a draft and targets trunk, offer to publish
-	if existingPR.Draft && base == pCtx.trunk {
+	// Prompt to publish if the PR is a draft and its base is transitioning to trunk.
+	// We use the live pre-adoption base (existingPR.Base.Ref) here since stored base
+	// metadata may not exist yet for a freshly adopted PR.
+	if existingPR.Draft && base == pCtx.trunk && existingPR.Base.Ref != pCtx.trunk {
 		promptMarkPRReady(pCtx.ghClient, existingPR.Number, branch, pCtx.trunk, pCtx.s)
 	}
 
 	return existingPR.Number, nil
 }
 
+// isTransitionToTrunk reports whether a PR's base branch is moving to trunk for
+// the first time as tracked by gh-stack. It uses the stored last-known PR base
+// (persisted after each successful submit) to detect the transition, avoiding
+// any additional GitHub API calls.
+//
+// Returns true when:
+//   - no stored base exists yet (first run after PR creation/adoption), or
+//   - stored base is not trunk (base is changing from something else to trunk).
+//
+// Returns false when the stored base is already trunk, meaning the PR was
+// already targeting trunk on the previous submit run.
+func isTransitionToTrunk(cfg *config.Config, branch, trunk string) bool {
+	storedBase, err := cfg.GetPRBase(branch)
+	if err == nil && storedBase == trunk {
+		return false
+	}
+	return true
+}
+
 // maybeMarkPRReady checks if a PR is a draft targeting trunk and offers to publish it.
 // This handles the case where a PR was created as a draft (middle of stack) but now
 // targets trunk because its parent was merged.
-func maybeMarkPRReady(ghClient *github.Client, prNumber int, branch, base, trunk string, s *style.Style) {
+//
+// The prompt fires only when the base branch is transitioning to trunk for the first
+// time — i.e., the stored last-known base is not already trunk. This prevents the
+// prompt from appearing on every submit run once the PR already targets trunk.
+func maybeMarkPRReady(ghClient *github.Client, cfg *config.Config, prNumber int, branch, base, trunk string, s *style.Style) {
 	// Only relevant if PR now targets trunk
 	if base != trunk {
 		return
@@ -739,6 +773,11 @@ func maybeMarkPRReady(ghClient *github.Client, prNumber int, branch, base, trunk
 		return
 	}
 
+	// Only prompt when the base is transitioning to trunk this run.
+	if !isTransitionToTrunk(cfg, branch, trunk) {
+		return
+	}
+
 	promptMarkPRReady(ghClient, prNumber, branch, trunk, s)
 }
 

cmd/submit_internal_test.go

@@ -9,9 +9,13 @@ package cmd
 import (
 	"errors"
 	"fmt"
+	"os/exec"
 	"testing"
 
+	"github.com/boneskull/gh-stack/internal/config"
+	"github.com/boneskull/gh-stack/internal/git"
 	"github.com/boneskull/gh-stack/internal/github"
+	"github.com/boneskull/gh-stack/internal/style"
 	"github.com/boneskull/gh-stack/internal/tree"
 )
 
@@ -319,6 +323,142 @@ func TestIsBaseBranchInvalidError(t *testing.T) {
 	}
 }
 
+func setupTestRepo(t *testing.T) *config.Config {
+	t.Helper()
+	dir := t.TempDir()
+	if err := exec.Command("git", "init", dir).Run(); err != nil {
+		t.Fatalf("git init failed: %v", err)
+	}
+	exec.Command("git", "-C", dir, "config", "user.email", "test@test.com").Run() //nolint:errcheck
+	exec.Command("git", "-C", dir, "config", "user.name", "Test").Run()           //nolint:errcheck
+	cfg, err := config.Load(dir)
+	if err != nil {
+		t.Fatalf("config.Load failed: %v", err)
+	}
+	return cfg
+}
+
+// setupTestRepoWithDir is like setupTestRepo but also returns the directory path
+// for callers that need to run git commands directly or construct a git.Git instance.
+func setupTestRepoWithDir(t *testing.T) (*config.Config, string) {
+	t.Helper()
+	dir := t.TempDir()
+	if err := exec.Command("git", "init", dir).Run(); err != nil {
+		t.Fatalf("git init failed: %v", err)
+	}
+	exec.Command("git", "-C", dir, "config", "user.email", "test@test.com").Run() //nolint:errcheck
+	exec.Command("git", "-C", dir, "config", "user.name", "Test").Run()           //nolint:errcheck
+	// Create an initial commit so the repo has a HEAD and we can create branches.
+	exec.Command("git", "-C", dir, "commit", "--allow-empty", "-m", "init").Run() //nolint:errcheck
+	cfg, err := config.Load(dir)
+	if err != nil {
+		t.Fatalf("config.Load failed: %v", err)
+	}
+	return cfg, dir
+}
+
+func TestIsTransitionToTrunk(t *testing.T) {
+	trunk := "main"
+
+	t.Run("no_stored_base_returns_true", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		// No stored base — first run after PR creation; should prompt.
+		if !isTransitionToTrunk(cfg, "feat-a", trunk) {
+			t.Error("expected true when no stored base exists")
+		}
+	})
+
+	t.Run("stored_base_is_not_trunk_returns_true", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		// Branch previously targeted a non-trunk parent; now it targets trunk.
+		if err := cfg.SetPRBase("feat-a", "feat-parent"); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		if !isTransitionToTrunk(cfg, "feat-a", trunk) {
+			t.Error("expected true when stored base is not trunk")
+		}
+	})
+
+	t.Run("stored_base_is_trunk_returns_false", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		// Branch was already targeting trunk on the previous run; don't re-prompt.
+		if err := cfg.SetPRBase("feat-a", trunk); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		if isTransitionToTrunk(cfg, "feat-a", trunk) {
+			t.Error("expected false when stored base is already trunk")
+		}
+	})
+
+	t.Run("different_branches_are_independent", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		// feat-a already targeting trunk; feat-b has no stored base.
+		if err := cfg.SetPRBase("feat-a", trunk); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		if isTransitionToTrunk(cfg, "feat-a", trunk) {
+			t.Error("feat-a: expected false when stored base is trunk")
+		}
+		if !isTransitionToTrunk(cfg, "feat-b", trunk) {
+			t.Error("feat-b: expected true when no stored base exists")
+		}
+	})
+
+	t.Run("custom_trunk_name_works", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		customTrunk := "master"
+		// Stored as "master"; should not prompt.
+		if err := cfg.SetPRBase("feat-a", customTrunk); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		if isTransitionToTrunk(cfg, "feat-a", customTrunk) {
+			t.Error("expected false with custom trunk name already stored")
+		}
+		// Stored as something else; should prompt.
+		if err := cfg.SetPRBase("feat-b", "other-branch"); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		if !isTransitionToTrunk(cfg, "feat-b", customTrunk) {
+			t.Error("expected true when stored base is not the custom trunk")
+		}
+	})
+}
+
+// TestIsTransitionToTrunkOrderingInvariant verifies the property that
+// isTransitionToTrunk must be evaluated BEFORE SetPRBase is called with trunk,
+// because SetPRBase overwrites the stored value that the function reads.
+// This guards against the regression where SetPRBase was called before
+// maybeMarkPRReady in the prActionUpdate path, silently suppressing the prompt.
+func TestIsTransitionToTrunkOrderingInvariant(t *testing.T) {
+	trunk := "main"
+
+	t.Run("returns_true_before_SetPRBase_when_parent_stored", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		if err := cfg.SetPRBase("feat-a", "feat-parent"); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		// Simulates the correct ordering: check transition BEFORE persisting trunk.
+		transitionDetected := isTransitionToTrunk(cfg, "feat-a", trunk)
+		_ = cfg.SetPRBase("feat-a", trunk)
+		if !transitionDetected {
+			t.Error("expected transition to be detected when checked before SetPRBase(trunk)")
+		}
+	})
+
+	t.Run("returns_false_after_SetPRBase_wrong_order", func(t *testing.T) {
+		cfg := setupTestRepo(t)
+		if err := cfg.SetPRBase("feat-a", "feat-parent"); err != nil {
+			t.Fatalf("SetPRBase failed: %v", err)
+		}
+		// Simulates the buggy ordering: persist trunk BEFORE checking transition.
+		_ = cfg.SetPRBase("feat-a", trunk)
+		transitionDetected := isTransitionToTrunk(cfg, "feat-a", trunk)
+		if transitionDetected {
+			t.Error("demonstrates the bug: SetPRBase before check suppresses the prompt")
+		}
+	})
+}
+
 func TestApplyMustPushForSkippedAncestors(t *testing.T) {
 	main := &tree.Node{Name: "main"}
 	featA := &tree.Node{Name: "feat-a", Parent: main}
@@ -392,3 +532,84 @@ func TestApplyMustPushForSkippedAncestors(t *testing.T) {
 		}
 	})
 }
+
+func TestDeleteMergedBranchClearsPRBase(t *testing.T) {
+	cfg, dir := setupTestRepoWithDir(t)
+	g := git.New(dir)
+	s := style.New()
+
+	trunk, err := g.CurrentBranch()
+	if err != nil {
+		t.Fatalf("CurrentBranch failed: %v", err)
+	}
+	if err := cfg.SetTrunk(trunk); err != nil {
+		t.Fatalf("SetTrunk failed: %v", err)
+	}
+
+	// Create feature-a with a commit so git can delete it later.
+	if err := exec.Command("git", "-C", dir, "checkout", "-b", "feature-a").Run(); err != nil {
+		t.Fatalf("create branch failed: %v", err)
+	}
+	if err := exec.Command("git", "-C", dir, "commit", "--allow-empty", "-m", "feat").Run(); err != nil {
+		t.Fatalf("commit failed: %v", err)
+	}
+	if err := exec.Command("git", "-C", dir, "checkout", trunk).Run(); err != nil {
+		t.Fatalf("checkout trunk failed: %v", err)
+	}
+
+	if err := cfg.SetParent("feature-a", trunk); err != nil {
+		t.Fatalf("SetParent failed: %v", err)
+	}
+	if err := cfg.SetPR("feature-a", 10); err != nil {
+		t.Fatalf("SetPR failed: %v", err)
+	}
+	if err := cfg.SetPRBase("feature-a", trunk); err != nil {
+		t.Fatalf("SetPRBase failed: %v", err)
+	}
+
+	currentBranch := trunk
+	deleteMergedBranch(g, cfg, "feature-a", trunk, &currentBranch, s)
+
+	if v, err := cfg.GetPRBase("feature-a"); err == nil {
+		t.Errorf("expected stackPRBase to be removed after deleteMergedBranch, got %q", v)
+	}
+	if v, err := cfg.GetPR("feature-a"); err == nil {
+		t.Errorf("expected stackPR to be removed after deleteMergedBranch, got %d", v)
+	}
+}
+
+func TestOrphanMergedBranchClearsPRBase(t *testing.T) {
+	cfg, dir := setupTestRepoWithDir(t)
+	g := git.New(dir)
+	s := style.New()
+
+	trunk, err := g.CurrentBranch()
+	if err != nil {
+		t.Fatalf("CurrentBranch failed: %v", err)
+	}
+	if err := cfg.SetTrunk(trunk); err != nil {
+		t.Fatalf("SetTrunk failed: %v", err)
+	}
+
+	if err := cfg.SetParent("feature-a", trunk); err != nil {
+		t.Fatalf("SetParent failed: %v", err)
+	}
+	if err := cfg.SetPR("feature-a", 11); err != nil {
+		t.Fatalf("SetPR failed: %v", err)
+	}
+	if err := cfg.SetPRBase("feature-a", trunk); err != nil {
+		t.Fatalf("SetPRBase failed: %v", err)
+	}
+
+	orphanMergedBranch(cfg, "feature-a", s)
+
+	if v, err := cfg.GetPRBase("feature-a"); err == nil {
+		t.Errorf("expected stackPRBase to be removed after orphanMergedBranch, got %q", v)
+	}
+	if v, err := cfg.GetPR("feature-a"); err == nil {
+		t.Errorf("expected stackPR to be removed after orphanMergedBranch, got %d", v)
+	}
+	if v, err := cfg.GetParent("feature-a"); err == nil {
+		t.Errorf("expected stackParent to be removed after orphanMergedBranch, got %q", v)
+	}
+}

cmd/sync.go

@@ -318,6 +318,10 @@ func runSync(cmd *cobra.Command, args []string) error {
 		if rt.childPR > 0 {
 			if updateErr := gh.UpdatePRBase(rt.childPR, trunk); updateErr != nil {
 				fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), rt.childPR, updateErr)
+			} else {
+				// Persist the new base so the submit publish-prompt knows this
+				// PR is already targeting trunk on its next run.
+				_ = cfg.SetPRBase(rt.childName, trunk) //nolint:errcheck // best effort
 			}
 		}
 
@@ -473,6 +477,7 @@ func deleteMergedBranch(g *git.Git, cfg *config.Config, branch, trunk string, cu
 	_ = cfg.RemoveParent(branch)    //nolint:errcheck // best effort cleanup
 	_ = cfg.RemovePR(branch)        //nolint:errcheck // best effort cleanup
 	_ = cfg.RemoveForkPoint(branch) //nolint:errcheck // best effort cleanup
+	_ = cfg.RemovePRBase(branch)    //nolint:errcheck // best effort cleanup
 	if err := g.DeleteBranch(branch); err != nil {
 		fmt.Printf("%s could not delete branch %s: %v\n", s.WarningIcon(), s.Branch(branch), err)
 	}
@@ -485,5 +490,6 @@ func orphanMergedBranch(cfg *config.Config, branch string, s *style.Style) merge
 	_ = cfg.RemoveParent(branch)    //nolint:errcheck // best effort cleanup
 	_ = cfg.RemovePR(branch)        //nolint:errcheck // best effort cleanup
 	_ = cfg.RemoveForkPoint(branch) //nolint:errcheck // best effort cleanup
+	_ = cfg.RemovePRBase(branch)    //nolint:errcheck // best effort cleanup
 	return mergedActionOrphan
 }

cmd/unlink.go

@@ -42,6 +42,7 @@ func runUnlink(cmd *cobra.Command, args []string) error {
 	if err := cfg.RemovePR(branch); err != nil {
 		return err
 	}
+	_ = cfg.RemovePRBase(branch) //nolint:errcheck // best effort cleanup
 
 	s := style.New()
 	fmt.Printf("%s Unlinked PR from branch %s\n", s.SuccessIcon(), s.Branch(branch))