Skip to content

Commit 5942730

Browse files
fentasclaude
andauthored
fix(preset,binary): argsh update stuck at old version (#152)
## 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>
1 parent d0250c0 commit 5942730

4 files changed

Lines changed: 63 additions & 6 deletions

File tree

pkg/binaries/argsh/argsh.go

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,12 +24,19 @@ func Binary(options *binaries.BinaryOptions) *binary.Binary {
2424
GitHubFile: "argsh",
2525
VersionF: binary.GithubLatest,
2626
VersionLocalF: func(b *binary.Binary) (string, error) {
27-
s, err := b.Exec("version")
27+
// argsh prints 'argsh v0.6.6 (<sha>)' to stdout for --version.
28+
// Parse the second whitespace-separated token so b's version
29+
// compare sees 'v0.6.6' instead of the commit sha that the
30+
// old 'strings.Split(s, " ")[last]' yielded.
31+
s, err := b.Exec("--version")
2832
if err != nil {
2933
return "", err
3034
}
31-
v := strings.Split(s, " ")
32-
return v[len(v)-1], nil
35+
fields := strings.Fields(s)
36+
if len(fields) < 2 {
37+
return "", nil
38+
}
39+
return fields[1], nil
3340
},
3441
}
3542
}

pkg/binary/binary.go

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -61,8 +61,14 @@ func (b *Binary) EnsureBinary(update bool) error {
6161
}
6262
local := b.LocalBinary(true)
6363

64-
if local.Version == local.Enforced || local.Enforced == "" && local.Latest == local.Version {
65-
return nil
64+
// Don't short-circuit when we can't tell what's installed. An empty
65+
// local.Version here means VersionLocalF either didn't run or
66+
// errored (e.g. preset uses the wrong subcommand and the binary
67+
// errors out); we must not mistake that for "matches the pin".
68+
if local.Version != "" {
69+
if local.Version == local.Enforced || local.Enforced == "" && local.Latest == local.Version {
70+
return nil
71+
}
6672
}
6773
}
6874

pkg/binary/binary_test.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,49 @@ func TestBinary_EnsureBinary_UpdateUpToDate(t *testing.T) {
258258
}
259259
}
260260

261+
// TestBinary_EnsureBinary_UpdateWhenLocalVersionUnknown guards against a
262+
// regression where a preset whose VersionLocalF errored (e.g. the binary
263+
// doesn't support the expected subcommand) would silently skip updates —
264+
// local.Version="" == local.Enforced="" used to satisfy the skip check,
265+
// so 'b update' did nothing even when the upstream release had moved.
266+
//
267+
// Now an empty local.Version means "unknown"; EnsureBinary must proceed
268+
// to DownloadBinary instead of early-returning.
269+
func TestBinary_EnsureBinary_UpdateWhenLocalVersionUnknown(t *testing.T) {
270+
tmp := t.TempDir()
271+
t.Setenv("PATH_BIN", tmp)
272+
// Pre-existing binary on disk, so the BinaryExists branch runs.
273+
if err := os.WriteFile(filepath.Join(tmp, "broken"), []byte("stale"), 0755); err != nil {
274+
t.Fatal(err)
275+
}
276+
b := &Binary{
277+
Name: "broken",
278+
// No pin.
279+
Version: "",
280+
VersionF: func(b *Binary) (string, error) {
281+
return "v2", nil // upstream has a newer version
282+
},
283+
VersionLocalF: func(b *Binary) (string, error) {
284+
// Preset's probe command failed — classic 'argsh version' bug:
285+
// subcommand doesn't exist so the binary exits non-zero and
286+
// VersionLocalF has no version to report.
287+
return "", os.ErrNotExist
288+
},
289+
// No URL/URLF/GitHubRepo so DownloadBinary will fail with
290+
// "no URL provided". That error is our sentinel: it proves the
291+
// download branch ran instead of the broken skip path. Before
292+
// the fix EnsureBinary returned nil here (Version=="" ==
293+
// Enforced=="") and the preset silently appeared up to date.
294+
}
295+
err := b.EnsureBinary(true)
296+
if err == nil {
297+
t.Fatal("expected DownloadBinary to be attempted (and fail without a download source), got nil")
298+
}
299+
if !strings.Contains(err.Error(), "no URL provided") {
300+
t.Errorf("expected the 'no URL provided' sentinel error, got: %v", err)
301+
}
302+
}
303+
261304
// --- exec.go ---
262305

263306
func TestBinary_Env(t *testing.T) {

pkg/state/yamlmerge.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@ func managedKey(path []string, key string) bool {
3535
// preserved verbatim.
3636
switch path[0] {
3737
case "binaries":
38+
// Matches BinaryList.MarshalYAML.
3839
switch key {
39-
case "version", "alias", "file", "asset":
40+
case "version", "enforced", "alias", "file", "asset":
4041
return true
4142
}
4243
return false

0 commit comments

Comments
 (0)