Report a signing failure from restack's rebase and get past it

docs/mcp.md

@@ -59,8 +59,12 @@ Codes: `not_repo`, `not_in_stack`, `not_at_top`, `conflict`, `partial`, `rebase_
 `interaction_required`, `api_failure`, `disambiguate`, `modify_recovery`, `checks_failing`,
 `checks_pending`, `signing_failed`, `unknown`.
 `signing_failed` comes from `stack_create` and `stack_modify` when git could not sign the commit,
-most often an SSH signing key whose passphrase nobody can type in (the server has no terminal);
-nothing was committed and `next_steps` says which key to `ssh-add`, or how to turn signing off.
+and from `stack_restack`, `stack_modify` and `stack_continue` when the git rebase a conflict
+needs could not sign a replayed commit; most often an SSH signing key whose passphrase nobody
+can type in (the server has no terminal). For a commit nothing was committed; for a rebase it is
+left in progress with the commit rescheduled, so `stack_continue` carries on once signing works
+and `stack_abort` still puts everything back. `next_steps` says which key to `ssh-add`, or how to
+turn signing off.
 `checks_failing` and `checks_pending` come from `stack_merge` when a pull request it would land
 has a check that failed, or (with none failed) one still queued or running, on its head commit.
 Nothing is merged. The error's `checks` lists each held up pull request:

pkg/app/mutate_test.go

@@ -505,3 +505,33 @@ func TestCreateUndoesTheBranchWhenTheCommitIsCancelled(t *testing.T) {
 		t.Errorf("stack = %v, want feat/b forgotten", names)
 	}
 }
+
+func TestModifyExplainsSigningFailure(t *testing.T) {
+	a, _, repo, dir, _ := mutFixture(t)
+	ctx := context.Background()
+	gittest.Run(t, dir, "config", "gpg.format", "ssh")
+	gittest.Run(t, dir, "config", "gpg.ssh.program", "false")
+	gittest.Run(t, dir, "config", "user.signingkey", "/keys/id_ed25519.pub")
+	gittest.Run(t, dir, "config", "commit.gpgsign", "true")
+	before := gittest.Run(t, dir, "rev-parse", "HEAD")
+	gittest.WriteFile(t, dir, "a.txt", "a2")
+	gittest.Run(t, dir, "add", "a.txt")
+
+	for _, newCommit := range []bool{false, true} {
+		_, err := a.Modify(ctx, repo, app.ModifyOptions{NewCommit: newCommit, Message: []string{"amended"}})
+		se, ok := errors.AsType[*stack.Error](err)
+		if !ok || se.Kind != stack.KindSigningFailed {
+			t.Fatalf("new commit %v: err = %v, want signing_failed", newCommit, err)
+		}
+		steps := strings.Join(se.NextSteps, "\n")
+		if !strings.Contains(steps, "ssh-add /keys/id_ed25519") || !strings.Contains(steps, "git stack modify") {
+			t.Errorf("steps = %q", se.NextSteps)
+		}
+	}
+	if gittest.Run(t, dir, "rev-parse", "HEAD") != before {
+		t.Error("the branch must be untouched")
+	}
+	if out := gittest.Run(t, dir, "diff", "--cached", "--name-only"); out != "a.txt" {
+		t.Errorf("staged = %q, want a.txt still staged", out)
+	}
+}

pkg/app/restack.go

@@ -225,7 +225,7 @@ func (a *App) startConflictRebase(ctx context.Context, run *restackRun, p *resta
 	}
 	stopped, err := a.d.Git.RebaseOnto(ctx, run.repo, p.conflictNewBase, p.conflictFrom, c.Branch)
 	if err != nil {
-		return err // the state stays so --abort can still put the moved branches back
+		return rebaseError(err) // the state stays so --abort can still put the moved branches back
 	}
 	if stopped {
 		files, ferr := a.d.Git.ConflictedFiles(ctx, run.repo)
@@ -465,7 +465,7 @@ func (a *App) restackContinue(ctx context.Context, repo git.Repo, stageAll bool)
 		}
 		stopped, err := a.d.Git.RebaseContinue(ctx, repo)
 		if err != nil {
-			return res, err
+			return res, rebaseError(err)
 		}
 		if stopped {
 			files, _ := a.d.Git.ConflictedFiles(ctx, repo)

pkg/app/restack_test.go

@@ -963,3 +963,52 @@ func TestRestackAbortSkipsDirtyOtherWorktree(t *testing.T) {
 		t.Error("b stays where the restack put it and the state is cleared")
 	}
 }
+
+// signingOff turns commit signing off again once a test has shown the failure.
+func signingOff(t *testing.T, dir string) {
+	t.Helper()
+	gittest.Run(t, dir, "config", "commit.gpgsign", "false")
+}
+
+// failSigning makes every signature fail, as a locked SSH key does without a
+// terminal to ask for its passphrase.
+func failSigning(t *testing.T, dir string) {
+	t.Helper()
+	gittest.Run(t, dir, "config", "gpg.format", "ssh")
+	gittest.Run(t, dir, "config", "gpg.ssh.program", "false")
+	gittest.Run(t, dir, "config", "user.signingkey", "/keys/id_ed25519.pub")
+	gittest.Run(t, dir, "config", "commit.gpgsign", "true")
+}
+
+func TestRestackConflictRebaseSigningFailureSaysContinue(t *testing.T) {
+	f := conflictFixture(t)
+	ctx := context.Background()
+	failSigning(t, f.dir)
+
+	_, err := f.app.Restack(ctx, f.repo, app.RestackOptions{})
+	var se *stack.Error
+	if !errors.As(err, &se) || se.Kind != stack.KindSigningFailed {
+		t.Fatalf("err = %+v, want signing_failed", err)
+	}
+	steps := strings.Join(se.NextSteps, "\n")
+	for _, want := range []string{"ssh-add /keys/id_ed25519", "git stack continue", "git stack abort"} {
+		if !strings.Contains(steps, want) {
+			t.Errorf("steps %q should mention %q", se.NextSteps, want)
+		}
+	}
+	if !f.rebaseActive(t) || !f.stateExists() {
+		t.Fatal("the rebase of b should be in progress with our state saved")
+	}
+	// Still failing: continue explains it again rather than claiming a conflict.
+	if _, err = f.app.Restack(ctx, f.repo, app.RestackOptions{Continue: true}); !errors.As(err, &se) || se.Kind != stack.KindSigningFailed {
+		t.Errorf("continue while still failing = %v, want signing_failed", err)
+	}
+	// Signing sorted: continue reaches the real conflict.
+	signingOff(t, f.dir)
+	if _, err = f.app.Restack(ctx, f.repo, app.RestackOptions{Continue: true}); !errors.As(err, &se) || se.Kind != stack.KindConflict || !slices.Equal(se.Files, []string{"shared.txt"}) {
+		t.Errorf("continue after fixing signing = %+v, want the shared.txt conflict", err)
+	}
+	if res, err := f.app.Restack(ctx, f.repo, app.RestackOptions{Abort: true}); err != nil || f.rebaseActive(t) || f.stateExists() {
+		t.Errorf("abort = %+v %v", res, err)
+	}
+}

pkg/app/staging.go

@@ -103,6 +103,18 @@ func commitError(err error, editorHint string) error {
 	return err
 }
 
+// rebaseError maps a failed git rebase from a restack. A signing failure
+// leaves the rebase in progress with the commit rescheduled, so the next
+// steps are the usual continue and abort once signing works.
+func rebaseError(err error) error {
+	if se, ok := errors.AsType[*git.SigningError](err); ok {
+		return asStackError(signingError(se)).WithSteps(
+			"then `git stack continue` to carry on the restack",
+			"or `git stack abort` to put the moved branches back")
+	}
+	return err
+}
+
 // signingError explains a signing failure. The common cause with an SSH key
 // is a passphrase protected key that isn't in ssh-agent: without a terminal
 // (the MCP server, a script) nothing can answer the passphrase prompt, so

pkg/git/commit.go

@@ -148,7 +148,10 @@ func (c *Client) Commit(ctx context.Context, repo Repo, o CommitOptions) (string
 const signingFailedMarker = "failed to write commit object"
 
 // SigningError reports that git could not sign a commit. Nothing was
-// committed and the index is untouched.
+// committed. From Commit the index is untouched; from a rebase (RebaseOnto,
+// RebaseContinue) the rebase is left in progress with the failed pick's
+// changes staged, for RebaseContinue to commit once signing works or
+// RebaseAbort to drop.
 type SigningError struct {
 	// Format is gpg.format: ssh, openpgp (the default) or x509.
 	Format string

pkg/git/commit_test.go

@@ -130,3 +130,80 @@ func TestCommitBareGpgsignKeyIsSigning(t *testing.T) {
 		t.Errorf("Key = %q, want %q", se.Key, want)
 	}
 }
+
+// TestRebaseWithFailingSignerIsASigningError covers the conflict path of a
+// restack: git rebase signs every commit it replays, so a key it can't use
+// stops the rebase on the first pick (rescheduled, no conflicts). That
+// must come back as a SigningError rather than a conflict stop, with the
+// rebase left in progress so continue and abort still work.
+func TestRebaseWithFailingSignerIsASigningError(t *testing.T) {
+	gittest.Isolate(t)
+	dir := gittest.InitRepo(t)
+	gittest.Run(t, dir, "switch", "-q", "-c", "feat")
+	gittest.Commit(t, dir, "f1", "1", "f1")
+	gittest.Run(t, dir, "switch", "-q", "main")
+	gittest.Commit(t, dir, "m1", "1", "m1")
+	gittest.Run(t, dir, "config", "gpg.format", "ssh")
+	gittest.Run(t, dir, "config", "gpg.ssh.program", "false")
+	gittest.Run(t, dir, "config", "user.signingkey", "/nowhere/id_ed25519.pub")
+	gittest.Run(t, dir, "config", "commit.gpgsign", "true")
+
+	c := newClient()
+	ctx := context.Background()
+	repo, _ := c.Discover(ctx, dir)
+	stopped, err := c.RebaseOnto(ctx, repo, "main", "main", "feat")
+	se, ok := errors.AsType[*git.SigningError](err)
+	if !ok || stopped {
+		t.Fatalf("RebaseOnto = %v, %v; want a *git.SigningError", stopped, err)
+	}
+	if se.Format != "ssh" || se.Key != "/nowhere/id_ed25519.pub" {
+		t.Errorf("SigningError = %+v", se)
+	}
+	if active, _ := c.RebaseInProgress(ctx, repo); !active {
+		t.Fatal("the rebase should be left in progress for continue or abort")
+	}
+	// Still locked: continue fails the same way.
+	if stopped, err = c.RebaseContinue(ctx, repo); !errors.As(err, &se) || stopped {
+		t.Errorf("RebaseContinue = %v, %v; want a *git.SigningError", stopped, err)
+	}
+	// Signing sorted: continue finishes the rebase.
+	gittest.Run(t, dir, "config", "commit.gpgsign", "false")
+	if stopped, err = c.RebaseContinue(ctx, repo); err != nil || stopped {
+		t.Fatalf("RebaseContinue after fixing signing = %v, %v", stopped, err)
+	}
+	if gittest.Run(t, dir, "rev-parse", "feat~1") != gittest.Run(t, dir, "rev-parse", "main") {
+		t.Error("feat should now sit on main")
+	}
+}
+
+// TestRebaseObjectWriteFailureIsNotSigning: git's sequencer prints the same
+// "failed to write commit object" line when it can't write the object at
+// all. With signing off that must not be mistaken for a signing failure,
+// and above all must not look like a rebase that finished.
+func TestRebaseObjectWriteFailureIsNotSigning(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("root can write to a read only directory")
+	}
+	gittest.Isolate(t)
+	dir := gittest.InitRepo(t)
+	gittest.Run(t, dir, "switch", "-q", "-c", "feat")
+	gittest.Commit(t, dir, "f1", "1", "f1")
+	gittest.Run(t, dir, "switch", "-q", "main")
+	gittest.Commit(t, dir, "m1", "1", "m1")
+	objects := filepath.Join(dir, ".git", "objects")
+	if err := os.Chmod(objects, 0o555); err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() { _ = os.Chmod(objects, 0o755) })
+
+	c := newClient()
+	ctx := context.Background()
+	repo, _ := c.Discover(ctx, dir)
+	stopped, err := c.RebaseOnto(ctx, repo, "main", "main", "feat")
+	if !stopped && err == nil {
+		t.Fatal("a rebase that could not write its commit was reported as finished")
+	}
+	if _, ok := errors.AsType[*git.SigningError](err); ok {
+		t.Errorf("err = %v, classified as a signing failure", err)
+	}
+}

pkg/git/objects.go

@@ -83,8 +83,10 @@ func (c *Client) MergeTree(ctx context.Context, repo Repo, base, ours, theirs st
 }
 
 // CommitTree creates a commit of tree on parent with info's author, author
-// date and message. The committer is the current user and signing follows
-// the user's commit.gpgsign, as commit-tree honours both.
+// date and message. The committer is the current user. The commit is not
+// signed: unlike commit and rebase, commit-tree ignores commit.gpgsign and
+// only signs when asked with -S, which we don't pass, so a replay never
+// needs the signing key.
 func (c *Client) CommitTree(ctx context.Context, repo Repo, tree, parent string, info CommitInfo) (string, error) {
 	env := []string{"GIT_AUTHOR_NAME=" + info.Author, "GIT_AUTHOR_EMAIL=" + info.Email, "GIT_AUTHOR_DATE=" + info.Date}
 	res, err := c.gitInput(ctx, repo, strings.NewReader(info.Message+"\n"), env, "commit-tree", tree, "-p", parent, "-F", "-")

pkg/git/rebase.go

@@ -129,9 +129,27 @@ func (c *Client) RebaseOnto(ctx context.Context, repo Repo, onto, upstream, bran
 // failure is an error.
 func (c *Client) RebaseContinue(ctx context.Context, repo Repo) (stopped bool, err error) {
 	_, err = c.gitInput(ctx, repo, nil, rebaseEnv, "rebase", "--continue")
+	if ee, ok := errors.AsType[*exec.ExitError](err); ok && strings.Contains(ee.Result.Err(), stagedChangesMarker) {
+		// A pick whose commit failed (signing, say) is left staged and git
+		// refuses to continue over it, telling you to commit it yourself.
+		// Do that: git takes the author and message from its own rebase
+		// state, and the rebase then drops the rescheduled pick as already
+		// applied. --no-verify skips pre-commit and commit-msg, the two
+		// hooks a pick skips too (both run prepare-commit-msg and
+		// post-commit), so the commit sees the same hooks the pick would
+		// have. Signing failing again comes back as a SigningError.
+		if _, cerr := c.Commit(ctx, repo, CommitOptions{NoEdit: true, NoVerify: true}); cerr != nil {
+			return false, cerr
+		}
+		_, err = c.gitInput(ctx, repo, nil, rebaseEnv, "rebase", "--continue")
+	}
 	return c.rebaseOutcome(ctx, repo, err)
 }
 
+// stagedChangesMarker is git rebase --continue's refusal when the previous
+// pick was applied but never committed.
+const stagedChangesMarker = "you have staged changes in your working tree"
+
 // RebaseAbort abandons a rebase in progress; git puts the branch it was
 // rebasing back and checks it out.
 func (c *Client) RebaseAbort(ctx context.Context, repo Repo) error {
@@ -152,6 +170,18 @@ func (c *Client) rebaseOutcome(ctx context.Context, repo Repo, err error) (bool,
 	if !ok {
 		return false, err
 	}
+	// A commit git could not sign stops the rebase too, with the pick
+	// rescheduled and nothing unmerged, so it is a rebase in progress that
+	// no amount of resolving would move on. Report the real cause; continue
+	// picks it up again once signing works, and abort still puts it back.
+	if stderr := strings.TrimSpace(ee.Result.Err()); strings.Contains(stderr, signingFailedMarker) {
+		// Only a signing failure when signing is on; git prints the same
+		// line when it can't write the object at all, which is a stop or
+		// an error like any other below.
+		if se := c.signingError(ctx, repo, stderr); se != nil {
+			return false, se
+		}
+	}
 	active, aerr := c.RebaseInProgress(ctx, repo)
 	if aerr != nil {
 		return false, aerr

pkg/mcp/instructions.go

@@ -6,6 +6,6 @@ const Instructions = `git-stack manages stacked branches and stacked pull reques
 
 Never run git rebase or git commit --amend by hand inside a stack: descendants would not be restacked. Use stack_modify / stack_restack instead.
 
-Errors are JSON objects {code, message, next_steps, files?, branch?, checks?}. On code "conflict": resolve the listed files, run git add on them, then call stack_continue (or stack_abort to give up). Code "not_at_top": gh stack can only add branches at the top of a stack; call stack_navigate {direction: "top"} first. Code "interaction_required": the operation would need a terminal; pass the required text (message, pull_requests) instead. Code "signing_failed": git could not sign the commit (usually an SSH signing key not loaded into ssh-agent; the server cannot answer a passphrase prompt); nothing was committed, so tell the user to run the ssh-add in next_steps (or turn signing off for the repository) and call the tool again. Code "partial" from stack_sync: everything else was done, but the branches in the result's notUpdated could not be updated (reason locked: another git process held the lock file in the entry's lock field, of kind lockKind: "index" for that checkout's index or "ref" for the branch's own ref, which can apply to a branch with no checkout; use those fields rather than assuming an index lock; refused: git would not move the checkout for another reason, such as a merge in progress there; entries with reason dirty or untracked list the files in the way and don't fail the sync); follow next_steps and call stack_sync again.
+Errors are JSON objects {code, message, next_steps, files?, branch?, checks?}. On code "conflict": resolve the listed files, run git add on them, then call stack_continue (or stack_abort to give up). Code "not_at_top": gh stack can only add branches at the top of a stack; call stack_navigate {direction: "top"} first. Code "interaction_required": the operation would need a terminal; pass the required text (message, pull_requests) instead. Code "signing_failed": git could not sign the commit (usually an SSH signing key not loaded into ssh-agent; the server cannot answer a passphrase prompt); nothing was committed, so tell the user to run the ssh-add in next_steps (or turn signing off for the repository) and call the tool again; from a restack the rebase is left in progress, so call stack_continue once signing works, or stack_abort to put the branches back. Code "partial" from stack_sync: everything else was done, but the branches in the result's notUpdated could not be updated (reason locked: another git process held the lock file in the entry's lock field, of kind lockKind: "index" for that checkout's index or "ref" for the branch's own ref, which can apply to a branch with no checkout; use those fields rather than assuming an index lock; refused: git would not move the checkout for another reason, such as a merge in progress there; entries with reason dirty or untracked list the files in the way and don't fail the sync); follow next_steps and call stack_sync again.
 
 Every tool accepts repo_path (an absolute path inside the repository); otherwise the client's first root, then the server's working directory, is used. stack_view is read-only; stack_submit pushes to the remote, stack_merge merges pull requests on the forge, and stack_sync deletes local branches.`