Skip to content

Commit 8c313e8

Browse files
committed
Protect reordered PRs during submit
1 parent 3c6a15f commit 8c313e8

7 files changed

Lines changed: 93 additions & 11 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ Push all branches and create/update PRs and the stack on GitHub.
433433
gh stack submit [flags]
434434
```
435435

436-
Pushes all active branches together atomically before creating or updating PRs. Atomicity applies to Git branch updates, not PR or stack API operations.
436+
Pushes all active branches together atomically before creating PRs or setting their final bases. For pending reorders, at-risk PRs temporarily target the trunk while branches are pushed. Atomicity applies to Git branch updates, not PR or stack API operations.
437437

438438
After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs will be added to the top of the stack.
439439

‎cmd/link_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1679,6 +1679,7 @@ func TestLink_UpdateDeletedStack_FallsBackToCreate(t *testing.T) {
16791679
}
16801680

16811681
func TestLink_PushesBranchesBeforeResolution(t *testing.T) {
1682+
t.Setenv("GH_STACK_NO_ATOMIC", "")
16821683
var pushedBranches []string
16831684
var pushedRemote string
16841685

‎cmd/push_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ func newPushMock(tmpDir string, currentBranch string) *git.MockOps {
2525
}
2626

2727
func TestPush_PushesAllBranches(t *testing.T) {
28+
t.Setenv("GH_STACK_NO_ATOMIC", "")
2829
s := stack.Stack{
2930
Trunk: stack.BranchRef{Branch: "main"},
3031
Branches: []stack.BranchRef{

‎cmd/submit.go‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,13 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
242242
drafts = collected
243243
}
244244

245+
if pendingModify {
246+
if err := prepareReorderedPRs(cfg, client, s); err != nil {
247+
cfg.Errorf("failed to prepare reordered PRs: %s", err)
248+
return ErrSilent
249+
}
250+
}
251+
245252
if len(activeBranches) > 0 {
246253
cfg.Printf("Pushing to %s...", remote)
247254
if err := git.Push(remote, activeBranches, true, !opts.noAtomic); err != nil {
@@ -307,6 +314,35 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error {
307314
return nil
308315
}
309316

317+
// Reversed PR relationships can cause indirect merges when the new heads are
318+
// pushed. Use the unchanged trunk until ensurePR restores the intended bases.
319+
func prepareReorderedPRs(cfg *config.Config, client github.ClientOps, s *stack.Stack) error {
320+
for i, b := range s.Branches {
321+
if b.IsSkipped() {
322+
continue
323+
}
324+
pr, err := client.FindPRForBranch(b.Branch)
325+
if err != nil {
326+
return fmt.Errorf("looking up PR for %s: %w", b.Branch, err)
327+
}
328+
if pr == nil || s.IndexOf(pr.BaseRefName) <= i {
329+
continue
330+
}
331+
if pr.IsAutoMergeEnabled() {
332+
if err := client.DisableAutoMerge(pr.ID); err != nil {
333+
return fmt.Errorf("disabling auto-merge for PR #%d: %w", pr.Number, err)
334+
}
335+
cfg.Warningf("Disabled auto-merge for PR %s (incompatible with stacked PRs)",
336+
cfg.PRLink(pr.Number, pr.URL))
337+
}
338+
if err := client.UpdatePRBase(pr.Number, s.Trunk.Branch); err != nil {
339+
return fmt.Errorf("temporarily setting base of PR #%d to %s: %w", pr.Number, s.Trunk.Branch, err)
340+
}
341+
cfg.Successf("Temporarily set base of PR %s to %s", cfg.PRLink(pr.Number, pr.URL), s.Trunk.Branch)
342+
}
343+
return nil
344+
}
345+
310346
// collectPRDrafts loads branch display data and runs the interactive submit TUI
311347
// so the user can choose which new branches become PRs and draft each one. It
312348
// returns the per-branch overrides, whether the user cancelled, and any error.

‎cmd/submit_test.go‎

Lines changed: 51 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,7 @@ func newSubmitMock(tmpDir string, currentBranch string) *git.MockOps {
9494
}
9595

9696
func TestSubmit_CreatesPRsAndStack(t *testing.T) {
97+
t.Setenv("GH_STACK_NO_ATOMIC", "")
9798
s := stack.Stack{
9899
Trunk: stack.BranchRef{Branch: "main"},
99100
Branches: []stack.BranchRef{
@@ -1885,6 +1886,8 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
18851886
retry bool
18861887
mergeOnRetry bool
18871888
mergeAfterUnstack bool
1889+
reorder bool
1890+
noAtomic bool
18881891
}{
18891892
{name: "one existing PR", branches: 1},
18901893
{name: "last PR merged while unstacking", branches: 1, mergeAfterUnstack: true},
@@ -1901,6 +1904,10 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
19011904
{name: "one PR lost during final refresh", branches: 1, failure: "refresh"},
19021905
{name: "one PR after partial unstack", branches: 1, mergedBase: true},
19031906
{name: "multiple PRs complete", branches: 3},
1907+
{name: "reordered PRs", branches: 3, reorder: true},
1908+
{name: "reordered PRs non-atomic", branches: 3, reorder: true, noAtomic: true},
1909+
{name: "reordered PRs preparation fails", branches: 3, reorder: true, failure: "prepare", retry: true},
1910+
{name: "reordered PRs push fails", branches: 3, reorder: true, failure: "push", retry: true},
19041911
{name: "multiple PRs after partial unstack", branches: 3, mergedBase: true},
19051912
{name: "retry push after partial unstack", branches: 3, mergedBase: true, failure: "push", retry: true},
19061913
{name: "released PR merged before retry", branches: 3, mergedBase: true, failure: "push", retry: true, mergeOnRetry: true},
@@ -1933,14 +1940,14 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
19331940
if i > 0 {
19341941
base = fmt.Sprintf("b%d", i)
19351942
}
1936-
if name == target {
1943+
if name == target && !tc.reorder {
19371944
base = "removed"
19381945
}
19391946
prs[name] = &github.PullRequest{
19401947
Number: 10 + i, ID: fmt.Sprintf("PR_%d", 10+i), State: "OPEN",
19411948
HeadRefName: name, BaseRefName: base,
19421949
}
1943-
if tc.failure == "auto-merge" && name == target {
1950+
if (tc.failure == "auto-merge" || tc.reorder) && name == target {
19441951
prs[name].AutoMergeRequest = &github.AutoMergeRequest{EnabledAt: "2026-09-29T00:00:00Z"}
19451952
}
19461953
prs[name].IsDraft = tc.failure == "ready" && name == target
@@ -1949,6 +1956,9 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
19491956
}
19501957
s.Branches = append(s.Branches, br)
19511958
}
1959+
if tc.reorder {
1960+
s.Branches[1], s.Branches[2] = s.Branches[2], s.Branches[1]
1961+
}
19521962
writeStackFile(t, dir, s)
19531963
pending := newPendingSubmitState("42")
19541964
if tc.priorDeleted {
@@ -1958,8 +1968,12 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
19581968
saveModifyState(t, dir, pending)
19591969
mock := newSubmitMock(dir, "b1")
19601970
var pushes []string
1961-
mock.PushFn = func(_ string, branches []string, _, _ bool) error {
1971+
mock.PushFn = func(_ string, branches []string, _, atomic bool) error {
19621972
require.NotEmpty(t, branches)
1973+
if tc.reorder {
1974+
assert.Equal(t, "main", prs[target].BaseRefName, "detach the reversed PR base before pushing")
1975+
assert.Equal(t, !tc.noAtomic, atomic)
1976+
}
19631977
pushes = append(pushes, branches...)
19641978
if tc.failure == "push" && slices.Contains(branches, target) {
19651979
return assert.AnError
@@ -2050,19 +2064,35 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
20502064
},
20512065
UpdatePRBaseFn: func(number int, base string) error {
20522066
baseUpdates++
2053-
if tc.failure == "base" {
2067+
if tc.failure == "base" || tc.failure == "prepare" {
20542068
return assert.AnError
20552069
}
2056-
require.Equal(t, prs[target].Number, number)
2057-
prs[target].BaseRefName = base
2070+
if !tc.reorder {
2071+
require.Equal(t, prs[target].Number, number)
2072+
}
2073+
for _, pr := range prs {
2074+
if pr.Number == number {
2075+
if tc.reorder {
2076+
assert.False(t, pr.IsAutoMergeEnabled(), "disable auto-merge before changing bases")
2077+
}
2078+
pr.BaseRefName = base
2079+
}
2080+
}
20582081
if tc.failure == "save" {
20592082
var err error
20602083
catalogLock, err = stack.Lock(dir)
20612084
require.NoError(t, err)
20622085
}
20632086
return nil
20642087
},
2065-
DisableAutoMergeFn: func(string) error { return assert.AnError },
2088+
DisableAutoMergeFn: func(id string) error {
2089+
if tc.failure == "auto-merge" {
2090+
return assert.AnError
2091+
}
2092+
require.Equal(t, prs[target].ID, id)
2093+
prs[target].AutoMergeRequest = nil
2094+
return nil
2095+
},
20662096
MarkPRReadyForReviewFn: func(string) error { return assert.AnError },
20672097
CreateStackFn: func(numbers []int) (*github.RemoteStack, error) {
20682098
require.GreaterOrEqual(t, len(numbers), 2)
@@ -2077,7 +2107,7 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
20772107
cfg, outR, errR := config.NewTestConfig()
20782108
cfg.GitHubClientOverride = client
20792109

2080-
err := runSubmit(cfg, &submitOptions{auto: true, open: tc.failure == "ready"})
2110+
err := runSubmit(cfg, &submitOptions{auto: true, open: tc.failure == "ready", noAtomic: tc.noAtomic})
20812111

20822112
_, output := commandOutput(t, cfg, outR, errR)
20832113
if tc.failure != "" {
@@ -2090,6 +2120,9 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
20902120
assert.NotContains(t, output, "Stack recreated")
20912121
assert.NotContains(t, output, "Changes from modify submitted")
20922122
assert.Nil(t, replacement, "an incomplete modification must not create a partial replacement stack")
2123+
if tc.failure == "prepare" {
2124+
assert.Empty(t, pushes, "do not push unless all reversed bases were detached")
2125+
}
20932126
} else {
20942127
require.NoError(t, err, output)
20952128
assert.NoFileExists(t, modify.StatePath(dir))
@@ -2113,6 +2146,8 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
21132146
assert.Contains(t, output, "Changes from modify submitted")
21142147
if tc.newPR {
21152148
assert.Equal(t, 1, createdPRs)
2149+
} else if tc.reorder {
2150+
assert.Equal(t, 3, baseUpdates)
21162151
} else if !tc.mergeAfterUnstack {
21172152
assert.Equal(t, 1, baseUpdates)
21182153
}
@@ -2129,7 +2164,7 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
21292164
pushStart := len(pushes)
21302165
retryCfg, retryOut, retryErr := config.NewTestConfig()
21312166
retryCfg.GitHubClientOverride = client
2132-
err := runSubmit(retryCfg, &submitOptions{auto: true})
2167+
err := runSubmit(retryCfg, &submitOptions{auto: true, noAtomic: tc.noAtomic})
21332168
_, diagnostics := commandOutput(t, retryCfg, retryOut, retryErr)
21342169
require.NoError(t, err, diagnostics)
21352170
assert.NoFileExists(t, modify.StatePath(dir))
@@ -2153,6 +2188,12 @@ func TestSubmit_PendingModifyCompletion(t *testing.T) {
21532188
} else {
21542189
assert.Equal(t, 1, unstackCalls)
21552190
}
2191+
if tc.reorder {
2192+
assert.Equal(t, "b1", prs["b3"].BaseRefName)
2193+
assert.Equal(t, "b3", prs["b2"].BaseRefName)
2194+
require.NotNil(t, replacement)
2195+
assert.Equal(t, []int{10, 12, 11}, replacement.PullRequests)
2196+
}
21562197
})
21572198
}
21582199
}
@@ -2540,6 +2581,7 @@ func TestClearPendingModifyState_NoFile(t *testing.T) {
25402581
}
25412582

25422583
func TestSubmit_WithPendingModify_AtomicPush(t *testing.T) {
2584+
t.Setenv("GH_STACK_NO_ATOMIC", "")
25432585
s := stack.Stack{
25442586
ID: "42",
25452587
Number: 7,

‎cmd/sync_test.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,7 @@ func newSyncMock(tmpDir string, currentBranch string) *git.MockOps {
7676
// TestSync_TrunkAlreadyUpToDate verifies that when trunk and origin/trunk have
7777
// the same SHA, no rebase occurs and push is normal (not force).
7878
func TestSync_TrunkAlreadyUpToDate(t *testing.T) {
79+
t.Setenv("GH_STACK_NO_ATOMIC", "")
7980
s := stack.Stack{
8081
Trunk: stack.BranchRef{Branch: "main"},
8182
Branches: []stack.BranchRef{
@@ -642,6 +643,7 @@ func TestSync_NoRebaseWhenTrunkDidntMove(t *testing.T) {
642643
// TestSync_PushForceFlagDependsOnRebase verifies that the force flag on Push
643644
// correlates with whether a rebase actually happened.
644645
func TestSync_PushForceFlagDependsOnRebase(t *testing.T) {
646+
t.Setenv("GH_STACK_NO_ATOMIC", "")
645647
tests := []struct {
646648
name string
647649
trunkMoved bool

‎docs/src/content/docs/reference/cli.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ gh stack submit [flags]
350350
| `--remote <name>` | Remote to push to (defaults to auto-detected remote) |
351351
| `--no-atomic` | Allow partial branch updates |
352352

353-
Pushes all active branches together atomically before creating or updating PRs. After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs are added to the existing stack. Atomicity applies to Git branch updates, not PR or stack API operations.
353+
Pushes all active branches together atomically before creating PRs or setting their final bases. For pending reorders, at-risk PRs temporarily target the trunk while branches are pushed. After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs are added to the existing stack. Atomicity applies to Git branch updates, not PR or stack API operations.
354354

355355
If every PR in the stack has already been merged, that stack is complete and can't be extended. In that case `submit` automatically starts a **new** stack rooted at the trunk for your unmerged branches and creates it on GitHub, leaving the merged stack untouched.
356356

0 commit comments

Comments
 (0)