[johnl-amd]

A machine.yml still holding runner_type fails with:

error: .../evals/machine.yml: unknown key(s): runner_type.
A machine.yml holds only labels and os.

True, but it leaves the reader to work out on their own that the key used to be
valid and that labels took its place. Two files in amd/skills
(hyperloom-workload-optimizer and serving-llms-on-instinct) are in exactly
that state today, so this is the first thing anyone pointing skillscope at the
catalog sees.

Now:

error: .../evals/machine.yml: unknown key(s): runner_type.
A machine.yml holds only labels and os.
  `runner_type` is replaced by `labels`, which names the runner labels the work
  needs rather than a hardware class something else has to resolve. A skill that
  asked for `runner_type: instinct` now says, for example, `labels: [mi300x]`.

Why explain rather than accept

Accepting the old key as an alias would need a class-to-label mapping, which is
the second file this format deliberately does not have. machine_plan's own
docstring makes the case:

Labels rather than a class name, because a class name has to be defined
somewhere and that somewhere is a second file to keep in step.

So the old key is explained, not translated. Valid files behave exactly as
before; only the error path changes.

Keyed by a table rather than a special case, so the next rename is one entry.

[nowycondro]

Review against ai-instruction-quality

Reviewed with the ai-instruction-quality skill from silogen/dev-tooling (the skill named in the request), worked top-down through its references/quality-checklist.md.

A scope note first, so the framing is visible. That skill's own description scopes it to AI instruction files — SKILL.md, references, scripts, cards, CLAUDE.md, AGENTS.md, agents, commands. This PR is Python in a tool that validates skill catalogs, not an instruction file. I applied the parts that genuinely map to what the diff changes — the error string is instruction text read by whoever (or whatever) has to fix a machine.yml, and the module comment and machine_plan docstring are instructions to future maintainers — plus the checklist's [cross] docs-vs-code and script-behaviour items. The rest is ordinary code review. Where the skill had nothing to say, I have not invented a finding to fill the space.

Verdict: no Tier 1 (blocking) defect. Two Tier 2 findings and two Tier 3 notes. Behaviour on valid files is unchanged, the whole test suite passes, and the design instinct here is right. The substantive gap is that the change solves one instance of a problem the repo's own tests say has five.

---

What this gets right

The checklist's content items land squarely in this PR's favour, and they are worth naming because they are the hard part:

• "Explain Why — the reason is a mechanism, never a citation." The new text says why the model changed — labels names what GitHub actually matches a runner on, so there is no class-to-label mapping in a second file to keep in step — rather than just asserting a rename. A reader who hits this error on some other retired key can extrapolate from that. This is the checklist item most often failed, and it passes cleanly.
• "Solve, don't punt." The error handles an expected failure in place with a useful message instead of leaving the reader to reverse-engineer the schema.
• "Avoid time-sensitive content" — superseded guidance lives in a named table rather than a dated aside, which is the ## Old patterns shape applied to a validator.
• "Use concrete examples" — runner_type: instinct → labels: [mi300x] makes the shape unambiguous.
• Correctly refuses to auto-translate. Accepting the old key as an alias would need exactly the mapping file the format exists to avoid, and the checklist's "nothing buried in a script that the agent should decide" points the same way: a silent translation is a decision the reader cannot see or override, and a wrong one puts a skill on the wrong hardware.
• "Filtering errs permissive" — a plain typo (typo_key) still gets the short message and is not dressed up as a migration. Verified.
• No invisible Unicode; no line-length violations; RETIRED_MACHINE_KEYS is disjoint from MACHINE_KEYS.

---

Tier 2 — Important

1. Four of the five known retired keys still get the bare error [cross]

Checklist: "Parallel areas have symmetric coverage — adding a rule to one asks whether its siblings need it."

tests/test_skillscope.py:401 names the retired set explicitly, and it is not one key:

runner_type, runner, gate, environment, and reason all used to live here.

RETIRED_MACHINE_KEYS holds one of those five. I ran each against machine_plan on this branch:

runner_type: instinct            -> explained
gate: enable_mi_ci               -> "unknown key(s): gate. A machine.yml holds only labels and os."
environment: behavioral-instinct -> "unknown key(s): environment. ..."
reason: because                  -> "unknown key(s): reason. ..."
runner: [a, b]                   -> "unknown key(s): runner. ..."

The realistic input is worse than any single key, because an old-format file carried several at once:

error: .../machine.yml: unknown key(s): environment, gate, reason, runner_type.
A machine.yml holds only labels and os.
  `runner_type` is replaced by `labels`, ... `labels: [mi300x]`.

The reader learns what replaced one key of four and is left in exactly the state this PR opens by calling out for the other three — now with the added implication that the silent ones are different in kind from the explained one, which they are not.

The four are also the more confusing half. labels is at least a visible successor to runner_type; gate, environment and reason have no successor key at all — they moved out of the file and into the repo's workflow. machine.schema.json already states that ("the pull-request label rationing that pool, the environment holding its credentials — belongs to the repo and is passed to skillscope by the workflow, not restated here"), so the explanation exists and simply is not wired to the error path. The PR's own argument — "the next rename is one entry" — makes the case: the table is the right structure, and four entries it should already hold are missing.

Suggested, in the same shape as the existing entry:

"gate": (
    "no longer part of this file. Which pull-request label rations a scarce "
    "pool is the repo's decision, passed to skillscope by the workflow"
),
"environment": (
    "no longer part of this file. Which environment's credentials pay for a "
    "scoped run is the repo's decision, passed to skillscope by the workflow"
),
"reason": "no longer read. The labels say what the work needs; prose above them says why",
"runner": "replaced by `labels`, which adds to the base labels the workflow supplies",

2. The stated motivating case no longer exists [cross]

The PR description says:

Two files in amd/skills (hyperloom-workload-optimizer and serving-llms-on-instinct) are in exactly that state today, so this is the first thing anyone pointing skillscope at the catalog sees.

That is not true as of this review, and was not true when the PR was opened. Both files were migrated in amd/skills commit 52a827d6 ("Replace built-in harness with amd/skillscope harness", #206) on 2026-09-08. This PR was opened 2026-09-11, three days later. All four machine.yml files in amd/skills@main are valid today:

skills/hyperloom-workload-optimizer/evals/machine.yml   os: [Linux]  labels: [mi300x, gpu, rocm]
skills/serving-llms-on-epyc/evals/machine.yml           os: [Linux]
skills/serving-llms-on-instinct/evals/machine.yml       os: [Linux]  labels: [mi300x, gpu, rocm]
skills/tracelens-analysis-orchestrator/evals/machine.yml os: [Linux]

(serving-llms-on-instinct/evals/machine.yml at 52a827d6~1 was indeed runner_type: instinct, so the observation was accurate when made — it just does not describe the catalog any more.)

This does not make the change wrong. Forks, unmerged branches, and anyone carrying an old file locally still hit it, and the checklist's point about a rule only having force where it loads applies to an error message too. But the urgency claim is what a reviewer weighs the change on, so the description should be corrected rather than left to be discovered — and with the catalog already clean, this is a change with no in-tree case exercising it, which is worth saying out loud.

---

Tier 3 — Polish and signals

3. The example is the right scenario with the wrong answer [file]

Checklist: "Examples are concrete and accurate." The message says a skill that asked for runner_type: instinct "now says, for example, labels: [mi300x]". The one real instance of that exact migration in the catalog resolved runner_type: instinct to labels: [mi300x, gpu, rocm], and docs/authoring-evals.md shows labels: [mi300x, gpu]. Three renderings of the same example, and the error message picks the thinnest one.

"for example" carries some of the weight, and the skill does allow a literal where the value is illustrating a format. But the risk is specific rather than stylistic: the message reads as a 1:1 substitution — the very translation the new comment correctly says cannot be made — and a reader who copies labels: [mi300x] for a ROCm workload lands on a pool that may not carry rocm. Matching the real migration (labels: [mi300x, gpu, rocm]) costs nothing and makes the "this is a change of model, not a rename" point self-evidently.

4. The continuation line collides with the bullet indent [cross]

datasets.structural_errors collects this message into the error list and cli._report_failures prints each as - {err}. The embedded \n puts the explanation at column 2 — the same column as the - marker, not the column 4 where the error text starts. Actual output:

Structural checks failed:
  - /path/evals/machine.yml: unknown key(s): runner_type. A machine.yml holds only labels and os.
  `runner_type` is replaced by `labels`, ... `labels: [mi300x]`.

In a list of several failures the explanation reads as a stray entry rather than as a continuation of the one above. \n (four spaces) would hang it under the text. Cosmetic, and it does not affect the direct machine_plan path where the message is already well-formed.

5. Nothing holds the table to the retired set [file]

test_documented_keys_match_the_parser is a good pattern — it pins machine.schema.json to MACHINE_KEYS so the two cannot drift. There is no equivalent tying RETIRED_MACHINE_KEYS to the retired keys enumerated in test_a_retired_key_is_rejected_rather_than_ignored, so the two lists can diverge exactly as they have. If finding 1 is addressed, hoisting that literal tuple to a module-level constant in the test file and asserting both tests read from it would keep them in step. Low cost, and it is the mechanism that would have caught this.

The new test itself is fine but narrow: assertIn("runner_type") and assertIn("`labels`") do discriminate against the old message (which had no backticks around labels), though neither asserts the replacement relationship the test is named for. assertIn("replaced by", message) would.

---

Checklist items I could not verify

Recorded per the checklist's evidence discipline rather than ticked:

• [run] Verified. All 227 tests pass on this branch (python3 -m unittest discover -s tests, with pyyaml installed). I also exercised machine_plan and cli._structural_or_exit directly against each retired key and against a plain typo; the outputs quoted above are real, not reasoned.
• [run] Not verified. Behaviour in real CI — whether the multi-line error renders as described in GitHub Actions log output, where folding and annotation parsing may differ from a plain terminal.
• [cross] Not verified. Whether any consumer outside amd/skills and this repo parses skillscope's structural-error output as one-error-per-line. If something does, finding 4 stops being cosmetic. I found no such consumer in either repo.

Summary

The design is right and the reasoning in the new comment is the best part of the diff — it explains a mechanism rather than asserting a rename, which is the thing that lets a reader handle a case the author did not foresee. Two things to settle before merge: fill in the other four retired keys the repo's own tests already name (finding 1), and correct the PR description, since the catalog it cites was cleaned up three days before this branch was opened (finding 2). Neither is a correctness bug and nothing here blocks.

---

🤖 Automated review by agent-hub — ai-instruction-quality on AMD AgentHub.
Model claude-opus-5[1m] · took 5m · diff 58660e9ccd57 · run
Not a human review. Reply here if a finding is wrong.

[nowycondro]

🤖 Automated review · review-code · head c5260efb

Review — no blocking findings

Full review of the whole change.
No ticket named; PR description only.

Blocking

None

Non-blocking

• Only 1 of the 5 historically-retired machine.yml keys gets the new explanation — skillscope/datasets.py:546-553, skillscope/datasets.py:613-623, tests/test_skillscope.py:402-415
RETIRED_MACHINE_KEYS holds only runner_type. The repo's own pre-existing test, TestMachineRejections.test_a_retired_key_is_rejected_rather_than_ignored, documents that runner_type, runner, gate, environment, and reason "all used to live here." I reproduced this directly: feeding machine_plan a file with all five keys still raises unknown key(s): environment, gate, reason, runner, runner_type... with the \n key is ... detail appended for runner_type only — the other four get no explanation. Nothing ties RETIRED_MACHINE_KEYS to that test's list, so the two can silently diverge further. The PR's own description frames this as deliberately scoped to the one currently-live rename ("so the next rename is one entry"), which is why this is non-blocking rather than a defect, but given the table-driven mechanism now exists, the other four are a natural, low-cost follow-up.
  Confidence 75 · architectural · Fix: add entries for runner, gate, environment, reason (the schema's own prose already explains where gate/environment moved to), and consider hoisting the five-key list in the test file to a shared constant both tests read from.

• The appended explanation misaligns under the CLI's bulleted failure list — skillscope/datasets.py:616-618, skillscope/cli.py:119-122
machine_plan embeds \n (2 spaces) before each retired-key explanation. cli._report_failures renders each collected error as f" - {err}", which puts the first line's text at column 4 but leaves the embedded continuation at column 2 — under the - bullet marker rather than under the text above it. I verified this by running a multi-key machine.yml through structural_errors() and rendering it the way _report_failures does; the explanation prints as a visually separate bullet-like line rather than a continuation. Does not affect the direct machine_plan exception message (which reads fine on its own), only the skillscope structural bulleted-list path.
  Confidence 80 · mechanical · Fix: use \n (4 spaces) so the continuation hangs under the error text instead of the bullet marker.

Decisions for the author

None

Positive signals

• A plain typo in machine.yml still gets the short, unembellished message (the detail is only appended for keys actually present in RETIRED_MACHINE_KEYS), so the new code doesn't overclaim on inputs it wasn't designed for.
• Keying the explanation by a {key: prose} table rather than an if key == "runner_type" special case is the right shape for the stated goal of making "the next rename... one entry."

Deployment notes

None

What this covered

Single-pass review (2 changed files, 34 added lines in one coupled region — small enough for one worker to read completely rather than fanning out). Read skillscope/datasets.py in full, the full _report_failures/_structural_or_exit section of skillscope/cli.py, skillscope/schema/machine.schema.json in full, the relevant section of docs/authoring-evals.md, and the complete TestSchemaStaysInSyncWithParser/TestMachineSchema/TestMachineRejections/TestConfig test classes plus the file's imports and Repo/tier0_dataset helpers in tests/test_skillscope.py (did not read the remainder of that 2519-line file's unrelated test classes, e.g. routing/behavior/credentials/structure, which this diff does not touch). Diffed 0f734b2d...c5260efb (PR-open-time base, confirmed as the current merge-base). Ran the full suite (227 tests, all passing) and independently reproduced both findings above by executing machine_plan/structural_errors against a multi-retired-key machine.yml. Checked CI: all 5 check runs green at the reviewed head sha; no ticket is named by the PR. Noted but not a finding: origin/main has advanced past this branch's base (adding a third sandbox machine key); git merge-tree shows this still merges cleanly with no conflict.

Did not run: agent-instruction adherence (no CLAUDE.md/AGENTS.md in this repo); prior-changes pass (no accessible earlier review comments on this file's history beyond this PR's own thread, which per method is out of scope for a blind pass anyway).

Not reconciled against any discussion on this PR, including its existing review thread.

---

Not a human review. This is an automated code review; please verify findings independently before acting on them.

[johnl-amd]

Review — needs work

Full review of the whole change (2 files, +34/-0).
No ticket named. The description scopes this fix to the runner_type case the author observed in the wild (two files in a related skills catalog still using the retired key); it does not claim to cover every historically-retired key, so the finding below is a completeness gap against this repo's own test suite, not a mismatch with the PR's stated claims.

Blocking

• RETIRED_MACHINE_KEYS explains only 1 of the 5 keys this codebase's own tests already document as retired — skillscope/datasets.py:546-553, tests/test_skillscope.py:402-415
tests/test_skillscope.py:402-415 (TestMachineRejections.test_a_retired_key_is_rejected_rather_than_ignored, present since the repo's first commit) states in its own comment that "runner_type, runner, gate, environment, and reason all used to live here" and asserts all five are rejected as unknown keys. The new RETIRED_MACHINE_KEYS table adds an explanation for runner_type only. A machine.yml with gate: enable_mi_ci, environment: behavioral-instinct, reason: because, or runner: [a, b] still gets the bare "unknown key(s)" message the diff's own comment says is insufficient ("runner_type ... leaves the reader to work out on their own ... that the key was replaced and by what" — equally true of the other four). Confirmed by reading the test directly and running python -m unittest tests.test_skillscope.TestMachineRejections against the PR head (passes unchanged, i.e. the gap is real and silent — those four keys still produce no explanation).
  Confidence 85 · architectural · Fix: add entries for runner, gate, environment, and reason to RETIRED_MACHINE_KEYS, or state in a comment why only runner_type currently warrants one.

Non-blocking

None

Decisions for the author

None

Positive signals

• The new test's assertion on the backtick-quoted `labels` is deliberately specific: I reverted the production change by hand and re-ran the test — it fails against the pre-change code, because the old message's "A machine.yml holds only labels and os." contains labels without backticks. A looser assertion (e.g. just checking for runner_type) would have passed vacuously against the old code; this one doesn't.

Deployment notes

None

What this covered

Full read of both changed files in complete context — skillscope/datasets.py (631 lines) and tests/test_skillscope.py (2519 lines) — plus skillscope/schema/machine.schema.json, skillscope/schema/evals.schema.json, README.md, and git history on both changed files. Reviewed 0f734b2...c5260ef (the PR head against its actual merge-base with origin/main; origin/main has since advanced ~28 commits past that base, including an unrelated later addition of a sandbox key to MACHINE_KEYS, which is why the base used is older than current main — using current main as the base would have misattributed that unrelated commit to this PR). Ran the full TestMachineSchema/TestMachineRejections suite at the PR head (13/13 pass, matching the green CI already reported on the PR) and separately re-ran the new test after manually reverting the production change, to directly confirm the test is non-vacuous.

One independent blind sub-pass (impact-analysis, history, prior-PRs, agent-instruction-adherence, code-comment-compliance) ran as a separate subagent and is the source of the blocking finding above, which I independently re-verified against the source and with an actual test run. A second planned independent worker — a dedicated blind code-correctness pass over the diff — could not be spawned: the shared execution environment was at its concurrent-subagent cap for the duration of this review (two spawn attempts both rejected), so I performed that pass myself directly against the source instead of via an independent context; per the review method this is disclosed as a limitation rather than presented as a fully independent fan-out. No CLAUDE.md/AGENTS.md exists anywhere in this repository, so the agent-instruction-adherence pass found nothing to check (confirmed independently and by the sub-pass). Not reconciled against any discussion on this PR.

---

🤖 Automated review by agent-hub on AMD AgentHub.
Model claude-sonnet-5[1m] · Not a human review. Reply here if a finding is wrong.