Skip to content

Add isolated_paths declaration to the docker driver - #96

Open
dmorosanu wants to merge 1 commit into
feat/protected-mockdfrom
feat/isolated-paths-declaration
Open

Add isolated_paths declaration to the docker driver#96
dmorosanu wants to merge 1 commit into
feat/protected-mockdfrom
feat/isolated-paths-declaration

Conversation

@dmorosanu

Copy link
Copy Markdown
Contributor

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/grader parent 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

  1. Config merge resolves the field like any other sandbox.docker key (experiment defaults -> task -> variant -> CLI -D sandbox.docker.isolated_paths=...), with strategy="replace" declared on the model.
  2. Pydantic validation on SandboxConfig requires driver: docker and agent_isolation: true. A declaration that cannot be enforced is a hard error rather than a silent pass.
  3. DockerRunner.run() calls _enforce_isolated_paths() after _prepare_isolated_sources and _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/expandvars and resolved non-strictly (a path that does not exist has nothing to expose):

  1. Overlap check against every agent-visible mount source, in either direction of containment. A hit raises DockerRunError naming the declared path, the resolved path, the agent-visible destination, and the host source.
  2. Manifest check against each staged plugin bundle: if any manifest entry lies under (or is) the declared path, the projection would expose declared content, and the error names the offending manifest entry.

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"]
Loading

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 in antigravity_agent.py).
  • Custom lint (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.
  • Full suite with coverage - 3973 passed, 105 skipped (POSIX-only), 2 failed: the pre-existing tests/test_sandbox.py symlink-privilege failures (WinError 1314) on this machine. Coverage 88.46%, above the 80% gate.
  • New file tests/test_isolated_paths.py (12 tests) covers: overlap with an agent-visible mount in both containment directions, the ~/.claude copy mounted at the agent HOME, ~ and $VAR expansion, 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 -D override 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 ~/.claude copy mounted at the agent's HOME. Auto-mounts and extra_mounts are not enumerated because the runner already refuses to render them under isolation (extra_mounts is rejected outright), and isolation is required for this field, so checking them would be unreachable code.

@github-actions

github-actions Bot commented Aug 10, 2026

Copy link
Copy Markdown

Claude finished @dmorosanu's task in 1m 24s —— View job


Code Review in Progress

Todo List:

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Examine the full diff (git diff origin/feat/protected-mockd...HEAD)
  • Read each changed file in full for context
  • Perform cross-file consistency checks
  • Analyze "what's missing"
  • Design-level scrutiny
  • Post comprehensive review feedback

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. [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 bare isolated_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 raises working_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_path in orchestration/task_loader.py:126-166, the last two of which additionally require the path to exist). isolated_paths gets 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, with SKILLS_REPO_PATH unset: '$SKILLS_REPO_PATH/tests' -> <cwd>/$SKILLS_REPO_PATH/tests; 'tests' -> <cwd>/tests; '' -> <cwd>. Combined with the deliberate A declared path that does not exist on the host has nothing to expose, so it resolves non-strictly and passes rule (docstring, docker_runner.py:880-881) plus test_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-only Not forwarding unstaged SKILLS_REPO_PATH... at 1452). So a run on a machine that forgot to export SKILLS_REPO_PATH reports no error and enforces nothing. Fix: add a @field_validator("isolated_paths") next to _validate_working_dir that (a) rejects an empty/whitespace entry, (b) rejects an entry that is still not absolute after expanduser/expandvars, and (c) rejects an entry that still contains $ after expandvars (unresolved variable). Existence must stay optional, but unresolvability must not be silently equivalent to it. Add tests for the relative, empty, and unset-$VAR cases — tests/test_isolated_paths.py currently only covers the SET env-var case (test_env_var_in_declaration_is_expanded, line 112-121).
  2. [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 is await asyncio.to_thread(self._enforce_isolated_paths) and sits inside the coverage-missing range 572-674 (verified: uv run pytest tests/ --cov=coder_eval.isolation.docker_runner --cov-report=term-missing633 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.py only ever calls the guard directly (lines 79, 95, 109, 120, 131, 138, 161, 167) — nothing drives run(), 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-82 already shows the cheap recipe: monkeypatch.setattr(dr, "_preflight", lambda: None) + stub _build_image, then asyncio.run(runner.run()). Add a test that stubs _preflight/_build_image/_preflight_agent_isolation_image and asyncio.create_subprocess_exec (to a sentinel-raising spy), configures an overlapping isolated_paths, and asserts DockerRunError is raised AND the subprocess spy was never called — i.e. the guard fires strictly before docker run.
  3. [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 -v sites in _build_argv (grep argv += ["-v" → lines 1480, 1501, 1505, 1513, 1525, 1528, 1530, 1532, 1571, 1604). Under agent_isolation (which isolated_paths requires), 1571 (_auto_mount) and 1604 (extra_mounts) are dead (extra_mounts hard-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 on LITELLM_COST_LOG being set, LITELLM_COST_LOG being in env_passthrough (it is, by default, models/sandbox.py:263) and network != "none"; there is no agent_isolation gate. 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 -v through 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 target startswith(CONTAINER_GRADER_DIR), and add a CEnnn lint rule that fails on a raw argv += ["-v", ...] outside that helper.
  4. [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 ~/.claude copy:
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

  1. [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

  1. [Axis 1] isolated_paths validation is bolted onto a model validator named validate_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 -v sites, 1480-1571) plus _prepare_isolated_sources' template branch (813) — the parallel paths were not updated, so the host-absolute LITELLM_COST_LOG parent mount (1480) and template-source trees copied into /work/agent are invisible to the guard; every future mount added to _build_argv shrinks 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=tempdir now become hard failures for any task declaring isolated_paths — I reproduced both: the variant case raises a raw pydantic ValidationError inside resolve_task_for_variant, which experiment.py:709 collects as a resolution error and skips the task, so the tempdir cell of an A/B matrix quietly disappears instead of running; the -D case raises OverrideError. Either document the required -D sandbox.docker.isolated_paths='[]' escape hatch (working_dir gets silently ignored instead) or make the driver conflict an explicit, guided error like validate_early_stop's. (trigger: src/coder_eval/models/sandbox.py)
  • 🟡 No coder-eval plan surface for the new declaration, breaking the project's "every resolution-time guardrail is a hard error at plan and run" convention (plan_command.py:138 calls validate_early_stop for exactly this reason): only the shape checks in SandboxConfig reach plan, while the actual overlap proof lives run-only at docker_runner.py:635 — the cheap subset (declaration vs each plugin path and each TemplateDirSource.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_paths is called unconditionally at docker_runner.py:635 but its visible set is only meaningful under agent_isolation: with isolation off, _prepare_isolated_sources never runs (688), so _agent_plugin_mounts/_plugin_bundle_manifests are empty and every declaration passes vacuously while _auto_mount/extra_mounts expose 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:208 mutates SandboxConfig via model_copy, which skips validators). Add a defensive if not self._docker_config.agent_isolation: raise inside 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-82 already shows the stub-_preflight/_build_image recipe needed to assert the raise happens before asyncio.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/tests example) 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 to recover_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)
  • 🔵 TestConfigMerge only pins the happy path (experiment-defaults layer == -D layer). Missing: the conflict cases I reproduced — a variant driver: tempdir (task-skipping ValidationError) and -D sandbox.docker.agent_isolation=false (OverrideError) against a task that declares isolated_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_result was not extended for it: the DockerRunError from docker_runner.py:635 is raised after run_dir.mkdir (576) but before any _write_synthetic_task_json, so the run dir gets no task.json and no docker.log; the task is recorded only run-level as FinalStatus.ERROR with the misleading description Failed to load task from <file>: DockerRunError. Consequence: recover_task_results (batch.py:428 rglobs task.json) and therefore coder-eval report <run_dir> regeneration drop the breached task entirely, and --resume re-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: DockerBuildError got FinalStatus.BUILD_FAILED plus entries in models/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 generic ERROR and 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 declares isolated_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 resolved sandbox.docker dump 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 export SKILLS_REPO_PATH the 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 through template_sources passes 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 (class SingleMountSeam, wired into ALL_RULES in tests/lint/runner.py; next free id after CE033): inside src/coder_eval/isolation/, forbid a "-v" string literal appearing in any argv += [...] / argv.extend([...]) / list-literal assignment outside one designated helper (DockerRunner._add_mount). AST shape: walk AugAssign/Call(attr='extend'|'append') whose target name matches argv, flag when any element is Constant("-v") and the enclosing FunctionDef.name != "_add_mount". Verified today there are 9 such raw mount sites in docker_runner.py::_build_argv (lines 1480, 1501, 1505, 1513, 1525, 1528, 1530, 1532 plus the argv.extend(["-v", ...]) at 1571) and a 10th in the extra_mounts loop 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_paths read the same list, so a new mount cannot silently shrink the guard, and the ~/.claude entry naturally records the actually-mounted staged copy (self._claude_mount_src) instead of the raw host Path.home() / ".claude" at line 869. Pair it with an assert in _add_mount that every agent_visible=False target starts with CONTAINER_GRADER_DIR or /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 no agent_isolation gate, 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 ~/.claude arm 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.lint class backed by tests/lint/host_path_fields.py, not a per-AST BaseRule): for each field on the YAML-facing models whose name matches *path|*paths|*dir|*dirs|*file|*files or whose description contains "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 of orchestration/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 an EXEMPT = {field: reason} map. The candidate set is small and tractable today (11 fields across models/sandbox.py, models/tasks.py, models/templates.py), and isolated_paths (sandbox.py:296) is the only one with neither a validator nor a loader pass — its sibling working_dir has @field_validator at 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 after expanduser/expandvars, and entries still containing $ after expandvars) mirrors the precedent already documented in resolve_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 reusable Annotated[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_paths entries 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 C901 with a baseline. Add "C90" to lint.select in pyproject.toml:169 (currently E,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 frozen per-file-ignores baseline and a ratchet-down note; the headline offender is docker_runner._build_argv at 41 (the god-function whose mount pipeline the hand-maintained visible-source enumeration duplicates) plus DockerRunner.run at 17. At max-complexity=20 the 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_manifests as a fourth index-parallel per-plugin collection (declared sandbox docker_runner.py:529, reset 762, appended 793, single consumer 896) pushing _enforce_isolated_paths to 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.py style, wired as a @pytest.mark.lint class): 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_KINDS in coder_eval/isolation/ or models/container_paths.py), and fail the build when the prose lists in docs/TASK_DEFINITION_GUIDE.md:602 ("Raw task, reference, template, and plugin sources mounted below the root-only grader parent are exempt") and docs/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 that template sources are exempt, which is false, and the same wrong claim is duplicated in two files plus the _agent_visible_mount_sources docstring — three copies of one invariant with no gate. Prevents: A8-high: the docs assert template_sources are exempt from the isolated_paths guard, 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_validator scope must match its name. New rule tests/lint/rules/ce037_model_validator_scope.py: for each @model_validator(mode="after") method in src/coder_eval/models/, collect the distinct self.<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 reads self.protected_mocks, self.record_cli, self.docker.isolated_paths and self.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 legitimate self.driver read 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): the isolated_paths driver/agent_isolation precondition bolted onto validate_template_sources as 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.py that, for each _enforce_* / fail-closed preflight on DockerRunner, drives a real asyncio.run(runner.run()) with the cheap recipe already proven at tests/test_docker_build_failure.py:67-82 (monkeypatch.setattr(dr, "_preflight", lambda: None) + stubbed _build_image / _preflight_agent_isolation_image) plus a spy on asyncio.create_subprocess_exec, configures a violating input, and asserts (a) DockerRunError is raised and (b) the subprocess spy was never called — i.e. the guard fires strictly before any container starts. Today tests/test_isolated_paths.py only ever calls runner._enforce_isolated_paths() directly (lines 79, 95, 109, 120, 131, 138, 161, 167); mutating the sole call site await asyncio.to_thread(self._enforce_isolated_paths) (docker_runner.py:635) to pass leaves the full suite green (4133 passed) and make lint green. 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 before docker run — that ordering is a runtime property of run()'s control flow observed through a subprocess spy. Prevents: A3-high: the new fail-closed guard's run() 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 SandboxConfig exercising every route by which host bytes reach the agent — plugin bundle mounts, the staged ~/.claude copy, sandbox.template_sources (copied into /work/agent by sandbox.py:329::_apply_template_dir_source and then chowned to the agent UID by orchestrator.py:2288-2295::grant_agent_workspace), starter files, and the LITELLM_COST_LOG parent 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 under CONTAINER_GRADER_DIR or /opt/coder-eval/mock. Keep the route list as a checked-in inventory so adding a new delivery route without classifying it fails. Flip tests/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 via template_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.toml hides that coder_eval/isolation/docker_runner.py sits at 70.89% (633 stmts / 170 miss) with the whole run() body (572-674) unexercised. Add a per-module minimum (a small --cov-report=json post-check in make verify, or a coverage contexts 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: with SKILLS_REPO_PATH unset, "$SKILLS_REPO_PATH/tests" resolves to <cwd>/$SKILLS_REPO_PATH/tests and silently passes), and an entry under a CLAUDE_COPY_IGNORE subtree 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.py currently 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 ~/.claude subtree that is filtered out of the mounted copy).
  • Record the naming/failure-surface contract for assertion-shaped config fields. isolated_paths sits three lines below extra_mounts on the same model and every other list[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 a DockerRunError from run() (after image build, reported as an ERROR-status task), not a config error at coder-eval plan time. Either rename to an assertion-shaped name (assert_paths_private / require_private_paths) or state both facts in docs/TASK_DEFINITION_GUIDE.md; consider moving the driver/agent_isolation precondition 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_paths reading as a mount instruction, and the undocumented late/ERROR-status failure surface.

Top 5 Priority Actions

  1. Add every TemplateDirSource host path to _agent_visible_mount_sources() (src/coder_eval/isolation/docker_runner.py:866) and flip tests/test_isolated_paths.py:154 to expect a failure — today a task can declare isolated_paths: ["$SKILLS_REPO_PATH/tests"], stage those same golden fixtures via template_sources into the agent-owned /work/agent tree, and get a green guard while the agent reads the answers, inflating the graded score for identical agent behavior; also drop template from the exempt lists in docs/TASK_DEFINITION_GUIDE.md:602 and docs/DOCKER_ISOLATION.md:45.
  2. 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 after expanduser/expandvars, and entries still containing $ after expansion (keeping existence optional), because the non-strict resolve() at src/coder_eval/isolation/docker_runner.py:889 currently turns an unset SKILLS_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 $VAR error resolve_template_source_paths already raises for template paths.
  3. Add the host-absolute LITELLM_COST_LOG parent-dir mount (src/coder_eval/isolation/docker_runner.py:1480, which has no agent_isolation gate) 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_argv and _enforce_isolated_paths, asserting every private target sits under CONTAINER_GRADER_DIR and adding a CE lint rule banning raw argv += ["-v", ...] outside that helper.
  4. 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, spies asyncio.create_subprocess_exec, and asserts an overlapping declaration raises DockerRunError from run() with the spy never called — mutating src/coder_eval/isolation/docker_runner.py:635 to pass leaves the entire suite and make lint green today, so nothing proves the guard runs at all, let alone before docker run; also exercise the manifest.symlinks branch and the empty-declaration short-circuit.
  5. Fix the ~/.claude arm (src/coder_eval/isolation/docker_runner.py:869) to compare against the CLAUDE_COPY_IGNORE-filtered copy that is actually mounted rather than the live host home, so a reasonable isolated_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_manifests list (docker_runner.py:529/762/793) in favor of deriving the manifest at check time, and split or rename validate_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants