diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 30032e4aa..618a319b2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -263,8 +263,17 @@ 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. + # 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 - 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