feat(xtask): guard docs/architecture.md path citations in CI

[jussielo-amd]

Summary

• Adds cargo xtask check-architecture-doc: extracts every backtick-quoted
  path citation from docs/architecture.md and fails, naming each stale
  path, if it no longer exists in the tracked tree.
• Runs the new check from its own always-run CI job, independent of the
  rust path filter — the checker can validate a citation to any tracked
  file, not just the ones the doc happens to cite today, so gating it on a
  fixed filter list could skip the check on the very PR that goes stale.
• Same shape as the existing xtask/src/crate_edges.rs and
  xtask/src/workflow_contract.rs structural guards.
• Hardened through several review rounds to match CommonMark's actual
  rules for fenced code blocks, ATX heading indentation, and code-span
  delimiter matching (rather than simplified approximations of each), and
  to scope possessive/partial-suffix citations precisely instead of
  falling back to an overly broad match.
• Split run's filter-and-bail into a pure check_citations(markdown, tracked) so the gate's own failure branch is directly testable, and
  deduplicated the failure message so a file cited both bare and under a
  section heading is reported once, not twice.
• Closed a further gap in the same vein: run's own delegation to
  check_citations was itself unproven, so split out check_doc_at(root)
  and tested it end to end over a throwaway git repo. Also fixed a real bug
  the dedupe surfaced — the failure message hinted a directory for
  .md/.toml citations even though existence-checking never scopes those
  by section — and documented the new check in CONTRIBUTING.md and
  docs/architecture.md's own caution note.

Why

docs/architecture.md (added in #423) tells contributors to verify its
cited paths manually because nothing currently checks that they still
exist — a renamed or removed file rots silently in the doc until a reader
notices. Filed as #430, a follow-up to #423 (which stayed documentation-only
per its stated scope).

Non-obvious decisions

• The doc mixes real path citations with backtick-quoted Rust syntax
  (`crate::`, `pub(crate) fn`, bare type names like
  `ActionReport`) and cites some files by bare name only
  (`main.rs`, `lib.rs`), trusting surrounding prose/headings for
  which subsystem they belong to. The extraction heuristic and its known,
  deliberate blind spots (documented in the module's doc comments) were
  built and tested directly against the real doc's content rather than
  designed in the abstract.
• A bare .rs citation (or a partial slash-path suffix like `app/mod.rs`)
  is scoped to its nearest ### \`heading's directories, or to a single crate narrowed out of a multi-crate heading by possessive prose (``rocm-dash-tui's agent.rs``), rather than matched anywhere in the repo — the workspace has ~17 files literally namedmain.rs/lib.rs`, so
  an unscoped check would be a near no-op for exactly the citations it most
  needs to catch.
• Non-goal (per the issue): validates only that a cited path still exists,
  not prose accuracy (e.g. which extraction pattern a module follows).

Risk: low

Dev-tooling only (xtask + CI wiring); no change to shipped CLI/daemon
behavior. docs/architecture.md itself is explicitly out of scope for
xtask per its own module-map scoping note.

Test plan

• cargo test -p xtask — 68 unit tests in this module (including several
  regression tests pinned to real doc content, review-driven CommonMark
  edge cases, and direct tests of check_citations's and check_doc_at's
  failure branches), plus a real-tree test that runs the actual check
  against docs/architecture.md and asserts it passes.
• cargo xtask check-architecture-doc run manually against the doc (passes).
• Manually verified end-to-end by renaming a cited file
  (apps/rocmd/src/lib.rs, then engines/vllm/src/lib.rs) while leaving a
  same-named sibling file untouched, confirming the check fails and names
  the specific stale path — each time reverted before committing.
• cargo test --workspace, cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --check all clean.

PR checklist

• N/A — not a bug fix, no expectations.toml xfail rows involved.
• N/A — no new subcommand/subsystem in the shipped CLI; xtask is
  explicitly out of scope for docs/architecture.md's own module map.

Closes #430

[Copilot]

Copilot review overview

🟡 Changes recommended

The parser can ignore valid paths and mishandle valid or malformed CommonMark fences.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Allowlisted extensions miss valid bare path citations · New

Resolved since last review (3)

• Parse Markdown code spans using matching backtick runs
• Incorrectly parse four-space indented code as ATX headings
• Update PR description to reflect the current implementation

[jussielo-amd]

An independent code review surfaced 4 correctness bugs in the checker itself (each verified with a targeted repro against the real doc before fixing, in f27c73e):

• is_hyphenated_bare_word couldn't distinguish a crate name from ordinary hyphenated prose (`read-only`) — false-failed CI on such prose. Now only treated as a citation when declared in a heading or immediately preceding 's.
• A bare scoped citation (agent.rs) repeated later under the same heading without a connector lost its possessive narrowing and fell back to requiring existence in every crate the heading lists. A per-heading map now reuses the earlier narrowing.
• is_directory_shaped only excluded BARE_FILE_EXTENSIONS from the directory check, so a full path with any other extension (e.g. ci.yml) was misread as a directory, making anything scoped to it unmatchable. Generalized to any dotted last path segment.
• tracked_files didn't disable core.quotePath, so a non-ASCII tracked filename would come back git-escaped and never match a plain citation.

Also collapsed a duplicated SCOPED_BARE_EXTENSIONS check in citation_exists. Added a regression test per bug (59 unit tests total now, up from 56); full workspace test suite (209) + clippy + fmt all clean.

[Copilot]

Copilot review overview

🟡 Changes recommended

Citation extraction has unresolved Unicode, heading-state, and indented-code-block correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3

Open (3)

• Rejects Unicode filenames before tracked file validation · New
• Parses citations inside indented CommonMark code blocks · New
• Clears narrowed owners too late between headings · New

Resolved since last review (1)

• Allowlisted extensions miss valid bare path citations

[jussielo-amd]

Follow-up review found 3 more real bugs (verified with repros before fixing, in 5d62cef):

• is_path_safe rejected every non-ASCII char, so a citation like `café.rs` was discarded at extraction — before last round's core.quotePath=false fix in tracked_files ever ran, making that fix unreachable. Widened to char::is_alphanumeric().
• Only fenced code blocks were skipped; a 4+-space indented CommonMark code block was still parsed, so a removed-path shown as indented example could false-fail CI. Now skipped, including the "can't interrupt a paragraph" rule.
• narrowed_owners was cleared after (not before) a heading's own citations were processed, so a new heading citing a filename a prior section had narrowed inherited that stale scope — and, via the BTreeSet's dedup, silently merged into the old entry, leaving a missing file under the new heading undetected.

All 3 threads resolved. 213 workspace tests pass (63 in this module, up from 59), clippy/fmt clean.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5d62cef

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.

Summary

Adds cargo xtask check-architecture-doc, which fails CI when a backtick-quoted path citation in docs/architecture.md no longer exists in the git-tracked tree, and wires it into an always-run CI job. The checker is genuinely sound and unusually well tested; one blocking gap remains — the gate's own failure branch has no test, so a future refactor could disarm it with CI still green. Verified: in a scratch copy I planted bad citations four ways (a nonexistent slash path, and untracking apps/rocmd/src/lib.rs, apps/rocm/src/providers.rs, engines/vllm/src/lib.rs) — each was caught, named precisely, and exited 1, while the clean tree exited 0; I then ran 38 single-branch mutations against the extraction, fence/heading/code-span parsing, possessive-scoping and existence-matching logic, of which 36 were killed by the suite and 2 survived (reported below); I also dumped the extractor's real output over the doc and confirmed it checks 53 citations out of 81 backticked spans, with only one real path (a bare non-hyphenated directory name) skipped, which is deliberate and documented — coverage is real, not cosmetic. Not run: the full test suite, e2e, or anything beyond cargo test -p xtask architecture_doc in the scratch copy; the checkout under review was left untouched. The gate is sound as shipped, so the all-green CI is not contradicted — the blocking item is a missing regression test, not a live defect. Working from CI check counts at this head: 21 success, all green. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

xtask/src/architecture_doc.rs:732 — the gate's own failure branch is untested. Replacing if !stale.is_empty() { bail!(...) } with a no-op leaves the entire suite green. The only test that drives run() end to end is run_passes_against_the_real_doc at xtask/src/architecture_doc.rs:1743, which asserts only the Ok direction and would also pass vacuously against an empty citation set, so nothing in the suite would notice if a refactor stopped the check from ever failing — precisely the "green and silently disarmed" outcome this gate exists to prevent, and the kind of thing nobody revisits once it is merged. The fix is small: lift the filter-and-bail composition out of run() into a pure function over the markdown and the tracked-path list (e.g. a "check(markdown, tracked) -> Result<()>" that run() calls), then add a test asserting it returns Err naming a path that is absent from the tracked list, alongside the existing Ok case. Roughly ten lines, and it kills the surviving mutant.

Non-blocking

• xtask/src/architecture_doc.rs:343 — the if !is_scoped_extension { return None; } guard in possessive_owner_for also survives single-branch mutation with the suite green; I confirmed its only observable effect is the "(expected under ...)" hint printed for a stale non-.rs citation, never the pass/fail verdict, so it is diagnostic-only — worth either a test on the hint text or a line saying it is defensive.
• xtask/src/architecture_doc.rs:169 — any backticked prose span containing a slash becomes a checked citation unconditionally, so ordinary wording such as "read/write", "and/or" or "GPU/CPU" in the doc fails the build; I reproduced all three. Hyphenated prose got an explicit guard at xtask/src/architecture_doc.rs:574; this equivalent trap has neither a guard nor a note, and the only escape hatch is a code fence.
• xtask/src/architecture_doc.rs:723 — a file cited both under a directory heading and outside one is reported twice in the same failure message (I saw providers.rs listed once bare and once as "expected under apps/rocm"), which reads as two stale paths when it is one.
• .github/workflows/ci.yml:1135 — the job runs on push, pull_request and merge_group as intended, but nothing in the repository makes it a required check; that registration lives in branch-protection settings outside the repo and no follow-up names it, so until someone adds it the gate can go red without blocking a merge.
• xtask/src/architecture_doc.rs:337 — possessive_owner_for and narrowed_owners parse English possessive grammar to scope bare filenames, which no other checker in xtask/ does, and a rephrasing as innocuous as "agent.rs and app/mod.rs in rocm-dash-tui" would silently mis-scope rather than fail loudly; citing full repository-relative paths in the doc would remove the need for the machinery entirely. The hand-rolled CommonMark handling around it is defensible — the workspace has no markdown-parsing crate, so avoiding it would mean taking on a new third-party dependency and its notice obligations.

[siloteemu]

🔴 Automated review · pr-review-watcher · 5d62cef

Blocking: 1. This is the first round of review from this account on this pull request.

xtask/src/architecture_doc.rs:732 — the gate's own failure branch has no test, so a future change can disarm the gate with CI still green.

Replacing the filter-and-bail with a no-op leaves the entire suite passing. The only test that drives the entry point end to end asserts the success direction, and it would also pass vacuously against an empty citation set, so nothing in the suite would notice if the check stopped being able to fail. That is the exact outcome this gate exists to prevent, and a permanent build gate is the worst place to leave it, because nobody reopens a landed change to strengthen one.

To be clear about what is NOT wrong here: the checker itself is sound and unusually well tested. Planting a bad citation four different ways — a path that does not exist, and three real files removed from tracking — was caught each time, named precisely, and exited non-zero, while the clean tree passed. Of 38 single-branch mutations across the extraction, fence and code-span parsing, scoping and existence-matching logic, 36 were killed by the suite. The extractor checks 53 citations, with the single skipped path deliberate and documented. The all-green checks are not contradicted: this is a missing regression test, not a live defect.

The fix is about ten lines: lift the filter-and-bail composition out of the entry point into a pure function over the markdown and the tracked-path list, then assert it returns an error naming a path absent from that list, alongside the existing success case.

Five non-blocking items, including a prose-citation trap that fails the build on ordinary wording containing a slash, are in the review comment posted alongside this one.

Superseded: verified @siloteemu's automated review is correct that run()'s failure branch (bail!() on stale citations) is untested, which is a real blocking gap. Replacing this approval with a request-changes review.

[siloteemu]

🔴 Automated review · pr-review-watcher · feb99ed

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.

Summary

This PR adds a CI gate (cargo xtask check-architecture-doc, plus its own always-run workflow job) that extracts backtick-quoted path citations from docs/architecture.md and fails naming any that no longer exist in the tracked tree — needs work: the gate also fails on citations whose paths do exist, and all three pieces of new user-facing text say otherwise. Verified: ran the new checker's own test module at this head (71 passed), then thirteen single-line mutations in a throwaway copy — twelve were each killed by a specifically named test (the fence scanner, the heading/indent rules, the code-span splitter, the root-level and scoped-extension branches, the multi-directory rule, the dedupe, and the hint condition are all genuinely covered, so the suite is strong), and the one survivor is run's single delegation expression, which can still be replaced with let _ = …; Ok(()) leaving every test green — the prior round's disarm finding is unchanged at this head. I also confirmed by reverting docs/architecture.md:58 to its base wording that the gate then reports four files as no longer existing when all four are present in the tree, which is what the blocking item below rests on; and I re-read the extractor's termination and alternation invariants by hand. The full test suite, the lint pass and the end-to-end suites were not run here. Working from 28 successful and 1 pending check at this head, with no failures. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• xtask/src/architecture_doc.rs:771, CONTRIBUTING.md:74, docs/architecture.md:11 — the gate fails on citations whose paths exist, but its failure message and both new contributor-facing sentences state the opposite. A bare .rs citation under a heading that names two directories must exist in every one of them (a deliberate rule, locked by multi_directory_citation_requires_every_directory_to_have_the_file), so accurate prose under a two-crate heading fails the gate. This is not hypothetical: I reverted docs/architecture.md:58 to the wording at the base tip — factually correct prose — and the checker reported docs/architecture.md cites 4 path(s) that no longer exist in the tree for four files that are all present under engines/vllm/src/. The last commit in this PR is the author working around exactly that, rewriting true prose into a possessive slash-chain so the narrowing heuristic fires; the commit message explains the checker's requirement but nothing user-facing does. So a contributor who writes correct prose gets a merge-blocking failure asserting their files are gone, with no documented way to know why. The fix is small and entirely in text: make stale_message say the cited path was not found where the citation is scoped to rather than that it "no longer exist[s] in the tree" (the per-citation (expected under …) hint already carries the scope, so the summary line just needs to stop contradicting it), and change both CONTRIBUTING.md:74 and docs/architecture.md:11 to say the gate checks citations in the shapes it recognises, against the directories its nearest heading names — not "a backtick-quoted path … no longer exists in the tracked tree". Docs that overstate a guard are the defect shape this very PR exists to prevent, and it is in the direction that makes the change look safer than it is.

Non-blocking

• xtask/src/architecture_doc.rs:722-730 — run's wiring of the workspace root into check_doc_at is still unproven: discarding its result leaves all 71 tests green, so the CI gate can be silently disarmed. This is the same irreducible single line the prior round flagged and is fairly a stopping point, but a clause on run's doc comment naming what stays unproven would stop the next reader re-deriving it.
• xtask/src/architecture_doc.rs:61 — the comment asserts "the doc doesn't cite any other extension bare today", which is false now: docs/architecture.md:54 cites `report.json` bare, so it is silently never checked. Reword to name the exception rather than deny it exists.
• xtask/src/architecture_doc.rs:595, :687, :813 — is_scoped_extension is still computed identically in three places; the bug fixed in this PR was precisely two of them drifting apart, and a shared predicate would prevent the next divergence.
• xtask/src/architecture_doc.rs:520 — prev_line_blank is never updated for a fence delimiter or for lines inside a fence (both paths continue first), so it stays stale across a whole fenced block; an indented citation on the line right after a closing fence that followed a blank line is dropped rather than extracted. No such construct exists in the doc today, but it undercuts the "matches real markdown rules" claim.
• xtask/src/architecture_doc.rs:942, :1836 — the run_git test closure is still duplicated verbatim between the two temp-repo tests; a shared helper would be better than a second copy.

[siloteemu]

🔴 Automated review · pr-review-watcher · 15cf21a

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.

Summary

This PR adds a CI gate (cargo xtask check-architecture-doc, plus its own always-run job) that extracts backtick-quoted path citations from docs/architecture.md and fails naming any not found where they're cited — needs work: the round-3 overclaim is fixed in three of five places, left verbatim in the other two, and the replacement wording introduces a new inaccuracy I refuted by construction. Verified: ran the module's tests at this head (69 passed), then four single-line mutations on a scratch copy — reverting the fence/blank-line parser to its pre-fix form makes the new regression test the sole failure, so that test genuinely pins its production change; breaking the newly extracted is_scoped_extension is killed by ten named tests, so the de-duplication is safe; and run's delegation can still be replaced with let _ = …; Ok(()) leaving all 69 green, unchanged from the prior round. I re-ran the round-3 repro (base wording of the engines/vllm paragraph): the gate still fails, but now says "not found where they're cited" and hints the scope, so the summary line no longer contradicts the hint. I then constructed three fresh inputs against the gate: a bare `testing.md` citation fails even though that file is tracked (bare .md/.toml match the repo root only, not "anywhere in the tracked tree"), a bare `Cargo.toml` passes, and a bare `totally-bogus.json` slips past silently. I also checked the twin question directly: there is no second workflow, no required-check aggregator job, and the local hook config carries no sibling structural check, so this gate has no twin to keep in step; cargo fmt is clean. The full test suite, the lint pass and the end-to-end suites were not run here. Checks at review time: 3 pending, 26 success, no failures. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• xtask/src/main.rs:104, .github/workflows/ci.yml:1195, CONTRIBUTING.md:74, docs/architecture.md:11 — the round-3 overclaim is only partly discharged, and the repair introduced a new false claim. Taking the five surfaces one at a time: xtask/src/architecture_doc.rs:805 (stale_message) is fixed — I re-ran the repro and it now reports "not found where they're cited" instead of asserting files are gone, which was the sharpest edge of the original finding. The module's own doc comments are fixed and citation_exists's doc now spells out both the every-directory rule and the root-only rule correctly. But xtask/src/main.rs:104 still reads "Fail if any path cited (in backticks) in docs/architecture.md no longer exists in the tracked tree" — the exact sentence that was flagged, untouched by this round's commit, and it is the most user-facing surface of the three since it is what cargo xtask --help prints. .github/workflows/ci.yml:1195 likewise still claims the job "verifies every backtick-quoted path citation in docs/architecture.md still exists in the tracked tree", which overstates on both halves: most backtick spans are never candidates, and existence is not what is checked. Worse, the two surfaces that were rewritten now both end with "or anywhere in the tracked tree otherwise", and that is false: for a bare .md/.toml citation citation_exists requires p == Path::new(text), i.e. the repo root only. I confirmed this by adding `testing.md` to the doc on a scratch copy — docs/testing.md is tracked, the prose is accurate, and the gate fails it with no (expected under …) hint at all, so the contributor gets "not found where they're cited" with nothing saying where that is. The new wording is wrong in the direction that makes the gate look more permissive than it is, which is the same defect shape the previous round raised. Fix: update xtask/src/main.rs:104 and the comment at .github/workflows/ci.yml:1195 to the wording already used in the module doc comment, and in CONTRIBUTING.md:74 and docs/architecture.md:11 replace "anywhere in the tracked tree otherwise" with something true — a bare .md/.toml citation is checked against the repo root, and only a bare directory name or an unscoped citation is checked tree-wide. Separately, format_stale_citation at xtask/src/architecture_doc.rs:852 should stop printing a bare comma list for the multi-directory case: "(expected under a, b)" reads as a disjunction while the rule is a conjunction, so the reader checks one of the two named directories, finds the file there, and concludes the gate is broken.

Non-blocking

• xtask/src/architecture_doc.rs:747 — the new clause disclosing the unproven delegation says "no test drives run end-to-end", but run_passes_against_the_real_doc (:1855) calls run() and its own comment says it exercises the same path CI runs; the true gap is that it asserts only the Ok direction, so say that instead of contradicting the test five hundred lines below.
• xtask/src/architecture_doc.rs:756 — the disarm itself is unchanged: discarding check_doc_at's result still leaves all 69 tests green. Fairly a stopping point given check_doc_at is covered directly, and now at least documented.
• docs/architecture.md:58 — the engines/vllm paragraph is still phrased the way the checker's every-directory rule demands rather than the way it reads best; worth a line in CONTRIBUTING.md telling authors that a bare .rs citation under a two-crate heading must name its owner, since the failure gives no such guidance.
• xtask/src/architecture_doc.rs:89 and :966 — the shared is_scoped_extension predicate and the shared run_git test helper both land cleanly; the mutation run confirms the predicate is well covered, so the drift that caused the earlier bug can't recur silently.
• .github/workflows/ci.yml:1206 — the job's guard, timeout, sccache block and action pins all match the sibling jobs exactly, and the structural workflow guard's checks are name-agnostic, so nothing else needs updating in step with it; noting it so the next round doesn't re-derive the twin question.

Superseded by a new round at 15cf21a. Part of this objection is discharged; the remainder, plus a new inaccuracy introduced by the rewording, is restated in a fresh change request so there is only one live objection to answer.

[siloteemu]

🔴 Automated review · pr-review-watcher · 15cf21a

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.

The previous round's objection is only partly discharged, and the wording that replaced it introduces a new inaccuracy. Full detail is in the round comment on this PR; the blocking part is:

Two of the five surfaces still carry the original overclaim. xtask/src/main.rs:104 still reads "Fail if any path cited (in backticks) in docs/architecture.md no longer exists in the tracked tree" — the exact sentence flagged last round, untouched, and the most user-facing of the three since it is what the command's --help prints. .github/workflows/ci.yml:1195 still says the job "verifies every backtick-quoted path citation in docs/architecture.md still exists in the tracked tree". Both overstate on two counts: most backtick spans are never candidates, and existence is not what is checked.

The new wording is false in the permissive direction. CONTRIBUTING.md:74 and docs/architecture.md:11 now end with "or anywhere in the tracked tree otherwise". For a bare .md/.toml citation, citation_exists requires p == Path::new(text) — the repository root only. Confirmed by construction: adding a `testing.md` citation makes the gate fail even though docs/testing.md is tracked and the prose is accurate, and the failure prints no (expected under …) hint, so the contributor is told the path was "not found where they're cited" with nothing saying where that is.

Suggested fix. Use the module doc comment's wording — which is correct — at xtask/src/main.rs:104 and in the comment at .github/workflows/ci.yml:1195. In CONTRIBUTING.md:74 and docs/architecture.md:11, replace "anywhere in the tracked tree otherwise" with something true: a bare .md/.toml citation is checked against the repository root, and only a bare directory name or an unscoped citation is checked tree-wide.

Separately, and worth folding into the same change: format_stale_citation at xtask/src/architecture_doc.rs:852 prints a bare comma list for the multi-directory case, so "(expected under a, b)" reads as a disjunction while the rule is a conjunction. A reader checks one of the two named directories, finds the file there, and concludes the gate is broken.

The parser fix itself is sound and its regression test genuinely pins it: reverting the fence/blank-line handling to its pre-fix form makes that test the sole failure. The extracted is_scoped_extension helper is well covered — ten named tests kill a mutation of it — so the drift that caused the earlier bug cannot recur silently. This PR has no twin workflow or aggregator job to keep in step; that was checked directly.

[jussielo-amd]

Confirmed and fixed in c481e82:

• Blocking — the round-4 overclaim was only partly fixed, and the replacement wording introduced a new inaccuracy. All five surfaces are now aligned with the module's own (accurate) doc comment: xtask/src/main.rs's CheckArchitectureDoc help text and .github/workflows/ci.yml's job comment no longer claim a universal "still exists in the tracked tree" rule, and CONTRIBUTING.md/docs/architecture.md no longer claim unscoped citations are checked "anywhere in the tracked tree otherwise" — that was false for a bare .md/.toml citation, which citation_exists checks only at the repository root. Verified against citation_exists directly before wording any of the four texts. Also fixed the related format_stale_citation bug: a citation scoped to 2+ directories printed a bare comma list ("expected under a, b"), which reads as a disjunction even though the rule is a conjunction (multi_directory_citation_requires_every_directory_to_have_the_file) — now says "expected under all of ...", with a new regression test (stale_message_reads_a_multi_directory_citation_as_a_conjunction).
• Non-blocking (run's doc comment, architecture_doc.rs:747) — "no test drives run end-to-end" was itself inaccurate: run_passes_against_the_real_doc does call run() directly, it just only asserts the Ok direction. Reworded to say that precisely.

Left as-is: the suggestion to add a CONTRIBUTING.md line telling future doc authors that a bare .rs citation under a multi-crate heading must name its owner — a style/authoring-guidance nicety, not a defect, and this PR has already absorbed many rounds of hardening.

cargo test -p xtask architecture_doc (70 tests), cargo test --workspace (all crates green), cargo clippy --workspace --all-targets -- -D warnings, and cargo fmt --check all clean. cargo xtask check-architecture-doc and its reworded --help text both verified directly.

Superseded: this objection was filed against an earlier commit and is replaced by a fresh round published at the current head. Only the newest change request is operative, so this one is withdrawn to leave a single live objection.

[siloteemu]

🔴 Automated review · pr-review-watcher · c481e82

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.

Summary

Adds cargo xtask check-architecture-doc, a CommonMark-aware extractor that pins every backtick-quoted path citation in docs/architecture.md to a tracked file, plus an always-run CI job for it — Needs work: one surface still misstates the rule it enforces, and the wording it misstates is wording this review told the author to adopt. Verified: cargo test -p xtask architecture_doc (70 tests) passes on an untouched copy, and on a separate scratch copy 20 targeted mutations — one per production line a test claims to pin — were each killed by exactly the test that names it, including the multi-directory conjunction wording, the root-only .md/.toml branch, the every directory must have the file quantifier, the fence run-length and info-string rules, the possessive-owner narrowing, and the prev_line_blank staleness fix; separately confirmed by reading citation_exists that a bare .md/.toml citation is matched at the repository root only, and that run_passes_against_the_real_doc does call run(). The full workspace test suite, clippy and the e2e suites were not run here. Checks at review time: success=30, no failures, none pending. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

xtask/src/main.rs:105-107 (and its three twins) — this one is our fault, and it is still a defect. Last round we wrote: "Use the module doc comment's wording — which is correct — at xtask/src/main.rs:104 and in the comment at .github/workflows/ci.yml:1195." That was wrong. The module doc comment is not correct: it carries the same permissive overclaim we blocked on in the two prose files. The author did exactly as asked, so the falsehood now sits on four surfaces instead of two.

All four of xtask/src/architecture_doc.rs:13-14 (module doc), xtask/src/architecture_doc.rs:745-746 (run's doc comment), xtask/src/main.rs:105-107 (the --help text) and .github/workflows/ci.yml:1196-1198 (the job comment) state a two-way rule: "scoped to its nearest heading's directories, or anywhere in the tracked tree for an unscoped citation". A bare .md/.toml citation is unscoped — is_scoped_extension accepts only .rs — and citation_exists (xtask/src/architecture_doc.rs:733-738) matches it with p == Path::new(text), the repository root and nowhere else. So the stated rule is false in the permissive direction for a whole citation shape, and --help is the most user-facing of the four. The failure it produces says "not found where they're cited" and, by design (format_stale_citation's !is_scoped guard), prints no "(expected under …)" hint — so a contributor who adds a bare testing.md citation gets a red gate, a message that doesn't say where the file was expected, and help text that told them any tracked location would do.

This blocks because it is the defect class the whole change exists to prevent, and the accurate sentence already lives two files away.

Concrete fix. Copy the three-clause sentence already in CONTRIBUTING.md:74 / docs/architecture.md:11 — "scoped to its nearest heading's directories for a subsystem-specific .rs citation, at the repository root for a bare .md/.toml citation, or anywhere in the tracked tree for a bare directory name or an otherwise-unscoped citation" — onto all four surfaces, so the six texts agree and agree with the code. I traced each of those three clauses against citation_exists independently before proposing it: clause 1 covers both the bare and the partial-suffix .rs branches, clause 2 covers the root-level branch exactly, and clause 3 covers the leading whole-path match, the unconstrained suffix fallback and the path-component fallback. Adding a one-line test that asserts the --help string contains the .md/.toml clause would stop the six copies drifting apart again — the drift is what produced both this round's and last round's finding.

Non-blocking

• .github/workflows/ci.yml:1206-1212 — the cache-prefix comment says the new job reuses a cache "those jobs already compile", but clippy's own comment at :925-930 says clippy builds dependencies without --emit=link and routes workspace crates through clippy-driver, and every compiling step in build-and-test is gated on needs.changes.outputs.heavy == 'true'; on a docs-only change — precisely the case this deliberately always-run job exists for — neither sibling populates the cache in that run, so the job can pay a cold workspace compile.
• Whether this new job actually gates a merge depends on the required-status-check list, which is not visible in the repository; a job that runs but is not on that list will go green or red without blocking anything. Worth confirming outside the diff.
• xtask/src/architecture_doc.rs:605-612 — a bare hyphenated word is only treated as a directory citation on a heading line or immediately before 's; that restriction is explained at the code site but is missing from the module doc comment's list of deliberate blind spots, which otherwise reads as complete. No live effect today (every such word in the doc is on a heading or possessive), but a future "see also rocm-dash-collectors" sentence goes unchecked silently.
• xtask/src/architecture_doc.rs:757 — measured: replacing run's body with a discarded call to check_doc_at leaves all 70 tests green. The doc comment at :748-756 says exactly that, so the disclosure is accurate rather than flattering. Our earlier remark that no test drives run end to end was itself imprecise: run_passes_against_the_real_doc does call run() and asserts only the Ok direction, and the reworded comment now says so. Reviewer error on our side, and the rewording removes the invitation to repeat it.
• docs/architecture.md:58 — the engines/vllm sentence was rewritten purely so the heuristic would narrow its bare .rs citations away from the two-crate heading. That authoring rule — a bare .rs citation under a multi-crate heading must name its owner — still isn't written where doc authors look, so the next person writing the natural phrasing meets a red gate they have to reverse-engineer. The conjunction wording added this round softens the landing, but one line in CONTRIBUTING.md would prevent the trip.

[siloteemu]

🔴 Automated review · pr-review-watcher · c481e82

Change request filed by the automated review round at this commit. The full round, including non-blocking notes, is in the comment posted alongside it.

🚫 Blocking (must fix before merge)

xtask/src/main.rs:105-107 (and its three twins) — this one is our fault, and it is still a defect. Last round we wrote: "Use the module doc comment's wording — which is correct — at xtask/src/main.rs:104 and in the comment at .github/workflows/ci.yml:1195." That was wrong. The module doc comment is not correct: it carries the same permissive overclaim we blocked on in the two prose files. The author did exactly as asked, so the falsehood now sits on four surfaces instead of two.

All four of xtask/src/architecture_doc.rs:13-14 (module doc), xtask/src/architecture_doc.rs:745-746 (run's doc comment), xtask/src/main.rs:105-107 (the --help text) and .github/workflows/ci.yml:1196-1198 (the job comment) state a two-way rule: "scoped to its nearest heading's directories, or anywhere in the tracked tree for an unscoped citation". A bare .md/.toml citation is unscoped — is_scoped_extension accepts only .rs — and citation_exists (xtask/src/architecture_doc.rs:733-738) matches it with p == Path::new(text), the repository root and nowhere else. So the stated rule is false in the permissive direction for a whole citation shape, and --help is the most user-facing of the four. The failure it produces says "not found where they're cited" and, by design (format_stale_citation's !is_scoped guard), prints no "(expected under …)" hint — so a contributor who adds a bare testing.md citation gets a red gate, a message that doesn't say where the file was expected, and help text that told them any tracked location would do.

This blocks because it is the defect class the whole change exists to prevent, and the accurate sentence already lives two files away.

Concrete fix. Copy the three-clause sentence already in CONTRIBUTING.md:74 / docs/architecture.md:11 — "scoped to its nearest heading's directories for a subsystem-specific .rs citation, at the repository root for a bare .md/.toml citation, or anywhere in the tracked tree for a bare directory name or an otherwise-unscoped citation" — onto all four surfaces, so the six texts agree and agree with the code. I traced each of those three clauses against citation_exists independently before proposing it: clause 1 covers both the bare and the partial-suffix .rs branches, clause 2 covers the root-level branch exactly, and clause 3 covers the leading whole-path match, the unconstrained suffix fallback and the path-component fallback. Adding a one-line test that asserts the --help string contains the .md/.toml clause would stop the six copies drifting apart again — the drift is what produced both this round's and last round's finding.

[jussielo-amd]

Confirmed and fixed in c8067da: the module doc comment and run()'s doc comment (the two surfaces I hadn't touched, on top of xtask/src/main.rs's --help text and .github/workflows/ci.yml's job comment) still stated the false two-way rule. All four now carry the same three-clause wording already in CONTRIBUTING.md/docs/architecture.md, so all six surfaces agree with citation_exists.

Verified cargo run -p xtask -- --help directly: the check-architecture-doc line now reads the three-clause version. cargo test -p xtask architecture_doc (70 tests), cargo clippy -p xtask --all-targets -- -D warnings, and cargo fmt --check are all clean; cargo test --workspace is clean apart from one pre-existing flaky test (comfyui::status_reports_stopped_when_saved_comfyui_pid_is_gone, unrelated to this diff — passes in isolation).

Left as-is: the suggested --help-string regression test. There's no existing test infra in xtask/src/main.rs for asserting on rendered clap help text, and this round's defect was a wording drift rather than a logic bug — adding a first-of-its-kind CLI-help test felt like more machinery than the fix warrants on a PR that's already absorbed many hardening rounds. Happy to add it if you'd rather have that guardrail.

[siloteemu]

🔴 Automated review · pr-review-watcher · c8067da

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.

Summary

This PR adds a CI gate (cargo xtask check-architecture-doc, plus its own always-run workflow job) that extracts backtick-quoted path citations from the architecture doc and fails, naming each one, when a citation is not found where it is cited — no blocking findings; the change request we filed at the previous commit is discharged on its blocking count, and the head commit is prose-only (four doc/help/comment surfaces, no code). Verified: ran the checker module's own unit tests at this head (70 passed, 0 failed) and the command itself against the real doc (exit 0); then confirmed by construction in a scratch copy that the newly-copied three-clause sentence matches the code — a bare .md citation really is matched at the repository root only (a tracked file of the same name one directory down does not satisfy it), a bare directory name really is matched tree-wide, and a bare .rs citation under a heading really does fail when that section's file is absent, so the gate can fail on the thing it claims to catch; I also confirmed the one carried non-blocking item still holds (discarding the entry point's single delegation expression leaves every test green — now honestly described in that function's own doc comment, which is the agreed stopping point), and that removing one possessive clause from the doc's engines heading red-gates otherwise-accurate prose over four files. Not checked this round: the full test suite, the lint pass, the end-to-end suites, and a YAML syntax parse of the workflow file. Checks at review time: success=27, pending=2, and no failures; re-read immediately before publishing: success=28, pending=1, and no failures. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• docs/architecture.md:58 — the four bare .rs citations in the second half of this line only pass because the possessive slash-chain earlier on the same line narrowed them to one crate and that narrowing is remembered for the rest of the section; rewording just that clause makes the gate red-flag four files that exist, and no contributor-facing text names the remedy. A sentence in the contributor guide would help less than the thing the contributor actually reads — extend the multi-directory failure line to name the two ways out (qualify the citation with its directory, or narrow it with possessive prose).
• xtask/src/main.rs:105, and its five twins — the three-clause sentence now agrees across all six surfaces, but nothing pins them: the one-line help-text assertion suggested alongside our change request was not added, and drift across these copies has now produced a finding in two consecutive rounds. Note that asserting only on the help string covers one surface of six; the two markdown files and the workflow comment need a whitespace-normalising scan (their line wrapping and comment prefixes differ), or the clause should live in one constant that the help text renders.
• xtask/src/architecture_doc.rs:744 — the first clause ("scoped to its nearest heading's directories for a subsystem-specific .rs citation") is stricter than the code for a full slash path: that shape is matched tree-wide by whole-path prefix before any scoping is consulted, so a full path cited under an unrelated heading passes. Confirmed by construction. Harmless direction, but it is the one remaining place where the six texts and the code do not line up.
• .github/workflows/ci.yml:1190 — the new job is not referenced by any in-repo aggregator because the repository has none; a job becomes merge-blocking only by being added to the externally-held required-checks list, which this change cannot do and the PR text does not mention. I am inferring how gating works here from the absence of any in-tree wiring and cannot confirm it.
• PR description — "68 unit tests in this module" understates the actual count at this head (70). Trivial, but it is a claim about the change that no longer matches it.