Match recorded violations in strict packs

[iMacTia]

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. Comparisons now go through recorded_key(), which zeroes the flag.

That alone fixes the two unambiguous symptoms: a recorded violation in a strict pack was reported as new, and its todo entry was reported as stale.

The third change is a policy one, and separable if you'd rather not take it. build_strict_mode_violations now also skips recorded violations, matching packwerk's unlisted_strict_mode_violations (Shopify/packwerk#368) and what #166 described as out of scope at the time. --ignore-recorded-violations still surfaces them.

Tests

test_check_with_strict_mode was asserting the old behaviour against a fixture whose todo file already recorded the violation, so it now asserts tolerance and is renamed to say so. Two cases added to pin the parts that keep strict mode useful: an unrecorded strict violation still exits 1, and --ignore-recorded-violations still reports recorded ones. The CSV test moved to contains_strict_violations, which has no todo file, so it still has output to assert against.

cargo test, cargo clippy --all-targets --all-features -- -Dwarnings and cargo fmt --all -- --check all pass.

Effect on a real app

15.6k files, 65 packs, two of them strict with 66 recorded todo entries. Stock 0.4.0 on that tree: 185 strict violations and 135 stale todos. With this patch, both go to zero for the strict packs.

[iMacTia]

@technicalpickles @dduugg @martinemde I know it has only been 3 days, but I was hoping to get a quick turnaround on this one which unblocks a spike on my current sprint at work 🙏

If we can't get this merged and released in a timely fashion, I completely understand, please just le me know so I can work off a fork

[dduugg]

Thanks for the clear writeup, and for flagging up front that Part B is separable.

Part A is a real bug and the diagnosis looks right to me. package_todo.yml has no field for strict, so Pack::all_violations() hardcoding strict: false (pack.rs:195) isn't something to fix upstream; the asymmetry comes from the file format. I checked every site in src/ that compares or hashes a ViolationIdentifier, and all three are covered. I also confirmed the effect: main prints "There were stale violations found" on uses_strict_mode, this branch doesn't.

Part B matches packwerk on check. I checked the source rather than going off the PR description:

# lib/packwerk/offense_collection.rb
def unlisted_strict_mode_violations
  strict_mode_violations.reject { |offense| already_listed?(offense) }
end

check_command.rb uses that for both display and exit status, so your reading of Shopify/packwerk#368 is accurate.

The mixed case works too, which is the behavior that makes strict mode adoptable: I added a second, unrecorded reference next to the recorded ::Bar and only the new one gets reported.

I left inline notes on the specifics. One blocking issue on update (see the comment on build_strict_mode_violations), plus the docs and release items below.

Blocking (docs): CHECKERS.md says the opposite

CHECKERS.md:16-18:

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.

Note: You will need to remove all existing privacy violations before setting enforce_privacy to strict.

Both sentences become false under Part B and need rewriting in whichever PR carries it. CHECKERS.md:101-107 also presents strict_privacy_ignored_patterns as the way to "activate 'strict' mode on your package but have a few privacy violations you know you will deal with later." Part B now covers that case by default, so the docs should say when you'd reach for each.

Should-fix

update's summary message is wrong now. checker.rs:334-348 filters on .identifier.strict with no recorded filter, so in my run update printed "These violations must be fixed for check to succeed" for 2 violations while check said No violations detected!. It had also just deleted the record that made the message false. packwerk uses unlisted_strict_mode_violations for the equivalent message; same filter applies here.

CHANGELOG entry for Part B, in the respect_gitignore who's-affected / what-changes / opt-out format. Same class of change: silent, no config needed to trigger it, different results from the same tree. Two mechanical things: ## Unreleased is stale, since 2fe98b7 is an ancestor of v0.4.0 and everything under that heading already shipped, and at 0.4.0 pre-1.0 a breaking change wants 0.5.0.

Nit

pks update exits 0 where packwerk's update-todo exits 1 when unlisted strict violations exist. Pre-existing and separate from this PR.

Suggestion: take Part A now, split Part B

Part A is a straightforward bug fix, independently useful, and it covers most of what's hurting you (135 stale todos to 0). Part B still needs the write_violations_to_disk preservation fix, the corrected update message, and the CHECKERS.md and CHANGELOG updates, and Part A has to land first for the preservation fix anyway. Given your timeline, splitting probably gets you unblocked sooner than working through Part B here.

One thing to watch when you split: uses_strict_mode is the fixture whose meaning changes, and Part A on its own still changes its output. check should exit 1 with just the two strict messages, no "stale violations" line and no new-violation report. So test_check_with_strict_mode still needs an update in the Part A PR, just a different one than here.

Part B's follow-up would then carry: recorded-strict preservation in write_violations_to_disk, the update message filter, the CHECKERS.md rewrite, the CHANGELOG entry, a test for the listed-strict case on update, and a check -> update -> check test on uses_strict_mode.

On gating Part B behind a config option, I'd say don't. packwerk made it the default with no opt-out, --ignore-recorded-violations already covers the escape hatch, and you've wired it through build_strict_mode_violations the same way build_reportable_violations does it. A pks-only knob would cut against the parity goal.

Verification

cargo test, cargo clippy --all-targets --all-features -- -Dwarnings, and cargo fmt --all -- --check pass on this branch. One unrelated failure, test_gitignore_negation_patterns, reproduces the same way on main (local global gitignore with *.log).

[dduugg]

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.

[dduugg]

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.

[dduugg]

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.

[dduugg]

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.

[dduugg]

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).

[dduugg]

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.