Merge pull request #1145 from DAnaeto/fix/cli-keep-going-exit-status

Fix exit status after kept-going script errors

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)]

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())]

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)]

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);
 }

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;

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}");
+}