amd-smi path resolution

[r0x0r]

rocm dash assumes that amd-smi is in path (for displaying GPU info), which it is not for default installation. This PR adds logic for amd-smi path resolution.

[Copilot]

Pull request overview

This PR improves GPU telemetry collection reliability by resolving the amd-smi executable path for default managed ROCm SDK installs (where amd-smi is not on PATH). It threads the resolved binary path into rocm dash and updates other call sites to prefer the resolved location.

Changes:

• Add rocm_core::resolve_amd_smi_binary() to locate amd-smi in the managed runtime registry and common home fallbacks.
• Allow AmdSmiCollector/dash daemon runner to use an explicit amd-smi binary path.
• Update rocm (TUI/dash) and rocmd to run amd-smi via the resolver instead of assuming it is on PATH.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
crates/rocm-dash-daemon/src/runner.rs	Adds RunnerOptions::amd_smi_binary and uses it to detect GPU telemetry via an explicit amd-smi path.
crates/rocm-dash-collectors/src/amd_smi.rs	Adds detect_with_binary so collectors can run amd-smi from a specified path.
crates/rocm-core/src/lib.rs	Implements resolve_amd_smi_binary() plus unit tests for home/registry resolution behavior.
apps/rocmd/src/lib.rs	Uses the resolver when spawning amd-smi for GPU snapshots.
apps/rocm/src/tui.rs	Uses the resolver when spawning amd-smi for TUI GPU telemetry.
apps/rocm/src/dash.rs	Passes the resolved amd-smi path into dash daemon runner options.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[michaelroy-amd]

Triage findings — bug is real, CI is shallow, approach is sound

I picked this up since the regression was originally ours. Confirmed the bug still exists on main: amd-smi is invoked by bare name (PATH lookup) in three places, and the managed TheRock runtime ships amd-smi inside the wheel's bin dir, not on PATH, so rocm dash GPU info silently fails on a default install:

• apps/rocm/src/tui.rs — run_amd_smi_json (ProcessCommand::new("amd-smi"))
• apps/rocmd/src/lib.rs — capture_amd_smi_json
• crates/rocm-dash-collectors/src/amd_smi.rs — AmdSmiCollector default binary

The PR patches all three plus the codex-bridge path, resolves the absolute path via rocm_core::resolve_amd_smi_binary, threads it through the existing RunnerOptions seam, and falls back to "amd-smi" as last resort (so behavior is unchanged when nothing is found). Good shape, and it reuses the existing managed_therock_environment_records / managed_sdk_tool_path helpers rather than reinventing them. 👍

The three CI failures (two aren't this PR's fault)

Check	Root cause	Fix
clippy	unnecessary Debug formatting at crates/rocm-dash-daemon/src/server.rs:71/75/87/91. These exact {:?} lints were already fixed on main by #27. This branch forked before #27/#28/#25 landed, so its clippy run still hits the old code.	Rebase on main — disappears entirely, nothing to change here.
fmt	The new tests in crates/rocm-core/src/lib.rs aren't rustfmt-clean (the temp_root / new_bin let wrapping).	cargo fmt
windows-build-and-test	resolve_amd_smi_binary_prefers_home_rocm_venv_path builds the expected path with a combined "rocm_venvs/default/bin" component (forward slashes), then assert_eq!s the OsString literally against the function's per-join result (backslashes on Windows). Passes on Linux, mismatches on Windows. Test-only bug, not the product.	Build the expected path with per-component .join() (or compare as Path), not a /-joined literal.

One product nit worth fixing while we're here

resolve_amd_smi_binary_in_registry goes through managed_sdk_tool_path, which correctly appends .exe/.cmd/.bat on Windows — but the home-dir fallback (resolve_amd_smi_binary_in_home) hardcodes plain "amd-smi", so on Windows it would miss amd-smi.exe/.bat. Suggest routing the home fallback through managed_sdk_tool_path too for consistency. Minor; doesn't block.

How this should land relative to #38 (the recent app.rs split / dash refactor)

I checked #38 against this PR — no real conflicts, and the design is already compatible with the post-#38 architecture:

• No file collisions. §1+§2: polish, harden, invariant re-proofs + ROADMAP update #38 does not touch tui.rs, amd_smi.rs, apps/rocmd, runner.rs, or the collectors. The only shared file is apps/rocm/src/dash.rs, and the edits are in disjoint regions — §1+§2: polish, harden, invariant re-proofs + ROADMAP update #38 extracts build_dashboard_runtime (~L190–235), this PR adds amd_smi_binary to runner_options (~L35–60). A rebase should apply cleanly with no manual merge.
• Seam/invariant respected. §1+§2: polish, harden, invariant re-proofs + ROADMAP update #38's invariants keep the dash crates decoupled from rocm-core (rocm-dash-daemon/rocm-dash-collectors have no rocm-core dependency). This PR honors that: resolution happens in the app layer (which may depend on rocm-core) and is passed down as a plain String via RunnerOptions.amd_smi_binary; AmdSmiCollector::detect_with_binary(impl Into<String>) never reaches into rocm-core. Please keep it that way on rebase — don't let the collector call resolve_amd_smi_binary directly, or it reintroduces the edge §1+§2: polish, harden, invariant re-proofs + ROADMAP update #38 removed.

Recommended landing order: merge after current main (≥ #25/#27/#28) and after the #38 stack; rebasing onto that main auto-clears the clippy failure, leaving just cargo fmt + the one-line Windows test fix (and optionally the home-fallback .exe consistency nit).

Checklist to green this up

• Rebase on main (clears clippy)
• cargo fmt (clears fmt)
• Fix the Windows test to compare paths component-wise, not as a /-joined OsString literal (clears windows)
• (optional) Route the home-dir fallback through managed_sdk_tool_path for Windows-suffix parity

Happy to push these fixes onto the branch (preserving authorship) if that's easier.

[r0x0r]

Thanks for the thorough triage @michaelroy-amd — all four checklist items are addressed in 5446190:

• Rebase on `main` — done; the `Fix clippy lints in rocm-dash-daemon #27` `{:?}` clippy lints are gone (clippy check now green).
• `cargo fmt` — done; fmt check green.
• Windows test — `resolve_amd_smi_binary_prefers_home_rocm_venv_path` now builds the expected path component-wise via per-`join`, so it matches the resolver on Windows; `windows-build-and-test` is green.
• Home-fallback `.exe` parity (optional nit) — done; `resolve_amd_smi_binary_in_home` now routes both the venv and `~/.rocm/bin` lookups through `managed_sdk_tool_path`, picking up `.exe`/`.cmd`/`.bat` on Windows.

The seam invariant is preserved: resolution stays in the app layer and is passed down as `RunnerOptions.amd_smi_binary`; the collector never calls `resolve_amd_smi_binary` directly. All CI checks are green.