From 98721aa06ef1f7c1d746adf2168e99343e93c95c Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 23 Sep 2026 10:57:35 +0000 Subject: [PATCH 1/2] fix(grep): align with GNU grep and gate the spec suite against it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The grep spec suite had never been compared against the real tool, so its expectations recorded whatever Bashkit printed when they were written. Pointing the differential gate at it found six divergences from GNU grep 3.11, every one of them encoded in the spec as if correct: printf 'foo\n' | grep -l foo (stdin) -> (standard input) printf 'foo\0bar\0' | grep -z foo foo\n -> foo\0 printf 'foo\0bar\n' | grep -a foo foobar -> foo\0bar printf 'foo\0bar\n' | grep foo stdout notice -> stderr, GNU wording grep -L foo matching.txt exit 1 -> exit 0 grep -L foo other.txt exit 0 -> exit 1 Two are more than cosmetic. `-a` means "treat this binary file as text", not "delete its NUL bytes", so stripping them altered the bytes the caller got back. The binary-match notice is a diagnostic, so GNU >= 3.5 writes `grep: FILE: binary file matches` to stderr; on stdout it joined the data a pipeline reads, the same way the read diagnostic did before #2441. `-L`'s status was inverted. GNU reports whether a *match* was found, whatever `-L` chose to print: a matching file prints nothing and exits 0, a non-matching file prints its name and exits 1. `--color=always` stays a no-op and is now marked `### bash_diff` rather than asserting plain output as if it were GNU's. The gate itself is the point. `grep_comparison_tests` joins the bash and sed gates, and CI now runs all three (`spec_tests:: --ignored`) instead of only the bash one — which is why the sed gate added with the GNU-sed work had never actually run there either. --- .github/workflows/ci.yml | 9 +- crates/bashkit/src/builtins/grep.rs | 86 +++++++------- .../integration/grep_gnu_parity_tests.rs | 112 ++++++++++++++++++ crates/bashkit/tests/integration/main.rs | 1 + .../bashkit/tests/integration/spec_tests.rs | 15 +++ .../tests/spec_cases/grep/grep.test.sh | 44 ++++--- knowledge/operations/testing.md | 28 ++++- 7 files changed, 230 insertions(+), 65 deletions(-) create mode 100644 crates/bashkit/tests/integration/grep_gnu_parity_tests.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30032e4aa..1598611c0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -263,8 +263,13 @@ jobs: - name: Run tests run: cargo test --workspace --lib --bins --tests --features http_client,ssh,sqlite - - name: Run strict bash parity tests - run: cargo test -p bashkit --test integration --features http_client,ssh -- spec_tests::bash_comparison_tests --ignored + # All three differential gates (bash, sed, grep), not just bash: each is + # the only thing checking that its spec expectations still describe the + # real tool. `spec_tests::` + `--ignored` selects exactly the gates — + # they are the only ignored tests in the module — and is what + # `scripts/update-spec-expected.sh` runs locally. + - name: Run strict host-tool parity gates (bash, sed, grep) + run: cargo test -p bashkit --test integration --features http_client,ssh -- spec_tests:: --ignored - name: Run competitor regression corpus run: cargo test -p bashkit --test integration --features http_client,jq -- competitor_regression_tests diff --git a/crates/bashkit/src/builtins/grep.rs b/crates/bashkit/src/builtins/grep.rs index 00e3889b3..30f47c350 100644 --- a/crates/bashkit/src/builtins/grep.rs +++ b/crates/bashkit/src/builtins/grep.rs @@ -495,13 +495,12 @@ fn path_has_excluded_dir( }) } -fn process_content(content: Vec, binary_as_text: bool) -> String { - if binary_as_text { - let filtered: Vec = content.into_iter().filter(|&b| b != 0).collect(); - String::from_utf8_lossy(&filtered).into_owned() - } else { - String::from_utf8_lossy(&content).into_owned() - } +/// `-a` means "treat this binary file as text", not "delete its NUL bytes": +/// GNU grep passes the matching line through byte for byte, so +/// `printf 'foo\0bar\n' | grep -a foo` prints `foo\0bar`. Stripping the NUL +/// silently altered the data a caller got back. +fn process_content(content: Vec, _binary_as_text: bool) -> String { + String::from_utf8_lossy(&content).into_owned() } /// Convert a BRE (Basic Regular Expression) pattern to ERE for the regex crate. @@ -599,21 +598,18 @@ impl Builtin for Grep { let mut total_matches = 0usize; // Determine input sources - // Use "(standard input)" for -H flag, "(stdin)" for -l flag - let stdin_name = if opts.show_filename { - "(standard input)" - } else if opts.files_with_matches || opts.files_without_matches { - "(stdin)" - } else { - "" - }; + // GNU grep names stdin "(standard input)" everywhere it names it at + // all — under -H, -l and -L alike. + let stdin_name = + if opts.show_filename || opts.files_with_matches || opts.files_without_matches { + "(standard input)" + } else { + "" + }; let inputs: Vec<(String, String)> = if opts.files.is_empty() { // Read from stdin - let mut stdin_content = ctx.stdin.map(ToString::to_string).unwrap_or_default(); - if opts.binary_as_text { - // Filter null bytes for -a flag - stdin_content = stdin_content.replace('\0', ""); - } + // No NUL filtering here either — see `process_content`. + let stdin_content = ctx.stdin.map(ToString::to_string).unwrap_or_default(); vec![(stdin_name.to_string(), stdin_content)] } else if opts.recursive { // Try indexed search via SearchCapable if available. Skip it for -P: @@ -773,9 +769,7 @@ impl Builtin for Grep { // matcher as soon as grep's early-exit conditions are met. matcher.for_each_range(line, |_| { file_matched = true; - if !opts.files_without_matches { - any_match = true; - } + any_match = true; match_count += 1; total_matches += 1; @@ -807,9 +801,7 @@ impl Builtin for Grep { if should_match { file_matched = true; - if !opts.files_without_matches { - any_match = true; - } + any_match = true; match_count += 1; total_matches += 1; match_lines.push(line_num); @@ -850,21 +842,29 @@ impl Builtin for Grep { } else { filename.as_str() }; - output.push_str(&format!("Binary file {} matches\n", display_name)); + // A diagnostic, not data: GNU grep >= 3.5 writes + // `grep: FILE: binary file matches` to stderr, so a pipeline + // reading grep's output never sees it. + errors.push_str(&format!("grep: {display_name}: binary file matches\n")); continue 'file_loop; } // Filename terminator: \0 for -Z, \n otherwise let fname_term = if opts.null_filename { '\0' } else { '\n' }; + // Output record terminator: -z means NUL-terminated *records*, + // on the way out as well as on the way in. + let line_term = if opts.null_terminated { '\0' } else { '\n' }; // Filename separator in line output: \0 for -Z, : otherwise let fname_sep = if opts.null_filename { '\0' } else { ':' }; if opts.files_with_matches && file_matched { output.push_str(filename); output.push(fname_term); } else if opts.files_without_matches && !file_matched { + // The filename is printed, but the status still reports + // whether a *match* was found: GNU `grep -L foo matching.txt` + // prints nothing and exits 0, and `grep -L foo other.txt` + // prints the name and exits 1. output.push_str(filename); output.push(fname_term); - // -L means at least one file printed => success - any_match = true; } else if opts.files_without_matches { // -L mode but file matched: skip output for this file } else if opts.count_only { @@ -896,7 +896,7 @@ impl Builtin for Grep { output.push_str(&format!("{}:", line_num + 1)); } output.push_str(&line[start..end]); - output.push('\n'); + output.push(line_term); o_matches += 1; true }); @@ -949,7 +949,7 @@ impl Builtin for Grep { output.push_str(&format!("{}{}", line_idx + 1, separator)); } output.push_str(lines[line_idx]); - output.push('\n'); + output.push(line_term); } } else { // Normal mode: output matching lines @@ -970,7 +970,7 @@ impl Builtin for Grep { output.push_str(&format!("{}:", line_idx + 1)); } output.push_str(lines[line_idx]); - output.push('\n'); + output.push(line_term); } } } @@ -1413,7 +1413,7 @@ mod tests { async fn test_grep_files_with_matches_stdin() { let result = run_grep(&["-l", "foo"], Some("foo\nbar")).await.unwrap(); assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "(stdin)\n"); + assert_eq!(result.stdout, "(standard input)\n"); } #[test] @@ -2003,14 +2003,16 @@ mod tests { #[tokio::test] async fn test_grep_files_without_match_stdin() { let result = run_grep(&["-L", "xyz"], Some("foo\nbar")).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "(stdin)\n"); + // GNU: the name is printed, but no match was found, so the status is 1. + assert_eq!(result.exit_code, 1); + assert_eq!(result.stdout, "(standard input)\n"); } #[tokio::test] async fn test_grep_files_without_match_stdin_has_match() { let result = run_grep(&["-L", "foo"], Some("foo\nbar")).await.unwrap(); - assert_eq!(result.exit_code, 1); + // GNU: -L prints nothing here, but a match *was* found, so status 0. + assert_eq!(result.exit_code, 0); assert_eq!(result.stdout, ""); } @@ -2019,8 +2021,8 @@ mod tests { let result = run_grep(&["--files-without-match", "xyz"], Some("foo\nbar")) .await .unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "(stdin)\n"); + assert_eq!(result.exit_code, 1); + assert_eq!(result.stdout, "(standard input)\n"); } #[tokio::test] @@ -2268,14 +2270,14 @@ mod tests { async fn test_grep_null_filename_with_l() { let result = run_grep(&["-lZ", "foo"], Some("foo\nbar")).await.unwrap(); assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "(stdin)\0"); + assert_eq!(result.stdout, "(standard input)\0"); } #[tokio::test] async fn test_grep_null_filename_with_big_l() { let result = run_grep(&["-LZ", "xyz"], Some("foo\nbar")).await.unwrap(); - assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "(stdin)\0"); + assert_eq!(result.exit_code, 1); + assert_eq!(result.stdout, "(standard input)\0"); } #[tokio::test] @@ -2325,7 +2327,7 @@ mod tests { .await .unwrap(); assert_eq!(result.exit_code, 0); - assert_eq!(result.stdout, "(stdin)\0"); + assert_eq!(result.stdout, "(standard input)\0"); } // TM-INF-022: malformed-regex stderr must not leak `regex` crate Debug. diff --git a/crates/bashkit/tests/integration/grep_gnu_parity_tests.rs b/crates/bashkit/tests/integration/grep_gnu_parity_tests.rs new file mode 100644 index 000000000..af2cda693 --- /dev/null +++ b/crates/bashkit/tests/integration/grep_gnu_parity_tests.rs @@ -0,0 +1,112 @@ +//! Regression: six ways `grep` diverged from GNU grep 3.11, all found by +//! pointing the differential gate at the grep spec suite for the first time. +//! +//! The spec suite's expectations had been written from Bashkit's own output, +//! so they recorded the bugs rather than catching them. `spec_tests:: +//! grep_comparison_tests` now runs the same cases against the host tool, and +//! CI runs every gate rather than only the bash one. + +use bashkit::Bash; + +async fn run(script: &str) -> bashkit::ExecResult { + let mut bash = Bash::builder() + .mount_text("/m.txt", "foo\n") + .mount_text("/n.txt", "bar\n") + .build(); + bash.exec(script) + .await + .unwrap_or_else(|e| panic!("{script:?} failed: {e}")) +} + +/// GNU names stdin `(standard input)` wherever it names it — `-H`, `-l`, `-L` +/// alike. Bashkit used `(stdin)` for the list flags only, so the same stream +/// had two names depending on the flag. +#[tokio::test] +async fn stdin_is_named_standard_input_under_every_flag() { + assert_eq!( + run("printf 'foo\\n' | grep -l foo") + .await + .stdout + .to_string(), + "(standard input)\n" + ); + assert_eq!( + run("printf 'bar\\n' | grep -L foo") + .await + .stdout + .to_string(), + "(standard input)\n" + ); + assert_eq!( + run("printf 'foo\\n' | grep -H foo") + .await + .stdout + .to_string(), + "(standard input):foo\n" + ); +} + +/// `-z` means NUL-terminated *records*, on the way out as well as in. +#[tokio::test] +async fn null_data_terminates_output_records_with_nul() { + let r = run("printf 'foo\\0bar\\0' | grep -z foo").await; + assert_eq!(r.stdout.to_string(), "foo\0"); +} + +/// `-a` means "treat as text", not "delete the NUL bytes". Stripping them +/// silently altered the bytes the caller got back. +#[tokio::test] +async fn binary_as_text_preserves_nul_bytes() { + let r = run("printf 'foo\\0bar\\n' | grep -a foo").await; + assert_eq!(r.stdout.to_string(), "foo\0bar\n"); +} + +/// The binary-match notice is a diagnostic, so it belongs on stderr in GNU's +/// `grep: FILE: binary file matches` form. On stdout it became part of the +/// data a pipeline reads. +#[tokio::test] +async fn binary_match_notice_goes_to_stderr() { + let r = run("printf 'foo\\0bar\\n' | grep foo").await; + assert_eq!(r.stdout.to_string(), ""); + assert_eq!( + r.stderr.to_string(), + "grep: (standard input): binary file matches\n" + ); + assert_eq!(r.exit_code, 0); +} + +#[tokio::test] +async fn binary_match_notice_stays_out_of_a_pipeline() { + let r = run("printf 'foo\\0bar\\n' | grep foo 2>/dev/null | wc -l").await; + assert_eq!(r.stdout.to_string().trim(), "0"); +} + +/// The exit status reports whether a *match* was found, independent of what +/// `-L` chose to print. Bashkit had inverted it for `-L`. +#[tokio::test] +async fn exit_status_tracks_the_match_not_the_listing() { + // -L: matching file prints nothing and exits 0. + let matched = run("grep -L foo /m.txt").await; + assert_eq!(matched.stdout.to_string(), ""); + assert_eq!(matched.exit_code, 0); + + // -L: non-matching file prints the name and exits 1. + let unmatched = run("grep -L foo /n.txt").await; + assert_eq!(unmatched.stdout.to_string(), "/n.txt\n"); + assert_eq!(unmatched.exit_code, 1); + + // Mixed: one file matched, so 0, while -L still lists the other. + let mixed = run("grep -L foo /m.txt /n.txt").await; + assert_eq!(mixed.stdout.to_string(), "/n.txt\n"); + assert_eq!(mixed.exit_code, 0); + + // Nothing matched anywhere: both listed, status 1. + let none = run("grep -L zzz /m.txt /n.txt").await; + assert_eq!(none.stdout.to_string(), "/m.txt\n/n.txt\n"); + assert_eq!(none.exit_code, 1); + + // -l is unchanged by all this. + let listed = run("grep -l foo /m.txt /n.txt").await; + assert_eq!(listed.stdout.to_string(), "/m.txt\n"); + assert_eq!(listed.exit_code, 0); +} diff --git a/crates/bashkit/tests/integration/main.rs b/crates/bashkit/tests/integration/main.rs index aa4f16354..3898209e9 100644 --- a/crates/bashkit/tests/integration/main.rs +++ b/crates/bashkit/tests/integration/main.rs @@ -66,6 +66,7 @@ pub mod git_remote_security_tests; pub mod git_security_tests; pub mod glob_fuzz_scaffold_tests; pub mod glob_intermediate_component_tests; +pub mod grep_gnu_parity_tests; pub mod harness_example_tests; pub mod history_tests; pub mod host_call_execution_tests; diff --git a/crates/bashkit/tests/integration/spec_tests.rs b/crates/bashkit/tests/integration/spec_tests.rs index c75fbfc91..cd074505b 100644 --- a/crates/bashkit/tests/integration/spec_tests.rs +++ b/crates/bashkit/tests/integration/spec_tests.rs @@ -437,6 +437,21 @@ async fn sed_comparison_tests() { compare_against_real_bash("sed").await; } +/// The grep spec suite is a differential suite against GNU grep, for the same +/// reason sed is against GNU sed: every expectation is a claim about the real +/// tool, and nothing else checks it. +/// +/// It was added after the gate caught seven expectations that had captured +/// Bashkit's own bugs rather than grep's behaviour — the `(stdin)` label, `-z` +/// output terminators, `-a` deleting NUL bytes, the binary-match diagnostic +/// on stdout, and `-L`'s exit status. Divergences kept on purpose carry +/// `### bash_diff:` and are excluded, like the bash gate. +#[tokio::test] +#[ignore = "strict host-grep parity gate; run explicitly in CI or via just check-bash-compat"] +async fn grep_comparison_tests() { + compare_against_real_bash("grep").await; +} + async fn compare_against_real_bash(category: &str) { let dir = spec_cases_dir().join(category); let all_tests = load_spec_tests(&dir); diff --git a/crates/bashkit/tests/spec_cases/grep/grep.test.sh b/crates/bashkit/tests/spec_cases/grep/grep.test.sh index e25f4d31e..96708b395 100644 --- a/crates/bashkit/tests/spec_cases/grep/grep.test.sh +++ b/crates/bashkit/tests/spec_cases/grep/grep.test.sh @@ -104,10 +104,11 @@ world ### end ### grep_files_with_matches -# List matching files (shows (stdin) for stdin input) +# List matching files. GNU grep names stdin "(standard input)" under -l too, +# not "(stdin)". printf 'foo\nbar\n' | grep -l foo ### expect -(stdin) +(standard input) ### end ### grep_quiet @@ -201,10 +202,13 @@ echo $? ### end ### grep_null_data -# Null-terminated mode with -z -printf 'foo\0bar\0' | grep -z foo +# -z is null-terminated *records*, on output as well as input: GNU grep emits +# `foo\0`, not `foo\n`. Rendered through tr so the NUL is visible here. +# The trailing `echo` supplies the newline the spec format needs; grep -z +# itself terminates the record with the NUL, not a newline. +printf 'foo\0bar\0' | grep -z foo | tr '\0' '@'; echo ### expect -foo +foo@ ### end ### grep_only_matching_multiple @@ -335,6 +339,8 @@ foo ### end ### grep_color +### bash_diff: --color=always is accepted but not implemented; GNU grep wraps +### bash_diff: the match in SGR escapes. Tracked as a limitation, not a bug. # Color flag accepted (no-op, outputs plain text) printf 'foo\n' | grep --color=always foo ### expect @@ -349,17 +355,20 @@ foo123 ### end ### grep_binary_detect -# Binary file detection: content with null bytes triggers binary message -printf 'foo\0bar\n' | grep foo +# Binary file detection. The message is a diagnostic, so GNU grep >= 3.5 +# writes `grep: FILE: binary file matches` to stderr and leaves stdout empty; +# merged here so the spec asserts the text as well as the stream. +printf 'foo\0bar\n' | grep foo 2>&1 ### expect -Binary file (standard input) matches +grep: (standard input): binary file matches ### end ### grep_binary_with_a_flag -# -a flag treats binary as text, outputs match normally -printf 'foo\0bar\n' | grep -a foo +# -a treats binary as text; it does not delete the NUL bytes. GNU grep passes +# the line through byte for byte. Rendered through tr so the NUL is visible. +printf 'foo\0bar\n' | grep -a foo | tr '\0' '@' ### expect -foobar +foo@bar ### end ### grep_include_pattern @@ -416,10 +425,11 @@ printf '' | grep foo ### end ### grep_binary_files_text -# Treat binary as text with -a (filters null bytes) -printf 'foo\0bar\n' | grep -a foo +# Same as grep_binary_with_a_flag from the --binary-files=text angle: the NUL +# survives. +printf 'foo\0bar\n' | grep -a foo | tr '\0' '@' ### expect -foobar +foo@bar ### end ### grep_count_multiple_matches @@ -580,9 +590,11 @@ printf 'foo\n' > /tmp/grep_l_a.txt && printf 'bar\n' > /tmp/grep_l_b.txt && grep ### end ### grep_files_without_match_no_output -# -L does not print files that match +# -L does not print files that match. The status still reports whether a +# *match* was found, so a matching file means exit 0 even though -L prints +# nothing; a non-matching file prints the name and exits 1. printf 'foo\n' > /tmp/grep_l2_a.txt && grep -L foo /tmp/grep_l2_a.txt -### exit_code: 1 +### exit_code: 0 ### expect ### end diff --git a/knowledge/operations/testing.md b/knowledge/operations/testing.md index 53b713780..feff76be9 100644 --- a/knowledge/operations/testing.md +++ b/knowledge/operations/testing.md @@ -200,11 +200,29 @@ change must update the executable surface test and manifest in the same change. ## Comparison Testing -The `bash_comparison_tests` test is ignored by default for local `cargo test` -runs because it compares against the host shell environment. CI runs it -explicitly as a strict parity gate. Tests marked with `### bash_diff` are -excluded from comparison. Tests marked with `### skip` are excluded from both -spec tests and comparison. +Three gates compare a spec category against the real host tool: +`bash_comparison_tests`, `sed_comparison_tests` and `grep_comparison_tests`. +All are `#[ignore]`d for local `cargo test` runs because they need the host +tool; CI runs them together with `spec_tests:: --ignored`, which selects +exactly the gates because they are the only ignored tests in the module. +`scripts/update-spec-expected.sh` (`just check-bash-compat`) runs the same +set. Tests marked `### bash_diff` are excluded from comparison; `### skip` +excludes from both spec tests and comparison. + +**A spec category without a gate is unverified, not verified.** Its +expectations are only a record of what Bashkit printed when they were written. +Pointing the gate at `grep` for the first time turned up seven expectations +that had captured Bashkit's own bugs — the `(stdin)` label, `-z` output +terminators, `-a` deleting NUL bytes, the binary-match diagnostic on stdout, +`-L`'s inverted exit status, and `-s` reporting exit 1 where GNU grep exits 2. +Wiring only `bash_comparison_tests` into CI had the same effect on `sed`: the +gate added with the GNU-sed work existed but never ran there. + +Categories still without a gate: `awk`, `jq`, `yq`, `python`, `typescript`. +`awk` is not a straightforward candidate — the usual host `awk` is mawk, while +Bashkit targets gawk, so a gate would need `### bash_diff` on every gawk +extension (`**`, `gensub`, 3-argument `match`, `\u` escapes) and on `for (k in +arr)`, whose iteration order awk leaves unspecified. ### yq compatibility and fuzzing From b680d5c4a94e276ad2ccdd2672a25ab83db7aead Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Wed, 23 Sep 2026 11:11:33 +0000 Subject: [PATCH 2/2] fix(ci): quote the parity-gate command so ci.yml still parses `spec_tests::` followed by a space is a mapping indicator to the YAML parser, so the plain scalar run: cargo test ... -- spec_tests:: --ignored took the whole workflow file out of service: run 35851812160 failed immediately with the file path as its name and no job ever started, which means none of the Rust CI ran on the previous commit. The original line survived only because its filter ended in a letter (`spec_tests::bash_comparison_tests --ignored`), so the `::` was never followed by a space. Block scalar rather than quotes, matching the other multi-step `run:` blocks in the file. --- .github/workflows/ci.yml | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1598611c0..618a319b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -268,8 +268,12 @@ jobs: # real tool. `spec_tests::` + `--ignored` selects exactly the gates — # they are the only ignored tests in the module — and is what # `scripts/update-spec-expected.sh` runs locally. + # Block scalar, not a plain one: `spec_tests::` followed by a space is a + # mapping indicator to the YAML parser and silently takes the whole + # workflow file out of service. - name: Run strict host-tool parity gates (bash, sed, grep) - run: cargo test -p bashkit --test integration --features http_client,ssh -- spec_tests:: --ignored + run: | + cargo test -p bashkit --test integration --features http_client,ssh -- spec_tests:: --ignored - name: Run competitor regression corpus run: cargo test -p bashkit --test integration --features http_client,jq -- competitor_regression_tests