From 1d45806f5d42da5fc91dfcc523ba32db98425074 Mon Sep 17 00:00:00 2001 From: Andrei Petraru Date: Thu, 10 Sep 2026 16:31:57 +0300 Subject: [PATCH] fix(sandbox): give the sandbox venv system site packages `SandboxConfig.python` defaults to a `PythonEnvConfig()` instance, so `setup()` builds a venv for every task and installs nothing into it unless `env_packages` is set. That empty venv is not neutral. It goes on the criterion PATH via `_build_run_command_env`, so inside a task image that provisions packages globally the toolchain splits: which pip -> /usr/local/bin/pip (image global; `uv venv` seeds none in the venv) pip list -> langchain 1.3.14 (present!) python -c "import langchain" -> ModuleNotFoundError Measured cost, run 2026-09-10_04-18-49, `skill-agent-guardrail-coded-escalation-smoke`: the agent finished the guardrail at turn 19, spent turns 22-31 chasing that contradiction, repaired it with `uv sync`, then hit `max_turns: 40` at turn 41 before writing the app resource into bindings.json. Scored 0.80 with correct code. Create the venv with system site packages instead of removing it. The image's globals stay importable, `python` and `pip` agree, and installs still land in the venv (`sys.prefix` remains the sandbox), so a task's `env_packages` cannot leak into the image. Verified in a task image: `import pydantic` exits 1 on main and 0 here, with `sys.prefix` still the sandbox venv. Verified on the host that a criterion still cannot `import coder_eval`, so the grader's own environment does not leak in either -- `uv venv` bases on the system interpreter, not the active one. An earlier revision of this branch fixed the same defect by not creating the venv at all. Review found that cure wider than the disease: it desynchronized `setup` from `Sandbox.adopt` (which gates on `config.python` alone), so one trajectory could score differently under `run` vs `evaluate --in-place`, and it dropped the harness's `python`-on-PATH guarantee that 15 in-tree `run_command` criteria rely on. Keeping the venv keeps both gates identical and the guarantee intact. Also from that review: - The stdlib fallback produces a different artifact (it seeds pip) with no log and no coverage; it now warns and carries the same system-site-packages setting. - Tests assert the effect rather than the artifact: `pyvenv.cfg` carries `include-system-site-packages`, and `_build_run_command_env()` carries VIRTUAL_ENV plus the venv bin prefix. - `test_template_ignores_venv`'s only `.venv` assertion sat behind `if venv_bin.exists():` and could go vacuous; it is now unconditional, via a marker file the real venv can never contain. The marker is also what keeps it portable -- asserting on `.venv/bin/python` holds only on POSIX, since a real venv puts its interpreter in `Scripts/python.exe` on Windows. - `python: null` is documented user-facing behavior whose adopt arm was unexercised across the whole suite; it now has a test. - New live docker test pins the motivating scenario end to end, which nothing in CI asserted before. Co-Authored-By: Claude Opus 5 (1M context) --- docs/TASK_DEFINITION_GUIDE.md | 2 + src/coder_eval/models/sandbox.py | 7 +- src/coder_eval/sandbox.py | 37 +++++++++-- tests/test_sandbox.py | 57 +++++++++++++++- tests/test_sandbox_adopt.py | 22 +++++++ tests/test_sandbox_templates.py | 17 +++-- tests/test_sandbox_venv_live.py | 110 +++++++++++++++++++++++++++++++ 7 files changed, 239 insertions(+), 13 deletions(-) create mode 100644 tests/test_sandbox_venv_live.py diff --git a/docs/TASK_DEFINITION_GUIDE.md b/docs/TASK_DEFINITION_GUIDE.md index c575f28b..845f98d4 100644 --- a/docs/TASK_DEFINITION_GUIDE.md +++ b/docs/TASK_DEFINITION_GUIDE.md @@ -518,6 +518,8 @@ compares a truncated run against a full one): The `sandbox` block is optional. When omitted, it defaults to `driver: "tempdir"` with standard Python environment. +The sandbox venv is created with **system site packages**, so `run_command` criteria (and `pre_run`/`post_run`) can import packages the task image installed globally while anything `env_packages` adds still lands in the venv. An isolated venv would shadow the image's interpreter without providing a replacement. + ```yaml sandbox: driver: "tempdir" # Sandbox type ("tempdir" or "docker"); default: "tempdir" diff --git a/src/coder_eval/models/sandbox.py b/src/coder_eval/models/sandbox.py index 2e7f2dbc..b3f3d569 100644 --- a/src/coder_eval/models/sandbox.py +++ b/src/coder_eval/models/sandbox.py @@ -597,7 +597,12 @@ class SandboxConfig(BaseModel): ) python: PythonEnvConfig | None = Field( default_factory=PythonEnvConfig, - description="Python environment config; set to null in YAML (or None in Python) to skip venv creation", + description=( + "Python environment config. A venv is created whenever this block is present, with access to " + "system site packages so criteria can import what the task image installed globally (an isolated " + "venv shadows the interpreter while providing nothing). Set to null in YAML (or None in Python) " + "to skip venv creation, and to opt out of adopting a venv the agent created itself." + ), ) node: NodeEnvConfig | None = Field( default=None, diff --git a/src/coder_eval/sandbox.py b/src/coder_eval/sandbox.py index 8de18944..e9527713 100644 --- a/src/coder_eval/sandbox.py +++ b/src/coder_eval/sandbox.py @@ -406,7 +406,9 @@ def _setup_tempdir(self, target_dir: Path | None = None) -> Path: # Mark mock binaries executable so the agent's PATH can shadow real CLIs self._prepare_mock_path_dirs() - # Set up Python virtual environment (only if python config is provided) + # Set up Python virtual environment (only if python config is provided). + # The venv is created with system site packages -- see _setup_virtualenv + # for why an isolated one was actively harmful. if self.config.python: self._setup_virtualenv() @@ -835,7 +837,29 @@ def _matches_template_include_pattern(self, rel_path: Path, include_patterns: li return False def _setup_virtualenv(self) -> None: - """Create a Python virtual environment in the sandbox.""" + """Create a Python virtual environment in the sandbox, with system site packages. + + ``--system-site-packages`` is load-bearing, not a convenience. An ISOLATED + venv here shadows the interpreter while providing nothing: the sandbox venv + goes on the criterion PATH (``_build_run_command_env``, which governs every + ``run_command`` criterion plus ``pre_run``/``post_run``), so inside a task + image that provisions packages globally, ``python`` resolved to the empty + venv and could not import them while ``pip`` -- which ``uv venv`` does not + place in the venv at all -- fell through to the image's global pip and + reported them present. Measured in a task image: ``import langchain`` raised + ``ModuleNotFoundError`` while ``pip list`` showed ``langchain 1.3.14``. An + agent that tried to verify its own work chased that contradiction for ten + turns and ran out of budget before finishing. + + Note the venv is NOT on the agent's own PATH -- the orchestrator prepends + only ``resolved_mock_path_dirs`` there -- so the contradiction above is a + property of criterion and pre/post-run subprocesses. + + System site packages fixes it in the direction that keeps both halves: the + image's globals stay importable, ``python`` and ``pip`` agree, and installs + still land in the venv (``sys.prefix`` remains the sandbox), so a task's + ``env_packages`` cannot leak into the image. + """ if not self.sandbox_dir: raise RuntimeError("Sandbox directory not initialized") @@ -846,13 +870,16 @@ def _setup_virtualenv(self) -> None: # Check if uv is available subprocess.run(["uv", "--version"], check=True, capture_output=True, timeout=5) # Use uv to create venv - cmd = ["uv", "venv", str(self.venv_dir)] + cmd = ["uv", "venv", "--system-site-packages", str(self.venv_dir)] subprocess.run(cmd, check=True, capture_output=True, text=True, encoding="utf-8", timeout=60) except (subprocess.CalledProcessError, FileNotFoundError): - # Fallback to standard venv if uv is not available + # Fallback to standard venv if uv is not available. The two paths do not + # produce the same artifact -- this one seeds pip, `uv venv` does not -- + # so say which shape this host got rather than leaving it to be inferred. import venv - venv.create(self.venv_dir, with_pip=True) + logger.warning("uv unavailable; created %s with stdlib venv (pip seeded)", self.venv_dir) + venv.create(self.venv_dir, with_pip=True, system_site_packages=True) def _install_packages(self) -> None: """Install required Python packages in the virtual environment.""" diff --git a/tests/test_sandbox.py b/tests/test_sandbox.py index 1ae4e451..f1f2175f 100644 --- a/tests/test_sandbox.py +++ b/tests/test_sandbox.py @@ -486,7 +486,12 @@ def test_sandbox_with_packages(): sandbox = Sandbox(config, task_id="test_packages") try: - sandbox.setup() + sandbox_dir = sandbox.setup() + + # Asking for packages is what earns a venv (see + # test_default_python_config_creates_no_venv_when_nothing_to_install). + assert (sandbox_dir / ".venv").exists() + assert sandbox.venv_dir == sandbox_dir / ".venv" # Test that requests is installed exit_code, stdout, stderr = sandbox.run_command('python -c "import requests; print(requests.__version__)"') @@ -1310,3 +1315,53 @@ def test_absent_when_the_task_declares_no_reference(self, tmp_path): assert "REFERENCE_DIR" not in sb._build_run_command_env() finally: sb.cleanup(preserve=False) + + +def test_default_venv_can_import_system_site_packages(): + """The sandbox venv must not shadow the interpreter it is layered over. + + `SandboxConfig.python` defaults to a `PythonEnvConfig()` instance, so every + task gets a venv. Built ISOLATED, that venv split the toolchain inside a task + image that provisions packages globally: `python` resolved to the venv and + could not import them, while `pip` -- which `uv venv` never places in the venv + -- fell through to the image's global pip and reported them present. Measured + in a task image: `import langchain` raised ModuleNotFoundError while + `pip list` showed `langchain 1.3.14`. + + Asserted through `pyvenv.cfg` rather than a live import so the test is + hermetic: it holds on a host whose base interpreter has nothing installed. + """ + config = SandboxConfig(driver="tempdir") + assert config.python is not None, "default is an instance, not None -- the case this guards" + assert config.python.env_packages == [] + + sandbox = Sandbox(config, task_id="test_system_site_packages") + try: + sandbox_dir = sandbox.setup() + cfg = (sandbox_dir / ".venv" / "pyvenv.cfg").read_text(encoding="utf-8") + assert "include-system-site-packages = true" in cfg.lower() + finally: + sandbox.cleanup() + + +def test_venv_reaches_the_criterion_environment(): + """The venv is only worth creating because criteria run under it. + + `_build_run_command_env` is the single surface that carries it (its sole + caller is `Sandbox.run_command`, i.e. every `run_command` criterion plus + `pre_run`/`post_run`). The agent's own PATH never carries the venv -- the + orchestrator prepends only `resolved_mock_path_dirs` there -- so asserting + the artifact exists says nothing about the effect this change exists for. + """ + sandbox = Sandbox(SandboxConfig(driver="tempdir"), task_id="test_criterion_env") + try: + sandbox_dir = sandbox.setup() + venv_dir = sandbox_dir / ".venv" + assert sandbox.venv_dir == venv_dir + + env = sandbox._build_run_command_env() + assert env["VIRTUAL_ENV"] == str(venv_dir) + scripts_dir = "Scripts" if os.name == "nt" else "bin" + assert env["PATH"].startswith(f"{venv_dir / scripts_dir}{os.pathsep}") + finally: + sandbox.cleanup() diff --git a/tests/test_sandbox_adopt.py b/tests/test_sandbox_adopt.py index 39fafd9b..923aafb2 100644 --- a/tests/test_sandbox_adopt.py +++ b/tests/test_sandbox_adopt.py @@ -88,6 +88,28 @@ def test_adopt_leaves_venv_unset_when_there_is_none(tmp_path: Path) -> None: assert sandbox.venv_dir is None +def test_adopt_ignores_a_venv_when_python_is_null(tmp_path: Path) -> None: + """`python: null` opts out of BOTH halves: setup creates no venv, and adopt + declines to pick up one the agent wrote itself. + + Without this, the false arm of adopt's gate was unexercised across the whole + suite while the field description documents it as user-facing behavior. The + assertion is on the criterion environment, not just `venv_dir`, because that + is the surface the opt-out exists to control. + """ + ws = _workspace(tmp_path) + (ws / ".venv" / "bin").mkdir(parents=True) + sandbox = _sandbox(python=None) + sandbox.adopt(ws) + assert sandbox.venv_dir is None + # `_build_run_command_env` starts from `os.environ.copy()`, so an ambient + # VIRTUAL_ENV from the grader's own shell can be present; what must not + # happen is the harness pointing either variable at the WORKSPACE venv. + env = sandbox._build_run_command_env() + assert env.get("VIRTUAL_ENV") != str(ws.resolve() / ".venv") + assert str(ws.resolve() / ".venv") not in env["PATH"] + + def test_adopt_rejects_the_docker_driver(tmp_path: Path) -> None: """A container workspace is not reachable from the host, so adopting one would silently grade whatever happens to sit at that host path.""" diff --git a/tests/test_sandbox_templates.py b/tests/test_sandbox_templates.py index 12179300..6c737292 100644 --- a/tests/test_sandbox_templates.py +++ b/tests/test_sandbox_templates.py @@ -164,6 +164,7 @@ def test_template_ignores_venv(self, tmp_path): (template_dir / ".venv").mkdir() (template_dir / ".venv" / "bin").mkdir() (template_dir / ".venv" / "bin" / "python").write_text("fake python") + (template_dir / ".venv" / "from_template.marker").write_text("copied") config = SandboxConfig( driver="tempdir", @@ -179,12 +180,16 @@ def test_template_ignores_venv(self, tmp_path): # Verify main.py copied assert (sandbox_path / "main.py").exists() - # Verify .venv from template was NOT copied - # (sandbox creates its own .venv) - venv_bin = sandbox_path / ".venv" / "bin" - if venv_bin.exists(): - # If .venv exists, it should be the sandbox's venv, not the template's - assert not (venv_bin / "python").exists() or (venv_bin / "python").is_symlink() + # The .venv present here is the sandbox's own (setup creates one); + # assert the TEMPLATE's copy did not land, via a marker file the real + # venv can never contain. Unconditional on purpose: guarding this + # behind `if venv_bin.exists()` made the whole check vacuous the + # moment provisioning changed, which is exactly how it went + # unnoticed. The marker is also layout-independent -- asserting on + # `.venv/bin/python` would only work on POSIX, since a real venv puts + # its interpreter in `Scripts/python.exe` on Windows. + assert (sandbox_path / ".venv").exists() + assert not (sandbox_path / ".venv" / "from_template.marker").exists() finally: sandbox.cleanup(preserve=False) diff --git a/tests/test_sandbox_venv_live.py b/tests/test_sandbox_venv_live.py new file mode 100644 index 00000000..fd80ca8e --- /dev/null +++ b/tests/test_sandbox_venv_live.py @@ -0,0 +1,110 @@ +"""Live docker check that the sandbox venv does not shadow a task image's packages. + +Gated: needs a real docker daemon and the `coder-eval-agent` base image. + +This is the one thing the unit tests cannot prove. `tests/test_sandbox.py` asserts +the venv is created with system site packages by reading `pyvenv.cfg`, which is a +property of the flag, not of the outcome. The outcome only exists inside an image +that provisions packages GLOBALLY — the shape every task image has (the framework +image installs with `uv pip install --system`; skillsbench task images do +`RUN pip install ...`). There, an isolated sandbox venv split the toolchain: +`python` resolved to the venv and could not import the image's packages, while +`pip` fell through to the image's global pip and reported them present. + +Measured against this test's own scenario: + + main (isolated venv) python -c "import pydantic" -> exit 1 + with --system-site-packages python -c "import pydantic" -> exit 0 + +`pydantic` is a coder_eval runtime dependency, so the base image already has it +installed globally — no build and no network are needed to reproduce the shape. +""" + +from __future__ import annotations + +import shutil +import subprocess +import sys +import textwrap +from pathlib import Path + +import pytest + + +BASE_IMAGE = "coder-eval-agent:latest" +REPO_SRC = Path(__file__).resolve().parent.parent / "src" + +pytestmark = [ + pytest.mark.live, + pytest.mark.skipif(sys.platform == "win32", reason="docker driver is POSIX-only"), + pytest.mark.skipif(shutil.which("docker") is None, reason="docker CLI not available"), +] + + +def _docker_daemon_up() -> bool: + try: + return subprocess.run(["docker", "info"], capture_output=True, timeout=15).returncode == 0 + except (subprocess.TimeoutExpired, FileNotFoundError): + return False + + +def _image_present(image: str) -> bool: + return subprocess.run(["docker", "image", "inspect", image], capture_output=True, timeout=30).returncode == 0 + + +PROBE = textwrap.dedent( + """ + from coder_eval.models import SandboxConfig + from coder_eval.sandbox import Sandbox + + sandbox = Sandbox(SandboxConfig(driver="tempdir"), task_id="venv_probe") + try: + sandbox_dir = sandbox.setup() + import_rc, _, _ = sandbox.run_command('python -c "import pydantic"') + _, prefix, _ = sandbox.run_command('python -c "import sys; print(sys.prefix)"') + print(f"IMPORT_RC={import_rc}") + print(f"PREFIX={prefix.strip()}") + print(f"VENV={sandbox.venv_dir}") + finally: + sandbox.cleanup() + """ +) + + +def test_criteria_can_import_the_images_global_packages() -> None: + """A `run_command` criterion must see what the task image installed globally. + + Mounts this checkout's `src/` over the image's copy so the assertion is about + the code under test, not whatever coder_eval version the image was built with. + """ + if not _docker_daemon_up(): + pytest.skip("docker daemon not running") + if not _image_present(BASE_IMAGE): + pytest.skip(f"{BASE_IMAGE} not built locally") + + proc = subprocess.run( + [ + "docker", + "run", + "--rm", + "-v", + f"{REPO_SRC}:/opt/coder_eval/src:ro", + "--entrypoint", + "python3", + BASE_IMAGE, + "-c", + PROBE, + ], + capture_output=True, + text=True, + timeout=300, + ) + assert proc.returncode == 0, f"probe failed:\n{proc.stdout}\n{proc.stderr}" + out = dict(line.split("=", 1) for line in proc.stdout.splitlines() if "=" in line) + + assert out["IMPORT_RC"] == "0", ( + "a criterion could not import a package the image installed globally -- " + f"the sandbox venv is shadowing the image interpreter again:\n{proc.stdout}" + ) + # Isolation still holds: installs land in the sandbox, not the image. + assert out["PREFIX"] == out["VENV"], f"criterion did not run under the sandbox venv:\n{proc.stdout}"