diff --git a/apps/soundcraft-cli/src/args.rs b/apps/soundcraft-cli/src/args.rs index 40351779..6b583992 100644 --- a/apps/soundcraft-cli/src/args.rs +++ b/apps/soundcraft-cli/src/args.rs @@ -154,7 +154,8 @@ pub(crate) struct RunArgs { pub(crate) struct ScriptArgs { /// The script to run pub(crate) file: PathBuf, - /// Report a failing line and go on with the next + /// Continue after command errors and finish requested output, then exit nonzero if any command failed. + /// Malformed JSON stops immediately #[arg(long)] pub(crate) keep_going: bool, #[command(flatten)] diff --git a/apps/soundcraft-cli/src/error.rs b/apps/soundcraft-cli/src/error.rs index a048694c..ebf5af70 100644 --- a/apps/soundcraft-cli/src/error.rs +++ b/apps/soundcraft-cli/src/error.rs @@ -20,6 +20,8 @@ pub(crate) enum CliError { ScriptJson { path: PathBuf, line: usize, source: serde_json::Error }, #[error("{}:{line}: {id}: {source}", path.display())] ScriptLine { path: PathBuf, line: usize, id: String, source: EngineError }, + #[error("script finished with command errors")] + ScriptFailed, #[error("{}: {source}", path.display())] File { path: PathBuf, source: std::io::Error }, #[error("{}: {source}", path.display())] diff --git a/apps/soundcraft-cli/src/session.rs b/apps/soundcraft-cli/src/session.rs index 20e17b1a..3f09cf64 100644 --- a/apps/soundcraft-cli/src/session.rs +++ b/apps/soundcraft-cli/src/session.rs @@ -75,6 +75,7 @@ pub(crate) fn script(args: &ScriptArgs, out: &mut dyn Write, err: &mut dyn Write let text = std::fs::read_to_string(path).map_err(|source| CliError::File { path: path.clone(), source })?; let mut e = load_engine(&args.engine, err)?; let keep = args.keep_going; + let mut failed = false; for (n, line) in text.lines().enumerate() { let line = line.trim(); if line.is_empty() || line.starts_with('#') { @@ -90,10 +91,12 @@ pub(crate) fn script(args: &ScriptArgs, out: &mut dyn Write, err: &mut dyn Write return Err(failure); } outln!(err, "{failure}"); + failed = true; } } } - finish(&mut e, &args.engine, out) + finish(&mut e, &args.engine, out)?; + if failed { Err(CliError::ScriptFailed) } else { Ok(()) } } #[cfg(test)] diff --git a/apps/soundcraft-cli/tests/it/behaviour.rs b/apps/soundcraft-cli/tests/it/behaviour.rs index ce0076d1..69bdffa9 100644 --- a/apps/soundcraft-cli/tests/it/behaviour.rs +++ b/apps/soundcraft-cli/tests/it/behaviour.rs @@ -238,13 +238,13 @@ fn script_line_errors_stop_unless_keep_going() { let dir = sandbox("script-errors"); std::fs::write(dir.join("bad.txt"), "nosuch.cmd {}\ntrack.new {}\n").unwrap(); std::fs::write(dir.join("badjson.txt"), "track.new {x\n").unwrap(); - // A stopping line error carries the `error:` prefix; a --keep-going one is a bare line. + // Kept-going line errors stay bare; a final error summary supplies the failing status. let stop = invoke(&dir, &["script", "bad.txt"]); assert_eq!((stop.code, stop.stdout.as_str()), (1, "")); assert_eq!(stop.stderr, "error: bad.txt:1: nosuch.cmd: unknown command `nosuch.cmd`\n"); let keep = invoke(&dir, &["script", "bad.txt", "--keep-going"]); - assert_eq!((keep.code, keep.stdout.as_str()), (0, "track.new: {\"tracks\":[1]}\n")); - assert_eq!(keep.stderr, "bad.txt:1: nosuch.cmd: unknown command `nosuch.cmd`\n"); + assert_eq!((keep.code, keep.stdout.as_str()), (1, "track.new: {\"tracks\":[1]}\n")); + assert_eq!(keep.stderr, "bad.txt:1: nosuch.cmd: unknown command `nosuch.cmd`\nerror: script finished with command errors\n"); assert_fails(&invoke(&dir, &["script", "badjson.txt"]), "badjson.txt:1: bad JSON: key must be a string at line 1 column 2"); } @@ -515,10 +515,9 @@ fn mcp_roots_with_connect_are_a_usage_error() { ); } -/// A name that isn't valid Unicode works wherever the tool itself reads or writes the file -/// (`convert`); a session or bounce the engine opens or writes by text name is refused (exit 1), -/// naming the file. Run in-process: the names need no shell. -#[cfg(unix)] +/// Native file paths round-trip on filesystems that accept non-Unicode names. macOS rejects +/// these names with EILSEQ before the tool can write them. Run in-process: no shell is needed. +#[cfg(all(unix, not(target_os = "macos")))] #[test] fn non_unicode_file_names() { use std::ffi::OsString; @@ -543,8 +542,26 @@ fn non_unicode_file_names() { std::fs::rename(dir.join("s.scraft"), latin1("scraft")).unwrap(); let (code, err) = run(vec!["info".into(), latin1("scraft").into()]); assert_eq!(code, 0, "{err}"); - let (code, err) = run(vec!["run".into(), "--demo".into(), "--bounce".into(), latin1("wav").into()]); - assert_eq!(code, 1); - assert!(err.contains("caf\u{FFFD}.") && err.contains("not valid Unicode"), "{err}"); + let _ = std::fs::remove_dir_all(&dir); +} + +/// Engine output paths enter JSON, so invalid Unicode must be refused even on a filesystem +/// that cannot create such a file (including macOS). +#[cfg(unix)] +#[test] +fn non_unicode_session_outputs_are_rejected() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let dir = sandbox("nonunicode-outputs"); + for (flag, name) in [("--bounce", b"caf\xe9.wav".as_slice()), ("--save", b"caf\xe9.scraft".as_slice())] { + let path = dir.join(OsString::from_vec(name.to_vec())); + let (mut out, mut err) = (Vec::new(), Vec::new()); + let code = soundcraft_cli::run(vec!["run".into(), "--demo".into(), flag.into(), path.into_os_string()], &mut out, &mut err); + let err = String::from_utf8_lossy(&err); + assert_eq!(code, 1, "{err}"); + assert!(err.contains("caf\u{FFFD}.") && err.contains("not valid Unicode"), "{err}"); + assert!(out.is_empty()); + } let _ = std::fs::remove_dir_all(&dir); } diff --git a/apps/soundcraft-cli/tests/it/main.rs b/apps/soundcraft-cli/tests/it/main.rs index f1c44788..a5a22774 100644 --- a/apps/soundcraft-cli/tests/it/main.rs +++ b/apps/soundcraft-cli/tests/it/main.rs @@ -9,3 +9,4 @@ mod closed_stdout; mod man_and_completions; #[allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] mod run; +mod script_status; diff --git a/apps/soundcraft-cli/tests/it/script_status.rs b/apps/soundcraft-cli/tests/it/script_status.rs new file mode 100644 index 00000000..4321b8b6 --- /dev/null +++ b/apps/soundcraft-cli/tests/it/script_status.rs @@ -0,0 +1,90 @@ +use std::path::{Path, PathBuf}; +use std::process::{Command, Output}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::{SystemTime, UNIX_EPOCH}; + +struct Scratch(PathBuf); + +impl Scratch { + fn new() -> Self { + static NEXT: AtomicU64 = AtomicU64::new(0); + let sequence = NEXT.fetch_add(1, Ordering::Relaxed); + let stamp = SystemTime::now().duration_since(UNIX_EPOCH).unwrap().as_nanos(); + let path = std::env::temp_dir().join(format!("soundcraft-script-status-{}-{stamp}-{sequence}", std::process::id())); + std::fs::create_dir(&path).unwrap(); + Self(path) + } +} + +impl Drop for Scratch { + fn drop(&mut self) { + let _ = std::fs::remove_dir_all(&self.0); + } +} + +fn invoke(dir: &Path, args: &[&str]) -> Output { + Command::new(env!("CARGO_BIN_EXE_soundcraft-cli")).current_dir(dir).env("SOUNDCRAFT_NO_PREFS", "1").args(args).output().unwrap() +} + +#[test] +fn keep_going_runs_later_commands_and_finishes_but_reports_failure() { + let dir = Scratch::new(); + std::fs::write( + dir.0.join("audit.txt"), + "audit.deliberately_invalid {}\ntrack.new {\"name\":\"After failure\",\"count\":1,\"format\":\"stereo\"}\n", + ) + .unwrap(); + let stop = invoke(&dir.0, &["script", "audit.txt", "--save", "stopped.scraft"]); + assert_eq!(stop.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&stop.stderr).contains("audit.deliberately_invalid")); + assert!(!String::from_utf8_lossy(&stop.stdout).contains("track.new:")); + assert!(!dir.0.join("stopped.scraft").exists()); + + let keep = invoke( + &dir.0, + &["script", "audit.txt", "--keep-going", "--bounce", "after.wav", "--start", "0", "--end", "0.01", "--save", "after.scraft", "--inspect"], + ); + assert_eq!(keep.status.code(), Some(1), "{}", String::from_utf8_lossy(&keep.stderr)); + assert!(String::from_utf8_lossy(&keep.stderr).contains("audit.deliberately_invalid")); + let stdout = String::from_utf8_lossy(&keep.stdout); + assert!(stdout.contains("track.new:") && stdout.contains("After failure"), "{stdout}"); + assert!(dir.0.join("after.wav").is_file()); + let info = invoke(&dir.0, &["info", "after.scraft"]); + assert!(info.status.success(), "{}", String::from_utf8_lossy(&info.stderr)); + assert!(String::from_utf8_lossy(&info.stdout).contains("After failure")); +} + +#[test] +fn successful_script_and_parsing_or_finish_failures_keep_their_status() { + let dir = Scratch::new(); + std::fs::write(dir.0.join("good.txt"), "track.new {\"name\":\"Good\",\"count\":1,\"format\":\"stereo\"}\n").unwrap(); + let success = invoke(&dir.0, &["script", "good.txt", "--keep-going"]); + assert!(success.status.success(), "{}", String::from_utf8_lossy(&success.stderr)); + + std::fs::write(dir.0.join("bad.txt"), "track.new {bad\ntrack.new {\"name\":\"Too late\"}\n").unwrap(); + let malformed = invoke(&dir.0, &["script", "bad.txt", "--keep-going", "--save", "malformed.scraft"]); + assert_eq!(malformed.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&malformed.stderr).contains("bad JSON")); + assert!(!String::from_utf8_lossy(&malformed.stdout).contains("track.new:")); + assert!(!dir.0.join("malformed.scraft").exists()); + + std::fs::write(dir.0.join("parent"), "not a directory").unwrap(); + for (option, path) in [("--save", "parent/out.scraft"), ("--bounce", "parent/out.wav")] { + let fail = invoke(&dir.0, &["script", "good.txt", "--keep-going", option, path]); + assert_eq!(fail.status.code(), Some(1), "{option}: {}", String::from_utf8_lossy(&fail.stderr)); + assert!(String::from_utf8_lossy(&fail.stderr).contains("parent")); + } + + std::fs::write(dir.0.join("engine.txt"), "session.save {}\ntrack.new {\"name\":\"After engine failure\",\"count\":1}\n").unwrap(); + let fail = invoke(&dir.0, &["script", "engine.txt", "--keep-going", "--save", "engine.scraft"]); + assert_eq!(fail.status.code(), Some(1)); + assert!(String::from_utf8_lossy(&fail.stderr).contains("session.save")); + assert!(String::from_utf8_lossy(&fail.stdout).contains("track.new:")); + assert!(dir.0.join("engine.scraft").is_file()); + + let both_fail = invoke(&dir.0, &["script", "engine.txt", "--keep-going", "--save", "parent/out.scraft"]); + assert_eq!(both_fail.status.code(), Some(1)); + let errors = String::from_utf8_lossy(&both_fail.stderr); + assert!(errors.contains("session.save") && errors.contains("parent"), "{errors}"); + assert!(!errors.contains("script finished with command errors"), "{errors}"); +}