[jussielo-amd]

Summary

apps/rocm/src/examine_report_text.rs held two halves that never call each other: the rocm examine report plus the engine-inventory listing, and the renderer behind rocm model / rocm model --verbose. At 1,499 production lines (header comment and imports included) it sat one line under the planned 1,500-line module-size gate, and two open PRs (#678, #695) add to it. This moves the rocm model catalog renderer into its own module, model_catalog_text.rs, so each half can be read and changed on its own. It is a pure move: no behaviour change, and the output of rocm model, rocm models, rocm model --verbose and --help is byte-identical before and after (captured with the built binary from origin/main and from this branch, same host, compared with cmp).

What moved

Everything from model_catalog_platform_heading to format_gib in examine_report_text.rs is one contiguous span, moved verbatim into the new file with its doc comments and #[allow(dead_code)] attributes.

symbol	destination	visibility
render_model_registry_text_with_context_and_host, render_model_registry_verbose_text_with_context_and_host	model_catalog_text.rs	pub(crate) as before
format_bytes	model_catalog_text.rs	pub(crate) as before
model_registry_adapter_availability_note	model_catalog_text.rs	private -> pub(crate) (see below)
model_catalog_platform_heading, model_catalog_header, model_recipe_memory_label, model_recipe_gpu_fit_label	model_catalog_text.rs	private
append_model_recipe_registry_source, append_model_recipe_metadata_lines, append_model_engine_recipe_settings_lines, append_model_recipe_staleness_lines, append_model_artifact_lines, append_artifact_source_policy_lines, append_model_host_ram_fit_lines, append_model_fit_lines, append_manual_alternative_lines, append_model_engine_support_lines	model_catalog_text.rs	private
artifact_source_policy_label, format_list_or_none, format_string_map_or_none, format_gib, manual_alternative_recommendations, recipe_is_manual_fit, recipe_display_ref, preferred_engine_action_target, model_registry_reason, once_per_call	model_catalog_text.rs	private

Against the symbol list in the original plan: nothing in that list had gone missing or moved elsewhere. Five symbols in the span were not on the list and moved with it because they only serve the catalog: append_model_recipe_staleness_lines, append_model_host_ram_fit_lines, append_model_fit_lines, append_manual_alternative_lines and once_per_call. engine_runtime_is_native_windows_unsupported, which the old module doc and architecture entry still listed, no longer exists in the code, so the rewritten docs do not name it.

One helper is shared with the half that stays: missing_packaged_engine_reason is called by both append_engine_detect_summary (stays) and append_model_engine_support_lines (moved). It lives at the crate root and is imported by both modules; nothing is duplicated.

Visibility: only model_registry_adapter_availability_note widens, from private to pub(crate), because linux_only_engine_runtime_status_is_explicit_on_native_windows (which stays, see below) calls it. Outside users are unchanged: main.rs imports the two renderers and format_bytes from the new module, and chat.rs, storage.rs and automations.rs keep reaching them as crate::<name>, so the only consumer line that changed is main.rs's use.

Tests

The 18 tests whose primary subject is the catalog moved with it and keep their leaf names: format_bytes_steps_up_instead_of_printing_1024_of_the_smaller_unit, the format_bytes_renders_a_size_in_its_own_unit property test with its BYTE_UNITS constant and byte_count_strategy generator, model_catalog_platform_heading_marks_the_host_gpu_in_ascii, render_model_registry_text_lists_builtin_recipes, render_model_registry_text_shows_fit_when_gpu_known, model_catalog_header_only_names_source_for_configured_index, render_model_registry_verbose_text_keeps_diagnostics, model_recipe_artifact_lines_surface_signed_index_metadata, render_model_registry_text_reports_host_ram_fit, render_model_registry_text_reports_supported_and_unsupported_gpu_fit, verbose_model_listing_shows_aim_gpu_families_and_token_requirement, engine_action_target_follows_what_serve_would_pick_on_this_host, once_per_call_runs_the_probe_once, verbose_model_listing_shows_a_token_requirement_without_gfx_families, model_recipe_staleness_lines_warn_only_for_stale_engine_recipes, render_model_registry_text_marks_tiny_recipe_as_gpu_smoke_support, model_registry_marks_windows_vllm_adapter_as_runtime_unsupported and model_registry_engine_reasons_do_not_mention_cpu_fallbacks. The one test that touches both halves, linux_only_engine_runtime_status_is_explicit_on_native_windows, stays in examine_report_text.rs: its primary subject is engine_runtime_status_label, and it imports model_registry_adapter_availability_note for its second assertion. The function is pub(crate) only because that staying test calls it; moving the test's two assertions about it into model_catalog_text.rs would let it stay private, but that edits tests, which a pure move should not do, so it is left as a follow-up. The header_*, render_engine_inventory_*, examine_*, friendly_* and runtime_status_label_* tests stay. Leaf-name diff of cargo test -p rocm --bin rocm -- --list and -- --list --ignored between origin/main and this branch: empty (1,475 listed tests and 8 ignored, same multiset), with the only difference being 18 paths changing from examine_report_text::tests:: to model_catalog_text::tests::.

Proptest seed file

proptest finds saved failure cases by the source file of the test, so the format_bytes property test's seed (bytes = 1048525) is read from apps/rocm/proptest-regressions/<source-stem>.txt. It was sitting in main.txt, left behind by an earlier move, so it was not being replayed at all. The first commit renames it to model_catalog_text.txt (a pure rename, header comment kept, git diff -M shows 0 changed lines). To prove proptest reads that path I appended a deliberately unparsable line, ran the property test with --nocapture, and proptest printed proptest: .../apps/rocm/proptest-regressions/model_catalog_text.txt:8: unparsable line, ignoring; I then removed the line again. No other moved code has a proptest.

Size

Production lines per file, counting the header comment and imports and excluding the brace-matched #[cfg(test)] mod tests:

file	before	after
examine_report_text.rs	1,499	586
model_catalog_text.rs	n/a	895

Import edges

model_catalog_text.rs uses crate::engine_ipc (builtin_engine_available, resolve_engine_binary_path_with_paths), the crate-root items PlatformRuleOut, engine_ruled_out_by_platform_with and missing_packaged_engine_reason (the first two are defined in model_fit.rs and re-imported by main.rs, as before), and the qualified crate::serve_cmd::select_serve_engine. It does not import examine_report_text.rs, and examine_report_text.rs reaches it only from its own test module. No new cycle, and cargo xtask check-crate-edges is clean.

Verification

Ran: cargo fmt --all --check; cargo clippy -p rocm -p xtask --all-targets -- -D warnings; cargo test -p rocm --no-fail-fast (1,467 passed, 8 ignored, 0 failed in the bin target); cargo test -p rocm --bin rocm model (78 passed); cargo test -p rocm advised_commands (20 passed, 1 ignored, which is ignored on main too); cargo test -p xtask ascii (27 passed); cargo xtask check-crate-edges; cargo xtask manifest --check; each commit's tree was checked, and the full set was re-run on the branch tip. The before/after capture of rocm model, rocm models, rocm model --verbose, rocm model --help and rocm --help is byte-identical.

Not run: a clippy cross-check against x86_64-pc-windows-msvc fails to build here (the aws-lc-sys C build needs the MSVC headers, unrelated to this change), so the Windows lane is the check for platform-gated code. The moved code has no cfg(windows) item; the only gates are cfg!(windows) runtime checks and one #[cfg(unix)] block inside tests, all moved intact. cargo xtask e2e and scripts/smoke_local.py were not run; CI carries them.

User-visible behaviour

None, so no scenario is added: this moves code without changing it, and the output of the moved commands is byte-identical. The existing rocm model coverage (the ASCII-only assertions and the catalog tests above) moved with the code and still passes.

Notes for reviewers

Two commits. The first moves the production code, the 18 tests, the main.rs import and the proptest seed file; it compiles and passes on its own. The second is documentation only: the new model_catalog_text.rs entry in docs/architecture.md, the rewritten examine_report_text.rs entry and module doc (which listed the moved items by name), and the model_fit.rs module doc and architecture mentions that named examine_report_text.rs as the only reader of the platform gate. Path pins were checked: the advised_commands.rs entry on examine_report_text.rs (rocm interactive shell) is in render_launch_summary, which stays; xtask/src/ascii_output_contract.rs has no entry on the moved code; docs/testing.md filters by test name, which is unchanged.

Other open PRs that touch examine_report_text.rs, checked with git merge-tree against each PR head: #678 conflicts only in the three-line use crate::{...} block (it adds engine_inventory_note to the line this PR edits), a mechanical resolution; its other conflicts (README.md, main.rs, serve_summary.rs, rocm-core/src/lib.rs, docs/testing.md) already exist against main. #695 already conflicts with main in two hunks of engine_runtime_status_label and gains one more here, where its new test is inserted directly before the first moved test; all mechanical. #880 merges cleanly on main but conflicts after the move: its six-line removal of the missing_packaged_engine_reason branch lands in append_model_engine_support_lines, which now lives in model_catalog_text.rs (reapply the removal there and drop that import in the new file), plus its edits to the module doc and architecture text, which this PR rewrites. #889 merges cleanly on main and conflicts only in the module doc note and docs/architecture.md text that this PR rewrites; its note concerns a test this PR no longer mentions, so the resolution is to drop it.

[Copilot]

🟢 Approval recommended

No active review issues were identified for the catalog-renderer module extraction.

No open issues 🎉

What changed in this PR

Separates the rocm model catalog renderer from examine and engine-inventory reporting, reducing module size without changing CLI behavior.

Changes:

• Moves catalog rendering, formatting helpers, and 18 existing tests into model_catalog_text.rs.
• Updates crate-root imports and relocates the proptest regression seed to match the moved test.
• Refreshes module comments and architecture documentation to describe the new boundaries.

File	Description
docs/​architecture.md	Documents the catalog module and updated dependencies.
apps/​rocm/​src/​model_fit.rs	Updates comments to name both platform-gate consumers.
apps/​rocm/​src/​model_catalog_text.rs	Houses the relocated catalog renderer, helpers, and tests.
apps/​rocm/​src/​main.rs	Registers the new module and redirects imports.
apps/​rocm/​src/​examine_report_text.rs	Removes catalog code while retaining examine and engine reporting.
apps/​rocm/​proptest-regressions/​model_catalog_text.txt	Preserves the saved failure seed at the moved test’s expected path.

_{Review effort: Balanced}

[juhovainio]

I compared the removed lines in examine_report_text.rs against the added lines in model_catalog_text.rs. The function bodies, doc comments and all 18 tests are identical. The only differences are the module docs, imports, the license header and test scaffolding, and the visibility change below. This is a diff-only read: I didn't build or run anything locally, and I'm relying on CI plus your byte-identical rocm model output capture.

No blocking issues. Two small notes, neither needs to hold the merge:

• model_registry_adapter_availability_note is widened to pub(crate) only because linux_only_engine_runtime_status_is_explicit_on_native_windows stays behind. You already call this out. Moving that test's second assertion in a follow-up would let the function go private again.
• The docs/architecture.md line saying the catalog "shares only recipe_display_ref" from model_readiness is loosely true. The moved file keeps its own private recipe_display_ref wrapper that delegates to it. This was already the case before the PR, so a wording tweak is optional.

FYI: renaming the proptest seed file means proptest now actually replays bytes = 1048525 for format_bytes_renders_a_size_in_its_own_unit. It passes in CI, so it's a positive change, just worth knowing about.

On CI, the only failure I saw is the Strix Halo E2E at bench.feature:71 (GPU not drained after 120s), which looks like a runner problem rather than anything in this move. The MI300X and W7900 E2E jobs were still pending when I looked.

[jussielo-amd]

Thanks for the approval and the notes. On model_registry_adapter_availability_note: it is pub(crate) only because linux_only_engine_runtime_status_is_explicit_on_native_windows stays behind, and moving that test's second assertion so the function can go private again is a follow-up (ROCMAI-885) that I left out of this move on purpose. On the docs/architecture.md wording: you were right that "shares only recipe_display_ref" was loose, so 4eb0787 now says the catalog uses exactly two items from model_readiness: curated_alternatives, called directly, and recipe_display_ref, reached through a private one-line wrapper in model_catalog_text.rs (I checked this against the code). Thanks for the heads-up that the renamed proptest seed file now actually replays bytes = 1048525; no change needed, and ROCMAI-865 follows up on proptest seed hygiene. I also merged main into the branch twice (2d1e23f and 42eb229: plain merge commits, no rebase) because it moved under this PR; the only conflicts were in docs/architecture.md, kept from both sides. The test-name list matches main (1507 on both, and all 18 moved tests present), and clippy, fmt, manifest --check, check-crate-edges and lychee are clean.