[rominf]

Why

#582 replaced the cargo xtask check-architecture-doc checker with lychee and deleted the checker's architecture-doc pre-push hook. Since then, the docs-links CI job is the only check that a relative markdown link still resolves, so a moved or deleted file that breaks a link is first reported a full CI round-trip later. And the check only protects citations written as links: a bare backtick path goes stale with nothing to notice.

What changes

• lychee prek hook (.pre-commit-config.yaml), using the same lychee.toml as docs-links.
  • Runs on pre-commit: it checks the whole repo offline in well under a second.
  • always_run: true: a rename or deletion breaks links without touching any .md. Without it, a deletion-only commit leaves the hook "(no files to check) Skipped" while two links break.
  • In the local-tools group, so the prek CI job skips it and docs-links stays the gate.
  • Runs cargo xtask lychee --if-available (new xtask/src/lychee.rs, modelled on cargo xtask tpn --if-available). If lychee isn't installed, the hook prints a warning and passes, so a missing tool never blocks a commit. A lychee at another version, or one whose version can't be read, runs with a warning. Its output is shown only on failure. Cost: about 0.3 s per commit with xtask already built, about 5 s after an edit to xtask or rocm-core, the same build the existing xtask hooks share.
  • Walks . with --extensions md instead of CI's ./**/*.md glob. lychee's glob does not honour .gitignore, so gitignored scratch markdown (plans/, target/, ...) would otherwise fail every local commit while CI never sees it. Both forms skip hidden directories, so they check the same tracked files. lychee.toml's docs/rocm-docs/ exclusion now also matches the walk's ./ prefix and either path separator.
• Version pin. CONTRIBUTING.md documents cargo install lychee@0.24.2 --locked, next to the pinned hawkeye. 0.24.2 is the lycheeVersion default in lychee-action v2.9.0's action.yml at the pinned SHA. The docs-links step now sets lycheeVersion: v0.24.2 explicitly, so an action bump can't silently drift from the local pin, and a unit test ties both to the xtask constant.
• Citation rule in AGENTS.md §5, with CONTRIBUTING.md pointing at it rather than keeping a second copy. Cite a specific repo file as a relative link, not a bare backtick path.
  • Exceptions extend those in docs/architecture.md's intro: generic mentions, deleted files, globs, and gitignored or generated output stay bare.
  • The rule names the link forms that trip lychee: file.rs:42 is read as a filename, and a #L42 anchor is never checked.
  • It does not reach .github/ templates, docs/rocm-docs/, README.md and CONTRIBUTING.md, or links leaving a skills/ folder. README.md and CONTRIBUTING.md are {include}d into the Sphinx site, where sphinx-build -W fails on a link to a file outside it.
• Comment and doc claims about link coverage now say that hidden directories are not scanned, and the ci.yml prek-job comment lists the new local-tools hook.
• lychee.toml and CONTRIBUTING.md no longer describe all of docs/architecture.md's path citations as checked links. Only the file citations in its per-crate inventories are links; its section headings and directory names stay bare. docs/architecture.md already states this narrower scope, so the two now match it.

Verification

Run locally with lychee 0.24.2 and prek:

• prek run lychee --all-files passes: 171 links, 0 errors. The same command as CI (lychee --config lychee.toml './**/*.md') gives the same count. Re-run after rebasing onto main with docs(architecture): rewrite citations as links, delete arch-doc checker #582 merged: still 171 links, 0 errors.
• --dump-inputs lists the same 24 tracked files for the hook form and for the CI glob.
• Negative checks, each reverted afterwards:
  • A broken link in docs/architecture.md fails the hook.
  • Moving apps/rocm/src/uninstall.rs with no .md edit fails the hook.
  • A staged deletion fails the hook with always_run, and is skipped without it.
  • A wrong #anchor on a link to a markdown file fails the hook.
  • Broken-link files under gitignored plans/ and at active-implementation-log.md are ignored by the hook. The glob form flags them.
  • A broken file under docs/rocm-docs/ stays excluded.
• prek run --all-files --no-group local-tools (the prek CI job's set, including check-yaml) passes.
• sphinx-build -b html docs/rocm-docs <out> -W with docs/rocm-docs/sphinx/requirements.txt (the docs-build job's command) passes.
• cargo xtask verify-commits passes: all six commits are signed and signed off.

This is CI config and docs only, with no CLI behaviour change, so there is no e2e scenario and cargo test was not run.

Not verified: the hook on native Windows. That includes the backslash branch of the exclude_path regex, which I tested only with Python re, and the Windows (.cmd) stub the new unit tests use, which I only type-checked for that target; CI's windows-build-and-test runs it.

cargo test -p xtask passes (229 tests) and cargo clippy -p xtask --all-targets -D warnings is clean. The new tests run check against a stub lychee to cover each path: the pinned version passing and failing, another version and an unreadable version still running, and a missing lychee (passes with --if-available, fails naming the install command without it). A test also fails if LYCHEE_VERSION drifts from ci.yml's lycheeVersion or the CONTRIBUTING.md install line. Mutating the code to skip an unreadable version, or bumping ci.yml's version, made those tests fail.

[jussielo-amd]

Reviewed this change. Overall it looks solid — most of the diff (the lychee.toml regex, CI/pre-commit comment accuracy, version-pin consistency between lycheeVersion: v0.24.2 and cargo install lychee@0.24.2, Windows path-separator handling, and the script's "not found" vs. "found but erroring" distinction) checks out correctly. The notes below are non-blocking robustness and documentation-precision points, not correctness bugs.

scripts/lychee_if_available.py:217 — subprocess.run() has no timeout, so a hung or pathological lychee invocation blocks git commit indefinitely. Since this hook runs with always_run: true on stages: [pre-commit], a repo state lychee handles badly (e.g. a large generated tree accidentally un-ignored, a symlink loop, or a slow/contended filesystem) would hang every subsequent commit — worse than the intended "fail open when absent" behavior.

scripts/lychee_if_available.py:209 — shutil.which("lychee") only checks the hook subprocess's own PATH. If lychee is installed but not on the PATH the hook inherits (e.g. ~/.cargo/bin missing in a minimal shell or IDE-launched commit), it silently prints the "not found" warning and skips the check — indistinguishable from genuinely-not-installed, giving false confidence the check ran locally when it didn't.

.pre-commit-config.yaml:106 — The new lychee hook uses language: python, unlike every other local hook (commit-signing-configured, verify-commits, license-headers, cargo-manifest/catalog/tpn), which use language: system — with no comment explaining the deliberate choice. A future maintainer "fixing" the inconsistency to match siblings could reintroduce a "python: command not found" failure on systems without a python shim.

CONTRIBUTING.md:141 — Wording narrowed from "its path citations are relative markdown links" to "the file citations in its per-crate inventories are relative markdown links," which undersells what's checked — docs/architecture.md has file-link citations in ordinary prose too, and lychee checks the whole file regardless of this sentence's scope.

AGENTS.md:193 — The §5 bullet lumps .github/ and docs/rocm-docs/ (genuinely excluded/unreachable by lychee) together with "README.md and CONTRIBUTING.md" under "where the rule does not reach" — but README.md/CONTRIBUTING.md are not excluded from lychee's check; both are fully scanned by the docs-links CI job and this new local hook. The actual reason to avoid file-link citations there is a different, stricter failure mode (sphinx-build -W breaking on an out-of-tree relative link), which reads as if it belongs to the same "unreached" category as the truly-excluded paths.

[jussielo-amd]

LGTM — non-blocking notes left in a separate review comment.

[rominf]

Thanks @jussielo-amd. Your review was on 512615f, which had already replaced scripts/lychee_if_available.py with cargo xtask lychee (xtask/src/lychee.rs), so here is where each note stands:

• No timeout on the lychee call: still true in the xtask version, but left as is. lychee runs offline over the repo in about 0.1 s, and the docs-links CI job has no timeout either.
• lychee installed but off the hook's PATH reads as "not installed": the skip message now says "not found on PATH" (cf6daa9). cargo xtask also puts cargo's own bin directory on PATH for the child, which covers the usual ~/.cargo/bin case.
• language: python: gone. The hook is now language: system running cargo xtask lychee --if-available, like the other xtask hooks.
• CONTRIBUTING.md undersells what's checked: reworded in cf6daa9. architecture.md's file links are checked wherever they appear, including every concrete citation in the per-crate inventories; its section headings and directory names stay bare.
• AGENTS.md groups README.md/CONTRIBUTING.md with the excluded paths: now "where the rule does not apply" (cf6daa9). The per-item reason for those two is the sphinx-build -W failure, not lychee coverage.

Dismissing so the approval covers the current head: cf6daa9 changes wording in AGENTS.md, CONTRIBUTING.md and the xtask skip message in response to your notes. Re-requesting your review.

[jussielo-amd]

Looks good — no correctness issues found across a close read of the diff. This mirrors the existing docs-links CI job pattern well and comes with solid test coverage, including a pinned-version drift test for the lychee version check.

A few non-blocking notes for a possible follow-up:

1. xtask/src/lychee.rs:222 — the Windows unit-test stub spawns a .cmd script directly via Command::new(path), which relies on undocumented CreateProcess batch-file handling that Rust's own docs say may be removed (see rust-lang/rust#123728, related to CVE-2024-24576). The production path (invoking a real lychee.exe) isn't affected, but if that fallback is ever dropped, this test would start failing on Windows CI. Might be worth spawning via cmd.exe /c <script> explicitly instead.

2. xtask/src/lychee.rs:40 — the Probe/Checker/classify_checker/parse_lychee_version scaffold duplicates the pattern already used for the tpn command in xtask/src/tpn.rs. Since this is now the second occurrence of "probe an optional pinned external tool" in the crate, it might be worth extracting a shared helper before a third one copies it again.

3. xtask/src/lychee.rs:102 — the (Checker, if_available) match in check() interleaves two warning arms whose text doesn't depend on if_available with the one arm that does branch on it. Separating the always-run warnings from the single if_available-gated case would make the policy easier to scan.

None of these block the change.

Stale: approved cf6daa9; head is now 5918759, after a rebase onto main. git range-diff shows four of the seven commits identical. The other three differ only in surrounding CONTRIBUTING.md context, from main's hawkeye paragraph landing next to the lychee one. No change to what was approved. A fresh approval on the current head is needed to merge.