Repository navigation
pks check <file> and check-contents report recorded violations outside the subset as stale, so they always exit 1 #48
Description
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
Activity
dduugg commentedon Sep 27, 2026
Thank you for writing this up, @iMacTia, and for the clear repro. The editor and LSP angle is exactly why it's worth fixing, and yes please on the PR.
I'd go with your second direction, scoped to runs that were given files: when
checkgets paths, and forcheck-contents, only consider recorded entries whose file is in the checked set. Full runs should keep today's behaviour, where a recorded file that isn't in the checked set is treated as deleted.For what it's worth, packwerk has the same bug. It's Shopify/packwerk#369, which comes from the deleted-file detection added in Shopify/packwerk#332: in a scoped run, any recorded file that produced no new entries counts as deleted. The open fix there, Shopify/packwerk#415, takes the same approach and skips the deleted-file check when files are supplied. So this keeps pks in line with where packwerk is heading rather than diverging from it.
On keeping stale detection for a genuinely deleted file within the subset: when the argument is a file, a deleted file can't be in the checked set anyway, so there's nothing to keep. It only matters when the argument is a directory, like
pks check packs/foo. If it's easy, treating an entry as stale when its file is under a supplied path and no longer exists on disk would cover that case. I'd be happy with the simpler version too, though, since a fullcheckstill catches it.For tests, your repro on
uses_strict_modewould make a good set:check packs/bar/app/services/bar.rb, andcheck-contentson the same file, both exit 0, while a fullcheckwith the recorded file deleted still reports it stale. Please also add a line under## Unreleased→### Fixesin CHANGELOG.md, since releases now take their notes from there.One more thing that should make this quicker than last time: now that #43 and #45 are merged, CI runs on your PRs straight away, without waiting for approval.
Scope stale todo checks to given paths and warn about unmatched paths (…