From 35a6ade210544736794827e47eb09767f1a151e6 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 22 Jul 2026 00:07:01 -0400 Subject: [PATCH 01/11] merge cmd --- cmd/merge.go | 578 +++++++++++++++++++++++++++ cmd/merge_test.go | 469 ++++++++++++++++++++++ cmd/root.go | 4 + cmd/root_test.go | 2 +- internal/github/client_interface.go | 3 + internal/github/merge_async.go | 247 ++++++++++++ internal/github/merge_async_test.go | 193 +++++++++ internal/github/mock_client.go | 44 ++ internal/tui/mergeview/model.go | 366 +++++++++++++++++ internal/tui/mergeview/model_test.go | 243 +++++++++++ internal/tui/mergeview/types.go | 109 +++++ internal/tui/mergeview/view.go | 271 +++++++++++++ 12 files changed, 2528 insertions(+), 1 deletion(-) create mode 100644 cmd/merge.go create mode 100644 cmd/merge_test.go create mode 100644 internal/github/merge_async.go create mode 100644 internal/github/merge_async_test.go create mode 100644 internal/tui/mergeview/model.go create mode 100644 internal/tui/mergeview/model_test.go create mode 100644 internal/tui/mergeview/types.go create mode 100644 internal/tui/mergeview/view.go diff --git a/cmd/merge.go b/cmd/merge.go new file mode 100644 index 00000000..f285338d --- /dev/null +++ b/cmd/merge.go @@ -0,0 +1,578 @@ +package cmd + +import ( + "errors" + "fmt" + "net/http" + "strconv" + "strings" + "time" + + tea "github.com/charmbracelet/bubbletea" + "github.com/cli/go-gh/v2/pkg/api" + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/github" + "github.com/github/gh-stack/internal/stack" + "github.com/github/gh-stack/internal/tui/mergeview" + "github.com/spf13/cobra" +) + +type mergeOptions struct { + mergeMethod string + squash bool + rebase bool + merge bool + yes bool + + // pollInterval and maxPolls control the status polling loop in the + // non-interactive path. Zero values fall back to sane defaults; tests set + // them to keep runs fast. + pollInterval time.Duration + maxPolls int +} + +// mergeTarget describes an explicitly requested pull request to merge up to. +type mergeTarget struct { + prNumber int + hasPR bool +} + +// MergeCmd builds the `gh stack merge` command. +func MergeCmd(cfg *config.Config) *cobra.Command { + opts := &mergeOptions{} + + cmd := &cobra.Command{ + Use: "merge [ | ]", + Short: "Merge a stack of pull requests", + Long: `Merge some or all of a stack of pull requests using GitHub's atomic stack +merge. All members of the stack up to and including your chosen pull request are +merged into the base branch in a single, all-or-nothing operation: if any PR +cannot be merged, none are. + +With no argument, the stack for the current branch is used. Pass a stack number +to merge a stack you don't have checked out, or a pull request number to merge +directly up to that PR. A bare number is treated first as a stack number, then +as a pull request number. + +In an interactive terminal, a short wizard lets you choose how far up the stack +to merge (everything below your selection is always included), pick the merge +method, and confirm, then shows live progress. In a non-interactive terminal, or +with --yes, the whole stack (or everything up to the given PR) is merged without +prompting, using your last-used merge method unless one is specified. + +Only basic pull request state is checked before merging (open and not a draft); +GitHub evaluates branch protection and repository rules when the merge runs, so +any such failure is reported back to you.`, + Example: ` # Merge the current stack (interactive picker) + $ gh stack merge + + # Merge a stack you don't have checked out, by stack number + $ gh stack merge 7 + + # Merge everything up to and including PR #42 + $ gh stack merge 42 + + # Merge the whole current stack without prompting, squashing + $ gh stack merge --yes --squash`, + Args: cobra.MaximumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + return runMerge(cfg, opts, args) + }, + } + + cmd.Flags().StringVar(&opts.mergeMethod, "merge-method", "", "Merge method to use: merge, squash, or rebase") + cmd.Flags().BoolVar(&opts.merge, "merge", false, "Merge with a merge commit") + cmd.Flags().BoolVar(&opts.squash, "squash", false, "Squash and merge") + cmd.Flags().BoolVar(&opts.rebase, "rebase", false, "Rebase and merge") + cmd.Flags().BoolVarP(&opts.yes, "yes", "y", false, "Merge without prompting for confirmation") + + return cmd +} + +func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { + method, err := resolveMergeMethodFlag(opts) + if err != nil { + cfg.Errorf("%s", err) + return ErrInvalidArgs + } + + client, err := cfg.GitHubClient() + if err != nil { + cfg.Errorf("failed to create GitHub client: %s", err) + return ErrAPIFailure + } + + remoteStack, target, err := resolveMergeStack(cfg, client, args) + if err != nil { + return err + } + + candidates, blocker := mergeCandidates(remoteStack) + + preselectIndex := -1 + targetPR := 0 + if target.hasPR { + idx := indexOfPR(candidates, target.prNumber) + if idx < 0 { + return explainNonMergeableTarget(cfg, remoteStack, target.prNumber, blocker) + } + preselectIndex = idx + targetPR = candidates[idx].Number + } else if len(candidates) == 0 { + return explainNothingToMerge(cfg, remoteStack, blocker) + } + + mergeCfg, err := client.RepoMergeConfig() + if err != nil { + cfg.Errorf("failed to fetch repository merge settings: %s", err) + return ErrAPIFailure + } + allowed := mergeCfg.AllowedMethods() + if len(allowed) == 0 { + cfg.Errorf("this repository does not allow any merge methods") + return ErrAPIFailure + } + if method != "" && !mergeCfg.Allows(method) { + cfg.Errorf("this repository does not allow %s merges", method) + return ErrInvalidArgs + } + + base := remoteStack.Base.Ref + + if cfg.IsInteractive() && !opts.yes { + return runMergeInteractive(cfg, client, base, candidates, allowed, mergeCfg.DefaultMethod, method, preselectIndex, opts) + } + + // Non-interactive (or --yes): merge the whole stack (or up to the given PR) + // without prompting. + if !target.hasPR { + targetPR = candidates[len(candidates)-1].Number + } + if method == "" { + method = mergeCfg.DefaultMethod + if !mergeCfg.Allows(method) { + method = allowed[0] + } + } + return runMergeHeadless(cfg, client, base, candidates, targetPR, method, opts) +} + +// resolveMergeStack determines the remote stack (and any explicitly targeted PR) +// from the command arguments. It never reads local PR state: the local stack +// file is consulted only to discover the stack number when no argument is given. +func resolveMergeStack(cfg *config.Config, client github.ClientOps, args []string) (*github.RemoteStack, mergeTarget, error) { + if len(args) == 0 { + rs, err := resolveActiveRemoteStack(cfg, client) + return rs, mergeTarget{}, err + } + + n, err := strconv.Atoi(strings.TrimSpace(args[0])) + if err != nil || n <= 0 { + cfg.Errorf("invalid argument %q: expected a stack number or pull request number", args[0]) + return nil, mergeTarget{}, ErrInvalidArgs + } + + // Try as a stack number first (mirrors `gh stack checkout`). + rs, err := client.GetStack(n) + if err == nil && rs != nil { + return rs, mergeTarget{}, nil + } + if err != nil && !isNotFound(err) { + cfg.Errorf("failed to fetch stack #%d: %s", n, err) + return nil, mergeTarget{}, ErrAPIFailure + } + + // Not a stack number: try as a pull request number. + rs, err = client.FindStackForPR(n) + if err != nil { + if isNotFound(err) { + warnStacksUnavailable(cfg) + return nil, mergeTarget{}, ErrStacksUnavailable + } + cfg.Errorf("failed to look up pull request #%d: %s", n, err) + return nil, mergeTarget{}, ErrAPIFailure + } + if rs == nil { + cfg.Errorf("#%d is not a stack number or a stacked pull request", n) + return nil, mergeTarget{}, ErrNotInStack + } + return rs, mergeTarget{prNumber: n, hasPR: true}, nil +} + +// resolveActiveRemoteStack reads only the local stack number for the current +// branch, then fetches the full stack (and its PR states) from GitHub. +func resolveActiveRemoteStack(cfg *config.Config, client github.ClientOps) (*github.RemoteStack, error) { + gitDir, err := git.GitDir() + if err != nil { + cfg.Errorf("not a git repository") + return nil, ErrNotInStack + } + sf, err := stack.Load(gitDir) + if err != nil { + cfg.Errorf("failed to load stack state: %s", err) + return nil, ErrNotInStack + } + currentBranch, err := git.CurrentBranch() + if err != nil { + cfg.Errorf("failed to get current branch: %s", err) + return nil, ErrNotInStack + } + + stacks := sf.FindAllStacksForBranch(currentBranch) + if len(stacks) == 0 { + cfg.Errorf("current branch %q is not part of a stack", currentBranch) + return nil, ErrNotInStack + } + if len(stacks) > 1 { + cfg.Errorf("branch %q belongs to multiple stacks; check out a non-trunk branch first", currentBranch) + return nil, ErrDisambiguate + } + s := stacks[0] + if s.ID == "" && s.Number == 0 { + cfg.Errorf("this stack has not been submitted to GitHub yet; run `gh stack submit` first") + return nil, ErrNotInStack + } + + number, err := ensureStackNumber(client, s) + if err != nil { + if isNotFound(err) { + warnStacksUnavailable(cfg) + return nil, ErrStacksUnavailable + } + cfg.Errorf("failed to resolve stack number: %s", err) + return nil, ErrAPIFailure + } + if number == 0 { + cfg.Errorf("could not determine the stack number for the current stack") + return nil, ErrNotInStack + } + + rs, err := client.GetStack(number) + if err != nil { + if isNotFound(err) { + warnStacksUnavailable(cfg) + return nil, ErrStacksUnavailable + } + cfg.Errorf("failed to fetch stack #%d: %s", number, err) + return nil, ErrAPIFailure + } + return rs, nil +} + +func runMergeInteractive(cfg *config.Config, client github.ClientOps, base string, candidates []mergeview.PRItem, allowed []string, viewerDefault, methodFlag string, preselectIndex int, opts *mergeOptions) error { + defaultMethod := viewerDefault + if methodFlag != "" { + defaultMethod = methodFlag + } + + submit, poll := mergeFuncs(client) + + model := mergeview.New(mergeview.Options{ + PRs: candidates, + BaseRef: base, + AllowedMethods: allowed, + DefaultMethod: defaultMethod, + PreselectTopIndex: preselectIndex, + Submit: submit, + Poll: poll, + PollInterval: opts.pollInterval, + }) + + final, err := tea.NewProgram(model, tea.WithInput(cfg.In), tea.WithOutput(cfg.Out)).Run() + if err != nil { + cfg.Errorf("failed to run merge: %s", err) + return ErrSilent + } + + out := final.(mergeview.Model).Outcome() + switch { + case out.Err != nil: + if errors.Is(out.Err, github.ErrAsyncMergeUnavailable) { + warnAsyncMergeUnavailable(cfg) + return ErrStacksUnavailable + } + return ErrAPIFailure + case out.Merged: + return nil + case out.Failed: + return mergeFailureExit(out.Message) + default: + // Cancelled, or watching was stopped while the merge continued. + return ErrSilent + } +} + +func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, candidates []mergeview.PRItem, targetPR int, method string, opts *mergeOptions) error { + nums := numbersUpTo(candidates, targetPR) + list := prNumberList(nums) + + cfg.Printf("Merging %s into %s via %s...", list, base, method) + + res, err := client.MergeStackAsync(targetPR, method) + if err != nil { + if errors.Is(err, github.ErrAsyncMergeUnavailable) { + warnAsyncMergeUnavailable(cfg) + return ErrStacksUnavailable + } + cfg.Errorf("failed to start merge: %s", err) + return ErrAPIFailure + } + + if res.Merged { + cfg.Successf("Merged %s into %s", list, base) + return nil + } + if !res.Queued { + cfg.Errorf("cannot merge: %s", res.Details.Message) + return ErrAPIFailure + } + if res.StatusCode == http.StatusConflict { + cfg.Infof("A merge request already exists for this stack; tracking it.") + } + + uuid := res.Details.UUID + interval := opts.pollInterval + if interval <= 0 { + interval = time.Second + } + maxPolls := opts.maxPolls + if maxPolls <= 0 { + maxPolls = 600 + } + + for i := 0; i < maxPolls; i++ { + time.Sleep(interval) + + status, err := client.GetAsyncMergeResult(targetPR, uuid) + if err != nil { + cfg.Errorf("failed to check merge status: %s", err) + return ErrAPIFailure + } + if status.Merged { + cfg.Successf("Merged %s into %s", list, base) + if sha := status.Details.SHA; sha != "" { + cfg.Printf(" Merge commit %s", shortMergeSHA(sha)) + } + return nil + } + if !status.Queued { + cfg.Errorf("merge failed: %s", status.Details.Message) + cfg.Printf("The stack is atomic, so nothing was merged.") + return mergeFailureExit(status.Details.Message) + } + } + + cfg.Warningf("Merge is still in progress. Check the pull requests on GitHub.") + return ErrAPIFailure +} + +// mergeFuncs returns submit/poll closures that adapt the GitHub client to the +// mergeview injection points. +func mergeFuncs(client github.ClientOps) (mergeview.SubmitFunc, mergeview.PollFunc) { + submit := func(targetPR int, method string) (mergeview.MergeStatus, error) { + res, err := client.MergeStackAsync(targetPR, method) + if err != nil { + return mergeview.MergeStatus{}, err + } + return toMergeStatus(res), nil + } + poll := func(targetPR int, uuid string) (mergeview.MergeStatus, error) { + res, err := client.GetAsyncMergeResult(targetPR, uuid) + if err != nil { + return mergeview.MergeStatus{}, err + } + return toMergeStatus(res), nil + } + return submit, poll +} + +func toMergeStatus(res *github.AsyncMergeResult) mergeview.MergeStatus { + return mergeview.MergeStatus{ + Queued: res.Queued, + Merged: res.Merged, + Message: res.Details.Message, + UUID: res.Details.UUID, + SHA: res.Details.SHA, + } +} + +// mergeCandidates returns the pull requests that can be merged, ordered bottom to +// top: the contiguous run of open, non-draft PRs starting from the bottom of the +// stack (already-merged PRs at the bottom are skipped). The first draft or +// closed PR blocks everything above it and is returned as the blocker. +func mergeCandidates(rs *github.RemoteStack) (items []mergeview.PRItem, blocker *github.RemoteStackPR) { + if rs == nil { + return nil, nil + } + for i := range rs.PRDetails { + pr := rs.PRDetails[i] + if pr.IsMerged() { + continue + } + if pr.Draft || pr.State == "closed" { + b := pr + return items, &b + } + items = append(items, mergeview.PRItem{Number: pr.Number, Title: pr.Head.Ref}) + } + return items, nil +} + +func explainNothingToMerge(cfg *config.Config, rs *github.RemoteStack, blocker *github.RemoteStackPR) error { + if allMerged(rs) { + cfg.Successf("This stack is already fully merged.") + return nil + } + if blocker != nil { + cfg.Errorf("nothing to merge: pull request #%d is %s", blocker.Number, blockerState(blocker)) + return ErrNotInStack + } + cfg.Errorf("this stack has no open pull requests to merge") + return ErrNotInStack +} + +func explainNonMergeableTarget(cfg *config.Config, rs *github.RemoteStack, prNumber int, blocker *github.RemoteStackPR) error { + pr := findRemotePR(rs, prNumber) + switch { + case pr == nil: + cfg.Errorf("pull request #%d is not part of this stack", prNumber) + return ErrInvalidArgs + case pr.IsMerged(): + cfg.Successf("pull request #%d is already merged", prNumber) + return nil + case pr.Draft: + cfg.Errorf("pull request #%d is a draft; mark it ready for review before merging", prNumber) + return ErrInvalidArgs + case pr.State == "closed": + cfg.Errorf("pull request #%d is closed", prNumber) + return ErrInvalidArgs + case blocker != nil: + cfg.Errorf("pull request #%d cannot be merged yet: #%d below it is %s", prNumber, blocker.Number, blockerState(blocker)) + return ErrInvalidArgs + default: + cfg.Errorf("pull request #%d cannot be merged", prNumber) + return ErrInvalidArgs + } +} + +func warnAsyncMergeUnavailable(cfg *config.Config) { + cfg.Warningf("Async stack merge is not available for this repository") +} + +// mergeFailureExit maps a merge failure message to an exit code: rebase/merge +// conflicts get ErrConflict, everything else ErrAPIFailure. +func mergeFailureExit(message string) error { + if strings.Contains(strings.ToLower(message), "conflict") { + return ErrConflict + } + return ErrAPIFailure +} + +func resolveMergeMethodFlag(opts *mergeOptions) (string, error) { + var picks []string + if opts.merge { + picks = append(picks, github.MergeMethodMerge) + } + if opts.squash { + picks = append(picks, github.MergeMethodSquash) + } + if opts.rebase { + picks = append(picks, github.MergeMethodRebase) + } + if opts.mergeMethod != "" { + mm := strings.ToLower(strings.TrimSpace(opts.mergeMethod)) + switch mm { + case github.MergeMethodMerge, github.MergeMethodSquash, github.MergeMethodRebase: + picks = append(picks, mm) + default: + return "", fmt.Errorf("invalid --merge-method %q: must be merge, squash, or rebase", opts.mergeMethod) + } + } + + distinct := map[string]struct{}{} + for _, p := range picks { + distinct[p] = struct{}{} + } + if len(distinct) > 1 { + return "", errors.New("only one merge method may be specified") + } + for p := range distinct { + return p, nil + } + return "", nil +} + +func indexOfPR(items []mergeview.PRItem, number int) int { + for i, it := range items { + if it.Number == number { + return i + } + } + return -1 +} + +func numbersUpTo(items []mergeview.PRItem, targetPR int) []int { + var nums []int + for _, it := range items { + nums = append(nums, it.Number) + if it.Number == targetPR { + break + } + } + return nums +} + +func prNumberList(nums []int) string { + parts := make([]string, len(nums)) + for i, n := range nums { + parts[i] = fmt.Sprintf("#%d", n) + } + return strings.Join(parts, ", ") +} + +func findRemotePR(rs *github.RemoteStack, number int) *github.RemoteStackPR { + if rs == nil { + return nil + } + for i := range rs.PRDetails { + if rs.PRDetails[i].Number == number { + return &rs.PRDetails[i] + } + } + return nil +} + +func allMerged(rs *github.RemoteStack) bool { + if rs == nil || len(rs.PRDetails) == 0 { + return false + } + for i := range rs.PRDetails { + if !rs.PRDetails[i].IsMerged() { + return false + } + } + return true +} + +func blockerState(pr *github.RemoteStackPR) string { + if pr.Draft { + return "a draft" + } + if pr.State == "closed" { + return "closed" + } + return "not mergeable" +} + +func shortMergeSHA(sha string) string { + if len(sha) > 7 { + return sha[:7] + } + return sha +} + +func isNotFound(err error) bool { + var httpErr *api.HTTPError + return errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound +} diff --git a/cmd/merge_test.go b/cmd/merge_test.go new file mode 100644 index 00000000..f7b9de87 --- /dev/null +++ b/cmd/merge_test.go @@ -0,0 +1,469 @@ +package cmd + +import ( + "net/http" + "testing" + "time" + + "github.com/cli/go-gh/v2/pkg/api" + "github.com/github/gh-stack/internal/config" + "github.com/github/gh-stack/internal/git" + "github.com/github/gh-stack/internal/github" + "github.com/github/gh-stack/internal/stack" + "github.com/github/gh-stack/internal/tui/mergeview" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func openStackPR(n int, ref string) github.RemoteStackPR { + return github.RemoteStackPR{Number: n, State: "open", Head: github.RemoteStackPRHead{Ref: ref}} +} + +func draftStackPR(n int, ref string) github.RemoteStackPR { + return github.RemoteStackPR{Number: n, State: "open", Draft: true, Head: github.RemoteStackPRHead{Ref: ref}} +} + +func closedStackPR(n int, ref string) github.RemoteStackPR { + return github.RemoteStackPR{Number: n, State: "closed", Head: github.RemoteStackPRHead{Ref: ref}} +} + +func mergedStackPR(n int, ref string) github.RemoteStackPR { + at := "2026-01-01T00:00:00Z" + return github.RemoteStackPR{Number: n, State: "closed", MergedAt: &at, Head: github.RemoteStackPRHead{Ref: ref}} +} + +func remoteStack(number int, base string, prs ...github.RemoteStackPR) *github.RemoteStack { + nums := make([]int, len(prs)) + for i, p := range prs { + nums[i] = p.Number + } + return &github.RemoteStack{ + ID: number, + Number: number, + Base: github.RemoteStackBase{Ref: base}, + Open: true, + PullRequests: nums, + PRDetails: prs, + } +} + +func notFoundErr() error { return &api.HTTPError{StatusCode: http.StatusNotFound} } + +func fastOptions() *mergeOptions { + return &mergeOptions{pollInterval: time.Millisecond, maxPolls: 5} +} + +// setupLocalStack writes a single-stack file and mocks git so no-arg resolution +// finds it. +func setupLocalStack(t *testing.T, number int, currentBranch string, branches ...string) { + t.Helper() + gitDir := t.TempDir() + restore := git.SetOps(&git.MockOps{ + GitDirFn: func() (string, error) { return gitDir, nil }, + CurrentBranchFn: func() (string, error) { return currentBranch, nil }, + }) + t.Cleanup(restore) + + refs := make([]stack.BranchRef, len(branches)) + for i, b := range branches { + refs[i] = stack.BranchRef{Branch: b} + } + writeStackFile(t, gitDir, stack.Stack{ + ID: "s", + Number: number, + Trunk: stack.BranchRef{Branch: "main"}, + Branches: refs, + }) +} + +func TestRunMerge_NoArg_MergesWholeStack(t *testing.T) { + setupLocalStack(t, 100, "b2", "b1", "b2", "b3") + + var gotPR int + var gotMethod string + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + assert.Equal(t, 100, n) + return remoteStack(100, "main", openStackPR(1, "b1"), openStackPR(2, "b2"), openStackPR(3, "b3")), nil + }, + RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { + return &github.RepoMergeConfig{MergeAllowed: true, SquashAllowed: true, RebaseAllowed: true, DefaultMethod: "squash"}, nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotPR, gotMethod = pr, method + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}, StatusCode: 202}, nil + }, + GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc1234"}, StatusCode: 200}, nil + }, + } + + err := runMerge(cfg, fastOptions(), nil) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, 3, gotPR, "targets the top of the stack") + assert.Equal(t, "squash", gotMethod, "uses the viewer default method") + assert.Contains(t, output, "Merged #1, #2, #3 into main") +} + +func TestRunMerge_StackNumberArg(t *testing.T) { + var gotPR int + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + assert.Equal(t, 7, n) + return remoteStack(7, "main", openStackPR(10, "a"), openStackPR(11, "b")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotPR = pr + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, 11, gotPR) + assert.Contains(t, output, "Merged #10, #11 into main") +} + +func TestRunMerge_PRNumberArg(t *testing.T) { + var gotPR int + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return nil, notFoundErr() // not a stack number + }, + FindStackForPRFn: func(n int) (*github.RemoteStack, error) { + assert.Equal(t, 2, n) + return remoteStack(5, "main", openStackPR(1, "b1"), openStackPR(2, "b2"), openStackPR(3, "b3")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotPR = pr + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"2"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, 2, gotPR, "targets exactly the requested PR") + assert.Contains(t, output, "Merged #1, #2 into main") + assert.NotContains(t, output, "#3") +} + +func TestRunMerge_SquashFlag(t *testing.T) { + var gotMethod string + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotMethod = method + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + }, + } + + opts := fastOptions() + opts.squash = true + err := runMerge(cfg, opts, []string{"7"}) + _ = collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, "squash", gotMethod) +} + +func TestRunMerge_ConflictingMethodFlags(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + opts := fastOptions() + opts.squash = true + opts.rebase = true + + err := runMerge(cfg, opts, []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "only one merge method") +} + +func TestRunMerge_InvalidMergeMethod(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + opts := fastOptions() + opts.mergeMethod = "fast-forward" + + err := runMerge(cfg, opts, []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "invalid --merge-method") +} + +func TestRunMerge_DisallowedMethod(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { + return &github.RepoMergeConfig{MergeAllowed: true, DefaultMethod: "merge"}, nil + }, + } + opts := fastOptions() + opts.squash = true + + err := runMerge(cfg, opts, []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "does not allow squash") +} + +func TestRunMerge_DraftTarget(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { return nil, notFoundErr() }, + FindStackForPRFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(5, "main", openStackPR(1, "b1"), openStackPR(2, "b2"), draftStackPR(3, "b3")), nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"3"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "#3 is a draft") +} + +func TestRunMerge_BlockerBelowTarget(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { return nil, notFoundErr() }, + FindStackForPRFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(5, "main", openStackPR(1, "b1"), draftStackPR(2, "b2"), openStackPR(3, "b3")), nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"3"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "#2 below it is a draft") +} + +func TestRunMerge_AlreadyMergedTarget(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { return nil, notFoundErr() }, + FindStackForPRFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(5, "main", mergedStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"1"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Contains(t, output, "#1 is already merged") +} + +func TestRunMerge_NothingToMerge_AllMerged(t *testing.T) { + setupLocalStack(t, 100, "b1", "b1", "b2") + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(100, "main", mergedStackPR(1, "b1"), mergedStackPR(2, "b2")), nil + }, + } + + err := runMerge(cfg, fastOptions(), nil) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Contains(t, output, "already fully merged") +} + +func TestRunMerge_SubmitNotMergeable(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Queued: false, Merged: false, Details: github.AsyncMergeDetails{Message: "Pull request is closed."}, StatusCode: 400}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrAPIFailure) + assert.Contains(t, output, "cannot merge: Pull request is closed.") +} + +func TestRunMerge_PollFailedConflict(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}, StatusCode: 202}, nil + }, + GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Queued: false, Merged: false, Details: github.AsyncMergeDetails{Message: "Merge conflict: could not merge."}}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrConflict) + assert.Contains(t, output, "merge failed: Merge conflict") + assert.Contains(t, output, "nothing was merged") +} + +func TestRunMerge_AlreadyMergedOnSubmit(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc"}, StatusCode: 200}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Contains(t, output, "Merged #1, #2 into main") +} + +func TestRunMerge_AsyncMergeUnavailable(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + return nil, github.ErrAsyncMergeUnavailable + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrStacksUnavailable) + assert.Contains(t, output, "not available for this repository") +} + +func TestRunMerge_StacksUnavailable(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { return nil, notFoundErr() }, + FindStackForPRFn: func(n int) (*github.RemoteStack, error) { return nil, notFoundErr() }, + } + + err := runMerge(cfg, fastOptions(), []string{"5"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrStacksUnavailable) + assert.Contains(t, output, "not enabled for this repository") +} + +func TestRunMerge_NoArg_NotInStack(t *testing.T) { + setupLocalStack(t, 100, "other", "b1", "b2") // current branch not in stack + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{} + + err := runMerge(cfg, fastOptions(), nil) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrNotInStack) + assert.Contains(t, output, "not part of a stack") +} + +func TestRunMerge_DefaultMethodFallsBackToAllowed(t *testing.T) { + var gotMethod string + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { + // Viewer default is a method the repo no longer allows. + return &github.RepoMergeConfig{SquashAllowed: true, DefaultMethod: "merge"}, nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotMethod = method + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + _ = collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, "squash", gotMethod, "falls back to the only allowed method") +} + +func TestResolveMergeMethodFlag(t *testing.T) { + tests := []struct { + name string + opts mergeOptions + want string + wantErr bool + }{ + {"none", mergeOptions{}, "", false}, + {"merge", mergeOptions{merge: true}, "merge", false}, + {"squash", mergeOptions{squash: true}, "squash", false}, + {"rebase", mergeOptions{rebase: true}, "rebase", false}, + {"merge-method", mergeOptions{mergeMethod: "SQUASH"}, "squash", false}, + {"redundant same", mergeOptions{squash: true, mergeMethod: "squash"}, "squash", false}, + {"conflicting bools", mergeOptions{squash: true, rebase: true}, "", true}, + {"conflicting flag+bool", mergeOptions{merge: true, mergeMethod: "squash"}, "", true}, + {"invalid", mergeOptions{mergeMethod: "ff"}, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolveMergeMethodFlag(&tt.opts) + if tt.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestMergeCandidates(t *testing.T) { + t.Run("all open", func(t *testing.T) { + items, blocker := mergeCandidates(remoteStack(1, "main", openStackPR(1, "a"), openStackPR(2, "b"))) + assert.Nil(t, blocker) + require.Len(t, items, 2) + assert.Equal(t, 1, items[0].Number) + }) + t.Run("leading merged skipped", func(t *testing.T) { + items, blocker := mergeCandidates(remoteStack(1, "main", mergedStackPR(1, "a"), openStackPR(2, "b"), openStackPR(3, "c"))) + assert.Nil(t, blocker) + assert.Equal(t, []mergeview.PRItem{{Number: 2, Title: "b"}, {Number: 3, Title: "c"}}, items) + }) + t.Run("draft blocks above", func(t *testing.T) { + items, blocker := mergeCandidates(remoteStack(1, "main", openStackPR(1, "a"), draftStackPR(2, "b"), openStackPR(3, "c"))) + require.NotNil(t, blocker) + assert.Equal(t, 2, blocker.Number) + assert.Equal(t, []mergeview.PRItem{{Number: 1, Title: "a"}}, items) + }) + t.Run("closed blocks", func(t *testing.T) { + items, blocker := mergeCandidates(remoteStack(1, "main", closedStackPR(1, "a"), openStackPR(2, "b"))) + require.NotNil(t, blocker) + assert.Empty(t, items) + }) +} diff --git a/cmd/root.go b/cmd/root.go index bbbb6765..f4100c74 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -112,6 +112,10 @@ locally, then push to GitHub to create your stack of PRs.`, linkCmd.GroupID = "remote" root.AddCommand(linkCmd) + mergeCmd := MergeCmd(cfg) + mergeCmd.GroupID = "remote" + root.AddCommand(mergeCmd) + // Navigation commands switchCmd := SwitchCmd(cfg) switchCmd.GroupID = "nav" diff --git a/cmd/root_test.go b/cmd/root_test.go index 8138c7a7..47328fd9 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -10,7 +10,7 @@ import ( func TestRootCmd_SubcommandRegistration(t *testing.T) { root := RootCmd() - expected := []string{"init", "add", "checkout", "push", "sync", "unstack", "view", "rebase", "up", "down", "top", "bottom", "alias", "feedback", "submit"} + expected := []string{"init", "add", "checkout", "push", "sync", "unstack", "view", "rebase", "up", "down", "top", "bottom", "alias", "feedback", "submit", "merge"} registered := make(map[string]bool) for _, cmd := range root.Commands() { diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index 7e613814..2903df5d 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -17,6 +17,9 @@ type ClientOps interface { CreateStack(prNumbers []int) (*RemoteStack, error) AddToStack(stackNumber int, prNumbers []int) (*RemoteStack, error) Unstack(stackNumber int) (*RemoteStack, bool, error) + RepoMergeConfig() (*RepoMergeConfig, error) + MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) + GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go new file mode 100644 index 00000000..1dd68270 --- /dev/null +++ b/internal/github/merge_async.go @@ -0,0 +1,247 @@ +package github + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "net/http" + "strings" + + "github.com/cli/go-gh/v2/pkg/auth" + graphql "github.com/cli/shurcooL-graphql" +) + +// Merge method values accepted by the async merge REST API. +const ( + MergeMethodMerge = "merge" + MergeMethodSquash = "squash" + MergeMethodRebase = "rebase" +) + +// ErrAsyncMergeUnavailable indicates the async merge API is not available for +// the repository (or the token lacks access). Surfaced on a 404 from the submit +// endpoint. +var ErrAsyncMergeUnavailable = errors.New("async stack merge is not available for this repository") + +// RepoMergeConfig describes which merge methods a repository allows, along with +// the viewer's default (last-used) merge method. +type RepoMergeConfig struct { + MergeAllowed bool + SquashAllowed bool + RebaseAllowed bool + // DefaultMethod is the viewer's last-used merge method, or the repository + // default, as one of MergeMethodMerge/MergeMethodSquash/MergeMethodRebase. + DefaultMethod string +} + +// AllowedMethods returns the enabled merge methods in display order +// (merge, squash, rebase). +func (c RepoMergeConfig) AllowedMethods() []string { + var methods []string + if c.MergeAllowed { + methods = append(methods, MergeMethodMerge) + } + if c.SquashAllowed { + methods = append(methods, MergeMethodSquash) + } + if c.RebaseAllowed { + methods = append(methods, MergeMethodRebase) + } + return methods +} + +// Allows reports whether the given merge method is enabled for the repository. +func (c RepoMergeConfig) Allows(method string) bool { + switch method { + case MergeMethodMerge: + return c.MergeAllowed + case MergeMethodSquash: + return c.SquashAllowed + case MergeMethodRebase: + return c.RebaseAllowed + } + return false +} + +// AsyncMergeDetails is the polymorphic "details" object shared by the submit and +// poll responses. Fields are populated based on the current state: a queued +// request carries UUID/MergeMethod/ExpectedHeadSHA, an already-merged result +// carries SHA, and a failed/not-mergeable result carries only Message. +type AsyncMergeDetails struct { + Message string `json:"message"` + UUID string `json:"uuid"` + MergeMethod string `json:"merge_method"` + ExpectedHeadSHA string `json:"expected_head_sha"` + SHA string `json:"sha"` +} + +// AsyncMergeResult is the response body returned by both the submit and poll +// async merge endpoints. StatusCode carries the HTTP status of the submit +// response so callers can distinguish enqueued (202) from an existing request +// (409) and an already-merged PR (200). +type AsyncMergeResult struct { + Queued bool `json:"queued"` + Merged bool `json:"merged"` + Details AsyncMergeDetails `json:"details"` + StatusCode int `json:"-"` +} + +// InProgress reports whether the merge is still queued (running in the +// background). +func (r *AsyncMergeResult) InProgress() bool { + return r != nil && r.Queued && !r.Merged +} + +// RepoMergeConfig fetches the repository's allowed merge methods and the +// viewer's default (last-used) merge method. +func (c *Client) RepoMergeConfig() (*RepoMergeConfig, error) { + var query struct { + Repository struct { + MergeCommitAllowed bool `graphql:"mergeCommitAllowed"` + SquashMergeAllowed bool `graphql:"squashMergeAllowed"` + RebaseMergeAllowed bool `graphql:"rebaseMergeAllowed"` + ViewerDefaultMergeMethod string `graphql:"viewerDefaultMergeMethod"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": graphql.String(c.owner), + "name": graphql.String(c.repo), + } + + if err := c.gql.Query("RepoMergeConfig", &query, variables); err != nil { + return nil, fmt.Errorf("querying repository merge config: %w", err) + } + + r := query.Repository + return &RepoMergeConfig{ + MergeAllowed: r.MergeCommitAllowed, + SquashAllowed: r.SquashMergeAllowed, + RebaseAllowed: r.RebaseMergeAllowed, + DefaultMethod: mergeMethodFromEnum(r.ViewerDefaultMergeMethod), + }, nil +} + +// MergeStackAsync requests an asynchronous merge of the given pull request. For +// a stacked PR this merges all members of the stack up to and including +// prNumber. A blank method lets the server apply its default. +// +// The returned result is populated for the 200 (already merged), 202 (enqueued) +// 409 (a request already exists) and 400 (not mergeable) responses; the HTTP +// status is recorded on StatusCode. A 404 returns ErrAsyncMergeUnavailable. +func (c *Client) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) { + type reqBody struct { + MergeMethod string `json:"merge_method,omitempty"` + } + + body, err := json.Marshal(reqBody{MergeMethod: method}) + if err != nil { + return nil, fmt.Errorf("marshaling request: %w", err) + } + + path := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async", c.owner, c.repo, prNumber) + resp, err := c.doAsyncRequest(http.MethodPut, path, bytes.NewReader(body)) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + + switch resp.StatusCode { + case http.StatusOK, http.StatusAccepted, http.StatusConflict, http.StatusBadRequest: + return decodeAsyncMergeResult(resp) + case http.StatusNotFound: + return nil, ErrAsyncMergeUnavailable + default: + return nil, asyncMergeError(resp) + } +} + +// GetAsyncMergeResult fetches the current result of a previously submitted async +// merge, identified by the UUID returned from MergeStackAsync. +func (c *Client) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) { + path := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async/%s", c.owner, c.repo, prNumber, uuid) + resp, err := c.doAsyncRequest(http.MethodGet, path, nil) + if err != nil { + return nil, err + } + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusOK { + return decodeAsyncMergeResult(resp) + } + return nil, asyncMergeError(resp) +} + +// doAsyncRequest issues an authenticated request to the REST API and returns the +// raw response without treating non-2xx statuses as errors, so the caller can +// read the merge result body for 4xx responses (which carry the UUID/message). +func (c *Client) doAsyncRequest(method, path string, body io.Reader) (*http.Response, error) { + req, err := http.NewRequest(method, c.base+path, body) + if err != nil { + return nil, err + } + req.Header.Set("Accept", "application/vnd.github+json") + if body != nil { + req.Header.Set("Content-Type", "application/json") + } + return c.http.Do(req) +} + +func decodeAsyncMergeResult(resp *http.Response) (*AsyncMergeResult, error) { + var r AsyncMergeResult + if err := json.NewDecoder(resp.Body).Decode(&r); err != nil { + return nil, fmt.Errorf("decoding merge response: %w", err) + } + r.StatusCode = resp.StatusCode + return &r, nil +} + +// asyncMergeError builds an error from an unexpected (403/422/5xx) response, +// extracting the API message when present. +func asyncMergeError(resp *http.Response) error { + b, _ := io.ReadAll(resp.Body) + var parsed struct { + Message string `json:"message"` + } + _ = json.Unmarshal(b, &parsed) + if parsed.Message != "" { + return fmt.Errorf("HTTP %d: %s", resp.StatusCode, parsed.Message) + } + if trimmed := strings.TrimSpace(string(b)); trimmed != "" { + return fmt.Errorf("HTTP %d: %s", resp.StatusCode, trimmed) + } + return fmt.Errorf("HTTP %d", resp.StatusCode) +} + +// mergeMethodFromEnum maps a GraphQL PullRequestMergeMethod enum value +// (MERGE/SQUASH/REBASE) to the lowercase REST API value. Unknown values fall +// back to MergeMethodMerge. +func mergeMethodFromEnum(enum string) string { + switch strings.ToUpper(enum) { + case "SQUASH": + return MergeMethodSquash + case "REBASE": + return MergeMethodRebase + default: + return MergeMethodMerge + } +} + +// restBaseURL derives the REST API base URL for a host, mirroring go-gh's +// internal restPrefix so raw requests target the same endpoint as the REST +// client. +func restBaseURL(host string) string { + if host == "" { + host = "github.com" + } + normalized := auth.NormalizeHostname(host) + if auth.IsEnterprise(normalized) { + return fmt.Sprintf("https://%s/api/v3/", normalized) + } + if strings.EqualFold(normalized, "github.localhost") { + return fmt.Sprintf("http://api.%s/", normalized) + } + return fmt.Sprintf("https://api.%s/", normalized) +} diff --git a/internal/github/merge_async_test.go b/internal/github/merge_async_test.go new file mode 100644 index 00000000..1c7b1784 --- /dev/null +++ b/internal/github/merge_async_test.go @@ -0,0 +1,193 @@ +package github + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testAsyncClient builds a Client wired to a test server for the async merge +// REST methods, which only use the raw HTTP client and base URL. +func testAsyncClient(base string) *Client { + return &Client{http: http.DefaultClient, base: base + "/", owner: "o", repo: "r", slug: "o/r"} +} + +type recordedRequest struct { + method string + path string + body string +} + +func serveOnce(t *testing.T, status int, respBody string, rec *recordedRequest) *httptest.Server { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if rec != nil { + b, _ := io.ReadAll(r.Body) + rec.method = r.Method + rec.path = r.URL.Path + rec.body = string(b) + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _, _ = io.WriteString(w, respBody) + })) + t.Cleanup(srv.Close) + return srv +} + +func TestMergeStackAsync_Accepted(t *testing.T) { + var rec recordedRequest + body := `{"queued":true,"merged":false,"details":{"message":"Merge request enqueued.","uuid":"u-1","merge_method":"squash","expected_head_sha":"abc"}}` + srv := serveOnce(t, http.StatusAccepted, body, &rec) + + c := testAsyncClient(srv.URL) + res, err := c.MergeStackAsync(42, "squash") + require.NoError(t, err) + + assert.Equal(t, http.MethodPut, rec.method) + assert.Equal(t, "/repos/o/r/pulls/42/merge-async", rec.path) + assert.JSONEq(t, `{"merge_method":"squash"}`, rec.body) + + assert.True(t, res.Queued) + assert.False(t, res.Merged) + assert.Equal(t, http.StatusAccepted, res.StatusCode) + assert.Equal(t, "u-1", res.Details.UUID) + assert.Equal(t, "squash", res.Details.MergeMethod) + assert.True(t, res.InProgress()) +} + +func TestMergeStackAsync_AlreadyMerged(t *testing.T) { + body := `{"queued":false,"merged":true,"details":{"message":"Pull request is already merged.","sha":"deadbeef"}}` + srv := serveOnce(t, http.StatusOK, body, nil) + + res, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + require.NoError(t, err) + assert.True(t, res.Merged) + assert.Equal(t, "deadbeef", res.Details.SHA) + assert.Equal(t, http.StatusOK, res.StatusCode) +} + +func TestMergeStackAsync_ExistingRequestConflict(t *testing.T) { + body := `{"queued":true,"merged":false,"details":{"message":"A merge request already exists for this pull request.","uuid":"u-2","merge_method":"merge","expected_head_sha":"abc"}}` + srv := serveOnce(t, http.StatusConflict, body, nil) + + res, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + require.NoError(t, err) + assert.Equal(t, http.StatusConflict, res.StatusCode) + assert.Equal(t, "u-2", res.Details.UUID) + assert.True(t, res.InProgress()) +} + +func TestMergeStackAsync_NotMergeable(t *testing.T) { + body := `{"queued":false,"merged":false,"details":{"message":"Pull request is closed."}}` + srv := serveOnce(t, http.StatusBadRequest, body, nil) + + res, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + require.NoError(t, err) + assert.False(t, res.Queued) + assert.False(t, res.Merged) + assert.Equal(t, "Pull request is closed.", res.Details.Message) +} + +func TestMergeStackAsync_NotAvailable(t *testing.T) { + srv := serveOnce(t, http.StatusNotFound, `{"message":"Not Found"}`, nil) + _, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + assert.ErrorIs(t, err, ErrAsyncMergeUnavailable) +} + +func TestMergeStackAsync_ValidationFailed(t *testing.T) { + srv := serveOnce(t, http.StatusUnprocessableEntity, `{"message":"Validation Failed"}`, nil) + _, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + require.Error(t, err) + assert.Contains(t, err.Error(), "Validation Failed") +} + +func TestGetAsyncMergeResult_States(t *testing.T) { + tests := []struct { + name string + body string + wantQueued bool + wantMerged bool + }{ + {"pending", `{"queued":true,"merged":false,"details":{"message":"Merge request is in progress.","uuid":"u","merge_method":"merge","expected_head_sha":"abc"}}`, true, false}, + {"merged", `{"queued":false,"merged":true,"details":{"message":"Pull request was merged.","sha":"abc"}}`, false, true}, + {"failed", `{"queued":false,"merged":false,"details":{"message":"Merge conflict."}}`, false, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var rec recordedRequest + srv := serveOnce(t, http.StatusOK, tt.body, &rec) + res, err := testAsyncClient(srv.URL).GetAsyncMergeResult(42, "u") + require.NoError(t, err) + assert.Equal(t, http.MethodGet, rec.method) + assert.Equal(t, "/repos/o/r/pulls/42/merge-async/u", rec.path) + assert.Equal(t, tt.wantQueued, res.Queued) + assert.Equal(t, tt.wantMerged, res.Merged) + }) + } +} + +func TestGetAsyncMergeResult_NotFound(t *testing.T) { + srv := serveOnce(t, http.StatusNotFound, `{"message":"Not Found"}`, nil) + _, err := testAsyncClient(srv.URL).GetAsyncMergeResult(42, "missing") + require.Error(t, err) +} + +func TestRestBaseURL(t *testing.T) { + tests := []struct { + host string + want string + }{ + {"", "https://api.github.com/"}, + {"github.com", "https://api.github.com/"}, + {"github.example.com", "https://github.example.com/api/v3/"}, + } + for _, tt := range tests { + assert.Equal(t, tt.want, restBaseURL(tt.host), "host %q", tt.host) + } +} + +func TestMergeMethodFromEnum(t *testing.T) { + assert.Equal(t, MergeMethodMerge, mergeMethodFromEnum("MERGE")) + assert.Equal(t, MergeMethodSquash, mergeMethodFromEnum("SQUASH")) + assert.Equal(t, MergeMethodRebase, mergeMethodFromEnum("REBASE")) + assert.Equal(t, MergeMethodMerge, mergeMethodFromEnum("UNKNOWN")) + assert.Equal(t, MergeMethodMerge, mergeMethodFromEnum("")) +} + +func TestRepoMergeConfig_AllowedMethods(t *testing.T) { + c := RepoMergeConfig{MergeAllowed: true, RebaseAllowed: true} + assert.Equal(t, []string{"merge", "rebase"}, c.AllowedMethods()) + assert.True(t, c.Allows("merge")) + assert.False(t, c.Allows("squash")) + assert.True(t, c.Allows("rebase")) + + empty := RepoMergeConfig{} + assert.Empty(t, empty.AllowedMethods()) +} + +func TestAsyncMergeResult_InProgress(t *testing.T) { + assert.True(t, (&AsyncMergeResult{Queued: true}).InProgress()) + assert.False(t, (&AsyncMergeResult{Queued: true, Merged: true}).InProgress()) + assert.False(t, (&AsyncMergeResult{}).InProgress()) + var nilRes *AsyncMergeResult + assert.False(t, nilRes.InProgress()) +} + +// sanity check that the submit body omits merge_method when empty. +func TestMergeStackAsync_OmitsEmptyMethod(t *testing.T) { + var rec recordedRequest + srv := serveOnce(t, http.StatusAccepted, `{"queued":true,"merged":false,"details":{"message":"m","uuid":"u","merge_method":"merge","expected_head_sha":"x"}}`, &rec) + _, err := testAsyncClient(srv.URL).MergeStackAsync(1, "") + require.NoError(t, err) + + var parsed map[string]any + require.NoError(t, json.Unmarshal([]byte(rec.body), &parsed)) + _, hasMethod := parsed["merge_method"] + assert.False(t, hasMethod, "merge_method should be omitted when empty") +} diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 6c3a4678..d9b28791 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -17,6 +17,9 @@ type MockClient struct { CreateStackFn func([]int) (*RemoteStack, error) AddToStackFn func(int, []int) (*RemoteStack, error) UnstackFn func(int) (*RemoteStack, bool, error) + RepoMergeConfigFn func() (*RepoMergeConfig, error) + MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) + GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) } // Compile-time check that MockClient satisfies ClientOps. @@ -112,3 +115,44 @@ func (m *MockClient) Unstack(stackNumber int) (*RemoteStack, bool, error) { } return nil, false, nil } + +func (m *MockClient) RepoMergeConfig() (*RepoMergeConfig, error) { + if m.RepoMergeConfigFn != nil { + return m.RepoMergeConfigFn() + } + return &RepoMergeConfig{ + MergeAllowed: true, + SquashAllowed: true, + RebaseAllowed: true, + DefaultMethod: MergeMethodMerge, + }, nil +} + +func (m *MockClient) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) { + if m.MergeStackAsyncFn != nil { + return m.MergeStackAsyncFn(prNumber, method) + } + return &AsyncMergeResult{ + Queued: true, + Details: AsyncMergeDetails{ + Message: "Merge request enqueued.", + UUID: "mock-uuid", + MergeMethod: method, + }, + StatusCode: 202, + }, nil +} + +func (m *MockClient) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) { + if m.GetAsyncMergeResultFn != nil { + return m.GetAsyncMergeResultFn(prNumber, uuid) + } + return &AsyncMergeResult{ + Merged: true, + Details: AsyncMergeDetails{ + Message: "Pull request was merged.", + SHA: "mockmergesha", + }, + StatusCode: 200, + }, nil +} diff --git a/internal/tui/mergeview/model.go b/internal/tui/mergeview/model.go new file mode 100644 index 00000000..fcefc33e --- /dev/null +++ b/internal/tui/mergeview/model.go @@ -0,0 +1,366 @@ +package mergeview + +import ( + "errors" + "time" + + "github.com/charmbracelet/bubbles/spinner" + tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" + "github.com/github/gh-stack/internal/theme" +) + +// Model is the Bubble Tea model backing the merge wizard. +type Model struct { + opts Options + + step Step + + // topIndex is the highest selected PR index (the merge high-water mark); + // -1 means nothing selected. Selecting index i implies 0..i are included. + topIndex int + cursor int + + methodCursor int + method string + + spinner spinner.Model + status MergeStatus + + submitted bool + merged bool + failed bool + cancelled bool + message string + err error + + pollInterval time.Duration + + width int +} + +// New builds a merge wizard model from the given options. +func New(opts Options) Model { + interval := opts.PollInterval + if interval <= 0 { + interval = time.Second + } + + sp := spinner.New() + sp.Spinner = spinner.Dot + sp.Style = lipgloss.NewStyle().Foreground(theme.ColorAccent) + + m := Model{ + opts: opts, + topIndex: len(opts.PRs) - 1, + cursor: len(opts.PRs) - 1, + method: normalizeDefaultMethod(opts), + pollInterval: interval, + spinner: sp, + } + m.methodCursor = indexOf(opts.AllowedMethods, m.method) + + if opts.PreselectTopIndex >= 0 && opts.PreselectTopIndex < len(opts.PRs) { + // PR-number mode: the target is fixed, so skip the selection step. + m.topIndex = opts.PreselectTopIndex + m.cursor = opts.PreselectTopIndex + m.step = StepMethod + } + + return m +} + +// Init implements tea.Model. +func (m Model) Init() tea.Cmd { return nil } + +// Update implements tea.Model. +func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + switch msg := msg.(type) { + case tea.WindowSizeMsg: + m.width = msg.Width + return m, nil + + case spinner.TickMsg: + var cmd tea.Cmd + m.spinner, cmd = m.spinner.Update(msg) + return m, cmd + + case submitDoneMsg: + return m.handleSubmitDone(msg) + + case pollTickMsg: + if m.step == StepProgress && !m.done() { + return m, m.pollCmd() + } + return m, nil + + case pollDoneMsg: + return m.handlePollDone(msg) + + case tea.KeyMsg: + return m.handleKey(msg) + } + + return m, nil +} + +func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + key := msg.String() + + if key == "ctrl+c" { + if m.step == StepProgress && m.submitted && !m.done() { + // The merge is running server-side; stop watching without cancelling it. + m.message = "Stopped watching. The merge continues on GitHub." + return m.finish() + } + m.cancelled = true + return m.finish() + } + + switch m.step { + case StepSelectPRs: + return m.handleSelectKey(key) + case StepMethod: + return m.handleMethodKey(key) + case StepConfirm: + return m.handleConfirmKey(key) + } + return m, nil +} + +func (m Model) handleSelectKey(key string) (tea.Model, tea.Cmd) { + switch key { + case "up", "k": + // The stack renders top-first, so moving "up" goes toward the top of + // the stack (a higher index). + if m.cursor < len(m.opts.PRs)-1 { + m.cursor++ + } + case "down", "j": + if m.cursor > 0 { + m.cursor-- + } + case " ", "x": + if m.cursor <= m.topIndex { + // Uncheck the cursor and everything above it. + m.topIndex = m.cursor - 1 + } else { + // Fill selection up to and including the cursor. + m.topIndex = m.cursor + } + case "enter", "tab": + if m.topIndex >= 0 { + m.step = StepMethod + } + case "esc", "q": + m.cancelled = true + return m.finish() + } + return m, nil +} + +func (m Model) handleMethodKey(key string) (tea.Model, tea.Cmd) { + switch key { + case "up", "k": + if m.methodCursor > 0 { + m.methodCursor-- + } + case "down", "j": + if m.methodCursor < len(m.opts.AllowedMethods)-1 { + m.methodCursor++ + } + case "enter", "tab", " ": + if len(m.opts.AllowedMethods) > 0 { + m.method = m.opts.AllowedMethods[m.methodCursor] + m.step = StepConfirm + } + case "shift+tab": + if m.opts.PreselectTopIndex < 0 { + m.step = StepSelectPRs + } + case "esc", "q": + m.cancelled = true + return m.finish() + } + return m, nil +} + +func (m Model) handleConfirmKey(key string) (tea.Model, tea.Cmd) { + switch key { + case "enter", "y", "Y": + m.step = StepProgress + m.submitted = true + return m, tea.Batch(m.spinner.Tick, m.submitCmd()) + case "shift+tab": + m.step = StepMethod + case "esc", "q": + m.cancelled = true + return m.finish() + } + return m, nil +} + +func (m Model) handleSubmitDone(msg submitDoneMsg) (tea.Model, tea.Cmd) { + if msg.err != nil { + m.err = msg.err + m.failed = true + m.message = msg.err.Error() + return m.finish() + } + + m.status = msg.status + m.message = msg.status.Message + + switch { + case msg.status.Merged: + m.merged = true + return m.finish() + case msg.status.Queued && msg.status.UUID != "": + // Enqueued (or an existing request adopted): start polling. + return m, m.pollTickCmd() + default: + // Not queued and not merged: the PR could not be merged (e.g. 400). + m.failed = true + return m.finish() + } +} + +func (m Model) handlePollDone(msg pollDoneMsg) (tea.Model, tea.Cmd) { + if msg.err != nil { + m.err = msg.err + m.failed = true + m.message = msg.err.Error() + return m.finish() + } + + m.status = msg.status + m.message = msg.status.Message + + switch { + case msg.status.Merged: + m.merged = true + return m.finish() + case msg.status.Queued: + return m, m.pollTickCmd() + default: + m.failed = true + return m.finish() + } +} + +func (m Model) finish() (tea.Model, tea.Cmd) { + m.step = StepDone + return m, tea.Quit +} + +func (m Model) done() bool { return m.merged || m.failed || m.step == StepDone } + +// Outcome reports the final result of the wizard for the command layer. +func (m Model) Outcome() Outcome { + o := Outcome{ + Cancelled: m.cancelled, + Submitted: m.submitted, + Merged: m.merged, + Failed: m.failed, + Message: m.message, + TargetPR: m.targetPR(), + Method: m.method, + Err: m.err, + } + if m.merged { + o.MergedPRs = m.selectedNumbers() + } + return o +} + +func (m Model) targetPR() int { + if m.topIndex >= 0 && m.topIndex < len(m.opts.PRs) { + return m.opts.PRs[m.topIndex].Number + } + return 0 +} + +func (m Model) selectedNumbers() []int { + if m.topIndex < 0 { + return nil + } + nums := make([]int, 0, m.topIndex+1) + for i := 0; i <= m.topIndex && i < len(m.opts.PRs); i++ { + nums = append(nums, m.opts.PRs[i].Number) + } + return nums +} + +// --- async commands --- + +type submitDoneMsg struct { + status MergeStatus + err error +} + +type pollDoneMsg struct { + status MergeStatus + err error +} + +type pollTickMsg struct{} + +func (m Model) submitCmd() tea.Cmd { + target := m.targetPR() + method := m.method + submit := m.opts.Submit + return func() tea.Msg { + if submit == nil { + return submitDoneMsg{err: errors.New("no submit function configured")} + } + s, err := submit(target, method) + return submitDoneMsg{status: s, err: err} + } +} + +func (m Model) pollCmd() tea.Cmd { + target := m.targetPR() + uuid := m.status.UUID + poll := m.opts.Poll + return func() tea.Msg { + if poll == nil { + return pollDoneMsg{err: errors.New("no poll function configured")} + } + s, err := poll(target, uuid) + return pollDoneMsg{status: s, err: err} + } +} + +func (m Model) pollTickCmd() tea.Cmd { + return tea.Tick(m.pollInterval, func(time.Time) tea.Msg { return pollTickMsg{} }) +} + +// --- helpers --- + +func normalizeDefaultMethod(opts Options) string { + if opts.DefaultMethod != "" && contains(opts.AllowedMethods, opts.DefaultMethod) { + return opts.DefaultMethod + } + if len(opts.AllowedMethods) > 0 { + return opts.AllowedMethods[0] + } + return opts.DefaultMethod +} + +func indexOf(s []string, v string) int { + for i, x := range s { + if x == v { + return i + } + } + return 0 +} + +func contains(s []string, v string) bool { + for _, x := range s { + if x == v { + return true + } + } + return false +} diff --git a/internal/tui/mergeview/model_test.go b/internal/tui/mergeview/model_test.go new file mode 100644 index 00000000..be77b139 --- /dev/null +++ b/internal/tui/mergeview/model_test.go @@ -0,0 +1,243 @@ +package mergeview + +import ( + "errors" + "testing" + + tea "github.com/charmbracelet/bubbletea" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func baseOptions() Options { + return Options{ + PRs: []PRItem{{Number: 1, Title: "a"}, {Number: 2, Title: "b"}, {Number: 3, Title: "c"}}, + BaseRef: "main", + AllowedMethods: []string{"merge", "squash", "rebase"}, + DefaultMethod: "squash", + PreselectTopIndex: -1, + } +} + +func step(m Model, msg tea.Msg) Model { + next, _ := m.Update(msg) + return next.(Model) +} + +func keyType(t tea.KeyType) tea.KeyMsg { return tea.KeyMsg{Type: t} } + +func space() tea.KeyMsg { return tea.KeyMsg{Type: tea.KeySpace} } + +func TestNew_DefaultsSelectAll(t *testing.T) { + m := New(baseOptions()) + assert.Equal(t, StepSelectPRs, m.step) + assert.Equal(t, 2, m.topIndex, "all PRs selected by default") + assert.Equal(t, "squash", m.method) + assert.Equal(t, 1, m.methodCursor) + assert.Equal(t, []int{1, 2, 3}, m.selectedNumbers()) + assert.Equal(t, 3, m.targetPR()) +} + +func TestNew_DefaultMethodFallback(t *testing.T) { + opts := baseOptions() + opts.DefaultMethod = "rebase" + opts.AllowedMethods = []string{"merge", "rebase"} // squash disallowed + m := New(opts) + assert.Equal(t, "rebase", m.method) + + opts.DefaultMethod = "squash" // not allowed -> first allowed + m = New(opts) + assert.Equal(t, "merge", m.method) +} + +func TestNew_PreselectSkipsSelectStep(t *testing.T) { + opts := baseOptions() + opts.PreselectTopIndex = 1 + m := New(opts) + assert.Equal(t, StepMethod, m.step) + assert.Equal(t, 1, m.topIndex) + assert.Equal(t, 2, m.targetPR()) + assert.Equal(t, []int{1, 2}, m.selectedNumbers()) +} + +func TestSelect_CascadeToggle(t *testing.T) { + m := New(baseOptions()) // topIndex=2, cursor=2 + + // Toggling the top (index 2) lowers the water line to include only 1,2. + m = step(m, space()) + assert.Equal(t, []int{1, 2}, m.selectedNumbers()) + + // Move cursor to index 0 (down toward the bottom of the stack) and toggle: + // deselects everything. + m = step(m, keyType(tea.KeyDown)) + m = step(m, keyType(tea.KeyDown)) + assert.Equal(t, 0, m.cursor) + m = step(m, space()) + assert.Empty(t, m.selectedNumbers()) + + // Toggling index 0 again selects only the bottom PR. + m = step(m, space()) + assert.Equal(t, []int{1}, m.selectedNumbers()) + assert.Equal(t, 1, m.targetPR()) +} + +func TestSelect_ArrowDirection(t *testing.T) { + m := New(baseOptions()) // cursor starts at the top of the stack (index 2) + assert.Equal(t, 2, m.cursor) + + // "up" moves toward the top of the stack and is clamped there. + m = step(m, keyType(tea.KeyUp)) + assert.Equal(t, 2, m.cursor) + + // "down" moves toward the bottom of the stack (lower index). + m = step(m, keyType(tea.KeyDown)) + assert.Equal(t, 1, m.cursor) + m = step(m, keyType(tea.KeyUp)) + assert.Equal(t, 2, m.cursor) +} + +func TestSelect_AdvanceRequiresSelection(t *testing.T) { + m := New(baseOptions()) + + // Move to the bottom PR and deselect everything. + m = step(m, keyType(tea.KeyDown)) + m = step(m, keyType(tea.KeyDown)) + require.Equal(t, 0, m.cursor) + m = step(m, space()) + require.Empty(t, m.selectedNumbers()) + + // Enter should not advance with nothing selected. + m = step(m, keyType(tea.KeyEnter)) + assert.Equal(t, StepSelectPRs, m.step) + + // Select the bottom PR, then advance. + m = step(m, space()) + m = step(m, keyType(tea.KeyEnter)) + assert.Equal(t, StepMethod, m.step) +} + +func TestMethod_BackWithShiftTab(t *testing.T) { + m := New(baseOptions()) + m = step(m, keyType(tea.KeyEnter)) // -> method + require.Equal(t, StepMethod, m.step) + m = step(m, keyType(tea.KeyShiftTab)) + assert.Equal(t, StepSelectPRs, m.step) +} + +func TestConfirm_BackWithShiftTab(t *testing.T) { + m := New(baseOptions()) + m = step(m, keyType(tea.KeyTab)) // select -> method + m = step(m, keyType(tea.KeyTab)) // method -> confirm + require.Equal(t, StepConfirm, m.step) + m = step(m, keyType(tea.KeyShiftTab)) + assert.Equal(t, StepMethod, m.step) +} + +func TestMethod_SelectAndAdvance(t *testing.T) { + m := New(baseOptions()) + m = step(m, keyType(tea.KeyEnter)) // -> method step + require.Equal(t, StepMethod, m.step) + assert.Equal(t, 1, m.methodCursor) // squash preselected + + m = step(m, keyType(tea.KeyDown)) // rebase + m = step(m, keyType(tea.KeyEnter)) + assert.Equal(t, StepConfirm, m.step) + assert.Equal(t, "rebase", m.method) +} + +func TestMethod_EscCancels(t *testing.T) { + m := New(baseOptions()) + m = step(m, keyType(tea.KeyEnter)) + require.Equal(t, StepMethod, m.step) + m = step(m, keyType(tea.KeyEsc)) + assert.True(t, m.Outcome().Cancelled) +} + +func TestConfirm_SubmitAlreadyMerged(t *testing.T) { + m := New(baseOptions()) + // submitDoneMsg is handled regardless of step; simulate an already-merged + // response. + m = step(m, submitDoneMsg{status: MergeStatus{Merged: true, Message: "Pull request is already merged.", SHA: "abc1234"}}) + out := m.Outcome() + assert.True(t, out.Merged) + assert.False(t, out.Failed) + assert.Equal(t, []int{1, 2, 3}, out.MergedPRs) +} + +func TestProgress_QueuedThenFailed(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m.submitted = true + + m = step(m, submitDoneMsg{status: MergeStatus{Queued: true, UUID: "u1", Message: "enqueued"}}) + assert.False(t, m.done(), "still in progress after queued submit") + + m = step(m, pollDoneMsg{status: MergeStatus{Queued: false, Merged: false, Message: "Merge conflict."}}) + out := m.Outcome() + assert.True(t, out.Failed) + assert.False(t, out.Merged) + assert.Equal(t, "Merge conflict.", out.Message) +} + +func TestProgress_QueuedThenMerged(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m.submitted = true + + m = step(m, submitDoneMsg{status: MergeStatus{Queued: true, UUID: "u1"}}) + m = step(m, pollDoneMsg{status: MergeStatus{Merged: true, SHA: "deadbee"}}) + out := m.Outcome() + assert.True(t, out.Merged) + assert.Equal(t, []int{1, 2, 3}, out.MergedPRs) +} + +func TestSubmit_NotMergeable(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m.submitted = true + m = step(m, submitDoneMsg{status: MergeStatus{Queued: false, Merged: false, Message: "Pull request is closed."}}) + out := m.Outcome() + assert.True(t, out.Failed) + assert.Equal(t, "Pull request is closed.", out.Message) +} + +func TestSubmit_TransportError(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m = step(m, submitDoneMsg{err: errors.New("boom")}) + out := m.Outcome() + assert.Error(t, out.Err) + assert.True(t, out.Failed) +} + +func TestCancel_FromSelect(t *testing.T) { + m := New(baseOptions()) + m = step(m, keyType(tea.KeyEsc)) + out := m.Outcome() + assert.True(t, out.Cancelled) + assert.False(t, out.Merged) +} + +func TestView_RendersBannerAndSteps(t *testing.T) { + m := New(baseOptions()) + sel := m.View() + assert.Contains(t, sel, "Merge stack") + assert.Contains(t, sel, "Select PRs") + assert.Contains(t, sel, "Select Merge Method") + assert.Contains(t, sel, "Confirm") + assert.Contains(t, sel, "Select how far up the stack") + + m = step(m, keyType(tea.KeyTab)) + assert.Contains(t, m.View(), "Choose a merge method") + + m = step(m, keyType(tea.KeyTab)) + confirm := m.View() + assert.Contains(t, confirm, "Merge 3 PRs") + assert.Contains(t, confirm, "#1, #2, #3") +} + +func TestPRCount(t *testing.T) { + assert.Equal(t, "1 PR", prCount(1)) + assert.Equal(t, "2 PRs", prCount(2)) + assert.Equal(t, "5 PRs", prCount(5)) +} diff --git a/internal/tui/mergeview/types.go b/internal/tui/mergeview/types.go new file mode 100644 index 00000000..b79a855d --- /dev/null +++ b/internal/tui/mergeview/types.go @@ -0,0 +1,109 @@ +// Package mergeview implements the interactive wizard used by `gh stack merge`. +// +// The wizard walks the user through three selection steps — choosing how far up +// the stack to merge (a bottom-anchored checkbox list), picking the merge +// method, and confirming — then shows a live progress view while the +// asynchronous merge runs on GitHub. Because a stack merge is atomic, the +// progress view reports a single aggregate outcome: all selected PRs merge, or +// none do. +// +// The async merge submit/poll calls are injected as SubmitFunc/PollFunc so the +// wizard stays decoupled from the GitHub client and is easy to test. +package mergeview + +import "time" + +// Step identifies the current stage of the wizard. +type Step int + +const ( + // StepSelectPRs is the bottom-anchored checkbox list choosing how far up + // the stack to merge. + StepSelectPRs Step = iota + // StepMethod is the merge-method picker. + StepMethod + // StepConfirm is the confirmation summary. + StepConfirm + // StepProgress shows the live async merge status. + StepProgress + // StepDone is the terminal state after success, failure, or cancel. + StepDone +) + +// PRItem is a selectable pull request in the merge picker, ordered bottom to top +// of the stack. +type PRItem struct { + Number int + Title string +} + +// MergeStatus is the minimal async-merge status the progress view consumes, +// mapped by the caller from the API response. +type MergeStatus struct { + // Queued reports the merge is still running in the background. + Queued bool + // Merged reports the merge completed successfully. + Merged bool + // Message is the human-readable status or failure reason. + Message string + // UUID identifies an in-flight merge request, used for polling. + UUID string + // SHA is the resulting merge commit on success. + SHA string +} + +// SubmitFunc submits the async merge for the chosen target PR and method and +// returns the initial status. +type SubmitFunc func(targetPR int, method string) (MergeStatus, error) + +// PollFunc fetches the latest status for an in-flight merge request UUID on the +// given target PR. +type PollFunc func(targetPR int, uuid string) (MergeStatus, error) + +// Options configures the wizard model. +type Options struct { + // PRs are the selectable (open, mergeable) pull requests ordered bottom to + // top of the stack. + PRs []PRItem + // BaseRef is the branch the stack merges into (for display). + BaseRef string + // RepoSlug is owner/repo, for display. + RepoSlug string + // AllowedMethods are the repo's enabled merge methods in display order + // (subset of "merge", "squash", "rebase"). + AllowedMethods []string + // DefaultMethod is the method preselected in the picker (the viewer's + // last-used method). + DefaultMethod string + // PreselectTopIndex, when >= 0, preselects PRs[0..PreselectTopIndex] and + // skips the PR-selection step (PR-number mode). + PreselectTopIndex int + // Submit and Poll perform the async merge; injected by the command. + Submit SubmitFunc + Poll PollFunc + // PollInterval is the delay between status polls. Defaults to one second. + PollInterval time.Duration +} + +// Outcome is the result the command reads back from the finished wizard. +type Outcome struct { + // Cancelled reports the user quit before the merge was submitted. + Cancelled bool + // Submitted reports a merge request was sent to GitHub. + Submitted bool + // Merged reports the merge completed successfully. + Merged bool + // Failed reports the merge was attempted but did not complete (conflict, + // rule failure, or not mergeable). + Failed bool + // Message is the final status or failure message. + Message string + // TargetPR is the topmost selected PR (the merge high-water mark). + TargetPR int + // Method is the chosen merge method. + Method string + // MergedPRs are the PR numbers included in the merge. + MergedPRs []int + // Err is a transport/API error encountered during submit or polling. + Err error +} diff --git a/internal/tui/mergeview/view.go b/internal/tui/mergeview/view.go new file mode 100644 index 00000000..7cee21c0 --- /dev/null +++ b/internal/tui/mergeview/view.go @@ -0,0 +1,271 @@ +package mergeview + +import ( + "fmt" + "strings" + + "github.com/charmbracelet/lipgloss" + "github.com/github/gh-stack/internal/theme" +) + +var ( + titleStyle = lipgloss.NewStyle().Foreground(theme.ColorText).Bold(true) + mutedStyle = lipgloss.NewStyle().Foreground(theme.ColorTextMuted) + faintStyle = lipgloss.NewStyle().Foreground(theme.ColorTextFaint) + accentStyle = lipgloss.NewStyle().Foreground(theme.ColorAccent) + numberStyle = lipgloss.NewStyle().Foreground(theme.ColorAccent).Bold(true) + checkedStyle = lipgloss.NewStyle().Foreground(theme.ColorGreen) + textStyle = lipgloss.NewStyle().Foreground(theme.ColorText) + successStyle = lipgloss.NewStyle().Foreground(theme.ColorGreen).Bold(true) + failureStyle = lipgloss.NewStyle().Foreground(theme.ColorRed).Bold(true) + + // Wizard stepper. + stepActiveStyle = lipgloss.NewStyle().Foreground(theme.ColorText).Background(theme.ColorRowShade).Bold(true).Padding(0, 1) + stepDoneStyle = lipgloss.NewStyle().Foreground(theme.ColorAccent).Padding(0, 1) + stepUpcomingStyle = lipgloss.NewStyle().Foreground(theme.ColorTextFaint).Padding(0, 1) + stepArrowStyle = lipgloss.NewStyle().Foreground(theme.ColorBorder) + + shortcutKey = lipgloss.NewStyle().Foreground(theme.ColorText) + shortcutLabel = lipgloss.NewStyle().Foreground(theme.ColorTextMuted) +) + +var wizardSteps = []string{"Select PRs", "Select Merge Method", "Confirm"} + +// View implements tea.Model. +func (m Model) View() string { + switch m.step { + case StepSelectPRs: + return m.banner() + m.viewSelect() + case StepMethod: + return m.banner() + m.viewMethod() + case StepConfirm: + return m.banner() + m.viewConfirm() + case StepProgress: + return m.banner() + m.viewProgress() + default: + return m.banner() + m.viewDone() + } +} + +// banner renders the persistent title and wizard stepper shown at the top of +// every step. +func (m Model) banner() string { + return titleStyle.Render("Merge stack") + "\n" + m.stepper() + "\n\n" +} + +func (m Model) stepper() string { + cur := m.wizardIndex() + parts := make([]string, len(wizardSteps)) + for i, label := range wizardSteps { + switch { + case i < cur: + parts[i] = stepDoneStyle.Render("✓ " + label) + case i == cur: + parts[i] = stepActiveStyle.Render(label) + default: + parts[i] = stepUpcomingStyle.Render(label) + } + } + return strings.Join(parts, stepArrowStyle.Render("▸")) +} + +// wizardIndex maps the current step to its position in the stepper. Progress and +// done are past the last selectable step, so all three read as complete. +func (m Model) wizardIndex() int { + switch m.step { + case StepSelectPRs: + return 0 + case StepMethod: + return 1 + case StepConfirm: + return 2 + default: + return len(wizardSteps) + } +} + +func (m Model) viewSelect() string { + var b strings.Builder + b.WriteString(mutedStyle.Render("Select how far up the stack to merge (everything up to your choice merges).") + "\n\n") + + // Render top of stack first so the layout matches the CLI. + for i := len(m.opts.PRs) - 1; i >= 0; i-- { + pr := m.opts.PRs[i] + cursor := " " + if i == m.cursor { + cursor = accentStyle.Render("❯ ") + } + box := "[ ]" + if i <= m.topIndex { + box = checkedStyle.Render("[x]") + } + num := numberStyle.Render(fmt.Sprintf("#%d", pr.Number)) + title := truncate(pr.Title, 60) + titleStyled := mutedStyle.Render(title) + if i <= m.topIndex { + titleStyled = textStyle.Render(title) + } + b.WriteString(fmt.Sprintf("%s%s %s %s\n", cursor, box, num, titleStyled)) + } + + b.WriteString("\n") + if m.topIndex >= 0 { + b.WriteString(mutedStyle.Render(fmt.Sprintf("Merging %s into %s.", prCount(m.topIndex+1), m.opts.BaseRef))) + } else { + b.WriteString(faintStyle.Render("Select at least one pull request.")) + } + b.WriteString("\n\n") + b.WriteString(shortcuts( + [2]string{"↑/↓", "move"}, + [2]string{"space", "toggle"}, + [2]string{"tab/enter", "next"}, + [2]string{"esc", "cancel"}, + )) + return b.String() +} + +func (m Model) viewMethod() string { + var b strings.Builder + b.WriteString(mutedStyle.Render("Choose a merge method.") + "\n\n") + + for i, method := range m.opts.AllowedMethods { + cursor := " " + if i == m.methodCursor { + cursor = accentStyle.Render("❯ ") + } + radio := "( )" + label := mutedStyle.Render(methodLabel(method)) + if i == m.methodCursor { + radio = checkedStyle.Render("(•)") + label = textStyle.Render(methodLabel(method)) + } + b.WriteString(fmt.Sprintf("%s%s %s\n", cursor, radio, label)) + } + + b.WriteString("\n") + b.WriteString(shortcuts( + [2]string{"↑/↓", "move"}, + [2]string{"tab/enter", "next"}, + [2]string{"shift+tab", "back"}, + [2]string{"esc", "cancel"}, + )) + return b.String() +} + +func (m Model) viewConfirm() string { + var b strings.Builder + nums := m.selectedNumbers() + + b.WriteString(fmt.Sprintf("%s into %s via %s.\n", + titleStyle.Render("Merge "+prCount(len(nums))), + accentStyle.Render(m.opts.BaseRef), + accentStyle.Render(methodLabel(m.method)), + )) + b.WriteString(numberStyle.Render(prNumberList(nums)) + "\n\n") + b.WriteString(shortcuts( + [2]string{"enter", "merge"}, + [2]string{"shift+tab", "back"}, + [2]string{"esc", "cancel"}, + )) + return b.String() +} + +func (m Model) viewProgress() string { + var b strings.Builder + nums := m.selectedNumbers() + + b.WriteString(fmt.Sprintf("%s Merging %s into %s via %s…\n", + m.spinner.View(), + numberStyle.Render(prNumberList(nums)), + accentStyle.Render(m.opts.BaseRef), + accentStyle.Render(methodLabel(m.method)), + )) + if m.message != "" { + b.WriteString(faintStyle.Render(m.message) + "\n") + } + b.WriteString("\n") + b.WriteString(faintStyle.Render("ctrl+c: stop watching (the merge keeps running on GitHub)")) + return b.String() +} + +func (m Model) viewDone() string { + var b strings.Builder + nums := m.selectedNumbers() + + switch { + case m.merged: + b.WriteString(successStyle.Render("✓ Merged") + " ") + b.WriteString(fmt.Sprintf("%s into %s.\n", numberStyle.Render(prNumberList(nums)), m.opts.BaseRef)) + if m.status.SHA != "" { + b.WriteString(faintStyle.Render("Merge commit "+shortSHA(m.status.SHA)) + "\n") + } + case m.failed: + b.WriteString(failureStyle.Render("✗ Merge failed") + "\n") + if m.message != "" { + b.WriteString(mutedStyle.Render(m.message) + "\n") + } + b.WriteString(faintStyle.Render("The stack is atomic, so nothing was merged.") + "\n") + case m.cancelled: + b.WriteString(mutedStyle.Render("Merge cancelled.") + "\n") + default: + if m.message != "" { + b.WriteString(mutedStyle.Render(m.message) + "\n") + } + } + return b.String() +} + +func shortcuts(entries ...[2]string) string { + parts := make([]string, 0, len(entries)) + for _, e := range entries { + parts = append(parts, shortcutKey.Render(e[0])+" "+shortcutLabel.Render(e[1])) + } + return strings.Join(parts, faintStyle.Render(" · ")) +} + +// prCount renders a pull-request count with correct pluralization: "1 PR" or +// "N PRs". +func prCount(n int) string { + if n == 1 { + return "1 PR" + } + return fmt.Sprintf("%d PRs", n) +} + +func methodLabel(method string) string { + switch method { + case "merge": + return "Create a merge commit" + case "squash": + return "Squash and merge" + case "rebase": + return "Rebase and merge" + default: + return method + } +} + +func prNumberList(nums []int) string { + parts := make([]string, len(nums)) + for i, n := range nums { + parts[i] = fmt.Sprintf("#%d", n) + } + return strings.Join(parts, ", ") +} + +func shortSHA(sha string) string { + if len(sha) > 7 { + return sha[:7] + } + return sha +} + +func truncate(s string, max int) string { + if len(s) <= max { + return s + } + if max <= 1 { + return s[:max] + } + return s[:max-1] + "…" +} From 3fda76c928b16a4944fa3ec7223d866e82ab34bf Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 22 Jul 2026 00:20:15 -0400 Subject: [PATCH 02/11] Refine the merge TUI and simplify the async-merge client MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up polish for `gh stack merge` (the command itself landed in the previous commit). These changes refine the interactive wizard, enrich the PR picker, and replace the merge client's bespoke HTTP handling with the standard go-gh REST client. Wizard and stepper: - Redesign the top stepper as a segmented bar: completed steps are green, the active step is the brightest, and upcoming steps are dimmed. Steps are separated by a Powerline arrow that blends into the shading, with a graceful fallback to abutting segments on terminals that lack the glyph (e.g. Apple Terminal). Set GH_STACK_POWERLINE=1/0 to override detection. - Show the stack number in the header ("Merge stack #123"). - Hide the header and stepper once the merge is submitted so the live progress view stands on its own. PR picker: - Render each pull request on two lines: the title (white/black, a touch bolder when selected) above its "#number • branch" (gray, fainter when deselected). Titles are fetched in one batched GraphQL query (PRTitles) and fall back to the branch name. - Scroll long stacks in a fixed 10-item window with persistent "N more" indicators, so the list no longer jumps as those hints appear and disappear. Add shift+up / shift+down to jump to the top or bottom. Progress and outcome: - Always render a status line ("Submitting merge request...") so it does not pop in later and shift the view, and normalize messages to end in an ellipsis. - Print the final result from the command layer rather than the TUI: a success line that includes the merge commit SHA ("Merged #1, #2 into main (abc1234)"), an atomic-rollback note on failure, a distinct message when the user stops watching an in-flight merge, and "Cancelled operation, nothing merged" on cancel. - Clamp every rendered line to the terminal width so resizing no longer leaves duplicated header lines behind, and make truncation ANSI-aware. Async-merge client: - Use the go-gh REST client (c.rest.Put / c.rest.Get) for both the submit and poll endpoints, removing the bespoke http.Client, base-URL helper, and manual response decoding. The REST client discards non-2xx bodies, but that only costs the rare 400 message and 409 UUID: real merge failures still surface through the 200 poll body, and the in-range PRs are validated open, non-draft, and non-merged before submitting. - Add classifyAsyncMergeError to map status codes to clear errors (404 unavailable, 409 already exists, 400 no longer mergeable) and drop the now-unused AsyncMergeResult.StatusCode field. Rework the client tests to drive the REST client through a stub http.RoundTripper. --- cmd/merge.go | 58 ++++-- cmd/merge_test.go | 18 +- internal/github/client_interface.go | 1 + internal/github/merge_async.go | 172 ++++++++-------- internal/github/merge_async_test.go | 104 ++++------ internal/github/mock_client.go | 10 +- internal/tui/mergeview/model.go | 85 ++++++-- internal/tui/mergeview/model_test.go | 122 +++++++++++- internal/tui/mergeview/types.go | 8 + internal/tui/mergeview/view.go | 280 +++++++++++++++++++-------- 10 files changed, 585 insertions(+), 273 deletions(-) diff --git a/cmd/merge.go b/cmd/merge.go index f285338d..230ac3a5 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -141,7 +141,7 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { base := remoteStack.Base.Ref if cfg.IsInteractive() && !opts.yes { - return runMergeInteractive(cfg, client, base, candidates, allowed, mergeCfg.DefaultMethod, method, preselectIndex, opts) + return runMergeInteractive(cfg, client, remoteStack.Number, base, candidates, allowed, mergeCfg.DefaultMethod, method, preselectIndex, opts) } // Non-interactive (or --yes): merge the whole stack (or up to the given PR) @@ -260,16 +260,30 @@ func resolveActiveRemoteStack(cfg *config.Config, client github.ClientOps) (*git return rs, nil } -func runMergeInteractive(cfg *config.Config, client github.ClientOps, base string, candidates []mergeview.PRItem, allowed []string, viewerDefault, methodFlag string, preselectIndex int, opts *mergeOptions) error { +func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumber int, base string, candidates []mergeview.PRItem, allowed []string, viewerDefault, methodFlag string, preselectIndex int, opts *mergeOptions) error { defaultMethod := viewerDefault if methodFlag != "" { defaultMethod = methodFlag } + // Enrich the picker with PR titles (best-effort; the branch is shown either way). + nums := make([]int, len(candidates)) + for i, c := range candidates { + nums[i] = c.Number + } + if titles, err := client.PRTitles(nums); err == nil { + for i := range candidates { + if t := titles[candidates[i].Number]; t != "" { + candidates[i].Title = t + } + } + } + submit, poll := mergeFuncs(client) model := mergeview.New(mergeview.Options{ PRs: candidates, + StackNumber: stackNumber, BaseRef: base, AllowedMethods: allowed, DefaultMethod: defaultMethod, @@ -292,13 +306,21 @@ func runMergeInteractive(cfg *config.Config, client github.ClientOps, base strin warnAsyncMergeUnavailable(cfg) return ErrStacksUnavailable } + cfg.Errorf("merge failed: %s", out.Err) return ErrAPIFailure case out.Merged: + mergedSuccess(cfg, prNumberList(out.MergedPRs), base, out.SHA) return nil case out.Failed: + cfg.Errorf("merge failed: %s", out.Message) + cfg.Printf("The stack is atomic, so nothing was merged.") return mergeFailureExit(out.Message) + case out.WatchStopped: + cfg.Infof("Stopped watching. Merge is still in progress. Check the pull requests on GitHub.") + return ErrSilent default: - // Cancelled, or watching was stopped while the merge continued. + // Cancelled via esc/ctrl+c before submitting. + cfg.Infof("Cancelled operation, nothing merged") return ErrSilent } } @@ -320,18 +342,15 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, } if res.Merged { - cfg.Successf("Merged %s into %s", list, base) + mergedSuccess(cfg, list, base, res.Details.SHA) return nil } - if !res.Queued { - cfg.Errorf("cannot merge: %s", res.Details.Message) - return ErrAPIFailure - } - if res.StatusCode == http.StatusConflict { - cfg.Infof("A merge request already exists for this stack; tracking it.") - } uuid := res.Details.UUID + if uuid == "" { + cfg.Errorf("merge did not start as expected") + return ErrAPIFailure + } interval := opts.pollInterval if interval <= 0 { interval = time.Second @@ -350,10 +369,7 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, return ErrAPIFailure } if status.Merged { - cfg.Successf("Merged %s into %s", list, base) - if sha := status.Details.SHA; sha != "" { - cfg.Printf(" Merge commit %s", shortMergeSHA(sha)) - } + mergedSuccess(cfg, list, base, status.Details.SHA) return nil } if !status.Queued { @@ -414,7 +430,7 @@ func mergeCandidates(rs *github.RemoteStack) (items []mergeview.PRItem, blocker b := pr return items, &b } - items = append(items, mergeview.PRItem{Number: pr.Number, Title: pr.Head.Ref}) + items = append(items, mergeview.PRItem{Number: pr.Number, Branch: pr.Head.Ref}) } return items, nil } @@ -572,6 +588,16 @@ func shortMergeSHA(sha string) string { return sha } +// mergedSuccess prints the merge success line, appending the merge commit SHA in +// parentheses when known: "Merged #1, #2 into main (abc1234)". +func mergedSuccess(cfg *config.Config, list, base, sha string) { + if sha != "" { + cfg.Successf("Merged %s into %s (%s)", list, base, shortMergeSHA(sha)) + return + } + cfg.Successf("Merged %s into %s", list, base) +} + func isNotFound(err error) bool { var httpErr *api.HTTPError return errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound diff --git a/cmd/merge_test.go b/cmd/merge_test.go index f7b9de87..1e79245f 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "net/http" "testing" "time" @@ -92,10 +93,10 @@ func TestRunMerge_NoArg_MergesWholeStack(t *testing.T) { }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { gotPR, gotMethod = pr, method - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}, StatusCode: 202}, nil + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc1234"}, StatusCode: 200}, nil + return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc1234"}}, nil }, } @@ -294,7 +295,7 @@ func TestRunMerge_SubmitNotMergeable(t *testing.T) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Queued: false, Merged: false, Details: github.AsyncMergeDetails{Message: "Pull request is closed."}, StatusCode: 400}, nil + return nil, errors.New("the stack can no longer be merged as requested; refresh and try again") }, } @@ -302,7 +303,8 @@ func TestRunMerge_SubmitNotMergeable(t *testing.T) { output := collectOutput(cfg, outR, errR) assert.ErrorIs(t, err, ErrAPIFailure) - assert.Contains(t, output, "cannot merge: Pull request is closed.") + assert.Contains(t, output, "failed to start merge") + assert.Contains(t, output, "can no longer be merged") } func TestRunMerge_PollFailedConflict(t *testing.T) { @@ -312,7 +314,7 @@ func TestRunMerge_PollFailedConflict(t *testing.T) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}, StatusCode: 202}, nil + return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { return &github.AsyncMergeResult{Queued: false, Merged: false, Details: github.AsyncMergeDetails{Message: "Merge conflict: could not merge."}}, nil @@ -334,7 +336,7 @@ func TestRunMerge_AlreadyMergedOnSubmit(t *testing.T) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc"}, StatusCode: 200}, nil + return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc"}}, nil }, } @@ -453,13 +455,13 @@ func TestMergeCandidates(t *testing.T) { t.Run("leading merged skipped", func(t *testing.T) { items, blocker := mergeCandidates(remoteStack(1, "main", mergedStackPR(1, "a"), openStackPR(2, "b"), openStackPR(3, "c"))) assert.Nil(t, blocker) - assert.Equal(t, []mergeview.PRItem{{Number: 2, Title: "b"}, {Number: 3, Title: "c"}}, items) + assert.Equal(t, []mergeview.PRItem{{Number: 2, Branch: "b"}, {Number: 3, Branch: "c"}}, items) }) t.Run("draft blocks above", func(t *testing.T) { items, blocker := mergeCandidates(remoteStack(1, "main", openStackPR(1, "a"), draftStackPR(2, "b"), openStackPR(3, "c"))) require.NotNil(t, blocker) assert.Equal(t, 2, blocker.Number) - assert.Equal(t, []mergeview.PRItem{{Number: 1, Title: "a"}}, items) + assert.Equal(t, []mergeview.PRItem{{Number: 1, Branch: "a"}}, items) }) t.Run("closed blocks", func(t *testing.T) { items, blocker := mergeCandidates(remoteStack(1, "main", closedStackPR(1, "a"), openStackPR(2, "b"))) diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index 2903df5d..dd3bfe0c 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -20,6 +20,7 @@ type ClientOps interface { RepoMergeConfig() (*RepoMergeConfig, error) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) + PRTitles(numbers []int) (map[int]string, error) } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go index 1dd68270..34e29726 100644 --- a/internal/github/merge_async.go +++ b/internal/github/merge_async.go @@ -5,11 +5,10 @@ import ( "encoding/json" "errors" "fmt" - "io" "net/http" "strings" - "github.com/cli/go-gh/v2/pkg/auth" + "github.com/cli/go-gh/v2/pkg/api" graphql "github.com/cli/shurcooL-graphql" ) @@ -78,14 +77,12 @@ type AsyncMergeDetails struct { } // AsyncMergeResult is the response body returned by both the submit and poll -// async merge endpoints. StatusCode carries the HTTP status of the submit -// response so callers can distinguish enqueued (202) from an existing request -// (409) and an already-merged PR (200). +// async merge endpoints. The Queued/Merged flags distinguish an enqueued merge +// (202) from an already-merged pull request (200). type AsyncMergeResult struct { - Queued bool `json:"queued"` - Merged bool `json:"merged"` - Details AsyncMergeDetails `json:"details"` - StatusCode int `json:"-"` + Queued bool `json:"queued"` + Merged bool `json:"merged"` + Details AsyncMergeDetails `json:"details"` } // InProgress reports whether the merge is still queued (running in the @@ -128,9 +125,10 @@ func (c *Client) RepoMergeConfig() (*RepoMergeConfig, error) { // a stacked PR this merges all members of the stack up to and including // prNumber. A blank method lets the server apply its default. // -// The returned result is populated for the 200 (already merged), 202 (enqueued) -// 409 (a request already exists) and 400 (not mergeable) responses; the HTTP -// status is recorded on StatusCode. A 404 returns ErrAsyncMergeUnavailable. +// On success the returned result is populated for the 200 (already merged) and +// 202 (enqueued) responses. A 404 returns ErrAsyncMergeUnavailable, a 409 +// (a request already exists) returns a clear "already exists" error, and any +// other non-2xx status is returned as-is. func (c *Client) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) { type reqBody struct { MergeMethod string `json:"merge_method,omitempty"` @@ -142,77 +140,92 @@ func (c *Client) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult } path := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async", c.owner, c.repo, prNumber) - resp, err := c.doAsyncRequest(http.MethodPut, path, bytes.NewReader(body)) - if err != nil { - return nil, err - } - defer func() { _ = resp.Body.Close() }() - - switch resp.StatusCode { - case http.StatusOK, http.StatusAccepted, http.StatusConflict, http.StatusBadRequest: - return decodeAsyncMergeResult(resp) - case http.StatusNotFound: - return nil, ErrAsyncMergeUnavailable - default: - return nil, asyncMergeError(resp) + var result AsyncMergeResult + if err := c.rest.Put(path, bytes.NewReader(body), &result); err != nil { + return nil, classifyAsyncMergeError(err) } + return &result, nil } // GetAsyncMergeResult fetches the current result of a previously submitted async -// merge, identified by the UUID returned from MergeStackAsync. +// merge, identified by the UUID returned from MergeStackAsync. A valid lookup +// always returns 200, so the wrapped Queued/Merged/Details state reflects the +// merge's progress (queued, merged, or failed). func (c *Client) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) { path := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async/%s", c.owner, c.repo, prNumber, uuid) - resp, err := c.doAsyncRequest(http.MethodGet, path, nil) - if err != nil { - return nil, err - } - defer func() { _ = resp.Body.Close() }() - - if resp.StatusCode == http.StatusOK { - return decodeAsyncMergeResult(resp) - } - return nil, asyncMergeError(resp) -} - -// doAsyncRequest issues an authenticated request to the REST API and returns the -// raw response without treating non-2xx statuses as errors, so the caller can -// read the merge result body for 4xx responses (which carry the UUID/message). -func (c *Client) doAsyncRequest(method, path string, body io.Reader) (*http.Response, error) { - req, err := http.NewRequest(method, c.base+path, body) - if err != nil { + var result AsyncMergeResult + if err := c.rest.Get(path, &result); err != nil { return nil, err } - req.Header.Set("Accept", "application/vnd.github+json") - if body != nil { - req.Header.Set("Content-Type", "application/json") - } - return c.http.Do(req) + return &result, nil } -func decodeAsyncMergeResult(resp *http.Response) (*AsyncMergeResult, error) { - var r AsyncMergeResult - if err := json.NewDecoder(resp.Body).Decode(&r); err != nil { - return nil, fmt.Errorf("decoding merge response: %w", err) - } - r.StatusCode = resp.StatusCode - return &r, nil +// PRTitles fetches the titles for a set of pull request numbers in a single +// GraphQL query. Missing PRs are simply absent from the result. Best-effort: +// callers may ignore the error and proceed without titles. +func (c *Client) PRTitles(numbers []int) (map[int]string, error) { + titles := make(map[int]string, len(numbers)) + if len(numbers) == 0 { + return titles, nil + } + + // Batch to keep individual queries small for very large stacks. + const batchSize = 50 + for start := 0; start < len(numbers); start += batchSize { + end := start + batchSize + if end > len(numbers) { + end = len(numbers) + } + + var q strings.Builder + q.WriteString("query($owner:String!,$name:String!){repository(owner:$owner,name:$name){") + for i, n := range numbers[start:end] { + fmt.Fprintf(&q, "pr%d:pullRequest(number:%d){number title} ", i, n) + } + q.WriteString("}}") + + var resp struct { + Repository map[string]struct { + Number int `json:"number"` + Title string `json:"title"` + } `json:"repository"` + } + vars := map[string]interface{}{"owner": c.owner, "name": c.repo} + if err := c.gql.Do(q.String(), vars, &resp); err != nil { + return titles, fmt.Errorf("querying pull request titles: %w", err) + } + for _, pr := range resp.Repository { + if pr.Number != 0 { + titles[pr.Number] = pr.Title + } + } + } + return titles, nil } -// asyncMergeError builds an error from an unexpected (403/422/5xx) response, -// extracting the API message when present. -func asyncMergeError(resp *http.Response) error { - b, _ := io.ReadAll(resp.Body) - var parsed struct { - Message string `json:"message"` - } - _ = json.Unmarshal(b, &parsed) - if parsed.Message != "" { - return fmt.Errorf("HTTP %d: %s", resp.StatusCode, parsed.Message) - } - if trimmed := strings.TrimSpace(string(b)); trimmed != "" { - return fmt.Errorf("HTTP %d: %s", resp.StatusCode, trimmed) - } - return fmt.Errorf("HTTP %d", resp.StatusCode) +// classifyAsyncMergeError maps a go-gh REST error into a domain error. A 404 +// means async merge isn't available for the repository or token; a 409 means a +// merge request already exists for this stack. Other errors pass through. +// +// Note: the go-gh REST client discards non-2xx response bodies, so the specific +// "details.message" from a 400 (not mergeable) and the existing UUID from a 409 +// aren't recovered here. Those are rare — the in-range PRs are validated open, +// non-draft and non-merged before submitting, and real merge failures (e.g. +// conflicts) surface through the 200 poll body — so status-based handling is +// sufficient. +func classifyAsyncMergeError(err error) error { + var httpErr *api.HTTPError + if errors.As(err, &httpErr) { + switch httpErr.StatusCode { + case http.StatusNotFound: + return ErrAsyncMergeUnavailable + case http.StatusConflict: + return errors.New("a merge request already exists for this stack") + case http.StatusBadRequest: + return errors.New("the stack can no longer be merged as requested; refresh and try again") + } + } + return err } // mergeMethodFromEnum maps a GraphQL PullRequestMergeMethod enum value @@ -228,20 +241,3 @@ func mergeMethodFromEnum(enum string) string { return MergeMethodMerge } } - -// restBaseURL derives the REST API base URL for a host, mirroring go-gh's -// internal restPrefix so raw requests target the same endpoint as the REST -// client. -func restBaseURL(host string) string { - if host == "" { - host = "github.com" - } - normalized := auth.NormalizeHostname(host) - if auth.IsEnterprise(normalized) { - return fmt.Sprintf("https://%s/api/v3/", normalized) - } - if strings.EqualFold(normalized, "github.localhost") { - return fmt.Sprintf("http://api.%s/", normalized) - } - return fmt.Sprintf("https://api.%s/", normalized) -} diff --git a/internal/github/merge_async_test.go b/internal/github/merge_async_test.go index 1c7b1784..cb1b2902 100644 --- a/internal/github/merge_async_test.go +++ b/internal/github/merge_async_test.go @@ -4,18 +4,17 @@ import ( "encoding/json" "io" "net/http" - "net/http/httptest" + "strings" "testing" + "github.com/cli/go-gh/v2/pkg/api" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) -// testAsyncClient builds a Client wired to a test server for the async merge -// REST methods, which only use the raw HTTP client and base URL. -func testAsyncClient(base string) *Client { - return &Client{http: http.DefaultClient, base: base + "/", owner: "o", repo: "r", slug: "o/r"} -} +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } type recordedRequest struct { method string @@ -23,29 +22,37 @@ type recordedRequest struct { body string } -func serveOnce(t *testing.T, status int, respBody string, rec *recordedRequest) *httptest.Server { +// testAsyncClient builds a Client whose REST client is backed by a stub +// transport returning the given status and body. When rec is non-nil the +// request's method, path and body are captured for assertions. +func testAsyncClient(t *testing.T, status int, respBody string, rec *recordedRequest) *Client { t.Helper() - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + rt := roundTripFunc(func(r *http.Request) (*http.Response, error) { if rec != nil { - b, _ := io.ReadAll(r.Body) rec.method = r.Method rec.path = r.URL.Path - rec.body = string(b) + if r.Body != nil { + b, _ := io.ReadAll(r.Body) + rec.body = string(b) + } } - w.Header().Set("Content-Type", "application/json") - w.WriteHeader(status) - _, _ = io.WriteString(w, respBody) - })) - t.Cleanup(srv.Close) - return srv + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(strings.NewReader(respBody)), + Header: http.Header{"Content-Type": []string{"application/json"}}, + Request: r, + }, nil + }) + rest, err := api.NewRESTClient(api.ClientOptions{Host: "github.com", AuthToken: "x", Transport: rt}) + require.NoError(t, err) + return &Client{rest: rest, owner: "o", repo: "r"} } func TestMergeStackAsync_Accepted(t *testing.T) { var rec recordedRequest body := `{"queued":true,"merged":false,"details":{"message":"Merge request enqueued.","uuid":"u-1","merge_method":"squash","expected_head_sha":"abc"}}` - srv := serveOnce(t, http.StatusAccepted, body, &rec) + c := testAsyncClient(t, http.StatusAccepted, body, &rec) - c := testAsyncClient(srv.URL) res, err := c.MergeStackAsync(42, "squash") require.NoError(t, err) @@ -55,7 +62,6 @@ func TestMergeStackAsync_Accepted(t *testing.T) { assert.True(t, res.Queued) assert.False(t, res.Merged) - assert.Equal(t, http.StatusAccepted, res.StatusCode) assert.Equal(t, "u-1", res.Details.UUID) assert.Equal(t, "squash", res.Details.MergeMethod) assert.True(t, res.InProgress()) @@ -63,46 +69,35 @@ func TestMergeStackAsync_Accepted(t *testing.T) { func TestMergeStackAsync_AlreadyMerged(t *testing.T) { body := `{"queued":false,"merged":true,"details":{"message":"Pull request is already merged.","sha":"deadbeef"}}` - srv := serveOnce(t, http.StatusOK, body, nil) - - res, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + res, err := testAsyncClient(t, http.StatusOK, body, nil).MergeStackAsync(42, "merge") require.NoError(t, err) assert.True(t, res.Merged) assert.Equal(t, "deadbeef", res.Details.SHA) - assert.Equal(t, http.StatusOK, res.StatusCode) } func TestMergeStackAsync_ExistingRequestConflict(t *testing.T) { - body := `{"queued":true,"merged":false,"details":{"message":"A merge request already exists for this pull request.","uuid":"u-2","merge_method":"merge","expected_head_sha":"abc"}}` - srv := serveOnce(t, http.StatusConflict, body, nil) - - res, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") - require.NoError(t, err) - assert.Equal(t, http.StatusConflict, res.StatusCode) - assert.Equal(t, "u-2", res.Details.UUID) - assert.True(t, res.InProgress()) + // The go-gh REST client discards the 409 body, so we can't recover the + // existing UUID; the request surfaces as a clear "already exists" error. + _, err := testAsyncClient(t, http.StatusConflict, `{"queued":true,"merged":false,"details":{"uuid":"u-2"}}`, nil).MergeStackAsync(42, "merge") + require.Error(t, err) + assert.Contains(t, err.Error(), "already exists") } func TestMergeStackAsync_NotMergeable(t *testing.T) { - body := `{"queued":false,"merged":false,"details":{"message":"Pull request is closed."}}` - srv := serveOnce(t, http.StatusBadRequest, body, nil) - - res, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") - require.NoError(t, err) - assert.False(t, res.Queued) - assert.False(t, res.Merged) - assert.Equal(t, "Pull request is closed.", res.Details.Message) + // A 400 preflight failure is reported as a clear error (the specific + // details.message isn't recoverable through the REST client). + _, err := testAsyncClient(t, http.StatusBadRequest, `{"queued":false,"merged":false,"details":{"message":"Pull request is closed."}}`, nil).MergeStackAsync(42, "merge") + require.Error(t, err) + assert.Contains(t, err.Error(), "can no longer be merged") } func TestMergeStackAsync_NotAvailable(t *testing.T) { - srv := serveOnce(t, http.StatusNotFound, `{"message":"Not Found"}`, nil) - _, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusNotFound, `{"message":"Not Found"}`, nil).MergeStackAsync(42, "merge") assert.ErrorIs(t, err, ErrAsyncMergeUnavailable) } func TestMergeStackAsync_ValidationFailed(t *testing.T) { - srv := serveOnce(t, http.StatusUnprocessableEntity, `{"message":"Validation Failed"}`, nil) - _, err := testAsyncClient(srv.URL).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusUnprocessableEntity, `{"message":"Validation Failed"}`, nil).MergeStackAsync(42, "merge") require.Error(t, err) assert.Contains(t, err.Error(), "Validation Failed") } @@ -121,8 +116,7 @@ func TestGetAsyncMergeResult_States(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { var rec recordedRequest - srv := serveOnce(t, http.StatusOK, tt.body, &rec) - res, err := testAsyncClient(srv.URL).GetAsyncMergeResult(42, "u") + res, err := testAsyncClient(t, http.StatusOK, tt.body, &rec).GetAsyncMergeResult(42, "u") require.NoError(t, err) assert.Equal(t, http.MethodGet, rec.method) assert.Equal(t, "/repos/o/r/pulls/42/merge-async/u", rec.path) @@ -133,25 +127,10 @@ func TestGetAsyncMergeResult_States(t *testing.T) { } func TestGetAsyncMergeResult_NotFound(t *testing.T) { - srv := serveOnce(t, http.StatusNotFound, `{"message":"Not Found"}`, nil) - _, err := testAsyncClient(srv.URL).GetAsyncMergeResult(42, "missing") + _, err := testAsyncClient(t, http.StatusNotFound, `{"message":"Not Found"}`, nil).GetAsyncMergeResult(42, "missing") require.Error(t, err) } -func TestRestBaseURL(t *testing.T) { - tests := []struct { - host string - want string - }{ - {"", "https://api.github.com/"}, - {"github.com", "https://api.github.com/"}, - {"github.example.com", "https://github.example.com/api/v3/"}, - } - for _, tt := range tests { - assert.Equal(t, tt.want, restBaseURL(tt.host), "host %q", tt.host) - } -} - func TestMergeMethodFromEnum(t *testing.T) { assert.Equal(t, MergeMethodMerge, mergeMethodFromEnum("MERGE")) assert.Equal(t, MergeMethodSquash, mergeMethodFromEnum("SQUASH")) @@ -182,8 +161,7 @@ func TestAsyncMergeResult_InProgress(t *testing.T) { // sanity check that the submit body omits merge_method when empty. func TestMergeStackAsync_OmitsEmptyMethod(t *testing.T) { var rec recordedRequest - srv := serveOnce(t, http.StatusAccepted, `{"queued":true,"merged":false,"details":{"message":"m","uuid":"u","merge_method":"merge","expected_head_sha":"x"}}`, &rec) - _, err := testAsyncClient(srv.URL).MergeStackAsync(1, "") + _, err := testAsyncClient(t, http.StatusAccepted, `{"queued":true,"merged":false,"details":{"message":"m","uuid":"u","merge_method":"merge","expected_head_sha":"x"}}`, &rec).MergeStackAsync(1, "") require.NoError(t, err) var parsed map[string]any diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index d9b28791..64b50681 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -20,6 +20,7 @@ type MockClient struct { RepoMergeConfigFn func() (*RepoMergeConfig, error) MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) + PRTitlesFn func([]int) (map[int]string, error) } // Compile-time check that MockClient satisfies ClientOps. @@ -139,7 +140,6 @@ func (m *MockClient) MergeStackAsync(prNumber int, method string) (*AsyncMergeRe UUID: "mock-uuid", MergeMethod: method, }, - StatusCode: 202, }, nil } @@ -153,6 +153,12 @@ func (m *MockClient) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMerge Message: "Pull request was merged.", SHA: "mockmergesha", }, - StatusCode: 200, }, nil } + +func (m *MockClient) PRTitles(numbers []int) (map[int]string, error) { + if m.PRTitlesFn != nil { + return m.PRTitlesFn(numbers) + } + return map[int]string{}, nil +} diff --git a/internal/tui/mergeview/model.go b/internal/tui/mergeview/model.go index fcefc33e..b307d688 100644 --- a/internal/tui/mergeview/model.go +++ b/internal/tui/mergeview/model.go @@ -8,6 +8,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/charmbracelet/lipgloss" "github.com/github/gh-stack/internal/theme" + "github.com/github/gh-stack/internal/tui/shared" ) // Model is the Bubble Tea model backing the merge wizard. @@ -27,16 +28,20 @@ type Model struct { spinner spinner.Model status MergeStatus - submitted bool - merged bool - failed bool - cancelled bool - message string - err error + submitted bool + merged bool + failed bool + cancelled bool + watchStopped bool + message string + err error pollInterval time.Duration - width int + width int + height int + scrollOffset int + usePowerline bool } // New builds a merge wizard model from the given options. @@ -57,6 +62,7 @@ func New(opts Options) Model { method: normalizeDefaultMethod(opts), pollInterval: interval, spinner: sp, + usePowerline: powerlineEnabled(), } m.methodCursor = indexOf(opts.AllowedMethods, m.method) @@ -78,6 +84,8 @@ func (m Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case tea.WindowSizeMsg: m.width = msg.Width + m.height = msg.Height + m.scrollOffset = m.clampScroll() return m, nil case spinner.TickMsg: @@ -110,7 +118,7 @@ func (m Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { if key == "ctrl+c" { if m.step == StepProgress && m.submitted && !m.done() { // The merge is running server-side; stop watching without cancelling it. - m.message = "Stopped watching. The merge continues on GitHub." + m.watchStopped = true return m.finish() } m.cancelled = true @@ -140,6 +148,12 @@ func (m Model) handleSelectKey(key string) (tea.Model, tea.Cmd) { if m.cursor > 0 { m.cursor-- } + case "shift+up": + // Jump to the top of the stack. + m.cursor = len(m.opts.PRs) - 1 + case "shift+down": + // Jump to the bottom of the stack. + m.cursor = 0 case " ", "x": if m.cursor <= m.topIndex { // Uncheck the cursor and everything above it. @@ -156,9 +170,46 @@ func (m Model) handleSelectKey(key string) (tea.Model, tea.Cmd) { m.cancelled = true return m.finish() } + m.scrollOffset = m.clampScroll() return m, nil } +// maxVisibleItems caps how many pull requests the select step shows at once; the +// rest are reached by scrolling so the picker never takes over the screen. +const maxVisibleItems = 10 + +// visibleItems is the number of pull requests shown in the select window at +// once — capped at maxVisibleItems and shrunk to fit a short terminal (each +// item renders on two lines). When the terminal size is unknown, all are shown. +func (m Model) visibleItems() int { + n := len(m.opts.PRs) + if m.height <= 0 { + return n + } + // Reserve lines for the header, scroll indicators, summary, and footer. + const chrome = 11 + avail := (m.height - chrome) / 2 + limit := maxVisibleItems + if avail < limit { + limit = avail + } + if limit < 1 { + limit = 1 + } + if n < limit { + limit = n + } + return limit +} + +// clampScroll returns a scroll offset (in display rows, where row 0 is the top +// of the stack) that keeps the cursor's row visible within the select window. +func (m Model) clampScroll() int { + n := len(m.opts.PRs) + cursorRow := n - 1 - m.cursor + return shared.EnsureVisible(cursorRow, cursorRow+1, m.scrollOffset, m.visibleItems()) +} + func (m Model) handleMethodKey(key string) (tea.Model, tea.Cmd) { switch key { case "up", "k": @@ -258,14 +309,16 @@ func (m Model) done() bool { return m.merged || m.failed || m.step == StepDone } // Outcome reports the final result of the wizard for the command layer. func (m Model) Outcome() Outcome { o := Outcome{ - Cancelled: m.cancelled, - Submitted: m.submitted, - Merged: m.merged, - Failed: m.failed, - Message: m.message, - TargetPR: m.targetPR(), - Method: m.method, - Err: m.err, + Cancelled: m.cancelled, + Submitted: m.submitted, + Merged: m.merged, + Failed: m.failed, + WatchStopped: m.watchStopped, + Message: m.message, + TargetPR: m.targetPR(), + Method: m.method, + SHA: m.status.SHA, + Err: m.err, } if m.merged { o.MergedPRs = m.selectedNumbers() diff --git a/internal/tui/mergeview/model_test.go b/internal/tui/mergeview/model_test.go index be77b139..dd93b6b7 100644 --- a/internal/tui/mergeview/model_test.go +++ b/internal/tui/mergeview/model_test.go @@ -2,6 +2,8 @@ package mergeview import ( "errors" + "fmt" + "strings" "testing" tea "github.com/charmbracelet/bubbletea" @@ -11,7 +13,7 @@ import ( func baseOptions() Options { return Options{ - PRs: []PRItem{{Number: 1, Title: "a"}, {Number: 2, Title: "b"}, {Number: 3, Title: "c"}}, + PRs: []PRItem{{Number: 1, Title: "a", Branch: "feat-a"}, {Number: 2, Title: "b", Branch: "feat-b"}, {Number: 3, Title: "c", Branch: "feat-c"}}, BaseRef: "main", AllowedMethods: []string{"merge", "squash", "rebase"}, DefaultMethod: "squash", @@ -81,6 +83,45 @@ func TestSelect_CascadeToggle(t *testing.T) { assert.Equal(t, 1, m.targetPR()) } +func TestSelect_Viewport(t *testing.T) { + opts := baseOptions() + opts.PRs = nil + for i := 1; i <= 30; i++ { + opts.PRs = append(opts.PRs, PRItem{Number: i, Title: fmt.Sprintf("Title %d", i), Branch: fmt.Sprintf("b%d", i)}) + } + m := New(opts) + + // No size yet: all items are shown. + assert.Equal(t, 30, m.visibleItems()) + + // A tall terminal caps the window at maxVisibleItems (10). + nm, _ := m.Update(tea.WindowSizeMsg{Width: 100, Height: 60}) + m = nm.(Model) + assert.Equal(t, 10, m.visibleItems()) + assert.Equal(t, 0, m.scrollOffset) // cursor starts at the top of the stack + + // A short terminal shrinks the window further (each item is two lines). + nm, _ = m.Update(tea.WindowSizeMsg{Width: 100, Height: 30}) + m = nm.(Model) + assert.LessOrEqual(t, m.visibleItems(), 10) + assert.Greater(t, m.visibleItems(), 0) + + // Scrolling down keeps the cursor's display row within the window. + for i := 0; i < 20; i++ { + nm, _ := m.Update(tea.KeyMsg{Type: tea.KeyDown}) + m = nm.(Model) + cursorRow := len(m.opts.PRs) - 1 - m.cursor + assert.GreaterOrEqual(t, cursorRow, m.scrollOffset) + assert.Less(t, cursorRow, m.scrollOffset+m.visibleItems()) + } + + // The rendered select view shows at most visibleItems PRs (line 2 of each + // item contains "#N • branch") and a scroll indicator. + view := m.viewSelect() + assert.LessOrEqual(t, strings.Count(view, "•"), m.visibleItems()) + assert.Contains(t, view, "more") +} + func TestSelect_ArrowDirection(t *testing.T) { m := New(baseOptions()) // cursor starts at the top of the stack (index 2) assert.Equal(t, 2, m.cursor) @@ -218,6 +259,32 @@ func TestCancel_FromSelect(t *testing.T) { assert.False(t, out.Merged) } +func TestView_ClearsOnDone(t *testing.T) { + m := New(baseOptions()) + m = step(m, keyType(tea.KeyEsc)) // cancel -> StepDone + require.Equal(t, StepDone, m.step) + assert.Equal(t, "", m.View(), "the done state renders nothing so the inline TUI clears itself") +} + +func TestProgress_WatchStopped(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m.submitted = true + m = step(m, submitDoneMsg{status: MergeStatus{Queued: true, UUID: "u"}}) // in-flight + m = step(m, keyType(tea.KeyCtrlC)) + out := m.Outcome() + assert.True(t, out.WatchStopped) + assert.False(t, out.Cancelled) + assert.False(t, out.Merged) + assert.Equal(t, "", m.View()) +} + +func TestOutcome_SHA(t *testing.T) { + m := New(baseOptions()) + m = step(m, submitDoneMsg{status: MergeStatus{Merged: true, SHA: "deadbeef"}}) + assert.Equal(t, "deadbeef", m.Outcome().SHA) +} + func TestView_RendersBannerAndSteps(t *testing.T) { m := New(baseOptions()) sel := m.View() @@ -225,10 +292,11 @@ func TestView_RendersBannerAndSteps(t *testing.T) { assert.Contains(t, sel, "Select PRs") assert.Contains(t, sel, "Select Merge Method") assert.Contains(t, sel, "Confirm") - assert.Contains(t, sel, "Select how far up the stack") + assert.Contains(t, sel, "Will merge 3 PRs into main") + assert.Contains(t, sel, "feat-a") // branch shown on the item's second line m = step(m, keyType(tea.KeyTab)) - assert.Contains(t, m.View(), "Choose a merge method") + assert.Contains(t, m.View(), "Squash and merge") // method labels, no subheading m = step(m, keyType(tea.KeyTab)) confirm := m.View() @@ -236,6 +304,54 @@ func TestView_RendersBannerAndSteps(t *testing.T) { assert.Contains(t, confirm, "#1, #2, #3") } +func TestBanner_IncludesStackNumber(t *testing.T) { + opts := baseOptions() + opts.StackNumber = 42 + m := New(opts) + assert.Contains(t, m.View(), "Merge stack #42") +} + +func TestProgress_HidesHeader(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + v := m.View() + assert.NotContains(t, v, "Merge stack", "header/wizard hidden during progress") + assert.NotContains(t, v, "Select PRs") + assert.Contains(t, v, "Merging") + assert.NotContains(t, v, "…", "no trailing ellipsis on the Merging line") +} + +func TestSelect_JumpTopBottom(t *testing.T) { + opts := baseOptions() + opts.PRs = nil + for i := 1; i <= 6; i++ { + opts.PRs = append(opts.PRs, PRItem{Number: i, Branch: fmt.Sprintf("b%d", i)}) + } + m := New(opts) // cursor starts at the top of the stack (index 5) + require.Equal(t, 5, m.cursor) + + m = step(m, keyType(tea.KeyShiftDown)) // jump to bottom + assert.Equal(t, 0, m.cursor) + + m = step(m, keyType(tea.KeyShiftUp)) // jump to top + assert.Equal(t, 5, m.cursor) +} + +func TestProgressStatus(t *testing.T) { + assert.Equal(t, "Submitting merge request...", progressStatus("")) + assert.Equal(t, "Submitting merge request...", progressStatus(" ")) + assert.Equal(t, "Merge request is in progress...", progressStatus("Merge request is in progress.")) + assert.Equal(t, "Merge request enqueued...", progressStatus("Merge request enqueued.")) +} + +func TestStepper_PowerlineFallback(t *testing.T) { + t.Setenv("GH_STACK_POWERLINE", "0") + assert.NotContains(t, New(baseOptions()).View(), "\ue0b0", "no Powerline glyph in fallback mode") + + t.Setenv("GH_STACK_POWERLINE", "1") + assert.Contains(t, New(baseOptions()).View(), "\ue0b0", "Powerline glyph when enabled") +} + func TestPRCount(t *testing.T) { assert.Equal(t, "1 PR", prCount(1)) assert.Equal(t, "2 PRs", prCount(2)) diff --git a/internal/tui/mergeview/types.go b/internal/tui/mergeview/types.go index b79a855d..31af3476 100644 --- a/internal/tui/mergeview/types.go +++ b/internal/tui/mergeview/types.go @@ -35,6 +35,7 @@ const ( type PRItem struct { Number int Title string + Branch string } // MergeStatus is the minimal async-merge status the progress view consumes, @@ -65,6 +66,8 @@ type Options struct { // PRs are the selectable (open, mergeable) pull requests ordered bottom to // top of the stack. PRs []PRItem + // StackNumber is the repo-scoped stack number, shown in the header. + StackNumber int // BaseRef is the branch the stack merges into (for display). BaseRef string // RepoSlug is owner/repo, for display. @@ -96,6 +99,9 @@ type Outcome struct { // Failed reports the merge was attempted but did not complete (conflict, // rule failure, or not mergeable). Failed bool + // WatchStopped reports the user stopped watching an in-flight merge (ctrl+c + // during progress); the merge continues on GitHub. + WatchStopped bool // Message is the final status or failure message. Message string // TargetPR is the topmost selected PR (the merge high-water mark). @@ -104,6 +110,8 @@ type Outcome struct { Method string // MergedPRs are the PR numbers included in the merge. MergedPRs []int + // SHA is the resulting merge commit on success. + SHA string // Err is a transport/API error encountered during submit or polling. Err error } diff --git a/internal/tui/mergeview/view.go b/internal/tui/mergeview/view.go index 7cee21c0..2d45d53d 100644 --- a/internal/tui/mergeview/view.go +++ b/internal/tui/mergeview/view.go @@ -2,6 +2,7 @@ package mergeview import ( "fmt" + "os" "strings" "github.com/charmbracelet/lipgloss" @@ -16,57 +17,128 @@ var ( numberStyle = lipgloss.NewStyle().Foreground(theme.ColorAccent).Bold(true) checkedStyle = lipgloss.NewStyle().Foreground(theme.ColorGreen) textStyle = lipgloss.NewStyle().Foreground(theme.ColorText) - successStyle = lipgloss.NewStyle().Foreground(theme.ColorGreen).Bold(true) - failureStyle = lipgloss.NewStyle().Foreground(theme.ColorRed).Bold(true) - - // Wizard stepper. - stepActiveStyle = lipgloss.NewStyle().Foreground(theme.ColorText).Background(theme.ColorRowShade).Bold(true).Padding(0, 1) - stepDoneStyle = lipgloss.NewStyle().Foreground(theme.ColorAccent).Padding(0, 1) - stepUpcomingStyle = lipgloss.NewStyle().Foreground(theme.ColorTextFaint).Padding(0, 1) - stepArrowStyle = lipgloss.NewStyle().Foreground(theme.ColorBorder) + // selectedTitleStyle makes the selected PR's title stand out a touch more + // than the others while staying white/black. + selectedTitleStyle = lipgloss.NewStyle().Foreground(theme.ColorText).Bold(true) shortcutKey = lipgloss.NewStyle().Foreground(theme.ColorText) shortcutLabel = lipgloss.NewStyle().Foreground(theme.ColorTextMuted) ) +// stepArrow is the Powerline right-triangle separator, rendered in the current +// segment's background color over the next segment's background so the arrow +// blends seamlessly into the shading. +const stepArrow = "\ue0b0" + var wizardSteps = []string{"Select PRs", "Select Merge Method", "Confirm"} // View implements tea.Model. func (m Model) View() string { + var s string switch m.step { case StepSelectPRs: - return m.banner() + m.viewSelect() + s = m.banner() + m.viewSelect() case StepMethod: - return m.banner() + m.viewMethod() + s = m.banner() + m.viewMethod() case StepConfirm: - return m.banner() + m.viewConfirm() + s = m.banner() + m.viewConfirm() case StepProgress: - return m.banner() + m.viewProgress() + // Once the merge is submitted, hide the header/wizard and just show + // live progress. + s = m.viewProgress() default: - return m.banner() + m.viewDone() + // StepDone: render nothing so the inline TUI clears itself on exit; the + // command prints the final outcome. + return "" } + // Ensure no rendered line exceeds the terminal width; otherwise a line wraps, + // the inline renderer miscounts its height, and repainting (e.g. on resize) + // leaves duplicated header lines behind. + return clampToWidth(s, m.width) } // banner renders the persistent title and wizard stepper shown at the top of -// every step. +// every step, followed by a single blank line of spacing. func (m Model) banner() string { - return titleStyle.Render("Merge stack") + "\n" + m.stepper() + "\n\n" + title := "Merge stack" + if m.opts.StackNumber > 0 { + title = fmt.Sprintf("Merge stack #%d", m.opts.StackNumber) + } + return titleStyle.Render(title) + "\n" + m.stepper() + "\n\n" +} + +// stepBg returns the background color for the step at index i given the current +// active step: completed steps are green, the active step is the brightest +// (near-white on dark, near-black on light), and upcoming steps are a dim gray. +func stepBg(i, cur int) lipgloss.TerminalColor { + switch { + case i < cur: + return theme.ColorGreen + case i == cur: + return theme.ColorText + default: + return theme.ColorBorder + } +} + +// stepFg returns the foreground color for the step at index i: dark text on the +// bright/green segments, and a dim muted text on the upcoming gray segments. +func stepFg(i, cur int) lipgloss.TerminalColor { + if i > cur { + return theme.ColorTextMuted + } + return theme.ColorOnFill } func (m Model) stepper() string { cur := m.wizardIndex() - parts := make([]string, len(wizardSteps)) + var b strings.Builder + n := len(wizardSteps) for i, label := range wizardSteps { - switch { - case i < cur: - parts[i] = stepDoneStyle.Render("✓ " + label) - case i == cur: - parts[i] = stepActiveStyle.Render(label) - default: - parts[i] = stepUpcomingStyle.Render(label) + bg := stepBg(i, cur) + icon := "•" + if i < cur { + icon = "✓" + } + seg := lipgloss.NewStyle().Background(bg).Foreground(stepFg(i, cur)).Bold(i == cur).Padding(0, 1) + b.WriteString(seg.Render(icon + " " + label)) + + if m.usePowerline { + // Powerline separator: the current background color, over the next + // segment's background (or the terminal default after the last step). + arrow := lipgloss.NewStyle().Foreground(bg) + if i < n-1 { + arrow = arrow.Background(stepBg(i+1, cur)) + } + b.WriteString(arrow.Render(stepArrow)) } + // Fallback: segments abut directly, so their background colors form a + // seamless segmented bar without any Powerline glyph. + } + return b.String() +} + +// powerlineEnabled reports whether the terminal is known to render Powerline +// glyphs (U+E0Bx). Most terminals need a patched/Nerd font, so this defaults to +// off and only opts in for terminals with built-in Powerline glyph support, +// avoiding the missing-glyph box seen in e.g. Apple Terminal. Set +// GH_STACK_POWERLINE=1/0 to override. +func powerlineEnabled() bool { + switch strings.ToLower(os.Getenv("GH_STACK_POWERLINE")) { + case "1", "true", "yes", "on": + return true + case "0", "false", "no", "off": + return false + } + switch os.Getenv("TERM_PROGRAM") { + case "ghostty", "WezTerm": + return true + } + switch os.Getenv("TERM") { + case "xterm-ghostty", "xterm-kitty": + return true } - return strings.Join(parts, stepArrowStyle.Render("▸")) + return os.Getenv("KITTY_WINDOW_ID") != "" } // wizardIndex maps the current step to its position in the stepper. Progress and @@ -86,31 +158,66 @@ func (m Model) wizardIndex() int { func (m Model) viewSelect() string { var b strings.Builder - b.WriteString(mutedStyle.Render("Select how far up the stack to merge (everything up to your choice merges).") + "\n\n") + n := len(m.opts.PRs) + h := m.visibleItems() + start := m.scrollOffset + if start > n-h { + start = n - h + } + if start < 0 { + start = 0 + } + end := start + h + if end > n { + end = n + } + + // Reserve the indicator lines at all times (blank when nothing is hidden) so + // the list doesn't shift as the ↑/↓ hints appear and disappear while scrolling. + if start > 0 { + b.WriteString(faintStyle.Render(fmt.Sprintf(" ↑ %d more", start)) + "\n") + } else { + b.WriteString("\n") + } // Render top of stack first so the layout matches the CLI. - for i := len(m.opts.PRs) - 1; i >= 0; i-- { + for r := start; r < end; r++ { + i := n - 1 - r pr := m.opts.PRs[i] - cursor := " " + selected := i <= m.topIndex + + cursorMark := " " if i == m.cursor { - cursor = accentStyle.Render("❯ ") + cursorMark = accentStyle.Render("❯ ") } - box := "[ ]" - if i <= m.topIndex { + box := mutedStyle.Render("[ ]") + if selected { box = checkedStyle.Render("[x]") } - num := numberStyle.Render(fmt.Sprintf("#%d", pr.Number)) - title := truncate(pr.Title, 60) - titleStyled := mutedStyle.Render(title) - if i <= m.topIndex { - titleStyled = textStyle.Render(title) + // Title: white/black for all, a touch bolder when selected. + titleField := textStyle + // Number + branch: gray for all, fainter when deselected. + metaField := faintStyle + if selected { + titleField = selectedTitleStyle + metaField = mutedStyle + } + title := pr.Title + if title == "" { + title = pr.Branch } - b.WriteString(fmt.Sprintf("%s%s %s %s\n", cursor, box, num, titleStyled)) + b.WriteString(fmt.Sprintf("%s%s %s\n", cursorMark, box, titleField.Render(title))) + b.WriteString(" " + metaField.Render(fmt.Sprintf("#%d • %s", pr.Number, pr.Branch)) + "\n") + } + if end < n { + b.WriteString(faintStyle.Render(fmt.Sprintf(" ↓ %d more", n-end)) + "\n") + } else { + b.WriteString("\n") } b.WriteString("\n") if m.topIndex >= 0 { - b.WriteString(mutedStyle.Render(fmt.Sprintf("Merging %s into %s.", prCount(m.topIndex+1), m.opts.BaseRef))) + b.WriteString(mutedStyle.Render(fmt.Sprintf("Will merge %s into %s.", prCount(m.topIndex+1), m.opts.BaseRef))) } else { b.WriteString(faintStyle.Render("Select at least one pull request.")) } @@ -126,7 +233,6 @@ func (m Model) viewSelect() string { func (m Model) viewMethod() string { var b strings.Builder - b.WriteString(mutedStyle.Render("Choose a merge method.") + "\n\n") for i, method := range m.opts.AllowedMethods { cursor := " " @@ -156,12 +262,17 @@ func (m Model) viewConfirm() string { var b strings.Builder nums := m.selectedNumbers() - b.WriteString(fmt.Sprintf("%s into %s via %s.\n", + b.WriteString(fmt.Sprintf("%s into %s with %s.\n", titleStyle.Render("Merge "+prCount(len(nums))), accentStyle.Render(m.opts.BaseRef), accentStyle.Render(methodLabel(m.method)), )) - b.WriteString(numberStyle.Render(prNumberList(nums)) + "\n\n") + // Wrap the PR list so a long stack isn't cut off at the screen edge. + listStyle := numberStyle + if m.width > 0 { + listStyle = listStyle.Width(m.width) + } + b.WriteString(listStyle.Render(prNumberList(nums)) + "\n\n") b.WriteString(shortcuts( [2]string{"enter", "merge"}, [2]string{"shift+tab", "back"}, @@ -174,45 +285,28 @@ func (m Model) viewProgress() string { var b strings.Builder nums := m.selectedNumbers() - b.WriteString(fmt.Sprintf("%s Merging %s into %s via %s…\n", + b.WriteString(fmt.Sprintf("%s Merging %s into %s via %s\n", m.spinner.View(), numberStyle.Render(prNumberList(nums)), accentStyle.Render(m.opts.BaseRef), accentStyle.Render(methodLabel(m.method)), )) - if m.message != "" { - b.WriteString(faintStyle.Render(m.message) + "\n") - } + // Always render a status line so it doesn't pop in later and shift the view. + b.WriteString(faintStyle.Render(progressStatus(m.message)) + "\n") b.WriteString("\n") b.WriteString(faintStyle.Render("ctrl+c: stop watching (the merge keeps running on GitHub)")) return b.String() } -func (m Model) viewDone() string { - var b strings.Builder - nums := m.selectedNumbers() - - switch { - case m.merged: - b.WriteString(successStyle.Render("✓ Merged") + " ") - b.WriteString(fmt.Sprintf("%s into %s.\n", numberStyle.Render(prNumberList(nums)), m.opts.BaseRef)) - if m.status.SHA != "" { - b.WriteString(faintStyle.Render("Merge commit "+shortSHA(m.status.SHA)) + "\n") - } - case m.failed: - b.WriteString(failureStyle.Render("✗ Merge failed") + "\n") - if m.message != "" { - b.WriteString(mutedStyle.Render(m.message) + "\n") - } - b.WriteString(faintStyle.Render("The stack is atomic, so nothing was merged.") + "\n") - case m.cancelled: - b.WriteString(mutedStyle.Render("Merge cancelled.") + "\n") - default: - if m.message != "" { - b.WriteString(mutedStyle.Render(m.message) + "\n") - } +// progressStatus normalizes an async-merge status message for display: a blank +// message shows an initial "Submitting…" line, and messages end in an ellipsis +// rather than a period. +func progressStatus(msg string) string { + msg = strings.TrimSpace(msg) + if msg == "" { + return "Submitting merge request..." } - return b.String() + return strings.TrimRight(msg, ". ") + "..." } func shortcuts(entries ...[2]string) string { @@ -253,19 +347,51 @@ func prNumberList(nums []int) string { return strings.Join(parts, ", ") } -func shortSHA(sha string) string { - if len(sha) > 7 { - return sha[:7] +// clampToWidth truncates every line of s to at most width cells so nothing +// wraps. +func clampToWidth(s string, width int) string { + if width <= 0 { + return s + } + lines := strings.Split(s, "\n") + for i, ln := range lines { + if lipgloss.Width(ln) > width { + lines[i] = truncate(ln, width) + } } - return sha + return strings.Join(lines, "\n") } -func truncate(s string, max int) string { - if len(s) <= max { +// truncate shortens s to at most width display cells, appending an ellipsis and +// resetting styling. It skips ANSI escape sequences when counting width. +func truncate(s string, width int) string { + if width <= 0 { + return "" + } + if lipgloss.Width(s) <= width { return s } - if max <= 1 { - return s[:max] + var b strings.Builder + w := 0 + inEscape := false + for _, r := range s { + if r == '\x1b' { + inEscape = true + } + if inEscape { + b.WriteRune(r) + if r == 'm' { + inEscape = false + } + continue + } + if w >= width-1 { + b.WriteString("…") + b.WriteString("\x1b[0m") + break + } + b.WriteRune(r) + w++ } - return s[:max-1] + "…" + return b.String() } From 2375388c44c0ea72ef47b7be1eb47b54b15c72f9 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 07:08:27 -0400 Subject: [PATCH 03/11] warn merge queue unsupported --- cmd/merge.go | 21 +++++++++++ cmd/merge_test.go | 24 +++++++++++++ internal/github/client_interface.go | 1 + internal/github/merge_async.go | 48 +++++++++++++++++++++++++ internal/github/merge_async_test.go | 55 +++++++++++++++++++++++++++++ internal/github/mock_client.go | 8 +++++ 6 files changed, 157 insertions(+) diff --git a/cmd/merge.go b/cmd/merge.go index 230ac3a5..7e01749e 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -140,6 +140,18 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { base := remoteStack.Base.Ref + // Merge-queue and admin-bypass policy for the stack's base branch. The async + // stack merge cannot use a merge queue, so bail out early (before any + // prompting) when the base requires one. + policy, err := client.BaseBranchPolicy(base) + if err != nil { + cfg.Errorf("failed to check base branch merge settings: %s", err) + return ErrAPIFailure + } + if policy.RequiresMergeQueue { + return explainMergeQueueUnsupported(cfg, base) + } + if cfg.IsInteractive() && !opts.yes { return runMergeInteractive(cfg, client, remoteStack.Number, base, candidates, allowed, mergeCfg.DefaultMethod, method, preselectIndex, opts) } @@ -476,6 +488,15 @@ func warnAsyncMergeUnavailable(cfg *config.Config) { cfg.Warningf("Async stack merge is not available for this repository") } +// explainMergeQueueUnsupported reports that the stack's base branch merges +// through a merge queue, which the async stack merge cannot use, and points the +// user to the web UI. +func explainMergeQueueUnsupported(cfg *config.Config, base string) error { + cfg.Errorf("the base branch %q requires a merge queue, which \"gh stack merge\" does not support", base) + cfg.Printf("Merge this stack using `%q` or from the GitHub web UI instead.", "gh pr merge") + return ErrSilent +} + // mergeFailureExit maps a merge failure message to an exit code: rebase/merge // conflicts get ErrConflict, everything else ErrAPIFailure. func mergeFailureExit(message string) error { diff --git a/cmd/merge_test.go b/cmd/merge_test.go index 1e79245f..b6af7e88 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -347,6 +347,30 @@ func TestRunMerge_AlreadyMergedOnSubmit(t *testing.T) { assert.Contains(t, output, "Merged #1, #2 into main") } +func TestRunMerge_MergeQueueRequired(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil + }, + BaseBranchPolicyFn: func(base string) (*github.BaseBranchPolicy, error) { + assert.Equal(t, "main", base) + return &github.BaseBranchPolicy{RequiresMergeQueue: true}, nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + t.Fatal("merge must not be attempted when the base requires a merge queue") + return nil, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "merge queue") + assert.Contains(t, output, "web UI") +} + func TestRunMerge_AsyncMergeUnavailable(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index dd3bfe0c..632b4555 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -21,6 +21,7 @@ type ClientOps interface { MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) PRTitles(numbers []int) (map[int]string, error) + BaseBranchPolicy(baseRef string) (*BaseBranchPolicy, error) } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go index 34e29726..fa31f752 100644 --- a/internal/github/merge_async.go +++ b/internal/github/merge_async.go @@ -121,6 +121,54 @@ func (c *Client) RepoMergeConfig() (*RepoMergeConfig, error) { }, nil } +// BaseBranchPolicy describes merge-relevant policy on a stack's base branch. +type BaseBranchPolicy struct { + // RequiresMergeQueue reports that the base branch merges through a merge + // queue, so a direct async stack merge is not possible. + RequiresMergeQueue bool +} + +// BaseBranchPolicy reports whether the given base branch requires a merge queue, +// which the async stack merge cannot use. +func (c *Client) BaseBranchPolicy(baseRef string) (*BaseBranchPolicy, error) { + var query struct { + Repository struct { + MergeQueue *struct { + ID string `graphql:"id"` + } `graphql:"mergeQueue(branch: $branch)"` + Ref *struct { + Rules struct { + Nodes []struct { + Type string `graphql:"type"` + } `graphql:"nodes"` + } `graphql:"rules(first: 50)"` + } `graphql:"ref(qualifiedName: $qualified)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": graphql.String(c.owner), + "name": graphql.String(c.repo), + "branch": graphql.String(baseRef), + "qualified": graphql.String("refs/heads/" + baseRef), + } + + if err := c.gql.Query("BaseBranchPolicy", &query, variables); err != nil { + return nil, fmt.Errorf("querying base branch policy: %w", err) + } + + r := query.Repository + policy := &BaseBranchPolicy{RequiresMergeQueue: r.MergeQueue != nil} + if r.Ref != nil { + for _, node := range r.Ref.Rules.Nodes { + if node.Type == "MERGE_QUEUE" { + policy.RequiresMergeQueue = true + } + } + } + return policy, nil +} + // MergeStackAsync requests an asynchronous merge of the given pull request. For // a stacked PR this merges all members of the stack up to and including // prNumber. A blank method lets the server apply its default. diff --git a/internal/github/merge_async_test.go b/internal/github/merge_async_test.go index cb1b2902..72de6a3b 100644 --- a/internal/github/merge_async_test.go +++ b/internal/github/merge_async_test.go @@ -169,3 +169,58 @@ func TestMergeStackAsync_OmitsEmptyMethod(t *testing.T) { _, hasMethod := parsed["merge_method"] assert.False(t, hasMethod, "merge_method should be omitted when empty") } + +// testPolicyClient builds a Client whose GraphQL client is backed by a stub +// transport returning the given response body. +func testPolicyClient(t *testing.T, graphqlResp string) *Client { + t.Helper() + rt := roundTripFunc(func(r *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(graphqlResp)), + Header: http.Header{"Content-Type": []string{"application/json"}}, + Request: r, + }, nil + }) + gql, err := api.NewGraphQLClient(api.ClientOptions{Host: "github.com", AuthToken: "x", Transport: rt}) + require.NoError(t, err) + return &Client{gql: gql, owner: "o", repo: "r"} +} + +func TestBaseBranchPolicy(t *testing.T) { + tests := []struct { + name string + body string + wantQueue bool + }{ + { + name: "no merge queue", + body: `{"data":{"repository":{"mergeQueue":null,"ref":{"rules":{"nodes":[]}}}}}`, + }, + { + name: "merge queue via mergeQueue field", + body: `{"data":{"repository":{"mergeQueue":{"id":"MQ"},"ref":{"rules":{"nodes":[]}}}}}`, + wantQueue: true, + }, + { + name: "merge queue via ruleset type", + body: `{"data":{"repository":{"mergeQueue":null,"ref":{"rules":{"nodes":[{"type":"MERGE_QUEUE"}]}}}}}`, + wantQueue: true, + }, + { + name: "other rules, no merge queue", + body: `{"data":{"repository":{"mergeQueue":null,"ref":{"rules":{"nodes":[{"type":"PULL_REQUEST"}]}}}}}`, + }, + { + name: "null ref", + body: `{"data":{"repository":{"mergeQueue":null,"ref":null}}}`, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + policy, err := testPolicyClient(t, tt.body).BaseBranchPolicy("main") + require.NoError(t, err) + assert.Equal(t, tt.wantQueue, policy.RequiresMergeQueue, "RequiresMergeQueue") + }) + } +} diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 64b50681..0d7ded48 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -21,6 +21,7 @@ type MockClient struct { MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) PRTitlesFn func([]int) (map[int]string, error) + BaseBranchPolicyFn func(string) (*BaseBranchPolicy, error) } // Compile-time check that MockClient satisfies ClientOps. @@ -162,3 +163,10 @@ func (m *MockClient) PRTitles(numbers []int) (map[int]string, error) { } return map[int]string{}, nil } + +func (m *MockClient) BaseBranchPolicy(baseRef string) (*BaseBranchPolicy, error) { + if m.BaseBranchPolicyFn != nil { + return m.BaseBranchPolicyFn(baseRef) + } + return &BaseBranchPolicy{}, nil +} From c3ca38587dacf2e18f361d191d8bb282e7e79084 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 07:56:13 -0400 Subject: [PATCH 04/11] update for new status field from api --- cmd/merge.go | 25 ++++++++++---- cmd/merge_test.go | 18 +++++----- internal/github/merge_async.go | 37 ++++++++++++++------- internal/github/merge_async_test.go | 49 ++++++++++++++++------------ internal/github/mock_client.go | 4 +-- internal/tui/mergeview/model.go | 27 ++++++++------- internal/tui/mergeview/model_test.go | 20 ++++++------ internal/tui/mergeview/types.go | 20 +++++++++--- 8 files changed, 125 insertions(+), 75 deletions(-) diff --git a/cmd/merge.go b/cmd/merge.go index 7e01749e..1b257571 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -325,7 +325,7 @@ func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumbe return nil case out.Failed: cfg.Errorf("merge failed: %s", out.Message) - cfg.Printf("The stack is atomic, so nothing was merged.") + cfg.Printf("Stack merges are atomic, so nothing was merged.") return mergeFailureExit(out.Message) case out.WatchStopped: cfg.Infof("Stopped watching. Merge is still in progress. Check the pull requests on GitHub.") @@ -353,10 +353,15 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, return ErrAPIFailure } - if res.Merged { + if res.IsMerged() { mergedSuccess(cfg, list, base, res.Details.SHA) return nil } + if res.IsFailed() { + cfg.Errorf("merge failed: %s", res.Details.Message) + cfg.Printf("Stack merges are atomic, so nothing was merged.") + return mergeFailureExit(res.Details.Message) + } uuid := res.Details.UUID if uuid == "" { @@ -380,13 +385,13 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, cfg.Errorf("failed to check merge status: %s", err) return ErrAPIFailure } - if status.Merged { + if status.IsMerged() { mergedSuccess(cfg, list, base, status.Details.SHA) return nil } - if !status.Queued { + if status.IsFailed() { cfg.Errorf("merge failed: %s", status.Details.Message) - cfg.Printf("The stack is atomic, so nothing was merged.") + cfg.Printf("Stack merges are atomic, so nothing was merged.") return mergeFailureExit(status.Details.Message) } } @@ -416,9 +421,15 @@ func mergeFuncs(client github.ClientOps) (mergeview.SubmitFunc, mergeview.PollFu } func toMergeStatus(res *github.AsyncMergeResult) mergeview.MergeStatus { + status := mergeview.StatusPending + switch { + case res.IsMerged(): + status = mergeview.StatusMerged + case res.IsFailed(): + status = mergeview.StatusFailed + } return mergeview.MergeStatus{ - Queued: res.Queued, - Merged: res.Merged, + Status: status, Message: res.Details.Message, UUID: res.Details.UUID, SHA: res.Details.SHA, diff --git a/cmd/merge_test.go b/cmd/merge_test.go index b6af7e88..8284d7cb 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -93,10 +93,10 @@ func TestRunMerge_NoArg_MergesWholeStack(t *testing.T) { }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { gotPR, gotMethod = pr, method - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc1234"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusMerged, Details: github.AsyncMergeDetails{SHA: "abc1234"}}, nil }, } @@ -119,7 +119,7 @@ func TestRunMerge_StackNumberArg(t *testing.T) { }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { gotPR = pr - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, } @@ -144,7 +144,7 @@ func TestRunMerge_PRNumberArg(t *testing.T) { }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { gotPR = pr - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, } @@ -166,7 +166,7 @@ func TestRunMerge_SquashFlag(t *testing.T) { }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { gotMethod = method - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, } @@ -314,10 +314,10 @@ func TestRunMerge_PollFailedConflict(t *testing.T) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Queued: false, Merged: false, Details: github.AsyncMergeDetails{Message: "Merge conflict: could not merge."}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusFailed, Details: github.AsyncMergeDetails{Message: "Merge conflict: could not merge."}}, nil }, } @@ -336,7 +336,7 @@ func TestRunMerge_AlreadyMergedOnSubmit(t *testing.T) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - return &github.AsyncMergeResult{Merged: true, Details: github.AsyncMergeDetails{SHA: "abc"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusMerged, Details: github.AsyncMergeDetails{SHA: "abc"}}, nil }, } @@ -428,7 +428,7 @@ func TestRunMerge_DefaultMethodFallsBackToAllowed(t *testing.T) { }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { gotMethod = method - return &github.AsyncMergeResult{Queued: true, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, } diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go index fa31f752..3928c1a7 100644 --- a/internal/github/merge_async.go +++ b/internal/github/merge_async.go @@ -65,9 +65,9 @@ func (c RepoMergeConfig) Allows(method string) bool { } // AsyncMergeDetails is the polymorphic "details" object shared by the submit and -// poll responses. Fields are populated based on the current state: a queued -// request carries UUID/MergeMethod/ExpectedHeadSHA, an already-merged result -// carries SHA, and a failed/not-mergeable result carries only Message. +// poll responses. Fields are populated based on the current state: a pending +// request carries UUID/MergeMethod/ExpectedHeadSHA, a merged result carries SHA, +// and a failed/not-mergeable result carries only Message. type AsyncMergeDetails struct { Message string `json:"message"` UUID string `json:"uuid"` @@ -77,18 +77,33 @@ type AsyncMergeDetails struct { } // AsyncMergeResult is the response body returned by both the submit and poll -// async merge endpoints. The Queued/Merged flags distinguish an enqueued merge -// (202) from an already-merged pull request (200). +// async merge endpoints. Status is one of the AsyncMergeStatus* values: +// "pending" (running in the background), "merged" (completed), or "failed". type AsyncMergeResult struct { - Queued bool `json:"queued"` - Merged bool `json:"merged"` + Status string `json:"status"` Details AsyncMergeDetails `json:"details"` } -// InProgress reports whether the merge is still queued (running in the -// background). -func (r *AsyncMergeResult) InProgress() bool { - return r != nil && r.Queued && !r.Merged +// Async merge status values returned in the response's "status" field. +const ( + AsyncMergeStatusPending = "pending" + AsyncMergeStatusMerged = "merged" + AsyncMergeStatusFailed = "failed" +) + +// IsMerged reports whether the merge completed successfully. +func (r *AsyncMergeResult) IsMerged() bool { + return r != nil && r.Status == AsyncMergeStatusMerged +} + +// IsFailed reports whether the merge was attempted but did not complete. +func (r *AsyncMergeResult) IsFailed() bool { + return r != nil && r.Status == AsyncMergeStatusFailed +} + +// IsPending reports whether the merge is still running in the background. +func (r *AsyncMergeResult) IsPending() bool { + return r != nil && r.Status == AsyncMergeStatusPending } // RepoMergeConfig fetches the repository's allowed merge methods and the diff --git a/internal/github/merge_async_test.go b/internal/github/merge_async_test.go index 72de6a3b..d01e6fec 100644 --- a/internal/github/merge_async_test.go +++ b/internal/github/merge_async_test.go @@ -50,7 +50,7 @@ func testAsyncClient(t *testing.T, status int, respBody string, rec *recordedReq func TestMergeStackAsync_Accepted(t *testing.T) { var rec recordedRequest - body := `{"queued":true,"merged":false,"details":{"message":"Merge request enqueued.","uuid":"u-1","merge_method":"squash","expected_head_sha":"abc"}}` + body := `{"status":"pending","details":{"message":"Merge request enqueued.","uuid":"u-1","merge_method":"squash","expected_head_sha":"abc"}}` c := testAsyncClient(t, http.StatusAccepted, body, &rec) res, err := c.MergeStackAsync(42, "squash") @@ -60,25 +60,24 @@ func TestMergeStackAsync_Accepted(t *testing.T) { assert.Equal(t, "/repos/o/r/pulls/42/merge-async", rec.path) assert.JSONEq(t, `{"merge_method":"squash"}`, rec.body) - assert.True(t, res.Queued) - assert.False(t, res.Merged) + assert.True(t, res.IsPending()) + assert.False(t, res.IsMerged()) assert.Equal(t, "u-1", res.Details.UUID) assert.Equal(t, "squash", res.Details.MergeMethod) - assert.True(t, res.InProgress()) } func TestMergeStackAsync_AlreadyMerged(t *testing.T) { - body := `{"queued":false,"merged":true,"details":{"message":"Pull request is already merged.","sha":"deadbeef"}}` + body := `{"status":"merged","details":{"message":"Pull request is already merged.","sha":"deadbeef"}}` res, err := testAsyncClient(t, http.StatusOK, body, nil).MergeStackAsync(42, "merge") require.NoError(t, err) - assert.True(t, res.Merged) + assert.True(t, res.IsMerged()) assert.Equal(t, "deadbeef", res.Details.SHA) } func TestMergeStackAsync_ExistingRequestConflict(t *testing.T) { // The go-gh REST client discards the 409 body, so we can't recover the // existing UUID; the request surfaces as a clear "already exists" error. - _, err := testAsyncClient(t, http.StatusConflict, `{"queued":true,"merged":false,"details":{"uuid":"u-2"}}`, nil).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusConflict, `{"status":"pending","details":{"uuid":"u-2"}}`, nil).MergeStackAsync(42, "merge") require.Error(t, err) assert.Contains(t, err.Error(), "already exists") } @@ -86,7 +85,7 @@ func TestMergeStackAsync_ExistingRequestConflict(t *testing.T) { func TestMergeStackAsync_NotMergeable(t *testing.T) { // A 400 preflight failure is reported as a clear error (the specific // details.message isn't recoverable through the REST client). - _, err := testAsyncClient(t, http.StatusBadRequest, `{"queued":false,"merged":false,"details":{"message":"Pull request is closed."}}`, nil).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusBadRequest, `{"status":"failed","details":{"message":"Pull request is closed."}}`, nil).MergeStackAsync(42, "merge") require.Error(t, err) assert.Contains(t, err.Error(), "can no longer be merged") } @@ -106,12 +105,11 @@ func TestGetAsyncMergeResult_States(t *testing.T) { tests := []struct { name string body string - wantQueued bool - wantMerged bool + wantStatus string }{ - {"pending", `{"queued":true,"merged":false,"details":{"message":"Merge request is in progress.","uuid":"u","merge_method":"merge","expected_head_sha":"abc"}}`, true, false}, - {"merged", `{"queued":false,"merged":true,"details":{"message":"Pull request was merged.","sha":"abc"}}`, false, true}, - {"failed", `{"queued":false,"merged":false,"details":{"message":"Merge conflict."}}`, false, false}, + {"pending", `{"status":"pending","details":{"message":"Merge request is in progress.","uuid":"u","merge_method":"merge","expected_head_sha":"abc"}}`, AsyncMergeStatusPending}, + {"merged", `{"status":"merged","details":{"message":"Pull request was merged.","sha":"abc"}}`, AsyncMergeStatusMerged}, + {"failed", `{"status":"failed","details":{"message":"Merge conflict."}}`, AsyncMergeStatusFailed}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -120,8 +118,7 @@ func TestGetAsyncMergeResult_States(t *testing.T) { require.NoError(t, err) assert.Equal(t, http.MethodGet, rec.method) assert.Equal(t, "/repos/o/r/pulls/42/merge-async/u", rec.path) - assert.Equal(t, tt.wantQueued, res.Queued) - assert.Equal(t, tt.wantMerged, res.Merged) + assert.Equal(t, tt.wantStatus, res.Status) }) } } @@ -150,12 +147,24 @@ func TestRepoMergeConfig_AllowedMethods(t *testing.T) { assert.Empty(t, empty.AllowedMethods()) } -func TestAsyncMergeResult_InProgress(t *testing.T) { - assert.True(t, (&AsyncMergeResult{Queued: true}).InProgress()) - assert.False(t, (&AsyncMergeResult{Queued: true, Merged: true}).InProgress()) - assert.False(t, (&AsyncMergeResult{}).InProgress()) +func TestAsyncMergeResult_Status(t *testing.T) { + pending := &AsyncMergeResult{Status: AsyncMergeStatusPending} + assert.True(t, pending.IsPending()) + assert.False(t, pending.IsMerged()) + assert.False(t, pending.IsFailed()) + + merged := &AsyncMergeResult{Status: AsyncMergeStatusMerged} + assert.True(t, merged.IsMerged()) + assert.False(t, merged.IsPending()) + + failed := &AsyncMergeResult{Status: AsyncMergeStatusFailed} + assert.True(t, failed.IsFailed()) + assert.False(t, failed.IsMerged()) + var nilRes *AsyncMergeResult - assert.False(t, nilRes.InProgress()) + assert.False(t, nilRes.IsPending()) + assert.False(t, nilRes.IsMerged()) + assert.False(t, nilRes.IsFailed()) } // sanity check that the submit body omits merge_method when empty. diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 0d7ded48..a23ed8b1 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -135,7 +135,7 @@ func (m *MockClient) MergeStackAsync(prNumber int, method string) (*AsyncMergeRe return m.MergeStackAsyncFn(prNumber, method) } return &AsyncMergeResult{ - Queued: true, + Status: AsyncMergeStatusPending, Details: AsyncMergeDetails{ Message: "Merge request enqueued.", UUID: "mock-uuid", @@ -149,7 +149,7 @@ func (m *MockClient) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMerge return m.GetAsyncMergeResultFn(prNumber, uuid) } return &AsyncMergeResult{ - Merged: true, + Status: AsyncMergeStatusMerged, Details: AsyncMergeDetails{ Message: "Pull request was merged.", SHA: "mockmergesha", diff --git a/internal/tui/mergeview/model.go b/internal/tui/mergeview/model.go index b307d688..e15c030b 100644 --- a/internal/tui/mergeview/model.go +++ b/internal/tui/mergeview/model.go @@ -262,15 +262,19 @@ func (m Model) handleSubmitDone(msg submitDoneMsg) (tea.Model, tea.Cmd) { m.status = msg.status m.message = msg.status.Message - switch { - case msg.status.Merged: + switch msg.status.Status { + case StatusMerged: m.merged = true return m.finish() - case msg.status.Queued && msg.status.UUID != "": - // Enqueued (or an existing request adopted): start polling. - return m, m.pollTickCmd() + case StatusFailed: + m.failed = true + return m.finish() default: - // Not queued and not merged: the PR could not be merged (e.g. 400). + // Pending (enqueued, or an existing request adopted): poll if we have a + // UUID; otherwise this is unexpected, so treat it as a failure. + if msg.status.UUID != "" { + return m, m.pollTickCmd() + } m.failed = true return m.finish() } @@ -287,15 +291,16 @@ func (m Model) handlePollDone(msg pollDoneMsg) (tea.Model, tea.Cmd) { m.status = msg.status m.message = msg.status.Message - switch { - case msg.status.Merged: + switch msg.status.Status { + case StatusMerged: m.merged = true return m.finish() - case msg.status.Queued: - return m, m.pollTickCmd() - default: + case StatusFailed: m.failed = true return m.finish() + default: + // Still pending: keep polling. + return m, m.pollTickCmd() } } diff --git a/internal/tui/mergeview/model_test.go b/internal/tui/mergeview/model_test.go index dd93b6b7..a1ae9bda 100644 --- a/internal/tui/mergeview/model_test.go +++ b/internal/tui/mergeview/model_test.go @@ -198,35 +198,35 @@ func TestConfirm_SubmitAlreadyMerged(t *testing.T) { m := New(baseOptions()) // submitDoneMsg is handled regardless of step; simulate an already-merged // response. - m = step(m, submitDoneMsg{status: MergeStatus{Merged: true, Message: "Pull request is already merged.", SHA: "abc1234"}}) + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusMerged, Message: "Pull request is already merged.", SHA: "abc1234"}}) out := m.Outcome() assert.True(t, out.Merged) assert.False(t, out.Failed) assert.Equal(t, []int{1, 2, 3}, out.MergedPRs) } -func TestProgress_QueuedThenFailed(t *testing.T) { +func TestProgress_PendingThenFailed(t *testing.T) { m := New(baseOptions()) m.step = StepProgress m.submitted = true - m = step(m, submitDoneMsg{status: MergeStatus{Queued: true, UUID: "u1", Message: "enqueued"}}) + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusPending, UUID: "u1", Message: "enqueued"}}) assert.False(t, m.done(), "still in progress after queued submit") - m = step(m, pollDoneMsg{status: MergeStatus{Queued: false, Merged: false, Message: "Merge conflict."}}) + m = step(m, pollDoneMsg{status: MergeStatus{Status: StatusFailed, Message: "Merge conflict."}}) out := m.Outcome() assert.True(t, out.Failed) assert.False(t, out.Merged) assert.Equal(t, "Merge conflict.", out.Message) } -func TestProgress_QueuedThenMerged(t *testing.T) { +func TestProgress_PendingThenMerged(t *testing.T) { m := New(baseOptions()) m.step = StepProgress m.submitted = true - m = step(m, submitDoneMsg{status: MergeStatus{Queued: true, UUID: "u1"}}) - m = step(m, pollDoneMsg{status: MergeStatus{Merged: true, SHA: "deadbee"}}) + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusPending, UUID: "u1"}}) + m = step(m, pollDoneMsg{status: MergeStatus{Status: StatusMerged, SHA: "deadbee"}}) out := m.Outcome() assert.True(t, out.Merged) assert.Equal(t, []int{1, 2, 3}, out.MergedPRs) @@ -236,7 +236,7 @@ func TestSubmit_NotMergeable(t *testing.T) { m := New(baseOptions()) m.step = StepProgress m.submitted = true - m = step(m, submitDoneMsg{status: MergeStatus{Queued: false, Merged: false, Message: "Pull request is closed."}}) + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusFailed, Message: "Pull request is closed."}}) out := m.Outcome() assert.True(t, out.Failed) assert.Equal(t, "Pull request is closed.", out.Message) @@ -270,7 +270,7 @@ func TestProgress_WatchStopped(t *testing.T) { m := New(baseOptions()) m.step = StepProgress m.submitted = true - m = step(m, submitDoneMsg{status: MergeStatus{Queued: true, UUID: "u"}}) // in-flight + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusPending, UUID: "u"}}) // in-flight m = step(m, keyType(tea.KeyCtrlC)) out := m.Outcome() assert.True(t, out.WatchStopped) @@ -281,7 +281,7 @@ func TestProgress_WatchStopped(t *testing.T) { func TestOutcome_SHA(t *testing.T) { m := New(baseOptions()) - m = step(m, submitDoneMsg{status: MergeStatus{Merged: true, SHA: "deadbeef"}}) + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusMerged, SHA: "deadbeef"}}) assert.Equal(t, "deadbeef", m.Outcome().SHA) } diff --git a/internal/tui/mergeview/types.go b/internal/tui/mergeview/types.go index 31af3476..f0f329c2 100644 --- a/internal/tui/mergeview/types.go +++ b/internal/tui/mergeview/types.go @@ -38,13 +38,23 @@ type PRItem struct { Branch string } -// MergeStatus is the minimal async-merge status the progress view consumes, +// Status is the async-merge state, mirroring the API's `status` field. +type Status string + +const ( + // StatusPending means the merge is still running in the background. + StatusPending Status = "pending" + // StatusMerged means the merge completed successfully. + StatusMerged Status = "merged" + // StatusFailed means the merge was attempted but did not complete. + StatusFailed Status = "failed" +) + +// MergeStatus is the minimal async-merge result the progress view consumes, // mapped by the caller from the API response. type MergeStatus struct { - // Queued reports the merge is still running in the background. - Queued bool - // Merged reports the merge completed successfully. - Merged bool + // Status is the current merge state. + Status Status // Message is the human-readable status or failure reason. Message string // UUID identifies an in-flight merge request, used for polling. From 049e0b8d996910ea40d6bcfce18fb21be37d00cf Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 18:18:19 -0400 Subject: [PATCH 05/11] merge cmd docs --- .github/copilot-instructions.md | 2 +- AGENTS.md | 6 ++-- README.md | 40 +++++++++++++++++++++ cmd/merge.go | 7 ++-- docs/src/content/docs/guides/workflows.md | 31 ++++++++++++++++- docs/src/content/docs/reference/cli.md | 42 +++++++++++++++++++++++ skills/gh-stack/SKILL.md | 7 +++- 7 files changed, 127 insertions(+), 8 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d564e185..ec387e50 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,7 +17,7 @@ No Makefile, no code generation, no external linter config. Standard Go toolchai - `cmd/`: One Cobra command per file. Each exports `Cmd(cfg *config.Config)` with logic in `run()`. - `internal/git/`: `Ops` interface (52 methods) wrapping git CLI. `MockOps` for tests. Package-level functions delegate to swappable `ops` variable. -- `internal/github/`: `ClientOps` interface (13 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`). +- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`), with `BaseBranchPolicy` (GraphQL) gating merge-queue branches. - `internal/config/`: `Config` struct passed to all commands. Holds I/O, colors, and test hooks (`SelectFn`, `ConfirmFn`, `InputFn`, `GitHubClientOverride`). - `internal/stack/`: Stack file (`.git/gh-stack`, JSON) management with file locking. - `internal/tui/`: bubbletea views (`stackview`, `modifyview`). diff --git a/AGENTS.md b/AGENTS.md index 98875637..545fea46 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,7 @@ internal/ gitops.go # Ops interface (52 methods) mock_ops.go # MockOps. Each method has a corresponding *Fn field. github/ # github.ClientOps interface + real Client - client_interface.go # ClientOps interface (13 methods) + client_interface.go # ClientOps interface (18 methods) mock_client.go # MockClient. Uses function-pointer fields for testing. stack/ # stack file (.git/gh-stack) management, JSON schema, locking schema.json # JSON Schema for the stack file format @@ -57,7 +57,7 @@ skills/ # AI agent skill definition (SKILL.md) | Group | Commands | |-------|----------| | Stack management | `init`, `add`, `view`, `checkout`, `modify`, `unstack` | -| Remote operations | `submit`, `sync`, `rebase`, `push`, `link` | +| Remote operations | `submit`, `sync`, `rebase`, `push`, `link`, `merge` | | Navigation | `switch`, `up`, `down`, `top`, `bottom`, `trunk` | | Utilities | `alias`, `feedback` | @@ -109,7 +109,7 @@ if errors.As(err, &exitErr) { ... } ### Key interfaces - **`git.Ops`** (`internal/git/gitops.go`): 52 methods wrapping git CLI calls. The production implementation uses `cli/go-gh`'s `client.Command()` via `run()` and `runSilent()` helpers. Package-level functions (e.g., `git.CurrentBranch()`) delegate to a swappable package-level `ops` variable. -- **`github.ClientOps`** (`internal/github/client_interface.go`): 13 methods for GitHub API (PRs, stacks). Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`): `ListStacks`, `FindStackForPR`, `GetStack`, `CreateStack`, `AddToStack` (delta append), `Unstack`. Injected via `cfg.GitHubClientOverride` in tests. +- **`github.ClientOps`** (`internal/github/client_interface.go`): 18 methods for GitHub API (PRs, stacks, merges). Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`): `ListStacks`, `FindStackForPR`, `GetStack`, `CreateStack`, `AddToStack` (delta append), `Unstack`. Async stack merges use `RepoMergeConfig` (GraphQL allowed methods + viewer default), `MergeStackAsync`, and `GetAsyncMergeResult` (`/repos/{owner}/{repo}/pulls/{n}/merge-async`); `BaseBranchPolicy` (GraphQL) reports whether the base branch requires a merge queue (unsupported). Injected via `cfg.GitHubClientOverride` in tests. - **`config.Config`** (`internal/config/config.go`): Central configuration passed to all commands. Holds I/O streams, color functions, and test hook fields (`SelectFn`, `ConfirmFn`, `InputFn`, `RepoOverride`). ### Stack file diff --git a/README.md b/README.md index 26267269..a8063424 100644 --- a/README.md +++ b/README.md @@ -439,6 +439,46 @@ gh stack link 42 43 feature-auth feature-ui gh stack link --base develop --open feat-a feat-b feat-c ``` +### `gh stack merge` + +Merge one or multiple stacked PRs at once. + +``` +gh stack merge [ | ] +``` + +All members of the stack up to and including your chosen pull request are merged into the base branch in a single, all-or-nothing operation: if any PR can't be merged, none are. + +With no argument, the current active local stack is used. Pass a stack number to merge a stack you don't have checked out (a purely remote operation), or a pull request number to merge directly up to that PR. + +In an interactive terminal, a short wizard walks you through three steps — choose which PRs to merge, pick the merge method, and confirm. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. + +Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Admin bypass is not supported** for stacked PR merges at this time. + +Note that this command does not work with merge queues. If the stack's base branch uses a merge queue, use `gh pr merge` instead. + +| Flag | Description | +|------|-------------| +| `--merge-method ` | Merge method to use: `merge`, `squash`, or `rebase` | +| `--merge` / `--squash` / `--rebase` | Shorthands for the corresponding merge method | +| `-y, --yes` | Merge without prompting for confirmation | + +**Examples:** + +```sh +# Merge the current stack (interactive picker) +gh stack merge + +# Merge a stack you don't have checked out, by stack number +gh stack merge 7 + +# Merge everything up to and including PR #42 +gh stack merge 42 + +# Merge the whole current stack without prompting, squashing +gh stack merge --yes --squash +``` + ### `gh stack view` View the current stack. diff --git a/cmd/merge.go b/cmd/merge.go index 1b257571..726edc35 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -63,7 +63,10 @@ prompting, using your last-used merge method unless one is specified. Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so -any such failure is reported back to you.`, +any such failure is reported back to you. + +If the base branch uses a merge queue, this command isn't supported (it merges +directly, not through the queue); use "gh pr merge" or the web UI instead.`, Example: ` # Merge the current stack (interactive picker) $ gh stack merge @@ -501,7 +504,7 @@ func warnAsyncMergeUnavailable(cfg *config.Config) { // explainMergeQueueUnsupported reports that the stack's base branch merges // through a merge queue, which the async stack merge cannot use, and points the -// user to the web UI. +// user to the merge queue (via `gh pr merge` or the web UI) instead. func explainMergeQueueUnsupported(cfg *config.Config, base string) error { cfg.Errorf("the base branch %q requires a merge queue, which \"gh stack merge\" does not support", base) cfg.Printf("Merge this stack using `%q` or from the GitHub web UI instead.", "gh pr merge") diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index 1ad0d6e7..f9c867d5 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -33,7 +33,10 @@ gh stack rebase # 7. Push the updated branches gh stack push -# 8. Sync upstream changes as PRs get merged +# 8. Land the stack once it's approved (merges bottom to top, atomically) +gh stack merge + +# 9. Sync upstream changes as PRs get merged gh stack sync ``` @@ -120,6 +123,32 @@ gh stack push The rebase ensures all branches above the changed one pick up the fixes. `gh stack push` uses `--force-with-lease` to safely update the rebased branches. +## Merging Your Stack + +When your stack is approved, land it with `gh stack merge`. Regular `gh pr merge` doesn't work with stacked PRs — `gh stack merge` uses GitHub's atomic stack merge, which merges every PR up to and including your chosen one in a single, all-or-nothing operation. If any PR can't be merged, none are. + +```sh +# Merge the current stack (interactive picker for how far up to merge) +gh stack merge + +# Merge everything up to and including a specific PR +gh stack merge 42 + +# Merge a stack you don't have checked out, by its stack number +gh stack merge 7 + +# Merge without prompting for confirmation, specifying the merge method +gh stack merge --yes --squash +``` + +In an interactive terminal, a short wizard lets you choose how far up the stack to merge, pick the merge method (only the ones your repository allows, defaulting to your last-used method), and confirm — then shows live progress. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting. After merging, run `gh stack sync` to update your local branches. + +The exception is a trunk that uses a merge queue: `gh stack merge` merges directly rather than through the queue, so it isn't supported there. Use `gh pr merge` or the GitHub web UI to merge through the queue instead. + +:::note[Admin bypass not supported] +Stack merges currently do not support admin bypass merging. +::: + ## Syncing After Merges When a PR at the bottom of the stack is merged on GitHub, use `gh stack sync` to update your local state: diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index 311d134e..e8283024 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -460,6 +460,48 @@ gh stack link --base develop --open feat-a feat-b feat-c --- +### `gh stack merge` + +Merge one or multiple stacked PRs at once. + +```sh +gh stack merge [ | ] +``` + +All members of the stack up to and including your chosen pull request are merged into the base branch in a single, all-or-nothing operation: if any PR can't be merged, none are. + +With no argument, the current active local stack is used. Pass a stack number to merge a stack you don't have checked out (a purely remote operation), or a pull request number to merge directly up to that PR. + +In an interactive terminal, a short wizard walks you through three steps — choose which PRs to merge, pick the merge method, and confirm. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. + +Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Admin bypass is not supported** for stacked PR merges at this time. + +Note that this command does not work with merge queues. If the stack's base branch uses a merge queue, use `gh pr merge` instead. + +| Flag | Description | +|------|-------------| +| `--merge-method ` | Merge method to use: `merge`, `squash`, or `rebase` | +| `--merge` / `--squash` / `--rebase` | Shorthands for the corresponding merge method | +| `-y, --yes` | Merge without prompting for confirmation | + +**Examples:** + +```sh +# Merge the current stack (interactive picker) +gh stack merge + +# Merge a stack you don't have checked out, by stack number +gh stack merge 7 + +# Merge everything up to and including PR #42 +gh stack merge 42 + +# Merge the whole current stack without prompting, squashing +gh stack merge --yes --squash +``` + +--- + ## Navigation Move between branches in the current stack without having to remember branch names. The **bottom** of the stack is the branch closest to the trunk, and the **top** is furthest from it. `up` moves away from trunk; `down` moves toward it. diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index fcb67d96..6433a9f5 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -7,7 +7,7 @@ description: > branch chains, or incremental code review workflows. metadata: author: github - version: "0.0.8" + version: "0.0.9" --- # gh-stack @@ -61,6 +61,7 @@ git config remote.pushDefault origin # if multiple remotes exist (skips remo 7. **Use standard `git add` and `git commit` for staging and committing.** This gives you full control over which changes go into each branch. The `-Am` shortcut is available but should not be the default approach—stacked PRs are most effective when each branch contains a deliberate, logical set of changes. 8. **Navigate down the stack when you need to change a lower layer.** If you're working on a frontend branch and realize you need API changes, don't hack around it at the current layer. Navigate to the appropriate branch (`gh stack down`, `gh stack checkout`, or `gh stack bottom`), make and commit the changes there, run `gh stack rebase --upstack`, then navigate back up to continue. 9. **Use `gh stack link` for external tool workflows.** When branches are managed by an external tool (jj, Sapling, etc.), use `gh stack link branch-a branch-b`. `link` does not rely on local tracking state and is intended for API-driven PR and stack management. Provide at least two branches/PRs to create or update a stack, or a stack number followed by the new branches/PRs to append them to the top of an existing stack (e.g. `gh stack link 7 branch-c`). +10. **Use `gh stack merge --yes` to merge stacked PRs.** `gh pr merge` does not work with stacked PRs. In a non-interactive terminal `gh stack merge` runs without prompting and merges the entire stack (bottom to top) atomically; pass `--yes` to be explicit. Scope the merge by passing a pull request number (`gh stack merge 42 --yes` merges everything up to and including PR #42) or a stack number (`gh stack merge 7 --yes`, which needs no local checkout). Choose the method with `--squash`, `--rebase`, `--merge`, or `--merge-method `; without one, the last-used method is used. The merge is all-or-nothing — if any PR can't be merged, none are, and the failure reason is reported. Only basic pull request state is checked before merging (open and not a draft); admin bypass is not supported for stacked PR merges. This command does not work with merge queues — if the base branch uses a merge queue, the command exits with an error, so use `gh pr merge` instead. **Never do any of the following — each triggers an interactive prompt or TUI that will hang:** - ❌ `gh stack view` or `gh stack view --short` — always use `gh stack view --json` @@ -164,6 +165,10 @@ Small, incidental fixes (e.g., fixing a typo you noticed) can go in the current | Check out by branch (local only) | `gh stack checkout feature-auth` | | Tear down the current stack to restructure it | `gh stack unstack` | | Tear down a specific stack by number | `gh stack unstack 7` | +| Merge the whole current stack | `gh stack merge --yes` | +| Merge a stack by number | `gh stack merge 7 --yes` | +| Merge up to a specific PR | `gh stack merge 42 --yes` | +| Merge with a specific method | `gh stack merge --yes --squash` | --- From 64594e4cf083b238abe335e8fac799458be5db65 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 18:32:49 -0400 Subject: [PATCH 06/11] more helpful error msgs --- cmd/merge.go | 4 +++- cmd/merge_test.go | 2 ++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/merge.go b/cmd/merge.go index 726edc35..667ad045 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -237,10 +237,12 @@ func resolveActiveRemoteStack(cfg *config.Config, client github.ClientOps) (*git stacks := sf.FindAllStacksForBranch(currentBranch) if len(stacks) == 0 { cfg.Errorf("current branch %q is not part of a stack", currentBranch) + cfg.Printf("Checkout a stack first, or specify which stack or pull request to merge with `%s`", cfg.ColorCyan("gh stack merge [number]")) return nil, ErrNotInStack } if len(stacks) > 1 { - cfg.Errorf("branch %q belongs to multiple stacks; check out a non-trunk branch first", currentBranch) + cfg.Errorf("branch %q belongs to multiple stacks", currentBranch) + cfg.Printf("Checkout a stack first, or specify which stack or pull request to merge with `%s`", cfg.ColorCyan("gh stack merge [number]")) return nil, ErrDisambiguate } s := stacks[0] diff --git a/cmd/merge_test.go b/cmd/merge_test.go index 8284d7cb..ebdba7e9 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -413,6 +413,8 @@ func TestRunMerge_NoArg_NotInStack(t *testing.T) { assert.ErrorIs(t, err, ErrNotInStack) assert.Contains(t, output, "not part of a stack") + assert.Contains(t, output, "Checkout a stack first, or specify which stack or pull request to merge with") + assert.Contains(t, output, "gh stack merge [number]") } func TestRunMerge_DefaultMethodFallsBackToAllowed(t *testing.T) { From ab98f132e56aa6dae171c53d3e56bbfbfbbf92ee Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Sun, 26 Jul 2026 15:03:48 -0400 Subject: [PATCH 07/11] update to support merge queue --- .github/copilot-instructions.md | 2 +- AGENTS.md | 4 +- README.md | 4 +- cmd/merge.go | 48 ++++++------- cmd/merge_test.go | 33 ++++++--- docs/src/content/docs/guides/workflows.md | 2 +- docs/src/content/docs/reference/cli.md | 4 +- internal/github/client_interface.go | 1 - internal/github/merge_async.go | 86 +++++++---------------- internal/github/merge_async_test.go | 72 ++++--------------- internal/github/mock_client.go | 8 --- internal/tui/mergeview/model.go | 16 +++-- internal/tui/mergeview/model_test.go | 24 +++++++ internal/tui/mergeview/types.go | 5 ++ skills/gh-stack/SKILL.md | 7 +- 15 files changed, 139 insertions(+), 177 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index ec387e50..d1fef842 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,7 +17,7 @@ No Makefile, no code generation, no external linter config. Standard Go toolchai - `cmd/`: One Cobra command per file. Each exports `Cmd(cfg *config.Config)` with logic in `run()`. - `internal/git/`: `Ops` interface (52 methods) wrapping git CLI. `MockOps` for tests. Package-level functions delegate to swappable `ops` variable. -- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`), with `BaseBranchPolicy` (GraphQL) gating merge-queue branches. +- `internal/github/`: `ClientOps` interface (17 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`), which routes to a direct merge or the base branch's merge queue automatically. - `internal/config/`: `Config` struct passed to all commands. Holds I/O, colors, and test hooks (`SelectFn`, `ConfirmFn`, `InputFn`, `GitHubClientOverride`). - `internal/stack/`: Stack file (`.git/gh-stack`, JSON) management with file locking. - `internal/tui/`: bubbletea views (`stackview`, `modifyview`). diff --git a/AGENTS.md b/AGENTS.md index 545fea46..eef527e9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,7 @@ internal/ gitops.go # Ops interface (52 methods) mock_ops.go # MockOps. Each method has a corresponding *Fn field. github/ # github.ClientOps interface + real Client - client_interface.go # ClientOps interface (18 methods) + client_interface.go # ClientOps interface (17 methods) mock_client.go # MockClient. Uses function-pointer fields for testing. stack/ # stack file (.git/gh-stack) management, JSON schema, locking schema.json # JSON Schema for the stack file format @@ -109,7 +109,7 @@ if errors.As(err, &exitErr) { ... } ### Key interfaces - **`git.Ops`** (`internal/git/gitops.go`): 52 methods wrapping git CLI calls. The production implementation uses `cli/go-gh`'s `client.Command()` via `run()` and `runSilent()` helpers. Package-level functions (e.g., `git.CurrentBranch()`) delegate to a swappable package-level `ops` variable. -- **`github.ClientOps`** (`internal/github/client_interface.go`): 18 methods for GitHub API (PRs, stacks, merges). Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`): `ListStacks`, `FindStackForPR`, `GetStack`, `CreateStack`, `AddToStack` (delta append), `Unstack`. Async stack merges use `RepoMergeConfig` (GraphQL allowed methods + viewer default), `MergeStackAsync`, and `GetAsyncMergeResult` (`/repos/{owner}/{repo}/pulls/{n}/merge-async`); `BaseBranchPolicy` (GraphQL) reports whether the base branch requires a merge queue (unsupported). Injected via `cfg.GitHubClientOverride` in tests. +- **`github.ClientOps`** (`internal/github/client_interface.go`): 17 methods for GitHub API (PRs, stacks, merges). Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`): `ListStacks`, `FindStackForPR`, `GetStack`, `CreateStack`, `AddToStack` (delta append), `Unstack`. Async stack merges use `RepoMergeConfig` (GraphQL: allowed merge methods + viewer's default), `MergeStackAsync`, and `GetAsyncMergeResult` (`/repos/{owner}/{repo}/pulls/{n}/merge-async`). Injected via `cfg.GitHubClientOverride` in tests. - **`config.Config`** (`internal/config/config.go`): Central configuration passed to all commands. Holds I/O streams, color functions, and test hook fields (`SelectFn`, `ConfirmFn`, `InputFn`, `RepoOverride`). ### Stack file diff --git a/README.md b/README.md index a8063424..1ed0392f 100644 --- a/README.md +++ b/README.md @@ -453,9 +453,9 @@ With no argument, the current active local stack is used. Pass a stack number to In an interactive terminal, a short wizard walks you through three steps — choose which PRs to merge, pick the merge method, and confirm. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. -Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Admin bypass is not supported** for stacked PR merges at this time. +Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Bypassing merge requirements is not supported** for stacked PR merges. -Note that this command does not work with merge queues. If the stack's base branch uses a merge queue, use `gh pr merge` instead. +If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it's merged directly. | Flag | Description | |------|-------------| diff --git a/cmd/merge.go b/cmd/merge.go index 667ad045..1956fd75 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -63,10 +63,11 @@ prompting, using your last-used merge method unless one is specified. Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so -any such failure is reported back to you. +any such failure is reported back to you. Bypassing merge requirements with admin +privileges is not supported for stacks. -If the base branch uses a merge queue, this command isn't supported (it merges -directly, not through the queue); use "gh pr merge" or the web UI instead.`, +If the base branch uses a merge queue, the stack is added to the queue and merges +once the queue processes it; otherwise it is merged directly.`, Example: ` # Merge the current stack (interactive picker) $ gh stack merge @@ -143,18 +144,6 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { base := remoteStack.Base.Ref - // Merge-queue and admin-bypass policy for the stack's base branch. The async - // stack merge cannot use a merge queue, so bail out early (before any - // prompting) when the base requires one. - policy, err := client.BaseBranchPolicy(base) - if err != nil { - cfg.Errorf("failed to check base branch merge settings: %s", err) - return ErrAPIFailure - } - if policy.RequiresMergeQueue { - return explainMergeQueueUnsupported(cfg, base) - } - if cfg.IsInteractive() && !opts.yes { return runMergeInteractive(cfg, client, remoteStack.Number, base, candidates, allowed, mergeCfg.DefaultMethod, method, preselectIndex, opts) } @@ -328,6 +317,9 @@ func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumbe case out.Merged: mergedSuccess(cfg, prNumberList(out.MergedPRs), base, out.SHA) return nil + case out.Enqueued: + enqueuedSuccess(cfg, prNumberList(out.MergedPRs), base) + return nil case out.Failed: cfg.Errorf("merge failed: %s", out.Message) cfg.Printf("Stack merges are atomic, so nothing was merged.") @@ -362,6 +354,10 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, mergedSuccess(cfg, list, base, res.Details.SHA) return nil } + if res.IsEnqueued() { + enqueuedSuccess(cfg, list, base) + return nil + } if res.IsFailed() { cfg.Errorf("merge failed: %s", res.Details.Message) cfg.Printf("Stack merges are atomic, so nothing was merged.") @@ -394,6 +390,10 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, mergedSuccess(cfg, list, base, status.Details.SHA) return nil } + if status.IsEnqueued() { + enqueuedSuccess(cfg, list, base) + return nil + } if status.IsFailed() { cfg.Errorf("merge failed: %s", status.Details.Message) cfg.Printf("Stack merges are atomic, so nothing was merged.") @@ -430,6 +430,8 @@ func toMergeStatus(res *github.AsyncMergeResult) mergeview.MergeStatus { switch { case res.IsMerged(): status = mergeview.StatusMerged + case res.IsEnqueued(): + status = mergeview.StatusEnqueued case res.IsFailed(): status = mergeview.StatusFailed } @@ -504,15 +506,6 @@ func warnAsyncMergeUnavailable(cfg *config.Config) { cfg.Warningf("Async stack merge is not available for this repository") } -// explainMergeQueueUnsupported reports that the stack's base branch merges -// through a merge queue, which the async stack merge cannot use, and points the -// user to the merge queue (via `gh pr merge` or the web UI) instead. -func explainMergeQueueUnsupported(cfg *config.Config, base string) error { - cfg.Errorf("the base branch %q requires a merge queue, which \"gh stack merge\" does not support", base) - cfg.Printf("Merge this stack using `%q` or from the GitHub web UI instead.", "gh pr merge") - return ErrSilent -} - // mergeFailureExit maps a merge failure message to an exit code: rebase/merge // conflicts get ErrConflict, everything else ErrAPIFailure. func mergeFailureExit(message string) error { @@ -635,6 +628,13 @@ func mergedSuccess(cfg *config.Config, list, base, sha string) { cfg.Successf("Merged %s into %s", list, base) } +// enqueuedSuccess prints the success line when the base branch uses a merge +// queue: the stack was added to the queue and will merge once it's processed. +func enqueuedSuccess(cfg *config.Config, list, base string) { + cfg.Successf("Added %s to the merge queue for %s", list, base) + cfg.Printf("They will merge once the queue processes them.") +} + func isNotFound(err error) bool { var httpErr *api.HTTPError return errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound diff --git a/cmd/merge_test.go b/cmd/merge_test.go index ebdba7e9..68b7ba5a 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -347,28 +347,43 @@ func TestRunMerge_AlreadyMergedOnSubmit(t *testing.T) { assert.Contains(t, output, "Merged #1, #2 into main") } -func TestRunMerge_MergeQueueRequired(t *testing.T) { +func TestRunMerge_Enqueued(t *testing.T) { cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - BaseBranchPolicyFn: func(base string) (*github.BaseBranchPolicy, error) { - assert.Equal(t, "main", base) - return &github.BaseBranchPolicy{RequiresMergeQueue: true}, nil + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil + }, + GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusEnqueued, Details: github.AsyncMergeDetails{Message: "Pull request was added to the merge queue."}}, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Contains(t, output, "Added #1, #2 to the merge queue for main") +} + +func TestRunMerge_EnqueuedOnSubmit(t *testing.T) { + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - t.Fatal("merge must not be attempted when the base requires a merge queue") - return nil, nil + return &github.AsyncMergeResult{Status: github.AsyncMergeStatusEnqueued, Details: github.AsyncMergeDetails{Message: "Pull request was added to the merge queue."}}, nil }, } err := runMerge(cfg, fastOptions(), []string{"7"}) output := collectOutput(cfg, outR, errR) - assert.ErrorIs(t, err, ErrSilent) - assert.Contains(t, output, "merge queue") - assert.Contains(t, output, "web UI") + require.NoError(t, err) + assert.Contains(t, output, "Added #1, #2 to the merge queue for main") } func TestRunMerge_AsyncMergeUnavailable(t *testing.T) { diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index f9c867d5..f0cbba71 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -143,7 +143,7 @@ gh stack merge --yes --squash In an interactive terminal, a short wizard lets you choose how far up the stack to merge, pick the merge method (only the ones your repository allows, defaulting to your last-used method), and confirm — then shows live progress. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting. After merging, run `gh stack sync` to update your local branches. -The exception is a trunk that uses a merge queue: `gh stack merge` merges directly rather than through the queue, so it isn't supported there. Use `gh pr merge` or the GitHub web UI to merge through the queue instead. +If the base branch uses a merge queue, `gh stack merge` adds the stack to the queue instead of merging directly — it merges once the queue processes it. :::note[Admin bypass not supported] Stack merges currently do not support admin bypass merging. diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index e8283024..b8723ddb 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -474,9 +474,9 @@ With no argument, the current active local stack is used. Pass a stack number to In an interactive terminal, a short wizard walks you through three steps — choose which PRs to merge, pick the merge method, and confirm. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. -Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Admin bypass is not supported** for stacked PR merges at this time. +Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Bypassing merge requirements is not supported** for stacked PR merges. -Note that this command does not work with merge queues. If the stack's base branch uses a merge queue, use `gh pr merge` instead. +If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it's merged directly. | Flag | Description | |------|-------------| diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index 632b4555..dd3bfe0c 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -21,7 +21,6 @@ type ClientOps interface { MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) PRTitles(numbers []int) (map[int]string, error) - BaseBranchPolicy(baseRef string) (*BaseBranchPolicy, error) } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go index 3928c1a7..6154054a 100644 --- a/internal/github/merge_async.go +++ b/internal/github/merge_async.go @@ -66,19 +66,21 @@ func (c RepoMergeConfig) Allows(method string) bool { // AsyncMergeDetails is the polymorphic "details" object shared by the submit and // poll responses. Fields are populated based on the current state: a pending -// request carries UUID/MergeMethod/ExpectedHeadSHA, a merged result carries SHA, -// and a failed/not-mergeable result carries only Message. +// request carries UUID/MergeMethod/MergeAction/ExpectedHeadSHA, a merged result +// carries SHA, and a failed/not-mergeable result carries only Message. type AsyncMergeDetails struct { Message string `json:"message"` UUID string `json:"uuid"` MergeMethod string `json:"merge_method"` + MergeAction string `json:"merge_action"` ExpectedHeadSHA string `json:"expected_head_sha"` SHA string `json:"sha"` } // AsyncMergeResult is the response body returned by both the submit and poll // async merge endpoints. Status is one of the AsyncMergeStatus* values: -// "pending" (running in the background), "merged" (completed), or "failed". +// "pending" (running in the background), "merged" (merged directly), "enqueued" +// (added to the base branch's merge queue), or "failed". type AsyncMergeResult struct { Status string `json:"status"` Details AsyncMergeDetails `json:"details"` @@ -86,9 +88,10 @@ type AsyncMergeResult struct { // Async merge status values returned in the response's "status" field. const ( - AsyncMergeStatusPending = "pending" - AsyncMergeStatusMerged = "merged" - AsyncMergeStatusFailed = "failed" + AsyncMergeStatusPending = "pending" + AsyncMergeStatusMerged = "merged" + AsyncMergeStatusEnqueued = "enqueued" + AsyncMergeStatusFailed = "failed" ) // IsMerged reports whether the merge completed successfully. @@ -96,6 +99,12 @@ func (r *AsyncMergeResult) IsMerged() bool { return r != nil && r.Status == AsyncMergeStatusMerged } +// IsEnqueued reports whether the stack was added to the base branch's merge +// queue (it will merge once the queue processes it). +func (r *AsyncMergeResult) IsEnqueued() bool { + return r != nil && r.Status == AsyncMergeStatusEnqueued +} + // IsFailed reports whether the merge was attempted but did not complete. func (r *AsyncMergeResult) IsFailed() bool { return r != nil && r.Status == AsyncMergeStatusFailed @@ -136,68 +145,25 @@ func (c *Client) RepoMergeConfig() (*RepoMergeConfig, error) { }, nil } -// BaseBranchPolicy describes merge-relevant policy on a stack's base branch. -type BaseBranchPolicy struct { - // RequiresMergeQueue reports that the base branch merges through a merge - // queue, so a direct async stack merge is not possible. - RequiresMergeQueue bool -} - -// BaseBranchPolicy reports whether the given base branch requires a merge queue, -// which the async stack merge cannot use. -func (c *Client) BaseBranchPolicy(baseRef string) (*BaseBranchPolicy, error) { - var query struct { - Repository struct { - MergeQueue *struct { - ID string `graphql:"id"` - } `graphql:"mergeQueue(branch: $branch)"` - Ref *struct { - Rules struct { - Nodes []struct { - Type string `graphql:"type"` - } `graphql:"nodes"` - } `graphql:"rules(first: 50)"` - } `graphql:"ref(qualifiedName: $qualified)"` - } `graphql:"repository(owner: $owner, name: $name)"` - } - - variables := map[string]interface{}{ - "owner": graphql.String(c.owner), - "name": graphql.String(c.repo), - "branch": graphql.String(baseRef), - "qualified": graphql.String("refs/heads/" + baseRef), - } - - if err := c.gql.Query("BaseBranchPolicy", &query, variables); err != nil { - return nil, fmt.Errorf("querying base branch policy: %w", err) - } - - r := query.Repository - policy := &BaseBranchPolicy{RequiresMergeQueue: r.MergeQueue != nil} - if r.Ref != nil { - for _, node := range r.Ref.Rules.Nodes { - if node.Type == "MERGE_QUEUE" { - policy.RequiresMergeQueue = true - } - } - } - return policy, nil -} - // MergeStackAsync requests an asynchronous merge of the given pull request. For // a stacked PR this merges all members of the stack up to and including // prNumber. A blank method lets the server apply its default. // +// The merge_action is always "default", which lets the server route the stack +// to a direct merge or the base branch's merge queue automatically, based on +// the repository's rules and configuration. +// // On success the returned result is populated for the 200 (already merged) and -// 202 (enqueued) responses. A 404 returns ErrAsyncMergeUnavailable, a 409 -// (a request already exists) returns a clear "already exists" error, and any -// other non-2xx status is returned as-is. +// 202 (enqueued for background processing) responses. A 404 returns +// ErrAsyncMergeUnavailable, a 409 (a request already exists) returns a clear +// "already exists" error, and any other non-2xx status is returned as-is. func (c *Client) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) { type reqBody struct { MergeMethod string `json:"merge_method,omitempty"` + MergeAction string `json:"merge_action"` } - body, err := json.Marshal(reqBody{MergeMethod: method}) + body, err := json.Marshal(reqBody{MergeMethod: method, MergeAction: "default"}) if err != nil { return nil, fmt.Errorf("marshaling request: %w", err) } @@ -212,8 +178,8 @@ func (c *Client) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult // GetAsyncMergeResult fetches the current result of a previously submitted async // merge, identified by the UUID returned from MergeStackAsync. A valid lookup -// always returns 200, so the wrapped Queued/Merged/Details state reflects the -// merge's progress (queued, merged, or failed). +// always returns 200, so the wrapped status/details reflect the merge's progress +// (pending, merged, enqueued, or failed). func (c *Client) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) { path := fmt.Sprintf("repos/%s/%s/pulls/%d/merge-async/%s", c.owner, c.repo, prNumber, uuid) var result AsyncMergeResult diff --git a/internal/github/merge_async_test.go b/internal/github/merge_async_test.go index d01e6fec..92c414b2 100644 --- a/internal/github/merge_async_test.go +++ b/internal/github/merge_async_test.go @@ -58,7 +58,7 @@ func TestMergeStackAsync_Accepted(t *testing.T) { assert.Equal(t, http.MethodPut, rec.method) assert.Equal(t, "/repos/o/r/pulls/42/merge-async", rec.path) - assert.JSONEq(t, `{"merge_method":"squash"}`, rec.body) + assert.JSONEq(t, `{"merge_method":"squash","merge_action":"default"}`, rec.body) assert.True(t, res.IsPending()) assert.False(t, res.IsMerged()) @@ -107,8 +107,9 @@ func TestGetAsyncMergeResult_States(t *testing.T) { body string wantStatus string }{ - {"pending", `{"status":"pending","details":{"message":"Merge request is in progress.","uuid":"u","merge_method":"merge","expected_head_sha":"abc"}}`, AsyncMergeStatusPending}, + {"pending", `{"status":"pending","details":{"message":"Merge request is in progress.","uuid":"u","merge_method":"merge","merge_action":"default","expected_head_sha":"abc"}}`, AsyncMergeStatusPending}, {"merged", `{"status":"merged","details":{"message":"Pull request was merged.","sha":"abc"}}`, AsyncMergeStatusMerged}, + {"enqueued", `{"status":"enqueued","details":{"message":"Pull request was added to the merge queue."}}`, AsyncMergeStatusEnqueued}, {"failed", `{"status":"failed","details":{"message":"Merge conflict."}}`, AsyncMergeStatusFailed}, } for _, tt := range tests { @@ -161,75 +162,28 @@ func TestAsyncMergeResult_Status(t *testing.T) { assert.True(t, failed.IsFailed()) assert.False(t, failed.IsMerged()) + enqueued := &AsyncMergeResult{Status: AsyncMergeStatusEnqueued} + assert.True(t, enqueued.IsEnqueued()) + assert.False(t, enqueued.IsMerged()) + assert.False(t, enqueued.IsPending()) + var nilRes *AsyncMergeResult assert.False(t, nilRes.IsPending()) assert.False(t, nilRes.IsMerged()) + assert.False(t, nilRes.IsEnqueued()) assert.False(t, nilRes.IsFailed()) } -// sanity check that the submit body omits merge_method when empty. +// sanity check that the submit body omits merge_method when empty but always +// sends merge_action. func TestMergeStackAsync_OmitsEmptyMethod(t *testing.T) { var rec recordedRequest - _, err := testAsyncClient(t, http.StatusAccepted, `{"queued":true,"merged":false,"details":{"message":"m","uuid":"u","merge_method":"merge","expected_head_sha":"x"}}`, &rec).MergeStackAsync(1, "") + _, err := testAsyncClient(t, http.StatusAccepted, `{"status":"pending","details":{"message":"m","uuid":"u","merge_method":"merge","merge_action":"default","expected_head_sha":"x"}}`, &rec).MergeStackAsync(1, "") require.NoError(t, err) var parsed map[string]any require.NoError(t, json.Unmarshal([]byte(rec.body), &parsed)) _, hasMethod := parsed["merge_method"] assert.False(t, hasMethod, "merge_method should be omitted when empty") -} - -// testPolicyClient builds a Client whose GraphQL client is backed by a stub -// transport returning the given response body. -func testPolicyClient(t *testing.T, graphqlResp string) *Client { - t.Helper() - rt := roundTripFunc(func(r *http.Request) (*http.Response, error) { - return &http.Response{ - StatusCode: http.StatusOK, - Body: io.NopCloser(strings.NewReader(graphqlResp)), - Header: http.Header{"Content-Type": []string{"application/json"}}, - Request: r, - }, nil - }) - gql, err := api.NewGraphQLClient(api.ClientOptions{Host: "github.com", AuthToken: "x", Transport: rt}) - require.NoError(t, err) - return &Client{gql: gql, owner: "o", repo: "r"} -} - -func TestBaseBranchPolicy(t *testing.T) { - tests := []struct { - name string - body string - wantQueue bool - }{ - { - name: "no merge queue", - body: `{"data":{"repository":{"mergeQueue":null,"ref":{"rules":{"nodes":[]}}}}}`, - }, - { - name: "merge queue via mergeQueue field", - body: `{"data":{"repository":{"mergeQueue":{"id":"MQ"},"ref":{"rules":{"nodes":[]}}}}}`, - wantQueue: true, - }, - { - name: "merge queue via ruleset type", - body: `{"data":{"repository":{"mergeQueue":null,"ref":{"rules":{"nodes":[{"type":"MERGE_QUEUE"}]}}}}}`, - wantQueue: true, - }, - { - name: "other rules, no merge queue", - body: `{"data":{"repository":{"mergeQueue":null,"ref":{"rules":{"nodes":[{"type":"PULL_REQUEST"}]}}}}}`, - }, - { - name: "null ref", - body: `{"data":{"repository":{"mergeQueue":null,"ref":null}}}`, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - policy, err := testPolicyClient(t, tt.body).BaseBranchPolicy("main") - require.NoError(t, err) - assert.Equal(t, tt.wantQueue, policy.RequiresMergeQueue, "RequiresMergeQueue") - }) - } + assert.Equal(t, "default", parsed["merge_action"], "merge_action should always be sent") } diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index a23ed8b1..b201b799 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -21,7 +21,6 @@ type MockClient struct { MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) PRTitlesFn func([]int) (map[int]string, error) - BaseBranchPolicyFn func(string) (*BaseBranchPolicy, error) } // Compile-time check that MockClient satisfies ClientOps. @@ -163,10 +162,3 @@ func (m *MockClient) PRTitles(numbers []int) (map[int]string, error) { } return map[int]string{}, nil } - -func (m *MockClient) BaseBranchPolicy(baseRef string) (*BaseBranchPolicy, error) { - if m.BaseBranchPolicyFn != nil { - return m.BaseBranchPolicyFn(baseRef) - } - return &BaseBranchPolicy{}, nil -} diff --git a/internal/tui/mergeview/model.go b/internal/tui/mergeview/model.go index e15c030b..0df2ee5c 100644 --- a/internal/tui/mergeview/model.go +++ b/internal/tui/mergeview/model.go @@ -30,6 +30,7 @@ type Model struct { submitted bool merged bool + enqueued bool failed bool cancelled bool watchStopped bool @@ -266,12 +267,15 @@ func (m Model) handleSubmitDone(msg submitDoneMsg) (tea.Model, tea.Cmd) { case StatusMerged: m.merged = true return m.finish() + case StatusEnqueued: + m.enqueued = true + return m.finish() case StatusFailed: m.failed = true return m.finish() default: - // Pending (enqueued, or an existing request adopted): poll if we have a - // UUID; otherwise this is unexpected, so treat it as a failure. + // Pending (or an existing request adopted): poll if we have a UUID; + // otherwise this is unexpected, so treat it as a failure. if msg.status.UUID != "" { return m, m.pollTickCmd() } @@ -295,6 +299,9 @@ func (m Model) handlePollDone(msg pollDoneMsg) (tea.Model, tea.Cmd) { case StatusMerged: m.merged = true return m.finish() + case StatusEnqueued: + m.enqueued = true + return m.finish() case StatusFailed: m.failed = true return m.finish() @@ -309,7 +316,7 @@ func (m Model) finish() (tea.Model, tea.Cmd) { return m, tea.Quit } -func (m Model) done() bool { return m.merged || m.failed || m.step == StepDone } +func (m Model) done() bool { return m.merged || m.enqueued || m.failed || m.step == StepDone } // Outcome reports the final result of the wizard for the command layer. func (m Model) Outcome() Outcome { @@ -317,6 +324,7 @@ func (m Model) Outcome() Outcome { Cancelled: m.cancelled, Submitted: m.submitted, Merged: m.merged, + Enqueued: m.enqueued, Failed: m.failed, WatchStopped: m.watchStopped, Message: m.message, @@ -325,7 +333,7 @@ func (m Model) Outcome() Outcome { SHA: m.status.SHA, Err: m.err, } - if m.merged { + if m.merged || m.enqueued { o.MergedPRs = m.selectedNumbers() } return o diff --git a/internal/tui/mergeview/model_test.go b/internal/tui/mergeview/model_test.go index a1ae9bda..3802dce9 100644 --- a/internal/tui/mergeview/model_test.go +++ b/internal/tui/mergeview/model_test.go @@ -232,6 +232,30 @@ func TestProgress_PendingThenMerged(t *testing.T) { assert.Equal(t, []int{1, 2, 3}, out.MergedPRs) } +func TestProgress_PendingThenEnqueued(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m.submitted = true + + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusPending, UUID: "u1"}}) + m = step(m, pollDoneMsg{status: MergeStatus{Status: StatusEnqueued, Message: "Pull request was added to the merge queue."}}) + out := m.Outcome() + assert.True(t, out.Enqueued) + assert.False(t, out.Merged) + assert.False(t, out.Failed) + assert.Equal(t, []int{1, 2, 3}, out.MergedPRs) +} + +func TestSubmit_Enqueued(t *testing.T) { + m := New(baseOptions()) + m.step = StepProgress + m.submitted = true + m = step(m, submitDoneMsg{status: MergeStatus{Status: StatusEnqueued, Message: "Pull request was added to the merge queue."}}) + out := m.Outcome() + assert.True(t, out.Enqueued) + assert.False(t, out.Merged) +} + func TestSubmit_NotMergeable(t *testing.T) { m := New(baseOptions()) m.step = StepProgress diff --git a/internal/tui/mergeview/types.go b/internal/tui/mergeview/types.go index f0f329c2..1741827e 100644 --- a/internal/tui/mergeview/types.go +++ b/internal/tui/mergeview/types.go @@ -46,6 +46,8 @@ const ( StatusPending Status = "pending" // StatusMerged means the merge completed successfully. StatusMerged Status = "merged" + // StatusEnqueued means the stack was added to the base branch's merge queue. + StatusEnqueued Status = "enqueued" // StatusFailed means the merge was attempted but did not complete. StatusFailed Status = "failed" ) @@ -106,6 +108,9 @@ type Outcome struct { Submitted bool // Merged reports the merge completed successfully. Merged bool + // Enqueued reports the stack was added to the base branch's merge queue + // (it will merge once the queue processes it). + Enqueued bool // Failed reports the merge was attempted but did not complete (conflict, // rule failure, or not mergeable). Failed bool diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 6433a9f5..831c046d 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -61,7 +61,7 @@ git config remote.pushDefault origin # if multiple remotes exist (skips remo 7. **Use standard `git add` and `git commit` for staging and committing.** This gives you full control over which changes go into each branch. The `-Am` shortcut is available but should not be the default approach—stacked PRs are most effective when each branch contains a deliberate, logical set of changes. 8. **Navigate down the stack when you need to change a lower layer.** If you're working on a frontend branch and realize you need API changes, don't hack around it at the current layer. Navigate to the appropriate branch (`gh stack down`, `gh stack checkout`, or `gh stack bottom`), make and commit the changes there, run `gh stack rebase --upstack`, then navigate back up to continue. 9. **Use `gh stack link` for external tool workflows.** When branches are managed by an external tool (jj, Sapling, etc.), use `gh stack link branch-a branch-b`. `link` does not rely on local tracking state and is intended for API-driven PR and stack management. Provide at least two branches/PRs to create or update a stack, or a stack number followed by the new branches/PRs to append them to the top of an existing stack (e.g. `gh stack link 7 branch-c`). -10. **Use `gh stack merge --yes` to merge stacked PRs.** `gh pr merge` does not work with stacked PRs. In a non-interactive terminal `gh stack merge` runs without prompting and merges the entire stack (bottom to top) atomically; pass `--yes` to be explicit. Scope the merge by passing a pull request number (`gh stack merge 42 --yes` merges everything up to and including PR #42) or a stack number (`gh stack merge 7 --yes`, which needs no local checkout). Choose the method with `--squash`, `--rebase`, `--merge`, or `--merge-method `; without one, the last-used method is used. The merge is all-or-nothing — if any PR can't be merged, none are, and the failure reason is reported. Only basic pull request state is checked before merging (open and not a draft); admin bypass is not supported for stacked PR merges. This command does not work with merge queues — if the base branch uses a merge queue, the command exits with an error, so use `gh pr merge` instead. +10. **Use `gh stack merge --yes` to merge stacked PRs.** `gh pr merge` does not work with stacked PRs. In a non-interactive terminal `gh stack merge` runs without prompting and merges the entire stack (bottom to top) atomically; pass `--yes` to be explicit. Scope the merge by passing a pull request number (`gh stack merge 42 --yes` merges everything up to and including PR #42) or a stack number (`gh stack merge 7 --yes`, which needs no local checkout). Choose the method with `--squash`, `--rebase`, `--merge`, or `--merge-method `; without one, the last-used method is used. The merge is all-or-nothing — if any PR can't be merged, none are, and the failure reason is reported. Only basic pull request state is checked before merging (open and not a draft); bypassing merge requirements is not supported for stacks. If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it's merged directly. **Never do any of the following — each triggers an interactive prompt or TUI that will hang:** - ❌ `gh stack view` or `gh stack view --short` — always use `gh stack view --json` @@ -887,6 +887,5 @@ gh stack unstack --local 1. **Stacks are strictly linear.** Branching stacks (multiple children on a single parent) are not supported. Each branch has exactly one parent and at most one child. If you need parallel workstreams, use separate stacks. 2. **Stack disambiguation cannot be bypassed.** If the current branch is the trunk of multiple stacks, commands error with code 6. Check out a non-shared branch first. 3. **Multiple remotes require `--remote` or config.** If more than one remote is configured, set `remote.pushDefault` in git config, or pass `--remote ` to the commands that accept it (`push`, `submit`, `sync`, `rebase`, `link`). `checkout`, `modify`, and `trunk` have no `--remote` flag and rely on `remote.pushDefault`. -4. **Merging PRs:** Merging Stacked PRs from the CLI is not supported yet. Direct users to open the PR URL in a browser to merge PRs. -5. **Remote stack checkout requires a stack or PR number.** `checkout` with a branch name only works with locally tracked stacks. Use a stack number or PR number (e.g. `gh stack checkout 7` or `gh stack checkout 123`) to pull a stack from GitHub. -6. **PR title and body are auto-generated.** There is no flag to set a custom PR title or body during `submit`. The title and body are generated from commit messages plus a footer. Use `gh pr edit` to modify PR title and body after creation. +4. **Remote stack checkout requires a stack or PR number.** `checkout` with a branch name only works with locally tracked stacks. Use a stack number or PR number (e.g. `gh stack checkout 7` or `gh stack checkout 123`) to pull a stack from GitHub. +5. **PR title and body are auto-generated.** There is no flag to set a custom PR title or body during `submit`. The title and body are generated from commit messages plus a footer. Use `gh pr edit` to modify PR title and body after creation. From 2f8fc0b2180a72168e70f13fc0c038ee818d58ba Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Tue, 28 Jul 2026 22:47:49 -0400 Subject: [PATCH 08/11] addressing review comments --- cmd/merge.go | 13 ++++++++++-- cmd/merge_test.go | 26 +++++++++++++++++++++++ docs/src/content/docs/guides/workflows.md | 4 ++-- internal/tui/mergeview/model.go | 6 +++++- internal/tui/mergeview/model_test.go | 21 ++++++++++++++++-- internal/tui/mergeview/view.go | 5 +++-- 6 files changed, 66 insertions(+), 9 deletions(-) diff --git a/cmd/merge.go b/cmd/merge.go index 1956fd75..31da944e 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -63,8 +63,8 @@ prompting, using your last-used merge method unless one is specified. Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so -any such failure is reported back to you. Bypassing merge requirements with admin -privileges is not supported for stacks. +any such failure is reported back to you. Bypassing merge requirements is not +supported for stacks. If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it is merged directly.`, @@ -151,6 +151,15 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { // Non-interactive (or --yes): merge the whole stack (or up to the given PR) // without prompting. if !target.hasPR { + // A draft or closed pull request partway up the stack blocks everything + // above it. Rather than silently merging only the portion below it, + // refuse and let the user target an explicit pull request. + if blocker != nil { + top := candidates[len(candidates)-1].Number + cfg.Errorf("cannot merge the whole stack: pull request #%d is %s", blocker.Number, blockerState(blocker)) + cfg.Printf("Merge up to #%d with `%s`", top, cfg.ColorCyan(fmt.Sprintf("gh stack merge %d", top))) + return ErrInvalidArgs + } targetPR = candidates[len(candidates)-1].Number } if method == "" { diff --git a/cmd/merge_test.go b/cmd/merge_test.go index 68b7ba5a..f72320d0 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -272,6 +272,32 @@ func TestRunMerge_AlreadyMergedTarget(t *testing.T) { assert.Contains(t, output, "#1 is already merged") } +func TestRunMerge_WholeStackBlockedByDraft(t *testing.T) { + submitCalled := false + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(5, "main", openStackPR(1, "b1"), draftStackPR(2, "b2"), openStackPR(3, "b3")), nil + }, + RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { + return &github.RepoMergeConfig{MergeAllowed: true, DefaultMethod: "merge"}, nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + submitCalled = true + return nil, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"5"}) + output := collectOutput(cfg, outR, errR) + + assert.ErrorIs(t, err, ErrInvalidArgs) + assert.Contains(t, output, "cannot merge the whole stack") + assert.Contains(t, output, "#2 is a draft") + assert.Contains(t, output, "gh stack merge 1") + assert.False(t, submitCalled, "must not silently merge only the portion below the blocker") +} + func TestRunMerge_NothingToMerge_AllMerged(t *testing.T) { setupLocalStack(t, 100, "b1", "b1", "b2") cfg, outR, errR := config.NewTestConfig() diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index f0cbba71..96f574e9 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -145,8 +145,8 @@ In an interactive terminal, a short wizard lets you choose how far up the stack If the base branch uses a merge queue, `gh stack merge` adds the stack to the queue instead of merging directly — it merges once the queue processes it. -:::note[Admin bypass not supported] -Stack merges currently do not support admin bypass merging. +:::note[Bypassing merge requirements not supported] +Stack merges do not support bypassing merge requirements. ::: ## Syncing After Merges diff --git a/internal/tui/mergeview/model.go b/internal/tui/mergeview/model.go index 0df2ee5c..e87dbc6e 100644 --- a/internal/tui/mergeview/model.go +++ b/internal/tui/mergeview/model.go @@ -181,10 +181,14 @@ const maxVisibleItems = 10 // visibleItems is the number of pull requests shown in the select window at // once — capped at maxVisibleItems and shrunk to fit a short terminal (each -// item renders on two lines). When the terminal size is unknown, all are shown. +// item renders on two lines). When the terminal size is unknown, it still caps +// at maxVisibleItems so the first frame can't overflow a large stack. func (m Model) visibleItems() int { n := len(m.opts.PRs) if m.height <= 0 { + if n > maxVisibleItems { + return maxVisibleItems + } return n } // Reserve lines for the header, scroll indicators, summary, and footer. diff --git a/internal/tui/mergeview/model_test.go b/internal/tui/mergeview/model_test.go index 3802dce9..eac48701 100644 --- a/internal/tui/mergeview/model_test.go +++ b/internal/tui/mergeview/model_test.go @@ -7,6 +7,7 @@ import ( "testing" tea "github.com/charmbracelet/bubbletea" + "github.com/charmbracelet/lipgloss" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -91,8 +92,9 @@ func TestSelect_Viewport(t *testing.T) { } m := New(opts) - // No size yet: all items are shown. - assert.Equal(t, 30, m.visibleItems()) + // No size yet: capped at maxVisibleItems so a large stack can't overflow + // the first frame. + assert.Equal(t, 10, m.visibleItems()) // A tall terminal caps the window at maxVisibleItems (10). nm, _ := m.Update(tea.WindowSizeMsg{Width: 100, Height: 60}) @@ -137,6 +139,21 @@ func TestSelect_ArrowDirection(t *testing.T) { assert.Equal(t, 2, m.cursor) } +func TestTruncate_WideRunes(t *testing.T) { + // ASCII truncates to the requested width with a trailing ellipsis (plus an + // ANSI reset, which has zero display width). + ascii := truncate("abcdef", 3) + assert.True(t, strings.HasPrefix(ascii, "ab…")) + assert.LessOrEqual(t, lipgloss.Width(ascii), 3) + + // Double-width runes must not push the result past the requested display + // width (each CJK rune is two cells). + assert.LessOrEqual(t, lipgloss.Width(truncate("你好世界", 5)), 5) + + // A string that already fits is returned unchanged. + assert.Equal(t, "hi", truncate("hi", 5)) +} + func TestSelect_AdvanceRequiresSelection(t *testing.T) { m := New(baseOptions()) diff --git a/internal/tui/mergeview/view.go b/internal/tui/mergeview/view.go index 2d45d53d..672d1fd3 100644 --- a/internal/tui/mergeview/view.go +++ b/internal/tui/mergeview/view.go @@ -385,13 +385,14 @@ func truncate(s string, width int) string { } continue } - if w >= width-1 { + rw := lipgloss.Width(string(r)) + if w+rw > width-1 { b.WriteString("…") b.WriteString("\x1b[0m") break } b.WriteRune(r) - w++ + w += rw } return b.String() } From c37947be932ab5b9ce8468351559d19c6231ccc4 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 29 Jul 2026 06:03:41 -0400 Subject: [PATCH 09/11] hide merge method step for merge queue --- .github/copilot-instructions.md | 2 +- AGENTS.md | 2 +- cmd/merge.go | 79 +++++++++++++++++++------- cmd/merge_test.go | 85 ++++++++++++++++++++++++++++ internal/github/client_interface.go | 1 + internal/github/merge_async.go | 47 +++++++++++++++ internal/github/mock_client.go | 42 ++++++++------ internal/tui/mergeview/model.go | 33 ++++++++++- internal/tui/mergeview/model_test.go | 57 +++++++++++++++++++ internal/tui/mergeview/types.go | 5 ++ internal/tui/mergeview/view.go | 82 +++++++++++++++++++++------ 11 files changed, 375 insertions(+), 60 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d1fef842..120eacbe 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,7 +17,7 @@ No Makefile, no code generation, no external linter config. Standard Go toolchai - `cmd/`: One Cobra command per file. Each exports `Cmd(cfg *config.Config)` with logic in `run()`. - `internal/git/`: `Ops` interface (52 methods) wrapping git CLI. `MockOps` for tests. Package-level functions delegate to swappable `ops` variable. -- `internal/github/`: `ClientOps` interface (17 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`), which routes to a direct merge or the base branch's merge queue automatically. +- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`), which routes to a direct merge or the base branch's merge queue automatically. - `internal/config/`: `Config` struct passed to all commands. Holds I/O, colors, and test hooks (`SelectFn`, `ConfirmFn`, `InputFn`, `GitHubClientOverride`). - `internal/stack/`: Stack file (`.git/gh-stack`, JSON) management with file locking. - `internal/tui/`: bubbletea views (`stackview`, `modifyview`). diff --git a/AGENTS.md b/AGENTS.md index eef527e9..11d241fc 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,7 @@ internal/ gitops.go # Ops interface (52 methods) mock_ops.go # MockOps. Each method has a corresponding *Fn field. github/ # github.ClientOps interface + real Client - client_interface.go # ClientOps interface (17 methods) + client_interface.go # ClientOps interface (18 methods) mock_client.go # MockClient. Uses function-pointer fields for testing. stack/ # stack file (.git/gh-stack) management, JSON schema, locking schema.json # JSON Schema for the stack file format diff --git a/cmd/merge.go b/cmd/merge.go index 31da944e..d9b9fdc3 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -127,25 +127,44 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { return explainNothingToMerge(cfg, remoteStack, blocker) } - mergeCfg, err := client.RepoMergeConfig() - if err != nil { - cfg.Errorf("failed to fetch repository merge settings: %s", err) - return ErrAPIFailure - } - allowed := mergeCfg.AllowedMethods() - if len(allowed) == 0 { - cfg.Errorf("this repository does not allow any merge methods") - return ErrAPIFailure - } - if method != "" && !mergeCfg.Allows(method) { - cfg.Errorf("this repository does not allow %s merges", method) - return ErrInvalidArgs - } - base := remoteStack.Base.Ref + // Detect whether the base branch merges through a merge queue so the wizard + // can skip the merge-method step and enqueue instead of merging directly. + usesMergeQueue := baseBranchUsesMergeQueue(client, base) + + var mergeCfg *github.RepoMergeConfig + var allowed []string + if usesMergeQueue { + // The queue picks the merge method from its own configuration, so a + // requested method does not apply. + if method != "" { + cfg.Warningf("the base branch %q uses a merge queue; ignoring the merge method", base) + method = "" + } + } else { + mergeCfg, err = client.RepoMergeConfig() + if err != nil { + cfg.Errorf("failed to fetch repository merge settings: %s", err) + return ErrAPIFailure + } + allowed = mergeCfg.AllowedMethods() + if len(allowed) == 0 { + cfg.Errorf("this repository does not allow any merge methods") + return ErrAPIFailure + } + if method != "" && !mergeCfg.Allows(method) { + cfg.Errorf("this repository does not allow %s merges", method) + return ErrInvalidArgs + } + } + if cfg.IsInteractive() && !opts.yes { - return runMergeInteractive(cfg, client, remoteStack.Number, base, candidates, allowed, mergeCfg.DefaultMethod, method, preselectIndex, opts) + defaultMethod := "" + if mergeCfg != nil { + defaultMethod = mergeCfg.DefaultMethod + } + return runMergeInteractive(cfg, client, remoteStack.Number, base, candidates, allowed, defaultMethod, method, preselectIndex, usesMergeQueue, opts) } // Non-interactive (or --yes): merge the whole stack (or up to the given PR) @@ -162,13 +181,13 @@ func runMerge(cfg *config.Config, opts *mergeOptions, args []string) error { } targetPR = candidates[len(candidates)-1].Number } - if method == "" { + if !usesMergeQueue && method == "" { method = mergeCfg.DefaultMethod if !mergeCfg.Allows(method) { method = allowed[0] } } - return runMergeHeadless(cfg, client, base, candidates, targetPR, method, opts) + return runMergeHeadless(cfg, client, base, candidates, targetPR, method, usesMergeQueue, opts) } // resolveMergeStack determines the remote stack (and any explicitly targeted PR) @@ -275,7 +294,7 @@ func resolveActiveRemoteStack(cfg *config.Config, client github.ClientOps) (*git return rs, nil } -func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumber int, base string, candidates []mergeview.PRItem, allowed []string, viewerDefault, methodFlag string, preselectIndex int, opts *mergeOptions) error { +func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumber int, base string, candidates []mergeview.PRItem, allowed []string, viewerDefault, methodFlag string, preselectIndex int, usesMergeQueue bool, opts *mergeOptions) error { defaultMethod := viewerDefault if methodFlag != "" { defaultMethod = methodFlag @@ -303,6 +322,7 @@ func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumbe AllowedMethods: allowed, DefaultMethod: defaultMethod, PreselectTopIndex: preselectIndex, + UsesMergeQueue: usesMergeQueue, Submit: submit, Poll: poll, PollInterval: opts.pollInterval, @@ -343,11 +363,15 @@ func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumbe } } -func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, candidates []mergeview.PRItem, targetPR int, method string, opts *mergeOptions) error { +func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, candidates []mergeview.PRItem, targetPR int, method string, usesMergeQueue bool, opts *mergeOptions) error { nums := numbersUpTo(candidates, targetPR) list := prNumberList(nums) - cfg.Printf("Merging %s into %s via %s...", list, base, method) + if usesMergeQueue { + cfg.Printf("Adding %s to the merge queue for %s...", list, base) + } else { + cfg.Printf("Merging %s into %s via %s...", list, base, method) + } res, err := client.MergeStackAsync(targetPR, method) if err != nil { @@ -414,6 +438,19 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, return ErrAPIFailure } +// baseBranchUsesMergeQueue reports whether the stack's base branch merges through +// a merge queue. Detection only tailors the wizard (skipping the method step and +// switching to enqueue wording), so a lookup failure falls back to the +// direct-merge flow: the async merge always sends merge_action "default" and the +// server still routes to the queue when the branch requires one. +func baseBranchUsesMergeQueue(client github.ClientOps, base string) bool { + uses, err := client.BaseBranchUsesMergeQueue(base) + if err != nil { + return false + } + return uses +} + // mergeFuncs returns submit/poll closures that adapt the GitHub client to the // mergeview injection points. func mergeFuncs(client github.ClientOps) (mergeview.SubmitFunc, mergeview.PollFunc) { diff --git a/cmd/merge_test.go b/cmd/merge_test.go index f72320d0..8b38c446 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -131,6 +131,91 @@ func TestRunMerge_StackNumberArg(t *testing.T) { assert.Contains(t, output, "Merged #10, #11 into main") } +func TestRunMerge_MergeQueue_Headless(t *testing.T) { + gotMethod := "unset" + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(10, "a"), openStackPR(11, "b")), nil + }, + BaseBranchUsesMergeQueueFn: func(base string) (bool, error) { + assert.Equal(t, "main", base) + return true, nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotMethod = method + return &github.AsyncMergeResult{ + Status: github.AsyncMergeStatusEnqueued, + Details: github.AsyncMergeDetails{Message: "Pull request was added to the merge queue."}, + }, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, "", gotMethod, "a merge queue picks the method; none is sent") + assert.Contains(t, output, "merge queue") +} + +func TestRunMerge_MergeQueue_IgnoresMethodFlag(t *testing.T) { + gotMethod := "unset" + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(10, "a"), openStackPR(11, "b")), nil + }, + BaseBranchUsesMergeQueueFn: func(base string) (bool, error) { return true, nil }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotMethod = method + return &github.AsyncMergeResult{ + Status: github.AsyncMergeStatusEnqueued, + Details: github.AsyncMergeDetails{Message: "queued"}, + }, nil + }, + } + + opts := fastOptions() + opts.squash = true + err := runMerge(cfg, opts, []string{"7"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, "", gotMethod, "the requested method is ignored under a merge queue") + assert.Contains(t, output, "ignoring the merge method") +} + +func TestRunMerge_MergeQueueDetectionError_FallsBackToDirect(t *testing.T) { + gotMethod := "unset" + cfg, outR, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + GetStackFn: func(n int) (*github.RemoteStack, error) { + return remoteStack(7, "main", openStackPR(10, "a"), openStackPR(11, "b")), nil + }, + BaseBranchUsesMergeQueueFn: func(base string) (bool, error) { + return false, errors.New("boom") + }, + RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { + return &github.RepoMergeConfig{MergeAllowed: true, DefaultMethod: "merge"}, nil + }, + MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + gotMethod = method + return &github.AsyncMergeResult{ + Status: github.AsyncMergeStatusMerged, + Details: github.AsyncMergeDetails{SHA: "abc1234"}, + }, nil + }, + } + + err := runMerge(cfg, fastOptions(), []string{"7"}) + output := collectOutput(cfg, outR, errR) + + require.NoError(t, err) + assert.Equal(t, "merge", gotMethod, "detection failure falls back to a direct merge with a method") + assert.Contains(t, output, "Merged") +} + func TestRunMerge_PRNumberArg(t *testing.T) { var gotPR int cfg, outR, errR := config.NewTestConfig() diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index dd3bfe0c..b64f60af 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -21,6 +21,7 @@ type ClientOps interface { MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) PRTitles(numbers []int) (map[int]string, error) + BaseBranchUsesMergeQueue(baseRef string) (bool, error) } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go index 6154054a..c2ea4cb4 100644 --- a/internal/github/merge_async.go +++ b/internal/github/merge_async.go @@ -145,6 +145,53 @@ func (c *Client) RepoMergeConfig() (*RepoMergeConfig, error) { }, nil } +// BaseBranchUsesMergeQueue reports whether the given base branch merges through a +// merge queue, detected via the branch's merge queue object or a MERGE_QUEUE +// repository rule. It is used only to tailor the merge wizard (skipping the +// merge-method step and switching to "enqueue" wording): the async stack merge +// itself always sends merge_action "default", which lets the server route the +// stack to the queue or a direct merge automatically. +func (c *Client) BaseBranchUsesMergeQueue(baseRef string) (bool, error) { + var query struct { + Repository struct { + MergeQueue *struct { + ID string `graphql:"id"` + } `graphql:"mergeQueue(branch: $branch)"` + Ref *struct { + Rules struct { + Nodes []struct { + Type string `graphql:"type"` + } `graphql:"nodes"` + } `graphql:"rules(first: 50)"` + } `graphql:"ref(qualifiedName: $qualified)"` + } `graphql:"repository(owner: $owner, name: $name)"` + } + + variables := map[string]interface{}{ + "owner": graphql.String(c.owner), + "name": graphql.String(c.repo), + "branch": graphql.String(baseRef), + "qualified": graphql.String("refs/heads/" + baseRef), + } + + if err := c.gql.Query("BaseBranchMergeQueue", &query, variables); err != nil { + return false, fmt.Errorf("querying base branch merge queue: %w", err) + } + + r := query.Repository + if r.MergeQueue != nil { + return true, nil + } + if r.Ref != nil { + for _, node := range r.Ref.Rules.Nodes { + if node.Type == "MERGE_QUEUE" { + return true, nil + } + } + } + return false, nil +} + // MergeStackAsync requests an asynchronous merge of the given pull request. For // a stacked PR this merges all members of the stack up to and including // prNumber. A blank method lets the server apply its default. diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index b201b799..9a06aaf6 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -4,23 +4,24 @@ package github // Each field is an optional function that, when set, handles the corresponding // ClientOps method call. When nil, a reasonable default is returned. type MockClient struct { - FindPRForBranchFn func(string) (*PullRequest, error) - FindPRByNumberFn func(int) (*PullRequest, error) - FindPRDetailsForBranchFn func(string) (*PRDetails, error) - CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) - UpdatePRBaseFn func(int, string) error - MarkPRReadyForReviewFn func(string) error - DisableAutoMergeFn func(string) error - ListStacksFn func() ([]RemoteStack, error) - FindStackForPRFn func(int) (*RemoteStack, error) - GetStackFn func(int) (*RemoteStack, error) - CreateStackFn func([]int) (*RemoteStack, error) - AddToStackFn func(int, []int) (*RemoteStack, error) - UnstackFn func(int) (*RemoteStack, bool, error) - RepoMergeConfigFn func() (*RepoMergeConfig, error) - MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) - GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) - PRTitlesFn func([]int) (map[int]string, error) + FindPRForBranchFn func(string) (*PullRequest, error) + FindPRByNumberFn func(int) (*PullRequest, error) + FindPRDetailsForBranchFn func(string) (*PRDetails, error) + CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) + UpdatePRBaseFn func(int, string) error + MarkPRReadyForReviewFn func(string) error + DisableAutoMergeFn func(string) error + ListStacksFn func() ([]RemoteStack, error) + FindStackForPRFn func(int) (*RemoteStack, error) + GetStackFn func(int) (*RemoteStack, error) + CreateStackFn func([]int) (*RemoteStack, error) + AddToStackFn func(int, []int) (*RemoteStack, error) + UnstackFn func(int) (*RemoteStack, bool, error) + RepoMergeConfigFn func() (*RepoMergeConfig, error) + MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) + GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) + PRTitlesFn func([]int) (map[int]string, error) + BaseBranchUsesMergeQueueFn func(string) (bool, error) } // Compile-time check that MockClient satisfies ClientOps. @@ -162,3 +163,10 @@ func (m *MockClient) PRTitles(numbers []int) (map[int]string, error) { } return map[int]string{}, nil } + +func (m *MockClient) BaseBranchUsesMergeQueue(baseRef string) (bool, error) { + if m.BaseBranchUsesMergeQueueFn != nil { + return m.BaseBranchUsesMergeQueueFn(baseRef) + } + return false, nil +} diff --git a/internal/tui/mergeview/model.go b/internal/tui/mergeview/model.go index e87dbc6e..9d14e850 100644 --- a/internal/tui/mergeview/model.go +++ b/internal/tui/mergeview/model.go @@ -65,13 +65,22 @@ func New(opts Options) Model { spinner: sp, usePowerline: powerlineEnabled(), } + // A merge queue picks the merge method from its own configuration, so the + // wizard doesn't choose one and sends a blank method. + if opts.UsesMergeQueue { + m.method = "" + } m.methodCursor = indexOf(opts.AllowedMethods, m.method) if opts.PreselectTopIndex >= 0 && opts.PreselectTopIndex < len(opts.PRs) { - // PR-number mode: the target is fixed, so skip the selection step. + // PR-number mode: the target is fixed, so skip the selection step (and + // the method step too when the base uses a merge queue). m.topIndex = opts.PreselectTopIndex m.cursor = opts.PreselectTopIndex m.step = StepMethod + if opts.UsesMergeQueue { + m.step = StepConfirm + } } return m @@ -165,7 +174,7 @@ func (m Model) handleSelectKey(key string) (tea.Model, tea.Cmd) { } case "enter", "tab": if m.topIndex >= 0 { - m.step = StepMethod + m.step = m.stepAfterSelect() } case "esc", "q": m.cancelled = true @@ -175,6 +184,16 @@ func (m Model) handleSelectKey(key string) (tea.Model, tea.Cmd) { return m, nil } +// stepAfterSelect is the step reached from the PR-selection step: the merge +// method picker normally, or straight to confirmation when the base branch uses +// a merge queue (which chooses the method itself). +func (m Model) stepAfterSelect() Step { + if m.opts.UsesMergeQueue { + return StepConfirm + } + return StepMethod +} + // maxVisibleItems caps how many pull requests the select step shows at once; the // rest are reached by scrolling so the picker never takes over the screen. const maxVisibleItems = 10 @@ -248,7 +267,15 @@ func (m Model) handleConfirmKey(key string) (tea.Model, tea.Cmd) { m.submitted = true return m, tea.Batch(m.spinner.Tick, m.submitCmd()) case "shift+tab": - m.step = StepMethod + if m.opts.UsesMergeQueue { + // No method step to go back to; return to selection unless the + // target was fixed by PR-number mode. + if m.opts.PreselectTopIndex < 0 { + m.step = StepSelectPRs + } + } else { + m.step = StepMethod + } case "esc", "q": m.cancelled = true return m.finish() diff --git a/internal/tui/mergeview/model_test.go b/internal/tui/mergeview/model_test.go index eac48701..f9c7c5ea 100644 --- a/internal/tui/mergeview/model_test.go +++ b/internal/tui/mergeview/model_test.go @@ -191,6 +191,63 @@ func TestConfirm_BackWithShiftTab(t *testing.T) { assert.Equal(t, StepMethod, m.step) } +func mergeQueueOptions() Options { + o := baseOptions() + o.UsesMergeQueue = true + return o +} + +func TestMergeQueue_SkipsMethodStep(t *testing.T) { + m := New(mergeQueueOptions()) + assert.Equal(t, StepSelectPRs, m.step) + assert.Equal(t, "", m.method, "a merge queue picks the method itself") + + // Enter from the selection step goes straight to Confirm, skipping method. + m = step(m, keyType(tea.KeyEnter)) + assert.Equal(t, StepConfirm, m.step) +} + +func TestMergeQueue_ConfirmBackToSelect(t *testing.T) { + m := New(mergeQueueOptions()) + m = step(m, keyType(tea.KeyEnter)) + require.Equal(t, StepConfirm, m.step) + + // shift+tab returns to selection (there is no method step in between). + m = step(m, keyType(tea.KeyShiftTab)) + assert.Equal(t, StepSelectPRs, m.step) +} + +func TestMergeQueue_PreselectStartsAtConfirm(t *testing.T) { + o := mergeQueueOptions() + o.PreselectTopIndex = 1 + m := New(o) + assert.Equal(t, StepConfirm, m.step, "PR-number mode skips both select and method") + assert.Equal(t, []int{1, 2}, m.selectedNumbers()) + + // Confirm is the first step here, so shift+tab has nowhere to go back to. + m = step(m, keyType(tea.KeyShiftTab)) + assert.Equal(t, StepConfirm, m.step) +} + +func TestMergeQueue_ViewWording(t *testing.T) { + m := New(mergeQueueOptions()) + + // The stepper drops the merge-method stage. + assert.NotContains(t, m.stepper(), "Merge Method") + + // The selection summary mentions the merge queue. + assert.Contains(t, m.viewSelect(), "via merge queue") + + // The confirm step uses merge-queue wording and an "enqueue" action hint, + // and shows no merge method. + m = step(m, keyType(tea.KeyEnter)) + require.Equal(t, StepConfirm, m.step) + confirm := m.viewConfirm() + assert.Contains(t, confirm, "merge queue") + assert.Contains(t, confirm, "enqueue") + assert.NotContains(t, confirm, "Create a merge commit") +} + func TestMethod_SelectAndAdvance(t *testing.T) { m := New(baseOptions()) m = step(m, keyType(tea.KeyEnter)) // -> method step diff --git a/internal/tui/mergeview/types.go b/internal/tui/mergeview/types.go index 1741827e..8ee0277f 100644 --- a/internal/tui/mergeview/types.go +++ b/internal/tui/mergeview/types.go @@ -90,6 +90,11 @@ type Options struct { // DefaultMethod is the method preselected in the picker (the viewer's // last-used method). DefaultMethod string + // UsesMergeQueue reports that the stack's base branch merges through a merge + // queue. When set, the wizard skips the merge-method step (the queue picks + // the method), labels the summary "via merge queue", and enqueues instead of + // merging directly. + UsesMergeQueue bool // PreselectTopIndex, when >= 0, preselects PRs[0..PreselectTopIndex] and // skips the PR-selection step (PR-number mode). PreselectTopIndex int diff --git a/internal/tui/mergeview/view.go b/internal/tui/mergeview/view.go index 672d1fd3..e4dd2efc 100644 --- a/internal/tui/mergeview/view.go +++ b/internal/tui/mergeview/view.go @@ -30,7 +30,18 @@ var ( // blends seamlessly into the shading. const stepArrow = "\ue0b0" +// wizardSteps are the selectable stages shown in the top stepper. A merge-queue +// merge has no method to choose, so it uses the shorter two-step sequence. var wizardSteps = []string{"Select PRs", "Select Merge Method", "Confirm"} +var wizardStepsMergeQueue = []string{"Select PRs", "Confirm"} + +// steps returns the stepper labels for the current mode. +func (m Model) steps() []string { + if m.opts.UsesMergeQueue { + return wizardStepsMergeQueue + } + return wizardSteps +} // View implements tea.Model. func (m Model) View() string { @@ -92,9 +103,10 @@ func stepFg(i, cur int) lipgloss.TerminalColor { func (m Model) stepper() string { cur := m.wizardIndex() + steps := m.steps() var b strings.Builder - n := len(wizardSteps) - for i, label := range wizardSteps { + n := len(steps) + for i, label := range steps { bg := stepBg(i, cur) icon := "•" if i < cur { @@ -142,8 +154,20 @@ func powerlineEnabled() bool { } // wizardIndex maps the current step to its position in the stepper. Progress and -// done are past the last selectable step, so all three read as complete. +// done are past the last selectable step, so all steps read as complete. When +// the base branch uses a merge queue there is no method step, so confirm is the +// second (index 1) stage. func (m Model) wizardIndex() int { + if m.opts.UsesMergeQueue { + switch m.step { + case StepSelectPRs: + return 0 + case StepConfirm: + return 1 + default: + return len(m.steps()) + } + } switch m.step { case StepSelectPRs: return 0 @@ -152,7 +176,7 @@ func (m Model) wizardIndex() int { case StepConfirm: return 2 default: - return len(wizardSteps) + return len(m.steps()) } } @@ -217,7 +241,11 @@ func (m Model) viewSelect() string { b.WriteString("\n") if m.topIndex >= 0 { - b.WriteString(mutedStyle.Render(fmt.Sprintf("Will merge %s into %s.", prCount(m.topIndex+1), m.opts.BaseRef))) + summary := fmt.Sprintf("Will merge %s into %s.", prCount(m.topIndex+1), m.opts.BaseRef) + if m.opts.UsesMergeQueue { + summary = fmt.Sprintf("Will merge %s into %s via merge queue.", prCount(m.topIndex+1), m.opts.BaseRef) + } + b.WriteString(mutedStyle.Render(summary)) } else { b.WriteString(faintStyle.Render("Select at least one pull request.")) } @@ -262,19 +290,31 @@ func (m Model) viewConfirm() string { var b strings.Builder nums := m.selectedNumbers() - b.WriteString(fmt.Sprintf("%s into %s with %s.\n", - titleStyle.Render("Merge "+prCount(len(nums))), - accentStyle.Render(m.opts.BaseRef), - accentStyle.Render(methodLabel(m.method)), - )) + if m.opts.UsesMergeQueue { + b.WriteString(fmt.Sprintf("%s into %s via %s.\n", + titleStyle.Render("Merge "+prCount(len(nums))), + accentStyle.Render(m.opts.BaseRef), + accentStyle.Render("merge queue"), + )) + } else { + b.WriteString(fmt.Sprintf("%s into %s with %s.\n", + titleStyle.Render("Merge "+prCount(len(nums))), + accentStyle.Render(m.opts.BaseRef), + accentStyle.Render(methodLabel(m.method)), + )) + } // Wrap the PR list so a long stack isn't cut off at the screen edge. listStyle := numberStyle if m.width > 0 { listStyle = listStyle.Width(m.width) } b.WriteString(listStyle.Render(prNumberList(nums)) + "\n\n") + confirmLabel := "merge" + if m.opts.UsesMergeQueue { + confirmLabel = "enqueue" + } b.WriteString(shortcuts( - [2]string{"enter", "merge"}, + [2]string{"enter", confirmLabel}, [2]string{"shift+tab", "back"}, [2]string{"esc", "cancel"}, )) @@ -285,12 +325,20 @@ func (m Model) viewProgress() string { var b strings.Builder nums := m.selectedNumbers() - b.WriteString(fmt.Sprintf("%s Merging %s into %s via %s\n", - m.spinner.View(), - numberStyle.Render(prNumberList(nums)), - accentStyle.Render(m.opts.BaseRef), - accentStyle.Render(methodLabel(m.method)), - )) + if m.opts.UsesMergeQueue { + b.WriteString(fmt.Sprintf("%s Adding %s to the merge queue for %s\n", + m.spinner.View(), + numberStyle.Render(prNumberList(nums)), + accentStyle.Render(m.opts.BaseRef), + )) + } else { + b.WriteString(fmt.Sprintf("%s Merging %s into %s via %s\n", + m.spinner.View(), + numberStyle.Render(prNumberList(nums)), + accentStyle.Render(m.opts.BaseRef), + accentStyle.Render(methodLabel(m.method)), + )) + } // Always render a status line so it doesn't pop in later and shift the view. b.WriteString(faintStyle.Render(progressStatus(m.message)) + "\n") b.WriteString("\n") From fb48d5228c766998a184fbac47607dbd7221aa93 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 29 Jul 2026 06:16:39 -0400 Subject: [PATCH 10/11] set merge action explicitly --- .github/copilot-instructions.md | 2 +- AGENTS.md | 2 +- cmd/merge.go | 30 ++++++++++++------ cmd/merge_test.go | 49 ++++++++++++++++------------- internal/github/client_interface.go | 2 +- internal/github/merge_async.go | 27 +++++++++++++--- internal/github/merge_async_test.go | 18 +++++------ internal/github/mock_client.go | 6 ++-- 8 files changed, 85 insertions(+), 51 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 120eacbe..8f220c53 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,7 +17,7 @@ No Makefile, no code generation, no external linter config. Standard Go toolchai - `cmd/`: One Cobra command per file. Each exports `Cmd(cfg *config.Config)` with logic in `run()`. - `internal/git/`: `Ops` interface (52 methods) wrapping git CLI. `MockOps` for tests. Package-level functions delegate to swappable `ops` variable. -- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`), which routes to a direct merge or the base branch's merge queue automatically. +- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`) with an explicit `merge_action` (`direct_merge` or `merge_queue`) selected from the base branch's merge-queue detection. - `internal/config/`: `Config` struct passed to all commands. Holds I/O, colors, and test hooks (`SelectFn`, `ConfirmFn`, `InputFn`, `GitHubClientOverride`). - `internal/stack/`: Stack file (`.git/gh-stack`, JSON) management with file locking. - `internal/tui/`: bubbletea views (`stackview`, `modifyview`). diff --git a/AGENTS.md b/AGENTS.md index 11d241fc..efe966fb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -109,7 +109,7 @@ if errors.As(err, &exitErr) { ... } ### Key interfaces - **`git.Ops`** (`internal/git/gitops.go`): 52 methods wrapping git CLI calls. The production implementation uses `cli/go-gh`'s `client.Command()` via `run()` and `runSilent()` helpers. Package-level functions (e.g., `git.CurrentBranch()`) delegate to a swappable package-level `ops` variable. -- **`github.ClientOps`** (`internal/github/client_interface.go`): 17 methods for GitHub API (PRs, stacks, merges). Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`): `ListStacks`, `FindStackForPR`, `GetStack`, `CreateStack`, `AddToStack` (delta append), `Unstack`. Async stack merges use `RepoMergeConfig` (GraphQL: allowed merge methods + viewer's default), `MergeStackAsync`, and `GetAsyncMergeResult` (`/repos/{owner}/{repo}/pulls/{n}/merge-async`). Injected via `cfg.GitHubClientOverride` in tests. +- **`github.ClientOps`** (`internal/github/client_interface.go`): 18 methods for GitHub API (PRs, stacks, merges). Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`): `ListStacks`, `FindStackForPR`, `GetStack`, `CreateStack`, `AddToStack` (delta append), `Unstack`. Async stack merges use `RepoMergeConfig` (GraphQL: allowed merge methods + viewer's default), `BaseBranchUsesMergeQueue` (GraphQL: detects a base-branch merge queue to select the explicit `merge_action`), `MergeStackAsync`, and `GetAsyncMergeResult` (`/repos/{owner}/{repo}/pulls/{n}/merge-async`). Injected via `cfg.GitHubClientOverride` in tests. - **`config.Config`** (`internal/config/config.go`): Central configuration passed to all commands. Holds I/O streams, color functions, and test hook fields (`SelectFn`, `ConfirmFn`, `InputFn`, `RepoOverride`). ### Stack file diff --git a/cmd/merge.go b/cmd/merge.go index d9b9fdc3..be61147b 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -313,7 +313,7 @@ func runMergeInteractive(cfg *config.Config, client github.ClientOps, stackNumbe } } - submit, poll := mergeFuncs(client) + submit, poll := mergeFuncs(client, mergeActionFor(usesMergeQueue)) model := mergeview.New(mergeview.Options{ PRs: candidates, @@ -373,7 +373,7 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, cfg.Printf("Merging %s into %s via %s...", list, base, method) } - res, err := client.MergeStackAsync(targetPR, method) + res, err := client.MergeStackAsync(targetPR, method, mergeActionFor(usesMergeQueue)) if err != nil { if errors.Is(err, github.ErrAsyncMergeUnavailable) { warnAsyncMergeUnavailable(cfg) @@ -439,10 +439,10 @@ func runMergeHeadless(cfg *config.Config, client github.ClientOps, base string, } // baseBranchUsesMergeQueue reports whether the stack's base branch merges through -// a merge queue. Detection only tailors the wizard (skipping the method step and -// switching to enqueue wording), so a lookup failure falls back to the -// direct-merge flow: the async merge always sends merge_action "default" and the -// server still routes to the queue when the branch requires one. +// a merge queue. Detection tailors the wizard (skipping the method step and +// switching to enqueue wording) and selects the explicit merge_action. On a +// lookup failure it falls back to the direct-merge flow (method step shown, +// "direct_merge" sent), matching what the user is shown. func baseBranchUsesMergeQueue(client github.ClientOps, base string) bool { uses, err := client.BaseBranchUsesMergeQueue(base) if err != nil { @@ -451,11 +451,23 @@ func baseBranchUsesMergeQueue(client github.ClientOps, base string) bool { return uses } +// mergeActionFor maps merge-queue detection to the explicit async-merge action: a +// detected queue forces "merge_queue" (the server rejects it when the branch has +// no queue, guarding against a wrong guess), and everything else forces a +// "direct_merge". Being explicit ensures the merge matches the wizard the user +// saw rather than letting the server choose the routing. +func mergeActionFor(usesMergeQueue bool) string { + if usesMergeQueue { + return github.MergeActionMergeQueue + } + return github.MergeActionDirectMerge +} + // mergeFuncs returns submit/poll closures that adapt the GitHub client to the -// mergeview injection points. -func mergeFuncs(client github.ClientOps) (mergeview.SubmitFunc, mergeview.PollFunc) { +// mergeview injection points. The merge action is fixed for the session. +func mergeFuncs(client github.ClientOps, mergeAction string) (mergeview.SubmitFunc, mergeview.PollFunc) { submit := func(targetPR int, method string) (mergeview.MergeStatus, error) { - res, err := client.MergeStackAsync(targetPR, method) + res, err := client.MergeStackAsync(targetPR, method, mergeAction) if err != nil { return mergeview.MergeStatus{}, err } diff --git a/cmd/merge_test.go b/cmd/merge_test.go index 8b38c446..e9e63609 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -91,7 +91,7 @@ func TestRunMerge_NoArg_MergesWholeStack(t *testing.T) { RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { return &github.RepoMergeConfig{MergeAllowed: true, SquashAllowed: true, RebaseAllowed: true, DefaultMethod: "squash"}, nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { gotPR, gotMethod = pr, method return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, @@ -111,14 +111,15 @@ func TestRunMerge_NoArg_MergesWholeStack(t *testing.T) { func TestRunMerge_StackNumberArg(t *testing.T) { var gotPR int + gotAction := "unset" cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ GetStackFn: func(n int) (*github.RemoteStack, error) { assert.Equal(t, 7, n) return remoteStack(7, "main", openStackPR(10, "a"), openStackPR(11, "b")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - gotPR = pr + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { + gotPR, gotAction = pr, mergeAction return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, } @@ -128,11 +129,12 @@ func TestRunMerge_StackNumberArg(t *testing.T) { require.NoError(t, err) assert.Equal(t, 11, gotPR) + assert.Equal(t, github.MergeActionDirectMerge, gotAction, "a non-queue base sends an explicit direct_merge action") assert.Contains(t, output, "Merged #10, #11 into main") } func TestRunMerge_MergeQueue_Headless(t *testing.T) { - gotMethod := "unset" + gotMethod, gotAction := "unset", "unset" cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ GetStackFn: func(n int) (*github.RemoteStack, error) { @@ -142,8 +144,8 @@ func TestRunMerge_MergeQueue_Headless(t *testing.T) { assert.Equal(t, "main", base) return true, nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - gotMethod = method + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { + gotMethod, gotAction = method, mergeAction return &github.AsyncMergeResult{ Status: github.AsyncMergeStatusEnqueued, Details: github.AsyncMergeDetails{Message: "Pull request was added to the merge queue."}, @@ -156,19 +158,20 @@ func TestRunMerge_MergeQueue_Headless(t *testing.T) { require.NoError(t, err) assert.Equal(t, "", gotMethod, "a merge queue picks the method; none is sent") + assert.Equal(t, github.MergeActionMergeQueue, gotAction, "an explicit merge_queue action is sent") assert.Contains(t, output, "merge queue") } func TestRunMerge_MergeQueue_IgnoresMethodFlag(t *testing.T) { - gotMethod := "unset" + gotMethod, gotAction := "unset", "unset" cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(10, "a"), openStackPR(11, "b")), nil }, BaseBranchUsesMergeQueueFn: func(base string) (bool, error) { return true, nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - gotMethod = method + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { + gotMethod, gotAction = method, mergeAction return &github.AsyncMergeResult{ Status: github.AsyncMergeStatusEnqueued, Details: github.AsyncMergeDetails{Message: "queued"}, @@ -183,11 +186,12 @@ func TestRunMerge_MergeQueue_IgnoresMethodFlag(t *testing.T) { require.NoError(t, err) assert.Equal(t, "", gotMethod, "the requested method is ignored under a merge queue") + assert.Equal(t, github.MergeActionMergeQueue, gotAction) assert.Contains(t, output, "ignoring the merge method") } func TestRunMerge_MergeQueueDetectionError_FallsBackToDirect(t *testing.T) { - gotMethod := "unset" + gotMethod, gotAction := "unset", "unset" cfg, outR, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{ GetStackFn: func(n int) (*github.RemoteStack, error) { @@ -199,8 +203,8 @@ func TestRunMerge_MergeQueueDetectionError_FallsBackToDirect(t *testing.T) { RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { return &github.RepoMergeConfig{MergeAllowed: true, DefaultMethod: "merge"}, nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { - gotMethod = method + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { + gotMethod, gotAction = method, mergeAction return &github.AsyncMergeResult{ Status: github.AsyncMergeStatusMerged, Details: github.AsyncMergeDetails{SHA: "abc1234"}, @@ -213,6 +217,7 @@ func TestRunMerge_MergeQueueDetectionError_FallsBackToDirect(t *testing.T) { require.NoError(t, err) assert.Equal(t, "merge", gotMethod, "detection failure falls back to a direct merge with a method") + assert.Equal(t, github.MergeActionDirectMerge, gotAction, "the direct-merge UI sends an explicit direct_merge action") assert.Contains(t, output, "Merged") } @@ -227,7 +232,7 @@ func TestRunMerge_PRNumberArg(t *testing.T) { assert.Equal(t, 2, n) return remoteStack(5, "main", openStackPR(1, "b1"), openStackPR(2, "b2"), openStackPR(3, "b3")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { gotPR = pr return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, @@ -249,7 +254,7 @@ func TestRunMerge_SquashFlag(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { gotMethod = method return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, @@ -367,7 +372,7 @@ func TestRunMerge_WholeStackBlockedByDraft(t *testing.T) { RepoMergeConfigFn: func() (*github.RepoMergeConfig, error) { return &github.RepoMergeConfig{MergeAllowed: true, DefaultMethod: "merge"}, nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { submitCalled = true return nil, nil }, @@ -405,7 +410,7 @@ func TestRunMerge_SubmitNotMergeable(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { return nil, errors.New("the stack can no longer be merged as requested; refresh and try again") }, } @@ -424,7 +429,7 @@ func TestRunMerge_PollFailedConflict(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { @@ -446,7 +451,7 @@ func TestRunMerge_AlreadyMergedOnSubmit(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { return &github.AsyncMergeResult{Status: github.AsyncMergeStatusMerged, Details: github.AsyncMergeDetails{SHA: "abc"}}, nil }, } @@ -464,7 +469,7 @@ func TestRunMerge_Enqueued(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, GetAsyncMergeResultFn: func(pr int, uuid string) (*github.AsyncMergeResult, error) { @@ -485,7 +490,7 @@ func TestRunMerge_EnqueuedOnSubmit(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { return &github.AsyncMergeResult{Status: github.AsyncMergeStatusEnqueued, Details: github.AsyncMergeDetails{Message: "Pull request was added to the merge queue."}}, nil }, } @@ -503,7 +508,7 @@ func TestRunMerge_AsyncMergeUnavailable(t *testing.T) { GetStackFn: func(n int) (*github.RemoteStack, error) { return remoteStack(7, "main", openStackPR(1, "b1"), openStackPR(2, "b2")), nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { return nil, github.ErrAsyncMergeUnavailable }, } @@ -554,7 +559,7 @@ func TestRunMerge_DefaultMethodFallsBackToAllowed(t *testing.T) { // Viewer default is a method the repo no longer allows. return &github.RepoMergeConfig{SquashAllowed: true, DefaultMethod: "merge"}, nil }, - MergeStackAsyncFn: func(pr int, method string) (*github.AsyncMergeResult, error) { + MergeStackAsyncFn: func(pr int, method, mergeAction string) (*github.AsyncMergeResult, error) { gotMethod = method return &github.AsyncMergeResult{Status: github.AsyncMergeStatusPending, Details: github.AsyncMergeDetails{UUID: "u"}}, nil }, diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index b64f60af..c1281ed1 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -18,7 +18,7 @@ type ClientOps interface { AddToStack(stackNumber int, prNumbers []int) (*RemoteStack, error) Unstack(stackNumber int) (*RemoteStack, bool, error) RepoMergeConfig() (*RepoMergeConfig, error) - MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) + MergeStackAsync(prNumber int, method, mergeAction string) (*AsyncMergeResult, error) GetAsyncMergeResult(prNumber int, uuid string) (*AsyncMergeResult, error) PRTitles(numbers []int) (map[int]string, error) BaseBranchUsesMergeQueue(baseRef string) (bool, error) diff --git a/internal/github/merge_async.go b/internal/github/merge_async.go index c2ea4cb4..8393b2b3 100644 --- a/internal/github/merge_async.go +++ b/internal/github/merge_async.go @@ -19,6 +19,18 @@ const ( MergeMethodRebase = "rebase" ) +// Merge action values for the async merge API's merge_action field. "default" +// lets the server choose between a direct merge and the base branch's merge +// queue; "direct_merge" and "merge_queue" force the respective path. Sending an +// explicit action makes the caller's merge-queue detection authoritative: the +// server rejects "merge_queue" on a branch with no queue rather than silently +// merging directly. +const ( + MergeActionDefault = "default" + MergeActionDirectMerge = "direct_merge" + MergeActionMergeQueue = "merge_queue" +) + // ErrAsyncMergeUnavailable indicates the async merge API is not available for // the repository (or the token lacks access). Surfaced on a 404 from the submit // endpoint. @@ -196,21 +208,26 @@ func (c *Client) BaseBranchUsesMergeQueue(baseRef string) (bool, error) { // a stacked PR this merges all members of the stack up to and including // prNumber. A blank method lets the server apply its default. // -// The merge_action is always "default", which lets the server route the stack -// to a direct merge or the base branch's merge queue automatically, based on -// the repository's rules and configuration. +// mergeAction selects the routing: MergeActionDirectMerge or MergeActionMergeQueue +// force the respective path, and MergeActionDefault (also the fallback for an +// empty value) lets the server choose. Sending an explicit action makes the +// caller's merge-queue detection authoritative — the server rejects +// "merge_queue" on a branch with no queue instead of silently merging directly. // // On success the returned result is populated for the 200 (already merged) and // 202 (enqueued for background processing) responses. A 404 returns // ErrAsyncMergeUnavailable, a 409 (a request already exists) returns a clear // "already exists" error, and any other non-2xx status is returned as-is. -func (c *Client) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) { +func (c *Client) MergeStackAsync(prNumber int, method, mergeAction string) (*AsyncMergeResult, error) { + if mergeAction == "" { + mergeAction = MergeActionDefault + } type reqBody struct { MergeMethod string `json:"merge_method,omitempty"` MergeAction string `json:"merge_action"` } - body, err := json.Marshal(reqBody{MergeMethod: method, MergeAction: "default"}) + body, err := json.Marshal(reqBody{MergeMethod: method, MergeAction: mergeAction}) if err != nil { return nil, fmt.Errorf("marshaling request: %w", err) } diff --git a/internal/github/merge_async_test.go b/internal/github/merge_async_test.go index 92c414b2..e0f144bc 100644 --- a/internal/github/merge_async_test.go +++ b/internal/github/merge_async_test.go @@ -53,12 +53,12 @@ func TestMergeStackAsync_Accepted(t *testing.T) { body := `{"status":"pending","details":{"message":"Merge request enqueued.","uuid":"u-1","merge_method":"squash","expected_head_sha":"abc"}}` c := testAsyncClient(t, http.StatusAccepted, body, &rec) - res, err := c.MergeStackAsync(42, "squash") + res, err := c.MergeStackAsync(42, "squash", MergeActionDirectMerge) require.NoError(t, err) assert.Equal(t, http.MethodPut, rec.method) assert.Equal(t, "/repos/o/r/pulls/42/merge-async", rec.path) - assert.JSONEq(t, `{"merge_method":"squash","merge_action":"default"}`, rec.body) + assert.JSONEq(t, `{"merge_method":"squash","merge_action":"direct_merge"}`, rec.body) assert.True(t, res.IsPending()) assert.False(t, res.IsMerged()) @@ -68,7 +68,7 @@ func TestMergeStackAsync_Accepted(t *testing.T) { func TestMergeStackAsync_AlreadyMerged(t *testing.T) { body := `{"status":"merged","details":{"message":"Pull request is already merged.","sha":"deadbeef"}}` - res, err := testAsyncClient(t, http.StatusOK, body, nil).MergeStackAsync(42, "merge") + res, err := testAsyncClient(t, http.StatusOK, body, nil).MergeStackAsync(42, "merge", MergeActionDirectMerge) require.NoError(t, err) assert.True(t, res.IsMerged()) assert.Equal(t, "deadbeef", res.Details.SHA) @@ -77,7 +77,7 @@ func TestMergeStackAsync_AlreadyMerged(t *testing.T) { func TestMergeStackAsync_ExistingRequestConflict(t *testing.T) { // The go-gh REST client discards the 409 body, so we can't recover the // existing UUID; the request surfaces as a clear "already exists" error. - _, err := testAsyncClient(t, http.StatusConflict, `{"status":"pending","details":{"uuid":"u-2"}}`, nil).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusConflict, `{"status":"pending","details":{"uuid":"u-2"}}`, nil).MergeStackAsync(42, "merge", MergeActionDirectMerge) require.Error(t, err) assert.Contains(t, err.Error(), "already exists") } @@ -85,18 +85,18 @@ func TestMergeStackAsync_ExistingRequestConflict(t *testing.T) { func TestMergeStackAsync_NotMergeable(t *testing.T) { // A 400 preflight failure is reported as a clear error (the specific // details.message isn't recoverable through the REST client). - _, err := testAsyncClient(t, http.StatusBadRequest, `{"status":"failed","details":{"message":"Pull request is closed."}}`, nil).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusBadRequest, `{"status":"failed","details":{"message":"Pull request is closed."}}`, nil).MergeStackAsync(42, "merge", MergeActionDirectMerge) require.Error(t, err) assert.Contains(t, err.Error(), "can no longer be merged") } func TestMergeStackAsync_NotAvailable(t *testing.T) { - _, err := testAsyncClient(t, http.StatusNotFound, `{"message":"Not Found"}`, nil).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusNotFound, `{"message":"Not Found"}`, nil).MergeStackAsync(42, "merge", MergeActionDirectMerge) assert.ErrorIs(t, err, ErrAsyncMergeUnavailable) } func TestMergeStackAsync_ValidationFailed(t *testing.T) { - _, err := testAsyncClient(t, http.StatusUnprocessableEntity, `{"message":"Validation Failed"}`, nil).MergeStackAsync(42, "merge") + _, err := testAsyncClient(t, http.StatusUnprocessableEntity, `{"message":"Validation Failed"}`, nil).MergeStackAsync(42, "merge", MergeActionDirectMerge) require.Error(t, err) assert.Contains(t, err.Error(), "Validation Failed") } @@ -178,12 +178,12 @@ func TestAsyncMergeResult_Status(t *testing.T) { // sends merge_action. func TestMergeStackAsync_OmitsEmptyMethod(t *testing.T) { var rec recordedRequest - _, err := testAsyncClient(t, http.StatusAccepted, `{"status":"pending","details":{"message":"m","uuid":"u","merge_method":"merge","merge_action":"default","expected_head_sha":"x"}}`, &rec).MergeStackAsync(1, "") + _, err := testAsyncClient(t, http.StatusAccepted, `{"status":"pending","details":{"message":"m","uuid":"u","merge_method":"merge","merge_action":"default","expected_head_sha":"x"}}`, &rec).MergeStackAsync(1, "", MergeActionMergeQueue) require.NoError(t, err) var parsed map[string]any require.NoError(t, json.Unmarshal([]byte(rec.body), &parsed)) _, hasMethod := parsed["merge_method"] assert.False(t, hasMethod, "merge_method should be omitted when empty") - assert.Equal(t, "default", parsed["merge_action"], "merge_action should always be sent") + assert.Equal(t, "merge_queue", parsed["merge_action"], "the explicit merge_action is always sent") } diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 9a06aaf6..a31dd4ef 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -18,7 +18,7 @@ type MockClient struct { AddToStackFn func(int, []int) (*RemoteStack, error) UnstackFn func(int) (*RemoteStack, bool, error) RepoMergeConfigFn func() (*RepoMergeConfig, error) - MergeStackAsyncFn func(int, string) (*AsyncMergeResult, error) + MergeStackAsyncFn func(int, string, string) (*AsyncMergeResult, error) GetAsyncMergeResultFn func(int, string) (*AsyncMergeResult, error) PRTitlesFn func([]int) (map[int]string, error) BaseBranchUsesMergeQueueFn func(string) (bool, error) @@ -130,9 +130,9 @@ func (m *MockClient) RepoMergeConfig() (*RepoMergeConfig, error) { }, nil } -func (m *MockClient) MergeStackAsync(prNumber int, method string) (*AsyncMergeResult, error) { +func (m *MockClient) MergeStackAsync(prNumber int, method, mergeAction string) (*AsyncMergeResult, error) { if m.MergeStackAsyncFn != nil { - return m.MergeStackAsyncFn(prNumber, method) + return m.MergeStackAsyncFn(prNumber, method, mergeAction) } return &AsyncMergeResult{ Status: AsyncMergeStatusPending, From ff3379369b4fa2ca37a195beb06e9a25d521a092 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 29 Jul 2026 11:55:09 -0400 Subject: [PATCH 11/11] address review comments to clarify docs on merge/api behavior --- .github/copilot-instructions.md | 2 +- README.md | 4 ++-- docs/src/content/docs/guides/workflows.md | 2 +- docs/src/content/docs/reference/cli.md | 4 ++-- skills/gh-stack/SKILL.md | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 8f220c53..36a0e1cb 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -17,7 +17,7 @@ No Makefile, no code generation, no external linter config. Standard Go toolchai - `cmd/`: One Cobra command per file. Each exports `Cmd(cfg *config.Config)` with logic in `run()`. - `internal/git/`: `Ops` interface (52 methods) wrapping git CLI. `MockOps` for tests. Package-level functions delegate to swappable `ops` variable. -- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`) with an explicit `merge_action` (`direct_merge` or `merge_queue`) selected from the base branch's merge-queue detection. +- `internal/github/`: `ClientOps` interface (18 methods) for GitHub API. `MockClient` for tests. Stack operations use the public Stacks REST API (`/repos/{owner}/{repo}/stacks`); merges use the async merge API (`/repos/{owner}/{repo}/pulls/{n}/merge-async`) with an explicit `merge_action` (`direct_merge` or `merge_queue`) chosen from the base branch's merge-queue detection. `merge_action` is optional — omitting it (or sending `default`) lets the server auto-route (merge queue if one is configured, else direct merge) — but the CLI sends it explicitly so a wrong detection fails loudly instead of silently merging directly. - `internal/config/`: `Config` struct passed to all commands. Holds I/O, colors, and test hooks (`SelectFn`, `ConfirmFn`, `InputFn`, `GitHubClientOverride`). - `internal/stack/`: Stack file (`.git/gh-stack`, JSON) management with file locking. - `internal/tui/`: bubbletea views (`stackview`, `modifyview`). diff --git a/README.md b/README.md index 1ed0392f..f680c87c 100644 --- a/README.md +++ b/README.md @@ -451,11 +451,11 @@ All members of the stack up to and including your chosen pull request are merged With no argument, the current active local stack is used. Pass a stack number to merge a stack you don't have checked out (a purely remote operation), or a pull request number to merge directly up to that PR. -In an interactive terminal, a short wizard walks you through three steps — choose which PRs to merge, pick the merge method, and confirm. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. +In an interactive terminal, a short wizard walks you through choosing which PRs to merge, picking the merge method, and confirming. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Bypassing merge requirements is not supported** for stacked PR merges. -If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it's merged directly. +If the base branch uses a merge queue, the stack is added to the queue instead of merging directly. The queue chooses the merge method, so the wizard skips the method step and any `--merge-method` (or `--squash`/`--rebase`/`--merge`) flag is ignored with a warning. The selected pull requests are added to the queue together but merge as the queue processes them — they may land in separate groups rather than all at once. | Flag | Description | |------|-------------| diff --git a/docs/src/content/docs/guides/workflows.md b/docs/src/content/docs/guides/workflows.md index 96f574e9..26e4f1f4 100644 --- a/docs/src/content/docs/guides/workflows.md +++ b/docs/src/content/docs/guides/workflows.md @@ -143,7 +143,7 @@ gh stack merge --yes --squash In an interactive terminal, a short wizard lets you choose how far up the stack to merge, pick the merge method (only the ones your repository allows, defaulting to your last-used method), and confirm — then shows live progress. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting. After merging, run `gh stack sync` to update your local branches. -If the base branch uses a merge queue, `gh stack merge` adds the stack to the queue instead of merging directly — it merges once the queue processes it. +If the base branch uses a merge queue, `gh stack merge` adds the stack to the queue instead of merging directly. The queue chooses the merge method, so the wizard skips the method step and any merge method you pass (for example `--squash`) is ignored with a warning. The selected pull requests are added to the queue together but merge as the queue processes them — they may land in separate groups rather than all at once. :::note[Bypassing merge requirements not supported] Stack merges do not support bypassing merge requirements. diff --git a/docs/src/content/docs/reference/cli.md b/docs/src/content/docs/reference/cli.md index b8723ddb..44369a7d 100644 --- a/docs/src/content/docs/reference/cli.md +++ b/docs/src/content/docs/reference/cli.md @@ -472,11 +472,11 @@ All members of the stack up to and including your chosen pull request are merged With no argument, the current active local stack is used. Pass a stack number to merge a stack you don't have checked out (a purely remote operation), or a pull request number to merge directly up to that PR. -In an interactive terminal, a short wizard walks you through three steps — choose which PRs to merge, pick the merge method, and confirm. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. +In an interactive terminal, a short wizard walks you through choosing which PRs to merge, picking the merge method, and confirming. In a non-interactive terminal, or with `--yes`, the whole stack (or everything up to the given PR) is merged without prompting, using your last-used merge method unless one is specified. Only basic pull request state is checked before merging (open and not a draft); GitHub evaluates branch protection and repository rules when the merge runs, so any such failure is reported back to you. **Bypassing merge requirements is not supported** for stacked PR merges. -If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it's merged directly. +If the base branch uses a merge queue, the stack is added to the queue instead of merging directly. The queue chooses the merge method, so the wizard skips the method step and any `--merge-method` (or `--squash`/`--rebase`/`--merge`) flag is ignored with a warning. The selected pull requests are added to the queue together but merge as the queue processes them — they may land in separate groups rather than all at once. | Flag | Description | |------|-------------| diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 831c046d..3f8a60d6 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -61,7 +61,7 @@ git config remote.pushDefault origin # if multiple remotes exist (skips remo 7. **Use standard `git add` and `git commit` for staging and committing.** This gives you full control over which changes go into each branch. The `-Am` shortcut is available but should not be the default approach—stacked PRs are most effective when each branch contains a deliberate, logical set of changes. 8. **Navigate down the stack when you need to change a lower layer.** If you're working on a frontend branch and realize you need API changes, don't hack around it at the current layer. Navigate to the appropriate branch (`gh stack down`, `gh stack checkout`, or `gh stack bottom`), make and commit the changes there, run `gh stack rebase --upstack`, then navigate back up to continue. 9. **Use `gh stack link` for external tool workflows.** When branches are managed by an external tool (jj, Sapling, etc.), use `gh stack link branch-a branch-b`. `link` does not rely on local tracking state and is intended for API-driven PR and stack management. Provide at least two branches/PRs to create or update a stack, or a stack number followed by the new branches/PRs to append them to the top of an existing stack (e.g. `gh stack link 7 branch-c`). -10. **Use `gh stack merge --yes` to merge stacked PRs.** `gh pr merge` does not work with stacked PRs. In a non-interactive terminal `gh stack merge` runs without prompting and merges the entire stack (bottom to top) atomically; pass `--yes` to be explicit. Scope the merge by passing a pull request number (`gh stack merge 42 --yes` merges everything up to and including PR #42) or a stack number (`gh stack merge 7 --yes`, which needs no local checkout). Choose the method with `--squash`, `--rebase`, `--merge`, or `--merge-method `; without one, the last-used method is used. The merge is all-or-nothing — if any PR can't be merged, none are, and the failure reason is reported. Only basic pull request state is checked before merging (open and not a draft); bypassing merge requirements is not supported for stacks. If the base branch uses a merge queue, the stack is added to the queue and merges once the queue processes it; otherwise it's merged directly. +10. **Use `gh stack merge --yes` to merge stacked PRs.** `gh pr merge` does not work with stacked PRs. In a non-interactive terminal `gh stack merge` runs without prompting and merges the entire stack (bottom to top) atomically; pass `--yes` to be explicit. Scope the merge by passing a pull request number (`gh stack merge 42 --yes` merges everything up to and including PR #42) or a stack number (`gh stack merge 7 --yes`, which needs no local checkout). Choose the method with `--squash`, `--rebase`, `--merge`, or `--merge-method `; without one, the last-used method is used. The merge is all-or-nothing — if any PR can't be merged, none are, and the failure reason is reported. Only basic pull request state is checked before merging (open and not a draft); bypassing merge requirements is not supported for stacks. If the base branch uses a merge queue, the stack is added to the queue instead of merging directly: the queue chooses the merge method (any method you pass is ignored with a warning), and the pull requests are added to the queue together but merge as the queue processes them, so they may land in separate groups rather than all at once. **Never do any of the following — each triggers an interactive prompt or TUI that will hang:** - ❌ `gh stack view` or `gh stack view --short` — always use `gh stack view --json`