Centralize ANSI escape sequences in console package#14340
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR centralizes terminal ANSI escape sequences within the pkg/console package and updates call sites to use the shared helpers/constants, improving consistency and ensuring ANSI output is gated by TTY detection (stderr).
Changes:
- Added
pkg/console/terminal.gowith shared ANSI constants and cursor movement helpers guarded bytty.IsStderrTerminal(). - Updated existing console/CLI code to use centralized constants/helpers instead of inline escape sequences.
- Adjusted
ClearScreen()behavior to write to stderr for consistency with other console output.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/console/terminal.go | Introduces centralized ANSI constants and cursor movement helpers with stderr TTY gating. |
| pkg/console/terminal_test.go | Adds tests around cursor helpers and non-TTY behavior (currently has correctness/robustness issues). |
| pkg/console/console.go | Switches ClearScreen/ClearLine to shared ANSI constants and stderr TTY detection. |
| pkg/console/spinner.go | Replaces inline escape sequences with shared ANSI constants. |
| pkg/cli/add_interactive_orchestrator.go | Replaces inline clear-screen escape sequence with console.ClearScreen(). |
Comments suppressed due to low confidence (1)
pkg/console/terminal_test.go:114
- These assertions are ineffective: output is a string, so it can never be nil. If the intent is “does not panic”, use assert.NotPanics around MoveCursorDown, and/or assert the captured output is empty given captureStderr forces stderr to be non-TTY.
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
output := captureStderr(t, func() {
MoveCursorDown(tt.lines)
})
// In non-TTY environments, output should be empty
// We just ensure no panic occurs
assert.NotNil(t, output, "MoveCursorDown should not panic")
})
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func MoveCursorUp(n int) { | ||
| if tty.IsStderrTerminal() { | ||
| fmt.Fprintf(os.Stderr, "\033[%dA", n) | ||
| } | ||
| } | ||
|
|
||
| // MoveCursorDown moves cursor down n lines if stderr is a TTY. | ||
| // Uses ANSI escape code: \033[nB where n is the number of lines. | ||
| func MoveCursorDown(n int) { | ||
| if tty.IsStderrTerminal() { | ||
| fmt.Fprintf(os.Stderr, "\033[%dB", n) | ||
| } |
There was a problem hiding this comment.
MoveCursorUp/MoveCursorDown will emit escape sequences for n==0 and will format negative values into the ANSI sequence (e.g., "\033[-1A"), which is not meaningful and may behave inconsistently across terminals. Consider validating n > 0 and returning early otherwise (or clamping at 0) to make the API safer.
| // Save original stderr | ||
| oldStderr := os.Stderr | ||
|
|
||
| // Create a pipe to capture stderr | ||
| r, w, err := os.Pipe() | ||
| if err != nil { | ||
| t.Fatalf("Failed to create pipe: %v", err) | ||
| } | ||
|
|
||
| // Replace stderr with the write end of the pipe | ||
| os.Stderr = w | ||
|
|
||
| // Create a channel to receive the captured output | ||
| outputChan := make(chan string, 1) | ||
|
|
||
| // Read from the pipe in a goroutine | ||
| go func() { | ||
| var buf bytes.Buffer | ||
| io.Copy(&buf, r) | ||
| outputChan <- buf.String() | ||
| }() | ||
|
|
||
| // Execute the function | ||
| fn() | ||
|
|
||
| // Close the write end and restore stderr | ||
| w.Close() | ||
| os.Stderr = oldStderr | ||
|
|
||
| // Get the captured output | ||
| output := <-outputChan | ||
| r.Close() | ||
|
|
There was a problem hiding this comment.
captureStderr mutates global os.Stderr but doesn’t guarantee restoration/pipe closure if fn panics, which can leak the redirected stderr into later tests and can also deadlock the reader goroutine (io.Copy won’t finish until w is closed). Use t.Cleanup/defer to restore os.Stderr and close w/r, and consider recovering panic to ensure cleanup always runs.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| output := captureStderr(t, func() { | ||
| MoveCursorUp(tt.lines) | ||
| }) | ||
|
|
||
| // In non-TTY environments, output should be empty | ||
| // We just ensure no panic occurs | ||
| assert.NotNil(t, output, "MoveCursorUp should not panic") | ||
| }) |
There was a problem hiding this comment.
These assertions are ineffective: output is a string, so it can never be nil. If the intent is “does not panic”, use assert.NotPanics around MoveCursorUp, and/or assert the captured output is empty given captureStderr forces stderr to be non-TTY.
This issue also appears on line 105 of the same file.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| output := captureStderr(t, tt.fn) | ||
|
|
||
| // Since tests typically run in non-TTY, verify output is empty | ||
| // This ensures we properly respect TTY detection | ||
| if os.Getenv("CI") != "" || !isRealTerminal() { | ||
| assert.Empty(t, output, "%s should not output ANSI codes in non-TTY", tt.name) | ||
| } | ||
| }) |
There was a problem hiding this comment.
This test can become a no-op locally because the assertion is gated on CI env / isRealTerminal(). Since captureStderr always redirects stderr to a pipe (non-TTY), you can assert output is empty unconditionally here to actually validate the TTY guard behavior.
Manual ANSI escape codes were duplicated across 6 locations in
pkg/cliandpkg/console. This centralizes them into constants and functions with proper TTY detection.Changes
New module:
pkg/console/terminal.goansiClearScreen,ansiClearLine,ansiCarriageReturnMoveCursorUp(n),MoveCursorDown(n)with TTY detectiontty.IsStderrTerminal()before emitting ANSI codesUpdated existing code:
pkg/cli/add_interactive_orchestrator.go: Usesconsole.ClearScreen()instead of inline\033[H\033[2Jpkg/console/console.go:ClearScreen()andClearLine()now use constants fromterminal.gopkg/console/spinner.go: Three methods (render(),Stop(),StopWithMessage()) use constantsBug fix:
ClearScreen()now writes to stderr (was stdout), consistent with other console operationsExample
Before:
After:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.