diff --git a/src/benchflow/agents/install.py b/src/benchflow/agents/install.py index a65d8d6df..62cd56f40 100644 --- a/src/benchflow/agents/install.py +++ b/src/benchflow/agents/install.py @@ -365,3 +365,49 @@ async def deploy_skills( label = agent_cfg.name if agent_cfg else "oracle" if count: logger.info(f"Skills distributed to {count} paths for {label}") + + +async def assert_no_skill_isolation(env, agent_cfg, sandbox_user: str | None) -> None: + """Fail a no-skill rollout that can still reach skills. + + The no-skill condition is the primary capability signal for skill + benchmarks, and until now nothing verified it: benchflow could resolve a + rollout as no-skill, correctly skip every deployment path, and still start + the agent in an image another rollout had baked skills into. The run then + reported a normal pass and no counter disagreed — ``total_skill_invocations`` + stays at 0 whether or not the agent read a SKILL.md off disk. + + Check the agent's own discovery paths plus the canonical mount point, and + raise rather than let a contaminated run be recorded as a result. + """ + home = f"/home/{sandbox_user}" if sandbox_user else "/root" + candidates = ["/skills"] + if agent_cfg is not None: + for skill_path in agent_cfg.skill_paths: + candidates.append(skill_path.replace("$HOME", home)) + + quoted = " ".join(shlex.quote(path) for path in dict.fromkeys(candidates)) + # -L so a symlinked discovery path is followed to the real tree. + probe = ( + f"for p in {quoted}; do " + 'if [ -e "$p" ]; then find -L "$p" -name SKILL.md 2>/dev/null; fi; ' + "done" + ) + result = await env.exec(probe, timeout_sec=20) + # Only absolute paths to a SKILL.md count. The probe's stdout can also carry + # shell noise, and a substring match would turn that into a false positive. + found = [ + line.strip() + for line in (result.stdout or "").splitlines() + if line.strip().startswith("/") and line.strip().endswith("/SKILL.md") + ] + if not found: + return + + raise RuntimeError( + "no-skill rollout is contaminated: skills are reachable inside the " + f"sandbox at {', '.join(found[:5])}" + + (f" (+{len(found) - 5} more)" if len(found) > 5 else "") + + ". The agent can read mentor guidance the condition is meant to " + "withhold, so the result would not measure no-skill capability." + ) diff --git a/src/benchflow/contracts/planes.py b/src/benchflow/contracts/planes.py index 7fd52c66a..a55919d5f 100644 --- a/src/benchflow/contracts/planes.py +++ b/src/benchflow/contracts/planes.py @@ -71,6 +71,10 @@ async def seed_verifier_workspace( self, env: Any, *, workspace: str, sandbox_user: str | None ) -> None: ... async def deploy_skills(self, *args: Any, **kwargs: Any) -> None: ... + + async def assert_no_skill_isolation( + self, *args: Any, **kwargs: Any + ) -> None: ... async def lockdown_paths(self, env: Any, locked_paths: list[str]) -> None: ... async def install_agent( self, diff --git a/src/benchflow/rollout/__init__.py b/src/benchflow/rollout/__init__.py index 7c7d1ed8d..b0d57c230 100644 --- a/src/benchflow/rollout/__init__.py +++ b/src/benchflow/rollout/__init__.py @@ -1150,6 +1150,12 @@ async def install_agent(self) -> None: self._agent_cwd, skills_sandbox_dir=self._effective_skills_sandbox_dir, ) + if cfg.recorded_skill_mode == SKILL_MODE_NO_SKILL: + # Verify the condition rather than assume it: a stale image can + # carry skills that no deployment path in this rollout put there. + await self._planes.assert_no_skill_isolation( + self._env, self._agent_cfg, cfg.sandbox_user + ) if cfg.export_generated_skills_to: await _ensure_sandbox_dir( self._env, cfg.generated_skills_root, cfg.sandbox_user diff --git a/src/benchflow/rollout_planes.py b/src/benchflow/rollout_planes.py index 6d2272d9a..9e1b8a91e 100644 --- a/src/benchflow/rollout_planes.py +++ b/src/benchflow/rollout_planes.py @@ -20,6 +20,7 @@ from benchflow.agents.install import ( _link_skill_paths, apply_web_tool_policy, + assert_no_skill_isolation, deploy_skills, install_agent, ) @@ -146,6 +147,9 @@ async def seed_verifier_workspace( async def deploy_skills(self, *args: Any, **kwargs: Any) -> None: await deploy_skills(*args, **kwargs) + async def assert_no_skill_isolation(self, *args: Any, **kwargs: Any) -> None: + await assert_no_skill_isolation(*args, **kwargs) + async def lockdown_paths(self, env: Any, locked_paths: list[str]) -> None: await lockdown_paths(env, locked_paths) diff --git a/src/benchflow/sandbox/docker.py b/src/benchflow/sandbox/docker.py index c14b18fa9..ca672b76c 100644 --- a/src/benchflow/sandbox/docker.py +++ b/src/benchflow/sandbox/docker.py @@ -8,6 +8,7 @@ import asyncio import asyncio.subprocess import contextlib +import hashlib import json import logging import os @@ -69,6 +70,36 @@ def _sanitize_docker_image_name(name: str) -> str: return name +def _build_context_fingerprint(environment_dir: Path) -> str: + """Short digest of the inputs that decide what the built image contains. + + The image tag must distinguish builds whose contents differ. The Dockerfile + is rewritten in place when skills are baked in (``--skills-dir`` appends + ``COPY _deps/skills`` plus symlinks into every agent skill path), so a tag + derived from the task name alone is shared by a with-skill and a no-skill + build of the same task. Whichever built last wins, and the other rollout + starts from it — a no-skill rollout then runs in an image that already + contains ``/skills`` and the agent-side symlinks, silently invalidating the + no-skill condition. + + Hashing the Dockerfile, and the presence of the staged skills payload, + gives the two builds different tags. + """ + hasher = hashlib.sha256() + dockerfile = environment_dir / "Dockerfile" + if dockerfile.is_file(): + hasher.update(dockerfile.read_bytes()) + # The staged payload is what COPY pulls in; include its layout so a change + # of skills — not just of the Dockerfile line — yields a different tag. + skills_root = environment_dir / "_deps" / "skills" + if skills_root.is_dir(): + for path in sorted(skills_root.rglob("*")): + hasher.update(str(path.relative_to(skills_root)).encode()) + if path.is_file(): + hasher.update(str(path.stat().st_size).encode()) + return hasher.hexdigest()[:12] + + def _sanitize_docker_compose_project_name(name: str) -> str: name = name.lower() if not re.match(r"^[a-z0-9]", name): @@ -191,7 +222,10 @@ def __init__( ) self._env_vars = DockerSandboxEnvVars( - main_image_name=_sanitize_docker_image_name(f"bf__{environment_name}"), + main_image_name=_sanitize_docker_image_name( + f"bf__{environment_name}__" + f"{_build_context_fingerprint(self.environment_dir)}" + ), context_dir=str(self.environment_dir.resolve().absolute()), host_verifier_logs_path=verifier_dir, host_agent_logs_path=agent_dir, diff --git a/tests/test_no_skill_image_isolation.py b/tests/test_no_skill_image_isolation.py new file mode 100644 index 000000000..d62b1f9b0 --- /dev/null +++ b/tests/test_no_skill_image_isolation.py @@ -0,0 +1,118 @@ +"""Regression tests for no-skill rollout isolation. + +A with-skill build rewrites the task Dockerfile in place (``COPY _deps/skills`` +plus symlinks into every agent skill path). While the image tag was derived from +the task name alone, that build and a no-skill build of the same task shared one +tag, so whichever ran last decided what both started from — and a no-skill +rollout could silently run inside an image containing ``/skills``. +""" + +from __future__ import annotations + +from typing import ClassVar + +import pytest + +from benchflow.agents.install import assert_no_skill_isolation +from benchflow.sandbox.docker import _build_context_fingerprint + + +def _write_env(tmp_path, dockerfile_body: str, skills: dict[str, str] | None = None): + env_dir = tmp_path / "environment" + env_dir.mkdir(parents=True, exist_ok=True) + (env_dir / "Dockerfile").write_text(dockerfile_body) + if skills: + for rel, content in skills.items(): + target = env_dir / "_deps" / "skills" / rel + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content) + return env_dir + + +BASE_DOCKERFILE = "FROM python:3.12-slim\nCOPY payload.stl /root/payload.stl\n" +SKILL_INJECTED = BASE_DOCKERFILE + ( + "\n# Skills directory (injected by benchflow --skills-dir)\n" + "COPY _deps/skills /skills/\n" + "RUN mkdir -p /home/agent/.opencode && ln -sf /skills /home/agent/.opencode/skills\n" +) + + +def test_fingerprint_is_stable_for_identical_context(tmp_path): + a = _write_env(tmp_path / "a", BASE_DOCKERFILE) + b = _write_env(tmp_path / "b", BASE_DOCKERFILE) + assert _build_context_fingerprint(a) == _build_context_fingerprint(b) + + +def test_skill_injection_changes_the_fingerprint(tmp_path): + """The core regression: the two builds must not share an image tag.""" + clean = _write_env(tmp_path / "clean", BASE_DOCKERFILE) + injected = _write_env( + tmp_path / "injected", + SKILL_INJECTED, + skills={"mentor/SKILL.md": "# mentor\n"}, + ) + assert _build_context_fingerprint(clean) != _build_context_fingerprint(injected) + + +def test_changing_skill_payload_changes_the_fingerprint(tmp_path): + one = _write_env( + tmp_path / "one", SKILL_INJECTED, skills={"mentor/SKILL.md": "# mentor\n"} + ) + two = _write_env( + tmp_path / "two", + SKILL_INJECTED, + skills={"mentor/SKILL.md": "# mentor\n", "second/SKILL.md": "# second\n"}, + ) + assert _build_context_fingerprint(one) != _build_context_fingerprint(two) + + +class _FakeEnv: + """Minimal env double returning canned `find` output.""" + + def __init__(self, stdout: str): + self._stdout = stdout + self.commands: list[str] = [] + + async def exec(self, cmd: str, timeout_sec: int = 20): + self.commands.append(cmd) + + class _Result: + stdout = self._stdout + stderr = "" + return_code = 0 + + return _Result() + + +class _FakeAgentCfg: + skill_paths: ClassVar[list[str]] = ["$HOME/.opencode/skills"] + + +@pytest.mark.asyncio +async def test_isolation_check_passes_when_no_skills_present(): + env = _FakeEnv("") + await assert_no_skill_isolation(env, _FakeAgentCfg(), "agent") + assert "/home/agent/.opencode/skills" in env.commands[0] + assert "/skills" in env.commands[0] + + +@pytest.mark.asyncio +async def test_isolation_check_raises_when_skills_are_reachable(): + env = _FakeEnv("/skills/ion-shuttling-mentor/SKILL.md\n") + with pytest.raises(RuntimeError, match="no-skill rollout is contaminated"): + await assert_no_skill_isolation(env, _FakeAgentCfg(), "agent") + + +@pytest.mark.asyncio +async def test_isolation_check_handles_missing_agent_config(): + """The oracle path passes no agent config; the mount point is still checked.""" + env = _FakeEnv("") + await assert_no_skill_isolation(env, None, None) + assert "/skills" in env.commands[0] + + +@pytest.mark.asyncio +async def test_isolation_check_ignores_non_skill_output(): + """Shell noise on stdout must not be mistaken for a contaminated sandbox.""" + env = _FakeEnv("/workspace\nsome unrelated line\n") + await assert_no_skill_isolation(env, _FakeAgentCfg(), "agent")