diff --git a/.bca-baseline.toml b/.bca-baseline.toml index 31ef76155..88151fcd7 100644 --- a/.bca-baseline.toml +++ b/.bca-baseline.toml @@ -98,14 +98,14 @@ value = 7.0 [[entry]] path = "big-code-analysis-cli/src/commands/check.rs" qualified = "emit_check_results" -start_line = 510 +start_line = 560 metric = "nargs" value = 8.0 [[entry]] path = "big-code-analysis-cli/src/commands/check.rs" qualified = "filter_by_baseline" -start_line = 321 +start_line = 371 metric = "nargs" value = 8.0 @@ -114,7 +114,7 @@ path = "big-code-analysis-cli/src/commands/check.rs" qualified = "run_check" start_line = 13 metric = "halstead.effort" -value = 69823.99851963262 +value = 70257.02522730803 [[entry]] path = "big-code-analysis-cli/src/commands/check/effective_config.rs" @@ -196,21 +196,21 @@ value = 6.0 [[entry]] path = "big-code-analysis-cli/src/dispatch.rs" qualified = "dispatch_exemptions" -start_line = 536 +start_line = 548 metric = "nexits" value = 5.0 [[entry]] path = "big-code-analysis-cli/src/dispatch.rs" qualified = "dispatch_metrics" -start_line = 191 +start_line = 203 metric = "nargs" value = 7.0 [[entry]] path = "big-code-analysis-cli/src/dispatch.rs" qualified = "dispatch_ops" -start_line = 252 +start_line = 264 metric = "nargs" value = 8.0 @@ -266,7 +266,7 @@ value = 7.0 [[entry]] path = "big-code-analysis-cli/src/lib.rs" qualified = "legacy_hint" -start_line = 519 +start_line = 529 metric = "halstead.effort" value = 59351.805921378844 @@ -982,7 +982,7 @@ path = "src/tools.rs" qualified = "read_file_with_eol" start_line = 140 metric = "nexits" -value = 6.0 +value = 7.0 [[entry]] path = "src/vcs/bus_factor.rs" diff --git a/CHANGELOG.md b/CHANGELOG.md index 4411d2e4e..889f79ac9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -204,6 +204,39 @@ for historical reference. ### Fixed +- `bca check` no longer exits `0` when input files could not be read + (#1060). The counter backing the "no input files matched" guard was + bumped before the read was attempted, so a tree whose every file was + unreadable (permission denied, a broken symlink, a container + volume-mount mismatch) reported `error processing : …` on + stderr and then exited `0` — the worst failure mode a CI gate has. + The counter now moves only for files that were actually read, and + read failures are tallied separately: any input file that failed to + read exits `1` (tool error, distinct from `2` = gate breach) with a + summary line, because a partially analysed gate is not a passing + gate. Like the pre-existing empty-input guard, the check runs before + the gate is evaluated and is not suppressed by `--no-fail`, which + suppresses threshold failures rather than broken input. `bca init` + scaffolds its baseline through the same walk, so it inherits the + guard and refuses to pin a baseline that would silently under-record + the debt in a file it could not read. Both guards' messages are now + prefixed `bca:` rather than `bca check:`, which misattributed an + `init` failure to a subcommand the user never ran. Other subcommands + are unchanged for now; extending the same contract past `check` is + tracked separately. + + `read_file_with_eol` is fixed on the same issue: its "≤ 3 bytes is + not worth parsing" shortcut returned `Ok(None)` from a bare `stat`, + and `stat` succeeds on a file the process cannot open — so a tiny + unreadable file was indistinguishable from an empty one and the + permission error the function documents never surfaced. `bca check` + on a 3-byte unreadable file therefore exited 0 with no diagnostic at + all, not even the per-file `error processing` line. The shortcut now + confirms readability by opening the file first; the open is skipped + for anything that is not a regular file, so the function still never + blocks on a FIFO. Behaviour is unchanged for readable files of any + size. + - `Ops::operators` and `Ops::operands` are now sorted in byte-lexicographic order, so `bca ops` produces identical bytes for identical input (#1091). Both vectors were collected from `HashMap` diff --git a/big-code-analysis-book/src/commands/check.md b/big-code-analysis-book/src/commands/check.md index ae2d718f1..7f3449802 100644 --- a/big-code-analysis-book/src/commands/check.md +++ b/big-code-analysis-book/src/commands/check.md @@ -18,11 +18,20 @@ fails the pipeline before the change lands. |------|---------| | `0` | All functions within thresholds (or `--no-fail` set). | | `2` | At least one threshold exceeded. | -| `1` | Tool error (bad arguments, unreadable config, unknown metric). | +| `1` | Tool error (bad arguments, unreadable config or input, unknown metric). | `1` is reserved so CI can distinguish a regression (`2`) from a tool misconfiguration (`1`). +A gate that could not read all of its input has no verdict to report, +so two input problems exit `1` rather than `0`: nothing matched +`--paths` / `--include` / `--exclude`, and any input file that failed +to read (permission denied, a broken symlink, a path that vanished +mid-walk). Each unreadable file is named on stderr as `error processing +: …` before the summary line. Both checks run before the gate is +evaluated and are not suppressed by `--no-fail`, which suppresses +threshold failures, not broken input. + ### Tiered exit codes (`--exit-codes=tiered`) {#tiered-exit-codes---exit-codestiered} `--exit-codes=tiered` (or `[check] exit_codes = "tiered"` in diff --git a/big-code-analysis-cli/src/commands/check.rs b/big-code-analysis-cli/src/commands/check.rs index fc3bb4f3a..ebdb9e635 100644 --- a/big-code-analysis-cli/src/commands/check.rs +++ b/big-code-analysis-cli/src/commands/check.rs @@ -68,15 +68,9 @@ pub(crate) fn run_check( // `format_remediation_block` needs the resolved `--paths` / // `--exclude` set to compose a copy-paste-safe refresh command. let globals_for_remediation = globals.clone(); - let (violations, files_dispatched) = run_check_walk(globals, &args, preproc, set); - - if files_dispatched.load(Ordering::Relaxed) == 0 { - // No files survived `--paths` expansion + `--include`/`--exclude` - // filtering. Treat this as a tool error (exit 1), not a clean - // pass (exit 0): a typo in `--paths` would otherwise silently - // green-light CI. - die("bca check: no input files matched; check --paths, --include, --exclude"); - } + let walk = run_check_walk(globals, &args, preproc, set); + enforce_usable_input(&walk); + let violations = walk.violations; // Drop offenders from `[check.exclude]` files (#378) before *any* // downstream consumer sees them — so `--write-baseline` never @@ -163,24 +157,80 @@ pub(crate) fn run_check( } } +/// What the check walk produced: the sorted violations plus the two +/// post-walk tallies `run_check` consults before it trusts the gate +/// verdict. Named fields rather than a tuple because the counters are +/// the same primitive type and mean opposite things — swapping them +/// would turn "nothing matched" into "everything was unreadable". +struct CheckWalk { + violations: Vec, + /// Files whose contents were read and handed to the pre-dispatch + /// filters. Zero means nothing survived `--paths` expansion plus + /// `--include` / `--exclude` filtering. + files_dispatched: usize, + /// Files the runner could not read at all (#1060). Non-zero means + /// the gate saw less than its input set. + read_failures: usize, +} + +/// Fail the run when the walk did not see a usable input set — any file +/// that could not be read at all, or no files whatsoever. Both are tool +/// errors (exit 1) rather than gate results (exit 2), because a gate +/// that analysed less than its input has no verdict to report, and both +/// fire before the gate is evaluated, so neither is suppressed by +/// `--no-fail` (which suppresses threshold failures, not broken input) +/// and neither lets `--write-baseline` record a partial run. Mirrors +/// `enforce_explicit_unrecognized` on the analyze side. +/// +/// Both messages are prefixed `bca:` rather than `bca check:`: `bca +/// init` scaffolds its baseline through `run_check` +/// (`commands::init::scaffold_baseline`), so naming the subcommand here +/// misattributes the failure to a command the user never ran. +fn enforce_usable_input(walk: &CheckWalk) { + if walk.read_failures > 0 { + // The consumer has already printed one `error processing : + // …` line per failure. Checked before the empty-input guard so + // an all-unreadable run names its real cause instead of blaming + // the path filters (#1060). + let noun = if walk.read_failures == 1 { + "file" + } else { + "files" + }; + die(format_args!( + "bca: {} input {noun} could not be read (see the errors \ + above); refusing to trust a partially analysed input set", + walk.read_failures + )); + } + + if walk.files_dispatched == 0 { + // No files survived `--paths` expansion + `--include`/`--exclude` + // filtering. Treat this as a tool error (exit 1), not a clean + // pass (exit 0): a typo in `--paths` would otherwise silently + // green-light CI. + die("bca: no input files matched; check --paths, --include, --exclude"); + } +} + /// Run the parallel walker with a check-flavoured `Config`, collect /// every emitted `Violation`, and sort them by `(path, start_line, /// metric)` so CI diff tooling sees identical output across runs over -/// the same tree. Returns the sorted vector plus the -/// `files_dispatched` counter so the caller can detect the "no inputs -/// matched" case. -pub(crate) fn run_check_walk( +/// the same tree. +fn run_check_walk( globals: GlobalOpts, args: &CheckArgs, preproc: Option>, set: Arc, -) -> (Vec, Arc) { +) -> CheckWalk { let (tx, rx) = std::sync::mpsc::channel(); let files_dispatched = Arc::new(AtomicUsize::new(0)); + let read_failures = Arc::new(AtomicUsize::new(0)); let cfg = Config { threshold_set: Some(set), check_tx: Some(Mutex::new(tx)), files_dispatched: Some(Arc::clone(&files_dispatched)), + read_failures: Some(Arc::clone(&read_failures)), suppression_policy: SuppressionPolicy::from_no_suppress(args.no_suppress), report_suppressed: args.report_suppressed, // Compute body hashes during the walk only when fuzzy matching @@ -204,7 +254,11 @@ pub(crate) fn run_check_walk( .then(a.metric.cmp(b.metric)) }); - (violations, files_dispatched) + CheckWalk { + violations, + files_dispatched: files_dispatched.load(Ordering::Relaxed), + read_failures: read_failures.load(Ordering::Relaxed), + } } /// Serialize and write the collected violations as a baseline TOML diff --git a/big-code-analysis-cli/src/dispatch.rs b/big-code-analysis-cli/src/dispatch.rs index 58efb1ec6..fa7d9816e 100644 --- a/big-code-analysis-cli/src/dispatch.rs +++ b/big-code-analysis-cli/src/dispatch.rs @@ -1,7 +1,7 @@ //! Per-file dispatch for the `bca` walker. //! //! `act_on_file` is the entry point: it runs the shared pre-dispatch -//! filters (file-count bump, empty-file skip, generated-code skip, +//! filters (read + file-count bump, empty-file skip, generated-code skip, //! language resolution) via `validate_and_resolve_file`, then forwards //! to the per-action `dispatch_*` helper that implements one `Action` //! variant. The helpers are intentionally one-screen each so a reader @@ -93,7 +93,8 @@ pub(crate) fn act_on_file(path: PathBuf, cfg: &Config) -> std::io::Result<()> { } /// Apply the three pre-dispatch filters every CLI subcommand shares: -/// bump the `files_dispatched` counter, skip empty files, skip +/// read the file (bumping `files_dispatched` on success and +/// `read_failures` on failure), skip empty files, skip /// generated files (unless we're producing preproc data — that /// pipeline genuinely needs every C/C++ file walked), and resolve /// the source language. Returns `Ok(None)` when the file should be @@ -103,16 +104,27 @@ fn validate_and_resolve_file( path: PathBuf, cfg: &Config, ) -> std::io::Result, LANG)>> { + // Read first, count second: a file we could not open was never + // analysed, and counting it let the zero-files-matched guard in + // `run_check` pass a gate run that read nothing at all (#1060). The + // failure is tallied separately so the caller can distinguish + // "nothing matched" from "everything was unreadable". + let source = read_file_with_eol(&path).inspect_err(|_| { + if let Some(counter) = &cfg.read_failures { + counter.fetch_add(1, Ordering::Relaxed); + } + })?; + if let Some(counter) = &cfg.files_dispatched { - // Count every dispatched file, including those skipped below for - // empty content / unrecognized language. The user pointed at - // these files and the runner walked them — they count as "the - // input was non-empty" for the zero-files-matched check in - // `run_check`. + // Count every file we managed to read, including those skipped + // below for empty content / unrecognized language. The user + // pointed at these files and the runner walked them — they count + // as "the input was non-empty" for the zero-files-matched check + // in `run_check`. counter.fetch_add(1, Ordering::Relaxed); } - let Some(source) = read_file_with_eol(&path)? else { + let Some(source) = source else { if cfg.warning { warn(format_args!("skipping empty file: {}", path.display())); } @@ -618,6 +630,7 @@ mod tests { use super::*; use big_code_analysis::SuppressionPolicy; use std::sync::Mutex; + use std::sync::atomic::AtomicUsize; // Minimal `Config` for exercising `dispatch_preproc` in isolation. // Only `preproc_lock` and `warning` are load-bearing here; every @@ -640,6 +653,7 @@ mod tests { check_tx: None, exemptions_tx: None, files_dispatched: None, + read_failures: None, explicit_seeds: Arc::new(std::collections::HashSet::new()), explicit_unrecognized: None, output_produced: None, @@ -658,6 +672,102 @@ mod tests { } } + // The two post-walk counters `run_check` consults, owned together + // with the `Config` that feeds them. Named fields rather than a pair + // of `Arc` arguments: transposing them at a call site + // would invert the very distinction these tests exist to pin. + struct Counters { + dispatched: Arc, + failures: Arc, + } + + impl Counters { + fn new() -> Self { + Self { + dispatched: Arc::new(AtomicUsize::new(0)), + failures: Arc::new(AtomicUsize::new(0)), + } + } + + fn config(&self) -> Config { + Config { + files_dispatched: Some(Arc::clone(&self.dispatched)), + read_failures: Some(Arc::clone(&self.failures)), + ..preproc_test_config(None) + } + } + + fn dispatched(&self) -> usize { + self.dispatched.load(Ordering::Relaxed) + } + + fn failures(&self) -> usize { + self.failures.load(Ordering::Relaxed) + } + } + + // Regression test for issue #1060: a file the runner cannot read + // was never analysed, so it must not bump `files_dispatched` — that + // counter is what turns "no input files matched" into a tool error + // in `run_check`, and counting an unreadable file made `bca check` + // exit 0 on a run that analysed nothing. A missing path stands in + // for the reported permission-denied case: both surface as an + // `Err` from `read_file_with_eol`, and this form also runs where + // POSIX modes do not (Windows, and as root). + #[test] + fn unreadable_file_counts_as_read_failure_not_dispatched() { + let dir = tempfile::tempdir().expect("tempdir"); + let counters = Counters::new(); + let cfg = counters.config(); + + let err = validate_and_resolve_file(dir.path().join("missing.py"), &cfg) + .expect_err("a nonexistent path must surface the read error"); + + assert_eq!(err.kind(), std::io::ErrorKind::NotFound); + assert_eq!(counters.dispatched(), 0); + assert_eq!(counters.failures(), 1); + } + + // The counterpart to the test above: a file that reads cleanly is + // dispatched and leaves the failure tally untouched. + #[test] + fn readable_file_counts_as_dispatched() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("ok.py"); + std::fs::write(&path, "def f():\n return 1\n").expect("write fixture"); + let counters = Counters::new(); + let cfg = counters.config(); + + let (_, source, language) = validate_and_resolve_file(path, &cfg) + .expect("readable file") + .expect("a Python file must resolve a language"); + + assert_eq!(language, LANG::Python); + assert_eq!(source, b"def f():\n return 1\n"); + assert_eq!(counters.dispatched(), 1); + assert_eq!(counters.failures(), 0); + } + + // The #1060 reorder moved the `files_dispatched` bump below the + // read, which must not narrow it to files that produce metrics: an + // empty file is skipped for analysis but still counts as "the user + // pointed at something", the semantics `run_check`'s zero-files + // guard is written against. + #[test] + fn empty_file_still_counts_as_dispatched() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("empty.py"); + std::fs::write(&path, "").expect("write fixture"); + let counters = Counters::new(); + let cfg = counters.config(); + + let resolved = validate_and_resolve_file(path, &cfg).expect("readable file"); + + assert!(resolved.is_none(), "an empty file is skipped for analysis"); + assert_eq!(counters.dispatched(), 1); + assert_eq!(counters.failures(), 0); + } + // Regression test for issue #425: a poisoned `preproc_lock` must // degrade like the sibling worker dispatchers (warn + `Ok(())`) // rather than panic and cascade across the pool. Verified by diff --git a/big-code-analysis-cli/src/lib.rs b/big-code-analysis-cli/src/lib.rs index 6e74156aa..77001d1e2 100644 --- a/big-code-analysis-cli/src/lib.rs +++ b/big-code-analysis-cli/src/lib.rs @@ -181,6 +181,15 @@ struct Config { /// no violations) from "no files matched" (counter == 0), so a /// typo in `--paths` does not silently pass CI. files_dispatched: Option>, + /// Counts input files whose contents could not be read at all — + /// permission denied, a broken symlink, a path that vanished + /// mid-walk. A failed read deliberately leaves `files_dispatched` + /// untouched, so this counter is the only record that the runner + /// saw the file. `Action::Check` reads it after the walk and exits + /// 1 rather than reporting a gate verdict derived from a partially + /// analysed input set (#1060). `None` for flows that do not enforce + /// the rule. + read_failures: Option>, /// Seeds the user named *explicitly as files* on the command line /// (or via `--paths-from` / a manifest `paths` key), in the emitted /// path form. A file in this set whose language is unrecognized is a @@ -299,6 +308,7 @@ impl Config { check_tx: None, exemptions_tx: None, files_dispatched: None, + read_failures: None, explicit_seeds: Arc::new(std::collections::HashSet::new()), explicit_unrecognized: None, output_produced: None, diff --git a/big-code-analysis-cli/tests/check_thresholds.rs b/big-code-analysis-cli/tests/check_thresholds.rs index 8f0a18da1..4ebaae9a3 100644 --- a/big-code-analysis-cli/tests/check_thresholds.rs +++ b/big-code-analysis-cli/tests/check_thresholds.rs @@ -327,7 +327,7 @@ fn check_with_no_matching_files_exits_one() { // `error:` prefix (matching clap's own usage errors), never the // old capitalized `Error:`. .stderr(predicate::str::contains( - "error: bca check: no input files matched", + "error: bca: no input files matched", )) .stderr(predicate::str::contains("Error:").not()); } @@ -1719,3 +1719,136 @@ fn no_cyclomatic_try_alias_warns_but_honors() { "`--no-cyclomatic-try` is deprecated; use `--cyclomatic-count-try=false`", )); } + +/// Write `body` to `name` and strip every permission bit so reading it +/// fails with `EACCES`. Returns `None` when the caller is privileged +/// enough to read it anyway (root ignores mode bits), because then the +/// scenario under test cannot be staged at all. +#[cfg(unix)] +fn unreadable_fixture(dir: &TempDir, name: &str, body: &str) -> Option { + use std::os::unix::fs::PermissionsExt; + + let path = dir.path().join(name); + fs::write(&path, body).expect("write fixture"); + fs::set_permissions(&path, fs::Permissions::from_mode(0o000)).expect("chmod 000"); + // Probe the actual capability rather than the uid: `fs::read` + // succeeding here means mode bits do not deny this process. + if fs::read(&path).is_ok() { + return None; + } + Some(path.to_str().expect("utf8 fixture path").to_string()) +} + +/// #1060: every input file failing to read must exit 1, not 0. The +/// per-file read error already reached stderr, but the counter that +/// backs the "no input files matched" guard was bumped before the read +/// was attempted, so the gate reported success on a run that analysed +/// nothing. +#[cfg(unix)] +#[test] +fn check_exits_one_when_every_input_file_is_unreadable() { + let dir = TempDir::new().unwrap(); + let Some(path) = unreadable_fixture(&dir, "branchy.rs", BRANCHY_RUST) else { + eprintln!("skipping: this process can read a mode-000 file"); + return; + }; + + cli(dir.path()) + .args([ + "check", + "--no-config", + "--paths", + &path, + "--threshold", + "cyclomatic=1", + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("Permission denied")) + .stderr(predicate::str::contains("1 input file could not be read")); +} + +/// A partially-read gate is not a passing gate (#1060): the readable +/// half being clean does not license exit 0 when the other half was +/// never analysed. Two unreadable files, so this also pins the plural +/// wording of the summary line. +#[cfg(unix)] +#[test] +fn check_exits_one_when_some_input_files_are_unreadable() { + let dir = TempDir::new().unwrap(); + write_fixture(&dir, "clean.rs", TRIVIAL_RUST); + if unreadable_fixture(&dir, "locked.rs", BRANCHY_RUST).is_none() + || unreadable_fixture(&dir, "locked_too.rs", BRANCHY_RUST).is_none() + { + eprintln!("skipping: this process can read a mode-000 file"); + return; + } + + cli(dir.path()) + .args([ + "check", + "--no-config", + "--paths", + dir.path().to_str().unwrap(), + "--threshold", + "cyclomatic=100", + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("2 input files could not be read")); +} + +/// #1060, the size class the other tests miss: `read_file_with_eol` +/// short-circuits files of three bytes or fewer, and `stat` succeeds +/// without read permission — so a tiny unreadable file used to be +/// counted as an empty one and the run exited 0 with no diagnostic at +/// all, not even the per-file `error processing` line. +#[cfg(unix)] +#[test] +fn check_exits_one_when_a_tiny_input_file_is_unreadable() { + let dir = TempDir::new().unwrap(); + let Some(path) = unreadable_fixture(&dir, "tiny.c", "a;\n") else { + eprintln!("skipping: this process can read a mode-000 file"); + return; + }; + + cli(dir.path()) + .args([ + "check", + "--no-config", + "--paths", + &path, + "--threshold", + "cyclomatic=1", + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("Permission denied")) + .stderr(predicate::str::contains("1 input file could not be read")); +} + +/// `--no-fail` suppresses threshold failures, not broken input: an +/// unreadable file is a tool error and still exits 1 (#1060). +#[cfg(unix)] +#[test] +fn check_no_fail_does_not_mask_an_unreadable_input() { + let dir = TempDir::new().unwrap(); + let Some(path) = unreadable_fixture(&dir, "branchy.rs", BRANCHY_RUST) else { + eprintln!("skipping: this process can read a mode-000 file"); + return; + }; + + cli(dir.path()) + .args([ + "check", + "--no-config", + "--paths", + &path, + "--threshold", + "cyclomatic=1", + "--no-fail", + ]) + .assert() + .code(1) + .stderr(predicate::str::contains("1 input file could not be read")); +} diff --git a/big-code-analysis-cli/tests/init.rs b/big-code-analysis-cli/tests/init.rs index ef7ae2803..d8d08266c 100644 --- a/big-code-analysis-cli/tests/init.rs +++ b/big-code-analysis-cli/tests/init.rs @@ -194,3 +194,44 @@ fn init_no_baseline_writes_empty_placeholder() { "--no-baseline must produce empty file with no entries: {baseline}" ); } + +/// #1060: `scaffold_baseline` produces its baseline by driving +/// `run_check`, so `init` inherits the guard that refuses to trust a +/// partially-read input set. An unreadable file therefore aborts the +/// run rather than pinning a baseline that silently under-records the +/// debt in the file it could not read. Unix-only: it relies on POSIX +/// permission bits. +#[cfg(unix)] +#[test] +fn init_refuses_to_baseline_a_partially_readable_tree() { + use std::os::unix::fs::PermissionsExt; + + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("lib.rs"), TRIVIAL_RUST).unwrap(); + let locked = dir.path().join("locked.rs"); + fs::write(&locked, TRIVIAL_RUST).unwrap(); + fs::set_permissions(&locked, fs::Permissions::from_mode(0o000)).unwrap(); + // Probe the real capability rather than the uid: root ignores mode + // bits, and then the scenario cannot be staged at all. + if fs::read(&locked).is_ok() { + eprintln!("skipping: this process can read a mode-000 file"); + return; + } + + cli() + .current_dir(dir.path()) + .args(["init"]) + .assert() + .code(1) + // The diagnostic names the tool, not `check` — the user ran + // `init` and never invoked a subcommand called `check`. + .stderr(predicate::str::contains( + "error: bca: 1 input file could not be read", + )) + .stderr(predicate::str::contains("bca check:").not()); + + assert!( + !dir.path().join(".bca-baseline.toml").exists(), + "a partially-read tree must not produce a baseline" + ); +} diff --git a/src/tools.rs b/src/tools.rs index 561ab7261..6c4e801a2 100644 --- a/src/tools.rs +++ b/src/tools.rs @@ -114,8 +114,8 @@ fn probe_decodable_prefix(start: &[u8], file_size: usize, probe_len: usize) -> O } /// Reads a file, normalising all CR-only and CRLF line endings to LF, and ensures -/// the buffer ends with exactly one `\n`. Returns `None` for files ≤ 3 bytes or -/// files that appear to be non-UTF-8. +/// the buffer ends with exactly one `\n`. Returns `None` for readable files +/// ≤ 3 bytes or files that appear to be non-UTF-8. /// /// # Errors /// @@ -125,7 +125,8 @@ fn probe_decodable_prefix(start: &[u8], file_size: usize, probe_len: usize) -> O /// during the probe (`UnexpectedEof`) yields `Ok(None)`; any other /// `read_exact` error kind propagates as `Err`. A non-UTF-8 head, a /// too-small file, or a UTF-16 BE/LE BOM is reported via `Ok(None)`, -/// not an error. +/// not an error — but "too small" is decided only for a file that can +/// be opened, so an unreadable one errors instead (#1060). /// /// # Examples /// @@ -138,9 +139,18 @@ fn probe_decodable_prefix(start: &[u8], file_size: usize, probe_len: usize) -> O /// read_file_with_eol(&path).unwrap(); /// ``` pub fn read_file_with_eol(path: &Path) -> std::io::Result>> { - let file_size = fs::metadata(path).map_or(1024 * 1024, |m| m.len() as usize); + let meta = fs::metadata(path); + let file_size = meta.as_ref().map_or(1024 * 1024, |m| m.len() as usize); if file_size <= 3 { - // this file is very likely almost empty... so nothing to do on it + // Nothing worth parsing this small — but `stat` alone must not + // decide it: `stat` succeeds on a file the process cannot open, + // so an unreadable tiny file read as empty and `bca check` + // exited 0 on a tree it never read (#1060). `meta` is + // necessarily `Ok` here; the open is a discarded readability + // probe, skipped for non-regular files because a FIFO blocks. + if meta.is_ok_and(|m| m.is_file()) { + File::open(path)?; + } return Ok(None); } diff --git a/src/tools_tests.rs b/src/tools_tests.rs index 771aa5c0c..431e3101f 100644 --- a/src/tools_tests.rs +++ b/src/tools_tests.rs @@ -898,3 +898,43 @@ fn read_eol_propagates_non_eof_io_error() { let err = read_file_with_eol(&dir).expect_err("reading a directory must error"); assert_ne!(err.kind(), std::io::ErrorKind::UnexpectedEof); } + +#[test] +fn read_eol_reports_tiny_readable_file_as_none() { + // The "≤ 3 bytes is not worth parsing" shortcut still holds for a file + // the process can read: `Ok(None)`, no error. + let path = write_tmp("bca_read_eol_tiny_readable", b"a;\n"); + assert_eq!(read_file_with_eol(&path).unwrap(), None); +} + +/// #1060: a tiny file the process cannot read must surface the permission +/// error, not the `Ok(None)` that means "empty, nothing to do". `stat` +/// succeeds without read permission, so the size-only shortcut made an +/// unreadable 3-byte file indistinguishable from an empty one — and +/// `bca check` counted it as analysed and exited 0 on a tree it never +/// read. Unix-only: it relies on POSIX permission bits. +#[cfg(unix)] +#[test] +fn read_eol_surfaces_permission_error_for_tiny_file() { + use std::os::unix::fs::PermissionsExt; + + // A private `TempDir` rather than the shared `write_tmp` path: an + // unreadable file left behind at a fixed name makes the *next* run of + // this test fail in setup, and `TempDir` cleans up even when an + // assertion panics. Unlinking a mode-000 file needs write permission + // on the directory, not the file, so no chmod-back is required. + let dir = tempfile::tempdir().unwrap(); + // 3 bytes, so the file lands inside the too-small shortcut. + let path = dir.path().join("tiny.c"); + write_file(&path, b"a;\n").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o000)).unwrap(); + // Probe the real capability rather than the uid: root ignores mode + // bits, and then the scenario cannot be staged at all. + if std::fs::read(&path).is_ok() { + eprintln!("skipping: this process can read a mode-000 file"); + return; + } + + let err = read_file_with_eol(&path).expect_err("an unreadable file must error"); + assert_eq!(err.kind(), std::io::ErrorKind::PermissionDenied); +}