From eff0763d0a7c850d08d8f92763ebf9b194a14b5a Mon Sep 17 00:00:00 2001 From: sneak Date: Sun, 8 Nov 2026 14:08:50 +0000 Subject: [PATCH] Honor NO_COLOR in the terminal log handler (closes #402) The TTY log handler always wrote ANSI color, unlike internal/ui, which disables color when NO_COLOR is set. Add a color flag to TTYHandler, set from NO_COLOR with the same test internal/ui uses, and route every color code through a paint helper that returns plain text when color is off. The README's color sentence now covers log lines too. Model: deepseek-v4-pro --- README.md | 2 ++ internal/log/tty_handler.go | 43 ++++++++++++++++++++++++-------- internal/log/tty_handler_test.go | 16 ++++++++++++ 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index f83d065..3233aaa 100644 --- a/README.md +++ b/README.md @@ -836,6 +836,8 @@ Work planned after 1.0. Loosely ordered by priority. Every command's user-facing output is governed by `internal/ui`, in one of two ways. Color is enabled when the stream written to is a TTY and the `NO_COLOR` environment variable is unset (https://no-color.org/). +The same rule governs the colorized log lines `internal/log` writes to a +terminal. * **Status, progress, warnings, and errors** go through the `internal/ui` message methods below: marker-prefixed, colored on a TTY, and — except diff --git a/internal/log/tty_handler.go b/internal/log/tty_handler.go index fbcef68..288835d 100644 --- a/internal/log/tty_handler.go +++ b/internal/log/tty_handler.go @@ -5,6 +5,7 @@ import ( "fmt" "io" "log/slog" + "os" "strconv" "strings" "sync" @@ -60,6 +61,12 @@ type TTYHandler struct { mu *sync.Mutex out io.Writer + // color reports whether records are written with ANSI color. It is + // false when NO_COLOR is set, matching internal/ui's shouldColor + // test; the caller already chose this handler for a terminal, so + // TTY-ness is not re-checked here. + color bool + // attrs are the attributes accumulated through WithAttrs, emitted // ahead of each record's own attributes. Their keys already carry // the group path that was open when they were added, so no @@ -72,16 +79,19 @@ type TTYHandler struct { groups []string } -// NewTTYHandler creates a new TTY handler with colored output. +// NewTTYHandler creates a new TTY handler. Color is enabled unless the +// NO_COLOR environment variable is set to a non-empty value, the same +// test internal/ui applies to status lines. func NewTTYHandler(out io.Writer, opts *slog.HandlerOptions) *TTYHandler { if opts == nil { opts = &slog.HandlerOptions{} } return &TTYHandler{ - out: out, - opts: *opts, - mu: &sync.Mutex{}, + out: out, + opts: *opts, + mu: &sync.Mutex{}, + color: os.Getenv("NO_COLOR") == "", } } @@ -124,10 +134,10 @@ func (h *TTYHandler) Handle(_ context.Context, r slog.Record) error { // are written around it: it can carry text from an untrusted source // (a storage error, for one), and a raw control character would // otherwise reach the terminal. - _, _ = fmt.Fprintf(h.out, "%s%s%s %s%s%s %s%s%s", - colorGray, timestamp, colorReset, - levelColor, level, colorReset, - colorBold, sanitize(r.Message), colorReset) + _, _ = fmt.Fprintf(h.out, "%s %s %s", + h.paint(colorGray, timestamp), + h.paint(levelColor, level), + h.paint(colorBold, sanitize(r.Message))) // Attributes carried by the handler come first, then the record's // own. Handler attributes were qualified when they were added; the @@ -236,6 +246,7 @@ func (h *TTYHandler) clone() *TTYHandler { opts: h.opts, mu: h.mu, out: h.out, + color: h.color, attrs: make([]slog.Attr, len(h.attrs), len(h.attrs)+1), groups: make([]string, len(h.groups), len(h.groups)+1), } @@ -273,9 +284,19 @@ func (h *TTYHandler) writeAttr(a slog.Attr) { // the snapshot database — so a control character in one of them must // be rendered as an escape sequence rather than reaching the terminal, // where it could move the cursor or inject its own colours. - _, _ = fmt.Fprintf(h.out, " %s%s%s=%s%s%s", - colorCyan, sanitize(a.Key), colorReset, - colorBlue, sanitize(value), colorReset) + _, _ = fmt.Fprintf(h.out, " %s=%s", + h.paint(colorCyan, sanitize(a.Key)), + h.paint(colorBlue, sanitize(value))) +} + +// paint wraps s in the given ANSI color when color is enabled, and +// returns s unchanged otherwise. +func (h *TTYHandler) paint(color, s string) string { + if !h.color { + return s + } + + return color + s + colorReset } // sanitize returns s unchanged when every rune in it is printable, and a diff --git a/internal/log/tty_handler_test.go b/internal/log/tty_handler_test.go index 8d0819d..acb6ee4 100644 --- a/internal/log/tty_handler_test.go +++ b/internal/log/tty_handler_test.go @@ -421,3 +421,19 @@ func TestTTYHandlerEmptyGroupAndAttrsAreNoOps(t *testing.T) { assert.Contains(t, plain, "kept=yes") assert.NotContains(t, plain, " =") } + +// TestTTYHandlerRespectsNoColor checks that the handler writes no ANSI +// color when NO_COLOR is set, matching internal/ui's shouldColor test. +// The handler is only ever chosen for a terminal, so this is the one +// remaining condition that can still leave color in log output. +func TestTTYHandlerRespectsNoColor(t *testing.T) { + t.Setenv("NO_COLOR", "1") + + var buf bytes.Buffer + + logger := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions())) + logger.Info("hello", "key", "value") + + assert.NotContains(t, buf.String(), "\x1b[", + "NO_COLOR must suppress every ANSI escape in log output") +}