Skip to content

Commit b3250b9

Browse files
skarimskarim
committed
Fix link failing when an existing stack PR is queued for merge
gh stack link validated PR eligibility before it fetched the repository's stacks, so validatePREligibility rejected any queued PR unconditionally — including one already a member of the stack being updated. Because link is additive (every existing stack PR must be re-listed or the update is refused for dropping them), a stack whose bottom PR was in the merge queue could never take new PRs on top: re-listing the queued PR failed eligibility, and omitting it failed the drop check. Fetch the stacks and resolve the target stack before validating eligibility, then skip the eligibility checks (queued, auto-merge, merged, closed) for any PR that is already a member of that stack — those PRs are not being added, so the checks don't apply. PRs not already in the stack, and brand-new stacks, keep the previous strict behavior. prevalidateStack now takes the resolved stack instead of looking it up a second time. Add coverage for linking new PRs onto a stack whose existing PR is queued, merged, or has auto-merge enabled, and for still rejecting a queued PR that is not yet part of the target stack.
1 parent acf1fec commit b3250b9

2 files changed

Lines changed: 258 additions & 44 deletions

File tree

‎cmd/link.go‎

Lines changed: 68 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -104,15 +104,9 @@ func runLink(cfg *config.Config, opts *linkOptions, args []string) error {
104104
return err
105105
}
106106

107-
// Phase 2b: Validate that all found PRs are eligible to be added to a stack.
108-
// Only open/draft PRs without auto-merge enabled are allowed.
109-
if err := validatePREligibility(cfg, found); err != nil {
110-
return err
111-
}
112-
113-
// Phase 3: Pre-validate the stack — check that adding these PRs won't
114-
// conflict with existing stacks before creating any new PRs.
115-
// Also fetches stacks for reuse in the upsert phase.
107+
// Phase 2b: Fetch existing stacks first so eligibility validation and
108+
// stack pre-validation can account for PRs that are already members of
109+
// the target stack. The stacks are also reused in the upsert phase.
116110
knownPRNumbers := make([]int, 0, len(found))
117111
for _, r := range found {
118112
if r != nil {
@@ -124,8 +118,28 @@ func runLink(cfg *config.Config, opts *linkOptions, args []string) error {
124118
if err != nil {
125119
return err
126120
}
127-
if len(knownPRNumbers) > 0 {
128-
if err := prevalidateStack(cfg, stacks, knownPRNumbers); err != nil {
121+
122+
// Determine the stack these PRs already belong to (if any). PRs that are
123+
// already members of this stack are exempt from the eligibility checks
124+
// below, since they are not being added — they are already present.
125+
targetStack, err := findMatchingStack(stacks, knownPRNumbers)
126+
if err != nil {
127+
cfg.Errorf("%s", err)
128+
return ErrDisambiguate
129+
}
130+
131+
// Validate that all found PRs are eligible to be added to a stack. Only
132+
// open/draft PRs without auto-merge enabled are allowed, except for PRs
133+
// already in the target stack.
134+
if err := validatePREligibility(cfg, found, targetStack); err != nil {
135+
return err
136+
}
137+
138+
// Phase 3: Pre-validate the stack — check that adding these PRs won't
139+
// drop existing PRs from the target stack before creating any new PRs,
140+
// so we can fail early without leaving orphaned PRs.
141+
if targetStack != nil {
142+
if err := prevalidateStack(cfg, targetStack, knownPRNumbers); err != nil {
129143
return err
130144
}
131145
}
@@ -299,13 +313,29 @@ func findExistingPR(cfg *config.Config, client github.ClientOps, arg string) (*r
299313
// validatePREligibility checks that all found PRs are eligible to be added
300314
// to a stack. Only open or draft PRs without auto-merge enabled are allowed.
301315
// Merged, closed, queued, and auto-merge-enabled PRs are rejected.
302-
// Reports all invalid PRs at once before returning.
303-
func validatePREligibility(cfg *config.Config, found []*resolvedArg) error {
316+
//
317+
// PRs that are already members of targetStack are exempt from these checks:
318+
// they are not being added (they are already present), so re-including them —
319+
// as an additive update requires — must not fail the operation. Reports all
320+
// invalid PRs at once before returning.
321+
func validatePREligibility(cfg *config.Config, found []*resolvedArg, targetStack *github.RemoteStack) error {
322+
inTargetStack := make(map[int]bool)
323+
if targetStack != nil {
324+
for _, n := range targetStack.PullRequests {
325+
inTargetStack[n] = true
326+
}
327+
}
328+
304329
invalid := 0
305330
for _, r := range found {
306331
if r == nil || r.pr == nil {
307332
continue
308333
}
334+
// PRs already in the target stack are not being added, so the
335+
// eligibility checks below do not apply to them.
336+
if inTargetStack[r.prNumber] {
337+
continue
338+
}
309339
pr := r.pr
310340
reason := ""
311341
switch {
@@ -345,41 +375,35 @@ func listStacksSafe(cfg *config.Config, client github.ClientOps) ([]github.Remot
345375
return stacks, nil
346376
}
347377

348-
// prevalidateStack checks whether the known PRs would conflict with
349-
// existing stacks. This runs before creating new PRs so we can fail
350-
// early without leaving orphaned PRs.
351-
func prevalidateStack(cfg *config.Config, stacks []github.RemoteStack, knownPRNumbers []int) error {
352-
matchedStack, err := findMatchingStack(stacks, knownPRNumbers)
353-
if err != nil {
354-
cfg.Errorf("%s", err)
355-
return ErrDisambiguate
378+
// prevalidateStack checks whether adding the known PRs to the matched target
379+
// stack would remove any of the stack's existing PRs. This runs before creating
380+
// new PRs so we can fail early without leaving orphaned PRs. The caller is
381+
// responsible for passing a non-nil matchedStack (the result of
382+
// findMatchingStack); when no stack matches there is nothing to pre-validate.
383+
func prevalidateStack(cfg *config.Config, matchedStack *github.RemoteStack, knownPRNumbers []int) error {
384+
// Check that we won't be removing PRs from the existing stack.
385+
// At this point we only have the known PR numbers (existing PRs).
386+
// New PRs will be created later and added. Since new PRs can't
387+
// match existing stack PRs (they don't exist yet), we just need
388+
// to check that all existing stack PRs are in the known set.
389+
knownSet := make(map[int]bool, len(knownPRNumbers))
390+
for _, n := range knownPRNumbers {
391+
knownSet[n] = true
356392
}
357393

358-
if matchedStack != nil {
359-
// Check that we won't be removing PRs from the existing stack.
360-
// At this point we only have the known PR numbers (existing PRs).
361-
// New PRs will be created later and added. Since new PRs can't
362-
// match existing stack PRs (they don't exist yet), we just need
363-
// to check that all existing stack PRs are in the known set.
364-
knownSet := make(map[int]bool, len(knownPRNumbers))
365-
for _, n := range knownPRNumbers {
366-
knownSet[n] = true
367-
}
368-
369-
var dropped []int
370-
for _, n := range matchedStack.PullRequests {
371-
if !knownSet[n] {
372-
dropped = append(dropped, n)
373-
}
394+
var dropped []int
395+
for _, n := range matchedStack.PullRequests {
396+
if !knownSet[n] {
397+
dropped = append(dropped, n)
374398
}
399+
}
375400

376-
if len(dropped) > 0 {
377-
cfg.Errorf("Cannot update stack: this would remove %s from the stack",
378-
formatPRList(dropped))
379-
cfg.Printf("Current stack: %s", formatPRList(matchedStack.PullRequests))
380-
cfg.Printf("Include all existing PRs in the command to update the stack")
381-
return ErrInvalidArgs
382-
}
401+
if len(dropped) > 0 {
402+
cfg.Errorf("Cannot update stack: this would remove %s from the stack",
403+
formatPRList(dropped))
404+
cfg.Printf("Current stack: %s", formatPRList(matchedStack.PullRequests))
405+
cfg.Printf("Include all existing PRs in the command to update the stack")
406+
return ErrInvalidArgs
383407
}
384408

385409
return nil

‎cmd/link_test.go‎

Lines changed: 190 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -558,6 +558,196 @@ func TestLink_ReportsMultipleIneligiblePRs(t *testing.T) {
558558
assert.Contains(t, output, "auto-merge")
559559
}
560560

561+
// Regression test to ensure a queued PR that is already a member of
562+
// the target stack does not block adding new PRs to that same stack.
563+
func TestLink_AllowsQueuedPRAlreadyInStack(t *testing.T) {
564+
var updatedID string
565+
var updatedPRs []int
566+
cfg, _, errR := config.NewTestConfig()
567+
cfg.GitHubClientOverride = &github.MockClient{
568+
FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
569+
pr := &github.PullRequest{
570+
Number: n,
571+
State: "OPEN",
572+
HeadRefName: fmt.Sprintf("branch-%d", n),
573+
BaseRefName: "main",
574+
URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n),
575+
}
576+
// PR 100 is the queued bottom PR already in the stack.
577+
if n == 100 {
578+
pr.MergeQueueEntry = &github.MergeQueueEntry{ID: "MQE_100"}
579+
}
580+
return pr, nil
581+
},
582+
ListStacksFn: func() ([]github.RemoteStack, error) {
583+
return []github.RemoteStack{
584+
{ID: 7, PullRequests: []int{100}},
585+
}, nil
586+
},
587+
UpdateStackFn: func(stackID string, prNumbers []int) error {
588+
updatedID = stackID
589+
updatedPRs = prNumbers
590+
return nil
591+
},
592+
CreateStackFn: func([]int) (int, error) {
593+
t.Fatal("CreateStack should not be called when updating an existing stack")
594+
return 0, nil
595+
},
596+
}
597+
598+
cmd := LinkCmd(cfg)
599+
cmd.SetArgs([]string{"100", "101", "102"})
600+
cmd.SetOut(io.Discard)
601+
cmd.SetErr(io.Discard)
602+
err := cmd.Execute()
603+
604+
cfg.Err.Close()
605+
errOut, _ := io.ReadAll(errR)
606+
output := string(errOut)
607+
608+
require.NoError(t, err)
609+
assert.Equal(t, "7", updatedID)
610+
assert.Equal(t, []int{100, 101, 102}, updatedPRs)
611+
assert.NotContains(t, output, "cannot be added to a stack")
612+
}
613+
614+
// TestLink_AllowsMergedPRAlreadyInStack verifies the exemption also covers
615+
// state-based ineligibility (merged/closed) for PRs already in the stack.
616+
func TestLink_AllowsMergedPRAlreadyInStack(t *testing.T) {
617+
var updatedPRs []int
618+
cfg, _, errR := config.NewTestConfig()
619+
cfg.GitHubClientOverride = &github.MockClient{
620+
FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
621+
pr := &github.PullRequest{
622+
Number: n,
623+
State: "OPEN",
624+
HeadRefName: fmt.Sprintf("branch-%d", n),
625+
BaseRefName: "main",
626+
URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n),
627+
}
628+
if n == 100 {
629+
pr.State = "MERGED"
630+
pr.Merged = true
631+
}
632+
return pr, nil
633+
},
634+
ListStacksFn: func() ([]github.RemoteStack, error) {
635+
return []github.RemoteStack{
636+
{ID: 8, PullRequests: []int{100}},
637+
}, nil
638+
},
639+
UpdateStackFn: func(_ string, prNumbers []int) error {
640+
updatedPRs = prNumbers
641+
return nil
642+
},
643+
}
644+
645+
cmd := LinkCmd(cfg)
646+
cmd.SetArgs([]string{"100", "101"})
647+
cmd.SetOut(io.Discard)
648+
cmd.SetErr(io.Discard)
649+
err := cmd.Execute()
650+
651+
cfg.Err.Close()
652+
errOut, _ := io.ReadAll(errR)
653+
output := string(errOut)
654+
655+
require.NoError(t, err)
656+
assert.Equal(t, []int{100, 101}, updatedPRs)
657+
assert.NotContains(t, output, "cannot be added to a stack")
658+
}
659+
660+
// TestLink_AllowsAutoMergePRAlreadyInStack verifies the exemption also covers
661+
// auto-merge-enabled PRs already in the stack.
662+
func TestLink_AllowsAutoMergePRAlreadyInStack(t *testing.T) {
663+
var updatedPRs []int
664+
cfg, _, errR := config.NewTestConfig()
665+
cfg.GitHubClientOverride = &github.MockClient{
666+
FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
667+
pr := &github.PullRequest{
668+
Number: n,
669+
State: "OPEN",
670+
HeadRefName: fmt.Sprintf("branch-%d", n),
671+
BaseRefName: "main",
672+
URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n),
673+
}
674+
if n == 100 {
675+
pr.AutoMergeRequest = &github.AutoMergeRequest{EnabledAt: "2024-01-01T00:00:00Z"}
676+
}
677+
return pr, nil
678+
},
679+
ListStacksFn: func() ([]github.RemoteStack, error) {
680+
return []github.RemoteStack{
681+
{ID: 9, PullRequests: []int{100}},
682+
}, nil
683+
},
684+
UpdateStackFn: func(_ string, prNumbers []int) error {
685+
updatedPRs = prNumbers
686+
return nil
687+
},
688+
}
689+
690+
cmd := LinkCmd(cfg)
691+
cmd.SetArgs([]string{"100", "101"})
692+
cmd.SetOut(io.Discard)
693+
cmd.SetErr(io.Discard)
694+
err := cmd.Execute()
695+
696+
cfg.Err.Close()
697+
errOut, _ := io.ReadAll(errR)
698+
output := string(errOut)
699+
700+
require.NoError(t, err)
701+
assert.Equal(t, []int{100, 101}, updatedPRs)
702+
assert.NotContains(t, output, "cannot be added to a stack")
703+
}
704+
705+
// TestLink_RejectsQueuedPRNotInStack_WhenAddingToExistingStack confirms the
706+
// exemption is scoped correctly: a queued PR that is NOT already a member of the
707+
// matched stack is still rejected, even when the command targets that stack.
708+
func TestLink_RejectsQueuedPRNotInStack_WhenAddingToExistingStack(t *testing.T) {
709+
cfg, _, errR := config.NewTestConfig()
710+
cfg.GitHubClientOverride = &github.MockClient{
711+
FindPRByNumberFn: func(n int) (*github.PullRequest, error) {
712+
pr := &github.PullRequest{
713+
Number: n,
714+
State: "OPEN",
715+
HeadRefName: fmt.Sprintf("branch-%d", n),
716+
BaseRefName: "main",
717+
URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n),
718+
}
719+
// PR 200 is queued and is NOT part of the existing stack.
720+
if n == 200 {
721+
pr.MergeQueueEntry = &github.MergeQueueEntry{ID: "MQE_200"}
722+
}
723+
return pr, nil
724+
},
725+
ListStacksFn: func() ([]github.RemoteStack, error) {
726+
return []github.RemoteStack{
727+
{ID: 7, PullRequests: []int{100}},
728+
}, nil
729+
},
730+
UpdateStackFn: func(string, []int) error {
731+
t.Fatal("UpdateStack should not be called when a new PR is ineligible")
732+
return nil
733+
},
734+
}
735+
736+
cmd := LinkCmd(cfg)
737+
cmd.SetArgs([]string{"100", "200"})
738+
cmd.SetOut(io.Discard)
739+
cmd.SetErr(io.Discard)
740+
err := cmd.Execute()
741+
742+
cfg.Err.Close()
743+
errOut, _ := io.ReadAll(errR)
744+
output := string(errOut)
745+
746+
assert.ErrorIs(t, err, ErrInvalidArgs)
747+
assert.Contains(t, output, "cannot be added to a stack")
748+
assert.Contains(t, output, "queued for merge")
749+
}
750+
561751
// --- Branch name tests ---
562752

563753
func TestLink_BranchNames_AllHavePRs(t *testing.T) {

0 commit comments

Comments
 (0)