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

.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

@@ -456,7 +456,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`: