ci: grade the rocm-doctor skill with skillscope - #356
Conversation
Exit codes 1, 4 and 5 were asserted nowhere, and no test reached a runner that writes to disk. The two that looked like coverage did not: the dry-run 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 runs. Split consent_without_prompt out of confirm, and let pin_device_in_rc_file take the rc path plus an injected consent verdict, so the mutation path is reachable from a test without touching $HOME or stdin. Behaviour is unchanged. Ten tests then cover the consent gate, both mutation outcomes, and the dry-run short-circuit. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
check_9_igpu_dgpu_collision reported auto_applicable: false on Linux while fix::RECIPES -- what `rocm fix` dispatches on -- says true. It was the only OS-split checker whose branches diverged, and an agent branches on that field, so it was told fix-9 was print-only while `rocm fix` would happily apply it. route_when_no_match also 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. Those arms were unreachable, so the CLI advertised routing it cannot do. Routing an app that merely appears in the symptom text belongs to the caller, not the probe. Both are pinned: one test runs diagnose for each OS family and asserts every emitted fix agrees with the catalog, another pins the reachable target set. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The rocm-doctor skill is a thin driver: the probe, the closed 15-mode catalog and the fixes all ship inside the `rocm` binary. 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. Because it describes this binary, this repo is its source of truth; the catalog it is published through vendors the folder from here. Nothing tested that seam. diagnose.feature deliberately asserts only the shape of a diagnosis so it stays host-independent, so a renamed fix-id or a 16th failure mode would merge green and silently break the published skill. rocm_doctor_skill.feature parses reference.md's catalog table, its auto-applicable prose, the fields and thresholds it names, the verdicts it enumerates and the trackers it lists, then diffs each against what the CLI really reports. Every scenario is a query -- no GPU, no serve, no download, no mutation -- so they run on the blocking mock lane and need no capability tags. Reading a document as test data is a deliberate, narrow exception to the suite's black-box rule: nothing is imported from the codebase, and the document is the thing under test. The skill folder is excluded from the licence-header check: SKILL.md must open with YAML frontmatter for the skill loader, which pushes any header out of hawkeye's detection window. Verified in both directions -- the folder is exempt, and stripping a header elsewhere still fails the check. skills/** joins the `heavy` paths filter, since reference.md is now a fixture and a skill-only edit has to run the e2e job. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The skill told an agent to route upstream "when `matched` is empty", which `DiagnoseReport::has_match` exists precisely to prevent: several checkers open with a nonzero base score for a merely potentially relevant situation, so a healthy host returns a non-empty `matched` of sub-threshold entries. An agent following that instruction proposes a fix for a machine with nothing wrong and never routes upstream. Both skill files now name `has_match` and gate on it. That gap was invisible to the contract feature because the field check ran one way only -- documented ⊆ emitted -- so a field the CLI emits and the document never mentions could not fail anything. It is now an equality, and `rocm diagnose` emits exactly the six keys the reference names. The route check had the same shape. It compared the CLI's target against every tracker in the Framework routing section, including the three the skill hands over itself, so a documented-but-unreachable CLI target passed silently. The section now separates the two lists and the CLI is held to its own. Scenario 6's Given was inert: `route_when_no_match` is populated whether or not anything matched, so the scenario passed identically for a symptom that did match. It now asserts `has_match` is false first. Where the catalog is out of scope the check still holds, since the catalog is not run at all; a matching symptom can only be produced on a host the catalog covers, so the no-GPU Linux lane is where it bites. Finally, `route_when_no_match`'s comment claimed a test would flag a newly added framework. That test iterates a hand-maintained list and reads nothing from `examine.rs`, so it would not. Reworded to name the three places that must be edited together. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
… holds still Two CI failures, both mine. `rocm_doctor_skill.feature` had no `FEATURE_KEYS` entry, so `feature_files_and_declared_keys_agree` panicked and took `Test (affected crates)`, `windows-build-and-test` and `E2E tests` down with it. Adds the key `skill` and renames the six scenarios to `skill-01`..`skill-06`, which the sibling naming tests require once a file is indexed. The `@id:` tags already carried the prefix. I missed this locally by running the workspace suite with `--exclude e2e-cucumber` -- the crate that holds the test. Scenario 6 then asserted `has_match == false` on the grounds that its symptom carried no catalog keyword. That premise does not survive a real host: `diagnose` scores several checkers from host state alone, and the GitHub-hosted Linux runner ships /etc/modprobe.d/blacklist-radeon-instinct.conf with amdgpu unloaded, which is `fix-5-amdgpu-load` at score 90 no matter what symptom is passed. No symptom can hold that still, so the assertion only encoded a runner's state. It passed on WSL2 because the catalog is skipped there entirely, which is why it looked host-independent. The claim moves to `rocm-core`, where the Examination is constructed instead of probed: a host whose only causes are sub-threshold has a NON-empty `matched` and `has_match` false, and the route still names somewhere to go -- the exact shape that makes gating on emptiness wrong. A companion test asserts a matching symptom flips the flag, so the assertion is not free. What is left in the feature is the half only it can check: whatever target the CLI hands back is one the published document names. The scenario is renamed to say that, rather than to claim a premise it does not establish. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
952903f to
ea5f1d3
Compare
…Linux `reference.md` told an agent that auto fixes "print the exact command, honor --dry-run, refuse on a non-interactive shell without --yes, and confirm before mutating". That is false for fix-2 on Linux. `run_unset_override` splits by platform: the Windows arm takes `FixOptions` and does exactly that, while `run_unset_override_linux` takes no options at all -- it reports where the override is set and which rc files carry it, then stops, because editing a user's dotfiles is not its business. So there is no prompt to answer, nothing for --dry-run to preview, and --yes is never read. An agent following the doc would tell a Linux user a change was previewed that was never going to happen. `auto_applicable` is a flat bool on a const recipe, and making it OS-aware would ripple through the listing, the diagnosis JSON and the catalog row, so the flag keeps its meaning and the docs stop overpromising: the Auto-fix column is now defined as what `rocm fix` actually reports -- the CLI has a runner and will carry it out itself -- and the mutation contract moves to prose that names fix-2-on-Linux as the exception. Pinned so it cannot drift back. `auto_applicable_recipes_have_a_runner` cannot catch this, because fix-2 does have a runner. The rc-file scan is split into `report_persistent_override`, taking its candidate paths the way this PR already made `pin_device_in_rc_file` take its rc path -- deriving them from $HOME would mean mutating a process-global in a parallel test suite. The test asserts the files are byte-identical afterwards rather than asserting an exit code, since a runner that started stripping the line would still return 0; confirmed it fails when the runner is made to mutate. Also drops a stale duplicate doc line left on `upstream_tracker` when the framework-keyed arms were removed. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
The skill's prose is now tested against the binary, but nothing checked the parts an agent runtime reads before it ever gets that far: the frontmatter that decides whether the skill loads at all, and the links it follows into reference.md. A `name` that disagrees with its folder, or a link into a renamed file, fails silently -- the agent simply never uses the skill, or follows the link, finds nothing, and improvises. skillscope is AMD's harness for exactly this. Only its `structural` command runs here: no agent, no API key, and no network beyond the install, so it can gate a merge without ever being wrong for a reason of its own. It checks the frontmatter, the `skill-card.md` sections, every internal reference, and the dataset's coverage bar. `evals/evals.json` is that dataset -- four prompts the skill must answer and two near misses it must decline, over the scope gate, the consent rule and the Phase 0 probe. One case pins the correction this series just made: a vague report where nothing clears the threshold must be routed upstream on `has_match`, not diagnosed from the sub-threshold entries still sitting in `matched`. Federation does not carry `evals/`, so this dataset is ours and the catalog keeps its own. `routing` and `behavioral` stay off: both need an authenticated `claude` CLI and an ANTHROPIC_API_KEY this repo does not have. They are the half of the question that asks whether the skill actually fires, so the dataset is written and checked to make switching them on a workflow change and nothing more. skills/rocm-cli-assistant/ is deliberately out of scope. Despite living under skills/ it is not a published skill: main.rs embeds it verbatim into the chat system prompt with include_str!, so giving it the frontmatter this check requires would inject that frontmatter into a live prompt. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
ea5f1d3 to
a2d814d
Compare
rominf
left a comment
rominf
left a comment
There was a problem hiding this comment.
Reviewed against test/rocm-doctor-skill-contract rather than main, so #149's changes aren't counted against this PR. Noting it's still a draft — reviewing because you asked, not to push it forward before its dependency lands.
The CI shape is safe and I checked the parts that usually go wrong: pull_request rather than pull_request_target, inherited contents: read, no secrets reaching the job, a timeout, fail-closed on grader error. The evals file validates against skillscope's schema and clears the real Tier 0 minimum with 4 positive and 2 negative cases, and both logs_contain literals do appear in the skill, so it isn't vacuous in the ways I could check. .gitignore adds only /.skillscope/. Stacked conduct is right: draft, and the body links #149 with the rebase plan.
Two things need fixing before this is ready, both in the same category — the PR is about making a check real, and in two places it asserts an enforcement that doesn't currently exist.
I'd also fold this into a conversation with #149 rather than treating them separately: that PR deleted its drift-check on the strength of a federation that doesn't cover this repo, and this one documents a gate that isn't in branch protection. Same shape twice.
| }, | ||
| { | ||
| "id": "rocm-under-wsl2", | ||
| "skill_should_trigger": false, |
There was a problem hiding this comment.
I think this case is the wrong shape, and it will read as a skill bug rather than an eval bug when it fires.
Per skillscope's evals.schema.json, skill_should_trigger: false means no skill loads at all, so there's no behavioural phase to grade. But SKILL.md devotes a scope gate and an "Out of scope" section to WSL2 — rocm examine/diagnose detect it and route out, relay that guidance, point at AMD's ROCm-on-WSL guide — and that's behaviour that only happens once the skill has loaded. This case's own note says the skill "declines rather than troubleshoots", which is a description of it firing.
The prompt also matches the frontmatter description almost word for word (torch.cuda.is_available() false, AMD GPU, Linux or Windows), so routing will very likely fire it. Nothing catches the mismatch today because structural never runs the prompts — it'll surface the moment routing is enabled, and the tempting fix at that point is to weaken the skill description, which would be the wrong repair.
Suggest making it a triggering case that grades the decline: skill_should_trigger: true, expected_behavior = state WSL2 is out of scope and point at the ROCm-on-WSL guide, unexpected_behavior = run examine/diagnose/fix or offer any WSL2 troubleshooting. You'd then want a genuine second near-miss to keep the two-negative floor — a pure Windows-driver or NVIDIA-container prompt would do it. cuda-broken-on-nvidia is a clean negative as written; it's only this one.
There was a problem hiding this comment.
Agreed, this was the wrong shape. Flipped rocm-under-wsl2 to skill_should_trigger: true with expected_behavior (state WSL2 is out of scope, point at AMD's ROCm-on-WSL guide) and unexpected_behavior (run examine/diagnose/fix, or offer any WSL2 troubleshooting), matching the skill's own Out-of-scope section almost verbatim. Added a fresh negative, nvidia-container-toolkit-unrelated (a pure NVIDIA container-runtime error with no ROCm/AMD vocabulary), to keep the two-negative floor. Re-ran the pinned skillscope structural command locally against the new file: 12 cases, exit 0.
| uses: amd/skillscope@6dde8e8a34ad5456d3c2ff3418bc81c2cd9d5d69 # v0.1.0 | ||
| with: | ||
| command: structural | ||
| skills: skills/rocm-doctor |
There was a problem hiding this comment.
This names one directory while the comment above and the paths filter both describe the whole of skills/, so a second skill added later is silently never graded.
skillscope's own safety net can't help here: structure.errors() only reports "no skill found" when the configured set is empty, and this still resolves rocm-doctor, so a new skills/foo/ with broken frontmatter passes green. That's the same silent-non-grading failure this PR is built to prevent, just deferred by one skill.
Either pass a glob once rocm-cli-assistant is excluded or moved, or add a one-line guard that fails when ls -d skills/*/ turns up a directory that isn't in the graded list — so adding a skill forces a deliberate CI edit instead of quietly widening the gap.
There was a problem hiding this comment.
Added the cheap guard rather than a glob: a step that lists skills/*/ and fails closed if it finds a directory that's neither graded by the skillscope step nor excluded by design (rocm-cli-assistant, per AGENTS.md #7). A third skill folder now forces a deliberate CI edit instead of passing green ungraded. Left the skillscope 'skills:' input pointed at skills/rocm-doctor specifically, since skillscope grades one directory at a time and rocm-cli-assistant still needs to stay excluded either way.
Address review feedback: the AGENTS.md/ci.yml text claimed skill-evals already blocks merges, when it isn't a required status check yet, and claimed the pinned skillscope action fully covers supply-chain drift, when the composite's own steps and the uvx-resolved harness aren't pinned. Both are now stated accurately instead of asserted. Also: the changes-job `skills` filter now matches its siblings by covering all of `.github/workflows/**` instead of just `ci.yml`; a new guard step fails the job if a skills/ directory shows up that isn't graded or excluded by design, so a second skill can't go ungraded silently; and the WSL2 eval case is reshaped to `skill_should_trigger: true` (the skill loads to decline it, per its own scope gate) with a fresh negative case restoring the two-negative floor. Signed-off-by: Eugene Volen <Eugene.Volen@amd.com>
c7a236c to
88b35c6
Compare
2 participants
What
#149 makes the rocm-doctor skill's prose testable against the binary it drives.
This adds the other half: the parts an agent runtime reads before it gets that
far.
A
namethat disagrees with its folder, adescriptionthat never got written,frontmatter that is not valid YAML — each one stops the skill loading or stops it
being found, and every one of them fails silently. What you see is an agent that
simply never uses the skill. A relative link into a renamed file fails the same
way: the agent follows it, finds nothing, and improvises.
amd/skillscopeis AMD's harness for this.Changes
skill-evalsjob — runs only skillscope'sstructuralcommand: no agent, noAPI key, and no network beyond the install, so it can gate a merge without ever
being wrong for a reason of its own. It checks the frontmatter, the
skill-card.mdsections, every internal markdown reference, and the dataset'scoverage bar. Gated on a new
skillspaths filter, and modelled on the existinglicense-headersjob.evals/evals.json— four prompts the skill must answer and two near misses itmust decline, covering the scope gate, the consent rule and the Phase 0 probe.
Seeded from the dataset
amd/skillsalready grades this skill with, plus onecase that pins the correction #149 makes: a vague report where nothing clears the
threshold must be routed upstream on
has_match, not diagnosed from thesub-threshold entries still sitting in
matched.Federation does not carry a skill's
evals/folder in either direction, so thisdataset is ours and the catalog keeps its own.
Scope
routingandbehavioralare not run. Both need an authenticatedclaudeCLI and an
ANTHROPIC_API_KEY, and this repo has no such secret. They are thehalf of the question that asks whether the skill actually fires — the failure
most skills have — so this job is not a substitute for them. The dataset is
written and structurally checked so switching them on is a workflow change and
nothing more.
skills/rocm-cli-assistant/is deliberately out of scope. Despite the path it isnot a published skill:
apps/rocm/src/main.rsembeds it verbatim into the chatassistant's system prompt with
include_str!, so giving it the YAML frontmatterthis check requires would inject that frontmatter into a live prompt. It would
also need a dataset of its own, for something no agent runtime ever routes to.
Verification
Run against the exact SHA the workflow pins (
6dde8e8a,v0.1.0), with theexact CI arguments:
skillscope structural --skills-dir 'skills/rocm-doctor' --skill-files skill-card.md --skill-sections Description,Owner,License— green, exit 0--external— 9 outbound URLs answered. CI omits it on purpose: a rate-limitedhost is a fact about the run, not a broken link.
Checked that it is not a vacuous green — each of these exits 1 with a specific
message, and the restored state exits 0:
name: rocm-doctorrnamedisagrees with the folderskill_should_trigger: false[reference.md](refrence.md)Also confirmed the action's launcher accepts a commit-SHA pin
(
github.action_ref→resolve_version.py, whose ref pattern takes a 40-hexSHA), and that
/.skillscope/is ignored.