diff --git a/cmd/bd/config.go b/cmd/bd/config.go index 3510473a54..4ea5430bc2 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 { @@ -327,15 +330,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) } @@ -406,6 +418,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(): @@ -517,7 +532,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)) + } } } @@ -535,7 +558,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) } @@ -571,25 +597,73 @@ var configUnsetCmd = &cobra.Command{ if config.IsYamlOnlyKey(key) { location := "config.yaml" var unsetErr error + var trackedValue string + var clearedTracked, clearedLocal bool if config.IsUserGlobalKey(key) { unsetErr = config.UnsetUserYamlConfig(key) location = config.UserConfigYamlDisplayPath() } else { - unsetErr = config.UnsetYamlConfig(key) + if config.IsMachineLocalKey(key) { + location = config.LocalConfigFileName + } + 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 { - if err := outputJSON(map[string]interface{}{ + payload := map[string]interface{}{ "key": key, "location": location, - }); err != nil { + "removed": true, + } + // 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) + if clearedTracked { + fmt.Printf(" also removed %s = %s from config.yaml (tracked in git — commit the change)\n", key, trackedValue) + } } printConfigSideEffects(checkConfigUnsetSideEffects(key)) return nil @@ -931,7 +1005,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" } @@ -951,6 +1034,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/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/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/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/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 new file mode 100644 index 0000000000..89dfb4d2a7 --- /dev/null +++ b/internal/config/machine_local.go @@ -0,0 +1,302 @@ +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" + +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.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 +// 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.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 + } + // 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 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, clearedLocal bool, err error) { + localPath := LocalConfigPathFor(configPath) + 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, false, fmt.Errorf("failed to write %s: %w", LocalConfigFileName, writeErr) + } + clearedLocal = true + } + } else if !os.IsNotExist(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 + // "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. + // + // 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, clearedLocal, nil + } + return "", false, clearedLocal, fmt.Errorf("failed to read config.yaml: %w", readErr) + } + value, found := yamlValueInContent(string(trackedRaw), normalized) + if !found { + 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, clearedLocal, nil + } + if writeErr := os.WriteFile(configPath, []byte(updated), 0o600); writeErr != nil { + return "", false, clearedLocal, fmt.Errorf("failed to write config.yaml: %w", writeErr) + } + return value, true, clearedLocal, 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)) +} + +// 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" + // 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 +// 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) + } + // The file is untracked; the ignore rule must exist by the time it does. + ensureSidecarIgnored(filepath.Dir(localPath)) + return nil +} + +// 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_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 new file mode 100644 index 0000000000..b90a2b9e07 --- /dev/null +++ b/internal/config/machine_local_test.go @@ -0,0 +1,398 @@ +package config + +import ( + "os" + "path/filepath" + "sort" + "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 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: "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", + 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) + } + }) + } +} + +// 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) + } +} + +// 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 got := readFile(t, configPath); got != tracked { + t.Errorf("config.yaml was rewritten:\n got: %q\nwant: %q", got, tracked) + } + 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) + } +} + +// 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") + 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) + } + + tracked, cleared, _, err := unsetMachineLocalYamlConfig(configPath, "dolt.mode") + if err != nil { + t.Fatalf("unset: %v", err) + } + 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") + } +} + +// 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 _, _, _, err := unsetMachineLocalYamlConfig(configPath, "dolt.socket"); err != nil { + t.Fatalf("unset: %v", err) + } + + 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.go b/internal/config/yaml_config.go index 7f6896452d..f23bee91d1 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 @@ -540,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, clearedLocal bool, err error) { + if !IsMachineLocalKey(key) { + return "", false, true, UnsetYamlConfig(key) + } + configPath, err := findProjectConfigYaml() + if err != nil { + return "", false, 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 { @@ -548,6 +586,14 @@ func UnsetYamlConfig(key string) error { return err } + // 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) { + _, _, _, err := unsetMachineLocalYamlConfig(configPath, key) + return err + } + normalizedKey := normalizeYamlKey(key) content, err := os.ReadFile(configPath) //nolint:gosec // configPath is from findProjectConfigYaml @@ -653,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) @@ -798,27 +851,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. @@ -919,3 +971,130 @@ 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. +// +// 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 { + return out + } + + lines := strings.Split(out, "\n") + path := findNestedKeyPath(lines, parts, 0, 0, len(lines), -1) + if path == nil { + return out + } + + 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 + } + 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 indent != childIndent { + continue + } + 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...) + } + return nil + } + return nil +} + +// 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 indentWidth(line string) int { + return len(line) - len(strings.TrimLeft(line, " \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 +}