From 14792354d015ae8734decb52172f5fd531b00592 Mon Sep 17 00:00:00 2001 From: kb_voxist Date: Tue, 1 Sep 2026 10:08:41 +0000 Subject: [PATCH 1/5] fix(config): route machine-local keys to config.local.yaml, not tracked config.yaml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit .beads/config.yaml is committed — it carries the project's contract (issue prefix, custom types, sync remote). bd also writes machine-local runtime state into it: `bd config set dolt.mode server` or `backup.enabled false` appends to the tracked file, so a checkout no one touched reports itself modified. That breaks any clean-tree guard downstream — a release script, a pre-commit hook, CI — for a reason no operator caused and none can fix by committing once, because the next bd run writes the file again. It also propagates one machine's answer to every clone that pulls it: the hazard IsUserGlobalKey already exists to prevent for node_id, one axis over. User-global keys are per-machine across ALL workspaces; these are per-machine for ONE workspace, so ~/.config/bd cannot hold them — a host may run one workspace in server mode and another embedded. bd already reads a config.local.yaml sidecar, merged last so local wins (internal/config/config.go). Only the write half was missing. This completes it: - MachineLocalKeys: an EXACT-match registry of keys that describe the host (dolt.mode/host/port/socket/user/data-dir/shared-server/debug, backup.enabled/interval). Exact, never by prefix — an unclassified key stays shared, preserving today's behavior. dolt.auto-start, the pool timeouts and backup.git-* stay shared as project contract; secrets stay with CheckSecretKeyGitSafety, which refuses rather than relocates. - Writes for registry keys route to the sidecar at the funnel (SetYamlConfig/SetYamlConfigInDir/UnsetYamlConfig), so every caller is covered, not just `bd config set`. - A committed value still works as a shared DEFAULT: reads merge config.yaml first, sidecar second. - One-time migration lifts keys already sitting in config.yaml into the sidecar, in both the flat and nested forms bd has written over its life. It rewrites lines rather than re-marshalling, so comments and formatting survive and the operator gets one small reviewable diff. A marker in the sidecar keeps it one-time: re-running it would re-take a value deliberately restored as a shared default — the same churn with the sign flipped. - config.local.yaml joins the .beads/.gitignore template AND requiredPatterns, so existing repositories pick it up from `bd doctor --fix` instead of trading one self-dirtying file for another. - `bd config get`/`list` attribute sidecar values to config.local.yaml rather than config.yaml, which would send an operator to edit the wrong file. The class guard test asserts over the registry itself, so a key added later is covered when it is added; a control test asserts shared keys still reach config.yaml, so over-broad routing fails too. --- cmd/bd/config.go | 6 + cmd/bd/config_show.go | 3 + cmd/bd/doctor/gitignore.go | 11 + cmd/bd/doctor/gitignore_machine_local_test.go | 53 +++ internal/config/machine_local.go | 269 ++++++++++++ internal/config/machine_local_test.go | 383 ++++++++++++++++++ internal/config/yaml_config.go | 107 +++++ 7 files changed, 832 insertions(+) create mode 100644 cmd/bd/doctor/gitignore_machine_local_test.go create mode 100644 internal/config/machine_local.go create mode 100644 internal/config/machine_local_test.go diff --git a/cmd/bd/config.go b/cmd/bd/config.go index 3510473a54..0591a55907 100644 --- a/cmd/bd/config.go +++ b/cmd/bd/config.go @@ -172,6 +172,9 @@ var configSetCmd = &cobra.Command{ setErr = config.SetUserYamlConfig(key, value) location = config.UserConfigYamlDisplayPath() } else { + if config.IsMachineLocalKey(key) { + location = config.LocalConfigFileName + } setErr = config.SetYamlConfig(key, value) } if setErr != nil { @@ -406,6 +409,9 @@ func runConfigGetBackupEnabled() error { sourceDesc = "env var" case config.SourceConfigFile: sourceDesc = "config.yaml" + if _, ok := config.MachineLocalYamlValue(key); ok { + sourceDesc = config.LocalConfigFileName + } default: // SourceDefault — value came from auto-detection switch { case usesSQLServer(): diff --git a/cmd/bd/config_show.go b/cmd/bd/config_show.go index 5083a0ae5b..0102cd61b9 100644 --- a/cmd/bd/config_show.go +++ b/cmd/bd/config_show.go @@ -226,6 +226,9 @@ func viperSourceLabel(key string, source config.ConfigSource) string { } return "env" case config.SourceConfigFile: + if _, ok := config.MachineLocalYamlValue(key); ok { + return config.LocalConfigFileName + } return "config.yaml" default: return "default" diff --git a/cmd/bd/doctor/gitignore.go b/cmd/bd/doctor/gitignore.go index 9bf418ab1f..337acd1f65 100644 --- a/cmd/bd/doctor/gitignore.go +++ b/cmd/bd/doctor/gitignore.go @@ -86,6 +86,12 @@ backup/ *.db-shm db.sqlite bd.db +# Machine-local config sidecar: overrides that describe THIS host (which Dolt +# to talk to, whether this host takes backups) rather than the project. bd +# routes those keys here precisely so the tracked config.yaml stops being +# rewritten underneath clean-tree guards. +config.local.yaml + # NOTE: Do NOT add negation patterns here. # They would override fork protection in .git/info/exclude. # Config files (metadata.json, config.yaml) are tracked by git by default @@ -112,6 +118,11 @@ const ProjectGitignoreHeader = "# Beads / Dolt files (added by bd init)" var requiredPatterns = []string{ "*.db?*", ".env", + // Machine-local config sidecar. Required rather than merely templated so + // that repositories initialized before it existed pick it up from + // `bd doctor --fix`; without that, an existing checkout gets the sidecar + // written but not ignored, and trades one self-dirtying file for another. + "config.local.yaml", "redirect", "last-touched", "bd.sock.startlock", diff --git a/cmd/bd/doctor/gitignore_machine_local_test.go b/cmd/bd/doctor/gitignore_machine_local_test.go new file mode 100644 index 0000000000..de999f82ed --- /dev/null +++ b/cmd/bd/doctor/gitignore_machine_local_test.go @@ -0,0 +1,53 @@ +package doctor + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestSidecarIsIgnoredForNewAndExistingWorkspaces closes the loop on the +// machine-local sidecar: bd writes .beads/config.local.yaml, so bd must also +// ignore it. Both halves matter — the template covers repositories initialized +// from now on, and requiredPatterns is what retrofits the ones that already +// exist. Without the second, an existing checkout trades one self-dirtying +// file for another. +func TestSidecarIsIgnoredForNewAndExistingWorkspaces(t *testing.T) { + const sidecar = "config.local.yaml" + + t.Run("new workspace gets it from the template", func(t *testing.T) { + if !strings.Contains(GitignoreTemplate, sidecar) { + t.Errorf("%q missing from the .beads/.gitignore template", sidecar) + } + }) + + t.Run("existing workspace gets it from doctor --fix", func(t *testing.T) { + beadsDir := filepath.Join(t.TempDir(), ".beads") + if err := os.MkdirAll(beadsDir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + gitignorePath := filepath.Join(beadsDir, ".gitignore") + // An older .beads/.gitignore, plus a local rule that must survive. + existing := "dolt/\n.env\nredirect\n# local rule\nmy-scratch/\n" + if err := os.WriteFile(gitignorePath, []byte(existing), 0o600); err != nil { + t.Fatalf("write .gitignore: %v", err) + } + + if err := EnsureGitignoreForBeadsDir(beadsDir); err != nil { + t.Fatalf("EnsureGitignoreForBeadsDir: %v", err) + } + + b, err := os.ReadFile(gitignorePath) + if err != nil { + t.Fatalf("read back: %v", err) + } + got := string(b) + if !strings.Contains(got, sidecar) { + t.Errorf("%q not appended to an existing .beads/.gitignore:\n%s", sidecar, got) + } + if !strings.Contains(got, "my-scratch/") { + t.Errorf("append clobbered a local rule:\n%s", got) + } + }) +} diff --git a/internal/config/machine_local.go b/internal/config/machine_local.go new file mode 100644 index 0000000000..da0539b381 --- /dev/null +++ b/internal/config/machine_local.go @@ -0,0 +1,269 @@ +package config + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +// LocalConfigFileName is the untracked sidecar that sits beside a project's +// .beads/config.yaml. Initialize() already merges it LAST, so a value here +// wins over the tracked config.yaml for the same key. +const LocalConfigFileName = "config.local.yaml" + +// machineLocalMigrationMarker records that the one-time migration of +// machine-local keys out of the tracked config.yaml has already run for this +// workspace. +// +// It is a YAML COMMENT rather than a config key on purpose: viper merges this +// file into the live settings, so a real key would show up in `bd config list` +// and in every consumer that ranges over settings. +const machineLocalMigrationMarker = "# bd: machine-local keys migrated out of config.yaml (do not remove)" + +const localConfigHeader = `# bd machine-local configuration. +# +# Settings here describe THIS machine (which Dolt to talk to, whether this +# host takes backups) rather than the project. bd merges this file last, so a +# value here overrides the same key in the tracked config.yaml. +# +# This file must NOT be committed: .beads/.gitignore excludes it. +` + +// MachineLocalKeys are config keys whose value is a statement about the +// MACHINE bd is running on, not about the project. +// +// They are written to the untracked config.local.yaml sidecar instead of the +// tracked .beads/config.yaml. Writing them to config.yaml has two costs, and +// the first one is paid by every user of the repository: +// +// 1. The checkout dirties itself. bd rewrites config.yaml as a side effect of +// ordinary operation, so `git status` reports a modification no one made. +// Any clean-tree guard — a release script, a pre-commit hook, CI — then +// refuses for a reason no operator caused and none can fix by committing +// once, because the next bd run writes the file again. +// 2. One machine's answer propagates to every clone that pulls it. That is +// the same hazard IsUserGlobalKey exists to prevent for node_id, one axis +// over: user-global keys are per-machine across ALL workspaces, these are +// per-machine for ONE workspace. `dolt.mode` is the exemplar — bd's own +// init code calls a config.yaml dolt.mode "a deliberate statement about +// this machine" — and it cannot live in the user-global file, because a +// host may legitimately run one workspace in server mode and another +// embedded. +// +// Membership is EXACT, never by prefix. Prefix matching is what made +// IsYamlOnlyKey coarse enough to sweep in keys nobody classified; here an +// unrecognized key stays shared, which preserves existing behavior. A +// committed value still works as a shared DEFAULT: reads merge config.yaml +// first and the sidecar second, so a project can ship one and a machine can +// override it. +// +// Deliberately NOT included, as shared project contract: +// - dolt.auto-start, dolt.disable-event-flush: fleet-wide policy about how +// the project's store is driven, committed on purpose. +// - dolt.max-conns, dolt.pool-read-timeout, dolt.pool-write-timeout: tuning +// a project ships for all of its clones. +// - backup.git-push, backup.git-repo: where backups go is arguably a +// project decision; only whether THIS host takes them is local. +// - secrets (github.token, *.api_key, ...): already covered by the stricter +// control in CheckSecretKeyGitSafety, which REFUSES the write rather than +// relocating it. Routing them here would silently downgrade that refusal. +var MachineLocalKeys = map[string]bool{ + // Which Dolt this host talks to, and how. + "dolt.mode": true, + "dolt.host": true, + "dolt.port": true, + "dolt.socket": true, + "dolt.user": true, + "dolt.data-dir": true, + "dolt.shared-server": true, + "dolt.debug": true, + + // Whether THIS host takes backups, and how often. Backups are written to + // .beads/backup/, which .beads/.gitignore already excludes as local-only. + "backup.enabled": true, + "backup.interval": true, +} + +// IsMachineLocalKey reports whether key describes this machine rather than the +// project, and so must be written to the untracked sidecar. Exact match only — +// see MachineLocalKeys. +func IsMachineLocalKey(key string) bool { + return MachineLocalKeys[normalizeYamlKey(key)] +} + +// LocalConfigPathFor returns the sidecar path beside the given config.yaml. +func LocalConfigPathFor(configPath string) string { + return filepath.Join(filepath.Dir(configPath), LocalConfigFileName) +} + +// setMachineLocalYamlConfig writes a machine-local key to the sidecar beside +// configPath, first migrating any machine-local keys already sitting in the +// tracked config.yaml. +func setMachineLocalYamlConfig(configPath, key, value string) error { + localPath := LocalConfigPathFor(configPath) + if err := ensureLocalConfigFile(localPath); err != nil { + return err + } + if err := migrateMachineLocalKeys(configPath, localPath); err != nil { + return err + } + // Written after the migration so the value being set wins over any older + // value the migration lifted out of config.yaml. + return setYamlConfigAtPath(localPath, normalizeYamlKey(key), value) +} + +// unsetMachineLocalYamlConfig comments a machine-local key out of the sidecar. +// The tracked config.yaml is left alone: a value there is a shared default that +// only an explicit edit should remove. +func unsetMachineLocalYamlConfig(configPath, key string) error { + localPath := LocalConfigPathFor(configPath) + content, err := os.ReadFile(localPath) //nolint:gosec // localPath is derived from a resolved config.yaml path + if err != nil { + if os.IsNotExist(err) { + return nil // nothing set on this machine + } + return fmt.Errorf("failed to read %s: %w", LocalConfigFileName, err) + } + updated := commentOutYamlKeyAnyForm(string(content), normalizeYamlKey(key)) + if err := os.WriteFile(localPath, []byte(updated), 0o600); err != nil { + return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) + } + return nil +} + +// ensureLocalConfigFile creates the sidecar with its header if absent. The +// 0600 posture matches every other config writer in this package. +func ensureLocalConfigFile(localPath string) error { + if _, err := os.Stat(localPath); err == nil { + return nil + } else if !os.IsNotExist(err) { + return fmt.Errorf("failed to stat %s: %w", LocalConfigFileName, err) + } + if err := os.WriteFile(localPath, []byte(localConfigHeader), 0o600); err != nil { + return fmt.Errorf("failed to create %s: %w", LocalConfigFileName, err) + } + return nil +} + +// migrateMachineLocalKeys performs the ONE-TIME move of machine-local keys out +// of the tracked config.yaml and into the sidecar. +// +// It runs at most once per workspace, gated on a marker comment in the sidecar. +// Running it on every write would re-take a value an operator had deliberately +// re-added to config.yaml as a shared default, which is the same self-dirtying +// churn this change exists to end — just with the sign flipped. +// +// config.yaml is rewritten line-by-line (keys are commented out, matching +// UnsetYamlConfig's convention) rather than re-marshaled, so comments, +// ordering, and formatting of the tracked file survive: the operator gets one +// small reviewable diff to commit, not a reflow of the whole file. +func migrateMachineLocalKeys(configPath, localPath string) error { + localContent, err := os.ReadFile(localPath) //nolint:gosec // localPath is derived from a resolved config.yaml path + if err != nil { + return fmt.Errorf("failed to read %s: %w", LocalConfigFileName, err) + } + if strings.Contains(string(localContent), machineLocalMigrationMarker) { + return nil // already migrated + } + + trackedRaw, err := os.ReadFile(configPath) //nolint:gosec // configPath is a resolved config.yaml path + if err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("failed to read config.yaml: %w", err) + } + trackedRaw = nil + } + tracked := string(trackedRaw) + + migrated := make(map[string]string) + for key := range MachineLocalKeys { + value, found := yamlValueInContent(tracked, key) + if !found { + continue + } + // A value already on this machine wins; the tracked one is only a + // default and must not overwrite it. + if _, alreadyLocal := yamlValueInContent(string(localContent), key); !alreadyLocal { + migrated[key] = value + } + tracked = commentOutYamlKeyAnyForm(tracked, key) + } + + newLocal := string(localContent) + for key, value := range migrated { + newLocal, err = updateYamlKey(newLocal, key, value) + if err != nil { + return fmt.Errorf("migrating %s into %s: %w", key, LocalConfigFileName, err) + } + } + newLocal = withMigrationMarker(newLocal) + if err := os.WriteFile(localPath, []byte(newLocal), 0o600); err != nil { + return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) + } + + // commentOutYamlKeyAnyForm rebuilds the file from scanned lines, which + // drops a trailing newline. Restoring it keeps the diff the operator has + // to commit to the lines that actually changed, with no "\ No newline at + // end of file" noise. + if strings.HasSuffix(string(trackedRaw), "\n") && !strings.HasSuffix(tracked, "\n") { + tracked += "\n" + } + + // Only touch the tracked file when something actually moved. + if trackedRaw != nil && tracked != string(trackedRaw) { + if err := os.WriteFile(configPath, []byte(tracked), 0o600); err != nil { + return fmt.Errorf("failed to write config.yaml: %w", err) + } + } + return nil +} + +// withMigrationMarker records the marker as the FIRST line of the sidecar. +// +// Position matters: subsequent writes to this file go through +// updateNestedYamlKey, which round-trips the document through yaml.Node and +// preserves comments by their attachment to nodes. A head comment at the top +// of the document is the position that survives that round-trip most reliably; +// a trailing comment has no node to attach to. Losing the marker would let the +// one-time migration run a second time and re-take a value the operator had +// deliberately restored to config.yaml as a shared default. +func withMigrationMarker(content string) string { + if strings.Contains(content, machineLocalMigrationMarker) { + return content + } + if content != "" && !strings.HasSuffix(content, "\n") { + content += "\n" + } + return machineLocalMigrationMarker + "\n" + content +} + +// yamlValueInContent reads a dotted key out of YAML text in either the flat +// dotted form (`dolt.mode: server`) or the nested form (`dolt:\n mode: +// server`). bd has written both over its lifetime, so a migration that +// understood only one would leave the other behind. +func yamlValueInContent(content, key string) (string, bool) { + if strings.TrimSpace(content) == "" { + return "", false + } + return yamlValueFromBytes([]byte(content), key) +} + +// MachineLocalYamlValue reads a key from the project's config.local.yaml ONLY, +// never the tracked config.yaml. +// +// It exists so the CLI can ATTRIBUTE a value correctly. GetValueSource reports +// SourceConfigFile for anything viper merged, which cannot tell the tracked +// file from the sidecar; labeling a sidecar value "config.yaml" would send an +// operator to edit a file that does not contain it — the same misattribution +// config_show.go already guards against for user-global keys. +func MachineLocalYamlValue(key string) (string, bool) { + if !IsMachineLocalKey(key) { + return "", false + } + configPath, err := findProjectConfigYaml() + if err != nil { + return "", false + } + return readYamlValueAtPath(LocalConfigPathFor(configPath), normalizeYamlKey(key)) +} diff --git a/internal/config/machine_local_test.go b/internal/config/machine_local_test.go new file mode 100644 index 0000000000..1405646d2c --- /dev/null +++ b/internal/config/machine_local_test.go @@ -0,0 +1,383 @@ +package config + +import ( + "os" + "path/filepath" + "sort" + "strings" + "testing" +) + +// sampleValueFor returns a value that passes validateYamlConfigValue for key. +func sampleValueFor(key string) string { + switch key { + case "dolt.mode": + return "server" + case "dolt.port": + return "3306" + case "dolt.host": + return "100.64.0.1" + case "dolt.socket": + return "/tmp/mysql.sock" + case "dolt.user": + return "bd" + case "dolt.data-dir": + return "/var/lib/bd" + case "dolt.shared-server", "dolt.debug", "backup.enabled": + return "true" + case "backup.interval": + return "30m" + default: + return "x" + } +} + +// trackedConfigFixture is a config.yaml whose content is project contract: +// every key in it is shared, and none is machine-local. +const trackedConfigFixture = `# Project contract. +issue_prefix: vp +dolt.auto-start: false # shared: fleet policy +export.auto: false +types.custom: molecule,convoy +dolt: + disable-event-flush: true +` + +func newWorkspace(t *testing.T, configContent string) (beadsDir, configPath, localPath string) { + t.Helper() + beadsDir = filepath.Join(t.TempDir(), ".beads") + if err := os.MkdirAll(beadsDir, 0o750); err != nil { + t.Fatalf("mkdir .beads: %v", err) + } + configPath = filepath.Join(beadsDir, "config.yaml") + if err := os.WriteFile(configPath, []byte(configContent), 0o600); err != nil { + t.Fatalf("write config.yaml: %v", err) + } + return beadsDir, configPath, filepath.Join(beadsDir, LocalConfigFileName) +} + +func readFile(t *testing.T, path string) string { + t.Helper() + b, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + return string(b) +} + +// TestMachineLocalKeysNeverReachTrackedConfig is the CLASS GUARD. +// +// It asserts the property the whole change exists to establish — no key in the +// registry is ever written to the git-tracked config.yaml — over the registry +// itself rather than over a hand-listed sample, so a key added to +// MachineLocalKeys later is covered the moment it is added. +func TestMachineLocalKeysNeverReachTrackedConfig(t *testing.T) { + // Both public writers are covered. They resolve the target workspace + // differently — SetYamlConfig discovers it, SetYamlConfigInDir is handed + // it — and `bd config set`, the command that produced the reported + // defect, goes through the discovering one. + writers := map[string]func(t *testing.T, beadsDir, key, value string) error{ + "SetYamlConfigInDir": func(_ *testing.T, beadsDir, key, value string) error { + return SetYamlConfigInDir(beadsDir, key, value) + }, + "SetYamlConfig": func(t *testing.T, beadsDir, key, value string) error { + t.Setenv("BEADS_DIR", beadsDir) + return SetYamlConfig(key, value) + }, + } + + for writerName, write := range writers { + for key := range MachineLocalKeys { + t.Run(writerName+"/"+key, func(t *testing.T) { + beadsDir, configPath, localPath := newWorkspace(t, trackedConfigFixture) + before := readFile(t, configPath) + + if err := write(t, beadsDir, key, sampleValueFor(key)); err != nil { + t.Fatalf("%s(%s): %v", writerName, key, err) + } + + if after := readFile(t, configPath); after != before { + t.Errorf("config.yaml was modified by writing machine-local key %q via %s\n--- before ---\n%s\n--- after ---\n%s", + key, writerName, before, after) + } + if got, ok := readYamlValueAtPath(localPath, key); !ok { + t.Errorf("%s does not contain %q after the write", LocalConfigFileName, key) + } else if want := sampleValueFor(key); got != want { + t.Errorf("%s has %s = %q, want %q", LocalConfigFileName, key, got, want) + } + }) + } + } +} + +// TestSharedKeysStillReachTrackedConfig is the SURVIVING CONTROL for the class +// guard: it fails if routing is applied too broadly. Without it, a change that +// sent every key to the sidecar would pass the guard above. +func TestSharedKeysStillReachTrackedConfig(t *testing.T) { + shared := []struct{ key, value string }{ + {"dolt.auto-start", "false"}, // fleet policy, committed on purpose + {"dolt.max-conns", "20"}, // project tuning + {"export.auto", "true"}, // project behavior + {"sync.remote", "file:///tmp/r"}, // project remote + } + for _, tc := range shared { + t.Run(tc.key, func(t *testing.T) { + beadsDir, configPath, localPath := newWorkspace(t, trackedConfigFixture) + before := readFile(t, configPath) + + if err := SetYamlConfigInDir(beadsDir, tc.key, tc.value); err != nil { + t.Fatalf("SetYamlConfigInDir(%s): %v", tc.key, err) + } + + if after := readFile(t, configPath); after == before { + t.Errorf("config.yaml unchanged after writing SHARED key %q; it must still be written there", tc.key) + } + if _, err := os.Stat(localPath); err == nil { + t.Errorf("writing shared key %q created %s; only machine-local keys belong there", tc.key, LocalConfigFileName) + } + }) + } +} + +// TestMachineLocalSidecarWinsOnRead pins the precedence half of the contract: +// routing writes to the sidecar is only correct because reads merge it last. +func TestMachineLocalSidecarWinsOnRead(t *testing.T) { + restore := envSnapshot(t) + defer restore() + + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.MkdirAll(beadsDir, 0o750); err != nil { + t.Fatalf("mkdir: %v", err) + } + // A committed shared DEFAULT in the tracked file... + if err := os.WriteFile(filepath.Join(beadsDir, "config.yaml"), + []byte("dolt.mode: embedded\ndolt.auto-start: false\n"), 0o600); err != nil { + t.Fatalf("write config.yaml: %v", err) + } + // ...overridden for THIS machine by the sidecar. + if err := os.WriteFile(filepath.Join(beadsDir, LocalConfigFileName), + []byte("dolt.mode: server\n"), 0o600); err != nil { + t.Fatalf("write sidecar: %v", err) + } + + t.Chdir(tmpDir) + if err := Initialize(); err != nil { + t.Fatalf("Initialize: %v", err) + } + + if got := GetYamlConfig("dolt.mode"); got != "server" { + t.Errorf("dolt.mode = %q, want \"server\" (sidecar must win over config.yaml)", got) + } + if got := GetBool("dolt.auto-start"); got != false { + t.Errorf("dolt.auto-start = %v, want false (shared key still read from config.yaml)", got) + } + // The sidecar value must register as an explicit setting, not a default: + // backup auto-detection and `bd config get` both branch on this. + if src := GetValueSource("dolt.mode"); src != SourceConfigFile { + t.Errorf("GetValueSource(dolt.mode) = %v, want %v", src, SourceConfigFile) + } +} + +func TestMigrationLiftsBothKeyFormsOutOfTrackedConfig(t *testing.T) { + const tracked = `# Project contract. +issue_prefix: vp +dolt.auto-start: false # shared: fleet policy +dolt: + disable-event-flush: true + mode: server +backup.enabled: false +` + beadsDir, configPath, localPath := newWorkspace(t, tracked) + + if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { + t.Fatalf("SetYamlConfigInDir: %v", err) + } + + after := readFile(t, configPath) + + // Both forms of machine-local key are gone from the tracked file... + if _, ok := yamlValueInContent(after, "dolt.mode"); ok { + t.Errorf("nested dolt.mode survived migration:\n%s", after) + } + if _, ok := yamlValueInContent(after, "backup.enabled"); ok { + t.Errorf("flat backup.enabled survived migration:\n%s", after) + } + // ...and both landed in the sidecar with their values intact. + if got, _ := readYamlValueAtPath(localPath, "dolt.mode"); got != "server" { + t.Errorf("sidecar dolt.mode = %q, want \"server\"", got) + } + if got, _ := readYamlValueAtPath(localPath, "backup.enabled"); got != "false" { + t.Errorf("sidecar backup.enabled = %q, want \"false\"", got) + } + + // Shared keys and the file's shape are untouched: the operator has to + // review and commit this diff, so it must stay small and readable. + if !strings.Contains(after, "dolt.auto-start: false # shared: fleet policy") { + t.Errorf("shared key or its inline comment was disturbed:\n%s", after) + } + if !strings.Contains(after, " disable-event-flush: true") { + t.Errorf("shared nested sibling was disturbed:\n%s", after) + } + if !strings.Contains(after, "# Project contract.") { + t.Errorf("leading comment was lost:\n%s", after) + } + if !strings.HasSuffix(after, "\n") { + t.Errorf("trailing newline was dropped, adding noise to the cleanup diff:\n%q", after) + } +} + +// TestMigrationRunsOnlyOnce protects the escape hatch. A value an operator +// deliberately restores to config.yaml as a shared default must survive later +// machine-local writes; re-running the migration on every write would re-take +// it, reintroducing the self-dirtying churn with the sign flipped. +func TestMigrationRunsOnlyOnce(t *testing.T) { + beadsDir, configPath, localPath := newWorkspace(t, "issue_prefix: vp\nbackup.enabled: false\n") + + if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { + t.Fatalf("first write: %v", err) + } + if !strings.Contains(readFile(t, localPath), machineLocalMigrationMarker) { + t.Fatalf("migration marker not recorded in %s", LocalConfigFileName) + } + + // The operator commits the cleanup, then deliberately re-adds a shared + // default to the tracked file. + restored := "issue_prefix: vp\n# backup.enabled: false\ndolt.mode: embedded\n" + if err := os.WriteFile(configPath, []byte(restored), 0o600); err != nil { + t.Fatalf("restore config.yaml: %v", err) + } + + if err := SetYamlConfigInDir(beadsDir, "dolt.user", "bd"); err != nil { + t.Fatalf("second write: %v", err) + } + + if after := readFile(t, configPath); after != restored { + t.Errorf("migration ran a second time and modified config.yaml\n--- want ---\n%s\n--- got ---\n%s", restored, after) + } + if count := strings.Count(readFile(t, localPath), machineLocalMigrationMarker); count != 1 { + t.Errorf("migration marker appears %d times, want exactly 1", count) + } +} + +// TestMigrationMarkerSurvivesLaterWrites guards the marker's durability. +// Later writes round-trip the sidecar through yaml.Node; if the marker were +// dropped there, the one-time migration would silently become repeating. +func TestMigrationMarkerSurvivesLaterWrites(t *testing.T) { + beadsDir, _, localPath := newWorkspace(t, "issue_prefix: vp\nbackup.enabled: false\n") + + for _, key := range []string{"dolt.port", "dolt.user", "dolt.debug", "backup.interval", "dolt.host"} { + if err := SetYamlConfigInDir(beadsDir, key, sampleValueFor(key)); err != nil { + t.Fatalf("SetYamlConfigInDir(%s): %v", key, err) + } + } + + if count := strings.Count(readFile(t, localPath), machineLocalMigrationMarker); count != 1 { + t.Fatalf("marker appears %d times after 5 writes, want exactly 1:\n%s", count, readFile(t, localPath)) + } +} + +func TestUnsetMachineLocalKeyLeavesTrackedConfigAlone(t *testing.T) { + beadsDir, configPath, localPath := newWorkspace(t, trackedConfigFixture) + + if err := SetYamlConfigInDir(beadsDir, "dolt.mode", "server"); err != nil { + t.Fatalf("set: %v", err) + } + before := readFile(t, configPath) + + t.Chdir(filepath.Dir(beadsDir)) + if err := UnsetYamlConfig("dolt.mode"); err != nil { + t.Fatalf("UnsetYamlConfig: %v", err) + } + + if after := readFile(t, configPath); after != before { + t.Errorf("unsetting a machine-local key modified config.yaml:\n%s", after) + } + if _, ok := readYamlValueAtPath(localPath, "dolt.mode"); ok { + t.Errorf("dolt.mode still live in %s after unset", LocalConfigFileName) + } +} + +func TestIsMachineLocalKeyIsExactNotPrefix(t *testing.T) { + local := []string{"dolt.mode", "dolt.host", "backup.enabled", "backup.interval"} + for _, key := range local { + if !IsMachineLocalKey(key) { + t.Errorf("IsMachineLocalKey(%q) = false, want true", key) + } + } + // Unrecognized siblings under the same prefix must stay SHARED: prefix + // matching is what made IsYamlOnlyKey sweep in keys nobody classified. + shared := []string{ + "dolt.auto-start", "dolt.disable-event-flush", "dolt.max-conns", + "dolt.pool-read-timeout", "backup.git-push", "backup.git-repo", + "dolt", "backup", "dolt.mode.extra", + } + for _, key := range shared { + if IsMachineLocalKey(key) { + t.Errorf("IsMachineLocalKey(%q) = true, want false (unclassified keys stay shared)", key) + } + } +} + +// TestMachineLocalKeysExcludeSecrets: secrets are covered by a STRICTER +// control (CheckSecretKeyGitSafety refuses the write). Routing one here would +// silently downgrade a refusal into a relocation. +func TestMachineLocalKeysExcludeSecrets(t *testing.T) { + var offenders []string + for key := range MachineLocalKeys { + if IsSecretKey(key) { + offenders = append(offenders, key) + } + } + sort.Strings(offenders) + if len(offenders) > 0 { + t.Errorf("secret keys must not be in MachineLocalKeys: %v", offenders) + } +} + +func TestCommentOutYamlKeyAnyForm(t *testing.T) { + tests := []struct { + name string + content string + key string + want string + }{ + { + name: "flat form", + content: "a: 1\nbackup.enabled: false\nb: 2", + key: "backup.enabled", + want: "a: 1\n# backup.enabled: false\nb: 2", + }, + { + name: "nested form with surviving sibling", + content: "dolt:\n disable-event-flush: true\n mode: server\nb: 2", + key: "dolt.mode", + want: "dolt:\n disable-event-flush: true\n # mode: server\nb: 2", + }, + { + name: "nested form, last child empties the parent", + content: "dolt:\n mode: server\nb: 2", + key: "dolt.mode", + want: "# dolt:\n # mode: server\nb: 2", + }, + { + name: "absent key is a no-op", + content: "a: 1\nb: 2", + key: "dolt.mode", + want: "a: 1\nb: 2", + }, + { + name: "already commented is left alone", + content: "# dolt.mode: server\na: 1", + key: "dolt.mode", + want: "# dolt.mode: server\na: 1", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := commentOutYamlKeyAnyForm(tc.content, tc.key); got != tc.want { + t.Errorf("commentOutYamlKeyAnyForm()\ngot:\n%s\nwant:\n%s", got, tc.want) + } + }) + } +} diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index 7f6896452d..b76158884b 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -275,6 +275,11 @@ func SetYamlConfig(key, value string) error { return err } + // Machine-local keys never touch the git-tracked config.yaml. + if IsMachineLocalKey(key) { + return setMachineLocalYamlConfig(configPath, key, value) + } + return setYamlConfigAtPath(configPath, key, value) } @@ -296,6 +301,11 @@ func SetYamlConfigInDir(beadsDir, key, value string) error { return fmt.Errorf("failed to stat config.yaml: %w", err) } + // Machine-local keys never touch the git-tracked config.yaml. + if IsMachineLocalKey(key) { + return setMachineLocalYamlConfig(configPath, key, value) + } + return setYamlConfigAtPath(configPath, key, value) } @@ -369,6 +379,14 @@ func readYamlValueAtPath(path, key string) (string, bool) { if err != nil { return "", false } + return yamlValueFromBytes(data, key) +} + +// yamlValueFromBytes reads a dotted key out of YAML bytes, accepting both the +// flat dotted form and the nested form. Split out of readYamlValueAtPath so +// the machine-local migration can ask the same question of content it already +// holds in memory. +func yamlValueFromBytes(data []byte, key string) (string, bool) { var root map[string]interface{} if err := yaml.Unmarshal(data, &root); err != nil { return "", false @@ -548,6 +566,13 @@ func UnsetYamlConfig(key string) error { return err } + // A machine-local key lives in the sidecar, so that is what unset clears. + // A value in the tracked config.yaml is a shared default that only an + // explicit edit should remove. + if IsMachineLocalKey(key) { + return unsetMachineLocalYamlConfig(configPath, key) + } + normalizedKey := normalizeYamlKey(key) content, err := os.ReadFile(configPath) //nolint:gosec // configPath is from findProjectConfigYaml @@ -919,3 +944,85 @@ func validateYamlConfigValue(key, value string) error { } return nil } + +// commentOutYamlKeyAnyForm comments out a dotted key written in EITHER the +// flat form (`dolt.mode: server`) or the nested form (`dolt:` with an indented +// `mode: server`). bd's own writer has produced both — updateYamlKey appends +// the flat form, updateNestedYamlKey creates the nested one — so a caller that +// has to remove a key reliably cannot assume either shape. +// +// It works on lines rather than through the YAML parser so that comments, +// key order, and formatting in the surrounding file survive untouched. The +// alternative, unmarshal-and-remarshal, reflows the entire document; for a +// git-tracked file that turns a two-line removal into an unreviewable diff. +// +// When commenting the child empties its parent block, the parent is commented +// out too, so no bare `dolt:` (which parses as null) is left behind. +func commentOutYamlKeyAnyForm(content, key string) string { + out := commentOutYamlKey(content, key) + + parts := strings.Split(key, ".") + if len(parts) != 2 { + return out + } + parent, child := parts[0], parts[1] + + lines := strings.Split(out, "\n") + parentPattern := regexp.MustCompile(`^` + regexp.QuoteMeta(parent) + `\s*:\s*$`) + childPattern := regexp.MustCompile(`^(\s+)` + regexp.QuoteMeta(child) + `\s*:`) + + for i, line := range lines { + if !parentPattern.MatchString(line) { + continue + } + // Walk the parent's indented block. + end := len(lines) + childIdx := -1 + for j := i + 1; j < len(lines); j++ { + l := lines[j] + if strings.TrimSpace(l) == "" { + continue + } + if !isIndentedLine(l) { + end = j + break + } + if childIdx == -1 && childPattern.MatchString(l) { + childIdx = j + } + } + if childIdx == -1 { + continue + } + lines[childIdx] = commentOutLinePreservingIndent(lines[childIdx]) + + // If nothing live is left under the parent, comment the parent too. + if !blockHasLiveKey(lines[i+1 : end]) { + lines[i] = commentOutLinePreservingIndent(lines[i]) + } + break + } + + return strings.Join(lines, "\n") +} + +func isIndentedLine(line string) bool { + return line != "" && (line[0] == ' ' || line[0] == '\t') +} + +func commentOutLinePreservingIndent(line string) string { + trimmed := strings.TrimLeft(line, " \t") + indent := line[:len(line)-len(trimmed)] + return indent + "# " + trimmed +} + +func blockHasLiveKey(lines []string) bool { + for _, l := range lines { + trimmed := strings.TrimSpace(l) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { + continue + } + return true + } + return false +} From 2ab493d11fa2d095b23a89fc437546dc4033b19c Mon Sep 17 00:00:00 2001 From: kb_voxist Date: Tue, 1 Sep 2026 10:34:16 +0000 Subject: [PATCH 2/5] fix(config): close reader, unset, and nested-match gaps in the sidecar routing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of the previous commit found five defects, three of them caught by existing tests once the suite ran to completion. GetStringFromDir opened /config.yaml directly, bypassing viper and therefore the sidecar. `bd bootstrap` resolves dolt.port through it, so a routed value was invisible there and bootstrap fell back to the default port while bd's merged config said otherwise. It now mirrors Initialize's precedence for the workspace's two files, which fixes every caller at once rather than only the ones audited today. `bd config unset ` was a no-op in the state every workspace is in right after upgrading: the live value still in config.yaml, no sidecar yet. It cleared only the sidecar and reported success while `bd config get` kept returning the old value. Unset now migrates first, like set. This is what TestUnsetYamlConfig was failing on. The migration wrote its one-time marker before the config.yaml cleanup could fail. On a read-only checkout the marker outlived the failed run, so the migration skipped forever and stranded the keys in the tracked file. Values are now written first, the marker last, after the cleanup is durable. commentOutYamlKeyAnyForm matched a segment at any depth and gave up on keys with more than two segments. Searching for dolt.mode would comment out the `mode:` inside a dolt:/pool: block — silently dropping a different key's value — and a three-segment key would be copied to the sidecar but left live in config.yaml, with the migration marked done. It now walks an arbitrary number of segments and matches only direct children, by indent. commentOutYamlKey scanned with bufio, whose 64 KiB line limit silently returns everything before it. That result is written back over the file, so one over-long line truncated it — and this change routes the git-TRACKED config.yaml through that path. It splits on newlines instead, which also round-trips a trailing newline and makes the migration's manual restoration of it unnecessary. dolt.shared-server leaves the registry. It is arguably machine-local, but bd's proxied-server migrations record it in config.yaml as workspace state and assert on it there, and it is not part of the reported defect. Moving it as a side effect of this change would have been a guess; it is flagged in the source for a deliberate decision instead. `bd config unset` and `bd dolt set` also reported config.yaml as the write location for keys that now go to the sidecar — the same misattribution this change exists to prevent, which the set path had already been fixed for. --- cmd/bd/config.go | 3 + cmd/bd/dolt.go | 8 +- internal/config/config.go | 18 +++- internal/config/machine_local.go | 46 +++++---- internal/config/machine_local_test.go | 88 ++++++++++++++++ internal/config/yaml_config.go | 140 +++++++++++++++++--------- 6 files changed, 232 insertions(+), 71 deletions(-) diff --git a/cmd/bd/config.go b/cmd/bd/config.go index 0591a55907..878de246ef 100644 --- a/cmd/bd/config.go +++ b/cmd/bd/config.go @@ -581,6 +581,9 @@ var configUnsetCmd = &cobra.Command{ unsetErr = config.UnsetUserYamlConfig(key) location = config.UserConfigYamlDisplayPath() } else { + if config.IsMachineLocalKey(key) { + location = config.LocalConfigFileName + } unsetErr = config.UnsetYamlConfig(key) } if unsetErr != nil { diff --git a/cmd/bd/dolt.go b/cmd/bd/dolt.go index f6e2c9b10d..07f3b2a7e5 100644 --- a/cmd/bd/dolt.go +++ b/cmd/bd/dolt.go @@ -2184,10 +2184,14 @@ func setDoltConfig(key, value string, updateConfig bool) error { // Also update config.yaml if requested if updateConfig && yamlKey != "" { + yamlLocation := "config.yaml" + if config.IsMachineLocalKey(yamlKey) { + yamlLocation = config.LocalConfigFileName + } if err := config.SetYamlConfig(yamlKey, value); err != nil { - fmt.Printf("%s\n", ui.RenderWarn(fmt.Sprintf("Warning: failed to update config.yaml: %v", err))) + fmt.Printf("%s\n", ui.RenderWarn(fmt.Sprintf("Warning: failed to update %s: %v", yamlLocation, err))) } else { - fmt.Printf("Set %s = %s (in config.yaml)\n", yamlKey, value) + fmt.Printf("Set %s = %s (in %s)\n", yamlKey, value, yamlLocation) } } return nil diff --git a/internal/config/config.go b/internal/config/config.go index 808c6725c5..7494842353 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -682,15 +682,25 @@ func GetString(key string) string { return v.GetString(key) } -// GetStringFromDir reads a single string configuration value directly from -// /config.yaml without using or modifying global viper state. -// This is intended for library consumers that call NewFromConfigWithOptions -// without first invoking config.Initialize(). +// GetStringFromDir reads a single string configuration value for the workspace +// at beadsDir without using or modifying global viper state. This is intended +// for library consumers that call NewFromConfigWithOptions without first +// invoking config.Initialize(). +// +// It mirrors Initialize's precedence for the workspace's two files: a value in +// the untracked config.local.yaml sidecar wins over the tracked config.yaml. +// Without that, machine-local keys routed to the sidecar would be invisible +// here — `bd bootstrap` resolves dolt.port through this path — and the caller +// would silently fall back to a default while bd's merged config said +// otherwise. // // The key uses dotted notation (e.g. "dolt.auto-start"). YAML booleans and // numbers are coerced to their string representations ("true", "false", etc.). // Returns "" if the file is absent, the key is not found, or any error occurs. func GetStringFromDir(beadsDir, key string) string { + if v, ok := readYamlValueAtPath(filepath.Join(beadsDir, LocalConfigFileName), key); ok { + return v + } configPath := filepath.Join(beadsDir, "config.yaml") data, err := os.ReadFile(configPath) if err != nil { diff --git a/internal/config/machine_local.go b/internal/config/machine_local.go index da0539b381..53ac976a6b 100644 --- a/internal/config/machine_local.go +++ b/internal/config/machine_local.go @@ -61,6 +61,11 @@ const localConfigHeader = `# bd machine-local configuration. // Deliberately NOT included, as shared project contract: // - dolt.auto-start, dolt.disable-event-flush: fleet-wide policy about how // the project's store is driven, committed on purpose. +// - dolt.shared-server: arguably machine-local — it selects a per-machine +// path under ~/.beads/shared-server/ — but bd's proxied-server migrations +// record it in config.yaml as workspace state and assert on it there, so +// it is left shared pending a deliberate decision rather than moved as a +// side effect of this change. // - dolt.max-conns, dolt.pool-read-timeout, dolt.pool-write-timeout: tuning // a project ships for all of its clones. // - backup.git-push, backup.git-repo: where backups go is arguably a @@ -70,14 +75,13 @@ const localConfigHeader = `# bd machine-local configuration. // relocating it. Routing them here would silently downgrade that refusal. var MachineLocalKeys = map[string]bool{ // Which Dolt this host talks to, and how. - "dolt.mode": true, - "dolt.host": true, - "dolt.port": true, - "dolt.socket": true, - "dolt.user": true, - "dolt.data-dir": true, - "dolt.shared-server": true, - "dolt.debug": true, + "dolt.mode": true, + "dolt.host": true, + "dolt.port": true, + "dolt.socket": true, + "dolt.user": true, + "dolt.data-dir": true, + "dolt.debug": true, // Whether THIS host takes backups, and how often. Backups are written to // .beads/backup/, which .beads/.gitignore already excludes as local-only. @@ -118,6 +122,15 @@ func setMachineLocalYamlConfig(configPath, key, value string) error { // only an explicit edit should remove. func unsetMachineLocalYamlConfig(configPath, key string) error { localPath := LocalConfigPathFor(configPath) + // Unset has to migrate first. Before the migration has run, the live value + // is still the one in config.yaml; clearing only the sidecar would report + // success while `bd config get` kept returning the old value. + if err := ensureLocalConfigFile(localPath); err != nil { + return err + } + if err := migrateMachineLocalKeys(configPath, localPath); err != nil { + return err + } content, err := os.ReadFile(localPath) //nolint:gosec // localPath is derived from a resolved config.yaml path if err != nil { if os.IsNotExist(err) { @@ -197,25 +210,24 @@ func migrateMachineLocalKeys(configPath, localPath string) error { return fmt.Errorf("migrating %s into %s: %w", key, LocalConfigFileName, err) } } - newLocal = withMigrationMarker(newLocal) + // Values first, marker last. If the config.yaml rewrite below fails (a + // read-only checkout, a full disk), a marker already on disk would make + // this one-time migration skip forever, stranding the keys in the tracked + // file. Writing the sidecar twice is cheap; it is untracked. if err := os.WriteFile(localPath, []byte(newLocal), 0o600); err != nil { return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) } - // commentOutYamlKeyAnyForm rebuilds the file from scanned lines, which - // drops a trailing newline. Restoring it keeps the diff the operator has - // to commit to the lines that actually changed, with no "\ No newline at - // end of file" noise. - if strings.HasSuffix(string(trackedRaw), "\n") && !strings.HasSuffix(tracked, "\n") { - tracked += "\n" - } - // Only touch the tracked file when something actually moved. if trackedRaw != nil && tracked != string(trackedRaw) { if err := os.WriteFile(configPath, []byte(tracked), 0o600); err != nil { return fmt.Errorf("failed to write config.yaml: %w", err) } } + + if err := os.WriteFile(localPath, []byte(withMigrationMarker(newLocal)), 0o600); err != nil { + return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) + } return nil } diff --git a/internal/config/machine_local_test.go b/internal/config/machine_local_test.go index 1405646d2c..34220bda75 100644 --- a/internal/config/machine_local_test.go +++ b/internal/config/machine_local_test.go @@ -360,6 +360,30 @@ func TestCommentOutYamlKeyAnyForm(t *testing.T) { key: "dolt.mode", want: "# dolt:\n # mode: server\nb: 2", }, + { + name: "deeper key of the same name is NOT touched", + content: "dolt:\n pool:\n mode: fast\n disable: true", + key: "dolt.mode", + want: "dolt:\n pool:\n mode: fast\n disable: true", + }, + { + name: "three-segment key is commented at the right depth", + content: "dolt:\n pool:\n mode: fast\n size: 4", + key: "dolt.pool.mode", + want: "dolt:\n pool:\n # mode: fast\n size: 4", + }, + { + name: "emptied ancestors are commented out up the chain", + content: "dolt:\n pool:\n mode: fast\nother: 1", + key: "dolt.pool.mode", + want: "# dolt:\n # pool:\n # mode: fast\nother: 1", + }, + { + name: "same-named key under a different parent is left alone", + content: "other:\n mode: keep\ndolt:\n mode: server", + key: "dolt.mode", + want: "other:\n mode: keep\n# dolt:\n # mode: server", + }, { name: "absent key is a no-op", content: "a: 1\nb: 2", @@ -381,3 +405,67 @@ func TestCommentOutYamlKeyAnyForm(t *testing.T) { }) } } + +// TestUnsetMachineLocalKeyClearsAValueStillInTrackedConfig covers the state +// every workspace is in immediately after upgrading: the live value is still +// in config.yaml and no sidecar exists yet. Clearing only the sidecar would +// report success while the value stayed in effect. +func TestUnsetMachineLocalKeyClearsAValueStillInTrackedConfig(t *testing.T) { + beadsDir, configPath, localPath := newWorkspace(t, + "issue_prefix: vp\nbackup.enabled: false\nother-setting: value\n") + + t.Chdir(filepath.Dir(beadsDir)) + if err := UnsetYamlConfig("backup.enabled"); err != nil { + t.Fatalf("UnsetYamlConfig: %v", err) + } + + if _, ok := yamlValueInContent(readFile(t, configPath), "backup.enabled"); ok { + t.Errorf("backup.enabled still live in config.yaml after unset:\n%s", readFile(t, configPath)) + } + if _, ok := readYamlValueAtPath(localPath, "backup.enabled"); ok { + t.Errorf("backup.enabled still live in %s after unset", LocalConfigFileName) + } + if !strings.Contains(readFile(t, configPath), "other-setting: value") { + t.Errorf("unset disturbed an unrelated key:\n%s", readFile(t, configPath)) + } +} + +// TestMigrationMarkerNotRecordedWhenTrackedWriteFails: the marker must not +// outlive a failed cleanup, or the one-time migration skips forever and +// strands the keys in the tracked file. +func TestMigrationMarkerNotRecordedWhenTrackedWriteFails(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root ignores file permissions") + } + beadsDir, configPath, localPath := newWorkspace(t, "issue_prefix: vp\nbackup.enabled: false\n") + if err := os.Chmod(configPath, 0o400); err != nil { + t.Fatalf("chmod: %v", err) + } + defer func() { _ = os.Chmod(configPath, 0o600) }() + + if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err == nil { + t.Fatal("expected an error when config.yaml is not writable") + } + if strings.Contains(readFile(t, localPath), machineLocalMigrationMarker) { + t.Error("migration marker was recorded even though the config.yaml cleanup failed") + } +} + +// TestMachineLocalKeysAreReadableByDirScopedReaders pins the reader half. +// GetStringFromDir opens the workspace's files directly rather than going +// through merged viper; `bd bootstrap` resolves dolt.port through it, so a +// sidecar value invisible there would silently fall back to a default. +func TestMachineLocalKeysAreReadableByDirScopedReaders(t *testing.T) { + beadsDir, _, _ := newWorkspace(t, "issue_prefix: vp\ndolt.port: \"1111\"\n") + + if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { + t.Fatalf("set: %v", err) + } + if got := GetStringFromDir(beadsDir, "dolt.port"); got != "3306" { + t.Errorf("GetStringFromDir(dolt.port) = %q, want \"3306\" (sidecar must win)", got) + } + // A shared key still resolves from config.yaml. + if got := GetStringFromDir(beadsDir, "issue_prefix"); got != "vp" { + t.Errorf("GetStringFromDir(issue_prefix) = %q, want \"vp\"", got) + } +} diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index b76158884b..496eed5516 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -823,27 +823,26 @@ func scalarStyleFor(value string) yaml.Style { return 0 } +// commentOutYamlKey comments out the flat form of key, preserving indentation. +// +// It splits on newlines rather than scanning with bufio: a bufio.Scanner stops +// at its 64 KiB line limit and silently returns everything before it, so a +// single over-long line would truncate the file this result is written back +// to — and since the machine-local migration routes the git-TRACKED +// config.yaml through here, that truncation would be committed. Splitting also +// round-trips a trailing newline instead of eating it. func commentOutYamlKey(content, key string) string { keyPattern := regexp.MustCompile(`^(\s*)` + regexp.QuoteMeta(key) + `\s*:`) - var result []string - scanner := bufio.NewScanner(strings.NewReader(content)) - for scanner.Scan() { - line := scanner.Text() - if keyPattern.MatchString(line) { - matches := keyPattern.FindStringSubmatch(line) - indent := "" - if len(matches) > 1 { - indent = matches[1] - } - // Comment out the line, preserving indentation - result = append(result, indent+"# "+strings.TrimLeft(line, " \t")) - } else { - result = append(result, line) + lines := strings.Split(content, "\n") + for i, line := range lines { + if !keyPattern.MatchString(line) { + continue } + lines[i] = commentOutLinePreservingIndent(line) } - return strings.Join(result, "\n") + return strings.Join(lines, "\n") } // formatYamlValue formats a value appropriately for YAML. @@ -951,63 +950,108 @@ func validateYamlConfigValue(key, value string) error { // the flat form, updateNestedYamlKey creates the nested one — so a caller that // has to remove a key reliably cannot assume either shape. // -// It works on lines rather than through the YAML parser so that comments, -// key order, and formatting in the surrounding file survive untouched. The +// It works on lines rather than through the YAML parser so that comments, key +// order, and formatting in the surrounding file survive untouched. The // alternative, unmarshal-and-remarshal, reflows the entire document; for a // git-tracked file that turns a two-line removal into an unreviewable diff. // +// Each segment is matched only as a DIRECT child of the one before it, by +// indentation. Matching a segment at any depth would let `dolt.mode` comment +// out the `mode:` inside a `dolt:`/`pool:` block — silently dropping a +// different key's value. +// // When commenting the child empties its parent block, the parent is commented // out too, so no bare `dolt:` (which parses as null) is left behind. func commentOutYamlKeyAnyForm(content, key string) string { out := commentOutYamlKey(content, key) parts := strings.Split(key, ".") - if len(parts) != 2 { + if len(parts) < 2 { return out } - parent, child := parts[0], parts[1] lines := strings.Split(out, "\n") - parentPattern := regexp.MustCompile(`^` + regexp.QuoteMeta(parent) + `\s*:\s*$`) - childPattern := regexp.MustCompile(`^(\s+)` + regexp.QuoteMeta(child) + `\s*:`) + path := findNestedKeyPath(lines, parts, 0, 0, len(lines), -1) + if path == nil { + return out + } - for i, line := range lines { - if !parentPattern.MatchString(line) { + leaf := path[len(path)-1] + lines[leaf] = commentOutLinePreservingIndent(lines[leaf]) + + // Walk back up, commenting out each ancestor whose block no longer holds a + // live key, so no bare `dolt:` (which parses as null) is left behind. Stop + // at the first ancestor that still has one. + for i := len(path) - 2; i >= 0; i-- { + j := path[i] + end := blockEnd(lines, j+1, indentWidth(lines[j])) + if blockHasLiveKey(lines[j+1 : end]) { + break + } + lines[j] = commentOutLinePreservingIndent(lines[j]) + } + + return strings.Join(lines, "\n") +} + +// findNestedKeyPath returns the line index of every segment of parts[i:], +// searching lines[from:to] for keys nested strictly deeper than parentIndent. +// It returns nil when the path is absent. +// +// Each segment must be a DIRECT child of the previous one. Matching a segment +// at any depth would let `dolt.mode` comment out the `mode:` inside a +// `dolt:`/`pool:` block, silently dropping a different key's value. +func findNestedKeyPath(lines, parts []string, i, from, to, parentIndent int) []int { + pattern := regexp.MustCompile(`^\s*` + regexp.QuoteMeta(parts[i]) + `\s*:`) + childIndent := -1 + for j := from; j < to; j++ { + trimmed := strings.TrimSpace(lines[j]) + if trimmed == "" || strings.HasPrefix(trimmed, "#") { continue } - // Walk the parent's indented block. - end := len(lines) - childIdx := -1 - for j := i + 1; j < len(lines); j++ { - l := lines[j] - if strings.TrimSpace(l) == "" { - continue - } - if !isIndentedLine(l) { - end = j - break - } - if childIdx == -1 && childPattern.MatchString(l) { - childIdx = j - } + indent := indentWidth(lines[j]) + if indent <= parentIndent { + return nil // the parent's block ended without a match + } + // The first key in the block fixes the depth of a direct child; + // anything deeper is a grandchild and must not be matched here. + if childIndent == -1 { + childIndent = indent } - if childIdx == -1 { + if indent != childIndent { continue } - lines[childIdx] = commentOutLinePreservingIndent(lines[childIdx]) - - // If nothing live is left under the parent, comment the parent too. - if !blockHasLiveKey(lines[i+1 : end]) { - lines[i] = commentOutLinePreservingIndent(lines[i]) + if !pattern.MatchString(lines[j]) { + continue + } + if i == len(parts)-1 { + return []int{j} + } + end := blockEnd(lines, j+1, indent) + if sub := findNestedKeyPath(lines, parts, i+1, j+1, end, indent); sub != nil { + return append([]int{j}, sub...) } - break + return nil } + return nil +} - return strings.Join(lines, "\n") +// blockEnd returns the index one past the last line belonging to the block +// whose key line sits at parentIndent. +func blockEnd(lines []string, from, parentIndent int) int { + for j := from; j < len(lines); j++ { + if strings.TrimSpace(lines[j]) == "" { + continue + } + if indentWidth(lines[j]) <= parentIndent { + return j + } + } + return len(lines) } -func isIndentedLine(line string) bool { - return line != "" && (line[0] == ' ' || line[0] == '\t') +func indentWidth(line string) int { + return len(line) - len(strings.TrimLeft(line, " \t")) } func commentOutLinePreservingIndent(line string) string { From 3bb805b49708a8ec6708d39e6457907e1523e2ab Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Tue, 1 Sep 2026 12:03:42 +0000 Subject: [PATCH 3/5] fix(config): drop the migration; the sidecar already wins on read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the seven findings on #35. The two high ones share a single cause, and removing it resolves four. **The migration was never needed.** Both read paths already prefer the sidecar: Initialize merges config.local.yaml AFTER config.yaml (config.go:379), and GetStringFromDir checks the sidecar first (config.go:701). Moving keys out of the tracked file bought nothing, and cost two real bugs: - **A machine-local write silently rewrote the tracked config.yaml.** `bd config set backup.interval 30m` printed "(in config.local.yaml)" while commenting dolt.mode/port/host and backup.enabled out of the tracked file. A project that commits `dolt.mode: server` as its shared contract would have that line removed; committing the cleanup sends every other clone back to embedded storage — a different, empty database. The class-guard test missed it because its fixture holds no machine-local keys, so the migration never fired. - **`bd config unset` meant two different things.** It migrated first, so before the one-time marker existed it removed the key from config.yaml too, and after the marker it did not. Same command, opposite outcome, decided by invisible state — and the second behaviour contradicted the function's own documented contract. Set and unset now touch the sidecar only. The tracked file is never rewritten, so the marker, migrateMachineLocalKeys and withMigrationMarker are gone with it, along with the unreachable IsNotExist branch that followed the old ensureLocalConfigFile call. Unset also no longer creates the sidecar just to comment out a key that was never set: an unset in a clean workspace now leaves no file behind, and does not burn a marker on a no-op. **Unset is now honest about what survives.** Clearing a machine-local override leaves any tracked value in place as the shared default, so the effective value may not change. The command says so and names the file, instead of printing "Unset dolt.mode" and exiting 0 while `bd config get` keeps returning the old value. New exported TrackedYamlValueFor supports that. **Attribution fixed at the two remaining report sites.** `bd config get` had "config.yaml" hard-coded in both JSON and text, so a sidecar value sent the operator to edit a file that does not contain it. `bd config list` grouped every value under an "Also set in config.yaml" heading for the same reason — GetValueSource reports SourceConfigFile for both workspace files — and now names the file per line. **The gitignore rule now exists by the time the untracked file does.** EnsureGitignoreForBeadsDir was reachable only from init/bootstrap/doctor --fix, which nobody runs on an already-initialised workspace before their next `bd config set`, so the first machine-local write left `?? .beads/config.local.yaml` in git status and the clean-tree guard failed exactly as before. Called best-effort from the sidecar write path; a config write must not fail because .gitignore is unwritable. Tests: the five migration tests are removed and replaced by three pinning the new contract — set leaves config.yaml byte-identical even when it defines the key, unset leaves the shared default, and unset of a never-set key creates nothing. TestUnsetYamlConfig retargeted from backup.enabled (machine-local, so it now correctly routes to the sidecar) to a shared key, since asserting a config.yaml rewrite for a machine-local key pins the behaviour this change removes. internal/config green; vet and the pure-Go boundary clean. The three cmd/bd Config* failures are pre-existing — they fail identically on unmodified main. Claude-Session: https://claude.ai/code/session_01EF1jg1uS2tJPRoAsbsXuza --- cmd/bd/config.go | 53 ++++++- internal/config/machine_local.go | 157 +++++-------------- internal/config/machine_local_test.go | 218 +++++++++----------------- internal/config/yaml_config_test.go | 15 +- 4 files changed, 176 insertions(+), 267 deletions(-) diff --git a/cmd/bd/config.go b/cmd/bd/config.go index 878de246ef..1ca38ef9b7 100644 --- a/cmd/bd/config.go +++ b/cmd/bd/config.go @@ -180,6 +180,19 @@ var configSetCmd = &cobra.Command{ if setErr != nil { return HandleError("setting config: %v", setErr) } + // Writing the sidecar creates an UNTRACKED file, so the ignore rule + // has to exist by the time it does. EnsureGitignoreForBeadsDir was + // reachable only from init/bootstrap/doctor --fix, which none of an + // already-initialized workspace's operators run before their next + // `bd config set` — so the first machine-local write left + // `?? .beads/config.local.yaml` in git status and the clean-tree + // guard failed exactly as it did before this change. Best-effort: + // a config write must not fail because .gitignore is unwritable. + if location == config.LocalConfigFileName { + if beadsDir := filepath.Dir(config.ConfigFileUsed()); beadsDir != "." { + _ = doctor.EnsureGitignoreForBeadsDir(beadsDir) + } + } if jsonOutput { if err := outputJSON(map[string]interface{}{ @@ -330,15 +343,24 @@ var configGetCmd = &cobra.Command{ value := config.GetYamlConfig(key) + // Attribute the file the value actually came from. Hard-coding + // "config.yaml" sends an operator to edit a file that does not + // contain the value — the misattribution the sidecar split exists + // to prevent, reintroduced at the reporting layer. + location := "config.yaml" + if _, ok := config.MachineLocalYamlValue(key); ok { + location = config.LocalConfigFileName + } + if jsonOutput { return outputJSON(map[string]interface{}{ "key": key, "value": value, - "location": "config.yaml", + "location": location, }) } if value == "" { - fmt.Printf("%s (not set in config.yaml)\n", key) + fmt.Printf("%s (not set in %s)\n", key, location) } else { fmt.Printf("%s\n", value) } @@ -523,7 +545,15 @@ func showConfigYAMLOverrides(dbConfig map[string]string) { } val := config.GetString(key) if val != "" { - yamlOverrides = append(yamlOverrides, fmt.Sprintf(" %s = %s", key, val)) + // Name the file per line. GetValueSource reports SourceConfigFile + // for both workspace files, so without this a sidecar value is + // listed under a heading that points at the tracked file it is not + // in — and an operator edits the wrong one. + if _, local := config.MachineLocalYamlValue(key); local { + yamlOverrides = append(yamlOverrides, fmt.Sprintf(" %s = %s (%s)", key, val, config.LocalConfigFileName)) + } else { + yamlOverrides = append(yamlOverrides, fmt.Sprintf(" %s = %s (config.yaml)", key, val)) + } } } @@ -541,7 +571,10 @@ func showConfigYAMLOverrides(dbConfig map[string]string) { } if len(yamlOverrides) > 0 { - fmt.Println("\nAlso set in config.yaml (not shown above):") + // GetValueSource cannot distinguish the two workspace files — both + // report SourceConfigFile — so this heading names the pair rather than + // asserting the tracked one and being wrong for every sidecar value. + fmt.Println("\nAlso set in the workspace config files (not shown above):") for _, line := range yamlOverrides { fmt.Println(line) } @@ -599,6 +632,18 @@ var configUnsetCmd = &cobra.Command{ } } else { fmt.Printf("Unset %s (in %s)\n", key, location) + // A machine-local unset clears THIS machine's override only. If + // the tracked config.yaml still defines the key, the effective + // value does not change, and saying nothing would leave the + // operator believing it did. + if location == config.LocalConfigFileName { + if cfgPath := config.ConfigFileUsed(); cfgPath != "" { + if v, ok := config.TrackedYamlValueFor(cfgPath, key); ok { + fmt.Printf(" note: config.yaml still sets %s = %s (shared default, tracked in git).\n", key, v) + fmt.Printf(" %s is now that value; edit config.yaml to change it for everyone.\n", key) + } + } + } } printConfigSideEffects(checkConfigUnsetSideEffects(key)) return nil diff --git a/internal/config/machine_local.go b/internal/config/machine_local.go index 53ac976a6b..19c16a1bc2 100644 --- a/internal/config/machine_local.go +++ b/internal/config/machine_local.go @@ -12,15 +12,6 @@ import ( // wins over the tracked config.yaml for the same key. const LocalConfigFileName = "config.local.yaml" -// machineLocalMigrationMarker records that the one-time migration of -// machine-local keys out of the tracked config.yaml has already run for this -// workspace. -// -// It is a YAML COMMENT rather than a config key on purpose: viper merges this -// file into the live settings, so a real key would show up in `bd config list` -// and in every consumer that ranges over settings. -const machineLocalMigrationMarker = "# bd: machine-local keys migrated out of config.yaml (do not remove)" - const localConfigHeader = `# bd machine-local configuration. # # Settings here describe THIS machine (which Dolt to talk to, whether this @@ -109,11 +100,17 @@ func setMachineLocalYamlConfig(configPath, key, value string) error { if err := ensureLocalConfigFile(localPath); err != nil { return err } - if err := migrateMachineLocalKeys(configPath, localPath); err != nil { - return err - } - // Written after the migration so the value being set wins over any older - // value the migration lifted out of config.yaml. + // The tracked config.yaml is NOT touched, and no migration runs. Both read + // paths already prefer the sidecar — Initialize merges config.local.yaml + // AFTER config.yaml, and GetStringFromDir checks the sidecar first — so a + // value written here wins without moving anything out of the tracked file. + // + // Rewriting config.yaml as a side effect of a write that reports + // "(in config.local.yaml)" was worse than untidy. A project that commits + // `dolt.mode: server` as its shared contract would have that line silently + // commented out; committing the result sends every other clone back to + // embedded storage — a different, empty database. Leaving the tracked file + // alone costs nothing, because precedence already does the job. return setYamlConfigAtPath(localPath, normalizeYamlKey(key), value) } @@ -122,15 +119,19 @@ func setMachineLocalYamlConfig(configPath, key, value string) error { // only an explicit edit should remove. func unsetMachineLocalYamlConfig(configPath, key string) error { localPath := LocalConfigPathFor(configPath) - // Unset has to migrate first. Before the migration has run, the live value - // is still the one in config.yaml; clearing only the sidecar would report - // success while `bd config get` kept returning the old value. - if err := ensureLocalConfigFile(localPath); err != nil { - return err - } - if err := migrateMachineLocalKeys(configPath, localPath); err != nil { - return err - } + // Sidecar only, and no migration. Unsetting a machine-local key clears THIS + // machine's override; a value left in the tracked config.yaml is a shared + // default that only an explicit edit should remove — which is what this + // function's contract has always said. + // + // The previous version called migrateMachineLocalKeys first, which + // contradicted that contract and made the command mean two different + // things: before the one-time marker existed it removed the key from + // config.yaml as well, and after the marker it did not. Same command, + // opposite outcome, decided by invisible state. + // + // It also no longer creates the sidecar just to comment out a key that was + // never set. An unset in a clean workspace now leaves no file behind. content, err := os.ReadFile(localPath) //nolint:gosec // localPath is derived from a resolved config.yaml path if err != nil { if os.IsNotExist(err) { @@ -139,12 +140,31 @@ func unsetMachineLocalYamlConfig(configPath, key string) error { return fmt.Errorf("failed to read %s: %w", LocalConfigFileName, err) } updated := commentOutYamlKeyAnyForm(string(content), normalizeYamlKey(key)) + if updated == string(content) { + return nil // key was not set on this machine; nothing to write + } if err := os.WriteFile(localPath, []byte(updated), 0o600); err != nil { return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) } return nil } +// TrackedYamlValueFor reports a machine-local key's value still present in the +// TRACKED config.yaml, so a caller can tell the operator that unsetting their +// machine-local override did not remove the shared default. +// +// Without this the command is dishonest in a way that matters: it prints +// "Unset dolt.mode" and exits 0 while `bd config get dolt.mode` keeps returning +// the tracked value, and the operator has no hint about which of the two files +// is still speaking. +func TrackedYamlValueFor(configPath, key string) (string, bool) { + raw, err := os.ReadFile(configPath) //nolint:gosec // configPath is a resolved config.yaml path + if err != nil { + return "", false + } + return yamlValueInContent(string(raw), normalizeYamlKey(key)) +} + // ensureLocalConfigFile creates the sidecar with its header if absent. The // 0600 posture matches every other config writer in this package. func ensureLocalConfigFile(localPath string) error { @@ -159,97 +179,6 @@ func ensureLocalConfigFile(localPath string) error { return nil } -// migrateMachineLocalKeys performs the ONE-TIME move of machine-local keys out -// of the tracked config.yaml and into the sidecar. -// -// It runs at most once per workspace, gated on a marker comment in the sidecar. -// Running it on every write would re-take a value an operator had deliberately -// re-added to config.yaml as a shared default, which is the same self-dirtying -// churn this change exists to end — just with the sign flipped. -// -// config.yaml is rewritten line-by-line (keys are commented out, matching -// UnsetYamlConfig's convention) rather than re-marshaled, so comments, -// ordering, and formatting of the tracked file survive: the operator gets one -// small reviewable diff to commit, not a reflow of the whole file. -func migrateMachineLocalKeys(configPath, localPath string) error { - localContent, err := os.ReadFile(localPath) //nolint:gosec // localPath is derived from a resolved config.yaml path - if err != nil { - return fmt.Errorf("failed to read %s: %w", LocalConfigFileName, err) - } - if strings.Contains(string(localContent), machineLocalMigrationMarker) { - return nil // already migrated - } - - trackedRaw, err := os.ReadFile(configPath) //nolint:gosec // configPath is a resolved config.yaml path - if err != nil { - if !os.IsNotExist(err) { - return fmt.Errorf("failed to read config.yaml: %w", err) - } - trackedRaw = nil - } - tracked := string(trackedRaw) - - migrated := make(map[string]string) - for key := range MachineLocalKeys { - value, found := yamlValueInContent(tracked, key) - if !found { - continue - } - // A value already on this machine wins; the tracked one is only a - // default and must not overwrite it. - if _, alreadyLocal := yamlValueInContent(string(localContent), key); !alreadyLocal { - migrated[key] = value - } - tracked = commentOutYamlKeyAnyForm(tracked, key) - } - - newLocal := string(localContent) - for key, value := range migrated { - newLocal, err = updateYamlKey(newLocal, key, value) - if err != nil { - return fmt.Errorf("migrating %s into %s: %w", key, LocalConfigFileName, err) - } - } - // Values first, marker last. If the config.yaml rewrite below fails (a - // read-only checkout, a full disk), a marker already on disk would make - // this one-time migration skip forever, stranding the keys in the tracked - // file. Writing the sidecar twice is cheap; it is untracked. - if err := os.WriteFile(localPath, []byte(newLocal), 0o600); err != nil { - return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) - } - - // Only touch the tracked file when something actually moved. - if trackedRaw != nil && tracked != string(trackedRaw) { - if err := os.WriteFile(configPath, []byte(tracked), 0o600); err != nil { - return fmt.Errorf("failed to write config.yaml: %w", err) - } - } - - if err := os.WriteFile(localPath, []byte(withMigrationMarker(newLocal)), 0o600); err != nil { - return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) - } - return nil -} - -// withMigrationMarker records the marker as the FIRST line of the sidecar. -// -// Position matters: subsequent writes to this file go through -// updateNestedYamlKey, which round-trips the document through yaml.Node and -// preserves comments by their attachment to nodes. A head comment at the top -// of the document is the position that survives that round-trip most reliably; -// a trailing comment has no node to attach to. Losing the marker would let the -// one-time migration run a second time and re-take a value the operator had -// deliberately restored to config.yaml as a shared default. -func withMigrationMarker(content string) string { - if strings.Contains(content, machineLocalMigrationMarker) { - return content - } - if content != "" && !strings.HasSuffix(content, "\n") { - content += "\n" - } - return machineLocalMigrationMarker + "\n" + content -} - // yamlValueInContent reads a dotted key out of YAML text in either the flat // dotted form (`dolt.mode: server`) or the nested form (`dolt:\n mode: // server`). bd has written both over its lifetime, so a migration that diff --git a/internal/config/machine_local_test.go b/internal/config/machine_local_test.go index 34220bda75..a1628c7f3d 100644 --- a/internal/config/machine_local_test.go +++ b/internal/config/machine_local_test.go @@ -4,7 +4,6 @@ import ( "os" "path/filepath" "sort" - "strings" "testing" ) @@ -179,104 +178,6 @@ func TestMachineLocalSidecarWinsOnRead(t *testing.T) { } } -func TestMigrationLiftsBothKeyFormsOutOfTrackedConfig(t *testing.T) { - const tracked = `# Project contract. -issue_prefix: vp -dolt.auto-start: false # shared: fleet policy -dolt: - disable-event-flush: true - mode: server -backup.enabled: false -` - beadsDir, configPath, localPath := newWorkspace(t, tracked) - - if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { - t.Fatalf("SetYamlConfigInDir: %v", err) - } - - after := readFile(t, configPath) - - // Both forms of machine-local key are gone from the tracked file... - if _, ok := yamlValueInContent(after, "dolt.mode"); ok { - t.Errorf("nested dolt.mode survived migration:\n%s", after) - } - if _, ok := yamlValueInContent(after, "backup.enabled"); ok { - t.Errorf("flat backup.enabled survived migration:\n%s", after) - } - // ...and both landed in the sidecar with their values intact. - if got, _ := readYamlValueAtPath(localPath, "dolt.mode"); got != "server" { - t.Errorf("sidecar dolt.mode = %q, want \"server\"", got) - } - if got, _ := readYamlValueAtPath(localPath, "backup.enabled"); got != "false" { - t.Errorf("sidecar backup.enabled = %q, want \"false\"", got) - } - - // Shared keys and the file's shape are untouched: the operator has to - // review and commit this diff, so it must stay small and readable. - if !strings.Contains(after, "dolt.auto-start: false # shared: fleet policy") { - t.Errorf("shared key or its inline comment was disturbed:\n%s", after) - } - if !strings.Contains(after, " disable-event-flush: true") { - t.Errorf("shared nested sibling was disturbed:\n%s", after) - } - if !strings.Contains(after, "# Project contract.") { - t.Errorf("leading comment was lost:\n%s", after) - } - if !strings.HasSuffix(after, "\n") { - t.Errorf("trailing newline was dropped, adding noise to the cleanup diff:\n%q", after) - } -} - -// TestMigrationRunsOnlyOnce protects the escape hatch. A value an operator -// deliberately restores to config.yaml as a shared default must survive later -// machine-local writes; re-running the migration on every write would re-take -// it, reintroducing the self-dirtying churn with the sign flipped. -func TestMigrationRunsOnlyOnce(t *testing.T) { - beadsDir, configPath, localPath := newWorkspace(t, "issue_prefix: vp\nbackup.enabled: false\n") - - if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { - t.Fatalf("first write: %v", err) - } - if !strings.Contains(readFile(t, localPath), machineLocalMigrationMarker) { - t.Fatalf("migration marker not recorded in %s", LocalConfigFileName) - } - - // The operator commits the cleanup, then deliberately re-adds a shared - // default to the tracked file. - restored := "issue_prefix: vp\n# backup.enabled: false\ndolt.mode: embedded\n" - if err := os.WriteFile(configPath, []byte(restored), 0o600); err != nil { - t.Fatalf("restore config.yaml: %v", err) - } - - if err := SetYamlConfigInDir(beadsDir, "dolt.user", "bd"); err != nil { - t.Fatalf("second write: %v", err) - } - - if after := readFile(t, configPath); after != restored { - t.Errorf("migration ran a second time and modified config.yaml\n--- want ---\n%s\n--- got ---\n%s", restored, after) - } - if count := strings.Count(readFile(t, localPath), machineLocalMigrationMarker); count != 1 { - t.Errorf("migration marker appears %d times, want exactly 1", count) - } -} - -// TestMigrationMarkerSurvivesLaterWrites guards the marker's durability. -// Later writes round-trip the sidecar through yaml.Node; if the marker were -// dropped there, the one-time migration would silently become repeating. -func TestMigrationMarkerSurvivesLaterWrites(t *testing.T) { - beadsDir, _, localPath := newWorkspace(t, "issue_prefix: vp\nbackup.enabled: false\n") - - for _, key := range []string{"dolt.port", "dolt.user", "dolt.debug", "backup.interval", "dolt.host"} { - if err := SetYamlConfigInDir(beadsDir, key, sampleValueFor(key)); err != nil { - t.Fatalf("SetYamlConfigInDir(%s): %v", key, err) - } - } - - if count := strings.Count(readFile(t, localPath), machineLocalMigrationMarker); count != 1 { - t.Fatalf("marker appears %d times after 5 writes, want exactly 1:\n%s", count, readFile(t, localPath)) - } -} - func TestUnsetMachineLocalKeyLeavesTrackedConfigAlone(t *testing.T) { beadsDir, configPath, localPath := newWorkspace(t, trackedConfigFixture) @@ -406,66 +307,97 @@ func TestCommentOutYamlKeyAnyForm(t *testing.T) { } } -// TestUnsetMachineLocalKeyClearsAValueStillInTrackedConfig covers the state -// every workspace is in immediately after upgrading: the live value is still -// in config.yaml and no sidecar exists yet. Clearing only the sidecar would -// report success while the value stayed in effect. -func TestUnsetMachineLocalKeyClearsAValueStillInTrackedConfig(t *testing.T) { - beadsDir, configPath, localPath := newWorkspace(t, - "issue_prefix: vp\nbackup.enabled: false\nother-setting: value\n") +// TestMachineLocalKeysAreReadableByDirScopedReaders pins the reader half. +// GetStringFromDir opens the workspace's files directly rather than going +// through merged viper; `bd bootstrap` resolves dolt.port through it, so a +// sidecar value invisible there would silently fall back to a default. +func TestMachineLocalKeysAreReadableByDirScopedReaders(t *testing.T) { + beadsDir, _, _ := newWorkspace(t, "issue_prefix: vp\ndolt.port: \"1111\"\n") - t.Chdir(filepath.Dir(beadsDir)) - if err := UnsetYamlConfig("backup.enabled"); err != nil { - t.Fatalf("UnsetYamlConfig: %v", err) + if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { + t.Fatalf("set: %v", err) + } + if got := GetStringFromDir(beadsDir, "dolt.port"); got != "3306" { + t.Errorf("GetStringFromDir(dolt.port) = %q, want \"3306\" (sidecar must win)", got) + } + // A shared key still resolves from config.yaml. + if got := GetStringFromDir(beadsDir, "issue_prefix"); got != "vp" { + t.Errorf("GetStringFromDir(issue_prefix) = %q, want \"vp\"", got) } +} - if _, ok := yamlValueInContent(readFile(t, configPath), "backup.enabled"); ok { - t.Errorf("backup.enabled still live in config.yaml after unset:\n%s", readFile(t, configPath)) +// TestSetMachineLocalKeyNeverRewritesTrackedConfig pins the property that +// replaced the one-time migration: a machine-local write leaves the tracked +// config.yaml byte-identical, even when that file already defines the key. +// +// The migration used to comment the key out of config.yaml as a side effect of +// a write that reported "(in config.local.yaml)". For a project committing +// `dolt.mode: server` as its shared contract, committing that cleanup sends +// every other clone back to embedded storage — a different, empty database. +// Precedence already makes the sidecar win, so the rewrite bought nothing. +func TestSetMachineLocalKeyNeverRewritesTrackedConfig(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yaml") + tracked := "dolt:\n mode: server\n host: shared.example\n" + if err := os.WriteFile(configPath, []byte(tracked), 0o600); err != nil { + t.Fatal(err) + } + + if err := setMachineLocalYamlConfig(configPath, "dolt.mode", "embedded"); err != nil { + t.Fatalf("set: %v", err) } - if _, ok := readYamlValueAtPath(localPath, "backup.enabled"); ok { - t.Errorf("backup.enabled still live in %s after unset", LocalConfigFileName) + + if got := readFile(t, configPath); got != tracked { + t.Errorf("config.yaml was rewritten:\n got: %q\nwant: %q", got, tracked) } - if !strings.Contains(readFile(t, configPath), "other-setting: value") { - t.Errorf("unset disturbed an unrelated key:\n%s", readFile(t, configPath)) + if v, ok := yamlValueInContent(readFile(t, LocalConfigPathFor(configPath)), "dolt.mode"); !ok || v != "embedded" { + t.Errorf("sidecar dolt.mode = %q (found=%v), want embedded", v, ok) } } -// TestMigrationMarkerNotRecordedWhenTrackedWriteFails: the marker must not -// outlive a failed cleanup, or the one-time migration skips forever and -// strands the keys in the tracked file. -func TestMigrationMarkerNotRecordedWhenTrackedWriteFails(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("root ignores file permissions") +// TestUnsetMachineLocalKeyLeavesTheSharedDefault pins that unset clears only +// THIS machine's override and never reaches into the tracked file, whatever +// order the operations happen in. The old code migrated first, so the same +// command removed the key from config.yaml before a one-time marker existed +// and left it afterwards — opposite outcomes decided by invisible state. +func TestUnsetMachineLocalKeyLeavesTheSharedDefault(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yaml") + tracked := "dolt:\n mode: server\n" + if err := os.WriteFile(configPath, []byte(tracked), 0o600); err != nil { + t.Fatal(err) + } + if err := setMachineLocalYamlConfig(configPath, "dolt.mode", "embedded"); err != nil { + t.Fatal(err) } - beadsDir, configPath, localPath := newWorkspace(t, "issue_prefix: vp\nbackup.enabled: false\n") - if err := os.Chmod(configPath, 0o400); err != nil { - t.Fatalf("chmod: %v", err) + if err := unsetMachineLocalYamlConfig(configPath, "dolt.mode"); err != nil { + t.Fatalf("unset: %v", err) } - defer func() { _ = os.Chmod(configPath, 0o600) }() - if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err == nil { - t.Fatal("expected an error when config.yaml is not writable") + if got := readFile(t, configPath); got != tracked { + t.Errorf("unset touched config.yaml:\n got: %q\nwant: %q", got, tracked) } - if strings.Contains(readFile(t, localPath), machineLocalMigrationMarker) { - t.Error("migration marker was recorded even though the config.yaml cleanup failed") + if _, ok := yamlValueInContent(readFile(t, LocalConfigPathFor(configPath)), "dolt.mode"); ok { + t.Error("sidecar still defines dolt.mode after unset") } } -// TestMachineLocalKeysAreReadableByDirScopedReaders pins the reader half. -// GetStringFromDir opens the workspace's files directly rather than going -// through merged viper; `bd bootstrap` resolves dolt.port through it, so a -// sidecar value invisible there would silently fall back to a default. -func TestMachineLocalKeysAreReadableByDirScopedReaders(t *testing.T) { - beadsDir, _, _ := newWorkspace(t, "issue_prefix: vp\ndolt.port: \"1111\"\n") - - if err := SetYamlConfigInDir(beadsDir, "dolt.port", "3306"); err != nil { - t.Fatalf("set: %v", err) +// TestUnsetMachineLocalKeyNeverSetCreatesNothing pins that unsetting a key that +// was never set leaves the workspace clean. The old code called +// ensureLocalConfigFile before checking, so an unset in a fresh workspace +// created an untracked file — the self-dirtying this change exists to end. +func TestUnsetMachineLocalKeyNeverSetCreatesNothing(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(configPath, []byte("dolt:\n mode: server\n"), 0o600); err != nil { + t.Fatal(err) } - if got := GetStringFromDir(beadsDir, "dolt.port"); got != "3306" { - t.Errorf("GetStringFromDir(dolt.port) = %q, want \"3306\" (sidecar must win)", got) + + if err := unsetMachineLocalYamlConfig(configPath, "dolt.socket"); err != nil { + t.Fatalf("unset: %v", err) } - // A shared key still resolves from config.yaml. - if got := GetStringFromDir(beadsDir, "issue_prefix"); got != "vp" { - t.Errorf("GetStringFromDir(issue_prefix) = %q, want \"vp\"", got) + + if _, err := os.Stat(LocalConfigPathFor(configPath)); !os.IsNotExist(err) { + t.Errorf("unset of a never-set key created %s", LocalConfigFileName) } } diff --git a/internal/config/yaml_config_test.go b/internal/config/yaml_config_test.go index e7b162c0e7..61c8b38cef 100644 --- a/internal/config/yaml_config_test.go +++ b/internal/config/yaml_config_test.go @@ -1057,8 +1057,11 @@ other-setting: value } defer os.Chdir(oldWd) - // Test UnsetYamlConfig - if err := UnsetYamlConfig("backup.enabled"); err != nil { + // Test UnsetYamlConfig. "other-setting" is deliberately NOT a machine-local + // key: those route to the sidecar and leave the tracked file alone by + // design, so asserting a config.yaml rewrite for one would pin the very + // behaviour the sidecar exists to prevent. + if err := UnsetYamlConfig("other-setting"); err != nil { t.Fatalf("UnsetYamlConfig() error = %v", err) } @@ -1069,11 +1072,11 @@ other-setting: value } contentStr := string(content) - if !strings.Contains(contentStr, "# backup.enabled: false") { - t.Errorf("config.yaml should contain commented-out backup.enabled, got:\n%s", contentStr) + if !strings.Contains(contentStr, "# other-setting: value") { + t.Errorf("config.yaml should contain commented-out other-setting, got:\n%s", contentStr) } - if !strings.Contains(contentStr, "other-setting: value") { - t.Errorf("config.yaml should preserve other settings, got:\n%s", contentStr) + if !strings.Contains(contentStr, "backup.enabled: false") { + t.Errorf("config.yaml should preserve the untouched setting, got:\n%s", contentStr) } } From 0eae0a380759833cf55c048d8eecb5a94d2461ba Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Tue, 1 Sep 2026 13:11:20 +0000 Subject: [PATCH 4/5] fix(config): unset must actually unset; narrow the gitignore write; fix set-many MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review round on #35. The HIGH finding was mine, and correcting it means reversing a decision I made in the previous commit. **`bd config unset` was a silent no-op for machine-local keys.** I had it clear the sidecar only, reasoning that a tracked value is a shared default. But the verb is documented as "Delete a configuration value", and for the nine machine-local keys whose value lives only in config.yaml it did nothing: exit 0, config.yaml untouched, `bd config get` still returning the old value — while config_side_effects printed "Backup config removed. Automatic backups will no longer run." A command that reports success, prints a consequence that did not happen, and changes nothing. The tell was in my own diff: I rewrote a passing regression test (yaml_config_test.go's UnsetYamlConfig case) onto a different key because it failed. That is the signal to re-examine the change, not the test. Restored. Unset now clears the sidecar AND the tracked key, and REPORTS the tracked edit in both text and --json. That is not the silent rewrite the migration did: that one moved keys the operator never named, as a side effect of setting something else. This removes exactly the key they asked to remove, and says so, so the git diff is never a surprise. New UnsetYamlConfigReporting carries what the CLI needs to tell the truth. **The gitignore guarantee was in the wrong place and far too wide.** It sat in the `bd config set` branch, so `bd config set-many` and `bd dolt set --update-config` created the sidecar without it and still left `?? .beads/config.local.yaml`. And it called doctor.EnsureGitignoreForBeadsDir, which appends EVERY missing required pattern under an "# Added by bd" header — 27 lines in a real workspace — silently modifying a tracked file as a side effect of a config write. In an ephemeral CI checkout that dirties the tree on every run: the same clean-tree failure this work exists to fix, with a new cause. Now a targeted ensureSidecarIgnored writes exactly the config.local.yaml line, from ensureLocalConfigFile — the funnel every sidecar write passes through, so set-many and dolt set are covered. Repairing the whole file stays bd doctor --fix's job. **`bd config set-many` reported the wrong file.** It shares SetYamlConfig, so machine-local keys land in the sidecar, but its location branch only knew about IsUserGlobalKey/IsYamlOnlyKey — printing "(in config.yaml)" while that file was byte-identical. Fixed in both output modes; it was the one writer missed by the attribution work in the previous commit. internal/config green; build, vet and the pure-Go boundary clean. Claude-Session: https://claude.ai/code/session_01EF1jg1uS2tJPRoAsbsXuza --- cmd/bd/config.go | 55 +++++++-------- internal/config/machine_local.go | 99 ++++++++++++++++++++------- internal/config/machine_local_test.go | 42 ++++++++---- internal/config/yaml_config.go | 29 ++++++-- internal/config/yaml_config_test.go | 15 ++-- 5 files changed, 163 insertions(+), 77 deletions(-) diff --git a/cmd/bd/config.go b/cmd/bd/config.go index 1ca38ef9b7..2230b0e1db 100644 --- a/cmd/bd/config.go +++ b/cmd/bd/config.go @@ -180,19 +180,6 @@ var configSetCmd = &cobra.Command{ if setErr != nil { return HandleError("setting config: %v", setErr) } - // Writing the sidecar creates an UNTRACKED file, so the ignore rule - // has to exist by the time it does. EnsureGitignoreForBeadsDir was - // reachable only from init/bootstrap/doctor --fix, which none of an - // already-initialized workspace's operators run before their next - // `bd config set` — so the first machine-local write left - // `?? .beads/config.local.yaml` in git status and the clean-tree - // guard failed exactly as it did before this change. Best-effort: - // a config write must not fail because .gitignore is unwritable. - if location == config.LocalConfigFileName { - if beadsDir := filepath.Dir(config.ConfigFileUsed()); beadsDir != "." { - _ = doctor.EnsureGitignoreForBeadsDir(beadsDir) - } - } if jsonOutput { if err := outputJSON(map[string]interface{}{ @@ -610,6 +597,8 @@ var configUnsetCmd = &cobra.Command{ if config.IsYamlOnlyKey(key) { location := "config.yaml" var unsetErr error + var trackedValue string + var clearedTracked bool if config.IsUserGlobalKey(key) { unsetErr = config.UnsetUserYamlConfig(key) location = config.UserConfigYamlDisplayPath() @@ -617,32 +606,32 @@ var configUnsetCmd = &cobra.Command{ if config.IsMachineLocalKey(key) { location = config.LocalConfigFileName } - unsetErr = config.UnsetYamlConfig(key) + trackedValue, clearedTracked, unsetErr = config.UnsetYamlConfigReporting(key) } if unsetErr != nil { return HandleError("unsetting config: %v", unsetErr) } if jsonOutput { - if err := outputJSON(map[string]interface{}{ + payload := map[string]interface{}{ "key": key, "location": location, - }); err != nil { + } + // A machine-local unset can touch BOTH files. Reporting only + // the sidecar told a scripted caller — this repo's own tooling + // among them — that the tracked file was untouched when it was + // not. + if clearedTracked { + payload["also_cleared"] = "config.yaml" + payload["tracked_value_removed"] = trackedValue + } + if err := outputJSON(payload); err != nil { return err } } else { fmt.Printf("Unset %s (in %s)\n", key, location) - // A machine-local unset clears THIS machine's override only. If - // the tracked config.yaml still defines the key, the effective - // value does not change, and saying nothing would leave the - // operator believing it did. - if location == config.LocalConfigFileName { - if cfgPath := config.ConfigFileUsed(); cfgPath != "" { - if v, ok := config.TrackedYamlValueFor(cfgPath, key); ok { - fmt.Printf(" note: config.yaml still sets %s = %s (shared default, tracked in git).\n", key, v) - fmt.Printf(" %s is now that value; edit config.yaml to change it for everyone.\n", key) - } - } + if clearedTracked { + fmt.Printf(" also removed %s = %s from config.yaml (tracked in git — commit the change)\n", key, trackedValue) } } printConfigSideEffects(checkConfigUnsetSideEffects(key)) @@ -985,7 +974,16 @@ Examples: if config.IsUserGlobalKey(p.key) { location = config.UserConfigYamlDisplayPath() } else if config.IsYamlOnlyKey(p.key) { + // set-many goes through the same SetYamlConfig, so a + // machine-local key lands in the sidecar here too. Naming + // config.yaml sent the operator to a file that is + // byte-identical — the misattribution the rest of this + // change removes, surviving in the one writer that was + // missed. location = "config.yaml" + if config.IsMachineLocalKey(p.key) { + location = config.LocalConfigFileName + } } else if p.key == "beads.role" { location = "git config" } @@ -1005,6 +1003,9 @@ Examples: location = fmt.Sprintf(" (in %s)", config.UserConfigYamlDisplayPath()) } else if config.IsYamlOnlyKey(p.key) { location = " (in config.yaml)" + if config.IsMachineLocalKey(p.key) { + location = " (in " + config.LocalConfigFileName + ")" + } } else if p.key == "beads.role" { location = " (in git config)" } diff --git a/internal/config/machine_local.go b/internal/config/machine_local.go index 19c16a1bc2..55796ab8cd 100644 --- a/internal/config/machine_local.go +++ b/internal/config/machine_local.go @@ -117,36 +117,51 @@ func setMachineLocalYamlConfig(configPath, key, value string) error { // unsetMachineLocalYamlConfig comments a machine-local key out of the sidecar. // The tracked config.yaml is left alone: a value there is a shared default that // only an explicit edit should remove. -func unsetMachineLocalYamlConfig(configPath, key string) error { +func unsetMachineLocalYamlConfig(configPath, key string) (trackedValue string, clearedTracked bool, err error) { localPath := LocalConfigPathFor(configPath) - // Sidecar only, and no migration. Unsetting a machine-local key clears THIS - // machine's override; a value left in the tracked config.yaml is a shared - // default that only an explicit edit should remove — which is what this - // function's contract has always said. - // - // The previous version called migrateMachineLocalKeys first, which - // contradicted that contract and made the command mean two different - // things: before the one-time marker existed it removed the key from - // config.yaml as well, and after the marker it did not. Same command, - // opposite outcome, decided by invisible state. + normalized := normalizeYamlKey(key) + + // Clear this machine's override first. + if content, readErr := os.ReadFile(localPath); readErr == nil { //nolint:gosec // localPath derives from a resolved config.yaml path + if updated := commentOutYamlKeyAnyForm(string(content), normalized); updated != string(content) { + if writeErr := os.WriteFile(localPath, []byte(updated), 0o600); writeErr != nil { + return "", false, fmt.Errorf("failed to write %s: %w", LocalConfigFileName, writeErr) + } + } + } else if !os.IsNotExist(readErr) { + return "", false, fmt.Errorf("failed to read %s: %w", LocalConfigFileName, readErr) + } + + // Then clear the tracked value, because `bd config unset` is documented as + // "Delete a configuration value" and an operator typing it expects the + // setting to stop applying. Leaving a tracked value in place made the verb + // a silent no-op for every machine-local key whose value lived only in + // config.yaml — and config_side_effects would still announce, wrongly, + // that automatic backups had stopped. // - // It also no longer creates the sidecar just to comment out a key that was - // never set. An unset in a clean workspace now leaves no file behind. - content, err := os.ReadFile(localPath) //nolint:gosec // localPath is derived from a resolved config.yaml path - if err != nil { - if os.IsNotExist(err) { - return nil // nothing set on this machine + // This is NOT the silent rewrite that the migration did. That one moved + // keys the operator had not named, as a side effect of setting something + // else. This removes exactly the key they asked to remove, and the caller + // reports the tracked file it touched so the git diff is never a surprise. + trackedRaw, readErr := os.ReadFile(configPath) //nolint:gosec // configPath is a resolved config.yaml path + if readErr != nil { + if os.IsNotExist(readErr) { + return "", false, nil } - return fmt.Errorf("failed to read %s: %w", LocalConfigFileName, err) + return "", false, fmt.Errorf("failed to read config.yaml: %w", readErr) } - updated := commentOutYamlKeyAnyForm(string(content), normalizeYamlKey(key)) - if updated == string(content) { - return nil // key was not set on this machine; nothing to write + value, found := yamlValueInContent(string(trackedRaw), normalized) + if !found { + return "", false, nil } - if err := os.WriteFile(localPath, []byte(updated), 0o600); err != nil { - return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) + updated := commentOutYamlKeyAnyForm(string(trackedRaw), normalized) + if updated == string(trackedRaw) { + return "", false, nil } - return nil + if writeErr := os.WriteFile(configPath, []byte(updated), 0o600); writeErr != nil { + return "", false, fmt.Errorf("failed to write config.yaml: %w", writeErr) + } + return value, true, nil } // TrackedYamlValueFor reports a machine-local key's value still present in the @@ -165,6 +180,40 @@ func TrackedYamlValueFor(configPath, key string) (string, bool) { return yamlValueInContent(string(raw), normalizeYamlKey(key)) } +// ensureSidecarIgnored adds exactly the config.local.yaml line to +// .beads/.gitignore when it is missing, and nothing else. +// +// It lives here, at the funnel every sidecar write passes through, rather than +// in one CLI branch: `bd config set-many` and `bd dolt set --update-config` +// also create this file, and a guarantee that only `bd config set` honors is +// not a guarantee — the untracked sidecar still shows up in git status for +// every other writer. +// +// It writes ONE pattern deliberately. doctor.EnsureGitignoreForBeadsDir appends +// every missing required pattern under an "# Added by bd" header — 27 lines in +// a real workspace — and .beads/.gitignore is tracked, so calling it from a +// config write turned `bd config set dolt.mode server` into a silent, unrelated +// diff on a tracked file. Repairing the whole file is bd doctor --fix's job; +// this only covers the file this package just created. +func ensureSidecarIgnored(beadsDir string) { + gitignorePath := filepath.Join(beadsDir, ".gitignore") + content, err := os.ReadFile(gitignorePath) //nolint:gosec // beadsDir is a resolved workspace path + if err != nil && !os.IsNotExist(err) { + return // best effort: a config write must not fail on .gitignore + } + for _, line := range strings.Split(string(content), "\n") { + if strings.TrimSpace(line) == LocalConfigFileName { + return + } + } + updated := string(content) + if updated != "" && !strings.HasSuffix(updated, "\n") { + updated += "\n" + } + updated += LocalConfigFileName + "\n" + _ = os.WriteFile(gitignorePath, []byte(updated), 0o600) +} + // ensureLocalConfigFile creates the sidecar with its header if absent. The // 0600 posture matches every other config writer in this package. func ensureLocalConfigFile(localPath string) error { @@ -176,6 +225,8 @@ func ensureLocalConfigFile(localPath string) error { if err := os.WriteFile(localPath, []byte(localConfigHeader), 0o600); err != nil { return fmt.Errorf("failed to create %s: %w", LocalConfigFileName, err) } + // The file is untracked; the ignore rule must exist by the time it does. + ensureSidecarIgnored(filepath.Dir(localPath)) return nil } diff --git a/internal/config/machine_local_test.go b/internal/config/machine_local_test.go index a1628c7f3d..9662fbd0e2 100644 --- a/internal/config/machine_local_test.go +++ b/internal/config/machine_local_test.go @@ -355,27 +355,43 @@ func TestSetMachineLocalKeyNeverRewritesTrackedConfig(t *testing.T) { } } -// TestUnsetMachineLocalKeyLeavesTheSharedDefault pins that unset clears only -// THIS machine's override and never reaches into the tracked file, whatever -// order the operations happen in. The old code migrated first, so the same -// command removed the key from config.yaml before a one-time marker existed -// and left it afterwards — opposite outcomes decided by invisible state. -func TestUnsetMachineLocalKeyLeavesTheSharedDefault(t *testing.T) { +// TestUnsetMachineLocalKeyClearsBothFiles pins that `bd config unset` actually +// unsets. +// +// An earlier revision of this change cleared only the sidecar, on the theory +// that a tracked value is a shared default. That made the verb — documented as +// "Delete a configuration value" — a silent no-op for every machine-local key +// whose value lived only in config.yaml, while config_side_effects still +// announced that automatic backups had stopped. The tell was that it required +// rewriting a passing regression test (yaml_config_test.go's UnsetYamlConfig +// case) to a different key. +// +// Removing the key the operator NAMED is not the silent rewrite the migration +// did: that one moved keys nobody asked about, as a side effect of setting +// something else. The caller reports the tracked edit so the git diff is never +// a surprise. +func TestUnsetMachineLocalKeyClearsBothFiles(t *testing.T) { dir := t.TempDir() configPath := filepath.Join(dir, "config.yaml") - tracked := "dolt:\n mode: server\n" - if err := os.WriteFile(configPath, []byte(tracked), 0o600); err != nil { + if err := os.WriteFile(configPath, []byte("dolt:\n mode: server\n"), 0o600); err != nil { t.Fatal(err) } if err := setMachineLocalYamlConfig(configPath, "dolt.mode", "embedded"); err != nil { t.Fatal(err) } - if err := unsetMachineLocalYamlConfig(configPath, "dolt.mode"); err != nil { + + tracked, cleared, err := unsetMachineLocalYamlConfig(configPath, "dolt.mode") + if err != nil { t.Fatalf("unset: %v", err) } - - if got := readFile(t, configPath); got != tracked { - t.Errorf("unset touched config.yaml:\n got: %q\nwant: %q", got, tracked) + if !cleared { + t.Error("clearedTracked = false, want true: config.yaml defined the key") + } + if tracked != "server" { + t.Errorf("trackedValue = %q, want %q — the caller reports this to the operator", tracked, "server") + } + if _, ok := yamlValueInContent(readFile(t, configPath), "dolt.mode"); ok { + t.Error("config.yaml still defines dolt.mode after unset") } if _, ok := yamlValueInContent(readFile(t, LocalConfigPathFor(configPath)), "dolt.mode"); ok { t.Error("sidecar still defines dolt.mode after unset") @@ -393,7 +409,7 @@ func TestUnsetMachineLocalKeyNeverSetCreatesNothing(t *testing.T) { t.Fatal(err) } - if err := unsetMachineLocalYamlConfig(configPath, "dolt.socket"); err != nil { + if _, _, err := unsetMachineLocalYamlConfig(configPath, "dolt.socket"); err != nil { t.Fatalf("unset: %v", err) } diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index 496eed5516..fb0a11315b 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -558,6 +558,26 @@ func GetYamlConfig(key string) string { return v.GetString(normalizedKey) } +// UnsetYamlConfigReporting is UnsetYamlConfig plus what a caller needs to tell +// the operator the truth about a machine-local unset. +// +// It reports whether the TRACKED config.yaml was modified and what value was +// removed from it, so the command can say so. Without that the operator sees +// only "(in config.local.yaml)" while their git status grows a modified tracked +// file — and, when the value lived only in config.yaml, the previous behavior +// was worse still: unset silently did nothing while config_side_effects +// announced that automatic backups had stopped. +func UnsetYamlConfigReporting(key string) (trackedValue string, clearedTracked bool, err error) { + if !IsMachineLocalKey(key) { + return "", false, UnsetYamlConfig(key) + } + configPath, err := findProjectConfigYaml() + if err != nil { + return "", false, err + } + return unsetMachineLocalYamlConfig(configPath, key) +} + // UnsetYamlConfig removes a configuration value from the project's config.yaml file. // The key line is commented out (prefixed with "# ") to preserve it as documentation. func UnsetYamlConfig(key string) error { @@ -566,11 +586,12 @@ func UnsetYamlConfig(key string) error { return err } - // A machine-local key lives in the sidecar, so that is what unset clears. - // A value in the tracked config.yaml is a shared default that only an - // explicit edit should remove. + // A machine-local key is cleared from the sidecar AND from the tracked + // config.yaml — see UnsetYamlConfigReporting for why, and use that variant + // when the caller can tell the operator which files it touched. if IsMachineLocalKey(key) { - return unsetMachineLocalYamlConfig(configPath, key) + _, _, err := unsetMachineLocalYamlConfig(configPath, key) + return err } normalizedKey := normalizeYamlKey(key) diff --git a/internal/config/yaml_config_test.go b/internal/config/yaml_config_test.go index 61c8b38cef..e7b162c0e7 100644 --- a/internal/config/yaml_config_test.go +++ b/internal/config/yaml_config_test.go @@ -1057,11 +1057,8 @@ other-setting: value } defer os.Chdir(oldWd) - // Test UnsetYamlConfig. "other-setting" is deliberately NOT a machine-local - // key: those route to the sidecar and leave the tracked file alone by - // design, so asserting a config.yaml rewrite for one would pin the very - // behaviour the sidecar exists to prevent. - if err := UnsetYamlConfig("other-setting"); err != nil { + // Test UnsetYamlConfig + if err := UnsetYamlConfig("backup.enabled"); err != nil { t.Fatalf("UnsetYamlConfig() error = %v", err) } @@ -1072,11 +1069,11 @@ other-setting: value } contentStr := string(content) - if !strings.Contains(contentStr, "# other-setting: value") { - t.Errorf("config.yaml should contain commented-out other-setting, got:\n%s", contentStr) + if !strings.Contains(contentStr, "# backup.enabled: false") { + t.Errorf("config.yaml should contain commented-out backup.enabled, got:\n%s", contentStr) } - if !strings.Contains(contentStr, "backup.enabled: false") { - t.Errorf("config.yaml should preserve the untouched setting, got:\n%s", contentStr) + if !strings.Contains(contentStr, "other-setting: value") { + t.Errorf("config.yaml should preserve other settings, got:\n%s", contentStr) } } From 8c75ec0bb3ff532f283e6ba87810a72991319d84 Mon Sep 17 00:00:00 2001 From: "voxist.executor" Date: Tue, 1 Sep 2026 13:28:14 +0000 Subject: [PATCH 5/5] fix(config): pin the sidecar to a canonical key shape; stop claiming unmade removals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third review round on #35. The HIGH finding is real and I verified it myself, because the previous round had explicitly cleared this same point as "all forms resolve correctly through viper". It does not. **A sidecar write could be silently overridden by the tracked file.** viper's lookup tries the longest joined prefix first, so a FLAT `dolt.port:` beats a nested `dolt: {port:}` whatever the merge order — being merged last does not save the sidecar. And the shape was decided by accident of file state: updateNestedYamlKey bails on a comment-only file, so the first write into a fresh sidecar lands flat and every later one lands nested. A stock bd init config.yaml is comment-only too, so the first machine-local key old bd wrote there is flat. Reproduced on this branch: tracked config.yaml: dolt.port: 9999 sidecar: dolt: port: 3307 merged viper -> "9999" (the operator asked for 3307) GetStringFromDir -> "3307" Two read paths, two answers: bd bootstrap provisions one port while everything on merged viper dials another. `bd config set` reports success throughout. New setSidecarYamlKey always writes the flat dotted form, so the sidecar's key is at least as specific as anything in the tracked file and last-merge-wins holds. updateFlatYamlKey is updateYamlKey's flat half, split out. The new test covers all four tracked shapes — flat, nested, comment-only, key absent — and asserts the two read paths agree; the old class guard missed this because every subtest wrote one key into a fresh sidecar, which always took the flat branch. **Unset still claimed removals it had not made.** commentOutYamlKeyAnyForm is line-based and cannot reach a key inside a flow mapping (`dolt: {mode: server}`), so unset left the file untouched while the CLI printed success and config_side_effects announced that automatic backups had stopped. Unset now reports whether it cleared the sidecar, the tracked file, or neither, and the CLI prints what actually happened — naming only files it edited, and skipping the side-effect hint when nothing was removed. It also no longer names config.local.yaml for a key whose value lives only in the tracked file. Also: .gitignore is written 0644 to match doctor.ensureProjectGitignore, which has a test pinning that mode for the same file; and TestUnsetMachineLocalKeyLeavesTrackedConfigAlone is removed — it pinned the contract commit 4 reversed and passed only vacuously, because its fixture holds no machine-local key. Docs: configuration.md now says machine-local keys route to config.local.yaml on write, lists them, and shows the two outcomes side by side. It described the sidecar as hand-written only, so an operator would set a key and find nothing in config.yaml. Written per the beads-docs house style; docsync and doc-flags green. Claude-Session: https://claude.ai/code/session_01EF1jg1uS2tJPRoAsbsXuza --- cmd/bd/config.go | 35 ++++++++++- docs/reference/configuration.md | 16 ++++- internal/config/machine_local.go | 63 +++++++++++++++---- internal/config/machine_local_shape_test.go | 70 +++++++++++++++++++++ internal/config/machine_local_test.go | 25 +------- internal/config/yaml_config.go | 15 +++-- 6 files changed, 182 insertions(+), 42 deletions(-) create mode 100644 internal/config/machine_local_shape_test.go diff --git a/cmd/bd/config.go b/cmd/bd/config.go index 2230b0e1db..4ea5430bc2 100644 --- a/cmd/bd/config.go +++ b/cmd/bd/config.go @@ -598,7 +598,7 @@ var configUnsetCmd = &cobra.Command{ location := "config.yaml" var unsetErr error var trackedValue string - var clearedTracked bool + var clearedTracked, clearedLocal bool if config.IsUserGlobalKey(key) { unsetErr = config.UnsetUserYamlConfig(key) location = config.UserConfigYamlDisplayPath() @@ -606,16 +606,47 @@ var configUnsetCmd = &cobra.Command{ if config.IsMachineLocalKey(key) { location = config.LocalConfigFileName } - trackedValue, clearedTracked, unsetErr = config.UnsetYamlConfigReporting(key) + trackedValue, clearedTracked, clearedLocal, unsetErr = config.UnsetYamlConfigReporting(key) } if unsetErr != nil { return HandleError("unsetting config: %v", unsetErr) } + // Name only files actually edited, and do not claim a removal that + // did not happen: a key inside a YAML flow mapping is out of the + // line-based remover's reach, and a key that was never set has + // nothing to remove. Both used to print success plus a + // config_side_effects consequence. + if config.IsMachineLocalKey(key) { + switch { + case clearedLocal && clearedTracked: + location = config.LocalConfigFileName + " and config.yaml" + case clearedLocal: + location = config.LocalConfigFileName + case clearedTracked: + location = "config.yaml" + } + } + removedSomething := clearedLocal || clearedTracked + + if !removedSomething { + if jsonOutput { + return outputJSON(map[string]interface{}{ + "key": key, + "removed": false, + "reason": "not set in config.local.yaml or config.yaml, or defined inside a YAML flow mapping that must be edited by hand", + }) + } + fmt.Printf("%s was not removed: it is not set in %s or config.yaml.\n", key, config.LocalConfigFileName) + fmt.Printf(" (a key written inside a flow mapping, e.g. `dolt: {mode: server}`, must be edited by hand)\n") + return nil + } + if jsonOutput { payload := map[string]interface{}{ "key": key, "location": location, + "removed": true, } // A machine-local unset can touch BOTH files. Reporting only // the sidecar told a scripted caller — this repo's own tooling diff --git a/docs/reference/configuration.md b/docs/reference/configuration.md index 471ecbaa36..d99a61c6c3 100644 --- a/docs/reference/configuration.md +++ b/docs/reference/configuration.md @@ -27,7 +27,19 @@ Dolt is the only storage backend. Embedded mode (the default) stores data at `.b 3. `/.beads/config.yaml` (project-level, walked up from the current directory) 4. `$BEADS_DIR/config.yaml` (highest priority, when `BEADS_DIR` points at a different workspace) -A `config.local.yaml` next to the project `config.yaml` is also merged in last for machine-specific overrides that should not be committed. +A `config.local.yaml` next to the project `config.yaml` is merged in last, so anything in it wins. + +Some settings describe *this machine*, not the project — where the Dolt server listens, whether backups run here. Committing those to `config.yaml` makes every clone inherit one machine's answer. So `bd config set` routes them to `config.local.yaml` automatically; you do not have to remember which is which: + +```bash +bd config set dolt.mode server +# Set dolt.mode = server (in config.local.yaml) + +bd config set project.name my-app +# Set project.name = my-app (in config.yaml) +``` + +The routed keys are `dolt.mode`, `dolt.host`, `dolt.port`, `dolt.socket`, `dolt.user`, `dolt.data-dir`, `dolt.debug`, `backup.enabled`, and `backup.interval`. `bd config get` and `bd config list` name the file each value came from, and `bd init` adds `config.local.yaml` to `.beads/.gitignore`. ## Precedence @@ -535,7 +547,7 @@ output: title-length: 255 ``` -For machine-specific overrides that should not be committed, drop them in `.beads/config.local.yaml`; it is merged in last. +For machine-specific overrides that should not be committed, drop them in `.beads/config.local.yaml`; it is merged in last. The Dolt connection and backup keys go there on their own — see [Config Files](#config-files). ## Per-Command Override diff --git a/internal/config/machine_local.go b/internal/config/machine_local.go index 55796ab8cd..89dfb4d2a7 100644 --- a/internal/config/machine_local.go +++ b/internal/config/machine_local.go @@ -111,13 +111,46 @@ func setMachineLocalYamlConfig(configPath, key, value string) error { // commented out; committing the result sends every other clone back to // embedded storage — a different, empty database. Leaving the tracked file // alone costs nothing, because precedence already does the job. - return setYamlConfigAtPath(localPath, normalizeYamlKey(key), value) + return setSidecarYamlKey(localPath, normalizeYamlKey(key), value) +} + +// setSidecarYamlKey writes a key into the sidecar in the FLAT dotted form, +// always, whatever shape the file is already in. +// +// The form is load-bearing, not cosmetic. viper's key lookup tries the longest +// joined prefix first, so a flat `dolt.port:` BEATS a nested `dolt: {port:}` +// regardless of merge order — the sidecar being merged last does not save it. +// setYamlConfigAtPath picks the shape by accident of file state: a fresh +// sidecar is comment-only so updateNestedYamlKey bails and the key lands flat, +// while every later write finds a mapping and lands nested. A stock `bd init` +// config.yaml is comment-only too, so the first machine-local key old bd wrote +// there is flat as well. +// +// Combine those and the sidecar silently loses: `bd config set dolt.port 3307` +// reports success while merged viper keeps returning the tracked 9999, and +// GetStringFromDir (sidecar-first) returns 3307 — the two read paths disagree, +// so bootstrap provisions one port and the runtime dials another. Pinning the +// flat form makes the sidecar's key at least as specific as anything in the +// tracked file, so last-merge-wins holds. +func setSidecarYamlKey(localPath, key, value string) error { + content, err := os.ReadFile(localPath) //nolint:gosec // localPath derives from a resolved config.yaml path + if err != nil { + return fmt.Errorf("failed to read %s: %w", LocalConfigFileName, err) + } + updated, err := updateFlatYamlKey(string(content), key, value) + if err != nil { + return err + } + if err := os.WriteFile(localPath, []byte(updated), 0o600); err != nil { + return fmt.Errorf("failed to write %s: %w", LocalConfigFileName, err) + } + return nil } // unsetMachineLocalYamlConfig comments a machine-local key out of the sidecar. // The tracked config.yaml is left alone: a value there is a shared default that // only an explicit edit should remove. -func unsetMachineLocalYamlConfig(configPath, key string) (trackedValue string, clearedTracked bool, err error) { +func unsetMachineLocalYamlConfig(configPath, key string) (trackedValue string, clearedTracked, clearedLocal bool, err error) { localPath := LocalConfigPathFor(configPath) normalized := normalizeYamlKey(key) @@ -125,11 +158,12 @@ func unsetMachineLocalYamlConfig(configPath, key string) (trackedValue string, c if content, readErr := os.ReadFile(localPath); readErr == nil { //nolint:gosec // localPath derives from a resolved config.yaml path if updated := commentOutYamlKeyAnyForm(string(content), normalized); updated != string(content) { if writeErr := os.WriteFile(localPath, []byte(updated), 0o600); writeErr != nil { - return "", false, fmt.Errorf("failed to write %s: %w", LocalConfigFileName, writeErr) + return "", false, false, fmt.Errorf("failed to write %s: %w", LocalConfigFileName, writeErr) } + clearedLocal = true } } else if !os.IsNotExist(readErr) { - return "", false, fmt.Errorf("failed to read %s: %w", LocalConfigFileName, readErr) + return "", false, false, fmt.Errorf("failed to read %s: %w", LocalConfigFileName, readErr) } // Then clear the tracked value, because `bd config unset` is documented as @@ -146,22 +180,26 @@ func unsetMachineLocalYamlConfig(configPath, key string) (trackedValue string, c trackedRaw, readErr := os.ReadFile(configPath) //nolint:gosec // configPath is a resolved config.yaml path if readErr != nil { if os.IsNotExist(readErr) { - return "", false, nil + return "", false, clearedLocal, nil } - return "", false, fmt.Errorf("failed to read config.yaml: %w", readErr) + return "", false, clearedLocal, fmt.Errorf("failed to read config.yaml: %w", readErr) } value, found := yamlValueInContent(string(trackedRaw), normalized) if !found { - return "", false, nil + return "", false, clearedLocal, nil } + // commentOutYamlKeyAnyForm is line-based and cannot reach a key inside a + // FLOW mapping (`dolt: {mode: server}`). Reporting clearedTracked=false + // there is what lets the caller say nothing was removed, instead of + // printing success and a side-effect consequence that did not happen. updated := commentOutYamlKeyAnyForm(string(trackedRaw), normalized) if updated == string(trackedRaw) { - return "", false, nil + return "", false, clearedLocal, nil } if writeErr := os.WriteFile(configPath, []byte(updated), 0o600); writeErr != nil { - return "", false, fmt.Errorf("failed to write config.yaml: %w", writeErr) + return "", false, clearedLocal, fmt.Errorf("failed to write config.yaml: %w", writeErr) } - return value, true, nil + return value, true, clearedLocal, nil } // TrackedYamlValueFor reports a machine-local key's value still present in the @@ -211,7 +249,10 @@ func ensureSidecarIgnored(beadsDir string) { updated += "\n" } updated += LocalConfigFileName + "\n" - _ = os.WriteFile(gitignorePath, []byte(updated), 0o600) + // 0644 matches doctor.ensureProjectGitignore, which has a test pinning that + // mode for this same file; 0600 here would make the two writers disagree + // depending on which one created it. + _ = os.WriteFile(gitignorePath, []byte(updated), 0o644) //nolint:gosec // .gitignore is not sensitive and must match doctor's mode } // ensureLocalConfigFile creates the sidecar with its header if absent. The diff --git a/internal/config/machine_local_shape_test.go b/internal/config/machine_local_shape_test.go new file mode 100644 index 0000000000..651f8aa190 --- /dev/null +++ b/internal/config/machine_local_shape_test.go @@ -0,0 +1,70 @@ +package config + +import ( + "os" + "path/filepath" + "testing" + + "github.com/spf13/viper" +) + +// TestSidecarWinsWhateverShapeTheTrackedFileUses pins the property the sidecar +// split depends on: a machine-local value must win, for every combination of +// key shapes the two files can be in. +// +// viper's lookup tries the longest joined prefix first, so a FLAT `dolt.port:` +// beats a nested `dolt: {port:}` no matter which file is merged last. Before +// the sidecar was pinned to the flat form, its shape was decided by accident of +// file state — flat on the first write into a comment-only file, nested on +// every write after — so `bd config set dolt.port 3307` could report success +// while merged viper kept returning the tracked value, and GetStringFromDir +// (sidecar-first) returned the new one. Two read paths, two answers. +func TestSidecarWinsWhateverShapeTheTrackedFileUses(t *testing.T) { + for _, tc := range []struct { + name string + tracked string + }{ + {"tracked flat", "dolt.port: 9999\n"}, + {"tracked nested", "dolt:\n port: 9999\n"}, + {"tracked comment-only", "# Beads config\n"}, + {"tracked absent key", "backup:\n enabled: true\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(configPath, []byte(tc.tracked), 0o600); err != nil { + t.Fatal(err) + } + + // Two writes: the second is where the shape used to flip to nested. + if err := setMachineLocalYamlConfig(configPath, "dolt.port", "3307"); err != nil { + t.Fatal(err) + } + if err := setMachineLocalYamlConfig(configPath, "dolt.mode", "server"); err != nil { + t.Fatal(err) + } + + vp := viper.New() + vp.SetConfigType("yaml") + vp.SetConfigFile(configPath) + if err := vp.ReadInConfig(); err != nil { + t.Fatal(err) + } + vp.SetConfigFile(LocalConfigPathFor(configPath)) + if err := vp.MergeInConfig(); err != nil { + t.Fatal(err) + } + + if got := vp.GetString("dolt.port"); got != "3307" { + t.Errorf("merged viper dolt.port = %q, want 3307 (the sidecar value)", got) + } + if got := GetStringFromDir(dir, "dolt.port"); got != "3307" { + t.Errorf("GetStringFromDir dolt.port = %q, want 3307", got) + } + // The two read paths must agree; disagreement is the bootstrap/runtime split. + if vp.GetString("dolt.port") != GetStringFromDir(dir, "dolt.port") { + t.Error("merged viper and GetStringFromDir disagree") + } + }) + } +} diff --git a/internal/config/machine_local_test.go b/internal/config/machine_local_test.go index 9662fbd0e2..b90a2b9e07 100644 --- a/internal/config/machine_local_test.go +++ b/internal/config/machine_local_test.go @@ -178,27 +178,6 @@ func TestMachineLocalSidecarWinsOnRead(t *testing.T) { } } -func TestUnsetMachineLocalKeyLeavesTrackedConfigAlone(t *testing.T) { - beadsDir, configPath, localPath := newWorkspace(t, trackedConfigFixture) - - if err := SetYamlConfigInDir(beadsDir, "dolt.mode", "server"); err != nil { - t.Fatalf("set: %v", err) - } - before := readFile(t, configPath) - - t.Chdir(filepath.Dir(beadsDir)) - if err := UnsetYamlConfig("dolt.mode"); err != nil { - t.Fatalf("UnsetYamlConfig: %v", err) - } - - if after := readFile(t, configPath); after != before { - t.Errorf("unsetting a machine-local key modified config.yaml:\n%s", after) - } - if _, ok := readYamlValueAtPath(localPath, "dolt.mode"); ok { - t.Errorf("dolt.mode still live in %s after unset", LocalConfigFileName) - } -} - func TestIsMachineLocalKeyIsExactNotPrefix(t *testing.T) { local := []string{"dolt.mode", "dolt.host", "backup.enabled", "backup.interval"} for _, key := range local { @@ -380,7 +359,7 @@ func TestUnsetMachineLocalKeyClearsBothFiles(t *testing.T) { t.Fatal(err) } - tracked, cleared, err := unsetMachineLocalYamlConfig(configPath, "dolt.mode") + tracked, cleared, _, err := unsetMachineLocalYamlConfig(configPath, "dolt.mode") if err != nil { t.Fatalf("unset: %v", err) } @@ -409,7 +388,7 @@ func TestUnsetMachineLocalKeyNeverSetCreatesNothing(t *testing.T) { t.Fatal(err) } - if _, _, err := unsetMachineLocalYamlConfig(configPath, "dolt.socket"); err != nil { + if _, _, _, err := unsetMachineLocalYamlConfig(configPath, "dolt.socket"); err != nil { t.Fatalf("unset: %v", err) } diff --git a/internal/config/yaml_config.go b/internal/config/yaml_config.go index fb0a11315b..f23bee91d1 100644 --- a/internal/config/yaml_config.go +++ b/internal/config/yaml_config.go @@ -567,13 +567,13 @@ func GetYamlConfig(key string) string { // file — and, when the value lived only in config.yaml, the previous behavior // was worse still: unset silently did nothing while config_side_effects // announced that automatic backups had stopped. -func UnsetYamlConfigReporting(key string) (trackedValue string, clearedTracked bool, err error) { +func UnsetYamlConfigReporting(key string) (trackedValue string, clearedTracked, clearedLocal bool, err error) { if !IsMachineLocalKey(key) { - return "", false, UnsetYamlConfig(key) + return "", false, true, UnsetYamlConfig(key) } configPath, err := findProjectConfigYaml() if err != nil { - return "", false, err + return "", false, false, err } return unsetMachineLocalYamlConfig(configPath, key) } @@ -590,7 +590,7 @@ func UnsetYamlConfig(key string) error { // config.yaml — see UnsetYamlConfigReporting for why, and use that variant // when the caller can tell the operator which files it touched. if IsMachineLocalKey(key) { - _, _, err := unsetMachineLocalYamlConfig(configPath, key) + _, _, _, err := unsetMachineLocalYamlConfig(configPath, key) return err } @@ -699,7 +699,14 @@ func updateYamlKey(content, key, value string) (string, error) { return updated, nil } } + return updateFlatYamlKey(content, key, value) +} +// updateFlatYamlKey writes `key: value` as a single flat line, never descending +// into a nested mapping. Callers that need a predictable key SHAPE — the +// machine-local sidecar, where a nested key would lose to a flat one in the +// tracked file under viper's longest-prefix lookup — use this directly. +func updateFlatYamlKey(content, key, value string) (string, error) { formattedValue := formatYamlValue(value) newLine := fmt.Sprintf("%s: %s", key, formattedValue)