diff --git a/docs/adr/42945-route-lipgloss-v2-output-through-colorprofile-writer.md b/docs/adr/42945-route-lipgloss-v2-output-through-colorprofile-writer.md new file mode 100644 index 00000000000..e0415d244d3 --- /dev/null +++ b/docs/adr/42945-route-lipgloss-v2-output-through-colorprofile-writer.md @@ -0,0 +1,44 @@ +# ADR-42945: Route Lip Gloss v2 Output Through colorprofile.Writer + +**Date**: 2026-07-02 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +Lip Gloss v2 removed automatic terminal capability detection that was present in v1. As a result, styled output paths in `pkg/console` and `pkg/logger` began emitting raw ANSI escape codes regardless of `NO_COLOR`, `COLORTERM`, or terminal type — including piped output and limited-color terminals. This violates the [NO_COLOR](https://no-color.org) convention and can corrupt output on terminals that do not support truecolor. The codebase has multiple write sites that emit directly to `os.Stderr`, which bypasses any capability probing entirely. + +### Decision + +We will route all styled stderr output through a `colorprofile.Writer` (from `github.com/charmbracelet/colorprofile`) wrapper rather than writing to `os.Stderr` directly. A shared factory function `stderrWriter()` is introduced in `pkg/console` (with a passthrough stub for wasm builds) so all output paths consistently consult the environment for `NO_COLOR`, `COLORTERM`, and terminal capability before rendering ANSI sequences. + +### Alternatives Considered + +#### Alternative 1: Manual NO_COLOR guard at each write site + +Each styled `fmt.Fprintf(os.Stderr, ...)` call could be preceded by an `if os.Getenv("NO_COLOR") == ""` check. This is the lowest-effort change but requires repetitive, error-prone guards scattered across every output site, with no coverage for terminal capability degradation (e.g., downgrading truecolor to 256-color on constrained terminals). It does not scale as new write sites are added. + +#### Alternative 2: Configure a global Lip Gloss renderer with the correct profile + +Lip Gloss v2 exposes a `Renderer` that can be constructed with a specific `colorprofile.Profile`. A single global renderer could be initialized once at startup and used for all style rendering. This would centralize capability probing but requires replacing every `lipgloss.NewStyle()` call with renderer-scoped style construction, a larger refactor surface. It also does not address `lipgloss.Fprintf` calls that accept an `io.Writer` rather than using a renderer. + +### Consequences + +#### Positive +- `NO_COLOR` is correctly honored across all `pkg/console` and `pkg/logger` output paths without per-call guards. +- Terminal capability is degraded appropriately (e.g., truecolor → 256-color → plain) based on the detected environment at write time. +- Wasm builds retain their original `os.Stderr` passthrough via a build-tagged stub, preserving existing behavior on that platform. + +#### Negative +- `stderrWriter()` calls `colorprofile.NewWriter(os.Stderr, os.Environ())` on every invocation, allocating a new writer wrapper per write site call rather than reusing a singleton. This introduces minor per-call overhead that could be avoided with a cached writer. +- The `newColorProfileWriter`/`stderrWriter` helper functions are duplicated between `pkg/console` and `pkg/logger`, creating two independent copies that could drift in behavior. [TODO: verify] whether consolidating into a shared package is feasible. + +#### Neutral +- Existing tests for `NO_COLOR` behavior in `pkg/logger` (`TestNoColorEnvironment`) continue to pass; new regression tests confirm ANSI stripping via the colorprofile writer path. +- The `pkg/styles/theme.go` comment update clarifying `adaptiveColor` vs `lipgloss.LightDark` usage is documentation-only and carries no runtime impact. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/go.mod b/go.mod index 71fc695ea64..d96ee591154 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.7 charm.land/huh/v2 v2.0.3 charm.land/lipgloss/v2 v2.0.4 + github.com/charmbracelet/colorprofile v0.4.3 github.com/charmbracelet/x/exp/golden v0.0.0-20260622092256-25656177ba8e github.com/charmbracelet/x/term v0.2.2 github.com/cli/go-gh/v2 v2.13.0 @@ -43,7 +44,6 @@ require ( github.com/catppuccin/go v0.3.0 // indirect github.com/ccojocar/zxcvbn-go v1.0.4 // indirect github.com/cespare/xxhash/v2 v2.3.0 // indirect - github.com/charmbracelet/colorprofile v0.4.3 // indirect github.com/charmbracelet/harmonica v0.2.0 // indirect github.com/charmbracelet/ultraviolet v0.0.0-20260525132238-948f4557a654 // indirect github.com/charmbracelet/x/ansi v0.11.7 // indirect diff --git a/pkg/cli/compile_schedule_calendar.go b/pkg/cli/compile_schedule_calendar.go index a31d0c76c04..49626c74fa2 100644 --- a/pkg/cli/compile_schedule_calendar.go +++ b/pkg/cli/compile_schedule_calendar.go @@ -200,7 +200,7 @@ func intensityChar(count int) string { func intensityStyle(count int, isTerminal bool) lipgloss.Style { if !isTerminal { // Keep glyph rendering unchanged while preventing ANSI escapes in piped output. - return lipgloss.Style{} + return lipgloss.NewStyle() } switch { diff --git a/pkg/console/banner.go b/pkg/console/banner.go index 0dc89a386ea..07a48e811d2 100644 --- a/pkg/console/banner.go +++ b/pkg/console/banner.go @@ -5,7 +5,6 @@ package console import ( _ "embed" "fmt" - "os" "strings" lipgloss "charm.land/lipgloss/v2" @@ -31,6 +30,7 @@ func FormatBanner() string { // PrintBanner prints the ASCII logo to stderr with purple GitHub color theme. // This is used by the --banner flag to display the logo at the start of command execution. func PrintBanner() { - fmt.Fprintln(os.Stderr, FormatBanner()) - fmt.Fprintln(os.Stderr) + out := stderrWriter() + fmt.Fprintln(out, FormatBanner()) + fmt.Fprintln(out) } diff --git a/pkg/console/colorprofile_writer.go b/pkg/console/colorprofile_writer.go new file mode 100644 index 00000000000..8cfbbb2b2bc --- /dev/null +++ b/pkg/console/colorprofile_writer.go @@ -0,0 +1,18 @@ +//go:build !js && !wasm + +package console + +import ( + "io" + "os" + + "github.com/charmbracelet/colorprofile" +) + +func newColorProfileWriter(w io.Writer, environ []string) io.Writer { + return colorprofile.NewWriter(w, environ) +} + +func stderrWriter() io.Writer { + return newColorProfileWriter(os.Stderr, os.Environ()) +} diff --git a/pkg/console/colorprofile_writer_test.go b/pkg/console/colorprofile_writer_test.go new file mode 100644 index 00000000000..3911ec3602f --- /dev/null +++ b/pkg/console/colorprofile_writer_test.go @@ -0,0 +1,23 @@ +//go:build !integration && !js && !wasm + +package console + +import ( + "bytes" + "fmt" + "strings" + "testing" +) + +func TestColorProfileWriterStripsANSIWithNoColor(t *testing.T) { + var buf bytes.Buffer + w := newColorProfileWriter(&buf, []string{"NO_COLOR=1", "TERM=xterm-256color"}) + + if _, err := fmt.Fprint(w, "\x1b[38;2;255;0;0mhello\x1b[0m"); err != nil { + t.Fatalf("write failed: %v", err) + } + + if strings.Contains(buf.String(), "\x1b[") { + t.Fatalf("expected ANSI-free output with NO_COLOR, got %q", buf.String()) + } +} diff --git a/pkg/console/colorprofile_writer_wasm.go b/pkg/console/colorprofile_writer_wasm.go new file mode 100644 index 00000000000..136e3e7c139 --- /dev/null +++ b/pkg/console/colorprofile_writer_wasm.go @@ -0,0 +1,16 @@ +//go:build js || wasm + +package console + +import ( + "io" + "os" +) + +func newColorProfileWriter(w io.Writer, _ []string) io.Writer { + return w +} + +func stderrWriter() io.Writer { + return os.Stderr +} diff --git a/pkg/console/confirm.go b/pkg/console/confirm.go index a24d4106f8b..ccb25ec37b6 100644 --- a/pkg/console/confirm.go +++ b/pkg/console/confirm.go @@ -51,11 +51,12 @@ func ConfirmAction(title, affirmative, negative string) (bool, error) { // showTextConfirm displays a non-interactive confirmation prompt for non-TTY environments func showTextConfirm(title, affirmative, negative string, reader io.Reader) (bool, error) { confirmLog.Printf("Showing text confirm: title=%s", title) + out := stderrWriter() - fmt.Fprintf(os.Stderr, "\n%s\n\n", title) - fmt.Fprintf(os.Stderr, " 1) %s\n", affirmative) - fmt.Fprintf(os.Stderr, " 2) %s\n", negative) - fmt.Fprintf(os.Stderr, "\nEnter y/yes/1 to confirm, n/no/2 to cancel: ") + fmt.Fprintf(out, "\n%s\n\n", title) + fmt.Fprintf(out, " 1) %s\n", affirmative) + fmt.Fprintf(out, " 2) %s\n", negative) + fmt.Fprintf(out, "\nEnter y/yes/1 to confirm, n/no/2 to cancel: ") var input string _, err := fmt.Fscan(reader, &input) diff --git a/pkg/console/console.go b/pkg/console/console.go index cb5dfa156a5..40ef11db79d 100644 --- a/pkg/console/console.go +++ b/pkg/console/console.go @@ -5,7 +5,6 @@ package console import ( "errors" "fmt" - "os" "strconv" "strings" @@ -449,16 +448,17 @@ func RenderInfoSection(content string) []string { // RenderComposedSections composes and outputs a slice of sections to stderr func RenderComposedSections(sections []string) { + out := stderrWriter() if tty.IsStderrTerminal() { plan := lipgloss.JoinVertical(lipgloss.Left, sections...) - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, plan) - fmt.Fprintln(os.Stderr, "") + fmt.Fprintln(out, "") + fmt.Fprintln(out, plan) + fmt.Fprintln(out, "") } else { - fmt.Fprintln(os.Stderr, "") + fmt.Fprintln(out, "") for _, section := range sections { - fmt.Fprintln(os.Stderr, section) + fmt.Fprintln(out, section) } - fmt.Fprintln(os.Stderr, "") + fmt.Fprintln(out, "") } } diff --git a/pkg/console/list.go b/pkg/console/list.go index b09498c5ea3..5b0b383c225 100644 --- a/pkg/console/list.go +++ b/pkg/console/list.go @@ -5,7 +5,6 @@ package console import ( "errors" "fmt" - "os" "charm.land/huh/v2" "github.com/github/gh-aw/pkg/logger" @@ -65,15 +64,16 @@ func ShowInteractiveList(title string, items []ListItem) (string, error) { // showTextList displays a non-interactive numbered list for non-TTY environments func showTextList(title string, items []ListItem) (string, error) { listLog.Printf("Showing text list: title=%s, items=%d", title, len(items)) + out := stderrWriter() - fmt.Fprintf(os.Stderr, "\n%s\n\n", title) + fmt.Fprintf(out, "\n%s\n\n", title) for i, item := range items { - fmt.Fprintf(os.Stderr, " %d) %s\n", i+1, item.title) + fmt.Fprintf(out, " %d) %s\n", i+1, item.title) if item.description != "" { - fmt.Fprintf(os.Stderr, " %s\n", item.description) + fmt.Fprintf(out, " %s\n", item.description) } } - fmt.Fprintf(os.Stderr, "\nSelect (1-%d): ", len(items)) + fmt.Fprintf(out, "\nSelect (1-%d): ", len(items)) var choice int _, err := fmt.Scanf("%d", &choice) diff --git a/pkg/console/spinner.go b/pkg/console/spinner.go index 0480c941a2c..443fe737165 100644 --- a/pkg/console/spinner.go +++ b/pkg/console/spinner.go @@ -40,7 +40,7 @@ package console import ( "fmt" - "os" + "io" "sync" "charm.land/bubbles/v2/spinner" @@ -60,7 +60,7 @@ type updateMessageMsg string type spinnerModel struct { spinner spinner.Model message string - output *os.File + output io.Writer } func (m spinnerModel) Init() tea.Cmd { return m.spinner.Tick } @@ -91,6 +91,7 @@ func (m spinnerModel) render() { // SpinnerWrapper wraps the spinner functionality with TTY detection and Bubble Tea program type SpinnerWrapper struct { program *tea.Program + out io.Writer enabled bool running bool mu sync.Mutex @@ -104,19 +105,20 @@ func NewSpinner(message string) *SpinnerWrapper { isAccessible := IsAccessibleMode() enabled := isTTY && !isAccessible spinnerLog.Printf("Creating spinner: message=%q, tty=%t, accessible=%t, enabled=%t", message, isTTY, isAccessible, enabled) - s := &SpinnerWrapper{enabled: enabled} + out := stderrWriter() + s := &SpinnerWrapper{enabled: enabled, out: out} if enabled { model := spinnerModel{ spinner: spinner.New(spinner.WithSpinner(spinner.MiniDot), spinner.WithStyle(styles.Info)), message: message, - output: os.Stderr, + output: out, } // tea.WithInput(nil) disables stdin reading so the spinner does not consume key // events that should be handled by subsequent interactive forms (e.g. huh.Select). // Ctrl+C is still handled via OS signal delivery (SIGINT), which bubbletea // processes independently of the input reader. - s.program = tea.NewProgram(model, tea.WithOutput(os.Stderr), tea.WithoutRenderer(), tea.WithInput(nil)) + s.program = tea.NewProgram(model, tea.WithOutput(out), tea.WithoutRenderer(), tea.WithInput(nil)) } return s } @@ -170,7 +172,7 @@ func (s *SpinnerWrapper) Stop() { spinnerLog.Print("Stopping spinner") s.program.Quit() s.wg.Wait() // Wait for the goroutine to complete - fmt.Fprintf(os.Stderr, "%s%s", ansiCarriageReturn, ansiClearLine) + fmt.Fprintf(s.out, "%s%s", ansiCarriageReturn, ansiClearLine) } } } @@ -189,14 +191,14 @@ func (s *SpinnerWrapper) StopWithMessage(msg string) { if wasRunning { s.program.Quit() s.wg.Wait() // Wait for the goroutine to complete - fmt.Fprintf(os.Stderr, "%s%s%s\n", ansiCarriageReturn, ansiClearLine, msg) + fmt.Fprintf(s.out, "%s%s%s\n", ansiCarriageReturn, ansiClearLine, msg) } else { // Still print the message even if spinner wasn't running - fmt.Fprintf(os.Stderr, "%s\n", msg) + fmt.Fprintf(s.out, "%s\n", msg) } } else if msg != "" { // If spinner is disabled, still print the message for user feedback - fmt.Fprintf(os.Stderr, "%s\n", msg) + fmt.Fprintf(s.out, "%s\n", msg) } } diff --git a/pkg/console/terminal.go b/pkg/console/terminal.go index 1cee1d06f73..bff9002efcf 100644 --- a/pkg/console/terminal.go +++ b/pkg/console/terminal.go @@ -2,7 +2,6 @@ package console import ( "fmt" - "os" "github.com/github/gh-aw/pkg/styles" "github.com/github/gh-aw/pkg/tty" @@ -24,7 +23,7 @@ const ( // Uses ANSI escape codes for cross-platform compatibility func ClearScreen() { if tty.IsStderrTerminal() { - fmt.Fprint(os.Stderr, ansiClearScreen) + fmt.Fprint(stderrWriter(), ansiClearScreen) } } @@ -32,7 +31,7 @@ func ClearScreen() { // Uses ANSI escape codes: \r moves cursor to start, \033[K clears to end of line func ClearLine() { if tty.IsStderrTerminal() { - fmt.Fprintf(os.Stderr, "%s%s", ansiCarriageReturn, ansiClearLine) + fmt.Fprintf(stderrWriter(), "%s%s", ansiCarriageReturn, ansiClearLine) } } @@ -44,9 +43,10 @@ func ShowWelcomeBanner(description string) { if tty.IsStderrTerminal() { header = styles.Header.Render(header) } - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, header) - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, description) - fmt.Fprintln(os.Stderr, "") + out := stderrWriter() + fmt.Fprintln(out, "") + fmt.Fprintln(out, header) + fmt.Fprintln(out, "") + fmt.Fprintln(out, description) + fmt.Fprintln(out, "") } diff --git a/pkg/console/verbose.go b/pkg/console/verbose.go index 75c6dfb99a8..30fdff7f2d9 100644 --- a/pkg/console/verbose.go +++ b/pkg/console/verbose.go @@ -2,13 +2,12 @@ package console import ( "fmt" - "os" ) // LogVerbose outputs a verbose message to stderr only when verbose mode is enabled. // This is a convenience helper to avoid repetitive if-verbose checks throughout the codebase. func LogVerbose(verbose bool, message string) { if verbose { - fmt.Fprintln(os.Stderr, FormatVerboseMessage(message)) + fmt.Fprintln(stderrWriter(), FormatVerboseMessage(message)) } } diff --git a/pkg/logger/colorprofile_writer.go b/pkg/logger/colorprofile_writer.go new file mode 100644 index 00000000000..192110c6722 --- /dev/null +++ b/pkg/logger/colorprofile_writer.go @@ -0,0 +1,18 @@ +//go:build !js && !wasm + +package logger + +import ( + "io" + "os" + + "github.com/charmbracelet/colorprofile" +) + +func newColorProfileWriter(w io.Writer, environ []string) io.Writer { + return colorprofile.NewWriter(w, environ) +} + +func stderrWriter() io.Writer { + return newColorProfileWriter(os.Stderr, os.Environ()) +} diff --git a/pkg/logger/colorprofile_writer_wasm.go b/pkg/logger/colorprofile_writer_wasm.go new file mode 100644 index 00000000000..55ed3ba2c0f --- /dev/null +++ b/pkg/logger/colorprofile_writer_wasm.go @@ -0,0 +1,16 @@ +//go:build js || wasm + +package logger + +import ( + "io" + "os" +) + +func newColorProfileWriter(w io.Writer, _ []string) io.Writer { + return w +} + +func stderrWriter() io.Writer { + return os.Stderr +} diff --git a/pkg/logger/logger.go b/pkg/logger/logger.go index 569a3b09366..7a693752873 100644 --- a/pkg/logger/logger.go +++ b/pkg/logger/logger.go @@ -3,6 +3,7 @@ package logger import ( "fmt" "hash/fnv" + "image/color" "os" "strings" "sync" @@ -30,23 +31,31 @@ var ( // DEBUG_COLORS environment variable to control color output. debugColors = os.Getenv("DEBUG_COLORS") != "0" //nolint:osgetenvlibrary - // Color palette for namespace coloring, using adaptive styles. - colorPalette = []lipgloss.Style{ - lipgloss.NewStyle().Foreground(styles.ColorInfo), - lipgloss.NewStyle().Foreground(styles.ColorSuccess), - lipgloss.NewStyle().Foreground(styles.ColorWarning), - lipgloss.NewStyle().Foreground(styles.ColorPurple), - lipgloss.NewStyle().Foreground(styles.ColorYellow), - lipgloss.NewStyle().Foreground(styles.ColorError), - lipgloss.NewStyle().Foreground(styles.ColorComment), - lipgloss.NewStyle().Foreground(styles.ColorForeground), - lipgloss.NewStyle().Foreground(styles.ColorBorder), - lipgloss.NewStyle().Foreground(styles.ColorInfo), - lipgloss.NewStyle().Foreground(styles.ColorSuccess), - lipgloss.NewStyle().Foreground(styles.ColorPurple), + basePaletteColors = []color.Color{ + styles.ColorInfo, + styles.ColorSuccess, + styles.ColorWarning, + styles.ColorPurple, + styles.ColorYellow, + styles.ColorError, + styles.ColorComment, + styles.ColorForeground, + styles.ColorBorder, } + + // Color palette for namespace coloring, using adaptive styles. + colorPalette = buildColorPalette() ) +func buildColorPalette() []lipgloss.Style { + order := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 3} + palette := make([]lipgloss.Style, 0, len(order)) + for _, idx := range order { + palette = append(palette, lipgloss.NewStyle().Foreground(basePaletteColors[idx])) + } + return palette +} + // initDebugEnv resolves the effective debug pattern. // If DEBUG is set, it takes precedence. Otherwise, if ACTIONS_RUNNER_DEBUG=true, // all loggers are enabled (equivalent to DEBUG=*). @@ -115,7 +124,7 @@ func (l *Logger) Printf(format string, args ...any) { diff := l.tickTime() message := fmt.Sprintf(format, args...) - lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) + lipgloss.Fprintf(stderrWriter(), "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) } // Print prints a message if the logger is enabled. @@ -128,7 +137,7 @@ func (l *Logger) Print(args ...any) { diff := l.tickTime() message := fmt.Sprint(args...) - lipgloss.Fprintf(os.Stderr, "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) + lipgloss.Fprintf(stderrWriter(), "%s %s +%s\n", l.label, message, timeutil.FormatDuration(diff)) } func (l *Logger) tickTime() time.Duration { diff --git a/pkg/styles/theme.go b/pkg/styles/theme.go index 6e21b17e4fd..875c3321a32 100644 --- a/pkg/styles/theme.go +++ b/pkg/styles/theme.go @@ -66,6 +66,9 @@ var hasDarkBackground = true // adaptiveColor selects between a light and a dark color variant based on the // terminal background detected at startup. +// Use this for shared package-level colors that should follow gh-aw's startup +// probe; for per-render selection in one-off UI code, prefer lipgloss.LightDark +// (see huh_theme.go). type adaptiveColor struct { Light color.Color Dark color.Color