Skip to content

fix(preset,binary): argsh update stuck at old version#152

Merged
fentas merged 2 commits into
mainfrom
fix/argsh-version-parsing
Apr 16, 2026
Merged

fix(preset,binary): argsh update stuck at old version#152
fentas merged 2 commits into
mainfrom
fix/argsh-version-parsing

Conversation

@fentas
Copy link
Copy Markdown
Owner

@fentas fentas commented 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 ()` 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.` — `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

  • `TestBinary_EnsureBinary_UpdateWhenLocalVersionUnknown` — guards against the silent-skip regression.
  • Existing `TestBinary_argsh` still passes (binary generation unchanged).
  • `go test ./...` — all 39 packages pass.
  • 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

'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>
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 update argsh (and similar presets) getting stuck by correcting argsh local version detection and preventing EnsureBinary from silently skipping updates when the local version is unknown.

Changes:

  • Fix argsh local version probing to use --version and parse the actual semver token.
  • Update EnsureBinary to avoid early-returning when local.Version is empty/unknown.
  • Align YAML merge schema management with BinaryList.MarshalYAML by treating enforced as a managed key.

Reviewed changes

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

File Description
pkg/state/yamlmerge.go Adds enforced to the managed key set for binaries.<name> to match marshal output.
pkg/binary/binary_test.go Adds regression test to ensure updates proceed when local version detection fails.
pkg/binary/binary.go Prevents update skip when local.Version is empty/unknown.
pkg/binaries/argsh/argsh.go Fixes argsh local version command and parsing to return vX.Y.Z correctly.

Comment thread pkg/binary/binary_test.go Outdated
Copilot round 1 on #152: the unused downloadCalled was dead code and
made the test's intent unclear. Removed it and tightened the
assertion — the 'no URL provided' error from DownloadBinary is the
real proof that the skip path was bypassed, so check the error string
explicitly instead of merely non-nil.

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 4 out of 4 changed files in this pull request and generated no new comments.

@fentas fentas merged commit 5942730 into main Apr 16, 2026
12 checks passed
@fentas fentas deleted the fix/argsh-version-parsing branch April 16, 2026 13:51
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).
fentas added a commit that referenced this pull request May 8, 2026
…on) (#156)

## Summary
PR #152 changed \`EnsureBinary\` to treat empty \`local.Version\` (from
a failing \`VersionLocalF\`) as "needs download". This caused **tilt to
re-download its 41MB binary on every \`b update\`** because tilt's
version probe fails when \`getent\` isn't in \`\$PATH\`.

## Root cause
\`tilt version\` → exits non-zero ("Fatal error initializing analytics:
getent not found") → \`VersionLocalF\` returns \`("", err)\` →
\`local.Version = ""\`.

With #152's guard: \`local.Version != ""\` = false → skip the skip-check
→ always download.

Without #152's guard (original logic): \`"" == ""\` → skip. Binary stays
on disk.

## Fix
Revert to the original \`EnsureBinary\` logic. The argsh fix in #152 was
at the **preset level** (calling \`--version\` instead of \`version\`),
so the framework-level guard isn't needed.

## Trade-off
A genuinely outdated binary whose version probe is broken won't
auto-update until:
- The preset's \`VersionLocalF\` is fixed, or
- The user runs \`b update --force\`

This is acceptable — re-downloading 41MB on every update is worse.

## Test plan
- [x] \`TestBinary_EnsureBinary_UpdateSkipsWhenVersionUnknown\` —
verifies skip behaviour
- [x] \`go test ./...\` — all 39 packages pass
- [x] Manual: \`b u tilt\` now shows \`0B\` (skipped) instead of
\`41.05MB\`

🤖 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 May 8, 2026
🤖 I have created a release *beep* *boop*
---


## [4.17.1](v4.17.0...v4.17.1)
(2026-05-08)


### Bug Fixes

* **binary:** revert EnsureBinary unknown-version guard
([#152](#152) regression)
([#156](#156))
([37428f4](37428f4))
* **preset:** disable tilt analytics during version check
([#158](#158))
([f30b803](f30b803))

---
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