Use semver-aware update notices for newer pinned pre-releases#35588
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #35588 does not have the 'implementation' label and has only 75 new lines (≤100 threshold) in business logic directories. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect CLI update notifications by replacing lexicographic version ordering with semver-aware comparisons, ensuring newer pinned pre-releases don’t get flagged as “outdated” vs older stable releases.
Changes:
- Switch update-notice “current vs latest” ordering logic to use
golang.org/x/mod/semverwhen both versions are valid semver, with a string-compare fallback for non-semver inputs. - Preserve existing behavior for exact matches and
v-prefix-only differences. - Add regression tests covering stable and pre-release ordering and
v-prefix equivalence.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/update_check.go |
Introduces isCurrentVersionAtLeastLatest using semver comparison (with fallback) and wires it into the update notification skip logic. |
pkg/cli/update_check_test.go |
Adds table-driven tests to validate semver ordering and v-prefix equivalence for the new helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| if semver.IsValid(currentSV) && semver.IsValid(latestSV) { | ||
| return semver.Compare(currentSV, latestSV) >= 0 | ||
| } | ||
|
|
||
| return currentVersionNormalized > latestVersionNormalized |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 80/100 — Excellent
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). One table-driven design test covering 6 semver comparison scenarios including prerelease edge cases. Minor style note: add assertion messages per project guidelines.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /diagnose — approving with minor suggestions on test coverage gaps.
📋 Key Themes & Highlights
Key Themes
- Missing edge case test:
v0.77.0-beta.1vsv0.77.0(pre-release of same version vs its stable release) — the most subtle semver boundary, not yet covered. - Fallback path untested: the lexicographic fallback for non-semver inputs (e.g.
dev,nightly) has no test cases. - Assertion messages absent: project convention requires descriptive messages on
assert.*calls. - Redundant early-returns in helper: the function duplicates equality checks already performed by the caller — harmless, but worth a doc comment.
Positive Highlights
- ✅ Correct use of
golang.org/x/mod/semver— the standard Go module for semver, already in the module graph. - ✅ Clean extraction of
isCurrentVersionAtLeastLatestmakes the logic independently testable. - ✅ Good regression test table covering the previously broken
older-prerelease-vs-stableboundary. - ✅ Graceful fallback to string comparison for non-semver inputs preserves existing behaviour.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1M
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, tt.want, isCurrentVersionAtLeastLatest(tt.currentVersion, tt.latestVersion)) |
There was a problem hiding this comment.
[/tdd] Missing edge case: v0.77.0-beta.1 vs v0.77.0 — a pre-release of a version should still prompt the user to upgrade to the stable release of the same version.
💡 Suggested test case
{
name: "prerelease of same version as latest stable should prompt upgrade",
currentVersion: "v0.77.0-beta.1",
latestVersion: "v0.77.0",
want: false, // pre-release < stable of same version
},Per semver, v0.77.0-beta.1 < v0.77.0, so a user on a beta of the same version should receive an update notice pointing at the stable release. Without this test, the boundary between "older pre-release of same version" and "newer pre-release of older version" is not covered.
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| assert.Equal(t, tt.want, isCurrentVersionAtLeastLatest(tt.currentVersion, tt.latestVersion)) |
There was a problem hiding this comment.
[/tdd] No test for the non-semver fallback path. The string-comparison fallback (currentVersionNormalized > latestVersionNormalized) is reachable when one or both versions are non-semver (e.g. dev, main, nightly).
💡 Suggested test cases
{
name: "non-semver current is lexicographically greater",
currentVersion: "nightly",
latestVersion: "v0.76.1",
want: false, // "nightly" < "v0.76.1" lexicographically after TrimPrefix
},
{
name: "non-semver both versions equal",
currentVersion: "dev",
latestVersion: "dev",
want: true,
},Document in the test (or a comment) that the fallback intentionally uses lexicographic ordering so future maintainers don't remove it thinking it's dead code.
| latestVersion: "v0.76.1", | ||
| want: false, | ||
| }, | ||
| } |
There was a problem hiding this comment.
[/tdd] Assertion messages are missing — project convention (AGENTS.md) requires descriptive messages on every assert.* call so failures are self-explanatory without re-reading the test table.
💡 Quick fix
assert.Equal(t, tt.want, isCurrentVersionAtLeastLatest(tt.currentVersion, tt.latestVersion),
"isCurrentVersionAtLeastLatest(%q, %q)", tt.currentVersion, tt.latestVersion)This makes failure output read: isCurrentVersionAtLeastLatest("v0.77.0-beta.1", "v0.76.1"): expected true, got false instead of just expected true, got false.
| fmt.Fprintln(os.Stderr, "") | ||
| } | ||
|
|
||
| func isCurrentVersionAtLeastLatest(currentVersion, latestVersion string) bool { |
There was a problem hiding this comment.
[/diagnose] The early-return equality checks inside isCurrentVersionAtLeastLatest (lines 214–224) are unreachable when called from checkForUpdates, which already performs those exact checks before calling this helper. This is harmless for correctness but can create confusion about whether the function is intended to be a general-purpose utility or a narrow helper.
💡 Options
Option A — keep as-is and add a doc comment clarifying the function is self-contained by design (preferred if you want to keep it easily testable and reusable):
// isCurrentVersionAtLeastLatest reports whether currentVersion is >= latestVersion
// using semver comparison. It is self-contained and can be called independently
// of the equality checks in checkForUpdates.
func isCurrentVersionAtLeastLatest(...) bool {Option B — remove the redundant checks if this is intended purely as an internal helper, and rely on the caller to handle equality before calling.
Option A is the safer choice since the function is independently tested.
There was a problem hiding this comment.
🔎 Code quality review by PR Code Quality Reviewer · sonnet46 1.1M
| @@ -215,6 +211,27 @@ func checkForUpdates(noCheckUpdate bool, verbose bool) { | |||
| fmt.Fprintln(os.Stderr, "") | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Redundant equality guards: the two early-return checks inside isCurrentVersionAtLeastLatest (exact-match and v-prefix-only match) duplicate conditions that checkForUpdates already exits on before reaching this call.
💡 Detail
By the time checkForUpdates calls isCurrentVersionAtLeastLatest, it has already returned for both the latestVersion == currentVersion case (line ~185) and the currentVersionNormalized == latestVersionNormalized case (line ~193). So the helper's first two branches are permanently dead code in production.
This isn't a logic bug today, but it makes the helper look like a general-purpose utility (handling all cases including equality) while actually being tightly coupled to a specific call-site contract. Future maintainers who change the early-exit ordering in checkForUpdates may not realise the helper silently duplicates those checks, leading to subtle divergence.
Since the equality cases are already handled upstream, the helper could be simplified to just the semver-compare + string-fallback branches:
func isCurrentVersionNewer(currentVersion, latestVersion string) bool {
currentSV := ensureSemverPrefix(currentVersion)
latestSV := ensureSemverPrefix(latestVersion)
if semver.IsValid(currentSV) && semver.IsValid(latestSV) {
return semver.Compare(currentSV, latestSV) > 0
}
currentNorm := strings.TrimPrefix(currentVersion, "v")
latestNorm := strings.TrimPrefix(latestVersion, "v")
return currentNorm > latestNorm
}Renaming from isCurrentVersionAtLeastLatest → isCurrentVersionNewer (using > 0 instead of >= 0) would also make the intent at the call site clearer and eliminate the equality ambiguity.
The CLI update notice was comparing versions lexicographically, which caused newer pinned pre-releases to be treated as outdated and prompted a downgrade recommendation (for example,
0.77.0→0.76.1). This change makes the background update notifier follow semver ordering instead.Update-notice version comparison
pkg/cli/update_check.gowith semver-aware comparison.v-prefix-only differences.Regression coverage
vprefixResult