From 25761c7bf3ea578c9b0ce80fdea933caa3134ee2 Mon Sep 17 00:00:00 2001 From: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com> Date: Fri, 11 Sep 2026 11:59:49 +1000 Subject: [PATCH 1/2] test(cli): Show panic when canonicalize fails mid-walk Co-authored-by: Alexander Kireev Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com> --- crates/typos-cli/tests/walk_canonicalize.rs | 82 +++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 crates/typos-cli/tests/walk_canonicalize.rs diff --git a/crates/typos-cli/tests/walk_canonicalize.rs b/crates/typos-cli/tests/walk_canonicalize.rs new file mode 100644 index 00000000..5580f27c --- /dev/null +++ b/crates/typos-cli/tests/walk_canonicalize.rs @@ -0,0 +1,82 @@ +//! Regression test for +//! +//! `walk_entry` canonicalizes each entry's path to look up its policy. When that fails, +//! `report_result` reports the error and then hands back `PathBuf::default()`, an empty +//! path that is never a key in `ConfigEngine`'s directory map, so the `policy()` call +//! right after it panics with `` `walk()` should be called first ``. +#![cfg(unix)] + +struct CollectingReporter { + errors: std::sync::Mutex>, +} + +impl CollectingReporter { + fn new() -> Self { + Self { + errors: std::sync::Mutex::new(Vec::new()), + } + } +} + +impl typos_cli::report::Report for CollectingReporter { + fn report(&self, msg: typos_cli::report::Message<'_>) -> Result<(), std::io::Error> { + if msg.is_error() { + self.errors.lock().unwrap().push(format!("{msg:?}")); + } + Ok(()) + } +} + +#[test] +fn walk_path_reports_file_removed_mid_walk() { + let temp = assert_fs::TempDir::new().unwrap(); + let vanishing = temp.path().join("vanishing.txt"); + std::fs::write(&vanishing, b"helllo world\n").unwrap(); + + let storage = typos_cli::policy::ConfigStorage::new(); + let mut engine = typos_cli::policy::ConfigEngine::new(&storage); + engine.set_isolated(true); + engine.set_overrides(typos_cli::config::Config::default()); + let cwd = temp.path().canonicalize().unwrap(); + engine.init_dir(&cwd).unwrap(); + + // Remove the file as the walker visits it. `ignore` still yields it as an `Ok` entry, + // but the `canonicalize()` in `walk_entry` then fails with `NotFound` -- the same + // outcome as a file that a concurrent process removes mid-scan. + let mut builder = ignore::WalkBuilder::new(temp.path()); + let target = vanishing.clone(); + let removed = std::sync::atomic::AtomicBool::new(false); + builder.filter_entry(move |entry| { + if entry.path() == target && !removed.swap(true, std::sync::atomic::Ordering::SeqCst) { + std::fs::remove_file(&target).unwrap(); + } + true + }); + let reporter = CollectingReporter::new(); + + let payload = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + typos_cli::file::walk_path( + builder.build(), + &typos_cli::file::Typos, + &engine, + &reporter, + false, + ) + })) + .expect_err("walk_path should panic on the failed policy lookup"); + let message = payload + .downcast_ref::() + .map(String::as_str) + .or_else(|| payload.downcast_ref::<&str>().copied()); + // `policy()` checks the path with a `debug_assert!` before reaching its `expect`. + let expected = if cfg!(debug_assertions) { + " is not absolute" + } else { + "`walk()` should be called first" + }; + assert_eq!(message, Some(expected)); + assert!( + !reporter.errors.lock().unwrap().is_empty(), + "expected the vanished file's canonicalize() failure to be reported" + ); +} From 160d79280571abaefdec19d48d108457d85b45fd Mon Sep 17 00:00:00 2001 From: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com> Date: Fri, 11 Sep 2026 12:00:46 +1000 Subject: [PATCH 2/2] fix(cli): Don't panic when canonicalize fails mid-walk `walk_entry` canonicalizes each entry's path to look up its policy, and routes a failure through `report_result`, which reports the error and then substitutes `T::default()` -- for a `PathBuf` that is the empty path. `ConfigEngine` never saw that path during the walk, so the `policy()` call immediately after aborts the process: a debug build trips the `debug_assert!` on `path.is_absolute()`, a release build reaches the `expect` below it. Skip the entry after reporting the error instead, matching how a walk error is already handled a few lines above. A file removed by another process mid-scan is enough to reach this, so an abort here takes out an otherwise healthy run. Fixes #1444 and Closes #1577 Co-authored-by: Alexander Kireev Signed-off-by: Amir Zarrinkafsh <3339418+nightah@users.noreply.github.com> --- crates/typos-cli/src/file.rs | 9 ++++- crates/typos-cli/tests/walk_canonicalize.rs | 39 ++++++++------------- 2 files changed, 23 insertions(+), 25 deletions(-) diff --git a/crates/typos-cli/src/file.rs b/crates/typos-cli/src/file.rs index fd57e2a2..421ddc90 100644 --- a/crates/typos-cli/src/file.rs +++ b/crates/typos-cli/src/file.rs @@ -960,7 +960,14 @@ fn walk_entry( (path, cwd) } else { let path = entry.path(); - let abs_path = report_result(path.canonicalize(), Some(path), reporter)?; + let abs_path = match path.canonicalize() { + Ok(abs_path) => abs_path, + Err(err) => { + report_error(err, Some(path), reporter)?; + // Avoid a failed `engine.policy` lookup + return Ok(()); + } + }; (path, abs_path) }; let policy = engine.policy(&lookup_path); diff --git a/crates/typos-cli/tests/walk_canonicalize.rs b/crates/typos-cli/tests/walk_canonicalize.rs index 5580f27c..214141a1 100644 --- a/crates/typos-cli/tests/walk_canonicalize.rs +++ b/crates/typos-cli/tests/walk_canonicalize.rs @@ -1,9 +1,9 @@ //! Regression test for //! -//! `walk_entry` canonicalizes each entry's path to look up its policy. When that fails, -//! `report_result` reports the error and then hands back `PathBuf::default()`, an empty +//! `walk_entry` canonicalizes each entry's path to look up its policy. When that failed, +//! `report_result` reported the error and then handed back `PathBuf::default()`, an empty //! path that is never a key in `ConfigEngine`'s directory map, so the `policy()` call -//! right after it panics with `` `walk()` should be called first ``. +//! right after it panicked with `` `walk()` should be called first ``. #![cfg(unix)] struct CollectingReporter { @@ -54,27 +54,18 @@ fn walk_path_reports_file_removed_mid_walk() { }); let reporter = CollectingReporter::new(); - let payload = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { - typos_cli::file::walk_path( - builder.build(), - &typos_cli::file::Typos, - &engine, - &reporter, - false, - ) - })) - .expect_err("walk_path should panic on the failed policy lookup"); - let message = payload - .downcast_ref::() - .map(String::as_str) - .or_else(|| payload.downcast_ref::<&str>().copied()); - // `policy()` checks the path with a `debug_assert!` before reaching its `expect`. - let expected = if cfg!(debug_assertions) { - " is not absolute" - } else { - "`walk()` should be called first" - }; - assert_eq!(message, Some(expected)); + let result = typos_cli::file::walk_path( + builder.build(), + &typos_cli::file::Typos, + &engine, + &reporter, + false, + ); + + assert!( + result.is_ok(), + "walk_path should not surface an ignore::Error: {result:?}" + ); assert!( !reporter.errors.lock().unwrap().is_empty(), "expected the vanished file's canonicalize() failure to be reported"