From 7de1735202c6fc59cee0204aaca95e205beae60d Mon Sep 17 00:00:00 2001 From: Dominic Black Date: Fri, 9 Oct 2026 13:23:58 +0100 Subject: [PATCH] Report a signing failure from restack's rebase and get past it The conflict path of a restack is a real git rebase, and a rebase signs every commit it replays, so a key git can't use stops it on the first pick. We read any rebase still in progress after a non-zero exit as a conflict stop, so the user was told to resolve files from merge-tree that weren't conflicted at all, and never that the key needed unlocking. Worse, git won't continue over the failed pick: it leaves the changes staged and says to commit them yourself, so continue was stuck even once the key was loaded. The rebase outcome now spots git's signing marker and returns the same SigningError commit does. Restack and continue map it to signing_failed with the ssh-add line, and continue and abort as the next steps; the rebase is left where it is so abort still puts the moved branches back. Continue does what git's hint says: when rebase --continue refuses because the failed pick is still staged, it commits it (git keeps the pick's author and message in its own state and then drops the rescheduled pick as already applied) and continues again. Modify's amend already came through the commit path; it now has a test proving it. While testing this I found commit-tree doesn't honour commit.gpgsign at all, whatever its docs say, so the native replay never needs the key. Its comment claimed the opposite and now says what it actually does. --- docs/mcp.md | 8 +++-- pkg/app/mutate_test.go | 30 ++++++++++++++++ pkg/app/restack.go | 4 +-- pkg/app/restack_test.go | 49 ++++++++++++++++++++++++++ pkg/app/staging.go | 12 +++++++ pkg/git/commit.go | 5 ++- pkg/git/commit_test.go | 77 +++++++++++++++++++++++++++++++++++++++++ pkg/git/objects.go | 6 ++-- pkg/git/rebase.go | 30 ++++++++++++++++ pkg/mcp/instructions.go | 2 +- 10 files changed, 215 insertions(+), 8 deletions(-) diff --git a/docs/mcp.md b/docs/mcp.md index 62119fe..38da417 100644 --- a/docs/mcp.md +++ b/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: diff --git a/pkg/app/mutate_test.go b/pkg/app/mutate_test.go index 5bf135f..829c2d5 100644 --- a/pkg/app/mutate_test.go +++ b/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) + } +} diff --git a/pkg/app/restack.go b/pkg/app/restack.go index 36938d7..abfb592 100644 --- a/pkg/app/restack.go +++ b/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) diff --git a/pkg/app/restack_test.go b/pkg/app/restack_test.go index 60def91..9683d69 100644 --- a/pkg/app/restack_test.go +++ b/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) + } +} diff --git a/pkg/app/staging.go b/pkg/app/staging.go index 5b7bd3d..35555ac 100644 --- a/pkg/app/staging.go +++ b/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 diff --git a/pkg/git/commit.go b/pkg/git/commit.go index 8a72bca..ad1e62b 100644 --- a/pkg/git/commit.go +++ b/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 diff --git a/pkg/git/commit_test.go b/pkg/git/commit_test.go index 7b24f10..4dd84ee 100644 --- a/pkg/git/commit_test.go +++ b/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) + } +} diff --git a/pkg/git/objects.go b/pkg/git/objects.go index 18a42b2..b9e52bc 100644 --- a/pkg/git/objects.go +++ b/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", "-") diff --git a/pkg/git/rebase.go b/pkg/git/rebase.go index 1f81748..5bd4601 100644 --- a/pkg/git/rebase.go +++ b/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 diff --git a/pkg/mcp/instructions.go b/pkg/mcp/instructions.go index ecb66ae..045eba3 100644 --- a/pkg/mcp/instructions.go +++ b/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.`