-
Notifications
You must be signed in to change notification settings - Fork 445
Honor NO_COLOR/color-profile degradation for Lip Gloss v2 output paths #42945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+205
−56
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
5b5c803
Initial plan
Copilot 40e400c
Honor NO_COLOR via colorprofile writers
Copilot 51915fa
docs(adr): add draft ADR-42945 for colorprofile.Writer routing
github-actions[bot] 91dde54
plan: mirror console build-tag pattern in pkg/logger
Copilot a19d6b7
refactor(logger): split colorprofile writer into build-tagged files
Copilot 910ba45
Merge branch 'main' into copilot/go-fan-review-lipgloss-v2
github-actions[bot] 38d3cf0
chore: plan spinner writer sharing, logger bounds guard, duplicate te…
Copilot d29af84
fix(console/logger): share single colorprofile writer in spinner, add…
Copilot 1b7995f
Merge remote-tracking branch 'origin/main' into copilot/go-fan-review…
Copilot 38c7ff0
fix(logger): remove panic from buildColorPalette to satisfy panicinli…
Copilot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
44 changes: 44 additions & 0 deletions
44
docs/adr/42945-route-lipgloss-v2-output-through-colorprofile-writer.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.* |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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()) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertion can pass vacuously if
colorprofile.Writeremits nothing — the test never verifies the payload was actually forwarded, only that no ANSI escapes are present.💡 Suggested fix
If a future version of
colorprofile.NewWriterdrops unrecognised sequences instead of passing them through as plain text,buf.String()would be""— still ANSI-free, so the test passes while the real behavior has silently regressed.Add a non-empty check before the ANSI assertion:
The same gap exists in
pkg/logger/logger_test.goTestColorProfileWriterStripsANSIWithNoColor.