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..9b78b4ca90 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 @@ -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 diff --git a/crates/openshell-cli/tests/cli_color_integration.rs b/crates/openshell-cli/tests/cli_color_integration.rs index 23109e9774..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,26 +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") - .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 diff --git a/docs/sandboxes/manage-sandboxes.mdx b/docs/sandboxes/manage-sandboxes.mdx index d8abacce23..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, 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. You can also forward a port at creation time with `--forward`: