From e2b8adc635d8ba9837769880dffac2127a163245 Mon Sep 17 00:00:00 2001 From: James Ainslie <42301770+jamesainslie@users.noreply.github.com> Date: Wed, 12 Aug 2026 10:25:01 -0700 Subject: [PATCH] fix(config): coerce config set values and keep config get in lockstep dot config set delivers every value as a string, but most typed setters type-asserted the raw argument, so boolean and integer keys could never be set from the CLI (symlinks.backup, output.verbosity, doctor.*, and friends all failed with "value must be bool"). Route every boolean field through toBool, add toInt for the integer fields, and validate field names before coercing so an unknown field is reported as unknown. config get had drifted the other way: its hand-written getter map knew only 15 keys, so a freshly set key read back as "unknown config key". The getter map now covers the full settable surface, completion derives from it, and a round-trip test walks getValidConfigKeys so the two surfaces cannot drift apart silently again. symlinks.backup_dir, which completion offered but the setter rejected, is now settable too. Fixes #94 --- cmd/dot/config.go | 105 +++++++++++++++++++----------- cmd/dot/config_test.go | 92 ++++++++++++++++++++++++++ internal/config/writer.go | 115 +++++++++++++++++++-------------- internal/config/writer_test.go | 87 +++++++++++++++++++++++++ 4 files changed, 311 insertions(+), 88 deletions(-) diff --git a/cmd/dot/config.go b/cmd/dot/config.go index d7e48631..39430a9a 100644 --- a/cmd/dot/config.go +++ b/cmd/dot/config.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "path/filepath" + "sort" + "strconv" "strings" "github.com/spf13/cobra" @@ -217,49 +219,76 @@ func runConfigGet(key string) error { // getValidConfigKeys returns all valid configuration keys for completion. func getValidConfigKeys() []string { - return []string{ - "directories.package", - "directories.target", - "directories.manifest", - "logging.level", - "logging.format", - "logging.destination", - "symlinks.mode", - "symlinks.backup_suffix", - "symlinks.backup_dir", - "dotfile.prefix", - "dotfile.translate", - "dotfile.package_name_mapping", - "output.format", - "output.color", - "packages.sort_by", + keys := make([]string, 0, len(configGetters)) + for key := range configGetters { + keys = append(keys, key) } + sort.Strings(keys) + return keys +} + +// configGetters maps every gettable key to its renderer. Kept in lockstep +// with the setters in internal/config: every key config set accepts must +// resolve here, or a freshly set key reads back as unknown. +var configGetters = map[string]func(cfg *dot.ExtendedConfig) string{ + "directories.package": func(c *dot.ExtendedConfig) string { return c.Directories.Package }, + "directories.target": func(c *dot.ExtendedConfig) string { return c.Directories.Target }, + "directories.manifest": func(c *dot.ExtendedConfig) string { return c.Directories.Manifest }, + + "logging.level": func(c *dot.ExtendedConfig) string { return c.Logging.Level }, + "logging.format": func(c *dot.ExtendedConfig) string { return c.Logging.Format }, + "logging.destination": func(c *dot.ExtendedConfig) string { return c.Logging.Destination }, + "logging.file": func(c *dot.ExtendedConfig) string { return c.Logging.File }, + + "symlinks.mode": func(c *dot.ExtendedConfig) string { return c.Symlinks.Mode }, + "symlinks.folding": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Symlinks.Folding) }, + "symlinks.overwrite": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Symlinks.Overwrite) }, + "symlinks.backup": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Symlinks.Backup) }, + "symlinks.backup_suffix": func(c *dot.ExtendedConfig) string { return c.Symlinks.BackupSuffix }, + "symlinks.backup_dir": func(c *dot.ExtendedConfig) string { return c.Symlinks.BackupDir }, + + "ignore.use_defaults": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Ignore.UseDefaults) }, + "ignore.patterns": func(c *dot.ExtendedConfig) string { return strings.Join(c.Ignore.Patterns, ",") }, + "ignore.overrides": func(c *dot.ExtendedConfig) string { return strings.Join(c.Ignore.Overrides, ",") }, + + "dotfile.translate": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Dotfile.Translate) }, + "dotfile.prefix": func(c *dot.ExtendedConfig) string { return c.Dotfile.Prefix }, + "dotfile.package_name_mapping": func(c *dot.ExtendedConfig) string { + return strconv.FormatBool(c.Dotfile.PackageNameMapping) + }, + + "output.format": func(c *dot.ExtendedConfig) string { return c.Output.Format }, + "output.color": func(c *dot.ExtendedConfig) string { return c.Output.Color }, + "output.progress": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Output.Progress) }, + "output.verbosity": func(c *dot.ExtendedConfig) string { return strconv.Itoa(c.Output.Verbosity) }, + "output.width": func(c *dot.ExtendedConfig) string { return strconv.Itoa(c.Output.Width) }, + + "operations.dry_run": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Operations.DryRun) }, + "operations.atomic": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Operations.Atomic) }, + "operations.max_parallel": func(c *dot.ExtendedConfig) string { return strconv.Itoa(c.Operations.MaxParallel) }, + + "packages.sort_by": func(c *dot.ExtendedConfig) string { return c.Packages.SortBy }, + "packages.auto_discover": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Packages.AutoDiscover) }, + "packages.validate_names": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Packages.ValidateNames) }, + + "doctor.auto_fix": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Doctor.AutoFix) }, + "doctor.check_manifest": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Doctor.CheckManifest) }, + "doctor.check_broken_links": func(c *dot.ExtendedConfig) string { + return strconv.FormatBool(c.Doctor.CheckBrokenLinks) + }, + "doctor.check_orphaned": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Doctor.CheckOrphaned) }, + "doctor.check_permissions": func(c *dot.ExtendedConfig) string { + return strconv.FormatBool(c.Doctor.CheckPermissions) + }, + + "experimental.parallel": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Experimental.Parallel) }, + "experimental.profiling": func(c *dot.ExtendedConfig) string { return strconv.FormatBool(c.Experimental.Profiling) }, } // getConfigValue retrieves a value from config by key path. func getConfigValue(cfg *dot.ExtendedConfig, key string) (string, error) { - getters := map[string]func() string{ - "directories.package": func() string { return cfg.Directories.Package }, - "directories.target": func() string { return cfg.Directories.Target }, - "directories.manifest": func() string { return cfg.Directories.Manifest }, - "logging.level": func() string { return cfg.Logging.Level }, - "logging.format": func() string { return cfg.Logging.Format }, - "logging.destination": func() string { return cfg.Logging.Destination }, - "symlinks.mode": func() string { return cfg.Symlinks.Mode }, - "symlinks.backup_suffix": func() string { return cfg.Symlinks.BackupSuffix }, - "symlinks.backup_dir": func() string { return cfg.Symlinks.BackupDir }, - "dotfile.prefix": func() string { return cfg.Dotfile.Prefix }, - "dotfile.translate": func() string { return fmt.Sprintf("%t", cfg.Dotfile.Translate) }, - "dotfile.package_name_mapping": func() string { - return fmt.Sprintf("%t", cfg.Dotfile.PackageNameMapping) - }, - "output.format": func() string { return cfg.Output.Format }, - "output.color": func() string { return cfg.Output.Color }, - "packages.sort_by": func() string { return cfg.Packages.SortBy }, - } - - if getter, ok := getters[key]; ok { - return getter(), nil + if getter, ok := configGetters[key]; ok { + return getter(cfg), nil } return "", fmt.Errorf("unknown config key: %s", key) } diff --git a/cmd/dot/config_test.go b/cmd/dot/config_test.go index 7170386b..318a9ead 100644 --- a/cmd/dot/config_test.go +++ b/cmd/dot/config_test.go @@ -726,3 +726,95 @@ func TestRunConfigListCmd(t *testing.T) { err = runConfigListCmd(cmd, []string{}) assert.NoError(t, err) } + +func TestConfigCommand_EveryCompletedKeyRoundTrips(t *testing.T) { + // Shell completion offers getValidConfigKeys() for BOTH get and set, so + // every completed key must survive set -> load -> get (issue #94: the + // getter and setter hand-lists had drifted, so freshly set keys read + // back as unknown). Driving the loop from getValidConfigKeys keeps the + // invariant self-maintaining: a new getter without a matching setter + // (or sample here) fails this test instead of failing users. + samples := map[string]string{ + "directories.package": "/set/pkgs", + "directories.target": "/set/target", + "directories.manifest": "/set/manifest", + "logging.level": "DEBUG", + "logging.format": "json", + "logging.destination": "stdout", + "logging.file": "/set/dot.log", + "symlinks.mode": "absolute", + "symlinks.folding": "false", + "symlinks.overwrite": "true", + "symlinks.backup": "true", + "symlinks.backup_suffix": ".orig", + "symlinks.backup_dir": "/set/backups", + "ignore.use_defaults": "false", + "ignore.patterns": "*.log,cache", + "ignore.overrides": "keep,also", + "dotfile.translate": "false", + "dotfile.prefix": "custom-", + "dotfile.package_name_mapping": "false", + "output.format": "json", + "output.color": "always", + "output.progress": "false", + "output.verbosity": "2", + "output.width": "120", + "operations.dry_run": "true", + "operations.atomic": "false", + "operations.max_parallel": "4", + "packages.sort_by": "links", + "packages.auto_discover": "false", + "packages.validate_names": "false", + "doctor.auto_fix": "true", + "doctor.check_manifest": "false", + "doctor.check_broken_links": "false", + "doctor.check_orphaned": "false", + "doctor.check_permissions": "false", + "experimental.parallel": "true", + "experimental.profiling": "true", + } + + for _, key := range getValidConfigKeys() { + t.Run(key, func(t *testing.T) { + sample, ok := samples[key] + require.True(t, ok, "key %s is offered for completion but has no sample here; add one and make sure config set accepts it", key) + + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + require.NoError(t, writer.Update(key, sample), "config set rejects a key it offers for completion") + + loaded, err := config.LoadExtendedFromFile(configPath) + require.NoError(t, err) + + got, err := getConfigValue(loaded, key) + require.NoError(t, err, "config get rejects a key it offers for completion") + assert.Equal(t, sample, got) + }) + } +} + +func TestConfigCommand_GetRendersTypedValues(t *testing.T) { + cfg := config.DefaultExtended() + cfg.Symlinks.Backup = true + cfg.Output.Verbosity = 2 + cfg.Ignore.Patterns = []string{"*.log", "cache"} + + tests := []struct { + key string + expected string + }{ + {"symlinks.backup", "true"}, + {"output.verbosity", "2"}, + {"ignore.patterns", "*.log,cache"}, + } + + for _, tt := range tests { + t.Run(tt.key, func(t *testing.T) { + value, err := getConfigValue(cfg, tt.key) + require.NoError(t, err) + assert.Equal(t, tt.expected, value) + }) + } +} diff --git a/internal/config/writer.go b/internal/config/writer.go index 8c417f91..13df379f 100644 --- a/internal/config/writer.go +++ b/internal/config/writer.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path/filepath" + "strconv" "strings" "github.com/yaklabco/dot/internal/domain" @@ -212,7 +213,7 @@ func setLoggingValue(cfg *LoggingConfig, field string, value interface{}) error func setSymlinksValue(cfg *SymlinksConfig, field string, value interface{}) error { switch field { - case "mode", "backup_suffix": + case "mode", "backup_suffix", "backup_dir": str, ok := value.(string) if !ok { return fmt.Errorf("symlinks.%s: value must be string", field) @@ -223,12 +224,14 @@ func setSymlinksValue(cfg *SymlinksConfig, field string, value interface{}) erro cfg.Mode = str case "backup_suffix": cfg.BackupSuffix = str + case "backup_dir": + cfg.BackupDir = str } case "folding", "overwrite", "backup": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("symlinks.%s: value must be bool", field) + b, err := toBool(value, "symlinks."+field) + if err != nil { + return err } switch field { @@ -250,9 +253,9 @@ func setSymlinksValue(cfg *SymlinksConfig, field string, value interface{}) erro func setIgnoreValue(cfg *IgnoreConfig, field string, value interface{}) error { switch field { case "use_defaults": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("ignore.%s: value must be bool", field) + b, err := toBool(value, "ignore."+field) + if err != nil { + return err } cfg.UseDefaults = b @@ -330,21 +333,16 @@ func setOutputValue(cfg *OutputConfig, field string, value interface{}) error { } case "progress": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("output.%s: value must be bool", field) + b, err := toBool(value, "output."+field) + if err != nil { + return err } cfg.Progress = b case "verbosity", "width": - var i int - switch v := value.(type) { - case int: - i = v - case float64: - i = int(v) - default: - return fmt.Errorf("output.%s: value must be int", field) + i, err := toInt(value, "output."+field) + if err != nil { + return err } switch field { @@ -364,9 +362,9 @@ func setOutputValue(cfg *OutputConfig, field string, value interface{}) error { func setOperationsValue(cfg *OperationsConfig, field string, value interface{}) error { switch field { case "dry_run", "atomic": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("operations.%s: value must be bool", field) + b, err := toBool(value, "operations."+field) + if err != nil { + return err } switch field { @@ -377,14 +375,9 @@ func setOperationsValue(cfg *OperationsConfig, field string, value interface{}) } case "max_parallel": - var i int - switch v := value.(type) { - case int: - i = v - case float64: - i = int(v) - default: - return fmt.Errorf("operations.%s: value must be int", field) + i, err := toInt(value, "operations."+field) + if err != nil { + return err } cfg.MaxParallel = i @@ -405,9 +398,9 @@ func setPackagesValue(cfg *PackagesConfig, field string, value interface{}) erro cfg.SortBy = str case "auto_discover", "validate_names": - b, ok := value.(bool) - if !ok { - return fmt.Errorf("packages.%s: value must be bool", field) + b, err := toBool(value, "packages."+field) + if err != nil { + return err } switch field { @@ -425,44 +418,48 @@ func setPackagesValue(cfg *PackagesConfig, field string, value interface{}) erro } func setDoctorValue(cfg *DoctorConfig, field string, value interface{}) error { - b, ok := value.(bool) - if !ok { - return fmt.Errorf("doctor.%s: value must be bool", field) - } - + var target *bool switch field { case "auto_fix": - cfg.AutoFix = b + target = &cfg.AutoFix case "check_manifest": - cfg.CheckManifest = b + target = &cfg.CheckManifest case "check_broken_links": - cfg.CheckBrokenLinks = b + target = &cfg.CheckBrokenLinks case "check_orphaned": - cfg.CheckOrphaned = b + target = &cfg.CheckOrphaned case "check_permissions": - cfg.CheckPermissions = b + target = &cfg.CheckPermissions default: return fmt.Errorf("unknown field: doctor.%s", field) } + b, err := toBool(value, "doctor."+field) + if err != nil { + return err + } + *target = b + return nil } func setExperimentalValue(cfg *ExperimentalConfig, field string, value interface{}) error { - b, ok := value.(bool) - if !ok { - return fmt.Errorf("experimental.%s: value must be bool", field) - } - + var target *bool switch field { case "parallel": - cfg.Parallel = b + target = &cfg.Parallel case "profiling": - cfg.Profiling = b + target = &cfg.Profiling default: return fmt.Errorf("unknown field: experimental.%s", field) } + b, err := toBool(value, "experimental."+field) + if err != nil { + return err + } + *target = b + return nil } @@ -506,7 +503,7 @@ func toBool(value interface{}, fieldName string) (bool, error) { case bool: return v, nil case string: - switch strings.ToLower(v) { + switch strings.ToLower(strings.TrimSpace(v)) { case "true": return true, nil case "false": @@ -519,6 +516,24 @@ func toBool(value interface{}, fieldName string) (bool, error) { } } +// toInt converts a value to int, accepting int, float64, and decimal strings. +func toInt(value interface{}, fieldName string) (int, error) { + switch v := value.(type) { + case int: + return v, nil + case float64: + return int(v), nil + case string: + i, err := strconv.Atoi(strings.TrimSpace(v)) + if err != nil { + return 0, fmt.Errorf("%s: value must be an integer, got %q", fieldName, v) + } + return i, nil + default: + return 0, fmt.Errorf("%s: value must be int", fieldName) + } +} + // fileExists checks if a file exists. func fileExists(path string) bool { _, err := os.Stat(path) diff --git a/internal/config/writer_test.go b/internal/config/writer_test.go index 88d38a9d..d9312051 100644 --- a/internal/config/writer_test.go +++ b/internal/config/writer_test.go @@ -249,3 +249,90 @@ func TestWriter_DetectFormat(t *testing.T) { }) } } + +func TestWriter_UpdateCoercesCLIStrings(t *testing.T) { + // dot config set delivers every value as a string; each typed field must + // coerce it rather than type-assert it (issue #94). + tests := []struct { + key string + value string + read func(cfg *config.ExtendedConfig) interface{} + want interface{} + }{ + {"symlinks.folding", "false", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Folding }, false}, + {"symlinks.overwrite", "true", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Overwrite }, true}, + {"symlinks.backup", "true", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Backup }, true}, + {"ignore.use_defaults", "false", func(c *config.ExtendedConfig) interface{} { return c.Ignore.UseDefaults }, false}, + {"dotfile.translate", "false", func(c *config.ExtendedConfig) interface{} { return c.Dotfile.Translate }, false}, + {"dotfile.package_name_mapping", "false", func(c *config.ExtendedConfig) interface{} { return c.Dotfile.PackageNameMapping }, false}, + {"output.progress", "false", func(c *config.ExtendedConfig) interface{} { return c.Output.Progress }, false}, + {"output.verbosity", "2", func(c *config.ExtendedConfig) interface{} { return c.Output.Verbosity }, 2}, + {"output.width", "120", func(c *config.ExtendedConfig) interface{} { return c.Output.Width }, 120}, + {"operations.dry_run", "true", func(c *config.ExtendedConfig) interface{} { return c.Operations.DryRun }, true}, + {"operations.atomic", "false", func(c *config.ExtendedConfig) interface{} { return c.Operations.Atomic }, false}, + {"operations.max_parallel", "4", func(c *config.ExtendedConfig) interface{} { return c.Operations.MaxParallel }, 4}, + {"packages.auto_discover", "false", func(c *config.ExtendedConfig) interface{} { return c.Packages.AutoDiscover }, false}, + {"packages.validate_names", "false", func(c *config.ExtendedConfig) interface{} { return c.Packages.ValidateNames }, false}, + {"doctor.auto_fix", "true", func(c *config.ExtendedConfig) interface{} { return c.Doctor.AutoFix }, true}, + {"doctor.check_manifest", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckManifest }, false}, + {"doctor.check_broken_links", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckBrokenLinks }, false}, + {"doctor.check_orphaned", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckOrphaned }, false}, + {"doctor.check_permissions", "false", func(c *config.ExtendedConfig) interface{} { return c.Doctor.CheckPermissions }, false}, + {"experimental.parallel", "true", func(c *config.ExtendedConfig) interface{} { return c.Experimental.Parallel }, true}, + {"experimental.profiling", "true", func(c *config.ExtendedConfig) interface{} { return c.Experimental.Profiling }, true}, + {"symlinks.backup_dir", "/set/backups", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.BackupDir }, "/set/backups"}, + // A quoting accident must not change the outcome by type. + {"symlinks.folding", " true ", func(c *config.ExtendedConfig) interface{} { return c.Symlinks.Folding }, true}, + {"output.width", " 120 ", func(c *config.ExtendedConfig) interface{} { return c.Output.Width }, 120}, + } + + for _, tt := range tests { + t.Run(tt.key+"="+tt.value, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + require.NoError(t, writer.Update(tt.key, tt.value)) + + loaded, err := config.LoadExtendedFromFile(configPath) + require.NoError(t, err) + assert.Equal(t, tt.want, tt.read(loaded)) + }) + } +} + +func TestWriter_UpdateRejectsUnparsableStrings(t *testing.T) { + tests := []struct { + key string + value string + }{ + {"symlinks.backup", "banana"}, + {"output.verbosity", "loud"}, + {"operations.max_parallel", "many"}, + } + + for _, tt := range tests { + t.Run(tt.key+"="+tt.value, func(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + err := writer.Update(tt.key, tt.value) + require.Error(t, err) + assert.Contains(t, err.Error(), tt.key) + }) + } +} + +func TestWriter_UpdateUnknownFieldNamesTheField(t *testing.T) { + // An unknown field must be reported as unknown, not as a value error. + configPath := filepath.Join(t.TempDir(), "config.yaml") + writer := config.NewWriter(configPath) + require.NoError(t, writer.WriteDefault(config.WriteOptions{Format: "yaml"})) + + for _, key := range []string{"doctor.bogus", "experimental.bogus"} { + err := writer.Update(key, "banana") + require.Error(t, err) + assert.Contains(t, err.Error(), "unknown field: "+key) + } +}