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

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

Follow-up to #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 #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. #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>

Author: mrunalp

.agents/skills/openshell-cli/cli-reference.md

@@ -12,7 +12,7 @@ Quick-reference for the `openshell` command-line interface. For workflow guidanc
 | `-g`, `--gateway <NAME>` | Gateway to operate on. Also settable via `OPENSHELL_GATEWAY` env var. Falls back to active gateway in `~/.config/openshell/active_gateway`. |
 | `--gateway-endpoint <URL>` | Connect directly to a gateway endpoint without looking up stored metadata. Also settable via `OPENSHELL_GATEWAY_ENDPOINT`. |
 | `--gateway-insecure` | Skip TLS certificate verification. Also settable via `OPENSHELL_GATEWAY_INSECURE`; use only for trusted development endpoints. |
-| `--color <WHEN>` | `auto` (default), `always`, or `never`. `auto` decides per stream, so a redirected stream is plain text while a stream still on the terminal stays styled. Covers tables, `-v` log lines, progress spinners, prompts, and error messages. Also settable via `OPENSHELL_COLOR`. |
+| `--color <WHEN>` | `auto` (default), `always`, or `never`. `auto` decides per stream, so a redirected stream is plain text while a stream still on the terminal stays styled, and it skips terminals that do not render ANSI (`TERM=dumb` or unset). Covers tables, `-v` log lines, progress spinners, prompts, and error messages. Also settable via `OPENSHELL_COLOR`. |
 
 ## Environment Variables
 

crates/openshell-cli/src/color.rs

@@ -34,7 +34,15 @@
 //! 1. `--color always|never` on the command line.
 //! 2. `NO_COLOR`, set and non-empty, disables color (<https://no-color.org>).
 //! 3. `FORCE_COLOR`, set and non-empty, forces color on (<https://force-color.org>).
-//! 4. Otherwise the stream is styled only when that stream is a terminal.
+//! 4. Otherwise the stream is styled only when that stream is a terminal *and*
+//!    that terminal renders ANSI.
+//!
+//! Attachment and capability are separate questions. `TERM=dumb` is a terminal
+//! that does not interpret escapes, so `auto` must not style it — and neither
+//! `console` nor `miette` can apply their own `TERM` checks any more, because
+//! [`init`] overrides both. Capability is consulted only under `auto`, so
+//! `--color always` and `FORCE_COLOR` still force styling on a `dumb` terminal
+//! for anyone who wants it.
 //!
 //! Step 4 is resolved per stream. Redirecting one must not decide for the other:
 //! `openshell ... 2> build.log` from a terminal should keep a styled stdout and
@@ -87,6 +95,7 @@ pub enum ColorChoice {
 pub fn init(choice: ColorChoice) {
     let no_color = std::env::var_os("NO_COLOR");
     let force_color = std::env::var_os("FORCE_COLOR");
+    let term = std::env::var_os("TERM");
 
     // Under `auto` each stream answers for itself. Redirecting one must not
     // decide for the other: `openshell ... 2> build.log` from a terminal has a
@@ -95,13 +104,13 @@ pub fn init(choice: ColorChoice) {
         choice,
         no_color.as_deref(),
         force_color.as_deref(),
-        std::io::stdout().is_terminal(),
+        terminal_supports_ansi(std::io::stdout().is_terminal(), term.as_deref()),
     );
     let stderr_enabled = resolve(
         choice,
         no_color.as_deref(),
         force_color.as_deref(),
-        std::io::stderr().is_terminal(),
+        terminal_supports_ansi(std::io::stderr().is_terminal(), term.as_deref()),
     );
     STDOUT_ENABLED.store(stdout_enabled, Ordering::Relaxed);
     STDERR_ENABLED.store(stderr_enabled, Ordering::Relaxed);
@@ -159,7 +168,7 @@ fn resolve(
     choice: ColorChoice,
     no_color: Option<&OsStr>,
     force_color: Option<&OsStr>,
-    stream_is_terminal: bool,
+    stream_supports_ansi: bool,
 ) -> bool {
     match choice {
         ColorChoice::Always => return true,
@@ -177,7 +186,39 @@ fn resolve(
         return true;
     }
 
+    // Only `auto` consults the terminal. An explicit request above has already
+    // returned, so `--color always` and `FORCE_COLOR` still win on a terminal
+    // that reports no ANSI support.
+    stream_supports_ansi
+}
+
+/// Whether this output stream's terminal renders ANSI escapes.
+///
+/// Being attached to a terminal is not the same as that terminal rendering
+/// ANSI. `TERM` is the unix signal for it; Windows consoles enable virtual
+/// terminal processing instead and do not set `TERM`, so the check does not
+/// apply there.
+fn terminal_supports_ansi(stream_is_terminal: bool, term: Option<&OsStr>) -> bool {
     stream_is_terminal
+        && if cfg!(unix) {
+            term_supports_ansi(term)
+        } else {
+            true
+        }
+}
+
+/// Whether the terminal named by `TERM` renders ANSI escapes on Unix.
+///
+/// Follows the rule `console` applies on unix, which this module overrides:
+/// `dumb` means no, and an unset `TERM` means no because nothing identifies a
+/// capable terminal.
+///
+/// Empty is treated as unset, which is a deliberate divergence: `console` reads
+/// `TERM=""` as `Ok("")`, and since that is not `"dumb"` it counts as capable.
+/// An empty value names no terminal type, and every other variable here already
+/// treats empty as unset, so it is handled the same way.
+fn term_supports_ansi(term: Option<&OsStr>) -> bool {
+    is_set(term) && term != Some(OsStr::new("dumb"))
 }
 
 /// Whether an environment variable counts as set: present and not empty.
@@ -329,7 +370,7 @@ mod tests {
     // and make every `.green()` below ambiguous.
     use super::{
         ColorChoice, Colorize, Ordering, STDERR_ENABLED, STDOUT_ENABLED, Style, painted_enabled,
-        resolve,
+        resolve, term_supports_ansi,
     };
     use std::ffi::OsStr;
 
@@ -500,6 +541,44 @@ mod tests {
         assert!(!resolve(ColorChoice::Auto, None, Some(env("")), false));
     }
 
+    #[test]
+    fn term_capability_follows_the_console_rule() {
+        assert!(term_supports_ansi(Some(OsStr::new("xterm-256color"))));
+        assert!(term_supports_ansi(Some(OsStr::new("screen"))));
+        assert!(!term_supports_ansi(Some(OsStr::new("dumb"))));
+        // Nothing to suggest a capable terminal, so assume none.
+        assert!(!term_supports_ansi(None));
+        // Empty names no terminal type; treated as unset, unlike `console`.
+        assert!(!term_supports_ansi(Some(OsStr::new(""))));
+        // Only an exact match counts; `dumb-something` is a different terminal.
+        assert!(term_supports_ansi(Some(OsStr::new("dumb-but-color"))));
+    }
+
+    #[test]
+    fn auto_does_not_style_an_incapable_terminal() {
+        // A `dumb` terminal is still a terminal, so `is_terminal()` alone would
+        // wrongly enable color.
+        assert!(!resolve(ColorChoice::Auto, None, None, false));
+        assert!(resolve(ColorChoice::Auto, None, None, true));
+    }
+
+    #[test]
+    fn explicit_requests_outrank_terminal_capability() {
+        // `--color always` and FORCE_COLOR are for callers who know better than
+        // the detection, so an incapable terminal must not veto them.
+        assert!(resolve(ColorChoice::Always, None, None, false));
+        assert!(resolve(ColorChoice::Auto, None, Some(env("1")), false));
+        // The negative direction still wins over capability too.
+        assert!(!resolve(ColorChoice::Never, None, None, true));
+        assert!(!resolve(ColorChoice::Auto, Some(env("1")), None, true));
+    }
+
+    #[test]
+    fn capability_does_not_rescue_a_redirected_stream() {
+        // Capability is an additional requirement, not an alternative one.
+        assert!(!resolve(ColorChoice::Auto, None, None, false));
+    }
+
     #[test]
     fn auto_resolves_each_stream_independently() {
         // `openshell ... 2> build.log` from a terminal: stdout is styled, the

crates/openshell-cli/tests/cli_color_integration.rs

@@ -176,42 +176,25 @@ fn tracing_output_is_free_of_escape_sequences_when_piped() {
     );
 }
 
-/// Run a failing command with stdout attached to a pseudo-terminal and stderr
-/// on a pipe, returning what each stream received.
-///
-/// `Command::output` gives both streams pipes, so it cannot distinguish a
-/// per-stream decision from a single one resolved off stdout. This asymmetric
-/// setup is the only way to catch a stream being handed the other stream's
-/// answer.
+/// Run a command with stdout attached to a pseudo-terminal and stderr on a
+/// pipe, returning what each stream received.
 #[cfg(target_os = "linux")]
-fn split_streams_stdout_tty(args: &[&str]) -> (String, String) {
+fn run_with_stdout_tty(mut command: Command) -> (String, String) {
     use std::io::Read;
     use std::os::fd::{AsRawFd, OwnedFd};
 
     let pty = nix::pty::openpty(None, None).expect("openpty");
     let controller: OwnedFd = pty.master;
     let follower: OwnedFd = pty.slave;
 
-    let tmpdir = tempfile::tempdir().expect("create tmpdir");
-    let mut child = Command::new(env!("CARGO_BIN_EXE_openshell"))
-        .args([
-            "sandbox",
-            "list",
-            "--gateway",
-            "test-gateway",
-            "--gateway-endpoint",
-            "http://127.0.0.1:1",
-        ])
-        .args(args)
-        .env("XDG_CONFIG_HOME", tmpdir.path())
-        .env("RUST_LOG", "debug")
-        .env_remove("NO_COLOR")
-        .env_remove("FORCE_COLOR")
-        .env_remove("OPENSHELL_COLOR")
+    let mut child = command
         .stdout(follower.try_clone().expect("dup pty follower"))
         .stderr(std::process::Stdio::piped())
         .spawn()
         .expect("spawn openshell");
+    // `Command` retains its configured stdio handles after spawning. Drop it so
+    // the controller sees EIO once the child exits.
+    drop(command);
 
     // Drop every follower handle in this process, or reading the controller
     // blocks forever instead of returning EIO once the child exits.
@@ -242,6 +225,135 @@ fn split_streams_stdout_tty(args: &[&str]) -> (String, String) {
     )
 }
 
+/// Run a failing command with stdout attached to a pseudo-terminal and stderr
+/// on a pipe, returning what each stream received.
+///
+/// `Command::output` gives both streams pipes, so it cannot distinguish a
+/// per-stream decision from a single one resolved off stdout. This asymmetric
+/// setup is the only way to catch a stream being handed the other stream's
+/// answer.
+#[cfg(target_os = "linux")]
+fn split_streams_stdout_tty(args: &[&str]) -> (String, String) {
+    let tmpdir = tempfile::tempdir().expect("create tmpdir");
+    let mut command = Command::new(env!("CARGO_BIN_EXE_openshell"));
+    command
+        .args([
+            "sandbox",
+            "list",
+            "--gateway",
+            "test-gateway",
+            "--gateway-endpoint",
+            "http://127.0.0.1:1",
+        ])
+        .args(args)
+        .env("XDG_CONFIG_HOME", tmpdir.path())
+        .env("RUST_LOG", "debug")
+        // Pin TERM: `auto` now requires a capable terminal, and CI runners
+        // often leave TERM unset, which would make this test's outcome depend
+        // on the ambient environment.
+        .env("TERM", "xterm-256color")
+        .env_remove("NO_COLOR")
+        .env_remove("FORCE_COLOR")
+        .env_remove("OPENSHELL_COLOR");
+
+    run_with_stdout_tty(command)
+}
+
+/// Run `forward list` with stdout on a pseudo-terminal, under the given `TERM`,
+/// and return everything stdout received.
+///
+/// When `stderr_on_tty` is true, both streams share the terminal so the
+/// `owo-colors` table is styled too. Otherwise, stderr is redirected to
+/// `/dev/null`, which verifies the conservative table behavior.
+#[cfg(target_os = "linux")]
+fn forward_list_on_pty(term: &str, args: &[&str], stderr_on_tty: bool) -> String {
+    use std::os::fd::{AsRawFd, OwnedFd};
+
+    let pty = nix::pty::openpty(None, None).expect("openpty");
+    let controller: OwnedFd = pty.master;
+    let follower: OwnedFd = pty.slave;
+
+    let tmpdir = tempfile::tempdir().expect("create tmpdir");
+    config_dir_with_forward(tmpdir.path());
+
+    let mut command = Command::new(env!("CARGO_BIN_EXE_openshell"));
+    command
+        .args(["forward", "list"])
+        .args(args)
+        .env("XDG_CONFIG_HOME", tmpdir.path())
+        .env("TERM", term)
+        .env_remove("NO_COLOR")
+        .env_remove("FORCE_COLOR")
+        .env_remove("OPENSHELL_COLOR")
+        .stdout(follower.try_clone().expect("dup pty follower"));
+    if stderr_on_tty {
+        command.stderr(follower.try_clone().expect("dup pty follower"));
+    } else {
+        command.stderr(std::process::Stdio::null());
+    }
+    let mut child = command.spawn().expect("spawn openshell");
+    // `Command` retains its configured stdio handles after spawning. Drop it so
+    // the controller sees EIO once the child exits.
+    drop(command);
+
+    // Drop every follower handle here, or the controller read never sees EIO.
+    drop(follower);
+
+    let mut buf = Vec::new();
+    let mut chunk = [0u8; 4096];
+    loop {
+        match nix::unistd::read(controller.as_raw_fd(), &mut chunk) {
+            Ok(0) | Err(_) => break,
+            Ok(n) => buf.extend_from_slice(&chunk[..n]),
+        }
+    }
+    child.wait().expect("wait for openshell");
+
+    let out = String::from_utf8_lossy(&buf).into_owned();
+    assert!(
+        out.contains(SANDBOX),
+        "expected the seeded forward in the table, got: {out:?}"
+    );
+    out
+}
+
+/// A terminal that does not render ANSI must not be styled under `auto`.
+///
+/// `TERM=dumb` is still a terminal, so an `is_terminal()` check alone reports it
+/// as styleable. `console` and `miette` apply their own `TERM` checks, but the
+/// color switch overrides both, so the check has to live here.
+#[cfg(target_os = "linux")]
+#[test]
+fn dumb_terminal_is_not_styled_under_auto() {
+    let dumb = forward_list_on_pty("dumb", &[], true);
+    // Positive control: the same session on a capable terminal is styled, so a
+    // plain result below means capability was consulted, not that the pty setup
+    // silently produced nothing.
+    let capable = forward_list_on_pty("xterm-256color", &[], true);
+
+    assert!(
+        capable.contains(ESC),
+        "expected styling on a capable terminal; got: {capable:?}"
+    );
+    assert!(
+        !dumb.contains(ESC),
+        "TERM=dumb must not be styled, got: {dumb:?}"
+    );
+}
+
+/// An explicit request outranks the capability check, for callers who know
+/// their terminal better than `TERM` does.
+#[cfg(target_os = "linux")]
+#[test]
+fn color_always_overrides_a_dumb_terminal() {
+    let forced = forward_list_on_pty("dumb", &["--color", "always"], true);
+
+    assert!(
+        forced.contains(ESC),
+        "--color always must style even a dumb terminal, got: {forced:?}"
+    );
+}
+
 /// Regression test for a redirected stream inheriting the other stream's
 /// terminal check.
 ///
@@ -264,6 +376,23 @@ fn redirected_stderr_stays_plain_while_stdout_is_a_terminal() {
     );
 }
 
+/// `Painted` cannot identify its destination stream, so table styling is
+/// deliberately disabled when either stream is redirected.
+#[cfg(target_os = "linux")]
+#[test]
+fn status_table_is_plain_when_stderr_is_redirected() {
+    let stdout = forward_list_on_pty("xterm-256color", &[], false);
+
+    assert!(
+        stdout.contains(SANDBOX),
+        "expected the seeded forward in the table, got: {stdout:?}"
+    );
+    assert!(
+        !stdout.contains(ESC),
+        "STATUS table must stay plain when stderr is redirected, got: {stdout:?}"
+    );
+}
+
 #[test]
 fn error_output_follows_the_color_setting() {
     // miette renders errors to stderr through its own handler. It already

docs/sandboxes/manage-sandboxes.mdx

@@ -527,7 +527,7 @@ Structured output includes `sandbox`, `bind_address`, `port`, `pid`, and
 expected OpenShell SSH forward; it does not probe the forwarded socket. When no
 forwards are tracked, structured output returns an empty collection.
 
-The default table colorizes the `STATUS` column, but only when the stream it is written to is a terminal, so piping or redirecting gives plain text. Each stream is decided on its own, so redirecting one leaves the other styled. Prefer `--output json` for automation rather than matching on the table. Set `NO_COLOR` to any non-empty value or pass `--color never` to suppress color, and `--color always` to keep it when piping into a pager. `--color` applies to every `openshell` command and covers all styled output: tables, log lines from `-v`, progress spinners, prompts, and error messages.
+The default table colorizes the `STATUS` column only when both standard output and standard error are capable ANSI terminals; piping or redirecting either stream, or running under `TERM=dumb`, gives a plain-text table. Other styled output—including `-v` log lines, progress spinners, prompts, and error messages—is decided per stream, so redirecting one stream leaves the other styled. Prefer `--output json` for automation rather than matching on the table. Set `NO_COLOR` to any non-empty value or pass `--color never` to suppress ANSI formatting, and `--color always` to force it when piping into a pager. `--color` applies to every `openshell` command.
 
 <Tip>
 You can also forward a port at creation time with `--forward`: