credhelper: stop dev builds nagging about an update to themselves

isNewerVersion split versions on "." and ran each component through
strconv.Atoi, discarding the error and substituting 0. For a git-describe
build the last component is "4-18-g8f70cce", which does not parse, so it
became 0 and every published release compared as newer. Running
v0.1.4-18-g8f70cce printed "Update available: v0.1.4" on every single
invocation, naming a version the binary was already 18 commits past.

Versions are now parsed properly: the "-<commits>-g<sha>" tail is recognised
and kept as a count of commits past the tag, and a version that cannot be
read in full returns false rather than being silently treated as 0. That
second part is the actual root cause — the comparison could not distinguish
"this component is zero" from "I could not read this component".

Ordering for a git-describe build is deliberately not semver, where a
prerelease sorts below its release. Such a build is commits AHEAD of its tag,
so v0.1.4-18-g8f70cce is newer than v0.1.4 and older than v0.1.4-20-gabc1234.

The function had no tests. Both failing cases are pinned along with the
ordinary release comparisons, so the git-describe handling cannot regress the
normal upgrade path.

Pre-existing at efabb677 rather than introduced by this range.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pkg/credhelper/cmd_update.go

@@ -8,6 +8,7 @@ import (
 	"os/exec"
 	"path"
 	"path/filepath"
+	"regexp"
 	"runtime"
 	"strconv"
 	"strings"
@@ -76,36 +77,79 @@ func fetchLatestVersion() (string, error) {
 }
 
 // isNewerVersion compares two version strings (simple semver comparison)
+// describeSuffix matches the "-<commits>-g<sha>" tail `git describe` appends to
+// the most recent tag, as in v0.1.4-18-g8f70cce.
+var describeSuffix = regexp.MustCompile(`^(.*)-(\d+)-g[0-9a-f]+$`)
+
+// parsedVersion is a version split into its numeric components plus, for a
+// git-describe build, how many commits it sits past its tag.
+type parsedVersion struct {
+	nums  []int
+	ahead int
+}
+
+// parseVersion reads "v0.1.4" and "v0.1.4-18-g8f70cce". It reports ok=false for
+// anything it cannot read in full, so an unparseable version is never mistaken
+// for an upgrade — the previous code swallowed strconv errors and substituted
+// 0, which made every release look newer than any git-describe build.
+func parseVersion(s string) (parsedVersion, bool) {
+	s = strings.TrimPrefix(strings.TrimSpace(s), "v")
+	if s == "" {
+		return parsedVersion{}, false
+	}
+
+	var ahead int
+	if m := describeSuffix.FindStringSubmatch(s); m != nil {
+		n, err := strconv.Atoi(m[2])
+		if err != nil {
+			return parsedVersion{}, false
+		}
+		s, ahead = m[1], n
+	}
+
+	parts := strings.Split(s, ".")
+	nums := make([]int, 0, len(parts))
+	for _, p := range parts {
+		n, err := strconv.Atoi(p)
+		if err != nil {
+			return parsedVersion{}, false
+		}
+		nums = append(nums, n)
+	}
+	return parsedVersion{nums: nums, ahead: ahead}, true
+}
+
+// versionComponent reads the i'th numeric component, treating absent trailing
+// components as 0 so "0.1" and "0.1.0" compare equal.
+func versionComponent(nums []int, i int) int {
+	if i < len(nums) {
+		return nums[i]
+	}
+	return 0
+}
+
 func isNewerVersion(newVersion, currentVersion string) bool {
 	if currentVersion == "dev" {
 		return true
 	}
 
-	newV := strings.TrimPrefix(newVersion, "v")
-	curV := strings.TrimPrefix(currentVersion, "v")
+	newV, okNew := parseVersion(newVersion)
+	curV, okCur := parseVersion(currentVersion)
+	if !okNew || !okCur {
+		return false
+	}
 
-	newParts := strings.Split(newV, ".")
-	curParts := strings.Split(curV, ".")
-
-	for i := range min(len(newParts), len(curParts)) {
-		newNum := 0
-		if parsed, err := strconv.Atoi(newParts[i]); err == nil {
-			newNum = parsed
-		}
-		curNum := 0
-		if parsed, err := strconv.Atoi(curParts[i]); err == nil {
-			curNum = parsed
-		}
-
-		if newNum > curNum {
-			return true
-		}
-		if newNum < curNum {
-			return false
+	for i := range max(len(newV.nums), len(curV.nums)) {
+		a, b := versionComponent(newV.nums, i), versionComponent(curV.nums, i)
+		if a != b {
+			return a > b
 		}
 	}
 
-	return len(newParts) > len(curParts)
+	// Same tag: whichever build sits further past it is the newer one. This is
+	// deliberately not semver ordering, where a prerelease sorts below its
+	// release — a git-describe build is commits AHEAD of its tag, not behind.
+	return newV.ahead > curV.ahead
 }
 
 // goreleaserArchiveName returns the archive filename goreleaser publishes for

pkg/credhelper/version_compare_test.go

@@ -0,0 +1,50 @@
+package credhelper
+
+import "testing"
+
+// isNewerVersion had no tests, and a git-describe build ("v0.1.4-18-g8f70cce")
+// nagged about an update on every single invocation. The cause was a silent
+// default: strconv.Atoi("4-18-g8f70cce") fails, the failure was swallowed, the
+// component became 0, and so every published release compared as newer than any
+// dev build of the same tag.
+func TestIsNewerVersion(t *testing.T) {
+	tests := []struct {
+		name    string
+		latest  string
+		current string
+		want    bool
+	}{
+		// The reported case: a dev build 18 commits past v0.1.4 is AHEAD of
+		// v0.1.4, so there is nothing to update to.
+		{"git describe build is ahead of its own tag", "v0.1.4", "v0.1.4-18-g8f70cce", false},
+		{"git describe build vs a real later release", "v0.2.0", "v0.1.4-18-g8f70cce", true},
+		{"git describe build vs a later patch", "v0.1.5", "v0.1.4-18-g8f70cce", true},
+		{"further ahead of the same tag", "v0.1.4-2-gabc1234", "v0.1.4-18-g8f70cce", false},
+		{"behind another describe build", "v0.1.4-20-gabc1234", "v0.1.4-18-g8f70cce", true},
+
+		// Plain releases.
+		{"newer patch", "v0.1.5", "v0.1.4", true},
+		{"newer minor", "v0.2.0", "v0.1.9", true},
+		{"newer major", "v1.0.0", "v0.9.9", true},
+		{"same version", "v0.1.4", "v0.1.4", false},
+		{"older patch", "v0.1.3", "v0.1.4", false},
+		{"older major", "v0.9.9", "v1.0.0", false},
+		{"missing v prefix still compares", "0.1.5", "0.1.4", true},
+
+		// An untagged build always updates.
+		{"dev always updates", "v0.1.4", "dev", true},
+
+		// Nothing parseable means no nagging.
+		{"garbage latest does not nag", "not-a-version", "v0.1.4", false},
+		{"empty latest does not nag", "", "v0.1.4", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isNewerVersion(tt.latest, tt.current); got != tt.want {
+				t.Errorf("isNewerVersion(%q, %q) = %v, want %v",
+					tt.latest, tt.current, got, tt.want)
+			}
+		})
+	}
+}