EAI-8240: Fix diagnose emitting invalid UNKNOWN render-group remediation

[r0x0r]

Summary

rocm diagnose built the render-group remediation command from the /dev/kfd
owner group, obtained via stat -c %G /dev/kfd. When the device GID has no
matching group name, stat prints the literal string UNKNOWN. The guard in
check_4_render_group only rejected empty strings, so UNKNOWN flowed straight
into the suggested fix:

sudo usermod -a -G UNKNOWN,video "$USER"

No group named UNKNOWN exists, so the command fails if a user copy-pastes it.
The bad command appeared in the human-readable plan, the summary, and the
machine-readable diagnose --json output.

Root cause

In crates/rocm-core/src/diagnose.rs (check_4_render_group), the group was
selected with a filter that only rejects empty strings (!g.is_empty()), not the
literal UNKNOWN that stat -c %G emits when the GID has no name.

Fix

Reject UNKNOWN (case-insensitive) alongside empty values so the group falls back
to render. The emitted command is now:

sudo usermod -a -G render,video "$USER"

This makes diagnose's fallback coincide with the render,video that
fix.rs hardcodes in the UNKNOWN/empty case. It does not unify the two
group sources: diagnose and fix still compute the group independently, and
they still diverge whenever /dev/kfd has a real group name that is not render
(diagnose suggests that name; rocm fix fix-4-render-group still runs
render,video). That pre-existing divergence is out of scope here.

Reproduction

On any container where /dev/kfd's GID is not mapped to a group name:

$ stat -c %G /dev/kfd
UNKNOWN

Before the fix, rocm diagnose (and --json) emitted the invalid UNKNOWN,video
command.

Testing

• Added regression test unknown_kfd_group_falls_back_to_render asserting both
  the UNKNOWN and unknown sentinel casings produce the render fallback and
  never appear in the command or summary.
• cargo test -p rocm-core diagnose:: — all 23 diagnose tests pass.
• cargo clippy -p rocm-core --all-targets -- -D warnings — clean.

Gherkin scenario deferred to follow-up (AGENTS.md §3)

This changes user-observable CLI output (the remediation command in the plan,
summary, and --json), which §3 requires a Gherkin scenario for. An earlier
version of this section claimed the condition "cannot be staged on the CI lanes";
that was incorrect and has been removed.

The condition is reachable end-to-end. stat_device
(crates/rocm-core/src/examine.rs) populates the owner group by shelling out to a
bare stat (Command::new("stat"), no env_clear), which resolves through the
child's PATH. So a stat shim printing an UNKNOWN owner group — the same
PATH-shim technique tests/e2e-cucumber/tests/e2e/dependency_guard_steps.rs
already uses for apt-get/ldconfig/sudo — drives the exact condition through
the real CLI, black-box, with no product-crate test seam.

One genuine constraint makes it a lane gate rather than an impossibility:
stat_device short-circuits on a real Path::new("/dev/kfd").exists(), a direct
std::fs call a PATH shim cannot intercept, so /dev/kfd must actually be
present. The scenario therefore belongs on the @requires-bare-metal lane that
the self-hosted Linux GPU runners exercise as part of the full suite — exactly the
"scenario can only run on a gated lane, say so and name the lane" case §3
describes.

Rather than land it here, the scenario is deferred to the follow-up that also
carries the non-blocking diagnose/fix clean-ups (the coherent
GID-capture/group-unification class). In the meantime the behavior is covered by
the unit-level regression test above, which reproduces the UNKNOWN GID directly.

[volen-silo]

Review: changes requested (both items are PR-text edits, not code changes)

The code change itself is correct, minimal, and well-tested. I ran cargo test -p rocm-core diagnose:: (23/23 pass) and cargo clippy -p rocm-core --all-targets -- -D warnings (clean) locally, and CI is green across the matrix. Two required items below, then some non-blocking notes.

Required

1. AGENTS.md §3 — user-observable behavior change with no scenario and no justification.

The PR body itself states the bad command appeared "in the human-readable plan, the summary, and the machine-readable diagnose --json output" — that is user-observable CLI output, which §3 says needs a Gherkin scenario in tests/e2e-cucumber/features/, and explicitly says "a unit test asserting the internal helper does NOT discharge this."

I checked: none of the 12 scenarios in tests/e2e-cucumber/features/diagnose.feature cover this. The file's own header says the scenarios "assert the SHAPE of a diagnosis", and the closest step — every reported cause comes with a command that applies it (tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:233-249) — only counts apply with: rocm fix lines, never inspects their content. A regression reintroducing UNKNOWN,video would sail through e2e.

I don't think a new scenario is the right answer here — the trigger (an unmapped /dev/kfd GID) can't be staged on the CI lanes, so a scenario would be vacuous. §3 anticipates that and requires the PR text to say so. A sentence in the PR body naming the gap and why it can't be covered would close this.

2. The PR body and commit message claim something the diff does not do.

This also unifies the group source so diagnose and fix no longer disagree.

There is no shared group source to unify. crates/rocm-core/src/fix.rs:129 and run_render_group (fix.rs:563-623, e.g. line 586) hardcode "render,video" unconditionally and never read Examination.kfd.owner_group; the two paths are looked up independently by fix_id with no Examination passed in. The patch only makes diagnose's independently-computed fallback coincide with fix.rs's hardcoded value in the UNKNOWN/empty case. The paths still diverge whenever /dev/kfd has a real group name that isn't render — diagnose suggests that name, rocm fix fix-4-render-group still runs render,video. That divergence predates this PR, but the claim as written overstates the change. Please soften the wording.

Non-blocking

3. The fallback is syntactically valid but not necessarily effective. Nothing in the codebase ever captures the numeric GID — stat_device asks for %A|%U|%G (never %g), and Device (crates/rocm-core/src/examine.rs:85-94) has no GID field. In the exact container case this PR targets, the local render group's GID usually won't match the unmapped GID on /dev/kfd, so sudo usermod -a -G render,video "$USER" can still leave the user without access — while the summary asserts it will fix it. That is a silent fallback that reads as a remediation. Worth considering: capture %g and either name the numeric GID or add a note when the group name is unresolvable, so the guidance is honest about the container case. (check_10_container_devices has the same limitation with its hardcoded --group-add render at diagnose.rs:1013 — pre-existing.)

4. Consider normalizing the sentinel at the parse boundary instead. stat -c %U emits the same UNKNOWN literal for an unmapped UID, and probe_devices populates render_devices[].owner_group through the same stat_device call. I verified there is no second leak path today (owner_user only feeds mode_access; render_devices is only checked for .is_empty()), so this is future-proofing rather than a bug — but mapping UNKNOWN to "" in stat_device would protect every present and future consumer by construction instead of requiring each call site to remember the guard.

5. The case-insensitive arm is untested. The new test only sets owner_group: "UNKNOWN" (uppercase), so swapping eq_ignore_ascii_case("UNKNOWN") for == "UNKNOWN" would keep all 23 tests green even though the PR body specifically claims case-insensitive rejection. Either add a lowercase case, or drop to an exact match — stat's sentinel is a fixed uppercase literal, and the loose comparison also discards a real group genuinely named unknown (unlikely, but a behavior difference bought for nothing).

6. Adjacent, same class, while you're here. check_6_path_missing (diagnose.rs:670-758) splices bin_dir unvalidated into directly runnable command strings — export PATH={bin_dir}:$PATH, echo 'export PATH={bin_dir}:$PATH' >> ~/.bashrc, setx PATH "%PATH%;{bin_dir}". Same "probed value flows into a suggested command" pattern as the bug being fixed here, with a much larger input surface (an arbitrary filesystem path that may contain spaces or shell metacharacters) and no filter at all. Pre-existing and out of scope, but a reasonable follow-up if this class is being cleaned up.

Positive

• Leaving the raw value in the evidence line (diagnose.rs:566-569, "group UNKNOWN") is the right call — evidence should report what was observed; only the remediation has to be actionable. The test correctly makes no assertion about it, which shows the scoping was deliberate rather than an oversight.
• The regression test asserts the actual command content and the negative (summary must not contain the bogus name), rather than just "a fix exists". It genuinely fails before the change.
• Single logical change, signed commit with DCO sign-off, no AI-generated footers.

[r0x0r]

Thanks — both required items addressed, plus the untested-arm note. Force-pushed 8af2cb5 (amend of the single commit) and updated the PR body.

1. §3 justification (required): added a "No Gherkin scenario" section to the PR body. The trigger is an unmapped /dev/kfd GID (stat -c %G → UNKNOWN), which the e2e harness can't stage — there's no way to present a device whose GID has no group name — so a scenario would be vacuous. The unit regression test reproduces the UNKNOWN GID directly, and §3's carve-out for behavior that can't run on the available lanes is now named explicitly rather than left implicit.

2. Softened the overclaim (required): the "unifies the group source" sentence is gone. The PR body and the amended commit message now say the fix makes diagnose's fallback coincide with the render,video that fix.rs hardcodes in the UNKNOWN/empty case only, and explicitly note the two sources are still computed independently and diverge when /dev/kfd has a real non-render group name — a pre-existing divergence out of scope here. (For the record, the "unifies" phrasing was only in the PR body; the original commit message said "matching the group that fix.rs hardcodes", which I've also made more precise.)

5. Case-insensitive arm now tested (non-blocking): unknown_kfd_group_falls_back_to_render now loops over both UNKNOWN and unknown, so dropping eq_ignore_ascii_case to an exact match would fail the lowercase case. Kept the case-insensitive guard rather than tightening to exact match. 23/23 diagnose tests pass.

3, 4, 6 (non-blocking, follow-ups): agreed and out of scope for this one logical change:

• Migrate Python environment management from pip to uv #3 the fallback is syntactically valid but the local render GID may not match the unmapped /dev/kfd GID in the container case — capturing %g to name the numeric GID (or noting unresolvability) is the honest fix; check_10_container_devices shares the limitation.
• Fix CI: prek hooks, headless/flaky test fixes, and drop vendored Codex #4 normalizing UNKNOWN→"" at the stat_device boundary would protect every present/future consumer by construction; reasonable future-proofing.
• Land #4 (prek hooks + flaky-test fixes + drop vendored Codex), rebased onto #1-3 #6 check_6_path_missing splices bin_dir unvalidated into runnable command strings — same "probed value into a suggested command" class, larger surface.

I'll file these as a follow-up so the class gets cleaned up deliberately rather than piecemeal.

[volen-silo]

Follow-up review at 8af2cb5 — one item remains, and it is mine to own

Re-verified against the current head (8af2cb5, force-pushed amend), not the state at the time of the first review.

Gates, run locally at this head (exit codes, not grepped output):

• cargo fmt --all -- --check — exit 0
• cargo clippy --workspace --all-targets -- -D warnings — exit 0
• cargo test --workspace --all-targets — exit 0

Per finding

2. Overclaim softened — resolved. Verified in both surfaces. The commit message now reads "This makes diagnose's fallback coincide with the render,video that fix.rs hardcodes in the UNKNOWN/empty case; it does not unify the two group sources", and the PR body says the same. I re-checked the underlying facts at this head: crates/rocm-core/src/fix.rs:129 and run_render_group (fix.rs:586,597) still hardcode "render,video" and never read Examination.kfd.owner_group, so the softened wording is accurate. Nothing overstated remains.

5. Case-insensitive arm — resolved, and I confirmed it by mutation rather than by reading. The test now loops for sentinel in ["UNKNOWN", "unknown"]. I killed two mutants locally at this head:

• dropping the guard back to .filter(|g| !g.is_empty()) → fails with left: ["sudo usermod -a -G UNKNOWN,video \"$USER\""]
• swapping eq_ignore_ascii_case("UNKNOWN") for g != "UNKNOWN" → fails on the lowercase pass with left: ["sudo usermod -a -G unknown,video \"$USER\""]

Both mutants are caught. The test is real, and the case-insensitive claim is now actually load-bearing.

3, 4, 6 (non-blocking) — fine as deferred. Agreed these are a coherent class and better cleaned up deliberately than piecemeal. No objection to them leaving this PR.

1. §3 scenario justification — the item I asked for is done, but the premise I gave you was wrong, and the text now states something untrue.

You did exactly what I asked: the PR body has a "No Gherkin scenario" section. The problem is that I told you the trigger "can't be staged on the CI lanes", you reasonably took that at face value, and I was wrong. I checked properly this time.

The harness can stage it, and the technique is already in the repo:

• stat_device (crates/rocm-core/src/examine.rs:876-885) populates owner_group by shelling out — run("stat", &["-c", "%A|%U|%G", path], SHORT) — and run (examine.rs:330) uses Command::new("stat"), a bare program name with no env_clear, so it resolves through the child's PATH.
• run_rocm_with_env (tests/e2e-cucumber/tests/e2e.rs:527) sets arbitrary env vars on the CLI under test, PATH included.
• dependency_guard_steps.rs:39,116 already does precisely this: writes executable shims (apt-get, ldconfig, sudo) into a temp dir and runs the CLI with PATH pointed at it.

So a stat shim printing crw-rw----|root|UNKNOWN gets the exact condition through the real CLI, black-box, with no test seam in the product crates.

One genuine constraint, which is a lane gate rather than an impossibility: stat_device short-circuits on exists: Path::new(path).exists() (examine.rs:879), a direct std::fs call that a PATH shim cannot intercept. /dev/kfd has to really be there — so this belongs on @requires-bare-metal, which the self-hosted Linux GPU lanes run as part of the full suite (tests/e2e-cucumber/README.md, tags table). That is the case §3 covers explicitly: "if the scenario can only run on a gated lane, say so in the PR text and name the lane that will exercise it." §3 has no "cannot be staged at all" carve-out — I invented that in the first review and the PR body now cites it back.

What I think should happen: I am not going to block this a second time on a requirement I waived myself. The fix is correct, minimal, and now genuinely well-tested, and re-litigating it would cost more than it buys. But the justification paragraph should not merge as written, because it asserts a harness limitation that does not exist and will be quoted as precedent the next time someone wants to skip a scenario. Minimum: replace "the e2e harness has no way to present a device whose GID has no group name" with the accurate version — the condition is reachable via a stat PATH shim on a bare-metal lane, and the scenario is being deferred to the follow-up you already committed to. Better: fold the scenario into that follow-up alongside items 3/4/6 and say so. Whether the scenario must land here or can ride the follow-up is a maintainer call, and given I am the reason it was skipped, I would not hold the PR for it.

CI

Three checks are red at this head — all three are pre-existing, none attributable to this diff. Flagging so nobody blames the PR:

• E2E tests (GPU): 81 scenarios (72 passed, 9 failed) with 4 unexpected regressions (bench-load-real-serve, chat-end-to-end-local-model, serve-vllm-inference, serve-vllm-default-on-instinct). The most recent completed run on main shows the identical count and the identical four. All 12 diagnose scenarios passed here.
• E2E tests (Strix Halo, Ubuntu): 35m timeout, stuck after Model serving / Scenario 1 panicked with no 'resolved model' in output on empty stdout. Same timeout, same panic, same spot on a main run a day earlier.
• Read the Docs: fails on unrelated PRs against main too; the repo has no .readthedocs.yaml at all. Systemic, not this change.

Verdict

Blocking items 1 and 2 as originally stated are both discharged, and non-blocking item 5 is fixed and verified. The code is good. The one thing I would not merge as-is is the §3 justification text, because it is factually wrong about the harness — and that error started with me, not with you. Sorry for the bad steer.

[r0x0r]

Appreciated — and no worries about the earlier steer.

I've corrected the §3 justification in the PR body. It no longer claims the condition "cannot be staged"; instead it documents that the UNKNOWN owner group is reachable end-to-end via a stat PATH shim (the technique dependency_guard_steps.rs already uses for apt-get/ldconfig/sudo), that stat_device's real Path::new("/dev/kfd").exists() short-circuit makes it a @requires-bare-metal lane gate rather than an impossibility, and that the scenario is deferred to the follow-up alongside the coherent items 3/4/6 — with the unit-level regression test covering it in the meantime. No code change; body text only.

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