Skip to content

Commit de77989

Browse files
fix: compare versions by semver, not string equality (#48)
The update nag treated any version difference as "newer", so a fresh throttle cache holding a tag that was latest when written but is now OLDER than the installed binary nagged backward. This happens in normal operation: release vX, self-upgrade within the 24h cache window, and the cached older tag triggers a "1.1.0 → v1.0.0" notice. Meanwhile `praxis update` (live fetch) correctly reported "already latest", so the two disagreed. Replace the string-equality compare with a small semver comparator (compareSemver) that only nags when the latest tag is strictly newer. `praxis update` now uses the same comparator (<= 0 → already latest), so it never offers a downgrade and can't disagree with the nag. No new dependency — the comparator handles v-prefix, missing components, build metadata, and release-outranks-prerelease. Regression tests cover the exact backward-nag scenario (fresh cache older than installed) plus the comparator's edge cases. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent a05367d commit de77989

3 files changed

Lines changed: 140 additions & 10 deletions

File tree

cmd/update.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,11 @@ Homebrew users: prefer 'brew upgrade praxis' so brew tracks the version.`,
5656

5757
latest := strings.TrimPrefix(rel.TagName, "v")
5858
current := strings.TrimPrefix(version, "v")
59-
if latest == current {
59+
// Treat "not strictly newer" as already-latest: an equal version, or a
60+
// release that is older than the installed build (so we never offer a
61+
// downgrade). Uses the same comparator as the background update nag
62+
// (cmd/update_check.go) so the two can't disagree.
63+
if compareSemver(rel.TagName, version) <= 0 {
6064
if asJSON {
6165
return render.JSON(out, map[string]any{
6266
"updated": false,

cmd/update_check.go

Lines changed: 68 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"io"
77
"os"
88
"regexp"
9+
"strconv"
910
"strings"
1011
"time"
1112
"unicode"
@@ -57,11 +58,10 @@ func checkForUpdate() string {
5758
if isDevBuild(version) {
5859
return ""
5960
}
60-
currentClean := strings.TrimPrefix(version, "v")
6161

6262
// Serve from cache while fresh.
6363
if cached, err := readUpdateCache(); err == nil && time.Since(cached.CheckedAt) < updateCheckInterval {
64-
return newerThan(currentClean, cached.LatestVersion)
64+
return newerThan(version, cached.LatestVersion)
6565
}
6666

6767
// Live fetch with one silent retry on any error.
@@ -80,7 +80,7 @@ func checkForUpdate() string {
8080
LatestVersion: rel.TagName,
8181
})
8282

83-
return newerThan(currentClean, rel.TagName)
83+
return newerThan(version, rel.TagName)
8484
}
8585

8686
// gitDescribeAhead matches the "-<n>-g<sha>" suffix git describe appends when
@@ -99,17 +99,76 @@ func isDevBuild(v string) bool {
9999
return gitDescribeAhead.MatchString(v)
100100
}
101101

102-
// newerThan reports latestTag (e.g. "v1.2.3") as the result when it differs
103-
// from the current version, else "". Comparison is string equality after
104-
// stripping the "v" prefix — matching praxis update (cmd/update.go) and raptor.
105-
func newerThan(currentClean, latestTag string) string {
106-
latestClean := strings.TrimPrefix(latestTag, "v")
107-
if latestClean != "" && latestClean != currentClean {
102+
// newerThan reports latestTag (e.g. "v1.2.3") as the result only when it is
103+
// strictly semver-newer than current, else "". A plain string-inequality test
104+
// is wrong here: the throttle cache can hold a tag that was latest when written
105+
// but is now OLDER than the installed binary (e.g. the user released vX and
106+
// self-upgraded within the 24h window), which would otherwise nag backward.
107+
func newerThan(current, latestTag string) string {
108+
if latestTag == "" {
109+
return ""
110+
}
111+
if compareSemver(latestTag, current) > 0 {
108112
return latestTag
109113
}
110114
return ""
111115
}
112116

117+
// compareSemver compares two dotted versions, ignoring a leading "v" and any
118+
// build metadata. Returns -1 if a<b, 0 if equal, 1 if a>b. The numeric core
119+
// (major.minor.patch) is compared numerically; a release outranks a prerelease
120+
// of the same core (1.0.0 > 1.0.0-rc1); two prereleases fall back to a string
121+
// compare. This is sufficient for goreleaser's clean release tags; "dev"/
122+
// "ahead"/"dirty" builds never reach here (filtered by isDevBuild).
123+
func compareSemver(a, b string) int {
124+
ac, ap := splitVersion(a)
125+
bc, bp := splitVersion(b)
126+
for i := 0; i < 3; i++ {
127+
if ac[i] != bc[i] {
128+
if ac[i] < bc[i] {
129+
return -1
130+
}
131+
return 1
132+
}
133+
}
134+
switch {
135+
case ap == bp:
136+
return 0
137+
case ap == "": // a is a release, b a prerelease of the same core → a newer
138+
return 1
139+
case bp == "":
140+
return -1
141+
case ap < bp:
142+
return -1
143+
default:
144+
return 1
145+
}
146+
}
147+
148+
// splitVersion parses "v1.2.3-rc1+build" into its numeric core [1,2,3] and
149+
// prerelease ("rc1"). Missing core components default to 0; non-numeric ones
150+
// are treated as 0.
151+
func splitVersion(v string) ([3]int, string) {
152+
v = strings.TrimPrefix(v, "v")
153+
core, pre := v, ""
154+
if i := strings.IndexByte(v, '-'); i >= 0 {
155+
core, pre = v[:i], v[i+1:]
156+
}
157+
// Drop any build metadata (from the core if there was no prerelease, or
158+
// from the tail of the prerelease).
159+
if j := strings.IndexByte(core, '+'); j >= 0 {
160+
core = core[:j]
161+
}
162+
if j := strings.IndexByte(pre, '+'); j >= 0 {
163+
pre = pre[:j]
164+
}
165+
var nums [3]int
166+
for i, p := range strings.SplitN(core, ".", 3) {
167+
nums[i], _ = strconv.Atoi(p)
168+
}
169+
return nums, pre
170+
}
171+
113172
// fetchLatestReleaseWithRetry calls the fetchLatestRelease seam up to twice,
114173
// pausing briefly between attempts. Returns nil if both attempts fail.
115174
func fetchLatestReleaseWithRetry() *selfupdate.Release {

cmd/update_check_test.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,12 @@ func TestCheckForUpdate(t *testing.T) {
6363
rel: &selfupdate.Release{TagName: "v1.2.0"},
6464
want: "v1.2.0",
6565
},
66+
{
67+
name: "newer patch returns latest tag",
68+
version: "1.0.0",
69+
rel: &selfupdate.Release{TagName: "v1.0.1"},
70+
want: "v1.0.1",
71+
},
6672
{
6773
name: "equal returns empty",
6874
version: "1.2.0",
@@ -75,6 +81,14 @@ func TestCheckForUpdate(t *testing.T) {
7581
rel: &selfupdate.Release{TagName: "1.2.0"},
7682
want: "",
7783
},
84+
{
85+
// Regression: installed binary is NEWER than the live "latest".
86+
// String-equality would nag backward; semver must stay silent.
87+
name: "installed newer than latest stays silent",
88+
version: "1.2.0",
89+
rel: &selfupdate.Release{TagName: "v1.0.0"},
90+
want: "",
91+
},
7892
{
7993
name: "dev build skipped",
8094
version: "dev",
@@ -124,6 +138,18 @@ func TestCheckForUpdate(t *testing.T) {
124138
failFetch: true,
125139
want: "",
126140
},
141+
{
142+
// Exact production bug: a fresh cache holds the tag that was latest
143+
// when written (v1.0.0), but the user has since self-upgraded to
144+
// 1.1.0 within the 24h window. Must NOT nag backward.
145+
name: "fresh cache older than installed stays silent",
146+
version: "1.1.0",
147+
seedCache: func(t *testing.T) {
148+
writeCache(t, time.Hour, "v1.0.0")
149+
},
150+
failFetch: true,
151+
want: "",
152+
},
127153
{
128154
name: "stale cache triggers fetch",
129155
version: "1.0.0",
@@ -251,6 +277,47 @@ func TestCheckForUpdate_ThrottlesFailures(t *testing.T) {
251277
}
252278
}
253279

280+
func TestCompareSemver(t *testing.T) {
281+
tests := []struct {
282+
a, b string
283+
want int
284+
}{
285+
{"1.0.0", "1.0.0", 0},
286+
{"v1.0.0", "1.0.0", 0}, // v-prefix ignored
287+
{"1.1.0", "1.0.0", 1},
288+
{"1.0.0", "1.1.0", -1},
289+
{"1.0.1", "1.0.0", 1},
290+
{"2.0.0", "1.9.9", 1},
291+
{"1.10.0", "1.9.0", 1}, // numeric, not lexical
292+
{"1.0", "1.0.0", 0}, // missing patch defaults to 0
293+
{"1.0.0", "1.0.0-rc1", 1}, // release outranks prerelease
294+
{"1.0.0-rc1", "1.0.0", -1}, // and vice versa
295+
{"1.0.0-rc2", "1.0.0-rc1", 1}, // prerelease string compare
296+
{"1.2.3+build", "1.2.3", 0}, // build metadata ignored
297+
}
298+
for _, tt := range tests {
299+
if got := compareSemver(tt.a, tt.b); got != tt.want {
300+
t.Errorf("compareSemver(%q, %q) = %d, want %d", tt.a, tt.b, got, tt.want)
301+
}
302+
}
303+
}
304+
305+
func TestNewerThan(t *testing.T) {
306+
tests := []struct {
307+
current, latestTag, want string
308+
}{
309+
{"1.0.0", "v1.1.0", "v1.1.0"}, // newer → returns the tag verbatim
310+
{"1.0.0", "v1.0.0", ""}, // equal
311+
{"1.1.0", "v1.0.0", ""}, // older → silent (the bug)
312+
{"1.0.0", "", ""}, // empty (failed-fetch marker)
313+
}
314+
for _, tt := range tests {
315+
if got := newerThan(tt.current, tt.latestTag); got != tt.want {
316+
t.Errorf("newerThan(%q, %q) = %q, want %q", tt.current, tt.latestTag, got, tt.want)
317+
}
318+
}
319+
}
320+
254321
func TestPrintUpdateNotification(t *testing.T) {
255322
withVersion(t, "1.0.0")
256323
var buf bytes.Buffer

0 commit comments

Comments
 (0)