feat(orchestration): describe the run layout as two independent directory templates - #189
Conversation
…tory templates
Harbor needs coder-eval's per-task bookkeeping (task.json/task.log) in its own
agent logs dir and the agent's artifacts left at the container's WORKDIR. Those
are unrelated parts of the filesystem, but artifacts were structurally a CHILD of
the run dir -- every cleanup arm wrote to `run_dir/"artifacts"` -- so pointing
--run-dir at Harbor's logs dir duplicated the whole workspace underneath it.
Replace that with two independent templates on BatchRunConfig,
`logging_dir_template` and `artifacts_dir_template`, resolved late (per task,
the only point where ${variant}/${task}/${repeat} exist). An override needs no
special-casing: substituting a template with no placeholders is the identity
function, so a static `/work` resolves to itself down the same path as the
default. The defaults spell out the historical layout, so an unspecified run
writes byte-identical paths -- asserted against the literal old layout, not
against build_task_run_dir (which now calls the resolver).
Harbor then passes two static paths and nothing is copied, because the artifacts
destination IS the workspace and capture_as's self-referential guard makes the
copy a no-op. No "should I copy?" flag exists, and `effective_run_dir`'s
workspace_dir special case is gone: "flat" is what a static template resolves to,
not a mode the dispatch seam detects.
Also fixed, each a silent degradation rather than a loud failure once the two
directories could diverge:
- Anything DISCOVERING per-task files by walking run_dir now takes the resolved
logging dirs. emit_trajectories_for_run found no task.json at all (leaving
Harbor's token/cost totals empty, via a debug log); aggregate_task_logs wrote
an empty experiment.log. The latter also raised ValueError from
relative_to(run_dir) for a logging dir outside it.
- default_workspace takes the resolved artifacts dir as a SECOND TRUSTED ROOT.
The roots were widened, never the check relaxed -- roots stay
operator-supplied, candidates stay untrusted -- which is what makes --resume
usable over a decoupled layout instead of failing containment and then finding
no run_dir/artifacts. This removes the --resume/--logging-dir restriction.
- clear_rerun_artifacts went through a hand-built run_dir/artifacts/<task_id>,
so it cleared the wrong path under an override and left a partial run's output
for a file-based criterion to pass on. All four consumers now share
BatchRunConfig.resolve_{logging,artifacts}_dir.
- sandbox.docker.working_dir rejected /work, which is coder-eval-agent's OWN
declared WORKDIR -- the validator refused the shipped image's real working
directory. /work is exempt now; / and the /work/* mount targets still are not.
- The Harbor exporter defaults the task.toml `artifacts` source to /work instead
of requiring every experiment to restate it. [environment].workdir stays unset
on purpose: it drives `docker exec -w`, where a wrong value is a hard exit 127,
whereas a wrong artifacts source is only a best-effort collection miss.
Two implementation constraints, both load-bearing and both tested:
string.Template rather than re.sub (re.sub interprets backslashes in the
REPLACEMENT, mangling a Windows C:\runs\2026), and ${task} may contain a
separator because a dataset-expanded task_id is "<task>/<row>".
Harbor keeps --run-dir despite ${run_dir} never being substituted: run-level
files still follow it and its default is CWD-RELATIVE, so omitting it wrote
runs/<timestamp>/ INTO the agent's workspace (cwd == WORKDIR) and Harbor
collected it. It points at /tmp/coder-eval-run, outside the workspace.
Verified live against real Harbor + Docker: 4/4 trials reward 1.0 with zero
runs/ pollution, zero duplicate workspaces, zero run-level files in the logging
dir, and trajectory.json present in all four.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fixes to the two-directory-template commit surfaced by an 8-axis review: - run_batch now refuses --artifacts-dir under sandbox.driver: docker (the in-container Orchestrator has no way to receive it -- ContainerContext has no artifacts_dir field, so it was silently a no-op) and refuses a static (placeholder-free) --logging-dir/--artifacts-dir on a multi-task run (every task would resolve to the identical directory and collide). - emit_trajectories_for_run now logs a warning on a task_dir with no task.json, matching the sibling unreadable-task.json branch, instead of silently dropping the row's ATIF export. - default_workspace's docstring now states its actual precedence (artifacts_dir over sandbox_path); documented the resulting --resume vs. detached-evaluate asymmetry in orchestration.md. - run_command.py's harbor branch now reuses one resolved task_dirs list for both aggregate_task_logs and emit_trajectories_for_run instead of re-deriving it via a second resolve_dir_template call. - Moved several docstrings/comments that exceeded make docs-budget's caps into .claude/notes/, per CLAUDE.md's comment-budget rule; fixed a leftover half-sentence comment and a stale hardcoded-path description in BatchRunConfig. make verify passes: ruff, pyright, docs-budget, pytest (6468 passed), and the custom lint suite are all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI's bandit step (bandit -r src/ -ll) failed the PR check: _THROWAWAY_RUN_DIR is a static, non-attacker-influenced bookkeeping path scoped to a single- purpose one-container-per-task trust model, not a real hardcoded-tmp-dir race. Annotated with # nosec B108 and a one-line reason, matching the existing comment above it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:189 (31 files) axis:1,2,3,4,5,6,7,8 --post-comment
Scope: pr:189 (31 files) axis:1,2,3,4,5,6,7,8 --post-comment · branch akshaya/harborframework_workdir · 9edaaeca (pr-189 HEAD); local base ed0e2947 · 2026-09-18T01:14Z · workflow variant (8 axes, adversarial verification)
Change class: complex — introduces two independently-resolved directory templates on BatchRunConfig, changes path-resolution control flow across orchestrator/batch/regrade/sandbox/harbor, and adds new validation/rejection branches, so correctness requires reasoning
The two-template run-layout split is a sound architectural idea with clean security posture (9.9) and a well-defended core design (9.5), but it ships as a generalized ${} path mini-language that is then re-restricted by partial guards, and the gaps in those guards are the real risk: an uncontained rmtree of an operator-supplied --artifacts-dir that fires on --resume before any validation, a missing --logging-dir docker guard and a mis-specified dir_template_is_static that both let stale or colliding artifacts change a task's score, and a /work exemption that puts the reference solution inside the graded workspace — so the bottom line is that the feature should not merge until those four score-affecting paths are guarded and tested, after which the remaining unmigrated run_dir walkers and stale docs are ordinary follow-up.
This comment is condensed to fit GitHub's 65k comment limit. Full per-axis reports, every finding's full evidence, and
results.jsonare intmp/code-review-260917-1814/.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 5.9 / 10 | 0 | 3 | 2 | 1 | Single-trajectory promotion at run_command.py:736 is unreachable on its intended single-task path, copies one task's trajectory into another task's … |
| 2. Type Safety | 7.4 / 10 | 0 | 2 | 1 | 1 | New CLI-fed template fields are bare str with no validator, so a malformed --artifacts-dir becomes N mislabelled ERROR rows (or an uncaught … |
| 3. Test Health | 8.4 / 10 | 0 | 1 | 1 | 1 | The new dir-template surface ships untested: the multi-task static-template rejection branch, dir_template_is_static, the clear_rerun_artifacts … |
| 4. Security | 9.9 / 10 | 0 | 0 | 0 | 1 | New # nosec B108 on the hardcoded /tmp Harbor run dir justifies the wrong property (content provenance) rather than the predictable-path risk B108 … |
| 5. Architecture & Design | 9.5 / 10 | 0 | 0 | 1 | 0 | Layout resolution is spread across four sites instead of resolved once onto ResolvedTask, pushing 22 lines of config validation into a function … |
| 6. Error Handling & Resilience | 6.9 / 10 | 1 | 0 | 0 | 1 | clear_rerun_artifacts rmtree's the operator-supplied --artifacts-dir with no containment guard, and runs before run_batch's … |
| 7. API Surface & Maintainability | 8.4 / 10 | 0 | 1 | 1 | 1 | Help strings and inline rationale comments in run_command.py still document the flat --workspace-dir layout / code path this PR deleted |
| 8. Evaluation Harness Quality | 7.5 / 10 | 0 | 2 | 1 | 0 | --logging-dir has no docker guard counterpart, desynchronizing the container's real artifacts path from what clear_rerun_artifacts clears on … |
Overall Score: 8 / 10 · Weakest Axis: Code Quality & Style at 5.9 / 10
Totals: 🔴 1 · 🟠 9 · 🟡 7 · 🔵 6 across 8 axes.
Verification: 55 findings proposed → 43 verified, 6 refuted as false positives and dropped, 34 corrected in place (12 🔵 passed through unverified).
Blockers (🔴 Critical / 🟠 High)
- [Axis 1 · 🟠 High] Single-trajectory promotion at run_command.py:736 is unreachable on its intended single-task path, copies one task's trajectory into another task's logging dir on the surviving path, has a dead
if task_dirs else run_dirfallback, and a stale docstring (src/coder_eval/cli/run_command.py:736) · Cross-axis: flagged by axes 1, 3, 6flat_trajectory_path = (task_dirs[0] if task_dirs else run_dir) / "trajectory.json"/if len(written) == 1 and written[0] != flat_trajectory_path:(lines 736-738).writtenis built byemit_trajectories_for_run(task_dirs)from the SAMEtask_dirslist, so for a single-task runwritten[0]IStask_dirs[0]/"trajectory.json"-- the inequality is always False and the copy never runs. The branch is therefore dead for the one case it exists for (previouslyflat_trajectory_pathwasrun_dir/"trajectory.json", which was always different, so the copy always ran). The only surviving way to enter it is a MULTI-task run where exactly one task produced atask.json(the others crashed): thenwritten[0]belongs to some task N whiletask_dirs[0]is task 0, so task N's trajectory is copied into task 0's logging directory and misattributed. The docstring at line 631 still documents …
- [Axis 1 · 🟠 High] The new logging/artifacts templates leave the remaining run_dir-walking consumers on the old layout — a second source of truth (
src/coder_eval/orchestration/batch.py:514) · Cross-axis: flagged by axes 1, 8- The PR migrated exactly two run_dir walkers to the resolved dirs (
aggregate_task_logs,emit_trajectories_for_run) but left the rest assuming the default layout, so an overridden--logging-dirsilently degrades them:recover_task_resultsat batch.py:514 (for task_json in run_dir.rglob(TASK_JSON_FILENAME):) -- the disk half of the run-summary seam used byrebuild_run_summary, socoder-eval report <run_dir>andevaluaterebuild an EMPTY summary;src/coder_eval/reports/markdown.py:975(build_task_run_dir(run_dir, variant_id, row.task_id, replicate_index=row.replicate_index) / TASK_JSON_FILENAME) -- suite.json/suite.mdtask_json_relpathlinks point at paths that do not exist;src/coder_eval/cli/run_helpers.py:157(for task_log in sorted(run_dir.glob(f"**/{TASK_LOG_FILENAME}")):) -- the end-of-run 'Log Files' listing prints nothing; …
- The PR migrated exactly two run_dir walkers to the resolved dirs (
- [Axis 1 · 🟠 High] A ${} path mini-language plus two guards is disproportionate machinery for the two configurations that actually exist (
src/coder_eval/path_utils.py:190)resolve_dir_template(path_utils.py:190) introduces a user-facing placeholder language (${run_dir}/${variant}/${task}/${repeat}) with unknown-placeholder validation, malformed-template handling, a docstring paragraph defending against are.subimplementation that was never written, plusdir_template_is_static(path_utils.py:236) which exists only to power one rejection branch; two new CLI flags on bothrunandexecute; two newBatchRunConfigfields + two resolver methods; a newOrchestrator.artifacts_dirfield +_final_artifacts_dir(); and two newSandboxmethods. Only two configurations are ever produced in the whole repo: the two defaults (path_utils.py:184-185) and Harbor's two static paths (--logging-dir /logs/agent --artifacts-dir "$(pwd)", harbor/agent.py). The generality is then re-restricted by the guards at batch.py:139-158 and by …
- [Axis 2 · 🟠 High] New CLI-fed template fields are bare
strwith no validator, so a malformed--artifacts-dirbecomes N mislabelled ERROR rows (or an uncaught ValueError under--resume) instead of a cleantyper.BadParameter(src/coder_eval/orchestration/config.py:130) · Cross-axis: flagged by axes 2, 7BatchRunConfigdeclares the two new CLI-fed fields with no constraint beyondstr:logging_dir_template: str = Field( default=DEFAULT_LOGGING_DIR_TEMPLATE,(config.py:130) andartifacts_dir_template: str = Field( default=DEFAULT_ARTIFACTS_DIR_TEMPLATE,(config.py:137).resolve_dir_templateis the only thing that rejects a bad placeholder, and it runs LATE. The two flags then fail asymmetrically: ---logging-dir '${tsak}/x'is resolved inorchestration/experiment.py:685(resolve_all_tasks), outside any handler, so the run aborts with the good message. ---artifacts-dir '${tsak}/x'is resolved atorchestration/batch.py:230:artifacts_dir=config.resolve_artifacts_dir(rt.variant_id, rt.task.task_id, rt.replicate_index),which sits insidetry:(batch.py:178) underexcept Exception as exc:(batch.py:244). Each task is therefore converted by …
- [Axis 2 · 🟠 High]
dir_template_is_statictests for "no placeholders" rather than "no per-task placeholder", so a${run_dir}-only template passes the multi-task collision guard and every task overwrites the previous one (src/coder_eval/path_utils.py:236) · Cross-axis: flagged by axes 1, 2, 6- The predicate is:
def dir_template_is_static(template: str) -> bool: """True whentemplatehas no${...}placeholders, so every task resolves it to the identical path -- fine for a single task (Harbor's use case), a collision for more than one.""" return not Template(template).get_identifiers()(path_utils.py:236-240) Its docstring states the invariant it is meant to encode ("every task resolves it to the identical path"), but the implementation tests a strictly weaker property: any identifier present. Of the four members of_DIR_TEMPLATE_PLACEHOLDERS = ("run_dir", "variant", "task", "repeat")(path_utils.py:187), onlyvariant/task/repeatvary per task —run_dirisconfig.run_dir, constant for the whole run. The tuple is a flat untypedtuple[str, ...]with no run-level/task-level distinction, so nothing makes that difference checkable. Consequence at the only …
- The predicate is:
- [Axis 3 · 🟠 High] The new dir-template surface ships untested: the multi-task static-template rejection branch,
dir_template_is_static, theclear_rerun_artifactsoverride fix on a non-default template, and the--logging-dir/--artifacts-dirflags end-to-end (src/coder_eval/orchestration/batch.py:148) · Cross-axis: flagged by axes 3, 8- This PR adds two new rejection branches; only one got a test. The docker one is pinned by
tests/test_parallel.py:197 test_artifacts_dir_template_rejected_for_docker_driver. The multi-task one is not tested at all:148: if len(resolved_tasks) > 1: 149: for flag, template in ( 150: ("--logging-dir", config.logging_dir_template), 151: ("--artifacts-dir", config.artifacts_dir_template), 152: ): 153: if dir_template_is_static(template): 154: raise ValueError( 155: f"{flag} {template!r} has no placeholders, so every one of this run's "Routed coverage confirms it:orchestration/batch.py 92.28% — missing 128, 149-154, ...— the entire loop AND the raise never execute, meaningrun_batchis never called with more than one resolved task anywhere in the suite. Independently,path_utils.py ... missing 240is the whole body of the new public helper: …
- This PR adds two new rejection branches; only one got a test. The docker one is pinned by
- [Axis 6 · 🔴 Critical]
clear_rerun_artifactsrmtree's the operator-supplied--artifacts-dirwith no containment guard, and runs before run_batch's static-template/docker guards (src/coder_eval/orchestration/batch.py:464) · Cross-axis: flagged by axes 1, 4, 6, 7, 8clear_rerun_artifactsnow deletes whateverartifacts_dir_templateresolves to:artifacts = config.resolve_artifacts_dir(rt.variant_id, rt.task.task_id, rt.replicate_index) if artifacts.exists(): shutil.rmtree(artifacts, ignore_errors=True)Before this PR the path was the hand-builtrt.run_dir / "artifacts" / rt.task.task_id— a directory the harness itself created, which is what makes the docstring's claim safe ("a pre-existing dir here is always a stale DIRECT_WRITE partial", batch.py:452-454). With--artifacts-dirthat claim is false: the flag's own help text and this PR's Harbor precedent (--artifacts-dir "$(pwd)", harbor/agent.py:101) point it at a pre-existing tree the harness did not create. Reproduced against the PR worktree: …
- [Axis 7 · 🟠 High] Help strings and inline rationale comments in run_command.py still document the flat
--workspace-dirlayout / code path this PR deleted (src/coder_eval/cli/run_command.py:368)run_batchdeleted the flat-run_dir special case in this PR (orchestration/batch.py:177, diff:-effective_run_dir = config.run_dir if config.workspace_dir is not None else rt.run_dir/+effective_run_dir = rt.run_dir), and the test that pinned it was inverted (tests/test_parallel.py:test_workspace_dir_constructs_orchestrator_with_flat_run_dir->test_orchestrator_run_dir_is_the_resolved_logging_dir, assertion flipped from== run_dirto== nested_run_dir). The in-repo caller was migrated (harbor/agent.pynow passes--logging-dir), but the two user-facing help strings that document the old contract were not touched: - run_command.py:368 —"standard run_dir/artifacts workspace (copied out to run_dir/artifacts/<task> at "/"cleanup). ..."— the copy target is now<run_dir>/<variant>/<task>/<NN>/artifacts/<task>(DEFAULT_ARTIFACTS_DIR_TEMPLATE). - …
- [Axis 8 · 🟠 High]
--logging-dirhas no docker guard counterpart, desynchronizing the container's real artifacts path from whatclear_rerun_artifactsclears on--resume(src/coder_eval/orchestration/batch.py:139) · Cross-axis: flagged by axes 5, 6, 8- The PR adds the docker guard for only one of the two templates:
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 "The equal-and-opposite guard for--logging-diris missing, and under docker the two templates are not independent.docker_runner.py:639setsoutput_dir = self.rt.run_dir.resolve()and:1307mounts it as-v {output_dir}:/work/output; the in-container Orchestrator getsartifacts_dir=None, so_final_artifacts_dir()returnsrun_dir/"artifacts"/task_id→ hostrt.run_dir/artifacts/<task_id>, i.e. anchored on the resolved LOGGING dir. But the host'sconfig.resolve_artifacts_dir()resolves …
- The PR adds the docker guard for only one of the two templates:
- [Axis 8 · 🟠 High] Exempting
/workfrom RESERVED_CONTAINER_DIRS lets the graded workspace be the framework mount root: capture copies/workinto its own/work/output/artifacts/<task_id>descendant and drags in/work/references(src/coder_eval/models/sandbox.py:300) · Cross-axis: flagged by axes 4, 5, 6, 8
- The validator (and its host-side twin at
isolation/docker_runner.py:384) now exempts/work:if norm in RESERVED_CONTAINER_DIRS - {CONTAINER_WORK_DIR} or norm.startswith(CONTAINER_WORK_DIR + "/"):But/workis the mount ROOT, not a neutral directory.models/container_paths.py:79-92puts every framework mount under it:/work/input(ro),/work/output(the host run dir, mounted read-write — it holds this task'stask.json),/work/task_dir,/work/references(the reference solution),/work/workspace. Withworking_dir: /workthe driver runsdocker run -w /work,_resolve_workspace_dirreturns/work, and the in-container Orchestrator takes theworkspace_dirbranch — so the agent's sandbox, the directory criteria are evaluated against, and the treecapture_ascopies out all become/work. Consequences: the agent can write/work/output/task.jsonand forge …
Non-blocking, but please consider before merge (🟡 Medium)
- [Axis 1 · 🟡 Medium] preserve_to / capture_to have zero production call sites after this PR — dead wrappers whose docstrings claim callers that do not exist (
src/coder_eval/sandbox.py:1456)git grep -n "\.preserve_to(\|\.capture_to(" pr-189 -- src testsreturns only the two internal delegations (sandbox.py:1472return self.preserve_as(artifact_dir / self.task_id), sandbox.py:1537return self.capture_as(artifact_dir / self.task_id)) and seven test call sites. Both orchestrator …
- [Axis 1 · 🟡 Medium] The PR measurably raises cyclomatic complexity in the orchestration/CLI functions it touches (run_batch C(15)->D(21), _run_all_tasks C(15)->C(20)) (
src/coder_eval/orchestration/batch.py:89) · Cross-axis: flagged by axes 1, 7- Measured with
uv run radon cc -son the PR worktree vs the same three files at origin/main:run_batchC(15) -> D(21) (batch.py:89, +6 from the two new guard blocks at lines 139-158),_run_all_tasksC(15) -> C(20) (cli/run_command.py:588, +5 from thetask_dirscomprehension and the reworked …
- Measured with
- [Axis 2 · 🟡 Medium] The default dir templates are hardcoded/hand-copied outside the path_utils SSOT (
build_task_run_dirpins DEFAULT_LOGGING_DIR_TEMPLATE; orchestrator.py keeps its own artifacts layout), so a non-default template yields wrong paths (src/coder_eval/path_utils.py:243) · Cross-axis: flagged by axes 2, 5- The PR made
build_task_run_dira wrapper that hardcodes one template:return resolve_dir_template( DEFAULT_LOGGING_DIR_TEMPLATE, run_dir=run_dir,(path_utils.py:250-252) Its docstring still says it builds "the per-task run dir", which is now only true whenlogging_dir_templateis at its …
- The PR made
- [Axis 3 · 🟡 Medium]
aggregate_task_logs's new production path (explicittask_dirs) is covered by one single-directory test; every multi-task and nested-header test still exercises the now-production-dead glob fallback (logging_config.py:366) (tests/test_path_utils.py:281)run_command.py:722now always passestask_dirs=, so therun_dir.glob(f"**/{TASK_LOG_FILENAME}")fallback inlogging_config.py:365is no longer reached in production. But the suite's substantive aggregation tests still callaggregate_task_logs(tmp_path)with notask_dirs— …
- [Axis 5 · 🟡 Medium] Layout resolution is spread across four sites instead of resolved once onto ResolvedTask, pushing 22 lines of config validation into a function documented as a pure executor and reconstructing logging dirs from serialized dict rows (
src/coder_eval/orchestration/batch.py:102)run_batch's own docstring atbatch.py:101-102states the contract:Tasks must be fully resolved (all config layers applied, tag filtering done). This function is a pure executor — no configuration or loading logic.The PR adds 33 lines of layout configuration validation directly under it …
- [Axis 7 · 🟡 Medium] Generated Harbor
tests/test.shdocuments a nonexistent--no-capture-workspaceflag and a--run-dirvalue the agent no longer passes; the stale text is golden-pinned (src/coder_eval/harbor/packager.py:66) · Cross-axis: flagged by axes 1, 3, 5, 6, 7, 8TEST_SH(shipped verbatim into everycoder-eval export --format harborbundle, golden-pinned attests/_fixtures/harbor_export_golden/expected/tests/test.sh:18,22) now reads: packager.py:65# not a plain workdir. CoderEvalAgent's \coder-eval execute --run-dir /logs/agentpackager.py:66# …
- [Axis 8 · 🟡 Medium] The run-layout SSOT (
.claude/notes/persistence.md) was not updated for a PR that makes the layout configurable, and its enumeration of converted run_dir walkers is incomplete (.claude/notes/persistence.md:72) · Cross-axis: flagged by axes 5, 8- This PR adds
## The run layout as two directory templatesto a notes file (rationale, not contract) while.claude/shared/run-layout.md— which opens with "the factual contract every run-reading command and skill follows. If the run directory structure changes, update it here and every consumer …
- This PR adds
Nits (🔵 Low)
- [Axis 1 · 🔵 Low] Unreachable defensive fallbacks in the new task_dirs comprehension, including a hardcoded "default" variant id (
src/coder_eval/cli/run_command.py:712) - [Axis 2 · 🔵 Low]
_dir_template_overridespasses config field names as dict string keys, so a field rename is invisible to pyright (src/coder_eval/cli/run_command.py:451) - [Axis 3 · 🔵 Low] Two new tests in test_parallel.py duplicate test_path_utils.py coverage and are
async defwith nothing awaited (tests/test_parallel.py:139) - [Axis 4 · 🔵 Low] New
# nosec B108on the hardcoded /tmp Harbor run dir justifies the wrong property (content provenance) rather than the predictable-path risk B108 actually flags (src/coder_eval/harbor/agent.py:52)- CVSS:
CVSS:3.1/AV:L/AC:H/PR:L/UI:N/S:U/C:N/I:L/A:N
- CVSS:
- [Axis 6 · 🔵 Low] aggregate_task_logs drops missing task.logs and writes "No task logs found." with no warning, unlike the sibling it was changed alongside (
src/coder_eval/logging_config.py:364) - [Axis 7 · 🔵 Low]
--artifacts-dirhelp omits its default and both hard rejections that the neighbouring--workspace-dirhelp documents (src/coder_eval/cli/run_command.py:386)
What's Missing
- 🟠 parallel-paths — Only two of the seven per-task discovery sites were converted to the resolved
task_dirs—recover_task_results(batch.py:514),reports/markdown.py:753and:975,cli/run_helpers.py:157andcli/report_command.py:149still walk/rebuildrun_dir, soreport --rebuild/evaluateerror out and command stats, … - 🟠 parallel-paths — The docker path was updated on the host side only:
batch.py:139rejects a non-default--artifacts-dirfordriver: docker, but nothing rejects a non-default--logging-dir, andrun_task_internal_command.py:207still builds the in-container Orchestrator with noartifacts_dir, so the container's real artifacts … - 🟠 parallel-paths —
/workwas exempted in both reserved-path twins (models/sandbox.py:300, docker_runner.py:384) but the capture denylist_WORKSPACE_CAPTURE_IGNORE(sandbox.py:42-67) was not extended with the framework mount names (input,output,references,task_dir,workspace), so aworking_dir: /worktask captures the … - 🟡 parallel-paths —
default_workspacegained anartifacts_dirtrusted root and--resume(run_command.py:872) passes it, but detachedevaluate(cli/evaluate_command.py:169) still calls it two-arg — and since nothing persists the resolved template inrun.json,evaluatecan never recover it, which is why the generated Harbor … - 🟡 parallel-paths — The
preserve_to→preserve_as/capture_to→capture_asmigration converted the two Orchestrator call sites but left the old wrappers in place with zero production callers and a docstring claiming callers that no longer exist (sandbox.py:1456, :1480, :1519). - 🟠 tests — Neither new CLI flag is exercised anywhere: no test in
tests/passes--logging-dir/--artifacts-diror calls_dir_template_overrides(run_command.py:451), so a dropped kwarg in eitherrun_command→run_pipeline→_run_all_tasksor the identicalexecute_command.py:272-273forwarding chain would ship green on … - 🟠 tests — The multi-task static-template rejection (batch.py:148-159) and
dir_template_is_static(path_utils.py:236) have no tests at all — everyrun_batch(call site in the suite passes a 0- or 1-element list, so the guard that stops N tasks from overwriting each other'stask.jsonnever executes in CI. - 🟠 tests — The host-side twin of the
/workrelaxation is untested:TestWorkspaceDirin tests/test_docker_runner_mounts.py still only pins/work/outputraising, and nothing asserts that_assert_workspace_not_reserved("/work")now passes or thatworking_dir: autoon the defaultcoder-eval-agentimage (Dockerfile … - 🟠 tests — No regression test points
artifacts_dir_templateat a pre-existing populated directory and asserts it survivesclear_rerun_artifacts(batch.py:464-466) — the only new coverage (tests/test_preservation_mode.py) uses a harness-created path, so the uncontainedrmtreeof an operator-supplied tree is invisible to the … - 🟡 tests —
aggregate_task_logs's new explicit-task_dirsbranch has exactly one single-directory test (tests/test_path_utils.py:281); the multi-task, ordering and nested<variant>/<suite>/<row>/<NN>header assertions still run only through the now production-dead glob fallback (logging_config.py:366), and nothing pins the … - 🟡 tests —
_final_artifacts_dir()is asserted on the capture/preserve arms only; the DIRECT_WRITE arm that now uses it (orchestrator.py:1553) has no test showing a caller-suppliedartifacts_dirbecomes the sandbox's direct-write target — the shape that makes the harness write the agent's workspace straight into an … - 🔵 tests — The load-bearing "no flag needed, the self-referential guard makes it a no-op" claim for the Harbor shape (
artifacts_dir == workspace_dir, an arbitrary path with notask_idsegment) is asserted only against a MagicMock sandbox (tests/test_cleanup_preservation_guard.py:196); the one real-Sandbox guard test still … - 🔵 tests — The golden Harbor fixture now pins the fictional
--no-capture-workspaceflag and the wrong--run-dir /logs/agentattribution at tests/_fixtures/harbor_export_golden/expected/tests/test.sh:17,18,22 (repeated in tests/test_harbor_packager.py:126), so the test suite actively locks in text that contradicts the shipped … - 🟡 downstream-consumers — The per-task path is now configurable but both consumer-facing schema statements still declare it fixed:
docs/REPORT_SCHEMA.md:25(<variant>/<task_id>/<NN>/task.json, the cross-repo contract the external eval-runner reads) and thetask_json_relpathfield description inmodels/results.py:839, which is persisted … - 🟡 downstream-consumers — The declared layout SSOT
.claude/shared/run-layout.mdand its "update both together" mirrorplugins/coder-eval/reference/run-layout.mdwere not touched, anddocs/USER_GUIDE.mdgains neither flag — every skill and command that computes<run_dir>/<variant>/<task>/<NN>/task.jsonfrom that contract is now wrong … - 🔵 downstream-consumers — The packager now emits a new top-level
artifacts = [...]key into every exportedtask.toml(packager.py:632) while the same file'stest.shcomment block still tells the reader artifacts land at/logs/agent/artifacts/<task_id>/; nothing outside the golden fixture checks the bundle against Harbor's … - 🔵 display-mapping — The placeholder set is a flat
_DIR_TEMPLATE_PLACEHOLDERStuple (path_utils.py:187) with no run-level vs per-task split and no link toresolve_dir_template's kwargs, so adding a placeholder means editing five unchecked copies — the tuple, the resolver signature, and the four hand-duplicated help strings at … - 🟠 daily-nightly — Version-skew lockstep is unstated:
models/sandbox.py's relaxedworking_dirvalidator is re-applied INSIDE the container (run_task_internal_command.py:178re-loads the whole dumpedtask.yaml), so a task authored withworking_dir: /workfails validation in any agent image built from a pre-PR coder-eval — the … - 🟡 daily-nightly — The PR rewires the production run path (
build_task_run_dir,ResolvedTask.run_dir, the Orchestrator capture target,clear_rerun_artifacts) but says nothing about the nightly; the defaults are byte-identical (pinned by tests/test_path_utils.py:140) so nothing moves today — that "no change to the nightly, defaults … - 🟡 daily-nightly — Cross-repo blast radius unstated: the external
coder-eval-uipath/ eval-runner pipeline readsrun.json/task.jsonat fixed relative paths, and the moment the nightly adopts--logging-dirthose reads — plus this repo's ownreport --rebuildandevaluate— break with no contract surface warning the consumer.
Harness & Lint Improvements
Static checks (lint / type):
- CE068 — no
run_dir-walking discovery of per-task run-record files. New AST ruletests/lint/rules/ce068_no_run_dir_walk_discovery.py(import +ALL_RULESentry intests/lint/runner.py, id added to[tool.ruff.lint].externalinpyproject.toml,TestCE068…class in … - CE069 —
shutil.rmtreeonly through a containment-checked chokepoint. New AST rule forbidding a directshutil.rmtree(...)call anywhere insrc/coder_eval/except insidepath_utils.py, which growsremove_tree_within(root: Path, target: Path)asserting … - CE070 — no set-arithmetic on a reserved/denylist constant at a check site. New AST rule (direct sibling of CE018) firing on
RESERVED_CONTAINER_DIRS - {…},… .difference(…), or anyBinOp/method call that narrows a security constant inline at a validation site, in … - CE071 — every long CLI flag named in shipped text must be a registered option. Whole-tree rule wired as a
@pytest.mark.lintclass (CE026 pattern, not the AST runner): collect the real flag inventory by walkingcoder_eval.cli.app's registered commands and theirtyper.Optiondecls; then … - CE072 — CLI-flag ↔ user-doc parity. Extend the CE030 doc-parity family (
tests/lint/doc_schema_parity.pyis the SSOT pattern): every long option registered on the user-facing commands (run,execute,evaluate,report,export) must appear as Markdown inline code in … - CE073 — run-layout SSOT mirror against
path_utils. CE065-style generated-surface check (tests/lint/generated.py+make docs-indexes-style regeneration):.claude/shared/run-layout.mdmust contain, inside<!-- run-layout:start/end -->markers, the literal values of … - CE074 — a grammar-constrained config
strmust declare its validator. AST rule: insrc/coder_eval/orchestration/config.pyandsrc/coder_eval/models/, a Pydantic field annotated barestrwhose name ends in_template(extendable to_pattern,_glob) must be named by a … - CE075 — no run-record row-key string literals outside
run_record.py. AST rule in the CE053/CE054 family: insrc/coder_eval/, a subscript or.get("…")whose key literal is a field name emitted byrun_record.eval_result_to_task_dict(read from the serializer, never retyped) is a violation … - CE076 — statement/branch cap on
run_batch, pinned below its current value. CE022 precedent (tests/lint/rules/ce022_dialog_loop_statement_cap.pytargets one named function): caporchestration/batch.py::run_batchat its pre-PR size so any new config validation must land in a named … - CE077 — widen CE037 to single-delegation method wrappers.
tests/lint/rules/ce037_no_dead_private_helper.pytoday covers only module-level privatedefs. Extend it (or add a sibling id) to methods whose body is a singlereturn self.<other>(…)delegation: such a method must have at least one … - Enable
RUF029(unused-async) — it is preview-gated, so set[tool.ruff] preview = truewith an explicitextend-select = ["RUF029"](and pin the rest of the preview surface if the incidental new diagnostics are unwelcome).make checkalready lintstests/( … - Two type-level tightenings, both making a currently-invisible mistake a typecheck failure: (1) make
_dir_template_overrides(cli/run_command.py:451-462) return aTypedDict(total=False)whose keys are declared, or drop the dict entirely and passlogging_dir_template=…/ … - Ban new hardcoded
/tmp/...path literals insrc/coder_eval/at the rule level rather than per-site: keepbandit's B108 and add a narrow CE check (or a bandit baseline entry) requiring each# nosec B108to state the environmental precondition that makes the fixed path safe. Better still …
Harness improvements (not statically reachable):
- Replace the collision predicate with a collision assertion over the real task set, and test it. After
resolve_all_tasks, assertlen({rt.run_dir for rt in resolved_tasks}) == len(resolved_tasks)(and the same for the resolved artifacts dirs) inside the extractedvalidate_run_layout, … - A host↔container path round-trip test for the docker driver. With a fake/stubbed
DockerRunner, assert that under a non-default--logging-dirthe path the in-containerOrchestrator._final_artifacts_dir()resolves to (/work/output/artifacts/<task_id>, i.e. anchored on the mounted … - A destructive-operation guard in the test harness plus one regression test. Add an autouse fixture that wraps
shutil.rmtreeand fails any test whose deletion target is not undertmp_path(or an explicitly opted-in path), and a regression test that pointsartifacts_dir_templateat a … - Capture-containment invariants at cleanup time, with tests. Assert at
capture_as/preserve_asthat the destination is not inside the capture source (today's guard only short-circuits on exact equality) and that the capture source is not a framework mount root; add a docker test asserting a … - Diff-coverage gate in
make verify/ CI. Add a per-change coverage check (diff-coveragainst the merge base, or a changed-files--cov-fail-under) alongside the existing global 80% threshold, so a PR cannot add an unexecuted branch while the repo-wide number stays green. - A Harbor end-to-end smoke test pinned to the flags
CoderEvalAgentactually emits. Runcoder-eval execute --format harborwith the exact argv built inharbor/agent.py:98-101against a NoOp task, and assert the files the generated verifier reads exist where it reads them: … - Resolve the layout once onto
ResolvedTaskand make that the only mechanism. GiveResolvedTaska resolvedartifacts_dirbeside its existingrun_dir, both populated inresolve_all_taskswhere the placeholders first exist, and persist the resolved logging dir per row inrun.jsonso the … - Narrow the mechanism before institutionalizing it. Before adding rules around the
${}mini-language, consider deleting it: only two configurations exist in the whole repo (the two defaults atpath_utils.py:184-185and Harbor's two static paths atharbor/agent.py:101), and the generality …
Top 5 Priority Actions
-
Fix the uncontained
shutil.rmtreeof the operator-supplied artifacts dir inclear_rerun_artifacts(src/coder_eval/orchestration/batch.py:464-466), which runs from_apply_resume(cli/run_command.py:1127) beforerun_batch's new guards (batch.py:139-159) and was reproduced deleting a populated user directory — refuse--resumewith a non-defaultartifacts_dir_templateinrun_pipelinealongside the existing--workspace-dircheck, make the clear a no-op when the harness cannot prove it owns the directory, and add a regression test that points the template at a populated tree and asserts it survives. -
Give
--logging-dirthe docker guard its--artifacts-dirsibling already has at src/coder_eval/orchestration/batch.py:139 (or anchorDEFAULT_ARTIFACTS_DIR_TEMPLATEon the resolved logging dir): underdriver: dockerthe container writes artifacts tort.run_dir/artifacts/<task>whileclear_rerun_artifactsclears the${run_dir}-anchored path, so a resumed run returnscleared=0and the killed run's files stay on disk forfile_exists/commandcriteria to pass on — a stale-artifact false pass, reproduced at HEAD. -
Replace
dir_template_is_static(src/coder_eval/path_utils.py:236) with a per-task-collision predicate: it tests "no placeholders at all", so--logging-dir '${run_dir}/logs'slips past the multi-task guard at batch.py:148-159 and all N tasks overwrite each other'stask.json/task.login one directory; the correct test is that${task}(and${variant}/${repeat}where those axes fan out) is present, and the branch plus the helper currently have zero test coverage (batch.py:149-154 and path_utils.py:240 are both uncovered). -
Keep
/workreserved inmodels/sandbox.py:300and its host-side twinisolation/docker_runner.py:384: exempting the mount root makesworking_dir: autoon the default image (docker/Dockerfile:112WORKDIR /work) silently resolve the graded workspace to the tree holding/work/referencesand/work/output, so path-relative criteria can match the reference solution and cleanup runscopytree("/work", "/work/output/artifacts/<task>")into its own descendant — nothing in the Harbor packager needs this relaxation (packager.py:277 never passes through the validator). -
Route the five remaining
run_dir-walking consumers through the resolved logging dir —recover_task_results(src/coder_eval/orchestration/batch.py:514),reports/markdown.py:753and:975,cli/run_helpers.py:157,cli/report_command.py:149— or persist the resolved logging dir per row inrun.json, since only two of seven discovery sites were migrated and a non-default--logging-dirmakesreport --rebuild/evaluatefail outright while command stats,suite.jsontask_json_relpathlinks and the end-of-run log listing degrade silently; fold in the cheap companions while you are there: a@field_validatoron the twostrtemplate fields (orchestration/config.py:130,137) so a typo fails atBatchRunConfig(...)instead of becoming N mislabelled ERROR rows, deletion of the dead trajectory-promotion branch at cli/run_command.py:736 plus its stale docstring at :631, and updates to.claude/shared/run-layout.md, its plugin mirror,docs/USER_GUIDE.mdand the four stale help strings (run_command.py:360,368; execute_command.py:184,192) and the fictional--no-capture-workspacein harbor/packager.py:66,70.
uipreliga
left a comment
There was a problem hiding this comment.
Fix what you agree with and 🚢
Addresses the highest-confidence findings from the second 8-axis review of PR #189 (--post-comment on pr:189): - Replace the artifacts/logging-dir static-template heuristic with a real collision check: resolve every task's dir and assert the results are unique, instead of testing "has any placeholder at all" (dir_template_is_static let ${run_dir}-only templates slip through and overwrite every task's task.json). - Reject a non-default --logging-dir for sandbox.driver: docker, mirroring --artifacts-dir's existing guard -- the container's real artifacts land under the resolved logging dir, so an override desyncs it from the artifacts_dir_template default that clear_rerun_artifacts/--resume rely on. - Refuse --resume together with --artifacts-dir: clear_rerun_artifacts rmtree's whatever the template resolves to, which is safe only because the default always names a directory the harness itself created -- an operator-supplied override points at a pre-existing tree with no such guarantee. - Revert the /work exemption in the working_dir reserved-path validator (models/sandbox.py, isolation/docker_runner.py): /work is the framework's mount ROOT (input/output/references/task_dir all live under it), so exempting it let a task's graded workspace become the same tree holding the reference solution and the run's own task.json. - Add a field_validator on BatchRunConfig's two *_template fields so an unknown placeholder is a clean ValidationError at config-construction time instead of N mislabelled ERROR rows discovered per-task. - Delete the dead/misattributing single-trajectory promotion branch in run_command.py: now that task_dirs is resolved consistently via config.resolve_logging_dir, the promotion's guard condition is always false for the single-task case it exists for, and the only way to enter it (a multi-task run where exactly one task produced a task.json) copies the wrong task's trajectory into task 0's directory. - Fix the # nosec B108 justification on the Harbor throwaway run dir to name the property that actually makes a hardcoded /tmp path safe here (single-tenant container, nothing to race), not content provenance. - Correct the generated Harbor test.sh's stale comment (a fictional --no-capture-workspace flag, wrong --run-dir attribution) to match what CoderEvalAgent actually invokes. - Update --workspace-dir/--logging-dir/--artifacts-dir help text on both `run` and `execute` to document the docker/multi-task/--resume restrictions. Not addressed in this pass (tracked as follow-up, not score-affecting today since Harbor is the only non-default-template consumer and it's single-task with static templates): migrating the remaining run_dir-walking discovery sites (recover_task_results, reports/markdown.py, run_helpers.py, report_command.py) to the resolved logging dir, and persisting the resolved template in run.json for detached `evaluate` to recover. make verify passes: ruff, pyright, docs-budget, pytest (6471 passed, 8 skipped, 92.75% coverage), and the custom lint suite are all clean. bandit -r src/ -ll (CI's exact invocation): no issues. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Harbor needs coder-eval's per-task bookkeeping (task.json/task.log) in its own agent logs dir and the agent's artifacts left at the container's WORKDIR — two unrelated parts of the filesystem, but artifacts were structurally a child of the run dir, so pointing
--run-dirat Harbor's logs dir duplicated the whole workspace underneath it.BatchRunConfig,logging_dir_templateandartifacts_dir_template, resolved late (per task). An unspecified run writes byte-identical paths to the historical layout; a static override (Harbor's use case) needs no special-casing since substituting a template with no placeholders is the identity function.emit_trajectories_for_run/aggregate_task_logswalkingrun_dirinstead of the resolved logging dirs,clear_rerun_artifactsclearing the wrong path under an override, and--resumefailing containment over a decoupled layout.ed0e2947) closes gaps a full 8-axis code review (/coder-eval-code-review-full) surfaced against the first commit:--artifacts-diris now rejected outright forsandbox.driver: docker(previously a silent no-op —ContainerContexthas no way to carry it into the container), a static--logging-dir/--artifacts-diris now rejected on a multi-task run (previously every task would collide on the same directory),emit_trajectories_for_runnow logs on a missingtask.jsoninstead of silently dropping the row, and several docstrings/comments were trimmed to satisfymake docs-budget.Verified live against real Harbor + Docker: 4/4 trials reward 1.0 with zero
runs/pollution, zero duplicate workspaces, andtrajectory.jsonpresent in all four.Test plan
make verify(ruff, pyright, docs-budget, pytest, custom lint) — all green, 6468 tests passeduv run bandit -r src/coder_eval/ -ll/uv run pip-audit— no new findings--artifacts-dirrejection, static-template + multi-task rejection,emit_trajectories_for_run(outside-run_dir dirs, dedup, missing/unreadable task.json)🤖 Generated with Claude Code