From 1f0beb1c330e900930dea3f6b88ace07724c4941 Mon Sep 17 00:00:00 2001 From: Mrunal Patel Date: Tue, 1 Sep 2026 17:08:46 -0700 Subject: [PATCH 1/4] fix(cli): require ANSI-capable terminal before colorizing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .agents/skills/openshell-cli/cli-reference.md | 2 +- crates/openshell-cli/src/color.rs | 161 ++++++++++++++++-- .../tests/cli_color_integration.rs | 92 ++++++++++ docs/sandboxes/manage-sandboxes.mdx | 2 +- 4 files changed, 240 insertions(+), 17 deletions(-) diff --git a/.agents/skills/openshell-cli/cli-reference.md b/.agents/skills/openshell-cli/cli-reference.md index a3dc91c958..d3a3aaede1 100644 --- a/.agents/skills/openshell-cli/cli-reference.md +++ b/.agents/skills/openshell-cli/cli-reference.md @@ -12,7 +12,7 @@ Quick-reference for the `openshell` command-line interface. For workflow guidanc | `-g`, `--gateway ` | Gateway to operate on. Also settable via `OPENSHELL_GATEWAY` env var. Falls back to active gateway in `~/.config/openshell/active_gateway`. | | `--gateway-endpoint ` | 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 ` | `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 ` | `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 diff --git a/crates/openshell-cli/src/color.rs b/crates/openshell-cli/src/color.rs index e6a185bdad..d82d2ceb8d 100644 --- a/crates/openshell-cli/src/color.rs +++ b/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 (). //! 3. `FORCE_COLOR`, set and non-empty, forces color on (). -//! 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 @@ -88,6 +96,16 @@ pub fn init(choice: ColorChoice) { let no_color = std::env::var_os("NO_COLOR"); let force_color = std::env::var_os("FORCE_COLOR"); + // 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. + let term_capable = if cfg!(unix) { + term_supports_ansi(std::env::var_os("TERM").as_deref()) + } else { + true + }; + // Under `auto` each stream answers for itself. Redirecting one must not // decide for the other: `openshell ... 2> build.log` from a terminal has a // styled stdout and a plain-text stderr, and vice versa for `| grep`. @@ -95,12 +113,14 @@ pub fn init(choice: ColorChoice) { choice, no_color.as_deref(), force_color.as_deref(), + term_capable, std::io::stdout().is_terminal(), ); let stderr_enabled = resolve( choice, no_color.as_deref(), force_color.as_deref(), + term_capable, std::io::stderr().is_terminal(), ); STDOUT_ENABLED.store(stdout_enabled, Ordering::Relaxed); @@ -159,6 +179,7 @@ fn resolve( choice: ColorChoice, no_color: Option<&OsStr>, force_color: Option<&OsStr>, + term_capable: bool, stream_is_terminal: bool, ) -> bool { match choice { @@ -177,7 +198,24 @@ fn resolve( return true; } - stream_is_terminal + // 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_is_terminal && term_capable +} + +/// Whether the terminal named by `TERM` renders ANSI escapes. +/// +/// 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 +367,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; @@ -470,34 +508,127 @@ mod tests { #[test] fn explicit_choice_overrides_environment_and_terminal() { - assert!(resolve(ColorChoice::Always, Some(env("1")), None, false)); - assert!(!resolve(ColorChoice::Never, None, Some(env("1")), true)); + assert!(resolve( + ColorChoice::Always, + Some(env("1")), + None, + true, + false + )); + assert!(!resolve( + ColorChoice::Never, + None, + Some(env("1")), + true, + true + )); } #[test] fn no_color_disables_when_set_and_non_empty() { - assert!(!resolve(ColorChoice::Auto, Some(env("1")), None, true)); + assert!(!resolve( + ColorChoice::Auto, + Some(env("1")), + None, + true, + true + )); // Presence is what counts; the value is not interpreted, so a value that // reads as falsy still disables color. - assert!(!resolve(ColorChoice::Auto, Some(env("0")), None, true)); + assert!(!resolve( + ColorChoice::Auto, + Some(env("0")), + None, + true, + true + )); // NO_COLOR outranks FORCE_COLOR. assert!(!resolve( ColorChoice::Auto, Some(env("1")), Some(env("1")), + true, true )); // An empty value is not "set" for the purposes of the convention. - assert!(resolve(ColorChoice::Auto, Some(env("")), None, true)); + assert!(resolve(ColorChoice::Auto, Some(env("")), None, true, true)); } #[test] fn force_color_enables_when_set_and_non_empty() { - assert!(resolve(ColorChoice::Auto, None, Some(env("1")), false)); + assert!(resolve( + ColorChoice::Auto, + None, + Some(env("1")), + true, + false + )); // Same presence rule as NO_COLOR: `0` is a value, not an opt-out. // keys on presence and non-emptiness only. - assert!(resolve(ColorChoice::Auto, None, Some(env("0")), false)); - assert!(!resolve(ColorChoice::Auto, None, Some(env("")), false)); + assert!(resolve( + ColorChoice::Auto, + None, + Some(env("0")), + true, + false + )); + assert!(!resolve( + ColorChoice::Auto, + None, + Some(env("")), + true, + 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, true)); + assert!(resolve(ColorChoice::Auto, None, None, true, 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, true)); + assert!(resolve( + ColorChoice::Auto, + None, + Some(env("1")), + false, + true + )); + // The negative direction still wins over capability too. + assert!(!resolve(ColorChoice::Never, None, None, true, true)); + assert!(!resolve( + ColorChoice::Auto, + Some(env("1")), + None, + true, + 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, true, false)); } #[test] @@ -505,8 +636,8 @@ mod tests { // `openshell ... 2> build.log` from a terminal: stdout is styled, the // log file is not. Resolving both from stdout's answer would put escapes // in the log. - assert!(resolve(ColorChoice::Auto, None, None, true)); - assert!(!resolve(ColorChoice::Auto, None, None, false)); + assert!(resolve(ColorChoice::Auto, None, None, true, true)); + assert!(!resolve(ColorChoice::Auto, None, None, true, false)); } #[test] @@ -528,7 +659,7 @@ mod tests { #[test] fn auto_follows_the_terminal() { - assert!(resolve(ColorChoice::Auto, None, None, true)); - assert!(!resolve(ColorChoice::Auto, None, None, false)); + assert!(resolve(ColorChoice::Auto, None, None, true, true)); + assert!(!resolve(ColorChoice::Auto, None, None, true, false)); } } diff --git a/crates/openshell-cli/tests/cli_color_integration.rs b/crates/openshell-cli/tests/cli_color_integration.rs index 23109e9774..de396e7c5d 100644 --- a/crates/openshell-cli/tests/cli_color_integration.rs +++ b/crates/openshell-cli/tests/cli_color_integration.rs @@ -205,6 +205,10 @@ fn split_streams_stdout_tty(args: &[&str]) -> (String, String) { .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") @@ -242,6 +246,94 @@ fn split_streams_stdout_tty(args: &[&str]) -> (String, String) { ) } +/// Run `forward list` with *both* streams on one pseudo-terminal, under the +/// given `TERM`, and return everything the terminal received. +/// +/// Both streams share the terminal so the `owo-colors` table is styled too — it +/// requires both streams to accept escapes. This is the shape of a real +/// interactive session, which is the only place terminal capability matters. +#[cfg(target_os = "linux")] +fn forward_list_on_pty(term: &str, args: &[&str]) -> 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 child = Command::new(env!("CARGO_BIN_EXE_openshell")) + .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")) + .stderr(follower.try_clone().expect("dup pty follower")) + .spawn() + .expect("spawn openshell"); + + // 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", &[]); + // 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", &[]); + + 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"]); + + 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. /// diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index d8abacce23..0d76838ef6 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/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, but only when the stream it is written to is a terminal that renders ANSI, so piping, redirecting, or running under `TERM=dumb` 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. You can also forward a port at creation time with `--forward`: From af6eb3c82fec12f24f840ba8537737e4743cd79a Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 2 Sep 2026 13:27:59 +0200 Subject: [PATCH 2/4] refactor(cli): combine stream and terminal capability checks Signed-off-by: Evan Lezar --- crates/openshell-cli/src/color.rs | 132 +++++++++--------------------- 1 file changed, 40 insertions(+), 92 deletions(-) diff --git a/crates/openshell-cli/src/color.rs b/crates/openshell-cli/src/color.rs index d82d2ceb8d..9b78b4ca90 100644 --- a/crates/openshell-cli/src/color.rs +++ b/crates/openshell-cli/src/color.rs @@ -95,16 +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"); - - // 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. - let term_capable = if cfg!(unix) { - term_supports_ansi(std::env::var_os("TERM").as_deref()) - } else { - true - }; + 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 @@ -113,15 +104,13 @@ pub fn init(choice: ColorChoice) { choice, no_color.as_deref(), force_color.as_deref(), - term_capable, - 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(), - term_capable, - 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); @@ -179,8 +168,7 @@ fn resolve( choice: ColorChoice, no_color: Option<&OsStr>, force_color: Option<&OsStr>, - term_capable: bool, - stream_is_terminal: bool, + stream_supports_ansi: bool, ) -> bool { match choice { ColorChoice::Always => return true, @@ -201,10 +189,25 @@ fn resolve( // 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_is_terminal && term_capable + 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. +/// 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 @@ -508,77 +511,34 @@ mod tests { #[test] fn explicit_choice_overrides_environment_and_terminal() { - assert!(resolve( - ColorChoice::Always, - Some(env("1")), - None, - true, - false - )); - assert!(!resolve( - ColorChoice::Never, - None, - Some(env("1")), - true, - true - )); + assert!(resolve(ColorChoice::Always, Some(env("1")), None, false)); + assert!(!resolve(ColorChoice::Never, None, Some(env("1")), true)); } #[test] fn no_color_disables_when_set_and_non_empty() { - assert!(!resolve( - ColorChoice::Auto, - Some(env("1")), - None, - true, - true - )); + assert!(!resolve(ColorChoice::Auto, Some(env("1")), None, true)); // Presence is what counts; the value is not interpreted, so a value that // reads as falsy still disables color. - assert!(!resolve( - ColorChoice::Auto, - Some(env("0")), - None, - true, - true - )); + assert!(!resolve(ColorChoice::Auto, Some(env("0")), None, true)); // NO_COLOR outranks FORCE_COLOR. assert!(!resolve( ColorChoice::Auto, Some(env("1")), Some(env("1")), - true, true )); // An empty value is not "set" for the purposes of the convention. - assert!(resolve(ColorChoice::Auto, Some(env("")), None, true, true)); + assert!(resolve(ColorChoice::Auto, Some(env("")), None, true)); } #[test] fn force_color_enables_when_set_and_non_empty() { - assert!(resolve( - ColorChoice::Auto, - None, - Some(env("1")), - true, - false - )); + assert!(resolve(ColorChoice::Auto, None, Some(env("1")), false)); // Same presence rule as NO_COLOR: `0` is a value, not an opt-out. // keys on presence and non-emptiness only. - assert!(resolve( - ColorChoice::Auto, - None, - Some(env("0")), - true, - false - )); - assert!(!resolve( - ColorChoice::Auto, - None, - Some(env("")), - true, - false - )); + assert!(resolve(ColorChoice::Auto, None, Some(env("0")), false)); + assert!(!resolve(ColorChoice::Auto, None, Some(env("")), false)); } #[test] @@ -598,37 +558,25 @@ mod tests { 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, true)); - assert!(resolve(ColorChoice::Auto, None, None, true, true)); + 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, true)); - assert!(resolve( - ColorChoice::Auto, - None, - Some(env("1")), - false, - true - )); + 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, true)); - assert!(!resolve( - ColorChoice::Auto, - Some(env("1")), - None, - true, - true - )); + 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, true, false)); + assert!(!resolve(ColorChoice::Auto, None, None, false)); } #[test] @@ -636,8 +584,8 @@ mod tests { // `openshell ... 2> build.log` from a terminal: stdout is styled, the // log file is not. Resolving both from stdout's answer would put escapes // in the log. - assert!(resolve(ColorChoice::Auto, None, None, true, true)); - assert!(!resolve(ColorChoice::Auto, None, None, true, false)); + assert!(resolve(ColorChoice::Auto, None, None, true)); + assert!(!resolve(ColorChoice::Auto, None, None, false)); } #[test] @@ -659,7 +607,7 @@ mod tests { #[test] fn auto_follows_the_terminal() { - assert!(resolve(ColorChoice::Auto, None, None, true, true)); - assert!(!resolve(ColorChoice::Auto, None, None, true, false)); + assert!(resolve(ColorChoice::Auto, None, None, true)); + assert!(!resolve(ColorChoice::Auto, None, None, false)); } } From 9122a8df5f6bd99d28b6b52bc5bbafb359a654fd Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 2 Sep 2026 13:28:05 +0200 Subject: [PATCH 3/4] docs(cli): clarify table color behavior Signed-off-by: Evan Lezar --- docs/sandboxes/manage-sandboxes.mdx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index 0d76838ef6..39c4a73958 100644 --- a/docs/sandboxes/manage-sandboxes.mdx +++ b/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 that renders ANSI, so piping, redirecting, or running under `TERM=dumb` 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. You can also forward a port at creation time with `--forward`: From c8a02a603bfa5607be428ea2be5f4d4e46c23f47 Mon Sep 17 00:00:00 2001 From: Evan Lezar Date: Wed, 2 Sep 2026 13:28:09 +0200 Subject: [PATCH 4/4] test(cli): cover redirected status table colors Signed-off-by: Evan Lezar --- .../tests/cli_color_integration.rs | 121 ++++++++++++------ 1 file changed, 79 insertions(+), 42 deletions(-) diff --git a/crates/openshell-cli/tests/cli_color_integration.rs b/crates/openshell-cli/tests/cli_color_integration.rs index de396e7c5d..54f2dc2f0a 100644 --- a/crates/openshell-cli/tests/cli_color_integration.rs +++ b/crates/openshell-cli/tests/cli_color_integration.rs @@ -176,15 +176,10 @@ 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}; @@ -192,30 +187,14 @@ fn split_streams_stdout_tty(args: &[&str]) -> (String, String) { 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") - // 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") + 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. @@ -246,14 +225,48 @@ fn split_streams_stdout_tty(args: &[&str]) -> (String, String) { ) } -/// Run `forward list` with *both* streams on one pseudo-terminal, under the -/// given `TERM`, and return everything the terminal received. +/// 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. /// -/// Both streams share the terminal so the `owo-colors` table is styled too — it -/// requires both streams to accept escapes. This is the shape of a real -/// interactive session, which is the only place terminal capability matters. +/// 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]) -> String { +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"); @@ -263,7 +276,8 @@ fn forward_list_on_pty(term: &str, args: &[&str]) -> String { let tmpdir = tempfile::tempdir().expect("create tmpdir"); config_dir_with_forward(tmpdir.path()); - let mut child = Command::new(env!("CARGO_BIN_EXE_openshell")) + let mut command = Command::new(env!("CARGO_BIN_EXE_openshell")); + command .args(["forward", "list"]) .args(args) .env("XDG_CONFIG_HOME", tmpdir.path()) @@ -271,10 +285,16 @@ fn forward_list_on_pty(term: &str, args: &[&str]) -> String { .env_remove("NO_COLOR") .env_remove("FORCE_COLOR") .env_remove("OPENSHELL_COLOR") - .stdout(follower.try_clone().expect("dup pty follower")) - .stderr(follower.try_clone().expect("dup pty follower")) - .spawn() - .expect("spawn openshell"); + .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); @@ -305,11 +325,11 @@ fn forward_list_on_pty(term: &str, args: &[&str]) -> String { #[cfg(target_os = "linux")] #[test] fn dumb_terminal_is_not_styled_under_auto() { - let dumb = forward_list_on_pty("dumb", &[]); + 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", &[]); + let capable = forward_list_on_pty("xterm-256color", &[], true); assert!( capable.contains(ESC), @@ -326,7 +346,7 @@ fn dumb_terminal_is_not_styled_under_auto() { #[cfg(target_os = "linux")] #[test] fn color_always_overrides_a_dumb_terminal() { - let forced = forward_list_on_pty("dumb", &["--color", "always"]); + let forced = forward_list_on_pty("dumb", &["--color", "always"], true); assert!( forced.contains(ESC), @@ -356,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