ci: add lychee link-check gate for markdown by jussielo-amd · Pull Request #579 · ROCm/rocm-cli

jussielo-amd · 2026-10-07T07:35:39Z

Summary

xtask/src/architecture_doc.rs's hand-rolled parser validates docs/architecture.md's
backtick-quoted path citations against the tracked tree, and has grown into ~1,500 lines
of prose-inference heuristics to do it (possessive-owner chains, sentence-boundary
detection, an abbreviation list, a prose-slash-word list). #492, #440, and #524 all trace
back to that file's complexity. The agreed replacement (#578, from review on #493) is to
make citations explicit markdown links and check them with
lychee instead — a link carries its own full
path, so nothing needs inferring.

This PR lands the CI side of that migration on its own, ahead of the citation rewrite:
it only checks links that already exist today (~50 across 14 markdown files, none yet in
docs/architecture.md), so it's additive and doesn't touch any content docs/architecture.md
PRs #493/#564 are concurrently editing.

lychee.toml scopes the check to local file/anchor links only (offline = true) rather
than also following external https:// URLs, to keep this gate immune to external-host
flakiness. It excludes docs/rocm-docs/: that tree is Sphinx/MyST source, and its
cross-doc anchors (e.g. commands.md#model-serving) are generated at Sphinx build time
from {include} directives lychee doesn't evaluate — checking it there produced false
positives in testing. That tree's correctness is already covered by the docs-build
(Sphinx -W) job, which does understand MyST. It also excludes SECURITY.md's two
../../security/advisories/new links, which are relative-looking but meant to resolve on
GitHub's web UI, not as local files.

CONTRIBUTING.md's list of enforced CI invariants gains a line for this job, phrased to
match lychee.toml's actual scope (local links only, offline, with docs/rocm-docs/
excluded) rather than claiming full link coverage.

Scope

Not included here: a local pre-push mirror of this check (the existing hygiene hooks all
wrap a cargo xtask binary already being compiled for other hooks; lychee is a separate
binary with no install step in this repo's tooling yet) and network/external-link
checking (same flakiness concern as above). Both are easy follow-ups if wanted later.

The citation rewrite and architecture_doc.rs deletion are a separate, following PR,
blocked on #493 and #564 merging first since both touch the exact content that rewrite
would touch.

Test plan

lychee --config lychee.toml './**/*.md' (v0.24.2, matching the version pinned via
lychee-action@v2.9.0): 18 OK, 0 errors, 43 excluded.
prek run --no-group local-tools --files lychee.toml .github/workflows/ci.yml —
hygiene hooks (YAML validity, trailing whitespace, merge-conflict markers) pass.
yq '.jobs."docs-links"' .github/workflows/ci.yml confirms the new job parses as
intended.
prek run --files CONTRIBUTING.md passes for the coverage-claim wording fix. No
regression test added for it: it's a doc-accuracy correction with no behavior change,
nothing to regress.

docs/architecture.md's path citations are about to move to explicit markdown links instead of bare backtick filenames, checked by an off-the-shelf link checker instead of a hand-rolled parser (see ROCm#578). This adds that checker now, additively: it only looks at links that already exist, so it can land independently of the citation rewrite and doesn't conflict with ROCm#493/ROCm#564, which are still editing docs/architecture.md's prose. Scoped offline (no network) to avoid external-host flakiness, and excludes docs/rocm-docs/ (Sphinx/MyST source whose cross-doc anchors are resolved by the docs-build job, not plain markdown). Signed-off-by: Jussi Elo <jussi.elo@amd.com>

Copilot

Copilot review overview

🟢 Approval recommended

The scoped CI addition has no identified blocking issues; runtime checks were not rerun during this review.

Review effort: Balanced
Findings: None

What changed in this PR

Adds an offline Markdown link gate ahead of #578’s architecture-citation migration, leaving the existing checker unchanged.

Changes:

Adds a SHA-pinned lychee CI job.
Checks local files and anchors, excluding Sphinx sources and GitHub advisory links.

File	Description
`lychee.toml`	Configures offline checks and exclusions.
`.github/workflows/ci.yml`	Adds link checking outside manual E2E dispatches.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

lychee.toml's comment said it replaces architecture_doc.rs's checker; that checker is untouched and still runs, so say "first step toward replacing" instead. Also add the docs-links gate to CONTRIBUTING.md's list of enforced CI invariants, next to check-architecture-doc. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

docs/architecture.md's backtick-quoted path citations (e.g. `main.rs`) are now explicit relative markdown links, checked by the lychee gate added in ROCm#579 instead of xtask/src/architecture_doc.rs's hand-rolled parser. That parser existed to disambiguate a bare citation against the repo's 17 files named main.rs/lib.rs; a link carries its own full path, so there's nothing left to disambiguate. Deletes the ~1,500-line parser along with the heuristics it kept growing (possessive-owner chains, sentence-boundary detection, an abbreviation list, a prose-slash-word list) and its CI job/prek hook — the root cause behind ROCm#492, ROCm#440, and ROCm#524. Drops a stale "arg/" from apps/rocmd's common.rs entry as a drive-by: parse_gpu_indices_arg/optional_arg moved back to lib.rs in ROCm#479's review fixup and the doc wording was never updated. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

rominf

🔴 Automated review · pr-review-watcher · b9a93cd

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Review — needs work

Full review of the whole change.
Implements step 2 of #578 (add the lychee CI gate), not the citation rewrite or the architecture_doc.rs removal. The PR does not claim to close #578, which is correct.

Blocking

(new) CONTRIBUTING.md says every link is checked, but most links are not — CONTRIBUTING.md:82, lychee.toml:11, lychee.toml:19-21, lychee.toml:29-31
The new sentence reads: "Every markdown link and #anchor fragment in the repo is checked by the docs-links CI job … which fails if one doesn't resolve." The config says otherwise, and the config is what governs because it is what CI runs:
- offline = true skips every external http(s) link.
- exclude_path = ['^docs/rocm-docs/'] skips 7 of the repo's 34 markdown files.
- exclude skips the SECURITY.md advisory links.
The CI run for b9a93cd confirms it: Total 61, Successful 18, Excluded 43. So about 70% of the repo's links are never checked. This is the same kind of overclaim the fix commit set out to correct in lychee.toml, and AGENTS.md §5 requires behaviour claims to be checked against the code path. A contributor reading this line will think external links and the Sphinx tree are covered.
Confidence 100 · mechanical · Fix: narrow the sentence. For example: "Every local (relative-path) markdown link and #anchor fragment outside docs/rocm-docs/ is checked by the docs-links CI job (lychee.toml, offline; external URLs are not checked) …"
(new) The PR description does not mention the CONTRIBUTING.md change — CONTRIBUTING.md:82
The Summary, Scope and Test plan cover only ci.yml and lychee.toml. The contributor-facing doc edit from b9a93cd is not listed, and it is the line that carries the overclaim above.
Confidence 85 · mechanical · Fix: add one line to the description noting the CONTRIBUTING.md addition, in its corrected wording.

Non-blocking

None

Decisions for the author

Offline-only checking — tradeoff
offline = true keeps the gate deterministic and safe from rate limits. The cost is that dead external URLs (43 of 61 links today) stay invisible. The description already defers network checking as a follow-up. Confirm that leaving external links unchecked is intended for now, and make sure the CONTRIBUTING wording reflects it.

Positive signals

Both action pins resolve exactly to their tags: lychee-action v2.9.0 → e7477775… and checkout v7.0.1 → 3d3c42e5…. Both are lightweight tags pointing straight at commits, and each pin carries the # vX.Y.Z comment AGENTS.md §6 requires.
The docs/rocm-docs/ exclusion actually works, and it is safe. docs/rocm-docs/getting-started.md links commands.md#model-serving, but commands.md has no such heading; it only {include}s README. That link would fail if the exclusion were not taking effect, and CI reports 0 errors. That tree stays covered by docs-build, whose path filter includes README.md, CONTRIBUTING.md and docs/vllm.md, the files it includes.
Running the job without a path filter matches the architecture-doc job's reasoning, and the comment says why.

Deployment notes

The docs-links job blocks merges only if its check context, Markdown links resolve (lychee), is added to branch protection's required status checks. That needs an admin. I did not verify the current protection settings.

What this covered

I read all 3 changed files completely (.github/workflows/ci.yml, CONTRIBUTING.md, lychee.toml) against prw-base 830f379…b9a93cd8. I also read the neighbouring architecture-doc and docs-build jobs, the changes path filter, docs/rocm-docs/conf.py, the lychee v0.24.2 README for the meaning of include_fragments, the CI job log for the lychee run, and the statusCheckRollup. All checks are green at b9a93cd.

Did not run:

Independent fan-out: I chose single-context mode because the change is 52 lines across 3 files. The design questions were also answered in this same context, so they were not independent.
Prior-changes pass: not run, to limit cost.
Local lychee run: not done, because downloading the lychee binary was denied. The checks on exclusions and coverage are therefore static, cross-checked against the CI run's output.
Cargo build: not needed, since only CI and config files changed.

Not reconciled against any discussion here.

The docs-links line said every link in the repo is checked, but lychee.toml runs offline (no external https:// links) and excludes docs/rocm-docs/ (Sphinx/MyST source, covered by docs-build instead). Say what the job actually covers and point at lychee.toml for the exclusion list, instead of overclaiming. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd · 2026-10-07T10:49:01Z

Addressed in 84c8a8aa:

CONTRIBUTING.md:82 overclaim — rewritten to match lychee.toml's actual scope
(local/relative links + anchors only, outside docs/rocm-docs/, offline — no external
https:// checking), pointing at lychee.toml for the current exclusion list instead
of re-stating it inline (so the two can't drift apart again).
PR description — updated in place (not appended) to mention the CONTRIBUTING.md
line and its corrected wording, and to note the prek run --files CONTRIBUTING.md pass.
Offline-only decision — confirmed intentional; the description's Scope section
already defers external-link checking as a follow-up, unchanged by this fix.

No regression test added for the CONTRIBUTING.md change: it's a doc-accuracy wording fix
with no behavior change, so there's nothing to regress.

docs/architecture.md's backtick-quoted path citations (e.g. `main.rs`) are now explicit relative markdown links, checked by the lychee gate added in ROCm#579 instead of xtask/src/architecture_doc.rs's hand-rolled parser. That parser existed to disambiguate a bare citation against the repo's 17 files named main.rs/lib.rs; a link carries its own full path, so there's nothing left to disambiguate. Deletes the ~1,500-line parser along with the heuristics it kept growing (possessive-owner chains, sentence-boundary detection, an abbreviation list, a prose-slash-word list) and its CI job/prek hook — the root cause behind ROCm#492, ROCm#440, and ROCm#524. Drops a stale "arg/" from apps/rocmd's common.rs entry as a drive-by: parse_gpu_indices_arg/optional_arg moved back to lib.rs in ROCm#479's review fixup and the doc wording was never updated. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

rominf

Thank you!

docs/architecture.md's backtick-quoted path citations (e.g. `main.rs`) are now explicit relative markdown links, checked by the lychee gate added in ROCm#579 instead of xtask/src/architecture_doc.rs's hand-rolled parser. That parser existed to disambiguate a bare citation against the repo's 17 files named main.rs/lib.rs; a link carries its own full path, so there's nothing left to disambiguate. Deletes the ~1,500-line parser along with the heuristics it kept growing (possessive-owner chains, sentence-boundary detection, an abbreviation list, a prose-slash-word list) and its CI job/prek hook — the root cause behind ROCm#492, ROCm#440, and ROCm#524. Drops a stale "arg/" from apps/rocmd's common.rs entry as a drive-by: parse_gpu_indices_arg/optional_arg moved back to lib.rs in ROCm#479's review fixup and the doc wording was never updated. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

docs/architecture.md's backtick-quoted path citations (e.g. `main.rs`) are now explicit relative markdown links, checked by the lychee gate added in ROCm#579 instead of xtask/src/architecture_doc.rs's hand-rolled parser. That parser existed to disambiguate a bare citation against the repo's 17 files named main.rs/lib.rs; a link carries its own full path, so there's nothing left to disambiguate. Deletes the ~1,500-line parser along with the heuristics it kept growing (possessive-owner chains, sentence-boundary detection, an abbreviation list, a prose-slash-word list) and its CI job/prek hook — the root cause behind ROCm#492, ROCm#440, and ROCm#524. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The intro paragraph's guarantee claimed every citation in the doc was linked, but directory headings, the Module organization convention section's illustrative main.rs/lib.rs mentions, and this paragraph's own lychee.toml/Cargo.toml references never were. Scope it to the Module map's per-crate inventories and name the two exceptions inline instead of only in the PR description. CONTRIBUTING.md picked up two adjacent paragraphs describing the same docs-links/lychee gate once ROCm#579 merged: this PR's architecture.md- specific one and ROCm#579's general one. Folded the specific detail into the module-organization pointer and kept ROCm#579's general paragraph. lychee.toml's header still described this PR as a not-yet-landed follow-up; updated now that it's the current state. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

docs/architecture.md's backtick-quoted path citations (e.g. `main.rs`) are now explicit relative markdown links, checked by the lychee gate added in ROCm#579 instead of xtask/src/architecture_doc.rs's hand-rolled parser. That parser existed to disambiguate a bare citation against the repo's 17 files named main.rs/lib.rs; a link carries its own full path, so there's nothing left to disambiguate. Deletes the ~1,500-line parser along with the heuristics it kept growing (possessive-owner chains, sentence-boundary detection, an abbreviation list, a prose-slash-word list) and its CI job/prek hook — the root cause behind ROCm#492, ROCm#440, and ROCm#524. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

The intro paragraph's guarantee claimed every citation in the doc was linked, but directory headings, the Module organization convention section's illustrative main.rs/lib.rs mentions, and this paragraph's own lychee.toml/Cargo.toml references never were. Scope it to the Module map's per-crate inventories and name the two exceptions inline instead of only in the PR description. CONTRIBUTING.md picked up two adjacent paragraphs describing the same docs-links/lychee gate once ROCm#579 merged: this PR's architecture.md- specific one and ROCm#579's general one. Folded the specific detail into the module-organization pointer and kept ROCm#579's general paragraph. lychee.toml's header still described this PR as a not-yet-landed follow-up; updated now that it's the current state. Signed-off-by: Jussi Elo <jussi.elo@amd.com>

…er (ROCm#582) * docs(architecture): rewrite citations as links, delete arch-doc checker docs/architecture.md's backtick-quoted path citations (e.g. `main.rs`) are now explicit relative markdown links, checked by the lychee gate added in ROCm#579 instead of xtask/src/architecture_doc.rs's hand-rolled parser. That parser existed to disambiguate a bare citation against the repo's 17 files named main.rs/lib.rs; a link carries its own full path, so there's nothing left to disambiguate. Deletes the ~1,500-line parser along with the heuristics it kept growing (possessive-owner chains, sentence-boundary detection, an abbreviation list, a prose-slash-word list) and its CI job/prek hook — the root cause behind ROCm#492, ROCm#440, and ROCm#524. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * chore: drop pulldown-cmark, orphaned by architecture_doc.rs's deletion architecture_doc.rs was its only consumer; no .rs file in the workspace references it anymore. Cargo.lock updated accordingly. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * chore: regenerate dependency inventories after pulldown-cmark removal cargo xtask manifest --check and cargo xtask tpn --check both failed after the prior commit dropped pulldown-cmark without regenerating the two inventories that still listed it. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs(architecture): link the remaining repeated file citations Several files already introduced with a link were repeated afterward as bare backtick spans (agent/mod.rs, app/mod.rs and siblings, parse.rs, main.rs, common.rs/lib.rs, the vLLM/lemonade per-file breakdowns). A rename only breaks the first, linked mention; lychee would stay green while the bare repeats quietly went stale. Also links app/chat.rs, app/slash.rs, and app/summary.rs, named but never linked at all. Scope stays inside the Module map's per-crate inventories, where the guarantee in the intro paragraph applies; the Module organization convention section's illustrative `main.rs`/`lib.rs` mentions remain bare, same as the two documented exceptions (agent.rs, report.json). Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs: narrow the link-everywhere guarantee, dedupe docs-links mentions The intro paragraph's guarantee claimed every citation in the doc was linked, but directory headings, the Module organization convention section's illustrative main.rs/lib.rs mentions, and this paragraph's own lychee.toml/Cargo.toml references never were. Scope it to the Module map's per-crate inventories and name the two exceptions inline instead of only in the PR description. CONTRIBUTING.md picked up two adjacent paragraphs describing the same docs-links/lychee gate once ROCm#579 merged: this PR's architecture.md- specific one and ROCm#579's general one. Folded the specific detail into the module-organization pointer and kept ROCm#579's general paragraph. lychee.toml's header still described this PR as a not-yet-landed follow-up; updated now that it's the current state. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * ci: drop stray blank line left after architecture-doc job removal Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs(architecture): name xtask/tests/e2e-cucumber as directory, not file, citations Closes the reviewer's open confirmation: the Module map's scoping paragraph bare-names xtask and tests/e2e-cucumber as workspace directories, not file citations, so the per-crate-inventory link guarantee in the intro doesn't apply to them — same reasoning as the per-crate section headings. Signed-off-by: Jussi Elo <jussi.elo@amd.com> * docs(architecture): drop redundant directory-citation exemption clause Reviewer flagged the clause added in 2906f8f as unnecessary: the Module map's scope paragraph sits before any per-crate inventory, so it was never covered by the link-everywhere guarantee the clause was defending against. It was also uneven, since the same sentence bare-names a third directory (crates/e2e-report) the clause didn't mention. Reverting to the prior wording. Signed-off-by: Jussi Elo <jussi.elo@amd.com> --------- Signed-off-by: Jussi Elo <jussi.elo@amd.com>

jussielo-amd requested a balanced review from Copilot October 7, 2026 08:14

Copilot started reviewing on behalf of jussielo-amd October 7, 2026 08:15 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

jussielo-amd mentioned this pull request Oct 7, 2026

docs(architecture): rewrite citations as links, delete arch-doc checker #582

Merged

jussielo-amd marked this pull request as ready for review October 7, 2026 09:27

jussielo-amd requested a review from a team as a code owner October 7, 2026 09:27

jussielo-amd requested a review from volen-silo October 7, 2026 09:27

jussielo-amd requested a review from rominf October 7, 2026 10:17

rominf requested changes Oct 7, 2026

View reviewed changes

jussielo-amd requested a review from rominf October 7, 2026 12:06

rominf approved these changes Oct 7, 2026

View reviewed changes

jussielo-amd added this pull request to the merge queue Oct 7, 2026

Merged via the queue into ROCm:main with commit 58c03df Oct 7, 2026
25 checks passed

jussielo-amd deleted the issue-578-lychee-migration branch October 7, 2026 12:33

rominf mentioned this pull request Oct 8, 2026

chore(hooks): close hawkeye pin/coverage gaps flagged on #474 #475

Merged

rominf mentioned this pull request Oct 9, 2026

docs: copyedit README, CONTRIBUTING, and vLLM; add uninstall and verify guidance #616

Open

5 of 8 tasks

Navigation Menu

Repository navigation

ci: add lychee link-check gate for markdown - #579

ci: add lychee link-check gate for markdown#579
jussielo-amd merged 3 commits into
ROCm:mainfrom
jussielo-amd:issue-578-lychee-migration

jussielo-amd commented Oct 7, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

rominf left a comment

Uh oh!

jussielo-amd commented Oct 7, 2026

Uh oh!

rominf left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jussielo-amd commented Oct 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Scope

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟢 Approval recommended

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Review — needs work

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Uh oh!

jussielo-amd commented Oct 7, 2026

Uh oh!

rominf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jussielo-amd commented Oct 7, 2026 •

edited

Loading