Skip to content

fix(with-watch): address merged PR review feedback #367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kdy1 merged 2 commits into from
Apr 10, 2026
Merged

fix(with-watch): address merged PR review feedback #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e8b1a83
fix(with-watch): address merged PR review feedback
kdy1 Apr 10, 2026
2d49604
fix(with-watch): satisfy clippy enum lint
kdy1 Apr 10, 2026
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
135 changes: 118 additions & 17 deletions crates/with-watch/src/analysis.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1131,20 +1131,27 @@ fn analyze_grep(
index += 1;
continue;
}
if let Some(value) = token.strip_prefix("-e") {
if !value.is_empty() {
explicit_pattern = true;
index += 1;
continue;
}
}
if let Some(value) = token.strip_prefix("-f") {
if !value.is_empty() {
explicit_pattern = true;
push_inferred_input(&mut inputs, value, cwd)?;
index += 1;
continue;
if let Some(option) = parse_grep_short_pattern_option(token) {
explicit_pattern = true;
match option {
GrepShortPatternOption::Inline => {}
GrepShortPatternOption::Next => {
index += 2;
continue;
}
GrepShortPatternOption::FileInline(value) => {
push_inferred_input(&mut inputs, value, cwd)?;
}
GrepShortPatternOption::FileNext => {
if let Some(value) = argv.get(index + 1) {
push_inferred_input(&mut inputs, value.as_str(), cwd)?;
}
index += 2;
continue;
}
}
index += 1;
continue;
}
if token.starts_with('-') {
index += 1;
Expand Down Expand Up @@ -1173,6 +1180,34 @@ fn analyze_grep(
Ok(analysis)
}

#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum GrepShortPatternOption<'a> {
Inline,
Next,
FileInline(&'a str),
FileNext,
}

fn parse_grep_short_pattern_option(token: &str) -> Option<GrepShortPatternOption<'_>> {
if !token.starts_with('-') || token == "-" || token.starts_with("--") {
return None;
}

let flags = token.trim_start_matches('-');
for (index, flag) in flags.char_indices() {
let value = &flags[index + flag.len_utf8()..];
match flag {
'e' if value.is_empty() => return Some(GrepShortPatternOption::Next),
'e' => return Some(GrepShortPatternOption::Inline),
'f' if value.is_empty() => return Some(GrepShortPatternOption::FileNext),
'f' => return Some(GrepShortPatternOption::FileInline(value)),
_ => {}
}
}

None
}

fn analyze_sed(
argv: &[String],
redirects: &[ShellRedirect],
Expand Down Expand Up @@ -1367,9 +1402,11 @@ fn analyze_find(
index += 1;
continue;
}
if !saw_expression && FIND_GLOBAL_OPTIONS.contains(&token) {
index += 1;
continue;
if !saw_expression {
if let Some(next_index) = consume_find_global_option(argv, index) {
index = next_index;
continue;
}
}
if !saw_expression && !is_find_expression_token(token) {
push_inferred_input(&mut inputs, token, cwd)?;
Expand Down Expand Up @@ -1398,7 +1435,31 @@ fn analyze_find(
Ok(analysis)
}

const FIND_GLOBAL_OPTIONS: &[&str] = &["-H", "-L", "-P", "-D", "-O"];
fn consume_find_global_option(argv: &[String], index: usize) -> Option<usize> {
let token = argv[index].as_str();
match token {
"-H" | "-L" | "-P" => Some(index + 1),
"-D" => Some((index + 2).min(argv.len())),
"-O" => {
if argv
.get(index + 1)
.is_some_and(|value| is_unsigned_integer(value))
{
Some(index + 2)
} else {
Some(index + 1)
}
}
_ if token.starts_with("-D") && token.len() > 2 => Some(index + 1),
_ if token.starts_with("-O") && token.len() > 2 => Some(index + 1),
_ => None,
}
}

fn is_unsigned_integer(token: &str) -> bool {
let trimmed = token.trim();
!trimmed.is_empty() && trimmed.chars().all(|character| character.is_ascii_digit())
}

fn analyze_xargs(
argv: &[String],
Expand Down Expand Up @@ -2173,6 +2234,18 @@ mod tests {
)
.expect("analyze");
assert_eq!(analysis.inputs.len(), 2);

let grouped = analyze_argv(
&[
OsString::from("grep"),
OsString::from("-rf"),
OsString::from("patterns.txt"),
OsString::from("src"),
],
cwd.path(),
)
.expect("analyze");
assert_eq!(grouped.inputs.len(), 2);
}

#[test]
Expand Down Expand Up @@ -2214,6 +2287,34 @@ mod tests {
let analysis = analyze_argv(&[OsString::from("find")], cwd.path()).expect("analyze");
assert_eq!(analysis.inputs.len(), 1);
assert!(analysis.default_watch_root_used);

let analysis = analyze_argv(
&[
OsString::from("find"),
OsString::from("-D"),
OsString::from("stat"),
OsString::from("-name"),
OsString::from("*.rs"),
],
cwd.path(),
)
.expect("analyze");
assert_eq!(analysis.inputs.len(), 1);
assert!(analysis.default_watch_root_used);

let analysis = analyze_argv(
&[
OsString::from("find"),
OsString::from("-O"),
OsString::from("3"),
OsString::from("-name"),
OsString::from("*.rs"),
],
cwd.path(),
)
.expect("analyze");
assert_eq!(analysis.inputs.len(), 1);
assert!(analysis.default_watch_root_used);
}

#[test]
Expand Down
13 changes: 13 additions & 0 deletions crates/with-watch/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ fn explicit_watch_inputs(raw_inputs: &[String], cwd: &Path) -> Result<Vec<WatchI
};
push_unique_input(&mut inputs, input);
}
if inputs.is_empty() {
return Err(WithWatchError::NoWatchInputs);
}
Ok(inputs)
}

Expand All @@ -139,6 +142,7 @@ fn has_glob_magic(raw: &str) -> bool {
#[cfg(test)]
mod tests {
use super::explicit_watch_inputs;
use crate::error::WithWatchError;

#[test]
fn explicit_inputs_accept_globs_and_paths() {
Expand All @@ -151,4 +155,13 @@ mod tests {

assert_eq!(inputs.len(), 2);
}

#[test]
fn explicit_inputs_reject_blank_values() {
let temp_dir = tempfile::tempdir().expect("create tempdir");
let error = explicit_watch_inputs(&[" ".to_string(), "\t".to_string()], temp_dir.path())
.expect_err("blank inputs should fail");

assert!(matches!(error, WithWatchError::NoWatchInputs));
}
}
16 changes: 13 additions & 3 deletions crates/with-watch/src/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,9 +140,8 @@ fn build_shell_command(command: starbase_args::Command) -> Result<ShellCommand>
shell_command.argv.push(flag);
}
starbase_args::Argument::Option(option, Some(value)) => {
shell_command
.argv
.push(format!("{option}={}", value.as_str()));
shell_command.argv.push(option);
shell_command.argv.push(value.as_str().to_string());
Comment on lines 142 to +144
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve option-value pairing for shell analysis

Splitting Argument::Option(option, Some(value)) into two argv tokens causes downstream analyzers to reinterpret option values as positional arguments when they don't explicitly model that option. A concrete regression is --shell "grep --color=auto foo": --color=auto becomes --color auto, auto is treated as the consumed pattern, and foo is incorrectly inferred as a watch input instead of returning NoWatchInputs, so shell mode can silently watch bogus paths.

Useful? React with 👍 / 👎.

}
starbase_args::Argument::Option(option, None) => {
shell_command.argv.push(option);
Expand Down Expand Up @@ -250,6 +249,17 @@ mod tests {
assert_eq!(command.redirects[1].target, "output.txt");
}

#[test]
fn preserves_shell_option_values_as_separate_tokens() {
let parsed = parse_shell_expression("grep -f patterns.txt file.txt").expect("parse shell");

assert_eq!(parsed.commands.len(), 1);
assert_eq!(
parsed.commands[0].argv,
vec!["grep", "-f", "patterns.txt", "file.txt"]
);
}

#[test]
fn rejects_shell_control_flow_keywords() {
let error =
Expand Down
Loading
Loading