diff --git a/src-tauri/src/cli/agent_review_handlers.rs b/src-tauri/src/cli/agent_review_handlers.rs index 1c8cf5bfa..49d29f80f 100644 --- a/src-tauri/src/cli/agent_review_handlers.rs +++ b/src-tauri/src/cli/agent_review_handlers.rs @@ -31,7 +31,9 @@ fn parse_line_number(value: &str, name: &str) -> Result { value .trim() .parse::() - .map_err(|_| format!("--{name} must be a positive line number, got '{value}'")) + .ok() + .filter(|line| *line > 0) + .ok_or_else(|| format!("--{name} must be a positive line number, got '{value}'")) } fn parse_side(value: Option<&str>) -> Result, String> { @@ -238,6 +240,15 @@ mod tests { assert_eq!(strip_suggestion_fence("```suggestion\n```"), ""); } + #[test] + fn line_numbers_must_be_positive() { + assert_eq!(parse_line_number("7", "start-line"), Ok(7)); + for bad in ["0", "-1"] { + let error = parse_line_number(bad, "start-line").unwrap_err(); + assert!(error.contains("must be a positive line number"), "{error}"); + } + } + #[test] fn rejects_an_unknown_side() { assert!(parse_side(Some("both")).is_err()); diff --git a/src-tauri/src/cli/args.rs b/src-tauri/src/cli/args.rs index 92c4b09cd..46e1a7222 100644 --- a/src-tauri/src/cli/args.rs +++ b/src-tauri/src/cli/args.rs @@ -61,7 +61,10 @@ fn build_command(name: &str, config: &CommandConfig) -> Command { command = command.after_help(after_help.clone()); } for arg in &config.args { - let mut clap_arg = ClapArg::new(arg.name.clone()).required(arg.required); + // Numeric values like `-1` parse as values (then fail validation), not flags. + let mut clap_arg = ClapArg::new(arg.name.clone()) + .required(arg.required) + .allow_negative_numbers(arg.takes_value || arg.multiple); match arg.index { Some(index) => clap_arg = clap_arg.index(index), None => { diff --git a/src-tauri/src/cli/mod.rs b/src-tauri/src/cli/mod.rs index 3cd61e314..9208b1d1d 100644 --- a/src-tauri/src/cli/mod.rs +++ b/src-tauri/src/cli/mod.rs @@ -252,11 +252,26 @@ fn optional_usize(matches: &Matches, name: &str) -> Result, String .map(|value| { value .parse() - .map_err(|_| format!("--{name} must be a positive integer")) + .ok() + .filter(|n| *n > 0) + .ok_or_else(|| format!("invalid_arguments: --{name} must be a positive integer")) }) .transpose() } +fn line_range(matches: &Matches) -> Result<(Option, Option), String> { + let start = optional_usize(matches, "start-line")?; + let end = optional_usize(matches, "end-line")?; + if let (Some(start), Some(end)) = (start, end) { + if start > end { + return Err(format!( + "invalid_arguments: invalid line range {start}..{end}: expected 1 <= start-line <= end-line" + )); + } + } + Ok((start, end)) +} + fn split_csv(value: Option) -> Vec { value .map(|value| { @@ -360,6 +375,7 @@ pub(crate) fn parse_remote_command_request( .clone() .ok_or_else(|| format!("--target ({what}) is required")) }; + let (start_line, end_line) = line_range(matches)?; match (command, action.as_str()) { ("repo", "status") => Ok(TreqCommandRequest::RepositoryStatus { repo }), ("repo", "branches") => Ok(TreqCommandRequest::ListBranches { repo }), @@ -460,8 +476,8 @@ pub(crate) fn parse_remote_command_request( "parent" => FileRevision::Parent, _ => return Err("--revision must be working-copy or parent".to_string()), }, - start_line: optional_usize(matches, "start-line")?, - end_line: optional_usize(matches, "end-line")?, + start_line, + end_line, }), ("file", "restore") => Ok(TreqCommandRequest::RestoreFile { repo, diff --git a/src-tauri/src/cli/tests.rs b/src-tauri/src/cli/tests.rs index 6117fec71..1ca65f487 100644 --- a/src-tauri/src/cli/tests.rs +++ b/src-tauri/src/cli/tests.rs @@ -1434,3 +1434,64 @@ mod help_text { assert!(subcommand_help("diff").contains("changes workspace-diff")); } } + +mod numeric_args { + use super::*; + + fn file_read(extra: &[(&str, &str)]) -> Result { + let mut args = vec![("action", "read"), ("repo", "/tmp/r"), ("path", "a.rs")]; + args.extend_from_slice(extra); + parse_remote_command_request("file", &remote_matches(&args)) + } + + #[test] + fn file_read_rejects_line_zero_and_reversed_ranges() { + file_read(&[("start-line", "2"), ("end-line", "2")]).unwrap(); + for extra in [ + &[("start-line", "0")][..], + &[("end-line", "0")][..], + &[("start-line", "5"), ("end-line", "2")][..], + ] { + let error = file_read(extra).unwrap_err(); + assert!( + error.starts_with("invalid_arguments:"), + "{extra:?}: {error}" + ); + } + } + + #[test] + fn file_search_rejects_a_zero_limit() { + let error = parse_remote_command_request( + "file", + &remote_matches(&[("action", "search"), ("repo", "/tmp/r"), ("limit", "0")]), + ) + .unwrap_err(); + assert!( + error.contains("--limit must be a positive integer"), + "{error}" + ); + } + + #[test] + fn negative_numbers_parse_as_values_and_are_rejected() { + let matches = crate::cli::args::parse([ + "treq", + "file", + "read", + "--repo", + "/tmp/r", + "--path", + "a.rs", + "--start-line", + "-1", + ]) + .unwrap(); + let sub = matches.subcommand.unwrap(); + let error = parse_remote_command_request("file", &sub.matches).unwrap_err(); + assert!( + error.contains("--start-line must be a positive integer"), + "{error}" + ); + } +} diff --git a/src-tauri/src/core/remote.rs b/src-tauri/src/core/remote.rs index bbe52e77e..57f832e0b 100644 --- a/src-tauri/src/core/remote.rs +++ b/src-tauri/src/core/remote.rs @@ -1822,7 +1822,9 @@ fn workspace_id(value: Option<&String>) -> Result, String> { .map(|value| { value .parse::() - .map_err(|_| "invalid_arguments: workspace must be a numeric id".to_string()) + .ok() + .filter(|id| *id > 0) + .ok_or_else(|| "invalid_arguments: workspace must be a positive numeric id".to_string()) }) .transpose() } @@ -1880,14 +1882,17 @@ pub fn execute_local_request(request: TreqCommandRequest) -> Result json(crate::core::changes::get_file_lines( - &repo, - workspace_id(workspace.as_ref())?, - &path, - revision == FileRevision::Parent, - start_line.unwrap_or(1), - end_line.unwrap_or(300), - )), + } => { + let start_line = start_line.unwrap_or(1); + json(crate::core::changes::get_file_lines( + &repo, + workspace_id(workspace.as_ref())?, + &path, + revision == FileRevision::Parent, + start_line, + end_line.unwrap_or(start_line.saturating_add(299)), + )) + } TreqCommandRequest::ListCommits { repo, workspace } => { json(crate::core::commits::list_commits( &repo, @@ -3166,6 +3171,37 @@ mod tests { use super::*; use std::process::Command; + #[test] + fn workspace_id_rejects_zero_and_negative_ids() { + assert_eq!(workspace_id(Some(&"3".to_string())), Ok(Some(3))); + for bad in ["0", "-1"] { + let error = workspace_id(Some(&bad.to_string())).unwrap_err(); + assert!(error.contains("positive"), "{bad}: {error}"); + } + } + + #[test] + fn read_file_defaults_end_line_to_a_300_line_window_from_start_line() { + let repo_dir = tempfile::tempdir().unwrap(); + let content: String = (1..=700).map(|n| format!("{n}\n")).collect(); + std::fs::write(repo_dir.path().join("long.txt"), content).unwrap(); + let read = |start_line: Option| { + let value = execute_local_request(TreqCommandRequest::ReadFile { + repo: repo_dir.path().to_str().unwrap().to_string(), + workspace: None, + path: "long.txt".into(), + revision: FileRevision::WorkingCopy, + start_line, + end_line: None, + }) + .unwrap(); + (value["start_line"].clone(), value["end_line"].clone()) + }; + + assert_eq!(read(None), (1.into(), 300.into())); + assert_eq!(read(Some(301)), (301.into(), 600.into())); + } + #[test] fn parses_ssh_hosts_ignoring_patterns() { let hosts = parse_ssh_config_hosts("\nHost prod bastion\n HostName example.com\nHost *\nHost !blocked *.internal test?\nHost dev\n"); diff --git a/src-tauri/tauri.conf.json b/src-tauri/tauri.conf.json index f431ebe2a..3fea9813b 100644 --- a/src-tauri/tauri.conf.json +++ b/src-tauri/tauri.conf.json @@ -418,7 +418,7 @@ { "name": "end-line", "takesValue": true, - "description": "Last line to read, inclusive (default: 300)" + "description": "Last line to read, inclusive (default: --start-line + 299, a 300-line window)" }, { "name": "value", diff --git a/src-tauri/tests/cli_parallel_commit_test.rs b/src-tauri/tests/cli_parallel_commit_test.rs index 8e619645f..25a7b9480 100644 --- a/src-tauri/tests/cli_parallel_commit_test.rs +++ b/src-tauri/tests/cli_parallel_commit_test.rs @@ -38,15 +38,19 @@ fn parallel_commits_on_one_workspace_do_not_diverge() { let mut succeeded = 0; for child in children { let out = child.wait_with_output().unwrap(); - let stderr = String::from_utf8_lossy(&out.stderr); + let output = format!( + "{}{}", + String::from_utf8_lossy(&out.stderr), + String::from_utf8_lossy(&out.stdout) + ); // A losing process sees a clean working copy; with an empty-commit guard it // fails with "nothing to commit", otherwise it is a no-op. if out.status.success() { succeeded += 1; } else { assert!( - stderr.contains("nothing to commit"), - "unexpected failure: {stderr}" + output.contains("nothing to commit"), + "unexpected failure: {output}" ); } }