Skip to content

ROCMAI-830: pin the fix-13 gate for a Lemonade Strix Halo host with a managed runtime - #835

Merged
volen-silo merged 1 commit into
mainfrom
rocmai-830-diagnose-hip-sdk
Nov 3, 2026
Merged

volen-silo merged 1 commit into
mainfrom
rocmai-830-diagnose-hip-sdk

Conversation

@volen-silo

@volen-silo volen-silo commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

On the v0.2.1 release, rocm diagnose on a native-Windows Strix Halo (gfx1151) host that serves through Lemonade ranked fix-13-hip-sdk-missing first (score 35), with and without a managed TheRock wheel runtime installed.

Root cause, and what this PR is

The checker was already fixed on main before this PR: #676 (c9e9a327, 2026-10-16, ROCMAI-546) added the needs_hip gate to check_13_hip_sdk_missing. release/v0.2.1 was cut from main on 2026-10-07 and #676 was not cherry-picked onto it, so the release binary keeps the old scoring; a build of current main on the same host reports no match.

So this PR changes no product code. It pins existing behaviour, which had no test for this host shape: the ROCMAI-546 regression test is a gfx90c laptop, where another entry outranks fix-13 anyway. It is the regression test the ROCMAI-830 ticket asks for, not a fix. Backporting #676 to the v0.2.1 release line is a separate decision.

Change

  • Unit test fix_13_does_not_fire_on_a_lemonade_strix_halo_host_with_a_managed_runtime in crates/rocm-core/src/diagnose.rs. It builds the host from that machine's examine output (gfx1151 APU, no system SDK, Adrenalin driver, MSVC runtime present) in four arms: no runtime, a managed runtime whose torch probe timed out, a managed HIP torch (each must leave fix-13 out of matched), and an ambient HIP torch as a positive control (fix-13 must still rank first).
  • Unit test fix_13_lists_weakly_for_ambient_hip_torch_beside_a_healthy_system_sdk: pins what production does for an ambient HIP-built PyTorch beside a healthy system SDK. check_13_hip_sdk_missing adds 25 for an ambient HIP torch whether or not the SDK is usable, so fix-13 is listed weakly, below the 50-point match threshold, while the same host with a managed runtime scores 0. That listing looks wrong (the SDK is fine); it is tracked as ROCMAI-837 and left unchanged here.
  • Scenario diagnose-48 (@id:diagnose-windows-hip-sdk-advice-follows-the-inspection, @requires-os:windows @requires-gpu) in tests/e2e-cucumber/features/diagnose.feature. Like diagnose-41, it compares diagnose --json with examine --json on the same machine. Its oracle mirrors check_13's scoring: fix-13 appears exactly when the inspection finds an AMD GPU and either a HIP-built llama.cpp with no usable system SDK or a non-managed HIP-built PyTorch. The earlier version expected fix-13 only when the SDK was unusable, which would have failed on a healthy host with ambient HIP torch.
  • docs/testing.md names both tests and states the same condition.

skills/rocm-doctor/reference.md is unchanged: the catalog's scoping did not change.

The tests fail when the gate is broken

Breaking the gate by forcing needs_hip on Windows (which removes the managed-runtime / Lemonade exemption):

Test Gate broken Gate intact
fix_13_does_not_fire_on_a_lemonade_strix_halo_host_with_a_managed_runtime fails passes
diagnose-48 (run on this Strix Halo Windows host) fails: fix-13-hip-sdk-missing present: true, inspection finds no HIP framework and no SDK passes

On this lane diagnose-48 only proves absence, because its expected value is always false here (no HIP-built framework, no SDK). It turns red on any regression that makes fix-13 score for a host with no HIP evidence, and on a different host shape (ambient HIP torch, or a HIP-built llama.cpp with an unusable SDK) it checks that fix-13 is present, so it cannot go red there without a gate regression either.

Verification

  • cargo nextest run -p rocm-core (876 passed), clippy for rocm-core and e2e-cucumber (including --test e2e), cargo fmt --check: pass.
  • diagnose-48 passed on the E2E tests (Strix Halo, Windows, current) lane for this PR (15m28s), and locally against a debug build.
  • Known caveat: the scenario runs the PyTorch probe once for diagnose and once for examine. If one probe times out and the other finds a HIP torch on PATH, the runs disagree and the scenario fails spuriously. A lane with no torch on PATH or only a managed runtime cannot hit this.
  • 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 PR adds no product code, and diagnose-48 is Windows-only.

@volen-silo
volen-silo marked this pull request as ready for review November 2, 2026 14:37
@volen-silo
volen-silo requested a review from a team as a code owner November 2, 2026 14:37
@volen-silo
volen-silo requested a review from tomastola November 2, 2026 14:37

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test-only change that pins the fix-13 gate for a Strix Halo Lemonade host with a managed runtime. It matches ROCMAI-830, and the unit test arms (no runtime, probe timeout, HIP-built torch, ambient HIP SDK) cover the class well. I found nothing blocking in the code. Three things need an answer before this is ready:

  1. Negative check. Please say in the PR text that you broke the gate (for example removed the managed-runtime/Lemonade exemption) and saw both the unit test and diagnose-48 go red (AGENTS §3).
  2. diagnose-48 oracle. The step restates check_13's predicate (SDK path empty or hipinfo absent), so a shared drift stays green. On the target lane the expected value is always false, so it only proves absence. Please confirm it matches the production condition exactly and name the break that turns it red.
  3. PR body wording. The gate already landed with ROCMAI-546, so this pins existing behavior. If the body says "Fixes", it should say the fix is already on main.

CI: E2E tests (Strix Halo, Ubuntu, nightly, current) failed after 45m32s with logs unavailable. The change adds no product code and diagnose-48 is Windows-only, so it looks unrelated, but please check it once the run completes. MI300X, W7900 and Strix Halo Ubuntu release are still pending.

);
}

let ambient = diagnose(&strix_halo("pytorch", "hip=7.14.60850", "path"), "");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Positive arm is good. Please confirm in the PR text that you broke the gate (e.g. removed the managed-runtime/Lemonade exemption) and saw this test go red, per AGENTS §3.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and shown in the PR description: with needs_hip forced on (removing the managed-runtime / Lemonade exemption), this test fails and diagnose-48 fails too; with the gate intact both pass. Pushed as 864aef2.

&& text("framework_source") != "managed-runtime";
let hip_llama_cpp = framework == "llama-cpp" && text("framework_rocm_version") == "GGML_HIP=ON";
let needs_hip = has_amd_gpu && (ambient_hip_torch || hip_llama_cpp);
let sdk_unusable =

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This oracle restates check_13's predicate (SDK path empty or hipinfo absent). Please confirm it matches the production condition exactly. On the target Strix Halo lane expected is always false, so the step only proves absence; note which break of the gate turns diagnose-48 red.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, and nowycondro found the same thing: check_13_hip_sdk_missing also adds 25 for an ambient HIP-built PyTorch whether or not the SDK is usable, so the oracle was wrong for a healthy-SDK host with ambient torch. The oracle now mirrors the scoring exactly (an AMD GPU plus either a HIP-built llama.cpp with no usable SDK or a non-managed HIP-built PyTorch), and a new unit test pins that host. On this lane the expected value is always false, so diagnose-48 only proves absence here; it goes red when a regression makes fix-13 score for a host with no HIP evidence (verified by forcing needs_hip on). The weak listing itself looks wrong and is tracked as ROCMAI-837; production scoring is unchanged in this test-only PR.

# `fix_13_does_not_fire_on_a_lemonade_strix_halo_host_with_a_managed_runtime`
# in crates/rocm-core/src/diagnose.rs.
@id:diagnose-windows-hip-sdk-advice-follows-the-inspection @requires-os:windows @requires-gpu
Scenario: diagnose-48 - On Windows the diagnosis proposes the HIP SDK only when the inspection finds something that needs it

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Windows-only and requires-gpu is fine, and the lane is named in docs/testing.md. Consider noting in the PR body that diagnose-48 can only go red on a Windows host with HIP-built ambient torch or on a regression of the gate on Strix Halo.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to the description, together with the caveat that the scenario can go red on a different host shape only through a regression of the gate. The description also says plainly that the gate already landed with #676 and this PR pins existing behaviour; it changes no product code.

@nowycondro nowycondro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review — needs work

Full review of the whole change (base bcd879d0…head 7b07071e). Implements the regression-test ask from ROCMAI-830 ("Regression test on a host shaped like this one; hosts that really use the HIP SDK keep fix-13"); the ticket's production fix already landed on main via #676, so this PR is test-only by design and does not claim to fix anything beyond that.

Blocking

  • The new e2e oracle's expected formula does not match check_13_hip_sdk_missing's actual scoring, so diagnose-48 can fail on a legitimately healthy host — tests/e2e-cucumber/tests/e2e/diagnose_steps.rs:3303-3306 (assert_hip_sdk_advice_follows_the_inspection), vs. crates/rocm-core/src/diagnose.rs:1762-1792 (check_13_hip_sdk_missing); same overclaim repeated in docs/testing.md's new paragraph ("fix-13-hip-sdk-missing has to appear exactly when the inspection reports an AMD GPU, a HIP-built framework that is not from a managed runtime, and no usable system SDK").
    The production function has three independent score-contributing branches inside if needs_hip { ... }: sdk_path.is_empty() → +35, else !e.hipinfo_present → +30, and a separate, non-exclusive if pytorch_is_ambient_hip_build { score += 25; ... } (lines 1782-1783) that is not gated on either of the first two. So whenever pytorch_is_ambient_hip_build is true — which is also one of the two disjuncts of needs_hip itself — the function adds +25 and pushes evidence regardless of whether the system HIP SDK is actually present and healthy. run_all_checks (diagnose.rs:3155-3165) only filters score > 0 for membership in DiagnoseReport::matched, not the 50-point MIN_SCORE_FOR_MATCH, so this nonzero score makes fix-13-hip-sdk-missing show up in matched.
    The e2e step's oracle computes expected = needs_hip && sdk_unusable (sdk_unusable = hip_sdk_path.is_empty() || hipinfo_present != true). For a host where PyTorch is an ambient (non-managed) HIP build and the system HIP SDK is fully present and working (sdk_unusable == false), expected is false while the real present (membership in matched) is true — assert_eq!(present, expected, ...) at line ~3312 would fail on this host, even though it is exactly the kind of healthy configuration the scenario exists to certify as clean.
    This gap is invisible to the PR's own new unit test: fix_13_does_not_fire_on_a_lemonade_strix_halo_host_with_a_managed_runtime's strix_halo fixture hardcodes hip_sdk_path: String::new() / hipinfo_present: false in every one of its four cases, so sdk_unusable is always true there and the discrepancy never triggers. Confirmed by history too: the commit that introduced this scoring (c9e9a32, #676) explicitly pins only "a half-installed SDK under an ambient HIP torch (30 + 25 = 55)" and "the same SDK on a host where nothing calls into HIP (0)" — no existing unit test anywhere in the file exercises hipinfo_present: true together with an ambient HIP PyTorch build. diagnose-48 is tagged only @requires-os:windows @requires-gpu, with no host-shape restriction in the step itself, so it will run on any future Windows GPU lane host, not only the current no-SDK Strix Halo runner the PR's own text assumes.
    The PR's "Known caveat" section documents a different, narrower flakiness source (the PyTorch probe racing between the diagnose and examine calls) and does not mention this one.
    Fix: either correct expected to mirror the real three-branch scoring (e.g. needs_hip && (sdk_unusable || ambient_hip_torch)), or make check_13_hip_sdk_missing itself gate the pytorch_is_ambient_hip_build bonus on sdk_unusable (matching the documented intent that the entry should be silent once nothing on the host is actually blocked by a missing/broken SDK), and soften/correct the docs/testing.md claim to match whichever side is authoritative.
    Confidence 90 — traced directly against the deterministic scoring function and corroborated by an independent second pass that reached the identical root cause via separate reasoning. · logic

Non-blocking

  • report.matched.is_empty() && !report.has_match is a stronger invariant than the sibling test uses — crates/rocm-core/src/diagnose.rs:3845-3848 vs. the neighboring fix_13_does_not_fire_on_a_lemonade_vulkan_host_with_no_hip_evidence, which only asserts fix-13's absence (that host's GPU is unsupported, so info-20 legitimately fires there). For the Strix Halo/gfx1151 shape nothing else is expected to fire, which the PR's own comment explains, but it does couple this test to every other checker in the ~20-entry catalog never scoring on these exact field values — a future catalog addition that incidentally scores on this fixture (e.g. a new advice-only driver check) would break this test for a reason unrelated to the fix-13 gate itself. Worth a one-line acknowledgment, not a blocker.

Decisions for the author

  • The PR body's "diagnose-48 has not run yet; it needs the Windows GPU lane. I will report its result here" — tradeoff / status note. CI evidence as of this review shows E2E tests (Strix Halo, Windows, current) already completed successfully and its logs show all three steps of diagnose-48 passing. This isn't a scope misstatement (it doesn't claim false completed work), just a status line that is now stale relative to the PR's own updated_at; worth refreshing before merge per the author's own "live state" convention (AGENTS.md §4/§5).

Positive signals

  • The new unit test's four-arm design (no runtime, timed-out probe, managed HIP torch, and an ambient-HIP positive control) follows the repo's established differential-test pattern exactly, including a positive control that proves the gate still fires when it should — crates/rocm-core/src/diagnose.rs:3770 onward.
  • diagnose-48 reuses the existing Given a machine with an AMD GPU / When the user diagnoses this machine in machine-readable form, with no symptom steps from diagnose-41 rather than duplicating them, and the doc cross-references ([diagnose.rs](../crates/rocm-core/src/diagnose.rs), [diagnose.feature](../tests/e2e-cucumber/features/diagnose.feature)) use the relative-link form AGENTS.md §5 requires, with no bare paths or :line suffixes.

Deployment notes

None — test-only change, no production code, no schema/config/API impact.

What this covered

Read both changed source files (crates/rocm-core/src/diagnose.rs, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs) completely, plus docs/testing.md and tests/e2e-cucumber/features/diagnose.feature's diffs and surrounding sections, against bcd879d007ba2db287688147637af2386e979616…7b07071ee9c9dfc2555e28c1ae19323ae514b8d7. Also read in full: check_13_hip_sdk_missing, diagnose()/run_all_checks()/any_cleared_threshold(), MIN_SCORE_FOR_MATCH, the Examination/Gpu struct definitions, the neighboring unit tests and the diagnose-41 step definitions for comparison, the full root AGENTS.md, the #676 commit history (all of its review rounds) via git log/git show, the live CI status-check rollup and the Windows GPU lane's job log (confirmed diagnose-48 ran and passed), and the named ticket ROCMAI-830 (retrieved by key, not by following a link).

Fan-out: one independent code-correctness worker completed and reached the primary finding above independently (confidence 90, same root cause, same lines). A second worker intended for the source-reading passes (agent-instruction adherence, history, prior-changes, code-comment compliance) failed to launch twice on a "concurrent subagent limit reached" error — the sandbox was shared with roughly thirty unrelated agents from other sessions throughout this review. Those four passes were therefore performed directly by this driver rather than by an independent blind worker; everything else in this report reflects independently-reached conclusions. Not reconciled against this PR's existing review thread or comments.

@volen-silo
volen-silo force-pushed the rocmai-830-diagnose-hip-sdk branch from 7b07071 to 864aef2 Compare November 3, 2026 08:00
@volen-silo

Copy link
Copy Markdown
Collaborator Author

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

  1. Negative check: done and in the description, for the unit test and diagnose-48 (the latter run against a debug build on this Strix Halo host).
  2. Oracle (blocking in the second review): fixed. It now mirrors check_13 exactly, including the ambient-torch bonus that applies even beside a healthy SDK. A new unit test pins that host, and docs/testing.md states the same condition. The listing itself is tracked as ROCMAI-837 and left unchanged here.
  3. Wording: the description now says the gate landed with fix(diagnose): stop ranking fix-13-hip-sdk-missing first with no HIP-based setup #676 and this PR adds tests only.
  4. Non-blocking: the managed-runtime test now asserts only that fix-13 is absent, not that nothing else fires.
  5. Stale status: updated. diagnose-48 passed on the Windows Strix Halo lane.
  6. CI: the two Strix Halo, Ubuntu lanes timed out at 45 minutes with no logs here, and fail identically on several unrelated open PRs, so I take it to be the runner; this PR adds no product code and diagnose-48 is Windows-only.

@volen-silo
volen-silo dismissed juhovainio’s stale review November 3, 2026 08:02

All points are addressed in 864aef2 and the updated description; see the replies and the summary comment. A re-review is welcome.

@juhovainio juhovainio left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests-and-docs-only change that pins the fix-13 gate for a Lemonade Strix Halo host with a managed runtime. It matches the ROCMAI-830 ask for a regression test on a host of this shape, and I found nothing blocking. The positive-control arm (ambient HIP torch still ranks fix-13 first) keeps the gate test honest.

Non-blocking notes are inline: a garbled doc comment, an arm label that proves less than it suggests, and the diagnose-48 oracle mirroring check_13's logic while pinning the behaviour ROCMAI-837 calls wrong.

Two things for the author and ticket owner:

  • The release/v0.2.1 backport is left out by design. Please confirm ROCMAI-830 is meant to close on this PR alone.
  • CI: E2E Strix Halo Windows failed 4 of 174 scenarios, but diagnose-48 passed. The failures are serve-style scenarios next to llama-server download failures from github.com, so they look like a network flake. A rerun should settle it. The Strix Halo Ubuntu lanes were cancelled by the known 45-minute timeouts, and MI300X and W7900 were still queued when I looked.

One small nit: the new paragraph in docs/testing.md has a very long line (~2246) that breaks the file's wrapping.

Approving on the basis that the E2E lanes come back green or are shown to be infra flakes.

Comment thread crates/rocm-core/src/diagnose.rs Outdated
/// answered (a `hip=` build) or timed out (`unknown`, as on the reporting
/// machine under load).
///
/// The fields mirror that machine's `rocm examine --json`. The last arm keeps the gate honest: the

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this doc comment reads as spliced fragments ("The fields mirror that machine's rocm examine --json. The last arm keeps the gate honest: ..." on one over-long line). Please reflow it into one coherent paragraph.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reflowed into one paragraph in 65c1646.

Comment thread crates/rocm-core/src/diagnose.rs Outdated
strix_halo("unknown", "", ""),
),
(
"managed runtime whose torch probe timed out",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This arm uses framework "unknown", so needs_hip is false whatever framework_source is, and it does not exercise the managed-runtime exemption (only the next arm does). Fine as a documented host shape, but the label suggests more than it proves.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. The arm is relabelled "managed runtime whose torch probe timed out (no HIP evidence)", with a comment that needs_hip is false whatever the framework source says, so only the next arm exercises the managed-runtime exemption. The assertions are unchanged.

// the SDK at all. `run_all_checks` keeps any score above zero in
// `matched`, so a host with ambient HIP torch and a healthy SDK still
// lists the entry (weakly, 25).
let expected = needs_hip && (sdk_unusable || ambient_hip_torch);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This oracle copies check_13's gating and the +25 rule, and pins the weak listing that ROCMAI-837 considers wrong. Please name ROCMAI-837 in the comment here (and in the feature comment) so whoever changes the +25 bonus finds this copy too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: the oracle comment and the feature comment now name ROCMAI-837 and say that whoever changes the +25 bonus must update this copy.

# `fix_13_does_not_fire_on_a_lemonade_strix_halo_host_with_a_managed_runtime`
# in crates/rocm-core/src/diagnose.rs.
@id:diagnose-windows-hip-sdk-advice-follows-the-inspection @requires-os:windows @requires-gpu
Scenario: diagnose-48 - On Windows the diagnosis proposes the HIP SDK only when the inspection finds something that needs it

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The title says the diagnosis "proposes" the HIP SDK, but the oracle counts a weak listing (below the 50-point match threshold) as present. Something like "lists the HIP SDK cause" would match what is asserted.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworded: the title now says the diagnosis lists the HIP SDK cause only when the inspection finds something that needs it, and the Then step says "lists the HIP SDK cause". The number (diagnose-48) and the @id are unchanged.

v0.2.1 ranked fix-13-hip-sdk-missing #1 (score 35) on a healthy native
Windows Strix Halo (gfx1151) serving through Lemonade, before and after
`rocm install sdk`. The release branch was cut before the ROCMAI-546
gate (#676) landed on main and did not pick it up; on main the checker
already scores nothing here, and `rocm diagnose` on the same machine
reports no match.

That host shape had no test of its own: the ROCMAI-546 regression test
is a gfx90c laptop, where info-20 outranks fix-13 anyway. Add a unit
test driving diagnose() on the gfx1151 shape (no runtime, a managed
runtime whose torch probe timed out, a managed HIP torch) asserting
fix-13 is absent, with an ambient HIP torch arm keeping fix-13 at #1,
and one pinning that an ambient HIP torch beside a healthy system SDK
lists fix-13 weakly (25, below the match threshold), as production does.

Add diagnose-48 for the Windows GPU lane: like diagnose-41 it names no
host shape and holds `diagnose --json` to `examine --json` on the same
machine, with an oracle that mirrors check_13's scoring: fix-13 appears
exactly when the inspection finds an AMD GPU and either a HIP-built
llama.cpp with no usable system SDK or a non-managed HIP-built PyTorch.

Signed-off-by: Eugene Volen <eugene.volen@amd.com>
@volen-silo

Copy link
Copy Markdown
Collaborator Author

Pushed 65c1646 (rebased onto main). The four nits are done (reflowed doc comment, accurate arm label, ROCMAI-837 named where the oracle copies the +25 rule, scenario title and step wording, and the long docs/testing.md line rewrapped). diagnose-48 still passes on a debug build on this Strix Halo Windows host.

On the questions: the release/v0.2.1 backport stays out of this PR, and ROCMAI-830 is meant to close on this PR as its regression test, with the backport a separate release decision. On CI: for the Strix Halo, Windows lane I read the report artifact on this PR. diagnose-48 passed; the four failures were serve-01 and serve-02 (no "resolved model" line, empty output, consistent with a failed llama.cpp download but I could not see the 404 itself) and runtime-02 and runtime-09, which look like runner configuration (a prewarmed runtime and a mirror index override). The two Strix Halo, Ubuntu lanes hit the 45-minute cap, and a fix for that is up in #852.

@volen-silo
volen-silo enabled auto-merge November 3, 2026 10:57
@volen-silo
volen-silo added this pull request to the merge queue Nov 3, 2026

@nowycondro nowycondro left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated review (Claude Code) · 65c1646

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 — no blocking findings

Full review of the whole change (base c6ab82b5…head 65c1646d).
Implements the regression-test ask from ROCMAI-830 ("Regression test on a host shaped like this one; hosts that really use the HIP SDK keep fix-13"); the ticket's production fix already landed on main via #676, so this PR is test-only by design and does not claim to fix anything beyond that. ROCMAI-830 is "In Progress", not claimed closed by this PR's text.

Blocking

None. A prior review round on an older commit of this PR (7b07071e) raised a blocking finding that the e2e oracle's expected formula in diagnose_steps.rs (assert_hip_sdk_advice_follows_the_inspection) didn't match check_13_hip_sdk_missing's real three-branch scoring — specifically, it missed that the unconditional pytorch_is_ambient_hip_build → +25 branch is not gated on SDK health. Re-tracing this fresh against the current head: the oracle now reads expected = needs_hip && (sdk_unusable || ambient_hip_torch), which is algebraically identical to production's actual membership condition (needs_hip && (sdk_unusable || pytorch_is_ambient_hip_build)) for every combination of needs_hip-via-ambient-torch vs. needs_hip-via-llama-cpp-only, crossed with SDK healthy/unhealthy. This was independently re-derived (not copied) by two parallel correctness workers and confirmed directly against crates/rocm-core/src/diagnose.rs:1736-1816, so the gap is closed at this head.

Non-blocking

  • skills/rocm-doctor/reference.md's fix-13 catalog row is stale relative to real scoring, and this PR's own new docs make the gap more visible — skills/rocm-doctor/reference.md:134. Its prose ("something on the host actually calls into HIP ... and no HIP SDK under Program Files, or hipInfo.exe missing") only describes the needs_hip && sdk_unusable branches; it omits the unconditional pytorch_is_ambient_hip_build → +25 branch that fires even when the SDK is healthy (same gap the old e2e oracle had). This was introduced by c9e9a327 (#676) itself, not by this PR, and this PR correctly leaves the file untouched since "the catalog's scoping did not change" — but this PR's own docs/testing.md paragraph and new unit test now state the accurate three-branch condition right next to a still-inaccurate catalog entry, on the exact topic this PR is about. Confidence 85 · logic (doc accuracy) · Fix: a follow-up one-line update to the reference.md row, or at least a cross-reference to ROCMAI-837, would prevent the two docs disagreeing about when fix-13 fires.
  • The self-hosted Windows GPU lane that actually exercises diagnose-48 on real hardware has not completed for this exact head commit — confirmed via the Actions API: the E2E self-hosted workflow run for 65c1646d is status: pending with zero jobs recorded as of this review; the same workflow was cancelled at the PR's first commit and queued (never completing) at its second. The repository's own ci.yml documents self-hosted GPU jobs as "non-blocking while proven out," so this is not a merge gate, and the required mock (@requires-gpu-skipping) E2E tests / E2E consolidated report / all other required checks are green for this head. The PR body's claim "diagnose-48 passed on the ... lane for this PR (15m28s)" describes an earlier commit's run (production code is unchanged between commits, so the result likely still holds), but is worth reconfirming against this exact head before merge, since no completed self-hosted run exists for it yet. Confidence 85 · architectural (verification coverage) · Fix: none required before merge per project policy; reconfirm the self-hosted lane result lands green for 65c1646d specifically.

Decisions for the author

None.

Positive signals

  • The e2e oracle's expected = needs_hip && (sdk_unusable || ambient_hip_torch) is algebraically exact against check_13_hip_sdk_missing's real three independent scoring branches — closing the earlier gap precisely rather than approximately, confirmed via two independent hand-traced truth tables.
  • The new unit test fix_13_lists_weakly_for_ambient_hip_torch_beside_a_healthy_system_sdk pins a previously-untested interaction (ambient HIP torch beside a healthy system SDK) and explicitly cross-references ROCMAI-837 so a future change to the scoring is flagged to update both the production code and this test.
  • fix_13_does_not_fire_on_a_lemonade_strix_halo_host_with_a_managed_runtime's report.matched.is_empty() && !report.has_match assertion is a strong invariant: independently confirmed that every other Windows-applicable checker in the ~20-entry catalog genuinely zeroes out on this exact fixture, not just fix-13.
  • Doc cross-references in docs/testing.md use the relative-link form and bare-path-in-.feature-comment convention AGENTS.md §5 and the repo's existing .feature files already establish.

Deployment notes

None — test-only change, no production code, no schema/config/API impact.

What this covered

Read all four changed files completely at the PR head (crates/rocm-core/src/diagnose.rs, docs/testing.md, tests/e2e-cucumber/features/diagnose.feature, tests/e2e-cucumber/tests/e2e/diagnose_steps.rs) against base c6ab82b54f685b01e6a3dacd87403c5fd16c9c4c (merge-base of origin/main and the PR head), fetched read-only via git fetch origin +pull/835/head:refs/remotes/pr/835 and never checked out. Also read in full: check_13_hip_sdk_missing, run_all_checks, diagnose(), MIN_SCORE_FOR_MATCH, the Examination/Gpu struct definitions, every Windows-applicable checker in the catalog, neighboring unit tests, the diagnose-41 step definitions, the full root AGENTS.md, the #676/c9e9a327 commit history and its review-driven rework, skills/rocm-doctor/reference.md's history, the live CI check-run/status rollup and per-workflow-run history for all three of this PR's head commits, and ticket ROCMAI-830 (retrieved by key via the tracker tool, not by following a link).

Fan-out: three independent workers ran — two parallel correctness passes over the full coupled change (both reached the same conclusion that the oracle fix is exact, independently), and one source-reading pass covering agent-instruction adherence, history, prior-changes, and code-comment compliance (this one failed to launch once on a concurrent-subagent-limit error and was re-spawned successfully; all four of its passes then ran to completion and found nothing). No pass was absorbed by this driver undisclosed.

Disclosure: the mandatory claim-check step (checking for an existing "nowycondro" review before starting) necessarily surfaced the full text of all prior reviews on this PR, including a stale "nowycondro" review pinned to an older commit (7b07071e) and a human reviewer's approval. This review's findings were reached blind by three fresh workers plus this driver's own independent trace of the production scoring function and oracle, before that text was ever reasoned from; nothing read in those prior review bodies was used to derive any finding above, including the one finding that happens to overlap with the stale review's (the oracle-formula fix was verified fresh against the code, not assumed from the old report). Not reconciled against this PR's discussion threads otherwise.


🤖 Generated with Claude Code

Merged via the queue into main with commit 9875940 Nov 3, 2026
26 checks passed
@volen-silo
volen-silo deleted the rocmai-830-diagnose-hip-sdk branch November 3, 2026 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants