From 1036d8709cb4f0900a3a2da9cdd2f6d56fa40240 Mon Sep 17 00:00:00 2001 From: Dominic Black Date: Fri, 9 Oct 2026 13:08:30 +0100 Subject: [PATCH] Say which key to unlock when commit signing fails Once the runner stopped waiting on the passphrase prompt, stack_create came back with code "unknown" and git's stderr, with "incorrect passphrase supplied to decrypt private key" buried after "failed to write commit object". Nothing said the key just needed loading into ssh-agent, which is the whole fix for the common case (commit.gpgsign with an SSH key and an agent that hasn't got it, run from somewhere with no terminal). A git commit that fails on git's signing marker now comes back as a typed SigningError carrying gpg.format and user.signingkey, and create and modify turn that into a signing_failed error: the ssh-add line for that key (or the gpg check when the format is openpgp), the option of turning signing off for the repository, and git's output as the detail. Nothing is committed and the staged changes stay staged. The MCP instructions and docs list the new code. --- docs/mcp.md | 5 +- pkg/app/create.go | 2 +- pkg/app/modify.go | 2 +- pkg/app/mutate_test.go | 47 ++++++++++++++ pkg/app/staging.go | 54 +++++++++++++++- pkg/git/commit.go | 73 +++++++++++++++++++++- pkg/git/commit_test.go | 132 ++++++++++++++++++++++++++++++++++++++++ pkg/mcp/instructions.go | 2 +- pkg/stack/errors.go | 4 ++ 9 files changed, 313 insertions(+), 8 deletions(-) create mode 100644 pkg/git/commit_test.go diff --git a/docs/mcp.md b/docs/mcp.md index d6d9384..62119fe 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -57,7 +57,10 @@ Tool errors (`isError: true`) carry a JSON object in the text content: Codes: `not_repo`, `not_in_stack`, `not_at_top`, `conflict`, `partial`, `rebase_active`, `locked`, `stacks_unavailable`, `auth_required`, `not_installed`, `unsupported`, `invalid_args`, `interaction_required`, `api_failure`, `disambiguate`, `modify_recovery`, `checks_failing`, -`checks_pending`, `unknown`. +`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. `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/create.go b/pkg/app/create.go index 856c698..887e0ff 100644 --- a/pkg/app/create.go +++ b/pkg/app/create.go @@ -96,7 +96,7 @@ 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, editorError(err, "pass -m (the branch was created; commit with git commit)") + return res, commitError(err, "pass -m (the branch was created; commit with git commit)") } info := a.commitInfo(ctx, repo, sha) res.Commit = &info diff --git a/pkg/app/modify.go b/pkg/app/modify.go index 77f64e8..68e825b 100644 --- a/pkg/app/modify.go +++ b/pkg/app/modify.go @@ -104,7 +104,7 @@ 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{}, editorError(err, "pass -m or --no-edit") + return ModifyResult{}, commitError(err, "pass -m or --no-edit") } res.Commit = a.commitInfo(ctx, repo, sha) diff --git a/pkg/app/mutate_test.go b/pkg/app/mutate_test.go index d129ac1..1be866f 100644 --- a/pkg/app/mutate_test.go +++ b/pkg/app/mutate_test.go @@ -336,3 +336,50 @@ func TestRestack(t *testing.T) { t.Errorf("restack: %+v %v", res, err) } } + +func TestCreateExplainsSigningFailure(t *testing.T) { + cases := []struct { + name string + config [][]string + want string + }{ + {"ssh key file", [][]string{{"gpg.format", "ssh"}, {"gpg.ssh.program", "false"}, {"user.signingkey", "KEYDIR/id_ed25519.pub"}}, "ssh-add KEYDIR/id_ed25519"}, + {"ssh key file with a space", [][]string{{"gpg.format", "ssh"}, {"gpg.ssh.program", "false"}, {"user.signingkey", "KEYDIR/my keys/id_ed25519.pub"}}, "ssh-add 'KEYDIR/my keys/id_ed25519'"}, + {"ssh literal key", [][]string{{"gpg.format", "ssh"}, {"gpg.ssh.program", "false"}, {"user.signingkey", "key::ssh-ed25519 AAAAC3Nza"}}, "ssh-add"}, + {"ssh literal key without key::", [][]string{{"gpg.format", "ssh"}, {"gpg.ssh.program", "false"}, {"user.signingkey", "ssh-ed25519 AAAAC3Nza"}}, "with ssh-add"}, + {"ssh relative key file", [][]string{{"gpg.format", "ssh"}, {"gpg.ssh.program", "false"}, {"user.signingkey", "keys/id_ed25519.pub"}}, "ssh-add REPO/keys/id_ed25519"}, + {"gpg", [][]string{{"gpg.program", "false"}}, "gpg"}, + {"x509", [][]string{{"gpg.format", "x509"}, {"gpg.x509.program", "false"}}, "gpgsm"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + a, _, repo, dir, _ := mutFixture(t) + keyDir := t.TempDir() + gittest.WriteFile(t, keyDir, "id_ed25519.pub", "ssh-ed25519 AAAAC3Nza test\n") + gittest.WriteFile(t, keyDir, "my keys/id_ed25519.pub", "ssh-ed25519 AAAAC3Nza test\n") + gittest.WriteFile(t, dir, "keys/id_ed25519.pub", "ssh-ed25519 AAAAC3Nza test\n") + gittest.Run(t, dir, "config", "commit.gpgsign", "true") + for _, kv := range tc.config { + gittest.Run(t, dir, "config", kv[0], strings.ReplaceAll(kv[1], "KEYDIR", keyDir)) + } + gittest.WriteFile(t, dir, "b.txt", "b") + gittest.Run(t, dir, "add", "b.txt") + + _, err := a.Create(context.Background(), repo, app.CreateOptions{Name: "feat/b", Message: []string{"Add b"}}) + se, ok := errors.AsType[*stack.Error](err) + if !ok || se.Kind != stack.KindSigningFailed { + t.Fatalf("err = %v, want signing_failed", err) + } + want := strings.NewReplacer("KEYDIR", keyDir, "REPO", repo.TopLevel).Replace(tc.want) + if !slices.ContainsFunc(se.NextSteps, func(s string) bool { return strings.Contains(s, want) }) { + t.Errorf("next steps %q do not mention %q", se.NextSteps, want) + } + if !slices.ContainsFunc(se.NextSteps, func(s string) bool { return strings.Contains(s, "commit.gpgsign false") }) { + t.Errorf("next steps %q do not offer turning signing off", se.NextSteps) + } + if se.Detail == "" { + t.Error("Detail should carry git's stderr") + } + }) + } +} diff --git a/pkg/app/staging.go b/pkg/app/staging.go index c93b712..33f37ea 100644 --- a/pkg/app/staging.go +++ b/pkg/app/staging.go @@ -3,9 +3,12 @@ package app import ( "context" "errors" + "path/filepath" + "strings" "github.com/DomBlack/git-stack/pkg/exec" "github.com/DomBlack/git-stack/pkg/git" + "github.com/DomBlack/git-stack/pkg/shell" "github.com/DomBlack/git-stack/pkg/stack" ) @@ -86,11 +89,56 @@ func (a *App) commitInfo(ctx context.Context, repo git.Repo, sha string) CommitI return CommitInfo{SHA: sha, Subject: subject} } -// editorError maps a missing TTY for an editor-backed commit. -func editorError(err error, hint string) error { +// commitError maps the ways git commit fails that the user can act on: a +// missing TTY for an editor-backed commit (editorHint says how to pass the +// message instead) and a signing failure. +func commitError(err error, editorHint string) error { if errors.Is(err, exec.ErrTTYUnavailable) { return stack.New(stack.KindInteractionRequired, "a commit message editor is needed but no terminal is available"). - WithSteps(hint) + WithSteps(editorHint) + } + if se, ok := errors.AsType[*git.SigningError](err); ok { + return signingError(se) } 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 +// git fails instead. With gpg it is usually an agent whose pinentry can't +// prompt either. +func signingError(se *git.SigningError) error { + var e *stack.Error + switch se.Format { + case "ssh": + if se.Key == "" { + e = stack.New(stack.KindSigningFailed, "commit signing failed: gpg.format is ssh but user.signingkey is not set"). + WithSteps("set user.signingkey to the public key file to sign with") + } else { + // The private key sits next to the configured .pub; quote it + // so the line can be pasted (a path a shell can't take, one + // with a control character, gets described instead). + // git resolved a key file to an absolute path (or ~); anything + // else is a literal key, which can't be ssh-added by name. + step := "load the signing key into ssh-agent with ssh-add" + if filepath.IsAbs(se.Key) || strings.HasPrefix(se.Key, "~") { + if q, ok := shell.Path(strings.TrimSuffix(se.Key, ".pub")); ok { + step = "load the key into ssh-agent: ssh-add " + q + } else { + step = "load the private key next to " + se.Key + " into ssh-agent with ssh-add" + } + } + e = stack.Newf(stack.KindSigningFailed, "commit signing failed: the SSH key %s could not be used (is it unlocked in ssh-agent?)", se.Key). + WithSteps(step) + } + case "x509": + e = stack.New(stack.KindSigningFailed, "commit signing failed: the X.509 signer could not sign the commit (it may need a terminal to ask for the passphrase)"). + WithSteps("check that the signer (gpg.x509.program, gpgsm by default) can sign without a prompt") + default: + 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"). + WithDetail(se.Detail).WithCause(se) +} diff --git a/pkg/git/commit.go b/pkg/git/commit.go index f2cf802..8a72bca 100644 --- a/pkg/git/commit.go +++ b/pkg/git/commit.go @@ -4,6 +4,8 @@ import ( "context" "errors" "fmt" + "os" + "path/filepath" "strings" "github.com/DomBlack/git-stack/pkg/exec" @@ -126,13 +128,82 @@ func (c *Client) Commit(ctx context.Context, repo Repo, o CommitOptions) (string } if err != nil { if ee, ok := errors.AsType[*exec.ExitError](err); ok { - return "", fmt.Errorf("git commit: %s", strings.TrimSpace(ee.Result.Err())) + stderr := strings.TrimSpace(ee.Result.Err()) + if strings.Contains(stderr, signingFailedMarker) { + if se := c.signingError(ctx, repo, stderr); se != nil { + return "", se + } + } + return "", fmt.Errorf("git commit: %s", stderr) } return "", err } return c.RevParse(ctx, repo, "HEAD") } +// signingFailedMarker is what git prints when the signing program fails, +// whatever gpg.format is; the program's own output comes before it. Git +// prints the same line when it can't write the object at all (a read only +// object store), so it only means signing when signing is turned on. +const signingFailedMarker = "failed to write commit object" + +// SigningError reports that git could not sign a commit. Nothing was +// committed and the index is untouched. +type SigningError struct { + // Format is gpg.format: ssh, openpgp (the default) or x509. + Format string + // Key is user.signingkey as configured, which for ssh is usually the + // path of the public key file; empty when unset. + Key string + // Detail is git's stderr, including the signing program's output. + Detail string +} + +func (e *SigningError) Error() string { + return "git commit: signing failed: " + lastLine(e.Detail) +} + +// signingError builds a SigningError from git's stderr, reading the signing +// configuration so the caller can say which key to unlock. It returns nil +// when signing is off, as then the failure was writing the object itself. +func (c *Client) signingError(ctx context.Context, repo Repo, stderr string) error { + // Let git read the boolean: it takes yes/on/1 and a bare `gpgsign` + // with no value as true. Unset exits 1, which is false here too. + if res, err := c.config(ctx, repo, ScopeMerged, "--type=bool", "--get", "commit.gpgsign"); err != nil || res.Out() != "true" { + return nil + } + e := &SigningError{Format: "openpgp", Detail: stderr} + if v, ok, err := c.ConfigGet(ctx, repo, ScopeMerged, "gpg.format"); err == nil && ok && v != "" { + e.Format = v + } + if v, ok, err := c.ConfigGet(ctx, repo, ScopeMerged, "user.signingkey"); err == nil && ok { + e.Key = v + // For ssh the value is a key file when one exists at that path, + // relative to the repository where git runs the signer, and a + // literal key otherwise. Resolve a file so the path means the + // same from wherever the user is. + if e.Format == "ssh" && v != "" && !filepath.IsAbs(v) { + if p := filepath.Join(repo.TopLevel, v); fileExists(p) { + e.Key = p + } + } + } + return e +} + +func fileExists(p string) bool { + st, err := os.Stat(p) + return err == nil && !st.IsDir() +} + +func lastLine(s string) string { + s = strings.TrimSpace(s) + if i := strings.LastIndex(s, "\n"); i >= 0 { + return strings.TrimSpace(s[i+1:]) + } + return s +} + // Subject returns the first line of the commit message of rev. func (c *Client) Subject(ctx context.Context, repo Repo, rev string) (string, error) { res, err := c.gitIn(ctx, repo, "log", "-1", "--format=%s", rev) diff --git a/pkg/git/commit_test.go b/pkg/git/commit_test.go new file mode 100644 index 0000000..7b24f10 --- /dev/null +++ b/pkg/git/commit_test.go @@ -0,0 +1,132 @@ +package git_test + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/DomBlack/git-stack/pkg/exec" + "github.com/DomBlack/git-stack/pkg/git" + "github.com/DomBlack/git-stack/pkg/git/gittest" +) + +// TestCommitWithLockedSSHKeyFailsFast signs with a passphrase protected key +// that is not in any agent, the setup behind the MCP hang: ssh-keygen must +// fail at once rather than wait for a passphrase, and the failure must come +// back as a SigningError naming the key. +func TestCommitWithLockedSSHKeyFailsFast(t *testing.T) { + gittest.Isolate(t) + t.Setenv("SSH_AUTH_SOCK", "") + dir := gittest.InitRepo(t) + key := filepath.Join(t.TempDir(), "id_ed25519") + res, err := exec.New().Run(context.Background(), exec.Cmd{ + Name: "ssh-keygen", Args: []string{"-q", "-t", "ed25519", "-N", "secret", "-C", "test", "-f", key}, + }) + if _, missing := errors.AsType[*exec.NotFoundError](err); missing { + t.Skip("ssh-keygen not installed") + } + if err != nil { + t.Fatalf("ssh-keygen: %v\n%s", err, res.Err()) + } + gittest.Run(t, dir, "config", "gpg.format", "ssh") + gittest.Run(t, dir, "config", "user.signingkey", key+".pub") + gittest.Run(t, dir, "config", "commit.gpgsign", "true") + gittest.WriteFile(t, dir, "a.txt", "a\n") + gittest.Run(t, dir, "add", "a.txt") + + c := newClient() + ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second) + defer cancel() + repo, _ := c.Discover(ctx, dir) + start := time.Now() + _, err = c.Commit(ctx, repo, git.CommitOptions{Message: []string{"signed"}}) + if errors.Is(err, context.DeadlineExceeded) { + t.Fatal("git commit waited on the passphrase prompt instead of failing") + } + se, ok := errors.AsType[*git.SigningError](err) + if !ok { + t.Fatalf("err = %v (%T), want *git.SigningError", err, err) + } + if se.Format != "ssh" || se.Key != key+".pub" { + t.Errorf("SigningError = %+v, want format ssh and key %s.pub", se, key) + } + if !strings.Contains(se.Detail, "failed to write commit object") { + t.Errorf("Detail = %q, want git's stderr", se.Detail) + } + if time.Since(start) > 10*time.Second { + t.Errorf("commit took %s", time.Since(start)) + } + if staged, _ := c.HasStagedChanges(ctx, repo); !staged { + t.Error("the staged changes should still be staged") + } +} + +// TestCommitObjectWriteFailureIsNotSigning makes git print "failed to write +// commit object" for a reason that has nothing to do with signing (a read +// only object store); that must stay an ordinary error. +func TestCommitObjectWriteFailureIsNotSigning(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.WriteFile(t, dir, "a.txt", "a\n") + gittest.Run(t, dir, "add", "a.txt") + 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) + _, err := c.Commit(ctx, repo, git.CommitOptions{Message: []string{"x"}}) + if err == nil { + t.Fatal("the commit should fail with a read only object store") + } + if _, ok := errors.AsType[*git.SigningError](err); ok { + t.Fatalf("err = %v, classified as a signing failure", err) + } + if !strings.Contains(err.Error(), "git commit:") { + t.Errorf("err = %v, want git's message", err) + } +} + +// TestCommitBareGpgsignKeyIsSigning: git reads `[commit] gpgsign` with no +// value as true, so a signing failure under it must still be recognised. +func TestCommitBareGpgsignKeyIsSigning(t *testing.T) { + gittest.Isolate(t) + dir := gittest.InitRepo(t) + f, err := os.OpenFile(filepath.Join(dir, ".git", "config"), os.O_APPEND|os.O_WRONLY, 0o644) + if err != nil { + t.Fatal(err) + } + if _, err := f.WriteString("[commit]\n\tgpgsign\n"); err != nil { + t.Fatal(err) + } + _ = f.Close() + 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.WriteFile(t, dir, "keys/id_ed25519.pub", "ssh-ed25519 AAAAC3Nza test\n") + gittest.WriteFile(t, dir, "a.txt", "a\n") + gittest.Run(t, dir, "add", "a.txt") + + c := newClient() + ctx := context.Background() + repo, _ := c.Discover(ctx, dir) + _, err = c.Commit(ctx, repo, git.CommitOptions{Message: []string{"x"}}) + se, ok := errors.AsType[*git.SigningError](err) + if !ok { + t.Fatalf("err = %v (%T), want *git.SigningError", err, err) + } + // A relative key file is resolved against the repository. + if want := filepath.Join(repo.TopLevel, "keys", "id_ed25519.pub"); se.Key != want { + t.Errorf("Key = %q, want %q", se.Key, want) + } +} diff --git a/pkg/mcp/instructions.go b/pkg/mcp/instructions.go index bb7d4df..ecb66ae 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 "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. 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.` diff --git a/pkg/stack/errors.go b/pkg/stack/errors.go index b7dad07..bfe0ece 100644 --- a/pkg/stack/errors.go +++ b/pkg/stack/errors.go @@ -51,6 +51,9 @@ const ( // KindChecksPending: a merge was refused because checks are still // running on pull requests it would land; Checks says which. KindChecksPending + // KindSigningFailed: git could not sign a commit (a locked SSH key, a + // gpg agent that can't prompt); nothing was committed. + KindSigningFailed ) var kindCodes = map[Kind]string{ @@ -73,6 +76,7 @@ var kindCodes = map[Kind]string{ KindPartial: "partial", KindChecksFailing: "checks_failing", KindChecksPending: "checks_pending", + KindSigningFailed: "signing_failed", } // Code is the stable machine-readable name of the kind.