From 70a62d002bb810f823f533435112c01edb34f256 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 29 Jul 2026 10:33:37 +0000 Subject: [PATCH 1/3] test(rocm-core): cover the fix consent and exit-code contract `rocm fix`'s documented exit codes 1, 4 and 5 were asserted nowhere, and no test reached a runner that writes to disk. The two that looked like they covered it did not: the dry-run unit test picks fix-2, whose Linux runner takes no FixOptions and never prompts, and the e2e preview scenario picks print-only fix-1, which returns before any runner. The confirm-before-mutating behaviour the rocm-doctor skill tells an agent to rely on was therefore untested. Open two seams so the real code can be driven directly: - `consent_without_prompt` splits the policy out of `confirm`. Testing `confirm` itself would block on `read_line` whenever the suite runs with a terminal attached, which is why the non-interactive refusal rule had no test. - `pin_device_in_rc_file` extracts fix-9's mutation body, taking the rc path and the consent verdict as parameters, so it runs against a temp file with no $HOME mutation and no TTY dependency. Behaviour is unchanged. The dry-run and already-pinned tests assert by panicking if the consent gate is reached, so reordering those guards fails loudly. Signed-off-by: Eugene Volen --- crates/rocm-core/src/fix.rs | 230 ++++++++++++++++++++++++++++++++++-- 1 file changed, 222 insertions(+), 8 deletions(-) diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index 67602038..e2ba7f10 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -476,13 +476,30 @@ pub fn apply(fix_id: &str, opts: &FixOptions) -> i32 { // Consent / environment helpers // --------------------------------------------------------------------------- -fn confirm(prompt: &str, assume_yes: bool) -> bool { +/// The half of the consent decision that needs no I/O: `Some(verdict)` when the +/// answer is already determined, `None` when the user must actually be asked. +/// +/// Split out from [`confirm`] so the two rules that matter — `--yes` approves, +/// and a non-interactive shell *refuses* rather than silently proceeding — are +/// testable without a controlled stdin. A test that drove `confirm` directly +/// would block on `read_line` whenever the suite happened to run with a terminal +/// attached. +const fn consent_without_prompt(assume_yes: bool, stdin_is_terminal: bool) -> Option { if assume_yes { - return true; + return Some(true); } - if !std::io::stdin().is_terminal() { - println!("Non-interactive shell and --yes not passed; refusing to apply."); - return false; + if !stdin_is_terminal { + return Some(false); + } + None +} + +fn confirm(prompt: &str, assume_yes: bool) -> bool { + if let Some(verdict) = consent_without_prompt(assume_yes, std::io::stdin().is_terminal()) { + if !verdict { + println!("Non-interactive shell and --yes not passed; refusing to apply."); + } + return verdict; } print!("{prompt} [y/N]: "); let _ = std::io::stdout().flush(); @@ -831,8 +848,29 @@ fn run_hip_visible_devices_linux(opts: &FixOptions) -> i32 { println!("Could not determine your home directory."); return 3; }; + pin_device_in_rc_file(&rc_file, idx, opts, || { + confirm(&format!("Append to {}?", rc_file.display()), opts.yes) + }) +} + +/// The part of fix-9 that actually touches the user's machine: decide whether +/// the rc file already pins a device, honour `--dry-run`, ask for consent, and +/// append. +/// +/// Taking the rc path and the consent decision as parameters keeps this — the +/// one auto-fix path on Linux that really mutates a file — testable end to end +/// against a temp file, with no `$HOME` mutation and no dependency on whether +/// stdin happens to be a terminal. Exit codes are the contract documented in +/// `skills/rocm-doctor/reference.md`: 0 applied/dry-run/already-set, 4 write +/// failed, 5 declined. +fn pin_device_in_rc_file( + rc_file: &Path, + idx: i64, + opts: &FixOptions, + consent: impl FnOnce() -> bool, +) -> i32 { let export_line = format!("export HIP_VISIBLE_DEVICES={idx}"); - if let Ok(existing) = std::fs::read_to_string(&rc_file) + if let Ok(existing) = std::fs::read_to_string(rc_file) && existing.contains("HIP_VISIBLE_DEVICES=") { println!( @@ -847,11 +885,11 @@ fn run_hip_visible_devices_linux(opts: &FixOptions) -> i32 { println!("(dry-run; not executed)"); return 0; } - if !confirm(&format!("Append to {}?", rc_file.display()), opts.yes) { + if !consent() { return 5; } if let Err(exc) = append_line( - &rc_file, + rc_file, "# Added by rocm examine (fix-9-igpu-dgpu)", &export_line, ) { @@ -1040,4 +1078,180 @@ mod tests { assert!(listing.contains(r.fix_id), "listing missing {}", r.fix_id); } } + + // ── Consent and mutation contract ────────────────────────────── + // + // `skills/rocm-doctor/reference.md` documents six exit codes, and the skill + // tells an agent it may run an auto-fix because the CLI "refuses on a + // non-interactive shell without --yes" and "confirms before mutating". + // Before these tests, codes 1, 4 and 5 were asserted nowhere in the repo and + // no test ever reached a runner that writes to disk: the two that look like + // they cover it don't — `dry_run_never_mutates_...` picks fix-2, whose Linux + // runner takes no FixOptions and never prompts, and diagnose.feature's + // preview scenario picks print-only fix-1, which returns before any runner. + // + // fix-9 is the auto-fix used here because its Linux runner is the one that + // genuinely appends to a file, and `pin_device_in_rc_file` lets that run + // against a temp path with an injected consent verdict. + + /// A unique scratch path under the workspace's test-artifact dir, following + /// the convention in `lib.rs`'s tests (no `tempfile` dev-dependency here). + fn scratch_dir(name: &str) -> PathBuf { + let dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join(".rocm-work") + .join("tests") + .join("core") + .join(format!( + "fix-{name}-{}-{:?}", + std::process::id(), + std::thread::current().id() + )); + std::fs::create_dir_all(&dir).expect("failed to create scratch dir"); + dir + } + + fn pinning_opts() -> FixOptions { + FixOptions { + device_index: Some(1), + ..FixOptions::default() + } + } + + #[test] + fn yes_flag_approves_without_prompting() { + assert_eq!(consent_without_prompt(true, false), Some(true)); + assert_eq!(consent_without_prompt(true, true), Some(true)); + } + + #[test] + fn non_interactive_shell_refuses_instead_of_proceeding() { + // The rule the skill relies on: without --yes and with nothing to prompt, + // the answer is a definite NO, never an implicit yes. + assert_eq!(consent_without_prompt(false, false), Some(false)); + } + + #[test] + fn interactive_shell_without_yes_must_actually_ask() { + assert_eq!(consent_without_prompt(false, true), None); + } + + #[test] + fn declining_consent_returns_5_and_writes_nothing() { + let rc = scratch_dir("declined").join(".bashrc"); + std::fs::write(&rc, "# existing\n").expect("seed rc file"); + + let code = pin_device_in_rc_file(&rc, 1, &pinning_opts(), || false); + + assert_eq!(code, 5, "a declined fix must exit 5"); + assert_eq!( + std::fs::read_to_string(&rc).expect("read rc"), + "# existing\n", + "a declined fix must not touch the file" + ); + std::fs::remove_dir_all(rc.parent().expect("parent")).ok(); + } + + #[test] + fn dry_run_returns_0_and_writes_nothing_even_with_consent() { + let rc = scratch_dir("dryrun").join(".bashrc"); + std::fs::write(&rc, "# existing\n").expect("seed rc file"); + let opts = FixOptions { + dry_run: true, + ..pinning_opts() + }; + + // Consent would be granted; --dry-run must short-circuit before it. + let code = pin_device_in_rc_file(&rc, 1, &opts, || { + panic!("dry-run must not reach the consent gate") + }); + + assert_eq!(code, 0); + assert_eq!( + std::fs::read_to_string(&rc).expect("read rc"), + "# existing\n", + "a dry-run must not touch the file" + ); + std::fs::remove_dir_all(rc.parent().expect("parent")).ok(); + } + + #[test] + fn granting_consent_appends_the_export_and_returns_0() { + let rc = scratch_dir("granted").join(".bashrc"); + std::fs::write(&rc, "# existing\n").expect("seed rc file"); + + let code = pin_device_in_rc_file(&rc, 3, &pinning_opts(), || true); + + assert_eq!(code, 0); + let body = std::fs::read_to_string(&rc).expect("read rc"); + assert!( + body.starts_with("# existing\n"), + "existing rc content must be preserved:\n{body}" + ); + assert!( + body.contains("export HIP_VISIBLE_DEVICES=3"), + "expected the export line to be appended:\n{body}" + ); + std::fs::remove_dir_all(rc.parent().expect("parent")).ok(); + } + + #[test] + fn an_rc_file_that_already_pins_a_device_is_left_alone() { + let rc = scratch_dir("already-pinned").join(".bashrc"); + let original = "export HIP_VISIBLE_DEVICES=0\n"; + std::fs::write(&rc, original).expect("seed rc file"); + + let code = pin_device_in_rc_file(&rc, 1, &pinning_opts(), || { + panic!("an already-pinned rc file must not reach the consent gate") + }); + + assert_eq!(code, 0, "already-pinned is success, not failure"); + assert_eq!( + std::fs::read_to_string(&rc).expect("read rc"), + original, + "must not append a second, conflicting pin" + ); + std::fs::remove_dir_all(rc.parent().expect("parent")).ok(); + } + + #[test] + fn a_failed_write_returns_4_rather_than_reporting_success() { + // A directory where the rc file should be: the append fails at open(). + // This is the only way the documented "attempted but the command + // failed" code is reachable for this fix. + let dir = scratch_dir("write-fails"); + let rc = dir.join(".bashrc"); + std::fs::create_dir_all(&rc).expect("create dir in place of rc file"); + + let code = pin_device_in_rc_file(&rc, 1, &pinning_opts(), || true); + + assert_eq!(code, 4, "a failed write must exit 4, not 0"); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn pinning_without_a_device_index_is_not_applicable() { + if !runtime_is_linux() { + return; + } + // Reached through the real runner: the missing --device-index guard sits + // ahead of any home-directory lookup, so this needs no scratch state. + let code = run_hip_visible_devices_linux(&FixOptions::default()); + assert_eq!(code, 3, "a missing --device-index is 'not applicable' (3)"); + } + + #[test] + fn exit_code_1_is_unreachable_by_construction() { + // apply() returns 1 only for an auto_applicable recipe with no runner. + // `auto_applicable_recipes_have_a_runner` keeps that impossible; assert + // the reachability argument here so reference.md's row 1 is accounted + // for rather than silently untested. + assert!( + RECIPES + .iter() + .all(|r| !r.auto_applicable || r.runner.is_some()), + "an auto-applicable recipe without a runner would make exit 1 reachable" + ); + } } From 987dd9a230b2aafa2804f6ddab52d05fff6a74a9 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 29 Jul 2026 10:33:57 +0000 Subject: [PATCH 2/3] test(e2e): mirror the rocm-doctor skill and test its CLI contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit rocm-doctor is a thin driver: the probe, the closed 15-mode catalog and the fixes all ship inside the binary, so what the skill owns is a contract — which fix-ids exist, which the CLI applies itself, which machines each is for, and what a diagnosis carries. Nothing tested that seam. amd/skills has no rocm binary in CI, and diagnose.feature deliberately asserts only the shape of a diagnosis so it stays host-independent. A renamed fix-id or a 16th failure mode merged green here and silently broke the published skill. Mirror the three files byte-verbatim next to the binary they drive, and parse reference.md's catalog table as the expected-value fixture for a new feature that diffs it against what `rocm fix` really reports. That is a narrow, deliberate exception to the suite's black-box rule: nothing is imported from the codebase, a documentation artifact is read as test data, and that artifact is the thing under test. Scope stays narrow — claims already proven elsewhere are not restated. The exit codes and consent machinery are unit tested in rocm-core, the status enum in examine.rs, and the WSL2 out-of-scope rule in diagnose.rs. What is left needs a real binary: that the documented catalog and the shipped one agree, and that the JSON an agent reads is actually plumbed through. The mirror carries no licence header: SKILL.md must open with YAML frontmatter for the skill loader, which pushes any header outside hawkeye's detection window. A positive exclude cannot override a hawkeye negation, so licenserc.toml now enumerates the markdown scopes to check and omits this one. Verified that a headerless file elsewhere still fails the check. skills/** joins the heavy paths filter because reference.md is now a fixture, and an advisory, non-blocking job reports drift against amd/skills. Signed-off-by: Eugene Volen --- .github/workflows/ci.yml | 60 +++ AGENTS.md | 21 + licenserc.toml | 25 +- skills/rocm-doctor/SKILL.md | 139 ++++++ skills/rocm-doctor/reference.md | 91 ++++ skills/rocm-doctor/skill-card.md | 17 + tests/e2e-cucumber/README.md | 34 +- .../features/rocm_doctor_skill.feature | 66 +++ tests/e2e-cucumber/tests/e2e.rs | 6 + tests/e2e-cucumber/tests/e2e/skill_steps.rs | 404 ++++++++++++++++++ 10 files changed, 854 insertions(+), 9 deletions(-) create mode 100644 skills/rocm-doctor/SKILL.md create mode 100644 skills/rocm-doctor/reference.md create mode 100644 skills/rocm-doctor/skill-card.md create mode 100644 tests/e2e-cucumber/features/rocm_doctor_skill.feature create mode 100644 tests/e2e-cucumber/tests/e2e/skill_steps.rs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 69ef6b38..625b4670 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -116,6 +116,11 @@ jobs: - '**/*.ps1' - '**/*.feature' - 'tests/e2e-cucumber/expectations.toml' + # skills/rocm-doctor/reference.md is an e2e FIXTURE: the + # rocm_doctor_skill.feature scenarios parse its catalog table and + # compare it to `rocm fix`. A skill-only edit must therefore run + # the E2E job, which gates on `heavy`. + - 'skills/**' - 'install*' # Pinned-key consistency compares docs/keys/* against the installers, # so a canonical-key-only change must trigger the heavy job that runs @@ -178,6 +183,61 @@ jobs: --body "$PR_BODY" \ --commits-file /tmp/pr-commits.txt >> "$GITHUB_STEP_SUMMARY" + # Advisory nudge: `skills/rocm-doctor/` is a byte-verbatim mirror of the skill + # published in amd/skills, kept here so the e2e suite can test its claims + # against the binary it drives. The mirror rots when the UPSTREAM copy moves + # and this one doesn't, so the check runs on every PR rather than only when + # skills/ is touched — otherwise the case it exists for never fires. Never + # blocks: advisory only, fail-open on any fetch problem, and deliberately NOT + # a required check. + skill-mirror-drift: + name: rocm-doctor mirror drift (advisory) + runs-on: ubuntu-latest + if: github.event_name == 'pull_request' + steps: + - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 + - name: Compare skills/rocm-doctor against amd/skills + env: + UPSTREAM: https://raw.githubusercontent.com/amd/skills/main/skills/rocm-doctor + run: | + drifted=0 + missing=0 + for f in SKILL.md reference.md skill-card.md; do + if ! curl --proto '=https' --tlsv1.2 -fsSL "${UPSTREAM}/${f}" -o "/tmp/upstream-${f}"; then + echo "- \`${f}\`: could not fetch from amd/skills (not published yet, or upstream unreachable)" >> /tmp/drift.md + missing=$((missing + 1)) + continue + fi + if ! diff -u "/tmp/upstream-${f}" "skills/rocm-doctor/${f}" > "/tmp/diff-${f}"; then + drifted=$((drifted + 1)) + { + echo "
${f} differs from amd/skills" + echo + echo '```diff' + head -c 8000 "/tmp/diff-${f}" + echo '```' + echo "
" + } >> /tmp/drift.md + fi + done + { + echo "## rocm-doctor skill mirror" + echo + if [ "${drifted}" -eq 0 ] && [ "${missing}" -eq 0 ]; then + echo "In sync with \`amd/skills@main\`." + elif [ "${drifted}" -eq 0 ]; then + echo "Nothing to compare against yet — the skill is not on \`amd/skills@main\`." + echo + cat /tmp/drift.md + else + echo "\`skills/rocm-doctor/\` is a verbatim mirror. Reconcile it with the" + echo "published copy in \`amd/skills\` — and remember the failure-mode catalog" + echo "itself is authoritative in \`crates/rocm-core\`, so the docs follow the CLI." + echo + cat /tmp/drift.md + fi + } >> "$GITHUB_STEP_SUMMARY" + build-and-test: runs-on: ubuntu-latest # Don't spend per-OS build/test cycles unless the cheap lint gate is green. diff --git a/AGENTS.md b/AGENTS.md index 85400243..f4bfca38 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -152,6 +152,7 @@ When changing assistant-adjacent behavior, keep consistency with: - `docs/llm-tool-use.md` - `skills/rocm-cli-assistant/SKILL.md` +- `skills/rocm-doctor/SKILL.md` and `skills/rocm-doctor/reference.md` Required consistency points: @@ -160,6 +161,26 @@ Required consistency points: - avoid invented shell/package-manager commands in assistant behavior paths - preserve built-in assistant constraints and no-CPU-fallback policy +### `skills/rocm-doctor/` — a mirror, and a test fixture + +`skills/rocm-cli-assistant/SKILL.md` is compiled into the binary +(`include_str!` in `apps/rocm/src/main.rs`). `skills/rocm-doctor/` is different +on two counts, and both change how you edit it: + +- **It is a byte-verbatim mirror** of the skill published in + [`amd/skills`](https://github.com/amd/skills/tree/main/skills/rocm-doctor). + Change both copies together. The advisory `skill-mirror-drift` CI job diffs + them on every PR and reports in the step summary; it never blocks. Because the + files must stay byte-identical to upstream they carry no AMD licence header, + which is why `licenserc.toml` force-includes markdown per scope rather than + with one blanket `!**/*.md` — read the note at the top of that file before + changing it. +- **`reference.md` is an e2e fixture.** `tests/e2e-cucumber/features/rocm_doctor_skill.feature` + parses its closed-catalog table and compares it to what `rocm fix` reports. + The catalog is authoritative in `crates/rocm-core/src/fix.rs` — adding, + renaming, or re-scoping a failure mode means changing the CLI **first**, then + the two docs. That feature is what catches you if you forget. + ## 8) Verification Matrix For This Repo Minimum quality gate before upstream-ready status: diff --git a/licenserc.toml b/licenserc.toml index 50de08d8..69f7784f 100644 --- a/licenserc.toml +++ b/licenserc.toml @@ -2,13 +2,18 @@ ## Run locally: hawkeye check ## Docs: https://github.com/korandoru/hawkeye ## -## Note: `!**/*.md` is a force-include that overrides hawkeye's built-in -## default exclusion of *.md. It also overrides the positive excludes below -## for .md files — there is no way to subsequently re-exclude a subset of -## .md paths once the negation is in effect. In practice this is harmless -## because hawkeye's git integration skips untracked directories, but any -## tracked *.md file (e.g. in third_party/) would be required to carry a -## copyright header. +## Note on *.md: hawkeye excludes markdown by default, so each scope we DO want +## header-checked is force-included with a `!` negation below. A negation cannot +## be narrowed afterwards — a positive exclude never wins against it, and a +## pattern with no slash (`!*.md`) matches at every depth — so the scopes are +## enumerated and anchored instead of using one blanket `!**/*.md`. Adding a new +## top-level directory that holds tracked markdown means adding it to that list; +## otherwise its headers go unchecked. +## +## `skills/rocm-doctor/` is deliberately absent: it is a byte-verbatim mirror of +## the published skill in amd/skills (MIT there too), and its SKILL.md must open +## with YAML frontmatter for the skill loader, which pushes any header out of +## hawkeye's detection window. See AGENTS.md §7. inlineHeader = """ Copyright © Advanced Micro Devices, Inc., or its affiliates. @@ -26,7 +31,11 @@ includes = [ ] excludes = [ - "!**/*.md", + "!/*.md", + "!.github/**/*.md", + "!docs/**/*.md", + "!skills/rocm-cli-assistant/*.md", + "!tests/**/*.md", "target/**", ".claude/**", ".tmp-*/**", diff --git a/skills/rocm-doctor/SKILL.md b/skills/rocm-doctor/SKILL.md new file mode 100644 index 00000000..d526f252 --- /dev/null +++ b/skills/rocm-doctor/SKILL.md @@ -0,0 +1,139 @@ +--- +name: rocm-doctor +description: >- + Diagnoses why ROCm, the HIP SDK, PyTorch, or llama.cpp is broken on an AMD GPU + on Linux or Windows, then applies a low-risk fix with consent or hands back the + exact next step. Also routes Lemonade, LM Studio, and Ollama problems to the + right upstream channel. Use when the user reports that ROCm or HIP "isn't + working", torch.cuda.is_available() is False, rocminfo / hipInfo can't see the + GPU, or hits hipErrorNoBinaryForGpu, HSA_STATUS_ERROR_INVALID_ISA, "invalid + device function", "no kernel image is available", cannot open /dev/kfd, + permission denied on /dev/kfd, "ROCk module is NOT loaded", a missing + libamdhip64.so / amdhip64_6.dll / hipblas.dll / vcruntime140_1.dll, an + HSA_OVERRIDE_GFX_VERSION page fault, an iGPU+dGPU crash, a container that can't + see the GPU, or an amdgpu-install / DKMS failure. Backed by the `rocm` CLI + (`rocm examine` / `rocm diagnose` / `rocm fix`); this skill is a thin driver + over those commands, not a re-implementation. +--- + +# ROCm Doctor + +Given a "ROCm / PyTorch / llama.cpp isn't working on my AMD GPU" complaint, +identify which **known misconfiguration** is the cause and either fix it (with +consent) or hand back the exact next step. + +This skill does **not** probe or reason on its own. The `rocm` CLI owns the +probe, the closed failure-mode catalog, and the fixes; the skill just drives it +and relays the results. The catalog is a **closed list** — if the symptom +doesn't match a known mode, route the user upstream instead of guessing. + +## Workflow + +0. **Ensure the `rocm` CLI is present.** Everything below shells out to it, so + check first and install it if missing: + + ``` + rocm --version + ``` + + If that succeeds, skip to step 1. If it's not found, install it **with the + user's consent** (this fetches and runs an installer that drops the `rocm` and + `rocmd` binaries into `~/.local/bin`). Only nightly builds are published + today, so install from the `nightly` channel: + + - **Linux / macOS:** + ``` + curl -fsSL https://raw.githubusercontent.com/ROCm/rocm-cli/main/install.sh | sh -s -- nightly + ``` + - **Windows (PowerShell):** + ``` + $env:ROCM_CLI_CHANNEL = "nightly" + irm https://raw.githubusercontent.com/ROCm/rocm-cli/main/install.ps1 | iex + ``` + + (Once rocm-cli cuts a stable release, drop the `nightly` channel — `sh` / + `iex` alone will pull the latest stable build.) + + After install, confirm `~/.local/bin` is on `PATH` and re-run `rocm --version`. + If it still isn't available, hand the user the install page + (https://github.com/ROCm/rocm-cli) and stop. + +1. **Diagnose.** Pass the user's error text as the symptom: + + ``` + rocm diagnose --symptom "" --json + ``` + + Read the JSON: + - `matched[]` — ranked causes, each with `id`, `title`, `score` (0–100), + `evidence[]`, and a `fix` (with `fix_id`, `summary`, `commands`, `verify`, + `notes`, and the `needs_sudo` / `needs_reboot` / `needs_relogin` / + `auto_applicable` flags). `score >= 75` = high confidence; `50–74` = likely + (confirm one more piece of evidence with the user first). + - `out_of_scope` — when set (e.g. WSL2), do **not** diagnose. First, if the + user's symptom clearly names an app that ships its own runtime (Lemonade, + Ollama, LM Studio), route them to that app's tracker (see + [Framework routing](#framework-routing)) — those trackers apply regardless + of platform. Otherwise relay the `out_of_scope` message and stop (see + [Out of scope](#out-of-scope)). + - `route_when_no_match` — when `matched` is empty, hand the user this + upstream tracker; **do not speculate**. Note the CLI picks this target from + the *host-detected* framework, not from the symptom text — so for an app + named only in the symptom, route it yourself per + [Framework routing](#framework-routing). + +2. **Propose the fix.** Show the top match's `title`, `evidence`, plan, and + `verify` command. Only propose applying it when the user is on board. + +3. **Apply with consent.** For an auto-applicable fix: + + ``` + rocm fix # auto fixes: prompt before changing anything + rocm fix --dry-run # show the exact change, touch nothing + rocm fix --yes # required to apply in a non-interactive shell + ``` + + Only the four auto-applicable fixes prompt and mutate. The other 11 are + **print-only** (bootloader, kernel, reinstall, Windows driver, …): `rocm fix + ` just prints the plan for the user to run themselves — no prompt, and the + CLI never performs those. + +4. **Verify.** Have the user run the `verify` command from the diagnosis. + +Use `rocm examine` (or `rocm examine --json`) when you only need the host state +(GPU, driver, ROCm install, groups, framework) without a diagnosis. + +## Framework routing + +`rocm diagnose` covers frameworks that build against the **system** ROCm/HIP: + +- **PyTorch**, **llama.cpp** — in scope; diagnose normally. + +Apps that ship their **own** ROCm runtime aren't diagnosed here — route the user +to the right tracker. (The CLI's `route_when_no_match` also targets these, but +only when the host probe *detects* that app; when the app is named only in the +symptom, do the routing yourself using the list below.) + +- **Lemonade** → https://github.com/lemonade-sdk/lemonade/issues +- **Ollama** → https://github.com/ollama/ollama/issues +- **LM Studio** → in-app support (no public repo) +- Anything else with no catalog match → ROCm core: + https://github.com/ROCm/ROCm/issues (this is what `route_when_no_match` + returns by default). + +## Out of scope + +- **WSL2** — a distinct platform (`/dev/dxg` + the Windows host driver, not the + in-tree `amdgpu` module or `/dev/kfd`). `rocm examine`/`diagnose` detect it and + route out; relay that guidance and point at AMD's ROCm-on-WSL guide. +- **NVIDIA / Intel / Apple Silicon GPUs**, and **fresh installs on a clean + machine** (a setup task, not a diagnosis). Exit cleanly and say so. + +## Rules + +- Never invent a fix. If `rocm diagnose` returns no match, route upstream. +- Never run a mutating fix without the user's explicit OK; prefer `--dry-run` + first. New failure modes are added to the CLI catalog, not improvised here. + +See `reference.md` for the full closed catalog and the CLI command/exit-code +reference. diff --git a/skills/rocm-doctor/reference.md b/skills/rocm-doctor/reference.md new file mode 100644 index 00000000..172eaa99 --- /dev/null +++ b/skills/rocm-doctor/reference.md @@ -0,0 +1,91 @@ +# ROCm Doctor — reference + +The closed failure-mode catalog and the CLI it drives. The catalog is +authoritative in the `rocm` CLI (`crates/rocm-core`); this file mirrors it for +humans. To add or change a failure mode, change the CLI catalog — not this doc. + +## CLI commands + +### `rocm examine [--json]` + +Inspect the host: GPU + gfx target, driver, ROCm install, render/video groups, +`/dev/kfd` + render devices, kernel modules, framework introspection, recent +amdgpu kernel-log evidence. `--json` emits the **Examination** document for +tooling. + +- It's a general system inspector, so it **always exits 0**. The verdict is the + `status` field: `ok` · `no-amd-gpu` · `wsl` · `unsupported-os` · `degraded`. + +### `rocm diagnose [--symptom ""] [--top N] [--json]` + +Match the host + symptom against the closed catalog. **Always exits 0**; read +the result from `--json`: + +- `matched[]` — ranked `{ id, title, score, evidence[], fix }`. Tiers: + `>= 75` high confidence, `50–74` likely, `< 50` weak. +- `min_score_for_match` (50), `high_confidence_threshold` (75). +- `out_of_scope` — set when the host is off-catalog (e.g. WSL2); `matched` is empty. +- `route_when_no_match` — `{ target, url }` upstream tracker to use when nothing matched. + +### `rocm fix [] [--yes] [--dry-run] [--device-index N]` + +Apply a fix by id (run with no id to list). Exit codes: + +| code | meaning | +| --- | --- | +| 0 | applied / dry-run / print-only plan / list | +| 1 | internal error | +| 2 | usage error (incl. unknown fix-id) | +| 3 | not applicable on this host (OS mismatch, missing/negative `--device-index`) — nothing changed | +| 4 | attempted but the command failed | +| 5 | user declined at the prompt | + +Only four fixes are auto-applicable — `fix-2-unset-override`, +`fix-4-render-group`, `fix-6-path`, `fix-9-igpu-dgpu` — and the rest print their +plan for the user to run. Pass the **full** id (`rocm fix fix-2-unset-override`, +not `rocm fix fix-2`; a short id returns exit 2, unknown fix-id). Auto fixes +print the exact command, honor `--dry-run`, refuse on a non-interactive shell +without `--yes`, and confirm before mutating. + +## Closed catalog (15 failure modes) + +| id | OS | Failure mode | Typical signal | Auto-fix | +| --- | --- | --- | --- | --- | +| `fix-1-arch` | both | GPU gfx target not in the framework's build arch list | `hipErrorNoBinaryForGpu`, `HSA_STATUS_ERROR_INVALID_ISA`, "invalid device function" | no | +| `fix-2-unset-override` | both | `HSA_OVERRIDE_GFX_VERSION` set on a GPU that now has a native wheel | page faults / `OUT_OF_REGISTERS`, override set in env | yes | +| `fix-3-rocm-kernel` | linux | ROCm + distro/kernel form an unsupported triple | ROCm installed but `amdgpu` not loaded; DKMS build failure | no | +| `fix-4-render-group` | linux | User not in `render`/`video` group (or `/dev/kfd` owned by the other group) | cannot open `/dev/kfd`, permission denied | yes | +| `fix-5-amdgpu-load` | linux | `amdgpu` module not loaded (or blacklisted) | "ROCk module is NOT loaded", blacklist entry, Secure Boot | no | +| `fix-6-path` | both | ROCm/HIP binaries not on PATH after install | `rocminfo: command not found`, `hipInfo` missing from PATH | yes | +| `fix-7-stale-repos` | linux | Stale/conflicting APT/DNF repos from prior installer runs | apt 404 `repo.radeon.com`, unmet deps, ≥2 ROCm repo files | no | +| `fix-8-wheel-rocm` | both | Framework wheel built for a different ROCm major than the system | `libamdhip64.so.X` / `amdhip64_X.dll` load failure | no | +| `fix-9-igpu-dgpu` | both | iGPU enumerated alongside dGPU, destabilising the runtime | APU + discrete AMD present, `HIP_VISIBLE_DEVICES` unset, crash/segfault | yes | +| `fix-10-container` | linux | Container can't see `/dev/kfd` or `/dev/dri/renderD*` | running in docker/podman, kfd/render devices missing | no | +| `fix-11-iommu` | linux | Multi-GPU hang with IOMMU enabled | ≥2 AMD GPUs, `iommu=` not `pt`, hang/deadlock/timeout | no | +| `fix-12-installer` | linux | `amdgpu-install` left a broken DKMS / repo state | dpkg half-configured, DKMS failed, `--accept-eula` | no | +| `fix-13-hip-sdk-missing` | windows | HIP SDK not installed | no HIP SDK under Program Files, `hipInfo` not recognized | no | +| `fix-14-adrenalin-too-old` | windows | Adrenalin / kernel-mode driver too old for the HIP SDK | `hipInfo` can't enumerate, "driver too old", HSA "no agents found" | no | +| `fix-15-msvc-redist` | windows | MSVC runtime missing (HIP DLLs can't load) | `vcruntime140.dll` / `vcruntime140_1.dll` missing | no | + +Linux-only: fix-3, -4, -5, -7, -10, -11, -12. Windows-only: fix-13, -14, -15. +Cross-platform: fix-1, -2, -6, -8, -9. + +## Framework routing + +`rocm diagnose` diagnoses frameworks built against the **system** ROCm/HIP: + +- **PyTorch**, **llama.cpp** — in scope. + +Apps that ship their own runtime are routed upstream (via `route_when_no_match`): + +- **Lemonade** → https://github.com/lemonade-sdk/lemonade/issues +- **Ollama** → https://github.com/ollama/ollama/issues +- **LM Studio** → in-app support (no public repo) +- Otherwise → ROCm core: https://github.com/ROCm/ROCm/issues + +## Out of scope + +- **WSL2** — distinct platform (`/dev/dxg` + Windows host driver). Detected and + routed out; point at AMD's ROCm-on-WSL guide: + https://rocm.docs.amd.com/projects/radeon-ryzen/en/latest/docs/install/installryz/wsl/howto_wsl.html +- NVIDIA / Intel / Apple Silicon GPUs; fresh installs on a clean machine. diff --git a/skills/rocm-doctor/skill-card.md b/skills/rocm-doctor/skill-card.md new file mode 100644 index 00000000..aef615ea --- /dev/null +++ b/skills/rocm-doctor/skill-card.md @@ -0,0 +1,17 @@ +# Skill Card + +## Description + +Diagnoses ROCm / HIP SDK / PyTorch / llama.cpp failures on AMD GPUs (Linux and +Windows) against a closed list of known misconfigurations, and applies a +low-risk fix with consent or routes the user to the right upstream channel. A +thin driver over the `rocm` CLI (`rocm examine` / `rocm diagnose` / `rocm fix`) — +the probe, catalog, and fixes live in the CLI, versioned with the binary. + +## Owner + +rocm-cli team (AMD) + +## License + +MIT diff --git a/tests/e2e-cucumber/README.md b/tests/e2e-cucumber/README.md index d9afbc8a..7183ed1b 100644 --- a/tests/e2e-cucumber/README.md +++ b/tests/e2e-cucumber/README.md @@ -33,6 +33,7 @@ tests/e2e-cucumber/ │ ├── chat.feature │ ├── examine.feature │ ├── model_serving.feature +│ ├── rocm_doctor_skill.feature # skill↔CLI contract (see below) │ └── runtime_setup.feature │ ├── tests/ # test binary + step modules @@ -41,7 +42,8 @@ tests/e2e-cucumber/ │ ├── chat_steps.rs │ ├── examine_steps.rs │ ├── runtime_steps.rs -│ └── serving_steps.rs +│ ├── serving_steps.rs +│ └── skill_steps.rs │ └── src/ # shared test infrastructure ├── lib.rs @@ -177,3 +179,33 @@ The `.feature` file is both the spec and the test input — cucumber reads it at - **Isolated state.** Each scenario uses isolated config, data, and cache directories. Tests never touch `~/.rocm`. - **Behavioral language.** Feature files describe what users care about, not implementation details. How steps are implemented (mock vs real, which port, which API) stays in the step functions. - **OS-assigned ports.** The mock server binds to `127.0.0.1:0` to avoid port conflicts between tests. + +## The rocm-doctor skill contract + +`rocm_doctor_skill.feature` is the one feature that treats a file in this repo as +an expected value. `skills/rocm-doctor/` is a byte-verbatim mirror of a skill +published in [`amd/skills`](https://github.com/amd/skills); the skill owns no +probe, no catalog and no fixes — all of that ships inside the binary — so what it +does own is a **contract**: which fix-ids exist, which ones the CLI applies +itself, which machines each is for, what a diagnosis carries, what the exit codes +mean. Nothing else tests that seam. `diagnose.feature` deliberately asserts only +the *shape* of a diagnosis so it stays host-independent, and the `rocm-core` unit +tests cannot see a document that lives outside the crate. + +So `skill_steps.rs` parses the closed-catalog table out of +`skills/rocm-doctor/reference.md` and diffs it against `rocm fix`. That is a +deliberate, narrow exception to **Black-box only** above: nothing is imported +from the rocm-cli codebase — a *documentation artifact* is read as test data, and +that artifact is the thing under test. + +Two rules follow: + +- The catalog is authoritative in `crates/rocm-core/src/fix.rs`. When one of + these scenarios fails, the CLI is right and the docs are what change. +- A skill-only edit must still run this job, so `skills/**` is in the `heavy` + paths filter that gates the `e2e` job. + +None of the scenarios assert that a symptom actually matched: on a host the +catalog rules out of scope (WSL2) an empty `matched` list is the correct answer. +They assert the coupling — out of scope implies no causes offered — so the +feature is clean on WSL2, the no-GPU lane, and the GPU lanes alike. diff --git a/tests/e2e-cucumber/features/rocm_doctor_skill.feature b/tests/e2e-cucumber/features/rocm_doctor_skill.feature new file mode 100644 index 00000000..ab6468f1 --- /dev/null +++ b/tests/e2e-cucumber/features/rocm_doctor_skill.feature @@ -0,0 +1,66 @@ +Feature: The ROCm Doctor skill's instructions match the CLI they drive + + # `skills/rocm-doctor/` is a byte-verbatim mirror of the skill published in + # amd/skills. The skill owns no probe, no catalog and no fixes — all of that + # ships inside the `rocm` binary — so what it DOES own is a contract: which + # fix-ids exist, which the CLI applies itself, which machines each one is for, + # and what a diagnosis carries. + # + # The centre of this feature is the catalog diff (scenarios 1-3): the table in + # `reference.md` is parsed and compared to what `rocm fix` really reports, so a + # renamed fix-id, a re-scoped OS, a flipped auto-flag, or a 16th failure mode + # cannot merge here and silently break the published skill. Nothing else tests + # that seam — `diagnose.feature` deliberately asserts only the SHAPE of a + # diagnosis so it stays host-independent. + # + # The catalog is authoritative in `crates/rocm-core`. When one of these fails, + # the CLI is right and `skills/rocm-doctor/reference.md` is what changes. + # + # Scope is deliberately narrow. Claims already proven elsewhere are NOT + # restated here: the exit-code and confirm-before-mutating contract is unit + # tested in `crates/rocm-core/src/fix.rs` (where a declined consent or a failed + # write can be driven directly), the `status` enum in `examine.rs`, and the + # WSL2 out-of-scope rule in `diagnose.rs`. This feature covers what only a real + # binary can show: that the documented catalog and the shipped one agree, and + # that the JSON an agent reads is actually plumbed through. + # + # Every scenario is a query: no GPU, no serve, no download, no mutation — so + # they all run on the blocking mock lane and need no capability tags. + + @id:skill-catalog-ids-match-cli + Scenario: 1 - Every remediation the skill documents is one the CLI still offers + Given the ROCm Doctor skill as it is published + When an agent asks the CLI which remediations it knows + Then the skill and the CLI describe the same set of remediations + + @id:skill-auto-fix-set-matches-cli + Scenario: 2 - The skill agrees with the CLI about which remediations the CLI will run itself + Given the ROCm Doctor skill as it is published + When an agent asks the CLI which remediations it knows + Then the skill and the CLI agree on which ones the CLI applies without help + + @id:skill-os-scope-matches-cli + Scenario: 3 - The skill agrees with the CLI about which machines each remediation applies to + Given the ROCm Doctor skill as it is published + When an agent asks the CLI which remediations it knows + Then the skill and the CLI agree on which machines each remediation is for + + @id:skill-diagnosis-shape-is-readable + Scenario: 4 - A diagnosis hands the agent everything the skill tells it to read + Given a user who reports a recognised ROCm failure + When an agent asks the CLI to diagnose that report for tooling + Then the diagnosis carries the confidence thresholds the skill reasons about + And every cause it offers carries a title, a confidence, its evidence, and a plan + + @id:skill-examine-verdict-is-known + Scenario: 5 - Inspecting the machine returns a verdict the skill knows how to read + When an agent inspects the machine for tooling + Then the inspection succeeds whatever it finds + And its verdict is one the skill accounts for + + @id:skill-wrong-os-fix-declined + Scenario: 6 - A remediation meant for a different machine is declined without changing anything + Given an agent that picked a remediation meant for a different kind of machine + When the agent asks the CLI to apply that remediation + Then the CLI declines because it does not apply here + And no managed state is written diff --git a/tests/e2e-cucumber/tests/e2e.rs b/tests/e2e-cucumber/tests/e2e.rs index 4691cb87..0aa3d93d 100644 --- a/tests/e2e-cucumber/tests/e2e.rs +++ b/tests/e2e-cucumber/tests/e2e.rs @@ -21,6 +21,7 @@ mod e2e { pub mod examine_steps; pub mod runtime_steps; pub mod serving_steps; + pub mod skill_steps; pub mod tui_driver; } @@ -70,6 +71,10 @@ pub struct E2eWorld { /// launch step knows to pass `--chat-mock` (deterministic offline agent, no /// endpoint detection) instead of driving the real detection/consent path. pub chat_use_mock: bool, + /// Raw text of `skills/rocm-doctor/reference.md`, loaded by the rocm-doctor + /// skill scenarios. That document is the EXPECTED-value fixture for the + /// skill↔CLI contract checks — see `e2e::skill_steps`. + pub skill_reference: Option, } /// Resolve a CI-provided shared-directory env var to a validated, existing path. @@ -174,6 +179,7 @@ impl Default for E2eWorld { expect_xfail: false, tui: None, chat_use_mock: false, + skill_reference: None, } } } diff --git a/tests/e2e-cucumber/tests/e2e/skill_steps.rs b/tests/e2e-cucumber/tests/e2e/skill_steps.rs new file mode 100644 index 00000000..a85b34ff --- /dev/null +++ b/tests/e2e-cucumber/tests/e2e/skill_steps.rs @@ -0,0 +1,404 @@ +// Copyright © Advanced Micro Devices, Inc., or its affiliates. +// +// SPDX-License-Identifier: MIT + +//! Contract steps for the `rocm-doctor` skill (`skills/rocm-doctor/`). +//! +//! The skill is a thin driver: the probe, the closed catalog and the fixes all +//! live in `crates/rocm-core` and ship with the binary. What the skill owns is a +//! set of literal claims about how that binary behaves. These steps drive the +//! real `rocm` through the sequence the skill prescribes and check the claims +//! still hold. +//! +//! `reference.md` is read as the EXPECTED-value fixture. That is a deliberate, +//! narrow exception to the suite's black-box rule: nothing is imported from the +//! rocm-cli codebase — a documentation artifact is read as test data, and that +//! artifact is the thing under test. + +use std::collections::BTreeMap; +use std::path::PathBuf; + +use cucumber::{given, then, when}; + +use crate::E2eWorld; + +/// Same symptom the diagnose steps use: it keys off a `LINUX_AND_WINDOWS` +/// checker and so renders identically on either OS. See the rationale on +/// `diagnose_steps::KNOWN_SYMPTOM`. +const KNOWN_SYMPTOM: &str = "HSA_STATUS_ERROR_INVALID_ISA"; + +/// The four ids the skill names as the ones the CLI will apply itself. Spelled +/// out here so a rename in the catalog fails loudly rather than being absorbed +/// by a set comparison that only ever sees the doc and the CLI agree. +const AUTO_APPLICABLE: [&str; 4] = [ + "fix-2-unset-override", + "fix-4-render-group", + "fix-6-path", + "fix-9-igpu-dgpu", +]; + +/// Verdicts `rocm examine` can report, as enumerated by the skill's reference. +const KNOWN_VERDICTS: [&str; 5] = ["ok", "no-amd-gpu", "wsl", "unsupported-os", "degraded"]; + +/// Confidence thresholds the skill's workflow reasons about ("`score >= 75` = +/// high confidence; `50-74` = likely"). +const MIN_SCORE_FOR_MATCH: i64 = 50; +const HIGH_CONFIDENCE_THRESHOLD: i64 = 75; + +/// One catalog row, from either side of the comparison. +#[derive(Debug, PartialEq, Eq)] +struct Remediation { + /// Machines it applies to, normalised to the CLI's spelling + /// (`linux`, `windows`, `linux/windows`). + os_scope: String, + /// Whether the CLI applies it itself, as opposed to printing a plan. + auto: bool, +} + +fn reference_md_path() -> PathBuf { + // /tests/e2e-cucumber -> /skills/rocm-doctor/reference.md + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("..") + .join("..") + .join("skills") + .join("rocm-doctor") + .join("reference.md") +} + +/// Read the closed-catalog table out of the skill's reference doc. +/// +/// Rows look like: +/// `| `fix-1-arch` | both | | | no |` +/// Only rows whose first cell is a backticked `fix-*` id are taken, which skips +/// the header, the separator, and the exit-code table further up the file. +fn parse_reference_catalog(md: &str) -> BTreeMap { + let mut out = BTreeMap::new(); + for line in md.lines() { + let line = line.trim(); + if !line.starts_with('|') { + continue; + } + let cells: Vec<&str> = line.trim_matches('|').split('|').map(str::trim).collect(); + if cells.len() < 5 { + continue; + } + let id = cells[0].trim_matches('`'); + if !id.starts_with("fix-") { + continue; + } + let os_scope = match cells[1] { + "both" => "linux/windows".to_owned(), + other => other.to_owned(), + }; + let auto = match *cells.last().expect("row has cells") { + "yes" => true, + "no" => false, + other => panic!("{id}: unexpected Auto-fix cell {other:?} in reference.md"), + }; + out.insert(id.to_owned(), Remediation { os_scope, auto }); + } + assert!( + !out.is_empty(), + "no catalog rows found in {}", + reference_md_path().display() + ); + out +} + +/// Read the same shape out of `rocm fix`, whose rows look like: +/// ` [ AUTO] [ linux/windows] fix-2-unset-override -- Unset ...` +fn parse_fix_listing(stdout: &str) -> BTreeMap { + let mut out = BTreeMap::new(); + for line in stdout.lines() { + let line = line.trim(); + let Some(rest) = line.strip_prefix('[') else { + continue; + }; + let Some((marker, rest)) = rest.split_once(']') else { + continue; + }; + let auto = match marker.trim() { + "AUTO" => true, + "PRINT-ONLY" => false, + other => panic!("unexpected applicability marker {other:?} in `rocm fix` listing"), + }; + let Some((os_scope, rest)) = rest.trim_start().trim_start_matches('[').split_once(']') + else { + continue; + }; + let id = rest.split_whitespace().next().unwrap_or_default(); + if !id.starts_with("fix-") { + continue; + } + out.insert( + id.to_owned(), + Remediation { + os_scope: os_scope.trim().to_owned(), + auto, + }, + ); + } + assert!( + !out.is_empty(), + "no fix rows parsed out of the `rocm fix` listing:\n{stdout}" + ); + out +} + +/// Both sides of a comparison, or a panic explaining which step is missing. +fn both_sides(world: &E2eWorld) -> (BTreeMap, BTreeMap) { + let doc = world + .skill_reference + .as_ref() + .expect("skill reference not loaded"); + let listing = world + .cli_output + .as_ref() + .expect("no `rocm fix` listing captured"); + (parse_reference_catalog(doc), parse_fix_listing(listing)) +} + +/// The parsed diagnosis report, or a panic quoting what was emitted instead. +fn diagnosis(world: &E2eWorld) -> serde_json::Value { + assert_eq!( + world.cli_rc, + Some(0), + "the skill reads the JSON, not the exit code: diagnose must always exit 0" + ); + let output = world.cli_output.as_ref().expect("no diagnose output"); + serde_json::from_str(output).expect("diagnose --json did not emit valid JSON") +} + +// ── Given ────────────────────────────────────────────────────────── + +#[given("the ROCm Doctor skill as it is published")] +async fn load_skill_reference(world: &mut E2eWorld) { + let path = reference_md_path(); + let text = std::fs::read_to_string(&path) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", path.display())); + world.skill_reference = Some(text); +} + +#[given("a user who reports a recognised ROCm failure")] +async fn user_reports_known_failure(world: &mut E2eWorld) { + world.model_name = Some(KNOWN_SYMPTOM.to_owned()); +} + +#[given("an agent that picked a remediation meant for a different kind of machine")] +async fn agent_picks_wrong_os_fix(world: &mut E2eWorld) { + // Chosen from the OTHER OS's set so the scenario needs no @requires-os tag + // and behaves the same on every lane. + let id = if cfg!(windows) { + "fix-5-amdgpu-load" + } else { + "fix-13-hip-sdk-missing" + }; + world.model_name = Some(id.to_owned()); +} + +// ── When ─────────────────────────────────────────────────────────── + +#[when("an agent asks the CLI which remediations it knows")] +async fn agent_lists_remediations(world: &mut E2eWorld) { + let (stdout, _, rc) = crate::run_rocm(world, &["fix"]); + world.cli_output = Some(stdout); + world.cli_rc = Some(rc); +} + +#[when("an agent asks the CLI to diagnose that report for tooling")] +async fn agent_diagnoses_for_tooling(world: &mut E2eWorld) { + let symptom = world.model_name.clone().expect("no symptom set"); + let (stdout, _, rc) = crate::run_rocm(world, &["diagnose", "--symptom", &symptom, "--json"]); + world.cli_output = Some(stdout); + world.cli_rc = Some(rc); +} + +#[when("an agent inspects the machine for tooling")] +async fn agent_examines_for_tooling(world: &mut E2eWorld) { + let (stdout, _, rc) = crate::run_rocm(world, &["examine", "--json"]); + world.cli_output = Some(stdout); + world.cli_rc = Some(rc); +} + +#[when("the agent asks the CLI to apply that remediation")] +async fn agent_applies_remediation(world: &mut E2eWorld) { + let fix_id = world.model_name.clone().expect("no fix id set"); + let (stdout, stderr, rc) = crate::run_rocm(world, &["fix", &fix_id]); + world.cli_output = Some(stdout); + world.cli_stderr = Some(stderr); + world.cli_rc = Some(rc); +} + +// ── Then ─────────────────────────────────────────────────────────── + +#[then("the skill and the CLI describe the same set of remediations")] +async fn assert_same_ids(world: &mut E2eWorld) { + let (doc, cli) = both_sides(world); + let documented: Vec<&String> = doc.keys().collect(); + let offered: Vec<&String> = cli.keys().collect(); + assert_eq!( + documented, offered, + "the skill's catalog and `rocm fix` disagree.\n\ + The catalog is authoritative in crates/rocm-core — update \ + skills/rocm-doctor/reference.md (and the amd/skills copy) to match the CLI.\n\ + documented: {documented:?}\n\ + offered: {offered:?}" + ); +} + +#[then("the skill and the CLI agree on which ones the CLI applies without help")] +async fn assert_same_auto_set(world: &mut E2eWorld) { + let (doc, cli) = both_sides(world); + for (id, offered) in &cli { + let documented = doc.get(id).unwrap_or_else(|| { + panic!( + "`rocm fix` offers {id}, which skills/rocm-doctor/reference.md does not document" + ) + }); + assert_eq!( + documented.auto, offered.auto, + "{id}: reference.md says auto-applicable={}, `rocm fix` says {}", + documented.auto, offered.auto + ); + } + // Pin the set itself: both files name these four ids in prose, so a rename + // that happened to be applied to reference.md and the CLI together would + // still leave SKILL.md's workflow text stale. + let auto: Vec<&String> = cli + .iter() + .filter(|(_, r)| r.auto) + .map(|(id, _)| id) + .collect(); + assert_eq!( + auto, AUTO_APPLICABLE, + "the set of fixes the CLI applies itself changed; the skill names these four in prose" + ); +} + +#[then("the skill and the CLI agree on which machines each remediation is for")] +async fn assert_same_os_scope(world: &mut E2eWorld) { + let (doc, cli) = both_sides(world); + for (id, offered) in &cli { + let documented = doc + .get(id) + .unwrap_or_else(|| panic!("`rocm fix` offers {id}, undocumented in reference.md")); + assert_eq!( + documented.os_scope, offered.os_scope, + "{id}: reference.md scopes it to {:?}, `rocm fix` to {:?}", + documented.os_scope, offered.os_scope + ); + } +} + +#[then("the diagnosis carries the confidence thresholds the skill reasons about")] +async fn assert_thresholds(world: &mut E2eWorld) { + let report = diagnosis(world); + for (field, expected) in [ + ("min_score_for_match", MIN_SCORE_FOR_MATCH), + ("high_confidence_threshold", HIGH_CONFIDENCE_THRESHOLD), + ] { + let actual = report + .get(field) + .and_then(serde_json::Value::as_i64) + .unwrap_or_else(|| panic!("diagnose JSON has no numeric '{field}'")); + assert_eq!( + actual, expected, + "{field} changed; the skill quotes {expected} in its workflow text" + ); + } +} + +#[then("every cause it offers carries a title, a confidence, its evidence, and a plan")] +async fn assert_match_shape(world: &mut E2eWorld) { + let report = diagnosis(world); + let matched = report + .get("matched") + .and_then(serde_json::Value::as_array) + .expect("diagnose JSON has no 'matched' array"); + // Deliberately not asserting `matched` is non-empty: on a host the catalog + // rules out of scope (WSL2) an empty list is the correct answer, and + // scenario 5 covers that branch. What must hold is that anything offered is + // fully readable by an agent following the skill. + for cause in matched { + for field in ["id", "title", "score", "evidence", "fix"] { + assert!( + cause.get(field).is_some(), + "a matched cause is missing '{field}', which the skill reads:\n{cause:#}" + ); + } + let fix = &cause["fix"]; + for field in [ + "fix_id", + "summary", + "commands", + "verify", + "notes", + "needs_sudo", + "needs_reboot", + "needs_relogin", + "auto_applicable", + ] { + assert!( + fix.get(field).is_some(), + "a fix plan is missing '{field}', which the skill reads:\n{fix:#}" + ); + } + } +} + +#[then("the inspection succeeds whatever it finds")] +async fn assert_examine_exits_zero(world: &mut E2eWorld) { + assert_eq!( + world.cli_rc, + Some(0), + "the skill reads `status`, not the exit code: examine must always exit 0" + ); +} + +#[then("its verdict is one the skill accounts for")] +async fn assert_known_verdict(world: &mut E2eWorld) { + let output = world.cli_output.as_ref().expect("no examine output"); + let report: serde_json::Value = + serde_json::from_str(output).expect("examine --json did not emit valid JSON"); + let status = report + .get("status") + .and_then(serde_json::Value::as_str) + .expect("examine JSON has no 'status'"); + assert!( + KNOWN_VERDICTS.contains(&status), + "examine reported {status:?}, which the skill does not account for \ + (it documents {KNOWN_VERDICTS:?})" + ); +} + +#[then("the CLI declines because it does not apply here")] +async fn assert_not_applicable(world: &mut E2eWorld) { + assert_eq!( + world.cli_rc, + Some(3), + "a fix scoped to another OS must exit 3 (not applicable on this host)" + ); +} + +#[then("no managed state is written")] +async fn assert_no_managed_state(world: &mut E2eWorld) { + // Same definition of "changed nothing" the fix-preview scenario uses: + // incidental dirs (logging init) don't count, managed state does. + let root = world + .isolated_root + .as_ref() + .expect("scenario has no isolated root") + .path(); + for managed in [ + root.join("data").join("runtimes"), + root.join("data").join("services"), + root.join("config"), + ] { + let touched = managed.read_dir().is_ok_and(|mut d| d.next().is_some()); + assert!( + !touched, + "a declined fix wrote managed state at {}", + managed.display() + ); + } +} From 529f7747a91589d24dc7fcda82d8cf50203dd729 Mon Sep 17 00:00:00 2001 From: Eugene Volen Date: Wed, 29 Jul 2026 11:28:56 +0000 Subject: [PATCH 3/3] fix(diagnose): report fix-9 as auto-applicable on Linux MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `check_9_igpu_dgpu_collision` built its Fix per-OS and the two branches disagreed: Windows said auto_applicable true, Linux said false, while fix::RECIPES — what `rocm fix` actually dispatches on — says true for both. It was the only OS-split checker whose branches diverged. The consequence is a CLI that contradicts itself. An agent following the rocm-doctor skill branches on `fix.auto_applicable` from `diagnose --json` to decide whether to offer to run a fix or merely print the plan; on Linux it was told fix-9 was print-only, and `rocm fix fix-9-igpu-dgpu --device-index N` then prompted and appended to the shell rc anyway. Guard it where the divergence lives: a test that runs `diagnose` for both OS families and asserts every emitted fix agrees with the catalog. Verified it fails with the old value and passes with the new one. Also drop the unreachable routing arms. `route_when_no_match` matched on lemonade / ollama / lm-studio, and `upstream_tracker` carried those plus amdgpu-install, but `Examination::probe` only ever reports skipped, pytorch, llama-cpp or unknown — so those arms could never be taken and described a capability the CLI does not have. A companion test pins the reachable target set so a re-added arm has to arrive with a probe that reaches it. Signed-off-by: Eugene Volen --- crates/rocm-core/src/diagnose.rs | 123 +++++++++++++++++++++++++++++-- crates/rocm-core/src/fix.rs | 12 +++ 2 files changed, 127 insertions(+), 8 deletions(-) diff --git a/crates/rocm-core/src/diagnose.rs b/crates/rocm-core/src/diagnose.rs index 77cc93d6..0dd3c8bd 100644 --- a/crates/rocm-core/src/diagnose.rs +++ b/crates/rocm-core/src/diagnose.rs @@ -79,14 +79,17 @@ impl DiagnoseReport { } /// Upstream tracker for a framework key. +/// Upstream tracker for a routing target. +/// +/// Only the targets [`route_when_no_match`] can actually produce are listed. +/// Arms for lemonade / ollama / lm-studio / amdgpu-install were unreachable — +/// the host probe never reports those frameworks — so they described a +/// capability the CLI does not have. Routing an app that merely appears in the +/// symptom text is the caller's job, not the probe's. fn upstream_tracker(target: &str) -> &'static str { match target { "pytorch" => "https://github.com/pytorch/pytorch/issues (tag with rocm label)", "llama-cpp" => "https://github.com/ggml-org/llama.cpp/issues", - "lemonade" => "https://github.com/lemonade-sdk/lemonade/issues", - "ollama" => "https://github.com/ollama/ollama/issues", - "lm-studio" => "https://lmstudio.ai/docs/app (use in-app support; no public repo)", - "amdgpu-install" => "https://repo.radeon.com (raise via your AMD support contact)", _ => "https://github.com/ROCm/ROCm/issues", } } @@ -926,7 +929,11 @@ fn check_9_igpu_dgpu_collision(e: &Examination, symptom: &str) -> Diagnosis { "# Persist in your shell rc or your launch script.".to_owned(), ], fix_id: "fix-9-igpu-dgpu".to_owned(), - auto_applicable: false, + // `rocm fix fix-9-igpu-dgpu --device-index N` really does apply this + // on Linux (see `fix::run_hip_visible_devices_linux`), so the + // diagnosis must not tell an agent otherwise — it branches on this + // flag to decide whether to offer to run the fix or just print it. + auto_applicable: true, verify: "HIP_VISIBLE_DEVICES=1 python -c \"import torch; print(torch.cuda.device_count())\"".to_owned(), notes: vec![note], ..Fix::default() @@ -1280,13 +1287,17 @@ fn run_all_checks(e: &Examination, symptom: &str) -> Vec { results } +/// Where to send a user when nothing in the catalog matched. +/// +/// Keyed off the *host-detected* framework, which `Examination::probe` only +/// ever sets to `pytorch`, `llama-cpp`, `unknown` or `skipped` — so those two +/// named arms plus the ROCm-core default are the whole reachable set. If a probe +/// for another framework is added, add its arm here and in [`upstream_tracker`]; +/// `routing_targets_cover_every_framework_the_probe_reports` will flag it. fn route_when_no_match(e: &Examination) -> Route { let target = match e.framework.as_str() { "pytorch" => "pytorch", "llama-cpp" => "llama-cpp", - "lemonade" => "lemonade", - "ollama" => "ollama", - "lm-studio" => "lm-studio", _ => "rocm-core", }; Route { @@ -1473,6 +1484,102 @@ mod tests { assert_eq!(top.score, 100); } + /// Routing must stay defined for every framework the probe can report, and + /// must not claim targets it can never reach. + /// + /// `Examination::probe` sets `framework` to exactly these four values (see + /// `examine.rs`). The catalog docs previously advertised lemonade / ollama / + /// lm-studio routing that no probe could ever trigger; this pins the + /// reachable set so a re-added arm has to come with a probe that reaches it. + #[test] + fn routing_targets_cover_every_framework_the_probe_reports() { + let mut targets = Vec::new(); + for framework in ["skipped", "pytorch", "llama-cpp", "unknown"] { + let e = Examination { + framework: framework.to_owned(), + ..Examination::default() + }; + let route = route_when_no_match(&e); + assert!( + route.url.starts_with("http"), + "{framework}: routed to a non-URL {:?}", + route.url + ); + targets.push(route.target); + } + targets.sort_unstable(); + targets.dedup(); + assert_eq!( + targets, + vec!["llama-cpp", "pytorch", "rocm-core"], + "the reachable routing targets changed; update the docs in \ + skills/rocm-doctor/reference.md (and the amd/skills copy) to match" + ); + } + + /// Every diagnosis must agree with the fix catalog about whether the CLI + /// can apply the fix itself. + /// + /// An agent following the rocm-doctor skill branches on `auto_applicable` + /// from `diagnose --json` to decide whether to offer to run `rocm fix` or + /// merely print the plan, while `fix::apply` dispatches on `RECIPES`. When + /// the two disagree the CLI contradicts itself: fix-9 advertised + /// `auto_applicable: false` on Linux and then applied itself anyway. + /// + /// Both OS families are exercised because the checkers build their `Fix` + /// per-OS and only one branch is taken per run — the Linux branch was the + /// stale one, and a Linux-only test would not have seen it. + #[test] + fn every_diagnosis_agrees_with_the_fix_catalog_on_auto_applicability() { + for os in ["linux", "windows"] { + let mut e = Examination { + os_family: os.to_owned(), + ..Examination::default() + }; + // Trip several checkers at once so this covers more of the catalog + // than a single fix. + e.in_render_group = Some(false); + e.in_video_group = Some(false); + e.has_apu = true; + e.has_discrete_amd = true; + e.gpus = vec![ + Gpu { + gfx_target: "gfx1103".to_owned(), + is_amd: true, + is_apu: Some(true), + ..Gpu::default() + }, + Gpu { + gfx_target: "gfx1100".to_owned(), + is_amd: true, + is_apu: Some(false), + ..Gpu::default() + }, + ]; + + let report = diagnose(&e, "torch crashes with a segfault"); + assert!( + !report.matched.is_empty(), + "{os}: expected at least one diagnosis to compare against the catalog" + ); + for d in &report.matched { + let fix = d + .fix + .as_ref() + .unwrap_or_else(|| panic!("{os}/{}: matched with no fix", d.id)); + let catalog = crate::fix::auto_applicable_for(&fix.fix_id).unwrap_or_else(|| { + panic!("{os}/{}: emitted a fix-id not in the catalog", fix.fix_id) + }); + assert_eq!( + fix.auto_applicable, catalog, + "{os}/{}: diagnose reports auto_applicable={}, but the fix catalog \ + (what `rocm fix` actually does) says {catalog}", + fix.fix_id, fix.auto_applicable + ); + } + } + } + #[test] fn igpu_dgpu_collision_fires_for_rdna3_apu_plus_discrete() { // End-to-end guard for EAI-7412: a gfx1103 (Phoenix APU) + gfx1100 diff --git a/crates/rocm-core/src/fix.rs b/crates/rocm-core/src/fix.rs index e2ba7f10..8d88c802 100644 --- a/crates/rocm-core/src/fix.rs +++ b/crates/rocm-core/src/fix.rs @@ -380,6 +380,18 @@ const fn current_os() -> &'static str { } } +/// Whether the CLI will apply `fix_id` itself, or `None` if it isn't a known +/// fix. `RECIPES` is the authority: `apply()` dispatches on it, so this is the +/// value any other surface describing a fix has to agree with. +/// +/// Test-only: the one production consumer is `apply()`, which reads the recipe +/// directly. This exists so `diagnose`'s tests can assert the two surfaces +/// agree without exposing `RECIPES`. +#[cfg(test)] +pub(crate) fn auto_applicable_for(fix_id: &str) -> Option { + find_recipe(fix_id).map(|r| r.auto_applicable) +} + /// List every fix-id (id, kind, OS scope, title). #[must_use] pub fn list_recipes() -> String {