docs(dash,xtask): remove doc pointers to documents that never existed and gate against new ones - #371
Open
siloteemu wants to merge 11 commits into
Open
docs(dash,xtask): remove doc pointers to documents that never existed and gate against new ones#371siloteemu wants to merge 11 commits into
siloteemu wants to merge 11 commits into
Conversation
Module docs cited 18 `../wiki/…` markdown paths across 14 files. No `wiki/` directory has ever existed in the tree or anywhere in git history, and the project has no GitHub wiki. The paths were also mutually inconsistent about depth (`../`, `../../`, `../../../`), and every one sat inside backticks, so none of them ever rendered as a link even where a target might have existed. None of the 14 cited documents exists anywhere, so each pointer is either removed or replaced with a short explanation inline in the file it was meant to send the reader away from. One further citation of the same class, a bare `rocm-cli-unification-working-agreements.md`, was equally unresolvable and is fixed here too. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
`cargo xtask verify-doc-paths` scans `//!` and `///` lines in tracked `.rs` files for citations of `.md` files and resolves each one against the citing file's own directory or the workspace root, both of which this tree uses. Every violation is reported at once rather than only the first. The check runs in the existing `clippy` CI job. `**/*.md` joins the `rust` path filter so that deleting or renaming a cited markdown file also triggers it, even when the change touches no `.rs` file. URLs and lines outside doc comments are deliberately ignored: ordinary code names `.md` files legitimately in string literals, and flagging those would make the check unusable. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
`cargo xtask verify-doc-paths` failed on the file that implements it. The scan is line-based, and the test fixtures were multi-line string literals joined with a trailing backslash, so each continuation line began with `//!` or `///` once its indentation was trimmed. Two lines of fixture data — paths chosen precisely because they resolve to nothing — were read as real citations on `doc_paths.rs` itself. Building the fixtures with `concat!` keeps the prefix off the start of a line and leaves what each test asserts untouched. The paths they carry are now neutral placeholders: the check's acceptance criterion is that no `.rs` file in the tree names the directory that motivated it, and a test fixture is no exception. Both unresolvable shapes are still covered, the `../`-prefixed form and the bare filename. A design note records the limitation, since the same shape can bite a doc comment that quotes source or holds a fenced code block. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
`resolve` joined the candidate onto both the citing file's directory and the workspace root and asked the filesystem, so a `../`-prefixed citation could climb out of the checkout and match a file in a sibling directory. Whether such a directory exists is a property of one machine, not of the tree: the same citation would resolve on the author's disk and dangle in CI, and the shape that motivated this check is exactly the one that escapes. Both candidates are now normalized lexically — `canonicalize` is no use on paths that do not exist — and rejected unless they land inside the workspace root. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
The module doc said rows are grouped by `(cell, model, backend, concurrency)`. `RollupKey` also holds `tp` and `dtype`, and the field is `engine`, not `backend` — a difference the reader would only find by disbelieving the doc, and one that misstates what Pass^N and Pass@N are computed over. `row_verdict`'s summary is also split into a real first sentence. The paragraph break existed only to satisfy `too_long_first_doc_paragraph`, which left rustdoc showing half a sentence as the item's summary. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
The module doc said the trial-group key is "every field that defines the run except the trial itself". It is not: `BenchmarkRow` also carries `task`, `prompt_class`, `pp`, `max_num_seqs`, `attention_backend`, `kv_dtype`, `spec`, `input_len`, `output_len` and `cache_mode`, none of which `RollupKey` holds. Two rows that differ only in `input_len` are folded into one trial set, so the sentence promised a guarantee the code does not make. `RollupKey`'s own doc carried the same overreach from further back — "everything that defines the backend config is held fixed" — while `endpoint`, `pp`, `max_num_seqs`, `attention_backend`, `kv_dtype` and `spec` sit in the schema's backend-config block and outside the key. Both now state the key and say plainly what it does not separate. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
Two doc comments pointed at a line range in another file. Both had already rotted: `agent.rs:59-62` lands on `InferenceParams`' sampling knobs rather than the read-only chat seam it names, and `runner.rs:446-460` on nothing in particular — the `known_services.difference(&disc.seen)` it cites now sits near line 519. A line range is a citation that decays on the next edit above it, which is the same failure this branch is removing elsewhere, so both now name the file and the symbol and leave the reader to grep. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
`**/*.md` joined the `rust` filter so a deleted or renamed doc file reaches `cargo xtask verify-doc-paths`. dorny/paths-filter matches patterns with picomatch, where a leading `**/` also matches zero directories, so the glob already covers the root-level `MANIFEST.md` entry sitting above it. Keeping both left a rule that can never decide anything, and two comments where one states why markdown is a Rust input at all. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
`"////foo".strip_prefix("///")` succeeds, so a four-slash separator
banner was scanned like a doc line. rustc treats `////` as an ordinary
comment and rustdoc renders nothing from it, so a file name inside one is
not a citation the reader can follow, and flagging it would fail CI over
text no document ever pointed at.
None exists in the tree today, which is why the gate stayed green. The
design note also records that the block forms `/*! … */` and `/** … */`
are not scanned at all.
Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
Two tests passed for the wrong reason. `ignores_urls_ending_in_md` used only `scheme://host` links, where the scheme's `//` leads the character run and the absolute-path guard rejects the candidate on its own — the URL filter could be deleted and the test would still be green. It now also carries an anchored link, whose fragment puts a bare relative path inside the token, so dropping the whole token is what keeps the line quiet. `flags_every_unresolvable_citation_not_just_the_first` re-implemented the collection loop with an iterator instead of calling the code that does it, so `run()` could have returned at the first miss and the test would not have noticed. The loop moves into `check_source`, which returns the citations checked and a violation line for each miss; the test now covers both the count and the rendered messages, and `run()` keeps its totals. Verified by mutation: neutering the URL filter or capping the collection at one now fails the matching test. A design note records what the URL filter does not cover — a link written without a scheme, and a citation glued to a URL with no space. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.com>
Mutating each guard in `candidates_in_line` and deleting it showed two that no test noticed. Removing the angle brackets from the placeholder delimiter set left every test green, because the only bracketed fixture (`<name>.md`) has an empty stem and is rejected by the stem check first. Removing the stem check left every test green in turn, because that same fixture is rejected by the delimiter check. Each guard was standing in for the other, so either could have been deleted unnoticed. Add a bracketed name with a real stem, and a bare extension with nothing next to it, so each guard is now the only thing rejecting one case. Signed-off-by: Teemu Kuusisto <teemu.kuusisto@amd.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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Module-level doc comments across the
rocm-dashcrates sent readers to 18../wiki/…markdown paths in 14 files. Nowiki/directory has ever existedin this repository or anywhere in its history, and the repository has no GitHub
wiki, so none of the 14 cited documents could ever be opened. The paths were
mutually inconsistent about depth (
../,../../,../../../) and all satinside backticks, so none of them ever rendered as a link either.
These pointers sat on the modules carrying the least obvious design — the
state reducer, the metric registry, the benchmark result schema, GPU partition
modes — so the reader was sent away exactly where the explanation was most
needed.
Each pointer is either removed or replaced with a short explanation written in
the file, since none of the cited content exists anywhere to link to instead.
Three further citations of the same class are fixed here as well: a bare
rocm-cli-unification-working-agreements.md, which is equally unresolvable,and two pointers that cited line ranges in other files which had already
drifted onto unrelated code.
To stop the pattern returning,
cargo xtask verify-doc-pathsscans//!and///lines in tracked.rsfiles for.mdcitations and resolves each oneagainst the citing file's directory or the workspace root, reporting every
violation in a single run. It ignores URLs, non-doc-comment lines and
////banner comments so that string literals and build-artifact paths are not
flagged. It is wired into the existing
clippyjob, and**/*.mdwas added tothe
rustpath filter so that deleting a cited markdown file also runs thecheck.
Verification
cargo xtask verify-doc-paths—16 citation(s) across 188 file(s) resolvecitations fails the check and names the file and line; reverting it passes.
must fail a named test — so the unit tests cannot pass for the wrong reason.
cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace --all-targets,cargo test --workspace --doc,hawkeye check,prek run --all-files,python scripts/smoke_local.py— all pass.No acceptance scenario accompanies this change: it alters documentation text
and adds a build-time consistency check, and changes no runtime behaviour. The
new check is covered by unit tests in
xtask/src/doc_paths.rs.tests/e2e-cucumber/expectations.tomlfor the fixed ticket ID and removed/narrowed any now-stale xfail rows.Not applicable — this changes no runtime behaviour, and no xfail rows relate to it.