fix(import): gate import progress ANSI on ShouldStyle (review follow-up to #1848) - #1867
Merged
gtrrz-victor merged 3 commits intoJul 28, 2026
Conversation
…soles The import progress spinner (#1848) drew its animated frames with cursor-control escapes (\r\033[K) whenever the writer was a terminal, gating only on IsTerminalWriter and bypassing interactive.ShouldStyle — the repo's single gate for writer-scoped ANSI. On a console that can't render ANSI (TERM=cygwin renders the ESC byte as a literal "←", GH #1267) or when NO_COLOR is set, every frame emitted "←[K" garbage. The per-frame \033[K this PR added widened a previously stop-line-only escape to the whole animation, so legacy Windows consoles regressed from clean bare-\r frames to visible garbage. - startUpdatableSpinner: fall back to the completion-line-only (ANSI-free) path unless w both is a terminal and ShouldStyle(w) — fixes it at the source for every spinner caller, not just import. - newImportProgressReporter: route non-styleable terminals to the plain per-session line path (same as non-TTY/ACCESSIBLE), so NO_COLOR/cygwin users still get per-session progress instead of a lone final line. - setup_import_test.go: correct a comment naming a test (TestNewImportProgressReporter_TTYAdvancesOnSkip) that does not exist. The terminal+!ShouldStyle branch is only observable against a real TTY, so it carries no unit test — matching the PR's existing PTY-bound coverage limits; the shouldStyle decision itself is already unit-tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01KYM4JTVVADFMT258B3020DV9
Contributor
There was a problem hiding this comment.
Pull request overview
This PR is a follow-up to the import-progress work, ensuring that spinner/progress output that relies on ANSI cursor-control sequences is additionally gated by interactive.ShouldStyle(w) (covering NO_COLOR and TERM=cygwin), preventing escape-sequence garbage on terminals that can’t reliably render ANSI.
Changes:
- Gate
startUpdatableSpinner’s animated frames on both “is a terminal” andinteractive.ShouldStyle(w)so\r\033[Kframes aren’t emitted when styling is disabled. - Route import progress on non-styleable terminals to the plain per-session line path (same as non-TTY/
ACCESSIBLE), rather than attempting an updatable spinner. - Fix an incorrect test comment that referenced a non-existent test name.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/entire/cli/progress.go | Adds ShouldStyle gating so live spinner frames using \033[K are only emitted when ANSI styling is allowed. |
| cmd/entire/cli/import_progress.go | Aligns import progress reporting with the same ShouldStyle gating, using plain lines when ANSI styling is suppressed. |
| cmd/entire/cli/setup_import_test.go | Updates a test comment to accurately describe existing coverage and constraints. |
…yle gates ShouldStyle already performs the terminal check internally and returns false for a non-terminal writer, so the extra !IsTerminalWriter(w) disjunct was redundant and did a second terminal probe. Collapse both gates to ShouldStyle alone; behavior is identical for non-TTY, NO_COLOR, and TERM=cygwin. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 01KYM5X08J7VHQ9WHN6M4TBH2Z
gtrrz-victor
merged commit Jul 28, 2026
36d5a13
into
eric/staqpro-771-enable-import-progress
10 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/949
Stacked on top of #1848 (base =
eric/staqpro-771-enable-import-progress). Addresses two findings from reviewing that PR. Review this PR's diff alone; merge #1848 first.Why
The import progress spinner added in #1848 emits its animated frames with cursor-control escapes (
\r\033[K), gating only onIsTerminalWriterand bypassinginteractive.ShouldStyle— the repo's documented single gate for writer-scoped ANSI (handlesNO_COLORandTERM=cygwin, the←[K-garbage case from GH #1267).TERM=cygwinunder mintty/Git Bash), the ESC byte renders as a literal←, so every frame shows←[K⣾ Importing…. Go's stdlib does not auto-enableENABLE_VIRTUAL_TERMINAL_PROCESSING, and this repo enables it nowhere.\033[Kfrom the stop line onto every animation frame, so legacy consoles regressed from clean bare-\rframes to visible garbage across the whole animation.ACCESSIBLEpath was already ANSI-free and is unaffected — agents and CI on any OS were never at risk.Changes
progress.go—startUpdatableSpinner: fall back to the completion-line-only (ANSI-free) path unlesswboth is a terminal andShouldStyle(w). Root-cause fix: every spinner caller now respectsNO_COLOR/cygwin, not just import.import_progress.go—newImportProgressReporter: route non-styleable terminals to the plain per-session line path (same as non-TTY/ACCESSIBLE), soNO_COLOR/cygwin users still get one progress line per session instead of a lone final line.setup_import_test.go: correct a comment that named a test (TestNewImportProgressReporter_TTYAdvancesOnSkip) which does not exist anywhere in the branch.Testing
go test ./cmd/entire/cli/ ./cmd/entire/cli/agentimport/— green.golangci-lint(v2.11.3) on both packages — 0 issues.terminal && !ShouldStyle) is only observable against a real TTY, matching feat(enable): show progress while importing existing sessions #1848's existing PTY-bound coverage limits. The pureshouldStyledecision it relies on is already unit-tested.🤖 Generated with Claude Code