From b213e64e1f0a909257013150ec6f1514f7c00763 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Mon, 26 Oct 2026 09:48:46 +0000 Subject: [PATCH 1/3] Let a skill declare its prerequisites once, for both engines 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 --- .github/workflows/selftest.yml | 42 + README.md | 5 +- docs/authoring-evals.md | 104 ++- docs/usage.md | 4 +- pyproject.toml | 3 +- skillscope/cli.py | 13 +- skillscope/datasets.py | 234 ++++- skillscope/engine/behavioral.py | 115 ++- skillscope/engine/hooks.py | 22 +- skillscope/engine/no_sandbox.py | 21 + skillscope/engine/prerequisites.py | 434 +++++++++ skillscope/engine/verify.py | 61 +- skillscope/schema/machine.schema.json | 56 +- skillscope/structure.py | 2 +- tests/test_skillscope.py | 1161 ++++++++++++++++++++++++- tools/sandbox_smoketest.py | 89 +- 16 files changed, 2269 insertions(+), 97 deletions(-) create mode 100644 skillscope/engine/prerequisites.py diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml index d87667b..7e7eff2 100644 --- a/.github/workflows/selftest.yml +++ b/.github/workflows/selftest.yml @@ -119,6 +119,38 @@ jobs: ] } EOF + # A skill that declares a prerequisite: `install` builds a file that + # `check` then finds through the env the agent is handed. + mkdir -p fixture/prereq-skill/evals + cat > fixture/prereq-skill/SKILL.md <<'EOF' + --- + name: prereq-skill + description: Needs a tool its container does not ship, for a test that provides it. + --- + EOF + cat > fixture/prereq-skill/evals/install.sh <<'EOF' + echo built > "$SKILL_CACHE_DIR/tool.txt" + EOF + cat > fixture/prereq-skill/evals/machine.yml <<'EOF' + prerequisites: + - name: tool + check: [sh, -c, 'test -f "${PREREQ_TOOL:-/nonexistent}"'] + install: [sh, install.sh] + env: + PREREQ_TOOL: "${SKILL_CACHE_DIR}/tool.txt" + EOF + cat > fixture/prereq-skill/evals/evals.json <<'EOF' + { + "evaluations": [ + {"id": "prereq-a", "skill_should_trigger": true, "prompt": "use the tool", + "files_exist": ["prerequisites-ok.txt"]}, + {"id": "prereq-b", "skill_should_trigger": true, "prompt": "another way to ask"}, + {"id": "prereq-c", "skill_should_trigger": true, "prompt": "a third phrasing"}, + {"id": "prereq-d", "skill_should_trigger": false, "prompt": "something adjacent"}, + {"id": "prereq-e", "skill_should_trigger": false, "prompt": "something else entirely"} + ] + } + EOF # No key, no model, no agent: a stub solver stands in for the agent so # only the machinery beneath it can pass the case. That is what makes @@ -133,6 +165,16 @@ jobs: python tools/sandbox_smoketest.py fixture --skill demo-skill --expect-sandbox ${{ matrix.os == 'windows-latest' && 'local' || 'docker' }} + # The `claude-code` leg's prerequisite step in a real container: the + # checks, the install, the copied script and its removal, and the env + # reaching the agent factory. Linux only, because only a container has a + # guest for those to happen in. + - name: Provide a skill's prerequisites in the container + if: matrix.os == 'ubuntu-latest' + run: > + python tools/sandbox_smoketest.py fixture --skill prereq-skill + --expect-sandbox docker --prerequisites + action: name: Action against a throwaway repo runs-on: ubuntu-latest diff --git a/README.md b/README.md index f3600f0..ad315f1 100644 --- a/README.md +++ b/README.md @@ -93,8 +93,9 @@ python -m pip install -e . python -m unittest discover -s tests -t . ``` -Runtime dependencies are PyYAML for frontmatter, `inspect-ai` for the eval -framework both engines are built on, and `anthropic` for the model client. +Runtime dependencies are PyYAML for frontmatter and `evals/machine.yml`, +`inspect-ai` for the eval framework both engines are built on, and `anthropic` +for the model client. `inspect-ai` became required when the last stdlib-only engine was removed; the test suite still runs on the standard library alone, and builds a throwaway repo in a temp directory rather than reading the tree it runs in. diff --git a/docs/authoring-evals.md b/docs/authoring-evals.md index 25a6b19..e08576b 100644 --- a/docs/authoring-evals.md +++ b/docs/authoring-evals.md @@ -129,8 +129,10 @@ Two optional files sit beside the dataset. ### `evals/machine.yml` -Only for a skill whose behavioral cases cannot run on the everyday runners on -every platform. Both keys are optional: +Only for a skill whose behavioral cases need something the everyday runners +do not give them: a narrower set of platforms, particular hardware, a sandbox of +its own (`sandbox:`, see [usage.md](usage.md#where-a-sandboxed-run-is-sandboxed)), +or [prerequisites](#prerequisites). Every key is optional: ```yaml os: [Linux] # defaults to whatever platforms the repo runs on @@ -146,6 +148,86 @@ moves the leg onto that pool. Most skills that need this file need only `os: [Linux]`, to drop a Windows leg that would just exercise the failure path of Linux-only tooling. +#### Prerequisites + +A skill whose behavioral cases need something the runner may not have, such as +a library to build or a package to install, declares it here once. Every engine +applies the same declaration where its agent runs. Routing never does: it runs +no case to completion. + +```yaml +os: [Linux] +prerequisites: + - name: rocprof-trace-decoder + check: [python3, -c, "import elftools, rocprof_trace_decoder"] + install: [sh, install-decoder.sh] # beside this file + env: + PYTHONPATH: "${SKILL_CACHE_DIR}/decoder/python" + ROCPROF_TRACE_DECODER_LIB: "${SKILL_CACHE_DIR}/decoder/lib/librocprof-trace-decoder.so" +``` + +- **The order is check, then install, then check again.** + - `check` runs first, without this prerequisite's `env`. If it exits 0, the + prerequisite is already there: nothing is installed and its `env` is not + applied, because whatever provides it already says where it is. + - Otherwise `install` runs, then `check` runs again with `env` applied. If it + still fails, the skill's cases are reported as failed, with the step's + output. + - Prerequisites run in order, and each one's steps see the `env` of the ones + before it that had to be installed. +- **When it runs depends on the engine.** + - The default engine runs it once per skill, on the host, before the first + case. + - `--engine claude-code` runs it in each case's container, before the agent + starts, so the container needs whatever `install` uses: network access for + a download, which the default sandbox does not have, and a user that can + write to `/opt/skillscope`. +- **Commands are argv lists.** A command may also be a mapping per platform, + for example `{linux: [sh, x.sh], windows: [pwsh, x.ps1]}`. A container + always counts as `linux`, and the structural check wants a command for every + platform in `os`. On the host, steps run in this directory. In a container + they run in a scratch directory holding only the files they name. +- **What a container gets.** Under `claude-code`, an argv item of `check` or + `install` that is a path to a file in this directory or below it, like + `install-decoder.sh` above, is copied into the container for the step, at the + same relative path, and removed before the agent starts. A file mentioned + only inside a string, as in `sh -c "..."`, is not. A copied file is not + executable there, so run a script through its interpreter (`[sh, install.sh]`, + not `[./install.sh]`). A step that names `evals.json` or `extended_evals.json` + is refused, and the skill's cases fail, rather than carry the answer key into + the agent's room. +- **No step has a time limit of its own.** On the host only `--timeout` stops + a hung install. It stops the processes the install started as well, whether + it stops the run gracefully or kills it outright. In a container the case's + own time limit does, which `--timeout` sets; without one, nothing does. A + host step also waits for everything holding its output open, so a step that + leaves a server running in the background does not finish. +- **Steps see two variables, and `env` values may refer to these two only.** + - `SKILL_SOURCE_DIR` is the source tree the skill came from, resolved the + way `sources.resolve` does (below). It is host only: a container has no + copy of it, and on the host it is unset if no tree can be found. An `env` + value naming it fails before anything is installed when it is missing. + - `SKILL_CACHE_DIR` is scratch space for what `install` builds. On the host + it is removed after the skill's last case. In a container it is + `/opt/skillscope/cache`. +- **An `env` value replaces the variable, it does not extend it.** No other + `$` is expanded, so `PATH: ${SKILL_CACHE_DIR}/bin` would leave the agent with + that one directory. Variables the harness sets for the agent + (`ANTHROPIC_*`, `CLAUDE_*`, `IS_SANDBOX`) cannot be set here. +- **Installing once per image instead of once per case.** Under `claude-code`, + build your `sandbox:` image so that `check` passes in it without this + prerequisite's `env`: run the same `install` at `SKILL_CACHE_DIR=/opt/skillscope/cache` + and set the same `env` in the image, or install wherever the image's own + environment already finds it. A source tree the install needs has to be put + into the image as well. +- **What it sets stays with its skill.** The `env` reaches only its own skill's + agent, and `SKILL_CACHE_DIR` is removed after the skill. Anything `install` + writes elsewhere on the host, such as a pip cache, is not cleaned up. +- **Steps are trusted like the rest of the skill.** On the host they run with + the runner's whole environment, credentials included, as `evals/hooks.py` + does; in a container they see only the container's. A failing step's output + ends up in the report. + ### `evals/hooks.py` Setup a dataset cannot express: cloning a repo, tearing down a container, @@ -186,15 +268,17 @@ def setup_session(cache_dir): ... # NOT supported def check(run, case, ctx): ... # NOT supported ``` -`check` was handed the retired engine's `Run` object; the engines here have -inspect's `TaskState`, which is a different thing with different attributes. -`setup_session` returned `{name: value}` pairs substituted into prompts as -`{placeholders}`, and prompts are built before any hook runs -- so honouring it -would mean constructing every case after the hook rather than before. A `setup` -that *returns* placeholders is refused at runtime for the same reason. +`setup_session` is replaced by [prerequisites](#prerequisites). It ran once, +on the host, and anything it set reached the agent only through the host's +environment, so a skill that also ran under `claude-code` had to describe its +environment a second time for the container, and the two descriptions +drifted. Its `{placeholder}` return value has no replacement: nothing used it. +Put such values in the dataset instead. -If you were relying on either, move the work into `setup`/`teardown`, or put -the value in the dataset. +`check` was handed the retired engine's `Run` object. The engines here have +inspect's `TaskState`, which is a different thing with different attributes. +Move what it asserted into the dataset. A `setup` that *returns* placeholders +is refused at runtime, because prompts are built before any hook runs. Keep prompts and expectations in the dataset even when you use hooks, so what is being asserted stays readable without opening Python. diff --git a/docs/usage.md b/docs/usage.md index 442d1b1..e24e90c 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -322,7 +322,9 @@ membership. `sandbox: compose.yaml` in its `evals/machine.yml`, resolved beside it. Skills get a container with no network by default; one that installs a server or pulls a model cannot run that way and says so. Selecting a provider does not discard what a skill asked -for -- the compose file rides along. +for -- the compose file rides along. A skill that needs a tool installed first +declares it as [prerequisites](authoring-evals.md#prerequisites) in the same +file, and an install that downloads anything needs that egress too. [`examples/skill-with-a-device/evals/`](../examples/skill-with-a-device/evals) is the pair, worked through: a `machine.yml` that asks for GPU runners and diff --git a/pyproject.toml b/pyproject.toml index 3ab8e4e..df1c6a8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,7 +29,8 @@ authors = [{ name = "Advanced Micro Devices, Inc." }] # # `anthropic` is listed explicitly: inspect-ai treats every model provider as # optional, so installing it alone gets you a harness that cannot reach a model. -# PyYAML reads the optional evals/machine.yml, which only CI planning touches. +# PyYAML reads the optional evals/machine.yml: CI planning, and a behavioral +# run reading each skill's prerequisites. dependencies = [ "pyyaml>=6.0", "inspect-ai>=0.3.263", diff --git a/skillscope/cli.py b/skillscope/cli.py index 0b9da79..daabd5e 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -466,10 +466,11 @@ def _require_hook_support(engine: str, skills: list[str], command: str) -> None: " They would be skipped silently, and the cases graded as though " "they had run.\n" " `setup` and `teardown` are supported. `check` needs the legacy " - "engine's Run object, and `setup_session` returns template variables " - "that are substituted before any hook runs -- neither has an " - "equivalent here.\n" - " Move what they do into the dataset, or into setup/teardown." + "engine's Run object, which has no equivalent here; move what it " + "asserts into the dataset.\n" + " `setup_session` is replaced by `prerequisites:` in " + "evals/machine.yml: a check, an install and the env the agent needs, " + "applied the same way on every engine. See docs/authoring-evals.md." ) @@ -713,8 +714,8 @@ def cmd_behavioral(args: argparse.Namespace) -> int: gradable, engine_models.resolve(args.model), args.effort, - solver_factory=lambda d: no_sandbox.claude_code_no_sandbox( - args.model, args.effort, d + solver_factory=no_sandbox.behavioral_solver_factory( + args.model, args.effort ), ) else: diff --git a/skillscope/datasets.py b/skillscope/datasets.py index 250865c..e69bcb3 100644 --- a/skillscope/datasets.py +++ b/skillscope/datasets.py @@ -59,13 +59,15 @@ * an evaluation in the shared pool -> ``unrelated`` (belongs to no skill's domain) -Stdlib only, so a run needs no wheels beyond this package. ``machine_plan`` is -the one exception and imports PyYAML lazily; nothing on the run path calls it. +Stdlib only, apart from ``evals/machine.yml``: reading it imports PyYAML +lazily, so a module importing this one does not pay for it. A behavioral run +reads it for each skill's prerequisites, so PyYAML is a dependency. """ from __future__ import annotations import json +import re from collections import Counter from dataclasses import dataclass, field from pathlib import Path @@ -497,7 +499,12 @@ def structural_errors(skills: list[str] | None = None) -> list[str]: # runners, which hangs rather than failing. Surface it here, where it # costs seconds. try: - machine_plan(skill) + plan = machine_plan(skill) + # Here rather than at run time for the same reason: a prerequisite + # that cannot be parsed, or has no command for a platform the skill + # runs on, would otherwise surface as every case of the skill + # failing, after paying for the runner. + errors.extend(_prerequisite_gaps(skill, plan, machine_prerequisites(skill))) except SystemExit as exc: errors.append(str(exc).removeprefix("error: ")) return errors @@ -535,21 +542,66 @@ def tier0_errors(skill: str, cases: list[Case]) -> list[str]: return errors -MACHINE_KEYS = {"os", "labels", "sandbox"} +MACHINE_KEYS = {"os", "labels", "sandbox", "prerequisites"} + +PREREQUISITE_KEYS = {"name", "check", "install", "env"} +# Keys a per-OS command may use, matched against the platform a step runs on. +# A container is always `linux`, whatever the host is. +COMMAND_PLATFORMS = ("linux", "windows", "macos") +# The only variables an `env` value may name. Anything else would be read from +# whichever process applied the prerequisite, and that differs between the +# host and a container -- the same declaration would mean two things. +PREREQUISITE_INPUTS = ("SKILL_SOURCE_DIR", "SKILL_CACHE_DIR") +_ENV_INPUT = re.compile(r"\$\{(?:SKILL_SOURCE_DIR|SKILL_CACHE_DIR)\}") +# Variables the harness sets for the agent itself. A prerequisite overriding +# one would change which model answers or where its config lives -- and +# differently per engine, since each sets a different subset. +_RESERVED_ENV = re.compile(r"^(?:ANTHROPIC_|CLAUDE_)|^IS_SANDBOX$", re.IGNORECASE) +# A name a process environment can hold on every platform. +_ENV_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +# `machine.yml`'s `os` labels as `COMMAND_PLATFORMS`. +_OS_PLATFORM = {"linux": "linux", "windows": "windows", "macos": "macos"} + + +@dataclass(frozen=True) +class Prerequisite: + """Something a skill's behavioral cases need, and how to provide it. + + `check` and `install` map a platform from `COMMAND_PLATFORMS` -- or `"*"` + for every platform -- to an argv. `env` is what the agent needs set to + use the prerequisite once it is installed, with `${SKILL_SOURCE_DIR}` and + `${SKILL_CACHE_DIR}` still unexpanded: what they expand to depends on + where the prerequisite is applied. + """ + + name: str + check: dict[str, list[str]] + install: dict[str, list[str]] + env: dict[str, str] = field(default_factory=dict) + + def command(self, step: str, platform: str) -> list[str] | None: + """The argv for `step` on `platform`, or None if none applies there.""" + commands = getattr(self, step) + return commands.get(platform) or commands.get("*") def _read_machine(skill: str) -> dict: """The raw ``evals/machine.yml`` for `skill`, or ``{}`` when it has none. - PyYAML is imported here rather than at module scope: nothing on the run - path needs this, so a run stays dependency-free. + PyYAML is imported here rather than at module scope, so importing this + module stays cheap for everything that never reads a machine.yml. """ path = machine_path(skill) if not path.is_file(): return {} - import yaml # noqa: PLC0415 -- keeps the run path stdlib-only + import yaml # noqa: PLC0415 -- importing this module stays cheap - data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + try: + data = yaml.safe_load(path.read_text(encoding="utf-8")) or {} + except yaml.YAMLError as exc: + # As SystemExit, which every per-skill handler turns into that skill's + # failure; a parser exception would end the whole command instead. + raise SystemExit(f"error: {path} is not valid YAML: {exc}") from None if not isinstance(data, dict): raise SystemExit(f"error: {path} must be a YAML mapping.") return data @@ -596,16 +648,17 @@ def machine_plan(skill: str) -> dict: which pull-request label rations the pool, which environment holds its credentials -- is the repo's business and comes from the workflow, not from here. Returns ``{os, labels}``, raising SystemExit on a malformed file so - CI stops at planning. + CI stops at planning. ``prerequisites``, the fourth key, is read by + `machine_prerequisites`. """ path = machine_path(skill) data = _read_machine(skill) - unknown = sorted(set(data) - MACHINE_KEYS) + unknown = sorted(set(data) - MACHINE_KEYS, key=str) if unknown: raise SystemExit( - f"error: {path}: unknown key(s): {', '.join(unknown)}. " - f"A machine.yml holds only {' and '.join(sorted(MACHINE_KEYS))}." + f"error: {path}: unknown key(s): {', '.join(map(str, unknown))}. " + f"A machine.yml holds only {', '.join(sorted(MACHINE_KEYS))}." ) platforms = ( @@ -615,3 +668,160 @@ def machine_plan(skill: str) -> dict: ) labels = _string_list(path, "labels", data["labels"]) if "labels" in data else [] return {"os": platforms, "labels": labels} + + +def _argv(path: Path, where: str, value: object) -> list[str]: + """An argv as written. Not stripped: `sh -c` bodies keep their newlines.""" + if ( + not isinstance(value, list) + or not value + or not all(isinstance(item, str) for item in value) + or not value[0].strip() + ): + raise SystemExit( + f"error: {path}: `{where}` must be a non-empty list of strings; " + f"got {value!r}." + ) + return list(value) + + +def _command(path: Path, where: str, value: object) -> dict[str, list[str]]: + """An argv, or a mapping from platform to argv, as `{platform: argv}`.""" + if isinstance(value, list): + return {"*": _argv(path, where, value)} + if isinstance(value, dict) and value: + unknown = sorted(set(value) - set(COMMAND_PLATFORMS), key=str) + if unknown: + raise SystemExit( + f"error: {path}: `{where}` names unknown platform(s): " + f"{', '.join(map(str, unknown))}. Use " + f"{', '.join(COMMAND_PLATFORMS)}." + ) + return { + platform: _argv(path, f"{where}.{platform}", argv) + for platform, argv in value.items() + } + raise SystemExit( + f"error: {path}: `{where}` must be a command (a list of strings) or a " + f"mapping from platform to command; got {value!r}." + ) + + +def _prerequisite_env(path: Path, where: str, value: object) -> dict[str, str]: + if not isinstance(value, dict) or not all( + isinstance(k, str) and k and isinstance(v, str) for k, v in value.items() + ): + raise SystemExit( + f"error: {path}: `{where}` must map variable names to strings; " + f"got {value!r}." + ) + for name, text in value.items(): + if not _ENV_NAME.match(name) or "\0" in text: + raise SystemExit( + f"error: {path}: `{where}` has an entry that cannot be set in a " + f"process environment: {name!r}. Names are letters, digits and " + "underscores, not starting with a digit; values hold no NUL." + ) + if name in PREREQUISITE_INPUTS: + raise SystemExit( + f"error: {path}: `{where}.{name}` names an input skillscope " + "supplies to every step; a prerequisite may not override it." + ) + if _RESERVED_ENV.search(name): + raise SystemExit( + f"error: {path}: `{where}.{name}` is set by the harness for the " + "agent itself, and a prerequisite may not override it." + ) + if "$" in _ENV_INPUT.sub("", text): + raise SystemExit( + f"error: {path}: `{where}.{name}` uses `$` other than in " + f"{' or '.join('${' + n + '}' for n in PREREQUISITE_INPUTS)}. " + "Nothing else is expanded: any other variable would be read " + "from whichever process applies the prerequisite, which is not " + "the same one on every engine, and a value replaces the " + "variable rather than extending it." + ) + return dict(value) + + +def _prerequisite_gaps( + skill: str, plan: dict, declared: list[Prerequisite] +) -> list[str]: + """Each platform the skill runs on that a prerequisite has no command for.""" + gaps: list[str] = [] + for label in plan["os"]: + platform = _OS_PLATFORM.get(label.lower()) + if platform is None: + continue + for item in declared: + missing = [ + step for step in ("check", "install") if item.command(step, platform) is None + ] + if missing: + gaps.append( + f"{machine_path(skill)}: prerequisite `{item.name}` has no " + f"{' or '.join(missing)} command for {label}, which this " + "skill runs on. Add one, or narrow `os`." + ) + return gaps + + +def machine_prerequisites(skill: str) -> list[Prerequisite]: + """What `skill`'s behavioral cases need provided before they run. + + Declared in ``evals/machine.yml`` and applied by every engine, each where + its agent runs -- once per skill on the host, in each case's container + under ``claude-code``. One declaration rather than a hook per engine, so + the two cannot drift apart:: + + prerequisites: + - name: trace-decoder + check: [python3, -c, "import rocprof_trace_decoder"] + install: [sh, install-decoder.sh] + env: + PYTHONPATH: "${SKILL_CACHE_DIR}/decoder/python" + + Returns ``[]`` for a skill that declares none, and raises SystemExit on a + malformed declaration so the structural check stops it. + """ + path = machine_path(skill) + data = _read_machine(skill) + if "prerequisites" not in data: + return [] + declared = data["prerequisites"] + # Present but null -- every item commented out -- is a mistake, not "none". + if not isinstance(declared, list) or not declared: + raise SystemExit( + f"error: {path}: `prerequisites` must be a non-empty list; " + f"got {declared!r}." + ) + + found: list[Prerequisite] = [] + for index, entry in enumerate(declared): + where = f"prerequisites[{index}]" + if not isinstance(entry, dict): + raise SystemExit(f"error: {path}: `{where}` must be a mapping.") + unknown = sorted(set(entry) - PREREQUISITE_KEYS, key=str) + missing = sorted({"name", "check", "install"} - set(entry)) + if unknown or missing: + raise SystemExit( + f"error: {path}: `{where}` " + + (f"has unknown key(s): {', '.join(map(str, unknown))}. " if unknown else "") + + (f"is missing {', '.join(missing)}. " if missing else "") + + "A prerequisite has `name`, `check` and `install`, and " + "optionally `env`." + ) + name = entry["name"] + if not isinstance(name, str) or not name.strip(): + raise SystemExit(f"error: {path}: `{where}.name` must be a non-empty string.") + if any(p.name == name.strip() for p in found): + raise SystemExit(f"error: {path}: prerequisite `{name}` is declared twice.") + found.append( + Prerequisite( + name=name.strip(), + check=_command(path, f"{where}.check", entry["check"]), + install=_command(path, f"{where}.install", entry["install"]), + env=_prerequisite_env(path, f"{where}.env", entry.get("env", {})), + ) + ) + return found diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index 06ca60f..f667c47 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -22,7 +22,15 @@ from .. import agent, config, deadline, routing as routing_core from ..behavior import BehaviorOutcome from ..datasets import Case -from . import convert, hooks, models, sandbox as sandbox_spec, scorers, stats +from . import ( + convert, + hooks, + models, + prerequisites, + sandbox as sandbox_spec, + scorers, + stats, +) # An agent that never decides it is finished must still stop. The legacy engine # bounded this with `--case-timeout` and a process kill; inspect expresses it @@ -85,13 +93,16 @@ def build_task( ctx: dict | None = None, *, solver_factory, + env: dict[str, str] | None = None, ): """One inspect `Task` per skill: its cases, its skill installed, its scorer. `solver_factory` is what drives the agent; everything around it -- scoring, judging, reporting -- stays the same whichever one is passed. It receives the skill's directory because staging is the driver's job: each driver puts - the skill where the agent it runs will look for it. + the skill where the agent it runs will look for it. It also receives `env`, + what the skill's prerequisites need set for the agent (see + `prerequisites.on_host`), because the driver is what starts the agent. Required, and keyword-only. It was optional while a built-in react agent was the default, and a caller that forgot it silently graded a different @@ -113,7 +124,7 @@ def build_task( name=f"behavioral-{skill}", dataset=samples, setup=hooks.setup_solver(hook, skill, cases), - solver=solver_factory(skill_dir), + solver=solver_factory(skill_dir, dict(env or {})), cleanup=hooks.cleanup_fn(hook, skill, cases), scorer=scorers.expectations(), sandbox=sandbox_spec.for_skill(skill), @@ -122,8 +133,15 @@ def build_task( ) -def _failed(skill: str, cases: list[Case], detail: str) -> list[BehaviorOutcome]: - """One failed outcome per case, for a skill that could not be run at all.""" +def _failed( + skill: str, cases: list[Case], detail: str, degraded: bool | None = None +) -> list[BehaviorOutcome]: + """One failed outcome per case, for a skill that could not be run at all. + + `degraded` defaults to whether `detail` reads as a provider outage. Pass + it where the cause is known: a failed install's output is a build log, and + a build log is full of the words that heuristic looks for. + """ return [ BehaviorOutcome( id=case.id, @@ -132,12 +150,29 @@ def _failed(skill: str, cases: list[Case], detail: str) -> list[BehaviorOutcome] passed=False, elapsed_s=0.0, error=detail, - degraded=routing_core.is_provider_error(detail), + degraded=( + routing_core.is_provider_error(detail) if degraded is None else degraded + ), ) for case in cases ] +def _is_outage(sample_error, detail: str | None) -> bool: + """Whether a case's error reads as the provider failing rather than the case. + + Not for a prerequisite that could not be provided in the case's container: + its message carries the install's output, and a build log is full of the + words the provider heuristic looks for. The same failure on the host is + reported with `degraded=False` directly. + """ + # Qualified: inspect has a `PrerequisiteError` of its own, which is not this. + qualified = f"{prerequisites.PrerequisiteError.__module__}.PrerequisiteError" + if qualified in (getattr(sample_error, "traceback", None) or ""): + return False + return routing_core.is_provider_error(detail) + + def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: """Map one inspect `EvalLog` back onto skillscope's outcome objects. @@ -182,7 +217,7 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: elapsed_s=round(getattr(sample, "total_time", None) or 0.0, 2), checks=checks, error=error, - degraded=routing_core.is_provider_error(error), + degraded=_is_outage(sample.error, error), ) ) @@ -313,33 +348,57 @@ def run( print(f"[behavioral] {skill}: {len(skill_cases)} case(s)", flush=True) try: - logs = run_eval( - inspect_eval, - build_task(skill, skill_cases, model, solver_factory=solver_factory), - model=model, - model_args=models.model_args(model), - log_dir=log_dir or str(Path(".skillscope") / "logs"), - log_realtime=realtime_logging(), - # A sample that raises is that case's failure, not the batch's. - # inspect's default ends the task on the first error, so one - # container that would not start discarded every case already - # graded -- `_outcomes` then reported the whole skill failed. - # Errored samples still come back in the log and are still - # graded as failures; what changes is that their neighbours - # survive. - fail_on_error=False, - # skillscope's own progress lines are the report; inspect's rich - # display takes over the terminal and produces nothing useful - # when a CI job pipes stdout to a file. - display="plain", + # Around the whole task rather than per case: on the host one + # install serves every case, and what it built is removed once the + # last of them is graded. + with prerequisites.on_host(skill) as env: + logs = run_eval( + inspect_eval, + build_task( + skill, skill_cases, model, solver_factory=solver_factory, env=env + ), + model=model, + model_args=models.model_args(model), + log_dir=log_dir or str(Path(".skillscope") / "logs"), + log_realtime=realtime_logging(), + # A sample that raises is that case's failure, not the batch's. + # inspect's default ends the task on the first error, so one + # container that would not start discarded every case already + # graded -- `_outcomes` then reported the whole skill failed. + # Errored samples still come back in the log and are still + # graded as failures; what changes is that their neighbours + # survive. + fail_on_error=False, + # skillscope's own progress lines are the report; inspect's + # rich display takes over the terminal and produces nothing + # useful when a CI job pipes stdout to a file. + display="plain", + ) + except prerequisites.PrerequisiteError as exc: + # The skill could not be given what it needs, so none of its cases + # were run -- the same disposition as a broken sandbox declaration. + # Not a provider outage, whatever a build log happens to contain. + outcomes.extend( + _failed(skill, skill_cases, f"{skill}: {exc}", degraded=False) ) + continue + except KeyboardInterrupt: + # The deadline's graceful stop landing in a host install, outside + # inspect, which would otherwise have turned it into a cancel. Fail + # this skill and keep everything already graded. A Ctrl-C from a + # person still stops the command. + if not stop_requested(): + raise + outcomes.extend(_failed(skill, skill_cases, deadline.active().message())) + continue except SystemExit as exc: # One skill's broken setup is that skill's failure, not everybody's. # A malformed sandbox declaration used to abort the whole command, # throwing away results for skills already graded and paid for -- # the same reason the structural gate reads only the skills a run - # is about. - outcomes.extend(_failed(skill, skill_cases, str(exc))) + # is about. A declaration that does not parse is never a provider + # outage, whatever the path in its message happens to contain. + outcomes.extend(_failed(skill, skill_cases, str(exc), degraded=False)) continue if not logs and stop_requested(): diff --git a/skillscope/engine/hooks.py b/skillscope/engine/hooks.py index 2807942..7425c1e 100644 --- a/skillscope/engine/hooks.py +++ b/skillscope/engine/hooks.py @@ -42,18 +42,16 @@ thing with different attributes; passing one where the other is expected would fail inside somebody's skill with an error about their code. -`setup_session(cache_dir)` returns `{name: value}` pairs that `expand()` -substitutes into prompts and expectations. Those are baked into the `Sample` -before any solver runs, so honouring it means building the dataset after the -hook rather than before -- a change to how every case is constructed, for a -feature nothing in the catalogue uses. - -A `setup` that *returns* template variables has the same problem, and is caught -at runtime rather than by inspection because a docstring cannot say what a -function returns. - -Nothing in the catalogue this was written against uses either: one skill ships -a hook, it defines `setup` and `teardown` only, and neither returns anything. +`setup_session(cache_dir)` is replaced by `prerequisites:` in +`evals/machine.yml` (see `prerequisites.py`). It ran once, on the host, and +what it set reached the agent only through this process's environment, so a +skill that also ran under `claude-code` had to describe its environment again +for the container, and the two descriptions drifted. A declaration both +engines read cannot. + +A `setup` that *returns* template variables is refused at runtime: prompts and +expectations are baked into the `Sample` before any solver runs, and a +docstring cannot say what a function returns. """ from __future__ import annotations diff --git a/skillscope/engine/no_sandbox.py b/skillscope/engine/no_sandbox.py index 8d31bae..41c0ff7 100644 --- a/skillscope/engine/no_sandbox.py +++ b/skillscope/engine/no_sandbox.py @@ -396,6 +396,20 @@ def _record_room(state, event: dict, room_names: list[str] | None) -> None: state.store.set(EXTRA_SKILLS_KEY, routing_core.init_extra_skills(event, room_names)) +def behavioral_solver_factory(model: str | None, effort: str | None): + """The `solver_factory` the behavioral leg is run with on this engine. + + `behavioral.build_task` calls it per skill with the skill's directory and + the env its prerequisites produced; both go to the solver, the env as + `extra_env` so it reaches this skill's agent and no other. + """ + + def factory(skill_dir: Path, env: dict[str, str]): + return claude_code_no_sandbox(model, effort, skill_dir, extra_env=env) + + return factory + + def claude_code_no_sandbox( model: str | None, effort: str | None, @@ -404,9 +418,15 @@ def claude_code_no_sandbox( extra_flags: list[str] | None = None, stop_when_factory: Callable[[], Callable[[dict], str | None]] | None = None, room_names: list[str] | None = None, + extra_env: dict[str, str] | None = None, ): """Solver: install the skill, run the real CLI once, record what it did. + `extra_env` is set for the CLI on top of this process's environment: what + the skill's prerequisites need the agent to see (`prerequisites.on_host`). + Passed here rather than written into ``os.environ``, so it reaches this + skill's agent and nothing else. + `extra_flags` are the CLI's own controls -- `--max-budget-usd` and `--no-session-persistence` -- built by `routing.host_cost_flags`, which probes `--help` first: an older build rejects an unknown flag and every @@ -465,6 +485,7 @@ async def solve(state, generate): cmd += list(extra_flags or []) env = legacy_agent.claude_env() + env.update(extra_env or {}) if config_dir is not None: env["CLAUDE_CONFIG_DIR"] = str(config_dir) diff --git a/skillscope/engine/prerequisites.py b/skillscope/engine/prerequisites.py new file mode 100644 index 0000000..031315e --- /dev/null +++ b/skillscope/engine/prerequisites.py @@ -0,0 +1,434 @@ +# Copyright Advanced Micro Devices, Inc. +# +# SPDX-License-Identifier: MIT + +"""Provide what a skill's behavioral cases need, where its agent runs. + +A skill declares its prerequisites once, in ``evals/machine.yml`` (parsed by +`datasets.machine_prerequisites`), and every engine applies that one +declaration. The alternative it replaces -- a Python ``setup_session`` hook -- +could only prepare the host, so a skill that also ran under ``claude-code`` +had to describe its environment a second time in a compose file, and the two +descriptions drifted. + +Each prerequisite is a ``check`` and an ``install`` command plus the ``env`` +the agent needs once it is installed. The rule is the same on every engine, +and lives in `provide` so it cannot differ between them: + +* ``check`` exits 0: the prerequisite is already there. Nothing is installed + and its ``env`` is not applied -- the environment that provides it already + says where it is. +* Otherwise ``install`` runs, then ``check`` again *with* the ``env`` applied, + because an installed tool is usually only findable through it. Still failing + is the prerequisite's failure, reported with the step's own output. + +Steps see two inputs, and ``env`` values may name only these: + +* ``SKILL_SOURCE_DIR`` -- the skill's source tree, as `sources.resolve` finds + it. Host only, and unset there too when no tree can be found: a container + has neither the tree nor, by default, a network to fetch it with. A step + that needs it sees it unset; an ``env`` value naming it fails before + anything is installed, with the reason it is missing. +* ``SKILL_CACHE_DIR`` -- scratch space for what ``install`` builds. Per skill + on the host, removed after its cases; a fixed path in each case's container, + so an image that bakes the same ``install`` in at that path makes ``check`` + pass and skips the per-case cost. + +Where each engine runs it: + +* ``claude-code-no-sandbox``: once per skill, before its task is built, in + `on_host`. The resulting ``env`` reaches the agent through the solver, not + through ``os.environ`` -- nothing a skill installs is visible to the next. +* ``claude-code``: in each case's container, before the agent, in + `claude_code_agent`, because inspect starts one container per case. +""" + +from __future__ import annotations + +import asyncio +import contextlib +import os +import platform +import shutil +import signal +import subprocess +import tempfile +from collections.abc import Awaitable, Callable, Iterator +from pathlib import Path + +from .. import datasets, deadline, sources +from . import no_sandbox +from ..datasets import Prerequisite + +# Where a container keeps the inputs. Fixed rather than random so that an image +# can bake an `install` in at the same `SKILL_CACHE_DIR` and have `check` find +# it; documented in docs/authoring-evals.md, so changing it breaks those images. +CONTAINER_ROOT = "/opt/skillscope" +CONTAINER_CACHE = f"{CONTAINER_ROOT}/cache" +# Where the files a step names are copied, and removed again before the agent +# starts: evals/ also holds the answer key, and only what a step names leaves. +CONTAINER_STEPS = f"{CONTAINER_ROOT}/steps" + +# How much of a failing step's output the report carries. The end of a build +# log is where its error is; the start is usually configuration chatter. +OUTPUT_TAIL = 2000 + +Runner = Callable[[list[str], dict[str, str]], Awaitable[tuple[int, str]]] + + +class PrerequisiteError(Exception): + """A prerequisite could not be provided. Fails the cases that needed it.""" + + def __init__(self, message: str) -> None: + super().__init__(message) + # inspect reports a sample's error by `.message` where there is one and + # by `repr()` where there is not, which would put escaped newlines and + # the class name in the report instead of the step's output. + self.message = message + + +def host_platform() -> str: + """This host as one of `datasets.COMMAND_PLATFORMS`.""" + name = platform.system().lower() + return {"darwin": "macos"}.get(name, name) + + +def expand( + env: dict[str, str], inputs: dict[str, str], unavailable: dict[str, str] +) -> dict[str, str]: + """`env` with ``${SKILL_SOURCE_DIR}`` and ``${SKILL_CACHE_DIR}`` filled in. + + The declaration was checked to name nothing else, so an input missing from + `inputs` is one this engine could not supply, and `unavailable` says why. + """ + expanded: dict[str, str] = {} + for name, value in env.items(): + for key in datasets.PREREQUISITE_INPUTS: + token = "${" + key + "}" + if token in value: + if key not in inputs: + raise PrerequisiteError( + f"`{name}` refers to {token}, which is not available " + f"here: {unavailable.get(key, 'nothing provides it')}" + ) + value = value.replace(token, inputs[key]) + expanded[name] = value + return expanded + + +def _tail(output: str) -> str: + output = output.strip() + return output if len(output) <= OUTPUT_TAIL else "..." + output[-OUTPUT_TAIL:] + + +async def provide( + declared: list[Prerequisite], + inputs: dict[str, str], + where: str, + run: Runner, + unavailable: dict[str, str] | None = None, +) -> dict[str, str]: + """Apply `declared` in order; return the env the agent needs. + + `where` is the platform the steps run on, and `run` runs one argv with + extra environment, returning its exit code and combined output. Each + prerequisite's steps see the env of the ones before it, so a later one can + build on an earlier one. `unavailable` explains any input missing from + `inputs`, for the error raised if an `env` value names it. + """ + applied: dict[str, str] = {} + for item in declared: + check = item.command("check", where) + install = item.command("install", where) + if check is None or install is None: + raise PrerequisiteError( + f"prerequisite `{item.name}` declares no command for {where}." + ) + step_env = {**inputs, **applied} + code, _ = await run(check, step_env) + if code == 0: + continue + # Before the install rather than after it: an `env` this engine cannot + # fill should not cost a full build to discover. + env = expand(item.env, inputs, unavailable or {}) + code, output = await run(install, step_env) + if code != 0: + # The step's own output first: a report keeps only the start of + # this. Then, for an input the step mentions and this engine could + # not supply, why it was unset -- not for every failed install. + said = " ".join(install) + output + missing = "".join( + f"\n({name} was not available: {why})" + for name, why in (unavailable or {}).items() + if name in said + ) + raise PrerequisiteError( + f"prerequisite `{item.name}`: install exited {code}: " + f"{_tail(output)}{missing}" + ) + code, output = await run(check, {**step_env, **env}) + if code != 0: + raise PrerequisiteError( + f"prerequisite `{item.name}`: still not present after install " + f"(check exited {code}).\n{_tail(output)}" + ) + applied.update(env) + return applied + + +# What the `--timeout` watchdog's hard exit has to clean up, because `os._exit` +# reaches neither the `finally` below nor the kill in `_host_runner`. Same +# pattern as `hooks.py`. +_created_caches: set[str] = set() +# Host steps still running, by pid. Each leads a session or process group of +# its own (see `_host_runner`), so the pid names everything it started too. +_live_steps: set[int] = set() + + +def _discard_all_caches() -> None: + for path in list(_created_caches): + shutil.rmtree(path, ignore_errors=True) + _created_caches.clear() + + +def _kill_live_steps() -> None: + """Kill every running host step and what it started. For the watchdog. + + Synchronous, because the watchdog fires from a thread with no event loop. + """ + for pid in list(_live_steps): + try: + if os.name == "nt": + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(pid)], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + timeout=5, + ) + else: + os.killpg(pid, signal.SIGKILL) + except (OSError, subprocess.SubprocessError): + pass + _live_steps.discard(pid) + + +def _on_expire() -> None: + # Steps first: a build still running would go on writing into the cache + # being removed. + _kill_live_steps() + _discard_all_caches() + + +deadline.on_expire(_on_expire) + + +def _host_runner(cwd: Path) -> Runner: + async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: + # Asynchronous rather than `subprocess.run`, and stopped on the way + # out, because a build is the slowest thing a run does outside inspect: + # the deadline's graceful stop interrupts the main thread, and a + # blocking call would sit out the whole build before noticing -- long + # enough for the hard kill to take the report with it. + # + # In a session of its own, so the stop reaches what the step started + # too: an install is usually a script running a build, and the build + # would otherwise go on writing into a cache that is being removed. + spawn: dict = {} + if os.name == "nt": + spawn["creationflags"] = subprocess.CREATE_NEW_PROCESS_GROUP + else: + spawn["start_new_session"] = True + env = { + name: value + for name, value in os.environ.items() + # Only what this run resolved: a value left in the runner's own + # environment would stand in for an input this run could not find. + if name not in datasets.PREREQUISITE_INPUTS + } + try: + proc = await asyncio.create_subprocess_exec( + *argv, + cwd=cwd, + env={**env, **extra}, + stdin=asyncio.subprocess.DEVNULL, + stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.STDOUT, + **spawn, + ) + except (OSError, ValueError) as exc: + return 127, f"could not start {argv[0]!r}: {exc}" + _live_steps.add(proc.pid) + try: + output, _ = await proc.communicate() + except BaseException: + # Shielded: this runs while the step is being cancelled, and the + # kill has to finish rather than be cancelled in turn. + await asyncio.shield(no_sandbox._terminate(proc)) + raise + finally: + _live_steps.discard(proc.pid) + return proc.returncode, output.decode(errors="replace") + + return run + + +@contextlib.contextmanager +def on_host(skill: str) -> Iterator[dict[str, str]]: + """Provide `skill`'s prerequisites on this host for the length of the block. + + Yields the env its agent needs -- empty for a skill that declares none, + without creating anything. Raises `PrerequisiteError` before yielding if + one cannot be provided. + ``os.environ`` is never changed: the env goes to the agent through its + solver, so nothing provided here leaks into the next skill. + """ + declared = datasets.machine_prerequisites(skill) + if not declared: + yield {} + return + + cache = Path(tempfile.mkdtemp(prefix=f"skillscope-prerequisites-{skill}-")) + _created_caches.add(str(cache)) + try: + inputs = {"SKILL_CACHE_DIR": str(cache)} + unavailable: dict[str, str] = {} + # Resolved here but not required: a skill whose steps never use its + # source tree must not fail because there is none -- not inside a + # checkout, or a vendored skill whose fetch failed. A step that needs + # it sees it unset, and an `env` value naming it fails with the + # resolver's own reason. + try: + inputs["SKILL_SOURCE_DIR"] = str(sources.resolve(skill, cache).path) + except (SystemExit, Exception) as exc: + unavailable["SKILL_SOURCE_DIR"] = str(exc).removeprefix("error: ") + print(f"[prerequisites] {skill}: {len(declared)} to provide", flush=True) + env = asyncio.run( + provide( + declared, + inputs, + host_platform(), + _host_runner(datasets.machine_path(skill).parent), + unavailable, + ) + ) + yield env + finally: + shutil.rmtree(cache, ignore_errors=True) + _created_caches.discard(str(cache)) + + +def _named_files(skill: str, declared: list[Prerequisite]) -> dict[str, Path]: + """The files beside machine.yml that a container step names, by argv item. + + Only these are copied in. The rest of evals/ includes the answer key, and a + step has no business carrying it into the agent's room -- so naming a + dataset file is refused rather than copied. + """ + evals_dir = datasets.machine_path(skill).parent.resolve() + keys = { + (datasets.skill_path(skill) / rel).resolve() + for rel in (datasets.DATASET_RELPATH, datasets.EXTENDED_DATASET_RELPATH) + } + named: dict[str, Path] = {} + for item in declared: + for step in ("check", "install"): + for arg in item.command(step, "linux") or []: + # Every argv item is probed, including the body of an inline + # `sh -c` script, which can be no valid path at all. + try: + candidate = (evals_dir / arg).resolve() + if not candidate.is_file() or evals_dir not in candidate.parents: + continue + except (OSError, ValueError): + continue + if candidate in keys: + raise PrerequisiteError( + f"prerequisite `{item.name}` names {arg}, which is this " + "skill's answer key and is never copied into the agent's " + "container." + ) + named[candidate.relative_to(evals_dir).as_posix()] = candidate + return named + + +def claude_code_agent(skill: str, make_agent: Callable[[dict[str, str]], object]): + """Solver: provide `skill`'s prerequisites in this case's container, then run. + + `make_agent` builds the agent from the env the prerequisites produced. It is + built here, per case, rather than once with the task, because that env is + only known once `check` has run in this case's container: a prerequisite + the image already provides gets none of its declared env, same as on the + host. + + `None` for a skill that declares none, so its task is built exactly as it + was before prerequisites existed. + + Assumes a Linux container: the fixed paths and `mkdir`/`rm` are the + guest's. `verify.require` refuses the unisolated `local` provider and + Windows before any task is built, which is what makes that hold. + """ + declared = datasets.machine_prerequisites(skill) + if not declared: + return None + named = _named_files(skill, declared) + + from inspect_ai.agent import as_solver + from inspect_ai.solver import solver + from inspect_ai.util import OutputLimitExceededError, sandbox + + @solver + def _with_prerequisites(): + async def solve(state, generate): + box = sandbox() + + async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: + # inspect raises for output too large to return or not text; + # as a failed step, `provide` reports it under the + # prerequisite's name rather than as a bare sandbox error. + try: + result = await box.exec(argv, cwd=CONTAINER_STEPS, env=extra) + except (OutputLimitExceededError, OSError, UnicodeDecodeError) as exc: + return 126, f"{type(exc).__name__}: {exc}" + return result.returncode, result.stdout + result.stderr + + made = await box.exec(["mkdir", "-p", CONTAINER_STEPS, CONTAINER_CACHE]) + if not made.success: + raise PrerequisiteError( + f"could not create {CONTAINER_ROOT} in the container: " + f"{_tail(made.stderr)}" + ) + try: + # Inside the `try`, so a copy that fails partway still has the + # files already copied removed before anything else runs. + for relative, path in named.items(): + await box.write_file( + f"{CONTAINER_STEPS}/{relative}", path.read_bytes() + ) + env = await provide( + declared, + {"SKILL_CACHE_DIR": CONTAINER_CACHE}, + "linux", + run, + { + "SKILL_SOURCE_DIR": "a container has no copy of the " + "skill's source tree. Build what needs it into the " + "image instead." + }, + ) + except BaseException: + # Best effort, and never in place of the error being raised. + with contextlib.suppress(Exception): + await box.exec(["rm", "-rf", CONTAINER_STEPS]) + raise + removed = await box.exec(["rm", "-rf", CONTAINER_STEPS]) + if not removed.success: + # The agent must not start with copied evals/ files in reach. + raise PrerequisiteError( + f"could not remove {CONTAINER_STEPS} before the agent " + f"starts: {_tail(removed.stderr)}" + ) + return await as_solver(make_agent(env))(state, generate) + + return solve + + return _with_prerequisites() diff --git a/skillscope/engine/verify.py b/skillscope/engine/verify.py index d06cde2..b897abb 100644 --- a/skillscope/engine/verify.py +++ b/skillscope/engine/verify.py @@ -37,6 +37,7 @@ convert, hooks, models, + prerequisites, sandbox as sandbox_spec, scorers, stats, @@ -238,28 +239,40 @@ def build_task( # whichever engine started it. hook = hooks.load(skill) - bound = deadline.active() - return Task( - name=f"claude-code-{skill}", - setup=hooks.setup_solver(hook, skill, cases), - cleanup=hooks.cleanup_fn(hook, skill, cases), - dataset=samples, + def agent(env: dict[str, str]): # `skills=` installs into .claude/skills inside the sandbox, which is # where the real harness looks -- the point of this leg is that its # discovery machinery, not ours, decides what happens. # `effort` reaches the agent rather than being dropped here: unset is # not neutral, it is the model's own default, and the legs this one is # compared against all pass the value they were given. - # A behavioral case has more riding on this than a routing one: it runs - # the skill to completion, so a `reference.md` the installer dropped is - # a step the agent cannot take, and the scorer records it as work the - # skill failed to do. + # `env` is unset rather than empty when there is nothing to add, so a + # skill without prerequisites builds the agent it always has. + return claude_code( + skills=[skill_dir], + cwd=tools.workdir_path(), + effort=effort or None, + env=env or None, + ) + + bound = deadline.active() + return Task( + name=f"claude-code-{skill}", + setup=hooks.setup_solver(hook, skill, cases), + cleanup=hooks.cleanup_fn(hook, skill, cases), + dataset=samples, + # A behavioral case has more riding on a complete skill than a routing + # one: it runs the skill to completion, so a `reference.md` the + # installer dropped is a step the agent cannot take, and the scorer + # records it as work the skill failed to do. solver=chain( _ensure_workdir(), complete_skills_solver({skill_dir.name: skill_dir}), - claude_code( - skills=[skill_dir], cwd=tools.workdir_path(), effort=effort or None - ), + # A skill that declares prerequisites gets them provided in this + # case's container first, and an agent built with the env they + # produced. One that declares none gets the agent exactly as before. + prerequisites.claude_code_agent(skill, agent) + or agent({}), ), scorer=scorers.expectations(), sandbox=sandbox_spec.for_skill(skill), @@ -292,13 +305,33 @@ def run( continue print(f"[claude-code] {skill}: {len(skill_cases)} case(s)", flush=True) + try: + task = build_task(skill, skill_cases, model, effort=effort) + except prerequisites.PrerequisiteError as exc: + # A declaration this engine refuses to carry into a container -- a + # step naming the answer key -- fails its skill, as on the host. + outcomes.extend( + behavioral._failed(skill, skill_cases, f"{skill}: {exc}", degraded=False) + ) + continue + except SystemExit as exc: + # A machine.yml that does not parse, as `behavioral.run` treats it: + # that skill's failure, not the end of every skill after it. Not a + # provider outage, whatever the path in the message happens to say. + outcomes.extend( + behavioral._failed(skill, skill_cases, str(exc), degraded=False) + ) + continue logs = behavioral.run_eval( inspect_eval, - build_task(skill, skill_cases, model, effort=effort), + task, model=model, model_args=models.model_args(model), log_dir=str(Path(".skillscope") / "logs"), log_realtime=behavioral.realtime_logging(), + # A case whose container could not be prepared -- an install that + # failed there -- is that case's failure. See `behavioral.run`. + fail_on_error=False, display="plain", ) if not logs and behavioral.stop_requested(): diff --git a/skillscope/schema/machine.schema.json b/skillscope/schema/machine.schema.json index 734f93a..9b5430d 100644 --- a/skillscope/schema/machine.schema.json +++ b/skillscope/schema/machine.schema.json @@ -2,7 +2,7 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://github.com/amd/skillscope/skillscope/schema/machine.schema.json", "title": "skillscope skill machine requirements", - "description": "Optional file at /evals/machine.yml, present ONLY for a skill whose behavioral cases cannot run on the everyday runners on every platform. Absent means exactly that, which is the common case. Say which platforms to use and which runner labels the work needs; what follows from asking for hardware -- the base runs-on labels, the pull-request label rationing that pool, the environment holding its credentials -- belongs to the repo and is passed to skillscope by the workflow, not restated here.", + "description": "Optional file at /evals/machine.yml, present ONLY for a skill whose behavioral cases need something the everyday runners do not give them: fewer platforms, particular hardware, a sandbox of its own, or prerequisites to provide. Absent means none of those, which is the common case. Say which platforms to use and which runner labels the work needs; what follows from asking for hardware -- the base runs-on labels, the pull-request label rationing that pool, the environment holding its credentials -- belongs to the repo and is passed to skillscope by the workflow, not restated here.", "type": "object", "additionalProperties": false, "properties": { @@ -25,6 +25,60 @@ "minLength": 1, "description": "Compose file, resolved beside this machine.yml in evals/, describing the sandbox the behavioral cases need under `--engine claude-code`. Absent means the default container with no network, which is what a skill that only reads and writes files should want. Name one to opt into network egress -- a skill that installs a server or pulls a model cannot run without it -- or to bind a device in. Ignored on Windows, where inspect's sandbox layer assumes a POSIX guest and cases run unsandboxed on the host instead.", "examples": ["compose.yaml"] + }, + "prerequisites": { + "type": "array", + "minItems": 1, + "description": "What the behavioral cases need provided before they run, applied by every engine where its agent runs: once per skill on the host under the default engine, in each case's container under `--engine claude-code`. Each runs `check`; if that fails, `install` runs and `check` is retried with `env` applied, and only then is `env` handed to the agent. A prerequisite whose `check` passes is used as is and none of its `env` is applied. On the host, steps run with the directory holding this file as their working directory; in a container, in `/opt/skillscope/steps`, which holds only the files they name. Steps see `SKILL_SOURCE_DIR` (the skill's source tree; host only) and `SKILL_CACHE_DIR` (scratch space, removed after the skill on the host; `/opt/skillscope/cache` in a container, so an image can bake an install in at the same path).", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["name", "check", "install"], + "properties": { + "name": { + "type": "string", + "minLength": 1, + "description": "What this provides, as reports should name it. Unique within the file." + }, + "check": { + "$ref": "#/$defs/command", + "description": "Exits 0 when the prerequisite is present. Files it names are copied into a container the same way as for `install`." + }, + "install": { + "$ref": "#/$defs/command", + "description": "Provides the prerequisite. Under `--engine claude-code`, an argv item that is a path to a file in the directory holding this machine.yml, or below it, is copied into the container at the same relative path for the step and removed before the agent starts; a file mentioned only inside a string is not, and a step naming an eval dataset is refused." + }, + "env": { + "type": "object", + "additionalProperties": { "type": "string" }, + "description": "Set for the agent once the prerequisite has been installed. A value replaces the variable rather than extending it. Values may refer to `${SKILL_SOURCE_DIR}` and `${SKILL_CACHE_DIR}`, and no other `$` may appear; names the harness sets for the agent (`ANTHROPIC_*`, `CLAUDE_*`, `IS_SANDBOX`) are refused. The skillscope parser enforces those rules, a command for every platform in `os`, and unique names, none of which this schema expresses.", + "examples": [{ "PYTHONPATH": "${SKILL_CACHE_DIR}/decoder/python" }] + } + } + } + } + }, + "$defs": { + "command": { + "description": "An argv, or a mapping from platform (`linux`, `windows`, `macos`) to argv. A container is always `linux`.", + "oneOf": [ + { "$ref": "#/$defs/argv" }, + { + "type": "object", + "minProperties": 1, + "additionalProperties": false, + "properties": { + "linux": { "$ref": "#/$defs/argv" }, + "windows": { "$ref": "#/$defs/argv" }, + "macos": { "$ref": "#/$defs/argv" } + } + } + ] + }, + "argv": { + "type": "array", + "minItems": 1, + "items": { "type": "string", "minLength": 1 } } } } diff --git a/skillscope/structure.py b/skillscope/structure.py index 171eaf5..190d440 100644 --- a/skillscope/structure.py +++ b/skillscope/structure.py @@ -141,7 +141,7 @@ def _frontmatter(text: str) -> tuple[dict | None, str, str]: "to match a prompt against, so the skill is never loaded." ) - import yaml # noqa: PLC0415 -- keeps the run path stdlib-only + import yaml # noqa: PLC0415 -- importing this module stays cheap try: declared = yaml.safe_load(match.group("frontmatter")) diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 5d4cf7b..789e4e1 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -31,6 +31,7 @@ import shutil import subprocess import tempfile +import threading import time import types import unittest @@ -69,6 +70,7 @@ from skillscope.engine import scorers as engine_scorers from skillscope.engine import stats as engine_stats from skillscope.engine import pricing as engine_pricing +from skillscope.engine import prerequisites as engine_prerequisites from skillscope import usage REPO_ROOT = datasets.PACKAGE_DIR.parent @@ -352,6 +354,10 @@ def setUp(self) -> None: def test_documented_keys_match_the_parser(self) -> None: self.assertEqual(set(self.schema["properties"]), datasets.MACHINE_KEYS) + item = self.schema["properties"]["prerequisites"]["items"] + self.assertEqual(set(item["properties"]), datasets.PREREQUISITE_KEYS) + per_platform = self.schema["$defs"]["command"]["oneOf"][1]["properties"] + self.assertEqual(set(per_platform), set(datasets.COMMAND_PLATFORMS)) def test_no_list_key_is_enumerated_in_the_schema(self) -> None: # Neither `os` nor `labels` can be: a label means whatever a repo @@ -3445,8 +3451,8 @@ class TestHooksRunOrTheRunStops(unittest.TestCase): `check` and `setup_session` do not. `check` is handed the legacy engine's `Run` object and these engines have inspect's `TaskState`; `setup_session` - returns template variables that are substituted into prompts before any - hook runs. Both are refused rather than skipped, because skipping leaves + is replaced by `prerequisites:` in machine.yml, which every engine applies. + Both are refused rather than skipped, because skipping leaves no trace: the case is graded as though the hook had run, and the failure surfaces later as a skill that mysteriously does not work on this runner. @@ -3497,6 +3503,8 @@ def test_a_setup_session_hook_is_refused_by_name(self) -> None: with self.assertRaises(SystemExit) as caught: self.refuse("claude-code", ["sessioned"]) self.assertIn("setup_session", str(caught.exception)) + # And says what replaced it, so the author is not left to search. + self.assertIn("prerequisites:", str(caught.exception)) def test_the_refusal_names_every_skill_it_blocks(self) -> None: # Naming one of three sends someone round the loop twice. @@ -5318,12 +5326,27 @@ def _an_outcome(**kwargs): _STUB_CLAUDE = r''' -import json, sys +import json, os, sys if "--help" in sys.argv: print("--max-budget-usd --no-session-persistence") sys.exit(0) -sys.stdin.read() +prompt = sys.stdin.read() +# What the agent would have seen, for tests that need to know: the prompt it was +# given, and every PREREQ_* variable with whether the path it names exists. One +# file per process: agents run concurrently, and appends to one shared file are +# not atomic on Windows, so a record could be lost. +record = os.environ.get("STUB_CLAUDE_RECORD") +if record: + seen = {k: v for k, v in os.environ.items() if k.startswith("PREREQ_")} + os.makedirs(record, exist_ok=True) + with open(os.path.join(record, f"{os.getpid()}.json"), "w", encoding="utf-8") as out: + out.write(json.dumps({ + "argv": sys.argv[1:], + "prompt": prompt, + "env": seen, + "exists": {k: os.path.exists(v) for k, v in seen.items()}, + }) + "\n") for event in [ {"type": "system", "subtype": "init", "skills": ["alpha", "a-stray-runner-skill"], "tools": ["Skill"]}, @@ -5462,6 +5485,11 @@ def setUp(self) -> None: self.addCleanup(deadline.use, previous) self.logs = tempfile.mkdtemp(prefix="ss-stopped-") self.addCleanup(shutil.rmtree, self.logs, True) + # A real skill, because the run reads its evals/machine.yml before + # building the task that these tests replace. + repo = Repo(self) + repo.skill("alpha", dataset=tier0_dataset("alpha")) + repo.activate() self.case = datasets.Case(id="c1", skill="alpha", prompt="p", skill_should_trigger=True, files_exist=["x"]) for patch in (mock.patch.object(engine_sandbox, "require_provider"), @@ -5966,5 +5994,1130 @@ def test_a_good_table_still_binds(self) -> None: self.assertTrue(engine_models.cost_limit_binds("mockllm/model")) +class TestTheCliHandsTheFactoryItsFlags(unittest.TestCase): + """`--model` and `--effort` reach the default engine's agent factory. + + The end-to-end tests above build the factory themselves, so they cannot + see the CLI dropping a flag on the way -- the class of bug that once ran a + whole comparison at the wrong reasoning effort. + """ + + def test_the_flags_are_passed_through(self) -> None: + repo = Repo(self) + repo.skill("alpha", dataset=tier0_dataset( + "alpha", evaluations=[{"id": "a1", TRIGGER_KEY: True, "prompt": "p", + "logs_contain": ["x"]}])) + repo.activate() + args = cli.build_parser().parse_args( + ["behavioral", "--engine", "claude-code-no-sandbox", + "--model", "opus", "--effort", "low"]) + handed = [] + with mock.patch.object(cli, "_prepare_graded_run", lambda a: ["alpha"]), \ + mock.patch.object(engine_no_sandbox, "require_local"), \ + mock.patch.object(engine_no_sandbox, "behavioral_solver_factory", + lambda model, effort: handed.append((model, effort))), \ + mock.patch.object(engine_behavioral, "run", lambda *a, **k: []), \ + mock.patch.object(cli, "_write_report"), \ + contextlib.redirect_stdout(io.StringIO()): + cli.cmd_behavioral(args) + self.assertEqual(handed, [("opus", "low")]) + + +# A prerequisite the tests can provide on any platform: `check` passes when +# PREREQ_TOOL names a file that exists, and `install` creates one in the cache +# and logs that it ran, so a test can count installs. +_PY = sys.executable +_CHECK_TOOL = ( + "import os, sys; p = os.environ.get('PREREQ_TOOL'); " + "sys.exit(0 if p and os.path.exists(p) else 1)" +) +_INSTALL_TOOL = ( + "import os, pathlib; " + "pathlib.Path(os.environ['SKILL_CACHE_DIR'], 'tool.txt').write_text('x'); " + "open(os.environ['SS_TEST_INSTALL_LOG'], 'a').write('installed\\n')" +) + + +def _alive(pid: int) -> bool: + """Whether a process is still running. Windows has no signal 0.""" + if os.name == "nt": + out = subprocess.run(["tasklist", "/FI", f"PID eq {pid}"], + capture_output=True, text=True).stdout + return str(pid) in out + try: + os.kill(pid, 0) + except ProcessLookupError: + return False + except PermissionError: + return True + # A zombie still answers signal 0; reap-or-not, it is no longer running. + try: + return Path(f"/proc/{pid}/stat").read_text().split()[2] != "Z" + except OSError: + return True + + +def _machine(*prerequisites: dict) -> str: + """A machine.yml declaring `prerequisites`. JSON is YAML, and quotes paths.""" + return json.dumps({"prerequisites": list(prerequisites)}) + + +def _tool(**overrides) -> dict: + declared = { + "name": "tool", + "check": [_PY, "-c", _CHECK_TOOL], + "install": [_PY, "-c", _INSTALL_TOOL], + "env": {"PREREQ_TOOL": "${SKILL_CACHE_DIR}/tool.txt"}, + } + declared.update(overrides) + return declared + + +def _fake_inspect_swe(claude_code): + """`inspect_swe` with `claude_code` replaced, whether or not it is installed. + + The unit-test job installs skillscope without the `verify` extra, so a test + that skipped when `inspect_swe` was missing never ran in CI. + """ + fake = types.ModuleType("inspect_swe") + fake.claude_code = claude_code + return mock.patch.dict(sys.modules, {"inspect_swe": fake}) + + +def _noop_solver(): + from inspect_ai.solver import solver + + @solver + def noop(): + async def solve(state, generate): + return state + + return solve + + return noop() + + +class TestAPrerequisiteDeclarationIsCheckedUpFront(unittest.TestCase): + """A malformed declaration stops at the structural check, not at run time. + + At run time it would surface as every case of the skill failing on a runner + already paid for; here it costs seconds and names the field. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.activate() + + def errors_for(self, machine: str) -> list[str]: + self.repo.skill("demo", dataset=tier0_dataset("demo"), machine=machine) + return datasets.structural_errors(["demo"]) + + def test_a_well_formed_declaration_passes(self) -> None: + self.assertEqual(self.errors_for(_machine(_tool())), []) + [found] = datasets.machine_prerequisites("demo") + self.assertEqual(found.name, "tool") + self.assertEqual(found.command("check", "windows"), [_PY, "-c", _CHECK_TOOL]) + + def test_a_command_can_differ_per_platform(self) -> None: + self.errors_for(_machine(_tool(install={"linux": ["sh", "a.sh"], + "windows": ["pwsh", "a.ps1"]}))) + [found] = datasets.machine_prerequisites("demo") + self.assertEqual(found.command("install", "linux"), ["sh", "a.sh"]) + self.assertEqual(found.command("install", "windows"), ["pwsh", "a.ps1"]) + self.assertIsNone(found.command("install", "macos")) + + def test_each_mistake_is_named(self) -> None: + missing_install = _tool() + del missing_install["install"] + for machine, expected in [ + (_machine(_tool(extra=1)), "unknown key(s): extra"), + (_machine(missing_install), "is missing install"), + (_machine(_tool(env={"X": "${HOME}/x"})), "uses `$`"), + (_machine(_tool(check={"solaris": ["true"]})), "unknown platform(s): solaris"), + (_machine(_tool(check="true")), "must be a command"), + (_machine(_tool(), _tool(name=" tool ")), "declared twice"), + (json.dumps({"prerequisites": []}), "non-empty list"), + (json.dumps({"prerequisites": None}), "non-empty list"), + (json.dumps({"prerequisites": ["tool"]}), "must be a mapping"), + (_machine(_tool(name=" ")), "non-empty string"), + (_machine(_tool(name=3)), "non-empty string"), + (_machine(_tool(env={"X": 1})), "map variable names to strings"), + (_machine(_tool(env={"": "x"})), "map variable names to strings"), + (_machine(_tool(env={"X": "$HOME/x"})), "`prerequisites[0].env.X` uses `$`"), + (_machine(_tool(env={"X": "${SKILL_CACHE_DIR"})), "uses `$`"), + (_machine(_tool(env={"ANTHROPIC_BASE_URL": "x"})), "set by the harness"), + (_machine(_tool(env={"CLAUDE_CONFIG_DIR": "x"})), "set by the harness"), + (_machine(_tool(env={"IS_SANDBOX": "0"})), "set by the harness"), + # Windows environment names are case-insensitive. + (_machine(_tool(env={"anthropic_api_key": "x"})), "set by the harness"), + (_machine(_tool(env={"A=B": "x"})), "cannot be set in a process environment"), + (_machine(_tool(env={"1A": "x"})), "cannot be set in a process environment"), + (_machine(_tool(env={"A": "x\u0000y"})), "cannot be set in a process environment"), + # Two unknown keys of different types: sorting them must not crash. + ("os: [Linux]\nprerequisites:\n - {name: t, check: [a], install: [b], 1: x, z: y}\n", + "unknown key(s): 1, z"), + (_machine(_tool(check={"windows": ["a"]})), "no check command for Linux"), + (_machine(_tool(), _tool(name="b", install={"linux": ["sh", 1]})), + "`prerequisites[1].install.linux` must be a non-empty list"), + (_machine(_tool(env=["X"])), "`prerequisites[0].env` must map"), + ("prerequisites:\n - {name: t, check: [a], install: [b], env: {1: x}}\n", + "`prerequisites[0].env` must map"), + (_machine(_tool(check={})), "`prerequisites[0].check` must be a command"), + (json.dumps({"prerequisites": "abc"}), "non-empty list"), + ]: + with self.subTest(expected=expected): + errors = self.errors_for(machine) + self.assertTrue(any(expected in e for e in errors), errors) + + def test_an_argv_is_kept_as_written(self) -> None: + # An inline script's newline and an argument's leading spaces are part + # of the command; trimming them changes what runs. + self.errors_for(_machine(_tool(install=["sh", "-c", "echo a\n", " x"]))) + [found] = datasets.machine_prerequisites("demo") + self.assertEqual(found.command("install", "linux"), ["sh", "-c", "echo a\n", " x"]) + + def test_yaml_that_does_not_parse_is_named_not_raised(self) -> None: + errors = self.errors_for("prerequisites: [unclosed\n") + self.assertTrue(any("is not valid YAML" in e for e in errors), errors) + + def test_a_per_platform_command_is_still_an_argv(self) -> None: + for bad in ("sh install.sh", []): + with self.subTest(command=bad): + errors = self.errors_for(_machine(_tool(install={"linux": bad}))) + self.assertTrue( + any("must be a non-empty list of strings" in e for e in errors), errors) + + def test_an_os_label_with_no_command_platform_is_not_a_gap(self) -> None: + # A runner label like `Plan9` is the workflow's business; there is no + # command key for it to be missing. + linux_only = _tool(check={"linux": ["c"]}, install={"linux": ["i"]}) + machine = json.dumps({"os": ["Linux", "Plan9"], "prerequisites": [linux_only]}) + self.assertEqual(self.errors_for(machine), []) + + def test_a_prerequisite_may_not_override_the_inputs(self) -> None: + for name in datasets.PREREQUISITE_INPUTS: + with self.subTest(name=name): + errors = self.errors_for(_machine(_tool(env={name: "/elsewhere"}))) + self.assertTrue(any("supplies to every step" in e for e in errors), errors) + + def test_mixed_type_top_level_keys_are_named_not_crashed_on(self) -> None: + errors = self.errors_for("os: [Linux]\n1: x\nz: y\n") + self.assertTrue(any("unknown key(s): 1, z" in e for e in errors), errors) + + def test_a_missing_install_for_a_platform_is_a_gap_too(self) -> None: + machine = json.dumps({"os": ["Linux", "Windows"], "prerequisites": [ + _tool(install={"linux": ["sh", "i.sh"]})]}) + errors = self.errors_for(machine) + self.assertTrue(any("no install command for Windows" in e for e in errors), errors) + + def test_a_name_is_reported_as_written_without_padding(self) -> None: + self.errors_for(_machine(_tool(name=" tool "))) + [found] = datasets.machine_prerequisites("demo") + self.assertEqual(found.name, "tool") + + def test_macos_commands_are_accepted(self) -> None: + self.errors_for(_machine(_tool(install={"macos": ["sh", "m.sh"]}))) + [found] = datasets.machine_prerequisites("demo") + self.assertEqual(found.command("install", "macos"), ["sh", "m.sh"]) + + def test_os_narrows_which_platforms_need_a_command(self) -> None: + # `os` narrows what is required: a Windows-only command is a gap only + # for a skill that also runs on Linux. + windows_only = _tool(check={"windows": ["a"]}, install={"windows": ["b"]}) + machine = json.dumps({"os": ["Windows"], "prerequisites": [windows_only]}) + self.assertEqual(self.errors_for(machine), []) + + +class TestProvidingAPrerequisite(unittest.TestCase): + """The one rule both engines apply, driven with a recording runner.""" + + def run_provide(self, declared, present=False, install_ok=True, recheck_ok=True): + calls = [] + + async def run(argv, env): + step = argv[0] + calls.append((step, dict(env))) + if step == "check": + first = sum(1 for s, _ in calls if s == "check") == 1 + if first: + return (0 if present else 1), "" + return (0 if recheck_ok else 1), "still missing" + return (0, "built") if install_ok else (2, "compiler exploded") + + env = asyncio.run( + engine_prerequisites.provide( + declared, {"SKILL_CACHE_DIR": "/cache"}, "linux", run + ) + ) + return env, calls + + def declared(self, **env): + return [datasets.Prerequisite( + name="tool", check={"*": ["check"]}, install={"*": ["install"]}, + env=env or {"TOOL": "${SKILL_CACHE_DIR}/tool"}, + )] + + def test_present_means_nothing_installed_and_no_env(self) -> None: + env, calls = self.run_provide(self.declared(), present=True) + self.assertEqual(env, {}) + self.assertEqual([s for s, _ in calls], ["check"]) + + def test_missing_means_install_then_check_again_with_the_env(self) -> None: + env, calls = self.run_provide(self.declared()) + self.assertEqual([s for s, _ in calls], ["check", "install", "check"]) + self.assertEqual(env, {"TOOL": "/cache/tool"}) + # The re-check sees the declared env; the steps before it do not. + self.assertNotIn("TOOL", calls[1][1]) + self.assertEqual(calls[2][1]["TOOL"], "/cache/tool") + self.assertEqual(calls[2][1]["SKILL_CACHE_DIR"], "/cache") + + def test_a_failed_install_says_why(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_provide(self.declared(), install_ok=False) + self.assertIn("install exited 2", str(raised.exception)) + self.assertIn("compiler exploded", str(raised.exception)) + + def test_an_install_that_did_not_take_is_a_failure(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_provide(self.declared(), recheck_ok=False) + self.assertIn("still not present after install", str(raised.exception)) + + def test_an_input_this_engine_cannot_supply_is_named_before_installing(self) -> None: + steps = [] + + async def run(argv, env): + steps.append(argv[0]) + return 1, "" + + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + asyncio.run(engine_prerequisites.provide( + self.declared(SRC="${SKILL_SOURCE_DIR}/x"), + {"SKILL_CACHE_DIR": "/cache"}, "linux", run, + {"SKILL_SOURCE_DIR": "no tree here"}, + )) + self.assertIn("${SKILL_SOURCE_DIR}", str(raised.exception)) + self.assertIn("no tree here", str(raised.exception)) + self.assertEqual(steps, ["check"]) # no build was spent finding out + + def test_a_failed_skill_is_an_outage_only_when_its_detail_reads_as_one(self) -> None: + case = datasets.Case(id="c", skill="s", prompt="p", skill_should_trigger=True) + [outage] = engine_behavioral._failed("s", [case], "upstream 502 gateway timeout") + [plain] = engine_behavioral._failed("s", [case], "no such file") + self.assertTrue(outage.degraded) + self.assertFalse(plain.degraded) + + def test_only_this_engines_prerequisite_error_is_excused_as_an_outage(self) -> None: + def error(traceback): + return types.SimpleNamespace(traceback=traceback) + + ours = "skillscope.engine.prerequisites.PrerequisiteError: install exited 1" + inspects = "inspect_ai._util.error.PrerequisiteError: upstream 502 gateway" + self.assertFalse(engine_behavioral._is_outage(error(ours), "upstream 502 gateway")) + # inspect's class of the same name is not ours, so the heuristic applies. + self.assertTrue(engine_behavioral._is_outage(error(inspects), "upstream 502 gateway")) + + def test_no_command_for_this_platform_is_named(self) -> None: + # Reachable on a host whose platform `os` does not list, which the + # structural check therefore did not look at. + async def run(argv, env): + return 1, "" + + declared = [datasets.Prerequisite("tool", {"windows": ["c"]}, {"windows": ["i"]})] + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + asyncio.run(engine_prerequisites.provide(declared, {}, "linux", run)) + self.assertIn("declares no command for linux", str(raised.exception)) + + def test_a_long_failure_keeps_its_end(self) -> None: + async def run(argv, env): + return (1, "") if argv[0] == "check" else (1, "noise " * 2000 + "THE REAL ERROR") + + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + asyncio.run(engine_prerequisites.provide( + self.declared(), {"SKILL_CACHE_DIR": "/c"}, "linux", run)) + message = str(raised.exception) + self.assertTrue(message.endswith("THE REAL ERROR")) + self.assertLess(len(message), engine_prerequisites.OUTPUT_TAIL + 200) + + def test_an_argv_the_os_cannot_take_is_a_failed_step(self) -> None: + run = engine_prerequisites._host_runner(Path(tempfile.gettempdir())) + code, output = asyncio.run(run(["bad\0argv"], {})) + self.assertEqual(code, 127) + self.assertIn("could not start", output) + + def test_the_host_names_macs_the_way_declarations_do(self) -> None: + with mock.patch.object(engine_prerequisites.platform, "system", lambda: "Darwin"): + self.assertEqual(engine_prerequisites.host_platform(), "macos") + with mock.patch.object(engine_prerequisites.platform, "system", lambda: "Windows"): + self.assertEqual(engine_prerequisites.host_platform(), "windows") + + def test_a_later_prerequisite_builds_on_an_earlier_one(self) -> None: + seen = [] + + async def run(argv, env): + seen.append((argv[0], dict(env))) + return (1 if argv[0] == "first-check" and len(seen) == 1 else 0), "" + + declared = [ + datasets.Prerequisite("a", {"*": ["first-check"]}, {"*": ["first-install"]}, + {"A": "${SKILL_CACHE_DIR}/a"}), + datasets.Prerequisite("b", {"*": ["second-check"]}, {"*": ["second-install"]}), + ] + asyncio.run(engine_prerequisites.provide( + declared, {"SKILL_CACHE_DIR": "/cache"}, "linux", run + )) + second = next(env for step, env in seen if step == "second-check") + self.assertEqual(second["A"], "/cache/a") + + +class TestPrerequisitesOnTheDefaultEngine(unittest.TestCase): + """The host leg end to end: real solver, real inspect `eval()`, stub `claude`. + + The stub records the PREREQ_* variables each agent was started with and + whether the paths they name exist, so these tests observe what the agent + saw rather than how the engine was written. + """ + + def setUp(self) -> None: + _stub_claude_on_path(self) + self.scratch = Path(tempfile.mkdtemp(prefix="ss-prereq-")) + self.addCleanup(shutil.rmtree, self.scratch, True) + self.record = self.scratch / "agents" + self.installs = self.scratch / "installs.log" + env = mock.patch.dict(os.environ, { + engine_sandbox.SANDBOX_ENV: "local", + "ANTHROPIC_API_KEY": "sk-test-not-a-key", + "STUB_CLAUDE_RECORD": str(self.record), + "SS_TEST_INSTALL_LOG": str(self.installs), + }) + env.start() + self.addCleanup(env.stop) + os.environ.pop("PREREQ_TOOL", None) + self.repo = Repo(self) + self.repo.activate() + + def skill(self, name: str, machine: str | None = None) -> None: + self.repo.skill(name, dataset=tier0_dataset(name), machine=machine) + + def run_cases(self, *skills: str, per_skill: int = 1, model=None, effort=None): + cases = [ + datasets.Case(id=f"{skill}-{i}", skill=skill, prompt=f"work for {skill} {i}", + skill_should_trigger=True, logs_contain=["done"]) + for skill in skills for i in range(per_skill) + ] + with contextlib.redirect_stdout(io.StringIO()): + return engine_behavioral.run( + list(skills), cases, "mockllm/model", "high", + # The factory the CLI uses; what the CLI hands it is tested in + # `TestTheCliHandsTheFactoryItsFlags`. + solver_factory=engine_no_sandbox.behavioral_solver_factory(model, effort), + log_dir=str(self.scratch / "logs"), + ) + + def seen(self) -> list[dict]: + """What each agent saw, in the order the cases were given.""" + if not self.record.exists(): + return [] + records = [json.loads(f.read_text()) for f in self.record.iterdir()] + return sorted(records, key=lambda r: r["prompt"]) + + def install_count(self) -> int: + return len(self.installs.read_text().splitlines()) if self.installs.exists() else 0 + + def test_a_missing_prerequisite_is_installed_once_and_every_case_can_use_it(self) -> None: + self.skill("needs-tool", _machine(_tool())) + outcomes = self.run_cases("needs-tool", per_skill=2) + self.assertFalse([o.error for o in outcomes if o.error]) + self.assertEqual(self.install_count(), 1) + seen = self.seen() + self.assertEqual(len(seen), 2) + for agent in seen: + self.assertTrue(agent["env"].get("PREREQ_TOOL"), agent) + self.assertTrue(agent["exists"]["PREREQ_TOOL"], agent) + + def test_a_prerequisite_already_present_is_used_as_is(self) -> None: + existing = self.scratch / "preinstalled.txt" + existing.write_text("x") + os.environ["PREREQ_TOOL"] = str(existing) + self.skill("needs-tool", _machine(_tool())) + self.run_cases("needs-tool") + self.assertEqual(self.install_count(), 0) + [agent] = self.seen() + self.assertEqual(agent["env"]["PREREQ_TOOL"], str(existing)) + + def test_a_prerequisite_that_cannot_be_installed_fails_only_its_skill(self) -> None: + # Output full of what a provider outage looks like, as build logs are. + self.skill("broken", _machine(_tool(install=[_PY, "-c", ( + "import sys; print('no compiler here: upstream 502 gateway timeout'); " + "sys.exit(3)" + )]))) + self.skill("fine") + outcomes = {o.skill: o for o in self.run_cases("broken", "fine")} + self.assertFalse(outcomes["broken"].passed) + self.assertTrue(outcomes["broken"].error.startswith("broken: "), outcomes["broken"].error) + self.assertIn("install exited 3", outcomes["broken"].error) + self.assertIn("no compiler here", outcomes["broken"].error) + self.assertFalse(outcomes["fine"].error) + # A build log is not a provider outage, whatever words it contains. + self.assertFalse(outcomes["broken"].degraded) + # Only the skill that ran reached an agent. + self.assertEqual([a["prompt"] for a in self.seen()], ["work for fine 0"]) + + def test_the_host_runs_the_command_for_its_own_platform(self) -> None: + # The host is told it is Windows and only Windows commands exist, so + # the run passes only if the host's platform is what selects them. + self.skill("keyed", _machine(_tool( + check={"windows": [_PY, "-c", _CHECK_TOOL]}, + install={"windows": [_PY, "-c", _INSTALL_TOOL]}, + ))) + with mock.patch.object(engine_prerequisites, "host_platform", lambda: "windows"): + [outcome] = self.run_cases("keyed") + self.assertFalse(outcome.error) + self.assertEqual(self.install_count(), 1) + + def test_the_watchdogs_hard_exit_kills_a_running_install_and_its_children(self) -> None: + # `os._exit` reaches neither the step's own kill nor any `finally`, so + # the watchdog's callback is all that stops what the install started. + self.assertIn(engine_prerequisites._on_expire, deadline._expire_callbacks) + pids_file = self.scratch / "pids" + script = ( + "import subprocess, sys, time; " + "child = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)']); " + f"open({str(pids_file)!r}, 'w').write(f'{{child.pid}}'); " + "time.sleep(60)" + ) + run = engine_prerequisites._host_runner(self.scratch) + worker = threading.Thread( + target=lambda: asyncio.run(run([_PY, "-c", script], {})), daemon=True) + worker.start() + for _ in range(100): + if pids_file.exists() and pids_file.read_text(): + break + time.sleep(0.1) + [step] = list(engine_prerequisites._live_steps) + child = int(pids_file.read_text()) + engine_prerequisites._on_expire() + worker.join(timeout=10) + for pid in (step, child): + for _ in range(50): + if not _alive(pid): + break + time.sleep(0.1) + self.assertFalse(_alive(pid), f"process {pid} outlived the hard exit") + self.assertEqual(engine_prerequisites._live_steps, set()) + + def test_steps_run_beside_machine_yml(self) -> None: + # The documented form: a script named by its path relative to the file. + folder = self.repo.skill("scripted", dataset=tier0_dataset("scripted"), + machine=_machine(_tool(install=[_PY, "install.py"]))) + (folder / "evals" / "install.py").write_text( + "import os, pathlib\n" + "pathlib.Path(os.environ['SKILL_CACHE_DIR'], 'tool.txt').write_text('x')\n" + "open(os.environ['SS_TEST_INSTALL_LOG'], 'a').write('installed\\n')\n", + encoding="utf-8", + ) + [outcome] = self.run_cases("scripted") + self.assertFalse(outcome.error) + self.assertEqual(self.install_count(), 1) + + def test_what_a_failing_step_says_on_stderr_reaches_the_report(self) -> None: + self.skill("noisy", _machine(_tool(install=[_PY, "-c", ( + "import sys; sys.stderr.write('fatal: STDERR-MARK\\n'); sys.exit(2)" + )]))) + [outcome] = self.run_cases("noisy") + self.assertIn("STDERR-MARK", outcome.error) + + def test_a_skill_without_prerequisites_resolves_and_creates_nothing(self) -> None: + self.skill("plain") + before = set(engine_prerequisites._created_caches) + with mock.patch.object(engine_prerequisites.sources, "resolve") as resolve, \ + mock.patch.object(engine_prerequisites.tempfile, "mkdtemp") as mkdtemp: + with engine_prerequisites.on_host("plain") as env: + self.assertEqual(env, {}) + resolve.assert_not_called() + mkdtemp.assert_not_called() + self.assertEqual(set(engine_prerequisites._created_caches), before) + + def test_the_model_and_effort_reach_the_agent_with_the_env(self) -> None: + self.skill("needs-tool", _machine(_tool())) + self.run_cases("needs-tool", model="opus", effort="low") + [agent] = self.seen() + argv = agent["argv"] + self.assertEqual(argv[argv.index("--model") + 1], "opus") + self.assertEqual(argv[argv.index("--effort") + 1], "low") + self.assertTrue(agent["env"].get("PREREQ_TOOL")) + + def test_steps_are_given_the_skill_source_tree(self) -> None: + # Resolved as `sources.resolve` would: here, the git checkout the skill + # sits in. Not via $SKILL_SOURCE_DIR, which a step would inherit from + # this process anyway and so could not tell the two apart. + os.environ.pop("SKILL_SOURCE_DIR", None) + subprocess.run(["git", "init", "-q", str(self.repo.root)], check=True) + tree = self.repo.root + self.skill("needs-tool", _machine(_tool(install=[_PY, "-c", ( + "import os, pathlib; " + "pathlib.Path(os.environ['SKILL_CACHE_DIR'], 'tool.txt').write_text('x'); " + "open(os.environ['SS_TEST_INSTALL_LOG'], 'a')" + ".write(os.environ['SKILL_SOURCE_DIR'] + '\\n')" + )]))) + self.run_cases("needs-tool") + self.assertEqual(self.installs.read_text().strip(), str(tree.resolve())) + + def test_an_env_value_can_name_the_source_tree(self) -> None: + os.environ.pop("SKILL_SOURCE_DIR", None) + subprocess.run(["git", "init", "-q", str(self.repo.root)], check=True) + self.skill("needs-tool", _machine(_tool(env={ + "PREREQ_TOOL": "${SKILL_CACHE_DIR}/tool.txt", + "PREREQ_SRC": "${SKILL_SOURCE_DIR}/sub", + }))) + self.run_cases("needs-tool") + [agent] = self.seen() + # Compared as a path: on Windows the `/` written in the declaration + # sits beside the `\\` of the expanded directory, and both are valid. + self.assertEqual(Path(agent["env"]["PREREQ_SRC"]), self.repo.root / "sub") + + def test_a_finished_step_is_no_longer_the_watchdogs_to_kill(self) -> None: + # A pid left behind could later be signalled after the OS reused it. + self.skill("needs-tool", _machine(_tool())) + self.run_cases("needs-tool") + self.assertEqual(engine_prerequisites._live_steps, set()) + + def test_a_declaration_that_does_not_parse_fails_only_its_skill(self) -> None: + # Named so its path carries words a provider outage is recognised by. + self.skill("upstream-gateway", json.dumps({"prerequisites": [5]})) + self.skill("broken-yaml", "prerequisites: [unclosed\n") + self.skill("fine") + outcomes = {o.skill: o for o in self.run_cases("upstream-gateway", "broken-yaml", "fine")} + self.assertIn("must be a mapping", outcomes["upstream-gateway"].error) + self.assertFalse(outcomes["upstream-gateway"].degraded) + self.assertIn("is not valid YAML", outcomes["broken-yaml"].error) + self.assertFalse(outcomes["fine"].error) + + def test_a_failed_install_leaves_no_cache_behind(self) -> None: + made = [] + real = tempfile.mkdtemp + + def recording(*a, **k): + made.append(real(*a, **k)) + return made[-1] + + self.skill("broken", _machine(_tool(install=[_PY, "-c", "import sys; sys.exit(1)"]))) + with mock.patch.object(engine_prerequisites.tempfile, "mkdtemp", recording): + self.run_cases("broken") + [cache] = [m for m in made if "skillscope-prerequisites" in m] + self.assertFalse(Path(cache).exists()) + + def test_the_declared_env_wins_over_a_stale_value(self) -> None: + # The runner's own PREREQ_TOOL names nothing, so `check` fails and the + # install runs; the agent must then see what the install provided. + os.environ["PREREQ_TOOL"] = str(self.scratch / "nothing-here") + self.skill("needs-tool", _machine(_tool())) + self.run_cases("needs-tool") + [agent] = self.seen() + self.assertNotEqual(agent["env"]["PREREQ_TOOL"], str(self.scratch / "nothing-here")) + self.assertTrue(agent["exists"]["PREREQ_TOOL"]) + + def test_output_that_is_not_text_is_still_reported(self) -> None: + self.skill("binary", _machine(_tool(install=[_PY, "-c", ( + "import sys; sys.stdout.buffer.write(b'bad \\xff byte'); sys.exit(1)" + )]))) + [outcome] = self.run_cases("binary") + self.assertIn("bad \ufffd byte", outcome.error) + + def test_an_install_that_cannot_start_says_so(self) -> None: + self.skill("absent", _machine(_tool(install=["no-such-installer-xyz"]))) + [outcome] = self.run_cases("absent") + self.assertIn("install exited 127", outcome.error) + self.assertIn("no-such-installer-xyz", outcome.error) + + def test_a_source_tree_that_cannot_be_fetched_fails_only_what_needs_it(self) -> None: + # A vendored skill's fetch raises `RuntimeError`, not `SystemExit`. A + # skill whose prerequisites never use the tree must run regardless. + self.skill("needs-tool", _machine(_tool())) + with mock.patch.object( + engine_prerequisites.sources, "resolve", + side_effect=RuntimeError("git fetch failed"), + ): + [outcome] = self.run_cases("needs-tool") + self.assertFalse(outcome.error) + self.assertEqual(self.install_count(), 1) + + def test_the_watchdog_takes_the_host_caches_with_it(self) -> None: + # The `--timeout` hard exit skips the `finally` that removes the cache, + # so the cache a real install made must be where the watchdog looks. + self.skill("needs-tool", _machine(_tool())) + self.assertIn(engine_prerequisites._on_expire, deadline._expire_callbacks) + with contextlib.redirect_stdout(io.StringIO()), \ + engine_prerequisites.on_host("needs-tool") as env: + cache = Path(env["PREREQ_TOOL"]).parent + self.assertIn(str(cache), engine_prerequisites._created_caches) + engine_prerequisites._on_expire() + self.assertFalse(cache.exists()) + + def test_a_finished_skill_leaves_nothing_for_the_watchdog(self) -> None: + self.skill("needs-tool", _machine(_tool())) + with contextlib.redirect_stdout(io.StringIO()), \ + engine_prerequisites.on_host("needs-tool") as env: + cache = Path(env["PREREQ_TOOL"]).parent + self.assertFalse(cache.exists()) + self.assertNotIn(str(cache), engine_prerequisites._created_caches) + + def test_an_input_this_run_could_not_resolve_is_unset_and_explained(self) -> None: + # The runner's own $SKILL_SOURCE_DIR names nothing, so resolution fails. + # A step must then see the input unset, not the stale value, and a + # failure must say why it was unset. + os.environ["SKILL_SOURCE_DIR"] = str(self.scratch / "no-such-tree") + self.addCleanup(os.environ.pop, "SKILL_SOURCE_DIR", None) + self.skill("needs-source", _machine(_tool(install=[_PY, "-c", ( + "import os, sys; " + "open(os.environ['SS_TEST_INSTALL_LOG'], 'a')" + ".write(os.environ.get('SKILL_SOURCE_DIR', 'unset') + '\\n'); " + "print('MISSING-TREE'); sys.exit(1)" + )]))) + [outcome] = self.run_cases("needs-source") + self.assertEqual(self.installs.read_text().strip(), "unset") + # The step's own words before the explanation, so a summary that keeps + # only the start of the error still shows what the step said. + self.assertIn("install exited 1: MISSING-TREE", outcome.error) + self.assertIn("SKILL_SOURCE_DIR was not available", outcome.error) + + def test_a_stop_during_an_install_fails_the_skill_and_keeps_the_report(self) -> None: + # The deadline's graceful stage interrupts the main thread. Landing in + # a host install -- the slowest thing a run does outside inspect -- it + # must end the install promptly and fail that skill, not escape `run` + # and take every earlier skill's result with it. + self.skill("graded") + # The install starts a child of its own, as a build script does; the + # stop has to reach that too. + self.skill("slow", _machine(_tool(install=[_PY, "-c", ( + "import os, subprocess, sys, time; " + "child = subprocess.Popen([sys.executable, '-c', 'import time; time.sleep(60)']); " + "open(os.environ['SS_TEST_INSTALL_LOG'], 'a')" + ".write(f'{os.getpid()} {child.pid}\\n'); " + "time.sleep(60)" + )]))) + bound = deadline.Deadline(600.0, command="behavioral") + previous = deadline.use(bound) + self.addCleanup(deadline.use, previous) + + # Fired once the install has started, not on a clock: the stop has to + # land in the install, and the skill before it takes as long as it takes. + def stop_once_installing() -> None: + for _ in range(600): + if self.installs.exists() and len(self.installs.read_text().split()) == 2: + bound.interrupted = True + deadline._interrupt_main() + return + time.sleep(0.1) + + threading.Thread(target=stop_once_installing, daemon=True).start() + started = time.monotonic() + try: + outcomes = {o.skill: o for o in self.run_cases("graded", "slow")} + except KeyboardInterrupt: + self.fail("the stop escaped the run and lost the report") + self.assertLess(time.monotonic() - started, 30, "the install was not stopped") + self.assertFalse(outcomes["graded"].error) + self.assertIn("exceeded --timeout", outcomes["slow"].error) + # Stopped, not abandoned: the install and what it started are gone. + for pid in map(int, self.installs.read_text().split()): + for _ in range(50): + if not _alive(pid): + break + time.sleep(0.1) + self.assertFalse(_alive(pid), f"process {pid} outlived the stop") + + def test_a_person_pressing_ctrl_c_still_stops_the_command(self) -> None: + # Only the deadline's stop is turned into a failed skill. + self.skill("needs-tool", _machine(_tool())) + with mock.patch.object( + engine_prerequisites, "provide", side_effect=KeyboardInterrupt + ), self.assertRaises(KeyboardInterrupt): + self.run_cases("needs-tool") + + def test_nothing_a_skill_installs_reaches_the_next_one(self) -> None: + self.skill("needs-tool", _machine(_tool())) + self.skill("plain") + self.run_cases("needs-tool", "plain") + first, second = self.seen() + self.assertIn("needs-tool", first["prompt"]) + self.assertEqual(second["env"], {}) + self.assertNotIn("PREREQ_TOOL", os.environ) + # Its scratch space went with it. + self.assertFalse(Path(first["env"]["PREREQ_TOOL"]).exists()) + + +class TestPrerequisitesInAContainer(unittest.TestCase): + """The `claude-code` leg's solver, against a recording stand-in sandbox.""" + + class _Box: + def __init__(self, present: bool, install_fails=False, mkdir_fails=False) -> None: + self.present = present + self.install_fails = install_fails + self.mkdir_fails = mkdir_fails + self.calls: list[tuple] = [] + self.files: dict[str, bytes] = {} + + async def exec(self, argv, cwd=None, env=None): + self.calls.append((list(argv), cwd, dict(env or {}))) + code = 0 + if argv[0] == "mkdir" and self.mkdir_fails: + code = 1 + elif argv[0] == _PY and argv[2] == _CHECK_TOOL: + code = 0 if self.present else 1 + if (env or {}).get("PREREQ_TOOL"): + code = 0 # after install + elif argv[0] == _PY and self.install_fails: + code = 4 + return types.SimpleNamespace( + returncode=code, success=code == 0, stdout="", stderr="denied" + ) + + async def write_file(self, path, contents): + self.files[path] = contents + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.activate() + + def run_solver(self, machine: str, present: bool = False, files=None, box=None): + folder = self.repo.skill("boxed", dataset=tier0_dataset("boxed"), machine=machine) + for name, text in (files or {}).items(): + (folder / "evals" / name).write_text(text, encoding="utf-8") + box = box or self._Box(present) + self.box = box + built: list[dict] = [] + + def make_agent(env): + built.append(env) + + async def agent(state, generate): + box.calls.append((["AGENT"], None, {})) + return state + + return agent + + with mock.patch("inspect_ai.util.sandbox", lambda *a, **k: box), \ + mock.patch("inspect_ai.agent.as_solver", lambda a: a): + solver = engine_prerequisites.claude_code_agent("boxed", make_agent) + asyncio.run(solver(object(), None)) + return box, built + + def test_a_skill_with_none_gets_the_agent_it_always_had(self) -> None: + self.repo.skill("bare", dataset=tier0_dataset("bare")) + self.assertIsNone(engine_prerequisites.claude_code_agent("bare", lambda env: None)) + + def test_the_same_declaration_prepares_the_container_before_the_agent(self) -> None: + box, [env] = self.run_solver(_machine(_tool())) + steps = [argv for argv, _, _ in box.calls if argv[0] == _PY] + self.assertEqual(len(steps), 3) # check, install, check + self.assertEqual(env, {"PREREQ_TOOL": f"{engine_prerequisites.CONTAINER_CACHE}/tool.txt"}) + # The steps directory is gone before the agent starts, and it starts. + self.assertEqual(box.calls[-2][0][:2], ["rm", "-rf"]) + self.assertEqual(box.calls[-1][0], ["AGENT"]) + + def test_steps_run_where_their_files_were_copied(self) -> None: + box, _ = self.run_solver(_machine(_tool())) + for argv, cwd, _ in box.calls: + if argv[0] == _PY: + self.assertEqual(cwd, engine_prerequisites.CONTAINER_STEPS) + + def test_a_failed_install_still_removes_the_copied_steps(self) -> None: + # The files copied out of evals/ must not outlive the step, and a case + # whose install failed is exactly the one nobody looks at afterwards. + with self.assertRaises(engine_prerequisites.PrerequisiteError): + self.run_solver(_machine(_tool()), box=self._Box(False, install_fails=True)) + self.assertEqual(self.box.calls[-1][0][:2], ["rm", "-rf"]) + + def test_what_a_failing_container_step_says_on_stderr_is_kept(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool()), box=self._Box(False, install_fails=True)) + message = str(raised.exception) + # The step's own words first, where a summary that keeps only the + # start of an error will still show them; no note about an input the + # step never mentioned. + self.assertTrue(message.startswith("prerequisite `tool`: install exited 4: denied"), message) + self.assertNotIn("SKILL_SOURCE_DIR", message) + + def test_what_a_failing_container_step_says_on_stdout_is_kept(self) -> None: + class Chatty(self._Box): + async def exec(self, argv, cwd=None, env=None): + result = await super().exec(argv, cwd, env) + if argv[0] == _PY and argv[2] != _CHECK_TOOL: + result.stdout, result.stderr = "make: *** [all] Error 2", "" + return result + + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool()), box=Chatty(False, install_fails=True)) + self.assertIn("make: *** [all] Error 2", str(raised.exception)) + + def test_copied_steps_that_cannot_be_removed_keep_the_agent_from_starting(self) -> None: + class Sticky(self._Box): + async def exec(self, argv, cwd=None, env=None): + result = await super().exec(argv, cwd, env) + if argv[:2] == ["rm", "-rf"]: + result.returncode, result.success = 1, False + return result + + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool()), box=Sticky(False)) + self.assertIn("could not remove", str(raised.exception)) + self.assertNotIn(["AGENT"], [c[0] for c in self.box.calls]) + + def test_a_cancelled_step_still_removes_the_copied_files(self) -> None: + class Cancelled(self._Box): + async def exec(self, argv, cwd=None, env=None): + if argv[0] == _PY and argv[2] != _CHECK_TOOL: + self.calls.append((list(argv), cwd, dict(env or {}))) + raise asyncio.CancelledError() + return await super().exec(argv, cwd, env) + + with self.assertRaises(asyncio.CancelledError): + self.run_solver(_machine(_tool()), box=Cancelled(False)) + self.assertEqual(self.box.calls[-1][0][:2], ["rm", "-rf"]) + + def test_each_kind_of_exec_failure_names_the_prerequisite(self) -> None: + for error in (OSError("container gone"), + UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid start byte")): + class Failing(self._Box): + async def exec(self, argv, cwd=None, env=None, _error=error): + if argv[0] == _PY and argv[2] != _CHECK_TOOL: + raise _error + return await super().exec(argv, cwd, env) + + with self.subTest(error=type(error).__name__): + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool()), box=Failing(False)) + self.assertIn("prerequisite `tool`", str(raised.exception)) + self.assertIn(type(error).__name__, str(raised.exception)) + + def test_the_container_paths_are_the_documented_ones(self) -> None: + # Images bake an install in at these paths, so they are a contract. + self.assertEqual(engine_prerequisites.CONTAINER_ROOT, "/opt/skillscope") + self.assertEqual(engine_prerequisites.CONTAINER_CACHE, "/opt/skillscope/cache") + box, _ = self.run_solver(_machine(_tool())) + [mkdir] = [argv for argv, _, _ in box.calls if argv[0] == "mkdir"] + self.assertIn("/opt/skillscope/cache", mkdir) + self.assertIn(engine_prerequisites.CONTAINER_STEPS, mkdir) + + def test_a_container_cannot_supply_the_source_tree(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool(env={"SRC": "${SKILL_SOURCE_DIR}/x"}))) + self.assertIn("no copy of the skill's source tree", str(raised.exception)) + + def test_a_container_that_cannot_hold_the_inputs_says_so(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool()), box=self._Box(False, mkdir_fails=True)) + self.assertIn(engine_prerequisites.CONTAINER_ROOT, str(raised.exception)) + self.assertFalse([c for c in self.box.calls if c[0][0] == _PY]) + + def test_present_in_the_image_means_no_install_and_no_env(self) -> None: + box, [env] = self.run_solver(_machine(_tool()), present=True) + self.assertEqual(env, {}) + self.assertEqual(len([c for c in box.calls if c[0][0] == _PY]), 1) + + def test_files_named_by_either_step_arrive_at_their_relative_paths(self) -> None: + # A container counts as linux, so a linux-keyed map is what applies. + folder = self.repo.skill( + "boxed", dataset=tier0_dataset("boxed"), + machine=_machine(_tool(check={"linux": ["sh", "tools/check.sh"]}, + install={"linux": ["sh", "install.sh"]})), + ) + (folder / "evals" / "tools").mkdir() + (folder / "evals" / "tools" / "check.sh").write_text("true\n", encoding="utf-8") + (folder / "evals" / "install.sh").write_text("true\n", encoding="utf-8") + box, _ = self.run_solver(_machine(_tool(check={"linux": ["sh", "tools/check.sh"]}, + install={"linux": ["sh", "install.sh"]}))) + steps = engine_prerequisites.CONTAINER_STEPS + self.assertEqual(set(box.files), {f"{steps}/tools/check.sh", f"{steps}/install.sh"}) + + def test_a_sandbox_error_in_a_step_names_the_prerequisite(self) -> None: + from inspect_ai.util import OutputLimitExceededError + + class Flooding(self._Box): + async def exec(self, argv, cwd=None, env=None): + if argv[0] == _PY and argv[2] != _CHECK_TOOL: + raise OutputLimitExceededError(limit_str="10 MiB", truncated_output=None) + return await super().exec(argv, cwd, env) + + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool()), box=Flooding(False)) + self.assertIn("prerequisite `tool`", str(raised.exception)) + self.assertIn("OutputLimitExceededError", str(raised.exception)) + + def test_only_the_files_a_step_names_are_copied_in(self) -> None: + box, _ = self.run_solver( + _machine(_tool(install=["sh", "install.sh"])), + files={"install.sh": "echo hi\n", "notes.txt": "private\n"}, + ) + self.assertEqual( + list(box.files), [f"{engine_prerequisites.CONTAINER_STEPS}/install.sh"] + ) + + def test_an_inline_script_is_not_mistaken_for_a_file(self) -> None: + # The body of `sh -c` is probed as a path like any other argv item, and + # one longer than a file name may be must not stop the run. + box, [env] = self.run_solver(_machine(_tool(install=["sh", "-c", "x" * 400]))) + self.assertEqual(box.files, {}) + + def test_the_extended_answer_key_is_refused_too(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool(install=["cat", "extended_evals.json"])), + files={"extended_evals.json": "{}"}) + self.assertIn("answer key", str(raised.exception)) + + def test_a_failed_install_in_a_container_is_not_a_provider_outage(self) -> None: + # Through a real `eval()`: the failure is raised inside a case, and the + # outcome is built from what inspect recorded about it. + from inspect_ai import Task, eval as inspect_eval + from inspect_ai.dataset import Sample + from inspect_ai.solver import solver + + def task(error): + @solver + def failing(): + async def solve(state, generate): + raise error + return solve + return Task(dataset=[Sample(id="c1", input="p")], solver=failing()) + + logs_dir = tempfile.mkdtemp(prefix="ss-prereq-log-") + self.addCleanup(shutil.rmtree, logs_dir, True) + case = datasets.Case(id="c1", skill="boxed", prompt="p", skill_should_trigger=True) + results = {} + for name, error in [ + ("prerequisite", engine_prerequisites.PrerequisiteError( + "install exited 1.\nupstream 502 gateway timeout")), + ("outage", RuntimeError("upstream 502 gateway timeout")), + ]: + with contextlib.redirect_stdout(io.StringIO()): + [log] = inspect_eval(task(error), model="mockllm/model", + fail_on_error=False, display="none", + log_dir=logs_dir) + [results[name]] = engine_behavioral._outcomes(log, "boxed", [case]) + self.assertIn("502", results["prerequisite"].error) + # Reported as its message, not as `PrerequisiteError('...\\n...')`. + self.assertTrue(results["prerequisite"].error.startswith("install exited 1."), + results["prerequisite"].error) + self.assertFalse(results["prerequisite"].degraded) + self.assertTrue(results["outage"].degraded) # the heuristic still works + + def test_a_step_naming_the_answer_key_is_refused(self) -> None: + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + self.run_solver(_machine(_tool(install=["cat", "evals.json"]))) + self.assertIn("answer key", str(raised.exception)) + + def test_the_sandboxed_task_runs_its_agent_through_the_prerequisites(self) -> None: + # The call site in `verify.build_task`: the solver above is only worth + # anything if the task it builds is the one that uses it, and the agent + # it is handed is the real Claude Code agent built with the env. + self.repo.skill("boxed", dataset=tier0_dataset("boxed"), machine=_machine(_tool())) + from inspect_ai.solver import solver + + @solver + def noop(): + async def solve(state, generate): + return state + + return solve + + seen: dict = {} + + def fake(skill, make_agent): + seen["skill"] = skill + seen["agent"] = make_agent({"PREREQ_TOOL": "/x"}) + return noop() + + with mock.patch.object(engine_prerequisites, "claude_code_agent", fake), \ + _fake_inspect_swe(lambda **kw: kw): + engine_verify.build_task( + "boxed", + [datasets.Case(id="b1", skill="boxed", prompt="p", + skill_should_trigger=True, logs_contain=["x"])], + "mockllm/model", + effort="high", + ) + self.assertEqual(seen["skill"], "boxed") + self.assertEqual(seen["agent"]["env"], {"PREREQ_TOOL": "/x"}) + self.assertEqual(seen["agent"]["effort"], "high") + self.assertEqual(seen["agent"]["skills"], [datasets.skill_path("boxed")]) + self.assertEqual(seen["agent"]["cwd"], engine_tools.workdir_path()) + + def test_a_skill_without_prerequisites_gets_the_agent_as_before(self) -> None: + self.repo.skill("bare", dataset=tier0_dataset("bare")) + built = [] + + def claude_code(**kwargs): + built.append(kwargs) + return _noop_solver() + + with _fake_inspect_swe(claude_code): + engine_verify.build_task( + "bare", + [datasets.Case(id="b1", skill="bare", prompt="p", + skill_should_trigger=True, logs_contain=["x"])], + "mockllm/model", + ) + [kwargs] = built + self.assertIsNone(kwargs["env"]) + self.assertEqual(kwargs["skills"], [datasets.skill_path("bare")]) + self.assertEqual(kwargs["cwd"], engine_tools.workdir_path()) + + def test_the_sandboxed_run_keeps_one_failure_to_its_case_or_skill(self) -> None: + # A declaration the engine refuses fails its skill and the next skill + # still runs; and inspect is told not to end a task on one case's error. + self.repo.skill("bad", dataset=tier0_dataset("bad")) + self.repo.skill("good", dataset=tier0_dataset("good")) + cases = [datasets.Case(id=f"{s}-1", skill=s, prompt="p", + skill_should_trigger=True, logs_contain=["x"]) + for s in ("bad", "good")] + calls = [] + + def build_task(skill, *a, **k): + if skill == "bad": + # Words a provider outage is recognised by, as build output has. + raise engine_prerequisites.PrerequisiteError( + "names the answer key; upstream 502 gateway timeout") + return "task" + + def run_eval(inspect_eval, task, **kwargs): + calls.append(kwargs) + return [] + + with mock.patch.object(engine_verify, "require"), \ + mock.patch.object(engine_sandbox, "require_provider"), \ + mock.patch.object(engine_verify, "build_task", build_task), \ + mock.patch.object(engine_behavioral, "run_eval", run_eval), \ + contextlib.redirect_stdout(io.StringIO()): + outcomes = engine_verify.run(["bad", "good"], cases, "mockllm/model", "high") + bad = [o for o in outcomes if o.skill == "bad"] + self.assertTrue(bad and bad[0].error.startswith("bad: "), bad) + self.assertTrue("answer key" in bad[0].error) + self.assertFalse(bad[0].degraded) + [good_call] = calls + self.assertIs(good_call["fail_on_error"], False) + + def test_a_malformed_declaration_fails_only_its_skill_in_the_sandbox(self) -> None: + # Named so its path carries words a provider outage is recognised by. + self.repo.skill("upstream-gateway", dataset=tier0_dataset("bad"), + machine=json.dumps({"prerequisites": [5]})) + self.repo.skill("good", dataset=tier0_dataset("good")) + cases = [datasets.Case(id=f"{s}-1", skill=s, prompt="p", + skill_should_trigger=True, logs_contain=["x"]) + for s in ("upstream-gateway", "good")] + ran = [] + with mock.patch.object(engine_verify, "require"), \ + mock.patch.object(engine_sandbox, "require_provider"), \ + mock.patch.object(engine_behavioral, "run_eval", + lambda e, task, **k: (ran.append(task.name), [])[1]), \ + _fake_inspect_swe(lambda **kw: _noop_solver()), \ + contextlib.redirect_stdout(io.StringIO()): + outcomes = engine_verify.run( + ["upstream-gateway", "good"], cases, "mockllm/model", "high") + [bad] = [o for o in outcomes if o.skill == "upstream-gateway"] + self.assertIn("must be a mapping", bad.error) + self.assertFalse(bad.degraded) + self.assertEqual(ran, ["claude-code-good"]) + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/tools/sandbox_smoketest.py b/tools/sandbox_smoketest.py index 42194be..fe5d7cb 100644 --- a/tools/sandbox_smoketest.py +++ b/tools/sandbox_smoketest.py @@ -23,7 +23,17 @@ listing was matched -- and it cannot be passed by an agent that got lucky. No model is ever called, so this costs nothing and is deterministic. +With `--prerequisites` the stand-in does one thing more. It runs after the +skill's `machine.yml` prerequisites have been provided in the container, the +way `--engine claude-code` provides them, and checks from inside the container +that the env it was handed names something that was installed and that the +copied steps are gone. Only then does it write the file the case expects, so +here the expected file is the stand-in's report rather than a seeded fixture. +That covers the prerequisite step itself, not `verify.build_task` or the +real agent's `env=`, which the unit tests cover. + tools/sandbox_smoketest.py [--skill demo-skill] [--expect-sandbox docker] + tools/sandbox_smoketest.py --skill prereq-skill --expect-sandbox docker --prerequisites Exits non-zero, loudly, on the first thing that did not hold. """ @@ -44,7 +54,7 @@ MODEL = "mockllm/model" -def stub_solver(skill_dir: Path): +def stub_solver(skill_dir: Path, env: dict[str, str]): """An agent that does nothing, so only the machinery can pass the case.""" from inspect_ai.solver import solver @@ -58,6 +68,49 @@ async def solve(state, generate): return _noop() +def prerequisites_solver(skill: str): + """The `claude-code` leg's prerequisite step, with a stub for the agent. + + Runs the skill's declared prerequisites in the real container, then hands + their env to a stand-in agent that checks, from inside the same container, + the three things the agent relies on: the env arrived, what it names was + installed, and the steps directory -- which held files copied from evals/ + -- is gone. It leaves a marker the case asserts, so a stand-in that never + ran cannot pass. + """ + from inspect_ai.agent import agent + from inspect_ai.util import sandbox + + from skillscope.engine import prerequisites, tools + + def make_agent(env: dict[str, str]): + @agent + def stand_in(): + async def execute(state): + box = sandbox() + tool = env.get("PREREQ_TOOL") + if not tool: + raise RuntimeError(f"the agent was given no PREREQ_TOOL: {env}") + if not (await box.exec(["test", "-f", tool])).success: + raise RuntimeError(f"{tool} was not installed in the container") + steps = prerequisites.CONTAINER_STEPS + if (await box.exec(["test", "-e", steps])).success: + raise RuntimeError(f"{steps} was left for the agent to see") + workdir = tools.workdir_path() + await box.exec(["mkdir", "-p", workdir]) + await box.write_file(f"{workdir}/prerequisites-ok.txt", "ok\n") + return state + + return execute + + return stand_in() + + solver = prerequisites.claude_code_agent(skill, make_agent) + if solver is None: + fail(f"{skill} declares no prerequisites, so nothing is proven") + return solver + + def fail(message: str) -> None: print(f"FAIL: {message}", file=sys.stderr) raise SystemExit(1) @@ -67,6 +120,15 @@ def main() -> int: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("repo", help="Repo holding the fixture skill.") parser.add_argument("--skill", default="demo-skill") + parser.add_argument( + "--prerequisites", + action="store_true", + help=( + "Run the skill's machine.yml prerequisites in the sandbox, the way " + "`--engine claude-code` does, before a stand-in agent that checks " + "what they provided. Needs a real container." + ), + ) parser.add_argument( "--expect-sandbox", default="", @@ -103,11 +165,28 @@ def main() -> int: f"unsandboxed run passes every check below for the wrong reason." ) + # The container leg's prerequisite step writes to a fixed path in the + # guest. On an unisolated provider the guest is this machine. + if args.prerequisites and provider in sandbox_spec.NOT_ISOLATED: + fail( + f"--prerequisites needs a container; {provider!r} would run the " + "steps on this machine" + ) + sandbox_spec.require_provider() from inspect_ai import eval as inspect_eval logs = inspect_eval( - behavioral.build_task(args.skill, cases, MODEL, solver_factory=stub_solver), + behavioral.build_task( + args.skill, + cases, + MODEL, + solver_factory=( + (lambda skill_dir, env: prerequisites_solver(args.skill)) + if args.prerequisites + else stub_solver + ), + ), model=MODEL, log_dir=str(Path(".skillscope") / "logs"), log_realtime=behavioral.realtime_logging(), @@ -136,14 +215,14 @@ def main() -> int: seeded = [c for c in checks if c.get("kind") == "files_exist"] if not seeded: - fail("the seeded-file check did not run, so staging was never exercised") + fail("the expected-file check did not run, so nothing was exercised") unmet = [c for c in seeded if not c.get("passed")] if unmet: - fail(f"a file the case seeded was not found in the sandbox: {unmet}") + fail(f"a file the case expects was not found in the sandbox: {unmet}") print( f"[smoketest] ok -- {len(checks)} check(s) graded, " - f"{len(seeded)} seeded fixture(s) found, sandbox {provider!r}." + f"{len(seeded)} expected file(s) found, sandbox {provider!r}." ) return 0 From 6a0a18cf0728623a8e6ffe3a3c0ce753972002aa Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 30 Oct 2026 11:08:58 +0200 Subject: [PATCH 2/3] Say which prerequisites the environment already had `check` exiting 0 drops that prerequisite's `env`, and the log said only how many there were, so a runner that already had the tool and one that installed it looked the same. The line now names each branch. The docs state the obligation that goes with it: `check` passes only when the agent would work without the `env`. `PATH` is refused. A value replaces the variable, the CLI is already resolved, and everything the agent starts would see only that directory. `/opt/skillscope/cache` is documented as provisional until a skill's image is built against it. Signed-off-by: Roman Inflianskas Co-authored-by: Cursor --- docs/authoring-evals.md | 14 +++-- skillscope/datasets.py | 29 +++++++--- skillscope/engine/prerequisites.py | 61 ++++++++++++++++---- skillscope/schema/machine.schema.json | 6 +- tests/test_skillscope.py | 82 ++++++++++++++++++++++++--- 5 files changed, 159 insertions(+), 33 deletions(-) diff --git a/docs/authoring-evals.md b/docs/authoring-evals.md index e08576b..7a31d24 100644 --- a/docs/authoring-evals.md +++ b/docs/authoring-evals.md @@ -169,7 +169,9 @@ prerequisites: - **The order is check, then install, then check again.** - `check` runs first, without this prerequisite's `env`. If it exits 0, the prerequisite is already there: nothing is installed and its `env` is not - applied, because whatever provides it already says where it is. + applied, because whatever provides it already says where it is. Write + `check` so it exits 0 only when the agent would work without this `env`. + The log names which were satisfied by the environment and which were installed. - Otherwise `install` runs, then `check` runs again with `env` applied. If it still fails, the skill's cases are reported as failed, with the step's output. @@ -209,11 +211,13 @@ prerequisites: value naming it fails before anything is installed when it is missing. - `SKILL_CACHE_DIR` is scratch space for what `install` builds. On the host it is removed after the skill's last case. In a container it is - `/opt/skillscope/cache`. + `/opt/skillscope/cache`. That path is provisional until a skill's image + is built against it. - **An `env` value replaces the variable, it does not extend it.** No other - `$` is expanded, so `PATH: ${SKILL_CACHE_DIR}/bin` would leave the agent with - that one directory. Variables the harness sets for the agent - (`ANTHROPIC_*`, `CLAUDE_*`, `IS_SANDBOX`) cannot be set here. + `$` is expanded. `PATH` cannot be set here: it would replace the agent's + search path, and everything the agent starts would see only that value. + Variables the harness sets for the agent (`ANTHROPIC_*`, `CLAUDE_*`, + `IS_SANDBOX`) cannot be set here either. - **Installing once per image instead of once per case.** Under `claude-code`, build your `sandbox:` image so that `check` passes in it without this prerequisite's `env`: run the same `install` at `SKILL_CACHE_DIR=/opt/skillscope/cache` diff --git a/skillscope/datasets.py b/skillscope/datasets.py index e69bcb3..7489ce3 100644 --- a/skillscope/datasets.py +++ b/skillscope/datasets.py @@ -553,10 +553,15 @@ def tier0_errors(skill: str, cases: list[Case]) -> list[str]: # host and a container -- the same declaration would mean two things. PREREQUISITE_INPUTS = ("SKILL_SOURCE_DIR", "SKILL_CACHE_DIR") _ENV_INPUT = re.compile(r"\$\{(?:SKILL_SOURCE_DIR|SKILL_CACHE_DIR)\}") -# Variables the harness sets for the agent itself. A prerequisite overriding -# one would change which model answers or where its config lives -- and -# differently per engine, since each sets a different subset. -_RESERVED_ENV = re.compile(r"^(?:ANTHROPIC_|CLAUDE_)|^IS_SANDBOX$", re.IGNORECASE) +# Variables a prerequisite may not set. The harness ones decide which model +# answers and where its config lives, and each engine sets a different subset. +# `PATH` is replaced rather than extended: the CLI is already resolved by the +# time the agent starts, so a replacement leaves everything it spawns with +# only that directory. Windows environment names are case-insensitive, so +# `path` is the same variable. +_RESERVED_ENV = re.compile( + r"^(?:ANTHROPIC_|CLAUDE_)|^IS_SANDBOX$|^PATH$", re.IGNORECASE +) # A name a process environment can hold on every platform. _ENV_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") # `machine.yml`'s `os` labels as `COMMAND_PLATFORMS`. @@ -728,10 +733,18 @@ def _prerequisite_env(path: Path, where: str, value: object) -> dict[str, str]: "supplies to every step; a prerequisite may not override it." ) if _RESERVED_ENV.search(name): - raise SystemExit( - f"error: {path}: `{where}.{name}` is set by the harness for the " - "agent itself, and a prerequisite may not override it." - ) + if name.upper() == "PATH": + why = ( + "replaces PATH rather than extending it, so the agent and " + "everything it starts would see only that value, and a " + "prerequisite may not set it." + ) + else: + why = ( + "is set by the harness for the agent itself, and a " + "prerequisite may not override it." + ) + raise SystemExit(f"error: {path}: `{where}.{name}` {why}") if "$" in _ENV_INPUT.sub("", text): raise SystemExit( f"error: {path}: `{where}.{name}` uses `$` other than in " diff --git a/skillscope/engine/prerequisites.py b/skillscope/engine/prerequisites.py index 031315e..42676c8 100644 --- a/skillscope/engine/prerequisites.py +++ b/skillscope/engine/prerequisites.py @@ -17,7 +17,8 @@ * ``check`` exits 0: the prerequisite is already there. Nothing is installed and its ``env`` is not applied -- the environment that provides it already - says where it is. + says where it is. An author writes ``check`` so that is true only when the + agent would work without the ``env``. * Otherwise ``install`` runs, then ``check`` again *with* the ``env`` applied, because an installed tool is usually only findable through it. Still failing is the prerequisite's failure, reported with the step's own output. @@ -32,7 +33,8 @@ * ``SKILL_CACHE_DIR`` -- scratch space for what ``install`` builds. Per skill on the host, removed after its cases; a fixed path in each case's container, so an image that bakes the same ``install`` in at that path makes ``check`` - pass and skips the per-case cost. + pass and skips the per-case cost. That path is provisional until a skill's + image is built against it. Where each engine runs it: @@ -54,6 +56,7 @@ import subprocess import tempfile from collections.abc import Awaitable, Callable, Iterator +from dataclasses import dataclass from pathlib import Path from .. import datasets, deadline, sources @@ -62,7 +65,8 @@ # Where a container keeps the inputs. Fixed rather than random so that an image # can bake an `install` in at the same `SKILL_CACHE_DIR` and have `check` find -# it; documented in docs/authoring-evals.md, so changing it breaks those images. +# it. Provisional until a skill's image is built against it: documented as such +# in docs/authoring-evals.md, and nothing pins the path yet. CONTAINER_ROOT = "/opt/skillscope" CONTAINER_CACHE = f"{CONTAINER_ROOT}/cache" # Where the files a step names are copied, and removed again before the agent @@ -121,14 +125,41 @@ def _tail(output: str) -> str: return output if len(output) <= OUTPUT_TAIL else "..." + output[-OUTPUT_TAIL:] +@dataclass(frozen=True) +class Provision: + """The env the agent needs, and which prerequisites produced it. + + `satisfied` passed `check`, so nothing was installed and none of their + `env` was applied. `installed` are the ones this run provided. + """ + + env: dict[str, str] + satisfied: tuple[str, ...] + installed: tuple[str, ...] + + +def _provision_line(skill: str, provision: Provision) -> str: + """Name each branch `provide` took. The count alone does not.""" + parts: list[str] = [] + if provision.satisfied: + parts.append(f"{', '.join(provision.satisfied)} satisfied by the environment") + if provision.installed: + parts.append(f"{', '.join(provision.installed)} installed") + return f"[prerequisites] {skill}: {'; '.join(parts)}" + + async def provide( declared: list[Prerequisite], inputs: dict[str, str], where: str, run: Runner, unavailable: dict[str, str] | None = None, -) -> dict[str, str]: - """Apply `declared` in order; return the env the agent needs. +) -> Provision: + """Apply `declared` in order. + + The result's `env` is what the agent needs. `satisfied` and `installed` + name which prerequisites were already present and which this run provided, + because a count alone is the same either way. `where` is the platform the steps run on, and `run` runs one argv with extra environment, returning its exit code and combined output. Each @@ -137,6 +168,8 @@ async def provide( `inputs`, for the error raised if an `env` value names it. """ applied: dict[str, str] = {} + satisfied: list[str] = [] + installed: list[str] = [] for item in declared: check = item.command("check", where) install = item.command("install", where) @@ -147,6 +180,7 @@ async def provide( step_env = {**inputs, **applied} code, _ = await run(check, step_env) if code == 0: + satisfied.append(item.name) continue # Before the install rather than after it: an `env` this engine cannot # fill should not cost a full build to discover. @@ -173,7 +207,8 @@ async def provide( f"(check exited {code}).\n{_tail(output)}" ) applied.update(env) - return applied + installed.append(item.name) + return Provision(applied, tuple(satisfied), tuple(installed)) # What the `--timeout` watchdog's hard exit has to clean up, because `os._exit` @@ -302,7 +337,7 @@ def on_host(skill: str) -> Iterator[dict[str, str]]: except (SystemExit, Exception) as exc: unavailable["SKILL_SOURCE_DIR"] = str(exc).removeprefix("error: ") print(f"[prerequisites] {skill}: {len(declared)} to provide", flush=True) - env = asyncio.run( + provision = asyncio.run( provide( declared, inputs, @@ -311,7 +346,8 @@ def on_host(skill: str) -> Iterator[dict[str, str]]: unavailable, ) ) - yield env + print(_provision_line(skill, provision), flush=True) + yield provision.env finally: shutil.rmtree(cache, ignore_errors=True) _created_caches.discard(str(cache)) @@ -404,7 +440,11 @@ async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: await box.write_file( f"{CONTAINER_STEPS}/{relative}", path.read_bytes() ) - env = await provide( + print( + f"[prerequisites] {skill}: {len(declared)} to provide", + flush=True, + ) + provision = await provide( declared, {"SKILL_CACHE_DIR": CONTAINER_CACHE}, "linux", @@ -415,6 +455,7 @@ async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: "image instead." }, ) + print(_provision_line(skill, provision), flush=True) except BaseException: # Best effort, and never in place of the error being raised. with contextlib.suppress(Exception): @@ -427,7 +468,7 @@ async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: f"could not remove {CONTAINER_STEPS} before the agent " f"starts: {_tail(removed.stderr)}" ) - return await as_solver(make_agent(env))(state, generate) + return await as_solver(make_agent(provision.env))(state, generate) return solve diff --git a/skillscope/schema/machine.schema.json b/skillscope/schema/machine.schema.json index 9b5430d..332894d 100644 --- a/skillscope/schema/machine.schema.json +++ b/skillscope/schema/machine.schema.json @@ -29,7 +29,7 @@ "prerequisites": { "type": "array", "minItems": 1, - "description": "What the behavioral cases need provided before they run, applied by every engine where its agent runs: once per skill on the host under the default engine, in each case's container under `--engine claude-code`. Each runs `check`; if that fails, `install` runs and `check` is retried with `env` applied, and only then is `env` handed to the agent. A prerequisite whose `check` passes is used as is and none of its `env` is applied. On the host, steps run with the directory holding this file as their working directory; in a container, in `/opt/skillscope/steps`, which holds only the files they name. Steps see `SKILL_SOURCE_DIR` (the skill's source tree; host only) and `SKILL_CACHE_DIR` (scratch space, removed after the skill on the host; `/opt/skillscope/cache` in a container, so an image can bake an install in at the same path).", + "description": "What the behavioral cases need provided before they run, applied by every engine where its agent runs: once per skill on the host under the default engine, in each case's container under `--engine claude-code`. Each runs `check`; if that fails, `install` runs and `check` is retried with `env` applied, and only then is `env` handed to the agent. A prerequisite whose `check` passes is used as is and none of its `env` is applied, so `check` must exit 0 only when the agent would work without that `env`. On the host, steps run with the directory holding this file as their working directory; in a container, in `/opt/skillscope/steps`, which holds only the files they name. Steps see `SKILL_SOURCE_DIR` (the skill's source tree; host only) and `SKILL_CACHE_DIR` (scratch space, removed after the skill on the host; `/opt/skillscope/cache` in a container, so an image can bake an install in at the same path; provisional until a skill's image is built against it).", "items": { "type": "object", "additionalProperties": false, @@ -42,7 +42,7 @@ }, "check": { "$ref": "#/$defs/command", - "description": "Exits 0 when the prerequisite is present. Files it names are copied into a container the same way as for `install`." + "description": "Exits 0 only when the agent would work without this prerequisite's `env`. A check that passes while the variable the agent needs is unset skips the install and never applies `env`. Files it names are copied into a container the same way as for `install`." }, "install": { "$ref": "#/$defs/command", @@ -51,7 +51,7 @@ "env": { "type": "object", "additionalProperties": { "type": "string" }, - "description": "Set for the agent once the prerequisite has been installed. A value replaces the variable rather than extending it. Values may refer to `${SKILL_SOURCE_DIR}` and `${SKILL_CACHE_DIR}`, and no other `$` may appear; names the harness sets for the agent (`ANTHROPIC_*`, `CLAUDE_*`, `IS_SANDBOX`) are refused. The skillscope parser enforces those rules, a command for every platform in `os`, and unique names, none of which this schema expresses.", + "description": "Set for the agent once the prerequisite has been installed. A value replaces the variable rather than extending it. Values may refer to `${SKILL_SOURCE_DIR}` and `${SKILL_CACHE_DIR}`, and no other `$` may appear; `PATH` and the names the harness sets for the agent (`ANTHROPIC_*`, `CLAUDE_*`, `IS_SANDBOX`) are refused. The skillscope parser enforces those rules, a command for every platform in `os`, and unique names, none of which this schema expresses.", "examples": [{ "PYTHONPATH": "${SKILL_CACHE_DIR}/decoder/python" }] } } diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 789e4e1..46f29d8 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6148,7 +6148,10 @@ def test_each_mistake_is_named(self) -> None: (_machine(_tool(env={"ANTHROPIC_BASE_URL": "x"})), "set by the harness"), (_machine(_tool(env={"CLAUDE_CONFIG_DIR": "x"})), "set by the harness"), (_machine(_tool(env={"IS_SANDBOX": "0"})), "set by the harness"), - # Windows environment names are case-insensitive. + (_machine(_tool(env={"PATH": "${SKILL_CACHE_DIR}/bin"})), "replaces PATH"), + # Windows environment names are case-insensitive, so `Path` is + # `PATH` and `anthropic_api_key` is `ANTHROPIC_API_KEY`. + (_machine(_tool(env={"Path": "/usr/bin"})), "replaces PATH"), (_machine(_tool(env={"anthropic_api_key": "x"})), "set by the harness"), (_machine(_tool(env={"A=B": "x"})), "cannot be set in a process environment"), (_machine(_tool(env={"1A": "x"})), "cannot be set in a process environment"), @@ -6244,12 +6247,12 @@ async def run(argv, env): return (0 if recheck_ok else 1), "still missing" return (0, "built") if install_ok else (2, "compiler exploded") - env = asyncio.run( + provision = asyncio.run( engine_prerequisites.provide( declared, {"SKILL_CACHE_DIR": "/cache"}, "linux", run ) ) - return env, calls + return provision, calls def declared(self, **env): return [datasets.Prerequisite( @@ -6258,14 +6261,18 @@ def declared(self, **env): )] def test_present_means_nothing_installed_and_no_env(self) -> None: - env, calls = self.run_provide(self.declared(), present=True) - self.assertEqual(env, {}) + provision, calls = self.run_provide(self.declared(), present=True) + self.assertEqual(provision.env, {}) + self.assertEqual(provision.satisfied, ("tool",)) + self.assertEqual(provision.installed, ()) self.assertEqual([s for s, _ in calls], ["check"]) def test_missing_means_install_then_check_again_with_the_env(self) -> None: - env, calls = self.run_provide(self.declared()) + provision, calls = self.run_provide(self.declared()) self.assertEqual([s for s, _ in calls], ["check", "install", "check"]) - self.assertEqual(env, {"TOOL": "/cache/tool"}) + self.assertEqual(provision.env, {"TOOL": "/cache/tool"}) + self.assertEqual(provision.installed, ("tool",)) + self.assertEqual(provision.satisfied, ()) # The re-check sees the declared env; the steps before it do not. self.assertNotIn("TOOL", calls[1][1]) self.assertEqual(calls[2][1]["TOOL"], "/cache/tool") @@ -6368,6 +6375,35 @@ async def run(argv, env): second = next(env for step, env in seen if step == "second-check") self.assertEqual(second["A"], "/cache/a") + def test_the_report_names_each_branch(self) -> None: + # A check that passes and one that does not must be distinguishable. + # The count of prerequisites is the same either way. + present = datasets.Prerequisite( + "system", {"*": ["present"]}, {"*": ["install-present"]}, {"S": "1"} + ) + missing = datasets.Prerequisite( + "built", {"*": ["missing"]}, {"*": ["install-missing"]}, + {"B": "${SKILL_CACHE_DIR}/b"}, + ) + + async def run(argv, env): + # The first `missing` check fails; install succeeds; the re-check + # sees the env the install just applied. + if argv[0] in ("present", "install-missing") or env.get("B"): + return 0, "" + return 1, "" + + provision = asyncio.run(engine_prerequisites.provide( + [present, missing], {"SKILL_CACHE_DIR": "/cache"}, "linux", run + )) + self.assertEqual(provision.satisfied, ("system",)) + self.assertEqual(provision.installed, ("built",)) + self.assertEqual(provision.env, {"B": "/cache/b"}) + self.assertEqual( + engine_prerequisites._provision_line("demo", provision), + "[prerequisites] demo: system satisfied by the environment; built installed", + ) + class TestPrerequisitesOnTheDefaultEngine(unittest.TestCase): """The host leg end to end: real solver, real inspect `eval()`, stub `claude`. @@ -6434,6 +6470,25 @@ def test_a_missing_prerequisite_is_installed_once_and_every_case_can_use_it(self self.assertTrue(agent["env"].get("PREREQ_TOOL"), agent) self.assertTrue(agent["exists"]["PREREQ_TOOL"], agent) + def test_the_log_names_the_branch_that_provided_it(self) -> None: + self.skill("needs-tool", _machine(_tool())) + out = io.StringIO() + with contextlib.redirect_stdout(out), engine_prerequisites.on_host("needs-tool"): + pass + text = out.getvalue() + self.assertIn("[prerequisites] needs-tool: 1 to provide", text) + self.assertIn("[prerequisites] needs-tool: tool installed", text) + + existing = self.scratch / "preinstalled.txt" + existing.write_text("x") + os.environ["PREREQ_TOOL"] = str(existing) + out = io.StringIO() + with contextlib.redirect_stdout(out), engine_prerequisites.on_host("needs-tool"): + pass + text = out.getvalue() + self.assertIn("tool satisfied by the environment", text) + self.assertNotIn("installed", text) + def test_a_prerequisite_already_present_is_used_as_is(self) -> None: existing = self.scratch / "preinstalled.txt" existing.write_text("x") @@ -6806,6 +6861,19 @@ def test_a_skill_with_none_gets_the_agent_it_always_had(self) -> None: self.repo.skill("bare", dataset=tier0_dataset("bare")) self.assertIsNone(engine_prerequisites.claude_code_agent("bare", lambda env: None)) + def test_the_log_names_the_branch_in_the_container_too(self) -> None: + out = io.StringIO() + with contextlib.redirect_stdout(out): + self.run_solver(_machine(_tool())) + self.assertIn("[prerequisites] boxed: tool installed", out.getvalue()) + + out = io.StringIO() + with contextlib.redirect_stdout(out): + self.run_solver(_machine(_tool()), present=True) + text = out.getvalue() + self.assertIn("tool satisfied by the environment", text) + self.assertNotIn("installed", text) + def test_the_same_declaration_prepares_the_container_before_the_agent(self) -> None: box, [env] = self.run_solver(_machine(_tool())) steps = [argv for argv, _, _ in box.calls if argv[0] == _PY] From 3a183f922b591d9464950857a384d92f70453e46 Mon Sep 17 00:00:00 2001 From: Roman Inflianskas Date: Fri, 30 Oct 2026 11:42:22 +0200 Subject: [PATCH 3/3] Close the gaps the review left for a follow-up A case-different path to the answer key compared unequal after resolve(), so on a case-insensitive volume it was copied into the container. The file is what is compared now, and a path that resolves outside evals/ is covered by a test. An os label that is not a platform was skipped, so Liunx met the command-for-every-platform rule by never being checked. A newline in an env name passed `$`, and an empty argv item passed a check that only looked at the first one. The watchdog snapshot and the pid registration share a lock. A step that appears after the snapshot is killed by the spawner, because os._exit will not look again. A machine.yml that does not parse no longer drops the matrix for every skill after it. Signed-off-by: Roman Inflianskas Co-authored-by: Cursor --- docs/authoring-evals.md | 4 +- skillscope/datasets.py | 21 +++++-- skillscope/engine/prerequisites.py | 92 ++++++++++++++++++++++++------ skillscope/selection.py | 26 +++++++-- tests/test_skillscope.py | 73 ++++++++++++++++++++++-- 5 files changed, 184 insertions(+), 32 deletions(-) diff --git a/docs/authoring-evals.md b/docs/authoring-evals.md index 7a31d24..40a4c3b 100644 --- a/docs/authoring-evals.md +++ b/docs/authoring-evals.md @@ -187,7 +187,9 @@ prerequisites: - **Commands are argv lists.** A command may also be a mapping per platform, for example `{linux: [sh, x.sh], windows: [pwsh, x.ps1]}`. A container always counts as `linux`, and the structural check wants a command for every - platform in `os`. On the host, steps run in this directory. In a container + platform in `os`. 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. On the host, steps run in this directory. In a container they run in a scratch directory holding only the files they name. - **What a container gets.** Under `claude-code`, an argv item of `check` or `install` that is a path to a file in this directory or below it, like diff --git a/skillscope/datasets.py b/skillscope/datasets.py index 7489ce3..4385986 100644 --- a/skillscope/datasets.py +++ b/skillscope/datasets.py @@ -562,7 +562,9 @@ def tier0_errors(skill: str, cases: list[Case]) -> list[str]: _RESERVED_ENV = re.compile( r"^(?:ANTHROPIC_|CLAUDE_)|^IS_SANDBOX$|^PATH$", re.IGNORECASE ) -# A name a process environment can hold on every platform. +# A name a process environment can hold on every platform. `fullmatch`, +# because `$` also matches before a trailing newline: `"FOO\n"` would +# otherwise pass, and the error next to the check says it cannot. _ENV_NAME = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") # `machine.yml`'s `os` labels as `COMMAND_PLATFORMS`. _OS_PLATFORM = {"linux": "linux", "windows": "windows", "macos": "macos"} @@ -680,7 +682,7 @@ def _argv(path: Path, where: str, value: object) -> list[str]: if ( not isinstance(value, list) or not value - or not all(isinstance(item, str) for item in value) + or not all(isinstance(item, str) and item for item in value) or not value[0].strip() ): raise SystemExit( @@ -721,7 +723,7 @@ def _prerequisite_env(path: Path, where: str, value: object) -> dict[str, str]: f"got {value!r}." ) for name, text in value.items(): - if not _ENV_NAME.match(name) or "\0" in text: + if not _ENV_NAME.fullmatch(name) or "\0" in text: raise SystemExit( f"error: {path}: `{where}` has an entry that cannot be set in a " f"process environment: {name!r}. Names are letters, digits and " @@ -760,11 +762,22 @@ def _prerequisite_env(path: Path, where: str, value: object) -> dict[str, str]: def _prerequisite_gaps( skill: str, plan: dict, declared: list[Prerequisite] ) -> list[str]: - """Each platform the skill runs on that a prerequisite has no command for.""" + """Each platform the skill runs on that a prerequisite has no command for. + + An `os` label that is not a platform is reported too. Skipping it let + `os: [Liunx]` satisfy the check without a command for any real platform. + """ gaps: list[str] = [] + if not declared: + return gaps for label in plan["os"]: platform = _OS_PLATFORM.get(label.lower()) if platform is None: + gaps.append( + f"{machine_path(skill)}: `os` names {label!r}, which is not " + "linux, windows, or macos. A prerequisite's command for it " + "cannot be checked. Use one of those, or drop the label." + ) continue for item in declared: missing = [ diff --git a/skillscope/engine/prerequisites.py b/skillscope/engine/prerequisites.py index 42676c8..841d866 100644 --- a/skillscope/engine/prerequisites.py +++ b/skillscope/engine/prerequisites.py @@ -55,6 +55,7 @@ import signal import subprocess import tempfile +import threading from collections.abc import Awaitable, Callable, Iterator from dataclasses import dataclass from pathlib import Path @@ -217,7 +218,11 @@ async def provide( _created_caches: set[str] = set() # Host steps still running, by pid. Each leads a session or process group of # its own (see `_host_runner`), so the pid names everything it started too. +# The lock is shared with the watchdog: the pid is only known once the +# process exists, and a snapshot that missed it must not be the last look. _live_steps: set[int] = set() +_steps_lock = threading.Lock() +_steps_closed = False def _discard_all_caches() -> None: @@ -226,25 +231,54 @@ def _discard_all_caches() -> None: _created_caches.clear() +def _kill_pid(pid: int) -> None: + try: + if os.name == "nt": + subprocess.run( + ["taskkill", "/F", "/T", "/PID", str(pid)], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + timeout=5, + ) + else: + os.killpg(pid, signal.SIGKILL) + except (OSError, subprocess.SubprocessError): + pass + + +def _track_step(pid: int) -> bool: + """Record `pid` for the watchdog. + + True when the watchdog has already snapped the set. The caller kills + `pid` itself: `os._exit` will not look again, and a session leader it + missed would be reparented and keep running. + """ + with _steps_lock: + if _steps_closed: + return True + _live_steps.add(pid) + return False + + +def _untrack_step(pid: int) -> None: + with _steps_lock: + _live_steps.discard(pid) + + def _kill_live_steps() -> None: """Kill every running host step and what it started. For the watchdog. Synchronous, because the watchdog fires from a thread with no event loop. """ - for pid in list(_live_steps): - try: - if os.name == "nt": - subprocess.run( - ["taskkill", "/F", "/T", "/PID", str(pid)], - stdout=subprocess.DEVNULL, - stderr=subprocess.DEVNULL, - timeout=5, - ) - else: - os.killpg(pid, signal.SIGKILL) - except (OSError, subprocess.SubprocessError): - pass - _live_steps.discard(pid) + global _steps_closed + with _steps_lock: + # Stays set. The process is about to exit, and a step that appears + # after this snapshot has to be killed by whoever started it. + _steps_closed = True + pids = list(_live_steps) + _live_steps.clear() + for pid in pids: + _kill_pid(pid) def _on_expire() -> None: @@ -292,7 +326,10 @@ async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: ) except (OSError, ValueError) as exc: return 127, f"could not start {argv[0]!r}: {exc}" - _live_steps.add(proc.pid) + # After the spawn, under the watchdog's lock. If the hard exit already + # snapped the set, kill here: nothing else will. + if _track_step(proc.pid): + _kill_pid(proc.pid) try: output, _ = await proc.communicate() except BaseException: @@ -301,7 +338,7 @@ async def run(argv: list[str], extra: dict[str, str]) -> tuple[int, str]: await asyncio.shield(no_sandbox._terminate(proc)) raise finally: - _live_steps.discard(proc.pid) + _untrack_step(proc.pid) return proc.returncode, output.decode(errors="replace") return run @@ -353,12 +390,31 @@ def on_host(skill: str) -> Iterator[dict[str, str]]: _created_caches.discard(str(cache)) +def _is_answer_key(candidate: Path, keys: set[Path]) -> bool: + """Whether `candidate` is a dataset file, whatever path named it. + + `Path.resolve()` does not canonicalise case, so on a case-insensitive + volume `EVALS.JSON` and `evals.json` compare unequal and the answer key + would be copied in. `samefile` is the directory entry. + """ + for key in keys: + if candidate == key: + return True + try: + if candidate.samefile(key): + return True + except OSError: + continue + return False + + def _named_files(skill: str, declared: list[Prerequisite]) -> dict[str, Path]: """The files beside machine.yml that a container step names, by argv item. Only these are copied in. The rest of evals/ includes the answer key, and a step has no business carrying it into the agent's room -- so naming a - dataset file is refused rather than copied. + dataset file is refused rather than copied. A path that resolves outside + this directory is not copied either. """ evals_dir = datasets.machine_path(skill).parent.resolve() keys = { @@ -377,7 +433,7 @@ def _named_files(skill: str, declared: list[Prerequisite]) -> dict[str, Path]: continue except (OSError, ValueError): continue - if candidate in keys: + if _is_answer_key(candidate, keys): raise PrerequisiteError( f"prerequisite `{item.name}` names {arg}, which is this " "skill's answer key and is never copied into the agent's " diff --git a/skillscope/selection.py b/skillscope/selection.py index e8b926c..54d5f99 100644 --- a/skillscope/selection.py +++ b/skillscope/selection.py @@ -48,7 +48,9 @@ ``skipped`` holds legs whose gate label is missing from the pull request. They are reported so the gate job can warn that a change shipped without ever running on real hardware, rather than failing a pull request for a test it -deliberately did not request. +deliberately did not request. A leg whose ``machine.yml`` could not be read +is held back the same way, with ``error`` set and ``gate`` empty, so one +broken file does not drop the matrix for every other skill. """ from __future__ import annotations @@ -169,7 +171,11 @@ def matrix_entries( ignore_gates: bool = False, extended: bool = True, ) -> tuple[list[dict], list[dict]]: - """Split `skills` into matrix legs to run and legs held back by a gate.""" + """Split `skills` into matrix legs to run and legs held back. + + A missing gate holds a leg back, and so does a ``machine.yml`` that + cannot be read. Either way the other skills are still planned. + """ cfg = config.active() include: list[dict] = [] skipped: list[dict] = [] @@ -177,7 +183,19 @@ def matrix_entries( for skill in skills: if not has_behavior_cases(skill, extended): continue - plan = datasets.machine_plan(skill) + try: + plan = datasets.machine_plan(skill) + except SystemExit as exc: + # One skill's broken machine.yml used to raise out of this loop + # and drop the matrix for every skill after it. It is named with + # the legs held back; the rest are still planned. + print(exc, file=sys.stderr) + skipped.append({ + "skill": skill, + "gate": "", + "error": str(exc).removeprefix("error: "), + }) + continue # Asking for a runner label is what makes a leg scoped: it is going on # hardware the repo singled out, which is also the hardware it rations # and pays for separately. @@ -266,5 +284,5 @@ def plan( "default": [leg for leg in include if "environment" not in leg], "scoped": [leg for leg in include if "environment" in leg], "skipped": skipped, - "gates": sorted({leg["gate"] for leg in skipped}), + "gates": sorted(leg["gate"] for leg in skipped if leg["gate"]), } diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 46f29d8..c07acd6 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -1133,6 +1133,19 @@ def test_hardware_with_no_environment_stays_in_one_matrix(self) -> None: json.loads(plan["default"][0]["runner"]), ["self-hosted", "gpu", "Linux"] ) + def test_one_bad_machine_yml_does_not_drop_the_matrix(self) -> None: + self.repo.skill("gamma", dataset=self.graded) + (self.repo.root / "gamma" / "evals" / "machine.yml").write_text( + "os: [\n", encoding="utf-8" + ) + with contextlib.redirect_stderr(io.StringIO()): + plan = select_module.plan(["alpha", "gamma"], routing=False, labels=set()) + self.assertIn("alpha", [leg["skill"] for leg in plan["default"]]) + [broken] = [item for item in plan["skipped"] if item["skill"] == "gamma"] + self.assertEqual(broken["gate"], "") + self.assertIn("not valid YAML", broken["error"]) + self.assertNotIn("", plan["gates"]) + class TestWhatABranchChanged(unittest.TestCase): """Selection is planned from the merge base, not from the base branch tip. @@ -6156,6 +6169,9 @@ def test_each_mistake_is_named(self) -> None: (_machine(_tool(env={"A=B": "x"})), "cannot be set in a process environment"), (_machine(_tool(env={"1A": "x"})), "cannot be set in a process environment"), (_machine(_tool(env={"A": "x\u0000y"})), "cannot be set in a process environment"), + # `$` matches before a trailing newline; the name is still illegal. + (_machine(_tool(env={"FOO\n": "x"})), "cannot be set in a process environment"), + (_machine(_tool(install=["cmd", ""])), "must be a non-empty list of strings"), # Two unknown keys of different types: sorting them must not crash. ("os: [Linux]\nprerequisites:\n - {name: t, check: [a], install: [b], 1: x, z: y}\n", "unknown key(s): 1, z"), @@ -6190,12 +6206,17 @@ def test_a_per_platform_command_is_still_an_argv(self) -> None: self.assertTrue( any("must be a non-empty list of strings" in e for e in errors), errors) - def test_an_os_label_with_no_command_platform_is_not_a_gap(self) -> None: - # A runner label like `Plan9` is the workflow's business; there is no - # command key for it to be missing. + def test_an_unrecognised_os_label_is_not_a_silent_skip(self) -> None: + # `Liunx` used to be skipped like a custom runner label, so the + # command-for-every-platform check never saw it. linux_only = _tool(check={"linux": ["c"]}, install={"linux": ["i"]}) - machine = json.dumps({"os": ["Linux", "Plan9"], "prerequisites": [linux_only]}) - self.assertEqual(self.errors_for(machine), []) + machine = json.dumps({"os": ["Linux", "Liunx"], "prerequisites": [linux_only]}) + errors = self.errors_for(machine) + self.assertTrue(any("Liunx" in e for e in errors), errors) + + def test_an_os_label_without_prerequisites_stays_a_runner_label(self) -> None: + # No command to check, so a label the workflow invented is its business. + self.assertEqual(self.errors_for(json.dumps({"os": ["Plan9"]})), []) def test_a_prerequisite_may_not_override_the_inputs(self) -> None: for name in datasets.PREREQUISITE_INPUTS: @@ -6550,6 +6571,8 @@ def test_the_watchdogs_hard_exit_kills_a_running_install_and_its_children(self) time.sleep(0.1) [step] = list(engine_prerequisites._live_steps) child = int(pids_file.read_text()) + # The flag stays set: the process is about to exit. This one is not. + self.addCleanup(setattr, engine_prerequisites, "_steps_closed", False) engine_prerequisites._on_expire() worker.join(timeout=10) for pid in (step, child): @@ -6560,6 +6583,19 @@ def test_the_watchdogs_hard_exit_kills_a_running_install_and_its_children(self) self.assertFalse(_alive(pid), f"process {pid} outlived the hard exit") self.assertEqual(engine_prerequisites._live_steps, set()) + def test_a_step_that_appears_after_the_snapshot_is_killed_too(self) -> None: + # The pid is known only once the process exists. If the watchdog has + # already snapped the set, the spawner has to kill it: os._exit will not. + self.addCleanup(setattr, engine_prerequisites, "_steps_closed", False) + engine_prerequisites._steps_closed = True + run = engine_prerequisites._host_runner(self.scratch) + code, _ = asyncio.run(asyncio.wait_for( + run([_PY, "-c", "import time; time.sleep(30)"], {}), + 5, + )) + self.assertNotEqual(code, 0) + self.assertEqual(engine_prerequisites._live_steps, set()) + def test_steps_run_beside_machine_yml(self) -> None: # The documented form: a script named by its path relative to the file. folder = self.repo.skill("scripted", dataset=tier0_dataset("scripted"), @@ -6705,6 +6741,7 @@ def test_the_watchdog_takes_the_host_caches_with_it(self) -> None: engine_prerequisites.on_host("needs-tool") as env: cache = Path(env["PREREQ_TOOL"]).parent self.assertIn(str(cache), engine_prerequisites._created_caches) + self.addCleanup(setattr, engine_prerequisites, "_steps_closed", False) engine_prerequisites._on_expire() self.assertFalse(cache.exists()) @@ -7069,6 +7106,32 @@ async def solve(state, generate): self.assertFalse(results["prerequisite"].degraded) self.assertTrue(results["outage"].degraded) # the heuristic still works + def test_another_name_for_the_answer_key_is_still_refused(self) -> None: + # `resolve()` keeps the case you typed, so path equality misses + # `EVALS.JSON` on a case-insensitive volume. A hard link is the same + # situation on every filesystem: two paths, one file. + folder = self.repo.skill( + "boxed", dataset=tier0_dataset("boxed"), + machine=_machine(_tool(install=["cat", "evals-link.json"])), + ) + os.link(folder / "evals" / "evals.json", folder / "evals" / "evals-link.json") + with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: + engine_prerequisites._named_files( + "boxed", datasets.machine_prerequisites("boxed") + ) + self.assertIn("answer key", str(raised.exception)) + + def test_a_file_outside_evals_is_not_copied(self) -> None: + folder = self.repo.skill( + "boxed", dataset=tier0_dataset("boxed"), + machine=_machine(_tool(install=["sh", "../secret.txt"])), + ) + (folder / "secret.txt").write_text("nope\n", encoding="utf-8") + named = engine_prerequisites._named_files( + "boxed", datasets.machine_prerequisites("boxed") + ) + self.assertEqual(named, {}) + def test_a_step_naming_the_answer_key_is_refused(self) -> None: with self.assertRaises(engine_prerequisites.PrerequisiteError) as raised: self.run_solver(_machine(_tool(install=["cat", "evals.json"])))