fix(cli): suppress ANSI color when stdout is not a terminal by mrunalp · Pull Request #3026 · NVIDIA/OpenShell

mrunalp · 2026-08-31T04:37:33Z

Summary

The CLI colorized output unconditionally, so piping any command matched against ANSI escape bytes the caller could not see. This resolves colorization once at startup from --color, NO_COLOR, CLICOLOR_FORCE, and whether stdout is a terminal, and routes every styled surface through that one setting.

Related Issue

Fixes #3025

Changes

Add crates/openshell-cli/src/color.rs: a process-wide switch resolved once in run_async before any output is written.
Command modules import the module's Colorize trait instead of owo_colors::OwoColorize. The method names match, so the ~450 call sites are unchanged, but each consults the switch when it renders and delegates to owo-colors so the escape bytes stay identical when color is on. The two traits collide by design: importing both in one module is an ambiguity error, which keeps unconditional coloring from creeping back in. New call sites added by other work inherit the fix without anyone having to remember.
Bring the other styled paths under the same setting, since each carried its own default:
- tracing_subscriber formats with ANSI on, does no terminal detection, and writes to stdout — so openshell -v … | … leaked escapes exactly like the tables did. It now takes the setting via with_ansi.
- indicatif and dialoguer both style through console, which has its own detection but cannot learn about --color. Overriding console's global switch covers every progress bar and prompt rather than only the ones constructed today. Both the stdout and stderr switches are set, since prompts and bars draw to stderr.
- miette renders errors through its own handler, likewise unaware of --color, so init installs one built from the setting.
Add a global --color <auto|always|never> flag (OPENSHELL_COLOR).
Declare console as a direct dependency. It was already in the tree transitively via both indicatif and dialoguer, which resolve to the same version; it is now declared because we call it directly.

Resolution order, highest precedence first: --color, then NO_COLOR (any non-empty value), then CLICOLOR_FORCE, then whether stdout is a terminal.

Two behaviors worth a reviewer's attention:

The decision is made against stdout even for text written to stderr. A single switch keeps every call site consistent without each one having to declare its destination stream, and stdout is the stream that gets parsed. Redirecting stdout therefore also drops color from stderr diagnostics; --color always is the escape hatch.
Column alignment is unaffected. The format spec is forwarded to the inner Display, so widths measure text rather than text plus escapes.

Testing

Verified against the built binary across both streams and all four styling paths:

surface	stream	default	`--color never`	`NO_COLOR`	`--color always`
owo tables	pipe	plain	plain	plain	color
owo tables	tty	color	plain	plain	color
tracing + miette	pipe	plain	plain	plain	color
tracing + miette	tty	color	plain	plain	color

The reporting pattern from the issue ('^<sandbox>[[:space:]].*[[:space:]]<port>[[:space:]].*[[:space:]]running') now matches on default piped output.

10 unit tests cover the precedence rules, that enabled output stays byte-identical to owo-colors, that padding measures text rather than escapes, and that the console override governs dialoguer rendering. 8 integration tests drive the real binary through a pipe. Each integration test that asserts an absence of escapes is paired with a --color always positive control, so it cannot pass vacuously — I checked this by forcing ColorChoice::Always to simulate the old behavior, which fails 4 of 6 while the controls still pass.

mise run pre-commit passes
Unit tests added/updated
E2E tests added/updated (if applicable) — not applicable; this is CLI-local output formatting with no gateway or sandbox involvement

mise run test passes on the full workspace.

Checklist

Follows Conventional Commits
Commits are signed off (DCO)
Architecture docs updated (if applicable) — not applicable; no subsystem boundary or data-flow change. User-facing docs updated in docs/sandboxes/manage-sandboxes.mdx, and the flag and environment variables are documented in the openshell-cli skill reference.

Follow-ups (not in this PR)

Found while investigating, deliberately left out of scope:

forward list has no -o json. OutputFormat with -o/--output is already threaded through roughly two dozen other subcommands while ForwardCommands::List is a bare variant. This is the durable fix for integrators and is what forced the fragile text parse in the first place.
forward list reports running from PID liveness plus an argv match, with no socket probe, so a wedged tunnel still reports running. [OpenShell] 'openshell forward list' reports STATUS: dead but there is no supervisor, auto-restart, or alerting — silently unreachable dashboards #874 already covers this area.

copy-pr-bot · 2026-08-31T04:37:36Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

natedemoss · 2026-08-31T05:02:14Z

might be misreading the diff, but i think init hands console and miette the stdout bool even under auto, so openshell ... 2> build.log from a terminal puts escapes in the log -- and the tests won't catch it since Command::output() pipes both streams

Review feedback on NVIDIA#3026. Resolving one answer from stdout and handing it to every library meant a redirected stream inherited the other stream's terminal check. Running `openshell ... 2> build.log` from a terminal wrote escapes into the log, because console's stderr switch and miette's handler were both given stdout's answer. That is worse than the behavior before this branch, where both libraries did their own per-stream detection. Resolve `auto` separately for stdout and stderr and hand each library the answer for the stream it writes to: tracing and console's stdout switch get stdout, miette and console's stderr switch get stderr. The owo-colors wrapper is the exception, since its call sites are split across println! and eprintln! and a Painted value cannot tell which macro will consume it; it styles only when both streams accept escapes, erring toward plain text rather than risking a redirected stream. Existing tests could not catch this: Command::output gives both streams pipes, so a per-stream decision and a single stdout-derived one look identical. Add a test that puts stdout on a pty and stderr on a pipe, which fails when stderr is handed stdout's answer. Replace CLICOLOR_FORCE with FORCE_COLOR. The clicolors spec does not say how to treat `0`, and implementations that special-case it disagree with force-color.org, which keys on presence and non-emptiness only. Using FORCE_COLOR gives it the same rule as NO_COLOR: set and non-empty means yes, whatever the value. Nothing depended on CLICOLOR_FORCE, which was introduced earlier on this branch and never released. Carry the whole style in an owo_colors::Style rather than dispatching a local enum through a six-arm match, and merge styles when chaining so `x.green().bold()` emits one `\x1b[32;1m...\x1b[0m` instead of nesting two wrappers. No call site styles already-styled text, so merging is safe; the emitted bytes are shorter and there is a single reset. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp · 2026-08-31T15:22:41Z

You read it right, and both halves of that are correct — thanks, this was a real regression.

init resolved one answer from stdout().is_terminal() and handed it to console's stderr switch and to miette. So openshell ... 2> build.log from a terminal put escapes in the log. Worse, it was a regression against main: console and miette both do their own per-stream detection, and I overrode it with the wrong stream's answer.

And the tests could not have caught it. Command::output() gives both streams pipes, so a per-stream decision and a single stdout-derived one produce identical output.

Fixed in 8188704:

auto now resolves per stream. tracing and console's stdout switch get stdout's answer; miette and console's stderr switch get stderr's.
New test puts stdout on a pty and stderr on a pipe. I confirmed it catches the bug by reintroducing let stderr_enabled = stdout_enabled; — that test fails, the other 8 still pass.

case	stdout	stderr
both piped, default	plain	plain
stdout tty / stderr piped	color	plain
stdout tty / stderr piped, `--color never`	plain	plain
stdout tty / stderr piped, `--color always`	color	color
stdout tty / stderr piped, `NO_COLOR=1`	plain	plain

One case I could not resolve as cleanly, so flagging it explicitly. The owo-colors wrapper is used from both println! and eprintln! (about 390 and 125 sites), and a Painted value cannot tell which macro will consume it. Rather than pick a stream and be wrong for the other, it styles only when both streams accept escapes. That never leaks into a redirected stream, but it costs colour in one case: openshell forward list 2>/dev/null from a terminal prints an uncoloured table even though stdout is still a tty. --color always overrides it.

I went with the conservative rule because leaking escapes into a captured stream is the silent failure this PR exists to fix, whereas losing colour is visible and recoverable. If you would rather keep stdout styled there, the alternative is threading the destination stream through the call sites — happy to do it, but it is a much larger diff than this one.

mrunalp · 2026-08-31T20:52:18Z

/ok to test 8188704

github-actions · 2026-08-31T20:52:41Z

Label test:e2e applied for 8188704. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

Review feedback on NVIDIA#3026. Resolving one answer from stdout and handing it to every library meant a redirected stream inherited the other stream's terminal check. Running `openshell ... 2> build.log` from a terminal wrote escapes into the log, because console's stderr switch and miette's handler were both given stdout's answer. That is worse than the behavior before this branch, where both libraries did their own per-stream detection. Resolve `auto` separately for stdout and stderr and hand each library the answer for the stream it writes to: tracing and console's stdout switch get stdout, miette and console's stderr switch get stderr. The owo-colors wrapper is the exception, since its call sites are split across println! and eprintln! and a Painted value cannot tell which macro will consume it; it styles only when both streams accept escapes, erring toward plain text rather than risking a redirected stream. Existing tests could not catch this: Command::output gives both streams pipes, so a per-stream decision and a single stdout-derived one look identical. Add a test that puts stdout on a pty and stderr on a pipe, which fails when stderr is handed stdout's answer. Replace CLICOLOR_FORCE with FORCE_COLOR. The clicolors spec does not say how to treat `0`, and implementations that special-case it disagree with force-color.org, which keys on presence and non-emptiness only. Using FORCE_COLOR gives it the same rule as NO_COLOR: set and non-empty means yes, whatever the value. Nothing depended on CLICOLOR_FORCE, which was introduced earlier on this branch and never released. Carry the whole style in an owo_colors::Style rather than dispatching a local enum through a six-arm match, and merge styles when chaining so `x.green().bold()` emits one `\x1b[32;1m...\x1b[0m` instead of nesting two wrappers. No call site styles already-styled text, so merging is safe; the emitted bytes are shorter and there is a single reset. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp · 2026-08-31T22:40:37Z

/ok to test 28917ac

Review feedback on NVIDIA#3026. Resolving one answer from stdout and handing it to every library meant a redirected stream inherited the other stream's terminal check. Running `openshell ... 2> build.log` from a terminal wrote escapes into the log, because console's stderr switch and miette's handler were both given stdout's answer. That is worse than the behavior before this branch, where both libraries did their own per-stream detection. Resolve `auto` separately for stdout and stderr and hand each library the answer for the stream it writes to: tracing and console's stdout switch get stdout, miette and console's stderr switch get stderr. The owo-colors wrapper is the exception, since its call sites are split across println! and eprintln! and a Painted value cannot tell which macro will consume it; it styles only when both streams accept escapes, erring toward plain text rather than risking a redirected stream. Existing tests could not catch this: Command::output gives both streams pipes, so a per-stream decision and a single stdout-derived one look identical. Add a test that puts stdout on a pty and stderr on a pipe, which fails when stderr is handed stdout's answer. Replace CLICOLOR_FORCE with FORCE_COLOR. The clicolors spec does not say how to treat `0`, and implementations that special-case it disagree with force-color.org, which keys on presence and non-emptiness only. Using FORCE_COLOR gives it the same rule as NO_COLOR: set and non-empty means yes, whatever the value. Nothing depended on CLICOLOR_FORCE, which was introduced earlier on this branch and never released. Carry the whole style in an owo_colors::Style rather than dispatching a local enum through a six-arm match, and merge styles when chaining so `x.green().bold()` emits one `\x1b[32;1m...\x1b[0m` instead of nesting two wrappers. No call site styles already-styled text, so merging is safe; the emitted bytes are shorter and there is a single reset. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp · 2026-09-01T15:01:30Z

/ok to test 1b456ac

The CLI colorized output unconditionally. owo-colors is built without its `supports-colors` feature, so `.green()` and friends emitted escape sequences regardless of destination, and nothing in the CLI read NO_COLOR. Piping any command through grep or awk matched against bytes the caller could not see; `forward list` was the case that surfaced it, where an escape sits immediately before the STATUS word and defeats a pattern anchored on whitespace. Add a `color` module holding a process-wide switch resolved once in run_async, before any output. Command modules import its `Colorize` trait in place of `OwoColorize`; the method names match, so the ~450 call sites are unchanged, but each consults the switch when it renders and delegates to owo-colors so the escape bytes stay identical. The two traits collide by design: importing both in one module is an ambiguity error, which keeps unconditional coloring from returning. owo-colors is not the only styled path, and the rest each carry their own default, so the switch governs them too: - tracing_subscriber formats with ANSI on, does no terminal detection, and writes to stdout, so `openshell -v ... | ...` leaked escapes the same way the tables did. It now takes the setting via with_ansi. - indicatif and dialoguer both style through console, which has its own detection but cannot learn about --color. Overriding console's global switch covers every progress bar and prompt rather than the specific ones constructed today. Both the stdout and stderr switches are set, since prompts and progress bars draw to stderr. - miette renders errors through its own handler, likewise unaware of --color, so init installs one built from the setting. Resolution order: `--color always|never`, then NO_COLOR, then CLICOLOR_FORCE, then whether stdout is a terminal. The decision is made against stdout even for stderr text, since stdout is what gets parsed; `--color always` restores styling when redirecting. Padding is unaffected — the format spec is forwarded to the inner Display, so widths measure text rather than text plus escapes. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

Review feedback on NVIDIA#3026. Resolving one answer from stdout and handing it to every library meant a redirected stream inherited the other stream's terminal check. Running `openshell ... 2> build.log` from a terminal wrote escapes into the log, because console's stderr switch and miette's handler were both given stdout's answer. That is worse than the behavior before this branch, where both libraries did their own per-stream detection. Resolve `auto` separately for stdout and stderr and hand each library the answer for the stream it writes to: tracing and console's stdout switch get stdout, miette and console's stderr switch get stderr. The owo-colors wrapper is the exception, since its call sites are split across println! and eprintln! and a Painted value cannot tell which macro will consume it; it styles only when both streams accept escapes, erring toward plain text rather than risking a redirected stream. Existing tests could not catch this: Command::output gives both streams pipes, so a per-stream decision and a single stdout-derived one look identical. Add a test that puts stdout on a pty and stderr on a pipe, which fails when stderr is handed stdout's answer. Replace CLICOLOR_FORCE with FORCE_COLOR. The clicolors spec does not say how to treat `0`, and implementations that special-case it disagree with force-color.org, which keys on presence and non-emptiness only. Using FORCE_COLOR gives it the same rule as NO_COLOR: set and non-empty means yes, whatever the value. Nothing depended on CLICOLOR_FORCE, which was introduced earlier on this branch and never released. Carry the whole style in an owo_colors::Style rather than dispatching a local enum through a six-arm match, and merge styles when chaining so `x.green().bold()` emits one `\x1b[32;1m...\x1b[0m` instead of nesting two wrappers. No call site styles already-styled text, so merging is safe; the emitted bytes are shorter and there is a single reset. Signed-off-by: Mrunal Patel <mrunalp@gmail.com>

mrunalp · 2026-09-01T19:13:52Z

/ok to test d0a8437

elezar

Approved. Non-blocking follow-up: --color=auto currently treats a TTY as ANSI-capable, so TERM=dumb openshell … can still emit escape sequences. Please consider terminal-capability detection (with --color=always and FORCE_COLOR overriding it) and add a TERM=dumb test in a follow-up.

* fix(cli): require ANSI-capable terminal before colorizing Follow-up to NVIDIA#3026, raised in review. `auto` treated any terminal as styleable, so `TERM=dumb openshell ...` still emitted escapes into a terminal that renders them literally. An unset TERM had the same problem. This is partly a regression that NVIDIA#3026 introduced. `console`, which drives indicatif and dialoguer, already refused to colorize when TERM is `dumb` or unset, and miette applies the same check through supports-color. NVIDIA#3026 overrides both with its own switch, so it replaced two working checks rather than only failing to add one. tracing and the owo-colors wrapper never had detection, so those two are a gap rather than a regression. Add the capability check to the `auto` branch only, matching console's unix rule: `dumb` is not capable, and an unset TERM is not capable because nothing identifies a capable terminal. Empty is treated as unset, which diverges from console — it reads `TERM=""` as capable since the value is not `dumb` — because an empty value names no terminal type and every other variable here already treats empty as unset. Because the check sits after the explicit branches, `--color always` and FORCE_COLOR still force styling on a dumb terminal, and `--color never` and NO_COLOR still suppress it on a capable one. TERM is a unix signal; Windows consoles enable virtual terminal processing and do not set it, so the check does not apply there. The existing pty test now pins TERM. It previously inherited the ambient value, which would make its outcome depend on the environment now that capability is consulted — CI runners frequently leave TERM unset. Signed-off-by: Mrunal Patel <mrunalp@gmail.com> * refactor(cli): combine stream and terminal capability checks Signed-off-by: Evan Lezar <elezar@nvidia.com> * docs(cli): clarify table color behavior Signed-off-by: Evan Lezar <elezar@nvidia.com> * test(cli): cover redirected status table colors Signed-off-by: Evan Lezar <elezar@nvidia.com> --------- Signed-off-by: Mrunal Patel <mrunalp@gmail.com> Signed-off-by: Evan Lezar <elezar@nvidia.com> Co-authored-by: Evan Lezar <elezar@nvidia.com>

mrunalp requested review from a team, derekwaynecarr and sjenning as code owners August 31, 2026 04:37

elezar reviewed Aug 31, 2026

View reviewed changes

Comment thread .agents/skills/openshell-cli/cli-reference.md Outdated

elezar reviewed Aug 31, 2026

View reviewed changes

Comment thread crates/openshell-cli/src/color.rs Outdated

elezar reviewed Aug 31, 2026

View reviewed changes

Comment thread crates/openshell-cli/src/color.rs Outdated

mrunalp added the test:e2e Requires end-to-end coverage label Aug 31, 2026

mrunalp force-pushed the cli-honor-no-color/mrunalp branch from 8188704 to 28917ac Compare August 31, 2026 21:58

mrunalp force-pushed the cli-honor-no-color/mrunalp branch from 28917ac to 1b456ac Compare September 1, 2026 15:00

mrunalp added 2 commits September 1, 2026 10:25

mrunalp force-pushed the cli-honor-no-color/mrunalp branch from 1b456ac to d0a8437 Compare September 1, 2026 18:01

elezar approved these changes Sep 1, 2026

View reviewed changes

mrunalp enabled auto-merge September 1, 2026 20:57

mrunalp added this pull request to the merge queue Sep 1, 2026

Merged via the queue into NVIDIA:main with commit b4afcd8 Sep 1, 2026
76 checks passed

mrunalp deleted the cli-honor-no-color/mrunalp branch September 1, 2026 21:24

mrunalp mentioned this pull request Sep 2, 2026

fix(cli): --color auto styles terminals that do not render ANSI (TERM=dumb) #3120

Closed

8 tasks

mrunalp mentioned this pull request Sep 2, 2026

fix(cli): require ANSI-capable terminal before colorizing #3121

Merged

6 tasks

Navigation Menu

Uh oh!

fix(cli): suppress ANSI color when stdout is not a terminal - #3026

fix(cli): suppress ANSI color when stdout is not a terminal#3026
mrunalp merged 2 commits into
NVIDIA:mainfrom
mrunalp:cli-honor-no-color/mrunalp

mrunalp commented Aug 31, 2026

Uh oh!

copy-pr-bot Bot commented Aug 31, 2026

Uh oh!

natedemoss commented Aug 31, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrunalp commented Aug 31, 2026

Uh oh!

mrunalp commented Aug 31, 2026

Uh oh!

github-actions Bot commented Aug 31, 2026

Uh oh!

mrunalp commented Aug 31, 2026

Uh oh!

mrunalp commented Sep 1, 2026

Uh oh!

mrunalp commented Sep 1, 2026

Uh oh!

elezar left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mrunalp commented Aug 31, 2026

Summary

Related Issue

Changes

Testing

Checklist

Follow-ups (not in this PR)

Uh oh!

copy-pr-bot Bot commented Aug 31, 2026

Uh oh!

natedemoss commented Aug 31, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mrunalp commented Aug 31, 2026

Uh oh!

mrunalp commented Aug 31, 2026

Uh oh!

github-actions Bot commented Aug 31, 2026

Uh oh!

mrunalp commented Aug 31, 2026

Uh oh!

mrunalp commented Sep 1, 2026

Uh oh!

mrunalp commented Sep 1, 2026

Uh oh!

elezar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants