Skip to content

Commit 8d96385

Browse files
committed
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.
1 parent 3eb1bfa commit 8d96385

2 files changed

Lines changed: 41 additions & 4 deletions

File tree

cmd/submit.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -468,15 +468,17 @@ func executePRDecisions(g *git.Git, cfg *config.Config, root *tree.Node, decisio
468468
fmt.Printf("%s failed to update PR #%d base: %v\n", s.WarningIcon(), d.prNum, err)
469469
} else {
470470
fmt.Println(s.Success("ok"))
471+
// Check for trunk transition BEFORE persisting the new base, so
472+
// isTransitionToTrunk compares against the previous stored value.
473+
maybeMarkPRReady(ghClient, cfg, d.prNum, b.Name, parent, trunk, s)
471474
_ = cfg.SetPRBase(b.Name, parent) //nolint:errcheck // best effort — used for transition detection only
475+
if err := ghClient.GenerateAndPostStackComment(root, b.Name, trunk, d.prNum, remoteBranches); err != nil {
476+
fmt.Printf("%s failed to update stack comment for PR #%d: %v\n", s.WarningIcon(), d.prNum, err)
477+
}
472478
if opts.OpenWeb {
473479
prURLs = append(prURLs, ghClient.PRURL(d.prNum))
474480
}
475481
}
476-
if err := ghClient.GenerateAndPostStackComment(root, b.Name, trunk, d.prNum, remoteBranches); err != nil {
477-
fmt.Printf("%s failed to update stack comment for PR #%d: %v\n", s.WarningIcon(), d.prNum, err)
478-
}
479-
maybeMarkPRReady(ghClient, cfg, d.prNum, b.Name, parent, trunk, s)
480482
}
481483
case prActionAdopt:
482484
if opts.DryRun {

cmd/submit_internal_test.go

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -424,6 +424,41 @@ func TestIsTransitionToTrunk(t *testing.T) {
424424
})
425425
}
426426

427+
// TestIsTransitionToTrunkOrderingInvariant verifies the property that
428+
// isTransitionToTrunk must be evaluated BEFORE SetPRBase is called with trunk,
429+
// because SetPRBase overwrites the stored value that the function reads.
430+
// This guards against the regression where SetPRBase was called before
431+
// maybeMarkPRReady in the prActionUpdate path, silently suppressing the prompt.
432+
func TestIsTransitionToTrunkOrderingInvariant(t *testing.T) {
433+
trunk := "main"
434+
435+
t.Run("returns_true_before_SetPRBase_when_parent_stored", func(t *testing.T) {
436+
cfg := setupTestRepo(t)
437+
if err := cfg.SetPRBase("feat-a", "feat-parent"); err != nil {
438+
t.Fatalf("SetPRBase failed: %v", err)
439+
}
440+
// Simulates the correct ordering: check transition BEFORE persisting trunk.
441+
transitionDetected := isTransitionToTrunk(cfg, "feat-a", trunk)
442+
_ = cfg.SetPRBase("feat-a", trunk)
443+
if !transitionDetected {
444+
t.Error("expected transition to be detected when checked before SetPRBase(trunk)")
445+
}
446+
})
447+
448+
t.Run("returns_false_after_SetPRBase_wrong_order", func(t *testing.T) {
449+
cfg := setupTestRepo(t)
450+
if err := cfg.SetPRBase("feat-a", "feat-parent"); err != nil {
451+
t.Fatalf("SetPRBase failed: %v", err)
452+
}
453+
// Simulates the buggy ordering: persist trunk BEFORE checking transition.
454+
_ = cfg.SetPRBase("feat-a", trunk)
455+
transitionDetected := isTransitionToTrunk(cfg, "feat-a", trunk)
456+
if transitionDetected {
457+
t.Error("demonstrates the bug: SetPRBase before check suppresses the prompt")
458+
}
459+
})
460+
}
461+
427462
func TestApplyMustPushForSkippedAncestors(t *testing.T) {
428463
main := &tree.Node{Name: "main"}
429464
featA := &tree.Node{Name: "feat-a", Parent: main}

0 commit comments

Comments
 (0)