Keep children of a TTY-less runner away from the terminal

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

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
+}

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}
+}

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

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</dev/tty"}})
+	return err == nil
+}
+
+func TestCaptureWithoutTTYDetachesFromTerminal(t *testing.T) {
+	hadTTY := hasControllingTerminal(t)
+	// A child of a runner with no TTY must not reach the terminal the
+	// process was started from, even when there is one: the MCP server's
+	// git would otherwise block on a passphrase prompt nobody can answer.
+	res, err := exec.New().Run(context.Background(), exec.Cmd{Name: "sh", Args: []string{"-c", "exec 3</dev/tty"}})
+	if err == nil {
+		t.Errorf("child of a TTY-less runner opened /dev/tty (test has terminal: %v)", hadTTY)
+	} else if _, ok := errors.AsType[*exec.ExitError](err); !ok {
+		t.Errorf("err = %v (%T), want *exec.ExitError from the failed open; stderr %q", err, err, res.Err())
+	}
+	// That check is vacuous where the test itself has no terminal (CI), so
+	// also check the mechanism: a child started in its own session leads
+	// its own process group, so its pgid is its pid. One that merely
+	// inherited ours is in our group instead.
+	pgid := func(r exec.Runner) string {
+		t.Helper()
+		res, err := r.Run(context.Background(), exec.Cmd{Name: "sh", Args: []string{"-c", `[ "$(ps -o pgid= -p $$ | tr -d ' ')" = "$$" ] && echo leader || echo inherited`}})
+		if err != nil {
+			t.Fatalf("ps: %v: %s", err, res.Err())
+		}
+		return res.Out()
+	}
+	if got := pgid(exec.New()); got != "leader" {
+		t.Errorf("child of a TTY-less runner is %s, want its own session leader", got)
+	}
+	if got := pgid(exec.New(exec.WithTTY(exec.TTY{In: os.Stdin, Out: os.Stdout, Err: os.Stderr}))); got != "inherited" {
+		t.Errorf("child of a TTY runner is %s, want to inherit our process group", got)
+	}
+}
+
+func TestCaptureWithoutTTYDisablesPrompts(t *testing.T) {
+	t.Setenv("GPG_TTY", "/dev/ttys000")
+	script := `echo "${GIT_TERMINAL_PROMPT:-unset}|${GPG_TTY:-unset}"`
+	res, err := exec.New().Run(context.Background(), exec.Cmd{Name: "sh", Args: []string{"-c", script}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got, want := res.Out(), "0|unset"; got != want {
+		t.Errorf("TTY-less env = %q, want %q", got, want)
+	}
+	// A runner with a terminal leaves prompts alone: the CLI user can answer them.
+	r := exec.New(exec.WithTTY(exec.TTY{In: os.Stdin, Out: os.Stdout, Err: os.Stderr}))
+	res, err = r.Run(context.Background(), exec.Cmd{Name: "sh", Args: []string{"-c", script}})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got, want := res.Out(), "unset|/dev/ttys000"; got != want {
+		t.Errorf("TTY env = %q, want %q", got, want)
+	}
+}