Repository navigation
ci: fail on a prose reference to a source file the repository does not track - #1250
Conversation
There was a problem hiding this comment.
Reviewed 971ff891: changes requested.
- The tracked-parent rule mistakes deleted or mistyped source directories for generated output:
crates/cuda-device/src/missing.rs -> rejected
crates/cuda-device/deleted/mod.rs -> accepted as "all resolve"
- A local untracked file can also hide a missing repository reference; an external URL suffix can be treated as a repository path.
- Define tracked-source versus generated-output handling explicitly, and bound repository-path matching. Add deletion/rename, typo, external-URL and untracked-file controls through the real scanner.
The current tree and known historical cases pass, but those controls do not establish the proposed policy. #1248 remains open.
971ff89 to
f5d25d3
Compare
|
Revised on your review. The three points, each with the old behaviour beside the newSame probe planted in a tracked prose file, run against the old guard and this one:
What changedResolution is now "tracked by git", not "present on disk". Those are different questions and only the first is reproducible: a local build artifact or somebody's stray file resolves for them and nobody else, so the filesystem test made the guard's answer depend on whose checkout it ran in. That is your third point, and it is the same change that fixes the first two. Generated outputs are declared. Repository-path boundaries. The roots are verified against the index before each sweep, and a traversal segment is reported rather than resolved. That check earned its place immediately: Controls
One fragility the historical control exposed, worth naming because it would have bitten quietly: the self-test used this script's own path as its tracked example, so it depended on the guard already being committed — false on the branch that adds it, and in any worktree. It now takes that example from the index. Still not checked, and still stated rather than papered over: crate-relative and bare-basename spellings. Your #1197 follow-up fixed 14 references written that way and this would not have found them. |
|
The rewrite fixes the missing-directory and untracked-file cases. I’m adding two small repairs on this branch:
I’ll validate the repaired head and update the review. |
f5d25d3 to
8ebb864
Compare
64064ca to
8ebb864
Compare
|
First, an operational apology. I force-pushed over Second: your version supersedes what I was about to push, and I have dropped mine rather than re-apply it. I had a Verification of
|
| control | result |
|---|---|
| clean tree | 62 repo-anchored paths in prose |
historical, at 6f5f538a^ (before #1197) |
reports exactly #1197's three, nothing else |
| missing parent directory | catches |
| misspelled parent directory | catches |
| deletion/rename under a live directory | catches |
| untracked file that exists on disk | catches |
| traversal segment | catches |
| foreign URL whose tail looks like a path here | passes |
| declared generated output, tracked file | pass |
Eight mutations, each asserted to have applied: tracked membership always satisfied, doc-comment filter matching every line, URI arm never matching, emphasis unwrapping removed, ./ handling removed, line-citation stripping removed, and the declared-output tracked-check disabled — seven caught by your self-test. The eighth is below.
The 67 -> 62 difference, in case it looked like lost coverage
Five paths my regex counted and yours does not. Four are the github.com/NVlabs/cuda-oxide/blob/main/... links in kernel-families.md, overlapping-transfers-and-compute.md and pinned_overlap/README.md — external references by your rule, deliberately.
The fifth is a defect in mine that yours fixes. crates/cargo-oxide/src/backend_source.rs:23 documents a cache path:
//! ~/.cargo/git/checkouts/cuda-oxide-<hash>/<sha>/crates/cuda-device/Cargo.toml
My pattern matched the tail of that line as crates/cuda-device/Cargo.toml. It happens to be tracked, so it never failed — but "longer paths never donate suffixes" in your token comment is doing real work on the current tree, not just in principle.
Two findings
1. The traversal control does not discriminate. Disabling the traversal branch entirely — if ".." in path.split("/") -> if False — leaves the self-test green.
("case.md", "crates/../scripts/no-such-source-reference-file.sh", 1)
Both branches produce one finding for that input: with the rule, "contains an unsupported traversal segment"; without it, the path is not a string git tracks, so the membership arm reports it as untracked. The control asserts the count, and the count is 1 either way. broken_references already yields the reason, so comparing reasons rather than lengths pins it. (I hit the same thing in my own version and concluded the branch was dead weight; that was the wrong conclusion — your message is better, the control just needs to assert it.)
2. A path immediately after a colon is skipped. No live instance in the tree, so this costs nothing today:
[spec]:crates/nope/src/lib.rs missed [spec]: crates/nope/src/lib.rs caught
TODO:crates/nope/src/lib.rs missed TODO: crates/nope/src/lib.rs caught
TODO: satisfies [A-Za-z][A-Za-z0-9+.-]*:, so the URI arm consumes the span; in the reference-definition form the token begins at the colon and fails fullmatch. Both are the spelling-without-a-space case, both latent. git grep finds zero of either in prose right now, so this is a note for whenever the tokenizer is next touched, not something I would change under you.
Happy to push either as a commit on top if you want them; I did not want to touch this branch again without being asked.
|
Thanks @vyncint — the reference-link case needs a fix in my extractor. I will handle the Markdown delimiter before token classification and make the traversal control check its diagnostic.
|
…t track NVIDIA#1190 split large files into module directories and left three prose references pointing at paths it had removed; NVIDIA#1197 repointed those and 14 more. Nothing catches that: a path in prose is compiled by nothing, `check-host-api-paths.sh` checks Rust API paths rather than files, and the book gate only builds. "In the repository" means tracked by git, not present on disk. Those are different questions and only the first is reproducible -- a path that exists as a local build artifact, or an untracked file someone happens to have, resolves for them and nobody else, so a filesystem test makes the guard's answer depend on whose checkout it ran in. Generated outputs are declared, never inferred. An earlier revision skipped any path whose parent directory held no tracked file, reasoning that such a directory must be an output location. That is backwards: deleting or misspelling a directory produces exactly the same signal as generating into one, so the broken reference this guard exists to catch was the case it let through. `GENERATED_OUTPUT_PATHS` is now the whole accommodation -- one entry, `crates/fuzzer/artifacts/summary.jsonl`, which `run_seed.py` writes -- and it is verified rather than trusted: an entry that becomes tracked, or that nothing refers to any more, is an error instead of a silent exemption. Roots are verified too. `docs/` was in the original list and holds no tracked file in this repository, so the pattern anchored there matched nothing and would have gone on matching nothing after a rename. The check that caught it runs before every sweep. Paths containing a traversal segment are reported rather than resolved, since they cannot name a tracked file relative to the repository root. The self-test runs the real scanner over four cases -- a missing file under a tracked directory, a missing file under a directory the repo does not have, a file that exists only in this checkout, and a tracked file -- and requires the first three to be reported and the fourth not. It takes the tracked example from the index rather than naming this script, which had made the self-test depend on the guard already being committed. Crate-relative and bare-basename spellings are still not checked. NVIDIA#1197's follow-up fixed 14 references written that way, so that is a real gap; telling `rvalue.rs` in prose from any other mention of it is guesswork, and a guard that guesses is worse than one that is narrow. Signed-off-by: Vyncint Ng <115854244+vyncint@users.noreply.github.com>
Signed-off-by: nihalp <nihalp@nvidia.com>
Signed-off-by: nihalp <nihalp@nvidia.com>
8ebb864 to
603aa86
Compare
There was a problem hiding this comment.
I rechecked the tracked-file rewrite and added the remaining extractor follow-up:
- Markdown reference definitions such as
[source]:crates/pkg/lib.rsnow use the same path check as ordinary links and generated-output references. - External and custom-scheme URIs stay intact.
- The traversal control now checks its diagnostic, so removing the rule fails the test.
At 603aa864, 43 independent script controls, eight scanner mutations and all seven affected guards passed. The historical tree still reports exactly the three stale paths from #1197. Hosted CI also passes on this head.
This partially addresses #1248; docs/ paths remain outside coverage.
What this fixes
Catch stale repository source paths in Markdown and Rust line doc comments before readers follow them.
What we fixed
Verification
Checked locally at
603aa864:Caveats
The guard covers declared extensions under
crates/,cuda-oxide-book/andscripts/. It checks tracked Markdown and Rust line doc comments; it does not resolve external URLs, crate-relative paths or bare filenames. Scheme-prefixed tokens such asTODO:crates/file.rsremain URI tokens.Partially addresses #1248; its requested
docs/root is outside this guard's coverage.