EAI-8032: Modularize crates/e2e-report/src/lib.rs

[jussielo-amd]

Summary

Phase 1 of the rocm-cli modularization epic (EAI-7768): splits crates/e2e-report/src/lib.rs (3,228 lines) along the parsing-vs-rendering seam. Picked first because it's a leaf crate (only xtask and tests/e2e-cucumber depend on it) — lowest blast radius, good place to validate the phased-modularization workflow before touching riskier files.

• parse.rs — cucumber report.json data model, parsing, @expected-failure xfail evaluation
• single_report.rs — single-platform HTML report generation (generate())
• consolidated.rs — the PlatformReport/manifest/expectation model, the reconciled scenario × platform Grid, and the multi-platform HTML/markdown generation built on top (the largest module)
• components.rs — shared maud HTML fragment rendering, plus the CSS and timestamp helpers both generators use; depends only on parse.rs types to keep the module graph acyclic
• lib.rs is now just pub mod + pub use re-exports of the crate's existing public surface (XfailReport, evaluate_xfail, scenario_results_by_id, generate, RunMeta, generate_consolidated, consolidated_summary_markdown) — zero call-site changes needed in xtask or tests/e2e-cucumber
• Updates docs/architecture.md's crates/e2e-report entry to describe the new module map, in the same PR per EAI-7768's rule against deferring documentation

Pure code motion — no behavior change.

Boundary deviations from the ticket's stated line ranges

EAI-7768 requires re-verifying cluster boundaries against current main by function/module name rather than trusting a child ticket's stored line ranges (historical estimates that drift). Doing that surfaced three placements that diverge from EAI-8032's line-range estimate:

• stats_bar → components.rs, not parse.rs — it renders Markup, not data.
• Descriptor/RunMeta → consolidated.rs, not single_report.rs — neither is used by the single-platform generate().
• matrix_table/legend/platform_section → consolidated.rs, not components.rs — they render PlatformReport (a consolidated-only type); keeping them in components.rs would have forced a dependency back on consolidated.rs.

Also: two small test helpers (feature_json, write_report) are intentionally duplicated between parse.rs's and consolidated.rs's test modules, since both need them and there's no shared test-utils module in this crate.

Test plan

• cargo build -p e2e-report, cargo clippy --workspace --all-targets -- -D warnings, cargo fmt --check — all clean
• cargo test -p e2e-report: 46/46 pass — same count as main's original #[test] total, confirmed independently by grep-counting #[test] and by diffing the full set of item declarations (fn/struct/enum/impl/const names) between main's lib.rs and the concatenated new files
• xtask and tests/e2e-cucumber build cleanly against the refactored crate with no source changes
• cargo xtask manifest --check — clean
• prek run --no-group local-tools (matching CI's own invocation) — clean
• Not run locally: cargo nextest (not installed here; cargo test exercises the same test binary) and the Windows CI job (nothing in this crate is platform-conditional, so no local Windows-specific check applies)

Checklist

• N/A, verified — not a bug fix (EAI-8032 is a Task); grep -n "EAI-8032" tests/e2e-cucumber/expectations.toml returns no match, so there are no xfail rows tied to this ticket to narrow
• N/A, verified — not a new subcommand/subsystem: git diff main --stat touches only crates/e2e-report/** and docs/architecture.md, no apps/rocm/apps/rocmd clap/dispatch changes. This PR reorganizes an existing subsystem rather than adding one, but the result follows the same full-domain-extraction convention docs/architecture.md describes (each new module owns its own types), and that doc's crates/e2e-report entry is updated in this PR to reflect it.

[siloteemu]

🔴 Automated review · pr-review-watcher · 8cc878a

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

Splits the 3,228-line crates/e2e-report/src/lib.rs into parse.rs, single_report.rs, consolidated.rs and components.rs, leaving lib.rs as module declarations plus re-exports, and updates the architecture doc; a second commit narrows visibility that the split had over-widened. The refactor is behaviour-preserving as claimed — the only defect found is a factual error in the documentation the PR itself adds. Verified: I diffed against both the base tip and the true merge base (they differ — merge base 738690d, base tip 8788394 — but the changed-file set is identical; findings below use the merge base); a per-item body comparison of all 121 top-level items showed every moved body byte-identical to its original modulo added visibility modifiers, the derive/attribute multiset is unchanged, the public API surface is item-for-item identical, all 46 test function names and bodies are unchanged, and the one targeted check I ran (building and testing this crate alone) produced zero warnings and 46/46 passing — the full test suite and the workspace-wide build were not run here. Because this is a pure move, reverting it would leave all 46 tests passing unchanged; that is the correct shape for a refactor — the tests are the control proving equivalence, not a detector of the move — and single-symbol mutation is covered by the compiler, since narrowing any genuinely cross-module item to private fails to build. The second commit changes visibility only, no bodies, and restores these items to exactly the privacy they had before the split. Leak scan clean; no prompt-injection content found. Checks at review start: 23 success, 2 failure, 1 pending, 1 skipped — I did not attribute those failures to any job and cannot confirm their cause. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• docs/architecture.md:60 — the new text says lib.rs is "just pub mod + pub use re-exports of the crate's public API", but the four module declarations at crates/e2e-report/src/lib.rs:11-14 are private mod. A reader would conclude paths through the module names are reachable by consumers; they are not. The same file at line 17 states the convention for library crates as public modules plus a re-export, so the PR both misdescribes its own code and quietly departs from the convention stated just above — in the one document whose job is describing module structure. Either reword the sentence to say the modules are private with public re-exports and say why, or make the declarations public to match the convention; the first is smaller and the better encapsulation. The same wording is in the first commit's message, so it is worth fixing there too if the branch is amended. Raised as non-blocking because nothing in the code is wrong and a reader acting on the mistaken reading is corrected by the compiler immediately — but it is the most substantial item here, and this is the document people will reach for later.
• The test helpers feature_json and write_report are now duplicated byte-identically into crates/e2e-report/src/parse.rs and crates/e2e-report/src/consolidated.rs; nothing catches a future divergence at compile time, so a change to the fixture JSON shape must be remembered in both places.
• crates/e2e-report/src/single_report.rs:15-17 inlines file read plus JSON deserialization rather than calling parse::parse_features. This is byte-identical to the original and predates the PR, but the split makes parse.rs the designated owner of parsing, so it now reads as a boundary violation worth a follow-up.
• crates/e2e-report/src/consolidated.rs is still 2,524 lines and carries 34 of the 46 tests; the split's stated seam is parsing-versus-rendering, and the consolidated module holds both a data model and two generators. Reasonable for a first phase, but the module is not yet at a size the convention would call settled.
• On what invited the wrong conclusion here: the misstatement appears in both the doc and the commit message, so a reader checking either surface is told the modules are public. Correcting the architecture doc sentence is the cheap fix that stops this recurring, since that doc is the reference future readers will reach for.

[siloteemu]

🔴 Automated review · pr-review-watcher · 8cc878a

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

Splits the 3,228-line crates/e2e-report/src/lib.rs into parse.rs, single_report.rs, consolidated.rs and components.rs, leaving lib.rs as module declarations plus re-exports, and updates the architecture doc; a second commit narrows visibility that the split had over-widened. The refactor is behaviour-preserving as claimed — the only defect found is a factual error in the documentation the PR itself adds. Verified: I diffed against both the base tip and the true merge base (they differ — merge base 738690d, base tip 8788394 — but the changed-file set is identical; findings below use the merge base); a per-item body comparison of all 121 top-level items showed every moved body byte-identical to its original modulo added visibility modifiers, the derive/attribute multiset is unchanged, the public API surface is item-for-item identical, all 46 test function names and bodies are unchanged, and the one targeted check I ran (building and testing this crate alone) produced zero warnings and 46/46 passing — the full test suite and the workspace-wide build were not run here. Because this is a pure move, reverting it would leave all 46 tests passing unchanged; that is the correct shape for a refactor — the tests are the control proving equivalence, not a detector of the move — and single-symbol mutation is covered by the compiler, since narrowing any genuinely cross-module item to private fails to build. The second commit changes visibility only, no bodies, and restores these items to exactly the privacy they had before the split. Leak scan clean; no prompt-injection content found. Checks at review start: 23 success, 2 failure, 1 pending, 1 skipped — I did not attribute those failures to any job and cannot confirm their cause. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• docs/architecture.md:60 — the new text says lib.rs is "just pub mod + pub use re-exports of the crate's public API", but the four module declarations at crates/e2e-report/src/lib.rs:11-14 are private mod. A reader would conclude paths through the module names are reachable by consumers; they are not. The same file at line 17 states the convention for library crates as public modules plus a re-export, so the PR both misdescribes its own code and quietly departs from the convention stated just above — in the one document whose job is describing module structure. Either reword the sentence to say the modules are private with public re-exports and say why, or make the declarations public to match the convention; the first is smaller and the better encapsulation. The same wording is in the first commit's message, so it is worth fixing there too if the branch is amended. Raised as non-blocking because nothing in the code is wrong and a reader acting on the mistaken reading is corrected by the compiler immediately — but it is the most substantial item here, and this is the document people will reach for later.
• The test helpers feature_json and write_report are now duplicated byte-identically into crates/e2e-report/src/parse.rs and crates/e2e-report/src/consolidated.rs; nothing catches a future divergence at compile time, so a change to the fixture JSON shape must be remembered in both places.
• crates/e2e-report/src/single_report.rs:15-17 inlines file read plus JSON deserialization rather than calling parse::parse_features. This is byte-identical to the original and predates the PR, but the split makes parse.rs the designated owner of parsing, so it now reads as a boundary violation worth a follow-up.
• crates/e2e-report/src/consolidated.rs is still 2,524 lines and carries 34 of the 46 tests; the split's stated seam is parsing-versus-rendering, and the consolidated module holds both a data model and two generators. Reasonable for a first phase, but the module is not yet at a size the convention would call settled.
• On what invited the wrong conclusion here: the misstatement appears in both the doc and the commit message, so a reader checking either surface is told the modules are public. Correcting the architecture doc sentence is the cheap fix that stops this recurring, since that doc is the reference future readers will reach for.

[jussielo-amd]

Re @siloteemu's review (#pullrequestreview-5302435744):

Fixed — docs/architecture.md's e2e-report entry claimed lib.rs re-exports via pub mod + pub use; it actually uses private mod + pub use. Reworded to describe the actual visibility and why it's intentionally tighter than the full-domain-extraction default (f292b13).

Left as-is — the other three non-blocking points each ask for a change that would override a rule this PR is deliberately following, per parent epic EAI-7768:

• Duplicated feature_json/write_report test helpers — EAI-7768 requires tests to "colocate (#[cfg(test)] mod tests at the bottom of each new file) and move in the same PR as the code they test." Extracting the shared helpers into a common test-utils module would pull them out of that colocation, so the duplication stays.
• single_report.rs inlining read+parse instead of calling parse::parse_features — pre-existing, byte-identical to main. EAI-7768: "Never mix a mechanical move with a behavior change in the same PR." Fixing it here would be exactly that mix; tracked as a follow-up instead.
• consolidated.rs still 2,524 lines / 34 of 46 tests — EAI-7768 explicitly disclaims a file-line-count gate ("a file-level cap today would be premature"), and EAI-8032's own module breakdown calls this module "the largest piece" by design. No AC requires further splitting in this phase.

[siloteemu]

🔴 Automated review · pr-review-watcher · f292b13

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

Pure module split: crates/e2e-report/src/lib.rs (3228 lines) becomes four private modules (parse.rs, consolidated.rs, components.rs, single_report.rs) with lib.rs reduced to mod declarations plus seven pub use re-exports, plus a docs/architecture.md update. No blocking findings. One check is red and one still pending at this head; I could not attribute the red one to anything in this diff, so I record it as an observation rather than a finding of mine. Verified: diffed against both the base-branch tip and the derived merge base — identical file sets (6 files) either way; ran an item-by-item body comparison of all 76 top-level items from the pre-split file against the 82 at head (the 6 extra being the 4 mod declarations and the test module now existing in three places) — every item present, none dropped, and after normalising visibility markers only two differences exist in the entire change: one added blank line inside generate_consolidated and one intra-crate doc-link path updated in id_pass_map's doc comment; test count is 46 before and 46 after with every test body byte-identical, no test renamed, moved-but-orphaned, or newly gated (no #[ignore]/#[cfg_attr]/#[should_panic] in either revision); confirmed the public API surface is exactly unchanged (same seven items, same fields, XfailReport::is_ok still public, RunMeta::line still private) and that both consumers reach the crate only through the crate root, never through a module-qualified path; confirmed the module graph is acyclic as claimed (components.rs imports only from parse); confirmed every claim in the new docs/architecture.md paragraph against the code, including that the "full-domain-extraction convention" it cites really is described earlier in the same document; leak and prompt-injection scans over the diff and commit messages came back clean (only license headers, sign-off trailers, permitted bare ticket identifiers, and pre-existing fixture strings). One targeted check was run — building and running this crate's unit tests, 46 passed / 0 failed / 0 ignored. The full workspace suite, clippy and e2e were not run here. CI at this head: 24 success, 1 skipped, 1 pending, 1 failure. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Note on what is not blocking: an automated check of the visibility narrowing initially flagged parse.rs:45 (struct Tag) and parse.rs:58 (struct StepResult) as leftover over-widening, on the grounds that neither type is named outside parse.rs. I refuted that with a standalone compilation experiment: Rust rejects field access through a private type from another module with a hard error ("type is private"), so both markers are load-bearing — components.rs, consolidated.rs and single_report.rs read tag.name and step.result.status and would not compile without them. Cause of the wrong claim: reviewer error applying a name-grep heuristic without checking Rust's private-type rule, not anything misleading in the code. This confusion will not recur durably for a human, because the compiler rejects the "cleanup" immediately rather than accepting it silently — so no comment or assertion is warranted.

Non-blocking

• CI at this head: one check failed and one is still pending. I can see only the aggregate conclusion counts, not which check failed or why, so I am explicitly not attributing it to this change. Everything verifiable locally is consistent with a behaviour-neutral code move, but the failing log is worth reading before merge, and the pending check should be allowed to finish.

• crates/e2e-report/src/consolidated.rs — at 2524 lines this module is 78% of the original file's size, so the split relocates the bulk of the problem rather than resolving it; the doc paragraph does own this ("the largest module"), but a reviewer should confirm the follow-up phase is genuinely planned rather than implied.

• crates/e2e-report/src/parse.rs:378 and :386 vs crates/e2e-report/src/consolidated.rs:1628 and :1654 — the test helpers write_report and feature_json were duplicated byte-identically into both test modules by the split; a shared #[cfg(test)] helper module would avoid the two copies drifting apart.

• crates/e2e-report/src/single_report.rs — the only module with zero tests; its sole public function generate has no unit coverage. Pre-existing (it had none before the split either), but the split makes the gap visible in a way it was not when everything shared one test module.

• docs/architecture.md:58-73 — the new paragraph is hard-wrapped to roughly 70 columns while every neighbouring crate entry in the same file uses single long lines, so the section reads inconsistently and future edits will produce noisy diffs.

[siloteemu]

🔴 Automated review · pr-review-watcher · f292b13

This is a formal review recording our position on the record. It is deliberately non-gating: this automation files no approval, so no approving review will appear here whatever the outcome, and the merge decision stays with a human reviewer.

Blocking: 0 · Non-blocking: 5. This round reviewed a pure module split of the reporting crate, verified item by item against the merge base: every top-level item accounted for, test count unchanged at 46 with byte-identical bodies, public surface unchanged, module graph acyclic. Behaviour-preserving as claimed.

Check conclusions this review worked from, read at this exact head immediately before publishing: 24 success, 1 skipped, 1 pending, 1 failure.

The full findings, including the non-blocking items, are in the review comment posted alongside this one.

[jussielo-amd]

Re @siloteemu's second review (#pullrequestreview-5303647961):

Fixed — docs/architecture.md's e2e-report paragraph was the only hard-wrapped entry in the file; every neighboring crate entry is a single long line. Rewrapped to match (d1cd42c).

Left as-is — the remaining four non-blocking points:

• CI: E2E tests (Strix Halo, Ubuntu) failure — checked the job log: GPU preflight timeout ("VRAM never dropped below the floor... a serve is likely still holding the GPU"). Shared-runner contention, not this diff — the PR touches no GPU/serve code. MI350P was still pending at review time; letting it finish.
• consolidated.rs still 2524 lines (78% of original) — same as the prior round: EAI-7768 explicitly disclaims a file-line-count gate, and the doc already calls this out as "the largest module" by design. No AC requires further splitting in this phase.
• Duplicated feature_json/write_report test helpers — same as the prior round: EAI-7768 requires tests to colocate and move in the same PR as the code they test; pulling the helpers into a shared module would break that.
• single_report.rs has zero unit tests — pre-existing gap, not introduced by this PR. The AC calls for tests "where applicable" to validate new boundaries; there's no new boundary logic here, just a re-export move, so closing this gap is out of scope for a mechanical-move PR. Worth a follow-up ticket, not a blocker here.

[juhovainio]

Checked this out and ran it rather than just reading the diff. Public API is byte-identical to what main's lib.rs exported (XfailReport, evaluate_xfail, scenario_results_by_id, generate, RunMeta, generate_consolidated, consolidated_summary_markdown), the module split matches what the description claims (stats_bar in components.rs, matrix_table/legend/platform_section/Descriptor/RunMeta in consolidated.rs, components.rs importing only from parse.rs so the module graph stays acyclic), and cargo test -p e2e-report gives 46/46 passing with clippy clean on the actual head commit. xtask and tests/e2e-cucumber build against the refactored crate with no source changes, as claimed. docs/architecture.md's updated entry matches the real file layout.

The two failing E2E lanes (Strix Halo Ubuntu, Strix Halo WSL2) are unrelated infra flakes, not this PR: one is the known VRAM-preflight timeout, the other is a silent crash after a lemonade backend download failure from GitHub releases mid-run. Neither touches anything in this report-formatting crate.

This reads like exactly what it claims to be: pure code motion, no behavior change. Approving.

[siloteemu]

🔴 Automated review · pr-review-watcher · d1cd42c

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

Splits crates/e2e-report/src/lib.rs (3226 lines) into four private modules — parse.rs, components.rs, single_report.rs, consolidated.rs — behind a pub use facade, and updates the module map entry in docs/architecture.md; it holds up as a genuine pure move with no blocking findings. Verified: item-by-item body comparison against the merge base found 75 non-test items, all 75 located in exactly one new file with bodies identical after normalizing indentation and the visibility prefix, 0 attribute changes (#[derive]/#[serde]/#[allow]/#[cfg(test)] all unchanged), and 46 tests before → 46 after with an identical set of test names and identical bodies, not merely a matching count; the crate's exported surface is the same 7 items (XfailReport, evaluate_xfail, scenario_results_by_id, generate, RunMeta, generate_consolidated, consolidated_summary_markdown) with all four mod declarations private, so nothing became more public — the only visibility movement is private → pub(crate) for cross-module items, and I confirmed per symbol that every remaining pub(crate) item in parse.rs and components.rs is genuinely referenced from another module (consolidated.rs and single_report.rs retain none), so the visibility-narrowing remediation commit fully answers the earlier round; I ran cargo check -p e2e-report --all-targets in a scratch copy (clean, zero warnings, so no unused or missing imports and no new dependencies) and confirmed both external consumers still compile against the unchanged facade; the full test suite and the e2e suite were not run here. Since this is a refactor, the "would a test fail on revert" probe does not apply — no test was added or changed, so no single-branch mutation probe was applicable either; the body comparison above is the substitute. The one body that legitimately differs is an intra-doc link in crates/e2e-report/src/consolidated.rs retargeted to crate::parse::scenario_results_by_id, which the move requires. No prompt-injection content was found in the diff, commit messages or PR text. Working from CI check counts at this head: 23 success, 2 failure, 1 skipped, 1 pending. Blocking: 0 · Non-blocking: 3.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/e2e-report/src/parse.rs:378 and crates/e2e-report/src/consolidated.rs:1628 — the split duplicated write_report and feature_json verbatim (~31 lines) into two test modules; a shared #[cfg(test)] mod test_support; would keep the two copies from drifting.
• crates/e2e-report/src/consolidated.rs:1 — at 2524 lines (1623 of them production code) this remains by far the largest module and still holds two loosely coupled clusters, the grid/expectation model and the command-coverage surface; the doc entry acknowledges it as "the largest module", so a follow-up split is worth scheduling rather than leaving implied.
• xtask/src/e2e_report.rs:112 — a doc comment points at e2e_report::parse_descriptor, a path that was never exported and now sits in a private submodule; pre-existing, but this PR is precisely about where things live, so it is cheap to correct while here.
• Observation, not a finding: the two red checks at this head cannot be attributed to any defect in this diff, and I did not attempt to identify which jobs they are.

[siloteemu]

🔴 Automated review · pr-review-watcher · d1cd42c

Blocking: 0. This is a formal review record, not an approval — this account files no GitHub approvals.

Verified as a genuine pure move: 75 non-test items relocated with identical bodies, 46 tests before and after with identical names and bodies, no attribute changes, and the crate's exported surface unchanged. The visibility-narrowing asked for in the previous round is fully answered.

Three non-blocking items and the full verification detail are in the review comment posted alongside this one.

[fredespi]

Reviewed the current head (d1cd42c7) against merge base 738690d8. I found no blocking code issues.

The split preserves all 75 production items and the same seven-item public API; all 46 tests retain identical bodies, and cargo test -p e2e-report passed 46/46. The architecture entry matches the private-module/public-re-export layout. This looks safe to approve after final validation.

CI caveat: workflow run #1550 still has two failing self-hosted E2E lanes. Ubuntu failed GPU preflight because no VRAM was free; WSL2 stopped during an unrelated E2E scenario after compiling this crate successfully, without a diagnostic tying the failure to this refactor. I recommend merging once those red checks are rerun or otherwise dispositioned. This comment is a diff-review result, not a formal approval.