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) {