Skip to content

fix(update): make "update available" nag actually print#188

Merged
AlephNotation merged 2 commits intomainfrom
fix/update-nag-not-printing
May 6, 2026
Merged

fix(update): make "update available" nag actually print#188
AlephNotation merged 2 commits intomainfrom
fix/update-nag-not-printing

Conversation

@AlephNotation
Copy link
Copy Markdown
Contributor

Why

Jordan flagged in Slack that he had no idea a newer vers had shipped — the "update available" banner never printed for him. Investigation showed the startup check has been silently broken since it landed.

The bug

PersistentPreRunE in cmd/root.go did this:

if !skipUpdateCheck && update.ShouldCheckForUpdate() {
    update.UpdateCheckTime()        // (1) bumps NextCheck *before* the fetch
    go func() {                     // (2) detached goroutine
        hasUpdate, latestVersion, err := update.CheckForUpdates(...)
        if err == nil && hasUpdate {
            fmt.Printf("💡 Update available: ...")
        }
    }()
}
  1. UpdateCheckTime() immediately pushes NextCheck an hour into the future.
  2. The fetch runs in a detached goroutine with no synchronization.
  3. Short commands like vers ps exit in ~50ms — well before the HTTP call to api.github.com finishes — so the banner never prints and the goroutine is killed mid-flight.
  4. On the next run, ShouldCheckForUpdate() returns false (we just bumped NextCheck), so no check happens either.

Net effect: the nag was effectively never shown. Reproduced locally with v0.9.0 against the live v0.10.0 release — banner never appeared.

What this PR does

Replaces the racy goroutine with update.MaybeNotifyUpdate, which:

  • Caches the latest known release tag in ~/.vers/config.json (update_check.latest_version). If the cached tag is newer than the running build, the banner prints synchronously with zero network I/O on every invocation. This is the path Jordan would hit on his second vers run.
  • Performs a single, synchronous, bounded (800ms) HTTP refresh once per check interval — no goroutine, no race. 800ms is well below the threshold where users notice CLI lag, but enough for api.github.com from any reasonable network.
  • Only advances NextCheck on a successful fetch. On failure (timeout, 5xx), uses a short 5-minute backoff so a flaky network at the moment of first launch doesn't suppress the nag for an hour.
  • Skips dev/dirty builds via a centralized IsDevVersion helper (dev, unknown, dev-<sha>, *-dirty, empty).
  • Does proper semver-aware comparison so v0.9.0 < v0.10.0. The previous string-equality check would also flag downgrades, which is wrong.
  • Writes the banner to stderr (was stdout) so piped output of commands like vers ps | grep ... isn't contaminated.

Smoke test

Built v0.9.0 locally, pointed it at a fresh $HOME, ran twice against real GitHub:

$ HOME=/tmp/x vers ps
💡 vers update available: v0.9.0 -> v0.10.0 (run 'vers upgrade')

HEAD: 37d2f5c5-...
...
$ cat /tmp/x/.vers/config.json
{ "update_check": { ..., "latest_version": "v0.10.0" } }

$ time HOME=/tmp/x vers ps                 # warm cache
💡 vers update available: v0.9.0 -> v0.10.0 (run 'vers upgrade')
...
real    0m0.265s                            # no GitHub call

Tests

New file internal/update/update_test.go covers:

  • TestIsNewerSemver — semver comparison edge cases (0.9 < 0.10, missing components, prereleases, lexical fallback).
  • TestIsDevVersion — dev / dirty / unknown / clean.
  • TestMaybeNotifyUpdate_PrintsFromCacheWithoutNetwork — fast path: pre-seeds cache, asserts banner prints and zero HTTP calls (httptest server fails the test if hit).
  • TestMaybeNotifyUpdate_RefreshesWhenStale — slow path: stale NextCheck, fake server returns v0.10.0, asserts exactly 1 HTTP call, banner printed, cache + NextCheck updated.
  • TestMaybeNotifyUpdate_NetworkFailureDoesNotSuppressForFullInterval — server returns 500, asserts NextCheck is the 5-min backoff, not 1h.
  • TestMaybeNotifyUpdate_DevVersionSkipsEverything — dev build → no network, no print.
  • TestMaybeNotifyUpdate_NoBannerWhenAlreadyOnLatest — same version → no banner.
  • TestPrintUpdateBannerFormat — banner wording regression test (downstream onboarding docs reference the "vers upgrade" phrase).
ok  	github.com/hdresearch/vers-cli/internal/update	0.255s

Full unit test suite (make test-unit minus the unrelated tunnel integration test) passes.

Out of scope / follow-ups

  • Static-linking question Jordan also raised: confirmed via file vers-linux-amd64 on the latest GitHub release — already statically linked. His dynamic copy must come from somewhere other than the Releases page (likely an older bundle inside an agent-services image). Worth pinging him for which -a vers once he installs via install.sh.
  • vers upgrade itself is unchanged. Once this PR lands, users on any release ≥ this one will start seeing the nag and discover vers upgrade organically.

The startup update check was racy and broken in practice:

1. `UpdateCheckTime()` ran *before* the HTTP fetch, immediately
   pushing NextCheck out by the full check interval (1h).
2. The fetch ran in a detached `go func()` with no synchronization.
3. Short commands like `vers ps` exit in ~50ms — well before the
   goroutine's HTTP call to api.github.com completes — so the banner
   never printed and the goroutine got killed mid-flight.
4. On the next invocation, ShouldCheckForUpdate() returned false (we
   bumped NextCheck on the previous run), so no check happened either.

Net effect: the nag was effectively never shown. Users (e.g. Jordan)
who built from source or grabbed an older bundled binary had no way
to discover that newer releases existed without manually running
`vers upgrade`.

This change introduces `update.MaybeNotifyUpdate` which:

- Caches the latest known release tag in `~/.vers/config.json`
  (`update_check.latest_version`). If a previously-cached version
  is newer than the running build, the banner prints synchronously
  on every invocation with zero network I/O.
- Performs a single, synchronous, bounded (800ms) HTTP refresh once
  per check interval — no goroutine, no race.
- Only advances NextCheck after a *successful* fetch. On failure,
  uses a short 5-minute backoff so a flaky network at the moment of
  first launch doesn't suppress the nag for an hour.
- Skips dev/dirty builds via a centralized `IsDevVersion` helper.
- Does proper semver-aware comparison so e.g. v0.9.0 < v0.10.0
  (the previous string-equality check would have flagged any
  difference, including downgrades).

The banner now writes to stderr (was stdout) so it doesn't
contaminate piped output of commands like `vers ps | grep ...`.

Fully covered by unit tests in internal/update/update_test.go:
fast-path-without-network, network refresh, failure backoff,
dev-version skip, no-banner-when-on-latest, and a banner format
regression test.
…hatch

CI integration test `TestRepoLifecycle` failed on PR #188:

  expected 'v1' in quiet tag list, got:
  💡 vers update available: v0.0.0-20260506213344-eb1e5d25fa7c -> v0.10.0 (run 'vers upgrade')

Two issues:

1. The integration test runner builds the CLI via `go build` against the
   PR merge commit, producing a Go module pseudo-version like
   `v0.0.0-<timestamp>-<commit>`. The previous `IsDevVersion` only
   matched `dev` / `dev-*` / `*-dirty`, so the pseudo-version sailed
   through, hit the live GitHub API, and printed the banner.

2. `testutil.RunVers` uses `cmd.CombinedOutput()`, so even though the
   banner correctly writes to stderr, it still ended up in the buffer
   that tests parse. Tests doing `strings.TrimSpace(out) != "v1"` are
   particularly fragile.

Fixes:

- IsDevVersion now also matches `v0.0.0-...` pseudo-versions. This
  protects `go install`, `go run`, and any module-graph build against
  an untagged commit — none of which represent a published release the
  user can "upgrade" to.
- New `VERS_NO_UPDATE_CHECK` env var (also accepts `NO_UPDATE_NOTIFIER`
  for cross-tool consistency with npm) silences the check entirely
  before any other logic runs. Truthy parsing follows convention:
  unset/empty/0/false/no/off mean "don't suppress", anything else
  suppresses.
- testutil.RunVers and RunVersInDir now set `VERS_NO_UPDATE_CHECK=1`
  unconditionally so future integration tests are immune to this class
  of contamination even if a real release ships during a CI run.

New tests cover pseudo-version detection, both env-var names, and
falsy-value rejection.
@AlephNotation AlephNotation merged commit f21b7f8 into main May 6, 2026
6 checks passed
@AlephNotation AlephNotation deleted the fix/update-nag-not-printing branch May 6, 2026 23:56
AlephNotation added a commit that referenced this pull request May 7, 2026
Adopt the lowercase-prefix convention used by serious dev tools (gcc,
rustc, clippy, go) for status messages, and drop decorative emoji
that were inherited from earlier prototypes.

Rationale:

- Emoji and unusual glyphs in CLI output break programmatic
  consumption (grep / awk / jq / log shippers must handle multi-byte
  noise), break alignment in fixed-width displays (most emoji are
  East-Asian-Wide and occupy 2 cells inconsistently across terminals),
  and turn into '?' on non-UTF-8 transports (serial consoles, some
  log re-encoders, older Windows terminals).
- The recent CI failure on #188 (`TestRepoLifecycle` couldn't find
  'v1' because the update banner was in the buffer) is the natural
  consequence of putting decorative output through stdout.
- None of the tools we want vers-cli to feel adjacent to (git, go,
  cargo, kubectl, terraform, docker, aws, gcloud, ripgrep) decorate
  their output with emoji. The closest, `gh`, uses only the
  Unicode check/cross glyphs and TTY-gates them.

Replacements applied:

  ✓ <message>            -> <message>                 (drop; success implied)
  ✗ <message>            -> error: <message>          (lowercase compiler-style prefix)
  ⚠️  <message>          -> warning: <message>
  💡 <message>           -> note: <message>
  📧 Verification email  -> Verification email        (drop; text already explanatory)
  Println("✓")           -> Println("ok")
  Println("✗")           -> Println("failed")
  ├─ / └─                -> |- / \\-                   (ASCII tree in commit lineage)
  →                      -> ->                        (ASCII arrow everywhere)

Also lowercased the first character following the new lowercase
prefixes (`error: failed to ...` rather than `error: Failed to ...`)
to match clippy/rustc/gcc convention exactly.

Output changes are user-facing but minor; they make grepping CLI
output more reliable and the project look less prototype-y. No
behavior changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant