From 5d6cc737a55b71c886adfb6f3bdb75cd7c521081 Mon Sep 17 00:00:00 2001 From: Clay McGinnis Date: Thu, 30 Apr 2026 08:44:46 -0700 Subject: [PATCH 1/2] fix: move update notice to start and suppress during upgrade MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "newer version available" notice printed at the end of every command, including `wherobots upgrade` itself — so a successful upgrade still ended with a prompt to upgrade. Two changes: - Suppress the notice entirely when the user is running `upgrade`. - Try to print the notice at the start of output (with a 500ms wait for the background check), falling back to the end only if the check is still in flight. Make it visually prominent with a `[!]` lead and ANSI bold-yellow when stderr is a TTY. --- internal/commands/upgrade.go | 11 ++++++ internal/commands/upgrade_test.go | 30 +++++++++++++++ internal/version/check.go | 11 +++++- internal/version/check_test.go | 38 +++++++++++++++++++ main.go | 62 ++++++++++++++++++++++++++++--- 5 files changed, 144 insertions(+), 8 deletions(-) diff --git a/internal/commands/upgrade.go b/internal/commands/upgrade.go index 10cb2b9..7c24d3d 100644 --- a/internal/commands/upgrade.go +++ b/internal/commands/upgrade.go @@ -29,6 +29,17 @@ type upgradeOptions struct { currentVersion string } +// IsUpgradeInvocation reports whether the given args resolve to the +// "upgrade" command. Used by main to suppress the update-available notice +// when the user is already upgrading. +func IsUpgradeInvocation(root *cobra.Command, args []string) bool { + cmd, _, err := root.Find(args) + if err != nil || cmd == nil { + return false + } + return cmd.Name() == "upgrade" +} + // AddUpgradeCommand registers the "upgrade" subcommand on the root command. // It requires the current build version to display during the upgrade flow. func AddUpgradeCommand(root *cobra.Command, currentVersion string) { diff --git a/internal/commands/upgrade_test.go b/internal/commands/upgrade_test.go index 631824f..727ee32 100644 --- a/internal/commands/upgrade_test.go +++ b/internal/commands/upgrade_test.go @@ -7,8 +7,38 @@ import ( "path/filepath" "runtime" "testing" + + "github.com/spf13/cobra" ) +func TestIsUpgradeInvocation(t *testing.T) { + root := &cobra.Command{Use: "wherobots"} + other := &cobra.Command{Use: "list", Run: func(*cobra.Command, []string) {}} + root.AddCommand(other) + AddUpgradeCommand(root, "1.0.0") + + tests := []struct { + name string + args []string + want bool + }{ + {"upgrade alone", []string{"upgrade"}, true}, + {"upgrade with flag", []string{"upgrade", "--tag", "v1.0.1"}, true}, + {"other command", []string{"list"}, false}, + {"no args", []string{}, false}, + {"unknown command", []string{"nope"}, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := IsUpgradeInvocation(root, tt.args) + if got != tt.want { + t.Fatalf("IsUpgradeInvocation(%v) = %v, want %v", tt.args, got, tt.want) + } + }) + } +} + func TestDetectPlatform(t *testing.T) { osName, archName, err := detectPlatform() if err != nil { diff --git a/internal/version/check.go b/internal/version/check.go index 366f2d7..b2f1d46 100644 --- a/internal/version/check.go +++ b/internal/version/check.go @@ -67,10 +67,17 @@ func CheckInBackground(ctx context.Context, currentVersion string) <-chan *Resul // Collect waits briefly for the background check result. Returns nil if no // update is available, the check was skipped, or the timeout elapsed. func Collect(ch <-chan *Result) *Result { + return TryCollect(ch, collectTimeout) +} + +// TryCollect waits up to timeout for a result. Returns nil on timeout (the +// caller may try again later) or when the channel was closed without a +// result. +func TryCollect(ch <-chan *Result, timeout time.Duration) *Result { select { case r := <-ch: return r - case <-time.After(collectTimeout): + case <-time.After(timeout): return nil } } @@ -78,7 +85,7 @@ func Collect(ch <-chan *Result) *Result { // FormatNotice returns the human-readable update message to display. func FormatNotice(r *Result) string { return fmt.Sprintf( - "A newer version of the Wherobots CLI is available: %s (current: %s).\nRun `wherobots upgrade` to update.", + "[!] A newer version of the Wherobots CLI is available: %s (current: %s).\n Run `wherobots upgrade` to update.", r.Latest, r.Current, ) } diff --git a/internal/version/check_test.go b/internal/version/check_test.go index 9a29d22..4535921 100644 --- a/internal/version/check_test.go +++ b/internal/version/check_test.go @@ -2,8 +2,46 @@ package version import ( "testing" + "time" ) +func TestTryCollect_ReturnsResult(t *testing.T) { + ch := make(chan *Result, 1) + want := &Result{Current: "1.0.0", Latest: "1.0.1", Outdated: true} + ch <- want + close(ch) + + got := TryCollect(ch, 50*time.Millisecond) + if got != want { + t.Fatalf("TryCollect returned %+v, want %+v", got, want) + } +} + +func TestTryCollect_TimeoutLeavesChannelReadable(t *testing.T) { + ch := make(chan *Result, 1) + + if got := TryCollect(ch, 10*time.Millisecond); got != nil { + t.Fatalf("TryCollect should time out and return nil, got %+v", got) + } + + want := &Result{Current: "1.0.0", Latest: "1.0.1", Outdated: true} + ch <- want + close(ch) + + if got := TryCollect(ch, 50*time.Millisecond); got != want { + t.Fatalf("second TryCollect returned %+v, want %+v", got, want) + } +} + +func TestTryCollect_ClosedChannel(t *testing.T) { + ch := make(chan *Result) + close(ch) + + if got := TryCollect(ch, 50*time.Millisecond); got != nil { + t.Fatalf("TryCollect on closed channel should return nil, got %+v", got) + } +} + func TestParseSemver(t *testing.T) { tests := []struct { input string diff --git a/main.go b/main.go index 02749c7..9267c49 100644 --- a/main.go +++ b/main.go @@ -3,7 +3,9 @@ package main import ( "context" "fmt" + "io" "os" + "time" "wherobots/cli/internal/commands" "wherobots/cli/internal/config" @@ -17,6 +19,12 @@ var ( date = "unknown" ) +// startNoticeWait is how long we wait for the background update check to +// finish before executing the user's command. Spec loading typically gives +// the check enough time to return; if not, we fall back to printing at the +// end so we never block the command on a slow network probe. +const startNoticeWait = 500 * time.Millisecond + func main() { if err := run(context.Background()); err != nil { fmt.Fprintln(os.Stderr, err.Error()) @@ -50,16 +58,58 @@ func run(ctx context.Context) error { root.Version = versionString root.Short = fmt.Sprintf("Wherobots CLI %s", buildVersion) commands.AddUpgradeCommand(root, buildVersion) + + // Suppress the "update available" notice when the user is already running + // `upgrade` — the check races with the upgrade itself and would otherwise + // nag about the very version that just got installed. + suppressNotice := commands.IsUpgradeInvocation(root, os.Args[1:]) + + noticeShown := false + if !suppressNotice { + if result := version.TryCollect(updateCh, startNoticeWait); result != nil { + printUpdateNotice(os.Stderr, result) + noticeShown = true + } + } + execErr := root.ExecuteContext(ctx) - // After the command finishes, print an update notice if one is available. - if result := version.Collect(updateCh); result != nil { - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, version.FormatNotice(result)) - if execErr != nil { - fmt.Fprintln(os.Stderr, "Note: your CLI is out of date. Run `wherobots upgrade` to update — it may resolve this issue.") + // Fallback: if the check hadn't returned by the time we started executing, + // print the notice at the end rather than dropping it. + if !suppressNotice && !noticeShown { + if result := version.Collect(updateCh); result != nil { + fmt.Fprintln(os.Stderr, "") + printUpdateNotice(os.Stderr, result) + noticeShown = true } } + if execErr != nil && noticeShown { + fmt.Fprintln(os.Stderr, "Note: your CLI is out of date. Run `wherobots upgrade` to update — it may resolve this issue.") + } + return execErr } + +func printUpdateNotice(w io.Writer, r *version.Result) { + notice := version.FormatNotice(r) + if isTTY(w) { + notice = "\033[1;33m" + notice + "\033[0m" + } + fmt.Fprintln(w, notice) + fmt.Fprintln(w, "") +} + +// isTTY reports whether w is an *os.File pointing at a terminal. Stdlib only; +// avoids pulling in a tty-detection dependency for a single notice. +func isTTY(w io.Writer) bool { + f, ok := w.(*os.File) + if !ok { + return false + } + fi, err := f.Stat() + if err != nil { + return false + } + return (fi.Mode() & os.ModeCharDevice) != 0 +} From 15388c157158806a68f2a4e9a3e5a2f4be6be835 Mon Sep 17 00:00:00 2001 From: Clay McGinnis Date: Thu, 30 Apr 2026 08:46:53 -0700 Subject: [PATCH 2/2] keep notice at end of output, drop start-of-output wait Reverts the 500ms TryCollect at the top of run(): the simpler end-of- output flow has no startup-latency cost. The upgrade-command suppression and the prominent [!]/bold-yellow formatting stay. --- internal/version/check.go | 9 +------- internal/version/check_test.go | 38 ---------------------------------- main.go | 28 ++++--------------------- 3 files changed, 5 insertions(+), 70 deletions(-) diff --git a/internal/version/check.go b/internal/version/check.go index b2f1d46..b825beb 100644 --- a/internal/version/check.go +++ b/internal/version/check.go @@ -67,17 +67,10 @@ func CheckInBackground(ctx context.Context, currentVersion string) <-chan *Resul // Collect waits briefly for the background check result. Returns nil if no // update is available, the check was skipped, or the timeout elapsed. func Collect(ch <-chan *Result) *Result { - return TryCollect(ch, collectTimeout) -} - -// TryCollect waits up to timeout for a result. Returns nil on timeout (the -// caller may try again later) or when the channel was closed without a -// result. -func TryCollect(ch <-chan *Result, timeout time.Duration) *Result { select { case r := <-ch: return r - case <-time.After(timeout): + case <-time.After(collectTimeout): return nil } } diff --git a/internal/version/check_test.go b/internal/version/check_test.go index 4535921..9a29d22 100644 --- a/internal/version/check_test.go +++ b/internal/version/check_test.go @@ -2,46 +2,8 @@ package version import ( "testing" - "time" ) -func TestTryCollect_ReturnsResult(t *testing.T) { - ch := make(chan *Result, 1) - want := &Result{Current: "1.0.0", Latest: "1.0.1", Outdated: true} - ch <- want - close(ch) - - got := TryCollect(ch, 50*time.Millisecond) - if got != want { - t.Fatalf("TryCollect returned %+v, want %+v", got, want) - } -} - -func TestTryCollect_TimeoutLeavesChannelReadable(t *testing.T) { - ch := make(chan *Result, 1) - - if got := TryCollect(ch, 10*time.Millisecond); got != nil { - t.Fatalf("TryCollect should time out and return nil, got %+v", got) - } - - want := &Result{Current: "1.0.0", Latest: "1.0.1", Outdated: true} - ch <- want - close(ch) - - if got := TryCollect(ch, 50*time.Millisecond); got != want { - t.Fatalf("second TryCollect returned %+v, want %+v", got, want) - } -} - -func TestTryCollect_ClosedChannel(t *testing.T) { - ch := make(chan *Result) - close(ch) - - if got := TryCollect(ch, 50*time.Millisecond); got != nil { - t.Fatalf("TryCollect on closed channel should return nil, got %+v", got) - } -} - func TestParseSemver(t *testing.T) { tests := []struct { input string diff --git a/main.go b/main.go index 9267c49..35136c3 100644 --- a/main.go +++ b/main.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "os" - "time" "wherobots/cli/internal/commands" "wherobots/cli/internal/config" @@ -19,12 +18,6 @@ var ( date = "unknown" ) -// startNoticeWait is how long we wait for the background update check to -// finish before executing the user's command. Spec loading typically gives -// the check enough time to return; if not, we fall back to printing at the -// end so we never block the command on a slow network probe. -const startNoticeWait = 500 * time.Millisecond - func main() { if err := run(context.Background()); err != nil { fmt.Fprintln(os.Stderr, err.Error()) @@ -64,30 +57,18 @@ func run(ctx context.Context) error { // nag about the very version that just got installed. suppressNotice := commands.IsUpgradeInvocation(root, os.Args[1:]) - noticeShown := false - if !suppressNotice { - if result := version.TryCollect(updateCh, startNoticeWait); result != nil { - printUpdateNotice(os.Stderr, result) - noticeShown = true - } - } - execErr := root.ExecuteContext(ctx) - // Fallback: if the check hadn't returned by the time we started executing, - // print the notice at the end rather than dropping it. - if !suppressNotice && !noticeShown { + if !suppressNotice { if result := version.Collect(updateCh); result != nil { fmt.Fprintln(os.Stderr, "") printUpdateNotice(os.Stderr, result) - noticeShown = true + if execErr != nil { + fmt.Fprintln(os.Stderr, "Note: your CLI is out of date. Run `wherobots upgrade` to update — it may resolve this issue.") + } } } - if execErr != nil && noticeShown { - fmt.Fprintln(os.Stderr, "Note: your CLI is out of date. Run `wherobots upgrade` to update — it may resolve this issue.") - } - return execErr } @@ -97,7 +78,6 @@ func printUpdateNotice(w io.Writer, r *version.Result) { notice = "\033[1;33m" + notice + "\033[0m" } fmt.Fprintln(w, notice) - fmt.Fprintln(w, "") } // isTTY reports whether w is an *os.File pointing at a terminal. Stdlib only;