Found by @dduugg while reviewing #45. Filing separately since it is pre-existing and unrelated to that PR.
pks check <file> and pks check-contents treat every recorded violation outside the checked subset as stale, so they exit 1 in any repo that has a package_todo.yml, and pks update cannot clear it.
Repro
tests/fixtures/uses_strict_mode records ::Bar in packs/foo/package_todo.yml, referenced from packs/foo/app/services/foo.rb.
A full check is clean:
$ pks --project-root tests/fixtures/uses_strict_mode check
No violations detected!
$ echo $?
0
Checking a file that is not the one holding the recorded reference is not:
$ pks --project-root tests/fixtures/uses_strict_mode check packs/bar/app/services/bar.rb
There were stale violations found, please run `packs update`
$ echo $?
1
Byte-identical on main and on the #45 branch, so this is not a regression from that work.
Why it happens
build_stale_violations derives relative_files from found_violations.absolute_paths, which is the checked set. A recorded violation whose file is not in that set takes the else branch of is_stale_violation and is reported stale, on the reasoning that the file no longer exists. When the subset is narrowed by an argument rather than by the file being deleted, that inference does not hold.
Why it matters more than it looks
This is the path editors and LSP integrations use, since they check the file being edited rather than the tree. So in any repo with a todo file, the editor integration exits 1 on every save, and pks update will not quiet it because nothing is actually stale.
Possible directions
Either skip the stale check entirely when the run is scoped to a subset, or restrict it to recorded violations whose file is in the checked set. The second keeps stale detection working for a genuinely deleted file within the subset.
Happy to send a PR if you have a preference on which.
Sent with Claude Code
Found by @dduugg while reviewing #45. Filing separately since it is pre-existing and unrelated to that PR.
pks check <file>andpks check-contentstreat every recorded violation outside the checked subset as stale, so they exit 1 in any repo that has apackage_todo.yml, andpks updatecannot clear it.Repro
tests/fixtures/uses_strict_moderecords::Barinpacks/foo/package_todo.yml, referenced frompacks/foo/app/services/foo.rb.A full check is clean:
Checking a file that is not the one holding the recorded reference is not:
Byte-identical on
mainand on the #45 branch, so this is not a regression from that work.Why it happens
build_stale_violationsderivesrelative_filesfromfound_violations.absolute_paths, which is the checked set. A recorded violation whose file is not in that set takes theelsebranch ofis_stale_violationand is reported stale, on the reasoning that the file no longer exists. When the subset is narrowed by an argument rather than by the file being deleted, that inference does not hold.Why it matters more than it looks
This is the path editors and LSP integrations use, since they check the file being edited rather than the tree. So in any repo with a todo file, the editor integration exits 1 on every save, and
pks updatewill not quiet it because nothing is actually stale.Possible directions
Either skip the stale check entirely when the run is scoped to a subset, or restrict it to recorded violations whose file is in the checked set. The second keeps stale detection working for a genuinely deleted file within the subset.
Happy to send a PR if you have a preference on which.
Sent with Claude Code