[jussielo-amd]

Closes ROCMAI-867.

Summary

rocm install sdk --dry-run is meant to be a read-only preview, and the assistant's install_sdk_dry_run tool is listed among the read-only tools. On a host where no usable Python can be found (a minimal container, a fresh Windows install), it was not read-only: the preview went and prepared a managed Python before printing any plan. That meant downloading a managed uv, having uv install Python, and saving that Python's path in the CLI configuration. The preview needs a Python only to learn which wheel tags to select packages for, so none of that had to happen. Now the preview reports that a real install would prepare a managed Python, says so in a python_note: line, and prepares no Python and saves no Python path. A real install is unchanged. One qualification: the no-uv, no-tools-folder part holds for the canonical layout only, because a preview of a ROCm 10 or newer wheel resolves its package versions with uv pip compile and can still fetch the managed uv on a clean host (see the notes). The ticket was narrowed to the Python half this change delivers, and the uv half is split into a follow-up ticket of its own.

Reproduction

The ticket described this from reading the code and had not reproduced it. I reproduced it through the end-to-end harness against a build of origin/main. The scenario serves one canonical release pip index from a loopback fixture, points PATH at a single empty folder (so no Python is found) with ROCM_CLI_PYTHON unset, and points ROCM_CLI_UV_BINARY at a recording stand-in for uv so the attempt is observable without the network. Running rocm install sdk --channel release --format wheel --family gfx1200 --dry-run on that build printed Preparing Python 3.12..., Installing Python 3.12 via uv... and Finding Python 3.12... and exited 1 (the stand-in uv finds no Python), so the dry run had called the uv python install step. The path is install_wheel_runtime in apps/rocm/src/therock.rs calling resolve_python_launcher before its dry_run return, which falls through to ensure_managed_python in apps/rocm/src/therock/python.rs. The assistant tool, the MCP tool and rocm install sdk --dry-run all reach that same function, so one fix covers them.

What I did not demonstrate: a real download of uv and Python from the internet (the stand-in uv keeps the run offline, so I observed the call to it, not bytes arriving), and a Windows run.

After the fix the same scenario, a canonical-layout preview, passes: the preview exits 0 with a plan, the recording uv is never invoked (its log does not exist, which is the evidence that nothing was prepared), no managed tools folder exists under the data folder, and no config.json was written. The scenario also looks for a tool archive in its own cache folder, but only as a backstop: with ROCM_CLI_UV_BINARY set the CLI returns the stand-in before any download could start, so that folder stays empty either way and is not the evidence.

Fix

resolve_python_launcher_in is split at the point where it decides to prepare a managed Python. The search itself (an explicit ROCM_CLI_PYTHON, the PATH search, the saved managed Python, the bootstrap-disabled refusal) moves unchanged into find_python_launcher_in, which returns either a usable interpreter or NeedsManagedBootstrap. The real resolver calls ensure_managed_python on the second answer exactly as before. A new plan_python_launcher reports it as PlannedPython::WouldPrepareManaged instead, and python_for_install is the one place the wheel install chooses between the two based on dry_run. Every error the real resolution raises before the bootstrap step is raised for the preview as well, so a preview does not print a plan on a host where the install would refuse.

That includes a managed-Python version override. ROCM_CLI_MANAGED_PYTHON_VERSION can name a version whose wheel tag is not the layout's (for example 3.13 against the canonical cp312 layout); a real install prepares that Python and then refuses it, so the preview fails with the same message (Python wheel tag cp313 is not supported by this ROCm install; cp312 is required) instead of announcing a plan. This refusal is part of the same change rather than a separate one because it is the same rule the install applies: the comparison is one shared function, ensure_python_tag_matches, used both for an existing interpreter and for the version the preview would prepare, and without it the preview would announce a plan for a Python the install refuses. The tag is read off the version the way uv accepts it for a CPython release: 3.13, 3.13.1, 3.13.*, the free-threaded 3.13t, and any of those after cpython- or cpython@. Only the major and minor decide the tag, so what follows the minor is not validated; a wildcard that leaves the minor open (3.*) is unreadable. uv does the real parsing and this repo has no parser of its own to share, so this reader is deliberately small and can drift from what uv accepts, which is benign: a version whose tag cannot be read off its name (another implementation such as pypy@3.10, a range, a pre-release spelled on the minor) is announced as unchecked, not refused. The note and the progress line say its wheel tag cannot be checked until it is installed and what the install requires, rather than promising it, and both take that caveat from one function so they cannot word it differently.

For wheel selection the preview uses the interpreter tag the layout requires (cp312 for the canonical layout, cp314 for ROCm 10 and newer), because a real install refuses any other interpreter. The printed command shows uv venv --python 3.12 ... since the real path does not exist yet. The preview output gains python_note: only when a Python would be prepared, python_launcher: reads not prepared by this preview in that case, and python_source: is managed. Hosts that have a usable Python print exactly what they printed before. The interpreter's origin, previously a bare string on PythonLauncher, is now a PythonSource enum; the printed python_source: values are unchanged.

Every progress line the preview prints about Python is derived from the same decision the install takes, so a line that says what a real install would do cannot appear on a path that ends in a refusal. The opening line, which said if needed, ROCm CLI will prepare Python X, reads Checking Python for the ROCm install. in a preview and promises nothing. The two lines about an unusable PATH Python and an unusable saved managed Python used to say a real install would use ROCm CLI's managed Python and a real install would prepare Python again in a preview, which was premature: bootstrap can be disabled, or the version override refused, after them. In a preview they now state only the finding (Python from PATH is not usable for this ROCm install (cp312 is required)., and the same for the saved Python). The single line that predicts the preparation, No usable Python found; a real install would prepare Python X. This preview does not., comes from PlannedPython::progress_text, which exists only for a decision that already passed the bootstrap policy and the version check; python_for_install prints it, together with the Using ... lines of the other outcomes, which moved there from therock.rs so the whole Python output is one value that a unit test can capture. A real install prints all of its lines exactly as before, so non-dry-run output is byte-identical.

The two inputs that decide what happens when no interpreter is found, whether the managed bootstrap is disabled and which managed Python version is requested, used to be read from the process environment inside the decision. They now travel in the injectable PythonResolverEnv next to the search directories (the real resolver fills it from the process environment as before), and the preview, the install's ensure_managed_python and the bootstrap-disabled refusal all read them from it, so a decision made from one environment cannot disagree between preview and install and can be driven by a test without touching process-wide variables. Whether a preview could check the wheel tag of the version it would prepare is a TagCheck enum (Checked, Unchecked) rather than a bool carried beside the requirement, and the two lines about an unusable PATH Python and an unusable saved Python are built by one LookupPurpose::unusable_python_line instead of two parallel match blocks; the output of both is unchanged.

The dash-TUI tool-group comment in crates/rocm-dash-tui/src/agent/tools.rs listed this side effect for install_sdk_dry_run; it now says update_check can still prepare a Python, and that a ROCm 10 preview can still fetch the managed uv.

Verification

Ran, on Linux with a private target dir, on the final tree: cargo fmt --all --check; cargo clippy --locked -p rocm --all-targets -- -D warnings; cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings; cargo clippy -p rocm --all-targets --target x86_64-pc-windows-msvc --locked -- -D warnings (with stub C compilers for the ring and aws-lc-sys build scripts, since check and clippy never link them); cargo test -p rocm --bin rocm therock:: (216 passed, 1 ignored); the feature_naming guard; cargo xtask manifest --check; cargo xtask check-crate-edges; scenarios runtime-23 to runtime-25 against a debug build of the branch (they pass); the leak scan against origin/main. Each new test was shown to fail when the code it covers is broken; the runs are listed under Shown red below. Earlier in the PR's life the whole cargo test -p rocm passed (1474 tests, against 1467 on main at the time, before the later rounds added more unit tests) and runtime-23 was run against the unfixed build, where it fails.

Not run: the full workspace gate and scripts/smoke_local.py on the final tree, a real network download of uv and Python, and any Windows run of the tests (the Windows target only gets the clippy check, which builds the platform-neutral test described below; it runs on the Windows CI job).

Shown red

Mutations, each reverted by a targeted edit afterwards: making python_for_install ignore dry_run turns a_dry_run_with_no_usable_python_prepares_nothing and a_dry_run_with_an_unusable_saved_python_prepares_nothing red; making the preview save a Python path in the config turns the same two red and fails the runtime-23 Then step with the config path in the message; passing false instead of dry_run at the call site in therock.rs turns runtime-23 red at the When step with Installing Python 3.12 via uv... in the output; skipping the tag check in the preview turns a_dry_run_refuses_a_managed_python_version_the_install_would_refuse red; removing the t suffix handling, and separately the cpython- and cpython@ prefix handling, from the version reader turns both a_managed_python_version_is_read_as_the_wheel_tag_it_reports and a_dry_run_reads_every_spelling_of_a_managed_python_version_the_same_way red each time; making an unreadable minor read as 0, and separately dropping the shared unchecked-tag caveat, turns the reader test, the spelling test and the state matrix red; restoring the promising opening progress line in a preview turns runtime-24 red; restoring the premature a real install would use ROCm CLI's managed Python text on the PATH line, then on the saved-Python line, and printing the prediction before the version check each turn the state-matrix test a_dry_runs_whole_python_output_follows_one_decision_in_every_state red, naming the row; asserting against the shared cache folder again, with a leftover archive planted in it, reproduces the CI failure runtime-23 once had. On the head under review (c5ea04f) I re-ran two of these again: with python_for_install made to ignore dry_run, a_dry_run_with_no_usable_python_prepares_nothing, a_dry_run_with_an_unusable_saved_python_prepares_nothing, a_dry_run_refuses_a_managed_python_version_the_install_would_refuse, a_dry_run_reads_every_spelling_of_a_managed_python_version_the_same_way and the state matrix are red; with the tag check skipped in the preview, the version-refusal test, the spelling test and the state matrix are red. The platform-neutral test a_dry_run_decides_from_an_injected_environment_on_every_platform was shot red by ignoring dry_run, by skipping the tag check and by ignoring the disabled-bootstrap flag, and the test that pins the unusable-Python wording for an install and a preview by changing the install wording. Each mutation was reverted by a reverse edit.

User-visible behaviour

Scenario runtime-23 (runtime-install-preview-prepares-no-python) covers the preview on a host with no usable Python, runtime-24 (runtime-install-preview-refuses-unusable-managed-python) the version-override refusal, and runtime-25 (runtime-install-preview-refuses-disabled-managed-python) the policy refusal when the host's only Python is unusable and managed Python is disabled, all in tests/e2e-cucumber/features/runtime_setup.feature. All three are @requires-os:linux and need no GPU or network, so they run on the ordinary hermetic e2e lane. The empty-PATH host is built for the Linux search order. The decision itself runs on every platform, the Windows CI job included, in the unit test a_dry_run_decides_from_an_injected_environment_on_every_platform, which hands python_for_install an injected resolver environment with nothing to find and the policy written into it (no interpreter scripts): it asserts the whole progress output and the state left behind for the default, an accepted patch release, a blank override, an unreadable spelling, each refused spelling and the disabled bootstrap, with the install on the same host as the control. The Windows search order (python, python3, py with PATHEXT) and the interpreter probes are still covered by Unix-only tests, and no scenario runs the preview on a Windows host, so the Windows lane exercises the decision but not the search. runtime-23's final step asserts the preview's own claim (the python_note: line and the This preview does not. progress line) together with the state it left, with the recording uv's log as the evidence nothing was prepared. runtime-25 asserts that the output holds the neutral finding about the PATH Python, the same refusal as the install, no line predicting anything, and no side effects. runtime-24 asserts the final output as a whole: the neutral opening line, none of the promises the refusal would contradict (would prepare Python 3.13, ROCm CLI will prepare, a printed plan), the refusal text on stderr, and the same absence of side effects. Both give the CLI a cache folder of their own (ROCM_CLI_CACHE_DIR under the scenario root) because the harness otherwise shares one cache across scenarios and CI runs, where a leftover archive would trip the backstop check. fake-uv gains an opt-in FAKE_UV_LOG file so a scenario can show uv was never run. The wording of the progress line for a version whose tag cannot be read (pypy@3.10) is covered by unit tests only, not by a scenario.

The unit test a_dry_runs_whole_python_output_follows_one_decision_in_every_state is the state matrix: every combination of PATH Python (usable, unusable, absent), saved Python (usable, unusable, absent), bootstrap policy (enabled, disabled) and version override (default, accepted patch release, unreadable, refused tag), 72 rows, each asserting the complete progress output against an independently written expectation, that no prediction line exists unless the row ends in the one promise, and that uv was not run and no file or config was written. Other boundary cases are unit-tested in apps/rocm/src/therock/python.rs too: no Python and no config, data or cache folder at all (nothing is created); a usable Python on the search path; a usable saved managed Python (the manifest and config are left untouched); an unusable saved managed Python (reported as needing a Python, record left alone); managed bootstrap disabled (preview and install refuse with the same message); a version override that the install would refuse, in the spellings 3.13, 3.13t, cpython-3.13 and cpython@3.13; patch and cpython spellings of the accepted minor; and an unreadable spelling announced as unchecked. docs/testing.md and docs/manual-testing.md describe the preview behaviour and the manual check, including that the plan's uv venv --python 3.12 names a version rather than a path, so it is not copy-pasteable verbatim if uv resolves 3.12 to a different Python than the managed one a real install prepares. The --help text and README did not claim anything this change makes false, so they are unchanged; neither do docs/llm-tool-use.md nor the skill texts describe the preview's Python handling.

Notes for reviewers

The preview still writes the package-index metadata cache under the cache folder (the ticket mentions it): that is an HTTP cache of the index the plan is resolved from, and docs/testing.md says so. I left it alone rather than thread a no-write mode through the resolver. A preview of a ROCm 10 or newer wheel also still calls ensure_uv_binary through resolve_published_pip_package_versions, because it resolves the published versions with uv pip compile; on a clean host that can download the managed uv and create the data folder's tools folder, although no Python is prepared. This PR does not fix that; the ticket was narrowed to the Python half, and a follow-up ticket tracks this one. Only uv pip compile knows the torch and torchvision versions on that layout, and those versions are what the plan's package_specs, runtime_key, target, manifest and command: lines are made of; it is also the check that the published stack is jointly installable, so a preview that skipped it would print plans the install refuses. Avoiding the fetch means a different resolver for the preview (or a preview that prints those lines as unresolved), which is not a small behaviour-preserving change and runs through resolve_pip_runtime, which rocm update shares. docs/testing.md, docs/manual-testing.md, the scenario comments and the tool-group comment therefore limit the no-uv guarantee to the canonical layout. The update_check tool and rocm update --check resolve Python through the same resolve_python_launcher in therock/update.rs and can still prepare a managed Python on such a host; that is a sibling of this bug, outside what this change covers, and the corrected tool-group comment says so. It is a natural follow-up using the same plan_python_launcher seam. A preview that uses an existing Python still runs that interpreter and its -m venv probe (in a temporary folder that is removed), as it always did.

Reading order: therock/python.rs (the split, PlannedPython and the version reader), then the install_wheel_runtime hunks in therock.rs, then the scenarios and their steps. Open PRs touching the same code: #563 and #568 both edit the uv_venv_args(&python_launcher.executable, ...) line in the dry-run block of install_wheel_runtime, which this change replaces with python.venv_python_arg(); the textual conflict is one line and either resolution is mechanical. #569 changes ensure_uv_venv on the real-install path only. #820 and #819 edit other parts of therock.rs and the same feature and step files, so expect trivial hunk conflicts. #901 and #812 touch agent/tools.rs elsewhere than the comment block edited here. Another scenario named runtime-22 landed on main while this was open, so these three are numbered runtime-23 to runtime-25.

[Copilot]

🟡 Changes recommended

The preview can report an incompatible managed-Python plan as valid, and the E2E cache assertion can fail because of pre-existing shared-cache contents.

2 open issues

• Reject managed Python overrides that conflict with wheel layout · New
• Use a scenario-local cache for the runtime-22 tools check · New

What changed in this PR

This PR separates Python discovery from preparation so SDK dry-run previews can report a planned managed Python without downloading tools or saving its path. Real installs retain the existing preparation flow.

Changes:

• Add preview-aware Python planning and explanatory output.
• Add unit and end-to-end regression coverage, including recorded uv invocations.
• Document preview behavior and its remaining package-index cache writes.

File	Description
tests/​e2e-cucumber/​tests/​e2e/​therock_steps.rs	Add no-Python fixtures and side-effect assertions.
tests/​e2e-cucumber/​src/​bin/​fake-uv.rs	Add optional invocation logging.
tests/​e2e-cucumber/​features/​runtime_setup.feature	Add a Linux preview regression scenario.
docs/​testing.md	Describe coverage and remaining cache writes.
docs/​manual-testing.md	Document manual preview checks.
crates/​rocm-dash-tui/​src/​agent/​tools.rs	Correct the comment describing tool side effects.
apps/​rocm/​src/​therock/​python.rs	Separate discovery from preparation and test preview decisions.
apps/​rocm/​src/​therock.rs	Use planned Python information in SDK previews.

_{Review effort: Balanced}

[jussielo-amd]

Fixed in 1936bce: the runtime-22 Given now points the cache at a per-scenario scratch folder (ROCM_CLI_CACHE_DIR), so a warm shared cache can no longer fail it; I reproduced the CI failure by planting a leftover archive in the shared cache and the fixed scenarios pass in that setup. The recording uv log is the evidence that nothing was prepared, and the cache check is a backstop (794081a).

[Copilot]

🟡 Changes recommended

Python previews claim preparation before policy and version checks pass, while testing docs omit ROCm 10 metadata resolution’s possible managed-uv download and tools-directory creation.

3 open issues

• Use a scenario-local cache for the runtime-22 tools check
• Preview messages claim managed Python before bootstrap checks pass · New
• ROCm 10 previews may bootstrap uv despite Python bootstrap being skipped · New

1 resolved since last review

• Reject managed Python overrides that conflict with wheel layout

_{Review effort: Balanced}

[Copilot]

🔵 Needs a closer look

A clean-host run with real uv is still needed to confirm ROCm 10 previews work without provisioning Python, since the new no-Python tests cover only the canonical layout.

2 open issues

• Use a scenario-local cache for the runtime-22 tools check
• ROCm 10 previews may bootstrap uv despite Python bootstrap being skipped

1 resolved since last review

• Preview messages claim managed Python before bootstrap checks pass

_{Review effort: Balanced}

[juhovainio]

Thanks for the work here — the Python download and config write are gone from the preview, and CI is green. Requesting changes on one scope point, plus a few smaller notes.

Needs resolution

• ROCMAI-867's acceptance says "no download of Python or uv". A ROCm 10+ preview can still fetch the managed uv (and create tools/) via uv pip compile. The PR discloses this but is titled/linked as "Fixes ROCMAI-867". Either finish the work (no uv fetch in the preview) or narrow the "Fixes" to what is actually fixed.

Non-blocking

• The doc comment on python_tag_for_version says a wildcard yields None, but the unit test asserts "3.13.*" returns cp313 (inline comment).
• The hand-rolled version reader can drift from what uv accepts. The failure is benign (the version is announced as unchecked, not refused), but worth a comment saying so.
• At ~1800 lines this is large for a fix, and the version-override refusal (runtime-24/25) goes beyond the ticket. Splitting it out would make review easier.
• Windows runs none of the dry-run decision tests (cfg(unix) tests, @requires-os:linux scenarios). Disclosed, noted for the record.
• update_check still prepares a managed Python (disclosed). docs/llm-tool-use.md and the skill text are not touched; please confirm they don't describe the old preview behaviour.
• Minor: tag_checked looks speculative, note() and progress_text() duplicate wording, and source is a bare string.

CI is all green on 72551d7.

[juhovainio]

This says a wildcard yields None, but the unit test asserts "3.13.*" -> cp313 (the patch part isn't validated). Please reword the comment to match the behavior (or tighten the parser if None was intended).

[jussielo-amd]

You are right that the comment and the test disagreed, and the behaviour is the intended one: only the major and minor decide the wheel tag, so 3.13.* reads as cp313 and whatever follows the minor is not validated. What yields None is a wildcard that leaves the minor open, such as 3.* or *. Fixed in 4ef1703: the doc comment now says exactly that, and 3.* and * are added to the unreadable rows of a_managed_python_version_is_read_as_the_wheel_tag_it_reports so the reworded claim is pinned from both sides. I did not tighten the parser: the part after the minor cannot change the tag, so reading it is correct, and a stricter reader would only turn versions whose tag is known into unchecked ones.

[jussielo-amd]

Answers to the review points (in the order of the review), for the push at c5ea04f.

Needs resolution (uv fetch on a ROCm 10 or newer preview): I looked at finishing it and it is not a contained change, so I narrowed the scope instead of widening the diff: the ticket is now limited to the Python half this PR delivers, and the uv half is split into a follow-up ticket of its own, so that each ticket has exactly one PR. What this PR delivers is the Python half of the acceptance: no managed Python is prepared and no Python path is saved, and on the canonical layout there is also no uv download and no tools folder. The uv half is not done for ROCm 10 and newer previews. The reason is that uv pip compile is the only place the torch and torchvision versions come from on that layout (the CLI itself picks the rocm version and the torchaudio pin). Those versions feed the package_specs, runtime_key (a hash of the specs), target, manifest and command: lines of the plan, so a preview that skips uv either prints those with placeholders or invents them. uv pip compile is also what checks that the published stack is jointly installable, and a preview without it would print a plan on hosts where the real install refuses, which is the property this PR sets out to keep. The other way, selecting torch and torchvision from the index listings the way the canonical layout does, is a second resolver with different results from uv, not a small behaviour-preserving change. It also runs through resolve_pip_runtime, which update and the startup check share. That follow-up ticket is filed and starts after this PR merges. In this push there is no behaviour change for it; the docs and the tool-group comment already limit the no-uv guarantee to the canonical layout.

Wildcard doc comment: fixed in 4ef1703, see the inline reply.

Hand-rolled version reader can drift from what uv accepts: added in 4ef1703. The doc comment on python_tag_for_version now says the reader is an approximation that can drift from uv in either direction, and that both directions are benign: an unreadable spelling is announced as unchecked rather than refused, and a spelling that reads as a tag but that uv then rejects only means the preview predicted an install that fails at uv python install with uv's own error.

PR size and the version-override refusal (runtime-24 and runtime-25): I did not split it. The refusal is not an extra feature but the same rule the install applies: ensure_python_tag_matches is shared by the existing-interpreter check and by the preview's check of the managed version it would prepare, and without it the preview would announce a plan for a Python the install refuses, which is the preview-must-match-the-install requirement of the ticket. The state-matrix test and the scenarios assert the refusal and the preview's lines together, so pulling it out would leave the first half with a known false "would prepare" line. The new commits in this push are small (97 insertions, 54 deletions, all in therock/python.rs, therock/update.rs and therock.rs) and the reading order in the description still holds.

Windows runs none of the dry-run decision tests: acknowledged, unchanged. The tests are cfg(unix) because the Python stand-ins are shell scripts, and the scenarios are @requires-os:linux. The non-test code has no cfg(windows) branches, and cargo clippy -p rocm --all-targets --target x86_64-pc-windows-msvc -- -D warnings passes on the new commits (with stub C compilers, because the aws-lc-sys build script does not cross-compile on my host).

update_check still prepares a managed Python, and the docs and skill text: update_check is disclosed and stays a separate follow-up. I grepped docs/llm-tool-use.md, skills/rocm-cli-assistant/SKILL.md, skills/rocm-doctor/SKILL.md and reference.md, README.md and docs/*.md for install_sdk_dry_run, install sdk --dry-run and the words dry run and preview. None of them describes the old behaviour: docs/llm-tool-use.md and both skills never mention the preview's Python handling (the only hits for those words are about rocm fix --dry-run and rocm update), README only shows the command, and docs/architecture.md only names install_sdk_dry_run in a module map. The model-facing tool descriptions ("Preview a TheRock SDK install without changing the machine") are generic and unchanged. The two documents that do describe the preview, docs/testing.md and docs/manual-testing.md, are already scoped to the canonical layout.

tag_checked, note() and progress_text(), and source: three separate judgments. tag_checked is not speculative: it is the one fact that note(), progress_text() and the state-matrix test all branch on (the version's tag could be read off its name or not), and carrying it in the decision is what stops those from re-parsing the version and disagreeing, so I kept it. The duplicated wording is fixed in 148f9fb: both lines now take the caveat from one function, unchecked_tag_caveat. The note is unchanged, and the progress line now says "(its wheel tag cannot be checked until it is installed; cp312 is required)" in the same words, with the unit tests that pin both lines updated. source is fixed in c5ea04f: PythonLauncher::source was a bare string compared in three places with a catch-all arm, so it is now a PythonSource enum (Env, Path, Managed, Manifest) matched exhaustively, and the printed python_source: line is unchanged.

Checks on the new commits: cargo fmt --all --check, cargo test -p rocm --bin rocm therock:: (214 passed, 1 ignored), scenarios runtime-23 to runtime-25 through cargo xtask e2e (3 scenarios passed), feature_naming, clippy for rocm and for e2e-cucumber, Windows-target clippy for rocm, cargo xtask manifest --check and check-crate-edges. A mutation of the version reader (an unreadable minor reads as 0) turns three tests red, and so does disabling the shared caveat.

[Copilot]

🔵 Needs a closer look

The ROCm 10 dry run still invokes uv pip compile, leaving real uv interpreter-bootstrap behavior unverified with no installed Python outside the canonical layout.

1 open issue

• Use a scenario-local cache for the runtime-22 tools check

1 resolved since last review

• ROCm 10 previews may bootstrap uv despite Python bootstrap being skipped

_{Review effort: Balanced}

[juhovainio]

I went through this PR against the ticket's narrowed scope (the Python half of the preview fix). The core change looks right: the preview no longer prepares Python, the printed plan is derived from the same decision value the branch uses, and the new scenario asserts both the message and the resulting state (no uv call, no tools dir, no saved config), with a control showing the real install does reach uv. I found nothing that is functionally wrong, but I'd like the following addressed before merge:

1. Show the new tests fail when the guard is removed. I couldn't find a statement that the unit tests and the state matrix were run red with the guard reverted (the PR body only mentions reproducing the e2e scenario on main). Please do that and note it in the PR text.
2. Windows coverage. The ticket names a fresh Windows install as an affected host, but the unit tests are cfg(unix) and runtime-23/24/25 are @requires-os:linux. Either add a platform-neutral test of the decision logic (e.g. with an injectable resolver environment instead of shell-script fake interpreters) so it runs on the Windows job, or state in the PR which lane exercises the Windows preview.
3. Scope. The managed-Python version-override refusal, the hand-rolled uv version parser and its 72-row matrix, plus runtime-24/25, roughly triple the size of the fix and go beyond the ticket text. Please consider splitting that into its own PR so this one stays a single logical change.

Smaller notes, not blocking on their own:

• tag_checked: bool travelling together with requirement reads like a small enum (TagCheck) waiting to happen.
• The two parallel Install/Preview progress-line match blocks in find_python_launcher_in differ only by a suffix and could share one.
• The printed uv venv --python 3.12 names a version rather than a path, so it isn't copy-pasteable verbatim if uv resolves 3.12 to a different interpreter than the managed one. Fine if intentional, but worth a line in the docs.
• Package-index cache writes and the ROCm 10+ uv fetch remain in the dry run; documented and tracked separately, so no change requested.

[juhovainio]

This is a hand-rolled reader of uv's version grammar. Drift is documented and benign, but it (together with the override refusal in ensure_python_tag_matches) is beyond what the dry-run no-prepare fix needs; consider moving it to a separate PR.

[jussielo-amd]

I kept the version-override refusal and the version reader in this PR rather than splitting them out, deliberately. The refusal is the same rule the install applies, not an extra feature: a real install prepares the managed Python named by ROCM_CLI_MANAGED_PYTHON_VERSION and then refuses it when its wheel tag is not the layout's, and the comparison is one shared function, ensure_python_tag_matches, used both for an interpreter that exists and for the version the preview would prepare. Without it the preview would announce a plan for a Python the install refuses, which is the false-promise class this PR exists to remove, so splitting it would leave the first PR with a preview that can still print a plan the install will not carry out. The reader is small and deliberately approximate (its doc comment says it can drift from what uv accepts, and that either direction is benign: an unreadable spelling is announced as unchecked, never refused). I did take the reviewer's size concern into account where it was cheap: the new commits only touch the resolver's tests and wiring, and the reader itself is unchanged. If you would still rather see it as its own change after reading this, say so and I will split it.

[juhovainio]

This scenario is Linux-only, while the ticket names fresh Windows installs as affected. Please say which CI lane, if any, exercises the Windows preview, or add a platform-neutral unit test of python_for_install with an injectable resolver environment.

[jussielo-amd]

Fixed with a platform-neutral test rather than a statement. The preview's Python decision needs no interpreter once the resolver's environment carries the managed-Python policy, so 41f30a1 moves the two policy inputs (the disabled-bootstrap flag and the requested managed version) into the injectable PythonResolverEnv, which the real resolver still fills from the process environment, and 8f93821 adds a_dry_run_decides_from_an_injected_environment_on_every_platform. It runs python_for_install through an injected environment with nothing to find, has no cfg(unix) and no shell-script interpreters, so it runs on the Windows CI job. Each row asserts the complete progress output and the state left behind (no config, manifest, or folder created): the default, a patch release, a blank override and an unreadable spelling are announced (the last as unchecked), a version whose wheel tag is not the layout's is refused in every spelling that reads as one, and a disabled bootstrap is refused with the install's own message. A control runs the real install on the same host and checks it reaches the managed uv (a file that cannot be launched, so nothing is downloaded), so the silence about uv in the preview rows means something. I shot it red with three targeted mutations, each reverted by a reverse edit: making python_for_install ignore dry_run, skipping the tag check in the preview, and ignoring the disabled-bootstrap flag each turn it red at the expected row. What is still not covered on Windows: the python, python3, py search order with PATHEXT and the interpreter probes (those tests need script interpreters and stay Unix-only), and any end-to-end run of the preview on a Windows host; scenarios runtime-23 to runtime-25 remain @requires-os:linux. docs/testing.md and the runtime-23 comment now say exactly that (afbb38c).

Addressed in c5ea04f, 148f9fb and 4ef1703, with replies on the threads and on the PR. The uv half is split into a follow-up ticket and the ticket scope narrowed; please re-review.

[jussielo-amd]

A correction from me: I dismissed the change-request review of 10:41 UTC (the one on c5ea04f) before addressing it, which was premature. Its findings still stand and I am working through them: the platform-neutral test of the decision logic for the Windows preview (or a statement of which lane covers it), the smaller notes on tag_checked, the duplicated progress-line blocks and the uv venv --python 3.12 wording in the docs, and a reply on the red-by-mutation evidence and the scope question. Please re-review once those are pushed; the earlier replies on the threads still describe the current head.

[jussielo-amd]

Answers to the review points, for the push at afbb38c.

Going through the points in order. Point 1 (show the new tests fail when the guard is removed): the PR body had this in one long paragraph that was easy to miss, so I added a short Shown red subsection and a line in Verification that points to it. I also re-ran two of the listed mutations on the head you reviewed (c5ea04f) before changing anything. With python_for_install made to ignore dry_run, a_dry_run_with_no_usable_python_prepares_nothing, a_dry_run_with_an_unusable_saved_python_prepares_nothing, a_dry_run_refuses_a_managed_python_version_the_install_would_refuse, a_dry_run_reads_every_spelling_of_a_managed_python_version_the_same_way and the 72-row state matrix a_dry_runs_whole_python_output_follows_one_decision_in_every_state go red (three unrelated resolver tests also failed in that run, because a panicking test poisons the shared environment lock). With the tag check skipped in the preview, a_dry_run_refuses_a_managed_python_version_the_install_would_refuse, a_dry_run_reads_every_spelling_of_a_managed_python_version_the_same_way and the state matrix go red. Both were reverted by reverse edit and the tree was clean afterwards. The new tests from this round were shot red the same way (see the reply on the Windows point), and the pin added before the progress-line refactor was shown red by changing the install wording.

Point 2 (Windows coverage): added a platform-neutral test, 8f93821 on top of 41f30a1; details are in the reply on the runtime_setup.feature thread.

Point 3 (scope): I am not splitting it, for the reason in the reply on the python.rs:897 thread: the refusal is the rule the install itself applies, and the preview must not announce plans the install refuses. I would be glad to be told if you still think it should be separate.

Smaller notes: tag_checked: bool is now a TagCheck enum with Checked and Unchecked (869b8cb), output unchanged. The two parallel Install/Preview progress-line blocks in find_python_launcher_in now share LookupPurpose::unusable_python_line (6176347); because the output had to stay byte-identical, 93c6908 first pins both purposes' wording, on every platform, and passes on the unchanged code, and the pin goes red when the shared helper's wording is changed. The uv venv --python 3.12 caveat is documented: docs/testing.md and docs/manual-testing.md now say the printed command names a version, not a path, so uv resolves it itself and may choose a different Python 3.12 than the managed one a real install prepares (afbb38c). The cache-write and ROCm 10 uv fetch notes needed no change, as you said.

[Copilot]

🟢 Approval recommended

No active review issues are recorded for the changes preventing dry-run SDK previews from bootstrapping managed Python or saving its path.

1 open issue

• Use a scenario-local cache for the runtime-22 tools check

_{Review effort: Balanced}

[juhovainio]

I read through the full diff and found nothing blocking. The preview now takes its Python decision from the same value the real install branches on, so the printed plan and the behaviour can't drift apart. Scenarios runtime-23 to runtime-25 assert the message and the resulting state together (no uv call, no tools dir, no config written), and the injected-env unit test runs the decision logic on the Windows lane.

A few non-blocking notes:

• The preview's command: line shows uv venv --python 3.12 (a version), whereas a real install passes an interpreter path. The docs call this out, so I'm fine with it.
• The PR is broader than the fix: the PythonSource enum, resolver-env injection, a hand-rolled python_tag_for_version reader (which can drift from uv's own parsing) and about 1000 lines of tests in python.rs. That file may be worth splitting later.
• The preview's refusal of a wrong-tag managed Python assumes a real install "prepares then refuses" it. That logic is in ensure_managed_python, outside this diff, so I couldn't check it. runtime-24 only covers the preview side.
• One long unwrapped line in docs/manual-testing.md (the "This holds for the canonical layout..." sentence).

This is a diff-only read. I didn't build or run the tests, and several CI jobs (clippy, build-and-test, Windows, coverage, commit signatures) were still pending when I looked, so please wait for those to go green before merging.

[jussielo-amd]

Answers to the notes of the approving review, for the push at fc2fa6e.

The long unwrapped line in docs/manual-testing.md is wrapped, and so is a similar paragraph in docs/testing.md (no word changed, checked with a word-level diff).

On the preview's refusal of a wrong-tag managed Python: the install side does behave as the preview assumes. ensure_managed_python installs the version with uv, finds the interpreter with uv python find, and then calls python_launcher_install_ready, which applies ensure_python_tag_matches to the interpreter's wheel tag. So a real install prepares the Python first and then refuses it, and the preview applies the same ensure_python_tag_matches rule to the version it would prepare. runtime-24 covers the preview side only, because the install side would need a real uv python install; the shared rule is what keeps the two together.

The uv venv --python 3.12 wording is in the docs as noted, and splitting python.rs further is something I will keep in mind for a later change.

[Copilot]

🔵 Needs a closer look

The ROCm 10 preview needs validation on a Python-free host because it invokes uv pip compile without disabling Python downloads, while end-to-end tests cover only canonical resolution.

1 open issue

• Use a scenario-local cache for the runtime-22 tools check

_{Review effort: Balanced}