From a67b5cca1709218b09cae6467cdf1f0edc476e12 Mon Sep 17 00:00:00 2001 From: Dimitri Kennedy Date: Wed, 25 Mar 2026 21:36:08 -0400 Subject: [PATCH] fix: enforce stack parent invariants and deterministic PR lookup --- README.md | 7 ++ docs/how-it-works.md | 4 + docs/troubleshooting.md | 5 ++ docs/usage.md | 5 ++ internal/cmd/root.go | 37 ++++++++- internal/cmd/root_test.go | 97 ++++++++++++++++++++++ internal/github/client.go | 20 ++++- internal/github/client_test.go | 142 +++++++++++++++++++++++++++++++++ 8 files changed, 312 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index c0135df..c84a7d3 100644 --- a/README.md +++ b/README.md @@ -42,6 +42,10 @@ base, head, and remote state, then hands that PR to GitHub auto-merge or merge queue. After the merge lands, `stack sync` helps the rest of the stack catch up without guessing through ambiguous cases. +That handoff uses GitHub's own auto-merge path via `gh pr merge --auto`, so the +repository must have auto-merge enabled. If the repo also uses merge queue, +GitHub decides whether the PR goes straight to auto-merge or enters the queue. + ## How it differs from Graphite and similar tools `stack` is closest in spirit to tools that keep explicit local stack metadata, @@ -75,6 +79,9 @@ stack submit --all stack queue feature/base ``` +Before using `stack queue`, make sure the GitHub repository has auto-merge +enabled. + For the full daily workflow, start with [docs/usage.md](docs/usage.md). ## Starting from existing PRs diff --git a/docs/how-it-works.md b/docs/how-it-works.md index 1fa03fe..5be44a2 100644 --- a/docs/how-it-works.md +++ b/docs/how-it-works.md @@ -45,6 +45,10 @@ branch, and PR head all match, then hands that PR to GitHub auto-merge or merge queue. After the merge lands, `stack sync` helps advance the remaining stack to the next safe state. +Because `stack` delegates that final step to GitHub, the repository must have +auto-merge enabled. On repos with merge queue configured, GitHub applies queue +policy after the handoff. + ## How this differs from Graphite and similar tools The important difference is workflow shape: `stack` keeps branches and PRs diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 1da544b..1bebec6 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -72,6 +72,7 @@ Then choose a repair path deliberately. Check: - `gh auth status` +- GitHub repository auto-merge is enabled - the branch is pushed to the expected remote - the tracked PR is open and on the expected base - the local head still matches the pushed head @@ -83,6 +84,10 @@ stack submit stack queue ``` +If the CLI reports multiple open PRs for one head branch, it is refusing to +guess which live PR owns that branch. Close or retarget the duplicate until one +open PR remains for that head name, then rerun `stack submit`. + ## Release automation does not update the tap The release workflow needs: diff --git a/docs/usage.md b/docs/usage.md index fc632fd..66f7e2d 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -45,6 +45,10 @@ and cached PR state. 4. Run `stack queue ` only when the bottom branch targets trunk and is healthy. 5. Run `stack sync` after merges or GitHub-side base changes. +For `stack queue`, GitHub repository auto-merge must be enabled. `stack` hands +off through `gh pr merge --auto`, then GitHub applies the repo's normal +auto-merge or merge-queue policy. + ## Repair loop Use `stack sync` first when local metadata and GitHub disagree. @@ -68,3 +72,4 @@ clean recovery point. - `move`, `restack`, `submit`, and `queue` preview before destructive work unless you pass `--yes` - `sync` stops on ambiguous merged-parent cases instead of guessing - `queue` is only for a healthy bottom-of-stack PR +- `queue` requires GitHub repository auto-merge to be enabled diff --git a/internal/cmd/root.go b/internal/cmd/root.go index b685045..f2cb4b9 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -153,6 +153,9 @@ func newCreateCommand(runtime *stackruntime.Runtime) *cobra.Command { if err != nil { return err } + if err := ensureCreateParentAllowed(state, parent); err != nil { + return err + } if err := runtime.Git.SwitchCreate(runtime.Context, args[0]); err != nil { return err @@ -211,6 +214,9 @@ func newTrackCommand(runtime *stackruntime.Runtime) *cobra.Command { if parent != state.Trunk && !runtime.Git.BranchExists(runtime.Context, parent) { return fmt.Errorf("parent branch %q does not exist locally", parent) } + if err := ensureTrackedParentAllowed(state, parent); err != nil { + return err + } if err := stack.EnsureBranchCanParent(state, branch, parent); err != nil { return err } @@ -463,10 +469,8 @@ stack move feature/b --parent main --yes if !runtime.Git.BranchExists(runtime.Context, parent) && parent != state.Trunk { return fmt.Errorf("parent branch %q does not exist locally", parent) } - if parent != state.Trunk { - if _, ok := state.Branches[parent]; !ok { - return fmt.Errorf("parent branch %q is not tracked in local metadata; track it first or move under %s", parent, state.Trunk) - } + if err := ensureTrackedParentAllowed(state, parent); err != nil { + return err } if err := stack.EnsureBranchCanParent(state, branch, parent); err != nil { return err @@ -1322,6 +1326,31 @@ func validateTrackedPR(branch string, record store.BranchRecord) error { return nil } +func ensureCreateParentAllowed(state store.RepoState, parent string) error { + parent = strings.TrimSpace(parent) + if parent == "" { + return fmt.Errorf("cannot create a tracked branch from detached HEAD; switch to %s or another tracked branch first", state.Trunk) + } + if parent == state.Trunk { + return nil + } + if _, ok := state.Branches[parent]; ok { + return nil + } + return fmt.Errorf("current branch %q is not tracked in local metadata; track it first or switch to %s", parent, state.Trunk) +} + +func ensureTrackedParentAllowed(state store.RepoState, parent string) error { + parent = strings.TrimSpace(parent) + if parent == "" || parent == state.Trunk { + return nil + } + if _, ok := state.Branches[parent]; ok { + return nil + } + return fmt.Errorf("parent branch %q is not tracked in local metadata; track it first or move under %s", parent, state.Trunk) +} + func resolveOID(runtime *stackruntime.Runtime, ref string) string { oid, err := runtime.Git.ResolveRef(runtime.Context, ref) if err != nil { diff --git a/internal/cmd/root_test.go b/internal/cmd/root_test.go index 149b9b6..db2b52e 100644 --- a/internal/cmd/root_test.go +++ b/internal/cmd/root_test.go @@ -821,6 +821,103 @@ func TestSubmitCreatesAndTracksPR(t *testing.T) { } } +func TestCreateRejectsDetachedHEAD(t *testing.T) { + repo := testutil.SetupGitRepo(t) + testutil.Run(t, repo, "git", "checkout", "--detach") + + runtime := newTestRuntime(repo) + state := store.RepoState{ + Version: 1, + Repo: "hack-dance/stack", + DefaultRemote: "origin", + Trunk: "main", + Branches: map[string]store.BranchRecord{}, + } + if err := runtime.Store.WriteState(runtime.Context, state); err != nil { + t.Fatalf("write state: %v", err) + } + + err := executeCommandExpectError(runtime, "create", "feature/a") + if err == nil || !strings.Contains(err.Error(), "detached HEAD") { + t.Fatalf("expected detached HEAD error, got %v", err) + } + + state, err = runtime.Store.ReadState(runtime.Context) + if err != nil { + t.Fatalf("read state: %v", err) + } + if len(state.Branches) != 0 { + t.Fatalf("expected no tracked branches, got %+v", state.Branches) + } + if runtime.Git.BranchExists(runtime.Context, "feature/a") { + t.Fatalf("expected feature/a branch to not be created") + } +} + +func TestCreateRejectsUntrackedCurrentParent(t *testing.T) { + repo := testutil.SetupGitRepo(t) + testutil.Run(t, repo, "git", "switch", "-c", "feature/base") + + runtime := newTestRuntime(repo) + state := store.RepoState{ + Version: 1, + Repo: "hack-dance/stack", + DefaultRemote: "origin", + Trunk: "main", + Branches: map[string]store.BranchRecord{}, + } + if err := runtime.Store.WriteState(runtime.Context, state); err != nil { + t.Fatalf("write state: %v", err) + } + + err := executeCommandExpectError(runtime, "create", "feature/child") + if err == nil || !strings.Contains(err.Error(), "not tracked in local metadata") { + t.Fatalf("expected untracked parent error, got %v", err) + } + + state, err = runtime.Store.ReadState(runtime.Context) + if err != nil { + t.Fatalf("read state: %v", err) + } + if len(state.Branches) != 0 { + t.Fatalf("expected no tracked branches, got %+v", state.Branches) + } + if runtime.Git.BranchExists(runtime.Context, "feature/child") { + t.Fatalf("expected feature/child branch to not be created") + } +} + +func TestTrackRejectsUntrackedParent(t *testing.T) { + repo := testutil.SetupGitRepo(t) + testutil.Run(t, repo, "git", "switch", "-c", "feature/base") + testutil.Run(t, repo, "git", "switch", "-c", "feature/child") + + runtime := newTestRuntime(repo) + state := store.RepoState{ + Version: 1, + Repo: "hack-dance/stack", + DefaultRemote: "origin", + Trunk: "main", + Branches: map[string]store.BranchRecord{}, + } + if err := runtime.Store.WriteState(runtime.Context, state); err != nil { + t.Fatalf("write state: %v", err) + } + + err := executeCommandExpectError(runtime, "track", "feature/child", "--parent", "feature/base") + if err == nil || !strings.Contains(err.Error(), "parent branch \"feature/base\" is not tracked in local metadata") { + t.Fatalf("expected untracked parent error, got %v", err) + } + + state, err = runtime.Store.ReadState(runtime.Context) + if err != nil { + t.Fatalf("read state: %v", err) + } + if len(state.Branches) != 0 { + t.Fatalf("expected no tracked branches, got %+v", state.Branches) + } +} + func TestVersionCommandPrintsBuildInfo(t *testing.T) { repo := testutil.SetupGitRepo(t) runtime := newTestRuntime(repo) diff --git a/internal/github/client.go b/internal/github/client.go index 76fdd17..34752bc 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -94,7 +94,25 @@ func (c *Client) FindPRByHead(ctx context.Context, branch string) (store.PullReq return store.PullRequest{}, nil } - return payload[0].toStorePullRequest(), nil + open := make([]pullRequestPayload, 0, len(payload)) + for _, pr := range payload { + if pr.State == "OPEN" { + open = append(open, pr) + } + } + + switch len(open) { + case 0: + return store.PullRequest{}, nil + case 1: + return open[0].toStorePullRequest(), nil + default: + numbers := make([]string, 0, len(open)) + for _, pr := range open { + numbers = append(numbers, fmt.Sprintf("#%d", pr.Number)) + } + return store.PullRequest{}, fmt.Errorf("multiple open pull requests match head %q: %s", branch, strings.Join(numbers, ", ")) + } } func (c *Client) CreatePR(ctx context.Context, base string, head string, title string, body string, draft bool) (store.PullRequest, error) { diff --git a/internal/github/client_test.go b/internal/github/client_test.go index d3fda0e..9e0a581 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -3,6 +3,7 @@ package github_test import ( "context" "os" + "strings" "testing" stackgh "github.com/hack-dance/stack/internal/github" @@ -52,3 +53,144 @@ func TestViewPRMapsGitHubOIDsIntoCachedState(t *testing.T) { t.Fatalf("expected base oid def456, got %q", pr.LastSeenBaseOID) } } + +func TestFindPRByHeadPrefersSingleOpenMatch(t *testing.T) { + repo := testutil.SetupGitRepo(t) + ghStub := testutil.SetupGHStub(t, "hack-dance/stack", "main") + t.Setenv("STACK_TEST_GH_STATE", ghStub.StatePath) + t.Setenv("STACK_TEST_GH_LOG", ghStub.LogPath) + t.Setenv("PATH", ghStub.Dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + testutil.WriteFile(t, ghStub.StatePath, `{ + "repo": { + "nameWithOwner": "hack-dance/stack", + "url": "https://github.com/hack-dance/stack", + "defaultBranchRef": { "name": "main" } + }, + "prs": { + "3": { + "id": "PR_3", + "number": 3, + "url": "https://example.com/hack-dance/stack/pull/3", + "repo": "hack-dance/stack", + "headRefName": "feature/a", + "baseRefName": "main", + "state": "MERGED", + "isDraft": false + }, + "7": { + "id": "PR_7", + "number": 7, + "url": "https://example.com/hack-dance/stack/pull/7", + "repo": "hack-dance/stack", + "headRefName": "feature/a", + "baseRefName": "main", + "state": "OPEN", + "isDraft": false + } + }, + "next_number": 8 +}`) + + client := stackgh.NewClient(repo) + pr, err := client.FindPRByHead(context.Background(), "feature/a") + if err != nil { + t.Fatalf("find pr by head: %v", err) + } + if pr.Number != 7 { + t.Fatalf("expected open PR #7, got %+v", pr) + } +} + +func TestFindPRByHeadIgnoresHistoricalMatchesWithoutOpenPR(t *testing.T) { + repo := testutil.SetupGitRepo(t) + ghStub := testutil.SetupGHStub(t, "hack-dance/stack", "main") + t.Setenv("STACK_TEST_GH_STATE", ghStub.StatePath) + t.Setenv("STACK_TEST_GH_LOG", ghStub.LogPath) + t.Setenv("PATH", ghStub.Dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + testutil.WriteFile(t, ghStub.StatePath, `{ + "repo": { + "nameWithOwner": "hack-dance/stack", + "url": "https://github.com/hack-dance/stack", + "defaultBranchRef": { "name": "main" } + }, + "prs": { + "3": { + "id": "PR_3", + "number": 3, + "url": "https://example.com/hack-dance/stack/pull/3", + "repo": "hack-dance/stack", + "headRefName": "feature/a", + "baseRefName": "main", + "state": "MERGED", + "isDraft": false + }, + "4": { + "id": "PR_4", + "number": 4, + "url": "https://example.com/hack-dance/stack/pull/4", + "repo": "hack-dance/stack", + "headRefName": "feature/a", + "baseRefName": "main", + "state": "CLOSED", + "isDraft": false + } + }, + "next_number": 5 +}`) + + client := stackgh.NewClient(repo) + pr, err := client.FindPRByHead(context.Background(), "feature/a") + if err != nil { + t.Fatalf("find pr by head: %v", err) + } + if pr.Number != 0 { + t.Fatalf("expected no live PR match, got %+v", pr) + } +} + +func TestFindPRByHeadRejectsAmbiguousOpenMatches(t *testing.T) { + repo := testutil.SetupGitRepo(t) + ghStub := testutil.SetupGHStub(t, "hack-dance/stack", "main") + t.Setenv("STACK_TEST_GH_STATE", ghStub.StatePath) + t.Setenv("STACK_TEST_GH_LOG", ghStub.LogPath) + t.Setenv("PATH", ghStub.Dir+string(os.PathListSeparator)+os.Getenv("PATH")) + + testutil.WriteFile(t, ghStub.StatePath, `{ + "repo": { + "nameWithOwner": "hack-dance/stack", + "url": "https://github.com/hack-dance/stack", + "defaultBranchRef": { "name": "main" } + }, + "prs": { + "7": { + "id": "PR_7", + "number": 7, + "url": "https://example.com/hack-dance/stack/pull/7", + "repo": "hack-dance/stack", + "headRefName": "feature/a", + "baseRefName": "main", + "state": "OPEN", + "isDraft": false + }, + "8": { + "id": "PR_8", + "number": 8, + "url": "https://example.com/hack-dance/stack/pull/8", + "repo": "hack-dance/stack", + "headRefName": "feature/a", + "baseRefName": "feature/base", + "state": "OPEN", + "isDraft": false + } + }, + "next_number": 9 +}`) + + client := stackgh.NewClient(repo) + _, err := client.FindPRByHead(context.Background(), "feature/a") + if err == nil || !strings.Contains(err.Error(), "multiple open pull requests match head") { + t.Fatalf("expected ambiguous open-pr error, got %v", err) + } +}