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

[jussielo-amd]

Summary

xtask/src/architecture_doc.rs validated docs/architecture.md's backtick-quoted path
citations (e.g. `main.rs`) against the tracked tree, and had grown to ~1,500 lines of
prose-inference heuristics to disambiguate which crate a bare citation belongs to (the
repo has 17 files literally named main.rs/lib.rs) — possessive-owner chains,
sentence-boundary detection, an abbreviation list, a prose-slash-word list. #492, #440,
and the test-isolation bug in #524 all trace back to that complexity.

This rewrites every citation in docs/architecture.md as an explicit relative markdown
link and deletes the checker outright: a link carries its own full path, so there's
nothing left to disambiguate. That includes every repeated mention of an already-linked
file, not just its first appearance — a bare repeat would stay unchecked by lychee even
after the file moved. #579 added the docs-links CI job (lychee) this relies on; this
branch is now rebased onto main past that merge, so the job runs for real on this PR
(confirmed green) instead of only being promised.

Also removes pulldown-cmark from xtask/Cargo.toml and the workspace's
[workspace.dependencies]: architecture_doc.rs was its only consumer anywhere in the
tree. MANIFEST.md and THIRD_PARTY_NOTICES.txt are regenerated to match.

Scope

Two citations are deliberately not links: "the old agent.rs" in the rocm-dash-tui
entry (historical prose about a file that no longer exists — linking it would be a dead
link to something that was never meant to resolve), and report.json in the
crates/e2e-report entry (cucumber's generated output, not a tracked source file — a
pre-existing blind spot the old checker never validated either). Both exceptions are now
named inline in the doc's own intro paragraph, not just here.

The link-everywhere guarantee covers the Module map's per-crate inventories. Also
intentionally bare, and now said so in the doc itself: the Module organization convention
section's generic `main.rs`/`lib.rs`-style illustrations (e.g. "they should not
grow inside main.rs/lib.rs", which names no specific file — same category the old
checker's abbreviation-list reasoning existed to separate from a real citation); the
apps/rocm-style section headings that name a directory rather than a file (the old
checker validated these too, as bare directory names — a check this migration doesn't
replicate, since a directory doesn't go stale the way a file citation does); and this
intro paragraph's own lychee.toml/Cargo.toml tool references.

Not included: a lint guarding against a new bare citation reintroducing the unchecked
form. A regex that reliably tells a real citation apart from this doc's generic pattern
references ends up needing the same kind of inference this migration removes. A new bare
citation is a literal, reviewable string in the diff — the same reasoning #578's proposal
already applies to a citation that resolves to the wrong file.

Test plan

• docs-links (lychee) CI job: green on this branch's current head — the real gate added
  by ci: add lychee link-check gate for markdown #579, not a local approximation.
• cargo test -p xtask: 223 passed (was 271 before this PR; removes architecture_doc's
  46 tests plus 2 others no longer applicable).
• cargo clippy -p xtask --all-targets -- -D warnings, cargo fmt --check: clean.
• cargo check -p xtask --all-targets after removing pulldown-cmark: clean, Cargo.lock
  updated (12 lines removed, no other crate referenced it).
• cargo xtask manifest --check / cargo xtask tpn --check (cargo-about 0.9.1, pinned):
  both pass — MANIFEST.md/THIRD_PARTY_NOTICES.txt regenerated, pulldown-cmark gone
  from both.
• Confirmed no remaining reference to architecture_doc/check-architecture-doc anywhere
  in the tree (docs, CI, prek config, xtask source).

Not committed: cargo xtask coverage --bless. Removing ~1,500 lines and 46 tests
changes the xtask crate's measured floor in coverage-floors.toml. I did run the full
cargo llvm-cov --workspace measurement this needs, but coverage-floors.toml hasn't
been re-blessed since it was introduced in #482, so the result moved all 13 crates' numbers
— most by margins this PR's diff doesn't touch (e.g. rocm-core's measured line count alone
shifted by +4177 lines from unrelated merged work). Committing that wholesale would fold
unrelated accumulated drift into this PR under this PR's name. Keeping the original plan
instead: CI's Coverage (rocm-dash crates, ratcheted) job (its name is stale — it covers
every workspace crate, including xtask) reports the real number against the now-stale
floor; if it fails, a follow-up commit re-blesses it against that measurement.

[jussielo-amd]

Depends on #579 for the docs-links CI job and lychee.toml this PR's rewritten links need checked. Not stacked on its branch (this is based on main directly) since #579 is additive and doesn't touch any file this PR touches — no rebase needed either way, but #579 landing first means the links get checked continuously rather than after a gap.

[Copilot]

Copilot review overview

🟡 Changes recommended

Generated inventories and the coverage floor are stale, and several concrete file citations remain unlinked.

Review effort: Balanced
Findings: 2 · 1

Open (3)

• Regenerate dependency inventories after removal · New
• Update xtask coverage floor after checker deletion · New
• Convert remaining file references to links · New

What changed in this PR

Replaces the custom architecture citation checker with explicit Markdown links intended for validation by #579’s lychee CI job.

Changes:

• Converts architecture paths to relative links and updates contributor guidance.
• Deletes the xtask checker and related CI/pre-push wiring.
• Removes the checker’s pulldown-cmark dependency.

File	Description
docs/​architecture.md	Rewrites path references as links.
CONTRIBUTING.md	Documents lychee-based validation.
xtask/​src/​architecture_doc.rs	Deletes the custom checker and tests.
xtask/​src/​main.rs	Removes the checker command.
xtask/​Cargo.toml	Removes the parser dependency.
Cargo.toml	Removes the workspace dependency.
Cargo.lock	Removes the resolved package.
.pre-commit-config.yaml	Removes the local checker hook.
.github/​workflows/​ci.yml	Removes the old CI job.

---

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

[rominf]

🔴 Automated review · pr-review-watcher · 0a0e7ca

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 steps 1 and 4 of #578 (open): links instead of citations, and the checker deleted. It skips optional step 3 (a lint against bare citations), and the description says so. Step 2 (lychee in CI) arrived separately in #579.

Blocking

• (new) The PR is unmergeable as based, and its stated premise for not rebasing is false — .github/workflows/ci.yml:1316, CONTRIBUTING.md:80
  The description says the branch is "based on main directly, since #579 is additive and touches none of the files this PR touches". That is not so: #579 (58c03df) changes ci.yml, CONTRIBUTING.md and .pre-commit-config.yaml.

  • The PR forks at b863f77, before #579 merged. gh api …/pulls/582 reports mergeable: false, mergeable_state: dirty.
  • git merge-tree --write-tree prw-base HEAD gives CONFLICT (content) in ci.yml. The new docs-links job sits directly after the architecture-doc job this PR deletes.
  • None of this PR's green checks ran the docs-links/lychee gate. The links the doc now relies on have never been checked in CI on this branch.
  • The rebase also has to settle the following (required consequences):
    • docs-links job comment. It says "for the same reason as architecture-doc above". After this PR that job is gone.
    • lychee.toml header. It says the xtask checker "stays in place, running alongside this gate, until a follow-up PR … removes it". This PR is that follow-up.
    • CONTRIBUTING.md overlap. It auto-merges into two overlapping paragraphs about the same gate: this PR's line 80 and #579's line 82. Keep one.

  Confidence 100 · architectural · Fix: rebase onto current main, resolve the ci.yml conflict, update the two stale comments, dedupe CONTRIBUTING.md, and let docs-links run on the result.

• (new) The description claims a drive-by edit that is not in the diff, and its rationale is wrong — docs/architecture.md:34
  The description (and commit b5cf5eb) says the PR "drops a stale 'arg/' from the apps/rocmd common.rs entry — parse_gpu_indices_arg/optional_arg moved back to lib.rs". The edit is not there:

  • Base and head both read "small arg/healthcheck/endpoint-key utilities".
  • The wording is still correct. optional_arg lives at apps/rocmd/src/common.rs:264, and only parse_gpu_indices_arg is in lib.rs:1656.

  Confidence 100 · mechanical · Fix: remove the claim from the description (and from the squash message). The doc wording should stay as it is.

Non-blocking

• (new) The doc's new guarantee is broader than what the PR delivers — docs/architecture.md:11
  The doc says: "Every file this doc cites is a relative markdown link rather than a bare filename". Bare file citations remain in the same doc:

  • lychee.toml (line 11)
  • providers.rs in the parenthetical (line 17)
  • workspace Cargo.toml (line 20)
  • "the old agent.rs"
  • report.json
  • every directory heading (apps/rocm, crates/rocm-core, …), which the old checker verified as bare directory names

  The description scopes the guarantee to "the Module map's per-crate inventories" and names the agent.rs/report.json exceptions. The doc carries neither, and the doc is what a reader trusts. The code governs here: these citations really are unlinked, so the sentence is what is wrong.

  Confidence 85 · mechanical · Fix: narrow the sentence to the scope the description states, naming the exceptions. Or link lychee.toml, providers.rs, Cargo.toml and the directory headings, which lychee resolves as directories.

• (new) Commit messages carry stale or incorrect references — commits 16b8385, 0a0e7ca

  • 16b8385 says inventories failed "after 5041d33 dropped pulldown-cmark". That SHA does not exist in this history; the commit is 173245d.
  • 0a0e7ca calls agent.rs/report.json "the two documented exceptions". They are not documented in the doc.
  • The repo squash-merges, so these matter only if they end up in the squash message.

  Confidence 85 · mechanical · Fix: correct them in the squash message.

Decisions for the author

• Dropping enforcement against bare citations — tradeoff
  • Against: the old checker failed on any bare citation that did not resolve, including directories. With it gone, a new bare backtick citation is unchecked by anything, and so is a born-wrong citation that names the wrong crate. The doc's "every file is a link" claim is a convention, not an invariant.
  • For: about 1,500 lines of heuristic parser and a dependency are deleted. The description argues a regex lint would need the same inference again, and that a bare citation is a reviewable literal in the diff. #578 accepted the born-wrong loss explicitly.
  • Confirm the loss of directory-heading checking is also intended. Neither the description nor #578 mentions it.

Positive signals

• Every one of the roughly 100 relative links resolves at HEAD. Each bare lib.rs/main.rs repeat points at the correct crate's file (rocmd, rocm-core, e2e-report, vllm, lemonade), which is exactly the ambiguity the old parser existed to resolve.
• The removal is complete: job, prek hook, clap variant, dispatch arm, module, workspace dependency, lockfile, and both license inventories. There are zero leftover references in the tree.
• The coverage floor was deliberately not re-blessed, rather than folding unrelated drift into this PR. CI's coverage check passed.

Deployment notes

• After this lands, the only citation guard is docs-links. Neither it nor the old architecture-doc job is in main's required status checks (queried live), so a broken link turns CI red but does not block a merge. This is no regression, but the doc's "fails CI immediately" is advisory until an admin adds the check as required.

What this covered

All files in b863f77…0a0e7ca8 (merge-base…head), read in full. Also read: prw-base (58c03df) versions of lychee.toml, ci.yml's docs-links job, CONTRIBUTING.md, and the deleted architecture_doc.rs.

Checks run:

• git merge-tree against prw-base.
• cargo build -p xtask and cargo clippy -p xtask --all-targets -D warnings: clean.
• cargo xtask manifest --check: passes.
• Local resolution of every link target.
• Live PR mergeable_state, the CI rollup (all green, on the pre-#579 base), and the branch-protection required checks.

Did not run:

• lychee itself (not installed here).
• cargo xtask tpn --check (cargo-about not installed).
• cargo xtask coverage (relied on CI's passing coverage check instead).

How the work was split: fan-out into four workers covering docs/links, checker removal, design questions, and the four source-reading passes. The source-reading passes (instruction adherence, history, prior changes, comments) ran together in one worker rather than independently. One worker's claim that the old check was "presumably required" was refuted by the live query and dropped.

Not reconciled against any PR discussion here.

[jussielo-amd]

Addressing the automated review from @rominf (review id 5442339841, taken at 0a0e7ca8):

Blocking — unmergeable, false "no rebase needed" claim: Fixed. Rebased onto current main (now past #579), resolved the ci.yml conflict (kept the new docs-links job, dropped the architecture-doc job it was sitting next to, which this PR deletes), fixed the now-stale docs-links job comment and lychee.toml header that referenced this PR as a not-yet-landed follow-up, and deduped the two overlapping CONTRIBUTING.md paragraphs describing the same gate (kept #579's general one, folded the architecture.md-specific detail into the module-organization pointer instead). The docs-links (lychee) job has now run for real on this branch and passed — confirmed via gh api .../check-runs, not just the local approximation from before. PR description updated to state this instead of the stale "no rebase needed" claim.

Blocking — false drive-by claim about common.rs's "arg/" wording: Confirmed and fixed. optional_arg is back in apps/rocmd/src/common.rs (added by #483, which landed after this PR's original claim was written), so the "arg/" wording was already correct — the claimed edit was never in the diff and the doc text is right as-is. Removed the false claim from the PR description and reworded the commit message that carried it.

Non-blocking — guarantee broader than delivered: Fixed. Narrowed docs/architecture.md's intro-paragraph guarantee to the Module map's per-crate inventories, named the two exceptions (agent.rs, report.json) inline in the doc itself rather than only in this description, and documented the three categories it doesn't cover (the Module organization convention section's generic main.rs/lib.rs illustrations, apps/rocm-style directory headings, and this paragraph's own lychee.toml/Cargo.toml tool references) in the Scope section below.

Non-blocking — stale commit message refs: Fixed. Reworded both: the inventory-regeneration commit no longer cites a SHA that doesn't exist in this history (rephrased to not depend on a specific hash, since this branch has been rebased several times since); the citation-link commit's "documented exceptions" phrase is now literally true, since the exceptions are documented inline in the doc as of the guarantee-narrowing fix above.

Decision — directory-heading checking loss: Confirmed intended, not an oversight. Documented in the Scope section: a directory doesn't go stale the way a file citation does, so this migration doesn't replicate that part of the old checker's coverage.

Deployment note — docs-links not yet a required status check: Acknowledged, no action taken — that's an admin-side branch-protection change, out of scope for this PR.

Fixes applied.

[rominf]

🔴 Automated review · pr-review-watcher · ebac5f3

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 — no blocking findings

Full review of the whole change.
Implements steps 1 and 4 of #578 (citations rewritten as links, checker and its CI job deleted). Step 2, the lychee job, landed earlier in #579. It does not do the optional step 3 (a lint against new bare citations), and the description says so. The description does not claim to close #578.

Blocking

None

Non-blocking

None

Decisions for the author

• (new) Directory citations are no longer enforced — answered-by-intent
  • The old citation_exists checked bare directory names and slash paths. Its own test bare_crate_directory_name_matches_a_path_component (at prw-base:xtask/src/architecture_doc.rs:998) shows this.
  • Now nothing checks these citations: crates/rocm-engine-protocol (docs/architecture.md:44), rocm-dash-core/rocm-dash-collectors/rocm-dash-daemon (:40) and tests/e2e-cucumber (:24). None of them has a linked file underneath, so renaming one would now pass CI.
  • Otherwise this would be a non-blocking coverage regression. The description answers it: "the apps/rocm-style section headings that name a directory rather than a file (the old checker validated these too … a check this migration doesn't replicate, since a directory doesn't go stale the way a file citation does)".
  • That passage covers the headings. tests/e2e-cucumber on line 24 is running prose rather than a heading, so confirm it falls under the same reasoning.
• (new) Enforcement now depends on authors writing links — tradeoff
  • What changes: the old gate caught any new backtick citation, linked or not. Lychee sees only links, so a new bare `foo.rs` passes silently.
  • For the change: a large heuristic engine and its bug history go away, and #578 explicitly accepts leaving that kind of citation to code review.
  • Against: the guarantee in the doc's header (docs/architecture.md:11) now holds only by reviewer discipline, and step 3 of #578 is deferred.
  • The description argues this deliberately, so this item only confirms the choice.
• (new) No local pre-push check replaces the removed hook — tradeoff
  • The architecture-doc hook in .pre-commit-config.yaml caught stale citations before push. No lychee hook replaces it, so the first signal is now the CI job.
  • Neither the old architecture-doc job nor Markdown links resolve (lychee) is in main's required status checks (I queried them live). So the merge gate itself is unchanged, and removing the job leaves no required check stranded.

Positive signals

• Every repeated mention of a file is linked, not just its first one. Without that, a later bare repeat would sit unchecked next to a linked first mention. A scripted check confirmed all 100+ link targets exist and that each label's basename matches its target. Every bare lib.rs/main.rs/install.rs/state.rs points into the crate of the section it sits in.
• The description is unusually exact about scope: which citations stay bare and why, the directory check it gives up, and the coverage floor it chose not to re-bless.

Deployment notes

None

What this covered

• What was read: every file in the change, completely, 24993519…ebac5f32. That includes the deleted xtask/src/architecture_doc.rs, read from prw-base to establish what it enforced.
• Run by workers:
  • cargo clippy -p xtask --all-targets --locked -- -D warnings: clean.
  • cargo test -p xtask --locked: 223 passed.
  • cargo tree -i pulldown-cmark: the package is absent.
  • cargo xtask manifest --check: passes.
  • Tree-wide git grep for leftover references: only the intentional historical note at lychee.toml:4.
• CI at the reviewed head: every check passed except E2E tests (MI300X), which was still queued. Those include Markdown links resolve (lychee) and the coverage gate Coverage (rocm-dash crates, ratcheted). The coverage pass settles whether xtask's floor survives the deletion. The issue's local lychee trial already showed lychee failing on a renamed file.
• Did not run:
  • Lychee locally, because it is not installed. So I did no local break-a-link mutation; the CI run stands in for it.
  • THIRD_PARTY_NOTICES regeneration via cargo-about. That file was checked by reading only, alongside the green Third-party notices current check.
  • The prior-changes pass (review comments on earlier PRs touching these files), deliberately: this run reads no review discussion.
• Not independent: the design questions in Step 7 were answered by the driver from the workers' results, not by separate workers.
• Not reconciled against any PR discussion here.

Standing objections from the change request at 0a0e7ca

Both blockers of the earlier change request (review 5442339841, since dismissed by the author) were standing objections, not re-found blind this round. Each was settled by putting its wording and the author's reply to the reviewer after this report was frozen (anchored evidence), and both point the same way as the blind review's silence:

• Unmergeable as based / false no-rebase premise — no longer holds. #579 is now in the branch's history, git merge-tree against current main is clean, the docs-links comment, lychee.toml header and CONTRIBUTING.md duplication are fixed, the lychee check passed at ebac5f32, and the description no longer claims no rebase was needed. The fork point trails current main, so re-check conflicts if main moves before merge.
• Description claimed an "arg/" drive-by edit not in the diff — no longer holds. Neither the description nor any commit in the range makes the claim, and the doc wording is unchanged and correct.

No blocking objection remains, so this round files no change request.

[jussielo-amd]

Confirmed: tests/e2e-cucumber (and xtask) at docs/architecture.md:24 are bare
directory names in the Module map's scoping prose, not file citations within the
per-crate inventories the link guarantee covers — same reasoning as the directory
section headings.

Named it explicitly in 2906f8f so the doc states this itself rather than leaving
it to inference, matching the intro's existing pattern of naming every bare-citation
exception.

[rominf]

🔴 Automated review · pr-review-watcher · 2906f8f

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 — no blocking findings

Review of changes since the review at ebac5f3, plus the files they touch (docs/architecture.md).
The PR names no ticket. Its description refers to #579, #578, #492, #440 and #524 only as background, and none is a ticket this change claims to close.

Blocking

None

Non-blocking

• (new) The description doesn't list the newly documented exemption — PR description, Scope section; docs/architecture.md:24
  The Scope section lists the citations that are "Also intentionally bare, and now said so in the doc itself": the convention-section main.rs/lib.rs illustrations, the apps/rocm-style section headings, and the intro's lychee.toml/Cargo.toml. Commit 2906f8f adds a fourth case that the doc now states: the Module map scope paragraph's bare xtask and tests/e2e-cucumber. It is the same category as the section headings (a directory, not a file), so no reader is misled about anything substantive. Still, the description's list is no longer complete.
  Confidence 70 · mechanical · Fix: add the scope paragraph's xtask/tests/e2e-cucumber to the Scope list, or reword that list as "directory names, e.g. the section headings and the scope paragraph".

Decisions for the author

• (new) Should the new clause stay at all? — non-blocking-improvement
  The line-11 guarantee covers only "the Module map's per-crate inventories below". Line 24 is the Module map's scope paragraph, which comes before any ### inventory, so it was never inside the guarantee. The new clause therefore explains an exemption that wasn't needed. The clause is accurate.
  It is also uneven. The same sentence bare-names crates/e2e-report, a third directory, but "both" covers only xtask and tests/e2e-cucumber. You can either keep the clause and make it cover every directory name in that paragraph, or drop it. Since the paragraph sits outside "per-crate inventories", it is out of scope by the guarantee's own wording.

Positive signals

None

Deployment notes

None

What this covered

I read git diff ebac5f32..HEAD (2906f8f, one line in docs/architecture.md) and all 58 lines of docs/architecture.md. I checked every bare (unlinked) filename in the Module map inventories against the line-11 exception list. Only agent.rs (line 42, twice) and report.json (line 54) are unlinked, which matches the exceptions. I checked lychee.toml and the docs-links job in .github/workflows/ci.yml against the doc's claims, and they match. I also read the PR description and commit messages for ebac5f3 and 2906f8f. I ran the AGENTS.md leak scan by eye over the changed line, and it is clean. CI rollup at 2906f8f is all green, including Markdown links resolve (lychee), Commit signatures + sign-off, E2E tests and Coverage. Read against prw-base…2906f8f4b1c61407c9c7f67182c660529a9b1154.

I ran it as a single pass, since the change is one line. One independent worker reviewed the file and diff blind. I ran history myself: git log, and I checked the line-11 guarantee against the prw-base version. The agent-instruction pass was also mine.

Did not run:

• Prior-changes pass: it would require reading earlier review discussion, which this run excludes.
• Builds and tests: none needed for a prose-only change.

Not reconciled against any discussion here.

[jussielo-amd]

Addressing the review at `2906f8f4` (review id 5455664569):

Non-blocking — description doesn't list the new exemption: moot now, see below.

Decision — should the new clause stay at all: Dropped it in e6c121b. Agreed with the review's reasoning: the Module map's scope paragraph (line 24) sits before any per-crate `###` inventory, so it was never covered by the link-everywhere guarantee ("per-crate inventories below") to begin with — the clause was defending against something that was never in question. It was also uneven, since the same sentence bare-names a third directory, `crates/e2e-report`, that the clause didn't cover. Reverted to the pre-2906f8f4 wording rather than extending it to cover all three, since the paragraph doesn't need the carve-out at all.

Since the clause is gone, there's nothing new in the doc for the PR description to list either — the first finding is resolved as a side effect.

[rominf]

Approving — links all resolve to the right files and CI was green at the previous head; e6c121b only drops the line-24 exemption clause. One non-blocking nit: lychee.toml:2-4 and CONTRIBUTING.md:76 still describe all of architecture.md's path citations as links, but the section headings and directory names (:20, :24, :26-56) stay bare and unchecked. docs/architecture.md:11 has the accurate scope; worth matching those two to "the per-crate inventories' file citations".