Add isolated_paths declaration to the docker driver - #96
Conversation
|
Claude finished @dmorosanu's task in 1m 24s —— View job Code Review in Progress
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:96 (5 files) axis:1,2,3,4,5,6,7,8
Scope: pr:96 (5 files) axis:1,2,3,4,5,6,7,8 · branch feat/isolated-paths-declaration · 8d3e4f3 · 2026-08-10T09:49Z · workflow variant
Change class: complex — introduces new security-boundary enforcement control flow (path-overlap proof over container mount sources + plugin bundle manifests) whose correctness requires reasoning about path resolution, symlinks, and which mounts are agent-visible
The codebase is in strong shape — clean typing, zero security findings, well-factored merge/criteria/streaming layers, and a large green test suite — but the PR's new fail-closed isolated_paths guard is the real risk: its agent-visible mount enumeration is incomplete (template sources, the LiteLLM cost-log mount) and its declarations get no shape validation, so it can silently pass on a genuinely leaked path and inflate task scores, while its run() call site is unproven by any test; close the completeness, validation, and wiring-coverage gaps and this ships at high confidence.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.4 / 10 | 0 | 0 | 1 | 1 | _plugin_bundle_manifests is redundant parallel per-plugin state: the (Path, BundleManifest) list duplicates BundleManifest.source and its single consumer could be answered from existing structures |
| 2. Type Safety | 9 / 10 | 0 | 1 | 0 | 0 | isolated_paths entries get no shape validation: an unexpanded/misspelled $VAR, relative, or empty entry resolves non-strictly under the host CWD and turns the fail-closed guard into a silent no-op |
| 3. Test Health | 9 / 10 | 0 | 1 | 0 | 0 | The new isolated-paths guard is effectively untested: the run() call site and the empty-declaration short-circuit are uncovered, the documented 'passes' contracts assert vacuously, and the manifest.symlinks branch is never exercised |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 8.9 / 10 | 0 | 1 | 0 | 1 | _agent_visible_mount_sources() docstring's completeness invariant is false: the ungated LITELLM_COST_LOG parent-dir bind mount (and other non-grader mounts such as extra_mounts / build context) are unenumerated, so _enforce_isolated_paths() can pass on an agent-readable path |
| 6. Error Handling & Resilience | 9.9 / 10 | 0 | 0 | 0 | 1 | The ~/.claude entry compares against the live host home instead of the CLAUDE_COPY_IGNORE-filtered copy that is actually mounted, so declaring an excluded subtree hard-fails the run on a false overlap |
| 7. API Surface & Maintainability | 9.9 / 10 | 0 | 0 | 0 | 1 | isolated_paths reads as a mount instruction rather than an assertion, unlike its neighbours on the same model |
| 8. Evaluation Harness Quality | 9 / 10 | 0 | 1 | 0 | 0 | isolated_paths guard fails open on sandbox.template_sources: _agent_visible_mount_sources() omits template dirs whose bytes are copied into the agent-owned workspace (and a test pins the unsafe pass) |
Overall Score: 9.4 / 10 · Weakest Axis: Architecture & Design at 8.9 / 10
Totals: 🔴 0 · 🟠 4 · 🟡 1 · 🔵 4 across 8 axes.
Blockers
- [Axis 2] isolated_paths entries get no shape validation: an unexpanded/misspelled $VAR, relative, or empty entry resolves non-strictly under the host CWD and turns the fail-closed guard into a silent no-op (
src/coder_eval/models/sandbox.py:296) — The new field is declared as a bareisolated_paths: list[str] = MergeField(strategy="replace", default_factory=list, ...)(sandbox.py:296) with no@field_validator, while its sibling on the very same model does have one —@field_validator("working_dir")at sandbox.py:307 raisesworking_dir {v!r} must be an absolute path or the sentinel 'auto'.Every other user-authored host-path field is also normalized at load time against the task YAML dir (resolve_template_paths/resolve_protected_mock_paths/resolve_dockerfile_pathin orchestration/task_loader.py:126-166, the last two of which additionally require the path to exist).isolated_pathsgets none of that; the only processing is docker_runner.py:889:declared = Path(os.path.expandvars(os.path.expanduser(raw))).resolve(). Verified with the interpreter, withSKILLS_REPO_PATHunset:'$SKILLS_REPO_PATH/tests'-><cwd>/$SKILLS_REPO_PATH/tests;'tests'-><cwd>/tests;''-><cwd>. Combined with the deliberateA declared path that does not exist on the host has nothing to expose, so it resolves non-strictly and passesrule (docstring, docker_runner.py:880-881) plustest_missing_declared_path_passes(tests/test_isolated_paths.py:133-138), any of those three malformed inputs makes the whole declaration a silent no-op — fail OPEN on a field documented as a fail-closed security contract. This is not hypothetical: the documented example in docs/TASK_DEFINITION_GUIDE.md:599 is literally- "$SKILLS_REPO_PATH/tests", and the runner itself treats that variable as optionally-unset (if cfg.agent_isolation and (skills_repo := os.environ.get("SKILLS_REPO_PATH")), docker_runner.py:1446, with a debug-onlyNot forwarding unstaged SKILLS_REPO_PATH...at 1452). So a run on a machine that forgot to exportSKILLS_REPO_PATHreports no error and enforces nothing. Fix: add a@field_validator("isolated_paths")next to_validate_working_dirthat (a) rejects an empty/whitespace entry, (b) rejects an entry that is still not absolute afterexpanduser/expandvars, and (c) rejects an entry that still contains$afterexpandvars(unresolved variable). Existence must stay optional, but unresolvability must not be silently equivalent to it. Add tests for the relative, empty, and unset-$VARcases — tests/test_isolated_paths.py currently only covers the SET env-var case (test_env_var_in_declaration_is_expanded, line 112-121). - [Axis 3] The new isolated-paths guard is effectively untested: the run() call site and the empty-declaration short-circuit are uncovered, the documented 'passes' contracts assert vacuously, and the manifest.symlinks branch is never exercised (
src/coder_eval/isolation/docker_runner.py:635) — Line 635 isawait asyncio.to_thread(self._enforce_isolated_paths)and sits inside the coverage-missing range572-674(verified:uv run pytest tests/ --cov=coder_eval.isolation.docker_runner --cov-report=term-missing→633 stmts, 170 miss ... 70.89% ... 572-674, ...). I confirmed by mutation: removing that exact line leaves the worktree with a fully green docker/plugin/protected-mock test set.tests/test_isolated_paths.pyonly ever calls the guard directly (lines 79, 95, 109, 120, 131, 138, 161, 167) — nothing drivesrun(), so neither the invocation nor its ordering requirement ("before any container starts") is asserted. A fail-closed security guard that is never proven to be wired in is worth nothing.tests/test_docker_build_failure.py:67-82already shows the cheap recipe:monkeypatch.setattr(dr, "_preflight", lambda: None)+ stub_build_image, thenasyncio.run(runner.run()). Add a test that stubs_preflight/_build_image/_preflight_agent_isolation_imageandasyncio.create_subprocess_exec(to a sentinel-raising spy), configures an overlappingisolated_paths, and assertsDockerRunErroris raised AND the subprocess spy was never called — i.e. the guard fires strictly beforedocker run. - [Axis 5] _agent_visible_mount_sources() docstring's completeness invariant is false: the ungated LITELLM_COST_LOG parent-dir bind mount (and other non-grader mounts such as extra_mounts / build context) are unenumerated, so _enforce_isolated_paths() can pass on an agent-readable path (
src/coder_eval/isolation/docker_runner.py:860) — The new method's docstring asserts an architectural invariant that nothing enforces and that is already false: lines 860-862 claim "Every other mount the runner renders lands below the root-only grader parent (or the mockd fixture parent), so it is outside this set by construction." I walked all eleven-vsites in_build_argv(grepargv += ["-v"→ lines 1480, 1501, 1505, 1513, 1525, 1528, 1530, 1532, 1571, 1604). Underagent_isolation(whichisolated_pathsrequires), 1571 (_auto_mount) and 1604 (extra_mounts) are dead (extra_mountshard-errors at line 589), 1501/1505/1513 land under/opt/coder-eval/grader(0700 root, docker/Dockerfile:47-54), 1532 lands under/opt/coder-eval/mock(0500 mockd, Dockerfile:55-58), and 1525/1528 are enumerated at lines 866-869. Line 1480 is not:argv += ["-v", f"{abs_log.parent}:{abs_log.parent}:ro", "--env", f"LITELLM_COST_LOG={abs_log}"]mounts a host directory at its own absolute container path — not below the grader parent — and is gated only onLITELLM_COST_LOGbeing set,LITELLM_COST_LOGbeing inenv_passthrough(it is, by default, models/sandbox.py:263) andnetwork != "none"; there is noagent_isolationgate. tests/test_docker_litellm_env.py:102 asserts exactly this shape (f"{log_dir.resolve()}:{log_dir.resolve()}:ro" in argv). Docker creates the intermediate container-side dirs root:root 0755, so the agent UID can traverse and read it whenever the host dir mode permits. Fix the instance by appending(f"litellm cost-log mount {abs_log.parent}", abs_log.parent)to the visible set, but fix the architecture too: the enumeration is a hand-maintained second source of truth for_build_argv's mount pipeline, so the next mount added will silently shrink the guard. Route every-vthrough one_add_mount(source, target, *, agent_visible: bool)helper that appends to a single mount table read by both argv rendering and_enforce_isolated_paths, assert every private targetstartswith(CONTAINER_GRADER_DIR), and add a CEnnn lint rule that fails on a rawargv += ["-v", ...]outside that helper. - [Axis 8] isolated_paths guard fails open on sandbox.template_sources: _agent_visible_mount_sources() omits template dirs whose bytes are copied into the agent-owned workspace (and a test pins the unsafe pass) (
src/coder_eval/isolation/docker_runner.py:866) — The method returns only plugin-bundle mounts plus the~/.claudecopy:
sources = [(f"plugin bundle mount {target}", source.resolve()) for source, target in self._agent_plugin_mounts]and its docstring claims (line 860-862) "Every other mount the runner renders lands below the root-only grader
parent (or the mockd fixture parent), so it is outside this set by construction." That is false for TemplateDirSource. Trace: _prepare_isolated_sources line 813 registers each template dir as a PRIVATE mount (self._register_private_mount(host_path, f"/opt/coder-eval/grader/templates/source-{template_index}")) and _rewrite_task_paths rewrites source.path to it — but a template source exists precisely so its bytes are DELIVERED to the agent: Sandbox._apply_template_dir_source (src/coder_eval/sandbox.py:329) copies the tree into sandbox_dir, which under isolation is /work/agent (orchestrator.py:1046 direct_target = self.workspace_dir), and orchestrator.py:1085 then calls _grant_current_sandbox_to_agent() → grant_agent_workspace(self.sandbox.sandbox_dir) (orchestrator.py:2288-2295), chowning the whole tree to the agent UID. The private mount hides only the raw host PATH, never the CONTENT.
The PR enshrines the hole: tests/test_isolated_paths.py:148 builds the runner with template_dir=plugin / "tests" while declaring .../skills-repo/tests, and test_declared_tests_tree_outside_the_projection_passes (line 154-161) asserts runner._enforce_isolated_paths() returns cleanly. So a suite that declares isolated_paths: ["$SKILLS_REPO_PATH/tests"] (the documented flagship example) AND stages a fixture project from under that tree via template_sources gets a green declaration while the agent reads the golden fixtures out of its own cwd — inflating the graded score with no signal that the boundary was breached. docs/TASK_DEFINITION_GUIDE.md:602 ("Raw task, reference, template, and plugin sources mounted below the root-only grader parent are exempt") states the wrong claim to the task author; reference/task_dir genuinely stay grader-only, template_sources do not.
Fix: add each TemplateDirSource host path to _agent_visible_mount_sources() (label it "template source copied into the agent workspace"), delete template from the exempt list in both docs, and change the test to assert the overlap now FAILS. Keep the reference/task_dir exemptions as-is.
Non-blocking, but please consider before merge
- [Axis 1] _plugin_bundle_manifests is redundant parallel per-plugin state: the (Path, BundleManifest) list duplicates BundleManifest.source and its single consumer could be answered from existing structures (
src/coder_eval/isolation/docker_runner.py:896) — The bundle check is:
for source, manifest in self._plugin_bundle_manifests:
for relative in (*manifest.files, *manifest.symlinks):
entry = source / relative
if entry == declared or declared in entry.parents:
This is the only consumer of the new self._plugin_bundle_manifests: list[tuple[Path, BundleManifest]] field (line 529), which is a fourth per-plugin collection populated in the same loop as _agent_plugin_mounts (792), _host_plugin_to_agent_paths (795) and _register_private_mount (794) — index-parallel bookkeeping that must now be reset in lockstep (762) and can silently drift. The projection surface is already a closed, declared set: stage_bundle/build_manifest copy only top-level entries under plugin_bundle.PLUGIN_AGENT_ALLOWED_SUBDIRS ({"skills", "commands", "agents", ".claude-plugin", "hooks"}) and raise rather than silently skip anything hidden inside them (_is_hidden_material). So declared.relative_to(source).parts[0] in PLUGIN_AGENT_ALLOWED_SUBDIRS (for a declared under a plugin source) is an exact, allowlist-based, O(1) equivalent that needs no new field and no manifest walk — and it is strictly fail-closed where the manifest walk is not (an allowed subtree containing no files today yields no manifest entry). It would also drop _enforce_isolated_paths from radon C (11) to a flat loop. If the manifest really is wanted for the error message, derive it from the already-persisted manifest_path_for(bundle_dir) / verify_bundle() at check time instead of holding a fourth parallel list.
Nits
- [Axis 1]
isolated_pathsvalidation is bolted onto a model validator namedvalidate_template_sources(src/coder_eval/models/sandbox.py:578) — The new block
if self.docker.isolated_paths:
if self.driver != "docker":
raise ValueError("sandbox.docker.isolated_paths requires driver: docker")
is appended inside @model_validator(mode="after") def validate_template_sources(self) -> SandboxConfig: (line 564, docstring """Validate template sources configuration."""), which already carries an unrelated protected_mocks/record_cli block. That makes three unrelated concerns under a name and docstring that promise one. Rename to something like _validate_sandbox_consistency (or split the docker-driver preconditions into their own validator) and update the docstring; the method name is the only thing a future reader greps for.
2. [Axis 5] Third unrelated cross-field concern added to the misnamed validate_template_sources model validator (src/coder_eval/models/sandbox.py:578) — def validate_template_sources(self) -> SandboxConfig: (line 564, docstring "Validate template sources configuration.") now also validates protected_mocks (pre-existing, lines 569-577) and, from line 578, if self.docker.isolated_paths: — three unrelated cross-field concerns behind a name that advertises one, in a model that reconstructs on every merge-layer resolution (config_merge.resolve_root line 385). Split into separate @model_validator(mode="after") methods (_validate_protected_mocks, _validate_isolated_paths) or rename to _validate_cross_field_config. Note the deliberate part is fine: hard-erroring for a driver mismatch (line 580) rather than the silent "Docker driver only; ignored under driver:tempdir" treatment working_dir gets (line 213) is the correct stance for a security declaration and mirrors protected_mocks — the in-process driver is not left with a silently-ignored knob, and the inverse guard (Technique 5) is covered on both axes (driver and agent_isolation). Only the validator's growing scope is the issue.
3. [Axis 6] The ~/.claude entry compares against the live host home instead of the CLAUDE_COPY_IGNORE-filtered copy that is actually mounted, so declaring an excluded subtree hard-fails the run on a false overlap (src/coder_eval/isolation/docker_runner.py:869) — sources.append((f"agent home mount {agent_claude}", (Path.home() / ".claude").resolve())) (docker_runner.py:869) treats the whole host ~/.claude as agent-visible, but the mounted artifact is the lean copy made by _copy_claude_home, which drops every CLAUDE_COPY_IGNORE entry — including projects (transcripts), security, sessions, telemetry (docker_runner.py:111-130). So isolated_paths: ["~/.claude/projects"], a reasonable declaration for "the agent must not read my other sessions", aborts the run with "overlaps the agent-visible agent home mount" even though that subtree is never copied into the container. The direction is fail-closed so this is a usability nit, not a safety hole; if fixed, compare against self._claude_mount_src's actual contents (or skip declarations whose first path component under ~/.claude matches CLAUDE_COPY_IGNORE) and say so in the error message.
4. [Axis 7] isolated_paths reads as a mount instruction rather than an assertion, unlike its neighbours on the same model (src/coder_eval/models/sandbox.py:296) — Line 296's isolated_paths sits three lines below extra_mounts (line 291), and every other list[str] on DockerDriverConfig (env_passthrough, env_passthrough_extra, extra_mounts) is an instruction the runner executes; a task author scanning the schema will reasonably read this one the same way ("isolate these paths") rather than "prove these paths are already isolated, else abort". Both docs additions do disambiguate — docs/TASK_DEFINITION_GUIDE.md:592 "It mounts nothing; it is a contract the runner checks before any container starts" and docs/DOCKER_ISOLATION.md:45 "The declaration mounts nothing" — so this is naming only, but an assertion-shaped name (assert_paths_private / require_private_paths) would carry the semantics into the YAML itself where the docs are not present, and would make a [] override read as obviously self-defeating. Also worth adding to the guide: the failure surface is a DockerRunError raised from run() (docker_runner.py:635) — i.e. AFTER _build_image()/image preflight and reported as an ERROR-status task, not a config error at coder-eval plan time.
What's Missing
Parallel paths:
- 🟠
_agent_visible_mount_sources()(docker_runner.py:857-870) was added as a second, hand-maintained view of a mount pipeline whose real source of truth is_build_argv(nine-vsites, 1480-1571) plus_prepare_isolated_sources' template branch (813) — the parallel paths were not updated, so the host-absoluteLITELLM_COST_LOGparent mount (1480) and template-source trees copied into/work/agentare invisible to the guard; every future mount added to_build_argvshrinks the guard silently unless both places are edited (see also Axis 8 for the template_sources arm). (trigger: src/coder_eval/isolation/docker_runner.py) _(restates: Axis 5: agent_visible_mount_sources() completeness invariant is false (LITELLM_COST_LOG mount unenumerated)) - 🟡 The variant/driver override path was not considered:
ExperimentVariant.driver(models/experiment.py:66, documented for "variant-A=tempdir vs variant-B=docker" comparisons) and--driver tempdir/-D sandbox.driver=tempdirnow become hard failures for any task declaringisolated_paths— I reproduced both: the variant case raises a raw pydanticValidationErrorinsideresolve_task_for_variant, whichexperiment.py:709collects as a resolution error and skips the task, so the tempdir cell of an A/B matrix quietly disappears instead of running; the-Dcase raisesOverrideError. Either document the required-D sandbox.docker.isolated_paths='[]'escape hatch (working_dirgets silently ignored instead) or make the driver conflict an explicit, guided error likevalidate_early_stop's. (trigger: src/coder_eval/models/sandbox.py) - 🟡 No
coder-eval plansurface for the new declaration, breaking the project's "every resolution-time guardrail is a hard error at plan and run" convention (plan_command.py:138callsvalidate_early_stopfor exactly this reason): only the shape checks inSandboxConfigreach plan, while the actual overlap proof lives run-only at docker_runner.py:635 — the cheap subset (declaration vs each pluginpathand eachTemplateDirSource.path) needs no staging and could run at plan time, so a misconfigured suite is caught before dispatch instead of as an ERROR-status task per row. (trigger: src/coder_eval/isolation/docker_runner.py) - 🔵
_enforce_isolated_pathsis called unconditionally at docker_runner.py:635 but its visible set is only meaningful underagent_isolation: with isolation off,_prepare_isolated_sourcesnever runs (688), so_agent_plugin_mounts/_plugin_bundle_manifestsare empty and every declaration passes vacuously while_auto_mount/extra_mountsexpose the host — the model validator is the sole thing preventing that state, and validation-bypassing paths already exist in-tree (run_task_internal_command.py:208mutatesSandboxConfigviamodel_copy, which skips validators). Add a defensiveif not self._docker_config.agent_isolation: raiseinside the guard rather than trusting a distant validator. (trigger: src/coder_eval/isolation/docker_runner.py)
Tests:
- 🟠 No test drives
DockerRunner.run(), so neither the wiring of the new guard nor its "before any container starts" ordering is asserted (all eight test call sites invoke_enforce_isolated_paths()directly);tests/test_docker_build_failure.py:67-82already shows the stub-_preflight/_build_imagerecipe needed to assert the raise happens beforeasyncio.create_subprocess_exec. (trigger: tests/test_isolated_paths.py) _(restates: Axis 3: enforce_isolated_paths call site and branches untested) - 🟡 The new test file has no negative-shape cases for the declaration itself — relative entry, empty string, and unset
$VAR(the documented$SKILLS_REPO_PATH/testsexample) all resolve under the host CWD and pass silently; only the set env-var case is covered (test_isolated_paths.py:111-120), so the fail-open behavior is untested in either direction. (trigger: tests/test_isolated_paths.py) (restates: Axis 2: isolated_paths entries get no shape validation) - 🟡 No test covers the reporting of a triggered guard: nothing asserts what a breach looks like to
orchestration/batch.py::_create_error_result, to run.json, or torecover_task_results. Since the whole feature exists to make a boundary breach visible, a test that a fired guard yields a per-task artifact with a recognizable status is as load-bearing as the overlap logic itself. (trigger: src/coder_eval/isolation/docker_runner.py) - 🔵
TestConfigMergeonly pins the happy path (experiment-defaults layer ==-Dlayer). Missing: the conflict cases I reproduced — a variantdriver: tempdir(task-skippingValidationError) and-D sandbox.docker.agent_isolation=false(OverrideError) against a task that declaresisolated_paths— which are the interactions most likely to bite an A/B suite. (trigger: tests/test_isolated_paths.py)
Downstream consumers:
- 🟠 The new pre-container failure path leaves no per-task artifact, and
orchestration/batch.py::_create_error_resultwas not extended for it: theDockerRunErrorfrom docker_runner.py:635 is raised afterrun_dir.mkdir(576) but before any_write_synthetic_task_json, so the run dir gets notask.jsonand nodocker.log; the task is recorded only run-level asFinalStatus.ERRORwith the misleading descriptionFailed to load task from <file>: DockerRunError. Consequence:recover_task_results(batch.py:428 rglobstask.json) and thereforecoder-eval report <run_dir>regeneration drop the breached task entirely, and--resumere-runs it — the exact empty-result-dir failure that_record_build_failure(docker_runner.py:1124) was written to fix for image builds, now reintroduced for the isolation guard. (trigger: src/coder_eval/isolation/docker_runner.py)
Display & mapping dicts:
- 🟡 No distinct status/reason was added for a declaration breach, despite the in-tree precedent:
DockerBuildErrorgotFinalStatus.BUILD_FAILEDplus entries inmodels/enums.py's category map (:41) and letter map (:55) so "reports/run.json distinguish it" (batch.py:293). An isolated_paths breach — a security-contract regression — collapses into genericERRORand is indistinguishable in reports/JUnit/evalboard from a docker daemon flake or a crashed container. (trigger: src/coder_eval/isolation/docker_runner.py)
Daily/nightly:
- 🟠 Blast radius on the production run path is unstated and unexercised: a new unconditional step now runs inside
DockerRunner.run()for every docker-driver task, yet no in-tree task, experiment, or CI workflow declaresisolated_paths(grep finds it only in the two doc snippets and the new unit test), so the guard ships with zero end-to-end/nightly coverage and the PR never says whether the motivating skills suite adopts it, what a nightly breach does to the gate (currently a generic ERROR task, which nightly triage reads as infra flake), or that the resolvedsandbox.dockerdump consumed by the evalboard gains a field. (trigger: src/coder_eval/isolation/docker_runner.py) - 🟡 If the nightly skills suite adopts the documented flagship form (
- "$SKILLS_REPO_PATH/tests", TASK_DEFINITION_GUIDE.md:599) the nightly reports a green boundary it does not have: on a runner that forgot to exportSKILLS_REPO_PATHthe entry silently resolves under the CWD and enforces nothing (Axis 2 — the runner itself treats that variable as optionally-unset at docker_runner.py:1446), and a fixture tree staged throughtemplate_sourcespasses the guard while still being copied into the agent's workspace (Axis 8). The PR should state which nightly suites are expected to carry the declaration and how a breach is escalated. (trigger: docs/TASK_DEFINITION_GUIDE.md) (restates: Axis 2: isolated_paths entries get no shape validation)
Harness & Lint Improvements
Static checks (lint / type):
- [ce-lint] CE034 — single mount seam in the isolation layer. New rule
tests/lint/rules/ce034_single_mount_seam.py(classSingleMountSeam, wired intoALL_RULESintests/lint/runner.py; next free id after CE033): insidesrc/coder_eval/isolation/, forbid a"-v"string literal appearing in anyargv += [...]/argv.extend([...])/ list-literal assignment outside one designated helper (DockerRunner._add_mount). AST shape: walkAugAssign/Call(attr='extend'|'append')whose target name matchesargv, flag when any element isConstant("-v")and the enclosingFunctionDef.name != "_add_mount". Verified today there are 9 such raw mount sites indocker_runner.py::_build_argv(lines 1480, 1501, 1505, 1513, 1525, 1528, 1530, 1532 plus theargv.extend(["-v", ...])at 1571) and a 10th in theextra_mountsloop at 1604, while the agent-visible enumeration_agent_visible_mount_sources()(857-870) hand-lists only 2 of them. Forcing every mount through_add_mount(source, target, *, agent_visible: bool)that appends to one mount table makes both argv rendering and_enforce_isolated_pathsread the same list, so a new mount cannot silently shrink the guard, and the~/.claudeentry naturally records the actually-mounted staged copy (self._claude_mount_src) instead of the raw hostPath.home() / ".claude"at line 869. Pair it with anassertin_add_mountthat everyagent_visible=Falsetarget starts withCONTAINER_GRADER_DIRor/opt/coder-eval/mock. Prevents: A5-high: the LITELLM_COST_LOG parent-dir mount at docker_runner.py:1480 targets the host absolute path (not under the grader parent) and has noagent_isolationgate, yet is missing from_agent_visible_mount_sources(), so the docstring's "by construction" invariant (860-862) is false and the fail-closed guard returns a false negative. Also A6/A1/A8-low: the~/.claudearm at :869 comparing against the live host home instead of the CLAUDE_COPY_IGNORE-filtered copy that is actually mounted. - [ce-lint] CE035 — every user-authored host-path field must declare shape validation. New whole-tree check in the CE030/CE031 family (a dedicated
@pytest.mark.lintclass backed bytests/lint/host_path_fields.py, not a per-ASTBaseRule): for each field on the YAML-facing models whose name matches*path|*paths|*dir|*dirs|*file|*filesor whosedescriptioncontains "host path", require that the field name appears either (a) in a@field_validator("<name>")on its own model, or (b) as a resolved field in one oforchestration/task_loader.py's resolvers (resolve_template_source_paths,resolve_protected_mock_paths,resolve_dockerfile_path,resolve_initial_prompt_file,resolve_system_prompt_files), or (c) in anEXEMPT = {field: reason}map. The candidate set is small and tractable today (11 fields acrossmodels/sandbox.py,models/tasks.py,models/templates.py), andisolated_paths(sandbox.py:296) is the only one with neither a validator nor a loader pass — its siblingworking_dirhas@field_validatorat sandbox.py:307 and every other host-path field is normalized at load time. The accompanying fix (a@field_validator("isolated_paths")rejecting empty/whitespace entries, entries still non-absolute afterexpanduser/expandvars, and entries still containing$afterexpandvars) mirrors the precedent already documented inresolve_template_source_paths("an unresolved variable would otherwise surface as a cryptic error, far from the actual configuration mistake"). Consider also giving the shape check a reusableAnnotated[str, AfterValidator(_require_absolute_host_path)]alias so future host-path fields satisfy the rule by construction. Prevents: A2-high (merged from axes 2/4/5/6/7/8):isolated_pathsentries get no shape validation, so an unexpanded$SKILLS_REPO_PATH/tests(the documented flagship example), a relative entry, or an empty string resolves non-strictly under the host CWD, matches no agent-visible source, and turns the entire fail-closed security declaration into a silent no-op. - [ruff] Enable mccabe
C901with a baseline. Add"C90"tolint.selectinpyproject.toml:169(currentlyE,F,I,N,W,UP,B,SIM,RUF,PLR0915,PLR0912— statement/branch caps are on, but cyclomatic complexity is not) and set[tool.ruff.lint.mccabe] max-complexity = 15. Measured on the PR HEAD worktree that yields exactly 14 offenders, so it can land with a frozenper-file-ignoresbaseline and a ratchet-down note; the headline offender isdocker_runner._build_argvat 41 (the god-function whose mount pipeline the hand-maintained visible-source enumeration duplicates) plusDockerRunner.runat 17. Atmax-complexity=20the list shrinks to 4 if a gentler first step is wanted. This is the mechanical gate that would have made the mount-seam refactor (CE034) mandatory rather than optional, and it flags the complexity inflation the redundant fourth per-plugin collection contributes to_prepare_isolated_sources/_enforce_isolated_paths. Prevents: A1-medium:_plugin_bundle_manifestsas a fourth index-parallel per-plugin collection (declared sandbox docker_runner.py:529, reset 762, appended 793, single consumer 896) pushing_enforce_isolated_pathsto radon C (11) inside a radon-D (24) enclosing method. Also structurally enables A5-high by capping_build_argv(41). - [ce-lint] CE036 — docs must not hand-maintain the agent-visible / exempt source list. Extend the existing doc-parity family (
tests/lint/doc_schema_parity.py/action_docs.pystyle, wired as a@pytest.mark.lintclass): introduce a code-side constant enumerating the source kinds the isolation guard treats as agent-visible vs grader-private (e.g.AGENT_VISIBLE_SOURCE_KINDS/GRADER_PRIVATE_SOURCE_KINDSincoder_eval/isolation/ormodels/container_paths.py), and fail the build when the prose lists indocs/TASK_DEFINITION_GUIDE.md:602("Raw task, reference, template, and plugin sources mounted below the root-only grader parent are exempt") anddocs/DOCKER_ISOLATION.md:45("Sources mounted below the root-only grader parent are exempt") do not match that constant. Both docs currently tell the task author thattemplatesources are exempt, which is false, and the same wrong claim is duplicated in two files plus the_agent_visible_mount_sourcesdocstring — three copies of one invariant with no gate. Prevents: A8-high: the docs asserttemplate_sourcesare exempt from theisolated_pathsguard, while their bytes are copied into the agent-owned workspace and chowned to the agent UID — the documented exemption is what makes the false negative look intentional to a task author. - [ce-lint] CE037 —
@model_validatorscope must match its name. New ruletests/lint/rules/ce037_model_validator_scope.py: for each@model_validator(mode="after")method insrc/coder_eval/models/, collect the distinctself.<field>roots read in the body and flag when a root is not mentioned in the method name, unless the name matches a generic escape hatch (_validate_cross_field*/_validate_*_consistency). Also require the_prefix (private) for consistency with_validate_working_dir.SandboxConfig.validate_template_sources(models/sandbox.py:564, docstring "Validate template sources configuration.") now readsself.protected_mocks,self.record_cli,self.docker.isolated_pathsandself.driver— three unrelated cross-field concerns behind a name advertising one, in a model reconstructed on every merge-layer resolution. False-positive risk is real (a legitimateself.driverread inside a template-source guard), which is exactly what the generic-name escape hatch is for: the rule's effect is to force an honest name or a split, never to forbid the check. Prevents: A1-low and A5-low (same site, two axes): theisolated_pathsdriver/agent_isolation precondition bolted ontovalidate_template_sourcesas its third unrelated concern, under a name and docstring that promise one.
Harness improvements (not statically reachable):
- Guard-wiring + ordering test, registry-driven over all fail-closed guards. Add
tests/test_security_guard_wiring.pythat, for each_enforce_*/ fail-closed preflight onDockerRunner, drives a realasyncio.run(runner.run())with the cheap recipe already proven attests/test_docker_build_failure.py:67-82(monkeypatch.setattr(dr, "_preflight", lambda: None)+ stubbed_build_image/_preflight_agent_isolation_image) plus a spy onasyncio.create_subprocess_exec, configures a violating input, and asserts (a)DockerRunErroris raised and (b) the subprocess spy was never called — i.e. the guard fires strictly before any container starts. Todaytests/test_isolated_paths.pyonly ever callsrunner._enforce_isolated_paths()directly (lines 79, 95, 109, 120, 131, 138, 161, 167); mutating the sole call siteawait asyncio.to_thread(self._enforce_isolated_paths)(docker_runner.py:635) topassleaves the full suite green (4133 passed) andmake lintgreen. Why not static: A static rule can see that line 635 exists, but it cannot prove any test exercises it, nor that the call is ordered beforedocker run— that ordering is a runtime property ofrun()'s control flow observed through a subprocess spy. Prevents: A3-high: the new fail-closed guard'srun()call site and its ordering requirement are entirely unasserted (coverage-missing range 572-674 in a module at 70.89%), so the guard could be removed outright without a single failure. - Agent-visibility inventory parity test. Add a test that builds a maximal isolated
SandboxConfigexercising every route by which host bytes reach the agent — plugin bundle mounts, the staged~/.claudecopy,sandbox.template_sources(copied into/work/agentbysandbox.py:329::_apply_template_dir_sourceand then chowned to the agent UID byorchestrator.py:2288-2295::grant_agent_workspace), starter files, and theLITELLM_COST_LOGparent mount — renders_build_argv, and asserts that every route's host source is reported by_agent_visible_mount_sources()while every remaining mount target sits underCONTAINER_GRADER_DIRor/opt/coder-eval/mock. Keep the route list as a checked-in inventory so adding a new delivery route without classifying it fails. Fliptests/test_isolated_paths.py::test_declared_tests_tree_outside_the_projection_passes(lines 154-161), which currently pins the unsafe pass for a golden-fixture tree staged viatemplate_dir, into a must-fail case. Why not static: Whether a source is agent-visible is not a syntactic property of the mount table: template sources are registered as grader-private mounts and only become agent-readable through a runtime copy + chown into the workspace. Establishing that requires constructing a runner and executing the staging path. Prevents: A8-high (template_sources fail-open, with a test pinning the unsafe pass) and A5-high (the unenumerated LITELLM_COST_LOG mount) — the same enumeration drift from two directions. - Per-module coverage floor for the isolation layer. The global 80% gate in
pyproject.tomlhides thatcoder_eval/isolation/docker_runner.pysits at 70.89% (633 stmts / 170 miss) with the wholerun()body (572-674) unexercised. Add a per-module minimum (a small--cov-report=jsonpost-check inmake verify, or acoveragecontexts assertion) for the security-critical modules —isolation/docker_runner.py,isolation/plugin_bundle.py— starting at today's measured number as a ratchet so it can only go up. Why not static: Coverage is a runtime measurement of an executed test suite; no AST or type check can produce it. Prevents: - Negative-shape fixture set for
isolated_paths, driven end to end. Add cases covering a relative entry, an empty/whitespace entry, an entry containing an unset$VAR(verified today: withSKILLS_REPO_PATHunset,"$SKILLS_REPO_PATH/tests"resolves to<cwd>/$SKILLS_REPO_PATH/testsand silently passes), and an entry under aCLAUDE_COPY_IGNOREsubtree such as~/.claude/projects(which must not be reported as an overlap, since that subtree is never copied into the container).tests/test_isolated_paths.pycurrently covers only the SET env-var case (test_env_var_in_declaration_is_expanded, lines 111-120). Why not static: CE035 can force that validation exists but cannot encode which entries should pass: the copy-ignore interaction and env-var expansion outcomes depend on process environment and on_copy_claude_home's filter at runtime. Prevents: A2-high (silent no-op on malformed declarations) and A6-low (false overlap hard-failing a run for a declared~/.claudesubtree that is filtered out of the mounted copy). - Record the naming/failure-surface contract for assertion-shaped config fields.
isolated_pathssits three lines belowextra_mountson the same model and every otherlist[str]there is an instruction the runner executes, so the schema reads as "isolate these paths" rather than "prove these are already isolated, else abort"; and its failure surface is aDockerRunErrorfromrun()(after image build, reported as an ERROR-status task), not a config error atcoder-eval plantime. Either rename to an assertion-shaped name (assert_paths_private/require_private_paths) or state both facts indocs/TASK_DEFINITION_GUIDE.md; consider moving the driver/agent_isolationprecondition check to plan-time so a misconfiguration fails before any image work. Why not static: Whether a field name reads as an assertion or an instruction is semantic judgment; no lint rule can adjudicate it (CE030 already forces the field to be documented, which is the mechanical part and is satisfied). Prevents: A7-low:isolated_pathsreading as a mount instruction, and the undocumented late/ERROR-status failure surface.
Top 5 Priority Actions
- Add every
TemplateDirSourcehost path to_agent_visible_mount_sources()(src/coder_eval/isolation/docker_runner.py:866) and fliptests/test_isolated_paths.py:154to expect a failure — today a task can declareisolated_paths: ["$SKILLS_REPO_PATH/tests"], stage those same golden fixtures viatemplate_sourcesinto the agent-owned/work/agenttree, and get a green guard while the agent reads the answers, inflating the graded score for identical agent behavior; also droptemplatefrom the exempt lists in docs/TASK_DEFINITION_GUIDE.md:602 and docs/DOCKER_ISOLATION.md:45. - Add a
@field_validator("isolated_paths")beside_validate_working_dir(src/coder_eval/models/sandbox.py:296 and :307) rejecting empty/whitespace entries, entries still non-absolute afterexpanduser/expandvars, and entries still containing$after expansion (keeping existence optional), because the non-strictresolve()at src/coder_eval/isolation/docker_runner.py:889 currently turns an unsetSKILLS_REPO_PATH, a relative entry, or""into a silent no-op — fail-open on a field documented as a fail-closed contract, and inconsistent with the load-time$VARerrorresolve_template_source_pathsalready raises for template paths. - Add the host-absolute
LITELLM_COST_LOGparent-dir mount (src/coder_eval/isolation/docker_runner.py:1480, which has noagent_isolationgate) to the visible set and correct the false 'by construction' invariant in the docstring at docker_runner.py:860-862, then eliminate the second source of truth by routing all mount rendering through one_add_mount(source, target, *, agent_visible)table read by both_build_argvand_enforce_isolated_paths, asserting every private target sits underCONTAINER_GRADER_DIRand adding a CE lint rule banning rawargv += ["-v", ...]outside that helper. - Cover the guard's wiring with a test in the style of tests/test_docker_build_failure.py:67-82 that stubs
_preflight/_build_image/_preflight_agent_isolation_image, spiesasyncio.create_subprocess_exec, and asserts an overlapping declaration raisesDockerRunErrorfromrun()with the spy never called — mutating src/coder_eval/isolation/docker_runner.py:635 topassleaves the entire suite andmake lintgreen today, so nothing proves the guard runs at all, let alone beforedocker run; also exercise themanifest.symlinksbranch and the empty-declaration short-circuit. - Fix the
~/.claudearm (src/coder_eval/isolation/docker_runner.py:869) to compare against theCLAUDE_COPY_IGNORE-filtered copy that is actually mounted rather than the live host home, so a reasonableisolated_paths: ["~/.claude/projects"]stops aborting runs on a false overlap, and take the two cheap cleanups in the same pass: drop the redundant fourth parallel_plugin_bundle_manifestslist (docker_runner.py:529/762/793) in favor of deriving the manifest at check time, and split or renamevalidate_template_sources(src/coder_eval/models/sandbox.py:564-586), which now carries three unrelated cross-field concerns.
Stats: 0 🔴 · 4 🟠 · 1 🟡 · 4 🔵 across 8 axes reviewed.

Adds
sandbox.docker.isolated_paths, a declared list of host paths that must stay agent-invisible, enforced fail-closed at run start; it stacks on the protected-mock/plugin-bundle PR because the check consults the sanitized bundle manifests that PR introduces. The intended first consumer is the skills nightly experiment, which will declare its tests tree so the fixtures stay outside everything the evaluated agent can read.How it works
The field is a contract, not a mount instruction. Nothing new is mounted and nothing is hidden as a side effect: the runner proves that no agent-visible surface (the agent-readable mounts and the sanitized plugin bundle projections) carries content from a declared path. Private grader mounts are exempt, because a raw source sitting below the root-only
/opt/coder-eval/graderparent is the isolation working as intended, not a violation.Today the plugin bundle allowlist already excludes a repo's
tests/tree. This field turns that implementation detail into a checked contract: if the allowlist grows, or a symlink starts projecting fixture content, the run stops instead of quietly grading against readable answers.How it starts
sandbox.dockerkey (experiment defaults -> task -> variant -> CLI-D sandbox.docker.isolated_paths=...), withstrategy="replace"declared on the model.SandboxConfigrequiresdriver: dockerandagent_isolation: true. A declaration that cannot be enforced is a hard error rather than a silent pass.DockerRunner.run()calls_enforce_isolated_paths()after_prepare_isolated_sourcesand_prepare_host_mounts(so every agent-visible source is known), alongside the other fail-closed isolation guards, before the docker argv is rendered and before any container starts.Algorithm
For each declared entry, expanded with
expanduser/expandvarsand resolved non-strictly (a path that does not exist has nothing to expose):DockerRunErrornaming the declared path, the resolved path, the agent-visible destination, and the host source.Otherwise the declaration holds and the run proceeds unchanged.
flowchart TD A["declared path (expanduser + expandvars, resolved)"] --> B{"overlaps an agent-visible mount source?"} B -- yes --> F1["fail: names declared path + mount source"] B -- no --> C{"a plugin bundle manifest entry under it?"} C -- yes --> F2["fail: names declared path + manifest entry"] C -- no --> D["declaration holds, run proceeds"]Validation
Run from the repo root on Windows (make is not on PATH, so the Makefile targets were replicated):
ruff format .- 373 files unchanged;ruff check- all checks passed.pyright- 0 errors (1 pre-existing warning inantigravity_agent.py).pytest tests/test_custom_lint.py) - 170 passed, 1 failed:TestCE028DocIndexParity::test_every_published_doc_is_in_the_nav, naming exactly the five untracked local scratch docs (EVAL_SECURITY_ISOLATION_PLAN.md,HARNESS_ISOLATION_PLAN_REVIEW.md,PR_87_90_CODE_REVIEW.md,TEST_DATA_SEPARATION_HANDOFF.md,TEST_DATA_SEPARATION_WORKLOG.md). None are part of this branch, so this passes in CI.tests/test_sandbox.pysymlink-privilege failures (WinError 1314) on this machine. Coverage 88.46%, above the 80% gate.tests/test_isolated_paths.py(12 tests) covers: overlap with an agent-visible mount in both containment directions, the~/.claudecopy mounted at the agent HOME,~and$VARexpansion, a declared path under a private grader mount passing, a non-existent declared path passing, a plugin bundle that projects a file under the declared path failing, both pydantic validation rejections, and resolution of the field through the layered merge versus the-Doverride path. A mutation check (short-circuiting the guard) confirmed all five raising tests fail without the enforcement.Note on scope
The enforcement enumerates the surfaces that are actually agent-visible under
agent_isolation: the plugin bundle mounts and the~/.claudecopy mounted at the agent's HOME. Auto-mounts andextra_mountsare not enumerated because the runner already refuses to render them under isolation (extra_mountsis rejected outright), and isolation is required for this field, so checking them would be unreachable code.