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.`