fix(file): dash-safe args, real error status, symlink delete, streaming read#14
Conversation
…ng read - grep/ls get a `--` separator (find gets a ./ anchor): a pattern like `->` errored, and `-r` silently recursed with the path as pattern. - sh() now returns Err on non-zero exit instead of reporting failures as MCP successes whose body happens to contain the error text; grep exit 1 maps to an explicit "[grep: no matches]" result. - delete uses symlink_metadata: a dir-symlink is unlinked (not refused by remove_dir_all), a dangling symlink is deletable at all. - read streams line-by-line holding only the requested window, instead of materializing the whole file (an OOM risk on multi-GB files) before paginating. Same page format and byte/line ceilings via jobs::paginate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughChanges to src/tools/files.rs rework file-reading to stream lines via an async BufReader rather than loading whole files, switch delete to symlink_metadata to correctly unlink symlinks, and revise list/grep shell execution to sanitize arguments and return a new ShError on command failure instead of Ok. Tests were added accordingly. ChangesFile tools hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant list_grep as list/grep
participant sh
participant Shell
Caller->>list_grep: invoke with path/pattern
list_grep->>sh: run command with -- delimiters
sh->>Shell: execute
Shell-->>sh: exit code, stdout, stderr
alt grep exit 1, empty stdout
sh-->>list_grep: "[grep: no matches]"
else non-success exit
sh-->>list_grep: ShError
else success
sh-->>list_grep: Ok(output)
end
list_grep-->>Caller: result or error
Related Issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: rust, bugfix, security Suggested reviewers: None found in the provided context. Poem: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/tools/files.rs`:
- Around line 293-365: The test code currently lives inline in the main files
module, pushing the file over the LOC limit. Move the existing `#[tokio::test]`
cases out of `src/tools/files.rs` into a new `tests` submodule and wire it up
with `#[cfg(test)] mod tests;` so the production module stays under 300 LOC.
Keep the test names like
`grep_pattern_starting_with_dash_is_a_pattern_not_an_option`,
`grep_no_matches_is_ok_and_distinguishable`,
`failed_shell_command_is_an_error_not_a_success`, and
`delete_unlinks_symlinks_instead_of_following` unchanged in the new
`src/tools/files/tests.rs` file.
- Around line 41-56: The pagination logic in the file-reading path can still
buffer an entire oversized line before trimming, so `read_until` in the
line-reading flow should be changed to enforce a bounded per-line read strategy.
Update the line handling in `src/tools/files.rs` around the `read_until` loop so
it either reads in capped chunks or stops accumulating once a maximum line size
is reached before applying `cursor` and `limit`, and add a regression test for a
very long file with no newline to verify memory stays bounded.
- Around line 163-166: Update the ShError handling in files-related command
execution so the grep no-match case stays distinct from signal-based exits; keep
the existing Option<i32> status representation, or introduce a separate signal
variant instead of collapsing everything into i32. Also derive thiserror::Error
for ShError and adjust the matching in the function around the grep/status
handling to preserve the separate no-match path while keeping error conversion
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2cab304e-d345-4321-92c2-1192ff9a7d8c
📒 Files selected for processing (1)
src/tools/files.rs
- read() streams via fill_buf/consume with a 64 KiB per-line cap (append_capped) so a no-newline multi-GB file can't OOM; regression test added - ShError derives thiserror::Error; split Status/Signal variants make illegal states unrepresentable, grep no-match path stays distinct - move test module to src/tools/files/tests.rs; files.rs now 263 LOC (was 444), under the 300 cap Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
File-tool correctness fixes from the bug hunt, all with tests:
grep/ls. Patterns/paths were passed positionally with no--:grepfor->failed with invalid option, and a pattern like-rsilently shifted arguments (the path became the pattern, recursing cwd — wrong results, no error).findhas no--, so a leading-dash relative path is anchored with./.sh()ignored the exit status, sofile(action="list", path="/does/not/exist")returned a success whose body wasls: cannot access .... Non-zero exit is now an error; grep's exit-1 (zero matches) maps to an explicit[grep: no matches]result instead of an ambiguous empty string.deletefollowed symlinks.fs::metadataclassified a dir-symlink as a directory (remove_dir_allrefuses the top-level link → undeletable) and errored ENOENT on dangling links. Now stats withsymlink_metadataand unlinks the link itself.readslurped whole files. Reading 200 lines of a multi-GB log materialized the entire file (2–3× its size in RAM) before paginating — an OOM risk that takes down every client. Now streams line-by-line, holding only the requested window; identical page format and ceilings.🤖 Generated with Claude Code
Summary by CodeRabbit