Fixing output colors and window resizing#527
Fixing output colors and window resizing#527Lasse Gaardsholt (Gaardsholt) wants to merge 7 commits intomasterfrom
Conversation
…ling; add tests for success and exit code scenarios Signed-off-by: Lasse Gaardsholt <lasse.gaardsholt@bestseller.com>
Signed-off-by: Lasse Gaardsholt <lasse.gaardsholt@bestseller.com>
…dPTY to use it - to make it not fail on windows builds Signed-off-by: Lasse Gaardsholt <lasse.gaardsholt@bestseller.com>
…run our tests on linux, macos and windows. Signed-off-by: Lasse Gaardsholt <lasse.gaardsholt@bestseller.com>
Signed-off-by: Lasse Gaardsholt <lasse.gaardsholt@bestseller.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the dev subcommand’s child-process execution to run inside a pseudo-terminal (PTY) so downstream CLI tools keep TTY behavior (notably colored output) while still redacting secrets, and adds terminal resize handling for better interactive UX.
Changes:
- Added
util.RunCmdPTYto execute commands under a PTY and stream output throughutil.Redactor. - Added OS-specific resize handling (
SIGWINCHon non-Windows; no-op on Windows). - Switched
cmd/dev.goto useRunCmdPTY, and updated Go dependencies (creack/pty,x/term,x/sys).
Reviewed changes
Copilot reviewed 6 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| util/ptyrun.go | New PTY-based command runner that redacts output and handles raw-mode input + resize hooks. |
| util/resize_unix.go | Adds SIGWINCH-driven PTY resizing on non-Windows platforms. |
| util/resize_windows.go | Provides a Windows no-op resize handler to satisfy cross-platform builds. |
| util/ptyrun_test.go | Adds basic tests for PTY execution success and exit-code propagation. |
| cmd/dev.go | Replaces manual Stdout/Stderr redactors with util.RunCmdPTY. |
| go.mod | Adds direct dependencies needed for PTY + terminal handling. |
| go.sum | Updates checksums for bumped/added modules. |
| .github/workflows/test.yml | Whitespace-only cleanup in permissions block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // RunCmdPTY runs the given command in a pseudo-terminal to preserve colored output, | ||
| // wiring its output through the Redactor to ensure sensitive values are stripped. | ||
| func RunCmdPTY(cmd *exec.Cmd, secretEnvs []string, redact bool) error { | ||
| // Start the command with a pseudo-terminal. |
There was a problem hiding this comment.
RunCmdPTY always starts the child under a PTY. This forces TTY behavior even when the caller’s stdout/stderr are redirected (e.g., piping to a file), which can introduce ANSI escape codes/progress UIs and changes behavior vs the previous pipe-based execution. Consider only using a PTY when the parent output is a TTY (e.g., term.IsTerminal on os.Stdout/os.Stderr) and falling back to the prior Stdout/Stderr redactor wiring when output is not a TTY.
| // Start the command with a pseudo-terminal. | |
| // Decide whether to use a PTY based on whether stdout/stderr are terminals. | |
| stdoutIsTerm := term.IsTerminal(int(os.Stdout.Fd())) | |
| stderrIsTerm := term.IsTerminal(int(os.Stderr.Fd())) | |
| usePTY := stdoutIsTerm || stderrIsTerm | |
| if !usePTY { | |
| // When output is not a TTY (e.g., being piped or redirected), avoid forcing PTY | |
| // behavior and instead wire stdout/stderr directly through the Redactor. | |
| redactor := &Redactor{ | |
| Writer: os.Stdout, | |
| Envs: secretEnvs, | |
| Redact: redact, | |
| } | |
| cmd.Stdin = os.Stdin | |
| cmd.Stdout = redactor | |
| cmd.Stderr = redactor | |
| if err := cmd.Start(); err != nil { | |
| return err | |
| } | |
| return cmd.Wait() | |
| } | |
| // Start the command with a pseudo-terminal to preserve interactive behavior. |
| // The pseudo-terminal unites both stdout and stderr and gives it to us here. | ||
| redactor := &Redactor{ | ||
| Writer: os.Stdout, | ||
| Envs: secretEnvs, | ||
| Redact: redact, |
There was a problem hiding this comment.
The PTY stream combines stdout and stderr, but the redactor is currently hard-wired to write everything to os.Stdout. This changes stream semantics for callers (errors no longer go to stderr), which can break tooling that relies on stderr separation. If you keep the PTY path, consider documenting this clearly and/or only using the PTY path for interactive TTY output while preserving separate stdout/stderr in the non-PTY fallback.
| // Copy os.Stdin directly to the pseudo-terminal | ||
| // Note: This goroutine intentionally leaks when the command finishes, | ||
| // as standard input reads block indefinitely. | ||
| go func() { | ||
| _, _ = io.Copy(ptyFile, os.Stdin) |
There was a problem hiding this comment.
The stdin->PTY io.Copy goroutine is documented as intentionally leaking because reads from os.Stdin can block indefinitely. If RunCmdPTY can be called more than once per process (or from long-running commands), this will accumulate blocked goroutines. Consider adding a way to stop the copy loop when the child exits (e.g., by copying from a cancellable reader / using an explicit context and OS-specific non-blocking reads), or gating stdin forwarding to interactive scenarios where the process exits immediately after the child command finishes.
| func TestRunCmdPTY_Success(t *testing.T) { | ||
| cmd := exec.Command("sh", "-c", "echo 'hello, world'") | ||
| secretEnvs := []string{"SUPER_SECRET_ENV"} |
There was a problem hiding this comment.
These tests shell out via sh, which will fail on Windows (and other environments without a POSIX shell). Since the project produces Windows binaries, consider adding a build tag (e.g., //go:build !windows) or skipping the test at runtime on unsupported platforms, and/or using a Go helper binary/script-free command for portability.
| // This should run smoothly and not throw EIO or panics | ||
| // even when standard input is not a real terminal (like in `go test` and CI). | ||
| err := RunCmdPTY(cmd, secretEnvs, redact) | ||
| if err != nil { | ||
| t.Fatalf("expected no error, got: %v", err) | ||
| } |
There was a problem hiding this comment.
RunCmdPTY writes command output directly to the real os.Stdout, so this test will emit output during go test runs. To keep test output clean and deterministic, consider temporarily redirecting os.Stdout/os.Stderr to a pipe/buffer during the test (and restoring afterwards), or refactoring RunCmdPTY to accept an io.Writer so tests can capture output without global state changes.
| } | ||
|
|
||
| if err := execCmd.Run(); err != nil { | ||
| if err := util.RunCmdPTY(execCmd, secretEnvs, redact); err != nil { |
There was a problem hiding this comment.
Switching dev execution to RunCmdPTY will also force TTY behavior and merge stderr into stdout for the child process. That’s a noticeable behavioral change if users pipe harpocrates dev ... output or rely on stderr separation. If this command is used in non-interactive contexts, consider choosing PTY vs non-PTY mode based on whether the parent stdout/stderr are TTYs (or adding a flag to control it).
Explaining What We Are Trying to Fix
CLI tools like
k6,kubectl,npm, etc., strip their colored output when they detect that their standard output/error streams aren't directly connected to a Terminal (TTY).Because we use
&util.Redactor{}instead ofos.Stdoutandos.Stderrdirectly withinexec.CommandContext, Go'sos/execpackage internally creates a pipe for the command's output, attaching&util.Redactor{}to one end. Since the child command detects that it is writing to a pipe instead of a TTY, it automatically disables its colorized output.The Fix
TL;DR: Let
github.com/creack/ptyhandle most of it for us.By using
github.com/creack/pty, we can start the child process inside a pseudo-terminal (PTY). This tricks child commands (likenpm,kubectl, etc.) into thinking they are directly connected to a TTY, preserving their colored output. We then read the output from the PTY, pipe it through ourutil.Redactorto strip any secrets, and finally print it toos.Stdout.Additionally, we added a new
RunCmdPTYfunction to theutilpackage to handle the PTY setup. This includes listening for terminal resize events and correctly restoring the terminal state after execution. Finally, we replaced the manualexecCmd.StdoutandexecCmd.Stderrassignments incmd/dev.gowith a call to this new function.Proofs
Output Colors
Before:

After:

Window Sizing
Before:

After:
