diff --git a/src/cli.rs b/src/cli.rs index 4d3c67c079..6acbfcbe5e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -913,6 +913,25 @@ pub(super) fn parse_u64_flag(flag: &str, value: &str) -> std::io::Result { .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 { + 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 { 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", + ] + ); + } } diff --git a/src/cli/pane.rs b/src/cli/pane.rs index e7d6570238..5979fd9964 100644 --- a/src/cli/pane.rs +++ b/src/cli/pane.rs @@ -451,42 +451,55 @@ fn pane_rename(args: &[String]) -> std::io::Result { } fn pane_read(args: &[String]) -> std::io::Result { - let Some(raw_pane_id) = args.first() else { - eprintln!("usage: herdr pane read [--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 { + const USAGE: &str = "usage: herdr pane read [--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 { 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 { @@ -946,28 +964,42 @@ fn pane_run(args: &[String]) -> std::io::Result { } fn pane_wait_output(args: &[String]) -> std::io::Result { - let Some(raw_pane_id) = args.first() else { - eprintln!("usage: herdr pane wait-output (--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 { + const USAGE: &str = "usage: herdr pane wait-output (--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 { } "--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(pane_id) = pane_id else { + return Err(USAGE.into()); + }; let Some(matcher) = matcher else { - eprintln!("missing required --match or --regex"); - return Ok(2); + return Err("missing required --match or --regex".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, - }), - })?) + Ok(PaneWaitForOutputParams { + pane_id, + source, + lines, + r#match: matcher, + timeout_ms, + strip_ansi, + }) } fn pane_report_agent(args: &[String]) -> std::io::Result { @@ -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")); + } } diff --git a/tests/cli/panes.rs b/tests/cli/panes.rs index 32110b365d..fb6cd3dee1 100644 --- a/tests/cli/panes.rs +++ b/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")); +}