diff --git a/internal/engine/ghstack.go b/internal/engine/ghstack.go new file mode 100644 index 0000000..e869813 --- /dev/null +++ b/internal/engine/ghstack.go @@ -0,0 +1,545 @@ +package engine + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "os/exec" + "sort" + "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 []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. +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) { + seg := mapSegment(prInfo, segment) + + if len(seg.prNumbers) < minStackSize { + continue + } + + 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(seg.branches, " -> "), err) + continue + } + if stack == nil { + continue + } + + for _, name := range seg.branches { + prInfo.Branches[name].StackNumber = stack.Number + } + if !quiet { + fmt.Printf("GitHub stack #%d: %s\n", stack.Number, strings.Join(seg.branches, " -> ")) + } + } +} + +// 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. +// +// 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 +} + +// 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 — 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 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 { + 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, nil, false +} + +// 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 +} + +// 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(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) { + // 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()) + 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(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 the create fails after the unstack succeeds, + // the PRs are left ungrouped — the same known gap noted on + // resolveDivergedStack, not one this path introduces. + return rebuildStack(recorded, prNumbers, baseByPR) + } + } + + if len(newAbove) == 0 { + return remote, nil + } + return ghAddToStack(existing, newAbove) +} + +// 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..5472684 --- /dev/null +++ b/internal/engine/ghstack_test.go @@ -0,0 +1,305 @@ +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. +// 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)) + } +} + +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 TestClassifyAgainstRemote(t *testing.T) { + tests := map[string]struct { + local, remote []int + wantBelow, wantAbove []int + wantFound bool + }{ + "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 { + gotBelow, gotAbove, gotFound := classifyAgainstRemote(tc.local, tc.remote) + if gotFound != tc.wantFound { + t.Errorf("%s: found = %v, want %v", name, gotFound, tc.wantFound) + continue + } + 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) + } +} + +// 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"}) + + 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) + } + } +} + +// 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) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("%s = %v, want %v", label, got, want) + } + } +} + +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) + } + // 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. + wantInts(t, "recorded", seg.recorded, []int{7}) +} + +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) { + 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 +// 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) + } + } +} 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 f4c02d1..261bd96 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 @@ -448,12 +465,28 @@ 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" } 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"` }