fix(state): preserve unknown YAML keys in b.yaml on save by fentas · Pull Request #148 · fentas/b

fentas · 2026-04-16T12:08:08Z

Summary

`b install --add oci://docker@cli` deleted user-defined `groups:` blocks from b.yaml. Root cause: `SaveConfigPreserving` diffed the re-marshaled in-memory state against the existing YAML tree and removed every key not present in the marshal output. Any field that b's own struct doesn't understand (top-level or per-binary) was silently dropped.

Fix

Introduce a schema predicate (`managedKey(path, key)`) that says which keys b actually owns at each path in b.yaml. The merge only removes keys that b manages and that are absent from the new state; all other dst-only keys (custom user annotations, unknown future fields) survive verbatim with their comments.

`managedKey` knows:

Root: `binaries`, `envs`, `profiles`.
One level in: any binary/env/profile name is managed (b owns the lists).
Two levels in: the field set emitted by `BinaryList`/`EnvList.MarshalYAML` (e.g. `version`, `alias`, `file`, `asset` for binaries; the `EnvEntry` fields for envs/profiles).
Deeper: managed — b owns the schema (e.g. `files..dest`).

Workflow

The fix is split into two commits for clarity:

`test(state): failing regression` — adds two tests that currently fail.
`fix(state): preserve unknown YAML keys` — implements the predicate; both tests now pass.

Test plan

`TestSaveConfig_PreservesUnknownTopLevelKeys` — asserts a user's top-level `groups:` section survives a save.
`TestSaveConfig_PreservesUnknownBinaryFields` — asserts per-binary custom fields (`groups`, `owner`) survive.
Existing `TestSaveConfig_PreservesComments`, `TestMergeMappings_PreservesComments` still pass.
`go test ./...` — all 39 packages pass.

🤖 Generated with Claude Code

'b install --add' wipes any YAML keys that aren't part of the b.yaml schema known to the current b version: - Top-level sections (e.g. a user-defined 'groups:' group-map used by external tooling) are dropped by the SaveConfigPreserving merge pass. - Per-binary custom fields (e.g. 'groups: [core]', 'owner: ...') on individual binaries are stripped because BinaryList.MarshalYAML only emits the fields b knows about. Both tests currently fail and document the expected behaviour — a follow-up commit will make them pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

'b install --add' (and any other path that calls SaveConfig) wiped: 1. User-defined top-level sections like 'groups:' used by external tooling. 2. Per-binary custom fields like 'groups:', 'owner:' on individual binaries. Both are forward-compat / extension points that b shouldn't silently drop when it rewrites b.yaml. The old mergeMappings removed every dst key not present in the freshly marshaled state, which dropped anything b's own struct didn't know about. Fix: introduce a schema predicate (managedKey) that says which keys b actually owns at each path. Only keys that b manages are eligible for removal when they disappear from the new state; everything else is treated as user-owned and preserved in-place with its comments. managedKey knows: - Root: binaries, envs, profiles. - One level in: a binary/env/profile name is always managed (b's lists). - Two levels in: the field set from BinaryList/EnvList MarshalYAML (version, alias, file, asset, enforced, latest for binaries; the EnvEntry fields for envs/profiles). - Deeper levels (e.g. files.<glob>.dest): managed (b owns the schema there, so deletions still propagate). The prior regression tests now pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes SaveConfigPreserving so saving b.yaml no longer deletes user-owned / unknown YAML keys (e.g., custom top-level groups: or per-binary annotations), while still allowing b-managed keys to be removed when they disappear from the in-memory state.

Changes:

Add schema-aware key-ownership predicate (managedKey) and use it during YAML tree merges.
Introduce mergeMappingsAt / mergeMappingsPath to preserve user-owned keys during SaveConfig merges while keeping the old merge behavior available.
Add regression tests covering preservation of unknown top-level and per-binary keys.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
pkg/state/yamlmerge.go	Implements schema-aware merge logic so only `b`-managed keys are eligible for deletion during merge.
pkg/state/yamlmerge_test.go	Adds regression tests ensuring unknown YAML keys survive a SaveConfig round-trip.

…ring Copilot round-1: 1. managedKey listed 'latest' and 'enforced' for binary entries, but BinaryList.MarshalYAML only emits version/alias/file/asset. Managed keys that never appear in src are *always* deleted from dst, so having them in the managed set could wipe user data if they were ever written to b.yaml. Limit to what the marshaler actually emits and note the coupling in the comment. 2. Regression tests used flat strings.Contains, which would pass for 'kubectl' even if the binary entry wasn't written because the initial 'groups:' list already contained it. Re-parse the saved YAML and assert structurally — 'kubectl' must live under binaries:, not just appear somewhere. 3. mergeMappingsAt docstring claimed a 'path' parameter that isn't on the signature. Reword to describe what the function actually does. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes SaveConfigPreserving so saving b.yaml no longer deletes user-defined / unknown YAML keys (e.g. top-level groups: or per-binary annotations) by making the YAML merge schema-aware and only removing keys that b “manages”.

Changes:

Added regression tests ensuring unknown top-level keys and unknown per-binary fields survive a SaveConfig.
Introduced a managedKey(path, key) predicate and a schema-aware merge (mergeMappingsAt) to preserve user-owned keys while still removing managed keys that are deleted from state.
Kept the old schema-agnostic mergeMappings behavior via a wrapper that always treats keys as managed (for tests/old semantics).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/state/yamlmerge.go	Adds schema-aware YAML merge with `managedKey` to preserve unknown/user-owned keys on save.
pkg/state/yamlmerge_test.go	Adds regression tests covering preservation of unknown top-level keys and unknown per-binary fields.

…a-test Copilot round-2: 1. managedKey now has schema-specific logic for envs/profiles but the regression tests only covered binaries. Added TestSaveConfig_PreservesUnknownEnvFields which loads a b.yaml with custom 'owner', 'labels' on an env entry and a 'owner' on a profile, runs SaveConfig, and asserts they all survive. 2. To prevent silent drift between managedKey and the marshalers, added TestManagedKey_MatchesMarshalOutput which drives BinaryList and EnvList MarshalYAML with representative values and asserts every emitted key is reported as managed at its path, plus sanity checks that common custom fields (owner/groups/labels/team/notes) are NOT reported as managed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes a config round-trip issue where SaveConfigPreserving could delete user-defined / unknown YAML keys in b.yaml (e.g., a custom top-level groups: block or per-binary annotations) when saving after operations like b install --add.

Changes:

Adds schema-aware key ownership via managedKey(path, key) and updates the merge logic to only delete dst-only keys that b manages.
Refactors the merge implementation into mergeMappingsAt / mergeMappingsPath to carry key-path context during merges.
Adds regression tests to ensure unknown keys at the top level and within binaries/envs/profiles survive SaveConfig.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/state/yamlmerge.go	Introduces schema-aware merge behavior to preserve user-owned YAML keys during comment-preserving saves.
pkg/state/yamlmerge_test.go	Adds regression tests covering preservation of unknown top-level keys and unknown fields in binaries/envs/profiles, plus a drift test for `managedKey`.

Copilot

Pull request overview

Fixes SaveConfigPreserving so saving b.yaml no longer deletes user-owned / unknown YAML keys (e.g. custom groups: blocks or per-entry annotations) by introducing a schema-aware “managed key” predicate that only deletes keys the tool owns.

Changes:

Add managedKey + mergeMappingsAt/mergeMappingsPath to make YAML merges schema-aware and preserve unknown keys while still removing managed keys that disappear from state.
Update SaveConfigPreserving to use the schema-aware merge.
Add regression tests covering preservation of unknown keys at top-level, per-binary, and env/profile entries, plus a “drift guard” test for managedKey vs marshal output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
pkg/state/yamlmerge.go	Introduces schema-aware merge logic (`managedKey`, `mergeMappingsAt`) and uses it when saving config to preserve unknown/user keys.
pkg/state/yamlmerge_test.go	Adds regression coverage for preserving unknown YAML keys and a test intended to keep `managedKey` aligned with marshal output.

Copilot round-3: 1. managedKey previously defaulted to 'managed=true' for any path depth greater than 2, meaning unknown keys nested under files.<glob> (or any future deeper schema) would still be wiped. Tighten to the known envs/profiles.files.<glob>.{dest,ignore,select} shape. Anything deeper than that is treated as user-owned and preserved. 2. The marshaler can emit envs.<name>.files.<glob> as a scalar shorthand (just the dest path) when only dest is set, while the existing tree may be a mapping carrying user keys like 'owner:'. Previously this shape change replaced the whole node and wiped the user keys. Now mergeMappingsPath detects a dst-mapping vs. src-scalar at the files- glob path and expands the scalar to {dest: <value>} before merging, so user keys survive. Tests: - TestSaveConfig_PreservesUnknownFilesGlobFields covers the scalar-vs- mapping shape case end-to-end. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot round-4: the two-level type assertions in the drift-guard test dropped failures silently. If MarshalYAML's output shape changed (wrong type, missing entry, empty map), 'range over nil' would make the test pass without actually checking managedKey ↔ marshaler alignment. Now each step of the extraction fails the test explicitly: - root cast to map[string]interface{} - entry key lookup - entry value cast to the expected type - non-empty entry (otherwise the range would silently be a no-op) Same hardening applied to both the binaries and envs halves. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes SaveConfigPreserving so saving b.yaml no longer deletes user-defined/unknown YAML keys (e.g., a top-level groups: block or custom per-binary/env annotations) by making the merge schema-aware.

Changes:

Add a managedKey(path, key) predicate to distinguish b-managed keys from user-owned/unknown keys.
Replace the previous schema-agnostic merge with mergeMappingsAt(...) so only managed keys are deleted when absent from the newly marshaled state.
Add regression tests covering preservation of unknown top-level keys, per-binary fields, env/profile fields, and files.<glob> shorthand edge cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File	Description
pkg/state/yamlmerge.go	Introduces schema-aware merging via `managedKey` + `mergeMappingsAt`, and normalizes `files.<glob>` scalar shorthand to preserve unknown keys.
pkg/state/yamlmerge_test.go	Adds multiple regression tests ensuring unknown YAML keys survive `SaveConfig` rewrites and the managed-key predicate stays aligned with marshal output.

Copilot round 5: 1. filesGlobScalarToMap synthesised 'dest: ""' for any scalar under files.<glob>, including the null/empty scalar that EnvList.MarshalYAML emits for a bare-key glob with empty GlobConfig. This added a managed 'dest' field the marshaler intentionally omitted, changing the saved shape. Renamed to filesGlobScalarToMap (from scalarDestToMap) and special-case null/empty scalars: return an empty mapping so the merge still preserves user-owned keys without inventing 'dest: ""'. 2. One remaining 'result, _ := os.ReadFile(...)' in the new files-glob preservation test ignored the error. Propagated with t.Fatalf. New test: - TestSaveConfig_PreservesBareFileGlob exercises the bare-key shorthand path (glob has no 'dest' — marshaler emits null scalar) and asserts the merge doesn't invent a 'dest: ""' while still preserving the user's 'owner:' key under the glob. Copilot also flagged "leading space" on envs: fixtures and "ignored type assertion" on envEntry/binEntry — verified both are false positives (envs: is at column 0, and the type assertions already call t.Fatalf on failure from round 4). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR fixes a data-loss bug when saving b.yaml by making the YAML merge schema-aware: only keys that b explicitly manages are eligible for deletion during a save, while unknown/user-owned keys (and their comments) are preserved.

Changes:

Add managedKey(path, key) to define which YAML keys are owned by b at each schema depth.
Replace the previous schema-agnostic merge with mergeMappingsAt so dst-only unknown keys survive SaveConfigPreserving.
Add regression tests covering preservation of unknown top-level keys, per-entry unknown fields, and files.<glob> shorthand edge cases, plus a drift-guard test tying managedKey to actual marshal output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File	Description
`pkg/state/yamlmerge.go`	Introduces schema-aware merge (`managedKey`, `mergeMappingsAt`) and special handling for `files.<glob>` scalar shorthands to preserve user-owned nested fields.
`pkg/state/yamlmerge_test.go`	Adds regression tests ensuring unknown YAML keys are preserved across `SaveConfig` and adds a drift-guard test for `managedKey` vs marshal output.

Copilot round 6: the drift-guard test's envSample never set Files, so it only validated managedKey for depth 2 (envs.<name>.<field>) and missed the nested envs.<name>.files.<glob> and envs.<name>.files.<glob>.<field> layers. Populate Files on the sample envSample entry with dest/ignore/select set. The test now: - confirms every key the marshaler emits at envs.<name>.files.<glob> (dest/ignore/select) is managed at depth 4, - confirms the glob key itself is managed at depth 3 (so deletions propagate), - sanity-checks that user-custom keys at depth 4 (owner, notes) are NOT managed. Copilot also re-flagged "leading space on envs:/profiles:" four times — verified again with cat -A: every fixture has the keys at column 0. The visible indent is the Go raw-string literal's source indent, not YAML content. Tests parse and pass. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

Fixes SaveConfigPreserving so it no longer deletes unknown/user-defined YAML keys in b.yaml (e.g., custom groups: blocks or per-entry annotations) by making the merge schema-aware and only removing keys that b explicitly manages.

Changes:

Added a managedKey(path, key) predicate and a schema-aware merge (mergeMappingsAt / mergeMappingsPath) so dst-only keys are removed only when they’re schema-managed.
Added special handling for envs/profiles.*.files.<glob> scalar shorthands so existing mapping nodes with user fields aren’t collapsed/wiped.
Added multiple regression tests covering unknown top-level keys, unknown per-binary/per-env/per-profile fields, and files.<glob> shorthand edge cases, plus a drift-guard test tying managedKey to actual MarshalYAML output.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
pkg/state/yamlmerge.go	Introduces schema-aware merge logic (managed-key predicate + shorthand normalization) so unknown keys/comments survive saves.
pkg/state/yamlmerge_test.go	Adds regression coverage for preserving unknown YAML keys across save, including tricky `files.<glob>` shorthand cases and a managedKey↔marshal drift guard.

## Summary \`b u argsh\` stayed on the currently-installed version forever even when upstream had a newer release. Two compounding bugs: **1. \`pkg/binaries/argsh/argsh.go\`** — \`VersionLocalF\` ran \`argsh version\`, but argsh accepts \`--version\` (no subcommand). The exec returned a non-zero exit so \`VersionLocalF\` returned \`("", err)\`. Even if the subcommand had worked, the parser split the output on whitespace and took the **last** token, which for real output \`argsh v0.6.5 (<sha>)\` was the commit sha, not the version. Fix: call \`--version\`, use \`strings.Fields\`, and grab \`fields[1]\` → \`v0.6.5\`. **2. \`pkg/binary/binary.go\`** — \`EnsureBinary\`'s skip check was: \`\`\`go if local.Version == local.Enforced || local.Enforced == "" && local.Latest == local.Version { return nil } \`\`\` When \`VersionLocalF\` errors, \`LocalBinary\` swallows the error and sets \`version=""\`. The check then matches \`"" == ""\` and returns nil. Net effect: any preset whose version probe breaks silently skips every update. Fix: treat \`local.Version == ""\` as "unknown" and fall through to \`DownloadBinary\` instead of short-circuiting. This is a **latent-bug class** — beyond argsh, any preset with a broken version probe (or a new CLI where the probed subcommand was removed) was failing silently. The fix is at the framework level so future presets can't hit the same wall. ## Also in this PR \`managedKey\` in \`pkg/state/yamlmerge.go\` was missing \`enforced\` at \`binaries.<name>\` — \`BinaryList.MarshalYAML\` emits \`enforced:\` (added in #149) but \`managedKey\` (landed in #148, slightly earlier) didn't know about it. \`TestManagedKey_MatchesMarshalOutput\` was red on main. Fixed here because the fix is in the same small surface area. ## Test plan - [x] \`TestBinary_EnsureBinary_UpdateWhenLocalVersionUnknown\` — guards against the silent-skip regression. - [x] Existing \`TestBinary_argsh\` still passes (binary generation unchanged). - [x] \`go test ./...\` — all 39 packages pass. - [x] End-to-end: \`b u argsh\` now moves from v0.6.5 → v0.6.6 (verified with a local build against \`github.com/kernpilot/lok8s\`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

🤖 I have created a release *beep* *boop* --- ## [4.16.0](v4.15.1...v4.16.0) (2026-04-16) ### Features * **update:** digest-based re-pull detection for docker:// / oci:// ([#151](#151)) ([5ba7ab9](5ba7ab9)) ### Bug Fixes * **preset,binary:** argsh update stuck at old version ([#152](#152)) ([5942730](5942730)) * **state:** preserve 'version:' on b.yaml round-trip (--fix, oci:// tags) ([#149](#149)) ([dbdc3a5](dbdc3a5)) * **state:** preserve unknown YAML keys in b.yaml on save ([#148](#148)) ([d0250c0](d0250c0)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

fentas and others added 2 commits April 16, 2026 14:05

fentas requested a review from Copilot April 16, 2026 12:08

Copilot started reviewing on behalf of fentas April 16, 2026 12:08 View session