Skip to content

Match recorded violations in strict packs #43

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iMacTia wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Match recorded violations in strict packs #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 36 additions & 7 deletions src/packs/checker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,19 @@ pub struct ViolationIdentifier {
pub referencing_pack_name: String,
pub defining_pack_name: String,
}

impl ViolationIdentifier {
/// `strict` describes how a violation should be treated, not which violation
/// it is, and `package_todo.yml` has nowhere to record it, so recorded
/// violations are always rebuilt with `strict: false`. Compare through this
/// so a violation in a strict pack can still match its recorded entry.
pub fn recorded_key(&self) -> Self {

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design note, non-blocking, and fine to defer to a follow-up.

Consider moving strict off ViolationIdentifier and onto Violation instead of normalizing at comparison time. Your comment here already says why: strict describes how a violation should be treated, not which violation it is. The doc comment just below at checker.rs:55-64 sets the same rule for source_location, that the identifier defines sameness for comparison against package_todo.yml, "which doesn't store line/column." strict isn't stored there either.

The change is mechanical. Every reader of .identifier.strict (json.rs:56,90; csv.rs:12,53; package_todo.rs:144) already has a full &Violation, and build_strict_violation_message never reads the field. Constructors are pack.rs:195, which is where #41 starts and which then stops having to invent strict: false, plus pack_checker.rs:180 and four test constructors. You'd get all three comparison sites back to plain contains(&v.identifier), #41 becomes impossible to express instead of something a future call site has to remember to guard, and the extra allocations go away.

One alternative to skip: excluding strict from a manual PartialEq/Hash. Violation's derived Eq/Hash delegate to the identifier, and get_all_violations dedupes into a HashSet<Violation>, so making strict: true equal strict: false lets an insert keep the wrong flag, which build_strict_mode_violations then filters on.

recorded_key() is correct as written. This is about where the field lives, not about a bug.

Self {
strict: false,
..self.clone()
}
}
}
/// A violation combines an identifier with display metadata.
///
/// `source_location` is intentionally separate from `ViolationIdentifier` because:
Expand Down Expand Up @@ -124,7 +137,7 @@ impl<'a> CheckAllBuilder<'a> {
.cloned()
.collect(),
strict_mode_violations: self
.build_strict_mode_violations()
.build_strict_mode_violations(recorded_violations)
.into_iter()
.collect(),
})
Expand All @@ -142,7 +155,10 @@ impl<'a> CheckAllBuilder<'a> {
self.found_violations
.violations
.iter()
.filter(|v| !recorded_violations.contains(&v.identifier))
.filter(|v| {
!recorded_violations
.contains(&v.identifier.recorded_key())
})
.collect()
};
reportable_violations
Expand All @@ -152,11 +168,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

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: this moves from HashSet<&ViolationIdentifier> to an owned HashSet<ViolationIdentifier>, so it now clones 4 Strings per found violation rather than copying a pointer. Small next to parsing 15.6k files, so fine to leave.

If you want the cheaper version, a borrowed key tuple that excludes strict avoids the allocations entirely. Moving strict onto Violation (see my note on recorded_key) would also let this go back to borrowing.

.found_violations
.violations
.par_iter()
.map(|v| &v.identifier)
.map(|v| v.identifier.recorded_key())
.collect();
let relative_files = self
.found_violations
Expand Down Expand Up @@ -198,23 +214,36 @@ impl<'a> CheckAllBuilder<'a> {

fn is_stale_violation(
relative_files: &HashSet<&str>,
found_violation_identifiers: &HashSet<&ViolationIdentifier>,
found_violation_identifiers: &HashSet<ViolationIdentifier>,
todo_violation_identifier: &ViolationIdentifier,
) -> bool {
let violation_path_exists =
relative_files.contains(todo_violation_identifier.file.as_str());
if violation_path_exists {
!found_violation_identifiers.contains(todo_violation_identifier)
!found_violation_identifiers
.contains(&todo_violation_identifier.recorded_key())

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this call does nothing. todo_violation_identifier comes from pack_set.all_violations, which is always built with strict: false (pack.rs:195), so recorded_key() clones 4 Strings per recorded violation and changes nothing. I reverted just this call and the whole suite stays green.

The found-side .map(|v| v.identifier.recorded_key()) above is the one doing the work. Either drop this one or add a comment saying recorded identifiers arrive already normalized, so a future reader doesn't assume it matters.

} else {
true // The todo violation references a file that no longer exists
}
}

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| {

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking: pks update erases the entries this now depends on.

write_violations_to_disk drops every strict violation when regenerating todo files:

// src/packs/package_todo.rs:144
if violation.identifier.strict {
    continue;
}

That line is pre-existing and untouched here, but this filter is what starts depending on those todo entries. On main the asymmetry was invisible, because check failed either way. Now, on tests/fixtures/uses_strict_mode, with no source change in between:

$ pks check
No violations detected!                      # exit 0, working as intended

$ pks update
2 strict mode violation(s) detected. These violations must be fixed for `check` to succeed.
Successfully updated package_todo.yml files!  # exit 0
# packs/foo/package_todo.yml is now DELETED. Both of that pack's recorded
# violations are strict, so nothing is written for foo and the None branch
# hits delete_package_todo_from_disk.

$ pks check
2 violation(s) detected: ...
packs/foo cannot have privacy violations on packs/bar because strict mode is enabled ...
                                             # exit 1

A routine pks update un-grandfathers every recorded violation in a strict pack and turns a green build red. Your real-app numbers hold until someone runs update.

I'd call this blocking rather than a pre-existing quirk to port later, because packwerk does the opposite here:

# lib/packwerk/offense_collection.rb#add_offense
if strict_mode_violation?(offense)
  add_to_package_todo(offense) if already_listed
  strict_mode_violations << offense
else
  add_to_package_todo(offense)
end

An unlisted strict violation never gets added, so you can't silence strict mode by running update-todo. An already-listed one gets re-added, and that re-add is what keeps the entry in the file, since PackageTodo#dump writes new_entries wholesale. packwerk protects the state its own check tolerance reads. pks treats both cases the same.

Suggested fix: in write_violations_to_disk, drop only the unlisted strict violations. The recorded set is already at configuration.pack_set.all_violations, the same source CheckAllBuilder uses, and the comparison needs recorded_key(), so Part A comes first.

This doesn't require changing tests/update_test.rs:199-225. That test runs against contains_strict_violations, which ships no package_todo.yml and gets remove_file'd first, so its violation is unlisted, and "todo should not be created for strict violations" is what packwerk does in that case. The already-listed case has no test, which is how this stayed hidden.

self.configuration.ignore_recorded_violations
|| !recorded_violations
.contains(&v.identifier.recorded_key())
})
.cloned()
.collect()
}
Expand Down
43 changes: 40 additions & 3 deletions tests/check_test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,11 +320,31 @@ fn test_check_without_stale_violations() -> Result<(), Box<dyn Error>> {
}

#[test]
fn test_check_with_strict_mode() -> Result<(), Box<dyn Error>> {
fn test_check_with_recorded_strict_mode_violation() -> Result<(), Box<dyn Error>>
{
// 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!"));

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test gap worth pinning: a strict pack with some recorded and some unrecorded violations in the same run.

I checked and the behavior is right. Adding a second, unrecorded reference alongside the recorded ::Bar in this fixture reports only the new one. That's the case that makes strict mode adoptable, and nothing in the suite covers it today, so a regression here would be silent.

Also worth a check -> update -> check test on this fixture, asserting the second check is still clean. That's the round trip that currently breaks (see my comment on build_strict_mode_violations).


common::teardown();
Ok(())
}

#[test]
fn test_check_with_recorded_strict_mode_violation_ignoring_todo(
) -> Result<(), Box<dyn Error>> {
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(
Expand All @@ -338,18 +358,35 @@ fn test_check_with_strict_mode() -> Result<(), Box<dyn Error>> {
Ok(())
}

#[test]
fn test_check_with_unrecorded_strict_mode_violation(
) -> Result<(), Box<dyn Error>> {
// No package_todo.yml entry for this one, so strict mode must still fail.
cargo_bin_cmd!("pks")
.arg("--project-root")
.arg("tests/fixtures/contains_strict_violations")
.arg("check")
.assert()
.code(1)
.stdout(predicate::str::contains(
"packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file",
));

common::teardown();
Ok(())
}

#[test]
fn test_check_with_strict_mode_output_csv() -> Result<(), Box<dyn Error>> {
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",

@dduugg dduugg Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No coverage lost here, just flagging why for the record: the removed line is byte-identical to the one kept below it, so this drops a duplicate assertion.

The duplication was pointing at something real, though. Unrecorded strict violations get reported twice, since build_reportable_violations doesn't filter on .strict and the formatters concatenate both sets, which is why -o csv emits the same row twice on this fixture. It reproduces on main, so it's pre-existing and not yours to fix here.

));
Expand Down