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 (implementation); opus-5-5 (rebase)
2026-11-10 22:25:49 +00:00
7 changed files with 93 additions and 205 deletions
@@ -831,7 +831,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/).
+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
@@ -1041,28 +1042,20 @@ is `git describe --tags --always --dirty`, which `script/version` runs for the
 A `docker build .` of a clone, with no build arguments, runs the same
 `git describe` (without `--dirty`) on the `.git` in its build context, so it
 stamps the same value for a clean commit; the build fails if the context carries
-`.git` and no version, commit or commit date comes out. `script/docker` and
-`script/cibuild` stamp `unknown` when they run outside a git checkout.
-
-A binary built by plain `go install` or `go build` carries no stamped version.
-It reports the version Go recorded in it instead:
-
- a `go install` of the tag `v1.0.0` → `v1.0.0`.
- a `go build` of an untagged commit → a Go pseudo-version, such as
-  `v1.0.1-0.20261110231226-0123456789ab`.
- a build where Go recorded no version → `dev`.
+`.git` and no version, commit or commit date comes out. A binary built without
+git metadata reports `dev`, or `unknown` when `script/docker` or
+`script/cibuild` built it outside a git checkout.

 `goreleaser` stamps a release binary with the tag minus its leading `v`, so the
 tag `v1.0.0` produces `vaultik 1.0.0`, matching the archive name
 `vaultik_1.0.0_linux_amd64.tar.gz`. `goreleaser --snapshot` stamps
 `dev-<12 chars of the commit sha>` rather than inventing the next patch number.
 `vaultik version` calls a build a development build when its version is `dev`,
-`unknown`, `dev-<sha>`, the short commit sha, `<tag>-<N>-g<short sha>` or a Go
-pseudo-version, with or without `-dirty` or `+dirty`; only a plain tag, such as
-`v1.0.0` or `1.0.0`, is a release. If `script/version` cannot be run at all,
-`make` stops with an error instead of building an unversioned binary, and a
-binary that somehow carries an empty version string still reports itself as a
-development build.
+`unknown`, `dev-<sha>`, the short commit sha or `<tag>-<N>-g<short sha>`, with
+or without `-dirty`; only a plain tag, such as `v1.0.0` or `1.0.0`, is a
+release. If `script/version` cannot be run at all, `make` stops with an error
+instead of building an unversioned binary, and a binary that somehow carries an
+empty version string still reports itself as a development build.

 ### cutting a release

@@ -24,7 +24,6 @@ require (
 	github.com/stretchr/testify v1.11.1
 	go.uber.org/dig v1.19.0
 	go.uber.org/fx v1.24.0
-	golang.org/x/mod v0.29.0
 	golang.org/x/sync v0.18.0
 	golang.org/x/sys v0.38.0
 	golang.org/x/term v0.37.0
@@ -21,8 +21,6 @@ const shortCommitLen = 12
 const exitCodeInterrupted = 130

 // Entry is the main entry point for the CLI application.
-// It first takes the version, commit and commit date the linker flags
-// left unset from the Go build info (see globals.FromBuildInfo).
 // It prints the startup banner to stderr (unless a banner-suppressing
 // flag is present in os.Args — see bannerSuppressedInArgs), executes the
 // root cobra command, and routes any returned error through the
@@ -35,7 +33,6 @@ const exitCodeInterrupted = 130
 // deferred profile writers run before the process ends. See run in
 // cmd/vaultik/main.go.
 func Entry() int {
-	globals.UseBuildInfo()
 	emitStartupBanner(os.Args[1:], os.Stderr)

 	rootCmd := NewRootCommand()
@@ -1,24 +1,19 @@
 // Package globals holds application-wide metadata (name, version,
-// commit) that is populated at build time via linker flags, or from the
-// build info Go records in the binary when the linker flags set none.
+// commit) that is populated at build time via linker flags.
 package globals

 import (
 	"regexp"
-	"runtime/debug"
 	"strings"
 	"time"
-
-	"golang.org/x/mod/module"
 )

 // Appname is the application name, populated from main().
 var Appname = "vaultik" //nolint:gochecknoglobals // set via -ldflags at build time

 // DevVersion is the version a binary reports when it was built without
-// git metadata: script/version emits it outside a git checkout, and a
-// binary keeps it when neither its linker flags nor its Go build info
-// name a version. goreleaser's snapshot template stamps
+// git metadata: script/version emits it outside a git checkout, and an
+// unstamped `go build` keeps it. goreleaser's snapshot template stamps
 // it followed by "-" and the commit it was built from. It is
 // deliberately not a number.
 const DevVersion = "dev"
@@ -37,52 +32,6 @@ var Commit = Unknown //nolint:gochecknoglobals // set via -ldflags at build time
 // CommitDate is the ISO-8601 date of the commit, populated from main().
 var CommitDate = Unknown //nolint:gochecknoglobals // set via -ldflags at build time

-// develVersion is the main module version Go records when it cannot
-// tell which version it built.
-const develVersion = "(devel)"
-
-// UseBuildInfo applies FromBuildInfo to Version, Commit and CommitDate,
-// with the build info of the running binary.
-func UseBuildInfo() {
-	info, ok := debug.ReadBuildInfo()
-	if !ok {
-		return
-	}
-
-	Version, Commit, CommitDate = FromBuildInfo(
-		info, Version, Commit, CommitDate)
-}
-
-// FromBuildInfo returns version, commit and commitDate, with each one
-// the linker flags left unset taken from info. A `go install` of a tag
-// sets no linker flags, but Go records the tag as the main module's
-// version. Go records the commit and its time only when it builds from
-// a git checkout. The time is cut to a date, as the Makefile stamps it.
-func FromBuildInfo(
-	info *debug.BuildInfo, version, commit, commitDate string,
-) (string, string, string) {
-	if (version == "" || version == DevVersion) &&
-		info.Main.Version != "" && info.Main.Version != develVersion {
-		version = info.Main.Version
-	}
-
-	for _, s := range info.Settings {
-		switch s.Key {
-		case "vcs.revision":
-			if commit == Unknown {
-				commit = s.Value
-			}
-		case "vcs.time":
-			t, err := time.Parse(time.RFC3339, s.Value)
-			if commitDate == Unknown && err == nil {
-				commitDate = t.UTC().Format(time.DateOnly)
-			}
-		}
-	}
-
-	return version, commit, commitDate
-}
-
 // Author identifies the upstream author of vaultik.
 const Author = "Jeffrey Paul <sneak@sneak.berlin>"

@@ -119,10 +68,8 @@ func New() (*Globals, error) {
 // a release. "dev" and goreleaser's snapshot "dev-<sha>" count, and so
 // does what `git describe --tags --always --dirty` gives a make or
 // docker build of an untagged commit: the bare short commit, or
-// tag-N-gHASH on a commit after a tag. A Go pseudo-version counts: Go
-// records one for a build of an untagged commit without linker flags.
-// Any version ending in "-dirty" or "+dirty" counts, a modified
-// checkout of a tag ("v1.0.0-dirty", "v1.0.0+dirty") included.
+// tag-N-gHASH on a commit after a tag. Any version ending in "-dirty"
+// counts, a modified checkout of a tag ("v1.0.0-dirty") included.
 // A plain tag such as "v1.0.0" or "1.0.0" is a release.
 //
 // The empty string counts too. Nothing that knows its version reports
@@ -135,8 +82,7 @@ func New() (*Globals, error) {
 func IsDevVersion(v string) bool {
 	if v == "" || v == Unknown || v == DevVersion ||
 		strings.HasPrefix(v, DevVersion+"-") ||
-		strings.HasSuffix(v, "-dirty") || strings.HasSuffix(v, "+dirty") ||
-		module.IsPseudoVersion(v) {
+		strings.HasSuffix(v, "-dirty") {
 		return true
 	}

@@ -1,7 +1,6 @@
 package globals_test

 import (
-	"runtime/debug"
 	"testing"

 	"sneak.berlin/go/vaultik/internal/globals"
@@ -60,20 +59,12 @@ func TestIsDevVersion(t *testing.T) {
 		{"1.0.0-rc.1-12-g877eb2f", true},
 		// A tagged commit with uncommitted changes is not that tag.
 		{"v1.0.0-dirty", true},
-		// What Go records for a build without linker flags: a
-		// pseudo-version for an untagged commit, "+dirty" for a
-		// modified checkout.
-		{"v1.0.0-rc3.0.20261111001226-abcdefabcdef", true},
-		{"v0.0.0-20261111001226-abcdefabcdef", true},
-		{"v1.0.1-0.20261111001226-abcdefabcdef", true},
-		{"v1.0.0+dirty", true},
 		// What a tagged build produces (goreleaser's .Version strips
 		// the leading "v"; script/version keeps it).
 		{"1.0.0", false},
 		{"0.1.0", false},
 		{"1.0.0-rc.1", false},
 		{"v1.0.0", false},
-		{"v1.0.0-rc3", false},
 		// A release must not be mistaken for a dev build just because
 		// the string happens to contain "dev".
 		{"1.0.0-dev", false},
@@ -95,9 +86,10 @@ func TestIsDevVersion(t *testing.T) {
 	}
 }

-// TestDefaultVersionIsDev pins the default: a binary whose linker flags
-// and build info name no version must report itself as a development
-// build rather than as some default release number.
+// TestDefaultVersionIsDev pins the linker-flag contract: an unstamped
+// binary (no -ldflags at all, which is what `go build ./...` and `go
+// install` produce) must report itself as a development build rather
+// than as some default release number.
 func TestDefaultVersionIsDev(t *testing.T) {
 	t.Parallel()

@@ -106,104 +98,3 @@ func TestDefaultVersionIsDev(t *testing.T) {
 			globals.DevVersion)
 	}
 }
-
-// buildInfo returns build info with the given main module version and,
-// when revision is not empty, the commit settings Go records for a
-// build from a git checkout.
-func buildInfo(version, revision string) *debug.BuildInfo {
-	info := &debug.BuildInfo{
-		Main: debug.Module{Path: "sneak.berlin/go/vaultik", Version: version},
-	}
-
-	if revision != "" {
-		info.Settings = []debug.BuildSetting{
-			{Key: "vcs.revision", Value: revision},
-			{Key: "vcs.time", Value: "2026-11-10T23:12:26Z"},
-		}
-	}
-
-	return info
-}
-
-// TestFromBuildInfo covers a binary from `go install` of a tag, which
-// carries no linker flags: it must report the tag. Values the linker
-// flags set are kept.
-func TestFromBuildInfo(t *testing.T) {
-	t.Parallel()
-
-	const (
-		tag = "v1.0.0-rc3"
-		rev = "0123456789abcdef0123456789abcdef01234567"
-	)
-
-	cases := []struct {
-		name                    string
-		info                    *debug.BuildInfo
-		version, commit, date   string
-		wantVersion, wantCommit string
-		wantDate                string
-	}{
-		{
-			name:        "go install of a tag",
-			info:        buildInfo(tag, ""),
-			version:     globals.DevVersion,
-			commit:      globals.Unknown,
-			date:        globals.Unknown,
-			wantVersion: tag,
-			wantCommit:  globals.Unknown,
-			wantDate:    globals.Unknown,
-		},
-		{
-			name:        "empty version",
-			info:        buildInfo(tag, ""),
-			version:     "",
-			commit:      globals.Unknown,
-			date:        globals.Unknown,
-			wantVersion: tag,
-			wantCommit:  globals.Unknown,
-			wantDate:    globals.Unknown,
-		},
-		{
-			name:        "go build in a git checkout",
-			info:        buildInfo(tag, rev),
-			version:     globals.DevVersion,
-			commit:      globals.Unknown,
-			date:        globals.Unknown,
-			wantVersion: tag,
-			wantCommit:  rev,
-			wantDate:    "2026-11-10",
-		},
-		{
-			name:        "Go could not tell the version",
-			info:        buildInfo("(devel)", ""),
-			version:     globals.DevVersion,
-			commit:      globals.Unknown,
-			date:        globals.Unknown,
-			wantVersion: globals.DevVersion,
-			wantCommit:  globals.Unknown,
-			wantDate:    globals.Unknown,
-		},
-		{
-			name:        "linker flags set",
-			info:        buildInfo(tag, rev),
-			version:     "v1.0.0-rc2-4-gabcdef1",
-			commit:      "abcdef1",
-			date:        "2026-11-01",
-			wantVersion: "v1.0.0-rc2-4-gabcdef1",
-			wantCommit:  "abcdef1",
-			wantDate:    "2026-11-01",
-		},
-	}
-
-	for _, tc := range cases {
-		version, commit, date := globals.FromBuildInfo(
-			tc.info, tc.version, tc.commit, tc.date)
-
-		if version != tc.wantVersion || commit != tc.wantCommit ||
-			date != tc.wantDate {
-			t.Errorf("%s: got %q, %q, %q; want %q, %q, %q", tc.name,
-				version, commit, date,
-				tc.wantVersion, tc.wantCommit, tc.wantDate)
-		}
-	}
-}
@@ -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
@@ -421,3 +421,44 @@ func TestTTYHandlerEmptyGroupAndAttrsAreNoOps(t *testing.T) {
 	assert.Contains(t, plain, "kept=yes")
 	assert.NotContains(t, plain, " =")
 }
+
+// TestTTYHandlerRespectsNoColor checks that only a non-empty NO_COLOR
+// turns color off, 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) {
+	cases := []struct {
+		name    string
+		noColor string
+		colored bool
+	}{
+		{
+			name:    "NO_COLOR turns color off",
+			noColor: "1",
+		},
+		{
+			name:    "empty NO_COLOR keeps color on",
+			noColor: "",
+			colored: true,
+		},
+	}
+
+	for _, testCase := range cases {
+		t.Run(testCase.name, func(t *testing.T) {
+			t.Setenv("NO_COLOR", testCase.noColor)
+
+			var buf bytes.Buffer
+
+			logger := slog.New(log.NewTTYHandler(&buf, debugHandlerOptions()))
+			logger.Info("hello", "key", "value")
+
+			if testCase.colored {
+				assert.Contains(t, buf.String(), "\x1b[",
+					"an empty NO_COLOR must leave ANSI color in log output")
+			} else {
+				assert.NotContains(t, buf.String(), "\x1b[",
+					"NO_COLOR must suppress every ANSI escape in log output")
+			}
+		})
+	}
+}