diff --git a/apps/gridcraft-cli/src/check.rs b/apps/gridcraft-cli/src/check.rs --- a/apps/gridcraft-cli/src/check.rs +++ b/apps/gridcraft-cli/src/check.rs @@ -191,7 +191,7 @@ impl Diffs { /// every checked formula matches and every round trip is clean; 1 otherwise; 2 on a usage error. pub fn main(args: &[String]) -> ExitCode { let usage = |e: String| { - eprintln!("gridcraft-cli: {e}\n{USAGE}"); + errln!("gridcraft-cli: {e}\n{USAGE}"); ExitCode::from(2) }; let a = match Args::parse(args, &["tolerance", "max-report"], &["json"]) { diff --git a/apps/gridcraft-cli/src/main.rs b/apps/gridcraft-cli/src/main.rs --- a/apps/gridcraft-cli/src/main.rs +++ b/apps/gridcraft-cli/src/main.rs @@ -17,18 +17,32 @@ #![forbid(unsafe_code)] use std::process::ExitCode; +use std::sync::atomic::{AtomicBool, Ordering}; -/// `println!` that ends the program quietly when stdout is closed (`gridcraft-cli commands | -/// head`) instead of panicking on a broken pipe. +static STDOUT_CLOSED: AtomicBool = AtomicBool::new(false); + +/// `println!` that suppresses further output when a reader closes stdout, while allowing work to finish. macro_rules! outln { ($($arg:tt)*) => {{ - use std::io::Write as _; - if let Err(e) = writeln!(std::io::stdout(), $($arg)*) { + if !$crate::STDOUT_CLOSED.load(std::sync::atomic::Ordering::Relaxed) + && let Err(e) = $crate::write_stdout(format_args!("{}\n", format_args!($($arg)*))) + { $crate::stdout_failed(e); } }}; } +macro_rules! errln { + ($($arg:tt)*) => {{ + use std::io::Write as _; + if let Err(e) = writeln!(std::io::stderr().lock(), $($arg)*) + && e.kind() != std::io::ErrorKind::BrokenPipe + { + std::process::exit(1); + } + }}; +} + mod check; use gridcraft_engine::Session; @@ -45,20 +59,38 @@ const USAGE: &str = "usage: [--out FILE] [--print RANGE] [--quiet] a script line is id={json} or {\"command\": \"id\", \"params\": {…}}; blank lines and lines starting with # or // are skipped + closing stdout does not cancel work; cancel the process to stop it gridcraft-cli commands [--json] [--search X] gridcraft-cli functions [--json] [--search X] [--category C] gridcraft-cli mcp [--connect PORT] [--in FILE | --sample NAME] gridcraft-cli send [json] gridcraft-cli version"; -/// stdout went away. A reader that stopped early (a closed pipe) ends the program quietly; any -/// other write error is reported. -fn stdout_failed(e: std::io::Error) -> ! { +fn write_stdout(args: std::fmt::Arguments<'_>) -> std::io::Result<()> { + use std::io::Write as _; + #[cfg(unix)] + { + // Stdout suppresses EBADF, so write through a duplicate to report invalid destinations. + use std::os::fd::AsFd as _; + let stdout = std::io::stdout(); + std::fs::File::from(stdout.as_fd().try_clone_to_owned()?).write_all(args.to_string().as_bytes()) + } + #[cfg(not(unix))] + { + let stdout = std::io::stdout(); + let mut out = stdout.lock(); + out.write_fmt(args).and_then(|()| out.flush()) + } +} + +/// Stop writing after a reader closes the pipe; report any other write error. +fn stdout_failed(e: std::io::Error) { if e.kind() == std::io::ErrorKind::BrokenPipe { - std::process::exit(0); + STDOUT_CLOSED.store(true, Ordering::Relaxed); + } else { + errln!("gridcraft-cli: can't write to stdout: {e}"); + std::process::exit(1); } - eprintln!("gridcraft-cli: can't write to stdout: {e}"); - std::process::exit(1); } fn main() -> ExitCode { @@ -89,7 +121,7 @@ fn main() -> ExitCode { match r { Ok(()) => ExitCode::SUCCESS, Err(e) => { - eprintln!("gridcraft-cli: {e}"); + errln!("gridcraft-cli: {e}"); ExitCode::FAILURE } } @@ -159,7 +191,7 @@ fn exec(s: &mut Session, id: &str, params: Value) -> Result { && let Some(warnings) = value.get("warnings").and_then(Value::as_array) { for warning in warnings.iter().filter_map(Value::as_str) { - eprintln!("Warning: {warning}"); + errln!("Warning: {warning}"); } } r @@ -292,13 +324,13 @@ fn convert(args: &[String]) -> Result<(), String> { activate_sheet(&mut s, a.opt("sheet"))?; if pdf { let result = exec(&mut s, "file.exportPdf", json!({"path": out}))?; - eprintln!("wrote {out} ({} bytes, {} pages)", result["bytes"], result["pages"]); + errln!("wrote {out} ({} bytes, {} pages)", result["bytes"], result["pages"]); return Ok(()); } let d = s.doc().map_err(|e| e.to_string())?; let bytes = gridcraft_engine::io::save_file_bytes(&d.wb, out).map_err(|e| e.to_string())?; gridcraft_engine::io::write_file(out, &bytes).map_err(|e| e.to_string())?; - eprintln!("wrote {out} ({} bytes)", bytes.len()); + errln!("wrote {out} ({} bytes)", bytes.len()); Ok(()) } @@ -451,7 +483,7 @@ fn run(args: &[String]) -> Result<(), String> { } if let Some(out) = a.opt("out") { let r = exec(&mut s, "file.save", json!({"path": out}))?; - eprintln!("wrote {out} ({} bytes)", r["bytes"]); + errln!("wrote {out} ({} bytes)", r["bytes"]); } for (name, range) in &a.opts { if name == "print" { @@ -491,7 +523,7 @@ fn commands(args: &[String]) -> Result<(), String> { outln!("{: Result<(), String> { let desc = f["description"].as_str().unwrap_or(""); outln!(" {cat}: {desc}"); } - eprintln!("{} functions", list.len()); + errln!("{} functions", list.len()); Ok(()) } @@ -549,7 +581,7 @@ fn mcp(args: &[String]) -> Result<(), String> { Box::new(Headless::with_session(session)) } }; - eprintln!("gridcraft-cli: MCP server on stdio ({})", backend.describe()); + errln!("gridcraft-cli: MCP server on stdio ({})", backend.describe()); let stdin = std::io::stdin(); let stdout = std::io::stdout(); Server::new(backend).serve(stdin.lock(), stdout.lock()).map_err(|e| e.to_string()) diff --git a/apps/gridcraft-cli/tests/it/cli.rs b/apps/gridcraft-cli/tests/it/cli.rs --- a/apps/gridcraft-cli/tests/it/cli.rs +++ b/apps/gridcraft-cli/tests/it/cli.rs @@ -22,6 +22,105 @@ fn tmpdir(name: &str) -> PathBuf { d } +#[cfg(unix)] +fn run_with_closed_stdout(args: &[&str]) -> Output { + // Hold the shell until the read end is closed, so the first result always sees EPIPE. + let mut child = Command::new("sh") + .args(["-c", "IFS= read -r ready; exec \"$@\"", "sh", env!("CARGO_BIN_EXE_gridcraft-cli")]) + .args(args) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .unwrap(); + drop(child.stdout.take()); + writeln!(child.stdin.take().unwrap(), "go").unwrap(); + child.wait_with_output().unwrap() +} + +#[cfg(unix)] +#[test] +fn broken_stdout_still_runs_later_commands_and_saves_workbook() { + let dir = tmpdir("broken-stdout-save"); + let book = dir.join("out.xlsx"); + let out = run_with_closed_stdout(&[ + "run", + "--cmd", + r#"cell.set={"cell":"A1","input":"first"}"#, + "--cmd", + r#"cell.set={"cell":"A2","input":"saved"}"#, + "--out", + book.to_str().unwrap(), + ]); + assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr)); + assert!(book.exists(), "workbook was not saved"); + assert_eq!(ok(&["cat", book.to_str().unwrap(), "--range", "A1:A2"]).split_whitespace().collect::>(), ["first", "saved"]); + let _ = std::fs::remove_dir_all(dir); +} + +#[cfg(unix)] +#[test] +fn broken_stdout_does_not_hide_later_work_or_save_errors() { + let dir = tmpdir("broken-stdout-errors"); + let book = dir.join("missing").join("out.xlsx"); + let fail_save = run_with_closed_stdout(&["run", "--cmd", r#"cell.set={"cell":"A1","input":"first"}"#, "--out", book.to_str().unwrap()]); + assert_eq!(fail_save.status.code(), Some(1), "save failure must return 1 without panicking"); + assert!(!fail_save.stderr.is_empty(), "save failure must include a diagnostic"); + let fail_command = run_with_closed_stdout(&["run", "--cmd", r#"cell.set={"cell":"A1","input":"first"}"#, "--cmd", "no.such.command"]); + assert_eq!(fail_command.status.code(), Some(1), "later command failure must return 1 without panicking"); + assert!(String::from_utf8_lossy(&fail_command.stderr).contains("no.such.command")); + let _ = std::fs::remove_dir_all(dir); +} + +#[cfg(unix)] +#[test] +fn closed_stdout_and_stderr_do_not_hide_successful_save() { + use std::os::fd::OwnedFd; + use std::os::unix::net::UnixStream; + + let dir = tmpdir("closed-both-save"); + let book = dir.join("out.xlsx"); + let (writer, reader) = UnixStream::pair().unwrap(); + let error_writer = writer.try_clone().unwrap(); + drop(reader); + let status = bin() + .args(["run", "--cmd", r#"cell.set={"cell":"A1","input":"saved"}"#, "--out", book.to_str().unwrap()]) + .stdout(Stdio::from(OwnedFd::from(writer))) + .stderr(Stdio::from(OwnedFd::from(error_writer))) + .status() + .unwrap(); + assert!(status.success(), "successful save must not fail after the reader closes: {status}"); + assert_eq!(ok(&["cat", book.to_str().unwrap(), "--range", "A1"]).trim(), "saved"); + let _ = std::fs::remove_dir_all(dir); +} + +#[cfg(unix)] +#[test] +fn closed_stdout_and_stderr_still_report_work_failure() { + use std::os::fd::OwnedFd; + use std::os::unix::net::UnixStream; + + let (writer, reader) = UnixStream::pair().unwrap(); + let error_writer = writer.try_clone().unwrap(); + drop(reader); + let status = bin() + .args(["run", "--cmd", r#"cell.set={"cell":"A1","input":"first"}"#, "--cmd", "no.such.command"]) + .stdout(Stdio::from(OwnedFd::from(writer))) + .stderr(Stdio::from(OwnedFd::from(error_writer))) + .status() + .unwrap(); + assert_eq!(status.code(), Some(1), "failed work should exit 1 without panicking"); +} + +#[cfg(unix)] +#[test] +fn stdout_errors_other_than_broken_pipe_fail() { + let read_only = std::fs::File::open(env!("CARGO_BIN_EXE_gridcraft-cli")).unwrap(); + let out = bin().arg("version").stdout(Stdio::from(read_only)).output().unwrap(); + assert!(!out.status.success()); + assert!(String::from_utf8_lossy(&out.stderr).contains("can't write to stdout")); +} + #[test] fn version_and_usage() { assert_eq!(ok(&["--version"]).trim(), format!("gridcraft-cli {}", env!("CARGO_PKG_VERSION"))); diff --git a/docs/cli.md b/docs/cli.md --- a/docs/cli.md +++ b/docs/cli.md @@ -45,6 +45,9 @@ gridcraft-cli run [--in FILE | --sample budget|sales|grades] \ Opens a file (or a sample, or a blank workbook), runs `--cmd` and `--script` steps in the order given, then saves `--out` and prints each `--print` range. Each non-null command result is printed as a JSON line unless `--quiet`. The first failing step stops the run with a non-zero exit. +If the reader of a stdout pipe closes early, printed output stops but the run continues, including later steps and a +requested `--out` save. The exit status is 0 if the work succeeds, or non-zero if it fails. Other stdout write errors +stop execution with a non-zero exit. Stop the process to cancel the run. A `--cmd` is `id`, `id={json}` or `id {json}`. A script has one command per line in the same forms, or as JSON objects `{"command": "cell.set", "params": {...}}`; blank lines and `#` comments are skipped.