Merge pull request #1867 from entireio/fix/import-progress-ansi-gating

fix(import): gate import progress ANSI on ShouldStyle (review follow-up to #1848)

Author: gtrrz-victor

cmd/entire/cli/import_progress.go

@@ -14,11 +14,16 @@ import (
 // tracks "Importing <agentName> sessions... (session i/N · turn j/M)";
 // callers must call the returned stop exactly once when the run finishes —
 // on both the success and error paths — so no spinner frame is left
-// dangling to corrupt whatever prints next. Otherwise (non-TTY, piped, or
-// ACCESSIBLE mode) it prints one plain, ANSI-free line per session from
-// SessionStart, and stop is a no-op.
+// dangling to corrupt whatever prints next. Otherwise — non-TTY, piped,
+// ACCESSIBLE mode, or a terminal that can't render ANSI (NO_COLOR,
+// TERM=cygwin; see interactive.ShouldStyle) — it prints one plain, ANSI-free
+// line per session from SessionStart, and stop is a no-op. The ShouldStyle
+// gate matches startUpdatableSpinner's own gate, so the spinner branch here
+// is taken only when the animation it drives can actually be rendered.
 func newImportProgressReporter(w io.Writer, agentName string) (progress *agentimport.Progress, stop func(success bool)) {
-	if !interactive.IsTerminalWriter(w) || IsAccessibleMode() {
+	// ShouldStyle already returns false for a non-terminal writer, so it
+	// subsumes the non-TTY/piped case as well as NO_COLOR and TERM=cygwin.
+	if IsAccessibleMode() || !interactive.ShouldStyle(w) {
 		return &agentimport.Progress{
 			SessionStart: func(sessionIndex, sessionTotal int, _, _ string, turnCount int) {
 				fmt.Fprintf(w, "Importing %s session %d/%d (%d %s)...\n",

cmd/entire/cli/progress.go

@@ -33,11 +33,18 @@ func startSpinner(w io.Writer, msg string) func(success bool) {
 
 // startUpdatableSpinner is startSpinner's variant for an operation whose
 // status text changes while it runs (e.g. "session 2/5 · turn 3/10"). update
-// replaces the message the next frame draws — or, on a non-terminal writer,
+// replaces the message the next frame draws — or, on the non-animated path,
 // the message stop's completion line uses. update is safe to call at any
 // point, including before the spinner's first frame draws and after stop
 // returns. stop behaves exactly like startSpinner's, rendering whichever
 // message update last set (or msg, if update was never called).
+//
+// The live animation is emitted only when w both is a terminal and can render
+// ANSI (interactive.ShouldStyle) — the frames use cursor-control escapes
+// (\r\033[K), which a legacy console that can't handle ANSI (e.g.
+// TERM=cygwin) renders as literal "←[K" garbage, and which NO_COLOR asks us
+// to suppress. When styling is off we fall back to the completion-line-only
+// path, so no escape byte is ever written to such a writer.
 func startUpdatableSpinner(w io.Writer, msg string) (update func(string), stop func(success bool)) {
 	var mu sync.Mutex
 	current := msg
@@ -52,7 +59,9 @@ func startUpdatableSpinner(w io.Writer, msg string) (update func(string), stop f
 		return current
 	}
 
-	if !interactive.IsTerminalWriter(w) {
+	// ShouldStyle already returns false for a non-terminal writer, so this
+	// single gate also covers the plain non-TTY case.
+	if !interactive.ShouldStyle(w) {
 		return setMsg, func(success bool) {
 			if success {
 				fmt.Fprintf(w, "✓ %s\n", getMsg())

cmd/entire/cli/setup_import_test.go

@@ -380,9 +380,13 @@ func TestRunSelectedImports_NonTTYProgressLines(t *testing.T) {
 // idempotent pass over an already-imported corpus (every turn hits
 // agentimport's TurnSkipped path, not TurnWritten) still prints one plain
 // progress line per session and reports the correct "0 imported" summary —
-// the non-TTY side of the P2 Codex's pre-push review caught (the TTY side is
-// covered by agentimport's TestRun_ReimportFiresTurnSkippedNotTurnWritten
-// and this package's TestNewImportProgressReporter_TTYAdvancesOnSkip).
+// the non-TTY side of the bug the P2 Codex's pre-push review caught. The TTY
+// side (a fully-skipped session must still sweep the spinner to turn M/M) is
+// covered at the agentimport layer by
+// TestRun_ReimportFiresTurnSkippedNotTurnWritten; the cli wiring it depends on
+// — newImportProgressReporter routing TurnSkipped and TurnWritten through one
+// shared advance path — has no direct test because the spinner branch only
+// runs against a real terminal.
 func TestRunSelectedImports_NonTTYProgressLines_Reimport(t *testing.T) {
 	// Not parallel: chdirs into a temp repo and performs real checkpoint writes.
 	dir := t.TempDir()