From d4adefe23822e09cd5a790df186af60c6f1790e9 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Tue, 31 Mar 2026 10:33:02 -0400 Subject: [PATCH 1/7] create stack api --- README.md | 4 +- cmd/push.go | 65 ++++++++-- cmd/push_test.go | 194 ++++++++++++++++++++++++++++ internal/github/client_interface.go | 1 + internal/github/github.go | 28 ++++ internal/github/mock_client.go | 8 ++ skills/gh-stack/SKILL.md | 1 + 7 files changed, 291 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 103cdbd..e3ac7e3 100644 --- a/README.md +++ b/README.md @@ -55,7 +55,7 @@ main (trunk) The **bottom** of the stack is the branch closest to the trunk, and the **top** is the branch furthest from it. Each branch inherits from the one below it. Navigation commands (`up`, `down`, `top`, `bottom`) follow this model: `up` moves away from trunk, `down` moves toward it. -When you push, `gh stack` creates one PR per branch. Each PR's base is set to the branch below it in the stack, so reviewers see only the diff for that layer. +When you push, `gh stack` creates one PR per branch and links them together as a **Stack** on GitHub. Each PR's base is set to the branch below it in the stack, so reviewers see only the diff for that layer. ### Local tracking @@ -261,6 +261,8 @@ gh stack push [flags] Pushes every branch to the remote, then for each branch either creates a new PR (with the correct base branch) or updates the base of an existing PR if it has changed. Uses `--force-with-lease` by default to safely update rebased branches. +After creating PRs, `push` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous push), it is left as-is — updating existing stacks will be supported in an upcoming release. + When creating new PRs, you will be prompted to enter a title for each one. Press Enter to accept the default (branch name), or use `--auto` to skip prompting entirely. | Flag | Description | diff --git a/cmd/push.go b/cmd/push.go index 5e47f74..b0aef15 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -3,8 +3,10 @@ package cmd import ( "errors" "fmt" + "strconv" "strings" + "github.com/cli/go-gh/v2/pkg/api" "github.com/cli/go-gh/v2/pkg/prompter" "github.com/github/gh-stack/internal/config" "github.com/github/gh-stack/internal/git" @@ -165,15 +167,8 @@ func runPush(cfg *config.Config, opts *pushOptions) error { } } - // TODO: Add PRs to a stack - // - // We can call an API after all the individual PRs are created/updated to create the stack at once, - // or we can add a flag to the existing PR API to incrementally build the stack. - // - // For now, the PRs are pushed and created individually but are NOT linked as a formal stack on GitHub. - cfg.Warningf("Stacked PRs is not yet implemented — PRs were created individually.") - fmt.Fprintf(cfg.Err, " Once the GitHub Stacks API is available, PRs will be automatically\n") - fmt.Fprintf(cfg.Err, " grouped into a Stack.\n") + // Create or update the stack on GitHub + createStack(cfg, client, s) // Update base commit hashes and sync PR state updateBaseSHAs(s) @@ -261,3 +256,55 @@ func pickRemote(cfg *config.Config, branch, remoteOverride string) (string, erro } return multi.Remotes[selected], nil } + +// createStack attempts to create a stack on GitHub from the active PRs. +// It is a best-effort operation: failures are reported as warnings but do +// not cause the push command to fail (the PRs are already created). +func createStack(cfg *config.Config, client interface{ CreateStack([]int) (int, error) }, s *stack.Stack) { + // Collect PR numbers in stack order (bottom to top). + var prNumbers []int + for _, b := range s.Branches { + if b.IsMerged() { + continue + } + if b.PullRequest != nil { + prNumbers = append(prNumbers, b.PullRequest.Number) + } + } + + // The API requires at least 2 PRs to form a stack. + if len(prNumbers) < 2 { + return + } + + stackID, err := client.CreateStack(prNumbers) + if err == nil { + s.ID = strconv.Itoa(stackID) + cfg.Successf("Stack created on GitHub with %d PRs", len(prNumbers)) + return + } + + cfg.Warningf("Failed to create stack on GitHub: %v", err) + var httpErr *api.HTTPError + if !errors.As(err, &httpErr) { + cfg.Warningf("Failed to create stack on GitHub: %v", err) + return + } + + cfg.Warningf("error %s with code %s", httpErr.StatusCode, httpErr.Message) + switch httpErr.StatusCode { + case 422: + if s.ID != "" { + // Stack was already created in a previous push; the update API + // (PUT) is needed to modify it but is not yet available. + cfg.Infof("Stack already exists on GitHub") + } else { + cfg.Warningf("Could not create stack: %s", httpErr.Message) + cfg.Printf(" Updating existing stacks will be supported in an upcoming release.") + } + case 404: + cfg.Warningf("Stacked PRs are not enabled for this repository") + default: + cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message) + } +} diff --git a/cmd/push_test.go b/cmd/push_test.go index 8a38a8d..a9cc1d7 100644 --- a/cmd/push_test.go +++ b/cmd/push_test.go @@ -3,8 +3,10 @@ package cmd import ( "fmt" "io" + "net/url" "testing" + "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" @@ -225,3 +227,195 @@ func TestPush_Humanize(t *testing.T) { }) } } + +func TestCreateStack_Success(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var gotNumbers []int + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + gotNumbers = prNumbers + return 42, nil + }, + } + + cfg, _, errR := config.NewTestConfig() + createStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Equal(t, []int{10, 11}, gotNumbers) + assert.Equal(t, "42", s.ID) + assert.Contains(t, output, "Stack created on GitHub with 2 PRs") +} + +func TestCreateStack_AlreadyExists_WithLocalID(t *testing.T) { + s := &stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests are already in a stack", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + createStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "Stack already exists on GitHub") + assert.Equal(t, "99", s.ID) +} + +func TestCreateStack_AlreadyExists_NoLocalID(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests are already in a stack", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + createStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "Could not create stack") + assert.Contains(t, output, "Updating existing stacks will be supported in an upcoming release") +} + +func TestCreateStack_NotAvailable(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 404, + Message: "Not Found", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + createStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "not yet available") +} + +func TestCreateStack_SkippedForSinglePR(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + }, + } + + called := false + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + called = true + return 42, nil + }, + } + + cfg, _, _ := config.NewTestConfig() + createStack(cfg, mock, s) + cfg.Err.Close() + + assert.False(t, called, "CreateStack should not be called with fewer than 2 PRs") +} + +func TestCreateStack_SkipsMergedBranches(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var gotNumbers []int + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + gotNumbers = prNumbers + return 42, nil + }, + } + + cfg, _, _ := config.NewTestConfig() + createStack(cfg, mock, s) + cfg.Err.Close() + + assert.Equal(t, []int{11, 12}, gotNumbers, "should only include non-merged PRs") +} + +func TestCreateStack_SkipsBranchesWithoutPR(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, // no PR yet + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var gotNumbers []int + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + gotNumbers = prNumbers + return 42, nil + }, + } + + cfg, _, _ := config.NewTestConfig() + createStack(cfg, mock, s) + cfg.Err.Close() + + assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs") +} diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index 99af071..bcbb7d6 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -8,6 +8,7 @@ type ClientOps interface { FindAnyPRForBranch(branch string) (*PullRequest, error) FindPRDetailsForBranch(branch string) (*PRDetails, error) CreatePR(base, head, title, body string, draft bool) (*PullRequest, error) + CreateStack(prNumbers []int) (int, error) DeleteStack() error } diff --git a/internal/github/github.go b/internal/github/github.go index 73b6126..6e3937e 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -1,6 +1,8 @@ package github import ( + "bytes" + "encoding/json" "fmt" "github.com/cli/go-gh/v2/pkg/api" @@ -270,6 +272,32 @@ func (c *Client) DeleteStack() error { return fmt.Errorf("deleting a stack on GitHub is not yet supported by the API") } +// CreateStack creates a stack on GitHub from an ordered list of PR numbers. +// The PR numbers must be ordered from bottom to top of the stack and must +// form a valid base-to-head chain. Returns the server-assigned stack ID. +func (c *Client) CreateStack(prNumbers []int) (int, error) { + type createStackRequest struct { + PullRequestNumbers []int `json:"pull_request_numbers"` + } + + body, err := json.Marshal(createStackRequest{PullRequestNumbers: prNumbers}) + if err != nil { + return 0, fmt.Errorf("marshaling request: %w", err) + } + + path := fmt.Sprintf("repos/%s/%s/cli_internal/pulls/stacks", c.owner, c.repo) + + var response struct { + ID int `json:"id"` + } + + if err := c.rest.Post(path, bytes.NewReader(body), &response); err != nil { + return 0, err + } + + return response.ID, nil +} + func (c *Client) repositoryID() (string, error) { var query struct { Repository struct { diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 5b59177..569f1ec 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -10,6 +10,7 @@ type MockClient struct { FindAnyPRForBranchFn func(string) (*PullRequest, error) FindPRDetailsForBranchFn func(string) (*PRDetails, error) CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) + CreateStackFn func([]int) (int, error) DeleteStackFn func() error } @@ -50,3 +51,10 @@ func (m *MockClient) DeleteStack() error { } return fmt.Errorf("deleting a stack on GitHub is not yet supported by the API") } + +func (m *MockClient) CreateStack(prNumbers []int) (int, error) { + if m.CreateStackFn != nil { + return m.CreateStackFn(prNumbers) + } + return 0, nil +} diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 4dd113a..6fa2569 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -489,6 +489,7 @@ gh stack push --skip-prs - Pushes all active (non-merged) branches atomically (`--force-with-lease --atomic`) - Creates a new PR for each branch that doesn't have one (base set to the first non-merged ancestor branch) +- After creating PRs, links them together as a **Stack** on GitHub (requires the repository to have stacks enabled) - Syncs PR metadata for branches that already have PRs **PR title auto-generation (`--auto`):** From 891f51f9437efa8b32795c9990f93a9906d41ea3 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 3 Apr 2026 01:50:08 -0400 Subject: [PATCH 2/7] add to stack api --- cmd/push.go | 81 ++++++++++++--- cmd/push_test.go | 152 +++++++++++++++++++++++----- internal/github/client_interface.go | 1 + internal/github/github.go | 23 +++++ internal/github/mock_client.go | 14 ++- 5 files changed, 230 insertions(+), 41 deletions(-) diff --git a/cmd/push.go b/cmd/push.go index b0aef15..a2f8ed9 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -10,6 +10,7 @@ import ( "github.com/cli/go-gh/v2/pkg/prompter" "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/spf13/cobra" ) @@ -168,7 +169,7 @@ func runPush(cfg *config.Config, opts *pushOptions) error { } // Create or update the stack on GitHub - createStack(cfg, client, s) + syncStack(cfg, client, s) // Update base commit hashes and sync PR state updateBaseSHAs(s) @@ -257,10 +258,13 @@ func pickRemote(cfg *config.Config, branch, remoteOverride string) (string, erro return multi.Remotes[selected], nil } -// createStack attempts to create a stack on GitHub from the active PRs. -// It is a best-effort operation: failures are reported as warnings but do +// syncStack creates or updates a stack on GitHub from the active PRs. +// If the stack already exists (s.ID is set), it calls the PUT endpoint with +// the full list of PRs to keep the remote stack in sync. If no stack exists +// yet, it calls POST to create one. +// This is a best-effort operation: failures are reported as warnings but do // not cause the push command to fail (the PRs are already created). -func createStack(cfg *config.Config, client interface{ CreateStack([]int) (int, error) }, s *stack.Stack) { +func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) { // Collect PR numbers in stack order (bottom to top). var prNumbers []int for _, b := range s.Branches { @@ -277,6 +281,38 @@ func createStack(cfg *config.Config, client interface{ CreateStack([]int) (int, return } + if s.ID != "" { + updateStack(cfg, client, s, prNumbers) + } else { + createNewStack(cfg, client, s, prNumbers) + } +} + +// updateStack calls the PUT endpoint to sync the full PR list for an existing stack. +func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { + if err := client.UpdateStack(s.ID, prNumbers); err != nil { + var httpErr *api.HTTPError + if errors.As(err, &httpErr) { + switch httpErr.StatusCode { + case 404: + cfg.Warningf("Stack not found on GitHub (ID %s) — it may have been deleted", s.ID) + cfg.Printf(" Run %s to re-create the stack on your next push.", + cfg.ColorCyan("gh stack push")) + s.ID = "" // Clear stale ID so the next push creates a new stack + default: + cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message) + } + } else { + cfg.Warningf("Failed to update stack on GitHub: %v", err) + } + return + } + cfg.Successf("Stack updated on GitHub with %d PRs", len(prNumbers)) +} + +// createNewStack calls the POST endpoint to create a new stack, handling the +// three types of 422 errors the API may return. +func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { stackID, err := client.CreateStack(prNumbers) if err == nil { s.ID = strconv.Itoa(stackID) @@ -284,27 +320,42 @@ func createStack(cfg *config.Config, client interface{ CreateStack([]int) (int, return } - cfg.Warningf("Failed to create stack on GitHub: %v", err) var httpErr *api.HTTPError if !errors.As(err, &httpErr) { cfg.Warningf("Failed to create stack on GitHub: %v", err) return } - cfg.Warningf("error %s with code %s", httpErr.StatusCode, httpErr.Message) switch httpErr.StatusCode { case 422: - if s.ID != "" { - // Stack was already created in a previous push; the update API - // (PUT) is needed to modify it but is not yet available. - cfg.Infof("Stack already exists on GitHub") - } else { - cfg.Warningf("Could not create stack: %s", httpErr.Message) - cfg.Printf(" Updating existing stacks will be supported in an upcoming release.") - } + handleCreate422(cfg, httpErr) case 404: - cfg.Warningf("Stacked PRs are not enabled for this repository") + cfg.Warningf("Stacked PRs are not yet available for this repository") default: cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message) } } + +// handleCreate422 handles 422 errors from the create stack endpoint. +// The three known error messages are: +// - "Stack must contain at least two pull requests" +// - "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref" +// - "Pull requests #123, #124, #125 are already stacked" +func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) { + msg := httpErr.Message + + if strings.Contains(msg, "already stacked") { + cfg.Warningf("%s", msg) + cfg.Printf(" The stack already exists on GitHub. Updating existing stacks will be supported in an upcoming release.") + return + } + + if strings.Contains(msg, "must form a stack") { + cfg.Errorf("Cannot create stack: %s", msg) + cfg.Printf(" Each PR's base branch must match the previous PR's head branch.") + return + } + + // "at least two" or any other validation error + cfg.Warningf("Could not create stack: %s", msg) +} diff --git a/cmd/push_test.go b/cmd/push_test.go index a9cc1d7..2d4263f 100644 --- a/cmd/push_test.go +++ b/cmd/push_test.go @@ -228,7 +228,7 @@ func TestPush_Humanize(t *testing.T) { } } -func TestCreateStack_Success(t *testing.T) { +func TestSyncStack_NewStack_CreateSuccess(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -246,7 +246,7 @@ func TestCreateStack_Success(t *testing.T) { } cfg, _, errR := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() errOut, _ := io.ReadAll(errR) @@ -257,9 +257,109 @@ func TestCreateStack_Success(t *testing.T) { assert.Contains(t, output, "Stack created on GitHub with 2 PRs") } -func TestCreateStack_AlreadyExists_WithLocalID(t *testing.T) { +func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) { s := &stack.Stack{ ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var gotStackID string + var gotNumbers []int + createCalled := false + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + UpdateStackFn: func(stackID string, prNumbers []int) error { + gotStackID = stackID + gotNumbers = prNumbers + return nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.False(t, createCalled, "CreateStack should not be called when s.ID is set") + assert.Equal(t, "99", gotStackID) + assert.Equal(t, []int{10, 11, 12}, gotNumbers) + assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") +} + +func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) { + s := &stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + UpdateStackFn: func(string, []int) error { + return &api.HTTPError{ + StatusCode: 422, + Message: "Validation failed", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "Failed to update stack") +} + +func TestSyncStack_ExistingStack_Update404(t *testing.T) { + s := &stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + UpdateStackFn: func(string, []int) error { + return &api.HTTPError{ + StatusCode: 404, + Message: "Not Found", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "Stack not found on GitHub") + assert.Contains(t, output, "may have been deleted") + assert.Equal(t, "", s.ID, "stale ID should be cleared so next push creates a new stack") +} + +func TestSyncStack_AlreadyStacked_422(t *testing.T) { + s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, @@ -271,24 +371,24 @@ func TestCreateStack_AlreadyExists_WithLocalID(t *testing.T) { CreateStackFn: func([]int) (int, error) { return 0, &api.HTTPError{ StatusCode: 422, - Message: "Pull requests are already in a stack", + Message: "Pull requests #10, #11 are already stacked", RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, } }, } cfg, _, errR := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() errOut, _ := io.ReadAll(errR) output := string(errOut) - assert.Contains(t, output, "Stack already exists on GitHub") - assert.Equal(t, "99", s.ID) + assert.Contains(t, output, "already stacked") + assert.Contains(t, output, "Updating existing stacks will be supported in an upcoming release") } -func TestCreateStack_AlreadyExists_NoLocalID(t *testing.T) { +func TestSyncStack_InvalidChain_422(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -301,24 +401,24 @@ func TestCreateStack_AlreadyExists_NoLocalID(t *testing.T) { CreateStackFn: func([]int) (int, error) { return 0, &api.HTTPError{ StatusCode: 422, - Message: "Pull requests are already in a stack", + Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, } }, } cfg, _, errR := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() errOut, _ := io.ReadAll(errR) output := string(errOut) - assert.Contains(t, output, "Could not create stack") - assert.Contains(t, output, "Updating existing stacks will be supported in an upcoming release") + assert.Contains(t, output, "must form a stack") + assert.Contains(t, output, "base branch must match") } -func TestCreateStack_NotAvailable(t *testing.T) { +func TestSyncStack_NotAvailable(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -338,7 +438,7 @@ func TestCreateStack_NotAvailable(t *testing.T) { } cfg, _, errR := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() errOut, _ := io.ReadAll(errR) @@ -347,7 +447,7 @@ func TestCreateStack_NotAvailable(t *testing.T) { assert.Contains(t, output, "not yet available") } -func TestCreateStack_SkippedForSinglePR(t *testing.T) { +func TestSyncStack_SkippedForSinglePR(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -355,22 +455,28 @@ func TestCreateStack_SkippedForSinglePR(t *testing.T) { }, } - called := false + createCalled := false + updateCalled := false mock := &github.MockClient{ CreateStackFn: func([]int) (int, error) { - called = true + createCalled = true return 42, nil }, + UpdateStackFn: func(string, []int) error { + updateCalled = true + return nil + }, } cfg, _, _ := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() - assert.False(t, called, "CreateStack should not be called with fewer than 2 PRs") + assert.False(t, createCalled, "CreateStack should not be called with fewer than 2 PRs") + assert.False(t, updateCalled, "UpdateStack should not be called with fewer than 2 PRs") } -func TestCreateStack_SkipsMergedBranches(t *testing.T) { +func TestSyncStack_SkipsMergedBranches(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -389,13 +495,13 @@ func TestCreateStack_SkipsMergedBranches(t *testing.T) { } cfg, _, _ := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() assert.Equal(t, []int{11, 12}, gotNumbers, "should only include non-merged PRs") } -func TestCreateStack_SkipsBranchesWithoutPR(t *testing.T) { +func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -414,7 +520,7 @@ func TestCreateStack_SkipsBranchesWithoutPR(t *testing.T) { } cfg, _, _ := config.NewTestConfig() - createStack(cfg, mock, s) + syncStack(cfg, mock, s) cfg.Err.Close() assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs") diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index bcbb7d6..d64b698 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -9,6 +9,7 @@ type ClientOps interface { FindPRDetailsForBranch(branch string) (*PRDetails, error) CreatePR(base, head, title, body string, draft bool) (*PullRequest, error) CreateStack(prNumbers []int) (int, error) + UpdateStack(stackID string, prNumbers []int) error DeleteStack() error } diff --git a/internal/github/github.go b/internal/github/github.go index 6e3937e..e9acb5c 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -298,6 +298,29 @@ func (c *Client) CreateStack(prNumbers []int) (int, error) { return response.ID, nil } +// UpdateStack adds pull requests to an existing stack on GitHub. +// The stack is identified by stackID. The full list of PR numbers in the +// updated stack must be provided, including existing and new PRs, ordered +// from bottom to top. +func (c *Client) UpdateStack(stackID string, prNumbers []int) error { + type updateStackRequest struct { + PullRequestNumbers []int `json:"pull_request_numbers"` + } + + body, err := json.Marshal(updateStackRequest{PullRequestNumbers: prNumbers}) + if err != nil { + return fmt.Errorf("marshaling request: %w", err) + } + + path := fmt.Sprintf("repos/%s/%s/cli_internal/pulls/stacks/%s", c.owner, c.repo, stackID) + + var response struct { + ID int `json:"id"` + } + + return c.rest.Put(path, bytes.NewReader(body), &response) +} + func (c *Client) repositoryID() (string, error) { var query struct { Repository struct { diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 569f1ec..786ac06 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -9,9 +9,10 @@ type MockClient struct { FindPRForBranchFn func(string) (*PullRequest, error) FindAnyPRForBranchFn func(string) (*PullRequest, error) FindPRDetailsForBranchFn func(string) (*PRDetails, error) - CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) - CreateStackFn func([]int) (int, error) - DeleteStackFn func() error + CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) + CreateStackFn func([]int) (int, error) + UpdateStackFn func(string, []int) error + DeleteStackFn func() error } // Compile-time check that MockClient satisfies ClientOps. @@ -58,3 +59,10 @@ func (m *MockClient) CreateStack(prNumbers []int) (int, error) { } return 0, nil } + +func (m *MockClient) UpdateStack(stackID string, prNumbers []int) error { + if m.UpdateStackFn != nil { + return m.UpdateStackFn(stackID, prNumbers) + } + return nil +} From d4967d8d04ba3313578956fa07dfc68acbe72a44 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 3 Apr 2026 03:29:06 -0400 Subject: [PATCH 3/7] submit command --- README.md | 48 +++- cmd/init.go | 2 +- cmd/merge.go | 2 +- cmd/merge_test.go | 2 +- cmd/push.go | 248 ++--------------- cmd/push_test.go | 433 ++++-------------------------- cmd/rebase.go | 4 +- cmd/root.go | 1 + cmd/submit.go | 321 ++++++++++++++++++++++ cmd/submit_test.go | 565 +++++++++++++++++++++++++++++++++++++++ skills/gh-stack/SKILL.md | 59 ++-- 11 files changed, 1043 insertions(+), 642 deletions(-) create mode 100644 cmd/submit.go create mode 100644 cmd/submit_test.go diff --git a/README.md b/README.md index e3ac7e3..732d02d 100644 --- a/README.md +++ b/README.md @@ -34,11 +34,14 @@ gh stack add auth-layer gh stack add api-endpoints # ... make commits ... -# Push all branches and create/update PRs +# Push all branches gh stack push # View the stack gh stack view + +# Open a stack of PRs +gh stack submit ``` ## How it works @@ -253,15 +256,36 @@ gh stack sync ### `gh stack push` -Push all branches in the current stack and create or update pull requests. +Push all branches in the current stack to the remote. ``` gh stack push [flags] ``` -Pushes every branch to the remote, then for each branch either creates a new PR (with the correct base branch) or updates the base of an existing PR if it has changed. Uses `--force-with-lease` by default to safely update rebased branches. +Pushes every branch to the remote using `--force-with-lease --atomic`. This is a lightweight wrapper around `git push` that knows about all branches in the stack. It does not create or update pull requests — use `gh stack submit` for that. + +| Flag | Description | +|------|-------------| +| `--remote ` | Remote to push to (defaults to auto-detected remote) | + +**Examples:** + +```sh +gh stack push +gh stack push --remote upstream +``` + +### `gh stack submit` + +Push all branches and create/update PRs and the stack on GitHub. + +``` +gh stack submit [flags] +``` + +Creates a Stacked PR for every branch in the stack, pushing branches to remote if needed. -After creating PRs, `push` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous push), it is left as-is — updating existing stacks will be supported in an upcoming release. +After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs will be added to the top of the stack. When creating new PRs, you will be prompted to enter a title for each one. Press Enter to accept the default (branch name), or use `--auto` to skip prompting entirely. @@ -269,16 +293,14 @@ When creating new PRs, you will be prompted to enter a title for each one. Press |------|-------------| | `--auto` | Use auto-generated PR titles without prompting | | `--draft` | Create new PRs as drafts | -| `--skip-prs` | Push branches without creating or updating PRs | | `--remote ` | Remote to push to (defaults to auto-detected remote) | **Examples:** ```sh -gh stack push -gh stack push --auto -gh stack push --draft -gh stack push --skip-prs +gh stack submit +gh stack submit --auto +gh stack submit --draft ``` ### `gh stack view` @@ -401,8 +423,8 @@ gh stack add auth-middleware gh stack add api-routes # ... write code, make commits ... -# 4. Push everything and create PRs -gh stack push +# 4. Push everything and create Stacked PRs +gh stack submit # 5. Reviewer requests changes on the first PR gh stack bottom @@ -411,7 +433,7 @@ gh stack bottom # 6. Rebase the rest of the stack on top of your fix gh stack rebase -# 7. Push the updated stack +# 7. Push the updated branches gh stack push # 8. When the first PR is merged, sync the stack @@ -452,7 +474,7 @@ gh stack add -Am "Frontend components" # → feat/02 already has commits, creates feat/03 and commits there # 7. Push everything and create PRs -gh stack push +gh stack submit ``` Compared to the typical workflow, there's no need to name branches, run `git add`, or run `git commit` separately. Each `gh stack add -Am "..."` does it all. diff --git a/cmd/init.go b/cmd/init.go index 7c002d2..8ae7db4 100644 --- a/cmd/init.go +++ b/cmd/init.go @@ -351,7 +351,7 @@ func runInit(cfg *config.Config, opts *initOptions) error { } cfg.Printf("To add a new layer to your stack, run `%s`", cfg.ColorCyan("gh stack add")) - cfg.Printf("When you're ready to push to GitHub and open a stack of PRs, run `%s`", cfg.ColorCyan("gh stack push")) + cfg.Printf("When you're ready to push to GitHub and open a stack of PRs, run `%s`", cfg.ColorCyan("gh stack submit")) return nil } diff --git a/cmd/merge.go b/cmd/merge.go index ce5bf8e..b882551 100644 --- a/cmd/merge.go +++ b/cmd/merge.go @@ -69,7 +69,7 @@ func runMerge(cfg *config.Config, target string) error { if br.PullRequest == nil { cfg.Errorf("no pull request found for branch %q", br.Branch) - cfg.Printf(" Run %s to create PRs for this stack.", cfg.ColorCyan("gh stack push")) + cfg.Printf(" Run %s to create PRs for this stack.", cfg.ColorCyan("gh stack submit")) return ErrSilent } diff --git a/cmd/merge_test.go b/cmd/merge_test.go index e065ebe..bb59b94 100644 --- a/cmd/merge_test.go +++ b/cmd/merge_test.go @@ -43,7 +43,7 @@ func TestMerge_NoPullRequest(t *testing.T) { assert.ErrorIs(t, err, ErrSilent) assert.Contains(t, output, "no pull request found") - assert.Contains(t, output, "gh stack push") + assert.Contains(t, output, "gh stack submit") } func TestMerge_AlreadyMerged(t *testing.T) { diff --git a/cmd/push.go b/cmd/push.go index a2f8ed9..cee0e91 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -3,23 +3,16 @@ package cmd import ( "errors" "fmt" - "strconv" - "strings" - "github.com/cli/go-gh/v2/pkg/api" "github.com/cli/go-gh/v2/pkg/prompter" "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/spf13/cobra" ) type pushOptions struct { - auto bool - draft bool - skipPRs bool - remote string + remote string } func PushCmd(cfg *config.Config) *cobra.Command { @@ -27,15 +20,12 @@ func PushCmd(cfg *config.Config) *cobra.Command { cmd := &cobra.Command{ Use: "push", - Short: "Push all branches in the current stack and create/update PRs", + Short: "Push all branches in the current stack to the remote", RunE: func(cmd *cobra.Command, args []string) error { return runPush(cfg, opts) }, } - cmd.Flags().BoolVar(&opts.auto, "auto", false, "Use auto-generated PR titles without prompting") - cmd.Flags().BoolVar(&opts.draft, "draft", false, "Create PRs as drafts") - cmd.Flags().BoolVar(&opts.skipPRs, "skip-prs", false, "Push branches without creating or updating PRs") cmd.Flags().StringVar(&opts.remote, "remote", "", "Remote to push to (defaults to auto-detected remote)") return cmd @@ -73,12 +63,6 @@ func runPush(cfg *config.Config, opts *pushOptions) error { } s := stacks[0] - client, err := cfg.GitHubClient() - if err != nil { - cfg.Errorf("failed to create GitHub client: %s", err) - return ErrAPIFailure - } - // Push all active branches atomically remote, err := pickRemote(cfg, currentBranch, opts.remote) if err != nil { @@ -98,128 +82,28 @@ func runPush(cfg *config.Config, opts *pushOptions) error { return ErrSilent } - if opts.skipPRs { - cfg.Successf("Pushed %d branches (PR creation skipped)", len(s.ActiveBranches())) - return nil - } - - // Create or update PRs - for i, b := range s.Branches { - if s.Branches[i].IsMerged() { - continue - } - baseBranch := s.ActiveBaseBranch(b.Branch) - - pr, err := client.FindPRForBranch(b.Branch) - if err != nil { - cfg.Warningf("failed to check PR for %s: %v", b.Branch, err) - continue - } - - if pr == nil { - // Create new PR — auto-generate title from commits/branch name, - // then prompt interactively unless --auto or non-interactive. - baseBranchForDiff := s.ActiveBaseBranch(b.Branch) - title, commitBody := defaultPRTitleBody(baseBranchForDiff, b.Branch) - originalTitle := title - if !opts.auto && cfg.IsInteractive() { - p := prompter.New(cfg.In, cfg.Out, cfg.Err) - input, err := p.Input(fmt.Sprintf("Title for PR (branch %s):", b.Branch), title) - if err != nil { - if isInterruptError(err) { - printInterrupt(cfg) - return ErrSilent - } - // Non-interrupt error: keep the auto-generated title. - } else if input != "" { - title = input - } - } - - // If the user changed the title and the commit had a multi-line - // message, put the full commit message in the PR body so no - // content is lost. - prBody := commitBody - if title != originalTitle && commitBody != "" { - prBody = originalTitle + "\n\n" + commitBody - } - body := generatePRBody(prBody) - - newPR, createErr := client.CreatePR(baseBranch, b.Branch, title, body, opts.draft) - if createErr != nil { - cfg.Warningf("failed to create PR for %s: %v", b.Branch, createErr) - continue - } - cfg.Successf("Created PR %s for %s", cfg.PRLink(newPR.Number, newPR.URL), b.Branch) - s.Branches[i].PullRequest = &stack.PullRequestRef{ - Number: newPR.Number, - ID: newPR.ID, - URL: newPR.URL, - } - } else { - cfg.Printf("PR %s for %s is up to date", cfg.PRLink(pr.Number, pr.URL), b.Branch) - if s.Branches[i].PullRequest == nil { - s.Branches[i].PullRequest = &stack.PullRequestRef{ - Number: pr.Number, - ID: pr.ID, - URL: pr.URL, - } - } - } - } - - // Create or update the stack on GitHub - syncStack(cfg, client, s) - - // Update base commit hashes and sync PR state + // Update base commit hashes after push updateBaseSHAs(s) - syncStackPRs(cfg, s) if err := stack.Save(gitDir, sf); err != nil { return handleSaveError(cfg, err) } - cfg.Successf("Pushed and synced %d branches", len(s.ActiveBranches())) - return nil -} + cfg.Successf("Pushed %d branches", len(activeBranches)) -// defaultPRTitleBody generates a PR title and body from the branch's commits. -// If there is exactly one commit, use its subject as the title and its body -// (if any) as the PR body. Otherwise, humanize the branch name for the title. -func defaultPRTitleBody(base, head string) (string, string) { - commits, err := git.LogRange(base, head) - if err == nil && len(commits) == 1 { - return commits[0].Subject, strings.TrimSpace(commits[0].Body) + // Hint about submit only if there are branches without PRs + hasBranchWithoutPR := false + for _, b := range s.ActiveBranches() { + if b.PullRequest == nil { + hasBranchWithoutPR = true + break + } } - return humanize(head), "" -} - -// generatePRBody builds a PR description from the commit body (if any) -// and a footer linking to the CLI and feedback form. -func generatePRBody(commitBody string) string { - var parts []string - - if commitBody != "" { - parts = append(parts, commitBody) + if hasBranchWithoutPR { + cfg.Printf("To create PRs for this stack, run `%s`", + cfg.ColorCyan("gh stack submit")) } - - footer := fmt.Sprintf( - "Stack created with GitHub Stacks CLI • Give Feedback 💬", - feedbackBaseURL, - ) - parts = append(parts, footer) - - return strings.Join(parts, "\n\n---\n\n") -} - -// humanize replaces hyphens and underscores with spaces. -func humanize(s string) string { - return strings.Map(func(r rune) rune { - if r == '-' || r == '_' { - return ' ' - } - return r - }, s) + return nil } // pickRemote determines which remote to push to. If remoteOverride is @@ -257,105 +141,3 @@ func pickRemote(cfg *config.Config, branch, remoteOverride string) (string, erro } return multi.Remotes[selected], nil } - -// syncStack creates or updates a stack on GitHub from the active PRs. -// If the stack already exists (s.ID is set), it calls the PUT endpoint with -// the full list of PRs to keep the remote stack in sync. If no stack exists -// yet, it calls POST to create one. -// This is a best-effort operation: failures are reported as warnings but do -// not cause the push command to fail (the PRs are already created). -func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) { - // Collect PR numbers in stack order (bottom to top). - var prNumbers []int - for _, b := range s.Branches { - if b.IsMerged() { - continue - } - if b.PullRequest != nil { - prNumbers = append(prNumbers, b.PullRequest.Number) - } - } - - // The API requires at least 2 PRs to form a stack. - if len(prNumbers) < 2 { - return - } - - if s.ID != "" { - updateStack(cfg, client, s, prNumbers) - } else { - createNewStack(cfg, client, s, prNumbers) - } -} - -// updateStack calls the PUT endpoint to sync the full PR list for an existing stack. -func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { - if err := client.UpdateStack(s.ID, prNumbers); err != nil { - var httpErr *api.HTTPError - if errors.As(err, &httpErr) { - switch httpErr.StatusCode { - case 404: - cfg.Warningf("Stack not found on GitHub (ID %s) — it may have been deleted", s.ID) - cfg.Printf(" Run %s to re-create the stack on your next push.", - cfg.ColorCyan("gh stack push")) - s.ID = "" // Clear stale ID so the next push creates a new stack - default: - cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message) - } - } else { - cfg.Warningf("Failed to update stack on GitHub: %v", err) - } - return - } - cfg.Successf("Stack updated on GitHub with %d PRs", len(prNumbers)) -} - -// createNewStack calls the POST endpoint to create a new stack, handling the -// three types of 422 errors the API may return. -func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { - stackID, err := client.CreateStack(prNumbers) - if err == nil { - s.ID = strconv.Itoa(stackID) - cfg.Successf("Stack created on GitHub with %d PRs", len(prNumbers)) - return - } - - var httpErr *api.HTTPError - if !errors.As(err, &httpErr) { - cfg.Warningf("Failed to create stack on GitHub: %v", err) - return - } - - switch httpErr.StatusCode { - case 422: - handleCreate422(cfg, httpErr) - case 404: - cfg.Warningf("Stacked PRs are not yet available for this repository") - default: - cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message) - } -} - -// handleCreate422 handles 422 errors from the create stack endpoint. -// The three known error messages are: -// - "Stack must contain at least two pull requests" -// - "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref" -// - "Pull requests #123, #124, #125 are already stacked" -func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) { - msg := httpErr.Message - - if strings.Contains(msg, "already stacked") { - cfg.Warningf("%s", msg) - cfg.Printf(" The stack already exists on GitHub. Updating existing stacks will be supported in an upcoming release.") - return - } - - if strings.Contains(msg, "must form a stack") { - cfg.Errorf("Cannot create stack: %s", msg) - cfg.Printf(" Each PR's base branch must match the previous PR's head branch.") - return - } - - // "at least two" or any other validation error - cfg.Warningf("Could not create stack: %s", msg) -} diff --git a/cmd/push_test.go b/cmd/push_test.go index 2d4263f..371b857 100644 --- a/cmd/push_test.go +++ b/cmd/push_test.go @@ -3,10 +3,8 @@ package cmd import ( "fmt" "io" - "net/url" "testing" - "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" @@ -15,42 +13,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestGeneratePRBody(t *testing.T) { - tests := []struct { - name string - commitBody string - wantContains []string - }{ - { - name: "empty commit body", - commitBody: "", - wantContains: []string{ - "GitHub Stacks CLI", - feedbackBaseURL, - "", - }, - }, - { - name: "with commit body", - commitBody: "This is a detailed description\nof the change.", - wantContains: []string{ - "This is a detailed description\nof the change.", - "GitHub Stacks CLI", - "", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := generatePRBody(tt.commitBody) - for _, want := range tt.wantContains { - assert.Contains(t, got, want) - } - }) - } -} - // newPushMock creates a MockOps pre-configured for push tests. func newPushMock(tmpDir string, currentBranch string) *git.MockOps { return &git.MockOps{ @@ -61,7 +23,7 @@ func newPushMock(tmpDir string, currentBranch string) *git.MockOps { } } -func TestPush_SkipPRs(t *testing.T) { +func TestPush_PushesAllBranches(t *testing.T) { s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -87,13 +49,13 @@ func TestPush_SkipPRs(t *testing.T) { cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{} cmd := PushCmd(cfg) - cmd.SetArgs([]string{"--skip-prs"}) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) err := cmd.Execute() cfg.Err.Close() - _, _ = io.ReadAll(errR) + errOut, _ := io.ReadAll(errR) + output := string(errOut) assert.NoError(t, err) require.Len(t, pushCalls, 1) @@ -101,6 +63,40 @@ func TestPush_SkipPRs(t *testing.T) { assert.Equal(t, []string{"b1", "b2"}, pushCalls[0].branches) assert.True(t, pushCalls[0].force) assert.True(t, pushCalls[0].atomic) + assert.Contains(t, output, "Pushed 2 branches") + assert.Contains(t, output, "gh stack submit", "should hint about submit when branches have no PRs") +} + +func TestPush_NoSubmitHintWhenPRsExist(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + mock := newPushMock(tmpDir, "b1") + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{} + cmd := PushCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.Contains(t, output, "Pushed 2 branches") + assert.NotContains(t, output, "gh stack submit", "should not hint about submit when all branches have PRs") } func TestPush_SkipsMergedBranches(t *testing.T) { @@ -130,7 +126,6 @@ func TestPush_SkipsMergedBranches(t *testing.T) { cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{} cmd := PushCmd(cfg) - cmd.SetArgs([]string{"--skip-prs"}) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) err := cmd.Execute() @@ -165,7 +160,6 @@ func TestPush_PushFailure(t *testing.T) { cfg, _, errR := config.NewTestConfig() cfg.GitHubClientOverride = &github.MockClient{} cmd := PushCmd(cfg) - cmd.SetArgs([]string{"--skip-prs"}) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) err := cmd.Execute() @@ -178,350 +172,39 @@ func TestPush_PushFailure(t *testing.T) { assert.Contains(t, output, "failed to push") } -func TestPush_DefaultPRTitleBody(t *testing.T) { - t.Run("single_commit", func(t *testing.T) { - restore := git.SetOps(&git.MockOps{ - LogRangeFn: func(base, head string) ([]git.CommitInfo, error) { - return []git.CommitInfo{ - {Subject: "Add login page", Body: "Implements the OAuth flow"}, - }, nil - }, - }) - defer restore() - - title, body := defaultPRTitleBody("main", "feat-login") - assert.Equal(t, "Add login page", title) - assert.Equal(t, "Implements the OAuth flow", body) - }) - - t.Run("multiple_commits", func(t *testing.T) { - restore := git.SetOps(&git.MockOps{ - LogRangeFn: func(base, head string) ([]git.CommitInfo, error) { - return []git.CommitInfo{ - {Subject: "First commit"}, - {Subject: "Second commit"}, - }, nil - }, - }) - defer restore() - - title, body := defaultPRTitleBody("main", "my-feature") - assert.Equal(t, "my feature", title) - assert.Equal(t, "", body) - }) -} - -func TestPush_Humanize(t *testing.T) { - tests := []struct { - input string - want string - }{ - {"my-branch", "my branch"}, - {"my_branch", "my branch"}, - {"nobranch", "nobranch"}, - } - - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, humanize(tt.input)) - }) - } -} - -func TestSyncStack_NewStack_CreateSuccess(t *testing.T) { - s := &stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - }, - } - - var gotNumbers []int - mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { - gotNumbers = prNumbers - return 42, nil - }, - } - - cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Equal(t, []int{10, 11}, gotNumbers) - assert.Equal(t, "42", s.ID) - assert.Contains(t, output, "Stack created on GitHub with 2 PRs") -} - -func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) { - s := &stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, - }, - } - - var gotStackID string - var gotNumbers []int - createCalled := false - mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - createCalled = true - return 0, nil - }, - UpdateStackFn: func(stackID string, prNumbers []int) error { - gotStackID = stackID - gotNumbers = prNumbers - return nil - }, - } - - cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.False(t, createCalled, "CreateStack should not be called when s.ID is set") - assert.Equal(t, "99", gotStackID) - assert.Equal(t, []int{10, 11, 12}, gotNumbers) - assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") -} - -func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) { - s := &stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - }, - } - - mock := &github.MockClient{ - UpdateStackFn: func(string, []int) error { - return &api.HTTPError{ - StatusCode: 422, - Message: "Validation failed", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, - } - }, - } - - cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Contains(t, output, "Failed to update stack") -} - -func TestSyncStack_ExistingStack_Update404(t *testing.T) { - s := &stack.Stack{ - ID: "99", - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - }, - } - - mock := &github.MockClient{ - UpdateStackFn: func(string, []int) error { - return &api.HTTPError{ - StatusCode: 404, - Message: "Not Found", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, - } - }, - } - - cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Contains(t, output, "Stack not found on GitHub") - assert.Contains(t, output, "may have been deleted") - assert.Equal(t, "", s.ID, "stale ID should be cleared so next push creates a new stack") -} - -func TestSyncStack_AlreadyStacked_422(t *testing.T) { - s := &stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - }, - } - - mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ - StatusCode: 422, - Message: "Pull requests #10, #11 are already stacked", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, - } - }, - } - - cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Contains(t, output, "already stacked") - assert.Contains(t, output, "Updating existing stacks will be supported in an upcoming release") -} - -func TestSyncStack_InvalidChain_422(t *testing.T) { - s := &stack.Stack{ +func TestPush_DoesNotCreatePRs(t *testing.T) { + s := stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - }, - } - - mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ - StatusCode: 422, - Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, - } + {Branch: "b1"}, + {Branch: "b2"}, }, } - cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Contains(t, output, "must form a stack") - assert.Contains(t, output, "base branch must match") -} + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) -func TestSyncStack_NotAvailable(t *testing.T) { - s := &stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - }, - } + mock := newPushMock(tmpDir, "b1") - mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - return 0, &api.HTTPError{ - StatusCode: 404, - Message: "Not Found", - RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, - } - }, - } + restore := git.SetOps(mock) + defer restore() + createPRCalled := false cfg, _, errR := config.NewTestConfig() - syncStack(cfg, mock, s) - - cfg.Err.Close() - errOut, _ := io.ReadAll(errR) - output := string(errOut) - - assert.Contains(t, output, "not yet available") -} - -func TestSyncStack_SkippedForSinglePR(t *testing.T) { - s := &stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - }, - } - - createCalled := false - updateCalled := false - mock := &github.MockClient{ - CreateStackFn: func([]int) (int, error) { - createCalled = true - return 42, nil - }, - UpdateStackFn: func(string, []int) error { - updateCalled = true - return nil - }, - } - - cfg, _, _ := config.NewTestConfig() - syncStack(cfg, mock, s) - cfg.Err.Close() - - assert.False(t, createCalled, "CreateStack should not be called with fewer than 2 PRs") - assert.False(t, updateCalled, "UpdateStack should not be called with fewer than 2 PRs") -} - -func TestSyncStack_SkipsMergedBranches(t *testing.T) { - s := &stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, - {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, - {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, - }, - } - - var gotNumbers []int - mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { - gotNumbers = prNumbers - return 42, nil - }, - } - - cfg, _, _ := config.NewTestConfig() - syncStack(cfg, mock, s) - cfg.Err.Close() - - assert.Equal(t, []int{11, 12}, gotNumbers, "should only include non-merged PRs") -} - -func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { - s := &stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{ - {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2"}, // no PR yet - {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, - }, - } - - var gotNumbers []int - mock := &github.MockClient{ - CreateStackFn: func(prNumbers []int) (int, error) { - gotNumbers = prNumbers - return 42, nil + cfg.GitHubClientOverride = &github.MockClient{ + CreatePRFn: func(string, string, string, string, bool) (*github.PullRequest, error) { + createPRCalled = true + return nil, nil }, } + cmd := PushCmd(cfg) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() - cfg, _, _ := config.NewTestConfig() - syncStack(cfg, mock, s) cfg.Err.Close() + _, _ = io.ReadAll(errR) - assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs") + assert.NoError(t, err) + assert.False(t, createPRCalled, "push should not create PRs") } diff --git a/cmd/rebase.go b/cmd/rebase.go index b9f8d52..1515040 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -325,7 +325,7 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { cfg.Printf("%s rebased locally with %s", rangeDesc, s.Trunk.Branch) cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`", - cfg.ColorCyan("gh stack push")) + cfg.ColorCyan("gh stack submit")) return nil } @@ -500,7 +500,7 @@ func continueRebase(cfg *config.Config, gitDir string) error { cfg.Printf("All branches in stack rebased locally with %s", s.Trunk.Branch) cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`", - cfg.ColorCyan("gh stack push")) + cfg.ColorCyan("gh stack submit")) return nil } diff --git a/cmd/root.go b/cmd/root.go index 7426e1c..704fe1e 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -33,6 +33,7 @@ func RootCmd() *cobra.Command { // Remote operations root.AddCommand(CheckoutCmd(cfg)) root.AddCommand(PushCmd(cfg)) + root.AddCommand(SubmitCmd(cfg)) root.AddCommand(SyncCmd(cfg)) root.AddCommand(UnstackCmd(cfg)) root.AddCommand(MergeCmd(cfg)) diff --git a/cmd/submit.go b/cmd/submit.go new file mode 100644 index 0000000..ca0cd63 --- /dev/null +++ b/cmd/submit.go @@ -0,0 +1,321 @@ +package cmd + +import ( + "errors" + "fmt" + "strconv" + "strings" + + "github.com/cli/go-gh/v2/pkg/api" + "github.com/cli/go-gh/v2/pkg/prompter" + "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/spf13/cobra" +) + +type submitOptions struct { + auto bool + draft bool + remote string +} + +func SubmitCmd(cfg *config.Config) *cobra.Command { + opts := &submitOptions{} + + cmd := &cobra.Command{ + Use: "submit", + Short: "Create a stack of PRs on GitHub", + RunE: func(cmd *cobra.Command, args []string) error { + return runSubmit(cfg, opts) + }, + } + + cmd.Flags().BoolVar(&opts.auto, "auto", false, "Use auto-generated PR titles without prompting") + cmd.Flags().BoolVar(&opts.draft, "draft", false, "Create PRs as drafts") + cmd.Flags().StringVar(&opts.remote, "remote", "", "Remote to push to (defaults to auto-detected remote)") + + return cmd +} + +func runSubmit(cfg *config.Config, opts *submitOptions) error { + gitDir, err := git.GitDir() + if err != nil { + cfg.Errorf("not a git repository") + return ErrNotInStack + } + + sf, err := stack.Load(gitDir) + if err != nil { + cfg.Errorf("failed to load stack state: %s", err) + return ErrNotInStack + } + + currentBranch, err := git.CurrentBranch() + if err != nil { + cfg.Errorf("failed to get current branch: %s", err) + return ErrNotInStack + } + + // Find the stack for the current branch without switching branches. + // Submit should never change the user's checked-out branch. + stacks := sf.FindAllStacksForBranch(currentBranch) + if len(stacks) == 0 { + cfg.Errorf("current branch %q is not part of a stack", currentBranch) + return ErrNotInStack + } + if len(stacks) > 1 { + cfg.Errorf("branch %q belongs to multiple stacks; checkout a non-trunk branch first", currentBranch) + return ErrDisambiguate + } + s := stacks[0] + + client, err := cfg.GitHubClient() + if err != nil { + cfg.Errorf("failed to create GitHub client: %s", err) + return ErrAPIFailure + } + + // Push all active branches atomically + remote, err := pickRemote(cfg, currentBranch, opts.remote) + if err != nil { + if !errors.Is(err, errInterrupt) { + cfg.Errorf("%s", err) + } + return ErrSilent + } + merged := s.MergedBranches() + if len(merged) > 0 { + cfg.Printf("Skipping %d merged %s", len(merged), plural(len(merged), "branch", "branches")) + } + activeBranches := activeBranchNames(s) + cfg.Printf("Pushing %d %s to %s...", len(activeBranches), plural(len(activeBranches), "branch", "branches"), remote) + if err := git.Push(remote, activeBranches, true, true); err != nil { + cfg.Errorf("failed to push: %s", err) + return ErrSilent + } + + // Create or update PRs + for i, b := range s.Branches { + if s.Branches[i].IsMerged() { + continue + } + baseBranch := s.ActiveBaseBranch(b.Branch) + + pr, err := client.FindPRForBranch(b.Branch) + if err != nil { + cfg.Warningf("failed to check PR for %s: %v", b.Branch, err) + continue + } + + if pr == nil { + // Create new PR — auto-generate title from commits/branch name, + // then prompt interactively unless --auto or non-interactive. + baseBranchForDiff := s.ActiveBaseBranch(b.Branch) + title, commitBody := defaultPRTitleBody(baseBranchForDiff, b.Branch) + originalTitle := title + if !opts.auto && cfg.IsInteractive() { + p := prompter.New(cfg.In, cfg.Out, cfg.Err) + input, err := p.Input(fmt.Sprintf("Title for PR (branch %s):", b.Branch), title) + if err != nil { + if isInterruptError(err) { + printInterrupt(cfg) + return ErrSilent + } + // Non-interrupt error: keep the auto-generated title. + } else if input != "" { + title = input + } + } + + // If the user changed the title and the commit had a multi-line + // message, put the full commit message in the PR body so no + // content is lost. + prBody := commitBody + if title != originalTitle && commitBody != "" { + prBody = originalTitle + "\n\n" + commitBody + } + body := generatePRBody(prBody) + + newPR, createErr := client.CreatePR(baseBranch, b.Branch, title, body, opts.draft) + if createErr != nil { + cfg.Warningf("failed to create PR for %s: %v", b.Branch, createErr) + continue + } + cfg.Successf("Created PR %s for %s", cfg.PRLink(newPR.Number, newPR.URL), b.Branch) + s.Branches[i].PullRequest = &stack.PullRequestRef{ + Number: newPR.Number, + ID: newPR.ID, + URL: newPR.URL, + } + } else { + cfg.Printf("PR %s for %s is up to date", cfg.PRLink(pr.Number, pr.URL), b.Branch) + if s.Branches[i].PullRequest == nil { + s.Branches[i].PullRequest = &stack.PullRequestRef{ + Number: pr.Number, + ID: pr.ID, + URL: pr.URL, + } + } + } + } + + // Create or update the stack on GitHub + syncStack(cfg, client, s) + + // Update base commit hashes and sync PR state + updateBaseSHAs(s) + syncStackPRs(cfg, s) + + if err := stack.Save(gitDir, sf); err != nil { + return handleSaveError(cfg, err) + } + + cfg.Successf("Pushed and synced %d branches", len(s.ActiveBranches())) + return nil +} + +// defaultPRTitleBody generates a PR title and body from the branch's commits. +// If there is exactly one commit, use its subject as the title and its body +// (if any) as the PR body. Otherwise, humanize the branch name for the title. +func defaultPRTitleBody(base, head string) (string, string) { + commits, err := git.LogRange(base, head) + if err == nil && len(commits) == 1 { + return commits[0].Subject, strings.TrimSpace(commits[0].Body) + } + return humanize(head), "" +} + +// generatePRBody builds a PR description from the commit body (if any) +// and a footer linking to the CLI and feedback form. +func generatePRBody(commitBody string) string { + var parts []string + + if commitBody != "" { + parts = append(parts, commitBody) + } + + footer := fmt.Sprintf( + "Stack created with GitHub Stacks CLI • Give Feedback 💬", + feedbackBaseURL, + ) + parts = append(parts, footer) + + return strings.Join(parts, "\n\n---\n\n") +} + +// humanize replaces hyphens and underscores with spaces. +func humanize(s string) string { + return strings.Map(func(r rune) rune { + if r == '-' || r == '_' { + return ' ' + } + return r + }, s) +} + +// syncStack creates or updates a stack on GitHub from the active PRs. +// If the stack already exists (s.ID is set), it calls the PUT endpoint with +// the full list of PRs to keep the remote stack in sync. If no stack exists +// yet, it calls POST to create one. +// This is a best-effort operation: failures are reported as warnings but do +// not cause the submit command to fail (the PRs are already created). +func syncStack(cfg *config.Config, client github.ClientOps, s *stack.Stack) { + // Collect PR numbers in stack order (bottom to top). + var prNumbers []int + for _, b := range s.Branches { + if b.IsMerged() { + continue + } + if b.PullRequest != nil { + prNumbers = append(prNumbers, b.PullRequest.Number) + } + } + + // The API requires at least 2 PRs to form a stack. + if len(prNumbers) < 2 { + return + } + + if s.ID != "" { + updateStack(cfg, client, s, prNumbers) + } else { + createNewStack(cfg, client, s, prNumbers) + } +} + +// updateStack calls the PUT endpoint to sync the full PR list for an existing stack. +// If the remote stack was deleted (404), it clears the local ID and falls through +// to createNewStack so the user doesn't need to re-run the command. +func updateStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { + if err := client.UpdateStack(s.ID, prNumbers); err != nil { + var httpErr *api.HTTPError + if errors.As(err, &httpErr) { + switch httpErr.StatusCode { + case 404: + // Stack was deleted on GitHub — clear the stale ID and + // immediately try to re-create it. + s.ID = "" + createNewStack(cfg, client, s, prNumbers) + default: + cfg.Warningf("Failed to update stack on GitHub: %s", httpErr.Message) + } + } else { + cfg.Warningf("Failed to update stack on GitHub: %v", err) + } + return + } + cfg.Successf("Stack updated on GitHub with %d PRs", len(prNumbers)) +} + +// createNewStack calls the POST endpoint to create a new stack, handling the +// three types of 422 errors the API may return. +func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, prNumbers []int) { + stackID, err := client.CreateStack(prNumbers) + if err == nil { + s.ID = strconv.Itoa(stackID) + cfg.Successf("Stack created on GitHub with %d PRs", len(prNumbers)) + return + } + + var httpErr *api.HTTPError + if !errors.As(err, &httpErr) { + cfg.Warningf("Failed to create stack on GitHub: %v", err) + return + } + + switch httpErr.StatusCode { + case 422: + handleCreate422(cfg, httpErr) + case 404: + cfg.Warningf("Stacked PRs are not yet available for this repository") + default: + cfg.Warningf("Failed to create stack on GitHub: %s", httpErr.Message) + } +} + +// handleCreate422 handles 422 errors from the create stack endpoint. +// The three known error messages are: +// - "Stack must contain at least two pull requests" +// - "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref" +// - "Pull requests #123, #124, #125 are already stacked" +func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) { + msg := httpErr.Message + + if strings.Contains(msg, "already stacked") { + cfg.Errorf("One or more PRs are already part of an existing stack on GitHub") + cfg.Printf(" To fix this, unstack the PRs from the web, then `%s`", + cfg.ColorCyan("gh stack submit")) + return + } + + if strings.Contains(msg, "must form a stack") { + cfg.Errorf("Cannot create stack: %s", msg) + cfg.Printf(" Each PR's base branch must match the previous PR's head branch.") + return + } + + // "at least two" or any other validation error + cfg.Warningf("Could not create stack: %s", msg) +} diff --git a/cmd/submit_test.go b/cmd/submit_test.go new file mode 100644 index 0000000..c9d80e0 --- /dev/null +++ b/cmd/submit_test.go @@ -0,0 +1,565 @@ +package cmd + +import ( + "fmt" + "io" + "net/url" + "testing" + + "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/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGeneratePRBody(t *testing.T) { + tests := []struct { + name string + commitBody string + wantContains []string + }{ + { + name: "empty commit body", + commitBody: "", + wantContains: []string{ + "GitHub Stacks CLI", + feedbackBaseURL, + "", + }, + }, + { + name: "with commit body", + commitBody: "This is a detailed description\nof the change.", + wantContains: []string{ + "This is a detailed description\nof the change.", + "GitHub Stacks CLI", + "", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := generatePRBody(tt.commitBody) + for _, want := range tt.wantContains { + assert.Contains(t, got, want) + } + }) + } +} + +// newSubmitMock creates a MockOps pre-configured for submit tests. +func newSubmitMock(tmpDir string, currentBranch string) *git.MockOps { + return &git.MockOps{ + GitDirFn: func() (string, error) { return tmpDir, nil }, + CurrentBranchFn: func() (string, error) { return currentBranch, nil }, + ResolveRemoteFn: func(string) (string, error) { return "origin", nil }, + PushFn: func(string, []string, bool, bool) error { return nil }, + } +} + +func TestSubmit_CreatesPRsAndStack(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + {Branch: "b2"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var pushCalls []pushCall + var createdPRs []string + + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + mock.LogRangeFn = func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{{Subject: "commit for " + head}}, nil + } + + restore := git.SetOps(mock) + defer restore() + + prCounter := 100 + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + return nil, nil // No existing PR + }, + CreatePRFn: func(base, head, title, body string, draft bool) (*github.PullRequest, error) { + createdPRs = append(createdPRs, head) + prCounter++ + return &github.PullRequest{ + Number: prCounter, + ID: fmt.Sprintf("PR_%d", prCounter), + URL: fmt.Sprintf("https://github.com/owner/repo/pull/%d", prCounter), + }, nil + }, + CreateStackFn: func(prNumbers []int) (int, error) { + return 42, nil + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + + // Branches should be pushed + require.Len(t, pushCalls, 1) + assert.Equal(t, "origin", pushCalls[0].remote) + assert.Equal(t, []string{"b1", "b2"}, pushCalls[0].branches) + + // PRs should be created + assert.Equal(t, []string{"b1", "b2"}, createdPRs) + + // Stack should be created + assert.Contains(t, output, "Stack created on GitHub with 2 PRs") + assert.Contains(t, output, "Pushed and synced 2 branches") +} + +func TestSubmit_PushFailure(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1"}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + mock := newSubmitMock(tmpDir, "b1") + mock.PushFn = func(string, []string, bool, bool) error { + return fmt.Errorf("remote rejected") + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{} + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.ErrorIs(t, err, ErrSilent) + assert.Contains(t, output, "failed to push") +} + +func TestSubmit_SkipsMergedBranches(t *testing.T) { + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 1, Merged: true}}, + {Branch: "b2"}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 3, Merged: true}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + var pushCalls []pushCall + + mock := newSubmitMock(tmpDir, "b2") + mock.PushFn = func(remote string, branches []string, force, atomic bool) error { + pushCalls = append(pushCalls, pushCall{remote, branches, force, atomic}) + return nil + } + + restore := git.SetOps(mock) + defer restore() + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + return &github.PullRequest{Number: 2, URL: "https://github.com/owner/repo/pull/2"}, nil + }, + } + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + _, _ = io.ReadAll(errR) + + assert.NoError(t, err) + require.Len(t, pushCalls, 1) + assert.Equal(t, []string{"b2"}, pushCalls[0].branches) +} + +func TestSubmit_DefaultPRTitleBody(t *testing.T) { + t.Run("single_commit", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + LogRangeFn: func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{ + {Subject: "Add login page", Body: "Implements the OAuth flow"}, + }, nil + }, + }) + defer restore() + + title, body := defaultPRTitleBody("main", "feat-login") + assert.Equal(t, "Add login page", title) + assert.Equal(t, "Implements the OAuth flow", body) + }) + + t.Run("multiple_commits", func(t *testing.T) { + restore := git.SetOps(&git.MockOps{ + LogRangeFn: func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{ + {Subject: "First commit"}, + {Subject: "Second commit"}, + }, nil + }, + }) + defer restore() + + title, body := defaultPRTitleBody("main", "my-feature") + assert.Equal(t, "my feature", title) + assert.Equal(t, "", body) + }) +} + +func TestSubmit_Humanize(t *testing.T) { + tests := []struct { + input string + want string + }{ + {"my-branch", "my branch"}, + {"my_branch", "my branch"}, + {"nobranch", "nobranch"}, + } + + for _, tt := range tests { + t.Run(tt.input, func(t *testing.T) { + assert.Equal(t, tt.want, humanize(tt.input)) + }) + } +} + +func TestSyncStack_NewStack_CreateSuccess(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var gotNumbers []int + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + gotNumbers = prNumbers + return 42, nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Equal(t, []int{10, 11}, gotNumbers) + assert.Equal(t, "42", s.ID) + assert.Contains(t, output, "Stack created on GitHub with 2 PRs") +} + +func TestSyncStack_ExistingStack_UpdateSuccess(t *testing.T) { + s := &stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var gotStackID string + var gotNumbers []int + createCalled := false + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 0, nil + }, + UpdateStackFn: func(stackID string, prNumbers []int) error { + gotStackID = stackID + gotNumbers = prNumbers + return nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.False(t, createCalled, "CreateStack should not be called when s.ID is set") + assert.Equal(t, "99", gotStackID) + assert.Equal(t, []int{10, 11, 12}, gotNumbers) + assert.Contains(t, output, "Stack updated on GitHub with 3 PRs") +} + +func TestSyncStack_ExistingStack_UpdateFails(t *testing.T) { + s := &stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + UpdateStackFn: func(string, []int) error { + return &api.HTTPError{ + StatusCode: 422, + Message: "Validation failed", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "Failed to update stack") +} + +func TestSyncStack_ExistingStack_Update404(t *testing.T) { + s := &stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + var createCalled bool + mock := &github.MockClient{ + UpdateStackFn: func(string, []int) error { + return &api.HTTPError{ + StatusCode: 404, + Message: "Not Found", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks/99"}, + } + }, + CreateStackFn: func(prNumbers []int) (int, error) { + createCalled = true + return 55, nil + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.True(t, createCalled, "should fall through to CreateStack after 404") + assert.Equal(t, "55", s.ID, "should set new stack ID from create response") + assert.Contains(t, output, "Stack created on GitHub with 2 PRs") +} + +func TestSyncStack_AlreadyStacked_422(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests #10, #11 are already stacked", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "already part of an existing stack") +} + +func TestSyncStack_InvalidChain_422(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "must form a stack") + assert.Contains(t, output, "base branch must match") +} + +func TestSyncStack_NotAvailable(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 404, + Message: "Not Found", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "not yet available") +} + +func TestSyncStack_SkippedForSinglePR(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + }, + } + + createCalled := false + updateCalled := false + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + createCalled = true + return 42, nil + }, + UpdateStackFn: func(string, []int) error { + updateCalled = true + return nil + }, + } + + cfg, _, _ := config.NewTestConfig() + syncStack(cfg, mock, s) + cfg.Err.Close() + + assert.False(t, createCalled, "CreateStack should not be called with fewer than 2 PRs") + assert.False(t, updateCalled, "UpdateStack should not be called with fewer than 2 PRs") +} + +func TestSyncStack_SkipsMergedBranches(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10, Merged: true}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var gotNumbers []int + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + gotNumbers = prNumbers + return 42, nil + }, + } + + cfg, _, _ := config.NewTestConfig() + syncStack(cfg, mock, s) + cfg.Err.Close() + + assert.Equal(t, []int{11, 12}, gotNumbers, "should only include non-merged PRs") +} + +func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, // no PR yet + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + var gotNumbers []int + mock := &github.MockClient{ + CreateStackFn: func(prNumbers []int) (int, error) { + gotNumbers = prNumbers + return 42, nil + }, + } + + cfg, _, _ := config.NewTestConfig() + syncStack(cfg, mock, s) + cfg.Err.Close() + + assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs") +} diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index 6fa2569..c039175 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -127,10 +127,10 @@ Small, incidental fixes (e.g., fixing a typo you noticed) can go in the current | Set custom trunk | `gh stack init --base develop branch-a` | | Add a branch to stack (suffix only if prefix set) | `gh stack add api-routes` | | Add branch + stage all + commit | `gh stack add -Am "message" api-routes` | -| Push + create PRs | `gh stack push --auto` | -| Push as drafts | `gh stack push --auto --draft` | -| Push without creating PRs | `gh stack push --skip-prs` | -| Push to specific remote | `gh stack push --auto --remote origin` | +| Push branches to remote | `gh stack push` | +| Push to specific remote | `gh stack push --remote origin` | +| Push branches + create PRs | `gh stack submit --auto` | +| Create PRs as drafts | `gh stack submit --auto --draft` | | Sync (fetch, rebase, push) | `gh stack sync` | | Sync with specific remote | `gh stack sync --remote origin` | | Rebase entire stack | `gh stack rebase` | @@ -214,7 +214,7 @@ git commit -m "Add frontend dashboard" # ── Stack complete: feat/auth → feat/api-routes → feat/frontend ── # 7. Push everything and create draft PRs -gh stack push --auto --draft +gh stack submit --auto --draft # 8. Verify the stack gh stack view --json @@ -278,7 +278,7 @@ git commit -m "Fix auth token validation" gh stack rebase --upstack # 4. Push the updated stack -gh stack push --auto +gh stack push ``` ### Routine sync after merges @@ -459,30 +459,57 @@ gh stack add -um "Fix auth bug" auth-fix --- -### Push branches and create PRs — `gh stack push` +### Push branches to remote — `gh stack push` -Push all stack branches and create/update PRs. +Push all stack branches to the remote. ``` gh stack push [flags] ``` ```bash -# Push and auto-title new PRs -gh stack push --auto +# Push all branches +gh stack push -# Push and create PRs as drafts -gh stack push --auto --draft +# Push to specific remote +gh stack push --remote upstream +``` + +| Flag | Description | +|------|-------------| +| `--remote ` | Remote to push to (use if multiple remotes exist) | + +**Behavior:** + +- Pushes all active (non-merged) branches atomically (`--force-with-lease --atomic`) +- Does **not** create or update pull requests — use `gh stack submit` for that + +**Output (stderr):** + +- `Pushed N branches` summary + +--- + +### Submit branches and create PRs — `gh stack submit` + +Push all stack branches and create PRs on GitHub. + +``` +gh stack submit [flags] +``` + +```bash +# Submit and auto-title new PRs +gh stack submit --auto -# Push branches only, no PR creation -gh stack push --skip-prs +# Submit and create PRs as drafts +gh stack submit --auto --draft ``` | Flag | Description | |------|-------------| | `--auto` | Auto-generate PR titles without prompting | | `--draft` | Create new PRs as drafts | -| `--skip-prs` | Push branches without creating or updating PRs | | `--remote ` | Remote to push to (use if multiple remotes exist) | **Behavior:** @@ -743,4 +770,4 @@ gh stack unstack feature-auth --local 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. **Server-side stack deletion is not supported.** Use `unstack --local`. 6. **Server-side stack discovery is not supported.** `checkout` only works with locally tracked stacks. -7. **PR title and body are auto-generated.** There is no flag to set a custom PR title or body during `push`. The title and body are generated from commit messages plus a footer. Use `gh pr edit` to modify PR title and body after creation. +7. **PR title and body are auto-generated.** There is no flag to set a custom PR title or body during `submit`. The title and body are generated from commit messages plus a footer. Use `gh pr edit` to modify PR title and body after creation. From 4604b798c01a18b7be3e2373114cdde6cf9e53e3 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 3 Apr 2026 04:15:41 -0400 Subject: [PATCH 4/7] rm unstack --- README.md | 31 -------- cmd/root.go | 1 - cmd/root_test.go | 2 +- cmd/unstack.go | 72 ----------------- cmd/unstack_test.go | 118 ---------------------------- internal/github/client_interface.go | 1 - internal/github/github.go | 6 -- internal/github/mock_client.go | 10 --- internal/stack/stack_test.go | 2 +- skills/gh-stack/SKILL.md | 36 +-------- 10 files changed, 4 insertions(+), 275 deletions(-) delete mode 100644 cmd/unstack.go delete mode 100644 cmd/unstack_test.go diff --git a/README.md b/README.md index 732d02d..95b4976 100644 --- a/README.md +++ b/README.md @@ -326,37 +326,6 @@ gh stack view --short gh stack view --json ``` -### `gh stack unstack` - -Remove a stack from local tracking and optionally delete it on GitHub. - -``` -gh stack unstack [branch] [flags] -``` - -If no branch is specified, uses the current branch to find the stack. By default, the stack is removed from both local tracking and GitHub. Use `--local` to only remove the local tracking entry. - -| Flag | Description | -|------|-------------| -| `--local` | Only delete the stack locally (keep it on GitHub) | - -| Argument | Description | -|----------|-------------| -| `[branch]` | A branch in the stack to delete (defaults to the current branch) | - -**Examples:** - -```sh -# Remove the stack from local tracking and GitHub -gh stack unstack - -# Only remove local tracking -gh stack unstack --local - -# Specify a branch to identify the stack -gh stack unstack feature-auth -``` - ### `gh stack merge` Merge a stack of PRs. diff --git a/cmd/root.go b/cmd/root.go index 704fe1e..a55f9dd 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -35,7 +35,6 @@ func RootCmd() *cobra.Command { root.AddCommand(PushCmd(cfg)) root.AddCommand(SubmitCmd(cfg)) root.AddCommand(SyncCmd(cfg)) - root.AddCommand(UnstackCmd(cfg)) root.AddCommand(MergeCmd(cfg)) // Helper commands diff --git a/cmd/root_test.go b/cmd/root_test.go index a5bba22..2041fb0 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -8,7 +8,7 @@ import ( func TestRootCmd_SubcommandRegistration(t *testing.T) { root := RootCmd() - expected := []string{"init", "add", "checkout", "push", "sync", "unstack", "merge", "view", "rebase", "up", "down", "top", "bottom", "alias", "feedback"} + expected := []string{"init", "add", "checkout", "push", "sync", "merge", "view", "rebase", "up", "down", "top", "bottom", "alias", "feedback"} registered := make(map[string]bool) for _, cmd := range root.Commands() { diff --git a/cmd/unstack.go b/cmd/unstack.go deleted file mode 100644 index 0be869c..0000000 --- a/cmd/unstack.go +++ /dev/null @@ -1,72 +0,0 @@ -package cmd - -import ( - "github.com/github/gh-stack/internal/config" - "github.com/github/gh-stack/internal/stack" - "github.com/spf13/cobra" -) - -type unstackOptions struct { - target string - local bool -} - -func UnstackCmd(cfg *config.Config) *cobra.Command { - opts := &unstackOptions{} - - cmd := &cobra.Command{ - Use: "unstack [branch]", - Short: "Delete a stack locally and on GitHub", - Long: "Remove a stack from local tracking and delete it on GitHub. Use --local to only remove local tracking.", - Args: cobra.MaximumNArgs(1), - RunE: func(cmd *cobra.Command, args []string) error { - if len(args) > 0 { - opts.target = args[0] - } - return runUnstack(cfg, opts) - }, - } - - cmd.Flags().BoolVar(&opts.local, "local", false, "Only delete the stack locally") - - return cmd -} - -func runUnstack(cfg *config.Config, opts *unstackOptions) error { - result, err := loadStack(cfg, opts.target) - if err != nil { - return ErrNotInStack - } - gitDir := result.GitDir - sf := result.StackFile - s := result.Stack - target := opts.target - if target == "" { - target = result.CurrentBranch - } - - cfg.Printf("Stack branches: %v", s.BranchNames()) - - // Remove from local tracking - sf.RemoveStackForBranch(target) - if err := stack.Save(gitDir, sf); err != nil { - return handleSaveError(cfg, err) - } - cfg.Successf("Stack removed from local tracking") - - // Delete the stack on GitHub - if !opts.local { - client, err := cfg.GitHubClient() - if err != nil { - cfg.Errorf("failed to create GitHub client: %s", err) - return ErrAPIFailure - } - if err := client.DeleteStack(); err != nil { - cfg.Warningf("%v", err) - } else { - cfg.Successf("Stack deleted on GitHub") - } - } - - return nil -} diff --git a/cmd/unstack_test.go b/cmd/unstack_test.go deleted file mode 100644 index 6087887..0000000 --- a/cmd/unstack_test.go +++ /dev/null @@ -1,118 +0,0 @@ -package cmd - -import ( - "encoding/json" - "os" - "path/filepath" - "testing" - - "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/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func writeTwoStacks(t *testing.T, dir string, s1, s2 stack.Stack) { - t.Helper() - sf := &stack.StackFile{ - SchemaVersion: 1, - Stacks: []stack.Stack{s1, s2}, - } - data, err := json.MarshalIndent(sf, "", " ") - require.NoError(t, err) - require.NoError(t, os.WriteFile(filepath.Join(dir, "gh-stack"), data, 0644)) -} - -func TestUnstack_RemovesStack(t *testing.T) { - gitDir := t.TempDir() - restore := git.SetOps(&git.MockOps{ - GitDirFn: func() (string, error) { return gitDir, nil }, - CurrentBranchFn: func() (string, error) { return "b1", nil }, - }) - defer restore() - - s1 := stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, - } - s2 := stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b3"}, {Branch: "b4"}}, - } - writeTwoStacks(t, gitDir, s1, s2) - - cfg, outR, errR := config.NewTestConfig() - cfg.GitHubClientOverride = &github.MockClient{} - err := runUnstack(cfg, &unstackOptions{}) - output := collectOutput(cfg, outR, errR) - - // The GitHub API call will fail (no real repo), but the command should not - // return a fatal error — only a warning is printed. - require.NoError(t, err) - assert.Contains(t, output, "Stack removed from local tracking") - - sf, err := stack.Load(gitDir) - require.NoError(t, err) - require.Len(t, sf.Stacks, 1) - assert.Equal(t, []string{"b3", "b4"}, sf.Stacks[0].BranchNames()) -} - -func TestUnstack_Local(t *testing.T) { - gitDir := t.TempDir() - restore := git.SetOps(&git.MockOps{ - GitDirFn: func() (string, error) { return gitDir, nil }, - CurrentBranchFn: func() (string, error) { return "b1", nil }, - }) - defer restore() - - writeStackFile(t, gitDir, stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, - }) - - cfg, outR, errR := config.NewTestConfig() - err := runUnstack(cfg, &unstackOptions{local: true}) - output := collectOutput(cfg, outR, errR) - - require.NoError(t, err) - assert.Contains(t, output, "Stack removed") - // With --local, the GitHub API error message should NOT appear. - assert.NotContains(t, output, "failed to create GitHub client") - - sf, err := stack.Load(gitDir) - require.NoError(t, err) - assert.Empty(t, sf.Stacks) -} - -func TestUnstack_WithTarget(t *testing.T) { - gitDir := t.TempDir() - restore := git.SetOps(&git.MockOps{ - GitDirFn: func() (string, error) { return gitDir, nil }, - CurrentBranchFn: func() (string, error) { return "unrelated", nil }, - }) - defer restore() - - s1 := stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b1"}, {Branch: "b2"}}, - } - s2 := stack.Stack{ - Trunk: stack.BranchRef{Branch: "main"}, - Branches: []stack.BranchRef{{Branch: "b3"}, {Branch: "b4"}}, - } - writeTwoStacks(t, gitDir, s1, s2) - - cfg, outR, errR := config.NewTestConfig() - err := runUnstack(cfg, &unstackOptions{target: "b3", local: true}) - output := collectOutput(cfg, outR, errR) - - require.NoError(t, err) - assert.Contains(t, output, "Stack removed") - - sf, err := stack.Load(gitDir) - require.NoError(t, err) - require.Len(t, sf.Stacks, 1) - assert.Equal(t, []string{"b1", "b2"}, sf.Stacks[0].BranchNames()) -} diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index d64b698..f5e9c57 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -10,7 +10,6 @@ type ClientOps interface { CreatePR(base, head, title, body string, draft bool) (*PullRequest, error) CreateStack(prNumbers []int) (int, error) UpdateStack(stackID string, prNumbers []int) error - DeleteStack() error } // Compile-time check that Client satisfies ClientOps. diff --git a/internal/github/github.go b/internal/github/github.go index e9acb5c..d04c678 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -266,12 +266,6 @@ func (c *Client) FindPRDetailsForBranch(branch string) (*PRDetails, error) { }, nil } -// DeleteStack deletes a stack on GitHub. -// TODO: Implement once the stack API is available. -func (c *Client) DeleteStack() error { - return fmt.Errorf("deleting a stack on GitHub is not yet supported by the API") -} - // CreateStack creates a stack on GitHub from an ordered list of PR numbers. // The PR numbers must be ordered from bottom to top of the stack and must // form a valid base-to-head chain. Returns the server-assigned stack ID. diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index 786ac06..c5ec058 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -1,7 +1,5 @@ package github -import "fmt" - // MockClient is a test double for GitHub API operations. // Each field is an optional function that, when set, handles the corresponding // ClientOps method call. When nil, a reasonable default is returned. @@ -12,7 +10,6 @@ type MockClient struct { CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) CreateStackFn func([]int) (int, error) UpdateStackFn func(string, []int) error - DeleteStackFn func() error } // Compile-time check that MockClient satisfies ClientOps. @@ -46,13 +43,6 @@ func (m *MockClient) CreatePR(base, head, title, body string, draft bool) (*Pull return nil, nil } -func (m *MockClient) DeleteStack() error { - if m.DeleteStackFn != nil { - return m.DeleteStackFn() - } - return fmt.Errorf("deleting a stack on GitHub is not yet supported by the API") -} - func (m *MockClient) CreateStack(prNumbers []int) (int, error) { if m.CreateStackFn != nil { return m.CreateStackFn(prNumbers) diff --git a/internal/stack/stack_test.go b/internal/stack/stack_test.go index 3fa65de..7735993 100644 --- a/internal/stack/stack_test.go +++ b/internal/stack/stack_test.go @@ -359,7 +359,7 @@ func TestValidateNoDuplicateBranch(t *testing.T) { }) } -// --- RemoveStackForBranch: used by unstack --- +// --- RemoveStackForBranch --- func TestRemoveStackForBranch(t *testing.T) { t.Run("found and removed", func(t *testing.T) { diff --git a/skills/gh-stack/SKILL.md b/skills/gh-stack/SKILL.md index c039175..63ca208 100644 --- a/skills/gh-stack/SKILL.md +++ b/skills/gh-stack/SKILL.md @@ -142,7 +142,6 @@ Small, incidental fixes (e.g., fixing a typo you noticed) can go in the current | Switch to top/bottom branch | `gh stack top` / `gh stack bottom` | | Check out by PR | `gh stack checkout 42` | | Check out by branch | `gh stack checkout feature-auth` | -| Remove stack | `gh stack unstack --local` | --- @@ -357,12 +356,6 @@ echo "$output" | jq -r '.currentBranch' echo "$output" | jq '[.branches[] | .isMerged] | all' ``` -### Clean up after all PRs are merged - -```bash -gh stack unstack --local -``` - --- ## Commands @@ -716,30 +709,6 @@ Resolves the target against locally tracked stacks. Accepts a PR number, PR URL, --- -### Remove a stack — `gh stack unstack` - -Remove a stack from local tracking. Use `--local` to avoid warnings about unsupported server-side deletion. - -``` -gh stack unstack [branch] [flags] -``` - -```bash -# Remove from local tracking -gh stack unstack --local - -# Specify a branch to identify which stack -gh stack unstack feature-auth --local -``` - -| Flag | Description | -|------|-------------| -| `--local` | Only delete the stack locally (recommended) | - -| Argument | Description | -|----------|-------------| -| `[branch]` | A branch in the stack (defaults to the current branch) | - --- ## Output conventions @@ -768,6 +737,5 @@ gh stack unstack feature-auth --local 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, pass `--remote ` or set `remote.pushDefault` in git config before running `push`, `sync`, or `rebase`. 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. **Server-side stack deletion is not supported.** Use `unstack --local`. -6. **Server-side stack discovery is not supported.** `checkout` only works with locally tracked stacks. -7. **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. +5. **Server-side stack discovery is not supported.** `checkout` only works with locally tracked stacks. +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. From 9b6b712a546b4d465a94428a15f9697c4d0fcee8 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 3 Apr 2026 04:32:37 -0400 Subject: [PATCH 5/7] Apply suggestions from code review Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- README.md | 2 +- cmd/root_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 95b4976..9784067 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,7 @@ main (trunk) The **bottom** of the stack is the branch closest to the trunk, and the **top** is the branch furthest from it. Each branch inherits from the one below it. Navigation commands (`up`, `down`, `top`, `bottom`) follow this model: `up` moves away from trunk, `down` moves toward it. -When you push, `gh stack` creates one PR per branch and links them together as a **Stack** on GitHub. Each PR's base is set to the branch below it in the stack, so reviewers see only the diff for that layer. +When you submit, `gh stack` creates one PR per branch and links them together as a **Stack** on GitHub. Each PR's base is set to the branch below it in the stack, so reviewers see only the diff for that layer. ### Local tracking diff --git a/cmd/root_test.go b/cmd/root_test.go index 2041fb0..d902390 100644 --- a/cmd/root_test.go +++ b/cmd/root_test.go @@ -8,7 +8,7 @@ import ( func TestRootCmd_SubcommandRegistration(t *testing.T) { root := RootCmd() - expected := []string{"init", "add", "checkout", "push", "sync", "merge", "view", "rebase", "up", "down", "top", "bottom", "alias", "feedback"} + expected := []string{"init", "add", "checkout", "push", "sync", "merge", "view", "rebase", "up", "down", "top", "bottom", "alias", "feedback", "submit"} registered := make(map[string]bool) for _, cmd := range root.Commands() { From bd76d31ef3b8e2e02f2068e12cfdda165f75eea2 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 3 Apr 2026 09:43:47 -0400 Subject: [PATCH 6/7] early exit if all branches merged --- README.md | 2 +- cmd/push.go | 4 ++++ cmd/rebase.go | 4 ++-- cmd/submit.go | 8 ++++++-- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index 9784067..dd1c848 100644 --- a/README.md +++ b/README.md @@ -283,7 +283,7 @@ Push all branches and create/update PRs and the stack on GitHub. gh stack submit [flags] ``` -Creates a Stacked PR for every branch in the stack, pushing branches to remote if needed. +Creates a Stacked PR for every branch in the stack, pushing branches to the remote. After creating PRs, `submit` automatically creates a **Stack** on GitHub to link the PRs together. If the stack already exists on GitHub (e.g., from a previous submit), new PRs will be added to the top of the stack. diff --git a/cmd/push.go b/cmd/push.go index cee0e91..e3d84da 100644 --- a/cmd/push.go +++ b/cmd/push.go @@ -76,6 +76,10 @@ func runPush(cfg *config.Config, opts *pushOptions) error { cfg.Printf("Skipping %d merged %s", len(merged), plural(len(merged), "branch", "branches")) } activeBranches := activeBranchNames(s) + if len(activeBranches) == 0 { + cfg.Printf("No active branches to push (all merged)") + return nil + } cfg.Printf("Pushing %d %s to %s...", len(activeBranches), plural(len(activeBranches), "branch", "branches"), remote) if err := git.Push(remote, activeBranches, true, true); err != nil { cfg.Errorf("failed to push: %s", err) diff --git a/cmd/rebase.go b/cmd/rebase.go index 1515040..7805d47 100644 --- a/cmd/rebase.go +++ b/cmd/rebase.go @@ -324,8 +324,8 @@ func runRebase(cfg *config.Config, opts *rebaseOptions) error { } cfg.Printf("%s rebased locally with %s", rangeDesc, s.Trunk.Branch) - cfg.Printf("To push up your changes and open/update the stack of PRs, run `%s`", - cfg.ColorCyan("gh stack submit")) + cfg.Printf("To push up your changes, run `%s`", + cfg.ColorCyan("gh stack push")) return nil } diff --git a/cmd/submit.go b/cmd/submit.go index ca0cd63..372fa7a 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -90,6 +90,10 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { cfg.Printf("Skipping %d merged %s", len(merged), plural(len(merged), "branch", "branches")) } activeBranches := activeBranchNames(s) + if len(activeBranches) == 0 { + cfg.Printf("All branches are merged, nothing to submit") + return nil + } cfg.Printf("Pushing %d %s to %s...", len(activeBranches), plural(len(activeBranches), "branch", "branches"), remote) if err := git.Push(remote, activeBranches, true, true); err != nil { cfg.Errorf("failed to push: %s", err) @@ -304,14 +308,14 @@ func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) { msg := httpErr.Message if strings.Contains(msg, "already stacked") { - cfg.Errorf("One or more PRs are already part of an existing stack on GitHub") + cfg.Warningf("One or more PRs are already part of an existing stack on GitHub") cfg.Printf(" To fix this, unstack the PRs from the web, then `%s`", cfg.ColorCyan("gh stack submit")) return } if strings.Contains(msg, "must form a stack") { - cfg.Errorf("Cannot create stack: %s", msg) + cfg.Warningf("Cannot create stack: %s", msg) cfg.Printf(" Each PR's base branch must match the previous PR's head branch.") return } From cb7f9fa895e9b4bca53ef02895ac47e0a16dde59 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Fri, 3 Apr 2026 11:33:14 -0400 Subject: [PATCH 7/7] update pr base if not already part of a stack --- cmd/submit.go | 50 ++++- cmd/submit_test.go | 272 +++++++++++++++++++++++++++- internal/github/client_interface.go | 1 + internal/github/github.go | 15 ++ internal/github/mock_client.go | 14 +- 5 files changed, 341 insertions(+), 11 deletions(-) diff --git a/cmd/submit.go b/cmd/submit.go index 372fa7a..1fc7ca6 100644 --- a/cmd/submit.go +++ b/cmd/submit.go @@ -100,7 +100,9 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { return ErrSilent } - // Create or update PRs + // Create or update PRs — ensure every active branch has a PR with the + // correct base branch. This makes submit idempotent: running it again + // fills gaps and fixes base branches before syncing the stack. for i, b := range s.Branches { if s.Branches[i].IsMerged() { continue @@ -154,7 +156,7 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { URL: newPR.URL, } } else { - cfg.Printf("PR %s for %s is up to date", cfg.PRLink(pr.Number, pr.URL), b.Branch) + // PR already exists — record it and fix base branch if needed. if s.Branches[i].PullRequest == nil { s.Branches[i].PullRequest = &stack.PullRequestRef{ Number: pr.Number, @@ -162,6 +164,25 @@ func runSubmit(cfg *config.Config, opts *submitOptions) error { URL: pr.URL, } } + + if pr.BaseRefName != baseBranch { + if s.ID != "" { + // PRs in an existing stack can't have their base updated + // via the API — the stack owns the base relationships. + cfg.Warningf("PR %s has base %q (expected %q) but cannot update while stacked", + cfg.PRLink(pr.Number, pr.URL), pr.BaseRefName, baseBranch) + } else { + if err := client.UpdatePRBase(pr.Number, baseBranch); err != nil { + cfg.Warningf("failed to update base branch for PR %s: %v", + cfg.PRLink(pr.Number, pr.URL), err) + } else { + cfg.Successf("Updated base branch for PR %s to %s", + cfg.PRLink(pr.Number, pr.URL), baseBranch) + } + } + } else { + cfg.Printf("PR %s for %s is up to date", cfg.PRLink(pr.Number, pr.URL), b.Branch) + } } } @@ -291,7 +312,7 @@ func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, switch httpErr.StatusCode { case 422: - handleCreate422(cfg, httpErr) + handleCreate422(cfg, httpErr, prNumbers) case 404: cfg.Warningf("Stacked PRs are not yet available for this repository") default: @@ -304,11 +325,18 @@ func createNewStack(cfg *config.Config, client github.ClientOps, s *stack.Stack, // - "Stack must contain at least two pull requests" // - "Pull requests must form a stack, where each PR's base ref is the previous PR's head ref" // - "Pull requests #123, #124, #125 are already stacked" -func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) { +func handleCreate422(cfg *config.Config, httpErr *api.HTTPError, prNumbers []int) { msg := httpErr.Message if strings.Contains(msg, "already stacked") { - cfg.Warningf("One or more PRs are already part of an existing stack on GitHub") + // Check if the error lists exactly the same PRs we're trying to + // stack. If so, they're already in a stack together — nothing to do. + // If only a subset matches, the PRs are in a different stack. + if allPRsInMessage(msg, prNumbers) { + cfg.Successf("Stack with %d PRs is up to date", len(prNumbers)) + return + } + cfg.Warningf("One or more PRs are already part of a different stack on GitHub") cfg.Printf(" To fix this, unstack the PRs from the web, then `%s`", cfg.ColorCyan("gh stack submit")) return @@ -323,3 +351,15 @@ func handleCreate422(cfg *config.Config, httpErr *api.HTTPError) { // "at least two" or any other validation error cfg.Warningf("Could not create stack: %s", msg) } + +// allPRsInMessage checks whether every PR number in prNumbers appears +// in the error message (e.g. as "#65"). This distinguishes "our PRs are +// already stacked together" from "some PRs are in a different stack." +func allPRsInMessage(msg string, prNumbers []int) bool { + for _, n := range prNumbers { + if !strings.Contains(msg, fmt.Sprintf("#%d", n)) { + return false + } + } + return true +} diff --git a/cmd/submit_test.go b/cmd/submit_test.go index c9d80e0..7b0179b 100644 --- a/cmd/submit_test.go +++ b/cmd/submit_test.go @@ -397,7 +397,8 @@ func TestSyncStack_ExistingStack_Update404(t *testing.T) { assert.Contains(t, output, "Stack created on GitHub with 2 PRs") } -func TestSyncStack_AlreadyStacked_422(t *testing.T) { +func TestSyncStack_AlreadyStacked_OurStack(t *testing.T) { + // All our PRs are listed as "already stacked" — this is our stack, show up-to-date. s := &stack.Stack{ Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ @@ -423,7 +424,40 @@ func TestSyncStack_AlreadyStacked_422(t *testing.T) { errOut, _ := io.ReadAll(errR) output := string(errOut) - assert.Contains(t, output, "already part of an existing stack") + assert.Contains(t, output, "Stack with 2 PRs is up to date") + assert.NotContains(t, output, "different stack") +} + +func TestSyncStack_AlreadyStacked_DifferentStack(t *testing.T) { + // Only a subset of our PRs are listed — they're in a different stack. + s := &stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + mock := &github.MockClient{ + CreateStackFn: func([]int) (int, error) { + return 0, &api.HTTPError{ + StatusCode: 422, + Message: "Pull requests #10, #11 are already stacked", + RequestURL: &url.URL{Path: "/repos/o/r/cli_internal/pulls/stacks"}, + } + }, + } + + cfg, _, errR := config.NewTestConfig() + syncStack(cfg, mock, s) + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.Contains(t, output, "different stack") + assert.NotContains(t, output, "up to date") } func TestSyncStack_InvalidChain_422(t *testing.T) { @@ -544,7 +578,7 @@ func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { Trunk: stack.BranchRef{Branch: "main"}, Branches: []stack.BranchRef{ {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, - {Branch: "b2"}, // no PR yet + {Branch: "b2"}, // no PR — skipped {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, }, } @@ -563,3 +597,235 @@ func TestSyncStack_SkipsBranchesWithoutPR(t *testing.T) { assert.Equal(t, []int{10, 12}, gotNumbers, "should skip branches without PRs") } + +func TestSubmit_UpdatesBaseBranch(t *testing.T) { + // b1's PR has base "main" but it should be "main" (correct). + // b2's PR has base "main" but it should be "b1" (wrong — needs update). + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + mock := newSubmitMock(tmpDir, "b1") + + restore := git.SetOps(mock) + defer restore() + + var updatedPRs []struct { + number int + base string + } + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + switch branch { + case "b1": + return &github.PullRequest{ + Number: 10, ID: "PR_10", + URL: "https://github.com/owner/repo/pull/10", + BaseRefName: "main", HeadRefName: "b1", + }, nil + case "b2": + return &github.PullRequest{ + Number: 11, ID: "PR_11", + URL: "https://github.com/owner/repo/pull/11", + BaseRefName: "main", HeadRefName: "b2", // wrong base + }, nil + } + return nil, nil + }, + UpdatePRBaseFn: func(number int, base string) error { + updatedPRs = append(updatedPRs, struct { + number int + base string + }{number, base}) + return nil + }, + CreateStackFn: func(prNumbers []int) (int, error) { + return 42, nil + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + // b1's base is "main" which is correct — no update. + // b2's base is "main" but should be "b1" — should be updated. + require.Len(t, updatedPRs, 1) + assert.Equal(t, 11, updatedPRs[0].number) + assert.Equal(t, "b1", updatedPRs[0].base) + assert.Contains(t, output, "Updated base branch for PR") +} + +func TestSubmit_SkipsBaseUpdateWhenStacked(t *testing.T) { + // Stack already exists (s.ID is set), so base updates should be skipped. + s := stack.Stack{ + ID: "99", + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2", PullRequest: &stack.PullRequestRef{Number: 11}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + mock := newSubmitMock(tmpDir, "b1") + + restore := git.SetOps(mock) + defer restore() + + updateCalled := false + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + switch branch { + case "b1": + return &github.PullRequest{ + Number: 10, ID: "PR_10", + URL: "https://github.com/owner/repo/pull/10", + BaseRefName: "main", HeadRefName: "b1", + }, nil + case "b2": + return &github.PullRequest{ + Number: 11, ID: "PR_11", + URL: "https://github.com/owner/repo/pull/11", + BaseRefName: "main", HeadRefName: "b2", // wrong base + }, nil + } + return nil, nil + }, + UpdatePRBaseFn: func(number int, base string) error { + updateCalled = true + return nil + }, + UpdateStackFn: func(stackID string, prNumbers []int) error { + return nil + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + assert.False(t, updateCalled, "should not call UpdatePRBase when stack exists") + assert.Contains(t, output, "cannot update while stacked") +} + +func TestSubmit_CreatesMissingPRsAndUpdatesExisting(t *testing.T) { + // b1 has a PR, b2 does not, b3 has a PR with wrong base. + // Submit should create b2's PR and fix b3's base. + s := stack.Stack{ + Trunk: stack.BranchRef{Branch: "main"}, + Branches: []stack.BranchRef{ + {Branch: "b1", PullRequest: &stack.PullRequestRef{Number: 10}}, + {Branch: "b2"}, + {Branch: "b3", PullRequest: &stack.PullRequestRef{Number: 12}}, + }, + } + + tmpDir := t.TempDir() + writeStackFile(t, tmpDir, s) + + mock := newSubmitMock(tmpDir, "b1") + mock.LogRangeFn = func(base, head string) ([]git.CommitInfo, error) { + return []git.CommitInfo{{Subject: "commit for " + head}}, nil + } + + restore := git.SetOps(mock) + defer restore() + + var createdPRs []string + var updatedBases []struct { + number int + base string + } + + cfg, _, errR := config.NewTestConfig() + cfg.GitHubClientOverride = &github.MockClient{ + FindPRForBranchFn: func(branch string) (*github.PullRequest, error) { + switch branch { + case "b1": + return &github.PullRequest{ + Number: 10, ID: "PR_10", + URL: "https://github.com/owner/repo/pull/10", + BaseRefName: "main", HeadRefName: "b1", + }, nil + case "b2": + return nil, nil // no PR + case "b3": + return &github.PullRequest{ + Number: 12, ID: "PR_12", + URL: "https://github.com/owner/repo/pull/12", + BaseRefName: "main", HeadRefName: "b3", // wrong base — should be b2 + }, nil + } + return nil, nil + }, + CreatePRFn: func(base, head, title, body string, draft bool) (*github.PullRequest, error) { + createdPRs = append(createdPRs, head) + return &github.PullRequest{ + Number: 11, ID: "PR_11", + URL: "https://github.com/owner/repo/pull/11", + }, nil + }, + UpdatePRBaseFn: func(number int, base string) error { + updatedBases = append(updatedBases, struct { + number int + base string + }{number, base}) + return nil + }, + CreateStackFn: func(prNumbers []int) (int, error) { + return 42, nil + }, + } + + cmd := SubmitCmd(cfg) + cmd.SetArgs([]string{"--auto"}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + err := cmd.Execute() + + cfg.Err.Close() + errOut, _ := io.ReadAll(errR) + output := string(errOut) + + assert.NoError(t, err) + + // b2 should have been created + assert.Equal(t, []string{"b2"}, createdPRs) + assert.Contains(t, output, "Created PR") + + // b3's base should have been updated from "main" to "b2" + require.Len(t, updatedBases, 1) + assert.Equal(t, 12, updatedBases[0].number) + assert.Equal(t, "b2", updatedBases[0].base) + assert.Contains(t, output, "Updated base branch for PR") + + // Stack should be created with all 3 PRs + assert.Contains(t, output, "Stack created on GitHub with 3 PRs") +} diff --git a/internal/github/client_interface.go b/internal/github/client_interface.go index f5e9c57..9faff2f 100644 --- a/internal/github/client_interface.go +++ b/internal/github/client_interface.go @@ -8,6 +8,7 @@ type ClientOps interface { FindAnyPRForBranch(branch string) (*PullRequest, error) FindPRDetailsForBranch(branch string) (*PRDetails, error) CreatePR(base, head, title, body string, draft bool) (*PullRequest, error) + UpdatePRBase(number int, base string) error CreateStack(prNumbers []int) (int, error) UpdateStack(stackID string, prNumbers []int) error } diff --git a/internal/github/github.go b/internal/github/github.go index d04c678..a045136 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -204,6 +204,21 @@ func (c *Client) CreatePR(base, head, title, body string, draft bool) (*PullRequ }, nil } +// UpdatePRBase updates the base branch of an existing pull request. +func (c *Client) UpdatePRBase(number int, base string) error { + type updatePRRequest struct { + Base string `json:"base"` + } + + body, err := json.Marshal(updatePRRequest{Base: base}) + if err != nil { + return fmt.Errorf("marshaling request: %w", err) + } + + path := fmt.Sprintf("repos/%s/%s/pulls/%d", c.owner, c.repo, number) + return c.rest.Patch(path, bytes.NewReader(body), nil) +} + // PRDetails holds enriched pull request data for display in the TUI. type PRDetails struct { Number int diff --git a/internal/github/mock_client.go b/internal/github/mock_client.go index c5ec058..74fc6cb 100644 --- a/internal/github/mock_client.go +++ b/internal/github/mock_client.go @@ -7,9 +7,10 @@ type MockClient struct { FindPRForBranchFn func(string) (*PullRequest, error) FindAnyPRForBranchFn func(string) (*PullRequest, error) FindPRDetailsForBranchFn func(string) (*PRDetails, error) - CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) - CreateStackFn func([]int) (int, error) - UpdateStackFn func(string, []int) error + CreatePRFn func(string, string, string, string, bool) (*PullRequest, error) + UpdatePRBaseFn func(int, string) error + CreateStackFn func([]int) (int, error) + UpdateStackFn func(string, []int) error } // Compile-time check that MockClient satisfies ClientOps. @@ -43,6 +44,13 @@ func (m *MockClient) CreatePR(base, head, title, body string, draft bool) (*Pull return nil, nil } +func (m *MockClient) UpdatePRBase(number int, base string) error { + if m.UpdatePRBaseFn != nil { + return m.UpdatePRBaseFn(number, base) + } + return nil +} + func (m *MockClient) CreateStack(prNumbers []int) (int, error) { if m.CreateStackFn != nil { return m.CreateStackFn(prNumbers)