Skip to content

Conversation

@wesleytodd
Copy link
Contributor

Description

Checking stdin would allow for cases where stdout is redirected but you still want colors to be maintained (which is rather common IME). Since it is relatively less common (and there are few workarounds other than setting FORCE_COLORS that I know of) to have stdin be a TTY but still want colors, it seems like this would be a good change for the next major. It is a breaking change, so totally understand if it would need to wait.

Testing Instructions

I didn't even test this, but happy to if you think this has a chance of landing.

@wesleytodd wesleytodd requested a review from a team as a code owner September 18, 2025 00:07
@vercel
Copy link
Contributor

vercel bot commented Sep 18, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
examples-basic-web Ready Ready Preview Comment Sep 18, 2025 0:07am
examples-designsystem-docs Ready Ready Preview Comment Sep 18, 2025 0:07am
examples-tailwind-web Ready Ready Preview Comment Sep 18, 2025 0:07am
examples-vite-web Ready Ready Preview Comment Sep 18, 2025 0:07am

@vercel
Copy link
Contributor

vercel bot commented Sep 18, 2025

@wesleytodd is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

_ => None,
});
let should_strip_ansi = env_setting.unwrap_or_else(|| !atty::is(atty::Stream::Stdout));
let should_strip_ansi = env_setting.unwrap_or_else(|| !atty::is(atty::Stream::Stdin));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let should_strip_ansi = env_setting.unwrap_or_else(|| !atty::is(atty::Stream::Stdin));
let should_strip_ansi = env_setting.unwrap_or_else(|| !atty::is(atty::Stream::Stdout));

The color detection logic incorrectly checks stdin instead of stdout for TTY status, which breaks color output in common shell redirection scenarios.

View Details

Analysis

ColorConfig::infer() checks stdin instead of stdout for TTY, breaking shell redirection

What fails: ColorConfig::infer() in crates/turborepo-ui/src/lib.rs:167 uses atty::is(atty::Stream::Stdin) instead of atty::is(atty::Stream::Stdout) for color output decisions

How to reproduce:

# Should show colors but won't (stdin is not TTY, stdout is TTY):
echo "data" | turborepo build

# Should disable colors but won't (stdin is TTY, stdout is not TTY):  
turborepo build > logfile.txt

Result: Colors are incorrectly enabled/disabled based on input source rather than output destination, causing missing colors in pipelines and ANSI codes polluting log files

Expected: Color decisions should check stdout TTY status per atty documentation examples and standard Unix convention - colors are output to stdout/stderr, so output stream TTY status determines color support

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol, well this is funny. This check breaks my use case. I assume this is some AI comment, so I will wait for a human to address this.

@anthonyshew
Copy link
Contributor

Hey, Wes, thanks for the PR.

I'm a little suspicious of the semantics here. I think of stdin and stdout as their own separate concepts, so using one to describe the behavior is throwing me off. For instance, stdin being a TTY doesn't guarantee the output will be displayed in a color-capable terminal, so you'd get the ASCII spewed at you, even though stdout tried to do the right thing.

Example: command < /dev/tty > logfile.txt could output [31mError:[0m Something went wrong which doesn't seem like what we want.

I might not be realizing if this is common behavior, though. Do you have examples of tools that do this?

Another thing is that we have --color and --no-color in our global flags. Would that be suitable for handling your use case?

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.

2 participants