Repository navigation
Conversation
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>
|
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? |
|
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' |
johnl-amd
left a comment
There was a problem hiding this comment.
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_filescompares resolved paths by equality andPath.resolve()doesn't canonicalise case, so on a case-insensitive hostinstall: [cat, EVALS.JSON]gets copied in.verify.require()refuses Windows andlocalbut not macOS, where APFS is case-insensitive by default.- Nothing tests the
evals_dir in candidate.parentstraversal guard, the only branch of_named_fileswith no coverage. Pairs with the above. _prerequisite_gapssilently skips anyos:label it doesn't recognise, soos: [Liunx]defeats the "a command for every platform inos" guarantee with no diagnostic. Root cause is_string_listnever constrainingos:, which predates you, but the new guarantee is what makes it bite._ENV_NAMEuses$rather than\Z, so"FOO\n"passes as an env name, contradicting the error text next to it.fullmatchfixes it._argvonly validatesvalue[0], so["cmd", ""]is accepted while the schema'sminLength: 1on every item rejects it._live_steps.add(proc.pid)happens after the spawn, so if the hard timeout fires in that window the child survivesos._exitand gets reparented, being a session leader.- Not yours, but adjacent:
selection.matrix_entriescallsmachine_planwith noSystemExitguard, unlike every other caller, so one badmachine.ymltakes out the whole matrix.
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_sessionentry point inevals/hooks.py. The inspect engines refuse that entry point, so such a skill can no longer run its behavioral leg. ROCm/rocm-systems'thread-traceskill 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-codewould 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.ymltakes aprerequisites:list. Every engine reads that one declaration and applies it where its agent runs.checkruns first. If it passes, the prerequisite is already there and itsenvis not applied. Otherwiseinstallruns, thencheckruns again withenvapplied, and only then doesenvreach the agent.--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 aslinux.SKILL_SOURCE_DIR(the skill's source tree, host only) andSKILL_CACHE_DIR(scratch space, removed after the skill). Anenvvalue 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.os.environis never changed.claude-codeonly the files a step names are copied into the container, and they are removed before the agent starts. A step that namesevals.jsonorextended_evals.jsonis refused.machine.ymlthat does not parse fails only its skill too.--timeoutstop 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.oshas a command.setup_session. Its refusal now points atprerequisites:.docs/authoring-evals.mddocuments the key, including how to bake an install into asandbox:image so a container does not repeat it for every case.solver_factorythatbehavioral.build_tasktakes now receives(skill_dir, env). The CLI andtools/sandbox_smoketest.pyare updated.Verification
ruff --select F401andactionlintare clean.--modeland--effort.eval(), with a stubclaudethat 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.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:
thread-tracepass: it needs a ROCm GPU.Scope
Left out, as follow-ups:
--timeoutbounds 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.checkpasses.linuxcommand. It checks the platforms inos, but a container is alwayslinux, so amacOS-only declaration passes there and fails underclaude-code.checkhooks inevals/hooks.pystay refused. ROCm/rocm-systems'decode-and-triageuses one, and replacing it needs its own design.