feat(diagnose): report a code object manager that is not the runtime's own

[volen-silo]

Stacked on #384 — based on feat/report-comgr-copies, so this diff is just the one commit. Will be rebased onto main once #384 lands.

What

#384 made every copy of libamd_comgr visible. This adds the diagnosis: the catalog now reports when the copy that would load belongs to a different installation than the HIP runtime that would load — the state in which device code compilation fails with an error naming neither the library nor the second copy.

Why the rule is symmetric

The design note for this entry proposed a special case: "when the active HIP runtime is the managed runtime, the matching copy is the wheel copy. Without this rule the entry fires on every healthy CLI installation."

That is a patch over a rule stated asymmetrically. Asking one question of both libraries instead — does the libamd_comgr that would load come from the same installation as the libamdhip64 that would load? — makes every case fall out of the rule rather than being legislated:

Situation	Owning installs	Fires?
Healthy system-only install	both /opt/rocm	No
Healthy managed install	both the managed runtime	No — no special case needed
Wheel lib dir on the search path, runtime from /opt/rocm	differ	Yes
Two copies, the right one wins	same	No, and both stay listed
One copy, or none	nothing to differ	No

A control that has to be written as an exception is a rule that has not been stated correctly yet.

The detail it all turns on

A managed runtime spreads its libraries across separate _rocm_sdk_* packages that sit beside each other under one site-packages, not nested under the runtime's own root (_rocm_sdk_devel, when the devel extra is installed) at all. Ownership is therefore decided by looking a found file's directory up against the same directories install_library_dirs already walks for each root — never a string-prefix match against the bare root, which attributed nothing on exactly this shape and never by walking up from the file either, which would call each package its own installation and fire on the most common install we ship.

That is not a theoretical concern — an earlier version of this PR used a string-prefix match, which left every managed runtime installed with the devel extra unattributed (comgr_matches_runtime: null instead of true on a healthy install), and the two unit tests meant to catch it were built from a venv-shaped fixture production never produces, so they passed anyway. See "Review remediation" below.

Two further guards on firing

• An unattributed copy is not a mismatch. It means the search found a library no known installation claims — a gap in what the probe knows, not a finding about the user's machine.
• The runtime must actually ship a copy of its own, or there is nothing to switch to and the advice would be empty.

It advises and ranks nothing

Print-only. Removing a stack and reordering the search path can each break a working Python environment, and which is right depends on which stack the user means to keep — a question only they can answer. The finding also states that it describes the environment outside the managed runtimes: rocm serve puts a managed runtime's libraries first on purpose and gets a different, correct answer, and a report that did not name the environment it examined would read as a claim about one it never looked at.

Verification

• cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo clippy -p e2e-cucumber --test e2e -- -D warnings (the harness = false target --all-targets skips) — all clean
• cargo test --workspace --all-targets — clean, run twice after the review round below to rule out the one unrelated parallel-execution flake (restarting_a_keyless_public_service_names_the_public_bind_not_the_key_flag, a filesystem path race under full-suite parallelism, confirmed to pass in isolation) as masking a real failure
• GPU e2e lanes run in CI, not locally; see "CI" below

Every new assertion was verified to fail, not just to pass, including the ones added in the review round below — each was confirmed red against a deliberately broken version of the thing it covers, then restored.

CI

The scenario this PR's own GPU coverage depends on, examine-finds-the-managed-runtimes-own-compilation-library, is examine-19 in the current examine.feature (renumbered again by #384's own rebase onto a newer main; earlier revisions of this PR body called it examine-17, which is now a different, unrelated scenario). It was introduced in #384, not here — along with the AppPaths::discover data-dir fix and the @requires-os:linux tag that make it pass on every GPU lane including native Windows's absence. Nothing in this PR's own commit touches that scenario or its fixes; it only rebases on top of them.

Review remediation

A human review found a real gap in this PR's own commit, beyond the mechanical renumbering above: see "The detail it all turns on" for the attribution bug on managed runtimes installed with the devel extra, and its fix. Also fixed in this round, all in this PR's own commit:

• Two doc-comment splices (a missing blank /// line left one function's docs attached to the next item instead): check_18_comgr_conflict had absorbed check_17's doc paragraph, leaving check_17 undocumented; known_install_roots was undocumented the same way, under dedup_roots_keeping_first's docs.
• docs/wsl.md said fix-19-shm-too-small was the only non-WSL entry that applies on WSL; registering check_18_comgr_conflict for ["linux", "wsl"] made that stale. Updated to name both.
• The "Neither option is recommended..." sentence was stated independently (and already slightly differently worded) in fix.rs's static recipe and diagnose.rs's dynamically-generated one, and only the fix.rs copy was pinned by an e2e assertion. Both now read a shared fix::COMGR_CONFLICT_NEITHER_OPTION_RECOMMENDED constant, so they cannot drift apart.
• find_library_copies also picked up the active-managed-runtime search-priority fix feat(examine): report every code object manager library, not just the first #384's own review round added to the comgr-only search (generalized here to cover HIP too), and the loader cache / directory walk are now gathered once per rocm examine/rocm diagnose invocation and shared between the comgr and HIP searches rather than duplicated.
• Two earlier review comments (fix.rs's option (b) wording, and comgr_matches_runtime's parity with check_18_comgr_conflict) were checked against the current code and found already correct — not re-fixed, just verified.

Full detail and verification is in the commit message's second Review: paragraph.

Coverage boundary

The suite cannot install a second ROCm stack, so e2e cannot provoke a real conflict. It pins the half that matters if the entry ever stops being advisory — asking for the fix changes nothing and recommends neither option. The detection rule is proven by unit tests that construct the machine state directly, including the two cases that must stay silent.

Risk

Medium, concentrated in one place: a false report on a healthy managed installation. That is the risk the ticket names, and the symmetric rule plus root-based attribution are the two things standing against it, each with a test that fails when broken.

Follow-up

Overlaps #382, which replaces auto_applicable with a per-platform class. This branch is off #384, so the new recipe uses the current bool; whichever lands second rebases. If #382 lands first this entry becomes PRINT-ONLY in the new class directly.

[jussielo-amd]

This is a solid feature (surfacing comgr/HIP runtime mismatches), but I found a few issues that affect correctness of the diagnosis output and the fix advice text, plus a couple of secondary concerns. Left inline comments on the ones anchored to this diff; one test-coverage gap below since it points at a file this PR doesn't touch.

Untested field, tests/e2e-cucumber/tests/e2e/examine_steps.rs (not modified by this PR): the e2e assertion for comgr_paths entries still only requires ["path", "real_path", "version", "source"] and wasn't extended to require the new install_root field this PR adds to the wire format, and no e2e step checks hip_paths/hip_selected either. Right now only in-process unit tests exercise install_root — the actual CLI --json output for the field this design turns on is unverified end-to-end.

Requesting changes mainly for the fix-18 advice/data mismatch (correctness bug in user-facing guidance) and the comgr_matches_runtime vs diagnose disagreement — both are logic errors, not style nits.

[jussielo-amd]

Option (b) here says "Keep the wheel: order the search path so the runtime's own copy is found first", followed by an LD_LIBRARY_PATH command — but per this PR's own conflict definition (diagnose.rs's matching), "the runtime's own copy" is the copy belonging to the currently-active HIP runtime, which in the canonical wheel/system conflict is the system install, not the wheel. A user who wants to keep the wheel and follows this advice literally will export LD_LIBRARY_PATH to the system comgr directory instead — the opposite of the stated intent — and there's no obvious symptom afterward besides now picking up the system's libraries. Worth double-checking which path this recipe actually resolves to for option (b) and fixing the wording/command so it points at the wheel's own comgr, not the runtime's.

[jussielo-amd]

The documented --json field comgr_matches_runtime is computed with a weaker rule (comgr.install_root != hip.install_root, both non-empty) than check_18_comgr_conflict, which additionally requires the active runtime to ship a matching comgr copy of its own before firing. When the runtime ships no comgr copy of its own — the exact case this PR's own test a_runtime_with_no_copy_of_its_own_is_not_a_conflict covers — rocm examine --json will report comgr_matches_runtime: false while rocm diagnose reports nothing at all. Anyone consuming this field programmatically (or a user comparing the two commands) sees a contradiction. Suggest computing this field with the same logic check_18_comgr_conflict uses.

[jussielo-amd]

install_library_dirs now calls collect_sdk_library_paths (which also scans root/bin and root/lib/rocm_sysdeps/lib) unconditionally for every root, whereas the code this replaces (comgr_search_dirs) only added root/lib/root/lib64 for rocm-install-sourced roots. This silently widens what's probed for plain bare-metal /opt/rocm installs — e.g. a vendored dependency under lib/rocm_sysdeps/lib or bin that happens to match the comgr/hip library name pattern would now be picked up as an extra copy that never showed up before this PR, changing comgr_selected/comgr_paths ordering and copy counts on existing installs. This behavior change to an established field isn't called out in the PR description — was the widened scan for bare-metal roots intentional?

[jussielo-amd]

find_library_copies recomputes install_library_dirs(roots) and re-invokes paths_in_loader_cache (which spawns ldconfig -p) once per prefix, so both now run twice per rocm diagnose/rocm examine call — once for comgr, once for hip — even though neither depends on which prefix is being searched. That's an extra ldconfig -p subprocess plus a second full walk of every managed runtime's lib/lib64 → pythonX/site-packages → _rocm_sdk_* directories on every invocation of a command that's typically run repeatedly while debugging. Might be worth hoisting the roots/loader-cache computation out so it's shared between the two calls.

[jussielo-amd]

known_install_roots dedups with Vec::dedup_by, which only removes adjacent duplicates. e.rocm_path is pushed first and the sorted /opt/* siblings are appended after, so if e.rocm_path equals one of those sorted entries but isn't adjacent to it in the final vector (e.g. e.rocm_path is /opt/rocm-6.4 and /opt also contains rocm and rocm-6.4, sorting to [rocm, rocm-6.4]), the roots vector ends up [rocm-6.4, rocm, rocm-6.4, ...] and the two rocm-6.4 entries survive the dedup. install_library_dirs/find_library_copies then scan that install's directories twice per probe — masked from the final output only because record_library_copy separately dedups by resolved real path downstream. Sorting the full vector (or using a HashSet) before dedup would close this.

Addressed

[r0x0r]

What the change does. Teaches examine/diagnose to notice that the libamd_comgr copy the loader would pick belongs to a different installation than the libamdhip64 runtime, reports the HIP copies alongside the comgr ones, and adds fix-18-comgr-conflict as an advisory-only entry that states both remedies without ranking them.

Coverage. Full read of crates/rocm-core/src/{examine,diagnose,fix,lib}.rs and the four e2e-cucumber files at head, reviewed against the PR's own base (feat/report-comgr-copies), so #384's changes are excluded. Checked against AGENTS.md §3. Both findings from the earlier, now-dismissed review were re-tested specifically. No tests run.

Assessment: looks sound — no blocking findings survived verification.

The earlier review's two findings are addressed

• fix-18 advice/data mismatch — fixed. The machine-specific Fix in diagnose.rs derives its LD_LIBRARY_PATH directory from matching.real_path, where matching is the comgr copy whose install_root equals the HIP runtime's, so "order the search path so the runtime's own copy is found first" now describes what the command actually does. The <directory of the wheel's own copy> placeholder that remains in fix.rs is the generic catalog recipe, which has no live paths to fill in — correct for its role.

• comgr_matches_runtime vs diagnose disagreeing — fixed. probe_comgr and check_18_comgr_conflict now call the same examine::comgr_matches_runtime, and comgr_matches_runtime_never_disagrees_with_the_diagnosis pins that invariant across the catalog cases, including the one that used to diverge (a runtime shipping no comgr copy of its own). Making the disagreement structurally impossible, rather than fixing the two sites to agree, is the right shape of fix.

• The e2e gap is closed too. assert_comgr_copies_reported now requires install_root:

  for field in ["path", "real_path", "version", "source", "install_root"] {

  and two new steps assert hip_paths and hip_selected, wired into examine-16.

AGENTS.md §3 is satisfied: diagnose-21 (@id:diagnose-fix-comgr-conflict-is-advisory-only) covers the user-observable half, and its comment explains honestly why the conflict itself cannot be provoked in the suite — the detection rule is left to unit tests that build machine state directly. That is the explanation §3 asks for rather than a silent omission.

Non-blocking

• managed_runtime_roots() runs twice per find_library_copies. known_install_roots (examine.rs:2170) calls it to build roots, then install_library_dirs (:2200) calls it again to build managed_library_paths, and probe_comgr runs the pair twice (comgr, then HIP) — four registry walks per examination. It is a deterministic filesystem read, so this is cost rather than correctness, but threading the first result through would remove both the cost and any question about the two views agreeing.
• comgr_version_from_file_name is now called for HIP paths too (record_library_copy). The soname parsing is correct for both; only the name still says comgr.
• assert_hip_selection_is_stated passes trivially on the mock lane — with hip_paths == [] it only asserts hip_selected is null, so the list/selection agreement is never exercised off a GPU lane. It inherits this from the comgr sibling rather than introducing it.
• assert_fix_is_advisory uses contains("print-only") || contains("will NOT run it"), so dropping either string still passes. assert_both_options_unranked covers the dangerous case, so this is slack rather than a hole.
• Untested: the ordering contract in find_library_copies (LD_LIBRARY_PATH beats loader cache beats install dirs) is the whole point of the function and has no unit test; neither does the partial-version evidence branch in diagnose.rs where exactly one side has a version string.

Tradeoffs

• comgr_matches_runtime returns None both for "no known installation claims this copy" and for "the runtime ships no comgr of its own". Serialized, a consumer cannot tell those apart from "not computed". The comments document the distinction; the wire format does not carry it.
• The symmetric rule ("do both come from the same installation?") stays quiet on a machine holding only the mismatched comgr, because there would be no alternative to point at. Deliberate and documented, but it does mean a genuinely broken single-stack install reports nothing.

Positive signals

• Driving the JSON field and the diagnostic from one function, then testing that they agree, removes a whole class of future drift.
• Longest-prefix attribution in owning_install_root handles managed runtimes spread across _rocm_sdk_* subdirectories without special cases, and dedup_roots_keeping_first has a test for the non-adjacent duplicate that a naive consecutive dedup would miss.
• auto_applicable: false, with both remedies stated and neither recommended, is the right posture for a fix where either choice can break a working Python environment.
• #[serde(default)] on every new field keeps older consumers reading the new output.

One thing outside the diff

This PR's base is #384, which currently has changes requested and 6 failing checks, so #385 cannot land ahead of it regardless of its own state. Its own checks are green at this head apart from one still running.

The merge decision is yours; this review is posted as a comment and files no approval or change request.

---

🤖 by agent-hub on AMD AgentHub

[juhovainio]

I reviewed this PR and found one real gap plus a handful of smaller things, left inline below.

The gap: on a managed runtime installed with the devel extra, the new check can never fire, and comgr_matches_runtime comes back null rather than true on a healthy install. This PR fixes the discovery half of that problem, install_library_dirs now prefers the probe's recorded library_paths over re-deriving the layout, but the attribution half still compares those paths against a root that is the _rocm_sdk_devel directory, which they are not under. The unit tests do not catch it because their fixtures are built from a venv root that production never supplies.

The rest are documentation and description accuracy: two doc comments have drifted onto the wrong functions, docs/wsl.md now contradicts the registration table, and the description's CI section describes work that moved to #384.

One concern I chased and dropped: the claim that the venv-layout fallback had disappeared from examine's search. It has not, library_paths has been in the probe script since the initial import.

All five items addressed: attribution fixed from prefix-match to an exact directory-owner map (dir_owners), with the regression fixture rebuilt from the production sibling-directory layout and confirmed by mutation testing (reverted to the old prefix match, watched the test go red, reverted back, confirmed green and no residual diff); both doc-comment splices fixed to sit on their own functions; docs/wsl.md updated so it no longer contradicts the fix-18 registration; PR description corrected for examine-19 naming and CI attribution to #384; and the fix.rs/diagnose.rs remedy text now shares one constant, structurally removing the drift the e2e pin could miss. Full verification bar is clean: cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo clippy -p e2e-cucumber --test e2e -- -D warnings, cargo test --workspace --all-targets (45 binaries, 0 failures), and cargo xtask e2e -- -n skill (6 scenarios, 22 steps, 0 failures).

[juhovainio]

Re-reviewed after the fixes for the last round — all 5 previously-flagged issues are genuinely fixed, including the owning_install_root attribution bug (this now does exact-directory attribution via dir_owners, not just discovery).

One new issue in the comgr/HIP generalization: probe_comgr documents the comgr and HIP searches as symmetric ("searched the same way, and for the same reason"), but the "N copies found; X would load" note is only emitted for comgr. The HIP branch only checks hip.is_empty() and never calls multi_copy_note for it, so a machine with multiple libamdhip64 copies gets no note, even though the equivalent comgr case does. Left as an inline comment below.

Addressed