Merge pull request #1005 from Nael-Nathanael/fix/desktop-args-997

# Conflicts:
#	contributors/people.toml

apps/effectcraft/src/main.rs

@@ -1,6 +1,8 @@
 //! EffectCraft desktop app.
 //!
-//! Usage: `effectcraft [--control <port>] [--demo|--empty|--home] [project.ecproj | media files…]`
+//! Usage: `effectcraft [--control <port>] [--demo|--empty|--home] [--] [project.ecproj | media files…]`
+//!
+//! An unknown option or a bad port prints an error and exits with status 1 before anything starts.
 //!
 //! Without a project the app starts with an empty Untitled Project, as After Effects does;
 //! `--demo` opens the demo project instead (also Help ▸ Open Demo Project).
@@ -70,22 +72,51 @@ struct Args {
     home: Option<bool>,
 }
 
-/// Parse the arguments after the program name; `None` for `--version`. `env_port` is
-/// `EFFECTCRAFT_CONTROL_PORT`.
-fn parse_args(args: impl IntoIterator<Item = String>, env_port: Option<String>) -> Option<Args> {
-    let mut out = Args { control_port: env_port.and_then(|p| p.parse().ok()), ..Default::default() };
+/// Parse the arguments after the program name; `Ok(None)` for `--version`. `env_port` is
+/// `EFFECTCRAFT_CONTROL_PORT`, used when `--control` isn't given. An unknown option or a bad port
+/// is an error, so it is neither imported as a file nor dropped; after `--` every argument is a file.
+fn parse_args(args: impl IntoIterator<Item = String>, env_port: Result<String, std::env::VarError>) -> Result<Option<Args>, String> {
+    let mut out = Args::default();
     let mut args = args.into_iter();
+    let mut options = true;
     while let Some(a) = args.next() {
+        if !options {
+            out.files.push(a);
+            continue;
+        }
         match a.as_str() {
-            "--control" => out.control_port = args.next().and_then(|p| p.parse().ok()),
+            "--" => options = false,
+            "--control" => out.control_port = Some(control_port("--control", args.next().as_deref())?),
             "--demo" => out.demo = true,
             "--empty" => out.demo = false,
             "--home" => out.home = Some(true),
-            "--version" => return None,
-            _ => out.files.push(a),
+            "--version" => return Ok(None),
+            _ => match a.strip_prefix("--control=") {
+                Some(v) => out.control_port = Some(control_port("--control", Some(v))?),
+                None if a.starts_with('-') && a != "-" => return Err(format!("unknown option `{a}`")),
+                None => out.files.push(a),
+            },
         }
     }
-    Some(out)
+    if out.control_port.is_none() {
+        out.control_port = match env_port {
+            Ok(v) if v.is_empty() => None,
+            Ok(v) => Some(control_port("EFFECTCRAFT_CONTROL_PORT", Some(&v))?),
+            Err(std::env::VarError::NotPresent) => None,
+            Err(std::env::VarError::NotUnicode(v)) => Some(control_port("EFFECTCRAFT_CONTROL_PORT", Some(&v.to_string_lossy()))?),
+        };
+    }
+    Ok(Some(out))
+}
+
+/// A control-channel port from 1 to 65535 (0 would listen on a port nobody is told); `name` is
+/// the flag or variable it came from.
+fn control_port(name: &str, value: Option<&str>) -> Result<u16, String> {
+    let need = format!("{name} needs a port number from 1 to 65535");
+    match value {
+        None => Err(need),
+        Some(v) => v.parse().ok().filter(|&p| p != 0).ok_or(format!("{need}, not `{v}`")),
+    }
 }
 
 fn main() -> eframe::Result {
@@ -94,9 +125,17 @@ fn main() -> eframe::Result {
     // feed the compatibility report.
     effectcraft_engine::logging::install();
     effectcraft_engine::logging::install_panic_hook();
-    let Some(Args { control_port, files, demo, home }) = parse_args(std::env::args().skip(1), std::env::var("EFFECTCRAFT_CONTROL_PORT").ok()) else {
-        println!("effectcraft {}", env!("CARGO_PKG_VERSION"));
-        return Ok(());
+    let Args { control_port, files, demo, home } = match parse_args(std::env::args().skip(1), std::env::var("EFFECTCRAFT_CONTROL_PORT")) {
+        Ok(Some(args)) => args,
+        Ok(None) => {
+            println!("effectcraft {}", env!("CARGO_PKG_VERSION"));
+            return Ok(());
+        }
+        // Before the session log, preferences and the window: nothing has started yet.
+        Err(e) => {
+            eprintln!("effectcraft: {e}");
+            std::process::exit(1);
+        }
     };
     // The session log lives in the settings folder (`Logs/effectcraft.log`, the previous two runs
     // kept): a launch from the Dock or a menu has no terminal. Opened after the arguments, so
@@ -425,9 +464,18 @@ fn disable_app_nap() {
 #[cfg(test)]
 mod tests {
     use super::{Args, parse_args};
+    use std::env::VarError;
+
+    fn parse_env(args: &[&str], env_port: Result<String, VarError>) -> Result<Option<Args>, String> {
+        parse_args(args.iter().map(|a| a.to_string()), env_port)
+    }
 
     fn parse(args: &[&str]) -> Option<Args> {
-        parse_args(args.iter().map(|a| a.to_string()), None)
+        parse_env(args, Err(VarError::NotPresent)).unwrap()
+    }
+
+    fn fails(args: &[&str]) -> String {
+        parse_env(args, Err(VarError::NotPresent)).unwrap_err()
     }
 
     #[cfg(target_os = "linux")]
@@ -477,6 +525,53 @@ mod tests {
         let a = parse(&["--control", "9877", "--home", "a.ecproj"]).unwrap_or_default();
         assert_eq!((a.control_port, a.home, a.files), (Some(9877), Some(true), vec!["a.ecproj".to_string()]));
         assert_eq!(parse(&["--version"]), None);
-        assert_eq!(parse_args(Vec::new(), Some("9000".into())).map(|a| a.control_port), Some(Some(9000)));
+        assert_eq!(parse_env(&[], Ok("9000".into())).unwrap().map(|a| a.control_port), Some(Some(9000)));
+    }
+
+    /// A missing, non-numeric or zero `--control` value stops the launch instead of starting
+    /// without a control server, and the argument after `--control` isn't reinterpreted (#997).
+    #[test]
+    fn control_needs_a_port_from_1_to_65535() {
+        let need = "--control needs a port number from 1 to 65535";
+        assert_eq!(fails(&["--control"]), need);
+        assert_eq!(fails(&["--control", "--demo"]), format!("{need}, not `--demo`"));
+        assert_eq!(fails(&["--control", "--version"]), format!("{need}, not `--version`"));
+        for bad in ["abc", "0", "70000", "-1", ""] {
+            assert_eq!(fails(&["--control", bad]), format!("{need}, not `{bad}`"));
+            assert_eq!(fails(&[&format!("--control={bad}")]), format!("{need}, not `{bad}`"));
+        }
+        assert_eq!(parse(&["--control=9877"]).map(|a| a.control_port), Some(Some(9877)));
+        assert_eq!(parse(&["--control", "65535"]).map(|a| a.control_port), Some(Some(65535)));
+    }
+
+    /// An unknown option is an error, not a file to import (#997); `--` ends the options.
+    #[test]
+    fn unknown_options_are_errors_not_files() {
+        for bad in ["--automation-read-root", "--automation-write-root=/tmp", "-h", "--Demo", "--demo=yes"] {
+            assert_eq!(fails(&[bad, "/tmp/folder"]), format!("unknown option `{bad}`"));
+        }
+        // Parsed left to right: an error before `--version` wins.
+        assert_eq!(fails(&["--nope", "--version"]), "unknown option `--nope`");
+        let a = parse(&["clip.mov", "-", "--", "--demo", "-name.png"]).unwrap_or_default();
+        assert_eq!(a.files, ["clip.mov", "-", "--demo", "-name.png"]);
+        assert!(!a.demo);
+    }
+
+    /// `--control` wins over `EFFECTCRAFT_CONTROL_PORT`; the variable is checked only when it is
+    /// used, so a bad value doesn't stop `--control` or `--version`.
+    #[test]
+    fn control_port_variable_is_checked_when_used() {
+        let need = "EFFECTCRAFT_CONTROL_PORT needs a port number from 1 to 65535";
+        assert_eq!(parse_env(&[], Ok("abc".into())).unwrap_err(), format!("{need}, not `abc`"));
+        assert_eq!(parse_env(&[], Ok("0".into())).unwrap_err(), format!("{need}, not `0`"));
+        assert_eq!(parse_env(&[], Ok(String::new())), Ok(Some(Args::default())));
+        assert_eq!(parse_env(&["--control", "9877"], Ok("abc".into())).unwrap().map(|a| a.control_port), Some(Some(9877)));
+        assert_eq!(parse_env(&["--version"], Ok("abc".into())), Ok(None));
+        #[cfg(unix)]
+        {
+            use std::os::unix::ffi::OsStringExt;
+            let not_utf8 = VarError::NotUnicode(std::ffi::OsString::from_vec(vec![0x39, 0xff]));
+            assert_eq!(parse_env(&[], Err(not_utf8)).unwrap_err(), format!("{need}, not `9\u{fffd}`"));
+        }
     }
 }

apps/effectcraft/tests/args.rs

@@ -0,0 +1,73 @@
+//! The desktop binary rejects a bad command line before it starts anything (#997).
+// Integration tests: unwrapping and panicking on failure is fine here.
+#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)]
+
+use std::path::PathBuf;
+use std::process::{Command, Stdio};
+use std::time::{Duration, Instant};
+
+/// A settings folder of its own, so a launch that got past the arguments would show up as
+/// preferences (Recent Footage) written there instead of in the user's settings.
+fn config_dir(name: &str) -> PathBuf {
+    let d = std::env::temp_dir().join(format!("ec-args-test-{}-{name}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&d);
+    std::fs::create_dir_all(&d).unwrap();
+    d
+}
+
+/// Runs the app, which must exit at once with `effectcraft: <message>` and a failure status. A
+/// regression fails instead of leaving a window open: the process is killed after 5 s.
+fn rejects(name: &str, args: &[&str], env: &[(&str, &str)], message: &str) {
+    let config = config_dir(name);
+    let mut cmd = Command::new(env!("CARGO_BIN_EXE_effectcraft"));
+    cmd.args(args).env_remove("EFFECTCRAFT_CONTROL_PORT").env_remove("RUST_LOG").env("EFFECTCRAFT_CONFIG_DIR", &config);
+    for (k, v) in env {
+        cmd.env(k, v);
+    }
+    let mut child = cmd.stdin(Stdio::null()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap();
+    let deadline = Instant::now() + Duration::from_secs(5);
+    let timed_out = loop {
+        if child.try_wait().unwrap().is_some() {
+            break false;
+        }
+        if Instant::now() >= deadline {
+            child.kill().unwrap();
+            break true;
+        }
+        std::thread::sleep(Duration::from_millis(10));
+    };
+    let out = child.wait_with_output().unwrap();
+    assert!(!timed_out, "{args:?}: the app started or did not exit promptly");
+    assert_eq!(out.status.code(), Some(1), "{args:?}");
+    assert!(out.stdout.is_empty(), "{args:?}: {:?}", String::from_utf8_lossy(&out.stdout));
+    assert_eq!(String::from_utf8(out.stderr).unwrap(), format!("effectcraft: {message}\n"), "{args:?}");
+    let written: Vec<_> = std::fs::read_dir(&config).unwrap().collect();
+    assert!(written.is_empty(), "{args:?}: wrote {written:?}");
+    let _ = std::fs::remove_dir_all(&config);
+}
+
+#[test]
+fn unknown_options_stop_the_launch_before_anything_is_imported() {
+    let folder = config_dir("folder");
+    rejects("read-root", &["--control", "47613", "--automation-read-root", folder.to_str().unwrap()], &[], "unknown option `--automation-read-root`");
+    rejects("write-root", &["--automation-write-root=/tmp"], &[], "unknown option `--automation-write-root=/tmp`");
+    rejects("short", &["-h"], &[], "unknown option `-h`");
+    let _ = std::fs::remove_dir_all(&folder);
+}
+
+#[test]
+fn a_bad_control_port_stops_the_launch() {
+    let need = "--control needs a port number from 1 to 65535";
+    rejects("missing", &["--control"], &[], need);
+    rejects("flag", &["--control", "--demo"], &[], &format!("{need}, not `--demo`"));
+    rejects("text", &["--control", "abc"], &[], &format!("{need}, not `abc`"));
+    rejects("zero", &["--control=0"], &[], &format!("{need}, not `0`"));
+    rejects("env", &[], &[("EFFECTCRAFT_CONTROL_PORT", "abc")], "EFFECTCRAFT_CONTROL_PORT needs a port number from 1 to 65535, not `abc`");
+}
+
+#[test]
+fn version_still_prints_with_a_bad_port_variable() {
+    let out = Command::new(env!("CARGO_BIN_EXE_effectcraft")).arg("--version").env("EFFECTCRAFT_CONTROL_PORT", "abc").output().unwrap();
+    assert!(out.status.success(), "{}", String::from_utf8_lossy(&out.stderr));
+    assert_eq!(String::from_utf8(out.stdout).unwrap(), format!("effectcraft {}\n", env!("CARGO_PKG_VERSION")));
+}

contributors/people.toml

@@ -67,3 +67,6 @@ display_name = "Jason Brashear"
 
 [people.nanox333]
 display_name = "nanox333"
+
+[people.Nael-Nathanael]
+sync_github_name = true

docs/control-protocol.md

@@ -6,6 +6,9 @@ The desktop app exposes a control channel for agents, tests and the MCP bridge:
 effectcraft --control 9877        # or EFFECTCRAFT_CONTROL_PORT=9877
 ```
 
+The port must be a number from 1 to 65535, and `--control` overrides the variable. A bad port or an
+unknown option prints an error and the app exits with status 1 before it opens a window.
+
 - **Transport:** TCP on `127.0.0.1` only, one JSON object per line in each direction. Connections are
   persistent; requests on one connection are answered in order.
 - **Request:** `{"id": 1, "method": "engine.execute", "params": {...}}` (`id` is echoed back, `params`