diff --git a/.claude/notes/orchestration.md b/.claude/notes/orchestration.md index 01063f22..a37c6588 100644 --- a/.claude/notes/orchestration.md +++ b/.claude/notes/orchestration.md @@ -759,6 +759,25 @@ It RAISES rather than guessing when neither is conclusive. Guessing is worse tha grading the wrong directory makes every path-relative criterion fail as a locating artifact rather than as a verdict, and reports that as an ordinary score. +A resolved `artifacts_dir_template` is passed in as a SECOND TRUSTED ROOT and preferred as +the candidate, since that template may place artifacts outside `run_dir` entirely. Note the +direction: the roots were WIDENED, never the check relaxed — roots stay operator-supplied, +candidates stay untrusted. Without it, `--resume` over a decoupled layout failed the +containment check, fell through to a `run_dir/artifacts` that did not exist, and raised +`RegradeError`. An artifacts dir that exists outranks the recorded `sandbox_path`, which is +merely what the run claimed. + +Only `--resume` (`run_command.py`'s `_grade_resumed_tasks`) passes the resolved +`artifacts_dir` — it has the run's own `BatchRunConfig` in hand, so `resolve_artifacts_dir` +needs no new state. Detached `coder-eval evaluate ` does not: it has no +`BatchRunConfig` to resolve a template from, only the untrusted `task.json` it's grading, and +trusting THAT to widen its own containment root would defeat the check the widening exists +to keep intact. A run made with a non-default `artifacts_dir_template` therefore still needs +`--workspace` passed explicitly to `evaluate` — the same escape hatch `RegradeError` already +names. Not a regression: `evaluate` never auto-located such a workspace before this template +existed either, and the alternative (trusting the recorded path) reopens the class of bug the +previous paragraph describes. + **Every return goes through the containment check, rooted at the RUN DIRECTORY.** Both `sandbox_path` and `task_id` are unvalidated strings out of the run's own `task.json`, so `"../../../../home/victim"` joins to a real directory `is_dir()` happily confirms. The diff --git a/.claude/notes/persistence.md b/.claude/notes/persistence.md index cacd4b41..5d9991b8 100644 --- a/.claude/notes/persistence.md +++ b/.claude/notes/persistence.md @@ -69,6 +69,94 @@ trajectory log the run had already paid for. `grade.docker.log` exists for the s one layer down: on the `run --resume` path `docker.log` is already the executed container's log. +## The run layout as two directory templates + +`logging_dir_template` and `artifacts_dir_template` (`BatchRunConfig`, resolved by +`path_utils.resolve_dir_template`) describe where a task's bookkeeping and its artifacts +go. They are resolved LATE — per task, which is the only point where `${variant}`, +`${task}` and `${repeat}` exist at all — and they are INDEPENDENT: artifacts nesting under +the logging dir is a default, not a law. + +That independence is the point. Harbor puts logs in its own agent logs dir and artifacts at +the container's WORKDIR, two unrelated parts of the filesystem, and because artifacts were +never a child of the logging dir there is nothing to copy and nothing lands twice. The +previous design reached the same end state with a `--workspace-dir`/`--artifacts-dir` pair +that had to be passed the SAME path so an equality check could infer "don't copy"; a +coincidence standing in for an intention. + +An override needs no special-casing anywhere, because substituting a template that contains +no placeholders is the identity function: `/work/output` in, `/work/output` out, down the +same code path as the default. The defaults spell out the historical layout, so an +unspecified run writes byte-identical paths — asserted in `test_path_utils.py` against the +literal old layout rather than against `build_task_run_dir`, which now calls the resolver +and would make the test vacuous. + +Two implementation constraints, both load-bearing: + +- **`string.Template`, never `re.sub`.** `re.sub` interprets backslashes in the + REPLACEMENT, so a Windows `run_dir` of `C:\runs\2026` comes out mangled. +- **`${task}` may contain a separator.** A dataset-expanded `task_id` is + `"/"` (`expand_dataset`, whose row ids are validated precisely because they + become directories), so it nests — on Windows too, where `pathlib` splits on both + separators. + +`${task}` appears twice in the artifacts default (`.../${task}/${repeat}/artifacts/${task}`) +because that IS the layout on disk: the per-task run dir carries it, and +`preserve_to`/`capture_to`/DIRECT_WRITE each appended it again. Kept for compatibility, and +now one string to change rather than five call sites. The `*_as` variants +(`preserve_as`/`capture_as`) exist so a caller-supplied artifacts dir is used as the FINAL +path instead of having `task_id` appended to it a second time; `preserve_to`/`capture_to` +remain as the parent-relative wrappers. + +### What run_dir still owns + +`${run_dir}` is only a placeholder VALUE. But run-LEVEL files — `run.json`, `run.md`, +`experiment.*`, `resume_fingerprint.json` — still follow `--run-dir`, and its default is +CWD-RELATIVE (`runs/`, `config.py`). Omitting `--run-dir` therefore does not +leave those files harmlessly uncollected; when cwd is the agent's WORKDIR it writes them +INSIDE the workspace, polluting the very directory `artifacts_dir_template` names. +Confirmed live: `artifacts/work/runs//run.json` in a collected Harbor trial. +Harbor consequently passes `--run-dir /tmp/coder-eval-run`, a throwaway path outside the +workspace. + +Anything that used to DISCOVER per-task files by walking `run_dir` had to be given the +resolved logging dirs instead, because they need not live under `run_dir` at all and the +walk silently finds nothing: `atif_emit.emit_trajectories_for_run` (which would emit no +trajectory, leaving Harbor's token/cost totals empty) and +`logging_config.aggregate_task_logs` (which would write an empty `experiment.log`). + +### A static template is single-task only + +A placeholder-free template is the identity function, so every task in a multi-task +`run`/`execute` would resolve `--logging-dir`/`--artifacts-dir` to the SAME directory and +overwrite each other's `task.json`/artifacts. `run_batch` refuses this loud +(`ValueError`) when `len(resolved_tasks) > 1` and either template has no `${...}` +placeholders (`path_utils.dir_template_is_static`), mirroring the pre-existing +`--workspace-dir` + multi-task rejection. Static paths exist for exactly the single-task +case below. + +### A flat run_dir needs no special case in run_batch + +Harbor's single-task, static-template mode writes `task.json`/`task.html`/`task.log`/ +artifacts flat at the top-level `run_dir` instead of the usual +`//` nesting -- the multi-task guard above already guarantees +exactly one resolved task here, so that nesting only exists to disambiguate siblings that +can't occur. A flat `run_dir` also means `trajectory.json` (`emit_trajectories_for_run`'s +sibling write) lands at a fixed, predictable path instead of requiring a recursive glob. +`run_batch`'s `run_single` needs no `workspace_dir` special case for this: `rt.run_dir` IS +the resolved `logging_dir_template`, so "flat" is simply what a static template resolves +to, not a mode this seam has to detect. + +### CoderEvalAgent passes both templates as static paths + +`harbor/agent.py`'s `CoderEvalAgent.run()` passes `--logging-dir`/`--artifacts-dir` as two +STATIC paths (Harbor's own agent logs dir, and the container's WORKDIR via `$(pwd)`) rather +than templates with placeholders — the case the identity-function property above exists for. +Because artifacts are no longer a child of the logging dir, nothing lands twice; because the +artifacts destination IS the workspace, `capture_as`'s self-referential guard makes the copy +a no-op. `--run-dir` still points at `_THROWAWAY_RUN_DIR` (`/tmp/coder-eval-run`), never +substituted into either template, for the reason in "What run_dir still owns" above. + ## Judge persistence A judge transcript — tool calls, raw verdict, rendered prompt, system prompt — runs 10-100 diff --git a/.claude/notes/reporting.md b/.claude/notes/reporting.md index 58c8e060..989ff111 100644 --- a/.claude/notes/reporting.md +++ b/.claude/notes/reporting.md @@ -453,6 +453,25 @@ container, never where the verifier looks. Confirmed live — the agent's output every criterion scored 0 as "file does not exist". `$(pwd)` is resolved by the container's shell at exec time and equals the WORKDIR because the exec is given no explicit cwd. +### The artifacts default is CONTAINER_WORK_DIR, not a required field + +Harbor's own artifact collection runs after the agent phase but before the verifier and +container teardown, snapshotting `task.toml`'s `artifacts` source to +`/artifacts//` on the host. +`CoderEvalAgent`'s `--workspace-dir "$(pwd)"` runs the agent in-place at the container's +WORKDIR and skips coder-eval's own copy-out, so without an `artifacts` entry nothing the +agent produced would ever be visible on the host. + +Defaults to `CONTAINER_WORK_DIR` (`/work`, coder-eval's own image WORKDIR) rather than +requiring every task/experiment to restate it — a package exported by coder-eval needs +coder-eval installed in the image, which in practice means derived from +`coder-eval-agent` (a mismatch already warns; see `_MISSING_CODER_EVAL_WARNING`). Unlike +`[environment].workdir` above — deliberately left unset so the container's own WORKDIR +decides `docker exec -w` — guessing wrong here is not fatal: a nonexistent source is a +best-effort collection miss recorded in the artifact manifest, not an exit 127. Declared +as a plain string, not an `ArtifactConfig` table, because Harbor normalizes +`artifacts = ["/x"]` to `ArtifactConfig(source="/x")` itself. + ### What the export carries, and what it refuses to carry No Dockerfile is written unless the task sets `sandbox.docker.dockerfile_path` — only diff --git a/src/coder_eval/cli/execute_command.py b/src/coder_eval/cli/execute_command.py index 51df9af0..da750bbc 100644 --- a/src/coder_eval/cli/execute_command.py +++ b/src/coder_eval/cli/execute_command.py @@ -189,13 +189,38 @@ def execute_command( "--workspace-dir", help=( "Run the single resolved task's agent in-place at this absolute path instead of the " - "standard run_dir/artifacts workspace (copied out to run_dir/artifacts/ at " + "standard artifacts workspace named by --artifacts-dir (copied out there at " "cleanup). Requires exactly one resolved task; refused for sandbox.driver: docker " "(the docker driver already aligns automatically via sandbox.docker.working_dir). " "Meant for a Harbor `CoderEvalAgent` invocation, so the agent's writes land at the " "container's own WORKDIR, where Harbor's verifier phase looks for them." ), ), + logging_dir: str | None = typer.Option( + None, + "--logging-dir", + help=( + "Where task.json/task.log go, as a path template. Placeholders: ${run_dir}, " + "${variant}, ${task}, ${repeat}. Default reproduces ///. " + "A static path (e.g. /logs/agent) resolves every task to itself, so it is only for a " + "single-task run (e.g. Harbor); refused for sandbox.driver: docker and for more than " + "one resolved task." + ), + ), + artifacts_dir: str | None = typer.Option( + None, + "--artifacts-dir", + help=( + "Where the agent's artifacts go -- the FINAL directory, same placeholders as " + "--logging-dir, and independent of it (Harbor puts logs at /logs/agent and " + "artifacts at the container's WORKDIR). When it already holds the workspace " + "there is nothing to copy. Default reproduces ////" + "artifacts/. A static path is only for a single-task run; refused for " + "sandbox.driver: docker (the in-container Orchestrator has no way to receive it) " + "and for more than one resolved task, and refused together with --resume (it would " + "clear an operator-supplied tree the harness did not create)." + ), + ), ) -> None: """Run evaluation tasks WITHOUT checking their success criteria. @@ -250,4 +275,6 @@ def execute_command( set_overrides=set_overrides, format=format, workspace_dir=workspace_dir, + logging_dir=logging_dir, + artifacts_dir=artifacts_dir, ) diff --git a/src/coder_eval/cli/run_command.py b/src/coder_eval/cli/run_command.py index 275f267d..a8ceb645 100644 --- a/src/coder_eval/cli/run_command.py +++ b/src/coder_eval/cli/run_command.py @@ -3,7 +3,6 @@ import asyncio import logging import os -import shutil import sys import urllib.error import urllib.parse @@ -365,13 +364,38 @@ def run_command( "--workspace-dir", help=( "Run the single resolved task's agent in-place at this absolute path instead of the " - "standard run_dir/artifacts workspace (copied out to run_dir/artifacts/ at " + "standard artifacts workspace named by --artifacts-dir (copied out there at " "cleanup). Requires exactly one resolved task; refused for sandbox.driver: docker " "(the docker driver already aligns automatically via sandbox.docker.working_dir). " "Meant for a Harbor `CoderEvalAgent` invocation, so the agent's writes land at the " "container's own WORKDIR, where Harbor's verifier phase looks for them." ), ), + logging_dir: str | None = typer.Option( + None, + "--logging-dir", + help=( + "Where task.json/task.log go, as a path template. Placeholders: ${run_dir}, " + "${variant}, ${task}, ${repeat}. Default reproduces ///. " + "A static path (e.g. /logs/agent) resolves every task to itself, so it is only for a " + "single-task run (e.g. Harbor); refused for sandbox.driver: docker and for more than " + "one resolved task." + ), + ), + artifacts_dir: str | None = typer.Option( + None, + "--artifacts-dir", + help=( + "Where the agent's artifacts go -- the FINAL directory, same placeholders as " + "--logging-dir, and independent of it (Harbor puts logs at /logs/agent and " + "artifacts at the container's WORKDIR). When it already holds the workspace " + "there is nothing to copy. Default reproduces ////" + "artifacts/. A static path is only for a single-task run; refused for " + "sandbox.driver: docker (the in-container Orchestrator has no way to receive it) " + "and for more than one resolved task, and refused together with --resume (it would " + "clear an operator-supplied tree the harness did not create)." + ), + ), ) -> None: """Run evaluation tasks (optionally in parallel). @@ -424,9 +448,25 @@ def run_command( set_overrides=set_overrides, format=format, workspace_dir=workspace_dir, + logging_dir=logging_dir, + artifacts_dir=artifacts_dir, ) +def _dir_template_overrides(logging_dir: str | None, artifacts_dir: str | None) -> dict[str, str]: + """Map the two CLI flags onto BatchRunConfig's template fields. + + ``None`` is omitted rather than forwarded, so an unpassed flag leaves the + model default (today's layout) authoritative in exactly one place. + """ + overrides: dict[str, str] = {} + if logging_dir is not None: + overrides["logging_dir_template"] = logging_dir + if artifacts_dir is not None: + overrides["artifacts_dir_template"] = artifacts_dir + return overrides + + def run_pipeline( *, grade: bool, @@ -454,6 +494,8 @@ def run_pipeline( set_overrides: list[str], format: str | None = None, workspace_dir: Path | None = None, + logging_dir: str | None = None, + artifacts_dir: str | None = None, ) -> None: """The shared body of ``coder-eval run`` and ``coder-eval execute``. @@ -474,6 +516,16 @@ def run_pipeline( # resumed workspace-dir run would never recognize its own prior result. if resume and workspace_dir is not None: raise typer.BadParameter("--resume is not supported together with --workspace-dir.") + # clear_rerun_artifacts rmtree's whatever artifacts_dir_template resolves to for a + # re-running task. The default template is always a directory the harness itself + # created (run_dir/.../artifacts/), which is what makes that safe -- an + # operator-supplied --artifacts-dir points at a pre-existing tree the harness did + # not create and has no way to prove it owns. + if resume and artifacts_dir is not None: + raise typer.BadParameter( + "--resume is not supported together with --artifacts-dir -- clearing a re-running " + + "task's stale artifacts would rmtree an operator-supplied tree the harness didn't create." + ) # Without --resume this flag parsed, was accepted, and did nothing at all. Its # sibling mode-scoped flag (`evaluate --workspace`) hard-errors on exactly this. if allow_host_grading and not resume: @@ -539,6 +591,8 @@ def run_pipeline( grade=grade, format=format, workspace_dir=workspace_dir, + logging_dir=logging_dir, + artifacts_dir=artifacts_dir, ) ) except KeyboardInterrupt: @@ -568,6 +622,8 @@ async def _run_all_tasks( grade: bool = True, format: str | None = None, workspace_dir: Path | None = None, + logging_dir: str | None = None, + artifacts_dir: str | None = None, ) -> None: """Async entry point for running all tasks (optionally in parallel). @@ -590,6 +646,7 @@ async def _run_all_tasks( promotes it to `/trajectory.json` when the run wrote exactly one (a multi-task run is left nested; there is nothing to promote) workspace_dir: Run the agent here instead of run_dir/artifacts + logging_dir / artifacts_dir: path templates for the run layout (see run_command options) """ # Prepare run directory run_dir = prepare_run_directory(run_dir) @@ -617,6 +674,7 @@ async def _run_all_tasks( include_skipped=include_skipped, grade=grade, workspace_dir=workspace_dir, + **_dir_template_overrides(logging_dir, artifacts_dir), ) from ..telemetry import flush_telemetry, track_event @@ -657,20 +715,32 @@ async def _run_all_tasks( ) # Aggregate task logs into run.log + + # Over the resolved logging dirs, not a run_dir walk: an overridden + # logging_dir_template need not live under run_dir, and the walk would + # silently aggregate nothing (empty experiment.log, no error). + # summary.task_results rows are plain dicts (RunSummary persists them + # that way), carrying the same variant/task/replicate the logging dir was + # built from. + task_dirs = [ + config.resolve_logging_dir( + row.get("variant_id") or "default", + row["task_id"], + row.get("replicate_index") or 0, + ) + for row in summary.task_results + if row.get("task_id") + ] + from ..logging_config import aggregate_task_logs - aggregate_task_logs(run_dir) + aggregate_task_logs(run_dir, task_dirs=task_dirs) if format == "harbor": from ..harbor.atif_emit import emit_trajectories_for_run - written = emit_trajectories_for_run(run_dir) - console.print(f"[dim]Wrote {len(written)} trajectory.json (ATIF) file(s) under {run_dir}[/dim]") - - flat_trajectory_path = run_dir / "trajectory.json" - if len(written) == 1 and written[0] != flat_trajectory_path: - await asyncio.to_thread(shutil.copy2, written[0], flat_trajectory_path) - console.print(f"[dim]Copied the single trajectory to {flat_trajectory_path}[/dim]") + written = emit_trajectories_for_run(task_dirs) + console.print(f"[dim]Wrote {len(written)} trajectory.json (ATIF) file(s)[/dim]") # Print execution summary print_execution_summary(run_dir, summary) @@ -764,7 +834,7 @@ def _unreadable_row_placeholder(rt: ResolvedTask, error: Exception) -> Evaluatio async def _grade_resumed_tasks( - to_grade: list[ResolvedTask], *, allow_host_grading: bool = False + to_grade: list[ResolvedTask], *, config: BatchRunConfig, allow_host_grading: bool = False ) -> list[tuple[ResolvedTask, TaskResult]]: """Grade the rows ``coder-eval execute`` left NOT_GRADED, in place. @@ -799,7 +869,15 @@ async def _grade_resumed_tasks( prior = load_prior_result(rt.run_dir) # The reference check lives inside regrade_in_place, so a caller # cannot forget it. - workspace = default_workspace(rt.run_dir, prior) + # The same template the run wrote with, so a workspace the template + # placed OUTSIDE run_dir is a trusted root rather than a containment + # failure -- which is what previously made --resume unusable alongside + # an overridden artifacts dir. + workspace = default_workspace( + rt.run_dir, + prior, + artifacts_dir=config.resolve_artifacts_dir(rt.variant_id, rt.task.task_id, rt.replicate_index), + ) # Preserve the ungraded record BEFORE the orchestrator overwrites # task.json in this same directory. back_up_pre_grade_record(rt.run_dir) @@ -860,7 +938,7 @@ async def _grade_resumed_tasks( async def _apply_resume( - resolved: list[ResolvedTask], *, grade: bool, allow_host_grading: bool + resolved: list[ResolvedTask], *, config: BatchRunConfig, grade: bool, allow_host_grading: bool ) -> tuple[list[ResolvedTask], list[TaskResult], list[ResolvedTask]]: """Split a resumed run into what still needs running, and what is carried in. @@ -879,7 +957,7 @@ async def _apply_resume( # Leftover artifacts from a partial run could let a file-based criterion pass on # the old output. to_grade is deliberately NOT cleared: its artifacts are what is # being graded. - cleared = clear_rerun_artifacts(part.to_run) + cleared = clear_rerun_artifacts(part.to_run, config=config) # Checked explicitly rather than left to regrade_in_place's own per-row guard, # so the whole batch is refused up front instead of one row at a time. # Rationale: .claude/notes/orchestration.md § Refusing a criteria-free task under grade @@ -892,7 +970,7 @@ async def _apply_resume( ) # Reusing the trajectory and workspace already on disk rather than paying for # the agent twice. Folded in as prior_results so the summary covers them. - for rt, tr in await _grade_resumed_tasks(part.to_grade, allow_host_grading=allow_host_grading): + for rt, tr in await _grade_resumed_tasks(part.to_grade, config=config, allow_host_grading=allow_host_grading): prior_results.append(tr) prior_resolved.append(rt) return part.to_run, prior_results, prior_resolved @@ -1051,7 +1129,7 @@ async def _run_with_experiment( prior_resolved: list[ResolvedTask] = [] if resume: to_run, prior_results, prior_resolved = await _apply_resume( - resolved, grade=grade, allow_host_grading=allow_host_grading + resolved, config=config, grade=grade, allow_host_grading=allow_host_grading ) # Against `to_run`, not `resolved`: an already-finalized row is never re-graded, diff --git a/src/coder_eval/harbor/agent.py b/src/coder_eval/harbor/agent.py index 40eb59b2..ba35efce 100644 --- a/src/coder_eval/harbor/agent.py +++ b/src/coder_eval/harbor/agent.py @@ -45,6 +45,13 @@ __version__ = "0.0.0" +# Run-level bookkeeping goes here and is discarded with the container. Anywhere +# outside the WORKDIR would do; /tmp is the one path guaranteed writable in every +# task image. NOT a bare "/tmp": a dedicated subdirectory keeps run.json/run.md/ +# experiment.* from littering a directory tasks themselves use. +_THROWAWAY_RUN_DIR = "/tmp/coder-eval-run" # nosec B108 -- predictable path is fine: single-use, single-tenant container, no other process/user shares /tmp to race or symlink-plant it + + class CoderEvalAgent(BaseInstalledAgent): """Runs coder-eval's own agent loop as a Harbor agent. @@ -76,20 +83,22 @@ async def install(self, environment: BaseEnvironment) -> None: async def run(self, instruction: str, environment: BaseEnvironment, context: AgentContext) -> None: """Run ``coder-eval execute --format harbor`` inside the environment. - ``instruction`` is NOT forwarded: the agent-phase task.yaml already carries - the identical resolved prompt. Token and cost totals are filled in afterward - by ``populate_context_post_run``. - - ``--workspace-dir "$(pwd)"`` is load-bearing — without it the tempdir sandbox - writes the agent's workspace somewhere Harbor's verifier never looks. + ``instruction`` is NOT forwarded: the agent-phase task.yaml already carries the + identical resolved prompt. Token and cost totals are filled in afterward by + ``populate_context_post_run``. ``--workspace-dir "$(pwd)"`` is load-bearing -- + without it the tempdir sandbox writes the agent's workspace somewhere Harbor's + verifier never looks. ``--logging-dir``/``--artifacts-dir`` are static paths, not + templates; ``--run-dir`` is a throwaway path outside the workspace. - Rationale: .claude/notes/reporting.md § The non-obvious constraint in the emitted task.yaml + Rationale: .claude/notes/persistence.md § CoderEvalAgent passes both templates as static paths """ del instruction, context # nothing to forward; context is populated post-run run_dir = self.environment_logs_dir.as_posix() command = ( f"coder-eval execute {shlex.quote(AGENT_TASK_YAML_PATH)} --format harbor " - f'--run-dir {shlex.quote(run_dir)} --workspace-dir "$(pwd)"' + f"--run-dir {_THROWAWAY_RUN_DIR} " + f'--workspace-dir "$(pwd)" ' + f'--logging-dir {shlex.quote(run_dir)} --artifacts-dir "$(pwd)"' ) await self._exec(environment, command) diff --git a/src/coder_eval/harbor/atif_emit.py b/src/coder_eval/harbor/atif_emit.py index 51183524..1bc2a8b6 100644 --- a/src/coder_eval/harbor/atif_emit.py +++ b/src/coder_eval/harbor/atif_emit.py @@ -17,6 +17,7 @@ from __future__ import annotations import logging +from collections.abc import Iterable from pathlib import Path from typing import Any @@ -385,21 +386,24 @@ def write_trajectory_json(result: EvaluationResult, path: Path) -> Path | None: return None -def emit_trajectories_for_run(run_dir: Path) -> list[Path]: - """Write a ``trajectory.json`` sibling for every ``task.json`` under ``run_dir``. +def emit_trajectories_for_run(task_dirs: Iterable[Path]) -> list[Path]: + """Write a ``trajectory.json`` sibling for every ``task.json`` in ``task_dirs``. - The ``--format harbor`` post-pass for ``coder-eval execute``: ATIF emission - is opt-in (unlike the old always-on design this module's predecessor - shipped), so a plain ``run``/``execute`` never gains a new output file. - Walks the run directory rather than hooking the orchestrator's finalize - path, keeping this package's "translate coder-eval's own artifacts" - scope (see ``coder_eval.harbor``'s module docstring) — it needs no access - to orchestrator internals, only the ``task.json`` files a run already - wrote. Per-task failures are logged and skipped (see + ``task_dirs`` are the per-task logging directories, resolved from the same + ``logging_dir_template`` the run wrote ``task.json`` with -- taken as an argument + rather than discovered by walking a run directory, since an overridden logging dir + need not live under ``run_dir`` at all. The ``--format harbor`` post-pass for + ``coder-eval execute``; opt-in, so a plain ``run``/``execute`` never gains a new + output file. Per-task failures are logged and skipped (see :func:`write_trajectory_json`), never aborting the rest of the run's export. + + Rationale: .claude/notes/persistence.md § What run_dir still owns """ written: list[Path] = [] - for task_json in sorted(run_dir.glob(f"**/{TASK_JSON_FILENAME}")): + for task_json in sorted({d / TASK_JSON_FILENAME for d in task_dirs}): + if not task_json.is_file(): + logger.warning("No %s at %s — skipping ATIF emission", TASK_JSON_FILENAME, task_json) + continue try: result = EvaluationResult.model_validate_json(task_json.read_text(encoding="utf-8")) except (OSError, ValueError): diff --git a/src/coder_eval/harbor/packager.py b/src/coder_eval/harbor/packager.py index e93a0ef1..fe87b4a5 100644 --- a/src/coder_eval/harbor/packager.py +++ b/src/coder_eval/harbor/packager.py @@ -31,7 +31,7 @@ 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.models import CONTAINER_WORK_DIR, TaskDefinition, TemplateDirSource from coder_eval.orchestration.task_loader import load_task from coder_eval.path_utils import REFERENCE_COPY_IGNORE, ignore_patterns_and_symlinks @@ -62,21 +62,26 @@ set -u # `/tests/task.yaml /logs/agent` -- an explicit task file over a RUN DIRECTORY, -# not a plain workdir. CoderEvalAgent's `coder-eval execute --run-dir /logs/agent -# ...` always finishes with `/logs/agent/task.json` (this task's own recorded -# trajectory) and `/logs/agent/artifacts//` (the workspace it produced), -# so `coder-eval evaluate` recognizes /logs/agent as a run directory and grades -# against it directly -- no `$(pwd)` guess of the agent's WORKDIR needed (the -# workspace is located from task.json's own recorded sandbox_path instead), and -# no ATIF trajectory.json round-trip either (task.json already carries the same -# trajectory natively). Passing the task file explicitly (rather than the bare -# run directory alone) makes coder-eval grade with THIS file -- the exported -# contract -- instead of rebuilding the task from the run's own recorded config, -# which is also what keeps this off the untrusted-recorded-config path: that -# path exists for a shared run directory whose config is not to be trusted -# without --allow-recorded-commands, and does not apply once an explicit, -# operator-supplied task file is in hand. -coder-eval evaluate /tests/task.yaml /logs/agent --in-place --run-dir /logs/verifier || true +# not a plain workdir. CoderEvalAgent's `coder-eval execute --logging-dir +# /logs/agent --workspace-dir "$(pwd)" --artifacts-dir "$(pwd)"` (see +# harbor/agent.py) always finishes with `/logs/agent/task.json` (this task's +# own recorded trajectory) and the agent's workspace left in-place at the +# container's own WORKDIR ($(pwd) here too -- Harbor's verifier phase reuses +# the same image/WORKDIR) -- artifacts_dir IS the workspace here, so +# capture_as's self-referential guard makes the would-be copy a no-op. +# `--workspace "$(pwd)"` is therefore load-bearing: without it, `coder-eval +# evaluate` resolves the workspace from task.json's recorded sandbox_path via +# `default_workspace`, which requires it to resolve INSIDE /logs/agent and +# refuses otherwise -- exactly this WORKDIR case, which is legitimately +# outside /logs/agent. No ATIF trajectory.json round-trip either (task.json +# already carries the same trajectory natively). Passing the task file +# explicitly (rather than the bare run directory alone) makes coder-eval grade +# with THIS file -- the exported contract -- instead of rebuilding the task +# from the run's own recorded config, which is also what keeps this off the +# untrusted-recorded-config path: that path exists for a shared run directory +# whose config is not to be trusted without --allow-recorded-commands, and +# does not apply once an explicit, operator-supplied task file is in hand. +coder-eval evaluate /tests/task.yaml /logs/agent --workspace "$(pwd)" --in-place --run-dir /logs/verifier || true coder-eval harbor reward /logs/verifier --out /logs/verifier/reward.json """ @@ -624,6 +629,8 @@ def _write_task_toml(task: TaskDefinition, out_dir: Path, *, workdir: str | None doc["verifier"] = verifier_section if task.run_limits is not None and task.run_limits.task_timeout is not None: doc["agent"] = {"timeout_sec": float(task.run_limits.task_timeout)} + # Rationale: .claude/notes/reporting.md § The artifacts default is CONTAINER_WORK_DIR, not a required field + doc["artifacts"] = [workdir or CONTAINER_WORK_DIR] (out_dir / "task.toml").write_bytes(tomli_w.dumps(doc).encode("utf-8")) diff --git a/src/coder_eval/isolation/docker_runner.py b/src/coder_eval/isolation/docker_runner.py index 7f8b19d7..58134768 100644 --- a/src/coder_eval/isolation/docker_runner.py +++ b/src/coder_eval/isolation/docker_runner.py @@ -381,9 +381,7 @@ def _assert_workspace_not_reserved(path: str) -> None: """ norm = path.rstrip("/") or "/" if norm in RESERVED_CONTAINER_DIRS or norm.startswith(CONTAINER_WORK_DIR + "/"): - raise DockerRunError( - f"working_dir {path!r} collides with a framework-reserved container path (/, /work, /work/*)." - ) + raise DockerRunError(f"working_dir {path!r} collides with a framework-reserved container path (/, /work/*).") def _resolve_workspace_dir(cfg_working_dir: str | None, image: str) -> str | None: diff --git a/src/coder_eval/logging_config.py b/src/coder_eval/logging_config.py index 41d5fad0..cdf34f74 100644 --- a/src/coder_eval/logging_config.py +++ b/src/coder_eval/logging_config.py @@ -13,7 +13,7 @@ import sys import threading from collections import deque -from collections.abc import Generator +from collections.abc import Generator, Iterable from contextlib import contextmanager from contextvars import ContextVar, Token from dataclasses import dataclass @@ -339,7 +339,7 @@ def task_log_handler( tail_buffer.close() -def aggregate_task_logs(run_dir: Path) -> None: +def aggregate_task_logs(run_dir: Path, *, task_dirs: Iterable[Path] | None = None) -> None: """Aggregate all task logs into a single experiment.log file. This function should be called after all tasks have completed. @@ -347,7 +347,11 @@ def aggregate_task_logs(run_dir: Path) -> None: with clear separators and metadata. Args: - run_dir: Path to run directory containing task subdirectories + run_dir: Path to run directory; ``experiment.log`` is written here. + task_dirs: The per-task logging directories, resolved from + ``logging_dir_template``. Required whenever that template may place them + outside ``run_dir`` -- the ``run_dir`` glob fallback would then find + nothing and write an empty log with no error. Example: >>> # After running tasks @@ -355,7 +359,11 @@ def aggregate_task_logs(run_dir: Path) -> None: >>> # Creates: runs/2025-10-16_14-25-18/experiment.log """ run_log_path = run_dir / "experiment.log" - task_log_paths = sorted(run_dir.glob(f"**/{TASK_LOG_FILENAME}")) + if task_dirs is not None: + task_log_paths = sorted({d / TASK_LOG_FILENAME for d in task_dirs}) + task_log_paths = [p for p in task_log_paths if p.is_file()] + else: + task_log_paths = sorted(run_dir.glob(f"**/{TASK_LOG_FILENAME}")) if not task_log_paths: # No task logs found - create empty experiment.log @@ -372,7 +380,12 @@ def aggregate_task_logs(run_dir: Path) -> None: # Relative to run_dir, so a dataset-fanned task id renders with full # context rather than just its leaf. as_posix() keeps the header # consistent across platforms. - task_id = task_log_file.parent.relative_to(run_dir).as_posix() + # A logging dir outside run_dir has no relative form; fall back to the + # absolute path rather than raising ValueError mid-aggregation. + try: + task_id = task_log_file.parent.relative_to(run_dir).as_posix() + except ValueError: + task_id = task_log_file.parent.as_posix() outfile.write(f"\n{'=' * 80}\n") outfile.write(f"TASK: {task_id}\n") outfile.write(f"{'=' * 80}\n\n") diff --git a/src/coder_eval/orchestration/batch.py b/src/coder_eval/orchestration/batch.py index 4d49ee1a..2087d036 100644 --- a/src/coder_eval/orchestration/batch.py +++ b/src/coder_eval/orchestration/batch.py @@ -28,7 +28,13 @@ TaskDefinition, TaskResult, ) -from ..path_utils import TASK_JSON_FILENAME, format_task_log_id, write_text_atomic +from ..path_utils import ( + DEFAULT_ARTIFACTS_DIR_TEMPLATE, + DEFAULT_LOGGING_DIR_TEMPLATE, + TASK_JSON_FILENAME, + format_task_log_id, + write_text_atomic, +) from ..pricing import unpriced_models from ..run_record import eval_result_to_task_dict from ..streaming.callbacks import StreamCallback @@ -130,6 +136,38 @@ async def run_batch( + "aligns automatically via sandbox.docker.working_dir (see DockerRunner)." ) + if config.artifacts_dir_template != DEFAULT_ARTIFACTS_DIR_TEMPLATE and any( + rt.task.sandbox.driver == "docker" for rt in resolved_tasks + ): + raise ValueError( + "--artifacts-dir is not for sandbox.driver: docker tasks -- the in-container Orchestrator " + + "has no way to receive it (see models/container_context.py); the container's own WORKDIR " + + "is always what gets captured back." + ) + + if config.logging_dir_template != DEFAULT_LOGGING_DIR_TEMPLATE and any( + rt.task.sandbox.driver == "docker" for rt in resolved_tasks + ): + raise ValueError( + "--logging-dir is not for sandbox.driver: docker tasks -- the container's artifacts land under " + + "the resolved logging dir (DockerRunner mounts it as the container's /work/output), so " + + "clear_rerun_artifacts and --artifacts-dir's default template -- both anchored on config.run_dir " + + "-- would target a different directory than the one the container actually wrote to." + ) + + if len(resolved_tasks) > 1: + for flag, resolver in ( + ("--logging-dir", config.resolve_logging_dir), + ("--artifacts-dir", config.resolve_artifacts_dir), + ): + resolved_dirs = [resolver(rt.variant_id, rt.task.task_id, rt.replicate_index) for rt in resolved_tasks] + if len(set(resolved_dirs)) != len(resolved_dirs): + raise ValueError( + f"{flag}'s template resolves two or more of this run's {len(resolved_tasks)} tasks to " + + "the same directory, so they would overwrite each other's task.json/artifacts. Use " + + "${variant}/${task}/${repeat} to keep multiple tasks apart." + ) + check_pricing_coverage(resolved_tasks) if on_batch_start is not None: @@ -148,15 +186,8 @@ async def run_single(rt: ResolvedTask) -> TaskResult: task_callback = stream_callback_factory(stream_label) if stream_callback_factory else None async with semaphore: try: - # --workspace-dir mode (Harbor CoderEvalAgent, single task): write - # task.json/task.html/task.log/artifacts flat at the top-level run_dir - # instead of the usual // nesting. The guard above - # already guarantees exactly one resolved task here, so that nesting only - # exists to disambiguate sibling tasks that can never occur in this mode — - # and a flat run_dir means trajectory.json (written by - # emit_trajectories_for_run as task.json's sibling) lands at a fixed, - # predictable path instead of requiring a recursive glob to find it. - effective_run_dir = config.run_dir if config.workspace_dir is not None else rt.run_dir + # Rationale: .claude/notes/persistence.md § A flat run_dir needs no special case in run_batch + effective_run_dir = rt.run_dir effective_run_dir.mkdir(parents=True, exist_ok=True) # noqa: CE002 — mkdir on local FS is nanoseconds sandbox_cfg = rt.task.sandbox # HERE, where the original driver is still visible: the @@ -206,6 +237,7 @@ async def run_single(rt: ResolvedTask) -> TaskResult: replicate_index=rt.replicate_index, grade=config.grade, workspace_dir=config.workspace_dir, + artifacts_dir=config.resolve_artifacts_dir(rt.variant_id, rt.task.task_id, rt.replicate_index), ) result = await orchestrator.run() tr = TaskResult( @@ -418,8 +450,11 @@ def partition_for_resume(resolved_tasks: list[ResolvedTask], *, grade: bool = Tr return ResumePartition(to_run, to_grade, prior_results, prior_resolved) -def clear_rerun_artifacts(to_run: list[ResolvedTask]) -> int: - """Remove stale ``artifacts/`` dirs for tasks about to re-run under --resume. +def clear_rerun_artifacts(to_run: list[ResolvedTask], *, config: BatchRunConfig) -> int: + """Remove stale artifacts dirs for tasks about to re-run under --resume. + + ``config`` supplies ``resolve_artifacts_dir``, so this clears the SAME path the + run will write to even when ``artifacts_dir_template`` was overridden. A task in ``to_run`` is non-finalized (``partition_for_resume`` excluded every finalized task), so it re-executes from scratch and any leftover artifacts are @@ -432,7 +467,11 @@ def clear_rerun_artifacts(to_run: list[ResolvedTask]) -> int: """ cleared = 0 for rt in to_run: - artifacts = rt.run_dir / "artifacts" / rt.task.task_id + # Through the config chokepoint, NOT a hand-built run_dir/artifacts/: + # an overridden template puts artifacts somewhere else entirely, and clearing + # the wrong path would leave a partial run's output in place for a file-based + # criterion to pass on. + artifacts = config.resolve_artifacts_dir(rt.variant_id, rt.task.task_id, rt.replicate_index) if artifacts.exists(): shutil.rmtree(artifacts, ignore_errors=True) cleared += 1 diff --git a/src/coder_eval/orchestration/config.py b/src/coder_eval/orchestration/config.py index a3cfb9d4..28dac180 100644 --- a/src/coder_eval/orchestration/config.py +++ b/src/coder_eval/orchestration/config.py @@ -1,11 +1,34 @@ """Configuration models for orchestration.""" from pathlib import Path +from string import Template from typing import Any -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, field_validator from coder_eval.models import PreservationMode +from coder_eval.path_utils import ( + DEFAULT_ARTIFACTS_DIR_TEMPLATE, + DEFAULT_LOGGING_DIR_TEMPLATE, + resolve_dir_template, +) + + +_VALID_DIR_TEMPLATE_PLACEHOLDERS = frozenset({"run_dir", "variant", "task", "repeat"}) + + +def _check_dir_template(v: str) -> str: + """Reject an unknown ``${...}`` placeholder at config-construction time. + + ``resolve_dir_template`` catches this too, but only when a task is actually + resolved against it -- late enough that a typo becomes N mislabelled ERROR + rows instead of one clean ``typer.BadParameter`` at CLI parse time. + """ + unknown = set(Template(v).get_identifiers()) - _VALID_DIR_TEMPLATE_PLACEHOLDERS + if unknown: + valid = ", ".join(f"${{{p}}}" for p in sorted(_VALID_DIR_TEMPLATE_PLACEHOLDERS)) + raise ValueError(f"{v!r} references unknown placeholder(s) {sorted(unknown)}. Valid placeholders: {valid}.") + return v def resolve_preservation_mode(explicit: PreservationMode | None, driver: str) -> PreservationMode: @@ -112,12 +135,64 @@ class BatchRunConfig(BaseModel): default=None, description=( "Run the agent in-place at this absolute path instead of the standard " - "run_dir/artifacts workspace, copying it out to run_dir/artifacts/ at " + "artifacts workspace named by artifacts_dir_template, copying it out there at " "cleanup. For a single task only. Not for sandbox.driver: docker tasks — " "the docker driver already aligns automatically via sandbox.docker.working_dir." ), ) + # The run's on-disk layout, as two independent templates resolved LATE (per + # task, where ${variant}/${task}/${repeat} first exist). Defaults reproduce + # today's layout byte-for-byte; a static override needs no special-casing + # because substituting a string with no placeholders is the identity function. + logging_dir_template: str = Field( + default=DEFAULT_LOGGING_DIR_TEMPLATE, + description=( + "Where task.json/task.log go. Placeholders: ${run_dir}, ${variant}, ${task}, " + "${repeat}. A static path (e.g. /logs/agent) resolves to itself." + ), + ) + artifacts_dir_template: str = Field( + default=DEFAULT_ARTIFACTS_DIR_TEMPLATE, + description=( + "Where the agent's artifacts go -- the FINAL directory, not a parent. Same " + "placeholders as logging_dir_template, and independent of it: the two may live in " + "unrelated parts of the filesystem (Harbor puts logs at /logs/agent and artifacts " + "at the container's WORKDIR). When it already holds the workspace there is nothing " + "to copy." + ), + ) + + @field_validator("logging_dir_template", "artifacts_dir_template") + @classmethod + def _validate_dir_template(cls, v: str) -> str: + return _check_dir_template(v) + + def resolve_logging_dir(self, variant_id: str, task_id: str, replicate_index: int = 0) -> Path: + """This task's logging directory, per ``logging_dir_template``.""" + return resolve_dir_template( + self.logging_dir_template, + run_dir=self.run_dir, + variant_id=variant_id, + task_id=task_id, + replicate_index=replicate_index, + ) + + def resolve_artifacts_dir(self, variant_id: str, task_id: str, replicate_index: int = 0) -> Path: + """This task's FINAL artifacts directory, per ``artifacts_dir_template``. + + One chokepoint for every consumer -- the orchestrator's capture/direct-write + target, ``--resume``'s stale-artifact clearing, and the regrade workspace + lookup -- so they cannot disagree about where a task's artifacts live. + """ + return resolve_dir_template( + self.artifacts_dir_template, + run_dir=self.run_dir, + variant_id=variant_id, + task_id=task_id, + replicate_index=replicate_index, + ) + # TODO(container-death-diagnostics): containers run uncapped today, so at a # high --max-parallel one runaway task can pressure the host. An opt-in # default is already expressible through the layered sandbox config; a diff --git a/src/coder_eval/orchestration/experiment.py b/src/coder_eval/orchestration/experiment.py index fb8a583f..cbc10026 100644 --- a/src/coder_eval/orchestration/experiment.py +++ b/src/coder_eval/orchestration/experiment.py @@ -38,7 +38,7 @@ VariantResult, apply_prompt_mutations, ) -from ..path_utils import build_task_run_dir +from ..path_utils import resolve_dir_template from .config import BatchRunConfig from .config_merge import ConfigSource, Layer, merge_layers, resolve_root from .task_loader import ( @@ -682,10 +682,11 @@ def resolve_all_tasks( ResolvedTask( task=resolved_task, task_file=task_file, - run_dir=build_task_run_dir( - config.run_dir, - variant.variant_id, - resolved_task.task_id, + run_dir=resolve_dir_template( + config.logging_dir_template, + run_dir=config.run_dir, + variant_id=variant.variant_id, + task_id=resolved_task.task_id, replicate_index=rep, ), variant_id=variant.variant_id, diff --git a/src/coder_eval/orchestration/regrade.py b/src/coder_eval/orchestration/regrade.py index 996176ce..b80f2067 100644 --- a/src/coder_eval/orchestration/regrade.py +++ b/src/coder_eval/orchestration/regrade.py @@ -389,32 +389,37 @@ def _fall_back_to_source( return task, source_yaml -def default_workspace(run_dir: Path, prior: EvaluationResult) -> Path: +def default_workspace(run_dir: Path, prior: EvaluationResult, *, artifacts_dir: Path | None = None) -> Path: """Locate the workspace a finished run left behind. - ``sandbox_path`` is authoritative when it still exists; otherwise the - preserved artifacts tree, where preservation nests the workspace under the - task id. - - RAISES rather than guessing when neither is conclusive — grading the WRONG - directory makes every path-relative criterion fail as a locating artifact and - reports that as an ordinary score. - - **Every** return goes through ``_contained``, checked against ``run_dir``. + Precedence: ``artifacts_dir`` (the caller's resolved ``artifacts_dir_template``, an + operator-supplied second TRUSTED ROOT), then the recorded ``sandbox_path``, then the + preserved artifacts tree. RAISES rather than guessing when none is conclusive, and + every return is contained within ``run_dir``/``artifacts_dir`` — never relaxed, only + widened. Rationale: .claude/notes/orchestration.md § Locating the workspace a finished run left behind """ + roots = [run_dir] if artifacts_dir is None else [run_dir, artifacts_dir] def _contained(candidate: Path, description: str) -> Path: - # One chokepoint, one root. `run_dir` is the operator-supplied path; a - # candidate is only ever derived from the untrusted record. - if not _is_within(candidate, run_dir): + # One chokepoint. Roots are operator-supplied paths; a candidate is only + # ever derived from the untrusted record. + if not any(_is_within(candidate, root) for root in roots): raise RegradeError( - f"{description} resolves outside the run directory ({run_dir}). " - + "Pass --workspace explicitly to grade a directory outside the run." + f"{description} resolves outside the run directory ({run_dir})" + + (f" and the artifacts directory ({artifacts_dir})" if artifacts_dir else "") + + ". Pass --workspace explicitly to grade a directory outside the run " + + "(needed when the run used a logging/artifacts dir template that " + + "placed the workspace outside run_dir)." ) return candidate + if artifacts_dir is not None and artifacts_dir.is_dir(): + # Already the final path -- NOT nested by task_id the way the run_dir + # fallback below is, because the template resolved that in. + return _contained(artifacts_dir, f"The resolved artifacts_dir ({artifacts_dir})") + if prior.sandbox_path: recorded = Path(prior.sandbox_path) if recorded.is_dir(): diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index b96bba42..e1909e64 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -368,6 +368,7 @@ def __init__( config_lineage: dict[str, ConfigLineageEntry] | None = None, replicate_index: int = 0, workspace_dir: Path | None = None, + artifacts_dir: Path | None = None, grade: bool = True, prior_result: EvaluationResult | None = None, recorded_task: TaskDefinition | None = None, @@ -391,6 +392,11 @@ def __init__( workspace_dir: Docker WORKDIR alignment — the agent runs in-place there and the workspace is copied out at cleanup. Takes precedence over ``preservation_mode``. + artifacts_dir: The FINAL directory this task's artifacts belong in, + already resolved from a directory template by the caller (see + ``path_utils.resolve_dir_template``). ``None`` keeps the built-in + ``run_dir/artifacts/`` layout. When it already holds the + workspace, the copy self-cancels -- no flag needed. grade: False is ``coder-eval execute``. Deliberately NOT a task-config field — a task YAML must never declare itself ungraded. prior_result: A completed run's result to re-grade. @@ -410,6 +416,7 @@ def __init__( self._cost_attempt_nonce = uuid.uuid4().hex self.preservation_mode = preservation_mode self.workspace_dir = workspace_dir + self.artifacts_dir = artifacts_dir self.task_file = task_file self.stream_callback = stream_callback self.sandbox = sandbox @@ -1460,6 +1467,18 @@ def _arm_early_stop(self) -> None: + "the full trajectory is the deliverable." ) + def _final_artifacts_dir(self) -> Path: + """This task's FINAL artifacts directory. + + ``artifacts_dir`` when the caller resolved one from a directory template, + otherwise the built-in ``run_dir/artifacts/`` layout -- which is + exactly what ``DEFAULT_ARTIFACTS_DIR_TEMPLATE`` resolves to, so the two + agree by construction. + """ + if self.artifacts_dir is not None: + return self.artifacts_dir + return self.run_dir / "artifacts" / self.task.task_id + def _restore_recorded_command_path(self) -> None: """Re-apply the graded run's own PATH before its criteria run. @@ -1531,7 +1550,7 @@ async def _setup(self) -> None: ) direct_target = self.workspace_dir elif self.preservation_mode == PreservationMode.DIRECT_WRITE: - direct_target = self.run_dir / "artifacts" / self.task.task_id + direct_target = self._final_artifacts_dir() else: direct_target = None # DIRECT_WRITE deliberately does NOT clear the target, so a reused @@ -3026,17 +3045,20 @@ async def _cleanup(self) -> None: if self.sandbox: try: if self.workspace_dir is not None and self.result: - # Docker WORKDIR alignment: the agent ran in-place at the - # image WORKDIR, so copy that workspace out. Takes precedence - # over preservation_mode. - artifacts_dir = self.run_dir / "artifacts" - preserved_path = await asyncio.to_thread(self.sandbox.capture_to, artifacts_dir) + # Docker WORKDIR alignment: the agent ran in-place at the image + # WORKDIR, which is NOT where artifacts belong -- so copy it to + # the resolved artifacts dir. Takes precedence over + # preservation_mode. If that directory IS the workspace (a + # caller-supplied artifacts template pointing at the WORKDIR, as + # Harbor does), capture_as's self-referential guard makes this a + # no-op instead of a duplicating copy -- which is why no + # "should I copy?" flag exists. + preserved_path = await asyncio.to_thread(self.sandbox.capture_as, self._final_artifacts_dir()) self.result.sandbox_path = str(preserved_path) logger.info("Workspace captured out: %s -> %s", self.workspace_dir, preserved_path) elif self.preservation_mode == PreservationMode.MOVE_ON_WRITE and self.result: - # Sandbox ran in a tempdir — move it into run_dir/artifacts. - artifacts_dir = self.run_dir / "artifacts" - preserved_path = await asyncio.to_thread(self.sandbox.preserve_to, artifacts_dir) + # Sandbox ran in a tempdir — move it to the resolved artifacts dir. + preserved_path = await asyncio.to_thread(self.sandbox.preserve_as, self._final_artifacts_dir()) self.result.sandbox_path = str(preserved_path) logger.info(f"Sandbox preserved to: {preserved_path}") elif self.preservation_mode == PreservationMode.DIRECT_WRITE and self.result: diff --git a/src/coder_eval/path_utils.py b/src/coder_eval/path_utils.py index 1324d4ae..103e3b82 100644 --- a/src/coder_eval/path_utils.py +++ b/src/coder_eval/path_utils.py @@ -10,6 +10,7 @@ from collections.abc import Callable from datetime import datetime from pathlib import Path +from string import Template logger = logging.getLogger(__name__) @@ -175,14 +176,82 @@ def replicate_subdir_name(replicate_index: int) -> str: return f"{replicate_index:02d}" +# The run's on-disk layout, as data rather than as path joins spread across the +# orchestrator, resolved LATE per task. Both spelled out in full (not artifacts +# relative to logging) since under Harbor they live in unrelated parts of the +# filesystem, with no shared prefix to factor out. +# Rationale: .claude/notes/persistence.md § The run layout as two directory templates +DEFAULT_LOGGING_DIR_TEMPLATE = "${run_dir}/${variant}/${task}/${repeat}" +DEFAULT_ARTIFACTS_DIR_TEMPLATE = "${run_dir}/${variant}/${task}/${repeat}/artifacts/${task}" + +_DIR_TEMPLATE_PLACEHOLDERS = ("run_dir", "variant", "task", "repeat") + + +def resolve_dir_template( + template: str, + *, + run_dir: Path, + variant_id: str, + task_id: str, + replicate_index: int = 0, +) -> Path: + """Resolve a logging/artifacts directory template for ONE task. + + ``${task}`` may legitimately expand to a value CONTAINING a separator: a + dataset-expanded task_id is ``"/"`` (see + ``task_loader.expand_dataset``, whose row ids are validated as safe directory + names precisely because they become directories). That nests, which is the + pre-existing behaviour and works on Windows too -- ``pathlib`` splits on both + separators there, so a forward slash inside a substituted value is a + separator, not a literal character in a filename. + + Windows also dictates HOW the substitution happens: ``string.Template`` + inserts values verbatim, whereas ``re.sub`` would interpret backslashes in + the REPLACEMENT as escapes -- turning a ``run_dir`` of ``C:\\runs\\2026`` into + mangled output. Never swap this for a regex. + + Raises: + ValueError: the template references an unknown placeholder. + """ + mapping = { + "run_dir": str(run_dir), + "variant": variant_id, + "task": task_id, + "repeat": replicate_subdir_name(replicate_index), + } + try: + resolved = Template(template).substitute(mapping) + except KeyError as e: + raise ValueError( + f"Directory template {template!r} references unknown placeholder {e.args[0]!r}. " + + f"Valid placeholders: {', '.join('${' + p + '}' for p in _DIR_TEMPLATE_PLACEHOLDERS)}." + ) from e + except ValueError as e: + raise ValueError(f"Directory template {template!r} is malformed: {e}") from e + # Path() normalizes the mixed separators a Windows run_dir produces + # ("C:\\runs\\x" + "/default/...") into a single native form. + return Path(resolved) + + def build_task_run_dir( run_dir: Path, variant_id: str, task_id: str, replicate_index: int = 0, ) -> Path: - """Build the per-task run dir: ``////``.""" - return run_dir / variant_id / task_id / replicate_subdir_name(replicate_index) + """Build the per-task run dir: ``////``. + + Thin wrapper over ``resolve_dir_template`` with the default logging template, + kept so the many existing callers that want the standard layout need not + restate it. The two agree by construction. + """ + return resolve_dir_template( + DEFAULT_LOGGING_DIR_TEMPLATE, + run_dir=run_dir, + variant_id=variant_id, + task_id=task_id, + replicate_index=replicate_index, + ) def format_task_log_id(variant_id: str, task_id: str, replicate_index: int = 0) -> str: diff --git a/src/coder_eval/sandbox.py b/src/coder_eval/sandbox.py index 8772eeea..9c456d34 100644 --- a/src/coder_eval/sandbox.py +++ b/src/coder_eval/sandbox.py @@ -1469,11 +1469,21 @@ def preserve_to(self, artifact_dir: Path) -> Path: Raises: RuntimeError: If sandbox is not set up. """ + return self.preserve_as(artifact_dir / self.task_id) + + def preserve_as(self, preserve_path: Path) -> Path: + """``preserve_to``, but naming the FINAL directory rather than its parent. + + Exists because a caller-supplied artifacts directory (resolved from a + directory template) IS the final path -- appending ``task_id`` to it + again would re-nest a directory the caller deliberately placed. + ``preserve_to`` is the parent-relative wrapper for everyone who wants the + default layout. + """ if not self.sandbox_dir: raise RuntimeError("Sandbox not set up") # task_id may contain "/" (dataset row tasks); ensure the parent exists. - preserve_path = artifact_dir / self.task_id preserve_path.parent.mkdir(parents=True, exist_ok=True) # Guard against self-referential move (sandbox already at target). @@ -1524,11 +1534,19 @@ def capture_to(self, artifact_dir: Path) -> Path: Rationale: .claude/notes/isolation.md § preserve_to, capture_to, and the capture denylist """ + return self.capture_as(artifact_dir / self.task_id) + + def capture_as(self, preserve_path: Path) -> Path: + """``capture_to``, but naming the FINAL directory rather than its parent. + + Same rationale as :meth:`preserve_as`. The self-referential guard below + is what makes a caller-supplied artifacts directory that ALREADY holds + the workspace a no-op rather than a duplicating copy. + """ if not self.sandbox_dir: raise RuntimeError("Sandbox not set up") # task_id may contain "/" (dataset row tasks); ensure the parent exists. - preserve_path = artifact_dir / self.task_id preserve_path.parent.mkdir(parents=True, exist_ok=True) # Guard against a self-referential copy (workspace already at target). diff --git a/tests/_fixtures/harbor_export_golden/expected/task.toml b/tests/_fixtures/harbor_export_golden/expected/task.toml index 9c559e2c..f0b36846 100644 --- a/tests/_fixtures/harbor_export_golden/expected/task.toml +++ b/tests/_fixtures/harbor_export_golden/expected/task.toml @@ -1,4 +1,7 @@ schema_version = "1.4" +artifacts = [ + "/work", +] [task] name = "coder-eval/golden_export_example" diff --git a/tests/_fixtures/harbor_export_golden/expected/tests/test.sh b/tests/_fixtures/harbor_export_golden/expected/tests/test.sh index 6168226d..2ac6686b 100755 --- a/tests/_fixtures/harbor_export_golden/expected/tests/test.sh +++ b/tests/_fixtures/harbor_export_golden/expected/tests/test.sh @@ -14,19 +14,24 @@ set -u # `/tests/task.yaml /logs/agent` -- an explicit task file over a RUN DIRECTORY, -# not a plain workdir. CoderEvalAgent's `coder-eval execute --run-dir /logs/agent -# ...` always finishes with `/logs/agent/task.json` (this task's own recorded -# trajectory) and `/logs/agent/artifacts//` (the workspace it produced), -# so `coder-eval evaluate` recognizes /logs/agent as a run directory and grades -# against it directly -- no `$(pwd)` guess of the agent's WORKDIR needed (the -# workspace is located from task.json's own recorded sandbox_path instead), and -# no ATIF trajectory.json round-trip either (task.json already carries the same -# trajectory natively). Passing the task file explicitly (rather than the bare -# run directory alone) makes coder-eval grade with THIS file -- the exported -# contract -- instead of rebuilding the task from the run's own recorded config, -# which is also what keeps this off the untrusted-recorded-config path: that -# path exists for a shared run directory whose config is not to be trusted -# without --allow-recorded-commands, and does not apply once an explicit, -# operator-supplied task file is in hand. -coder-eval evaluate /tests/task.yaml /logs/agent --in-place --run-dir /logs/verifier || true +# not a plain workdir. CoderEvalAgent's `coder-eval execute --logging-dir +# /logs/agent --workspace-dir "$(pwd)" --artifacts-dir "$(pwd)"` (see +# harbor/agent.py) always finishes with `/logs/agent/task.json` (this task's +# own recorded trajectory) and the agent's workspace left in-place at the +# container's own WORKDIR ($(pwd) here too -- Harbor's verifier phase reuses +# the same image/WORKDIR) -- artifacts_dir IS the workspace here, so +# capture_as's self-referential guard makes the would-be copy a no-op. +# `--workspace "$(pwd)"` is therefore load-bearing: without it, `coder-eval +# evaluate` resolves the workspace from task.json's recorded sandbox_path via +# `default_workspace`, which requires it to resolve INSIDE /logs/agent and +# refuses otherwise -- exactly this WORKDIR case, which is legitimately +# outside /logs/agent. No ATIF trajectory.json round-trip either (task.json +# already carries the same trajectory natively). Passing the task file +# explicitly (rather than the bare run directory alone) makes coder-eval grade +# with THIS file -- the exported contract -- instead of rebuilding the task +# from the run's own recorded config, which is also what keeps this off the +# untrusted-recorded-config path: that path exists for a shared run directory +# whose config is not to be trusted without --allow-recorded-commands, and +# does not apply once an explicit, operator-supplied task file is in hand. +coder-eval evaluate /tests/task.yaml /logs/agent --workspace "$(pwd)" --in-place --run-dir /logs/verifier || true coder-eval harbor reward /logs/verifier --out /logs/verifier/reward.json diff --git a/tests/test_atif_emit.py b/tests/test_atif_emit.py index a0b60769..ed0208f5 100644 --- a/tests/test_atif_emit.py +++ b/tests/test_atif_emit.py @@ -357,6 +357,58 @@ def test_converter_exception_swallowed(self, tmp_path, monkeypatch): assert not path.exists() +class TestEmitTrajectoriesForRun: + def test_writes_trajectory_for_dirs_outside_run_dir(self, tmp_path): + """task_dirs need not live under a common run_dir -- an overridden + logging_dir_template (Harbor's own agent logs dir) is exactly the case this + exists for.""" + result = _result([_turn(messages=[_assistant()])]) + dir_a = tmp_path / "elsewhere" / "a" + dir_b = tmp_path / "somewhere_else" / "b" + dir_a.mkdir(parents=True) + dir_b.mkdir(parents=True) + (dir_a / "task.json").write_text(result.model_dump_json(), encoding="utf-8") + (dir_b / "task.json").write_text(result.model_dump_json(), encoding="utf-8") + + written = atif_emit.emit_trajectories_for_run([dir_a, dir_b]) + + assert sorted(written) == sorted([dir_a / "trajectory.json", dir_b / "trajectory.json"]) + for d in (dir_a, dir_b): + parsed = Trajectory.model_validate(json.loads((d / "trajectory.json").read_text(encoding="utf-8"))) + assert parsed.session_id == "atif_emit_test/default" + + def test_dedupes_repeated_task_dirs(self, tmp_path): + result = _result([_turn(messages=[_assistant()])]) + task_dir = tmp_path / "task" + task_dir.mkdir() + (task_dir / "task.json").write_text(result.model_dump_json(), encoding="utf-8") + + written = atif_emit.emit_trajectories_for_run([task_dir, task_dir, task_dir]) + + assert written == [task_dir / "trajectory.json"] + + def test_missing_task_json_is_skipped_and_logged(self, tmp_path, caplog): + missing_dir = tmp_path / "never_wrote" + missing_dir.mkdir() + + with caplog.at_level("WARNING"): + written = atif_emit.emit_trajectories_for_run([missing_dir]) + + assert written == [] + assert "No task.json" in caplog.text + + def test_unreadable_task_json_is_skipped_and_logged(self, tmp_path, caplog): + bad_dir = tmp_path / "bad" + bad_dir.mkdir() + (bad_dir / "task.json").write_text("not valid json", encoding="utf-8") + + with caplog.at_level("WARNING"): + written = atif_emit.emit_trajectories_for_run([bad_dir]) + + assert written == [] + assert "Could not read" in caplog.text + + class TestAtomicWriteText: def test_writes_and_leaves_no_tmp(self, tmp_path): path = tmp_path / "out.json" diff --git a/tests/test_cleanup_preservation_guard.py b/tests/test_cleanup_preservation_guard.py index b2519cd8..abeb90a3 100644 --- a/tests/test_cleanup_preservation_guard.py +++ b/tests/test_cleanup_preservation_guard.py @@ -63,7 +63,7 @@ async def test_preserve_failure_does_not_skip_cleanup(tmp_path) -> None: orchestrator = _make_orchestrator(tmp_path) orchestrator.preservation_mode = PreservationMode.MOVE_ON_WRITE mock_sandbox = MagicMock() - mock_sandbox.preserve_to = MagicMock(side_effect=OSError("No space left on device")) + mock_sandbox.preserve_as = MagicMock(side_effect=OSError("No space left on device")) orchestrator.sandbox = mock_sandbox await orchestrator._cleanup() @@ -78,7 +78,7 @@ async def test_preserve_success_sets_path_and_cleanup_runs(tmp_path) -> None: orchestrator.preservation_mode = PreservationMode.MOVE_ON_WRITE preserved = tmp_path / "run" / "cleanup_guard_test" / "artifacts" mock_sandbox = MagicMock() - mock_sandbox.preserve_to = MagicMock(return_value=preserved) + mock_sandbox.preserve_as = MagicMock(return_value=preserved) orchestrator.sandbox = mock_sandbox await orchestrator._cleanup() @@ -159,17 +159,17 @@ async def test_workspace_dir_captures_out_even_with_preservation_none(tmp_path) orchestrator.workspace_dir = Path("/root") captured = tmp_path / "run" / "cleanup_guard_test" / "artifacts" / "cleanup_guard_test" mock_sandbox = MagicMock() - mock_sandbox.capture_to = MagicMock(return_value=captured) + mock_sandbox.capture_as = MagicMock(return_value=captured) orchestrator.sandbox = mock_sandbox await orchestrator._cleanup() # workspace_dir wins: capture_to is invoked with the artifacts dir and its # returned path is recorded (NONE would otherwise have nulled sandbox_path). - mock_sandbox.capture_to.assert_called_once_with(orchestrator.run_dir / "artifacts") + mock_sandbox.capture_as.assert_called_once_with(orchestrator.run_dir / "artifacts" / "cleanup_guard_test") assert orchestrator.result.sandbox_path == str(captured) # The NONE / MOVE_ON_WRITE arms must not run when workspace_dir is set. - mock_sandbox.preserve_to.assert_not_called() + mock_sandbox.preserve_as.assert_not_called() mock_sandbox.cleanup.assert_called_once_with(preserve=False) @@ -181,17 +181,65 @@ async def test_workspace_capture_failure_with_none_does_not_skip_cleanup(tmp_pat orchestrator.preservation_mode = PreservationMode.NONE orchestrator.workspace_dir = Path("/root") mock_sandbox = MagicMock() - mock_sandbox.capture_to = MagicMock(side_effect=OSError("No space left on device")) + mock_sandbox.capture_as = MagicMock(side_effect=OSError("No space left on device")) orchestrator.sandbox = mock_sandbox await orchestrator._cleanup() # must not raise - mock_sandbox.capture_to.assert_called_once_with(orchestrator.run_dir / "artifacts") + mock_sandbox.capture_as.assert_called_once_with(orchestrator.run_dir / "artifacts" / "cleanup_guard_test") # capture_to raised before assigning the path, so it stays at its default (None). assert orchestrator.result.sandbox_path is None mock_sandbox.cleanup.assert_called_once_with(preserve=False) +@pytest.mark.asyncio +async def test_artifacts_dir_overrides_the_capture_destination(tmp_path) -> None: + """A caller-supplied artifacts dir (resolved from a directory template) IS the + final destination -- capture_as receives it verbatim, with no run_dir nesting and + no task_id appended. + + This is the Harbor case: it points the artifacts template at the container's own + WORKDIR, which is also where the agent ran. No "should I copy?" flag is involved + -- when destination == workspace, Sandbox.capture_as's self-referential guard + (covered in test_sandbox.py) turns the copy into a no-op.""" + orchestrator = _make_orchestrator(tmp_path) + workspace = tmp_path / "workdir" + workspace.mkdir() + orchestrator.workspace_dir = workspace + orchestrator.artifacts_dir = workspace + mock_sandbox = MagicMock() + mock_sandbox.capture_as = MagicMock(return_value=workspace) + orchestrator.sandbox = mock_sandbox + + await orchestrator._cleanup() + + mock_sandbox.capture_as.assert_called_once_with(workspace) + assert orchestrator.result.sandbox_path == str(workspace) + mock_sandbox.cleanup.assert_called_once_with(preserve=False) + + +@pytest.mark.asyncio +async def test_no_artifacts_dir_keeps_the_builtin_layout(tmp_path) -> None: + """artifacts_dir=None (the in-container docker WORKDIR-alignment path, which + passes no template) must resolve to the historical run_dir/artifacts/ + -- byte-identical to what preserve_to/capture_to appended before.""" + orchestrator = _make_orchestrator(tmp_path) + orchestrator.workspace_dir = Path("/root") + orchestrator.artifacts_dir = None + captured = orchestrator.run_dir / "artifacts" / "cleanup_guard_test" + mock_sandbox = MagicMock() + mock_sandbox.capture_as = MagicMock(return_value=captured) + orchestrator.sandbox = mock_sandbox + + await orchestrator._cleanup() + + mock_sandbox.capture_as.assert_called_once_with(captured) + assert orchestrator.result.sandbox_path == str(captured) + # The NONE / MOVE_ON_WRITE arms must not run when workspace_dir is set. + mock_sandbox.preserve_as.assert_not_called() + mock_sandbox.cleanup.assert_called_once_with(preserve=False) + + @pytest.mark.asyncio async def test_none_without_workspace_dir_discards_path(tmp_path) -> None: """Contrast case: NONE with no workspace_dir takes the discard arm — no @@ -209,7 +257,7 @@ async def test_none_without_workspace_dir_discards_path(tmp_path) -> None: await orchestrator._cleanup() - mock_sandbox.capture_to.assert_not_called() - mock_sandbox.preserve_to.assert_not_called() + mock_sandbox.capture_as.assert_not_called() + mock_sandbox.preserve_as.assert_not_called() assert orchestrator.result.sandbox_path is None mock_sandbox.cleanup.assert_called_once_with(preserve=False) diff --git a/tests/test_cli_telemetry.py b/tests/test_cli_telemetry.py index 06f49cad..031be98d 100644 --- a/tests/test_cli_telemetry.py +++ b/tests/test_cli_telemetry.py @@ -79,7 +79,7 @@ def test_help_never_crashes_when_telemetry_enabled_and_config_unwritable(tmp_pat async def test_run_emits_run_start_and_flushes(tmp_path): - summary = Mock(tasks_failed=0, tasks_error=0, tasks_not_graded=0) + summary = Mock(tasks_failed=0, tasks_error=0, tasks_not_graded=0, task_results=[]) with ( patch("coder_eval.cli.run_command.prepare_run_directory", return_value=tmp_path), @@ -115,7 +115,7 @@ async def test_run_emits_run_start_and_flushes(tmp_path): async def test_run_start_uses_default_fallbacks_for_none_inputs(tmp_path): # agent_type=None / stream_mode=None must surface as the "default"/"none" # fallback property values, not as null. - summary = Mock(tasks_failed=0, tasks_error=0, tasks_not_graded=0) + summary = Mock(tasks_failed=0, tasks_error=0, tasks_not_graded=0, task_results=[]) with ( patch("coder_eval.cli.run_command.prepare_run_directory", return_value=tmp_path), diff --git a/tests/test_detached_grading_boundaries.py b/tests/test_detached_grading_boundaries.py index 3b049ab6..804971e5 100644 --- a/tests/test_detached_grading_boundaries.py +++ b/tests/test_detached_grading_boundaries.py @@ -479,6 +479,7 @@ async def test_the_resume_error_arm_restores_and_folds_back_ungraded(self, tmp_p """The arm a real grading crash takes: `regrade_in_place` RETURNS an ERROR result instead of raising.""" from coder_eval.cli.run_command import _grade_resumed_tasks + from coder_eval.orchestration.config import BatchRunConfig # task.json starts UNGRADED, as `execute` left it. The ERROR lands on # disk during the grade, exactly as _finalize_result writes it before @@ -503,7 +504,7 @@ async def _crash(**_kwargs) -> EvaluationResult: patch("coder_eval.orchestration.regrade.default_workspace", return_value=tmp_path), patch("coder_eval.orchestration.regrade.regrade_in_place", new=_crash), ): - graded = await _grade_resumed_tasks([rt]) + graded = await _grade_resumed_tasks([rt], config=BatchRunConfig(run_dir=tmp_path)) assert len(graded) == 1 folded = graded[0][1].result @@ -520,6 +521,7 @@ async def test_an_unreadable_row_still_appears_as_ungraded(self, tmp_path: Path) which is the counter the exit gate reads — so a resume whose rows were all unreadable reported success.""" from coder_eval.cli.run_command import _grade_resumed_tasks + from coder_eval.orchestration.config import BatchRunConfig run_dir = tmp_path / "00" run_dir.mkdir(parents=True) @@ -532,7 +534,7 @@ async def test_an_unreadable_row_still_appears_as_ungraded(self, tmp_path: Path) original_task_id="t", ) - graded = await _grade_resumed_tasks([rt]) + graded = await _grade_resumed_tasks([rt], config=BatchRunConfig(run_dir=tmp_path)) assert len(graded) == 1 assert graded[0][1].result.final_status is FinalStatus.NOT_GRADED diff --git a/tests/test_harbor_agent.py b/tests/test_harbor_agent.py index dff56ff1..de33f516 100644 --- a/tests/test_harbor_agent.py +++ b/tests/test_harbor_agent.py @@ -62,11 +62,17 @@ def _make_agent(module, *, logs_dir: Path): class TestRunCommandConstruction: - async def test_run_shells_out_to_execute_with_workspace_dir_and_run_dir(self, coder_eval_agent_module, tmp_path): + async def test_run_shells_out_to_execute_with_static_dir_templates(self, coder_eval_agent_module, tmp_path): """`--workspace-dir "$(pwd)"` is Gap 2's real fix: without it the agent's tempdir sandbox writes outside the container's WORKDIR, where Harbor's - verifier phase looks. `--run-dir` must point at `environment_logs_dir` - (bind-mounted from Harbor's `self.logs_dir` on the host).""" + verifier phase looks. + + `--logging-dir` must point at `environment_logs_dir` (bind-mounted from + Harbor's `self.logs_dir` on the host) -- that is where task.json/task.log and + the promoted trajectory.json this class reads back land. `--artifacts-dir` + names the WORKDIR, so artifacts are NOT a child of the logging dir and + nothing is copied. There is deliberately no `--run-dir`: both templates are + static, so `${run_dir}` is never substituted.""" agent = _make_agent(coder_eval_agent_module, logs_dir=tmp_path) captured: dict[str, object] = {} @@ -85,8 +91,17 @@ async def _fake_exec(environment, command): assert isinstance(command, str) assert command.startswith("coder-eval execute ") assert "--format harbor" in command - assert f"--run-dir {tmp_path.as_posix()}" in command + assert f"--logging-dir {tmp_path.as_posix()}" in command + assert '--artifacts-dir "$(pwd)"' in command assert '--workspace-dir "$(pwd)"' in command + # --run-dir must be passed even though the templates are static: run-level + # bookkeeping follows it and its default is CWD-relative, so omitting it + # writes runs// into the agent's workspace (cwd == WORKDIR) and + # pollutes the collected artifacts. Regression-pinned from a live trial. + # It points at a throwaway tmp dir, NOT the logs dir: Harbor has its own + # trial reporting, so run-level files would only clutter what it syncs back. + assert "--run-dir /tmp/coder-eval-run" in command + assert f"--run-dir {tmp_path.as_posix()}" not in command class TestPopulateContextPostRun: diff --git a/tests/test_harbor_packager.py b/tests/test_harbor_packager.py index eb9b8c66..cb383aca 100644 --- a/tests/test_harbor_packager.py +++ b/tests/test_harbor_packager.py @@ -122,10 +122,11 @@ def test_instruction_md_is_a_fixed_placeholder_not_the_real_prompt(self, tmp_pat def test_test_sh_is_executable_and_grades_the_run_directory(self, tmp_path: Path) -> None: """test.sh must be workdir-agnostic: it grades `/logs/agent` as a run - directory (its own recorded sandbox_path locates the workspace), not a - `$(pwd)` guess 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`).""" + directory with `--workspace "$(pwd)"` naming the actual workspace -- + CoderEvalAgent's `--artifacts-dir "$(pwd)"` (harbor/agent.py) makes the + workspace IS the artifacts dir, so capture_as's self-referential guard + never copies it under /logs/agent/artifacts/, so `--workspace` must + point there explicitly (see packager.py's `_write_environment`).""" task_file = _write_task(tmp_path) out_dir = tmp_path / "out" @@ -135,7 +136,11 @@ def test_test_sh_is_executable_and_grades_the_run_directory(self, tmp_path: Path 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 "coder-eval evaluate /tests/task.yaml /logs/agent --in-place --run-dir /logs/verifier" in content + assert ( + 'coder-eval evaluate /tests/task.yaml /logs/agent --workspace "$(pwd)" --in-place ' + + "--run-dir /logs/verifier" + 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: @@ -321,6 +326,11 @@ def test_dockerfile_with_no_workdir_is_left_unset_and_untouched(self, tmp_path: 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"] + # `[environment].workdir` stays unset (the image's own WORKDIR drives + # `docker exec -w`), but the artifacts source still falls back to /work + # so Harbor has something to snapshot -- a wrong source there is only a + # best-effort collection miss, never an exit 127. + assert doc["artifacts"] == ["/work"] def test_dockerfile_with_an_existing_workdir_is_respected_and_not_touched(self, tmp_path: Path) -> None: env_dir = tmp_path / "environment" @@ -355,6 +365,12 @@ def test_dockerfile_with_an_existing_workdir_is_respected_and_not_touched(self, ) doc = tomllib.loads((out_dir / "task.toml").read_text(encoding="utf-8")) assert doc["environment"]["workdir"] == "/workspace" + # Declared as a top-level Harbor artifact so its own collection pass + # (SingleStepTrial._collect_artifacts, before the verifier runs and + # before the container is torn down) snapshots the agent's in-place + # workspace to /artifacts/workspace/ on the host -- otherwise + # nothing the agent wrote is ever visible once the container is gone. + assert doc["artifacts"] == ["/workspace"] def test_docker_working_dir_override_wins_over_the_dockerfile(self, tmp_path: Path) -> None: env_dir = tmp_path / "environment" @@ -481,6 +497,38 @@ def test_explicit_working_dir_is_used_verbatim(self, tmp_path: Path) -> None: doc = tomllib.loads((tmp_path / "out" / "task.toml").read_text(encoding="utf-8")) assert doc["environment"]["workdir"] == "/explicit" + def test_working_dir_rejects_the_mount_root(self, tmp_path: Path) -> None: + """/work is the framework's mount ROOT (input/output/references/task_dir all + live under it) -- it stays reserved even though it's also + coder-eval-agent:latest's declared image WORKDIR, so a task can't put the + agent's graded workspace there and inherit the reference solution.""" + task_file = _write_task(tmp_path, {"sandbox": {"driver": "docker", "docker": {"working_dir": "/work"}}}) + with pytest.raises(ValueError, match="framework-reserved container path"): + export_task(task_file, tmp_path / "out") + + def test_artifacts_defaults_to_container_work_dir(self, tmp_path: Path) -> None: + """No task should have to restate /work: it is the WORKDIR coder-eval's own + image bakes, so the artifacts source defaults to it. `[environment].workdir` + stays UNSET though -- that one drives `docker exec -w` and a wrong guess is + a hard exit 127, whereas a wrong artifacts source is only a collection miss.""" + task_file = _write_task(tmp_path, {"sandbox": {"driver": "docker", "docker": {}}}) + + result = export_task(task_file, tmp_path / "out") + + assert result.workdir is None + doc = tomllib.loads((tmp_path / "out" / "task.toml").read_text(encoding="utf-8")) + assert doc["artifacts"] == ["/work"] + assert "workdir" not in doc["environment"] + + def test_working_dir_overrides_the_default_artifacts_source(self, tmp_path: Path) -> None: + task_file = _write_task(tmp_path, {"sandbox": {"driver": "docker", "docker": {"working_dir": "/app"}}}) + + out_dir = export_task(task_file, tmp_path / "out").out_dir + doc = tomllib.loads((out_dir / "task.toml").read_text(encoding="utf-8")) + + assert doc["artifacts"] == ["/app"] + assert doc["environment"]["workdir"] == "/app" + class TestEnvPassthroughSections: """``task.toml``'s ``[environment.env]``/``[verifier.env]`` -- the SSOT-reuse fix: these diff --git a/tests/test_models.py b/tests/test_models.py index d4c90789..8694bfeb 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -572,6 +572,9 @@ def test_accepts_valid(self, value): assert cfg.docker.working_dir == value assert DockerDriverConfig(working_dir=value).working_dir == value + # /work is REJECTED, not just its children: it is the mount ROOT (input/output/ + # references/task_dir/workspace all live under it), so an agent running AT /work + # would have the reference solution and the run's own task.json in its workspace. @pytest.mark.parametrize("value", ["/", "/work", "/work/", "/work/output", "/work/input", "/work/task_dir"]) def test_rejects_reserved(self, value): from pydantic import ValidationError diff --git a/tests/test_parallel.py b/tests/test_parallel.py index 54f5786b..34b99150 100644 --- a/tests/test_parallel.py +++ b/tests/test_parallel.py @@ -2,6 +2,7 @@ import asyncio import time +from pathlib import Path from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -80,15 +81,16 @@ async def test_sequential_mode(tmp_path): @pytest.mark.asyncio -async def test_workspace_dir_constructs_orchestrator_with_flat_run_dir(tmp_path): - """--workspace-dir mode must pass config.run_dir (flat) to Orchestrator, not the - nested // path a ResolvedTask normally carries. - - Exercises the effective_run_dir branch in run_batch's run_single -- the exact - code path the real Harbor CoderEvalAgent drives via `coder-eval execute - --format harbor --workspace-dir "$(pwd)"`. Patches Orchestrator itself (rather - than driving a full run) so the assertion is directly on the value this branch - computes, independent of what a real run happens to persist to disk. +async def test_orchestrator_run_dir_is_the_resolved_logging_dir(tmp_path): + """Orchestrator's run_dir is whatever logging_dir_template resolved to for this + task -- ResolvedTask.run_dir, verbatim. + + There is deliberately NO workspace_dir special case here any more. "Flat" used to + be a mode this seam detected (effective_run_dir = config.run_dir when + workspace_dir was set, so Harbor's task.json landed at a predictable path); it is + now simply what a static logging template resolves to, which the companion test + below pins. Patches Orchestrator itself (rather than driving a full run) so the + assertion is on the value this seam computes. """ task = TaskDefinition( task_id="test_workspace_dir", @@ -128,11 +130,205 @@ async def test_workspace_dir_constructs_orchestrator_with_flat_run_dir(tmp_path) await run_batch([resolved_task], config) assert mock_orchestrator_cls.call_count == 1 - assert mock_orchestrator_cls.call_args.kwargs["run_dir"] == run_dir, ( - "workspace_dir mode must construct Orchestrator with the flat config.run_dir, not the nested per-task run_dir" + assert mock_orchestrator_cls.call_args.kwargs["run_dir"] == nested_run_dir, ( + "Orchestrator's run_dir must be the task's resolved logging dir, not config.run_dir" ) +@pytest.mark.asyncio +async def test_a_static_logging_template_makes_the_run_dir_flat(tmp_path): + """The replacement for the deleted effective_run_dir special case: Harbor gets a + flat task.json path by RESOLVING a static logging template to it, not by the + orchestration seam noticing workspace_dir and substituting config.run_dir.""" + from coder_eval.path_utils import resolve_dir_template + + assert resolve_dir_template( + "/logs/agent", run_dir=tmp_path / "run", variant_id="default", task_id="t", replicate_index=0 + ) == Path("/logs/agent") + + +@pytest.mark.asyncio +async def test_artifacts_dir_template_resolved_and_threaded_into_orchestrator(tmp_path): + """--artifacts-dir reaches Orchestrator already RESOLVED to a concrete path. + + A static template (no placeholders) resolves to itself -- the identity case that + lets Harbor pass a literal container path with no special-casing anywhere.""" + task = TaskDefinition( + task_id="test_capture_workspace", + description="Test capture_workspace threading", + initial_prompt="Test prompt", + agent={"type": "claude-code"}, + sandbox={"driver": "tempdir"}, + success_criteria=[{"type": "file_exists", "path": "test.txt", "description": "Check for test.txt"}], + ) + task_file = tmp_path / "test_task.yaml" + task_file.write_text("task_id: test_capture_workspace\n") + + run_dir = tmp_path / "run" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + config = BatchRunConfig( + run_dir=run_dir, + max_parallel=1, + preservation_mode=PreservationMode.NONE, + workspace_dir=workspace_dir, + artifacts_dir_template="/work/output", + ) + + resolved_task = ResolvedTask( + task=task, + task_file=task_file, + run_dir=run_dir / "default" / "test_capture_workspace" / "default", + variant_id="default", + original_task_id="test_capture_workspace", + ) + + mock_orchestrator = MagicMock() + mock_orchestrator.run = AsyncMock(return_value=MagicMock(duration_seconds=0.0)) + mock_orchestrator_cls = MagicMock(return_value=mock_orchestrator) + + with patch("coder_eval.orchestrator.Orchestrator", mock_orchestrator_cls): + await run_batch([resolved_task], config) + + assert mock_orchestrator_cls.call_args.kwargs["artifacts_dir"] == Path("/work/output") + + +@pytest.mark.asyncio +async def test_artifacts_dir_template_rejected_for_docker_driver(tmp_path): + """A non-default artifacts_dir_template is a no-op for driver: docker (the in-container + Orchestrator has no way to receive it) -- run_batch refuses it loud rather than silently + ignoring the override, mirroring --workspace-dir's existing docker rejection.""" + task = TaskDefinition( + task_id="test_docker_artifacts", + description="Test artifacts_dir_template + docker rejection", + initial_prompt="Test prompt", + agent={"type": "claude-code"}, + sandbox={"driver": "docker", "docker": {"image": "coder-eval-agent"}}, + success_criteria=[{"type": "file_exists", "path": "test.txt", "description": "Check for test.txt"}], + ) + task_file = tmp_path / "test_task.yaml" + task_file.write_text("task_id: test_docker_artifacts\n") + + run_dir = tmp_path / "run" + config = BatchRunConfig( + run_dir=run_dir, + max_parallel=1, + preservation_mode=PreservationMode.NONE, + artifacts_dir_template="/work/output", + ) + + resolved_task = ResolvedTask( + task=task, + task_file=task_file, + run_dir=run_dir / "default" / "test_docker_artifacts" / "default", + variant_id="default", + original_task_id="test_docker_artifacts", + ) + + with pytest.raises(ValueError, match=r"--artifacts-dir is not for sandbox\.driver: docker"): + await run_batch([resolved_task], config) + + +@pytest.mark.asyncio +async def test_logging_dir_template_rejected_for_docker_driver(tmp_path): + """A non-default logging_dir_template is rejected for driver: docker, mirroring + --artifacts-dir's rejection -- the container's real artifacts land under the + resolved logging dir (DockerRunner's output_dir), so an override desyncs it from + the artifacts_dir_template default that clear_rerun_artifacts/--resume rely on.""" + task = TaskDefinition( + task_id="test_docker_logging", + description="Test logging_dir_template + docker rejection", + initial_prompt="Test prompt", + agent={"type": "claude-code"}, + sandbox={"driver": "docker", "docker": {"image": "coder-eval-agent"}}, + success_criteria=[{"type": "file_exists", "path": "test.txt", "description": "Check for test.txt"}], + ) + task_file = tmp_path / "test_task.yaml" + task_file.write_text("task_id: test_docker_logging\n") + + run_dir = tmp_path / "run" + config = BatchRunConfig( + run_dir=run_dir, + max_parallel=1, + preservation_mode=PreservationMode.NONE, + logging_dir_template="${run_dir}/flat/${task}", + ) + + resolved_task = ResolvedTask( + task=task, + task_file=task_file, + run_dir=run_dir / "flat" / "test_docker_logging", + variant_id="default", + original_task_id="test_docker_logging", + ) + + with pytest.raises(ValueError, match=r"--logging-dir is not for sandbox\.driver: docker"): + await run_batch([resolved_task], config) + + +def _make_resolved_task(tmp_path: Path, task_id: str, run_dir: Path) -> ResolvedTask: + task = TaskDefinition( + task_id=task_id, + description="Test multi-task collision", + initial_prompt="Test prompt", + agent={"type": "claude-code"}, + sandbox={"driver": "tempdir"}, + success_criteria=[{"type": "file_exists", "path": "test.txt", "description": "Check for test.txt"}], + ) + task_file = tmp_path / f"{task_id}.yaml" + task_file.write_text(f"task_id: {task_id}\n") + return ResolvedTask( + task=task, + task_file=task_file, + run_dir=run_dir, + variant_id="default", + original_task_id=task_id, + ) + + +@pytest.mark.asyncio +async def test_static_logging_template_rejected_for_multiple_tasks(tmp_path): + """A logging_dir_template that resolves every task to the SAME directory is refused + for a multi-task run -- each task's task.json/task.log would overwrite the last.""" + run_dir = tmp_path / "run" + config = BatchRunConfig(run_dir=run_dir, max_parallel=1, logging_dir_template="${run_dir}/flat") + tasks = [ + _make_resolved_task(tmp_path, "task_a", run_dir / "flat"), + _make_resolved_task(tmp_path, "task_b", run_dir / "flat"), + ] + + with pytest.raises(ValueError, match=r"--logging-dir's template resolves two or more"): + await run_batch(tasks, config) + + +@pytest.mark.asyncio +async def test_static_artifacts_template_rejected_for_multiple_tasks(tmp_path): + """Same collision guard, for --artifacts-dir: distinct logging dirs but a shared + artifacts_dir_template still collides on the artifacts side.""" + run_dir = tmp_path / "run" + config = BatchRunConfig( + run_dir=run_dir, + max_parallel=1, + artifacts_dir_template="${run_dir}/shared-artifacts", + ) + tasks = [ + _make_resolved_task(tmp_path, "task_a", run_dir / "default" / "task_a" / "00"), + _make_resolved_task(tmp_path, "task_b", run_dir / "default" / "task_b" / "00"), + ] + + with pytest.raises(ValueError, match=r"--artifacts-dir's template resolves two or more"): + await run_batch(tasks, config) + + +@pytest.mark.asyncio +async def test_dir_templates_default_to_todays_layout(tmp_path): + """The defaults must reproduce the historical layout exactly, so an unspecified run + writes to byte-identical paths.""" + cfg = BatchRunConfig(run_dir=tmp_path / "run") + assert cfg.logging_dir_template == "${run_dir}/${variant}/${task}/${repeat}" + assert cfg.artifacts_dir_template == "${run_dir}/${variant}/${task}/${repeat}/artifacts/${task}" + + @pytest.mark.asyncio async def test_semaphore_limits_concurrency(tmp_path): """Test that semaphore actually limits concurrent tasks.""" diff --git a/tests/test_path_utils.py b/tests/test_path_utils.py index c0e8fc25..14064848 100644 --- a/tests/test_path_utils.py +++ b/tests/test_path_utils.py @@ -129,3 +129,177 @@ def test_create_latest_symlink_updates_existing(tmp_path): if platform.system() != "Windows": assert latest.is_symlink() assert latest.resolve() == run_dir2 # Should point to newer run + + +class TestDirTemplates: + """``resolve_dir_template`` — the run layout as data, resolved late. + + Rationale: the logging and artifacts directories are independent, so a caller + (Harbor) can put them in unrelated parts of the filesystem and no copy is + needed between them. + """ + + def test_defaults_reproduce_the_historical_layout(self): + """The whole backward-compatibility claim: an unspecified run must write to + byte-identical paths, so the defaults are pinned against the literal layout + rather than against build_task_run_dir (which now calls the resolver).""" + from coder_eval.path_utils import ( + DEFAULT_ARTIFACTS_DIR_TEMPLATE, + DEFAULT_LOGGING_DIR_TEMPLATE, + resolve_dir_template, + ) + + kwargs = {"run_dir": Path("/runs/2026"), "variant_id": "default", "task_id": "greet"} + assert resolve_dir_template(DEFAULT_LOGGING_DIR_TEMPLATE, **kwargs) == Path("/runs/2026/default/greet/00") + assert resolve_dir_template(DEFAULT_ARTIFACTS_DIR_TEMPLATE, **kwargs) == Path( + "/runs/2026/default/greet/00/artifacts/greet" + ) + + def test_build_task_run_dir_agrees_with_the_default_logging_template(self): + from coder_eval.path_utils import DEFAULT_LOGGING_DIR_TEMPLATE, build_task_run_dir, resolve_dir_template + + for task_id in ("greet", "suite/row-7"): + for rep in (0, 3): + assert build_task_run_dir(Path("/r"), "v", task_id, replicate_index=rep) == resolve_dir_template( + DEFAULT_LOGGING_DIR_TEMPLATE, + run_dir=Path("/r"), + variant_id="v", + task_id=task_id, + replicate_index=rep, + ) + + def test_dataset_task_id_containing_a_separator_nests(self): + """A dataset-expanded task_id is "/" (task_loader.expand_dataset), + and those row ids are validated precisely because they become directories.""" + from coder_eval.path_utils import DEFAULT_ARTIFACTS_DIR_TEMPLATE, resolve_dir_template + + assert resolve_dir_template( + DEFAULT_ARTIFACTS_DIR_TEMPLATE, + run_dir=Path("/runs/2026"), + variant_id="default", + task_id="suite/row-7", + replicate_index=3, + ) == Path("/runs/2026/default/suite/row-7/03/artifacts/suite/row-7") + + @pytest.mark.parametrize("static", ["/work", "/work/output", "/logs/agent"]) + def test_a_static_template_is_the_identity_function(self, static): + """The load-bearing property: an override with no placeholders resolves to + itself down the SAME code path as the default, so nothing anywhere needs an + "is this overridden?" branch.""" + from coder_eval.path_utils import resolve_dir_template + + assert resolve_dir_template( + static, run_dir=Path("/runs/2026"), variant_id="v", task_id="t", replicate_index=5 + ) == Path(static) + + def test_a_windows_run_dir_is_not_mangled_by_backslash_escapes(self): + """HAZARD: ``re.sub`` interprets backslashes in the REPLACEMENT, which would + corrupt a Windows run_dir (``C:\\runs\\2026`` -> ``\\r`` etc.). This is why the + implementation uses ``string.Template``, which inserts values verbatim.""" + from coder_eval.path_utils import resolve_dir_template + + resolved = resolve_dir_template( + "${run_dir}/${task}", run_dir=Path(r"C:\runs\2026"), variant_id="v", task_id="t" + ) + assert "runs" in str(resolved) and "2026" in str(resolved) + assert resolved == Path(r"C:\runs\2026") / "t" + + def test_unknown_placeholder_names_the_valid_ones(self): + from coder_eval.path_utils import resolve_dir_template + + with pytest.raises(ValueError, match=r"unknown placeholder 'nope'"): + resolve_dir_template("${nope}/x", run_dir=Path("/r"), variant_id="v", task_id="t") + + def test_malformed_template_is_a_clean_error(self): + from coder_eval.path_utils import resolve_dir_template + + with pytest.raises(ValueError, match="malformed"): + resolve_dir_template("${run_dir", run_dir=Path("/r"), variant_id="v", task_id="t") + + +def _prior_result(sandbox_path: str): + """A minimal finished-run record, for default_workspace's containment checks.""" + from datetime import datetime + + from coder_eval.models import AgentKind, EvaluationResult + + return EvaluationResult( + task_id="t", + task_description="d", + variant_id="default", + agent_type=AgentKind.CLAUDE_CODE, + started_at=datetime.now(), + final_status="FAILURE", + iteration_count=0, + environment_info={}, + sandbox_path=sandbox_path, + ) + + +class TestDecoupledLayoutConsumers: + """Regressions from decoupling the artifacts dir from run_dir. + + Each of these silently degraded rather than failing loudly, which is why they + are pinned here. + """ + + def test_default_workspace_trusts_an_operator_supplied_artifacts_dir(self, tmp_path): + """`_contained` rejects a recorded sandbox_path outside run_dir -- a real + security guard, since criteria execute with cwd there. An artifacts dir the + OPERATOR resolved from their own template is trusted, so widening the roots + (never relaxing the check) is what makes a decoupled layout regradeable.""" + from coder_eval.orchestration.regrade import default_workspace + + run_dir = tmp_path / "run" + run_dir.mkdir() + artifacts = tmp_path / "elsewhere" / "work" + artifacts.mkdir(parents=True) + prior = _prior_result(str(artifacts)) + + assert default_workspace(run_dir, prior, artifacts_dir=artifacts) == artifacts + + def test_default_workspace_still_refuses_an_untrusted_outside_path(self, tmp_path): + """The guard must not have been weakened: a recorded sandbox_path outside + BOTH roots is still refused. + + The artifacts dir deliberately does NOT exist here, so resolution falls + through to the untrusted sandbox_path -- the path the guard protects. (When + the artifacts dir DOES exist it is returned directly, since an + operator-supplied directory outranks anything the record claims.)""" + from coder_eval.orchestration.regrade import RegradeError, default_workspace + + run_dir = tmp_path / "run" + run_dir.mkdir() + artifacts = tmp_path / "elsewhere" # never created + rogue = tmp_path / "rogue" + rogue.mkdir() + prior = _prior_result(str(rogue)) + + with pytest.raises(RegradeError, match="resolves outside"): + default_workspace(run_dir, prior, artifacts_dir=artifacts) + + def test_aggregate_task_logs_reads_logging_dirs_outside_run_dir(self, tmp_path): + """The run_dir glob found nothing when the logging dir was elsewhere, writing + an empty experiment.log with no error.""" + from coder_eval.logging_config import aggregate_task_logs + + run_dir = tmp_path / "run" + run_dir.mkdir() + logs = tmp_path / "logs" / "agent" + logs.mkdir(parents=True) + (logs / "task.log").write_text("hello from the task\n", encoding="utf-8") + + aggregate_task_logs(run_dir, task_dirs=[logs]) + + aggregated = (run_dir / "experiment.log").read_text(encoding="utf-8") + assert "hello from the task" in aggregated + assert "No task logs found" not in aggregated + + def test_config_resolvers_are_the_single_chokepoint(self, tmp_path): + """Orchestrator capture target, --resume clearing, and the regrade lookup all + go through these, so they cannot disagree about where artifacts live.""" + from coder_eval.orchestration.config import BatchRunConfig + + cfg = BatchRunConfig(run_dir=tmp_path, artifacts_dir_template="/work") + assert cfg.resolve_artifacts_dir("default", "t") == Path("/work") + assert cfg.resolve_logging_dir("default", "t") == tmp_path / "default" / "t" / "00" diff --git a/tests/test_preservation_mode.py b/tests/test_preservation_mode.py index cc5f69b4..0777d781 100644 --- a/tests/test_preservation_mode.py +++ b/tests/test_preservation_mode.py @@ -104,9 +104,15 @@ def test_the_container_has_no_preservation_mode_fallback(): def test_clear_rerun_artifacts_removes_only_existing(tmp_path): - """clear_rerun_artifacts wipes a stale artifacts/ for each re-run task.""" + """clear_rerun_artifacts wipes a stale artifacts dir for each re-run task. + + Paths come from BatchRunConfig's own resolver, so this also pins that clearing + targets the SAME directory the run writes to -- a hand-built + run_dir/artifacts/ would silently miss an overridden template. + """ from coder_eval.models import ResolvedTask, TaskDefinition from coder_eval.orchestration.batch import clear_rerun_artifacts + from coder_eval.orchestration.config import BatchRunConfig def _rt(task_id: str) -> ResolvedTask: task = TaskDefinition( @@ -120,20 +126,22 @@ def _rt(task_id: str) -> ResolvedTask: return ResolvedTask( task=task, task_file=tmp_path / "t.yaml", - run_dir=tmp_path / task_id / "00", + run_dir=tmp_path / "default" / task_id / "00", variant_id="default", original_task_id=task_id, ) + config = BatchRunConfig(run_dir=tmp_path) stale = _rt("stale") - (stale.run_dir / "artifacts" / "stale").mkdir(parents=True) - (stale.run_dir / "artifacts" / "stale" / "leftover.txt").write_text("from killed run") + stale_artifacts = config.resolve_artifacts_dir("default", "stale") + stale_artifacts.mkdir(parents=True) + (stale_artifacts / "leftover.txt").write_text("from killed run") fresh = _rt("fresh") # no artifacts dir - cleared = clear_rerun_artifacts([stale, fresh]) + cleared = clear_rerun_artifacts([stale, fresh], config=config) assert cleared == 1 - assert not (stale.run_dir / "artifacts" / "stale").exists() + assert not stale_artifacts.exists() @pytest.mark.asyncio diff --git a/tests/test_run_command_junit.py b/tests/test_run_command_junit.py index 7f8e5341..52956be5 100644 --- a/tests/test_run_command_junit.py +++ b/tests/test_run_command_junit.py @@ -26,7 +26,7 @@ async def _invoke( status: str, failed: bool, ) -> None: - summary = Mock(tasks_failed=1 if failed else 0, tasks_error=0, tasks_not_graded=0) + summary = Mock(tasks_failed=1 if failed else 0, tasks_error=0, tasks_not_graded=0, task_results=[]) async def _fake(*_args, **_kwargs): # Mirror production: run.json is persisted inside _run_with_experiment.