fix(config): route machine-local keys to config.local.yaml, not the tracked config.yaml - #35
Merged
Merged
Conversation
…ed config.yaml
.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.
…r routing Review of the previous commit found five defects, three of them caught by existing tests once the suite ran to completion. GetStringFromDir opened <beadsDir>/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 <machine-local key>` 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.
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
…ix set-many 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
…unmade removals 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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
.beads/config.yamlis tracked — its committed content is the project contract (issue_prefix, custom types, sync remote). bd also writes machine-local runtime state into that same file, so a checkout dirties itself with no one editing anything.Every runtime sibling in
.beads/is already gitignored —dolt/,backup/,dolt-server.*,.env.config.yamlis not, though bd writes it for the same reason.Downstream this breaks any clean-tree tooling: a release script, a
pre-commithook, a CI "no uncommitted changes" step. Such a guard refuses for a reason no operator caused and none can fix by committing once, because the next bd run writes the file again. The guard is behaving correctly; the file is the problem.Minimal repro — stock bd, no orchestrator
Verified verbatim against released bd 1.2.2. No orchestrator, no agent framework — plain
bd config setagainst a plain git repo. With this branch the same two commands leaveconfig.yamlbyte-identical and write.beads/config.local.yamlinstead.TestMachineLocalKeysNeverReachTrackedConfigis the same property as a beads-only test.Why a sidecar, and not the user-global config
bd already recognizes this class.
userGlobalExactKeys/IsUserGlobalKeyexists so per-machine settings never reach the tracked project file, and its comment spells out the hazard fornode_id: a committed value "propagates one machine's answer to every clone that pulls it". It routes those writes to~/.config/bd/config.yaml— one file for every workspace on the machine.That is the wrong axis for these keys. A host can legitimately run one workspace against a Dolt server and another embedded, so
dolt.modecannot be machine-global. The axis needed is per-machine, per-workspace — and it already exists, half-built:internal/config/config.gomerges.beads/config.local.yamllast, commented "machine-specific settings without polluting tracked config".docs/reference/configuration.mddocuments it in two places.This completes that shipped design rather than adding a new one. It also fixes a gap in the existing feature:
config.local.yamlwas never added to.beads/.gitignore, so the file bd told users to put machine-specific overrides in was itself tracked by default.The change
MachineLocalKeys— an exact-match registry of nine keys:dolt.mode,dolt.host,dolt.port,dolt.socket,dolt.user,dolt.data-dir,dolt.debug,backup.enabled,backup.interval. Exact rather than by prefix, so an unclassified key stays shared and existing behavior is preserved by default.Deliberately left shared, and documented in the source:
dolt.auto-startanddolt.disable-event-flush(fleet policy, committed on purpose), the pool timeouts andbackup.git-push/git-repo(project tuning), anddolt.shared-server— arguably machine-local, but bd's proxied-server migrations record it inconfig.yamlas workspace state and assert on it there, so moving it would have been a guess rather than a fix. Secrets stay withCheckSecretKeyGitSafety, which refuses the write rather than relocating it; routing them here would silently downgrade that refusal.SetYamlConfig,SetYamlConfigInDir,UnsetYamlConfig), so every caller is covered rather thanbd config setalone.GetStringFromDirnow mirrors the same precedence. It opens the workspace's files directly rather than going through merged viper —bd bootstrapresolvesdolt.portthrough it — so without this a routed value would be invisible there and bootstrap would fall back to a default while bd's merged config said otherwise.config.yamlfirst and the sidecar second, so a project can ship a default and a machine can override it.config.yaml, handling both the flat (backup.enabled: false) and nested (dolt:/mode: server) forms bd has written over its life. It rewrites lines rather than re-marshalling, so comments, ordering and formatting survive and the diff stays reviewable:A marker comment in the sidecar keeps it one-time: re-running it on every write would re-take a value someone deliberately restored to
config.yamlas a shared default — the same churn with the sign flipped. The marker is written only after the tracked cleanup succeeds, so a failed run (read-only checkout, full disk) can be retried instead of being skipped forever.config.local.yamladded to the.beads/.gitignoretemplate AND torequiredPatterns, so repositories that already exist pick it up frombd doctor --fixrather than trading one self-dirtying file for another.bd config get/list/unsetandbd dolt setreportconfig.local.yamlfor sidecar values.config_show.goalready guards against exactly this misattribution for user-global keys.Two supporting fixes fell out of review, both on paths this change newly exercises:
commentOutYamlKeyAnyFormmatched a segment at any depth and gave up beyond two segments, sodolt.modewould have commented out themode:inside adolt:/pool:block — dropping a different key's value. It now walks any number of segments and matches only direct children, by indent.commentOutYamlKeyscanned withbufio, 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. It splits on newlines instead. This matters here because the migration routes the git-trackedconfig.yamlthrough that path.Verification
make testgreen across all 97 packages;golangci-lintreports 0 issues.The class guard asserts over the registry itself — a key added later is covered when it is added — through both public writers. A control asserts shared keys still reach
config.yaml, so over-broad routing fails too.Mutation-tested: 10 mutants, all killed by test-assertion failures with 10 distinct kill sets, plus a control mutant that survived as intended.
dolt.modeSetYamlConfigrouting removedSetYamlConfigarm (9 keys)IsMachineLocalKeymatches by prefixrequiredPatternsdrops the sidecarGetStringFromDirsidecar lookup removedWorth noting: the first mutant does not kill the class guard, because the guard ranges over the registry and a removed key removes its own subtest.
TestIsMachineLocalKeyIsExactNotPrefix, which names keys explicitly, is what covers removal.Fleet context
Root fix for
vp-i84i. In/Users/Shared/Github/voxist-platformthe two lines bd appended were blockingbin/release-platform's_assert_clean_tree, so the release-on-approval order refused every release. Operator decision of 2026-09-01 was to fix at the root here and PR upstream, leaving the guard fail-closed.Schema-neutral: zero migration/schema/SQL files touched — 9 files, all Go, in
cmd/bdandinternal/config. Qualifies for the binary-only patch deploy path (ADR-0022 precedent).Deploy order once merged: bfork merge → binary-only patch → city
[bd].commitpin bump.Per repository, the first machine-local write after the upgrade produces one tracked-file change (the migration commenting the keys out) for the operator to commit once; after that commit the tree stays clean.
voxist-platform/.beads/.gitignoreis tracked and bd-owned, sobd doctor --fixadds theconfig.local.yamlline itself — no separate hand-written.gitignorechange is needed.Upstream PR: gastownhall#6125