From 05cd61b6599d7fad95a33498da93efb8dcc04fae Mon Sep 17 00:00:00 2001 From: Dorian Karter Date: Mon, 5 Oct 2026 12:31:27 -0500 Subject: [PATCH] fix: preserve terminal stderr for ticket pickers --- e2e/ticket_terminal_test.go | 86 ++++++++ go.mod | 3 +- go.sum | 2 + internal/worktree/ticket_command.go | 16 +- internal/worktree/ticket_command_pty_test.go | 209 +++++++++++++++++++ internal/worktree/ticket_command_test.go | 45 ++++ openspec/specs/tickets-and-reviews/spec.md | 2 + 7 files changed, 360 insertions(+), 3 deletions(-) create mode 100644 e2e/ticket_terminal_test.go create mode 100644 internal/worktree/ticket_command_pty_test.go diff --git a/e2e/ticket_terminal_test.go b/e2e/ticket_terminal_test.go new file mode 100644 index 0000000..0d20dd5 --- /dev/null +++ b/e2e/ticket_terminal_test.go @@ -0,0 +1,86 @@ +//go:build darwin || linux + +package e2e + +import ( + "context" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "syscall" + "testing" + "time" + + "github.com/creack/pty" +) + +func TestREV010_TicketPickerInheritsTerminalWithoutShellRedirection(t *testing.T) { + s := newSandbox(t) + repo := s.repo() + herdr := s.fakeHerdr(repo) + s.tool("tickets", ` +[ -t 0 ] && [ -t 2 ] || { printf 'picker needs terminal stdin/stderr\n' >&2; exit 2; } +size=$(/bin/stty size <&2) +[ "$size" = '30 100' ] || { printf 'incorrect dimensions: %s\n' "$size" >&2; exit 2; } +printf 'PICKER_READY %s\n' "$size" >&2 +IFS= read -r selection +[ "$selection" = select ] || exit 23 +printf '%s\n' '{"branchName":"test-123-selected","metadata":{"identifier":"TEST-123"}}' +`) + mustWrite(t, filepath.Join(repo, ".herdr-worktree.yaml"), "ticket_commands:\n default:\n command: [tickets, '{input}']\n", 0o600) + master, slave, err := pty.Open() + if err != nil { + t.Fatal(err) + } + defer slave.Close() + fd, err := syscall.Dup(int(master.Fd())) + _ = master.Close() + if err != nil { + t.Fatal(err) + } + syscall.CloseOnExec(fd) + if err := syscall.SetNonblock(fd, true); err != nil { + _ = syscall.Close(fd) + t.Fatal(err) + } + master = os.NewFile(uintptr(fd), "ticket-pty") + defer master.Close() + if err := master.SetReadDeadline(time.Now().Add(15 * time.Second)); err != nil { + t.Fatal(err) + } + if err := pty.Setsize(slave, &pty.Winsize{Rows: 30, Cols: 100}); err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, hwtBinary, "--herdr-bin", herdr, "create", "--ticket", "--json") + cmd.Dir, cmd.Env = repo, s.env + cmd.Stdin, cmd.Stderr = slave, slave + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + cmd.Cancel = func() error { return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } + cmd.WaitDelay = time.Second + var stdout strings.Builder + cmd.Stdout = &stdout + // This canonical-mode input is buffered until the fake picker reads it. + if _, err := io.WriteString(master, "select\n"); err != nil { + t.Fatal(err) + } + if err := cmd.Run(); err != nil { + t.Fatalf("ticket create failed: %v; stdout: %s", err, stdout.String()) + } + result := decode(t, stdout.String()) + if result["branch"] != "test-123-selected" { + t.Fatalf("selected branch = %#v", result) + } + gitDir := s.git(result["path"].(string), "rev-parse", "--git-dir") + metadata := mustRead(t, filepath.Join(gitDir, "hwt-ticket-metadata-v1.json")) + requireContains(t, metadata, "TEST-123") + ui := make([]byte, 4096) + n, err := master.Read(ui) + if err != nil { + t.Fatal(err) + } + requireContains(t, string(ui[:n]), "PICKER_READY 30 100") +} diff --git a/go.mod b/go.mod index 3e93e89..da10d6c 100644 --- a/go.mod +++ b/go.mod @@ -5,6 +5,8 @@ go 1.25.0 require ( charm.land/bubbletea/v2 v2.0.8 charm.land/lipgloss/v2 v2.0.5 + github.com/charmbracelet/x/term v0.2.2 + github.com/creack/pty v1.1.24 github.com/spf13/cobra v1.10.2 go.yaml.in/yaml/v3 v3.0.4 golang.org/x/sys v0.47.0 @@ -14,7 +16,6 @@ require ( github.com/charmbracelet/colorprofile v0.4.3 // indirect github.com/charmbracelet/ultraviolet v0.0.0-20260703014108-f5a850f9c2b7 // indirect github.com/charmbracelet/x/ansi v0.11.7 // indirect - github.com/charmbracelet/x/term v0.2.2 // indirect github.com/charmbracelet/x/termios v0.1.1 // indirect github.com/charmbracelet/x/windows v0.2.2 // indirect github.com/clipperhouse/displaywidth v0.11.0 // indirect diff --git a/go.sum b/go.sum index 87cb3ec..6587209 100644 --- a/go.sum +++ b/go.sum @@ -23,6 +23,8 @@ github.com/clipperhouse/displaywidth v0.11.0/go.mod h1:bkrFNkf81G8HyVqmKGxsPufD3 github.com/clipperhouse/uax29/v2 v2.7.0 h1:+gs4oBZ2gPfVrKPthwbMzWZDaAFPGYK72F0NJv2v7Vk= github.com/clipperhouse/uax29/v2 v2.7.0/go.mod h1:EFJ2TJMRUaplDxHKj1qAEhCtQPW2tJSwu5BF98AuoVM= github.com/cpuguy83/go-md2man/v2 v2.0.6/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= +github.com/creack/pty v1.1.24 h1:bJrF4RRfyJnbTJqzRLHzcGaZK1NeM5kTC9jGgovnR1s= +github.com/creack/pty v1.1.24/go.mod h1:08sCNb52WyoAwi2QDyzUCTgcvVFhUzewun7wtTfvcwE= github.com/inconshreveable/mousetrap v1.1.0 h1:wN+x4NVGpMsO7ErUn/mUI3vEoE6Jt13X2s0bqwp9tc8= github.com/inconshreveable/mousetrap v1.1.0/go.mod h1:vpF70FUmC8bwa3OWnCshd2FqLfsEA9PFc4w1p2J65bw= github.com/lucasb-eyer/go-colorful v1.4.0 h1:UtrWVfLdarDgc44HcS7pYloGHJUjHV/4FwW4TvVgFr4= diff --git a/internal/worktree/ticket_command.go b/internal/worktree/ticket_command.go index 18bdb56..de2eb48 100644 --- a/internal/worktree/ticket_command.go +++ b/internal/worktree/ticket_command.go @@ -10,6 +10,7 @@ import ( "os/exec" "strings" + "github.com/charmbracelet/x/term" "github.com/dkarter/hwt/internal/config" ) @@ -27,13 +28,24 @@ func runTicketCommandWithStderr(cwd string, ticket config.TicketCommand, input s cmd.Stdin = os.Stdin var stdout, stderr bytes.Buffer cmd.Stdout = &stdout - cmd.Stderr = io.MultiWriter(terminalStderr, &stderr) + // exec.Cmd only inherits a file descriptor when the writer is an *os.File. + // Wrapping a terminal in MultiWriter creates a pipe instead, hiding terminal + // dimensions and preventing interactive pickers from rendering their UI. + terminalFile, isFile := terminalStderr.(*os.File) + interactiveStderr := isFile && term.IsTerminal(terminalFile.Fd()) + if interactiveStderr { + cmd.Stderr = terminalFile + } else { + cmd.Stderr = io.MultiWriter(terminalStderr, &stderr) + } if err := cmd.Run(); err != nil { if errors.Is(err, exec.ErrNotFound) { return ticketResult{}, fmt.Errorf("find ticket command %q: %w", ticket.Command[0], err) } message := "" - if strings.TrimSpace(stderr.String()) == "" { + // Terminal diagnostics have already been displayed and are not captured; + // only use stdout as a fallback when we know stderr was empty. + if !interactiveStderr && strings.TrimSpace(stderr.String()) == "" { message = strings.TrimSpace(stdout.String()) } if message != "" { diff --git a/internal/worktree/ticket_command_pty_test.go b/internal/worktree/ticket_command_pty_test.go new file mode 100644 index 0000000..6a60836 --- /dev/null +++ b/internal/worktree/ticket_command_pty_test.go @@ -0,0 +1,209 @@ +//go:build darwin || linux + +package worktree + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "os" + "os/exec" + "strings" + "syscall" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/charmbracelet/x/term" + "github.com/creack/pty" + "github.com/dkarter/hwt/internal/config" +) + +// Exercise both subprocess boundaries: HWT inherits terminal stdin/stderr, +// then launches a picker with captured JSON stdout and terminal UI on stderr. +func TestRunTicketCommandInteractiveTerminal(t *testing.T) { + for _, test := range []struct { + name string + key string + want string + }{ + {name: "select", key: "\r", want: "feature/selected"}, + {name: "cancel", key: "\x1b", want: "exit status 23"}, + } { + t.Run(test.name, func(t *testing.T) { + master, slave, err := pty.Open() + if err != nil { + t.Fatal(err) + } + defer slave.Close() + // Register a nonblocking descriptor with Go's poller so deadlines + // and Close can interrupt the UI reader, including on Darwin. + fd, err := syscall.Dup(int(master.Fd())) + _ = master.Close() + if err != nil { + t.Fatal(err) + } + syscall.CloseOnExec(fd) + if err := syscall.SetNonblock(fd, true); err != nil { + _ = syscall.Close(fd) + t.Fatal(err) + } + master = os.NewFile(uintptr(fd), "picker-pty") + defer master.Close() + if err := pty.Setsize(slave, &pty.Winsize{Rows: 30, Cols: 100}); err != nil { + t.Fatal(err) + } + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) + defer cancel() + deadline, _ := ctx.Deadline() + if err := master.SetReadDeadline(deadline); err != nil { + t.Fatal(err) + } + cmd := exec.CommandContext(ctx, executable, "-test.run=^TestTicketPickerProcess$", "--", "transport") + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + // Cancel the picker as well as its transport parent on timeout. + cmd.Cancel = func() error { return syscall.Kill(-cmd.Process.Pid, syscall.SIGKILL) } + cmd.WaitDelay = 2 * time.Second + cmd.Stdin, cmd.Stderr = slave, slave + var stdout bytes.Buffer + cmd.Stdout = &stdout + if err := cmd.Start(); err != nil { + t.Fatal(err) + } + // Read while the UI runs; wait for the measured size before sending input. + uiDone := make(chan string, 1) + go func() { + var ui strings.Builder + buf := make([]byte, 4096) + sent := false + for { + n, err := master.Read(buf) + ui.Write(buf[:n]) + if !sent && strings.Contains(ui.String(), "PICKER_READY 100x30") { + _, _ = io.WriteString(master, test.key) + sent = true + } + if err != nil { + uiDone <- ui.String() + return + } + } + }() + waitErr := cmd.Wait() + _ = slave.Close() + // Darwin PTYs need the master closed to unblock a read after slave exit. + _ = master.Close() + ui := <-uiDone + if waitErr != nil { + t.Fatalf("transport: %v; UI: %q; stdout: %q", waitErr, ui, stdout.String()) + } + if !strings.Contains(ui, "PICKER_READY 100x30") { + t.Fatalf("picker did not receive terminal dimensions: %q", ui) + } + var result struct { + Branch string `json:"branch"` + Metadata map[string]string `json:"metadata"` + Error string `json:"error"` + } + if err := json.Unmarshal(stdout.Bytes(), &result); err != nil { + t.Fatalf("UI leaked into JSON stdout %q: %v", stdout.String(), err) + } + if test.name == "select" { + if result.Branch != test.want || result.Metadata["identifier"] != "TEST-123" || result.Error != "" { + t.Fatalf("selected result = %#v", result) + } + } else if !strings.Contains(result.Error, test.want) || result.Branch != "" { + t.Fatalf("cancellation result = %#v", result) + } + }) + } +} + +// Invoked only as a subprocess, with fake ticket data and no API access. +func TestTicketPickerProcess(t *testing.T) { + args := os.Args + if len(args) < 2 || args[len(args)-2] != "--" { + return + } + switch args[len(args)-1] { + case "transport": + executable, err := os.Executable() + if err != nil { + t.Fatal(err) + } + result, err := runTicketCommand("", config.TicketCommand{ + Command: []string{executable, "-test.run=^TestTicketPickerProcess$", "--", "picker"}, + }, "") + output := map[string]any{"branch": result.BranchName, "metadata": result.Metadata} + if err != nil { + output["error"] = err.Error() + } + if err := json.NewEncoder(os.Stdout).Encode(output); err != nil { + t.Fatal(err) + } + os.Exit(0) // Keep the test runner's PASS line out of machine-readable stdout. + case "picker": + if !term.IsTerminal(os.Stdin.Fd()) || !term.IsTerminal(os.Stderr.Fd()) { + fmt.Fprintln(os.Stderr, "picker stdin/stderr must be terminals") + os.Exit(2) + } + result, err := tea.NewProgram(ticketPickerTestModel{}, tea.WithInput(os.Stdin), tea.WithOutput(os.Stderr)).Run() + if err != nil { + fmt.Fprintln(os.Stderr, err) + os.Exit(2) + } + model := result.(ticketPickerTestModel) + if model.selected && model.width == 100 && model.height == 30 { + fmt.Println(`{"branchName":"feature/selected","metadata":{"identifier":"TEST-123"}}`) + os.Exit(0) + } + if model.cancelled { + os.Exit(23) + } + fmt.Fprintln(os.Stderr, "picker timed out or received incorrect dimensions") + os.Exit(2) + } +} + +type ticketPickerTestTimeout struct{} + +type ticketPickerTestModel struct { + width, height int + selected bool + cancelled bool +} + +func (m ticketPickerTestModel) Init() tea.Cmd { + return tea.Tick(5*time.Second, func(time.Time) tea.Msg { return ticketPickerTestTimeout{} }) +} + +func (m ticketPickerTestModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { + switch msg := msg.(type) { + case tea.WindowSizeMsg: + m.width, m.height = msg.Width, msg.Height + case tea.KeyPressMsg: + switch msg.String() { + case "enter": + m.selected = true + return m, tea.Quit + case "esc": + m.cancelled = true + return m, tea.Quit + } + case ticketPickerTestTimeout: + return m, tea.Quit + } + return m, nil +} + +func (m ticketPickerTestModel) View() tea.View { + view := tea.NewView(fmt.Sprintf("PICKER_READY %dx%d\nEnter selects; Escape cancels", m.width, m.height)) + view.AltScreen = true + return view +} diff --git a/internal/worktree/ticket_command_test.go b/internal/worktree/ticket_command_test.go index 26f6206..4e55f29 100644 --- a/internal/worktree/ticket_command_test.go +++ b/internal/worktree/ticket_command_test.go @@ -2,6 +2,9 @@ package worktree import ( "bytes" + "errors" + "os" + "os/exec" "reflect" "strings" "testing" @@ -147,4 +150,46 @@ func TestRunTicketCommandDoesNotRepeatStreamedFailureStderr(t *testing.T) { if got := terminalStderr.String(); got != "authentication required\n" { t.Fatalf("streamed stderr = %q", got) } + var exitErr *exec.ExitError + if !errors.As(err, &exitErr) || exitErr.ExitCode() != 23 { + t.Fatalf("exit status was not preserved: %v", err) + } +} + +func TestRunTicketCommandRedirectedStderr(t *testing.T) { + file, err := os.CreateTemp(t.TempDir(), "stderr") + if err != nil { + t.Fatal(err) + } + defer file.Close() + ticket := config.TicketCommand{Command: []string{"sh", "-c", `printf 'diagnostic\n' >&2; printf 'do not repeat stdout\n'; exit 23`}} + _, err = runTicketCommandWithStderr(t.TempDir(), ticket, "", file) + if err == nil || strings.Contains(err.Error(), "diagnostic") || strings.Contains(err.Error(), "do not repeat stdout") { + t.Fatalf("failure repeated already streamed diagnostics or unrelated stdout: %v", err) + } + data, err := os.ReadFile(file.Name()) + if err != nil { + t.Fatal(err) + } + if string(data) != "diagnostic\n" { + t.Fatalf("redirected stderr = %q", data) + } +} + +func TestRunTicketCommandFailureFallsBackToStdout(t *testing.T) { + var stderr bytes.Buffer + ticket := config.TicketCommand{Command: []string{"sh", "-c", `printf 'failure detail\n'; exit 23`}} + _, err := runTicketCommandWithStderr(t.TempDir(), ticket, "", &stderr) + if err == nil || !strings.Contains(err.Error(), "failure detail") { + t.Fatalf("missing stdout failure diagnostic: %v", err) + } +} + +func TestRunTicketCommandMissingExecutable(t *testing.T) { + var stderr bytes.Buffer + ticket := config.TicketCommand{Command: []string{"hwt-nonexistent-ticket-command"}} + _, err := runTicketCommandWithStderr(t.TempDir(), ticket, "", &stderr) + if err == nil || !errors.Is(err, exec.ErrNotFound) { + t.Fatalf("missing executable error = %v", err) + } } diff --git a/openspec/specs/tickets-and-reviews/spec.md b/openspec/specs/tickets-and-reviews/spec.md index 7d982aa..c55641c 100644 --- a/openspec/specs/tickets-and-reviews/spec.md +++ b/openspec/specs/tickets-and-reviews/spec.md @@ -40,6 +40,8 @@ HWT SHALL normalize whitespace in a positional title to hyphens unless `--ticket - GIVEN `ticket_commands.default` supports interactive selection and its final argument is `{input}` - WHEN the user runs `hwt create --ticket` without a query - THEN HWT omits that argument, connects terminal input and stderr for the picker, and uses its mapped branch from stdout JSON +- AND terminal stderr retains its terminal identity and dimensions without a shell redirection workaround +- AND picker UI remains separate from stdout JSON, including ticket metadata ### Requirement: Open a branch pull request