docs(architecture): reflow module-map paragraphs into per-module lists

[jussielo-amd]

Why

docs/architecture.md's "Module map" section is eight dense, run-on paragraphs — one per crate/app. Each paragraph is a single git line with no hard wraps, so any edit to it (adding one newly-extracted file's citation, updating a "still pending" status) rewrites the whole paragraph as one diff hunk. That's a recurring conflict source: two PRs editing the same crate's paragraph collide on every rebase, and a doc-wide formatting pass collides with every modularization PR in flight at once.

What changed

Reflowed each of the eight per-crate sections from one narrative paragraph into a sorted, one-module-per-line bullet list, so an edit to one module's entry touches only its own line instead of rewriting a shared paragraph. Status/progress notes (e.g. "Phase 6a extracted X; the rest is planned") move to a short leading note instead of being rewritten end-to-end on each extraction; status notes that previously shared a line with an unrelated fact (rocm-core's "still in lib.rs" and "not yet modularized" notes) now get their own line, and hard-coded per-crate module counts that needed editing on every extraction (e2e-report, engines/lemonade) are dropped in favor of just the list below them.

The per-module list format changes what's convenient to state inline. comfyui.rs's entry names its dedicated clap subcommand (comfyui) and states that its command enum and dispatch fn stay in main.rs — per the Module organization convention above, and unlike bootstrap.rs, which owns its own. therock.rs's entry instead states what the module itself owns (TheRock SDK install/update, the runtime-manifest model), since the runtimes subcommand's own command enum, dispatch fn, and most of its domain logic stay in main.rs rather than in therock.rs. The map's intro sentence also scopes its "sorted alphabetically by filename" claim to the inventories that actually have one — rocm-engine-protocol and rocm-deps don't; rocm-dash-tui's directory-split agent/ and app/ lists are ordered mod.rs-first then logical (not alphabetical), and the sentence now says so directly instead of being folded into the flat-list rule.

This does not eliminate same-crate conflicts (two PRs extracting modules from the same crate at the same time will still collide), but it shrinks them to independent line insertions instead of two incompatible rewrites of one paragraph, and it stops unrelated-crate PRs from colliding with each other through a shared paragraph they never touched.

Doc-only change, no behavior or code change.

Verification

• Checked all relative link targets in the rewritten file resolve to existing files (lychee wasn't available locally, so this was a manual resolution check equivalent to the docs-links CI job / lychee.toml config), and that every file citation in the per-crate inventories is a markdown link except the two declared exceptions (the deleted agent.rs, and report.json).
• Checked every ownership/location claim in the bullets against source, not assumed from the original prose: comfyui.rs/therock.rs/bootstrap.rs/driver_install.rs's command-enum, dispatch-fn, and type-ownership claims against apps/rocm/src/main.rs and those modules directly; the rocm-dash-tui agent//app/ lists' file set and ordering against their source trees.
• git grep -n "docs/architecture.md" across the repo — no other doc references a section heading or wording that moved.

Checklist

• Bug-fix xfail check — N/A, no bug fix.
• New subcommand/subsystem file placement — N/A, no new subsystem; this PR documents the existing module map, doesn't add to it.
• User-facing message test coverage (AGENTS.md §3) — N/A, no user-facing message changed; this is a contributor-facing doc only.

Heads up for reviewers

This touches every paragraph in the file, so it will conflict with other open PRs that edit docs/architecture.md's module map. Happy to rebase once those land, or to hold this until the queue is shorter — reviewer's call on sequencing.

[rominf]

🔴 Automated review · pr-review-watcher · 6b0c068

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.
The description says the change only moves content and that everything carries over unchanged. That claim is partly false (see the Blocking findings). No ticket is named and no issue is linked.

First automated round on this PR: every finding below is new.

Blocking

• (new) The reflow adds file citations that are not links, which breaks the rule the doc states at its top. One of them was already a link before this change. — docs/architecture.md:11, :37, :40, :49, :82, :96, :115

  • The rule: the intro blockquote at :11 says every file named in the per-crate inventories is a relative markdown link. It allows two exceptions only: "the old agent.rs" and "report.json". This rule comes from #582 (efcd480), which turned bare citations into links so that lychee would fail when a cited file moves or is deleted.
  • Bare citations at prw-base: a grep of the Module map finds only serve_cmd.rs and the two allowed exceptions.
  • Bare citations at HEAD:
    • bootstrap.rs twice, at :37 and :49.
    • main.rs's at :40.
    • agent/mod.rs at :82. This was [agent/mod.rs](../crates/rocm-dash-tui/src/agent/mod.rs) at base, so the change undoes one of #582's own conversions.
  • The exceptions no longer match the text:
    • The body now writes "The old agent.rs" in backticks, but the intro still quotes "the old agent.rs".
    • report.json was moved from backticks to double quotes. It is now the only filename in the doc written that way.
  • Which side governs: the intro rule. It is the deliberate policy behind the lychee docs-links gate. These mentions pass CI only because lychee never sees a bare name, so they can go stale without anything failing.
  • Confidence 85 · mechanical · Fix: link the new mentions (or rephrase so they don't cite a file), restore the agent/mod.rs link, and make the intro's exception wording match the body.

• (new) The PR states a new layout rule and breaks it in the same change. — docs/architecture.md:26, :96

  • :26 says "Each crate's modules are listed one per line". :96 puts app/chat.rs, app/slash.rs and app/summary.rs on one bullet, which is exactly the kind of shared line the PR exists to get rid of.
  • "every section below" also covers sections that have no list at all: rocm-engine-protocol (:100), rocm-deps (:104), and the dash-core/collectors/daemon crates under the :80 heading.
  • Which side governs: the rule. One line per module is the stated purpose of the change.
  • Confidence 85 · mechanical · Fix: split :96 into three bullets, and limit the sentence to the sections that actually have inventories.

• (new) The description and the first commit message both say nothing was lost or changed, and that is not true. — PR description ("Diffed old vs. new … confirm every citation link, extraction-pattern fact, and status note … carries over unchanged"); commit d974fa2 ("Every citation, link, and ownership note carries over unchanged")

  • Lost or changed:
    • The agent/mod.rs link was removed (above).
    • The ownership qualifier "(each owns its own types)" on the full-domain-extraction group is gone.
    • "shared config stays at the crate root" on the dispatch-adjacent group is gone.
    • The pointer "(see the Phase 6a variant note above)" is gone.
    • The dash-tui completeness statement ("are every file this phase's dashboard-TUI split touches") is gone.
  • Changed but not mentioned:
    • The intro blockquote edit (report.json → "report.json").
    • The new providers.rs parenthetical at :45, which is accurate and copied from :17.
    • The rocm-core list being newly labelled "full-domain-extraction".
  • Why this matters: the repo squash-merges and builds the squash body from commit messages, so d974fa2's false "carries over unchanged" will land on main. The three follow-up commits (b15cd3c, bcf2257, 6b0c068) describe fixes to defects this PR itself introduced and that never existed on main. They will read as upstream bug fixes.
  • Confidence 85 · logic · Fix: correct the description and the d974fa2 message to list what was dropped or changed. Either restore the dropped notes, or say they were dropped deliberately because the convention section (:17–18) already defines them. Fold or reword the follow-up commit messages so the squash body describes what the change does relative to main.

Non-blocking

• (new) The new therock.rs bullet overstates what therock.rs does for runtimes. — docs/architecture.md:49

  • "full domain extraction; serves the runtimes subcommand" suggests the subcommand's domain logic lives in therock.rs. Most of it is in apps/rocm/src/main.rs:
    • RuntimesCommand at :796 and runtimes() at :5805 (the bullet does say these stay in main.rs).
    • Also render_runtimes_text (:6077), activate_runtime (:6210) and import_runtime_manifest (:6656).
  • therock.rs supplies manifest types and loaders, and its own entry points serve install and update (therock::install_sdk at main.rs:3453; install_sdk_for_update at :15952).
  • The pairing of therock.rs with RuntimesCommand already existed at :17, but this bullet makes it a stronger claim.
  • Confidence 70 · mechanical · Fix: say what therock.rs owns (TheRock install/update and runtime-manifest model) instead of "serves the runtimes subcommand".

• (new) The reviewer heads-up undercounts the conflicts. — PR description, "Heads up for reviewers"

  • It names #599, #593 and #594. A live gh pr list shows 12 open PRs touching docs/architecture.md: #625, #610, #604, #599, #594, #593, #592, #563, #541, #526, #424, #299. Every one edits a paragraph this PR rewrites.
  • Confidence 85 · mechanical · Fix: update the list so whoever sequences merges knows the full rebase cost.

Decisions for the author

• The layout only partly delivers the conflict reduction it aims for — non-blocking-improvement

  • The status prose still sits on single shared lines: "The five modules are private…" (:117), "across six modules" (:131), and the "Still in lib.rs" / "lib.rs itself is…" trailers. In this file's history, extraction PRs (#487, #528, #540, #591) edited exactly those sentences: counts, "not yet" → "partially", and similar.
  • New modules that sort into the same gap still conflict with each other. For example, #604 (service_hints.rs), #625 (service_launch.rs) and #563 (shell_quote.rs) all land between serve_summary.rs and storage.rs.
  • Dropping the hard-coded counts and moving pending items into bullets would get closer to the stated goal. Same-gap collisions are an inherent tradeoff of sorted lists.

• The agent/ and app/ order is "logical", which gives no insertion point — non-blocking-improvement

  • "mod.rs first, then logical (not alphabetical) order" (:26) gives a contributor adding an app/*.rs no defined position, so they will append at the end, which brings the conflict back.
  • "mod.rs first, then alphabetical" would be a rule anyone can follow. Nothing enforces the ordering either; only link resolution is gated.

• Landing this now forces a rebase of the whole backlog — tradeoff

  • Merging it now means all 12 open PRs touching the file must convert their doc edits into the new format.
  • Merging it last avoids that, but delays the benefit.
  • The description leaves sequencing to the reviewer, so settle it explicitly (e.g. against #625).

Positive signals

• Every flat inventory is correctly alphabetical, including provider_keys.rs before providers.rs.
• The full set of linked targets is identical to prw-base, so lychee coverage of linked citations is unchanged.
• The previously bare serve_cmd.rs citation is now a link.

Deployment notes

None

What this covered

• Read: docs/architecture.md completely at prw-base (c1c2aeb) and HEAD (6b0c068), plus the full prw-base...HEAD diff.

• Checked against source: every new or altered claim, against apps/rocm/src/{main,comfyui,therock,providers,bootstrap}.rs and the module directories of every listed crate.

• Also checked: every repo reference to the doc (AGENTS.md, CONTRIBUTING.md, README.md, lychee.toml, PR template), the file's history including #582, the open PRs touching the file, and CI.

• CI: all checks for 6b0c068 pass or are skipped by path filter. Lychee is green, which is expected because it cannot see bare citations.

• Fan-out: three independent read-only workers: content fidelity, instruction/history/commits, and design questions.

• Did not run or could not do:

  • The prior-changes pass covered commit history only. Review comments on earlier PRs to this file were not read.
  • No tests were run. The change is docs-only, adds no tests, and changes no behaviour, so the two test verdicts do not apply and no scenario is needed.

• Not reconciled against any discussion on the PR here.

[rominf]

🔴 Automated review · pr-review-watcher · 59adde5

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.
No ticket named. The description says the change is a doc-only reflow of the module map into per-module lists, plus a few corrections it lists. The diff matches that.

Round 2. The previous round's review point (6b0c068) was force-pushed away, so this round re-reviewed the whole change blind. Every finding below is new this round.

The three blockers from the previous round's change request are all resolved at 59adde5. None were re-found by the blind pass. Each was then put to the reviewer as a standing objection after its findings were frozen; that answer is anchored, not blind. I also re-checked each one against the code myself:

• Bare file citations / removed agent/mod.rs link: resolved. bootstrap.rs, main.rs and agent/mod.rs are links again (:86). The exception wording in the intro matches the body, and report.json is in backticks in both places. The repeated bare agent.rs in the app/ bullets refers to the deleted file the exception covers; see the non-blocking repetition finding below.
• "One module per line" broken by a shared bullet: resolved. app/chat.rs, app/slash.rs and app/summary.rs are now separate bullets. The rule at :26 is now scoped to per-crate inventories.
• Commit message claims everything "carries over unchanged": resolved. The PR is now one commit (59adde5), confirmed against GitHub's commit list. Its message makes no no-loss claim, and what it does claim matches the diff. The dropped Phase 6a pointer is not mentioned, but nothing in the message is false because of it; it is listed below as non-blocking.

Blocking

None

Non-blocking

• (new) The app/ lead-in now calls three mechanically relocated files "full domain extraction" — docs/architecture.md:93, :100-102
  Line 93 introduces all eight app/ bullets as "modularized … into an app/ directory following full domain extraction:". The app/chat.rs, app/slash.rs and app/summary.rs bullets under it say they followed "the same mechanical-relocation convention". In the old text those three files were in a separate sentence, outside the full-domain-extraction clause, so the reflow created this contradiction. The bullets govern because they keep the old text's own classification of these files. I checked this by comparing git show prw-base:docs/architecture.md with HEAD.
  Confidence 85 · mechanical · Fix: limit the lead-in to the files that follow full domain extraction (for example, end the list after actions.rs and introduce the three earlier extractions separately), or drop the pattern label from the lead-in.

• (new) therock.rs is still listed with RuntimesCommand in the conventions section, though the change's own correction says that command isn't in therock.rs — docs/architecture.md:17, :49
  The new therock.rs bullet at line 49 says correctly that RuntimesCommand, runtimes() and the runtimes logic stay in main.rs (RuntimesCommand is at main.rs:796 and runtimes() at :5805). Line 17 still gives "Examples: apps/rocm/src/therock.rs (RuntimesCommand)", the same suggestion the description says it removed. The line is unchanged, but it is the twin of the fix, and AGENTS.md §5 says to grep for a claim's wording across every surface. Line 49 governs because it matches the source.
  Confidence 70 · mechanical · Fix: change the line 17 example so it doesn't pair therock.rs with RuntimesCommand (for example, name what the module owns, as line 49 now does).

• (new) The three Phase 6a bullets lost their pointer to the variant definition — docs/architecture.md:40, :42, :46 (the driver_install.rs, engines_cmd.rs and serve_cmd.rs bullets)
  The old text said "Mechanically relocated clusters that do own private types (see the Phase 6a variant note above)". PR #540 (5eca5a6) added that pointer. The new bullets say only "mechanical relocation, owns private types (Phase 6a)", with no link back to line 18. Line 18 is the only place that explains why a module that owns types still counts as mechanical relocation.
  Confidence 70 · mechanical · Fix: put "(see the Phase 6a variant note above)" back on the first of the three bullets, or on a short lead-in.

• (new) One sentence is now repeated three times, and "this phase's" has lost its referent — docs/architecture.md:100-102
  The chat.rs, slash.rs and summary.rs bullets each repeat "following the same mechanical-relocation convention this phase's agent.rs split mirrors" word for word. The old text said it once, after "every file this phase's dashboard-TUI split touches", which told the reader which phase. Line 84 now says "the dashboard-TUI split has touched so far", so "this phase's" points at nothing definite. The three copies also add two more bare agent.rs mentions; line 11's exception list covers only "the old agent.rs".
  Confidence 70 · mechanical · Fix: state the shared provenance once, as a lead-in to those three bullets, and say "the agent.rs split" without "this phase's".

• (new) The watchers.rs bullet now reads as if two non-watcher jobs were watchers — docs/architecture.md:60
  The old text set the watcher names off with dashes: "all built-in watchers — TheRock update, …, server-recover — plus managed-service recovery classification…". The new text uses a colon and drops the closing dash: "all built-in watchers: TheRock update, …, server-recover, plus managed-service recovery classification and automation-proposal queuing". That puts recovery classification and proposal queuing inside the watcher list.
  Confidence 70 · mechanical · Fix: put the watcher names in parentheses, or end the list before "plus" with a semicolon.

Decisions for the author

• The new therock.rs bullet adds a list of function names that someone has to keep up to date — tradeoff
  Line 49 now names install_sdk, install_sdk_for_update, render_runtimes_text, activate_runtime and import_runtime_manifest. That makes the correction precise and checkable, and each name is correct today. Against it: a #540 reviewer warned that hand-kept caller inventories in this doc go stale and the compiler already knows them, and line 11 itself tells readers to grep rather than trust the doc's wording. Naming the responsibilities ("SDK install/update; runtimes rendering, activation and import stay in main.rs") would say the same thing without symbols that can rot.

Positive signals

• The sorting rule is stated together with its exceptions, and it holds: every flat inventory is in byte order (including dash.rs < dash_seam.rs and provider_keys < providers), and agent/ and app/ are the only unsorted lists.
• The ownership corrections were checked against source: ComfyuiCommand and comfyui() are in main.rs, bootstrap.rs owns BootstrapCommand and run, and install_sdk and install_sdk_for_update are in therock.rs.

Deployment notes

None

What this covered

I read the whole of docs/architecture.md at HEAD and the whole diff (prw-base...59adde54a1888eb06ecb620442944c5e0f8cd2cc, one commit). Every ownership, location, sorting and completeness claim the change adds was checked against apps/rocm, apps/rocmd, crates/rocm-core, crates/rocm-dash-tui, crates/e2e-report and engines/* by grep and ls. Outside the file, I grepped for references to the doc and for each removed phrase: AGENTS.md, CONTRIBUTING.md, README.md, the PR template, lychee.toml, .pre-commit-config.yaml and source comments. lychee --config lychee.toml docs/architecture.md ran locally: 109 OK, 0 errors.

Two independent workers covered the code pass and three source-reading passes: AGENTS.md adherence including the §2 leak scan, history, and review comments on earlier PRs #591, #540, #582, #487 and #483. I ran the design-review step myself rather than with separate workers, because a one-file reflow raised no question beyond the tradeoff above. The commit message was in view when I resolved scope, before the findings were settled; the workers never saw it. CI on 59adde5 is green on every check that ran, including the commit-signature/sign-off and lychee jobs. The GPU E2E lanes, Sphinx and skillscope were skipped, as expected for a docs-only path.

Not reconciled against any PR discussion here.

All three blocking objections were rechecked against 59adde5 and none still holds. The bare file citations are links again, including agent/mod.rs. The app/ files are one per line, and the rule is scoped to inventories. The single commit's message no longer claims nothing changed. See review 5488222117.

[jussielo-amd]

Addressed the five non-blocking findings from round 2 (5488222117) in 5bba84a:

• Split the app/ lead-in so "full domain extraction" only applies to the files it's actually true of (types.rs/event_loop.rs/scrollbar.rs/actions.rs), and stated the chat.rs/slash.rs/summary.rs mechanical-relocation provenance once instead of three times — also drops the now-dangling "this phase's" wording.
• Un-paired therock.rs from RuntimesCommand in the conventions-section example list, since its Module map entry already says that command stays in main.rs.
• Restored the "(see the Phase 6a variant note above)" pointer on the driver_install.rs bullet.
• Reworded the watchers.rs bullet so managed-service recovery classification and automation-proposal queuing can't read as watchers themselves.

On the tradeoff note (hand-kept function names in the therock.rs entry going stale): reworded to responsibilities only (SDK install/update; rendering, activation, and manifest import staying in main.rs), dropping the five symbol names — matches the intro's own "grep, don't trust this doc" guidance and how every other entry in the list already reads.

CI green on 5bba84a.