diff --git a/.github/workflows/agentic-auto-upgrade.yml b/.github/workflows/agentic-auto-upgrade.yml index 7035101ee35..e169cea32b6 100644 --- a/.github/workflows/agentic-auto-upgrade.yml +++ b/.github/workflows/agentic-auto-upgrade.yml @@ -34,7 +34,7 @@ name: Agentic Auto-Upgrade on: schedule: - - cron: "21 3 * * 5" # Weekly (auto-upgrade) + - cron: "11 4 * * 6" # Weekly (auto-upgrade) workflow_dispatch: permissions: diff --git a/docs/adr/46197-hoist-palette-vars-and-honor-no-color-stdout.md b/docs/adr/46197-hoist-palette-vars-and-honor-no-color-stdout.md new file mode 100644 index 00000000000..ad7dbb654f8 --- /dev/null +++ b/docs/adr/46197-hoist-palette-vars-and-honor-no-color-stdout.md @@ -0,0 +1,56 @@ +# ADR-46197: Hoist Palette Color Vars and Honor NO_COLOR on Stdout + +**Date**: 2026-07-18 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +The `pkg/styles` and `pkg/console` packages use lipgloss v2 for terminal styling. Two related defects were surfaced during a lipgloss v2 Go Fan review: + +1. `huh_theme.go` called `lipgloss.Color(hexConstant)` inside `HuhTheme(isDark)` on every invocation, re-parsing the same hex strings that `theme.go` already parsed once at package init for its `adaptiveColor` vars — duplicating work and creating two independent `color.Color` objects from the same source constants. + +2. `applyStyle` in `pkg/console` gated on TTY presence only (`isTTY()`), but did not consult the color-profile environment (`NO_COLOR`, `COLORTERM`, `TERM`). A terminal session with `NO_COLOR=1` still received raw ANSI escape sequences from string-returning format helpers, while the equivalent stderr path already honored the color profile via `stderrWriter()`. + +### Decision + +We will make two coordinated changes to address both defects: + +1. Declare 22 package-level `color.Color` vars in `pkg/styles/theme.go`, parsed once from the existing hex constants at init time, and have both the `adaptiveColor` structs and `huh_theme.go`'s `LightDark` calls reference these shared vars — eliminating duplicate hex parsing and establishing a single source of truth for the palette. + +2. Add `colorwriter.Stdout()` and `colorwriter.Degrade(s, environ)` to `pkg/colorwriter`, then update `applyStyle` in `pkg/console` to call `colorwriter.Degrade` when stdout is a TTY, routing rendered ANSI through a `colorprofile.Writer` that honors `NO_COLOR`, `COLORTERM`, and `TERM` — matching the existing stderr behavior. + +### Alternatives Considered + +#### Alternative 1: Keep Inline `lipgloss.Color()` Calls in `huh_theme.go` + +Leave `huh_theme.go` parsing hex strings on each `HuhTheme(isDark)` call. This is self-contained and requires no structural change. Rejected because it duplicates parsing work already done in `theme.go` and creates two independent `color.Color` objects from the same constants, violating the single-source-of-truth principle and adding unnecessary per-render overhead. + +#### Alternative 2: Route All Stdout Output Through an `io.Writer` Pipeline + +Replace the string-returning `applyStyle` pattern with an io.Writer pipeline where all styling writes directly to `colorwriter.Stdout()`. This would eliminate the need for a separate `Degrade` step. Rejected because it would require refactoring every caller from string-returning helpers to an io.Writer-passing API — a significantly larger change that does not align with the cleanup scope of this PR. + +#### Alternative 3: Check `NO_COLOR` Inline in `applyStyle` + +Manually check `os.Getenv("NO_COLOR") != ""` inside `applyStyle` before applying styles. Simpler than using `colorwriter.Degrade`, but covers only `NO_COLOR` and ignores the full color-profile spec (`COLORTERM`, `TERM`, capability-level downgrading). Rejected because `colorprofile.Writer` already encodes the full spec, and partial coverage would leave users of non-standard terminals without proper degradation. + +### Consequences + +#### Positive +- Single source of truth for palette colors: hex-to-`color.Color` parsing happens exactly once at package init, shared by both the `adaptiveColor` (startup-probe) path and the `LightDark` (per-render) path in huh themes. +- `NO_COLOR`, `COLORTERM`, and `TERM` are now honored for all stdout-bound styled strings, matching the existing behavior on the stderr path. +- Wasm build remains fully functional: platform stubs for `Stdout()` and `Degrade()` pass through unchanged. + +#### Negative +- `applyStyle` now calls `colorwriter.Degrade` on every TTY invocation, adding one extra string allocation and a `colorprofile.Writer` pass compared to the previous direct `style.Render(text)` return. +- Adding 22 package-level `color.Color` vars to `pkg/styles` marginally increases module initialization footprint. + +#### Neutral +- The two rendering paths (`adaptiveColor` structs for startup-probe selection vs `lipgloss.LightDark` for per-render selection in huh themes) remain architecturally distinct; this PR makes the shared palette the common underpinning but does not unify the selection strategies. +- `applyStyleWithTTY` (stderr-bound helpers) is unchanged; those paths already obtain colorprofile degradation at print time through `stderrWriter()`. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/colorwriter/README.md b/pkg/colorwriter/README.md index a5f38a7a505..03c87a9ac7a 100644 --- a/pkg/colorwriter/README.md +++ b/pkg/colorwriter/README.md @@ -6,7 +6,7 @@ The `colorwriter` package provides a factory for `io.Writer` values that adapt ANSI color output based on the current environment. On non-wasm platforms it wraps the given writer with [`github.com/charmbracelet/colorprofile`](https://pkg.go.dev/github.com/charmbracelet/colorprofile) so that `NO_COLOR`, `COLORTERM`, and terminal capability are consulted automatically. On wasm (`js` / `wasm` build tags) the package returns the writer unchanged, since color-profile detection is not supported on that platform. -It is imported by `pkg/console` and `pkg/logger` to obtain a consistent stderr writer. +It is imported by `pkg/console` and `pkg/logger` to obtain consistent stdout/stderr writers and to degrade already-rendered ANSI strings when output helpers must return a string instead of writing directly. ## Public API @@ -14,13 +14,15 @@ It is imported by `pkg/console` and `pkg/logger` to obtain a consistent stderr w |--------|-----------|-------------| | `New` | `func(w io.Writer, environ []string) io.Writer` | Returns a color-profile-aware writer wrapping `w` using `environ` (e.g. `os.Environ()`) to detect `NO_COLOR`, `COLORTERM`, and terminal capabilities. On wasm, returns `w` unchanged. | | `Stderr` | `func() io.Writer` | Convenience wrapper that calls `New(os.Stderr, os.Environ())`. On wasm, returns `os.Stderr` directly. | +| `Stdout` | `func() io.Writer` | Convenience wrapper that calls `New(os.Stdout, os.Environ())`. On wasm, returns `os.Stdout` directly. | +| `Degrade` | `func(s string, environ []string) string` | Routes a pre-rendered ANSI string through a color-profile-aware writer backed by a string builder, downgrading or stripping ANSI according to `environ`. On wasm, returns `s` unchanged. | ### Build variants | Build constraint | Behavior | |-----------------|----------| -| `!js && !wasm` (`colorprofile_writer.go`) | `New` delegates to `colorprofile.NewWriter`; `Stderr` wraps `os.Stderr` with the process environment. | -| `js \|\| wasm` (`colorprofile_writer_wasm.go`) | `New` returns `w` unchanged; `Stderr` returns `os.Stderr` directly. Color-profile detection is not supported on wasm. | +| `!js && !wasm` (`colorprofile_writer.go`) | `New` delegates to `colorprofile.NewWriter`; `Stderr` and `Stdout` wrap the corresponding standard streams with the process environment; `Degrade` transforms a rendered ANSI string through an in-memory color-profile-aware writer. | +| `js \|\| wasm` (`colorprofile_writer_wasm.go`) | `New` returns `w` unchanged; `Stderr` and `Stdout` return the corresponding standard streams directly; `Degrade` returns the original string unchanged. Color-profile detection is not supported on wasm. | ## Usage Examples @@ -38,6 +40,10 @@ fmt.Fprintln(w, "styled output respects NO_COLOR and terminal capabilities") // Obtain a ready-to-use stderr writer. stderr := colorwriter.Stderr() fmt.Fprintln(stderr, "styled output to os.Stderr") + +// Degrade a rendered ANSI string before returning it to a stdout caller. +plain := colorwriter.Degrade("\x1b[31mwarning\x1b[0m", []string{"NO_COLOR=1", "TERM=xterm-256color"}) +fmt.Println(plain) ``` ## Dependencies @@ -55,7 +61,7 @@ This appendix is generated from the current non-test Go source files in this pac | Types | 0 | | Constants | 0 | | Variables | 0 | -| Functions and methods | 2 | +| Functions and methods | 4 | | Additional symbols documented in this appendix | 0 | The sections above already mention every exported top-level symbol in the current source tree. diff --git a/pkg/colorwriter/colorprofile_writer.go b/pkg/colorwriter/colorprofile_writer.go index efd566ca42f..1415d20b079 100644 --- a/pkg/colorwriter/colorprofile_writer.go +++ b/pkg/colorwriter/colorprofile_writer.go @@ -5,6 +5,7 @@ package colorwriter import ( "io" "os" + "strings" "github.com/charmbracelet/colorprofile" ) @@ -20,3 +21,25 @@ func New(w io.Writer, environ []string) io.Writer { func Stderr() io.Writer { return New(os.Stderr, os.Environ()) } + +// Stdout returns a color-profile-aware writer for os.Stdout using the current +// process environment. +func Stdout() io.Writer { + return New(os.Stdout, os.Environ()) +} + +// Degrade returns s with ANSI sequences downgraded (or stripped) according to +// the current process environment (NO_COLOR, COLORTERM, TERM). It is intended +// for use with string-returning format helpers: render the style first, then +// call Degrade so that the caller's output honors the color profile. +func Degrade(s string, environ []string) string { + var buf strings.Builder + w := colorprofile.NewWriter(&buf, environ) + // colorprofile.Writer writes synchronously and does not buffer past Write, + // and strings.Builder writes cannot fail, so a write error would indicate an + // unexpected future behavior change; fall back to the original string then. + if _, err := io.WriteString(w, s); err != nil { + return s + } + return buf.String() +} diff --git a/pkg/colorwriter/colorprofile_writer_wasm.go b/pkg/colorwriter/colorprofile_writer_wasm.go index 5029535b10c..7807faab83c 100644 --- a/pkg/colorwriter/colorprofile_writer_wasm.go +++ b/pkg/colorwriter/colorprofile_writer_wasm.go @@ -17,3 +17,14 @@ func New(w io.Writer, _ []string) io.Writer { func Stderr() io.Writer { return os.Stderr } + +// Stdout returns os.Stdout directly; color-profile detection is not supported +// on wasm. +func Stdout() io.Writer { + return os.Stdout +} + +// Degrade returns s unchanged; color-profile detection is not supported on wasm. +func Degrade(s string, _ []string) string { + return s +} diff --git a/pkg/colorwriter/spec_test.go b/pkg/colorwriter/spec_test.go index 2fca41a66ae..3c4f9547526 100644 --- a/pkg/colorwriter/spec_test.go +++ b/pkg/colorwriter/spec_test.go @@ -46,3 +46,52 @@ func TestSpec_PublicAPI_Stderr(t *testing.T) { require.NotNil(t, w, "Stderr should return a non-nil io.Writer") assert.Implements(t, (*io.Writer)(nil), w, "Stderr should return an io.Writer as documented") } + +// TestSpec_PublicAPI_Stdout validates the documented behavior of Stdout from the +// README.md specification. +func TestSpec_PublicAPI_Stdout(t *testing.T) { + w := colorwriter.Stdout() + require.NotNil(t, w, "Stdout should return a non-nil io.Writer") + assert.Implements(t, (*io.Writer)(nil), w, "Stdout should return an io.Writer as documented") +} + +// TestSpec_PublicAPI_Degrade validates the documented behavior of Degrade from the +// README.md specification. +func TestSpec_PublicAPI_Degrade(t *testing.T) { + const ansiRed = "\x1b[31mhello\x1b[0m" + + tests := []struct { + name string + environ []string + want string + }{ + { + name: "strips ansi when NO_COLOR is set", + environ: []string{"NO_COLOR=1", "TERM=xterm-256color"}, + want: "hello", + }, + { + name: "strips ansi for dumb terminals", + environ: []string{"TERM=dumb"}, + want: "hello", + }, + { + name: "preserves ansi for forced color profiles", + environ: []string{"TERM=xterm-256color", "CLICOLOR_FORCE=1"}, + want: ansiRed, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := colorwriter.Degrade(ansiRed, tt.environ) + if tt.want == ansiRed { + assert.Contains(t, got, "hello") + assert.Contains(t, got, "\x1b[31m") + assert.Contains(t, got, "\x1b[m") + return + } + assert.Equal(t, tt.want, got) + }) + } +} diff --git a/pkg/console/console.go b/pkg/console/console.go index f603ae632cd..06b5ee1df48 100644 --- a/pkg/console/console.go +++ b/pkg/console/console.go @@ -5,11 +5,14 @@ package console import ( "errors" "fmt" + "os" "strconv" "strings" + "sync" lipgloss "charm.land/lipgloss/v2" "charm.land/lipgloss/v2/table" + "github.com/github/gh-aw/pkg/colorwriter" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/styles" "github.com/github/gh-aw/pkg/tty" @@ -17,6 +20,10 @@ import ( var consoleLog = logger.New("console:console") +// stdoutEnviron caches the process environment on first use so stdout styling +// helpers do not repeatedly copy and re-parse it while rendering output. +var stdoutEnviron = sync.OnceValue(os.Environ) + // isTTY checks if stdout is a terminal func isTTY() bool { return tty.IsStdoutTerminal() @@ -27,12 +34,24 @@ func isStderrTTY() bool { return tty.IsStderrTerminal() } -// applyStyle conditionally applies styling based on TTY status +// applyStyle conditionally applies styling based on TTY status and color profile. +// When stdout is a TTY the rendered ANSI is downgraded through the colorprofile +// writer so that NO_COLOR, COLORTERM, and TERM are honored. func applyStyle(style lipgloss.Style, text string) string { - return applyStyleWithTTY(style, text, isTTY) + return applyStdoutStyleWithTTY(style, text, isTTY, stdoutEnviron()) } -// applyStyleWithTTY conditionally applies styling based on a provided TTY check. +func applyStdoutStyleWithTTY(style lipgloss.Style, text string, ttyCheck func() bool, environ []string) string { + if !ttyCheck() { + return text + } + return colorwriter.Degrade(style.Render(text), environ) +} + +// applyStyleWithTTY conditionally renders raw ANSI based on a provided TTY check. +// Use this only for strings that will later be written through a color-profile- +// aware writer (for example stderrWriter); stdout-facing helpers should use +// applyStdoutStyleWithTTY so environment-based degradation happens here. func applyStyleWithTTY(style lipgloss.Style, text string, ttyCheck func() bool) string { if ttyCheck() { return style.Render(text) @@ -154,29 +173,37 @@ func renderContext(err CompilerError) string { // FormatSuccessMessage formats a success message with styling func FormatSuccessMessage(message string) string { - return formatSuccessMessageWithTTY(message, isTTY) + return formatSuccessMessageWithTTY(message, isTTY, stdoutEnviron()) } // FormatSuccessMessageStderr formats a success message for stderr output. func FormatSuccessMessageStderr(message string) string { - return formatSuccessMessageWithTTY(message, isStderrTTY) + return formatSuccessMessageStderrWithTTY(message, isStderrTTY) } -func formatSuccessMessageWithTTY(message string, ttyCheck func() bool) string { +func formatSuccessMessageWithTTY(message string, ttyCheck func() bool, environ []string) string { + return applyStdoutStyleWithTTY(styles.Success, "✓ ", ttyCheck, environ) + message +} + +func formatSuccessMessageStderrWithTTY(message string, ttyCheck func() bool) string { return applyStyleWithTTY(styles.Success, "✓ ", ttyCheck) + message } // FormatInfoMessage formats an informational message func FormatInfoMessage(message string) string { - return formatInfoMessageWithTTY(message, isTTY) + return formatInfoMessageWithTTY(message, isTTY, stdoutEnviron()) } // FormatInfoMessageStderr formats an informational message for stderr output. func FormatInfoMessageStderr(message string) string { - return formatInfoMessageWithTTY(message, isStderrTTY) + return formatInfoMessageStderrWithTTY(message, isStderrTTY) +} + +func formatInfoMessageWithTTY(message string, ttyCheck func() bool, environ []string) string { + return applyStdoutStyleWithTTY(styles.Info, "i ", ttyCheck, environ) + message } -func formatInfoMessageWithTTY(message string, ttyCheck func() bool) string { +func formatInfoMessageStderrWithTTY(message string, ttyCheck func() bool) string { return applyStyleWithTTY(styles.Info, "i ", ttyCheck) + message } @@ -206,14 +233,51 @@ func RenderTable(config TableConfig) string { // Use caller-supplied TTY detector when provided (e.g. tty.IsStderrTerminal // for tables written to stderr), otherwise fall back to stdout detection. ttyCheck := isTTY + stdoutOutput := true if config.TTYFunc != nil { ttyCheck = config.TTYFunc + stdoutOutput = false + } + return renderTableWithTTY(config, ttyCheck, stdoutEnviron(), stdoutOutput) +} + +// buildTableStyleFunc returns the lipgloss style function used by RenderTable. +// config supplies the ShowTotal/TotalRow flags; ttyCheck detects terminal output; +// dataRowCount is the number of data rows (excluding any total row). +func buildTableStyleFunc(config TableConfig, ttyCheck func() bool, dataRowCount int) func(int, int) lipgloss.Style { + return func(row, col int) lipgloss.Style { + if !ttyCheck() { + return lipgloss.NewStyle() + } + if row == table.HeaderRow { + return styles.TableHeader.PaddingLeft(1).PaddingRight(1) + } + if config.ShowTotal && len(config.TotalRow) > 0 && row == dataRowCount { + return styles.TableTotal.PaddingLeft(1).PaddingRight(1) + } + if row%2 == 0 { + return styles.TableCell.PaddingLeft(1).PaddingRight(1) + } + return lipgloss.NewStyle(). + Foreground(styles.ColorForeground). + Background(styles.ColorTableAltRow). + PaddingLeft(1). + PaddingRight(1) } +} +func renderTableWithTTY(config TableConfig, ttyCheck func() bool, environ []string, degradeStdout bool) string { var output strings.Builder + titleStyle := applyStyleWithTTY + if degradeStdout { + titleStyle = func(style lipgloss.Style, text string, ttyCheck func() bool) string { + return applyStdoutStyleWithTTY(style, text, ttyCheck, environ) + } + } + if config.Title != "" { - output.WriteString(applyStyle(styles.TableTitle, config.Title)) + output.WriteString(titleStyle(styles.TableTitle, config.Title, ttyCheck)) output.WriteString("\n") } @@ -240,32 +304,11 @@ func RenderTable(config TableConfig) string { output.WriteString(t.String()) output.WriteString("\n") - return output.String() -} - -// buildTableStyleFunc returns the lipgloss style function used by RenderTable. -// config supplies the ShowTotal/TotalRow flags; ttyCheck detects terminal output; -// dataRowCount is the number of data rows (excluding any total row). -func buildTableStyleFunc(config TableConfig, ttyCheck func() bool, dataRowCount int) func(int, int) lipgloss.Style { - return func(row, col int) lipgloss.Style { - if !ttyCheck() { - return lipgloss.NewStyle() - } - if row == table.HeaderRow { - return styles.TableHeader.PaddingLeft(1).PaddingRight(1) - } - if config.ShowTotal && len(config.TotalRow) > 0 && row == dataRowCount { - return styles.TableTotal.PaddingLeft(1).PaddingRight(1) - } - if row%2 == 0 { - return styles.TableCell.PaddingLeft(1).PaddingRight(1) - } - return lipgloss.NewStyle(). - Foreground(styles.ColorForeground). - Background(styles.ColorTableAltRow). - PaddingLeft(1). - PaddingRight(1) + if degradeStdout && ttyCheck() { + return colorwriter.Degrade(output.String(), environ) } + + return output.String() } // FormatCommandMessage formats a command execution message @@ -290,15 +333,19 @@ func FormatVerboseMessage(message string) string { // FormatListItem formats an item in a list func FormatListItem(item string) string { - return formatListItemWithTTY(item, isTTY) + return formatListItemWithTTY(item, isTTY, stdoutEnviron()) } // FormatListItemStderr formats a list item for stderr output. func FormatListItemStderr(item string) string { - return formatListItemWithTTY(item, isStderrTTY) + return formatListItemStderrWithTTY(item, isStderrTTY) } -func formatListItemWithTTY(item string, ttyCheck func() bool) string { +func formatListItemWithTTY(item string, ttyCheck func() bool, environ []string) string { + return applyStdoutStyleWithTTY(styles.ListItem, " • "+item, ttyCheck, environ) +} + +func formatListItemStderrWithTTY(item string, ttyCheck func() bool) string { return applyStyleWithTTY(styles.ListItem, " • "+item, ttyCheck) } @@ -398,15 +445,19 @@ func formatMultilineError(msg string) string { // FormatSectionHeader formats a section header with proper styling func FormatSectionHeader(header string) string { - return formatSectionHeaderWithTTY(header, isTTY) + return formatSectionHeaderWithTTY(header, isTTY, stdoutEnviron()) } // FormatSectionHeaderStderr formats a section header for stderr output. func FormatSectionHeaderStderr(header string) string { - return formatSectionHeaderWithTTY(header, isStderrTTY) + return formatSectionHeaderStderrWithTTY(header, isStderrTTY) +} + +func formatSectionHeaderWithTTY(header string, ttyCheck func() bool, environ []string) string { + return applyStdoutStyleWithTTY(styles.Header, header, ttyCheck, environ) } -func formatSectionHeaderWithTTY(header string, ttyCheck func() bool) string { +func formatSectionHeaderStderrWithTTY(header string, ttyCheck func() bool) string { return applyStyleWithTTY(styles.Header, header, ttyCheck) } diff --git a/pkg/console/console_test.go b/pkg/console/console_test.go index f21c7b522bc..8417fe0f0db 100644 --- a/pkg/console/console_test.go +++ b/pkg/console/console_test.go @@ -191,16 +191,16 @@ func TestFormatWarningMessage(t *testing.T) { func TestFormatHelpersWithTTYCheck(t *testing.T) { t.Run("plain output when tty check is false", func(t *testing.T) { - if got := formatInfoMessageWithTTY("processing file", func() bool { return false }); got != "i processing file" { + if got := formatInfoMessageWithTTY("processing file", func() bool { return false }, nil); got != "i processing file" { t.Fatalf("expected unstyled info message, got %q", got) } - if got := formatSuccessMessageWithTTY("done", func() bool { return false }); got != "✓ done" { + if got := formatSuccessMessageWithTTY("done", func() bool { return false }, nil); got != "✓ done" { t.Fatalf("expected unstyled success message, got %q", got) } - if got := formatListItemWithTTY("item", func() bool { return false }); got != " • item" { + if got := formatListItemWithTTY("item", func() bool { return false }, nil); got != " • item" { t.Fatalf("expected unstyled list item, got %q", got) } - if got := formatSectionHeaderWithTTY("Header", func() bool { return false }); got != "Header" { + if got := formatSectionHeaderWithTTY("Header", func() bool { return false }, nil); got != "Header" { t.Fatalf("expected unstyled header, got %q", got) } }) @@ -210,11 +210,29 @@ func TestFormatHelpersWithTTYCheck(t *testing.T) { _ = formatInfoMessageWithTTY("x", func() bool { calls++ return false - }) + }, nil) if calls != 1 { t.Fatalf("expected tty check to be called once, got %d", calls) } }) + + t.Run("stdout helpers strip ansi when NO_COLOR is set", func(t *testing.T) { + environ := []string{"NO_COLOR=1", "TERM=xterm-256color"} + if got := formatInfoMessageWithTTY("processing file", func() bool { return true }, environ); got != "i processing file" { + t.Fatalf("expected info message without ANSI, got %q", got) + } + if got := formatSuccessMessageWithTTY("done", func() bool { return true }, environ); got != "✓ done" { + t.Fatalf("expected success message without ANSI, got %q", got) + } + if got := formatListItemWithTTY("item", func() bool { return true }, environ); got != " • item" { + t.Fatalf("expected list item without ANSI, got %q", got) + } + if got := formatSectionHeaderWithTTY("Header", func() bool { return true }, environ); strings.Contains(got, "\x1b[") { + t.Fatalf("expected section header without ANSI, got %q", got) + } else if !strings.Contains(got, "Header") { + t.Fatalf("expected section header text to be preserved, got %q", got) + } + }) } func TestRenderTable(t *testing.T) { @@ -296,6 +314,23 @@ func TestRenderTable(t *testing.T) { } } +func TestRenderTableWithTTY_StripsAnsiWhenNoColorIsSet(t *testing.T) { + output := renderTableWithTTY(TableConfig{ + Title: "Workflow Results", + Headers: []string{"Name", "Status"}, + Rows: [][]string{{"build", "success"}}, + }, func() bool { return true }, []string{"NO_COLOR=1", "TERM=xterm-256color"}, true) + + if strings.Contains(output, "\x1b[") { + t.Fatalf("expected NO_COLOR table output without ANSI escapes, got %q", output) + } + for _, want := range []string{"Workflow Results", "Name", "Status", "build", "success"} { + if !strings.Contains(output, want) { + t.Fatalf("expected output to contain %q, got %q", want, output) + } + } +} + func TestToRelativePath(t *testing.T) { tests := []struct { name string diff --git a/pkg/styles/huh_theme.go b/pkg/styles/huh_theme.go index 0dd8ad153e5..0bd4ee92f6b 100644 --- a/pkg/styles/huh_theme.go +++ b/pkg/styles/huh_theme.go @@ -4,7 +4,7 @@ package styles import ( "charm.land/huh/v2" - "charm.land/lipgloss/v2" + lipgloss "charm.land/lipgloss/v2" ) // HuhTheme is a huh.ThemeFunc that maps the pkg/styles Dracula-inspired @@ -15,15 +15,17 @@ var HuhTheme huh.ThemeFunc = func(isDark bool) *huh.Styles { lightDark := lipgloss.LightDark(isDark) // Map the pkg/styles palette using lipgloss v2's LightDark helper. + // The color* vars are the same package-level color.Color values used by + // the adaptiveColor vars in theme.go, ensuring a single source of truth. var ( - primary = lightDark(lipgloss.Color(hexColorPurpleLight), lipgloss.Color(hexColorPurpleDark)) - success = lightDark(lipgloss.Color(hexColorSuccessLight), lipgloss.Color(hexColorSuccessDark)) - errorColor = lightDark(lipgloss.Color(hexColorErrorLight), lipgloss.Color(hexColorErrorDark)) - warning = lightDark(lipgloss.Color(hexColorWarningLight), lipgloss.Color(hexColorWarningDark)) - comment = lightDark(lipgloss.Color(hexColorCommentLight), lipgloss.Color(hexColorCommentDark)) - fg = lightDark(lipgloss.Color(hexColorForegroundLight), lipgloss.Color(hexColorForegroundDark)) - bg = lightDark(lipgloss.Color(hexColorBackgroundLight), lipgloss.Color(hexColorBackgroundDark)) - border = lightDark(lipgloss.Color(hexColorBorderLight), lipgloss.Color(hexColorBorderDark)) + primary = lightDark(colorPurpleLight, colorPurpleDark) + success = lightDark(colorSuccessLight, colorSuccessDark) + errorColor = lightDark(colorErrorLight, colorErrorDark) + warning = lightDark(colorWarningLight, colorWarningDark) + comment = lightDark(colorCommentLight, colorCommentDark) + fg = lightDark(colorForegroundLight, colorForegroundDark) + bg = lightDark(colorBackgroundLight, colorBackgroundDark) + border = lightDark(colorBorderLight, colorBorderDark) ) // Focused field styles diff --git a/pkg/styles/theme.go b/pkg/styles/theme.go index 875c3321a32..a9da6aca520 100644 --- a/pkg/styles/theme.go +++ b/pkg/styles/theme.go @@ -107,7 +107,7 @@ func init() { } // Hex color constants for light and dark variants. -// These are used both to build the AdaptiveColor values at runtime and to +// These are used both to build the color.Color values at runtime and to // enable straightforward assertions in tests (same package). const ( hexColorErrorLight = "#D73737" @@ -134,74 +134,105 @@ const ( hexColorTableAltRowDark = "#1A1A1A" ) +// Package-level color.Color values parsed once from the hex constants above. +// Both the adaptiveColor vars below (startup-probe path) and huh_theme.go +// (per-render LightDark path) reference these, so the hex parsing happens +// only once and there is a single source of truth for the palette. +var ( + colorErrorLight = lipgloss.Color(hexColorErrorLight) + colorErrorDark = lipgloss.Color(hexColorErrorDark) + colorWarningLight = lipgloss.Color(hexColorWarningLight) + colorWarningDark = lipgloss.Color(hexColorWarningDark) + colorSuccessLight = lipgloss.Color(hexColorSuccessLight) + colorSuccessDark = lipgloss.Color(hexColorSuccessDark) + colorInfoLight = lipgloss.Color(hexColorInfoLight) + colorInfoDark = lipgloss.Color(hexColorInfoDark) + colorPurpleLight = lipgloss.Color(hexColorPurpleLight) + colorPurpleDark = lipgloss.Color(hexColorPurpleDark) + colorYellowLight = lipgloss.Color(hexColorYellowLight) + colorYellowDark = lipgloss.Color(hexColorYellowDark) + colorCommentLight = lipgloss.Color(hexColorCommentLight) + colorCommentDark = lipgloss.Color(hexColorCommentDark) + colorForegroundLight = lipgloss.Color(hexColorForegroundLight) + colorForegroundDark = lipgloss.Color(hexColorForegroundDark) + colorBackgroundLight = lipgloss.Color(hexColorBackgroundLight) + colorBackgroundDark = lipgloss.Color(hexColorBackgroundDark) + colorBorderLight = lipgloss.Color(hexColorBorderLight) + colorBorderDark = lipgloss.Color(hexColorBorderDark) + colorTableAltRowLight = lipgloss.Color(hexColorTableAltRowLight) + colorTableAltRowDark = lipgloss.Color(hexColorTableAltRowDark) +) + // Adaptive colors that work well in both light and dark terminal themes. // Light variants use darker, more saturated colors for visibility on light backgrounds. // Dark variants use brighter colors (Dracula theme inspired) for dark backgrounds. +// Note: huh_theme.go uses the same color* vars above via lipgloss.LightDark for +// per-render selection; both paths share the same palette constants. var ( // ColorError is used for error messages and critical issues. ColorError = adaptiveColor{ - Light: lipgloss.Color(hexColorErrorLight), // Darker red for light backgrounds - Dark: lipgloss.Color(hexColorErrorDark), // Bright red for dark backgrounds (Dracula) + Light: colorErrorLight, // Darker red for light backgrounds + Dark: colorErrorDark, // Bright red for dark backgrounds (Dracula) } // ColorWarning is used for warning messages and cautionary information. ColorWarning = adaptiveColor{ - Light: lipgloss.Color(hexColorWarningLight), // Darker orange for light backgrounds - Dark: lipgloss.Color(hexColorWarningDark), // Bright orange for dark backgrounds (Dracula) + Light: colorWarningLight, // Darker orange for light backgrounds + Dark: colorWarningDark, // Bright orange for dark backgrounds (Dracula) } // ColorSuccess is used for success messages and confirmations. ColorSuccess = adaptiveColor{ - Light: lipgloss.Color(hexColorSuccessLight), // Darker green for light backgrounds - Dark: lipgloss.Color(hexColorSuccessDark), // Bright green for dark backgrounds (Dracula) + Light: colorSuccessLight, // Darker green for light backgrounds + Dark: colorSuccessDark, // Bright green for dark backgrounds (Dracula) } // ColorInfo is used for informational messages ColorInfo = adaptiveColor{ - Light: lipgloss.Color(hexColorInfoLight), // Darker cyan/blue for light backgrounds - Dark: lipgloss.Color(hexColorInfoDark), // Bright cyan for dark backgrounds (Dracula) + Light: colorInfoLight, // Darker cyan/blue for light backgrounds + Dark: colorInfoDark, // Bright cyan for dark backgrounds (Dracula) } // ColorPurple is used for file paths, commands, and highlights ColorPurple = adaptiveColor{ - Light: lipgloss.Color(hexColorPurpleLight), // Darker purple for light backgrounds - Dark: lipgloss.Color(hexColorPurpleDark), // Bright purple for dark backgrounds (Dracula) + Light: colorPurpleLight, // Darker purple for light backgrounds + Dark: colorPurpleDark, // Bright purple for dark backgrounds (Dracula) } // ColorYellow is used for progress messages and attention-grabbing content ColorYellow = adaptiveColor{ - Light: lipgloss.Color(hexColorYellowLight), // Darker yellow/gold for light backgrounds - Dark: lipgloss.Color(hexColorYellowDark), // Bright yellow for dark backgrounds (Dracula) + Light: colorYellowLight, // Darker yellow/gold for light backgrounds + Dark: colorYellowDark, // Bright yellow for dark backgrounds (Dracula) } // ColorComment is used for secondary/muted information like line numbers ColorComment = adaptiveColor{ - Light: lipgloss.Color(hexColorCommentLight), // Muted gray-blue for light backgrounds - Dark: lipgloss.Color(hexColorCommentDark), // Muted purple-gray for dark backgrounds (Dracula) + Light: colorCommentLight, // Muted gray-blue for light backgrounds + Dark: colorCommentDark, // Muted purple-gray for dark backgrounds (Dracula) } // ColorForeground is used for primary text content ColorForeground = adaptiveColor{ - Light: lipgloss.Color(hexColorForegroundLight), // Dark gray for light backgrounds - Dark: lipgloss.Color(hexColorForegroundDark), // Light gray/white for dark backgrounds (Dracula) + Light: colorForegroundLight, // Dark gray for light backgrounds + Dark: colorForegroundDark, // Light gray/white for dark backgrounds (Dracula) } // ColorBackground is used for highlighted backgrounds ColorBackground = adaptiveColor{ - Light: lipgloss.Color(hexColorBackgroundLight), // Light gray for light backgrounds - Dark: lipgloss.Color(hexColorBackgroundDark), // Dark purple/gray for dark backgrounds (Dracula) + Light: colorBackgroundLight, // Light gray for light backgrounds + Dark: colorBackgroundDark, // Dark purple/gray for dark backgrounds (Dracula) } // ColorBorder is used for table borders and dividers ColorBorder = adaptiveColor{ - Light: lipgloss.Color(hexColorBorderLight), // Light gray border for light backgrounds - Dark: lipgloss.Color(hexColorBorderDark), // Dark purple border for dark backgrounds (Dracula) + Light: colorBorderLight, // Light gray border for light backgrounds + Dark: colorBorderDark, // Dark purple border for dark backgrounds (Dracula) } // ColorTableAltRow is used for alternating row backgrounds in tables (zebra striping) ColorTableAltRow = adaptiveColor{ - Light: lipgloss.Color(hexColorTableAltRowLight), // Subtle light gray for light backgrounds - Dark: lipgloss.Color(hexColorTableAltRowDark), // Subtle darker background for dark backgrounds + Light: colorTableAltRowLight, // Subtle light gray for light backgrounds + Dark: colorTableAltRowDark, // Subtle darker background for dark backgrounds } )