Repository navigation
fix(read_only): sed is a read only with -n and a script that cannot write or run - #95
Conversation
Tiny Sweeper reviewReviewed head: Verdict: Ready for maintainer review · Priority: none Diagnostics and supporting detailsFull summaryPull request purpose (title): fix(read_only): sed is a read only with -n and a script that cannot write or run. This pull request narrows the `sed` allowance in `is_read_only_command` so that `sed` counts as a plain file read only when its sole flag is `-n` and its script contains none of `w`, `W`, `e` or `r`. Previously any `sed` without `-i`/`--in-place` was treated as read-only, which admitted scripts that write files or execute shell commands. A new table-driven test pins both the accepted `-n` quoted-script form and the rejected write/execute, missing `-n`, and `-E`/`-e` forms. No trusted push-delta comparison is available, so this describes the whole pull request rather than a push delta. Review snapshot
Completeness: Complete What changed`sed` classification is tightened from "no `-i`/`--in-place" to "plain `-n` read with a script free of `w`/`W`/`e`/`r`". Callers of `is_read_only_command` / `is_read_only_call` will now get `false` for `sed` invocations lacking `-n`, using any other flag (`-E`, `-e`, …), or whose script contains those letters, so the host summarizes those calls instead of skipping the summary. The quoted-script case is handled and the module/`READERS` docs are updated to match. The check is deliberately conservative: a regex such as `/error/p` is refused too, costing only the summary skip. Features
TestsNo supported feature-to-test mapping was produced. Test execution is not inferred. FindingsNo active actionable findings. Before mergeNone. Agent review detailscritique
security
tests
commits
description
Evidence and run details
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configuration
Comment |
Summary
is_read_only_commandlets a host skip the LLM summary for shell commands that only print files. It treated anysedwithout-i/--in-placeas a plain read. A sed script can still write or run things without-i:sed -n '1 w out.txt' fandsed -n 's/a/b/w out.txt' fwrite a file, andsed -n '1e touch x' fands/x/y/erun a shell command. All of these were classified as read-only.sedwithout-nwas also accepted, although it prints the whole stream rather than a chosen slice.Now
sedcounts as a read only when its only flag is-nand its script has now,W,eorrcommand. The script is the first non-flag argument, and quoted scripts with spaces are handled. The check is deliberately conservative: it refuses those letters anywhere in the script, so a regex such as/error/pis refused as well. That costs only the summary skip for that call, so the conservative choice is the safe one.Related issue
None.
API or behavior changes
is_read_only_command/is_read_only_callnow returnfalseforsedcalls that lack-n, use any other flag (-E,-e, …), or have a script containingw,W,eorr. The host then summarizes those calls as it does for any other command.sed -n '<range>p' file, the form agents actually use, is still read-only.Validation
Commands actually run, with their outcome:
cargo fmt --all -- --check:rustfmt --edition 2024on the touched files only.cargo clippy --all-targets --all-features -- -D warningswas left to CI.cargo build --all-targets --all-featureswas left to CI.cargo test --all-featureswas left to CI. Only thefilesystem::read_onlytests ran locally:sed_counts_only_with_n_and_a_script_that_cannot_write_or_runbefore the fix failed (sed -n '1 w out.txt' notes.mdwas read-only); after the fix it passed, along with the 3 existing read_only tests.Tests
filesystem/read_only_tests.rs::sed_counts_only_with_n_and_a_script_that_cannot_write_or_runcoversw,s///w,W,e,s///e,r, missing-n,-Eand-e, which must all be refused. It also checks that a quoted script with spaces (sed -n '/fn main/p') is still a read.Documentation
The module doc and the
READERSdoc now describe the narrowersedrule.Checklist
#[allow(...)],#[ignore], or relaxed lints.envcontents in the diff or the description