From e3f17b92201dbefface9ecdd23ccf34d3a493618 Mon Sep 17 00:00:00 2001 From: Dominic Black Date: Fri, 9 Oct 2026 13:13:54 +0100 Subject: [PATCH] Undo the branch when create's commit fails Create makes the branch, checks it out and registers it with gh stack before it commits, so a failed commit (a signing key nobody could unlock, a pre-commit hook saying no) left a tracked branch sat at the parent's commit with nothing of its own and the changes still staged. The error didn't say so either; from the MCP side it looked like nothing had happened, and calling stack_create again fails because the branch already exists. The only way out was to notice it in stack_view and reach for stack_modify instead. Create now undoes itself when the commit fails: the branch is forgotten through Update (an empty stack goes with it), we switch back to the parent and delete the branch. Both point at the same commit so the staged changes are untouched, and the error says nothing was created and to run create again once the cause is fixed. If the undo itself fails the error says what was left behind and that modify -c commits on it. The tool description and architecture notes say the same. --- docs/architecture.md | 6 +- pkg/app/create.go | 60 +++++++++++++++++++- pkg/app/modify.go | 6 +- pkg/app/mutate_test.go | 122 +++++++++++++++++++++++++++++++++++++++++ pkg/app/staging.go | 2 +- pkg/mcp/tools.go | 2 +- 6 files changed, 193 insertions(+), 5 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 069abc0..3dd3c84 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -188,7 +188,11 @@ rather than a confusing git error. **`create` from trunk runs `gh stack init`, from the top runs `gh stack add`, and we always commit natively.** This sidesteps both `add` quirks above. From the middle of a stack you get a `not_at_top` error with next steps; from an untracked branch you get told -the branch isn't in a stack. +the branch isn't in a stack. The branch is created, checked out and registered before the +commit, so when the commit fails (a signing key that isn't unlocked, a hook saying no) create +undoes all of that, forgetting the branch through `Update`, switching back and deleting it, +and the staged changes are still staged; the error says to run it again. If the undo itself +fails the error says what was left behind and that `modify -c` commits on it. **`modify` amends natively, then runs the native restack upstack.** Conflicts come back as a `conflict` error listing the files and the `git stack continue` / `git stack abort` diff --git a/pkg/app/create.go b/pkg/app/create.go index 887e0ff..5bf5ca0 100644 --- a/pkg/app/create.go +++ b/pkg/app/create.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "slices" "strings" + "time" "github.com/DomBlack/git-stack/pkg/ai" "github.com/DomBlack/git-stack/pkg/git" @@ -36,6 +38,10 @@ type CreateResult struct { // recentSubjectsCount is how many trunk subjects feed the AI prompt. const recentSubjectsCount = 10 +// undoTimeout bounds the undo of a failed create, which runs even when the +// command's own context was cancelled. +const undoTimeout = 30 * time.Second + // Create stages, creates a branch on top of the current one, commits and // registers the branch with the backend. func (a *App) Create(ctx context.Context, repo git.Repo, o CreateOptions) (CreateResult, error) { @@ -96,13 +102,65 @@ func (a *App) Create(ctx context.Context, repo git.Repo, o CreateOptions) (Creat } sha, err := a.d.Git.Commit(ctx, repo, git.CommitOptions{Message: message, NoVerify: o.NoVerify}) if err != nil { - return res, commitError(err, "pass -m (the branch was created; commit with git commit)") + return CreateResult{}, a.undoCreate(ctx, repo, name, current, commitError(err, "pass -m ")) } info := a.commitInfo(ctx, repo, sha) res.Commit = &info return res, nil } +// undoCreate makes a failed create all or nothing: the branch was already +// created, checked out and registered before the commit failed (a locked +// signing key, a hook saying no), so forget it, go back to the parent and +// delete it. The staged changes survive, both branches point at the same +// commit. The error says to run create again, or, when undoing failed too, +// what was left behind and how to commit on it instead. +func (a *App) undoCreate(ctx context.Context, repo git.Repo, name, parent string, cause error) error { + err := asStackError(cause) + // The commit may have failed because ctx was cancelled; the undo still + // has to run, so give it its own bounded context. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), undoTimeout) + defer cancel() + // Commit can fail after git has actually committed (reading HEAD back + // under a cancelled context, say). Then the branch holds the commit + // and must stay: deleting it would orphan the work. + if tip, ok, terr := a.d.Git.Tip(ctx, repo, name); terr == nil && ok { + if parentTip, _, perr := a.d.Git.Tip(ctx, repo, parent); perr == nil && tip != parentTip { + return err.WithSteps(fmt.Sprintf("%s has a commit after all, so it was kept and is tracked; check `git stack log`", name)) + } + } + undo := func() error { + if err := a.d.Meta.Update(ctx, repo, func(g *stack.Graph) error { + for i := range g.Stacks { + g.Stacks[i].Branches = slices.DeleteFunc(g.Stacks[i].Branches, func(b stack.Branch) bool { return b.Name == name }) + } + return nil + }); err != nil { + return fmt.Errorf("forget %s: %w", name, err) + } + if err := a.d.Git.Switch(ctx, repo, parent); err != nil { + return err + } + return a.d.Git.DeleteBranch(ctx, repo, name) + } + if undoErr := undo(); undoErr != nil { + a.d.Log.Debug("undo create", "branch", name, "err", undoErr) + return err.WithSteps( + fmt.Sprintf("%s was left behind with no commit of its own (undoing it failed: %v)", name, undoErr), + "check `git stack log`; if it is still tracked, commit on it with `git stack modify -c -m `, otherwise delete it") + } + return err.WithSteps("nothing was created; run `git stack create` again once that is sorted") +} + +// asStackError returns err as a *stack.Error, wrapping anything else as +// KindUnknown so next steps can be attached. +func asStackError(err error) *stack.Error { + if se, ok := errors.AsType[*stack.Error](err); ok { + return se + } + return stack.New(stack.KindUnknown, err.Error()).WithCause(err) +} + // isDefaultBranch reports whether name is the repository's default branch // while no stack has declared a trunk yet (the first stack starts here). func (a *App) isDefaultBranch(ctx context.Context, repo git.Repo, graph *stack.Graph, name string) bool { diff --git a/pkg/app/modify.go b/pkg/app/modify.go index 68e825b..5c6b942 100644 --- a/pkg/app/modify.go +++ b/pkg/app/modify.go @@ -104,7 +104,11 @@ func (a *App) Modify(ctx context.Context, repo git.Repo, o ModifyOptions) (Modif } sha, err := a.d.Git.Commit(ctx, repo, co) if err != nil { - return ModifyResult{}, commitError(err, "pass -m or --no-edit") + err = commitError(err, "pass -m or --no-edit") + if errors.Is(err, &stack.Error{Kind: stack.KindSigningFailed}) { + err = asStackError(err).WithSteps("then run `git stack modify` again; the branch is untouched") + } + return ModifyResult{}, err } res.Commit = a.commitInfo(ctx, repo, sha) diff --git a/pkg/app/mutate_test.go b/pkg/app/mutate_test.go index 1be866f..5bf135f 100644 --- a/pkg/app/mutate_test.go +++ b/pkg/app/mutate_test.go @@ -3,9 +3,12 @@ package app_test import ( "context" "errors" + "os" + "path/filepath" "slices" "strings" "testing" + "time" "github.com/DomBlack/git-stack/pkg/ai" "github.com/DomBlack/git-stack/pkg/app" @@ -21,11 +24,16 @@ import ( type fakeBackend struct { git *git.Client graph *stack.Graph + // updateErr makes Update fail, for the paths that undo a change. + updateErr error } func (f *fakeBackend) Load(context.Context, git.Repo) (*stack.Graph, error) { return f.graph, nil } func (f *fakeBackend) Update(_ context.Context, _ git.Repo, fn func(*stack.Graph) error) error { + if f.updateErr != nil { + return f.updateErr + } if err := fn(f.graph); err != nil { return err } @@ -383,3 +391,117 @@ func TestCreateExplainsSigningFailure(t *testing.T) { }) } } + +// rejectCommits installs a pre-commit hook that fails every commit. +func rejectCommits(t *testing.T, dir string) { + t.Helper() + hooks := filepath.Join(dir, ".git", "hooks") + if err := os.MkdirAll(hooks, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(hooks, "pre-commit"), []byte("#!/bin/sh\necho nope >&2\nexit 1\n"), 0o755); err != nil { + t.Fatal(err) + } +} + +func TestCreateUndoesTheBranchWhenTheCommitFails(t *testing.T) { + ctx := context.Background() + + t.Run("on top of a stack", func(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + rejectCommits(t, dir) + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + se, ok := errors.AsType[*stack.Error](err) + if !ok || !strings.Contains(se.Detail+se.Msg, "nope") { + t.Fatalf("err = %v, want the hook's output", err) + } + if !slices.ContainsFunc(se.NextSteps, func(s string) bool { return strings.Contains(s, "git stack create") }) { + t.Errorf("next steps %q should say to run create again", se.NextSteps) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "a" { + t.Errorf("on %s, want back on a", cur) + } + if exists, _ := fb.git.BranchExists(ctx, repo, "feat/b"); exists { + t.Error("feat/b should have been deleted") + } + if names := fb.graph.Stacks[0].Names(); !slices.Equal(names, []string{"a"}) { + t.Errorf("stack = %v, want feat/b forgotten", names) + } + if out := gittest.Run(t, dir, "diff", "--cached", "--name-only"); out != "b.txt" { + t.Errorf("staged = %q, want b.txt still staged", out) + } + }) + + t.Run("new stack from trunk", func(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + gittest.Run(t, dir, "switch", "-q", "main") + fb.graph = stack.NewGraph(nil) + rejectCommits(t, dir) + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + if _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}); err == nil { + t.Fatal("expected the commit to fail") + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "main" { + t.Errorf("on %s, want back on main", cur) + } + if len(fb.graph.Stacks) != 0 { + t.Errorf("stacks = %+v, want the empty stack gone", fb.graph.Stacks) + } + }) + + t.Run("undo fails", func(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + rejectCommits(t, dir) + fb.updateErr = errors.New("metadata locked") + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + se, ok := errors.AsType[*stack.Error](err) + if !ok { + t.Fatalf("err = %v, want *stack.Error", err) + } + if steps := strings.Join(se.NextSteps, "\n"); !strings.Contains(steps, "feat/b was left behind") || !strings.Contains(steps, "git stack modify") { + t.Errorf("next steps %q should say feat/b is left behind and how to commit on it", se.NextSteps) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "feat/b" { + t.Errorf("on %s, want still on feat/b", cur) + } + }) +} + +func TestCreateUndoesTheBranchWhenTheCommitIsCancelled(t *testing.T) { + a, fb, repo, dir, _ := mutFixture(t) + // A hook that outlives the deadline: the commit fails with the + // context's error, and the undo must still run on a context of its own. + hooks := filepath.Join(dir, ".git", "hooks") + if err := os.MkdirAll(hooks, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(hooks, "pre-commit"), []byte("#!/bin/sh\nsleep 5\n"), 0o755); err != nil { + t.Fatal(err) + } + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond) + defer cancel() + _, err := a.Create(ctx, repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("err = %v, want the deadline", err) + } + if cur := gittest.Run(t, dir, "branch", "--show-current"); cur != "a" { + t.Errorf("on %s, want back on a", cur) + } + if exists, _ := fb.git.BranchExists(context.Background(), repo, "feat/b"); exists { + t.Error("feat/b should have been deleted") + } + if names := fb.graph.Stacks[0].Names(); !slices.Equal(names, []string{"a"}) { + t.Errorf("stack = %v, want feat/b forgotten", names) + } +} diff --git a/pkg/app/staging.go b/pkg/app/staging.go index 33f37ea..5b7bd3d 100644 --- a/pkg/app/staging.go +++ b/pkg/app/staging.go @@ -139,6 +139,6 @@ func signingError(se *git.SigningError) error { e = stack.New(stack.KindSigningFailed, "commit signing failed: gpg could not sign the commit (its agent may need a terminal to ask for the passphrase)"). WithSteps("check that gpg can sign without a prompt: echo test | gpg --batch --clearsign") } - return e.WithSteps("or turn signing off for this repository: git config commit.gpgsign false", "then run the command again"). + return e.WithSteps("or turn signing off for this repository: git config commit.gpgsign false"). WithDetail(se.Detail).WithCause(se) } diff --git a/pkg/mcp/tools.go b/pkg/mcp/tools.go index 3748b06..10f2088 100644 --- a/pkg/mcp/tools.go +++ b/pkg/mcp/tools.go @@ -435,7 +435,7 @@ func (s *Server) registerTools() { mcp.AddTool(s.mcp, &mcp.Tool{ Name: "stack_create", Title: "Create a stacked branch", - Description: "Create a new branch on top of the current branch, commit staged changes with the given message, and register it in the stack. From the trunk this starts a new stack. Fails with not_at_top when the current branch is not the top of its stack.", + Description: "Create a new branch on top of the current branch, commit staged changes with the given message, and register it in the stack. From the trunk this starts a new stack. Fails with not_at_top when the current branch is not the top of its stack. If the commit fails (signing_failed, a hook) the branch is removed again and the changes stay staged, so fix the cause and call it again; should removing it fail too, next_steps says what was left behind and how to commit on it.", Annotations: &mcp.ToolAnnotations{DestructiveHint: boolp(false), OpenWorldHint: boolp(false)}, }, s.create) mcp.AddTool(s.mcp, &mcp.Tool{