fix(install): resolve driver repo version and package release in plans

[r0x0r]

Summary

rocm install driver --dry-run printed an unexpanded shell-style placeholder on
the repo_version: line, and the same unresolved template would have been
written into the apt sources list on Debian/Ubuntu:

repo_version: ${ROCM_CLI_AMDGPU_VERSION:-7.2.4}

The driver install plan stored the repo version and package release as raw shell
parameter-expansion templates (${ROCM_CLI_AMDGPU_VERSION:-7.2.4} and
${ROCM_CLI_AMDGPU_PACKAGE_RELEASE:-70204}). Two problems followed:

1. Cosmetic leak — the human-readable plan summary printed the version
   template verbatim on its repo_version: line, showing an unexpanded shell
   placeholder in user-facing dry-run output.
2. Install-breaking bug (a real behavior change to fix) — the Debian/Ubuntu
   apt source line wrapped the template in single quotes, so the shell never
   expanded it and the literal placeholder would have been written into
   /etc/apt/sources.list.d/amdgpu.list.

Fix

Resolve both templates once, at plan-build time in build_driver_install_plan,
to their effective values (the env var when set and non-empty — matching shell
:- default semantics — otherwise the documented default) and bake the concrete
values into every rendered URL, repo path, and command. The apt sources-list
line now interpolates in Rust before the string is embedded in the single-quoted
shell literal, so it no longer depends on an expansion that could never happen.

The resolver is generalized to resolve_shell_default_template and hardened to
pass bare ${VAR} and nested-default shapes through unchanged rather than
emitting a partially rewritten value.

Behavioral note

This is not a display-only change. A user who previews the plan and later
copy-pastes the printed commands into a different shell now gets the version
baked in from the previewing process's environment rather than resolved by the
pasting shell. That is more predictable than before, and it is what the apt fix
requires, but it is a genuine behavior change to the emitted commands.

Environment variables

The effective version/release are controlled by ROCM_CLI_AMDGPU_VERSION and
ROCM_CLI_AMDGPU_PACKAGE_RELEASE; both fall back to the documented defaults when
unset or empty.

Testing

• Unit tests for the resolver: default, env override, empty-as-unset,
  non-template pass-through, bare-var and nested-default pass-through.
• Every distro plan test updated to assert the concrete resolved values.
• A render-level regression test asserting the dry-run summary shows the
  resolved version rather than the raw placeholder.
• End-to-end scenario @id:install-driver-dry-run-resolves-repo-version
  (tests/e2e-cucumber/features/examine.feature, tagged @requires-os:linux).
  install driver --dry-run short-circuits before any mutation and prints
  repo_version: unconditionally, so the scenario runs on the no-GPU mock lane
  and exits 0 on any Linux host. The assertion is discriminating: the old
  ${ROCM_CLI_AMDGPU_VERSION:-7.2.4} output fails both the !contains("${")
  and the leading-ascii-digit checks.
• Tests that read these process env vars serialize on a shared lock and
  save/restore prior values so they stay deterministic under edition-2024 env
  semantics.

Gates

• cargo fmt --all -- --check — clean
• cargo clippy --workspace --all-targets -- -D warnings — clean
• cargo test -p rocm --bin rocm driver — 26 passed
• cargo xtask e2e -- -n "The driver install dry-run shows the effective repo version" — exit 0
• diff leak scan — clean

[volen-silo]

Review: changes requested

Scope: 1 file, +82/-1 — resolve_repo_version helper plus its use on the repo_version: line of render_driver_install_plan, and 5 new tests. Clippy (cargo clippy -p rocm --all-targets -- -D warnings) is clean, cargo test -p rocm --bin rocm driver_plan passes (20), and the new regression test genuinely fails before the fix and passes after — verified locally.

The helper itself is correct for the input it actually receives. The blocking items are about project conventions and the layer the fix sits at.

---

Blocking

1. User-observable output change with no scenario and no justification (AGENTS.md §3, checklist §13 item 5)

AGENTS.md §3 is explicit: if a change alters what a user of the CLI can observe — command output included — the behavior must be covered by a Gherkin scenario in tests/e2e-cucumber/features/, and "a unit test asserting the internal helper does NOT discharge this". If a scenario already covers it, the PR text must name the @id:; if the change is purely internal, the PR text must say why no scenario is needed.

This PR changes rocm install driver --dry-run stdout. I grepped tests/e2e-cucumber/features/ — nothing covers install driver or its dry-run output (the only dry-run hit is @id:fix-dry-run-changes-nothing in diagnose.feature, which is rocm diagnose fix, unrelated). The PR body cites only the Rust tests and doesn't address the requirement either way.

Minimum to unblock: add a scenario covering the dry-run summary line, or state in the PR body why one isn't warranted here.

2. New tests mutate process-global env without the lock this repo already established for exactly this

apps/rocm/src/main.rs:24039 and :24055 call std::env::set_var/remove_var under bare unsafe with no serialization. The justifying comments — "A made-up variable name that nothing else sets keeps this race-free" (:24034) and "Uses a unique per-test variable name so it cannot race" (:24041-24042) — address a name collision, which was never the hazard. set_var became unsafe in edition 2024 because concurrent env mutation races with any concurrent env::var read anywhere in the process, regardless of variable name, and cargo test runs these functions multi-threaded in one binary. So the comments assert a safety property the code doesn't have, which is worse than no comment.

Concretely reachable: the new driver_plan_dry_run_repo_version_line_is_resolved (:24069) calls resolve_repo_version → env::var, and can run concurrently with the two mutating tests in the same binary.

This codebase already solved this twice, and neither pattern was used:

• ScopedEnvVar (RAII save/restore) at apps/rocm/src/main.rs:16579-16605, serialized via BUILTIN_ENGINE_ENV_LOCK in with_scoped_builtin_engine_env
• PROCESS_ENV_TEST_LOCK: Mutex<()> at apps/rocm/src/therock.rs:3571, held at :4060, :4316, :4367, :4420, with the comment "env is shared across all test threads"

Fix is small: add a module-level mutex in the main.rs test module (or reuse ScopedEnvVar), hold it in all three tests that touch env — including the render-level regression test — and correct the misleading comments.

2b. Related, same fix: driver_plan_dry_run_repo_version_line_is_resolved (:24069) asserts repo_version: 7.2.4 while reading the real ROCM_CLI_AMDGPU_VERSION. It fails for any developer or runner that legitimately exports that override. Save/clear/restore it under the same guard.

3. Resolving at render time is the wrong layer, and on apt hosts it makes the summary less truthful, not more

render_driver_install_plan receives a &DriverInstallPlan whose repo_version_expr is the single source of truth for the render. Reading ambient process env inside the renderer (main.rs:3472) means the same plan can render differently depending on when it's rendered. build_driver_install_plan (main.rs:2903) already owns that expression and is the natural place to resolve it.

That's a style point on its own. What makes it blocking is what it collides with:

The apt/Debian command at apps/rocm/src/main.rs:3162-3165 embeds the template inside a single-quoted string:

printf '%s\n' 'deb [...] https://repo.radeon.com/graphics/${ROCM_CLI_AMDGPU_VERSION:-7.2.4}/ubuntu <codename> main' | sudo tee /etc/apt/sources.list.d/amdgpu.list >/dev/null

POSIX single quotes suppress all expansion. Verified empirically — the literal ${ROCM_CLI_AMDGPU_VERSION:-7.2.4} is what printf emits, so that's what lands in the sources list. The dnf/zypper paths embed the same template unquoted (sudo dnf install -y <url>), so those do expand correctly; apt is the odd one out.

This is pre-existing, not introduced here — but it means that for Ubuntu 22.04/24.04, Debian 12/13, and ID_LIKE-matched derivatives (build_driver_install_plan, main.rs:2941-2966, likely the largest user segment), the new repo_version: 7.2.4 line now asserts an effective version that the executed command never uses. Before this PR the raw placeholder on the summary line at least mirrored what actually got written; now the summary and the command disagree.

Two acceptable resolutions, both fine by me:

• Preferred: resolve once in build_driver_install_plan and interpolate the concrete value into the summary and all three command builders (main.rs:3163, amdgpu_install_rpm_url :3427, amdgpu_install_sles_rpm_url :3433), dropping shell-template expansion entirely. This fixes the apt bug in the same change. Cost is ~12 existing tests that assert the raw template verbatim — mechanical.
• Or: keep this PR display-only, but open an issue for the apt single-quoting bug and reference it here, so the mismatch is tracked rather than papered over.

Worth stating plainly either way: the apt driver install path looks broken today independent of any env override, because a URL containing a literal ${...} goes into /etc/apt/sources.list.d/amdgpu.list and apt-get update will choke on it.

---

Non-blocking

• Helper name is narrower than its behavior. resolve_repo_version (main.rs:3465) is a generic ${VAR:-default} resolver per its own doc comment. A second identical template (${ROCM_CLI_AMDGPU_PACKAGE_RELEASE:-70204}) exists in the same plan and isn't routed through it. Something like resolve_shell_default_template invites the reuse; or the question goes away entirely if resolution moves to the build layer per blocking item 3.
• Nested defaults degrade badly rather than falling back. split_once(":-") matches the first occurrence, so ${A:-${B:-x}} with A unset returns the still-unresolved literal ${B:-x} instead of returning the input unchanged. ${VAR-default}, ${VAR:=x}, and bare ${VAR} all fall through safely. Not reachable from the one call site today; a guard (bail if the default contains ${) or a note would keep it that way.
• One line resolved, its neighbours not. execution_commands: still shows raw ${ROCM_CLI_AMDGPU_VERSION:-...} and ${ROCM_CLI_AMDGPU_PACKAGE_RELEASE:-70204} a few lines below the now-resolved repo_version:. Defensible — that section is literal shell source — but the reader sees both forms in one output. If display-only is the chosen direction, repo_version: 7.2.4 (${ROCM_CLI_AMDGPU_VERSION:-7.2.4}) would be honest about both.
• Both override env vars are undocumented. ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE appear nowhere in docs/, README.md, or skills/. Now that the dry-run summary shows a concrete resolved version, a user is likely to ask how to change it.
• No test for the malformed/embedded-template shapes. No coverage for ${VAR} (no :-) or a template embedded in a larger string. Both are unreachable from the current call site, so low value — mentioning only because the helper is written to be general.

---

Tradeoff worth noting

Fixing display only keeps this PR cosmetic and low-risk, at the cost of leaving the display/execution divergence in place for apt hosts. Resolving eagerly in Rust turns it into a behavior change (bigger blast radius, ~12 test updates) but closes the gap. Either is a reasonable call — it just shouldn't be made implicitly.

---

Positive signals

• Root cause in the PR body and commit message is precise and correctly identifies why the template exists rather than just deleting it.
• The regression test is at the render level, not just the helper — it asserts both the presence of the resolved value and the absence of the raw placeholder, so it can't pass vacuously.
• Empty-string-as-unset is handled, correctly matching :- rather than - semantics.
• Scoped to one logical change, DCO sign-off present, no AI footers.

[r0x0r]

Thanks for the thorough review, @volen-silo. I went with Option A and pushed a rebased, amended commit. Summary of what changed:

Blocking 3 — render layer was the wrong place + the apt single-quote bug.
Resolution now happens once at plan-build time in build_driver_install_plan, not at render time. Both the version and the package release are resolved there, and the concrete values are baked into every rendered URL, repo path, and shell command. This also fixes the Debian/Ubuntu apt source line, which wrapped the template in single quotes so the shell never expanded it — the literal ${ROCM_CLI_AMDGPU_VERSION:-7.2.4} would have been written into /etc/apt/sources.list.d/amdgpu.list. The repo_version: summary line now just prints the already-resolved plan.repo_version. I updated every distro plan test to assert the concrete values.

Blocking 2 / 2b — unsafe env mutation + regression test reading the real env var.
Added a ScopedTestEnv RAII helper in the test module: it holds one process-wide lock for the duration of any test that reads or mutates these env vars, and saves/restores prior values on drop. Every driver_plan_* test and the resolver tests now take the guard, and the version-asserting ones clear ROCM_CLI_AMDGPU_VERSION/ROCM_CLI_AMDGPU_PACKAGE_RELEASE so a value exported in the dev's or runner's shell can't leak in. The doc comment explains the real hazard (a mutation racing a concurrent env::var read anywhere in the process, not a name collision).

Blocking 1 — no scenario for the dry-run stdout change.
Added @id:install-driver-dry-run-resolves-repo-version in examine.feature (tagged @requires-os:linux) with steps in examine_steps.rs. install driver --dry-run renders the plan and returns before touching the system, so it runs on the no-GPU mock lane; the scenario asserts the repo_version: line is a concrete version with no ${ placeholder.

Non-blocking, also addressed.

• Helper renamed resolve_repo_version → resolve_shell_default_template since it's a general ${VAR:-default} resolver, not version-specific.
• Hardened it to pass bare ${VAR} and nested-default (${A:-${B:-x}}) shapes through unchanged instead of emitting a partially rewritten value, with tests for both.
• Resolved package_release in the same place so the one-line-resolved neighbours are consistent.

Verification (local, macOS): cargo test -p rocm --bin rocm driver_plan (20 pass), ... resolve_shell (6 pass), cargo test -p e2e-cucumber --no-run compiles. Clippy's authoritative gate is Linux CI — locally it's blocked by pre-existing macOS-only missing_const_for_fn noise in rocm-core and my temporary macOS build shim (reverted before commit), neither of which touches this change. Watching CI to green.

[volen-silo]

Follow-up review — re-verified against the current head efca8890

The branch was rebased and the commit amended since my review (that review was against the previous head; the whole PR is now one commit on top of 4512ce7b). Scope grew from +82/-1 in one file to +308/-77 across apps/rocm/src/main.rs, tests/e2e-cucumber/features/examine.feature, and tests/e2e-cucumber/tests/e2e/examine_steps.rs. You took Option A. I re-checked every finding against the code at this head rather than against the reply.

Blocking 1 — no scenario for the user-observable output change: resolved

@id:install-driver-dry-run-resolves-repo-version is real and it works, not just present:

• the step definitions bind to helpers that actually exist — field_value is defined in the same file, run_rocm matches its (String, String, i32) signature, and E2eWorld has both cli_output and cli_rc
• @requires-os:linux is a live tag honoured by the harness (tests/e2e-cucumber/src/expectation.rs), not an invented one, and the @id: is unique across all feature files
• the scenario runs on the blocking no-GPU mock lane: install driver --dry-run short-circuits in install_driver before any mutation and returns Ok, so it exits 0 on any Linux host including an unsupported distro, and repo_version: is printed unconditionally by render_driver_install_plan
• the assertion is not vacuous — the old output ${ROCM_CLI_AMDGPU_VERSION:-7.2.4} fails both the !contains("${") check and the leading-ascii-digit check

Ran it locally: cargo xtask e2e -- -n "The driver install dry-run shows the effective repo version" — exit 0, 1 scenario passed. The E2E tests CI job is green too.

Blocking 2 — unsafe env mutation without serialization: resolved in substance, two stragglers left

ScopedTestEnv is correct. The static LOCK inside new() is a single process-wide instance; Mutex resolves via the module-level import; save() captures the original value exactly once per key so a set followed by a clear can't overwrite the true original; Drop restores in reverse. unsafe_code is deny (not forbid) at the workspace level, so the per-fn #[allow] is legitimate. And the doc comment now states the actual hazard — a mutation racing a concurrent read, not a name collision — which was the part I cared most about.

The leftover: this PR turned ~22 previously env-free build_driver_install_plan call sites in tests into env readers, because the resolution moved into that function. 20 of them took the guard. Two did not:

• windows_install_driver_is_validate_only (apps/rocm/src/main.rs:24541)
• wsl_install_driver_uses_rocdxg_guidance_without_dkms (apps/rocm/src/main.rs:24557)

Both call build_driver_install_plan, which calls resolve_shell_default_template → std::env::var unconditionally at the top of the function, before the Windows/WSL branches. So they read env, unguarded, concurrently with the guarded mutators in the same binary — the exact race the guard exists to prevent, on the two call sites the sweep missed.

I'm not calling this blocking on its own: the mutating side is now serialized, which is the side edition 2024 made unsafe, and this binary already has plenty of unguarded env readers predating this PR (three disjoint env locks live in it — the new one, BUILTIN_ENGINE_ENV_LOCK, and PROCESS_ENV_TEST_LOCK in therock.rs, so full closure was never on the table here). But it's a two-line fix and the PR itself introduced those two readers, so I'd like it in this change rather than left as debris.

Blocking 2b — regression test reading the real override: resolved

driver_plan_dry_run_repo_version_line_is_resolved now takes with_amd_overrides_cleared(). Verified by running with the override actually exported:

ROCM_CLI_AMDGPU_VERSION=9.9.9 ROCM_CLI_AMDGPU_PACKAGE_RELEASE=99999 \
  cargo test -p rocm --bin rocm driver_plan

exit 0, 20 passed.

Blocking 3 — wrong layer, and the apt single-quote bug: resolved

Option A implemented properly, and I could not find a hole in it:

• both templates resolve once at the top of build_driver_install_plan; every command builder (apt_driver_plan, dnf_driver_plan, sles_driver_plan, amdgpu_install_rpm_url, amdgpu_install_sles_rpm_url, driver_plan_via_id_like) now takes concrete &str/String values
• repo-wide grep for ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE returns only the two resolution call sites, the test-isolation helper, one test assertion, and a feature-file comment describing the old behaviour. No generated command string still carries a ${...}
• the apt sources.list line now interpolates in Rust before the string is embedded in the single-quoted shell literal, so it no longer depends on an expansion that could never happen. That was a genuine install-breaking bug for Ubuntu/Debian and derivatives, and it's fixed here
• the $(uname -r) occurrences are all inside double quotes and still expand at execution time — no parallel bug introduced
• the amdgpu_install_rpm_url refactor renamed the local repo_version (the path segment) to repo_version_path while the parameter took the repo_version name. I checked slot by slot: no path segment was swapped, the URL is semantically identical modulo resolution. Same for the SLES variant
• no execution-time gap: DriverInstallPlan derives no Serialize, isn't persisted, and the commands run in the same process under the same env the resolution saw
• repo_version_expr is fully gone from the repo; the only external consumer is the e2e step, which parses the rendered text line, not a struct field or JSON key

One behavioural nuance worth stating out loud, inherent to Option A rather than a defect: a user who previews the plan and copy-pastes the printed commands into a different shell later now gets the version baked in from the previewing process's env, not from the pasting shell. That's more predictable than before, and it's what the apt fix requires, but it is a change.

Non-blocking items from the original review

Addressed: helper renamed to resolve_shell_default_template; bare ${VAR} and nested-default shapes now pass through unchanged with tests for both; the "one line resolved, its neighbours not" inconsistency is moot now that everything resolves.

Still open, still non-blocking:

• ROCM_CLI_AMDGPU_VERSION and ROCM_CLI_AMDGPU_PACKAGE_RELEASE remain undocumented in docs/, README.md, and skills/. More relevant now that they're the only lever on the version.
• The helper's doc comment overclaims slightly. It says anything other than a flat ${VAR:-default} "is returned unchanged", but strip_prefix("${") / strip_suffix('}') only inspect the first and last characters, so multi-brace inputs slip through and get mis-parsed rather than returned: ${A}${B:-x} splits into var = "A}${B", default = "x" and yields x; ${VAR:-a}} folds the trailing brace into the default. Unreachable from either call site — both pass fixed literals — so this is a docstring/precision nit, not a bug. Either tighten the guard (bail if the stripped inner still contains ${ or }) or soften the sentence.
• Related: now that both callers pass compile-time literals, the generic parser arguably doesn't earn its keep versus two direct env::var(...).ok().filter(...).unwrap_or(...) lookups. Genuinely a matter of taste — mentioning it, not asking for it.

New — the PR description is now stale, and misleading

This is the one thing I'd like fixed before merge, and it's a text edit. The body still describes the abandoned display-only approach:

• "Resolve the template to its effective value for display only" — no longer true
• "The executable commands still carry the literal template, so the runtime shell resolves it as before — only the summary line changes" — the opposite of what the code now does
• "This is a cosmetic/clarity change to dry-run output; no behavior change to the executed commands" — actively wrong, and it buries the best part of this PR: the apt sources-list fix is a real behaviour change that fixes a broken install path
• the listed test names (resolve_repo_version_*) no longer exist
• the new scenario isn't named. AGENTS.md §3 requires the PR text to say which @id: covers the behaviour; that's in your comment but not in the body, and the body is the durable artifact
• the title understates the change for the same reason

The commit message, by contrast, is accurate and well written — it already says most of what the body should. Lifting it into the description would be enough.

Gates

Run at this head:

• cargo fmt --all -- --check — exit 0
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 101, one failure: providers::tests::local_provider_prefers_lemonade_over_stale_builtin_services. Not this PR. It passes in isolation, and I reproduced the same failure on the merge base 4512ce7b (1 of 3 full-suite runs there), so it's a pre-existing flake in a test that spawns local HTTP servers and is load-sensitive.
• cargo xtask e2e -- -n "The driver install dry-run shows the effective repo version" — exit 0
• diff leak scan — clean
• CI is green apart from the Read the Docs build, which is failing identically on every other open PR (infrastructure, unrelated)

Verdict

All three original blocking findings are resolved, and I verified each against the code rather than taking the reply at face value. Blocking 3 in particular came out better than I asked for — resolving package_release alongside the version, and hardening the helper, weren't required.

Two things left, neither of them deep:

1. Update the PR description — it currently contradicts the diff (must-fix).
2. Add the guard to windows_install_driver_is_validate_only and wsl_install_driver_uses_rocdxg_guidance_without_dkms (two lines, would-like).

With those, LGTM.

[r0x0r]

Both remaining items are done.

• PR description (must-fix): rewritten from the commit message so it matches the diff. It now leads with the two real problems — the cosmetic repo_version: leak and the install-breaking apt sources-list bug — and explicitly states this is a behavior change, not display-only. It names the scenario (@id:install-driver-dry-run-resolves-repo-version), drops the stale resolve_repo_version_* test names, and the title is updated to match the accurate commit subject.
• Env guards (would-like): windows_install_driver_is_validate_only and wsl_install_driver_uses_rocdxg_guidance_without_dkms now take ScopedTestEnv::with_amd_overrides_cleared(), matching the other call sites. cargo test -p rocm --bin rocm driver — 26 passed.

Non-blocking, deferred and acknowledged: documenting ROCM_CLI_AMDGPU_VERSION / ROCM_CLI_AMDGPU_PACKAGE_RELEASE in docs//README/skills, and tightening the resolve_shell_default_template docstring re multi-brace inputs (unreachable from either call site today). I'd rather land those as a small dedicated follow-up than widen this PR.

Note on history: the fix commit was signed-off but not cryptographically signed, so it was failing the commit-signatures gate and blocking the push hook. I re-signed the range and force-pushed with lease; git range-diff confirms the fix commit is byte-identical (signature only) and the guard change is the one new commit on top. verify-commits, clippy, fmt and test all pass at the new head.