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.