Skip to content

Let a skill declare its prerequisites once, for both engines - #29

Open
rominf wants to merge 1 commit into
amd:mainfrom
rominf:prerequisites
Open

rominf wants to merge 1 commit into
amd:mainfrom
rominf:prerequisites

Conversation

@rominf

@rominf rominf commented Oct 26, 2026

Copy link
Copy Markdown
Collaborator

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, and it cannot set the variables the harness sets for the agent.
  • 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. It is never counted as a provider outage. A machine.yml that does not parse fails only its skill too.
  • 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

  • 579 tests pass on Python 3.10, 3.12 and 3.13. ruff --select F401 and actionlint are 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;
    • 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.
  • Windows environment names are compared case-sensitively against the reserved names.
  • check hooks in evals/hooks.py stay refused. ROCm/rocm-systems' decode-and-triage uses one, and replacing it needs its own design.

A skill whose behavioral cases need something the runner may not have --
a library to build, a package to install -- had one way to say so: an
`evals/hooks.py` `setup_session`, which these engines refuse. It ran on
the host only, so a skill that also ran under `--engine claude-code` had
to describe its environment a second time for the container, and the two
descriptions drifted.

`evals/machine.yml` now takes `prerequisites:`, each a `check` and an
`install` command (optionally per platform) plus the `env` the agent
needs once it is installed. Both engines apply the same declaration
where their agent runs: once per skill on the host, in each case's
container under `claude-code`. `check` runs first; only if it fails does
`install` run, then `check` again with `env` applied, and only then is
`env` handed to the agent. Steps see `SKILL_SOURCE_DIR` and
`SKILL_CACHE_DIR`, and nothing a skill installs reaches another skill.

A prerequisite that cannot be provided fails its own skill's cases with
the step's output and is never counted as a provider outage. A stop from
`--timeout` ends a running install and everything it started, on both
the graceful and the hard path. Under `claude-code` only the files a
step names are copied into the container, the answer key never, and
they are removed before the agent starts. The structural check validates
the declaration up front, including a command for every platform the
skill runs on.

The `setup_session` refusal now points at `prerequisites:`.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf marked this pull request as ready for review October 26, 2026 11:35
@danielholanda

Copy link
Copy Markdown
Collaborator

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

rominf commented Oct 27, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @danielholanda. Partly:

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 johnl-amd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants