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)

README.md

@@ -313,10 +313,12 @@ when the next path element is a number (so
 `vaultik config set age_recipients.0 age1...` works on a file without
 `age_recipients`), otherwise as a map. If the config loads, a change after which
 it would not, such as `vaultik config set compression_level 99`, is refused and
-the file is left unchanged. A config that does not load is checked one setting
-at a time. Examples are the file `config init` writes before `storage_url` is
-set, and one with a `${ENV:...}` reference to a variable not set in this shell.
-In such a config, an unknown key or a value of the wrong type is refused.
+the file is left unchanged. A config that does not load, such as the one
+`config init` writes before `storage_url` is set or one with a `${ENV:...}`
+reference to a variable not set in this shell, is checked one setting at a time:
+only a change after which the changed setting cannot be read, such as one giving
+it the wrong type (a single value where a list belongs or a word where a number
+belongs), is refused.
 
 **`snapshot create`**: Perform incremental backup of configured snapshots.
 
@@ -829,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

internal/cli/config.go

@@ -46,7 +46,6 @@ var (
 	errNeedNumericIndex = errors.New("key is a list; use a numeric index")
 	errIndexOutOfRange  = errors.New("index out of range")
 	errNotMapOrList     = errors.New("key is not a map or list")
-	errNoSuchSetting    = errors.New("no setting has this key")
 	errWouldNotLoad     = errors.New(
 		"vaultik would not load the config after this change, " +
 			"so the file was not changed")
@@ -444,10 +443,11 @@ number, otherwise as a map.
 
 If the config loads, a change after which it would not, such as
 compression_level 99, is refused and the file is left unchanged. A
-config that does not load is checked one setting at a time. Examples
-are the file config init writes before storage_url is set, and one
-with a ${ENV:...} reference to a variable not set in this shell. In
-such a config, an unknown key or a value of the wrong type is refused.
+config that does not load, such as the one config init writes before
+storage_url is set or one with a ${ENV:...} reference to a variable
+not set here, is checked one setting at a time: only a change after
+which the changed setting cannot be read, such as one giving it the
+wrong type, is refused.
 
 Examples:
   vaultik config set storage_url "file:///mnt/backups"
@@ -484,11 +484,6 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {
 
 	keys := strings.Split(key, ".")
 
-	_, ok := config.SettingType(keys)
-	if !ok {
-		return fmt.Errorf("%w: %s", errNoSuchSetting, key)
-	}
-
 	err = yamlPathSet(root, keys, value)
 	if err != nil {
 		return err
@@ -744,11 +739,41 @@ func yamlPathSet(root *yaml.Node, keys []string, value string) error {
 }
 
 // configKeyIsString reports whether the dotted key names a string in
-// config.Config, as s3.access_key_id and snapshots.home.exclude.0 do.
+// config.Config, following the fields' yaml tags, as s3.access_key_id and
+// snapshots.home.exclude.0 do.
 func configKeyIsString(keys []string) bool {
-	typ, ok := config.SettingType(keys)
+	typ := reflect.TypeFor[config.Config]()
 
-	return ok && typ.Kind() == reflect.String
+	for _, key := range keys {
+		switch {
+		case typ.Kind() == reflect.Map || typ.Kind() == reflect.Slice:
+			// The key is a snapshot name or a list index.
+			typ = typ.Elem()
+		case typ.Kind() == reflect.Struct:
+			field, ok := yamlField(typ, key)
+			if !ok {
+				return false
+			}
+
+			typ = field.Type
+		default:
+			return false
+		}
+	}
+
+	return typ.Kind() == reflect.String
+}
+
+// yamlField returns the field of struct type typ whose yaml tag names key.
+func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
+	for field := range typ.Fields() {
+		name, _, _ := strings.Cut(field.Tag.Get("yaml"), ",")
+		if name == key {
+			return field, true
+		}
+	}
+
+	return reflect.StructField{}, false
 }
 
 // yamlSetInMapping resolves (creating if needed) the value node for the

internal/cli/config_test.go

@@ -225,7 +225,7 @@ func TestConfigSetIndexUnderMissingKey(t *testing.T) {
 // change after which the config would not load, with an error giving the
 // reason, and leaves the file as it was. On the file `config init` writes,
 // which does not load until storage_url is set, and on one that cannot be
-// read here, only an unknown key or a value of the wrong type is refused.
+// read here, only a value of the wrong type is refused.
 func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 	t.Parallel()
 
@@ -237,14 +237,6 @@ func TestConfigSetKeepsFileThatWouldNotLoad(t *testing.T) {
 		value  string
 		reason string
 	}{
-		// Keys that no setting has, on a config that loads and on one
-		// that does not.
-		{configWithoutRecipients, "nosuchkey", "1",
-			"no setting has this key: nosuchkey"},
-		{configWithoutRecipients, "snapshots.home.exlude", "/tmp",
-			"no setting has this key: snapshots.home.exlude"},
-		{defaultConfigText(macOS), "compression_lvl", "9",
-			"no setting has this key: compression_lvl"},
 		// Index 1 of the empty list the missing key gives.
 		{configWithoutRecipients, "age_recipients.1", testRecipient,
 			"index out of range"},

internal/config/config.go

@@ -9,7 +9,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"reflect"
 	"sort"
 	"strings"
 	"time"
@@ -287,36 +286,15 @@ func Load(path string) (*Config, error) {
 }
 
 // Parse reads YAML config data as Load does, without validating it. It
-// refuses a key that no setting has, applies default values for optional
-// fields, fills in ${...} references using smartconfig, and expands ~ in
-// paths.
+// applies default values for optional fields, fills in ${...} references
+// using smartconfig, and expands ~ in paths.
 func Parse(data []byte) (*Config, error) {
-	// smartconfig drops line numbers, so keys are checked in the data as
-	// written.
-	var root yaml.Node
-
-	err := yaml.Unmarshal(data, &root)
-	if err != nil {
-		return nil, fmt.Errorf("failed to parse config: %w", err)
-	}
-
-	err = checkKeys(&root, reflect.TypeFor[Config](), "")
-	if err != nil {
-		return nil, err
-	}
-
 	// Load config using smartconfig for interpolation
 	sc, err := smartconfig.NewFromReader(bytes.NewReader(data))
 	if err != nil {
 		return nil, fmt.Errorf("failed to load config file: %w", err)
 	}
 
-	// A ${...} reference can bring in keys that are not in the file.
-	err = checkFilledKeys(sc.Data())
-	if err != nil {
-		return nil, err
-	}
-
 	cfg := &Config{
 		// Set defaults
 		BlobSizeLimit:    defaultBlobSizeLimit,

internal/config/keys.go

@@ -1,176 +0,0 @@
-package config
-
-import (
-	"errors"
-	"fmt"
-	"reflect"
-	"strconv"
-	"strings"
-
-	"gopkg.in/yaml.v3"
-)
-
-// yamlMergeTag is the tag YAML gives a merge key (<<), which adds the keys
-// of the map it names to the map it sits in.
-const yamlMergeTag = "!!merge"
-
-// envSection is the top-level key of a map of environment variables that
-// smartconfig sets while it loads the config. It is not a vaultik setting,
-// so its keys are not checked.
-const envSection = "env"
-
-var errUnknownKey = errors.New("unknown config key")
-
-// SettingType returns the type of the setting that the dotted key names in
-// Config, following the fields' yaml tags, as for s3.bucket or
-// snapshots.home.exclude.0. A snapshot name or a list index may be any
-// word. It returns false when no setting has that key.
-func SettingType(keys []string) (reflect.Type, bool) {
-	typ := reflect.TypeFor[Config]()
-
-	for _, key := range keys {
-		switch {
-		case typ.Kind() == reflect.Map || typ.Kind() == reflect.Slice:
-			typ = typ.Elem()
-		case typ.Kind() == reflect.Struct:
-			field, ok := yamlField(typ, key)
-			if !ok {
-				return nil, false
-			}
-
-			typ = field.Type
-		default:
-			return nil, false
-		}
-	}
-
-	return typ, true
-}
-
-// yamlField returns the field of struct type typ whose yaml tag names key.
-func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
-	for field := range typ.Fields() {
-		name, _, _ := strings.Cut(field.Tag.Get("yaml"), ",")
-		if name == key && name != "-" {
-			return field, true
-		}
-	}
-
-	return reflect.StructField{}, false
-}
-
-// checkKeys returns an error naming the first key in node, a parsed config
-// or part of one at the dotted path, that no setting in typ has. The error
-// gives the key's line in the file, when it has one. A value of the wrong
-// type is left for the YAML decoder to report.
-func checkKeys(node *yaml.Node, typ reflect.Type, path string) error {
-	switch node.Kind {
-	case yaml.DocumentNode:
-		for _, child := range node.Content {
-			err := checkKeys(child, typ, path)
-			if err != nil {
-				return err
-			}
-		}
-	case yaml.AliasNode:
-		return checkKeys(node.Alias, typ, path)
-	case yaml.SequenceNode:
-		if typ.Kind() != reflect.Slice {
-			return nil
-		}
-
-		for i, child := range node.Content {
-			err := checkKeys(child, typ.Elem(), joinKey(path, strconv.Itoa(i)))
-			if err != nil {
-				return err
-			}
-		}
-	case yaml.MappingNode:
-		return checkMappingKeys(node, typ, path)
-	case yaml.ScalarNode:
-		// A scalar has no keys.
-	}
-
-	return nil
-}
-
-// checkFilledKeys is checkKeys for data, the config after smartconfig has
-// filled in its ${...} references. A key that a reference brings in is not
-// in the file, and the copy that Encode makes has no line numbers.
-func checkFilledKeys(data map[string]any) error {
-	var filled yaml.Node
-
-	err := filled.Encode(data)
-	if err != nil {
-		return fmt.Errorf("failed to marshal config data: %w", err)
-	}
-
-	return checkKeys(&filled, reflect.TypeFor[Config](), "")
-}
-
-// checkMappingKeys is checkKeys for a map.
-func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string) error {
-	for i := 0; i+1 < len(node.Content); i += 2 {
-		key, value := node.Content[i], node.Content[i+1]
-
-		if key.Tag == yamlMergeTag {
-			// The value is one map or a list of maps.
-			merged := []*yaml.Node{value}
-			if value.Kind == yaml.SequenceNode {
-				merged = value.Content
-			}
-
-			for _, m := range merged {
-				err := checkKeys(m, typ, path)
-				if err != nil {
-					return err
-				}
-			}
-
-			continue
-		}
-
-		keyPath := joinKey(path, key.Value)
-		if keyPath == envSection {
-			continue
-		}
-
-		var valueType reflect.Type
-
-		switch {
-		case typ.Kind() == reflect.Map:
-			valueType = typ.Elem()
-		case typ.Kind() == reflect.Struct:
-			field, ok := yamlField(typ, key.Value)
-			if !ok && key.Line == 0 {
-				// A key that a ${...} reference brought in has no line.
-				return fmt.Errorf("%w %q", errUnknownKey, keyPath)
-			}
-
-			if !ok {
-				return fmt.Errorf("%w %q on line %d",
-					errUnknownKey, keyPath, key.Line)
-			}
-
-			valueType = field.Type
-		default:
-			return nil
-		}
-
-		err := checkKeys(value, valueType, keyPath)
-		if err != nil {
-			return err
-		}
-	}
-
-	return nil
-}
-
-// joinKey appends key to the dotted path.
-func joinKey(path, key string) string {
-	if path == "" {
-		return key
-	}
-
-	return path + "." + key
-}

internal/config/keys_test.go

@@ -1,115 +0,0 @@
-package config //nolint:testpackage // checks the unexported errUnknownKey
-
-import (
-	"errors"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-)
-
-// keysTestConfig loads; each case below adds one line to it.
-const keysTestConfig = "storage_url: file:///tmp/vaultik-test-storage\n" +
-	"s3:\n" +
-	"  region: us-east-1\n" +
-	"snapshots:\n" +
-	"  test:\n" +
-	"    paths: [/tmp/vaultik-test-source]\n"
-
-// TestParseRefusesUnknownKey checks that a key no setting has, at any
-// depth, fails the parse with an error naming the key and its line. A key
-// that a ${...} reference brings in is not in the file, so its error gives
-// no line.
-func TestParseRefusesUnknownKey(t *testing.T) {
-	t.Parallel()
-
-	snapshotPath := filepath.Join(t.TempDir(), "snapshot.yml")
-
-	err := os.WriteFile(snapshotPath, []byte("other:\n"+
-		"  paths: [/tmp/vaultik-test-source]\n"+
-		"  exlude: [/tmp/cache]\n"), 0o600)
-	if err != nil {
-		t.Fatalf("write snapshot: %v", err)
-	}
-
-	tests := []struct {
-		name string
-		yaml string
-		want string
-	}{
-		{"top level", keysTestConfig + "compression_lvl: 9\n",
-			`unknown config key "compression_lvl" on line 7`},
-		{"in a snapshot", keysTestConfig + "    exlude: [/tmp/cache]\n",
-			`unknown config key "snapshots.test.exlude" on line 7`},
-		{"in s3", strings.Replace(keysTestConfig, "  region:",
-			"  bukcet: b\n  region:", 1),
-			`unknown config key "s3.bukcet" on line 3`},
-		{"from a reference",
-			keysTestConfig + "  other: ${YAML:" + snapshotPath + ":other}\n",
-			`unknown config key "snapshots.other.exlude"`},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			t.Parallel()
-
-			_, err := Parse([]byte(tt.yaml))
-			if !errors.Is(err, errUnknownKey) {
-				t.Fatalf("Parse() error = %v, want errUnknownKey", err)
-			}
-
-			if err.Error() != tt.want {
-				t.Errorf("Parse() error = %q, want %q", err.Error(), tt.want)
-			}
-		})
-	}
-}
-
-// TestParseAcceptsKnownKeys checks that a config with a ${...} reference
-// and a YAML merge key (<<) still parses.
-func TestParseAcceptsKnownKeys(t *testing.T) {
-	t.Parallel()
-
-	levelPath := filepath.Join(t.TempDir(), "level")
-
-	err := os.WriteFile(levelPath, []byte("9"), 0o600)
-	if err != nil {
-		t.Fatalf("write level: %v", err)
-	}
-
-	data := strings.Replace(keysTestConfig, "  test:", "  test: &test", 1) +
-		"  other:\n" +
-		"    <<: *test\n" +
-		"    exclude: [/tmp/vaultik-test-source/cache]\n" +
-		"compression_level: ${FILE:" + levelPath + "}\n"
-
-	cfg, err := Parse([]byte(data))
-	if err != nil {
-		t.Fatalf("Parse() unexpected error: %v", err)
-	}
-
-	if cfg.CompressionLevel != 9 {
-		t.Errorf("compression_level = %d, want 9", cfg.CompressionLevel)
-	}
-
-	other := cfg.Snapshots["other"]
-	if len(other.Paths) != 1 || len(other.Exclude) != 1 {
-		t.Errorf("snapshot other = %+v, want the paths of test and one exclude",
-			other)
-	}
-}
-
-// TestParseAcceptsEnvSection checks that a config with a top-level env
-// section still parses. smartconfig sets the environment variables it
-// lists, so its keys are not settings.
-func TestParseAcceptsEnvSection(t *testing.T) {
-	const name = "VAULTIK_TEST_ENV_SECTION"
-
-	// Parse sets the variable; t.Setenv restores it after the test.
-	t.Setenv(name, "")
-
-	_, err := Parse([]byte(keysTestConfig + "env:\n  " + name + ": set\n"))
-	if err != nil {
-		t.Fatalf("Parse() unexpected error: %v", err)
-	}
-}

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

internal/log/tty_handler_test.go

@@ -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")
+			}
+		})
+	}
+}