[volen-silo]

Summary

Several doctor fixes that apply on Windows printed lines written for one shell. Windows PowerShell 5.1 rejects them or runs them wrongly. This change rewrites the lines so that they run in Windows PowerShell 5.1 and, where the same line is also shown to cmd users, in cmd. A few lines that only rocm diagnose prints for Windows are PowerShell-only by design.

This follows #680, which added the fix-22 recipe. The widened text rule below found a bug in fix-22's verify line, and it is fixed here.

• fix-15-msvc-redist verify. It was where vcruntime140.dll && where vcruntime140_1.dll. In Windows PowerShell 5.1 && is a syntax error, and where is the alias for Where-Object. It is now one powershell -NoProfile -Command "..." line that prints the file version of both DLLs. It finds the DLLs with [Environment]::GetFolderPath('System') inside the body, so the body holds no $ and an outer PowerShell has nothing to expand (an expanded $env:SystemRoot would split the command at a space in the path). It uses -ErrorAction Stop: Get-Item reports a missing path as a non-terminating error, which leaves the exit code at 0, so without the flag a missing DLL would not fail the verify as it did before. The fix-21 verify and the diagnose copies use the same GetFolderPath form, and fix-21's line carries -ErrorAction Stop too, for the same reason: msvcp140.dll missing must fail it.
• fix-6-path verify. It chained two commands with &&, which Windows PowerShell 5.1 rejects, and the plan is printed on every OS. The catalog now prints two lines: Verify: is hipcc --version, which runs in every shell and checks that the bin directory is on PATH, and a new Verify (Linux, WSL): line is rocminfo | head -n 5. The second line shows the GPU and, unlike hipcc, exists on a runtime-only Linux install, so it passes there when PATH is fixed. It is a new optional recipe field, printed after verify, in the way the recipe's # Linux: command lines are already printed on every OS; the plain Verify: line, the catalog manifest and the exit codes are unchanged. A Linux note says which line to run and why the plain one fails on a runtime-only install. rocm diagnose on Linux (an arm that only builds for a bash host) prints the catalog's Linux line alone, rocminfo | head -n 5. It used to chain && hipcc --version, which fails on a runtime-only install with PATH fixed. rocminfo lives in the bin directory this fix puts on PATH, so it printing the GPU shows both that PATH works and that the GPU is visible; the check this fix exists for is kept, and hipcc is not needed for it. The Windows arm of the checker verifies with & '<bin>\hipInfo.exe' | Select-Object -First 5, with a ' in the folder doubled by the shared escape.
• fix-2-unset-override. The verify was env | grep HSA_OVERRIDE_GFX_VERSION || echo OK_UNSET. It is now a python -c one-liner that no shell changes. A stock Ubuntu has python3 and no python, so that line fails there with "command not found" after a correct fix. The recipe therefore also has a Linux and WSL line, printed as Verify (Linux, WSL): echo "${HSA_OVERRIDE_GFX_VERSION:-OK_UNSET}". It needs no Python and no external command, and it has no || (the text rule refuses || on a line a Windows user sees, so the old env | grep ... || echo OK_UNSET form could not be reused as is). A note tells the user to run the labelled line on Linux and WSL. On Linux, rocm diagnose prints that line followed by python3 -c "import torch; print(torch.cuda.is_available())", so the user can tell whether the GPU still works after the override is cleared (the shared catalog line cannot assume torch). python3 is used because the interpreter that has torch is reachable as python3 in a stock install, a virtual environment and a conda environment alike. The Windows clear line was setx HSA_OVERRIDE_GFX_VERSION ""; Windows PowerShell 5.1 drops the empty argument, so setx failed with "Invalid syntax" and cleared nothing. It is now [Environment]::SetEnvironmentVariable('HSA_OVERRIDE_GFX_VERSION',$null,'User'). The Windows commands also tell the user that the change only affects new shells. rocm fix fix-2-unset-override on Windows does not run that text: it spawns setx directly, with no shell, and passes the empty argument intact.
• fix-13, fix-14 and fix-9 (Windows) verify lines. They were wrapped in powershell -Command "..." but could not be pasted into PowerShell: \" in the body ends the string early, and $_ or $env:NAME=... is expanded by the outer shell. They now use single quotes, Join-Path, Where-Object Name -match and SetEnvironmentVariable(..., 'Process').
• fix-22 verify (added in feat(examine): show the AMD driver version on Windows and flag old drivers #680). It used Where-Object { $_.Name ... } in a double-quoted -Command body, so $_ was expanded to nothing and the line failed with "The term '.Name' is not recognized". It now uses Where-Object Name -match, as fix-14 does. The widened text rule below is what found it.

The fix-6 PATH write is not changed here. It keeps the bare PowerShell line, its two Windows notes and ps_single_quote_escape that are already on main.

Tests

• fails_in_windows_powershell is the rule for a line shown on Windows. It is a text rule over known-bad constructs and does not prove that a line runs; a construct it does not list passes. It refuses && and ||; a bare where (after |, ;, (, { or a lone &, at the start of a -Command body, and after cmd /c); a %NAME%; an empty-string argument outside a -Command body; a single-quoted -Command argument; and inside a double-quoted -Command body a \" or any $, with no exemption. It also refuses a Get-Item inside a -Command body that does not carry -ErrorAction Stop, because a missing path is a non-terminating error and the line would exit 0. Other cmdlets that behave this way are not listed. -c and the other prefixes of -Command count as the flag, and the matches (-Command, and a bare where such as CMD /C WHERE x) ignore case.
• The rule exists twice, because the e2e crate cannot depend on rocm-core. Both copies are tested against one table of cases, tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt (flag <line> and pass <line>). A case that one rule gets wrong fails on that side, so the two cannot drift on a listed line. The e2e copy is a module of the e2e crate with its own test; no crate dependency was added (cargo xtask check-crate-edges passes).
• Unit tests apply the rule to every command, the verify line and the Linux verify line of each recipe that applies on Windows (taken from applies_on, so a new recipe is covered) and to the Fix each Windows checker builds.
• The shared-id test keeps the catalog and diagnose copies of the fix-15, fix-13, fix-14 and fix-22 lines equal. Separate tests do the same for fix-2 (the Linux arm is the catalog's Linux line plus the python3 torch check) and fix-6 (the Linux arm is the catalog's Linux line alone). fix-21's two copies are covered by the shared-id test. One test gives the fix-6 checker C:\Users\O'Neil\ROCm and asserts the apostrophe is doubled in the PATH write and the verify line. fix-9's Windows line exists only in the diagnose checker, so it has its own tests.
• the_path_fix_verifies_the_gpu_on_linux_without_needing_hipcc asserts fix-6's two printed verify lines, that the Linux one shows the GPU and does not name hipcc, that the plain one does not name rocminfo, and that a note names the runtime-only install and the labelled line. the_override_fix_has_a_linux_verify_line_that_needs_no_python does the same for fix-2 (printed lines, no python in the Linux one, a note naming python3 and the labelled line). a_linux_verify_line_belongs_to_a_recipe_that_applies_on_linux_or_wsl and a_print_only_plan_labels_its_linux_verify_step cover the field and its printing.
• Unix-only tests run the Linux lines in /bin/sh with a PATH made of stand-in executables, so the claims in the notes are checked against a run, not a string: the_path_fix_verify_passes_on_a_runtime_only_linux_install (the rocm diagnose line exits 0 and prints the GPU rows with a rocminfo and no hipcc on PATH, and the old && hipcc --version chain fails there), the_override_fix_verify_runs_where_only_python3_exists (the rocm diagnose line prints the override's value or OK_UNSET, then the torch check, with python3 and no python, set and unset) and the_override_linux_verify_runs_with_nothing_on_path (the catalog's Linux line, with an empty PATH). They run on the Linux CI lanes and under WSL2.
• Text tests that run on every host check the shape of the fix-15, fix-2, fix-13, fix-14 and fix-9 lines.
• Windows-only unit tests run the printed lines through cmd and Windows PowerShell, with the line fed on stdin as if typed. One helper in fix.rs does the running, and starts Windows PowerShell without an inherited PSModulePath (from a PowerShell 7 session the 5.1 child otherwise loads PowerShell 7's PSReadLine and writes "Cannot load PSReadline module" to stderr). Each asserts exit 0 and an empty stderr, plus what is specific to its line:
  • the_msvc_verify_runs_in_cmd_and_powershell (fix-15): one row for each of vcruntime140.dll and vcruntime140_1.dll, each with a dotted version.
  • the_msvc_verify_exits_nonzero_when_a_dll_is_missing (fix-15): with the second DLL's name changed to one that does not exist, both shells exit non-zero and name the file. It exits 0 without -ErrorAction Stop.
  • the_outdated_msvc_verify_prints_a_version_and_fails_when_the_dll_is_missing (fix-21): the line prints a dotted version with an empty stderr, and with the DLL's name changed to one that does not exist both shells exit non-zero and name the file. It exits 0 without -ErrorAction Stop.
  • the_override_verify_runs_in_cmd_and_powershell (fix-2): OK_UNSET with the override unset, its value with it set.
  • the_hip_sdk_verify_runs_the_exe_under_hip_path (fix-13): HIP_PATH points at a throwaway directory whose name has a space and whose bin\hipInfo.exe is a copy of whoami.exe, and the line must print its output. With no hipInfo.exe it must print nothing and report an error.
  • the_adrenalin_verify_runs_and_its_filter_keeps_only_amd (fix-14): the line runs, and the Where-Object stage, taken from the recipe text, is run over made-up adapters so only the AMD and Radeon ones come out.
  • the_display_driver_verify_runs_in_cmd_and_powershell (fix-22): the line runs with an empty stderr. It fails on the old $_ form.
  • the_igpu_verify_pins_the_variable_it_names (fix-9, in diagnose.rs): a stand-in torch module returns what HIP_VISIBLE_DEVICES holds, and the unchanged line must print 1.
• Scenarios diagnose-43 (diagnose-msvc-verify-line-text-is-powershell-safe), diagnose-44 (diagnose-override-fix-lines-are-powershell-safe) and diagnose-45 (diagnose-path-fix-lines-are-powershell-safe) preview fix-15, fix-2 and fix-6 with rocm fix --dry-run and apply the same rule to the printed lines (they were 42 to 44 until main took diagnose-42). diagnose-44 also asserts that the printed Linux verify line reads the override without Python and that a note names the python3 case. diagnose-45 asserts that the printed Linux verify line shows the GPU without hipcc, and that a note names the runtime-only install. The plan prints before the platform gate, so they run on the Linux lane. They see what the catalog prints, not the Fix values rocm diagnose builds for Windows (see the next point), and they check text and do not run the lines.
• Scenarios diagnose-46 (diagnose-override-verify-line-names-python3) and diagnose-47 (diagnose-path-verify-line-is-rocminfo-alone) assert what rocm diagnose itself prints on Linux: for fix-2 with HSA_OVERRIDE_GFX_VERSION set, the exact verify after fix: line (the override read and python3, no bare python); for fix-6 on a simulated Ubuntu host, rocminfo | head -n 5 with no hipcc. Both are @requires-os:linux, so they run on every Linux lane (WSL2 included) and are skipped on Windows. The Windows arms of rocm diagnose (fix-2's clear command and the verify lines of fix-6, 9, 13, 14, 15, 21 and 22) have no scenario: they are covered by windows_diagnosis_remediation_passes_the_powershell_text_rule (text) and the Windows-only tests that run the lines in cmd and PowerShell, which the windows-build-and-test lane runs. I checked diagnose-46 and 47 by changing only production code (python3 back to python, && hipcc --version back on) in a WSL copy of the tree: both fail, and 43 to 47 pass without the change.
• The fix-2 Windows runner now labels the setx line it prints as arguments the CLI runs, not a line to paste; the text is built from the same array that is passed to setx. Nothing pinned the old text, and no test captures this output.
• The tests for the changed lines were shown to fail without the change, by changing only production code: with fix-2's verify_linux removed, with the rocm diagnose fix-2 arm back on python, with the rocm diagnose fix-6 arm back on && hipcc --version, with -ErrorAction Stop removed from fix-21, with the bare-where match back on the un-lowered text, and with the Get-Item check disabled (each in both copies of the rule where the rule is involved), the named unit tests fail, and the shared-table test fails on each side; diagnose-44 fails on the missing Linux line. Earlier: with the fix-15 line back on $env:SystemRoot its tests fail.

The user-facing behavior change is in the plan rocm fix prints (for fix-2 and fix-6 a second labelled verify line; for fix-2, fix-6, fix-15 and fix-21 different verify text), and rocm diagnose output on Linux for fix-2 (python3, no python) and fix-6 (no hipcc). docs/testing.md describes the rule, the shared table and the two verify lines. There is no manual step to add: the automated tests above run the lines on Windows.

Test plan

• cargo fmt --check, workspace clippy -D warnings, cargo xtask check-crate-edges, cargo nextest run -p rocm-core and cargo nextest run -p rocm --bin rocm pass locally on Windows 11 with Windows PowerShell 5.1, where the Windows-only tests run (the single-process cargo test form that the Windows CI lane uses was not run for this update). cargo test -p rocm-core --lib also passes under WSL2 (that is where the Unix-only tests run locally), as do diagnose-42 to diagnose-45 there.
• cargo xtask e2e for diagnose-41, diagnose-43 to diagnose-45, examine-29 and driver-install-04 passes on Windows. diagnose-40 and diagnose-42 are Linux-only scenarios (diagnose-40 fails on a Windows host by design).
• On CI, windows-build-and-test runs the Windows-only tests above. It was green on the previous head of this branch; the new head's status is to be read after the push.
• python scripts/smoke_local.py and a full cargo test --workspace --all-targets were not run locally for this update.

Not verified

• fix-13 was run against a stand-in executable, not a real HIP SDK, and fix-14, fix-22 and fix-9 were not checked against a real torch (fix-14 and fix-22 do run against the local GPU adapter list).
• No test runs the fix-6 PATH write, because it changes the user's Path.
• The Linux verify lines are run in /bin/sh against stand-in executables, not against a ROCm install or a real torch: rocminfo and python3 are stubs.
• The plain fix-2 line still calls python: it is the line cmd and PowerShell run. A Windows machine with no Python install, or only the Microsoft Store stub, would fail it; the override matters only to a framework wheel, which normally has a Python. Other rocm diagnose verify lines (for example fix-1) also call python and were not changed here.
• fix-21 is checked on msvcp140.dll only; the text rule's Get-Item check is limited to Get-Item, not to other cmdlets that report a missing path without a terminating error.
• fix-15 looks only in the system directory, where the old where searched PATH. Format-Table output can wrap in a narrow console.

[juhovainio]

Reviewed this with a review-code fan-out pass plus a separate Standards/Spec pass. The core change (switching the MSVC-redist verify command to a PowerShell one-liner that prints version info, and the Linux fix-6-path verify string that dropped the &&-chained rocminfo call) is a reasonable fix for the PowerShell-hostility problem the new tests describe, and the new regression test deriving the Windows recipe set from applies_on so future Windows recipes get checked automatically is a nice touch.

Requesting changes because CI is currently red: the new printed_verify_line helper in the e2e step file trips clippy::map_unwrap_or, left as the first inline comment below. The rest are non-blocking gaps worth a look, also left inline - none of them undermine the approach, they're mostly about the new regression test's actual reach versus what the PR description implies it covers.

[juhovainio]

This trips clippy::map_unwrap_or - .map(str::trim).unwrap_or_else(|| panic!(...)) should be .map_or_else(|| panic!("the preview printed no Verify: line:\n{output}"), str::trim). This is what's making CI red right now.

[volen-silo]

Fixed earlier by using map_or_else. Clippy with that lint is clean on bb1df14.

[juhovainio]

This verify string is duplicated in fix.rs:382 (the RECIPES entry for the same fix-id), and nothing cross-checks the two the way diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids (diagnose.rs ~5542) already does for fix-15/fix-16. The two copies can drift silently.

[volen-silo]

The two fix-6 copies are now compared by the_path_fix_verify_matches_the_catalog_copy, and the existing shared-id test already compares the fix-15 and fix-16 copies. The Windows arm of fix-6 is intentionally different and is checked as a PowerShell command. See bb1df14.

[juhovainio]

This new test only covers RECIPES (the catalog in this file) - it doesn't touch diagnose.rs's hand-built Fix literals, which have their own Windows-specific verify strings (around lines ~727, ~985-998, ~1207, ~1429, ~1486, ~1523) that could just as easily regress with a && chain or bare where and wouldn't be caught here.

[volen-silo]

The diagnose-side sweep windows_diagnosis_remediation_avoids_and_chains_and_bare_where applies the same rule to the fixes the checkers build. It also asserts which fix ids it saw, so it cannot pass while silently skipping a checker.

[juhovainio]

fix-2-unset-override (fix.rs:307, not touched by this PR) has the same bug class this detector is meant to catch - it uses || plus Unix-only env/grep in a recipe that also applies on Windows - but it isn't flagged, since the detector only checks for && and bare where, not the broader "this won't run in PowerShell" question. Worth considering whether it belongs in scope here or as a fast follow.

[volen-silo]

Agreed, and it is now in this PR. The fix-2 verify line is one python -c line that bash, cmd and PowerShell run the same way. It prints the override's value, or OK_UNSET. The detector, now fails_in_windows_powershell, also flags || and %NAME% references, so the old fix-2 line and the fix-6 setx PATH "%PATH%;..." line both fail it. fix-6's Windows command now appends to the User Path through PowerShell. A Windows-only test runs the fix-2 line in cmd and PowerShell, with the override set and unset. Scenarios diagnose-39 and diagnose-40 check the printed lines of both fixes. See adc5b30.

[juhovainio]

Minor: this e2e assertion duplicates (and is weaker than) the Rust unit test's uses_and_chain_or_bare_where detector - it only checks the printed verify line for this one scenario rather than deriving the whole Windows recipe set. Not wrong, just redundant coverage that could drift out of sync with the unit test's rules over time. Also worth noting the new unit test's doc comment candidly says the verify command's real behavior was "checked by hand in powershell.exe; no test runs it" - only the printed text is pinned, not that it actually works in a shell.

[volen-silo]

The e2e bare-where check now uses the same split as the unit rule. The "by hand" wording is gone, and the scenario says it checks the printed text only. A Windows-only test runs the line in cmd and in PowerShell (fed on stdin, as if typed). It passes in the Windows CI job on bb1df14. See bb1df14.

[rominf]

🔴 Automated review · pr-review-watcher · 284ed3c

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.

First automated round on this PR: every finding below is new.

Review — needs work

Full review of the whole change.
No ticket named. The description matches the diff and openly says the new fix-15 line was never run on Windows.

Blocking

• The new verify command is only checked as text, so a broken command passes every test (AGENTS.md §3) — crates/rocm-core/src/fix.rs:573, crates/rocm-core/src/diagnose.rs:1590, crates/rocm-core/src/fix.rs:2597-2608, tests/e2e-cucumber/features/diagnose.feature:560-571, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:279-312
  • The rule. §3 says a remediation line naming a command must exist, accept those flags and do what it claims. A test that only pins the wording "certifies the string, not the truth of it." Text pinned on its own needs a comment naming the test that proves the behaviour.
  • What I ran. In both copies (catalog and checker) I changed (Get-Item $env:SystemRoot to (Get-Itm $env:SystemRot, giving a non-existent cmdlet and a wrong variable. All 164 fix::/diagnose:: tests still passed. The e2e steps check only starts_with("powershell -NoProfile -Command ") and contains("FileVersion"), so the mutant passes them too.
  • Titles that overclaim. The scenario title "…works in PowerShell and prints a version" and the step "prints the runtime version" describe behaviour nothing exercises.
  • Comment contradicts the commit. The feature comment says "That the line runs in a real shell is covered by hand". The commit message and PR body say "the new line was not run in Windows PowerShell 5.1 or in cmd". The commit governs, because it is the author's own account of what was done. So the comment claims coverage that does not exist.
  • Testing on Windows is practical. windows-build-and-test already runs cargo test on Windows, and hosted Windows runners have powershell.exe and System32\vcruntime140*.dll.
  • What I checked locally (pwsh 7.4 on Linux, not Windows PowerShell 5.1, with a fake SystemRoot):
    • the inner command parses and lists both files;
    • with one DLL missing, it errors for that file and still lists the other;
    • an outer PowerShell expands $env:SystemRoot before handing the string on, and the result is still a valid command.
  • Confidence 100 · logic · Fix: add a #[cfg(windows)] test that runs the fix-15 verify line through cmd /C and through powershell. Assert exit 0 and that both DLL names and a version appear in the output. Then either point the e2e assertions at that test in a comment, or reword the scenario and step titles to say the text is checked. Remove or correct "covered by hand".

Non-blocking

• The new "safe in PowerShell" tests pass a fix-6 command that corrupts the user PATH in PowerShell (pre-existing) — crates/rocm-core/src/fix.rs:376, crates/rocm-core/src/diagnose.rs:1043
  • The premise. Both new sweep tests say Windows commands "are shown to users of Windows PowerShell 5.1 as well as cmd".
  • The command. fix-6 prints setx PATH "%PATH%;<bin>". PowerShell does not expand %PATH%. In pwsh I confirmed the argument arrives as the literal %PATH%;C:\Program Files\AMD\ROCm\6.4\bin, so running it writes a literal %PATH% into the user Path.
  • Why it matters here. The bug predates this PR. But uses_and_chain_or_bare_where passes this line, and the PR's framing makes this class of problem look handled.
  • Confidence 85 · logic · Fix: give the Windows line a form that works in both shells, or label it cmd-only. Consider adding %…% expansion to the detector.
• The new e2e bare-where check is weaker than the unit rule it mirrors — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:290-293
  • It checks only the first whitespace token. uses_and_chain_or_bare_where (fix.rs:1066-1074) also splits on | and ;, so x | where foo passes the scenario but fails the unit test.
  • Confidence 70 · mechanical · Fix: split on |/; the same way.

Decisions for the author

• fix-6 now verifies only hipcc --version, while its Linux trigger is a missing rocminfo — tradeoff
  • For the change. One catalog line has to hold for Linux, Windows and WSL. The old line ran rocminfo, which the Windows HIP SDK does not ship, and used &&.
  • Against.
    • The Linux arm of check_6_path_missing scores on !e.rocminfo_present (diagnose.rs:1023-1027), and the rationale names rocminfo. Now nothing re-checks that symptom.
    • A distro hipcc in /usr/bin passes the verify while /opt/rocm/bin is still off PATH.
    • The new the_path_fix_verify_matches_the_catalog_copy forces the Linux-only checker arm to match the catalog. That rules out keeping rocminfo there, even though that arm never prints on Windows.
  • Confirm this was deliberate.
• where.exe vcruntime140.dll vcruntime140_1.dll would be a simpler fix-15 verify — non-blocking-improvement
  • For it. It is valid in cmd and PowerShell and needs no nested quoting. It also searches PATH, which is what the detector (examine.rs) and the "not resolvable on PATH" evidence describe.
  • For the current line. It reports the file version, which helps tell this fix apart from fix-21 (outdated runtime). But it looks only in System32, and a 32-bit host shell would be redirected to SysWOW64.

Positive signals

• windows_recipes_avoid_and_chains_and_bare_where takes its recipe set from applies_on instead of a hand-kept list, so a new Windows recipe is covered automatically. I confirmed it catches the reverted fix-6 and fix-15 lines; for the fix-15 revert, two more tests fail as well.
• The diagnose-side Windows sweep checks which fix ids it actually saw, so it cannot pass while silently waking no checkers.

Deployment notes

None

What this covered

• Read: all four changed files, against prw-base 21be003…284ed3c8a9de, with the surrounding code:
  • check_6_path_missing and check_15_msvc_redist;
  • print_recipe, plan_of and act_on;
  • the existing shared-id catalog guard in diagnose.rs;
  • the consumer apps/rocm/src/remote/doctor.rs (aim_at).
• Run:
  • cargo test -p rocm-core --lib -- fix:: diagnose:: (164 passed; all 5 new tests ran by name);
  • three mutation runs: fix-6 reverted, fix-15 reverted, fix-15 broken;
  • pwsh 7.4 (Linux) runs of the fix-15 line and the fix-6 setx line.
• Read but not run: the e2e scenario. CI for this exact head has every check green, including E2E tests and windows-build-and-test. Skillscope and Sphinx were skipped.
• Fan-out:
  • One read-only worker covered history, impact, agent-instruction adherence and code comments. It found no stale copies of the old verify strings in docs, README, skills or fixtures, no diagnose-38 or @id collision, and no feature_naming problem.
  • I ran the code pass and the Step 7 design questions myself, in one context, because the change is small (4 files). They were not independent workers.
  • The prior-changes pass did not run, because reading earlier PR review comments is out of bounds for this run.
• Order of reading: the commit message body appeared in the first git log output, so the stated intent was in view before the findings were settled, not only at Step 9.
• Discussion: not reconciled against any PR discussion here.

[volen-silo]

On the two decisions:

• fix-6 verify is hipcc --version on purpose. One catalog line has to hold on Linux, Windows and WSL. rocminfo is not in the Windows HIP SDK, and hipcc is in every one of them. The Linux checker still scores on a missing rocminfo, so that symptom is still detected. Only the verify line moved. A distro hipcc in /usr/bin can pass the verify while /opt/rocm/bin is off PATH. That gap is real, but small, and it is not new to this PR.
• fix-15 keeps the Get-Item ... VersionInfo line, not where.exe. The version is what tells this fix apart from fix-21 (runtime present but too old), and where.exe cannot show it. The line looks only in System32, which is where the redistributable installs the 64-bit DLLs.

[rominf]

🔴 Automated review · pr-review-watcher · b34a052

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.

Second automated round. The previous round's commit (284ed3c) was force-pushed away, so this is a full review of the whole change. Every finding below is new.

Standing objection from round 1: settled, and the change request is being withdrawn. It said the fix-15 verify line was only checked as text. This round's blind pass did not raise it again. It was then put to the reviewer together with the author's reply. The reviewer's answer is anchored evidence (it was told what to look for):

• the_msvc_verify_runs_in_cmd_and_powershell runs the line through cmd and through PowerShell. It asserts success, empty stderr and a version. The Windows CI log for b34a052 shows it passed.
• the_msvc_verify_is_a_wellformed_powershell_command runs on every host and catches a misspelled cmdlet or variable.
• The shared-id guard catches a misspelling in the diagnose copy.
• The "covered by hand" wording is gone.
• Still open, minor: the Windows test counts the vcruntime140 stem twice instead of checking both file names.

The findings listed under Blocking below are not filed as a change request. That gate is spent only where merging would leave a defect behind. Here:

• GitHub already blocks the merge on the conflict, and the rebase will be reviewed again.
• The fix-6 and fix-13 Windows verify lines that break in PowerShell are unchanged from main. What this PR adds is test wording that claims too much.
• The new apostrophe escaping is correct, and the rebase onto #710's shared helper is expected to replace it.
• The scenario numbering is wrong in the description only.

Review — needs work

Full review of the whole change.
No ticket named. The description describes the change accurately apart from its scenario numbering and its "rebased on main" claim (see Blocking).

Blocking

• The PR conflicts with main, and main already rewrote the same fix-6 lines a different way — crates/rocm-core/src/fix.rs:384-394, crates/rocm-core/src/diagnose.rs:1042-1060

  • GitHub reports mergeable_state: dirty. git merge-tree --write-tree prw-base HEAD reports content conflicts in both files.
  • The cause is 0acf698 (#710, "stop advertising setx for the Windows PATH write"), which landed after this PR's merge base. In the catalog and in check_6_path_missing, #710 prints a bare PowerShell [Environment]::SetEnvironmentVariable('PATH', …) line.
  • #710 also adds two Windows notes to the recipe, one of them the reopen-the-shell advice, and makes the diagnose copy reuse crate::fix::ps_single_quote_escape.
  • This PR instead wraps the line in powershell -NoProfile -Command "…", adds its own # reopen line, and hand-copies the escape as bin_dir.replace('\'', "''").
  • The two designs cannot simply be merged. The rebase has to choose one, and keep one escape helper and one reopen hint.
  • The description says the work was "Rebased on main after #691" and that cargo xtask e2e passed "on the rebased head". Neither is true of the current base.
  • Confidence 100 · architectural · Fix: rebase onto current main. Keep ps_single_quote_escape rather than the inline copy. Decide between the bare PowerShell line and the powershell -Command wrapper (see Decisions).

• The new tests and scenario titles claim the Windows lines run in PowerShell, but the check only rejects a few known-bad patterns, and a line it vouches for is broken in PowerShell — crates/rocm-core/src/diagnose.rs:1058-1060 (the fix-6 Windows-arm verify), diagnose.rs tests the_path_fix_verify_matches_the_catalog_copy and windows_diagnosis_remediation_passes_the_powershell_text_rule, tests/e2e-cucumber/features/diagnose.feature diagnose-41/42/43 titles

  • What the tests claim. The sweep's doc says "a hand-built verify string or command cannot regress unseen". The path test says the Windows arm "is checked for being a PowerShell command instead".
  • What the check actually does. fails_in_windows_powershell rejects only &&, ||, a bare where and %NAME%. The path test asserts only the powershell -NoProfile -Command prefix.
  • The broken line. The fix-6 Windows verify is powershell -NoProfile -Command "& \"{bin_dir}\hipInfo.exe\" | Select-Object -First 5". In PowerShell, a backslash does not escape a quote.
  • Evidence. I tokenised that line with the PowerShell parser in pwsh 7.4, using bin_dir = C:\Program Files\AMD\ROCm\6.4\bin. It gives 6 command elements instead of 4: "& \", C:\Program, Files\…\hipInfo.exe\" | Select-Object -First 5". A child process that echoes its argv received '& \', 'C:\Program' and 'Files\...\hipInfo.exe\ | Select-Object -First 5', so the inner command is cut apart. This was not run on real Windows PowerShell 5.1.
  • Other lines, by reasoning only. These also pass the rule but look PowerShell-hostile; none was run:
    • fix-13 verify: \" plus $env:HIP_PATH, at fix.rs:542 and diagnose.rs:1535.
    • fix-14 verify: $_, which the outer shell expands, at fix.rs:564 and diagnose.rs:1592.
    • fix-2 Windows command: setx HSA_OVERRIDE_GFX_VERSION "", where PowerShell 5.1 drops the empty argument.
  • Why it matters. The line itself predates this PR. What the PR adds is tests whose names and comments say the line is covered, and a check that claims coverage it lacks stops the next person looking.
  • Confidence 85 · logic · Fix: fix the fix-6 Windows verify quoting (for example, single quotes inside the body: & '{bin_dir}\hipInfo.exe'). Then either extend the structural check that fix-15 has to every powershell -Command line, or reword the test docs and scenario titles to "avoids these known-bad constructs".

• No test covers the new apostrophe escaping in the diagnose fix-6 command — crates/rocm-core/src/diagnose.rs:1049-1052

  • I removed .replace('\'', "''") and passed bin_dir raw. All 750 rocm-core lib tests still pass, because no test uses a bin_dir containing '.
  • Rebasing onto #710's shared ps_single_quote_escape would remove this finding along with the copy.
  • Confidence 100 · mechanical · Fix: reuse crate::fix::ps_single_quote_escape, or add a case with C:\Users\O'Neil\….

• The description gives the wrong scenario numbers — PR description, "Tests" section

  • The description lists the scenarios as diagnose-40, diagnose-41 and diagnose-42, with @ids mapped to those numbers, and the Notes say they "moved to 40 to 42".
  • The feature file has diagnose-41, diagnose-42 and diagnose-43; the code governs. The commit message has the right numbers.
  • Confidence 100 · mechanical · Fix: correct the description. The rebase in the first finding may shift the numbers again.

Non-blocking

• The fix-2 verify gives a false "still set" in the shell where the fix was applied on Windows — crates/rocm-core/src/fix.rs:303-317

  • The new line reads the current process environment. The Windows step is setx, which only affects new shells, so the same shell still prints the old value after a correct fix.
  • The catalog recipe has no reopen hint before Verify:. The diagnose Windows arm sidesteps this by reading the User scope (diagnose.rs:794).
  • Confidence 70 · logic · Fix: add a "reopen the terminal before verifying" comment line to the Windows block of the recipe.

• The e2e step is a third hand-written copy of the PowerShell rule — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:333-363

  • It re-implements fails_in_windows_powershell, which is #[cfg(test)] in rocm-core and so cannot be called from the e2e crate. The two copies can drift silently; its comment, "The same rule as …", is the only link.
  • Confidence 70 · mechanical · Fix: expose the rule from a non-test module (or a test-support feature) and call it from the step.

• The fix-2 catalog verify and its diagnose Linux copy now say different things, and nothing compares them

  • The diagnose Linux arm (diagnose.rs:805) still prints env | grep … || echo OK_UNSET; python -c "import torch…". The catalog prints the new python one-liner.
  • The shared-id guard does not cover fix-2. This is pre-existing drift that the change widens. It is valid in bash, so it is not a PowerShell defect.
  • Confidence 70 · mechanical · Fix: align the two, or add fix-2 to the guard.

Decisions for the author

• Bare PowerShell line (#710, on main) or a powershell -NoProfile -Command "…" wrapper (this PR) — tradeoff
  • The wrapper runs from cmd as well as PowerShell, but adds an outer layer of quoting and $ expansion (the second Blocking finding shows that layer going wrong).
  • The bare line is simpler and matches what the runner executes, but it only works in PowerShell. The rebase forces this choice; make it explicitly.
• No test runs the fix-6 Windows PATH command — answered-by-intent
  • Description: "No test runs the fix-6 Windows command, because it changes the user's Path."
  • Otherwise this would be blocking under AGENTS.md §3: remediation that names a command must be shown to actually clear the condition.
  • A Windows-only test could save and restore the User Path, or run the same expression against a throwaway variable name. Confirm that leaving it unrun is deliberate.
• The fix-2 verify calls python, not python3 — answered-by-intent
  • Description: "Ubuntu ships only python3, but the override matters only to a framework wheel, which normally runs in a virtual environment that has python."
  • Otherwise this would be a non-blocking regression: the old line worked in any Linux bash. Confirm.

Positive signals

• The catalog sweep takes its recipes from applies_on, so it is not a hand-kept list. Mutation checks confirmed that reverting each old line, in the catalog or a diagnose checker, turns a unit test red. Reverting the fix-15 and fix-6 catalog lines also turns diagnose-41 and diagnose-43 red.
• The Windows-only tests feed the line to PowerShell on stdin "as if typed". The comment explains why passing it as an argument would test something else.

Deployment notes

None

What this covered

• What was read: all four changed files in full, f0d6ce3…b34a052c (merge base…head). I also read the surrounding checker and runner code in fix.rs and diagnose.rs, the e2e "previews that fix" step, feature_naming.rs, docs and skills searched for the old lines, and the conflicting main commit 0acf698.
• Fan-out: two workers (one running tests and mutations, one read-only source passes) plus my own pass. I investigated the design questions myself rather than in separate workers, because of the subagent budget.
• Commands run:
  • Unit tests: cargo test -p rocm-core --lib, 750 passed. The 6 new tests are among them.
  • Mutation checks a–g on both rule sweeps and the escape.
  • e2e diagnose-41/42/43: 3 scenarios passed. Two mutations turned the matching scenario red.
  • cargo clippy -D warnings for rocm-core and for --test e2e: clean.
  • PowerShell parser tokenisation in pwsh 7.4.
• What did not run, and why:
  • The two #[cfg(windows)] tests were typechecked for the Windows target but not executed. The full Windows cross-check failed for lack of a mingw C compiler (ring).
  • Nothing was run in real Windows PowerShell 5.1 or cmd.
  • Prior-changes pass (review comments on earlier PRs touching these files): not run, for lack of time.
• CI on b34a052 is all green or skipped. Skillscope and Sphinx were skipped.
• Discussion: I did not reconcile this report against the PR's discussion.

The fix-15 verify line is now run through cmd and PowerShell by the_msvc_verify_runs_in_cmd_and_powershell, which passed on Windows CI at b34a052. A misspelled cmdlet or variable now fails tests on Linux in both copies. See review 5599812298.

Addressed in d39aeaa: the clippy failure in the e2e step is gone, and the other points are covered by the shared-id check, the sweep over the diagnose checkers and the fix-2 changes. The remaining red e2e job fails the same four scenarios on main.

[rominf]

🔴 Automated review · pr-review-watcher · d39aeaa

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.

Third automated round. The previous round's commit (b34a052) was force-pushed away, so this is a full review of the whole change, and every finding below is new. This account holds no change request on this PR, and this round files none.

The previous round's four blocking-rated items were not re-found by this blind full review. Checked directly against the current head: GitHub no longer reports a conflict, the fix-6 Windows verify now uses single quotes and the shared ps_single_quote_escape, the inline escape copy is gone, and the description now numbers the scenarios diagnose-41..43.

The finding under Blocking below is not filed as a change request. Merging leaves no broken line behind: the fix-9 line itself is correct, and the description now says the rule is a text check over known-bad constructs. What merging leaves is one missing regression guard and one overclaiming test comment. That is the same shape the previous round reported without gating.

Review — needs work

Full review of the whole change.
No ticket named. The description claims every changed Windows line now runs in cmd and Windows PowerShell 5.1, and that the new tests guard against regressions. One defect the description says it fixed is not covered by the rule that is supposed to catch it.

Blocking

• New: the PowerShell text rule does not catch the $env:NAME= defect this PR names as fixed in fix-9. The diagnose sweep says hand-built lines "cannot regress unseen", and for this defect they can. — crates/rocm-core/src/fix.rs (fails_in_windows_powershell), crates/rocm-core/src/diagnose.rs (windows_diagnosis_remediation_passes_the_powershell_text_rule, check_9_igpu_dgpu_collision), tests/e2e-cucumber/tests/e2e/diagnose_steps.rs (the hand copy of the rule)
  • The description lists $env:NAME=... beside $_ as text "expanded by the outer shell", and says fix-9 was changed to avoid it. The rule only flags $_ inside a -Command "..." body.
  • Changing check_9's verify to powershell -NoProfile -Command "$env:HIP_VISIBLE_DEVICES=1; python -c 'import torch; ...'" left all 12 targeted rocm-core tests green, including the Windows sweep.
  • In pwsh, Write-Output "$env:HIP_VISIBLE_DEVICES=1; python -c x" prints =1; python -c x. Pasted into PowerShell, the inner shell therefore runs =1, which errors, and the variable is never set.
  • The old fix-9 line was flagged only because it also contained \". Nothing guards the $env: half of the fix.
  • Confidence 100 · logic · Fix: inside a -Command " body, flag $env:<NAME>=. Better still, flag any $ except an allow-listed read such as $env:SystemRoot. Add the old fix-9 shape to the must-flag list, and update the e2e copy to match.

Non-blocking

• New: the fix-2 Windows command setx HSA_OVERRIDE_GFX_VERSION "" still fails when pasted into Windows PowerShell 5.1, and the new rule passes it. — crates/rocm-core/src/fix.rs (fix-2 commands), crates/rocm-core/src/diagnose.rs (check_2_hsa_override_unneeded, Windows arm)
  • PowerShell before 7.3 drops empty-string arguments to native programs. In pwsh with $PSNativeCommandArgumentPassing='Legacy', python3 -c 'import sys;print(sys.argv[1:])' X "" printed ['X'].
  • So setx receives one argument, which is invalid syntax for it. That setx error comes from its documented behaviour; it was not run here.
  • This predates the PR. But the PR adds a note right below this line, and its summary says "This change makes each printed line valid in cmd and in Windows PowerShell".
  • Confidence 70 · logic · Fix: use [Environment]::SetEnvironmentVariable('HSA_OVERRIDE_GFX_VERSION', $null, 'User'), or narrow the summary's "each printed line" claim.
• New: test comments that the code contradicts.
  • fix.rs, doc on windows_recipes_pass_the_powershell_text_rule: "Only the_msvc_verify_runs_in_cmd_and_powershell runs a verify command in a shell, and only fix-15's". the_override_verify_runs_in_cmd_and_powershell also runs one, for fix-2.
  • diagnose.rs, the comment above fix-2's Linux verify: "the shared-id guard test keeps them equal". The shared-id guard, diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids, covers fix-15, fix-16, fix-21 and info-20, not fix-2. What actually keeps fix-2 equal is the new the_override_fix_verify_matches_the_catalog_copy.
  • diagnose.rs, doc on the_path_fix_verify_matches_the_catalog_copy: says the shared-id guard covers "fix-15 and fix-16". It also covers fix-21 and info-20.
  • The code governs in each case: these are the tests that actually exist and the ids they actually check.
  • Confidence 85 · mechanical · Fix: correct the comments.
• New: the Windows diagnose sweep claims to run "every checker that answers on Windows", but never reaches fix-21, and its guard list cannot notice a missing checker. — crates/rocm-core/src/diagnose.rs (windows_diagnosis_remediation_passes_the_powershell_text_rule)
  • Instrumented, it saw fix-1, 2, 6, 8, 9, 13, 14 and 15. check_21_msvc_redist_outdated never fires, because the test leaves msvc_redist_version unset.
  • The guard asserts only six ids, so fix-1 and fix-8 could also drop out silently.
  • fix-21's line is still covered through the catalog test plus the shared-id guard, so the risk is low.
  • Confidence 85 · mechanical · Fix: set msvc_redist_version to an old version, and assert every id the sweep is expected to see.
• New: the fix-13 and fix-14 verify lines were edited in lockstep in two places with no equality guard. — fix.rs RECIPES fix-13 and fix-14; diagnose.rs check_13_hip_sdk_missing and check_14_adrenalin_too_old
  • They are equal today. The PR adds equality guards for fix-6 and fix-2 only, so these two pairs can drift unseen.
  • Confidence 70 · mechanical · Fix: add them to the shared-id guard, or extend the new guard tests.
• New: a stale copy of the fix-6 output remains in the e2e crate. — tests/e2e-cucumber/src/lib.rs:183 (PATH_FIX_CATALOG_BLOCK)
  • It still reads Verify: rocminfo | head -n 5 && hipcc --version. The code now prints hipcc --version, and the code governs, since the sample claims to reproduce rocm fix fix-6-path output.
  • Nothing fails, because the sample is abridged and frozen.
  • Confidence 70 · mechanical · Fix: update the line to hipcc --version.

Decisions for the author

• fix-6's verify dropped rocminfo, so the diagnose Linux arm can equal the cross-OS catalog line — tradeoff
  • For: one string valid in every shell, guarded by the_path_fix_verify_matches_the_catalog_copy.
  • Against: check_6 reports "rocminfo is not on PATH" but no longer verifies rocminfo. On Windows, the catalog now prints hipcc --version while the notes talk about hipInfo.
• Only fix-15 and fix-2 are actually executed; fix-6 (Windows), fix-9, fix-13 and fix-14 are text-only checks — non-blocking-improvement
  • AGENTS.md §3: "A test that only pins the wording certifies the string, not the truth of it."
  • fix-13 and fix-9 need an SDK or GPU. But fix-14 (Get-CimInstance Win32_VideoController | Where-Object Name -match ...) and the fix-6 Windows form could run on the Windows CI lane the way fix-15 does.
  • The new scenarios diagnose-41..43 are text checks, and say so honestly.
• The fix-2 verify now needs python, which stock Ubuntu does not provide — answered-by-intent
  • Description: "The fix-2 line calls python. Ubuntu ships only python3, but the override matters only to a framework wheel, which normally runs in a virtual environment that has python. The existing fix-1 verify line makes the same assumption."
  • It would otherwise have been a non-blocking finding: the old Linux line needed no Python, and the diagnose Linux arm also lost its torch.cuda.is_available() check. Confirm that trade is intended.

Positive signals

• The catalog sweep derives its recipe set from applies_on instead of a hand list, so a new Windows recipe is covered without editing the test.
• The fix-6 Windows verify uses the same ps_single_quote_escape as the PATH write, with an apostrophe test (O'Neil) that pins both.
• The Windows-only run tests feed the line on stdin (-Command -), so it is parsed as typed text rather than through argument re-joining.

Deployment notes

None

What this covered

• What was read: all four changed files' hunks in full, against 32834b29…d39aeaa4. That included the surrounding checkers, the RECIPES entries, ps_single_quote_escape, the shared-id guard, the rocm fix --dry-run print path, the e2e step definitions and tests/e2e-cucumber/src/lib.rs.
• Fan-out: two read-only workers did the passes, one on the code and one on agent-instruction adherence, history, code comments and CI. Fan-out was not degraded.
• What was run:
  • Unit tests: targeted rocm-core tests passed, along with one-at-a-time mutations of the verify lines and the rule.
  • Naming checks: feature_naming and skill_reference passed.
  • New scenarios: cargo xtask e2e -- -n 'diagnose-4[123] ' passed 3/3 locally.
  • PowerShell: the new lines were parsed and executed in pwsh 7, with Legacy argument passing standing in for Windows PowerShell 5.1.
• What did not run:
  • Windows PowerShell 5.1 itself and cmd; there are no Windows hosts here. The Windows-only tests passed on the PR's Windows CI lane.
  • A real setx call.
  • The prior-changes pass: earlier PR reviews were deliberately not read.
  • ai-instruction-quality: no instruction files are in scope.
• CI: the only failure is E2E tests, on two dash scenarios (dash-gen-tps-expiry-boundary, dash-esc-menu-quit-row-confirms-when-serving). The same job fails on main's recent runs, including the base commit, and diagnose-41..43 passed in the PR run, so this PR did not cause it. Skill checks (skillscope) and Sphinx docs build were skipped by path filters.
• Ordering note: the head commit message, whose text largely matches the description, was read before the findings were settled, not only at Step 9. No finding was dropped on its strength.
• Discussion: not reconciled against the PR's discussion here.

[juhovainio]

Nice cleanup overall — the rewritten verify lines read clearly and most of the previous review rounds' blocking issues (clippy, the fix-6 quoting bug, the apostrophe-escaping test, the scenario numbering) are genuinely resolved here.

One gap before this lands: fix-15 and fix-2 each got a #[cfg(windows)] execution test that actually spawns cmd/powershell and checks the output, but fix-9, fix-13 and fix-14's rewritten PowerShell expressions only get the coarse fails_in_windows_powershell text-pattern check, which can't catch a wrong variable name or a structurally broken expression. Given the pattern already exists for 2 of 5 lines, extending it to the other 3 seems like a small, mechanical addition rather than a redesign. Left inline comments on each.

Also flagged a non-blocking weak assertion in the existing vcruntime140 execution test. CI's one failing check (E2E Linux lane, 2 dash-* scenarios) looks unrelated to this diff — no TUI code touched here.

[juhovainio]

fix-9's rewritten verify ([Environment]::SetEnvironmentVariable(...)) only has the text-pattern check (fails_in_windows_powershell) going for it — no #[cfg(windows)] execution test like the_override_verify_runs_in_cmd_and_powershell runs for fix-2. A wrong variable name or malformed expression here would pass every existing test silently.

[juhovainio]

Same gap as the other two: this Join-Path / GetEnvironmentVariable rewrite (mirrored in diagnose.rs around line 1542) has no execution test, only the text-pattern rule. Worth adding a #[cfg(windows)] test alongside the_msvc_verify_runs_in_cmd_and_powershell / the_override_verify_runs_in_cmd_and_powershell.

[juhovainio]

Same as fix-13/fix-9: this Where-Object Name -match rewrite (mirrored in diagnose.rs around line 1599) isn't covered by an execution test, only the text-pattern rule.

[juhovainio]

Non-blocking: this asserts text.matches("vcruntime140").count() >= 2, a stem count rather than checking both distinct filenames (vcruntime140.dll and vcruntime140_1.dll) actually appear in the captured output. A command that printed one DLL's info twice while dropping the other would still pass.

Addressed in 1d96e5d: execution tests for the fix-9, fix-13 and fix-14 lines, and the vcruntime140 test now checks both file names. All of them pass in windows-build-and-test. The remaining red e2e job is the quit-timeout failure that #719 fixes; it fails the same way on main.

[rominf]

🔴 Automated review · pr-review-watcher · 1d96e5d

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.

Fourth automated round. The previous round's commit (d39aeaa) was force-pushed away, so this is a full review of the whole change. Each finding is marked new or standing (raised in an earlier round). This account holds no change request on this PR, and this round files none.

Standing objection from the previous round (the $env:NAME= half of fix-9 was unguarded): settled, mostly no longer holds. Put to the reviewer after its blind report was frozen (anchored evidence): with check_9's Windows verify mutated back to $env:HIP_VISIBLE_DEVICES=1; python -c ..., the_windows_igpu_verify_is_one_quoted_powershell_argument fails on Linux (18 ran, 1 failed), and the Windows-only the_igpu_verify_pins_the_variable_it_names should fail too (reasoned, not run). What still holds: the doc comment on windows_diagnosis_remediation_passes_the_powershell_text_rule says a hand-built line "cannot regress unseen", but that sweep itself does not catch this regression; the per-line shape test does. That is a comment overstating its test, and the rule's narrowness is reported below as non-blocking.

The finding under Blocking is not filed as a change request. The previous round reported the same setx line as non-blocking, and nothing new makes it worse. The overclaim is in the PR description; the commit message claims only that the new verify lines are valid in both shells, which holds. Fixing the line or narrowing the description resolves it.

Review — needs work

Full review of the whole change.
No ticket named. The description says the change "makes each printed line valid in cmd and in Windows PowerShell". That claim does not hold for one Windows command in the same fix it edits; see the Blocking finding.

Blocking

• Standing: the description says every printed Windows line now runs in Windows PowerShell 5.1, but the fix-2 setx line still does not — crates/rocm-core/src/fix.rs:309 (RECIPES fix-2 commands), crates/rocm-core/src/diagnose.rs:789 (Windows arm of check_2_hsa_override_unneeded)
  • The PR summary says: "This change makes each printed line valid in cmd and in Windows PowerShell."
  • Both copies still print setx HSA_OVERRIDE_GFX_VERSION "". Windows PowerShell 5.1 drops an empty-string argument when it calls a native program. setx then receives only the variable name and fails with a syntax error.
  • I reproduced the argument drop with pwsh in Legacy argument mode, $PSNativeCommandArgumentPassing='Legacy'. python3 -c "import sys;print(sys.argv)" A "" printed ['-c', 'A']. In Standard mode it printed ['-c', 'A', ''].
  • In the diagnose Windows arm, the lines around it are [Environment]::... calls, which only run in PowerShell. So the user is certainly typing into PowerShell when they reach the setx line.
  • This change adds a note directly under that line in the catalog ("reopen this terminal before running the verify command"). Its new windows_recipes_pass_the_powershell_text_rule and windows_diagnosis_remediation_passes_the_powershell_text_rule both sweep this line and pass it.
  • Confidence 85 · logic.
  • Fix: either replace the line with something that works in both shells, or narrow the description to "the verify lines". For example, reg delete HKCU\Environment /v HSA_OVERRIDE_GFX_VERSION /f runs in both shells; in the PowerShell-only diagnose block, [Environment]::SetEnvironmentVariable('HSA_OVERRIDE_GFX_VERSION', $null, 'User') also works.

Non-blocking

• Standing: the PowerShell text rule is narrower than its doc comment says — crates/rocm-core/src/fix.rs (fails_in_windows_powershell and its doc comment); the same gap is in the e2e copy at tests/e2e-cucumber/tests/e2e/diagnose_steps.rs (assert_printed_lines_avoid_powershell_hostile_syntax)
  • Inside a -Command "…" body the rule only flags $_. An outer PowerShell expands every $name in a double-quoted string, not just $_. The rule therefore passes $env:X=1; …, which is the old fix-9 line without its \", and an unquoted $env:HIP_PATH path containing a space. Avoiding that second case is the reason fix-13 now uses Join-Path.
  • The doc comment also says these lines are "shown to users of Windows PowerShell 5.1 as well as cmd", but nothing in the rule checks cmd. The rule passes [Environment]::… lines that cmd cannot run.
  • Each of today's lines is also pinned by its own shape test, so this only weakens protection for future lines.
  • Confidence 85 · mechanical.
  • Fix: flag any $ inside a -Command "…" body unless it is on an allowlist (today's lines need $env:SystemRoot), or reword the doc comment to say what the rule actually checks.
• Standing: two comments name the wrong guard test — crates/rocm-core/src/diagnose.rs:805, and the doc comment on the_path_fix_verify_matches_the_catalog_copy
  • The comment at line 805 says "the shared-id guard test keeps them equal" about fix-2. That guard, diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids, does not include fix-2. The test that does is the_override_fix_verify_matches_the_catalog_copy, which is also what the PR description says.
  • The other doc comment says the shared-id guard "covers fix-15 and fix-16". This same change extends that guard to fix-13 and fix-14.
  • The code is right in both places; the comments are what is wrong.
  • Confidence 100 · mechanical.
  • Fix: name the right test, and list the guard's current fix ids.
• Standing: a test fixture still carries the old fix-6 verify line — tests/e2e-cucumber/src/lib.rs:183
  • PATH_FIX_CATALOG_BLOCK says it reproduces the catalog as print_recipe prints it, but it still contains Verify: rocminfo | head -n 5 && hipcc --version.
  • The current catalog line is hipcc --version, and that line governs because it is what the CLI prints.
  • Nothing fails today, because the tests that use the fixture only read the PATH-write region.
  • Confidence 100 · mechanical.
  • Fix: update the fixture's Verify line.
• Standing: the two new fix-6 verify lines are the only changed lines that no test runs — crates/rocm-core/src/diagnose.rs:1066 (Windows & '<bin>\hipInfo.exe' | Select-Object -First 5), crates/rocm-core/src/fix.rs:393 (hipcc --version)
  • Every other rewritten line has a Windows-only test that runs it in cmd and PowerShell. The fix-6 lines are covered only by the text rule and the apostrophe-doubling string check.
  • I ran the Windows line through pwsh in Legacy mode against a stub hipInfo.exe under a folder named O'Neil dir. The stub ran and the command exited 0, so the line works. The gap is only that no test runs it.
  • AGENTS.md §3 asks that a printed command be proven by a test, not only by its wording.
  • A related point that predates this change: the Windows verify runs hipInfo.exe by its absolute path. It therefore passes whether or not the PATH fix took effect.
  • Confidence 75 · logic.
  • Fix: add a Windows test like the_hip_sdk_verify_runs_the_exe_under_hip_path that runs the fix-6 diagnose verify against a stand-in exe in a folder containing '.
• New: the fix-15 verify exits 0 when one DLL is missing — crates/rocm-core/src/fix.rs:592, crates/rocm-core/src/diagnose.rs:1636
  • A fan-out worker ran the line in pwsh 7 with one DLL absent. Get-Item wrote an error to stderr, listed the DLL it did find, and the process exited 0.
  • The old where a && where b exited non-zero in that case.
  • The line is read by a person, who will see the error, so this is a small loss of signal.
  • Confidence 85 (run on pwsh 7, not 5.1) · logic.
  • Fix: optional — append -ErrorAction Stop to Get-Item.

Decisions for the author

• The shared verify lines are shell-neutral now, and they check less on Linux — tradeoff
  • To work in bash, cmd and PowerShell, the Linux verify lines lost checks they used to make:
    • fix-2's diagnose line no longer runs torch.cuda.is_available(), which tested whether removing the override fixed anything.
    • fix-6 dropped rocminfo, even though check 6 scores on rocminfo being absent from PATH.
  • hipcc is not present on runtime-only ROCm installs, so the new fix-6 verify can fail after a correct PATH fix.
  • The other side: one line shared by both copies means one string to keep valid in all three shells.
• The fix-2 verify needs a python binary on Linux — answered-by-intent
  • The description says: "The fix-2 line calls python. Ubuntu ships only python3, but the override matters only to a framework wheel, which normally runs in a virtual environment that has python. The existing fix-1 verify line makes the same assumption."
  • Without that passage this would have been a non-blocking finding: the old env | grep line worked on any Linux host. Confirm this is still what you intend.
• The e2e step keeps a hand-made copy of the text rule — answered-by-intent
  • The description says: "The e2e step keeps its own copy of the rule, because the e2e crate does not depend on rocm-core. The two must stay in step."
  • Without that passage this would have been a non-blocking finding. Nothing enforces that the copies match, and twin copies that drift apart are a recurring source of defects in this repo.

Positive signals

• The Windows-only tests type each line into cmd and into PowerShell on stdin, and use stand-ins (a copied whoami.exe as hipInfo.exe, a fake torch module that returns HIP_VISIBLE_DEVICES). They prove the line ran, not just that it parsed.
• windows_recipes_pass_the_powershell_text_rule works out its set of recipes from applies_on, so a new Windows recipe is swept without anyone editing the test.

Deployment notes

None

What this covered

I read every changed file in full, prw-base…1d96e5d7 (merge-base ea1d8713). That covered fix.rs, diagnose.rs, diagnose.feature and diagnose_steps.rs, plus the touched checkers, print_recipe and the dry-run path, the fix-2 runners, the CHECKERS table, sibling copies of each changed string across the repo, and docs and skills for stale references.

Two read-only workers ran: one for the code pass over the coupled group, one for the agent-instruction, history and code-comment passes plus impact analysis. The prior-changes pass did not run, because reading earlier review comments is outside this run's remit.

The Step 7 design questions were investigated in the driver context, not by independent workers, because the run was capped at 2 workers.

Mutation checks:

• Each Linux-runnable new test fails when the production lines are reverted, except the two drift guards (the_path_fix_verify_matches_the_catalog_copy and the extended shared-id guard), which pass on revert by design.
• Scenarios diagnose-41, 42 and 43 fail on revert and pass at the head.

What could not run here:

• The #[cfg(windows)] tests, cmd, and real Windows PowerShell 5.1 are not available in this environment. CI's windows-build-and-test lane did run the five Windows-only tests, and they passed.

CI:

• E2E tests is red. The failures are serve-01, serve-02 and a dashboard quit step, and main fails the same way at the base SHA (run 39793631227), so this change did not cause them. diagnose-41 to 43 passed in that same run.
• All other checks are green or skipped.

This report was not reconciled against the PR's discussion.

[juhovainio]

Windows PowerShell-safety fixes look solid overall: the fails_in_windows_powershell lint, its e2e-cucumber twin, and the new Windows-only execution tests are a good approach, and I verified the inline duplicate of the lint logic hasn't drifted from the original.

One thing I'd like addressed before merge: fix-6's Linux verify line silently drops the rocminfo GPU-detection check (see inline comments) rather than just making it cross-shell-safe. A ;-joined line would satisfy the PR's own fails_in_windows_powershell rule while keeping both checks, since && was never unsafe on the Linux arm to begin with.

Two smaller, non-blocking test-hardening notes:

• The bare-where detection in fails_in_windows_powershell only splits on |/;, so a lone & (cmd.exe's unconditional separator, distinct from &&) wouldn't surface a where token after it. No current recipe has this shape, so it's latent, but worth a follow-up if the rule's doc comment claims exhaustiveness.
• the_msvc_verify_runs_in_cmd_and_powershell checks that both DLL names and some dotted version appear in output, but doesn't confirm each DLL is paired with its own correct FileVersion — a broken Get-Item/Select-Object pairing could still pass.

CI is green aside from the top-level E2E tests aggregator, which is red only due to the pre-existing quit-timeout flake already on main (fixed by #719) — all underlying lanes pass, not a regression from this PR.

[juhovainio]

This drops the rocminfo | head -n 5 GPU-detection check entirely, not just the && operator. The stated motivation (PowerShell safety) only applies to the Windows side — on this Linux-displayed catalog line, && was never a problem. A ;-joined form ("rocminfo | head -n 5; hipcc --version") would pass the new fails_in_windows_powershell rule (which only flags &&/||, not ;) while keeping both checks. Was narrowing this verify line intentional, or would you prefer to preserve the rocminfo check with a ;? (Same change is duplicated at diagnose.rs in check_6_path_missing.)

[volen-silo]

Thanks for the review. The narrowing was intentional, and I'm keeping hipcc --version. Two reasons:

• The recipe has one verify string, and fix-6-path applies on Linux, Windows and WSL. So rocm fix fix-6-path prints the same line on all three. rocminfo | head -n 5 does not run on Windows: in Windows PowerShell 5.1 it fails with "rocminfo is not recognized", and cmd has no head. A ; join would print a line that fails for Windows users.
• fix-6-path only changes PATH, so the verify checks that the changed PATH works: hipcc is found. rocminfo checks whether the GPU is detected. That is a different failure class, and it has its own recipes.

On Linux the && form would run, but the line is shared, so I kept one form for all shells. The rocm diagnose copy follows the same rule, except that its Windows arm checks hipInfo.exe.

The two smaller points are fixed in ffe7a9b: the where check now also splits on a lone & (both copies of the rule, with test cases), and the msvc test now needs one output row per DLL, each with its own dotted version.

[juhovainio]

Same rocminfo check drop as fix.rs's fix-6-path recipe (see that comment) — this is the diagnose-copy of the same verify line.

[volen-silo]

Same answer as the fix.rs thread: the verify stays hipcc --version for PATH, and its Windows arm in rocm diagnose checks hipInfo.exe. rocminfo | head -n 5 does not run in Windows PowerShell 5.1 or cmd.

Addressed in ffe7a9b: the fix-6 verify is kept as hipcc --version (reasons in the thread), the lone & case is covered in the where check, and the msvc test now pairs each DLL with its own version. All required checks pass, and the five Windows execution tests pass in windows-build-and-test.

[nowycondro]

Automated review (medium effort, recall-biased): 3 findings posted inline, ranked most-severe first. The main one is a likely verify regression in fix-15 (Get-Item's non-terminating errors don't flip the exit code), plus a duplication risk and one lower-confidence edge case on fix-13's Join-Path.

[nowycondro]

Possible regression: this verify can report success even when a DLL is still missing.

Get-Item raises a non-terminating error (ItemNotFoundException, category ObjectNotFound) when a path doesn't exist. Non-terminating cmdlet errors don't set powershell.exe's process exit code — only unhandled terminating errors (or an explicit exit) do. So if, say, vcruntime140_1.dll is still missing after the user "fixes" this, Get-Item writes an error to stderr for that one path but the overall command still exits 0.

Compare with the old line, where vcruntime140.dll && where vcruntime140_1.dll: in cmd, where.exe returns a non-zero exit code when a DLL isn't found and && short-circuits, so a missing DLL was reliably signaled via exit code (the only thing broken was PowerShell's && syntax). The new line fixes the PowerShell syntax problem but loses that reliable failure signal in both shells, since cmd just shells out to the same powershell.exe invocation.

The same line is duplicated at crates/rocm-core/src/diagnose.rs:1636, so it has the same issue there. The Windows-only test the_msvc_verify_runs_in_cmd_and_powershell only exercises the "both DLLs present" case, so this gap isn't caught by CI.

A fix that preserves a hard failure signal would be something like:

Get-Item $env:SystemRoot\System32\vcruntime140.dll,$env:SystemRoot\System32\vcruntime140_1.dll -ErrorAction Stop | ...

(-ErrorAction Stop promotes the error to terminating, which does cause a non-zero exit.)

[nowycondro]

This re-implements fails_in_windows_powershell (crates/rocm-core/src/fix.rs:1237) by hand because this crate doesn't depend on rocm-core (acknowledged in the PR description under "Not verified"). That's a real maintenance risk: there's no test that keeps the two copies in sync, so a future tightening/loosening of the rule in one place (e.g. handling a new PowerShell-hostile construct) can silently fail to propagate to the other, and the e2e scenarios would keep passing against a stale rule while the unit tests enforce a different one.

Since this is test-only code, consider making tests/e2e-cucumber depend on rocm-core as a dev-dependency so both call the same fails_in_windows_powershell, rather than maintaining two hand-synced copies of safety-critical validation logic.

[nowycondro]

If HIP_PATH isn't set (plausible right after a fresh SDK install, before the user has actually reopened their shell as the preceding note instructs), [Environment]::GetEnvironmentVariable('HIP_PATH') returns $null, and Join-Path is called with $null for its mandatory -Path argument. Depending on how PowerShell 5.1 binds that, this either throws a parameter-binding error with a different message/shape than "file not found", or (if -Path isn't strictly [ValidateNotNull()]-guarded) could behave unexpectedly. Either way it's a different, less obvious failure mode than the old "$env:HIP_PATH\bin\hipInfo.exe" form produced (which degraded gracefully to a literal relative path lookup). Worth confirming the error a user sees in this state is still clear enough to point them at "HIP_PATH isn't set — reopen your shell," same as before. (The duplicate at crates/rocm-core/src/diagnose.rs:1542 has the same shape.)

[nowycondro]

🤖 Automated review (Claude Code) · ffe7a9b

Reviewed the diff for PR #677 at head commit ffe7a9b (fix(doctor): make the lines printed for Windows run in PowerShell). This PR rewrites several rocm doctor fix/verify lines (fix-2, fix-6, fix-9, fix-13, fix-14, fix-15) in crates/rocm-core/src/fix.rs and crates/rocm-core/src/diagnose.rs so they are valid in both cmd and Windows PowerShell 5.1, and adds/duplicates a fails_in_windows_powershell syntax-shape lint plus an e2e-cucumber copy of the same rule. Findings below (already posted as inline comments on this PR as well):

Blocking

• crates/rocm-core/src/fix.rs:592 (and the duplicate at crates/rocm-core/src/diagnose.rs:1636): the new fix-15 verify line uses Get-Item to check for vcruntime140.dll / vcruntime140_1.dll. Get-Item's file-not-found error is non-terminating in PowerShell, so powershell.exe still exits 0 even when one of the DLLs is genuinely missing. The old where x && where y form correctly propagated a non-zero exit code in that case (its only defect was the unsupported && in PowerShell). None of the new tests exercise the "one DLL missing" path — they only cover the all-present happy case — so this regression in the failure signal isn't caught by the added test suite. Confidence: medium-high (reasoned from documented Get-Item/-ErrorAction semantics; not executed against real PowerShell 5.1 in this sandbox).

Non-blocking

• crates/rocm-core/src/fix.rs:550 (and crates/rocm-core/src/diagnose.rs:1542): fix-13's verify now calls Join-Path with $env:HIP_PATH. If HIP_PATH isn't set yet (e.g., shell not reopened after install), Join-Path -Path $null ... likely raises a different/less clear error than the old string-interpolation form's silent degrade-to-relative-path. The Windows-only test only covers HIP_PATH pointing at an empty directory, not an unset variable. Confidence: medium.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:344 vs crates/rocm-core/src/fix.rs:1237: fails_in_windows_powershell is hand-duplicated between the e2e crate and rocm-core with nothing enforcing the two copies stay identical over time (acknowledged as an intentional tradeoff in the PR description, since the e2e crate doesn't depend on rocm-core, but still a drift risk worth flagging). Confidence: high that the duplication exists as described; low-impact today.

What I read / could not verify: Read the full diff (fix.rs, diagnose.rs, diagnose.feature, diagnose_steps.rs) via GitHub's PR diff/file APIs, plus surrounding context for the touched recipes and checkers. No Rust toolchain was available in this sandbox, so none of the added unit tests, the Windows-only #[cfg(windows)] tests, or the e2e cucumber scenarios were actually executed or compiled here — the findings above are based on static reading of the diff and documented PowerShell/Get-Item/Join-Path behavior, not on running the code. I also did not independently re-verify CI status beyond what's visible on the PR.

🤖 Generated with Claude Code

[nowycondro]

🤖 Automated review (Claude Code) · ffe7a9b

I reviewed the diff in crates/rocm-core/src/{diagnose,fix}.rs and tests/e2e-cucumber/tests/e2e/diagnose_steps.rs, plus the surrounding checker/catalog code, cross-checked against the file contents at the PR head commit.

Possible issue (confidence: medium-high): crates/rocm-core/src/diagnose.rs:1282 — fix-9's Windows commands list still contains "& \"$env:HIP_PATH\\bin\\hipInfo.exe\" | Select-String \"device#|Name|gcnArchName\"", a raw PowerShell-only line (leading & call operator, $env: expansion, Select-String) that breaks if pasted into cmd.exe. This line is unchanged by this PR (only fix-9's verify field was fixed), but it's the same kind of bug this PR sets out to eliminate, and it slips past the new fails_in_windows_powershell rule because that rule only matches &&/||, bare where, %NAME%, and \"/$_ inside a -Command body — not a bare $env:/call-operator line outside a -Command wrapper.

Non-blocking: crates/rocm-core/src/fix.rs:318 — the rewritten fix-2-unset-override verify line (python -c "import os; print(...)") only reports whether the env var is unset; it drops the prior torch.cuda.is_available() check, so a user could see a clean "OK_UNSET" even if HIP/CUDA is still broken for an unrelated reason. Worth confirming this narrowing is intentional.

On fix-15 (fix.rs:589, System32-only Get-Item instead of PATH-wide where): I see this was already raised and the author answered it directly in the thread (comment from volen-silo), explaining the System32 scope is deliberate to get version info for the 64-bit redistributable, so I'm not re-raising it as new.

Minor/non-blocking: tests/e2e-cucumber/tests/e2e/diagnose_steps.rs duplicates the fails_in_windows_powershell text rule by hand (acknowledged in the PR description as a known gap, since the e2e crate doesn't depend on rocm-core) — a future fix to the rule in one place could silently fail to propagate to the other, weakening the e2e guard without anyone noticing.

I did not build or run the test suite (no Rust/Windows toolchain available in this sandbox); findings are based on static reading of the diff and surrounding code.

🤖 Generated with Claude Code

[volen-silo]

Thanks for the careful review. Here is where each point ended up. The branch is rebased onto main now that #680 has merged (diagnose-41 belongs to that PR, so this change's scenarios are diagnose-42 to diagnose-44).

fix-2 setx ... "" (blocking). Fixed, as you suggested: both the catalog and the diagnose Windows arm now print [Environment]::SetEnvironmentVariable('HSA_OVERRIDE_GFX_VERSION',$null,'User'). fails_in_windows_powershell also refuses an empty-string argument outside a -Command body, and the e2e step does the same, so a line like the old one fails diagnose-43. The rule now matches -Command case-insensitively.

Execution tests for fix-9, fix-13 and fix-15 (the earlier inline comments). These exist now and run on Windows in cmd and in Windows PowerShell 5.1: the_igpu_verify_pins_the_variable_it_names (fix-9, with a stand-in torch), the_hip_sdk_verify_runs_the_exe_under_hip_path (fix-13, with a stand-in hipInfo.exe under a path that has a space) and the_msvc_verify_runs_in_cmd_and_powershell (fix-15). Running them locally showed one environment problem: started from a PowerShell 7 session, the 5.1 child inherits that session's PSModulePath, loads PowerShell 7's PSReadLine and writes "Cannot load PSReadline module" to stderr, which the tests (rightly) treat as a failure. The helper now starts PowerShell without an inherited PSModulePath; the stderr assertions are unchanged. I also added the_msvc_verify_exits_nonzero_when_a_dll_is_missing: Get-Item reports a missing path as a non-terminating error and exits 0, so the fix-15 line now has -ErrorAction Stop, and the test fails without it.

Stem count for vcruntime140. The test now needs one output row for each of vcruntime140.dll and vcruntime140_1.dll, each with its own dotted version, so printing one DLL twice cannot pass. It does not prove the version is the right one for the file, and the comment says so.

fix-6 on Linux. You were right that dropping rocminfo | head -n 5 lost the GPU-visible check and that the && was only a problem for the Windows reader. A per-OS verify field is not a small change here: the plan is printed before the platform gate (so every host prints every OS's line), the single verify string is what remote doctor wraps and what the catalog consumers read, and it would change the Verify: output and its docs. So this is the smaller version:

• The rocm diagnose Linux arm only builds for a bash host, so it prints rocminfo | head -n 5 && hipcc --version again (the pre-PR line), and the test pins it as the catalog line with the rocminfo check in front.
• The rocm fix fix-6-path catalog line stays hipcc --version, because the same line is printed for Windows, where rocminfo does not exist. A ; join would print a line that fails there. A new Linux note names rocminfo | head -n 5 for the GPU check and for a runtime-only install without hipcc, and diagnose-44 asserts that note.
• The Windows note no longer says to re-run hipInfo for a verify that runs hipcc.
  If you would rather have the per-OS field, I can do it as a separate change.

Gaps in fails_in_windows_powershell. Widened: -c and the other prefixes of -Command count as the flag; a single-quoted -Command argument is refused (cmd does not take it as one argument); inside a double-quoted body any $ is refused except $env:SystemRoot (same value in both shells; allowed because fix-15 uses it); a bare where is found after ( and {, at the start of a body and after cmd /c. Widening it found a real bug in the fix-22 verify from #680: Where-Object { $_.Name ... } in a double-quoted body, which fails in Windows PowerShell with "The term '.Name' is not recognized". It now uses Where-Object Name -match, with a Windows test that runs it. The rule is still a text check over known-bad constructs, not a parser, and its doc comment now says a construct it does not list passes.

Wording. I softened "valid in cmd and in Windows PowerShell" in the PR description: the lines are meant to run in both, the Windows tests run the catalog ones, and a few lines that only rocm diagnose prints are PowerShell-only by design. I also added a comment to the scenarios that they see what rocm fix --dry-run prints from the catalog, not the Windows Fix values from the checkers, which the unit sweep covers. The stale PATH_FIX_CATALOG_BLOCK fixture was refreshed earlier.

Smaller points. The fix-2 verify calling python is intentional, as you read it (same assumption as fix-1). The fix-15 line looks only in System32, where the old where searched PATH; I kept that, since the redistributable installs there, and listed it under "Not verified". Narrow-console wrapping of Format-Table is also listed there.

[nowycondro]

🔴 Automated review · pr-review-watcher · 349289f

Review — no blocking findings

Full review of the whole change.
No ticket named. The description is a precise, narrow account of fix-15, fix-2, fix-6, fix-13, fix-14, fix-9 and fix-22's verify/command lines, and it already names its own gaps (fix-13 tested against a stand-in exe, fix-14/22/9 not checked against a real torch, the fix-6 PATH write untested, the e2e crate keeping a hand-synced copy of the lint rule). Nothing in the diff falls outside what it claims.

Blocking

None

Non-blocking

• fix-21-msvc-redist-outdated's verify line has the exact Get-Item-without--ErrorAction Stop gap this PR just fixed for its sibling fix-15 — crates/rocm-core/src/fix.rs:908 (verify: "powershell -NoProfile -Command \"(Get-Item $env:SystemRoot\\System32\\msvcp140.dll).VersionInfo.FileVersion\""). Get-Item on a missing path is a non-terminating error in PowerShell 5.1, so a missing/corrupt runtime leaves exit code 0 — the same failure mode fix-15's verify (line 68 of the diff) was rewritten to close with -ErrorAction Stop. fix-21 is untouched by this diff (introduced in #679, confirmed via git log -S) and is out of this PR's stated scope, so it's pre-existing rather than something this change causes — but it's the identical pattern in the same file, in a recipe about the same DLL family, flagged by the same reasoning this PR used for fix-15. The widened "applies on every Windows recipe" lint sweep this PR adds also now runs over fix-21's verify line and passes it, because the lint only checks syntax, not this semantic gap — so the new regression-test net gives a false sense that every Windows verify line is now covered. Confidence 85 (read directly from the catalog; same construct, same consequence, sibling entry already fixed in this diff). Fix: add -ErrorAction Stop to fix-21's verify, in a follow-up or amended to this PR.
• fails_in_windows_powershell's bare-where check is case-sensitive outside a -Command "..." body but case-insensitive inside one, contradicting its own comment ("-Command is not case-sensitive, and neither is this match") — crates/rocm-core/src/fix.rs:1317-1335 (and the identical e2e copy in tests/e2e-cucumber/tests/e2e/diagnose_steps.rs). The outer-body check calls starts_bare_where(line) against the original-case line; the inner-body check calls it against body, a slice of the already-lowercased string. No current catalog line uses uppercase WHERE, so nothing misfires today, but the guard rail this PR is building would silently admit a future WHERE foo.exe written outside a -Command body. Confidence 85 (traced the exact code paths). Fix: lowercase once and run both checks against the lowered string.
• the_hip_sdk_verify_runs_the_exe_under_hip_path's "missing" assertion is weaker than its own doc comment — crates/rocm-core/src/fix.rs:3293-3349. The comment says "the line must fail and say so"; the assertion is !(success && stderr.trim().is_empty()) && stdout.trim().is_empty(), which by De Morgan accepts a run that succeeds (exit 0) as long as it wrote something to stderr — it never actually requires nonzero exit. In practice windows-build-and-test is green, so the real PowerShell/cmd behavior for a missing hipInfo.exe does fail outright and this slack isn't currently masking a live bug; it's a precision gap between the test's stated guarantee and what it enforces. Confidence 80 (traced the boolean logic directly). Fix: assert !success explicitly, or correct the comment to describe what's actually checked.

Decisions for the author

• The e2e-cucumber crate's copy of fails_in_windows_powershell is kept in sync with rocm-core's only by a comment on each, not by any shared fixture or build-time check — tradeoff. Verified character-for-character identical as of this head. The PR's own "Not verified" section already names this as a known gap ("The two must stay in step; only a comment on each keeps them so"), so this is confirming a deliberate, disclosed tradeoff (duplication because the e2e crate can't depend on rocm-core) rather than raising a new one — worth confirming it's still an acceptable cost given how easy silent drift would be to miss in a future change to either copy alone.
• Three new e2e #[given] steps hardcode fix-id string literals ("fix-15-msvc-redist", "fix-2-unset-override", "fix-6-path" in tests/e2e-cucumber/tests/e2e/diagnose_steps.rs) instead of reusing constants already defined a few lines above for the same ids — non-blocking-improvement, pure style, no behavioral risk.

Positive signals

• The shared-id guard test (diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids, crates/rocm-core/src/diagnose.rs) does genuine field-by-field equality checking and was correctly extended to also cover fix-13 and fix-14 with real driving fixtures, not just pinned strings.
• The fix-6 apostrophe-escaping test exercises a real path (C:\Users\O'Neil\ROCm) through ps_single_quote_escape and asserts the doubled quote in both the PATH write and the verify line, rather than merely pinning wording.
• The new Windows-only tests (the_msvc_verify_runs_in_cmd_and_powershell, the_igpu_verify_pins_the_variable_it_names, etc.) actually execute the printed lines in both cmd and Windows PowerShell 5.1 and assert exit code and stderr — this is the real, non-text-only verification the PR's own "Tests" section describes, and windows-build-and-test is green on CI, which is direct evidence these lines do run as claimed.
• The three new e2e-cucumber scenarios (diagnose-42/43/44) are genuine regression tests: tracing them against the pre-PR base commit, each would fail there and passes at head — they are not just newly-added text pins.

Deployment notes

None

What this covered

Full diff b86504a2...349289fe across all five changed files (crates/rocm-core/src/fix.rs, crates/rocm-core/src/diagnose.rs, tests/e2e-cucumber/features/diagnose.feature, tests/e2e-cucumber/src/lib.rs, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs), each read in full by an independent fan-out worker grouped by coupling (core catalog/checker logic vs. the e2e crate's separate copy of the same lint rule), plus a consolidation pass that independently re-traced every surviving finding against the source. CI status checked live (gh pr view --json statusCheckRollup): all applicable checks green, including windows-build-and-test, clippy, Lint (PowerShell), and Commit signatures + sign-off; GPU-gated E2E lanes report no run, which the PR text explains and AGENTS.md treats as expected without local GPU hardware. Commits carry DCO sign-off.

Did not run: an independent "agent-instruction adherence" pass beyond what's folded into the findings above (this repo's AGENTS.md rules on test regression coverage, scenario coverage, and message/behavior pairing were applied directly during the code passes rather than as a separate sweep); "prior-changes" pass (no visible record of review comments on earlier changes to these specific lines, prior to this PR's own review rounds, which were intentionally not read per this method's blind-review rule). Not reconciled against any prior review round or discussion thread on this PR — this is a blind read of the code as it stands at 349289f.

Fixed.

[juhovainio]

I went through the diff and found nothing that breaks the Windows PowerShell goal itself, but there are two places where the verify step got weaker than before, so I am requesting changes on those. The rest is smaller.

Worth fixing before merge (inline comments below):

1. fix-2 diagnose verify no longer runs the torch.cuda.is_available() check, so after clearing the override you can't tell whether the GPU works. The diagnose copy is allowed to differ from the catalog line, so it could keep that check.
2. fix-6 catalog verify is now hipcc --version. On a Linux runtime-only install there is no hipcc, so it fails even when PATH is fixed, and it no longer shows the GPU.

Smaller points, not blocking:

• fix-15 relies on $env:SystemRoot being exempt from the text rule. The outer PowerShell expands it, so a path with a space on a non-default install would split the inner -Command. The code comment already concedes this; low risk.
• The text rule and a copy of command_argument now live in both rocm-core and the e2e crate, kept in step only by comments. That is a drift risk.
• Two assert messages (in fix.rs and diagnose_steps.rs) contain literal line breaks inside string literals instead of \n.
• Question: fix-2 is "auto on windows" in the docs table. Does the auto path run the catalog command text, which is now PowerShell-only ([Environment]::SetEnvironmentVariable(...)), or a native implementation? If it shells out to that text, running it from cmd would break. I couldn't tell from the diff.

Please also confirm the windows-build-and-test lane is green, since it is the only one that runs the shell-run tests, and I could not run them here. This was a read of the diff only.

[juhovainio]

The old verify for fix-2 here also ran the torch.cuda.is_available() check, which is the signal that clearing the override didn't break the GPU. The shared catalog line can't carry it without assuming torch, but this copy is allowed to differ (like fix-6 on Linux). Is dropping it intentional, or could the torch check stay here?

[juhovainio]

hipcc --version fails on a runtime-only Linux install (no hipcc) and no longer shows the GPU. The note covers it, but the verify users actually run is now weaker than before. Is that the intended tradeoff?

[volen-silo]

Thanks for the review. I went through each point against the current code. The branch is rebased on main and squashed to one commit, and I say below where I did something other than what you asked.

fix-2 verify and the torch check (diagnose.rs). Dropping it was not intentional; it is back on Linux. The rocm diagnose Linux arm now prints the catalog line and then python -c "import torch; print(torch.cuda.is_available())". That arm only builds for a bash host, and the catalog line stays torch-free because it is shared with Windows. the_override_fix_verify_matches_the_catalog_copy pins the Linux line as the catalog line plus the torch check, and fails without it. I did not add a torch check to the Windows arm: the Windows arm printed only the User-scope read before this PR too, and a second command after it would need a separator that cmd and PowerShell 5.1 do not share (the text rule refuses && and ||).

fix-6 verify. My earlier answer said a per-OS verify field was too big. That was wrong: it is small. The catalog recipe now has an optional verify_linux, printed as a second line after Verify:, the way the # Linux: command lines are already printed on every OS. For fix-6, Verify: stays hipcc --version (runs on Windows too) and Verify (Linux, WSL): is rocminfo | head -n 5. It shows the GPU and has no hipcc dependency, so it passes on a runtime-only install. The manifest and the plain Verify: line are unchanged. What I did not do: the plain Verify: line still fails on a runtime-only Linux install even with PATH fixed, and rocm diagnose on Linux still ends with && hipcc --version as on main, so that line fails there too. The Linux note now says which line to run and why. diagnose-44 asserts both lines and the note; the Windows sweep applies the text rule to the Linux line as well, since a Windows user sees it printed. A unit test shows the Linux line names no hipcc and the plain line names no rocminfo.

fix-15 and $env:SystemRoot. Changed. The line now finds the DLLs with [Environment]::GetFolderPath('System') inside the body, so the body has no $ and the outer PowerShell has nothing to expand. I ran the new line in cmd and in Windows PowerShell 5.1 (the_msvc_verify_runs_in_cmd_and_powershell, and the missing-DLL test, pass). Because the exemption existed only for this line and fix-21's, I changed fix-21 and the diagnose copies the same way and removed the exemption from the rule: any $ in a double-quoted -Command body is now refused. This touches fix-21's verify line, which is on main; I did it so the rule has no special case, and I can split it out if you prefer.

The two copies of the rule. There is now one table of cases, tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt. The e2e crate's copy moved into a module with a test that reads the table, and a rocm-core test reads the same file. A case that one rule handles differently fails on that side (I weakened the e2e copy to check this, and the test fails). This is a guard on the listed cases, not a proof that the two functions are the same: an input not in the table could still differ. I did not add a crate dependency (cargo xtask check-crate-edges is clean).

Assert messages. Fixed: the two in fix.rs and the one in diagnose_steps.rs use \n now.

Is the fix-2 auto path the catalog text? No. run_unset_override_windows reads both scopes through PowerShell queries and then calls run("setx", ["HSA_OVERRIDE_GFX_VERSION", ""], ...), which spawns setx directly with no shell, so the empty argument arrives intact. The catalog commands are only printed. Running the printed PowerShell line from cmd is not part of the auto path. One related thing I left alone: the runner still prints Will run: setx HSA_OVERRIDE_GFX_VERSION "" as an echo of what it spawns; it is not meant to be pasted.

windows-build-and-test. On the head you reviewed (349289f) it is SUCCESS in the live check list, and so are build-and-test, the Windows and Ubuntu Strix Halo e2e lanes and clippy. The MI300X and W7900 e2e lanes were still queued. After this update I ran locally on Windows 11: cargo fmt --check, workspace clippy with -D warnings, cargo xtask check-crate-edges, cargo test -p rocm-core --all-targets, cargo test -p rocm --bin rocm, and the scenarios diagnose-41 to diagnose-44, examine-29 and driver-install-04. The rocm-core lib tests and diagnose-42 to diagnose-44 also pass in WSL2. I will read windows-build-and-test on the new head after it is pushed.

Fixed.

[juhovainio]

The Windows-side rewrite looks solid: the verify lines are actually executed in cmd and PowerShell on the Windows CI lane, with stderr asserted empty. I have two issues on the Linux side that I'd like fixed before merge, both left as inline comments.

1. The fix-6 verify in rocm diagnose still requires hipcc, which the new catalog note says is absent on runtime-only Linux installs. the_path_fix_verify_matches_the_catalog_copy currently pins that contradiction in place.
2. The fix-2 verify now needs python, which a stock Ubuntu install doesn't have (only python3), so it errors after a correct fix. The old env | grep form worked everywhere.

Minor, no action needed: the PowerShell text rule is duplicated across two crates and kept in sync by the shared case table, as the PR describes.

[juhovainio]

The Linux arm still verifies with rocminfo | head -n 5 && hipcc --version, but the new fix-6 note in fix.rs says hipcc is missing on runtime-only Linux installs and that this makes the plain verify fail even with PATH fixed. rocm diagnose users on that install hit exactly that failure. Consider using only the catalog's Linux line here, and relaxing the {catalog_linux} && {verify} pin in the_path_fix_verify_matches_the_catalog_copy, which currently holds the inconsistency in place.

[juhovainio]

Two python invocations here. Ubuntu ships python3 only, so this prints "command not found" after a correct fix, where the old env | grep ... || echo OK_UNSET worked on a bare system. Consider a Linux-only line (the new verify_linux field exists) that keeps the env | grep form, leaving python for Windows.

[nowycondro]

🔴 Automated review · pr-review-watcher · 194c06b

Review

This PR rewrites several rocm doctor/rocm fix Windows verify lines so they are valid PowerShell 5.1 (and cmd, where shared), backed by a new "text rule" (fails_in_windows_powershell / its e2e-crate twin powershell_text_rule::refusal) that scans every Windows-applicable recipe and checker-built Fix for known-bad constructs, plus real cmd/PowerShell execution tests on Windows CI. The engineering is careful and the new tests are genuinely wired against live catalog/checker content, not just the static fixture. Two logic defects survive review: one is a silent false-negative in a brand-new verify line that mirrors a bug this very PR fixes elsewhere, the other is an asymmetric case-sensitivity gap in the new validation rule itself.

Blocking

• fix-21's new PowerShell verify line never fails on a missing DLL, unlike its sibling fix-15 fixed in this same PR — crates/rocm-core/src/fix.rs:947 (RECIPES fix-21-msvc-redist-outdated), crates/rocm-core/src/diagnose.rs:2308 (check_21_msvc_redist_outdated).
  Evidence: the line is powershell -NoProfile -Command "(Get-Item ([Environment]::GetFolderPath('System') + '\msvcp140.dll')).VersionInfo.FileVersion" — no -ErrorAction Stop. Get-Item on a missing path is a non-terminating error in PowerShell 5.1 (default ErrorAction = Continue), so the expression evaluates to $null, .VersionInfo.FileVersion on $null returns nothing, and the line exits 0 with empty output instead of failing. This is the exact mechanism the PR's own test comment documents and fixes for the sibling recipe a few hundred lines away (fix.rs:620, -ErrorAction Stop, and the_msvc_verify_exits_nonzero_when_a_dll_is_missing). Confirmed by grep that fix-15 is the only Get-Item call in either file carrying -ErrorAction Stop; fix-21's is the only one missing it. No test exercises fix-21's verify at all (no Windows execution test, no structural assertion), unlike fix-15's three dedicated tests. The shared fix.rs/diagnose.rs identity test only asserts the two copies are byte-identical to each other, not that either is semantically correct, so nothing in the suite catches this.
  Confidence 85 · logic · Fix: add -ErrorAction Stop to both copies of fix-21's Get-Item call, and add a Windows execution test mirroring the_msvc_verify_exits_nonzero_when_a_dll_is_missing for msvcp140.dll.

• The new "bare where" detector in the shared PowerShell text rule is case-sensitive on one path while the rest of the rule is explicitly case-insensitive, contradicting its own documented invariant — crates/rocm-core/src/fix.rs:1385 (fails_in_windows_powershell), mirrored identically at tests/e2e-cucumber/src/powershell_text_rule.rs:37 (refusal).
  Evidence: the function lowercases the whole line into lowered specifically "so neither is this match" case-sensitive (its own comment), and command_argument(&lowered) is checked case-insensitively. But if starts_bare_where(line) || command.is_some_and(|(_, _, body)| starts_bare_where(body.trim_start())) passes the original, un-lowered line to the first disjunct — only the second disjunct (inside a -Command body) operates on lowered text. Concretely, refusal("CMD /C WHERE foo.dll") / fails_in_windows_powershell("CMD /C WHERE foo.dll") returns None (not flagged), even though WHERE resolves to Where-Object in real PowerShell exactly like the lowercase form the rule exists to catch. Verified by grep that no current RECIPES/diagnose.rs line triggers this today (all real usages are the hyphenated Where-Object, case notwithstanding), so it is latent rather than live, but it is a genuine logic bug in new code introduced by this PR, in the very mechanism meant to prevent future regressions of this class.
  Confidence 75 · logic · Fix: call starts_bare_where(&lowered) instead of starts_bare_where(line) in both copies, and add a fixture case (e.g. flag CMD /C WHERE foo.dll) to tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt to lock in the case-insensitivity the rest of the function already assumes.

Non-blocking

None.

Decisions for the author

• Generalizing the missing--ErrorAction Stop lesson into the text rule itself — non-blocking-improvement. The fix-21 bug above is exactly the class of mistake fails_in_windows_powershell was introduced to prevent, but the rule only checks syntax constructs, not this semantic one (a Get-Item/similar non-terminating-error cmdlet used for a presence/content check without -ErrorAction Stop). Worth considering whether the rule should flag this pattern generically, so a future recipe doesn't reintroduce it silently.
• The text rule exists as two independently-maintained copies (rocm-core, test-only, and the e2e crate, which cannot depend on rocm-core per the crate-layering invariant), synchronized only by a shared fixture file — tradeoff. This is deliberate and explained in the code/PR text, with a real safety net (both copies are tested against the same case table, so a case one gets wrong fails on that side) — flagging only so the author can confirm this is the long-term intended shape rather than a stopgap.

Positive signals

• check_6_path_missing's Windows PATH-write line correctly reuses the already-shared crate::quoting::ps_single_quote_escape helper (diagnose.rs:1052) rather than hand-rolling new escaping logic, consistent with the sibling consolidation PR (#743) merged just before this one.
• windows_recipes_pass_the_powershell_text_rule and windows_diagnosis_remediation_passes_the_powershell_text_rule derive their recipe/checker sets from applies_on/CHECKERS rather than a hand-picked list (with a self-guard asserting the expected fix-ids were actually seen), so a future Windows recipe or checker is automatically covered without editing the test.
• Real execution tests (run_typed_in_cmd_and_powershell) actually pipe the printed lines into live cmd/powershell processes on Windows CI, going well beyond the static text rule for the lines it covers (fix-2, fix-9, fix-13, fix-14, fix-15).

Deployment notes

None — internal CLI text/test-only changes, no migrations, no new runtime dependencies, no config surface.

What this covered

Full review of the whole change: all 8 files in the diff (crates/rocm-core/src/diagnose.rs, crates/rocm-core/src/fix.rs, docs/testing.md, tests/e2e-cucumber/features/diagnose.feature, tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt, tests/e2e-cucumber/src/lib.rs, tests/e2e-cucumber/src/powershell_text_rule.rs, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs), read completely, plus the full bodies of every touched function/recipe and relevant surrounding code (RECIPES entries, CHECKERS, print_recipe/verify_lines/verify_steps/act_on). Commit range 24e405a7614576b350dbe47ba7b5cc317f760f33...194c06be81b416ce047171253b707ebfe6ff8b32. Split across two independent fan-out workers (diagnose.rs+fix.rs core logic; the PowerShell text-rule test harness), each reading its files completely; both findings above were then independently re-derived and confirmed by me directly against the PR-head source (exact line numbers, grep-confirmed that each gap is isolated/latent as described) rather than taken on the workers' word. Checked this repo's AGENTS.md for applicable rules (user-observable-behavior-needs-a-scenario: satisfied via diagnose-42/-43/-44; test-weakening: none observed; doc-sync-on-behavior-change: not triggered, since only internal command text changed, not documented symptom descriptions in skills/rocm-doctor/reference.md/doctor-catalog.json, which were checked and are unaffected). Checked recent git history of both core files for context, which surfaced the immediately-preceding shared-quoting consolidation (#743) this PR correctly builds on.

Did not run: cargo test/cargo clippy locally — no Rust toolchain available in this review environment; relied instead on CI's windows-build-and-test and Test (affected crates) results (both pass on the reviewed commit), which are specifically what exercises this PR's new Windows-only execution tests. All CI checks pass on 194c06be, except Skill checks (skillscope) and Sphinx docs build (-W), both reported skipping (conditional/advisory, unrelated to this change, not failures). "Prior changes" (do review comments on earlier PRs to these files still apply) was covered only via git log, not a dedicated pass. Not reconciled against this PR's existing review threads/discussion (prior comments from other reviewers) — this is an independent blind review per method.

[rominf]

🔴 Automated review · pr-review-watcher · 349289f

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.

Informational: this report describes commit 349289f, not the current head. The head moved to 194c06b while this round was running, and the code there differs. Nothing below has been checked against 194c06b. This report does not gate and files no change request. A later round will review the current head and settle these findings there. Line numbers refer to 349289f.

Sixth automated round. The previous round's commit (ffe7a9b) was force-pushed away, so this is a full review of the whole change at 349289f. This account holds no change request on this PR.

Standing vs new. Every finding below is new this round, except the two items appended at the end of Non-blocking, which are standing from the previous round. They were not re-found blind. They were settled by putting each objection, with the author's reply, to the reviewer after its report was frozen (anchored evidence, not blind findings):

• fix-2 setx … "" printed as a Windows line: no longer holds for the catalog and diagnose copies. The runner's "Will run" line is a separate new finding below.
• fix-9's bare & "$env:HIP_PATH\…" line: still holds and is unchanged by this PR.
• The "unseen" doc comment: the $name-in-body gap is fixed. The mutation now fails two tests. The wording still overclaims.

The Blocking item would not have been filed as a change request at this commit either. The lines it names are already run by the unit sweep and, on Windows, by the execution tests. AGENTS.md §3 also allows the remedy to be a reason given in the PR text. So merging leaves a convention gap, not wrong behaviour. Both standing items are description or comment wording only. The commit messages claim only the verify lines, and that claim holds.

Review — needs work

Full review of the whole change.
No ticket named. The description follows #680 and matches the diff, apart from one claim about the text rule noted under Non-blocking.

Blocking

• Three changed printed lines have no scenario (AGENTS.md §3) — crates/rocm-core/src/fix.rs:551 (fix-13 verify), fix.rs:573 (fix-14 verify), fix.rs:933 (fix-22 verify), tests/e2e-cucumber/features/diagnose.feature:637-678
  AGENTS.md §3 says a change to "what a user of the CLI can observe — command output" must be covered by a Gherkin scenario. It also says "a unit test asserting the internal helper does NOT discharge this". If no scenario is added, the PR text has to say why.

  • Covered: diagnose-42/43/44 apply the rule to the rocm fix --dry-run output of fix-15, fix-2 and fix-6.
  • Not covered: the rewritten verify lines of fix-13, fix-14 and fix-22, which rocm fix <id> --dry-run prints just the same. The only coverage is the unit sweep windows_recipes_pass_the_powershell_text_rule.
  • The description does explain why the rocm diagnose copies are left to unit tests. It gives no reason for these three catalog lines.
  • Adding them is cheap. The diagnose-42 comment notes the plan "is printed before the fix's platform gate, so the text is the same on every lane", so the existing Given/When/Then steps would apply unchanged.

  Confidence 85 · mechanical · Fix: add fix-13, fix-14 and fix-22 to the same steps, for example as a Scenario Outline over the six ids. Otherwise state in the PR text why they are exempt.

Non-blocking

• The text rule misses a capitalised where and cmd.exe, though its doc and the description say both are caught — crates/rocm-core/src/fix.rs:266-279, twin at tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:354-369

  • starts_bare_where(line) runs on the raw line. Only the -Command body is taken from lowered.
  • The cmd hop only accepts the exact word cmd.
  • So Where foo.dll, WHERE foo.dll, CMD /C where foo.dll and cmd.exe /c where foo.dll all return None. A worker probe run confirmed this.
  • PowerShell aliases and cmd are both case-insensitive. The description says the rule refuses "a bare where … after cmd /c … and the match ignores case".
  • The || head.contains("pwsh") clause of command_argument is also untested: a mutation that removes it leaves every test green.

  Confidence 100 · logic · Fix: test the lowercased line and accept cmd.exe. Add these cases and a pwsh -c "…" case to the bad list in the_powershell_text_rule_flags_what_it_should.

• Nothing tests the e2e copy of the rule — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:333-410

  • Read side by side, the copy is equivalent to fails_in_windows_powershell today.
  • The scenarios only feed it output that already passes, so a clause dropped or drifted in the copy stays green everywhere.
  • The description says "only a comment on each keeps them so". That makes the twin's correctness depend on review alone.

  Confidence 85 · mechanical · Fix: run the same good/bad table against the copy in a unit test in the e2e crate. Alternatively, move the rule into a small shared test-support module both crates can use.

• The fix-2 verify now needs python on Linux and WSL, and the comment justifying it is inaccurate — crates/rocm-core/src/fix.rs:316-318, crates/rocm-core/src/diagnose.rs:805-806

  • The old catalog line, env | grep … || echo OK_UNSET, needed nothing. The new one fails with "command not found" on stock Ubuntu outside a venv.
  • The comment says "the override only matters to a framework wheel". In fact the ROCm runtime reads HSA_OVERRIDE_GFX_VERSION for any HIP application, such as llama.cpp, so a user checking a non-Python app may have no python at all.
  • The description acknowledges the python dependency. Its reason rests on the same premise.

  Confidence 70 · logic · Fix: correct the comment, and either add a note naming python3 / printenv HSA_OVERRIDE_GFX_VERSION for Linux, or accept the tradeoff explicitly.

• Windows rocm fix fix-2-unset-override still prints the setx line the PR removed — crates/rocm-core/src/fix.rs:2066, comment at fix.rs:2019-2023

  • report_and_clear_override_windows prints Will run: setx HSA_OVERRIDE_GFX_VERSION "". That is the same output where print_recipe now shows the .NET line.
  • What rocm itself runs is correct: it spawns setx directly, so the empty argument survives.
  • But a user who copies the "Will run" line into Windows PowerShell 5.1 hits the "Invalid syntax" failure this PR exists to remove. The new tests cannot see the line: the sweep reads only RECIPES/Fix, and the e2e step reads only $ and Verify: lines.

  Confidence 70 · mechanical · Fix: make the runner run and print the .NET call, the way run_path_export_windows works. Or at least print text that cannot be mistaken for a line to paste.

• The fix-6 runner's success message still says "re-running hipInfo" — crates/rocm-core/src/fix.rs:2286
  The recipe note now says "before running the verify command", and the catalog's verify, also printed on Windows, is now hipcc --version. One output therefore gives two different next steps. The code is the side that governs here, because the shared verify line is what rocm fix prints as Verify:. The fixture at tests/e2e-cucumber/src/lib.rs pins the old wording and needs to change with it.

  Confidence 70 · mechanical · Fix: align the runner message with the note.

• Comments name the wrong guard test, or leave out fix-22

  • crates/rocm-core/src/diagnose.rs:805 says "the shared-id guard test keeps them equal". fix-2 is not in diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids; the_override_fix_verify_matches_the_catalog_copy is the test that holds the two copies equal.
  • Three doc comments say only the fix-2/13/14/15 verify lines are run in a shell: on fails_in_windows_powershell (fix.rs:243-250), on windows_recipes_pass_the_powershell_text_rule ("the other catalog lines are not run"), and on the diagnose sweep. In fact the_display_driver_verify_runs_in_cmd_and_powershell also runs fix-22.
  • the_path_fix_verify_matches_the_catalog_copy says the shared-id guard covers "fix-13, fix-14, fix-15 and fix-16". It also covers fix-21, info-20 and fix-22.

  Confidence 90 · mechanical · Fix: correct the names and lists.

• (standing) fix-9's Windows line is PowerShell-only, but it is not labelled as such and the description says lines shown to cmd users run in cmd — crates/rocm-core/src/fix.rs:460 (catalog, under a plain # Windows: header), crates/rocm-core/src/diagnose.rs:1285, runner at fix.rs:2378

  • & "$env:HIP_PATH\bin\hipInfo.exe" | Select-String … breaks in cmd at &, and cmd does not expand $env:.
  • The author's reply says some PowerShell-only lines are printed only by rocm diagnose. That cannot cover this one, because rocm fix fix-9-igpu-dgpu prints it too.
  • The text rule passes it, because it only checks $ inside a -Command "…" body.
  • This PR did not change the line. The commit messages claim only the verify lines, so this is a description mismatch and is not gated.

  Fix: label it # Windows (PowerShell): as fix-2 now is, and narrow the description. Or wrap it in powershell -NoProfile -Command "…".

• (standing, narrowed) The diagnose sweep's doc comment still says hand-built lines "cannot regress unseen" — crates/rocm-core/src/diagnose.rs:7148

  • The $name-in-body case from the previous round is now caught. With $env:HIP_VISIBLE_DEVICES=1; … put back into the fix-9 verify, cargo test -p rocm-core --lib failed windows_diagnosis_remediation_passes_the_powershell_text_rule and the_windows_igpu_verify_is_one_quoted_powershell_argument.
  • The sentence still overclaims. A bare PowerShell-only line (fix-9 above) and a capitalised Where both pass the sweep at this commit.
  • The rule's own doc ("a construct it does not list passes") describes the code correctly, so it governs.

  Fix: narrow the sentence to the constructs the rule lists.

Decisions for the author

• One verify line shared by every platform forces lowest-common-denominator checks — tradeoff
  • What it costs: fix-6's catalog verify dropped the rocminfo GPU check, which now lives only in a note. fix-2's catalog verify now depends on python, and its diagnose Linux copy dropped the import torch check it used to carry.
  • What it keeps: the recipe schema stays simple, and the shared-id guards still hold the catalog and diagnose copies to one line.
  • The alternative: a per-platform verify on FixRecipe would let Linux keep rocminfo and env/printenv, and Windows keep hipInfo.exe, without any line having to run in all three shells.

Positive signals

• The Windows-only tests feed each printed line to real cmd and Windows PowerShell 5.1 over stdin, as it would be typed. Clearing an inherited PSModulePath avoids a false stderr failure from PowerShell 7's PSReadLine.
• the_igpu_verify_pins_the_variable_it_names uses a stand-in torch that echoes HIP_VISIBLE_DEVICES and removes the variable from the child first. It proves the line sets the variable, not just that it runs.
• Widening the rule found a real, already-shipped break in fix-22's $_ filter, and that fix is included with a test that fails on the old line.

Deployment notes

None

What this covered

Every changed file read completely, plus the surrounding runner code in fix.rs (report_and_clear_override_windows, run_path_export_windows, print_recipe), the advised-commands scanner, and repo-wide greps of README.md, docs/, skills/ (including rocm-doctor/reference.md), apps/ and scripts/ for the old printed strings. None remain. Reviewed prw-base...349289fe (merge base b86504a…). Nothing at 194c06b was read.

Fan-out: 2 independent workers, one for code and test quality, one for impact, history and agent-instruction adherence. Step 7 design questions were answered in the driver context, not by independent workers.

Run here:

• cargo test -p rocm-core --lib for the non-Windows tests the change adds: 72 matched, all passed.
• 24 single-branch mutations against those tests.
• A probe of the text rule.
• The fix-13 body run under pwsh 7.

Not run here:

• Windows PowerShell 5.1 and cmd behaviour; only pwsh 7 on Linux was available.
• The seven #[cfg(windows)] tests. CI windows-build-and-test is green on 349289f, but its logs were not read, so it is not confirmed that they ran there.
• The e2e suite.
• Whether the Windows HIP SDK ships hipcc for the shared fix-6 verify; this could not be established.

CI on 349289f at review time: every completed check is green. The self-hosted E2E lanes (MI300X, W7900) were still queued and had not reported. Sphinx docs build and skillscope were skipped. That is missing evidence, not a failure of this PR.

Sign-off: check_signoff.py exit 0, 3 of 3 commits signed off.

No prompt-injection content found. Not reconciled against any PR discussion here.

[volen-silo]

Thanks for the review. I went through each point against the current code. The branch is rebased onto the latest main (which took diagnose-42, so this change's scenarios are now diagnose-43 to diagnose-45) and is still one commit. Below, I say where I did something other than what you asked.

fix-6, rocm diagnose on Linux still needed hipcc (diagnose.rs). Agreed, and fixed as you suggested: the Linux arm now prints the catalog's Linux line alone, rocminfo | head -n 5. I considered whether dropping hipcc loses the proof that PATH was fixed. It does not: rocminfo lives in the same bin directory this fix puts on PATH, and the checker fires on rocminfo not being found, so rocminfo printing the GPU shows both that PATH works and that the GPU is visible. A hipcc check proves nothing more about this fix and fails on a runtime-only install. I relaxed the pin in the_path_fix_verify_matches_the_catalog_copy to "equals the catalog's Linux line, and does not name hipcc". A new Unix-only test, the_path_fix_verify_passes_on_a_runtime_only_linux_install, runs the line in /bin/sh on a PATH with a stand-in rocminfo and no hipcc: it exits 0 and prints the GPU rows, and the old && hipcc --version chain exits non-zero on the same PATH. One caveat I did not change: rocminfo | head -n 5 exits 0 even when rocminfo is missing (the pipeline takes head's status), so it fails by printing "command not found", not by exit code. That is the catalog line you already reviewed.

fix-2, python on a stock Ubuntu (diagnose.rs). Agreed that it fails there, and I used the verify_linux field, but not with the env | grep form you suggested. The line is printed on every host, including Windows, and the text rule refuses ||, so env | grep ... || echo OK_UNSET cannot be a catalog line. The catalog now prints Verify (Linux, WSL): echo "${HSA_OVERRIDE_GFX_VERSION:-OK_UNSET}". It prints the same answer as the plain line (the override's value, or OK_UNSET), needs no Python and no external command, and passes the rule. A note tells the user to run that line on Linux and WSL and says why the plain one fails there. The plain Verify: line keeps python -c: it is what cmd and PowerShell run, and the Windows run test covers it. The catch is a Windows machine with no Python install; I could not rule that out. For the torch check in rocm diagnose I used python3, not python: whichever interpreter has torch is reachable as python3 in a stock install, a virtual environment and a conda environment, so it is the more available name. The Linux arm now prints the catalog's Linux line, then python3 -c "import torch; print(torch.cuda.is_available())". the_override_fix_verify_matches_the_catalog_copy pins that, and rejects a bare python word. Two Unix-only tests run the lines: the_override_fix_verify_runs_where_only_python3_exists (a PATH with python3 and no python, override set and unset) and the_override_linux_verify_runs_with_nothing_on_path (the catalog Linux line with an empty PATH). Other rocm diagnose verify lines, such as fix-1, still call python; I left them alone.

The fix-2 plan is covered by the scenario diagnose-44 (the Linux line has no python, and a note names the python3 case); with the recipe's verify_linux removed it fails. The fix-6 change is in rocm diagnose, which the rocm fix --dry-run scenarios do not print; diagnose-45 still covers the catalog's lines and its text is unchanged, so it is covered by the unit tests above, not by a new scenario. I checked each unit test by changing only production code (removing verify_linux, putting python or && hipcc --version back in the diagnose arms) and the named tests fail.

The automated review of the same head. I checked each finding against the code.

• fix-21 verify had no -ErrorAction Stop (blocking). Valid. fix-21's line (catalog and diagnose copy) had the same flaw as fix-15's before it was fixed. Both copies now carry -ErrorAction Stop. New Windows-only test the_outdated_msvc_verify_prints_a_version_and_fails_when_the_dll_is_missing runs the catalog line in cmd and PowerShell (a dotted version, empty stderr), then with the DLL renamed to one that does not exist both shells must exit non-zero and name the file. It fails without the flag. The diagnose copy is held equal to the catalog line by the shared-id test.
• The bare-where check was case-sensitive on one path (blocking). Valid: CMD /C WHERE foo.dll passed in both copies of the rule. Both now match against the lowered text, and the case table has flag CMD /C WHERE foo.dll and flag WHERE foo.dll; the case-table test fails on each side without the change.
• Generalize the -ErrorAction Stop lesson into the rule (decision). I added a narrow version: a Get-Item inside a -Command body must carry -ErrorAction Stop, in both copies and with table cases. I did not try to cover every cmdlet that reports a missing path without a terminating error; that would flag lines (fix-14 filters an adapter list) where an empty result is correct. The PR text says the rule covers Get-Item only.
• The rule exists as two copies (decision). That is the intended shape, not a stopgap: the e2e crate cannot depend on rocm-core (cargo xtask check-crate-edges enforces it), and the shared case table is what keeps the two answering alike on every listed line. It is a guard on listed cases, not a proof that the functions are equal.

Not run. cargo test in its single-process form (what the Windows CI lane uses), python scripts/smoke_local.py and a full cargo test --workspace --all-targets were not run for this update. The Linux verify lines were run against stand-in executables, not a ROCm install or a real torch. I will read windows-build-and-test on the new head after it is pushed.

Addressed in 8c6de71; point-by-point reply: #677 (comment)

[nowycondro]

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

Review — needs work

Full review of the whole change (one commit, ea86403c…8c6de715).
No ticket named; the description references PR #680 as prior context (the fix-22 Where-Object { $_.Name ... } bug this PR fixes was introduced there) — retrieved and confirmed coherent with this PR's own account of it.

Blocking

• A required-looking check is failing on this head commit — docs/readthedocs.com:advanced-micro-devices-rocm-cli (status context, reported FAILURE via live gh pr view --json statusCheckRollup / gh pr checks 677 at 8c6de715)
  Every GitHub Actions check (clippy, both Clippy e2e-harness lints, windows-build-and-test, build-and-test, Test (affected crates), E2E tests, Coverage (rocm-dash crates, ratcheted), Commit signatures + sign-off, Markdown links resolve (lychee), etc.) is green; this one external status context is red. I could not read the actual build log (https://app.readthedocs.com/projects/advanced-micro-devices-rocm-cli/builds/4638870/ returns HTTP 403 to an unauthenticated fetch, and this sandbox's network policy also blocked gh api repos/ROCm/rocm-cli/commits/.../status and .../branches/main/protection, so I could not compare against main's own Read the Docs status to judge whether this is pre-existing/flaky or caused by this diff's docs/testing.md addition). Per a failing-check policy, this blocks until it's confirmed either fixed or a known, unrelated, already-red-on-main flake.
  Confidence 100 · ci · Fix: open the linked Read the Docs build log and either fix the real failure or, if it's an unrelated pre-existing flake, note that in the PR and/or get it excluded from the merge gate.

Non-blocking

• verify_lines's own doc comment is wrong for the new Linux/WSL label, and the formatting bug it describes is real (just not yet triggered) — crates/rocm-core/src/fix.rs:1768-1785
  The function's doc comment says "A multi-line line keeps its continuation under the label." indent_continuation(r.verify, " ") (line 1777) pairs the 12-character label "Verify: " with a matching 12-space indent — correct. But indent_continuation(linux, " ") (line 1783) reuses the same 12-space string for the label "Verify (Linux, WSL): ", which is 21 characters. A future multi-line verify_linux value would have its continuation lines land 9 columns short of the label's own text column, contradicting the doc comment. No current verify_linux value (fix-2's, fix-6's) spans multiple lines, so nothing visibly breaks today.
  Confidence 85 · mechanical · Fix: give the Linux/WSL branch its own 21-space indent string (or compute it from the label's length).

• The doc comment on fails_in_windows_powershell enumerating which tests actually run each Windows verify line in a shell omits two tests this same diff adds — crates/rocm-core/src/fix.rs:1394-1401
  "On Windows, the verify lines of fix-2, fix-13, fix-14 and fix-15 are run by the_override_verify_runs_in_cmd_and_powershell, the_hip_sdk_verify_runs_the_exe_under_hip_path, the_adrenalin_verify_runs_and_its_filter_keeps_only_amd and the_msvc_verify_runs_in_cmd_and_powershell..." omits the_outdated_msvc_verify_prints_a_version_and_fails_when_the_dll_is_missing (fix-21, line 3485 area) and the_display_driver_verify_runs_in_cmd_and_powershell (fix-22, line 3684), both of which this diff adds and which do exactly what the comment is listing.
  Confidence 85 · mechanical · Fix: add fix-21 and fix-22 to the enumeration.

• fix-22's Windows-shell test doesn't prove its adapter filter actually filters, unlike the textually-identical fix-14 test — crates/rocm-core/src/fix.rs:3684-3708 vs crates/rocm-core/src/fix.rs:3710-3745
  Both recipes build the same Where-Object Name -match 'AMD|Radeon' text (duplicated, no shared constant). the_adrenalin_verify_runs_and_its_filter_keeps_only_amd (fix-14) explicitly notes that "its output is empty on a host with no AMD adapter, so that alone would pass for a filter that matches nothing," and so additionally extracts the Where-Object stage and runs it over synthetic adapter objects, asserting exactly ["AMD Radeon X", "Radeon Pro W"] survive. the_display_driver_verify_runs_in_cmd_and_powershell (fix-22) only asserts success and empty stderr. If fix-22's filter text ever silently diverged from fix-14's (nothing ties the two together), a broken or over/under-matching filter for fix-22 specifically would pass this test on any CI host without an AMD GPU.
  Confidence 75 · logic · Fix: reuse the same synthetic-adapter assertion for fix-22 (factor it into a shared helper keyed by recipe id), or at minimum assert fix-14's and fix-22's filter substrings are equal.

• The Get-Item-without--ErrorAction Stop check is a whole-body substring check, not paired per call — crates/rocm-core/src/fix.rs:1461 (and its mirror, tests/e2e-cucumber/src/powershell_text_rule.rs)
  body.contains("get-item") && !body.contains("-erroraction stop") flags a -Command body only when the entire body has no -ErrorAction Stop anywhere. A body with two separate Get-Item calls, only one of which carries the flag, would not be flagged, because the single occurrence of -erroraction stop anywhere suppresses the warning for the whole body. No current catalog line has more than one Get-Item call (fix-15's single call takes both DLL paths as an array, fix-21's has one call), so this isn't triggered today, but it's a real soundness gap in a rule the PR explicitly relies on as the systematic safety net ("applies it to every catalog recipe that applies on Windows").
  Confidence 70 · logic · Fix: pair each Get-Item occurrence with its own nearby -ErrorAction Stop (e.g., split on Get-Item and check each segment), or note the limitation isn't just "other cmdlets" but "more than one Get-Item per body."

Decisions for the author

• The general Windows-paste-safety rule doesn't cover python -c "..." (or any other non-powershell/pwsh double-quoted argument) for the same $-expansion hazard it polices inside -Command bodies — tradeoff
  Typed directly into an interactive Windows PowerShell 5.1 session, any double-quoted argument (not just a -Command "..." one) is subject to the same $-expansion/quote-termination risk. fails_in_windows_powershell/refusal only inspects the body following a detected powershell/pwsh ... -Command flag (command_argument in crates/rocm-core/src/fix.rs:~1329 requires that substring). This is a deliberate, tested scope choice — the shared fixture (tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt) explicitly asserts pass python -c "import os; print(os.environ.get('A') or 'B')" — and the one Windows-visible python -c recipe today (fix-2's plain line) is separately covered by a bespoke test (the_override_verify_is_one_line_for_every_shell). The tradeoff: keep the narrow, simple rule and rely on a reviewer/author remembering a bespoke test for every future python -c-style Windows line, or broaden the general rule to cover any double-quoted top-level argument and lose that simplicity. Worth confirming this is the intended long-term posture rather than an oversight nobody revisits.

• Verify-line text keeps being duplicated between fix.rs's catalog and diagnose.rs's checkers rather than shared via a constant — non-blocking-improvement
  This continues a pattern a previous review on #680 already flagged ("the fix-22 verify PowerShell string is duplicated literally between fix.rs and diagnose.rs rather than shared via a constant, guarded only by a drift-detection test... low risk given the test exists, but a shared constant would remove the possibility entirely"). This PR adds more of the same: fix-13/fix-14's Windows verify text, and fix-2's/fix-6's Linux-only verify lines, are each hand-duplicated between the two files and held equal only by passing tests (diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids, the_path_fix_verify_matches_the_catalog_copy, the_override_fix_verify_matches_the_catalog_copy — all verified present and asserting real equality, and CI is green). Still low risk today, same conclusion as before; a shared constant would remove the drift risk outright rather than relying on tests to catch it after the fact.

Positive signals

• The test suite consistently includes a "control" run of the old, broken line alongside the new one (e.g. the_path_fix_verify_passes_on_a_runtime_only_linux_install also runs "rocminfo | head -n 5 && hipcc --version" and asserts it fails; the_msvc_verify_exits_nonzero_when_a_dll_is_missing and the fix-21 equivalent mutate the DLL name to prove -ErrorAction Stop is load-bearing), which is exactly the mutation-based discrimination a reviewer would otherwise have to do by hand.
• check_6_path_missing's Windows arm computes escaped_bin_dir once via the pre-existing ps_single_quote_escape and reuses that single escaped value for both the PATH-write command and the verify command, so the two printed lines cannot drift from each other on escaping — a better pattern than the hand-duplicated-across-files one noted above.
• Replacing $env:HIP_PATH/$env:SystemRoot with [Environment]::GetEnvironmentVariable(...)/[Environment]::GetFolderPath('System') inside -Command bodies is a genuinely good fix: it reads the value at runtime via .NET instead of needing the value itself to be escape-safe as a string literal, which also sidesteps the apostrophe/space problems a literal $env:HIP_PATH substitution would otherwise have.

Deployment notes

• User-visible output changes: rocm fix now prints a second Verify (Linux, WSL): line for fix-2-unset-override and fix-6-path, and different Verify: text for fix-2, fix-6, fix-13, fix-14, fix-15 and fix-21. rocm diagnose output on Linux changes accordingly (no more hipcc/python in the respective Linux verify text). No schema/contract change: confirmed doctor-catalog.json doesn't encode verify text at all (only id/title/platforms/needs_*/doc_url), so cargo xtask catalog --check correctly stays green with no regeneration needed.

What this covered

Read the full diff ea86403c0b1ea60eb560904d3c96d321e7d8e1e8...8c6de715e8f34f2d7c23e508cb599899de80b9a4 (8 files, ~1934 insertions) against a plain read-only clone (PR head fetched via refs/pull/677/head, never checked out). Read crates/rocm-core/src/fix.rs and crates/rocm-core/src/diagnose.rs completely (both full files, not just the changed hunks), the full new tests/e2e-cucumber/src/powershell_text_rule.rs, the full shared fixture tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt (hand-traced against its own logic), the new/changed portions of tests/e2e-cucumber/tests/e2e/diagnose_steps.rs, tests/e2e-cucumber/features/diagnose.feature, tests/e2e-cucumber/src/lib.rs, and docs/testing.md, plus this repo's AGENTS.md in full. One independent fan-out worker (covering fix.rs) returned after an unusually long delay caused by heavy, unrelated concurrency load on the shared execution environment (not something retriable within a practical budget: two further spawn attempts for diagnose.rs and the e2e/docs group both hit a hard "concurrent subagent limit reached" error); its findings are folded in above and were independently re-verified against the source rather than taken on trust. The diagnose.rs and e2e-cucumber/docs passes were instead performed directly by this same driver context rather than by independent workers, so for those two groups this is not an independent-worker review in the sense the method prefers — flagging per the method's own disclosure rule. Impact-analysis searches covered doctor-catalog.json, skills/rocm-doctor/reference.md, skills/rocm-doctor/SKILL.md, README.md and docs/manual-testing.md for stale references to the old command text (none found). Prior-changes pass: read PR #680's full review history on these same files (not this PR's own discussion) for recurring concerns; the one still-applicable point (duplicated literal verify text) is carried into Decisions for the author above. History pass: the PR is a single, DCO-signed, descriptively-messaged commit. CI read via live statusCheckRollup/gh pr checks at the re-confirmed head 8c6de715, taken at face value per the review method; not re-derived from workflow definitions beyond locating which job the failing docs/readthedocs.com status belongs to (it is a separate webhook-driven status context, not a GitHub Actions job). Not reconciled against any discussion on this PR — this is a blind re-review independent of any earlier round.

[juhovainio]

Approving. The PowerShell-safe verify/clear lines, the new Linux verify line and the diagnose-43/44/45 scenarios hang together, and the notes are asserted against the lines they name. Nothing here blocks merge. This is a read of the diff plus the CI results, not a run on a Windows box.

Non-blocking notes, take or leave:

1. fix-2: the Windows clear line ([Environment]::SetEnvironmentVariable(..., $null, 'User')) is PowerShell-only. The catalog copy in fix.rs labels it # Windows (PowerShell):; it would be good to keep the diagnose.rs copy labelled the same so a cmd user doesn't paste it and fail.
2. Verify (Linux, WSL): is longer than the Verify: / Note: label column, so the plan output is misaligned.
3. fix-15: the verify now only looks in GetFolderPath('System') (the old where searched PATH), and the Select-Object FileName,FileVersion table can wrap the path on a narrow console. Format-List would avoid the wrapping.
4. Test strictness: the Get-Item rule check lets a single -ErrorAction Stop anywhere in a body excuse every Get-Item in it, and the fix-13 "missing" assertion doesn't pin a non-zero exit the way the fix-15/21 tests do.
5. The PowerShell text rule is duplicated between fix.rs (fails_in_windows_powershell) and the e2e crate (powershell_text_rule::refusal). The shared case table limits drift, but only for the cases it lists.
6. The header of tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt says the steps serve "diagnose-42..44"; they are diagnose-43..45.

CI: everything passes on this head except the Read the Docs build. The PR's only docs change is docs/testing.md and lychee is green, so it looks unrelated, but it's worth confirming against main before merging.

[rominf]

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

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.

Seventh automated round. The previous round's commit (349289f), and 194c06b after it, were force-pushed away. This is a full review of the single commit at 8c6de71. This account holds no change request on this PR, and this round files none.

Both Blocking items are new and are not filed as a change request.

• The scenario gap: the lines it names are already run by unit tests, and AGENTS.md §3 accepts a reason in the PR text as the remedy. Merging leaves a convention gap, not wrong behaviour. The previous round treated the same kind of gap the same way.
• The bare-path citations: on their own they are below the gating floor, by the precedent set on #625.

Standing items from the previous round were not settled this round. The review was blind and did not re-find them, and they were not put back to the reviewer. Treat them as unverified, not as cleared:

• fix-13/14/22 verify lines had no scenario. diagnose-43..45 now cover the MSVC, GPU-override and PATH fixes, so fix-13/14 are probably covered and fix-22 is not visibly covered.
• fix-9's PowerShell-only line was not labelled as such.
• The "cannot regress unseen" wording: the sentence is still present at diagnose.rs:7450.

Red CI: E2E tests (Strix Halo, Ubuntu, release|nightly) fail on main (d59f596) with the same unexpected failure, dash-doctor-report-is-reviewed-before-sending. They do not bear on this PR. GitHub reports the branch as unstable.

Review — needs work

Full review of the whole change.
No ticket is named. The description and commit message match the diff in both directions, so neither under- nor over-describes it.

Blocking

• (new) The rocm diagnose output this change rewrites is covered by unit tests only, with no scenario and no stated reason — crates/rocm-core/src/diagnose.rs:811, :1104, :789, :1085, :1381, :1847, :1904, :1941, :2607, :2656; tests/e2e-cucumber/features/diagnose.feature:684-687

  • What changed in rocm diagnose output:
    • On Linux, the fix-2 verify after fix: line is now echo "${HSA_OVERRIDE_GFX_VERSION:-OK_UNSET}"; python3 -c ....
    • On Linux, fix-6 now prints rocminfo | head -n 5 alone.
    • On Windows, fix-2's clear command and the verify lines of fix-6, 9, 13, 14, 15, 21 and 22 are rewritten.
  • What the scenarios cover: diagnose-43, 44 and 45 run only rocm fix <id> --dry-run. The feature comment says outright that the rocm diagnose values are left to windows_diagnosis_remediation_passes_the_powershell_text_rule and the *_matches_the_catalog_copy unit tests.
  • Why this violates AGENTS.md §3: the rule says a unit test on the helper "does NOT discharge this". It also says a gated-lane scenario must be named in the PR text. The PR text calls the rocm diagnose Linux output for fix-2 and fix-6 a user-facing change, but names no scenario and no lane for it.
  • A Linux scenario is practical: the fix-2 checker reads HSA_OVERRIDE_GFX_VERSION from the examined environment, so rocm diagnose run with that variable set reaches it on a Linux lane.

  Confidence 85 · architectural · Fix: add a Linux-lane scenario that runs rocm diagnose with the override set, and one for the fix-6 Linux arm through the simulated host path, each asserting the printed verify line. For the Windows-only arms, either add a Windows-gated scenario or say in the PR text which lane covers them.

• (new) New citations in docs/testing.md are bare filenames, not relative links — docs/testing.md:1322, :1324, :1343

  • The added text cites "in fix.rs" and "in diagnose.rs" (twice) as bare backtick names.
  • diagnose.rs is not linked anywhere in the added paragraphs.
  • AGENTS.md §5 requires a citation the change adds to be a relative link, so the docs-links and lychee checks can see it go stale.

  Confidence 85 · mechanical · Fix: link [diagnose.rs](../crates/rocm-core/src/diagnose.rs), and either link or drop the repeated fix.rs mention.

Non-blocking

• (new) Comments added by this change name the wrong tests and scenarios — crates/rocm-core/src/fix.rs:1394-1400, fix.rs:3147-3150, diagnose.rs:7453-7455, tests/e2e-cucumber/fixtures/powershell_text_rule_cases.txt:6, docs/testing.md:1322, docs/testing.md:1344-1348

  • Windows-run tests: three comments say only the fix-2, 13, 14 and 15 lines are run in a shell. windows_recipes_pass_the_powershell_text_rule adds "the other catalog lines are not run". Yet the diff also adds the_outdated_msvc_verify_prints_a_version_and_fails_when_the_dll_is_missing (fix-21) and the_display_driver_verify_runs_in_cmd_and_powershell (fix-22). The tests govern, because they are what runs; docs/testing.md:1319 already lists all six.
  • Scenario range: the case-table header says "diagnose-42..44 steps". The scenarios are diagnose-43..45, and diagnose-42 is the stale-repo scenario. The feature file governs, because it holds the IDs the harness runs.
  • Test glob: the *_verify_runs_* glob in docs/testing.md does not match the fix-21 test or the_msvc_verify_exits_nonzero_when_a_dll_is_missing.
  • One mis-described test: docs/testing.md says the_override_linux_verify_runs_with_nothing_on_path runs the rocm diagnose Linux lines on a PATH of stand-in executables. It actually runs the catalog's verify_linux line on an empty PATH.

  Confidence 85 · mechanical · Fix: update the lists to match the tests and the scenario IDs.

• (new) Pre-existing, same pattern: the fix-2 Windows runner still prints the construct this PR removes from the paste lines — crates/rocm-core/src/fix.rs:2210

  • report_and_clear_override_windows prints Will run: setx HSA_OVERRIDE_GFX_VERSION "".
  • The runner itself works, because setx is spawned with no shell.
  • But the shown text is the exact line the case table flags. It also no longer matches the manual copy, which now deletes the variable through SetEnvironmentVariable(...,$null,'User').

  Confidence 70 · logic · Fix: print (or perform) the SetEnvironmentVariable form, or label the line as what the CLI runs rather than something to paste.

Decisions for the author

• Both verify lines are printed on every OS, the shared one first — tradeoff

  • Downside on Linux/WSL: the first line, labelled just Verify:, is the one the recipe's own notes say fails there: fix-2's python on stock Ubuntu, fix-6's hipcc on a runtime-only install. A note is then needed to send the user to the second line.
  • Alternative: print_recipe already knows current_os(), so it could print only this host's line and drop those notes.
  • Upside of the current choice: it keeps the plan identical on every host, as the # Linux: command lines already are, and it lets diagnose-44/45 assert both lines from a Linux lane.

• The catalog's fix-2 Windows Verify: needs Python, while its rocm diagnose twin does not — non-blocking-improvement

  • The diagnose.rs Windows arm verifies with powershell -NoProfile -Command "[Environment]::GetEnvironmentVariable('HSA_OVERRIDE_GFX_VERSION','User')".
  • The catalog uses python -c ..., which fails on a machine with no Python or only the Microsoft Store stub. The PR text acknowledges this under "Not verified".
  • A powershell -NoProfile -Command "..." form with no $ would run in both cmd and PowerShell without Python. Linux now has its own labelled line, so the plain line no longer has to run in bash.

Positive signals

• -ErrorAction Stop on the fix-15 and fix-21 Get-Item lines is a real correction.
  • Run in pwsh, (Get-Item ok,missing).VersionInfo | Select-Object ... exits 0 without the flag and 1 with it, so a missing DLL now fails the verify again.
  • The Windows-only tests check the missing-DLL exit as well as the success path.
• Holding both copies of the text rule to one shared case table is a sound answer to the crate-edge constraint. A worker compared the two copies condition by condition, and they agree on every branch.
• The Unix tests run the new Linux lines in /bin/sh on stand-in PATHs, each with a control showing that the old line fails there. That checks the notes' claims against a run, not a string.

Deployment notes

None

What this covered

• What was read: every file in the change, completely (prw-base…8c6de715, d59f596f…8c6de715). That includes the callers and siblings of the changed lines: print_recipe, act_on, plan_of, the fix-2 runners, catalog_manifest, the shared-id test, and the CHECKERS sweep.
• Fan-out: two independent workers, one on the coupled fix.rs/diagnose.rs group and one on the e2e, docs, agent-instruction and history passes. The second worker hit the subagent cap once and was re-spawned. The Step 7 design questions were answered by the driver, not by independent workers.
• Runs:
  • cargo test -p rocm-core --lib in the checkout's own target: 879 passed, 0 failed. Every new non-Windows test is listed by name.
  • The changed PowerShell lines were parsed with pwsh's parser, outer line and inner -Command body, with zero errors. Exit-code behaviour was checked with pwsh 7, not Windows PowerShell 5.1.
• Not run here:
  • The #[cfg(windows)] tests. The windows-build-and-test check is green for this head.
  • The e2e crate, which was not built locally. The CI E2E tests check is green, and diagnose-43..45 pass on both Strix Halo Ubuntu lanes.
• CI:
  • Every required check is green.
  • Two non-required lanes are red: E2E tests (Strix Halo, Ubuntu, release|nightly, current). Their failures are serve/chat/report scenarios on the same DevLab hosts, and both lanes are red on the last three prw-base commits too. They are not attributed to this change.
  • W7900 and MI300X were still queued.
• Sign-off: the sign-off check ran and passed: 1 commit checked, none missing.
• Did not run: the prior-changes pass, because earlier reviews are out of bounds for this run.
• Prompt injection: none found.
• Not reconciled against any PR discussion here.

[volen-silo]

Thanks for the seventh round. Checked each point against the code; the fixes are in 196b95d0 (still one commit, same base).

Blocking: no scenario for the rocm diagnose output. Agreed. Added diagnose-46 (fix-2: with HSA_OVERRIDE_GFX_VERSION set, rocm diagnose prints the exact verify after fix: line, with python3 and no bare python) and diagnose-47 (fix-6 on a simulated Ubuntu host prints rocminfo | head -n 5 with no hipcc). Both are @requires-os:linux and run on every Linux lane. I changed only production code (python3 back to python, && hipcc --version back on) in a WSL copy and both fail; 43 to 47 pass without the change. The Windows arms of rocm diagnose still have no scenario: no existing pattern makes one cheap (diagnose-38/39 mutate the real User PATH), so the PR text now says they are covered by windows_diagnosis_remediation_passes_the_powershell_text_rule plus the Windows-only run tests in windows-build-and-test.

Blocking: bare citations in docs/testing.md. Fixed: the fix.rs and diagnose.rs mentions are relative links.

Non-blocking, all valid and fixed: the comments and docs/testing.md now list the fix-21 and fix-22 run tests; the *_verify_runs_* glob is replaced by explicit test names (it missed two); the_override_linux_verify_runs_with_nothing_on_path is described as running the catalog's verify_linux line on an empty PATH; the fixture header says diagnose-43..45; the "cannot regress unseen" sentence is reworded to say a construct the rule does not list passes. The fix-2 Windows runner no longer prints setx HSA_OVERRIDE_GFX_VERSION "" as if it were a line to paste: it says the CLI runs those arguments, and the text is built from the same array that is passed to setx.

Left for the author: printing both verify lines on every OS versus only the host's is unchanged, and so is the suggestion to replace fix-2's Windows python -c.

Not run: diagnose-46/47 on Windows (they are Linux-only; run under WSL2 only). Two shared_runtime symlink tests fail locally with os error 1314 (privilege), unrelated to this change. The Ubuntu Strix Halo lanes fail on main the same way (dash-37), as the review notes.