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/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 67602038..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 { @@ -476,13 +488,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 +860,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 +897,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 +1090,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" + ); + } } 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() + ); + } +}