Refuse config keys that no setting has (closes #419 )

config.Parse walks the file as written and fails on the first key that matches no setting, naming its dotted path and line, so a typo in exclude or prune_grace_period no longer falls back silently. Keys that a ${...} reference brings in are checked once smartconfig has filled it in, and named without a line. YAML merge keys and a top-level env section, which smartconfig reads, still load; an anchor that contains itself is an error. config set refuses such a key before writing, with the same error. The key-to-field lookup moves from internal/cli to config.SettingType so both use it. Model: opus-5-5
2026-11-11 01:54:13 +00:00
9 changed files with 49 additions and 232 deletions
@@ -483,11 +483,9 @@ func writeConfigSet(out *ui.Writer, path, key, value string) error {

 	keys := strings.Split(key, ".")

-	if keys[0] != config.EnvSection {
-		_, err = config.SettingType(keys)
-		if err != nil {
-			return err
-		}
+	_, err = config.SettingType(keys)
+	if err != nil {
+		return err
 	}

 	err = yamlPathSet(root, keys, value)
@@ -343,39 +343,6 @@ func TestConfigSetRefusesOnlyItsOwnFailure(t *testing.T) {
 	}
 }

-// TestConfigSetWritesEnvSection checks that config set writes a key under
-// the top-level env section, whose keys are not settings, on a config that
-// loads.
-func TestConfigSetWritesEnvSection(t *testing.T) {
-	const name = "VAULTIK_TEST_CONFIG_SET_ENV"
-
-	// Loading the changed config sets the variable; t.Setenv restores it
-	// after the test.
-	t.Setenv(name, "")
-
-	path := filepath.Join(t.TempDir(), "config.yml")
-
-	err := os.WriteFile(path, []byte(configWithoutRecipients), configFileMode)
-	if err != nil {
-		t.Fatalf("write config: %v", err)
-	}
-
-	err = writeConfigSet(ui.NewWithColor(&bytes.Buffer{}, false),
-		path, "env."+name, "value")
-	if err != nil {
-		t.Fatalf("config set env.%s: %v", name, err)
-	}
-
-	data, err := os.ReadFile(path) //nolint:gosec // G304: test-controlled path
-	if err != nil {
-		t.Fatalf("read config: %v", err)
-	}
-
-	if !strings.Contains(string(data), "env:\n  "+name+": value\n") {
-		t.Errorf("config set env.%s wrote:\n%s", name, data)
-	}
-}
-
 // TestConfigSetStringLooksLikeNumber sets string settings to values that
 // YAML reads as numbers or booleans when they are unquoted, and checks that
 // config.Load returns each one unchanged.
@@ -300,8 +300,7 @@ func Parse(data []byte) (*Config, error) {
 		return nil, fmt.Errorf("failed to parse config: %w", err)
 	}

-	err = checkKeys(&root, reflect.TypeFor[Config](), "",
-		map[*yaml.Node]bool{}, map[nodeAndType]bool{})
+	err = checkKeys(&root, reflect.TypeFor[Config](), "", map[*yaml.Node]bool{})
 	if err != nil {
 		return nil, err
 	}
@@ -14,23 +14,16 @@ import (
 // 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
+// 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"
+const envSection = "env"

 var (
 	errUnknownKey           = errors.New("unknown config key")
 	errAnchorContainsItself = errors.New("anchor contains itself")
 )

-// nodeAndType is a node of a parsed config and the setting type it is
-// checked against.
-type nodeAndType struct {
-	node *yaml.Node
-	typ  reflect.Type
-}
-
 // 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
@@ -74,16 +67,14 @@ func yamlField(typ reflect.Type, key string) (reflect.StructField, bool) {
 // 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. following holds the aliases
 // the walk is inside. Meeting one of them again means its anchor contains
-// itself, which is an error. checked holds each anchored node the walk has
-// checked completely, with the type it was checked against. An alias to
-// that node is not followed again for that type.
+// itself, which is an error.
 func checkKeys(node *yaml.Node, typ reflect.Type, path string,
-	following map[*yaml.Node]bool, checked map[nodeAndType]bool,
+	following map[*yaml.Node]bool,
 ) error {
 	switch node.Kind {
 	case yaml.DocumentNode:
 		for _, child := range node.Content {
-			err := checkKeys(child, typ, path, following, checked)
+			err := checkKeys(child, typ, path, following)
 			if err != nil {
 				return err
 			}
@@ -94,34 +85,23 @@ func checkKeys(node *yaml.Node, typ reflect.Type, path string,
 				errAnchorContainsItself, node.Value, node.Line)
 		}

-		target := nodeAndType{node.Alias, typ}
-		if checked[target] {
-			return nil
-		}
-
 		following[node] = true
 		defer delete(following, node)

-		err := checkKeys(node.Alias, typ, path, following, checked)
-		if err != nil {
-			return err
-		}
-
-		checked[target] = true
+		return checkKeys(node.Alias, typ, path, following)
 	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)),
-				following, checked)
+			err := checkKeys(child, typ.Elem(), joinKey(path, strconv.Itoa(i)), following)
 			if err != nil {
 				return err
 			}
 		}
 	case yaml.MappingNode:
-		return checkMappingKeys(node, typ, path, following, checked)
+		return checkMappingKeys(node, typ, path, following)
 	case yaml.ScalarNode:
 		// A scalar has no keys.
 	}
@@ -140,13 +120,12 @@ func checkFilledKeys(data map[string]any) error {
 		return fmt.Errorf("failed to marshal config data: %w", err)
 	}

-	return checkKeys(&filled, reflect.TypeFor[Config](), "",
-		map[*yaml.Node]bool{}, map[nodeAndType]bool{})
+	return checkKeys(&filled, reflect.TypeFor[Config](), "", map[*yaml.Node]bool{})
 }

 // checkMappingKeys is checkKeys for a map.
 func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string,
-	following map[*yaml.Node]bool, checked map[nodeAndType]bool,
+	following map[*yaml.Node]bool,
 ) error {
 	for i := 0; i+1 < len(node.Content); i += 2 {
 		key, value := node.Content[i], node.Content[i+1]
@@ -159,7 +138,7 @@ func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string,
 			}

 			for _, m := range merged {
-				err := checkKeys(m, typ, path, following, checked)
+				err := checkKeys(m, typ, path, following)
 				if err != nil {
 					return err
 				}
@@ -169,7 +148,7 @@ func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string,
 		}

 		keyPath := joinKey(path, key.Value)
-		if keyPath == EnvSection {
+		if keyPath == envSection {
 			continue
 		}

@@ -195,7 +174,7 @@ func checkMappingKeys(node *yaml.Node, typ reflect.Type, path string,
 			return nil
 		}

-		err := checkKeys(value, valueType, keyPath, following, checked)
+		err := checkKeys(value, valueType, keyPath, following)
 		if err != nil {
 			return err
 		}
@@ -2,11 +2,8 @@ package config //nolint:testpackage // checks the unexported errUnknownKey

 import (
 	"errors"
-	"fmt"
 	"os"
 	"path/filepath"
-	"slices"
-	"strconv"
 	"strings"
 	"testing"
 )
@@ -87,32 +84,6 @@ func TestParseRefusesAnchorThatContainsItself(t *testing.T) {
 	}
 }

-// TestParseRefusesExcessiveAliasing checks that Parse returns an error for
-// a file whose merge keys (<<) fan out. Each of eight snapshots merges the
-// one before it ten times. A key check that followed every alias would walk
-// the first snapshot over 10^8 times and run past the package's test
-// timeout.
-func TestParseRefusesExcessiveAliasing(t *testing.T) {
-	t.Parallel()
-
-	const snapshots, merges = 8, 10
-
-	var data strings.Builder
-
-	data.WriteString(strings.Replace(keysTestConfig, "  test:", "  test: &s0", 1))
-
-	for i := 1; i <= snapshots; i++ {
-		aliases := slices.Repeat([]string{"*s" + strconv.Itoa(i-1)}, merges)
-		fmt.Fprintf(&data, "  s%d: &s%d\n    <<: [%s]\n",
-			i, i, strings.Join(aliases, ", "))
-	}
-
-	_, err := Parse([]byte(data.String()))
-	if err == nil {
-		t.Fatal("Parse() returned no error")
-	}
-}
-
 // TestParseAcceptsKnownKeys checks that a config with a ${...} reference
 // and a YAML merge key (<<) still parses.
 func TestParseAcceptsKnownKeys(t *testing.T) {
@@ -7,7 +7,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"sync"
 	"testing"

 	"github.com/spf13/afero"
@@ -25,28 +24,19 @@ import (
 // that is not plugged in, cannot be listed. It is not an empty store, so
 // no command may conclude from it that the local snapshots are gone.

-// newFileDestinationVaultik writes the fault source tree and returns a
-// Vaultik that backs it up to a file:// destination at storeDir.
+// newFileDestinationVaultik returns a Vaultik that backs up the
+// snapshot named "first" to a file:// destination at storeDir.
 func newFileDestinationVaultik(
 	ctx context.Context, t *testing.T, storeDir string,
 ) (*vaultik.Vaultik, *database.Repositories) {
 	t.Helper()

+	osFs := afero.NewOsFs()
 	tempDir := t.TempDir()
 	dataDir := filepath.Join(tempDir, "src")
-	writeFaultSourceTree(t, afero.NewOsFs(), dataDir)
+	dbPath := filepath.Join(tempDir, "index.sqlite")

-	return openFileDestinationVaultik(ctx, t, storeDir, dataDir,
-		filepath.Join(tempDir, "index.sqlite"))
-}
-
-// openFileDestinationVaultik returns a Vaultik that backs up dataDir to a
-// file:// destination at storeDir, with its local index at dbPath. The
-// index is closed when the test ends.
-func openFileDestinationVaultik(
-	ctx context.Context, t *testing.T, storeDir, dataDir, dbPath string,
-) (*vaultik.Vaultik, *database.Repositories) {
-	t.Helper()
+	writeFaultSourceTree(t, osFs, dataDir)

 	store, err := storage.NewFileStorer(storeDir)
 	require.NoError(t, err)
@@ -58,84 +48,22 @@ func openFileDestinationVaultik(
 	repos := database.NewRepositories(db)
 	cfg := changedFileConfig(dataDir, dbPath)

-	return newBackupVaultik(ctx, cfg, store, repos, db, afero.NewOsFs()), repos
-}
-
-// firstBackup is one backup of the snapshot named "first" to a file://
-// destination. dir holds its source tree in src, its store in store and
-// its closed local index in index/index.sqlite. The first test that
-// needs it makes it, and TestMain removes it. Every test that copies it
-// backs up the same source tree, so no test may change that tree.
-//
-//nolint:gochecknoglobals // made once and shared by the tests that copy it
-var firstBackup struct {
-	once  sync.Once
-	dir   string
-	ready bool
-}
-
-// TestMain removes firstBackup once every test has run.
-func TestMain(m *testing.M) {
-	code := m.Run()
-
-	_ = os.RemoveAll(firstBackup.dir)
-
-	os.Exit(code)
-}
-
-// makeFirstBackup writes the fault source tree and backs it up as the
-// snapshot named "first", all in a new directory that it records in
-// firstBackup.
-func makeFirstBackup(ctx context.Context, t *testing.T) {
-	t.Helper()
-
-	dir, err := os.MkdirTemp("", "vaultik-first-backup-")
-	require.NoError(t, err)
-
-	firstBackup.dir = dir
-	storeDir := filepath.Join(dir, "store")
-	dataDir := filepath.Join(dir, "src")
-	indexDir := filepath.Join(dir, "index")
-
-	require.NoError(t, os.Mkdir(storeDir, 0o750))
-	require.NoError(t, os.Mkdir(indexDir, 0o750))
-	writeFaultSourceTree(t, afero.NewOsFs(), dataDir)
-
-	v, _ := openFileDestinationVaultik(ctx, t, storeDir, dataDir,
-		filepath.Join(indexDir, "index.sqlite"))
-	require.NoError(t, backUp(v, "first"))
-	require.NoError(t, v.DB.Close())
-
-	firstBackup.ready = true
+	return newBackupVaultik(ctx, cfg, store, repos, db, osFs), repos
 }

 // backUpToFileDestination creates the directory storeDir, as a user must
-// before the first backup, and returns a Vaultik whose file://
-// destination there holds a backup of the snapshot named "first". The
-// store and the local index are copies of firstBackup's, which is faster
-// than backing up in every test. The returned Vaultik backs up a source
-// tree shared with every other caller, so no test may change that tree.
-// Everything the returned Vaultik prints after the backup goes to the
-// returned buffer.
+// before the first backup, and backs up the snapshot named "first" to a
+// file:// destination there. Everything the returned Vaultik prints
+// after the backup goes to the returned buffer.
 func backUpToFileDestination(
 	ctx context.Context, t *testing.T, storeDir string,
 ) (*vaultik.Vaultik, *database.Repositories, *bytes.Buffer) {
 	t.Helper()

-	firstBackup.once.Do(func() { makeFirstBackup(ctx, t) })
-	require.True(t, firstBackup.ready, "another test failed to make firstBackup")
-
 	require.NoError(t, os.MkdirAll(storeDir, 0o750))
-	require.NoError(t, os.CopyFS(storeDir,
-		os.DirFS(filepath.Join(firstBackup.dir, "store"))))

-	indexDir := t.TempDir()
-	require.NoError(t, os.CopyFS(indexDir,
-		os.DirFS(filepath.Join(firstBackup.dir, "index"))))
-
-	v, repos := openFileDestinationVaultik(ctx, t, storeDir,
-		filepath.Join(firstBackup.dir, "src"),
-		filepath.Join(indexDir, "index.sqlite"))
+	v, repos := newFileDestinationVaultik(ctx, t, storeDir)
+	require.NoError(t, backUp(v, "first"))

 	out := &bytes.Buffer{}
 	v.Stdout = out
@@ -46,11 +46,11 @@ import (
 //     slice out a few KB — currently the dominant cost in restore.
 //
 // The test deliberately constructs an adversarial scenario: three
-// blobs A/B/C of five 64 KiB chunks each, nine files distributed across
-// them, and path-ordered names that interleave the blobs (a1, b1, c1,
-// a2, b2, c2, …) so naive path-order processing would touch every blob
-// before finishing any of them.
-// localitySource is one 64 KiB fixture file used by the locality test.
+// blobs A/B/C of ~6 MB each, nine files distributed across them, and
+// path-ordered names that interleave the blobs (a1, b1, c1, a2, b2,
+// c2, …) so naive path-order processing would touch every blob before
+// finishing any of them.
+// localitySource is one 1 MiB fixture file used by the locality test.
 type localitySource struct {
 	path string
 	data []byte
@@ -65,10 +65,10 @@ type localityCopy struct {

 // localitySourceBytes is the size of each source file of the locality
 // fixture.
-const localitySourceBytes = 64 * 1024
+const localitySourceBytes = 1024 * 1024

 // buildLocalityFixture writes the adversarial source layout described
-// in TestRestoreLocalityAndReadAt: 15 sources of 64 KiB each (which the
+// in TestRestoreLocalityAndReadAt: 15 sources of 1 MiB each (which the
 // backup packs into 3 blobs of 5 chunks) plus 9 interleaved-name copies
 // (3 per blob group).
 func buildLocalityFixture(
@@ -27,10 +27,10 @@ import (
 // TestRestoreSweeperEvictsBlobs exercises the reference-counted blob
 // disk cache eviction during restore.
 //
-// The scenario: 30 unique 128 KiB random files plus 10 duplicates of
-// those (40 files total, 3.75 MiB of unique content) get backed up with
-// a 1 MiB blob_size_limit. After backup the snapshot's encrypted blobs
-// are restored through Vaultik.Restore, and per-key Get counts on the
+// The scenario: 30 unique 1 MB random files plus 10 duplicates of those
+// (40 files total, 30 MB of unique content) get backed up with a 10 MB
+// blob_size_limit. After backup the snapshot's encrypted blobs are
+// restored through Vaultik.Restore, and per-key Get counts on the
 // storage layer are recorded. Each blob in the snapshot MUST be
 // downloaded exactly once — re-downloads would mean the sweeper either
 // evicted a blob that was still needed (LRU regression) or that the
@@ -41,7 +41,7 @@ import (
 // original AND every duplicate referencing its chunks have been
 // restored.
 // buildSweeperFixture writes the source layout for the sweeper test: 30
-// unique 128 KiB random files plus 10 duplicates of a random subset. The
+// unique 1 MB random files plus 10 duplicates of a random subset. The
 // PRNG seed is fixed so failures are reproducible; the entropy is what
 // matters here — the FastCDC chunker needs realistic-looking data to
 // pick chunk boundaries naturally. Returns the expected content by
@@ -51,7 +51,7 @@ func buildSweeperFixture(
 ) map[string][]byte {
 	t.Helper()

-	const fileSize = 128 * 1024
+	const fileSize = 1 * 1024 * 1024

 	rng := rand.New(rand.NewSource(42)) //nolint:gosec // G404: fixture data only

@@ -159,7 +159,7 @@ func runSweeperBackup(
 	require.NoError(t, err)
 	require.Equal(t, uniqueFiles+duplicateFiles, scanResult.FilesScanned)
 	require.Greater(t, scanResult.BlobsCreated, 1,
-		"3.75 MiB of unique data at 1 MiB blob size should yield multiple blobs")
+		"30 MB of unique data at 10 MB blob size should yield multiple blobs")

 	require.NoError(t, sm.CompleteSnapshot(ctx, snapshotID))
 	require.NoError(t, sm.ExportSnapshotMetadata(ctx, dbPath, snapshotID))
@@ -199,8 +199,8 @@ func TestRestoreSweeperEvictsBlobs(t *testing.T) {

 	expected := buildSweeperFixture(t, fs, dataDir, uniqueFiles, duplicateFiles)

-	chunkSize := int64(16 * 1024)
-	maxBlobSize := int64(1024 * 1024)
+	chunkSize := int64(64 * 1024)
+	maxBlobSize := int64(10 * 1024 * 1024)

 	storer, err := storage.NewFileStorer(storeDir)
 	require.NoError(t, err)
@@ -187,17 +187,6 @@ func (e *listEnv) addRemoteRawTimestamp(
 	t.Helper()

 	remoteKey := snapshot.RemoteSnapshotKey(snapshotID)
-	e.putRemoteManifest(t, remoteKey,
-		encodeRemoteManifest(t, remoteKey, timestamp))
-
-	return remoteKey
-}
-
-// encodeRemoteManifest returns the encoded manifest of a one-blob
-// snapshot whose remote key is remoteKey.
-func encodeRemoteManifest(t *testing.T, remoteKey, timestamp string) []byte {
-	t.Helper()
-
 	manifest := &snapshot.Manifest{
 		// Note: the hashed key, never the human ID. That is precisely
 		// why a remote-only snapshot cannot be named.
@@ -213,17 +202,11 @@ func encodeRemoteManifest(t *testing.T, remoteKey, timestamp string) []byte {
 	data, err := snapshot.EncodeManifest(manifest, 3)
 	require.NoError(t, err)

-	return data
-}
-
-// putRemoteManifest stores an encoded manifest on the destination store
-// under remoteKey.
-func (e *listEnv) putRemoteManifest(t *testing.T, remoteKey string, data []byte) {
-	t.Helper()
-
-	err := e.store.Put(context.Background(),
+	err = e.store.Put(context.Background(),
 		"metadata/"+remoteKey+"/manifest.json.zst", bytes.NewReader(data))
 	require.NoError(t, err)
+
+	return remoteKey
 }

 // Fixtures shared across the listing tests.
@@ -808,17 +791,9 @@ func TestListSnapshots_JSONReportsTruncation(t *testing.T) {

 	timestamp := time.Date(2026, 3, 2, 11, 22, 33, 0, time.UTC)

-	// One past the cap, so exactly one snapshot is omitted. The listing
-	// names a snapshot by the key its manifest is stored under, never by
-	// the manifest's own ID. So all of them share one encoded manifest:
-	// encoding 1001 took seconds.
-	data := encodeRemoteManifest(t,
-		snapshot.RemoteSnapshotKey("otherhost_bulk"),
-		timestamp.Format(time.RFC3339))
-
+	// One past the cap, so exactly one snapshot is omitted.
 	for i := range maxRemoteOnlyRowsForTest + 1 {
-		env.putRemoteManifest(t, snapshot.RemoteSnapshotKey(
-			fmt.Sprintf("otherhost_bulk_%04d", i)), data)
+		env.addRemote(t, fmt.Sprintf("otherhost_bulk_%04d", i), timestamp)
 	}

 	stderr := captureProcessStderr(t, func() {