Honor NO_COLOR/color-profile degradation for Lip Gloss v2 output paths#42945
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
Routes stderr output that may include Lip Gloss v2 styling through colorprofile.Writer so NO_COLOR and terminal color-capability degradation are honored consistently, plus a few small style/theme cleanups and targeted regression tests.
Changes:
- Added a
stderrWriter()abstraction (and wasm passthrough) and updated multiple console stderr output paths (banner, verbose, list/confirm prompts, terminal helpers, composed sections, spinner output) to write through it. - Updated logger output to write through a colorprofile-wrapped stderr writer, and refactored the namespace color palette construction to reduce duplication.
- Added focused tests asserting ANSI is stripped when
NO_COLOR=1for the new writer path (console + logger), and made a small Lip Gloss style consistency adjustment.
Show a summary per file
| File | Description |
|---|---|
| pkg/styles/theme.go | Adds guidance comment clarifying when to use package-level adaptive colors vs lipgloss.LightDark(...). |
| pkg/logger/logger.go | Refactors namespace color palette creation and routes logger stderr writes through a colorprofile-wrapped writer. |
| pkg/logger/logger_test.go | Adds a regression test ensuring the colorprofile writer strips ANSI when NO_COLOR=1. |
| pkg/console/verbose.go | Routes verbose stderr output through stderrWriter(). |
| pkg/console/terminal.go | Routes terminal/banner-related stderr writes through stderrWriter(). |
| pkg/console/spinner.go | Switches spinner output to an io.Writer and routes output through stderrWriter(). |
| pkg/console/list.go | Routes non-interactive list rendering stderr output through stderrWriter(). |
| pkg/console/console.go | Routes composed section rendering stderr output through stderrWriter(). |
| pkg/console/confirm.go | Routes non-interactive confirm prompt stderr output through stderrWriter(). |
| pkg/console/colorprofile_writer.go | Introduces non-wasm stderrWriter() backed by colorprofile.NewWriter. |
| pkg/console/colorprofile_writer_wasm.go | Adds wasm/js stub that bypasses colorprofile and returns the underlying writer/stderr. |
| pkg/console/colorprofile_writer_test.go | Adds regression test for ANSI stripping with NO_COLOR=1 on the console writer. |
| pkg/console/banner.go | Routes banner printing to stderr through stderrWriter(). |
| pkg/cli/compile_schedule_calendar.go | Replaces lipgloss.Style{} with lipgloss.NewStyle() for consistency in non-terminal output. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 14/14 changed files
- Comments generated: 1
- Review effort level: Low
| lipgloss "charm.land/lipgloss/v2" | ||
| "github.com/charmbracelet/colorprofile" | ||
| "github.com/github/gh-aw/pkg/styles" | ||
| "github.com/github/gh-aw/pkg/timeutil" | ||
| ) |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (2 tests)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (147 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
REQUEST_CHANGES — The colorprofile integration is directionally correct, but three issues need addressing before merge.
### Blocking issues
-
Spinner: two separate
stderrWriter()calls at construction (spinner.golines 113 + 119) —spinnerModel.outputandtea.WithOutput(...)receive independentcolorprofile.Writerinstances. Direct writes (line 87) and tea-program output pass through different profile detections. Fix: capture one writer and share it. -
pkg/loggermissing wasm build-tag stub (logger.goline 65) —pkg/consolecorrectly gates colorprofile behind!js && !wasm. The logger duplicates the same functions with no build tag, so wasm builds of pkg/logger unconditionally callcolorprofile.NewWriter. This diverges from the console package's explicit passthrough-on-wasm decision and couples the logger to an inconsistent platform behavior.
### Non-blocking but worth fixing
stderrWriter()/os.Environ()called on every write inStop/StopWithMessage— profile should be captured once at spinner construction.buildColorPaletteunchecked index slice — silent panic risk atinit()ifbasePaletteColorsshrinks.colorprofile_writer_test.goassertion passes vacuously if writer drops bytes — add a non-empty guard.
🔎 Code quality review by PR Code Quality Reviewer · 73.5 AIC · ⌖ 14.9 AIC · ⊞ 1.6K
Comment /review to run again
| // 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(stderrWriter()), tea.WithoutRenderer(), tea.WithInput(nil)) |
There was a problem hiding this comment.
Two independent colorprofile.Writer instances wrap the same os.Stderr fd — model.output and tea.WithOutput(...) use separate writers, giving them independent profile detections and potentially inconsistent ANSI handling.
💡 Details and fix
NewSpinner calls stderrWriter() twice:
output: stderrWriter(), // line 113 — used in spinnerModel.Update direct writes (line 87)
...
tea.WithOutput(stderrWriter()), // line 119 — used by tea program rendererAt line 87, the model writes directly via m.output:
fmt.Fprintf(m.output, "%s%s%s %s", ansiCarriageReturn, ...)These two colorprofile.Writer instances each call os.Environ() separately. If the environment could vary between the calls (e.g. in tests that manipulate env vars), the spinner renderer and the direct terminal-control writes operate under different profiles.
Fix: capture one writer and pass it to both:
out := stderrWriter()
model := spinnerModel{
...
output: out,
}
s.program = tea.NewProgram(model, tea.WithOutput(out), tea.WithoutRenderer(), tea.WithInput(nil))| s.program.Quit() | ||
| s.wg.Wait() // Wait for the goroutine to complete | ||
| fmt.Fprintf(os.Stderr, "%s%s", ansiCarriageReturn, ansiClearLine) | ||
| fmt.Fprintf(stderrWriter(), "%s%s", ansiCarriageReturn, ansiClearLine) |
There was a problem hiding this comment.
stderrWriter() / os.Environ() called on every write in Stop and StopWithMessage — four fresh allocations per spinner lifecycle, none sharing a writer with the model constructed in NewSpinner.
💡 Details and fix
Lines 173, 192, 195, and 199 each call stderrWriter() independently. Every invocation calls os.Environ(), which copies the entire process environment as a []string. The profile is constant for the process lifetime; there is no reason to re-detect it on each write.
More critically, the writer used in Stop/StopWithMessage is a different instance from spinnerModel.output (set at line 113). Any profile state accumulated while the spinner was rendering is invisible to these stop-phase writes, meaning the profile can theoretically differ if env vars change between spinner construction and termination (common in tests).
Fix: store the captured writer in SpinnerWrapper and reuse it in Stop/StopWithMessage:
type SpinnerWrapper struct {
...
out io.Writer // captured once in NewSpinner via stderrWriter()
}
// In NewSpinner:
out := stderrWriter()
s.out = out
model := spinnerModel{..., output: out}
s.program = tea.NewProgram(model, tea.WithOutput(out), ...)
// In Stop:
fmt.Fprintf(s.out, "%s%s", ansiCarriageReturn, ansiClearLine)| return colorprofile.NewWriter(w, environ) | ||
| } | ||
|
|
||
| func stderrWriter() io.Writer { |
There was a problem hiding this comment.
pkg/logger duplicates stderrWriter/newColorProfileWriter without a wasm build-tag stub — unlike pkg/console, the logger always routes through colorprofile.Writer on all platforms, including wasm/js.
💡 Details
pkg/console correctly splits the implementation across two build-tag-gated files:
colorprofile_writer.go—//go:build !js && !wasm— real colorprofilecolorprofile_writer_wasm.go—//go:build js || wasm— passthrough
pkg/logger/logger.go has no build tag and imports colorprofile unconditionally. On wasm/js builds the logger will call colorprofile.NewWriter(os.Stderr, os.Environ()). Since WASM stderr maps to the browser console (which does not interpret ANSI), the stripping logic is both unnecessary and a divergence from the console package's explicit passthrough decision.
Additionally, maintaining two copies means any future change to profile-writer construction (options, library version, caching) must be made in both places.
Fix: extract a shared internal package (e.g. pkg/internal/profilewriter) with proper wasm/non-wasm stubs, and have both pkg/console and pkg/logger import from it.
| ) | ||
|
|
||
| func buildColorPalette() []lipgloss.Style { | ||
| order := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 3} |
There was a problem hiding this comment.
buildColorPalette indexes basePaletteColors with no bounds check — removing or reordering any entry in basePaletteColors causes a silent index-out-of-range panic at package init.
💡 Details and fix
order := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 3} // max index = 8basePaletteColors currently has exactly 9 entries (indices 0–8), so this works today. But the invariant is entirely implicit — a future maintainer adding or removing a color will produce a cryptic runtime: index out of range panic during init() with no indication of where to look.
Simplest fix: add an init-time guard:
for _, idx := range order {
if idx >= len(basePaletteColors) {
panic(fmt.Sprintf("logger: buildColorPalette index %d out of range (len=%d)", idx, len(basePaletteColors)))
}
palette = append(palette, lipgloss.NewStyle().Foreground(basePaletteColors[idx]))
}Better fix: add a unit test TestPaletteIndicesInRange that iterates order and calls t.Fatalf if any index ≥ len(basePaletteColors). This catches the violation at test time rather than at runtime.
| } | ||
|
|
||
| if strings.Contains(buf.String(), "\x1b[") { | ||
| t.Fatalf("expected ANSI-free output with NO_COLOR, got %q", buf.String()) |
There was a problem hiding this comment.
The assertion can pass vacuously if colorprofile.Writer emits 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.NewWriter drops 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:
if buf.Len() == 0 {
t.Fatal("expected non-empty output; writer appears to have dropped all bytes")
}
if strings.Contains(buf.String(), "\x1b[") {
t.Fatalf("expected ANSI-free output with NO_COLOR, got %q", buf.String())
}The same gap exists in pkg/logger/logger_test.go TestColorProfileWriterStripsANSIWithNoColor.
There was a problem hiding this comment.
Review: Honor NO_COLOR/color-profile degradation for Lip Gloss v2
The overall direction is sound and well-structured. Routing styled output through colorprofile.Writer is the right fix for Lip Gloss v2 losing auto-detection. The console-level build-tag split (native vs. wasm passthrough) is a good pattern.
Blocking issues:
-
pkg/loggerlacks the wasm build split (see inline comment onlogger.go:65).pkg/consolecorrectly uses//go:build !js && !wasmto gate thecolorprofileimport;pkg/loggeradds the same helpers but unconditionally importscolorprofile. If logger is compiled for a wasm target this will break. Needs the same file-pair split. -
Spinner creates two independent
colorprofile.Writerinstances (inline onspinner.go:119).model.outputandtea.WithOutputeach callstderrWriter()separately. Capture a single writer inNewSpinnerand pass it to both, then store it onSpinnerWrapperfor use inStop/StopWithMessageinstead of callingstderrWriter()again after quit.
Non-blocking:
buildColorPaletteindex slice (inline onlogger.go:53) — the hardcodedorderindices will silently panic ifbasePaletteColorsis ever trimmed. A startup bounds check would make the invariant explicit.
What looks good
- Build-tag split in
pkg/console(native + wasm stub) is clean and correct. - Replacing
lipgloss.Style{}withlipgloss.NewStyle()in the calendar file is a correct consistency fix. - The
adaptiveColorvslipgloss.LightDarkcomment intheme.gois useful guidance. NO_COLORtests in bothpkg/consoleandpkg/loggerare well-written.- Palette refactor reduces the duplicate list and is behaviorally equivalent.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 69.2 AIC · ⌖ 8.13 AIC · ⊞ 4.9K
| return colorprofile.NewWriter(w, environ) | ||
| } | ||
|
|
||
| func stderrWriter() io.Writer { |
There was a problem hiding this comment.
Missing wasm build split — pkg/console correctly isolates the colorprofile import behind a //go:build !js && !wasm guard, with a passthrough stub in colorprofile_writer_wasm.go. This file adds the same newColorProfileWriter/stderrWriter helpers but with an unconditional colorprofile import and no wasm build tag.
If pkg/logger is compiled for wasm/js targets this will either fail to build or pull in colorprofile unexpectedly. Apply the same split here:
// logger_colorprofile.go (go/redacted):build !js && !wasm
func stderrWriter() io.Writer { return colorprofile.NewWriter(os.Stderr, os.Environ()) }
// logger_colorprofile_wasm.go (go/redacted):build js || wasm
func stderrWriter() io.Writer { return os.Stderr }@copilot please address this.
| // 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(stderrWriter()), tea.WithoutRenderer(), tea.WithInput(nil)) |
There was a problem hiding this comment.
Two separate colorprofile.Writer instances for the same logical output — model.output (line 113) and the tea.WithOutput writer (line 119) are created by two separate stderrWriter() calls, producing two independent buffered colorprofile.Writer wrappers around os.Stderr.
Both ultimately write to the same os.Stderr file descriptor, so output is not lost. However interleaved writes from two separate colorprofile.Writer wrappers could in principle produce inconsistent profile negotiation or redundant reset sequences.
Consider capturing a single writer and passing it to both:
output := stderrWriter()
model := spinnerModel{..., output: output}
s.program = tea.NewProgram(model, tea.WithOutput(output), ...)@copilot please address this.
| s.program.Quit() | ||
| s.wg.Wait() // Wait for the goroutine to complete | ||
| fmt.Fprintf(os.Stderr, "%s%s", ansiCarriageReturn, ansiClearLine) | ||
| fmt.Fprintf(stderrWriter(), "%s%s", ansiCarriageReturn, ansiClearLine) |
There was a problem hiding this comment.
Fresh stderrWriter() calls after program.Quit() / wg.Wait() create a new colorprofile.Writer — the ANSI clear-line sequences at lines 173, 192, and 195 are written through freshly allocated writers that have no context about what the Bubble Tea program already rendered. This is harmless for plain-text sequences like \r / clear-line, but if colorprofile.Writer ever buffers profile state, the teardown writes will use a fresh negotiation rather than the one the spinner program used.
The same single output captured in NewSpinner should be plumbed into SpinnerWrapper and reused here, rather than calling stderrWriter() again at stop time.
@copilot please address this.
| ) | ||
|
|
||
| func buildColorPalette() []lipgloss.Style { | ||
| order := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 3} |
There was a problem hiding this comment.
buildColorPalette index hard-codes out-of-sync with basePaletteColors length — basePaletteColors has 9 entries (indices 0–8). The order slice uses index 8 (valid) but if basePaletteColors is ever shortened the out-of-bounds access will panic at startup. Consider adding a bounds guard or using a test/assertion:
for _, idx := range order {
if idx >= len(basePaletteColors) {
panic(fmt.Sprintf("buildColorPalette: index %d out of range (%d colors)", idx, len(basePaletteColors)))
}
...
}(Minor, but the refactor's goal is to reduce drift risk — an explicit guard makes the contract visible.)
@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a few concrete issues before merging.
📋 Key Themes & Highlights
Key Themes
- Spinner writer fragmentation —
NewSpinnerallocates two independentcolorprofile.Writerinstances andStop/StopWithMessagecreate fresh ones at call time. All four should share a single instance stored on the struct. - Duplicated
stderrWriter/newColorProfileWriter— the same pair of functions exists in bothpkg/consoleandpkg/logger. A shared export (or a thinpkg/colorprofilepackage) would prevent drift. - Test coverage gaps — only the
NO_COLORhard-strip case is tested; the positive (color preserved) and downgrade (truecolor → 256-color) cases are missing. The identical test inlogger_test.goshould be removed once the duplication is resolved. - Magic index slice —
order := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 3}inbuildColorPaletteis opaque; named indices would make the intent self-documenting. - Interactive huh path — it is worth confirming whether the huh-driven TTY forms also need a profile-writer wrapper; the PR only addresses the non-interactive / plain-text output paths.
Positive Highlights
- ✅ Clean platform split via build tags (
!js && !wasmvsjs || wasm) with sensible pass-through stub - ✅
lipgloss.Style{}→lipgloss.NewStyle()is the right idiomatic fix for Lip Gloss v2 - ✅ Focused comment in
theme.goclarifyingadaptiveColorvslipgloss.LightDarkis genuinely useful - ✅ Logger palette refactor removes the duplicated literal entries and reduces future drift risk
- ✅ Regression test is well-structured and covers the primary reported symptom
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 91.7 AIC · ⌖ 9.19 AIC · ⊞ 6.6K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/console/spinner.go:119
[/diagnosing-bugs] NewSpinner calls stderrWriter() twice (lines 113 and 119), producing two separate colorprofile.Writer instances wrapping the same os.Stderr. If the two writers detect terminal capabilities independently, any state-dependent profiling could diverge mid-spinner.
<details>
<summary>💡 Suggestion</summary>
Capture the writer once and reuse it:
outputWriter := stderrWriter()
model := spinnerModel{
...
output: outputWriter,
}
s.program = tea.NewProgram(…
</details>
<details><summary>pkg/console/spinner.go:173</summary>
**[/diagnosing-bugs]** `Stop()` and `StopWithMessage()` call `stderrWriter()` at the time of invocation, creating fresh `colorprofile.Writer` instances that are different from the one the spinner program was configured with (line 119). This means the terminal-clearing escape codes (`\r\033[K`) could be emitted by a different writer than the one used to render the spinner progress lines, potentially defeating the ANSI-stripping behavior under `NO_COLOR`.
<details>
<summary>💡 Suggestion</summar…
</details>
<details><summary>pkg/console/colorprofile_writer_test.go:20</summary>
**[/tdd]** The test verifies that ANSI escape codes are stripped with `NO_COLOR=1`, but does not test the positive case — that color codes are **preserved** when `NO_COLOR` is absent and the profile supports color. Adding a complementary assertion would make the test read as a full specification rather than just a guard.
<details>
<summary>💡 Suggested addition</summary>
```go
func TestColorProfileWriterPreservesANSIWithColor(t *testing.T) {
var buf bytes.Buffer
w := newColorProfileWr…
</details>
<details><summary>pkg/logger/logger.go:65</summary>
**[/diagnosing-bugs]** `logger.go` defines its own `stderrWriter()` / `newColorProfileWriter()` that is an exact duplicate of the same pair in `pkg/console/colorprofile_writer.go`. Two independent copies of the same wrapping logic mean future drift (e.g. adding a `FORCE_COLOR` override) would require changes in two places.
<details>
<summary>💡 Suggestion</summary>
Export a shared constructor from `pkg/console` (or a dedicated `pkg/colorprofile` package) and import it here:
```go
// pkg/cons…
</details>
<details><summary>pkg/logger/logger.go:53</summary>
**[/diagnosing-bugs]** The `order` slice `{0, 1, 2, 3, 4, 5, 6, 7, 8, 0, 1, 3}` is magic — the choice of 12 entries, and specifically repeating indices 0 (Info), 1 (Success), and 3 (Purple) but not others, requires a reader to cross-reference `basePaletteColors` to understand. This is an improvement over duplicated literal structs, but the intent is still opaque.
<details>
<summary>💡 Suggestion</summary>
Either add an inline comment naming what each repeat represents, or use named constants:…
</details>
<details><summary>pkg/logger/logger_test.go:341</summary>
**[/tdd]** `TestColorProfileWriterStripsANSIWithNoColor` in the logger package is functionally identical to the same test in `pkg/console/colorprofile_writer_test.go`. Since the duplication follows from the duplicated `newColorProfileWriter` implementation (see related comment on logger.go:65), if that duplication is resolved the duplicate test should be removed as well — otherwise passing tests in one package can mask a failing implementation in the other.
@copilot please address this.
</details>
<details><summary>pkg/console/colorprofile_writer_test.go:12</summary>
**[/tdd]** There is no test for the `ANSI_COLORS_DISABLED=1` path (another `NO_COLOR`-equivalent that `colorprofile` honours), nor for the color-profile downgrade case (e.g. truecolor → 256-color when `COLORTERM` is absent). The test only exercises the hard strip case. Covering at least one downgrade scenario would give confidence that the profiler is wired correctly, not just that it can disable output.
<details>
<summary>💡 Example: 256-color downgrade</summary>
```go
func TestColorProfileW…
</details>
<details><summary>pkg/console/colorprofile_writer.go:16</summary>
**[/codebase-design]** `stderrWriter()` is a package-private helper that calls `os.Environ()` and constructs a new `colorprofile.Writer` on every call. A call site like `RenderComposedSections` (console.go) captures `out := stderrWriter()` once per function, which is fine, but sites like `spinner.go` call it 3+ times for the same spinner lifetime. The repeated environment reads and writer allocations are cheap on most paths, but there is no way to inject an alternative writer in tests — the fun…
</details>
<details><summary>pkg/console/confirm.go:63</summary>
**[/diagnosing-bugs]** `showTextConfirm` now routes styled output through `stderrWriter()` which strips ANSI, but the prompt text ("Enter y/yes/1 to confirm...") is plain text with no ANSI anyway. The real regression risk is that `showInteractiveConfirm` (the TTY path, which drives huh forms) is not touched by this PR — if huh renders styled output directly to its own `os.Stderr` reference without going through the profile writer, the fix is incomplete for interactive mode. Worth confirming whe…
</details>
🤖 PR Triage
Score breakdown: Impact 38 · Urgency 20 · Quality 10 CI blocked; 3× CHANGES_REQUESTED from automated reviews. Modifies 14 files across
|
|
@copilot please run the
Run: https://github.com/github/gh-aw/actions/runs/28592810254
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Mirror the pkg/console pattern: move newColorProfileWriter/stderrWriter into colorprofile_writer.go (!js && !wasm) and colorprofile_writer_wasm.go (js || wasm) so the colorprofile import is excluded from js/wasm builds. Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. Mirrored the
|
|
@copilot please run the
|
…st removal Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
… bounds guard, remove duplicate test - spinner.go: capture stderrWriter() once in NewSpinner, store on SpinnerWrapper.out, share with spinnerModel.output, tea.WithOutput, and Stop/StopWithMessage calls - logger.go: add explicit panic in buildColorPalette if an order index exceeds len(basePaletteColors), making the invariant visible at startup - logger_test.go: remove TestColorProfileWriterStripsANSIWithNoColor (duplicate of the same test in pkg/console/colorprofile_writer_test.go) Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed all three
|
|
@copilot merge main and recompile |
…-lipgloss-v2 # Conflicts: # .github/workflows/plan.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Merged |
…brarycode linter Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in the latest commit. The |
Lip Gloss v2 no longer auto-detects terminal capabilities, so styled output can emit ANSI even when users request no color and can over-target truecolor on limited terminals. This change routes styled write paths through
colorprofile.Writerand applies the small follow-up cleanups identified in the module review.Output-path colorprofile integration (console + logger)
stderrWriter()wrapper inpkg/consolebacked bycolorprofile.NewWriter(os.Stderr, os.Environ())(with wasm passthrough stub).banner,confirm,list,terminal,verbose, composed section rendering, spinner messages) to use the wrapped writer.lipgloss.Fprintfcalls to use a colorprofile-wrapped stderr writer.Lip Gloss style consistency cleanup
lipgloss.Style{}withlipgloss.NewStyle()incompile_schedule_calendar.gofor consistency with the rest of the codebase.Theme API guidance cleanup
pkg/styles/theme.goclarifying when to use package-leveladaptiveColorvs per-renderlipgloss.LightDark(...).Logger palette maintenance cleanup
Focused regression coverage
NO_COLOR=1via the new colorprofile writer path.