fix(diagnose): unify remediation flag wording with rocm fix

[jussielo-amd]

Summary

• rocm fix <id> and rocm diagnose each built their Flags:/flags: line with separate inline logic and different wording for the same concepts (e.g. "requires sudo" vs "sudo"), so the same fix-id read differently depending on which command surfaced it.
• rocm diagnose's line also stayed silent on manual fixes instead of stating it explicitly, unlike rocm fix.
• Extracted a shared format_flags() in fix.rs and updated both print_recipe (fix.rs) and render_report_text (diagnose.rs) to use it, standardizing on fix.rs's clearer wording.
• While reconciling the two surfaces, found check_9_igpu_dgpu_collision's Linux/else branch in diagnose.rs set auto_applicable: false, disagreeing with the fix-9-igpu-dgpu FixRecipe in fix.rs (auto_applicable: true, already runnable via run_hip_visible_devices). This is a behavior change, not text-only: it flips the Fix struct field that rocm diagnose --json serializes for that fix-id, in addition to its rendered text. No other FixRecipe/Fix field or JSON shape changed.
• Both diagnose surface's manual-only wording now names rocm fix explicitly instead of "this command", removing ambiguity when the line is read outside its original command's context.

Scope boundaries

• fix-9-igpu-dgpu is not the only diagnose-vs-catalog drift: fix-5-amdgpu-load's needs_reboot is conditional (!blacklisted.is_empty()) in diagnose.rs but unconditional true in the fix.rs catalog. Left unfixed here, and no general "diagnose-constructed Fix matches its catalog entry" guard was added — both are deliberately deferred to fix(diagnose): stop claiming the CLI will apply fixes it only reports #382, which replaces the flat auto_applicable bool with a derived enum across all 16 recipes and is the more natural place to close this drift class for good rather than patching one field at a time.

Test plan

• cargo test -p rocm-core — 396 passed
• cargo clippy -p rocm-core --all-targets -- -D warnings — clean
• cargo fmt -p rocm-core -- --check — clean
• Manually confirmed rocm fix fix-5-amdgpu-load --dry-run and rocm fix fix-4-render-group --dry-run print the unified wording
• format_flags unit test exercises all 16 (sudo, reboot, relogin, auto_applicable) combinations
• Unit test pins fix-9-igpu-dgpu's Linux auto_applicable: true against both the Fix struct field (JSON) and render_report_text's rendered flags: line (exact match), so a regression back to false, or a revert of the render call site, fails in CI
• Unit test pins fix-11-iommu's rendered flags: line (sudo+reboot, manual-only wording) exactly, closing the one case a revert of render_report_text to its pre-PR inline logic could otherwise still pass (mutation-verified)
• e2e diagnose-01 scenario asserts every reported cause in a real rocm diagnose run has its own flags: line ending in the auto/manual marker, exercising render_report_text's call site directly, and now asserts there's at least one cause to check rather than trusting a sibling step for that

[Copilot]

🟡 Changes recommended

Metadata inconsistencies remain, and requested regression coverage and documentation are missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR centralizes remediation-flag formatting for rocm fix and rocm diagnose.

Changes:

• Adds shared format_flags() logic.
• Updates both text renderers.
• Standardizes applicability and remediation wording.

File summaries

File	Summary	Final review comments
crates/rocm-core/src/fix.rs	Adds and uses shared flag formatting.	Moderate (1 vote): Metadata can still differ between commands. Nit (3 votes): Add regression coverage for flag combinations and CLI output. Nit (1 vote): Update related documentation.
crates/rocm-core/src/diagnose.rs	Reuses shared formatting in diagnosis reports.	Moderate (1 vote): Duplicated metadata can report incorrect applicability or reboot requirements; derive status from shared recipe data or reconcile the metadata.

Review details

Suppressed comments (3)

crates/rocm-core/src/diagnose.rs:2191

• This formatter still trusts the duplicated Diagnose::Fix metadata, so the output is not actually canonical for every fix-id. For example, the Linux branch of fix-9-igpu-dgpu sets auto_applicable to false, while the shared RECIPES entry marks it true and rocm fix can apply it when --device-index is supplied; this line consequently tells users manual only (this command will NOT run it) even though the command can run it. fix-5 also disagrees conditionally on needs_reboot. Reconcile the catalog metadata or derive the status flags from the shared recipe before using this formatter.

                fix.auto_applicable,

crates/rocm-core/src/fix.rs:766

• The helper unifies the labels, but it does not make a given fix-id's flags identical across the two commands. For example, diagnose sets fix-5-amdgpu-load.needs_reboot only when a blacklist is present (diagnose.rs:730), while the rocm fix recipe always sets needs_reboot: true (fix.rs:179); a non-blacklisted load failure therefore still renders different flags despite this comment's parity claim. Either align the underlying metadata/state handling or narrow the claim to wording-only consistency.

/// Canonical wording for a fix's remediation flags, shared by `rocm fix <id>` and
/// `rocm diagnose` so the same fix-id reads identically from either command.

crates/rocm-core/src/fix.rs:766

• This changes a public command's human-readable output and its exact remediation wording, but the corresponding README/help documentation and docs/testing.md/docs/manual-testing.md expectations are unchanged. Please document the new Flags:/flags: contract in the same change so user guidance and manual verification stay aligned with the implementation.

/// Canonical wording for a fix's remediation flags, shared by `rocm fix <id>` and
/// `rocm diagnose` so the same fix-id reads identically from either command.

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Lite (auto)

Note

Copilot is running an experiment and ran this review at Lite.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[siloteemu]

🔴 Automated review · pr-review-watcher · 4e31bd8

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 extracts a shared format_flags() in fix.rs and routes both rocm fix <id> (print_recipe) and rocm diagnose (render_report_text) through it, standardising on fix.rs's wording and making the flags line unconditional. Outcome: Needs work — the unification introduces a user-visible false statement for one fix-id, and the surface it actually changes (diagnose) carries no test. Verified: ran one targeted unit test (format_flags) in a scratch copy outside the checkout with five single-branch mutations (wording change, flag reordering, inverted auto/manual, dropped reboot branch, empty manual arm) — all five were caught; confirmed by reading source that the flags line is now unconditional on both surfaces, that print_recipe runs before the OS gate in apply (so the new scenario is safe to leave OS-ungated), that fix-4-render-group really is sudo+re-login+auto with no reboot, that the struct_excessive_bools workspace-lint claim in the new comment is true, and — by scripted comparison of every diagnose-constructed Fix against the fix.rs catalog — that fix-9-igpu-dgpu is the sole auto_applicable mismatch. The full test suite and the e2e suites were not run here. CI on this head: 18 success, 1 skipped, 0 failure, 0 pending. Blocking: 2 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/diagnose.rs:1041 (vs crates/rocm-core/src/fix.rs:251) — the unified wording now prints a false claim for fix-9-igpu-dgpu on Linux.
check_9_igpu_dgpu_collision builds its Fix twice: the Windows branch sets auto_applicable: true (line 1025), the Linux/else branch sets auto_applicable: false (line 1041). The catalog entry it mirrors — fix.rs:248-268 — is OS-independent auto_applicable: true, applies_on: LINUX_AND_WINDOWS, runner: Some(run_hip_visible_devices), which dispatches to a Linux implementation. So on Linux rocm fix fix-9-igpu-dgpu prints Flags: rocm fix can run it, while rocm diagnose now prints flags: manual only (this command will NOT run it) for the same fix-id. That is exactly the divergence the PR's own doc comment (fix.rs:765-766) promises is gone — "so the same fix-id reads identically from either command" — and it is the repository's most common defect class: prose stating false product behaviour. Note this is introduced by this PR: before the change, that Fix had no flags at all (all three optional booleans false, auto false), so the line was omitted and nothing false was said; making the auto/manual marker unconditional is what surfaces the latent mismatch as a user-visible claim. Fix: set auto_applicable: true on the Linux branch to match the catalog (no existing test asserts !auto_applicable for this fix-id — the two !fix.auto_applicable assertions at diagnose.rs:2360 and :3083 cover a shared-memory finding and fix-wsl-3-rocdxg-missing), or, if diagnose deliberately discourages auto-run there, stop presenting it as the same catalog id. Cause analysis for standing focus (c): the codebase invited this — diagnose.rs hand-copies the catalog's four booleans per finding instead of reading FixRecipe, so the two can drift silently. This will recur. The cheap prevention is one unit test in diagnose.rs that, for every diagnose-constructed Fix, asserts its auto_applicable/needs_sudo/needs_reboot/needs_relogin match the catalog entry for its fix_id — that assertion would have failed on this PR.

2. crates/rocm-core/src/diagnose.rs:2187-2193 — the changed surface has no test; the added tests all cover the other surface.
The diagnose renderer emits flags: (lowercase, 3-space indent); print_recipe emits Flags: (capitalised, padded). Both new e2e steps (diagnose_steps.rs:919-953) and the diagnose-05 addition drive rocm fix <id> --dry-run and assert the Flags: form — that is print_recipe, not diagnose. The new unit test exercises format_flags directly. Grepping the diagnose.rs test module for flags: / the old wording returns nothing, so no pre-existing test covers it either. Net: the diagnose output — whose wording changed from sudo / reboot required / re-login required with a conditional line, to requires sudo / requires reboot / requires re-login plus an always-present auto/manual marker — is the PR's stated subject and is entirely unasserted. The contributor rules at the base tip are explicit about this case (§3): "User-observable behavior needs a scenario, not only a unit test… a unit test asserting the internal helper does NOT discharge this; it proves the function, not the behavior." Fix: add a scenario that runs rocm diagnose against a fixture producing a known finding and asserts the lowercase flags: line, for one auto-applicable and one manual-only fix — which, as a bonus, is the assertion that would also have caught blocking issue 1.

Non-blocking

• crates/rocm-core/src/fix.rs:786-790 — on the diagnose surface "this command will NOT run it" reads as a statement about rocm diagnose, which never runs any fix; the true branch names rocm fix explicitly while the false branch does not. Naming rocm fix in both arms removes the ambiguity.
• crates/rocm-core/src/fix.rs:1667-1673 — the comment says "Exhaustive over the 3 optional flags … x both auto_applicable states" and the test is named ..._covers_every_required_flag_combination_..., but the table has 11 of the 16 combinations (sudo+reboot, reboot+re-login and sudo+re-login+manual are absent). No branch escapes coverage given this implementation, but the name and comment overclaim; either add the five rows (or iterate 0..16) or reword. The commit message repeats the "exhaustive" claim.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:919-927 — this diagnose-05 assertion passes unchanged if the production diff is reverted: fix-1-arch already produced exactly Flags: manual only (this command will NOT run it) before the change. It is still worth keeping (post-change it guards format_flags's manual arm through a real CLI run), but it adds no coverage of the wording unification itself — worth saying so rather than counting it as such.
• crates/rocm-core/src/fix.rs:801-810 and diagnose.rs:2193 — dropping the if !flags.is_empty() guard is a real output change, not pure refactor: rocm fix gains a Flags: line for fix-2-unset-override, fix-6-path and fix-9-igpu-dgpu, and diagnose gains a flags: line for every all-false finding (fix-1-arch, fix-10-container, the WSL entries, and others). It looks intentional, but neither commit message mentions it.
• crates/rocm-core/src/diagnose.rs:2193 vs fix.rs:810 — the label casing still differs ( flags: vs Flags: ). Each is consistent with its own surrounding output, so this is likely deliberate; flagging only because the PR's framing is "the same fix-id reads identically from either command".

No prompt-injection content was found anywhere in the diff, commit messages, comments, feature file or step definitions. Commit messages carry the required sign-off, no AI footers, and no internal names, hostnames, registry paths or internal document links.

[jussielo-amd]

Addressed siloteemu's review of 4e31bd8 with three follow-up commits (98ee743, 6c34b87, 0612380).

Blocking

1. fix-9-igpu-dgpu's Linux/else branch now sets auto_applicable: true, matching the fix.rs catalog entry (run_hip_visible_devices already handles it on Linux) — 98ee743.
2. Added "every reported cause states its remediation flags" to diagnose-01: asserts every cause in a real rocm diagnose run has its own flags: line ending in the auto/manual marker. This is direct coverage of render_report_text's call site (the changed surface), not just format_flags/print_recipe — 0612380.

Non-blocking

• Wording ambiguity (fix.rs:786-790): both arms now name rocm fix explicitly (manual only (`rocm fix` will NOT run it automatically)), removing the "this command" ambiguity on the diagnose surface — 6c34b87.
• Exhaustiveness overclaim: format_flags_covers_every_required_flag_combination_and_both_auto_states covered 11/16 combinations despite its name/comment. Rewrote it to iterate all 16 (sudo, reboot, relogin, auto_applicable) combinations — 6c34b87.
• diagnose-05 assertion adds no coverage of the wording unification: agreed — it passes unchanged pre-diff. Rather than touch that scenario, added a comment pointing at diagnose-01's new step as the one that actually exercises this surface — 0612380.
• Dropping the if !flags.is_empty() guard: confirmed intentional. It's from the original commit (a7ab88d), not this fixup, and yes it also changes rocm fix's own output — fix-2-unset-override, fix-6-path, and fix-9-igpu-dgpu now always show a Flags: line. Left as-is: an always-present line is what makes the auto/manual marker something tests can rely on for every fix-id.
• Flags:/flags: casing difference: confirmed deliberate and unchanged by this PR — each matches its own surrounding output style, and no README/docs reference this exact wording, so the doc-update suggestion from Copilot's review doesn't apply here.

Full verification: cargo test -p rocm-core --lib (394 passed), cargo clippy -p rocm-core --lib -- -D warnings and cargo clippy -p e2e-cucumber --tests -- -D warnings (clean), cargo fmt --check (clean), and cargo xtask e2e -- -n "diagnose|fix" (16/20 passed; the 4 failures are pre-existing and environment-specific — confirmed by reproducing them against the pre-fixup baseline on this WSL host).

Leaving all threads open for a maintainer to resolve per repo convention.

[Copilot]

🟡 Changes recommended

The fix-9 JSON behavior change needs explicit regression coverage and documentation in the PR description.

Get a fresh assessment by requesting another Copilot review.

Review details

• Files reviewed: 4/4 changed files
• Comments generated: 1
• Review effort level: Balanced

[jussielo-amd]

Follow-up review (98ee743..ae49a88)

Focused this pass on the changes since the last full review round, plus a blast-radius sanity check on the whole PR.

Addressed the outstanding Copilot finding (review 5245543252, diagnose.rs:1044): the fix-9-igpu-dgpu auto_applicable flip (false → true on Linux, from 98ee7433) is a real behavior change affecting both rocm diagnose --json and rendered text, not text-only as originally described.

• ae49a880 adds fix_9_igpu_dgpu_is_auto_applicable_on_linux, a regression test that pins both the struct field and the rendered flags: line for this case. Verified by mutation testing: flipping auto_applicable back to false in check_9_igpu_dgpu_collision's Linux branch makes this test fail immediately; reverted cleanly (confirmed empty git diff afterward).
• The PR description has been corrected to describe this as a behavior change rather than text-rendering only.

Quality gates on current HEAD (ae49a880) — all green:

• cargo test -p rocm-core: 395 passed, 0 failed
• cargo clippy -p rocm-core --all-targets -- -D warnings: clean
• cargo fmt -p rocm-core -- --check: clean

Blast-radius check on the full diff: no new issues found. Two pre-existing, non-blocking items remain from earlier review rounds (unchanged by this follow-up, out of its scope):

• fix-5-amdgpu-load's needs_reboot in diagnose.rs is conditional (!blacklisted.is_empty()), while the fix.rs FixRecipe catalog entry has it always true — same drift class this PR targets for fix-9, but for a different fix-id.
• The docs (README.md, docs/testing.md, docs/manual-testing.md) haven't been updated to describe the shared flags:/Flags: wording contract (a minor nit from the first review round).

No code changes were needed this round. PR looks mergeable pending CI.

[Copilot]

🔵 Needs a closer look

No user-observable scenario specifically verifies fix-9’s changed applicability value.

Review details

Suppressed comments (1)

crates/rocm-core/src/diagnose.rs:1044

• The fix-9-igpu-dgpu applicability change is still not covered by a user-observable scenario. diagnose-01 accepts either the auto or manual marker for every cause, so it passes even if this line regresses to false, and diagnose-20 exercises fix-4-render-group rather than this diagnosis/JSON field. Add a Gherkin scenario that triggers the iGPU+dGPU diagnosis and asserts this fix-id is reported as auto-applicable (including the gated lane if the fixture requires one).

            auto_applicable: true,

• Files reviewed: 4/4 changed files
• Comments generated: 0 new
• Review effort level: Balanced

Both counts resolved, verified by mechanism rather than from the commit descriptions: the Linux branch now matches the catalog entry for that fix id, confirmed by a mutation that reddens the new test, and the diagnose render surface now carries a scenario rather than only unit coverage of the other surface. Retiring this objection. No blocking findings remain at the current head.

[siloteemu]

🔴 Automated review · pr-review-watcher · ae49a88

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 extracts a shared format_flags() in fix.rs, routes both rocm fix <id> (print_recipe) and rocm diagnose (render_report_text) through it, makes the auto/manual marker always present, and — in the follow-up commits since our last review — corrects fix-9-igpu-dgpu's Linux auto_applicable to match the catalog, names rocm fix explicitly in the manual-only wording, and adds coverage on the diagnose surface. Outcome: no blocking issues remain; both counts of our previous change request are discharged, with a real but non-blocking coverage residual on the diagnose render call site. Verified: ran one targeted unit-test group (format_flags + fix_9_igpu_dgpu_is_auto_applicable_on_linux) in a scratch copy outside the checkout, green at head, then seven single-branch mutations there — flipping auto_applicable back to false (caught), reordering the flags (caught), rewording "requires sudo" (caught), inverting the auto/manual arms (caught by both tests), deleting the reboot branch (caught), reverting print_recipe to its pre-PR inline logic (all 395 unit tests still green — that surface is e2e-only), and reverting render_report_text to its pre-PR inline logic (all 395 unit tests still green — see non-blocking 1); independently confirmed the fix-9-igpu-dgpu catalog entry is auto_applicable: true with a Linux-capable runner and no platform gate refusing it, that a full sweep of every diagnose-constructed Fix against the catalog now leaves fix-5-amdgpu-load as the only remaining flag mismatch, that the new diagnose-01 step cannot pass vacuously because the preceding step already asserts causes > 0 on the same output, and that no README/docs/--help text quotes the changed flag strings. The full test suite and the e2e suites were not run here. Merge base and base-branch tip are different commits, but the diff against each is byte-identical. Check-run conclusions at this head: 25 success, 1 skipped, 1 pending, 0 failure. Blocking: 0 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/diagnose.rs:2187-2195 — the diagnose render call site has no unit-level pin: reverting that hunk to the pre-PR inline logic leaves all 395 rocm-core unit tests green (mutation-verified). The new fix_9_igpu_dgpu_is_auto_applicable_on_linux survives it because fix-9 carries only the auto flag, so old and new code print the same line for that one case. Cheap, deterministic fix: assert the exact rendered line ( flags: rocm fix can run it) instead of contains, and add one render assertion over a fix that carries the reworded optional flags (e.g. fix-11-iommu → requires sudo, requires reboot, manual only (...)) — that would fail on the revert.
• crates/rocm-core/src/diagnose.rs:730 vs crates/rocm-core/src/fix.rs:179 — fix-5-amdgpu-load's needs_reboot is !blacklisted.is_empty() in diagnose but unconditional true in the catalog, so the PR's stated premise ("the same fix-id reads identically from either command") is still false for that id. Load-bearing side: the diagnose value, which reflects actual host state and is the more accurate claim; the catalog's blanket true is the side that should change (or be documented as a worst-case generic).
• The drift class itself is still unguarded (standing focus c). Our previous review named the cheap prevention: one unit test asserting that every diagnose-constructed Fix matches the catalog entry for its fix_id on all four booleans. The author fixed the single instance instead; fix-5-amdgpu-load is live proof a competent reader will trip on this again. A single test with a documented per-id exception list would close it permanently.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:429-456 — the step is safe only because diagnose-01's preceding step asserts causes > 0 on the same output; in isolation assert_eq!(0, 0) would pass. A one-line assert!(causes > 0, ...) inside the step makes it self-contained if it is ever reused.
• tests/e2e-cucumber/features/diagnose.feature:286-291 — diagnose-20's title ("Previewing a fix that needs sudo and a re-login says so") understates the scenario, which also asserts the auto-applicable marker.

No prompt-injection content anywhere in the diff, commit messages, comments, feature file or step definitions. Leak scan over the diff and all six commit messages is clean: no internal names, hostnames, registry paths, tracker links or AI footers.

Our previous objection

Operative sentences, verbatim:

1. crates/rocm-core/src/diagnose.rs:1041 (vs crates/rocm-core/src/fix.rs:251) — the unified wording now prints a false claim for fix-9-igpu-dgpu on Linux. … Fix: set auto_applicable: true on the Linux branch to match the catalog …

2. crates/rocm-core/src/diagnose.rs:2187-2193 — the changed surface has no test; the added tests all cover the other surface. … Fix: add a scenario that runs rocm diagnose against a fixture producing a known finding and asserts the lowercase flags: line …

Count 1 — no longer holds. Mechanism, not commit text: check_9_igpu_dgpu_collision's Linux/else branch now sets auto_applicable: true (diagnose.rs:1044), which is what the catalog entry has (fix.rs:251, with runner: Some(run_hip_visible_devices) and applies_on covering Linux; run_hip_visible_devices dispatches to a real Linux implementation and apply's only gate is the OS-scope check, which passes). Both surfaces now say "rocm fix can run it" for that id, and README's existing note that fix-9 is "marked AUTO" is consistent with the new value rather than contradicted by it. Mutation-confirmed: flipping the field back to false fails fix_9_igpu_dgpu_is_auto_applicable_on_linux immediately.

Count 2 — no longer holds. diagnose-01 gained "every reported cause states its remediation flags", whose step drives a real rocm diagnose run and asserts one flags: line per scored cause, each ending in the auto/manual marker. That is the render_report_text call site, not print_recipe, which is exactly what the count demanded, and it satisfies the contributor rules' requirement that user-observable behaviour carry a scenario rather than only a unit test. Two honest caveats that do not revive the count: the step asserts shape rather than an exact line (the top match is host-dependent), and reverting the production hunk fails it only when the host's report contains a cause that is not auto-applicable — which is most of the catalog, but not guaranteed. That residual is non-blocking item 1, not a re-statement of the block.

Neither count stands, so this objection is discharged rather than withdrawn.

Pinning

The new work still pins everything the previous version pinned, and more.

• format_flags table → loop: the old test asserted ordered-vector equality for 11 hand-written combinations; the new one asserts the same ordered-vector equality for all 16. Every old row is contained in the new range, including the sudo+re-login+auto shape that motivated diagnose-20. The expected vector is built test-side from literal strings with its own push order, so it is not a mirror of the implementation: reordering the flags (mutation M2) and rewording one flag (M3) both still fail, as does deleting a branch (M5) and inverting the auto/manual arms (M4). Nothing weakened; the rename from ..._covers_every_required_flag_combination_... to ..._covers_every_flag_combination_... also removes the overclaim we flagged.
• diagnose-05's manual-only step: previously it asserted a string the PR had not changed, so it passed pre-diff (our earlier non-blocking note). At this head it asserts Flags: manual only ( + backticked rocm fix + will NOT run it automatically), a string this PR introduces, so it now fails on revert. Strictly stronger, not vacated.
• diagnose-20's two steps are untouched in the delta; the sudo+re-login assertion still requires the trailing comma that only the always-present marker produces, so it still fails against pre-PR print_recipe.
• Added, not removed: the fix-9 struct-field and rendered-line assertions.
• Nothing was dropped, relabelled or narrowed: the delta contains no deleted assertion other than the 11-row table that the 16-case loop supersedes, and no assertion changed from equality to a weaker check.

Sign-off

Six commits in the range, zero merge commits. All six carry a Signed-off-by trailer matching the author identity, read from the raw commit objects: ae49a88, 0612380, 6c34b87, 98ee743, 4e31bd8, a7ab88d. All six also carry an SSH signature block. No missing trailer.

Replies

• Reply 1 (answering our 4e31bd8 review): every claim checks out on mechanism — fix-9's Linux branch matches the catalog, diagnose-01's new step really does cover render_report_text rather than print_recipe/format_flags, both arms of the marker now name rocm fix, and the 11-of-16 overclaim is replaced by a genuine 16-case loop. One point is now stale in the author's own disfavour: they agreed diagnose-05's assertion "passes unchanged pre-diff" and added a clarifying comment instead of strengthening it — but their wording change in 6c34b87 made that step pin the diff after all, so it is stronger than the reply credits. The "no README/docs reference this exact wording" claim is correct; the AUTO/PRINT-ONLY text in README and --help describes a different surface and stays accurate.
• Reply 2 (follow-up on 98ee743..ae49a88): correct. The fix-9 change is indeed behavioural rather than text-only (it flips the serialised Fix field as well as the rendered line), and the claimed mutation result reproduces here — reverting auto_applicable to false fails the new test. The unit-test count is consistent with what I ran at this head. The two residuals the reply lists as remaining (the fix-5-amdgpu-load drift and the absent doc line) are both real; I confirmed the fix-5 mismatch independently and confirmed that no doc currently quotes the flag strings, which makes the doc item optional rather than a contributor-rule gap.

[jussielo-amd]

Addressed 3 of siloteemu's 5 non-blocking residuals from the ae49a880 review, in commit 8f7e2e0:

1. diagnose.rs:2187-2195 render call site had no unit-level pin — the fix-9-igpu-dgpu render assertion is now exact-match instead of contains, and a new fix_11_iommu_rendered_flags_line_is_exact test pins a fix-id whose optional flags (sudo+reboot, manual-only wording) actually differ between old and new format_flags output. Mutation-verified: reverting render_report_text's flags block to its pre-PR inline logic still passes the fix-9 test (confirming your point that a single-flag case can't tell old/new apart) but now fails the fix-11 test.
2. diagnose_steps.rs:429-456 step safe only via a sibling step — added assert!(causes > 0, ...) so assert_every_cause_has_flags is self-contained if reused elsewhere.
3. diagnose.feature:286-291 diagnose-20 title undersold the scenario — renamed to mention the auto-applicable assertion it also makes.

The other two residuals (fix-5-amdgpu-load's needs_reboot drift, and no general diagnose-vs-catalog drift-class guard) are deliberately left open — #382 replaces the flat auto_applicable bool with a derived enum across all 16 recipes and is the more natural place to close that drift class for good. Noted this explicitly in the PR description's new "Scope boundaries" section so it doesn't need rediscovering.

cargo test -p rocm-core: 396 passed. clippy/fmt clean on both rocm-core and e2e-cucumber.

[siloteemu]

🔴 Automated review · pr-review-watcher · 8f7e2e0

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 extracts a shared format_flags() in fix.rs, routes both rocm fix <id> (print_recipe) and rocm diagnose (render_report_text) through it, makes the auto/manual marker always present, and corrects fix-9-igpu-dgpu's Linux auto_applicable to match the catalog. Outcome: No blocking findings — the earlier clean verdict still holds, and the new head closes the coverage residual that motivated it. Verified: ran one targeted unit-test group (format_flags plus the two diagnose render tests) in a scratch copy outside the checkout, green at head, then six single-branch mutations there — reverting render_report_text's flags block to its pre-PR inline logic (now caught, by the new fix-11 test), flipping fix-9's auto_applicable back to false (caught), reordering sudo/reboot (caught), inverting the auto/manual arms (caught by all three), deleting the re-login branch (caught), and restoring the old "omit the line when only the marker is present" conditional (caught); independently confirmed that run_hip_visible_devices really performs the Linux change rather than printing, that fix-11-iommu's diagnose-side and catalog-side booleans agree, that score= and flags: are emitted exactly once per shown cause inside the same take(top) loop so the e2e count equality is sound, that the workspace lint table really does allow struct_excessive_bools, and that no README, --help or docs text quotes the changed flag strings. The full suite was not run here, and neither was the e2e suite. Check state at review: 24 success, 1 skipped, 1 pending, 0 failure. Blocking: 0 · Non-blocking: 5.

Since the last round

One new commit since ae49a880 (8f7e2e08), plus a base-branch merge that brings in unrelated upstream work; the PR's own diff is unchanged in shape — still the same four files. The delta is test-only and addresses three of the five non-blocking residuals from the previous round:

• crates/rocm-core/src/diagnose.rs — the fix-9-igpu-dgpu render assertion changed from two contains checks to one exact assert_eq! on the whole line, and a new fix_11_iommu_rendered_flags_line_is_exact pins a fix-id carrying sudo+reboot+manual, whose rendered line genuinely differs between the old and new wording.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs — one added assert!(causes > 0, ...) so the step no longer depends on a sibling step for its non-vacuity.
• tests/e2e-cucumber/features/diagnose.feature — diagnose-20's title now names the auto-applicable assertion it also makes.

Pinning (nothing vacated). The new work pins everything the previous head pinned and more. The only deleted assertions are the fix-9 contains("rocm fix can run it") and !contains("manual only") pair, superseded by an exact whole-line equality that strictly implies both. Nothing changed from equality to a weaker check, no assertion was narrowed or relabelled, and no scenario or step was removed. The residual I raised last round is closed on mechanism, not just on the commit message: reverting the render_report_text hunk to its pre-PR inline logic now fails fix_11_iommu_rendered_flags_line_is_exact deterministically, while fix_9_igpu_dgpu_is_auto_applicable_on_linux still passes — exactly the asymmetry the new test's own comment describes. The earlier clean finding therefore still holds, with a smaller residual than before.

Sign-off: seven commits in the range, no merge commits authored by the PR. Each carries exactly one Signed-off-by: trailer, read line-by-line from the raw commit objects. No missing trailer, no AI footer, no internal names, hostnames, registry paths or tracker links anywhere in the diff or the messages. No prompt-injection content in the diff, comments, feature file, step definitions or commit messages.

🚫 Blocking (must fix before merge)

None.

Non-blocking

• crates/rocm-core/src/diagnose.rs:731 vs crates/rocm-core/src/fix.rs:179 — fix-5-amdgpu-load's needs_reboot is still !blacklisted.is_empty() in diagnose but unconditional true in the catalog, so the PR's premise ("the same fix-id reads identically from either command") remains false for that id. Load-bearing side: the diagnose value, which reflects real host state; the catalog's blanket true is what should change or be documented as a worst-case generic. A full sweep of all other diagnose-constructed fixes against the catalog found no second mismatch. The author has stated this and the drift-class guard are deliberately deferred to other work; I did not verify the state of the change they reference.
• The drift class itself is still unguarded — one unit test asserting every diagnose-constructed Fix matches its catalog entry on all four booleans would close it permanently and would have caught both the fix-9 and fix-5 cases.
• crates/rocm-core/src/fix.rs:762-769 — the doc comment says rocm fix <id> and rocm diagnose now "read identically", but rocm fix with no id still labels the same property AUTO / PRINT-ONLY (list_recipes, and the matching text in README and --help). That third vocabulary is pre-existing and consistent within itself, so nothing here is wrong; the claim is just narrower than it sounds. Scoping it to "rocm fix <id> and rocm diagnose" would say what is true.
• crates/rocm-core/src/fix.rs:767 — the parenthetical names struct_excessive_bools while the attribute immediately below it is fn_params_excessive_bools. The comment is accurate (the struct lint is allowed workspace-wide, and the local allow is separately needed), but a second independent reader took it as claiming the local allow was redundant. Naming both lints explicitly in one clause prevents that misread recurring.
• crates/rocm-core/src/diagnose.rs:1044 — diagnose now prints "rocm fix can run it" for fix-9-igpu-dgpu, which matches the catalog and is the right call, but README's fix section notes that without --device-index that fix only prints the index query and changes nothing "despite being marked AUTO". The report carries no equivalent caveat. Pre-existing on the Windows branch and on the listing, so not introduced here; worth a note in the fix's notes if it ever bites.

[jussielo-amd]

Addressed 3 of the 4 non-blocking items from this review, in commit 6836c4d:

• fix.rs:762-769 doc-comment scope ambiguity — format_flags's doc comment now explicitly says the "reads identically" claim is scoped to rocm fix <id> and rocm diagnose only, and that the bare rocm fix catalog listing keeps its own separate, untouched AUTO/PRINT-ONLY vocabulary for the same property.
• fix.rs:767 lint-name ambiguity — the comment above #[allow(clippy::fn_params_excessive_bools)] now names both that lint and the workspace-wide clippy::struct_excessive_bools allow explicitly, and states why both are needed (struct fields vs. this free function's params), so neither reads as making the other redundant.
• diagnose.rs:1044 fix-9 note missing the --device-index caveat — the diagnose-side note now carries the same caveat README already states for rocm fix: without --device-index, the fix only prints the query and changes nothing, despite being marked AUTO. Pinned with a new assertion on the existing note test (assert!(note.contains("Without --device-index") && note.contains("despite being marked AUTO"))).

The remaining item (fix-5-amdgpu-load's needs_reboot drift + the general drift-class guard) stays deliberately deferred to #382, as already noted in the PR description's "Scope boundaries" section — no change there.

cargo test -p rocm-core: 396 passed. clippy/fmt clean.

[Copilot]

Copilot review overview

🟡 Changes recommended

Fix-9 is marked automatically applicable although the displayed command performs no remediation without --device-index.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1

Open (1)

• Narrow documentation to wording, not identical fix flags · New

Previously missed (2)

In code that hasn't changed since last review

Keep diagnosis non-applicable without required device argument

crates/​rocm-core/​src/​diagnose.rs:1051

auto_applicable: true makes both JSON and the new flags line claim that the displayed apply with: rocm fix fix-9-igpu-dgpu command can perform the remediation, but that command provides no --device-index; run_hip_visible_devices_linux only prints a query and returns without changing anything in that case. The new note explicitly acknowledges this contradiction. Keep this diagnosis non-auto-applicable until conditional applicability is represented (or emit the required argument) rather than changing the accurate value to match the stale catalog entry.

Clarify scenario verifies vocabulary, not identical flag values

tests/​e2e-cucumber/​tests/​e2e/​diagnose_steps.rs:439

This comment overstates what the scenario verifies: sharing format_flags gives both surfaces the same vocabulary, but their independently constructed flag values can still differ (for example, fix-5-amdgpu-load's conditional reboot flag). The assertions below check line presence and the applicability marker, not that a fix-id reads identically from both commands.

[johnl-amd]

Looks good, approving.

What I checked on c99aa64: all CI green (20 checks, docs skipped by path filter), no unresolved review threads, and the shared format_flags has real teeth. The mutation runs in the automated review are the convincing part, reverting either call site to its pre-PR inline logic now fails deterministically, which was the gap in the first pass.

One deliberate deferral worth naming on the way in. fix-9-igpu-dgpu now reports auto_applicable: true from diagnose to match the catalog, but as your own note says, without --device-index the fix prints a query and changes nothing. Adopting the catalog's value is the right call here because the drift between two sources of truth was a genuine bug, and patching a single bool is the wrong layer for the real problem. That problem is #382. Could you add the fix-9 case to it explicitly, so "AUTO but a no-op without a flag" is in scope when the bool becomes an enum rather than something we rediscover later?

Nothing else from me. The wording has converged and I do not think another round buys anything.