Tolerate recorded violations in strict mode (Part B of #43)

CHANGELOG.md

@@ -1,27 +1,70 @@
 # Changelog
 
+<!-- Whoever cuts the next release: retitle `## Unreleased` below to the version
+     being tagged. `.github/workflows/release.yml` builds the GitHub Release body
+     from cargo-dist's `announcement_github_body`, which matches a heading against
+     the tag, so an entry left under `## Unreleased` silently misses the release
+     notes. Pre-1.0, the breaking change below wants 0.5.0 rather than 0.4.x. -->
+
 ## Unreleased
 
 ### Breaking Changes
 
-#### `respect_gitignore` defaults to `true`
+#### Strict mode tolerates violations already recorded in `package_todo.yml`
 
-pks now respects `.gitignore` files by default. Files and directories matched by
-`.gitignore`, `.git/info/exclude`, or your global gitignore (`core.excludesFile`)
-are excluded from analysis.
+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)).
 
-**Who is affected:** any project that previously relied on pks analyzing gitignored
-paths — for example, vendored code checked into `.gitignore`-excluded directories,
-or generated files that matter for boundary checking.
+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.
 
-**What changes:** pks silently produces different (smaller) results without any
-configuration change. This is intentional: most projects want gitignored files
-excluded, and the old behavior (analyze everything) was rarely desired.
+**Who is affected:** any project with a strict checker whose existing violations
+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.
 
-**Opt out:** add the following to `packwerk.yml` to restore the previous behavior:
+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.
 
-```yaml
-respect_gitignore: false
+**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 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
+`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
 ```
 
 ### Internal
@@ -37,3 +80,27 @@ maintained fork, which is backed by `libyaml-rs` from the same org.
 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
+
+#### `respect_gitignore` defaults to `true`
+
+pks now respects `.gitignore` files by default. Files and directories matched by
+`.gitignore`, `.git/info/exclude`, or your global gitignore (`core.excludesFile`)
+are excluded from analysis.
+
+**Who is affected:** any project that previously relied on pks analyzing gitignored
+paths — for example, vendored code checked into `.gitignore`-excluded directories,
+or generated files that matter for boundary checking.
+
+**What changes:** pks silently produces different (smaller) results without any
+configuration change. This is intentional: most projects want gitignored files
+excluded, and the old behavior (analyze everything) was rarely desired.
+
+**Opt out:** add the following to `packwerk.yml` to restore the previous behavior:
+
+```yaml
+respect_gitignore: false
+```

CHECKERS.md

@@ -13,9 +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 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 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 will need to remove all existing privacy violations before setting `enforce_privacy` to `strict`.
+### 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:
+
+```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.
@@ -98,17 +115,29 @@ 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
-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.
+### 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 anywhere under `engines/another_engine/test/` will not fail pks checks.
+
+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.
+
+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.

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(crate) 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 +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(),
         })
@@ -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<ViolationIdentifier>,
     ) -> anyhow::Result<Vec<&'a ViolationIdentifier>> {
-        let found_violation_identifiers: HashSet<&ViolationIdentifier> = self
+        let found_violation_identifiers: HashSet<ViolationIdentifier> = 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<ViolationIdentifier>,
         todo_violation_identifier: &ViolationIdentifier,
     ) -> bool {
         let violation_path_exists =
@@ -210,11 +231,23 @@ impl<'a> CheckAllBuilder<'a> {
         }
     }
 
-    fn build_strict_mode_violations(&self) -> Vec<Violation> {
+    /// 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<ViolationIdentifier>,
+    ) -> Vec<Violation> {
         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()
     }
@@ -302,22 +335,35 @@ 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::<Vec<&Violation>>();
-    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()
         );
+        // 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, violations);
+    package_todo::write_violations_to_disk(
+        configuration,
+        violations,
+        recorded_violations,
+    );
     println!("Successfully updated package_todo.yml files!");
 
     Ok(())

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<Violation>,
+    recorded_violations: &HashSet<ViolationIdentifier>,
 ) {
     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<String, Vec<Violation>> =
         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 =