Match recorded violations in strict packs (Part A)

[iMacTia]

Part of #41. Deliberately not a closing reference: this is Part A of that issue, and #41 stays open
until #45 lands. Issue #41 lists three broken comparisons and calls the third a policy question; this
PR fixes the reported-as-new and reported-as-stale halves, and #45 fixes the strict-mode report, which
is the half that makes package_todo.yml actually take effect in a strict pack.

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.

Now Part A only

@dduugg suggested splitting this and I have taken that. The policy change, which makes check tolerate recorded strict violations, has moved to #45 together with the write_violations_to_disk preservation fix, the update message filter, the CHECKERS.md rewrite, the CHANGELOG entry and the extra tests.

Changes from your review

• Dropped the recorded_key() call in is_stale_violation. You were right that it does nothing, since todo_violation_identifier is already built with strict: false. There is a comment there now so a future reader does not assume it matters.
• test_check_with_strict_mode is updated for Part A rather than for the old combined behaviour, as you described. uses_strict_mode still exits 1 from strict mode, but reports neither a new violation nor a stale todo. It fails without the source fix, which I checked by reverting the source and keeping the test.

On the real-app numbers

The 185 strict violations and 135 stale todos in the original description were measured on the combined change, so I am not restating them for this PR. Part A is the half that stops the double reporting, as new and as stale. The strict-violation count needs #45.

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

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

[iMacTia]

Taken, and it went to #45 with the policy change, since that is the PR that starts depending on those entries. write_violations_to_disk now drops only the unlisted strict violations, which is the fix you suggested, comparing through recorded_key() against pack_set.all_violations. You have already confirmed the round trip there on uses_strict_mode_partially_recorded.

Nothing left to do on this branch, and the reason is specific rather than "it is Part B's problem". build_strict_mode_violations still has no recorded filter here, so a recorded strict violation still exits 1, and the sequence never starts from the green state it needs. Measured on uses_strict_mode with this branch built:

check    exit 1    two strict messages, no new-violation report, no stale line
update   exit 0    still deletes packs/foo/package_todo.yml
check    exit 1    the same two strict messages, plus a new-violation report

So update still erases the entry on Part A, exactly as on main, but the exit status is 1 either side of it. The only difference the erase makes here is that the third check regains the new-violation report, because the entry that used to match is gone. The green to red flip needs Part B's tolerance, and Part B carries the fix.

Sent with Claude Code

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

[iMacTia]

Agreed on the reasoning, and the part I find most persuasive is that it makes #41 impossible to express instead of something a future call site has to remember to guard. Not taking it here though, for two reasons.

It is a wider diff than the fix. On this branch .identifier.strict is read at checker.rs:238 and :328, csv.rs:12 and :53, json.rs:56 and :90, and package_todo.rs:144, and it is set at pack.rs:195, pack_checker.rs:180, and the test builders in common_test.rs, pack.rs and text.rs. All mechanical, as you say, but Part A is the half meant to merge and release on its own, and this would put a refactor in front of it.

The second reason is that #45 does not thin those readers out, it thickens two of them. write_violations_to_disk now reads .identifier.strict together with a recorded_key() lookup, and the update summary at checker.rs:346 does the same. The count of readers is unchanged, there is just more logic in the two that matter. So doing the move now means either redoing it after #45 lands or colliding with the exact lines #45 changes, in a PR you are mid-review on.

My preference is a follow-up once both are in. Happy to write it up as an issue with your reasoning so it does not get buried in a merged PR, if you would rather have it tracked than take my word that I will get to it.

Sent with Claude Code

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

[iMacTia]

Done in e198118. The call is gone, and there is a doc comment on is_stale_violation saying the recorded side arrives already normalised, since without it the asymmetry with the found side reads like an oversight. found_violation_identifiers went from HashSet<&ViolationIdentifier> to an owned HashSet<ViolationIdentifier> as part of that, which is the subject of your next note.

Sent with Claude Code

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

[iMacTia]

Leaving it, on your own numbers. You measured this on #45 and came down the same way: 5.87ms and 5.65MiB for the owned set against 4.21ms and 0.28MiB for a borrowed key at 20k violations, linear in violations rather than in files, and noise next to parsing the tree.

The borrowed RecordedKey<'a> is the version worth having, and it falls out of moving strict onto Violation instead of standing on its own, so I would rather do both in one follow-up than half of it here.

Sent with Claude Code

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

[iMacTia]

Both are in #45. The mixed case is uses_strict_mode_partially_recorded with test_check_with_partially_recorded_strict_mode_violations, and the round trip is test_check_update_check_round_trip_with_strict_mode. Your later note about a single violation type recorded in a strict pack is covered by the same fixture, through packs/qux and test_check_with_single_recorded_violation_type_in_strict_pack.

What Part A does not have is its own guard for the mixed case, and it turns out it could carry a useful partial one. That fixture records ::Bar for both violation types, ::Qux for privacy only, and ::Baz not at all. Run against three builds:

origin/main   6 reported (both ::Bar, both ::Baz, both ::Qux), stale line, exit 1
Part A        3 reported (::Baz dependency, ::Baz privacy, ::Qux dependency), 6 strict messages, exit 1
Part B        the same 3 reported,                                            3 strict messages, exit 1

The report narrows from 6 to 3 on Part A and the stale line goes, so the recorded pairs are already being matched at the (constant, violation type, file) granularity on this branch. A Part-A-only test could assert that without touching the strict messages or the exit code, and it would fail on main.

I left it out because the fixture is the one #45 adds, so it would exist on two open branches at once, which is churn on a PR you are mid-review on for a guard you get anyway when #45 lands. Say the word if you would rather have it in the tree at Part A's merge and I will find the least disruptive way to do it.

Sent with Claude Code

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

[iMacTia]

Noted, and I checked it still holds rather than carrying it over from your review. On contains_strict_violations, check -o csv emits the row twice, byte identical, both at origin/main (0ccf146) and on this branch. Message column trimmed here for width, it is identical too:

Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message
privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because …
privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because …

The mechanism is the one you named. build_reportable_violations does not filter on .strict, and the csv writer chains reportable_violations with strict_mode_violations at csv.rs:43, so an unrecorded strict violation lands in both sets. Pre-existing and untouched here, so I am leaving it. Happy to file it next to #48 and #49 if you would rather have it tracked than sitting in a thread on a PR that is about to be merged.

Sent with Claude Code

[iMacTia]

Split this per your suggestion, so #43 is now Part A on its own and Part B is up at #45. Description here is rewritten to match, and #45 is rebased on this branch.

Part B carries the write_violations_to_disk fix, which reproduced exactly as you described, plus the update message filter, the CHECKERS.md rewrite, the CHANGELOG entry, and the round-trip and mixed-case tests.

Thanks for the review, it was unusually thorough. The update bug in particular would have bitten us as soon as we went back to keeping the todo entries, which is exactly what we want Part B for.

[iMacTia]

@dduugg all six of your inline threads now have replies. Where to look, one line each:

• write_violations_to_disk, the blocking one, moved to Tolerate recorded violations in strict mode (Part B of #43) #45 with the policy change, and you have already verified the fix there. On this branch the sequence cannot reach the green state it starts from, and the thread has the measured trace.
• The no-op recorded_key() call is gone, replaced by a doc comment.
• The owned HashSet stays, on the numbers you measured on Tolerate recorded violations in strict mode (Part B of #43) #45.
• Moving strict onto Violation is a follow-up rather than this PR, reasons in the thread. Offered as an issue if you want it tracked.
• The mixed-case and round-trip tests are in Tolerate recorded violations in strict mode (Part B of #43) #45. The thread has what a Part-A-only version could assert, which turned out to be more than I expected, if you would rather have a guard in the tree at this merge.
• The duplicate csv row still reproduces at origin/main, so I am leaving it. Also offered as an issue.

The branch has not moved since 08-17. Still one commit at e198118, two files, checker.rs and check_test.rs. The only edit since is the description, which now says "Part of #41" instead of "Fixes #41", so merging this no longer auto-closes the issue while the policy half is still open.

Two asks. First, another pass, and an approval if you are happy with it, since this is the half that can land on its own and #45 carries this commit until it does.

Second, and this one is for whoever has write access rather than you specifically, @technicalpickles @martinemde: every workflow run on this branch is action_required, all nine of them, so CI has never run on Part A. The only test evidence either of us has is local. Mine at e198118 is cargo test --no-fail-fast 257 passed and 0 failed across the test binaries, with clippy and fmt clean.

One thing that would help me plan, and I ask because I would rather know than guess. We consume a released version rather than building from main, and the latest release is v0.4.0 from June, so a merge on its own does not reach us. If there is a route to a 0.5.0 once these two land, a rough sense of when would be useful. If there is not, that is genuinely fine and I will work off a fork.

Sent with Claude Code