From b3250b977317d97ef5b388b037a6c48020b04861 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 10 Jul 2026 12:08:14 -0400 Subject: [PATCH] Fix link failing when an existing stack PR is queued for merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- cmd/link.go | 112 +++++++++++++++++----------- cmd/link_test.go | 190 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 258 insertions(+), 44 deletions(-) diff --git a/cmd/link.go b/cmd/link.go index e74e545..93989f0 100644 --- a/cmd/link.go +++ b/cmd/link.go @@ -104,15 +104,9 @@ func runLink(cfg *config.Config, opts *linkOptions, args []string) error { return err } - // Phase 2b: Validate that all found PRs are eligible to be added to a stack. - // Only open/draft PRs without auto-merge enabled are allowed. - if err := validatePREligibility(cfg, found); err != nil { - return err - } - - // Phase 3: Pre-validate the stack — check that adding these PRs won't - // conflict with existing stacks before creating any new PRs. - // Also fetches stacks for reuse in the upsert phase. + // Phase 2b: Fetch existing stacks first so eligibility validation and + // stack pre-validation can account for PRs that are already members of + // the target stack. The stacks are also reused in the upsert phase. knownPRNumbers := make([]int, 0, len(found)) for _, r := range found { if r != nil { @@ -124,8 +118,28 @@ func runLink(cfg *config.Config, opts *linkOptions, args []string) error { if err != nil { return err } - if len(knownPRNumbers) > 0 { - if err := prevalidateStack(cfg, stacks, knownPRNumbers); err != nil { + + // Determine the stack these PRs already belong to (if any). PRs that are + // already members of this stack are exempt from the eligibility checks + // below, since they are not being added — they are already present. + targetStack, err := findMatchingStack(stacks, knownPRNumbers) + if err != nil { + cfg.Errorf("%s", err) + return ErrDisambiguate + } + + // Validate that all found PRs are eligible to be added to a stack. Only + // open/draft PRs without auto-merge enabled are allowed, except for PRs + // already in the target stack. + if err := validatePREligibility(cfg, found, targetStack); err != nil { + return err + } + + // Phase 3: Pre-validate the stack — check that adding these PRs won't + // drop existing PRs from the target stack before creating any new PRs, + // so we can fail early without leaving orphaned PRs. + if targetStack != nil { + if err := prevalidateStack(cfg, targetStack, knownPRNumbers); err != nil { return err } } @@ -299,13 +313,29 @@ func findExistingPR(cfg *config.Config, client github.ClientOps, arg string) (*r // validatePREligibility checks that all found PRs are eligible to be added // to a stack. Only open or draft PRs without auto-merge enabled are allowed. // Merged, closed, queued, and auto-merge-enabled PRs are rejected. -// Reports all invalid PRs at once before returning. -func validatePREligibility(cfg *config.Config, found []*resolvedArg) error { +// +// PRs that are already members of targetStack are exempt from these checks: +// they are not being added (they are already present), so re-including them — +// as an additive update requires — must not fail the operation. Reports all +// invalid PRs at once before returning. +func validatePREligibility(cfg *config.Config, found []*resolvedArg, targetStack *github.RemoteStack) error { + inTargetStack := make(map[int]bool) + if targetStack != nil { + for _, n := range targetStack.PullRequests { + inTargetStack[n] = true + } + } + invalid := 0 for _, r := range found { if r == nil || r.pr == nil { continue } + // PRs already in the target stack are not being added, so the + // eligibility checks below do not apply to them. + if inTargetStack[r.prNumber] { + continue + } pr := r.pr reason := "" switch { @@ -345,41 +375,35 @@ func listStacksSafe(cfg *config.Config, client github.ClientOps) ([]github.Remot return stacks, nil } -// prevalidateStack checks whether the known PRs would conflict with -// existing stacks. This runs before creating new PRs so we can fail -// early without leaving orphaned PRs. -func prevalidateStack(cfg *config.Config, stacks []github.RemoteStack, knownPRNumbers []int) error { - matchedStack, err := findMatchingStack(stacks, knownPRNumbers) - if err != nil { - cfg.Errorf("%s", err) - return ErrDisambiguate +// prevalidateStack checks whether adding the known PRs to the matched target +// stack would remove any of the stack's existing PRs. This runs before creating +// new PRs so we can fail early without leaving orphaned PRs. The caller is +// responsible for passing a non-nil matchedStack (the result of +// findMatchingStack); when no stack matches there is nothing to pre-validate. +func prevalidateStack(cfg *config.Config, matchedStack *github.RemoteStack, knownPRNumbers []int) error { + // Check that we won't be removing PRs from the existing stack. + // At this point we only have the known PR numbers (existing PRs). + // New PRs will be created later and added. Since new PRs can't + // match existing stack PRs (they don't exist yet), we just need + // to check that all existing stack PRs are in the known set. + knownSet := make(map[int]bool, len(knownPRNumbers)) + for _, n := range knownPRNumbers { + knownSet[n] = true } - if matchedStack != nil { - // Check that we won't be removing PRs from the existing stack. - // At this point we only have the known PR numbers (existing PRs). - // New PRs will be created later and added. Since new PRs can't - // match existing stack PRs (they don't exist yet), we just need - // to check that all existing stack PRs are in the known set. - knownSet := make(map[int]bool, len(knownPRNumbers)) - for _, n := range knownPRNumbers { - knownSet[n] = true - } - - var dropped []int - for _, n := range matchedStack.PullRequests { - if !knownSet[n] { - dropped = append(dropped, n) - } + var dropped []int + for _, n := range matchedStack.PullRequests { + if !knownSet[n] { + dropped = append(dropped, n) } + } - if len(dropped) > 0 { - cfg.Errorf("Cannot update stack: this would remove %s from the stack", - formatPRList(dropped)) - cfg.Printf("Current stack: %s", formatPRList(matchedStack.PullRequests)) - cfg.Printf("Include all existing PRs in the command to update the stack") - return ErrInvalidArgs - } + if len(dropped) > 0 { + cfg.Errorf("Cannot update stack: this would remove %s from the stack", + formatPRList(dropped)) + cfg.Printf("Current stack: %s", formatPRList(matchedStack.PullRequests)) + cfg.Printf("Include all existing PRs in the command to update the stack") + return ErrInvalidArgs } return nil diff --git a/cmd/link_test.go b/cmd/link_test.go index e740b74..7c6ccdd 100644 --- a/cmd/link_test.go +++ b/cmd/link_test.go @@ -558,6 +558,196 @@ func TestLink_ReportsMultipleIneligiblePRs(t *testing.T) { assert.Contains(t, output, "auto-merge") } +// Regression test to ensure a queued PR that is already a member of +// the target stack does not block adding new PRs to that same stack. +func TestLink_AllowsQueuedPRAlreadyInStack(t *testing.T) { + var updatedID string + var updatedPRs []int + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + pr := &github.PullRequest{ + Number: n, + State: "OPEN", + HeadRefName: fmt.Sprintf("branch-%d", n), + BaseRefName: "main", + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n), + } + // PR 100 is the queued bottom PR already in the stack. + if n == 100 { + pr.MergeQueueEntry = &github.MergeQueueEntry{ID: "MQE_100"} + } + return pr, nil + }, + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{ + {ID: 7, PullRequests: []int{100}}, + }, nil + }, + UpdateStackFn: func(stackID string, prNumbers []int) error { + updatedID = stackID + updatedPRs = prNumbers + return nil + }, + CreateStackFn: func([]int) (int, error) { + t.Fatal("CreateStack should not be called when updating an existing stack") + return 0, nil + }, + } + + cmd := LinkCmd(cfg) + cmd.SetArgs([]string{"100", "101", "102"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + require.NoError(t, err) + assert.Equal(t, "7", updatedID) + assert.Equal(t, []int{100, 101, 102}, updatedPRs) + assert.NotContains(t, output, "cannot be added to a stack") +} + +// TestLink_AllowsMergedPRAlreadyInStack verifies the exemption also covers +// state-based ineligibility (merged/closed) for PRs already in the stack. +func TestLink_AllowsMergedPRAlreadyInStack(t *testing.T) { + var updatedPRs []int + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + pr := &github.PullRequest{ + Number: n, + State: "OPEN", + HeadRefName: fmt.Sprintf("branch-%d", n), + BaseRefName: "main", + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n), + } + if n == 100 { + pr.State = "MERGED" + pr.Merged = true + } + return pr, nil + }, + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{ + {ID: 8, PullRequests: []int{100}}, + }, nil + }, + UpdateStackFn: func(_ string, prNumbers []int) error { + updatedPRs = prNumbers + return nil + }, + } + + cmd := LinkCmd(cfg) + cmd.SetArgs([]string{"100", "101"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + require.NoError(t, err) + assert.Equal(t, []int{100, 101}, updatedPRs) + assert.NotContains(t, output, "cannot be added to a stack") +} + +// TestLink_AllowsAutoMergePRAlreadyInStack verifies the exemption also covers +// auto-merge-enabled PRs already in the stack. +func TestLink_AllowsAutoMergePRAlreadyInStack(t *testing.T) { + var updatedPRs []int + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + pr := &github.PullRequest{ + Number: n, + State: "OPEN", + HeadRefName: fmt.Sprintf("branch-%d", n), + BaseRefName: "main", + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n), + } + if n == 100 { + pr.AutoMergeRequest = &github.AutoMergeRequest{EnabledAt: "2024-01-01T00:00:00Z"} + } + return pr, nil + }, + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{ + {ID: 9, PullRequests: []int{100}}, + }, nil + }, + UpdateStackFn: func(_ string, prNumbers []int) error { + updatedPRs = prNumbers + return nil + }, + } + + cmd := LinkCmd(cfg) + cmd.SetArgs([]string{"100", "101"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + require.NoError(t, err) + assert.Equal(t, []int{100, 101}, updatedPRs) + assert.NotContains(t, output, "cannot be added to a stack") +} + +// TestLink_RejectsQueuedPRNotInStack_WhenAddingToExistingStack confirms the +// exemption is scoped correctly: a queued PR that is NOT already a member of the +// matched stack is still rejected, even when the command targets that stack. +func TestLink_RejectsQueuedPRNotInStack_WhenAddingToExistingStack(t *testing.T) { + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRByNumberFn: func(n int) (*github.PullRequest, error) { + pr := &github.PullRequest{ + Number: n, + State: "OPEN", + HeadRefName: fmt.Sprintf("branch-%d", n), + BaseRefName: "main", + URL: fmt.Sprintf("https://github.com/o/r/pull/%d", n), + } + // PR 200 is queued and is NOT part of the existing stack. + if n == 200 { + pr.MergeQueueEntry = &github.MergeQueueEntry{ID: "MQE_200"} + } + return pr, nil + }, + ListStacksFn: func() ([]github.RemoteStack, error) { + return []github.RemoteStack{ + {ID: 7, PullRequests: []int{100}}, + }, nil + }, + UpdateStackFn: func(string, []int) error { + t.Fatal("UpdateStack should not be called when a new PR is ineligible") + return nil + }, + } + + cmd := LinkCmd(cfg) + cmd.SetArgs([]string{"100", "200"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "cannot be added to a stack") + assert.Contains(t, output, "queued for merge") +} + // --- Branch name tests --- func TestLink_BranchNames_AllHavePRs(t *testing.T) {