[iMacTia]

Fixes #48, following the direction in your comment.

pks check <paths> and pks check-contents only check the files they're given, but the stale check still compares every entry in every package_todo.yml against that subset. An entry for a file outside it looks as if its file had been deleted, so it's reported as stale and the run exits 1. The packwerk VS Code extension checks one file at a time, by its path relative to the root, when a file is opened or saved, so in any app with a todo entry in another file, every one of those checks fails.

Approach

A run given paths now only considers entries for files at or under those paths, and such an entry is stale if the run no longer finds it or its file is no longer included, which is the test a full run uses. So a deleted or newly excluded file is still caught when the run names the file, a directory containing it, or ., which covers a pre-commit hook running pks check $(git diff --cached --name-only).

A path that matches no included file checks nothing. In an app with todo entries those runs used to fail on other files' entries anyway; with the stale fix alone they would exit 0 silently. So check and check-contents now warn on stderr when an argument matches nothing and it's likely a mistake, naming the path to pass where there is one; an existing file that isn't included stays quiet. Stdout and the exit code don't change, so JSON and CSV output and the editor plugins are unaffected. packwerk only prints "No files found or given", on stdout, when nothing matched at all.

Where to look

• The given paths are kept apart from the checked set, because a path that matches no included file leaves that set empty and a deleted file is never in it.
• files_to_check intersects each argument on its own so it can report the ones that matched nothing; the union is the set a single call used to return, and a directory is still globbed once.

Sent with Claude Code

[dduugg]

Thanks for this, and for sticking so closely to the direction in #48. The fix does what it says. On uses_strict_mode, check packs/bar/app/services/bar.rb and check-contents on the same file now exit 0. On contains_stale_violations, a single-file run reports only that file's stale entries: 1 for foo.rb and 2 for bar.rb, where main reported 3 and 4.

I ran the new tests against main's src/: the seven you said fail do fail, and the full-run test passes. I also tried three plausible wrong versions of the flag (always scoped, skip every stale check when scoped, derive it from absolute_paths), and each one breaks at least one test. update output is byte-identical to main, --experimental-parser scopes the same way, and clippy, fmt and CI are all clean.

One request, one thing I'd like tracked before release, and a few small items.

Deleted files in a scoped run

I said in #48 that the simpler version was fine, but after running it I'd like to revisit that. The loss of deleted-file detection isn't limited to directories. On uses_strict_mode with foo.rb deleted, all three of these exit 0 on this branch, while a plain pks check exits 1:

pks check packs/foo/app/services/foo.rb
pks check packs/foo
pks check .

The first one worries me most. A pre-commit hook that runs pks check $(git diff --cached --name-only) gets the deleted file's path from git, so deleting a file that has todo entries passes the hook and leaves the entries behind.

. also puts us out of step with packwerk. packwerk check . runs as a full check, with or without Shopify/packwerk#415. Dir.glob("./**/*") yields paths that start with ./, and File.fnmatch?("**/*.{rb,rake,erb}", "./packs/bar/app/services/bar.rb", File::FNM_EXTGLOB) is false because of the leading period. So specified_files comes back empty and files_specified? is false. That is probably an accident on packwerk's side, but it's what packwerk check . users get today.

The rule I floated in #48 covers all three cases, and it turned out to be small. check_all still has the raw files, so nothing needs argument expansion to remember which arguments were directories. Keep the supplied paths as absolute paths in place of scoped:

supplied_paths: files
    .iter()
    .map(|f| configuration.absolute_root.join(f))
    .collect(),

Then, for an entry whose file wasn't checked, call it stale when the file is at or under one of those paths and isn't on disk:

} else if !supplied_paths.is_empty() {
    let path = absolute_root.join(&todo_violation_identifier.file);
    supplied_paths.iter().any(|p| path.starts_with(p)) && !path.exists()
} else {

Path::starts_with compares whole components, so packs/foo doesn't match packs/foobar, and join returns absolute arguments unchanged. I tried this on top of your branch. All 42 tests in check_test.rs still pass, the three commands above exit 1, and check packs/bar and check packs/bar/app/services/bar.rb still exit 0. It would need a test for each of the three commands.

If you'd rather keep the current behaviour, the CHANGELOG should say that naming the deleted file also stops it being reported. "A run given a directory" reads as though file arguments are unaffected.

Paths that match nothing now pass

You raised this in the notes, and I agree it belongs in its own issue. I'd like that issue open before this ships, though, because this PR is what makes the problem silent. An argument that matches no included file checks nothing, and it now exits 0 with "No violations detected!". That covers typos, paths containing .., absolute directories, and absolute paths through a symlink. For example, with a fixture copied under /tmp (a symlink to /private/tmp on macOS), pks check /private/tmp/.../foo.rb checks the file and reports its stale entry, while pks check /tmp/.../foo.rb checks nothing and exits 0. A warning when an argument matches no included file would probably be enough.

Smaller things

• tests/check_test.rs:456: the comment says editor integrations pass the absolute path of the file being saved, and the PR description says the same of the VS Code extension. As far as I can tell, neither plugin does. packwerk-vscode strips the workspace root and runs ${executable} ${relativeFileName} with cwd set to the root. packwerk-intellij passes VfsUtilCore.getRelativePath(...), to check-contents when it lints unsaved buffers. The test is still worth keeping, since absolute arguments take a different branch in user_inputted_paths_to_absolute_filepaths. I'd just reword the comment to say that.
• Two positive cases aren't covered: a directory argument where a file inside it does have a stale entry (the directory test uses a pack with no entries), and check-contents reporting a stale entry for the file it was given. Both go through code that's already tested, so they're cheap insurance.

Not yours to fix

• -o csv never includes stale entries. On contains_stale_violations, pks check -o csv prints the header and a "No violations detected!" row, then exits 1. Same on main.
• A ./ in the middle of a path isn't normalized. pks check packs/foo/./app/services/foo.rb on contains_stale_violations keeps that spelling in the violation's file, so the recorded entry doesn't match and main reports a new privacy violation. Pre-existing, and it doesn't interact with this change beyond the stale entry for that file being skipped too.

Happy to take another look once you've decided on the deleted-file rule.

[iMacTia]

Thanks for the careful review, and for trying the rule on top of the branch.

Deleted files in a scoped run. Agreed, and I've taken your rule with one change. check_all keeps the given paths, joined onto the root, in place of the flag. For an entry whose file wasn't checked, I test whether the file is at or under one of them and isn't an included file, instead of !path.exists(). That's the test a full run applies, so check . agrees with a full check for every entry, including a file that's still on disk but was excluded, which the disk check skipped. An absolute directory still doesn't get its files' entries reported as stale, because they're still included even though it isn't expanded. It also saves a stat per entry.

Each of your three commands now exits 1 on the deleted-file copy and has a test, and there are tests for an excluded file under . and for an absolute directory, which mustn't report entries for files that are still included.

Paths that match nothing. I decided to fix this here rather than leave it for a separate issue, since this PR is what makes it silent. check and check-contents now warn on stderr when an argument matches no included file and that's likely a mistake, naming the argument. That covers every case you listed: a typo, or a deleted file your hook passes, is reported as not existing, and a .. or symlinked spelling of an included file, or an absolute, .. or symlinked spelling of a directory that holds one, gets the path to pass instead. Only a directory with no included files gets the include and exclude advice.

I narrowed it so a hook passing every staged file doesn't warn on YAML, a README or the Gemfile: an existing file that just isn't included stays silent, unless its canonical path is an included file.

Editor integrations. You're right, packwerk-vscode passes the path relative to the root. The test comment now says the test is there for the absolute-path branch, and I've corrected the description.

The two positive cases. Added both: a directory argument with a stale entry inside it, and check-contents reporting a stale entry for the file it was given.

CSV and ./ mid-path. Agreed, leaving those for separate changes.

The changes are in seven new commits on top, so you can see just the delta.

Sent with Claude Code

[dduugg]

Both requests from my last review are handled, and I tried to break each of the seven commits before trusting it.

Deleted files in a scoped run

You went further than what I suggested, and it's the better rule. I'd proposed "stale if the file is under a supplied path and not on disk," but your version checks !included_files.contains(&path) instead, and that also catches a file that's still on disk but excluded, which mine would have missed. test_check_with_dot_reports_recorded_violations_of_excluded_files_as_stale pins exactly that case, and I confirmed it fails without the included_files check (a !path.exists() version would pass it incorrectly).

I reverted checker.rs to aa94bef (what I last reviewed) with these commits' tests kept, and nine of the sixteen new tests fail there, split cleanly between the deleted/excluded-file fix and the new warning. The other seven pass on both, which is what I'd expect from tests pinning scope restrictions and positive cases that the old code already got right.

All three commands from my last review now exit 1 on the deleted-file copy, each with its own test: the file itself, packs/foo, and ..

Paths that match nothing

This covers more than the cases I listed. I tried the symlink case again (/tmp/.../foo.rb through the /tmp to /private/tmp symlink on macOS) and it now warns and names the canonical path to pass. I also tried a wrong-case path on APFS (packs/Bar/app/services/BAR.rb) and a ..-laden relative path, neither of which you called out specifically, and both get the same "pass it as X instead" treatment, because included_spelling canonicalizes before comparing. A typo gets "does not exist," a README or package.yml stays silent, and mixing a real file with a typo in one invocation checks the real file and warns only about the typo. -o csv and -o json both still parse with a warning on stderr.

One edge case, low priority: pks check .. warns with "no included file matches .., check the include and exclude globs," which is the generic message rather than one about pointing outside the root. included_spelling can't suggest a relative spelling here, since ..'s canonical form doesn't strip against absolute_root the way a path inside the project does. Nobody has a reason to pass .., so I wouldn't hold the PR for it, just flagging it in case it's cheap to special-case later.

Everything else

cargo test, clippy --all-targets --all-features -- -Dwarnings, and fmt --all -- --check are all clean on 9f0be51, aside from the usual test_gitignore_negation_patterns failure from my own global gitignore. CI is green. The diff against current main is just checker.rs, CHANGELOG.md, and check_test.rs, so there's nothing to reconcile with #66 or #59.

This one's good to merge.