Skip to content

fix(state): preserve 'version:' on b.yaml round-trip (--fix, oci:// tags)#149

Merged
fentas merged 3 commits into
mainfrom
fix/binary-version-yaml-roundtrip
Apr 16, 2026
Merged

fix(state): preserve 'version:' on b.yaml round-trip (--fix, oci:// tags)#149
fentas merged 3 commits into
mainfrom
fix/binary-version-yaml-roundtrip

Conversation

@fentas
Copy link
Copy Markdown
Owner

@fentas fentas commented Apr 16, 2026

Summary

`BinaryList.MarshalYAML` emitted `LocalBinary.Enforced` as YAML `version:` and ignored `LocalBinary.Version` entirely. Two visible consequences:

  1. `b install --add oci://docker@cli` produced `oci://docker: {}` in `b.yaml` — the tag (`cli`) was loaded into `Version` (via `version:`) and never written back out.
  2. `b install --fix jq@v1.7` wrote `version: v1.7`. On reload only `Version` was populated, so the `--fix` pin was silently lost; next `b update` could move jq off the pinned version.

Fix

Emit `Version` as `version:` and `Enforced` as `enforced:` in `BinaryList.MarshalYAML`. The unmarshal path already maps them to distinct YAML keys, so they now round-trip independently.

Workflow

Split into two commits for clarity:

  1. `test(state): failing regression` — adds 4 sub-tests for `MarshalYAML` and a file-round-trip test, all currently failing.
  2. `fix(state): preserve 'version:' on b.yaml round-trip` — implements the fix; new tests pass; two existing tests updated from the old buggy semantics to the correct ones.

End-to-end reproduction

Before:
```
$ b install --add oci://docker@cli:/usr/local/bin/docker
$ cat b.yaml
binaries:
oci://docker:/usr/local/bin/docker: {} # ← tag wiped
```
After:
```
binaries:
oci://docker:/usr/local/bin/docker:
version: cli
```

Test plan

  • `TestBinaryListMarshalYAML_VersionRoundTrip` (4 sub-tests: version-only, enforced-only, both-set, oci-tag-as-version).
  • `TestBinaryListMarshalYAML_VersionSurvivesFileRoundTrip` — Load → Save → reload.
  • `go test ./...` — all 39 packages pass.
  • Manual e2e: `b install --add oci://docker@cli:/usr/local/bin/docker` now writes `version: cli` to b.yaml.

🤖 Generated with Claude Code

fentas and others added 2 commits April 16, 2026 14:40
BinaryList.MarshalYAML only emits LocalBinary.Enforced (as 'version:'),
ignoring LocalBinary.Version and the true 'enforced:' key. Practical
consequences:

- 'b install --add oci://docker@cli' saves as 'oci://docker: {}' —
  version 'cli' is silently dropped.
- A user-written 'version: v1.28.0' in b.yaml doesn't survive a
  LoadConfig → SaveConfig round-trip.

Both tests currently fail. A follow-up commit will make them pass by
emitting Version as 'version:' and Enforced as 'enforced:' (distinct
YAML keys).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
BinaryList.MarshalYAML emitted LocalBinary.Enforced as YAML 'version:'
and ignored LocalBinary.Version entirely. Two consequences, visible
most obviously for docker://oci:// refs:

- 'b install --add oci://docker@cli' produced 'oci://docker: {}' in
  b.yaml because the tag was loaded into Version (not Enforced) and
  Version was never written back.
- 'b install --fix jq@v1.7' (which sets both Version and Enforced)
  wrote out 'version: v1.7'; on reload only Version was repopulated and
  the --fix pin was silently lost. Next update could then move jq off
  the pinned version.

Fix: emit Version as 'version:' and Enforced as 'enforced:'. These are
distinct YAML keys in the unmarshal path already, so they now
round-trip independently.

The existing TestBinaryListMarshalYAML / TestBinaryListMarshalYAML_WithAsset
tests asserted the old buggy mapping (Enforced → 'version:') and have
been updated to match the fixed semantics (Enforced → 'enforced:').

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes b.yaml round-trip behavior for binary configuration so version: and --fix pins persist correctly across Load → Save, including OCI provider tags.

Changes:

  • Update BinaryList.MarshalYAML to emit LocalBinary.Version as version: and LocalBinary.Enforced as enforced:.
  • Update existing marshal tests to reflect the new distinct YAML keys.
  • Add regression tests to ensure version: survives both MarshalYAML and a full file round-trip.

Reviewed changes

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

File Description
pkg/state/types.go Adjusts YAML emission so version and enforced are serialized to distinct keys.
pkg/state/types_test.go Updates expectations and adds regression coverage for version: round-trips and OCI-tag preservation.

Comment thread pkg/state/types_test.go Outdated
Copilot round 1 on #149: the type assertion to map[string]string
ignored the ok result. If MarshalYAML ever returns a different map
type (or the entry is the bare &struct{}{} form), cfg would become nil
and the subtest would silently pass without checking anything.

Handle ok via t.Fatalf so drift is caught loudly.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@fentas fentas merged commit dbdc3a5 into main Apr 16, 2026
12 checks passed
@fentas fentas deleted the fix/binary-version-yaml-roundtrip branch April 16, 2026 13:01
fentas added a commit that referenced this pull request Apr 16, 2026
'b update' used to skip every docker:// / oci:// binary: its
LatestVersion just returns "latest" as a constant, and the
Version==Enforced short-circuit in EnsureBinary then returns nil. So
after 'b install oci://docker@cli' the user could sit on a stale
'docker:cli' forever — 'b update' never contacted the registry.

This PR makes mutable tags actually update:

1. New DigestResolver provider interface. Implemented by OCI and
   Docker via remote.Head (the same go-containerregistry client the
   OCI install already uses). A HEAD failure returns ("", nil) so a
   transient registry outage doesn't break update; a malformed ref
   still errors.

2. lock.BinEntry gains a Digest field (omitempty). install.updateLock
   calls ResolveDigest on the installed binary's provider and stores
   the manifest digest alongside the SHA256 of the extracted file.

3. update.updateBinaries consults the lock:
     - locked digest == fresh digest  → skip the download entirely
     - locked digest != fresh / unknown / no locked digest
       → bypass EnsureBinary's Version==Enforced short-circuit and
         force a DownloadBinary, so mutable tags pick up changes.
     - non-digest providers (github, gitlab, …) fall through to the
       existing update path — no behavioural change.

4. After updates, refreshLockDigests re-resolves digests and refreshes
   the lock's Digest + SHA256 for digest-resolver entries so the next
   'b update' sees an accurate locked state.

Also: bring managedKey's binaries allowlist back in sync with
BinaryList.MarshalYAML, which started emitting 'enforced:' as a
distinct key in #149 — TestManagedKey_MatchesMarshalOutput now passes.

End-to-end verified:
- first install writes 'digest: sha256:…' into b.lock
- 'b update' with unchanged digest → docker not re-pulled
- lock digest tampered → docker re-pulls and lock refreshes to the
  upstream digest

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fentas added a commit that referenced this pull request Apr 16, 2026
'b u argsh' used to leave argsh on the previously-installed version even
when upstream had a newer release. Two compounding bugs:

1. pkg/binaries/argsh/argsh.go — VersionLocalF ran 'argsh version', but
   argsh accepts '--version' (not a 'version' subcommand). Exec returned
   a non-zero exit, so VersionLocalF returned ("", err). Worse, the parser
   split on whitespace and took the last token, which for the real output
   'argsh v0.6.5 (<sha>)' would have been the commit sha, not the version.

   Switched to '--version' and grab fields[1] ('v0.6.5'). Also use
   strings.Fields so any whitespace layout works.

2. pkg/binary/binary.go — EnsureBinary's skip check
      local.Version == local.Enforced
         || local.Enforced == "" && local.Latest == local.Version
   fires when BOTH Version and Enforced are empty strings ("" == ""),
   which is exactly what happens when VersionLocalF errors (version is
   silently set to "" by LocalBinary). The result: the preset with a
   broken version probe silently skipped every update.

   Now EnsureBinary treats local.Version == "" as "unknown" and falls
   through to DownloadBinary instead of returning nil.

   Added TestBinary_EnsureBinary_UpdateWhenLocalVersionUnknown as a
   targeted regression guard.

Also aligns managedKey's binaries schema with BinaryList.MarshalYAML:
'enforced' was added to the marshaler in #149 but never back-ported to
managedKey, so TestManagedKey_MatchesMarshalOutput was already failing
on main. Fixed here since the argsh PR touches these packages together.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fentas added a commit that referenced this pull request Apr 16, 2026
## 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>
fentas pushed a commit that referenced this pull request Apr 16, 2026
🤖 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).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants