diff --git a/.changes/unreleased/Added-20260805-045615.yaml b/.changes/unreleased/Added-20260805-045615.yaml new file mode 100644 index 000000000..7d6c15963 --- /dev/null +++ b/.changes/unreleased/Added-20260805-045615.yaml @@ -0,0 +1,3 @@ +kind: Added +body: 'submit: Eligible GitHub pull requests are registered as native stacks after submission.' +time: 2026-08-05T04:56:15.840512-07:00 diff --git a/internal/handler/submit/handler.go b/internal/handler/submit/handler.go index 660947014..95e62cb3e 100644 --- a/internal/handler/submit/handler.go +++ b/internal/handler/submit/handler.go @@ -236,6 +236,7 @@ func (h *Handler) SubmitBatch(ctx context.Context, req *BatchRequest) error { return err } + stackUpdates := new(submitStackUpdates) var branchesToComment []string for _, branch := range req.Branches { // Shallow copy the options because submitBranch may modify them. @@ -244,7 +245,7 @@ func (h *Handler) SubmitBatch(ctx context.Context, req *BatchRequest) error { ctx, graph, branch, - &submitOptions{Options: &opts}, + &submitOptions{Options: &opts, stackUpdates: stackUpdates}, ) if err != nil { return fmt.Errorf("submit branch %s: %w", branch, err) @@ -258,7 +259,8 @@ func (h *Handler) SubmitBatch(ctx context.Context, req *BatchRequest) error { return nil // nothing to do } - return updateNavigationComments( + stackErr := h.updateStacks(ctx, branchesToComment, stackUpdates) + navCommentErr := updateNavigationComments( ctx, h.Store, h.Service, h.Log, opts.NavComment, @@ -271,6 +273,7 @@ func (h *Handler) SubmitBatch(ctx context.Context, req *BatchRequest) error { h.upstreamRepository, h.pushRepositoryID, ) + return errors.Join(stackErr, navCommentErr) } // Request is a request to submit a single branch to a remote repository. @@ -306,14 +309,16 @@ func (h *Handler) Submit(ctx context.Context, req *Request) error { return err } + stackUpdates := new(submitStackUpdates) status, err := h.submitBranch( ctx, graph, req.Branch, &submitOptions{ - Options: opts, - Title: req.Title, - Body: req.Body, + Options: opts, + Title: req.Title, + Body: req.Body, + stackUpdates: stackUpdates, }, ) if err != nil { @@ -325,7 +330,8 @@ func (h *Handler) Submit(ctx context.Context, req *Request) error { return nil } - return updateNavigationComments( + stackErr := h.updateStacks(ctx, []string{req.Branch}, stackUpdates) + navCommentErr := updateNavigationComments( ctx, h.Store, h.Service, h.Log, opts.NavComment, @@ -338,6 +344,7 @@ func (h *Handler) Submit(ctx context.Context, req *Request) error { h.upstreamRepository, h.pushRepositoryID, ) + return errors.Join(stackErr, navCommentErr) } type submitStatus struct { @@ -352,7 +359,8 @@ type submitStatus struct { type submitOptions struct { *Options - Title, Body string + Title, Body string + stackUpdates *submitStackUpdates } func (h *Handler) submitBranch( @@ -929,18 +937,31 @@ func (h *Handler) submitBranch( AddReviewers: reviewers, AddAssignees: opts.Assignees, } - // Some forges, including GitHub, treat setting an unchanged base - // as a mutation and may trigger redundant CI runs. - if pull.BaseName != upstreamBase { - editOpts.Base = upstreamBase - } - // remoteRepo is guaranteed to be available at this point. remoteRepo, err := h.upstreamRepository(ctx) if err != nil { return status, fmt.Errorf("edit CR %v: %w", pull.ID, err) } + // A native stack provider may need to dissolve existing membership + // before changing the pull request base. Defer that mutation to its + // planned transition; unsupported planning applies the same ordinary + // EditChange after submission. + if pull.BaseName != upstreamBase { + if _, ok := remoteRepo.(forge.StackRepository); ok && opts.stackUpdates != nil { + opts.stackUpdates.deferredBases = append( + opts.stackUpdates.deferredBases, + deferredBaseUpdate{ + repository: remoteRepo, + change: pull.ID, + base: upstreamBase, + }, + ) + } else { + editOpts.Base = upstreamBase + } + } + if err := remoteRepo.EditChange(ctx, pull.ID, editOpts); err != nil { return status, fmt.Errorf("edit CR %v: %w", pull.ID, err) } diff --git a/internal/handler/submit/stacks.go b/internal/handler/submit/stacks.go new file mode 100644 index 000000000..501f6248b --- /dev/null +++ b/internal/handler/submit/stacks.go @@ -0,0 +1,126 @@ +package submit + +import ( + "context" + "errors" + "fmt" + + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/spice" +) + +// updateStacks plans and executes the forge-native representation of the +// tracked stack trees affected by a successful submit. Unsupported planning +// applies the provider base edits deferred by the submit loop. +func (h *Handler) updateStacks( + ctx context.Context, + submitted []string, + stackUpdates *submitStackUpdates, +) error { + repo, err := h.upstreamRepository(ctx) + if err != nil { + return fmt.Errorf("get remote repository: %w", err) + } + + stackRepo, ok := repo.(forge.StackRepository) + if !ok { + return nil + } + + // Submission can create change metadata after the command builds its first + // branch graph. + // Reload it so newly published changes participate in the update. + graph, err := h.Service.BranchGraph(ctx, nil) + if err != nil { + return fmt.Errorf("build branch graph: %w", err) + } + + changes := nativeStackChanges(graph, repo.Forge().ID(), submitted) + if len(changes) == 0 { + return nil + } + + plan, err := stackRepo.PlanStackUpdate(ctx, changes) + if errors.Is(err, forge.ErrUnsupported) { + return stackUpdates.applyDeferredBases(ctx) + } + if err != nil { + h.Log.Warn("Could not plan stack update", "error", err) + return nil + } + if err := plan.Execute(ctx); err != nil { + h.Log.Warn("Could not update stacks", "error", err) + } + return nil +} + +type submitStackUpdates struct { + deferredBases []deferredBaseUpdate +} + +type deferredBaseUpdate struct { + repository forge.Repository + change forge.ChangeID + base string +} + +func (u *submitStackUpdates) applyDeferredBases(ctx context.Context) error { + var errs []error + for _, update := range u.deferredBases { + if err := update.repository.EditChange(ctx, update.change, forge.EditChangeOptions{ + Base: update.base, + }); err != nil { + errs = append(errs, fmt.Errorf("update %v base: %w", update.change, err)) + } + } + return errors.Join(errs...) +} + +// nativeStackChanges projects every published change in a tree containing a +// submitted branch into the forge's native-stack representation. +// +// A submission may affect any branch in the same tree: adding or updating one +// change can complete a relationship elsewhere in its divergent upstack. The +// projection therefore starts at each submitted branch's bottom and retains +// all published changes for the target forge. If a branch's base is absent +// from that projection, the forge contract treats the branch as a tree root. +func nativeStackChanges( + graph *spice.BranchGraph, + forgeID string, + submitted []string, +) []forge.StackChange { + affectedBranches := make(map[string]struct{}) + for _, branch := range submitted { + for member := range graph.Upstack(graph.Bottom(branch)) { + affectedBranches[member] = struct{}{} + } + } + + changeByBranch := make(map[string]forge.ChangeID, len(affectedBranches)) + for branch := range graph.All() { + if _, ok := affectedBranches[branch.Name]; !ok || branch.Change == nil { + continue + } + if branch.Change.ForgeID() == forgeID { + changeByBranch[branch.Name] = branch.Change.ChangeID() + } + } + + changes := make([]forge.StackChange, 0, len(changeByBranch)) + for branch := range graph.All() { + change, ok := changeByBranch[branch.Name] + if !ok { + continue + } + baseBranch := branch.Base + if base, ok := graph.Lookup(branch.Base); ok && base.UpstreamBranch != "" { + baseBranch = base.UpstreamBranch + } + changes = append(changes, forge.StackChange{ + Change: change, + BaseChange: changeByBranch[branch.Base], + BaseBranch: baseBranch, + }) + } + return changes +} diff --git a/internal/handler/submit/stacks_test.go b/internal/handler/submit/stacks_test.go new file mode 100644 index 000000000..56d2fd678 --- /dev/null +++ b/internal/handler/submit/stacks_test.go @@ -0,0 +1,222 @@ +package submit + +import ( + "bytes" + "context" + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.abhg.dev/gs/internal/forge" + "go.abhg.dev/gs/internal/forge/forgetest" + "go.abhg.dev/gs/internal/silog" + "go.abhg.dev/gs/internal/spice" + "go.abhg.dev/gs/internal/spice/spicetest" + "go.abhg.dev/gs/internal/spice/state" + "go.uber.org/mock/gomock" +) + +func TestNativeStackChanges(t *testing.T) { + graph := spicetest.NewBranchGraph(t, spicetest.BranchGraphConfig{ + Trunk: "main", + Branches: []spice.LoadBranchItem{ + {Name: "other", Base: "main", Change: submitFakeChange("pr-9")}, + {Name: "top", Base: "middle", Change: submitFakeChange("pr-3")}, + {Name: "bottom", Base: "main", Change: submitFakeChange("pr-1")}, + {Name: "divergent", Base: "middle", Change: submitFakeChange("pr-4")}, + {Name: "middle", Base: "bottom", Change: submitFakeChange("pr-2")}, + }, + }) + + assert.ElementsMatch(t, []forge.StackChange{ + {Change: submitFakeChangeID("pr-1"), BaseBranch: "main"}, + {Change: submitFakeChangeID("pr-2"), BaseChange: submitFakeChangeID("pr-1"), BaseBranch: "bottom"}, + {Change: submitFakeChangeID("pr-3"), BaseChange: submitFakeChangeID("pr-2"), BaseBranch: "middle"}, + {Change: submitFakeChangeID("pr-4"), BaseChange: submitFakeChangeID("pr-2"), BaseBranch: "middle"}, + }, nativeStackChanges(graph, "test", []string{"top"})) +} + +func TestHandler_updateStacks_unsupported(t *testing.T) { + ctrl := gomock.NewController(t) + remoteForge := forgetest.NewMockForge(ctrl) + remoteRepo := forgetest.NewMockRepository(ctrl) + + service := NewMockService(ctrl) + handler := new(Handler) + handler.Log = silog.Nop() + handler.Service = service + handler.FindRemote = func(context.Context) (state.Remote, error) { + return state.Remote{Upstream: "origin"}, nil + } + handler.ResolveRepository = func( + context.Context, + string, + ) (forge.Forge, forge.RepositoryID, error) { + return remoteForge, stubRepositoryID("acme/repo"), nil + } + handler.OpenRepository = func( + context.Context, + forge.Forge, + forge.RepositoryID, + ) (forge.Repository, error) { + return remoteRepo, nil + } + + require.NoError(t, handler.updateStacks( + t.Context(), + []string{"feature"}, + new(submitStackUpdates), + )) +} + +func TestHandler_updateStacks_errors(t *testing.T) { + tests := []struct { + name string + planErr error + runErr error + wantLog bool + }{ + {name: "Unsupported", planErr: forge.ErrUnsupported}, + {name: "Failure", runErr: errors.New("boom"), wantLog: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + remoteForge := forgetest.NewMockForge(ctrl) + remoteForge.EXPECT().ID().Return("test").AnyTimes() + + remoteRepo := forgetest.NewMockRepository(ctrl) + remoteRepo.EXPECT().Forge().Return(remoteForge) + stackRepo := &submitStackRepository{ + Repository: remoteRepo, + planErr: tt.planErr, + runErr: tt.runErr, + } + + graph := spicetest.NewBranchGraph(t, spicetest.BranchGraphConfig{ + Trunk: "main", + Branches: []spice.LoadBranchItem{ + {Name: "feature", Base: "main", Change: submitFakeChange("pr-1")}, + }, + }) + service := NewMockService(ctrl) + service.EXPECT().BranchGraph(gomock.Any(), nil).Return(graph, nil) + + var logs bytes.Buffer + handler := new(Handler) + handler.Log = silog.New(&logs, nil) + handler.Service = service + handler.FindRemote = func(context.Context) (state.Remote, error) { + return state.Remote{Upstream: "origin"}, nil + } + handler.ResolveRepository = func( + context.Context, + string, + ) (forge.Forge, forge.RepositoryID, error) { + return remoteForge, stubRepositoryID("acme/repo"), nil + } + handler.OpenRepository = func( + context.Context, + forge.Forge, + forge.RepositoryID, + ) (forge.Repository, error) { + return stackRepo, nil + } + + require.NoError(t, handler.updateStacks( + t.Context(), + []string{"feature"}, + new(submitStackUpdates), + )) + + if tt.wantLog { + assert.Contains(t, logs.String(), "Could not update stacks") + assert.Contains(t, logs.String(), "boom") + } else { + assert.Empty(t, logs.String()) + } + }) + } +} + +func TestHandler_updateStacks_fallsBackAfterUnsupportedPlan(t *testing.T) { + ctrl := gomock.NewController(t) + remoteForge := forgetest.NewMockForge(ctrl) + remoteForge.EXPECT().ID().Return("test").AnyTimes() + remoteRepo := forgetest.NewMockRepository(ctrl) + remoteRepo.EXPECT().Forge().Return(remoteForge) + change := submitFakeChangeID("pr-1") + remoteRepo.EXPECT().EditChange(gomock.Any(), change, forge.EditChangeOptions{ + Base: "main", + }).Return(nil) + stackRepo := &submitStackRepository{ + Repository: remoteRepo, + planErr: forge.ErrUnsupported, + } + + graph := spicetest.NewBranchGraph(t, spicetest.BranchGraphConfig{ + Trunk: "main", + Branches: []spice.LoadBranchItem{ + {Name: "feature", Base: "main", Change: submitFakeChange("pr-1")}, + }, + }) + service := NewMockService(ctrl) + service.EXPECT().BranchGraph(gomock.Any(), nil).Return(graph, nil) + handler := new(Handler) + handler.Log = silog.Nop() + handler.Service = service + handler.FindRemote = func(context.Context) (state.Remote, error) { + return state.Remote{Upstream: "origin"}, nil + } + handler.ResolveRepository = func( + context.Context, + string, + ) (forge.Forge, forge.RepositoryID, error) { + return remoteForge, stubRepositoryID("acme/repo"), nil + } + handler.OpenRepository = func( + context.Context, + forge.Forge, + forge.RepositoryID, + ) (forge.Repository, error) { + return stackRepo, nil + } + + require.NoError(t, handler.updateStacks( + t.Context(), + []string{"feature"}, + &submitStackUpdates{deferredBases: []deferredBaseUpdate{ + {repository: remoteRepo, change: change, base: "main"}, + }}, + )) +} + +type submitStackRepository struct { + forge.Repository + + planErr error + runErr error +} + +func (r *submitStackRepository) PlanStackUpdate( + _ context.Context, + _ []forge.StackChange, +) (forge.StackUpdatePlan, error) { + if r.planErr != nil { + return nil, r.planErr + } + return submitStackUpdatePlan{err: r.runErr}, nil +} + +type submitStackUpdatePlan struct{ err error } + +func (p submitStackUpdatePlan) Execute(context.Context) error { return p.err } + +func (*submitStackRepository) PlanMergeRanges( + context.Context, + []forge.StackChange, +) ([]forge.MergeRangePlan, error) { + return nil, forge.ErrUnsupported +} diff --git a/testdata/script/downstack_submit_native_stacks.txt b/testdata/script/downstack_submit_native_stacks.txt new file mode 100644 index 000000000..25d7cbbe3 --- /dev/null +++ b/testdata/script/downstack_submit_native_stacks.txt @@ -0,0 +1,74 @@ +# Native stacks are updated after submitting a stack (if supported). + +as 'Test ' +at '2026-08-05T12:00:00Z' + +cd repo +git init +git config spice.forge.shamhub.stacks on +git commit --allow-empty -m 'Initial commit' + +shamhub-setup +shamhub new origin alice/example.git +shamhub register alice +git push origin main +env SHAMHUB_USERNAME=alice +gs auth login + +git add bottom.txt +gs bc bottom -m 'Add bottom' +git add top.txt +gs bc top -m 'Add top' + +gs downstack submit --fill +stderr 'Created #1' +stderr 'Created #2' + +shamhub dump stacks alice/example +cmpenvJSON stdout $WORK/stacks.json + +# Insert a new branch between two already-stacked changes. Submit must defer +# retargeting #2 until ShamHub can replace the native stack atomically. +gs branch checkout bottom +git add middle.txt +gs branch create middle -m 'Add middle' +gs branch checkout top +gs branch onto --restack middle + +gs downstack submit --fill +stderr 'Created #3' +stderr 'Updated #2' + +shamhub dump stacks alice/example +cmpenvJSON stdout $WORK/reordered-stacks.json + +-- repo/bottom.txt -- +bottom +-- repo/top.txt -- +top +-- repo/middle.txt -- +middle +-- stacks.json -- +[ + { + "number": 1 + }, + { + "number": 2, + "base": 1 + } +] +-- reordered-stacks.json -- +[ + { + "number": 1 + }, + { + "number": 2, + "base": 3 + }, + { + "number": 3, + "base": 1 + } +]