From 056116fe7c1d8b8c62f2b4bb82fb4eaec4c77103 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 22 Jul 2026 00:07:01 -0400 Subject: [PATCH 1/7] 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 daee05f7d86008af14ef592a766abd47ab5b1cd2 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Wed, 22 Jul 2026 00:20:15 -0400 Subject: [PATCH 2/7] 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 f2fe1e1047dd614bdb7fb529ef0171100b3fa54b Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 07:08:27 -0400 Subject: [PATCH 3/7] 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 d4a3e3fd868e0992f729d8cb8a3140aabb096ddd Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 07:56:13 -0400 Subject: [PATCH 4/7] 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 028a5764bf6a79e82bd763731fc736373d7e6c55 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 18:18:19 -0400 Subject: [PATCH 5/7] 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 b55df82e..3fe24519 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 a102281e..8d4a07c8 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 8143627a..fb3a3b16 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 a09c25dd..b0962ef8 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 f0ec7d5ccfee61b72d374f5f3d3e48fe668e82e5 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 23 Jul 2026 18:32:49 -0400 Subject: [PATCH 6/7] 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 5847fdd1930990f7108419cd1ff6cf2e338ecd99 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Sun, 26 Jul 2026 15:03:48 -0400 Subject: [PATCH 7/7] 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 3fe24519..de6ffaae 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 8d4a07c8..5a083dfe 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 fb3a3b16..525cf605 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 b0962ef8..bd8dde07 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` @@ -885,6 +885,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.