Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion docs/mcp.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion pkg/app/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <message> (the branch was created; commit with git commit)")
return res, commitError(err, "pass -m <message> (the branch was created; commit with git commit)")

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what #29 in the same stack is for: create undoes the branch and its registration when the commit fails, so running it again is the right recovery there. On its own this PR only improves the message, and I'd rather not grow a second recovery path that #29 then deletes.

}
info := a.commitInfo(ctx, repo, sha)
res.Commit = &info
Expand Down
2 changes: 1 addition & 1 deletion pkg/app/modify.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 <message> or --no-edit")
return ModifyResult{}, commitError(err, "pass -m <message> or --no-edit")
}
res.Commit = a.commitInfo(ctx, repo, sha)

Expand Down
47 changes: 47 additions & 0 deletions pkg/app/mutate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
})
}
}
54 changes: 51 additions & 3 deletions pkg/app/staging.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
Comment on lines +124 to +133

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user.signingkey with a control character in it is a config I'm happy to print badly; the Detail already carries whatever git printed, which includes the same path. Leaving this one.

}
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")
Comment thread
Copilot marked this conversation as resolved.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair that it's not definitive, but it's a hint, not a check we act on; the real test is running the command again, which the next steps say. Leaving the wording.

}
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)
}
73 changes: 72 additions & 1 deletion pkg/git/commit.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"context"
"errors"
"fmt"
"os"
"path/filepath"
"strings"

"github.com/DomBlack/git-stack/pkg/exec"
Expand Down Expand Up @@ -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)
Expand Down
132 changes: 132 additions & 0 deletions pkg/git/commit_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
2 changes: 1 addition & 1 deletion pkg/mcp/instructions.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.`
Loading
Loading