[jussielo-amd]

Summary

apps/rocm/src/comfyui.rs had grown to about 5,000 lines, 2,300 of them production code, because it held four separate concerns in one file: choosing which ROCm runtime ComfyUI runs against, the ComfyUI process lifecycle, the install (source fetch, dependency install, GPU check), and status/log rendering with the manifest and state files. A reader looking for one of them had to scroll past the others, and every change to any of them touched the same file. This is part of the ongoing split of apps/rocm modules by seam. It moves the first three concerns into child modules of comfyui and leaves the types, constants, paths, manifest and state I/O, status/logs rendering and the probe/capture helpers in comfyui.rs, which drops to about 1,460 lines including its tests.

This is a pure move. No function body, message, flag or output changes, and every test keeps its name. There is one commit per module, in dependency order, and each commit builds and passes its tests on its own. A fourth, docs-only commit corrects the module notes and two comments in the e2e suite that named the old file. A merge of main follows: the one conflict was the three #[ignore] messages in the archive_properties tests that #557 reworded from known defect to known gap, which now live in comfyui/install.rs, so the new wording is applied there and comfyui.rs keeps no such module. A last small commit reflows the comment at the top of tests/e2e-cucumber/src/bin/fake-python.rs so each probe and its answer are separate sentences (comment text only).

What moved

Commit	Destination	Moved from comfyui.rs
1	comfyui/runtime_env.rs	select_runtime, format_available_runtime_keys, format_runtime_statuses, select_default_runtime, select_single_ready_runtime, select_runtime_by_selector, runtime_environment_for_manifest, runtime_environment_from_runtime, collect_rocm_runtime_paths, infer_python_env_root, apply_runtime_environment, runtime_environment_assignments, prepend_env_paths, split_runtime_paths, join_runtime_paths, push_existing_path, same_path_text
2	comfyui/process.rs	start, ensure_source_is_startable, running_from, refuse_while_running, running_advice, stop, spawn_comfyui_background and its _unix and _windows variants, evaluate_running_state, wait_for_running_state, comfyui_run_state_cli_label, endpoint_is_reachable, process_is_running, windows_process_is_running, unix_process_is_running, wait_until_stopped, terminate_process_tree, terminate_windows_process_tree, terminate_unix_process
3	comfyui/install.rs	install, swap_changes_summary, download_and_extract_source, unpack_source_archive, first_child_dir, download_file, filtered_requirement_specs, requirement_tokens, requirement_package_name, uv_install_args, TORCH_STACK_PACKAGES, TORCH_STACK_VERSION_PROBE, torch_stack_constraints, probe_torch_stack_versions, torch_constraint_lines, write_torch_constraints, run_uv_logged_command, OutputTarget, stream_logged_output

comfyui.rs re-exports start, stop and install, so main.rs and the other callers still write comfyui::start, comfyui::stop and comfyui::install and need no change. The types (ComfyUiManifest, ComfyUiState, SelectedRuntime, ComfyUiRuntimeEnvironment, the run-state types), comfyui_source_archive_url (both e2e-test-hooks variants), the paths, manifest and state I/O, status_summary, render_status, render_chat_status_answer, the logs code, interrupted_reinstall_advice, and the probe/capture helpers (probe_comfyui, capture_configured_command, the temp-dir helpers, quote_log_arg, child_path_string) stay where they were. Items are pub(super) only where the parent or a sibling calls them.

Tests moved with the code they exercise. The runtime-selection and environment tests went to runtime_env, the evaluate_running_state test to process, and the install tests plus the Unix-only archive_properties property tests (ignored by default, as before) to install. Four tests span start, status and install and stay in comfyui.rs: start_refuses_an_interrupted_reinstall_until_install_finishes_it and the three chat_status_answer_* tests. The fixtures that both the parent's tests and the install tests use (test_paths, ready_runtime_manifest, write_runtime_manifest, reinstall_fixture, the planted-release helpers and a few more) stay in the parent's tests module as pub(super), and the child test modules import them from there.

Because the tests moved, their full paths changed (for example comfyui::tests::reinstall_* is now comfyui::install::tests::reinstall_*), so the cargo test filter in docs/testing.md is updated to include comfyui::install. The two spinner-label entries in xtask/src/ascii_output_contract.rs (Resolving and installing packages with uv… and Fetching ComfyUI source archive…) now point at apps/rocm/src/comfyui/install.rs, because those literals moved. A doc comment in crates/rocm-core/src/quoting.rs that names the Windows launcher's file now names comfyui/process.rs. docs/architecture.md has an entry for each new module and an updated comfyui.rs entry.

Module edges

No cycles: runtime_env imports nothing from its siblings, process imports only runtime_env (and uses source_swap, which imports nothing from comfyui), and install imports runtime_env and process; none of them is imported back. Each child also uses the parent's types and helpers, which is the normal parent/child shape that source_swap already has.

• runtime_env: crate::runtimes_cmd::runtime_usability_status, crate::therock. Its tests also import crate::comfyui::render_status and the shared fixtures from crate::comfyui::tests.
• process: no crate:: edges beyond the parent; super::runtime_env::{apply_runtime_environment, runtime_environment_assignments, runtime_environment_for_manifest, same_path_text}, and source_swap (a child of comfyui that imports nothing from comfyui) for the interrupted-reinstall check in ensure_source_is_startable.
• install: crate::cli_progress::AnimatedSpinner, crate::freeform_planner::format_structured_tool_call, crate::therock; super::process::{refuse_while_running, running_advice, running_from} and super::runtime_env::{apply_runtime_environment, runtime_environment_from_runtime, select_runtime}. Its tests also import crate::comfyui::process::ensure_source_is_startable (Unix only) and the shared fixtures; the property-test module uses crate::archive_props.
• comfyui.rs no longer imports crate::cli_progress, crate::freeform_planner or crate::runtimes_cmd.

Verification

Run locally on Linux, against the final tip, with a private target directory:

• cargo fmt --all --check and cargo clippy -p rocm -p xtask -p rocm-core --all-targets -- -D warnings: clean (also clean after each commit on its own).
• cargo test -p rocm -p rocm-core -p xtask --no-fail-fast: 2,797 passed, 0 failed, 10 ignored.
• Leaf-name test diff of cargo test -p rocm --bin rocm -- --list and -- --list --ignored, taken on origin/main before the first edit and again after each commit: empty each time (1,450 tests, 7 ignored, including the four archive_properties cases that are ignored by default).
• cargo test -p rocm --bin rocm comfyui (88 passed, 4 ignored, same as before the move), cargo test -p rocm --bin rocm advised_commands, cargo test -p xtask ascii, cargo xtask manifest --check and cargo xtask check-crate-edges: pass.
• One run of status_reports_stopped_when_saved_comfyui_pid_is_gone failed once in a parallel run and passed on every rerun (three alone, once in the full comfyui filter). The test picks a free port by binding and releasing one, so another test's listener can take the same port; the test body is unchanged by this PR.

Not run: cargo xtask e2e and scripts/smoke_local.py (CI carries them), the Windows target. A cross-check with --target x86_64-pc-windows-msvc cannot run here because the C toolchain that aws-lc-sys needs for that target is not installed. The Windows lane is the check. To reduce what it has to catch: none of the moved production code is cfg(windows)-gated (the Windows spawn, process-probe and taskkill paths are selected at run time with runtime_is_windows()), so the Linux build compiles every one of them, and I re-read the Windows-specific functions in process.rs for missing imports. The part the Linux build cannot see is test-only imports that are used only by cfg(unix) tests; I gated those imports with #[cfg(unix)] so they are not unused on Windows (ensure_source_is_startable, load_manifest, plant_cached_source_archive, plant_installable_release).

User-visible behaviour

None, so no new scenario. This is a refactor with no change to any command, output, exit code, file or engine selection. The existing comfyui-* scenarios in tests/e2e-cucumber/features/comfyui.feature exercise the moved code through the built binary and are unmodified; CI runs them.

Notes for reviewers

Read the first three commits in order; each is a mechanical move and can be checked with git diff --color-moved. The only edits besides the moves are the imports each file needs, pub(super) on items that cross a module boundary, the re-export lines in comfyui.rs, the new module doc comments, and the docs and path pins listed above.

The fourth commit changes the path text in two comments in tests/e2e-cucumber/src/bin/fake-python.rs and tests/e2e-cucumber/tests/e2e/comfyui_steps.rs that described code now in comfyui/install.rs. It is comment-only: no scenario, step or tag changes.

#557 has merged; the proptest regression files under apps/rocm/proptest-regressions are keyed to other modules and none names comfyui, and the archive_properties tests do not persist failures at all (failure_persistence is off in the shared runner), so the module path change does not affect any regression lookup. Other open PRs that touch these files: #563 edits install and the interrupted-reinstall advice in comfyui.rs, so it will need its install hunks reapplied in comfyui/install.rs after commit 3 (and likely conflicts textually with commits 2 and 3, near interrupted_reinstall_advice and at the end of the test module). The docs/architecture.md, docs/testing.md and xtask/src/ascii_output_contract.rs edits can conflict with other PRs that touch the same lists.

[Copilot]

🟢 Approval recommended

No active review issues are reported for the ComfyUI module reorganization.

No open issues 🎉

What changed in this PR

Splits ComfyUI runtime selection, process lifecycle, and installation into focused child modules, continuing the CLI’s modularization without intended user-visible changes.

Changes:

• Moves implementation and associated tests into three child modules while preserving existing caller entry points.
• Updates architecture documentation and test filters for the new module layout.
• Adjusts spinner-label source paths and comment-only references to relocated code.

File	Description
xtask/​src/​ascii_output_contract.rs	Points spinner-label exemptions at the installation module.
tests/​e2e-cucumber/​tests/​e2e/​comfyui_steps.rs	Updates an installation-path reference in a comment only.
tests/​e2e-cucumber/​src/​bin/​fake-python.rs	Updates comments describing Python probe locations.
docs/​testing.md	Adds the relocated installation tests’ filter.
docs/​architecture.md	Documents the child modules and their responsibilities.
crates/​rocm-core/​src/​quoting.rs	Updates the Windows launcher path in a doc comment.
apps/​rocm/​src/​comfyui/​runtime_env.rs	Houses runtime selection, environment setup, and associated tests.
apps/​rocm/​src/​comfyui/​process.rs	Houses process lifecycle, running-state checks, and associated tests.
apps/​rocm/​src/​comfyui/​install.rs	Houses installation, dependency handling, and associated unit and property tests.

_{Review effort: Balanced}

[juhovainio]

Reviewed the full diff. The ComfyUI split into runtime_env, process and install is a pure move: comparing removed and added lines shows only visibility (pub(super)), imports, re-exports, module docs, test-fixture visibility and path text changed, and no function body was altered. The new module edges (install -> process/runtime_env, process -> runtime_env, runtime_env -> parent) match the docs, have no cycle, and the comfyui::{start,stop,install} re-exports keep callers unchanged. Skipping a new scenario is reasonable for a behavior-preserving refactor.

Non-blocking:

• The #[cfg(unix)] gating on test-only imports could not be cross-compiled for Windows, so the Windows CI lane is the real check for unused-import lints; worth confirming that lane is green before merge.
• One doc-comment nit inline.

[juhovainio]

Nit: the reworded sentence now has a mid-sentence parenthesis ("(in install.rs; answered here ...") that reads awkwardly. Consider separating the probe attribution from the answered-with clause.

[jussielo-amd]

Thanks, reworded in 3522979: the comment now describes the two Python calls as separate sentences, naming install.rs for the pre-install torch-stack probe and comfyui.rs for the post-install GPU check, with what this binary answers to each. It is a comment-only change. The same push merges main into the branch, because #557 had reworded the #[ignore] messages of the property tests that this PR moves into install.rs; the new wording is applied there.

[nowycondro]

🤖 Automated review (Claude Code) · 1a71afa

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 (commits 718881e, 3dd878b, afe37c2, 1a71afa; base 97d2e155). No ticket named; the PR implements a pure, behavior-preserving split of apps/rocm/src/comfyui.rs into comfyui/runtime_env.rs, comfyui/process.rs and comfyui/install.rs.

Blocking

• Two self-hosted E2E checks are failing at the reviewed commit — E2E tests (Strix Halo, Ubuntu, release, current) and E2E tests (Strix Halo, Ubuntu, nightly, current) (workflow run 41845673323). Both fail the same scenario, serve-40 - Serving a model with no automation enabled starts no background helper. The captured log shows the failure is in the unrelated lemonade model-serving path: an HTTP error 404 fetching a third-party llama.cpp release build (lemonade-sdk/llama.cpp) plus a GPU "NOT drained" carryover from a prior test — nothing in features/model_serving.feature/serving_steps.rs touches any file this PR changes (only comfyui.rs and its three new child modules). This reads as environment/external-dependency flakiness rather than a regression from this diff, but per this repo's own CI-ownership policy ("a red check means 'not ready' until resolved"; handle flakes by rerun or maintainer escalation with evidence) it remains a blocking item until rerun or explicitly triaged, not something to wave through silently.
  Confidence 100 · architectural · Fix: rerun the self-hosted E2E lane; if it reproduces as the known external-download/GPU-drain flake, escalate/track it separately from this PR rather than blocking on it indefinitely.

• The PR has a live merge conflict against main (mergeable: CONFLICTING as of this review). Root cause, confirmed independently via git log/git cherry against origin/main: PR #557 ("property-test --gpu selection…") already merged and reworded the #[ignore] messages on the same archive_properties property tests this PR relocates into comfyui/install.rs, so both touch the same lines in different files/locations. The author's own PR description already names this exact conflict in advance, so it isn't a surprise to anyone reading it — it just needs to actually be resolved before merge.
  Confidence 100 · mechanical · Fix: rebase onto current main and reapply the #[ignore]-message wording from #557 inside install.rs.

Non-blocking

• Two self-hosted GPU E2E lanes (MI300X, current, W7900, current) never completed for the reviewed commit (stuck QUEUED, apparently on runner availability rather than a failure). Nothing failed, so this isn't scored as blocking, but that coverage is simply unverified as of this review — worth confirming green before merge rather than assuming it's covered elsewhere.
  Confidence 85 · architectural · Fix: re-check these two checks before merging; re-run if they remain stuck.

Decisions for the author

None.

Positive signals

• Every one of the three extraction commits is independently a byte-for-byte code move: diffing each commit's removed lines in comfyui.rs against the added lines in its destination module (ignoring only visibility-keyword and import-line changes) gives ≥94.5% raw line-identity in every commit, and the only semantic differences are added pub(super) on items a sibling module now calls, new module-doc comments, and reorganized use blocks. No logic, message, default, or condition drifted during the move.
• The resulting module dependency graph is genuinely acyclic and matches what docs/architecture.md claims: runtime_env imports no sibling, process imports only runtime_env (+ source_swap), install imports both runtime_env and process, and nothing imports back up into a child that would create a cycle.
• Every incidental reference this split forces — docs/architecture.md's module list, docs/testing.md's cargo test filters, the two pinned spinner-label paths in xtask/src/ascii_output_contract.rs, a doc comment in crates/rocm-core/src/quoting.rs, and two explanatory comments in the e2e suite — was updated accurately in the same change rather than deferred, and all of it checks out against the actual code.
• The PR description's own "Notes for reviewers" section correctly anticipated the exact merge conflict with #557 before it happened, including which lines would collide.

Deployment notes

None — pure internal module split, no behavior, API, CLI output, or dependency change.

What this covered

Read completely and independently verified (line-by-line diffing of every moved function, import-graph tracing, and cross-checking every doc/comment edit against the real file contents it describes): apps/rocm/src/comfyui.rs (both pre- and post-split versions), the three new modules comfyui/runtime_env.rs, comfyui/process.rs, comfyui/install.rs, the unchanged sibling comfyui/source_swap.rs, crates/rocm-core/src/quoting.rs, docs/architecture.md, docs/testing.md, tests/e2e-cucumber/src/bin/fake-python.rs, tests/e2e-cucumber/tests/e2e/comfyui_steps.rs, xtask/src/ascii_output_contract.rs, and this repo's AGENTS.md. This was done as four independent fan-out passes (one per new module plus one cross-cutting pass for wiring/docs/AGENTS.md) run in parallel and blind to the PR description, consolidated with the orchestrating pass's own direct verification (byte-level move diffing, CI log retrieval, and git log/git cherry history analysis for the merge-conflict root cause). All four fan-out passes and the orchestrator's own checks returned clean (no ≥70-confidence code defects).

Did not run: a standalone "prior review comments on these files" pass (out of scope for a blind review) and an independent per-commit build of each of the four commits in isolation — the PR's own CI only builds/tests the squashed head, so the author's claim that each commit builds and passes tests on its own was not independently re-verified. Not reconciled against this PR's existing review discussion or comments.

🤖 Generated with Claude Code