fix(uv): colocate the uv Python install dir with the ROCm CLI data dir

[jgmelber]

Summary

Follow-on to #170. That PR pointed uv at a cache inside the ROCm CLI data directory; this one does the same for the standalone CPython interpreters uv python install downloads, which still land in $HOME.

Root cause

uv_command_env() now sets UV_CACHE_DIR, but never set UV_PYTHON_INSTALL_DIR. ensure_managed_python shells out to uv python install, so the interpreter falls back to uv's own default — $HOME/.local/share/uv/python/ on Linux/macOS, %USERPROFILE%\.local\share\uv\python\ on Windows — regardless of where ROCM_CLI_DATA_DIR points.

That is ~100MB of standalone CPython per interpreter version, outside the managed root. Measured with uv 0.12.3 against a throwaway HOME:

$ uv python install 3.12          # UV_PYTHON_INSTALL_DIR unset
$ du -sh $HOME/.local/share/uv/python/cpython-3.12.12-linux-x86_64-gnu
104M

With the variable set to a managed location, the same 104M lands under the data dir and $HOME keeps only uv's 12KB bin/python3.12 shim — a symlink into the managed interpreter, not a copy.

This is the same class of bug as #160 and it bites the same users: a split /home, a container overlay, or anyone who set ROCM_CLI_DATA_DIR to a larger disk and expects the CLI to stay there. rocm uninstall also could not reclaim it, since it only removes directories it knows about.

Technical decisions

Keyed off data_dir, for the same reason as the cache: uv hardlinks interpreters into the environments it creates, and ROCM_CLI_CACHE_DIR can point at a different filesystem from those environments.

Mirrors UvCacheSource exactly. UvPythonInstallDirSource has the same Managed / Override / Inherited variants, the same path() / is_override() surface, and the same precedence: namespaced ROCM_CLI_UV_PYTHON_INSTALL_DIR > ambient UV_PYTHON_INSTALL_DIR > managed default. It reuses meaningful_cache_dir for trimming rather than duplicating it. The two variables should read as one design applied twice, not two designs.

uv_command_env(&AppPaths) keeps its signature. Both env vars are composed inside it, so therock.rs, comfyui.rs, and the vLLM engine pick up the fix with no call-site edits. This matters for correctness beyond tidiness: ensure_managed_python calls uv python install and uv python find through the same helper, so find cannot disagree with where install put the interpreter.

A one-shot notice, not a migration, following maybe_notice_legacy_uv_cache with its own marker file. Nothing is moved or deleted — the legacy directory may hold interpreters other uv projects depend on.

Non-goal: --prefix installs — #187

Same gap as the cache, same reason. --prefix relocates install_root only; the interpreters stay keyed off data_dir. Documented in docs/manual-testing.md and pinned by uv_python_install_dir_does_not_follow_a_prefix_install_root, mirroring the existing cache test.

Consequences worth noting

• rocm uninstall now reclaims the interpreters too, since they sit under the data directory. --keep-data / --keep-cache help text updated to say so; --keep-cache does not cover them.
• Where the install dir has been pointed outside the data directory, uninstall now reports it as a shared cache it is not removing, extending fix(uninstall): report the caches it does not remove #175's reporting rather than silently leaving ~100MB behind.
• The interpreters are no longer shared with other uv projects. ROCM_CLI_UV_PYTHON_INSTALL_DIR restores that.
• Existing installs re-download the interpreter once.

Tests

Six new resolution tests mirroring the cache suite — unset, empty, whitespace-only, trimmed, inherited, namespaced-override, and precedence between the two — driving the pure resolver, so they need no set_var and stay parallel-safe. managed_python_install_dir_tracks_rocm_cli_data_dir goes through AppPaths::with_managed_root.

The uninstall-reporting test is the exception: it drives the real shared_cache_candidates() behind the existing env lock, because a version calling the pure helper with hand-built inputs still passed when the production block was deleted. Each new test was mutation-checked — production code removed, test confirmed failing, code restored.

cargo fmt --all --check and cargo clippy --locked --workspace --all-targets -- -D warnings are clean. Full workspace suite passes.

Manual verification

uv python install 3.12 under a throwaway HOME with both variables set: the 104M interpreter lands under the managed data dir, and $HOME receives only the 12KB shim symlink.

[fredespi]

Reviewed at c340bbe0 (diff against f8e9629, 7 files, +346/-29). One thing to fix, and it's narrow. The rest is careful work that does what the description says.

The uninstall report doesn't honour the override this PR introduces

shared_cache_candidates() resolves the interpreter location with:

let uv_python = env_path("UV_PYTHON_INSTALL_DIR").or_else(|| home/.local/share/uv/python);

but uv_python_install_dir_source() — the thing that actually decides where interpreters go — puts the namespaced variable first: ROCM_CLI_UV_PYTHON_INSTALL_DIR > UV_PYTHON_INSTALL_DIR > managed. So with ROCM_CLI_UV_PYTHON_INSTALL_DIR=/mnt/big/pythons set and the ambient variable unset — which is precisely the configuration this PR documents for restoring sharing with other uv projects — the interpreters land in /mnt/big/pythons, while rocm uninstall reports $HOME/.local/share/uv/python: a path that is stale, may not exist, and isn't where the ~100MB is. The real location is never mentioned.

That contradicts the stated consequence in the description ("uninstall now reports it as a shared cache it is not removing ... rather than silently leaving ~100MB behind"). It holds for the ambient variable and fails for the namespaced one.

The new test pins only the ambient path (ScopedEnvVar::set_path("UV_PYTHON_INSTALL_DIR", …)), so it passes without covering the gap.

Cleanest fix is to stop re-deriving the location in main.rs and call the resolver that already encodes the precedence — uv_python_install_dir_source(&paths).path() — with the same treatment for the cache entry above it. Worth noting the cache entry has the identical gap on main today (UV_CACHE_DIR read directly, ROCM_CLI_UV_CACHE_DIR ignored), so this isn't a regression you introduced — but this PR adds a second instance of it and makes a reporting promise that depends on it, which is what makes it worth closing now rather than inheriting.

What I checked and found sound

The two variables really are one design applied twice. UvPythonInstallDirSource mirrors UvCacheSource variant-for-variant, reuses meaningful_cache_dir for blank/whitespace trimming instead of re-implementing it, and the tests cover the same matrix — managed default, inherited, namespaced-wins-over-ambient, blank-is-not-an-override, whitespace-trimmed, and tracking a moved ROCM_CLI_DATA_DIR.

Keeping uv_command_env(&AppPaths)'s signature is load-bearing, not just tidy. Because both variables are composed inside it, uv python install and uv python find are guaranteed to agree on the location — a real correctness property, since a find that disagreed with install would silently re-download or resolve the wrong interpreter. Call sites in therock.rs, comfyui.rs and the vLLM engine pick it up untouched.

The --prefix non-goal is pinned, not just documented — uv_python_install_dir_does_not_follow_a_prefix_install_root mirrors the existing cache test, so the caveat can't quietly drift.

The legacy notice is correctly conservative — gated on the managed dir existing, skipped under an override, one-shot behind a marker file, and it neither moves nor deletes anything. Right call given other uv projects may depend on those interpreters.

Non-blocking

maybe_notice_legacy_uv_python_install_dir reads HOME/USERPROFILE directly, while the shared_cache_candidates hunk in this same diff uses rocm_core::runtime_home_dir(), which additionally handles the HOMEDRIVE+HOMEPATH fallback on Windows. The direct read is faithful to the sibling maybe_notice_legacy_uv_cache it's modelled on, so it's consistent with what's there — but the PR now contains both spellings a few hundred lines apart. Worst case is a missed advisory line on an unusual Windows setup, so it's cosmetic; just easier to unify while both are being touched.

Diff review only — I haven't run a real uv python install against a relocated data dir, so the ~104MB figure and the bin/python3.12 shim behaviour are taken from your measurement, not reproduced. CI is green (23/23).

[jgmelber]

Good catch on the precedence bug — you're right, it was exactly backwards from what the PR was trying to achieve. Pushed a fix:

• shared_cache_candidates() no longer re-derives the uv cache / python install dir paths from the ambient variables. It now calls uv_cache_source() / uv_python_install_dir_source() directly, so uninstall reporting resolves through the same namespaced-override-first precedence that uv_command_env() actually uses. Applied to both the cache dir and the install dir, as you suggested.
• Added shared_cache_candidates_respect_the_namespaced_override, which drives the real candidate list through ROCM_CLI_UV_CACHE_DIR / ROCM_CLI_UV_PYTHON_INSTALL_DIR and asserts both resolve to the override path. This is the case the existing test missed.
• Also took the cosmetic nit: the legacy-notice functions now go through runtime_home_dir() instead of an inline HOME/USERPROFILE check, matching the Windows-aware helper already used elsewhere in this module.

Rebased onto current main. Locally: cargo fmt --check, cargo clippy --all-targets -- -D warnings, and the full rocm test suite are all green — CI is running now.

[rominf]

I read through this diff (origin/main...pr/240, all 7 files) against the PR description and didn't find anything blocking.

What I checked:

• UvPythonInstallDirSource mirrors UvCacheSource variant-for-variant (Managed/Override/Inherited, same path()/is_override() surface, same precedence: ROCM_CLI_UV_PYTHON_INSTALL_DIR > UV_PYTHON_INSTALL_DIR > managed, same blank/whitespace trimming via the shared meaningful_cache_dir helper).
• uv_command_env(&AppPaths) keeps its signature and now composes both env vars, so the existing call sites in therock.rs (ensure_managed_python's uv python install and uv python find, plus venv creation and the generic run_uv_progress_command), comfyui.rs, and the vLLM engine all pick up the fix with no changes needed. install and find go through the same helper, so they can't disagree about where the interpreter landed.
• shared_cache_candidates() now resolves both the uv cache and the python install dir through uv_cache_source()/uv_python_install_dir_source() directly, so uninstall reporting uses the same namespaced-override-first precedence as the actual uv invocation — I saw the earlier review comment about this being backwards, and the fix for it is already in at the current head (43c6fd59), applied consistently to both the cache and the new python-install entry.
• --keep-cache / --keep-data help text and behavior are consistent: both uv-cache and uv-python live under data_dir, so only --keep-data covers them, matching the updated flag descriptions.
• The --prefix non-goal is pinned by a real test (uv_python_install_dir_does_not_follow_a_prefix_install_root), mirroring the existing cache test, and documented in docs/manual-testing.md.
• The legacy-notice function is conservative: gated on the managed dir already existing, skipped under an override, one-shot via its own marker file, and doesn't move or delete anything.

This is a diff-only read, so it doesn't replace CI or a maintainer's own pass — just flagging that from a static review I don't see anything to request changes on.

[r0x0r]

Clean follow-on to the cache work — mirroring UvCacheSource exactly (same variants, precedence, and blank-handling), keying off data_dir for the hardlink-filesystem reason, and keeping uv_command_env(&AppPaths)'s signature so therock.rs/comfyui.rs/vLLM pick it up with no call-site edits all read well. The resolver tests driving the pure function (no set_var, parallel-safe) plus the env-locked uninstall-reporting test are a good split, and mutation-checking each is the right rigor.

One substantive question on the legacy notice: maybe_notice_legacy_uv_python_install_dir builds the legacy path as runtime_home_dir() + [.local, share, uv, python], i.e. the XDG/Unix layout. The PR body states the Windows default is %USERPROFILE%\.local\share\uv\python\, but as far as I know uv resolves its managed-Python directory through the platform data dir on Windows (%APPDATA%/%LOCALAPPDATA%), not %USERPROFILE%\.local\share. If that's right, the notice never matches on Windows, so Windows users who already leaked ~100MB interpreters wouldn't be told they can reclaim them. It's at least consistent with maybe_notice_legacy_uv_cache (also Unix-layout), so this isn't a new inconsistency — but can you confirm uv's actual default install dir on Windows, and if it differs, either widen the legacy check or note the Windows gap explicitly? The fix itself (setting UV_PYTHON_INSTALL_DIR) works regardless; this is only about the one-shot reclaim hint.

[jgmelber]

but can you confirm uv's actual default install dir on Windows, and if it differs, either widen the legacy check or note the Windows gap explicitly

I have not tested this on Windows, I dont have a Windows machine to test on. I believe that is outside the scope of this PR. It would make a great follow on.

[siloteemu]

🔴 Automated review · pr-review-watcher · 43c6fd5

Summary

Pins UV_PYTHON_INSTALL_DIR so uv python install places standalone CPython under <data-dir>/uv-python instead of uv's own default, adds a one-shot legacy-location notice, and folds both uv directories into uninstall reporting. Needs work — the relocation silently never takes effect for the users it was written for, and one new test cannot fail. Verified: read the full diff and surrounding files; confirmed uv python install/find and every other uv spawn do apply uv_command_env (no bypass); traced the migration path through the managed-Python manifest and confirmed existing installs keep using the legacy interpreter and never trigger the notice; checked each new test against "would it pass if the production change were reverted?" (most would fail — one would not); checked uv's documented default install dir against the constant and the new doc text; on the red check I could not fetch logs, and evidence favours a build-and-test runner hang while branch drift remains a live alternative (see below). Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:1205 — the relocation and its notice are both dead for existing users; rocm uninstall then reports nothing about the interpreter that actually exists.
maybe_notice_legacy_uv_python_install_dir returns early unless <data-dir>/uv-python is already a directory. That directory is only created when uv python install runs. But apps/rocm/src/therock.rs:3073-3089 and :3210-3216 return early whenever the managed-Python manifest (<data-dir>/tools/registry/python.json, therock.rs:3008) records an executable that still exists and passes python_launcher_install_ready. For a user who installed before this PR, that manifest points into $HOME/.local/share/uv/python/..., that file still exists, so uv python install never runs, <data-dir>/uv-python is never created, and the notice never fires — not late, never. The interpreter stays at the legacy path indefinitely, outside the managed root the PR exists to establish. Worse, shared_cache_candidates (main.rs:16189-16192) reports uv_python_install_dir_source(paths).path(), i.e. the managed path, which shared_cache_notes_for filters out for being inside data_dir — so the uninstall plan stays silent about the legacy interpreter it is leaving behind, which is precisely the truthfulness goal stated in the new test's own doc comment. Note this differs from the uv-cache precedent it copies: the cache is repopulated on every uv operation, so its managed dir appears promptly and its notice does fire.
Fix: gate the notice on the legacy directory existing rather than on the managed one, and invalidate/re-resolve the managed-Python manifest when the recorded executable falls outside the resolved install dir (uv_python_install_dir_source), so the relocation actually applies on upgrade. Alternatively add the legacy path to shared_cache_candidates when it exists, so uninstall at least tells the truth.

apps/rocm/src/therock.rs:4281-4296 — uv_python_install_dir_does_not_follow_a_prefix_install_root passes whether or not the production change works.
The test computes install_dir = managed_uv_python_install_dir(&paths.data_dir), then asserts it does not start with a prefix_root that never enters the computation, and that it does start with paths.data_dir — which is what Path::join guarantees by construction. Both assertions are tautologies over the helper; revert the PR's actual behaviour change (setting UV_PYTHON_INSTALL_DIR on spawned uv commands) and this test still passes so long as the helper exists. It restates the implementation rather than the property. Fix: assert the property that matters — that the value uv_command_env actually emits for UV_PYTHON_INSTALL_DIR is unaffected by a --prefix install root, and shares a root with the venv/data dir. (The same tautology exists in the neighbouring uv_cache_does_not_follow_a_prefix_install_root; new code should not inherit it.)

apps/rocm/src/main.rs:1189 and docs/manual-testing.md:56-61 — the legacy location is wrong on Windows and ignores XDG_DATA_HOME.
Per uv's storage reference, managed Python versions live in a python/ subdirectory of uv's persistent data dir, which resolves to $XDG_DATA_HOME/uv then $HOME/.local/share/uv on Unix, and %APPDATA%\uv\data on Windows. The constant hardcodes [".local","share","uv","python"] relative to home, so the notice can never fire on Windows (a platform this repo builds and tests: windows-build-and-test), and misses Unix users with XDG_DATA_HOME set. The new doc paragraph compounds it by asserting the Windows default is %USERPROFILE%\.local\share\uv\python\ — factually wrong, in the Windows-oriented manual-testing doc where a tester will act on it. Fix: resolve the legacy location the way uv does (or shell out to uv python dir before applying the override), and correct the documented Windows path to %APPDATA%\uv\data\python.

Non-blocking

• apps/rocm/src/main.rs:16184-16187 — behaviour regression: uninstall previously reported $HOME/.cache/uv when no override was set; it now reports the managed path, which is filtered out, so an upgraded user's large legacy uv cache is no longer mentioned at uninstall time. Not called out in the PR description.
• apps/rocm/src/main.rs:1223-1227 — the notice invites deleting a tree that existing venvs may reference (uv's docs warn that moving the install location is not reflected in existing virtual environments). Self-healing in practice, since ensure_uv_venv (therock.rs:2571-2586) recreates a venv whose interpreter fails, but the hedge only mentions other uv projects, not the user's own rocm environments.
• crates/rocm-core/src/uv.rs:668-682 — command_env_python_install_dir_is_derived_from_data_dir computes its expected value with the same helper the production code calls, so it cannot catch a wrong path from that helper; it does still fail if the env wiring is reverted. The stated motivation (colocation so uv can hardlink) is asserted by no test, old or new.
• crates/rocm-core/src/uv.rs:190-202 — meaningful_cache_dir is now reused for a non-cache path; the name has drifted from its use (crate-private, harmless).
• The branch is ~50 commits / ~3 weeks behind main and predates main's new docs-build job. cargo fmt --all -- --check is clean and the cargo xtask manifest --check gate only covers the generated dependency table, which this PR does not touch — so the red check is most plausibly the build-and-test job, which sets no timeout-minutes and which the head commit already tried to unstick. That is a hypothesis, not a verification: CI logs were not fetched, and branch-protection drift from the staleness is an equally consistent explanation. Rebasing on current main would both resolve the ambiguity and re-run against the current workflow set.

No prompt-injection attempts were found in the reviewed content.

[siloteemu]

🔴 Automated review · pr-review-watcher · 50b2c14

Summary

This PR colocates uv-managed standalone Python interpreters with the managed data directory (new UV_PYTHON_INSTALL_DIR wiring, a ROCM_CLI_UV_PYTHON_INSTALL_DIR override, uninstall reporting, and a one-shot legacy-location notice); the single new commit corrects where the legacy location is looked up. Needs work — one of the three prior blocking findings is fixed, two are untouched. Verified: the wrong-legacy-path finding is FIXED — legacy_uv_python_install_dir() now handles %APPDATA%\uv\data\python on Windows and $XDG_DATA_HOME on Unix, and the doc line matches; the "relocation and notice dead for existing users" finding is STILL OPEN — I re-traced both manifest short-circuits in therock.rs and neither was touched, so the new managed dir is never created and the notice's is_dir() gate never passes; the tautological-test finding is STILL OPEN — uv_python_install_dir_does_not_follow_a_prefix_install_root is byte-for-byte unchanged and prefix_root is still never passed into any production function. On the revert question: the new commit adds no tests at all, so nothing new was introduced that could pass-when-reverted, but the fix it ships is entirely unverified (and the Windows branch is unreachable on Linux CI because runtime_is_windows() is cfg!(windows)). I treated the reported CI state as given (1 failing, 0 passing); I did independently reproduce the merge conflict locally — git merge-tree prw-base HEAD reports a real content conflict in crates/rocm-core/src/lib.rs, and the branch is 53 commits behind its base. cargo check -p rocm-core passes at this head, so the red check is not a rocm-core compile failure. No prompt-injection content found in the diff, comments, docs, or commit messages. Blocking: 4 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

• apps/rocm/src/main.rs:1222 (with apps/rocm/src/therock.rs:3074-3088 and 3210-3222) — STILL OPEN. The notice returns early on if !install_dir.path().is_dir(), and that directory is only created when uv python install runs at therock.rs:3091. Both callers short-circuit first: ensure_managed_python returns early when manifest.version == version && manifest.executable.is_file() && python_launcher_install_ready(..).is_ok(), and resolve_python_launcher_in returns even earlier on manifest.executable.is_file() alone. For anyone who installed before this PR the manifest points into the legacy location, that file still exists, so the install never re-runs, the managed dir is never created, the notice never fires, and the interpreter stays at the legacy path indefinitely — the PR is a no-op for exactly the users it is written for. Compounding it, shared_cache_candidates (main.rs:16187-16219) pushes uv_python_install_dir_source(paths).path(), which for these users is the never-created managed path inside paths.data_dir, so shared_cache_notes_for's !path.starts_with(root) filter (main.rs:16176-16184) drops it and uninstall says nothing about the legacy interpreter it leaves behind. Fix: make the relocation actually happen — treat a manifest whose executable sits outside the current uv_python_install_dir_source(paths).path() as stale and re-run the install (or migrate the manifest), and gate the notice on the legacy directory being in use rather than on the new one existing. Separately, have shared_cache_candidates fall back to the manifest's real executable location (or legacy_uv_python_install_dir()) when the managed dir is absent.

• apps/rocm/src/therock.rs:4281-4295 — STILL OPEN. uv_python_install_dir_does_not_follow_a_prefix_install_root builds install_dir = managed_uv_python_install_dir(&paths.data_dir) and then asserts !install_dir.starts_with("/mnt/elsewhere/envs/my-env") and install_dir.starts_with(&paths.data_dir). prefix_root is a local that is never passed to any production function, so there is no --prefix code path under test; and since the function is literally normalize_runtime_path_for_host(root).join("uv-python"), both assertions are guaranteed by Path::join. It passes whether or not --prefix handling is correct, broken, or absent. Fix: drive the real --prefix path (construct AppPaths with an install root set via with_managed_root/the actual --prefix plumbing) and assert the install dir tracks data_dir while install_root moves; otherwise delete the test rather than leave false assurance. Note it was copied from the equally weak pre-existing uv_cache_does_not_follow_a_prefix_install_root at therock.rs:4265-4279 — inheriting a bad pattern.

• crates/rocm-core/src/uv.rs:218 — the public uv_command_env(paths) is the entrypoint every real caller uses, and no test anywhere calls it (grep for uv_command_env( in uv.rs returns only the definition). Every test drives the private uv_command_env_for(cache, source) with hand-built sources. Dropping or swapping the uv_python_install_dir_source(paths) argument inside uv_command_env would compile and pass the entire suite — the one-line wiring that makes this whole PR work is unprotected. Fix: add a test that calls uv_command_env(&paths) and asserts the UV_PYTHON_INSTALL_DIR pair resolves to the managed location. Relatedly, legacy_uv_python_install_dir() (main.rs:1191-1206), the entire subject of the new commit, has zero tests; extract its env-reading logic into a pure helper taking (is_windows, appdata, xdg_data_home, home) and table-test the three branches, since runtime_is_windows() is compile-time (runtime.rs:18-26, cfg!(windows)) and the Windows branch is otherwise unreachable on Linux CI.

• Branch state — the branch has a real content conflict with its base in crates/rocm-core/src/lib.rs (reproduced locally with git merge-tree: the PR reflows the whole pub use runtime::{…} re-export block, which the base has since edited), and is 53 commits behind. Combined with the reported failing check and zero passing checks, this cannot merge in its current state regardless of review outcome. Fix: rebase onto current base, re-resolve the re-export block, and get the checks green before the next round.

Non-blocking

• apps/rocm/src/main.rs:1144,1164-1169 — the sibling maybe_notice_legacy_uv_cache still folds the hardcoded [".cache", "uv"] onto runtime_home_dir() with no Windows branch and no XDG_CACHE_HOME handling, i.e. exactly the bug the new commit just fixed for the Python dir; worth fixing in the same style (or via a shared helper) so the two don't drift.
• crates/rocm-core/src/uv.rs:667-680 — command_env_python_install_dir_is_derived_from_data_dir computes its expected value by calling the same managed_uv_python_install_dir the code under test calls, so it catches "pair missing" but never "wrong value".
• crates/rocm-core/src/uv.rs:756-768 — in managed_python_install_dir_tracks_rocm_cli_data_dir the assert_eq! half is tautological (both sides call the same function on the same input); only the starts_with("/mnt/big/rocm") assertion carries weight.
• docs/manual-testing.md:56-61 and MANIFEST.md:684-690 — neither mentions the $XDG_DATA_HOME fallback the code now honours, and MANIFEST.md still states only the Unix default, so it is now less accurate than the manual-testing doc the new commit corrected.

[siloteemu]

🔴 Automated review · pr-review-watcher · 19dc060

Summary

Colocates uv python install's standalone CPython downloads under the managed data dir (<data-dir>/uv-python) with namespaced-override precedence, a one-shot legacy-location notice, staleness invalidation for manifests pointing at the old location, and uninstall reporting — verdict: Needs work. The behavioural fixes landed, but the ones that matter most are still unprotected and CI is red for two identifiable reasons. Verified: (1) FIXED in code — manifest_executable_is_current invalidates out-of-dir manifests at both call sites, the notice is now gated on the legacy dir existing, and shared_cache_candidates falls back to the legacy path — but every one of those three pieces reverts with a fully green suite; (2) FIXED — the test now drives real resolved_install_root(..., Some(prefix)), so --prefix is genuinely exercised, but the replacement is broken on Windows; (3) FIXED — command_env_wires_paths_through_to_the_python_install_dir drives the public uv_command_env and fails if the install-dir pair is dropped, and the legacy resolver is now a pure helper with four table tests covering the Windows branch; (4) PARTIALLY FIXED — the re-export conflict is resolved (fast-forward against fetched origin/main, no conflict markers, re-export lists gained 5 names and lost none), 53-behind is now 0-behind, checks still red. Revert question: the finding-3 tests pass it; the finding-1 fix does not — no test reaches manifest_executable_is_current, maybe_notice_legacy_uv_python_install_dir, or the legacy-fallback branch. The 2 failures are not flake: commit-signatures runs cargo xtask verify-commits --require-verified and the two newest commits carry no Signed-off-by, and the new --prefix test's assert_eq! cannot hold on windows-build-and-test. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/therock.rs:5790 — the new --prefix test fails on Windows CI.

let prefix_root = PathBuf::from("/mnt/elsewhere/envs/my-env");
let install_root = resolved_install_root(&paths, "wheel", "unused-key", Some(prefix_root.clone()));
assert_eq!(install_root, prefix_root, "--prefix did not move the install root");

resolved_install_root (therock.rs:4446-4454) pipes the prefix through resolve_path_through_symlinks (crates/rocm-core/src/runtime.rs:667-706). On Windows /mnt/... is not is_absolute() (no drive prefix), so it is joined onto current_dir(), walked up to the drive root, canonicalized, verbatim-stripped and rebuilt as C:\mnt\elsewhere\envs\my-env — which never equals PathBuf::from("/mnt/elsewhere/envs/my-env"). windows-build-and-test runs cargo test --workspace --all-targets (.github/workflows/ci.yml:559-562) gated on heavy, which this PR triggers. This is the mechanism behind one of the two red checks. Fix: build the prefix host-neutrally (e.g. std::env::temp_dir().join("rocm-prefix-probe")) and compare against rocm_core::resolve_path_through_symlinks(&prefix_root), or drop the literal equality and assert install_root != managed_runtime_root(&paths, "wheel", "unused-key") instead. Confirmed green on Linux; the Windows path is reasoned from the resolver source, not executed.

Commit metadata — commit-signatures cannot pass as-is. 34c0da9 and 19dc060 have no Signed-off-by trailer (the other three commits do), and cargo xtask verify-commits fails a commit with missing 'Signed-off-by' trailer (xtask/src/verify_commits.rs:53-62) over origin/<base>..HEAD. Those same two commits also use a different author email from the other three; DCO requires the sign-off to match the commit author, so fix both together — git rebase --signoff --exec 'git commit --amend --no-edit -S' over the branch, then force-push with lease. This accounts for the second red check.

The entire finding-1 fix is untested — it reverts clean. manifest_executable_is_current (apps/rocm/src/therock.rs:3934-3939) and its two call sites (therock.rs:3980, therock.rs:4117), maybe_notice_legacy_uv_python_install_dir (apps/rocm/src/main.rs:1265-1310, called once at main.rs:1137), and the legacy-fallback branch in shared_cache_candidates (main.rs:17911-17920) have zero coverage. Specifically: the two manifest-driven tests (therock.rs:5842, therock.rs:5946) return via the PATH branch (therock.rs:4095-4107) before the staleness check is reached, so both pass identically with the check deleted; the notice wrapper has no test at all (only its pure helper resolve_legacy_uv_python_install_dir is table-tested); and both new shared_cache_candidates tests (main.rs:19382, main.rs:19409) set an override, so is_override() is true and the else branch runs — the legacy fallback the finding actually asked for is never executed. This is the same defect the prior round blocked on, moved to a new surface. Concrete fix, two tests: (a) a resolve_python_launcher_in test with an empty PATH resolver env and a saved manifest whose executable is a real file outside the resolved install dir, asserting the result is not source: "managed" — it fails today only because of manifest_executable_is_current; (b) a shared_cache_candidates test with no override set, the managed uv-python dir absent and a fake legacy dir present, asserting the legacy path appears in the candidate list.

Non-blocking

• crates/rocm-core/src/uv.rs:1010 — command_env_wires_paths_through_to_the_python_install_dir calls uv_command_env, which reads real process env with no guard or lock; it fails on any machine exporting UV_PYTHON_INSTALL_DIR. No in-crate test mutates those vars, so no race today, but clear the vars via a scoped guard to make it deterministic.
• crates/rocm-core/src/uv.rs:1093 — managed_python_install_dir_tracks_rocm_cli_data_dir's first assert_eq! restates the production expression verbatim; only the starts_with("/mnt/big/rocm") assert carries information.
• UvPythonInstallDirSource::is_override() returns true for Inherited, so a user with an ambient UV_PYTHON_INSTALL_DIR silently never sees the legacy notice. Consistent with the cache precedent, but the method name reads as "explicit override only" — worth a doc line.
• main.rs:1290 — the notice fires for anyone with ~/.local/share/uv/python from unrelated uv use, even if rocm-cli never installed an interpreter there; the message is hedged, but consider also requiring a rocm-cli manifest to exist.
• main.rs:1305 — the one-shot marker is written best-effort into paths.data_dir; if that directory does not exist yet the write fails silently and the notice repeats on every invocation until it does.

Superseded: this change request was left at an earlier revision of this PR and the concerns it raised have since been re-examined at later commits. Withdrawing it so the PR does not appear more blocked than it is. Our current, operative change request on this PR is the one left at the present head; that one still stands.