From 3acc7a460017583006f0594788df3a4582e5ab9d Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sat, 15 Aug 2026 06:51:12 -0700 Subject: [PATCH] submit: Reconcile native stacks after publishing Submission can publish change requests without refreshing provider-native stack metadata. For a stack-capable repository, changing an existing pull request base first can also prevent the provider from safely restructuring its current stack. Defer provider-facing base edits during submission. After every requested branch publishes successfully, load one branch graph, project each affected complete tree, and plan the desired native stacks in one batch. Execution then owns any required unstack, retarget, and recreate sequence. When planning reports `ErrUnsupported`, apply the deferred ordinary base edits instead. Warn for genuine native-stack planning or execution failures without changing the successful submission result. ShamHub script coverage inserts a branch between two already stacked changes to exercise the ordering boundary. --- .../unreleased/Added-20260805-045615.yaml | 3 + internal/handler/submit/handler.go | 47 +++- internal/handler/submit/stacks.go | 126 ++++++++++ internal/handler/submit/stacks_test.go | 222 ++++++++++++++++++ .../script/downstack_submit_native_stacks.txt | 74 ++++++ 5 files changed, 459 insertions(+), 13 deletions(-) create mode 100644 .changes/unreleased/Added-20260805-045615.yaml create mode 100644 internal/handler/submit/stacks.go create mode 100644 internal/handler/submit/stacks_test.go create mode 100644 testdata/script/downstack_submit_native_stacks.txt 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 + } +]