Repository navigation
feat(examine): show the AMD driver version on Windows and flag old drivers - #680
volen-silo wants to merge 1 commit into
Conversation
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
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.
| //! | ||
| //! Two callers need the same answer. `rocm examine` prints it, and | ||
| //! `rocm diagnose` raises [`DISPLAY_DRIVER_OLD_FIX_ID`] from it. Both read | ||
| //! the verdict from here, so the two cannot disagree. |
There was a problem hiding this comment.
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
left a comment
tomastola
left a comment
There was a problem hiding this comment.
🔴 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:
rocm diagnosereads the driver from a different probe thanrocm examineandrocm install driver, so they can disagree, though the code says they cannot.- 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. - No Gherkin scenario covers the new Windows output, and the stated reason (no Windows lane) does not hold.
- The wiring that puts driver facts into the examine summary, and the date-only re-query branch, are untested.
- 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 keepsPCI\VEN_1002, so both probes select it. I believe it reports DriverVersion10.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 a10.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 hostmatchedis never empty. The text report then drops "no known misconfiguration matched … file upstream" and ends with "runrocm 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
Thewindows_driver,display_driverandwindows_install_driverfilters miss:- the three probe-output tests in
examine.rs; - four
host_gpu.rstests (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_raisesin 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 three probe-output tests in
-
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_WINDOWSgoverns. 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 sdktext —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.mdstep 1,reference.md:125
The skill acts onhas_match, which fix-20 never sets, so an agent following it silently drops advice thatexamineand 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 aStringre-parsed by everyage()call (windows_driver.rs:96-102,:134-139). Thepubfield allows a value that silently reads asUnknown. --jsoncarries 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
WindowsDriverInfoas 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 fliphas_matchor the unattended install. - The report redaction sweep gained sentinels for the new driver version and date.
Deployment notes
rocm examine --jsongainsadrenalin_dateand an optionalsummary.driver.windows. Both are additive;serde(default)keeps older consumers working.- On Windows,
rocm serve, chat andexaminepay one extra PowerShell spawn (blocking finding 2). doctor-catalog.jsongrows to 27 entries. The downstreamrocm-doctorskill 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 --libon 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.
| //! The AMD display driver on Windows: what version and date it has, and | ||
| //! whether it is old. | ||
| //! | ||
| //! Two callers need the same answer. `rocm examine` prints it, and |
There was a problem hiding this comment.
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:
examineandinstall drivergo throughExamineSummary::gather→preferred_amd_display(). That ranks adapters, putting a ROCm-capable discrete GPU above a gfx90c iGPU.diagnoseusesWIN_ADRENALIN_SCRIPT. It takesSelect-Object -First 1in WMI order, has noAdapterCompatibilityclause, 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,
examinecan print "runrocm diagnose(fix-20-display-driver-old)" while diagnose raises nothing, or the reverse. - If the inventory's video-controller query times out,
install driversays the version "could not be read" while diagnose still raises fix-20.
- On an APU plus dGPU machine whose two drivers carry different dates,
- 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_raisesbuildsExaminationby hand and never renders the install text. It passes with thedriver_install.rschange 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.
| { | ||
| inventory.merge_missing_from(pnp); | ||
| } | ||
| // `pnputil` lists the adapter but not its driver version or date, so an |
There was a problem hiding this comment.
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_displayalways setsdriver_date: None, and pnputil is the first and normal source. Sopreferred_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_inventoryis shared, so the cost lands on callers that never use the date:detect_host_gpu_summary_fast(:3056,:3072), behindrocm serveand 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.
| reconcile path is also non-privileged. It refreshes passive checks and persists | ||
| the `total`/`present`/`missing` check summary without running DKMS commands. | ||
|
|
||
| Windows display driver checks (`fix-20-display-driver-old`) are covered by unit |
There was a problem hiding this comment.
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 examinelines; - a fix-20 row in
rocm diagnose; - Windows
install drivertext promising "no driver is installed or changed". §3 names that kind of promise specifically.
- three
- The Windows lane already exists:
e2e-gpu-strix-windowsin.github/workflows/e2e-selfhosted.yml:650, whose path filter includescrates/rocm-core/**andapps/rocm/**;- it passed on this head;
@requires-os:windowsscenarios 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:windowsscenarios forexamineand forinstall 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.
| } | ||
| }); | ||
| return windows_driver_summary(detail); | ||
| let info = windows_inventory.and_then(WindowsExamineInventory::amd_display_driver_info); |
There was a problem hiding this comment.
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_infocall sits behind the runtimeruntime_is_windows()check. - Replace it with
Noneand every test stays green, whileexamineandinstall driversilently lose the version, date and age.
- Every new examine test builds the summary through
- The date-only branch:
an_adapter_missing_its_version_asks_for_a_second_querycovers "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.
| needs_reboot: true, | ||
| fix_id: id.to_owned(), | ||
| verify: "powershell -NoProfile -Command \"Get-CimInstance Win32_VideoController | Where-Object { $_.Name -like '*AMD*' -or $_.Name -like '*Radeon*' } | Select-Object Name,DriverVersion,DriverDate\"".to_owned(), | ||
| notes: vec
|
Addressed in 7772b1f.
On the decisions:
Not verified: nothing ran on Windows. The Windows scenarios and the |
tomastola
left a comment
tomastola
left a comment
There was a problem hiding this comment.
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_actionreturnsResultbut can never fail, and part of it is dead —apps/rocm/src/driver_install.rs. It is alwaysOk; the!arg.starts_with("--yes=")clause cannot match, because--yesis 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_inforejects10.0.*/ "Microsoft Basic Display";detect_wsl_host_driverstill returnsVersion("10.0.26100.1"), socheck_wsl_6can say "host AMD display driver 10.0.26100.1 is installed" when rocminfo sees no GPU. Confidence 70 · Fix: route the probe throughamd_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 keepsreboot_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: falseon the validate-only plan. - Provider-assisted planning still treats Windows
install driveras approval-required —main.rsnear 9360 (provider_planner_args_require_approval). Conservative, not unsafe, but the comments say every planner derives its flags fromhost_install_driver_only_reports. Route it through the helper or narrow the comment. rocm-doctorrationale is inaccurate —skills/rocm-doctor/reference.md:39-40,SKILL.md:121-125say the advice-only entry "is shown to the user byrocm examineand the README";examineprints only adriver_ageline and an agent cannot see the README. Say "relay the entry'stitleandevidenceas advice".docs/testing.mdfilters claim "and nothing else" — the new cross-platform planner test matches none of the listed fragments, whiledisplay_driver/driver_factalso match the startup-path tests (names grepped,--listnot 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 atdiagnose_steps.rs:~2796comparesOption<i64> < Option<i64>, so a missingscorepasses. Retitle andexpectboth values.- The old-driver branch has no end-to-end seam —
examine-29,diagnose-41anddriver-install-04run against whatever driver the lane has, so breakingassess_date, thePredatesarm or the cutoff stays green; only thewindows_driver.rsunit tests cover it. The description discloses this. driver_host_guard.rs:12-35— raised last round, unchanged. It matches the literalrequire_native_windows();in step source, so a comment satisfies it.CalendarDate::parseis looser than its doc —2021-+1-+4and2023-02-31parse. The cutoff ordering is unaffected.
Decisions for the author
- A PowerShell query on every
ExamineSummary::gathercall — tradeoff. Callers (main.rs:2081,:2714,:2839,:5003;chat.rs:3320;driver_install.rs:49,:84,:147) now pay up to the 5 sWINDOWS_INVENTORY_QUERY_TIMEOUTon AMD Windows hosts, andexamine --jsonruns the inventory twice. One shared source of truth against startup/snapshot paths that don't print the driver; gathering once per run or givinggathera 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,
WindowsInventorySourcesrefactor, three scopes, the WSL probe change and theis_advice_onlytext rule; the last two are outside the stated scope (AGENTS.md §11). is_advice_onlyand the platform decision are stringly-typed — non-blocking-improvement. A hard-coded id and anos == "windows"shared by three sites, with the plan builder onexamine.osand 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_versionunchanged; JSON additions areadrenalin_dateandsummary.driver.windows. - #677 touches the same files; whichever merges second needs a rebase and recount, including the
CATALOG_FIX_IDSorder.
What this covered
- Read: every changed file at
pr/680, completely, excepthost_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), anddiagnose_steps.rs/driver_steps.rs(changed regions and neighbours). Range2d2dfc78…7772b1f0. - CI when read:
E2E testsandE2E 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.
| "fix-wsl-7-wsl1", | ||
| "fix-18-comgr-conflict", | ||
| "fix-19-shm-too-small", | ||
| "fix-22-display-driver-old", |
There was a problem hiding this comment.
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.
| let Some(inventory) = detect_windows_examine_inventory() else { | ||
| // This one reads the driver version, which `pnputil` never reports, so it | ||
| // asks for the driver facts. It is not a startup path. | ||
| let Some(inventory) = detect_windows_driver_inventory() else { |
There was a problem hiding this comment.
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.
| return plan_driver_request( | ||
| trimmed, | ||
| &lower, | ||
| driver_install::host_install_driver_only_reports(), |
There was a problem hiding this comment.
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.
7772b1f to
1302127
Compare
tomastola
left a comment
tomastola
left a comment
There was a problem hiding this comment.
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 tomainon 10-19) removed the test-onlybuild_driver_install_planwrapper. This PR's new tests call it five times. - The textual merge is clean, so neither the GitHub UI nor
git merge-treeshows a conflict. - On a trial merge with
main,cargo test -p rocm --bin rocmfails with 5 × E0425cannot find function build_driver_install_plan. Plaincargo buildsucceeds. - 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 focusedrocmbinary tests passed. - The rebase also clears the one red check (see CI below).
- #705 (
What my last review raised, and where it stands
Blocking:
CATALOG_FIX_IDSorder: fixed.fix-21now comes beforefix-22, anddiagnose-34passes on every lane, including Windows.NoAmdDisplayregression: fixed.detect_local_windows_host_driveruses 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_ontakes the platform decision as a parameter.the_production_planner_…andthe_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_actionreturned aResultthat 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 appliesis_microsoft_fallback_driver, and the Basic Display test asserts it. reboot_requiredon the Windows plan: fixed. It isfalse, the label readspost_install_checks:, and the test asserts thatpost_rebootis absent.- Provider-assisted planning: fixed.
provider_planner_args_require_approvalmakes Windowsinstall driverread-only, with a test. - rocm-doctor rationale: fixed. The skill now says to relay the entry's
titleandevidence. 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::parsetoo 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 needsSUMMARY. - Changing
detect_local_windows_host_driverback 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.
- Each scope is now one named one-line wrapper (
NoAmdDisplaystill depends on the CPU/RAM query succeeding. Ifpnputilanswers with no AMD adapter but that query fails, the result is stillUnreachable.check_wsl_6then abstains, which is the safe direction.- An
--applyexclusion that matches nothing.provider_planner_args_require_approvalskips the read-only path when--applyis present, but I found no--applyflag oninstall 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::gathercall, and the inventory running twice underexamine --json. - The PR bundles the WSL-probe change and the
is_advice_onlyrule with the feature. - The platform decision and
is_advice_onlyare 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
setxmust not appear, but the base's fix-6 recipe still printssetx PATH …. - This PR doesn't touch fix-6. #710 (
0acf698d) fixed it onmain, andmain's Windows lane passesdiagnose-38since then (runs 39244601953 and 39389535923). - The rebase above clears it.
- The step asserts that
- Passing on the Windows lane: the new scenarios (
examine-29,diagnose-41,driver-install-04,driver-install-05) anddiagnose-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_versionis unchanged;- the JSON additions are
adrenalin_dateandsummary.driver.windows.
- adds the print-only
- #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 inhost_gpu.rs; the Windows CI job logs for the unit tests and the e2e suite;main's Windows lane history fordiagnose-38. - Ran, on a trial merge of the PR head into current
main:- the
rocm-corefilters fromdocs/testing.md, pluslocal_host_driver_probeandcalendar: 39 passed; cargo test -p e2e-cucumber --test driver_host_guard: 2 passed;- the
rocmbinary tests failed to compile (the blocker above). After the one-line-per-call fix, the 10 focused binary tests passed.
- the
- Not run: anything on Windows (the Windows results above are from CI), the full workspace suite, and mutation runs.
1302127 to
04bfbdb
Compare
|
Addressed in 04bfbdb, rebased on current main.
On the Windows lane for the previous head, |
tomastola
left a comment
tomastola
left a comment
There was a problem hiding this comment.
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:
- Chat can no longer run
install driveron Windows at all.chat_install_driver_actionnow 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 forinstall driver— onlyinstall sdk --dry-run— so it errors with "unsupported in-process read-only rocm command" instead of reporting. (chat.rs:2435,:3440,:3649) - The Microsoft Basic Display Adapter case still reads as "detected." The new
driver_version/date/agefields correctly treat it as no AMD driver, but the olderdriver_status/driver_detailfields (still read byrocm examineandinstall driver's reconciliation output) don't apply the same exclusion, so they still sayamd_display_driver_detected. (host_gpu.rs:1702,:1722,:2538;driver_install.rs:210) - 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.
04bfbdb to
80f4a25
Compare
|
Addressed in 80f4a25.
Not verified: the changes are checked on Linux; the Windows lane will exercise them. |
Read the AMD display driver version and date through the preferred adapter in the shared Windows inventory. Examine, diagnose and driver installation use the same selection; independent probe failures can still differ. Startup and engine selection avoid the supplementary driver-fact query. Show date-based advice for drivers older than the first HIP SDK for Windows release on 2023-07-27. No minimum compatible version is established. The new fix-22 entry is print-only and below the match threshold, and advice alone preserves upstream escalation. Windows driver installation remains non-mutating with or without approval. Cover inventory wiring, optional dates, fallback drivers, adapter choice, recipe drift and advice-only reports with unit tests. Add Windows scenarios examine-29, diagnose-41, driver-install-04 and driver-install-05, including version/date assertions and installer-state checks. The two install steps refuse any host that is not native Windows before they run the CLI, because explicit cucumber filters bypass platform tags; a unit test pins that order without running an installer. Update the catalog, skill and behavior docs. Verified on Linux: fmt, workspace clippy and e2e-target clippy with warnings denied, serial workspace tests (the only failure is an xtask lychee test that cannot execute lychee here), crate edges, catalog, local smoke, and focused core and CLI tests. The Windows scenarios were not run: Windows GPU behavior and the old-driver hardware branch remain unverified locally. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
80f4a25 to
deee2cb
Compare
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.
Problem
On Windows,
rocm examineshowed the AMD display driver as a name only, with no version or date. A laptop with a 2021 driver raised no warning inrocm examineorrocm diagnose, androcm install driveronly said to runrocm 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_14needs a HIP SDK and a failinghipInfoor a symptom, and it never compares dates.Change
rocm examine,rocm diagnoseandrocm install driverread the driver version and date from one Windows device inventory and one adapter choice.rocm examineprintsdriver_version,driver_dateanddriver_age. With--json, it addsadrenalin_dateand an optionalsummary.driver.windows, both additive. A Microsoft Basic Display Adapter is treated as "no AMD driver".fix-22-display-driver-old. It needs no HIP SDK, scores below the match threshold, and never setshas_match. Slot 20 belongs toinfo-20-gpu-no-rocm-support, so this entry uses 22. The catalog now has 29 entries.rocm install driverprints the version, date, anoutdated: yes/no/unknownverdict and the fix id. It installs nothing, with or without--yes. The chat and freeform planners now match this on Windows: read-only, no--yes, and no "disruptive" wording. Linux and WSL are unchanged.windows_driver.rs. Startup paths skip the extra video controller query.What "old" means
AMD documents no minimum driver version for the HIP SDK for Windows or for the Lemonade Vulkan backend. The check compares the driver date only with the release date of the first HIP SDK for Windows. The text says the minimum is not established and never says the driver is broken. Advice is AMD's support page and a restart. rocm-cli installs no drivers on Windows.
Tests
fix-22plan drift guards, the planner text next to what the command does on each platform, and the Windows host check, which is a pure function called with several OS names.examine-29,diagnose-41,driver-install-04anddriver-install-05. The two install steps refuse any host that is not native Windows before they run the CLI.reference.mdand the catalog are updated.contract_versionis unchanged because this is an addition.cargo fmt, workspaceclippy -D warnings, e2eclippy, focused tests, andcargo xtask catalog --check.Not verified
cfg(windows)code is compiled by the Windows CI job only.xtasktest (lychee) cannot run in the local sandbox.Merge order
The verify-command fix (#677) is still open and touches the same files (the fix recipes, the catalog,
reference.mdand the fix counts). Whichever merges second needs a rebase and a recount.