From 883e489a392a541f281c6e4595f676ac9356a6dc Mon Sep 17 00:00:00 2001 From: Adam Mustafa <178707+amustafa@users.noreply.github.com> Date: Thu, 30 Jul 2026 20:16:32 -0400 Subject: [PATCH 1/5] feat(submit): register stacks on GitHub after submit Adds the GitHub stacked-PRs REST integration (public preview, X-GitHub-Api-Version 2026-03-10). stackr already creates each PR with --base set to its parent branch, which is exactly the chain GitHub validates, so registering a stack is one API call after submit. - ghstack.go: create/get/add/unstack via 'gh api --input -' - linearSegments: decompose stackr's branch tree into the maximal linear runs GitHub's model requires, cutting at every fork so each child starts a fresh stack based on the fork point - syncGitHubStacks: best-effort, runs on every submit path, scoped to what was actually pushed; a repo without the preview warns instead of failing a submit whose PRs already exist - BranchPR.StackNumber persists the stack so later submits extend it The reconciliation policy for a stack that has diverged remotely is left as a documented TODO in resolveDivergedStack. --- internal/engine/ghstack.go | 363 ++++++++++++++++++++++++++++++++ internal/engine/ghstack_test.go | 141 +++++++++++++ internal/engine/submit.go | 57 +++-- internal/store/pr_info.go | 6 + 4 files changed, 549 insertions(+), 18 deletions(-) create mode 100644 internal/engine/ghstack.go create mode 100644 internal/engine/ghstack_test.go diff --git a/internal/engine/ghstack.go b/internal/engine/ghstack.go new file mode 100644 index 0000000..cc533d7 --- /dev/null +++ b/internal/engine/ghstack.go @@ -0,0 +1,363 @@ +package engine + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "os/exec" + "strings" + + "github.com/amustafa/stackr/internal/graph" + "github.com/amustafa/stackr/internal/store" +) + +// ghStackAPIVersion pins the REST API version that introduced the stacks +// endpoints. GitHub dates its API versions; without this header the stacks +// routes are not served. +const ghStackAPIVersion = "2026-03-10" + +// minStackSize is GitHub's floor: a stack is "two or more pull requests". +// A lone PR is a perfectly valid PR, just not a stack. +const minStackSize = 2 + +// GHStack mirrors the stack object returned by the GitHub REST API. +type GHStack struct { + ID int `json:"id"` + Number int `json:"number"` + NodeID string `json:"node_id"` + URL string `json:"url"` + Open bool `json:"open"` + Base struct { + Ref string `json:"ref"` + } `json:"base"` + PullRequests []struct { + Number int `json:"number"` + State string `json:"state"` + Draft bool `json:"draft"` + MergedAt *string `json:"merged_at"` + Head struct { + Ref string `json:"ref"` + SHA string `json:"sha"` + } `json:"head"` + } `json:"pull_requests"` +} + +// prNumbers returns the stack's member PR numbers in bottom-to-top order. +func (s *GHStack) prNumbers() []int { + nums := make([]int, 0, len(s.PullRequests)) + for _, pr := range s.PullRequests { + nums = append(nums, pr.Number) + } + return nums +} + +// ghStackAPI invokes `gh api` against a stacks endpoint. A nil body sends no +// request payload (GET); a non-nil body is marshalled to JSON on stdin, which +// avoids `gh`'s field-flag syntax entirely and keeps the array ordering exact. +// +// gh expands the {owner}/{repo} placeholders from the current repository, so +// stackr never has to resolve the remote itself. +func ghStackAPI(method, path string, body any) ([]byte, error) { + args := []string{"api", + "--method", method, + path, + "-H", "Accept: application/vnd.github+json", + "-H", "X-GitHub-Api-Version: " + ghStackAPIVersion, + } + + var stdin bytes.Buffer + if body != nil { + data, err := json.Marshal(body) + if err != nil { + return nil, fmt.Errorf("failed to encode request: %w", err) + } + stdin.Write(data) + args = append(args, "--input", "-") + } + + ctx, cancel := context.WithTimeout(context.Background(), ghTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "gh", args...) + cmd.Env = append(cmd.Environ(), "GH_PROMPT_DISABLED=1") + if body != nil { + cmd.Stdin = &stdin + } + + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + if ctx.Err() != nil { + return nil, fmt.Errorf("gh api %s timed out after %s", path, ghTimeout) + } + return nil, fmt.Errorf("gh api %s %s failed: %s", method, path, strings.TrimSpace(stderr.String())) + } + return stdout.Bytes(), nil +} + +// ghCreateStack registers a new stack from PR numbers ordered bottom to top. +// GitHub validates that each PR's base ref matches the previous PR's head ref +// and returns 422 if the chain does not hold. +func ghCreateStack(prNumbers []int) (*GHStack, error) { + out, err := ghStackAPI("POST", "repos/{owner}/{repo}/stacks", + map[string]any{"pull_requests": prNumbers}) + if err != nil { + return nil, err + } + var s GHStack + if err := json.Unmarshal(out, &s); err != nil { + return nil, fmt.Errorf("failed to parse stack response: %w", err) + } + return &s, nil +} + +// ghGetStack reads a stack by its stack number. +// Returns nil, nil when the stack no longer exists. +func ghGetStack(number int) (*GHStack, error) { + out, err := ghStackAPI("GET", fmt.Sprintf("repos/{owner}/{repo}/stacks/%d", number), nil) + if err != nil { + // A dissolved or merged-away stack is an expected state, not a failure. + if strings.Contains(err.Error(), "404") || strings.Contains(err.Error(), "Not Found") { + return nil, nil + } + return nil, err + } + var s GHStack + if err := json.Unmarshal(out, &s); err != nil { + return nil, fmt.Errorf("failed to parse stack response: %w", err) + } + return &s, nil +} + +// ghAddToStack appends PRs above the current top of an existing stack. +// Only the new PRs are sent, ordered from the current top upward. +func ghAddToStack(stackNumber int, prNumbers []int) (*GHStack, error) { + out, err := ghStackAPI("POST", fmt.Sprintf("repos/{owner}/{repo}/stacks/%d/add", stackNumber), + map[string]any{"pull_requests": prNumbers}) + if err != nil { + return nil, err + } + var s GHStack + if err := json.Unmarshal(out, &s); err != nil { + return nil, fmt.Errorf("failed to parse stack response: %w", err) + } + return &s, nil +} + +// ghUnstack dissolves a stack on GitHub, leaving its pull requests intact and +// their base refs untouched. Only the grouping goes away. +func ghUnstack(stackNumber int) error { + _, err := ghStackAPI("POST", fmt.Sprintf("repos/{owner}/{repo}/stacks/%d/unstack", stackNumber), nil) + return err +} + +// linearSegments decomposes the submitted branches into maximal linear runs, +// each of which can become one GitHub stack. +// +// stackr's graph is a tree — a branch may have several children — but a GitHub +// stack is strictly linear, and a PR may only belong to one stack. The two +// models therefore do not map one-to-one, and a fork has to be split. +// +// The rule is to cut at every fork and let each child start a fresh run whose +// base is the fork point: +// +// main <- A <- B <- C segments: [A B] [C] [D] +// \- D stacks: [A B] (C and D are single PRs, +// already based on B, so they are +// left unregistered until they grow) +// +// This keeps every registered stack valid on GitHub's terms and never puts a +// PR in two stacks. What it gives up is that GitHub shows no relationship +// between [A B] and the PRs sitting on top of B — the base refs still chain +// correctly, so nothing is broken, but the shared history is not visible as a +// single stack. That is a limitation of GitHub's linear model, not of the +// decomposition. +// +// A branch starts a new segment when its parent is outside the submitted set +// (nothing to attach to) or when its parent has more than one submitted child +// (a fork). Otherwise it extends its parent's segment. +func linearSegments(g *graph.Graph, submitted []string) [][]string { + inSet := make(map[string]bool, len(submitted)) + for _, name := range submitted { + b := g.Branches[name] + if b == nil || b.IsTrunk { + continue + } + inSet[name] = true + } + + childrenInSet := func(name string) []string { + var kids []string + for _, child := range g.ChildrenOf(name) { + if inSet[child] { + kids = append(kids, child) + } + } + return kids + } + + // Walk `submitted` in order so segments come out deterministically; callers + // push bottom-up, which makes the lowest branch of each run come first. + var segments [][]string + claimed := make(map[string]bool, len(inSet)) + + for _, name := range submitted { + if !inSet[name] || claimed[name] { + continue + } + parent := g.Branches[name].ParentBranchName + startsSegment := !inSet[parent] || len(childrenInSet(parent)) != 1 + if !startsSegment { + continue + } + + // Extend upward while the run stays unambiguously linear. + segment := []string{name} + claimed[name] = true + for { + kids := childrenInSet(segment[len(segment)-1]) + if len(kids) != 1 { + break + } + segment = append(segment, kids[0]) + claimed[kids[0]] = true + } + segments = append(segments, segment) + } + + return segments +} + +// syncGitHubStacks registers the submitted branches as stacks on GitHub so the +// remote reflects the local stack shape. +// +// Best-effort by design, matching ghMergedHeadBranches: the PRs are already +// created and pushed by the time this runs, so a repository without the preview +// enabled, an offline machine, or an older gh must not turn a successful submit +// into a failure. Problems are reported, not fatal. +func syncGitHubStacks(g *graph.Graph, prInfo *store.PRInfo, submitted []string, quiet bool) { + for _, segment := range linearSegments(g, submitted) { + // Map branches to PR numbers, dropping any branch that was pushed but + // has no PR yet — a stack can only contain pull requests. + var ( + prNumbers []int + branches []string + existing int + ) + for _, name := range segment { + pr := prInfo.Branches[name] + if pr == nil || pr.Number == 0 { + continue + } + prNumbers = append(prNumbers, pr.Number) + branches = append(branches, name) + if existing == 0 { + existing = pr.StackNumber + } + } + + if len(prNumbers) < minStackSize { + continue + } + + stack, err := reconcileStack(existing, prNumbers) + if err != nil { + fmt.Printf("Warning: could not sync GitHub stack for %s: %v\n", + strings.Join(branches, " -> "), err) + continue + } + if stack == nil { + continue + } + + for _, name := range branches { + prInfo.Branches[name].StackNumber = stack.Number + } + if !quiet { + fmt.Printf("GitHub stack #%d: %s\n", stack.Number, strings.Join(branches, " -> ")) + } + } +} + +// reconcileStack brings one segment's stack on GitHub in line with the local +// PR chain: creating it, extending it, or leaving it alone if it already +// matches. Returns nil when there is nothing to record. +func reconcileStack(existing int, prNumbers []int) (*GHStack, error) { + if existing == 0 { + return ghCreateStack(prNumbers) + } + + remote, err := ghGetStack(existing) + if err != nil { + return nil, err + } + if remote == nil { + // The recorded stack is gone (dissolved, or fully merged). Start over. + return ghCreateStack(prNumbers) + } + + // GitHub drops merged PRs out of the stack and retargets what remains, so + // the remote list is a suffix of ours, not necessarily an exact match. + // Anything we hold above the remote's top is what still needs adding. + remoteNums := remote.prNumbers() + top := 0 + if len(remoteNums) > 0 { + top = remoteNums[len(remoteNums)-1] + } + + newPRs := prNumbers + found := false + for i, n := range prNumbers { + if n == top { + newPRs = prNumbers[i+1:] + found = true + break + } + } + + if !found && len(remoteNums) > 0 { + // The remote stack's top is not in our segment at all: the two have + // genuinely diverged, not merely fallen behind. See resolveDivergedStack. + return resolveDivergedStack(existing, remote, prNumbers) + } + + if len(newPRs) == 0 { + return remote, nil + } + return ghAddToStack(existing, newPRs) +} + +// resolveDivergedStack decides what to do when the stack recorded on GitHub no +// longer lines up with the local segment — its top PR is not in our chain at +// all, so there is nothing to simply append to. +// +// This happens for good reasons and bad ones: +// - `sr reorder` / `sr move` / `sr fold` reshaped the local stack, so the +// segment now covers a different set of PRs than when it was registered. +// - Someone regrouped the PRs by hand in the web UI or with `gh stack`. +// - A PR mid-stack was closed rather than merged. +// +// The trade-off is whose view wins. Rebuilding (ghUnstack + ghCreateStack) +// makes GitHub mirror the local graph, which is the whole point of the +// integration — but it silently discards any deliberate grouping someone made +// on the remote. Leaving it alone (return remote, nil, after a warning) never +// destroys remote intent, but lets GitHub drift from local indefinitely with +// only a warning that is easy to miss in a long submit. +// +// TODO(implement): choose the reconciliation policy. +// +// Things worth weighing: stackr already treats the local graph as the source +// of truth everywhere else (see restack.go), which argues for rebuilding. But +// unstack-then-create is not atomic — if ghCreateStack fails after ghUnstack +// succeeds, the PRs are left ungrouped and prInfo still points at a dead stack +// number, so a failure path needs to at least clear the recorded number. +// Also consider whether a non-interactive submit should ever be destructive, +// given c.Interactive exists and is threaded through the submit flow. +func resolveDivergedStack(existing int, remote *GHStack, prNumbers []int) (*GHStack, error) { + return nil, fmt.Errorf( + "local stack %v has diverged from GitHub stack #%d %v — reconciliation policy not implemented", + prNumbers, existing, remote.prNumbers()) +} diff --git a/internal/engine/ghstack_test.go b/internal/engine/ghstack_test.go new file mode 100644 index 0000000..c5ed44b --- /dev/null +++ b/internal/engine/ghstack_test.go @@ -0,0 +1,141 @@ +package engine + +import ( + "strings" + "testing" + + "github.com/amustafa/stackr/internal/graph" +) + +// buildGraph wires a trunk plus a parent->children map into a Graph. +// Revisions are irrelevant to segmentation, so they are left empty. +func buildGraph(t *testing.T, trunk string, edges map[string][]string) *graph.Graph { + t.Helper() + g := graph.New() + g.AddTrunk(trunk, "") + // Add in topological order: a branch can only be added once its parent is. + added := map[string]bool{trunk: true} + for progress := true; progress; { + progress = false + for parent, children := range edges { + if !added[parent] { + continue + } + for _, child := range children { + if added[child] { + continue + } + if err := g.AddBranch(child, parent, "", ""); err != nil { + t.Fatalf("AddBranch(%s -> %s): %v", child, parent, err) + } + added[child] = true + progress = true + } + } + } + return g +} + +func formatSegments(segments [][]string) string { + var parts []string + for _, s := range segments { + parts = append(parts, "["+strings.Join(s, " ")+"]") + } + return strings.Join(parts, " ") +} + +func TestLinearSegments_LinearStackIsOneSegment(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{ + "main": {"a"}, + "a": {"b"}, + "b": {"c"}, + }) + + got := formatSegments(linearSegments(g, []string{"a", "b", "c"})) + if want := "[a b c]"; got != want { + t.Errorf("linearSegments = %s, want %s", got, want) + } +} + +// The case from the design discussion: main <- a <- b, with b forking into +// c and d. The fork must cut the run, and each child starts its own segment +// based on b — never one stack containing both c and d. +func TestLinearSegments_ForkCutsSegmentAtForkPoint(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{ + "main": {"a"}, + "a": {"b"}, + "b": {"c", "d"}, + }) + + got := formatSegments(linearSegments(g, []string{"a", "b", "c", "d"})) + if want := "[a b] [c] [d]"; got != want { + t.Errorf("linearSegments = %s, want %s", got, want) + } +} + +// A fork whose children have children of their own: each side becomes a real +// multi-PR stack rooted at the fork point. +func TestLinearSegments_ForkedBranchesExtendUpward(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{ + "main": {"a"}, + "a": {"b"}, + "b": {"c", "d"}, + "c": {"c2"}, + "d": {"d2"}, + }) + + got := formatSegments(linearSegments(g, []string{"a", "b", "c", "c2", "d", "d2"})) + if want := "[a b] [c c2] [d d2]"; got != want { + t.Errorf("linearSegments = %s, want %s", got, want) + } +} + +// Only part of the stack was submitted, so the run starts where the submitted +// set starts — its base is simply whatever the parent branch already is. +func TestLinearSegments_PartialSubmitStartsAtSubmittedRoot(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{ + "main": {"a"}, + "a": {"b"}, + "b": {"c"}, + }) + + got := formatSegments(linearSegments(g, []string{"b", "c"})) + if want := "[b c]"; got != want { + t.Errorf("linearSegments = %s, want %s", got, want) + } +} + +// A fork is only a fork if both children are actually being submitted. +// Submitting one side leaves a single unambiguous run. +func TestLinearSegments_UnsubmittedSiblingIsNotAFork(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{ + "main": {"a"}, + "a": {"b"}, + "b": {"c", "d"}, + }) + + got := formatSegments(linearSegments(g, []string{"a", "b", "c"})) + if want := "[a b c]"; got != want { + t.Errorf("linearSegments = %s, want %s", got, want) + } +} + +func TestLinearSegments_TrunkIsExcluded(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{ + "main": {"a"}, + "a": {"b"}, + }) + + got := formatSegments(linearSegments(g, []string{"main", "a", "b"})) + if want := "[a b]"; got != want { + t.Errorf("linearSegments = %s, want %s", got, want) + } +} + +func TestLinearSegments_NoBranchesYieldsNoSegments(t *testing.T) { + g := buildGraph(t, "main", map[string][]string{"main": {"a"}}) + + if segments := linearSegments(g, nil); len(segments) != 0 { + t.Errorf("linearSegments = %s, want no segments", formatSegments(segments)) + } +} diff --git a/internal/engine/submit.go b/internal/engine/submit.go index f4c02d1..72d5bab 100644 --- a/internal/engine/submit.go +++ b/internal/engine/submit.go @@ -140,19 +140,23 @@ func submitAI(c *context.Context, opts SubmitOpts) error { // submitStack pushes downstack ancestors, current branch, and upstack dependents. func submitStack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string) error { - if err := pushDownstack(c, opts, g, cfg, prInfo, current); err != nil { + var pushed []string + + if err := pushDownstack(c, opts, g, cfg, prInfo, current, &pushed); err != nil { return err } b := g.Branches[current] - if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName, &pushed); err != nil { return err } - if err := pushUpstack(c, opts, g, cfg, prInfo, current); err != nil { + if err := pushUpstack(c, opts, g, cfg, prInfo, current, &pushed); err != nil { return err } + syncGitHubStacks(g, prInfo, pushed, c.Quiet) + if err := c.Store.WritePRInfo(prInfo); err != nil { return err } @@ -212,11 +216,13 @@ func submitSingle(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *stor func submitExisting(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string, existing *PRResult) error { fmt.Printf("PR #%d already exists for %s (%s)\n", existing.Number, current, existing.URL) - if err := pushDownstack(c, opts, g, cfg, prInfo, current); err != nil { + var pushed []string + + if err := pushDownstack(c, opts, g, cfg, prInfo, current, &pushed); err != nil { return err } - if err := pushBranch(c, cfg, opts, prInfo, current, g.Branches[current].ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, current, g.Branches[current].ParentBranchName, &pushed); err != nil { return err } @@ -231,10 +237,12 @@ func submitExisting(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *st pr.Title = existing.Title pr.Draft = existing.Draft - if err := offerUpstack(c, opts, g, cfg, prInfo, current); err != nil { + if err := offerUpstack(c, opts, g, cfg, prInfo, current, &pushed); err != nil { return err } + syncGitHubStacks(g, prInfo, pushed, c.Quiet) + return c.Store.WritePRInfo(prInfo) } @@ -242,14 +250,16 @@ func submitExisting(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *st func submitNewBranch(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string) error { b := g.Branches[current] + var pushed []string + // Push downstack ancestors first (always, all modes). - if err := pushDownstack(c, opts, g, cfg, prInfo, current); err != nil { + if err := pushDownstack(c, opts, g, cfg, prInfo, current, &pushed); err != nil { return err } // Programmatic mode: title and body provided, skip prompts. if opts.Title != "" { - if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName, &pushed); err != nil { return err } if opts.DryRun { @@ -270,6 +280,7 @@ func submitNewBranch(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *s fmt.Printf("Created PR #%d: %s\n", result.Number, result.URL) } } + syncGitHubStacks(g, prInfo, pushed, c.Quiet) return c.Store.WritePRInfo(prInfo) } @@ -278,9 +289,10 @@ func submitNewBranch(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *s if !c.Quiet { fmt.Println("Non-interactive mode, pushing only") } - if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName, &pushed); err != nil { return err } + syncGitHubStacks(g, prInfo, pushed, c.Quiet) return c.Store.WritePRInfo(prInfo) } @@ -295,7 +307,7 @@ func submitNewBranch(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *s return err } - if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, current, b.ParentBranchName, &pushed); err != nil { return err } @@ -351,15 +363,17 @@ func submitNewBranch(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *s } // Offer to push upstack. - if err := offerUpstack(c, opts, g, cfg, prInfo, current); err != nil { + if err := offerUpstack(c, opts, g, cfg, prInfo, current, &pushed); err != nil { return err } + syncGitHubStacks(g, prInfo, pushed, c.Quiet) + return c.Store.WritePRInfo(prInfo) } // pushDownstack pushes all downstack ancestors of current (excluding current and trunk). -func pushDownstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string) error { +func pushDownstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string, pushed *[]string) error { downstack := g.Downstack(current) // Downstack returns [current, parent, grandparent, ...trunk]. // Push in bottom-up order (reverse), skip current (index 0) and trunk. @@ -375,7 +389,7 @@ func pushDownstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *sto } for _, name := range ancestors { a := g.Branches[name] - if err := pushBranch(c, cfg, opts, prInfo, name, a.ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, name, a.ParentBranchName, pushed); err != nil { return err } } @@ -383,7 +397,7 @@ func pushDownstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *sto } // pushUpstack pushes all upstack dependents of current (excluding current). -func pushUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string) error { +func pushUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string, pushed *[]string) error { upstack := g.Upstack(current) if len(upstack) <= 1 { return nil @@ -397,7 +411,7 @@ func pushUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store if ub == nil || ub.IsTrunk || ub.Frozen { continue } - if err := pushBranch(c, cfg, opts, prInfo, name, ub.ParentBranchName); err != nil { + if err := pushBranch(c, cfg, opts, prInfo, name, ub.ParentBranchName, pushed); err != nil { return err } } @@ -405,7 +419,7 @@ func pushUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store } // offerUpstack prompts to push upstack dependents (interactive mode only). -func offerUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string) error { +func offerUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *store.Config, prInfo *store.PRInfo, current string, pushed *[]string) error { if !c.Interactive { return nil } @@ -419,13 +433,16 @@ func offerUpstack(c *context.Context, opts SubmitOpts, g *graph.Graph, cfg *stor return err } if yes { - return pushUpstack(c, opts, g, cfg, prInfo, current) + return pushUpstack(c, opts, g, cfg, prInfo, current, pushed) } return nil } // pushBranch pushes a single branch to the remote and records basic metadata. -func pushBranch(c *context.Context, cfg *store.Config, opts SubmitOpts, prInfo *store.PRInfo, name, parent string) error { +// Successfully pushed branches are appended to `pushed` in bottom-up order, +// which is the set syncGitHubStacks later segments into GitHub stacks. Dry runs +// never contribute, so no stack is registered for work that was not pushed. +func pushBranch(c *context.Context, cfg *store.Config, opts SubmitOpts, prInfo *store.PRInfo, name, parent string, pushed *[]string) error { if opts.DryRun { fmt.Printf("[dry-run] Would push %s to %s/%s (base: %s)\n", name, cfg.Remote, name, parent) return nil @@ -454,6 +471,10 @@ func pushBranch(c *context.Context, cfg *store.Config, opts SubmitOpts, prInfo * } pr.Draft = opts.Draft + if pushed != nil { + *pushed = append(*pushed, name) + } + return nil } diff --git a/internal/store/pr_info.go b/internal/store/pr_info.go index 229fbf2..2d4167f 100644 --- a/internal/store/pr_info.go +++ b/internal/store/pr_info.go @@ -27,4 +27,10 @@ type BranchPR struct { URL string `json:"url,omitempty"` Draft bool `json:"draft,omitempty"` BaseBranch string `json:"baseBranch,omitempty"` + + // StackNumber is the GitHub stack this PR belongs to, or 0 if it is not + // registered on GitHub. Every member of a stack carries the same number, + // which is what lets a later submit find the stack and extend it rather + // than trying to create a duplicate. + StackNumber int `json:"stackNumber,omitempty"` } From 9fffe39bc5cf70c491f40b68f43c0c815d6494fe Mon Sep 17 00:00:00 2001 From: Adam Mustafa <178707+amustafa@users.noreply.github.com> Date: Thu, 30 Jul 2026 21:48:26 -0400 Subject: [PATCH 2/5] fix(ghstack): start a new stack when the recorded one has finished MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A stack whose PRs have all merged is not deleted — it stays queryable with open:false and an emptied member list. The nil check only caught an explicit 404, so a finished stack fell through to POST /add against a closed stack instead of starting a fresh one. Extracts stackNeedsRebuild and prsAboveTop as pure functions so the reconciliation decision is testable without the network, and names GHStackPR instead of an inline anonymous struct. Found by CodeRabbit on #22. --- internal/engine/ghstack.go | 81 ++++++++++++++++++++------------- internal/engine/ghstack_test.go | 65 ++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 32 deletions(-) diff --git a/internal/engine/ghstack.go b/internal/engine/ghstack.go index cc533d7..2983a23 100644 --- a/internal/engine/ghstack.go +++ b/internal/engine/ghstack.go @@ -31,16 +31,20 @@ type GHStack struct { Base struct { Ref string `json:"ref"` } `json:"base"` - PullRequests []struct { - Number int `json:"number"` - State string `json:"state"` - Draft bool `json:"draft"` - MergedAt *string `json:"merged_at"` - Head struct { - Ref string `json:"ref"` - SHA string `json:"sha"` - } `json:"head"` - } `json:"pull_requests"` + PullRequests []GHStackPR `json:"pull_requests"` +} + +// GHStackPR is one pull request as reported inside a stack. GitHub returns a +// trimmed view here, not the full PR object. +type GHStackPR struct { + Number int `json:"number"` + State string `json:"state"` + Draft bool `json:"draft"` + MergedAt *string `json:"merged_at"` + Head struct { + Ref string `json:"ref"` + SHA string `json:"sha"` + } `json:"head"` } // prNumbers returns the stack's member PR numbers in bottom-to-top order. @@ -285,6 +289,37 @@ func syncGitHubStacks(g *graph.Graph, prInfo *store.PRInfo, submitted []string, // reconcileStack brings one segment's stack on GitHub in line with the local // PR chain: creating it, extending it, or leaving it alone if it already // matches. Returns nil when there is nothing to record. +// stackNeedsRebuild reports whether a recorded stack can no longer be extended, +// so the next submit has to start a fresh one. +// +// A stack whose pull requests have all merged is NOT deleted: it stays +// queryable with open:false and an emptied member list. Checking only for a +// 404 would therefore miss the most common end-of-life case and leave us +// POSTing /add against a closed stack. +func stackNeedsRebuild(remote *GHStack) bool { + return remote == nil || !remote.Open || len(remote.PullRequests) == 0 +} + +// prsAboveTop splits prNumbers at the remote stack's current top, returning the +// PRs that sit above it — the ones still to be added — and whether the top was +// found in our chain at all. +// +// GitHub drops merged PRs out of a stack and retargets what remains, so the +// remote list is normally a suffix of ours rather than an exact match. A top we +// cannot find means the two have genuinely diverged, not merely fallen behind. +func prsAboveTop(prNumbers []int, remoteNums []int) (newPRs []int, found bool) { + if len(remoteNums) == 0 { + return prNumbers, true + } + top := remoteNums[len(remoteNums)-1] + for i, n := range prNumbers { + if n == top { + return prNumbers[i+1:], true + } + } + return nil, false +} + func reconcileStack(existing int, prNumbers []int) (*GHStack, error) { if existing == 0 { return ghCreateStack(prNumbers) @@ -294,31 +329,13 @@ func reconcileStack(existing int, prNumbers []int) (*GHStack, error) { if err != nil { return nil, err } - if remote == nil { - // The recorded stack is gone (dissolved, or fully merged). Start over. - return ghCreateStack(prNumbers) - } - // GitHub drops merged PRs out of the stack and retargets what remains, so - // the remote list is a suffix of ours, not necessarily an exact match. - // Anything we hold above the remote's top is what still needs adding. - remoteNums := remote.prNumbers() - top := 0 - if len(remoteNums) > 0 { - top = remoteNums[len(remoteNums)-1] - } - - newPRs := prNumbers - found := false - for i, n := range prNumbers { - if n == top { - newPRs = prNumbers[i+1:] - found = true - break - } + if stackNeedsRebuild(remote) { + return ghCreateStack(prNumbers) } - if !found && len(remoteNums) > 0 { + newPRs, found := prsAboveTop(prNumbers, remote.prNumbers()) + if !found { // The remote stack's top is not in our segment at all: the two have // genuinely diverged, not merely fallen behind. See resolveDivergedStack. return resolveDivergedStack(existing, remote, prNumbers) diff --git a/internal/engine/ghstack_test.go b/internal/engine/ghstack_test.go index c5ed44b..b5fe73b 100644 --- a/internal/engine/ghstack_test.go +++ b/internal/engine/ghstack_test.go @@ -139,3 +139,68 @@ func TestLinearSegments_NoBranchesYieldsNoSegments(t *testing.T) { t.Errorf("linearSegments = %s, want no segments", formatSegments(segments)) } } + +func openStack(prs ...int) *GHStack { + s := &GHStack{Number: 7, Open: true} + for _, n := range prs { + s.PullRequests = append(s.PullRequests, GHStackPR{Number: n, State: "open"}) + } + return s +} + +// A stack whose PRs have all merged stays queryable with open:false rather than +// 404ing, so it must be treated as finished — otherwise the next submit adds to +// a closed stack instead of starting a new one. +func TestStackNeedsRebuild(t *testing.T) { + closed := openStack(42, 43) + closed.Open = false + + emptied := openStack() + emptied.Open = true + + tests := map[string]struct { + stack *GHStack + want bool + }{ + "missing stack": {nil, true}, + "closed stack": {closed, true}, + "open but emptied": {emptied, true}, + "open with members": {openStack(42, 43), false}, + } + for name, tc := range tests { + if got := stackNeedsRebuild(tc.stack); got != tc.want { + t.Errorf("%s: stackNeedsRebuild = %v, want %v", name, got, tc.want) + } + } +} + +func TestPRsAboveTop(t *testing.T) { + tests := map[string]struct { + local, remote []int + wantNew []int + wantFound bool + }{ + "nothing new": {[]int{42, 43}, []int{42, 43}, []int{}, true}, + "one to append": {[]int{42, 43, 44}, []int{42, 43}, []int{44}, true}, + "remote lost merged": {[]int{42, 43, 44}, []int{43}, []int{44}, true}, + "empty remote": {[]int{42, 43}, nil, []int{42, 43}, true}, + "diverged": {[]int{42, 43}, []int{99}, nil, false}, + } + for name, tc := range tests { + gotNew, gotFound := prsAboveTop(tc.local, tc.remote) + if gotFound != tc.wantFound { + t.Errorf("%s: found = %v, want %v", name, gotFound, tc.wantFound) + continue + } + if len(gotNew) != len(tc.wantNew) { + t.Errorf("%s: newPRs = %v, want %v", name, gotNew, tc.wantNew) + continue + } + for i := range tc.wantNew { + if gotNew[i] != tc.wantNew[i] { + t.Errorf("%s: newPRs = %v, want %v", name, gotNew, tc.wantNew) + break + } + } + } +} From 472a4394b308059e09811bcb99514641a3e108ed Mon Sep 17 00:00:00 2001 From: Adam Mustafa <178707+amustafa@users.noreply.github.com> Date: Thu, 30 Jul 2026 23:32:57 -0400 Subject: [PATCH 3/5] fix(ghstack): retarget PR base refs and rebuild stacks when a PR is inserted below the current bottom MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs compounded to leave a restacked PR pointed at the wrong base indefinitely, confirmed against the actual amustafa/stackr repo (PR #22 stayed based on main after am/sync-worktree-cleanup was inserted below it): 1. reconcileStack/prsAboveTop only ever checked whether remote's *top* PR appeared somewhere in the local segment, then appended whatever sat above it. It never verified the full remote sequence actually lined up, so a PR inserted *below* the stack's current bottom (e.g. a restack onto a new base branch) was indistinguishable from "nothing changed" — the old top was still there, so reconcileStack returned early and never touched GitHub. Replaced with classifyAgainstRemote, which locates remote's whole PR list as a contiguous run and reports what's below and above it separately. 2. Nothing ever synced an existing PR's base ref to GitHub after the local parent changed — pushBranch only force-pushed commits and updated stackr's own local cache. Added ghUpdatePRBase (a raw REST PATCH; `gh pr edit --base` fails outright via an unrelated GraphQL project-cards deprecation) and call it from pushBranch whenever the recorded base drifts from the local parent. Those two don't compose for free: GitHub refuses to retarget a PR's base while it's grouped into a stack, and refuses to create a stack containing a PR that's already grouped into another one. So when classifyAgainstRemote's newBelow PRs are still open (a real insertion, not just GitHub having already dropped a merged one), reconcileStack now unstacks the old group, retries the base retarget for every PR in the chain, and only then rebuilds. --- internal/engine/ghstack.go | 120 ++++++++++++++++++++++++++++---- internal/engine/ghstack_test.go | 43 +++++++----- internal/engine/github.go | 29 ++++++++ internal/engine/submit.go | 12 ++++ 4 files changed, 171 insertions(+), 33 deletions(-) diff --git a/internal/engine/ghstack.go b/internal/engine/ghstack.go index 2983a23..dd51f74 100644 --- a/internal/engine/ghstack.go +++ b/internal/engine/ghstack.go @@ -251,6 +251,7 @@ func syncGitHubStacks(g *graph.Graph, prInfo *store.PRInfo, submitted []string, branches []string existing int ) + baseByPR := map[int]string{} for _, name := range segment { pr := prInfo.Branches[name] if pr == nil || pr.Number == 0 { @@ -258,6 +259,7 @@ func syncGitHubStacks(g *graph.Graph, prInfo *store.PRInfo, submitted []string, } prNumbers = append(prNumbers, pr.Number) branches = append(branches, name) + baseByPR[pr.Number] = pr.BaseBranch if existing == 0 { existing = pr.StackNumber } @@ -267,7 +269,7 @@ func syncGitHubStacks(g *graph.Graph, prInfo *store.PRInfo, submitted []string, continue } - stack, err := reconcileStack(existing, prNumbers) + stack, err := reconcileStack(existing, prNumbers, baseByPR) if err != nil { fmt.Printf("Warning: could not sync GitHub stack for %s: %v\n", strings.Join(branches, " -> "), err) @@ -300,27 +302,73 @@ func stackNeedsRebuild(remote *GHStack) bool { return remote == nil || !remote.Open || len(remote.PullRequests) == 0 } -// prsAboveTop splits prNumbers at the remote stack's current top, returning the -// PRs that sit above it — the ones still to be added — and whether the top was -// found in our chain at all. +// classifyAgainstRemote locates remote's entire PR sequence as a contiguous, +// exact-order run within prNumbers, and splits what's left into newBelow (what +// precedes the run) and newAbove (what follows it). found is false when +// remote's sequence cannot be located as a contiguous run at all — a genuine +// divergence, not just growth in one direction. // // GitHub drops merged PRs out of a stack and retargets what remains, so the -// remote list is normally a suffix of ours rather than an exact match. A top we -// cannot find means the two have genuinely diverged, not merely fallen behind. -func prsAboveTop(prNumbers []int, remoteNums []int) (newPRs []int, found bool) { +// remote list is normally a suffix of ours rather than an exact match — that +// shows up as a non-empty newBelow, same shape as a PR genuinely inserted +// below the stack's current bottom (e.g. a restack onto a new base branch). +// The two are positionally indistinguishable; callers must check newBelow's +// PR state to tell them apart before deciding whether a rebuild is needed. +func classifyAgainstRemote(prNumbers, remoteNums []int) (newBelow, newAbove []int, found bool) { if len(remoteNums) == 0 { - return prNumbers, true + return nil, prNumbers, true } + if len(remoteNums) > len(prNumbers) { + return nil, nil, false + } + top := remoteNums[len(remoteNums)-1] for i, n := range prNumbers { - if n == top { - return prNumbers[i+1:], true + if n != top { + continue + } + start := i - len(remoteNums) + 1 + if start < 0 { + continue + } + matched := true + for j, want := range remoteNums { + if prNumbers[start+j] != want { + matched = false + break + } + } + if matched { + return prNumbers[:start], prNumbers[i+1:], true } } - return nil, false + return nil, nil, false } -func reconcileStack(existing int, prNumbers []int) (*GHStack, error) { +// anyPROpen reports whether any of the given PR numbers is still open on +// GitHub, as opposed to merged or closed. +func anyPROpen(prNumbers []int) (bool, error) { + for _, n := range prNumbers { + out, err := ghStackAPI("GET", fmt.Sprintf("repos/{owner}/{repo}/pulls/%d", n), nil) + if err != nil { + return false, err + } + var pr struct { + State string `json:"state"` + } + if err := json.Unmarshal(out, &pr); err != nil { + return false, fmt.Errorf("failed to parse PR #%d response: %w", n, err) + } + if pr.State == "open" { + return true, nil + } + } + return false, nil +} + +// baseByPR maps each PR number to the base branch it should have, per the +// local graph — used only to re-sync base refs when a stack is rebuilt. +func reconcileStack(existing int, prNumbers []int, baseByPR map[int]string) (*GHStack, error) { if existing == 0 { return ghCreateStack(prNumbers) } @@ -334,17 +382,59 @@ func reconcileStack(existing int, prNumbers []int) (*GHStack, error) { return ghCreateStack(prNumbers) } - newPRs, found := prsAboveTop(prNumbers, remote.prNumbers()) + newBelow, newAbove, found := classifyAgainstRemote(prNumbers, remote.prNumbers()) if !found { // The remote stack's top is not in our segment at all: the two have // genuinely diverged, not merely fallen behind. See resolveDivergedStack. return resolveDivergedStack(existing, remote, prNumbers) } - if len(newPRs) == 0 { + if len(newBelow) > 0 { + // GitHub's stacks API can only extend a stack upward — ghAddToStack + // appends above the current top, there is no way to attach a PR below + // the existing base. That's fine when newBelow is only PRs GitHub + // already dropped for merging (the common case), but if any of them + // are still open, a PR has genuinely been inserted below the stack's + // current bottom (e.g. `sr restack` onto a new base branch), and the + // only way to reflect that on GitHub is to rebuild the stack outright. + open, err := anyPROpen(newBelow) + if err != nil { + return nil, err + } + if open { + // GitHub refuses to create a stack containing a PR that's already + // a member of another one, so the old grouping has to be + // dissolved first. Not atomic: if ghCreateStack fails after this + // succeeds, the PRs are left ungrouped and existing now points at + // a dead stack number — the same known gap noted on + // resolveDivergedStack, not one this path introduces. + if err := ghUnstack(existing); err != nil { + return nil, fmt.Errorf("could not unstack #%d before rebuilding: %w", existing, err) + } + + // ghCreateStack validates that each PR's base ref equals the + // previous PR's head ref. A PR's base can't be retargeted while + // it's grouped into a stack, so submit's own attempt to do this + // earlier necessarily failed for every PR here — retry now that + // the group is dissolved, or the create below fails right back + // with the same chain-validation error. + for _, n := range prNumbers { + base, ok := baseByPR[n] + if !ok || base == "" { + continue + } + if err := ghUpdatePRBase(n, base); err != nil { + fmt.Printf("Warning: could not retarget PR #%d to %s: %v\n", n, base, err) + } + } + return ghCreateStack(prNumbers) + } + } + + if len(newAbove) == 0 { return remote, nil } - return ghAddToStack(existing, newPRs) + return ghAddToStack(existing, newAbove) } // resolveDivergedStack decides what to do when the stack recorded on GitHub no diff --git a/internal/engine/ghstack_test.go b/internal/engine/ghstack_test.go index b5fe73b..3986093 100644 --- a/internal/engine/ghstack_test.go +++ b/internal/engine/ghstack_test.go @@ -174,33 +174,40 @@ func TestStackNeedsRebuild(t *testing.T) { } } -func TestPRsAboveTop(t *testing.T) { +func TestClassifyAgainstRemote(t *testing.T) { tests := map[string]struct { - local, remote []int - wantNew []int - wantFound bool + local, remote []int + wantBelow, wantAbove []int + wantFound bool }{ - "nothing new": {[]int{42, 43}, []int{42, 43}, []int{}, true}, - "one to append": {[]int{42, 43, 44}, []int{42, 43}, []int{44}, true}, - "remote lost merged": {[]int{42, 43, 44}, []int{43}, []int{44}, true}, - "empty remote": {[]int{42, 43}, nil, []int{42, 43}, true}, - "diverged": {[]int{42, 43}, []int{99}, nil, false}, + "nothing new": {[]int{42, 43}, []int{42, 43}, []int{}, []int{}, true}, + "one to append": {[]int{42, 43, 44}, []int{42, 43}, []int{}, []int{44}, true}, + "remote lost merged": {[]int{42, 43, 44}, []int{43}, []int{42}, []int{44}, true}, + "empty remote": {[]int{42, 43}, nil, []int{}, []int{42, 43}, true}, + "diverged": {[]int{42, 43}, []int{99}, nil, nil, false}, + "new PR below (#26)": {[]int{26, 22, 23}, []int{22, 23}, []int{26}, []int{}, true}, + "below and above": {[]int{26, 22, 23, 44}, []int{22, 23}, []int{26}, []int{44}, true}, + "remote longer than local": {[]int{42}, []int{41, 42}, nil, nil, false}, } for name, tc := range tests { - gotNew, gotFound := prsAboveTop(tc.local, tc.remote) + gotBelow, gotAbove, gotFound := classifyAgainstRemote(tc.local, tc.remote) if gotFound != tc.wantFound { t.Errorf("%s: found = %v, want %v", name, gotFound, tc.wantFound) continue } - if len(gotNew) != len(tc.wantNew) { - t.Errorf("%s: newPRs = %v, want %v", name, gotNew, tc.wantNew) - continue - } - for i := range tc.wantNew { - if gotNew[i] != tc.wantNew[i] { - t.Errorf("%s: newPRs = %v, want %v", name, gotNew, tc.wantNew) - break + checkInts := func(label string, got, want []int) { + if len(got) != len(want) { + t.Errorf("%s: %s = %v, want %v", name, label, got, want) + return + } + for i := range want { + if got[i] != want[i] { + t.Errorf("%s: %s = %v, want %v", name, label, got, want) + return + } } } + checkInts("newBelow", gotBelow, tc.wantBelow) + checkInts("newAbove", gotAbove, tc.wantAbove) } } diff --git a/internal/engine/github.go b/internal/engine/github.go index ebccd81..f15379a 100644 --- a/internal/engine/github.go +++ b/internal/engine/github.go @@ -71,6 +71,35 @@ func ghPRForBranch(branch string) (*PRResult, error) { return &result, nil } +// ghUpdatePRBase retargets an existing PR's base branch on GitHub. +// +// This is REST, not `gh pr edit --base`: that command's mutation also +// requests project-card data GitHub has since deprecated, which makes the +// whole edit fail with a GraphQL error even though the base-branch change +// itself would have succeeded. PATCHing the REST endpoint directly sidesteps +// that field entirely. +func ghUpdatePRBase(number int, base string) error { + ctx, cancel := context.WithTimeout(context.Background(), ghTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, "gh", "api", + "--method", "PATCH", + fmt.Sprintf("repos/{owner}/{repo}/pulls/%d", number), + "-f", "base="+base) + cmd.Env = append(cmd.Environ(), "GH_PROMPT_DISABLED=1") + + var stderr bytes.Buffer + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + if ctx.Err() != nil { + return fmt.Errorf("gh api PATCH pulls/%d timed out after %s", number, ghTimeout) + } + return fmt.Errorf("gh api PATCH pulls/%d failed: %s", number, strings.TrimSpace(stderr.String())) + } + return nil +} + // ghMergedHeadBranches returns the set of head-branch names whose PRs are // merged, in a single batched query. // diff --git a/internal/engine/submit.go b/internal/engine/submit.go index 72d5bab..261bd96 100644 --- a/internal/engine/submit.go +++ b/internal/engine/submit.go @@ -465,6 +465,18 @@ func pushBranch(c *context.Context, cfg *store.Config, opts SubmitOpts, prInfo * prInfo.Branches[name] = &store.BranchPR{} } pr := prInfo.Branches[name] + + // The local parent is the source of truth; retarget the PR's base to + // match whenever it drifted (e.g. a restack reparented this branch). + // Best-effort: this fails while the PR is grouped into a GitHub stack + // (its base can't change mid-group), which syncGitHubStacks handles by + // unstacking and retrying once it detects the same drift below. + if pr.Number != 0 && pr.BaseBranch != "" && pr.BaseBranch != parent { + if err := ghUpdatePRBase(pr.Number, parent); err != nil && c.Debug { + fmt.Printf("Note: could not retarget PR #%d to %s yet: %v\n", pr.Number, parent, err) + } + } + pr.BaseBranch = parent if pr.State == "" { pr.State = "open" From 89f73a5c589aacd392b6d2e5f2a996af484446ac Mon Sep 17 00:00:00 2001 From: Adam Mustafa <178707+amustafa@users.noreply.github.com> Date: Fri, 31 Jul 2026 20:24:05 -0400 Subject: [PATCH 4/5] fix(ghstack): dissolve every recorded stack before rebuilding Addresses three CodeRabbit findings on #22 that share one root cause: stackr created a GitHub stack without first dissolving the stacks its PRs already belonged to, and GitHub allows a PR in only one stack. - The segment scan kept only the first non-zero StackNumber, so a segment spanning two recorded stacks picked one and failed /add forever. - The stackNeedsRebuild path created without unstacking, but a closed stack retains its members until explicitly unstacked. - reconcileStack's doc comment was attached to stackNeedsRebuild. --- internal/engine/ghstack.go | 187 ++++++++++++++++++++++---------- internal/engine/ghstack_test.go | 85 +++++++++++++++ 2 files changed, 216 insertions(+), 56 deletions(-) diff --git a/internal/engine/ghstack.go b/internal/engine/ghstack.go index dd51f74..e869813 100644 --- a/internal/engine/ghstack.go +++ b/internal/engine/ghstack.go @@ -6,6 +6,7 @@ import ( "encoding/json" "fmt" "os/exec" + "sort" "strings" "github.com/amustafa/stackr/internal/graph" @@ -244,53 +245,126 @@ func linearSegments(g *graph.Graph, submitted []string) [][]string { // into a failure. Problems are reported, not fatal. func syncGitHubStacks(g *graph.Graph, prInfo *store.PRInfo, submitted []string, quiet bool) { for _, segment := range linearSegments(g, submitted) { - // Map branches to PR numbers, dropping any branch that was pushed but - // has no PR yet — a stack can only contain pull requests. - var ( - prNumbers []int - branches []string - existing int - ) - baseByPR := map[int]string{} - for _, name := range segment { - pr := prInfo.Branches[name] - if pr == nil || pr.Number == 0 { - continue - } - prNumbers = append(prNumbers, pr.Number) - branches = append(branches, name) - baseByPR[pr.Number] = pr.BaseBranch - if existing == 0 { - existing = pr.StackNumber - } - } + seg := mapSegment(prInfo, segment) - if len(prNumbers) < minStackSize { + if len(seg.prNumbers) < minStackSize { continue } - stack, err := reconcileStack(existing, prNumbers, baseByPR) + stack, err := reconcileStack(seg.recorded, seg.prNumbers, seg.baseByPR) if err != nil { fmt.Printf("Warning: could not sync GitHub stack for %s: %v\n", - strings.Join(branches, " -> "), err) + strings.Join(seg.branches, " -> "), err) continue } if stack == nil { continue } - for _, name := range branches { + for _, name := range seg.branches { prInfo.Branches[name].StackNumber = stack.Number } if !quiet { - fmt.Printf("GitHub stack #%d: %s\n", stack.Number, strings.Join(branches, " -> ")) + fmt.Printf("GitHub stack #%d: %s\n", stack.Number, strings.Join(seg.branches, " -> ")) } } } -// reconcileStack brings one segment's stack on GitHub in line with the local -// PR chain: creating it, extending it, or leaving it alone if it already -// matches. Returns nil when there is nothing to record. +// segmentPRs is one linear segment resolved against the recorded PR metadata. +type segmentPRs struct { + branches []string // branches that have a PR, bottom-up + prNumbers []int // their PR numbers, same order + baseByPR map[int]string // PR number -> the base branch the local graph wants + recorded []int // every stack these PRs are already registered against +} + +// mapSegment resolves a segment's branches to pull requests, dropping any branch +// that was pushed but has no PR yet — a stack can only contain pull requests. +// +// recorded collects EVERY distinct stack number, not just the first one found. +// Local reshaping (`sr move`, `sr fold`, `sr reorder`) can merge two previously +// separate segments into one, and GitHub allows a PR in only one stack — so a +// segment spanning two recorded stacks has to dissolve both before it can be +// regrouped. Keeping only the first would make every subsequent submit fail the +// same way, with no path back to a consistent state. +func mapSegment(prInfo *store.PRInfo, segment []string) segmentPRs { + seg := segmentPRs{baseByPR: map[int]string{}} + recorded := map[int]bool{} + + for _, name := range segment { + pr := prInfo.Branches[name] + if pr == nil || pr.Number == 0 { + continue + } + seg.prNumbers = append(seg.prNumbers, pr.Number) + seg.branches = append(seg.branches, name) + seg.baseByPR[pr.Number] = pr.BaseBranch + if pr.StackNumber != 0 { + recorded[pr.StackNumber] = true + } + } + + seg.recorded = sortedKeys(recorded) + return seg +} + +// rebuildStack regroups a segment from scratch: dissolve every stack its pull +// requests are currently registered against, re-point their base refs, then +// create the new one. +// +// Dissolving first is not optional. GitHub allows a pull request to belong to +// only one stack, and a stack keeps its members even once it is closed — only an +// explicit unstack removes them. Creating without dissolving is therefore +// rejected for any PR GitHub still considers grouped, which is the shape of +// every failure this path exists to recover from. +// +// A stack that has already gone away is not an error: absent is exactly the +// state unstacking was meant to reach. +func rebuildStack(recorded []int, prNumbers []int, baseByPR map[int]string) (*GHStack, error) { + for _, n := range recorded { + if err := ghUnstack(n); err != nil { + if isMissingStack(err) { + continue + } + return nil, fmt.Errorf("could not unstack #%d before rebuilding: %w", n, err) + } + } + + // ghCreateStack validates that each PR's base ref equals the previous PR's + // head ref. A PR's base cannot be retargeted while it is grouped into a + // stack, so submit's own attempt to do this earlier necessarily failed for + // every PR here — retry now that the groups are dissolved, or the create + // fails right back with the same chain-validation error. + for _, n := range prNumbers { + base, ok := baseByPR[n] + if !ok || base == "" { + continue + } + if err := ghUpdatePRBase(n, base); err != nil { + fmt.Printf("Warning: could not retarget PR #%d to %s: %v\n", n, base, err) + } + } + + return ghCreateStack(prNumbers) +} + +// isMissingStack reports whether an error means the stack is already gone. +func isMissingStack(err error) bool { + msg := err.Error() + return strings.Contains(msg, "404") || strings.Contains(msg, "Not Found") +} + +// sortedKeys returns a set's members in ascending order, so stacks are +// dissolved in a deterministic order and warnings read the same way twice. +func sortedKeys(set map[int]bool) []int { + out := make([]int, 0, len(set)) + for n := range set { + out = append(out, n) + } + sort.Ints(out) + return out +} + // stackNeedsRebuild reports whether a recorded stack can no longer be extended, // so the next submit has to start a fresh one. // @@ -366,20 +440,41 @@ func anyPROpen(prNumbers []int) (bool, error) { return false, nil } +// reconcileStack brings one segment's stack on GitHub in line with the local +// PR chain: creating it, extending it, or leaving it alone if it already +// matches. Returns nil when there is nothing to record. +// +// recorded holds every stack number the segment's PRs are currently registered +// against. Normally that is zero or one, but local reshaping can merge two +// previously separate segments, and GitHub allows a PR in only one stack — so +// more than one means the group has to be rebuilt from scratch rather than +// extended. +// // baseByPR maps each PR number to the base branch it should have, per the // local graph — used only to re-sync base refs when a stack is rebuilt. -func reconcileStack(existing int, prNumbers []int, baseByPR map[int]string) (*GHStack, error) { - if existing == 0 { +func reconcileStack(recorded []int, prNumbers []int, baseByPR map[int]string) (*GHStack, error) { + if len(recorded) == 0 { return ghCreateStack(prNumbers) } + // A segment spanning several recorded stacks cannot be extended into any one + // of them: /add would be rejected for every PR that belongs to another. + if len(recorded) > 1 { + return rebuildStack(recorded, prNumbers, baseByPR) + } + + existing := recorded[0] remote, err := ghGetStack(existing) if err != nil { return nil, err } if stackNeedsRebuild(remote) { - return ghCreateStack(prNumbers) + // Rebuild, not create. A closed stack keeps its members — only an + // explicit unstack dissolves one — so creating straight away would be + // rejected for PRs GitHub still considers grouped. rebuildStack + // tolerates a stack that has genuinely gone away. + return rebuildStack(recorded, prNumbers, baseByPR) } newBelow, newAbove, found := classifyAgainstRemote(prNumbers, remote.prNumbers()) @@ -402,32 +497,12 @@ func reconcileStack(existing int, prNumbers []int, baseByPR map[int]string) (*GH return nil, err } if open { - // GitHub refuses to create a stack containing a PR that's already - // a member of another one, so the old grouping has to be - // dissolved first. Not atomic: if ghCreateStack fails after this - // succeeds, the PRs are left ungrouped and existing now points at - // a dead stack number — the same known gap noted on + // GitHub refuses to create a stack containing a PR that's already a + // member of another one, so the old grouping has to be dissolved + // first. Not atomic: if the create fails after the unstack succeeds, + // the PRs are left ungrouped — the same known gap noted on // resolveDivergedStack, not one this path introduces. - if err := ghUnstack(existing); err != nil { - return nil, fmt.Errorf("could not unstack #%d before rebuilding: %w", existing, err) - } - - // ghCreateStack validates that each PR's base ref equals the - // previous PR's head ref. A PR's base can't be retargeted while - // it's grouped into a stack, so submit's own attempt to do this - // earlier necessarily failed for every PR here — retry now that - // the group is dissolved, or the create below fails right back - // with the same chain-validation error. - for _, n := range prNumbers { - base, ok := baseByPR[n] - if !ok || base == "" { - continue - } - if err := ghUpdatePRBase(n, base); err != nil { - fmt.Printf("Warning: could not retarget PR #%d to %s: %v\n", n, base, err) - } - } - return ghCreateStack(prNumbers) + return rebuildStack(recorded, prNumbers, baseByPR) } } diff --git a/internal/engine/ghstack_test.go b/internal/engine/ghstack_test.go index 3986093..aeea44b 100644 --- a/internal/engine/ghstack_test.go +++ b/internal/engine/ghstack_test.go @@ -1,10 +1,12 @@ package engine import ( + "errors" "strings" "testing" "github.com/amustafa/stackr/internal/graph" + "github.com/amustafa/stackr/internal/store" ) // buildGraph wires a trunk plus a parent->children map into a Graph. @@ -211,3 +213,86 @@ func TestClassifyAgainstRemote(t *testing.T) { checkInts("newAbove", gotAbove, tc.wantAbove) } } + +// A segment can come to span two previously separate GitHub stacks — `sr move`, +// `sr fold` and `sr reorder` all merge segments. Keeping only the first recorded +// number would make ghAddToStack fail for every PR belonging to the other one, +// and since nothing would then update, every later submit would fail identically. +func TestMapSegment_CollectsEveryRecordedStack(t *testing.T) { + prInfo := &store.PRInfo{Branches: map[string]*store.BranchPR{ + "a": {Number: 42, BaseBranch: "main", StackNumber: 0}, + "b": {Number: 43, BaseBranch: "a", StackNumber: 7}, + "c": {Number: 44, BaseBranch: "b", StackNumber: 9}, + }} + + seg := mapSegment(prInfo, []string{"a", "b", "c"}) + + if len(seg.recorded) != 2 || seg.recorded[0] != 7 || seg.recorded[1] != 9 { + t.Fatalf("recorded = %v, want both stacks [7 9] so the rebuild can dissolve each", seg.recorded) + } + if len(seg.prNumbers) != 3 { + t.Errorf("prNumbers = %v, want all three", seg.prNumbers) + } + if seg.baseByPR[43] != "a" { + t.Errorf("baseByPR[43] = %q, want the local graph's parent", seg.baseByPR[43]) + } +} + +func TestMapSegment_SkipsBranchesWithoutAPR(t *testing.T) { + prInfo := &store.PRInfo{Branches: map[string]*store.BranchPR{ + "a": {Number: 42, StackNumber: 7}, + "b": {Number: 0}, // pushed, no PR yet + "c": {Number: 44, StackNumber: 7}, + }} + + seg := mapSegment(prInfo, []string{"a", "b", "c"}) + + if len(seg.branches) != 2 || seg.branches[0] != "a" || seg.branches[1] != "c" { + t.Fatalf("branches = %v, want only those with a PR", seg.branches) + } + // One distinct stack, so this segment can still be extended rather than rebuilt. + if len(seg.recorded) != 1 || seg.recorded[0] != 7 { + t.Errorf("recorded = %v, want [7]", seg.recorded) + } +} + +func TestMapSegment_NoRecordedStacksMeansCreateFresh(t *testing.T) { + prInfo := &store.PRInfo{Branches: map[string]*store.BranchPR{ + "a": {Number: 42}, + "b": {Number: 43}, + }} + + seg := mapSegment(prInfo, []string{"a", "b"}) + if len(seg.recorded) != 0 { + t.Errorf("recorded = %v, want empty so reconcileStack creates rather than rebuilds", seg.recorded) + } +} + +func TestSortedKeys_IsDeterministic(t *testing.T) { + got := sortedKeys(map[int]bool{9: true, 7: true, 12: true}) + want := []int{7, 9, 12} + if len(got) != len(want) { + t.Fatalf("sortedKeys = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("sortedKeys = %v, want %v", got, want) + } + } +} + +// A stack that has already gone away is the state unstacking was trying to +// reach, so rebuildStack must not treat it as a failure. +func TestIsMissingStack(t *testing.T) { + cases := map[string]bool{ + "gh api POST repos/x/y/stacks/7/unstack failed: HTTP 404: Not Found": true, + "gh api ... failed: 404": true, + "gh api ... failed: HTTP 422: Unprocessable Entity": false, + "gh api ... timed out after 30s": false, + } + for msg, want := range cases { + if got := isMissingStack(errors.New(msg)); got != want { + t.Errorf("isMissingStack(%q) = %v, want %v", msg, got, want) + } + } +} From 4f7b54651497e1a3c17b8ce837052ea19275731b Mon Sep 17 00:00:00 2001 From: Adam Mustafa <178707+amustafa@users.noreply.github.com> Date: Sat, 1 Aug 2026 00:08:39 -0400 Subject: [PATCH 5/5] test(ghstack): assert mapSegment's values, not just its lengths --- internal/engine/ghstack_test.go | 45 +++++++++++++++++++-------------- 1 file changed, 26 insertions(+), 19 deletions(-) diff --git a/internal/engine/ghstack_test.go b/internal/engine/ghstack_test.go index aeea44b..5472684 100644 --- a/internal/engine/ghstack_test.go +++ b/internal/engine/ghstack_test.go @@ -227,14 +227,29 @@ func TestMapSegment_CollectsEveryRecordedStack(t *testing.T) { seg := mapSegment(prInfo, []string{"a", "b", "c"}) - if len(seg.recorded) != 2 || seg.recorded[0] != 7 || seg.recorded[1] != 9 { - t.Fatalf("recorded = %v, want both stacks [7 9] so the rebuild can dissolve each", seg.recorded) + wantInts(t, "recorded", seg.recorded, []int{7, 9}) + // Order matters: ghCreateStack validates the chain bottom-to-top, so a + // reordered or mismapped PR list fails at GitHub rather than here. + wantInts(t, "prNumbers", seg.prNumbers, []int{42, 43, 44}) + + for pr, want := range map[int]string{42: "main", 43: "a", 44: "b"} { + if got := seg.baseByPR[pr]; got != want { + t.Errorf("baseByPR[%d] = %q, want %q", pr, got, want) + } } - if len(seg.prNumbers) != 3 { - t.Errorf("prNumbers = %v, want all three", seg.prNumbers) +} + +// wantInts compares an int slice element by element, so a test cannot pass on +// length alone while mapping the wrong values. +func wantInts(t *testing.T, label string, got, want []int) { + t.Helper() + if len(got) != len(want) { + t.Fatalf("%s = %v, want %v", label, got, want) } - if seg.baseByPR[43] != "a" { - t.Errorf("baseByPR[43] = %q, want the local graph's parent", seg.baseByPR[43]) + for i := range want { + if got[i] != want[i] { + t.Fatalf("%s = %v, want %v", label, got, want) + } } } @@ -250,10 +265,11 @@ func TestMapSegment_SkipsBranchesWithoutAPR(t *testing.T) { if len(seg.branches) != 2 || seg.branches[0] != "a" || seg.branches[1] != "c" { t.Fatalf("branches = %v, want only those with a PR", seg.branches) } + // The PR list must stay aligned with the branch list, or the stack is + // registered with the wrong members. + wantInts(t, "prNumbers", seg.prNumbers, []int{42, 44}) // One distinct stack, so this segment can still be extended rather than rebuilt. - if len(seg.recorded) != 1 || seg.recorded[0] != 7 { - t.Errorf("recorded = %v, want [7]", seg.recorded) - } + wantInts(t, "recorded", seg.recorded, []int{7}) } func TestMapSegment_NoRecordedStacksMeansCreateFresh(t *testing.T) { @@ -269,16 +285,7 @@ func TestMapSegment_NoRecordedStacksMeansCreateFresh(t *testing.T) { } func TestSortedKeys_IsDeterministic(t *testing.T) { - got := sortedKeys(map[int]bool{9: true, 7: true, 12: true}) - want := []int{7, 9, 12} - if len(got) != len(want) { - t.Fatalf("sortedKeys = %v, want %v", got, want) - } - for i := range want { - if got[i] != want[i] { - t.Fatalf("sortedKeys = %v, want %v", got, want) - } - } + wantInts(t, "sortedKeys", sortedKeys(map[int]bool{9: true, 7: true, 12: true}), []int{7, 9, 12}) } // A stack that has already gone away is the state unstacking was trying to