fix(cli): accept --flag=value and reordered options in pane read/wait-output (#2183)

* fix(cli): accept --flag=value and reordered options in pane read/wait-output

* test(cli): trim redundant pane parser coverage

---------

Co-authored-by: Can Celik <ogulcancelik@gmail.com>

src/cli.rs

@@ -913,6 +913,25 @@ pub(super) fn parse_u64_flag(flag: &str, value: &str) -> std::io::Result<u64> {
         .map_err(|_| std::io::Error::other(format!("invalid value for {flag}: {value}")))
 }
 
+/// Expand `--flag=value` tokens into separate `--flag` and `value` tokens so
+/// the hand-rolled subcommand parsers accept the same `--flag=value` form the
+/// clap-generated help and completions imply. Only `value_options` are split:
+/// boolean and unknown options keep their attached value so they still reach
+/// the parser's unknown-option branch.
+pub(super) fn expand_equals_args(args: &[String], value_options: &[&str]) -> Vec<String> {
+    let mut expanded = Vec::with_capacity(args.len());
+    for arg in args {
+        match arg.split_once('=') {
+            Some((flag, value)) if value_options.contains(&flag) => {
+                expanded.push(flag.to_string());
+                expanded.push(value.to_string());
+            }
+            _ => expanded.push(arg.clone()),
+        }
+    }
+    expanded
+}
+
 fn parse_session_json_only(args: &[String], usage: &str) -> Result<bool, i32> {
     match args {
         [] => Ok(false),
@@ -1121,4 +1140,29 @@ mod tests {
         );
         assert!(!super::server_not_running::was_reported(&mapped));
     }
+
+    #[test]
+    fn expand_equals_args_splits_value_options_only() {
+        // Known value options split; values may contain `=`. Boolean and
+        // unknown options keep the attached form so parsers still reject them.
+        let args = vec![
+            "--match=a=b".to_string(),
+            "name=value".to_string(),
+            "--raw=value".to_string(),
+            "--bogus=value".to_string(),
+            "--timeout=5000".to_string(),
+        ];
+        assert_eq!(
+            super::expand_equals_args(&args, &["--match", "--timeout"]),
+            vec![
+                "--match",
+                "a=b",
+                "name=value",
+                "--raw=value",
+                "--bogus=value",
+                "--timeout",
+                "5000",
+            ]
+        );
+    }
 }

src/cli/pane.rs

@@ -451,42 +451,55 @@ fn pane_rename(args: &[String]) -> std::io::Result<i32> {
 }
 
 fn pane_read(args: &[String]) -> std::io::Result<i32> {
-    let Some(raw_pane_id) = args.first() else {
-        eprintln!("usage: herdr pane read <pane_id> [--source visible|recent|recent-unwrapped] [--lines N] [--format text|ansi] [--ansi]");
-        return Ok(2);
+    let params = match parse_pane_read_args(args) {
+        Ok(params) => params,
+        Err(message) => {
+            eprintln!("{message}");
+            return Ok(2);
+        }
     };
 
-    let pane_id = super::normalize_pane_id(raw_pane_id);
+    let response = super::send_request(&Request {
+        id: "cli:pane:read".into(),
+        method: Method::PaneRead(params),
+    })?;
+
+    super::print_read_response(&response)
+}
+
+fn parse_pane_read_args(args: &[String]) -> Result<PaneReadParams, String> {
+    const USAGE: &str = "usage: herdr pane read <pane_id> [--source visible|recent|recent-unwrapped|detection] [--lines N] [--format text|ansi] [--ansi] [--raw]";
+
+    let args = super::expand_equals_args(args, &["--source", "--lines", "--format"]);
+    let mut pane_id = None;
     let mut source = ReadSource::Recent;
     let mut lines = None;
     let mut format = ReadFormat::Text;
     let mut strip_ansi = true;
 
-    let mut index = 1;
+    let mut index = 0;
     while index < args.len() {
         match args[index].as_str() {
             "--source" => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for --source");
-                    return Ok(2);
+                    return Err("missing value for --source".into());
                 };
-                source = super::parse_read_source(value)?;
+                source = super::parse_read_source(value).map_err(|err| err.to_string())?;
                 index += 2;
             }
             "--lines" => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for --lines");
-                    return Ok(2);
+                    return Err("missing value for --lines".into());
                 };
-                lines = Some(super::parse_u32_flag("--lines", value)?);
+                lines =
+                    Some(super::parse_u32_flag("--lines", value).map_err(|err| err.to_string())?);
                 index += 2;
             }
             "--format" => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for --format");
-                    return Ok(2);
+                    return Err("missing value for --format".into());
                 };
-                format = super::parse_read_format(value)?;
+                format = super::parse_read_format(value).map_err(|err| err.to_string())?;
                 index += 2;
             }
             "--ansi" => {
@@ -498,26 +511,31 @@ fn pane_read(args: &[String]) -> std::io::Result<i32> {
                 strip_ansi = false;
                 index += 1;
             }
-            other => {
-                eprintln!("unknown option: {other}");
-                return Ok(2);
+            option if option.starts_with('-') => {
+                return Err(format!("unknown option: {option}"));
+            }
+            positional => {
+                if pane_id.is_some() {
+                    return Err(format!("unexpected argument: {positional}"));
+                }
+                pane_id = Some(super::normalize_pane_id(positional));
+                index += 1;
             }
         }
     }
 
-    let response = super::send_request(&Request {
-        id: "cli:pane:read".into(),
-        method: Method::PaneRead(PaneReadParams {
-            pane_id,
-            source,
-            lines,
-            format,
-            strip_ansi,
-            intent: crate::api::schema::ReadIntent::Interactive,
-        }),
-    })?;
+    let Some(pane_id) = pane_id else {
+        return Err(USAGE.into());
+    };
 
-    super::print_read_response(&response)
+    Ok(PaneReadParams {
+        pane_id,
+        source,
+        lines,
+        format,
+        strip_ansi,
+        intent: crate::api::schema::ReadIntent::Interactive,
+    })
 }
 
 fn pane_split(args: &[String]) -> std::io::Result<i32> {
@@ -946,28 +964,42 @@ fn pane_run(args: &[String]) -> std::io::Result<i32> {
 }
 
 fn pane_wait_output(args: &[String]) -> std::io::Result<i32> {
-    let Some(raw_pane_id) = args.first() else {
-        eprintln!("usage: herdr pane wait-output <pane_id> (--match TEXT | --regex PATTERN) [--source visible|recent|recent-unwrapped] [--lines N] [--timeout MS] [--raw]");
-        return Ok(2);
+    let params = match parse_pane_wait_output_args(args) {
+        Ok(params) => params,
+        Err(message) => {
+            eprintln!("{message}");
+            return Ok(2);
+        }
     };
-    let pane_id = super::normalize_pane_id(raw_pane_id);
+
+    super::print_response(&super::send_request(&Request {
+        id: "cli:pane:wait-output".into(),
+        method: Method::PaneWaitForOutput(params),
+    })?)
+}
+
+fn parse_pane_wait_output_args(args: &[String]) -> Result<PaneWaitForOutputParams, String> {
+    const USAGE: &str = "usage: herdr pane wait-output <pane_id> (--match TEXT | --regex PATTERN) [--source visible|recent|recent-unwrapped] [--lines N] [--timeout MS] [--raw]";
+
+    let args = super::expand_equals_args(
+        args,
+        &["--match", "--regex", "--source", "--lines", "--timeout"],
+    );
+    let mut pane_id = None;
     let mut source = ReadSource::Recent;
     let mut lines = None;
     let mut timeout_ms = None;
     let mut strip_ansi = true;
     let mut matcher = None;
-    let mut index = 1;
+    let mut index = 0;
     while index < args.len() {
         match args[index].as_str() {
-            "--match" | "--regex" => {
-                let option = args[index].as_str();
+            option @ ("--match" | "--regex") => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for {option}");
-                    return Ok(2);
+                    return Err(format!("missing value for {option}"));
                 };
                 if matcher.is_some() {
-                    eprintln!("--match and --regex are mutually exclusive");
-                    return Ok(2);
+                    return Err("--match and --regex are mutually exclusive".into());
                 }
                 matcher = Some(if option == "--regex" {
                     OutputMatch::Regex {
@@ -982,53 +1014,57 @@ fn pane_wait_output(args: &[String]) -> std::io::Result<i32> {
             }
             "--source" => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for --source");
-                    return Ok(2);
+                    return Err("missing value for --source".into());
                 };
-                source = super::parse_read_source(value)?;
+                source = super::parse_read_source(value).map_err(|err| err.to_string())?;
                 index += 2;
             }
             "--lines" => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for --lines");
-                    return Ok(2);
+                    return Err("missing value for --lines".into());
                 };
-                lines = Some(super::parse_u32_flag("--lines", value)?);
+                lines =
+                    Some(super::parse_u32_flag("--lines", value).map_err(|err| err.to_string())?);
                 index += 2;
             }
             "--timeout" => {
                 let Some(value) = args.get(index + 1) else {
-                    eprintln!("missing value for --timeout");
-                    return Ok(2);
+                    return Err("missing value for --timeout".into());
                 };
-                timeout_ms = Some(super::parse_u64_flag("--timeout", value)?);
+                timeout_ms =
+                    Some(super::parse_u64_flag("--timeout", value).map_err(|err| err.to_string())?);
                 index += 2;
             }
             "--raw" => {
                 strip_ansi = false;
                 index += 1;
             }
-            other => {
-                eprintln!("unknown option: {other}");
-                return Ok(2);
+            option if option.starts_with('-') => {
+                return Err(format!("unknown option: {option}"));
+            }
+            positional => {
+                if pane_id.is_some() {
+                    return Err(format!("unexpected argument: {positional}"));
+                }
+                pane_id = Some(super::normalize_pane_id(positional));
+                index += 1;
             }
         }
     }
-    let Some(matcher) = matcher else {
-        eprintln!("missing required --match or --regex");
-        return Ok(2);
+    let Some(pane_id) = pane_id else {
+        return Err(USAGE.into());
     };
-    super::print_response(&super::send_request(&Request {
-        id: "cli:pane:wait-output".into(),
-        method: Method::PaneWaitForOutput(PaneWaitForOutputParams {
-            pane_id,
-            source,
-            lines,
-            r#match: matcher,
-            timeout_ms,
-            strip_ansi,
-        }),
-    })?)
+    let Some(matcher) = matcher else {
+        return Err("missing required --match or --regex".into());
+    };
+    Ok(PaneWaitForOutputParams {
+        pane_id,
+        source,
+        lines,
+        r#match: matcher,
+        timeout_ms,
+        strip_ansi,
+    })
 }
 
 fn pane_report_agent(args: &[String]) -> std::io::Result<i32> {
@@ -1799,4 +1835,90 @@ mod tests {
         assert_eq!(params.direction, PaneDirection::Left);
         assert_eq!(params.amount, Some(0.125));
     }
+
+    #[test]
+    fn parse_pane_read_args_defaults_with_bare_pane_id() {
+        let params = parse_pane_read_args(&args(&["issue-1"])).unwrap();
+
+        assert_eq!(params.pane_id, "issue-1");
+        assert_eq!(params.source, ReadSource::Recent);
+        assert_eq!(params.lines, None);
+        assert_eq!(params.format, ReadFormat::Text);
+        assert!(params.strip_ansi);
+    }
+
+    #[test]
+    fn parse_pane_read_args_accepts_space_separated_options() {
+        let params = parse_pane_read_args(&args(&[
+            "issue-1", "--source", "visible", "--lines", "5", "--ansi",
+        ]))
+        .unwrap();
+
+        assert_eq!(params.pane_id, "issue-1");
+        assert_eq!(params.source, ReadSource::Visible);
+        assert_eq!(params.lines, Some(5));
+        assert_eq!(params.format, ReadFormat::Ansi);
+    }
+
+    #[test]
+    fn parse_pane_read_args_accepts_reordered_equals_options() {
+        let params =
+            parse_pane_read_args(&args(&["--source=visible", "--lines=5", "issue-1"])).unwrap();
+
+        assert_eq!(params.pane_id, "issue-1");
+        assert_eq!(params.source, ReadSource::Visible);
+        assert_eq!(params.lines, Some(5));
+    }
+
+    #[test]
+    fn parse_pane_wait_output_args_accepts_space_separated_options() {
+        let params = parse_pane_wait_output_args(&args(&[
+            "issue-1",
+            "--match",
+            "ready",
+            "--timeout",
+            "5000",
+        ]))
+        .unwrap();
+
+        assert_eq!(params.pane_id, "issue-1");
+        assert_eq!(
+            params.r#match,
+            OutputMatch::Substring {
+                value: "ready".into()
+            }
+        );
+        assert_eq!(params.timeout_ms, Some(5000));
+        assert_eq!(params.source, ReadSource::Recent);
+        assert!(params.strip_ansi);
+    }
+
+    #[test]
+    fn parse_pane_wait_output_args_accepts_reordered_equals_options() {
+        let params =
+            parse_pane_wait_output_args(&args(&["--match=a=b", "--timeout=100", "issue-1"]))
+                .unwrap();
+
+        assert_eq!(params.pane_id, "issue-1");
+        assert_eq!(
+            params.r#match,
+            OutputMatch::Substring {
+                value: "a=b".into()
+            }
+        );
+        assert_eq!(params.timeout_ms, Some(100));
+    }
+
+    #[test]
+    fn parse_pane_wait_output_args_requires_matcher() {
+        let err = parse_pane_wait_output_args(&args(&["issue-1"])).unwrap_err();
+        assert!(err.contains("missing required --match or --regex"));
+    }
+
+    #[test]
+    fn parse_pane_wait_output_args_rejects_conflicting_matchers() {
+        let err = parse_pane_wait_output_args(&args(&["issue-1", "--match", "a", "--regex", "b"]))
+            .unwrap_err();
+        assert!(err.contains("mutually exclusive"));
+    }
 }

tests/cli/panes.rs

@@ -566,3 +566,17 @@ fn pane_shell_gets_herdr_socket_and_pane_env() {
 
     cleanup_spawned_herdr(herdr, base);
 }
+
+#[test]
+fn pane_read_rejects_invalid_value_with_usage_error() {
+    // Invalid option values fail as CLI usage errors before any server
+    // contact: exit 2 with the plain parser message, not the old
+    // `Error: Custom { ... }` io::Error wrapper from main.
+    let socket_path = Path::new("/tmp/herdr-cli-invalid-values-no-server.sock");
+
+    let read = run_cli(socket_path, &["pane", "read", "w1:p1", "--source", "bogus"]);
+    assert_eq!(read.status.code(), Some(2));
+    let stderr = String::from_utf8_lossy(&read.stderr);
+    assert!(stderr.contains("invalid read source: bogus"));
+    assert!(!stderr.contains("Error: Custom"));
+}