[jussielo-amd]

Summary

Phase 6f-5 of the main.rs modularization (epic ROCMAI-27). Mechanical relocation, no behavior change: moves the uninstall-plan building/rendering/removal helpers out of apps/rocm/src/main.rs into a new apps/rocm/src/uninstall_plan.rs.

This is a new, separate file from the existing uninstall.rs — docs/architecture.md already documented that these helpers stay out of uninstall.rs because storage.rs also depends on them; this PR gives them their own shared home instead of leaving them in main.rs, and updates that doc note accordingly.

• UninstallOptions, UninstallPlanEntry, UninstallPlan (types) and build_uninstall_plan, plan_installed_binaries, is_running_executable, render_uninstall_dry_run, render_uninstall_plan, shared_cache_notes/shared_cache_notes_for/shared_cache_candidates, env_path, confirm_uninstall, collect_installed_binary_candidates, is_rocm_install_entry_name, is_dev_binary_layout, entry_path, spelled_as_directory, existing_entry, remove_path moved verbatim, visibility set per actual caller found via repo-wide grep.
• Real external callers updated: uninstall.rs, storage.rs, chat.rs, runtimes_cmd.rs, deletion_properties.rs (the last three weren't part of the original ticket description — found by grep, not assumed).
• dispatch() confirmed byte-identical (independently re-verified by SHA-256 of the extracted function body).
• 6 tests partitioned into the new module's own test block in this same PR; total workspace test count unchanged.
• docs/architecture.md updated: new bullet for uninstall_plan.rs, and the existing uninstall.rs bullet corrected to point at the new location.

Test plan

• cargo build --workspace
• cargo clippy --workspace --all-targets -- -D warnings
• cargo fmt --all -- --check
• cargo test --workspace — 4322 passed, 0 failed, 17 ignored (independently re-run, matches pre-change baseline exactly)
• cargo xtask manifest --check
• python scripts/smoke_local.py --profile debug --skip-build
• dispatch() call sites confirmed byte-identical (SHA-256 of the function body)

Depends on #746 (ROCMAI-211) — second PR in the ROCMAI-86 (Phase 6f) stack.
Part of Jira ROCMAI-212 / epic ROCMAI-27.

[Copilot]

🔵 Needs a closer look

Dependency #746 was reported as draft and unmerged, and validation of the integrated stack remains unverified.

No open issues 🎉

What changed in this PR

Continues the main.rs modularization by moving shared uninstall planning, rendering, and removal helpers into uninstall_plan.rs, with no intended behavior change.

Changes:

• Extracts shared types and helpers while keeping uninstall-specific execution in uninstall.rs.
• Updates callers and relocates six existing tests alongside the extracted code.
• Documents the new module and its shared responsibilities.

Builds and tests were not rerun during this review.

File	Description
docs/​architecture.md	Documents the shared module and updates the uninstall module description.
apps/​rocm/​src/​uninstall.rs	Uses the extracted helpers and updates test references.
apps/​rocm/​src/​uninstall_plan.rs	Houses shared uninstall helpers, types, and six existing tests.
apps/​rocm/​src/​storage.rs	Imports shared planning and removal helpers from the new module.
apps/​rocm/​src/​runtimes_cmd.rs	Updates the uninstall confirmation import.
apps/​rocm/​src/​main.rs	Registers the module and removes relocated helpers and tests.
apps/​rocm/​src/​deletion_properties.rs	Updates property-test references and the removal-helper documentation link.
apps/​rocm/​src/​chat.rs	Imports uninstall preview rendering from the new module.

_{Review effort: Balanced}

[juhovainio]

Looks good. I compared the code removed from main.rs against the new uninstall_plan.rs: it is identical apart from the pub(crate) markers and one rustfmt re-wrap of the build_uninstall_plan signature, so no checks or messages were dropped. All six tests moved with the code, and nothing user-observable changes. This is a diff-only read; CI was green except E2E, which was still pending when I looked.

Non-blocking notes:

1. The architecture bullet for this module says it "owns private types", but the plan types and their fields are pub(crate) and shared across several modules. Details inline, along with the duplicated caller list.
2. The PR body ends with an AI-generated footer; please drop it.
3. The base is the #746 branch, so this should stay in draft until #746 merges.

[tomastola]

Review — no blocking findings

Full review of the whole change. I reviewed it at 915daae3; after #746 merged, the PR was rebased onto main as a5f80867, and git range-diff shows the PR's own commit is unchanged. Every line cited below is checked at a5f80867.

Blocking

None

Non-blocking

• The module doc comment and architecture.md give different lists of what storage.rs uses — apps/rocm/src/uninstall_plan.rs:33-34, docs/architecture.md:59

  • The module doc says storage.rs calls into it for UninstallPlan/UninstallPlanEntry/confirm_uninstall, and leaves out remove_path.
  • The code governs here: storage.rs:33 imports remove_path and storage.rs:997 calls it. architecture.md:59 lists it, and so do lines 28-30 of the same doc comment, so the comment also contradicts itself.
  • The same caller list is kept by hand in two places, and the two copies already disagree.
  • Fix: add remove_path. Better, keep the caller list only in the architecture.md entry, and keep the module doc to what the module owns.

• architecture.md's general sections were not updated for this phase — docs/architecture.md:17, :18, :30

  • Line 30 still ends at "Phase 6e (ROCMAI-87) extracted …".
  • Line 17 says the unqualified use crate::x::{...} import style "is limited to" a list of modules. uninstall_plan is not on the list, but uninstall.rs, storage.rs, chat.rs, runtimes_cmd.rs and main.rs all import it that way, so the "limited to" claim is no longer true.
  • Line 18 lists, phase by phase, which modules moved their types with them, and stops at 6c.
  • The 6b, 6c and 6e PRs each updated these sentences as part of the extraction, and an earlier review (#667) asked for this same fix. ROCMAI-211 has the same gap.
  • Fix: add Phase 6f to line 30, and add uninstall_plan.rs to the lists on lines 17 and 18.

• The new entry says the module "owns private types", but it has none — docs/architecture.md:59

  • All three types the module owns are pub(crate): UninstallOptions, UninstallPlan, UninstallPlanEntry (uninstall_plan.rs:54,65,71).
  • Other entries with this label say which types are private and why each pub(crate) one was widened (service_launch.rs, managed_service_status.rs).
  • Fix: say all three types are pub(crate), and name who reads them.

Decisions for the author

• uninstall_plan and uninstall depend on each other — non-blocking improvement
  • The module doc gives the reason for a separate file: it keeps storage.rs off a module that is about the uninstall command.
  • But uninstall_plan.rs imports uninstall (line 51), and UninstallPlan carries fields only the uninstall command uses: path_edits: Vec<uninstall::PathEdit>, path_record and self_delete. It also calls uninstall::plan_path_edits, removal_count, plan_is_empty and render_extra_plan.
  • So storage.rs still depends on the uninstall-only parts through the plan type. That is fine for a move with no behaviour change.
  • Either reword the doc comment so it doesn't suggest a clean separation, or plan a follow-up that moves the PATH-undo fields out of the shared plan type.

Positive signals

• The move is faithful. Diffing the 584 lines removed from main.rs against the new file, the only differences are the added pub(crate) markers, the header and module doc, a wrapping mod tests, and super:: dropped from three test calls. The Windows running-executable test moved next to the private is_running_executable it exercises.
• The import updates found the callers the ticket didn't name (chat.rs, runtimes_cmd.rs, deletion_properties.rs). A repo-wide search finds nothing still pointing at the old crate-root location, in code or docs.

Deployment notes

None. The change is internal to the crate, with no behaviour change.

What this covered

• Read: every changed file. uninstall_plan.rs was read completely, along with the full diffs and the surrounding regions of the other seven files. I also ran a mechanical diff of the removed code against the added code, and a repo-wide search for references to the moved items.
• Commits: reviewed df08cba0…915daae3; rechecked at a5f80867.
• Not run: cargo was not run locally. When I posted this, CI on a5f80867 was still running. Ticket ROCMAI-212 was not read; the description was checked against the code only.
• Not reconciled against any discussion on this PR.