[rominf]

Why

A skill whose behavioral cases need something the runner may not have, such as a library to build or a Python package to install, had one way to say so: a setup_session entry point in evals/hooks.py. The inspect engines refuse that entry point, so such a skill can no longer run its behavioral leg. ROCm/rocm-systems' thread-trace skill is one: its hook builds the rocprof trace decoder and exports the variables the agent needs.

Re-adding the hook would not have been enough. A hook runs in this process, so it can only prepare the host. A skill that also runs under --engine claude-code would have to describe its environment a second time, in a compose file for the container, and the two descriptions would drift.

What changes

evals/machine.yml takes a prerequisites: list. Every engine reads that one declaration and applies it where its agent runs.

prerequisites:
  - name: rocprof-trace-decoder
    check: [python3, -c, "import elftools, rocprof_trace_decoder"]
    install: [sh, install-decoder.sh]
    env:
      PYTHONPATH: "${SKILL_CACHE_DIR}/decoder/python"

• Order. check runs first. If it passes, the prerequisite is already there and its env is not applied. Otherwise install runs, then check runs again with env applied, and only then does env reach the agent.
• Where it runs. On the default engine, once per skill on the host. Under --engine claude-code, in each case's container, with the agent built per case from the resulting env. A command may differ per platform, and a container counts as linux.
• What steps see. SKILL_SOURCE_DIR (the skill's source tree, host only) and SKILL_CACHE_DIR (scratch space, removed after the skill). An env value may name those two and nothing else. It replaces a variable rather than extending it. It cannot set PATH, the names the agent's harness and model client read (ANTHROPIC_*, CLAUDE_*, COPILOT_*, IS_SANDBOX), or the two inputs themselves, in any case, and no variable may be set twice.
• Isolation. The env reaches only its own skill's agent. os.environ is never changed.
• The answer key stays out. Under claude-code only the files a step names are copied into the container, and they are removed before the agent starts. A step that names evals.json or extended_evals.json is refused.
• Failures stay local. A prerequisite that cannot be provided fails its own skill's cases, and only those, with the step's output in the report. That includes a host where the cache directory cannot be made, and a container in which the steps directory cannot be created or removed or a step's file cannot be copied in. It is never counted as a provider outage.
• A broken machine.yml holds back only its skill. One that cannot be read (not UTF-8, or not readable by the runner) or does not parse is reported as that skill's error, and select and the structural check go on to the other skills rather than stopping at it. select lists the skill as held back with the error, and the results job in skill-evals.yml fails on it with that error instead of reporting it as a missing gate label. A gate that holds back several skills is named once.
• Stopping. A --timeout stop ends a running install and every process it started, whether the run stops gracefully or the watchdog kills it outright. Results already graded are kept.
• Checked up front. The structural check validates the declaration, including that every platform in os has a command.
• setup_session. Its refusal now points at prerequisites:. docs/authoring-evals.md documents the key, including how to bake an install into a sandbox: image so a container does not repeat it for every case.
• Internal interface. The solver_factory that behavioral.build_task takes now receives (skill_dir, env). The CLI and tools/sandbox_smoketest.py are updated.

Verification

• The full suite (718 tests) runs in CI on Python 3.10 and 3.12, on Linux and Windows. ruff --select F401 is clean.
• The tests fail when the fixes are reverted. Each fix in this change was reverted on its own in a scratch copy: about 90 single-branch breaks, each caught by at least one test. They cover:
  • the check, install and re-check order;
  • env isolation;
  • cache and process cleanup on both stop paths;
  • the answer-key refusal;
  • outage classification;
  • a broken machine.yml holding back only its skill, and the results job's reading of it;
  • every validation rule;
  • the CLI forwarding --model and --effort.
• Real processes. The default engine is tested end to end through a real inspect eval(), with a stub claude that records the environment it was started with. A stop during an install is delivered for real, and the test checks that both the install and the child it started are gone.
• Container leg. It is tested against a stand-in sandbox. A new step in the "Sandbox machinery" CI job runs it in a real Docker container, with a fixture skill that declares a prerequisite, and checks from inside the container that the env arrived, the install happened and the copied steps are gone.
• thread-trace. With its hook ported to a prerequisites: declaration, the skill is no longer refused on the default engine. Run in a sparse checkout that lacks the decoder source, its install fails, and the skill's cases are reported failed with the install's own message.

Not verified:

• A full thread-trace pass: it needs a ROCm GPU.
• The container leg against a real Docker daemon outside CI. The only local engine is Podman, which fails inspect's Docker version check.
• The Windows and macOS host branches: CI's Windows job covers the unit tests, not a real install.

Scope

Left out, as follow-ups:

• No per-step time limit. On the host, only --timeout bounds a hung install. A step also waits for every process holding its output open, so an install that leaves a server running does not finish. The docs say both.
• The source tree is resolved eagerly for any skill that declares prerequisites. For a vendored skill that is a fetch even when check passes.
• The structural check doesn't require a linux command. It checks the platforms in os, but a container is always linux, so a macOS-only declaration passes there and fails under claude-code.
• A step that names the answer key is caught when a claude-code task is built, before any case runs, not by the structural check.
• check hooks in evals/hooks.py stay refused. ROCm/rocm-systems' decode-and-triage uses one, and replacing it needs its own design.

[danielholanda]

Hi @rominf, can you provide a human-written description of why this here is needed? Is this a regression in functionality caused by another PR?

[rominf]

Hi @danielholanda. Partly:

• Default engine (claude-code-no-sandbox): v0.1.3 could execute setup_session, but there were no consumers at the moment. My Rebuild the eval engine on inspect_ai and retire the legacy runner #19 dropped it with that reasoning. Then thread-trace (ROCm/rocm-systems) started using it. We noticed just before merging Rebuild the eval engine on inspect_ai and retire the legacy runner #19 and decided to merge anyway and fix it in a follow-up before next release, since every consumer pins v0.1.3 (I ensured it by creating a PR in one repository that used main). It's probably my mistake that I haven't noted that in Rebuild the eval engine on inspect_ai and retire the legacy runner #19.
• Sandbox engine (claude-code): not a regression, since this engine is new.

This PR specifies setup session instructions in readable, declarative format, which is suitable for both: sandboxed and default engines. After the merge, amd/skills' docs/evals.md needs to be updated accordingly.

[johnl-amd]

Nice work, this is careful code. CI is green, 579 tests pass on 3.10, 3.12 and 3.13 with nothing skipped, and I went looking for a test that couldn't fail without finding one.

Approving. Two small asks, one scope question, and a list for follow-ups.

Two small asks

The rule that check passing suppresses env needs writing down as the author's obligation, not just described as a mechanism. The docs say what happens but never say "write check so it passes only when the agent would work without this env". Without that, here's the failure: a runner has system ROCm with the Python bindings but doesn't export ROCPROF_TRACE_DECODER_LIB. check exits 0, nothing installs, the env is never set, and every case fails because rocprofv3 --att can't find the library. The better-provisioned machine is the one that breaks and the bare runner passes. Worse, [prerequisites] skill: 1 to provide prints identically either way, so the log doesn't tell you which branch you took. One sentence in docs/authoring-evals.md, plus ideally a line saying which prerequisites were satisfied by the environment versus installed.

PATH isn't reserved. The docs flag it as a hazard, pointing out PATH: ${SKILL_CACHE_DIR}/bin would leave the agent with just that directory, but _RESERVED_ENV only covers ANTHROPIC_, CLAUDE_ and IS_SANDBOX. shutil.which("claude") has already resolved so the CLI is fine, but anything it spawns isn't.

Scope question

SKILL_SOURCE_DIR is host only by construction, and prerequisites.py:433 hands the container leg a hardcoded "a container has no copy of the skill's source tree". But thread-trace builds its decoder from the rocm-systems tree, so under --engine claude-code it has to bake into the image, and then check passes and the declaration contributes nothing there. So the container half, most of the new module plus a test class and a Docker CI job, serves no skill that exists. That also undercuts the argument for this over a host-only hook, since thread-trace maintains the compose file either way and the drift happens anyway.

Not asking you to rip it out. But what are you picturing as the first real consumer? The design assumes its install is a plain download with no source tree. And /opt/skillscope/cache is fixed precisely so people can bake images against it, so it freezes the first time anyone does. A docs line saying that path is provisional until a skill uses it would keep the room to move it.

Follow-ups

• _named_files compares resolved paths by equality and Path.resolve() doesn't canonicalise case, so on a case-insensitive host install: [cat, EVALS.JSON] gets copied in. verify.require() refuses Windows and local but not macOS, where APFS is case-insensitive by default.
• Nothing tests the evals_dir in candidate.parents traversal guard, the only branch of _named_files with no coverage. Pairs with the above.
• _prerequisite_gaps silently skips any os: label it doesn't recognise, so os: [Liunx] defeats the "a command for every platform in os" guarantee with no diagnostic. Root cause is _string_list never constraining os:, which predates you, but the new guarantee is what makes it bite.
• _ENV_NAME uses $ rather than \Z, so "FOO\n" passes as an env name, contradicting the error text next to it. fullmatch fixes it.
• _argv only validates value[0], so ["cmd", ""] is accepted while the schema's minLength: 1 on every item rejects it.
• _live_steps.add(proc.pid) happens after the spawn, so if the hard timeout fires in that window the child survives os._exit and gets reparented, being a session leader.
• Not yours, but adjacent: selection.matrix_entries calls machine_plan with no SystemExit guard, unlike every other caller, so one bad machine.yml takes out the whole matrix.

[rominf]

@johnl-amd The two asks are in, and the cache path is marked provisional.

check passing is the author's obligation in docs/authoring-evals.md: it must exit 0 only when the agent would work without that prerequisite's env. After the steps, the log names the branch, on both engines:

[prerequisites] thread-trace: 1 to provide
[prerequisites] thread-trace: rocprof-trace-decoder satisfied by the environment

or rocprof-trace-decoder installed, and both when a skill has a mix (system satisfied by the environment; built installed).

PATH is refused, including Path and path. A value replaces the variable, the CLI is already resolved, and everything the agent starts would see only that directory.

On the container half: the install it can run is a command plus the files named beside machine.yml, with no SKILL_SOURCE_DIR. thread-trace is not that. It builds the decoder from the rocm-systems tree, so under claude-code the image has to contain it, and what the declaration does there is check — the same check and env the image was built to satisfy, rather than a second description. I don't have a skill yet whose install is a plain download into the container. /opt/skillscope/cache is documented as provisional until one bakes an image against it, so the path can still move.

The follow-ups (case-insensitive _named_files, the traversal-guard test, an unrecognised os: label, _ENV_NAME and _argv validation, the pid registered after spawn, and matrix_entries) I'll take separately.

[rominf]

@johnl-amd The follow-ups are in too.

• The answer key is compared with samefile, so a second path to the same file is refused. resolve() does not canonicalise case, and a hard link is that situation on a case-sensitive volume: two paths, one file. A path that resolves outside evals/ is not copied, which is the branch that had no test.
• os: [Liunx] is reported when the skill has prerequisites. A label that is not linux, windows, or macos was skipped, so the command-for-every-platform check never saw it. A label with no prerequisites is still just a runner label.
• Env names use fullmatch. $ matches before a trailing newline, so FOO\n passed and the error text next to it was wrong.
• Every argv item has to be a non-empty string. ["cmd", ""] was accepted because only the first item was checked; the schema already rejects it.
• The watchdog snapshot and the pid registration share a lock. The pid is only known once the process exists, so a step that appears after the snapshot is killed by the spawner. os._exit will not look again, and a session leader it missed would be reparented and keep running.
• matrix_entries catches SystemExit from machine_plan. That skill is held back with the error, and the rest of the matrix is still planned.

Signed-off-by is on 3a183f9.

Dismissed: this approval predates the follow-up commits (6a0a18c, 3a183f9) that address its asks. Re-review requested.

[rominf]

@johnl-amd I dismissed your approval from 28 Oct. It predates the three follow-up commits: 6a0a18c covers your two asks and 3a183f9 covers the follow-ups I listed in my last reply. Could you take another look when you have time? The checks on 3a183f9 are green.

[nowycondro]

🤖 Automated review · review-code · head ecd9a989

Review — needs work

Full review of the whole change (no narrowing — this is a first automated pass). No ticket named; the description is an accurate, detailed account of the change and matches what the diff actually does.

Blocking

• selection.plan's gates list is no longer deduplicated — skillscope/selection.py:287
"gates": sorted(leg["gate"] for leg in skipped if leg["gate"]) replaced the previous sorted({leg["gate"] for leg in skipped}). The old code deduplicated via a set; the new code is a plain generator expression, so every skipped leg contributes its gate name again. Since scoped_gate is a single repo-wide string (skillscope/config.py:123), not per-skill, any run with two or more scoped skills held back by the same missing gate now reports that gate's name once per skipped skill instead of once.
  Confirmed by running the real selection.plan(["alpha", "beta"], ...) with both skills gated behind the same scoped_gate (reusing the existing TestSelection fixtures): gates comes back as ['enable_gpu_ci', 'enable_gpu_ci'] instead of ['enable_gpu_ci']. No test exercises two skipped legs sharing a gate — test_a_gated_leg_is_reported_rather_than_run only gates a single skill, and test_one_bad_machine_yml_does_not_drop_the_matrix only checks that the empty-gate placeholder (from an unreadable machine.yml) is filtered out, not that real gates are deduplicated.
  Confidence 95 · logic · Fix: wrap the generator back in a set, e.g. sorted({leg["gate"] for leg in skipped if leg["gate"]}).

Non-blocking

None

Decisions for the author

None

Positive signals

• skillscope/datasets.py's Prerequisite/_prerequisite_gaps/_ENV_NAME/_argv validation is unusually careful: every argv item (not just index 0) is checked against the schema's minLength: 1, _ENV_NAME uses fullmatch so a trailing-newline name is correctly rejected, and an unrecognised os: label is reported rather than silently skipping the "a command for every platform" guarantee — all three are exercised by dedicated table-driven tests in TestAPrerequisiteDeclarationIsCheckedUpFront.
• TestMachineSchema.test_documented_keys_match_the_parser asserts the JSON schema's property sets equal the parser's MACHINE_KEYS/PREREQUISITE_KEYS/COMMAND_PLATFORMS constants directly, so the schema and the parser cannot silently drift apart.
• The watchdog/process-cleanup tests in TestPrerequisitesOnTheDefaultEngine (e.g. test_the_watchdogs_hard_exit_kills_a_running_install_and_its_children, test_a_stop_during_an_install_fails_the_skill_and_keeps_the_report) actually spawn real child processes and poll /proc/tasklist to confirm they are dead, rather than asserting on mocked calls — this is real verification of a hard-to-get-right concurrency path, not a simulation of it.

Deployment notes

• /opt/skillscope/cache (skillscope/engine/prerequisites.py:71-72, CONTAINER_CACHE) is a fixed container path so a baked image's install can land at the same place check later looks for it. The code and docs both flag this as provisional ("nothing pins the path yet"); once an image is built against it, moving it becomes a breaking change for that image.
• skillscope/engine/hooks.py's setup_session entry point is now refused unconditionally (replaced by prerequisites:); any skill still shipping one will fail with the new refusal message rather than running, which is the intended migration but is worth calling out to downstream skill authors.

What this covered

Every changed file read in full against the PR head, plus surrounding/calling code: skillscope/datasets.py, skillscope/schema/machine.schema.json, docs/authoring-evals.md, skillscope/engine/prerequisites.py, skillscope/engine/hooks.py, skillscope/engine/behavioral.py, skillscope/engine/no_sandbox.py (including the harness-abstraction env-layering added in the newest commit), skillscope/engine/verify.py, skillscope/cli.py, skillscope/selection.py, skillscope/structure.py, tools/sandbox_smoketest.py, .github/workflows/selftest.yml, docs/usage.md, README.md, pyproject.toml, and the ~1,290 new/changed lines of tests/test_skillscope.py plus the new ordering test in tests/test_harness.py. Commit range 39eff043(current main tip)…ecd9a989 (PR head); the head moved from the orchestrator-supplied 3a183f92 to ecd9a989 mid-review (one new commit, "Pin that a harness's own env wins over a skill's prerequisites") — re-diffed and confirmed every previously-read file was byte-identical except skillscope/engine/no_sandbox.py and tests/test_harness.py, which were reviewed fresh, including a live mutation test confirming the new environment-layering test actually fails when the fix is reverted. Also ran the full unit test suite at the PR head (587 tests, all green on Python 3.11) and confirmed via gh pr view --json statusCheckRollup that every CI check (unit tests on 3.10/3.12 × ubuntu/windows, sandbox-machinery Docker job, unused-imports, action-against-a-throwaway-repo) is green.

Degraded fan-out: this review ran single-context. Every attempt to spawn independent workers (grouped by coupling: declaration/schema, host+container engine, CLI/selection/CI/docs) failed with "Concurrent subagent limit reached" after two attempts, so this pass was done by one reviewer reading every file directly rather than via independent parallel passes. Coverage was kept as close to full-breadth as a single context allows (every file read in full, cross-file contracts traced by hand, tests executed and mutated), but the cross-cutting defects that a true multi-worker consolidation step is best at surfacing may be under-covered. Recommend re-running with fan-out available.
Did not run: the "prior changes" pass (no visibility into earlier review threads on these files was sought, by design) and the "agent-instruction adherence" pass (repo has no CLAUDE.md/AGENTS.md). History (git log/git blame) was checked for the pre-existing files touched (datasets.py, hooks.py, behavioral.py, no_sandbox.py, verify.py, cli.py, selection.py, structure.py); nothing in it contradicted this change.
Not reconciled against any discussion or prior review on this PR — this pass was kept blind to the existing human review on purpose, per its instructions.

---

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 (base 39eff04…head ecd9a98, PR #29's current head).
No ticket named. The description's own "Scope" section already discloses several known follow-ups (see Decisions for the author).

Blocking

• A machine.yml that can't be read at all (not just one with bad YAML) still crashes dataset planning for every skill, defeating this PR's own stated fix. skillscope/datasets.py:606-611 (_read_machine), skillscope/datasets.py:501-509 (structural_errors), skillscope/selection.py:186-198 (matrix_entries).
  _read_machine wraps yaml.safe_load(path.read_text(encoding="utf-8")) in a try that only catches yaml.YAMLError. path.read_text() can also raise OSError (permission denied, or the file replaced by a directory between the is_file() check and the read) or UnicodeDecodeError (non-UTF-8 bytes) — neither is a yaml.YAMLError. Both matrix_entries and structural_errors only guard their call to machine_plan/_read_machine with except SystemExit, so either of those two exception types propagates raw and aborts planning for every skill after the broken one — exactly the failure this PR's own selection.py docstring and commit ("Close the gaps the review left for a follow-up") say is now fixed. The PR's own new test, test_one_bad_machine_yml_does_not_drop_the_matrix, only exercises a YAML-syntax error, so it doesn't catch this gap.
  Confidence 85 · logic · Fix: widen _read_machine's except clause to except (OSError, UnicodeDecodeError, yaml.YAMLError) as exc and raise the same SystemExit.

Non-blocking

• claude_code_agent's container mkdir and its final cleanup rm -rf bypass the exception-to-PrerequisiteError translation the rest of the function relies on. skillscope/engine/prerequisites.py:486 (made = await box.exec(["mkdir", ...])), :520 (removed = await box.exec(["rm", "-rf", CONTAINER_STEPS])).
  Every other sandbox call in this solver goes through the local run() closure, which catches OutputLimitExceededError/OSError/UnicodeDecodeError from box.exec and turns them into a reported, named failure — the stated point of the module ("reported under the prerequisite's name rather than as a bare sandbox error"). These two calls invoke box.exec directly, outside that wrapper and outside the surrounding except BaseException (which covers only the provide()/file-copy block). If the sandbox raises one of those exceptions on mkdir or on the final rm -rf, it propagates as a raw, unnamed exception instead of a PrerequisiteError, inconsistent with the rest of the function.
  Confidence 75 · mechanical · Fix: route both calls through the same try/except (or the run() closure) used elsewhere in this function.

• _created_caches is mutated without the lock that protects the otherwise-identical _live_steps, re-opening the exact race the lock exists to close. skillscope/engine/prerequisites.py:218 (declaration), :228-231 (_discard_all_caches), :363/:390 (on_host's add/discard).
  _live_steps carries an explicit comment explaining why it needs _steps_lock ("the pid is only known once the process exists, and a snapshot that missed it must not be the last look"); _created_caches sits two lines above with the same justifying comment but no lock. If the watchdog's hard exit (_on_expire → _discard_all_caches) fires in the narrow window between tempfile.mkdtemp() returning and _created_caches.add(...) executing, that cache directory is in neither the pre-exit snapshot nor removed, and leaks permanently once os._exit fires. Consequence is a leaked temp directory, not data loss.
  Confidence 72 · architectural · Fix: guard _created_caches mutations with _steps_lock (or a dedicated lock), the same way _live_steps is protected.

• The existing CI workflow that reads selection.plan()'s skipped list doesn't know about the new "error" entries this PR adds, and labels them the same as an ordinary missing-gate-label skip. .github/workflows/skill-evals.yml:743-748 (unchanged by this PR) vs. skillscope/selection.py:193-197 (the new {"gate": "", "error": ...} shape).
  The results job prints "These skills' behavioral cases were held back for a missing gate label and did not run: {skipped}" whenever skipped is non-empty, with no check for the new error field and no mention of its content. A skill whose machine.yml has, say, an unknown key or a bad prerequisites entry — the exact case test_one_bad_machine_yml_does_not_drop_the_matrix is written to keep the rest of the matrix running through — is reported to a developer as "missing a gate label" rather than as the real configuration error. The discover job's separate structural step may already fail loudly first for some of these cases depending on scoping, which is why this is non-blocking rather than blocking, but the annotation text itself is stale regardless.
  Confidence 70 · logic · Fix: branch the annotation on entries with a non-empty error and print those distinctly (or at least include the error text).

• The rule that an env key may not be named SKILL_SOURCE_DIR/SKILL_CACHE_DIR is enforced in code but undocumented. skillscope/datasets.py:732-736 (_prerequisite_env's PREREQUISITE_INPUTS check) vs. docs/authoring-evals.md:218-222 and the schema's env description.
  The docs and schema both say PATH and the harness's own variables (ANTHROPIC_*, CLAUDE_*, IS_SANDBOX) "cannot be set here either," but never mention that naming either of the two input variables as an env key is also refused, with its own distinct error message ("names an input skillscope supplies to every step"). An author would only discover this by triggering the SystemExit.
  Confidence 70 · documentation · Fix: add a sentence next to the existing "cannot be set here either" line.

Decisions for the author

• The reserved-name check for SKILL_SOURCE_DIR/SKILL_CACHE_DIR is case-sensitive, unlike the neighbouring PATH/harness-variable check — answered-by-intent. skillscope/datasets.py:732 (if name in PREREQUISITE_INPUTS:) is an exact-string tuple-membership test, while three lines later _RESERVED_ENV explicitly uses re.IGNORECASE with a comment noting Windows env names are case-insensitive. A declaration like env: {skill_cache_dir: ...} passes validation today, and on Windows (where env-var names are case-insensitive) could collide with the real SKILL_CACHE_DIR the harness supplies. This would otherwise be blocking, but the PR description's own "Scope" section names it verbatim: "Windows environment names are compared case-sensitively against the reserved names." Confirm this is still the intended follow-up rather than something worth closing now, given it's the one gap in the validation that is specifically about overriding skillscope's own reserved inputs.
• Install failures are never treated as a retryable "provider outage," even when the real cause is a transient network blip during a container's install step — tradeoff. The PR deliberately makes this choice ("Failures stay local... It is never counted as a provider outage") so a build log's incidental wording (e.g. "502") can't masquerade as infrastructure flakiness, and it's tested that way (test_a_prerequisite_that_cannot_be_installed_fails_only_its_skill). The tradeoff: a prerequisite install that fails only because of a genuinely transient network issue is still reported as a hard, non-retryable failure. Worth confirming that's intended for any downstream consumer of BehaviorOutcome.degraded that might otherwise auto-retry.

Positive signals

• The check → install → re-check state machine in provide() is backed by genuinely adversarial tests: real subprocess cancellation, a real --timeout stop landing mid-install that verifies both the install and the child process it spawned are actually killed, and (per the description) each fix individually reverted to confirm a test catches it — an unusual level of rigor for concurrency/cleanup code, and it shows in how few issues survived review of that code.
• The env-expansion restriction (values may name only ${SKILL_SOURCE_DIR}/${SKILL_CACHE_DIR}, nothing else) closes off an entire class of "which process actually expands this" ambiguity between the host and container engines, and the schema, parser, and docs all agree on the resulting grammar.

Deployment notes

• Runtime dependencies now always include PyYAML for a behavioral run (previously only CI planning touched it), per the updated docstrings in datasets.py and pyproject.toml — confirmed accurate against the code; no action needed beyond what's already documented.
• The container engine's fixed paths (/opt/skillscope, /opt/skillscope/cache) are explicitly documented as provisional pending a skill image being built against them — already flagged in the code and docs, not a new concern.

What this covered

Every file in the diff, read completely, against 39eff043463b774e86e78af8b90637eca66d3e08…ecd9a989ee66d505a0b4ee7c028d38b4cf5b07ff (this PR's current head): skillscope/datasets.py, the new skillscope/engine/prerequisites.py, skillscope/engine/{behavioral,verify,hooks,no_sandbox}.py, skillscope/cli.py, skillscope/selection.py, skillscope/structure.py, skillscope/schema/machine.schema.json, docs/authoring-evals.md, docs/usage.md, README.md, pyproject.toml, .github/workflows/selftest.yml, tools/sandbox_smoketest.py, tests/test_harness.py, tests/test_skillscope.py — plus surrounding code each one touches (deadline.py's watchdog contract, every call site of machine_plan, and the unchanged .github/workflows/skill-evals.yml as a downstream consumer of selection.plan()'s output shape). Split across six independent file-grouped passes plus this consolidating pass. CI's statusCheckRollup for the reviewed commit was read directly and is green (8/8 checks passing).

Did not run: agent-instruction adherence (no CLAUDE.md/AGENTS.md or equivalent exists in this repo) and prior-changes (no earlier review round on this repo was consulted, by this method's own design). History (git log/blame) was read for the touched modules as part of each pass; nothing beyond what's reported above stood out. Not reconciled against this PR's own review discussion or earlier follow-up commits — this review reads the current head fresh, per its instructions.

---

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

[rominf]

Thanks to both reviews of ecd9a98: the automated review of 4 Nov, and @johnl-amd's review comment of the same day. Six commits on top of ecd9a98, pushed as a fast-forward, no force-push. Each new test was seen to fail when the code it covers is broken.

Blocking

• gates named a shared gate once per held-back skill: 43a8028. It is a set again. A new test holds two skills back behind the same gate and expects the gate once.
• A machine.yml that cannot be read still stopped planning for every skill after it: 7062c9a. _read_machine now turns OSError and UnicodeDecodeError from read_text into that skill's SystemExit, the same way bad YAML is. While testing that, I found that yaml.safe_load also rejects input without a YAMLError: a date like 2001-13-45 raises ValueError, and deep nesting raises RecursionError. Both are caught now too. The tests cover a non-UTF-8 file, a read denied with PermissionError (runs on every platform and as root), a mode-0 file (POSIX, non-root), and both YAML cases. They go through select and through the structural check, where the skill sorted after the broken one is still checked.

Non-blocking

• (a) Container mkdir and final rm -rf outside the error handling: confirmed, 83f303e. Both go through the same OutputLimitExceededError/OSError/UnicodeDecodeError handling as the steps, and fail as a PrerequisiteError naming the exception. Before, a raw sandbox error could reach the outage heuristic. Each call is tested with each exception type, and the agent does not start when the removal fails.
• (b) _created_caches without _steps_lock: confirmed, 50edc6f. Taking the lock only around the add would not close the window: the watchdog's snapshot could still land between mkdtemp and the add. The cache is now made and recorded under the lock. Once the watchdog has closed the sets, no cache is made. The watchdog also takes its cache snapshot under the lock. The test fires the watchdog as the directory is made and checks that nothing is left on disk once its callback returns.
• (c) skill-evals.yml reports error entries as a missing gate label: confirmed, a0b574b. Only entries without error get the gate-label warning now. An entry with error gets an ::error:: annotation with the message, escaped so that a multi-line YAML error stays one annotation, and it fails the results job. I went one step past relabelling there: a missing label is a run nobody asked for, and a broken machine.yml is not. In this workflow the structural step in discover usually fails on the same file first, so this is a backstop. The tests run the step's script from the YAML, including on what select actually emits for a broken file.
• (d) SKILL_SOURCE_DIR/SKILL_CACHE_DIR as env keys undocumented: a95d356. docs/authoring-evals.md and the schema's env description now list them. The parser's refusal said skillscope supplies them "to every step", which is not true in a container (there is no SKILL_SOURCE_DIR there), so it now says skillscope sets them for the steps itself.

Decisions for the author: follow-ups, not in this PR

• Case-insensitive reserved names on Windows: still a follow-up, as listed under Scope in the description.
• Install failures as retryable outages: kept non-retryable on purpose, so a build log's wording cannot pass as provider trouble. Retrying a transient install failure is a follow-up if a consumer needs it.

The description now covers the select/structural/results behaviour for a broken machine.yml, and the updated test count.

One correction to my 30 Oct comments: the commits I cited there, 6a0a18c and 3a183f9, were rewritten by a later force-push. The same patches are now 2aa2226 (the two asks) and 8aab806 (the follow-ups).

[rominf]

Rebased onto main (7a8c00b) to clear the conflicts with #34 and #40. The new head is 65ae187.

• pyproject.toml: kept Keep agent-client-protocol below 1.0 until inspect-ai supports it #40's agent-client-protocol note and this PR's PyYAML line (it is now read by a behavioral run too, not only CI planning).
• skillscope/cli.py: Add GitHub Copilot CLI as a harness, with routing that needs no model credential #34 passes the harness chosen on the command line to harness_solver, and this PR replaced that lambda with behavioral_solver_factory, whose factory also takes the prerequisites' env. behavioral_solver_factory now takes the harness as well (Claude Code when none is named) and calls harness_solver with it. The CLI passes the harness it resolved, and the existing CLI test now asserts that.

Only the first commit changed in the rebase; the other nine replayed as they were. The full suite (714 tests) and ruff --select F401 pass on the new head.

[rominf]

One more commit, 1340852: _read_machine now treats any exception from yaml.safe_load as the file being invalid YAML. PyYAML also rejects input with AttributeError (!!timestamp x) and KeyError (!!bool x), which still ended planning for every skill with a traceback. The test covers both tags. The full suite (714 tests) and ruff --select F401 pass.

[johnl-amd]

🤖 Automated review · review-code · head 13408520

Review — needs work

Full review of the whole change (base 7a8c00b9…head 13408520e8e91c262a85e2d08331aa3325710afe, +2934/-110 across 19 files, 11 commits), split across four independent fan-out workers grouped by coupling (the declaration schema, parsing and docs; the host engine core; CLI, selection and CI gating; the container engine and its CI smoketest), each blind to the others and to this PR's existing comment thread (and to any earlier review round on a prior head of this branch). No ticket named as closed; the description is a long, detailed account of the change, and is unusually candid about its own limits — it lists explicit "Not verified" items and a "Scope"/follow-ups section rather than overclaiming, and this review's findings below were checked against both.

Blocking

• Some prerequisite-provisioning failures are not translated into the friendly PrerequisiteError, and on the host engine this crashes the whole command instead of failing just the one skill — skillscope/engine/prerequisites.py:233-243 (_make_cache, host path: tempfile.mkdtemp()), skillscope/engine/prerequisites.py:536 (claude_code_agent's copy loop, container path: box.write_file(...)/path.read_bytes())
  The module exists specifically so a prerequisite problem reads as a clear per-skill message rather than an ugly traceback (PrerequisiteError's own rationale: inspect would otherwise report a sample's error via repr(), putting escaped newlines and the class name in the report). _make_cache calls tempfile.mkdtemp() with no try/except around it; if it raises (disk full, no permission, no /tmp), the raw OSError is not a PrerequisiteError. behavioral.run()'s per-skill exception handling only catches PrerequisiteError/KeyboardInterrupt/SystemExit, so this propagates out of run() entirely — crashing the whole command instead of degrading just the one skill, which directly contradicts the PR's own stated contract ("Failures stay local... fails its own skill's cases, and only those"). The same gap exists independently on the container engine's copy loop (unwrapped box.write_file/path.read_bytes failures), which two independent reviewers found without being shown each other's work — corroborating that this is a real, recurring gap in the new module's error-wrapping discipline rather than a one-off.
  Confidence 78 · architectural · Fix: wrap both call sites' expected failure modes (OSError at minimum) and re-raise as PrerequisiteError with the underlying message, matching the pattern already used for check/install subprocess failures elsewhere in this same module.

Non-blocking

• A container-side file-inclusion check for prerequisite steps compares paths case-sensitively, inconsistent with this change's otherwise careful handling of platform-dependent name matching — skillscope/engine/prerequisites.py:458 (_named_files's evals_dir not in candidate.parents guard)
  _is_answer_key elsewhere in this same function explicitly guards against case-insensitive filesystems via samefile, with its own docstring noting Path.resolve() does not canonicalise case — but the "is this candidate inside evals_dir" membership test is a plain, case-sensitive Path/string comparison. On a case-insensitive-but-case-preserving filesystem (e.g. macOS, where a contributor could build a claude-code task locally), a prerequisite argv path differing in case from the on-disk name could resolve to a real file inside evals_dir yet fail this membership check, silently excluding it from named without ever being copied. This fails toward not copying rather than toward a security hole, and is narrow/platform-dependent — not something this review executed.
  Confidence 70 · logic · Fix: resolve both sides consistently (e.g. compare resolved absolute paths via samefile/os.path.normcase-aware logic, mirroring the existing _is_answer_key guard) rather than relying on Path.parents membership.

• machine.schema.json's own disclaimer of what it doesn't express omits two rules the parser actually enforces — skillscope/schema/machine.schema.json:54 (the env property description)
  The description says the parser enforces "a command for every platform in os, and unique names, none of which this schema expresses" — but _prerequisite_env (skillscope/datasets.py:726-768) also enforces that env keys match _ENV_NAME (letters/digits/underscore, not starting with a digit) and that values contain no NUL byte, neither of which the schema's env property ("additionalProperties": {"type": "string"}, no propertyNames constraint) expresses, and neither of which the disclaimer lists.
  Confidence 70 · documentation · Fix: add the env-name-shape and NUL-byte rules to the disclaimer alongside the ones already listed.

• A test exercising ordinary outage-classification logic is misplaced inside the prerequisites test class, and doesn't exercise prerequisites at all — tests/test_skillscope.py:218-223 (test_a_failed_skill_is_an_outage_only_when_its_detail_reads_as_one, inside TestProvidingAPrerequisite)
  This test calls engine_behavioral._failed(...) and checks routing_core.is_provider_error-driven outage classification — it never calls provide() or touches any prerequisites machinery, and would still pass unchanged if provide() were deleted entirely.
  Confidence 75 · test-quality · Fix: move it to whichever test class already covers behavioral._failed/outage classification.

• Nested-subdirectory file copying for prerequisite steps is only validated against an in-memory fake sandbox, never a real one — tests/test_skillscope.py (test_files_named_by_either_step_arrive_at_their_relative_paths, TestPrerequisitesInAContainer), the fake _Box.write_file (a plain dict assignment with no real filesystem semantics)
  The real CI container smoke test (.github/workflows/selftest.yml's prereq-skill fixture) only names a top-level file, never a nested path (e.g. tools/check.sh). Whether the real sandbox's write_file auto-creates intermediate directories for a nested relative path is therefore asserted only by a mock standing in for that behavior, not proven end-to-end.
  Confidence 72 · test-coverage · Fix: add a nested-path file to the CI smoketest's fixture skill, or otherwise exercise this against a real sandbox rather than the fake.

Decisions for the author

• PREREQUISITE_INPUTS's override-protection check is case-sensitive, letting a differently-cased env key (e.g. skill_source_dir) bypass it on Windows — answered-by-intent. skillscope/datasets.py:741 (if name in PREREQUISITE_INPUTS:) is a plain, case-sensitive tuple membership test, unlike _RESERVED_ENV a few lines above it (datasets.py:558-564), which is deliberately compiled re.IGNORECASE specifically because "Windows environment names are case-insensitive." This review verified directly that _RESERVED_ENV already handles case correctly, while PREREQUISITE_INPUTS's separate protection (guarding SKILL_SOURCE_DIR/SKILL_CACHE_DIR specifically) does not — so a prerequisite could silently set the very inputs the design says it "may not override," on a case-insensitive platform. This would otherwise have been a non-blocking logic finding (confidence ~78), but the PR description's own "Scope" section already discloses: "Windows environment names are compared case-sensitively against the reserved names." Confirm whether that note is intended to cover this exact gap (it reads as the same symptom, even though the specific set involved — PREREQUISITE_INPUTS rather than _RESERVED_ENV — isn't named) before treating it as already tracked.
• Schema is looser than the parser on "stripped non-empty" requirements — non-blocking-improvement. _argv's argv[0] and machine_prerequisites's name both require .strip() to be non-empty, but the JSON schema only has "minLength": 1 on the corresponding properties, so e.g. check: [" ", "foo"] or name: " " would validate against the schema and then be rejected by the parser.
• PREREQUISITE_INPUTS and _ENV_INPUT's regex duplicate the same two names by hand — non-blocking-improvement. Nothing enforces that a future third input added to one is reflected in the other.
• contextlib.suppress(Exception) around the container engine's best-effort cleanup doesn't suppress BaseException (e.g. asyncio.CancelledError from a second, overlapping cancellation) — tradeoff. If the cleanup rm -rf itself is cancelled, the original prerequisite failure's message is replaced by a bare CancelledError, which contradicts the comment immediately above this code ("never in place of the error being raised") — but this is a narrow double-failure race and may be a deliberate "don't spend more time on cleanup" choice.

Positive signals

• TestAPrerequisiteDeclarationIsCheckedUpFront's full parametrized table was traced branch-by-branch against the parser (unknown/missing keys, the $-expansion rule, case-insensitive reserved-name rejection, name-format/NUL-byte rules, per-platform gap detection); every case hits the specific branch its position implies, and would fail outright if the corresponding parser code were reverted.
• _ENV_NAME's fullmatch (correctly rejecting a trailing-newline name, where match would accept it) and the ${SKILL_SOURCE_DIR}/${SKILL_CACHE_DIR} stripping regex were both verified directly against adversarial inputs and behave exactly as documented, with no bypass found.
• The watchdog/hard-exit tests (test_the_watchdogs_hard_exit_kills_a_running_install_and_its_children, test_a_step_that_appears_after_the_snapshot_is_killed_too) block on real process-liveness polling rather than asserting against mocks, so they would hang or fail — not silently pass — if the kill logic were broken.
• Env isolation is real, not assumed: a fresh env is computed per skill (host) or per case (container), os.environ is never mutated, and nothing a skill installs is visible to the next one.
• The CI container smoke test's fixture (argv-list check/install, ${SKILL_CACHE_DIR}-only env) matches the real parser's contract exactly, and is correctly gated to ubuntu-latest only, consistent with the Linux-only assumption this feature documents.

Deployment notes

• setup_session entry points in evals/hooks.py are now refused unconditionally in favor of prerequisites: — re-verified this round (_require_hook_support's message, hooks.py's own docstring, and docs/authoring-evals.md all agree). This is the intended migration, but is a breaking change for any skill still shipping the old hook (ROCm/rocm-systems' thread-trace, named in the PR description, is migrated here; its decode-and-triage sibling, using a check hook, is explicitly left refused).
• GitHub reports mergeable_state as blocked (review required) rather than a merge conflict, at the time of this review.
• The description's own "Not verified" and "Scope" disclosures (no full ROCm GPU pass, container leg not tested against real Docker outside CI, Windows/macOS host branches only unit-tested) are honest rather than overclaiming, and line up with real gaps this review independently found (e.g. the nested-path container-copy test only using a fake sandbox).

What this covered

Every changed file read in full against the PR head by four independent fan-out workers grouped by coupling: skillscope/datasets.py, skillscope/schema/machine.schema.json, docs/authoring-evals.md, docs/usage.md (schema/parsing/docs group); skillscope/engine/prerequisites.py (host path), skillscope/engine/hooks.py, skillscope/engine/no_sandbox.py (host engine group); skillscope/cli.py, skillscope/selection.py, .github/workflows/skill-evals.yml, README.md, pyproject.toml (CLI/selection/CI group); skillscope/engine/prerequisites.py (container path), skillscope/engine/verify.py, tools/sandbox_smoketest.py, .github/workflows/selftest.yml (container engine group); plus the relevant classes in tests/test_skillscope.py and tests/test_harness.py read by whichever group owned the production code they exercise. All 8 CI checks on this head commit are green (unit tests on ubuntu/windows × Python 3.10/3.12, sandbox machinery on both OSes, unused-imports, action-against-a-throwaway-repo).

Did not run: agent-instruction adherence (no CLAUDE.md/AGENTS.md exists in this repository). No worker executed the test suite directly against a checkout of this head (production code was read via git show, not checked out, per the review method's rule against checking out a PR head) — conclusions about test behavior on revert are from tracing, not execution, except where a worker explicitly built a separate scratch clone to run something (noted inline above where that happened). Not reconciled against this PR's existing 9-comment thread, or against any earlier review round on a prior head of this branch — this pass was kept blind to both by design, per the review method's independence rule.

---

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

[johnl-amd]

Review — no blocking findings

Full review of the whole change (19 files, +2934/-110 across 11 commits) at head 1340852 (13408520e8e91c262a85e2d08331aa3325710afe) against main's current tip 7a8c00b (7a8c00b9ea153c5efc5679f60d7a5e9df0deb056) — confirmed via git merge-base that the two are identical, so this is the whole change with nothing narrowed out.
No ticket named in the description (no #NNNN reference found anywhere in it).

Blocking

None

Non-blocking

• A misdeclared prerequisite command that would leak the answer key is only caught at task-build time, not by the cheap structural check — skillscope/engine/prerequisites.py:437-469 (_named_files), called only from skillscope/engine/prerequisites.py:491 inside claude_code_agent; skillscope/datasets.py:457 (structural_errors, whose own docstring says it exists so "a malformed dataset fails in seconds rather than halfway through a paid run") never calls it.
  The analogous "every platform in os has a command" gap is caught structurally, via _prerequisite_gaps (skillscope/datasets.py:771). An author who writes install: [sh, evals.json] by mistake only discovers it when a claude-code behavioral run actually builds the task — later and more expensively than the sibling check, and inconsistent with the module's own stated design intent ("Surface it here, where it costs seconds").
  Confidence 70 · architectural · Fix: call _named_files (or an equivalent host-side dry run) from structural_errors/_prerequisite_gaps so a reference to the answer key is reported structurally, the same way a missing per-platform command is.

• Two prerequisites in the same machine.yml that declare the same env variable name collide silently — skillscope/engine/prerequisites.py:210 (applied.update(env) inside provide()'s loop unconditionally overwrites), while validation only checks a single prerequisite's own env mapping for legality (skillscope/datasets.py:726, _prerequisite_env) and only rejects a duplicate prerequisite name (skillscope/datasets.py:804, machine_prerequisites) — never a variable declared by two different prerequisites. The second prerequisite's value silently wins, with nothing in the log (_provision_line) distinguishing the two, and no test exercises this case either way.
  Confidence 72 · logic · Fix: track env keys across all prerequisites while building the list in machine_prerequisites, and raise SystemExit on a name declared by more than one, mirroring the existing "declared twice" check for prerequisite name.

Decisions for the author

• Once-per-skill-on-host vs. once-per-case-in-container execution — tradeoff. no_sandbox runs a skill's check/install once on the host; claude-code runs them again in every case's container (skillscope/engine/verify.py, prerequisites.claude_code_agent). Nothing enforces that check/install are idempotent or side-effect-free, so a prerequisite with a side effect or non-deterministic check could behave differently across engines or across cases of the same skill. This cadence difference is already disclosed in the PR's own "Where it runs" section, so it reads as a deliberate, documented choice rather than an oversight — flagging it so the author can confirm authors are expected to write idempotent check/install by convention.
• The "held back, could not be planned" per-skill path in skill-evals.yml's results job looks like it is usually shadowed by an earlier all-or-nothing failure — non-blocking-improvement. discover's "Check every skill's structure" step runs skillscope structural unconditionally over every skill in the repo with no continue-on-error (.github/workflows/skill-evals.yml:348), and cmd_structural (skillscope/cli.py:300) calls the same datasets.structural_errors/machine_prerequisites read that would also raise for a broken machine.yml. If it does, discover fails outright and results' handling of the per-skill unplanned list (.github/workflows/skill-evals.yml:758-770) exits before reaching it. The adjacent code comment already says as much ("Usually the structural step has already failed the discover job on the same file; this is for when it has not"), so this reads as a conscious defense-in-depth choice rather than dead code — worth a regression test (or a comment pointing to one) that actually exercises the narrower case the fallback is for, since right now it's not obvious such a case exists in this repo's own workflow.

Positive signals

• The check → install → recheck sequencing in provide() (skillscope/engine/prerequisites.py:152) applies env only after a successful install and recheck, and expand() runs before install so a misconfigured env referencing an unavailable input fails in milliseconds rather than after a costly build.
• This PR also fixes a genuine pre-existing gap: before it, skillscope/engine/verify.py's call to behavioral.run_eval lacked fail_on_error=False (unlike the host leg's equivalent call, which already had it), so one case whose container failed to prepare could abort an entire skill's already-graded results on the sandboxed leg. This PR brings it to parity and adds a regression test for it.
• TestTheResultsJobReadsWhyALegWasHeldBack extracts and executes the actual embedded Python script from the shipped skill-evals.yml (via yaml.safe_load + subprocess) rather than reimplementing its logic in the test, so it validates the real shipped text rather than a parallel model of it.
• tools/sandbox_smoketest.py's --expect-sandbox is supplied by the test caller rather than read back from the harness under test — the harness cannot be asked to confirm its own answer, which would make the check unable to fail.

Deployment notes

• pyproject.toml's comment update confirms PyYAML is now a runtime dependency at eval time (not just for CI planning); it was already declared, so no action is needed, but a minimal/vendored install that previously assumed PyYAML was CI-only should note the change.
• The container-side cache path (/opt/skillscope/cache) is fixed but explicitly documented as provisional; an image baking an install in at that path today should expect it may need rebuilding if the path is ever repinned.
• Per the PR's own "Not verified" section: a full thread-trace pass (needs a ROCm GPU) and the container leg against a real Docker daemon outside CI (only Podman was available locally, which fails inspect's Docker version check) were not run. Anyone depending on this for a GPU-backed skill or a non-CI Docker setup should validate those paths before relying on them.

What this covered

Every file in the change read completely at the head commit, plus the surrounding code needed to judge it: the full skillscope/engine/prerequisites.py (new), skillscope/schema/machine.schema.json, skillscope/structure.py, docs/authoring-evals.md, skillscope/engine/behavioral.py, skillscope/engine/hooks.py, skillscope/engine/no_sandbox.py, skillscope/engine/verify.py, tools/sandbox_smoketest.py, tests/test_harness.py, skillscope/selection.py, skillscope/datasets.py, skillscope/cli.py, .github/workflows/skill-evals.yml, .github/workflows/selftest.yml, pyproject.toml, README.md, docs/usage.md, and every new/changed test class in tests/test_skillscope.py, run as four independent blind fan-out passes grouped by coupling, each unaware of the PR description or any discussion. The two findings that survived scoring were independently re-verified afterwards by reading the call graphs and validation logic directly (skillscope/engine/prerequisites.py, skillscope/datasets.py). 1340852 is reviewed against 7a8c00b (main's tip; confirmed identical to the merge-base, so nothing is narrowed).

Did not run: the "prior changes" source-reading pass (no record of review comments on earlier, separate changes to these files exists outside this PR's own discussion, which the method keeps out of a blind review). "History" ran as ordinary git log/context reading within each file group rather than as a separate pass; nothing of note surfaced beyond what's already in Positive signals. No agent-instruction files (CLAUDE.md/AGENTS.md) exist in this repository, so that pass does not apply. CI: all 8 checks reported by gh pr view 29 --json statusCheckRollup are green (SUCCESS).

Not reconciled against any PR discussion, including a prior automated review on this same PR — this is a blind review.

---

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

[johnl-amd]

Review — needs work

Full review of the whole change (base 7a8c00b9…head 13408520, +2934/-110 across 19 files, 11 commits). No ticket named; the description is a long, detailed, and unusually candid account of the change, including explicit "Not verified" and "Scope" sections. Judged against it at Step 9: its claim that a prerequisite failure "fails its own skill's cases, and only those... It is never counted as a provider outage" is contradicted by the two unwrapped-exception sites in Blocking below.

Blocking

• Two prerequisite-provisioning failure paths escape as raw exceptions instead of PrerequisiteError, contradicting the PR's own stated contract that a prerequisite failure stays local to one skill and is never read as a provider outage — skillscope/engine/prerequisites.py:241 (_make_cache's tempfile.mkdtemp(...), called at prerequisites.py:389 before on_host's own try/finally begins) and skillscope/engine/prerequisites.py:535-538 (the container copy loop, for relative, path in named.items(): await box.write_file(...), inside a bare except BaseException: ...; raise that re-raises unchanged rather than converting to PrerequisiteError).
  Traced both to their actual consumers: behavioral.py's per-skill handling (behavioral.py:377/385/394) only catches PrerequisiteError/KeyboardInterrupt/SystemExit, so a raw OSError from mkdtemp (disk full, no /tmp, permission denied) propagates out of the for skill in skills: loop and crashes the whole command instead of failing just that skill. On the container side, behavioral.py's _is_outage() decides "not an outage" only by finding the qualified PrerequisiteError class name in the sample's traceback — an unwrapped write_file/read_bytes failure has no such name in it, so it can be misclassified as a provider outage, the opposite of the documented intent. No test makes either call raise.
  Confidence 85 · architectural · Fix: wrap both call sites' expected failure modes and re-raise as PrerequisiteError, matching the pattern already used for check/install failures and the exec_errors/housekeeping() catches elsewhere in this same module.

• skill-evals.yml's new "machine.yml could not be planned" handling in the results job is unreachable through this workflow's own job graph — .github/workflows/skill-evals.yml:760-779, skillscope/selection.py:179-193 (matrix_entries's except SystemExit wraps only datasets.machine_plan(skill)), skillscope/datasets.py:457-508 (structural_errors, called repo-wide via cli.py:300's cmd_structural → cli.py:240's _structural_or_exit() with no skill scoping).
  Traced the full call graph: structural_errors catches SystemExit from machine_plan(skill) in the same try block as _prerequisite_gaps(...), over every skill in the repo (declared_skills()), and this runs in discover's unconditional "Check every skill's structure" step. matrix_entries (what select uses to build results's SKIPPED list) only ever catches a SystemExit from machine_plan too — it never calls machine_prerequisites/_prerequisite_gaps at all, so it cannot even report a broken prerequisites: section, only a broken top-level os/labels/unknown-key error. Since structural_errors is a strict superset check run repo-wide ahead of select, any failure that could make select's SKIPPED list carry an error entry would already have failed discover's structural step — and results's script sys.exits on DISCOVER != "success" (line just above this hunk) before ever reaching the new unplanned/::error:: code. The code's own comment ("usually the structural step has already failed... this is for when it has not") does not hold given this call graph: this reviewing context found no path that produces DISCOVER == "success" alongside a SKIPPED entry carrying error, and the diff's own test for this path constructs that exact combination manually, which the real pipeline cannot.
  Confidence 85 · architectural/test-coverage · Fix: either confirm there is a real caller this defends (e.g. select invoked outside this workflow without structural first) and say so in the comment, or drop the now-dead branch.

• _argv's whitespace-only-first-argv-item rejection has no test anywhere in the suite — skillscope/datasets.py:689-701 (the not value[0].strip() arm of the or-chain).
  Every other branch of this new validation is covered by a dedicated subTest in TestAPrerequisiteDeclarationIsCheckedUpFront; this is the one with none — confirmed by reading that class's full parametrized table. Reverting just this or arm leaves the suite green.
  Confidence 82 · test-coverage · Fix: add a case like install=[" ", "x"].

• test_a_gate_shared_by_two_held_skills_is_named_once does not exercise what this diff actually changed — tests/test_skillscope.py:1234-1241, skillscope/selection.py:287.
  The diff's only change to plan()'s gates line is adding if leg["gate"] to an already-existing set comprehension (sorted({leg["gate"] for leg in skipped if leg["gate"]})). This test gives both skipped legs the same non-empty gate, so the pre-diff code (sorted({leg["gate"] for leg in skipped}), dedup via the set alone) produces the identical ["enable_gpu_ci"] — reverting the if leg["gate"] filter leaves this test green. The filter itself is exercised elsewhere (test_one_bad_machine_yml_does_not_drop_the_matrix, which checks the empty-gate placeholder is excluded), so there is no live coverage gap, only a misleadingly-named test.
  Confidence 85 · test-quality · Fix: rename it to reflect what it actually asserts (plain gate dedup, pre-existing), or add a legs-with-one-empty-and-one-real-gate case that the filter alone decides.

Non-blocking

• machine.schema.json doesn't disclose that argv/name are stripped-non-empty, unlike its own env property, which does disclose its parser-only rules — skillscope/schema/machine.schema.json (the env description lists what the schema can't express; the sibling argv/name minLength: 1 properties have no equivalent note, though _argv and machine_prerequisites's name both additionally require .strip()). The schema is not runtime-enforced anywhere, so this is a documentation-accuracy gap, not a functional one. Confidence 75 · documentation · Fix: add the same kind of disclaimer next to argv/name.
• selection.py's new module/matrix_entries docstrings say a leg is held back when its machine.yml "could not be read", but the mechanism also covers a file that parses fine yet fails validation (bad os/labels/unknown key) — confirmed by the diff's own non-UTF8/unreadable/YAML-rejection test battery, which includes cases that are readable text but still fail. Confidence 75 · documentation · Fix: reword to "could not be read or planned."
• The real CI/Docker container smoketest (selftest.yml's prereq-skill fixture) never exercises nested-subdirectory file copying — only the unit-test suite's in-memory fake sandbox (TestPrerequisitesInAContainer's _Box.write_file, a plain dict assignment) does. Some test does reach this path, so it is not an uncovered-behaviour gap in the strict sense, just unverified against a real sandbox's directory-creation semantics for behaviour the module's own docstring calls out as important. Confidence 70 · test-coverage · Fix: add a nested-path file to the CI fixture.

Decisions for the author

• CONTAINER_ROOT = "/opt/skillscope" is a fixed path baked into the image contract before any concrete skill is confirmed to depend on the container leg — tradeoff. The PR's own motivating example (thread-trace) is a host-engine case; nothing in this diff's tests or docs names a skill that actually needs --engine claude-code prerequisites today. Pinning the path now buys a stable target for whoever bakes the first image against it; it also means moving it later is a breaking change for that image. Worth a docs line confirming the path is provisional until a real consumer exists, if that is still the intent.
• A later prerequisite in the same skill's list can silently overwrite an earlier one's identically-named env key — non-blocking-improvement. provide()'s accumulation (applied.update(env)) has no duplicate-key check across steps. The function's own framing treats later-wins as intentional layering (same pattern as the harness-vs-prerequisite layering elsewhere in this PR), so this is not obviously wrong, just unguarded.
• contextlib.suppress(Exception) around the container engine's best-effort cleanup (prerequisites.py:557) does not shield BaseException (e.g. asyncio.CancelledError) — tradeoff. If the cleanup rm -rf is itself cancelled while handling an earlier failure, the original, more informative error (e.g. a PrerequisiteError carrying the real install output) is replaced by a bare CancelledError, arguably at odds with the adjacent comment ("never in place of the error being raised"). This requires two overlapping cancellations to trigger and the existing tests only exercise the first; given the "best effort" framing already applied to this cleanup elsewhere, this may be an accepted gap rather than an oversight — confirm.

Positive signals

• TestAPrerequisiteDeclarationIsCheckedUpFront's full parametrized table traces nearly every SystemExit branch in the new validation to a dedicated, wording-specific subTest, including a defensive key=str sort for mixed-type YAML keys.
• A new bidirectional test in tests/test_harness.py (test_a_skills_prerequisites_reach_the_agent_under_what_the_harness_sets) proves the harness-vs-prerequisite environment layering order both ways: it fails if the layering is reverted and if the merge order is swapped, not just on one mutation.
• The container engine's check-vs-install distinction and the PrerequisiteError/_is_outage classification are verified end-to-end through a real inspect_eval() call rather than mocked at the boundary.
• matrix_entries's new per-skill try/except isolating one broken machine.yml from the rest of the CI matrix is covered by six distinct, well-targeted scenarios (invalid YAML, non-UTF-8, unreadable, several non-YAMLError PyYAML rejections).

Deployment notes

• evals/hooks.py's setup_session entry point is now refused unconditionally in favor of prerequisites:. Per the PR description, ROCm/rocm-systems' thread-trace is migrated; its decode-and-triage sibling (a check hook) is explicitly left refused as a named follow-up.
• CONTAINER_ROOT/CONTAINER_CACHE under /opt/skillscope is a fixed container path; once any image is baked against it, moving it becomes a breaking change for that image (see Decisions for the author).
• All 8 CI checks on this head are green (confirmed via gh pr view --json statusCheckRollup): unit tests on ubuntu/windows × Python 3.10/3.12, sandbox machinery on both OSes, unused-imports, and action-against-a-throwaway-repo.

What this covered

Every file in the change read completely by four independent fan-out workers grouped by coupling — declaration/schema/docs (skillscope/datasets.py, skillscope/schema/machine.schema.json, docs/authoring-evals.md, docs/usage.md); host engine core (skillscope/engine/prerequisites.py's host path, skillscope/engine/hooks.py, skillscope/engine/no_sandbox.py, skillscope/engine/behavioral.py, tests/test_harness.py); CLI/selection/CI (skillscope/cli.py, skillscope/selection.py, skillscope/structure.py, .github/workflows/skill-evals.yml, README.md, pyproject.toml); container engine + smoketest (skillscope/engine/prerequisites.py's container path, skillscope/engine/verify.py, tools/sandbox_smoketest.py, .github/workflows/selftest.yml) — plus the relevant classes in tests/test_skillscope.py each group owned. Commit range 7a8c00b9…13408520.

This reviewing context then independently re-traced every candidate finding reported at confidence ≥70 directly against the primary source before including it here (not taking any relayed summary at face value), including full function-level call-graph tracing for both Blocking findings above.

Provenance note, in the interest of full disclosure: this task's own duplicate-review check (confirming no existing review on this PR carries this reviewing setup's required marker) required reading the full text of the three existing reviews on this PR before any findings-producing work began. The four fan-out workers were never shown that content and were explicitly instructed not to read any PR discussion, and this report's findings were independently re-derived and verified from source rather than copied — but this reviewing context cannot claim to have been fully blind to that prior content when scoping the workers' instructions, and several findings above (the unwrapped-exception sites, the CONTAINER_ROOT question) converge with points already raised in the existing human review at this same head. The job-topology dead-code finding and the weak-test finding are not present in the prior reviews read.

Did not run: agent-instruction adherence (no CLAUDE.md/AGENTS.md exists in this repository, confirmed directly). Prior-changes pass did not run — no record of review comments on earlier, unrelated changes to these files was sought. History pass was light-touch only (git log on the core touched files); nothing in it contradicted this change.

Not reconciled against this PR's existing review threads' specific arguments or rebuttals — only their presence/absence of the required marker was checked, per this task's own instructions.

---

Automated review by an agent.

[rominf]

Thanks for the three reviews of 1340852: the automated review of 9 Nov, the review comment of 10 Nov, and the AgentHub review of 10 Nov. Three commits on top, pushed as a fast-forward, no force-push. Each new test was seen to fail when the code it covers is broken.

Blocking

• Provisioning failures that escaped as raw exceptions (raised in both reviews): d8d40aa. An OSError from making the host cache now fails only that skill, as a PrerequisiteError, and the run goes on to the next. A failure to read a file or copy it into the container is a PrerequisiteError naming the file, and the copied steps are still removed. Tests: a full disk for one of two skills (the other runs), and each sandbox error from write_file plus a PermissionError from read_bytes.
• results' handling of an unplanned skill is unreachable in this workflow: confirmed. structural_errors reads every declared skill's machine.yml, a superset of what select sees and more strictly, and its failure stops discover before select runs. I kept the branch and corrected the comment to say exactly that (f6ed7bb). Without it, an error entry that did reach results would pass silently, because the gate-label warning now leaves those entries out.
• No test for a whitespace-only argv[0]: added (9db8b90).
• test_a_gate_shared_by_two_held_skills_is_named_once doesn't exercise the diff: partly. Against main, the only change is the if, as you say. But within this branch the test pins the set that an earlier revision dropped (the 4 Nov blocker, fixed in 43a8028), and reverting to a list fails it. The test now says so, and names the test that covers the empty-gate filter (f6ed7bb).

Non-blocking

• Schema disclaimer missing the env-name, NUL, and stripped name/argv rules: added (9db8b90).
• Outage-classification test in the prerequisites class: moved to TestEngineSkillFailureIsContained (f6ed7bb).
• Nested-path copying only checked against the fake sandbox: the CI fixture's step is now tools/install.sh (f6ed7bb), so the Sandbox machinery job copies into a real container's subdirectory.
• selection.py docstrings say "could not be read": now "read or planned" (f6ed7bb).
• Two prerequisites setting the same env variable: refused, with an error naming the earlier prerequisite (9db8b90). So is one prerequisite setting two names that differ only in case.
• The answer-key check isn't structural: left as a follow-up and listed under Scope. It already runs when a claude-code task is built, before any case runs or any tokens are spent. Moving it would put the container-copy rules into the structural check.
• _named_files' evals_dir in candidate.parents is case-sensitive: no change. The candidate is evals_dir joined with the argv item, so its prefix is evals_dir's own spelling. Only a path that leaves the directory and comes back in different case (../EVALS/x.sh) misses, and it fails closed: the file isn't copied, and the step fails naming it. The answer-key check, the one that matters for safety, uses samefile.

Decisions for the author

• Case-sensitive check of SKILL_SOURCE_DIR/SKILL_CACHE_DIR as env keys: fixed now rather than left as a follow-up (9db8b90). The Scope bullet is gone. While there, COPILOT_* joined the reserved names: the Copilot harness from Add GitHub Copilot CLI as a harness, with routing that needs no model credential #34 sets some of them, and on the host its value wins, so a declared one was silently dropped. The docs now say what is and isn't reserved, including what setting GH_TOKEN does.
• PREREQUISITE_INPUTS and _ENV_INPUT duplicated by hand: _ENV_INPUT is now built from the tuple (9db8b90).
• contextlib.suppress(Exception) around the container cleanup: kept on purpose. Swallowing a CancelledError raised during the cleanup would leave the task uncancelled. The comment now says that this is the one thing that can replace the error being raised (d8d40aa).
• /opt/skillscope fixed before a consumer exists: already marked provisional in docs/authoring-evals.md ("That path is provisional until a skill's image is built against it") and in the module docstring.
• Idempotent check/install: yes, by convention. The docs now say the steps may run once or many times, on a machine that has the prerequisite or not, and must give the same result (f6ed7bb).

The description is updated: the failure-locality and env bullets, the Scope list, and the test count.

[johnl-amd]

Review — no blocking findings

Full review of the whole change (14 commits, 19 files, +3072/-110). No ticket named. The PR's own description is thorough and detailed, and matches what the review found: every disclosed limitation in "Scope" and "Not verified" corresponds to real, correctly-scoped behavior in the code, and no claimed-but-missing or undisclosed-but-present work was detected.

Blocking

None.

Non-blocking

• verify.require()'s inspect-swe version-floor guard checks only for effort, not for the new env parameter the same call now always sends — skillscope/engine/verify.py:91-99 (guard), skillscope/engine/verify.py:250-255 (build_task's agent() closure, which now unconditionally calls claude_code(..., env=env or None) for every skill, prerequisites or not). The PR's own test, test_a_current_inspect_swe_is_accepted, builds a fake claude_code with signature (skills=None, cwd=None, effort=None) — no env — and asserts require() accepts it as current. The guard's own comment explains exactly why this matters ("the failure arrives as a TypeError underneath a traceback... observed exactly that way on a runner whose inspect-swe predated the argument and so satisfied the old floor without upgrading"), but that protection was not extended to the second capability this PR now depends on. I independently fetched inspect_swe's source at its pinned floor tag (0.2.71, github.com/meridianlabs-ai/inspect_swe) and confirmed env is already a parameter there, so no installation satisfying today's stated floor can currently trigger this — it's a latent gap in the guard's completeness, not a live bug.
  Confidence 70 · logic · Fix: extend the check to "effort" not in params or "env" not in params, with a combined message.
• The "missing input" explanation attached to a failed prerequisite install is a heuristic substring search, not a check of what the prerequisite's own env actually references — skillscope/engine/prerequisites.py, inside provide() (missing = "".join(... if name in said), around lines 188-196). In the container engine, unavailable unconditionally includes SKILL_SOURCE_DIR for every skill (claude_code_agent, ~line 560), so any install failure whose output happens to literally contain the string SKILL_SOURCE_DIR gets an unrelated "(was not available: ...)" note appended, while a genuine missing-input failure whose script doesn't print the variable name gets none. The code's own comment acknowledges this is approximate ("for an input the step mentions"), so this is a documented trade-off rather than an unrecognized defect, but it's a real source of misleading error text.
  Confidence 70 · logic · Fix: key the explanation off whether the failing prerequisite's own declared env map references the unavailable token (item.env, already available at the call site), not off a string search over argv/output.
• _kill_pid's Windows branch runs subprocess.run(["taskkill", ...], timeout=5) synchronously inside the async event loop — skillscope/engine/prerequisites.py, _kill_pid, called from _host_runner's coroutine (if _track_step(proc.pid): _kill_pid(proc.pid)). In the narrow race where the --timeout watchdog has already fired exactly as a new step is spawned, this blocks the event loop for up to 5s on Windows (the POSIX branch is a single non-blocking os.killpg syscall). Not independently verified on Windows hardware; no test exercises this specific interleaving.
  Confidence 70 · logic · Fix: offload to asyncio.to_thread if this path needs to coexist with other in-flight installs on Windows.

Decisions for the author

• An os label typo (e.g. Liunx) is only caught when the skill also declares prerequisites — tradeoff. datasets.py's _prerequisite_gaps returns immediately for a skill with no declared prerequisites, so machine_plan never validates os against the known platform set otherwise. This is explicitly disclosed in docs/authoring-evals.md ("An os label that is not linux, windows, or macos is reported too, when this skill has prerequisites: a typo would otherwise skip that check") and in the PR's own "Scope" section, so it is a deliberate, documented boundary rather than an accidental gap. A stronger version would validate every skill's os label unconditionally in machine_plan itself.
• Prerequisite name uniqueness is compared case-sensitively, unlike every other identity check in the same function (env-var names, the two reserved inputs, and Windows path-name collisions are all folded case-insensitively) — non-blocking-improvement. name is used only for display/log purposes, so two prerequisites named tool and Tool being treated as distinct is low-impact, but it is an inconsistency in an otherwise very deliberately case-folded piece of code.

Positive signals

• provide() (the function implementing "check, then install, then re-check with env") is small, pure, and exhaustively unit-tested on its own, independent of either engine's plumbing — the PR's central claim, "one declaration, applied identically by both engines," is enforced by sharing a single tested function rather than by convention between two parallel implementations.
• The hard-exit watchdog's cache/process bookkeeping (_make_cache, _track_step, _kill_live_steps, _discard_all_caches, all under one lock) correctly closes the race its own comments describe, and is specifically exercised by tests that interleave a real mkdtemp call with the watchdog firing mid-call — a genuinely difficult concurrency surface to get right, and to test at all.
• Across the 14-commit series, every fix a later commit claims to make for a bug an earlier commit left behind (case-insensitive answer-key bypass, the "Liunx" os-label gap, a cache-creation/watchdog lock race, case-insensitive env-name collisions, unhandled container copy/mkdir failures) was independently traced and confirmed present and correct at HEAD — none were found only half-applied.
• Command execution throughout (_host_runner, the container run() closure) always uses argv lists via asyncio.create_subprocess_exec/box.exec, never an interpolated shell string.
• The answer-key exclusion when copying files into a case's container (_is_answer_key's samefile()-based check) is tested against a real hard link, not just a path-string comparison, proving the identity check is load-bearing rather than decorative.

Deployment notes

• A prerequisites: step's install/check commands run with the same trust level as the rest of a skill: unsandboxed on the host under the default engine, same as the agent itself already runs there. This is explicitly documented ("steps are trusted like the rest of the skill") and is not a new trust boundary relative to what the engine already does, but is worth restating for anyone pointing the default engine at untrusted skill submissions.
• The container engine's cache path (/opt/skillscope/cache) and steps path (/opt/skillscope/steps) are a new fixed convention; a skill's own pre-built sandbox: image that wants to avoid repeating install per case needs to bake artifacts at that exact path, and both the code and docs call the path "provisional."
• No behavior change for any existing skill that does not declare prerequisites: — confirmed the empty-declaration path is a no-op on both engines.

What this covered

Full change read completely across four independent fan-out passes grouped by coupling: (1) declaration/schema/docs — skillscope/schema/machine.schema.json, skillscope/datasets.py, docs/authoring-evals.md, docs/usage.md, README.md; (2) host engine core — skillscope/engine/prerequisites.py, no_sandbox.py, hooks.py, behavioral.py; (3) CLI/selection/CI — skillscope/cli.py, selection.py, structure.py, pyproject.toml, .github/workflows/skill-evals.yml; (4) container engine + CI smoketest — skillscope/engine/verify.py, tools/sandbox_smoketest.py, .github/workflows/selftest.yml (with prerequisites.py and engine/sandbox.py read in full as shared/callee context). All four workers read their full assigned scope and reported no inability to do so. Diffed 7a8c00b9ea153c5efc5679f60d7a5e9df0deb056…f6ed7bbc8628e6195841e47524ed3bc6dc951f07, fetched read-only and never checked out.

Every test class the PR added or changed, in both tests/test_skillscope.py and tests/test_harness.py, was read completely and given an explicit "would it still pass if the corresponding production change were reverted" / "does it exercise the specific branch it names" verdict; several were independently confirmed via mutation testing in disposable scratch clones outside the reviewed repository. CI for this exact head commit was read directly (gh pr view --json statusCheckRollup): all 8 checks (unit tests on Linux/Windows × Python 3.10/3.12, sandbox machinery, unused imports, action-against-a-throwaway-repo) report SUCCESS.

Did not run: agent-instruction-adherence (no CLAUDE.md/AGENTS.md exists in this repository) — recorded as skipped in every pass, not silently omitted. Prior-changes pass (review comments on earlier changes to these files) did not run: no mechanism was available to retrieve review comments on other, earlier PRs touching these files, as distinct from this PR's own discussion, which this review's independence rule forbids reading at any point. Not reconciled against any discussion on this PR — no review comments, review threads, or prior reviews on PR #29 were read before or during this review, per its governing instructions.

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

[rominf]

Thanks for the review of f6ed7bb. Nothing blocking, so no new commits. Answers to each point:

• verify.require() checks effort but not env: no change. The check is for an install that predates the floor. Any inspect_swe with claude_code(effort=) is at least 0.2.71, and as you confirmed, env is already a parameter at 0.2.71. So no install that passes the guard lacks env, and a second check would never fire.
• The missing-input note on a failed install is a substring search: deliberate. Keying it off item.env can't work. An env value that names an unavailable input is already refused before anything runs: expand runs ahead of install, and the test that checks this is test_an_input_this_engine_cannot_supply_is_named_before_installing. So the note only matters for a step that reads SKILL_SOURCE_DIR itself, and the only trace of that is the step's argv and output. A false positive appends one explanatory clause to a failure that is already reported. A miss leaves the step's own output, which is always kept.
• taskkill blocks the event loop on Windows: no change. The branch only runs when the --timeout watchdog has already closed the step set as a step is being spawned. The run is stopping at that point, and the kill is what it is waiting for, so a 5 s block delays nothing else.
• An os typo is only caught for a skill with prerequisites: as you note, that's the documented boundary. Checking os for every skill would change machine_plan for skills this PR doesn't touch.
• Prerequisite names compared case-sensitively: no change. A name only labels the prerequisite in the log and the report. Env names are folded because Windows treats X and x as one variable. Nothing treats Tool and tool as the same prerequisite.

[rominf]

@johnl-amd your re-review of f6ed7bb found nothing blocking, and CI is green on that head. Would you approve it so it can merge?