Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src-tauri/src/cli/agent_review_handlers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ fn parse_line_number(value: &str, name: &str) -> Result<i64, String> {
value
.trim()
.parse::<i64>()
.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<Option<String>, String> {
Expand Down Expand Up @@ -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());
Expand Down
5 changes: 4 additions & 1 deletion src-tauri/src/cli/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {
Expand Down
22 changes: 19 additions & 3 deletions src-tauri/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,26 @@ fn optional_usize(matches: &Matches, name: &str) -> Result<Option<usize>, 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<usize>, Option<usize>), 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<String>) -> Vec<String> {
value
.map(|value| {
Expand Down Expand Up @@ -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 }),
Expand Down Expand Up @@ -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,
Expand Down
61 changes: 61 additions & 0 deletions src-tauri/src/cli/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<crate::core::remote::TreqCommandRequest, String> {
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}"
);
}
}
54 changes: 45 additions & 9 deletions src-tauri/src/core/remote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1822,7 +1822,9 @@ fn workspace_id(value: Option<&String>) -> Result<Option<i64>, String> {
.map(|value| {
value
.parse::<i64>()
.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()
}
Expand Down Expand Up @@ -1880,14 +1882,17 @@ pub fn execute_local_request(request: TreqCommandRequest) -> Result<serde_json::
revision,
start_line,
end_line,
} => 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,
Expand Down Expand Up @@ -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<usize>| {
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");
Expand Down
2 changes: 1 addition & 1 deletion src-tauri/tauri.conf.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
10 changes: 7 additions & 3 deletions src-tauri/tests/cli_parallel_commit_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
);
}
}
Expand Down
Loading