cli/integration/version_check_integration_test.go (1)

41-60: ⚡ Quick win

Test does not verify its stated purpose.

The test name Test_Integration_VersionCheck_PrintsWarningToStderr implies it verifies that a warning is printed, but the comment on lines 57-59 acknowledges that "the warning may or may not appear depending on timing" and the test only verifies that Execute() doesn't error.

Either:

1. Fix the test to reliably verify the warning appears (e.g., add a small sleep or synchronization mechanism after Execute() completes and before checking stderr)
2. Rename the test to reflect what it actually verifies (e.g., Test_Integration_VersionCheck_DoesNotErrorWithOldVersion)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/integration/version_check_integration_test.go` around lines 41 - 60, The
test Test_Integration_VersionCheck_PrintsWarningToStderr currently sets
version.BuildVersion and captures stderr but never asserts that a warning was
printed (it only checks rootCmd.Execute() succeeds), so either make the test
deterministic by waiting for the async version check to complete and then assert
stderr contains the expected warning, or rename the test to reflect current
behavior; to fix, either add synchronization (e.g., have the version check
signal completion or poll/wait briefly after rootCmd.Execute() before reading
stderr) and assert stderr.String() includes the warning text, or change the test
name to something like Test_Integration_VersionCheck_DoesNotErrorWithOldVersion
so it matches the existing assertion that rootCmd.Execute() returns no error
(use the existing symbols Test_Integration_VersionCheck_PrintsWarningToStderr,
rootCmd.Execute, stderr, and version.BuildVersion to locate and update the
test).

cli/internal/versioncheck/cache_test.go (2)

10-45: ⚡ Quick win

Consider using fixed timestamps for deterministic tests.

The test uses time.Now() both when constructing test cases and when calling IsFresh(). If the system clock advances between these calls (even by microseconds), the test assertions might occasionally fail or pass unexpectedly.

♻️ Proposed fix using fixed timestamps

 func TestCacheEntry_IsFresh(t *testing.T) {
+	now := time.Date(2026, 5, 12, 12, 0, 0, 0, time.UTC)
 	tests := []struct {
 		name      string
 		checkedAt time.Time
-		now       time.Time
 		want      bool
 	}{
 		{
 			name:      "fresh cache",
-			checkedAt: time.Now().Add(-1 * time.Hour),
-			now:       time.Now(),
+			checkedAt: now.Add(-1 * time.Hour),
 			want:      true,
 		},
 		{
 			name:      "stale cache",
-			checkedAt: time.Now().Add(-25 * time.Hour),
-			now:       time.Now(),
+			checkedAt: now.Add(-25 * time.Hour),
 			want:      false,
 		},
 		{
 			name:      "exactly at boundary",
-			checkedAt: time.Now().Add(-24 * time.Hour),
-			now:       time.Now(),
+			checkedAt: now.Add(-24 * time.Hour),
 			want:      false,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			entry := &CacheEntry{CheckedAt: tt.checkedAt}
-			if got := entry.IsFresh(tt.now); got != tt.want {
+			if got := entry.IsFresh(now); got != tt.want {
 				t.Errorf("IsFresh() = %v, want %v", got, tt.want)
 			}
 		})
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/internal/versioncheck/cache_test.go` around lines 10 - 45, The test
TestCacheEntry_IsFresh is non-deterministic because it calls time.Now() multiple
times; replace those dynamic calls with a single fixed base time (e.g. base :=
time.Date(...)) and build each test case's checkedAt and now relative to that
base so CacheEntry{CheckedAt: ...} and entry.IsFresh(now) use deterministic
timestamps; update the three cases (fresh, stale, exactly at boundary) to use
base.Add(...) offsets to preserve the intended expectations for
CacheEntry.IsFresh.

---

47-82: ⚡ Quick win

Consider using fixed timestamps for deterministic tests.

Similar to TestCacheEntry_IsFresh, this test uses time.Now() in test case construction, which could lead to flaky behavior.

♻️ Proposed fix using fixed timestamps

 func TestCacheEntry_ShouldWarn(t *testing.T) {
+	now := time.Date(2026, 5, 12, 12, 0, 0, 0, time.UTC)
 	tests := []struct {
 		name    string
 		warnedAt time.Time
-		now      time.Time
 		want     bool
 	}{
 		{
 			name:     "never warned",
 			warnedAt: time.Time{},
-			now:      time.Now(),
 			want:     true,
 		},
 		{
 			name:     "warned recently",
-			warnedAt: time.Now().Add(-1 * time.Hour),
-			now:      time.Now(),
+			warnedAt: now.Add(-1 * time.Hour),
 			want:     false,
 		},
 		{
 			name:     "warned long ago",
-			warnedAt: time.Now().Add(-25 * time.Hour),
-			now:      time.Now(),
+			warnedAt: now.Add(-25 * time.Hour),
 			want:     true,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			entry := &CacheEntry{WarnedAt: tt.warnedAt}
-			if got := entry.ShouldWarn(tt.now); got != tt.want {
+			if got := entry.ShouldWarn(now); got != tt.want {
 				t.Errorf("ShouldWarn() = %v, want %v", got, tt.want)
 			}
 		})
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/internal/versioncheck/cache_test.go` around lines 47 - 82, The test
TestCacheEntry_ShouldWarn is using time.Now() which makes it flaky; update the
test to use fixed deterministic timestamps (e.g., base := time.Date(2023, 1, 1,
12, 0, 0, 0, time.UTC)) and create warnedAt and now values relative to that base
(e.g., base, base.Add(-1*time.Hour), base.Add(-25*time.Hour)) when constructing
the test cases, then use those values to initialize CacheEntry{WarnedAt: ...}
and pass now into entry.ShouldWarn so the expectations are stable.

cli/internal/versioncheck/check.go (2)

160-220: ⚖️ Poor tradeoff

Consider documenting the single-page limitation.

The implementation fetches only the first page of releases (20 items). If more than 20 releases have been published since the last stable CLI release, the version check will fail to find an update.

While this is unlikely given the PR description notes that the latest stable release is "typically within the first page," adding a code comment near line 163 could help future maintainers understand this design choice:

 func fetchLatestVersion(ctx context.Context, opts Options) (string, error) {
+	// Note: We fetch only the first page of releases. If the latest stable CLI
+	// release is beyond the first 20 releases, it will not be detected.
 	url := fmt.Sprintf("%s/repos/%s/%s/releases?per_page=%d",
 		opts.BaseURL, opts.GitHubOwner, opts.GitHubRepo, releasesPerPage)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/internal/versioncheck/check.go` around lines 160 - 220, In
fetchLatestVersion, add a concise comment near the URL/release-per-page
construction (around the releasesPerPage usage) documenting that the function
intentionally only requests the first page of GitHub releases
(per_page=releasesPerPage) and therefore may miss updates if more than that many
releases exist; mention the chosen page-size (releasesPerPage) and the
rationale/assumption (latest stable release is typically on page one) and note
that paging could be added later if needed so future maintainers understand this
limitation.

---

99-103: 💤 Low value

Consider logging cache read errors for debugging.

The code silently ignores cache read errors. While this aligns with the "silent failure" requirement, distinguishing between "cache not found" (expected on first run) and "permission denied" or "corrupted cache" (unexpected issues) could help users debug cache problems.

♻️ Optional: Add debug logging for cache read errors

 	// Use cached result if the last check was recent enough.
-	cache, _ := ReadCache(cacheDir)
+	cache, err := ReadCache(cacheDir)
+	if err != nil && !os.IsNotExist(err) {
+		slog.Debug("version check: cache read failed", slog.String("error", err.Error()))
+	}
 	if cache != nil && cache.IsFresh(now) {
 		return compareVersions(current, cache.LatestVersion)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/internal/versioncheck/check.go` around lines 99 - 103, The ReadCache call
currently ignores its error, so permission or corruption issues are silently
lost; update the logic around ReadCache(cacheDir) in check.go to capture the
returned error, and when an error is non-nil, log or debug-log it (distinguish
expected "not found" vs unexpected errors) before proceeding with the existing
fallback; keep the existing behavior of using cache only when cache != nil &&
cache.IsFresh(now) and then calling compareVersions(current,
cache.LatestVersion).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cli/integration/version_check_integration_test.go (1)

47-61: 💤 Low value

Mutating version.BuildVersion (a package global) inside a t.Parallel() test is racy.

version.BuildVersion is a process-wide variable. If any other parallel test in this package (now or later) reads or writes it, the writes here race with those reads under -race. The current siblings happen to be safe because Test_Integration_VersionCheck_DisabledByEnvVar calls t.Setenv (which forces serial execution), but this is fragile — adding any future parallel test that touches BuildVersion (or any cmd path that reads it) reintroduces the race.

Two safer options:

• Drop t.Parallel() from tests that mutate the global, or
• Plumb the version via an option/parameter instead of mutating the package var.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/integration/version_check_integration_test.go` around lines 47 - 61,
Test_Integration_VersionCheck_DoesNotErrorWithOldVersion mutates the
process-global version.BuildVersion while calling t.Parallel(), which creates a
data race; remove the t.Parallel() call from this test so the global mutation
and cleanup are executed serially, or alternatively refactor the code path
(e.g., cmd.New or the command execution) to accept an injected version value
instead of reading version.BuildVersion so the test can pass a custom version
without touching the package global; update
Test_Integration_VersionCheck_DoesNotErrorWithOldVersion to use the chosen
approach and keep the existing cleanup that restores version.BuildVersion if you
keep the global mutation.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes