From 87ed033cb5b6909c132dde889cef2cbaff9a2d83 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Mon, 3 Aug 2026 08:57:51 +0100 Subject: [PATCH] Match recorded violations in strict packs `ViolationIdentifier` carries `strict`, but violations rebuilt from `package_todo.yml` always get `strict: false`, so a found violation in a strict pack could never equal its recorded entry. That made all three comparisons in `CheckAllBuilder` miss at once: the same recorded violation was reported as new, as a strict-mode violation, and as a stale todo. Comparisons now go through `recorded_key()`, which zeroes the flag, since `strict` describes how a violation is treated rather than which one it is. `build_strict_mode_violations` also skips recorded violations now, matching packwerk's `unlisted_strict_mode_violations` (Shopify/packwerk#368), so turning strict on blocks new violations without requiring every recorded one to be fixed first. `--ignore-recorded-violations` still surfaces them. --- src/packs/checker.rs | 43 ++++++++++++++++++++++++++++++++++++------- tests/check_test.rs | 43 ++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 76 insertions(+), 10 deletions(-) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index cfd694b..984f846 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -39,6 +39,19 @@ pub struct ViolationIdentifier { pub referencing_pack_name: String, pub defining_pack_name: String, } + +impl ViolationIdentifier { + /// `strict` describes how a violation should be treated, not which violation + /// it is, and `package_todo.yml` has nowhere to record it, so recorded + /// violations are always rebuilt with `strict: false`. Compare through this + /// so a violation in a strict pack can still match its recorded entry. + pub fn recorded_key(&self) -> Self { + Self { + strict: false, + ..self.clone() + } + } +} /// A violation combines an identifier with display metadata. /// /// `source_location` is intentionally separate from `ViolationIdentifier` because: @@ -124,7 +137,7 @@ impl<'a> CheckAllBuilder<'a> { .cloned() .collect(), strict_mode_violations: self - .build_strict_mode_violations() + .build_strict_mode_violations(recorded_violations) .into_iter() .collect(), }) @@ -142,7 +155,10 @@ impl<'a> CheckAllBuilder<'a> { self.found_violations .violations .iter() - .filter(|v| !recorded_violations.contains(&v.identifier)) + .filter(|v| { + !recorded_violations + .contains(&v.identifier.recorded_key()) + }) .collect() }; reportable_violations @@ -152,11 +168,11 @@ impl<'a> CheckAllBuilder<'a> { &mut self, recorded_violations: &'a HashSet, ) -> anyhow::Result> { - let found_violation_identifiers: HashSet<&ViolationIdentifier> = self + let found_violation_identifiers: HashSet = self .found_violations .violations .par_iter() - .map(|v| &v.identifier) + .map(|v| v.identifier.recorded_key()) .collect(); let relative_files = self .found_violations @@ -198,23 +214,36 @@ impl<'a> CheckAllBuilder<'a> { fn is_stale_violation( relative_files: &HashSet<&str>, - found_violation_identifiers: &HashSet<&ViolationIdentifier>, + found_violation_identifiers: &HashSet, todo_violation_identifier: &ViolationIdentifier, ) -> bool { let violation_path_exists = relative_files.contains(todo_violation_identifier.file.as_str()); if violation_path_exists { - !found_violation_identifiers.contains(todo_violation_identifier) + !found_violation_identifiers + .contains(&todo_violation_identifier.recorded_key()) } else { true // The todo violation references a file that no longer exists } } - fn build_strict_mode_violations(&self) -> Vec { + /// Strict mode reports violations that are not already recorded in a + /// `package_todo.yml`, matching packwerk's `unlisted_strict_mode_violations` + /// (Shopify/packwerk#368). Turning strict on therefore blocks new violations + /// without also requiring every recorded one to be fixed first. + fn build_strict_mode_violations( + &self, + recorded_violations: &HashSet, + ) -> Vec { self.found_violations .violations .iter() .filter(|v| v.identifier.strict) + .filter(|v| { + self.configuration.ignore_recorded_violations + || !recorded_violations + .contains(&v.identifier.recorded_key()) + }) .cloned() .collect() } diff --git a/tests/check_test.rs b/tests/check_test.rs index 4f0d043..57bd310 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -320,11 +320,31 @@ fn test_check_without_stale_violations() -> Result<(), Box> { } #[test] -fn test_check_with_strict_mode() -> Result<(), Box> { +fn test_check_with_recorded_strict_mode_violation() -> Result<(), Box> +{ + // The violation is already recorded in packs/foo/package_todo.yml, so + // strict mode tolerates it and only blocks new ones. Matches packwerk's + // `unlisted_strict_mode_violations` (Shopify/packwerk#368). + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode") + .arg("check") + .assert() + .code(0) + .stdout(predicate::str::contains("No violations detected!")); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_recorded_strict_mode_violation_ignoring_todo( +) -> Result<(), Box> { cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") .arg("check") + .arg("--ignore-recorded-violations") .assert() .code(1) .stdout(predicate::str::contains( @@ -338,18 +358,35 @@ fn test_check_with_strict_mode() -> Result<(), Box> { Ok(()) } +#[test] +fn test_check_with_unrecorded_strict_mode_violation( +) -> Result<(), Box> { + // No package_todo.yml entry for this one, so strict mode must still fail. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_strict_violations") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + )); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { cargo_bin_cmd!("pks") .arg("--project-root") - .arg("tests/fixtures/uses_strict_mode") + .arg("tests/fixtures/contains_strict_violations") .arg("check") .arg("-o") .arg("csv") .assert() .code(1) .stdout(predicate::str::contains("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message")) - .stdout(predicate::str::contains("privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack\'s package.yml file")) .stdout(predicate::str::contains( "privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack\'s package.yml file", ));