From aa94bef98c643aa952b39f59156b35e1ff0c4837 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Mon, 28 Sep 2026 18:03:04 +0100 Subject: [PATCH 1/8] Don't report other files' todo entries as stale in a scoped check `check` with paths, and `check-contents`, checked only a subset of files but compared every recorded violation against what that subset found. An entry for a file outside the subset looked as if its file had been deleted, so it was reported as stale and the run exited 1. Checking a single file, as editor integrations do on save, failed whenever another file had a recorded violation. `check_all` now tells `CheckAllBuilder` whether paths were supplied. In a run given paths, an entry whose file wasn't checked is skipped, and an entry whose file was checked is still stale if the run no longer finds it. A full run still treats a recorded file it didn't check as deleted. This can't be read off the checked set. Paths that match no included file leave it empty, and that run must still count as one given paths, not as a full run that reports every entry as stale. A run given a directory, `pks check .` included, no longer reports entries for files deleted under it. Keeping that would mean knowing which arguments were directories, which argument expansion decides and then throws away. It's left out here, since a full `check` still reports those entries. packwerk's open fix for the same bug, Shopify/packwerk#415, also skips the deleted-file check whenever files are supplied. Seven new tests cover a relative and an absolute file argument, `check-contents`, a directory, paths that match no included file, and in-scope stale entries still being reported. All seven fail on main. One more pins that a full `check` still reports an entry for a deleted file. Fixes #48. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 ++ src/packs/checker.rs | 13 ++- tests/check_test.rs | 217 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 238 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 231523cc..56dce572 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,16 @@ identical entries. JSON also counted both in `violation_count`, so it could exceed the number of distinct violations. Each violation now appears once, and `violation_count` counts distinct violations. Text output is unchanged. +#### `check ` and `check-contents` no longer flag other files' entries as stale + +When given paths, `pks check` treated a `package_todo.yml` entry for any file +it had not checked as if that file had been deleted, and reported it as stale. +So checking one file, as editor integrations do on save, failed whenever any +other file had an entry. `check-contents` behaved the same way. A run given +paths now only considers entries for the files it checked. A full `pks check` +still reports entries for deleted files as stale; a run given a directory, such +as `pks check packs/foo` or `pks check .`, no longer does. + ## 0.5.0 ### Breaking Changes diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 11d194a2..13eda8f3 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -100,6 +100,9 @@ struct CheckAllBuilder<'a> { #[derive(Debug)] struct FoundViolations { absolute_paths: HashSet, + // Set when the caller named paths to check, even if none of them is an + // included file, so it cannot be derived from `absolute_paths`. + scoped: bool, violations: HashSet, } @@ -188,6 +191,7 @@ impl<'a> CheckAllBuilder<'a> { }) .collect::>>()?; + let scoped = self.found_violations.scoped; let stale_violations = recorded_violations .par_iter() .filter(|v_identifier| { @@ -195,6 +199,7 @@ impl<'a> CheckAllBuilder<'a> { &relative_files, &found_violation_identifiers, v_identifier, + scoped, ) }) .collect::>(); @@ -205,11 +210,14 @@ impl<'a> CheckAllBuilder<'a> { relative_files: &HashSet<&str>, found_violation_identifiers: &HashSet<&ViolationIdentifier>, todo_violation_identifier: &ViolationIdentifier, + scoped: bool, ) -> bool { - let violation_path_exists = + let file_was_checked = relative_files.contains(todo_violation_identifier.file.as_str()); - if violation_path_exists { + if file_was_checked { !found_violation_identifiers.contains(todo_violation_identifier) + } else if scoped { + false } else { true // The todo violation references a file that no longer exists } @@ -250,6 +258,7 @@ pub(crate) fn check_all( get_all_violations(configuration, &absolute_paths, &checkers)?; let found_violations = FoundViolations { absolute_paths, + scoped: !files.is_empty(), violations, }; debug!("Building check-all result (diffing against package_todo.yml)"); diff --git a/tests/check_test.rs b/tests/check_test.rs index 1b09c14b..7b5262ea 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -317,6 +317,223 @@ fn test_check_with_stale_violations_when_file_no_longer_exists( Ok(()) } +#[test] +fn test_check_with_recorded_strict_mode_violation_when_file_is_deleted( +) -> Result<(), Box> { + let fixture = common::Fixture::new("uses_strict_mode"); + fs::remove_file(fixture.path("packs/foo/app/services/foo.rb"))?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "There were stale violations found, please run `packs update`", + )); + + Ok(()) +} + +#[test] +fn test_check_with_single_file_ignores_recorded_violations_in_other_files( +) -> Result<(), Box> { + // In `uses_strict_mode`, every recorded violation is in foo.rb, so a run + // that checks only bar.rb has no recorded violations in scope. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/bar/app/services/bar.rb") + .assert() + .code(0) + .stdout( + predicate::str::contains("There were stale violations found").not(), + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_contents_ignores_recorded_violations_in_other_files( +) -> Result<(), Box> { + let project_root = "tests/fixtures/uses_strict_mode"; + let relative_path = "packs/bar/app/services/bar.rb"; + let bar_rb_contents = + fs::read_to_string(format!("{}/{}", project_root, relative_path))?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(project_root) + .arg("check-contents") + .arg(relative_path) + .write_stdin(bar_rb_contents) + .assert() + .code(0) + .stdout( + predicate::str::contains("There were stale violations found").not(), + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_directory_ignores_recorded_violations_in_other_files( +) -> Result<(), Box> { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/bar") + .assert() + .code(0) + .stdout( + predicate::str::contains("There were stale violations found").not(), + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_only_unincluded_files_ignores_recorded_violations( +) -> Result<(), Box> { + // package.yml is not in the include globs, so this run checks no files at + // all. It is still a run given paths, not a full run. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/foo/package.yml") + .assert() + .code(0) + .stdout( + predicate::str::contains("There were stale violations found").not(), + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_single_file_reports_stale_violations_in_foo_rb( +) -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_stale_violations") + .arg("check") + .arg("-o") + .arg("json") + .arg("packs/foo/app/services/foo.rb") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["stale_todo_count"], 1); + let stale_todos = json_output["stale_todos"].as_array().unwrap(); + assert_eq!(stale_todos.len(), 1); + assert_eq!(stale_todos[0]["file"], "packs/foo/app/services/foo.rb"); + assert_eq!(stale_todos[0]["constant_name"], "::Bar"); + assert_eq!(stale_todos[0]["violation_type"], "dependency"); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_single_absolute_file_reports_stale_violations_in_foo_rb( +) -> Result<(), Box> { + // Editor integrations pass the absolute path of the file being saved. + let foo_rb = fs::canonicalize( + "tests/fixtures/contains_stale_violations/packs/foo/app/services/foo.rb", + )?; + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_stale_violations") + .arg("check") + .arg("-o") + .arg("json") + .arg(foo_rb) + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["stale_todo_count"], 1); + let stale_todos = json_output["stale_todos"].as_array().unwrap(); + assert_eq!(stale_todos.len(), 1); + assert_eq!(stale_todos[0]["file"], "packs/foo/app/services/foo.rb"); + assert_eq!(stale_todos[0]["constant_name"], "::Bar"); + assert_eq!(stale_todos[0]["violation_type"], "dependency"); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_single_file_reports_stale_violations_in_bar_rb( +) -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_stale_violations") + .arg("check") + .arg("-o") + .arg("json") + .arg("packs/bar/app/services/bar.rb") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["stale_todo_count"], 2); + let mut stale_todos: Vec<(&str, &str, &str)> = json_output["stale_todos"] + .as_array() + .unwrap() + .iter() + .map(|t| { + ( + t["file"].as_str().unwrap(), + t["constant_name"].as_str().unwrap(), + t["violation_type"].as_str().unwrap(), + ) + }) + .collect(); + stale_todos.sort(); + assert_eq!( + stale_todos, + vec![ + ("packs/bar/app/services/bar.rb", "::Foo", "dependency"), + ("packs/bar/app/services/bar.rb", "::Foo", "privacy"), + ] + ); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_relationship_violations() -> Result<(), Box> { cargo_bin_cmd!("pks") From cea01ceac6aebe5491f4f8ed6cec8ad9e1ef7acd Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 29 Sep 2026 17:52:10 +0100 Subject: [PATCH 2/8] Report deleted files under a supplied path as stale A run given paths skipped every entry for a file it didn't check, so it stopped reporting entries for deleted files too. On `uses_strict_mode` with `foo.rb` deleted, `pks check packs/foo/app/services/foo.rb`, `pks check packs/foo` and `pks check .` all exited 0, while a full `check` exited 1. A pre-commit hook running `pks check $(git diff --cached --name-only)` gets the deleted file's path, so deleting a file with todo entries passed the hook and left the entries behind. `check_all` now keeps the supplied paths, joined onto the root, in place of the `scoped` flag. An entry whose file wasn't checked is stale when the file is at or under one of those paths and isn't on disk. Joining leaves absolute arguments unchanged, and `.` or a trailing slash still covers everything under it. Entries for files outside the supplied paths are still skipped, so checking one file no longer fails on entries elsewhere in the app. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 10 +-- src/packs/checker.rs | 33 +++++--- tests/check_test.rs | 186 +++++++++++++++++++++++++++++++++++++++---- 3 files changed, 200 insertions(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 56dce572..36816f68 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,11 +16,11 @@ exceed the number of distinct violations. Each violation now appears once, and When given paths, `pks check` treated a `package_todo.yml` entry for any file it had not checked as if that file had been deleted, and reported it as stale. -So checking one file, as editor integrations do on save, failed whenever any -other file had an entry. `check-contents` behaved the same way. A run given -paths now only considers entries for the files it checked. A full `pks check` -still reports entries for deleted files as stale; a run given a directory, such -as `pks check packs/foo` or `pks check .`, no longer does. +So checking one file, as editor integrations do, failed whenever any other file +had an entry. `check-contents` behaved the same way. A run given paths now only +considers entries for files at or under those paths. An entry for a deleted +file is still reported as stale when the run is given that file, a directory +containing it, or `.`. ## 0.5.0 diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 13eda8f3..17094ae0 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -24,7 +24,10 @@ use rayon::prelude::IntoParallelRefIterator; use rayon::prelude::ParallelIterator; use reference::Reference; use std::collections::HashMap; -use std::{collections::HashSet, path::PathBuf}; +use std::{ + collections::HashSet, + path::{Path, PathBuf}, +}; use tracing::debug; use super::bin_locater; @@ -100,9 +103,10 @@ struct CheckAllBuilder<'a> { #[derive(Debug)] struct FoundViolations { absolute_paths: HashSet, - // Set when the caller named paths to check, even if none of them is an - // included file, so it cannot be derived from `absolute_paths`. - scoped: bool, + // The paths the caller named, joined onto the root; empty for a full run. + // A named path can match no included file, or be a deleted one, so this + // cannot be derived from `absolute_paths`. + supplied_paths: Vec, violations: HashSet, } @@ -191,7 +195,8 @@ impl<'a> CheckAllBuilder<'a> { }) .collect::>>()?; - let scoped = self.found_violations.scoped; + let absolute_root = &self.configuration.absolute_root; + let supplied_paths = &self.found_violations.supplied_paths; let stale_violations = recorded_violations .par_iter() .filter(|v_identifier| { @@ -199,7 +204,8 @@ impl<'a> CheckAllBuilder<'a> { &relative_files, &found_violation_identifiers, v_identifier, - scoped, + absolute_root, + supplied_paths, ) }) .collect::>(); @@ -210,14 +216,18 @@ impl<'a> CheckAllBuilder<'a> { relative_files: &HashSet<&str>, found_violation_identifiers: &HashSet<&ViolationIdentifier>, todo_violation_identifier: &ViolationIdentifier, - scoped: bool, + absolute_root: &Path, + supplied_paths: &[PathBuf], ) -> bool { let file_was_checked = relative_files.contains(todo_violation_identifier.file.as_str()); if file_was_checked { !found_violation_identifiers.contains(todo_violation_identifier) - } else if scoped { - false + } else if !supplied_paths.is_empty() { + let path = absolute_root.join(&todo_violation_identifier.file); + // `Path::starts_with` compares whole components, so `packs/foo` + // does not cover `packs/foobar`. + supplied_paths.iter().any(|p| path.starts_with(p)) && !path.exists() } else { true // The todo violation references a file that no longer exists } @@ -258,7 +268,10 @@ pub(crate) fn check_all( get_all_violations(configuration, &absolute_paths, &checkers)?; let found_violations = FoundViolations { absolute_paths, - scoped: !files.is_empty(), + supplied_paths: files + .iter() + .map(|f| configuration.absolute_root.join(f)) + .collect(), violations, }; debug!("Building check-all result (diffing against package_todo.yml)"); diff --git a/tests/check_test.rs b/tests/check_test.rs index 7b5262ea..60fb4356 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -25,6 +25,25 @@ pub fn stripped_output(output: Vec) -> String { String::from_utf8_lossy(&strip_ansi_escapes::strip(output)).to_string() } +fn sorted_stale_todos( + json_output: &serde_json::Value, +) -> Vec<(&str, &str, &str)> { + let mut stale_todos: Vec<(&str, &str, &str)> = json_output["stale_todos"] + .as_array() + .unwrap() + .iter() + .map(|t| { + ( + t["file"].as_str().unwrap(), + t["constant_name"].as_str().unwrap(), + t["violation_type"].as_str().unwrap(), + ) + }) + .collect(); + stale_todos.sort(); + stale_todos +} + #[test] fn test_check_with_privacy_dependency_error_template_overrides( ) -> Result<(), Box> { @@ -336,6 +355,158 @@ fn test_check_with_recorded_strict_mode_violation_when_file_is_deleted( Ok(()) } +#[test] +fn test_check_with_deleted_file_reports_its_recorded_violations_as_stale( +) -> Result<(), Box> { + let fixture = common::Fixture::new("uses_strict_mode"); + fs::remove_file(fixture.path("packs/foo/app/services/foo.rb"))?; + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check") + .arg("-o") + .arg("json") + .arg("packs/foo/app/services/foo.rb") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!( + sorted_stale_todos(&json_output), + vec![ + ("packs/foo/app/services/foo.rb", "::Bar", "dependency"), + ("packs/foo/app/services/foo.rb", "::Bar", "privacy"), + ] + ); + + Ok(()) +} + +#[test] +fn test_check_with_directory_reports_recorded_violations_of_deleted_files_as_stale( +) -> Result<(), Box> { + let fixture = common::Fixture::new("uses_strict_mode"); + fs::remove_file(fixture.path("packs/foo/app/services/foo.rb"))?; + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check") + .arg("-o") + .arg("json") + .arg("packs/foo") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!( + sorted_stale_todos(&json_output), + vec![ + ("packs/foo/app/services/foo.rb", "::Bar", "dependency"), + ("packs/foo/app/services/foo.rb", "::Bar", "privacy"), + ] + ); + + Ok(()) +} + +#[test] +fn test_check_with_dot_reports_recorded_violations_of_deleted_files_as_stale( +) -> Result<(), Box> { + let fixture = common::Fixture::new("uses_strict_mode"); + fs::remove_file(fixture.path("packs/foo/app/services/foo.rb"))?; + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check") + .arg("-o") + .arg("json") + .arg(".") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!( + sorted_stale_todos(&json_output), + vec![ + ("packs/foo/app/services/foo.rb", "::Bar", "dependency"), + ("packs/foo/app/services/foo.rb", "::Bar", "privacy"), + ] + ); + + Ok(()) +} + +#[test] +fn test_check_with_other_paths_ignores_recorded_violations_of_deleted_files( +) -> Result<(), Box> { + let fixture = common::Fixture::new("uses_strict_mode"); + fs::remove_file(fixture.path("packs/foo/app/services/foo.rb"))?; + + for path in ["packs/bar", "packs/bar/app/services/bar.rb"] { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check") + .arg(path) + .assert() + .code(0) + .stdout( + predicate::str::contains("There were stale violations found") + .not(), + ); + } + + Ok(()) +} + +#[test] +fn test_check_with_absolute_directory_ignores_recorded_violations_of_existing_files( +) -> Result<(), Box> { + // Absolute directories aren't expanded into files, so this checks nothing. + // foo.rb is under the argument but still on disk, so its entries must not + // be reported as if it had been deleted. + let packs_foo = + fs::canonicalize("tests/fixtures/uses_strict_mode/packs/foo")?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg(packs_foo) + .assert() + .code(0) + .stdout( + predicate::str::contains("There were stale violations found").not(), + ); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_single_file_ignores_recorded_violations_in_other_files( ) -> Result<(), Box> { @@ -509,21 +680,8 @@ fn test_check_with_single_file_reports_stale_violations_in_bar_rb( validate_check_output_schema(&json_output); assert_eq!(json_output["summary"]["stale_todo_count"], 2); - let mut stale_todos: Vec<(&str, &str, &str)> = json_output["stale_todos"] - .as_array() - .unwrap() - .iter() - .map(|t| { - ( - t["file"].as_str().unwrap(), - t["constant_name"].as_str().unwrap(), - t["violation_type"].as_str().unwrap(), - ) - }) - .collect(); - stale_todos.sort(); assert_eq!( - stale_todos, + sorted_stale_todos(&json_output), vec![ ("packs/bar/app/services/bar.rb", "::Foo", "dependency"), ("packs/bar/app/services/bar.rb", "::Foo", "privacy"), From 1d5a6ffc11852aabe3f1f3b1831d6482fbec40b5 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 29 Sep 2026 17:55:17 +0100 Subject: [PATCH 3/8] Test stale entries under a directory argument and in check-contents Both cases go through code that other tests already reach, but neither had a test of its own: a directory argument where a file inside it has a stale entry, and `check-contents` reporting a stale entry for the file it was given. Also reword the absolute-path test's comment. packwerk-vscode passes a path relative to the root; the test is there because absolute arguments take their own branch in `user_inputted_paths_to_absolute_filepaths`. Co-Authored-By: Claude Opus 5.5 --- tests/check_test.rs | 59 ++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/tests/check_test.rs b/tests/check_test.rs index 60fb4356..0f82ce32 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -624,7 +624,8 @@ fn test_check_with_single_file_reports_stale_violations_in_foo_rb( #[test] fn test_check_with_single_absolute_file_reports_stale_violations_in_foo_rb( ) -> Result<(), Box> { - // Editor integrations pass the absolute path of the file being saved. + // Absolute arguments aren't joined onto the project root, so they take + // their own branch in `user_inputted_paths_to_absolute_filepaths`. let foo_rb = fs::canonicalize( "tests/fixtures/contains_stale_violations/packs/foo/app/services/foo.rb", )?; @@ -692,6 +693,62 @@ fn test_check_with_single_file_reports_stale_violations_in_bar_rb( Ok(()) } +#[test] +fn test_check_with_directory_reports_stale_violations_in_that_directory( +) -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_stale_violations") + .arg("check") + .arg("-o") + .arg("json") + .arg("packs/foo") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!( + sorted_stale_todos(&json_output), + vec![("packs/foo/app/services/foo.rb", "::Bar", "dependency")] + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_contents_reports_stale_violations_in_that_file( +) -> Result<(), Box> { + // bar.rb's entries are out of scope and bar.rb is still on disk, so the + // stale line can only come from foo.rb's own `::Bar` dependency entry. + let project_root = "tests/fixtures/contains_stale_violations"; + let relative_path = "packs/foo/app/services/foo.rb"; + let foo_rb_contents = + fs::read_to_string(format!("{}/{}", project_root, relative_path))?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(project_root) + .arg("check-contents") + .arg(relative_path) + .write_stdin(foo_rb_contents) + .assert() + .code(1) + .stdout(predicate::str::contains( + "There were stale violations found, please run `packs update`", + )); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_relationship_violations() -> Result<(), Box> { cargo_bin_cmd!("pks") From 41d4759a0b0a1ba749cb344c821d26d2e7fe5837 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Wed, 30 Sep 2026 09:44:53 +0100 Subject: [PATCH 4/8] Warn about path arguments that match no included file An argument that matches no included file checks nothing: a typo, an excluded file, a path containing `..`, an absolute directory, or an absolute path that doesn't go through the canonical project root, such as a symlinked `/tmp` on macOS. In an app with todo entries those runs used to fail on other files' stale entries. Now they exit 0 with "No violations detected!", so the mistake goes unnoticed. `check_all` now intersects each argument with the included files on its own and prints a warning on stderr for any that matched nothing. The union of those sets is what a single call returned, and a directory is still globbed once. The warning goes to stderr and doesn't change the exit code, so `-o json` and `-o csv` stdout stays parseable. Editor plugins check one file on every open and save. packwerk-vscode shows stderr only when its `showWarnings` setting is on, which is off by default, and packwerk-intellij reads only stdout, so neither treats the warning as a failure. packwerk prints "No files found or given" on stdout, and only when no argument matched at all. pks names each unmatched argument, including in a longer list, and keeps stdout clean. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++++ src/packs/checker.rs | 33 ++++++++++++- tests/check_test.rs | 108 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 36816f68..7252571d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,14 @@ considers entries for files at or under those paths. An entry for a deleted file is still reported as stale when the run is given that file, a directory containing it, or `.`. +#### `check` and `check-contents` warn about paths that match no included file + +A path argument that matched no included file was silently skipped, so a typo, +an excluded file, a path containing `..`, or an absolute path through a symlink +checked nothing without saying so. `check` and `check-contents` now print a +warning on stderr naming each such argument. The exit code is unchanged, and so +is stdout, so `-o json` and `-o csv` output still parses. + ## 0.5.0 ### Breaking Changes diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 17094ae0..b92193a8 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -261,8 +261,7 @@ pub(crate) fn check_all( let checkers = get_checkers(configuration); debug!("Intersecting input files with configuration included files"); - let absolute_paths: HashSet = - configuration.intersect_files(files.clone()); + let absolute_paths = files_to_check(configuration, &files); let violations: HashSet = get_all_violations(configuration, &absolute_paths, &checkers)?; @@ -280,6 +279,36 @@ pub(crate) fn check_all( result } +/// Intersects each path argument with the included files on its own, so an +/// argument that matches none of them can be named instead of silently +/// checking nothing. The union is what `intersect_files` returns for all of +/// them at once. +/// +/// The warning goes to stderr and leaves the exit code alone: `-o json` and +/// `-o csv` stay parseable, and editor plugins, which check one file on every +/// open and save, don't treat it as a failure. +fn files_to_check( + configuration: &Configuration, + files: &[String], +) -> HashSet { + if files.is_empty() { + return configuration.intersect_files(vec![]); + } + + let mut absolute_paths = HashSet::new(); + for file in files { + let matched = configuration.intersect_files(vec![file.clone()]); + if matched.is_empty() { + eprintln!( + "Warning: no included file matches `{}`, so it was not checked. Check the path and the include and exclude globs in the config file.", + file + ); + } + absolute_paths.extend(matched); + } + absolute_paths +} + fn validate(configuration: &Configuration) -> Vec { debug!("Running validators against packages"); let validators: Vec> = vec![ diff --git a/tests/check_test.rs b/tests/check_test.rs index 0f82ce32..f0750acc 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -589,6 +589,114 @@ fn test_check_with_only_unincluded_files_ignores_recorded_violations( Ok(()) } +#[test] +fn test_check_warns_about_path_matching_no_included_file( +) -> Result<(), Box> { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/foo/app/services/typo.rb") + .assert() + .code(0) + .stderr(predicate::str::contains( + "Warning: no included file matches `packs/foo/app/services/typo.rb`", + )); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_warns_only_about_paths_matching_no_included_file( +) -> Result<(), Box> { + // Absolute directories aren't expanded into files, so this one matches + // nothing, while bar.rb is still checked. + let packs_foo = + fs::canonicalize("tests/fixtures/uses_strict_mode/packs/foo")?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/bar/app/services/bar.rb") + .arg(&packs_foo) + .assert() + .code(0) + .stderr(predicate::str::contains(format!( + "Warning: no included file matches `{}`", + packs_foo.display() + ))) + .stderr(predicate::str::contains("bar.rb").not()); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_json_output_warns_on_stderr() -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("-o") + .arg("json") + .arg("packs/foo/app/services/typo.rb") + .assert() + .code(0) + .stderr(predicate::str::contains( + "Warning: no included file matches `packs/foo/app/services/typo.rb`", + )) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_contents_warns_about_path_matching_no_included_file( +) -> Result<(), Box> { + // package.yml exists but is not in the include globs. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check-contents") + .arg("packs/foo/package.yml") + .write_stdin("enforce_dependencies: true\n") + .assert() + .code(0) + .stderr(predicate::str::contains( + "Warning: no included file matches `packs/foo/package.yml`", + )); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_does_not_warn_when_every_path_matches_an_included_file( +) -> Result<(), Box> { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/bar/app/services/bar.rb") + .arg("packs/foo") + .assert() + .code(0) + .stderr(predicate::str::is_empty()); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_single_file_reports_stale_violations_in_foo_rb( ) -> Result<(), Box> { From 6945975d6ad61fbf7ef5bf06ec9395785635ab21 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Wed, 30 Sep 2026 10:05:24 +0100 Subject: [PATCH 5/8] Use the full run's test for unchecked files under a supplied path Calling an unchecked entry under a supplied path stale only when its file is missing from disk skips a file that still exists but is no longer included, because it was newly excluded for example. A full `check` reports that entry as stale, but `check .` and `check packs/foo` didn't. The test is now whether the file is an included file, which is what a full run checks, so a run given paths agrees with a full run for every entry under those paths. An absolute directory argument still flags nothing: it isn't expanded, so nothing under it is checked, but its files are still included. This also drops a stat per entry. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 +++--- src/packs/checker.rs | 11 +++++++--- tests/check_test.rs | 48 +++++++++++++++++++++++++++++++++++++++++--- 3 files changed, 56 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7252571d..eaaf2a4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,9 +18,9 @@ When given paths, `pks check` treated a `package_todo.yml` entry for any file it had not checked as if that file had been deleted, and reported it as stale. So checking one file, as editor integrations do, failed whenever any other file had an entry. `check-contents` behaved the same way. A run given paths now only -considers entries for files at or under those paths. An entry for a deleted -file is still reported as stale when the run is given that file, a directory -containing it, or `.`. +considers entries for files at or under those paths. An entry for a file that +was deleted or is no longer included is still reported as stale when the run is +given that file, a directory containing it, or `.`, as a full `pks check` does. #### `check` and `check-contents` warn about paths that match no included file diff --git a/src/packs/checker.rs b/src/packs/checker.rs index b92193a8..34ca29af 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -196,6 +196,7 @@ impl<'a> CheckAllBuilder<'a> { .collect::>>()?; let absolute_root = &self.configuration.absolute_root; + let included_files = &self.configuration.included_files; let supplied_paths = &self.found_violations.supplied_paths; let stale_violations = recorded_violations .par_iter() @@ -205,6 +206,7 @@ impl<'a> CheckAllBuilder<'a> { &found_violation_identifiers, v_identifier, absolute_root, + included_files, supplied_paths, ) }) @@ -217,6 +219,7 @@ impl<'a> CheckAllBuilder<'a> { found_violation_identifiers: &HashSet<&ViolationIdentifier>, todo_violation_identifier: &ViolationIdentifier, absolute_root: &Path, + included_files: &HashSet, supplied_paths: &[PathBuf], ) -> bool { let file_was_checked = @@ -225,9 +228,11 @@ impl<'a> CheckAllBuilder<'a> { !found_violation_identifiers.contains(todo_violation_identifier) } else if !supplied_paths.is_empty() { let path = absolute_root.join(&todo_violation_identifier.file); - // `Path::starts_with` compares whole components, so `packs/foo` - // does not cover `packs/foobar`. - supplied_paths.iter().any(|p| path.starts_with(p)) && !path.exists() + // Not being an included file is what makes an entry stale in a full + // run too. `Path::starts_with` compares whole components, so + // `packs/foo` does not cover `packs/foobar`. + supplied_paths.iter().any(|p| path.starts_with(p)) + && !included_files.contains(&path) } else { true // The todo violation references a file that no longer exists } diff --git a/tests/check_test.rs b/tests/check_test.rs index f0750acc..31af02d4 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -460,6 +460,48 @@ fn test_check_with_dot_reports_recorded_violations_of_deleted_files_as_stale( Ok(()) } +#[test] +fn test_check_with_dot_reports_recorded_violations_of_excluded_files_as_stale( +) -> Result<(), Box> { + // foo.rb is still on disk but no longer included, which a full run + // reports as stale too. + let fixture = common::Fixture::new("uses_strict_mode"); + let packwerk_yml = fixture.path("packwerk.yml"); + let config = fs::read_to_string(&packwerk_yml)?; + fs::write( + &packwerk_yml, + format!("{}\nexclude:\n- \"packs/foo/**/*\"\n", config), + )?; + + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg(fixture.root()) + .arg("check") + .arg("-o") + .arg("json") + .arg(".") + .assert() + .code(1) + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!( + sorted_stale_todos(&json_output), + vec![ + ("packs/foo/app/services/foo.rb", "::Bar", "dependency"), + ("packs/foo/app/services/foo.rb", "::Bar", "privacy"), + ] + ); + + Ok(()) +} + #[test] fn test_check_with_other_paths_ignores_recorded_violations_of_deleted_files( ) -> Result<(), Box> { @@ -484,11 +526,11 @@ fn test_check_with_other_paths_ignores_recorded_violations_of_deleted_files( } #[test] -fn test_check_with_absolute_directory_ignores_recorded_violations_of_existing_files( +fn test_check_with_absolute_directory_ignores_recorded_violations_of_included_files( ) -> Result<(), Box> { // Absolute directories aren't expanded into files, so this checks nothing. - // foo.rb is under the argument but still on disk, so its entries must not - // be reported as if it had been deleted. + // foo.rb is under the argument but still an included file, so its entries + // must not be reported as stale. let packs_foo = fs::canonicalize("tests/fixtures/uses_strict_mode/packs/foo")?; From afe30a56ea91aca210df0815f3dfbae4d9cfebe6 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Wed, 30 Sep 2026 10:07:25 +0100 Subject: [PATCH 6/8] Say a path doesn't exist instead of pointing at the include globs For a path that doesn't exist, the warning said no included file matched and suggested checking the include and exclude globs. That advice can't help, and for a deleted file it sat next to the stale report for that same file. A pre-commit hook passing `git diff --cached --name-only` hits this on every staged deletion. When the argument joined onto the root doesn't exist, the warning now says only that. An existing path that matches no included file keeps the previous message. Co-Authored-By: Claude Opus 5.5 --- src/packs/checker.rs | 12 ++++++++---- tests/check_test.rs | 10 +++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 34ca29af..7049cef4 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -304,10 +304,14 @@ fn files_to_check( for file in files { let matched = configuration.intersect_files(vec![file.clone()]); if matched.is_empty() { - eprintln!( - "Warning: no included file matches `{}`, so it was not checked. Check the path and the include and exclude globs in the config file.", - file - ); + if configuration.absolute_root.join(file).exists() { + eprintln!( + "Warning: no included file matches `{}`, so it was not checked. Check the path and the include and exclude globs in the config file.", + file + ); + } else { + eprintln!("Warning: `{}` does not exist.", file); + } } absolute_paths.extend(matched); } diff --git a/tests/check_test.rs b/tests/check_test.rs index 31af02d4..f2a241e9 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -370,6 +370,10 @@ fn test_check_with_deleted_file_reports_its_recorded_violations_as_stale( .arg("packs/foo/app/services/foo.rb") .assert() .code(1) + .stderr(predicate::str::contains( + "Warning: `packs/foo/app/services/foo.rb` does not exist.", + )) + .stderr(predicate::str::contains("include and exclude globs").not()) .get_output() .stdout .clone(); @@ -632,7 +636,7 @@ fn test_check_with_only_unincluded_files_ignores_recorded_violations( } #[test] -fn test_check_warns_about_path_matching_no_included_file( +fn test_check_warns_about_path_that_does_not_exist( ) -> Result<(), Box> { cargo_bin_cmd!("pks") .arg("--project-root") @@ -642,7 +646,7 @@ fn test_check_warns_about_path_matching_no_included_file( .assert() .code(0) .stderr(predicate::str::contains( - "Warning: no included file matches `packs/foo/app/services/typo.rb`", + "Warning: `packs/foo/app/services/typo.rb` does not exist.", )); common::teardown(); @@ -687,7 +691,7 @@ fn test_check_with_json_output_warns_on_stderr() -> Result<(), Box> { .assert() .code(0) .stderr(predicate::str::contains( - "Warning: no included file matches `packs/foo/app/services/typo.rb`", + "Warning: `packs/foo/app/services/typo.rb` does not exist.", )) .get_output() .stdout From 3e4569a7f2858ee0eb0c02fc7b7e43ce4d503204 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Wed, 30 Sep 2026 14:52:54 +0100 Subject: [PATCH 7/8] Don't warn about existing files that just aren't included A hook passing every staged file, or an editor opening the Gemfile, sends existing files that pks never checks: YAML, a README, the Gemfile, an excluded Ruby file. Warning about each of them adds noise and says nothing the user didn't already know. An argument that matches no included file now warns only when it's likely a mistake: - a path that doesn't exist, as before; - a directory with no included files, with the include and exclude advice, as before; - an existing file whose canonical path is an included file, which means it was spelled with `..` or through a symlink. The warning names the path to pass instead. Any other existing file is skipped quietly. That still covers the typo, `..`, absolute directory and symlink cases the warning was added for. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 13 +++++++++---- src/packs/checker.rs | 43 ++++++++++++++++++++++++++++++++++--------- tests/check_test.rs | 39 +++++++++++++++++++++++++++++++++++++-- 3 files changed, 80 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eaaf2a4f..8e23ffe9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,10 +25,15 @@ given that file, a directory containing it, or `.`, as a full `pks check` does. #### `check` and `check-contents` warn about paths that match no included file A path argument that matched no included file was silently skipped, so a typo, -an excluded file, a path containing `..`, or an absolute path through a symlink -checked nothing without saying so. `check` and `check-contents` now print a -warning on stderr naming each such argument. The exit code is unchanged, and so -is stdout, so `-o json` and `-o csv` output still parses. +a path containing `..`, an absolute directory, or an absolute path through a +symlink checked nothing without saying so. `check` and `check-contents` now +print a warning on stderr naming the argument: a missing path is reported as +missing, a `..` or symlinked spelling of an included file gets the path to pass +instead, and a directory with no included files gets a pointer to the include +and exclude globs. An existing file that just isn't included, such as a README, +`package.yml` or the Gemfile, is still skipped quietly, so passing every changed +file doesn't warn about each of them. The exit code is unchanged, and so is +stdout, so `-o json` and `-o csv` output still parses. ## 0.5.0 diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 7049cef4..3adf0213 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -26,6 +26,7 @@ use reference::Reference; use std::collections::HashMap; use std::{ collections::HashSet, + fs, path::{Path, PathBuf}, }; use tracing::debug; @@ -285,7 +286,7 @@ pub(crate) fn check_all( } /// Intersects each path argument with the included files on its own, so an -/// argument that matches none of them can be named instead of silently +/// argument that matches none of them can be reported instead of silently /// checking nothing. The union is what `intersect_files` returns for all of /// them at once. /// @@ -304,20 +305,44 @@ fn files_to_check( for file in files { let matched = configuration.intersect_files(vec![file.clone()]); if matched.is_empty() { - if configuration.absolute_root.join(file).exists() { - eprintln!( - "Warning: no included file matches `{}`, so it was not checked. Check the path and the include and exclude globs in the config file.", - file - ); - } else { - eprintln!("Warning: `{}` does not exist.", file); - } + warn_about_unmatched_path(configuration, file); } absolute_paths.extend(matched); } absolute_paths } +/// Stays silent for an existing file that simply isn't included (a README, +/// `package.yml`, the Gemfile), so a hook passing every staged file doesn't +/// warn on each of them. +fn warn_about_unmatched_path(configuration: &Configuration, file: &str) { + let path = configuration.absolute_root.join(file); + if !path.exists() { + eprintln!("Warning: `{}` does not exist.", file); + } else if path.is_file() { + // Included files are listed under the canonical root with no `..`, so + // a symlinked or `..` spelling of one matches nothing. + let included = fs::canonicalize(&path) + .ok() + .filter(|c| configuration.included_files.contains(c)); + if let Some(included) = included { + let relative = included + .strip_prefix(&configuration.absolute_root) + .unwrap_or(&included); + eprintln!( + "Warning: `{}` was not checked. Pass it as `{}` instead.", + file, + relative.display() + ); + } + } else { + eprintln!( + "Warning: no included file matches `{}`, so it was not checked. Check the path and the include and exclude globs in the config file.", + file + ); + } +} + fn validate(configuration: &Configuration) -> Vec { debug!("Running validators against packages"); let validators: Vec> = vec![ diff --git a/tests/check_test.rs b/tests/check_test.rs index f2a241e9..1a208e29 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -707,7 +707,7 @@ fn test_check_with_json_output_warns_on_stderr() -> Result<(), Box> { } #[test] -fn test_check_contents_warns_about_path_matching_no_included_file( +fn test_check_contents_does_not_warn_about_existing_file_that_is_not_included( ) -> Result<(), Box> { // package.yml exists but is not in the include globs. cargo_bin_cmd!("pks") @@ -718,8 +718,43 @@ fn test_check_contents_warns_about_path_matching_no_included_file( .write_stdin("enforce_dependencies: true\n") .assert() .code(0) + .stderr(predicate::str::is_empty()); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_does_not_warn_about_existing_files_that_are_not_included( +) -> Result<(), Box> { + // What a hook passing every staged file sends alongside the Ruby files. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/bar/app/services/bar.rb") + .arg("packs/foo/package.yml") + .arg("packwerk.yml") + .assert() + .code(0) + .stderr(predicate::str::is_empty()); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_warns_about_other_spelling_of_included_file( +) -> Result<(), Box> { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg("packs/bar/../foo/app/services/foo.rb") + .assert() + .code(0) .stderr(predicate::str::contains( - "Warning: no included file matches `packs/foo/package.yml`", + "Warning: `packs/bar/../foo/app/services/foo.rb` was not checked. Pass it as `packs/foo/app/services/foo.rb` instead.", )); common::teardown(); From 9f0be51bef3c725a77183640ee9ba07ecd714068 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Wed, 30 Sep 2026 15:07:13 +0100 Subject: [PATCH 8/8] Suggest the path to pass for a directory spelled another way An absolute directory isn't expanded into files, and a `..` or symlinked spelling of a directory matches nothing, so each of them got the advice to check the include and exclude globs. That can't help when the directory holds included files, and absolute directories are one of the cases the warning is for. A directory now gets the same check as a file: if any included file is under its canonical path, the warning names the root-relative path to pass instead, `.` for the project root. Only a directory with no included files keeps the include and exclude advice. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 14 ++++++------ src/packs/checker.rs | 51 +++++++++++++++++++++++++++++--------------- tests/check_test.rs | 26 +++++++++++++++++++++- 3 files changed, 65 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e23ffe9..20dc58e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,14 +26,12 @@ given that file, a directory containing it, or `.`, as a full `pks check` does. A path argument that matched no included file was silently skipped, so a typo, a path containing `..`, an absolute directory, or an absolute path through a -symlink checked nothing without saying so. `check` and `check-contents` now -print a warning on stderr naming the argument: a missing path is reported as -missing, a `..` or symlinked spelling of an included file gets the path to pass -instead, and a directory with no included files gets a pointer to the include -and exclude globs. An existing file that just isn't included, such as a README, -`package.yml` or the Gemfile, is still skipped quietly, so passing every changed -file doesn't warn about each of them. The exit code is unchanged, and so is -stdout, so `-o json` and `-o csv` output still parses. +symlink checked nothing without saying so. `check` and `check-contents` now warn +on stderr, naming the argument and, where there is one, the path to pass +instead. An existing file that just isn't included, such as a README or the +Gemfile, is still skipped quietly, so passing every changed file doesn't warn +about each of them. The exit code and stdout are unchanged, so `-o json` and +`-o csv` output still parses. ## 0.5.0 diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 3adf0213..27294c92 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -319,23 +319,13 @@ fn warn_about_unmatched_path(configuration: &Configuration, file: &str) { let path = configuration.absolute_root.join(file); if !path.exists() { eprintln!("Warning: `{}` does not exist.", file); - } else if path.is_file() { - // Included files are listed under the canonical root with no `..`, so - // a symlinked or `..` spelling of one matches nothing. - let included = fs::canonicalize(&path) - .ok() - .filter(|c| configuration.included_files.contains(c)); - if let Some(included) = included { - let relative = included - .strip_prefix(&configuration.absolute_root) - .unwrap_or(&included); - eprintln!( - "Warning: `{}` was not checked. Pass it as `{}` instead.", - file, - relative.display() - ); - } - } else { + } else if let Some(relative) = included_spelling(configuration, &path) { + eprintln!( + "Warning: `{}` was not checked. Pass it as `{}` instead.", + file, + relative.display() + ); + } else if !path.is_file() { eprintln!( "Warning: no included file matches `{}`, so it was not checked. Check the path and the include and exclude globs in the config file.", file @@ -343,6 +333,33 @@ fn warn_about_unmatched_path(configuration: &Configuration, file: &str) { } } +/// The root-relative path to pass instead of `path`, when `path` is an +/// included file or a directory holding one, spelled in a way that matches +/// nothing. Included files are listed under the canonical root with no `..`, +/// and only relative directories are expanded, so a `..`, symlinked or +/// absolute spelling misses them. +fn included_spelling( + configuration: &Configuration, + path: &Path, +) -> Option { + let canonical = fs::canonicalize(path).ok()?; + let included = &configuration.included_files; + let holds_included = if path.is_file() { + included.contains(&canonical) + } else { + included.iter().any(|f| f.starts_with(&canonical)) + }; + if !holds_included { + return None; + } + let relative = canonical.strip_prefix(&configuration.absolute_root).ok()?; + if relative.as_os_str().is_empty() { + Some(PathBuf::from(".")) + } else { + Some(relative.to_path_buf()) + } +} + fn validate(configuration: &Configuration) -> Vec { debug!("Running validators against packages"); let validators: Vec> = vec![ diff --git a/tests/check_test.rs b/tests/check_test.rs index 1a208e29..836c9c86 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -409,6 +409,9 @@ fn test_check_with_directory_reports_recorded_violations_of_deleted_files_as_sta .arg("packs/foo") .assert() .code(1) + .stderr(predicate::str::contains( + "Warning: no included file matches `packs/foo`", + )) .get_output() .stdout .clone(); @@ -670,7 +673,7 @@ fn test_check_warns_only_about_paths_matching_no_included_file( .assert() .code(0) .stderr(predicate::str::contains(format!( - "Warning: no included file matches `{}`", + "Warning: `{}` was not checked. Pass it as `packs/foo` instead.", packs_foo.display() ))) .stderr(predicate::str::contains("bar.rb").not()); @@ -679,6 +682,27 @@ fn test_check_warns_only_about_paths_matching_no_included_file( Ok(()) } +#[test] +fn test_check_warns_about_absolute_project_root() -> Result<(), Box> +{ + let root = fs::canonicalize("tests/fixtures/uses_strict_mode")?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .arg(&root) + .assert() + .code(0) + .stderr(predicate::str::contains(format!( + "Warning: `{}` was not checked. Pass it as `.` instead.", + root.display() + ))); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_json_output_warns_on_stderr() -> Result<(), Box> { let output = cargo_bin_cmd!("pks")