Skip to content

feat(sandbox): per-source template exclude_patterns and host-side grading convention - #91

Open
dmorosanu wants to merge 1 commit into
mainfrom
feat/template-exclude-patterns
Open

feat(sandbox): per-source template exclude_patterns and host-side grading convention#91
dmorosanu wants to merge 1 commit into
mainfrom
feat/template-exclude-patterns

Conversation

@dmorosanu

Copy link
Copy Markdown
Contributor

What

Closes the template path by which grading material reaches an evaluated agent's sandbox.

  1. TemplateDirSource.exclude_patterns - a new per-source list of template-relative globs that are never copied into the sandbox. Exclusion is terminal: it beats include_patterns and beats a !-negated ignore_patterns entry, so an excluded path cannot be brought back by either. Matching mirrors include_patterns exactly (fnmatchcase on the posix form, * spans /, leading ./ stripped); the validator is now shared between the two fields and rejects absolute paths, .. segments, and empty entries.

  2. Task-authoring convention docs - new "Grading Assets and the Sandbox Boundary" section in docs/TASK_DEFINITION_GUIDE.md: grading assets belong next to the task YAML (never copied into the sandbox) and are reached host-side via TASK_DIR in run_command/pre_run/post_run, $TASK_DIR/ entries in llm_judge/agent_judge files:, and reference:. Includes a "Test-data separation and its limits" note stating plainly that under the tempdir driver the agent runs as the host user, so these mechanisms are hygiene that removes every path the agent is pointed at, not containment (containment is the docker UID/GID work; transcript-level detection is separate).

  3. Two in-repo example tasks fixed:

    • tasks/fibonacci_with_template.yaml graded by running python -m pytest tests/ inside the sandbox, so the oracle was agent-writable. It now runs the pristine host copy of the same tests (resolved under TASK_DIR) against the sandbox's src/.
    • tasks/mock_path_dirs_template_dir/task.yaml asserted strings whose template was literally present in the mock scripts copied into the sandbox. The mocks now compose their receipt at runtime from the invocation (argument count and argument lengths), so no asserted line exists verbatim in anything that ships, and the prompt no longer points the agent at the mock sources.

Why

Evaluated agents read grading material when it ships into their sandbox, and a criterion that can be satisfied by reading rather than doing stops measuring the skill the task was written for. Sibling PRs cover plugin exposure (#89) and mock fixtures (#90); this one covers templates.

Notes on the fibonacci fix

  • templates/fibonacci-starter/pytest.ini (new, empty [pytest]) pins the template as its own pytest rootdir. Without it, pointing pytest at the host tests makes it discover the enclosing repo's pyproject.toml and fail on -n auto / unknown ini options.
  • TASK_DIR is read via os.environ inside python -c rather than as shell $TASK_DIR. run_command uses shell=True, which is cmd.exe on Windows where $TASK_DIR does not expand - the shell form silently graded every run as a failure there. The env-read form behaves identically under sh and cmd.exe.
  • -B and -p no:cacheprovider stop the grading run from writing bytecode or a cache directory into the host template.

Validation

  • New unit tests in tests/test_sandbox_templates.py: exclude beats include, exclude beats !-negated default-ignore, plain nested exclusion, validator rejection of absolute/../empty patterns.
  • Fibonacci criterion exercised end to end through a real Sandbox (setup, run_command) on Windows/cmd.exe and again under sh, across four states: untouched stub (fail), sandbox tests neutered plus wrong implementation (still fail - proves the host copy is what grades), correct implementation with sandbox tests neutered (pass), correct implementation with the sandbox tests/ deleted outright (pass). No files written into the repo working tree by the grading run.
  • Mock receipts verified by executing both scripts under sh; output matches the asserted strings exactly.
  • Both edited task YAMLs load through task_loader.load_task; tests/test_yaml_migration.py (loads every tasks/**/*.yaml) passes.
  • ruff format --check, ruff check, pyright (0 errors; 1 pre-existing warning in antigravity_agent.py), custom lint (171 passed), full suite 3925 passed, 102 skipped, coverage 90.39% (gate 80%). The 2 failures are the known pre-existing Windows symlink-privilege cases (test_build_run_command_env_preserves_external_plugin_tools_dir, test_capture_to_copies_and_tolerates_dangling_symlink).

@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:91

Scope: pr:91 · branch feat/template-exclude-patterns · 3b31224 · 2026-08-06T17:49Z · workflow variant

Change class: complex — adds a new per-source template exclude_patterns field with terminal precedence semantics over include/ignore patterns, changes the template-copy control flow in Sandbox._apply_template_dir_source, and rewrites a task's grading command to execute a host-side pristine test copy; correctness requires reasoning about pattern matching precedence and host/sandbox boundary.

Architecture, error handling, and harness design are excellent (10/10 across three axes) and the new exclude_patterns feature is well-tested in the shapes it covers, but the grading boundary this PR exists to establish is not actually airtight — a sandbox-writable pytest.py can force a passing verdict, a bare-directory exclude silently copies the whole grading tree in anyway, and the docs oversell both TASK_DIR and docker "containment" — so the bottom line is a healthy codebase with a handful of fail-open trust-boundary gaps that must be closed before task authors rely on the documented guarantees.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 9 / 10 0 0 2 0 Docs' exclude_patterns grading-oracle example **/*.expected cannot match template-root files under fnmatch, contradicting the semantics stated at docs:584 and leaking top-level grading oracles
2. Type Safety 9.9 / 10 0 0 0 1 info.field_name (`str
3. Test Health 9.5 / 10 0 0 1 0 exclude_patterns test coverage gaps in tests/test_sandbox_templates.py: only the both-patterns form is exercised (directory-only pattern and non-default mount_point interaction untested)
4. Security 8 / 10 0 2 0 0 Grading command's import pytest resolves from the agent-writable sandbox cwd, so a dropped pytest.py stub forces a passing verdict (and the 'graded against the pristine host copy' contract is untested)
5. Architecture & Design 10 / 10 0 0 0 0
6. Error Handling & Resilience 10 / 10 0 0 0 0
7. API Surface & Maintainability 7.9 / 10 0 2 0 1 Docs claim TASK_DIR is set for pre_run / post_run, but _run_command_list spawns them with no env= (orchestrator.py:2165)
8. Evaluation Harness Quality 10 / 10 0 0 0 0

Overall Score: 9.3 / 10 · Weakest Axis: API Surface & Maintainability at 7.9 / 10
Totals: 🔴 0 · 🟠 4 · 🟡 3 · 🔵 2 across 8 axes.

Blockers

  1. [Axis 4] Grading command's import pytest resolves from the agent-writable sandbox cwd, so a dropped pytest.py stub forces a passing verdict (and the 'graded against the pristine host copy' contract is untested) (tasks/fibonacci_with_template.yaml:42) — Line 42-44 is command: >- / python -B -c "import os, pathlib, sys, pytest; ... sys.exit(pytest.main([...]))". sandbox.run_command executes it with cwd=self.sandbox_dir (src/coder_eval/sandbox.py:1079) and python -c prepends '' (cwd) to sys.path, so import pytest resolves to <sandbox>/pytest.py if the agent wrote one. Reproduced: with src/main.py returning 999 and a 5-line <sandbox>/pytest.py defining def main(args=None): return 0, the exact command exits 0 (criterion passes); removing the shim gives 3 failed. This defeats the stated guarantee in the YAML comment on line 31-32 ("so edits to the sandbox copy cannot move the bar") and lets agent-controlled code run in the grading process, which inherits the parent environment (_build_run_command_env, src/coder_eval/sandbox.py:941: "Inherit parent env (so agent tools / credentials remain reachable)"). Fix: keep cwd off the front of sys.path — run python -P -B -c "import os, pathlib, sys; sys.path.append(os.getcwd()); import pytest; ..." (append, so the agent's src/ is still importable but stdlib/pytest are not shadowable), and set PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 so a plugin pip-installed by the agent into the sandbox .venv cannot register a pytest11 hook that forces a pass. CVSS:3.1/AV:L/AC:L/PR:N/UI:N/S:C/C:N/I:H/A:N
  2. [Axis 4] exclude_patterns has no subtree-prune semantics: a bare directory pattern withholds only the directory entry while its entire contents (grading material) still copy into the sandbox (src/coder_eval/sandbox.py:361) — Line 361 is if self._matches_template_exclude_pattern(rel_path, source.exclude_patterns): continue, and _matches_template_exclude_pattern (line 629) delegates to _matches_template_include_pattern, which only does fnmatch.fnmatchcase(rel_path.as_posix(), pattern) on the full relative path. Because template_path.rglob("*") (line 354) enumerates descendants independently of their parent, skipping the grading directory entry does not skip grading/expected.json — and dest_path.parent.mkdir(parents=True, exist_ok=True) (line 415) recreates the directory anyway. Reproduced against Sandbox.setup(): exclude_patterns=['grading'] → sandbox contains grading/expected.json and grading/fixtures/oracle.txt; ['grading/*'] → nothing leaks. Every comparable tool (gitignore, .dockerignore, rsync --exclude) prunes the subtree, so exclude_patterns: ['grading'] is the natural authoring shape and it fails open silently with no warning. Note the PR's own test at tests/test_sandbox_templates.py:352 has to pass exclude_patterns=["grading", "grading/*"] to make the assertion hold — the belt-and-braces pattern is masking the defect rather than covering it. Fix: after an exclude match on a directory entry, prune the subtree (match rel_path and every ancestor against the exclude list, e.g. any(self._matches_template_exclude_pattern(p, pats) for p in [rel_path, *rel_path.parents] if p != Path('.'))), and add a test asserting exclude_patterns=['grading'] alone withholds the descendants. CVSS:3.1/AV:L/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:N
  3. [Axis 7] Docs claim TASK_DIR is set for pre_run / post_run, but _run_command_list spawns them with no env= (orchestrator.py:2165) (docs/TASK_DEFINITION_GUIDE.md:664) — The new table row asserts: "| run_command criteria (and pre_run / post_run) | The TASK_DIR environment variable is set for every command, so \"$TASK_DIR/verifier/check.py\" resolves host-side …|". TASK_DIR is exported in exactly one place — Sandbox._build_run_command_env (src/coder_eval/sandbox.py:986, env["TASK_DIR"] = str(self.task_dir)), which is consumed only by Sandbox.run_command (sandbox.py:1069, env = self._build_run_command_env()). pre_run/post_run do NOT go through it: Orchestrator._run_command_list spawns them with await asyncio.create_subprocess_shell(cmd.command, cwd=str(sandbox_dir), stdout=…, stderr=…, limit=…) (src/coder_eval/orchestrator.py:2180-2186) with no env= argument, so they inherit the orchestrator's os.environ, and nothing in orchestrator.py or sandbox.py ever assigns os.environ["TASK_DIR"]. Fix the docs: restrict the row to run_command criteria, or (preferred, since the doc is teaching a convention) plumb the sandbox env into _run_command_list by passing env=self.sandbox._build_run_command_env() (promoting it to a public accessor) so the documented contract actually holds for all three surfaces. Note the failure mode is silent, not loud: under sh, $TASK_DIR expands to the empty string, so a documented "$TASK_DIR/verifier/check.py" becomes /verifier/check.py — and for pre_run, whose fail_on_error defaults to True, that lands the whole run as FinalStatus.ERROR with a misleading reason.
  4. [Axis 7] docs:689 'Containment' claim is false: the docker driver bind-mounts the task dir and every template_dir (unfiltered by exclude_patterns) read-only at identical host paths and ships no UID/GID isolation (docs/TASK_DEFINITION_GUIDE.md:689) — Line 689 asserts: "- Containment - the docker driver's UID/GID isolation, which makes host paths genuinely unreachable rather than merely unadvertised." and line 660 asserts "The task directory is never copied into the sandbox, so the agent has no path to it". Under the docker driver both grading-asset locations this new section is about are bind-mounted into the agent's own container at their identical host paths: the task dir at src/coder_eval/isolation/docker_runner.py:1176 (argv += ["-v", f"{host_task_dir}:{host_task_dir}:ro"]) and every template dir at docker_runner.py:1233-1236 (for source in … if isinstance(source, TemplateDirSource): _auto_mount(source.path)argv.extend(["-v", f"{target}:{target}:ro"]) at line 1226). The consequence for the new public field is concrete: exclude_patterns withholds a path only from the sandbox copy made in Sandbox._apply_template_dir_source, while the whole unfiltered template tree — including the excluded grading files — stays readable by the agent at the template's absolute host path inside the container. So the field's own description ("Use it to keep grading material (expected outputs, reference solutions, verifier fixtures) out of the agent's working copy", src/coder_eval/models/templates.py:59-67) is honoured only on tempdir, and the docs point the reader at exactly the driver where the boundary is weakest. Either scope the claims ("under docker the task dir and every template_dir path are mounted read-only at their host paths, so grading assets there ARE reachable — docker isolates writes and UID, not reads of these mounts"), or stop auto-mounting the template source dir when the source declares exclude_patterns (copy the filtered tree into the container instead).

Non-blocking, but please consider before merge

  1. [Axis 1] Docs' exclude_patterns grading-oracle example **/*.expected cannot match template-root files under fnmatch, contradicting the semantics stated at docs:584 and leaking top-level grading oracles (docs/TASK_DEFINITION_GUIDE.md:594) — Line 588 states the rule: "Both take template-relative glob patterns matched with fnmatch, where * does not stop at /". Line 594 then gives exclude_patterns: ["grading", "grading/*", "**/*.expected"]. Under fnmatch, ** is just two *s and the literal / in the pattern is still required, so **/*.expected matches only paths that contain a slash. Verified directly: fnmatch.fnmatchcase('report.expected', '**/*.expected')False; fnmatch.fnmatchcase('a/report.expected', '**/*.expected')True; fnmatch.translate('**/*.expected')(?s:(?>.*?/).*\.expected)\z. A task author who copies this example leaves every top-level *.expected grading file in the sandbox — the exact failure the section exists to prevent. Under the documented semantics the correct recursive form is simply *.expected (fnmatch.fnmatchcase('report.expected', '*.expected')True). Change the example to exclude_patterns: ["grading", "grading/*", "*.expected"] and add one sentence saying gitignore's ** has no special meaning here. (tests/test_sandbox_templates.py:379 propagates the same ** idiom in include_patterns=["tools/*/dist", "tools/*/dist/**"]; that one happens to work, but it teaches the same wrong mental model.)
  2. [Axis 1] mock_path_dirs anti-shortcut/anti-gaming rationale is restated across four shipped files and overstates what the mocks actually enforce (the scripts ship into the sandbox, so the asserted receipts are statically derivable) (tasks/mock_path_dirs_template_dir/mock-cli-bins/mocks/say_hello:2) — The same paragraph is now restated in four places: mocks/say_hello:2-5, mocks/echo_args:2-4, mock-cli-bins/README.md:22-25, and task.yaml:32-35 — four copies of one rationale that must now be kept in sync. The claim itself is also stronger than the code supports. say_hello:4-5 asserts "Reading the source is not a substitute for executing the mock", but the receipt is fully deterministic and derivable from the shipped source: line 11 is printf 'MOCK_PATH_OK say_hello argc=%s argchars=%s\n' "$argc" "$argchars", and argc/argchars are plain $# / ${#arg} arithmetic over the arguments the prompt itself specifies (say_hello worldargc=1 argchars=5), so the asserted string is reconstructible by reading alone. mock-cli-bins/README.md is copied into the sandbox root (task.yaml:14-16 copies the whole ./mock-cli-bins tree), so its new paragraph hands the agent the composition rule as well. Keep one authoritative copy of the rationale (the task.yaml comment, which never enters the sandbox), delete the other three, and either soften the claim to what is true ("the expected text does not appear verbatim") or use the exclude_patterns this PR just added to withhold README.md from the copy.
  3. [Axis 3] exclude_patterns test coverage gaps in tests/test_sandbox_templates.py: only the both-patterns form is exercised (directory-only pattern and non-default mount_point interaction untested) (tests/test_sandbox_templates.py:353) — All four new tests use pattern forms that happen to work, so the feature's principal footgun is masked. test_template_exclude_patterns_withhold_nested_paths at line 353 writes exclude_patterns=["grading", "grading/*"] — the both-patterns form. Nothing covers the natural single-pattern form a task author will write first.

Verified against the PR state with a real Sandbox.setup(): exclude_patterns=["grading"] yields ['grading/expected.json', 'grading/fixtures/oracle.txt', 'src/main.py'] — the entire grading tree lands in the sandbox. The continue at src/coder_eval/sandbox.py:361-362 skips only the directory entry; rglob("*") still yields its children and dest_path.parent.mkdir(parents=True, exist_ok=True) (line 418) recreates the directory. The one failure mode the feature exists to prevent therefore succeeds silently, with a green test suite.

Second instance of the same gap: the doc example added at docs/TASK_DEFINITION_GUIDE.md:594, exclude_patterns: ["grading", "grading/*", "**/*.expected"], is never exercised. fnmatch.fnmatchcase("foo.expected", "**/*.expected") is False (only a/foo.expected matches), so a top-level answers.expected copies through despite the documented pattern.

Fix: add a test asserting the directory-only form's actual behavior (either pin the leak as known-and-documented, or make a directory exclude prune its subtree and assert that), plus a test that walks the exact pattern list from the docs example over a fixture containing both answers.expected and nested/answers.expected. Pattern semantics that decide whether grading material reaches the agent should be pinned by assertion, not by prose.

Nits

  1. [Axis 2] info.field_name (str | None) is interpolated unnarrowed into three validator messages, and no test pins the field name (src/coder_eval/models/templates.py:85) — src/coder_eval/models/templates.py:85 binds the optional directly and lines 88/90/92 interpolate it:
    def _validate_patterns(cls, v: list[str], info: ValidationInfo) -> list[str]:
        field = info.field_name
        ...
                raise ValueError(f"{field} entries must not be empty")

ValidationInfo.field_name is annotated str | None (confirmed by reading the installed pydantic 2.12.5 pydantic_core.core_schema.ValidationInfo protocol), and field is never narrowed. Runtime is correct today — I executed the model at PR state and got include_patterns entries must not be empty / exclude_patterns entries must be relative, got: '/x' — but the type system permits None entries must not be empty, and no static check can catch that (f-strings accept object; pyright reports 0 diagnostics on this file in standard mode, and strict would not differ). The two validator tests, tests/test_sandbox_templates.py:331 and :418, match only on "must be relative" / "must not be empty" / "must not contain '\.\.', so the interpolated prefix is unasserted and a regression to None would ship green.

Fix: narrow at the binding — field = info.field_name or "pattern" — and tighten one assertion per field to include the prefix, e.g. pytest.raises(ValueError, match="exclude_patterns entries must be relative"). That converts the untestable Optional into a checked contract and makes the generalized two-field validator's whole point (naming the offending field) actually load-bearing.
2. [Axis 7] Public exclude_patterns field description points at a private method name task authors cannot see (src/coder_eval/models/templates.py:66) — The field description ends "copy. \` does not stop at `/`, see `_matches_template_exclude_pattern`."(line 66). This description is the user-facing surface — it is what shows up in the generated JSON schema and in any YAML-schema tooling a task author uses — and it dead-ends on a privateSandboxmethod that is not importable, not documented, and (per the new docstring at src/coder_eval/sandbox.py:629-637) just delegates to_matches_template_include_pattern. Replace the pointer with the fact itself (e.g. "matched with fnmatch, so crosses/and**has no special meaning; a directory name does not imply its contents") and/or link the doc sectiondocs/TASK_DEFINITION_GUIDE.md#per-source-include_patterns-and-exclude_patterns. The same nit exists on include_patterns(line 56,see `_matches_template_include_pattern`.`) and is pre-existing, but this PR replicates it into a second field rather than fixing it.

What's Missing

Parallel paths:

  • 🟠 The new exclude_patterns filter has exactly one consumer — Sandbox._apply_template_dir_source (sandbox.py:361). The docker driver's parallel path was not updated: docker_runner.py:1234-1237 auto-mounts every TemplateDirSource.path (and the task dir at :1176) read-only at its identical host path, so under driver: docker the unfiltered template tree — excluded grading oracles included — stays readable in-container even though the sandbox copy is filtered. Either skip the auto-mount for sources declaring exclude_patterns (copying the filtered tree in instead) or scope the field description/docs to tempdir. (trigger: src/coder_eval/sandbox.py) (restates: Axis 7: docs:689 'Containment' claim is false — docker bind-mounts task dir and template dirs unfiltered)
  • 🟡 exclude_patterns was added only to TemplateDirSource; the sibling RepoSource still clones wholesale via _apply_repo_source (sandbox.py:249-268, git clone with no ignore/include/exclude filtering at all), and StarterFilesSource has no equivalent. The new docs sentence "Where a template mixes working files and grading files in one tree, exclude_patterns withholds the grading paths" (TASK_DEFINITION_GUIDE.md:681) is therefore unimplementable for a repo-sourced task — the most likely shape for a real project that carries its own tests/answers. Either add the same filter to RepoSource or scope the guidance to template_dir explicitly. (trigger: src/coder_eval/models/templates.py)
  • 🟡 The anti-shortcut rewrite was applied to mock_path_dirs_template_dir but not to its sibling tasks/mock_path_dirs_smoke.yaml, which still ships an inline starter_files mock whose body is echo "MOCK_PATH_OK from say_hello, args=$*" while the criterion asserts includes: ["MOCK_PATH_OK from say_hello"] — the criterion string sits verbatim in a file the agent can read, i.e. exactly the gaming path this PR closed in the template_dir twin. Apply the same runtime-computed receipt (or drop the claim that this shape is hardened). (trigger: tasks/mock_path_dirs_template_dir/task.yaml)
  • 🔵 tasks/fibonacci_with_template.yaml now names the template location twice in two idioms — path: "../templates/fibonacci-starter" (line 20, resolved relative to the task YAML) and pathlib.Path(os.environ['TASK_DIR']).parent / 'templates' / 'fibonacci-starter' (line 43, re-derived by hand). Renaming or moving the template updates one and silently leaves the grader pointing elsewhere. If this convention is to be reused, expose the resolved template root to run_command (e.g. a TEMPLATE_DIR-style env or a documented helper) instead of duplicating the path arithmetic in every task that adopts it. (trigger: tasks/fibonacci_with_template.yaml)

Tests:

  • 🟠 The PR's headline invariant — "grading runs the PRISTINE host copy … so edits to the sandbox copy cannot move the bar" (tasks/fibonacci_with_template.yaml:29-32) — is asserted only in a YAML comment and exercised only by a live agent run. No test drives the new command over a synthetic sandbox to show that (a) a correct src/main.py passes, (b) a wrong one fails, and (c) rewriting the sandbox's tests/test_main.py to assert True does not flip the verdict. That missing test is precisely the one that would have caught the grader-shadowing hole. (trigger: tasks/fibonacci_with_template.yaml) (restates: Axis 4: Grading command's import pytest resolves from the agent-writable sandbox cwd)
  • 🟡 Nothing in CI or pytest executes the two rewritten mocks, yet six hand-computed literals now live in task.yaml (MOCK_PATH_OK say_hello argc=1 argchars=5, ECHO_ARGS_OK argc=3 argchars=11, arg[3]=one, arg[3]=two, arg[5]=three). I verified they match today by running the scripts, but any edit to say_hello/echo_args (or to the prompt's argv) silently desynchronizes criteria from producers — pr-checks.yml only runs --tags smoke-pass|smoke-fail|smoke-variants, and these tasks are tagged mock, template-dir (and golden, basic for fibonacci), so neither is executed anywhere. Cheap fix: a unit test that sh-executes both mocks with the prompt's arguments and asserts every includes: string parsed out of task.yaml. (trigger: tasks/mock_path_dirs_template_dir/mock-cli-bins/mocks/echo_args)
  • 🟡 No test covers exclude_patterns × symlinks, and the interaction defeats the documented "exclusion is terminal" guarantee. Reproduced against Sandbox.setup() at PR state: a template containing oracle_abs -> <template>/grading/expected.json with exclude_patterns=["grading", "grading/*"] yields a sandbox containing oracle_abs whose read_text() returns the excluded oracle (the symlink branch at sandbox.py:376-408 copies os.readlink verbatim, by design for host passthrough). The relative-link variant merely dangles, so only the absolute form leaks — but nothing warns, tests, or documents it. (trigger: tests/test_sandbox_templates.py) (restates: Axis 4: exclude_patterns has no subtree-prune semantics)
  • 🟡 The four new tests only exercise pattern shapes that happen to work: no test for the bare-directory form exclude_patterns=["grading"] (which leaks the whole subtree), none for the exact pattern list the new docs example teaches (**/*.expected, which misses top-level files), and none for exclude_patterns combined with a non-default mount_point. Pattern semantics that decide whether grading material reaches the agent should be pinned by assertion, not by prose. (trigger: tests/test_sandbox_templates.py) (restates: Axis 3: exclude_patterns test coverage gaps (directory-only pattern, mount_point interaction))
  • 🟡 The new grading-surface table claims TASK_DIR is set for pre_run / post_run as well as run_command criteria, but test coverage stops at run_command (tests/test_sandbox.py:124 set / :142 absent). There is no test asserting a pre_run command can see $TASK_DIR — which is why the docs row could ship stating a contract Orchestrator._run_command_list does not implement. (trigger: docs/TASK_DEFINITION_GUIDE.md) _(restates: Axis 7: Docs claim TASK_DIR is set for pre_run / post_run, but run_command_list spawns them with no env=)
  • 🔵 The new criterion's comment justifies its python -c shape by cross-shell portability ("behaves identically under sh and cmd.exe"), but the Windows CI job runs only tasks/hello_date.yaml (pr-checks.yml:306), so the claim is never exercised. Either add fibonacci_with_template to the Windows smoke step or soften the comment to "intended to". (trigger: tasks/fibonacci_with_template.yaml)

Display & mapping dicts:

  • 🟡 Exclusion is invisible on every output surface: the continue at sandbox.py:361 emits no log line, no warning when an exclude pattern matches zero paths, and no copy manifest in run.json / the report — compare the adjacent overwrite logging at sandbox.py:419-424. For a safety-relevant, fail-open filter this means a typo'd pattern (grading/ vs grading/*, or the docs' own **/*.expected) is indistinguishable from a working one: the task still runs green while the oracle sits in the sandbox. Add at least a debug log of withheld paths plus a warning for patterns that matched nothing. (trigger: src/coder_eval/sandbox.py)

Downstream consumers:

  • 🔵 tasks/README.md was not updated alongside the reshaped task: rows 29 and 65 still describe mock_path_dirs_template_dir/ as a "Template dir (mock CLI bins) consumed by mock_path_dirs_smoke", when it is a standalone task with its own criteria (and mock_path_dirs_smoke uses inline starter_files). The index that points authors at the worked examples now mis-describes the one this PR made canonical. (trigger: tasks/mock_path_dirs_template_dir/task.yaml)
  • 🔵 exclude_patterns is a new user-authored YAML field, but TemplateDirSource is outside the set CE030's doc-schema-parity lint tracks (TaskDefinition, RunLimits, Dataset, SimulationConfig — tests/lint/doc_schema_parity.py), so the docs update here is voluntary and the next template-source field can ship undocumented with make lint green. Extending the tracked-model list to the template-source models is the mechanical guard. (trigger: src/coder_eval/models/templates.py)

Daily/nightly:

  • 🟡 The PR states no docker-driver blast radius for the new host-side grading convention. It works in-container only because docker_runner mounts the task dir (:1176) and every TemplateDirSource.path (:1234-1237) at identical host paths, so TASK_DIR.parent/templates/fibonacci-starter happens to resolve; fibonacci_with_template.yaml pins driver: tempdir, so that coupling is never exercised, and the natural fix for the containment finding (stop auto-mounting a source that declares exclude_patterns) would break exactly this pattern. State the interaction and add a docker-driver run of the task to the nightly/CI matrix before recommending the convention to task authors. (trigger: tasks/fibonacci_with_template.yaml)
  • 🔵 The documented convention assumes TASK_DIR is always present, but Sandbox._build_run_command_env sets it only when task_dir is not None (sandbox.py:985-986), i.e. only when an Orchestrator was constructed with a task_file. Any consumer that drives Orchestrator programmatically (cross-repo eval-runner / coder-eval-uipath-style embedding) gets os.environ['TASK_DIR']KeyError and a traceback in place of a grading verdict. Docs should say the convention is CLI-entry-point-only, and the example should use os.environ.get(...) with an explicit error. (trigger: docs/TASK_DEFINITION_GUIDE.md)

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] CE032 — template glob patterns must be fnmatch-honest. Promote TemplateDirSource._validate_patterns (src/coder_eval/models/templates.py:81-96) to a module-level validate_template_pattern(field, pattern) (the model keeps calling it), and have it additionally reject the two shapes that silently fail under fnmatch: (a) any entry containing ** (fnmatch has no recursive-glob semantics — fnmatch.translate('**/*.expected') still requires a literal /, so the correct recursive form is plain *.expected); (b) a wildcard-free entry that names a directory, which withholds only the directory entry and not its contents. Then add a whole-tree lint test class TestCE032TemplateGlobPatterns in tests/test_custom_lint.py (CE027/CE029/CE030 precedent — it scans Markdown/YAML, so it is not a BaseRule in tests/lint/runner.py), reusing tests/lint/doc_examples.extract_yaml_blocks to walk every yaml fence in docs/**/*.md including fragments (CE029 skips fragments today, which is exactly why docs:594 shipped), plus tasks/**/*.yaml, experiments/**/*.yaml, and include_patterns=/exclude_patterns= list literals under tests/, running each entry through the shared validator. Add a <!-- lint-skip: template-glob --> escape mirroring CE029's SKIP_MARKER. Prevents: F1 (docs/TASK_DEFINITION_GUIDE.md:594 exclude_patterns: ["grading", "grading/*", "**/*.expected"], which leaks every template-root *.expected grading oracle); the authoring half of F5 (a bare grading entry withholds only the directory entry while grading/expected.json and grading/fixtures/oracle.txt still copy in); and the propagated ** idiom at tests/test_sandbox_templates.py:266, 319, 380 (tools/*/dist/**) that teaches the same wrong mental model.
  • [ce-lint] CE033 — a grading command must not let the agent-writable sandbox cwd shadow its imports. New whole-tree lint test scanning every task-authored command string (success_criteria[].command for type: run_command, plus pre_run[].command / post_run[].command) in tasks/**/*.yaml and in docs yaml fences; flag any command invoking python/python3 with -c or -m that lacks -P/-I (or PYTHONSAFEPATH=1 in the same string). Escalate when the command also mentions pytest: require PYTEST_DISABLE_PLUGIN_AUTOLOAD=1, since _build_run_command_env prepends the sandbox venv's scripts dir, so a pytest11 entry point the agent pip-installs into <sandbox>/.venv is autoloaded into the grading process. Rule docstring rationale: Sandbox.run_command executes with cwd=self.sandbox_dir (src/coder_eval/sandbox.py:1079) and python -c prepends '' to sys.path, so any import in a grading command resolves from a directory the agent fully controls. Prevents: F4 (tasks/fibonacci_with_template.yaml:42-44 — a 5-line agent-authored <sandbox>/pytest.py defining def main(args=None): return 0 makes the criterion exit 0 against a deliberately wrong src/main.py, defeating that file's own "edits to the sandbox copy cannot move the bar" comment at lines 29-32). Also pre-empts the same shadow in the skillsbench verifier tasks that shell out to python.
  • [ce-lint] Extend CE015 (tests/lint/rules/ce015_create_subprocess_limit.py) to also require an explicit env= kwarg on asyncio.create_subprocess_exec / create_subprocess_shell, renaming it to "explicit stream limit and explicit environment". There are exactly two spawn sites in src/ (orchestrator.py, docker_runner.py) and the rule already visits both, so the churn is two lines. Effect: every spawn of a task-authored command must state whether it receives the sandbox environment (Sandbox._build_run_command_env, the sole assigner of TASK_DIR at sandbox.py:986) or deliberately inherits os.environ (with # noqa: CE015 plus a reason). Env inheritance stops being an invisible default that documentation can drift away from. Prevents: F6 (docs/TASK_DEFINITION_GUIDE.md:664 documents TASK_DIR as "set for every command" for pre_run/post_run, but Orchestrator._run_command_list spawns them at orchestrator.py:2165-2170 with no env=; under sh the unset $TASK_DIR expands to empty, so a documented "$TASK_DIR/verifier/check.py" becomes /verifier/check.py — and since PreRunCommand.fail_on_error defaults True, the run silently lands as FinalStatus.ERROR with a misleading reason).
  • [ce-lint] CE034 — a documented isolation/containment mechanism must be emitted by the code that would implement it. Direct sibling of CE027's doc↔code env-var parity, implemented as tests/lint/doc_mechanism_parity.py + TestCE034DocContainmentParity. Keep a small explicit term→evidence map (e.g. "UID/GID isolation" / "runs as a non-root user" → requires --user or --userns in the argv built by src/coder_eval/isolation/docker_runner.py, or a USER directive in docker/Dockerfile; "host paths … unreachable" → requires that docker_runner does not bind-mount the task dir or template dirs). Fail when a doc asserts a term whose evidence token is absent from src/. A small explicit registry keeps false positives near zero, exactly as CE027 does. Prevents: F7 (docs/TASK_DEFINITION_GUIDE.md:689 credits "the docker driver's UID/GID isolation, which makes host paths genuinely unreachable" — git grep -nE '\"--user\"|--user=|userns' over src/ and docker/ returns zero hits and the Dockerfile has no USER, while docker_runner.py:1176 and :1226/1234-1237 bind-mount the task dir and every TemplateDirSource.path read-only at their identical host paths, unfiltered by exclude_patterns). Also guards the related false claim at docs:660 ("the agent has no path to it").
  • [ce-lint] CE035 — a user-facing Field(description=...) must not dead-end on a private symbol. A per-file BaseRule in tests/lint/rules/ce035_field_description_no_private_ref.py, wired into ALL_RULES in tests/lint/runner.py: for any Field(...) call inside a BaseModel subclass under src/coder_eval/models/, flag a description= string literal containing a backticked _-prefixed identifier (regex `_\w+`). Field descriptions are the JSON-schema surface a task author reads in YAML tooling; a pointer to a private, non-importable, undocumented Sandbox method is unresolvable for them. Suggested fix shape in the message: state the fact ("matched with fnmatch, so * crosses / and ** has no special meaning") or link the doc anchor. Prevents: F8 (src/coder_eval/models/templates.py:66 see \_matches_template_exclude_pattern`on the new publicexclude_patternsfield, plus the pre-existinginclude_patternsinstance at line 56 that this PR replicated rather than fixed). Second-order: that private pointer is precisely what made the**` / bare-directory semantics behind F1 and F5 unlearnable from the schema.
  • [pyright] Tighten the validator-message idiom so the existing pyright gate does the work: annotate the local as str. In TemplateDirSource._validate_patterns (src/coder_eval/models/templates.py:85) write field: str = info.field_name or "pattern" instead of field = info.field_name. ValidationInfo.field_name is str | None; with the explicit annotation, any revert to the bare optional becomes a pyright error (str | None not assignable to str) in the existing standard-mode run — no new rule needed. If the idiom recurs across validators, promote it to a one-line BaseRule (next free number after CE035) flagging info.field_name reaching an f-string or ValueError message without an or fallback or a str-annotated binding. Record the boundary explicitly: neither ruff nor pyright flags an optional interpolated into an f-string (f-strings accept object), so the annotation is the check. Prevents: F3 ({field} entries must not be empty / … must be relative at templates.py:88/90/92 can render as None entries must not be empty; the validator tests at tests/test_sandbox_templates.py:331 and :418 match only the suffix, so a regression to None ships green — pair the annotation with tightening one assertion per field to match="exclude_patterns entries must be relative").
  • [bandit-codeql] Custom bandit plugin / CodeQL query: flag a subprocess spawned with the full parent environment into an attacker-writable working directory. Pattern: a subprocess.* / asyncio.create_subprocess_* call whose env derives from os.environ (directly or via a helper such as Sandbox._build_run_command_env, whose own comment at sandbox.py:941 says it inherits the parent env "so agent tools / credentials remain reachable") and whose cwd is a sandbox path. Report it as an information-flow sink so the credential exposure of the grading path is a reviewed, annotated decision; the remediation the query text should suggest is an allowlisted env projection for grading commands (PATH + TASK_DIR + explicitly named vars) instead of wholesale inheritance. Prevents: The escalation half of F4 — once an agent-authored pytest.py (or sitecustomize.py, or conftest.py) executes inside the grading process, it runs with the evaluator's inherited credentials, turning a scoring-integrity bug into credential exposure (the finding's scope-change S:C vector). Also covers agent_judge, which the criterion docstring already flags as running with evaluator credentials.

Harness improvements (not statically reachable):

  • Sandbox manifest, materialized and pinned. Add coder-eval plan --print-sandbox-manifest <task> that runs the real template resolution (ignore_patterns → include_patterns → exclude_patterns → mount_point) over the actual template trees and prints the exact sorted path list the agent will receive; then add a snapshot test asserting the manifest for every in-repo task declaring a template_dir with include_patterns/exclude_patterns. Any change to matcher semantics or to a task's patterns must be re-approved as a visible diff of agent-reachable files. Why not static: The leak is emergent from the interaction of template_path.rglob("*") (sandbox.py:354) enumerating descendants independently of their parent, the per-entry continue at sandbox.py:361, and dest_path.parent.mkdir(parents=True, exist_ok=True) at sandbox.py:418 recreating the excluded directory. No single source token is wrong — only the resulting file set is, and producing it requires walking a real tree. Prevents: F5 (exclude_patterns=['grading'] alone yields grading/expected.json and grading/fixtures/oracle.txt inside the sandbox, fail-open with no warning); F2 (the manifest shows mock-cli-bins/README.md, carrying the mocks' composition rule, landing in the agent's working copy of tasks/mock_path_dirs_template_dir); F1's downstream effect.
  • Authoring-shape matrix test for include/exclude patterns. Parametrize one test over the shapes a task author actually writes — grading, grading/, grading/*, grading/**, *.expected, **/*.expected, tools/*/dist — against a fixture tree containing both a root-level and a nested oracle, and assert the exact resulting sandbox file set for each. Pin today's behavior explicitly (including the shapes that leak, if subtree pruning is not implemented) so the semantics live in assertions rather than in a docstring. Why not static: fnmatch semantics only manifest against a concrete path set; whether **/*.expected withholds report.expected is a runtime property of the copy loop, not of the pattern string in isolation. Prevents: The Test-Health finding (all four new tests at tests/test_sandbox_templates.py:353/381/407/423 use the belt-and-braces ["grading", "grading/*"] form, masking rather than covering the defect; no test exercises the bare-directory form or any ** pattern); F5; F1.
  • Grader-tamper regression suite. For each in-repo task with a run_command grading criterion, run the criterion twice against a sandbox seeded with a knowingly-wrong solution: once clean (must fail) and once with hostile shims dropped in the sandbox — pytest.py, conftest.py, sitecustomize.py, and a fake pytest11 entry point installed into the sandbox venv. The criterion must fail in both. Make this the standing acceptance test for any task that grades against a pristine host copy. Why not static: Whether a shim actually wins depends on the interpreter's resolved sys.path[0], the venv on PATH from _build_run_command_env, and pytest's plugin autoload — all runtime resolution. A lint rule can require -P (CE033); only execution proves the grader is genuinely unshadowable. Prevents: F4, including the variants CE033's flag-check cannot see (a conftest.py in the graded rootdir, or a plugin autoloaded from the sandbox venv).
  • Cross-driver reachability parity test (tempdir vs docker). One test that, for a task declaring exclude_patterns, asserts the set of paths the agent process can read is identical under both drivers — no excluded template path re-exposed by a bind mount, and the task dir unreadable where the docs say it is. Pair with a code-side guard that refuses (or loudly warns) when docker_runner._auto_mount would mount a TemplateDirSource that declares exclude_patterns, since the filtering applies only to the copy. Why not static: The exposure is the composition of a filtered copy (sandbox.py:361) with an unfiltered read-only bind mount at the identical host path (docker_runner.py:1226 / 1234-1237) — it exists only in the running container's filesystem view, not in either source site alone. Prevents: F7 (the docs point readers at docker as the stronger boundary precisely where exclude_patterns is honoured least); the driver-dependent half of F5.
  • A TASK_DIR contract smoke task in CI. A tiny in-repo task that reads $TASK_DIR from a pre_run command, a post_run command, and a run_command criterion, each asserting the variable is non-empty and resolves to a real host file, wired into the existing CI task-smoke job. It either proves the documented contract on all three surfaces or fails the moment one diverges. Why not static: The failure mode is shell expansion of an unset variable to the empty string — no exception, no log line, just a wrong path. Nothing in the source text is detectably wrong; only spawning the shell and observing the expansion reveals it. Prevents: F6 (documented at docs:664 for pre_run/post_run, implemented only for Sandbox.run_command). Corroborating signal the test would have made loud: every existing $TASK_DIR consumer in the repo is a run_command criterion, and the skillsbench pre_run blocks all use $PWD instead.
  • Make doc examples behaviorally true, not merely schema-valid. Extend tests/lint/doc_examples.py with a second mode for glob-bearing fragments: when a docs yaml fence contains include_patterns/exclude_patterns, execute it against a small fixture tree and assert the outcome the surrounding prose claims ("the grading oracle stays on the host"). CE029 today only checks that a complete task/experiment document parses, which is why a schema-valid but semantically broken example shipped. Why not static: The doc's claim is behavioral ("the oracle stays on the host"), not structural; verifying it requires running the copy path. CE032 catches the specific ** token; this catches the general case of an example whose stated effect and real effect diverge. Prevents: F1, and the class it belongs to — a documented pattern list whose prose promise the code does not keep.
  • Answer-leak scan as a coder-eval plan warning. Cross-check every literal a task's criteria assert (file_contains.contains, plain-string file_matches_regex.pattern, classification_match expected labels, reference.file contents) against the materialized sandbox manifest from item 1, and warn when an asserted literal appears verbatim in a file that ships into the sandbox. Add a task-authoring checklist line: anti-gaming rationale belongs in the task YAML comment (never copied), not in assets that ship — and prefer withholding such assets with exclude_patterns over restating the rationale inside them. Why not static: Needs the materialized file set (the manifest) plus the resolved criterion literals after dataset ${row.*} substitution — neither is available from a single file's AST or from the YAML text alone. Prevents: The shipped-into-sandbox half of F2 (mock-cli-bins/README.md:22-25 states the receipt composition rule inside the agent's working copy) and the general class of tasks that ship their own answer key. The duplicated-prose half of F2 (four paraphrases of one rationale across mocks/say_hello, mocks/echo_args, README.md, task.yaml) is judgment, not mechanically detectable — the checklist line is the honest fallback there.

Top 5 Priority Actions

  1. Close the grader import shadow in tasks/fibonacci_with_template.yaml:42 — python -c puts the agent-writable sandbox cwd at sys.path[0], so a 5-line pytest.py stub makes the run_command criterion exit 0 with a wrong solution (verified); switch to python -P -B -c "import os, sys; sys.path.append(os.getcwd()); import pytest; ..." and set PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 so an agent-installed pytest11 plugin cannot force a pass either.
  2. Make exclude_patterns prune subtrees in src/coder_eval/sandbox.py:361 — a bare exclude_patterns: ['grading'] (the natural authoring shape, and the one every ignore tool prunes) skips only the directory entry while rglob still copies grading/expected.json and line 418 recreates the directory, leaking grading oracles into the agent's working copy with no warning; match rel_path plus every ancestor against the exclude list.
  3. Fix the TASK_DIR claim at docs/TASK_DEFINITION_GUIDE.md:664 — _run_command_list spawns pre_run/post_run with no env= (src/coder_eval/orchestrator.py:2165-2170), so "$TASK_DIR/verifier/check.py" expands to /verifier/check.py and, since pre_run.fail_on_error defaults True, silently aborts the run as FinalStatus.ERROR; either scope the row to run_command criteria or plumb _build_run_command_env() into _run_command_list.
  4. Correct the broken worked example at docs/TASK_DEFINITION_GUIDE.md:594 — under the fnmatch semantics stated ten lines above at :584, **/*.expected requires a literal / and therefore misses every top-level *.expected oracle (plain *.expected is the correct recursive form), and add the two missing tests to tests/test_sandbox_templates.py (bare-directory exclude, and the exact documented pattern list over a fixture with both top-level and nested .expected files) so pattern semantics that decide oracle exposure are pinned by assertion rather than prose.
  5. Scope or retract the containment claim at docs/TASK_DEFINITION_GUIDE.md:689 — the docker driver bind-mounts the task dir (src/coder_eval/isolation/docker_runner.py:1176) and every unfiltered template_dir (docker_runner.py:1226/1234-1237) read-only at identical host paths, and ships no --user/USER UID-GID isolation at all, so excluded grading files stay readable in-container; either state that docker isolates writes not reads, or stop auto-mounting a template source that declares exclude_patterns.

Stats: 0 🔴 · 4 🟠 · 3 🟡 · 2 🔵 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