feat(diagnose): answer whether a model will run before downloading it

[volen-silo]

What this adds

rocm diagnose --model <ref> answers "will this model run on this machine" from
the curated recipe catalog and the host's own facts. It fetches nothing, so the
answer costs seconds instead of a failed multi-gigabyte download.

The environment diagnosis is unchanged. The model verdict rides on top of it.

The four verdicts

Verdict	Meaning
READY	The recipe's requirements are met here
DEGRADED	Hard requirements pass, but system RAM is below the recipe's recommendation
BLOCKED	A hard requirement fails: no usable GPU, not enough GPU memory, or no preferred engine available on this platform
UNDETERMINED	The question could not be answered: the catalog could not be read, the model is outside it, GPU memory could not be measured, or (APU only) telemetry exists but names the wrong memory pool (UnifiedMemoryUnreadable)

UNDETERMINED is kept distinct from BLOCKED on purpose. A catalog that could
not be read is a gap in what the CLI can see, not a fact about the model.
Collapsing the two would tell users a model is incompatible on the strength of a
failed read, which is the one answer here that is worse than no answer.

Design decisions worth review

Engine selection is passed in, not re-derived. select_serve_engine remains
the single place that answers "which engine serves this model on this host".
Doctor calls it rather than reimplementing the preference order, so it cannot
name an engine that rocm serve would not pick. A test drives both call sites
and compares.

An engine ruled out by the platform blocks the model. It is not silently
swapped for another engine. rocm serve does not fall back either, and a verdict
that pretended otherwise would be wrong in the user's favour.

Alternatives are filtered, not just forwarded. A recipe's declared
alternatives are editorial: an entry can name something smaller that is still too
large for this machine. Offering it unfiltered sends the user into a second
failed download, which is the failure this command exists to prevent.

GPU memory is the single most-capable visible card, not the sum of every
card. rocm serve pins exactly one GPU ordinal; there is no tensor-parallel
path anywhere in this binary. A homogeneous multi-GPU host (e.g. 8x192 GiB)
summing to ~1536 GiB would have reported READY for a model that OOMs on every
individual card. The figure is now the largest mask-visible card
(rocm_core::usable_amd_gpu_indices), the best case --gpu auto could actually
land on.

--model is refused with --distro. The verdict is about this machine, so
answering it for a declared distro would be answering a different question.
Keyed on whether --distro was passed, checked before any probe runs -- not on
a derived examination property -- so the refusal cannot depend on probe
internals and needs no reachable WSL distribution to exercise.

/model's missing-VRAM-reading message points at rocm diagnose --model.
Every production caller of this rendering path hardcodes
aggregate_gpu_vram_gib = None, so a prior wording naming amd-smi metric --json directly was itself a dead end: nothing the user runs changes what this
function sees. It now points at the command this PR adds, which does thread a
live reading through.

A measured GiB figure is floored, not rounded, for display. A card reporting
a hair under its nameplate size (8176 MiB = 7.9844 GiB) used to round to "8.0
GiB" against an "8 GiB" recipe minimum on a BLOCKED verdict -- evidence that
read as an exact match while the verdict said otherwise. required/
recommended values are always exact integers, so only measured figures are
affected.

Testing

• 11 unit tests in rocm-core, 15 in apps/rocm, and 6 end-to-end scenarios (diagnose-21 through diagnose-26).
• cargo test --workspace --all-targets, cargo clippy --workspace --all-targets -- -D warnings, cargo clippy -p e2e-cucumber --test e2e -- -D warnings, and cargo fmt --all -- --check are clean.
• cargo xtask e2e -- -n diagnose-2 passes all 7 scenarios / 34 steps.
• Each new/changed invariant was checked by mutation: the production code was broken deliberately (including a full revert of the --distro keying fix, confirmed to make diagnose-26 fail for the predicted reason) and the test confirmed to go red, then restored.

Known limitations

• Not exercised on a GPU host. This was developed on a machine with no AMD GPU, so the ready and blocked-with-a-measurement paths ran only in unit tests and on the no-GPU branch of the scenarios. The scenarios are written host-agnostically, so both halves are real assertions on a lane that can reach them.
• Not exercised on Windows. The native-Windows engine gate is covered by a unit test that injects the condition, not by a real Windows run.
• On a host with both an APU and a discrete GPU, the memory figure falls back to the existing single-GPU heuristic rather than a new rule.

Follow-up, not in this PR

GPU memory is read through amd-smi. A direct amdgpu sysfs fallback would let
the check still answer on hosts where amd-smi is missing, at the cost of not
covering APU unified memory. Worth doing as its own change.

[siloteemu]

🔴 Automated review · pr-review-watcher · 095c501

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.

Summary

Adds rocm diagnose --model <ref>, a pre-download READY/DEGRADED/BLOCKED/UNDETERMINED verdict computed from the curated recipe catalog plus host facts. The core logic and its tests are unusually solid; the defects are all in the host-fact layer that feeds it, where three paths produce a confident verdict from a measurement that does not mean what the code treats it as meaning — outcome: Needs work. Verified: mutation-tested all nine new tests on a throwaway copy (twelve mutations, eleven killed) — the engine is genuinely passed in, not re-derived (re-deriving from recipe.preferred_engines fails the named cross-check test), UNDETERMINED and BLOCKED are separately pinned, declared-alternative filtering is pinned, the renderer's claimed prefix coupling with the diagnosis renderer is accurate, the "8 unit tests" count is exact, and tracing every call on the path (catalog load, config discovery, GPU summary, amd-smi) found only local file reads and local subprocesses, so "fetches nothing" holds; a leak and injection scan over the whole diff was clean. Checks at review time: 24 success, 3 failures, 1 cancelled. Blocking: 3 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

apps/rocm/src/main.rs:2853 — the APU pool is total system RAM, which over-reports what the engine can actually allocate, and over-reporting yields READY for a model that will not fit.
The doc comment three lines above correctly explains that on an APU the allocator draws from GTT-backed system memory rather than the BIOS carve-out — and then supplies detect_system_ram_gib(), the full RAM figure, as that pool. GTT is not system RAM: the amdgpu driver caps the aperture well below total RAM by default, and the rest of RAM is in use by the OS and everything else on the box. This repository already knows the difference — the self-hosted preflight in .github/workflows/e2e-selfhosted.yml was changed on the base branch specifically to stop applying a floor to a pool the engine never draws on, and it measures the GTT total explicitly rather than assuming it equals RAM. A model sized between the real GTT cap and total RAM therefore reads READY here and then fails to allocate when served — precisely the wasted download this feature exists to prevent, on the exact hardware class (gfx1151) the feature's own doc comment uses as its motivating example. Fix: read the GTT pool total the way the preflight does and use that as the UnifiedSystemMemory figure; where no GTT pool can be read, return AcceleratorMemory::Unknown so the answer lands in UNDETERMINED rather than in an optimistic READY. A conservative fraction of RAM is an acceptable fallback if reading GTT is impractical, but the unreadable case must still degrade to Unknown, not to total RAM. Note that a real GTT read does not regress the motivating case — a 22 GiB model on a 128 GiB Strix Halo still fits comfortably inside a default aperture.

apps/rocm/src/main.rs:21146 — a VRAM total of 0 is accepted as a measurement, producing a confident BLOCKED where the honest answer is UNDETERMINED.
parse_gpu_vram_usage requires only that total_vram parse as a u64, and 0 parses. gpu_vram_usage()'s emptiness guard does not catch this because the row exists; host_accelerator_memory then sums to 0.0, vram_capacity_is_meaningful is true for a single discrete card, and the result is AcceleratorMemory::Dedicated(0.0). In the core, measured_gib() returns Some(0.0) rather than None, so the AcceleratorMemoryUnknown branch is skipped entirely and the host is told "this host offers 0 GiB" and BLOCKED. This directly contradicts the module's own stated non-collapse that unmeasured memory is not absent memory, and it is not hypothetical: the same zero-total reading from a wedged driver was observed and explicitly rejected on the workflow side of this repository. Fix: reject non-positive totals in parse_gpu_vram_usage (filter the row, or make a zero total yield None), so an all-zero reading reaches the Unknown path instead of the Dedicated one.

apps/rocm/src/main.rs:2840-2844 — on WSL the Unknown branch is unreachable, so an unmeasurable GPU is reported as no GPU.
The branch distinguishes "no GPU" from "GPU present but unmeasured" using examination.has_amd_gpu. That field is assigned in exactly one place, summarise_gpu_categories, which is called on the Linux and Windows probe paths only; the WSL path returns early before it and never populates gpus at all, so has_amd_gpu stays at its false default on every WSL host regardless of hardware. Consequently, on WSL, whenever amd-smi cannot be read — the case this branch exists for — the verdict is AcceleratorMemory::None, which the core turns into BLOCKED with "no GPU is visible to ROCm… there is no CPU fallback", on a machine whose GPU works. The root cause predates this change, but this is the first caller that converts it into a user-visible wrong verdict, on a platform with dedicated lanes here. Fix inside this diff's own function: when the examination reports WSL, treat an unreadable amd-smi as AcceleratorMemory::Unknown rather than None, since has_amd_gpu carries no information on that path. (A WSL distro with genuinely no passthrough then reports UNDETERMINED instead of BLOCKED — the correct trade, and the safe direction, since it cannot manufacture a READY.)

Non-blocking

• apps/rocm/src/main.rs:2882 — forcing unsupported_here to None here kills no test: the platform gate is pinned only at the core level from hand-built fixtures, so a regression that stopped the integration from populating it at all would go undetected. One assertion over a Windows-plus-vLLM host closes it.
• README.md:262 — the "Diagnose and fix" section's exhaustive signature still reads rocm diagnose [--symptom TEXT] [--top N] [--json] [--distro [NAME]], omitting --model even though the same file documents the flag a few sections later; the contributor rules require the flag surface to move in the same change.
• apps/rocm/src/main.rs:2745 — the --model + --distro refusal has no coverage at all, neither unit nor scenario, despite being a user-facing error path.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1419 — the READY scenario's assertions return early when no GPU memory was measured, so on the blocking GPU-less lane the whole scenario degenerates into re-testing the no-GPU path the BLOCKED scenario already covers; READY is proven only on non-blocking lanes.
• crates/rocm-core/src/model_readiness.rs:11 — the feature answers "will this run" but never checks free disk space, although artifact records already carry size_bytes and a disk-space module exists; a host that fits the model in memory but not on disk still gets READY.

[juhovainio]

Reviewed this against the PR description. There's already a review here from another reviewer flagging real problems in the host-fact layer (APU memory misclassification, WSL GPU detection) — I checked two of those independently by reading the code and they hold up, so I won't repeat them. Here's what I found on top of that:

The one that matters most: assess_model_on_this_host calls detect_host_gpu_summary(None) for its own GPU read, but a few lines later it separately calls AppPaths::discover() for the config lookup. That means the gfx-target fallback that comes from reading the TheRock SDK manifest (which serve's own engine-selection path uses) never gets applied here. On a host that only resolves its GPU target through that fallback, rocm diagnose --model and rocm serve can end up naming different engines for the same model — which is exactly the inconsistency this PR's own doc comment says assess_model_for_host exists to prevent. Left as an inline comment.

Docs didn't keep up with the new command. README.md got a new section (nice writeup), but docs/testing.md's WSL Preflight section and docs/manual-testing.md weren't touched, even though this adds new observable CLI behavior worth a manual-test entry.

Test count in the PR description doesn't match the diff. It says 3 new tests in apps/rocm; I only found 1 (the_engine_doctor_names_is_the_engine_serve_would_select). Not a big deal, just flagging so the description gets fixed or I'm missing something.

Quiet behavior change. The /model command's VRAM-missing message text changed from pointing at /examine to pointing at amd-smi metric --json. Probably the right call, but it's not called out anywhere in the PR body, and it's a message an existing command already prints today, not new-command scope.

E2E coverage gap. The 3 new scenarios cover BLOCKED, READY, and one flavor of UNDETERMINED, but not DEGRADED or the ModelNotCurated reason — both are verdict words a user can actually see.

Smaller stuff, not blocking: model_readiness.rs's core functions all take the same 4-5 params together (model_ref, catalog, host, engine_for) which might want bundling into a context type down the road; a few of the #[allow(dead_code)] helpers in main.rs got refactored to delegate into model_readiness but are still unreachable — worth double-checking they're actually wired up somewhere; and there's a theoretical edge case where a cpu_only recipe with a declared min_gpu_mem_gb on a true no-GPU host might report Undetermined instead of Blocked, though I couldn't confirm real catalog data hits this.

Requesting changes mainly for the engine-selection consistency bug and the docs gap — the rest is worth a look but not blocking on its own.

[juhovainio]

This calls detect_host_gpu_summary(None) directly, but AppPaths::discover() is resolved a few lines below for the config lookup. serve's engine selection uses the TheRock-SDK-manifest gfx-target fallback that only kicks in when AppPaths is available — this path never gets it, so on a host that depends on that fallback, this can name a different engine than rocm serve would actually pick. Worth calling AppPaths::discover() once up front and threading it into detect_host_gpu_summary too, if that's what unlocks the fallback.

[volen-silo]

Fixed: assess_model_on_this_host now calls AppPaths::discover() once and threads the result into both detect_host_gpu_summary and the config lookup (assess_model_with_host_paths), so they can never resolve different paths. A regression test (the_configured_default_engine_reaches_the_readiness_verdict_from_the_same_paths_as_the_gpu_summary) drives both call sites and compares.

[juhovainio]

Good writeup here, but docs/testing.md (WSL Preflight section) and docs/manual-testing.md weren't updated to mention rocm diagnose --model, despite it being new observable behavior worth a manual-test entry.

[volen-silo]

Fair gap, reporting rather than fixing: docs/testing.md now has a "Model Fit Preflight" section covering --model, but I left docs/manual-testing.md alone. That file's sections are interactive smoke-test walkthroughs for things without automated coverage (SDK install, server records, ComfyUI); diagnose --model already has unit, integration, and 5 e2e scenarios, so there's no manual-only gap to fill there today. Happy to add an entry if you'd rather have it regardless.

[juhovainio]

This changes the wording of an existing /model message (VRAM-missing action line, pointing at amd-smi instead of /examine) rather than adding new text. Reasonable change, but it's not mentioned in the PR description and isn't really in scope for a diagnose --model PR — might be worth its own commit/PR note.

[volen-silo]

Agreed it's a scope edge, but it's not incidental: the old /examine pointer was actively wrong (that command emits a static snapshot with no VRAM figure), and diagnose --model needed to name the correct remediation (amd-smi metric --json) for the same gap it just introduced — leaving /model pointing at a dead end while the new command pointed at the real fix would have been an inconsistency in the same release. Called out explicitly in the PR description's review-fixes list rather than a separate PR, since it's a one-line, low-risk correction directly motivated by this change.

[juhovainio]

The PR description says 3 new tests were added in apps/rocm, but this is the only new #[test] I could find in this diff. Worth fixing the description, or let me know if I'm missing two more somewhere.

[volen-silo]

Description was stale, fixed it. Current diff has 9 new #[test]s in rocm-core/src/model_readiness.rs and 13 in apps/rocm/src/main.rs (both counts grew again from further review rounds since your pass); description now reads '9 unit tests in rocm-core, 13 in apps/rocm, and 5 end-to-end scenarios' to match.

[juhovainio]

These three scenarios cover Blocked, Ready, and one Undetermined reason (unreachable catalog), but not Degraded or UndeterminedReason::ModelNotCurated — both are verdict words a user can see in the actual output. Worth adding at least one scenario for each before merge, per the Gherkin-coverage expectation for new observable behavior.

[volen-silo]

Already fixed: scenarios diagnose-24 (UndeterminedReason::ModelNotCurated) and diagnose-25 (Degraded, via a synthetic signed catalog since no built-in recipe can produce it against an arbitrary real host) were added covering exactly these two gaps.

Addressed

[siloteemu]

🔴 Automated review · pr-review-watcher · f0a8a68

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.

Summary

Adds rocm diagnose --model <ref>, which answers "will this model run here" from the curated recipe catalog and local host facts without fetching anything, reporting one of four verdicts. Outcome: Needs work — the round-3 blockers are genuinely fixed, but the fix for the APU one left behind an unreachable code path that the module still documents as live, and the new path now gives APU users remediation advice that cannot work. Verified: ran the targeted unit suites for the two touched crates (cargo test -p rocm-core model_readiness and filtered cargo test -p rocm --bin rocm runs) green on scratch copies, then mutation-tested each new test against the exact production line it names — the full test suite and the e2e suite were not run here; independently confirmed that no production code anywhere in the workspace constructs AcceleratorMemory::UnifiedSystemMemory, that HostFacts has exactly one production construction site, that the degraded e2e fixture now declares lemonade, that the catalog loader opens no socket (so the "no network call" claim holds), that the commit carries a DCO sign-off, and that nothing in the diff leaks internal references or contains prompt-injection text; the review worked from check counts success=26, failure=1, pending=1, with the single failure on a GPU hardware lane of a family that is intermittently red on trunk, so it is not attributed to this diff. Blocking: 3 · Non-blocking: 5.

Previous round

• Item 1 (APU capacity figure was total system RAM): resolved. classify_accelerator_memory (apps/rocm/src/main.rs:2937) now returns AcceleratorMemory::Unknown for an APU instead of fabricating a figure. Mutating that arm back to a total-RAM-derived UnifiedSystemMemory turns classify_accelerator_memory_reports_unknown_not_total_ram_for_an_apu red, so the regression is pinned.
• Item 2 (the APU test could not fail for the defect it named): partially resolved. The classifier is now split into a pure function with five new unit tests in apps/rocm, which is exactly what was asked for. But the rocm-core test that carried the bad name was left untouched, and the remediation turned it into a test of a branch production can no longer reach — see blocking 2.
• Item 3 (e2e scenario asserted degraded where the platform gate returns blocked): resolved. tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1600 now declares "preferred_engines": ["lemonade"], which no lane excludes, so the Windows lane can no longer reach the wrong assertion.
• The prior round's non-blocking observations were not re-checked item by item in this round; only the three items the change request itself raised were tracked above.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/model_readiness.rs:483-499 — the "could not measure your GPU" remediation is wrong for the machine class this change is about.
On an APU, amd-smi runs fine and returns a figure; classify_accelerator_memory deliberately discards it and returns Unknown because the carve-out is the wrong pool. That lands in the (Some(required), None) arm, which tells the user "the GPU memory on this host could not be read", then hands them a plan of "let the CLI read this host's GPU memory, then ask again", the command amd-smi metric --json, and the note "the reading comes from amd-smi; if it is missing or failing, the ROCm install is what needs attention first". For a Strix Halo user none of that is true: amd-smi is not missing, the ROCm install needs no attention, and re-running the command returns the identical answer forever. This is a user-facing remediation that cannot work, on a platform this project treats as first class, and it blocks because the whole point of the command is that its answer is actionable. AcceleratorMemory::Unknown currently conflates "no telemetry at all" with "telemetry exists but names the wrong pool", so assess cannot tell them apart. Fix: carry the distinction — a second unknown variant, or a fourth UndeterminedReason — and give the APU case its own text saying plainly that this host has no dedicated VRAM and the CLI cannot yet read the pool its engine allocates from, with no command to run for it today. Pin both branches with a test.

2. crates/rocm-core/src/model_readiness.rs:24-25, 82-92, 412-418, 959-982 — UnifiedSystemMemory is unreachable, but the module presents it as the design.
Nothing in the workspace constructs AcceleratorMemory::UnifiedSystemMemory any more: the only production HostFacts site is apps/rocm/src/main.rs:2857, whose classifier now returns Unknown for every APU. Three surfaces still say otherwise. The module doc lists "The memory an APU reports is not the memory its engine allocates from" as the third of "three distinctions [that] carry the design", pointing at the variant. The evidence branch at :412 formats a line ("its engine allocates from {} of GTT-backed system memory, which is the figure compared below") that no user can ever see. And the test an_apu_is_judged_on_the_memory_its_engine_allocates_from at :959 hand-builds the variant and asserts Ready plus that evidence string, under a doc comment saying "the pool the engine actually uses is what gets compared" — which is the opposite of what ships. Mutation confirms the test only pins the measured_gib arm that serves the dead variant; deleting the whole APU classification in apps/rocm does not touch it. This blocks on two counts: it is dead code with live-looking documentation, and a reader arriving at this module will conclude APU hosts are judged on a GTT figure, which is the confusion the round-3 finding already cost a round to unpick and which will recur. Fix: delete the variant, the evidence branch, the doc bullet and that test, and say in the module doc that the APU pool is not readable today so those hosts report Unknown; if the variant is kept for a near-term reader, the doc and the test name must state that no production path constructs it.

3. apps/rocm/src/main.rs:2993-2998 — the platform-gate wiring has no test on any platform.
The PR body raises this to a design guarantee: "An engine ruled out by the platform blocks the model. It is not silently swapped for another engine." The code that delivers it is the unsupported_here closure in assess_model_for_host, and deleting it outright — making unsupported_here unconditionally None — leaves every test in both crates green. The rocm-core test an_engine_the_platform_rules_out_blocks_a_model_that_would_otherwise_fit (crates/rocm-core/src/model_readiness.rs:1010) injects unsupported_here: Some(...) as a literal, so it proves rocm-core's handling of the flag and never touches the code that sets it; and engine_ruled_out_by_platform itself short-circuits on runtime_is_windows(), so gutting it to always-false also survives on Linux. The PR's own limitation note ("the native-Windows engine gate is covered by a unit test that injects the condition") reads as coverage of this and is the disclosure being measured here — it is inaccurate in the direction that makes the change look safer. Fix, platform-independently: extract the closure body into a pure helper taking the selection and a ruled-out boolean, and assert both branches — that a ruled-out engine yields a populated unsupported_here naming that engine, and that an allowed one yields None.

Non-blocking

• crates/rocm-core/src/model_readiness.rs:644-647, 672, 697 — of the four verdict labels only BLOCKED survives a full-string mutation; READY, DEGRADED and UNDETERMINED are unpinned, as are the " - " evidence prefix and the " verify after fix: " prefix, despite the renderer's doc calling those prefixes the contract that stops a second report shape drifting.
• crates/rocm-core/src/model_readiness.rs:217-239, 597-620 — the declared-alternatives-first ordering, the cap of three fallbacks, and the Ready-before-Degraded two-pass all survive mutation untouched; only the fits filter the PR body highlights is actually pinned.
• apps/rocm/src/main.rs:2776-2791 — the --model plus --distro refusal is disclosed as untestable, but inspected_remotely is a pure function of the examination; extracting it into a named helper makes the guard unit-testable with a constructed Examination and no WSL host at all.
• docs/testing.md (new "Model Fit Preflight" section) — "The e2e suite ... exercises all four verdicts" now holds only on a lane with a discrete GPU: after the APU change an APU lane reports no measurement, so the ready half of diagnose-22 and the degraded half of diagnose-25 both early-return there.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1529-1547 — workspace_root and xtask_command are copy-pasted from artifact_steps.rs rather than hoisted into the shared e2e module the way the other cross-file helpers are.

[siloteemu]

🔴 Automated review · pr-review-watcher · b1bad70

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.

Three items block. The full detail is in the review comment; the essentials are below.

1. The APU capacity figure is total system RAM, not the pool an engine can allocate from

apps/rocm/src/main.rs:2938, surfaced at crates/rocm-core/src/model_readiness.rs:412. On the unified-memory path host_accelerator_memory returns UnifiedSystemMemory(detect_system_ram_gib()), and detect_system_ram_gib reads MemTotal — total installed RAM, not free RAM and not the GTT aperture. model_readiness.rs:412 then states that number to the user as fact: "its engine allocates from {X} of GTT-backed system memory, which is the figure compared below".

The repository already knows these are different numbers and already measures the real one: xtask/src/workflow_contract.rs enforces that every APU-class preflight block runs rocm-smi --showmeminfo vram gtt, and its own fixtures pair a small carve-out with a much smaller GTT pool on the same host class this code would report at full installed capacity. The error direction is the dangerous one — a false READY for a model that will not fit, which is exactly the failed multi-gigabyte download this command exists to prevent.

Fix: read the GTT aperture (the same figure the preflight contract already requires) and compare against that; if it cannot be read, return Unknown so the verdict becomes Undetermined rather than an optimistic Ready. At minimum, stop asserting the number as the engine's allocation pool in user-facing evidence.

2. an_apu_is_judged_on_the_memory_its_engine_allocates_from cannot fail for the defect its name claims to catch

crates/rocm-core/src/model_readiness.rs:960. The test hand-constructs AcceleratorMemory::UnifiedSystemMemory(128.0) and asserts the verdict and evidence string. The judgement it names — deciding that this host is an APU and choosing which figure represents its memory — lives in host_accelerator_memory in a different crate, which rocm-core cannot call, so the classifier is structurally unreachable from this test. host_accelerator_memory has no test anywhere, and reverting it to unconditionally return Dedicated(total_gib) — deleting the whole APU behaviour — leaves every test green. That also refutes the description's claim that "each invariant test was checked by mutation: the production code was broken deliberately and the test confirmed to go red"; it does not hold for this one.

Fix: split the pure part of host_accelerator_memory so it takes the parsed VRAM rows and the gfx target as arguments, and test that — asserting both that an APU target yields the unified branch and that a discrete target does not.

3. A new scenario asserts degraded where this PR's own gate returns blocked

tests/e2e-cucumber/tests/e2e/diagnose_steps.rs around line 490, with tests/e2e-cucumber/features/diagnose.feature:362. The synthetic recipe declares "preferred_engines": ["vllm"]. engine_ruled_out_by_platform rules vLLM out on native Windows, and model_readiness.rs:440 returns Blocked on unsupported_here before the RAM-softening branch is reached. The step early-returns only when GPU memory could not be measured, so on a Windows host that measures one it asserts degraded against an actual blocked and fails. Unlike its siblings in the same file, none of the five new scenarios carries a platform or GPU tag, and the file header states these run on every lane.

Fix: declare a platform-neutral engine such as lemonade in the fixture — which preserves the coverage on every platform — rather than tagging the scenario away from Windows.

The non-blocking observations are in the review comment and none of them gate.

[siloteemu]

🔴 Automated review · pr-review-watcher · f0a8a68

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.

Summary

Adds rocm diagnose --model <ref>, which answers "will this model run here" from the curated recipe catalog and local host facts without fetching anything, reporting one of four verdicts. Outcome: Needs work — the round-3 blockers are genuinely fixed, but the fix for the APU one left behind an unreachable code path that the module still documents as live, and the new path now gives APU users remediation advice that cannot work. Verified: ran the targeted unit suites for the two touched crates (cargo test -p rocm-core model_readiness and filtered cargo test -p rocm --bin rocm runs) green on scratch copies, then mutation-tested each new test against the exact production line it names — the full test suite and the e2e suite were not run here; independently confirmed that no production code anywhere in the workspace constructs AcceleratorMemory::UnifiedSystemMemory, that HostFacts has exactly one production construction site, that the degraded e2e fixture now declares lemonade, that the catalog loader opens no socket (so the "no network call" claim holds), that the commit carries a DCO sign-off, and that nothing in the diff leaks internal references or contains prompt-injection text; the review worked from check counts success=26, failure=1, pending=1, with the single failure on a GPU hardware lane of a family that is intermittently red on trunk, so it is not attributed to this diff. Blocking: 3 · Non-blocking: 5.

Previous round

• Item 1 (APU capacity figure was total system RAM): resolved. classify_accelerator_memory (apps/rocm/src/main.rs:2937) now returns AcceleratorMemory::Unknown for an APU instead of fabricating a figure. Mutating that arm back to a total-RAM-derived UnifiedSystemMemory turns classify_accelerator_memory_reports_unknown_not_total_ram_for_an_apu red, so the regression is pinned.
• Item 2 (the APU test could not fail for the defect it named): partially resolved. The classifier is now split into a pure function with five new unit tests in apps/rocm, which is exactly what was asked for. But the rocm-core test that carried the bad name was left untouched, and the remediation turned it into a test of a branch production can no longer reach — see blocking 2.
• Item 3 (e2e scenario asserted degraded where the platform gate returns blocked): resolved. tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1600 now declares "preferred_engines": ["lemonade"], which no lane excludes, so the Windows lane can no longer reach the wrong assertion.
• The prior round's non-blocking observations were not re-checked item by item in this round; only the three items the change request itself raised were tracked above.

🚫 Blocking (must fix before merge)

1. crates/rocm-core/src/model_readiness.rs:483-499 — the "could not measure your GPU" remediation is wrong for the machine class this change is about.
On an APU, amd-smi runs fine and returns a figure; classify_accelerator_memory deliberately discards it and returns Unknown because the carve-out is the wrong pool. That lands in the (Some(required), None) arm, which tells the user "the GPU memory on this host could not be read", then hands them a plan of "let the CLI read this host's GPU memory, then ask again", the command amd-smi metric --json, and the note "the reading comes from amd-smi; if it is missing or failing, the ROCm install is what needs attention first". For a Strix Halo user none of that is true: amd-smi is not missing, the ROCm install needs no attention, and re-running the command returns the identical answer forever. This is a user-facing remediation that cannot work, on a platform this project treats as first class, and it blocks because the whole point of the command is that its answer is actionable. AcceleratorMemory::Unknown currently conflates "no telemetry at all" with "telemetry exists but names the wrong pool", so assess cannot tell them apart. Fix: carry the distinction — a second unknown variant, or a fourth UndeterminedReason — and give the APU case its own text saying plainly that this host has no dedicated VRAM and the CLI cannot yet read the pool its engine allocates from, with no command to run for it today. Pin both branches with a test.

2. crates/rocm-core/src/model_readiness.rs:24-25, 82-92, 412-418, 959-982 — UnifiedSystemMemory is unreachable, but the module presents it as the design.
Nothing in the workspace constructs AcceleratorMemory::UnifiedSystemMemory any more: the only production HostFacts site is apps/rocm/src/main.rs:2857, whose classifier now returns Unknown for every APU. Three surfaces still say otherwise. The module doc lists "The memory an APU reports is not the memory its engine allocates from" as the third of "three distinctions [that] carry the design", pointing at the variant. The evidence branch at :412 formats a line ("its engine allocates from {} of GTT-backed system memory, which is the figure compared below") that no user can ever see. And the test an_apu_is_judged_on_the_memory_its_engine_allocates_from at :959 hand-builds the variant and asserts Ready plus that evidence string, under a doc comment saying "the pool the engine actually uses is what gets compared" — which is the opposite of what ships. Mutation confirms the test only pins the measured_gib arm that serves the dead variant; deleting the whole APU classification in apps/rocm does not touch it. This blocks on two counts: it is dead code with live-looking documentation, and a reader arriving at this module will conclude APU hosts are judged on a GTT figure, which is the confusion the round-3 finding already cost a round to unpick and which will recur. Fix: delete the variant, the evidence branch, the doc bullet and that test, and say in the module doc that the APU pool is not readable today so those hosts report Unknown; if the variant is kept for a near-term reader, the doc and the test name must state that no production path constructs it.

3. apps/rocm/src/main.rs:2993-2998 — the platform-gate wiring has no test on any platform.
The PR body raises this to a design guarantee: "An engine ruled out by the platform blocks the model. It is not silently swapped for another engine." The code that delivers it is the unsupported_here closure in assess_model_for_host, and deleting it outright — making unsupported_here unconditionally None — leaves every test in both crates green. The rocm-core test an_engine_the_platform_rules_out_blocks_a_model_that_would_otherwise_fit (crates/rocm-core/src/model_readiness.rs:1010) injects unsupported_here: Some(...) as a literal, so it proves rocm-core's handling of the flag and never touches the code that sets it; and engine_ruled_out_by_platform itself short-circuits on runtime_is_windows(), so gutting it to always-false also survives on Linux. The PR's own limitation note ("the native-Windows engine gate is covered by a unit test that injects the condition") reads as coverage of this and is the disclosure being measured here — it is inaccurate in the direction that makes the change look safer. Fix, platform-independently: extract the closure body into a pure helper taking the selection and a ruled-out boolean, and assert both branches — that a ruled-out engine yields a populated unsupported_here naming that engine, and that an allowed one yields None.

Non-blocking

• crates/rocm-core/src/model_readiness.rs:644-647, 672, 697 — of the four verdict labels only BLOCKED survives a full-string mutation; READY, DEGRADED and UNDETERMINED are unpinned, as are the " - " evidence prefix and the " verify after fix: " prefix, despite the renderer's doc calling those prefixes the contract that stops a second report shape drifting.
• crates/rocm-core/src/model_readiness.rs:217-239, 597-620 — the declared-alternatives-first ordering, the cap of three fallbacks, and the Ready-before-Degraded two-pass all survive mutation untouched; only the fits filter the PR body highlights is actually pinned.
• apps/rocm/src/main.rs:2776-2791 — the --model plus --distro refusal is disclosed as untestable, but inspected_remotely is a pure function of the examination; extracting it into a named helper makes the guard unit-testable with a constructed Examination and no WSL host at all.
• docs/testing.md (new "Model Fit Preflight" section) — "The e2e suite ... exercises all four verdicts" now holds only on a lane with a discrete GPU: after the APU change an APU lane reports no measurement, so the ready half of diagnose-22 and the degraded half of diagnose-25 both early-return there.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1529-1547 — workspace_root and xtask_command are copy-pasted from artifact_steps.rs rather than hoisted into the shared e2e module the way the other cross-file helpers are.

[siloteemu]

🔴 Automated review · pr-review-watcher · f0a8a68

Requesting changes on three defects found at this head. The full round, with the evidence and the previous round's status, is in the review comment posted alongside this.

1. The "could not measure your GPU" remediation cannot work on the machine class this change is about. On an APU the telemetry tool runs fine and returns a figure; the classifier deliberately discards it because it names the wrong memory pool, and the result lands in the branch that tells the user the GPU memory could not be read and hands them a command to run, plus a note that the install is what needs attention. None of that is true there: the tool is present, the install is fine, and re-running returns the identical answer forever. The unknown state currently conflates "no telemetry at all" with "telemetry exists but names the wrong pool", so the assessment cannot tell them apart. Carry the distinction and give that case its own text, with both branches pinned by a test.

2. A memory variant is unreachable while three surfaces still present it as the design. No production path constructs it any more. The module doc still lists the distinction it represents as one of three that "carry the design"; an evidence branch formats a line no user can ever see; and a test hand-builds the variant and asserts a verdict under a doc comment saying the pool the engine actually uses is what gets compared, which is the opposite of what ships. Deleting the whole APU classification in the application does not touch that test. This is dead code with live-looking documentation, and it will lead the next reader to exactly the wrong conclusion — the same confusion the previous round already spent a round unpicking.

3. The platform gate that the description raises to a guarantee has no test on any platform. The description states that an engine ruled out by the platform blocks the model and is not silently swapped for another. Deleting the code that delivers that, so the ruled-out condition is never set, leaves every test in both crates green: the unit test that looks like coverage injects the condition as a literal and never reaches the code that sets it, and the setter short-circuits on a platform check, so gutting it survives elsewhere too. The description's own limitation note reads as coverage of this and is inaccurate in the direction that makes the change look safer. Extract the condition into a pure helper and assert both branches.

Item 3, and the residue carried over from the previous round's item 2, are test-coverage defects rather than behaviour defects, and they gate for the reason a weak test always does: nobody reopens a landed change to strengthen one.

Addressed

[volen-silo]

Addressed all 3 blocking items from the automated review at f0a8a68:

1. APU remediation dead end — AcceleratorMemory gains a UnifiedMemoryUnreadable variant and UndeterminedReason a matching case. The APU path now says plainly there's no dedicated VRAM and no command that makes the real pool readable today, with fix: None — distinct from genuine Unknown (no telemetry at all), which keeps the real amd-smi remediation. Both branches pinned by tests (an_apu_with_no_readable_pool_is_undetermined_with_no_dead_end_command, no_telemetry_at_all_still_points_at_amd_smi), each mutation-tested.
2. Dead UnifiedSystemMemory variant — removed the variant, its evidence branch, the module-doc bullet, and the test that hand-built it.
3. Untested platform gate — extracted unsupported_here_for(engine, ruled_out) as a pure helper out of the engine_for closure, unit-tested on both the ruled-out and allowed branches directly, mutation-tested to confirm the ruled-out test catches a gutted implementation.

The two active reviews from this bot (the CHANGES_REQUESTED one, and this COMMENTED one) are being closed out — dismissing the former; this comment stands in for the latter since GitHub's API refuses to dismiss a COMMENTED-state review.

On the 5 non-blocking items also raised here: reporting rather than fixing, since none are behavior defects or unfailable tests, only coverage/completeness observations —

1. Only the BLOCKED label is pinned against full-string mutation; READY/DEGRADED/UNDETERMINED and evidence/verify-line prefixes aren't. Left alone — real gap, but a coverage-breadth one, not a defect.
2. Declared-alternatives-first ordering, the cap-of-three fallback, and Ready-before-Degraded ordering all survive mutation untouched; only the fits filter is actually pinned. Same category as above.
3. inspected_remotely (main.rs, --model + --distro refusal) could be extracted as a pure function for direct unit testing instead of being disclosed as untestable. Left alone; the disclosure is accurate today.
4. docs/testing.md's "exercises all four verdicts" claim no longer fully holds on APU lanes after this fix. Checked directly against diagnose.feature: the Gherkin already has the right conditional branches (a machine with enough measured GPU memory... / a machine that could not measure its GPU...), so this is a doc-completeness gap, not a regression from this fix.
5. tests/e2e-cucumber/tests/e2e/diagnose_steps.rs has copy-pasted workspace_root/xtask_command helpers instead of a shared e2e module. Left alone; pre-existing pattern, not introduced by this PR.

Also fixed, from juhovainio's review: the PR description's stale test count (now 9 rocm-core / 13 apps/rocm / 5 e2e, replied inline with the count breakdown).

[siloteemu]

🔴 Automated review · pr-review-watcher · bade1c9

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.

Summary

Adds rocm diagnose --model <ref>, which answers READY/DEGRADED/BLOCKED/UNDETERMINED for "will this model run here" from the curated recipe catalog and local host facts without fetching anything. Outcome: Needs work — the three previous blockers are genuinely discharged, but the new APU-specific undetermined reason introduced to fix one of them was added to the core and its unit test and not to the end-to-end assertion that enumerates the acceptable reasons, so the scenarios reject the very value this change now produces on APU hardware. Verified: ran the two touched crates' unit suites on a scratch copy (9 green in the core readiness module, 961 green in the application binary) and put ten mutations through them — collapsing the new APU arm turns its named test red, forcing the platform gate always-on turns two tests red, and the pure memory classifier, the zero-telemetry-row drop and both branches of the WSL GPU-presence signal are each pinned by a test that fails when its line is reverted; independently confirmed that the previously dead unified-memory variant is gone from the whole repository, that engine selection is genuinely shared with serve behind a non-vacuity check rather than re-derived, that the shared alternatives helper preserves the pre-existing ordering and cap exactly, that the stated counts of 9 and 13 unit tests are exact, that a sign-off is present with no generated footer, and that a leak and prompt-injection scan over the whole authored diff is clean; the full suite, the end-to-end suite and an all-targets build were not run in this round. Checks at review time: success=25, failure=2, pending=1 — I judge these failures plausibly this PR's own, for the reason in the blocking item below, though mapping them to particular jobs is inference I cannot confirm. Blocking: 1 · Non-blocking: 5.

🚫 Blocking (must fix before merge)

tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1326-1335 — the scenarios assert the one undetermined reason this change stopped producing on APU hosts, so on the project's own APU lanes three scenarios fail on a correct verdict.
The "undetermined" arm of assert_unmeasured_machine_is_told_why is an assert_eq! against the literal "accelerator_memory_unknown". This head introduces a second reason — the APU case where telemetry exists but names the wrong pool — and routes every single-GPU host whose gfx target is in the APU family (gfx115x) to it: the classifier at apps/rocm/src/main.rs:2972 returns the unified-unreadable variant, which reports no measured figure, so the step's measured_gpu_gib guard does not return early and control reaches that arm with "unified_memory_unreadable". The step is used by diagnose-21 (tests/e2e-cucumber/features/diagnose.feature:315), diagnose-22 (:330) and diagnose-25 (:367), none of which carries a platform or hardware tag — unlike their tagged siblings in the same file — and the repository runs three dedicated gfx115x APU lanes on the full suite. So the change's own correct output is what makes its own scenarios red, on exactly the hardware class the change was reworked to serve; that is also the most plausible reading of the two red checks. The same omission has a second, quieter face: the step comment two lines above enumerates only two machines ("no GPU at all" / "a GPU whose memory it could not read"), and the fixture doc at tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1143-1145 still claims any machine that can measure a GPU at all can serve the smallest recipe, "so the ready half of diagnose-21 holds on every GPU lane" — a prose guarantee this change made false. Fix: accept both reasons in that arm and assert something reason-specific for each — for the unified case, that the evidence names the absence of dedicated VRAM and that no fix plan is attached, mirroring the unit test at crates/rocm-core/src/model_readiness.rs:988; then correct the step comment and the fixture doc to name the third machine class. Running the adversarial pass over that fix: simply widening the assertion to a set membership would be the naive version and would NOT hold — it would let a future regression that collapses the two reasons back together pass silently, which is precisely the distinction this round of work exists to create, so the per-reason assertion is load-bearing rather than decoration. Separately, this is a fix applied to one of two sites: the new reason reached the core, the classifier and the unit tests but not the scenario layer, so it is worth grepping for any other place that enumerates undetermined reasons before landing.

Non-blocking

• apps/rocm/src/main.rs:2999-3002 — the previous round's request was met exactly (a pure helper with both branches asserted), but the one line that computes the helper's argument is still unpinned: replacing it with an unconditional "not ruled out" leaves all 961 tests green, because the predicate is compile-time false off Windows. The over-blocking direction IS caught (forcing it always-on turns two tests red) and the description's limitation note is now accurate, so this does not gate; passing the ruled-out boolean into assess_model_for_host as a parameter would make the wiring itself exercisable on the lane that actually runs.
• crates/rocm-core/src/model_readiness.rs:246-279, 623-665 — the two-pass selection calls curated_alternatives once per pass, and each call decides declared-versus-fallback independently, so a recipe whose declared alternatives all land merely degraded has them silently replaced by unrelated catalog fallbacks that happen to be ready, contradicting the declared-first guarantee documented at :229-241. Not reachable with today's catalog; the sweep test pairs every host with ample RAM, so it cannot detect it either.
• crates/rocm-core/src/model_readiness.rs:644-647, 672, 697 and :217-239, 597-620 — carried over unaddressed from the previous round and re-confirmed by mutation at this head: of the four verdict words only BLOCKED is pinned, the evidence and verify-after-fix prefixes the renderer's own doc calls a contract are unpinned, and the fallback cap, the declared-first ordering and the ready-before-degraded two-pass all survive being broken.
• apps/rocm/src/main.rs:2776-2792 — the --model plus --distro refusal still has no coverage of any kind; the remoteness test remains an inline expression rather than the named helper that would make the guard unit-testable against a constructed examination with no live distribution.
• tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1529-1547 — the workspace-root and xtask-command helpers are copied in rather than hoisted, making this the third copy of an existing duplication; and docs/testing.md's new section's claim that the suite exercises all four verdicts holds only at the level of the four words, not of the two distinct undetermined reasons production can now return.

[siloteemu]

🔴 Automated review · pr-review-watcher · bade1c9

Requesting changes at this head. One blocking finding; the full round, including the five non-blocking items and what was and was not verified, is in the review comment posted alongside this.

🚫 Blocking (must fix before merge)

tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1326-1335 — the scenarios assert the one undetermined reason this change stopped producing on APU hosts, so on the project's own APU lanes three scenarios fail on a correct verdict.
The "undetermined" arm of assert_unmeasured_machine_is_told_why is an assert_eq! against the literal "accelerator_memory_unknown". This head introduces a second reason — the APU case where telemetry exists but names the wrong pool — and routes every single-GPU host whose gfx target is in the APU family (gfx115x) to it: the classifier at apps/rocm/src/main.rs:2972 returns the unified-unreadable variant, which reports no measured figure, so the step's measured_gpu_gib guard does not return early and control reaches that arm with "unified_memory_unreadable". The step is used by diagnose-21 (tests/e2e-cucumber/features/diagnose.feature:315), diagnose-22 (:330) and diagnose-25 (:367), none of which carries a platform or hardware tag — unlike their tagged siblings in the same file — and the repository runs three dedicated gfx115x APU lanes on the full suite. So the change's own correct output is what makes its own scenarios red, on exactly the hardware class the change was reworked to serve; that is also the most plausible reading of the two red checks. The same omission has a second, quieter face: the step comment two lines above enumerates only two machines ("no GPU at all" / "a GPU whose memory it could not read"), and the fixture doc at tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:1143-1145 still claims any machine that can measure a GPU at all can serve the smallest recipe, "so the ready half of diagnose-21 holds on every GPU lane" — a prose guarantee this change made false. Fix: accept both reasons in that arm and assert something reason-specific for each — for the unified case, that the evidence names the absence of dedicated VRAM and that no fix plan is attached, mirroring the unit test at crates/rocm-core/src/model_readiness.rs:988; then correct the step comment and the fixture doc to name the third machine class. Running the adversarial pass over that fix: simply widening the assertion to a set membership would be the naive version and would NOT hold — it would let a future regression that collapses the two reasons back together pass silently, which is precisely the distinction this round of work exists to create, so the per-reason assertion is load-bearing rather than decoration. Separately, this is a fix applied to one of two sites: the new reason reached the core, the classifier and the unit tests but not the scenario layer, so it is worth grepping for any other place that enumerates undetermined reasons before landing.

Checks at review time: success=25, failure=2, pending=1. The two red checks are judged plausibly this change's own, for the reason above; mapping them to particular jobs is inference and is not confirmed here.

The three blocking items from our previous round are genuinely discharged -- this request is about the new gap the latest remediation opened, not a re-statement of them.

placeholder

[volen-silo]

Addressed the blocking finding at bade1c9 (now b6b874e).

assert_unmeasured_machine_is_told_why's "undetermined" arm no longer does a single assert_eq! against "accelerator_memory_unknown". It now matches on the reason and asserts something reason-specific for each:

• accelerator_memory_unknown: evidence must contain "could not be read", and fix must be present (the real amd-smi metric --json remediation).
• unified_memory_unreadable: evidence must contain "no dedicated VRAM", and fix must be null (no dead-end command offered).
• any other reason string: panics, rather than passing by coincidence.

This is deliberately not the naive set-membership widening the review called out — a mutation that collapses the two reasons back together (correct reason string, but evidence/fix reverted to the other shape) fails this assertion. I verified that directly: copied the reason-specific match arm into a standalone scratch crate (outside the repo, discarded after use) with plain #[test]s under a normal harness, since the real e2e test target has harness = false and can't be driven by cargo test directly. Five cases: the two correct shapes pass; the two "collapsed" mutations panic on the expected message; and a copy of the old naive membership-only check was run against the same collapsed-APU mutation to confirm it does not fail, showing concretely what the reason-specific version catches that the naive one would not.

Also fixed the two stale-prose spots: the step's own two-machine comment and the SMALLEST_MODEL_REF fixture doc (:1143-1145) both now name the APU case as a third class alongside "no GPU" and "no telemetry at all".

Grepped the repo for every other site enumerating UndeterminedReason/AcceleratorMemory (UnifiedMemoryUnreadable, AcceleratorMemoryUnknown, unified_memory_unreadable, accelerator_memory_unknown) outside target/. Found only the three already known: crates/rocm-core/src/model_readiness.rs (construction + unit tests), the classifier in apps/rocm/src/main.rs, and this one e2e step. No fourth site.

Verification at the new head:

• cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace --all-targets: all clean, gated on real exit codes.
• The e2e test target is excluded from --all-targets (test = false), so I additionally ran cargo clippy -p e2e-cucumber --test e2e -- -D warnings (clean) and actually executed the three untagged scenarios (diagnose-21/22/25) via cargo xtask e2e -- -n ... against the local mock/no-GPU lane — all pass. That lane only exercises the blocked branch (no GPU here to reach either undetermined reason), so it's a real integration smoke test of the edit, not proof of the APU branch itself; the APU branch's correctness is what the standalone mutation test above establishes.
• Platform check: classify_accelerator_memory, its caller host_accelerator_memory/gpu_vram_usage, and the edited step logic are all platform-uniform — no #[cfg(windows)]/#[cfg(unix)] anywhere in the touched code, they operate on already-collected JSON/struct data — so there's no reason to expect Windows-specific behavior here, though I couldn't run the Windows lane itself locally.

Left alone: the pre-existing SMALLEST_MODEL_REF doc's reference to "diagnose-21" (that constant is actually consumed by the diagnose-22 Given step) — a pre-existing numbering quirk, not something this review flagged, so out of scope for this fix.

[siloteemu]

🔴 Automated review · pr-review-watcher · b6b874e

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.

Summary

Adds rocm diagnose --model <ref>, which answers READY/DEGRADED/BLOCKED/UNDETERMINED for "will this model run here" from the curated catalog and local host facts without fetching anything. Outcome: Needs work — the previous round's blocker is genuinely and well discharged, but the same defect shape survives one arm over: the step that judges an unmeasured-GPU host had its undetermined branch taught the new reasons and its blocked branch left asserting a single hard-coded evidence string, which the diff's own new engine-gate ordering can no longer produce on a native-Windows APU lane. Verified: read the whole authored diff and commit object, confirmed by reading source that the engine gate returns Blocked before the memory gate with a different evidence string, that glm5 (diagnose-21's subject) resolves to vLLM through select_serve_engine with no fallback, that the platform gate rules vLLM out on Windows, that an APU reports no measured figure so the step cannot early-return, and that no expectations entry exempts any diagnose scenario; separately confirmed the --distro refusal is sound because every success path of the host-side probe sets the remoteness marker, confirmed the declared-alternative displacement is reachable against the shipped catalog (Qwen/Qwen3.6-27B → qwen3-32b-fp8) which is unchanged by this PR, confirmed the stated counts of 9 / 13 / 5 are exact, and confirmed a leak scan and prompt-injection scan over the diff, commit message and docs are clean. The core module's 9 unit tests were run green in a scratch copy; the full suite, the end-to-end suite and an all-targets build were not checked in this round, and I did not modify the tree under review. Checks at review time: 26 success, 1 pending, 1 failure — the single failure is plausibly this PR's own for the reason below; attributing it to a particular named job is inference I cannot confirm. Blocking: 1 · Non-blocking: 5.

Prior round

• Blocking — "the undetermined arm of assert_unmeasured_machine_is_told_why is an assert_eq! against the literal accelerator_memory_unknown" and would reject the new APU reason: DISCHARGED, and discharged well. Each reason now has its own arm asserting reason-specific content (the no-telemetry case must say the memory "could not be read" and must carry a fix; the APU case must name "no dedicated VRAM" and must carry none), with a panic! catch-all instead of set membership — which is exactly the per-reason form the round asked for rather than the naive widening it warned against. The step comment now names three machine classes and the fixture doc no longer carries the false "any machine that can measure a GPU at all can serve the smallest recipe" guarantee.
• Non-blocking — the platform-gate argument computation is unpinned because "the predicate is compile-time false off Windows": REFUTED; our claim was wrong. The predicate is a runtime check, a Windows job runs the unit tests, and a pre-existing test already exercises both branches contingent on the host OS. A narrower gap is still live: the closure in assess_model_for_host that wires the gate into the engine choice, and the argument computation feeding the memory classifier, are each covered only by their pure halves plus end-to-end runs, not by a targeted unit test — which is weaker than "pure, unit-tested function separated from the subprocess call" implies.
• Non-blocking — the two-pass alternative selection lets declared alternatives be displaced: STILL LIVE, and our qualifier "not reachable with today's catalog" was wrong. It is reachable against the shipped catalog, which this PR does not touch and which was identical at the previous round. See the non-blocking item below.
• Non-blocking — "of the four verdict words only BLOCKED is pinned": REFUTED; our claim was wrong. All four are pinned by direct equality assertions that would fail if production emitted a different word, and the degraded pin is non-vacuous (ample GPU, deficient RAM, so only the RAM branch can produce it). The fallback cap and the renderer's evidence/verify-prefix contract do remain unpinned.
• Non-blocking — the --model plus --distro refusal has no coverage: STILL LIVE, and accurately disclosed in the description. The guard's logic is sound (verified: every success path of the host-side distro probe marks the examination non-local).
• Non-blocking — copied-in workspace-root/xtask helpers, and the docs' four-verdict claim: helpers STILL LIVE (a third copy alongside two existing ones); the four-verdict concern is discharged — the premise was stale, there are four undetermined reasons at this head and the suite discriminates all four. A different, new doc inaccuracy replaced it (below).

🚫 Blocking (must fix before merge)

tests/e2e-cucumber/tests/e2e/diagnose_steps.rs (the "blocked" arm of assert_unmeasured_machine_is_told_why, ~:1324-1336) — the fix went into one of two arms, so diagnose-21 fails on a native-Windows APU lane on a correct verdict.

The arm hard-asserts evidence.contains("no GPU is visible to ROCm"), on the premise that the only way a host with no measurement can reach Blocked is "no GPU visible". This diff makes that premise false. In crates/rocm-core/src/model_readiness.rs the engine gate is deliberately placed before the memory gate and returns Blocked early with its own evidence, "<engine> has no adapter on native Windows; serve it from WSL or Linux". Diagnose-21's subject, glm5, declares only vLLM and has no alternate engine recipe, so with no explicit or configured engine it resolves to vLLM; the platform gate rules vLLM out whenever the runtime is Windows. On an APU the memory classifier returns the unified-unreadable state, which reports no figure, so the step's opening early-return does not fire and control reaches this arm with the engine evidence. The sibling assertion (no figure reported) still passes; the evidence-string assertion fails. The scenario is untagged — consistent with its host-agnostic siblings, so that is not itself the defect — and no expectations entry exempts any diagnose scenario, so this is an unexpected failure on a lane the project runs. Diagnose-22 and diagnose-25 are unaffected: their subjects are Lemonade recipes the gate does not rule out. This is the strongest available explanation for the one red check, though mapping it to a specific job is inference I cannot confirm.

Fix: give the "blocked" arm the same per-cause shape the "undetermined" arm now has — discriminate the two causes and assert something cause-specific for each (the GPU-visibility refusal must name the absent GPU and carry the rocm diagnose fix; the engine refusal must name the engine it ruled out and offer the other-platform remedy), keeping the no-figure assertion common to both, and correct the scenario's own comment, which still enumerates only two halves. Running the adversarial pass over that remedy: the tempting shortcut — widening to contains(A) || contains(B) — would NOT hold. It would let a future change that collapses the engine refusal into the GPU refusal, or that emits either message for the wrong cause, pass silently, which is precisely the distinction the engine gate's own ordering comment exists to create. Discriminating first and asserting per cause is load-bearing, not decoration. Worth grepping for any other closed enumeration of verdict causes before landing, since this is the second time a new cause reached production and the unit tests but not the scenario layer.

Non-blocking

• crates/rocm-core/src/model_readiness.rs — the outer two-pass caller only tries its degraded-tolerant pass when the strict pass returns nothing, but the strict pass has its own catalog-wide fallback, so a recipe whose declared alternatives are merely degraded has them replaced by unrelated ready models; reachable today via Qwen/Qwen3.6-27B → qwen3-32b-fp8 on a GPU-sufficient, RAM-starved host. The outer comment's "ready first" and the helper's "declared first" now read in tension and neither names the precedence between them; the sweep test pins every host at 512 GiB RAM, so the degraded-tolerant pass is never exercised at all. One constrained-RAM host in that sweep is the cheap thing that would both pin the pass and have caught our own wrong "not reachable" claim last round — this is the confusion most likely to recur.
• apps/rocm/src/main.rs (~:2776-2792) — the --model/--distro refusal is still untested at any level. The guard is also expressed on a derived property of the probe result rather than on the flag the user typed, so it would lapse silently if any future probe path returned an examination without those facts; keying it on the flag, or extracting the remoteness test into a named helper, makes it both safer and unit-testable.
• docs/testing.md (new Model Fit Preflight section) — states that the not-curated verdict needs a synthetic signed catalog to trigger deterministically. It does not: that scenario names a fake ref against the real bundled catalog, and only the degraded scenario builds a signed fixture. A disclosed mechanism described wrongly misleads the next maintainer more than no description would.
• The contributor rules ask that docs/manual-testing.md be updated in the same change when a command's flags or observable behavior change; it is untouched, and this PR adds a user-visible flag whose own disclosed gaps (no GPU host, no Windows run) are exactly what a manual checklist entry would cover. Also still open from before: the fallback cap and the renderer's evidence/verify-prefix contract are unpinned, and the workspace-root/xtask helpers are copied in for a third time rather than hoisted.
• Positive signals worth keeping: the alternatives sweep opens with an explicit non-vacuity assertion that documents the stub run it once passed against; the platform-gate message was split into a pure function specifically so both branches are reachable from either OS's runner; and the engine-consistency test contains a self-check that fails unless at least one host actually exercises an override, so it cannot pass by comparing two no-ops.

[siloteemu]

🔴 Automated review · pr-review-watcher · b6b874e

Requesting changes on one item. The previous round's blocking item is genuinely and well discharged — the per-reason arms are exactly the form that round asked for rather than the naive widening it warned against. The full round, including four claims of ours that this round examined (two of which were wrong, and are corrected there), is in the comment posted alongside this.

The fix went into one of two arms, so the same defect shape survives one branch over (tests/e2e-cucumber/tests/e2e/diagnose_steps.rs, the blocked arm of the unmeasured-machine step, around lines 1324-1336).

The blocked arm hard-asserts that the evidence contains "no GPU is visible to ROCm", on the premise that the only way a host with no measurement reaches a blocked verdict is an absent GPU. This diff makes that premise false. In the readiness module the engine gate is deliberately placed before the memory gate and returns blocked early with its own, different evidence, naming an engine that has no adapter on native Windows and suggesting WSL or Linux instead.

Traced end to end: the scenario's subject declares only one engine and has no alternate recipe, so with no explicit or configured engine it resolves to that engine; the platform gate rules it out whenever the runtime is Windows; on an integrated GPU the memory classifier returns the unified-unreadable state, which reports no figure, so the step's opening early-return does not fire and control reaches this arm carrying the engine evidence. The sibling assertion still passes; the evidence-string assertion fails. The scenario is untagged, consistent with its host-agnostic siblings — that is not itself the defect — and no expectations entry exempts it, so this is an unexpected failure on a lane the project runs. Two neighbouring scenarios are unaffected, because their subjects use a recipe the gate does not rule out.

This is also the strongest available explanation for the one red check on this head. Mapping it to a particular named job is inference and cannot be confirmed from here.

Why it gates. It is the hard-blocker shape twice over: an assertion that now fails for a correct verdict, and a closed enumeration of causes that a production change has outgrown without anything forcing it to be revisited. It is the second time a new cause has reached production and the unit tests but not the scenario layer.

Suggested fix. Give the blocked arm the same per-cause shape the undetermined arm now has: discriminate the two causes and assert something cause-specific for each — the GPU-visibility refusal must name the absent GPU and carry its fix; the engine refusal must name the engine it ruled out and offer the other-platform remedy — keeping the no-figure assertion common to both. Correct the scenario's own comment, which still enumerates only two halves.

We ran the same adversarial pass over that remedy as over the finding, and it changed the answer: the tempting shortcut of widening to an either-string match would not hold. It would let a future change that collapses one refusal into the other, or that emits either message for the wrong cause, pass silently — which is precisely the distinction the engine gate's own ordering comment exists to create. Discriminating first and asserting per cause is load-bearing, not decoration.

Worth grepping for any other closed enumeration of verdict causes before landing.

[volen-silo]

Fixed at c63f0fd (amended into the one existing commit, force-pushed).

What changed (tests/e2e-cucumber/tests/e2e/diagnose_steps.rs, assert_unmeasured_machine_is_told_why): the blocked arm now discriminates its two causes instead of hard-asserting one evidence string. The discriminator is fix.summary, which is written independently at the two call sites in assess_model_for_host/assess_model_readiness (the engine-platform gate and the no-GPU gate), so a future collapse of one cause's evidence into the other's text still gets caught here rather than passing silently:

• "serve this model from a platform that has the engine" → asserts the evidence names the specific ruled-out engine ("{engine} has no adapter") and offers the other-platform remedy ("WSL or Linux").
• "make a GPU visible to ROCm, then ask again" → asserts the evidence names the absent GPU and that the fix still carries its rocm diagnose command.
• anything else → panic!, same closed-enumeration shape as the already-fixed undetermined arm.

The available_gpu_memory_gib null check stays common to both. The scenario's own comment above the match (in both diagnose_steps.rs and diagnose.feature) is corrected from two/three causes to the four it now has to be: no GPU, engine ruled out by the platform gate, memory unreadable, and the APU case.

I did not widen to an either-string match. I wrote a standalone throwaway harness (outside the repo, since the e2e target is harness = false) that transcribes the real match logic against hand-built JSON and ran it through catch_unwind:

• the two real payloads (engine-ruled-out, no-GPU) pass;
• collapsing either cause's evidence into the other's text, while leaving fix.summary for the original cause, makes the discriminated check panic in both directions;
• the rejected either-string remedy (contains(A) || contains(B)) does not catch that same collapse — I ran it side by side on the same corrupted input to confirm the review's rejection was correct;
• an unrecognized fix.summary panics (future third cause);
• each individual assertion (remedy text, fix-carrying, the common no-figure check) independently fails when mutated on its own.

Other enumeration sites: grepped the whole repo for ModelVerdict::, undetermined_reason/UndeterminedReason, unsupported_here/the two evidence strings, closed _ => panic!/other => panic! patterns in the e2e step files, expectations.toml, and the human-readable (non-JSON) report renderer in model_readiness.rs. Nothing else closes over these causes in a way that can silently miss a new one — the renderer iterates evidence/fix generically (fail-open by construction), and the only other wildcard arm found (route_when_no_match's _ => "rocm-core" in diagnose.rs) is pre-existing, unrelated to verdict causes, and fail-open by design, not the same defect shape.

Does this explain the red E2E tests (Strix Halo, Windows) check? Yes, confirmed, not inferred: that job's own log shows Step panicked. Captured output: the only refusal a machine with no measurement may make is about the GPU, and this one does not say so: { ... "engine": "vllm", "evidence": [..., "vllm has no adapter on native Windows; serve it from WSL or Linux"], "fix": { "summary": "serve this model from a platform that has the engine", ... } } — the exact pre-fix panic text, against the exact engine-platform-gate payload this round describes, and it's the only unexpected failure in that run (3 others were expected xfail). Replaying that captured JSON through the new discriminated check by hand passes.

Verified: cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, and cargo test --workspace --all-targets all exit 0 with zero failures (gated on exit status, not piped through an unconditional echo). The e2e integration target itself is harness = false and excluded from cargo test/--all-targets by design (only runs via cargo xtask e2e), so I additionally ran cargo clippy -p e2e-cucumber --test e2e -- -D warnings directly to typecheck/lint the edited file in its real target, and that's clean too.

Dismissing now that the fix is confirmed present at the new head.

Addressed at c63f0fd: the blocked arm now discriminates both causes (fix.summary) and asserts cause-specific content for each, verified by a mutation harness; the scenario comment is corrected; no other enumeration site found. See PR comment for details.

[juhovainio]

I reviewed this PR and found several issues worth fixing before merge, left inline below.

The most important one: on a multi-GPU host the VRAM figures are summed into a single number and compared against the model's minimum. Since rocm serve pins exactly one ordinal, that reports READY for models that will OOM on every individual card. That lands in the direction this PR's own description warns against, "a verdict that pretended otherwise would be wrong in the user's favour".

Two more that undercut stated goals: the rewritten /model remediation points at a command that cannot clear the condition that printed it, because every production caller discards the reading before the function sees it. And the new --model plus --distro refusal keys on whether the examination happened to be remote rather than on the flag, so some --distro runs fall through and answer for the local host under a heading naming another machine.

Plenty here is solid and I confirmed it rather than assuming: the curated_alternatives refactor is provably equivalent, the select_serve_engine call-site drift produces identical outcomes, examination_reports_amd_gpu keying on wsl.is_some() is a fix and not a regression, every Fix{} command exists with the flags given, and the exactly-fits boundary is handled correctly. I also chased the parse_gpu_vram_usage zero-row change and it does not regress any consumer.

Addressed

[juhovainio]

Re-reviewed after the fixes — all of the substantive issues from the last round are resolved (multi-GPU VRAM max-not-sum, the /model remediation dead end, the --distro+--model keying bug, the BLOCKED rounding contradiction, the disclosed UnifiedMemoryUnreadable variant). One of my original findings was actually wrong: model_registry_adapter_availability_note isn't dead code, it has live call sites — apologies for the noise there; the stale #[allow(dead_code)] on it is a harmless leftover worth a quick cleanup but not blocking. Nothing new turned up elsewhere in the diff. Looks safe to approve after CI.