Skip to content

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

Merged
kdy1 merged 2 commits intomainfrom
kdy1/with-watch-review-followups
Apr 10, 2026
Merged

fix(with-watch): address merged PR review feedback#367
kdy1 merged 2 commits intomainfrom
kdy1/with-watch-review-followups

Conversation

@kdy1
Copy link
Copy Markdown
Contributor

@kdy1 kdy1 commented Apr 10, 2026

Summary

  • address the remaining merged-PR review follow-ups across the with-watch parser, planner, and runner
  • stop logging raw delegated argv and shell expressions while keeping structured spawn metadata for debugging
  • tighten watch-input validation and inference for blank explicit inputs, shell option tokens, grouped grep -f flags, and find -D/-O options
  • preserve self-mutation suppression while rerunning once when an external change lands during a self-mutating command run

Testing

  • cargo test -p with-watch
  • cargo test

@kdy1 kdy1 enabled auto-merge (squash) April 10, 2026 19:01
@kdy1 kdy1 merged commit 9bfc954 into main Apr 10, 2026
16 checks passed
@kdy1 kdy1 deleted the kdy1/with-watch-review-followups branch April 10, 2026 19:06
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e8b1a83ac5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +228 to +231
let should_rerun = if plan.metadata.side_effect_profile
== SideEffectProfile::WritesWatchedInputs
{
pending_rerun || additional_change_after_suppression
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Avoid rerun when only self-mutations occurred

In run, self-mutating commands now rerun whenever pending_rerun is set, even if all observed in-run changes were produced by the delegated command itself. Because pending_rerun is set after the second meaningful snapshot delta, commands that legitimately mutate watched inputs multiple times in one execution (for example, a shell command chaining two sed -i edits) will enqueue another run without any external file edit, creating unnecessary reruns and possible loops.

Useful? React with 👍 / 👎.

Comment on lines 142 to +144
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());
Copy link
Copy Markdown

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 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant