diff --git a/docs/architecture.md b/docs/architecture.md index 5cca428..069abc0 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -59,8 +59,17 @@ and nothing outside `pkg/exec` touches `os/exec`. Everything goes through `pkg/exec.Runner`. Capture is the default. Passthrough (a real TTY handed to the child) is opt in, only exists on the CLI runner, and is only used for `git add -p`, editors, and `gh stack submit`'s interactive editor. The MCP runner is built -without a TTY so it physically can't enter passthrough mode, which is how we guarantee an -MCP tool never blocks waiting on a terminal. +without a TTY so it physically can't enter passthrough mode. + +That alone wasn't enough to keep an MCP tool from blocking on a terminal. Capture mode +hands the child a closed stdin, but ssh-keygen, gpg and git's credential code prompt on +`/dev/tty`, the controlling terminal, which every child inherits from the terminal the MCP +server was started in; a `stack_create` with `commit.gpgsign` and a locked SSH key sat on a +passphrase prompt nobody could see. So a runner without a TTY starts each child in its own +session (`setsid`, so there is no controlling terminal and `/dev/tty` fails to open), sets +`GIT_TERMINAL_PROMPT=0` and drops `GPG_TTY`. The prompt turns into an immediate error that +the use case can explain. A CLI runner with a terminal does none of this: its user can +answer the prompt. `pkg/git` also retries a command that failed because another process held `.git/index.lock`, backing off from 25ms up to 250ms for about 2 seconds in total before diff --git a/pkg/exec/detach_other.go b/pkg/exec/detach_other.go new file mode 100644 index 0000000..9c3490f --- /dev/null +++ b/pkg/exec/detach_other.go @@ -0,0 +1,11 @@ +//go:build !unix + +package exec + +import "syscall" + +// detached is a no-op where sessions don't exist; there is no /dev/tty to +// keep a child away from either. +func detached() *syscall.SysProcAttr { + return nil +} diff --git a/pkg/exec/detach_unix.go b/pkg/exec/detach_unix.go new file mode 100644 index 0000000..cf1031b --- /dev/null +++ b/pkg/exec/detach_unix.go @@ -0,0 +1,12 @@ +//go:build unix + +package exec + +import "syscall" + +// detached returns the process attributes that start a child in its own +// session, so it has no controlling terminal: opening /dev/tty fails +// instead of reaching the terminal this process was started from. +func detached() *syscall.SysProcAttr { + return &syscall.SysProcAttr{Setsid: true} +} diff --git a/pkg/exec/runner.go b/pkg/exec/runner.go index a0a1c5b..63ae505 100644 --- a/pkg/exec/runner.go +++ b/pkg/exec/runner.go @@ -6,6 +6,13 @@ // whatever the caller supplies (never the parent's stdin). Passthrough mode // attaches the real terminal and is only available to runners built with // WithTTY; the MCP server never constructs such a runner. +// +// A runner without a TTY also keeps its children away from the terminal the +// process happens to have been started from: they run in their own session +// (no controlling terminal, so /dev/tty cannot be opened), git's terminal +// prompts are disabled and GPG_TTY is dropped. Otherwise a passphrase or +// credential prompt from ssh-keygen, gpg or git would wait on a terminal +// nobody is watching, which is how the MCP server used to hang. package exec import ( @@ -139,6 +146,15 @@ func WithTTY(t TTY) Option { // context is cancelled before killing it. const waitDelay = 2 * time.Second +// noPromptEnv and noPromptUnset apply to every child of a runner without a +// TTY. GIT_TERMINAL_PROMPT=0 makes git fail a credential prompt outright +// rather than look for a terminal; dropping GPG_TTY stops gpg pointing its +// pinentry at the terminal this process inherited. +var ( + noPromptEnv = []string{"GIT_TERMINAL_PROMPT=0"} + noPromptUnset = []string{"GPG_TTY"} +) + type runner struct { debug io.Writer tty *TTY @@ -161,6 +177,10 @@ func (r *runner) Run(ctx context.Context, c Cmd) (Result, error) { cmd := osexec.CommandContext(ctx, c.Name, c.Args...) cmd.Dir = c.Dir cmd.Env = BuildEnv(os.Environ(), c.Env, c.Unset) + if r.tty == nil { + cmd.Env = BuildEnv(cmd.Env, noPromptEnv, noPromptUnset) + cmd.SysProcAttr = detached() + } cmd.WaitDelay = waitDelay cmd.Cancel = func() error { // Give the child a chance to clean up (e.g. git rebase state) before diff --git a/pkg/exec/runner_test.go b/pkg/exec/runner_test.go index d9208f9..b1e7494 100644 --- a/pkg/exec/runner_test.go +++ b/pkg/exec/runner_test.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "errors" + "os" + "runtime" "strings" "testing" "time" @@ -120,3 +122,67 @@ func TestBuildEnv(t *testing.T) { t.Errorf("BuildEnv = %v, want %v", got, want) } } + +// hasControllingTerminal reports whether this test process can open +// /dev/tty, which is what ssh-keygen and gpg do to prompt for a passphrase. +func hasControllingTerminal(t *testing.T) bool { + t.Helper() + if runtime.GOOS == "windows" { + t.Skip("no /dev/tty on windows") + } + _, err := exec.New(exec.WithTTY(exec.TTY{In: os.Stdin, Out: os.Stdout, Err: os.Stderr})). + Run(context.Background(), exec.Cmd{Name: "sh", Args: []string{"-c", "exec 3