[jussielo-amd]

Closes ROCMAI-877, ROCMAI-878, ROCMAI-880 and ROCMAI-882.

Summary

This PR collapses four pairs of duplicated rules in the runtime-install code into one definition each, so that the copies can no longer drift apart. Each rule had two or three private implementations that agreed on the common case and disagreed on the edges, and the edges are what users hit: which manifest files a registered runtime ends up with, which folder a Python interpreter belongs to, which runtime "the one ready runtime" is, and what counts as a non-empty install root or the real host's os-release. The four changes are independent of each other and are delivered here in reading order, one section each below; they are bundled in one PR on purpose, to cut review overhead, and each ticket is named on the first line. Each one was written as a tests-first pair of commits (tests that pin current behaviour and pass on the unmodified code, then the change), and the history keeps those commits so every behaviour change can be read against the test that moves with it. The branch also carries three signed merge commits that join the four lines of work; the only conflicts were in docs/testing.md, docs/architecture.md and, for the ComfyUI and registry work, runtime_adopt.rs and runtime_registry.rs, and the resolution keeps both sides' content.

Two of the four change what a user can observe, in narrow cases: rocm runtimes adopt --python <file> without --root now refuses an interpreter that is not in a bin or Scripts folder (and the vLLM launch fails up front for the same layout), and rocm comfyui install now finds the runtime that rocm setup created even when its registry entry was lost, the way rocm serve already did. A setup folder whose runtime is no longer usable no longer blocks that pick while a registered runtime is ready, for the ComfyUI install and, because the rule is shared, for rocm serve and rocm engines install too. The other two are behaviour-preserving deduplications with a source-reading or shared-table test that fails if a second copy comes back.

One manifest writer (ROCMAI-877)

The two functions that wrote a runtime's manifests are now one. A registered runtime can have two manifest files, its registry entry under the data folder and a .rocm-cli-runtime.json in its install root. The SDK install path (save_runtime_manifest) wrote both and always overwrote; the import, adopt and setup-recovery path (write_runtime_registry_manifest in runtime_adopt.rs) wrote only the registry entry and refused to overwrite unless --replace was given. Which files a runtime ended up with depended on which of the two a caller happened to pick, and nothing tied that choice to what it was meant to express.

write_runtime_manifest in runtime_registry.rs is now the only writer, and every way of registering a runtime goes through it: an SDK install (both the wheel and the tarball path), rocm runtimes import, rocm runtimes adopt, and the recovery of the runtime rocm setup created. Its one parameter is the overwrite policy, ManifestOverwrite::Replace or ::Refuse. Whether the install-root file is written is no longer a property of the caller: it follows from InstalledRuntimeManifest::owns_install_root (not read-only and not imported), the same rule uninstall and prune apply when they decide whether they may delete a folder. The private writer in runtime_adopt.rs is deleted. runtime_registry_dir and runtime_manifest_path were already one definition each, so there was nothing left to deduplicate there.

The ticket framed the missing install-root file on imported, adopted and recovered runtimes as the defect. Reading what consumes that file, the install path is the odd one out only in the sense that it is the one runtime source ROCm CLI owns, and the file is not meant to appear elsewhere. Uninstall and prune delete an install folder only when it carries a .rocm-cli-runtime.json matching the registry entry, and both check read_only and imported_from first, so for an imported or adopted runtime the file is never consulted. Activation requires the file only when the runtime is not read-only. Import and adopt register a folder somebody else owns, and rocm runtimes adopt is documented as not writing into the environment; an existing test and the adopt test already assert the folder gains no manifest file, and an import's source can itself be another install's .rocm-cli-runtime.json, which an unconditional write would overwrite. Setup recovery needs no file written either: it reads the runtime's manifest from the .rocm-cli-runtime.json already in the setup folder and only restores the missing registry entry, and it never rewrites that file. So the converged behaviour keeps both file sets as they were and makes the rule that selects between them explicit and shared. The overwrite policy is kept as it was too: an install replaces its own entry, import and adopt refuse an existing one unless --replace is given, and recovery refuses so that it only ever fills a missing entry. If you would rather have the file on every registered runtime, that is a one-line change in write_runtime_manifest, and the tests that pin the current file sets say exactly what flips.

Before	After
runtime_registry::save_runtime_manifest(paths, manifest) (registry entry plus install-root file, always overwrites)	runtime_registry::write_runtime_manifest(paths, manifest, ManifestOverwrite::Replace)
runtime_adopt::write_runtime_registry_manifest(paths, manifest, replace) (registry entry only, refuses unless replace)	write_runtime_manifest(paths, manifest, ManifestOverwrite::for_replace_flag(replace)) for import and adopt, ::Refuse for recovery; the private function is deleted
ownership rule written out separately in uninstall (should_remove_runtime_install_root) and prune (HoldReason::NotOwned)	unchanged there; InstalledRuntimeManifest::owns_install_root states the same rule for the writer, and a test ties the writer's decision to should_remove_runtime_install_root

One small behaviour addition: the install-root file is not rewritten when it already holds what would be written, judged on the decoded manifest and not on the bytes. Recovery restores the registry entry from that file, so without this it would rewrite the file it just read, and a setup folder the user cannot write to would turn a recovery that used to succeed into an error. A manifest written by an older binary omits optional keys that now read back as defaults and are written out as explicit values, so a byte comparison would have read such a file as changed; install_root_manifest_is_current treats a file that decodes to the manifest being written, once normalised for storage, as current. For recovery this means it never rewrites the file, whatever shape the file has: the manifest it restores is decoded from that very file, so the file always holds what would be written (a test pins this on the bytes with a hand-edited file, so it also holds for an administrator). The file is rewritten only when a different manifest is written over it, which is an SDK install replacing its own runtime. A file written by rocm install sdk round-trips and is left alone, and so is an older file that differs only in keys that hold no value.

The registry entry is written last, after the install-root file, because the entry is what registering a runtime means. A failed install-root write therefore leaves the registry exactly as it was, and a failed registry write leaves no new entry, so an error that says a runtime could not be registered or restored never describes one whose entry exists; before, the entry was written first and a failing second write reported a failure for a runtime that was in fact registered. A failed registry write can leave the install-root file behind, which holds what a retry writes. Neither file is written atomically; that stays with the shared atomic-writer work (#610, #811).

A source-reading test, only_the_shared_routine_writes_a_registry_entry, fails if a second writer comes back: production code in the rocm app, rocmd, rocm-core or the engines that both names the registry directory (through the two path helpers, or by spelling the runtimes and registry segments or one literal holding both) and calls a write primitive (a file builder such as OpenOptions counts only when a write option is chained on it, so a read-only probe in a file that builds the registry path, as amd_smi.rs does, is not a writer), wherever the two sit in the file; and a file outside a short reviewed list calling the two path helpers. registry_writer_detector_sees_every_way_of_building_the_path pins what the detector sees. It does not catch a writer that is handed the registry path by a function in another file, a write call that is not in its list of write primitives, or code outside those source roots; its doc comment says so.

One rule for finding a Python environment root (ROCMAI-878)

Three places turned a Python interpreter's path into "the environment folder this interpreter belongs to", and they gave different answers for the same path. The ComfyUI runtime environment returned a folder only when the interpreter sat in a bin or Scripts folder (any case) and otherwise left VIRTUAL_ENV unset. rocm runtimes adopt and the vLLM launch both took the interpreter's grandparent whatever its own folder was called: the adopt helper had a bin/Scripts check, but both of its branches returned the same value, so it was dead code, and the vLLM copy never had a check at all. An interpreter in some unrelated folder was therefore refused by one caller and guessed at by the other two, and a guess names a folder that is not an environment.

The three private copies are replaced by one rule in rocm-core, runtime_python_env_root_for_executable, next to the existing runtime_python_env_bin_dir and runtime_python_executable_in_env that it inverts. An interpreter belongs to an environment only when it sits directly in a bin or Scripts folder, in any letter case and on any host, and the answer is that folder's parent. Anything else returns None: another folder, a bare file name, a file at the filesystem root. The engine crate cannot depend on apps/rocm and rocmd must never depend on rocm, so rocm-core is the one place all three callers can reach, and cargo xtask check-crate-edges passes with no new edge. The ComfyUI behaviour wins because refusing is the only answer that cannot be wrong: the grandparent of an interpreter in /opt/tools/python is /opt, which adopt would have registered as a runtime and the vLLM launch would have searched for lib/python3.*/site-packages.

Before	After
infer_python_env_root in runtime_adopt.rs (grandparent, dead bin/Scripts branch)	deleted; resolve_adopt_python_input calls rocm_core::runtime_python_env_root_for_executable
infer_python_env_root in comfyui/runtime_env.rs (bin/Scripts or None)	deleted; both venv_root sites call the shared function
python.parent().and_then(Path::parent) in vllm_venv_root, engines/vllm/src/process.rs	calls the shared function; the existing "is not inside a venv (bin/Scripts) layout" error is kept
(new)	runtime_python_env_root_for_executable in crates/rocm-core/src/runtime.rs, re-exported at the crate root

Only the two callers that guessed change, and only for an interpreter that is not directly inside a bin or Scripts folder. For rocm runtimes adopt --python <file> without --root, where the file's folder is neither, the command used to adopt the interpreter's grandparent as the install root; now it stops with the existing message could not infer the Python environment folder from <path>; pass --root and registers nothing, and passing --root still adopts that same interpreter. The --root help text and the README's adopt paragraph say this (the help text used to call --root "Optional SDK root path"). For the vLLM launch on a ROCm 10.x discovery install whose recorded interpreter is not in a bin or Scripts folder, the grandparent used to be taken as the venv; now it fails up front with the error that was already written for this case, python executable ... is not inside a venv (bin/Scripts) layout. An SDK install never records such an interpreter (it records its own environment's bin or Scripts interpreter), so the refusal is reachable only through a registered manifest that does: a hand-edited or imported one, or rocm runtimes adopt --python <file> --root <dir> with an interpreter outside such a folder. ComfyUI is unchanged: VIRTUAL_ENV is set for bin, Scripts, SCRIPTS and BIN, and left unset otherwise. A relative interpreter path such as bin/python still yields an empty folder from all three callers, as before; adopt makes its input absolute first and manifests store absolute paths.

One rule for the single ready runtime, and the setup-recovery preview (ROCMAI-880)

With no runtime chosen, rocm comfyui install and the engine commands (rocm serve, rocm engines install) each carried their own copy of "use the one ready runtime". The copies agreed on what ready means (both end in validate_runtime_manifest_for_activation) but not on the steps around it. The engine copy first recovered the runtime rocm setup created (recover_setup_runtime_registration) and only then counted; the ComfyUI copy never recovered. So when a setup runtime's registry entry had been lost, rocm serve found the runtime and wrote the entry back, while rocm comfyui install on the same machine refused with "Install ROCm first", or, if another runtime was registered next to it, quietly installed into that one.

The rule is now one function, runtime_adopt::select_single_ready_runtime, next to recover_setup_runtime_registration: find the setup runtime, load the registry, count the runtimes the activation check accepts, and return one, none (with everything registered, so a caller can say why each one does not count) or several (the ready ones). Both callers use it and the duplicate is deleted. What each caller does with "none" and "several" stays its own: the engine path still treats both as "no selection" and leaves the advice to its callers, and ComfyUI still refuses with its own /runtimes, rocm runtimes activate <key> and --runtime-id <key> text. ComfyUI recovers setup because the recovery only restores an entry the install tree itself still carries, the engine commands already do it, and a user who just ran setup should not see a different answer from serve and from comfyui install.

The rule takes a SetupRecovery. Persist is what real installs and the engine paths use: the lost registry entry is written back, through the shared writer from the first section with ManifestOverwrite::Refuse. PreviewOnly is what rocm comfyui install --dry-run uses (SetupRecovery::for_dry_run): the setup runtime is counted from its own manifest, sorted newest-install-first as the registry will once the entry is back, and nothing is written, so the preview names the runtime the real run will use and a --dry-run does not restore the registry entry. Both modes report an unusable setup runtime the same way. One thing a dry run still does, because it already did before this change, is paths.ensure() at the top of install(), which creates the app directories; it does not touch the registry.

Symbol	Before	After
rule: recover setup, count ready runtimes, one / none / several	private copy in engine_runtime_select.rs (single_ready_runtime_key) and private copy in comfyui/runtime_env.rs (select_single_ready_runtime)	runtime_adopt::select_single_ready_runtime(paths, config, SetupRecovery), result type SingleReadyRuntime, pure half classify_single_ready_runtime, plus the private find_setup_runtime that recovery and the preview share
engine caller	recovered, filtered, matched on ready.as_slice()	single_ready_runtime_key maps One to the key and Zero/Many to no selection, with Persist
ComfyUI caller	filtered on runtime_usability_status == "ready", worded the three cases	single_ready_runtime_or_refusal words the answer only; select_runtime takes the SetupRecovery, which install derives from --dry-run
test fixtures	none	write_test_setup_runtime_missing_from_registry, runtime_registry_entry_names in test_support.rs

Every difference is on the path where nothing else says which runtime to use (no --runtime-id, no active runtime, no configured default), and the engine commands behave as before except in the one corner described at the end of this section. On the ComfyUI side: a ready runtime that only setup recovery can make visible is now found by rocm comfyui install, and a real install writes its registry entry back, as rocm serve and rocm examine already do; before, it refused with "Install ROCm first". A --dry-run finds the same runtime, prints the same install command: line naming it, and leaves the registry empty. That printed command works from the state the preview left: a choice that names a runtime (--runtime-id, the active runtime or the configured default) is answered by the registry first, and when the registry alone cannot answer it is looked up again among the registry plus the setup runtime (pick_from_registry in comfyui/runtime_env.rs, over runtime_adopt::registered_runtimes_with_setup, the list the single-ready rule also counts from), the way the engine paths already resolve an explicit --runtime-id. SetupRecovery still decides the write, so previewing the printed command writes nothing and running it writes the entry back; an unusable setup runtime leaves the registry's own refusal in place instead of an error about a runtime the user did not name. A failure to write the entry back is a different case and is not hidden behind that refusal: the setup runtime is found and picked without writing, whichever recovery was asked for, and a real run writes the entry back only for a pick it is going to use and reports a failed write as it is (failed to restore setup runtime <key> into <registry>), where it used to answer "Install ROCm first" or "runtime not found" about a runtime that was there. The engine commands already reported that failure on their explicit and default paths, and a unit test now pins both. When such a recovered runtime sits beside another ready runtime, the install now refuses as ambiguous ("Multiple ROCm runtimes are ready ...") instead of silently picking the other one.

If the setup folder's runtime no longer passes the activation check, or its manifest is unreadable or points elsewhere, it is not a ready runtime and cannot change which runtimes are ready, so it is set aside while the registry holds a ready runtime: that one is picked, several are refused as ambiguous with exactly those named, and nothing is written. That is the answer a choice that names a runtime already got from the registry, and the two ways of asking no longer disagree. Only when the registry holds no ready runtime is the recovery error returned ("setup runtime <key> ... is not usable"), because the setup runtime was then the only candidate; that is what the engine commands already did and ComfyUI now does too. A usable setup runtime whose entry cannot be written back is unchanged: it is one of the runtimes being counted, so the failed write is reported as it is, whatever the count would have said. Because the rule is shared, rocm serve and rocm engines install now also pick the registered runtime when the setup folder's runtime is unusable and one other runtime is ready, where they used to fail with the recovery error; their other recovery call sites (for example the environment lookup for the lemonade engine) are unchanged and still report an unusable setup runtime. Unchanged and pinned: zero registered still says "Install ROCm first: ...", registered-but-unusable still lists each runtime with its reason under "No ROCm runtime is ready", and several ready still names /runtimes, rocm runtimes activate <key> and --runtime-id <key> and lists the ready keys. The tests assert each message together with the state it describes and run the remedy it names; comfyui_runtime_selection_state_matrix covers the choice (none, a registered runtime, the setup runtime) against 0, 1 or 2 ready registered runtimes, a setup folder that is absent, usable or unusable, a registry that takes writes or not, and preview or real run, 96 rows, each asserting the message and the registry entries together.

One definition of a non-empty install root, and of the real host's os-release (ROCMAI-882)

Two pairs of helpers had each been written twice and could drift apart; they are collapsed to one definition apiece, with a test that fails if a second copy is reintroduced. Neither pair differed in anything a user can observe, so this is a deduplication and not a behaviour change, and both pairs were characterised first in tests-only commits so the equivalence is shown by tests rather than asserted.

The first pair answers "does this install root hold anything?". has_nontrivial_directory_contents in therock.rs (the tarball extraction guard, and the update path's check that a repair's replacement is already installed) and runtime_install_root_has_payload in runtime_registry.rs (activation validation of a tarball runtime) both ignore dot-entries (the runtime's own .rocm-cli-runtime.json marker is one) and count every other entry, a dangling symlink included. They differ in exactly one input: a path that does not exist, or a dangling-symlink root. The first says "empty", the second fails with "failed to read". No caller can reach that row: activation validation rejects a root that is not a directory ("install root is missing") before it asks, tarball extraction asks right after create_dir_all, and the update check maps any failure to "not installed" anyway. The surviving definition is the first one's answer, now rocm_core::install_root_has_payload in runtime.rs, so rocmd can use it without depending on rocm. A missing path holds nothing; a root that exists but cannot be listed is still an error.

The second pair reads the real host's os-release for package-manager plans and install hints (a simulated host root must not choose the distro those commands are written for). The reader in rocm-core/openmpi.rs and the one in apps/rocm/src/system_packages.rs were the same line, read_os_release(identity_path_for_str), with the same two paths, the same fallback and the same error rules, and there was a third spelling of the call in the e2e driver step. The reader is now rocm_core::hardware_root::real_os_release, the other copies are deleted, and the identity resolver is private to hardware_root.rs, so a caller outside it has no shared way to build a second reader. real_os_release is no longer Linux-gated in rocm-core; off Linux it reads the same two paths and finds neither, as the rocm copy always did.

Before	After
therock::has_nontrivial_directory_contents	rocm_core::install_root_has_payload (runtime.rs, re-exported at the crate root)
runtime_registry::runtime_install_root_has_payload	rocm_core::install_root_has_payload
the inline refusal in install_tarball_runtime	therock::ensure_tarball_install_target_is_clean (same message, now testable)
openmpi::real_os_release (Linux only), system_packages::real_os_release and the inline read_os_release(identity_path_for_str) in the e2e driver step	rocm_core::hardware_root::real_os_release
hardware_root::identity_path_for_str (pub)	private

How the two install-root checks answered each input before this change: every row is identical in both except the first two, which were "empty" versus "error" and are now "empty". Empty directory and a root holding only dot-files or a dot-directory are empty; any other file or directory, a dangling symlink entry inside, and a symlink to a populated directory as the root are a payload; a root that is a regular file, and a directory that cannot be listed, are errors.

Verification

Run on this combined tree on Linux (WSL), debug build, one private target directory, targeted only. The branch's earlier verification is in the first list; the second list is what ran after the last three commits, which answer the approving review.

• cargo fmt --all --check and cargo clippy -p rocm -p e2e-cucumber --all-targets -- -D warnings: clean.
• cargo test -p rocm --bin rocm -- runtime setup_recovery install_root os_release python_env comfyui single_ready preview payload adopt only_the_shared engine_selection real_os_release: 367 passed, 0 failed, 4 ignored. cargo test -p rocm-core --lib -- install_root runtime_python os_release payload: 34 passed. cargo test -p rocm-engine-vllm -- venv_root: 3 passed.
• cargo xtask manifest --check and cargo xtask check-crate-edges: clean. cargo test -p e2e-cucumber --test feature_naming: 4 passed. cargo xtask e2e -- -n "(comfyui-1[56]|runtime-23|runtime-lifecycle-06)": 4 scenarios, 23 steps passed (before the last review-fix commit); after it, cargo xtask e2e -- -n "comfyui-1[5678]" passes 4 scenarios, 24 steps, comfyui-17 fails with "Install ROCm first" when the lookup that fix adds is disabled, and comfyui-18 fails when the error from writing the entry back is discarded; after the last commit cargo test -p rocm --bin rocm with the filters comfyui:: (101 passed, 4 ignored), runtime_adopt:: (15), runtime_registry (13) and engine_runtime_select:: (15) all pass; -n "(comfyui-03|serve-29)": 2 scenarios passed. lychee in offline mode on docs/architecture.md, docs/testing.md and README.md: no errors.
• cargo clippy -p rocm -p rocm-core -p rocm-engine-vllm --bins --tests -- -D warnings against --target x86_64-pc-windows-msvc, with stub C compilers for the build scripts (check and clippy never link): clean.
• A trial merge of the branch with the current main is clean (git merge-tree --write-tree origin/main HEAD exits 0), and the leak scan against origin/main has only hits on the Rust word "private" in visibility wording and an unrelated function name.

After the last four commits, on Linux (WSL), debug build, targeted: cargo fmt --all --check, cargo clippy --locked -p rocm --all-targets -- -D warnings and cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings are clean. cargo test -p rocm --bin rocm with the filters comfyui:: (102 passed, 4 ignored), runtime_registry (15), runtime_adopt:: (17) and engine_runtime_select:: (15) all pass. The scenarios comfyui-1[5-9], comfyui-03, runtime-26 (numbered runtime-23 when it ran), runtime-lifecycle-06 and serve-29, run with -n against one debug binary built with rocm/e2e-test-hooks: 9 scenarios, 57 steps passed. cargo test -p e2e-cucumber --test feature_naming (4 passed), cargo xtask manifest --check and cargo xtask check-crate-edges are clean, and lychee in offline mode on docs/architecture.md and docs/testing.md reports no errors. cargo clippy -p rocm -p rocm-core -p rocm-engine-vllm --bins --tests -- -D warnings against --target x86_64-pc-windows-msvc (stub C compilers, never linked) is clean; the new state matrix has cfg(unix) and cfg(not(unix)) helpers, which is why it was run. git merge-tree --write-tree origin/main HEAD exits 0 on the current main, and the leak scan of the new commits has only the Rust word "private" in visibility wording.

Each new test was shown to fail when its target is broken, by a targeted edit that was then reverted. Letting an unusable setup runtime block the pick again fails the state matrix row Nothing, 1 ready, setup Unusable, an_unusable_setup_runtime_does_not_hide_the_registrys_ready_runtimes, the new row of comfyui_and_engine_paths_agree_on_the_single_ready_runtime and scenario comfyui-19; swallowing the unusable-setup error even when nothing else is ready fails the row with no registered runtime and comfyui_selection_reports_a_setup_runtime_that_is_no_longer_usable; discarding the registry write failure fails the unwritable-registry rows and a_choice_naming_the_setup_runtime_reports_a_registry_that_cannot_take_it_back. Writing the registry entry before the install-root file again fails a_failed_manifest_write_never_leaves_a_registered_runtime_behind, and rewriting the install-root file unconditionally fails setup_recovery_restores_the_entry_and_leaves_a_hand_edited_install_root_file_alone. A function in the storage module that joins the runtimes and registry segments and calls fs::write passed the previous single-writer guard and fails the new one. The state-matrix rows that need a registry holding runtimes that then refuses a write skip themselves where the process can write anyway (an administrator), and the rest of the 96 rows run there.

Carried over from the four branches before they were combined, not re-run here: each branch ran cargo test -p rocm --no-fail-fast and the changed crates' suites, with no test lost by leaf name, and each new test was shown to fail when its target is broken by a targeted edit that was then reverted. For the manifest writer that means dropping the install-root write, writing it unconditionally, flipping the overwrite policy, ignoring --replace in adopt, rewriting the file unconditionally and putting a private fs::write of a registry entry back each fail a named test; for the environment-root rule each caller's test and the shared table go red when the rule re-diverges; for the ready-runtime rule changing the readiness test, dropping the recovery call and re-introducing either private copy each fail the unit tests and the corresponding scenario; for the install-root payload rule dropping the dot filter or the existence check, or putting a private read_dir copy back in either caller, fails the table or the caller's test, and swapping the identity resolver in the os-release reader fails the install-hint test. Not run anywhere: the Windows tests themselves (only the cross-check compiles for that target), the full workspace gate, scripts/smoke_local.py, coverage and the GPU-only scenarios, which CI carries. The install-root manifest and the registry entry are still written with a plain fs::write; moving both onto an atomic writer belongs to the shared atomic-writer work (#610, #811) and is left alone here.

User-visible behaviour

Scenarios, all on the default no-GPU lane that runs on every PR unless noted:

• runtime-lifecycle-06 (@id:runtime-lifecycle-import-rejects-duplicate-unless-replacing) imports a manifest, is refused on the duplicate and succeeds with --replace; it gained an assertion that the registry entry exists and that the imported folder gained no .rocm-cli-runtime.json. This is the end-to-end half of the manifest-writer change, which creates no new file. The SDK install scenarios that would exercise the install-root file need a GPU and a download and run on the gated lanes; adopt and setup recovery need a Python environment to probe and a completed setup, so unit tests assert each path's file set instead.
• runtime-26 (@id:runtime-adopt-outside-a-venv-asks-for-root, Linux) is new: it plants a stand-in interpreter in a tools folder, adopts it without --root, and asserts the refusal asks for --root and that rocm runtimes list still shows no runtime. runtime-07 (adopting /usr/bin/python3 with --root /opt/rocm) is unaffected. The scenario is Linux-only because the stand-in interpreter is a POSIX shell script; other platforms rely on unit tests that also run on the Windows CI job: adopt_request_without_an_inferred_root_asks_for_root and the rocm-core table env_root_for_executable_needs_a_bin_or_scripts_folder. The vLLM launch half is internal to a path that needs a real vLLM install and is covered by the unit test vllm_venv_root_needs_a_bin_or_scripts_folder; the "--root clears the refusal" half is covered by adopt_request_without_an_inferred_root_asks_for_root.
• comfyui-15 (@id:comfyui-install-finds-the-runtime-setup-created) is new: it plants a ready runtime and a setup folder naming it but no registry entry, previews rocm comfyui install --dry-run, and asserts the install command: line names that runtime and that the runtime registry still has no entry for it afterwards. comfyui-16 (@id:comfyui-install-restores-the-runtime-setup-created) is new: the real install of the same state, with a ComfyUI checkout already in place that needs nothing beyond the runtime's torch stack so no network or package index is needed, uses the runtime setup created and asserts the registry entry is written back. comfyui-17 (@id:comfyui-install-preview-command-works-as-printed) is new: the same state, previews without choosing a runtime, then runs the install command: line exactly as it was printed and asserts the install uses the runtime setup created and writes its registry entry back; it fails with "Install ROCm first" if an explicit --runtime-id only looks in the registry. comfyui-18 (@id:comfyui-install-reports-a-registry-it-cannot-write) is new: the same printed command when the registry cannot take the entry back, with a plain file standing where the registry directory should be (a read-only directory would not stop root, who can run this suite). The preview still succeeds and names the runtime; running the printed command fails naming failed to restore setup runtime and the runtime, does not say "Install ROCm first", and leaves the registry as it was. The variant with a read-only registry directory that holds other runtimes is a unit test, because that arrangement does not bite as root. comfyui-19 (@id:comfyui-install-uses-the-ready-runtime-beside-an-unusable-setup-runtime) is new: one ready runtime is registered and the setup folder names a runtime whose interpreter is gone; previewing the install with no runtime chosen uses the registered one, does not report the setup runtime, and leaves the registry holding only the registered runtime. It fails (reporting setup runtime ... is not usable) when the unusable setup runtime is allowed to block the pick. comfyui-03 already covers the several-ready refusal and now reaches it through the shared function, and serve-29 keeps the engine path's recovery below the no-GPU refusal; both pass unchanged and were not touched. serve-31 is @requires-gpu, unchanged in behaviour, and was not run. The engine side of the unusable-setup-runtime change has no scenario of its own: comfyui_and_engine_paths_agree_on_the_single_ready_runtime has the row, and the rule itself is unit tested in runtime_adopt.rs. The remaining ComfyUI differences (ambiguity with a recovered runtime, an unusable setup runtime with nothing else ready) are variations of the same recovery and are covered by unit tests and the state matrix.
• The install-root payload and os-release changes have no scenario: the helpers answer every reachable input the same way as before and no printed message changed, so a scenario could not tell the old code from the new. Scenarios that traverse those paths and are unaffected include download-progress-01, runtime-lifecycle-01, runtime-lifecycle-02, runtime-lifecycle-07, update-07, update-08, driver-install-06 to driver-install-08 (a bare-metal lane) and deps-guard-01. The examine scenarios that plant an os-release under a simulated root read a different, root-routed reader and are untouched; only a comment in examine.feature that named the old location of real_os_release changed.

Notes for reviewers

Read the history in the order of the sections above; within each section the first commit only adds tests and passes on the unmodified code, which is what pins the behaviour before anything changes, and the next one makes the change. The manifest-writer section has a third commit that makes the skipped rewrite compare decoded manifests rather than bytes, and the ready-runtime section has a commit that pins the unusable-setup-runtime failure and one that keeps a --dry-run preview from restoring the registry entry; then a commit that makes an explicit choice report a registry that cannot take the setup runtime back. The last commits answer the approving review: one sets an unusable setup runtime aside while a registered runtime is ready (the engine commands share that rule), one writes the registry entry after the install-root file, one tightens the single-registry-writer guard, and one fixes lint findings in the new state-matrix test. Scenario ids comfyui-15 to comfyui-19 and runtime-26 are the next free numbers on this branch and will need renumbering if another scenario lands first. docs/architecture.md is edited in the entries for runtime_registry.rs, runtime_adopt.rs, runtime.rs, hardware_root.rs, therock.rs, therock/update.rs, system_packages.rs, driver_install.rs, engine_runtime_select.rs and comfyui/runtime_env.rs, which other open work also edits, so expect textual conflicts there. A test in rocm-core that sets the simulated host root would add to a race that already exists on main (examine::rocm_install_probe::tests::merge_live_repo_paths_scans_every_family_dir_including_zypp flakes when a test takes the host-root lock inconsistently), so the install-hint test sits in the rocm binary's tests next to the existing real_os_release_ignores_a_simulated_host_root.

[Copilot]

🟡 Changes recommended

Byte comparison misclassifies valid legacy manifests as changed, causing setup recovery to fail when it attempts to rewrite a read-only sidecar after restoring the missing registry entry.

1 open issue

• Avoid rewriting equivalent legacy manifests during recovery · New

What changed in this PR

This PR centralizes runtime registration in one manifest writer, making overwrite policy explicit and using runtime ownership to decide whether to write an install-root manifest.

Changes:

• Routes SDK installation, import, adoption, and setup recovery through write_runtime_manifest.
• Preserves registry-only writes for imported and adopted runtimes and skips byte-identical install-root rewrites.
• Adds regression coverage and documents the shared writer’s behavior.

Validation was limited to source inspection; project tests were not run.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​runtime_lifecycle_steps.rs	Adds assertions for registry-only import writes.
tests/​e2e-cucumber/​features/​runtime_lifecycle.feature	Checks file sets after initial and replacement imports.
docs/​testing.md	Documents registration behavior and focused tests.
docs/​architecture.md	Describes the shared writer and ownership policy.
apps/​rocm/​src/​therock.rs	Routes both SDK installation paths through the shared writer.
apps/​rocm/​src/​runtimes_cmd.rs	Adds registration, replacement, recovery, and ownership tests.
apps/​rocm/​src/​runtime_registry.rs	Implements the shared writer, overwrite policy, and ownership predicate.
apps/​rocm/​src/​runtime_adopt.rs	Removes the duplicate writer and migrates registration callers.

_{Review effort: Balanced}

[Copilot]

🟡 Changes recommended

The runtime-selection match suppresses persistence errors for usable setup runtimes, misleadingly reporting “Install ROCm first” instead of propagating the actual failure.

1 open issue

• Propagate setup runtime persistence failures · New

1 resolved since last review

• Explicit runtime selection bypasses setup recovery

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

No active review issues block the consolidation of shared runtime-install rules.

No open issues 🎉

1 resolved since last review

• Propagate setup runtime persistence failures

_{Review effort: Balanced}

[juhovainio]

Thanks for this. I read through the full diff and CI is green. I found nothing I'd call blocking, but a few things are worth a look.

Worth addressing or confirming

1. Scope: the PR itself says the four changes (877/878/880/882) are independent, and each ticket asks for one PR. At ~4.7k lines and 27 commits it is a lot to review at once. Splitting would help, unless the maintainers are fine landing it together.
2. rocm comfyui install with no runtime chosen now fails on an unusable setup runtime even when another ready runtime is registered, while the named-choice path keeps the registry's answer. Is that asymmetry intended? No test covers that row (see inline comment).
3. In recovery, the registry entry is written before the install-root file. If the second write fails, the error blames the registry although the entry now exists, and the next run succeeds silently (see inline comment).
4. only_the_shared_routine_writes_a_registry_entry is a fairly loose guard: a 200-char window, substring matching, and exemption by file name. It could miss a writer that builds its path another way.
5. vllm_venv_root now refuses an interpreter that is not directly in bin/Scripts. I couldn't confirm from the diff that only hand-edited manifests reach it, so it would be good to confirm that.

Questions

• The PR text says setup recovery "needs no file written", but recovery can now rewrite the install-root manifest when it differs. Maybe "normally" is the accurate word.
• For 877, the ticket framed the missing install-root file on imported/adopted runtimes as the defect, while the PR keeps it absent and explains why. That direction is worth confirming with the ticket owner.
• adopt --python without --root now refuses interpreters outside bin/Scripts. The e2e scenario (runtime-23) is Linux-only, so other platforms rely on unit tests.

Approving on the strength of the diff read and green CI; the points above are non-blocking.

[juhovainio]

With no choice made, select_single_ready_runtime(.., Persist)? propagates the setup recovery error, so a stale or unusable setup folder now fails rocm comfyui install even when another ready runtime is registered. The named-choice path (pick_from_registry) deliberately keeps the registry's answer in that case. Is the asymmetry intended? No test covers "no choice + one ready registered runtime + unusable setup runtime".

[jussielo-amd]

Not intended, and the asymmetry was real: a choice that names a runtime let the registry answer, while with nothing chosen the setup recovery error was propagated, and rocm serve and rocm engines install reach the same function, so they failed the same way. A setup runtime that cannot be used is not a ready runtime and cannot change which runtimes are ready, so failing on it while another runtime was ready told the user about a folder that had no say in the answer. fa5b41e fixes it in the shared rule (select_single_ready_runtime in runtime_adopt.rs): when the setup runtime is unusable, the registry alone is counted, so one ready runtime is picked, two are refused as ambiguous with exactly those two named, and nothing is written. The setup runtime's own error is still the answer when the registry holds no ready runtime (it was the only candidate), and a usable setup runtime whose entry cannot be written back is still reported as that failure, because it is one of the runtimes being counted. One consequence to be aware of: since the rule is shared, rocm serve and rocm engines install now pick the registered runtime in that corner too; their other recovery call sites are unchanged.

The missing row is covered by comfyui_runtime_selection_state_matrix, which runs every combination of the choice (none, a registered runtime by name, the setup runtime by name), 0, 1 or 2 ready registered runtimes, a setup folder that is absent, usable or unusable, a registry that takes writes or not, and preview or real run (96 rows), asserting the message and the registry entries together. The row you named fails without the change (it reports the setup error instead of picking the registered runtime) and passes with it; reverting only the "keep the error when nothing else is ready" half fails the row with no registered runtime, and swallowing the write failure fails the unwritable-registry rows. Also added: an_unusable_setup_runtime_does_not_hide_the_registrys_ready_runtimes for the rule itself, a new row in comfyui_and_engine_paths_agree_on_the_single_ready_runtime for the engine side, and scenario comfyui-19 for the install preview.

[juhovainio]

The registry entry is written before this install-root file. If this write fails during setup recovery, the caller reports "failed to restore setup runtime ... into " although the entry now exists, and the next run succeeds silently. Consider writing the install-root file first, or asserting and documenting this state; no test pins message plus state for this branch.

[jussielo-amd]

Agreed on the ordering, and f82b274 changes it: the install-root file is now written first and the registry entry last, so a failed install-root write leaves the registry exactly as it was and a failed registry write leaves no new entry. An error that says a runtime could not be registered or restored therefore never describes one whose entry exists (a failed registry write can leave the install-root file behind, which holds what a retry writes, and the doc comment says so). a_failed_manifest_write_never_leaves_a_registered_runtime_behind pins the error and the registry together for each failing write under both overwrite policies, and fails with the old order.

On reachability, I also checked the branch you described and it cannot be reached from setup recovery: the manifest recovery restores is decoded from that very install-root file, so the file is always what would be written and the skip applies, however hand-edited its shape. setup_recovery_restores_the_entry_and_leaves_a_hand_edited_install_root_file_alone pins that on the bytes (so it holds for an administrator too) and fails if the skip is removed. The PR text said recovery rewrites the file when it decodes to a different manifest; that was wrong and is corrected in the description. The second write is only reachable from an SDK install, where the error names the file that failed.

[jussielo-amd]

Thanks for the careful read. Going through the points in order.

1. Scope: the four changes were bundled on purpose to cut review overhead, and each ticket is named on the first line of the description; I know that makes it large (27 commits before this round). The tests-first commit pairs and the per-section reading order in the notes are there so each section can be read on its own, and I am happy to split along the four sections if you would rather land them separately. No code change for this point.

2. The no-choice versus named-choice asymmetry was not intended: fixed in fa5b41e, details in the reply on runtime_env.rs. The state matrix you asked for (including the row you named) is comfyui_runtime_selection_state_matrix, with scenario comfyui-19 for the observable behavior.

3. The ordering of the two writes: fixed in f82b274, details in the reply on runtime_registry.rs. The branch you described is not reachable from setup recovery (the file it restores from is always current), which is pinned by a test.

4. only_the_shared_routine_writes_a_registry_entry was loose in the ways you list, and a storage-module function building the path from the runtimes and registry segments and calling fs::write passed it. 1d6be02 rewrites it: a production file is a writer when it both calls a write primitive and names the registry directory by any of its spellings (the two helpers, the two segments, one literal), wherever the two sit in the file, after comments and test modules are cut. It reads the rocm app, rocmd, rocm-core and the engines, the files that call the two path helpers are an explicit reviewed list compared both ways, and the fixture modules are exempt because their declarations are #[cfg(test)] (checked), not because of their file name. registry_writer_detector_sees_every_way_of_building_the_path pins nine cases. The same mutation now fails the guard. What it still does not catch is written in its doc comment: a writer handed the registry path by a function in another file, a write primitive outside its list, and code outside the scanned roots.

5. vllm_venv_root (engines/vllm/src/process.rs) is reached only from apply_vllm_rocm10_discover_env, which apply_therock_env_with calls only when runtime_has_discover_build is true, and that needs a rocm_sdk_version recorded in a registered runtime manifest (collect_managed_runtime_candidates in engines/vllm/src/runtime.rs; the environment-variable interpreter path records none). An SDK install always records its own bin or Scripts interpreter (venv_python_path in therock.rs), so it never hits the refusal. The refusal is reachable through a hand-edited or imported manifest and also through rocm runtimes adopt --python <file> --root <dir> with an interpreter outside a bin or Scripts folder (adopt_runtime_from_probe records the given interpreter whatever the folder), so my earlier "hand-edited or imported" was incomplete and the description now says so. I kept the refusal: the old behavior guessed the grandparent folder, which is not an environment, and failed later with a less clear error. No code change.

Questions:

• "needs no file written": for setup recovery the sentence is accurate, and the paragraph that said recovery can rewrite the install-root manifest was the inaccurate part. Recovery restores from that file and never rewrites it, whatever its shape; the skipped rewrite is what makes that so, and the rewrite happens only for an SDK install replacing its own runtime. The description is corrected, and no doc or comment in the tree made the claim (I grepped docs/ and apps/rocm).
• 877 and the missing install-root file: the ticket framed the file's absence on imported, adopted and recovered runtimes as the defect. Reading its consumers, the file is ROCm CLI's proof that it owns the folder: uninstall and prune delete a folder only when it carries a matching file (and check read_only and imported_from first), and activation requires it only for a runtime that is not read-only. For an imported or adopted runtime the folder is somebody else's, rocm runtimes adopt is documented as not writing into the environment, and an import's source can itself be another install's file, so writing it would claim ownership and could overwrite it. The PR therefore makes the rule that selects between the two file sets explicit and shared (owns_install_root) rather than writing the file everywhere; if the ticket owner wants it on every registered runtime that is a one-line change in write_runtime_manifest and the tests that pin the file sets say what flips. This is a direction for the ticket owner to confirm.
• adopt --python without --root and platforms: scenario runtime-23 is Linux-only (its stand-in interpreter is a POSIX script). Other platforms are covered by unit tests that run on the Windows CI job as well: adopt_request_without_an_inferred_root_asks_for_root (the refusal asks for --root and an explicit root clears it) and env_root_for_executable_needs_a_bin_or_scripts_folder in rocm-core, a table that includes Scripts, SCRIPTS and bin in any case on any host.

[Copilot]

🟢 Approval recommended

No active review issues are reported for the runtime-install consolidation.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

No active review issues are reported for the runtime-install consolidation.

No open issues 🎉

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

Cross-platform filesystem changes to runtime persistence and setup recovery require maintainer confirmation of native Windows testing and full-workspace validation on the combined tree.

No open issues 🎉

_{Review effort: Balanced}