feat: add rn watch entity for file change monitoring#48
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a watch command to the run utility, enabling users to monitor a file for modifications and execute a specified command upon change. The update includes shell completion support and comprehensive unit tests. Review feedback highlights the need for more robust error handling in the watch loops; specifically, the inotifywait and polling implementations should gracefully handle file deletions or access errors to avoid infinite loops or high CPU usage. There is also a recommendation to reconsider the use of eval for command execution to mitigate potential security risks.
| while true; do | ||
| inotifywait -qq -e modify "$file" | ||
| eval "$cmd" | ||
| done |
There was a problem hiding this comment.
The current while true loop can lead to a high-CPU busy-loop if the watched file is deleted or becomes inaccessible. inotifywait will start failing with a non-zero exit code, but the loop will continue, causing it to be called repeatedly without blocking.
A better pattern is to use inotifywait as the condition for the while loop. This way, the loop will automatically terminate when inotifywait returns a non-zero exit code (e.g., on error or file deletion).
| while true; do | |
| inotifywait -qq -e modify "$file" | |
| eval "$cmd" | |
| done | |
| while inotifywait -qq -e modify "$file"; do | |
| eval "$cmd" | |
| done | |
| echo "File '$file' is no longer being watched." >&2 |
| while true; do | ||
| sleep 1 | ||
| local cur_mod | ||
| cur_mod=$(stat -c %Y "$file" 2>/dev/null || stat -f %m "$file" 2>/dev/null) | ||
| if [[ "$cur_mod" != "$last_mod" ]]; then | ||
| last_mod="$cur_mod" | ||
| eval "$cmd" | ||
| fi | ||
| done |
There was a problem hiding this comment.
The polling loop does not handle file deletion gracefully. If the file is deleted, stat will fail, cur_mod will be empty, and the command will be executed one last time before the loop goes idle. The loop will then continue polling a non-existent file indefinitely.
The loop should detect when stat fails and exit cleanly, informing the user that the file is no longer accessible.
| while true; do | |
| sleep 1 | |
| local cur_mod | |
| cur_mod=$(stat -c %Y "$file" 2>/dev/null || stat -f %m "$file" 2>/dev/null) | |
| if [[ "$cur_mod" != "$last_mod" ]]; then | |
| last_mod="$cur_mod" | |
| eval "$cmd" | |
| fi | |
| done | |
| while true; do | |
| sleep 1 | |
| local cur_mod | |
| cur_mod=$(stat -c %Y "$file" 2>/dev/null || stat -f %m "$file" 2>/dev/null) | |
| if [[ -z "$cur_mod" ]]; then | |
| echo "File '$file' no longer accessible. Stopping watch." >&2 | |
| break | |
| fi | |
| if [[ "$cur_mod" != "$last_mod" ]]; then | |
| last_mod="$cur_mod" | |
| eval "$cmd" | |
| fi | |
| done |
| if command -v inotifywait &>/dev/null; then | ||
| while true; do | ||
| inotifywait -qq -e modify "$file" | ||
| eval "$cmd" |
There was a problem hiding this comment.
Using eval on command strings can be risky and lead to unexpected behavior due to double evaluation of shell metacharacters. It's generally safer to execute commands in a more controlled environment.
While eval allows executing shell functions from the parent environment, consider if this is a required feature. If not, a safer alternative like bash -c "$cmd" could be used, which runs the command in an isolated subshell. This advice also applies to line 118.
Greptile SummaryThis PR adds a
Confidence Score: 5/5Safe to merge; the only remaining finding is a P2 improvement to the inotifywait event mask Both previously flagged P1 bugs are fixed. The single remaining comment is a P2 suggestion (missed events on atomic editor saves) — it does not block correctness and there are no P1 or P0 issues outstanding lib/run.sh lines 105-107 — consider expanding the inotifywait event list Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([u7 rn watch file run cmd]) --> B{Args valid?}
B -- No --> C[Print usage / return 1]
B -- Yes --> D{File exists?}
D -- No --> E[Print error / return 1]
D -- Yes --> F{Dry-run?}
F -- Yes --> G[Print dry-run message / return 0]
F -- No --> H{inotifywait available?}
H -- Yes --> I[while inotifywait -e modify file]
I -- modify event --> J[eval cmd]
J --> I
I -- exit non-zero --> K([Loop exits])
H -- No --> L[stat mtime loop, sleep 1s]
L --> M{File gone?}
M -- Yes --> N([Break with message])
M -- No --> O{mtime changed?}
O -- Yes --> P[eval cmd]
P --> L
O -- No --> L
Prompt To Fix All With AIThis is a comment left during a code review.
Path: lib/run.sh
Line: 105-107
Comment:
**`inotifywait -e modify` misses editor atomic writes**
Many editors (vim, nano, gedit, VS Code by default) use atomic saves: they write to a temp file and then `rename()` it over the original. The kernel fires `IN_MOVED_TO` (and `IN_CREATE`) on the directory, *not* `IN_MODIFY` on the original inode, so `inotifywait -e modify` never wakes up for those saves.
The polling fallback is immune to this (it checks `mtime` via `stat`), so users on Linux with `inotifywait` will see silently missed updates that users on macOS (poll path) would catch — a hard-to-debug behavioural split.
Adding the common safe-write events covers the gap:
```suggestion
while inotifywait -qq -e modify,close_write,create,moved_to "$file"; do
eval "$cmd"
done
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (2): Last reviewed commit: "fix: resolve merge conflict, keep both r..." | Re-trigger Greptile |
There was a problem hiding this comment.
Code Review
This pull request introduces a watch command to the u7 run utility, enabling users to monitor a specific file and execute a command whenever changes are detected. The implementation includes shell completion support, a primary execution path using inotifywait, and a polling-based fallback mechanism. Feedback suggests improving security and robustness by replacing eval with bash -c for command execution and removing a redundant touch operation in the test suite.
| if command -v inotifywait &>/dev/null; then | ||
| while true; do | ||
| inotifywait -qq -e modify "$file" | ||
| eval "$cmd" |
There was a problem hiding this comment.
Using eval can introduce security vulnerabilities and unexpected behavior due to its complex parsing rules. A safer and more robust approach is to execute the command in a subshell using bash -c. This contains the execution and avoids altering the current shell's state. This pattern is already used for the priority command in this script.
| eval "$cmd" | |
| bash -c "$cmd" |
| cur_mod=$(stat -c %Y "$file" 2>/dev/null || stat -f %m "$file" 2>/dev/null) | ||
| if [[ "$cur_mod" != "$last_mod" ]]; then | ||
| last_mod="$cur_mod" | ||
| eval "$cmd" |
| assert_contains "rn watch shows usage on missing args" "Usage" "$result" | ||
|
|
||
| # Test: rn watch usage (missing run keyword) | ||
| touch watch_target.txt |
There was a problem hiding this comment.
This touch command is unnecessary for this test case. The u7 rn watch command checks for the run keyword before it checks for the file's existence. The test will pass correctly without this line, making it more focused on its purpose: validating the presence of the run keyword. This line can be removed.
Summary
u7 rn watch <file> run <command>to watch a file for changes and execute a command when modifiedinotifywaitwhen available, falls back to a polling loop withstat(1-second interval)watchto tab completion in completions.shTest plan
bash test.shpasses all 142 tests (0 failures)u7 rn watch somefile run echo changedtriggers on file modificationu7 -n rn watch somefile run echo changedprints dry-run message