Uh oh!
There was an error while loading. Please reload this page.
fix: normalize the paths callers supply to validate - #126
Draft
perryqh wants to merge 1 commit into
Draft
Conversation
`validate <paths>` compared caller-supplied paths against project-relative ones
without reducing them to the same form first. What that costs depends on the
shape of owned_globs, and both outcomes are wrong.
Under directory-anchored globs (`{gems,ruby,...}/**/*.rb`), the mishandled path
matches nothing, is dropped before any ownership query runs, and the command
exits 0 having checked nothing. Silent, and in the unsafe direction: a
pre-commit hook or CI job reports success on a file it never looked at.
Under `**`-leading globs (`**/*.rb`), it survives the filter instead and is
queried in the caller's spelling, which matches no CODEOWNERS entry, so a
well-owned file is reported unowned.
Three spellings hit this. `./ruby/app/x.rb` and `ruby/a/../x.rb` were never
reduced at all. Absolute paths were reduced with strip_prefix against a root
that need not agree with them about symlinks: cli.rs canonicalizes
--project-root, so on macOS, where TMPDIR lives under /var, a symlink to
/private/var, a caller passing the TMPDIR spelling fails to strip -- and a
library caller building its own RunConfig (which is how the code_ownership gem
calls in) can pass an unresolved root against a resolved path, the mirror image.
Fixing only one side leaves the other failing exactly as silently, so the retry
resolves both. Absolute paths were also echoed back in the caller's spelling
rather than project-relative, since the raw string was what got reported.
path_utils::project_relative resolves `.` and `..` lexically and reports failure
rather than passing an unstrippable path through, which is how a /var/... path
came to be compared against project-relative ones in the first place. Lexically,
not by canonicalizing: the project walk records symlink paths rather than their
targets, so resolving symlinks would produce paths matching no walked file. The
first attempt uses the root as given, so relative paths -- the common case --
cost no syscalls, and the root is resolved once per run rather than per path.
Paths that no longer exist are now skipped. A changeset that deletes a file
lists it, so a deleted path reaches validate in normal use, and a deleted file
cannot have an owner -- reporting it as unowned fails a commit for removing
code. `gv <deleted file>` did exactly that. The gem already filters its list by
File.exist? before calling in, so this matches what its callers see and extends
it to direct library callers. Only a definite "not there" skips:
try_exists().unwrap_or(true) keeps a path whose status is unknown, because a
visible error is investigable and a silent pass is not.
Three tests asserted on valid_project/ruby/app/unowned.rb, which does not
exist -- valid_project has to validate cleanly, so it ships no unowned file.
They passed only because a nonexistent path was reported as unowned, meaning
they covered typo handling while claiming to cover unowned files, and skipping
nonexistent paths removes that accident. Repointed at invalid_project, which has
a real one, and narrowed to assert the path and exit status rather than the
category wording, so they do not depend on how the report is phrased.
The new tests assert the mechanism rather than the outcome: that the report
names the *normalized* path and does not echo the caller's spelling. Without
that they cannot distinguish "checked correctly" from "mishandled and
spuriously reported" -- an earlier draft of this file, written against
invalid_project's `**` globs, passed against unfixed code for precisely that
reason. Seven of the nine fail without this change; the two that pass are the
plain-relative control and the outside-the-project skip, both of which already
worked.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
validate <paths>compared caller-supplied paths against project-relative ones without reducing them to the same form first. What that costs depends on the shape ofowned_globs, and both outcomes are wrong:owned_globsshape{gems,ruby,…}/**/*.rb)**-leading (**/*.rb)The first is the dangerous one: silent, and in the unsafe direction. A pre-commit hook or CI job reports success on a file it never looked at.
Split out of #125 so it can be reviewed on its own — it fixes bugs that exist on
maintoday, independent of that PR's premise.The path forms that hit this
./ruby/app/x.rbandruby/a/../x.rbwere never reduced at all.Absolute paths were reduced with
strip_prefixagainst a root that need not agree with them about symlinks.cli.rscanonicalizes--project-root, so on macOS — whereTMPDIRlives under/var, a symlink to/private/var— a caller passing theTMPDIRspelling fails to strip:And a library caller building its own
RunConfig— which is how thecode_ownershipgem calls in — can pass an unresolved root against a resolved path, the mirror image:Fixing only one side leaves the other failing exactly as silently, so the retry resolves both. The first attempt uses the root as given, so relative paths — the common case — cost no syscalls, and the root is resolved once per run rather than once per path.
Absolute paths were also echoed back in the caller's spelling rather than project-relative, since the raw string was what got reported.
The fix
path_utils::project_relativeresolves.and..lexically and reports failure rather than passing an unstrippable path through — which is how a/var/...path came to be compared against project-relative ones in the first place.Lexically, not by canonicalizing: the project walk records symlink paths rather than their targets, so resolving symlinks here would produce paths that match no walked file.
Deleted paths are now skipped
A changeset that deletes a file lists it, so a deleted path reaches
validatein normal use — and a deleted file cannot have an owner. Reporting it as unowned fails a commit for removing code, whichgv <deleted file>did.The gem already filters its list by
File.exist?before calling in, so this matches what its callers see today and extends it to direct library callers. Only a definite "not there" skips:try_exists().unwrap_or(true)keeps a path whose status is unknown, because a visible error is investigable and a silent pass is not.A fixture that did not contain what three tests claimed
Three tests asserted on
valid_project/ruby/app/unowned.rb, which does not exist —valid_projecthas to validate cleanly, so it ships no unowned file. They passed only because a nonexistent path was reported as unowned, which means they were covering typo handling while claiming to cover unowned files. Skipping nonexistent paths removes that accident.Repointed at
invalid_project, which has a real one, and narrowed to assert the path and exit status rather than the category wording, so they don't depend on how the report is phrased.The tests assert the mechanism, not the outcome
Worth calling out, because the first draft of
tests/supplied_path_normalization_test.rswas nearly worthless and passed against unfixed code.It pointed at
invalid_project, whose**-leading globs mean a mishandled path is spuriously reported rather than dropped — and the assertion only checked that"unowned.rb"appeared somewhere in the output. So it passed for entirely the wrong reason: only 2 of 9 tests failed against unmodifiedmain.Rewritten against
valid_project(anchored globs, where mishandling drops the path) with an injected unowned file, and now asserting that the report names the normalized path and does not echo the caller's spelling. Without that, a test cannot distinguish "checked correctly" from "mishandled and spuriously reported."7 of the 9 fail without this change. The 2 that pass are the plain-relative control and the outside-the-project skip, both of which already worked. One test covers the
**-glob direction, since the symptom there is the opposite.Note for reviewers
Runner::validate_fileshere is the existing read-CODEOWNERS-back implementation. #125 replaces that method wholesale, so the ~30 lines of normalization wiring inside it are transitional — butpath_utils::project_relative,project_relative_path, the fixture correction and the whole test file survive that change unaltered. #125 will be rebased onto this branch.Unrelated, found while doing this: the
pre-commithook fails in any git worktree. Git exportsGIT_DIRto hooks, the tests spawngitsubprocesses that inherit it and read the wrong repository, and 11 git-dependent unit tests fail. Reproducible against unmodifiedmainwithGIT_DIR=<path> cargo test --lib. Worth a separate fix in.rusty-hook.toml.🤖 Generated with Claude Code