Tolerate recorded violations in strict mode (Part B of #43) - #45
Conversation
Fixes rubyatscale#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.
Builds on rubyatscale#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.
dduugg
left a comment
dduugg
left a comment
There was a problem hiding this comment.
Thanks for turning this around fast, and for carrying the write_violations_to_disk fix rather than deferring it. The round trip is stable now: on uses_strict_mode_partially_recorded, check then update then check keeps ::Bar, never writes ::Baz, and reports only ::Baz. The update summary line is accurate, and a deleted recorded reference still gets flagged stale and pruned.
Three things I probed that came back clean, so they don't get re-litigated later: a todo entry moved under a nonexistent defining pack does not grant tolerance, since both pack names are in the comparison key; recording only privacy for a constant still reports its dependency violation; and running update twice yields a byte-identical file.
clippy and fmt are clean. cargo test --no-fail-fast is 261 passed, 1 failed, the failure being test_gitignore_negation_patterns, which reproduces on main from a local global gitignore rule. Note that bare cargo test aborts at gitignore_test and never reaches update_test.
The code is right. Almost everything I left inline is about what the new prose tells people to do. One blocking item, on CHECKERS.md:18: the new adoption note describes the one ordering that does not work, and the ordering that does work is currently written down nowhere.
Two things I noticed that are not yours to fix
pks check <file> and check-contents flag every recorded violation outside the checked subset as stale, and update cannot clear it. On uses_strict_mode a full check is clean, while pks check packs/bar/app/services/bar.rb and the equivalent check-contents both exit 1 with "There were stale violations found, please run packs update". Byte-identical on main, so out of scope here, but it means the editor and LSP path exits 1 in every repo that has a todo file. Probably worth its own issue.
src/packs/checker/privacy.rs:64 has a live dbg!(constant_is_private, constant_is_in_private_namespace); firing on every privacy check for packs using private_constants. Pre-existing and untouched by this PR, but it writes to stderr in released builds.
Nothing except the adoption-order note needs to block, and that one is prose. Happy to re-review quickly once the docs get another pass.
| 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`. |
There was a problem hiding this comment.
Blocking: this describes the one adoption order that does not work.
"Turn it on, and any reference that is not already recorded will fail the check" is true, and the preceding sentence is true, but together they point at a dead end. Tolerance only matches entries that are already in a todo file, and once a pack is strict update will never create them. On contains_strict_violations, which is strict and ships no todo file:
$ pks check # exit 1
$ pks update # "1 strict mode violation(s) detected", then
# "Successfully updated package_todo.yml files!"
# no package_todo.yml is written
$ pks check # exit 1, unchanged
A user who follows this note lands there, and their only exits are fixing the reference, hand-writing the todo entry, or reverting to true. The supported order is the reverse of what the note implies: run update while the pack is still true, commit the todo files, then flip to strict.
That sentence is currently in neither CHECKERS.md nor the CHANGELOG, and it is the one this section most needs, given the PR exists to make strict mode adoptable. Not a code bug, and pks matches packwerk here.
| 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. |
There was a problem hiding this comment.
This promises a bit more than holds. It is true of update, but a hand-added todo entry does silence strict mode, and update then preserves the edit rather than dropping it:
$ printf -- '---\npacks/bar:\n "::Bar":\n violations:\n - dependency\n - privacy\n files:\n - packs/foo/app/services/foo.rb\n' > packs/foo/package_todo.yml
$ pks check # No violations detected!
$ pks update # rewrites the file, keeps the hand-added `- privacy`
$ pks check # No violations detected!
That matches packwerk, so it is not a bug. But "it keeps failing until the reference itself is dealt with" reads as a guarantee, when in practice the boundary is only as strong as review of package_todo.yml diffs. Worth a clause.
| ### 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. |
There was a problem hiding this comment.
This sentence and the one at line 114 now actively steer people toward strict_privacy_ignored_patterns, which pks does not implement.
grep -rn strict_privacy_ignored_patterns over the repo hits exactly one line: the yaml block just below, at CHECKERS.md:108. There is no field for it on Pack, and #[serde(flatten)] pub client_keys (pack.rs:125) swallows it with no error and no effect. So a user who takes this advice adds a key that does nothing and ships an unguarded strict pack.
The pks equivalent is enforcement_globs_ignore with enforcements: [privacy] (pack.rs:363), already documented at CHECKERS.md:190.
The dead yaml block predates this PR, but the PR is what turns it into a recommendation, so it would be good to either repoint these two sentences at enforcement_globs_ignore or drop the comparison and delete the stale section.
|
|
||
| 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. |
There was a problem hiding this comment.
"so a new reference from the same file still fails" is false as written. I added a second Bar reference to the already-recorded foo.rb in uses_strict_mode and check stayed at No violations detected!, exit 0.
Your own doc comment at checker.rs:60 says why: "Multiple references to the same constant in the same file are considered one violation, even if they occur at different lines." The second reference is the same violation, so it matches the recorded entry.
The stated unit is also narrower than the real one. Pack::all_violations builds the cross product of violation types and files, so what gets grandfathered is (defining pack, constant, violation type, referencing file), not (constant, file).
One word fixes the conclusion:
A
package_todo.ymlentry grandfathers one constant referenced from one file, so a reference to a different constant from the same file still fails.
That is also exactly what uses_strict_mode_partially_recorded tests, with ::Bar recorded and Baz not, both referenced from the same file. packwerk behaves the same way, since PackageTodo#listed? does files.include?(reference.relative_path), so this is wording only.
|
|
||
| #### Strict mode tolerates violations already recorded in `package_todo.yml` | ||
|
|
||
| `enforce_privacy: strict` and `enforce_dependencies: strict` now fail only on |
There was a problem hiding this comment.
This scopes the change to 2 of the 5 checkers, but the filter is checker-agnostic.
checker.rs:245 is filter(|v| v.identifier.strict), and all five arms of rules_checker_setting() (pack_checker.rs:85-101) can return CheckerSetting::Strict. So enforce_layers: strict, enforce_visibility: strict, and enforce_folder_privacy() are all affected too.
Confirmed on the layer case, using layer_violations with enforce_layers: strict on packs/feature_flags and the layer violation recorded in its todo file:
main: 1 violation(s) detected: Layer violation: `::Payments` ...
There were stale violations found, please run `packs update`
packs/feature_flags cannot have layer violations on packs/payments ...
exit=1
this branch: No violations detected!
exit=0
Anyone using enforce_layers: strict to hold a layer boundary hard gets the same silent relaxation, and the entry does not warn them. One line of prose.
| // 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; |
There was a problem hiding this comment.
This is right, and the comment explaining why is welcome. The gap is on the other side of it: nothing pins that update still prunes a recorded strict violation once its reference is gone.
Behavior is correct today, I checked. Deleting the recorded reference gets "There were stale violations found" from check, and update removes the entry and the file.
But the comment says an already-recorded strict violation "has to be re-written", and the natural over-correction to that is to union recorded_violations into the write set rather than intersecting it with found violations. That change would make strict todo entries immortal: never prunable, with check permanently green for a reference that no longer exists in the source. test_update_preserves_recorded_strict_violations only asserts the entry is present, so it would still pass, and so would the round-trip test.
That is the highest-value test this PR is missing. A Drop-in variant of the round-trip fixture with the reference removed, asserting the todo file is gone after update, would cover it.
| // 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. |
There was a problem hiding this comment.
The second half of this comment is right, the duplicate assertion really was byte-identical. The first half claims more than the test now delivers.
After the fixture swap this test no longer exercises strict tolerance at all. It passes on main, on this branch, and under a mutation that disables the recorded filter entirely, because contains_strict_violations ships no package_todo.yml, so the violation is an ordinary unrecorded one and the strict message is built from identifier.strict regardless of which bucket it lands in.
That is fine, a CSV format test is worth having. Just worth toning the comment down, or adding a recorded entry to the fixture so the CSV test actually distinguishes the recorded and unrecorded buckets.
| contents | ||
| ); | ||
|
|
||
| common::set_up_uses_strict_mode_round_trip_fixture(); |
There was a problem hiding this comment.
This restore, and the matching one at line 269, are trailing statements, so a panic skips them.
Concretely: run test_update_preserves_recorded_strict_violations against unpatched src/ and it leaves tests/fixtures/uses_strict_mode_round_trip/packs/foo/package_todo.yml deleted in the working tree. Test correctness is unaffected, since both tests also restore on entry, but the first run that catches a regression is the run that dirties the tree, and given the pre-commit hook that is a plausible way for a deleted fixture to get committed by accident.
A Drop guard around the fixture would make the restore unconditional and let you drop the paired call at the top of each test.
| violations: | ||
| - privacy | ||
| - dependency |
There was a problem hiding this comment.
This helper hand-writes violations: as privacy then dependency, but update writes them sorted, so dependency first. The pre-existing set_up_fixtures() further down this file uses the sorted order, so this is also inconsistent with local convention.
Harmless at runtime, since the reader collects into a set. The cost is that the round-trip test never compares the file against what update actually produced, only that "::Bar" appears somewhere in it. Writing the sorted order would let test_check_update_check_round_trip_with_strict_mode assert byte equality against the fixture, which would close the pruning gap I mentioned on write_violations_to_disk more or less for free.
Separately, and not about this helper: no fixture anywhere records a single violation type in a strict pack. Both uses_strict_mode and uses_strict_mode_partially_recorded record privacy and dependency together for ::Bar. I verified the single-type case works correctly today, since recorded_key() keeps violation_type. But if a future refactor ever normalized violation_type the way strict is normalized, recording one type would silence both and the whole suite would stay green.
| pks check --ignore-recorded-violations | ||
| ``` | ||
|
|
||
| ## 0.4.0 |
There was a problem hiding this comment.
This retitling is correct, and the evidence is stronger than the PR description claims. git log main --oneline -- CHANGELOG.md returns exactly one commit, 2fe98b7, so the entire old ## Unreleased section originated in a single pre-0.4.0 commit. Nothing shipped is left under Unreleased and nothing unshipped is being labelled as released. Tags do exist, v0.4.0 resolves to "Bump version to 0.4.0 for gitignore release (#38)", and Cargo.toml on main agrees at 0.4.0.
Agreed on leaving the version bump to a release PR, which matches how #25 and #38 were done.
One thing to flag for whoever cuts 0.5.0, since it makes these headings functional rather than bookkeeping: release.yml:281 builds the GitHub Release body from announcement_github_body, which cargo-dist derives by matching a CHANGELOG.md heading against the version being tagged. So ## Unreleased has to be retitled to ## 0.5.0 in the bump PR, or this entry silently will not reach the release notes.
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.
|
Thanks, this was a genuinely useful pass. The blocking one was a real defect in the prose and I had it backwards: I reproduced your
The rest of your inline notes, all taken:
On the pruning gap, which was the most valuable thing you found: added Also took the
Two smaller things you flagged: I toned down the CSV test comment, since after the fixture swap it exercises formatting only. And on the single-violation-type fixture gap, I have left it, but you are right that it is latent: nothing would catch a future refactor that normalized One measurement you may want, since you hit a gitignore failure too. Happy to file issues for the two out-of-scope items, the |
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 <noreply@anthropic.com>
|
Pushed I ran another pass over my own diff before handing it back, on the theory that you would find these on the next round otherwise. Three things, all mine:
The guard was easy to misbind.
One thing that is not a code change but affects how you read this PR: the issue reference was on the wrong one. #43 said "Fixes #41", which would have auto-closed that issue on merge while the policy half was still open here. Issue #41 lists three broken comparisons and calls the third a policy question, so #43 fixes the reported-as-new and reported-as-stale halves and this PR fixes the third, which is the one that makes And a state thing worth knowing, since it changes what "green" means here. No CI has ever run on either PR. Every workflow run on both branches is So the only test evidence on either PR is yours locally and mine locally. If you or a maintainer can approve the workflow runs, that would be worth more than another round of us both running
Sent with Claude Code |
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 <noreply@anthropic.com>
|
Pushed The
The CHANGELOG sent readers to the wrong file. I generalised your The CHANGELOG predicted something that cannot happen. I wrote that you should expect A comment claimed to be the only test catching the mutation. Two do, which Your single-violation-type gap is now closed. That was the one item I had left half done. Smaller ones. The two new fixtures disagreed on violation ordering, both sorted now. The deferred Two things I looked at and did not change. The
Sent with Claude Code |
…-recorded-violations
`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 <noreply@anthropic.com>
|
Pushed Your yaml_serde entry was about to be filed as shipped
Merged I merged rather than rebased on purpose: rebasing rewrites all five commits and would outdate your fifteen inline comments mid-review. Say the word if you would rather have a linear history and I will rebase once you are done reviewing. Three corrections that are mineThe glob fix was right about the behaviour and wrong about the cause, which is the same shape of error one level up. I wrote that these are "gitignore-style globs, so Git treats the two patterns identically and pks does not, so the note attributed pks behaviour to git semantics that predict the opposite. The mechanism is The new test's comment repeated the over-claim I had just fixed elsewhere in the same commit. It said nothing else would catch a A sibling comment had gone stale because I added Also recorded, on your
Sent with Claude Code |
2 participants
Fixes #41. Follow-up to #43, which needs to land first: every comparison here relies on that PR's
recorded_key().This is Part B of the split @dduugg suggested.
build_strict_mode_violationsnow skips violations already recorded in apackage_todo.yml, matching packwerk'sunlisted_strict_mode_violations(Shopify/packwerk#368). Turning strict mode on blocks new violations without requiring the existing list to be emptied first.--ignore-recorded-violationsstill surfaces everything the todo files are grandfathering.The blocking bug you found, and why it had to come with this
write_violations_to_diskdropped every strict violation when regenerating todo files. That line was pre-existing, but this tolerance is what starts depending on those entries, so a routinepks updatedeleted the records that made the build green. It reproduced exactly as you described onuses_strict_mode, with no source change in between:It now drops only the unlisted strict violations.
updatestill cannot be used to silence strict mode, but it no longer un-grandfathers whatchecktolerates. Same reasoning as packwerk re-adding already-listed offenses inOffenseCollection#add_offense.The rest of your list
update's summary message. It filtered on.identifier.strictwith no recorded filter, so it claimed N violations "must be fixed forcheckto succeed" whilecheckreported none. It uses the same filter as the checker now.CHECKERS.md. Both sentences you quoted are rewritten. Thestrict_privacy_ignored_patternssection now says when to reach for a path exemption instead: a todo entry grandfathers one(constant, file)pair, so a new reference from the same file still fails, whereas a pattern exempts the path outright.respect_gitignorewho's-affected format. You were right that## Unreleasedwas stale. I checked, and2fe98b7is an ancestor ofv0.4.0, so that section is now## 0.4.0and this sits under a fresh## Unreleased. Agreed on 0.5.0 rather than 0.4.x for the release itself. I have not touched the version inCargo.toml, since the gitignore PR did not bump it either and that felt like a release step rather than mine to take.--ignore-recorded-violationsis already the escape hatch.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: the mixed case you flagged as uncovered. One recorded (::Bar) and one unrecorded (::Baz) in the same strict pack in one run, and only the unrecorded one is reported.test_update_preserves_recorded_strict_violations: the recorded entry survivesupdate, and the false summary line is gone.test_check_update_check_round_trip_with_strict_mode: check, update, check, still clean.test_check_with_strict_mode_output_csvmoves tocontains_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 line 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.Still deferred, as you said was fine: moving
strictoffViolationIdentifierontoViolation, and the borrowed-key optimisation inbuild_stale_violations.cargo test,cargo clippy --all-targets --all-features -- -Dwarningsandcargo fmt --all -- --checkall pass.