fix(grep): skip binary and non-UTF-8 input instead of aborting

[so0k]

Description

grep read every line into a String, so the first file that wasn't valid UTF-8 made read_line fail with InvalidData. cmd_grep then propagated that with ?, which aborted the whole file loop: the command exited 1 with grep: stream did not contain valid UTF-8, and every file after the offending one (in directory order) was silently never scanned.

This change makes grep byte-oriented and handles binary input the way GNU grep 3.11 does (UTF-8 locale; checked against both its output and src/grep.c):

• Lines are read with read_until(b'\n'), so file content can no longer make grep fail.
• A NUL byte in the first 96 KiB (GNU's INITIAL_BUFSIZE) marks the whole file binary. Its lines are never printed; a match produces grep: FILE: binary file matches on stderr, with exit status 0, and reading stops after the first match. A NUL found later makes the rest of the file binary from that line on; lines already printed stay printed, and under -I the file counts as non-matching.
• Matching runs on raw bytes (regex::bytes), so . never matches an invalid byte. A line with invalid UTF-8 is not printed; the file's other lines print normally, followed by the same stderr note. With -o, only matches whose own bytes are invalid are hidden.
• -c, -l, -L and -q count binary matches as usual and print no note; -q/-l/-L stop at the first match.
• Context (-A/-B/-C) follows GNU's model: a hidden line keeps its slot in the window but is never written, -- only appears when a group isn't contiguous with the last line written, and -o writes no context lines. This also fixes two context bugs already on main: a spurious -- between adjacent groups, and -o printing context.
• New options: -a/--text, -I, and --binary-files=binary|text|without-match (a small BinaryFiles enum).
• A read error on one file is now reported as grep: PATH: ERR and grep moves on to the next file, the same way open errors are already handled. A closed stdout (BrokenPipe, e.g. grep -r … | head -n 1) still stops grep straight away.
• grep takes stderr once up front and shares it across files. Previously it called io::stderr() for each open error, and the second call failed because the writer had already been taken.

Before (0.3.3) and after, using the repro from #123 (Node binding, direct read-only bind):

$ grep -rn needle /mnt
- status: 1  stdout: "/mnt/c.txt:1:needle in c.txt too\n"  stderr: "strands-shell: grep: stream did not contain valid UTF-8\n"
+ status: 0  stdout: "/mnt/c.txt:1:needle in c.txt too\n/mnt/a.txt:1:needle in a.txt\n"  stderr: ""
$ grep -rnI needle /mnt
- status: 1  stderr: "strands-shell: grep: invalid option '-I'\n"
+ status: 0  (same two matches)

Known differences from GNU grep (not addressed here)

• -a prints invalid bytes as U+FFFD rather than raw bytes, because captured shell output must be valid UTF-8 (a raw byte would empty the whole stdout, as already happens with cat on such a file).
• After a NUL, GNU treats each NUL as a line break, so printf 'a\0a\n' | grep -c a gives 2 there and 1 here.
• A NUL found after the first 96 KiB switches to binary from its line on; GNU switches per read buffer.
• When the first selected line is hidden, GNU (with -A) prints the lines before it as after-context, an artifact of its unset lastout. This isn't copied.
• Trailing \r is stripped before matching (main stripped every trailing \r, this strips one), whereas GNU keeps it. CRLF files therefore print without \r.

Out of scope, possible follow-ups

• GNU exits with 2 when any file errored. This PR keeps the existing exit-status behaviour for open errors and read errors.
• -r visits files in unsorted directory order, as before.
• cargo clippy --workspace --all-targets -- -D warnings already fails on main (find.rs, lua.rs, ls.rs, exec.rs). This PR adds no new warnings.

Related Issues

Fixes #123

Documentation PR

None yet. The command reference page could list the new -a, -I and --binary-files options.

Type of Change

Bug fix

Testing

• New integration tests in tests/shell_integration.rs. They bind a host directory of raw-byte fixtures (\0, \xff) and cover: the [BUG] grep -r aborts the whole command (and silently drops matches from other files) when a bound directory contains a non-UTF-8 file #123 repro (recursive search continues past a binary file), invalid UTF-8 lines being hidden, -I/-a/--binary-files, -c/-l/-q, the stdin (standard input) note, context windows around hidden lines (pinned to GNU output), a binary file with no match (exit 1, no note), BrokenPipe stopping after the first file, . not matching an invalid byte, -o on invalid lines, and a NUL after the first 96 KiB.

• Ran the [BUG] grep -r aborts the whole command (and silently drops matches from other files) when a bound directory contains a non-UTF-8 file #123 repro end to end against a local npm run build:debug of the Node addon (output above).

• Compared against GNU grep 3.11 (debian:stable-slim, LC_ALL=C.UTF-8) and BSD grep 2.6.0, including a 273-case matrix (21 fixtures × 13 option sets) that is byte-identical to GNU except for the known differences above, and traced the binary and encoding-error paths in the grep 3.11 source (buf_has_nulls, print_line_head, print_line_middle).

• I ran the relevant test suites for the bindings I touched (cargo test --workspace --all-targets, pytest tests/python, npm test): Rust core only, no binding surface changed. Ran cargo test (all pass) and npm test (41/41); pytest not run locally.

• If I touched Rust, I ran cargo fmt and cargo clippy

Checklist

• I have read the CONTRIBUTING document
• I have reviewed and understand every line of code in this PR, including any generated by AI tools, and I can explain why it works
• My change is focused and reasonably small; I have split unrelated work into separate PRs
• I have added any necessary tests that prove my fix is effective or my feature works
• I have updated the documentation accordingly
• I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
• My changes generate no new warnings
• Any dependent changes have been merged and published

---

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AXaT7zwomYh4CtqSGrCY1Y

[so0k]

Hi @mkmeral @zastrowm, could one of you approve the fork CI run and take a look when you have a moment? This fixes #123: grep -r aborting (and dropping matches from other files) when a directory contains a non-UTF-8 file. Happy to adjust the approach if you'd prefer something smaller. Thanks!

[so0k]

Pushed efce0d9, which fixes a few places where the first commit differed from GNU grep 3.11 (checked against src/grep.c):

• Matching runs on raw bytes (regex::bytes), so . no longer matches an invalid byte.
• -o hides only matches whose own bytes are invalid, not every match on that line.
• A NUL anywhere in the first 96 KiB marks the file binary. A later NUL makes the rest of the file binary, and -I then counts the file as non-matching.
• -q/-l/-L stop at the first match.

The description is updated, including the remaining known differences (e.g. -a prints U+FFFD for invalid bytes).

[strandly-the-agent]

Verdict: comment — no blockers. The #123 abort is genuinely fixed and the plain-text path is untouched: 48 text-only invocations are byte-identical to main, and NUL-offset behaviour matches GNU 3.11 at 9 KB / 50 KB / 118 KB. Two 🟡 consistency bugs in the new binary path, both inline:

• grep -I on a non-UTF-8 file can exit 0 with empty stdout and empty stderr (src/commands/grep.rs:306).
• A hidden binary line evicts valid text before-context lines the user asked for (src/commands/grep.rs:330) — the "still occupy a context slot" comment undersells this, and your known-differences list covers separators, not dropped lines.

Non-blocking design question: hiding matches on invalid-UTF-8 lines in otherwise-text files (a Latin-1 source file) means stdout shows nothing and only stderr carries the note. GNU in C.UTF-8 prints the line. For an agent that reads stdout, is stderr-only the right channel for "I found it but won't show you"? -a is the escape hatch, but the agent has to know to reach for it.

✅ What I verified (head 5dc6163, base 8f29276)

• cargo test --test shell_integration → 1317 passed, 0 failed; grep_ filter → 94 passed.
• #123 repro on a directory containing \xff and \0 files: main → exit 1, 0 matching lines, grep: stream did not contain valid UTF-8; this branch → exit 0, every file scanned.
• 48 plain-text invocations (-n -c -v -o -A -B -C -m -l -L -H -h -e -F -i -w -x -r, plus combinations) diffed main vs branch: identical stdout+stderr+exit in all but the CRLF case in appendix 2.
• NUL offsets vs GNU grep 3.11: NUL at byte 9,510 / 49,910 / 117,910 → stdout, stderr and exit identical to GNU in all three, so fill_buf() really does deliver the full 96 KiB through the VFS. Late NUL arriving over a pipe on stdin also matches GNU.
• Your stderr claim checks out: take_writer removes the fd (src/os.rs:471-479), and on main a second unreadable file loses its message entirely — grep needle nope1 nope2 prints one line there, both here. That's a second real bug fixed by this PR.
• BrokenPipe: grep needle big.txt | head -n 2 → 2 lines, exit 0, no per-file repetition.
• Full log uploaded as pr124-verification.txt (commands + raw output for every claim above).

Appendix — non-blocking (6)

1. ⚪ -o + -A loses the note. binary_matched |= !opts.only_matching; at :403 and its comment assume -o prints no context — but it does here: printf 'foo\nbar\n' | grep -o -A1 foo prints foo and bar (GNU prints only foo). So grep -o -A1 needle file hides an invalid after-context line with no note at all, while -o -B1 reports it via :351. binary_matched = true; fixes the inconsistency.
2. ⚪ CRLF. strip_suffix at :294-295 replaces main's trim_end_matches, so a line ending \r\r\n now keeps one \r (main stripped both; GNU keeps both). Closer to GNU — just not in your differences list.
3. ⚪ Pre-existing, not introduced here: spurious -- on pure text. printf 'needle A\nl2\nl3\nl4\nneedle B\n' > f; grep -B3 needle f prints needle A / -- / l2 / l3 / l4 / needle B; GNU emits no separator because the groups are adjacent. Identical on main ((opts.before_context == 0 || !before_buf.is_empty()) && need_sep at :345). Not filed — worth a one-line issue if you agree.
4. ⚪ Pre-existing, and you already flag it in a test comment: single-file -l/-L prints an empty line instead of the filename (:444-449).
5. ⚪ One more GNU divergence not in your list: printf '\0\xffneedle\n' | grep -vc needle → 0, exit 1 here; GNU 3.11 says 1, exit 0. Yours looks like the defensible answer.
6. Scope. +155 lines and three new flags is more than the minimal "stop aborting" repair, but suppression is what keeps binary bytes out of an agent's context, and -a / -I are the escape hatches an agent will actually try. I'd keep it.

Reading order for a human reviewer

src/commands/grep.rs:255-283 (detection + flags) → :284-338 (hidden-match path, where both 🟡s live) → :397-435 (context slot accounting) → tests/shell_integration.rs:2143-2213 (context tests, which encode the current semantics).

Caveat on this review: the independent reviewer / adversarial subagents weren't available in this run, so all passes were run inline by me. The context state machine in particular deserves a human eye.

[strandly-the-agent]

grep -I on a non-UTF-8 file can report success with no output at all.

A line that is invalid UTF-8 but has no NUL still counts as a match (found = true, match_count += 1), is then hidden, and under --binary-files=without-match the stderr note at :450 is suppressed too:

$ printf 'caf\xe9 needle\nplain\n' > latin1.txt
$ grep -I needle latin1.txt    # exit 0, stdout empty, stderr empty
$ grep -cI needle latin1.txt   # 1
$ grep -lI needle latin1.txt   # lists the file

So -I reports matching for exactly the input it is meant to treat as non-matching, and the caller gets no signal at all. The NUL path at :298-302 already handles this by zeroing match_count/found; doing the same when a line is hidden as invalid would make -I consistent (exit 1, -c → 0). tests/shell_integration.rs:2209 only covers a file that also has valid matching lines, so this case isn't pinned.

[so0k]

GNU 3.11 (LC_ALL=C.UTF-8) behaves the same: -I only acts on NUL detection (src/grep.c:1558), and lines with encoding errors are dropped at print time with no note. On your latin1.txt, grep -I needle gives empty stdout, empty stderr, exit 0, and grep -cI prints 1. Leaving it for GNU parity.

[strandly-the-agent]

A hidden binary line discards valid text before-context lines inside the requested window.

before_buf.clear() here (and at :408 for the non-matching case) throws away lines the user explicitly asked for:

$ printf 'l1\nl2\n\xffneedle hid\nneedle vis\n' > hidm.txt
$ grep -B2 needle hidm.txt
needle vis                 # l1 and l2 are gone; GNU 3.11 prints both
$ printf 'needle A\nl2\n\xffbin\nl4\nneedle B\n' > sep4.txt
$ grep -B3 needle sep4.txt # needle A / -- / l4 / needle B; GNU keeps l2

Those dropped lines are plain text, and the only signal is binary file matches, which reads as "nothing printable here", not "I also withheld 2 text lines". :398's "they still occupy a context slot" describes slot accounting, not eviction.

Suggestion: keep before_buf intact and let the ring-buffer window drop lines naturally — last_binary_lineno already tracks whether the note is owed.

[so0k]

Good catch, fixed in 60f6373. A hidden line now keeps its slot in the window instead of clearing the buffer, following GNU's lastout model. Both of your examples now match GNU 3.11 output, and the new tests pin them. The same commit fixes appendix 1 (-o writes no context, so the note is reported) and appendix 3 (spurious -- between adjacent groups).

[so0k]

Thanks for the thorough review. On the stdout question: GNU 3.11 in C.UTF-8 also hides the line. grep needle latin1.txt gives empty stdout, grep: latin1.txt: binary file matches on stderr, exit 0. I'd keep that for parity, with -a as the opt-in.

Appendices 2 (CRLF) and 5 are both in the description's known differences. Appendix 5 is the NUL-as-line-break one: GNU splits \0\xffneedle into two lines, so -v selects the empty one. Appendix 4 predates this PR, so I've left it out.

[vincenthsh]

@strandly-the-agent we have an error running strands shell in a workspace with binary files grep errors, any update?