[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 makes each printed line valid in cmd and in Windows PowerShell.

• 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.
• fix-6-path verify. It chained two commands with &&. It is now hipcc --version, which checks that the bin directory is on PATH and is valid in every shell. 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 verify. It was env | grep HSA_OVERRIDE_GFX_VERSION || echo OK_UNSET. It is now a python -c one-liner that no shell changes. The Linux arm of the checker prints the same line, and the Windows commands now tell the user that setx only affects new shells, so they should reopen the terminal before verifying.
• 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').

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: no && or ||, no bare where, no %NAME%, and inside a powershell -Command "..." body no \" and no $_. It is a text rule over known-bad constructs and does not prove a line runs. A unit test lists lines it must flag, including the old ones, and lines it must pass.
• Unit tests apply the rule to every Windows recipe (taken from applies_on, so a new recipe is covered) and to the Fix each Windows checker builds.
• Unit tests check the shape of the fix-15 and fix-2 lines. A test keeps the fix-2 diagnose copy equal to the catalog copy. A 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.
• Two Windows-only unit tests run the fix-15 and fix-2 lines through cmd and Windows PowerShell, fed on stdin as if typed.
• Scenarios diagnose-41 (diagnose-msvc-verify-line-text-is-powershell-safe), diagnose-42 (diagnose-override-fix-lines-are-powershell-safe) and diagnose-43 (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. The plan prints before the platform gate, so they run on the Linux lane. They check text and do not run the lines.

Test plan

• cargo fmt, workspace clippy -D warnings, cargo xtask catalog --check, the rocm-core tests (754 passed), cargo xtask e2e for the three diagnose scenarios, and the smoke script pass locally on the rebased head. Mutation spot-checks (reverting a printed line, an unescaped apostrophe, a changed fix-2 line) turn the matching test or scenario red.
• The new fix-6 (Windows), fix-13, fix-14 and fix-9 verify lines were typed into Windows PowerShell 5.1 and run from a .bat file in cmd, through WSL interop.

Not verified

• The two Windows-only unit tests were not compiled locally. The Windows CI job is their first run on this head.
• The fix-13 and fix-9 lines were not run against a real HIP SDK or GPU.
• No test runs the fix-6 PATH write, because it changes the user's Path.
• 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.
• 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.

Notes

Rebased on main after #710, which also changed the fix-6 PATH write. This change takes main's version of that line.

[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.