diff --git a/.claude/notes/reporting.md b/.claude/notes/reporting.md index a8fdf1b0..29c7b674 100644 --- a/.claude/notes/reporting.md +++ b/.claude/notes/reporting.md @@ -454,11 +454,22 @@ shell at exec time and equals the WORKDIR because the exec is given no explicit No Dockerfile is written unless the task sets `sandbox.docker.dockerfile_path` — only real build steps (`RUN`) need one. Harbor's own `should_use_prebuilt_docker_image` (`harbor/environments/definition.py`) pulls `task.toml`'s `[environment].docker_image` -and skips the build, confirmed against a real `harbor` install; `WORKDIR` needs no -Dockerfile line either, because `[environment].workdir` is what Harbor passes as the -`cwd` / `-w` at `docker exec` time, whether the image was built or pulled. So the -Dockerfile-less shape is a real choice rather than a null-vs-set distinction: -`docker_cfg.image` always has a value (`default_factory=get_default_docker_image_tag`). +and skips the build, confirmed against a real `harbor` install. So the Dockerfile-less +shape is a real choice rather than a null-vs-set distinction: `docker_cfg.image` always +has a value (`default_factory=get_default_docker_image_tag`). + +`[environment].workdir` is what Harbor passes as `-w` at `docker exec` time, but the +packager only sets it for an EXPLICIT override (`sandbox.docker.working_dir`, or a +Dockerfile's own `WORKDIR` line) — never a guess. `docker exec` (unlike `docker run`) +hard-fails with exit 127 if `-w`'s path doesn't already exist in the image, so guessing +one at export time (originally via `docker image inspect`, falling back to a hardcoded +`/app` when that failed) meant an export-time snapshot could go stale against whatever +image the trial actually ran under, on a different machine or after the image changed — +confirmed live: a wrong guess baked into `task.toml` broke every trial with the same exit +127 well before the agent ever ran. Leaving `workdir` unset when the task names no +override means `docker exec` runs with no `-w` at all, so the container's OWN current +`WORKDIR` decides — always correct, nothing to go stale. `tests/test.sh` never needs the +value in advance either; see below. A bind mount also removes a hazard the `COPY` approach had: it never walks the export's own `-o` directory, so a plugin source that contains it cannot self-nest. @@ -497,9 +508,12 @@ letting a bare `OSError` escape, because the CLI catches only the export errors unreadable tree would otherwise abort a whole experiment export the docstring promises it will not abort. -The generated shell script quotes the workdir before interpolating it: that value comes -from a task-YAML field whose only validator checks for a leading slash, so it does not -reject quotes, substitutions, backticks or newlines. +The generated shell script no longer interpolates a workdir value at all — it resolves its +own cwd via `$(pwd)` at run time, a fixed literal in `_TEST_SH_TEMPLATE`. `docker exec` +(with `-w` when the task set an explicit override, or none when it did not — see above) +always lands the shell there, whether or not the same container's agent phase used an +explicit override too, so `pwd` is authoritative and there is no longer an injection +surface to `shlex.quote` against. ## The ATIF trajectory bridge diff --git a/src/coder_eval/harbor/packager.py b/src/coder_eval/harbor/packager.py index afd6b8e5..b5ca72e2 100644 --- a/src/coder_eval/harbor/packager.py +++ b/src/coder_eval/harbor/packager.py @@ -4,19 +4,13 @@ as the grader. Task *definition* only; ``harbor/reward.py`` is the runtime contract that makes the emitted ``tests/test.sh`` work once Harbor runs it. -Emitted layout:: +Layout:: / ├── task.toml - ├── instruction.md # placeholder -- the real prompt is in environment/task.yaml - ├── environment/ - │ ├── Dockerfile # only when sandbox.docker.dockerfile_path is set - │ ├── task.yaml # criteria-free copy for the CoderEvalAgent embed - │ └── docker-compose.yaml # always written; every input is bind-mounted, never COPY'd - └── tests/ - ├── test.sh # the two-line shim - ├── task.yaml # the criteria, as authored - └── reference/ # task.reference, verifier-side only + ├── instruction.md # placeholder + ├── environment/ # Dockerfile (optional), task.yaml, docker-compose.yaml + └── tests/ # test.sh, task.yaml, reference/ ``tests/task.yaml`` is uploaded whole into the container at ``/tests/``, which is why ``_TEST_SH_TEMPLATE`` references it absolutely rather than cwd-relative. @@ -27,9 +21,7 @@ from __future__ import annotations import os -import shlex import shutil -import subprocess from dataclasses import dataclass, field from pathlib import Path @@ -44,7 +36,6 @@ from coder_eval.path_utils import REFERENCE_COPY_IGNORE, ignore_patterns_and_symlinks -DEFAULT_WORKDIR = "/app" _HARBOR_SCHEMA_VERSION = "1.4" # pinned to the harbor 0.22.0 findings in tmp/harborframework.md § C0 # Graded via `coder-eval evaluate`, which never instantiates an agent on the @@ -70,7 +61,13 @@ # always reach it. set -u -coder-eval evaluate /tests/task.yaml "{workdir}" --in-place --run-dir /logs/verifier || true +# `$(pwd)` -- not a baked-in path -- so this script is agnostic of whatever +# WORKDIR the agent's container actually used (task.toml's `environment.workdir` +# when the task set one explicitly, or the image's own built-in WORKDIR +# otherwise; see _write_environment). `docker exec` (or `-w`, when set) always +# lands this shell's cwd there, so `pwd` is authoritative at run time -- no +# export-time guess needed, and nothing to drift if the image changes later. +coder-eval evaluate /tests/task.yaml "$(pwd)" --in-place --run-dir /logs/verifier || true coder-eval harbor reward /logs/verifier --out /logs/verifier/reward.json """ @@ -100,7 +97,7 @@ class ExportResult: """What ``export_task`` produced, for the CLI to report.""" out_dir: Path - workdir: str + workdir: str | None warnings: list[str] = field(default_factory=list) @@ -188,7 +185,7 @@ def export_resolved_task( 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_test_sh(out_dir) _write_reference(task, task_file, out_dir) _write_task_toml(task, out_dir, workdir=workdir, docker_image=docker_image) @@ -243,19 +240,22 @@ def _write_environment( task_file: Path, out_dir: Path, warnings: list[str], -) -> tuple[str, str | None]: - """Derive ``environment/`` and return ``(workdir, docker_image)``: +) -> tuple[str | None, str | None]: + """Derive ``environment/`` and return ``(workdir, docker_image)``. - - ``workdir``: the WORKDIR both it and test.sh must agree on. 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, so - ``task.toml``'s ``[environment].docker_image`` points straight at the pre-built - image; ``None`` when a Dockerfile was written and Harbor must build from it. + ``workdir`` is an EXPLICIT override only — ``sandbox.docker.working_dir`` or a + Dockerfile's own ``WORKDIR`` line. ``None`` otherwise, so Harbor's ``docker exec`` + gets no ``-w`` and lands wherever the image's OWN ``WORKDIR`` already puts it; + ``tests/test.sh`` resolves the real cwd itself at run time via ``$(pwd)``. - ``dockerfile_path`` set is the only shape that writes a Dockerfile: it is copied in - as the base, with a ``WORKDIR`` appended when it declared none. Unset writes none. + ``docker_image`` is set only when no ``environment/Dockerfile`` was written, so + ``task.toml``'s ``[environment].docker_image`` points at the pre-built image; + ``None`` when a Dockerfile was written and Harbor must build from it. + + A ``dockerfile_path`` is the only shape that writes a Dockerfile, copied in + UNCHANGED — no ``WORKDIR`` appended even when it declares none. ``environment/task.yaml`` is bind-mounted at :data:`AGENT_TASK_YAML_PATH`, never - ``COPY``'d, so no Dockerfile is involved in getting it there. + ``COPY``'d. Rationale: .claude/notes/reporting.md § What the export carries, and what it refuses to carry """ @@ -272,14 +272,11 @@ def _write_environment( if docker_cfg.dockerfile_path is not None: source_dockerfile = Path(docker_cfg.dockerfile_path) # already absolute — load_task resolves it shutil.copy2(source_dockerfile, dest_dockerfile) - workdir = docker_cfg.working_dir or _find_workdir(dest_dockerfile) or DEFAULT_WORKDIR - if _find_workdir(dest_dockerfile) is None: - with dest_dockerfile.open("a", encoding="utf-8") as fh: - fh.write(f"\nWORKDIR {workdir}\n") - warnings.append( - f"environment/Dockerfile declared no WORKDIR; appended `WORKDIR {workdir}` so the " - + "verifier and agent phases agree on a path." - ) + # No fabricated WORKDIR appended when the Dockerfile declares none -- + # the built image just inherits its base image's own default, and + # tests/test.sh finds it at run time via `$(pwd)` either way (see + # _TEST_SH_TEMPLATE and this function's docstring). + workdir = docker_cfg.working_dir or _find_workdir(dest_dockerfile) if not _from_line_mentions_coder_eval_agent(dest_dockerfile): warnings.append(_MISSING_CODER_EVAL_WARNING) # A build context beyond the Dockerfile itself is not carried over in v1; @@ -297,54 +294,18 @@ def _write_environment( # 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: - workdir = docker_cfg.working_dir - else: - inspected = _inspect_image_workdir(docker_cfg.image) - if inspected is not None: - workdir = inspected - else: - workdir = DEFAULT_WORKDIR - warnings.append( - f"Could not determine {docker_cfg.image}'s own WORKDIR (image not present locally, or " - + f"docker unavailable at export time) -- defaulting to `{DEFAULT_WORKDIR}`. Harbor's " - + "`docker exec -w` hard-fails if that path does not already exist in the image (unlike " - + "`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." - ) + # No `docker image inspect` guess here either (v1 used to shell out for + # one, defaulting to a fabricated path on failure -- a real bug: a stale + # or wrong guess baked into task.toml made Harbor's `docker exec -w` + # hard-fail with exit 127 the moment it didn't exist in the image that + # actually ran). Leaving `workdir` unset unless the task pins one is + # strictly safer: Harbor's docker environment only adds `-w ` + # when `[environment].workdir` is set at all, so `None` here means the + # container's OWN `WORKDIR` decides -- always current, never a snapshot. + workdir = docker_cfg.working_dir return workdir, docker_cfg.image -def _inspect_image_workdir(image: str) -> str | None: - """Best-effort ``docker image inspect`` for a pre-built image's own ``WORKDIR``. - - A pre-built image has no Dockerfile for ``_find_workdir`` to read, so this - is the only way to avoid guessing a path that doesn't exist in it (Harbor's - ``docker exec -w`` -- unlike ``docker run -w`` -- fails outright if the - directory isn't already there; confirmed live). Returns ``None`` (never - raises) whenever docker isn't available, the image isn't present locally, - or it declares no WORKDIR -- callers fall back to ``DEFAULT_WORKDIR`` and - warn. - """ - try: - result = subprocess.run( - # `--` before `image` (task-YAML-controlled) stops it from being - # parsed as an option if it happens to start with "-". - ["docker", "image", "inspect", "--format", "{{.Config.WorkingDir}}", "--", image], - capture_output=True, - text=True, - encoding="utf-8", - timeout=30, - check=False, - ) - except (OSError, subprocess.SubprocessError): - return None - if result.returncode != 0: - return None - workdir = result.stdout.strip() - return workdir or None - - _MISSING_CODER_EVAL_WARNING = ( "environment/ does not appear to be based on a coder-eval-agent image. `tests/test.sh` calls " "`coder-eval` (for the reward writer), which must be installed inside the container -- v1 assumes " @@ -605,12 +566,12 @@ def _write_agent_phase_task_yaml( (env_dir / "task.yaml").write_text(yaml.safe_dump(payload, sort_keys=False, allow_unicode=True), encoding="utf-8") -def _write_test_sh(out_dir: Path, *, workdir: str) -> None: +def _write_test_sh(out_dir: Path) -> None: + # No task-controlled value is interpolated into the template anymore -- + # `$(pwd)` is a fixed literal (see _TEST_SH_TEMPLATE) -- so there is no + # longer an injection surface here to shlex.quote against. path = out_dir / "tests" / "test.sh" - # HAZARD: `workdir` is task-authored and its only validator checks for a - # leading "/" -- it does not reject quotes, `$(...)`, backticks or newlines. - # shlex.quote before interpolating into the generated /bin/sh script. - path.write_text(_TEST_SH_TEMPLATE.format(workdir=shlex.quote(workdir)), encoding="utf-8") + path.write_text(_TEST_SH_TEMPLATE, encoding="utf-8") path.chmod(0o755) @@ -632,7 +593,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, docker_image: str | None) -> None: +def _write_task_toml(task: TaskDefinition, out_dir: Path, *, workdir: str | None, docker_image: str | None) -> None: verifier_section: dict[str, object] = {} env_names = _env_passthrough_names(task) if env_names: @@ -695,7 +656,9 @@ 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, docker_image: str | None) -> dict[str, object]: +def _build_environment_section( + task: TaskDefinition, *, workdir: str | None, 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 @@ -703,7 +666,9 @@ def _build_environment_section(task: TaskDefinition, *, workdir: str, docker_ima and Harbor builds from that file instead. """ docker_cfg = task.sandbox.docker - section: dict[str, object] = {"workdir": workdir} + section: dict[str, object] = {} + if workdir is not None: + section["workdir"] = workdir if docker_image is not None: section["docker_image"] = docker_image limits = task.sandbox.limits @@ -719,7 +684,6 @@ def _build_environment_section(task: TaskDefinition, *, workdir: str, docker_ima __all__ = [ - "DEFAULT_WORKDIR", "CriteriaNotExportableError", "ExportResult", "TaskNotExportableError", diff --git a/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile b/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile index 6f2be053..5a792680 100644 --- a/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile +++ b/tests/_fixtures/harbor_export_golden/expected/environment/Dockerfile @@ -1,4 +1,2 @@ FROM ubuntu:24.04 RUN apt-get update && apt-get install -y --no-install-recommends coreutils && rm -rf /var/lib/apt/lists/* - -WORKDIR /app diff --git a/tests/_fixtures/harbor_export_golden/expected/task.toml b/tests/_fixtures/harbor_export_golden/expected/task.toml index 1ae65469..9c559e2c 100644 --- a/tests/_fixtures/harbor_export_golden/expected/task.toml +++ b/tests/_fixtures/harbor_export_golden/expected/task.toml @@ -9,7 +9,6 @@ keywords = [ ] [environment] -workdir = "/app" memory_mb = 1024 cpus = 2 network_mode = "no-network" diff --git a/tests/_fixtures/harbor_export_golden/expected/tests/test.sh b/tests/_fixtures/harbor_export_golden/expected/tests/test.sh index f5aa589c..967e3159 100755 --- a/tests/_fixtures/harbor_export_golden/expected/tests/test.sh +++ b/tests/_fixtures/harbor_export_golden/expected/tests/test.sh @@ -13,5 +13,11 @@ # always reach it. set -u -coder-eval evaluate /tests/task.yaml "/app" --in-place --run-dir /logs/verifier || true +# `$(pwd)` -- not a baked-in path -- so this script is agnostic of whatever +# WORKDIR the agent's container actually used (task.toml's `environment.workdir` +# when the task set one explicitly, or the image's own built-in WORKDIR +# otherwise; see _write_environment). `docker exec` (or `-w`, when set) always +# lands this shell's cwd there, so `pwd` is authoritative at run time -- no +# export-time guess needed, and nothing to drift if the image changes later. +coder-eval evaluate /tests/task.yaml "$(pwd)" --in-place --run-dir /logs/verifier || true coder-eval harbor reward /logs/verifier --out /logs/verifier/reward.json diff --git a/tests/test_harbor_experiment_packager.py b/tests/test_harbor_experiment_packager.py index 2be63d64..bd966a0d 100644 --- a/tests/test_harbor_experiment_packager.py +++ b/tests/test_harbor_experiment_packager.py @@ -13,7 +13,6 @@ import pytest import yaml -from coder_eval.harbor import packager from coder_eval.harbor.experiment_packager import export_experiment @@ -32,12 +31,6 @@ } -@pytest.fixture(autouse=True) -def _no_real_docker_inspection(monkeypatch: pytest.MonkeyPatch) -> None: - """Same hermeticity guard as test_harbor_packager.py -- never shell out to real docker.""" - monkeypatch.setattr(packager, "_inspect_image_workdir", lambda image: None) - - def _write_task(tmp_path: Path, overrides: dict[str, object] | None = None, name: str = "task.yaml") -> Path: payload = {**_BASE_TASK, **(overrides or {})} task_file = tmp_path / name diff --git a/tests/test_harbor_packager.py b/tests/test_harbor_packager.py index f75fb931..974170ff 100644 --- a/tests/test_harbor_packager.py +++ b/tests/test_harbor_packager.py @@ -17,7 +17,6 @@ from coder_eval.harbor import packager from coder_eval.harbor.packager import ( - DEFAULT_WORKDIR, CriteriaNotExportableError, TaskNotExportableError, export_task, @@ -25,23 +24,6 @@ from coder_eval.models import TaskDefinition -_REAL_INSPECT_IMAGE_WORKDIR = packager._inspect_image_workdir - - -@pytest.fixture(autouse=True) -def _no_real_docker_inspection(monkeypatch: pytest.MonkeyPatch) -> None: - """Keep this module hermetic: never let ``export_task`` shell out to a real - ``docker image inspect``, whose answer depends on what happens to be - cached on the machine running the tests (an image literally named - ``byod-custom-image:0.1.0`` -- this file's own placeholder BYOD image - name -- built by an unrelated docker-integration test elsewhere in the - suite answered ``/work`` here once, silently flipping this file's - DEFAULT_WORKDIR assertions). Tests that care about the inspection path - itself override this via monkeypatch locally. - """ - monkeypatch.setattr(packager, "_inspect_image_workdir", lambda image: None) - - _BASE_TASK: dict[str, object] = { "task_id": "greet", "description": "Write a greeting to greeting.txt.", @@ -123,17 +105,20 @@ def test_instruction_md_is_a_fixed_placeholder_not_the_real_prompt(self, tmp_pat emitted = yaml.safe_load((out_dir / "environment" / "task.yaml").read_text(encoding="utf-8")) assert emitted["initial_prompt"] == "Write 'hello' to greeting.txt." # the real prompt lives here instead - def test_test_sh_is_executable_and_references_the_resolved_workdir(self, tmp_path: Path) -> None: + def test_test_sh_is_executable_and_resolves_workdir_dynamically(self, tmp_path: Path) -> None: + """test.sh must be workdir-agnostic: it uses `$(pwd)`, not a value baked in at + export time, so it works regardless of whether the task pinned a workdir or + left it to the image's own default (see packager.py's `_write_environment`).""" task_file = _write_task(tmp_path) out_dir = tmp_path / "out" - result = export_task(task_file, out_dir) + export_task(task_file, out_dir) test_sh = out_dir / "tests" / "test.sh" if os.name != "nt": # NTFS has no chmod executable bit assert test_sh.stat().st_mode & 0o111, "test.sh must be executable" content = test_sh.read_text(encoding="utf-8") - assert f'coder-eval evaluate /tests/task.yaml "{result.workdir}"' in content + assert 'coder-eval evaluate /tests/task.yaml "$(pwd)"' in content assert "coder-eval harbor reward /logs/verifier --out /logs/verifier/reward.json" in content def test_task_toml_parses_and_carries_the_mapped_fields(self, tmp_path: Path) -> None: @@ -152,7 +137,10 @@ def test_task_toml_parses_and_carries_the_mapped_fields(self, tmp_path: Path) -> assert doc["environment"]["memory_mb"] == 2048 assert doc["environment"]["cpus"] == 2 assert doc["environment"]["network_mode"] == "no-network" - assert doc["environment"]["workdir"] == DEFAULT_WORKDIR + # No sandbox.docker.working_dir override and no docker inspection at export + # time (removed -- see packager.py's _write_environment) -- `workdir` is + # omitted entirely so Harbor's `docker exec` uses the image's own WORKDIR. + assert "workdir" not in doc["environment"] def test_network_bridge_maps_to_public(self, tmp_path: Path) -> None: task_file = _write_task(tmp_path, {"sandbox": {"driver": "docker", "docker": {"network": "bridge"}}}) @@ -293,10 +281,14 @@ def test_prebuilt_image_with_no_dockerfile_path_gets_no_dockerfile_at_all(self, class TestDockerfileWorkdirResolution: - def test_dockerfile_with_no_workdir_gets_one_appended_and_warned(self, tmp_path: Path) -> None: + def test_dockerfile_with_no_workdir_is_left_unset_and_untouched(self, tmp_path: Path) -> None: + """No WORKDIR line is fabricated and appended anymore: the built image simply + inherits its base image's own default, and tests/test.sh finds the real cwd + at run time via `$(pwd)` regardless (see packager.py's `_write_environment`).""" + original = "FROM ubuntu:24.04\nRUN apt-get update\n" env_dir = tmp_path / "environment" env_dir.mkdir() - (env_dir / "Dockerfile").write_text("FROM ubuntu:24.04\nRUN apt-get update\n", encoding="utf-8") + (env_dir / "Dockerfile").write_text(original, encoding="utf-8") task_file = _write_task( tmp_path, {"sandbox": {"driver": "docker", "docker": {"dockerfile_path": "environment/Dockerfile"}}}, @@ -305,10 +297,12 @@ def test_dockerfile_with_no_workdir_gets_one_appended_and_warned(self, tmp_path: result = export_task(task_file, out_dir) - assert result.workdir == DEFAULT_WORKDIR + assert result.workdir is None dockerfile_text = (out_dir / "environment" / "Dockerfile").read_text(encoding="utf-8") - assert f"WORKDIR {DEFAULT_WORKDIR}" in dockerfile_text - assert any("declared no WORKDIR" in w for w in result.warnings) + assert dockerfile_text == original + assert not any("declared no WORKDIR" in w for w in result.warnings) + doc = tomllib.loads((out_dir / "task.toml").read_text(encoding="utf-8")) + assert "workdir" not in doc["environment"] def test_dockerfile_with_an_existing_workdir_is_respected_and_not_touched(self, tmp_path: Path) -> None: env_dir = tmp_path / "environment" @@ -333,8 +327,12 @@ def test_dockerfile_with_an_existing_workdir_is_respected_and_not_touched(self, 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") + # test.sh no longer needs to agree on a literal value -- it resolves the real + # cwd itself via `$(pwd)` -- but task.toml still surfaces the Dockerfile's own + # explicit WORKDIR so Harbor's `docker exec -w` pins the same path deliberately. + assert 'coder-eval evaluate /tests/task.yaml "$(pwd)"' in (out_dir / "tests" / "test.sh").read_text( + encoding="utf-8" + ) doc = tomllib.loads((out_dir / "task.toml").read_text(encoding="utf-8")) assert doc["environment"]["workdir"] == "/workspace" @@ -392,14 +390,7 @@ def test_warns_when_prebuilt_image_does_not_name_coder_eval_agent(self, tmp_path result = export_task(task_file, tmp_path / "out") assert any("coder-eval-agent" in w for w in result.warnings) - def test_no_warning_when_prebuilt_image_names_coder_eval_agent( - self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch - ) -> None: - # Give inspection a real-looking answer -- a None here would ALSO warn - # ("Could not determine ...'s own WORKDIR"), whose text incidentally - # contains "coder-eval-agent" (the image name), which is not the - # warning this test is checking for. - monkeypatch.setattr(packager, "_inspect_image_workdir", lambda image: "/work") + def test_no_warning_when_prebuilt_image_names_coder_eval_agent(self, tmp_path: Path) -> None: task_file = _write_task( tmp_path, {"sandbox": {"driver": "docker", "docker": {"image": "coder-eval-agent:0.12.0"}}} ) @@ -441,76 +432,34 @@ def test_no_pre_or_post_run_produces_no_such_warnings(self, tmp_path: Path) -> N assert not any("pre_run" in w or "post_run" in w for w in result.warnings) -class TestPrebuiltImageWorkdirInspection: +class TestPrebuiltImageWorkdir: """A pre-built (no ``dockerfile_path``) image has no Dockerfile ``WORKDIR`` line - to read, so the packager shells out to ``docker image inspect`` for it -- a - real bug this closes: defaulting to ``/app`` unconditionally exported a task - that failed at Harbor verify time with exit 127, because Harbor's - ``docker exec -w`` (unlike ``docker run -w``) refuses to chdir into a path - that doesn't already exist in the image (confirmed live against - ``coder-eval-agent:latest``, whose real WORKDIR is ``/work``). + to read, and v1 no longer shells out to ``docker image inspect`` to guess one + either -- a real bug that closed: an export-time snapshot (or its failure mode, + defaulting to a fabricated ``/app``) could go stale against whatever image the + trial actually ran under, and Harbor's ``docker exec -w`` (unlike ``docker run + -w``) refuses to chdir into a path that doesn't already exist in the image + (confirmed live). Leaving ``workdir`` unset unless the task pins one lets + Harbor's ``docker exec`` run with no ``-w`` at all, so the container's OWN + current ``WORKDIR`` always decides -- see packager.py's ``_write_environment``. """ - def test_uses_the_inspected_workdir_when_docker_reports_one( - self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch - ) -> None: - monkeypatch.setattr(packager, "_inspect_image_workdir", lambda image: "/work") + def test_no_working_dir_override_leaves_workdir_unset(self, tmp_path: Path) -> None: task_file = _write_task(tmp_path) # _BASE_TASK's image is byod-custom-image:0.1.0 result = export_task(task_file, tmp_path / "out") - assert result.workdir == "/work" - assert not any("Could not determine" in w for w in result.warnings) - - def test_falls_back_to_default_and_warns_when_inspection_fails( - self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch - ) -> None: - monkeypatch.setattr(packager, "_inspect_image_workdir", lambda image: None) - task_file = _write_task(tmp_path) + assert result.workdir is None + assert not any("WORKDIR" in w or "workdir" in w for w in result.warnings) - result = export_task(task_file, tmp_path / "out") - - assert result.workdir == DEFAULT_WORKDIR - assert any("Could not determine" in w and "byod-custom-image:0.1.0" in w for w in result.warnings) - - def test_explicit_working_dir_wins_over_inspection(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr(packager, "_inspect_image_workdir", lambda image: "/from-inspection") + def test_explicit_working_dir_is_used_verbatim(self, tmp_path: Path) -> None: task_file = _write_task(tmp_path, {"sandbox": {"driver": "docker", "docker": {"working_dir": "/explicit"}}}) result = export_task(task_file, tmp_path / "out") assert result.workdir == "/explicit" - - def test_inspect_image_workdir_parses_real_subprocess_output(self, monkeypatch: pytest.MonkeyPatch) -> None: - """Exercise the real (un-mocked) helper against a stubbed ``subprocess.run``. - - The module-wide autouse fixture stubs out ``_inspect_image_workdir`` - itself for hermeticity, so this restores the real function first -- - it is the one thing here under test. - """ - monkeypatch.setattr(packager, "_inspect_image_workdir", _REAL_INSPECT_IMAGE_WORKDIR) - - class _FakeResult: - returncode = 0 - stdout = "/work\n" - - def _fake_run(cmd, **kwargs): - assert cmd[:3] == ["docker", "image", "inspect"] - return _FakeResult() - - monkeypatch.setattr(packager.subprocess, "run", _fake_run) - assert packager._inspect_image_workdir("some-image:tag") == "/work" - - def test_inspect_image_workdir_returns_none_when_docker_binary_is_missing( - self, monkeypatch: pytest.MonkeyPatch - ) -> None: - monkeypatch.setattr(packager, "_inspect_image_workdir", _REAL_INSPECT_IMAGE_WORKDIR) - - def _raise(cmd, **kwargs): - raise FileNotFoundError("docker not found") - - monkeypatch.setattr(packager.subprocess, "run", _raise) - assert packager._inspect_image_workdir("some-image:tag") is None + doc = tomllib.loads((tmp_path / "out" / "task.toml").read_text(encoding="utf-8")) + assert doc["environment"]["workdir"] == "/explicit" class TestEnvPassthroughSections: