[volen-silo]

Problem

On a single-GPU native Windows machine, rocm serve <model> --gpu 7 was accepted: it printed a serve plan with gpu: 7 and launched a managed service.

Root cause

validate_pinned_gpu_index (apps/rocm/src/gpu_policy.rs) checks an explicit index against the visible-GPU set from usable_amd_gpu_indices (crates/rocm-core/src/host_gpu.rs), which has only a Linux probe and returns None on Windows, and against the amd-smi list count. On the test machine amd-smi is present but fails to load, so both sources were empty and any index passed silently. The visibility-mask refusal is the same validation, which is why it never fired on Windows either.

Change

• windows_amd_gpu_listing() in crates/rocm-core/src/host_gpu.rs lists the AMD display adapters from the Windows inventory rocm examine already uses (pnputil first, then the PowerShell queries), with HIP_VISIBLE_DEVICES applied. It reports one of three outcomes: unavailable, listed, or listed but the mask could not be resolved.
• serve uses it to bound an explicit --gpu <index> only. pinned_index_bound takes the stricter of the Windows list and the amd-smi count when both are known, so the adapter list can refuse an index but never loosens a check amd-smi already makes.
• --gpu auto and the engines are unchanged. The adapter count can include an adapter HIP does not support, so it may refuse an index but never picks one. Zero AMD adapters, or no source answering, counts as unknown, not "no GPU", so auto is never blocked.
• When the listing worked but HIP_VISIBLE_DEVICES could not be resolved (an out-of-range or UUID token) and amd-smi is down, the adapter count is still an upper bound: an index at or past it is refused (the message says the mask was unresolved and the count is the only bound), and an index below it is accepted with a warning that it was checked only against the GPU count, not against that mask. When no source can list the GPUs at all, an explicit index is let through with a warning saying so. The warning text comes from one function used by serve() and asserted as printed.
• An empty or whitespace-only HIP_VISIBLE_DEVICES on Windows is treated as a mask that was not understood, not as "all GPUs hidden": the HIP documentation lists the variable for Windows but says nothing about an empty value, and copying Linux/ROCr semantics would hard-refuse every index on a wrong guess. The adapter count still bounds the index and a warning is printed.
• When amd-smi already supplied a GPU count and no mask is set, the Windows listing is not read at all: the range check is the same one amd-smi alone makes, so nothing is weakened, and the listing could only add a false refusal. Every listing source (pnputil and the two PowerShell queries) runs under a 5 s timeout and is killed on expiry (WINDOWS_INVENTORY_QUERY_TIMEOUT in host_gpu/windows_inventory.rs), so the worst case on a degraded host where all three fail is about 15 s; a normal host only runs pnputil.
• The adapter count counts a row only with a PCI id starting PCI\VEN_1002 or a marketing name rocm examine recognises. The first version matched any name token "amd", which counted "AMD Bluetooth Capture Audio Device" as a GPU on the repo's own noisy fixture (count 2 instead of 1); a test now pins 1.
• On Windows only HIP_VISIBLE_DEVICES is named as a visibility mask; ROCR_VISIBLE_DEVICES has no effect there and is no longer named in a refusal. Linux wording is unchanged.
• Docs: README (--gpu paragraph, including that the Windows list can include an adapter the runtime does not support, so the check guards against a mistyped index, not a guarantee the device is usable), the --gpu help text, docs/testing.md (serve-16 on all lanes, where the unit tests live), and a new "Windows --gpu index check" section in docs/manual-testing.md. No other "Linux only" or "silently accepted" wording for --gpu exists in the README, help or docs.

Tests

• Unit, in rocm-core and gpu_policy.rs: one adapter gives [0]; zero adapters is unknown; the HIP mask is applied (empty means none visible, out-of-range means unresolvable); the pnputil-only path starts no PowerShell query; an Intel adapter is not counted; the adapter list never loosens the amd-smi count (the_windows_adapter_listing_never_loosens_the_amd_smi_count); the Windows fallback composes on any host (pinned_index_bound); the printed warning is exact and absent for a checked index; an unresolvable mask is told apart from a failed listing; the refusal names only the variable that acts on the platform.
• Scenario serve-16 (@id:serve-absent-gpu-index-rejected): no longer @requires-os:linux, so it runs on the Windows GPU lane too, and ends with the existing And no local server was recorded step (it lists <data>/services through the CLI; a missing folder counts as no records, any other read error fails the step). Shown red: with that step made to write a record into <data>/services, serve-16 fails with "a refused serve must not record a service, found: [...planted.json]". A refused serve never creates the folder, so a stricter "must be readable" rule would have failed a correct refusal. Passed locally against a debug build for this head; the Windows lane result for this head will show on the PR.
• cargo nextest run -p rocm-core -p rocm (2176 passed), clippy for rocm-core, rocm and e2e-cucumber (including --test e2e), cargo fmt --check: pass. Real binary on this Windows host: --gpu 7 is refused with "not present on this host; usable GPU indices: [0]" and no service record; with ROCR_VISIBLE_DEVICES=0 the message is the same; with HIP_VISIBLE_DEVICES=0 it names the visibility mask and only HIP_VISIBLE_DEVICES.

Other --gpu consumers (investigated, not changed here)

engines/lemonade/src/device_policy.rs and engines/vllm/src/process.rs each resolve --gpu N through rocm_core::usable_amd_gpu_indices(), which returns None on non-Linux, so an engine invoked directly on Windows passes the index through unchecked: the same class of bug. rocm serve validates first, so it is reachable only by invoking an engine directly, and rocm services restart replays the recorded index without re-checking. Fixing it means putting the Windows listing into usable_amd_gpu_indices, which this PR deliberately keeps it out of, because the listing can over-count and --gpu auto acts on that function. That is a separate decision.

Not verified

• The unchecked-index warning was never seen on a real run: reaching it launches a server, and no lane produces a host where no source can list GPUs. It is covered by the unit test of its printed text.
• Linux clippy was not run locally (Windows host). The Linux stub carries the same lint allow as the existing stub.
• On multi-GPU Windows machines, the adapter order could disagree with HIP's device order. Any mismatch errs towards accepting an index, never refusing a valid one.
• The two E2E tests (Strix Halo, Ubuntu, …) lanes ran out at 45 minutes with no logs on this PR, and the same two lanes fail the same way on several unrelated open PRs. This change is Windows-only in behaviour.

Behaviour changes

• On Windows, an explicit --gpu <index> past the adapter count is now refused (it used to launch), including when HIP_VISIBLE_DEVICES cannot be resolved. An in-range index under an unresolvable mask is still accepted, now always with a warning, even when amd-smi supplies a count (that case used to be silent).
• Beyond Windows: when no source can list the host's GPUs (the Linux KFD/DRM probe fails and amd-smi fails or is absent), an explicit --gpu <index> is still accepted, but rocm serve now prints a warning: line on stderr saying the index was not checked. Where amd-smi still gives a count but a HIP_VISIBLE_DEVICES or ROCR_VISIBLE_DEVICES mask is set and cannot be applied (the visible set is unknown), an index at or past the count is refused and a lower one is accepted with a warning that the mask was not checked. With an amd-smi count and no mask, nothing changes and nothing is printed.

Adapter order and what the count means

On Windows the adapter list (pnputil, then the CIM queries) and amd-smi are enumerated independently, and their device order is not guaranteed to match HIP's. When both are known the stricter count wins, and the list only ever refuses an index; it never selects one (--gpu auto and the engines still use usable_amd_gpu_indices). On a multi-GPU host an order mismatch can therefore let an index through that HIP cannot use, but it never refuses a valid one. The list can also count an adapter the ROCm runtime does not support, so the check is a guard against a mistyped index, not a guarantee that the device is usable.

[juhovainio]

I went through this against ROCMAI-824 and the change does what the ticket asks on Windows: an out-of-range --gpu index is refused, the GPUs that do exist are named, and nothing is started. CI is green on the lanes that have completed, including the Windows and WSL2 Strix Halo E2E runs. I found nothing that breaks the main path, but a few things should be addressed before merge:

1. The Windows adapter set can loosen a check that already worked. On Windows it is passed as the authoritative visible set, so it outranks the amd-smi count. If amd-smi works and pnputil counts an adapter HIP doesn't support (iGPU + dGPU), an index amd-smi would have refused is now accepted. The code comment says the new path "may refuse, must not loosen"; bounding by the detected count as well would match that.
2. The "no server started" e2e step can pass vacuously. It swallows a read_dir error with unwrap_or_default(), so it passes if the services directory is not where records are written on a given lane. Please assert the layout, or show the step fails when a record is written.
3. The "could not list the AMD GPUs" warning is wrong in one case. With amd-smi also failing, an out-of-range or UUID HIP_VISIBLE_DEVICES token makes the Windows listing return nothing even though the listing itself succeeded.
4. Docs are not in sync. serve-16 now runs on all platforms and Windows has new behavior, but docs/testing.md and docs/manual-testing.md are untouched. Please also check the --gpu help text for "Linux only" or silently-accepted wording.
5. The README promises a warning on stderr that no test ties to the unchecked branch. The unit test only asserts the Option is Some; the eprintln! in serve() is unasserted.

Not blockers, but worth confirming: accepting an unchecked index with a warning (rather than refusing) when no source can list GPUs, and that Windows adapter order may differ from HIP ordinal order. The PR body acknowledges both.

Three of the above are anchored inline.

[juhovainio]

On Windows this makes the adapter set authoritative over detect_gpu_count() in validate_pinned_gpu_index, which only falls back to detected when the set is None. If amd-smi works and pnputil counts an adapter HIP doesn't support, an index amd-smi would have refused is now accepted. Consider also bounding by detected.

[volen-silo]

Agreed, fixed in ccbc5cb. pinned_index_bound now takes the stricter of the Windows adapter list and the amd-smi count when both are known, so the list can refuse an index but never loosens a check amd-smi already makes. the_windows_adapter_listing_never_loosens_the_amd_smi_count covers adapters [0,1] with an amd-smi count of 1: --gpu 1 stays refused.

[juhovainio]

unwrap_or_default() makes this step pass whenever data/services can't be read (e.g. a different path on some lane). Please assert the directory layout, or otherwise show the step goes red when a service record is written.

[volen-silo]

Fixed, and the step was wrong in a way that mattered more than the vacuous pass: a refused serve never creates <data>/services at all, so a strict "must be readable" rule would have failed a correct refusal. The step now builds the path from AppPaths::services_dir(), asserts the CLI was handed ROCM_CLI_DATA_DIR=<root>/data, treats only a missing folder as "no records", and fails on any other read or entry error. I checked the path by placing a record in that folder and seeing rocm storage report count it; I did not run the step red against a written record (it needs the GPU lane), and the description says so.

[juhovainio]

This scenario now runs on Windows as well as Linux; docs/testing.md and docs/manual-testing.md should reflect that, along with the new Windows and warning behavior.

[volen-silo]

Updated: docs/testing.md (serve-16 on all lanes, the Windows source, where the unit tests live), a new "Windows --gpu index check" section in docs/manual-testing.md, the --gpu help text, and the README (which now also says the Windows list can include an adapter the runtime does not support, so the check guards against a mistyped index and is not a guarantee). A grep found no "Linux only" or "silently accepted" wording for --gpu left in the README, help text or docs.

[nowycondro]

Review — no blocking findings

Full review of the whole change (base bcd879d0…head 922bf0b9, 7 files, +331/-36).
Implements ROCMAI-824 ("rocm serve --gpu <n> accepts a GPU index that does not exist and launches anyway"): adds a Windows AMD-display-adapter-count probe so an explicit --gpu <index> is refused when it exceeds what Windows reports, matching the ticket's "refuse the index, name the GPUs that do, start nothing" ask. The ticket is still "In Progress" in Jira and the PR does not claim to close it outright, which is consistent with its own scope.

Blocking

None

Non-blocking

• README omits the adapter-overcount caveat the code itself documents — README.md (the new --gpu paragraph)
  The added prose states flatly that "On Windows the index is checked against the AMD display adapters Windows reports," implying a reliable refusal. But windows_amd_gpu_indices's own doc comment (crates/rocm-core/src/host_gpu.rs) says "the count may be too high: an adapter HIP does not support is still counted. So it is used to refuse an index, never to choose one" — i.e. the Windows check can under-refuse (let an actually-absent-to-HIP index through) in a way the Linux KFD/DRM path cannot. The PR body's own "Not verified" section separately acknowledges a related multi-GPU adapter-order risk, but neither caveat made it into the user-facing README.
  Confidence 75 · logic (doc accuracy) · Fix: add a short qualifying clause to the README noting the Windows check can overcount and is therefore not as strict a guarantee as the Linux path.

• The new Windows fallback composition in serve_cmd.rs has no unit-level seam, only e2e coverage — apps/rocm/src/serve_cmd.rs (let index_bound = visible_gpu_indices.clone().or_else(rocm_core::windows_amd_gpu_indices);)
  rocm_core::windows_amd_gpu_indices() is a hard None off Windows, so this one-line composition is only observable on an actual Windows host; no unit test on Linux CI exercises it, and the gpu_policy.rs test framed as covering the Windows case (validate_pinned_gpu_index_refuses_an_index_past_the_windows_adapter_set) only re-exercises the pre-existing, unchanged "visible is Some and out of range" branch with different fixture values, not this composition line. That said, this is mitigated in practice: the PR's own CI run (job "E2E tests (Strix Halo, Windows, current)", completed and green) actually executed scenario serve-16 on real Windows GPU hardware, including the new "no local server was started for it" step, and it passed — so the real end-to-end behavior is verified, just not via a fast unit test.
  Confidence 75 · architectural (testability) · Fix: extract the composition into a small pure/injectable helper so a unit test can simulate the Windows-adapter fallback on any CI runner, per this repo's own "platform-gated code gets a test that runs on its gate" convention.

• A Windows-only refusal message can name ROCR_VISIBLE_DEVICES when that variable has no effect there — apps/rocm/src/serve_cmd.rs (visibility_mask_active = rocr_visible_devices_set || hip_visible_devices_set) composed with apps/rocm/src/gpu_policy.rs's validate_pinned_gpu_index wording, against crates/rocm-core/src/host_gpu.rs's windows_amd_gpu_indices_from (which always passes rocr: None, since "native Windows HIP has no ROCr layer").
  visibility_mask_active is computed from both env vars unconditionally, regardless of platform. On native Windows, if a user sets only ROCR_VISIBLE_DEVICES (which the code itself documents as inapplicable there) and passes an index past the real adapter count, the refusal reads "not available under the active visibility mask (HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES)" instead of "not present on this host" — misattributing the cause to a variable that did nothing on that platform. This is new surface area: before this PR, visible was always None on Windows, so this branch could never be reached there at all. It does not change the accept/refuse outcome, only the stated reason.
  Confidence 80 · logic · Fix: gate visibility_mask_active's rocr_visible_devices_set term on !rocm_core::runtime_is_windows(), or have validate_pinned_gpu_index take a mask description instead of a bare bool.

Decisions for the author

• The Windows adapter-count source can overcount relative to HIP's own view — tradeoff
  Counting raw AMD display adapters (pnputil/CIM) rather than a HIP-aware source means an adapter HIP cannot use can still be counted, so the check is asymmetric by design ("may refuse, never picks"). The PR body already discloses the related multi-GPU adapter-order risk and accepts it for this fix's scope (a single-GPU machine silently accepting an absent index). Worth reconfirming this is still the intended tradeoff for the initial release of this check.

Positive signals

• is_amd_windows_display was cleanly extracted out of push_windows_pnputil_display into a shared helper instead of duplicating AMD-vendor-detection logic a third time, and a test (the_gpu_index_check_does_not_count_another_vendors_adapter) specifically proves the extracted filter still excludes a non-AMD adapter reaching the inventory through the unfiltered CIM fallback.
• InventoryScope::DISPLAYS_ONLY genuinely avoids the unwanted system/driver-fact PowerShell queries (traced through assemble_windows_inventory, confirmed by the sources.calls == vec!["pnputil"] assertion), rather than just being a cosmetic new constant.
• --gpu auto is structurally prevented from ever reaching this new validation path (the serve_cmd.rs match arm only binds GpuSelection::Index), so the stated "may refuse, never picks" invariant holds for that path at least.

Deployment notes

User-facing behavior change on Windows: an explicit --gpu <index> that exceeds the detected AMD-adapter count is now refused (previously silently accepted and a managed service launched for it); a new stderr warning appears when no source can enumerate GPUs at all. No migration or config changes.

CI for this head commit: every completed check is green (clippy, commit-signatures, Linux/Windows/WSL2 builds and unit/e2e suites). At review time, four self-hosted hardware lanes (MI300X, Strix Halo Ubuntu release/nightly, W7900) were still queued — not failing, just not yet run. The Strix Halo Windows GPU lane did complete and passed, and its log confirms scenario serve-16 ran and passed there, including the new "no local server was started for it" step — this directly exercises the fix on real Windows GPU hardware. That contradicts the PR body's own "Not verified"/"Status when marked ready" note that serve-16 "has not run on the Windows GPU lane yet" and that the GPU lanes "were still queued" — the Windows one has in fact completed and passed; worth refreshing that section before merge.

What this covered

Read every changed file in full against bcd879d007ba2db287688147637af2386e979616…922bf0b969fa5b8ed92695d3073ea14b5e5d2560 (README.md, apps/rocm/src/gpu_policy.rs, apps/rocm/src/serve_cmd.rs, crates/rocm-core/src/host_gpu.rs, crates/rocm-core/src/lib.rs, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e/serving_steps.rs), plus the surrounding code each touched symbol lives in (usable_amd_gpu_indices_from/mask_tokens_within/GpuVisibilityMask, assemble_windows_inventory/InventoryScope, runtime_is_windows, detect_gpu_count, every other caller of validate_pinned_gpu_index/PinnedGpuIndex/windows_amd_gpu_indices repo-wide), via three independent fan-out passes grouped by coupling (gpu_policy.rs+serve_cmd.rs; host_gpu.rs+lib.rs; README+e2e feature+e2e steps) plus my own direct reads. Checked this repo's AGENTS.md for applicable conventions. Read CI results for this exact head commit via the GitHub Actions API/job logs, including pulling the actual Windows-GPU-lane job log to confirm serve-16 ran and passed. Retrieved ROCMAI-824 from Jira for the Step 9 comparison.

Did not run: a dedicated history/prior-changes pass beyond git log/git blame on the touched regions (single-commit PR, no earlier reviewed revision to diff against). Could not execute cargo test/cargo clippy locally (no Rust toolchain available in this environment); correctness was verified by hand-tracing the code and by reading this PR's own CI results rather than by independently re-running the suite.

A review from a different author already exists on this PR at this same commit; this review was produced independently and was not reconciled against it.

[volen-silo]

Pushed ccbc5cb (squashed, rebased onto main). Points in the two reviews:

1. The adapter list can no longer loosen an amd-smi check: bounded by the stricter of the two.
2. The no-server step is hardened (path from AppPaths, data folder asserted, only a missing folder counts as empty); I did not run it red on a written record.
3. The warning now tells "no source could list the GPUs" apart from "the listing worked but HIP_VISIBLE_DEVICES could not be resolved".
4. Docs: docs/testing.md, docs/manual-testing.md, --gpu help and README updated.
5. The printed warning text comes from one function and is asserted as printed. No scenario reaches the unchecked branch (it needs a host where no source can list GPUs), so that stays a unit test.
6. Second review: README caveat added; the Windows fallback is now a pure helper with unit tests that run on any host; a refusal on Windows names only HIP_VISIBLE_DEVICES, since ROCR_VISIBLE_DEVICES does not act there.
7. Stale status: serve-16 ran and passed on the Windows Strix Halo lane for the earlier head; the description says so.

On the open decisions: accepting an unchecked index with a warning (rather than refusing) when nothing can list the GPUs, and the possible adapter-order mismatch on multi-GPU Windows machines, are unchanged and stated in the description. The two Strix Halo, Ubuntu lanes timed out at 45 minutes with no logs here and fail identically on several unrelated open PRs.

All five points are addressed in ccbc5cb and the updated description (one of them partly: the no-server step was hardened but not run red against a written record, which the description states); see the replies and the summary comment. A re-review is welcome.

[juhovainio]

The default Windows repro is fixed: serve-16 is green on the Windows lane and asserts that nothing was started. Nothing here blocks on correctness, but one gap remains against the ticket and two doc claims disagree with the code, so I'm requesting changes. The PR also needs a rebase, since it conflicts with main.

Needs fixing

• An out-of-range --gpu index still passes unchecked when the HIP mask can't be resolved and amd-smi is down. MaskUnresolved carries the adapter count, but nothing uses it (inline comment).
• docs/manual-testing.md describes a refusal for HIP_VISIBLE_DEVICES=3 that the code doesn't produce (inline comment).
• The README line "the index is used unchecked" is broader than the behavior (inline comment).

Worth considering

• Scope: the ROCR wording rule, the unchecked warning and the three-state enum could be split out of this one-bug fix. The ticket excludes ROCMAI-719.
• A HIP mask cut by the amd-smi count can leave an empty set, and the message then says "no usable AMD GPU present" instead of naming the mask.
• I didn't check whether other --gpu consumers (rocmd, the engines' own validation) bypass this check.

CI

• The Strix Halo Windows E2E lane failed 4 of 174 scenarios. They are unrelated to --gpu, and the log shows HTTP 404s on lemonade llama.cpp zip downloads. Please rerun after the rebase.
• The W7900, MI300X and Strix Halo Ubuntu E2E jobs were still pending when I checked.

[juhovainio]

With one adapter, HIP_VISIBLE_DEVICES=3 is an ordinal past the adapter count, which windows_gpu_listing_from reports as MaskUnresolved (see the PR's own test). With amd-smi unusable the index is then accepted with a warning rather than refused "under that mask". Either reword the example (use a mask that names an existing ordinal) or change the behavior.

[volen-silo]

Right, that example described a refusal the code does not produce. Rewritten after the fix above: HIP_VISIBLE_DEVICES=0 --gpu 7 is the "refused under the mask" example, and HIP_VISIBLE_DEVICES=3 is now described as the unresolved case (out of range by count, refused when the index is past the count). I ran each refusal case live with a scratch config and data folder; the accepted-with-warning case would start a server, so it is covered by unit tests only and the doc says it starts one.

[juhovainio]

MaskUnresolved { adapters } keeps the adapter count but nothing reads it. An index >= adapters cannot exist whatever the mask says, so --gpu 7 could still be refused here instead of passing unchecked.

[volen-silo]

Agreed, fixed in 3eef330: the adapter count is now an upper bound. With the mask unresolved and amd-smi down, --gpu 7 on one adapter is refused ("1 GPU(s) detected (valid indices 0..0); HIP_VISIBLE_DEVICES could not be resolved ..., so the count is the only bound"), and --gpu 0 is accepted with a warning that it was checked only against the count, not the mask. Unit tests cover both, and I ran the refusal cases on this single-GPU Strix Halo.

[juhovainio]

"the index is used unchecked" is too broad: when the mask is unresolved but amd-smi gives a count, the index is still checked against that count and no warning is printed. Say so.

[volen-silo]

Fixed: the README, the --gpu help text and docs/testing.md now say exactly when the index is unchecked (nothing could list GPUs: it is let through with a warning) and when it is only checked against the count with a warning that the mask was not applied (mask unresolved). A grep found no other copy of the claim.

[volen-silo]

Pushed 3eef330 (squashed, rebased onto main). Points in the review:

1. Out-of-range index under an unresolvable mask: now refused, using the adapter count as the bound; an in-range index is accepted with a warning that names what was and was not checked. The earlier "unchecked" wording is gone.
2. docs/manual-testing.md: example corrected and each refusal case run live; the accepted case is unit-tested only.
3. README: reworded, also in the help text and docs/testing.md.
4. A HIP mask cut to nothing by the amd-smi count: checked, the message already names the mask; regression test added.
5. Other --gpu consumers: the lemonade and vllm engines resolve the index through usable_amd_gpu_indices(), which is None off Linux, so an engine invoked directly on Windows is unchecked; rocm serve validates first. Not changed here, since fixing it means changing a function --gpu auto also uses; it is written up in the description for a separate decision.
6. Scope: the ROCR wording rule and the unchecked-warning plumbing are separable; the three-state listing is part of the fix, because the adapter bound now depends on it. I would rather keep this as one PR; say if you want it split.
7. CI: I have not diagnosed the four failed Strix Halo, Windows scenarios on this PR myself. The review reports llama.cpp download 404s from github.com. For the same lane on ROCMAI-830: pin the fix-13 gate for a Lemonade Strix Halo host with a managed runtime #835, whose artifact I read, the failures were serve-01 and serve-02 (empty serve output, consistent with a failed download but the 404 itself not visible) and runtime-02 and runtime-09 (runner configuration: a prewarmed runtime and a mirror index override). None involves --gpu; the lane needs a rerun after the rebase to settle it.

All the requested changes are addressed in 3eef330 and the updated description (the scope split is answered rather than done, and the other-consumers question is written up for a separate decision); see the replies and the summary comment. A re-review is welcome.

[juhovainio]

I went through the diff against ROCMAI-824 and found nothing that breaks the behavior: an out-of-range --gpu index is refused on Windows, the message names the GPUs that exist, and no service is started (serve-16 now runs on Windows and checks no service record is written). Docs and the scenario are updated.

I'm requesting changes only for small cleanups in the new tests, left inline: a comment that names a function that no longer exists, and a few comments that describe the review rather than the code.

Smaller points, no action required:

• count.saturating_sub(1) would print "valid indices 0..0" if amd-smi ever reported 0 GPUs; worth special-casing the empty list.
• The Windows-only body of windows_amd_gpu_listing is only exercised by the Windows lane running serve-16, so please keep that lane's result visible in the PR text.
• Linux hosts where the GPU count can't be probed now print a stderr warning; that's documented, just noting it is a behavior change.

I did not build the branch or run the break-the-code check on the new tests; this was a read of the diff only.

[juhovainio]

Stale name: rocm_core::windows_amd_gpu_indices no longer exists; the function is now windows_amd_gpu_listing.

[volen-silo]

Fixed in 7844424; a grep for the old name across crates/, apps/, docs/, README.md and tests/ finds nothing else.

[juhovainio]

This comment narrates the review ("ROCMAI-824 review:"). Keep the ticket id and describe the scenario instead; the same wording appears at the other "ROCMAI-824 review:" comments in this test module (around lines 1548 and 1573).

[volen-silo]

Fixed: the three "ROCMAI-824 review:" comments (the unresolvable-mask test, the mask-cut-to-nothing test and the amd-smi-count-not-loosened test) now describe what each test does and why, and keep only the ticket id.

[volen-silo]

Pushed 7844424 (rebased onto main). The test-comment cleanups are done (stale function name, review narration). On the smaller points: an amd-smi count of 0 now refuses with "no GPUs were detected" (and mentions an unresolved HIP_VISIBLE_DEVICES when that applies) instead of a "0..0" range, with a test. The unprobeable-Linux-host warning is already documented in the README, docs/manual-testing.md and docs/testing.md. For the Windows-only listing body, serve-16 passed on the Strix Halo, Windows lane for the earlier heads of this PR and ran locally against a debug build for this one; the lane result for this head will show on the PR when it completes.

The requested cleanups are done in 7844424; see the replies and the summary comment. A re-review is welcome.

[nowycondro]

🤖 Automated review (Claude Code) · 7844424

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 (merge-base 6b175eee…head 7844424b, same 10 files as throughout this PR's life: README.md, apps/rocm/src/gpu_policy.rs, apps/rocm/src/main.rs, apps/rocm/src/serve_cmd.rs, crates/rocm-core/src/host_gpu.rs, crates/rocm-core/src/lib.rs, docs/manual-testing.md, docs/testing.md, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e/serving_steps.rs).
Implements ROCMAI-824's "refuse the index, name the GPUs that do, start nothing" ask for the Windows path specifically; the ticket is still "In Progress" and explicitly scopes out ROCMAI-719 (visibility-mask-hidden indices), which this PR also correctly does not touch. The PR does not claim to close the ticket outright, consistent with its own stated scope.

Blocking

• CI is red at this exact head: a duplicate-symbol compile error breaks the e2e-cucumber test crate — observed directly from this head commit's own job logs, not inferred
  cargo clippy --locked -p e2e-cucumber --test e2e -- -D warnings fails with error[E0428]: the name 'assert_no_service_recorded' is defined multiple times, reported at tests/e2e-cucumber/tests/e2e/serving_steps.rs:1878:1 with a previous definition noted at line 1606. This cascades: clippy, windows-build-and-test, Windows build and test (debug) and Windows release build and lifecycle all report failure for commit 7844424b, and several other jobs (build-and-test, Build Linux PR binaries, Test (affected crates)) report skipped rather than running at all.
  Puzzle worth flagging to the author rather than silently working around: the committed blob for serving_steps.rs at this head is byte-identical (same git blob SHA, fa5503ad…) to the previous head (3eef3305, where this exact job was green) and contains assert_no_service_recorded only once, at line 1716 — neither 1606 nor 1878 exist in that file as committed. That strongly suggests the failure is reading content from GitHub's synthetic PR-merge ref against the current tip of main (which has moved since this branch's last rebase onto 6b175eee, itself already one rebase past this PR's prior CI-green run) rather than from a byte-for-byte difference in this branch's own commit — i.e. this branch most likely now collides with something merged into main in the interim. Either way, this is the real, current state of required CI for the commit under review and must be resolved (most likely via another rebase) before merge, independent of every other finding below.
  Confidence 100 · logic · Fix: rebase onto current main and resolve whatever now defines assert_no_service_recorded a second time, then re-run CI (the self-hosted hardware E2E lane, including the Windows GPU lane that actually exercises serve-16, was still queued/pending with zero jobs started for this head at review time, so it has not yet had a chance to confirm or deny the same failure on hardware).

• A Linux fallback path never consults the visibility mask it has, and prints no warning, directly contradicting the new field's own doc comment — apps/rocm/src/gpu_policy.rs:182-186 (doc comment) vs. :356-390 (validate_pinned_gpu_index's no-visible-set branch)
  PinnedGpuIndex::unchecked_warning's doc comment states it is set "when the index was accepted without a full check: neither source could list the host's GPUs, or HIP_VISIBLE_DEVICES could not be resolved... serve() prints it as a warning so an unchecked index is never accepted silently." But trace the no-visible-set fallback: when visible is None (the Linux KFD/DRM probe failed) and detected (amd-smi) succeeds with the index in range, the function returns unchecked_warning: mask_unresolved.then(|| ...) — and mask_unresolved is derived solely from the Windows-only WindowsGpuListing::MaskUnresolved variant (confirmed via pinned_index_bound, apps/rocm/src/gpu_policy.rs:257-280: on non-Windows, windows() always yields Unavailable, so mask_unresolved is always false on Linux). The mask: Option<&str> parameter — which does tell the function whether the user actually set HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES — is never read in this branch at all; it is only consulted inside the visible.is_some() branch above it, for wording. So on a Linux host where the KFD/DRM probe fails but amd-smi still answers (combine_amd_gpu_counts returns None only when both fail, which a restricted-sysfs container or similar can trigger) and the user has an active HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES mask, an index that mask would exclude is silently accepted with no warning — the exact "unchecked index accepted silently" case the field's own comment says can't happen. This is new, untested surface: PinnedGpuIndex/unchecked_warning did not exist before this PR, and no test in the file exercises mask: Some(_) together with visible: None.
  Confidence 85 · logic · Fix: in this fallback, also set unchecked_warning when mask.is_some() (a mask is actually set), not only when mask_unresolved (a Windows-only concept) is true — the mask was just as unchecked in that case.

• amd_display_count()'s vendor filter can count a non-GPU device as an AMD adapter, demonstrably so against the file's own existing test fixture — crates/rocm-core/src/host_gpu.rs:1795-1801 (amd_display_count) and :3040-3048 (is_amd_windows_display), vs. the pre-existing fixture at :5064 (windows_examine_inventory_prefers_real_gpu_over_noisy_amd_pnp_entries) and parse_windows_examine_inventory (:2937, which pushes any GPU\t... row from the CIM path into self.displays with no vendor filter at parse time, unlike the pnputil path's push_windows_pnputil_display, which does filter before pushing)
  is_amd_windows_display treats any whitespace-delimited name token exactly equal to "amd" as a match. Feed the file's own existing fixture text — "AMD Bluetooth Capture Audio Device\t\t{2101C4C0-...}\\CAPTURE&CP_111215637" plus a real "AMD Radeon 780M Graphics" row — through parse_windows_examine_inventory (as that pre-existing test already does, to prove preferred_amd_display/amd_display_name correctly pick the real GPU) and then call the new amd_display_count() on the same resulting inventory: the Bluetooth device's lowercased name tokenizes to ["amd","bluetooth","capture","audio","device"], containing the exact token "amd", and its PNPDeviceID has no VEN_1002, so is_amd_windows_display returns true for it — amd_display_count() on this exact pre-existing fixture returns 2, not 1. This function's own doc comment says the CIM fallback "also matches names like 'Intel(R) UHD Graphics', so the rows are filtered again here rather than trusted to be AMD" — but the filter used doesn't catch this other known-noisy case the same file already anticipated (only for the driver-name-display use, not for the index-bound use this PR adds). windows_amd_gpu_listing (the sole production caller of amd_display_count) reaches the CIM pnp_entity fallback specifically when pnputil and the video_controller CIM query both fail — i.e. exactly the degraded-host scenario this PR exists to make safer — and an inflated count here means pinned_index_bound/validate_pinned_gpu_index accepts an index that corresponds to no real, HIP-usable GPU at all, which is the precise failure class ROCMAI-824 is about. The one new test for this function (the_gpu_index_check_does_not_count_another_vendors_adapter) only proves an Intel-named adapter is excluded; it does not exercise the already-anticipated noisy-AMD-name case.
  Confidence 85 · logic · Fix: have amd_display_count() reuse preferred_amd_display's stricter PCI-id/marketing-name criteria (or at least require a resolvable AMD PCI device id when the name match is this loose), and add a test asserting the count on the existing noisy fixture.

• --gpu --help text overclaims: implies an unresolved mask is always accepted with a warning, when the code still refuses it past the GPU count — apps/rocm/src/main.rs:542-545 vs. apps/rocm/src/gpu_policy.rs:374-384 (validate_pinned_gpu_index)
  The help text reads: "If the GPUs cannot be listed, or HIP_VISIBLE_DEVICES cannot be read as ordinals, the index is used with a warning that it was not fully checked." Read plainly this is "(listing failed) OR (mask unresolved) ⇒ accepted-with-warning," unconditionally. But when mask_unresolved is true and the index is at or past the GPU count, the code bail!s a refusal ("…so the count is the only bound"); only an index below the count is accepted with a warning. This is directly confirmed by this PR's own test an_unresolvable_mask_still_refuses_an_index_past_the_adapters. README.md's new --gpu paragraph, added in this same PR, gets this exactly right ("…the index is still refused when it is at or past the GPU count, and one below the count is accepted with a warning…"), so this is an internal inconsistency this PR introduces between two docs it edits together, with the fuller/correct version already written down in the same diff.
  Confidence 85 · logic (doc-accuracy) · Fix: the code (and the PR's own README wording) governs; tighten the --help text to something like "...or HIP_VISIBLE_DEVICES cannot be read as ordinals, the index is refused at or past the GPU count, and otherwise used with a warning that the mask wasn't checked."

Non-blocking

None — everything else surfaced in this fan-out (a discarded .ok() on a warning write, a possibly-dead cfg(windows) guard matching ~8 sibling instances elsewhere in the same file, a var vs var_os lossy-UTF-8 nit, a cosmetic missing bullet marker in a doc comment, a test helper reconstructing AppPaths by hand) was below the confidence bar for a real, actionable defect once checked against the code, or is an established repo-wide convention this PR merely repeats rather than introduces.

Decisions for the author

• Mixing the Windows adapter-listing ordinal space with the amd-smi count — tradeoff
  pinned_index_bound cuts the Windows HIP-masked adapter set to the amd-smi count by raw index (set.retain(|index| (*index as usize) < count)) when both are known. These are two independently-probed enumerations (pnputil/CIM adapter order vs. amd-smi's own device order) that are not guaranteed to agree on multi-GPU hosts — the PR body itself already discloses this ("the adapter order could disagree with HIP's device order... errs towards accepting an index, never refusing a valid one"). Worth reconfirming this is the intended tradeoff for the initial release of this check, same as the PR's own text already frames it.

Positive signals

• WindowsGpuListing's three-way split (Unavailable / Indices / MaskUnresolved { adapters }) cleanly separates "nothing knowable" from "zero actually present" from "known count, mask unresolved," with no illegal-state conflation found in windows_gpu_listing_from or its callers.
• The new count == 0 special case in validate_pinned_gpu_index (this PR's latest commit) correctly fixes the "valid indices 0..0" misleading-range message a zero-GPU host would otherwise have printed, and ships with its own test (a_zero_gpu_count_refuses_an_index_without_naming_a_range) — a clean, well-targeted response to the earlier reviewer's "worth special-casing the empty list" note.
• The serve-16 e2e step's disk-state handling (assert_no_service_recorded) correctly distinguishes "directory not found" (pass) from any other read error (hard failure, not swallowed) from "directory exists and has a record" (fail) — directly answering an earlier review round's concern about a vacuous unwrap_or_default() pass; independently re-verified against the current code, not taken on faith.
• --gpu auto is structurally prevented from ever reaching the new Windows-listing validation path (the serve_cmd.rs match arm only binds GpuSelection::Index), so the stated "the adapter count may refuse an index but never picks one" invariant holds by construction for that path.

Deployment notes

User-facing behavior change on Windows: an explicit --gpu <index> past the detected AMD-adapter count is now refused (previously silently accepted and a managed service launched); a new stderr warning appears when no source can enumerate GPUs at all, or when HIP_VISIBLE_DEVICES can't be resolved to ordinals but the index is still in range. No migration or config changes.

CI status for head 7844424b at review time: clippy and three Windows build/test jobs are failing with a duplicate-symbol compile error (see Blocking, above) — this must be green before merge. The self-hosted hardware E2E workflow (which includes the only lane that runs serve-16 on real Windows GPU hardware) was still queued with zero jobs started for this head; it had completed and passed for the previous head (3eef3305), but that result does not carry forward to 7844424b and, given the compile error above, would likely fail to build at all if it ran right now.

What this covered

Read every file in the diff completely against merge-base 6b175eeeb1e881d4b293b9670894fb1811f61de1…head 7844424bbcaa72e0022fbf77b1d743985aaa6572 (confirmed this PR's own scope is unchanged from its prior 3eef3305 head — same 10 files, apps/rocm/src/gpu_policy.rs gained one further self-contained fix — via git diff --numstat against the correct merge-base; a raw head-to-head diff otherwise pulls in ~15 unrelated files from an intervening rebase onto a newer main, which were excluded from review as out of this PR's scope), via four independent fan-out workers grouped by coupling (gpu_policy.rs+serve_cmd.rs; host_gpu.rs+lib.rs; main.rs+README.md+docs/testing.md+docs/manual-testing.md; the e2e feature+step-definition pair), each reading its files completely plus the surrounding code and existing tests, followed by my own direct re-verification of every finding above against the source at the exact commit published against (including hand-tracing the noisy-adapter fixture and the Linux mask-fallback path line by line, not relying on the workers' confidence self-ratings). Checked this repo's AGENTS.md for applicable conventions (platform-gated testing, doc-sync requirements, the subprocess/FFI and WMI-field checklists, the "message asserted with behavior" rule). Read CI results for the exact head commit via the GitHub Actions API/job logs, including pulling the actual failing clippy and windows-build-and-test job logs to get the real compiler error rather than inferring one. Retrieved ROCMAI-824 from Jira directly by key.

Did not run: cargo test/cargo clippy locally (no Rust toolchain available in this review environment) — all test-mutation verdicts ("would this test still pass with the change reverted / with one branch mutated") are from hand-tracing the exact assertions against the exact code, not from execution. No dedicated prior-PRs-on-these-files review-comment-mining pass was run beyond git log/git blame on the touched regions (this PR is a single commit against its merge-base each time). The head commit moved twice during this review (from 3eef3305 to 7844424b, a rebase) — both the fan-out and my direct verification were redone/re-confirmed against the final, current head before publishing.

Unavoidable exposure, disclosed per protocol: the Step-0 claim-check tool call returned the full bodies of prior reviews on this PR (two from juhovainio, one from nowycondro at an older commit, several content-less COMMENTED entries from the author) before this review's findings were produced, since no metadata-only option exists for that check. Nothing read there was used to derive any finding above — every finding here was independently re-traced against the code (two of the four are demonstrated against specific line numbers and, for two of them, the repository's own existing test fixtures), and this report was not reconciled against those threads' text or against the author's replies to them.

---

🤖 Generated with Claude Code

[juhovainio]

I went through this against the current head. The --gpu refusal logic itself matches ROCMAI-824 (refuse a nonexistent index, name the GPUs that do exist, start nothing), but the PR can't merge as is: CI is red on the e2e crate.

Blocking

• assert_no_service_recorded in tests/e2e-cucumber/tests/e2e/serving_steps.rs is already defined on main (the no local server was recorded step). Merged with main, the crate fails with E0428: the name assert_no_service_recorded is defined multiple times. That is what turns clippy, Windows build and test (debug), Windows release build and lifecycle and the windows-build-and-test gate red. It is a real compile error, not a flake. Please reuse the existing step in serve-16, or fold the stricter data-dir check into it.
• Because of that, the PR body's "serve-16 passed on Windows lane" and "e2e clippy passed" are stale for the current merge. Please re-run and update the body once this is fixed.

Non-blocking

• docs/testing.md says the step fails if the services directory cannot be read, but a missing directory is treated as "no records" and only other read errors fail. Please reword.
• The new step was never shown to fail when its target is broken (the PR body says the path was only confirmed via rocm storage report). If you switch to the existing step, say whether it was shown red for serve-16.
• pinned_index_bound queries the Windows adapter listing on every explicit --gpu, even when amd-smi already gave a count. Please confirm those sources (pnputil / PowerShell fallbacks) are deadline-bounded, since this now runs synchronously on the serve path.
• An empty HIP_VISIBLE_DEVICES= is treated as "all hidden" on Windows, copied from Linux/ROCr semantics. I couldn't find anything verifying that against Windows HIP, and a wrong guess hard-refuses every index. Worth a manual check or a softer fallback.

For you to decide

• The adapter count can exceed the HIP-usable GPU count (iGPU + dGPU, unsupported adapters), so the check only guards against typos. That seems reasonable; please confirm it's intended.
• An index that neither source can check is allowed with a warning. That warning now also appears on Linux hosts where both sources fail, which is a behaviour change beyond Windows; please make sure the PR body lists it.

The warning text being produced by one function and asserted as printed, and the pure pinned_index_bound composition being testable on Linux, are both good.

[juhovainio]

assert_no_service_recorded already exists on main (the no local server was recorded step, around line 1606), so this fails to compile after merge (E0428; clippy and both Windows e2e jobs go red). Drop this step and use And no local server was recorded in serve-16, or fold the stricter data-dir check into the existing step.

[volen-silo]

Already gone in the head before this one (the new step was renamed), and now removed entirely in cd0acb5: serve-16 ends with the existing And no local server was recorded step, and my own step and its phrase are deleted.

[juhovainio]

Reuse the existing And no local server was recorded step (already used by another scenario on main) instead of introducing a new phrase for the same assertion.

[volen-silo]

Done: serve-16 now reuses And no local server was recorded. I showed it can go red for serve-16: with that step made to write a record into <data>/services, the scenario fails with "a refused serve must not record a service, found: [...planted.json]". The stricter data-dir equality check is not carried over; the existing step lists the services through the CLI, which reads the same data folder.

[juhovainio]

The step treats a missing services directory as "no records" and only fails on other read errors, so "fails if that directory cannot be read" overstates it.

[volen-silo]

Reworded: docs/testing.md now says the shared step lists <data>/services, a missing folder counts as no records, and any other read error fails the step.

[volen-silo]

Pushed 52125ff: rebased onto main after the host GPU detection split (#829). The change now lives in host_gpu/gpu_visibility.rs (the listing and its tests) and host_gpu/windows_inventory.rs (the displays-only inventory scope and the AMD display filter), with docs/architecture.md updated; behaviour is unchanged. serve-16 passes against a debug build, and the four refusal cases (no mask, empty mask, HIP_VISIBLE_DEVICES=0, HIP_VISIBLE_DEVICES=3) still give the documented messages on this single-GPU host. The Windows-only parts compile only on Windows, so the Linux build is covered by CI.

[nowycondro]

🤖 Automated review (Claude Code) · 52125ff

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 (merge-base d7faa84e…head 52125ffe, 13 files: README.md, apps/rocm/src/gpu_policy.rs, apps/rocm/src/main.rs, apps/rocm/src/serve_cmd.rs, crates/rocm-core/src/host_gpu.rs, crates/rocm-core/src/host_gpu/gpu_visibility.rs, crates/rocm-core/src/host_gpu/windows_inventory.rs, crates/rocm-core/src/lib.rs, docs/architecture.md, docs/manual-testing.md, docs/testing.md, tests/e2e-cucumber/features/model_serving.feature, tests/e2e-cucumber/tests/e2e/serving_steps.rs).
Implements ROCMAI-824 ("rocm serve --gpu <n> accepts a GPU index that does not exist and launches anyway") for the Windows path specifically; the ticket is still "In Progress" and explicitly scopes out ROCMAI-719 (visibility-mask-hidden indices), which this PR also correctly does not touch. The PR does not claim to close the ticket outright, consistent with its own stated scope.

Blocking

• The PR cannot currently be merged: a real content conflict with the current main — apps/rocm/src/serve_cmd.rs (the use crate::gpu_policy::{...} import block)
  Reproduced directly: git merge-tree origin/main pr-head reports CONFLICT (content): Merge conflict in apps/rocm/src/serve_cmd.rs. main has advanced 6 commits past this PR's merge-base, including one (ROCMAI-759, #840) that also edits the same import list this PR edits (adding GpuVramUsage where this PR adds active_visibility_mask, pinned_index_bound, write_unchecked_index_warning). The textual resolution is trivial (combine both additions), but GitHub's own mergeable_state is dirty and CI has not run against the merged result.
  Confidence 100 · mechanical · Fix: rebase onto current main, resolve the import-list conflict, re-run CI.

• amd_display_count() overcounts non-GPU devices whose name merely contains the word "AMD," inflating the very bound this PR adds to stop an absent GPU index from being accepted — crates/rocm-core/src/host_gpu/windows_inventory.rs (is_amd_windows_display, amd_display_count)
  is_amd_windows_display treats any whitespace-delimited token in the lowercased device name equal to "amd", "radeon", or "instinct" as a match. This file's own pre-existing fixture windows_examine_inventory_prefers_real_gpu_over_noisy_amd_pnp_entries already contains exactly this kind of noise: "AMD Bluetooth Capture Audio Device" (tokens include the exact word "amd", no VEN_1002 id) alongside the real "AMD Radeon 780M Graphics". Hand-tracing is_amd_windows_display against that fixture's three rows: the Bluetooth/audio device and the real Radeon GPU both return true (only "AMD-OpenGL User Mode Driver", whose single token is "amd-opengl", is correctly excluded) — so amd_display_count() on this exact existing fixture returns 2, not 1. This path is reached via detect_windows_amd_display_count's CIM fallback (parse_windows_examine_inventory, which pushes every GPU\t... row unfiltered at parse time) and is not corrected by amd_display_count()'s own re-filter, which uses the identical loose match. A host with one real GPU plus any AMD-branded non-display component visible to this probe would inflate the bound and let an otherwise out-of-range --gpu <index> pass validation — reproducing the ROCMAI-824 failure class (an absent index silently accepted) through a different mechanism than the one this PR closes. The function's own doc comment only anticipates real adapters HIP can't use being overcounted, not non-adapter devices being misclassified as adapters, and the new test added for this function (the_gpu_index_check_does_not_count_another_vendors_adapter) only proves a different-vendor (Intel) adapter is excluded — no test exercises the file's own noisy-AMD-name fixture against amd_display_count().
  Confidence 85 · logic · Fix: require a PCI VEN_1002 id (not a bare name-token match) before counting toward the index bound, or reuse preferred_amd_display's stricter ranking; add a test asserting amd_display_count() == 1 on the existing noisy fixture.

• The new "index accepted without a full check" warning never fires for the pre-existing Linux case its own doc comment says it covers — apps/rocm/src/gpu_policy.rs (pinned_index_bound, PinnedGpuIndex::unchecked_warning's doc comment, validate_pinned_gpu_index) vs. crates/rocm-core/src/host_gpu/gpu_visibility.rs (probe_usable_amd_gpu_indices, usable_amd_gpu_indices_from)
  pinned_index_bound only sets mask_unresolved: true by invoking the windows closure (rocm_core::windows_amd_gpu_listing) when visible.is_none(); off Windows that closure always returns WindowsGpuListing::Unavailable, so mask_unresolved is hard-wired to false on every non-Windows host regardless of why visible became None. But on Linux, usable_amd_gpu_indices() → usable_amd_gpu_indices_from can itself return None specifically because HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES holds a token that cannot be read by ordinal (a UUID, or an out-of-range value) — documented in that function's own comment ("None when the mask cannot be interpreted by ordinal alone..."). So: a Linux host where the KFD/DRM probe succeeds but the mask is unresolvable, with amd-smi still answering a count, produces visible = None, detected = Some(count), mask_unresolved = false (incorrectly) → validate_pinned_gpu_index accepts an in-range index with unchecked_warning: None — silently, exactly the "accepted without a full check" case PinnedGpuIndex::unchecked_warning's doc comment claims is now always flagged ("neither source could list the host's GPUs, or HIP_VISIBLE_DEVICES could not be resolved... so an unchecked index is never accepted silently"). No test in this diff exercises visible: None paired with a real (non-Windows-simulated) unresolved mask; every mask_unresolved: true test constructs it via the injected Windows closure.
  Confidence 85 · logic · Fix: derive mask_unresolved from whether visible became None because mask-resolution itself failed (platform-neutral), not solely from the Windows-only closure.

• --gpu --help text omits the "still refused" exception and now contradicts the fuller wording this same diff adds to README — apps/rocm/src/main.rs (--gpu doc comment) vs. README.md's new --gpu paragraph vs. apps/rocm/src/gpu_policy.rs's validate_pinned_gpu_index
  The new help text reads: "If the GPUs cannot be listed, or HIP_VISIBLE_DEVICES cannot be read as ordinals, the index is used with a warning that it was not fully checked." Read plainly this is unconditional. But the code — and this PR's own, more complete README paragraph added in the same diff — says an unresolved mask is only accepted-with-warning when the index is below the GPU count; at or past the count it is still refused ("--gpu index {index} is out of range...HIP_VISIBLE_DEVICES could not be resolved...so the count is the only bound", directly asserted by the test an_unresolvable_mask_still_refuses_an_index_past_the_adapters). So the --help text disagrees with the README text this PR writes in the same commit, and the --help text is the one that is wrong.
  Confidence 90 · logic (doc accuracy) · Fix: align the --help text with the README wording already in this diff.

Non-blocking

None

Decisions for the author

• Windows adapter-listing order is assumed to align with the amd-smi/HIP ordinal order on multi-GPU hosts — tradeoff
  pinned_index_bound cuts the Windows adapter set to the amd-smi count by raw index when both are known; these are two independently-probed enumerations not guaranteed to agree on ordering. The commit's own comments already disclose this as an accepted risk ("may refuse, never picks"). Worth reconfirming this is the intended scope for an initial release.
• The new Windows bound is wired in only at rocm serve's call site — tradeoff
  engines/lemonade, engines/vllm, engines/aim, and any later index replay (e.g. a service restart) still resolve solely through usable_amd_gpu_indices(), which stays None on Windows — the same unchecked-index exposure persists via those paths, reachable by invoking an engine directly rather than through rocm serve. This may be a deliberate scope limit (the referenced ticket explicitly separates this bug from ROCMAI-719's broader mask-visibility check), but is worth the author confirming it's intentional for this PR's scope rather than an oversight.

Positive signals

• WindowsGpuListing's three-way split (Unavailable / Indices / MaskUnresolved { adapters }) cleanly separates "nothing knowable" from "a known count with an unreadable mask" from "a known usable set," with no illegal-state conflation found in windows_gpu_listing_from or its callers.
• pinned_index_bound only invokes the Windows probe closure when visible.is_none(), so Linux hosts never pay for a Windows-only enumeration — a deliberate, well-documented cost-avoidance design, and --gpu auto is structurally prevented from ever reaching the new validation path (the serve_cmd.rs match arm only binds GpuSelection::Index), so the stated "may refuse, never picks" invariant holds by construction, not by convention.
• the_refusal_names_only_the_visibility_variable_that_acts_on_the_platform correctly encodes that native-Windows HIP has no ROCr layer, so a refusal never blames ROCR_VISIBLE_DEVICES there — a real, previously-reported class of bug that this code demonstrably gets right now.
• The e2e step for "no local server was started for it" correctly distinguishes "services directory absent" (pass) from any other read error (hard failure), rather than swallowing an unrelated I/O error as a vacuous pass — and independently verifies the CLI under test was actually handed the data directory the step reads, rather than trusting a hardcoded path.

Deployment notes

User-facing behavior change on Windows: an explicit --gpu <index> that exceeds the detected AMD-adapter count is now refused (previously silently accepted and a managed service launched); a new stderr warning appears when no source can enumerate GPUs at all, or when HIP_VISIBLE_DEVICES can't be resolved to ordinals but the index is still in range. No migration or config changes.

CI for head 52125ffe is green on every check that ran (clippy, Linux/Windows builds, unit and e2e suites, CodeQL, doc/link checks); Skill checks (skillscope) is skipped (advisory, not yet a required gate per this repo's own AGENTS.md). However, GitHub reports mergeable_state: dirty — see the first Blocking item; this is not reflected in any CI check and must be resolved by rebase regardless of check results.

What this covered

Read every file in the diff completely against merge-base d7faa84ec4c295b7b83ce6b830e05e78e7185de9…head 52125ffece8da045a921ee966daa8d1da090b0cb, plus surrounding code (all callers of validate_pinned_gpu_index, pinned_index_bound, windows_amd_gpu_listing, usable_amd_gpu_indices repo-wide, including engines/lemonade, engines/vllm, engines/aim), via two independent fan-out workers (host_gpu.rs + gpu_visibility.rs + windows_inventory.rs + lib.rs; and a third worker covering AGENTS.md adherence that did not return in time, see below) plus my own direct, complete reads of gpu_policy.rs + serve_cmd.rs, main.rs's changed help-text block, README.md, docs/architecture.md, docs/manual-testing.md, docs/testing.md, and both e2e files (feature + step definitions). Checked this repo's AGENTS.md directly for doc-sync, test-mutation, platform-gated-testing, and subprocess/FFI rules. Verified the reported CI conclusions via the GitHub Actions API for this exact head commit, and independently reproduced the merge-conflict finding with git merge-tree rather than trusting mergeable_state at face value. Retrieved ROCMAI-824 from Jira directly by key for the Step 9 comparison.

Did not run: cargo test/cargo clippy locally (no Rust toolchain in this review environment) — test-mutation verdicts are from hand-tracing assertions against code, not execution. Two of five planned fan-out workers (gpu_policy.rs+serve_cmd.rs; docs+main.rs help text; and the AGENTS.md/history/prior-changes/code-comments source-reading pass) failed to launch twice due to a shared-environment concurrent-subagent-limit error unrelated to this PR; that ground was covered directly by me instead (not independent) and is disclosed here rather than presented as fan-out coverage. A fourth worker (e2e feature + step definitions) was spawned successfully but had not returned by publish time; that file group was also covered directly by me as a backup (confirmed no duplicate step text/symbol collision repo-wide, confirmed the step's data-directory resolution and error handling by direct read) and should be treated as single-context, not independent, for that group. One independent worker (host_gpu core modules) completed and corroborated the amd_display_count finding above via its own separate hand-trace against the same fixture.

A review from a different author already exists on this PR at an earlier commit; GitHub's review list was read once, mechanically, only to confirm the current head's review state and avoid double-posting — this incidentally showed prior review text (including an earlier instance of this same automation's own review at a prior head, which had found CI red from a duplicate-symbol compile error now fixed, plus several logic findings resembling two of the ones above). Those findings were not used to derive anything in this report; every finding here was independently re-traced against the current code from scratch, and this report was not reconciled against that or any other prior review's text or the author's replies to it.

---

🤖 Generated with Claude Code

[volen-silo]

Pushed cd0acb5 (rebased onto main). On the two reviews of the previous head:

• The duplicate-step compile error was already fixed in the previous head (the step was renamed); it is now removed in favour of main's no local server was recorded step, with red evidence (thread).
• amd_display_count bug: confirmed on the repo's own noisy fixture (it returned 2: "AMD Bluetooth Capture Audio Device" matched the loose name token). A row now counts only with a PCI\VEN_1002 id or a marketing name rocm examine recognises; a test pins 1 and fails with the old filter.
• Linux fallback: with the visible set unknown and a mask set, an index below the count is accepted with a warning that the mask was not checked, and at or past the count is refused; test added.
• --help text: now matches the code and the README (refused at or past the count, otherwise warned).
• Probe deadlines: every Windows listing source runs under a 5 s timeout and is killed on expiry; the listing is also skipped when amd-smi gave a count and no mask is set.
• Empty HIP_VISIBLE_DEVICES= on Windows: I could not verify Linux semantics for Windows HIP (the documentation lists the variable but says nothing about an empty value), so it is now treated as an unresolved mask (bounded by the count, with a warning) instead of refusing every index.
• Decisions: the adapter-order tradeoff and the new warning on Linux hosts where nothing can list GPUs are both written up in the description.

The Windows-only live listing is exercised only by the Windows lane; serve-16 passed locally against a debug build for this head.

The blocking items are addressed in cd0acb5 and the updated description (the step is reused, the display count and the Linux fallback are fixed, and the help text matches); see the replies and the summary comment. A re-review is welcome.

[nowycondro]

🤖 Automated review (Claude Code) · cd0acb5

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 (base 02421f1…head cd0acb5, 16 files), split across five coupled-file workers (gpu_policy.rs+gpu_visibility.rs; windows_inventory.rs+host_gpu.rs+lib.rs; serve_cmd.rs+main.rs; README.md+docs/testing.md+docs/manual-testing.md+docs/architecture.md+both workflow YAMLs; model_serving.feature+expectation.rs+serving_steps.rs), plus a driver-level git comparison against the base commit's own parent. No Windows/amd-smi hardware was available to this review; correctness is reasoned from the code and its unit tests only.
Implements ROCMAI-824 ("rocm serve --gpu <n> accepts a GPU index that does not exist... on Windows") for the core bug: an out-of-range --gpu index on native Windows is now refused, the existing indices are named, and nothing starts. The PR's own description also states "This change is Windows-only in behaviour" — the first Blocking item below disputes that characterization.

Blocking

• The PR silently reverts a just-merged, deliberate CI/test fix (PR #852), undisclosed in the PR text, and in tension with the PR's own "Windows-only in behaviour" claim. — .github/workflows/e2e-selfhosted.yml (Strix Ubuntu job timeout-minutes), .github/workflows/nightly.yml (e2e-gpu-nightly-strix timeout-minutes and the deleted "Report whether Docker is reachable" step), docs/testing.md (deleted serve-26/serve-27 @nightly explanation), tests/e2e-cucumber/features/model_serving.feature (deleted @nightly tags on serve-26/serve-27), tests/e2e-cucumber/src/expectation.rs (deleted aim_serve_scenarios_stay_off_the_per_pr_lanes test).
  This PR's base commit, 02421f1, is itself "ci(e2e): move the AIM serve scenarios off the per-PR Strix Ubuntu lane (#852)" — a fix for the per-PR Strix Ubuntu E2E lane being cancelled at its 45-minute cap (dropping merge-queue entries) whenever AIM's serve-26/serve-27 scenarios ran per-PR. Diffing this PR's head directly against 02421f1's parent (git diff 02421f1^ cd0acb5 -- .github/workflows/nightly.yml .github/workflows/e2e-selfhosted.yml tests/e2e-cucumber/src/expectation.rs) produces zero output — i.e. this PR's head is byte-for-byte identical to the pre-#852 state in exactly the files #852 touched. The PR description never mentions touching CI timeouts, scenario tagging, or deleting a test; it only says (in "Not verified") that "the same two [Strix Ubuntu] lanes fail the same way on several unrelated open PRs. This change is Windows-only in behaviour" — which reads as the author believing this is pre-existing flakiness unrelated to their diff, when their own diff is what reintroduces the exact condition #852 fixed. CI is currently green on this run, but only because that likely means the current Strix Ubuntu runner didn't have the AIM engine/Docker available this time — per the reverted test's own logic, the timeout only manifests when it does.
  Confidence 95 · architectural · Fix: re-apply #852's hunks (restore the @nightly tags, the two timeouts, the Docker-reachability step, the docs/testing.md paragraph, and the deleted unit test), most likely via a rebase of this branch onto current main.

• A non-UTF-8 HIP_VISIBLE_DEVICES value on Windows is read as "unset" rather than "unresolved," defeating the PR's own "never accept an unchecked index silently" invariant. — crates/rocm-core/src/host_gpu/gpu_visibility.rs:119 (windows_amd_gpu_listing, using std::env::var("HIP_VISIBLE_DEVICES").ok()), contrasted with the Linux path at gpu_visibility.rs:214 (var_os(...).map(|v| v.to_string_lossy()...)).
  std::env::var(...).ok() collapses Err(VarError::NotUnicode(_)) (variable present but not valid Unicode) to None, indistinguishable from "not set." That None reaches windows_gpu_listing_from(adapters, None), which takes the "no mask" branch and returns WindowsGpuListing::Indices(0..adapters) — the full adapter range, treated downstream as a fully-resolved, authoritative visible set, accepted with no warning. This is exactly the "accepted without being checked against the real mask" failure this PR exists to close, just via a mask shape (non-UTF-8) the PR's explicit handling (empty value, UUID, out-of-range ordinal) doesn't cover. The Linux code one function away was deliberately written to avoid this exact collapse.
  Confidence 72 · logic · Fix: read via std::env::var_os + to_string_lossy() (matching the Linux path) so an undecodable value is treated as MaskUnresolved, not "no mask."

• README's new --gpu prose overclaims when the Windows adapter list and amd-smi count are combined. — README.md, the "Model serving" --gpu paragraph.
  The added text leads with "the index is checked against the AMD display adapters Windows reports (and against the amd-smi count when that works, whichever is lower)," implying both sources are always combined. Four sentences later the same paragraph correctly says "When amd-smi gives a count and no mask is set, the Windows list is not queried" — i.e. in the common unmasked case only one source is ever consulted, never a two-source minimum. Traced against pinned_index_bound in gpu_policy.rs: the Windows listing is only intersected with the amd-smi count in the one case where a mask is set and amd-smi also answers. docs/testing.md's parallel description of serve-16 gets this right; README's lead sentence does not, and AGENTS.md requires keeping docs and behavior claims in sync.
  Confidence 82 · mechanical · Fix: qualify the lead sentence to match the masked-only intersection, as docs/testing.md already does.

Non-blocking

None.

Decisions for the author

• Accepting an unchecked --gpu <index> with only a warning, rather than refusing, when no source can enumerate the host's GPUs at all — tradeoff. The linked ticket (ROCMAI-824) asks to "Refuse an index that does not exist... and start nothing," and this fallback (preserved from pre-existing Linux behavior, disclosed candidly in the PR's own "Behaviour changes"/"Not verified" sections) still launches on a fully-unprobeable host. Fail-open preserves serving on exotic/unsupported hosts rather than blocking every unprobeable machine; fail-closed would match the ticket's literal wording more closely. Confirm this is the intended scope before merge.
• amd_display_count's strict PCI-id prefix check (starts_with("PCI\VEN_1002")) and is_amd_windows_display's looser substring check (contains("VEN_1002")) are two independent implementations of "is this an AMD PCI id." — non-blocking-improvement. They agree for every realistic pnputil/CIM id shape today, but nothing ties them together, so an independent future edit to either could silently drift. Consider one shared helper.
• No test wires pinned_index_bound for "amd-smi gives a count, a mask is set, but the Windows listing itself is Unavailable." — non-blocking-improvement. Behavior is correct by inspection (falls through to the existing "mask set but GPUs could not be listed" warning path), but a dedicated test would close a real coverage gap rather than relying on inspection.

Positive signals

• pinned_index_bound's tests use a panicking closure to prove the Windows listing is never invoked when a visible set or an unmasked amd-smi count already answers — a mutation-resistant test design, not just a value assertion.
• The "never loosen a check another source makes" invariant (Windows list cut to the amd-smi count) is explicitly tested in both directions (amd-smi stricter and amd-smi looser).
• The PR description is unusually rigorous about its own limits: it names the two other --gpu consumers (engines/lemonade, engines/vllm) that share this bug class and explains why fixing them is deliberately out of scope, and it discloses the adapter-ordering risk on multi-GPU Windows hosts honestly.

Deployment notes

• Behavior change on native Windows: an explicit --gpu <index> past the detected adapter count is now refused where it previously launched silently.
• Behavior change on all platforms: when no source can enumerate the host's GPUs at all, rocm serve now prints a new warning: line on stderr for a pinned --gpu <index> it previously accepted silently — a script strictly parsing rocm serve's stderr for emptiness could be affected.
• See the first Blocking item: merging as-is reopens the per-PR Strix Ubuntu E2E timeout/merge-queue-drop risk that #852 had just closed.

What this covered

Every file in the diff, read in full by five independent workers grouped by contract coupling, plus this driver's own direct git comparison of the PR's base commit (02421f1) against its parent to establish the CI-revert finding, and a design-review pass over the consolidated results. CI check-runs for cd0acb5 were read directly from GitHub: all 25 completed, none failing (one skipped, "Skill checks (skillscope)"), so there is no separate blocking CI-check finding — but see the note above about why a green run here doesn't retire the CI-revert finding.

Did not run: the "prior changes" pass (whether review comments on earlier changes to these files apply here) was skipped by this review's own operating constraint of never reading this PR's review/discussion threads; the "history" pass beyond the specific git log/git show checks used to establish the CI-revert finding was not a full blame sweep of every changed line.
Not reconciled against any discussion on this PR. One earlier review's body text on this PR was inadvertently and briefly visible in a raw tool-output preview while this review was claim-checking for a prior, differently-pinned review (an artifact of the tool's output truncation, not a deliberate read) — it was not used as a source for, nor did it shape, any finding in this report; every finding above was independently (re)derived from the diff, git history, and this PR's own description/ticket.

---

🤖 Generated with Claude Code

[juhovainio]

Approving. The core fix does what ROCMAI-824 asks on native Windows: a --gpu index past the adapter list is refused, the valid indices are named, and nothing is started. Nothing below blocks merge.

Non-blocking notes:

• The PR body's Tests bullet says an empty HIP_VISIBLE_DEVICES means "none visible", but the code and tests treat it as unresolvable (and the "Change" section says so). Worth correcting the bullet.
• amd_display_count can undercount on the CIM fallback path (inline comment), which sits uneasily with the claim that a mismatch never refuses a valid index.
• In serve-16 on Windows, the no local server was recorded step's !listed.contains("vllm-") check can never fail for a lemonade id; the empty services directory check is what actually asserts. assert_absent_index_message also still accepts the engine's own late wording, so the scenario leans on that step to show the CLI pre-flight fired rather than the engine.
• The change is large for the ticket (about 1400 lines), and it includes Linux-visible behaviour: a new stderr warning: when no GPU list is available for an explicit --gpu. All of it is documented, but it could have been split out.
• validate_pinned_gpu_index now takes five parameters that mostly unpack a PinnedIndexBound (inline comment).

Questions for the author:

• An index that cannot be proven absent is still accepted with a warning, so "refuse an index that does not exist" does not hold on hosts where nothing can list GPUs. Is that the intended trade-off?
• The engines and rocm services restart still pass a Windows index unchecked. I'd suggest a follow-up ticket.

CI is green, but I don't see a Windows GPU lane in the check list, so serve-16 on real Windows hardware has no CI evidence for this head; the PR body says it passed locally.

[juhovainio]

This counts a row only with a PCI\VEN_1002 id or a marketing name the table recognizes. CIM-fallback rows can have no PnP id, so on a multi-adapter host an AMD GPU newer than the name table goes uncounted and a valid --gpu 1 could be refused. That contradicts the PR body's "never refusing a valid one". Either use the looser pnputil-style test for rows with no PnP id, or narrow the claim.

[juhovainio]

Minor, judgement call: this builds three near-identical PinnedIndexBound values, and validate_pinned_gpu_index then takes their fields back as five separate arguments. Passing the bound itself (or a small mask-state enum in place of mask + mask_unresolved) would shrink the call surface.