[volen-silo]

Problem

On Windows, rocm examine showed the AMD display driver as a name only, with no version or date. A laptop with a 2021 driver raised no warning in rocm examine or rocm diagnose, and rocm install driver only said to run rocm examine.

Cause

The inventory lists the adapter through pnputil, which has no driver version. The video controller query ran only when that list was empty. check_14 needs a HIP SDK and a failing hipInfo or a symptom, and it never compares dates.

Change

New output

• rocm examine prints driver_version, driver_date and driver_age on Windows when a driver version was read. With --json it adds adrenalin_date and an optional summary.driver.windows (version, date, age). Both are additive; contract_version is unchanged.
• A driver dated before 2023-07-27, the release of the first HIP SDK for Windows, gets advice only. The new print-only entry is fix-22-display-driver-old. It needs no HIP SDK, scores 25 (below the match threshold) and never sets has_match. Slot 20 belongs to info-20-gpu-no-rocm-support, so this entry uses 22. The catalog (doctor-catalog.json, reference.md) now has 29 entries.
• The date is only a floor. It cannot judge newer GPUs such as gfx120X, gfx1150 and gfx1151, which may need newer drivers. The docs, driver_age and the advice text say so. The outdated: no line in rocm install driver carries the same caveat; outdated: yes does not need it, because a date before the floor is old for every GPU.
• AMD documents no minimum driver version for the HIP SDK for Windows or for the Lemonade Vulkan backend. The text never says the driver is broken. The advice is AMD's support page and a restart. rocm-cli installs no drivers on Windows.
• The logic is in windows_driver.rs.

How the driver is read (what is and is not shared)

• On Windows, rocm examine, rocm diagnose and rocm install driver read the driver version and date through one function and one adapter choice (preferred_amd_display, which ranks a ROCm-supported discrete GPU above an integrated one). Each command makes its own query, so one run can succeed where another times out.
• rocm examine --json reads the driver twice: once for summary.driver.windows (with the CPU and RAM facts) and once for the adrenalin_* fields of the examination. On a host with more than one AMD adapter the two reads use the same selection rule, but they are separate queries. The windows_driver.rs module doc states this.
• Not covered by that shared read: the GPU driver: line of rocm version, and the detail fallback used when no inventory could be read at all. Both still use the older first-match PowerShell query. On a machine with an integrated and a discrete AMD adapter, rocm version can therefore name a different adapter's driver than rocm examine does. That query also does not exclude the Microsoft fallback driver (below), so on a machine running only that driver rocm version can still print it as the GPU driver.
• Startup and engine-selection paths use the fast inventory (InventoryScope::FAST) and start no driver query. ExamineSummary::gather() starts one extra PowerShell/CIM query on an AMD Windows host (measured about 2 seconds here, with a 5 second timeout). Most callers consume the result: the daemon, its MCP examine tool and sandbox, and the chat snapshot carry the whole summary, including driver.windows, into their JSON. Only the post-install host GPU check in runtime install finalize never reads the driver, and it now calls gather_without_driver_facts(). rocm examine --json and rocm diagnose still run the inventory twice (once in gather() and once for the adrenalin_* fields); fixing that needs a shared inventory handed into Examination, which changes the crate API, so it is left for a separate change.

Behavior changes beyond the new lines

• Microsoft Basic Display Adapter: it is treated as "no AMD driver". It is recognised by a name containing "Microsoft Basic Display" or a 10.0. driver version, and the name is localised, so on a non-English Windows only the version test applies. This follows from how Windows names the fallback driver and was not observed on a machine. Existing examine fields change with it: driver_status and driver_detail go from amd_display_driver_detected to not_detected and no detail for a host that runs only that driver. The input to check_14 takes the preferred adapter and drops the fallback driver, where it used to take the first AMD controller.
• WSL: "Linux and WSL are unchanged" does not hold for the host-driver probe. The shared PowerShell inventory script now also selects DriverDate, and the WSL host-driver probe (detect_wsl_host_driver, used by rocm version and the WSL checks) now excludes the Microsoft fallback driver, so it can report "no AMD display" where it used to report a 10.0.* version. The WSL path does not use the date. Linux native output is unchanged.
• The driver date is formatted in UTC. The CIM DriverDate is a local-time value for midnight UTC, so without the conversion a host west of UTC would print the previous day and could flag a driver dated exactly 2023-07-27.
• rocm install driver on Windows:
  • It prints the version, date and an outdated: yes/no/unknown verdict. It names fix-22-display-driver-old only when the verdict is yes, not in every run. When no version could be read it says so and points at rocm examine.
  • It installs nothing, with or without --yes or --dry-run. A pure should_execute gate is unit tested for this.
  • The plan's reboot_required changes from true to false, its check labels change from post_reboot_* to post_install_*, and the checks also list rocm diagnose. Linux and WSL plans are unchanged.
• rocm diagnose text on Windows: an advice-only row (today only fix-22) is never named "the highest-scoring match" and never replaces the upstream-routing text. With only fix-22 present, the "no known misconfiguration matched ... file upstream" block now prints first, followed by "Advice about this host follows ...", then the fix-22 entry. The "Next step" line comes from the first non-advice row. With a real cause present the report is as before. JSON (matched, has_match) is unchanged.
• Chat and the freeform and provider planners:
  • On Windows install driver is planned as a read-only call: no --yes, no approval, no "privileged or disruptive" wording. Linux and WSL keep approval and --yes.
  • Chat has a new in-process read-only handler for install driver. It runs the command as a dry run and accepts --dkms and --dry-run, and strips a model-supplied --yes and --yes=... the way the install sdk arm does. --reconcile is still refused there: it rewrites the saved driver install state, and it never worked from chat before this change either, because the approval path appended --yes, which the command rejects together with --reconcile.
  • The provider planner's approval check skips approval for a provider-proposed install driver on Windows only.
• rocm-doctor skill: SKILL.md gains a rule for advice-only entries (relay, never apply, keep routing upstream when has_match is false). The downstream copy of the skill needs its hand sync.

What "old" means

The check compares the driver date only with the release date of the first HIP SDK for Windows. It cannot fire on gfx120X, gfx1150 or gfx1151, which launched later. It is not a compatibility check.

Tests

• Unit tests cover date parsing and comparison (the boundary day, missing and unparsable values, non-Windows, current driver), the fallback driver, adapter choice, the UTC formatting of the date script, the fix-22 plan drift guards, the planner text next to what the command does on each platform, the Windows host check, and the gate that stops Windows from executing an install plan. A mixed rocm diagnose report (real cause below HIGH plus the old-driver row) covers the "Next step" selector.
• Scenarios for the Windows lane: examine-29, diagnose-41, driver-install-04 and driver-install-05. They run only on native Windows. examine-29 fails if a driver version is read with no date, so a silent date failure cannot leave all three green. Only the old-driver branch (a driver dated before 2023-07-27) has no end-to-end scenario, because it needs such a machine; unit tests cover it.
• README.md, reference.md, docs/testing.md, docs/manual-testing.md and docs/architecture.md are updated.
• The new Windows output is user-observable, and the scenarios above are its Gherkin coverage.

Verified locally

• Windows host (native), head of this branch: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test -p rocm-core --all-targets and cargo test -p rocm --bin rocm pass, and the four scenarios above pass against a real Windows AMD adapter. rocm examine printed a real version, date and age, and a real DriverDate was checked to be midnight UTC delivered as local time.
• WSL2 (Linux): cargo test -p rocm-core --lib and the driver and WSL-filtered tests pass.
• cargo test -p e2e-cucumber on Windows: 2 library tests that create symlinks fail with "Access denied" on this host (no symlink privilege). They do not touch this change.

Not verified

• The Basic Display Adapter case was not observed on a machine.
• The UTC conversion was checked by converting a real DriverDate into a UTC-8 zone in PowerShell, not on a host that is actually west of UTC.
• The old-driver branch end to end, as above.
• The Vulkan driver version is not read. No reliable read-only source was found.

Merge order

The verify-command fix (#677) touches the same files (the fix recipes, the catalog, reference.md and the fix counts). Whichever merges second needs a rebase and a recount.

[juhovainio]

Reviewed the whole diff end to end. This is a well-built addition: the CalendarDate/DriverAge comparison logic in the new windows_driver.rs module is correct and thoroughly tested (including the cutoff-day boundary and Windows' own "unset date" placeholders like 0001-01-01/1601-01-01), the fix-20 score is deliberately kept below the match threshold so it only ever shows as advice, and the catalog/fix.rs/doctor-catalog.json/reference.md/README entries are all mutually consistent. docs/testing.md also does the right thing by explicitly explaining why there's no e2e scenario here, rather than leaving the gap silent. The PR description already flags the merge-order coordination with #677/#679, so no need to repeat that.

Requesting changes on one real, non-blocking-in-the-common-case but inaccurate claim in the new module's own doc comment, left as an inline comment below: it says rocm examine and rocm diagnose "read the verdict from here, so the two cannot disagree" - that's true of the comparison logic, but not of the underlying facts. The two callers get their driver version/date from two separate PowerShell probes with different adapter-selection logic, so on a machine with more than one AMD display adapter they can disagree about which adapter's driver they're even looking at.

[juhovainio]

This guarantee only holds for the comparison logic in this module, not for the facts fed into it. rocm diagnose's check_20 reads Examination.adrenalin_version/adrenalin_date, populated by examine.rs's probe_adrenalin_windows via WIN_ADRENALIN_SCRIPT - a single CIM query that does Where-Object { PNPDeviceID -match 'VEN_1002' -or Name -match 'AMD|Radeon|Instinct' } | Select-Object -First 1, i.e. whichever matching adapter CIM happens to enumerate first.

rocm examine's rendering and rocm install driver's plan text instead read ExamineSummary.driver.windows, populated by host_gpu.rs's detect_driver_summary_with_windows_inventory -> a pnputil-first, video-controller-fallback pipeline that then picks the adapter via WindowsExamineInventory::preferred_amd_display(), which explicitly ranks a ROCm-supported discrete GPU above a recognized-but-unsupported iGPU (the comment there says this is so a gfx90c iGPU "cannot hide a discrete card").

These are two independently-run probes with two different selection algorithms. On exactly the multi-adapter machine preferred_amd_display() is designed to handle (an AMD iGPU plus a discrete AMD GPU), the two pipelines can pick different physical adapters and report different driver versions/dates for what a user perceives as one driver - which is the opposite of "cannot disagree." Worth either having rocm diagnose's check reuse the same inventory-based selection rocm examine uses, or softening this doc comment to describe what's actually guaranteed (shared comparison logic, not shared facts).

[tomastola]

🔴 Automated review · pr-review-watcher · e7aac6b

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.

Review — needs work

Full review of the whole change (one commit, 0cde0aa3…e7aac6bf).
No ticket named; the description notes this needs a rebase and recount after #677 and #679.

Blocking

Five findings, each posted as an inline comment on its first cited line:

1. rocm diagnose reads the driver from a different probe than rocm examine and rocm install driver, so they can disagree, though the code says they cannot.
2. The new video-controller query runs on every Windows inventory call, including the rocm serve, default-engine and chat pre-draw paths, and its comment says the opposite.
3. No Gherkin scenario covers the new Windows output, and the stated reason (no Windows lane) does not hold.
4. The wiring that puts driver facts into the examine summary, and the date-only re-query branch, are untested.
5. The two copies of the fix-20 plan already differ, and the drift guards were not extended to fix-20.

Non-blocking

• The Microsoft Basic Display Adapter would likely be reported as an "old AMD driver" — host_gpu.rs:36, examine.rs:3052
  With no AMD driver loaded, Windows falls back to the basic display driver. That adapter keeps PCI\VEN_1002, so both probes select it. I believe it reports DriverVersion 10.0.<build> with a placeholder DriverDate of 2006-06-21, which the year-before-2000 guard does not catch. This has not been checked on a Windows machine. If it holds, the user is told to update an AMD driver that is not installed. Suggest treating a 10.0. version or a Microsoft provider as "no AMD driver", not fix-20.

• An advice-only row replaces diagnose's upstream escalation in text output — diagnose.rs:2729-2737, :2815-2823, :2039
  fix-20 ignores the symptom, so on an old-driver host matched is never empty. The text report then drops "no known misconfiguration matched … file upstream" and ends with "run rocm fix fix-20-display-driver-old (the highest-scoring match)", whatever the user reported. JSON output is unaffected.

• The documented focused test commands miss 8 of the new tests — docs/testing.md:1163-1167
  The windows_driver, display_driver and windows_install_driver filters miss:

  • the three probe-output tests in examine.rs;
  • four host_gpu.rs tests (examine_prints_no_driver_age_lines_when_no_version_was_read, the_inventory_parser_reads_the_driver_date_and_drops_a_bad_one, an_adapter_missing_its_version_asks_for_a_second_query, a_version_less_adapter_gives_no_driver_info_and_is_filled_from_the_video_controller);
  • the_fix_id_the_install_text_names_is_the_one_diagnose_raises in the bin.

  The filters also pick up an unrelated diagnose test. Confirmed for the rocm-core filters with cargo test -p rocm-core <filter> -- --list.

• The docs give the cutoff as "before July 2023", but the code uses 2023-07-27 — docs/manual-testing.md:275, :279, :287; docs/testing.md:1172
FIRST_HIP_SDK_FOR_WINDOWS governs. A driver dated 2023-07-01 to 07-26 is flagged, while a tester following the doc would expect no flag.

• The README paragraph sits under the install sdk text — README.md:367-371
  "Driver installation" (README.md:440-443) still mentions Linux only.

• The rocm-doctor skill has no rule for advice-only entries — skills/rocm-doctor/SKILL.md step 1, reference.md:125
  The skill acts on has_match, which fix-20 never sets, so an agent following it silently drops advice that examine and the README present. Say whether to relay it.

• A stale count in a comment — fix.rs:2868
  "the other 23 fix-ids" should now be 26. It was already wrong before this change.

Decisions for the author

• A date cutoff reaches only some of the supported hardware — tradeoff
  The cutoff itself is right: AMD's ROCm 5.5.1 Windows Quick Start is dated 2023-07-27 (https://rocm.docs.amd.com/en/docs-5.5.1/deploy/windows/quick_start.html). That page also answers "Not verified: the cutoff date comes from press and discussion posts". The "no published minimum" claim also holds; AMD's pages only name the driver the HIP SDK installer bundles.
  The limit is reach. The check can never fire on gfx120X, gfx1150 or gfx1151, which all launched after mid-2023. It also misses the one driver issue AMD does document: lower performance on Strix Halo with 26.10-branch drivers. It still applies to RDNA2 and early RDNA3. Please confirm that reach is what you intend.
• Store the date as Option<CalendarDate> — non-blocking-improvement
  Today it is a String re-parsed by every age() call (windows_driver.rs:96-102, :134-139). The pub field allows a value that silently reads as Unknown.
• --json carries the version and date but not the verdict — non-blocking-improvement
  A consumer has to re-implement the cutoff itself.

Positive signals

• The install text is computed from the same WindowsDriverInfo as the verdict, and a test enforces the "minimum is not established" caveat in every statement.
• Years before 2000 are rejected, so the unset-date placeholder reads as unknown rather than "very old". Dates are formatted with InvariantCulture.
• fix-20's score is asserted below MIN_SCORE_FOR_MATCH, and its recipe is print-only, so it cannot flip has_match or the unattended install.
• The report redaction sweep gained sentinels for the new driver version and date.

Deployment notes

• rocm examine --json gains adrenalin_date and an optional summary.driver.windows. Both are additive; serde(default) keeps older consumers working.
• On Windows, rocm serve, chat and examine pay one extra PowerShell spawn (blocking finding 2).
• doctor-catalog.json grows to 27 entries. The downstream rocm-doctor skill copy needs its hand-sync (AGENTS.md §7).

What this covered

• Read: every file in the change, completely, at 0cde0aa3…e7aac6bf. Also the callers on the serve, chat and default-engine paths, the e2e workflows and feature files, and AGENTS.md.
• Passes: history, earlier review comments on these files, impact analysis, and a check of AMD's public docs.
• Ran: cargo test -p rocm-core --lib on this head (687 passed, 0 failed).
• Not run: nothing on Windows. The PowerShell paths and their timing are argued from the code. E2E tests (MI300X) had no result at review time.
• Not reconciled: this review was not reconciled against the PR's existing discussion.

[tomastola]

Blocking — rocm diagnose reads the driver from a different probe than rocm examine and rocm install driver, so they can disagree, though this comment says they cannot.

Locations: crates/rocm-core/src/windows_driver.rs:8-10, apps/rocm/src/driver_install.rs:827-828, crates/rocm-core/src/examine.rs:3052, crates/rocm-core/src/host_gpu.rs:1687-1693

• Two probes:
  • examine and install driver go through ExamineSummary::gather → preferred_amd_display(). That ranks adapters, putting a ROCm-capable discrete GPU above a gfx90c iGPU.
  • diagnose uses WIN_ADRENALIN_SCRIPT. It takes Select-Object -First 1 in WMI order, has no AdapterCompatibility clause, and uses an 8 s timeout instead of 5 s.
  • Only the verdict function is shared, not the facts it is given.
• How they disagree:
  • On an APU plus dGPU machine whose two drivers carry different dates, examine can print "run rocm diagnose (fix-20-display-driver-old)" while diagnose raises nothing, or the reverse.
  • If the inventory's video-controller query times out, install driver says the version "could not be read" while diagnose still raises fix-20.
• The rule: AGENTS.md §3 requires that a command named in remediation advice "actually clear the condition that printed it".
• The test can't catch it: the_fix_id_the_install_text_names_is_the_one_diagnose_raises builds Examination by hand and never renders the install text. It passes with the driver_install.rs change reverted.

Suggested fix: fill diagnose's adrenalin_* from the same inventory and preferred_amd_display selection. Then rename that test, or make it build both outputs from one probe result.

[tomastola]

Blocking — this second query runs on every Windows inventory call, including the startup paths, and the comment says the opposite.

Locations: crates/rocm-core/src/host_gpu.rs:2503-2511, :2727

• Why it always runs: push_windows_pnputil_display always sets driver_date: None, and pnputil is the first and normal source. So preferred_amd_display_misses_driver_facts() is true on every AMD host, and another PowerShell/CIM process (5 s timeout) starts each time. "A complete inventory costs nothing" never applies in practice.
• Who pays: detect_windows_examine_inventory is shared, so the cost lands on callers that never use the date:
  • detect_host_gpu_summary_fast (:3056, :3072), behind rocm serve and default-engine resolution;
  • chat_host_facts, whose header (apps/rocm/src/chat_host_facts.rs:15-19) says it must stay cheap because it runs before the dashboard draws;
  • detect_local_windows_host_driver (:509).

Suggested fix: fetch DriverDate only on the driver-summary path (gather / detect_driver_summary_with_windows_inventory), or add it to a query that already runs. Correct the comment either way.

[tomastola]

Blocking — no Gherkin scenario covers the new Windows output, and the reason given here does not hold.

Locations: docs/testing.md:1160-1172, docs/manual-testing.md:285-287, the "Tests" section of the description; nothing changed under tests/e2e-cucumber/features/

• What needs a scenario: AGENTS.md §3 asks for one when command output changes. This change adds:
  • three rocm examine lines;
  • a fix-20 row in rocm diagnose;
  • Windows install driver text promising "no driver is installed or changed". §3 names that kind of promise specifically.
• The Windows lane already exists:
  • e2e-gpu-strix-windows in .github/workflows/e2e-selfhosted.yml:650, whose path filter includes crates/rocm-core/** and apps/rocm/**;
  • it passed on this head;
  • @requires-os:windows scenarios already run on it (install_lifecycle.feature:136).
• What it can check today: only the "older" branch needs a machine with an old driver. The "not older" verdict, the three examine lines, and installs-nothing / no-execution-commands can all be asserted on that lane now.

Suggested fix:

• Add @requires-os:windows scenarios for examine and for install driver, both with and without --yes.
• Correct this paragraph, the one in docs/manual-testing.md, and the description, so that only the old-driver branch is named as the gap.

[tomastola]

Blocking — two pieces of the new wiring have no test: the code that puts driver facts into the examine summary, and the date-only re-query branch.

Locations: crates/rocm-core/src/host_gpu.rs:2331-2332, :1699-1700

• The summary wiring:
  • Every new examine test builds the summary through windows_driver_summary(…, info) directly.
  • This amd_display_driver_info call sits behind the runtime runtime_is_windows() check.
  • Replace it with None and every test stays green, while examine and install driver silently lose the version, date and age.
• The date-only branch:
  • an_adapter_missing_its_version_asks_for_a_second_query covers "no version" and "both present".
  • Delete || d.driver_date.is_none() and it stays green, yet that is the branch the pnputil path actually takes.

Suggested fix: extract a pure "driver summary from Option<&WindowsExamineInventory>" function, test it on every platform, and add a version-present / date-missing case.

[tomastola]

Blocking — the two copies of the fix-20 plan already differ, and the drift guards built for this were not extended to fix-20.

Locations: crates/rocm-core/src/diagnose.rs:2064-2066, crates/rocm-core/src/fix.rs:877-880

• The notes differ: this note adds "AMD publishes no minimum driver version for the HIP SDK or the Lemonade Vulkan backend." The catalog note in fix.rs stops after the first sentence, so rocm diagnose and rocm fix fix-20-display-driver-old print different notes for the same entry.
• The URL is duplicated: fix.rs hard-codes https://www.amd.com/en/support twice instead of using windows_driver::AMD_SUPPORT_URL.
• The guards skip fix-20: diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids and assert_plan_matches_the_catalog_copy exist for exactly this drift. Neither covers fix-20.

Suggested fix: make the notes identical, use the constant, and add fix-20 to both guards.

[tomastola]

Automated review · pr-review-watcher · 9fe6e07

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.

Review — needs work

Full review of the whole change at 9fe6e07c (one commit on fcc646e9). e7aac6bf, the commit my last review covered, is no longer on the branch, so this is not a diff against it.

Two blocking findings, posted as inline comments: diagnose-41 passes with the change reverted, and the check_22 placement. The third blocking point is in the body:

Blocking

• The description no longer matches the change — PR body, "Change" and "Tests"
  It names fix-20-display-driver-old; the code, catalog, README, skill reference, docs and tests use fix-22-display-driver-old. "No end-to-end scenario covers this" is no longer true: the change adds examine-29, diagnose-41, driver-install-04/-05, a Windows guard test and step files. "Needs a rebase and a recount" describes work that is already done. Fix: update the description; say only the old-driver branch lacks an end-to-end scenario.

Non-blocking

• docs/testing.md:1170 says "four modules"; the listed filters match five (diagnose, examine, fix, host_gpu, windows_driver). Say "five" or drop the number.
• Two comments narrate the replaced code — the doc comment on probe_adrenalin_windows in examine.rs, and windows_driver.rs:17-19. State only the current contract.

Decisions for the author

• A date cutoff reaches only some of the supported hardware — tradeoff (unchanged since last round). The 2023-07-27 cutoff can never fire on gfx120X, gfx1150 or gfx1151, which launched later, and it misses the one driver problem AMD documents (Strix Halo with 26.10-branch drivers). Please confirm that reach is intended.
• The new SKILL.md advice-only bullet does not say whether it overrides step 2 — non-blocking-improvement. skills/rocm-doctor/SKILL.md:121-125 says to relay a sub-threshold fix-22 entry; :166-169 says a lower cause may be mentioned "only if it also clears min_score_for_match". Add a pointer from step 2.
• On Windows the chat and freeform planners still frame install driver as a disruptive install — non-blocking-improvement. apps/rocm/src/main.rs:6984-6991 and :12320-12345 title it "Install driver", inject --yes and say "privileged or disruptive"; on Windows the command now only reports.
• driver_host_guard.rs greps step source text rather than testing behaviour, so it would pass if the helper's assertion were inverted.

Positive signals

• The Microsoft Basic Display Adapter is treated as "no AMD driver" (is_microsoft_fallback_driver), with tests.
• InventoryScope (FAST/SUMMARY/DRIVER_ONLY) and the WindowsInventorySources trait keep startup paths free of the extra query and let tests count queries on any platform.
• examine, install driver and diagnose read the driver through one function, amd_display_driver_info(); the fix-22 plan and URL are shared and both drift guards cover it.

Deployment notes

• rocm examine --json gains adrenalin_date and an optional summary.driver.windows; both are additive.
• doctor-catalog.json grows to 29 entries; the downstream rocm-doctor skill copy needs its hand-sync (AGENTS.md §7).
• Every ExamineSummary::gather() caller on Windows (including rocmd) can now start one extra PowerShell/CIM query; I did not check whether any is a hot path.
• windows-build-and-test, build-and-test, Test (affected crates), Build Linux PR binaries and Analyze (rust) had no conclusion when I read the checks; the cfg(windows) code was not compiled by anything I could see.

What this covered

• Read: windows_driver.rs, driver_install.rs and main.rs completely; for host_gpu.rs, examine.rs, diagnose.rs, fix.rs, report.rs, lib.rs and doctor-catalog.json the full diff and surrounding code, not every line; every hunk of the e2e files, docs, README and skills. Range fcc646e9…9fe6e07c.
• Ran (Linux only): cargo test -p rocm-core --lib on the touched modules (445 passed, 1 ignored), clippy clean, the e2e-cucumber guard/naming tests, cargo check of the e2e binary, and the five rocm bin tests for the install text.
• Did not run: anything on Windows; mutation/revert runs (revert verdicts are traced from the code); the history and prior-changes passes.
• Last round: all five earlier blocking objections are answered by the new code (one shared driver read, InventoryScope, pure tested summary wiring, shared plan with drift guards, four new scenarios). That was checked against my earlier wording after the findings were final.
• Not reconciled against the PR's existing discussion.

[tomastola]

Blocking — diagnose-41 passes with the change reverted, and its name overclaims.

Locations: tests/e2e-cucumber/features/diagnose.feature:627, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:2772 (assert_old_driver_advice_follows_the_inspection)

• The step computes old from windows_driver_record(&examine)… == Some("predates_first_hip_sdk"). Reverted, examine --json has no /summary/driver/windows, so old is false.
• Reverted, diagnose --json has no fix-22 row either, so assert_eq!(row.is_some(), old) is false == false and the scenario passes.
• On a current-driver lane, the only kind the feature comment says exists, old is never true, so it only checks that the advice is absent for a current driver.
• examine-29 does this correctly: it panics when the record is missing.

Confidence 85 · logic · Fix: have the step require the record as examine-29 does, so a reverted CLI fails it (if the record can legitimately be absent when no version was read, require it whenever examine's own text shows a driver_version: line). Keep the untested old-driver branch documented.

[tomastola]

Non-blocking — check_22 and its recipe sit against the repo's placement rule.

Locations: crates/rocm-core/src/diagnose.rs:2207-2246, crates/rocm-core/src/fix.rs (the fix-22 recipe), doctor-catalog.json:399 and :413

The comment at diagnose.rs:650 says: "A new checker goes at the END of this section, next to where its CHECKERS entry was appended." Here check_22 sits before check_21 (:2215 vs :2248), while CHECKERS appends it after. The recipe and catalog entry also precede fix-21, the reverse of the id order.

Confidence 85 · mechanical · Fix: move the constant and function after check_21, reorder the recipe and catalog to match, and re-run cargo xtask catalog --check.

[volen-silo]

Addressed in 7772b1f.

• diagnose-41 now fails when the CLI change is reverted. The step requires the Windows driver record, as examine-29 does. The old-driver branch itself is still untested end to end, and the docs say so.
• check_22, the fix-22 recipe and the catalog entry now follow the -21 entries. cargo xtask catalog --check passes.
• The PR description now describes the current change: fix-22, the four new scenarios, and that only the old-driver branch has no end-to-end scenario.
• docs/testing.md says five modules. The two comments that narrated replaced code now state the current contract. Step 2 of SKILL.md points to the advice-only fix-22 entry.
• driver_host_guard.rs now calls the host check with several OS names (windows passes; linux, macos and an empty name are refused), so an inverted check fails it. It still checks that the guard runs before the CLI call in both install steps, with CRLF-safe matching.

On the decisions:

• Date cutoff: it stays as it is. The docs, the doc comments and the advice text now say it is only a floor and cannot judge newer GPUs such as gfx120X, gfx1150 and gfx1151. A check for the Strix Halo 26.10 driver problem needs a verified source, so I left it for a follow-up.
• Planner wording: on Windows, the chat and freeform planners now treat install driver as read-only with no --yes and no "disruptive" wording, matching the command. A test asserts the planner text together with what the command does on each platform.

Not verified: nothing ran on Windows. The Windows scenarios and the cfg(windows) code run in CI.

[tomastola]

Automated review · pr-review-watcher · 7772b1f

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.

Review — needs work

Full review of the whole change at 7772b1f0 (one commit on 2d2dfc78). 9fe6e07c, the commit my last review covered, is no longer on the branch, so this is not a diff against it. Last round's description mismatch, the "four modules" count, the narrating comments and the planner wording are answered by this head; the recipe/catalog reordering from last round's placement point is done, which is what exposes blocker 1. Nothing was built or run locally; one finding is backed by CI, the rest are argued from the code.

Blocking

Three are inline: the catalog order (diagnose_steps.rs:130, CI-confirmed), the NoAmdDisplay regression (host_gpu.rs:511, traced by reading, not run), and the untested planner wiring (main.rs:8763). The fourth is here:

• chat_install_driver_action returns Result but can never fail, and part of it is dead — apps/rocm/src/driver_install.rs. It is always Ok; the !arg.starts_with("--yes=") clause cannot match, because --yes is a bool flag (main.rs:~798). Confidence 75 · mechanical · Fix: return the action directly and drop the clause.

Non-blocking

• The WSL probe skips the Microsoft-fallback exclusion the module docs say lives in one place — host_gpu.rs:540-556. amd_display_driver_info rejects 10.0.* / "Microsoft Basic Display"; detect_wsl_host_driver still returns Version("10.0.26100.1"), so check_wsl_6 can say "host AMD display driver 10.0.26100.1 is installed" when rocminfo sees no GPU. Confidence 70 · Fix: route the probe through amd_display_driver_info.
• The Windows install plan labels its checks post_reboot_checks: for a command that installs nothing — driver_install.rs:~893, render at :1592-1680. The plan keeps reboot_required: true, and the render code's own comment says that label would "tell the user to reboot for nothing". Confidence 70 · Fix: reboot_required: false on the validate-only plan.
• Provider-assisted planning still treats Windows install driver as approval-required — main.rs near 9360 (provider_planner_args_require_approval). Conservative, not unsafe, but the comments say every planner derives its flags from host_install_driver_only_reports. Route it through the helper or narrow the comment.
• rocm-doctor rationale is inaccurate — skills/rocm-doctor/reference.md:39-40, SKILL.md:121-125 say the advice-only entry "is shown to the user by rocm examine and the README"; examine prints only a driver_age line and an agent cannot see the README. Say "relay the entry's title and evidence as advice".
• docs/testing.md filters claim "and nothing else" — the new cross-platform planner test matches none of the listed fragments, while display_driver / driver_fact also match the startup-path tests (names grepped, --list not run).
• diagnose-41 — raised last round, still open. The title ("appears only for an old driver") overclaims: on a current-driver host the step only proves the advice is absent. Separately, the score check at diagnose_steps.rs:~2796 compares Option<i64> < Option<i64>, so a missing score passes. Retitle and expect both values.
• The old-driver branch has no end-to-end seam — examine-29, diagnose-41 and driver-install-04 run against whatever driver the lane has, so breaking assess_date, the Predates arm or the cutoff stays green; only the windows_driver.rs unit tests cover it. The description discloses this.
• driver_host_guard.rs:12-35 — raised last round, unchanged. It matches the literal require_native_windows(); in step source, so a comment satisfies it.
• CalendarDate::parse is looser than its doc — 2021-+1-+4 and 2023-02-31 parse. The cutoff ordering is unaffected.

Decisions for the author

• A PowerShell query on every ExamineSummary::gather call — tradeoff. Callers (main.rs:2081, :2714, :2839, :5003; chat.rs:3320; driver_install.rs:49, :84, :147) now pay up to the 5 s WINDOWS_INVENTORY_QUERY_TIMEOUT on AMD Windows hosts, and examine --json runs the inventory twice. One shared source of truth against startup/snapshot paths that don't print the driver; gathering once per run or giving gather a scope would do both. I did not check how often the codex bridge snapshot runs.
• The PR bundles several changes — non-blocking-improvement. Catalog entry, WindowsInventorySources refactor, three scopes, the WSL probe change and the is_advice_only text rule; the last two are outside the stated scope (AGENTS.md §11).
• is_advice_only and the platform decision are stringly-typed — non-blocking-improvement. A hard-coded id and an os == "windows" shared by three sites, with the plan builder on examine.os and the planners on the compile-time host OS; they agree today.

Deployment notes

• Adds print-only fix-22-display-driver-old (29 catalog entries); contract_version unchanged; JSON additions are adrenalin_date and summary.driver.windows.
• #677 touches the same files; whichever merges second needs a rebase and recount, including the CATALOG_FIX_IDS order.

What this covered

• Read: every changed file at pr/680, completely, except host_gpu.rs (diff, parser, merge, preferred-adapter, pnputil/video and WSL code, not all ~5,000 lines), main.rs (every changed or driver-related region and consumers), and diagnose_steps.rs / driver_steps.rs (changed regions and neighbours). Range 2d2dfc78…7772b1f0.
• CI when read: E2E tests and E2E tests (Strix Halo, WSL2) failing (the second not opened); Windows and two Ubuntu Strix Halo lanes in progress; MI300X and W7900 queued; the rest passed.
• Did not run: anything locally; mutation/revert runs; a full history pass.
• Not reconciled against the PR's existing discussion beyond removing points already raised.

[tomastola]

Blocking — CATALOG_FIX_IDS is in the wrong order, so diagnose-34 fails on every platform.

RECIPES (fix.rs) and doctor-catalog.json put fix-21-msvc-redist-outdated before fix-22-display-driver-old (the reorder from last round), and rocm fix --json emits that order. The assert at diagnose_steps.rs:2506 is an ordered assert_eq!, so the swapped pair fails. The scenario has no platform tag. CI confirms it: https://github.com/ROCm/rocm-cli/actions/runs/38910065163/job/117006436496 — "Then the catalog names every entry and what the CLI does with each … the published catalog and the documented one disagree". The listing step (assert_catalog_complete) is order-insensitive, so it hides this.

Confidence 100 (CI) · mechanical · Fix: put fix-21-msvc-redist-outdated first, then fix-22-display-driver-old.

[tomastola]

Blocking — detect_local_windows_host_driver can no longer answer NoAmdDisplay. Traced by reading the code and origin/main; not run.

The call moved from the FAST inventory to DRIVER_ONLY (host_gpu.rs:2577), which sets system: false. On a Windows host with no AMD adapter no display source returns anything and no CPU/RAM is fetched, so is_empty() (:1651) is true, assemble_windows_inventory returns None (:2612-2653) and this returns Unreachable. On main, the CPU/RAM data made the inventory non-empty and the probe returned NoAmdDisplay. check_wsl_6_host_driver_too_old abstains when the host is unreachable, so its "host reports no AMD display adapter" branch can no longer fire. No test calls this function; the DRIVER_ONLY test uses a host that has an adapter.

Confidence 85 · logic · Fix: make assemble_windows_inventory distinguish "all sources failed" from "sources answered, no adapters", or keep the system query for this caller.

[tomastola]

Blocking — the planner wiring that decides read-only vs. approval on Windows is untested.

This call and chat.rs:2525-2528 pass host_install_driver_only_reports(), but planners_describe_install_driver_as_the_command_behaves_on_each_platform calls the helpers directly with an only_reports value it computes itself, and hybrid_planner_driver_action_includes_yes_for_approved_execution now calls plan_driver_request(.., false) instead of build_freeform_plan("install the linux driver…"). Hard-coding false in either caller, or comparing the wrong OS in the helper, stays green. The same gap covers the cfg(windows) scope wiring in host_gpu.rs (gather, detect_windows_amd_driver_info, probe_adrenalin_windows): nothing asserts which scope each passes. On Windows the chat and freeform planners now drop the approval wording and --yes on this one decision (AGENTS.md §3, §7).

Confidence 85 · logic · Fix: put the OS and the inventory sources behind a parameter or trait so the callers can be tested.

[tomastola]

Automated review · pr-review-watcher · 1302127

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.

Review: needs a rebase, otherwise answered

All four blocking points from my last review are fixed, and so are almost all the non-blocking ones. One new blocking point: this PR's tests no longer compile on current main, and git does not report it as a conflict.

Blocking

• The PR's new tests don't compile on current main. File: apps/rocm/src/driver_install.rs, five calls at roughly lines 3026, 3145, 3184, 3199 and 3218 of the merged file.
  • #705 (4cd89639, merged to main on 10-19) removed the test-only build_driver_install_plan wrapper. This PR's new tests call it five times.
  • The textual merge is clean, so neither the GitHub UI nor git merge-tree shows a conflict.
  • On a trial merge with main, cargo test -p rocm --bin rocm fails with 5 × E0425 cannot find function build_driver_install_plan. Plain cargo build succeeds.
  • Fix: rebase and call build_driver_install_plan_at(&examine, "/etc/os-release", "", true, …). I made exactly that change in a scratch tree and all 10 focused rocm binary tests passed.
  • The rebase also clears the one red check (see CI below).

What my last review raised, and where it stands

Blocking:

• CATALOG_FIX_IDS order: fixed. fix-21 now comes before fix-22, and diagnose-34 passes on every lane, including Windows.
• NoAmdDisplay regression: fixed.
  • detect_local_windows_host_driver uses the full inventory (InventoryScope::SUMMARY) again, so the CPU/RAM answer keeps a host with no AMD adapter from reading as unreachable.
  • The mapping moved into a pure function, local_windows_host_driver_probe. A new test covers no adapter, no answer at all, and an adapter present, and it pins that the driver-only inventory reads as empty.
• Untested planner wiring: fixed for the planners.
  • build_freeform_plan_on takes the platform decision as a parameter.
  • the_production_planner_… and the_chat_entry_point_… go through the real entry points and assert results that differ by host OS. A hard-coded value would fail on Linux or Windows CI, and the Windows job (cargo test --workspace --all-targets) ran both and passed.
  • The cfg(windows) scope choice is still unasserted per caller. See non-blocking.
• chat_install_driver_action returned a Result that could never fail: fixed. It returns the action directly, and the dead --yes= clause is gone.

Non-blocking:

• WSL probe skipped the Microsoft-fallback exclusion: fixed. Both host-driver probes now read amd_display_driver_version(), which applies is_microsoft_fallback_driver, and the Basic Display test asserts it.
• reboot_required on the Windows plan: fixed. It is false, the label reads post_install_checks:, and the test asserts that post_reboot is absent.
• Provider-assisted planning: fixed. provider_planner_args_require_approval makes Windows install driver read-only, with a test.
• rocm-doctor rationale: fixed. The skill now says to relay the entry's title and evidence. It also notes that step 2's threshold rule does not apply to this entry.
• docs/testing.md "nothing else" claim: fixed. The text now admits neighbouring tests match, and the filter list includes the new tests.
• diagnose-41: fixed. It is retitled ("agrees with the inspection"), and a missing score or threshold now fails the check.
• driver_host_guard.rs: fixed for whole-line comments. A trailing comment (foo(); // require_native_windows();) would still satisfy it. That is a nit; I'm not asking for a change.
• CalendarDate::parse too loose: fixed. It accepts digits only and checks days per month, including the leap-year century rule, with tests.
• No end-to-end test of the old-driver branch: unchanged, and the description says so.

Non-blocking (new or remaining)

• Nothing asserts which inventory scope each cfg(windows) caller passes. This is the rest of last round's blocker 3.
  • Each scope is now one named one-line wrapper (…_with_driver_facts / detect_windows_driver_inventory / detect_windows_examine_inventory), and the new test documents why the local probe needs SUMMARY.
  • Changing detect_local_windows_host_driver back to the driver-only wrapper would still stay green, because those wrappers compile only on Windows and no test calls them.
  • I accept this as is.
• NoAmdDisplay still depends on the CPU/RAM query succeeding. If pnputil answers with no AMD adapter but that query fails, the result is still Unreachable. check_wsl_6 then abstains, which is the safe direction.
• An --apply exclusion that matches nothing. provider_planner_args_require_approval skips the read-only path when --apply is present, but I found no --apply flag on install driver. It is the same kind of dead clause as the --yes= one removed this round. Drop it, or say what it guards.

Decisions from last round with no reply

None of these block the merge:

• An extra PowerShell query on every ExamineSummary::gather call, and the inventory running twice under examine --json.
• The PR bundles the WSL-probe change and the is_advice_only rule with the feature.
• The platform decision and is_advice_only are compared as strings.

CI at 13021270

• Failing: E2E tests (Strix Halo, Windows, current). 149 scenarios: 145 passed, 3 were expected failures (xfail), and 1 failed unexpectedly: diagnose-38.
  • The step asserts that setx must not appear, but the base's fix-6 recipe still prints setx PATH ….
  • This PR doesn't touch fix-6. #710 (0acf698d) fixed it on main, and main's Windows lane passes diagnose-38 since then (runs 39244601953 and 39389535923).
  • The rebase above clears it.
• Passing on the Windows lane: the new scenarios (examine-29, diagnose-41, driver-install-04, driver-install-05) and diagnose-34.
• Every other check passes: windows-build-and-test, build-and-test, MI300X, W7900, WSL2 and both Ubuntu Strix Halo lanes.

Deployment notes

• Unchanged from last round:
  • adds the print-only fix-22-display-driver-old (29 catalog entries);
  • contract_version is unchanged;
  • the JSON additions are adrenalin_date and summary.driver.windows.
• #677 is still open and touches the same files. Whichever merges second needs a rebase and a recount.

What this covered

• Read: the range-diff 7772b1f0 → 13021270, every hunk; the inventory-scope code in host_gpu.rs; the Windows CI job logs for the unit tests and the e2e suite; main's Windows lane history for diagnose-38.
• Ran, on a trial merge of the PR head into current main:
  • the rocm-core filters from docs/testing.md, plus local_host_driver_probe and calendar: 39 passed;
  • cargo test -p e2e-cucumber --test driver_host_guard: 2 passed;
  • the rocm binary tests failed to compile (the blocker above). After the one-line-per-call fix, the 10 focused binary tests passed.
• Not run: anything on Windows (the Windows results above are from CI), the full workspace suite, and mutation runs.

[volen-silo]

Addressed in 04bfbdb, rebased on current main.

• The new tests compile again: the five calls now use build_driver_install_plan_at. The rebase also picks up the main change that fixes diagnose-38 on the Windows lane.
• The dead --apply exclusion in provider_planner_args_require_approval is removed. install driver has no such flag.
• Resolving the rebase conflict in chat.rs: the chat action enum and chat_rocm_command_action_from_args are pub(crate) again so the tests can reach them.

On the Windows lane for the previous head, examine-29, diagnose-41, driver-install-04 and driver-install-05 passed.

[tomastola]

Review: needs work — two real new bugs survived a full rewrite; everything from the last round is fixed.

Reviewed the whole diff fresh (not just the rebase delta) since the branch was rewritten. All three points from my last review (5591397855) are resolved: the five tests compile again with build_driver_install_plan_at, and the rebase cleared the one red CI check — everything is green on 04bfbdb5.

Three new blocking issues, each small and fixable:

1. Chat can no longer run install driver on Windows at all. chat_install_driver_action now correctly returns a read-only action there, but the in-process dispatcher it's routed through (run_rocm_read_only_in_process) has no case for install driver — only install sdk --dry-run — so it errors with "unsupported in-process read-only rocm command" instead of reporting. (chat.rs:2435, :3440, :3649)
2. The Microsoft Basic Display Adapter case still reads as "detected." The new driver_version/date/age fields correctly treat it as no AMD driver, but the older driver_status/driver_detail fields (still read by rocm examine and install driver's reconciliation output) don't apply the same exclusion, so they still say amd_display_driver_detected. (host_gpu.rs:1702, :1722, :2538; driver_install.rs:210)
3. A new advice line has no test. "Advice about this host follows... nothing in it is applied automatically" (diagnose.rs:3005) isn't asserted anywhere — exactly the kind of CLI-behavior promise AGENTS.md §3 asks to be tested alongside the behavior.

One non-blocking nit: the long "no documented minimum" note is copy-pasted in fix.rs:926/diagnose.rs:2302 instead of hoisted to a shared constant like its neighbor FLOOR_NOTE — a drift-guard test already protects against this diverging, so low risk.

Everything else holds up well: the date-comparison logic, the platform-decision plumbing, and the Windows e2e message+state coupling are all solid. No action needed on the cutoff-reach tradeoff from earlier rounds — still the same open, accepted decision.

[volen-silo]

Addressed in 80f4a25.

• Chat install driver on Windows: the in-process read-only dispatcher now handles install driver (with --dkms only). It runs as a dry run that never executes anything. install_driver_read_only_call_runs_in_process covers it, and a flag it does not know is still refused.
• Microsoft Basic Display Adapter: driver_status and driver_detail now apply the same exclusion as the new fields, so they read not_detected, and the reconciliation output follows. The existing fallback test asserts it.
• The advice line is now asserted together with its promise: every command in the advice is a printed comment, in the same test.
• The "no documented minimum" note is one shared constant used by the fix recipe and the diagnose copy.

Not verified: the changes are checked on Linux; the Windows lane will exercise them.

Addressed in deee2cb. The module comment now says what is guaranteed (one comparison function, and one device inventory and adapter choice for examine, install driver and diagnose), and what is not (separate queries can fail differently). rocm examine now takes the driver facts from the same inventory selection as the other two commands.

[juhovainio]

Thanks for the thorough work here, the strict date parsing, the scoped inventory sources and the fact that Windows can never mutate anything are all solid. One thing needs to change before this merges, plus a handful of smaller items.

Blocking: the PR description does not match what the code does

• "Linux and WSL are unchanged" is not accurate. The shared WSL inventory script now selects DriverDate, and the WSL host-driver probe now excludes the Microsoft fallback driver (crates/rocm-core/src/host_gpu.rs, around lines 527, 552 and 1748).
• "One inventory and one adapter choice" only holds within a single command. rocm examine reads the driver twice (the summary uses InventoryScope::SUMMARY, probe_adrenalin_windows uses DRIVER_ONLY; the windows_driver.rs module doc concedes the two can disagree). rocm status and the fallback detail line still use the old first-match query (host_gpu.rs ~2496-2504 and ~2559, apps/rocm/src/main.rs ~1647), so on an iGPU plus dGPU host status can print a different driver than examine.
• Behaviour changes that are not mentioned in the description:
  • amd_display_driver_detail now returns None for the Basic Display Adapter, so driver_status and driver_detail go from "detected" to "not_detected" in existing examine output.
  • The input to check_14 now takes the preferred adapter and drops the fallback driver, where it used to take the first AMD controller.
  • The Windows install plan's reboot_required went from true to false, the check labels changed from post_reboot_* to post_install_*, and the checks now also list rocm diagnose.
  • The rocm diagnose text rendering changed (is_advice_only / answers_symptom): with only fix-22 present, the upstream-routing block now prints before the advice.
  • Chat has a new in-process read-only arm for install driver, and the provider planner has a new approval bypass on Windows.
• "rocm install driver prints the fix id" reads as unconditional, but it is only printed for an old driver.

Either amend the description to cover these, or narrow the code. None of the code changes look wrong to me, they just need to be called out so reviewers and release notes see them.

Non-blocking, inline comments cover these: driver date timezone (N1), the missing newer-GPU caveat on outdated: no, extra Windows subprocesses on callers that never show the driver, chat flag handling, a stale count comment, a dead assertion, and a misleading doc comment.

Non-blocking, no inline anchor:

• Nothing runnable on Linux pins the gate that makes Windows install nothing (apps/rocm/src/driver_install.rs:56). The plan tests only assert plan fields and install_driver() is never called, so if the !plan.supported || !plan.mutating check were dropped, Windows --yes would write state.json and print "driver install completed" with only the Windows lane to catch it. Extracting a pure should_execute(plan, yes, dry_run) would make this unit-testable.
• The date "27 July 2023" is hardcoded in fix.rs (fix-22 rationale), README.md, skills/rocm-doctor/reference.md, docs/testing.md and docs/manual-testing.md. One assertion in the existing recipe test tying it to FIRST_HIP_SDK_FOR_WINDOWS would keep them in sync.
• The "Next step" selector change in diagnose.rs (find(|d| !is_advice_only(&d.id))) is not tested with a mixed report. The existing mixed test uses a 75-point fix-21, so it takes the HIGH branch. A LIKELY or WEAK real cause plus the old-driver row would cover it.
• The date half of examine-29, diagnose-41 and driver-install-04 passes when the date probe returns nothing (a missing date maps to "unknown" and diagnose then expects old=false). If the new PowerShell date script returned empty on the runner, all three would stay green. Asserting the date is present whenever the version is present in examine-29 would close that.
• docs/testing.md filter list omits three modified tests (diagnosis_remediation_matches_the_fix_catalog_for_shared_fix_ids, needs_reboot_true_fix_ids_match_the_known_set, every_recipe_id_is_unique_and_covers_the_catalog).
• driver-install-04/05 have no @requires-gpu tag but 04's third step needs an AMD driver, so it would fail on a Windows runner with no AMD adapter. Low risk on the current lanes.
• windows_driver.rs is not listed in the docs/architecture.md module list (not enforced).
• Design points to confirm: the fallback-driver heuristic (name contains "microsoft basic display" or version starts with "10.0.") is unverified on Windows, and the name is localized so only the version check helps on non-English Windows; fix-22 is identified as advice-only by a hardcoded id with no marker in the catalog JSON; needs_reboot is true on a recipe that installs nothing.

CI: all checks pass except "E2E tests (MI300X, current)", which fails on therock-next-09-live-install-reports-vllm-rocm10x-discovery-pins (uv cannot resolve vllm against amd-aiter==0.1.24.post1). The same job and scenario fail identically on main at the merge-base, so I am treating it as unrelated to this PR.

Also note #677 touches the same fix recipes, catalog and counts, so whichever lands second will need a rebase and recount.

Limits of this review: I read the Windows-only code by eye and did not run anything on Windows, so the date finding below is unconfirmed.

[juhovainio]

DriverDate.ToString('yyyy-MM-dd', Invariant) has no .ToUniversalTime(). The WMI/DMTF driver date carries a UTC offset, and as far as I recall Get-CimInstance hands back a local-time DateTime, so on a machine west of UTC the printed date can be a day earlier than Device Manager shows. That matters at the boundary: a driver dated exactly 2023-07-27 on a UTC-7 host would read 2023-07-26 and trigger fix-22 wrongly. I could not confirm this on Windows, so please verify on a host with a negative timezone offset. If it reproduces, $gpu.DriverDate.ToUniversalTime().ToString(...) should fix it.

[juhovainio]

outdated: no here drops the caveat that age_summary (above) carries via FLOOR_NOTE: the date cannot judge gfx120X, gfx1150 or gfx1151. A gfx1151 user with a driver dated just after 2023-07-27 will read outdated: no as a clean bill of health, which goes against the 'printed claims must be true as stated' rule. Suggest appending FLOOR_NOTE in this branch, and in the Predates branch and the diagnose evidence string for consistency.

[juhovainio]

The comment above says summary callers show the driver, but several ExamineSummary::gather callers never print it (rocmd/common.rs:36, rocmd/mcp.rs:569, rocmd/sandbox.rs:192, chat.rs:3330, runtime_install_finalize.rs:1541). On an AMD Windows host rocm examine goes from about 3 spawns to about 5, and each of pnputil and the video query has a 5s timeout, so a slow host adds seconds to examine, diagnose, the MCP examine tool and the bridge snapshot. Consider giving gather() a scope parameter so only printing callers pay for the extra query, and adding DriverDate to the existing WIN_GPU_SCRIPT instead of re-running the inventory in probe_adrenalin_windows (examine.rs:3203). The startup paths still use FAST and the the_startup_paths_do_not_start_the_driver_fact_query test pins that, which is good.

[juhovainio]

This arm accepts only --dkms, so install driver --dry-run, --reconcile or --yes=true from chat or MCP now hits bail!("unsupported in-process read-only rocm command") on Windows. Before this PR those became an approval that ran in a subprocess. chat_install_driver_action (line 2440) also only strips an exact --yes, whereas the install sdk arm also strips --yes= prefixes. Suggest accepting --dry-run in this arm and stripping --yes= the same way the sdk arm does.

[juhovainio]

The comment above still says '21 bare-metal/Windows entries ... plus the 7 WSL ones', but the assert is 29. With fix-22 added it should read 22 (22 + 7 = 29).

[juhovainio]

This negative assertion can never fail: the renderer emits execution_commands:, never a bare execution: line. It looks copied from the WSL step at line 67, where it has a different meaning. The other assertions already cover the point, so this can be dropped.

[juhovainio]

The doc comment says this reads 'the one probe result that rocm examine and rocm install driver also read', but each command runs its own inventory query (the host_gpu.rs comments around 2720-2721 admit this). Worth rewording so the next reader does not assume a shared cached fact.

[tomastola]

Review: the last round is answered, nothing blocking

This covers what changed after my last review at 04bfbdb5. The branch was rewritten twice. 80f4a25a holds the fixes (79 lines in 5 files). deee2cb3 rebases that onto current main (1c33248c, 6 commits ahead). The rebase changed only context lines in README.md and skills/rocm-doctor/reference.md, plus the Linux-only / Linux + WSL lists, which now take main's move of fix-17. Nothing was built or run locally. The points below come from reading the code and from CI on deee2cb3.

What my last review raised, and where it stands

Blocking:

• Chat could not run install driver on Windows: fixed.
  • run_rocm_read_only_in_process has a new branch for install driver (chat.rs:3776-3789). It calls install_driver(paths, dkms, false, /*dry_run*/ true), so it can never execute a plan.
  • It accepts --dkms and no other flag.
  • install_driver_read_only_call_runs_in_process checks both: the call runs in process, and an unknown flag is refused.
• The Basic Display Adapter read as "detected" in the old driver_status/driver_detail fields: fixed where the inventory is read.
  • amd_display_driver_detail (host_gpu.rs:1703) now applies is_microsoft_fallback_driver.
  • examine and the install driver reconciliation get not_detected and no detail.
  • The existing fallback test asserts both (host_gpu.rs:5440).
• The new advice line had no test: fixed. diagnose.rs:7017-7036 asserts the exact line, and in the same test asserts that every command in the advice is a # comment. That ties the "nothing is applied automatically" promise to what the code does.

Non-blocking:

• "No documented minimum" note was copied in two places: fixed. Both places now use windows_driver::NO_MINIMUM_EXPLANATION, like FLOOR_NOTE.

The description's claims still match the head. For example, the catalog has 29 entries in doctor-catalog.json and in reference.md.

Suggestions (non-blocking)

• install driver --dry-run from chat on Windows fails (chat.rs:3783). Traced by reading, not run.
  • --dry-run is a real flag on install driver (main.rs:810).
  • On Windows, chat_install_driver_action removes only --yes and returns the rest as a read-only call.
  • The in-process branch accepts only --dkms. So when the model asks for install driver --dry-run, the user gets "read-only assistant command is not implemented in-process" where they should get the report.
  • Before this PR, that call went through the approval path and worked.
  • Suggestion: accept --dry-run in the same rest.iter().all(..) test. The call is already a dry run, so the flag changes nothing.
• The Microsoft-fallback exclusion still misses one path.
  • detect_windows_amd_display_driver (host_gpu.rs:2559) is the raw PowerShell probe. It does not apply is_microsoft_fallback_driver.
  • It feeds rocm_core::detect_gpu_driver_version(), which prints GPU driver: … at main.rs:1647.
  • It is also the fallback_detail that windows_driver_summary_from_inventory uses when there is no inventory at all.
  • On a host running only the Basic Display Adapter, that line can still name the Microsoft driver as the GPU driver. This came before the PR and is outside the three commands it covers.
  • Suggestion: filter the probe's result through the same predicate, or say in the description that only examine, diagnose and install driver are covered.

CI

Every check is green on deee2cb3 except E2E tests (MI300X, current), which has nothing to do with this change.

• The failing scenario is therock-next-09-live-install-reports-vllm-rocm10x-discovery-pins. uv finds no version set it can install for the vLLM reinstall.
• #724 and #725 fail on the same lane today with the same scenario, plus two vLLM serve scenarios.

The Windows lane passed, which covers examine-29, diagnose-41, driver-install-04/-05 and the cfg(windows) build.

Decisions for the author

• The cutoff only reaches some GPUs: a date check can never fire on gfx120X, gfx1150 or gfx1151. This was accepted in earlier rounds and the docs say so. Nothing more is needed.

[volen-silo]

@juhovainio thanks for the thorough review. The branch is now rebased onto current main and re-signed, so the commit ids below are new. Head is 955d82e2.

Blocking: description did not match the code. I rewrote the PR description against the code. It now covers the WSL inventory change (DriverDate selected, fallback driver excluded in detect_wsl_host_driver), that rocm examine --json reads the driver twice, that rocm version and the detail fallback still use the older first-match query, the driver_status/driver_detail change for the Basic Display Adapter, the check_14 input, the install-plan reboot_required and check-label changes, the rocm diagnose ordering change, the chat and provider-planner changes, and that the fix id is printed only when the verdict is yes. It also lists what I did not verify.

Inline comments

• Driver date timezone (host_gpu.rs): fixed in dd40db80. The inventory script now calls .ToUniversalTime(), and a test pins the script text. I reproduced the shift by converting a real DriverDate into a UTC-8 zone in PowerShell, but I did not run this on a host that is actually west of UTC.
• Stale count comment (fix.rs): fixed in 955d82e2. It now reads 22 + 7 = 29.
• Dead execution: assertion (driver_steps.rs): removed in 955d82e2. Nothing in the plan renderer prints a bare execution: line.
• Doc comment saying the probe result is shared (examine.rs): reworded in ca001a61 to say each command makes its own read.
• Chat/MCP install driver flags: not changed in this round. The description lists it as a known gap. Accepting --dry-run and stripping --yes= in that arm is small; do you want it in this PR or as a follow-up?
• Extra Windows queries from ExamineSummary::gather() callers: not changed; the description now says so. A scope parameter is a larger change, so I would do it in a follow-up unless you want it here.
• outdated: no without the newer-GPU caveat: not changed; the description states it. I can append FLOOR_NOTE in this PR if you prefer.

Non-blocking points without an inline anchor

• Windows never executes an install plan: a pure should_execute(plan, yes, dry_run) now carries the gate, with a unit test (2ea7f9b1).
• The hardcoded "27 July 2023" is tied to FIRST_HIP_SDK_FOR_WINDOWS by an assertion in the recipe test (832d4821).
• The "Next step" selector has a mixed-report test, a real cause below the HIGH threshold plus the old-driver row (4623fba6).
• examine-29 now requires a yyyy-MM-dd date whenever a version was read (8a3dcf28).
• docs/testing.md lists the three missing test names, and windows_driver.rs is in docs/architecture.md (ca001a61).
• Not changed: the @requires-gpu tags on driver-install-04/-05, and the fallback-driver heuristic, which was not observed on a machine and is listed under "Not verified".

On the overlap with #677: agreed, whichever lands second needs a rebase and a recount. The description says so under "Merge order".

[volen-silo]

@juhovainio follow-up on the three points I left open in my last comment. Head is now 9a5e4e95. I measured and read the code for each one instead of taking them on trust, and I did not agree with all of it.

Chat/MCP install driver flags: done in b82f34f1.

• --dry-run was a real regression, so the in-process Windows arm now accepts it. --yes=true and a bare --yes from the model are stripped the way the install sdk arm does it, in the read-only and approval paths.
• I left --reconcile refused. It rewrites the saved driver install state, and it never worked from chat even before this PR: the approval path appended --yes, which the command rejects together with --reconcile. So that is not a new regression.
• Tests: chat_install_driver_strips_a_model_supplied_yes_in_both_forms and install_driver_read_only_call_accepts_dry_run_and_refuses_reconcile. Both fail with the two code hunks reverted.

outdated: no without the newer-GPU caveat: done in 56ef99ec, for the no branch only.

• outdated: no now ends with FLOOR_NOTE, and the new test asserts the printed text together with the underlying NotOlder state, including the 2023-07-27 boundary day.
• I did not add it to the Predates branch or the diagnose evidence string. A date before the floor is old for every GPU, so outdated: yes is true as stated. The diagnose finding fires only for old drivers, and its notes already carry FLOOR_NOTE. Tell me if you still want it there.

Extra Windows queries from gather() callers: partly done in 9a5e4e95, and I disagree with the premise for most callers.

• I measured with a logging shim around pnputil and powershell. gather() spawns two processes: pnputil (about 0.1 s) and one PowerShell video-controller query (about 2.0 to 2.4 s here). I could not reproduce the 3 to 5 spawns for rocm examine; it was 2 here. Total wall time was too noisy to quote, so only the PowerShell figure is solid.
• The MCP examine tool, the bridge snapshots, the sandbox snapshot and the chat snapshot all carry the whole ExamineSummary, including driver.windows, into their JSON or answers. Dropping the query there would remove a field from a contract surface, so I did not.
• The one caller that never reads the driver is the post-install host GPU check in runtime_install_finalize.rs. It now calls a new ExamineSummary::gather_without_driver_facts(). This saves one PowerShell query per install finalize. The test only checks that the GPU target fields match the full summary, so the saving itself rests on the measurement and on the existing source-counting test for the startup paths.
• I did not add DriverDate to WIN_GPU_SCRIPT. It would put the adapter choice back on a second algorithm, which is what your first review point asked to avoid.
• Still open, and left for a separate change: rocm examine --json and rocm diagnose run the inventory twice (once in gather() and once in probe_adrenalin_windows). Fixing that needs a shared inventory handed from host_gpu into Examination, which changes the crate API.

Checked locally on the Windows host before pushing: cargo fmt --check, cargo clippy --workspace --all-targets -- -D warnings, cargo xtask check-crate-edges, cargo test -p rocm-core --all-targets and cargo test -p rocm --bin rocm; the examine-29, diagnose-41, driver-install-04 and driver-install-05 scenarios pass against a real AMD adapter; and cargo test -p rocm-core --lib passes under WSL2. I updated the PR description to match: the chat arm, the caveat and the gather() notes.

[juhovainio]

No blocking issues from a read of the diff. The earlier chat install driver --dry-run / --yes= gap is fixed, and the newer-GPU caveat next to outdated: no is in. Non-blocking notes are inline: the lean-gather test can't prove the fast path is used, the manual-testing DriverDate claim differs between local time and UTC, and the 10.0.* fallback-driver heuristic is unverified on real hardware. The PR description is also stale (it still says outdated: no has no caveat and lists the chat --dry-run as a known gap), and the bundled scope plus overlap with #677 will need a rebase and recount. This reflects a diff-only read; CI was still pending when I checked and the Windows GPU lane is not exercised here, so please wait for green before merging.

[juhovainio]

Nothing tests that this branch picks the fast inventory. the_summary_without_driver_facts_keeps_the_gpu_target passes if gather_without_driver_facts fetches the driver facts anyway (on Linux both sides are None; on Windows the assertion allows lean == full). Consider a small pure scope selector that is unit-tested, or an injected inventory source.

[juhovainio]

This assertion allows lean.driver.windows == full.driver.windows, so it cannot detect that the lean path still ran the video-controller query. It also runs the real gather() twice. Assert lean.driver.windows.is_none() on an AMD Windows host, or test the scope mapping directly.

[juhovainio]

Get-CimInstance ... DriverDate prints a local-time DateTime, while rocm examine formats the date in UTC. West of UTC the two differ by a day, so "Both match" is not true everywhere. Reword, or print the date in UTC here (and in the fix-22 verify command).

[juhovainio]

Any version starting 10.0. is treated as the Microsoft fallback driver regardless of adapter name. The docs and tests say this was recalled, not observed, and it also changes legacy driver_status/driver_detail and the WSL host probe. Consider confirming on a machine with only the Basic Display driver, or splitting it out.

[nowycondro]

Automated high-recall review: 7 inline findings posted on touched lines. Two additional findings fall on code the diff doesn't actually change (so GitHub won't anchor an inline comment there) and are added as a follow-up comment below.

[nowycondro]

Legacy fallback_detail path can report the Microsoft Basic Display Adapter as an AMD driver.

When the new multi-source inventory (windows_inventory) is entirely None (every new query failed/timed out), windows_driver_summary_from_inventory falls back to the legacy detect_windows_amd_display_driver() single first-match PowerShell query. That legacy query never calls is_microsoft_fallback_driver, so on a host where the new pipeline totally fails but this old query still succeeds and matches the fallback driver (by name or 10.0.x version), rocm examine would report driver_status: amd_display_driver_detected with a driver_detail naming a driver that isn't actually an AMD driver — while driver.windows/driver_version/driver_date stay empty, since info is unconditionally None in this branch (windows_inventory.and_then(...) short-circuits on None).

This contradicts the PR's stated goal that a host running only the fallback driver reads as having no AMD driver. Note: the PR description does disclose this gap for rocm version's GPU driver: line, but this is the same underlying legacy query also feeding rocm examine's detail-only fallback — worth confirming that's the intended scope, since rocm examine/rocm diagnose/MCP examine all consume this same ExamineSummary.

[nowycondro]

Unverified 10.0. version-prefix heuristic could misclassify a real AMD driver as "no driver".

is_microsoft_fallback_driver treats any driver version starting with 10.0. as the Microsoft fallback driver (not a real AMD driver), causing amd_display_driver_info()/amd_display_driver_version() to return None for it. The module's own doc comment admits this is "recalled from how Windows reports this driver, not observed" and "has not been checked on a Windows machine." AMD's driver major version numbering has varied significantly over its history (this repo's own fixtures span 30.x–32.x), so it isn't established that no real AMD driver build ever reported a 10.0.x version string. If one exists, a genuine installed AMD driver would silently be reported as "no AMD driver detected" by rocm examine, rocm diagnose, and rocm install driver alike, since all three route through this one check.

Worth at least a code comment flagging this as a risk area to watch for in the field, or gating the version-prefix check more narrowly (e.g. combined with a known-fallback PNP device ID) if one is available.

[nowycondro]

The HIGH_CONFIDENCE "Next step" branch doesn't exclude advice-only entries, unlike the sibling else if branch three lines below.

if let Some(high) = report.matched.iter().find(|d| d.score >= HIGH_CONFIDENCE) {
    let _ = writeln!(out, "Next step: run `rocm fix {}`.", high.id);
} else if let Some(best) = report.matched.iter().find(|d| !is_advice_only(&d.id)) {

The else if branch was deliberately updated to filter out advice-only rows (e.g. fix-22-display-driver-old) so they're never named as "the highest-scoring match." But the first (HIGH_CONFIDENCE) branch has no such filter. Today this is harmless only because DISPLAY_DRIVER_OLD_SCORE = 25 is far below HIGH_CONFIDENCE = 75 — nothing in the code ties the two together. If a future advice-only check (the module doc suggests more may come) is ever given a score ≥ HIGH_CONFIDENCE, this branch would print "Next step: run rocm fix <advice-only-id>" as if it answers the symptom, which is exactly the problem is_advice_only was introduced to prevent in the other branch. Consider applying the same !is_advice_only filter here too, for symmetry and future-proofing.

[nowycondro]

is_advice_only hardcodes a single fix-id rather than a general property on Diagnosis.

fn is_advice_only(fix_id: &str) -> bool {
    fix_id == crate::windows_driver::DISPLAY_DRIVER_OLD_FIX_ID
}

Diagnosis has no kind/advice_only field; "advice-only" is expressed purely as (a) a score tuned below MIN_SCORE_FOR_MATCH, plus (b) this id-equality check consulted at two call sites in render_report_text. The two mechanisms aren't linked by the compiler or any assertion. A future advice-only entry needs its author to remember to extend this function by name and keep both call sites in sync; forgetting either silently lets an advice row "answer" the symptom. A kind: FindingKind (or advice_only: bool) field set once where the checker builds its Diagnosis would make this structural instead of a growing whitelist — worth considering now while there's only one entry to migrate, even though it's arguably minor with just one instance today.

[nowycondro]

amd_display_driver_version re-implements the preferred-adapter + fallback-filter lookup a third time.

amd_display_driver_detail (line 1703), amd_display_driver_info (line 1732), and this method each independently call preferred_amd_display() and re-apply the Microsoft-fallback-driver exclusion with slightly different code shapes. This method's result is logically just self.amd_display_driver_info().map(|i| i.version) — same adapter lookup, same fallback exclusion, same trimming — so it could delegate instead of hand-rolling the filter a third time, removing one of the three near-duplicate copies and the risk of them drifting apart (e.g. if the fallback-exclusion condition is ever tweaked in one copy but not the others).

[nowycondro]

Driver date goes through an unnecessary parse → format → re-parse round trip to reach this checker.

Examination::set_windows_driver (examine.rs) takes an already-parsed WindowsDriverInfo (with a structured Option<CalendarDate>), formats the date back to yyyy-MM-dd text, and stores it as the plain adrenalin_version/adrenalin_date strings on Examination. check_22_display_driver_old then calls WindowsDriverInfo::from_probe on those same two strings, re-parsing the date it had as a structured value a few lines of code earlier. DriverSummary.windows on ExamineSummary already stores the structured WindowsDriverInfo directly and avoids this; Examination could carry the same Option<WindowsDriverInfo> (or just the parsed CalendarDate) alongside/instead of the two raw strings, letting this checker use the value directly rather than reparsing text produced from a CalendarDate moments before.

[nowycondro]

Duplicated --yes stripping logic between this new function and the pre-existing install sdk arm.

args.retain(|arg| arg != "--yes" && !arg.starts_with("--yes="));

This exact statement also appears in the install sdk arm of chat_rocm_command_action_from_args further down in this file. Both sites exist for the same reason (stripping a model-supplied --yes/--yes=... before the command may later append its own bare --yes), and this function's own doc comment even says "as the install sdk arm does" — acknowledging the duplication rather than factoring it out. A shared fn strip_model_supplied_yes(args: &mut Vec<String>) helper would keep the logic (and the reasoning behind the exact-match-plus-prefix form, tied to clap's current rejection of an attached value on a bare bool flag) in one place for future edits.

[nowycondro]

Two more findings from the same review pass. Both sit on code this PR doesn't modify, so they can't be anchored as inline diff comments:

1. apps/rocm/src/chat.rs, the MCP examine/doctor tool handler (around line 3335)

"examine" | "doctor" => {
    let examine = ExamineSummary::gather()?;
    let text = render_examine_text()?;
    Ok(internal_mcp_tool_success(text, serde_json::json!(examine)))
}

render_examine_text() internally calls ExamineSummary::gather() a second time (via render_examine_text_with_paths → examine_human_report, which returns (String, ExamineSummary) specifically so callers can avoid re-probing, but is private to main.rs and unreachable from here). This double-gather predates this PR and was cheap before it, but this PR makes each gather() on a Windows+AMD host run a new ~2s PowerShell/CIM driver query, so a single MCP examine/doctor call now costs roughly double that (~4s) where it used to be negligible. Worth exposing examine_human_report (or equivalent) so this handler gathers once and reuses the result for both the JSON and the text.

2. apps/rocm/src/driver_install.rs, install_driver/execute_driver_install_plan (around line 56 and line 91)

install_driver() calls ExamineSummary::gather() for the pre-install plan, the post-execution re-check closure (|| ExamineSummary::gather().map(|summary| summary.driver)) calls it again, and reconcile_driver_install calls it a third time on a later invocation. Each call re-runs CPU/RAM/WSL detection in addition to the driver query, even though only the driver fields can plausibly change across a driver install/reconcile. Since this PR made gather() noticeably more expensive on Windows+AMD, the post-install/reconcile re-checks might be better served by a driver-only probe (e.g. detect_windows_driver_inventory/detect_windows_amd_driver_info, already added in host_gpu.rs for rocm diagnose) instead of a full re-gather().

[nowycondro]

🤖 Automated review (Claude Code) · 9a5e4e9

Note on scope: I was asked to review at head 955d82e2, but by the time I ran, the PR had already moved forward to 9a5e4e95 (a small follow-up addressing juhovainio's --dry-run/caveat/gather() comments). I reviewed the actual current head, 9a5e4e955a13d0e25f25960c7f536e654099a25a, and focused on what changed since the last approval (deee2cb3 → 9a5e4e95), plus a general pass over the whole diff.

This supplements the review I already posted (review #5655966524, 7 inline comments) and a follow-up comment (#6422336302, 2 more findings on untouched-but-related code). Summary, most important first:

Non-blocking (no hard blockers found)

1. crates/rocm-core/src/host_gpu.rs (~L2440) — the legacy single-query fallback path (used only when every new multi-source inventory query fails) never applies the Microsoft-fallback-driver exclusion, so on an inventory-failure + Basic-Display-Adapter host it could still report "amd_display_driver_detected" with no version. Confidence: medium — plausible but narrow (requires both inventory failure and no AMD driver).
2. crates/rocm-core/src/windows_driver.rs (~L229) — is_microsoft_fallback_driver's 10.0.-prefix heuristic is explicitly unverified on real hardware (per the module's own doc / PR's "Not verified" section); if any genuine AMD driver ever reports a 10.0.* version, it would be misclassified as "no driver". Confidence: low-medium, already flagged by the author as unverified.
3. crates/rocm-core/src/diagnose.rs (~L3130, ~L3026) — the HIGH_CONFIDENCE "Next step" branch doesn't filter advice-only entries the way the sibling branch does, and is_advice_only is a single hardcoded fix-id rather than a structural property on Diagnosis. Currently harmless (fix-22 scores well below HIGH_CONFIDENCE) but has no guard against a future advice-only check being scored higher. Confidence: medium, cosmetic/future-proofing.
4. apps/rocm/src/chat.rs (~L3335, MCP examine/doctor handler) and apps/rocm/src/driver_install.rs (~L56/L91, install/reconcile flow) — both call ExamineSummary::gather() more than once per logical operation; this PR makes each such call materially more expensive on Windows (~2s PowerShell/CIM query), turning previously-cheap double-gathers into multi-second ones. Confidence: medium, a perf/efficiency concern rather than correctness.
5. apps/rocm/src/chat.rs (~L2444) and crates/rocm-core/src/host_gpu.rs (~L1748) — minor duplication: the --yes/--yes= stripping logic is copy-pasted from the install sdk arm rather than shared, and the preferred-adapter/fallback-filter lookup is reimplemented a third time instead of delegating to the existing amd_display_driver_info. Confidence: medium, maintainability only.
6. crates/rocm-core/src/diagnose.rs (~L2332) — the driver date is parsed, formatted to a string, stored on Examination, then re-parsed back into a CalendarDate for check_22; an unnecessary round-trip rather than carrying the structured value through. Confidence: high (straightforward to verify by reading), low severity.

No correctness-blocking issues found in the deee2cb3 → 9a5e4e95 delta itself (the chat --dry-run fix and the outdated: no / FLOOR_NOTE addition both look correct and are each covered by a new test per the author's commit messages).

What I read / verified: the full PR diff (27 files), the windows_driver.rs/host_gpu.rs/diagnose.rs/chat.rs/driver_install.rs logic in detail, and the prior review history (juhovainio/tomastola rounds) to avoid re-litigating settled points. What I could not verify: no Rust toolchain available in this environment, so nothing was built, linted, or tested — this is a static read of the diff only, consistent with several of the human reviewers' own disclosed limitations (no Windows hardware).

🤖 Generated with Claude Code

[nowycondro]

🤖 Automated review (Claude Code) · 9a5e4e9

I read the diff for this PR (Windows driver version/date detection, fix-22-display-driver-old, and the related examine/diagnose/install-driver changes) plus the surrounding windows_driver.rs, host_gpu.rs, fix.rs, and diagnose.rs logic, independently of the existing long review thread.

Two items worth double-checking on the Windows driver-version/date path: (1) crates/rocm-core/src/host_gpu.rs around line 2575 — when the structured Windows driver inventory returns nothing at all (e.g. pnputil/video-controller/pnp-entity all fail/time out), the fallback appears to call the older unfiltered detect_windows_amd_display_driver() and trust it directly, which would bypass the new Microsoft-Basic-Display-Adapter exclusion this PR otherwise establishes everywhere else — on a host running only that fallback driver, driver_status/driver_detail could still report amd_display_driver_detected in this one path. Medium confidence, worth a human check against the actual fallback branch. (2) crates/rocm-core/src/windows_driver.rs around line 226 — is_microsoft_fallback_driver treats any version string starting with 10.0. as the fallback driver regardless of adapter name, and WindowsDriverInfo::from_probe (called with no adapter name) relies on this version-prefix test alone; a genuine AMD/OEM driver that happens to report a 10.0.* version would be silently dropped with no diagnostic trail. Lower confidence (plausible edge case, not confirmed against real hardware) but worth a comment noting the intentional trade-off if it's already known.

Non-blocking notes: rocm examine --json runs two separately-timed Windows driver probes (adrenalin_* via Examination::probe_with_interpreter and summary.driver.windows via ExamineSummary::gather()) that could theoretically disagree under transient flakiness — the PR description already acknowledges this as a known, deliberately-scoped-out limitation, so this is just a confirmation rather than a new finding. 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 (which the PR's own test comment says has already caught one divergence) — low risk given the test exists, but a shared constant would remove the possibility entirely. I did not have a Windows host or build toolchain available, so none of this was verified by compiling or running the test/scenario suite; the analysis is from static reading of the diff and surrounding code only.

🤖 Generated with Claude Code