Merge branch 'main' into research/hermes-configmap-skills-delivery

Author: bussyjd

internal/agentruntime/exec.go

@@ -16,7 +16,7 @@ import (
 // no side effects. argv is the full in-pod command vector; argv[0] is the
 // binary (e.g. "python3" or a runtime CLI path), argv[1:] its args.
 //
-// withTTY toggles the `-t` flag; callers usually pass stdinIsTerminal().
+// withTTY toggles the `-t` flag; callers usually pass shouldRequestTTY().
 func BuildExecArgs(runtime Runtime, id string, argv []string, withTTY bool) []string {
 	svc := Describe(runtime).ServiceName
 	out := []string{"exec", "-i"}
@@ -50,7 +50,7 @@ func ExecInPod(cfg *config.Config, runtime Runtime, id string, argv []string) er
 
 	kubectlBinary := filepath.Join(cfg.BinDir, "kubectl")
 
-	cmd := exec.Command(kubectlBinary, BuildExecArgs(runtime, id, argv, stdinIsTerminal())...)
+	cmd := exec.Command(kubectlBinary, BuildExecArgs(runtime, id, argv, shouldRequestTTY())...)
 	cmd.Env = append(os.Environ(), "KUBECONFIG="+kubeconfigPath)
 	cmd.Stdin = os.Stdin
 	cmd.Stdout = os.Stdout
@@ -68,7 +68,35 @@ func ExecInPod(cfg *config.Config, runtime Runtime, id string, argv []string) er
 	return nil
 }
 
-func stdinIsTerminal() bool {
-	info, err := os.Stdin.Stat()
+// shouldRequestTTY reports whether `kubectl exec` should be invoked with -t.
+// It mirrors kubectl's own setupTTY decision: a TTY is allocated only when BOTH
+// the process stdin AND stdout are terminals.
+//
+// Gating on stdin alone is wrong. When obol is invoked under command
+// substitution — e.g. the release smoke's `buy_output=$(obol buy inference …)`
+// run from a tmux pane — stdin is a pty but stdout is a pipe. Requesting -t in
+// that case is harmful on two counts:
+//   - it corrupts captured output with TTY line-ending/escape semantics, and
+//   - kubectl < 1.36 panics with a nil-pointer dereference in its
+//     terminal-resize path (terminalSizeQueueAdapter.Next on a nil receiver)
+//     when -t is set but stdout is not a terminal.
+//
+// Requiring both streams to be terminals avoids both and matches kubectl.
+func shouldRequestTTY() bool {
+	return streamsSupportTTY(os.Stdin, os.Stdout)
+}
+
+// streamsSupportTTY is the pure predicate behind shouldRequestTTY: both streams
+// must be character devices (terminals). Factored out so it is unit-testable
+// without touching the process-wide os.Stdin/os.Stdout.
+func streamsSupportTTY(in, out *os.File) bool {
+	return isCharDevice(in) && isCharDevice(out)
+}
+
+func isCharDevice(f *os.File) bool {
+	if f == nil {
+		return false
+	}
+	info, err := f.Stat()
 	return err == nil && info.Mode()&os.ModeCharDevice != 0
 }

internal/agentruntime/exec_test.go

@@ -1,6 +1,7 @@
 package agentruntime
 
 import (
+	"os"
 	"reflect"
 	"testing"
 
@@ -77,6 +78,87 @@ func TestBuildExecArgs(t *testing.T) {
 	}
 }
 
+// TestStreamsSupportTTY locks in the kubectl-mirroring gate: -t is requested
+// only when BOTH stdin and stdout are terminals. The decisive regression case
+// is "stdin is a terminal but stdout is a pipe" (command substitution like the
+// release smoke's `$(obol buy inference …)` from a tmux pane), which previously
+// added -t and crashed kubectl < 1.36 in its terminal-resize path.
+func TestStreamsSupportTTY(t *testing.T) {
+	// A regular file is not a character device.
+	regular, err := os.CreateTemp(t.TempDir(), "stream")
+	if err != nil {
+		t.Fatalf("CreateTemp: %v", err)
+	}
+	t.Cleanup(func() { regular.Close() })
+
+	// Pipe ends are not character devices either (they model the captured-output
+	// case: stdout wired to a pipe under `$(...)`).
+	pr, pw, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("os.Pipe: %v", err)
+	}
+	t.Cleanup(func() { pr.Close(); pw.Close() })
+
+	// /dev/null IS a character device — a portable stand-in for a terminal here.
+	// The "char-device stdin, pipe stdout" case is the exact regression: the old
+	// stdin-only gate returned true (→ -t → kubectl < 1.36 panic); the two-stream
+	// gate must return false. Guarded so the table degrades gracefully if /dev/null
+	// is somehow unavailable.
+	devNull, dnErr := os.Open(os.DevNull)
+	if dnErr == nil {
+		t.Cleanup(func() { devNull.Close() })
+		if !isCharDevice(devNull) {
+			t.Fatalf("expected %s to be a character device", os.DevNull)
+		}
+	}
+
+	type ttyCase struct {
+		name string
+		in   *os.File
+		out  *os.File
+		want bool
+	}
+	tests := []ttyCase{
+		{"both pipes", pr, pw, false},
+		{"stdin pipe, stdout regular file", pr, regular, false},
+		{"stdin regular file, stdout pipe", regular, pw, false},
+		{"nil stdin", nil, pw, false},
+		{"nil stdout", pr, nil, false},
+		{"both nil", nil, nil, false},
+	}
+	if devNull != nil {
+		tests = append(tests,
+			// Regression: terminal stdin + pipe stdout must NOT request a TTY.
+			ttyCase{"char-device stdin, pipe stdout (regression)", devNull, pw, false},
+			// Both terminals is the only true case.
+			ttyCase{"both char devices", devNull, devNull, true},
+		)
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := streamsSupportTTY(tc.in, tc.out); got != tc.want {
+				t.Fatalf("streamsSupportTTY(%s) = %v, want %v", tc.name, got, tc.want)
+			}
+		})
+	}
+}
+
+// TestExecInPod_NoTTYWhenStdoutPiped is the end-to-end guarantee: when this test
+// process runs (under `go test`, stdout is captured, not a terminal),
+// shouldRequestTTY must be false so BuildExecArgs omits -t.
+func TestExecInPod_NoTTYWhenStdoutPiped(t *testing.T) {
+	if shouldRequestTTY() {
+		t.Skip("test stdout is a terminal; the piped-output gate cannot be exercised here")
+	}
+	args := BuildExecArgs(Hermes, DefaultInstanceID, []string{"true"}, shouldRequestTTY())
+	for _, a := range args {
+		if a == "-t" {
+			t.Fatalf("BuildExecArgs unexpectedly contains -t when stdout is not a terminal: %#v", args)
+		}
+	}
+}
+
 func TestExecInPod_EmptyArgvRejected(t *testing.T) {
 	cfg := &config.Config{ConfigDir: t.TempDir(), BinDir: t.TempDir()}
 	err := ExecInPod(cfg, Hermes, DefaultInstanceID, nil)