feat(examine): report every code object manager library, not just the first

[volen-silo]

What

HIP compiles device code at run time through libamd_comgr, and a machine can hold more than one copy — a system ROCm install and a ROCm Python wheel each ship one. When the copy the loader picks does not belong to the active runtime, compilation fails with a general error naming neither the library nor the second copy.

This CLI installs the second copy itself. Its install path puts ROCm wheels into a managed environment, so a user who follows it on a host that already has system ROCm ends up holding both, having done nothing unusual and been warned about nothing.

Nothing looked past the first match, so the second copy was invisible. The inspection now records every copy in loader search order, which one would load, and its version.

Reporting only. The conflict rule and the catalog entry that states the user's options follow in a second PR. That is where the risk of a false report on a healthy managed install sits — every managed environment this CLI creates legitimately holds two copies — so it belongs with the rule it threatens rather than bundled with an uncontroversial probe.

Non-obvious decisions

No dlopen. The ticket specified reading the version via amd_comgr_get_version through dlopen. That runs the library's ELF initialisers — code from an unknown library, on a machine the tool was called to precisely because something is already wrong — and pulls a possibly conflicting HIP stack permanently into the process. The version is read from the versioned soname instead. A renamed file yields an empty version, which is honest; the conflict rule never needs the version, only the report text does.

Deduplicated on the resolved path. ROCm ships libamd_comgr.so.2.8.0 beside an unversioned symlink. Counting those as two copies would invent a conflict on an ordinary install — the false report that matters most, since it would fire on healthy machines.

It emulates the loader; it is not the loader. No account is taken of RUNPATH/RPATH, ld.so.preload, or a container remapping paths. The doc comment says so, and the list of copies is the evidence for the verdict rather than a guarantee.

It runs on WSL2 too — a deliberate departure from the ticket's "Linux" scope. The WSL2 early return exists for kernel driver questions, and the code says so while still running the framework probe. A shadowed comgr copy is a run-time compilation failure inside the framework, and ROCm on WSL2 is a supported configuration where the two copies collide identically. Skipping it would leave a WSL user unable to see a conflict that is really there.

Four existing symbols, plus two struct fields, were made pub(crate) rather than restating where a managed runtime keeps its libraries: ldconfig_cache, managed_therock_sdk_probe_candidates, collect_managed_runtime_library_paths, collect_sdk_library_paths, and TheRockSdkProbeCandidate's site_packages/root_path fields. A second description of that layout is a second thing to keep correct.

collect_libraries_in_dir now sorts its results. That is a behavior change, not purely additive: where a directory holds more than one file matching libamd_comgr/libamdhip64, the HIP probe's "first hit" can now be a different file than the OS-arbitrary read_dir order it used before. Deterministic is the better answer for a report two people compare side by side, but it is worth stating plainly rather than filing this PR as behavior-preserving outside the one field it says it touches.

Corrections to the ticket

• The ticket says examine already reports the active runtime's owning installation. It does not — Examination records the system install only. That knowledge lives in a separate rocm examine surface. Part 2 needs it, and rocm-core's own runtime helpers can resolve managed runtime roots, so no cross-crate dependency is required.
• The ticket asks for the copies to appear in the human-readable report. Examination has no text renderer — it surfaces through --json and feeds diagnose. Putting them in the separate text report would mean a second, independent probe. Deferred to part 2, where the diagnosis puts the conflict in front of the user in prose.

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 (examine-finds-the-managed-runtimes-own-compilation-library, @requires-gpu @requires-os:linux) run in CI, not locally

Every new or changed unit test was verified to fail, not just to pass, each against a deliberately broken version of the thing it covers, then restored. See "Review remediation" below for the round added after the first review.

Coverage boundary

The e2e suite cannot install a second ROCm stack, so it cannot prove the two-copy case. It proves the surface every lane has: the inspection answers the question, the reported copies carry enough to act on, and the selected copy agrees with the list. The two-copy, symlink-dedupe, version-parsing, active-runtime-priority, and attribution rules are proven by unit tests that build the directory layout directly. This is a real boundary, not an oversight.

Risk

Medium, not Low as originally assessed: a first review round found the LD_LIBRARY_PATH/loader-cache search could name the wrong copy as "would load" on a managed install, and a fallback path that could never actually resolve on a real one. Both are fixed now, each with a regression test confirmed to fail against the old behavior first (see "Review remediation"). Still additive in shape — four new fields and a probe that only reads — but no longer risk-free by construction the way the first version was.

The Examination field set is a frozen wire contract with a test guarding it; that test fired on this change and was updated deliberately, which is the guard working as intended.

Review remediation

A first review round found four issues, all addressed in this commit:

1. comgr_selected and the multi-copy note named the ld.so.cache system copy as "would load" whenever a managed wheel runtime was active but this process's own LD_LIBRARY_PATH was empty — wrong, because rocm serve/rocm chat prepend the active runtime's own library directories onto LD_LIBRARY_PATH before launching the engine, so the wheel copy is what a served process actually loads. The search now takes the active runtime's interpreter (the same one already threaded through the framework probe) and searches its library directories first, ahead of the ambient environment.
2. The site_packages: None fallback in collect_managed_runtime_library_paths guessed a venv layout (root/lib/<python>/site-packages) the installer never produces — root is one of the runtime's own _rocm_sdk_* package directories, not a venv root, so the guess always found nothing. It now reads root.parent() instead, which is the site-packages directory the guess could never reach. The test fixture was built from the same wrong venv shape and so could not have caught this; it is rebuilt from the real installer layout, and confirmed to fail against the old fallback before passing against the fix.
3. probe_comgr's internals had no unit tests at all, so the source ordering, the ldconfig -p parsing, the /opt/rocm* scan, the selection, and the multi-copy note could all break with the rest of the suite still green. The ordering/selection logic is split into a pure find_comgr_copies, the ldconfig -p line parsing into parse_ldconfig_cache_paths, and the /opt scan into rocm_install_siblings (the directory to scan is now a parameter, not hardcoded, so a test can point it at a temp folder); each is unit-tested directly.
4. This PR's commit message previously claimed a HIP copy list, a search that works with no registry record (true now, after item 2's fix — it wasn't, before), and a single shared description of the loader path launched processes use (true only for the SDK-tool-probe path, not the served-engine path, which still has its own separate implementation). Corrected in the commit message.

A second round, one automated and one human, reaching the same place by different routes:

5. Item 2's fallback fix was itself confirmed unreachable: ROCM_SDK_PROBE_SCRIPT has recorded site_packages unconditionally since its initial version, so every manifest that parses at all carries Some, and the test for it called the function directly with None rather than through a real candidate either way. Rather than keep a reachable-looking branch and a comment describing behavior the code does not have, the None arm is now a no-op and the test is removed; the neighboring root-format test, which already called the function directly with None, is what actually exercises that arm's contract and is kept.
6. Three of the new unit tests (item 3) had no #[cfg(unix)] gate and fed a path into the :-split LD_LIBRARY_PATH parser, failing the required windows-build-and-test lane — the entire Windows failure on this branch. Gated, along with the helper they share.
7. Item 1's active-runtime-priority fix regressed examine-finds-the-managed-runtimes-own-compilation-library on a healthy host: once a managed runtime is active, its directory is found first and labelled active-runtime, and this search's existing dedup-by-resolved-path then drops the later managed-runtime hit for the identical file — the scenario's step asserted on the label that fix makes unreachable. The step now accepts either label, and a new unit test pins that an overlapping directory keeps exactly one label, confirmed to fail when the active-runtime search is reordered to run last.

Full detail is in the commit message's two Review: paragraphs.

[siloteemu]

🔴 Automated review · pr-review-watcher · 09476f8

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

Adds a Linux probe that records every libamd_comgr copy on the machine (path, resolved path, version, source), plus four new Examination JSON fields, four unit tests and one Gherkin scenario — outcome: Needs work (one blocking issue). Verified: ran the rocm-core examine test module (41 pass) on a throwaway copy and mutated it per branch — reverting the multi-hit walk, the resolve-path dedup and the digits-only version filter each fails exactly one test, but removing the intra-directory sort(), flipping copies.first() to .last(), and gutting probe_comgr to an immediate return all leave every test green; confirmed examine --json flattens Examination so the new keys really surface, that no other consumer reads the new fields or hip_libs_on_ld_path, that the managed-runtime layout claim matches runtime.rs, and that no comgr probe existed on the base at all (so the title's "not just the first" describes a new field rather than a widened one). Checks at review time: 24 success, 2 failures, 1 skipped, 1 cancelled. Blocking: 1 · Non-blocking: 4.

🚫 Blocking (must fix before merge)

crates/rocm-core/src/examine.rs:1982 — comgr_paths_in_loader_cache gates on which("ldconfig"), and which (same file, ~line 616) walks only the process PATH with no /sbin fallback. This re-introduces a bug the same crate already fixed and documented: crates/rocm-core/src/lib.rs:2543-2560 defines ldconfig_cache() whose doc comment says in as many words that "ldconfig lives in /sbin, which is not on a non-root user's PATH on Debian and derivatives. Looking it up by bare name there yields nothing, and an empty cache is indistinguishable from a cache that does not list the library" — and records the user-visible harm it caused last time (a correctly installed library read as "not registered with the linker"). Its fix is to try ["ldconfig", "/sbin/ldconfig", "/usr/sbin/ldconfig"].

Why it blocks: on those hosts the whole loader-cache tier silently contributes nothing, and that tier is the one that finds the copy the loader actually resolves. The consequences are exactly the ones this feature exists to prevent — a copy registered only in ld.so.cache (a ROCm install outside /opt, or one whose directory is on ld.so.conf but not LD_LIBRARY_PATH) is either missed entirely, producing the "no libamd_comgr found …" note on a machine that has one, or is ranked below a rocm-install/managed-runtime entry so comgr_selected names the wrong copy. It also silently degrades rather than reporting "could not ask", which is the distinction the sibling function was written to preserve. AGENTS.md §5 requires checking sibling implementations before adding a probe.

Fix: drop the which gate and reuse the existing helper — make ldconfig_cache() pub(crate) (the same treatment this PR already applies to collect_sdk_library_paths) and parse its Option<String>, treating None as "could not ask" rather than "no copies". It is timeout-bounded (capture_optional_command → OPTIONAL_COMMAND_TIMEOUT), so this does not trade away the 5s bound the current run(..., SHORT) gives. A smaller variant — mirroring the three-candidate list locally — works too but leaves two descriptions of the same trap to keep in sync.

Non-blocking

• crates/rocm-core/src/examine.rs:236,239,1875 — "in loader search order" / "the copy the loader would pick" overclaims: the rocm-install and managed-runtime tiers are not loader search locations at all unless they also appear on LD_LIBRARY_PATH/ld.so.conf, yet the hedge names only RUNPATH, ld.so.preload and container remapping; one sentence saying the last two tiers are evidence of what exists rather than of what loads would make the source field's purpose explicit and keep the future conflict rule honest.
• crates/rocm-core/src/examine.rs:1889-1917 — probe_comgr itself has no test: replacing its body with an immediate return leaves all 41 examine tests green, and the new scenario passes too (the fields serialise as []/null from Default), so on every lane without ROCm present the scenario proves only that two keys exist; the copies.first() selection rule and the multi-copy note are unproven, and a variant taking its search roots as a parameter would make both testable.
• crates/rocm-core/src/examine.rs:1861 — deleting matches.sort() fails nothing, so the "sorted so the result does not depend on directory iteration order" promise has no test; the ordering test uses two separate directories and never exercises intra-directory order.
• crates/rocm-core/src/examine.rs:457 — the comment justifying the call site says the search consults LD_LIBRARY_PATH "(read by probe_env)", but probe_comgr re-reads the variable itself and uses nothing probe_env stored; only the probe_rocm_install half of the stated ordering dependency is real.
• crates/rocm-core/src/examine.rs:1907 — the "N copies … would load" note fires on any host holding a system ROCm install alongside a runtime this CLI installed, which the commit message itself calls the normal case; harmless while Examination.notes is JSON-only (no consumer prints it today), but it will read as a warning the moment one does.

[juhovainio]

Nice write-up, and the reasoning behind the non-obvious calls (no dlopen, resolved-path dedup, WSL2 inclusion) all held up.

One thing I think is a real gap: the managed-runtime search path doesn't look like it actually covers the wheel-install case this PR is meant to catch. managed_sdk_ld_library_path (the existing helper) finds a wheel's comgr copy by walking site_packages for _rocm_sdk_* siblings, because the actual libraries live in a sibling package, not under the root it's handed. comgr_search_dirs's new managed_runtime_roots() path reuses collect_sdk_library_paths on the root directly but skips that site_packages walk entirely - so for a wheel-format install this probe likely never finds the copy that's the PR's own headline scenario. Left an inline comment at the spot.

Two smaller notes, not blocking:

• The /opt/rocm* sibling-install scan isn't mentioned in the PR description, which frames this as strictly system-vs-wheel. Worth a line in the write-up.
• collect_libraries_in_dir now sorts before taking the first match, which does change the HIP probe's tie-break order when a directory has multiple matching files - minor, but the Risk section's "unchanged" claim isn't quite true for that edge case.

Also a couple of judgement-call code-smell notes worth a look when convenient (not blockers): the same 4-line "Unix-only" test comment is duplicated across three tests in examine.rs, and ComgrCopy.source is a closed 4-value set that reads like it wants to be an enum rather than a bare String.

[juhovainio]

This only walks root via collect_sdk_library_paths. The existing managed_sdk_ld_library_path (lib.rs) additionally walks candidate.site_packages for _rocm_sdk_* sibling packages, because that's where the actual libraries live for a wheel install - this loop doesn't have (and doesn't look up) an equivalent site_packages value for managed_runtime_roots()'s roots, so it likely misses a wheel-installed comgr copy entirely.

[juhovainio]

This /opt/rocm* sibling-install scan isn't mentioned in the PR description (which frames the conflict as system-install vs. wheel-install only) - worth a line explaining it's in scope too.

[juhovainio]

This sort changes the HIP probe's tie-break order when a directory has more than one matching file (previously OS-arbitrary read_dir order, now sorted). The Risk section says "the HIP probe still takes the first hit and its field is unchanged" - true for the single-match case, not quite for this edge case.

Addressed

[siloteemu]

🔴 Automated review · pr-review-watcher · ee48870

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

The change generalises the code object manager scan from find-first to find-all, adding four Examination fields plus a shared library-path walker; the plurality itself holds up end to end, but a descoped ownership feature left behind an assertion and a doc claim that the shipped code cannot satisfy — Needs work. Verified: ran the touched crate's unit tests (51 pass) and five targeted mutations on a scratch copy — reverting the walker to first-hit kills exactly the_library_path_is_searched_left_to_right_and_every_copy_is_kept (the PR's headline claim, confirmed), breaking canonicalisation kills the symlink test, breaking the version parser kills two, dropping the no-registry fallback kills its test; also confirmed ComgrCopy has exactly four fields (no install_root), that comgr_selected/comgr_version reading index 0 is the intended loader semantics and the e2e step pins selected == copies[0], that the zero-copy and one-copy paths are both handled, that no deny_unknown_fields exists and #[serde(default)] genuinely suffices for the cross-machine deserialiser (checked against a real fixture lacking the fields), that the new note cannot alter status, that no test asserts exact notes contents, and that the PR body's "Examination has no text renderer" is accurate (the human report is built from a different type). The full suite was not run here. Checks at review time: 19 success, 1 failure, 1 skipped. Blocking: 2 · Non-blocking: 4.

On the red check: I did find a defect in this diff that would deterministically fail any run that actually executes the new GPU-gated scenario (blocker 1). That scenario resolves to skip rather than fail where no AMD GPU is present, so whether it is the failing check depends on the lane; I cannot confirm which check failed and am not inferring one. Either way the defect is real and independent of the base branch's intermittency.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/tests/e2e/examine_steps.rs:859-872 — the new step for scenario examine-17 asserts that every copy whose source is managed-runtime carries a non-empty install_root. ComgrCopy (crates/rocm-core/src/examine.rs:159-177) has exactly four fields — path, real_path, version, source — and nothing anywhere in the workspace emits install_root on a comgr copy. copy.get("install_root") therefore always yields None, unwrap_or_default() gives "", and the assertion fails on every execution. This is a test that cannot pass for the reason its message states, and it is the headline coverage the PR text offers for the managed case ("examine-17 asserts that on a GPU lane"), so the case is in fact uncovered and the scenario is a guaranteed red wherever a GPU is present. Fix: delete the for copy in managed { … } loop and its preceding comment (lines 859-872). What remains — copies non-empty, and at least one with source == "managed-runtime" — is still a non-vacuous assertion of the thing the scenario names. If the owning-install data is wanted now rather than in the follow-up, the alternative is to add the field to ComgrCopy and populate it in record_comgr_copy, but that is the deferred work and should not be bolted on here.

• crates/rocm-core/src/lib.rs:4172-4177 — the doc comment on collect_sdk_package_library_paths states "Attribution is by longest known root and no _rocm_sdk_* directory is a known root, so each resolves to its runtime — which is what keeps a healthy managed install from looking like several installations in conflict." No attribution-by-longest-root mechanism exists in this diff or in the base; nothing computes an owning installation for a comgr copy, and comgr_matches_runtime is documented as permanently unset. The comment presents a safety property as provided when the code provides nothing of the kind, which is precisely the guard-in-prose-only pattern — and it is the same descoped feature that produced blocker 1. The commit body carries the same claim twice ("recording every copy with the installation that owns it, plus the same for the HIP runtime so the two can be compared" and "Ownership is matched against known installation roots, longest first, and never derived by walking up from the file"); a maintainer reading either will believe attribution shipped. Fix: cut the second and third sentences of the doc comment, keeping "They belong to the runtime that contains them, not to themselves.", and drop the two ownership paragraphs from the commit body when the branch is next amended.

Non-blocking

• crates/rocm-core/src/examine.rs:1876 — removing matches.sort(); fails no test at all (verified by mutation), yet the comment beside it correctly flags it as a deliberate change to the existing HIP probe's tie-break, not a no-op. A two-file-in-one-directory case would pin it cheaply.
• The PR text's "Nothing existing changes behaviour except the LD_LIBRARY_PATH walker" understates the diff: managed_sdk_ld_library_path gains a new fallback that scans the predictable venv layout when no site-packages was recorded, which changes the loader path handed to the agent enumerator during gfx-target detection. The shared helper's new branch is tested; its effect through that caller is not, and both existing tests there populate site_packages.
• "One existing helper was made pub(crate)" — four items were: the probe-candidate builder, the SDK library-path collector, the candidate struct and two of its fields, plus the new shared helper.
• comgr_matches_runtime ships permanently None and no diagnosis check reads any of the four new fields, so the data is inert on the wire today. Deferring is disclosed and defensible; worth confirming the follow-up is what makes it non-inert. Relatedly, examine-16 passes vacuously on a host holding zero copies — the PR discloses this accurately, and the zero/selection consistency it does assert is real.

[siloteemu]

🔴 Automated review · pr-review-watcher · ee48870

Change request filed by automation. The findings below are the blocking half of the round published in the report comment on this pull request; the non-blocking notes stay there. This will be withdrawn once they are addressed — no human needs to clear it.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/tests/e2e/examine_steps.rs:859-872 — the new step for scenario examine-17 asserts that every copy whose source is managed-runtime carries a non-empty install_root. ComgrCopy (crates/rocm-core/src/examine.rs:159-177) has exactly four fields — path, real_path, version, source — and nothing anywhere in the workspace emits install_root on a comgr copy. copy.get("install_root") therefore always yields None, unwrap_or_default() gives "", and the assertion fails on every execution. This is a test that cannot pass for the reason its message states, and it is the headline coverage the PR text offers for the managed case ("examine-17 asserts that on a GPU lane"), so the case is in fact uncovered and the scenario is a guaranteed red wherever a GPU is present. Fix: delete the for copy in managed { … } loop and its preceding comment (lines 859-872). What remains — copies non-empty, and at least one with source == "managed-runtime" — is still a non-vacuous assertion of the thing the scenario names. If the owning-install data is wanted now rather than in the follow-up, the alternative is to add the field to ComgrCopy and populate it in record_comgr_copy, but that is the deferred work and should not be bolted on here.

• crates/rocm-core/src/lib.rs:4172-4177 — the doc comment on collect_sdk_package_library_paths states "Attribution is by longest known root and no _rocm_sdk_* directory is a known root, so each resolves to its runtime — which is what keeps a healthy managed install from looking like several installations in conflict." No attribution-by-longest-root mechanism exists in this diff or in the base; nothing computes an owning installation for a comgr copy, and comgr_matches_runtime is documented as permanently unset. The comment presents a safety property as provided when the code provides nothing of the kind, which is precisely the guard-in-prose-only pattern — and it is the same descoped feature that produced blocker 1. The commit body carries the same claim twice ("recording every copy with the installation that owns it, plus the same for the HIP runtime so the two can be compared" and "Ownership is matched against known installation roots, longest first, and never derived by walking up from the file"); a maintainer reading either will believe attribution shipped. Fix: cut the second and third sentences of the doc comment, keeping "They belong to the runtime that contains them, not to themselves.", and drop the two ownership paragraphs from the commit body when the branch is next amended.

Superseded: this objection was filed against an earlier commit and is replaced by a fresh round at the current head. The earlier blocking item is discharged in the tree; a new, unrelated blocking finding is filed separately.

[siloteemu]

🔴 Automated review · pr-review-watcher · a8749a3

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Summary

The change replaces the first-match-only libamd_comgr lookup with a walker that records every copy on the machine, in the order the search consults, together with which one would load and its version; it is reporting only, with the conflict rule deferred. Outcome: Needs work — one scenario does not exercise the code it names on the lanes where it is the only coverage. Verified: the eight new unit tests in the core crate were run on a scratch copy and pass, then production lines were mutated one at a time on that copy — dropping the recorded-site-packages arm kills two tests, dropping the no-record fallback kills one, deduplicating on the given path instead of the resolved one kills the symlink test, removing the digits-and-dots version guard kills the version table, and restoring stop-at-first-hit kills the ordering test, so the headline defect and the managed-runtime layout are genuinely covered; removing the new matches.sort() kills nothing; and with both probe_comgr call sites deleted, replaying examine-16's exact assertion sequence against the serialized examination still passes. Both items from the previous round are discharged in the tree — the install_root loop is gone and what replaced it is non-vacuous, and the attribution-by-longest-root sentences are gone from both the doc comment and the commit body. The previous round's non-blocking notes were not re-checked in this round. The full test suite, the e2e suite and clippy were not run here. Checks at review time: success=16, skipped=6, failure=4, pending=2; at publication, success=16, skipped=6, failure=5, pending=1. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/features/examine.feature:184-194 — examine-16 is untagged, so it runs on every lane, and its comment claims "What every lane can prove is that the inspection answers the question at all rather than staying silent, and that finding none is reported as a finding rather than a failure — which is the case the mock lane actually has." The code does not provide that; the struct defaults do. All four new fields are #[serde(default)] and Examination::default() already yields comgr_paths: [] and comgr_selected: null, which is exactly what both steps assert when no copy is found. Measured, not read: with both probe_comgr(&mut e) calls deleted, the assertion sequence of assert_comgr_copies_reported and assert_comgr_selection_is_stated (tests/e2e-cucumber/tests/e2e/examine_steps.rs:773, :800) still passes against the serialized examination. That is the state of every Windows lane, where the probe is never called at all, and of the mock lane the comment names. So on precisely the lanes where examine-16 is the only coverage of this change, it passes with the change reverted, while its prose says it proves the opposite — and examine-17 cannot compensate because it is @requires-gpu. Fix: tag the scenario @requires-os:linux (the probe is Linux and WSL2 only, so the Windows pass is vacuous by construction), and in the empty branch of assert_comgr_selection_is_stated assert that notes contains the "no libamd_comgr found ..." entry that crates/rocm-core/src/examine.rs:1927-1930 pushes, in addition to comgr_selected being null. That note is produced only by probe_comgr running and finding nothing, so it distinguishes "probed, found none" from "never probed" — which is the distinction the scenario's own sentence claims to make, and the same distinction the PR already builds into comgr_matches_runtime. Assert on a substring of the note rather than the whole string so the wording stays free to change; do not assert it in the non-empty branch, where a single copy pushes no note at all.

Non-blocking

• crates/rocm-core/src/examine.rs:246,249 — the "emulates the loader, is not the loader" caveat lives only on the private function; the two public field docs state "in loader search order", "the one that would load" and "the copy the loader would pick" as fact, and the rocm-install and managed-runtime tiers are not loader-searched at all absent RPATH/RUNPATH, so a consumer reading only the wire contract gets a stronger promise than the code makes.
• crates/rocm-core/src/examine.rs:1876 — the commit body calls the new sort out as "a change, not a no-op", but no test pins it: removing matches.sort() leaves all eight new unit tests green, because every fixture directory holds at most one matching file.
• crates/rocm-core/src/examine.rs:1996 — the loader-cache tier has no test at any level, so the /sbin gating fix the commit body describes is entirely unverified; note also that the base branch has no which("ldconfig") gate and this function is new, so that paragraph reads as fixing shipped code when it describes an earlier revision of this branch.
• tests/e2e-cucumber/tests/e2e/examine_steps.rs:400 — the new When step runs examine --json and stores stdout in cli_output, which is exactly what user_inspects_both_ways at :389 already does; two step names for one action is the second description this change elsewhere takes care to avoid.
• tests/e2e-cucumber/tests/e2e/examine_steps.rs:818-828 — asserting comgr_selected.path == comgr_paths[0].path restates the single line that assigns it, so it can only fail if the two fields are decoupled later; worth keeping as a contract check, but it is not evidence that the search order itself is right.

[siloteemu]

🔴 Automated review · pr-review-watcher · a8749a3

Change request filed by the automated review round at this commit. The full round, including non-blocking notes, is in the comment posted alongside it.

🚫 Blocking (must fix before merge)

• tests/e2e-cucumber/features/examine.feature:184-194 — examine-16 is untagged, so it runs on every lane, and its comment claims "What every lane can prove is that the inspection answers the question at all rather than staying silent, and that finding none is reported as a finding rather than a failure — which is the case the mock lane actually has." The code does not provide that; the struct defaults do. All four new fields are #[serde(default)] and Examination::default() already yields comgr_paths: [] and comgr_selected: null, which is exactly what both steps assert when no copy is found. Measured, not read: with both probe_comgr(&mut e) calls deleted, the assertion sequence of assert_comgr_copies_reported and assert_comgr_selection_is_stated (tests/e2e-cucumber/tests/e2e/examine_steps.rs:773, :800) still passes against the serialized examination. That is the state of every Windows lane, where the probe is never called at all, and of the mock lane the comment names. So on precisely the lanes where examine-16 is the only coverage of this change, it passes with the change reverted, while its prose says it proves the opposite — and examine-17 cannot compensate because it is @requires-gpu. Fix: tag the scenario @requires-os:linux (the probe is Linux and WSL2 only, so the Windows pass is vacuous by construction), and in the empty branch of assert_comgr_selection_is_stated assert that notes contains the "no libamd_comgr found ..." entry that crates/rocm-core/src/examine.rs:1927-1930 pushes, in addition to comgr_selected being null. That note is produced only by probe_comgr running and finding nothing, so it distinguishes "probed, found none" from "never probed" — which is the distinction the scenario's own sentence claims to make, and the same distinction the PR already builds into comgr_matches_runtime. Assert on a substring of the note rather than the whole string so the wording stays free to change; do not assert it in the non-empty branch, where a single copy pushes no note at all.

Addressed: examine-17 (renumbered) is now tagged @requires-os:linux and its empty-result assertion requires the "no libamd_comgr found" note, so it can no longer pass with probe_comgr reverted or never called. Verified by mutation-testing the note (suppressed it, watched the scenario fail, restored it).

[rominf]

🔴 Automated review · pr-review-watcher · e33f8ee

This automation never files a GitHub approval, so no approving review will
appear here whatever the outcome — the merge decision stays with a human
reviewer.

Summary

The PR changes rocm examine to report every libamd_comgr copy it finds, in loader search order, and to say which one would load. The managed-runtime library-dir search is now shared with managed_sdk_ld_library_path. Outcome: Needs work. Three new unit tests are not gated for Windows and fail there, and the new GPU scenario examine-19 cannot pass when it runs. Reviewed: the whole change (main...e33f8ee7, one commit, 4 files: crates/rocm-core/src/{examine.rs,lib.rs}, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs), plus its blast radius: active_managed_framework_interpreter, ROCM_SDK_PROBE_SCRIPT, the lemonade LD_LIBRARY_PATH prepend, and feature_naming.rs. This was a full pass rather than a follow-up, because the previously reviewed commit c5c15f73 is gone after the force-push. Verified: on Linux, cargo test -p rocm-core --lib examine:: passes (63 tests) and cargo clippy -p rocm-core -p e2e-cucumber --all-targets -D warnings is clean. Tracing the code confirms that serve prepends library_entries onto LD_LIBRARY_PATH, which justifies ranking active-runtime first, and that the probe writes nothing. The Windows and GPU lanes were not run locally. CI's windows-build-and-test on this head fails on exactly the three tests in blocking item 1, with the assertion values that item predicts. Blocking: 2 · Non-blocking: 11.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/examine.rs:3584, :3620, :3650: three new tests fail on Windows, and they are what turns the required windows-build-and-test check red. The tests are the_active_runtimes_own_copy_outranks_the_ambient_environment, ld_library_path_outranks_loader_cache_and_search_dirs and the_multi_copy_note_only_fires_past_one_copy, and none has a #[cfg(unix)] gate.
   • Each one passes a temp_dir() path as the ld_library_path string into find_comgr_copies → libraries_on_ld_path, which splits on ':'.
   • A Windows path like C:\Users\...\Temp\... is split into "C" plus a drive-relative \Users\..., so the "ambient"/"ld"/"only" directory is lost.
   • The assertions then fail: one_copy.len() == 1 gets 0, the first source is loader-cache rather than ld-library-path, and len() == 3 gets 2.
   • The code under test only runs on Linux/WSL (probe_comgr is called only from those branches), so gating the tests loses no coverage.
   • Fix: add #[cfg(unix)] (or #[cfg(target_os = "linux")]) to all three, as the neighbouring LD-path tests at :3149/:3179/:3199 already have.
2. tests/e2e-cucumber/tests/e2e/examine_steps.rs:1103-1126 (examine-19, examine.feature:270-271), together with crates/rocm-core/src/examine.rs:2252-2321 (find_comgr_copies / record_comgr_copy): examine-19 cannot pass on the lane it is gated to.
   • examine --json passes active_managed_framework_interpreter(...) (apps/rocm/src/main.rs:2695).
   • That interpreter's library_paths come from managed_therock_environment_from_record (lib.rs:4068) and include the SDK probe's recorded library_paths.
   • ROCM_SDK_PROBE_SCRIPT's add_runtime_root (apps/rocm/src/therock.rs:5259-5301) records each package root's lib, including _rocm_sdk_core/lib. That is exactly where managed_comgr_dirs finds the managed copy.
   • find_comgr_copies searches the active-runtime dirs first, and record_comgr_copy dedupes on the canonical path, keeping the first label it saw. So under examine-19's own precondition ("a managed runtime is active"), the copy is always labelled active-runtime and the later managed-runtime hit is dropped.
   • The assertion any(source == "managed-runtime") therefore fails. No unit test feeds find_comgr_copies overlapping dirs.
   • Fix: the scenario's stated intent is to find the CLI-installed copy, so make the step accept either active-runtime or managed-runtime. Better, assert that some copy's real_path lies under the active runtime's recorded site_packages/library_paths. If managed-runtime is meant to mean "a managed runtime other than the active one", say so in the ComgrCopy::source doc and add a unit test with overlapping dirs.

Earlier change request (review 5392128801, filed at c5c15f73)

This round read every file that change request was about.

• Items 2–4 were re-reviewed and not re-found. The site_packages: None fallback now reads root.parent() (lib.rs:4219-4229), and the fixture uses the installer layout. The commit message no longer claims a HIP copy list. find_comgr_copies, parse_ldconfig_cache_paths and rocm_install_siblings are split out and unit-tested.
• Item 1 is half addressed. The active runtime's own library dirs are now searched ahead of the ambient environment, so the wheel copy outranks the ld.so.cache system copy.
• Item 1's second half still holds. It said that directories on no loader path (/opt/rocm*/lib, managed-runtime) can also become "would load". find_comgr_copies (examine.rs:2252-2290) still selects copies.first() whatever its source. On a host with no active runtime, nothing on LD_LIBRARY_PATH and no cache entry, the first /opt/rocm*/lib or managed-runtime hit is still set as comgr_selected and named in "N copies … X would load", even though no loader consults that directory.
  • Fix: either select only from the active-runtime/ld-library-path/loader-cache sources and leave comgr_selected empty otherwise, or word the note so it does not claim a load for a search-dir hit.
  • That change request therefore stays standing.

Non-blocking

• crates/rocm-core/src/examine.rs:466-468: the comment says probe_comgr must run after probe_env because it reads the LD_LIBRARY_PATH that probe_env stored. It doesn't: probe_comgr reads the env var itself (:2229). Its only real ordering dependency is e.rocm_path from probe_rocm_install.
• crates/rocm-core/src/examine.rs:2380-2387: parse_ldconfig_cache_paths matches with line.contains(prefix) across the whole line, including the => /path part, while the directory scan uses starts_with on the file name. Matching only the soname token before the first space would make the two consistent.
• crates/rocm-core/src/examine.rs:2380-2387: ldconfig -p output is not filtered by architecture, so a libc6 (32-bit) entry would be recorded and could be selected if it comes first. Low risk, but the "emulates the loader" doc should mention the gap.
• crates/rocm-core/src/lib.rs:4204-4229: root is itself a _rocm_sdk_* directory under the scanned parent, so its lib dirs are pushed twice into the SDK-tool LD_LIBRARY_PATH. The Some(site_packages) branch already did this, and the new None fallback now does too. Harmless, but a dedupe (as push_existing_runtime_path does) would be cleaner.
• crates/rocm-core/src/lib.rs:4168-4189: managed_sdk_ld_library_path behaves differently when site_packages is None: it now also adds the parent's sibling packages. No test exercises that function directly; the change is only covered through managed_comgr_dirs.
• crates/rocm-core/src/lib.rs:4252: collect_sdk_library_paths was made pub(crate) but is only called inside lib.rs, so it can go back to private.
• crates/rocm-core/src/lib.rs:4191-4198: the doc says "One description of the layout … every search shares it", but collect_runtime_environment_paths (the served-engine path) still restates the layout without the sibling scan, as the commit body admits. Name the two actual callers instead.
• tests/e2e-cucumber/features/examine.feature:176: this deletes an unrelated blank line between examine-15 and the examine-16 comment block; every other scenario boundary in the file keeps one.
• crates/rocm-core/src/examine.rs:3145, :3175, :3195: the same 4-line "Unix-only" comment is pasted above three tests. One shared comment would do.
• Commit message: about half of it narrates review rounds ("Review: four more issues…", "Fix CI…", the renumbering history) rather than describing the change. The repo squash-merges, so that history ends up permanently in the final commit. Rewrite it as a description of the change.
• crates/rocm-core/src/examine.rs:2181-2420: the new functions are inconsistent about paths. Some use fully qualified std::path::Path/PathBuf, while probe_comgr and find_comgr_copies, added in the same diff, use the imported names.

[rominf]

🔴 Automated review · pr-review-watcher · c5c15f7

Change request filed by the automated review round at this commit. The full round, including the detail for each item and the non-blocking notes, is in the comment posted alongside it. It will be withdrawn by the automation once these are addressed.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/examine.rs:2214-2244: comgr_selected and the "N copies … X would load" note name the ld.so.cache system copy when a managed wheel runtime is present and the shell's LD_LIBRARY_PATH is empty. The processes the CLI launches against that runtime prepend its library_entries (engines/lemonade/src/process.rs ~L220-229), so the wheel copy is what actually loads. Directories that are not on any loader path (/opt/rocm*/lib, managed-runtime) can also become "would load". Restrict the selection to real loader-path copies, or compute it under the managed runtime's environment, and pin the chosen behaviour with a unit test.
2. crates/rocm-core/src/lib.rs:4159-4168: the site_packages: None fallback scans root/lib{,64}/*/site-packages. The installer records root_path as <site-packages>/_rocm_sdk_devel (apps/rocm/src/therock.rs:5279), so that path never exists. The test fixture (examine.rs:3383) uses a venv-root layout the installer never produces. Either fix the fallback (use root.parent() for _rocm_sdk_* roots) and build the fixture from the real shape, or drop the fallback and its test.
3. The commit message, which is the PR's only description, claims things the diff does not do: a HIP copy list, a search that works with no registry record, and a single shared description of the loader path that launched processes use. Make the message match the code.
4. probe_comgr, comgr_paths_in_loader_cache and comgr_search_dirs (examine.rs:2214-2371) have no tests. Source ordering, ldconfig -p parsing, the /opt/rocm* scan, the selection and the multi-copy note can all break with every test still green. Split out a pure function and unit-test it.

[juhovainio]

I reviewed this PR and found one substantive issue plus some description and structure notes, left inline below.

The substantive one: the site_packages: None fallback is unreachable, and its directory shape could not match even if it were reached. It ships a seven-line comment explaining that it rescues a host whose registry is missing or stale, which it cannot do, since a missing registry produces zero candidates upstream. The test that appears to cover it only passes because its fixture inverts the real parent/child relation, and the description offers those tests as the substitute for e2e coverage.

The rest: collect_libraries_in_dir now sorts, which can change which library the HIP probe picks, and the description says that field is unchanged. The visibility change is four symbols and two struct fields rather than one helper. And examine-19 looks fragile on a healthy host.

Things I checked and did not flag: the #[serde(default)] justification holds, mid-struct field insertion is fine here, the probe_env HIP rewrite is equivalent, and the @requires-os:linux tagging is right.

[juhovainio]

The None arm of this match is unreachable, and its directory shape could not match if it were reached.

Unreachable: ROCM_SDK_PROBE_SCRIPT sets site_packages unconditionally, outside the try, at apps/rocm/src/therock.rs:5228, and has done since the initial import 837067fd. No candidate reaches here with None.

Could not match anyway: root_path is already inside site-packages, pinned by the probe's own contract test at apps/rocm/src/therock.rs:9696-9698. So the fallback's root/lib/<python>/site-packages is a directory no managed runtime has.

The seven-line comment above it says it rescues "a host whose registry is missing or stale". It cannot: a missing registry yields zero candidates at lib.rs:4060, so there is nothing for this arm to run on.

Deleting the arm and taking site_packages: &Path would make the signature say what is actually true.

[juhovainio]

This test only looks like it exercises the fallback above. Its fixture puts site_packages under root_path, which is the reverse of the real relation, where root_path lives inside site-packages (pinned at apps/rocm/src/therock.rs:9696-9698).

That matters more than usual here because the description offers these unit tests as the substitute for e2e coverage of this path. Right now they certify a shape production never produces.

[juhovainio]

collect_libraries_in_dir now sorts its results, so the HIP probe taking "the first hit" can select a different library than before on a directory holding more than one match. The description says "The HIP probe still takes the first hit and its field is unchanged", which understates this: the mechanism is unchanged, the outcome need not be.

Worth either stating the ordering change in the description or confirming explicitly that no multi-match directory exists in practice.

[juhovainio]

comgr_matches_runtime is always None as of this PR, with the consumer arriving in #385. That is a reasonable way to split a stack, it just means this field ships inert, and anything reading rocm examine --json in between sees a null that means "not computed" rather than "no mismatch". Worth a line in the description.

Two structural notes while here. The description says "One existing helper was made pub(crate)", but it is four symbols plus two struct fields. And flattening TheRockSdkProbeCandidate into anonymous tuples is what forces those exports: the fields travel together everywhere, so keeping the struct and passing it whole would cost fewer exports and read better at the call sites.

Last one, minor: roughly 60 new lines go into lib.rs, which is already past 13,000 lines. AGENTS.md section 6 asks for new subsystems to get their own file, and while this is an extension rather than a new subsystem, the managed-runtime path-collection helpers now form a coherent group that would sit well on its own.

[juhovainio]

This scenario looks fragile on a healthy host. Dedupe keeps the first label, so if the managed runtime's library directory is on LD_LIBRARY_PATH, which is a normal state after serving, the copy is labelled ld-library-path rather than as the managed runtime and the scenario fails.

Separately, the description's e2e count of 15 does not reconcile with the 19 scenarios in the file.

[rominf]

🔴 Automated review · pr-review-watcher · e33f8ee

Change request filed by the automated review round at this commit. The detail for each item, and the non-blocking notes, are in the round's report comment. The automation will withdraw this request once these items are addressed. The earlier change request (filed at c5c15f73) also stays standing: part of its item 1 still holds, as the report explains.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/examine.rs:3584, :3620, :3650: the_active_runtimes_own_copy_outranks_the_ambient_environment, ld_library_path_outranks_loader_cache_and_search_dirs and the_multi_copy_note_only_fires_past_one_copy have no #[cfg(unix)] gate.
   • They feed a Windows temp path into a ':'-split LD_LIBRARY_PATH parser, so they fail on the required windows-build-and-test lane. They are the three failures on this head.
   • Fix: gate them #[cfg(unix)], like the neighbouring LD-path tests.
2. examine-19 (tests/e2e-cucumber/tests/e2e/examine_steps.rs:1103-1126) cannot pass on its GPU lane.
   • With a managed runtime active, its _rocm_sdk_core/lib is already among the active interpreter's library_paths. So find_comgr_copies labels the copy active-runtime, and dedupes away the later managed-runtime hit the step asserts on.
   • Fix: assert that the CLI-installed copy is found, whichever of those two labels it carries, or pin the intended label semantics with a unit test over overlapping dirs.