From 83eea1e25694f4432dadbef6e9c5133540fe5eb0 Mon Sep 17 00:00:00 2001 From: Akshaya Shanbhogue Date: Mon, 14 Sep 2026 17:08:09 -0700 Subject: [PATCH] feat(harbor): bind-mount task.yaml/plugins/templates/extra_mounts instead of COPY, skip Dockerfile when unneeded, translate pre_run Harbor task exports previously left agent.plugins[] (e.g. a skill plugin directory) and sandbox.docker.extra_mounts (e.g. UiPath CLI credentials) unavailable inside the container -- docker_runner.py auto-mounts these for a normal docker sandbox run, but the Harbor exporter had no equivalent, so an exported agent ran with no skill content and no credentials. - environment/docker-compose.yaml now bind-mounts environment/task.yaml itself, each `type: local` agent.plugins[] dir, each TemplateDirSource in sandbox.template_sources, and each sandbox.docker.extra_mounts entry (its own ro/rw mode preserved) -- all at their own host path, so nothing needs COPYing into the image or rewriting in task.yaml. Mount paths are always emitted POSIX-style (as_posix()) since docker-compose volume specs are POSIX regardless of the host OS running the exporter. - environment/Dockerfile is now written only when sandbox.docker.dockerfile_path is set (real RUN build steps needed); otherwise task.toml's [environment].docker_image points Harbor straight at the pre-built image via its own should_use_prebuilt_docker_image path, and no Dockerfile is written at all. - pre_run commands are now translated into the agent-phase task.yaml -- they run before the agent starts, which coder-eval execute still does for the CoderEvalAgent embed, so this was a fixable gap rather than a fundamental limitation (unlike post_run, which belongs to the grading phase execute never runs, and stays untranslated with a warning). Verified end-to-end against a real harbor install: exported and ran two tasks (uipath-admin audit smoke, uipath-agents antipattern_openai_agents_hitl) through `harbor run -a coder_eval.harbor.agent:CoderEvalAgent`, both scoring 1.0 after these fixes. Co-Authored-By: Claude Sonnet 5 --- src/coder_eval/harbor/agent_paths.py | 25 +- src/coder_eval/harbor/packager.py | 378 ++++++++++++------ .../expected/environment/Dockerfile | 2 - .../expected/environment/docker-compose.yaml | 4 + tests/test_harbor_export_golden.py | 34 +- tests/test_harbor_packager.py | 91 +++-- 6 files changed, 347 insertions(+), 187 deletions(-) create mode 100644 tests/_fixtures/harbor_export_golden/expected/environment/docker-compose.yaml diff --git a/src/coder_eval/harbor/agent_paths.py b/src/coder_eval/harbor/agent_paths.py index 22d53744..2d77c60f 100644 --- a/src/coder_eval/harbor/agent_paths.py +++ b/src/coder_eval/harbor/agent_paths.py @@ -1,14 +1,15 @@ -"""Fixed in-container path for the agent-phase task.yaml a Harbor export bakes into its image. +"""Fixed in-container path for the agent-phase task.yaml a Harbor export bind-mounts in. -Shared by ``packager.py`` (the writer — bakes a criteria-free copy of the task -into ``environment/task.yaml`` and ``COPY``s it here) and ``agent.py``'s -``CoderEvalAgent`` (the reader — the Harbor agent that runs +Shared by ``packager.py`` (the writer — bakes a criteria-free copy of the task into +``environment/task.yaml`` and bind-mounts it read-only here via +``environment/docker-compose.yaml``, see ``_write_docker_compose_mounts``) and +``agent.py``'s ``CoderEvalAgent`` (the reader — the Harbor agent that runs ``coder-eval execute --format harbor`` against this exact path), so the two sides can never independently drift on where the file lives. Fixed rather than discovered: a Harbor agent has no way to ask the export what path it chose, so the path itself is the contract (tmp/harborframework.md's "Gap 1" resolution — -the agent can always execute a task.yaml at a fixed path; it's up to the -Dockerfile to put it there). +the agent can always execute a task.yaml at a fixed path; it's up to +docker-compose.yaml to put it there). """ from __future__ import annotations @@ -16,14 +17,4 @@ AGENT_TASK_YAML_PATH = "/opt/coder-eval-task/task.yaml" -AGENT_TASK_TEMPLATES_DIR = "/opt/coder-eval-task/templates" -"""Sibling of :data:`AGENT_TASK_YAML_PATH` for ``TemplateDirSource`` copies. A -``TemplateDirSource.path`` is resolved to an absolute HOST path at task-load -time (``task_loader.resolve_template_source_paths``) — that path does not -exist inside the container, so ``packager.py`` copies each source's directory -under here (``environment/templates/-/`` on the export side, ``COPY`` -'d into the image at build time) and rewrites ``environment/task.yaml``'s -``template_sources[].path`` to point at the in-container copy instead. -""" - -__all__ = ["AGENT_TASK_TEMPLATES_DIR", "AGENT_TASK_YAML_PATH"] +__all__ = ["AGENT_TASK_YAML_PATH"] diff --git a/src/coder_eval/harbor/packager.py b/src/coder_eval/harbor/packager.py index 2624534b..239c4df8 100644 --- a/src/coder_eval/harbor/packager.py +++ b/src/coder_eval/harbor/packager.py @@ -12,11 +12,21 @@ ├── task.toml ├── instruction.md # fixed placeholder -- the real prompt is in environment/task.yaml ├── environment/ - │ ├── Dockerfile # copied from dockerfile_path, or synthesized from - │ │ # sandbox.docker.image -- always written, WORKDIR pinned - │ ├── task.yaml # criteria-free copy for the CoderEvalAgent embed - │ └── templates/ # sandbox.template_sources' TemplateDirSource dirs, - │ # present only when the task has any (see agent_paths.py) + │ ├── Dockerfile # ONLY when sandbox.docker.dockerfile_path is set (real + │ │ # RUN build steps needed) -- copied in, WORKDIR pinned. + │ │ # Otherwise absent entirely: task.toml's + │ │ # [environment].docker_image names sandbox.docker.image + │ │ # directly and Harbor skips building (see _write_environment) + │ ├── task.yaml # criteria-free copy for the CoderEvalAgent embed -- + │ │ # bind-mounted in, not COPY'd (see docker-compose.yaml) + │ └── docker-compose.yaml # ALWAYS written: read-only mount of task.yaml itself, + │ # plus one read-only bind mount per `type: local` + │ # agent.plugins[] entry and per TemplateDirSource in + │ # sandbox.template_sources, plus one bind mount (its own + │ # ro/rw mode kept) per sandbox.docker.extra_mounts entry -- + │ # same host path in and out (mirrors docker_runner.py's own + │ # auto-mount; see _write_docker_compose_mounts). Nothing is + │ # ever COPY'd into the image anymore. └── tests/ ├── test.sh # C1.1's two-line shim ├── task.yaml # the criteria, as authored @@ -44,6 +54,7 @@ from __future__ import annotations +import os import shlex import shutil import subprocess @@ -53,8 +64,9 @@ import tomli_w import yaml -from coder_eval.harbor.agent_paths import AGENT_TASK_TEMPLATES_DIR, AGENT_TASK_YAML_PATH +from coder_eval.harbor.agent_paths import AGENT_TASK_YAML_PATH from coder_eval.harbor.portability import PortabilityIssue, audit_criteria +from coder_eval.isolation.docker_runner import _validate_extra_mount from coder_eval.models import TaskDefinition, TemplateDirSource from coder_eval.orchestration.task_loader import load_task from coder_eval.path_utils import REFERENCE_COPY_IGNORE, ignore_patterns_and_symlinks @@ -194,12 +206,6 @@ def export_resolved_task( ) warnings: list[str] = [] - if task.pre_run: - warnings.append( - f"{len(task.pre_run)} pre_run command(s) were NOT translated — they run against a live sandbox " - + "with template files already staged, which has no Dockerfile-build-time equivalent. Fold their " - + "effect into environment/Dockerfile by hand if the exported task needs it." - ) if task.post_run: warnings.append( f"{len(task.post_run)} post_run command(s) were NOT translated — they run after the verdict is " @@ -210,12 +216,12 @@ def export_resolved_task( out_dir.mkdir(parents=True, exist_ok=True) (out_dir / "tests").mkdir(parents=True, exist_ok=True) - workdir = _write_environment(task, task_file, out_dir, warnings) + workdir, docker_image = _write_environment(task, task_file, out_dir, warnings) _write_instruction(out_dir) _write_verifier_task_yaml(task, out_dir) _write_test_sh(out_dir, workdir=workdir) _write_reference(task, task_file, out_dir) - _write_task_toml(task, out_dir, workdir=workdir) + _write_task_toml(task, out_dir, workdir=workdir, docker_image=docker_image) return ExportResult(out_dir=out_dir, workdir=workdir, warnings=warnings) @@ -268,35 +274,48 @@ def _write_environment( task_file: Path, out_dir: Path, warnings: list[str], -) -> str: - """Copy/derive ``environment/`` and return the WORKDIR both it and test.sh must agree on. - - Per C0 § 5, Harbor has no fixed workspace path — the verifier (default - SHARED mode) runs at whatever the container's own WORKDIR is. This - function is the one place that decides it, so nothing downstream can - silently disagree. - - A ``Dockerfile`` is ALWAYS written here (never left to a pre-built - ``docker_image`` reference in ``task.toml``) — the whole point of - ``environment/task.yaml`` is to be baked in at :data:`AGENT_TASK_YAML_PATH` - for the ``CoderEvalAgent`` Harbor-agent embed, and a task exported without - a Dockerfile has no `COPY` step to put it there. Two shapes, both ending - in the same `COPY task.yaml ...` line: - - - ``dockerfile_path`` set: copy the user's Dockerfile as the base, appending - a `WORKDIR` (if it declared none) and the `COPY` line. - - unset: synthesize a minimal one (`FROM ` + `WORKDIR` + - `COPY`) — ``docker_cfg.image`` always has a value (default_factory= - ``get_default_docker_image_tag``), so this is a real choice, not a - null-vs-set distinction. +) -> tuple[str, str | None]: + """Derive ``environment/`` and return ``(workdir, docker_image)``: + + - ``workdir``: the WORKDIR both it and test.sh must agree on. Per C0 § 5, + Harbor has no fixed workspace path — the verifier (default SHARED mode) + runs at whatever the container's own WORKDIR is. This function is the one + place that decides it, so nothing downstream can silently disagree. + - ``docker_image``: set only when no ``environment/Dockerfile`` was written at + all, so ``_write_task_toml`` can point ``task.toml``'s ``[environment].docker_image`` + straight at the pre-built image; ``None`` when a Dockerfile was written and + Harbor must build from it instead. + + Two shapes: + + - ``dockerfile_path`` set: the task needs real build steps (``RUN`` etc.) that + only a Dockerfile can express, so it's copied in as the base (appending a + `WORKDIR` if it declared none). Returns ``(workdir, None)``. + - unset: no Dockerfile is written at all. Harbor's own + ``should_use_prebuilt_docker_image`` (``harbor/environments/definition.py``) + already supports pulling ``task.toml``'s ``[environment].docker_image`` + directly and skipping the build step entirely (confirmed against a real + ``harbor`` install) — `WORKDIR` doesn't need a Dockerfile line either: + ``[environment].workdir`` is what Harbor's docker environment passes as the + ``cwd``/``-w`` at ``docker exec`` time (``docker.py``), independent of + whether the image was built or pulled prebuilt. Returns + ``(workdir, docker_cfg.image)`` — ``docker_cfg.image`` always has a value + (default_factory=``get_default_docker_image_tag``), so this is a real + choice, not a null-vs-set distinction. + + ``environment/task.yaml`` itself is bind-mounted, not ``COPY``'d, in at + :data:`AGENT_TASK_YAML_PATH` for the ``CoderEvalAgent`` Harbor-agent embed + to read (see ``_write_docker_compose_mounts``) — neither shape's Dockerfile + (when one exists at all) plays any part in getting it there. """ env_dir = out_dir / "environment" env_dir.mkdir(parents=True, exist_ok=True) docker_cfg = task.sandbox.docker - has_templates = _write_agent_phase_task_yaml( - task, env_dir, initial_prompt=_resolve_prompt_text(task, task_file), warnings=warnings - ) - templates_copy_line = f"COPY templates/ {AGENT_TASK_TEMPLATES_DIR}/\n" if has_templates else "" + _write_agent_phase_task_yaml(task, env_dir, initial_prompt=_resolve_prompt_text(task, task_file), warnings=warnings) + # docker-compose.yaml (bind mounts: task.yaml itself, agent.plugins[], template_sources' + # TemplateDirSource dirs, sandbox.docker.extra_mounts) is a separate concern from the + # Dockerfile -- nothing is ever COPY'd in anymore, see _write_docker_compose_mounts. + _write_docker_compose_mounts(task, docker_cfg, env_dir, warnings) dest_dockerfile = env_dir / "Dockerfile" if docker_cfg.dockerfile_path is not None: @@ -310,8 +329,6 @@ def _write_environment( f"environment/Dockerfile declared no WORKDIR; appended `WORKDIR {workdir}` so the " + "verifier and agent phases agree on a path." ) - with dest_dockerfile.open("a", encoding="utf-8") as fh: - fh.write(f"\nCOPY task.yaml {AGENT_TASK_YAML_PATH}\n{templates_copy_line}") if not _from_line_mentions_coder_eval_agent(dest_dockerfile): warnings.append(_MISSING_CODER_EVAL_WARNING) # Non-Dockerfile build context (COPY sources etc.) is not carried over in @@ -324,11 +341,10 @@ def _write_environment( f"{source_dockerfile.parent} holds {len(other_files)} other file(s) alongside the " + "Dockerfile (build context) that were NOT copied — v1 only copies the Dockerfile itself." ) - return workdir + return workdir, None - # No dockerfile_path — synthesize a minimal Dockerfile on top of the - # pre-built image so the `CoderEvalAgent` embed always has somewhere to - # `COPY task.yaml` into. + # No dockerfile_path -- no Dockerfile at all. task.toml's [environment].docker_image + # points Harbor straight at the pre-built image instead (see docstring). if "coder-eval-agent" not in docker_cfg.image: warnings.append(_MISSING_CODER_EVAL_WARNING) if docker_cfg.working_dir is not None: @@ -346,11 +362,7 @@ def _write_environment( + "`docker run -w`, it will not create it); set `sandbox.docker.working_dir` explicitly " + "to the image's real WORKDIR to avoid a verify-time exit 127." ) - dest_dockerfile.write_text( - f"FROM {docker_cfg.image}\nWORKDIR {workdir}\n\nCOPY task.yaml {AGENT_TASK_YAML_PATH}\n{templates_copy_line}", - encoding="utf-8", - ) - return workdir + return workdir, docker_cfg.image def _inspect_image_workdir(image: str) -> str | None: @@ -449,86 +461,178 @@ def _write_verifier_task_yaml(task: TaskDefinition, out_dir: Path) -> None: ) -def _copy_template_sources(task: TaskDefinition, env_dir: Path, warnings: list[str]) -> list[dict[str, object]] | None: - """Copy each ``TemplateDirSource``'s directory into ``environment/templates/-/`` - and return a rewritten ``template_sources`` list pointing at the in-container copy - (:data:`AGENT_TASK_TEMPLATES_DIR`), or ``None`` if the task has no template sources. +def _template_volume_specs(task: TaskDefinition, warnings: list[str]) -> list[str]: + """Return one ``src:src:ro`` compose volume spec per ``TemplateDirSource`` in + ``sandbox.template_sources``, or ``[]`` if there are none. ``TemplateDirSource.path`` is resolved to an absolute HOST path at task-load time - (``task_loader.resolve_template_source_paths``) -- baking that path verbatim into - ``environment/task.yaml`` would point the in-container agent at a directory that - doesn't exist there. Only ``TemplateDirSource`` is copied: ``RepoSource`` (clones at - runtime) and ``StarterFilesSource`` (inline file content) resolve entirely inside the - container already and are carried over unchanged. + (``task_loader.resolve_template_source_paths``) — mounted at that same host path, + exactly like ``_plugin_volume_specs``, so ``environment/task.yaml``'s + ``sandbox.template_sources[].path`` needs no rewriting: ``task.sandbox.model_dump()`` + already carries the correct absolute path verbatim. + + Read-only is safe here: ``sandbox.py``'s ``_apply_template_dir_source`` only ever + reads from this path (it copies FROM here into the sandbox workdir at setup time), + never writes to it. + + Only ``TemplateDirSource`` is mounted — ``RepoSource`` (clones at runtime) and + ``StarterFilesSource`` (inline file content) resolve entirely inside the container + already and are carried over unchanged (nothing to mount; ``task.sandbox.model_dump()`` + keeps them verbatim). """ - sources = task.sandbox.template_sources - if not sources: - return None - templates_dir = env_dir / "templates" - rewritten: list[dict[str, object]] = [] + sources = task.sandbox.template_sources or [] + specs: list[str] = [] for i, source in enumerate(sources): - dumped = source.model_dump(mode="json", exclude_none=True) - if isinstance(source, TemplateDirSource): - source_path = Path(source.path) # already absolute — load_task resolved it - dest_name = f"{i:02d}-{source_path.name}" - dest = templates_dir / dest_name - if not source_path.is_dir(): - # A hard failure, not a warning: the agent-phase task.yaml - # still references this template (starter files, or a pytest - # suite the prompt expects), so a silently-skipped copy ships - # an export whose agent has no starter code -- every criterion - # then reads "file does not exist" indistinguishable from a - # real agent failure (the exact CE039 anti-pattern, one layer - # up at the export boundary instead of the grading boundary). - raise TaskNotExportableError( - f"template_sources[{i}].path {source_path} is not a directory -- cannot copy it into the " - + "export. Fix the task's template_sources entry before exporting." - ) - if dest.exists(): - shutil.rmtree(dest) - # Symlinks dereferenced by default (`shutil.copytree`'s default - # `symlinks=False`) would write a symlink TARGET's content into the - # distributable export -- e.g. a `creds -> /root/.aws/credentials` - # plant. Drop symlinks outright rather than following them, same - # rule as the sibling reference copy below and every other - # task-authored-tree copy in `src/` (`orchestration/evaluation.py`, - # `isolation/docker_runner.py`, `evaluation/sub_agent.py`). - shutil.copytree(source_path, dest, ignore=ignore_patterns_and_symlinks(REFERENCE_COPY_IGNORE)) - dumped["path"] = f"{AGENT_TASK_TEMPLATES_DIR}/{dest_name}" - else: + if not isinstance(source, TemplateDirSource): warnings.append( f"template_sources[{i}] ({type(source).__name__}) is not a TemplateDirSource -- carried over " - + "unchanged; only TemplateDirSource directories are copied into the export." + + "unchanged; only TemplateDirSource directories are mounted into the export." + ) + continue + source_path = Path(source.path) # already absolute — load_task resolved it + if not source_path.is_dir(): + # A hard failure, not a warning: the agent-phase task.yaml still + # references this template (starter files, or a pytest suite the + # prompt expects), so a silently-skipped mount ships an export whose + # agent has no starter code -- every criterion then reads "file does + # not exist" indistinguishable from a real agent failure (the exact + # CE039 anti-pattern, one layer up at the export boundary instead of + # the grading boundary). + raise TaskNotExportableError( + f"template_sources[{i}].path {source_path} is not a directory -- cannot mount it into the " + + "export. Fix the task's template_sources entry before exporting." ) - rewritten.append(dumped) - return rewritten + mount_path = source_path.as_posix() + specs.append(f"{mount_path}:{mount_path}:ro") + return specs + + +def _plugin_volume_specs(task: TaskDefinition) -> list[str]: + """Return one ``src:src:ro`` compose volume spec per ``type: local`` ``agent.plugins[]``. + + Mounted at its own host path, unmodified — exactly mirroring ``docker_runner.py``'s + own auto-mount for non-Harbor runs (``-v {target}:{target}:ro``) — so + ``environment/task.yaml``'s ``agent.plugins[].path`` needs no rewriting: the path is + identical inside and outside the container. This also means a bind mount never + touches this export's own output directory (unlike an earlier ``COPY``-based + approach, which had to guard against the plugin source containing the export's + ``-o`` directory) — there's nothing to walk or copy, so no self-nesting hazard here. + + Forced read-only (``:ro``), unconditionally: this mounts the skill/plugin content + an agent reads, never writable state, and the same host path may be mounted into + unrelated concurrent containers. + + Unlike ``TemplateDirSource.path`` (already resolved to an absolute host path by + ``load_task``), a plugin's ``path`` is carried unexpanded (e.g. literal + ``"$SKILLS_REPO_PATH"``) — ``docker_runner.py``'s own auto-mount expands it the same + way (``os.path.expandvars`` + ``os.path.expanduser``) at container-launch time, so this + mirrors that rather than requiring the export-time environment to already have it resolved. + """ + plugins = (task.agent.plugins if task.agent is not None else None) or [] + local_plugins = [p for p in plugins if isinstance(p, dict) and p.get("type") == "local"] + specs: list[str] = [] + for i, plugin in enumerate(local_plugins): + raw_path = plugin.get("path") + source_path = Path(os.path.expandvars(os.path.expanduser(raw_path or ""))).resolve() + if not source_path.is_dir(): + # Hard failure, not a warning — same rationale as the template-source guard + # below: the agent-phase task.yaml still references this plugin for skill + # discovery, so a silently-skipped mount ships an agent with no skill content + # and no error, indistinguishable from a real "the skill didn't trigger" defect. + raise TaskNotExportableError( + f"agent.plugins[{i}].path {raw_path!r} (resolved to {source_path}) is not a directory -- " + + "cannot mount it into the export. Fix the task's (or experiment's) agent.plugins entry, " + + "or unset SKILLS_REPO_PATH/whichever env var it references, before exporting." + ) + mount_path = source_path.as_posix() + specs.append(f"{mount_path}:{mount_path}:ro") + return specs + + +def _extra_mount_volume_specs(docker_cfg: object) -> list[str]: + """Return one ``src:dst:mode`` compose volume spec per ``sandbox.docker.extra_mounts`` entry. + + Reuses ``docker_runner._validate_extra_mount`` — the exact same validation + ``docker run -v`` gets for a non-Harbor run (var/`~` expansion, mode required and + checked, destination collision with framework-reserved paths rejected, source must + exist on the host at export time) — so an ``extra_mounts`` entry behaves identically + whether the task runs through the ordinary docker sandbox or through this export. + Unlike plugin mounts, an author-specified mode (``ro`` or ``rw``) is kept as-is + rather than forced. + """ + raw_specs = getattr(docker_cfg, "extra_mounts", None) or [] + specs: list[str] = [] + for raw_spec in raw_specs: + specs.append(_validate_extra_mount(raw_spec)) + return specs + + +def _write_docker_compose_mounts(task: TaskDefinition, docker_cfg: object, env_dir: Path, warnings: list[str]) -> None: + """Write ``environment/docker-compose.yaml``: always mounts ``environment/task.yaml`` + read-only at :data:`AGENT_TASK_YAML_PATH` (instead of the Dockerfile ``COPY``ing it + in), plus one read-only bind mount per local plugin dir and per ``TemplateDirSource`` + in ``sandbox.template_sources``, plus one bind mount (mode as authored) per + ``sandbox.docker.extra_mounts`` entry, when any of those are non-empty. + + Called AFTER ``_write_agent_phase_task_yaml`` has already written + ``environment/task.yaml`` — the mount source must exist on disk at export time for + the path to be meaningful. + + Harbor's Docker environment auto-detects ``environment/docker-compose.yaml`` and + layers it on top of the generated build/prebuilt compose file's ``main`` service + (see ``harborframework``'s ``docker.py::_docker_compose_paths``) — this is Harbor's + own documented mechanism for host bind mounts; ``task.toml`` itself has no + mount/volume field (checked directly against ``EnvironmentConfig`` in + ``harbor/models/task/config.py``: only resource limits, image selection, and env-var + passthrough live there). + """ + task_yaml_mount = f"{(env_dir / 'task.yaml').resolve().as_posix()}:{AGENT_TASK_YAML_PATH}:ro" + host_specific_specs = ( + _plugin_volume_specs(task) + _template_volume_specs(task, warnings) + _extra_mount_volume_specs(docker_cfg) + ) + volumes = [task_yaml_mount, *host_specific_specs] + compose_path = env_dir / "docker-compose.yaml" + compose_path.write_text( + yaml.safe_dump({"services": {"main": {"volumes": volumes}}}, sort_keys=False), + encoding="utf-8", + ) + if host_specific_specs: + warnings.append( + "environment/docker-compose.yaml bind-mounts agent.plugins[]/template_sources[]/" + + "sandbox.docker.extra_mounts path(s) from this machine's own host paths -- unlike the rest " + + "of the export, this is NOT portable to another machine (or CI runner) without the same " + + "paths present there too." + ) def _write_agent_phase_task_yaml( task: TaskDefinition, env_dir: Path, *, initial_prompt: str, warnings: list[str] -) -> bool: +) -> None: """``environment/task.yaml`` — the REAL agent config, but criteria-free. - Baked into the image at :data:`AGENT_TASK_YAML_PATH` (see the Dockerfile - ``COPY`` line in ``_write_environment``) for a ``CoderEvalAgent`` Harbor - agent embed (``harbor/agent.py``) to run via ``coder-eval execute --format - harbor``. Unlike ``tests/task.yaml`` (the verifier's placeholder-agent, - real-criteria file), this is the mirror image: ``task.agent`` and the - resolved prompt are carried over VERBATIM (the whole point is running the - task's actual configured agent), but ``success_criteria`` is forced to - ``[]`` — never leaked into the agent-visible image, and never read either - (`coder-eval execute` never grades). ``TaskDefinition`` no longer requires - at least one criterion, so this no longer needs a placeholder. + Bind-mounted read-only in at :data:`AGENT_TASK_YAML_PATH` (see + ``_write_docker_compose_mounts``, called after this from ``_write_environment``) + for a ``CoderEvalAgent`` Harbor agent embed (``harbor/agent.py``) to run via + ``coder-eval execute --format harbor``. Unlike ``tests/task.yaml`` (the + verifier's placeholder-agent, real-criteria file), this is the mirror image: + ``task.agent`` and the resolved prompt are carried over VERBATIM (the whole + point is running the task's actual configured agent), but ``success_criteria`` + is forced to ``[]`` — never leaked into the agent-visible container, and never + read either (`coder-eval execute` never grades). ``TaskDefinition`` no longer + requires at least one criterion, so this no longer needs a placeholder. ``sandbox`` is the original task's ``sandbox`` block, field-merged with - ``driver: tempdir`` and (when present) a rewritten ``template_sources`` — - everything else (``python.env_packages``, ``limits``, ...) is preserved, - not blanked. ``driver`` must be forced regardless of the original task's - driver: this file runs INSIDE the container Harbor already built, so - re-declaring ``driver: docker`` here would have ``coder-eval execute`` try - to launch a second, nested container rather than just using its own - in-process sandbox at the container's current workdir. ``docker`` config - is dropped along with it — moot once ``driver`` is forced to ``tempdir``. + ``driver: tempdir`` — everything else (``python.env_packages``, ``limits``, + ``template_sources``, ...) is preserved verbatim, not blanked. + ``template_sources``/``agent.plugins[]`` paths need no rewriting: they're + bind-mounted at their own unchanged host path (see ``_write_docker_compose_mounts``), + so whatever absolute path ``model_dump()`` already carries is correct as-is. + ``driver`` must be forced regardless of the original task's driver: this file + runs INSIDE the container Harbor already built, so re-declaring ``driver: + docker`` here would have ``coder-eval execute`` try to launch a second, nested + container rather than just using its own in-process sandbox at the container's + current workdir. ``docker`` config is dropped along with it — moot once + ``driver`` is forced to ``tempdir``. ``initial_prompt`` is omitted entirely for a ``type: none`` (agentless) task: coder-eval's own schema forbids a no-op agent from setting a prompt @@ -536,28 +640,35 @@ def _write_agent_phase_task_yaml( verified live via ``harbor run`` against a real ``harbor`` install, which surfaced exactly this ``TaskDefinition`` validation error before this guard was added. - - Returns whether any template directory was copied into ``environment/templates/``, - so ``_write_environment`` knows whether to add the corresponding Dockerfile ``COPY``. """ is_agentless = task.agent is not None and task.agent.type == "none" - rewritten_template_sources = _copy_template_sources(task, env_dir, warnings) sandbox_dict = task.sandbox.model_dump(mode="json", exclude_none=True) sandbox_dict["driver"] = "tempdir" sandbox_dict.pop("docker", None) - if rewritten_template_sources is not None: - sandbox_dict["template_sources"] = rewritten_template_sources + agent_dict = ( + task.agent.model_dump(mode="json", exclude_none=True) if task.agent is not None else {"type": "claude-code"} + ) payload: dict[str, object] = { "task_id": task.task_id, "description": task.description, - "agent": ( - task.agent.model_dump(mode="json", exclude_none=True) if task.agent is not None else {"type": "claude-code"} - ), + "agent": agent_dict, "sandbox": sandbox_dict, "success_criteria": [], } if not is_agentless: payload["initial_prompt"] = initial_prompt + if task.pre_run: + # `pre_run` runs "inside the sandbox after setup completes but before the + # agent starts" (PreRunCommand's own docstring) -- exactly the phase + # `coder-eval execute` still performs for the CoderEvalAgent embed (it shares + # `run`'s entire pipeline minus grading, see execute_command.py's module + # docstring), so this is a real translation, not a Dockerfile-build-time + # stand-in. Unlike `post_run` (belongs to the GRADING phase -- see + # orchestrator.py's own comment -- which `coder-eval execute` never runs at + # all), `pre_run` has a real place to run here. Commands are relative to the + # sandbox cwd, resolved the same way template_sources/`_setup_template` are, + # so no path rewriting is needed. + payload["pre_run"] = [c.model_dump(mode="json", exclude_none=True) for c in task.pre_run] if task.run_limits is not None: # `CoderEvalAgent.run()` invokes `coder-eval execute` against this # file, which enforces `max_turns`/`turn_timeout`/`task_timeout`/the @@ -567,7 +678,6 @@ def _write_agent_phase_task_yaml( # token ceiling at all). payload["run_limits"] = task.run_limits.model_dump(mode="json", exclude_none=True) (env_dir / "task.yaml").write_text(yaml.safe_dump(payload, sort_keys=False, allow_unicode=True), encoding="utf-8") - return (env_dir / "templates").is_dir() def _write_test_sh(out_dir: Path, *, workdir: str) -> None: @@ -604,7 +714,7 @@ def _write_reference(task: TaskDefinition, task_file: Path, out_dir: Path) -> No ) from e -def _write_task_toml(task: TaskDefinition, out_dir: Path, *, workdir: str) -> None: +def _write_task_toml(task: TaskDefinition, out_dir: Path, *, workdir: str, docker_image: str | None) -> None: verifier_section: dict[str, object] = {} env_names = _env_passthrough_names(task) if env_names: @@ -620,7 +730,7 @@ def _write_task_toml(task: TaskDefinition, out_dir: Path, *, workdir: str) -> No "description": task.description, "keywords": list(task.tags), }, - "environment": _build_environment_section(task, workdir=workdir), + "environment": _build_environment_section(task, workdir=workdir, docker_image=docker_image), } if verifier_section: doc["verifier"] = verifier_section @@ -670,15 +780,17 @@ def _env_template_dict(names: list[str]) -> dict[str, str]: return {name: f"${{{name}:-}}" for name in names} -def _build_environment_section(task: TaskDefinition, *, workdir: str) -> dict[str, object]: - """No ``docker_image`` key: ``_write_environment`` always writes ``environment/Dockerfile`` - - now (either copied from ``dockerfile_path`` or synthesized from - ``docker_cfg.image``), so Harbor always builds from that file rather than - pulling a bare image reference named in ``task.toml``. +def _build_environment_section(task: TaskDefinition, *, workdir: str, docker_image: str | None) -> dict[str, object]: + """``docker_image`` is set (by ``_write_environment``) only when no ``environment/Dockerfile`` + was written at all -- i.e. no ``dockerfile_path``, nothing to build -- so Harbor's own + ``should_use_prebuilt_docker_image`` pulls this image directly and skips building. When a + Dockerfile WAS written (``dockerfile_path`` set: real build steps needed), this stays unset + and Harbor builds from that file instead. """ docker_cfg = task.sandbox.docker section: dict[str, object] = {"workdir": workdir} + if docker_image is not None: + section["docker_image"] = docker_image limits = task.sandbox.limits if limits.max_memory_mb is not None: section["memory_mb"] = limits.max_memory_mb diff --git a/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile b/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile index 5a55d1ce..6f2be053 100644 --- a/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile +++ b/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile @@ -2,5 +2,3 @@ FROM ubuntu:24.04 RUN apt-get update && apt-get install -y --no-install-recommends coreutils && rm -rf /var/lib/apt/lists/* WORKDIR /app - -COPY task.yaml /opt/coder-eval-task/task.yaml diff --git a/tests/_fixtures/harbor_export_golden/expected/environment/docker-compose.yaml b/tests/_fixtures/harbor_export_golden/expected/environment/docker-compose.yaml new file mode 100644 index 00000000..32448d8e --- /dev/null +++ b/tests/_fixtures/harbor_export_golden/expected/environment/docker-compose.yaml @@ -0,0 +1,4 @@ +services: + main: + volumes: + - /environment/task.yaml:/opt/coder-eval-task/task.yaml:ro diff --git a/tests/test_harbor_export_golden.py b/tests/test_harbor_export_golden.py index f543ba9b..340f8ac3 100644 --- a/tests/test_harbor_export_golden.py +++ b/tests/test_harbor_export_golden.py @@ -37,17 +37,39 @@ class of bug directly, cheaper than enumerating every field in prose. _REGEN = os.environ.get("GOLDEN_REGEN", "").strip().lower() in {"1", "true", "yes", "on"} -def _relative_files(root: Path) -> dict[str, str]: - """Every file under root, as {posix-relative-path: text-content}.""" +_OUT_DIR_PLACEHOLDER = "" + + +def _relative_files(root: Path, *, out_dir: Path) -> dict[str, str]: + """Every file under root, as {posix-relative-path: text-content}. + + ``environment/docker-compose.yaml`` embeds an absolute bind-mount source path + for ``environment/task.yaml`` (``_write_docker_compose_mounts`` always mounts + it read-only rather than ``COPY``ing it in) -- that path is ``out_dir``, which + is a fresh ``tmp_path`` on every test run and would never match a committed + golden file byte-for-byte. Normalize it to a stable placeholder before + comparing (and before writing the golden fixture itself under + ``GOLDEN_REGEN=1``), same as any other run-specific value this test would + otherwise have to special-case. + + Compose volume specs are always POSIX-style (``_write_docker_compose_mounts`` + emits ``as_posix()``, since docker-compose volume specs are POSIX regardless + of the exporter's host OS) -- so the placeholder substitution must match on + the POSIX form of ``out_dir`` too, not the OS-native (backslash, on Windows) + form ``str()``/``resolve()`` would give. + """ + out_dir_str = out_dir.resolve().as_posix() return { - p.relative_to(root).as_posix(): p.read_text(encoding="utf-8") for p in sorted(root.rglob("*")) if p.is_file() + p.relative_to(root).as_posix(): p.read_text(encoding="utf-8").replace(out_dir_str, _OUT_DIR_PLACEHOLDER) + for p in sorted(root.rglob("*")) + if p.is_file() } def test_export_matches_the_committed_golden_tree(tmp_path: Path) -> None: out_dir = tmp_path / "out" export_task(_SOURCE_TASK, out_dir) - actual = _relative_files(out_dir) + actual = _relative_files(out_dir, out_dir=out_dir) if _REGEN: for rel_path, content in actual.items(): @@ -61,7 +83,9 @@ def test_export_matches_the_committed_golden_tree(tmp_path: Path) -> None: return assert _EXPECTED_DIR.is_dir(), "no committed golden tree yet -- run with GOLDEN_REGEN=1 first" - expected = _relative_files(_EXPECTED_DIR) + # out_dir=_EXPECTED_DIR here is a no-op substitution -- the committed golden content + # already holds the literal _OUT_DIR_PLACEHOLDER, never a real absolute path. + expected = _relative_files(_EXPECTED_DIR, out_dir=_EXPECTED_DIR) assert set(actual) == set(expected), ( f"emitted file set drifted from the golden tree.\n" diff --git a/tests/test_harbor_packager.py b/tests/test_harbor_packager.py index 7299f2a9..22038a52 100644 --- a/tests/test_harbor_packager.py +++ b/tests/test_harbor_packager.py @@ -145,7 +145,10 @@ def test_task_toml_parses_and_carries_the_mapped_fields(self, tmp_path: Path) -> doc = tomllib.loads((out_dir / "task.toml").read_text(encoding="utf-8")) assert doc["task"]["name"] == "coder-eval/greet" assert doc["task"]["keywords"] == ["smoke"] - assert "docker_image" not in doc["environment"] # always built from environment/Dockerfile now + # No dockerfile_path -- no environment/Dockerfile at all; task.toml points Harbor + # straight at the pre-built image instead (see _write_environment). + assert not (out_dir / "environment" / "Dockerfile").exists() + assert doc["environment"]["docker_image"] == "byod-custom-image:0.1.0" assert doc["environment"]["memory_mb"] == 2048 assert doc["environment"]["cpus"] == 2 assert doc["environment"]["network_mode"] == "no-network" @@ -228,8 +231,9 @@ def test_baked_at_fixed_path_via_dockerfile_copy(self, tmp_path: Path) -> None: export_task(task_file, out_dir) assert (out_dir / "environment" / "task.yaml").exists() - dockerfile_text = (out_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") - assert "COPY task.yaml /opt/coder-eval-task/task.yaml" in dockerfile_text + compose = yaml.safe_load((out_dir / "environment" / "docker-compose.yaml").read_text(encoding="utf-8")) + volumes = compose["services"]["main"]["volumes"] + assert any(v.endswith(":/opt/coder-eval-task/task.yaml:ro") for v in volumes) def test_carries_the_real_agent_config_but_no_real_criteria(self, tmp_path: Path) -> None: task_file = _write_task(tmp_path, {"agent": {"type": "claude-code", "model": "claude-opus-5"}}) @@ -263,7 +267,7 @@ def test_reloads_as_a_valid_task_definition(self, tmp_path: Path) -> None: reloaded = TaskDefinition.model_validate(emitted) # must not raise assert reloaded.task_id == "greet" - def test_prebuilt_image_with_no_dockerfile_path_gets_one_synthesized(self, tmp_path: Path) -> None: + def test_prebuilt_image_with_no_dockerfile_path_gets_no_dockerfile_at_all(self, tmp_path: Path) -> None: task_file = _write_task( tmp_path, {"sandbox": {"driver": "docker", "docker": {"image": "byod-custom-image:0.1.0"}}} ) @@ -272,9 +276,14 @@ def test_prebuilt_image_with_no_dockerfile_path_gets_one_synthesized(self, tmp_p result = export_task(task_file, out_dir) assert (out_dir / "environment" / "task.yaml").exists() - dockerfile_text = (out_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") - assert dockerfile_text.startswith("FROM byod-custom-image:0.1.0\n") - assert "COPY task.yaml /opt/coder-eval-task/task.yaml" in dockerfile_text + # No dockerfile_path -- no environment/Dockerfile written at all; task.toml's + # [environment].docker_image points Harbor at the image directly instead. + assert not (out_dir / "environment" / "Dockerfile").exists() + doc = tomllib.loads((out_dir / "task.toml").read_text(encoding="utf-8")) + assert doc["environment"]["docker_image"] == "byod-custom-image:0.1.0" + compose = yaml.safe_load((out_dir / "environment" / "docker-compose.yaml").read_text(encoding="utf-8")) + volumes = compose["services"]["main"]["volumes"] + assert any(v.endswith(":/opt/coder-eval-task/task.yaml:ro") for v in volumes) assert not any("No Dockerfile to bake" in w for w in result.warnings) @@ -311,10 +320,13 @@ def test_dockerfile_with_an_existing_workdir_is_respected_and_not_touched(self, assert result.workdir == "/workspace" dockerfile_text = (out_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") - # The WORKDIR-bearing content is untouched; only the task.yaml COPY - # line (baked in for a CoderEvalAgent embed, see agent_paths.py) is appended. + # The WORKDIR-bearing content is untouched -- task.yaml is bind-mounted in via + # docker-compose.yaml (see agent_paths.py), not COPY'd, so the Dockerfile gets + # no new lines here at all. assert dockerfile_text.startswith(original) - assert "COPY task.yaml /opt/coder-eval-task/task.yaml" in dockerfile_text + compose = yaml.safe_load((out_dir / "environment" / "docker-compose.yaml").read_text(encoding="utf-8")) + volumes = compose["services"]["main"]["volumes"] + assert any(v.endswith(":/opt/coder-eval-task/task.yaml:ro") for v in volumes) assert not any("declared no WORKDIR" in w for w in result.warnings) # test.sh and task.toml must agree with the same resolved workdir. assert '"/workspace"' in (out_dir / "tests" / "test.sh").read_text(encoding="utf-8") @@ -391,20 +403,32 @@ def test_no_warning_when_prebuilt_image_names_coder_eval_agent( class TestPrePostRunWarnings: - def test_pre_run_and_post_run_are_warned_not_silently_dropped(self, tmp_path: Path) -> None: - task_file = _write_task( - tmp_path, - { - "pre_run": [{"command": "echo setup"}], - "post_run": [{"command": "echo cleanup"}], - }, - ) + def test_pre_run_is_translated_into_the_agent_phase_task_yaml(self, tmp_path: Path) -> None: + """`pre_run` runs inside the sandbox before the agent starts (PreRunCommand's own + docstring) -- exactly the phase `coder-eval execute` still performs for the + CoderEvalAgent embed (it shares `run`'s pipeline minus grading), so unlike + `post_run` this is a real translation, not a dropped/warned-about field.""" + task_file = _write_task(tmp_path, {"pre_run": [{"command": "echo setup", "timeout": 15}]}) + out_dir = tmp_path / "out" + + result = export_task(task_file, out_dir) + + assert not any("pre_run" in w for w in result.warnings) + emitted = yaml.safe_load((out_dir / "environment" / "task.yaml").read_text(encoding="utf-8")) + assert emitted["pre_run"] == [{"command": "echo setup", "timeout": 15, "fail_on_error": True}] + + def test_post_run_is_warned_not_silently_dropped(self, tmp_path: Path) -> None: + """`post_run` belongs to the GRADING phase (orchestrator.py's own comment), + which `coder-eval execute` never runs at all -- there is no phase left for it + to execute in, so (unlike `pre_run`) it stays untranslated and warned.""" + task_file = _write_task(tmp_path, {"post_run": [{"command": "echo cleanup"}]}) out_dir = tmp_path / "out" result = export_task(task_file, out_dir) - assert any("pre_run" in w for w in result.warnings) assert any("post_run" in w for w in result.warnings) + emitted = yaml.safe_load((out_dir / "environment" / "task.yaml").read_text(encoding="utf-8")) + assert "post_run" not in emitted def test_no_pre_or_post_run_produces_no_such_warnings(self, tmp_path: Path) -> None: task_file = _write_task(tmp_path) @@ -541,8 +565,8 @@ def test_no_env_section_when_the_resolved_allowlist_is_empty(self, tmp_path: Pat class TestTemplateSourcesCopy: - """``TemplateDirSource`` directories must be copied into the export -- otherwise - ``environment/task.yaml`` would name an absolute HOST path (see + """``TemplateDirSource`` directories must be bind-mounted into the export -- + otherwise ``environment/task.yaml`` would name an absolute HOST path (see ``task_loader.resolve_template_source_paths``) that does not exist inside the container the ``CoderEvalAgent`` embed actually runs in. """ @@ -553,7 +577,7 @@ def _write_template_dir(self, tmp_path: Path, name: str = "starter") -> Path: (template_dir / "main.py").write_text("def stub(): ...\n", encoding="utf-8") return template_dir - def test_template_dir_is_copied_and_path_rewritten(self, tmp_path: Path) -> None: + def test_template_dir_is_mounted_read_only_at_its_own_path(self, tmp_path: Path) -> None: template_dir = self._write_template_dir(tmp_path) task_file = _write_task( tmp_path, @@ -569,14 +593,18 @@ def test_template_dir_is_copied_and_path_rewritten(self, tmp_path: Path) -> None export_task(task_file, out_dir) - copied = out_dir / "environment" / "templates" / "00-starter" / "main.py" - assert copied.read_text(encoding="utf-8") == "def stub(): ...\n" + # No copy on the export side -- the template dir stays exactly where it was. + assert not (out_dir / "environment" / "templates").exists() emitted = yaml.safe_load((out_dir / "environment" / "task.yaml").read_text(encoding="utf-8")) - assert emitted["sandbox"]["template_sources"][0]["path"] == "/opt/coder-eval-task/templates/00-starter" + # Path carried over verbatim -- mounted at its own host path, not rewritten. + assert emitted["sandbox"]["template_sources"][0]["path"] == str(template_dir) - dockerfile_text = (out_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") - assert "COPY templates/ /opt/coder-eval-task/templates/" in dockerfile_text + compose = yaml.safe_load((out_dir / "environment" / "docker-compose.yaml").read_text(encoding="utf-8")) + volumes = compose["services"]["main"]["volumes"] + # Compose volume specs are always POSIX-style, regardless of host OS. + template_posix = template_dir.as_posix() + assert f"{template_posix}:{template_posix}:ro" in volumes def test_agent_phase_sandbox_preserves_python_and_limits(self, tmp_path: Path) -> None: task_file = _write_task( @@ -598,15 +626,18 @@ 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_no_templates_dir_or_copy_line_when_the_task_has_no_template_sources(self, tmp_path: Path) -> None: + 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" export_task(task_file, out_dir) assert not (out_dir / "environment" / "templates").exists() - dockerfile_text = (out_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") - assert "templates" not in dockerfile_text + compose = yaml.safe_load((out_dir / "environment" / "docker-compose.yaml").read_text(encoding="utf-8")) + volumes = compose["services"]["main"]["volumes"] + # Only the always-present task.yaml mount -- no template_sources, so nothing else to mount. + assert len(volumes) == 1 + assert volumes[0].endswith(":/opt/coder-eval-task/task.yaml:ro") def test_nonexistent_template_dir_is_a_hard_export_failure(self, tmp_path: Path) -> None: """A missing template dir is NOT downgraded to a warning: the agent-phase