Gate update check on terminal-attachment, not NO_COLOR

Author: jclusso

cmd/root.go

@@ -246,15 +246,12 @@ func newRootCmd(v string) *cobra.Command {
 func Execute() {
 	root := newRootCmd(version)
 
-	// Pre-flight skip: the opt-out env var, CI, stderr TTY-ness, and the build
-	// version are all known before the command parses its flags, so when any
-	// of them opts out we never launch the network call at all. (JSONMode and
-	// Quiet come from flags parsed during Execute, so they can't be evaluated
-	// here — they're handled by the post-command notice gate below, which may
-	// still suppress a notice whose cache refresh we deliberately allowed.)
+	// Skip the network call entirely for opt-outs knowable before flags parse.
+	// Uses IsTerminal, not IsTTY, so NO_COLOR (a styling pref) still gets checks.
+	// JSONMode/Quiet are flag-derived and gate only the notice, below.
 	preSkip := updater.ShouldSkip(updater.Conditions{
 		CurrentVersion: version,
-		StderrTTY:      ui.IsTTY(root.ErrOrStderr()),
+		StderrTTY:      ui.IsTerminal(root.ErrOrStderr()),
 		OptOut:         env.UpdateNotifierOptOut(),
 	})
 
@@ -264,28 +261,24 @@ func Execute() {
 	defer updCancel()
 	resultCh := make(chan updater.Result, 1)
 	if preSkip == updater.SkipNone {
+		// Run even when we may end up skipping the notice (e.g. JSON mode) so
+		// the cache still refreshes; the gate below decides on printing.
 		go func() {
-			// Conservatively run the network call even when we might end up
-			// skipping the *notice* (e.g. JSON mode), so the cache refresh
-			// still happens. The notice gate below makes the final call.
 			resultCh <- updater.Check(updCtx, version, updater.CacheDir())
 		}()
 	}
 
 	runErr := root.Execute()
 
-	// Decide whether the *notice* should be considered for printing. This
-	// is the gate the user perceives — silent skips don't even touch the
-	// channel below, so a hung check doesn't delay exit in those cases. When
-	// the pre-flight already opted out, no goroutine is running, so we keep
-	// that reason and never wait on the channel.
+	// Re-check with the now-parsed flags to gate the notice. If the pre-flight
+	// already opted out, no goroutine ran, so keep that reason.
 	skip := preSkip
 	if skip == updater.SkipNone {
 		skip = updater.ShouldSkip(updater.Conditions{
 			CurrentVersion: version,
 			JSONMode:       jsonOutput,
 			Quiet:          quietMode,
-			StderrTTY:      ui.IsTTY(root.ErrOrStderr()),
+			StderrTTY:      ui.IsTerminal(root.ErrOrStderr()),
 			OptOut:         env.UpdateNotifierOptOut(),
 		})
 	}

internal/ui/spinner.go

@@ -49,6 +49,12 @@ var isTerminal = func(w io.Writer) bool {
 	return term.IsTerminal(int(f.Fd()))
 }
 
+// IsTerminal reports whether w is a terminal, ignoring NO_COLOR. Use it for
+// interactivity decisions; IsTTY (which honors NO_COLOR) is for styling.
+func IsTerminal(w io.Writer) bool {
+	return isTerminal(w)
+}
+
 // terminalWidth returns the column count of the terminal w is writing to,
 // or 0 if w isn't a TTY or the size can't be determined. Re-measured on
 // every frame so the progress bar tracks terminal resizes.

internal/ui/spinner_test.go

@@ -59,6 +59,11 @@ func TestIsTTY_NoColor(t *testing.T) {
 		t.Errorf("NO_COLOR=yes: IsTTY should be false, got true")
 	}
 
+	// IsTerminal ignores NO_COLOR, so it stays true here.
+	if !IsTerminal(&buf) {
+		t.Errorf("NO_COLOR set: IsTerminal should stay true")
+	}
+
 	// Sanity-check style helpers respect the propagated decision.
 	if got := Cyan("x", IsTTY(&buf)); got != "x" {
 		t.Errorf("Cyan with NO_COLOR set should return %q, got %q", "x", got)