From e198118644fdc88ea8afd4d99150ad983a2ec6d2 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Mon, 17 Aug 2026 14:25:56 +0100 Subject: [PATCH 1/6] Match recorded violations in strict packs Fixes #41. `ViolationIdentifier` carries `strict`, but violations rebuilt from `package_todo.yml` always get `strict: false`, so in a strict pack a found violation could never equal its recorded entry. Both comparisons against the recorded set now normalize the found side through `recorded_key()`, which zeroes the flag. That fixes both symptoms: a recorded violation in a strict pack was reported as new, and its todo entry was reported as stale. The todo side needs no normalization. `is_stale_violation` takes its argument from `pack_set.all_violations`, which already rebuilds every recorded violation with `strict: false`, so it is its own recorded key. There is a doc comment saying so, because the asymmetry with the found side reads like an oversight otherwise. `test_check_with_strict_mode` pins the corrected output on `uses_strict_mode`: still exit 1, because strict mode itself is unchanged here, but neither a new-violation report nor a stale-todo line. It fails without the fix. --- src/packs/checker.rs | 29 +++++++++++++++++++++++++---- tests/check_test.rs | 13 ++++++++++++- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/packs/checker.rs b/src/packs/checker.rs index cfd694b..9604219 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -39,6 +39,20 @@ 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: @@ -142,7 +156,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 +169,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 @@ -196,9 +213,13 @@ impl<'a> CheckAllBuilder<'a> { Ok(stale_violations) } + /// `found_violation_identifiers` is keyed by [`ViolationIdentifier::recorded_key`]. + /// `todo_violation_identifier` needs no such normalization: it comes from + /// `pack_set.all_violations`, which rebuilds every recorded violation with + /// `strict: false` already, so it is its own recorded key. 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 = diff --git a/tests/check_test.rs b/tests/check_test.rs index 4f0d043..85f4771 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -321,6 +321,10 @@ fn test_check_without_stale_violations() -> Result<(), Box> { #[test] fn test_check_with_strict_mode() -> Result<(), Box> { + // The violation here IS recorded in packs/foo/package_todo.yml, so it has to + // match its recorded entry: reported neither as a new violation nor as a + // stale todo. Strict mode still fails the run, which is what keeps this at + // exit 1, so the two strict messages are the whole of the output. cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode") @@ -332,7 +336,14 @@ fn test_check_with_strict_mode() -> Result<(), Box> { )) .stdout(predicate::str::contains( "packs/foo cannot have dependency violations on packs/bar because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", - )); + )) + .stdout( + predicate::str::contains( + "There were stale violations found, please run `packs update`", + ) + .not(), + ) + .stdout(predicate::str::contains("violation(s) detected:").not()); common::teardown(); Ok(()) From 7b22b866ed53e8f1951c41f0381f2915950e038d Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Mon, 17 Aug 2026 14:35:20 +0100 Subject: [PATCH 2/6] Tolerate recorded violations in strict mode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Builds on #43, which has to land first: every comparison here needs that PR's `recorded_key()`. `build_strict_mode_violations` now skips violations already recorded in a `package_todo.yml`, matching packwerk's `unlisted_strict_mode_violations` (Shopify/packwerk#368). Turning strict mode on therefore blocks new violations without also requiring the existing list to be emptied first. `--ignore-recorded-violations` still surfaces everything the todo files are grandfathering. Three things had to come with it, because the tolerance reads state that nothing was previously protecting. `write_violations_to_disk` preserved nothing for strict packs: it dropped every strict violation when regenerating todo files, so a routine `pks update` deleted the entries `check` had just started depending on. On `uses_strict_mode` that was check clean, update, check red, with no source change in between. It now drops only the *unlisted* strict violations, so `update` still cannot be used to silence strict mode, but it stops un-grandfathering what strict mode is now tolerating. packwerk keeps the entry for the same reason, in `OffenseCollection#add_offense`. `update`'s summary message filtered on `.identifier.strict` with no recorded filter, so it announced that N violations "must be fixed for `check` to succeed" while `check` reported none. It uses the same filter as the checker now. `CHECKERS.md` asserted the opposite of this behaviour in two places: that strict mode includes violations recorded in other packages' todo files, and that you must clear existing violations before enabling it. Both are rewritten, and the `strict_privacy_ignored_patterns` section now says when to reach for a path exemption rather than a recorded entry, since the recorded case is covered by default. Tests: - `test_check_with_recorded_strict_mode_violation` — the recorded case is clean - `test_check_with_recorded_strict_mode_violation_ignoring_todo` — the escape hatch still reports it - `test_check_with_unrecorded_strict_mode_violation` — an unrecorded strict violation still fails - `test_check_with_partially_recorded_strict_mode_violations` — one recorded and one unrecorded in the same strict pack, in one run. Only the unrecorded one is reported. This is the case that makes strict mode adoptable and nothing covered it, so a regression here would have been silent - `test_update_preserves_recorded_strict_violations` — the recorded entry survives `update`, and the misleading summary line is gone - `test_check_update_check_round_trip_with_strict_mode` — check, update, check, still clean. This is the round trip that was broken `test_check_with_strict_mode_output_csv` moves to `contains_strict_violations`, which ships no todo file, so it still has output to assert against. The duplicate assertion it carried was byte-identical to the one below it, so dropping it costs no coverage. Two new fixtures rather than edits to `uses_strict_mode`, so the mutating tests cannot race the read-only ones: `uses_strict_mode_partially_recorded` and `uses_strict_mode_round_trip`. The CHANGELOG entry follows the `respect_gitignore` who's-affected format. Its `## Unreleased` heading was stale — `2fe98b7` is an ancestor of v0.4.0, so everything under it had already shipped — so that section is now `## 0.4.0` and this change sits under a fresh `## Unreleased`. Pre-1.0, a breaking change like this wants 0.5.0 rather than 0.4.x. --- CHANGELOG.md | 27 ++++++ CHECKERS.md | 11 ++- src/packs/checker.rs | 37 ++++++-- src/packs/package_todo.rs | 15 +++- tests/check_test.rs | 85 +++++++++++++++++-- tests/common/mod.rs | 32 +++++++ .../package.yml | 2 + .../packs/bar/app/services/bar.rb | 2 + .../packs/bar/package.yml | 2 + .../packs/baz/app/services/baz.rb | 2 + .../packs/baz/package.yml | 2 + .../packs/foo/app/services/foo.rb | 9 ++ .../packs/foo/package.yml | 2 + .../packs/foo/package_todo.yml | 15 ++++ .../packwerk.yml | 23 +++++ .../uses_strict_mode_round_trip/package.yml | 2 + .../packs/bar/app/services/bar.rb | 2 + .../packs/bar/package.yml | 2 + .../packs/foo/app/services/foo.rb | 5 ++ .../packs/foo/package.yml | 2 + .../packs/foo/package_todo.yml | 15 ++++ .../uses_strict_mode_round_trip/packwerk.yml | 23 +++++ tests/update_test.rs | 74 ++++++++++++++++ 23 files changed, 370 insertions(+), 21 deletions(-) create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/package.yml create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml create mode 100644 tests/fixtures/uses_strict_mode_round_trip/package.yml create mode 100644 tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb create mode 100644 tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml create mode 100644 tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb create mode 100644 tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml create mode 100644 tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml create mode 100644 tests/fixtures/uses_strict_mode_round_trip/packwerk.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 654a6c7..66d736c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,33 @@ ### Breaking Changes +#### Strict mode tolerates violations already recorded in `package_todo.yml` + +`enforce_privacy: strict` and `enforce_dependencies: strict` now fail only on +references that are **not** already recorded in a `package_todo.yml`. This matches +packwerk's `unlisted_strict_mode_violations` (Shopify/packwerk#368). + +**Who is affected:** any project with a strict pack whose existing violations are +recorded in todo files. Previously `pks check` failed on every one of them, so a +strict pack could only be green with an empty todo list. + +**What changes:** pks silently produces different (smaller) results without any +configuration change. Strict packs that were red because of grandfathered +violations go green. New references into a strict pack still fail, and `pks update` +still refuses to record an unrecorded strict violation, so strict mode cannot be +silenced by running it. + +**Opt out:** there is no config flag, matching packwerk. To see everything the todo +files are grandfathering, run: + +``` +pks check --ignore-recorded-violations +``` + +## 0.4.0 + +### Breaking Changes + #### `respect_gitignore` defaults to `true` pks now respects `.gitignore` files by default. Files and directories matched by diff --git a/CHECKERS.md b/CHECKERS.md index 6cb2d18..b6239cd 100644 --- a/CHECKERS.md +++ b/CHECKERS.md @@ -13,9 +13,11 @@ enforce_privacy: true Setting `enforce_privacy` to `true` will make all references to private constants in your package a violation. -Setting `enforce_privacy` to `strict` will forbid all references to private constants in your package. **This includes violations that have been added to other packages' `package_todo.yml` files.** +Setting `enforce_privacy` to `strict` will forbid *new* references to private constants in your package. **Violations already recorded in another package's `package_todo.yml` are tolerated**, so strict mode stops the list growing rather than requiring it to be empty. -Note: You will need to remove all existing privacy violations before setting `enforce_privacy` to `strict`. +Note: you do not need to remove existing privacy violations before setting `enforce_privacy` to `strict`. Turn it on, and any reference that is not already recorded will fail the check. To see everything the todo files are currently grandfathering, run `pks check --ignore-recorded-violations`. + +Running `pks update` will not silence strict mode either: an unrecorded strict violation is never written to a `package_todo.yml`, so it keeps failing until the reference itself is dealt with. ### Using public folders You may enforce privacy either way mentioned above and still expose a public API for your package by placing constants in the public folder, which by default is `app/public`. The constants in the public folder will be made available for use by the rest of the application. @@ -99,8 +101,7 @@ end => Ideal solution. No exceptions from rubocop and very low risk of the magic Sometimes it is desirable to only enforce privacy on a subset of constants in a package. You can do so by defining a `private_constants` list in your package.yml. Note that `enforce_privacy` must be set to `true` or `'strict'` for this to work. ### Ignore strict mode for violation coming from specific path patterns -If you want to activate `'strict'` mode on your package but have a few privacy violations you know you will deal with later, -you can set a list of patterns to exclude. +You do not need this to adopt `'strict'` mode on a package that already has violations you will deal with later: violations recorded in a `package_todo.yml` are tolerated by default. Reach for these patterns when you want to exempt a **path** instead of a recorded list. ```yaml enforce_privacy: strict @@ -110,6 +111,8 @@ strict_privacy_ignored_patterns: In this example, violations on constants of your engine referenced in those files `engines/another_engine/test/**/*` will not fail Packwerk checks. +The difference matters. A `package_todo.yml` entry grandfathers one `(constant, file)` pair, so a *new* reference from the same file still fails. A pattern here exempts the path outright, so anything those files reference later is ignored too. Prefer the todo file unless you genuinely want the whole path exempt. + ### Package Privacy violation Packwerk thinks something is a privacy violation if you're referencing a constant, class, or module defined in the private implementation (i.e. not the public folder) of another package. We care about these because we want to make sure we only use parts of a package that have been exposed as public API. diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 9604219..3222a20 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -138,7 +138,7 @@ impl<'a> CheckAllBuilder<'a> { .cloned() .collect(), strict_mode_violations: self - .build_strict_mode_violations() + .build_strict_mode_violations(recorded_violations) .into_iter() .collect(), }) @@ -231,11 +231,23 @@ impl<'a> CheckAllBuilder<'a> { } } - 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() } @@ -323,22 +335,33 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { &checkers, )?; - let strict_violations = &violations + let recorded_violations = &configuration.pack_set.all_violations; + + // Only *unlisted* strict violations make `check` fail, so only those are + // worth reporting here. Reporting recorded ones too claimed `check` would + // fail when it succeeds. Same filter as `build_strict_mode_violations`, and + // as packwerk's `unlisted_strict_mode_violations`. + let unlisted_strict_violations = &violations .iter() .filter(|v| v.identifier.strict) + .filter(|v| !recorded_violations.contains(&v.identifier.recorded_key())) .collect::>(); - if !strict_violations.is_empty() { - for violation in strict_violations { + if !unlisted_strict_violations.is_empty() { + for violation in unlisted_strict_violations { let strict_message = build_strict_violation_message(&violation.identifier); println!("{}", strict_message); } println!( "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.", - &strict_violations.len() + &unlisted_strict_violations.len() ); } - package_todo::write_violations_to_disk(configuration, violations); + package_todo::write_violations_to_disk( + configuration, + violations, + recorded_violations, + ); println!("Successfully updated package_todo.yml files!"); Ok(()) diff --git a/src/packs/package_todo.rs b/src/packs/package_todo.rs index d966a26..73e70f4 100644 --- a/src/packs/package_todo.rs +++ b/src/packs/package_todo.rs @@ -3,6 +3,7 @@ use serde::{ser::SerializeMap, Deserialize, Serialize, Serializer}; use std::collections::{BTreeMap, HashMap, HashSet}; use tracing::debug; +use super::checker::ViolationIdentifier; use super::{pack::Pack, Configuration, Violation}; #[derive(PartialEq, Debug, Eq, Deserialize, Serialize, Default, Clone)] @@ -133,6 +134,7 @@ pub fn package_todos_for_pack_name( pub fn write_violations_to_disk( configuration: &Configuration, violations: HashSet, + recorded_violations: &HashSet, ) { debug!("Starting writing violations to disk"); // First we need to group the violations by the responsible pack, which today is always the referencing pack @@ -141,7 +143,18 @@ pub fn write_violations_to_disk( let mut violations_by_responsible_pack: HashMap> = HashMap::new(); for violation in violations { - if violation.identifier.strict { + // An *unlisted* strict violation is never recorded, so `update` cannot + // be used to silence strict mode. An already-recorded one has to be + // re-written, because `check` now tolerates recorded violations in + // strict packs and `PackageTodo` is dumped wholesale from these + // entries — dropping it here would delete the record that made the + // build green and fail the next `check` with no source change in + // between. packwerk keeps the entry for the same reason, in + // `OffenseCollection#add_offense`. + if violation.identifier.strict + && !recorded_violations + .contains(&violation.identifier.recorded_key()) + { continue; } let referencing_pack_name = diff --git a/tests/check_test.rs b/tests/check_test.rs index 85f4771..74702cf 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -320,30 +320,94 @@ fn test_check_without_stale_violations() -> Result<(), Box> { } #[test] -fn test_check_with_strict_mode() -> Result<(), Box> { - // The violation here IS recorded in packs/foo/package_todo.yml, so it has to - // match its recorded entry: reported neither as a new violation nor as a - // stale todo. Strict mode still fails the run, which is what keeps this at - // exit 1, so the two strict messages are the whole of the output. +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> { + // `--ignore-recorded-violations` is the escape hatch: it still surfaces + // everything the todo file is grandfathering. + 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( "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( "packs/foo cannot have dependency violations on packs/bar because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", + )); + + common::teardown(); + 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_partially_recorded_strict_mode_violations( +) -> Result<(), Box> { + // The case that makes strict mode adoptable, and the one nothing else + // covers: one recorded violation (::Bar) and one unrecorded (::Baz) in the + // same strict pack, in the same run. Only the unrecorded one is reported, + // and the run still fails because of it. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_partially_recorded") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "packs/foo cannot have privacy violations on packs/baz because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + )) + .stdout(predicate::str::contains( + "packs/foo cannot have dependency violations on packs/baz because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", )) + .stdout(predicate::str::contains("::Baz")) + // The recorded one stays silent: no strict message, no new-violation + // report, no stale-todo line. + .stdout(predicate::str::contains("packs/bar").not()) + .stdout(predicate::str::contains("::Bar").not()) .stdout( predicate::str::contains( "There were stale violations found, please run `packs update`", ) .not(), - ) - .stdout(predicate::str::contains("violation(s) detected:").not()); + ); common::teardown(); Ok(()) @@ -351,16 +415,19 @@ fn test_check_with_strict_mode() -> Result<(), Box> { #[test] fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { + // Uses `contains_strict_violations` rather than `uses_strict_mode`: the + // latter's violation is recorded, so there is nothing left to assert against + // in the CSV. The duplicate assertion this used to carry was byte-identical + // to the one below it, so nothing is lost by dropping it. 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", )); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index eb09c23..8359432 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -55,6 +55,38 @@ pub fn delete_foobar_app_with_custom_readme() { } } +// Restores the round-trip strict-mode fixture. Its todo file records a strict +// violation, which is the state `check` tolerance depends on, so any test that +// runs `update` against it has to put it back. +#[allow(dead_code)] +pub fn set_up_uses_strict_mode_round_trip_fixture() { + let package_todo = String::from( + "\ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + \"::Bar\": + violations: + - privacy + - dependency + files: + - packs/foo/app/services/foo.rb +", + ); + + fs::write( + "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml", + package_todo, + ) + .unwrap(); +} + // In case we want our tests to call `update` or otherwise mutate the file system #[allow(dead_code)] pub fn set_up_fixtures() { diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..5003150 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/bar/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb new file mode 100644 index 0000000..dbe89a2 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/app/services/baz.rb @@ -0,0 +1,2 @@ +module Baz +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/baz/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb new file mode 100644 index 0000000..e37ea45 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb @@ -0,0 +1,9 @@ +module Foo + def calls_bar_without_stated_dependency + Bar + end + + def calls_baz_without_stated_dependency + Baz + end +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml new file mode 100644 index 0000000..553aa2d --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml @@ -0,0 +1,15 @@ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + "::Bar": + violations: + - privacy + - dependency + files: + - packs/foo/app/services/foo.rb diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml new file mode 100644 index 0000000..51f2f3b --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packwerk.yml @@ -0,0 +1,23 @@ +# See: Setting up the configuration file +# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file + +# List of patterns for folder paths to include +# include: +# - "**/*.{rb,rake,erb}" + +# List of patterns for folder paths to exclude +# exclude: +# - "{bin,node_modules,script,tmp,vendor}/**/*" + +# Patterns to find package configuration files +# package_paths: "**/" + +# List of custom associations, if any +# custom_associations: +# - "cache_belongs_to" + +# Whether or not you want the cache enabled (disabled by default) +cache: false + +# Where you want the cache to be stored (default below) +# cache_directory: 'tmp/cache/packwerk' diff --git a/tests/fixtures/uses_strict_mode_round_trip/package.yml b/tests/fixtures/uses_strict_mode_round_trip/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb new file mode 100644 index 0000000..5003150 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/app/services/bar.rb @@ -0,0 +1,2 @@ +module Bar +end diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/bar/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb new file mode 100644 index 0000000..0884fdb --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb @@ -0,0 +1,5 @@ +module Foo + def calls_bar_without_stated_dependency + Bar + end +end diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml new file mode 100644 index 0000000..553aa2d --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml @@ -0,0 +1,15 @@ +# This file contains a list of dependencies that are not part of the long term plan for the +# 'packs/foo' package. +# We should generally work to reduce this list over time. +# +# You can regenerate this file using the following command: +# +# bin/packwerk update-todo +--- +packs/bar: + "::Bar": + violations: + - privacy + - dependency + files: + - packs/foo/app/services/foo.rb diff --git a/tests/fixtures/uses_strict_mode_round_trip/packwerk.yml b/tests/fixtures/uses_strict_mode_round_trip/packwerk.yml new file mode 100644 index 0000000..51f2f3b --- /dev/null +++ b/tests/fixtures/uses_strict_mode_round_trip/packwerk.yml @@ -0,0 +1,23 @@ +# See: Setting up the configuration file +# https://github.com/Shopify/packwerk/blob/main/USAGE.md#setting-up-the-configuration-file + +# List of patterns for folder paths to include +# include: +# - "**/*.{rb,rake,erb}" + +# List of patterns for folder paths to exclude +# exclude: +# - "{bin,node_modules,script,tmp,vendor}/**/*" + +# Patterns to find package configuration files +# package_paths: "**/" + +# List of custom associations, if any +# custom_associations: +# - "cache_belongs_to" + +# Whether or not you want the cache enabled (disabled by default) +cache: false + +# Where you want the cache to be stored (default below) +# cache_directory: 'tmp/cache/packwerk' diff --git a/tests/update_test.rs b/tests/update_test.rs index e8d76ff..6b77674 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -196,6 +196,80 @@ packs/bar: Ok(()) } +#[test] +#[serial] +// This and the round-trip test below both mutate +// tests/fixtures/uses_strict_mode_round_trip, so they run in serial and each +// restores the fixture on the way out. +fn test_update_preserves_recorded_strict_violations() -> anyhow::Result<()> { + common::set_up_uses_strict_mode_round_trip_fixture(); + + let path = Path::new( + "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml", + ); + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("update") + .assert() + .success() + .stdout(predicate::str::contains( + "Successfully updated package_todo.yml files!", + )) + // The violation is recorded, so `check` tolerates it. Claiming it must + // be fixed for `check` to succeed would be false. + .stdout( + predicate::str::contains( + "These violations must be fixed for `check` to succeed.", + ) + .not(), + ); + + assert!( + path.exists(), + "update must not delete the todo file that grandfathers a recorded strict violation" + ); + let contents = std::fs::read_to_string(path)?; + assert!( + contents.contains("\"::Bar\""), + "the recorded strict violation must survive update, got:\n{}", + contents + ); + + common::set_up_uses_strict_mode_round_trip_fixture(); + Ok(()) +} + +#[test] +#[serial] +fn test_check_update_check_round_trip_with_strict_mode() -> anyhow::Result<()> { + common::set_up_uses_strict_mode_round_trip_fixture(); + + let assert_check_is_clean = || { + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("check") + .assert() + .code(0) + .stdout(predicate::str::contains("No violations detected!")); + }; + + // A routine `update` between two checks must not turn a green build red. + assert_check_is_clean(); + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("update") + .assert() + .success(); + assert_check_is_clean(); + + common::set_up_uses_strict_mode_round_trip_fixture(); + Ok(()) +} + #[test] fn test_update_with_strict_violations() -> anyhow::Result<()> { let path = Path::new( From 22bc71061cf84025915c634b2044421d7999a520 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 18 Aug 2026 10:22:13 +0100 Subject: [PATCH 3/6] Address review: document the adoption order that works The blocking item first. CHECKERS.md described the one order that does not work: it said you could turn `strict` on and let tolerance cover the existing references. Tolerance only ever matches entries already in a `package_todo.yml`, and `update` will not create them once the pack is strict, so following that note left you with `check` red and no supported way out. Reproduced on `contains_strict_violations`: check exit 1, update prints "Successfully updated" and writes no todo file, check exit 1 unchanged. The section now leads with the order that works: run `update` while the pack is still `true`, commit the todo files, then flip to `strict`. It also says what happens if you flip first, since that is the state a reader arrives in. Other CHECKERS.md corrections: - "keeps failing until the reference is dealt with" was a guarantee about `update`, not about the file. A hand-added entry does silence strict mode and `update` preserves it, so the boundary is only as strong as review of `package_todo.yml` diffs. - "a *new* reference from the same file still fails" was wrong. Multiple references to one constant in one file are a single violation, so the second matches the recorded entry. Verified: a second `Bar` reference in the recorded `foo.rb` leaves check at exit 0. It is a *different constant* from the same file that still fails. Also corrected the grandfathering unit, which is (defining pack, constant, violation type, referencing file). - `strict_privacy_ignored_patterns` is packwerk's key and pks does not implement it. It appears nowhere outside that doc block, and `Pack`'s `#[serde(flatten)]` swallows it silently, so a reader following the old text shipped an unguarded strict pack. Repointed at `enforcement_globs_ignore` with `enforcements: [privacy]`, with a note that the packwerk key has no effect. CHANGELOG corrections: - Not limited to privacy and dependencies. The filter is checker-agnostic and all five checker types can be `strict`, so layers, visibility and folder privacy relax identically. - Entries live in the *referencing* pack's todo file, not the strict pack's (`all_violations` sets `referencing_pack_name: self.name`). Now agrees with CHECKERS.md. - "could only be green with an empty todo list" overstated it; green required no *strict* entries. - Added the `update` half, which touches committed files and is the strongest reason this is breaking: `update` used to erase recorded strict entries and now retains them, so expect it in a diff or a stale-todo CI step. - "Opt out:" is now "No opt out:", since `--ignore-recorded-violations` also disables recorded-violation filtering everywhere else and surfaces every recorded violation of every type. Tagged the fence, and used a full URL for packwerk#368, which does not autolink inside a Markdown file. Tests: - New `test_update_prunes_recorded_strict_violation_once_reference_is_gone`. Preserving recorded strict violations must not make them immortal. Mutating `write_violations_to_disk` so entries are never pruned is caught by this test and by the pre-existing `test_update_with_stale_violations`, but that fixture is non-strict, so the strict path had no coverage. - Fixture restoration moved into a `RoundTripFixture` Drop guard. Trailing restores were skipped on panic; the mutation run above demonstrated it by leaving `contains_stale_violations` dirty. - `tests/common/mod.rs` writes violation types in sorted order, matching what `update` emits and `set_up_fixtures()`. The round-trip and preserve tests now assert byte equality against that constant instead of grepping for "::Bar". - Toned down the CSV test comment: after the fixture swap it exercises CSV formatting only, not strict tolerance. `recorded_key` is `pub(crate)`. Left the allocation shape alone, per your measurements. Added a note scoping out the `update` exit code, which returns success while announcing violations that must be fixed. `cargo test --no-fail-fast` 263 passed 0 failed, clippy and fmt clean. --- CHANGELOG.md | 60 +++++++++++----- CHECKERS.md | 42 ++++++++--- src/packs/checker.rs | 7 +- tests/check_test.rs | 12 ++-- tests/common/mod.rs | 63 ++++++++++++---- .../packs/foo/package_todo.yml | 2 +- tests/update_test.rs | 72 ++++++++++++++----- 7 files changed, 192 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66d736c..5f0467d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,24 +6,48 @@ #### Strict mode tolerates violations already recorded in `package_todo.yml` -`enforce_privacy: strict` and `enforce_dependencies: strict` now fail only on -references that are **not** already recorded in a `package_todo.yml`. This matches -packwerk's `unlisted_strict_mode_violations` (Shopify/packwerk#368). - -**Who is affected:** any project with a strict pack whose existing violations are -recorded in todo files. Previously `pks check` failed on every one of them, so a -strict pack could only be green with an empty todo list. - -**What changes:** pks silently produces different (smaller) results without any -configuration change. Strict packs that were red because of grandfathered -violations go green. New references into a strict pack still fail, and `pks update` -still refuses to record an unrecorded strict violation, so strict mode cannot be -silenced by running it. - -**Opt out:** there is no config flag, matching packwerk. To see everything the todo -files are grandfathering, run: - -``` +Any checker set to `strict` now fails only on references that are **not** already +recorded in a `package_todo.yml`. This matches packwerk's +`unlisted_strict_mode_violations` +([Shopify/packwerk#368](https://github.com/Shopify/packwerk/pull/368)). + +This is not limited to privacy and dependencies. The filter is checker-agnostic, +so `enforce_layers: strict`, `enforce_visibility: strict` and strict folder +privacy relax in exactly the same way. If you are using one of those to hold a +boundary hard, this affects you too. + +**Who is affected:** any project with a strict checker whose existing violations +are recorded in todo files. Note that the entries live in the **referencing** +package's `package_todo.yml`, not the strict package's. Previously `pks check` +failed on every recorded strict violation, so a strict package could only be +green with no strict entries recorded against it. + +**What changes, in `check`:** pks silently produces different (smaller) results +with no configuration change. Strict packages that were red because of +grandfathered violations go green. New references still fail, and a reference to +a different constant from an already-recorded file still fails. + +**What changes, in `update`, and this is the half that touches committed files:** +previously `update` dropped every strict violation when regenerating todo files, +and a package left with no entries had its `package_todo.yml` deleted outright. +So `update` used to erase recorded strict entries, which silently un-did the +tolerance `check` now depends on. It retains them now. Expect `update` to +*re-add* strict entries to files in your repo, and to show up in a diff or a +stale-todo CI step. `update` still refuses to record an *unrecorded* strict +violation, so strict mode cannot be adopted by running it. + +**Adopting strict mode:** run `update` while the checker is still `true`, commit +the todo files, then set it to `strict`. Flipping first does not work, because +`update` will not record violations for a package that is already strict. See +CHECKERS.md. + +**No opt out:** there is no config flag, matching packwerk. `--ignore-recorded-violations` +is *not* a drop-in replacement for the old behaviour, because it also disables +recorded-violation filtering everywhere else and will surface every recorded +violation of every type in every package. It is useful for seeing what the todo +files are grandfathering: + +```sh pks check --ignore-recorded-violations ``` diff --git a/CHECKERS.md b/CHECKERS.md index b6239cd..4399a14 100644 --- a/CHECKERS.md +++ b/CHECKERS.md @@ -13,11 +13,26 @@ enforce_privacy: true Setting `enforce_privacy` to `true` will make all references to private constants in your package a violation. -Setting `enforce_privacy` to `strict` will forbid *new* references to private constants in your package. **Violations already recorded in another package's `package_todo.yml` are tolerated**, so strict mode stops the list growing rather than requiring it to be empty. +Setting `enforce_privacy` to `strict` will forbid *new* references to private constants in your package. **Violations already recorded in the referencing package's `package_todo.yml` are tolerated**, so strict mode stops the list growing rather than requiring it to be empty. -Note: you do not need to remove existing privacy violations before setting `enforce_privacy` to `strict`. Turn it on, and any reference that is not already recorded will fail the check. To see everything the todo files are currently grandfathering, run `pks check --ignore-recorded-violations`. +#### Adopting strict mode on a package that already has violations -Running `pks update` will not silence strict mode either: an unrecorded strict violation is never written to a `package_todo.yml`, so it keeps failing until the reference itself is dealt with. +**Record the existing violations first, then flip to `strict`.** The order matters, because tolerance only ever matches entries that are *already* in a `package_todo.yml`, and `update` will not create them once the package is strict: + +```sh +# 1. while the package is still `enforce_privacy: true` +pks update + +# 2. commit the package_todo.yml files this wrote + +# 3. now set enforce_privacy: strict +``` + +Flipping to `strict` first leaves you stuck: `check` fails on the existing references, and `pks update` will not record them, so the only ways out are fixing every reference, hand-writing the todo entries, or reverting to `true`. pks matches packwerk here. + +To see everything the todo files are currently grandfathering, run `pks check --ignore-recorded-violations`. + +Once the package is strict, `pks update` will not add new entries for it: an unrecorded strict violation is never written to a `package_todo.yml`, so it keeps failing until the reference is dealt with. Note that this is a guarantee about `update`, not about the file. A hand-added entry does silence strict mode, and `update` preserves it rather than dropping it, so the boundary is only as strong as your review of `package_todo.yml` diffs. ### Using public folders You may enforce privacy either way mentioned above and still expose a public API for your package by placing constants in the public folder, which by default is `app/public`. The constants in the public folder will be made available for use by the rest of the application. @@ -100,18 +115,27 @@ end => Ideal solution. No exceptions from rubocop and very low risk of the magic ### Using specific private constants Sometimes it is desirable to only enforce privacy on a subset of constants in a package. You can do so by defining a `private_constants` list in your package.yml. Note that `enforce_privacy` must be set to `true` or `'strict'` for this to work. -### Ignore strict mode for violation coming from specific path patterns -You do not need this to adopt `'strict'` mode on a package that already has violations you will deal with later: violations recorded in a `package_todo.yml` are tolerated by default. Reach for these patterns when you want to exempt a **path** instead of a recorded list. +### Ignore strict mode for violations coming from specific path patterns +You do not need this to adopt `'strict'` mode on a package that already has violations you will deal with later: record them first and they are tolerated, as described above. Reach for a path exemption when you want to exempt a **path** rather than a recorded list. + +Use [`enforcement_globs_ignore`](#enforcement-globs-ignore) with `enforcements: [privacy]`: ```yaml enforce_privacy: strict -strict_privacy_ignored_patterns: -- engines/another_engine/test/**/* + +enforcement_globs_ignore: +- enforcements: + - privacy + ignores: + - engines/another_engine/test/**/* + reason: test files reach into engine internals ``` -In this example, violations on constants of your engine referenced in those files `engines/another_engine/test/**/*` will not fail Packwerk checks. +In this example, privacy violations on constants of your engine referenced from `engines/another_engine/test/**/*` will not fail pks checks. + +> **Note:** packwerk spells this `strict_privacy_ignored_patterns`. **pks does not implement that key**, and because `Pack` collects unknown keys via `#[serde(flatten)]` it is accepted silently and has no effect, which leaves the pack unguarded. Use `enforcement_globs_ignore` instead. -The difference matters. A `package_todo.yml` entry grandfathers one `(constant, file)` pair, so a *new* reference from the same file still fails. A pattern here exempts the path outright, so anything those files reference later is ignored too. Prefer the todo file unless you genuinely want the whole path exempt. +The two mechanisms differ in what they grandfather, so they are not interchangeable. A `package_todo.yml` entry covers one constant referenced from one file, for one violation type, so a reference to a *different* constant from that same file still fails. A path exemption covers the path outright, so anything those files reference later is ignored too. Prefer the todo file unless you genuinely want the whole path exempt. ### Package Privacy violation Packwerk thinks something is a privacy violation if you're referencing a constant, class, or module defined in the private implementation (i.e. not the public folder) of another package. We care about these because we want to make sure we only use parts of a package that have been exposed as public API. diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 3222a20..5a4a93c 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -45,7 +45,7 @@ impl ViolationIdentifier { /// 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 { + pub(crate) fn recorded_key(&self) -> Self { Self { strict: false, ..self.clone() @@ -356,6 +356,11 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.", &unlisted_strict_violations.len() ); + // Out of scope here: packwerk's `update-todo` exits non-zero in this + // state (`unlisted_strict_mode_violations.empty? && errors.empty?` as + // its result), whereas `update` returns Ok unconditionally and goes on + // to print a success line. Pre-existing, and changing the exit code is + // a separate breaking change from the filter this commit touches. } package_todo::write_violations_to_disk( configuration, diff --git a/tests/check_test.rs b/tests/check_test.rs index 74702cf..182971b 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -415,10 +415,14 @@ fn test_check_with_partially_recorded_strict_mode_violations( #[test] fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { - // Uses `contains_strict_violations` rather than `uses_strict_mode`: the - // latter's violation is recorded, so there is nothing left to assert against - // in the CSV. The duplicate assertion this used to carry was byte-identical - // to the one below it, so nothing is lost by dropping it. + // A CSV format test, and only that. It uses `contains_strict_violations` + // rather than `uses_strict_mode` because the latter's violation is recorded, + // so there is nothing left to assert against in the CSV. Note that this + // fixture ships no `package_todo.yml`, so the violation is an ordinary + // unrecorded one: this test does not exercise strict tolerance and passes + // with the recorded filter disabled. The duplicate assertion it used to + // carry was byte-identical to the one below it, so dropping it costs no + // coverage. cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/contains_strict_violations") diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 8359432..32a6882 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -55,13 +55,24 @@ pub fn delete_foobar_app_with_custom_readme() { } } -// Restores the round-trip strict-mode fixture. Its todo file records a strict -// violation, which is the state `check` tolerance depends on, so any test that -// runs `update` against it has to put it back. +// The round-trip strict-mode fixture. Its todo file records a strict violation, +// which is the state `check` tolerance depends on, and its source file provides +// the reference that entry points at. Tests here mutate both, so they restore +// via the guard below rather than a trailing call: a panicking test would skip +// a trailing restore and leave a deleted fixture in the tree, which the +// pre-commit hook makes easy to commit by accident. #[allow(dead_code)] -pub fn set_up_uses_strict_mode_round_trip_fixture() { - let package_todo = String::from( - "\ +pub const ROUND_TRIP_TODO_PATH: &str = + "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml"; + +#[allow(dead_code)] +pub const ROUND_TRIP_SOURCE_PATH: &str = + "tests/fixtures/uses_strict_mode_round_trip/packs/foo/app/services/foo.rb"; + +// Violation types are in sorted order, matching what `update` writes, so tests +// can assert byte equality against this rather than only grepping for a key. +#[allow(dead_code)] +pub const ROUND_TRIP_TODO: &str = "\ # This file contains a list of dependencies that are not part of the long term plan for the # 'packs/foo' package. # We should generally work to reduce this list over time. @@ -73,18 +84,42 @@ pub fn set_up_uses_strict_mode_round_trip_fixture() { packs/bar: \"::Bar\": violations: - - privacy - dependency + - privacy files: - packs/foo/app/services/foo.rb -", - ); +"; - fs::write( - "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml", - package_todo, - ) - .unwrap(); +#[allow(dead_code)] +pub const ROUND_TRIP_SOURCE: &str = "\ +module Foo + def calls_bar_without_stated_dependency + Bar + end +end +"; + +/// Restores the round-trip fixture when it goes out of scope, panic or not. +#[allow(dead_code)] +pub struct RoundTripFixture; + +#[allow(dead_code)] +impl RoundTripFixture { + pub fn set_up() -> Self { + Self::restore(); + Self + } + + fn restore() { + fs::write(ROUND_TRIP_TODO_PATH, ROUND_TRIP_TODO).unwrap(); + fs::write(ROUND_TRIP_SOURCE_PATH, ROUND_TRIP_SOURCE).unwrap(); + } +} + +impl Drop for RoundTripFixture { + fn drop(&mut self) { + Self::restore(); + } } // In case we want our tests to call `update` or otherwise mutate the file system diff --git a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml index 553aa2d..fd1f18f 100644 --- a/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml +++ b/tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml @@ -9,7 +9,7 @@ packs/bar: "::Bar": violations: - - privacy - dependency + - privacy files: - packs/foo/app/services/foo.rb diff --git a/tests/update_test.rs b/tests/update_test.rs index 6b77674..2ee05ee 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -198,15 +198,11 @@ packs/bar: #[test] #[serial] -// This and the round-trip test below both mutate -// tests/fixtures/uses_strict_mode_round_trip, so they run in serial and each -// restores the fixture on the way out. +// These three all mutate tests/fixtures/uses_strict_mode_round_trip, so they run +// in serial and restore through `RoundTripFixture`'s Drop rather than a trailing +// call, which a panicking test would skip. fn test_update_preserves_recorded_strict_violations() -> anyhow::Result<()> { - common::set_up_uses_strict_mode_round_trip_fixture(); - - let path = Path::new( - "tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml", - ); + let _fixture = common::RoundTripFixture::set_up(); cargo_bin_cmd!("pks") .arg("--project-root") @@ -226,25 +222,60 @@ fn test_update_preserves_recorded_strict_violations() -> anyhow::Result<()> { .not(), ); + // Byte equality, not a substring: this pins the whole file `update` writes, + // so a change that preserved the entry but mangled the rest is caught too. + let actual = std::fs::read_to_string(common::ROUND_TRIP_TODO_PATH)?; + assert_eq!(common::ROUND_TRIP_TODO, actual); + + Ok(()) +} + +#[test] +#[serial] +// The counterpart to the test above, and the one that stops the obvious +// over-correction. Preserving recorded strict violations must not make them +// immortal: once the reference is gone the entry still has to be pruned. Union +// `recorded_violations` into the write set instead of intersecting it with the +// found violations and this is the only test that fails. +fn test_update_prunes_recorded_strict_violation_once_reference_is_gone( +) -> anyhow::Result<()> { + let _fixture = common::RoundTripFixture::set_up(); + + std::fs::write( + common::ROUND_TRIP_SOURCE_PATH, + "module Foo\n def no_longer_references_bar\n :nothing\n end\nend\n", + )?; + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "There were stale violations found, please run `packs update`", + )); + + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_round_trip") + .arg("update") + .assert() + .success(); + assert!( - path.exists(), - "update must not delete the todo file that grandfathers a recorded strict violation" - ); - let contents = std::fs::read_to_string(path)?; - assert!( - contents.contains("\"::Bar\""), - "the recorded strict violation must survive update, got:\n{}", - contents + !Path::new(common::ROUND_TRIP_TODO_PATH).exists(), + "update must prune a recorded strict violation whose reference is gone, \ + otherwise `check` stays green forever for code that no longer exists" ); - common::set_up_uses_strict_mode_round_trip_fixture(); Ok(()) } #[test] #[serial] fn test_check_update_check_round_trip_with_strict_mode() -> anyhow::Result<()> { - common::set_up_uses_strict_mode_round_trip_fixture(); + let _fixture = common::RoundTripFixture::set_up(); let assert_check_is_clean = || { cargo_bin_cmd!("pks") @@ -266,7 +297,10 @@ fn test_check_update_check_round_trip_with_strict_mode() -> anyhow::Result<()> { .success(); assert_check_is_clean(); - common::set_up_uses_strict_mode_round_trip_fixture(); + // And it must leave the file exactly as it found it. + let actual = std::fs::read_to_string(common::ROUND_TRIP_TODO_PATH)?; + assert_eq!(common::ROUND_TRIP_TODO, actual); + Ok(()) } From bfec99d64ea9d68735b227ad96f87ceb19ee90cb Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 18 Aug 2026 13:23:54 +0100 Subject: [PATCH 4/6] Harden the fixture guard and fix a heading level Self-review before handing this back, three things: `RoundTripFixture::drop` no longer unwraps. It runs while a failing test is unwinding, so a filesystem error there would have been a panic during a panic, which aborts the test binary and replaces the real failure with an abort. It now reports to stderr and carries on, matching `teardown()`. `set_up` still unwraps, where panicking is informative because the test has not started. The guard is `#[must_use]` with a doc comment saying to bind it to a named variable. `let _ = RoundTripFixture::set_up()` drops it immediately and silently removes the protection, and `#[must_use]` cannot catch that spelling, so the warning has to be written down. `CHECKERS.md` demotes the new adoption-order heading from `####` to `###`. It sat between an `##` and its `###` siblings, so it skipped a level and rendered as subordinate to nothing. Verified the guard still does its job rather than assuming: forced a panic inside a guarded test, confirmed the run reports FAILED rather than aborting, and confirmed the fixture came back clean during the unwind. `cargo test --no-fail-fast` 263 passed 0 failed, clippy and fmt clean. Co-Authored-By: Claude Opus 5 --- CHECKERS.md | 2 +- tests/common/mod.rs | 28 ++++++++++++++++++++++------ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/CHECKERS.md b/CHECKERS.md index 4399a14..4ef6cf2 100644 --- a/CHECKERS.md +++ b/CHECKERS.md @@ -15,7 +15,7 @@ Setting `enforce_privacy` to `true` will make all references to private constant Setting `enforce_privacy` to `strict` will forbid *new* references to private constants in your package. **Violations already recorded in the referencing package's `package_todo.yml` are tolerated**, so strict mode stops the list growing rather than requiring it to be empty. -#### Adopting strict mode on a package that already has violations +### Adopting strict mode on a package that already has violations **Record the existing violations first, then flip to `strict`.** The order matters, because tolerance only ever matches entries that are *already* in a `package_todo.yml`, and `update` will not create them once the package is strict: diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 32a6882..fef07fa 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -100,25 +100,41 @@ end "; /// Restores the round-trip fixture when it goes out of scope, panic or not. +/// +/// Bind it to a named variable, `let _fixture = RoundTripFixture::set_up();`. +/// Binding to `let _` drops it immediately and silently removes the protection, +/// which `#[must_use]` cannot catch. #[allow(dead_code)] +#[must_use = "bind this to a named variable; `let _` drops the guard immediately"] pub struct RoundTripFixture; #[allow(dead_code)] impl RoundTripFixture { pub fn set_up() -> Self { - Self::restore(); - Self - } - - fn restore() { + // Panicking here is fine and informative: the test has not run yet. fs::write(ROUND_TRIP_TODO_PATH, ROUND_TRIP_TODO).unwrap(); fs::write(ROUND_TRIP_SOURCE_PATH, ROUND_TRIP_SOURCE).unwrap(); + Self } } impl Drop for RoundTripFixture { fn drop(&mut self) { - Self::restore(); + // Deliberately does not unwrap. This runs while a failing test is + // unwinding, and a panic here would be a panic-during-panic, which + // aborts the whole test binary and replaces the real failure with an + // abort. Report and carry on, as `teardown()` does. + for (path, contents) in [ + (ROUND_TRIP_TODO_PATH, ROUND_TRIP_TODO), + (ROUND_TRIP_SOURCE_PATH, ROUND_TRIP_SOURCE), + ] { + if let Err(err) = fs::write(path, contents) { + eprintln!( + "Failed to restore {} during teardown: {}", + path, err + ); + } + } } } From 03ce358ae3a0d3fdfccbc2037fce3123d386ed96 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 18 Aug 2026 13:59:45 +0100 Subject: [PATCH 5/6] Fix a broken glob in the docs and three wrong claims An independent pass over this branch found more than my own did. Six things. **The `enforcement_globs_ignore` example did not work.** Replacing the inert packwerk key with a live recommendation is worthless if the recommendation is also inert, which is the same argument that motivated replacing it. Measured on a scratch app: `engines/another_engine/test/**/*` gives exit 1, byte-identical to no exemption at all, because `**` requires an intervening directory and so misses `test/a_test.rb`. `engines/another_engine/test/**` gives exit 0. Corrected, and the doc now says why, since a pattern matching nothing looks exactly like no exemption. **The CHANGELOG sent readers to the wrong file.** It said entries live in the referencing package's `package_todo.yml`, "not the strict package's", stated globally. That holds for the incoming checkers only. For `enforce_dependencies` and `enforce_layers` the enforcing package *is* the referencing package, so the entry is in the strict package's own file. The counterexample is the fixture this work is built on: `uses_strict_mode/packs/foo` is `enforce_dependencies: strict` and owns the todo file holding that entry. Split by checker direction. **The CHANGELOG predicted something that cannot happen.** It said to expect `update` to *re-add* strict entries. It never adds one: an unrecorded strict violation is still not written, so the write set for strict entries is always a subset of what is committed. Verified by removing `- privacy` from a committed todo, running `update`, and getting no diff. The real change is that it stops *deleting* them. Reworded to say that in the direction it actually goes. **A comment claimed to be the only test catching a mutation.** Two tests catch it, which `bfec99d`'s message said correctly while the comment in the file did not. Corrected, and it now says what is actually unique: the other test uses a non-strict fixture, so this is the only coverage of the strict path. **Added the single-recorded-violation-type case**, which was the one review item left half done. `::Qux` is recorded for `privacy` only in a pack strict on both, and its dependency violation still fails. Every other strict fixture records both types together, so nothing pinned `violation_type` inside the comparison key. Verified the fixture is now the only strict one with a single-type entry. **Smaller:** the two new fixtures disagreed on violation ordering, now both sorted as `update` emits; the deferred `update` exit-code note is a one-line `// TODO:` matching the 13 others in `src/` rather than a five-line block; and the fixture shared between `check_test` and `update_test` now carries a comment explaining why that coupling is safe and what would break it, since `serial_test` has no `file_locks` here and cannot serialise across binaries. `cargo test --no-fail-fast` 264 passed 0 failed, clippy and fmt clean. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 32 ++++++++++++++----- CHECKERS.md | 6 ++-- src/packs/checker.rs | 7 ++-- tests/check_test.rs | 32 +++++++++++++++++++ .../packs/foo/app/services/foo.rb | 4 +++ .../packs/foo/package_todo.yml | 8 ++++- .../packs/qux/app/services/qux.rb | 2 ++ .../packs/qux/package.yml | 2 ++ tests/update_test.rs | 11 ++++++- 9 files changed, 87 insertions(+), 17 deletions(-) create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb create mode 100644 tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f0467d..f89ec35 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog + + ## Unreleased ### Breaking Changes @@ -17,10 +23,16 @@ privacy relax in exactly the same way. If you are using one of those to hold a boundary hard, this affects you too. **Who is affected:** any project with a strict checker whose existing violations -are recorded in todo files. Note that the entries live in the **referencing** -package's `package_todo.yml`, not the strict package's. Previously `pks check` -failed on every recorded strict violation, so a strict package could only be -green with no strict entries recorded against it. +are recorded in todo files. Previously `pks check` failed on every recorded strict +violation, so a strict package could only be green with no strict entries +recorded against it. + +Entries always live in the **referencing** package's `package_todo.yml`, which is +not always the strict package. For `enforce_privacy`, `enforce_visibility` and +folder privacy the enforcing package is the one being referenced, so look in the +*other* package's file. For `enforce_dependencies` and `enforce_layers` the +enforcing package is the referencing package, so the entries are in the strict +package's own file. **What changes, in `check`:** pks silently produces different (smaller) results with no configuration change. Strict packages that were red because of @@ -31,10 +43,14 @@ a different constant from an already-recorded file still fails. previously `update` dropped every strict violation when regenerating todo files, and a package left with no entries had its `package_todo.yml` deleted outright. So `update` used to erase recorded strict entries, which silently un-did the -tolerance `check` now depends on. It retains them now. Expect `update` to -*re-add* strict entries to files in your repo, and to show up in a diff or a -stale-todo CI step. `update` still refuses to record an *unrecorded* strict -violation, so strict mode cannot be adopted by running it. +tolerance `check` now depends on. It preserves them now. + +To be precise about the direction, because it is easy to read this as the +opposite: `update` never *adds* a strict entry. An unrecorded strict violation is +still not written, so strict mode cannot be adopted by running `update`. What +changed is that it stops **deleting** the entries that are already committed. If +your workflow previously relied on `update` clearing them, expect those lines to +survive where they used to disappear. **Adopting strict mode:** run `update` while the checker is still `true`, commit the todo files, then set it to `strict`. Flipping first does not work, because diff --git a/CHECKERS.md b/CHECKERS.md index 4ef6cf2..6274041 100644 --- a/CHECKERS.md +++ b/CHECKERS.md @@ -127,11 +127,13 @@ enforcement_globs_ignore: - enforcements: - privacy ignores: - - engines/another_engine/test/**/* + - engines/another_engine/test/** reason: test files reach into engine internals ``` -In this example, privacy violations on constants of your engine referenced from `engines/another_engine/test/**/*` will not fail pks checks. +In this example, privacy violations on constants of your engine referenced from anywhere under `engines/another_engine/test/` will not fail pks checks. + +Note the trailing `**` rather than `**/*`. These are gitignore-style globs, so `**` matches the whole subtree including files directly inside `test/`, whereas `**/*` requires at least one intervening directory and would silently skip `test/a_test.rb`. A pattern that matches nothing looks identical to no exemption at all, so check a new pattern against a file you expect it to cover. > **Note:** packwerk spells this `strict_privacy_ignored_patterns`. **pks does not implement that key**, and because `Pack` collects unknown keys via `#[serde(flatten)]` it is accepted silently and has no effect, which leaves the pack unguarded. Use `enforcement_globs_ignore` instead. diff --git a/src/packs/checker.rs b/src/packs/checker.rs index 5a4a93c..7c7f419 100644 --- a/src/packs/checker.rs +++ b/src/packs/checker.rs @@ -356,11 +356,8 @@ pub(crate) fn update(configuration: &Configuration) -> anyhow::Result<()> { "{} strict mode violation(s) detected. These violations must be fixed for `check` to succeed.", &unlisted_strict_violations.len() ); - // Out of scope here: packwerk's `update-todo` exits non-zero in this - // state (`unlisted_strict_mode_violations.empty? && errors.empty?` as - // its result), whereas `update` returns Ok unconditionally and goes on - // to print a success line. Pre-existing, and changing the exit code is - // a separate breaking change from the filter this commit touches. + // TODO: packwerk's `update-todo` exits non-zero here; `update` returns + // Ok and prints a success line. Pre-existing, separate breaking change. } package_todo::write_violations_to_disk( configuration, diff --git a/tests/check_test.rs b/tests/check_test.rs index 182971b..46256f1 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -413,6 +413,38 @@ fn test_check_with_partially_recorded_strict_mode_violations( Ok(()) } +#[test] +fn test_check_with_single_recorded_violation_type_in_strict_pack( +) -> Result<(), Box> { + // `::Qux` is recorded for `privacy` only, while `packs/qux` is strict on both + // privacy and dependencies. Recording one type must tolerate only that type, + // so the dependency violation still fails the run. + // + // This pins `violation_type` inside the recorded-comparison key. A refactor + // that normalized it away, the way `strict` is normalized by + // `ViolationIdentifier::recorded_key`, would silence both types from a + // single-type entry, and every other strict fixture records privacy and + // dependency together so nothing else would catch it. + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/uses_strict_mode_partially_recorded") + .arg("check") + .assert() + .code(1) + .stdout(predicate::str::contains( + "packs/foo cannot have dependency violations on packs/qux because strict mode is enabled for dependency violations in the enforcing pack's package.yml file", + )) + .stdout( + predicate::str::contains( + "packs/foo cannot have privacy violations on packs/qux because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", + ) + .not(), + ); + + common::teardown(); + Ok(()) +} + #[test] fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { // A CSV format test, and only that. It uses `contains_strict_violations` diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb index e37ea45..7435e66 100644 --- a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/app/services/foo.rb @@ -6,4 +6,8 @@ def calls_bar_without_stated_dependency def calls_baz_without_stated_dependency Baz end + + def calls_qux_without_stated_dependency + Qux + end end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml index 553aa2d..8afa101 100644 --- a/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/foo/package_todo.yml @@ -9,7 +9,13 @@ packs/bar: "::Bar": violations: - - privacy - dependency + - privacy + files: + - packs/foo/app/services/foo.rb +packs/qux: + "::Qux": + violations: + - privacy files: - packs/foo/app/services/foo.rb diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb new file mode 100644 index 0000000..c528918 --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/app/services/qux.rb @@ -0,0 +1,2 @@ +module Qux +end diff --git a/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml new file mode 100644 index 0000000..f3c2aff --- /dev/null +++ b/tests/fixtures/uses_strict_mode_partially_recorded/packs/qux/package.yml @@ -0,0 +1,2 @@ +enforce_privacy: strict +enforce_dependencies: strict diff --git a/tests/update_test.rs b/tests/update_test.rs index 2ee05ee..04d0fda 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -236,7 +236,9 @@ fn test_update_preserves_recorded_strict_violations() -> anyhow::Result<()> { // over-correction. Preserving recorded strict violations must not make them // immortal: once the reference is gone the entry still has to be pruned. Union // `recorded_violations` into the write set instead of intersecting it with the -// found violations and this is the only test that fails. +// found violations and this test fails, as does the pre-existing +// `test_update_with_stale_violations`. That one uses a non-strict fixture, so +// this is the only coverage of the strict path. fn test_update_prunes_recorded_strict_violation_once_reference_is_gone( ) -> anyhow::Result<()> { let _fixture = common::RoundTripFixture::set_up(); @@ -305,6 +307,13 @@ fn test_check_update_check_round_trip_with_strict_mode() -> anyhow::Result<()> { } #[test] +// Shares `contains_strict_violations` with `check_test.rs`, which reads it. That +// is safe only because of what this test asserts: the committed fixture has no +// `package_todo.yml`, the `remove_file` below is defensive, and the assertion is +// that `update` does not create one. So the fixture is invariant across this +// test. If that assertion ever inverts, give this test its own fixture copy, +// because `serial_test` here has no `file_locks` feature and so cannot serialise +// across test binaries. fn test_update_with_strict_violations() -> anyhow::Result<()> { let path = Path::new( "tests/fixtures/contains_strict_violations/packs/foo/package_todo.yml", From 7ba01360c1daec9777dd05e9edd5410a192c8e22 Mon Sep 17 00:00:00 2001 From: Mattia Giuffrida Date: Tue, 18 Aug 2026 14:17:08 +0100 Subject: [PATCH 6/6] Keep the yaml_serde entry unreleased after merging main `origin/main` moved while this branch was in review, and the CHANGELOG restructure silently captured someone else's entry. `832309c` appended `### Internal` / `Replaced serde_yaml with yaml_serde` to the same `## Unreleased` section this branch retitles to `## 0.4.0`. The branch edits the heading at the top, main appended at the bottom, so there is no textual overlap and `git merge-tree` reports a clean merge. The merged file then files a genuinely unreleased entry under a tag that was cut before it existed. Confirmed it is unreleased: `git merge-base --is-ancestor 832309c v0.4.0` is false. Worse, the release-notes comment added in 03ce358 compounds it. Whoever retitles `## Unreleased` to `## 0.5.0` would carry the strict-mode entry into the notes and leave yaml_serde out of every release. That comment was written to prevent exactly this and could not see it, because the change arrived through the base rather than through the diff. Merged `0ccf146` and moved the `### Internal` block back under `## Unreleased`. Merge rather than rebase deliberately: rebasing rewrites all five commits and would outdate the fifteen inline review comments on this PR mid-review. Happy to rebase to a linear history before merge if that is preferred. Three corrections that are mine: **The glob note derived pks behaviour from gitignore, and they disagree.** The correction in 03ce358 was right about the behaviour and wrong about the cause. `git check-ignore` treats `test/**` and `test/**/*` identically, both matching `test/a_test.rb`; pks matches only the nested path with `**/*`. Measured both. The mechanism is `fnmatch_regex2::glob_to_regex` (`ignored.rs:19`), not gitignore, so a reader applying the stated rule predicts the opposite of what pks does. Names the real mechanism now and warns against reasoning from gitignore. **The new test's comment repeated the over-claim the same commit had just fixed elsewhere.** It said nothing else would catch a `violation_type` normalization. Untrue: collapsing it inside `recorded_key` fails eight tests, three of them strict-path. What is actually unique is narrower and measured: collapsing `violation_type` in the strict filter *specifically* fails this test and nothing else in the suite. The comment now says that, with the coarse-mutation caveat, so the next reader is not misled into thinking the coarse result proves anything. **A sibling comment had gone stale.** `test_check_with_partially_recorded_strict_mode_violations` still described a two-constant fixture after `::Qux` was added to it. Also recorded that cargo runs test binaries sequentially, so the shared-fixture coupling is latent, and that `cargo-nextest` would make it live without anyone touching the test. `cargo test --no-fail-fast` 264 passed 0 failed on the merged tree, clippy and fmt clean. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 28 ++++++++++++++-------------- CHECKERS.md | 2 +- tests/check_test.rs | 21 +++++++++++++++------ tests/update_test.rs | 5 ++++- 4 files changed, 34 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4c3c877..2ccec80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,6 +67,20 @@ files are grandfathering: pks check --ignore-recorded-violations ``` +### Internal + +#### Replaced `serde_yaml` with `yaml_serde` + +`serde_yaml` was discontinued in March 2024, and its `unsafe-libyaml` backend has +been unreleased since. pks now depends on +[`yaml_serde`](https://github.com/yaml/yaml-serde), the YAML organization's +maintained fork, which is backed by `libyaml-rs` from the same org. + +`yaml_serde` is an API-compatible fork whose only substantive changes are `no_std` +support and lint cleanups, so this is behavior-preserving: the bytes pks writes to +`package.yml` and `package_todo.yml` are unchanged, as are its YAML parse error +messages. No action is required. + ## 0.4.0 ### Breaking Changes @@ -90,17 +104,3 @@ excluded, and the old behavior (analyze everything) was rarely desired. ```yaml respect_gitignore: false ``` - -### Internal - -#### Replaced `serde_yaml` with `yaml_serde` - -`serde_yaml` was discontinued in March 2024, and its `unsafe-libyaml` backend has -been unreleased since. pks now depends on -[`yaml_serde`](https://github.com/yaml/yaml-serde), the YAML organization's -maintained fork, which is backed by `libyaml-rs` from the same org. - -`yaml_serde` is an API-compatible fork whose only substantive changes are `no_std` -support and lint cleanups, so this is behavior-preserving: the bytes pks writes to -`package.yml` and `package_todo.yml` are unchanged, as are its YAML parse error -messages. No action is required. diff --git a/CHECKERS.md b/CHECKERS.md index 6274041..1333f39 100644 --- a/CHECKERS.md +++ b/CHECKERS.md @@ -133,7 +133,7 @@ enforcement_globs_ignore: In this example, privacy violations on constants of your engine referenced from anywhere under `engines/another_engine/test/` will not fail pks checks. -Note the trailing `**` rather than `**/*`. These are gitignore-style globs, so `**` matches the whole subtree including files directly inside `test/`, whereas `**/*` requires at least one intervening directory and would silently skip `test/a_test.rb`. A pattern that matches nothing looks identical to no exemption at all, so check a new pattern against a file you expect it to cover. +Note the trailing `**` rather than `**/*`. `**` matches the whole subtree including files directly inside `test/`, whereas `**/*` requires at least one intervening directory and so silently skips `test/a_test.rb`. Do not reason about these from gitignore: `git check-ignore` treats `test/**` and `test/**/*` identically, and pks does not, because matching goes through `fnmatch_regex2::glob_to_regex` (`src/packs/ignored.rs`) rather than gitignore semantics. A pattern that matches nothing looks identical to no exemption at all, so check a new pattern against a file you expect it to cover. > **Note:** packwerk spells this `strict_privacy_ignored_patterns`. **pks does not implement that key**, and because `Pack` collects unknown keys via `#[serde(flatten)]` it is accepted silently and has no effect, which leaves the pack unguarded. Use `enforcement_globs_ignore` instead. diff --git a/tests/check_test.rs b/tests/check_test.rs index 46256f1..ca3c02c 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -381,10 +381,12 @@ fn test_check_with_unrecorded_strict_mode_violation( #[test] fn test_check_with_partially_recorded_strict_mode_violations( ) -> Result<(), Box> { - // The case that makes strict mode adoptable, and the one nothing else - // covers: one recorded violation (::Bar) and one unrecorded (::Baz) in the - // same strict pack, in the same run. Only the unrecorded one is reported, - // and the run still fails because of it. + // The case that makes strict mode adoptable: a recorded violation (::Bar) + // and an unrecorded one (::Baz) in the same strict pack, in the same run. + // Only the unrecorded one is reported, and the run still fails because of + // it. This fixture also carries ::Qux with a single-type entry, read by + // `test_check_with_single_recorded_violation_type_in_strict_pack`, so + // ::Qux's dependency message is expected in this output too. cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode_partially_recorded") @@ -423,8 +425,15 @@ fn test_check_with_single_recorded_violation_type_in_strict_pack( // This pins `violation_type` inside the recorded-comparison key. A refactor // that normalized it away, the way `strict` is normalized by // `ViolationIdentifier::recorded_key`, would silence both types from a - // single-type entry, and every other strict fixture records privacy and - // dependency together so nothing else would catch it. + // single-type entry. + // + // Measured, because the obvious way to say this overstates it: collapsing + // `violation_type` in the strict filter *specifically* fails exactly this + // test and nothing else in the suite. Collapsing it everywhere in + // `recorded_key` fails eight tests, since it also breaks reportable and + // stale comparisons, so that coarser mutation proves nothing about this + // one. Every other strict fixture records privacy and dependency together, + // which is why the narrow case needs its own coverage. cargo_bin_cmd!("pks") .arg("--project-root") .arg("tests/fixtures/uses_strict_mode_partially_recorded") diff --git a/tests/update_test.rs b/tests/update_test.rs index 04d0fda..5ec0ecf 100644 --- a/tests/update_test.rs +++ b/tests/update_test.rs @@ -313,7 +313,10 @@ fn test_check_update_check_round_trip_with_strict_mode() -> anyhow::Result<()> { // that `update` does not create one. So the fixture is invariant across this // test. If that assertion ever inverts, give this test its own fixture copy, // because `serial_test` here has no `file_locks` feature and so cannot serialise -// across test binaries. +// across test binaries. Cargo runs test binaries sequentially, so the coupling is +// latent rather than live, but that is cargo's behaviour and not a property of +// this design: `cargo-nextest` runs tests from different binaries concurrently, +// so adopting it would make this live without anyone touching this test. fn test_update_with_strict_violations() -> anyhow::Result<()> { let path = Path::new( "tests/fixtures/contains_strict_violations/packs/foo/package_todo.yml",